Merge pull request #8882 from overleaf/jk-web-reject-same-password

[web] Password set/reset: reject current password GitOrigin-RevId: 2c40dda4926d9c68564ae5126b3393b9286bb661
2024-11-21 20:47:08 -05:00 · 2022-07-19 09:32:23 +01:00 · 2022-07-19 09:32:23 +01:00 · d04ea76081
commit d04ea76081
parent 7f6e2c971e
12 changed files with 148 additions and 36 deletions
--- 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,
 }
--- 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) {
--- 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'),
--- 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 }
 }

--- 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
    }
--- 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"
--- 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",
--- 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 () {
--- 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 => {
--- 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()
+        }
      )
    })
  }
--- 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 () {
--- 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()
                }
              )