Merge pull request #3173 from overleaf/jel-institution-sso-audit-log

Update audit log when institution SSO is linked/unlinked GitOrigin-RevId: 264ffbed025dccb4dd202f86afe545c3bf0c1f76
2025-03-20 13:23:19 +00:00 · 2020-09-14 08:54:19 -05:00 · 2020-09-14 08:54:19 -05:00 · aee83bc0cf
commit aee83bc0cf
parent 7c093ba115
7 changed files with 446 additions and 173 deletions
--- 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: {
--- 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: [
+      `<span style="display:inline-block;padding: 0 20px;width:100%;">Added: <br/><b>${email}</b></span>`
+    ],
+    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: [
-      `<span style="display:inline-block;padding: 0 20px;width:100%;">Added: <br/><b>${email}</b></span>`
-    ],
-    action: 'secondary email address added'
-  }
-  await EmailHandler.promises.sendEmail('securityAlert', emailOptions)
+  await _sendSecurityAlertEmail(user, email)
+
  await UserEmailsConfirmationHandler.promises.sendConfirmationEmail(
    userId,
    email
--- 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),
--- 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) {
--- 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() {
--- 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() {
--- 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()
+            }
+          )
+        })
      })
    })
  })