Merge pull request #17401 from overleaf/jpa-skip-hibp-known-device

[web] skip HIBP check from known devices

GitOrigin-RevId: 897df02492aafeac010753c7c306e02bde5b1fd8

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

@@ -209,14 +209,16 @@ const AuthenticationController = {
               status: 429,
             })
           }
+          const { fromKnownDevice } = AuthenticationController.getAuditInfo(req)
           const auditLog = {
             ipAddress: req.ip,
-            info: { method: 'Password login' },
+            info: { method: 'Password login', fromKnownDevice },
           }
           AuthenticationManager.authenticate(
             { email },
             password,
             auditLog,
+            { skipHIBPCheck: fromKnownDevice },
             function (error, user) {
               if (error != null) {
                 if (error instanceof ParallelLoginError) {
@@ -224,17 +226,13 @@ const AuthenticationController = {
                 } else if (error instanceof PasswordReusedError) {
                   const text = `${req.i18n
                     .translate(
-                      'password_was_detected_on_a_public_list_of_known_compromised_passwords'
+                      'password_compromised_try_again_or_use_known_device_or_reset'
                     )
                     .replace('<0>', '')
+                    .replace('</0>', ' (https://haveibeenpwned.com)')
+                    .replace('<1>', '')
                     .replace(
-                      '</0>',
-                      ' (https://haveibeenpwned.com)'
-                    )}. ${req.i18n
-                    .translate('please_reset_your_password_to_login')
-                    .replace('<0>', '')
-                    .replace(
-                      '</0>',
+                      '</1>',
                       ` (${Settings.siteUrl}/user/password/reset)`
                     )}.`
                   return done(null, false, {

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

@@ -131,73 +131,76 @@ const AuthenticationManager = {
     })
   },
 
-  authenticate(query, password, auditLog, callback) {
-    if (typeof callback === 'undefined') {
-      callback = auditLog
-      auditLog = null
+  authenticate(query, password, auditLog, { skipHIBPCheck = false }, callback) {
+    const checkUserPassword = callback => {
+      if (skipHIBPCheck) {
+        AuthenticationManager._checkUserPasswordWithOutHIBPCheck(
+          query,
+          password,
+          callback
+        )
+      } else {
+        AuthenticationManager._checkUserPassword(query, password, callback)
+      }
     }
-    AuthenticationManager._checkUserPassword(
-      query,
-      password,
-      (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)
+    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'
+                    )
                   }
-                )
-              }
-            }
-            AuthenticationManager.checkRounds(
-              user,
-              user.hashedPassword,
-              password,
-              function (err) {
-                if (err) {
-                  return callback(err)
+                  callback(null, null)
                 }
-                callback(null, user)
-              }
-            )
+              )
+            }
           }
-        )
-      }
-    )
+          AuthenticationManager.checkRounds(
+            user,
+            user.hashedPassword,
+            password,
+            function (err) {
+              if (err) {
+                return callback(err)
+              }
+              callback(null, user)
+            }
+          )
+        }
+      )
+    })
   },
 
   validateEmail(email) {

services/web/app/src/Features/Captcha/CaptchaMiddleware.js

@@ -59,7 +59,9 @@ function validateCaptcha(action) {
     const reCaptchaResponse = req.body['g-recaptcha-response']
     if (action === 'login') {
       await initializeDeviceHistory(req)
-      if (!reCaptchaResponse && req.deviceHistory.has(req.body?.email)) {
+      const fromKnownDevice = req.deviceHistory.has(req.body?.email)
+      AuthenticationController.setAuditInfo(req, { fromKnownDevice })
+      if (!reCaptchaResponse && fromKnownDevice) {
         // The user has previously logged in from this device, which required
         //  solving a captcha or keeping the device history alive.
         // We can skip checking the (missing) captcha response.

services/web/app/src/Features/User/UserController.js

@@ -72,7 +72,9 @@ async function changePassword(req, res, next) {
 
   const user = await AuthenticationManager.promises.authenticate(
     { _id: userId },
-    req.body.currentPassword
+    req.body.currentPassword,
+    null,
+    { skipHIBPCheck: true }
   )
   if (!user) {
     return HttpErrorHandler.badRequest(
@@ -228,7 +230,9 @@ async function tryDeleteUser(req, res, next) {
 
   const user = await AuthenticationManager.promises.authenticate(
     { _id: userId },
-    password
+    password,
+    null,
+    { skipHIBPCheck: true }
   )
   if (!user) {
     logger.err({ userId }, 'auth failed during attempt to delete account')

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

@@ -16,9 +16,7 @@ block content
 								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'}}])}.
+								| !{translate('password_compromised_try_again_or_use_known_device_or_reset', {}, [{name: 'a', attrs: {href: 'https://haveibeenpwned.com', rel: 'noopener noreferrer', target: '_blank'}}, {name: 'a', attrs: {href: '/user/password/reset', target: '_blank'}}])}.
 							.form-group
 								input.form-control(
 									type='email',

services/web/locales/en.json

@@ -1280,6 +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_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.",
@@ -1338,7 +1339,6 @@
   "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/AuthenticationTests.js

@@ -69,6 +69,7 @@ describe('Authentication', function () {
       expect(auditLogEntry.info).to.deep.equal({
         method: 'Password login',
         captcha: 'solved',
+        fromKnownDevice: false,
       })
       expect(auditLogEntry.ipAddress).to.equal('127.0.0.1')
     })
@@ -101,6 +102,7 @@ describe('Authentication', function () {
       expect(auditLogEntry.operation).to.equal('failed-password-match')
       expect(auditLogEntry.info).to.deep.equal({
         method: 'Password login',
+        fromKnownDevice: true,
       })
       expect(auditLogEntry.ipAddress).to.equal('127.0.0.1')
     })

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

@@ -1,6 +1,13 @@
 const { db } = require('../../../app/src/infrastructure/mongodb')
 const { expect } = require('chai')
+const Settings = require('@overleaf/settings')
 const User = require('./helpers/User').promises
+const MockHaveIBeenPwnedApiClass = require('./mocks/MockHaveIBeenPwnedApi')
+
+let MockHaveIBeenPwnedApi
+before(function () {
+  MockHaveIBeenPwnedApi = MockHaveIBeenPwnedApiClass.instance()
+})
 
 describe('Captcha', function () {
   let user
@@ -83,6 +90,7 @@ describe('Captcha', function () {
     expect(auditLog[0].info).to.deep.equal({
       captcha: 'solved',
       method: 'Password login',
+      fromKnownDevice: false,
     })
   })
 
@@ -109,6 +117,7 @@ describe('Captcha', function () {
       expect(auditLog[1].info).to.deep.equal({
         captcha: 'skipped',
         method: 'Password login',
+        fromKnownDevice: true,
       })
     })
 
@@ -192,5 +201,45 @@ describe('Captcha', function () {
         expectBadCaptchaResponse(response, body)
       })
     })
+
+    describe('HIBP', function () {
+      before(function () {
+        Settings.apis.haveIBeenPwned.enabled = true
+      })
+      after(function () {
+        Settings.apis.haveIBeenPwned.enabled = false
+      })
+      beforeEach(async function () {
+        user = new User()
+        user.password = 'aLeakedPassword42'
+        await user.ensureUserExists()
+      })
+      beforeEach('login to populate deviceHistory', async function () {
+        const { response, body } = await loginWithCaptcha('valid')
+        expectSuccessfulLogin(response, body)
+      })
+      beforeEach(function () {
+        // echo -n aLeakedPassword42 | sha1sum
+        MockHaveIBeenPwnedApi.addPasswordByHash(
+          'D1ABBDEEE70CBE8BBCE5D9D039C53C0CE91C0C16'
+        )
+      })
+      it('should be able to skip HIBP check with deviceHistory and valid captcha', async function () {
+        const { response, body } = await loginWithCaptcha('valid')
+        expectSuccessfulLogin(response, body)
+      })
+
+      it('should be able to skip HIBP check with deviceHistory and skipped captcha', async function () {
+        const { response, body } = await loginWithCaptcha('')
+        expectSuccessfulLogin(response, body)
+      })
+
+      it('should not be able to skip HIBP check without deviceHistory', async function () {
+        user.resetCookies()
+        const { response, body } = await loginWithCaptcha('valid')
+        expect(response.statusCode).to.equal(400)
+        expect(body.message.key).to.equal('password-compromised')
+      })
+    })
   })
 })

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

@@ -165,7 +165,7 @@ describe('HaveIBeenPwnedApi', function () {
           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.`,
+            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).`,
           },
         })
       }

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

@@ -18,7 +18,9 @@ describe('UserHelper', function () {
         userHelper.user.email.should.equal(userHelper.getDefaultEmail())
         const authedUser = await AuthenticationManager.promises.authenticate(
           { _id: userHelper.user._id },
-          userHelper.getDefaultPassword()
+          userHelper.getDefaultPassword(),
+          null,
+          { skipHIBPCheck: true }
         )
         expect(authedUser).to.not.be.null
       })
@@ -32,7 +34,9 @@ describe('UserHelper', function () {
         userHelper.user.email.should.equal('foo@test.com')
         const authedUser = await AuthenticationManager.promises.authenticate(
           { _id: userHelper.user._id },
-          userHelper.getDefaultPassword()
+          userHelper.getDefaultPassword(),
+          null,
+          { skipHIBPCheck: true }
         )
         expect(authedUser).to.not.be.null
       })
@@ -46,7 +50,9 @@ describe('UserHelper', function () {
         userHelper.user.email.should.equal(userHelper.getDefaultEmail())
         const authedUser = await AuthenticationManager.promises.authenticate(
           { _id: userHelper.user._id },
-          'foofoofoo'
+          'foofoofoo',
+          null,
+          { skipHIBPCheck: true }
         )
         expect(authedUser).to.not.be.null
       })
@@ -124,7 +130,9 @@ describe('UserHelper', function () {
         userHelper.user.email.should.equal(userHelper.getDefaultEmail())
         const authedUser = await AuthenticationManager.promises.authenticate(
           { _id: userHelper.user._id },
-          userHelper.getDefaultPassword()
+          userHelper.getDefaultPassword(),
+          null,
+          { skipHIBPCheck: true }
         )
         expect(authedUser).to.not.be.null
       })
@@ -138,7 +146,9 @@ describe('UserHelper', function () {
         userHelper.user.email.should.equal('foo2@test.com')
         const authedUser = await AuthenticationManager.promises.authenticate(
           { _id: userHelper.user._id },
-          userHelper.getDefaultPassword()
+          userHelper.getDefaultPassword(),
+          null,
+          { skipHIBPCheck: true }
         )
         expect(authedUser).to.not.be.null
       })
@@ -152,7 +162,9 @@ describe('UserHelper', function () {
         userHelper.user.email.should.equal(userHelper.getDefaultEmail())
         const authedUser = await AuthenticationManager.promises.authenticate(
           { _id: userHelper.user._id },
-          'foofoofoo'
+          'foofoofoo',
+          null,
+          { skipHIBPCheck: true }
         )
         expect(authedUser).to.not.be.null
       })

services/web/test/acceptance/src/helpers/User.js

@@ -32,6 +32,13 @@ class User {
     })
   }
 
+  resetCookies() {
+    this.jar = request.jar()
+    this.request = request.defaults({
+      jar: this.jar,
+    })
+  }
+
   setExtraAttributes(user) {
     if ((user != null ? user._id : undefined) == null) {
       throw new Error('User does not exist')

services/web/test/unit/src/Authentication/AuthenticationManagerTests.js

@@ -13,6 +13,10 @@ describe('AuthenticationManager', function () {
     tk.freeze(Date.now())
     this.settings = { security: { bcryptRounds: 4 } }
     this.metrics = { inc: sinon.stub().returns() }
+    this.HaveIBeenPwned = {
+      checkPasswordForReuse: sinon.stub().yields(null, false),
+      checkPasswordForReuseInBackground: sinon.stub(),
+    }
     this.AuthenticationManager = SandboxedModule.require(modulePath, {
       requires: {
         '../../models/User': {
@@ -30,10 +34,7 @@ describe('AuthenticationManager', function () {
         '@overleaf/settings': this.settings,
         '../User/UserGetter': (this.UserGetter = {}),
         './AuthenticationErrors': AuthenticationErrors,
-        './HaveIBeenPwned': {
-          checkPasswordForReuse: sinon.stub().yields(null, false),
-          checkPasswordForReuseInBackground: sinon.stub(),
-        },
+        './HaveIBeenPwned': this.HaveIBeenPwned,
         '../User/UserAuditLogHandler': (this.UserAuditLogHandler = {
           addEntry: sinon.stub().callsArgWith(5, null),
         }),
@@ -76,6 +77,8 @@ describe('AuthenticationManager', function () {
           this.AuthenticationManager.authenticate(
             { email: this.email },
             this.unencryptedPassword,
+            null,
+            { skipHIBPCheck: true },
             (error, user) => {
               this.callback(error, user)
               done()
@@ -116,6 +119,8 @@ describe('AuthenticationManager', function () {
           this.AuthenticationManager.authenticate(
             { email: this.email },
             'notthecorrectpassword',
+            null,
+            { skipHIBPCheck: true },
             (...args) => {
               this.callback(...args)
               done()
@@ -153,6 +158,8 @@ describe('AuthenticationManager', function () {
             this.AuthenticationManager.authenticate(
               { email: this.email },
               'testpassword',
+              null,
+              { skipHIBPCheck: true },
               (...args) => {
                 this.callback(...args)
                 done()
@@ -175,6 +182,8 @@ describe('AuthenticationManager', function () {
             this.AuthenticationManager.authenticate(
               { email: this.email },
               'notthecorrectpassword',
+              null,
+              { skipHIBPCheck: true },
               (...args) => {
                 this.callback(...args)
                 done()
@@ -264,6 +273,8 @@ describe('AuthenticationManager', function () {
           this.AuthenticationManager.authenticate(
             { email: this.email },
             this.unencryptedPassword,
+            null,
+            { skipHIBPCheck: true },
             (error, user) => {
               this.callback(error, user)
               done()
@@ -292,6 +303,29 @@ describe('AuthenticationManager', function () {
         it('should return the user', function () {
           this.callback.calledWith(null, this.user).should.equal(true)
         })
+
+        describe('HIBP', function () {
+          it('should not check HIBP if not requested', function () {
+            this.HaveIBeenPwned.checkPasswordForReuse.should.not.have.been
+              .called
+          })
+
+          it('should check HIBP if requested', function (done) {
+            this.AuthenticationManager.authenticate(
+              { email: this.email },
+              this.unencryptedPassword,
+              null,
+              { skipHIBPCheck: false },
+              error => {
+                if (error) return done(error)
+                this.HaveIBeenPwned.checkPasswordForReuse.should.have.been.calledWith(
+                  this.unencryptedPassword
+                )
+                done()
+              }
+            )
+          })
+        })
       })
 
       describe('when the encrypted passwords do not match', function () {
@@ -301,6 +335,8 @@ describe('AuthenticationManager', function () {
           this.AuthenticationManager.authenticate(
             { email: this.email },
             this.unencryptedPassword,
+            null,
+            { skipHIBPCheck: true },
             this.callback
           )
         })
@@ -320,6 +356,7 @@ describe('AuthenticationManager', function () {
             { email: this.email },
             this.unencryptedPassword,
             this.auditLog,
+            { skipHIBPCheck: true },
             this.callback
           )
         })
@@ -350,6 +387,8 @@ describe('AuthenticationManager', function () {
           this.AuthenticationManager.authenticate(
             { email: this.email },
             this.unencryptedPassword,
+            null,
+            { skipHIBPCheck: true },
             (error, user) => {
               this.callback(error, user)
               done()
@@ -398,6 +437,8 @@ describe('AuthenticationManager', function () {
           this.AuthenticationManager.authenticate(
             { email: this.email },
             this.unencryptedPassword,
+            null,
+            { skipHIBPCheck: true },
             (error, user) => {
               this.callback(error, user)
               done()
@@ -431,6 +472,8 @@ describe('AuthenticationManager', function () {
         this.AuthenticationManager.authenticate(
           { email: this.email },
           this.unencrpytedPassword,
+          null,
+          { skipHIBPCheck: true },
           this.callback
         )
       })