From 3288f87dbe0451a4df1f0be2e81452ef5d4668ee Mon Sep 17 00:00:00 2001 From: June Kelly Date: Tue, 27 Sep 2022 12:24:17 +0100 Subject: [PATCH] [web] Password set/reset: reject current password (redux) (#8956) * [web] set-password: reject same as current password * [web] Add 'peek' operation on tokens This allows us to improve the UX of the reset-password form, by not invalidating the token in the case where the new password will be rejected by validation logic. We give up to three attempts before invalidating the token. * [web] Add hide-on-error feature to async forms This allows us to hide the form elements when certain named error conditions occur. * [web] reset-password: handle same-password rejection We also change the implementation to use the new peekValueFromToken API, and to expire the token explicitely after it has been used to set the new password. * [web] Validate OneTimeToken when loading password reset form * [web] Rate limit GET: /user/password/set Now that we are peeking at OneTimeToken when accessing this page, we add rate to the GET request, matching that of the POST request. * [web] Tidy up pug layout and mongo query for token peeking Co-authored-by: Mathias Jakobsen GitOrigin-RevId: 835205cc7c7ebe1209ee8e5b693efeb939a3056a --- .../Authentication/AuthenticationErrors.js | 2 + .../Authentication/AuthenticationManager.js | 87 ++++++++----- .../PasswordReset/PasswordResetController.js | 40 ++++-- .../PasswordReset/PasswordResetHandler.js | 27 ++-- .../PasswordReset/PasswordResetRouter.js | 10 ++ .../Features/Security/OneTimeTokenHandler.js | 50 ++++++++ .../app/src/Features/User/UserController.js | 6 + services/web/app/views/user/passwordReset.pug | 8 +- services/web/app/views/user/setPassword.pug | 76 ++++++------ .../js/features/form-helpers/hydrate-form.js | 15 +++ services/web/locales/en.json | 1 + .../test/acceptance/src/PasswordResetTests.js | 115 +++++++++++++++++- .../web/test/acceptance/src/SessionTests.js | 2 +- .../web/test/acceptance/src/helpers/User.js | 18 ++- .../AuthenticationManagerTests.js | 33 ++++- .../PasswordResetControllerTests.js | 26 ++++ .../PasswordResetHandlerTests.js | 66 +++++++++- .../src/Security/OneTimeTokenHandlerTests.js | 85 +++++++++++++ 18 files changed, 568 insertions(+), 99 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationErrors.js b/services/web/app/src/Features/Authentication/AuthenticationErrors.js index fa394903b1..c7b4caf470 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationErrors.js +++ b/services/web/app/src/Features/Authentication/AuthenticationErrors.js @@ -3,9 +3,11 @@ const Errors = require('../Errors/Errors') class InvalidEmailError extends Errors.BackwardCompatibleError {} class InvalidPasswordError extends Errors.BackwardCompatibleError {} class ParallelLoginError extends Errors.BackwardCompatibleError {} +class PasswordMustBeDifferentError extends Errors.BackwardCompatibleError {} module.exports = { InvalidEmailError, InvalidPasswordError, ParallelLoginError, + PasswordMustBeDifferentError, } diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 76d41f6131..451d2961ab 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -7,6 +7,7 @@ const { InvalidEmailError, InvalidPasswordError, ParallelLoginError, + PasswordMustBeDifferentError, } = require('./AuthenticationErrors') const util = require('util') const HaveIBeenPwned = require('./HaveIBeenPwned') @@ -24,7 +25,7 @@ const _checkWriteResult = function (result, callback) { } const AuthenticationManager = { - authenticate(query, password, callback) { + _checkUserPassword(query, password, callback) { // Using Mongoose for legacy reasons here. The returned User instance // gets serialized into the session and there may be subtle differences // between the user returned by Mongoose vs mongodb (such as default values) @@ -33,12 +34,28 @@ const AuthenticationManager = { return callback(error) } if (!user || !user.hashedPassword) { - return callback(null, null) + return callback(null, null, null) } bcrypt.compare(password, user.hashedPassword, function (error, match) { if (error) { return callback(error) } + return callback(null, user, match) + }) + }) + }, + + authenticate(query, password, callback) { + AuthenticationManager._checkUserPassword( + query, + password, + (error, user, match) => { + if (error) { + return callback(error) + } + if (!user) { + return callback(null, null) + } const update = { $inc: { loginEpoch: 1 } } if (!match) { update.$set = { lastFailedLogin: new Date() } @@ -71,8 +88,8 @@ const AuthenticationManager = { ) } ) - }) - }) + } + ) }, validateEmail(email) { @@ -182,31 +199,45 @@ const AuthenticationManager = { if (validationError) { return callback(validationError) } - this.hashPassword(password, function (error, hash) { - if (error) { - return callback(error) - } - db.users.updateOne( - { - _id: ObjectId(user._id.toString()), - }, - { - $set: { - hashedPassword: hash, - }, - $unset: { - password: true, - }, - }, - function (updateError, result) { - if (updateError) { - return callback(updateError) - } - _checkWriteResult(result, callback) - HaveIBeenPwned.checkPasswordForReuseInBackground(password) + // check if we can log in with this password. In which case we should reject it, + // because it is the same as the existing password. + AuthenticationManager._checkUserPassword( + { _id: user._id }, + password, + (err, _user, match) => { + if (err) { + return callback(err) } - ) - }) + if (match) { + return callback(new PasswordMustBeDifferentError()) + } + this.hashPassword(password, function (error, hash) { + if (error) { + return callback(error) + } + db.users.updateOne( + { + _id: ObjectId(user._id.toString()), + }, + { + $set: { + hashedPassword: hash, + }, + $unset: { + password: true, + }, + }, + function (updateError, result) { + if (updateError) { + return callback(updateError) + } + _checkWriteResult(result, callback) + HaveIBeenPwned.checkPasswordForReuseInBackground(password) + } + ) + }) + } + ) }, _passwordCharactersAreValid(password) { diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index ebf2d538a9..02fc6c0488 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetController.js @@ -78,6 +78,12 @@ async function setNewUserPassword(req, res, next) { key: 'invalid-password', }, }) + } else if (error.name === 'PasswordMustBeDifferentError') { + return res.status(400).json({ + message: { + key: 'password-must-be-different', + }, + }) } else { return res.status(500).json({ message: req.i18n.translate('error_performing_request'), @@ -92,7 +98,15 @@ async function setNewUserPassword(req, res, next) { module.exports = { renderRequestResetForm(req, res) { - res.render('user/passwordReset', { title: 'reset_password' }) + const errorQuery = req.query.error + let error = null + if (errorQuery === 'token_expired') { + error = 'password_reset_token_expired' + } + res.render('user/passwordReset', { + title: 'reset_password', + error, + }) }, requestReset(req, res, next) { @@ -126,15 +140,23 @@ module.exports = { renderSetPasswordForm(req, res) { if (req.query.passwordResetToken != null) { - req.session.resetToken = req.query.passwordResetToken - let emailQuery = '' - if (typeof req.query.email === 'string') { - const email = EmailsHelper.parseEmail(req.query.email) - if (email) { - emailQuery = `?email=${encodeURIComponent(email)}` + return PasswordResetHandler.getUserForPasswordResetToken( + req.query.passwordResetToken, + (err, user, remainingUses) => { + if (err || !user || remainingUses <= 0) { + return res.redirect('/user/password/reset?error=token_expired') + } + req.session.resetToken = req.query.passwordResetToken + let emailQuery = '' + if (typeof req.query.email === 'string') { + const email = EmailsHelper.parseEmail(req.query.email) + if (email) { + emailQuery = `?email=${encodeURIComponent(email)}` + } + } + return res.redirect('/user/password/set' + emailQuery) } - } - return res.redirect('/user/password/set' + emailQuery) + ) } if (req.session.resetToken == null) { return res.redirect('/user/password/reset') diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index d08d008368..9c703ea6a8 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js @@ -37,11 +37,17 @@ function generateAndEmailResetToken(email, callback) { }) } +function expirePasswordResetToken(token, callback) { + OneTimeTokenHandler.expireToken('password', token, err => { + return callback(err) + }) +} + function getUserForPasswordResetToken(token, callback) { - OneTimeTokenHandler.getValueFromTokenAndExpire( + OneTimeTokenHandler.peekValueFromToken( 'password', token, - (err, data) => { + (err, data, remainingUses) => { if (err != null) { if (err.name === 'NotFoundError') { return callback(null, null) @@ -50,7 +56,7 @@ function getUserForPasswordResetToken(token, callback) { } } if (data == null || data.email == null) { - return callback(null, null) + return callback(null, null, remainingUses) } UserGetter.getUserByMainEmail( data.email, @@ -59,20 +65,20 @@ function getUserForPasswordResetToken(token, callback) { if (err != null) { callback(err) } else if (user == null) { - callback(null, null) + callback(null, null, 0) } else if ( data.user_id != null && data.user_id === user._id.toString() ) { - callback(null, user) + callback(null, user, remainingUses) } else if ( data.v1_user_id != null && user.overleaf != null && data.v1_user_id === user.overleaf.id ) { - callback(null, user) + callback(null, user, remainingUses) } else { - callback(null, null) + callback(null, null, 0) } } ) @@ -105,6 +111,8 @@ async function setNewUserPassword(token, password, auditLog) { password ) + await PasswordResetHandler.promises.expirePasswordResetToken(token) + return { found: true, reset, userId: user._id } } @@ -114,12 +122,17 @@ const PasswordResetHandler = { setNewUserPassword: callbackify(setNewUserPassword), getUserForPasswordResetToken, + + expirePasswordResetToken, } PasswordResetHandler.promises = { getUserForPasswordResetToken: promisify( PasswordResetHandler.getUserForPasswordResetToken ), + expirePasswordResetToken: promisify( + PasswordResetHandler.expirePasswordResetToken + ), setNewUserPassword, } diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js b/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js index 22a6dfe503..c7b251fa9a 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js @@ -15,6 +15,9 @@ module.exports = { webRouter.get( '/user/password/reset', + validate({ + query: { error: Joi.string() }, + }), PasswordResetController.renderRequestResetForm ) webRouter.post( @@ -32,6 +35,13 @@ module.exports = { webRouter.get( '/user/password/set', + validate({ + query: { + email: Joi.string().required(), + passwordResetToken: Joi.string(), + }, + }), + rateLimit, PasswordResetController.renderSetPasswordForm ) webRouter.post( diff --git a/services/web/app/src/Features/Security/OneTimeTokenHandler.js b/services/web/app/src/Features/Security/OneTimeTokenHandler.js index 2767fe9ccd..cf57eee0a5 100644 --- a/services/web/app/src/Features/Security/OneTimeTokenHandler.js +++ b/services/web/app/src/Features/Security/OneTimeTokenHandler.js @@ -6,6 +6,8 @@ const { promisifyAll } = require('../../util/promises') const ONE_HOUR_IN_S = 60 * 60 const OneTimeTokenHandler = { + MAX_PEEKS: 4, + getNewToken(use, data, options, callback) { // options is optional if (!options) { @@ -44,6 +46,7 @@ const OneTimeTokenHandler = { token, expiresAt: { $gt: now }, usedAt: { $exists: false }, + peekCount: { $not: { $gte: OneTimeTokenHandler.MAX_PEEKS } }, }, { $set: { @@ -62,6 +65,53 @@ const OneTimeTokenHandler = { } ) }, + + peekValueFromToken(use, token, callback) { + db.tokens.findOneAndUpdate( + { + use, + token, + expiresAt: { $gt: new Date() }, + usedAt: { $exists: false }, + peekCount: { $not: { $gte: OneTimeTokenHandler.MAX_PEEKS } }, + }, + { + $inc: { peekCount: 1 }, + }, + { + returnDocument: 'after', + }, + function (error, result) { + if (error) { + return callback(error) + } + const token = result.value + if (!token) { + return callback(new Errors.NotFoundError('no token found')) + } + const remainingPeeks = OneTimeTokenHandler.MAX_PEEKS - token.peekCount + callback(null, token.data, remainingPeeks) + } + ) + }, + + expireToken(use, token, callback) { + const now = new Date() + db.tokens.updateOne( + { + use, + token, + }, + { + $set: { + usedAt: now, + }, + }, + error => { + callback(error) + } + ) + }, } OneTimeTokenHandler.promises = promisifyAll(OneTimeTokenHandler) diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 24453fdbde..91e3cc1cd7 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -97,6 +97,12 @@ async function changePassword(req, res, next) { } catch (error) { if (error.name === 'InvalidPasswordError') { return HttpErrorHandler.badRequest(req, res, error.message) + } else if (error.name === 'PasswordMustBeDifferentError') { + return HttpErrorHandler.badRequest( + req, + res, + req.i18n.translate('password_change_password_must_be_different') + ) } else { throw error } diff --git a/services/web/app/views/user/passwordReset.pug b/services/web/app/views/user/passwordReset.pug index 315ebc33eb..1f049a6a62 100644 --- a/services/web/app/views/user/passwordReset.pug +++ b/services/web/app/views/user/passwordReset.pug @@ -32,7 +32,13 @@ block content ) div(data-ol-not-sent) +formMessages() - + if error + div.alert.alert-danger( + role="alert" + aria-live="assertive" + ) + | #{translate(error)} + input(type="hidden", name="_csrf", value=csrfToken) .form-group label(for='email') #{translate("please_enter_email")} diff --git a/services/web/app/views/user/setPassword.pug b/services/web/app/views/user/setPassword.pug index 90335c29fb..be8081e3c0 100644 --- a/services/web/app/views/user/setPassword.pug +++ b/services/web/app/views/user/setPassword.pug @@ -13,7 +13,18 @@ block content name="passwordResetForm", action="/user/password/set", method="POST", + data-ol-hide-on-error="token-expired" ) + div.alert.alert-success( + hidden + role="alert" + aria-live="assertive" + data-ol-sent + ) + | #{translate("password_has_been_reset")}. + br + a(href='/login') #{translate("login_here")} + div(data-ol-not-sent) +formMessages() @@ -26,41 +37,34 @@ block content +customFormMessage('invalid-password', 'danger') | #{translate('invalid_password')} - div.alert.alert-success( - hidden - role="alert" - aria-live="assertive" - data-ol-sent - ) - | #{translate("password_has_been_reset")}. - br - a(href='/login') #{translate("login_here")} + +customFormMessage('password-must-be-different', 'danger') + | #{translate('password_change_password_must_be_different')} - input(type="hidden", name="_csrf", value=csrfToken) - input(type="hidden", name="email", value=email) + input(type="hidden", name="_csrf", value=csrfToken) + input(type="hidden", name="email", value=email) - .form-group - input.form-control#passwordField( - type='password', - name='password', - placeholder='new password', - autocomplete="new-password", - autofocus, - required, - minlength=settings.passwordStrengthOptions.length.min - ) - input( - type="hidden", - name="passwordResetToken", - value=passwordResetToken - ) - .actions - button.btn.btn-primary( - type='submit', - data-ol-disabled-inflight - aria-label=translate('set_new_password') - ) - span(data-ol-inflight="idle") - | #{translate('set_new_password')} - span(hidden data-ol-inflight="pending") - | #{translate('set_new_password')}… + .form-group + input.form-control#passwordField( + type='password', + name='password', + placeholder='new password', + autocomplete="new-password", + autofocus, + required, + minlength=settings.passwordStrengthOptions.length.min + ) + input( + type="hidden", + name="passwordResetToken", + value=passwordResetToken + ) + .actions + button.btn.btn-primary( + type='submit', + data-ol-disabled-inflight + aria-label=translate('set_new_password') + ) + span(data-ol-inflight="idle") + | #{translate('set_new_password')} + span(hidden data-ol-inflight="pending") + | #{translate('set_new_password')}… diff --git a/services/web/frontend/js/features/form-helpers/hydrate-form.js b/services/web/frontend/js/features/form-helpers/hydrate-form.js index 555e156f77..5f3074f47c 100644 --- a/services/web/frontend/js/features/form-helpers/hydrate-form.js +++ b/services/web/frontend/js/features/form-helpers/hydrate-form.js @@ -120,6 +120,12 @@ async function sendFormRequest(formEl, captchaResponse) { return postJSON(url, { body }) } +function hideFormElements(formEl) { + for (const e of formEl.elements) { + e.hidden = true + } +} + function showMessages(formEl, messageBag) { const messagesEl = formEl.querySelector('[data-ol-form-messages]') if (!messagesEl) return @@ -138,6 +144,15 @@ function showMessages(formEl, messageBag) { .forEach(el => { el.hidden = false }) + // Hide the form elements on specific message types + const hideOnError = formEl.attributes['data-ol-hide-on-error'] + if ( + hideOnError && + hideOnError.value && + hideOnError.value.match(message.key) + ) { + hideFormElements(formEl) + } return } diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 5c70309657..1699178225 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -330,6 +330,7 @@ "not_found_error_from_the_supplied_url": "The link to open this content on Overleaf pointed to a file that could not be found. If this keeps happening for links on a particular site, please report this to them.", "too_many_requests": "Too many requests were received in a short space of time. Please wait for a few moments and try again.", "password_change_passwords_do_not_match": "Passwords do not match", + "password_change_password_must_be_different": "The password you entered is the same as your current password. Please try a different password.", "password_change_old_password_wrong": "Your old password is wrong", "github_for_link_shared_projects": "This project was accessed via link-sharing and won’t be synchronised with your GitHub unless you are invited via e-mail by the project owner.", "browsing_project_latest_for_pseudo_label": "Browsing your project’s current state", diff --git a/services/web/test/acceptance/src/PasswordResetTests.js b/services/web/test/acceptance/src/PasswordResetTests.js index 2c02ea48db..c2388abe91 100644 --- a/services/web/test/acceptance/src/PasswordResetTests.js +++ b/services/web/test/acceptance/src/PasswordResetTests.js @@ -227,8 +227,119 @@ describe('PasswordReset', function () { const auditLog = userHelper.getAuditLogWithoutNoise() expect(auditLog.length).to.equal(1) }) + + it('when the password is the same as current, should return 400 and log the change', async function () { + // send reset request + response = await userHelper.request.post('/user/password/set', { + form: { + passwordResetToken: token, + password: userHelper.getDefaultPassword(), + }, + simple: false, + }) + expect(response.statusCode).to.equal(400) + expect(JSON.parse(response.body).message.key).to.equal( + 'password-must-be-different' + ) + userHelper = await UserHelper.getUser({ email }) + + const auditLog = userHelper.getAuditLogWithoutNoise() + expect(auditLog.length).to.equal(1) + }) }) }) + + describe('multiple attempts to set the password, reaching attempt limit', async 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${emailQuery}` + ) + }) + + it('should allow multiple attempts with same-password error, then deny further attempts', async function () { + const sendSamePasswordRequest = async function () { + return userHelper.request.post('/user/password/set', { + form: { + passwordResetToken: token, + password: userHelper.getDefaultPassword(), + }, + simple: false, + }) + } + // Three attempts at setting the password, all rejected for being the same as + // the current password + const response1 = await sendSamePasswordRequest() + expect(response1.statusCode).to.equal(400) + expect(JSON.parse(response1.body).message.key).to.equal( + 'password-must-be-different' + ) + const response2 = await sendSamePasswordRequest() + expect(response2.statusCode).to.equal(400) + expect(JSON.parse(response2.body).message.key).to.equal( + 'password-must-be-different' + ) + const response3 = await sendSamePasswordRequest() + expect(response3.statusCode).to.equal(400) + expect(JSON.parse(response3.body).message.key).to.equal( + 'password-must-be-different' + ) + // Fourth attempt is rejected because the token has been used too many times + const response4 = await sendSamePasswordRequest() + expect(response4.statusCode).to.equal(404) + expect(JSON.parse(response4.body).message.key).to.equal('token-expired') + }) + + it('should allow multiple attempts with same-password error, then set the password', async function () { + const sendSamePasswordRequest = async function () { + return userHelper.request.post('/user/password/set', { + form: { + passwordResetToken: token, + password: userHelper.getDefaultPassword(), + }, + simple: false, + }) + } + // Two attempts at setting the password, all rejected for being the same as + // the current password + const response1 = await sendSamePasswordRequest() + expect(response1.statusCode).to.equal(400) + expect(JSON.parse(response1.body).message.key).to.equal( + 'password-must-be-different' + ) + const response2 = await sendSamePasswordRequest() + expect(response2.statusCode).to.equal(400) + expect(JSON.parse(response2.body).message.key).to.equal( + 'password-must-be-different' + ) + // Third attempt is succeeds + const response3 = await userHelper.request.post('/user/password/set', { + form: { + passwordResetToken: token, + password: 'some-new-password', + }, + simple: false, + }) + expect(response3.statusCode).to.equal(200) + // Check the user and audit log + userHelper = await UserHelper.getUser({ email }) + user = userHelper.user + expect(user.hashedPassword).to.exist + expect(user.password).to.not.exist + const auditLog = userHelper.getAuditLogWithoutNoise() + expect(auditLog).to.exist + expect(auditLog[0]).to.exist + expect(auditLog[0].initiatorId).to.equal(null) + expect(auditLog[0].operation).to.equal('reset-password') + expect(auditLog[0].ipAddress).to.equal('127.0.0.1') + expect(auditLog[0].timestamp).to.exist + }) + }) + 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( @@ -238,7 +349,7 @@ describe('PasswordReset', function () { expect(response.statusCode).to.equal(302) expect(response.headers.location).to.equal('/user/password/reset') }) - it('should return 404 for invalid tokens', async function () { + it('should show error for invalid tokens and return 404 if used', async function () { const invalidToken = 'not-real-token' response = await userHelper.request.get( `/user/password/set?&passwordResetToken=${invalidToken}&email=${email}`, @@ -246,7 +357,7 @@ describe('PasswordReset', function () { ) expect(response.statusCode).to.equal(302) expect(response.headers.location).to.equal( - `/user/password/set${emailQuery}` + `/user/password/reset?error=token_expired` ) // send reset request response = await userHelper.request.post('/user/password/set', { diff --git a/services/web/test/acceptance/src/SessionTests.js b/services/web/test/acceptance/src/SessionTests.js index 7a9bd22418..bcf538c5c9 100644 --- a/services/web/test/acceptance/src/SessionTests.js +++ b/services/web/test/acceptance/src/SessionTests.js @@ -256,7 +256,7 @@ describe('Sessions', function () { // password reset from second session, should erase two of the three sessions next => { - this.user2.changePassword(err => next(err)) + this.user2.changePassword(`password${Date.now()}`, err => next(err)) }, next => { diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index 8de11c882f..f92704e4aa 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -152,7 +152,9 @@ class User { this.setExtraAttributes(user) AuthenticationManager.setUserPasswordInV2(user, this.password, error => { if (error != null) { - return callback(error) + if (error.name !== 'PasswordMustBeDifferentError') { + return callback(error) + } } this.mongoUpdate({ $set: { emails: this.emails } }, error => { if (error != null) { @@ -675,7 +677,7 @@ class User { ) } - changePassword(callback) { + changePassword(newPassword, callback) { this.getCsrfToken(error => { if (error != null) { return callback(error) @@ -685,11 +687,17 @@ class User { url: '/user/password/update', json: { currentPassword: this.password, - newPassword1: this.password, - newPassword2: this.password, + newPassword1: newPassword, + newPassword2: newPassword, }, }, - callback + err => { + if (err) { + return callback(err) + } + this.password = newPassword + callback() + } ) }) } diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 8907be1662..374008cdce 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -180,7 +180,8 @@ describe('AuthenticationManager', function () { email: (this.email = 'USER@sharelatex.com'), } this.db.users.updateOne = sinon - this.User.findOne = sinon.stub().callsArgWith(2, null, this.user) + this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) + this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false) this.db.users.updateOne = sinon .stub() .callsArgWith(2, null, { modifiedCount: 1 }) @@ -603,19 +604,39 @@ describe('AuthenticationManager', function () { describe('setUserPassword', function () { beforeEach(function () { this.user_id = ObjectId() - this.user = { - _id: this.user_id, - email: 'user@example.com', - } this.password = 'banana' this.hashedPassword = 'asdkjfa;osiuvandf' this.salt = 'saltaasdfasdfasdf' + this.user = { + _id: this.user_id, + email: 'user@example.com', + hashedPassword: this.hashedPassword, + } + this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false) this.bcrypt.genSalt = sinon.stub().callsArgWith(2, null, this.salt) this.bcrypt.hash = sinon.stub().callsArgWith(2, null, this.hashedPassword) - this.User.findOne = sinon.stub().callsArgWith(2, null, this.user) + this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) this.db.users.updateOne = sinon.stub().callsArg(2) }) + describe('same as previous password', function () { + beforeEach(function () { + this.bcrypt.compare.callsArgWith(2, null, true) + }) + + it('should return an error', function (done) { + this.AuthenticationManager.setUserPassword( + this.user, + this.password, + err => { + expect(err).to.exist + expect(err.name).to.equal('PasswordMustBeDifferentError') + done() + } + ) + }) + }) + describe('too long', function () { beforeEach(function () { this.settings.passwordStrengthOptions = { diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js index d2410556d8..c725329c41 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js @@ -39,6 +39,10 @@ describe('PasswordResetController', function () { .stub() .resolves({ found: true, reset: true, userID: this.user_id }), }, + getUserForPasswordResetToken: sinon + .stub() + .withArgs(this.token) + .yields(null, { _id: this.user_id }, 1), } this.UserSessionsManager = { promises: { @@ -372,6 +376,28 @@ describe('PasswordResetController', function () { }) }) + describe('with expired token in query', function () { + beforeEach(function () { + this.req.query.passwordResetToken = this.token + this.PasswordResetHandler.getUserForPasswordResetToken = sinon + .stub() + .withArgs(this.token) + .yields(null, { _id: this.user_id }, 0) + }) + + it('should redirect to the reset request page with an error message', function (done) { + this.res.redirect = path => { + path.should.equal('/user/password/reset?error=token_expired') + this.req.session.should.not.have.property('resetToken') + done() + } + this.res.render = (templatePath, options) => { + done('should not render') + } + this.PasswordResetController.renderSetPasswordForm(this.req, this.res) + }) + }) + describe('with token and email in query-string', function () { beforeEach(function () { this.req.query.passwordResetToken = this.token diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index 8473b8e812..9f9a941d1a 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -26,7 +26,8 @@ describe('PasswordResetHandler', function () { this.settings = { siteUrl: 'https://www.overleaf.com' } this.OneTimeTokenHandler = { getNewToken: sinon.stub(), - getValueFromTokenAndExpire: sinon.stub(), + peekValueFromToken: sinon.stub(), + expireToken: sinon.stub(), } this.UserGetter = { getUserByMainEmail: sinon.stub(), @@ -188,7 +189,7 @@ describe('PasswordResetHandler', function () { }) describe('when no data is found', function () { beforeEach(function () { - this.OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, null) + this.OneTimeTokenHandler.peekValueFromToken.yields(null, null) }) it('should return found == false and reset == false', function () { @@ -210,7 +211,7 @@ describe('PasswordResetHandler', function () { describe('when the token has a user_id and email', function () { beforeEach(function () { - this.OneTimeTokenHandler.getValueFromTokenAndExpire + this.OneTimeTokenHandler.peekValueFromToken .withArgs('password', this.token) .yields(null, { user_id: this.user._id, @@ -219,6 +220,9 @@ describe('PasswordResetHandler', function () { this.AuthenticationManager.promises.setUserPassword .withArgs(this.user, this.password) .resolves(true) + this.OneTimeTokenHandler.expireToken = sinon + .stub() + .callsArgWith(2, null) }) describe('when no user is found with this email', function () { @@ -238,6 +242,7 @@ describe('PasswordResetHandler', function () { expect(err).to.not.exist expect(found).to.be.false expect(reset).to.be.false + expect(this.OneTimeTokenHandler.expireToken.callCount).to.equal(0) done() } ) @@ -249,6 +254,7 @@ describe('PasswordResetHandler', function () { this.UserGetter.getUserByMainEmail .withArgs(this.email) .yields(null, { _id: 'not-the-same', email: this.email }) + this.OneTimeTokenHandler.expireToken.callsArgWith(2, null) }) it('should return found == false and reset == false', function (done) { @@ -261,6 +267,7 @@ describe('PasswordResetHandler', function () { expect(err).to.not.exist expect(found).to.be.false expect(reset).to.be.false + expect(this.OneTimeTokenHandler.expireToken.callCount).to.equal(0) done() } ) @@ -271,6 +278,9 @@ describe('PasswordResetHandler', function () { describe('success', function () { beforeEach(function () { this.UserGetter.getUserByMainEmail.yields(null, this.user) + this.OneTimeTokenHandler.expireToken = sinon + .stub() + .callsArgWith(2, null) }) it('should update the user audit log', function (done) { @@ -308,6 +318,20 @@ describe('PasswordResetHandler', function () { ) }) + it('should expire the token', function (done) { + this.PasswordResetHandler.setNewUserPassword( + this.token, + this.password, + this.auditLog, + (_err, _result) => { + expect(this.OneTimeTokenHandler.expireToken.called).to.equal( + true + ) + done() + } + ) + }) + describe('when logged in', function () { beforeEach(function () { this.auditLog.initiatorId = this.user_id @@ -335,6 +359,30 @@ describe('PasswordResetHandler', function () { }) describe('errors', function () { + describe('via setUserPassword', function () { + beforeEach(function () { + this.PasswordResetHandler.promises.getUserForPasswordResetToken = + sinon.stub().withArgs(this.token).resolves(this.user) + this.AuthenticationManager.promises.setUserPassword + .withArgs(this.user, this.password) + .rejects() + }) + it('should return the error', 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) + done() + } + ) + }) + }) + describe('via UserAuditLogHandler', function () { beforeEach(function () { this.PasswordResetHandler.promises.getUserForPasswordResetToken = @@ -367,7 +415,7 @@ describe('PasswordResetHandler', function () { describe('when the token has a v1_user_id and email', function () { beforeEach(function () { this.user.overleaf = { id: 184 } - this.OneTimeTokenHandler.getValueFromTokenAndExpire + this.OneTimeTokenHandler.peekValueFromToken .withArgs('password', this.token) .yields(null, { v1_user_id: this.user.overleaf.id, @@ -376,6 +424,9 @@ describe('PasswordResetHandler', function () { this.AuthenticationManager.promises.setUserPassword .withArgs(this.user, this.password) .resolves(true) + this.OneTimeTokenHandler.expireToken = sinon + .stub() + .callsArgWith(2, null) }) describe('when no user is reset with this email', function () { @@ -394,6 +445,9 @@ describe('PasswordResetHandler', function () { const { reset, userId } = result expect(err).to.not.exist expect(reset).to.be.false + expect(this.OneTimeTokenHandler.expireToken.called).to.equal( + false + ) done() } ) @@ -418,6 +472,9 @@ describe('PasswordResetHandler', function () { const { reset, userId } = result expect(err).to.not.exist expect(reset).to.be.false + expect(this.OneTimeTokenHandler.expireToken.called).to.equal( + false + ) done() } ) @@ -441,6 +498,7 @@ describe('PasswordResetHandler', function () { expect(err).to.not.exist expect(reset).to.be.true expect(userId).to.equal(this.user._id) + expect(this.OneTimeTokenHandler.expireToken.called).to.equal(true) done() } ) diff --git a/services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js b/services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js index 2f49c8a7dd..2f6df14f45 100644 --- a/services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js +++ b/services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js @@ -107,6 +107,90 @@ describe('OneTimeTokenHandler', function () { }) }) + describe('peekValueFromToken', function () { + describe('successfully', function () { + const data = 'some-mock-data' + beforeEach(function () { + this.db.tokens.findOneAndUpdate = sinon + .stub() + .yields(null, { value: { data } }) + return this.OneTimeTokenHandler.peekValueFromToken( + 'password', + 'mock-token', + this.callback + ) + }) + + it('should increment the peekCount', function () { + return this.db.tokens.findOneAndUpdate + .calledWith( + { + use: 'password', + token: 'mock-token', + expiresAt: { $gt: new Date() }, + usedAt: { $exists: false }, + peekCount: { $not: { $gte: this.OneTimeTokenHandler.MAX_PEEKS } }, + }, + { + $inc: { peekCount: 1 }, + } + ) + .should.equal(true) + }) + + it('should return the data', function () { + return this.callback.calledWith(null, data).should.equal(true) + }) + }) + + describe('when a valid token is not found', function () { + beforeEach(function () { + this.db.tokens.findOneAndUpdate = sinon + .stub() + .yields(null, { value: null }) + return this.OneTimeTokenHandler.peekValueFromToken( + 'password', + 'mock-token', + this.callback + ) + }) + + it('should return a NotFoundError', function () { + return this.callback + .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) + .should.equal(true) + }) + }) + }) + + describe('expireToken', function () { + beforeEach(function () { + this.db.tokens.updateOne = sinon.stub().yields(null) + this.OneTimeTokenHandler.expireToken( + 'password', + 'mock-token', + this.callback + ) + }) + + it('should expire the token', function () { + this.db.tokens.updateOne + .calledWith( + { + use: 'password', + token: 'mock-token', + }, + { + $set: { + usedAt: new Date(), + }, + } + ) + .should.equal(true) + this.callback.calledWith(null).should.equal(true) + }) + }) + describe('getValueFromTokenAndExpire', function () { describe('successfully', function () { beforeEach(function () { @@ -128,6 +212,7 @@ describe('OneTimeTokenHandler', function () { token: 'mock-token', expiresAt: { $gt: new Date() }, usedAt: { $exists: false }, + peekCount: { $not: { $gte: this.OneTimeTokenHandler.MAX_PEEKS } }, }, { $set: { usedAt: new Date() },