Merge pull request #3078 from overleaf/jel-log-password-reset-by-token

Update audit log when password reset by token

GitOrigin-RevId: 2ae7f59c5cdf2723e541a99c58c36564cc82adbf

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

@@ -26,7 +26,7 @@ function send401WithChallenge(res) {
   res.sendStatus(401)
 }
 
-const AuthenticationController = (module.exports = {
+const AuthenticationController = {
   serializeUser(user, callback) {
     if (!user._id || !user.email) {
       const err = new Error('serializeUser called with non-user object')
@@ -431,7 +431,7 @@ const AuthenticationController = (module.exports = {
       delete req.session.postLoginRedirect
     }
   }
-})
+}
 
 function _afterLoginSessionSetup(req, user, callback) {
   if (callback == null) {
@@ -495,3 +495,5 @@ function _loginAsyncHandlers(req, user) {
   // capture the request ip for use when creating the session
   return (user._login_req_ip = req.ip)
 }
+
+module.exports = AuthenticationController

services/web/app/src/Features/PasswordReset/PasswordResetController.js

@@ -6,6 +6,57 @@ const UserGetter = require('../User/UserGetter')
 const UserUpdater = require('../User/UserUpdater')
 const UserSessionsManager = require('../User/UserSessionsManager')
 const logger = require('logger-sharelatex')
+const { expressify } = require('../../util/promises')
+
+async function setNewUserPassword(req, res, next) {
+  let user
+  let { passwordResetToken, password } = req.body
+  if (!passwordResetToken || !password) {
+    return res.sendStatus(400)
+  }
+  passwordResetToken = passwordResetToken.trim()
+  if (AuthenticationManager.validatePassword(password) != null) {
+    return res.sendStatus(400)
+  }
+  delete req.session.resetToken
+
+  const initiatorId = AuthenticationController.getLoggedInUserId(req)
+  // password reset via tokens can be done while logged in, or not
+  const auditLog = {
+    initiatorId,
+    ip: req.ip
+  }
+
+  try {
+    const result = await PasswordResetHandler.promises.setNewUserPassword(
+      passwordResetToken,
+      password,
+      auditLog
+    )
+    let { found, reset, userId } = result
+    if (!found) return res.sendStatus(404)
+    if (!reset) return res.sendStatus(500)
+    await UserSessionsManager.promises.revokeAllUserSessions(
+      { _id: userId },
+      []
+    )
+    await UserUpdater.promises.removeReconfirmFlag(userId)
+    if (!req.session.doLoginAfterPasswordReset) {
+      return res.sendStatus(200)
+    }
+    user = await UserGetter.promises.getUser(userId)
+  } catch (error) {
+    if (error.name === 'NotFoundError') {
+      return res.sendStatus(404)
+    } else if (error.name === 'InvalidPasswordError') {
+      return res.sendStatus(400)
+    } else {
+      return res.sendStatus(500)
+    }
+  }
+
+  AuthenticationController.finishLogin(user, req, res, next)
+}
 
 module.exports = {
   renderRequestResetForm(req, res) {
@@ -67,53 +118,5 @@ module.exports = {
     })
   },
 
-  setNewUserPassword(req, res, next) {
-    let { passwordResetToken, password } = req.body
-    if (!passwordResetToken || !password) {
-      return res.sendStatus(400)
-    }
-    passwordResetToken = passwordResetToken.trim()
-    if (AuthenticationManager.validatePassword(password) != null) {
-      return res.sendStatus(400)
-    }
-    delete req.session.resetToken
-    PasswordResetHandler.setNewUserPassword(
-      passwordResetToken,
-      password,
-      (err, found, userId) => {
-        if ((err && err.name === 'NotFoundError') || !found) {
-          return res.status(404).send('NotFoundError')
-        } else if (err) {
-          return res.status(500)
-        }
-        UserSessionsManager.revokeAllUserSessions({ _id: userId }, [], err => {
-          if (err != null) {
-            return next(err)
-          }
-          UserUpdater.removeReconfirmFlag(userId, err => {
-            if (err != null) {
-              return next(err)
-            }
-            if (!req.session.doLoginAfterPasswordReset) {
-              return res.sendStatus(200)
-            }
-            UserGetter.getUser(userId, (err, user) => {
-              if (err != null) {
-                return next(err)
-              }
-              AuthenticationController.finishLogin(user, req, res, err => {
-                if (err != null) {
-                  logger.err(
-                    { err, email: user.email },
-                    'Error setting up session after setting password'
-                  )
-                }
-                next(err)
-              })
-            })
-          })
-        })
-      }
-    )
-  }
+  setNewUserPassword: expressify(setNewUserPassword)
 }

services/web/app/src/Features/PasswordReset/PasswordResetHandler.js

@@ -1,104 +1,126 @@
 const settings = require('settings-sharelatex')
+const UserAuditLogHandler = require('../User/UserAuditLogHandler')
 const UserGetter = require('../User/UserGetter')
 const OneTimeTokenHandler = require('../Security/OneTimeTokenHandler')
 const EmailHandler = require('../Email/EmailHandler')
 const AuthenticationManager = require('../Authentication/AuthenticationManager')
+const { callbackify, promisify } = require('util')
 
-const PasswordResetHandler = {
-  generateAndEmailResetToken(email, callback) {
-    UserGetter.getUserByAnyEmail(email, (err, user) => {
-      if (err || !user) {
-        return callback(err, null)
+function generateAndEmailResetToken(email, callback) {
+  UserGetter.getUserByAnyEmail(email, (err, user) => {
+    if (err || !user) {
+      return callback(err, null)
+    }
+    if (user.email !== email) {
+      return callback(null, 'secondary')
+    }
+    const data = { user_id: user._id.toString(), email: email }
+    OneTimeTokenHandler.getNewToken('password', data, (err, token) => {
+      if (err) {
+        return callback(err)
       }
-      if (user.email !== email) {
-        return callback(null, 'secondary')
+      const emailOptions = {
+        to: email,
+        setNewPasswordUrl: `${
+          settings.siteUrl
+        }/user/password/set?passwordResetToken=${token}&email=${encodeURIComponent(
+          email
+        )}`
       }
-      const data = { user_id: user._id.toString(), email: email }
-      OneTimeTokenHandler.getNewToken('password', data, (err, token) => {
+      EmailHandler.sendEmail('passwordResetRequested', emailOptions, err => {
         if (err) {
           return callback(err)
         }
-        const emailOptions = {
-          to: email,
-          setNewPasswordUrl: `${
-            settings.siteUrl
-          }/user/password/set?passwordResetToken=${token}&email=${encodeURIComponent(
-            email
-          )}`
-        }
-        EmailHandler.sendEmail('passwordResetRequested', emailOptions, err => {
-          if (err) {
-            return callback(err)
-          }
-          callback(null, 'primary')
-        })
+        callback(null, 'primary')
       })
     })
-  },
+  })
+}
 
-  setNewUserPassword(token, password, callback) {
-    PasswordResetHandler.getUserForPasswordResetToken(token, (err, user) => {
-      if (err) {
-        return callback(err)
+function getUserForPasswordResetToken(token, callback) {
+  OneTimeTokenHandler.getValueFromTokenAndExpire(
+    'password',
+    token,
+    (err, data) => {
+      if (err != null) {
+        if (err.name === 'NotFoundError') {
+          return callback(null, null)
+        } else {
+          return callback(err)
+        }
       }
-      if (!user) {
-        return callback(null, false, null)
+      if (data == null || data.email == null) {
+        return callback(null, null)
       }
-      AuthenticationManager.setUserPassword(
-        user._id,
-        password,
-        (err, reset) => {
-          if (err) {
-            return callback(err)
+      UserGetter.getUserByMainEmail(
+        data.email,
+        { _id: 1, 'overleaf.id': 1 },
+        (err, user) => {
+          if (err != null) {
+            callback(err)
+          } else if (user == null) {
+            callback(null, null)
+          } else if (
+            data.user_id != null &&
+            data.user_id === user._id.toString()
+          ) {
+            callback(null, user)
+          } else if (
+            data.v1_user_id != null &&
+            user.overleaf != null &&
+            data.v1_user_id === user.overleaf.id
+          ) {
+            callback(null, user)
+          } else {
+            callback(null, null)
           }
-          callback(null, reset, user._id)
         }
       )
-    })
-  },
+    }
+  )
+}
 
-  getUserForPasswordResetToken(token, callback) {
-    OneTimeTokenHandler.getValueFromTokenAndExpire(
-      'password',
-      token,
-      (err, data) => {
-        if (err != null) {
-          if (err.name === 'NotFoundError') {
-            return callback(null, null)
-          } else {
-            return callback(err)
-          }
-        }
-        if (data == null || data.email == null) {
-          return callback(null, null)
-        }
-        UserGetter.getUserByMainEmail(
-          data.email,
-          { _id: 1, 'overleaf.id': 1 },
-          (err, user) => {
-            if (err != null) {
-              callback(err)
-            } else if (user == null) {
-              callback(null, null)
-            } else if (
-              data.user_id != null &&
-              data.user_id === user._id.toString()
-            ) {
-              callback(null, user)
-            } else if (
-              data.v1_user_id != null &&
-              user.overleaf != null &&
-              data.v1_user_id === user.overleaf.id
-            ) {
-              callback(null, user)
-            } else {
-              callback(null, null)
-            }
-          }
-        )
-      }
-    )
+async function setNewUserPassword(token, password, auditLog) {
+  const user = await PasswordResetHandler.promises.getUserForPasswordResetToken(
+    token
+  )
+
+  if (!user) {
+    return {
+      found: false,
+      reset: false,
+      userId: null
+    }
   }
+
+  await UserAuditLogHandler.promises.addEntry(
+    user._id,
+    'reset-password',
+    auditLog.initiatorId,
+    auditLog.ip
+  )
+
+  const reset = await AuthenticationManager.promises.setUserPassword(
+    user._id,
+    password
+  )
+
+  return { found: true, reset, userId: user._id }
+}
+
+const PasswordResetHandler = {
+  generateAndEmailResetToken,
+
+  setNewUserPassword: callbackify(setNewUserPassword),
+
+  getUserForPasswordResetToken
+}
+
+PasswordResetHandler.promises = {
+  getUserForPasswordResetToken: promisify(
+    PasswordResetHandler.getUserForPasswordResetToken
+  ),
+  setNewUserPassword
 }
 
 module.exports = PasswordResetHandler

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

@@ -332,7 +332,8 @@ const promises = {
   addEmailAddress: promisify(UserUpdater.addEmailAddress),
   confirmEmail: promisify(UserUpdater.confirmEmail),
   setDefaultEmailAddress,
-  updateUser: promisify(UserUpdater.updateUser)
+  updateUser: promisify(UserUpdater.updateUser),
+  removeReconfirmFlag: promisify(UserUpdater.removeReconfirmFlag)
 }
 
 UserUpdater.promises = promises

services/web/app/views/user/setPassword.pug

@@ -20,15 +20,16 @@ block content
 								| #{translate("password_has_been_reset")}. 
 								a(href='/login') #{translate("login_here")}
 							div(ng-show="passwordResetForm.response.error == true")
-								.alert.alert-danger(ng-show="passwordResetForm.response.data == 'NotFoundError'")
-										| #{translate('password_reset_token_expired')}
-										br
-										a(href="/user/password/reset"  style="text-decoration: underline;")
-											| Request a new password reset email
-								.alert.alert-danger(ng-show="passwordResetForm.response.status == 400")
-										| #{translate('invalid_password')}
-								.alert.alert-danger(ng-show="passwordResetForm.response.status == 500")
-										| #{translate('error_performing_request')}
+								div(ng-switch="passwordResetForm.response.status")
+									.alert.alert-danger(ng-switch-when="404")
+											| #{translate('password_reset_token_expired')}
+											br
+											a(href="/user/password/reset")
+												| Request a new password reset email
+									.alert.alert-danger(ng-switch-when="400")
+											| #{translate('invalid_password')}
+									.alert.alert-danger(ng-switch-default)
+											| #{translate('error_performing_request')}
 
 
 							.form-group

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

@@ -0,0 +1,213 @@
+const { expect } = require('chai')
+const RateLimiter = require('../../../app/src/infrastructure/RateLimiter')
+const UserHelper = require('./helpers/UserHelper')
+const { db } = require('../../../app/src/infrastructure/mongojs')
+
+describe('PasswordReset', function() {
+  let email, response, user, userHelper, token
+  afterEach(async function() {
+    await RateLimiter.promises.clearRateLimit(
+      'password_reset_rate_limit',
+      '127.0.0.1'
+    )
+  })
+  beforeEach(async function() {
+    userHelper = new UserHelper()
+    email = userHelper.getDefaultEmail()
+    userHelper = await UserHelper.createUser({ email })
+    user = userHelper.user
+
+    // generate the token
+    await userHelper.getCsrfToken()
+    response = await userHelper.request.post('/user/password/reset', {
+      form: {
+        email
+      }
+    })
+
+    token = await new Promise(resolve => {
+      db.tokens.findOne(
+        { 'data.user_id': user._id.toString() },
+        (error, tokenData) => {
+          expect(error).to.not.exist
+          resolve(tokenData.token)
+        }
+      )
+    })
+  })
+  describe('with a valid token', function() {
+    describe('when logged in', function() {
+      beforeEach(async function() {
+        userHelper = await UserHelper.loginUser({
+          email,
+          password: userHelper.getDefaultPassword()
+        })
+        response = await userHelper.request.get(
+          `/user/password/set?passwordResetToken=${token}&email=${email}`,
+          { simple: false }
+        )
+        expect(response.statusCode).to.equal(302)
+        expect(response.headers.location).to.equal('/user/password/set')
+        // send reset request
+        response = await userHelper.request.post('/user/password/set', {
+          form: {
+            passwordResetToken: token,
+            password: 'a-password'
+          }
+        })
+        userHelper = await UserHelper.getUser({ email })
+        user = userHelper.user
+      })
+      it('update the password', async function() {
+        expect(user.hashedPassword).to.exist
+        expect(user.password).to.not.exist
+      })
+      it('log the change with initiatorId', async function() {
+        expect(user.auditLog).to.exist
+        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('reset-password')
+        expect(user.auditLog[0].ipAddress).to.equal('127.0.0.1')
+        expect(user.auditLog[0].timestamp).to.exist
+      })
+    })
+    describe('when logged in as another user', function() {
+      let otherUser, otherUserEmail
+      beforeEach(async function() {
+        otherUserEmail = userHelper.getDefaultEmail()
+        userHelper = await UserHelper.createUser({ email: otherUserEmail })
+        otherUser = userHelper.user
+        userHelper = await UserHelper.loginUser({
+          email: otherUserEmail,
+          password: userHelper.getDefaultPassword()
+        })
+        response = await userHelper.request.get(
+          `/user/password/set?passwordResetToken=${token}&email=${email}`,
+          { simple: false }
+        )
+        expect(response.statusCode).to.equal(302)
+        expect(response.headers.location).to.equal('/user/password/set')
+        // send reset request
+        response = await userHelper.request.post('/user/password/set', {
+          form: {
+            passwordResetToken: token,
+            password: 'a-password'
+          }
+        })
+        userHelper = await UserHelper.getUser({ email })
+        user = userHelper.user
+      })
+      it('update the password', async function() {
+        expect(user.hashedPassword).to.exist
+        expect(user.password).to.not.exist
+      })
+      it('log the change with the logged in user as the initiatorId', async function() {
+        expect(user.auditLog).to.exist
+        expect(user.auditLog[0]).to.exist
+        expect(typeof user.auditLog[0].initiatorId).to.equal('object')
+        expect(user.auditLog[0].initiatorId).to.deep.equal(otherUser._id)
+        expect(user.auditLog[0].operation).to.equal('reset-password')
+        expect(user.auditLog[0].ipAddress).to.equal('127.0.0.1')
+        expect(user.auditLog[0].timestamp).to.exist
+      })
+    })
+    describe('when not logged in', function() {
+      beforeEach(async function() {
+        response = await userHelper.request.get(
+          `/user/password/set?passwordResetToken=${token}&email=${email}`,
+          { simple: false }
+        )
+        expect(response.statusCode).to.equal(302)
+        expect(response.headers.location).to.equal('/user/password/set')
+        // send reset request
+        response = await userHelper.request.post('/user/password/set', {
+          form: {
+            passwordResetToken: token,
+            password: 'a-password'
+          }
+        })
+        userHelper = await UserHelper.getUser({ email })
+        user = userHelper.user
+      })
+      it('updates the password', function() {
+        expect(user.hashedPassword).to.exist
+        expect(user.password).to.not.exist
+      })
+      it('log the change', async function() {
+        expect(user.auditLog).to.exist
+        expect(user.auditLog[0]).to.exist
+        expect(user.auditLog[0].initiatorId).to.equal(null)
+        expect(user.auditLog[0].operation).to.equal('reset-password')
+        expect(user.auditLog[0].ipAddress).to.equal('127.0.0.1')
+        expect(user.auditLog[0].timestamp).to.exist
+      })
+    })
+    describe('password checks', function() {
+      beforeEach(async function() {
+        response = await userHelper.request.get(
+          `/user/password/set?passwordResetToken=${token}&email=${email}`,
+          { simple: false }
+        )
+        expect(response.statusCode).to.equal(302)
+        expect(response.headers.location).to.equal('/user/password/set')
+      })
+      it('without a password should return 400 and not log the change', async function() {
+        // send reset request
+        response = await userHelper.request.post('/user/password/set', {
+          form: {
+            passwordResetToken: token
+          },
+          simple: false
+        })
+        expect(response.statusCode).to.equal(400)
+        userHelper = await UserHelper.getUser({ email })
+        user = userHelper.user
+        expect(user.auditLog).to.deep.equal([])
+      })
+
+      it('without a valid password should return 400 and not log the change', async function() {
+        // send reset request
+        response = await userHelper.request.post('/user/password/set', {
+          form: {
+            passwordResetToken: token,
+            password: 'short'
+          },
+          simple: false
+        })
+        expect(response.statusCode).to.equal(400)
+        userHelper = await UserHelper.getUser({ email })
+        user = userHelper.user
+        expect(user.auditLog).to.deep.equal([])
+      })
+    })
+  })
+  describe('without a valid token', function() {
+    it('no token should redirect to page to re-request reset token', async function() {
+      response = await userHelper.request.get(
+        `/user/password/set?&email=${email}`,
+        { simple: false }
+      )
+      expect(response.statusCode).to.equal(302)
+      expect(response.headers.location).to.equal('/user/password/reset')
+    })
+    it('should return 404 for invalid tokens', async function() {
+      const invalidToken = 'not-real-token'
+      response = await userHelper.request.get(
+        `/user/password/set?&passwordResetToken=${invalidToken}&email=${email}`,
+        { simple: false }
+      )
+      expect(response.statusCode).to.equal(302)
+      expect(response.headers.location).to.equal('/user/password/set')
+      // send reset request
+      response = await userHelper.request.post('/user/password/set', {
+        form: {
+          passwordResetToken: invalidToken,
+          password: 'a-password'
+        },
+        simple: false
+      })
+      expect(response.statusCode).to.equal(404)
+    })
+  })
+})

services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js

@@ -32,15 +32,25 @@ describe('PasswordResetController', function() {
     this.settings = {}
     this.PasswordResetHandler = {
       generateAndEmailResetToken: sinon.stub(),
-      setNewUserPassword: sinon.stub().yields(null, true, this.user_id)
+      promises: {
+        setNewUserPassword: sinon
+          .stub()
+          .resolves({ found: true, reset: true, userID: this.user_id })
+      }
     }
     this.RateLimiter = { addCount: sinon.stub() }
     this.UserSessionsManager = {
-      revokeAllUserSessions: sinon.stub().callsArgWith(2, null)
+      promises: {
+        revokeAllUserSessions: sinon.stub().resolves()
+      }
+    }
+    this.AuthenticationManager = {
+      validatePassword: sinon.stub()
     }
-    this.AuthenticationManager = { validatePassword: sinon.stub() }
     this.UserUpdater = {
-      removeReconfirmFlag: sinon.stub().callsArgWith(1, null)
+      promises: {
+        removeReconfirmFlag: sinon.stub().resolves()
+      }
     }
     this.PasswordResetController = SandboxedModule.require(MODULE_PATH, {
       globals: {
@@ -52,12 +62,20 @@ describe('PasswordResetController', function() {
         'logger-sharelatex': {
           log() {},
           warn() {},
+          err: sinon.stub(),
           error() {}
         },
         '../../infrastructure/RateLimiter': this.RateLimiter,
-        '../Authentication/AuthenticationController': (this.AuthenticationController = {}),
+        '../Authentication/AuthenticationController': (this.AuthenticationController = {
+          getLoggedInUserId: sinon.stub(),
+          finishLogin: sinon.stub()
+        }),
         '../Authentication/AuthenticationManager': this.AuthenticationManager,
-        '../User/UserGetter': (this.UserGetter = {}),
+        '../User/UserGetter': (this.UserGetter = {
+          promises: {
+            getUser: sinon.stub()
+          }
+        }),
         '../User/UserSessionsManager': this.UserSessionsManager,
         '../User/UserUpdater': this.UserUpdater
       }
@@ -156,7 +174,7 @@ describe('PasswordResetController', function() {
     it('should tell the user handler to reset the password', function(done) {
       this.res.sendStatus = code => {
         code.should.equal(200)
-        this.PasswordResetHandler.setNewUserPassword
+        this.PasswordResetHandler.promises.setNewUserPassword
           .calledWith(this.token, this.password)
           .should.equal(true)
         done()
@@ -168,7 +186,7 @@ describe('PasswordResetController', function() {
       this.password = this.req.body.password = ' oh! clever! spaces around!   '
       this.res.sendStatus = code => {
         code.should.equal(200)
-        this.PasswordResetHandler.setNewUserPassword.should.have.been.calledWith(
+        this.PasswordResetHandler.promises.setNewUserPassword.should.have.been.calledWith(
           this.token,
           this.password
         )
@@ -177,24 +195,39 @@ describe('PasswordResetController', function() {
       this.PasswordResetController.setNewUserPassword(this.req, this.res)
     })
 
-    it("should send 404 if the token didn't work", function(done) {
-      this.PasswordResetHandler.setNewUserPassword.yields(
-        null,
-        false,
-        this.user_id
-      )
-      this.res.status = code => {
+    it('should send 404 if the token was not found', function(done) {
+      this.PasswordResetHandler.promises.setNewUserPassword.resolves({
+        found: false,
+        reset: false,
+        userId: this.user_id
+      })
+      this.res.sendStatus = code => {
         code.should.equal(404)
         done()
       }
       this.PasswordResetController.setNewUserPassword(this.req, this.res)
     })
 
+    it('should return 500 if not reset', function(done) {
+      this.PasswordResetHandler.promises.setNewUserPassword.resolves({
+        found: true,
+        reset: false,
+        userId: this.user_id
+      })
+      this.res.sendStatus = code => {
+        code.should.equal(500)
+        done()
+      }
+      this.PasswordResetController.setNewUserPassword(this.req, this.res)
+    })
+
     it('should return 400 (Bad Request) if there is no password', function(done) {
       this.req.body.password = ''
       this.res.sendStatus = code => {
         code.should.equal(400)
-        this.PasswordResetHandler.setNewUserPassword.called.should.equal(false)
+        this.PasswordResetHandler.promises.setNewUserPassword.called.should.equal(
+          false
+        )
         done()
       }
       this.PasswordResetController.setNewUserPassword(this.req, this.res)
@@ -204,7 +237,9 @@ describe('PasswordResetController', function() {
       this.req.body.passwordResetToken = ''
       this.res.sendStatus = code => {
         code.should.equal(400)
-        this.PasswordResetHandler.setNewUserPassword.called.should.equal(false)
+        this.PasswordResetHandler.promises.setNewUserPassword.called.should.equal(
+          false
+        )
         done()
       }
       this.PasswordResetController.setNewUserPassword(this.req, this.res)
@@ -217,7 +252,9 @@ describe('PasswordResetController', function() {
         .returns({ message: 'password contains invalid characters' })
       this.res.sendStatus = code => {
         code.should.equal(400)
-        this.PasswordResetHandler.setNewUserPassword.called.should.equal(false)
+        this.PasswordResetHandler.promises.setNewUserPassword.called.should.equal(
+          false
+        )
         done()
       }
       this.PasswordResetController.setNewUserPassword(this.req, this.res)
@@ -234,7 +271,9 @@ describe('PasswordResetController', function() {
 
     it('should clear sessions', function(done) {
       this.res.sendStatus = code => {
-        this.UserSessionsManager.revokeAllUserSessions.callCount.should.equal(1)
+        this.UserSessionsManager.promises.revokeAllUserSessions.callCount.should.equal(
+          1
+        )
         done()
       }
       this.PasswordResetController.setNewUserPassword(this.req, this.res)
@@ -242,38 +281,60 @@ describe('PasswordResetController', function() {
 
     it('should call removeReconfirmFlag', function(done) {
       this.res.sendStatus = code => {
-        this.UserUpdater.removeReconfirmFlag.callCount.should.equal(1)
+        this.UserUpdater.promises.removeReconfirmFlag.callCount.should.equal(1)
         done()
       }
       this.PasswordResetController.setNewUserPassword(this.req, this.res)
     })
 
+    describe('catch errors', function() {
+      it('should return 404 for NotFoundError', function(done) {
+        const anError = new Error('oops')
+        anError.name = 'NotFoundError'
+        this.PasswordResetHandler.promises.setNewUserPassword.rejects(anError)
+        this.res.sendStatus = code => {
+          code.should.equal(404)
+          done()
+        }
+        this.PasswordResetController.setNewUserPassword(this.req, this.res)
+      })
+      it('should return 400 for InvalidPasswordError', function(done) {
+        const anError = new Error('oops')
+        anError.name = 'InvalidPasswordError'
+        this.PasswordResetHandler.promises.setNewUserPassword.rejects(anError)
+        this.res.sendStatus = code => {
+          code.should.equal(400)
+          done()
+        }
+        this.PasswordResetController.setNewUserPassword(this.req, this.res)
+      })
+      it('should return 500 for other errors', function(done) {
+        const anError = new Error('oops')
+        this.PasswordResetHandler.promises.setNewUserPassword.rejects(anError)
+        this.res.sendStatus = code => {
+          code.should.equal(500)
+          done()
+        }
+        this.PasswordResetController.setNewUserPassword(this.req, this.res)
+      })
+    })
+
     describe('when doLoginAfterPasswordReset is set', function() {
       beforeEach(function() {
-        this.UserGetter.getUser = sinon
-          .stub()
-          .callsArgWith(1, null, { email: 'joe@example.com' })
+        this.user = {
+          _id: this.userId,
+          email: 'joe@example.com'
+        }
+        this.UserGetter.promises.getUser.resolves(this.user)
         this.req.session.doLoginAfterPasswordReset = 'true'
-        this.res.json = sinon.stub()
-        this.AuthenticationController.finishLogin = sinon.stub().yields()
-        this.AuthenticationController._getRedirectFromSession = sinon
-          .stub()
-          .returns('/some/path')
       })
 
       it('should login user', function(done) {
-        this.PasswordResetController.setNewUserPassword(
-          this.req,
-          this.res,
-          err => {
-            expect(err).to.not.exist
-            this.AuthenticationController.finishLogin.callCount.should.equal(1)
-            this.AuthenticationController.finishLogin
-              .calledWith({ email: 'joe@example.com' }, this.req)
-              .should.equal(true)
-            done()
-          }
-        )
+        this.AuthenticationController.finishLogin.callsFake((...args) => {
+          expect(args[0]).to.equal(this.user)
+          done()
+        })
+        this.PasswordResetController.setNewUserPassword(this.req, this.res)
       })
     })
   })

services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js

@@ -39,13 +39,20 @@ describe('PasswordResetHandler', function() {
     }
     this.EmailHandler = { sendEmail: sinon.stub() }
     this.AuthenticationManager = {
-      setUserPassword: sinon.stub(),
       setUserPasswordInV1: sinon.stub(),
-      setUserPasswordInV2: sinon.stub()
+      setUserPasswordInV2: sinon.stub(),
+      promises: {
+        setUserPassword: sinon.stub().resolves()
+      }
     }
     this.PasswordResetHandler = SandboxedModule.require(modulePath, {
       globals: { console: console },
       requires: {
+        '../User/UserAuditLogHandler': (this.UserAuditLogHandler = {
+          promises: {
+            addEntry: sinon.stub().resolves()
+          }
+        }),
         '../User/UserGetter': this.UserGetter,
         '../Security/OneTimeTokenHandler': this.OneTimeTokenHandler,
         '../Email/EmailHandler': this.EmailHandler,
@@ -59,7 +66,7 @@ describe('PasswordResetHandler', function() {
     })
     this.token = '12312321i'
     this.user_id = 'user_id_here'
-    this.user = { email: (this.email = 'bob@bob.com'), _id: 'user-id' }
+    this.user = { email: (this.email = 'bob@bob.com'), _id: this.user_id }
     this.password = 'my great secret password'
     this.callback = sinon.stub()
     // this should not have any effect now
@@ -186,19 +193,29 @@ describe('PasswordResetHandler', function() {
   })
 
   describe('setNewUserPassword', function() {
+    beforeEach(function() {
+      this.auditLog = { ip: '0:0:0:0' }
+    })
     describe('when no data is found', function() {
       beforeEach(function() {
         this.OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, null)
+      })
+
+      it('should return found == false and reset == false', function() {
         this.PasswordResetHandler.setNewUserPassword(
           this.token,
           this.password,
-          this.callback
+          this.auditLog,
+          (error, result) => {
+            expect(error).to.not.exist
+            expect(result).to.deep.equal({
+              found: false,
+              reset: false,
+              userId: null
+            })
+          }
         )
       })
-
-      it('should return exists == false', function() {
-        this.callback.calledWith(null, false).should.equal(true)
-      })
     })
 
     describe('when the token has a user_id and email', function() {
@@ -209,9 +226,9 @@ describe('PasswordResetHandler', function() {
             user_id: this.user._id,
             email: this.email
           })
-        this.AuthenticationManager.setUserPassword
+        this.AuthenticationManager.promises.setUserPassword
           .withArgs(this.user._id, this.password)
-          .yields(null, true, this.user._id)
+          .resolves(true)
       })
 
       describe('when no user is found with this email', function() {
@@ -221,15 +238,16 @@ describe('PasswordResetHandler', function() {
             .yields(null, null)
         })
 
-        it('should return found == false', function(done) {
+        it('should return found == false and reset == false', function(done) {
           this.PasswordResetHandler.setNewUserPassword(
             this.token,
             this.password,
-            (err, found) => {
-              if (err != null) {
-                return done(err)
-              }
+            this.auditLog,
+            (err, result) => {
+              const { found, reset, userId } = result
+              expect(err).to.not.exist
               expect(found).to.be.false
+              expect(reset).to.be.false
               done()
             }
           )
@@ -243,15 +261,16 @@ describe('PasswordResetHandler', function() {
             .yields(null, { _id: 'not-the-same', email: this.email })
         })
 
-        it('should return found == false', function(done) {
+        it('should return found == false and reset == false', function(done) {
           this.PasswordResetHandler.setNewUserPassword(
             this.token,
             this.password,
-            (err, found) => {
-              if (err != null) {
-                return done(err)
-              }
+            this.auditLog,
+            (err, result) => {
+              const { found, reset, userId } = result
+              expect(err).to.not.exist
               expect(found).to.be.false
+              expect(reset).to.be.false
               done()
             }
           )
@@ -259,26 +278,100 @@ describe('PasswordResetHandler', function() {
       })
 
       describe('when the email and user match', function() {
-        beforeEach(function() {
-          this.PasswordResetHandler.getUserForPasswordResetToken = sinon
-            .stub()
-            .withArgs(this.token)
-            .yields(null, this.user)
-        })
+        describe('success', function() {
+          beforeEach(function() {
+            this.UserGetter.getUserByMainEmail.yields(null, this.user)
+          })
 
-        it('should return found == true and the user id', function(done) {
-          this.PasswordResetHandler.setNewUserPassword(
-            this.token,
-            this.password,
-            (err, found, userId) => {
-              if (err != null) {
-                return done(err)
+          it('should update the user audit log', function(done) {
+            this.PasswordResetHandler.setNewUserPassword(
+              this.token,
+              this.password,
+              this.auditLog,
+              (error, result) => {
+                const { reset, userId } = result
+                expect(error).to.not.exist
+                const logCall = this.UserAuditLogHandler.promises.addEntry
+                  .lastCall
+                expect(logCall.args[0]).to.equal(this.user_id)
+                expect(logCall.args[1]).to.equal('reset-password')
+                expect(logCall.args[2]).to.equal(undefined)
+                expect(logCall.args[3]).to.equal(this.auditLog.ip)
+                expect(logCall.args[4]).to.equal(undefined)
+                done()
               }
-              expect(found).to.be.true
-              expect(userId).to.equal(this.user._id)
-              done()
-            }
-          )
+            )
+          })
+
+          it('should return reset == true and the user id', function(done) {
+            this.PasswordResetHandler.setNewUserPassword(
+              this.token,
+              this.password,
+              this.auditLog,
+              (err, result) => {
+                const { reset, userId } = result
+                expect(err).to.not.exist
+                expect(reset).to.be.true
+                expect(userId).to.equal(this.user._id)
+                done()
+              }
+            )
+          })
+
+          describe('when logged in', function() {
+            beforeEach(function() {
+              this.auditLog.initiatorId = this.user_id
+            })
+            it('should update the user audit log with initiatorId', function(done) {
+              this.PasswordResetHandler.setNewUserPassword(
+                this.token,
+                this.password,
+                this.auditLog,
+                (error, result) => {
+                  const { reset, userId } = result
+                  expect(error).to.not.exist
+                  const logCall = this.UserAuditLogHandler.promises.addEntry
+                    .lastCall
+                  expect(logCall.args[0]).to.equal(this.user_id)
+                  expect(logCall.args[1]).to.equal('reset-password')
+                  expect(logCall.args[2]).to.equal(this.user_id)
+                  expect(logCall.args[3]).to.equal(this.auditLog.ip)
+                  expect(logCall.args[4]).to.equal(undefined)
+                  done()
+                }
+              )
+            })
+          })
+        })
+
+        describe('errors', function() {
+          describe('via UserAuditLogHandler', function() {
+            beforeEach(function() {
+              this.PasswordResetHandler.promises.getUserForPasswordResetToken = sinon
+                .stub()
+                .withArgs(this.token)
+                .resolves(this.user)
+              this.UserAuditLogHandler.promises.addEntry.rejects(
+                new Error('oops')
+              )
+            })
+            it('should return the error and not update the password', function(done) {
+              this.PasswordResetHandler.setNewUserPassword(
+                this.token,
+                this.password,
+                this.auditLog,
+                (error, result) => {
+                  expect(error).to.exist
+                  expect(
+                    this.UserAuditLogHandler.promises.addEntry.callCount
+                  ).to.equal(1)
+                  expect(this.AuthenticationManager.promises.setUserPassword).to
+                    .not.have.been.called
+                  done()
+                }
+              )
+            })
+          })
         })
       })
     })
@@ -292,27 +385,27 @@ describe('PasswordResetHandler', function() {
             v1_user_id: this.user.overleaf.id,
             email: this.email
           })
-        this.AuthenticationManager.setUserPassword
+        this.AuthenticationManager.promises.setUserPassword
           .withArgs(this.user._id, this.password)
-          .yields(null, true)
+          .resolves(true)
       })
 
-      describe('when no user is found with this email', function() {
+      describe('when no user is reset with this email', function() {
         beforeEach(function() {
           this.UserGetter.getUserByMainEmail
             .withArgs(this.email)
             .yields(null, null)
         })
 
-        it('should return found == false', function(done) {
+        it('should return reset == false', function(done) {
           this.PasswordResetHandler.setNewUserPassword(
             this.token,
             this.password,
-            (err, found) => {
-              if (err != null) {
-                return done(err)
-              }
-              expect(found).to.be.false
+            this.auditLog,
+            (err, result) => {
+              const { reset, userId } = result
+              expect(err).to.not.exist
+              expect(reset).to.be.false
               done()
             }
           )
@@ -328,15 +421,15 @@ describe('PasswordResetHandler', function() {
           })
         })
 
-        it('should return found == false', function(done) {
+        it('should return reset == false', function(done) {
           this.PasswordResetHandler.setNewUserPassword(
             this.token,
             this.password,
-            (err, found) => {
-              if (err != null) {
-                return done(err)
-              }
-              expect(found).to.be.false
+            this.auditLog,
+            (err, result) => {
+              const { reset, userId } = result
+              expect(err).to.not.exist
+              expect(reset).to.be.false
               done()
             }
           )
@@ -350,15 +443,15 @@ describe('PasswordResetHandler', function() {
             .yields(null, this.user)
         })
 
-        it('should return found == true and the user id', function(done) {
+        it('should return reset == true and the user id', function(done) {
           this.PasswordResetHandler.setNewUserPassword(
             this.token,
             this.password,
-            (err, found, userId) => {
-              if (err != null) {
-                return done(err)
-              }
-              expect(found).to.be.true
+            this.auditLog,
+            (err, result) => {
+              const { reset, userId } = result
+              expect(err).to.not.exist
+              expect(reset).to.be.true
               expect(userId).to.equal(this.user._id)
               done()
             }