diff --git a/package-lock.json b/package-lock.json index 0f844f4620..9829427800 100644 --- a/package-lock.json +++ b/package-lock.json @@ -35052,6 +35052,7 @@ "multer": "https://github.com/overleaf/multer/archive/7a2928d7ea2da02dd92888ea1c9ba5704e07aeeb.tar.gz", "nocache": "^2.1.0", "nock": "^13.1.3", + "node-fetch": "^2.6.7", "nodemailer": "^6.7.0", "nodemailer-mandrill-transport": "^1.2.0", "nodemailer-ses-transport": "^1.5.1", @@ -35176,7 +35177,6 @@ "mocha": "^8.4.0", "mock-fs": "^5.1.2", "nock": "^13.1.1", - "node-fetch": "^2.6.7", "pirates": "^4.0.1", "postcss-loader": "^6.2.1", "requirejs": "^2.3.6", diff --git a/services/web/app/src/Features/User/SAMLIdentityManager.js b/services/web/app/src/Features/User/SAMLIdentityManager.js index 1fc2891cb4..fed1b1bd1d 100644 --- a/services/web/app/src/Features/User/SAMLIdentityManager.js +++ b/services/web/app/src/Features/User/SAMLIdentityManager.js @@ -10,26 +10,15 @@ const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') const logger = require('@overleaf/logger') const { User } = require('../../models/User') +const { promiseMapWithLimit } = require('../../util/promises') -async function _addAuditLogEntry( - link, - userId, - auditLog, - institutionEmail, - providerId, - providerName -) { - const operation = link ? 'link-institution-sso' : 'unlink-institution-sso' +async function _addAuditLogEntry(operation, userId, auditLog, extraInfo) { await UserAuditLogHandler.promises.addEntry( userId, operation, auditLog.initiatorId, auditLog.ipAddress, - { - institutionEmail, - providerId, - providerName, - } + extraInfo ) } @@ -95,19 +84,25 @@ async function _addIdentifier( hasEntitlement, institutionEmail, providerName, - auditLog + auditLog, + userIdAttribute ) { providerId = providerId.toString() await _ensureCanAddIdentifier(userId, institutionEmail, providerId) - await _addAuditLogEntry( - true, - userId, - auditLog, + const auditLogInfo = { institutionEmail, providerId, - providerName + providerName, + userIdAttribute, + } + + await _addAuditLogEntry( + 'link-institution-sso', + userId, + auditLog, + auditLogInfo ) hasEntitlement = !!hasEntitlement @@ -117,12 +112,14 @@ async function _addIdentifier( $ne: providerId, }, } + const update = { $push: { samlIdentifiers: { hasEntitlement, externalUserId, providerId, + userIdAttribute, }, }, } @@ -239,15 +236,16 @@ async function redundantSubscription(userId, providerId, providerName) { } } -async function linkAccounts( - userId, - externalUserId, - institutionEmail, - providerId, - providerName, - hasEntitlement, - auditLog -) { +async function linkAccounts(userId, samlData, auditLog) { + const { + externalUserId, + institutionEmail, + universityId: providerId, + universityName: providerName, + hasEntitlement, + userIdAttribute, + } = samlData + await _addIdentifier( userId, externalUserId, @@ -255,7 +253,8 @@ async function linkAccounts( hasEntitlement, institutionEmail, providerName, - auditLog + auditLog, + userIdAttribute ) try { await _addInstitutionEmail(userId, institutionEmail, providerId, auditLog) @@ -288,14 +287,11 @@ async function unlinkAccounts( ) { providerId = providerId.toString() - await _addAuditLogEntry( - false, - userId, - auditLog, + await _addAuditLogEntry('unlink-institution-sso', userId, auditLog, { institutionEmail, providerId, - providerName - ) + providerName, + }) // update v2 user await _removeIdentifier(userId, providerId) // update v1 affiliations record @@ -379,12 +375,96 @@ function userHasEntitlement(user, providerId) { return false } +async function migrateIdentifier( + userId, + externalUserId, + providerId, + hasEntitlement, + institutionEmail, + providerName, + auditLog, + userIdAttribute +) { + providerId = providerId.toString() + + const query = { + _id: userId, + 'samlIdentifiers.providerId': providerId, + } + + const update = { + $set: { + 'samlIdentifiers.$.externalUserId': externalUserId, + 'samlIdentifiers.$.userIdAttribute': userIdAttribute, + }, + } + await User.updateOne(query, update).exec() + + const auditLogInfo = { + institutionEmail, + providerId, + providerName, + userIdAttribute, + } + + await _addAuditLogEntry( + 'migrate-institution-sso-id', + userId, + auditLog, + auditLogInfo + ) +} + +async function unlinkNotMigrated(userId, providerId, providerName, auditLog) { + providerId = providerId.toString() + + const query = { + _id: userId, + 'emails.samlProviderId': providerId, + } + const update = { + $pull: { + samlIdentifiers: { + providerId, + }, + }, + $unset: { + 'emails.$.samlProviderId': 1, + }, + } + + const originalDoc = await User.findOneAndUpdate(query, update).exec() + + // should only be 1 + const linkedEmails = originalDoc.emails.filter(email => { + return email.samlProviderId === providerId + }) + + const auditLogInfo = { + providerId, + providerName, + } + + await _addAuditLogEntry( + 'unlink-institution-sso-not-migrated', + userId, + auditLog, + auditLogInfo + ) + + await promiseMapWithLimit(10, linkedEmails, async emailData => { + await InstitutionsAPI.promises.removeEntitlement(userId, emailData.email) + }) +} + const SAMLIdentityManager = { entitlementAttributeMatches, getUser, linkAccounts, + migrateIdentifier, redundantSubscription, unlinkAccounts, + unlinkNotMigrated, updateEntitlement, userHasEntitlement, } diff --git a/services/web/app/src/Features/User/UserAuditLogHandler.js b/services/web/app/src/Features/User/UserAuditLogHandler.js index 5ba1e3b8dd..d2f7f85670 100644 --- a/services/web/app/src/Features/User/UserAuditLogHandler.js +++ b/services/web/app/src/Features/User/UserAuditLogHandler.js @@ -8,6 +8,7 @@ function _canHaveNoInitiatorId(operation, info) { if (operation === 'reset-password') return true if (operation === 'unlink-sso' && info.providerId === 'collabratec') return true + if (operation === 'unlink-institution-sso-not-migrated') return true } /** diff --git a/services/web/package.json b/services/web/package.json index b2c89de2e8..ea9e148a6d 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -145,6 +145,7 @@ "multer": "https://github.com/overleaf/multer/archive/7a2928d7ea2da02dd92888ea1c9ba5704e07aeeb.tar.gz", "nocache": "^2.1.0", "nock": "^13.1.3", + "node-fetch": "^2.6.7", "nodemailer": "^6.7.0", "nodemailer-mandrill-transport": "^1.2.0", "nodemailer-ses-transport": "^1.5.1", @@ -269,7 +270,6 @@ "mocha": "^8.4.0", "mock-fs": "^5.1.2", "nock": "^13.1.1", - "node-fetch": "^2.6.7", "pirates": "^4.0.1", "postcss-loader": "^6.2.1", "requirejs": "^2.3.6", diff --git a/services/web/scripts/backfill_users_sso_attribute.js b/services/web/scripts/backfill_users_sso_attribute.js new file mode 100644 index 0000000000..365009da06 --- /dev/null +++ b/services/web/scripts/backfill_users_sso_attribute.js @@ -0,0 +1,38 @@ +const SAMLUserIdAttributeBatchHandler = require('../modules/overleaf-integration/app/src/SAML/SAMLUserIdAttributeBatchHandler') + +const COMMIT = process.argv.includes('--commit') +const startInstitutionId = parseInt(process.argv[2]) +const endInstitutionId = parseInt(process.argv[3]) + +process.env.LOG_LEVEL = 'info' + +let method = 'check' +if (COMMIT) { + method = 'run' + console.log('Setting attribute for linked users') +} else { + process.env.MONGO_CONNECTION_STRING = + process.env.READ_ONLY_MONGO_CONNECTION_STRING + console.log('Doing a dry run without --commit') + console.log('Checking users at institutions') +} + +console.log( + 'Start institution ID:', + startInstitutionId || + 'none provided, will start at beginning of ordered list.' +) +console.log( + 'End institution ID:', + endInstitutionId || 'none provided, will go to end of ordered list.' +) + +SAMLUserIdAttributeBatchHandler[method](startInstitutionId, endInstitutionId) + .then(result => { + console.log(result) + process.exit(0) + }) + .catch(error => { + console.error(error) + process.exit(1) + }) diff --git a/services/web/scripts/sso_id_migration_check.js b/services/web/scripts/sso_id_migration_check.js new file mode 100644 index 0000000000..f0ab47e8ea --- /dev/null +++ b/services/web/scripts/sso_id_migration_check.js @@ -0,0 +1,36 @@ +process.env.MONGO_CONNECTION_STRING = + process.env.READ_ONLY_MONGO_CONNECTION_STRING + +const { waitForDb } = require('../app/src/infrastructure/mongodb') +const SAMLUserIdMigrationHandler = require('../modules/overleaf-integration/app/src/SAML/SAMLUserIdMigrationHandler') + +const institutionId = parseInt(process.argv[2]) +if (isNaN(institutionId)) throw new Error('No institution id') +const emitUsers = process.argv.includes('--emit-users') + +console.log('Checking SSO user ID migration for institution:', institutionId) + +waitForDb() + .then(main) + .catch(error => { + console.error(error) + process.exit(1) + }) + +async function main() { + const result = await SAMLUserIdMigrationHandler.promises.checkMigration( + institutionId + ) + + if (emitUsers) { + console.log( + `\nMigrated: ${result.migrated}\nNot migrated: ${result.notMigrated}\nMultiple Identifers: ${result.multipleIdentifiers}` + ) + } + + console.log( + `\nMigrated: ${result.migrated.length}\nNot migrated: ${result.notMigrated.length}\nMultiple Identifers: ${result.multipleIdentifiers.length}` + ) + + process.exit() +} diff --git a/services/web/scripts/sso_id_remove_not_migrated.js b/services/web/scripts/sso_id_remove_not_migrated.js new file mode 100644 index 0000000000..f6924c8336 --- /dev/null +++ b/services/web/scripts/sso_id_remove_not_migrated.js @@ -0,0 +1,36 @@ +const { waitForDb } = require('../app/src/infrastructure/mongodb') +const SAMLUserIdMigrationHandler = require('../modules/overleaf-integration/app/src/SAML/SAMLUserIdMigrationHandler') + +const institutionId = parseInt(process.argv[2]) +if (isNaN(institutionId)) throw new Error('No institution id') +const emitUsers = process.argv.includes('--emit-users') + +console.log( + 'Remove SSO linking for users not migrated at institution:', + institutionId +) + +waitForDb() + .then(main) + .catch(error => { + console.error(error) + process.exit(1) + }) + +async function main() { + const result = await SAMLUserIdMigrationHandler.promises.removeNotMigrated( + institutionId + ) + + if (emitUsers) { + console.log( + `\nRemoved: ${result.success}\nFailed to remove: ${result.failed}` + ) + } + + console.log( + `\nRemoved: ${result.success.length}\nFailed to remove: ${result.failed.length}` + ) + + process.exit() +} diff --git a/services/web/test/unit/src/User/SAMLIdentityManagerTests.js b/services/web/test/unit/src/User/SAMLIdentityManagerTests.js index a24f48771d..0eeb1f0892 100644 --- a/services/web/test/unit/src/User/SAMLIdentityManagerTests.js +++ b/services/web/test/unit/src/User/SAMLIdentityManagerTests.js @@ -121,13 +121,7 @@ describe('SAMLIdentityManager', function () { it('should throw an error if missing data', async function () { let error try { - await this.SAMLIdentityManager.linkAccounts( - null, - null, - null, - null, - null - ) + await this.SAMLIdentityManager.linkAccounts(null, null, null) } catch (e) { error = e } finally { @@ -144,14 +138,17 @@ describe('SAMLIdentityManager', function () { it('should throw an EmailExistsError error', async function () { let error + try { await this.SAMLIdentityManager.linkAccounts( '6005c75b12cbcaf771f4a105', - 'not-linked-id', - 'exists@overleaf.com', - 'provider-id', - 'provider-name', - true, + { + externalUserId: 'not-linked-id', + institutionEmail: 'exists@overleaf.com', + universityId: 'provider-id', + universityName: 'provider-name', + hasEntitlement: true, + }, { intiatorId: '6005c75b12cbcaf771f4a105', ip: '0:0:0:0', @@ -181,11 +178,13 @@ describe('SAMLIdentityManager', function () { try { await this.SAMLIdentityManager.linkAccounts( '6005c75b12cbcaf771f4a105', - 'not-linked-id', - 'not-affiliated@overleaf.com', - 'provider-id', - 'provider-name', - true, + { + externalUserId: 'not-linked-id', + institutionEmail: 'not-affiliated@overleaf.com', + universityId: 'provider-id', + universityName: 'provider-name', + hasEntitlement: true, + }, { intiatorId: 'user-id-1', ip: '0:0:0:0', @@ -216,11 +215,13 @@ describe('SAMLIdentityManager', function () { try { await this.SAMLIdentityManager.linkAccounts( '6005c75b12cbcaf771f4a105', - 'not-linked-id', - 'affiliated@overleaf.com', - 'provider-id', - 'provider-name', - true, + { + externalUserId: 'not-linked-id', + institutionEmail: 'affiliated@overleaf.com', + universityId: 'provider-id', + universityName: 'provider-name', + hasEntitlement: true, + }, { intiatorId: 'user-id-1', ip: '0:0:0:0', @@ -249,11 +250,13 @@ describe('SAMLIdentityManager', function () { try { await this.SAMLIdentityManager.linkAccounts( '6005c75b12cbcaf771f4a105', - 'already-linked-id', - 'linked@overleaf.com', - 'provider-id', - 'provider-name', - true, + { + externalUserId: 'already-linked-id', + institutionEmail: 'linked@overleaf.com', + universityId: 'provider-id', + universityName: 'provider-name', + hasEntitlement: true, + }, { intiatorId: '6005c75b12cbcaf771f4a105', ip: '0:0:0:0', @@ -279,11 +282,13 @@ describe('SAMLIdentityManager', function () { try { await this.SAMLIdentityManager.linkAccounts( '6005c75b12cbcaf771f4a105', - 'already-linked-id', - 'linked@overleaf.com', - 123456, - 'provider-name', - true, + { + externalUserId: 'already-linked-id', + institutionEmail: 'linked@overleaf.com', + universityId: 123456, + universityName: 'provider-name', + hasEntitlement: true, + }, { intiatorId: '6005c75b12cbcaf771f4a105', ip: '0:0:0:0', @@ -311,11 +316,13 @@ describe('SAMLIdentityManager', function () { try { await this.SAMLIdentityManager.linkAccounts( this.user._id, - 'externalUserId', - this.user.email, - '1', - 'Overleaf University', - undefined, + { + externalUserId: 'externalUserId', + institutionEmail: this.user.email, + universityId: '1', + universityName: 'Overleaf University', + hasEntitlement: false, + }, { intiatorId: '6005c75b12cbcaf771f4a105', ipAddress: '0:0:0:0', @@ -346,11 +353,14 @@ describe('SAMLIdentityManager', function () { } await this.SAMLIdentityManager.linkAccounts( this.user._id, - 'externalUserId', - this.user.email, - '1', - 'Overleaf University', - undefined, + { + externalUserId: 'externalUserId', + institutionEmail: this.user.email, + universityId: '1', + universityName: 'Overleaf University', + hasEntitlement: false, + userIdAttribute: 'uniqueId', + }, auditLog ) @@ -365,6 +375,7 @@ describe('SAMLIdentityManager', function () { institutionEmail: this.user.email, providerId: '1', providerName: 'Overleaf University', + userIdAttribute: 'uniqueId', } ) }) @@ -372,11 +383,13 @@ describe('SAMLIdentityManager', function () { it('should send an email notification', async function () { await this.SAMLIdentityManager.linkAccounts( this.user._id, - 'externalUserId', - this.user.email, - '1', - 'Overleaf University', - undefined, + { + externalUserId: 'externalUserId', + institutionEmail: this.user.email, + universityId: '1', + universityName: 'Overleaf University', + hasEntitlement: false, + }, { intiatorId: '6005c75b12cbcaf771f4a105', ipAddress: '0:0:0:0', @@ -602,4 +615,123 @@ describe('SAMLIdentityManager', function () { }) }) }) + + describe('migrateIdentifier', function () { + const userId = '5efb8b6e9b647b0027e4c0b0' + const externalUserId = '987zyx' + const providerId = 123 + const hasEntitlement = false + const institutionEmail = 'someone@email.com' + const providerName = 'Example University' + const auditLog = { + initiatorId: userId, + ipAddress: '0.0.0.0', + migration: { + from: 'uniqueId', + to: 'newUniqueId', + }, + } + const userIdAttribute = 'newUniqueId' + + it('should remove the old identifier and add the new identifier', async function () { + this.UserGetter.promises.getUser.resolves() + this.UserGetter.promises.getUserByAnyEmail + .withArgs(institutionEmail) + .resolves({ _id: userId, emails: [{ email: institutionEmail }] }) + this.UserGetter.promises.getUserFullEmails.withArgs(userId).resolves([ + { + email: institutionEmail, + affiliation: { institution: { id: providerId } }, + }, + ]) + await this.SAMLIdentityManager.migrateIdentifier( + userId, + externalUserId, + providerId, + hasEntitlement, + institutionEmail, + providerName, + auditLog, + userIdAttribute + ) + + expect(this.User.updateOne).to.have.been.calledOnce + const query = { + _id: userId, + 'samlIdentifiers.providerId': providerId.toString(), + } + + const update = { + $set: { + 'samlIdentifiers.$.externalUserId': externalUserId, + 'samlIdentifiers.$.userIdAttribute': userIdAttribute, + }, + } + + expect(this.User.updateOne.lastCall.args).to.deep.equal([query, update]) + }) + }) + + describe('unlinkNotMigrated', function () { + const userId = '5efb8b6e9b647b0027e4c0b0' + const providerId = '123' + const institutionEmail = 'someone@email.com' + const providerName = 'Example University' + const auditLog = { + ipAddress: 'N/A', + } + + it('should remove the identifier om samlIdentifiers and samlProviderId on the email', async function () { + this.User.findOneAndUpdate = sinon.stub().returns({ + exec: sinon.stub().resolves({ + _id: userId, + emails: [{ email: institutionEmail, samlProviderId: providerId }], + }), + }) + + await this.SAMLIdentityManager.unlinkNotMigrated( + userId, + providerId, + providerName, + auditLog + ) + + expect(this.User.findOneAndUpdate).to.have.been.calledOnce + const query = { + _id: userId, + 'emails.samlProviderId': providerId, + } + const update = { + $pull: { + samlIdentifiers: { + providerId, + }, + }, + $unset: { + 'emails.$.samlProviderId': 1, + }, + } + expect(this.User.findOneAndUpdate.lastCall.args).to.deep.equal([ + query, + update, + ]) + + expect(this.UserAuditLogHandler.promises.addEntry).to.have.been.calledOnce + expect( + this.UserAuditLogHandler.promises.addEntry.lastCall.args + ).to.deep.equal([ + userId, + 'unlink-institution-sso-not-migrated', + undefined, + 'N/A', + { providerId, providerName }, + ]) + + expect(this.InstitutionsAPI.promises.removeEntitlement).to.have.been + .calledOnce + expect( + this.InstitutionsAPI.promises.removeEntitlement.lastCall.args + ).to.deep.equal([userId, institutionEmail]) + }) + }) })