From aee83bc0cfebd63a74644ad42c79b4a14e78e9ab Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Mon, 14 Sep 2020 08:54:19 -0500 Subject: [PATCH] Merge pull request #3173 from overleaf/jel-institution-sso-audit-log Update audit log when institution SSO is linked/unlinked GitOrigin-RevId: 264ffbed025dccb4dd202f86afe545c3bf0c1f76 --- .../src/Features/User/SAMLIdentityManager.js | 66 ++++- .../src/Features/User/UserEmailsController.js | 44 ++- .../web/app/src/Features/User/UserUpdater.js | 95 ++++--- .../web/test/acceptance/src/helpers/User.js | 8 +- .../unit/src/User/SAMLIdentityManagerTests.js | 266 ++++++++++++++---- .../src/User/UserEmailsControllerTests.js | 35 +-- .../test/unit/src/User/UserUpdaterTests.js | 105 +++++-- 7 files changed, 446 insertions(+), 173 deletions(-) diff --git a/services/web/app/src/Features/User/SAMLIdentityManager.js b/services/web/app/src/Features/User/SAMLIdentityManager.js index aac44ad35b..60712e7bbb 100644 --- a/services/web/app/src/Features/User/SAMLIdentityManager.js +++ b/services/web/app/src/Features/User/SAMLIdentityManager.js @@ -4,17 +4,42 @@ const InstitutionsAPI = require('../Institutions/InstitutionsAPI') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') const OError = require('@overleaf/o-error') const SubscriptionLocator = require('../Subscription/SubscriptionLocator') +const UserAuditLogHandler = require('../User/UserAuditLogHandler') const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') const logger = require('logger-sharelatex') const { User } = require('../../models/User') +async function _addAuditLogEntry( + link, + userId, + auditLog, + institutionEmail, + providerId, + providerName +) { + const operation = link ? 'link-institution-sso' : 'unlink-institution-sso' + await UserAuditLogHandler.promises.addEntry( + userId, + operation, + auditLog.initiatorId, + auditLog.ipAddress, + { + institutionEmail, + providerId, + providerName + } + ) +} + async function _addIdentifier( userId, externalUserId, providerId, hasEntitlement, - institutionEmail + institutionEmail, + providerName, + auditLog ) { // first check if institutionEmail linked to another account // before adding the identifier for the email @@ -33,7 +58,18 @@ async function _addIdentifier( throw new Errors.EmailExistsError() } } + providerId = providerId.toString() + + await _addAuditLogEntry( + true, + userId, + auditLog, + institutionEmail, + providerId, + providerName + ) + hasEntitlement = !!hasEntitlement const query = { _id: userId, @@ -65,7 +101,7 @@ async function _addIdentifier( } } -async function _addInstitutionEmail(userId, email, providerId) { +async function _addInstitutionEmail(userId, email, providerId, auditLog) { const user = await UserGetter.promises.getUser(userId) const query = { _id: userId, @@ -83,12 +119,10 @@ async function _addInstitutionEmail(userId, email, providerId) { if (emailAlreadyAssociated && emailAlreadyAssociated.confirmedAt) { await UserUpdater.promises.updateUser(query, update) } else if (emailAlreadyAssociated) { - // add and confirm email await UserUpdater.promises.confirmEmail(user._id, email) await UserUpdater.promises.updateUser(query, update) } else { - // add and confirm email - await UserUpdater.promises.addEmailAddress(user._id, email) + await UserUpdater.promises.addEmailAddress(user._id, email, {}, auditLog) await UserUpdater.promises.confirmEmail(user._id, email) await UserUpdater.promises.updateUser(query, update) } @@ -161,16 +195,19 @@ async function linkAccounts( institutionEmail, providerId, providerName, - hasEntitlement + hasEntitlement, + auditLog ) { await _addIdentifier( userId, externalUserId, providerId, hasEntitlement, - institutionEmail + institutionEmail, + providerName, + auditLog ) - await _addInstitutionEmail(userId, institutionEmail, providerId) + await _addInstitutionEmail(userId, institutionEmail, providerId, auditLog) await _sendLinkedEmail(userId, providerName) // update v1 affiliations record if (hasEntitlement) { @@ -190,12 +227,23 @@ async function unlinkAccounts( institutionEmail, primaryEmail, providerId, - providerName + providerName, + auditLog ) { providerId = providerId.toString() const query = { _id: userId } + + await _addAuditLogEntry( + false, + userId, + auditLog, + institutionEmail, + providerId, + providerName + ) + const update = { $pull: { samlIdentifiers: { diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index 7c48567f97..0efeb9e358 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/services/web/app/src/Features/User/UserEmailsController.js @@ -3,13 +3,26 @@ const UserGetter = require('./UserGetter') const UserUpdater = require('./UserUpdater') const EmailHandler = require('../Email/EmailHandler') const EmailHelper = require('../Helpers/EmailHelper') -const UserAuditLogHandler = require('./UserAuditLogHandler') const UserEmailsConfirmationHandler = require('./UserEmailsConfirmationHandler') const { endorseAffiliation } = require('../Institutions/InstitutionsAPI') const Errors = require('../Errors/Errors') const HttpErrorHandler = require('../Errors/HttpErrorHandler') const { expressify } = require('../../util/promises') +async function _sendSecurityAlertEmail(user, email) { + const emailOptions = { + to: user.email, + actionDescribed: `a secondary email address has been added to your account ${ + user.email + }`, + message: [ + `Added:
${email}` + ], + action: 'secondary email address added' + } + await EmailHandler.promises.sendEmail('securityAlert', emailOptions) +} + async function add(req, res, next) { const userId = AuthenticationController.getLoggedInUserId(req) const email = EmailHelper.parseEmail(req.body.email) @@ -24,37 +37,22 @@ async function add(req, res, next) { department: req.body.department } - await UserAuditLogHandler.promises.addEntry( - user._id, - 'add-email', - user._id, - req.ip, - { - newSecondaryEmail: email - } - ) - try { await UserUpdater.promises.addEmailAddress( userId, email, - affiliationOptions + affiliationOptions, + { + initiatorId: user._id, + ipAddress: req.ip + } ) } catch (error) { return UserEmailsController._handleEmailError(error, req, res, next) } - const emailOptions = { - to: user.email, - actionDescribed: `a secondary email address has been added to your account ${ - user.email - }`, - message: [ - `Added:
${email}` - ], - action: 'secondary email address added' - } - await EmailHandler.promises.sendEmail('securityAlert', emailOptions) + await _sendSecurityAlertEmail(user, email) + await UserEmailsConfirmationHandler.promises.sendConfirmationEmail( userId, email diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index 83da39724b..18069f1556 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/services/web/app/src/Features/User/UserUpdater.js @@ -9,7 +9,8 @@ const { callbackify, promisify } = require('util') const UserGetter = require('./UserGetter') const { addAffiliation, - removeAffiliation + removeAffiliation, + promises: InstitutionsAPIPromises } = require('../Institutions/InstitutionsAPI') const Features = require('../../infrastructure/Features') const FeaturesUpdater = require('../Subscription/FeaturesUpdater') @@ -20,6 +21,51 @@ const NewsletterManager = require('../Newsletter/NewsletterManager') const RecurlyWrapper = require('../Subscription/RecurlyWrapper') const UserAuditLogHandler = require('./UserAuditLogHandler') +async function addEmailAddress(userId, newEmail, affiliationOptions, auditLog) { + newEmail = EmailHelper.parseEmail(newEmail) + if (!newEmail) { + throw new Error('invalid email') + } + + await UserGetter.promises.ensureUniqueEmailAddress(newEmail) + + await UserAuditLogHandler.promises.addEntry( + userId, + 'add-email', + auditLog.initiatorId, + auditLog.ipAddress, + { + newSecondaryEmail: newEmail + } + ) + + try { + await InstitutionsAPIPromises.addAffiliation( + userId, + newEmail, + affiliationOptions + ) + } catch (error) { + throw OError.tag(error, 'problem adding affiliation while adding email') + } + + try { + const reversedHostname = newEmail + .split('@')[1] + .split('') + .reverse() + .join('') + const update = { + $push: { + emails: { email: newEmail, createdAt: new Date(), reversedHostname } + } + } + await UserUpdater.promises.updateUser(userId, update) + } catch (error) { + throw OError.tag(error, 'problem updating users emails') + } +} + async function setDefaultEmailAddress( userId, email, @@ -174,7 +220,7 @@ const UserUpdater = { oldEmail = email cb(error) }), - cb => UserUpdater.addEmailAddress(userId, newEmail, cb), + cb => UserUpdater.addEmailAddress(userId, newEmail, {}, auditLog, cb), cb => UserUpdater.setDefaultEmailAddress( userId, @@ -192,48 +238,7 @@ const UserUpdater = { // Add a new email address for the user. Email cannot be already used by this // or any other user - addEmailAddress(userId, newEmail, affiliationOptions, callback) { - if (callback == null) { - // affiliationOptions is optional - callback = affiliationOptions - affiliationOptions = {} - } - newEmail = EmailHelper.parseEmail(newEmail) - if (newEmail == null) { - return callback(new Error('invalid email')) - } - - UserGetter.ensureUniqueEmailAddress(newEmail, error => { - if (error != null) { - return callback(error) - } - - addAffiliation(userId, newEmail, affiliationOptions, error => { - if (error != null) { - OError.tag(error, 'problem adding affiliation while adding email') - return callback(error) - } - - const reversedHostname = newEmail - .split('@')[1] - .split('') - .reverse() - .join('') - const update = { - $push: { - emails: { email: newEmail, createdAt: new Date(), reversedHostname } - } - } - UserUpdater.updateUser(userId, update, error => { - if (error != null) { - OError.tag(error, 'problem updating users emails') - return callback(error) - } - callback() - }) - }) - }) - }, + addEmailAddress: callbackify(addEmailAddress), // remove one of the user's email addresses. The email cannot be the user's // default email address @@ -334,7 +339,7 @@ const UserUpdater = { const promises = { addAffiliationForNewUser: promisify(UserUpdater.addAffiliationForNewUser), - addEmailAddress: promisify(UserUpdater.addEmailAddress), + addEmailAddress, confirmEmail: promisify(UserUpdater.confirmEmail), setDefaultEmailAddress, updateUser: promisify(UserUpdater.updateUser), diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index 0093aaa1c8..ce98809dc1 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -186,7 +186,13 @@ class User { addEmail(email, callback) { this.emails.push({ email, createdAt: new Date() }) - UserUpdater.addEmailAddress(this.id, email, callback) + UserUpdater.addEmailAddress( + this.id, + email, + {}, + { initiatorId: this._id, ipAddress: '127:0:0:0' }, + callback + ) } confirmEmail(email, callback) { diff --git a/services/web/test/unit/src/User/SAMLIdentityManagerTests.js b/services/web/test/unit/src/User/SAMLIdentityManagerTests.js index db8bde435a..64c80192d6 100644 --- a/services/web/test/unit/src/User/SAMLIdentityManagerTests.js +++ b/services/web/test/unit/src/User/SAMLIdentityManagerTests.js @@ -11,12 +11,17 @@ describe('SAMLIdentityManager', function() { NotFoundError: sinon.stub(), SAMLIdentityExistsError: sinon.stub() } + this.userId = 'user-id-1' this.user = { - _id: 'user-id-1', + _id: this.userId, email: 'not-linked@overleaf.com', emails: [{ email: 'not-linked@overleaf.com' }], samlIdentifiers: [] } + this.auditLog = { + initiatorId: this.userId, + ipAddress: '0:0:0:0' + } this.userAlreadyLinked = { _id: 'user-id-2', email: 'linked@overleaf.com', @@ -43,6 +48,9 @@ describe('SAMLIdentityManager', function() { warn: sinon.stub() } this.SAMLIdentityManager = SandboxedModule.require(modulePath, { + globals: { + console: console + }, requires: { '../Email/EmailHandler': (this.EmailHandler = { sendEmail: sinon.stub().yields() @@ -73,6 +81,11 @@ describe('SAMLIdentityManager', function() { }) }) }, + '../User/UserAuditLogHandler': (this.UserAuditLogHandler = { + promises: { + addEntry: sinon.stub().resolves() + } + }), '../User/UserGetter': (this.UserGetter = { getUser: sinon.stub(), promises: { @@ -108,76 +121,157 @@ describe('SAMLIdentityManager', function() { }) describe('linkAccounts', function() { - it('should throw an error if missing data', async function() { - let error - try { - await this.SAMLIdentityManager.linkAccounts(null, null, null, null) - } catch (e) { - error = e - } finally { - expect(error).to.exist - } - }) - - describe('when email is already associated with another Overleaf account', function() { - beforeEach(function() { - this.UserGetter.promises.getUserByAnyEmail.resolves( - this.userEmailExists - ) - }) - - it('should throw an EmailExistsError error', async function() { + describe('errors', function() { + it('should throw an error if missing data', async function() { let error try { await this.SAMLIdentityManager.linkAccounts( - 'user-id-1', - 'not-linked-id', - 'exists@overleaf.com', - 'provider-id', - true + null, + null, + null, + null, + null ) } catch (e) { error = e } finally { - expect(error).to.be.instanceof(this.Errors.EmailExistsError) - expect(this.User.findOneAndUpdate).to.not.have.been.called + expect(error).to.exist } }) - }) - describe('when institution identifier is already associated with another Overleaf account', function() { - beforeEach(function() { - this.UserGetter.promises.getUserByAnyEmail.resolves( - this.userAlreadyLinked - ) + describe('when email is already associated with another Overleaf account', function() { + beforeEach(function() { + this.UserGetter.promises.getUserByAnyEmail.resolves( + this.userEmailExists + ) + }) + + it('should throw an EmailExistsError error', async function() { + let error + try { + await this.SAMLIdentityManager.linkAccounts( + 'user-id-1', + 'not-linked-id', + 'exists@overleaf.com', + 'provider-id', + 'provider-name', + true, + { + intiatorId: 'user-id-1', + ip: '0:0:0:0' + } + ) + } catch (e) { + error = e + } finally { + expect(error).to.be.instanceof(this.Errors.EmailExistsError) + expect(this.User.findOneAndUpdate).to.not.have.been.called + } + }) }) - it('should throw an SAMLIdentityExistsError error', async function() { + describe('when institution identifier is already associated with another Overleaf account', function() { + beforeEach(function() { + this.UserGetter.promises.getUserByAnyEmail.resolves( + this.userAlreadyLinked + ) + }) + + it('should throw an SAMLIdentityExistsError error', async function() { + let error + try { + await this.SAMLIdentityManager.linkAccounts( + 'user-id-1', + 'already-linked-id', + 'linked@overleaf.com', + 'provider-id', + 'provider-name', + true, + { + intiatorId: 'user-id-1', + ip: '0:0:0:0' + } + ) + } catch (e) { + error = e + } finally { + expect(error).to.be.instanceof(this.Errors.SAMLIdentityExistsError) + expect(this.User.findOneAndUpdate).to.not.have.been.called + } + }) + }) + + it('should pass back errors via UserAuditLogHandler', async function() { let error + const anError = new Error('oops') + this.UserAuditLogHandler.promises.addEntry.rejects(anError) try { await this.SAMLIdentityManager.linkAccounts( - 'user-id-1', - 'already-linked-id', - 'linked@overleaf.com', - 'provider-id', - true + this.user._id, + 'externalUserId', + this.user.email, + '1', + 'Overleaf University', + undefined, + { + intiatorId: 'user-id-1', + ipAddress: '0:0:0:0' + } ) } catch (e) { error = e } finally { - expect(error).to.be.instanceof(this.Errors.SAMLIdentityExistsError) - expect(this.User.findOneAndUpdate).to.not.have.been.called + expect(error).to.exist + expect(error).to.equal(anError) + expect(this.EmailHandler.sendEmail).to.not.have.been.called + expect(this.User.update).to.not.have.been.called } }) }) - describe('after linking', function() { - it('should send an email notification', function() { + describe('success', function() { + it('should update the user audit log', function() { + const auditLog = { + intiatorId: 'user-id-1', + ip: '0:0:0:0' + } this.SAMLIdentityManager.linkAccounts( this.user._id, + 'externalUserId', this.user.email, '1', 'Overleaf University', + undefined, + auditLog, + () => { + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledWith( + this.user._id, + 'link-institution-sso', + auditLog.initiatorId, + auditLog.ip, + { + institutionEmail: this.user.email, + providerId: '1', + providerName: 'Overleaf University' + } + ) + } + ) + }) + it('should send an email notification', function() { + this.SAMLIdentityManager.linkAccounts( + this.user._id, + 'externalUserId', + this.user.email, + '1', + 'Overleaf University', + undefined, + { + intiatorId: 'user-id-1', + ipAddress: '0:0:0:0' + }, () => { expect(this.User.update).to.have.been.called expect(this.EmailHandler.sendEmail).to.have.been.called @@ -188,18 +282,94 @@ describe('SAMLIdentityManager', function() { }) describe('unlinkAccounts', function() { - it('should send an email notification', function() { - this.SAMLIdentityManager.unlinkAccounts( + const linkedEmail = 'another@example.com' + it('should update the audit log', async function() { + await this.SAMLIdentityManager.unlinkAccounts( this.user._id, + linkedEmail, this.user.email, '1', 'Overleaf University', - () => { - expect(this.User.update).to.have.been.called - expect(this.EmailHandler.sendEmail).to.have.been.called + this.auditLog + ) + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledOnce.and.calledWithMatch( + this.user._id, + 'unlink-institution-sso', + this.auditLog.initiatorId, + this.auditLog.ipAddress, + { + institutionEmail: linkedEmail, + providerId: '1', + providerName: 'Overleaf University' } ) }) + it('should remove the identifier', async function() { + await this.SAMLIdentityManager.unlinkAccounts( + this.user._id, + linkedEmail, + this.user.email, + '1', + 'Overleaf University', + this.auditLog + ) + const query = { + _id: this.user._id + } + const update = { + $pull: { + samlIdentifiers: { + providerId: '1' + } + } + } + expect(this.User.update).to.have.been.calledOnce.and.calledWithMatch( + query, + update + ) + }) + it('should send an email notification', async function() { + await this.SAMLIdentityManager.unlinkAccounts( + this.user._id, + linkedEmail, + this.user.email, + '1', + 'Overleaf University', + this.auditLog + ) + expect(this.User.update).to.have.been.called + expect(this.EmailHandler.sendEmail).to.have.been.calledOnce + const emailArgs = this.EmailHandler.sendEmail.lastCall.args + expect(emailArgs[0]).to.equal('securityAlert') + expect(emailArgs[1].to).to.equal(this.user.email) + }) + + describe('errors', function() { + it('should pass back errors via UserAuditLogHandler', async function() { + let error + const anError = new Error('oops') + this.UserAuditLogHandler.promises.addEntry.rejects(anError) + try { + await this.SAMLIdentityManager.unlinkAccounts( + this.user._id, + linkedEmail, + this.user.email, + '1', + 'Overleaf University', + this.auditLog + ) + } catch (e) { + error = e + } finally { + expect(error).to.exist + expect(error).to.equal(anError) + expect(this.EmailHandler.sendEmail).to.not.have.been.called + expect(this.User.update).to.not.have.been.called + } + }) + }) }) describe('entitlementAttributeMatches', function() { diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index 238ee64717..67d6d50441 100644 --- a/services/web/test/unit/src/User/UserEmailsControllerTests.js +++ b/services/web/test/unit/src/User/UserEmailsControllerTests.js @@ -66,11 +66,6 @@ describe('UserEmailsController', function() { } }), '../Helpers/EmailHelper': this.EmailHelper, - './UserAuditLogHandler': (this.UserAuditLogHandler = { - promises: { - addEntry: sinon.stub().resolves() - } - }), './UserEmailsConfirmationHandler': (this.UserEmailsConfirmationHandler = { promises: { sendConfirmationEmail: sinon.stub().resolves() @@ -121,16 +116,14 @@ describe('UserEmailsController', function() { .yields() }) - it('adds an entry to user audit log', function(done) { + it('passed audit log to addEmailAddress', function(done) { this.res.sendStatus = sinon.stub() this.res.sendStatus.callsFake(() => { - this.UserAuditLogHandler.promises.addEntry.should.have.been.calledWith( - this.user._id, - 'add-email', - this.user._id, - this.req.ip, - { newSecondaryEmail: this.newEmail } - ) + const addCall = this.UserUpdater.promises.addEmailAddress.lastCall + expect(addCall.args[3]).to.deep.equal({ + initiatorId: this.user._id, + ipAddress: this.req.ip + }) done() }) this.UserEmailsController.add(this.req, this.res) @@ -246,22 +239,6 @@ describe('UserEmailsController', function() { }) this.UserEmailsController.add(this.req, this.res, this.next) }) - - describe('errors', function() { - describe('via UserAuditLogHandler', function() { - beforeEach(function() { - this.UserAuditLogHandler.promises.addEntry.throws('oops') - }) - it('should not add email and should return error', function(done) { - this.UserEmailsController.add(this.req, this.res, error => { - expect(error).to.exist - this.UserUpdater.promises.addEmailAddress.should.not.have.been - .called - done() - }) - }) - }) - }) }) describe('remove', function() { diff --git a/services/web/test/unit/src/User/UserUpdaterTests.js b/services/web/test/unit/src/User/UserUpdaterTests.js index 432a8c03cc..4b468ec218 100644 --- a/services/web/test/unit/src/User/UserUpdaterTests.js +++ b/services/web/test/unit/src/User/UserUpdaterTests.js @@ -21,8 +21,8 @@ describe('UserUpdater', function() { this.UserGetter = { getUserEmail: sinon.stub(), getUserByAnyEmail: sinon.stub(), - ensureUniqueEmailAddress: sinon.stub(), promises: { + ensureUniqueEmailAddress: sinon.stub(), getUser: sinon.stub() } } @@ -55,10 +55,13 @@ describe('UserUpdater', function() { timeAsyncMethod: sinon.stub() }, './UserGetter': this.UserGetter, - '../Institutions/InstitutionsAPI': { + '../Institutions/InstitutionsAPI': (this.InstitutionsAPI = { addAffiliation: this.addAffiliation, - removeAffiliation: this.removeAffiliation - }, + removeAffiliation: this.removeAffiliation, + promises: { + addAffiliation: sinon.stub() + } + }), '../Email/EmailHandler': (this.EmailHandler = { promises: { sendEmail: sinon.stub() @@ -140,7 +143,7 @@ describe('UserUpdater', function() { ipAddress: '0:0:0:0' } this.UserGetter.getUserEmail.callsArgWith(1, null, this.stubbedUser.email) - this.UserUpdater.addEmailAddress = sinon.stub().callsArgWith(2) + this.UserUpdater.addEmailAddress = sinon.stub().callsArgWith(4) this.UserUpdater.setDefaultEmailAddress = sinon.stub().yields() this.UserUpdater.removeEmailAddress = sinon.stub().callsArgWith(2) }) @@ -153,7 +156,7 @@ describe('UserUpdater', function() { err => { should.not.exist(err) this.UserUpdater.addEmailAddress - .calledWith(this.stubbedUser._id, this.newEmail) + .calledWith(this.stubbedUser._id, this.newEmail, {}, this.auditLog) .should.equal(true) this.UserUpdater.setDefaultEmailAddress .calledWith( @@ -200,23 +203,29 @@ describe('UserUpdater', function() { describe('addEmailAddress', function() { beforeEach(function() { - this.UserGetter.ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) - this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null) + this.UserGetter.promises.ensureUniqueEmailAddress = sinon + .stub() + .resolves() + this.UserUpdater.promises.updateUser = sinon.stub().resolves() }) it('add email', function(done) { this.UserUpdater.addEmailAddress( this.stubbedUser._id, this.newEmail, + {}, + { initiatorId: this.stubbedUser._id, ipAddress: '127:0:0:0' }, err => { - this.UserGetter.ensureUniqueEmailAddress.called.should.equal(true) - should.not.exist(err) + this.UserGetter.promises.ensureUniqueEmailAddress.called.should.equal( + true + ) + expect(err).to.not.exist const reversedHostname = this.newEmail .split('@')[1] .split('') .reverse() .join('') - this.UserUpdater.updateUser + this.UserUpdater.promises.updateUser .calledWith(this.stubbedUser._id, { $push: { emails: { @@ -242,10 +251,13 @@ describe('UserUpdater', function() { this.stubbedUser._id, this.newEmail, affiliationOptions, + { initiatorId: this.stubbedUser._id, ipAddress: '127:0:0:0' }, err => { should.not.exist(err) - this.addAffiliation.calledOnce.should.equal(true) - const { args } = this.addAffiliation.lastCall + this.InstitutionsAPI.promises.addAffiliation.calledOnce.should.equal( + true + ) + const { args } = this.InstitutionsAPI.promises.addAffiliation.lastCall args[0].should.equal(this.stubbedUser._id) args[1].should.equal(this.newEmail) args[2].should.equal(affiliationOptions) @@ -255,22 +267,79 @@ describe('UserUpdater', function() { }) it('handle affiliation error', function(done) { - this.addAffiliation.callsArgWith(3, new Error('nope')) + this.InstitutionsAPI.promises.addAffiliation.rejects(new Error('nope')) this.UserUpdater.addEmailAddress( this.stubbedUser._id, this.newEmail, + {}, + { initiatorId: this.stubbedUser._id, ipAddress: '127:0:0:0' }, err => { should.exist(err) - this.UserUpdater.updateUser.called.should.equal(false) + this.UserUpdater.promises.updateUser.called.should.equal(false) done() } ) }) it('validates email', function(done) { - this.UserUpdater.addEmailAddress(this.stubbedUser._id, 'bar', err => { - should.exist(err) - done() + this.UserUpdater.addEmailAddress( + this.stubbedUser._id, + 'bar', + {}, + { initiatorId: this.stubbedUser._id, ipAddress: '127:0:0:0' }, + err => { + should.exist(err) + done() + } + ) + }) + + it('updates the audit log', function(done) { + this.ip = '127:0:0:0' + this.UserUpdater.addEmailAddress( + this.stubbedUser._id, + this.newEmail, + {}, + { initiatorId: this.stubbedUser._id, ipAddress: this.ip }, + error => { + expect(error).to.not.exist + this.InstitutionsAPI.promises.addAffiliation.calledOnce.should.equal( + true + ) + const { args } = this.UserAuditLogHandler.promises.addEntry.lastCall + expect(args[0]).to.equal(this.stubbedUser._id) + expect(args[1]).to.equal('add-email') + expect(args[2]).to.equal(this.stubbedUser._id) + expect(args[3]).to.equal(this.ip) + expect(args[4]).to.deep.equal({ + newSecondaryEmail: this.newEmail + }) + done() + } + ) + }) + + describe('errors', function() { + describe('via UserAuditLogHandler', function() { + const anError = new Error('oops') + beforeEach(function() { + this.UserAuditLogHandler.promises.addEntry.throws(anError) + }) + it('should not add email and should return error', function(done) { + this.UserUpdater.addEmailAddress( + this.stubbedUser._id, + this.newEmail, + {}, + { initiatorId: this.stubbedUser._id, ipAddress: '127:0:0:0' }, + error => { + expect(error).to.exist + expect(error).to.equal(anError) + expect(this.UserUpdater.promises.updateUser).to.not.have.been + .called + done() + } + ) + }) }) }) })