diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 1b208ed3f8..17f5c5bc67 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/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('', ' (https://haveibeenpwned.com)') + .replace('<1>', '') .replace( - '', - ' (https://haveibeenpwned.com)' - )}. ${req.i18n - .translate('please_reset_your_password_to_login') - .replace('<0>', '') - .replace( - '', + '', ` (${Settings.siteUrl}/user/password/reset)` )}.` return done(null, false, { diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 7d8aa6bfce..5e7c1e105c 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/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 - } - 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) - } - ) - } - } - AuthenticationManager.checkRounds( - user, - user.hashedPassword, - password, - function (err) { - if (err) { - return callback(err) - } - callback(null, user) - } - ) - } + authenticate(query, password, auditLog, { skipHIBPCheck = false }, callback) { + const checkUserPassword = callback => { + if (skipHIBPCheck) { + AuthenticationManager._checkUserPasswordWithOutHIBPCheck( + query, + password, + callback ) + } else { + AuthenticationManager._checkUserPassword(query, password, callback) } - ) + } + 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' + ) + } + callback(null, null) + } + ) + } + } + AuthenticationManager.checkRounds( + user, + user.hashedPassword, + password, + function (err) { + if (err) { + return callback(err) + } + callback(null, user) + } + ) + } + ) + }) }, validateEmail(email) { diff --git a/services/web/app/src/Features/Captcha/CaptchaMiddleware.js b/services/web/app/src/Features/Captcha/CaptchaMiddleware.js index 1ee038e687..f796749f85 100644 --- a/services/web/app/src/Features/Captcha/CaptchaMiddleware.js +++ b/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. diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index d557c85c84..12defe00cb 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/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') diff --git a/services/web/app/views/user/login.pug b/services/web/app/views/user/login.pug index f54beb0a53..f390bdfe49 100644 --- a/services/web/app/views/user/login.pug +++ b/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', diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 0a5a9229a7..07b8b07e42 100644 --- a/services/web/locales/en.json +++ b/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. Please check it’s correct and try again. Alternatively, try logging in from a device you’ve previously used or <1>reset your password", "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 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", diff --git a/services/web/test/acceptance/src/AuthenticationTests.js b/services/web/test/acceptance/src/AuthenticationTests.js index cd79fb2614..ba56440708 100644 --- a/services/web/test/acceptance/src/AuthenticationTests.js +++ b/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') }) diff --git a/services/web/test/acceptance/src/CaptchaTests.js b/services/web/test/acceptance/src/CaptchaTests.js index cf1ab65ac4..0deb554e7e 100644 --- a/services/web/test/acceptance/src/CaptchaTests.js +++ b/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') + }) + }) }) }) diff --git a/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js index e5ccb93aaa..77d9eb7a53 100644 --- a/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js +++ b/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).`, }, }) } diff --git a/services/web/test/acceptance/src/UserHelperTests.js b/services/web/test/acceptance/src/UserHelperTests.js index f3aee82dab..cb5037044e 100644 --- a/services/web/test/acceptance/src/UserHelperTests.js +++ b/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 }) diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index 5c3b1613f9..b028a0784b 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/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') diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 9b9d89cf4a..0eb62707a8 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/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 ) })