diff --git a/services/web/app/src/Features/User/ThirdPartyIdentityManager.js b/services/web/app/src/Features/User/ThirdPartyIdentityManager.js index f88bb76b52..34f507ffa6 100644 --- a/services/web/app/src/Features/User/ThirdPartyIdentityManager.js +++ b/services/web/app/src/Features/User/ThirdPartyIdentityManager.js @@ -1,4 +1,5 @@ const APP_ROOT = '../../../../app/src' +const UserAuditLogHandler = require(`${APP_ROOT}/Features/User/UserAuditLogHandler`) const EmailHandler = require(`${APP_ROOT}/Features/Email/EmailHandler`) const Errors = require('../Errors/Errors') const _ = require('lodash') @@ -58,6 +59,7 @@ function link( providerId, externalUserId, externalData, + auditLog, callback, retry ) { @@ -65,80 +67,115 @@ function link( if (!oauthProviders[providerId]) { return callback(new Error('Not a valid provider')) } - const query = { - _id: userId, - 'thirdPartyIdentifiers.providerId': { - $ne: providerId - } - } - const update = { - $push: { - thirdPartyIdentifiers: { - externalUserId, - externalData, - providerId + + UserAuditLogHandler.addEntry( + userId, + 'link-sso', + auditLog.initiatorId, + auditLog.ipAddress, + { + providerId + }, + error => { + if (error) { + return callback(error) } - } - } - // add new tpi only if an entry for the provider does not exist - // projection includes thirdPartyIdentifiers for tests - User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => { - if (err && err.code === 11000) { - callback(new Errors.ThirdPartyIdentityExistsError()) - } else if (err != null) { - callback(err) - } else if (res) { - _sendSecurityAlert(accountLinked, providerId, res, userId) - callback(null, res) - } else if (retry) { - // if already retried then throw error - callback(new Error('update failed')) - } else { - // attempt to clear existing entry then retry - ThirdPartyIdentityManager.unlink(userId, providerId, function(err) { - if (err != null) { - return callback(err) + const query = { + _id: userId, + 'thirdPartyIdentifiers.providerId': { + $ne: providerId + } + } + const update = { + $push: { + thirdPartyIdentifiers: { + externalUserId, + externalData, + providerId + } + } + } + // add new tpi only if an entry for the provider does not exist + // projection includes thirdPartyIdentifiers for tests + User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => { + if (err && err.code === 11000) { + callback(new Errors.ThirdPartyIdentityExistsError()) + } else if (err != null) { + callback(err) + } else if (res) { + _sendSecurityAlert(accountLinked, providerId, res, userId) + callback(null, res) + } else if (retry) { + // if already retried then throw error + callback(new Error('update failed')) + } else { + // attempt to clear existing entry then retry + ThirdPartyIdentityManager.unlink( + userId, + providerId, + auditLog, + function(err) { + if (err != null) { + return callback(err) + } + ThirdPartyIdentityManager.link( + userId, + providerId, + externalUserId, + externalData, + auditLog, + callback, + true + ) + } + ) } - ThirdPartyIdentityManager.link( - userId, - providerId, - externalUserId, - externalData, - callback, - true - ) }) } - }) + ) } -function unlink(userId, providerId, callback) { +function unlink(userId, providerId, auditLog, callback) { const accountLinked = false if (!oauthProviders[providerId]) { return callback(new Error('Not a valid provider')) } - const query = { - _id: userId - } - const update = { - $pull: { - thirdPartyIdentifiers: { - providerId + UserAuditLogHandler.addEntry( + userId, + 'unlink-sso', + auditLog.initiatorId, + auditLog.ipAddress, + { + providerId + }, + error => { + if (error) { + return callback(error) } + const query = { + _id: userId + } + const update = { + $pull: { + thirdPartyIdentifiers: { + providerId + } + } + } + // projection includes thirdPartyIdentifiers for tests + User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => { + if (err != null) { + callback(err) + } else if (!res) { + callback(new Error('update failed')) + } else { + // no need to wait, errors are logged and not passed back + _sendSecurityAlert(accountLinked, providerId, res, userId) + callback(null, res) + } + }) } - } - // projection includes thirdPartyIdentifiers for tests - User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => { - if (err != null) { - callback(err) - } else if (!res) { - callback(new Error('update failed')) - } else { - // no need to wait, errors are logged and not passed back - _sendSecurityAlert(accountLinked, providerId, res, userId) - callback(null, res) - } - }) + ) } function _getUserQuery(providerId, externalUserId) { diff --git a/services/web/app/src/Features/User/UserAuditLogHandler.js b/services/web/app/src/Features/User/UserAuditLogHandler.js index 5e926758b9..cf3c4f151d 100644 --- a/services/web/app/src/Features/User/UserAuditLogHandler.js +++ b/services/web/app/src/Features/User/UserAuditLogHandler.js @@ -1,8 +1,15 @@ const OError = require('@overleaf/o-error') const { User } = require('../../models/User') +const { callbackify } = require('util') const MAX_AUDIT_LOG_ENTRIES = 200 +function _canHaveNoInitiatorId(operation, info) { + if (operation === 'reset-password') return true + if (operation === 'unlink-sso' && info.providerId === 'collabratec') + return true +} + /** * Add an audit log entry * @@ -22,11 +29,12 @@ async function addEntry(userId, operation, initiatorId, ipAddress, info = {}) { ipAddress }) - if (!initiatorId && operation !== 'reset-password') + if (!initiatorId && !_canHaveNoInitiatorId(operation, info)) { throw new OError('missing initiatorId for audit log', { operation, ipAddress }) + } const timestamp = new Date() const entry = { @@ -50,6 +58,7 @@ async function addEntry(userId, operation, initiatorId, ipAddress, info = {}) { } const UserAuditLogHandler = { + addEntry: callbackify(addEntry), promises: { addEntry } diff --git a/services/web/test/acceptance/src/UserThirdPartyIdentityTests.js b/services/web/test/acceptance/src/UserThirdPartyIdentityTests.js index 59eb311e70..7287641f7c 100644 --- a/services/web/test/acceptance/src/UserThirdPartyIdentityTests.js +++ b/services/web/test/acceptance/src/UserThirdPartyIdentityTests.js @@ -39,6 +39,7 @@ describe('ThirdPartyIdentityManager', function() { this.provider, this.externalUserId, this.externalData, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, done ) }) @@ -99,6 +100,7 @@ describe('ThirdPartyIdentityManager', function() { this.provider, this.externalUserId, this.externalData, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, (err, res) => { expect(res.thirdPartyIdentifiers.length).to.equal(1) return done() @@ -114,6 +116,7 @@ describe('ThirdPartyIdentityManager', function() { this.provider, this.externalUserId, this.externalData, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, done ) }) @@ -124,6 +127,7 @@ describe('ThirdPartyIdentityManager', function() { this.provider, this.externalUserId, this.externalData, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, (err, res) => { expect(res).to.exist done() @@ -137,6 +141,7 @@ describe('ThirdPartyIdentityManager', function() { this.provider, this.externalUserId, this.externalData, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, (err, user) => { expect(user.thirdPartyIdentifiers.length).to.equal(1) return done() @@ -151,6 +156,7 @@ describe('ThirdPartyIdentityManager', function() { this.provider, this.externalUserId, this.externalData, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, (err, user) => { expect(user.thirdPartyIdentifiers.length).to.equal(1) return done() @@ -187,6 +193,7 @@ describe('ThirdPartyIdentityManager', function() { return ThirdPartyIdentityManager.unlink( this.user.id, this.provider, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, (err, res) => { expect(err).to.be.null expect(res.thirdPartyIdentifiers.length).to.equal(0) @@ -203,6 +210,7 @@ describe('ThirdPartyIdentityManager', function() { this.provider, this.externalUserId, this.externalData, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, done ) }) @@ -211,6 +219,7 @@ describe('ThirdPartyIdentityManager', function() { return ThirdPartyIdentityManager.unlink( this.user.id, this.provider, + { initiatorId: this.user.id, ipAddress: '0:0:0:0' }, (err, user) => { expect(user.thirdPartyIdentifiers.length).to.equal(0) return done() diff --git a/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js b/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js index 118d1dedd4..b5d1f66006 100644 --- a/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js +++ b/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js @@ -14,11 +14,15 @@ describe('ThirdPartyIdentityManager', function() { } this.externalUserId = 'id789' this.externalData = {} + this.auditLog = { initiatorId: this.userId, ipAddress: '0:0:0:0' } this.ThirdPartyIdentityManager = SandboxedModule.require(modulePath, { globals: { console: console }, requires: { + '../../../../app/src/Features/User/UserAuditLogHandler': (this.UserAuditLogHandler = { + addEntry: sinon.stub().yields() + }), '../../../../app/src/Features/Email/EmailHandler': (this.EmailHandler = { sendEmail: sinon.stub().yields() }), @@ -54,7 +58,8 @@ describe('ThirdPartyIdentityManager', function() { this.userId, 'google', this.externalUserId, - this.externalData + this.externalData, + this.auditLog ) const emailCall = this.EmailHandler.sendEmail.getCall(0) expect(emailCall.args[0]).to.equal('securityAlert') @@ -62,8 +67,43 @@ describe('ThirdPartyIdentityManager', function() { 'a Google account was linked' ) }) + + it('should update user audit log', async function() { + await this.ThirdPartyIdentityManager.promises.link( + this.userId, + 'google', + this.externalUserId, + this.externalData, + this.auditLog + ) + expect(this.UserAuditLogHandler.addEntry).to.have.been.calledOnceWith( + this.userId, + 'link-sso', + this.auditLog.initiatorId, + this.auditLog.ipAddress, + { + providerId: 'google' + } + ) + }) describe('errors', function() { const anError = new Error('oops') + it('should not unlink if the UserAuditLogHandler throws an error', function(done) { + this.UserAuditLogHandler.addEntry.yields(anError) + this.ThirdPartyIdentityManager.link( + this.userId, + 'google', + this.externalUserId, + this.externalData, + this.auditLog, + error => { + expect(error).to.exist + expect(error).to.equal(anError) + expect(this.User.findOneAndUpdate).to.not.have.been.called + done() + } + ) + }) describe('EmailHandler', function() { beforeEach(function() { this.EmailHandler.sendEmail.yields(anError) @@ -74,6 +114,7 @@ describe('ThirdPartyIdentityManager', function() { 'google', this.externalUserId, this.externalData, + this.auditLog, error => { expect(error).to.not.exist const loggerCall = this.logger.error.getCall(0) @@ -93,15 +134,50 @@ describe('ThirdPartyIdentityManager', function() { }) describe('unlink', function() { it('should send email alert', async function() { - await this.ThirdPartyIdentityManager.promises.unlink(this.userId, 'orcid') + await this.ThirdPartyIdentityManager.promises.unlink( + this.userId, + 'orcid', + this.auditLog + ) const emailCall = this.EmailHandler.sendEmail.getCall(0) expect(emailCall.args[0]).to.equal('securityAlert') expect(emailCall.args[1].actionDescribed).to.contain( 'an Orcid account is no longer linked' ) }) + it('should update user audit log', async function() { + await this.ThirdPartyIdentityManager.promises.unlink( + this.userId, + 'orcid', + this.auditLog + ) + expect(this.UserAuditLogHandler.addEntry).to.have.been.calledOnceWith( + this.userId, + 'unlink-sso', + this.auditLog.initiatorId, + this.auditLog.ipAddress, + { + providerId: 'orcid' + } + ) + }) describe('errors', function() { const anError = new Error('oops') + it('should not unlink if the UserAuditLogHandler throws an error', function(done) { + this.UserAuditLogHandler.addEntry.yields(anError) + this.ThirdPartyIdentityManager.unlink( + this.userId, + 'orcid', + this.auditLog, + error => { + expect(error).to.exist + expect(error).to.equal(anError) + expect(this.User.findOneAndUpdate).to.not.have.been.called + done() + } + ) + expect(this.User.findOneAndUpdate).to.not.have.been.called + }) describe('EmailHandler', function() { beforeEach(function() { this.EmailHandler.sendEmail.yields(anError) @@ -110,6 +186,7 @@ describe('ThirdPartyIdentityManager', function() { this.ThirdPartyIdentityManager.unlink( this.userId, 'google', + this.auditLog, error => { expect(error).to.not.exist const loggerCall = this.logger.error.getCall(0)