From 5f1abee345fbc9e8697a83dfb99714a52b73a86f Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Tue, 19 Jul 2022 15:14:26 +0100 Subject: [PATCH] Merge pull request #8939 from overleaf/revert-8882-jk-web-reject-same-password Revert "[web] Password set/reset: reject current password" GitOrigin-RevId: f14f970fe93064658a8659537c5cb417e34e2751 --- .../Authentication/AuthenticationErrors.js | 2 - .../Authentication/AuthenticationManager.js | 63 +++++++------------ .../PasswordReset/PasswordResetController.js | 6 -- .../PasswordReset/PasswordResetHandler.js | 10 +-- .../app/src/Features/User/UserController.js | 6 -- services/web/app/views/user/setPassword.pug | 5 -- services/web/locales/en.json | 1 - .../test/acceptance/src/PasswordResetTests.js | 19 ------ .../web/test/acceptance/src/SessionTests.js | 2 +- .../web/test/acceptance/src/helpers/User.js | 18 ++---- .../AuthenticationManagerTests.js | 26 -------- .../PasswordResetHandlerTests.js | 26 +------- 12 files changed, 36 insertions(+), 148 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationErrors.js b/services/web/app/src/Features/Authentication/AuthenticationErrors.js index c7b4caf470..fa394903b1 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationErrors.js +++ b/services/web/app/src/Features/Authentication/AuthenticationErrors.js @@ -3,11 +3,9 @@ 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 4b30407c95..76d41f6131 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -7,7 +7,6 @@ const { InvalidEmailError, InvalidPasswordError, ParallelLoginError, - PasswordMustBeDifferentError, } = require('./AuthenticationErrors') const util = require('util') const HaveIBeenPwned = require('./HaveIBeenPwned') @@ -183,45 +182,31 @@ const AuthenticationManager = { if (validationError) { return callback(validationError) } - // 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.authenticate( - { _id: user._id }, - password, - (err, authedUser) => { - if (err) { - return callback(err) - } - if (authedUser) { - 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) - } - ) - }) + 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 e56a3444d3..ebf2d538a9 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetController.js @@ -78,12 +78,6 @@ 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'), diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index 94a9777bb4..d08d008368 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js @@ -93,11 +93,6 @@ async function setNewUserPassword(token, password, auditLog) { } } - const reset = await AuthenticationManager.promises.setUserPassword( - user, - password - ) - await UserAuditLogHandler.promises.addEntry( user._id, 'reset-password', @@ -105,6 +100,11 @@ async function setNewUserPassword(token, password, auditLog) { auditLog.ip ) + const reset = await AuthenticationManager.promises.setUserPassword( + user, + password + ) + return { found: true, reset, userId: user._id } } diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 319110470d..8495376413 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -96,12 +96,6 @@ 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/setPassword.pug b/services/web/app/views/user/setPassword.pug index a35aa88cef..90335c29fb 100644 --- a/services/web/app/views/user/setPassword.pug +++ b/services/web/app/views/user/setPassword.pug @@ -26,11 +26,6 @@ block content +customFormMessage('invalid-password', 'danger') | #{translate('invalid_password')} - +customFormMessage('password-must-be-different', 'danger') - | #{translate('password_change_password_must_be_different')} - br - a(href='/user/password/reset') #{translate("request_new_password_reset_email")} - div.alert.alert-success( hidden role="alert" diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 4ad5ce0752..def15faf10 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -328,7 +328,6 @@ "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 ae081d1de0..e7e41eb4a3 100644 --- a/services/web/test/acceptance/src/PasswordResetTests.js +++ b/services/web/test/acceptance/src/PasswordResetTests.js @@ -234,25 +234,6 @@ 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 not 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).to.deep.equal([]) - }) }) }) describe('without a valid token', function () { diff --git a/services/web/test/acceptance/src/SessionTests.js b/services/web/test/acceptance/src/SessionTests.js index bcf538c5c9..7a9bd22418 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(`password${Date.now()}`, err => next(err)) + this.user2.changePassword(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 f92704e4aa..8de11c882f 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -152,9 +152,7 @@ class User { this.setExtraAttributes(user) AuthenticationManager.setUserPasswordInV2(user, this.password, error => { if (error != null) { - if (error.name !== 'PasswordMustBeDifferentError') { - return callback(error) - } + return callback(error) } this.mongoUpdate({ $set: { emails: this.emails } }, error => { if (error != null) { @@ -677,7 +675,7 @@ class User { ) } - changePassword(newPassword, callback) { + changePassword(callback) { this.getCsrfToken(error => { if (error != null) { return callback(error) @@ -687,17 +685,11 @@ class User { url: '/user/password/update', json: { currentPassword: this.password, - newPassword1: newPassword, - newPassword2: newPassword, + newPassword1: this.password, + newPassword2: this.password, }, }, - err => { - if (err) { - return callback(err) - } - this.password = newPassword - callback() - } + callback ) }) } diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index fee27d1427..8907be1662 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -181,9 +181,6 @@ describe('AuthenticationManager', function () { } this.db.users.updateOne = sinon this.User.findOne = sinon.stub().callsArgWith(2, null, this.user) - this.AuthenticationManager.authenticate = sinon - .stub() - .callsArgWith(2, null, null) this.db.users.updateOne = sinon .stub() .callsArgWith(2, null, { modifiedCount: 1 }) @@ -617,29 +614,6 @@ describe('AuthenticationManager', function () { this.bcrypt.hash = sinon.stub().callsArgWith(2, null, this.hashedPassword) this.User.findOne = sinon.stub().callsArgWith(2, null, this.user) this.db.users.updateOne = sinon.stub().callsArg(2) - this.AuthenticationManager.authenticate = sinon - .stub() - .callsArgWith(2, null, null) - }) - - describe('same as previous password', function () { - beforeEach(function () { - this.AuthenticationManager.authenticate = sinon - .stub() - .callsArgWith(2, null, this.user) - }) - - 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 () { diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index 3ba563b92c..8473b8e812 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -335,30 +335,6 @@ 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(0) - done() - } - ) - }) - }) - describe('via UserAuditLogHandler', function () { beforeEach(function () { this.PasswordResetHandler.promises.getUserForPasswordResetToken = @@ -378,7 +354,7 @@ describe('PasswordResetHandler', function () { this.UserAuditLogHandler.promises.addEntry.callCount ).to.equal(1) expect(this.AuthenticationManager.promises.setUserPassword).to - .have.been.called + .not.have.been.called done() } )