Merge pull request #10394 from overleaf/ii-password-reset-and-strength-checking

[web] Password reset strength checking and UI updates

GitOrigin-RevId: 442a5c9e7e9d0a61d3ae649f3526bc3c02fd5704
This commit is contained in:
ilkin-overleaf 2022-12-06 12:22:20 +02:00 committed by Copybot
parent 887ac13f06
commit 2675cab92e
21 changed files with 309 additions and 159 deletions

View file

@ -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,
}

View file

@ -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)
}
)
}
)
},

View file

@ -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,
},
}

View file

@ -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'),

View file

@ -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
}

View file

@ -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

View file

@ -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

View file

@ -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')}…

View file

@ -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": ""
}

View file

@ -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',
})

View file

@ -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)

View file

@ -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 ? (
<FormGroup>
<Alert bsStyle="danger">{getUserFacingMessage(error)}</Alert>
<Alert bsStyle="danger">
{getErrorMessageKey(error) === 'password-must-be-strong' ? (
<>
<Trans
i18nKey="password_was_detected_on_a_public_list_of_known_compromised_passwords"
components={[
/* eslint-disable-next-line jsx-a11y/anchor-has-content, react/jsx-key */
<a
href="https://haveibeenpwned.com"
target="_blank"
rel="noreferrer noopener"
/>,
]}
/>
. {t('use_a_different_password')}
</>
) : (
getUserFacingMessage(error)
)}
</Alert>
</FormGroup>
) : null}
<Button

View file

@ -76,6 +76,10 @@ export class FetchError extends OError {
this.data = data
}
getErrorMessageKey() {
return this.data?.message?.key as string | undefined
}
getUserFacingMessage() {
const statusCode = this.response?.status
const defaultMessage = getErrorMessageForStatusCode(statusCode)
@ -210,6 +214,18 @@ async function parseResponseBody(response: Response) {
return {}
}
export function getErrorMessageKey(error: Error | null) {
if (!error) {
return undefined
}
if (error instanceof FetchError) {
return error.getErrorMessageKey()
}
return error.message
}
export function getUserFacingMessage(error: Error | null) {
if (!error) {
return undefined

View file

@ -10,3 +10,4 @@
@import 'app/import.less';
@import 'components/lists.less';
@import 'components/overbox.less';
@import 'components/container.less';

View file

@ -0,0 +1,3 @@
.container-custom-sm {
max-width: 400px;
}

View file

@ -530,3 +530,10 @@ input[type='checkbox'],
left: (@grid-gutter-width / 2);
}
}
.invalid-feedback {
display: flex;
align-items: baseline;
font-size: @font-size-small;
.text-emphasis-variant(@state-danger-text);
}

View file

@ -68,6 +68,7 @@
@import 'components/beta-badges.less';
@import 'components/divider.less';
@import 'components/input-switch.less';
@import 'components/container.less';
// Components w/ JavaScript
@import 'components/modals.less';

View file

@ -330,6 +330,24 @@
"password_change_passwords_do_not_match": "Passwords do not match",
"password_change_password_must_be_different": "The password you entered is the same as your current password. Please try a different password.",
"password_change_old_password_wrong": "Your old password is wrong",
"password_cant_be_the_same_as_current_one": "Password cant be the same as current one",
"password_was_detected_on_a_public_list_of_known_compromised_passwords": "This password was detected on a <0>public list of known compromised passwords</0>",
"use_a_different_password": "Use a different password",
"in_order_to_have_a_secure_account_make_sure_your_password": "In order to have a secure account, make sure your password:",
"is_longer_than_n_characters": "Is longer than __n__ characters",
"does_not_contain_or_significantly_match_your_email": "Does not contain or significantly match your email",
"is_not_a_common_or_obvious_password_as_defined_by_the_have_i_been_pwned_database": "Is not a common or obvious password, as defined by the <0>Have I Been Pwned</0> database",
"create_a_new_password_for_your_account": "Create a new password for your account",
"enter_your_email_address_below_and_we_will_send_you_a_link_to_reset_your_password": "Enter your email address below, and we will send you a link to reset your password",
"please_request_a_new_password_reset_email_and_follow_the_link": "Please request a new password reset email and follow the link",
"sorry_your_token_expired": "Sorry, your token expired",
"enter_your_email_address": "Enter your email address",
"check_your_email": "Check your email",
"back_to_log_in": "Back to log in",
"enter_your_new_password": "Enter your new password",
"password_updated": "Password updated",
"your_password_has_been_successfully_changed": "Your password has been successfully changed",
"log_in_now": "Log in now",
"github_for_link_shared_projects": "This project was accessed via link-sharing and wont be synchronised with your GitHub unless you are invited via e-mail by the project owner.",
"browsing_project_latest_for_pseudo_label": "Browsing your projects current state",
"history_label_project_current_state": "Current state",
@ -558,7 +576,7 @@
"tooltip_hide_filetree": "Click to hide the file-tree",
"tooltip_show_filetree": "Click to show the file-tree",
"cannot_verify_user_not_robot": "Sorry, we could not verify that you are not a robot. Please check that Google reCAPTCHA is not being blocked by an ad blocker or firewall.",
"recaptcha_conditions": "This site is protected by reCAPTCHA and the Google <1>Privacy Policy</1> and <2>Terms of Service</2> apply.",
"recaptcha_conditions": "The site is protected by reCAPTCHA and the Google <1>Privacy Policy</1> and <2>Terms of Service</2> apply.",
"uncompiled_changes": "Uncompiled Changes",
"code_check_failed": "Code check failed",
"code_check_failed_explanation": "Your code has errors that need to be fixed before the auto-compile can run",

View file

@ -34,6 +34,7 @@ async function getMetricFailure() {
}
let user, previous
async function resetPassword(password) {
await user.getCsrfToken()
await user.doRequest('POST', {
@ -51,13 +52,15 @@ async function resetPassword(password) {
await user.doRequest('GET', {
url: `/user/password/set?passwordResetToken=${token}&email=${user.email}`,
})
await user.doRequest('POST', {
const { response } = await user.doRequest('POST', {
url: '/user/password/set',
form: {
passwordResetToken: token,
password,
},
})
return response
}
describe('HaveIBeenPwnedApi', function () {
@ -187,7 +190,15 @@ describe('HaveIBeenPwnedApi', function () {
previous = await getMetricReUsed()
})
beforeEach('set password', async function () {
await resetPassword('aLeakedPassword42')
const response = await resetPassword('aLeakedPassword42')
expect(response.statusCode).to.equal(400)
expect(response.body).to.equal(
JSON.stringify({
message: {
key: 'password-must-be-strong',
},
})
)
await letPasswordCheckRunInBackground()
})
it('should track the weak password', async function () {
@ -208,7 +219,8 @@ describe('HaveIBeenPwnedApi', function () {
previous = await getMetricUnique()
})
beforeEach('set password', async function () {
await resetPassword('a-strong-new-password')
const response = await resetPassword('a-strong-new-password')
expect(response.statusCode).to.equal(200)
await letPasswordCheckRunInBackground()
})
it('should track the strong password', async function () {

View file

@ -157,25 +157,29 @@ class User {
ensureUserExists(callback) {
const filter = { email: this.email }
const options = { upsert: true, new: true, setDefaultsOnInsert: true }
UserModel.findOneAndUpdate(filter, {}, options, (error, user) => {
if (error != null) {
return callback(error)
}
this.setExtraAttributes(user)
AuthenticationManager.setUserPasswordInV2(user, this.password, error => {
AuthenticationManager.hashPassword(
this.password,
(error, hashedPassword) => {
if (error != null) {
if (error.name !== 'PasswordMustBeDifferentError') {
return callback(error)
}
return callback(error)
}
this.mongoUpdate({ $set: { emails: this.emails } }, error => {
if (error != null) {
return callback(error)
UserModel.findOneAndUpdate(
filter,
{ $set: { hashedPassword, emails: this.emails } },
options,
(error, user) => {
if (error != null) {
return callback(error)
}
this.setExtraAttributes(user)
callback(null, this.password)
}
callback(null, this.password)
})
})
})
)
}
)
}
setFeatures(features, callback) {

View file

@ -28,6 +28,7 @@ describe('AuthenticationManager', function () {
'../User/UserGetter': (this.UserGetter = {}),
'./AuthenticationErrors': AuthenticationErrors,
'./HaveIBeenPwned': {
checkPasswordForReuse: sinon.stub().yields(null, false),
checkPasswordForReuseInBackground: sinon.stub(),
},
'../User/UserAuditLogHandler': (this.UserAuditLogHandler = {