Merge pull request #6457 from overleaf/jpa-harden-login

[web] harden login process

GitOrigin-RevId: 5c0b7cc725efd5e3e879067ad8a42fe46a47b60d

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

@@ -23,6 +23,7 @@ const AnalyticsRegistrationSourceHelper = require('../Analytics/AnalyticsRegistr
 const {
   acceptsJson,
 } = require('../../infrastructure/RequestContentTypeDetection')
+const { ParallelLoginError } = require('./AuthenticationErrors')
 
 function send401WithChallenge(res) {
   res.setHeader('WWW-Authenticate', 'OverleafLogin')
@@ -89,7 +90,13 @@ const AuthenticationController = {
         } else {
           res.status(info.status || 200)
           delete info.status
-          return res.json({ message: info })
+          const body = { message: info }
+          const { errorReason } = info
+          if (errorReason) {
+            body.errorReason = errorReason
+            delete info.errorReason
+          }
+          return res.json(body)
         }
       }
     })(req, res, next)
@@ -188,9 +195,22 @@ const AuthenticationController = {
             password,
             function (error, user) {
               if (error != null) {
+                if (error instanceof ParallelLoginError) {
+                  return done(null, false, { status: 429 })
+                }
                 return done(error)
               }
-              if (user != null) {
+              if (
+                user &&
+                AuthenticationController.captchaRequiredForLogin(req, user)
+              ) {
+                done(null, false, {
+                  text: req.i18n.translate('cannot_verify_user_not_robot'),
+                  type: 'error',
+                  errorReason: 'cannot_verify_user_not_robot',
+                  status: 400,
+                })
+              } else if (user) {
                 // async actions
                 done(null, user)
               } else {
@@ -209,6 +229,30 @@ const AuthenticationController = {
     )
   },
 
+  captchaRequiredForLogin(req, user) {
+    switch (AuthenticationController.getAuditInfo(req).captcha) {
+      case 'disabled':
+        return false
+      case 'solved':
+        return false
+      case 'skipped': {
+        let required = false
+        if (user.lastFailedLogin) {
+          const requireCaptchaUntil =
+            user.lastFailedLogin.getTime() +
+            Settings.elevateAccountSecurityAfterFailedLogin
+          required = requireCaptchaUntil >= Date.now()
+        }
+        Metrics.inc('force_captcha_on_login', 1, {
+          status: required ? 'yes' : 'no',
+        })
+        return required
+      }
+      default:
+        throw new Error('captcha middleware missing in handler chain')
+    }
+  },
+
   ipMatchCheck(req, user) {
     if (req.ip !== user.lastLoginIp) {
       NotificationsBuilder.ipMatcherAffiliation(user._id).create(

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

@@ -2,8 +2,10 @@ const Errors = require('../Errors/Errors')
 
 class InvalidEmailError extends Errors.BackwardCompatibleError {}
 class InvalidPasswordError extends Errors.BackwardCompatibleError {}
+class ParallelLoginError extends Errors.BackwardCompatibleError {}
 
 module.exports = {
   InvalidEmailError,
   InvalidPasswordError,
+  ParallelLoginError,
 }

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

@@ -6,6 +6,7 @@ const EmailHelper = require('../Helpers/EmailHelper')
 const {
   InvalidEmailError,
   InvalidPasswordError,
+  ParallelLoginError,
 } = require('./AuthenticationErrors')
 const util = require('util')
 const HaveIBeenPwned = require('./HaveIBeenPwned')
@@ -38,19 +39,36 @@ const AuthenticationManager = {
         if (error) {
           return callback(error)
         }
+        const update = { $inc: { loginEpoch: 1 } }
         if (!match) {
-          return callback(null, null)
+          update.$set = { lastFailedLogin: new Date() }
         }
-        AuthenticationManager.checkRounds(
+        User.updateOne(
-          user,
+          { _id: user._id, loginEpoch: user.loginEpoch },
-          user.hashedPassword,
+          update,
-          password,
+          {},
-          function (err) {
+          (err, result) => {
             if (err) {
               return callback(err)
             }
-            callback(null, user)
+            if (result.nModified !== 1) {
-            HaveIBeenPwned.checkPasswordForReuseInBackground(password)
+              return callback(new ParallelLoginError())
+            }
+            if (!match) {
+              return callback(null, null)
+            }
+            AuthenticationManager.checkRounds(
+              user,
+              user.hashedPassword,
+              password,
+              function (err) {
+                if (err) {
+                  return callback(err)
+                }
+                callback(null, user)
+                HaveIBeenPwned.checkPasswordForReuseInBackground(password)
+              }
+            )
           }
         )
       })

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

@@ -6,11 +6,11 @@ const DeviceHistory = require('./DeviceHistory')
 const AuthenticationController = require('../Authentication/AuthenticationController')
 const { expressify } = require('../../util/promises')
 
-function respondInvalidCaptcha(res) {
+function respondInvalidCaptcha(req, res) {
   res.status(400).json({
     errorReason: 'cannot_verify_user_not_robot',
     message: {
-      text: 'Sorry, we could not verify that you are not a robot. Please check that Google reCAPTCHA is not being blocked by an ad blocker or firewall.',
+      text: req.i18n.translate('cannot_verify_user_not_robot'),
     },
   })
 }
@@ -36,24 +36,27 @@ async function canSkipCaptcha(req, res) {
 function validateCaptcha(action) {
   return expressify(async function (req, res, next) {
     if (!Settings.recaptcha?.siteKey || Settings.recaptcha.disabled[action]) {
+      if (action === 'login') {
+        AuthenticationController.setAuditInfo(req, { captcha: 'disabled' })
+      }
       Metrics.inc('captcha', 1, { path: action, status: 'disabled' })
       return next()
     }
+    const reCaptchaResponse = req.body['g-recaptcha-response']
     if (action === 'login') {
       await initializeDeviceHistory(req)
-      if (req.deviceHistory.has(req.body?.email)) {
+      if (!reCaptchaResponse && req.deviceHistory.has(req.body?.email)) {
         // 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 (potentially missing) captcha response.
+        // We can skip checking the (missing) captcha response.
         AuthenticationController.setAuditInfo(req, { captcha: 'skipped' })
         Metrics.inc('captcha', 1, { path: action, status: 'skipped' })
         return next()
       }
     }
-    const reCaptchaResponse = req.body['g-recaptcha-response']
     if (!reCaptchaResponse) {
       Metrics.inc('captcha', 1, { path: action, status: 'missing' })
-      return respondInvalidCaptcha(res)
+      return respondInvalidCaptcha(req, res)
     }
     const options = {
       method: 'POST',
@@ -84,7 +87,7 @@ function validateCaptcha(action) {
         'failed recaptcha siteverify request'
       )
       Metrics.inc('captcha', 1, { path: action, status: 'failed' })
-      return respondInvalidCaptcha(res)
+      return respondInvalidCaptcha(req, res)
     }
     Metrics.inc('captcha', 1, { path: action, status: 'solved' })
     if (action === 'login') {
@@ -95,6 +98,7 @@ function validateCaptcha(action) {
 }
 
 module.exports = {
+  respondInvalidCaptcha,
   validateCaptcha,
   canSkipCaptcha: expressify(canSkipCaptcha),
 }

services/web/app/src/models/User.js

@@ -56,7 +56,9 @@ const UserSchema = new Schema({
       return new Date()
     },
   },
+  loginEpoch: { type: Number },
   lastActive: { type: Date },
+  lastFailedLogin: { type: Date },
   lastLoggedIn: { type: Date },
   lastLoginIp: { type: String, default: '' },
   loginCount: { type: Number, default: 0 },

services/web/app/src/router.js

@@ -106,7 +106,11 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
   if (Settings.enableLegacyLogin) {
     AuthenticationController.addEndpointToLoginWhitelist('/login/legacy')
     webRouter.get('/login/legacy', UserPagesController.loginPage)
-    webRouter.post('/login/legacy', AuthenticationController.passportLogin)
+    webRouter.post(
+      '/login/legacy',
+      CaptchaMiddleware.validateCaptcha('login'),
+      AuthenticationController.passportLogin
+    )
   }
 
   webRouter.get(

services/web/config/settings.defaults.js

@@ -435,6 +435,10 @@ module.exports = {
     },
   },
 
+  elevateAccountSecurityAfterFailedLogin:
+    parseInt(process.env.ELEVATED_ACCOUNT_SECURITY_AFTER_FAILED_LOGIN_MS, 10) ||
+    24 * 60 * 60 * 1000,
+
   deviceHistory: {
     cookieName: process.env.DEVICE_HISTORY_COOKIE_NAME || 'deviceHistory',
     entryExpiry:

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

@@ -1,3 +1,4 @@
+const { db } = require('../../../app/src/infrastructure/mongodb')
 const { expect } = require('chai')
 const User = require('./helpers/User').promises
 
@@ -9,22 +10,26 @@ describe('Captcha', function () {
     await user.ensureUserExists()
   })
 
-  async function loginWithCaptcha(captchaResponse) {
+  async function login(email, password, captchaResponse) {
-    return loginWithEmailAndCaptcha(user.email, captchaResponse)
-  }
-
-  async function loginWithEmailAndCaptcha(email, captchaResponse) {
     await user.getCsrfToken()
     return user.doRequest('POST', {
       url: '/login',
       json: {
         email,
-        password: user.password,
+        password,
         'g-recaptcha-response': captchaResponse,
       },
     })
   }
 
+  async function loginWithCaptcha(captchaResponse) {
+    return login(user.email, user.password, captchaResponse)
+  }
+
+  async function loginWithEmailAndCaptcha(email, captchaResponse) {
+    return login(email, user.password, captchaResponse)
+  }
+
   async function canSkipCaptcha(email) {
     await user.getCsrfToken()
     const { response, body } = await user.doRequest('POST', {
@@ -45,6 +50,16 @@ describe('Captcha', function () {
     expect(body).to.deep.equal({ redir: '/project' })
   }
 
+  function expectBadLogin(response, body) {
+    expect(response.statusCode).to.equal(401)
+    expect(body).to.deep.equal({
+      message: {
+        text: 'Your email or password is incorrect. Please try again',
+        type: 'error',
+      },
+    })
+  }
+
   it('should reject a login without captcha response', async function () {
     const { response, body } = await loginWithCaptcha('')
     expectBadCaptchaResponse(response, body)
@@ -104,6 +119,58 @@ describe('Captcha', function () {
       expectBadCaptchaResponse(response, body)
     })
 
+    describe('login failure', function () {
+      beforeEach(async function () {
+        const { response, body } = await login(
+          user.email,
+          'bad password',
+          'valid'
+        )
+        expectBadLogin(response, body)
+      })
+
+      it('should be able to skip captcha per device history', async function () {
+        expect(await canSkipCaptcha(user.email)).to.equal(true)
+      })
+
+      it('should request a captcha despite device history entry', async function () {
+        const { response, body } = await loginWithCaptcha('')
+        expectBadCaptchaResponse(response, body)
+      })
+
+      it('should accept the login with captcha', async function () {
+        const { response, body } = await loginWithCaptcha('valid')
+        expectSuccessfulLogin(response, body)
+      })
+
+      describe('when the login failure happened a long time ago', function () {
+        beforeEach(async function () {
+          db.users.updateOne(
+            { email: user.email },
+            {
+              $set: {
+                lastFailedLogin: new Date(
+                  Date.now() - 90 * 24 * 60 * 60 * 1000
+                ),
+              },
+            }
+          )
+        })
+
+        it('should be able to skip captcha per device history', async function () {
+          expect(await canSkipCaptcha(user.email)).to.equal(true)
+        })
+        it('should accept the login without captcha', async function () {
+          const { response, body } = await loginWithCaptcha('')
+          expectSuccessfulLogin(response, body)
+        })
+        it('should accept the login with captcha', async function () {
+          const { response, body } = await loginWithCaptcha('valid')
+          expectSuccessfulLogin(response, body)
+        })
+      })
+    })
+
     describe('cycle history', function () {
       beforeEach('create and login with 10 other users', async function () {
         for (let i = 0; i < 10; i++) {

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

@@ -7,6 +7,7 @@ const tk = require('timekeeper')
 const MockRequest = require('../helpers/MockRequest')
 const MockResponse = require('../helpers/MockResponse')
 const { ObjectId } = require('mongodb')
+const AuthenticationErrors = require('../../../../app/src/Features/Authentication/AuthenticationErrors')
 
 describe('AuthenticationController', function () {
   beforeEach(function () {
@@ -32,6 +33,7 @@ describe('AuthenticationController', function () {
 
     this.AuthenticationController = SandboxedModule.require(modulePath, {
       requires: {
+        './AuthenticationErrors': AuthenticationErrors,
         '../User/UserAuditLogHandler': (this.UserAuditLogHandler = {
           addEntry: sinon.stub().yields(null),
         }),
@@ -63,6 +65,7 @@ describe('AuthenticationController', function () {
         '@overleaf/settings': (this.Settings = {
           siteUrl: 'http://www.foo.bar',
           httpAuthUsers: this.httpAuthUsers,
+          elevateAccountSecurityAfterFailedLogin: 90 * 24 * 60 * 60 * 1000,
         }),
         passport: (this.passport = {
           authenticate: sinon.stub().returns(sinon.stub()),
@@ -254,7 +257,7 @@ describe('AuthenticationController', function () {
           this.next
         )
         this.res.json.callCount.should.equal(1)
-        this.res.json.calledWith({ message: this.info }).should.equal(true)
+        this.res.json.should.have.been.calledWith({ message: this.info })
         expect(this.res.json.lastCall.args[0].redir != null).to.equal(false)
       })
     })
@@ -273,6 +276,7 @@ describe('AuthenticationController', function () {
           postLoginRedirect: '/path/to/redir/to',
         },
       }
+      this.req.__authAuditInfo = { captcha: 'disabled' }
       this.cb = sinon.stub()
     })
 
@@ -325,22 +329,103 @@ describe('AuthenticationController', function () {
           .stub()
           .callsArgWith(2, null, this.user)
         this.req.sessionID = Math.random()
-        this.AuthenticationController.doPassportLogin(
-          this.req,
-          this.req.body.email,
-          this.req.body.password,
-          this.cb
-        )
       })
 
-      it('should attempt to authorise the user', function () {
+      describe('happy path', function () {
-        this.AuthenticationManager.authenticate
+        beforeEach(function () {
-          .calledWith({ email: this.email.toLowerCase() }, this.password)
+          this.AuthenticationController.doPassportLogin(
-          .should.equal(true)
+            this.req,
+            this.req.body.email,
+            this.req.body.password,
+            this.cb
+          )
+        })
+        it('should attempt to authorise the user', function () {
+          this.AuthenticationManager.authenticate
+            .calledWith({ email: this.email.toLowerCase() }, this.password)
+            .should.equal(true)
+        })
+
+        it("should establish the user's session", function () {
+          this.cb.calledWith(null, this.user).should.equal(true)
+        })
       })
 
-      it("should establish the user's session", function () {
+      describe('when authenticate flags a parallel login', function () {
-        this.cb.calledWith(null, this.user).should.equal(true)
+        beforeEach(function () {
+          this.AuthenticationManager.authenticate = sinon
+            .stub()
+            .callsArgWith(2, new AuthenticationErrors.ParallelLoginError())
+          this.AuthenticationController.doPassportLogin(
+            this.req,
+            this.req.body.email,
+            this.req.body.password,
+            this.cb
+          )
+        })
+
+        it('should send a 429', function () {
+          this.cb.should.have.been.calledWith(null, false, { status: 429 })
+        })
+      })
+
+      describe('with a user having a recent failed login ', function () {
+        beforeEach(function () {
+          this.user.lastFailedLogin = new Date()
+        })
+
+        describe('with captcha disabled', function () {
+          beforeEach(function () {
+            this.req.__authAuditInfo.captcha = 'disabled'
+            this.AuthenticationController.doPassportLogin(
+              this.req,
+              this.req.body.email,
+              this.req.body.password,
+              this.cb
+            )
+          })
+
+          it('should let the user log in', function () {
+            this.cb.should.have.been.calledWith(null, this.user)
+          })
+        })
+
+        describe('with a solved captcha', function () {
+          beforeEach(function () {
+            this.req.__authAuditInfo.captcha = 'solved'
+            this.AuthenticationController.doPassportLogin(
+              this.req,
+              this.req.body.email,
+              this.req.body.password,
+              this.cb
+            )
+          })
+
+          it('should let the user log in', function () {
+            this.cb.should.have.been.calledWith(null, this.user)
+          })
+        })
+
+        describe('with a skipped captcha', function () {
+          beforeEach(function () {
+            this.req.__authAuditInfo.captcha = 'skipped'
+            this.AuthenticationController.doPassportLogin(
+              this.req,
+              this.req.body.email,
+              this.req.body.password,
+              this.cb
+            )
+          })
+
+          it('should request a captcha', function () {
+            this.cb.should.have.been.calledWith(null, false, {
+              text: 'cannot_verify_user_not_robot',
+              type: 'error',
+              errorReason: 'cannot_verify_user_not_robot',
+              status: 400,
+            })
+          })
+        })
       })
     })
 

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

@@ -3,17 +3,21 @@ const { expect } = require('chai')
 const SandboxedModule = require('sandboxed-module')
 const { ObjectId } = require('mongodb')
 const AuthenticationErrors = require('../../../../app/src/Features/Authentication/AuthenticationErrors')
+const tk = require('timekeeper')
 
 const modulePath =
   '../../../../app/src/Features/Authentication/AuthenticationManager.js'
 
 describe('AuthenticationManager', function () {
   beforeEach(function () {
+    tk.freeze(Date.now())
     this.settings = { security: { bcryptRounds: 4 } }
     this.AuthenticationManager = SandboxedModule.require(modulePath, {
       requires: {
         '../../models/User': {
-          User: (this.User = {}),
+          User: (this.User = {
+            updateOne: sinon.stub().callsArgWith(3, null, { nModified: 1 }),
+          }),
         },
         '../../infrastructure/mongodb': {
           db: (this.db = { users: {} }),
@@ -31,6 +35,10 @@ describe('AuthenticationManager', function () {
     this.callback = sinon.stub()
   })
 
+  afterEach(function () {
+    tk.reset()
+  })
+
   describe('with real bcrypt', function () {
     beforeEach(function () {
       const bcrypt = require('bcrypt')
@@ -49,13 +57,13 @@ describe('AuthenticationManager', function () {
           _id: 'user-id',
           email: (this.email = 'USER@sharelatex.com'),
         }
+        this.user.hashedPassword = this.testPassword
         this.User.findOne = sinon.stub().callsArgWith(1, null, this.user)
       })
 
       describe('when the hashed password matches', function () {
         beforeEach(function (done) {
           this.unencryptedPassword = 'testpassword'
-          this.user.hashedPassword = this.testPassword
           this.AuthenticationManager.authenticate(
             { email: this.email },
             this.unencryptedPassword,
@@ -70,17 +78,46 @@ describe('AuthenticationManager', function () {
           this.User.findOne.calledWith({ email: this.email }).should.equal(true)
         })
 
+        it('should bump epoch', function () {
+          this.User.updateOne.should.have.been.calledWith(
+            {
+              _id: this.user._id,
+              loginEpoch: this.user.loginEpoch,
+            },
+            {
+              $inc: { loginEpoch: 1 },
+            },
+            {}
+          )
+        })
+
         it('should return the user', function () {
           this.callback.calledWith(null, this.user).should.equal(true)
         })
       })
 
       describe('when the encrypted passwords do not match', function () {
-        beforeEach(function () {
+        beforeEach(function (done) {
           this.AuthenticationManager.authenticate(
             { email: this.email },
             'notthecorrectpassword',
-            this.callback
+            (...args) => {
+              this.callback(...args)
+              done()
+            }
+          )
+        })
+
+        it('should persist the login failure and bump epoch', function () {
+          this.User.updateOne.should.have.been.calledWith(
+            {
+              _id: this.user._id,
+              loginEpoch: this.user.loginEpoch,
+            },
+            {
+              $inc: { loginEpoch: 1 },
+              $set: { lastFailedLogin: new Date() },
+            }
           )
         })
 
@@ -88,6 +125,52 @@ describe('AuthenticationManager', function () {
           this.callback.calledWith(null, null).should.equal(true)
         })
       })
+
+      describe('when another request runs in parallel', function () {
+        beforeEach(function () {
+          this.User.updateOne = sinon
+            .stub()
+            .callsArgWith(3, null, { nModified: 0 })
+        })
+
+        describe('correct password', function () {
+          beforeEach(function (done) {
+            this.AuthenticationManager.authenticate(
+              { email: this.email },
+              'testpassword',
+              (...args) => {
+                this.callback(...args)
+                done()
+              }
+            )
+          })
+
+          it('should return an error', function () {
+            this.callback.should.have.been.calledWith(
+              sinon.match.instanceOf(AuthenticationErrors.ParallelLoginError)
+            )
+          })
+        })
+
+        describe('bad password', function () {
+          beforeEach(function (done) {
+            this.User.updateOne = sinon.stub().yields(null, { nModified: 0 })
+            this.AuthenticationManager.authenticate(
+              { email: this.email },
+              'notthecorrectpassword',
+              (...args) => {
+                this.callback(...args)
+                done()
+              }
+            )
+          })
+          it('should return an error', function () {
+            this.callback.should.have.been.calledWith(
+              sinon.match.instanceOf(AuthenticationErrors.ParallelLoginError)
+            )
+          })
+        })
+      })
     })
 
     describe('setUserPasswordInV2', function () {