Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: User can't reset password with backup codes when only security key is enabled (#27368) 

This commit fixes a problem where the user will not be able to reset
their password when they only have security keys and backup codes
configured.

This commit also makes the following changes/fixes:

1. Splits password reset system tests to
   `spec/system/forgot_password_spec.rb` instead of missing the system
   tests in `spec/system/login_spec.rb` which is mainly used to test
   the login flow.

2. Fixes a UX issue where the `Use backup codes` or `Use authenticator
   app` text is shown on the reset password form when the user does
   not have either backup codes or an authenticator app configured.
This commit is contained in:
Alan Guo Xiang Tan 2024-06-06 14:30:42 +08:00 committed by GitHub
parent 4b1e017722
commit 952f69ce60
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
16 changed files with 363 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/assets/javascripts/discourse/app/components/modal/login.hbs
View File

@ -58,6 +58,7 @@
@secondFactorMethod={{this.secondFactorMethod}}
@secondFactorToken={{this.secondFactorToken}}
@backupEnabled={{this.backupEnabled}}
@totpEnabled={{this.totpEnabled}}
@securityKeyAllowedCredentialIds={{this.securityKeyAllowedCredentialIds}}
@securityKeyChallenge={{this.securityKeyChallenge}}
@showSecurityKey={{this.showSecurityKey}}

3
app/assets/javascripts/discourse/app/components/modal/login/local-login-form.hbs
View File

@ -72,6 +72,7 @@
@secondFactorMethod={{@secondFactorMethod}}
@secondFactorToken={{@secondFactorToken}}
@backupEnabled={{@backupEnabled}}
@totpEnabled={{@totpEnabled}}
@isLogin={{true}}
class={{this.secondFactorClass}}
>
@ -82,6 +83,8 @@
@showSecurityKey={{@showSecurityKey}}
@showSecondFactor={{@showSecondFactor}}
@secondFactorMethod={{@secondFactorMethod}}
@backupEnabled={{@backupEnabled}}
@totpEnabled={{@totpEnabled}}
@otherMethodAllowed={{@otherMethodAllowed}}
@action={{this.authenticateSecurityKey}}
/>

6
app/assets/javascripts/discourse/app/components/second-factor-form.hbs
View File

@ -1,10 +1,14 @@
<div id="second-factor">
<h3>{{this.secondFactorTitle}}</h3>
{{#if this.optionalText}}
<p>{{html-safe this.optionalText}}</p>
{{/if}}
<p>{{this.secondFactorDescription}}</p>
<p class="second-factor__description">{{this.secondFactorDescription}}</p>
{{yield}}
{{#if this.showToggleMethodLink}}
<p>
<a

8
app/assets/javascripts/discourse/app/components/second-factor-form.js
View File

@ -42,10 +42,12 @@ export default Component.extend({
}
},
@discourseComputed("backupEnabled", "secondFactorMethod")
showToggleMethodLink(backupEnabled, secondFactorMethod) {
@discourseComputed("backupEnabled", "totpEnabled", "secondFactorMethod")
showToggleMethodLink(backupEnabled, totpEnabled, secondFactorMethod) {
return (
backupEnabled && secondFactorMethod !== SECOND_FACTOR_METHODS.SECURITY_KEY
backupEnabled &&
totpEnabled &&
secondFactorMethod !== SECOND_FACTOR_METHODS.SECURITY_KEY
);
},

7
app/assets/javascripts/discourse/app/components/security-key-form.js
View File

@ -8,6 +8,11 @@ export default Component.extend({
event?.preventDefault();
this.set("showSecurityKey", false);
this.set("showSecondFactor", true);
this.set("secondFactorMethod", SECOND_FACTOR_METHODS.TOTP);
if (this.totpEnabled) {
this.set("secondFactorMethod", SECOND_FACTOR_METHODS.TOTP);
} else if (this.backupEnabled) {
this.set("secondFactorMethod", SECOND_FACTOR_METHODS.BACKUP_CODE);
}
},
});

29
app/assets/javascripts/discourse/app/controllers/password-reset.js
View File

@ -22,12 +22,6 @@ export default Controller.extend(PasswordValidation, {
"model.security_key_required"
),
otherMethodAllowed: readOnly("model.multiple_second_factor_methods"),
@discourseComputed("model.security_key_required")
secondFactorMethod(security_key_required) {
return security_key_required
? SECOND_FACTOR_METHODS.SECURITY_KEY
: SECOND_FACTOR_METHODS.TOTP;
},
passwordRequired: true,
errorMessage: null,
successMessage: null,
@ -35,9 +29,25 @@ export default Controller.extend(PasswordValidation, {
redirected: false,
maskPassword: true,
init() {
this._super(...arguments);
this.set("selectedSecondFactorMethod", this.secondFactorMethod);
@discourseComputed("securityKeyRequired", "selectedSecondFactorMethod")
displaySecurityKeyForm(securityKeyRequired, selectedSecondFactorMethod) {
return (
securityKeyRequired &&
selectedSecondFactorMethod === SECOND_FACTOR_METHODS.SECURITY_KEY
);
},
initSelectedSecondFactorMethod() {
if (this.model.security_key_required) {
this.set(
"selectedSecondFactorMethod",
SECOND_FACTOR_METHODS.SECURITY_KEY
);
} else if (this.model.second_factor_required) {
this.set("selectedSecondFactorMethod", SECOND_FACTOR_METHODS.TOTP);
} else if (this.model.backup_enabled) {
this.set("selectedSecondFactorMethod", SECOND_FACTOR_METHODS.BACKUP_CODE);
}
},
@discourseComputed()
@ -140,6 +150,7 @@ export default Controller.extend(PasswordValidation, {
"selectedSecondFactorMethod",
SECOND_FACTOR_METHODS.SECURITY_KEY
);
getWebauthnCredential(
this.model.challenge,
this.model.allowed_credential_ids,

5
app/assets/javascripts/discourse/app/routes/password-reset.js
View File

@ -27,4 +27,9 @@ export default class PasswordReset extends DiscourseRoute {
});
}
}
setupController(controller, model) {
controller.set("model", model);
controller.initSelectedSecondFactorMethod();
}
}

3
app/assets/javascripts/discourse/app/templates/email-login.hbs
View File

@ -21,6 +21,8 @@
@showSecurityKey={{this.model.security_key_required}}
@showSecondFactor={{false}}
@secondFactorMethod={{this.secondFactorMethod}}
@backupEnabled={{this.model.backup_codes_enabled}}
@totpEnabled={{this.model.totp_enabled}}
@otherMethodAllowed={{this.secondFactorRequired}}
@action={{action "authenticateSecurityKey"}}
/>
@ -29,6 +31,7 @@
@secondFactorMethod={{this.secondFactorMethod}}
@secondFactorToken={{this.secondFactorToken}}
@backupEnabled={{this.model.backup_codes_enabled}}
@totpEnabled={{this.model.totp_enabled}}
@isLogin={{true}}
>
<SecondFactorInput

11
app/assets/javascripts/discourse/app/templates/password-reset.hbs
View File

@ -32,13 +32,16 @@
<div class="alert alert-error">{{this.errorMessage}}</div>
<br />
{{/if}}
{{#if this.securityKeyRequired}}
{{#if this.displaySecurityKeyForm}}
<SecurityKeyForm
@allowedCredentialIds={{this.model.allowed_credential_ids}}
@challenge={{this.model.security_key_challenge}}
@showSecurityKey={{this.model.security_key_required}}
@showSecurityKey={{false}}
@showSecondFactor={{false}}
@secondFactorMethod={{this.selectedSecondFactorMethod}}
@backupEnabled={{this.backupEnabled}}
@totpEnabled={{this.secondFactorRequired}}
@otherMethodAllowed={{this.otherMethodAllowed}}
@action={{action "authenticateSecurityKey"}}
/>
@ -47,6 +50,7 @@
@secondFactorMethod={{this.selectedSecondFactorMethod}}
@secondFactorToken={{this.secondFactorToken}}
@backupEnabled={{this.backupEnabled}}
@totpEnabled={{this.secondFactorRequired}}
@isLogin={{false}}
>
<SecondFactorInput
@ -60,7 +64,8 @@
/>
</SecondFactorForm>
{{/if}}
{{#unless this.securityKeyRequired}}
{{#unless this.displaySecurityKeyForm}}
<DButton
@action={{action "submit"}}
@label="submit"

1
app/controllers/session_controller.rb
View File

@ -418,6 +418,7 @@ class SessionController < ApplicationController
response.merge!(
second_factor_required: true,
backup_codes_enabled: matched_user&.backup_codes_enabled?,
totp_enabled: matched_user&.totp_enabled?,
)
end

1
spec/requests/session_controller_spec.rb
View File

@ -99,6 +99,7 @@ RSpec.describe SessionController do
expect(response_body_parsed["can_login"]).to eq(true)
expect(response_body_parsed["second_factor_required"]).to eq(true)
expect(response_body_parsed["backup_codes_enabled"]).to eq(true)
expect(response_body_parsed["totp_enabled"]).to eq(true)
end
end

2
spec/system/email_change_spec.rb
View File

@ -102,7 +102,7 @@ describe "Changing email", type: :system do
expect(page).to have_current_path("/u/#{user.username}/preferences/account")
expect(user_preferences_page).to have_primary_email(new_email)
ensure
authenticator.remove!
authenticator&.remove!
end
it "does not require login to verify" do

220
spec/system/forgot_password_spec.rb Normal file
View File

@ -0,0 +1,220 @@
# frozen_string_literal: true
require "rotp"
shared_examples "forgot password scenarios" do
let(:login_modal) { PageObjects::Modals::Login.new }
let(:user_preferences_security_page) { PageObjects::Pages::UserPreferencesSecurity.new }
fab!(:user) { Fabricate(:user, username: "john", password: "supersecurepassword") }
fab!(:password_reset_token) do
Fabricate(
:email_token,
user:,
scope: EmailToken.scopes[:password_reset],
email: user.email,
).token
end
let(:user_menu) { PageObjects::Components::UserMenu.new }
let(:user_reset_password_page) { PageObjects::Pages::UserResetPassword.new }
def visit_reset_password_link
visit("/u/password-reset/#{password_reset_token}")
end
def create_user_security_key(user)
# testing the 2FA flow requires a user that was created > 5 minutes ago
user.update!(created_at: 6.minutes.ago)
sign_in(user)
user_preferences_security_page.visit(user)
user_preferences_security_page.visit_second_factor("supersecurepassword")
find(".security-key .new-security-key").click
expect(user_preferences_security_page).to have_css("input#security-key-name")
find(".d-modal__body input#security-key-name").fill_in(with: "First Key")
find(".add-security-key").click
expect(user_preferences_security_page).to have_css(".security-key .second-factor-item")
user_menu.sign_out
end
context "when user does not have any multi-factor authentication configured" do
it "should allow a user to reset their password" do
visit_reset_password_link
user_reset_password_page.fill_in_new_password("newsuperpassword").submit_new_password
expect(user_reset_password_page).to have_logged_in_user
end
end
context "when user has multi-factor authentication configured" do
context "when user only has TOTP configured" do
fab!(:user_second_factor_totp) { Fabricate(:user_second_factor_totp, user:) }
it "should allow a user to reset password with TOTP" do
visit_reset_password_link
expect(user_reset_password_page).to have_no_toggle_button_to_second_factor_form
user_reset_password_page
.fill_in_totp(ROTP::TOTP.new(user_second_factor_totp.data).now)
.submit_totp
.fill_in_new_password("newsuperpassword")
.submit_new_password
expect(user_reset_password_page).to have_logged_in_user
end
end
context "when user only has security key configured" do
before do
@authenticator =
page.driver.browser.add_virtual_authenticator(
Selenium::WebDriver::VirtualAuthenticatorOptions.new,
)
create_user_security_key(user)
end
after { @authenticator.remove! }
it "should allow a user to reset password with a security key" do
visit_reset_password_link
expect(user_reset_password_page).to have_no_toggle_button_to_second_factor_form
user_reset_password_page.submit_security_key
user_reset_password_page.fill_in_new_password("newsuperpassword").submit_new_password
expect(user_reset_password_page).to have_logged_in_user
end
end
context "when user has TOTP and backup codes configured" do
fab!(:user_second_factor_backup) { Fabricate(:user_second_factor_backup, user:) }
fab!(:user_second_factor_totp) { Fabricate(:user_second_factor_totp, user:) }
it "should allow a user to reset password with backup code" do
visit_reset_password_link
user_reset_password_page
.use_backup_codes
.fill_in_backup_code("iAmValidBackupCode")
.submit_backup_code
.fill_in_new_password("newsuperpassword")
.submit_new_password
expect(user_reset_password_page).to have_logged_in_user
end
end
context "when user has security key and backup codes configured" do
fab!(:user_second_factor_backup) { Fabricate(:user_second_factor_backup, user:) }
before do
@authenticator =
page.driver.browser.add_virtual_authenticator(
Selenium::WebDriver::VirtualAuthenticatorOptions.new,
)
create_user_security_key(user)
end
after { @authenticator.remove! }
it "should allow a user to reset password with backup code instead of security key" do
visit_reset_password_link
user_reset_password_page.try_another_way
expect(user_reset_password_page).to have_no_toggle_button_in_second_factor_form
user_reset_password_page
.fill_in_backup_code("iAmValidBackupCode")
.submit_backup_code
.fill_in_new_password("newsuperpassword")
.submit_new_password
expect(user_reset_password_page).to have_logged_in_user
end
end
context "when user has TOTP, security key and backup codes configured" do
fab!(:user_second_factor_totp) { Fabricate(:user_second_factor_totp, user:) }
fab!(:user_second_factor_backup) { Fabricate(:user_second_factor_backup, user:) }
before do
@authenticator =
page.driver.browser.add_virtual_authenticator(
Selenium::WebDriver::VirtualAuthenticatorOptions.new,
)
create_user_security_key(user)
end
after { @authenticator.remove! }
it "should allow a user to toggle from security key to TOTP and between TOTP and backup codes" do
visit_reset_password_link
user_reset_password_page.try_another_way
expect(user_reset_password_page).to have_totp_description
user_reset_password_page.use_backup_codes
expect(user_reset_password_page).to have_backup_codes_description
user_reset_password_page.use_totp
expect(user_reset_password_page).to have_totp_description
end
end
context "when user has TOTP and security key configured but no backup codes" do
fab!(:user_second_factor_totp) { Fabricate(:user_second_factor_totp, user:) }
before do
@authenticator =
page.driver.browser.add_virtual_authenticator(
Selenium::WebDriver::VirtualAuthenticatorOptions.new,
)
create_user_security_key(user)
end
after { @authenticator.remove! }
it "should allow a user to reset password with TOTP instead of security key" do
visit_reset_password_link
user_reset_password_page.try_another_way
expect(user_reset_password_page).to have_no_toggle_button_in_second_factor_form
user_reset_password_page
.fill_in_totp(ROTP::TOTP.new(user_second_factor_totp.data).now)
.submit_totp
.fill_in_new_password("newsuperpassword")
.submit_new_password
expect(user_reset_password_page).to have_logged_in_user
end
end
end
end
describe "User resetting password", type: :system do
context "when desktop" do
include_examples "forgot password scenarios"
end
context "when mobile", mobile: true do
include_examples "forgot password scenarios"
end
end

55
spec/system/login_spec.rb
View File

@ -61,26 +61,13 @@ shared_examples "login scenarios" do
expect(login_modal.find("#modal-alert")).to have_content(
I18n.t("js.login.password_expired", reset_url: "/password-reset").gsub(/<.*?>/, ""),
)
login_modal.find("#modal-alert a").click
find("button.forgot-password-reset").click
reset_password_link = wait_for_email_link(user, :reset_password)
expect(reset_password_link).to be_present
end
it "can reset password" 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("#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
context "with login link" do
@ -226,46 +213,6 @@ shared_examples "login scenarios" do
find(".change-password-form .btn-primary").click
expect(page).to have_css(".header-dropdown-toggle.current-user")
end
it "can reset password with a security key" do
# testing the 2FA flow requires a user that was created > 5 minutes ago
user.created_at = 6.minutes.ago
user.save!
sign_in(user)
options = ::Selenium::WebDriver::VirtualAuthenticatorOptions.new
authenticator = page.driver.browser.add_virtual_authenticator(options)
user_preferences_security_page.visit(user)
user_preferences_security_page.visit_second_factor("supersecurepassword")
find(".security-key .new-security-key").click
expect(user_preferences_security_page).to have_css("input#security-key-name")
find(".d-modal__body input#security-key-name").fill_in(with: "First Key")
find(".add-security-key").click
expect(user_preferences_security_page).to have_css(".security-key .second-factor-item")
user_menu.sign_out
# reset password flow
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("#security-key .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")
ensure
# clear authenticator (otherwise it will interfere with other tests)
authenticator&.remove!
end
end
end

79
spec/system/page_objects/pages/user_reset_password.rb Normal file
View File

@ -0,0 +1,79 @@
# frozen_string_literal: true
module PageObjects
module Pages
class UserResetPassword < PageObjects::Pages::Base
def has_no_toggle_button_to_second_factor_form?
page.has_no_css?("#security-key .toggle-second-factor-method")
end
def has_no_toggle_button_in_second_factor_form?
page.has_no_css?("#second-factor .toggle-second-factor-method")
end
def has_totp_description?
page.find(".second-factor__description").has_text?(
I18n.t("js.login.second_factor_description"),
)
end
def has_backup_codes_description?
page.find(".second-factor__description").has_text?(
I18n.t("js.login.second_factor_backup_description"),
)
end
def has_logged_in_user?
page.has_css?(".header-dropdown-toggle.current-user")
end
def use_totp
find(".toggle-second-factor-method", text: I18n.t("js.user.second_factor.use")).click
end
def use_backup_codes
find(".toggle-second-factor-method", text: I18n.t("js.user.second_factor_backup.use")).click
self
end
def try_another_way
find("#security-key .toggle-second-factor-method").click
self
end
def submit_security_key
find("#security-key-authenticate-button").click
self
end
def fill_in_new_password(password)
find("#new-account-password").fill_in(with: password)
self
end
def submit_new_password
find(".change-password-form .btn-primary").click
self
end
def fill_in_backup_code(backup_code)
find("#second-factor .second-factor-token-input").fill_in(with: "iAmValidBackupCode")
self
end
def submit_backup_code
find(".change-password-form .btn-primary").click
self
end
def fill_in_totp(totp)
find("#second-factor .second-factor-token-input").fill_in(with: totp)
self
end
def submit_totp
find(".change-password-form .btn-primary").click
self
end
end
end
end

8
spec/system/user_page/user_preferences_security_spec.rb
View File

@ -45,9 +45,9 @@ describe "User preferences | Security", type: :system do
find("#security-key .btn-primary").click
expect(page).to have_css(".header-dropdown-toggle.current-user")
ensure
# clear authenticator (otherwise it will interfere with other tests)
authenticator.remove!
authenticator&.remove!
end
end
@ -106,9 +106,9 @@ describe "User preferences | Security", type: :system do
# ensures that we are redirected to the destination_url cookie
expect(page.driver.current_url).to include("/new")
ensure
# clear authenticator (otherwise it will interfere with other tests)
authenticator.remove!
authenticator&.remove!
end
end