From 9df283caef65501d1bf84f5abf60e9313a9ef531 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Tue, 27 Jul 2021 08:23:22 -0500 Subject: [PATCH] Merge pull request #4334 from overleaf/jel-check-institution-users Change check_institution_users.js output GitOrigin-RevId: c331d5312dc807fd5118f4ce78737bde04a82c66 --- .../Features/Institutions/InstitutionsAPI.js | 11 + .../Institutions/InstitutionsManager.js | 250 +++++++++++++----- .../web/app/src/Features/User/UserGetter.js | 19 +- .../web/scripts/check_institution_users.js | 16 +- .../Institutions/InstitutionsManagerTests.js | 182 +++++++++---- .../web/test/unit/src/User/UserGetterTests.js | 13 + 6 files changed, 372 insertions(+), 119 deletions(-) diff --git a/services/web/app/src/Features/Institutions/InstitutionsAPI.js b/services/web/app/src/Features/Institutions/InstitutionsAPI.js index e46288c7bc..81fc5e3906 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsAPI.js +++ b/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( { diff --git a/services/web/app/src/Features/Institutions/InstitutionsManager.js b/services/web/app/src/Features/Institutions/InstitutionsManager.js index 4c7d6a0e04..4318394a74 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsManager.js +++ b/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 diff --git a/services/web/app/src/Features/User/UserGetter.js b/services/web/app/src/Features/User/UserGetter.js index 0b1f4f39c9..6649459b0b 100644 --- a/services/web/app/src/Features/User/UserGetter.js +++ b/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 diff --git a/services/web/scripts/check_institution_users.js b/services/web/scripts/check_institution_users.js index 4a56fbd60a..fe808c4667 100644 --- a/services/web/scripts/check_institution_users.js +++ b/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() } diff --git a/services/web/test/unit/src/Institutions/InstitutionsManagerTests.js b/services/web/test/unit/src/Institutions/InstitutionsManagerTests.js index 47f007880d..a07134d375 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsManagerTests.js +++ b/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, + }, + }, + }) + } }) }) diff --git a/services/web/test/unit/src/User/UserGetterTests.js b/services/web/test/unit/src/User/UserGetterTests.js index a73d17a378..fffcc419fe 100644 --- a/services/web/test/unit/src/User/UserGetterTests.js +++ b/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' }