From 8c71878ff504b04695dcd0954a3d3e028af26d32 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Fri, 4 Mar 2022 06:43:06 +0300 Subject: [PATCH] UX: Add description to the 2FA page when adding new admins (#16098) This PR adds an extra description to the 2FA page when granting a user admin access. It also introduces a general system for adding customized descriptions that can be used by future actions. (Follow-up to dd6ec65061e747442f3d2ab0eb60764e88eff2d8) --- .../app/controllers/second-factor-auth.js | 1 + .../app/templates/second-factor-auth.hbs | 3 +++ .../acceptance/second-factor-auth-test.js | 19 +++++++++++++++++++ app/controllers/session_controller.rb | 3 +++ config/locales/server.en.yml | 3 +++ lib/second_factor/actions/grant_admin.rb | 7 ++++++- lib/second_factor/auth_manager.rb | 6 ++++++ .../second_factor/actions/grant_admin_spec.rb | 8 +++++++- spec/lib/second_factor/auth_manager_spec.rb | 3 ++- spec/requests/session_controller_spec.rb | 1 + spec/support/test_second_factor_action.rb | 3 ++- 11 files changed, 53 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js b/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js index 76d516dab84..996e5f06ff9 100644 --- a/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js +++ b/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js @@ -27,6 +27,7 @@ export default Controller.extend({ backupCodesEnabled: readOnly("model.backup_enabled"), securityKeysEnabled: readOnly("model.security_keys_enabled"), allowedMethods: readOnly("model.allowed_methods"), + customDescription: readOnly("model.description"), showTotpForm: equal("shownSecondFactorMethod", TOTP), showSecurityKeyForm: equal("shownSecondFactorMethod", SECURITY_KEY), diff --git a/app/assets/javascripts/discourse/app/templates/second-factor-auth.hbs b/app/assets/javascripts/discourse/app/templates/second-factor-auth.hbs index 2d5ab7bf692..606879871a1 100644 --- a/app/assets/javascripts/discourse/app/templates/second-factor-auth.hbs +++ b/app/assets/javascripts/discourse/app/templates/second-factor-auth.hbs @@ -3,6 +3,9 @@ {{/if}} {{#unless loadError}}

{{secondFactorTitle}}

+ {{#if customDescription}} +

{{customDescription}}

+ {{/if}}

{{secondFactorDescription}}

{{#if showSecurityKeyForm}}
diff --git a/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js b/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js index f1c9196597a..68d122db3a5 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js @@ -46,6 +46,15 @@ const RESPONSES = { }, }; +Object.keys(RESPONSES).forEach((k) => { + if (k.startsWith("ok")) { + const response = RESPONSES[k]; + if (!response.description) { + response.description = + "This is an additional description that can be customized per action"; + } + } +}); const WRONG_TOTP = "124323"; let callbackCount = 0; @@ -254,6 +263,16 @@ acceptance("Second Factor Auth Page", function (needs) { ); }); + test("2FA action description", async function (assert) { + await visit("/session/2fa?nonce=ok111111"); + + assert.equal( + query(".action-description").textContent.trim(), + "This is an additional description that can be customized per action", + "action description is rendered on the page" + ); + }); + test("error when submitting 2FA form", async function (assert) { await visit("/session/2fa?nonce=ok110111"); await fillIn("form.totp-token .second-factor-token-input", WRONG_TOTP); diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 9a35400ab22..585002b405e 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -456,6 +456,9 @@ class SessionController < ApplicationController else json[:security_keys_enabled] = false end + if challenge[:description] + json[:description] = challenge[:description] + end else json[:error] = I18n.t(error_key) end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 70f46f48cf6..fa7ecdf317e 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2583,6 +2583,9 @@ en: challenge_not_found: "Couldn't find a 2FA challenge in your current session." challenge_expired: "Too much time has passed since the 2FA challenge was staged and it's no longer valid. Please try again." challenge_not_completed: "You've not completed the 2FA challenge to perform this action. Please complete the 2FA challenge and try again." + actions: + grant_admin: + description: "For additional security measures, you need to confirm your 2FA before %{username} is granted admin access." admin: email: sent_test: "sent!" diff --git a/lib/second_factor/actions/grant_admin.rb b/lib/second_factor/actions/grant_admin.rb index 3d0641a9fe4..563a3b0ace0 100644 --- a/lib/second_factor/actions/grant_admin.rb +++ b/lib/second_factor/actions/grant_admin.rb @@ -9,9 +9,14 @@ module SecondFactor::Actions def second_factor_auth_required!(params) user = find_user(params[:user_id]) + description = I18n.t( + "second_factor_auth.actions.grant_admin.description", + username: "@#{user.username}" + ) { callback_params: { user_id: user.id }, - redirect_path: admin_user_show_path(id: user.id, username: user.username) + redirect_path: admin_user_show_path(id: user.id, username: user.username), + description: description } end diff --git a/lib/second_factor/auth_manager.rb b/lib/second_factor/auth_manager.rb index 689973ba1f2..68e481f513f 100644 --- a/lib/second_factor/auth_manager.rb +++ b/lib/second_factor/auth_manager.rb @@ -52,6 +52,9 @@ the following methods: redirected to after the action is finished. When this key is omitted, the redirect path is set to the homepage (/). + :description => optional action-specific description message that's shown on + the 2FA page. + After this method is called, the auth manager will send a 403 response with a JSON body. It does that by raising an exception that's then rescued by a `rescue_from` handler. The JSON response contains a challenge nonce which the @@ -169,6 +172,9 @@ class SecondFactor::AuthManager allowed_methods: allowed_methods.to_a, generated_at: Time.zone.now.to_i } + if config[:description] + challenge[:description] = config[:description] + end secure_session["current_second_factor_auth_challenge"] = challenge.to_json nonce end diff --git a/spec/lib/second_factor/actions/grant_admin_spec.rb b/spec/lib/second_factor/actions/grant_admin_spec.rb index f83fa4dfab1..9591f5941aa 100644 --- a/spec/lib/second_factor/actions/grant_admin_spec.rb +++ b/spec/lib/second_factor/actions/grant_admin_spec.rb @@ -40,11 +40,17 @@ describe SecondFactor::Actions::GrantAdmin do end describe "#second_factor_auth_required!" do - it "returns a hash with callback_params and redirect_path" do + it "returns a hash with callback_params, redirect_path and a description" do instance = create_instance(admin) hash = instance.second_factor_auth_required!(params({ user_id: user.id })) expect(hash[:callback_params]).to eq({ user_id: user.id }) expect(hash[:redirect_path]).to eq("/admin/users/#{user.id}/#{user.username}") + expect(hash[:description]).to eq( + I18n.t( + "second_factor_auth.actions.grant_admin.description", + username: "@#{user.username}" + ) + ) end it "ensures the acting user is admin" do diff --git a/spec/lib/second_factor/auth_manager_spec.rb b/spec/lib/second_factor/auth_manager_spec.rb index a0d3817d891..0b33219ab8b 100644 --- a/spec/lib/second_factor/auth_manager_spec.rb +++ b/spec/lib/second_factor/auth_manager_spec.rb @@ -94,7 +94,7 @@ describe SecondFactor::AuthManager do action .expects(:second_factor_auth_required!) .with({ expect_me: 131 }) - .returns({ callback_params: { call_me_back: 4314 }, redirect_path: "/gg" }) + .returns({ callback_params: { call_me_back: 4314 }, redirect_path: "/gg", description: "hello world!" }) .once action.expects(:second_factor_auth_completed!).never manager = create_manager(action) @@ -114,6 +114,7 @@ describe SecondFactor::AuthManager do expect(challenge[:redirect_path]).to eq("/gg") expect(challenge[:allowed_methods]).to eq(manager.allowed_methods.to_a) expect(challenge[:callback_params]).to eq({ call_me_back: 4314 }) + expect(challenge[:description]).to eq("hello world!") end it "sets the redirect_path to the root path if second_factor_auth_required! doesn't specify a redirect_path" do diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index c07daa9efd5..842cacc454b 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -2305,6 +2305,7 @@ describe SessionController do UserSecondFactor.methods[:totp], UserSecondFactor.methods[:security_key], ) + expect(challenge_data["description"]).to eq("this is description for test action") Fabricate( :user_security_key_with_random_credential, diff --git a/spec/support/test_second_factor_action.rb b/spec/support/test_second_factor_action.rb index 713a7f01481..58da03ab7ba 100644 --- a/spec/support/test_second_factor_action.rb +++ b/spec/support/test_second_factor_action.rb @@ -10,7 +10,8 @@ class TestSecondFactorAction < SecondFactor::Actions::Base callback_params: { saved_param_1: params[:saved_param_1], saved_param_2: params[:saved_param_2] - } + }, + description: "this is description for test action" } end