From a1c1f7ce75d8f881539778322f71d26cda53f03d Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Tue, 7 Nov 2023 08:26:10 -0800 Subject: [PATCH] DEV: Standardize session confirmation prompt (#24212) Switches to using a dialog to confirm a session (i.e. sudo mode for account changes where we want to be extra sure the current user is who they say they are) to match what we do with passkeys. --- .../controllers/preferences/second-factor.js | 49 +-- .../app/controllers/preferences/security.js | 24 ++ .../javascripts/discourse/app/models/user.js | 3 +- .../app/routes/preferences-second-factor.js | 7 +- .../templates/preferences-second-factor.hbs | 288 +++++++----------- .../app/templates/preferences/security.hbs | 34 +-- .../acceptance/enforce-second-factor-test.js | 40 +-- .../user-preferences-second-factor-test.js | 45 +-- app/assets/stylesheets/common/base/user.scss | 21 ++ app/assets/stylesheets/desktop/user.scss | 19 -- app/controllers/users_controller.rb | 8 +- config/locales/client.en.yml | 2 - spec/requests/users_controller_spec.rb | 61 ++-- .../pages/user_preferences_security.rb | 6 +- 14 files changed, 249 insertions(+), 358 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js b/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js index 4ced1bc0752..81fc760ecfb 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js @@ -21,17 +21,12 @@ export default Controller.extend(CanCheckEmails, { modal: service(), loading: false, dirty: false, - resetPasswordLoading: false, - resetPasswordProgress: "", - password: null, errorMessage: null, newUsername: null, backupEnabled: alias("model.second_factor_backup_enabled"), secondFactorMethod: SECOND_FACTOR_METHODS.TOTP, totps: null, - loaded: false, - init() { this._super(...arguments); this.set("totps", []); @@ -47,6 +42,11 @@ export default Controller.extend(CanCheckEmails, { return user && user.enforcedSecondFactor; }, + @discourseComputed("totps", "security_keys") + hasSecondFactors(totps, security_keys) { + return totps.length > 0 || security_keys.length > 0; + }, + @action handleError(error) { if (error.jqXHR) { @@ -81,7 +81,7 @@ export default Controller.extend(CanCheckEmails, { this.set("loading", true); this.model - .loadSecondFactorCodes(this.password) + .loadSecondFactorCodes() .then((response) => { if (response.error) { this.set("errorMessage", response.error); @@ -90,17 +90,10 @@ export default Controller.extend(CanCheckEmails, { this.setProperties({ errorMessage: null, - loaded: true, totps: response.totps, security_keys: response.security_keys, - password: null, dirty: false, }); - this.set( - "model.second_factor_enabled", - (response.totps && response.totps.length > 0) || - (response.security_keys && response.security_keys.length > 0) - ); }) .catch((e) => this.handleError(e)) .finally(() => this.set("loading", false)); @@ -111,37 +104,7 @@ export default Controller.extend(CanCheckEmails, { this.set("dirty", true); }, - @action - resetPassword(event) { - event?.preventDefault(); - - this.setProperties({ - resetPasswordLoading: true, - resetPasswordProgress: "", - }); - - return this.model - .changePassword() - .then(() => { - this.set( - "resetPasswordProgress", - I18n.t("user.change_password.success") - ); - }) - .catch(popupAjaxError) - .finally(() => this.set("resetPasswordLoading", false)); - }, - actions: { - confirmPassword() { - if (!this.password) { - return; - } - this.markDirty(); - this.loadSecondFactors(); - this.set("password", null); - }, - disableAllSecondFactors() { if (this.loading) { return; diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/security.js b/app/assets/javascripts/discourse/app/controllers/preferences/security.js index 3f3ce94cbd6..9483dd87a21 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/security.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/security.js @@ -2,6 +2,7 @@ import Controller from "@ember/controller"; import { action } from "@ember/object"; import { gt } from "@ember/object/computed"; import { inject as service } from "@ember/service"; +import ConfirmSession from "discourse/components/dialog-messages/confirm-session"; import AuthTokenModal from "discourse/components/modal/auth-token"; import { ajax } from "discourse/lib/ajax"; import { popupAjaxError } from "discourse/lib/ajax-error"; @@ -17,6 +18,8 @@ const DEFAULT_AUTH_TOKENS_COUNT = 2; export default Controller.extend(CanCheckEmails, { modal: service(), + dialog: service(), + router: service(), passwordProgress: null, subpageTitle: I18n.t("user.preferences_nav.security"), showAllAuthTokens: false, @@ -114,6 +117,27 @@ export default Controller.extend(CanCheckEmails, { .catch(popupAjaxError); }, + @action + async manage2FA() { + try { + const trustedSession = await this.model.trustedSession(); + + if (!trustedSession.success) { + this.dialog.dialog({ + title: I18n.t("user.confirm_access.title"), + type: "notice", + bodyComponent: ConfirmSession, + didConfirm: () => + this.router.transitionTo("preferences.second-factor"), + }); + } else { + await this.router.transitionTo("preferences.second-factor"); + } + } catch (error) { + popupAjaxError(error); + } + }, + actions: { save() { this.set("saved", false); diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js index f050d3905af..aea705bb47a 100644 --- a/app/assets/javascripts/discourse/app/models/user.js +++ b/app/assets/javascripts/discourse/app/models/user.js @@ -540,9 +540,8 @@ const User = RestModel.extend({ }); }, - loadSecondFactorCodes(password) { + loadSecondFactorCodes() { return ajax("/u/second_factors.json", { - data: { password }, type: "POST", }); }, diff --git a/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js b/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js index 189741aa1d6..514639b64e2 100644 --- a/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js +++ b/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js @@ -5,6 +5,7 @@ import RestrictedUserRoute from "discourse/routes/restricted-user"; export default RestrictedUserRoute.extend({ currentUser: service(), siteSettings: service(), + router: service(), model() { return this.modelFor("user"); @@ -15,15 +16,15 @@ export default RestrictedUserRoute.extend({ controller.set("loading", true); model - .loadSecondFactorCodes("") + .loadSecondFactorCodes() .then((response) => { if (response.error) { controller.set("errorMessage", response.error); + } else if (response.unconfirmed_session) { + this.router.transitionTo("preferences.security"); } else { controller.setProperties({ errorMessage: null, - loaded: !response.password_required, - dirty: !!response.password_required, totps: response.totps, security_keys: response.security_keys, }); diff --git a/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs b/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs index 48c946884f1..08e69013270 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs @@ -19,194 +19,140 @@
{{this.errorMessage}}
{{/if}} - {{#if this.loaded}} -
-
-

{{i18n "user.second_factor.totp.title"}}

- {{#each this.totps as |totp|}} -
-
- {{#if totp.name}} - {{totp.name}} - {{else}} - {{i18n "user.second_factor.totp.default_name"}} - {{/if}} -
- {{#if this.isCurrentUser}} -
- -
- {{/if}} -
- {{/each}} - -
-
- -
-
-

{{i18n "user.second_factor.security_key.title"}}

- {{#each this.security_keys as |security_key|}} -
-
- {{#if security_key.name}} - {{security_key.name}} - {{else}} - {{i18n "user.second_factor.security_key.default_name"}} - {{/if}} -
- - {{#if this.isCurrentUser}} -
- -
- {{/if}} -
- {{/each}} - -
-
- -
-
-

{{i18n "user.second_factor_backup.title"}}

+
+
+

{{i18n "user.second_factor.totp.title"}}

+ {{#each this.totps as |totp|}}
- {{#if this.model.second_factor_enabled}} -
- {{#if this.model.second_factor_backup_enabled}} - {{html-safe - (i18n - "user.second_factor_backup.manage" - count=this.model.second_factor_remaining_backup_codes - ) - }} - {{else}} - - {{/if}} -
- - {{#if - (and - this.model.second_factor_backup_enabled this.isCurrentUser - ) - }} -
- -
+
+ {{#if totp.name}} + {{totp.name}} + {{else}} + {{i18n "user.second_factor.totp.default_name"}} {{/if}} - - {{else}} - {{i18n "user.second_factor_backup.enable_prerequisites"}} +
+ {{#if this.isCurrentUser}} +
+ +
{{/if}}
-
+ {{/each}} +
+
- {{#if this.model.second_factor_enabled}} - {{#unless this.showEnforcedNotice}} -
-
- - +
+
+

{{i18n "user.second_factor.security_key.title"}}

+ {{#each this.security_keys as |security_key|}} +
+
+ {{#if security_key.name}} + {{security_key.name}} + {{else}} + {{i18n "user.second_factor.security_key.default_name"}} + {{/if}}
-
- {{/unless}} - {{/if}} - {{else}} -
- -
-
- -
-
- {{i18n "user.second_factor.confirm_password_description"}} + {{#if this.isCurrentUser}} +
+ +
+ {{/if}}
+ {{/each}} + +
+
+ +
+
+

{{i18n "user.second_factor_backup.title"}}

+
+ {{#if this.model.second_factor_enabled}} +
+ {{#if this.model.second_factor_backup_enabled}} + {{html-safe + (i18n + "user.second_factor_backup.manage" + count=this.model.second_factor_remaining_backup_codes + ) + }} + {{else}} + + {{/if}} +
+ + {{#if + (and this.model.second_factor_backup_enabled this.isCurrentUser) + }} +
+ +
+ {{/if}} + + {{else}} + {{i18n "user.second_factor_backup.enable_prerequisites"}} + {{/if}}
+
-
-
- - - {{#unless this.showEnforcedNotice}} + {{#if this.hasSecondFactors}} + {{#unless this.showEnforcedNotice}} +
+
+ - {{/unless}} +
-
- {{this.resetPasswordProgress}} - {{#unless this.resetPasswordLoading}} - {{i18n "user.second_factor.forgot_password"}} - {{/unless}} -
-
+ {{/unless}} {{/if}} diff --git a/app/assets/javascripts/discourse/app/templates/preferences/security.hbs b/app/assets/javascripts/discourse/app/templates/preferences/security.hbs index a16cd5ae82d..af2308afad7 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences/security.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences/security.hbs @@ -19,28 +19,26 @@ {{/if}} -
- - {{#unless this.model.second_factor_enabled}} + {{#if this.isCurrentUser}} +
+
{{i18n "user.second_factor.short_description"}}
- {{/unless}} -
- {{#if this.isCurrentUser}} - - {{d-icon "lock"}} - {{i18n "user.second_factor.enable"}} - - {{/if}} + +
+ +
-
+ {{/if}} {{/if}} {{#if this.canCheckEmails}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js b/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js index 991277c605f..4ef47d8524d 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js @@ -15,13 +15,13 @@ async function catchAbortedTransition() { } } -acceptance("Enforce Second Factor", function (needs) { +acceptance("Enforce Second Factor for unconfirmed session", function (needs) { needs.user(); needs.pretender((server, helper) => { server.post("/u/second_factors.json", () => { return helper.response({ success: "OK", - password_required: "true", + unconfirmed_session: "true", }); }); }); @@ -30,22 +30,10 @@ acceptance("Enforce Second Factor", function (needs) { await visit("/u/eviltrout/preferences/second-factor"); this.siteSettings.enforce_second_factor = "staff"; - await catchAbortedTransition(); - assert.strictEqual( currentRouteName(), - "preferences.second-factor", - "it will not transition from second-factor preferences" - ); - - await click( - ".sidebar-section[data-section-name='community'] .sidebar-section-link[data-link-name='admin']" - ); - - assert.strictEqual( - currentRouteName(), - "preferences.second-factor", - "it stays at second-factor preferences" + "preferences.security", + "it transitions to security preferences" ); }); @@ -55,26 +43,10 @@ acceptance("Enforce Second Factor", function (needs) { await visit("/u/eviltrout/preferences/second-factor"); this.siteSettings.enforce_second_factor = "all"; - await catchAbortedTransition(); - assert.strictEqual( currentRouteName(), - "preferences.second-factor", - "it will not transition from second-factor preferences" - ); - - await click( - ".sidebar-section[data-section-name='community'] .sidebar-more-section-links-details-summary" - ); - - await click( - ".sidebar-section[data-section-name='community'] .sidebar-section-link[data-link-name='about']" - ); - - assert.strictEqual( - currentRouteName(), - "preferences.second-factor", - "it stays at second-factor preferences" + "preferences.security", + "it will transition to security preferences" ); }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-second-factor-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-second-factor-test.js index 384109ee142..cae9e17598e 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-second-factor-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-second-factor-test.js @@ -1,4 +1,4 @@ -import { click, fillIn, visit } from "@ember/test-helpers"; +import { click, currentRouteName, fillIn, visit } from "@ember/test-helpers"; import { test } from "qunit"; import { acceptance, @@ -14,7 +14,6 @@ acceptance("User Preferences - Second Factor", function (needs) { server.post("/u/second_factors.json", () => { return helper.response({ success: "OK", - password_required: "true", totps: [{ id: 1, name: "one of them" }], security_keys: [{ id: 2, name: "key" }], }); @@ -57,12 +56,6 @@ acceptance("User Preferences - Second Factor", function (needs) { test("second factor totp", async function (assert) { await visit("/u/eviltrout/preferences/second-factor"); - assert.ok(exists("#password"), "it has a password input"); - - await fillIn("#password", "secrets"); - await click(".user-preferences .btn-primary"); - assert.notOk(exists("#password"), "it hides the password input"); - await click(".new-totp"); assert.ok(exists(".qr-code img"), "shows qr code image"); @@ -82,12 +75,6 @@ acceptance("User Preferences - Second Factor", function (needs) { test("second factor security keys", async function (assert) { await visit("/u/eviltrout/preferences/second-factor"); - assert.ok(exists("#password"), "it has a password input"); - - await fillIn("#password", "secrets"); - await click(".user-preferences .btn-primary"); - assert.notOk(exists("#password"), "it hides the password input"); - await click(".new-security-key"); assert.ok(exists("#security-key-name"), "shows security key name input"); @@ -109,10 +96,6 @@ acceptance("User Preferences - Second Factor", function (needs) { updateCurrentUser({ moderator: false, admin: false, trust_level: 1 }); await visit("/u/eviltrout/preferences/second-factor"); - assert.ok(exists("#password"), "it has a password input"); - - await fillIn("#password", "secrets"); - await click(".user-preferences .btn-primary"); await click(".token-based-auth-dropdown .select-kit-header"); await click("li[data-name='Disable']"); @@ -147,3 +130,29 @@ acceptance("User Preferences - Second Factor", function (needs) { ); }); }); + +acceptance( + "User Preferences - Second Factor - Unconfirmed Session", + function (needs) { + needs.user(); + + needs.pretender((server, helper) => { + server.post("/u/second_factors.json", () => { + return helper.response({ + success: "OK", + unconfirmed_session: "true", + }); + }); + }); + + test("redirects to security preferences", async function (assert) { + await visit("/u/eviltrout/preferences/second-factor"); + + assert.strictEqual( + currentRouteName(), + "preferences.security", + "it transitions to security preferences" + ); + }); + } +); diff --git a/app/assets/stylesheets/common/base/user.scss b/app/assets/stylesheets/common/base/user.scss index 9fa9aa632f8..bf35dd30645 100644 --- a/app/assets/stylesheets/common/base/user.scss +++ b/app/assets/stylesheets/common/base/user.scss @@ -518,6 +518,23 @@ } .user-preferences { + .form-vertical { + width: 500px; + max-width: 100%; + + .control-group { + margin-bottom: 2em; + } + } + + .category-selector, + .tag-chooser, + textarea, + input.user-selector, + .user-chooser { + width: 100%; + } + textarea { height: 100px; } @@ -735,6 +752,10 @@ } } } + + .pref-second-factor { + margin-top: 0.5em; + } } .paginated-topics-list { diff --git a/app/assets/stylesheets/desktop/user.scss b/app/assets/stylesheets/desktop/user.scss index 9582d19a52b..a3da4e45f7a 100644 --- a/app/assets/stylesheets/desktop/user.scss +++ b/app/assets/stylesheets/desktop/user.scss @@ -107,10 +107,6 @@ } } -.pref-second-factor { - margin-top: 10px; -} - .invite-controls .btn { margin-right: 0px; } @@ -252,21 +248,6 @@ table.user-invite-list { margin-right: 0.2em; } -.user-preferences { - .form-vertical { - width: 500px; - max-width: 100%; - } - - .category-selector, - .tag-chooser, - textarea, - input.user-selector, - .user-chooser { - width: 100%; - } -} - .user-crawler { .username { margin-left: 5px; diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 1b8280a5b79..968911f2a01 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1533,12 +1533,6 @@ class UsersController < ApplicationController raise Discourse::NotFound end - if params[:password].present? - if !confirm_secure_session - return render json: failed_json.merge(error: I18n.t("login.incorrect_password")) - end - end - if secure_session_confirmed? totp_second_factors = current_user @@ -1555,7 +1549,7 @@ class UsersController < ApplicationController render json: success_json.merge(totps: totp_second_factors, security_keys: security_keys) else - render json: success_json.merge(password_required: true) + render json: success_json.merge(unconfirmed_session: true) end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 43d643687f6..d18a140be92 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1463,8 +1463,6 @@ en: title: "Two-Factor Authentication" enable: "Manage Two-Factor Authentication" disable_all: "Disable All" - forgot_password: "Forgot password?" - confirm_password_description: "Please confirm your password to continue" name: "Name" label: "Code" rate_limit: "Please wait before trying another authentication code." diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index d3fb3ef31ce..caa11464358 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -6322,55 +6322,40 @@ RSpec.describe UsersController do SiteSetting.enable_discourse_connect = false end - context "when the password parameter is not provided" do - let(:password) { "" } + context "when the session is unconfirmed" do + it "returns unconfirmed session response" do + post "/u/second_factors.json" - before { post "/u/second_factors.json", params: { password: password } } - - it "returns password required response" do expect(response.status).to eq(200) response_body = response.parsed_body - expect(response_body["password_required"]).to eq(true) + expect(response_body["unconfirmed_session"]).to eq(true) end end - context "when the password is provided" do - fab!(:user) { Fabricate(:user, password: "8555039dd212cc66ec68") } + context "when the session is confirmed" do + fab!(:user) { Fabricate(:user, password: "acoolpassword") } - context "when the password is correct" do - let(:password) { "8555039dd212cc66ec68" } - - it "returns a list of enabled totps and security_key second factors" do - totp_second_factor = Fabricate(:user_second_factor_totp, user: user) - security_key_second_factor = - Fabricate( - :user_security_key, - user: user, - factor_type: UserSecurityKey.factor_types[:second_factor], - ) - - post "/u/second_factors.json", params: { password: password } - - expect(response.status).to eq(200) - response_body = response.parsed_body - expect(response_body["totps"].map { |second_factor| second_factor["id"] }).to include( - totp_second_factor.id, + it "returns a list of enabled totps and security_key second factors" do + totp_second_factor = Fabricate(:user_second_factor_totp, user: user) + security_key_second_factor = + Fabricate( + :user_security_key, + user: user, + factor_type: UserSecurityKey.factor_types[:second_factor], ) - expect( - response_body["security_keys"].map { |second_factor| second_factor["id"] }, - ).to include(security_key_second_factor.id) - end - end - context "when the password is not correct" do - let(:password) { "wrongpassword" } + post "/u/confirm-session.json", params: { password: "acoolpassword" } - it "returns the incorrect password response" do - post "/u/second_factors.json", params: { password: password } + post "/u/second_factors.json" - response_body = response.parsed_body - expect(response_body["error"]).to eq(I18n.t("login.incorrect_password")) - end + expect(response.status).to eq(200) + response_body = response.parsed_body + expect(response_body["totps"].map { |second_factor| second_factor["id"] }).to include( + totp_second_factor.id, + ) + expect( + response_body["security_keys"].map { |second_factor| second_factor["id"] }, + ).to include(security_key_second_factor.id) end end end diff --git a/spec/system/page_objects/pages/user_preferences_security.rb b/spec/system/page_objects/pages/user_preferences_security.rb index 8b50bc1d999..3253a22f795 100644 --- a/spec/system/page_objects/pages/user_preferences_security.rb +++ b/spec/system/page_objects/pages/user_preferences_security.rb @@ -9,10 +9,10 @@ module PageObjects end def visit_second_factor(password) - click_link(class: "btn-second-factor") + click_button "Manage Two-Factor Authentication" - find(".second-factor input#password").fill_in(with: password) - find(".second-factor .btn-primary").click + find(".dialog-body input#password").fill_in(with: password) + find(".dialog-body .btn-primary").click end end end