From 2675cab92ef5f9ede4a9ffdc08a2c2b2b3ff5266 Mon Sep 17 00:00:00 2001 From: ilkin-overleaf <100852799+ilkin-overleaf@users.noreply.github.com> Date: Tue, 6 Dec 2022 12:22:20 +0200 Subject: [PATCH] Merge pull request #10394 from overleaf/ii-password-reset-and-strength-checking [web] Password reset strength checking and UI updates GitOrigin-RevId: 442a5c9e7e9d0a61d3ae649f3526bc3c02fd5704 --- .../Authentication/AuthenticationErrors.js | 2 + .../Authentication/AuthenticationManager.js | 19 ++- .../Features/Authentication/HaveIBeenPwned.js | 45 ++++-- .../PasswordReset/PasswordResetController.js | 6 + .../app/src/Features/User/UserController.js | 6 + .../web/app/views/_mixins/formMessages.pug | 9 ++ services/web/app/views/user/passwordReset.pug | 108 +++++++-------- services/web/app/views/user/setPassword.pug | 130 ++++++++++-------- .../web/frontend/extracted-translations.json | 4 +- .../js/features/form-helpers/hydrate-form.js | 8 +- .../features/form-helpers/input-validator.js | 4 +- .../settings/components/password-section.tsx | 24 +++- .../frontend/js/infrastructure/fetch-json.ts | 16 +++ .../stylesheets/_ol_style_includes.less | 1 + .../stylesheets/components/container.less | 3 + .../stylesheets/components/forms.less | 7 + .../web/frontend/stylesheets/main-style.less | 1 + services/web/locales/en.json | 20 ++- .../acceptance/src/HaveIBeenPwnedApiTests.js | 18 ++- .../web/test/acceptance/src/helpers/User.js | 36 ++--- .../AuthenticationManagerTests.js | 1 + 21 files changed, 309 insertions(+), 159 deletions(-) create mode 100644 services/web/frontend/stylesheets/components/container.less diff --git a/services/web/app/src/Features/Authentication/AuthenticationErrors.js b/services/web/app/src/Features/Authentication/AuthenticationErrors.js index c7b4caf470..2773657a8c 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationErrors.js +++ b/services/web/app/src/Features/Authentication/AuthenticationErrors.js @@ -4,10 +4,12 @@ class InvalidEmailError extends Errors.BackwardCompatibleError {} class InvalidPasswordError extends Errors.BackwardCompatibleError {} class ParallelLoginError extends Errors.BackwardCompatibleError {} class PasswordMustBeDifferentError extends Errors.BackwardCompatibleError {} +class PasswordReusedError extends Errors.BackwardCompatibleError {} module.exports = { InvalidEmailError, InvalidPasswordError, ParallelLoginError, PasswordMustBeDifferentError, + PasswordReusedError, } diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 1c1b17ee80..75b7078186 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -8,6 +8,7 @@ const { InvalidPasswordError, ParallelLoginError, PasswordMustBeDifferentError, + PasswordReusedError, } = require('./AuthenticationErrors') const util = require('util') const HaveIBeenPwned = require('./HaveIBeenPwned') @@ -256,8 +257,22 @@ const AuthenticationManager = { if (match) { return callback(new PasswordMustBeDifferentError()) } - this._setUserPasswordInMongo(user, password, callback) - HaveIBeenPwned.checkPasswordForReuseInBackground(password) + + HaveIBeenPwned.checkPasswordForReuse( + password, + (error, isPasswordReused) => { + if (error) { + logger.err({ error }, 'cannot check password for re-use') + } + + if (!error && isPasswordReused) { + return callback(new PasswordReusedError()) + } + + // password is strong enough or the validation with the service did not happen + this._setUserPasswordInMongo(user, password, callback) + } + ) } ) }, diff --git a/services/web/app/src/Features/Authentication/HaveIBeenPwned.js b/services/web/app/src/Features/Authentication/HaveIBeenPwned.js index c459cd82a2..90d5604512 100644 --- a/services/web/app/src/Features/Authentication/HaveIBeenPwned.js +++ b/services/web/app/src/Features/Authentication/HaveIBeenPwned.js @@ -5,6 +5,7 @@ happy path or via an error (message or attributes). */ +const { callbackify } = require('util') const fetch = require('node-fetch') const crypto = require('crypto') const Settings = require('@overleaf/settings') @@ -89,29 +90,43 @@ async function isPasswordReused(password) { return score > 0 } -function checkPasswordForReuseInBackground(password) { +async function checkPasswordForReuse(password) { if (!Settings.apis.haveIBeenPwned.enabled) { return } - isPasswordReused(password) - .then(isReused => { - Metrics.inc('password_re_use', { - status: isReused ? 're-used' : 'unique', - }) - }) - .catch(err => { - // Make sure we do not leak any password details. - if (!CODED_ERROR_MESSAGES.includes(err.message)) { - err = new Error('hidden message') - } - err = new Error(err.message) + try { + const isReused = await isPasswordReused(password) - logger.err({ err }, 'cannot check password for re-use') - Metrics.inc('password_re_use', { status: 'failure' }) + Metrics.inc('password_re_use', { + status: isReused ? 're-used' : 'unique', }) + + return isReused + } catch (err) { + let error = err + // Make sure we do not leak any password details. + if (!CODED_ERROR_MESSAGES.includes(err.message)) { + error = new Error('hidden message') + } + error = new Error(error.message) + + Metrics.inc('password_re_use', { status: 'failure' }) + + throw error + } +} + +function checkPasswordForReuseInBackground(password) { + checkPasswordForReuse(password).catch(error => { + logger.err({ error }, 'cannot check password for re-use') + }) } module.exports = { + checkPasswordForReuse: callbackify(checkPasswordForReuse), checkPasswordForReuseInBackground, + promises: { + checkPasswordForReuse, + }, } diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index 02fc6c0488..f1a02b9209 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetController.js @@ -84,6 +84,12 @@ async function setNewUserPassword(req, res, next) { key: 'password-must-be-different', }, }) + } else if (error.name === 'PasswordReusedError') { + return res.status(400).json({ + message: { + key: 'password-must-be-strong', + }, + }) } else { return res.status(500).json({ message: req.i18n.translate('error_performing_request'), diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 91e3cc1cd7..072163531a 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -103,6 +103,12 @@ async function changePassword(req, res, next) { res, req.i18n.translate('password_change_password_must_be_different') ) + } else if (error.name === 'PasswordReusedError') { + return res.status(400).json({ + message: { + key: 'password-must-be-strong', + }, + }) } else { throw error } diff --git a/services/web/app/views/_mixins/formMessages.pug b/services/web/app/views/_mixins/formMessages.pug index feba09920b..01845a2f59 100644 --- a/services/web/app/views/_mixins/formMessages.pug +++ b/services/web/app/views/_mixins/formMessages.pug @@ -29,3 +29,12 @@ mixin customFormMessage(key, kind) aria-live="polite" ) block + +mixin customValidationMessage(key) + div.invalid-feedback.mt-2( + hidden, + data-ol-custom-form-message=key + ) + i.fa.fa-fw.fa-warning.me-1(aria-hidden="true") + div + block diff --git a/services/web/app/views/user/passwordReset.pug b/services/web/app/views/user/passwordReset.pug index 1f049a6a62..61516aea1d 100644 --- a/services/web/app/views/user/passwordReset.pug +++ b/services/web/app/views/user/passwordReset.pug @@ -15,60 +15,58 @@ block content ) main.content.content-alt#main-content - .container - .row - .col-md-6.col-md-offset-3.col-lg-4.col-lg-offset-4 - .card - .page-header - h1 #{translate("password_reset")} - .messageArea - form( - data-ol-async-form - name="passwordResetForm" - action="/user/password/reset", - method="POST", - captcha=(showCaptcha ? '' : false), - captcha-action-name=(showCaptcha ? "passwordReset" : false), - ) - div(data-ol-not-sent) - +formMessages() - if error - div.alert.alert-danger( - role="alert" - aria-live="assertive" - ) - | #{translate(error)} + .container-custom-sm.mx-auto + .card + form( + data-ol-async-form + name="passwordResetForm" + action="/user/password/reset", + method="POST", + captcha=(showCaptcha ? '' : false), + captcha-action-name=(showCaptcha ? "passwordReset" : false), + ) + if error === 'password_reset_token_expired' + h3.mt-0.mb-2 #{translate("sorry_your_token_expired")} + p #{translate('please_request_a_new_password_reset_email_and_follow_the_link')}. + else + h3.mt-0.mb-2(data-ol-not-sent) #{translate("password_reset")} + h3.mt-0.mb-2(hidden data-ol-sent) #{translate("check_your_email")} + p(data-ol-not-sent) #{translate("enter_your_email_address_below_and_we_will_send_you_a_link_to_reset_your_password")}. - input(type="hidden", name="_csrf", value=csrfToken) - .form-group - label(for='email') #{translate("please_enter_email")} - input.form-control#email( - aria-label="email" - type='email', - name='email', - placeholder='email@example.com', - required, - autocomplete="username", - autofocus - ) - .actions - button.btn.btn-primary( - type='submit', - data-ol-disabled-inflight, - aria-label=translate('request_password_reset_to_reconfirm') - ) - span(data-ol-inflight="idle") - | #{translate("request_password_reset")} - span(hidden data-ol-inflight="pending") - | #{translate("requesting_password_reset")}… - div(hidden data-ol-sent) - div.alert.alert-success( - role="alert" - aria-live="polite" - ) - span #{translate('password_reset_email_sent')} + div(data-ol-not-sent) + +formMessages() + if error && error !== 'password_reset_token_expired' + div.alert.alert-danger.mb-2( + role="alert" + aria-live="assertive" + ) + | #{translate(error)} - .row - .col-md-6.col-md-offset-3.col-lg-4.col-lg-offset-4 - if showCaptcha - +recaptchaConditions + input(type="hidden", name="_csrf", value=csrfToken) + .form-group.mb-3 + label(for='email') #{translate("email")} + input.form-control#email( + aria-label="email" + type='email', + name='email', + placeholder=translate("enter_your_email_address"), + required, + autocomplete="username", + autofocus + ) + .actions + button.btn.btn-primary.w-100( + type='submit', + data-ol-disabled-inflight, + aria-label=translate('request_password_reset_to_reconfirm') + ) + span(data-ol-inflight="idle") + | #{translate("request_password_reset")} + span(hidden data-ol-inflight="pending") + | #{translate("requesting_password_reset")}… + div(hidden data-ol-sent) + p.mb-4 #{translate('password_reset_email_sent')} + a(href="/login") #{translate('back_to_log_in')} + + if showCaptcha + +recaptchaConditions diff --git a/services/web/app/views/user/setPassword.pug b/services/web/app/views/user/setPassword.pug index be8081e3c0..859cb656d6 100644 --- a/services/web/app/views/user/setPassword.pug +++ b/services/web/app/views/user/setPassword.pug @@ -2,69 +2,77 @@ extends ../layout-marketing block content main.content.content-alt#main-content - .container - .row - .col-md-6.col-md-offset-3.col-lg-4.col-lg-offset-4 - .card - .page-header - h1 #{translate("reset_your_password")} - form( - data-ol-async-form, - name="passwordResetForm", - action="/user/password/set", - method="POST", - data-ol-hide-on-error="token-expired" - ) - div.alert.alert-success( - hidden - role="alert" - aria-live="assertive" - data-ol-sent + .container-custom-sm.mx-auto + .card + form( + data-ol-async-form, + name="passwordResetForm", + action="/user/password/set", + method="POST", + data-ol-hide-on-error="token-expired" + ) + div( + hidden + data-ol-sent + ) + h3.mt-0.mb-2 #{translate("password_updated")} + p.mb-4 #{translate("your_password_has_been_successfully_changed")}. + a(href='/login') #{translate("log_in_now")} + + div(data-ol-not-sent) + h3.mt-0.mb-2 #{translate("reset_your_password")} + p(data-ol-hide-on-error-message="token-expired") #{translate("create_a_new_password_for_your_account")}. + +formMessages() + + +customFormMessage('token-expired', 'danger') + | #{translate('password_reset_token_expired')} + br + a(href="/user/password/reset") + | #{translate('request_new_password_reset_email')} + + input(type="hidden", name="_csrf", value=csrfToken) + input(type="hidden", name="email", value=email) + + .form-group + label(for='passwordField', data-ol-hide-on-error-message="token-expired") #{translate("new_password")} + input.form-control#passwordField( + type='password', + name='password', + placeholder=translate("enter_your_new_password"), + autocomplete="new-password", + autofocus, + required, + minlength=settings.passwordStrengthOptions.length.min ) - | #{translate("password_has_been_reset")}. - br - a(href='/login') #{translate("login_here")} - div(data-ol-not-sent) - +formMessages() + +customValidationMessage('invalid-password') + | #{translate('invalid_password')} - +customFormMessage('token-expired', 'danger') - | #{translate('password_reset_token_expired')} - br - a(href="/user/password/reset") - | #{translate('request_new_password_reset_email')} + +customValidationMessage('password-must-be-different') + | #{translate('password_cant_be_the_same_as_current_one')}. - +customFormMessage('invalid-password', 'danger') - | #{translate('invalid_password')} + +customValidationMessage('password-must-be-strong') + | !{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('use_a_different_password')}. - +customFormMessage('password-must-be-different', 'danger') - | #{translate('password_change_password_must_be_different')} - - input(type="hidden", name="_csrf", value=csrfToken) - input(type="hidden", name="email", value=email) - - .form-group - input.form-control#passwordField( - type='password', - name='password', - placeholder='new password', - autocomplete="new-password", - autofocus, - required, - minlength=settings.passwordStrengthOptions.length.min - ) - input( - type="hidden", - name="passwordResetToken", - value=passwordResetToken - ) - .actions - button.btn.btn-primary( - type='submit', - data-ol-disabled-inflight - aria-label=translate('set_new_password') - ) - span(data-ol-inflight="idle") - | #{translate('set_new_password')} - span(hidden data-ol-inflight="pending") - | #{translate('set_new_password')}… + input( + type="hidden", + name="passwordResetToken", + value=passwordResetToken + ) + div(data-ol-hide-on-error-message="token-expired") + div #{translate('in_order_to_have_a_secure_account_make_sure_your_password')} + ul.mb-4.ps-4 + li #{translate('is_longer_than_n_characters', {n: 6})} + li #{translate('does_not_contain_or_significantly_match_your_email')} + li !{translate('is_not_a_common_or_obvious_password_as_defined_by_the_have_i_been_pwned_database', {}, [{name: 'a', attrs: {href: 'https://haveibeenpwned.com', rel: 'noopener noreferrer', target: '_blank'}}])} + .actions + button.btn.btn-primary.w-100( + type='submit', + data-ol-disabled-inflight + aria-label=translate('set_new_password') + ) + span(data-ol-inflight="idle") + | #{translate('set_new_password')} + span(hidden data-ol-inflight="pending") + | #{translate('set_new_password')}… diff --git a/services/web/frontend/extracted-translations.json b/services/web/frontend/extracted-translations.json index ed63a27ad7..cc30096efd 100644 --- a/services/web/frontend/extracted-translations.json +++ b/services/web/frontend/extracted-translations.json @@ -748,5 +748,7 @@ "zotero_reference_loading_error": "", "zotero_reference_loading_error_expired": "", "zotero_reference_loading_error_forbidden": "", - "zotero_sync_description": "" + "zotero_sync_description": "", + "use_a_different_password": "", + "password_was_detected_on_a_public_list_of_known_compromised_passwords": "" } diff --git a/services/web/frontend/js/features/form-helpers/hydrate-form.js b/services/web/frontend/js/features/form-helpers/hydrate-form.js index 5f3074f47c..e423cae576 100644 --- a/services/web/frontend/js/features/form-helpers/hydrate-form.js +++ b/services/web/frontend/js/features/form-helpers/hydrate-form.js @@ -153,11 +153,17 @@ function showMessages(formEl, messageBag) { ) { hideFormElements(formEl) } + // Hide any elements with specific `data-ol-hide-on-error-message` message + document + .querySelectorAll(`[data-ol-hide-on-error-message="${message.key}"]`) + .forEach(el => { + el.hidden = true + }) return } const messageEl = document.createElement('div') - messageEl.className = classNames('alert', { + messageEl.className = classNames('alert mb-2', { 'alert-danger': message.type === 'error', 'alert-success': message.type !== 'error', }) diff --git a/services/web/frontend/js/features/form-helpers/input-validator.js b/services/web/frontend/js/features/form-helpers/input-validator.js index 23bdf2112c..411c6c0e83 100644 --- a/services/web/frontend/js/features/form-helpers/input-validator.js +++ b/services/web/frontend/js/features/form-helpers/input-validator.js @@ -1,8 +1,8 @@ export default function inputValidator(inputEl) { - const messageEl = document.createElement('span') + const messageEl = document.createElement('div') messageEl.className = inputEl.getAttribute('data-ol-validation-message-classes') || - 'small text-danger' + 'small text-danger mt-2' messageEl.hidden = true inputEl.insertAdjacentElement('afterend', messageEl) diff --git a/services/web/frontend/js/features/settings/components/password-section.tsx b/services/web/frontend/js/features/settings/components/password-section.tsx index 9cb1b12192..9cc846d561 100644 --- a/services/web/frontend/js/features/settings/components/password-section.tsx +++ b/services/web/frontend/js/features/settings/components/password-section.tsx @@ -6,9 +6,10 @@ import { FormControl, FormGroup, } from 'react-bootstrap' -import { useTranslation } from 'react-i18next' +import { Trans, useTranslation } from 'react-i18next' import { getUserFacingMessage, + getErrorMessageKey, postJSON, } from '../../../infrastructure/fetch-json' import getMeta from '../../../utils/meta' @@ -147,7 +148,26 @@ function PasswordForm() { ) : null} {isError ? ( - {getUserFacingMessage(error)} + + {getErrorMessageKey(error) === 'password-must-be-strong' ? ( + <> + , + ]} + /> + . {t('use_a_different_password')} + + ) : ( + getUserFacingMessage(error) + )} + ) : null}