Merge pull request #4334 from overleaf/jel-check-institution-users

Change check_institution_users.js output

GitOrigin-RevId: c331d5312dc807fd5118f4ce78737bde04a82c66

services/web/app/src/Features/Institutions/InstitutionsAPI.js

@@ -22,6 +22,17 @@ const InstitutionsAPI = {
     )
   },
 
+  getInstitutionAffiliationsCounts(institutionId, callback) {
+    makeAffiliationRequest(
+      {
+        method: 'GET',
+        path: `/api/v2/institutions/${institutionId.toString()}/affiliations_counts`,
+        defaultErrorMessage: "Couldn't get institution counts",
+      },
+      (error, body) => callback(error, body || [])
+    )
+  },
+
   getLicencesForAnalytics(lag, queryDate, callback) {
     makeAffiliationRequest(
       {

services/web/app/src/Features/Institutions/InstitutionsManager.js

@@ -1,18 +1,198 @@
 const async = require('async')
 const _ = require('underscore')
+const { callbackify } = require('util')
 const { ObjectId } = require('mongodb')
-const { getInstitutionAffiliations } = require('./InstitutionsAPI')
+const Settings = require('@overleaf/settings')
+const {
+  getInstitutionAffiliations,
+  promises: InstitutionsAPIPromises,
+} = require('./InstitutionsAPI')
 const FeaturesUpdater = require('../Subscription/FeaturesUpdater')
 const UserGetter = require('../User/UserGetter')
-const SAMLIdentityManager = require('../User/SAMLIdentityManager')
 const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
 const SubscriptionLocator = require('../Subscription/SubscriptionLocator')
 const { Institution } = require('../../models/Institution')
 const { Subscription } = require('../../models/Subscription')
-const OError = require('@overleaf/o-error')
 
 const ASYNC_LIMIT = parseInt(process.env.ASYNC_LIMIT, 10) || 5
-module.exports = {
+
+async function _getSsoUsers(institutionId, lapsedUserIds) {
+  let currentNotEntitledCount = 0
+  const ssoNonEntitledUsersIds = []
+  const allSsoUsersByIds = {}
+
+  const allSsoUsers = await UserGetter.promises.getSsoUsersAtInstitution(
+    institutionId,
+    { samlIdentifiers: 1 }
+  )
+  allSsoUsers.forEach(user => {
+    allSsoUsersByIds[user._id] = user.samlIdentifiers.find(
+      identifer => identifer.providerId === institutionId.toString()
+    )
+  })
+  for (const userId in allSsoUsersByIds) {
+    if (!allSsoUsersByIds[userId].hasEntitlement) {
+      ssoNonEntitledUsersIds.push(userId)
+    }
+  }
+  if (ssoNonEntitledUsersIds.length > 0) {
+    currentNotEntitledCount = ssoNonEntitledUsersIds.filter(
+      id => !lapsedUserIds.includes(id)
+    ).length
+  }
+
+  return {
+    allSsoUsers,
+    allSsoUsersByIds,
+    currentNotEntitledCount,
+  }
+}
+
+async function _checkUsersFeatures(userIds) {
+  const users = await UserGetter.promises.getUsers(userIds, { features: 1 })
+  const result = {
+    proUserIds: [],
+    nonProUserIds: [],
+  }
+
+  await new Promise((resolve, reject) => {
+    async.eachLimit(
+      users,
+      ASYNC_LIMIT,
+      (user, callback) => {
+        const hasProFeaturesOrBetter = FeaturesUpdater.isFeatureSetBetter(
+          user.features,
+          Settings.features.professional
+        )
+
+        if (hasProFeaturesOrBetter) {
+          result.proUserIds.push(user._id)
+        } else {
+          result.nonProUserIds.push(user._id)
+        }
+        callback()
+      },
+      error => {
+        if (error) return reject(error)
+        resolve()
+      }
+    )
+  })
+  return result
+}
+
+async function checkInstitutionUsers(institutionId) {
+  /*
+    v1 has affiliation data. Via getInstitutionAffiliationsCounts, v1 will send
+    lapsed_user_ids, which includes all user types
+    (not linked, linked and entitled, linked not entitled).
+    However, for SSO institutions, it does not know which email is linked
+    to SSO when the license is non-trivial. Here we need to split that
+    lapsed count into SSO (entitled and not) or just email users
+  */
+
+  const result = {
+    emailUsers: {
+      total: 0, // v1 all users - v2 all SSO users
+      current: 0, // v1 current - v1 SSO entitled - (v2 calculated not entitled current)
+      lapsed: 0, // v1 lapsed user IDs that are not in v2 SSO users
+      pro: {
+        current: 0,
+        lapsed: 0,
+      },
+      nonPro: {
+        current: 0,
+        lapsed: 0,
+      },
+    },
+    ssoUsers: {
+      total: 0, // only v2
+      current: {
+        entitled: 0, // only v1
+        notEntitled: 0, // v2 non-entitled SSO users - v1 lapsed user IDs
+      },
+      lapsed: 0, // v2 SSO users that are in v1 lapsed user IDs
+      pro: {
+        current: 0,
+        lapsed: 0,
+      },
+      nonPro: {
+        current: 0,
+        lapsed: 0,
+      },
+    },
+  }
+
+  const {
+    user_ids: userIds, // confirmed and not removed users. Includes users with lapsed reconfirmations
+    current_users_count: currentUsersCount, // all users not with lapsed reconfirmations
+    lapsed_user_ids: lapsedUserIds, // includes all user types that did not reconfirm (sso entitled, sso not entitled, email only)
+    with_confirmed_email: withConfirmedEmail, // same count as affiliation metrics
+    entitled_via_sso: entitled, // same count as affiliation metrics
+  } = await InstitutionsAPIPromises.getInstitutionAffiliationsCounts(
+    institutionId
+  )
+  result.ssoUsers.current.entitled = entitled
+
+  const {
+    allSsoUsers,
+    allSsoUsersByIds,
+    currentNotEntitledCount,
+  } = await _getSsoUsers(institutionId, lapsedUserIds)
+  result.ssoUsers.total = allSsoUsers.length
+  result.ssoUsers.current.notEntitled = currentNotEntitledCount
+
+  // check if lapsed user ID an SSO user
+  const lapsedUsersByIds = {}
+  lapsedUserIds.forEach(id => {
+    lapsedUsersByIds[id] = true // create a map for more performant lookups
+    if (allSsoUsersByIds[id]) {
+      ++result.ssoUsers.lapsed
+    } else {
+      ++result.emailUsers.lapsed
+    }
+  })
+
+  result.emailUsers.current =
+    currentUsersCount - entitled - result.ssoUsers.current.notEntitled
+  result.emailUsers.total = userIds.length - allSsoUsers.length
+
+  // compare v1 and v2 counts.
+  if (
+    result.ssoUsers.current.notEntitled + result.emailUsers.current !==
+    withConfirmedEmail
+  ) {
+    result.databaseMismatch = {
+      withConfirmedEmail: {
+        v1: withConfirmedEmail,
+        v2: result.ssoUsers.current.notEntitled + result.emailUsers.current,
+      },
+    }
+  }
+
+  // Add Pro/NonPro status for users
+  // NOTE: Users not entitled via institution could have Pro via another method
+  const { proUserIds, nonProUserIds } = await _checkUsersFeatures(userIds)
+  proUserIds.forEach(id => {
+    const userType = lapsedUsersByIds[id] ? 'lapsed' : 'current'
+    if (allSsoUsersByIds[id]) {
+      result.ssoUsers.pro[userType]++
+    } else {
+      result.emailUsers.pro[userType]++
+    }
+  })
+  nonProUserIds.forEach(id => {
+    const userType = lapsedUsersByIds[id] ? 'lapsed' : 'current'
+    if (allSsoUsersByIds[id]) {
+      result.ssoUsers.nonPro[userType]++
+    } else {
+      result.emailUsers.nonPro[userType]++
+    }
+  })
+  return result
+}
+
+const InstitutionsManager = {
   refreshInstitutionUsers(institutionId, notify, callback) {
     const refreshFunction = notify ? refreshFeaturesAndNotify : refreshFeatures
     async.waterfall(
@@ -33,21 +213,7 @@ module.exports = {
     )
   },
 
-  checkInstitutionUsers(institutionId, callback) {
-    getInstitutionAffiliations(institutionId, (error, affiliations) => {
-      if (error) {
-        return callback(error)
-      }
-      UserGetter.getUsersByAnyConfirmedEmail(
-        affiliations.map(affiliation => affiliation.email),
-        { features: 1, samlIdentifiers: 1 },
-        (error, users) => {
-          if (error) return callback(OError.tag(error))
-          callback(error, checkFeatures(institutionId, users))
-        }
-      )
-    })
-  },
+  checkInstitutionUsers: callbackify(checkInstitutionUsers),
 
   getInstitutionUsersSubscriptions(institutionId, callback) {
     getInstitutionAffiliations(institutionId, function (error, affiliations) {
@@ -153,46 +319,8 @@ var notifyUser = (user, affiliation, subscription, featuresChanged, callback) =>
     callback
   )
 
-var checkFeatures = function (institutionId, users) {
-  const usersSummary = {
-    confirmedEmailUsers: {
-      total: users.length, // all users are confirmed email users
-      totalProUsers: 0,
-      totalNonProUsers: 0,
-      nonProUsers: [],
-    },
-    entitledSSOUsers: {
-      total: 0,
-      totalProUsers: 0,
-      totalNonProUsers: 0,
-      nonProUsers: [],
-    },
-  }
-  users.forEach(function (user) {
-    const isSSOEntitled = SAMLIdentityManager.userHasEntitlement(
-      user,
-      institutionId
-    )
-
-    if (isSSOEntitled) {
-      usersSummary.entitledSSOUsers.total += 1
-    }
-
-    if (user.features.collaborators === -1 && user.features.trackChanges) {
-      // user is on Pro
-      usersSummary.confirmedEmailUsers.totalProUsers += 1
-      if (isSSOEntitled) {
-        usersSummary.entitledSSOUsers.totalProUsers += 1
-      }
-    } else {
-      // user is not on Pro
-      usersSummary.confirmedEmailUsers.totalNonProUsers += 1
-      usersSummary.confirmedEmailUsers.nonProUsers.push(user._id)
-      if (isSSOEntitled) {
-        usersSummary.entitledSSOUsers.totalNonProUsers += 1
-        usersSummary.entitledSSOUsers.nonProUsers.push(user._id)
-      }
-    }
-  })
-  return usersSummary
+InstitutionsManager.promises = {
+  checkInstitutionUsers,
 }
+
+module.exports = InstitutionsManager

services/web/app/src/Features/User/UserGetter.js

@@ -11,6 +11,7 @@ const {
 const InstitutionsHelper = require('../Institutions/InstitutionsHelper')
 const Errors = require('../Errors/Errors')
 const Features = require('../../infrastructure/Features')
+const { User } = require('../../models/User')
 const { normalizeQuery, normalizeMultiQuery } = require('../Helpers/Mongo')
 
 function _lastDayToReconfirm(emailData, institutionData) {
@@ -81,7 +82,22 @@ async function getUserFullEmails(userId) {
   )
 }
 
+async function getSsoUsersAtInstitution(institutionId, projection) {
+  if (!projection) {
+    throw new Error('missing projection')
+  }
+
+  return await User.find(
+    {
+      'samlIdentifiers.providerId': institutionId.toString(),
+    },
+    projection
+  ).exec()
+}
+
 const UserGetter = {
+  getSsoUsersAtInstitution: callbackify(getSsoUsersAtInstitution),
+
   getUser(query, projection, callback) {
     if (arguments.length === 2) {
       callback = projection
@@ -253,8 +269,9 @@ var decorateFullEmails = (
 )
 
 UserGetter.promises = promisifyAll(UserGetter, {
-  without: ['getUserFullEmails'],
+  without: ['getSsoUsersAtInstitution', 'getUserFullEmails'],
 })
 UserGetter.promises.getUserFullEmails = getUserFullEmails
+UserGetter.promises.getSsoUsersAtInstitution = getSsoUsersAtInstitution
 
 module.exports = UserGetter

services/web/scripts/check_institution_users.js

@@ -12,16 +12,10 @@ waitForDb()
     process.exit(1)
   })
 
-function main() {
-  InstitutionsManager.checkInstitutionUsers(
-    institutionId,
-    function (error, usersSummary) {
-      if (error) {
-        console.log(error)
-      } else {
-        console.log(usersSummary)
-      }
-      process.exit()
-    }
+async function main() {
+  const usersSummary = await InstitutionsManager.promises.checkInstitutionUsers(
+    institutionId
   )
+  console.log(usersSummary)
+  process.exit()
 }

services/web/test/unit/src/Institutions/InstitutionsManagerTests.js

@@ -6,6 +6,7 @@ const modulePath = path.join(
   __dirname,
   '../../../../app/src/Features/Institutions/InstitutionsManager'
 )
+const Features = require('../../../../app/src/infrastructure/Features')
 
 describe('InstitutionsManager', function () {
   beforeEach(function () {
@@ -13,9 +14,43 @@ describe('InstitutionsManager', function () {
     this.user = {}
     this.getInstitutionAffiliations = sinon.stub()
     this.refreshFeatures = sinon.stub().yields()
+    this.users = [
+      { _id: 'lapsed', features: {} },
+      { _id: '1a', features: {} },
+      { _id: '2b', features: {} },
+      { _id: '3c', features: {} },
+    ]
+    this.ssoUsers = [
+      {
+        _id: '1a',
+        samlIdentifiers: [{ providerId: this.institutionId.toString() }],
+      },
+      {
+        _id: '2b',
+        samlIdentifiers: [
+          {
+            providerId: this.institutionId.toString(),
+            hasEntitlement: true,
+          },
+        ],
+      },
+      {
+        _id: 'lapsed',
+        samlIdentifiers: [{ providerId: this.institutionId.toString() }],
+        hasEntitlement: true,
+      },
+    ]
+
     this.UserGetter = {
       getUsersByAnyConfirmedEmail: sinon.stub().yields(),
       getUser: sinon.stub().callsArgWith(1, null, this.user),
+      promises: {
+        getUsers: sinon.stub().resolves(this.users),
+        getUsersByAnyConfirmedEmail: sinon.stub().resolves(),
+        getSsoUsersAtInstitution: (this.getSsoUsersAtInstitution = sinon
+          .stub()
+          .resolves(this.ssoUsers)),
+      },
     }
     this.creator = { create: sinon.stub().callsArg(0) }
     this.NotificationsBuilder = {
@@ -37,9 +72,6 @@ describe('InstitutionsManager', function () {
       },
     }
     this.subscriptionExec = sinon.stub().yields()
-    this.SAMLIdentityManager = {
-      userHasEntitlement: sinon.stub().returns(false),
-    }
     const SubscriptionModel = {
       Subscription: {
         find: () => ({
@@ -51,13 +83,30 @@ describe('InstitutionsManager', function () {
     }
     this.Mongo = { ObjectId: sinon.stub().returnsArg(0) }
 
+    this.v1Counts = {
+      user_ids: this.users.map(user => user._id),
+      current_users_count: 3,
+      lapsed_user_ids: ['lapsed'],
+      entitled_via_sso: 1, // 2 entitled, but 1 lapsed
+      with_confirmed_email: 2, // 1 non entitled SSO + 1 email user
+    }
+
     this.InstitutionsManager = SandboxedModule.require(modulePath, {
       requires: {
         './InstitutionsAPI': {
           getInstitutionAffiliations: this.getInstitutionAffiliations,
+          promises: {
+            getInstitutionAffiliations: (this.getInstitutionAffiliationsPromise = sinon
+              .stub()
+              .resolves(this.affiliations)),
+            getInstitutionAffiliationsCounts: (this.getInstitutionAffiliationsCounts = sinon
+              .stub()
+              .resolves(this.v1Counts)),
+          },
         },
         '../Subscription/FeaturesUpdater': {
           refreshFeatures: this.refreshFeatures,
+          isFeatureSetBetter: (this.isFeatureSetBetter = sinon.stub()),
         },
         '../User/UserGetter': this.UserGetter,
         '../Notifications/NotificationsBuilder': this.NotificationsBuilder,
@@ -65,7 +114,6 @@ describe('InstitutionsManager', function () {
         '../../models/Institution': this.InstitutionModel,
         '../../models/Subscription': SubscriptionModel,
         mongodb: this.Mongo,
-        '../User/SAMLIdentityManager': this.SAMLIdentityManager,
       },
     })
   })
@@ -154,49 +202,91 @@ describe('InstitutionsManager', function () {
   })
 
   describe('checkInstitutionUsers', function () {
-    it('check all users Features', function (done) {
-      const affiliations = [{ email: 'foo@bar.com' }, { email: 'baz@boo.edu' }]
-      const stubbedUsers = [
-        {
-          _id: '123abc123abc123abc123abc',
-          features: { collaborators: -1, trackChanges: true },
-        },
-        {
-          _id: '456def456def456def456def',
-          features: { collaborators: 10, trackChanges: false },
-        },
-        {
-          _id: '789def789def789def789def',
-          features: { collaborators: -1, trackChanges: false },
-        },
-      ]
-      this.getInstitutionAffiliations.yields(null, affiliations)
-      this.UserGetter.getUsersByAnyConfirmedEmail.yields(null, stubbedUsers)
-      this.SAMLIdentityManager.userHasEntitlement.onCall(0).returns(true)
-      this.SAMLIdentityManager.userHasEntitlement.onCall(1).returns(true)
-      this.SAMLIdentityManager.userHasEntitlement.onCall(2).returns(false)
-      this.InstitutionsManager.checkInstitutionUsers(
-        this.institutionId,
-        (error, usersSummary) => {
-          expect(error).not.to.exist
+    it('returns entitled/not, sso/not, lapsed/current, and pro counts', async function () {
+      if (Features.hasFeature('saas')) {
+        this.isFeatureSetBetter.returns(true)
+        const usersSummary = await this.InstitutionsManager.promises.checkInstitutionUsers(
+          this.institutionId
+        )
+        expect(usersSummary).to.deep.equal({
+          emailUsers: {
+            total: 1,
+            current: 1,
+            lapsed: 0,
+            pro: {
+              current: 1, // isFeatureSetBetter stubbed to return true for all
+              lapsed: 0,
+            },
+            nonPro: {
+              current: 0,
+              lapsed: 0,
+            },
+          },
+          ssoUsers: {
+            total: 3,
+            lapsed: 1,
+            current: {
+              entitled: 1,
+              notEntitled: 1,
+            },
+            pro: {
+              current: 2,
+              lapsed: 1, // isFeatureSetBetter stubbed to return true for all users
+            },
+            nonPro: {
+              current: 0,
+              lapsed: 0,
+            },
+          },
+        })
+      }
+    })
 
-          usersSummary.confirmedEmailUsers.total.should.equal(3)
-          usersSummary.confirmedEmailUsers.totalProUsers.should.equal(1)
-          usersSummary.confirmedEmailUsers.totalNonProUsers.should.equal(2)
-          expect(usersSummary.confirmedEmailUsers.nonProUsers).to.deep.equal([
-            '456def456def456def456def',
-            '789def789def789def789def',
-          ])
-
-          usersSummary.entitledSSOUsers.total.should.equal(2)
-          usersSummary.entitledSSOUsers.totalProUsers.should.equal(1)
-          usersSummary.entitledSSOUsers.totalNonProUsers.should.equal(1)
-          expect(usersSummary.entitledSSOUsers.nonProUsers).to.deep.equal([
-            '456def456def456def456def',
-          ])
-          done()
-        }
-      )
+    it('includes withConfirmedEmailMismatch when v1 and v2 counts do not add up', async function () {
+      if (Features.hasFeature('saas')) {
+        this.isFeatureSetBetter.returns(true)
+        this.v1Counts.with_confirmed_email = 100
+        const usersSummary = await this.InstitutionsManager.promises.checkInstitutionUsers(
+          this.institutionId
+        )
+        expect(usersSummary).to.deep.equal({
+          emailUsers: {
+            total: 1,
+            current: 1,
+            lapsed: 0,
+            pro: {
+              current: 1, // isFeatureSetBetter stubbed to return true for all
+              lapsed: 0,
+            },
+            nonPro: {
+              current: 0,
+              lapsed: 0,
+            },
+          },
+          ssoUsers: {
+            total: 3,
+            lapsed: 1,
+            current: {
+              entitled: 1,
+              notEntitled: 1,
+            },
+            pro: {
+              current: 2,
+              lapsed: 1, // isFeatureSetBetter stubbed to return true for all users
+            },
+            nonPro: {
+              current: 0,
+              lapsed: 0,
+            },
+          },
+          databaseMismatch: {
+            withConfirmedEmail: {
+              v1: 100,
+              v2: 2,
+            },
+          },
+        })
+      }
     })
   })
 

services/web/test/unit/src/User/UserGetterTests.js

@@ -61,10 +61,23 @@ describe('UserGetter', function () {
         '../../infrastructure/Features': {
           hasFeature: sinon.stub().returns(true),
         },
+        '../../models/User': {
+          User: (this.User = {}),
+        },
       },
     })
   })
 
+  describe('getSsoUsersAtInstitution', function () {
+    it('should throw an error when no projection is passed', function (done) {
+      this.UserGetter.getSsoUsersAtInstitution(1, undefined, error => {
+        expect(error).to.exist
+        expect(error.message).to.equal('missing projection')
+        done()
+      })
+    })
+  })
+
   describe('getUser', function () {
     it('should get user', function (done) {
       const query = { _id: '000000000000000000000000' }