Merge pull request #3079 from overleaf/jel-log-password-update

Update audit log when password updated

GitOrigin-RevId: 3228e39e8a3682d6e77264cd6ee580f3fc40642a

services/web/app/src/Features/Authentication/AuthenticationManager.js

@@ -220,7 +220,8 @@ const AuthenticationManager = {
 
 AuthenticationManager.promises = {
   authenticate: util.promisify(AuthenticationManager.authenticate),
-  hashPassword: util.promisify(AuthenticationManager.hashPassword)
+  hashPassword: util.promisify(AuthenticationManager.hashPassword),
+  setUserPassword: util.promisify(AuthenticationManager.setUserPassword)
 }
 
 module.exports = AuthenticationManager

services/web/app/src/Features/Email/EmailBuilder.js

@@ -225,21 +225,6 @@ If you didn't request a password reset, let us know.\
   }
 })
 
-templates.passwordChanged = NoCTAEmailTemplate({
-  subject(opts) {
-    return `Password Changed - ${settings.appName}`
-  },
-  title(opts) {
-    return `Password Changed`
-  },
-  message(opts) {
-    return [
-      `We're contacting you to notify you that your password has been set or changed.`,
-      `If you have recently set your password for the first time, or if you just changed your password, you don't need to take any further action. If you didn't set or change your password, please contact us.`
-    ]
-  }
-})
-
 templates.confirmEmail = CTAEmailTemplate({
   subject() {
     return `Confirm Email - ${settings.appName}`

services/web/app/src/Features/User/UserController.js

@@ -29,6 +29,71 @@ async function _ensureAffiliation(userId, emailData) {
   }
 }
 
+async function changePassword(req, res, next) {
+  metrics.inc('user.password-change')
+  const userId = AuthenticationController.getLoggedInUserId(req)
+
+  const user = await AuthenticationManager.promises.authenticate(
+    { _id: userId },
+    req.body.currentPassword
+  )
+  if (!user) {
+    return HttpErrorHandler.badRequest(req, res, 'Your old password is wrong')
+  }
+
+  if (req.body.newPassword1 !== req.body.newPassword2) {
+    return HttpErrorHandler.badRequest(
+      req,
+      res,
+      req.i18n.translate('password_change_passwords_do_not_match')
+    )
+  }
+  const validationError = AuthenticationManager.validatePassword(
+    req.body.newPassword1
+  )
+  if (validationError != null) {
+    return HttpErrorHandler.badRequest(req, res, validationError.message)
+  }
+
+  await UserAuditLogHandler.promises.addEntry(
+    user._id,
+    'update-password',
+    user._id,
+    req.ip
+  )
+
+  await AuthenticationManager.promises.setUserPassword(
+    user._id,
+    req.body.newPassword1
+  )
+
+  const emailOptions = {
+    to: user.email,
+    actionDescribed: `your password has been changed on your account ${
+      user.email
+    }`,
+    action: 'password changed'
+  }
+  EmailHandler.sendEmail('securityAlert', emailOptions, error => {
+    if (error) {
+      // log error when sending security alert email but do not pass back
+      logger.error({ err: error })
+    }
+  })
+
+  await UserSessionsManager.promises.revokeAllUserSessions(user, [
+    req.sessionID
+  ])
+
+  return res.json({
+    message: {
+      type: 'success',
+      email: user.email,
+      text: req.i18n.translate('password_change_successful')
+    }
+  })
+}
+
 async function clearSessions(req, res, next) {
   metrics.inc('user.clear-sessions')
   const user = AuthenticationController.getSessionUser(req)
@@ -394,84 +459,7 @@ const UserController = {
     )
   },
 
-  changePassword(req, res, next) {
-    metrics.inc('user.password-change')
-    const internalError = {
-      message: { type: 'error', text: req.i18n.translate('internal_error') }
-    }
-    const userId = AuthenticationController.getLoggedInUserId(req)
-    AuthenticationManager.authenticate(
-      { _id: userId },
-      req.body.currentPassword,
-      (err, user) => {
-        if (err) {
-          return res.status(500).json(internalError)
-        }
-        if (!user) {
-          return res.status(400).json({
-            message: {
-              type: 'error',
-              text: 'Your old password is wrong'
-            }
-          })
-        }
-        if (req.body.newPassword1 !== req.body.newPassword2) {
-          return res.status(400).json({
-            message: {
-              type: 'error',
-              text: req.i18n.translate('password_change_passwords_do_not_match')
-            }
-          })
-        }
-        const validationError = AuthenticationManager.validatePassword(
-          req.body.newPassword1
-        )
-        if (validationError != null) {
-          return res.status(400).json({
-            message: {
-              type: 'error',
-              text: validationError.message
-            }
-          })
-        }
-        AuthenticationManager.setUserPassword(
-          user._id,
-          req.body.newPassword1,
-          err => {
-            if (err) {
-              return res.status(500).json(internalError)
-            }
-            // log errors but do not wait for response
-            EmailHandler.sendEmail(
-              'passwordChanged',
-              { to: user.email },
-              err => {
-                if (err) {
-                  logger.warn(err)
-                }
-              }
-            )
-            UserSessionsManager.revokeAllUserSessions(
-              user,
-              [req.sessionID],
-              err => {
-                if (err != null) {
-                  return res.status(500).json(internalError)
-                }
-                res.json({
-                  message: {
-                    type: 'success',
-                    email: user.email,
-                    text: req.i18n.translate('password_change_successful')
-                  }
-                })
-              }
-            )
-          }
-        )
-      }
-    )
-  }
+  changePassword: expressify(changePassword)
 }
 
 UserController.promises = {

services/web/test/acceptance/src/PasswordUpdateTests.js

@@ -0,0 +1,133 @@
+const { expect } = require('chai')
+const RateLimiter = require('../../../app/src/infrastructure/RateLimiter')
+const UserHelper = require('./helpers/UserHelper')
+
+describe('PasswordUpdate', function() {
+  let email, password, response, user, userHelper
+  afterEach(async function() {
+    await RateLimiter.promises.clearRateLimit(
+      'password_reset_rate_limit',
+      '127.0.0.1'
+    )
+  })
+  beforeEach(async function() {
+    userHelper = new UserHelper()
+    email = userHelper.getDefaultEmail()
+    password = 'old-password'
+    userHelper = await UserHelper.createUser({ email, password })
+    userHelper = await UserHelper.loginUser({
+      email,
+      password
+    })
+    await userHelper.getCsrfToken()
+  })
+  describe('success', function() {
+    beforeEach(async function() {
+      response = await userHelper.request.post('/user/password/update', {
+        form: {
+          currentPassword: password,
+          newPassword1: 'new-password',
+          newPassword2: 'new-password'
+        },
+        simple: false
+      })
+      user = (await UserHelper.getUser({ email })).user
+    })
+    it('should return 200', async function() {
+      expect(response.statusCode).to.equal(200)
+    })
+    it('should update the audit log', function() {
+      expect(user.auditLog[0]).to.exist
+      expect(typeof user.auditLog[0].initiatorId).to.equal('object')
+      expect(user.auditLog[0].initiatorId).to.deep.equal(user._id)
+      expect(user.auditLog[0].operation).to.equal('update-password')
+      expect(user.auditLog[0].ipAddress).to.equal('127.0.0.1')
+      expect(user.auditLog[0].timestamp).to.exist
+    })
+  })
+  describe('errors', function() {
+    describe('missing current password', function() {
+      beforeEach(async function() {
+        response = await userHelper.request.post('/user/password/update', {
+          form: {
+            newPassword1: 'new-password',
+            newPassword2: 'new-password'
+          },
+          simple: false
+        })
+        user = (await UserHelper.getUser({ email })).user
+      })
+      it('should return 500', async function() {
+        expect(response.statusCode).to.equal(500)
+      })
+      it('should not update audit log', async function() {
+        expect(user.auditLog[0]).to.not.exist
+      })
+    })
+    describe('wrong current password', function() {
+      beforeEach(async function() {
+        response = await userHelper.request.post('/user/password/update', {
+          form: {
+            currentPassword: 'wrong-password',
+            newPassword1: 'new-password',
+            newPassword2: 'new-password'
+          },
+          simple: false
+        })
+        user = (await UserHelper.getUser({ email })).user
+      })
+      it('should return 400', async function() {
+        expect(response.statusCode).to.equal(400)
+      })
+      it('should not update audit log', async function() {
+        expect(user.auditLog[0]).to.not.exist
+      })
+    })
+    describe('newPassword1 does not match newPassword2', function() {
+      beforeEach(async function() {
+        response = await userHelper.request.post('/user/password/update', {
+          form: {
+            currentPassword: password,
+            newPassword1: 'new-password',
+            newPassword2: 'oops-password'
+          },
+          json: true,
+          simple: false
+        })
+        user = (await UserHelper.getUser({ email })).user
+      })
+      it('should return 400', async function() {
+        expect(response.statusCode).to.equal(400)
+      })
+      it('should return error message', async function() {
+        expect(response.body.message).to.equal('Passwords do not match')
+      })
+      it('should not update audit log', async function() {
+        expect(user.auditLog[0]).to.not.exist
+      })
+    })
+    describe('new password is not valid', function() {
+      beforeEach(async function() {
+        response = await userHelper.request.post('/user/password/update', {
+          form: {
+            currentPassword: password,
+            newPassword1: 'short',
+            newPassword2: 'short'
+          },
+          json: true,
+          simple: false
+        })
+        user = (await UserHelper.getUser({ email })).user
+      })
+      it('should return 400', async function() {
+        expect(response.statusCode).to.equal(400)
+      })
+      it('should return error message', async function() {
+        expect(response.body.message).to.equal('password is too short')
+      })
+      it('should not update audit log', async function() {
+        expect(user.auditLog[0]).to.not.exist
+      })
+    })
+  })
+})

services/web/test/unit/src/User/UserControllerTests.js

@@ -13,6 +13,7 @@ describe('UserController', function() {
 
     this.user = {
       _id: this.user_id,
+      email: 'email@overleaf.com',
       save: sinon.stub().callsArgWith(0),
       ace: {}
     }
@@ -31,6 +32,7 @@ describe('UserController', function() {
       i18n: {
         translate: text => text
       },
+      ip: '0:0:0:0',
       query: {}
     }
 
@@ -50,8 +52,11 @@ describe('UserController', function() {
     }
     this.AuthenticationManager = {
       authenticate: sinon.stub(),
-      setUserPassword: sinon.stub(),
-      validatePassword: sinon.stub()
+      validatePassword: sinon.stub(),
+      promises: {
+        authenticate: sinon.stub(),
+        setUserPassword: sinon.stub()
+      }
     }
     this.ReferalAllocator = { allocate: sinon.stub() }
     this.SubscriptionDomainHandler = { autoAllocate: sinon.stub() }
@@ -75,6 +80,7 @@ describe('UserController', function() {
     }
     this.SudoModeHandler = { clearSudoMode: sinon.stub() }
     this.HttpErrorHandler = {
+      badRequest: sinon.stub(),
       conflict: sinon.stub(),
       unprocessableEntity: sinon.stub(),
       legacyInternal: sinon.stub()
@@ -648,59 +654,163 @@ describe('UserController', function() {
   })
 
   describe('changePassword', function() {
-    it('should check the old password is the current one at the moment', function() {
-      this.AuthenticationManager.authenticate.yields()
-      this.req.body = { currentPassword: 'oldpasshere' }
-      this.UserController.changePassword(this.req, this.res, this.callback)
-      this.AuthenticationManager.authenticate.should.have.been.calledWith(
-        { _id: this.user._id },
-        'oldpasshere'
-      )
-      this.AuthenticationManager.setUserPassword.callCount.should.equal(0)
-    })
-
-    it('it should not set the new password if they do not match', function() {
-      this.AuthenticationManager.authenticate.yields(null, {})
-      this.req.body = {
-        newPassword1: '1',
-        newPassword2: '2'
-      }
-      this.UserController.changePassword(this.req, this.res, this.callback)
-      this.res.status.should.have.been.calledWith(400)
-      this.AuthenticationManager.setUserPassword.callCount.should.equal(0)
-    })
+    describe('success', function() {
+      beforeEach(function() {
+        this.AuthenticationManager.promises.authenticate.resolves(this.user)
+        this.AuthenticationManager.promises.setUserPassword.resolves()
+        this.req.body = {
+          newPassword1: 'newpass',
+          newPassword2: 'newpass'
+        }
+      })
+      it('should set the new password if they do match', function(done) {
+        this.res.json.callsFake(() => {
+          this.AuthenticationManager.promises.setUserPassword.should.have.been.calledWith(
+            this.user._id,
+            'newpass'
+          )
+          done()
+        })
+        this.UserController.changePassword(this.req, this.res)
+      })
 
-    it('should set the new password if they do match', function() {
-      this.AuthenticationManager.authenticate.yields(null, this.user)
-      this.AuthenticationManager.setUserPassword.yields()
-      this.req.body = {
-        newPassword1: 'newpass',
-        newPassword2: 'newpass'
-      }
-      this.UserController.changePassword(this.req, this.res, this.callback)
-      this.AuthenticationManager.setUserPassword.should.have.been.calledWith(
-        this.user._id,
-        'newpass'
-      )
+      it('should log the update', function(done) {
+        this.res.json.callsFake(() => {
+          this.UserAuditLogHandler.promises.addEntry.should.have.been.calledWith(
+            this.user._id,
+            'update-password',
+            this.user._id,
+            this.req.ip
+          )
+          this.AuthenticationManager.promises.setUserPassword.callCount.should.equal(
+            1
+          )
+          done()
+        })
+        this.UserController.changePassword(this.req, this.res)
+      })
+
+      it('should send security alert email', function(done) {
+        this.res.json.callsFake(() => {
+          const expectedArg = {
+            to: this.user.email,
+            actionDescribed: `your password has been changed on your account ${
+              this.user.email
+            }`,
+            action: 'password changed'
+          }
+          const emailCall = this.EmailHandler.sendEmail.lastCall
+          expect(emailCall.args[0]).to.equal('securityAlert')
+          expect(emailCall.args[1]).to.deep.equal(expectedArg)
+          done()
+        })
+        this.UserController.changePassword(this.req, this.res)
+      })
     })
 
-    it('it should not set the new password if it is invalid', function() {
-      this.AuthenticationManager.validatePassword = sinon
-        .stub()
-        .returns({ message: 'validation-error' })
-      this.AuthenticationManager.authenticate.yields(null, {})
-      this.req.body = {
-        newPassword1: 'newpass',
-        newPassword2: 'newpass'
-      }
-      this.UserController.changePassword(this.req, this.res, this.callback)
-      this.AuthenticationManager.setUserPassword.callCount.should.equal(0)
-      this.res.status.should.have.been.calledWith(400)
-      this.res.json.should.have.been.calledWith({
-        message: {
-          type: 'error',
-          text: 'validation-error'
+    describe('errors', function() {
+      it('should check the old password is the current one at the moment', function(done) {
+        this.AuthenticationManager.promises.authenticate.resolves()
+        this.req.body = { currentPassword: 'oldpasshere' }
+        this.HttpErrorHandler.badRequest.callsFake(() => {
+          expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith(
+            this.req,
+            this.res,
+            'Your old password is wrong'
+          )
+          this.AuthenticationManager.promises.authenticate.should.have.been.calledWith(
+            { _id: this.user._id },
+            'oldpasshere'
+          )
+          this.AuthenticationManager.promises.setUserPassword.callCount.should.equal(
+            0
+          )
+          done()
+        })
+        this.UserController.changePassword(this.req, this.res)
+      })
+
+      it('it should not set the new password if they do not match', function(done) {
+        this.AuthenticationManager.promises.authenticate.resolves({})
+        this.req.body = {
+          newPassword1: '1',
+          newPassword2: '2'
+        }
+        this.HttpErrorHandler.badRequest.callsFake(() => {
+          expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith(
+            this.req,
+            this.res,
+            'password_change_passwords_do_not_match'
+          )
+          this.AuthenticationManager.promises.setUserPassword.callCount.should.equal(
+            0
+          )
+          done()
+        })
+        this.UserController.changePassword(this.req, this.res)
+      })
+
+      it('it should not set the new password if it is invalid', function(done) {
+        this.AuthenticationManager.validatePassword = sinon
+          .stub()
+          .returns({ message: 'validation-error' })
+        this.AuthenticationManager.promises.authenticate.resolves({})
+        this.req.body = {
+          newPassword1: 'newpass',
+          newPassword2: 'newpass'
         }
+        this.HttpErrorHandler.badRequest.callsFake(() => {
+          expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith(
+            this.req,
+            this.res,
+            'validation-error'
+          )
+          this.AuthenticationManager.promises.setUserPassword.callCount.should.equal(
+            0
+          )
+          done()
+        })
+        this.UserController.changePassword(this.req, this.res)
+      })
+
+      describe('UserAuditLogHandler error', function() {
+        it('should return error and not update password', function(done) {
+          this.UserAuditLogHandler.promises.addEntry.rejects(new Error('oops'))
+          this.AuthenticationManager.promises.authenticate.resolves(this.user)
+          this.AuthenticationManager.promises.setUserPassword.resolves()
+          this.req.body = {
+            newPassword1: 'newpass',
+            newPassword2: 'newpass'
+          }
+
+          this.UserController.changePassword(this.req, this.res, error => {
+            expect(error).to.be.instanceof(Error)
+            this.AuthenticationManager.promises.setUserPassword.callCount.should.equal(
+              0
+            )
+            done()
+          })
+        })
+      })
+
+      describe('EmailHandler error', function() {
+        beforeEach(function() {
+          this.AuthenticationManager.promises.authenticate.resolves(this.user)
+          this.AuthenticationManager.promises.setUserPassword.resolves()
+          this.req.body = {
+            newPassword1: 'newpass',
+            newPassword2: 'newpass'
+          }
+          this.EmailHandler.sendEmail.yields(new Error('oops'))
+        })
+        it('should not return error but should log it', function(done) {
+          this.res.json.callsFake(result => {
+            expect(result.message.type).to.equal('success')
+            this.logger.error.callCount.should.equal(1)
+            done()
+          })
+          this.UserController.changePassword(this.req, this.res)
+        })
       })
     })
   })