diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index fb359c8bb1..1b208ed3f8 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -23,7 +23,10 @@ const AnalyticsRegistrationSourceHelper = require('../Analytics/AnalyticsRegistr const { acceptsJson, } = require('../../infrastructure/RequestContentTypeDetection') -const { ParallelLoginError } = require('./AuthenticationErrors') +const { + ParallelLoginError, + PasswordReusedError, +} = require('./AuthenticationErrors') const { hasAdminAccess } = require('../Helpers/AdminAuthorizationHelper') const Modules = require('../../infrastructure/Modules') const { expressify } = require('@overleaf/promise-utils') @@ -218,6 +221,28 @@ const AuthenticationController = { if (error != null) { if (error instanceof ParallelLoginError) { return done(null, false, { status: 429 }) + } else if (error instanceof PasswordReusedError) { + const text = `${req.i18n + .translate( + 'password_was_detected_on_a_public_list_of_known_compromised_passwords' + ) + .replace('<0>', '') + .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, { + status: 400, + type: 'error', + key: 'password-compromised', + text, + }) } return done(error) } diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index fdde74037e..7d8aa6bfce 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -60,6 +60,19 @@ function _metricsForSuccessfulPasswordMatch(password) { const AuthenticationManager = { _checkUserPassword(query, password, callback) { + HaveIBeenPwned.checkPasswordForReuse(password, (err, isPasswordReused) => { + if (err) { + logger.err({ err }, 'cannot check password for re-use') + } + if (isPasswordReused) return callback(new PasswordReusedError()) + AuthenticationManager._checkUserPasswordWithOutHIBPCheck( + query, + password, + callback + ) + }) + }, + _checkUserPasswordWithOutHIBPCheck(query, password, callback) { // Using Mongoose for legacy reasons here. The returned User instance // gets serialized into the session and there may be subtle differences // between the user returned by Mongoose vs mongodb (such as default values) @@ -179,7 +192,6 @@ const AuthenticationManager = { return callback(err) } callback(null, user) - HaveIBeenPwned.checkPasswordForReuseInBackground(password) } ) } @@ -333,7 +345,7 @@ const AuthenticationManager = { } // check if we can log in with this password. In which case we should reject it, // because it is the same as the existing password. - AuthenticationManager._checkUserPassword( + AuthenticationManager._checkUserPasswordWithOutHIBPCheck( { _id: user._id }, password, (err, _user, match) => { diff --git a/services/web/app/views/user/login.pug b/services/web/app/views/user/login.pug index af0194b7af..f54beb0a53 100644 --- a/services/web/app/views/user/login.pug +++ b/services/web/app/views/user/login.pug @@ -15,6 +15,10 @@ block content | !{translate('email_or_password_wrong_try_again_or_reset', {}, [{ name: 'a', attrs: { href: '/user/password/reset', 'aria-describedby': 'resetPasswordDescription' } }])} 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'}}])}. .form-group input.form-control( type='email', diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 3461916789..0a5a9229a7 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -1338,6 +1338,7 @@ "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/HaveIBeenPwnedApiTests.js b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js index 678fc68673..e5ccb93aaa 100644 --- a/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js +++ b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js @@ -4,17 +4,12 @@ const User = require('./helpers/User').promises const MockHaveIBeenPwnedApiClass = require('./mocks/MockHaveIBeenPwnedApi') const { db } = require('../../../app/src/infrastructure/mongodb') const { getMetric } = require('./helpers/metrics').promises -const sleep = require('util').promisify(setTimeout) let MockHaveIBeenPwnedApi before(function () { MockHaveIBeenPwnedApi = MockHaveIBeenPwnedApiClass.instance() }) -async function letPasswordCheckRunInBackground() { - await sleep(200) -} - async function getMetricReUsed() { return getMetric( line => line.includes('password_re_use') && line.includes('re-used') @@ -83,14 +78,18 @@ describe('HaveIBeenPwnedApi', function () { }) beforeEach('create the user', async function () { await user.ensureUserExists() - await letPasswordCheckRunInBackground() }) beforeEach('fetch previous count', async function () { previous = await getMetricReUsed() }) beforeEach('login', async function () { - await user.loginNoUpdate() - await letPasswordCheckRunInBackground() + try { + await user.loginNoUpdate() + } catch (e) { + expect(e.message).to.include('password-compromised') + return + } + expect.fail('should have failed login with weak password') }) it('should track the weak password', async function () { const after = await getMetricReUsed() @@ -105,14 +104,12 @@ describe('HaveIBeenPwnedApi', function () { }) beforeEach('create the user', async function () { await user.ensureUserExists() - await letPasswordCheckRunInBackground() }) beforeEach('fetch previous count', async function () { previous = await getMetricUnique() }) beforeEach('login', async function () { await user.loginNoUpdate() - await letPasswordCheckRunInBackground() }) it('should track the strong password', async function () { const after = await getMetricUnique() @@ -127,14 +124,12 @@ describe('HaveIBeenPwnedApi', function () { }) beforeEach('create the user', async function () { await user.ensureUserExists() - await letPasswordCheckRunInBackground() }) beforeEach('fetch previous count', async function () { previous = await getMetricFailure() }) beforeEach('login', async function () { await user.loginNoUpdate() - await letPasswordCheckRunInBackground() }) it('should track the failure to collect a score', async function () { const after = await getMetricFailure() @@ -152,7 +147,6 @@ describe('HaveIBeenPwnedApi', function () { }) beforeEach('create the user', async function () { await user.ensureUserExists() - await letPasswordCheckRunInBackground() }) beforeEach('fetch previous counts', async function () { previous = { @@ -166,16 +160,19 @@ describe('HaveIBeenPwnedApi', function () { await user.loginWithEmailPassword(user.email, 'aLeakedPassword42') expect.fail('expected the login request to fail') } catch (err) { - expect(err).to.match(/login failed: status=401/) + expect(err).to.match(/login failed: status=400/) expect(err.info.body).to.deep.equal({ - message: { type: 'error', key: 'invalid-password-retry-or-reset' }, + 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.`, + }, }) } - await letPasswordCheckRunInBackground() }) - it('should not increment any counter', async function () { + it('should increment the counter', async function () { expect(previous).to.deep.equal({ - reUsed: await getMetricReUsed(), + reUsed: (await getMetricReUsed()) - 1, unique: await getMetricUnique(), failure: await getMetricFailure(), }) @@ -192,7 +189,6 @@ describe('HaveIBeenPwnedApi', function () { }) beforeEach('create the user', async function () { await user.ensureUserExists() - await letPasswordCheckRunInBackground() }) beforeEach('fetch previous count', async function () { previous = await getMetricReUsed() @@ -207,7 +203,6 @@ describe('HaveIBeenPwnedApi', function () { }, }) ) - await letPasswordCheckRunInBackground() }) it('should track the weak password', async function () { const after = await getMetricReUsed() @@ -221,7 +216,6 @@ describe('HaveIBeenPwnedApi', function () { }) beforeEach('create the user', async function () { await user.ensureUserExists() - await letPasswordCheckRunInBackground() }) beforeEach('fetch previous count', async function () { previous = await getMetricUnique() @@ -229,7 +223,6 @@ describe('HaveIBeenPwnedApi', function () { beforeEach('set password', async function () { const response = await resetPassword('a-strong-new-password') expect(response.statusCode).to.equal(200) - await letPasswordCheckRunInBackground() }) it('should track the strong password', async function () { const after = await getMetricUnique()