[web] Merge authentication error handling (V1LoginController & AuthenticationController) (#19457)

* Promisify `AuthenticationController.doPassportLogin`

* Update tests `AuthenticationController.doPassportLogin`

* Add test on error handling for `AuthenticationController.doPassportLogin`

* Add test on error handling for `V1LoginController.doLogin`

* Extract error handling to `getErrorObject` function

* Simplify code

* Add `Metrics` calls

* Add `password is too long` in AuthenticationController

* Make `info` object consistent with the rest of the codebase

* Move error handling to `AuthenticationManager.handleAuthenticateErrors`

* Move `handleAuthenticateErrors` to other file

I moved this solely because I didn't manage to test it otherwise

* Update tests

* Remove `preDoPassportLogin` hook call

* Remove test on `preDoPassportLogin`

* Use try/catch block instead of `.catch()`

* Revert "Use try/catch block instead of `.catch()`"

This reverts commit 3475afa93ce4af7ad55c91bfc1d7ad3317600ea5.

* Replace `.catch` by `try/catch`

GitOrigin-RevId: 3fba65c30a2c5fc6e5abcd5b83c52801852ed462

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

@@ -22,13 +22,10 @@ const AnalyticsRegistrationSourceHelper = require('../Analytics/AnalyticsRegistr
 const {
   acceptsJson,
 } = require('../../infrastructure/RequestContentTypeDetection')
-const {
-  ParallelLoginError,
-  PasswordReusedError,
-} = require('./AuthenticationErrors')
 const { hasAdminAccess } = require('../Helpers/AdminAuthorizationHelper')
 const Modules = require('../../infrastructure/Modules')
 const { expressify, promisify } = require('@overleaf/promise-utils')
+const { handleAuthenticateErrors } = require('./AuthenticationErrors')
 
 function send401WithChallenge(res) {
   res.setHeader('WWW-Authenticate', 'OverleafLogin')
@@ -198,67 +195,63 @@ const AuthenticationController = {
     )
   },
 
-  doPassportLogin(req, username, password, done) {
+  async doPassportLogin(req, username, password, done) {
-    const email = username.toLowerCase()
+    let user, info
-    Modules.hooks.fire(
+    try {
-      'preDoPassportLogin',
+      ;({ user, info } = await AuthenticationController._doPassportLogin(
         req,
-      email,
+        username,
-      function (err, infoList) {
+        password
-        if (err) {
+      ))
-          return done(err)
+    } catch (error) {
-        }
+      return done(error)
-        const info = infoList.find(i => i != null)
-        if (info != null) {
-          return done(null, false, info)
     }
+    return done(undefined, user, info)
+  },
+
+  /**
+   *
+   * @param req
+   * @param username
+   * @param password
+   * @returns {Promise<{ user: any, info: any}>}
+   */
+  async _doPassportLogin(req, username, password) {
+    const email = username.toLowerCase()
     const { fromKnownDevice } = AuthenticationController.getAuditInfo(req)
     const auditLog = {
       ipAddress: req.ip,
       info: { method: 'Password login', fromKnownDevice },
     }
-        AuthenticationManager.authenticate(
+
+    let user, isPasswordReused
+    try {
+      ;({ user, isPasswordReused } =
+        await AuthenticationManager.promises.authenticate(
           { email },
           password,
           auditLog,
           {
             enforceHIBPCheck: !fromKnownDevice,
-          },
-          function (error, user, isPasswordReused) {
-            if (error != null) {
-              if (error instanceof ParallelLoginError) {
-                return done(null, false, { status: 429 })
-              } else if (error instanceof PasswordReusedError) {
-                const text = `${req.i18n
-                  .translate(
-                    'password_compromised_try_again_or_use_known_device_or_reset'
-                  )
-                  .replace('<0>', '')
-                  .replace('</0>', ' (https://haveibeenpwned.com/passwords)')
-                  .replace('<1>', '')
-                  .replace(
-                    '</1>',
-                    ` (${Settings.siteUrl}/user/password/reset)`
-                  )}.`
-                return done(null, false, {
-                  status: 400,
-                  type: 'error',
-                  key: 'password-compromised',
-                  text,
-                })
           }
-              return done(error)
+        ))
+    } catch (error) {
+      return {
+        user: false,
+        info: handleAuthenticateErrors(error, req),
       }
-            if (
+    }
-              user &&
+
-              AuthenticationController.captchaRequiredForLogin(req, user)
+    if (user && AuthenticationController.captchaRequiredForLogin(req, user)) {
-            ) {
+      return {
-              done(null, false, {
+        user: false,
+        info: {
           text: req.i18n.translate('cannot_verify_user_not_robot'),
           type: 'error',
           errorReason: 'cannot_verify_user_not_robot',
           status: 400,
-              })
+        },
+      }
     } else if (user) {
       if (
         isPasswordReused &&
@@ -271,20 +264,19 @@ const AuthenticationController = {
       }
 
       // async actions
-              done(null, user)
+      return { user, info: undefined }
     } else {
       AuthenticationController._recordFailedLogin()
       logger.debug({ email }, 'failed log in')
-              done(null, false, {
+      return {
+        user: false,
+        info: {
           type: 'error',
           key: 'invalid-password-retry-or-reset',
           status: 401,
-              })
+        },
       }
     }
-        )
-      }
-    )
   },
 
   captchaRequiredForLogin(req, user) {

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

@@ -1,3 +1,6 @@
+const Metrics = require('@overleaf/metrics')
+const OError = require('@overleaf/o-error')
+const Settings = require('@overleaf/settings')
 const Errors = require('../Errors/Errors')
 
 class InvalidEmailError extends Errors.BackwardCompatibleError {}
@@ -6,10 +9,50 @@ class ParallelLoginError extends Errors.BackwardCompatibleError {}
 class PasswordMustBeDifferentError extends Errors.BackwardCompatibleError {}
 class PasswordReusedError extends Errors.BackwardCompatibleError {}
 
+function handleAuthenticateErrors(error, req) {
+  if (error.message === 'password is too long') {
+    Metrics.inc('login_failure_reason', 1, {
+      status: 'password_is_too_long',
+    })
+    return {
+      status: 422,
+      type: 'error',
+      key: 'password-too-long',
+      text: req.i18n.translate('password_too_long_please_reset'),
+    }
+  }
+  if (error instanceof ParallelLoginError) {
+    Metrics.inc('login_failure_reason', 1, { status: 'parallel_login' })
+    return { status: 429 }
+  }
+  if (error instanceof PasswordReusedError) {
+    Metrics.inc('login_failure_reason', 1, {
+      status: 'password_compromised',
+    })
+    const text = `${req.i18n
+      .translate('password_compromised_try_again_or_use_known_device_or_reset')
+      .replace('<0>', '')
+      .replace('</0>', ' (https://haveibeenpwned.com/passwords)')
+      .replace('<1>', '')
+      .replace('</1>', ` (${Settings.siteUrl}/user/password/reset)`)}.`
+    return {
+      status: 400,
+      type: 'error',
+      key: 'password-compromised',
+      text,
+    }
+  }
+  Metrics.inc('login_failure_reason', 1, {
+    status: error instanceof OError ? error.name : 'error',
+  })
+  throw error
+}
+
 module.exports = {
   InvalidEmailError,
   InvalidPasswordError,
   ParallelLoginError,
   PasswordMustBeDifferentError,
   PasswordReusedError,
+  handleAuthenticateErrors,
 }

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

@@ -78,7 +78,9 @@ describe('AuthenticationController', function () {
         '../../infrastructure/RequestContentTypeDetection': {
           acceptsJson: (this.acceptsJson = sinon.stub().returns(false)),
         },
-        './AuthenticationManager': (this.AuthenticationManager = {}),
+        './AuthenticationManager': (this.AuthenticationManager = {
+          promises: {},
+        }),
         '../User/UserUpdater': (this.UserUpdater = {
           updateUser: sinon.stub(),
         }),
@@ -86,6 +88,10 @@ describe('AuthenticationController', function () {
         '../Security/LoginRateLimiter': (this.LoginRateLimiter = {
           processLoginRequest: sinon.stub(),
           recordSuccessfulLogin: sinon.stub(),
+          promises: {
+            processLoginRequest: sinon.stub(),
+            recordSuccessfulLogin: sinon.stub(),
+          },
         }),
         '../User/UserHandler': (this.UserHandler = {
           setupLoginData: sinon.stub(),
@@ -365,74 +371,95 @@ describe('AuthenticationController', function () {
       this.cb = sinon.stub()
     })
 
-    describe('when the preDoPassportLogin hooks produce an info object', function () {
+    describe('when the authentication errors', function () {
       beforeEach(function () {
-        this.Modules.hooks.fire = sinon
+        this.LoginRateLimiter.promises.processLoginRequest.resolves(true)
+        this.errorsWith = (error, done) => {
+          this.AuthenticationManager.promises.authenticate = sinon
             .stub()
-          .yields(null, [null, { redir: '/somewhere' }, null])
+            .rejects(error)
-      })
-
-      it('should stop early and call done with this info object', function (done) {
           this.AuthenticationController.doPassportLogin(
             this.req,
             this.req.body.email,
             this.req.body.password,
-          this.cb
+            this.cb.callsFake(() => done())
           )
-        this.cb.callCount.should.equal(1)
+        }
-        this.cb
+      })
-          .calledWith(null, false, { redir: '/somewhere' })
+      describe('with "password is too long"', function () {
-          .should.equal(true)
+        beforeEach(function (done) {
-        this.LoginRateLimiter.processLoginRequest.callCount.should.equal(0)
+          this.errorsWith(new Error('password is too long'), done)
-        done()
+        })
+        it('should send a 429', function () {
+          this.cb.should.have.been.calledWith(undefined, false, {
+            status: 422,
+            type: 'error',
+            key: 'password-too-long',
+            text: 'password_too_long_please_reset',
+          })
+        })
+      })
+      describe('with ParallelLoginError', function () {
+        beforeEach(function (done) {
+          this.errorsWith(new AuthenticationErrors.ParallelLoginError(), done)
+        })
+        it('should send a 429', function () {
+          this.cb.should.have.been.calledWith(undefined, false, {
+            status: 429,
+          })
+        })
+      })
+      describe('with PasswordReusedError', function () {
+        beforeEach(function (done) {
+          this.errorsWith(new AuthenticationErrors.PasswordReusedError(), done)
+        })
+        it('should send a 400', function () {
+          this.cb.should.have.been.calledWith(undefined, false, {
+            status: 400,
+            type: 'error',
+            key: 'password-compromised',
+            text: 'password_compromised_try_again_or_use_known_device_or_reset.',
+          })
+        })
+      })
+      describe('with another error', function () {
+        const err = new Error('unhandled error')
+        beforeEach(function (done) {
+          this.errorsWith(err, done)
+        })
+        it('should send a 400', function () {
+          this.cb.should.have.been.calledWith(err)
+        })
       })
     })
 
     describe('when the user is authenticated', function () {
       beforeEach(function () {
         this.cb = sinon.stub()
-        this.LoginRateLimiter.processLoginRequest.yields(null, true)
+        this.LoginRateLimiter.promises.processLoginRequest.resolves(true)
-        this.AuthenticationManager.authenticate = sinon
+        this.AuthenticationManager.promises.authenticate = sinon
           .stub()
-          .yields(null, this.user)
+          .resolves({ user: this.user })
         this.req.sessionID = Math.random()
       })
 
       describe('happy path', function () {
-        beforeEach(function () {
+        beforeEach(function (done) {
           this.AuthenticationController.doPassportLogin(
             this.req,
             this.req.body.email,
             this.req.body.password,
-            this.cb
+            this.cb.callsFake(() => done())
           )
         })
         it('should attempt to authorise the user', function () {
-          this.AuthenticationManager.authenticate
+          this.AuthenticationManager.promises.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)
+          this.cb.calledWith(undefined, this.user).should.equal(true)
-        })
-      })
-
-      describe('when authenticate flags a parallel login', function () {
-        beforeEach(function () {
-          this.AuthenticationManager.authenticate = sinon
-            .stub()
-            .yields(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 })
         })
       })
 
@@ -442,50 +469,50 @@ describe('AuthenticationController', function () {
         })
 
         describe('with captcha disabled', function () {
-          beforeEach(function () {
+          beforeEach(function (done) {
             this.req.__authAuditInfo.captcha = 'disabled'
             this.AuthenticationController.doPassportLogin(
               this.req,
               this.req.body.email,
               this.req.body.password,
-              this.cb
+              this.cb.callsFake(() => done())
             )
           })
 
           it('should let the user log in', function () {
-            this.cb.should.have.been.calledWith(null, this.user)
+            this.cb.should.have.been.calledWith(undefined, this.user)
           })
         })
 
         describe('with a solved captcha', function () {
-          beforeEach(function () {
+          beforeEach(function (done) {
             this.req.__authAuditInfo.captcha = 'solved'
             this.AuthenticationController.doPassportLogin(
               this.req,
               this.req.body.email,
               this.req.body.password,
-              this.cb
+              this.cb.callsFake(() => done())
             )
           })
 
           it('should let the user log in', function () {
-            this.cb.should.have.been.calledWith(null, this.user)
+            this.cb.should.have.been.calledWith(undefined, this.user)
           })
         })
 
         describe('with a skipped captcha', function () {
-          beforeEach(function () {
+          beforeEach(function (done) {
             this.req.__authAuditInfo.captcha = 'skipped'
             this.AuthenticationController.doPassportLogin(
               this.req,
               this.req.body.email,
               this.req.body.password,
-              this.cb
+              this.cb.callsFake(() => done())
             )
           })
 
           it('should request a captcha', function () {
-            this.cb.should.have.been.calledWith(null, false, {
+            this.cb.should.have.been.calledWith(undefined, false, {
               text: 'cannot_verify_user_not_robot',
               type: 'error',
               errorReason: 'cannot_verify_user_not_robot',
@@ -497,12 +524,12 @@ describe('AuthenticationController', function () {
     })
 
     describe('when the user is not authenticated', function () {
-      beforeEach(function () {
+      beforeEach(function (done) {
-        this.LoginRateLimiter.processLoginRequest.yields(null, true)
+        this.LoginRateLimiter.promises.processLoginRequest.resolves(true)
-        this.AuthenticationManager.authenticate = sinon
+        this.AuthenticationManager.promises.authenticate = sinon
           .stub()
-          .yields(null, null)
+          .resolves({ user: null })
-        this.cb = sinon.stub()
+        this.cb = sinon.stub().callsFake(() => done())
         this.AuthenticationController.doPassportLogin(
           this.req,
           this.req.body.email,