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

Revert "[web] Password set/reset: reject current password"

GitOrigin-RevId: f14f970fe93064658a8659537c5cb417e34e2751

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,
 }

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) {

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'),

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 }
 }
 

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
     }

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"

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",

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 () {

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 => {

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
       )
     })
   }

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 () {

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()
                 }
               )