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..4b30407c95 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') @@ -182,31 +183,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.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) + } + ) + }) + } + ) }, _passwordCharactersAreValid(password) { diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index ebf2d538a9..e56a3444d3 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'), diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index d08d008368..94a9777bb4 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js @@ -93,6 +93,11 @@ async function setNewUserPassword(token, password, auditLog) { } } + const reset = await AuthenticationManager.promises.setUserPassword( + user, + password + ) + await UserAuditLogHandler.promises.addEntry( user._id, 'reset-password', @@ -100,11 +105,6 @@ 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 8495376413..319110470d 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -96,6 +96,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/setPassword.pug b/services/web/app/views/user/setPassword.pug index 90335c29fb..a35aa88cef 100644 --- a/services/web/app/views/user/setPassword.pug +++ b/services/web/app/views/user/setPassword.pug @@ -26,6 +26,11 @@ 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 def15faf10..4ad5ce0752 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -328,6 +328,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 e7e41eb4a3..ae081d1de0 100644 --- a/services/web/test/acceptance/src/PasswordResetTests.js +++ b/services/web/test/acceptance/src/PasswordResetTests.js @@ -234,6 +234,25 @@ 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 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..fee27d1427 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -181,6 +181,9 @@ 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 }) @@ -614,6 +617,29 @@ 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 8473b8e812..3ba563b92c 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -335,6 +335,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(0) + done() + } + ) + }) + }) + describe('via UserAuditLogHandler', function () { beforeEach(function () { this.PasswordResetHandler.promises.getUserForPasswordResetToken = @@ -354,7 +378,7 @@ describe('PasswordResetHandler', function () { this.UserAuditLogHandler.promises.addEntry.callCount ).to.equal(1) expect(this.AuthenticationManager.promises.setUserPassword).to - .not.have.been.called + .have.been.called done() } )