From 9daacea6cbea7005fc079ac5510bf9cdda8d807c Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 4 Mar 2024 16:38:23 +0000 Subject: [PATCH] Merge pull request #17409 from overleaf/jpa-check-before-hibp [web] check user password before HIBP check GitOrigin-RevId: 7c1bdc220fb9369733a1ff3bf26bed8cacc8e8d4 --- .../Authentication/AuthenticationManager.js | 160 +++++++++--------- services/web/locales/en.json | 2 +- .../acceptance/src/HaveIBeenPwnedApiTests.js | 26 +-- 3 files changed, 93 insertions(+), 95 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 5e7c1e105c..d58697a67f 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -60,19 +60,6 @@ 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) @@ -132,75 +119,84 @@ const AuthenticationManager = { }, 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) - } - ) + 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) + } + if (skipHIBPCheck) { + callback(null, user) + HaveIBeenPwned.checkPasswordForReuseInBackground(password) + return + } + HaveIBeenPwned.checkPasswordForReuse( + password, + (err, isPasswordReused) => { + if (err) { + logger.err({ err }, 'cannot check password for re-use') + } + if (isPasswordReused) { + return callback(new PasswordReusedError()) + } + callback(null, user) + } + ) + } + ) + } + ) + } + ) }, validateEmail(email) { @@ -348,7 +344,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._checkUserPasswordWithOutHIBPCheck( + AuthenticationManager._checkUserPassword( { _id: user._id }, password, (err, _user, match) => { diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 07b8b07e42..2d5a1828dc 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -1280,7 +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_compromised_try_again_or_use_known_device_or_reset": "The password you’ve entered is on a <0>public list of compromised passwords. Please 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.", diff --git a/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js index 77d9eb7a53..2fc477fa1d 100644 --- a/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js +++ b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js @@ -85,11 +85,17 @@ describe('HaveIBeenPwnedApi', function () { beforeEach('login', async function () { try { await user.loginNoUpdate() - } catch (e) { - expect(e.message).to.include('password-compromised') - return + expect.fail('should have failed login with weak password') + } catch (err) { + expect(err).to.match(/login failed: status=400/) + expect(err.info.body).to.deep.equal({ + message: { + type: 'error', + key: 'password-compromised', + text: `The password you’ve entered is on a public list of compromised passwords (https://haveibeenpwned.com). Please try logging in from a device you’ve previously used or reset your password (${Settings.siteUrl}/user/password/reset).`, + }, + }) } - expect.fail('should have failed login with weak password') }) it('should track the weak password', async function () { const after = await getMetricReUsed() @@ -160,19 +166,15 @@ 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=400/) + expect(err).to.match(/login failed: status=401/) expect(err.info.body).to.deep.equal({ - message: { - type: 'error', - key: 'password-compromised', - 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).`, - }, + message: { type: 'error', key: 'invalid-password-retry-or-reset' }, }) } }) - it('should increment the counter', async function () { + it('should not increment the counter', async function () { expect(previous).to.deep.equal({ - reUsed: (await getMetricReUsed()) - 1, + reUsed: await getMetricReUsed(), unique: await getMetricUnique(), failure: await getMetricFailure(), })