UX: Streamline reset password page ()

This commit includes various UX improvements to the reset
password page:

* Introduce a `hide-application-header-buttons` helper to do the following:
  * Hide Sign Up and Log In buttons, they are not necessary on this flow
  * Hide the sidebar, it is a distraction on this flow
* Improve messaging when a 2FA confirmation is required first
* Improve display of server-side ActiveRecord model validation errors
  in password form, e.g. instead of "is the same as your current password"
  we do "The password is the same as your current password"
* Move password tip to next line below input and move caps lock hint
  inline with Show/Hide password toggle
* Add system specs for 2FA flow on reset password page
* Fixes a computed property conflict issue on the password reset
   page when toggling 2FA methods
This commit is contained in:
Martin Brennan 2024-06-05 15:22:59 +10:00 committed by GitHub
parent aa88b07640
commit 0434112aa7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
17 changed files with 256 additions and 84 deletions

@ -1,14 +1,25 @@
import Component from "@glimmer/component";
import { service } from "@ember/service";
import { and, not } from "truth-helpers";
import DButton from "discourse/components/d-button";
export default class AuthButtons extends Component {
@service header;
get showSignupButton() {
return (
this.args.canSignUp &&
!this.header.headerButtonsHidden.includes("signup") &&
!this.header.topic
);
}
get showLoginButton() {
return !this.header.headerButtonsHidden.includes("login");
}
<template>
<span class="auth-buttons">
{{#if (and @canSignUp (not this.header.topic))}}
{{#if this.showSignupButton}}
<DButton
class="btn-primary btn-small sign-up-button"
@action={{@showCreateAccount}}
@ -16,12 +27,14 @@ export default class AuthButtons extends Component {
/>
{{/if}}
<DButton
class="btn-primary btn-small login-button"
@action={{@showLogin}}
@label="log_in"
@icon="user"
/>
{{#if this.showLoginButton}}
<DButton
class="btn-primary btn-small login-button"
@action={{@showLogin}}
@label="log_in"
@icon="user"
/>
{{/if}}
</span>
</template>
}

@ -47,6 +47,10 @@ export default class Icons extends Component {
return !this.args.sidebarEnabled || this.site.mobileView;
}
get hideSearchButton() {
return this.header.headerButtonsHidden.includes("search");
}
@action
toggleHamburger() {
if (this.sidebarState.adminSidebarAllowedWithLegacyNavigationMenu) {
@ -60,16 +64,18 @@ export default class Icons extends Component {
<ul class="icons d-header-icons">
{{#each (headerIcons.resolve) as |entry|}}
{{#if (eq entry.key "search")}}
<Dropdown
@title="search.title"
@icon="search"
@iconId={{@searchButtonId}}
@onClick={{@toggleSearchMenu}}
@active={{this.search.visible}}
@href={{getURL "/search"}}
@className="search-dropdown"
@targetSelector=".search-menu-panel"
/>
{{#unless this.hideSearchButton}}
<Dropdown
@title="search.title"
@icon="search"
@iconId={{@searchButtonId}}
@onClick={{@toggleSearchMenu}}
@active={{this.search.visible}}
@href={{getURL "/search"}}
@className="search-dropdown"
@targetSelector=".search-menu-panel"
/>
{{/unless}}
{{else if (eq entry.key "hamburger")}}
{{#if this.showHamburger}}
<Dropdown

@ -35,6 +35,11 @@ export default Controller.extend(PasswordValidation, {
redirected: false,
maskPassword: true,
init() {
this._super(...arguments);
this.set("selectedSecondFactorMethod", this.secondFactorMethod);
},
@discourseComputed()
continueButtonText() {
return I18n.t("password_reset.continue", {
@ -73,7 +78,7 @@ export default Controller.extend(PasswordValidation, {
password: this.accountPassword,
second_factor_token:
this.securityKeyCredential || this.secondFactorToken,
second_factor_method: this.secondFactorMethod,
second_factor_method: this.selectedSecondFactorMethod,
timezone: moment.tz.guess(),
},
})
@ -109,7 +114,7 @@ export default Controller.extend(PasswordValidation, {
this.rejectedPasswords.pushObject(this.accountPassword);
this.rejectedPasswordsMessages.set(
this.accountPassword,
result.errors.password[0]
(result.friendly_messages || []).join("\n")
);
}

@ -0,0 +1,15 @@
import Helper from "@ember/component/helper";
import { scheduleOnce } from "@ember/runloop";
import { service } from "@ember/service";
export default class HideApplicationHeaderButtons extends Helper {
@service header;
registerHider(buttons) {
this.header.registerHider(this, buttons);
}
compute([...buttons]) {
scheduleOnce("afterRender", this, this.registerHider, buttons);
}
}

@ -81,7 +81,9 @@ export default Mixin.create({
if (password.length < passwordMinLength) {
return EmberObject.create(
Object.assign(failedAttrs, {
reason: I18n.t("user.password.too_short"),
reason: I18n.t("user.password.too_short", {
password_min_length: passwordMinLength,
}),
})
);
}

@ -1,5 +1,7 @@
import { tracked } from "@glimmer/tracking";
import { registerDestructor } from "@ember/destroyable";
import Service, { service } from "@ember/service";
import { TrackedMap } from "@ember-compat/tracked-built-ins";
import { disableImplicitInjections } from "discourse/lib/implicit-injections";
@disableImplicitInjections
@ -11,6 +13,8 @@ export default class Header extends Service {
@tracked userVisible = false;
@tracked anyWidgetHeaderOverrides = false;
#hiders = new TrackedMap();
get useGlimmerHeader() {
if (this.siteSettings.glimmer_header_mode === "disabled") {
return false;
@ -29,4 +33,22 @@ export default class Header extends Service {
}
}
}
registerHider(ref, buttons) {
this.#hiders.set(ref, buttons);
registerDestructor(ref, () => {
this.#hiders.delete(ref);
});
}
get headerButtonsHidden() {
const buttonsToHide = new Set();
this.#hiders.forEach((buttons) => {
buttons.forEach((button) => {
buttonsToHide.add(button);
});
});
return Array.from(buttonsToHide);
}
}

@ -1,3 +1,6 @@
{{body-class "password-reset-page"}}
{{hide-application-sidebar}}
{{hide-application-header-buttons "search" "login" "signup"}}
<div class="container password-reset clearfix">
<div class="pull-left col-image">
<img src={{this.lockImageUrl}} class="password-reset-img" alt="" />
@ -19,8 +22,12 @@
{{/unless}}
{{/if}}
{{else}}
<form>
<form class="change-password-form">
{{#if this.securityKeyOrSecondFactorRequired}}
<h2>{{i18n "user.change_password.title"}}</h2>
<p>
{{i18n "user.change_password.verify_identity"}}
</p>
{{#if this.errorMessage}}
<div class="alert alert-error">{{this.errorMessage}}</div>
<br />
@ -31,13 +38,13 @@
@challenge={{this.model.security_key_challenge}}
@showSecurityKey={{this.model.security_key_required}}
@showSecondFactor={{false}}
@secondFactorMethod={{this.secondFactorMethod}}
@secondFactorMethod={{this.selectedSecondFactorMethod}}
@otherMethodAllowed={{this.otherMethodAllowed}}
@action={{action "authenticateSecurityKey"}}
/>
{{else}}
<SecondFactorForm
@secondFactorMethod={{this.secondFactorMethod}}
@secondFactorMethod={{this.selectedSecondFactorMethod}}
@secondFactorToken={{this.secondFactorToken}}
@backupEnabled={{this.backupEnabled}}
@isLogin={{false}}
@ -47,7 +54,7 @@
"input"
(with-event-value (fn (mut this.secondFactorToken)))
}}
@secondFactorMethod={{this.secondFactorMethod}}
@secondFactorMethod={{this.selectedSecondFactorMethod}}
value={{this.secondFactorToken}}
id="second-factor"
/>
@ -77,16 +84,15 @@
@maskPassword={{this.maskPassword}}
@togglePasswordMask={{this.togglePasswordMask}}
/>
<InputTip @validation={{this.passwordValidation}} />
</div>
<div class="instructions">
<div class="caps-lock-warning {{unless this.capsLockOn 'hidden'}}">
{{d-icon "exclamation-triangle"}}
{{i18n "login.caps_lock_warning"}}
</div>
</div>
<InputTip @validation={{this.passwordValidation}} />
<DButton
@action={{action "submit"}}
@label="user.change_password.set_password"

@ -232,7 +232,7 @@ createWidget(
);
createWidget("header-icons", {
services: ["search"],
services: ["search", "header"],
tagName: "ul.icons.d-header-icons",
init() {
@ -250,17 +250,19 @@ createWidget("header-icons", {
resolvedIcons.forEach((icon) => {
if (icon.key === "search") {
icons.push(
this.attach("header-dropdown", {
title: "search.title",
icon: "search",
iconId: SEARCH_BUTTON_ID,
action: "toggleSearchMenu",
active: this.search.visible,
href: getURL("/search"),
classNames: ["search-dropdown"],
})
);
if (!this.header.headerButtonsHidden.includes("search")) {
icons.push(
this.attach("header-dropdown", {
title: "search.title",
icon: "search",
iconId: SEARCH_BUTTON_ID,
action: "toggleSearchMenu",
active: this.search.visible,
href: getURL("/search"),
classNames: ["search-dropdown"],
})
);
}
} else if (icon.key === "user-menu" && attrs.user) {
icons.push(
this.attach("user-dropdown", {
@ -294,6 +296,7 @@ createWidget("header-icons", {
});
createWidget("header-buttons", {
services: ["header"],
tagName: "span.auth-buttons",
html(attrs) {
@ -303,7 +306,11 @@ createWidget("header-buttons", {
const buttons = [];
if (attrs.canSignUp && !attrs.topic) {
if (
attrs.canSignUp &&
!attrs.topic &&
!this.header.headerButtonsHidden.includes("signup")
) {
buttons.push(
this.attach("button", {
label: "sign_up",
@ -313,14 +320,17 @@ createWidget("header-buttons", {
);
}
buttons.push(
this.attach("button", {
label: "log_in",
className: "btn-primary btn-small login-button",
action: "showLogin",
icon: "user",
})
);
if (!this.header.headerButtonsHidden.includes("login")) {
buttons.push(
this.attach("button", {
label: "log_in",
className: "btn-primary btn-small login-button",
action: "showLogin",
icon: "user",
})
);
}
return buttons;
},
});

@ -27,6 +27,7 @@ acceptance("Password Reset", function (needs) {
return helper.response({
success: false,
errors: { password: ["is the name of your cat"] },
friendly_messages: ["Password is the name of your cat"],
});
} else {
return helper.response({
@ -52,6 +53,7 @@ acceptance("Password Reset", function (needs) {
return helper.response({
success: false,
errors: { password: ["invalid"] },
friendly_messages: ["Password is invalid"],
});
} else {
return helper.response({
@ -76,7 +78,9 @@ acceptance("Password Reset", function (needs) {
assert.ok(exists(".password-reset .tip.bad"), "input is not valid");
assert.ok(
query(".password-reset .tip.bad").innerHTML.includes(
I18n.t("user.password.too_short")
I18n.t("user.password.too_short", {
password_min_length: this.siteSettings.min_password_length,
})
),
"password too short"
);
@ -86,7 +90,7 @@ acceptance("Password Reset", function (needs) {
assert.ok(exists(".password-reset .tip.bad"), "input is not valid");
assert.ok(
query(".password-reset .tip.bad").innerHTML.includes(
"is the name of your cat"
"Password is the name of your cat"
),
"server validation error message shows"
);

@ -1,3 +1,4 @@
import { getOwner } from "@ember/application";
import { settled } from "@ember/test-helpers";
import { setupTest } from "ember-qunit";
import { module, test } from "qunit";
@ -75,8 +76,14 @@ module("Unit | Component | create-account", function (hooks) {
);
};
const siteSettings = getOwner(this).lookup("service:site-settings");
testInvalidPassword("", null);
testInvalidPassword("x", I18n.t("user.password.too_short"));
testInvalidPassword(
"x",
I18n.t("user.password.too_short", {
password_min_length: siteSettings.min_password_length,
})
);
testInvalidPassword(
"porkchops123",
I18n.t("user.password.same_as_username")

@ -50,6 +50,17 @@ body.invite-page {
}
}
.password-reset-page {
.caps-lock-warning {
display: inline;
}
.change-password-form {
.tip {
display: block;
}
}
}
.toggle-password-mask {
align-self: start;
line-height: 1.4; // aligns with input description text

@ -938,6 +938,7 @@ class UsersController < ApplicationController
success: false,
message: @error,
errors: @user&.errors&.to_hash,
friendly_messages: @user&.errors&.full_messages,
is_developer: UsernameCheckerService.is_developer?(@user&.email),
admin: @user&.admin?,
}

@ -1497,6 +1497,8 @@ en:
set_password: "Set Password"
choose_new: "Choose a new password"
choose: "Choose a password"
verify_identity: "To continue, please verify your identity."
title: "Password Reset"
second_factor_backup:
title: "Two-Factor Backup Codes"
@ -1933,7 +1935,9 @@ en:
fine_print: "We are asking you to confirm your identity because this is a potentially sensitive action. Once authenticated, you will only be asked to re-authenticate again after a few hours of inactivity."
password:
title: "Password"
too_short: "Your password is too short."
too_short:
one: "Your password is too short (minimum is %{password_min_length} character)."
other: "Your password is too short (minimum is %{password_min_length} characters)."
common: "That password is too common."
same_as_username: "Your password is the same as your username."
same_as_email: "Your password is the same as your email."

@ -723,6 +723,7 @@ en:
bio_raw: "About Me"
user:
ip_address: ""
password: "Password"
errors:
<<: *errors
models:

@ -192,6 +192,23 @@ RSpec.describe "Glimmer Header", type: :system do
)
end
context "when resetting password" do
fab!(:current_user) { Fabricate(:user) }
it "does not show search, login, or signup buttons" do
email_token =
current_user.email_tokens.create!(
email: current_user.email,
scope: EmailToken.scopes[:password_reset],
)
visit "/u/password-reset/#{email_token.token}"
expect(page).not_to have_css("button.login-button")
expect(page).not_to have_css("button.sign-up-button")
expect(page).not_to have_css(".search-dropdown #search-button")
end
end
context "when logged in and login required" do
fab!(:current_user) { Fabricate(:user) }

@ -0,0 +1,22 @@
# frozen_string_literal: true
RSpec.describe "Legacy Widget Header", type: :system do
before { SiteSetting.glimmer_header_mode = "disabled" }
context "when resetting password" do
fab!(:current_user) { Fabricate(:user) }
it "does not show search, login, or signup buttons" do
email_token =
current_user.email_tokens.create!(
email: current_user.email,
scope: EmailToken.scopes[:password_reset],
)
visit "/u/password-reset/#{email_token.token}"
expect(page).not_to have_css("button.login-button")
expect(page).not_to have_css("button.sign-up-button")
expect(page).not_to have_css(".search-dropdown #search-button")
end
end
end

@ -8,6 +8,19 @@ shared_examples "login scenarios" do
before { Jobs.run_immediately! }
def wait_for_email_link(user, type)
wait_for(timeout: 5) { ActionMailer::Base.deliveries.count != 0 }
mail = ActionMailer::Base.deliveries.last
expect(mail.to).to contain_exactly(user.email)
if type == :reset_password
mail.body.to_s[%r{/u/password-reset/\S+}]
elsif type == :activation
mail.body.to_s[%r{/u/activate-account/\S+}]
elsif type == :email_login
mail.body.to_s[%r{/session/email-login/\S+}]
end
end
context "with username and password" do
it "can login" do
EmailToken.confirm(Fabricate(:email_token, user: user).token)
@ -25,11 +38,7 @@ shared_examples "login scenarios" do
expect(page).to have_css(".not-activated-modal")
login_modal.click(".activation-controls button.resend")
wait_for(timeout: 5) { ActionMailer::Base.deliveries.count != 0 }
mail = ActionMailer::Base.deliveries.last
expect(mail.to).to contain_exactly(user.email)
activation_link = mail.body.to_s[%r{/u/activate-account/\S+}]
activation_link = wait_for_email_link(user, :activation)
visit activation_link
find("#activate-account-button").click
@ -53,11 +62,8 @@ shared_examples "login scenarios" do
login_modal.find("#modal-alert a").click
find("button.forgot-password-reset").click
wait_for(timeout: 5) { ActionMailer::Base.deliveries.count != 0 }
mail = ActionMailer::Base.deliveries.last
expect(mail.to).to contain_exactly(user.email)
expect(mail.body).to match(%r{/u/password-reset/\S+})
reset_password_link = wait_for_email_link(user, :reset_password)
expect(reset_password_link).to be_present
end
it "can reset password" do
@ -66,15 +72,11 @@ shared_examples "login scenarios" do
login_modal.forgot_password
find("button.forgot-password-reset").click
wait_for(timeout: 5) { ActionMailer::Base.deliveries.count != 0 }
mail = ActionMailer::Base.deliveries.last
expect(mail.to).to contain_exactly(user.email)
reset_password_link = mail.body.to_s[%r{/u/password-reset/\S+}]
reset_password_link = wait_for_email_link(user, :reset_password)
visit reset_password_link
find("#new-account-password").fill_in(with: "newsuperpassword")
find("form .btn-primary").click
find(".change-password-form .btn-primary").click
expect(page).to have_css(".header-dropdown-toggle.current-user")
end
end
@ -85,11 +87,7 @@ shared_examples "login scenarios" do
login_modal.fill_username("john")
login_modal.email_login_link
wait_for(timeout: 5) { ActionMailer::Base.deliveries.count != 0 }
mail = ActionMailer::Base.deliveries.last
expect(mail.to).to contain_exactly(user.email)
login_link = mail.body.to_s[%r{/session/email-login/\S+}]
login_link = wait_for_email_link(user, :email_login)
visit login_link
find(".email-login-form .btn-primary").click
@ -148,11 +146,7 @@ shared_examples "login scenarios" do
login_modal.fill_username("john")
login_modal.email_login_link
wait_for(timeout: 5) { ActionMailer::Base.deliveries.count != 0 }
mail = ActionMailer::Base.deliveries.last
expect(mail.to).to contain_exactly(user.email)
login_link = mail.body.to_s[%r{/session/email-login/\S+}]
login_link = wait_for_email_link(user, :email_login)
visit login_link
totp = ROTP::TOTP.new(user_second_factor.data).now
@ -166,11 +160,7 @@ shared_examples "login scenarios" do
login_modal.fill_username("john")
login_modal.email_login_link
wait_for(timeout: 5) { ActionMailer::Base.deliveries.count != 0 }
mail = ActionMailer::Base.deliveries.last
expect(mail.to).to contain_exactly(user.email)
login_link = mail.body.to_s[%r{/session/email-login/\S+}]
login_link = wait_for_email_link(user, :email_login)
visit login_link
find(".toggle-second-factor-method").click
@ -178,6 +168,42 @@ shared_examples "login scenarios" do
find(".email-login-form .btn-primary").click
expect(page).to have_css(".header-dropdown-toggle.current-user")
end
it "can reset password with TOTP" do
login_modal.open
login_modal.fill_username("john")
login_modal.forgot_password
find("button.forgot-password-reset").click
reset_password_link = wait_for_email_link(user, :reset_password)
visit reset_password_link
totp = ROTP::TOTP.new(user_second_factor.data).now
find(".second-factor-token-input").fill_in(with: totp)
find(".password-reset .btn-primary").click
find("#new-account-password").fill_in(with: "newsuperpassword")
find(".change-password-form .btn-primary").click
expect(page).to have_css(".header-dropdown-toggle.current-user")
end
it "can reset password with a backup code" do
login_modal.open
login_modal.fill_username("john")
login_modal.forgot_password
find("button.forgot-password-reset").click
reset_password_link = wait_for_email_link(user, :reset_password)
visit reset_password_link
find(".toggle-second-factor-method").click
find(".second-factor-token-input").fill_in(with: "iAmValidBackupCode")
find(".password-reset .btn-primary").click
find("#new-account-password").fill_in(with: "newsuperpassword")
find(".change-password-form .btn-primary").click
expect(page).to have_css(".header-dropdown-toggle.current-user")
end
end
end