Merge pull request #17091 from overleaf/jel-promisify-password-reset

[web] Promisify password reset

GitOrigin-RevId: bc8399727a86276b1d5baa380369d988772c268a

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

@@ -104,6 +104,41 @@ async function setNewUserPassword(req, res, next) {
   AuthenticationController.finishLogin(user, req, res, next)
 }
 
+async function requestReset(req, res, next) {
+  const email = EmailsHelper.parseEmail(req.body.email)
+  if (!email) {
+    return res.status(400).json({
+      message: req.i18n.translate('must_be_email_address'),
+    })
+  }
+
+  let status
+  try {
+    status = await PasswordResetHandler.promises.generateAndEmailResetToken(
+      email
+    )
+  } catch (err) {
+    OError.tag(err, 'failed to generate and email password reset token', {
+      email,
+    })
+    throw err
+  }
+
+  if (status === 'primary') {
+    return res.status(200).json({
+      message: req.i18n.translate('password_reset_email_sent'),
+    })
+  } else if (status === 'secondary') {
+    return res.status(404).json({
+      message: req.i18n.translate('secondary_email_password_reset'),
+    })
+  } else {
+    return res.status(404).json({
+      message: req.i18n.translate('cant_find_email'),
+    })
+  }
+}
+
 module.exports = {
   renderRequestResetForm(req, res) {
     const errorQuery = req.query.error
@@ -117,34 +152,7 @@ module.exports = {
     })
   },
 
-  requestReset(req, res, next) {
-    const email = EmailsHelper.parseEmail(req.body.email)
-    if (!email) {
-      return res.status(400).json({
-        message: req.i18n.translate('must_be_email_address'),
-      })
-    }
-    PasswordResetHandler.generateAndEmailResetToken(email, (err, status) => {
-      if (err != null) {
-        OError.tag(err, 'failed to generate and email password reset token', {
-          email,
-        })
-        next(err)
-      } else if (status === 'primary') {
-        res.status(200).json({
-          message: req.i18n.translate('password_reset_email_sent'),
-        })
-      } else if (status === 'secondary') {
-        res.status(404).json({
-          message: req.i18n.translate('secondary_email_password_reset'),
-        })
-      } else {
-        res.status(404).json({
-          message: req.i18n.translate('cant_find_email'),
-        })
-      }
-    })
-  },
+  requestReset: expressify(requestReset),
 
   renderSetPasswordForm(req, res) {
     if (req.query.passwordResetToken != null) {

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

@@ -8,35 +8,32 @@ const { callbackify, promisify } = require('util')
 
 const AUDIT_LOG_TOKEN_PREFIX_LENGTH = 10
 
-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 }
-    OneTimeTokenHandler.getNewToken('password', data, (err, token) => {
-      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')
-      })
-    })
-  })
+async function generateAndEmailResetToken(email) {
+  const user = await UserGetter.promises.getUserByAnyEmail(email)
+
+  if (!user) {
+    return null
+  }
+
+  if (user.email !== email) {
+    return 'secondary'
+  }
+
+  const data = { user_id: user._id.toString(), email }
+  const token = await OneTimeTokenHandler.promises.getNewToken('password', data)
+
+  const emailOptions = {
+    to: email,
+    setNewPasswordUrl: `${
+      settings.siteUrl
+    }/user/password/set?passwordResetToken=${token}&email=${encodeURIComponent(
+      email
+    )}`,
+  }
+
+  await EmailHandler.promises.sendEmail('passwordResetRequested', emailOptions)
+
+  return 'primary'
 }
 
 function expirePasswordResetToken(token, callback) {
@@ -120,7 +117,7 @@ async function setNewUserPassword(token, password, auditLog) {
 }
 
 const PasswordResetHandler = {
-  generateAndEmailResetToken,
+  generateAndEmailResetToken: callbackify(generateAndEmailResetToken),
 
   setNewUserPassword: callbackify(setNewUserPassword),
 
@@ -130,6 +127,7 @@ const PasswordResetHandler = {
 }
 
 PasswordResetHandler.promises = {
+  generateAndEmailResetToken,
   getUserForPasswordResetToken: promisify(
     PasswordResetHandler.getUserForPasswordResetToken
   ),

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

@@ -35,6 +35,7 @@ describe('PasswordResetController', function () {
     this.PasswordResetHandler = {
       generateAndEmailResetToken: sinon.stub(),
       promises: {
+        generateAndEmailResetToken: sinon.stub(),
         setNewUserPassword: sinon
           .stub()
           .resolves({ found: true, reset: true, userID: this.user_id }),
@@ -79,27 +80,24 @@ describe('PasswordResetController', function () {
   })
 
   describe('requestReset', function () {
-    beforeEach(function () {
-      this.res.json = sinon.stub()
-    })
     it('should tell the handler to process that email', function (done) {
-      this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith(
-        1,
-        null,
+      this.PasswordResetHandler.promises.generateAndEmailResetToken.resolves(
         'primary'
       )
+      this.res.callback = () => {
+        this.res.statusCode.should.equal(200)
+        this.res.json.calledWith(sinon.match.has('message')).should.equal(true)
+        expect(
+          this.PasswordResetHandler.promises.generateAndEmailResetToken.lastCall
+            .args[0]
+        ).equal(this.email)
+        done()
+      }
       this.PasswordResetController.requestReset(this.req, this.res)
-      this.PasswordResetHandler.generateAndEmailResetToken
-        .calledWith(this.email)
-        .should.equal(true)
-      this.res.statusCode.should.equal(200)
-      this.res.json.calledWith(sinon.match.has('message')).should.equal(true)
-      done()
     })
 
     it('should send a 500 if there is an error', function (done) {
-      this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith(
-        1,
+      this.PasswordResetHandler.promises.generateAndEmailResetToken.rejects(
         new Error('error')
       )
       this.PasswordResetController.requestReset(this.req, this.res, error => {
@@ -109,44 +107,41 @@ describe('PasswordResetController', function () {
     })
 
     it("should send a 404 if the email doesn't exist", function (done) {
-      this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith(
-        1,
-        null,
+      this.PasswordResetHandler.promises.generateAndEmailResetToken.resolves(
         null
       )
+      this.res.callback = () => {
+        this.res.statusCode.should.equal(404)
+        this.res.json.calledWith(sinon.match.has('message')).should.equal(true)
+        done()
+      }
       this.PasswordResetController.requestReset(this.req, this.res)
-      this.res.statusCode.should.equal(404)
-      this.res.json.calledWith(sinon.match.has('message')).should.equal(true)
-      done()
     })
 
     it('should send a 404 if the email is registered as a secondard email', function (done) {
-      this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith(
-        1,
-        null,
+      this.PasswordResetHandler.promises.generateAndEmailResetToken.resolves(
         'secondary'
       )
+      this.res.callback = () => {
+        this.res.statusCode.should.equal(404)
+        this.res.json.calledWith(sinon.match.has('message')).should.equal(true)
+        done()
+      }
       this.PasswordResetController.requestReset(this.req, this.res)
-      this.res.statusCode.should.equal(404)
-      this.res.json.calledWith(sinon.match.has('message')).should.equal(true)
-      done()
     })
 
     it('should normalize the email address', function (done) {
       this.email = '  UPperCaseEMAILWithSpacesAround@example.Com '
       this.req.body.email = this.email
-      this.PasswordResetHandler.generateAndEmailResetToken.callsArgWith(
-        1,
-        null,
+      this.PasswordResetHandler.promises.generateAndEmailResetToken.resolves(
         'primary'
       )
+      this.res.callback = () => {
+        this.res.statusCode.should.equal(200)
+        this.res.json.calledWith(sinon.match.has('message')).should.equal(true)
+        done()
+      }
       this.PasswordResetController.requestReset(this.req, this.res)
-      this.PasswordResetHandler.generateAndEmailResetToken
-        .calledWith(this.email.toLowerCase().trim())
-        .should.equal(true)
-      this.res.statusCode.should.equal(200)
-      this.res.json.calledWith(sinon.match.has('message')).should.equal(true)
-      done()
     })
   })
 

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

@@ -1,18 +1,4 @@
-/* eslint-disable
-    n/handle-callback-err,
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const SandboxedModule = require('sandboxed-module')
-const assert = require('assert')
 const path = require('path')
 const sinon = require('sinon')
 const { expect } = require('chai')
@@ -25,16 +11,20 @@ describe('PasswordResetHandler', function () {
   beforeEach(function () {
     this.settings = { siteUrl: 'https://www.overleaf.com' }
     this.OneTimeTokenHandler = {
-      getNewToken: sinon.stub(),
+      promises: {
+        getNewToken: sinon.stub(),
+      },
       peekValueFromToken: sinon.stub(),
       expireToken: sinon.stub(),
     }
     this.UserGetter = {
       getUserByMainEmail: sinon.stub(),
       getUser: sinon.stub(),
-      getUserByAnyEmail: sinon.stub(),
+      promises: {
+        getUserByAnyEmail: sinon.stub(),
+      },
     }
-    this.EmailHandler = { sendEmail: sinon.stub() }
+    this.EmailHandler = { promises: { sendEmail: sinon.stub() } }
     this.AuthenticationManager = {
       setUserPasswordInV2: sinon.stub(),
       promises: {
@@ -70,26 +60,27 @@ describe('PasswordResetHandler', function () {
 
   describe('generateAndEmailResetToken', function () {
     it('should check the user exists', function () {
-      this.UserGetter.getUserByAnyEmail.yields()
+      this.UserGetter.promises.getUserByAnyEmail.resolves()
       this.PasswordResetHandler.generateAndEmailResetToken(
         this.user.email,
         this.callback
       )
-      this.UserGetter.getUserByAnyEmail.should.have.been.calledWith(
+      this.UserGetter.promises.getUserByAnyEmail.should.have.been.calledWith(
         this.user.email
       )
     })
 
     it('should send the email with the token', function (done) {
-      this.UserGetter.getUserByAnyEmail.yields(null, this.user)
-      this.OneTimeTokenHandler.getNewToken.yields(null, this.token)
-      this.EmailHandler.sendEmail.yields()
+      this.UserGetter.promises.getUserByAnyEmail.resolves(this.user)
+      this.OneTimeTokenHandler.promises.getNewToken.resolves(this.token)
+      this.EmailHandler.promises.sendEmail.resolves()
       this.PasswordResetHandler.generateAndEmailResetToken(
         this.user.email,
         (err, status) => {
-          this.EmailHandler.sendEmail.called.should.equal(true)
+          expect(err).to.not.exist
+          this.EmailHandler.promises.sendEmail.called.should.equal(true)
           status.should.equal('primary')
-          const args = this.EmailHandler.sendEmail.args[0]
+          const args = this.EmailHandler.promises.sendEmail.args[0]
           args[0].should.equal('passwordResetRequested')
           args[1].setNewPasswordUrl.should.equal(
             `${this.settings.siteUrl}/user/password/set?passwordResetToken=${
@@ -101,19 +92,32 @@ describe('PasswordResetHandler', function () {
       )
     })
 
+    it('should return errors from getUserByAnyEmail', function (done) {
+      const err = new Error('oops')
+      this.UserGetter.promises.getUserByAnyEmail.rejects(err)
+      this.PasswordResetHandler.generateAndEmailResetToken(
+        this.user.email,
+        err => {
+          expect(err).to.equal(err)
+          done()
+        }
+      )
+    })
+
     describe('when the email exists', function () {
-      beforeEach(function () {
-        this.UserGetter.getUserByAnyEmail.yields(null, this.user)
-        this.OneTimeTokenHandler.getNewToken.yields(null, this.token)
-        this.EmailHandler.sendEmail.yields()
-        this.PasswordResetHandler.generateAndEmailResetToken(
-          this.email,
-          this.callback
-        )
+      let result
+      beforeEach(async function () {
+        this.UserGetter.promises.getUserByAnyEmail.resolves(this.user)
+        this.OneTimeTokenHandler.promises.getNewToken.resolves(this.token)
+        this.EmailHandler.promises.sendEmail.resolves()
+        result =
+          await this.PasswordResetHandler.promises.generateAndEmailResetToken(
+            this.email
+          )
       })
 
       it('should set the password token data to the user id and email', function () {
-        this.OneTimeTokenHandler.getNewToken.should.have.been.calledWith(
+        this.OneTimeTokenHandler.promises.getNewToken.should.have.been.calledWith(
           'password',
           {
             email: this.email,
@@ -123,8 +127,8 @@ describe('PasswordResetHandler', function () {
       })
 
       it('should send an email with the token', function () {
-        this.EmailHandler.sendEmail.called.should.equal(true)
-        const args = this.EmailHandler.sendEmail.args[0]
+        this.EmailHandler.promises.sendEmail.called.should.equal(true)
+        const args = this.EmailHandler.promises.sendEmail.args[0]
         args[0].should.equal('passwordResetRequested')
         args[1].setNewPasswordUrl.should.equal(
           `${this.settings.siteUrl}/user/password/set?passwordResetToken=${
@@ -133,52 +137,54 @@ describe('PasswordResetHandler', function () {
         )
       })
 
-      it('should return status == true', function () {
-        this.callback.calledWith(null, 'primary').should.equal(true)
+      it('should return status == true', async function () {
+        expect(result).to.equal('primary')
       })
     })
 
     describe("when the email doesn't exist", function () {
-      beforeEach(function () {
-        this.UserGetter.getUserByAnyEmail.yields(null, null)
-        this.PasswordResetHandler.generateAndEmailResetToken(
-          this.email,
-          this.callback
-        )
+      let result
+      beforeEach(async function () {
+        this.UserGetter.promises.getUserByAnyEmail.resolves(null)
+        result =
+          await this.PasswordResetHandler.promises.generateAndEmailResetToken(
+            this.email
+          )
       })
 
       it('should not set the password token data', function () {
-        this.OneTimeTokenHandler.getNewToken.called.should.equal(false)
+        this.OneTimeTokenHandler.promises.getNewToken.called.should.equal(false)
       })
 
       it('should send an email with the token', function () {
-        this.EmailHandler.sendEmail.called.should.equal(false)
+        this.EmailHandler.promises.sendEmail.called.should.equal(false)
       })
 
       it('should return status == null', function () {
-        this.callback.calledWith(null, null).should.equal(true)
+        expect(result).to.equal(null)
       })
     })
 
     describe('when the email is a secondary email', function () {
-      beforeEach(function () {
-        this.UserGetter.getUserByAnyEmail.callsArgWith(1, null, this.user)
-        this.PasswordResetHandler.generateAndEmailResetToken(
-          'secondary@email.com',
-          this.callback
-        )
+      let result
+      beforeEach(async function () {
+        this.UserGetter.promises.getUserByAnyEmail.resolves(this.user)
+        result =
+          await this.PasswordResetHandler.promises.generateAndEmailResetToken(
+            'secondary@email.com'
+          )
       })
 
       it('should not set the password token data', function () {
-        this.OneTimeTokenHandler.getNewToken.called.should.equal(false)
+        this.OneTimeTokenHandler.promises.getNewToken.called.should.equal(false)
       })
 
       it('should not send an email with the token', function () {
-        this.EmailHandler.sendEmail.called.should.equal(false)
+        this.EmailHandler.promises.sendEmail.called.should.equal(false)
       })
 
       it('should return status == secondary', function () {
-        this.callback.calledWith(null, 'secondary').should.equal(true)
+        expect(result).to.equal('secondary')
       })
     })
   })
@@ -238,7 +244,7 @@ describe('PasswordResetHandler', function () {
             this.password,
             this.auditLog,
             (err, result) => {
-              const { found, reset, userId } = result
+              const { found, reset } = result
               expect(err).to.not.exist
               expect(found).to.be.false
               expect(reset).to.be.false
@@ -263,7 +269,7 @@ describe('PasswordResetHandler', function () {
             this.password,
             this.auditLog,
             (err, result) => {
-              const { found, reset, userId } = result
+              const { found, reset } = result
               expect(err).to.not.exist
               expect(found).to.be.false
               expect(reset).to.be.false
@@ -289,7 +295,6 @@ describe('PasswordResetHandler', function () {
               this.password,
               this.auditLog,
               (error, result) => {
-                const { reset, userId } = result
                 sinon.assert.calledWith(
                   this.UserAuditLogHandler.promises.addEntry,
                   this.user_id,
@@ -343,7 +348,6 @@ describe('PasswordResetHandler', function () {
                 this.password,
                 this.auditLog,
                 (error, result) => {
-                  const { reset, userId } = result
                   expect(error).to.not.exist
                   sinon.assert.calledWith(
                     this.UserAuditLogHandler.promises.addEntry,
@@ -444,7 +448,7 @@ describe('PasswordResetHandler', function () {
             this.password,
             this.auditLog,
             (err, result) => {
-              const { reset, userId } = result
+              const { reset } = result
               expect(err).to.not.exist
               expect(reset).to.be.false
               expect(this.OneTimeTokenHandler.expireToken.called).to.equal(
@@ -471,7 +475,7 @@ describe('PasswordResetHandler', function () {
             this.password,
             this.auditLog,
             (err, result) => {
-              const { reset, userId } = result
+              const { reset } = result
               expect(err).to.not.exist
               expect(reset).to.be.false
               expect(this.OneTimeTokenHandler.expireToken.called).to.equal(