Merge pull request #3091 from overleaf/jel-log-default-email-change

Update audit log for default email changes GitOrigin-RevId: c7b4e4e888aa5ffd976062d72f660ded303f0885
2024-11-21 20:47:08 -05:00 · 2020-08-12 16:19:33 +02:00 · 2020-08-12 16:19:33 +02:00 · 3babf23444
commit 3babf23444
parent 0cec198a08
8 changed files with 264 additions and 33 deletions
--- a/services/web/app/src/Features/User/UserAuditLogHandler.js
+++ b/services/web/app/src/Features/User/UserAuditLogHandler.js
@ -15,6 +15,19 @@ const MAX_AUDIT_LOG_ENTRIES = 200
 * - info: an object detailing what happened
 */
 async function addEntry(userId, operation, initiatorId, ipAddress, info = {}) {
+  if (!operation || !ipAddress)
+    throw new OError('missing required audit log data', {
+      operation,
+      initiatorId,
+      ipAddress
+    })
+
+  if (!initiatorId && operation !== 'reset-password')
+    throw new OError('missing initiatorId for audit log', {
+      operation,
+      ipAddress
+    })
+
  const timestamp = new Date()
  const entry = {
    operation,
--- a/services/web/app/src/Features/User/UserController.js
+++ b/services/web/app/src/Features/User/UserController.js
@ -338,7 +338,11 @@ const UserController = {
          res.sendStatus(400)
        } else {
          // update the user email
-          UserUpdater.changeEmailAddress(userId, newEmail, err => {
+          const auditLog = {
+            initiatorId: userId,
+            ipAddress: req.ip
+          }
+          UserUpdater.changeEmailAddress(userId, newEmail, auditLog, err => {
            if (err) {
              if (err instanceof Errors.EmailExistsError) {
                const translation = req.i18n.translate(
--- a/services/web/app/src/Features/User/UserEmailsController.js
+++ b/services/web/app/src/Features/User/UserEmailsController.js
@ -121,7 +121,11 @@ const UserEmailsController = {
    if (!email) {
      return res.sendStatus(422)
    }
-    UserUpdater.setDefaultEmailAddress(userId, email, false, err => {
+    const auditLog = {
+      initiatorId: userId,
+      ipAddress: req.ip
+    }
+    UserUpdater.setDefaultEmailAddress(userId, email, false, auditLog, err => {
      if (err) {
        return UserEmailsController._handleEmailError(err, req, res, next)
      }
--- a/services/web/app/src/Features/User/UserUpdater.js
+++ b/services/web/app/src/Features/User/UserUpdater.js
@ -17,8 +17,14 @@ const EmailHelper = require('../Helpers/EmailHelper')
 const Errors = require('../Errors/Errors')
 const NewsletterManager = require('../Newsletter/NewsletterManager')
 const RecurlyWrapper = require('../Subscription/RecurlyWrapper')
+const UserAuditLogHandler = require('./UserAuditLogHandler')

-async function setDefaultEmailAddress(userId, email, allowUnconfirmed) {
+async function setDefaultEmailAddress(
+  userId,
+  email,
+  allowUnconfirmed,
+  auditLog
+) {
  email = EmailHelper.parseEmail(email)
  if (email == null) {
    throw new Error('invalid email')
@ -41,6 +47,17 @@ async function setDefaultEmailAddress(userId, email, allowUnconfirmed) {
    throw new Errors.UnconfirmedEmailError()
  }

+  await UserAuditLogHandler.promises.addEntry(
+    userId,
+    'change-primary-email',
+    auditLog.initiatorId,
+    auditLog.ipAddress,
+    {
+      newPrimaryEmail: email,
+      oldPrimaryEmail: oldEmail
+    }
+  )
+
  const query = { _id: userId, 'emails.email': email }
  const update = { $set: { email } }
  const res = await UserUpdater.promises.updateUser(query, update)
@ -117,7 +134,7 @@ const UserUpdater = {
  // default email and removing the old email.  Prefer manipulating multiple
  // emails and the default rather than calling this method directly
  //
-  changeEmailAddress(userId, newEmail, callback) {
+  changeEmailAddress(userId, newEmail, auditLog, callback) {
    newEmail = EmailHelper.parseEmail(newEmail)
    if (newEmail == null) {
      return callback(new Error('invalid email'))
@ -132,7 +149,14 @@ const UserUpdater = {
            cb(error)
          }),
        cb => UserUpdater.addEmailAddress(userId, newEmail, cb),
-        cb => UserUpdater.setDefaultEmailAddress(userId, newEmail, true, cb),
+        cb =>
+          UserUpdater.setDefaultEmailAddress(
+            userId,
+            newEmail,
+            true,
+            auditLog,
+            cb
+          ),
        cb => UserUpdater.removeEmailAddress(userId, oldEmail, cb)
      ],
      callback
--- a/services/web/test/acceptance/src/UserEmailsTests.js
+++ b/services/web/test/acceptance/src/UserEmailsTests.js
@ -774,6 +774,69 @@ describe('UserEmails', function() {
        done
      )
    })
+
+    describe('audit log', function() {
+      const originalEmail = 'original@overleaf.com'
+      let otherEmail, response, userHelper, user, userId
+      beforeEach(async function() {
+        otherEmail = 'other@overleaf.com'
+        userHelper = new UserHelper()
+        userHelper = await UserHelper.createUser({
+          email: originalEmail
+        })
+        userHelper = await UserHelper.loginUser({
+          email: originalEmail,
+          password: userHelper.getDefaultPassword()
+        })
+        userId = userHelper.user._id
+        response = await userHelper.request.post({
+          form: {
+            email: otherEmail
+          },
+          simple: false,
+          uri: '/user/emails'
+        })
+        expect(response.statusCode).to.equal(204)
+        const token = await new Promise(resolve => {
+          db.tokens.findOne(
+            {
+              'data.user_id': userId.toString(),
+              'data.email': otherEmail
+            },
+            (error, tokenData) => {
+              expect(error).to.not.exist
+              resolve(tokenData.token)
+            }
+          )
+        })
+        response = await userHelper.request.post(`/user/emails/confirm`, {
+          form: {
+            token
+          },
+          simple: false
+        })
+        expect(response.statusCode).to.equal(200)
+        response = await userHelper.request.post('/user/emails/default', {
+          form: {
+            email: otherEmail
+          },
+          simple: false
+        })
+        expect(response.statusCode).to.equal(200)
+        userHelper = await UserHelper.getUser(userId)
+        user = userHelper.user
+      })
+      it('should be updated', function() {
+        const entry = user.auditLog[user.auditLog.length - 1]
+        expect(typeof entry.initiatorId).to.equal('object')
+        expect(entry.initiatorId).to.deep.equal(user._id)
+        expect(entry.ipAddress).to.equal('127.0.0.1')
+        expect(entry.info).to.deep.equal({
+          newPrimaryEmail: otherEmail,
+          oldPrimaryEmail: originalEmail
+        })
+      })
+    })
  })

  describe('when not logged in', function() {
--- a/services/web/test/unit/src/User/UserAuditLogHandlerTests.js
+++ b/services/web/test/unit/src/User/UserAuditLogHandlerTests.js
@ -40,10 +40,12 @@ describe('UserAuditLogHandler', function() {

  describe('addEntry', function() {
    describe('success', function() {
-      beforeEach(async function() {
+      beforeEach(function() {
        this.dbUpdate = this.UserMock.expects('updateOne')
          .chain('exec')
          .resolves({ nModified: 1 })
+      })
+      it('writes a log', async function() {
        await this.UserAuditLogHandler.promises.addEntry(
          this.userId,
          this.action.operation,
@ -51,30 +53,80 @@ describe('UserAuditLogHandler', function() {
          this.action.ip,
          this.action.info
        )
+        this.UserMock.verify()
      })

-      it('writes a log', async function() {
+      it('updates the log for password reset operation witout a initiatorId', async function() {
+        await expect(
+          this.UserAuditLogHandler.promises.addEntry(
+            this.userId,
+            'reset-password',
+            undefined,
+            this.action.ip,
+            this.action.info
+          )
+        )
        this.UserMock.verify()
      })
    })

-    describe('when the user does not exist', function() {
-      beforeEach(function() {
-        this.UserMock.expects('updateOne')
-          .chain('exec')
-          .resolves({ nModified: 0 })
+    describe('errors', function() {
+      describe('when the user does not exist', function() {
+        beforeEach(function() {
+          this.UserMock.expects('updateOne')
+            .chain('exec')
+            .resolves({ nModified: 0 })
+        })
+
+        it('throws an error', async function() {
+          await expect(
+            this.UserAuditLogHandler.promises.addEntry(
+              this.userId,
+              this.action.operation,
+              this.action.initiatorId,
+              this.action.ip,
+              this.action.info
+            )
+          ).to.be.rejected
+        })
      })

-      it('throws an error', async function() {
-        await expect(
-          this.UserAuditLogHandler.promises.addEntry(
-            this.userId,
-            this.action.operation,
-            this.action.initiatorId,
-            this.action.ip,
-            this.action.info
-          )
-        ).to.be.rejected
+      describe('missing parameters', function() {
+        it('throws an error when no operation', async function() {
+          await expect(
+            this.UserAuditLogHandler.promises.addEntry(
+              this.userId,
+              undefined,
+              this.action.initiatorId,
+              this.action.ip,
+              this.action.info
+            )
+          ).to.be.rejected
+        })
+
+        it('throws an error when no IP', async function() {
+          await expect(
+            this.UserAuditLogHandler.promises.addEntry(
+              this.userId,
+              this.action.operation,
+              this.action.initiatorId,
+              undefined,
+              this.action.info
+            )
+          ).to.be.rejected
+        })
+
+        it('throws an error when no initiatorId and not a password reset operation', async function() {
+          await expect(
+            this.UserAuditLogHandler.promises.addEntry(
+              this.userId,
+              this.action.operation,
+              undefined,
+              this.action.ip,
+              this.action.info
+            )
+          ).to.be.rejected
+        })
      })
    })
  })
--- a/services/web/test/unit/src/User/UserControllerTests.js
+++ b/services/web/test/unit/src/User/UserControllerTests.js
@ -303,6 +303,7 @@ describe('UserController', function() {

  describe('updateUserSettings', function() {
    beforeEach(function() {
+      this.auditLog = { initiatorId: this.user_id, ipAddress: this.req.ip }
      this.newEmail = 'hello@world.com'
      this.req.externalAuthenticationSystemUsed = sinon.stub().returns(false)
    })
@ -381,11 +382,11 @@ describe('UserController', function() {

    it('should call the user updater with the new email and user _id', function(done) {
      this.req.body.email = this.newEmail.toUpperCase()
-      this.UserUpdater.changeEmailAddress.callsArgWith(2)
+      this.UserUpdater.changeEmailAddress.callsArgWith(3)
      this.res.sendStatus = code => {
        code.should.equal(200)
        this.UserUpdater.changeEmailAddress
-          .calledWith(this.user_id, this.newEmail)
+          .calledWith(this.user_id, this.newEmail, this.auditLog)
          .should.equal(true)
        done()
      }
@ -394,7 +395,7 @@ describe('UserController', function() {

    it('should update the email on the session', function(done) {
      this.req.body.email = this.newEmail.toUpperCase()
-      this.UserUpdater.changeEmailAddress.callsArgWith(2)
+      this.UserUpdater.changeEmailAddress.callsArgWith(3)
      let callcount = 0
      this.User.findById = (id, cb) => {
        if (++callcount === 2) {
@ -418,7 +419,7 @@ describe('UserController', function() {

    it('should call populateTeamInvites', function(done) {
      this.req.body.email = this.newEmail.toUpperCase()
-      this.UserUpdater.changeEmailAddress.callsArgWith(2)
+      this.UserUpdater.changeEmailAddress.callsArgWith(3)
      this.res.sendStatus = code => {
        code.should.equal(200)
        this.UserHandler.populateTeamInvites
@ -432,7 +433,7 @@ describe('UserController', function() {
    describe('when changeEmailAddress yields an error', function() {
      it('should pass on an error and not send a success status', function(done) {
        this.req.body.email = this.newEmail.toUpperCase()
-        this.UserUpdater.changeEmailAddress.callsArgWith(2, new OError())
+        this.UserUpdater.changeEmailAddress.callsArgWith(3, new OError())
        this.HttpErrorHandler.legacyInternal = sinon.spy(
          (req, res, message, error) => {
            expect(req).to.exist
@ -454,7 +455,7 @@ describe('UserController', function() {
        })
        this.req.body.email = this.newEmail.toUpperCase()
        this.UserUpdater.changeEmailAddress.callsArgWith(
-          2,
+          3,
          new Errors.EmailExistsError()
        )
        this.UserController.updateUserSettings(this.req, this.res)
--- a/services/web/test/unit/src/User/UserUpdaterTests.js
+++ b/services/web/test/unit/src/User/UserUpdaterTests.js
@ -68,7 +68,12 @@ describe('UserUpdater', function() {
        'settings-sharelatex': (this.settings = {}),
        request: (this.request = {}),
        '../Newsletter/NewsletterManager': this.NewsletterManager,
-        '../Subscription/RecurlyWrapper': this.RecurlyWrapper
+        '../Subscription/RecurlyWrapper': this.RecurlyWrapper,
+        './UserAuditLogHandler': (this.UserAuditLogHandler = {
+          promises: {
+            addEntry: sinon.stub().resolves()
+          }
+        })
      }
    })

@ -125,6 +130,10 @@ describe('UserUpdater', function() {

  describe('changeEmailAddress', function() {
    beforeEach(function() {
+      this.auditLog = {
+        initiatorId: 'abc123',
+        ipAddress: '0:0:0:0'
+      }
      this.UserGetter.getUserEmail.callsArgWith(1, null, this.stubbedUser.email)
      this.UserUpdater.addEmailAddress = sinon.stub().callsArgWith(2)
      this.UserUpdater.setDefaultEmailAddress = sinon.stub().yields()
@ -135,6 +144,7 @@ describe('UserUpdater', function() {
      this.UserUpdater.changeEmailAddress(
        this.stubbedUser._id,
        this.newEmail,
+        this.auditLog,
        err => {
          should.not.exist(err)
          this.UserUpdater.addEmailAddress
@ -152,10 +162,15 @@ describe('UserUpdater', function() {
    })

    it('validates email', function(done) {
-      this.UserUpdater.changeEmailAddress(this.stubbedUser._id, 'foo', err => {
-        should.exist(err)
-        done()
-      })
+      this.UserUpdater.changeEmailAddress(
+        this.stubbedUser._id,
+        'foo',
+        this.auditLog,
+        err => {
+          should.exist(err)
+          done()
+        }
+      )
    })

    it('handle error', function(done) {
@ -163,6 +178,7 @@ describe('UserUpdater', function() {
      this.UserUpdater.changeEmailAddress(
        this.stubbedUser._id,
        this.newEmail,
+        this.auditLog,
        err => {
          should.exist(err)
          done()
@ -350,6 +366,10 @@ describe('UserUpdater', function() {

  describe('setDefaultEmailAddress', function() {
    beforeEach(function() {
+      this.auditLog = {
+        initiatorId: this.stubbedUser,
+        ipAddress: '0:0:0:0'
+      }
      this.stubbedUser.emails = [
        {
          email: this.newEmail,
@ -368,6 +388,7 @@ describe('UserUpdater', function() {
        this.stubbedUser._id,
        this.newEmail,
        false,
+        this.auditLog,
        err => {
          should.not.exist(err)
          this.UserUpdater.promises.updateUser
@ -388,6 +409,7 @@ describe('UserUpdater', function() {
        this.stubbedUser._id,
        this.newEmail,
        false,
+        this.auditLog,
        err => {
          should.not.exist(err)
          this.NewsletterManager.promises.changeEmail
@ -408,6 +430,7 @@ describe('UserUpdater', function() {
        this.stubbedUser._id,
        this.newEmail,
        false,
+        this.auditLog,
        err => {
          should.exist(err)
          done()
@ -422,6 +445,7 @@ describe('UserUpdater', function() {
        this.stubbedUser._id,
        this.newEmail,
        false,
+        this.auditLog,
        err => {
          should.exist(err)
          done()
@ -434,6 +458,7 @@ describe('UserUpdater', function() {
        this.stubbedUser._id,
        '.edu',
        false,
+        this.auditLog,
        err => {
          should.exist(err)
          done()
@ -441,6 +466,49 @@ describe('UserUpdater', function() {
      )
    })

+    it('updates audit log', function(done) {
+      this.UserUpdater.promises.updateUser = sinon.stub().resolves({ n: 1 })
+
+      this.UserUpdater.setDefaultEmailAddress(
+        this.stubbedUser._id,
+        this.newEmail,
+        false,
+        this.auditLog,
+        error => {
+          expect(error).to.not.exist
+          expect(
+            this.UserAuditLogHandler.promises.addEntry
+          ).to.have.been.calledWith(
+            this.stubbedUser._id,
+            'change-primary-email',
+            this.auditLog.initiatorId,
+            this.auditLog.ipAddress,
+            {
+              newPrimaryEmail: this.newEmail,
+              oldPrimaryEmail: this.stubbedUser.email
+            }
+          )
+          done()
+        }
+      )
+    })
+
+    it('blocks email update if audit log returns an error', function(done) {
+      this.UserUpdater.promises.updateUser = sinon.stub()
+      this.UserAuditLogHandler.promises.addEntry.rejects(new Error('oops'))
+      this.UserUpdater.setDefaultEmailAddress(
+        this.stubbedUser._id,
+        this.newEmail,
+        false,
+        this.auditLog,
+        error => {
+          expect(error).to.exist
+          expect(this.UserUpdater.promises.updateUser).to.not.have.been.called
+          done()
+        }
+      )
+    })
+
    describe('when email not confirmed', function() {
      beforeEach(function() {
        this.stubbedUser.emails = [
@ -457,6 +525,7 @@ describe('UserUpdater', function() {
          this.stubbedUser._id,
          this.newEmail,
          false,
+          this.auditLog,
          error => {
            expect(error).to.exist
            expect(error.name).to.equal('UnconfirmedEmailError')
@ -481,6 +550,7 @@ describe('UserUpdater', function() {
          this.stubbedUser._id,
          this.newEmail,
          false,
+          this.auditLog,
          error => {
            expect(error).to.exist
            expect(error.name).to.equal('Error')