Merge pull request #17409 from overleaf/jpa-check-before-hibp

[web] check user password before HIBP check

GitOrigin-RevId: 7c1bdc220fb9369733a1ff3bf26bed8cacc8e8d4

services/web/app/src/Features/Authentication/AuthenticationManager.js

@@ -60,19 +60,6 @@ function _metricsForSuccessfulPasswordMatch(password) {
 
 const AuthenticationManager = {
   _checkUserPassword(query, password, callback) {
-    HaveIBeenPwned.checkPasswordForReuse(password, (err, isPasswordReused) => {
-      if (err) {
-        logger.err({ err }, 'cannot check password for re-use')
-      }
-      if (isPasswordReused) return callback(new PasswordReusedError())
-      AuthenticationManager._checkUserPasswordWithOutHIBPCheck(
-        query,
-        password,
-        callback
-      )
-    })
-  },
-  _checkUserPasswordWithOutHIBPCheck(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)
@@ -132,75 +119,84 @@ const AuthenticationManager = {
   },
 
   authenticate(query, password, auditLog, { skipHIBPCheck = false }, callback) {
-    const checkUserPassword = callback => {
+    AuthenticationManager._checkUserPassword(
-      if (skipHIBPCheck) {
+      query,
-        AuthenticationManager._checkUserPasswordWithOutHIBPCheck(
+      password,
-          query,
+      (error, user, match) => {
-          password,
+        if (error) {
-          callback
+          return callback(error)
-        )
-      } else {
-        AuthenticationManager._checkUserPassword(query, password, callback)
-      }
-    }
-    checkUserPassword((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() }
-      }
-      User.updateOne(
-        { _id: user._id, loginEpoch: user.loginEpoch },
-        update,
-        {},
-        (err, result) => {
-          if (err) {
-            return callback(err)
-          }
-          if (result.modifiedCount !== 1) {
-            return callback(new ParallelLoginError())
-          }
-          if (!match) {
-            if (!auditLog) {
-              return callback(null, null)
-            } else {
-              return UserAuditLogHandler.addEntry(
-                user._id,
-                'failed-password-match',
-                user._id,
-                auditLog.ipAddress,
-                auditLog.info,
-                err => {
-                  if (err) {
-                    logger.error(
-                      { userId: user._id, err, info: auditLog.info },
-                      'Error while adding AuditLog entry for failed-password-match'
-                    )
-                  }
-                  callback(null, null)
-                }
-              )
-            }
-          }
-          AuthenticationManager.checkRounds(
-            user,
-            user.hashedPassword,
-            password,
-            function (err) {
-              if (err) {
-                return callback(err)
-              }
-              callback(null, user)
-            }
-          )
         }
-      )
+        if (!user) {
-    })
+          return callback(null, null)
+        }
+        const update = { $inc: { loginEpoch: 1 } }
+        if (!match) {
+          update.$set = { lastFailedLogin: new Date() }
+        }
+        User.updateOne(
+          { _id: user._id, loginEpoch: user.loginEpoch },
+          update,
+          {},
+          (err, result) => {
+            if (err) {
+              return callback(err)
+            }
+            if (result.modifiedCount !== 1) {
+              return callback(new ParallelLoginError())
+            }
+            if (!match) {
+              if (!auditLog) {
+                return callback(null, null)
+              } else {
+                return UserAuditLogHandler.addEntry(
+                  user._id,
+                  'failed-password-match',
+                  user._id,
+                  auditLog.ipAddress,
+                  auditLog.info,
+                  err => {
+                    if (err) {
+                      logger.error(
+                        { userId: user._id, err, info: auditLog.info },
+                        'Error while adding AuditLog entry for failed-password-match'
+                      )
+                    }
+                    callback(null, null)
+                  }
+                )
+              }
+            }
+            AuthenticationManager.checkRounds(
+              user,
+              user.hashedPassword,
+              password,
+              function (err) {
+                if (err) {
+                  return callback(err)
+                }
+                if (skipHIBPCheck) {
+                  callback(null, user)
+                  HaveIBeenPwned.checkPasswordForReuseInBackground(password)
+                  return
+                }
+                HaveIBeenPwned.checkPasswordForReuse(
+                  password,
+                  (err, isPasswordReused) => {
+                    if (err) {
+                      logger.err({ err }, 'cannot check password for re-use')
+                    }
+                    if (isPasswordReused) {
+                      return callback(new PasswordReusedError())
+                    }
+                    callback(null, user)
+                  }
+                )
+              }
+            )
+          }
+        )
+      }
+    )
   },
 
   validateEmail(email) {
@@ -348,7 +344,7 @@ const AuthenticationManager = {
     }
     // 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._checkUserPasswordWithOutHIBPCheck(
+    AuthenticationManager._checkUserPassword(
       { _id: user._id },
       password,
       (err, _user, match) => {

services/web/locales/en.json

@@ -1280,7 +1280,7 @@
   "password_change_password_must_be_different": "The password you entered is the same as your current password. Please try a different password.",
   "password_change_passwords_do_not_match": "Passwords do not match",
   "password_change_successful": "Password changed",
-  "password_compromised_try_again_or_use_known_device_or_reset": "The password you’ve entered is on a <0>public list of compromised passwords</0>. Please check it’s correct and try again. Alternatively, try logging in from a device you’ve previously used or <1>reset your password</1>",
+  "password_compromised_try_again_or_use_known_device_or_reset": "The password you’ve entered is on a <0>public list of compromised passwords</0>. Please try logging in from a device you’ve previously used or <1>reset your password</1>",
   "password_managed_externally": "Password settings are managed externally",
   "password_reset": "Password Reset",
   "password_reset_email_sent": "You have been sent an email to complete your password reset.",

services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js

@@ -85,11 +85,17 @@ describe('HaveIBeenPwnedApi', function () {
     beforeEach('login', async function () {
       try {
         await user.loginNoUpdate()
-      } catch (e) {
+        expect.fail('should have failed login with weak password')
-        expect(e.message).to.include('password-compromised')
+      } catch (err) {
-        return
+        expect(err).to.match(/login failed: status=400/)
+        expect(err.info.body).to.deep.equal({
+          message: {
+            type: 'error',
+            key: 'password-compromised',
+            text: `The password you’ve entered is on a public list of compromised passwords (https://haveibeenpwned.com). Please try logging in from a device you’ve previously used or reset your password (${Settings.siteUrl}/user/password/reset).`,
+          },
+        })
       }
-      expect.fail('should have failed login with weak password')
     })
     it('should track the weak password', async function () {
       const after = await getMetricReUsed()
@@ -160,19 +166,15 @@ describe('HaveIBeenPwnedApi', function () {
         await user.loginWithEmailPassword(user.email, 'aLeakedPassword42')
         expect.fail('expected the login request to fail')
       } catch (err) {
-        expect(err).to.match(/login failed: status=400/)
+        expect(err).to.match(/login failed: status=401/)
         expect(err.info.body).to.deep.equal({
-          message: {
+          message: { type: 'error', key: 'invalid-password-retry-or-reset' },
-            type: 'error',
-            key: 'password-compromised',
-            text: `The password you’ve entered is on a public list of compromised passwords (https://haveibeenpwned.com). Please check it’s correct and try again. Alternatively, try logging in from a device you’ve previously used or reset your password (${Settings.siteUrl}/user/password/reset).`,
-          },
         })
       }
     })
-    it('should increment the counter', async function () {
+    it('should not increment the counter', async function () {
       expect(previous).to.deep.equal({
-        reUsed: (await getMetricReUsed()) - 1,
+        reUsed: await getMetricReUsed(),
         unique: await getMetricUnique(),
         failure: await getMetricFailure(),
       })