From 4ad6d3cb5f7313e301b436514f60f1a5b575b794 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe <5312836+lawshe@users.noreply.github.com> Date: Fri, 16 Feb 2024 09:44:31 -0600 Subject: [PATCH] Merge pull request #17091 from overleaf/jel-promisify-password-reset [web] Promisify password reset GitOrigin-RevId: bc8399727a86276b1d5baa380369d988772c268a --- .../PasswordReset/PasswordResetController.js | 64 +++++---- .../PasswordReset/PasswordResetHandler.js | 58 ++++---- .../PasswordResetControllerTests.js | 65 +++++---- .../PasswordResetHandlerTests.js | 126 +++++++++--------- 4 files changed, 159 insertions(+), 154 deletions(-) diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index 5bdd23b94d..e1969f58c3 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js +++ b/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) { diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index dee316b0a6..aaf378b53b 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/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 ), diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js index c725329c41..f953775e37 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js +++ b/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() }) }) diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index 01194bfbf5..b13483569a 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/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(