Merge pull request #17399 from overleaf/jpa-hibp-login

[web] check HIBP on login

GitOrigin-RevId: e052926e4d970f9a15821f1ea9c8af46bdab90cb

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

@@ -23,7 +23,10 @@ const AnalyticsRegistrationSourceHelper = require('../Analytics/AnalyticsRegistr
 const {
   acceptsJson,
 } = require('../../infrastructure/RequestContentTypeDetection')
-const { ParallelLoginError } = require('./AuthenticationErrors')
+const {
+  ParallelLoginError,
+  PasswordReusedError,
+} = require('./AuthenticationErrors')
 const { hasAdminAccess } = require('../Helpers/AdminAuthorizationHelper')
 const Modules = require('../../infrastructure/Modules')
 const { expressify } = require('@overleaf/promise-utils')
@@ -218,6 +221,28 @@ const AuthenticationController = {
               if (error != null) {
                 if (error instanceof ParallelLoginError) {
                   return done(null, false, { status: 429 })
+                } else if (error instanceof PasswordReusedError) {
+                  const text = `${req.i18n
+                    .translate(
+                      'password_was_detected_on_a_public_list_of_known_compromised_passwords'
+                    )
+                    .replace('<0>', '')
+                    .replace(
+                      '</0>',
+                      ' (https://haveibeenpwned.com)'
+                    )}. ${req.i18n
+                    .translate('please_reset_your_password_to_login')
+                    .replace('<0>', '')
+                    .replace(
+                      '</0>',
+                      ` (${Settings.siteUrl}/user/password/reset)`
+                    )}.`
+                  return done(null, false, {
+                    status: 400,
+                    type: 'error',
+                    key: 'password-compromised',
+                    text,
+                  })
                 }
                 return done(error)
               }

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

@@ -60,6 +60,19 @@ 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)
@@ -179,7 +192,6 @@ const AuthenticationManager = {
                   return callback(err)
                 }
                 callback(null, user)
-                HaveIBeenPwned.checkPasswordForReuseInBackground(password)
               }
             )
           }
@@ -333,7 +345,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._checkUserPassword(
+    AuthenticationManager._checkUserPasswordWithOutHIBPCheck(
       { _id: user._id },
       password,
       (err, _user, match) => {

services/web/app/views/user/login.pug

@@ -15,6 +15,10 @@ block content
 								| !{translate('email_or_password_wrong_try_again_or_reset', {}, [{ name: 'a', attrs: { href: '/user/password/reset', 'aria-describedby': 'resetPasswordDescription' } }])}
 								span.sr-only(id='resetPasswordDescription')
 									| #{translate('reset_password_link')}
+							+customValidationMessage('password-compromised')
+								| !{translate('password_was_detected_on_a_public_list_of_known_compromised_passwords', {}, [{name: 'a', attrs: {href: 'https://haveibeenpwned.com', rel: 'noopener noreferrer', target: '_blank'}}])}.
+								|
+								| !{translate('please_reset_your_password_to_login', {}, [{name: 'a', attrs: {href: '/user/password/reset', target: '_blank'}}])}.
 							.form-group
 								input.form-control(
 									type='email',

services/web/locales/en.json

@@ -1338,6 +1338,7 @@
   "please_reconfirm_your_affiliation_before_making_this_primary": "Please confirm your affiliation before making this the primary.",
   "please_refresh": "Please refresh the page to continue.",
   "please_request_a_new_password_reset_email_and_follow_the_link": "Please request a new password reset email and follow the link",
+  "please_reset_your_password_to_login": "Please reset your password <0>here</0> to login",
   "please_select_a_file": "Please Select a File",
   "please_select_a_project": "Please Select a Project",
   "please_select_an_output_file": "Please Select an Output File",

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

@@ -4,17 +4,12 @@ const User = require('./helpers/User').promises
 const MockHaveIBeenPwnedApiClass = require('./mocks/MockHaveIBeenPwnedApi')
 const { db } = require('../../../app/src/infrastructure/mongodb')
 const { getMetric } = require('./helpers/metrics').promises
-const sleep = require('util').promisify(setTimeout)
 
 let MockHaveIBeenPwnedApi
 before(function () {
   MockHaveIBeenPwnedApi = MockHaveIBeenPwnedApiClass.instance()
 })
 
-async function letPasswordCheckRunInBackground() {
-  await sleep(200)
-}
-
 async function getMetricReUsed() {
   return getMetric(
     line => line.includes('password_re_use') && line.includes('re-used')
@@ -83,14 +78,18 @@ describe('HaveIBeenPwnedApi', function () {
     })
     beforeEach('create the user', async function () {
       await user.ensureUserExists()
-      await letPasswordCheckRunInBackground()
     })
     beforeEach('fetch previous count', async function () {
       previous = await getMetricReUsed()
     })
     beforeEach('login', async function () {
+      try {
         await user.loginNoUpdate()
-      await letPasswordCheckRunInBackground()
+      } catch (e) {
+        expect(e.message).to.include('password-compromised')
+        return
+      }
+      expect.fail('should have failed login with weak password')
     })
     it('should track the weak password', async function () {
       const after = await getMetricReUsed()
@@ -105,14 +104,12 @@ describe('HaveIBeenPwnedApi', function () {
     })
     beforeEach('create the user', async function () {
       await user.ensureUserExists()
-      await letPasswordCheckRunInBackground()
     })
     beforeEach('fetch previous count', async function () {
       previous = await getMetricUnique()
     })
     beforeEach('login', async function () {
       await user.loginNoUpdate()
-      await letPasswordCheckRunInBackground()
     })
     it('should track the strong password', async function () {
       const after = await getMetricUnique()
@@ -127,14 +124,12 @@ describe('HaveIBeenPwnedApi', function () {
     })
     beforeEach('create the user', async function () {
       await user.ensureUserExists()
-      await letPasswordCheckRunInBackground()
     })
     beforeEach('fetch previous count', async function () {
       previous = await getMetricFailure()
     })
     beforeEach('login', async function () {
       await user.loginNoUpdate()
-      await letPasswordCheckRunInBackground()
     })
     it('should track the failure to collect a score', async function () {
       const after = await getMetricFailure()
@@ -152,7 +147,6 @@ describe('HaveIBeenPwnedApi', function () {
     })
     beforeEach('create the user', async function () {
       await user.ensureUserExists()
-      await letPasswordCheckRunInBackground()
     })
     beforeEach('fetch previous counts', async function () {
       previous = {
@@ -166,16 +160,19 @@ 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=401/)
+        expect(err).to.match(/login failed: status=400/)
         expect(err.info.body).to.deep.equal({
-          message: { type: 'error', key: 'invalid-password-retry-or-reset' },
+          message: {
+            type: 'error',
+            key: 'password-compromised',
+            text: `This password was detected on a public list of known compromised passwords (https://haveibeenpwned.com). Please reset your password here (${Settings.siteUrl}/user/password/reset) to login.`,
+          },
         })
       }
-      await letPasswordCheckRunInBackground()
     })
-    it('should not increment any counter', async function () {
+    it('should increment the counter', async function () {
       expect(previous).to.deep.equal({
-        reUsed: await getMetricReUsed(),
+        reUsed: (await getMetricReUsed()) - 1,
         unique: await getMetricUnique(),
         failure: await getMetricFailure(),
       })
@@ -192,7 +189,6 @@ describe('HaveIBeenPwnedApi', function () {
     })
     beforeEach('create the user', async function () {
       await user.ensureUserExists()
-      await letPasswordCheckRunInBackground()
     })
     beforeEach('fetch previous count', async function () {
       previous = await getMetricReUsed()
@@ -207,7 +203,6 @@ describe('HaveIBeenPwnedApi', function () {
           },
         })
       )
-      await letPasswordCheckRunInBackground()
     })
     it('should track the weak password', async function () {
       const after = await getMetricReUsed()
@@ -221,7 +216,6 @@ describe('HaveIBeenPwnedApi', function () {
     })
     beforeEach('create the user', async function () {
       await user.ensureUserExists()
-      await letPasswordCheckRunInBackground()
     })
     beforeEach('fetch previous count', async function () {
       previous = await getMetricUnique()
@@ -229,7 +223,6 @@ describe('HaveIBeenPwnedApi', function () {
     beforeEach('set password', async function () {
       const response = await resetPassword('a-strong-new-password')
       expect(response.statusCode).to.equal(200)
-      await letPasswordCheckRunInBackground()
     })
     it('should track the strong password', async function () {
       const after = await getMetricUnique()