From 1e36db524f9dfd1bf6afc26d9e997b6c8b92301a Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Tue, 30 Jul 2024 14:59:51 +0200 Subject: [PATCH] [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 --- .../AuthenticationController.js | 156 +++++++++--------- .../Authentication/AuthenticationErrors.js | 43 +++++ .../AuthenticationControllerTests.js | 143 +++++++++------- 3 files changed, 202 insertions(+), 140 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 2a0cd5aa30..fef22e7828 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/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,93 +195,88 @@ const AuthenticationController = { ) }, - doPassportLogin(req, username, password, done) { + async doPassportLogin(req, username, password, done) { + let user, info + try { + ;({ user, info } = await AuthenticationController._doPassportLogin( + req, + username, + password + )) + } catch (error) { + return done(error) + } + 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() - Modules.hooks.fire( - 'preDoPassportLogin', - req, - email, - function (err, infoList) { - if (err) { - return done(err) - } - const info = infoList.find(i => i != null) - if (info != null) { - return done(null, false, info) - } - const { fromKnownDevice } = AuthenticationController.getAuditInfo(req) - const auditLog = { - ipAddress: req.ip, - info: { method: 'Password login', fromKnownDevice }, - } - AuthenticationManager.authenticate( + const { fromKnownDevice } = AuthenticationController.getAuditInfo(req) + const auditLog = { + ipAddress: req.ip, + info: { method: 'Password login', fromKnownDevice }, + } + + 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('', ' (https://haveibeenpwned.com/passwords)') - .replace('<1>', '') - .replace( - '', - ` (${Settings.siteUrl}/user/password/reset)` - )}.` - return done(null, false, { - status: 400, - type: 'error', - key: 'password-compromised', - text, - }) - } - return done(error) - } - 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) { - if ( - isPasswordReused && - AuthenticationController.getRedirectFromSession(req) == null - ) { - AuthenticationController.setRedirectInSession( - req, - '/compromised-password' - ) - } - - // async actions - done(null, user) - } else { - AuthenticationController._recordFailedLogin() - logger.debug({ email }, 'failed log in') - done(null, false, { - type: 'error', - key: 'invalid-password-retry-or-reset', - status: 401, - }) - } } + )) + } catch (error) { + return { + user: false, + info: handleAuthenticateErrors(error, req), + } + } + + if (user && AuthenticationController.captchaRequiredForLogin(req, user)) { + return { + 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 && + AuthenticationController.getRedirectFromSession(req) == null + ) { + AuthenticationController.setRedirectInSession( + req, + '/compromised-password' ) } - ) + + // async actions + return { user, info: undefined } + } else { + AuthenticationController._recordFailedLogin() + logger.debug({ email }, 'failed log in') + return { + user: false, + info: { + type: 'error', + key: 'invalid-password-retry-or-reset', + status: 401, + }, + } + } }, captchaRequiredForLogin(req, user) { diff --git a/services/web/app/src/Features/Authentication/AuthenticationErrors.js b/services/web/app/src/Features/Authentication/AuthenticationErrors.js index 2773657a8c..c5dc8bac33 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationErrors.js +++ b/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('', ' (https://haveibeenpwned.com/passwords)') + .replace('<1>', '') + .replace('', ` (${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, } diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 2390981a44..474860a0f5 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/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 - .stub() - .yields(null, [null, { redir: '/somewhere' }, null]) + this.LoginRateLimiter.promises.processLoginRequest.resolves(true) + this.errorsWith = (error, done) => { + this.AuthenticationManager.promises.authenticate = sinon + .stub() + .rejects(error) + this.AuthenticationController.doPassportLogin( + this.req, + this.req.body.email, + this.req.body.password, + this.cb.callsFake(() => done()) + ) + } }) - - 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.callCount.should.equal(1) - this.cb - .calledWith(null, false, { redir: '/somewhere' }) - .should.equal(true) - this.LoginRateLimiter.processLoginRequest.callCount.should.equal(0) - done() + describe('with "password is too long"', function () { + beforeEach(function (done) { + this.errorsWith(new Error('password is too long'), 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.AuthenticationManager.authenticate = sinon + this.LoginRateLimiter.promises.processLoginRequest.resolves(true) + 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) - }) - }) - - 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 }) + this.cb.calledWith(undefined, this.user).should.equal(true) }) }) @@ -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 () { - this.LoginRateLimiter.processLoginRequest.yields(null, true) - this.AuthenticationManager.authenticate = sinon + beforeEach(function (done) { + this.LoginRateLimiter.promises.processLoginRequest.resolves(true) + this.AuthenticationManager.promises.authenticate = sinon .stub() - .yields(null, null) - this.cb = sinon.stub() + .resolves({ user: null }) + this.cb = sinon.stub().callsFake(() => done()) this.AuthenticationController.doPassportLogin( this.req, this.req.body.email,