From dd6ec65061e747442f3d2ab0eb60764e88eff2d8 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Thu, 17 Feb 2022 12:12:59 +0300 Subject: [PATCH] FEATURE: Centralized 2FA page (#15377) 2FA support in Discourse was added and grown gradually over the years: we first added support for TOTP for logins, then we implemented backup codes, and last but not least, security keys. 2FA usage was initially limited to logging in, but it has been expanded and we now require 2FA for risky actions such as adding a new admin to the site. As a result of this gradual growth of the 2FA system, technical debt has accumulated to the point where it has become difficult to require 2FA for more actions. We now have 5 different 2FA UI implementations and each one has to support all 3 2FA methods (TOTP, backup codes, and security keys) which makes it difficult to maintain a consistent UX for these different implementations. Moreover, there is a lot of repeated logic in the server-side code behind these 5 UI implementations which hinders maintainability even more. This commit is the first step towards repaying the technical debt: it builds a system that centralizes as much as possible of the 2FA server-side logic and UI. The 2 main components of this system are: 1. A dedicated page for 2FA with support for all 3 methods. 2. A reusable server-side class that centralizes the 2FA logic (the `SecondFactor::AuthManager` class). From a top-level view, the 2FA flow in this new system looks like this: 1. User initiates an action that requires 2FA; 2. Server is aware that 2FA is required for this action, so it redirects the user to the 2FA page if the user has a 2FA method, otherwise the action is performed. 3. User submits the 2FA form on the page; 4. Server validates the 2FA and if it's successful, the action is performed and the user is redirected to the previous page. A more technically-detailed explanation/documentation of the new system is available as a comment at the top of the `lib/second_factor/auth_manager.rb` file. Please note that the details are not set in stone and will likely change in the future, so please don't use the system in your plugins yet. Since this is a new system that needs to be tested, we've decided to migrate only the 2FA for adding a new admin to the new system at this time (in this commit). Our plan is to gradually migrate the remaining 2FA implementations to the new system. For screenshots of the 2FA page, see PR #15377 on GitHub. --- .../addon/controllers/admin-user-index.js | 12 +- .../app/components/second-factor-input.js | 8 + .../app/controllers/second-factor-auth.js | 232 +++++++++++++++ .../discourse/app/routes/app-route-map.js | 1 + .../app/routes/second-factor-auth.js | 37 +++ .../components/second-factor-input.hbs | 4 +- .../app/templates/second-factor-auth.hbs | 48 +++ .../acceptance/second-factor-auth-test.js | 279 ++++++++++++++++++ app/controllers/admin/users_controller.rb | 22 +- app/controllers/application_controller.rb | 24 ++ app/controllers/session_controller.rb | 111 +++++++ config/locales/client.en.yml | 3 + config/locales/server.en.yml | 4 + config/routes.rb | 7 +- lib/admin_confirmation.rb | 1 - lib/global_path.rb | 1 + lib/second_factor/actions/base.rb | 25 ++ lib/second_factor/actions/grant_admin.rb | 33 +++ lib/second_factor/auth_manager.rb | 201 +++++++++++++ lib/second_factor/auth_manager_result.rb | 25 ++ lib/second_factor/bad_challenge.rb | 10 + .../second_factor/actions/grant_admin_spec.rb | 80 +++++ spec/lib/second_factor/auth_manager_spec.rb | 225 ++++++++++++++ spec/requests/admin/users_controller_spec.rb | 110 ++++++- spec/requests/session_controller_spec.rb | 160 ++++++++++ .../session_controller_helper_routes.rb | 20 ++ spec/support/test_second_factor_action.rb | 19 ++ 27 files changed, 1673 insertions(+), 29 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/controllers/second-factor-auth.js create mode 100644 app/assets/javascripts/discourse/app/routes/second-factor-auth.js create mode 100644 app/assets/javascripts/discourse/app/templates/second-factor-auth.hbs create mode 100644 app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js create mode 100644 lib/second_factor/actions/base.rb create mode 100644 lib/second_factor/actions/grant_admin.rb create mode 100644 lib/second_factor/auth_manager.rb create mode 100644 lib/second_factor/auth_manager_result.rb create mode 100644 lib/second_factor/bad_challenge.rb create mode 100644 spec/lib/second_factor/actions/grant_admin_spec.rb create mode 100644 spec/lib/second_factor/auth_manager_spec.rb create mode 100644 spec/support/session_controller_helper_routes.rb create mode 100644 spec/support/test_second_factor_action.rb diff --git a/app/assets/javascripts/admin/addon/controllers/admin-user-index.js b/app/assets/javascripts/admin/addon/controllers/admin-user-index.js index fdd21f7332e..c11df41a5f9 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-user-index.js +++ b/app/assets/javascripts/admin/addon/controllers/admin-user-index.js @@ -16,6 +16,7 @@ import { inject as service } from "@ember/service"; import showModal from "discourse/lib/show-modal"; export default Controller.extend(CanCheckEmails, { + router: service(), adminTools: service(), originalPrimaryGroupId: null, customGroupIdsBuffer: null, @@ -228,7 +229,16 @@ export default Controller.extend(CanCheckEmails, { controller.setResult(result); } }) - .catch(popupAjaxError); + .catch((error) => { + const nonce = error.jqXHR?.responseJSON.second_factor_challenge_nonce; + if (nonce) { + this.router.transitionTo("second-factor-auth", { + queryParams: { nonce }, + }); + } else { + popupAjaxError(error); + } + }); }, revokeModeration() { return this.model.revokeModeration(); diff --git a/app/assets/javascripts/discourse/app/components/second-factor-input.js b/app/assets/javascripts/discourse/app/components/second-factor-input.js index b1da9e9d0ea..aa149d67660 100644 --- a/app/assets/javascripts/discourse/app/components/second-factor-input.js +++ b/app/assets/javascripts/discourse/app/components/second-factor-input.js @@ -1,6 +1,7 @@ import Component from "@ember/component"; import { SECOND_FACTOR_METHODS } from "discourse/models/user"; import discourseComputed from "discourse-common/utils/decorators"; +import { action } from "@ember/object"; export default Component.extend({ @discourseComputed("secondFactorMethod") @@ -32,4 +33,11 @@ export default Component.extend({ return "32"; } }, + + @action + onInput() { + if (this.onTokenInput) { + this.onTokenInput(...arguments); + } + }, }); diff --git a/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js b/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js new file mode 100644 index 00000000000..76d516dab84 --- /dev/null +++ b/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js @@ -0,0 +1,232 @@ +import Controller from "@ember/controller"; +import { SECOND_FACTOR_METHODS } from "discourse/models/user"; +import I18n from "I18n"; +import { ajax } from "discourse/lib/ajax"; +import { extractError } from "discourse/lib/ajax-error"; +import { action } from "@ember/object"; +import discourseComputed from "discourse-common/utils/decorators"; +import { equal, readOnly } from "@ember/object/computed"; +import { getWebauthnCredential } from "discourse/lib/webauthn"; +import DiscourseURL from "discourse/lib/url"; + +const { TOTP, BACKUP_CODE, SECURITY_KEY } = SECOND_FACTOR_METHODS; +export default Controller.extend({ + TOTP, + BACKUP_CODE, + SECURITY_KEY, + + queryParams: ["nonce"], + + message: null, + loadError: false, + messageIsError: false, + secondFactorToken: null, + userSelectedMethod: null, + + totpEnabled: readOnly("model.totp_enabled"), + backupCodesEnabled: readOnly("model.backup_enabled"), + securityKeysEnabled: readOnly("model.security_keys_enabled"), + allowedMethods: readOnly("model.allowed_methods"), + + showTotpForm: equal("shownSecondFactorMethod", TOTP), + showSecurityKeyForm: equal("shownSecondFactorMethod", SECURITY_KEY), + showBackupCodesForm: equal("shownSecondFactorMethod", BACKUP_CODE), + + @discourseComputed("allowedMethods.[]", "totpEnabled") + totpAvailable() { + return this.totpEnabled && this.allowedMethods.includes(TOTP); + }, + + @discourseComputed("allowedMethods.[]", "backupCodesEnabled") + backupCodesAvailable() { + return this.backupCodesEnabled && this.allowedMethods.includes(BACKUP_CODE); + }, + + @discourseComputed("allowedMethods.[]", "securityKeysEnabled") + securityKeysAvailable() { + return ( + this.securityKeysEnabled && this.allowedMethods.includes(SECURITY_KEY) + ); + }, + + @discourseComputed( + "userSelectedMethod", + "securityKeysAvailable", + "totpAvailable", + "backupCodesAvailable" + ) + shownSecondFactorMethod( + userSelectedMethod, + securityKeysAvailable, + totpAvailable, + backupCodesAvailable + ) { + if (userSelectedMethod !== null) { + return userSelectedMethod; + } else { + if (securityKeysAvailable) { + return SECURITY_KEY; + } else if (totpAvailable) { + return TOTP; + } else if (backupCodesAvailable) { + return BACKUP_CODE; + } else { + throw new Error("unpexected state of user 2fa settings!"); + } + } + }, + + @discourseComputed( + "shownSecondFactorMethod", + "securityKeysAvailable", + "totpAvailable", + "backupCodesAvailable" + ) + alternativeMethods( + shownSecondFactorMethod, + securityKeysAvailable, + totpAvailable, + backupCodesAvailable + ) { + const alts = []; + if (securityKeysAvailable && shownSecondFactorMethod !== SECURITY_KEY) { + alts.push({ + id: SECURITY_KEY, + translationKey: "login.second_factor_toggle.security_key", + class: "security-key", + }); + } + + if (totpAvailable && shownSecondFactorMethod !== TOTP) { + alts.push({ + id: TOTP, + translationKey: "login.second_factor_toggle.totp", + class: "totp", + }); + } + + if (backupCodesAvailable && shownSecondFactorMethod !== BACKUP_CODE) { + alts.push({ + id: BACKUP_CODE, + translationKey: "login.second_factor_toggle.backup_code", + class: "backup-code", + }); + } + + return alts; + }, + + @discourseComputed("shownSecondFactorMethod") + secondFactorTitle(shownSecondFactorMethod) { + switch (shownSecondFactorMethod) { + case TOTP: + return I18n.t("login.second_factor_title"); + case SECURITY_KEY: + return I18n.t("login.second_factor_title"); + case BACKUP_CODE: + return I18n.t("login.second_factor_backup_title"); + } + }, + + @discourseComputed("shownSecondFactorMethod") + secondFactorDescription(shownSecondFactorMethod) { + switch (shownSecondFactorMethod) { + case TOTP: + return I18n.t("login.second_factor_description"); + case SECURITY_KEY: + return I18n.t("login.security_key_description"); + case BACKUP_CODE: + return I18n.t("login.second_factor_backup_description"); + } + }, + + @discourseComputed("messageIsError") + alertClass(messageIsError) { + if (messageIsError) { + return "alert-error"; + } else { + return "alert-success"; + } + }, + + @discourseComputed("showTotpForm", "showBackupCodesForm") + inputFormClass(showTotpForm, showBackupCodesForm) { + if (showTotpForm) { + return "totp-token"; + } else if (showBackupCodesForm) { + return "backup-code-token"; + } + }, + + resetState() { + this.set("message", null); + this.set("messageIsError", false); + this.set("secondFactorToken", null); + this.set("userSelectedMethod", null); + this.set("loadError", false); + }, + + displayError(message) { + this.set("message", message); + this.set("messageIsError", true); + }, + + displaySuccess(message) { + this.set("message", message); + this.set("messageIsError", false); + }, + + verifySecondFactor(data) { + return ajax("/session/2fa", { + type: "POST", + data: { + ...data, + second_factor_method: this.shownSecondFactorMethod, + nonce: this.nonce, + }, + }) + .then((response) => { + this.displaySuccess( + I18n.t("second_factor_auth.redirect_after_success") + ); + ajax(response.callback_path, { + type: response.callback_method, + data: { second_factor_nonce: this.nonce }, + }) + .then(() => DiscourseURL.routeTo(response.redirect_path)) + .catch((error) => this.displayError(extractError(error))); + }) + .catch((error) => { + this.displayError(extractError(error)); + }); + }, + + @action + onTokenInput(event) { + this.set("secondFactorToken", event.target.value); + }, + + @action + useAnotherMethod(newMethod) { + this.set("userSelectedMethod", newMethod); + }, + + @action + authenticateSecurityKey() { + getWebauthnCredential( + this.model.challenge, + this.model.allowed_credential_ids, + (credentialData) => { + this.verifySecondFactor({ second_factor_token: credentialData }); + }, + (errorMessage) => { + this.displayError(errorMessage); + } + ); + }, + + @action + authenticateToken() { + this.verifySecondFactor({ second_factor_token: this.secondFactorToken }); + }, +}); diff --git a/app/assets/javascripts/discourse/app/routes/app-route-map.js b/app/assets/javascripts/discourse/app/routes/app-route-map.js index b4c778fdfac..8e7dd439170 100644 --- a/app/assets/javascripts/discourse/app/routes/app-route-map.js +++ b/app/assets/javascripts/discourse/app/routes/app-route-map.js @@ -191,6 +191,7 @@ export default function () { this.route("signup", { path: "/signup" }); this.route("login", { path: "/login" }); this.route("email-login", { path: "/session/email-login/:token" }); + this.route("second-factor-auth", { path: "/session/2fa" }); this.route("associate-account", { path: "/associate/:token" }); this.route("login-preferences"); this.route("forgot-password", { path: "/password-reset" }); diff --git a/app/assets/javascripts/discourse/app/routes/second-factor-auth.js b/app/assets/javascripts/discourse/app/routes/second-factor-auth.js new file mode 100644 index 00000000000..61d71fa5abc --- /dev/null +++ b/app/assets/javascripts/discourse/app/routes/second-factor-auth.js @@ -0,0 +1,37 @@ +import DiscourseRoute from "discourse/routes/discourse"; +import PreloadStore from "discourse/lib/preload-store"; +import { ajax } from "discourse/lib/ajax"; +import { extractError } from "discourse/lib/ajax-error"; + +export default DiscourseRoute.extend({ + queryParams: { + nonce: { refreshModel: true }, + }, + + model(params) { + if (PreloadStore.data.has("2fa_challenge_data")) { + return PreloadStore.getAndRemove("2fa_challenge_data"); + } else { + return ajax("/session/2fa.json", { + type: "GET", + data: { nonce: params.nonce }, + }).catch((errorResponse) => { + const error = extractError(errorResponse); + if (error) { + return { error }; + } else { + throw errorResponse; + } + }); + } + }, + + setupController(controller, model) { + this._super(...arguments); + controller.resetState(); + if (model.error) { + controller.displayError(model.error); + controller.set("loadError", true); + } + }, +}); diff --git a/app/assets/javascripts/discourse/app/templates/components/second-factor-input.hbs b/app/assets/javascripts/discourse/app/templates/components/second-factor-input.hbs index 6251cb0c8fa..eb841a81c99 100644 --- a/app/assets/javascripts/discourse/app/templates/components/second-factor-input.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/second-factor-input.hbs @@ -1,4 +1,5 @@ -{{text-field value=value +{{text-field + value=value type=type pattern=pattern maxlength=maxlength @@ -9,4 +10,5 @@ autocorrect="off" autofocus="autofocus" placeholder=placeholder + input=(action "onInput") }} diff --git a/app/assets/javascripts/discourse/app/templates/second-factor-auth.hbs b/app/assets/javascripts/discourse/app/templates/second-factor-auth.hbs new file mode 100644 index 00000000000..2d5ab7bf692 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/second-factor-auth.hbs @@ -0,0 +1,48 @@ +{{#if message}} +
{{message}}
+{{/if}} +{{#unless loadError}} +

{{secondFactorTitle}}

+

{{secondFactorDescription}}

+ {{#if showSecurityKeyForm}} +
+ {{d-button + action=(action "authenticateSecurityKey") + icon="key" + id="security-key-authenticate-button" + label="login.security_key_authenticate" + type="button" + class="btn btn-large btn-primary" + }} +
+ {{else if (or showTotpForm showBackupCodesForm)}} +
+ {{second-factor-input + value=secondFactorToken + secondFactorMethod=shownSecondFactorMethod + onTokenInput=(action "onTokenInput") + }} + {{d-button + action=(action "authenticateToken") + class="btn-primary" + label="submit" + type="submit" + }} +
+ {{/if}} + + {{#if alternativeMethods.length}} +

+ {{#each alternativeMethods as |method index|}} + {{#if (gt index 0)}} + · + {{/if}} + + + {{i18n method.translationKey}} + + + {{/each}} +

+ {{/if}} +{{/unless}} 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 new file mode 100644 index 00000000000..f1c9196597a --- /dev/null +++ b/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js @@ -0,0 +1,279 @@ +import { + acceptance, + exists, + query, +} from "discourse/tests/helpers/qunit-helpers"; +import { click, currentURL, fillIn, visit } from "@ember/test-helpers"; +import { SECOND_FACTOR_METHODS } from "discourse/models/user"; +import { test } from "qunit"; + +const { TOTP, BACKUP_CODE, SECURITY_KEY } = SECOND_FACTOR_METHODS; + +const RESPONSES = { + failed: { + status: 404, + error: "could not find an active challenge in your session", + }, + ok111111: { + totp_enabled: true, + backup_enabled: true, + security_keys_enabled: true, + allowed_methods: [TOTP, BACKUP_CODE, SECURITY_KEY], + }, + ok111110: { + totp_enabled: true, + backup_enabled: true, + security_keys_enabled: true, + allowed_methods: [TOTP, BACKUP_CODE], + }, + ok110111: { + totp_enabled: true, + backup_enabled: true, + security_keys_enabled: false, + allowed_methods: [TOTP, BACKUP_CODE, SECURITY_KEY], + }, + ok100111: { + totp_enabled: true, + backup_enabled: false, + security_keys_enabled: false, + allowed_methods: [TOTP, BACKUP_CODE, SECURITY_KEY], + }, + ok111010: { + totp_enabled: true, + backup_enabled: true, + security_keys_enabled: true, + allowed_methods: [BACKUP_CODE], + }, +}; + +const WRONG_TOTP = "124323"; +let callbackCount = 0; + +acceptance("Second Factor Auth Page", function (needs) { + needs.user(); + needs.pretender((server, helpers) => { + server.get("/session/2fa.json", (request) => { + const response = { ...RESPONSES[request.queryParams.nonce] }; + const status = response.status || 200; + delete response.status; + return [status, { "Content-Type": "application/json" }, response]; + }); + + server.post("/session/2fa", (request) => { + const params = helpers.parsePostData(request.requestBody); + if (params.second_factor_token === WRONG_TOTP) { + return [ + 401, + { "Content-Type": "application/json" }, + { + error: "invalid token man", + ok: false, + }, + ]; + } else { + return [ + 200, + { "Content-Type": "application/json" }, + { + ok: true, + callback_method: "PUT", + callback_path: "/callback-path", + redirect_path: "/", + }, + ]; + } + }); + + server.put("/callback-path", () => { + callbackCount++; + return [ + 200, + { "Content-Type": "application/json" }, + { + whatever: true, + }, + ]; + }); + }); + + needs.hooks.beforeEach(() => (callbackCount = 0)); + + test("when challenge data fails to load", async function (assert) { + await visit("/session/2fa?nonce=failed"); + assert.equal( + query(".alert-error").textContent, + "could not find an active challenge in your session", + "load error message is shown" + ); + }); + + test("default 2FA method", async function (assert) { + await visit("/session/2fa?nonce=ok111111"); + assert.ok( + exists("#security-key-authenticate-button"), + "security key is the default method" + ); + assert.ok( + !exists("form.totp-token"), + "totp is not shown by default when security key is allowed" + ); + assert.ok( + !exists("form.backup-code-token"), + "backup code form is not shown by default when security key is allowed" + ); + + await visit("/"); + await visit("/session/2fa?nonce=ok111110"); + assert.ok( + !exists("#security-key-authenticate-button"), + "security key method is not shown when it's not allowed" + ); + assert.ok( + exists("form.totp-token"), + "totp is the default method when security key is not allowed" + ); + assert.ok( + !exists("form.backup-code-token"), + "backup code form is not shown by default when TOTP is allowed" + ); + + await visit("/"); + await visit("/session/2fa?nonce=ok110111"); + assert.ok( + !exists("#security-key-authenticate-button"), + "security key method is not shown when it's not enabled" + ); + assert.ok( + exists("form.totp-token"), + "totp is the default method when security key is not enabled" + ); + assert.ok( + !exists("form.backup-code-token"), + "backup code form is not shown by default when TOTP is enabled" + ); + }); + + test("alternative 2FA methods", async function (assert) { + await visit("/session/2fa?nonce=ok111111"); + assert.ok( + exists(".toggle-second-factor-method.totp"), + "TOTP is shown as an alternative method if it's enabled and allowed" + ); + assert.ok( + exists(".toggle-second-factor-method.backup-code"), + "backup code is shown as an alternative method if it's enabled and allowed" + ); + assert.ok( + !exists(".toggle-second-factor-method.security-key"), + "security key is not shown as an alternative method when it's selected" + ); + + await visit("/"); + await visit("/session/2fa?nonce=ok100111"); + assert.ok( + !exists(".toggle-second-factor-method"), + "no alternative methods are shown if only 1 method is enabled" + ); + + await visit("/"); + await visit("/session/2fa?nonce=ok111010"); + assert.ok( + !exists(".toggle-second-factor-method"), + "no alternative methods are shown if only 1 method is allowed" + ); + }); + + test("switching 2FA methods", async function (assert) { + await visit("/session/2fa?nonce=ok111111"); + assert.ok( + exists("#security-key-authenticate-button"), + "security key form is shown because it's the default" + ); + assert.ok( + exists(".toggle-second-factor-method.totp"), + "TOTP is shown as an alternative method" + ); + assert.ok( + exists(".toggle-second-factor-method.backup-code"), + "backup code is shown as an alternative method" + ); + assert.ok( + !exists(".toggle-second-factor-method.security-key"), + "security key is not shown as an alternative method because it's selected" + ); + + await click(".toggle-second-factor-method.totp"); + assert.ok(exists("form.totp-token"), "TOTP form is now shown"); + assert.ok( + exists(".toggle-second-factor-method.security-key"), + "security key is now shown as alternative method" + ); + assert.ok( + exists(".toggle-second-factor-method.backup-code"), + "backup code is still shown as an alternative method" + ); + assert.ok( + !exists(".toggle-second-factor-method.totp"), + "TOTP is no longer shown as an alternative method" + ); + + await click(".toggle-second-factor-method.backup-code"); + assert.ok( + exists("form.backup-code-token"), + "backup code form is now shown" + ); + assert.ok( + exists(".toggle-second-factor-method.security-key"), + "security key is still shown as alternative method" + ); + assert.ok( + exists(".toggle-second-factor-method.totp"), + "TOTP is now shown as an alternative method" + ); + assert.ok( + !exists(".toggle-second-factor-method.backup-code"), + "backup code is no longer shown as an alternative method" + ); + + await click(".toggle-second-factor-method.security-key"); + assert.ok( + exists("#security-key-authenticate-button"), + "security key form is back" + ); + assert.ok( + !exists(".toggle-second-factor-method.security-key"), + "security key is no longer shown as alternative method" + ); + assert.ok( + exists(".toggle-second-factor-method.totp"), + "TOTP is now shown as an alternative method" + ); + assert.ok( + exists(".toggle-second-factor-method.backup-code"), + "backup code is now shown as an alternative method" + ); + }); + + 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); + await click('form.totp-token .btn-primary[type="submit"]'); + assert.equal( + query(".alert-error").textContent.trim(), + "invalid token man", + "error message from the server is displayed" + ); + }); + + test("successful 2FA form submit", async function (assert) { + await visit("/session/2fa?nonce=ok110111"); + await fillIn("form.totp-token .second-factor-token-input", "323421"); + await click('form.totp-token .btn-primary[type="submit"]'); + assert.equal( + currentURL(), + "/", + "user has been redirected to the redirect_path" + ); + assert.equal(callbackCount, 1, "callback request has been performed"); + }); +}); diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index d719db0ab88..f8263615577 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -6,7 +6,6 @@ class Admin::UsersController < Admin::AdminController :unsuspend, :log_out, :revoke_admin, - :grant_admin, :revoke_moderation, :grant_moderation, :approve, @@ -191,24 +190,11 @@ class Admin::UsersController < Admin::AdminController end def grant_admin - guardian.ensure_can_grant_admin!(@user) - if current_user.has_any_second_factor_methods_enabled? - second_factor_authentication_result = current_user.authenticate_second_factor(params, secure_session) - if second_factor_authentication_result.ok - @user.grant_admin! - StaffActionLogger.new(current_user).log_grant_admin(@user) - render json: success_json - else - failure_payload = second_factor_authentication_result.to_h - if current_user.security_keys_enabled? - Webauthn.stage_challenge(current_user, secure_session) - failure_payload.merge!(Webauthn.allowed_credentials(current_user, secure_session)) - end - render json: failed_json.merge(failure_payload) - end - else - AdminConfirmation.new(@user, current_user).create_confirmation + result = run_second_factor!(SecondFactor::Actions::GrantAdmin) + if result.no_second_factors_enabled? render json: success_json.merge(email_confirmation_required: true) + else + render json: success_json end end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dbe83dc2ef4..b4b1013b372 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -247,6 +247,16 @@ class ApplicationController < ActionController::Base end end + rescue_from SecondFactor::AuthManager::SecondFactorRequired do |e| + render json: { + second_factor_challenge_nonce: e.nonce + }, status: 403 + end + + rescue_from SecondFactor::BadChallenge do |e| + render json: { error: I18n.t(e.error_translation_key) }, status: e.status_code + end + def redirect_with_client_support(url, options) if request.xhr? response.headers['Discourse-Xhr-Redirect'] = 'true' @@ -964,4 +974,18 @@ class ApplicationController < ActionController::Base end end end + + def run_second_factor!(action_class) + action = action_class.new(guardian) + manager = SecondFactor::AuthManager.new(guardian, action) + yield(manager) if block_given? + result = manager.run!(request, params, secure_session) + + if !result.no_second_factors_enabled? && !result.second_factor_auth_completed? + # should never happen, but I want to know if somehow it does! (osama) + raise "2fa process ended up in a bad state!" + end + + result + end end diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 62264baa602..1ce588b4f92 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -6,6 +6,10 @@ class SessionController < ApplicationController skip_before_action :redirect_to_login_if_required skip_before_action :preload_json, :check_xhr, only: %i(sso sso_login sso_provider destroy one_time_password) + skip_before_action :check_xhr, only: %i(second_factor_auth_show) + + requires_login only: [:second_factor_auth_show, :second_factor_auth_perform] + ACTIVATE_USER_KEY = "activate_user" def csrf @@ -424,6 +428,113 @@ class SessionController < ApplicationController render layout: 'no_ember', locals: { hide_auth_buttons: true } end + def second_factor_auth_show + user = current_user + + nonce = params.require(:nonce) + challenge = nil + error_key = nil + status_code = 200 + begin + challenge = SecondFactor::AuthManager.find_second_factor_challenge(nonce, secure_session) + rescue SecondFactor::BadChallenge => exception + error_key = exception.error_translation_key + status_code = exception.status_code + end + + json = {} + if challenge + json.merge!( + totp_enabled: user.totp_enabled?, + backup_enabled: user.backup_codes_enabled?, + allowed_methods: challenge[:allowed_methods] + ) + if user.security_keys_enabled? + Webauthn.stage_challenge(user, secure_session) + json.merge!(Webauthn.allowed_credentials(user, secure_session)) + json[:security_keys_enabled] = true + else + json[:security_keys_enabled] = false + end + else + json[:error] = I18n.t(error_key) + end + + respond_to do |format| + format.html do + store_preloaded("2fa_challenge_data", MultiJson.dump(json)) + raise ApplicationController::RenderEmpty.new + end + + format.json do + render json: json, status: status_code + end + end + end + + def second_factor_auth_perform + nonce = params.require(:nonce) + challenge = nil + error_key = nil + status_code = 200 + begin + challenge = SecondFactor::AuthManager.find_second_factor_challenge(nonce, secure_session) + rescue SecondFactor::BadChallenge => exception + error_key = exception.error_translation_key + status_code = exception.status_code + end + + if error_key + json = failed_json.merge( + ok: false, + error: I18n.t(error_key), + reason: "challenge_not_found_or_expired" + ) + render json: failed_json.merge(json), status: status_code + return + end + + # no proper error messages for these cases because the only way they can + # happen is if someone is messing with us. + # the first one can only happen if someone disables a 2FA method after + # they're redirected to the 2fa page and then uses the same method they've + # disabled. + second_factor_method = params[:second_factor_method].to_i + if !current_user.valid_second_factor_method_for_user?(second_factor_method) + raise Discourse::InvalidAccess.new + end + # and this happens if someone tries to use a 2FA method that's not accepted + # for the action they're trying to perform. e.g. using backup codes to + # grant someone admin status. + if !challenge[:allowed_methods].include?(second_factor_method) + raise Discourse::InvalidAccess.new + end + + if !challenge[:successful] + rate_limit_second_factor!(current_user) + second_factor_auth_result = current_user.authenticate_second_factor(params, secure_session) + if second_factor_auth_result.ok + challenge[:successful] = true + challenge[:generated_at] += 1.minute.to_i + secure_session["current_second_factor_auth_challenge"] = challenge.to_json + else + error_json = second_factor_auth_result + .to_h + .deep_symbolize_keys + .slice(:ok, :error, :reason) + .merge(failed_json) + render json: error_json, status: 400 + return + end + end + render json: { + ok: true, + callback_method: challenge[:callback_method], + callback_path: challenge[:callback_path], + redirect_path: challenge[:redirect_path] + }, status: 200 + end + def forgot_password params.require(:login) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index a778b2e6763..6ed1e901691 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1980,6 +1980,7 @@ en: second_factor_toggle: totp: "Use an authenticator app instead" backup_code: "Use a backup code instead" + security_key: "Use a security key instead" invites: accept_title: "Invitation" emoji: "envelope emoji" @@ -3975,6 +3976,8 @@ en: fullscreen_table: expand_btn: "Expand Table" + second_factor_auth: + redirect_after_success: "Second factor authentication is successful. Redirecting to the previous page…" # This section is exported to the javascript for i18n in the admin section admin_js: type_to_filter: "type to filter..." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 18abca279ca..793305a4d90 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2576,6 +2576,10 @@ en: totp: "Use an authenticator app or security key instead" backup_code: "Use a backup code instead" + second_factor_auth: + 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." admin: email: sent_test: "sent!" diff --git a/config/routes.rb b/config/routes.rb index f2733e9abe5..2c4aa4eae54 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -146,7 +146,7 @@ Discourse::Application.routes.draw do delete "sso_record" end get "users/:id.json" => 'users#show', defaults: { format: 'json' } - get 'users/:id/:username' => 'users#show', constraints: { username: RouteFormat.username } + get 'users/:id/:username' => 'users#show', constraints: { username: RouteFormat.username }, as: :user_show get 'users/:id/:username/badges' => 'users#show' get 'users/:id/:username/tl3_requirements' => 'users#show' @@ -371,6 +371,11 @@ Discourse::Application.routes.draw do post "session/email-login/:token" => "session#email_login" get "session/otp/:token" => "session#one_time_password", constraints: { token: /[0-9a-f]+/ } post "session/otp/:token" => "session#one_time_password", constraints: { token: /[0-9a-f]+/ } + get "session/2fa" => "session#second_factor_auth_show" + post "session/2fa" => "session#second_factor_auth_perform" + if Rails.env.test? + post "session/2fa/test-action" => "session#test_second_factor_restricted_route" + end get "composer_messages" => "composer_messages#index" resources :static diff --git a/lib/admin_confirmation.rb b/lib/admin_confirmation.rb index 28f305b5ad2..fa73f66de70 100644 --- a/lib/admin_confirmation.rb +++ b/lib/admin_confirmation.rb @@ -58,5 +58,4 @@ class AdminConfirmation ac.token = token ac end - end diff --git a/lib/global_path.rb b/lib/global_path.rb index 79147568407..a2f956476da 100644 --- a/lib/global_path.rb +++ b/lib/global_path.rb @@ -31,4 +31,5 @@ module GlobalPath uri.to_s end + extend self end diff --git a/lib/second_factor/actions/base.rb b/lib/second_factor/actions/base.rb new file mode 100644 index 00000000000..d55f2b560da --- /dev/null +++ b/lib/second_factor/actions/base.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module SecondFactor::Actions + class Base + include Rails.application.routes.url_helpers + attr_reader :current_user, :guardian + + def initialize(guardian) + @guardian = guardian + @current_user = guardian.user + end + + def no_second_factors_enabled!(params) + raise NotImplementedError.new + end + + def second_factor_auth_required!(params) + raise NotImplementedError.new + end + + def second_factor_auth_completed!(callback_params) + raise NotImplementedError.new + end + end +end diff --git a/lib/second_factor/actions/grant_admin.rb b/lib/second_factor/actions/grant_admin.rb new file mode 100644 index 00000000000..3d0641a9fe4 --- /dev/null +++ b/lib/second_factor/actions/grant_admin.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module SecondFactor::Actions + class GrantAdmin < Base + def no_second_factors_enabled!(params) + user = find_user(params[:user_id]) + AdminConfirmation.new(user, current_user).create_confirmation + end + + def second_factor_auth_required!(params) + user = find_user(params[:user_id]) + { + callback_params: { user_id: user.id }, + redirect_path: admin_user_show_path(id: user.id, username: user.username) + } + end + + def second_factor_auth_completed!(callback_params) + user = find_user(callback_params[:user_id]) + user.grant_admin! + StaffActionLogger.new(current_user).log_grant_admin(user) + end + + private + + def find_user(id) + user = User.find_by(id: id) + raise Discourse::NotFound if !user + guardian.ensure_can_grant_admin!(user) + user + end + end +end diff --git a/lib/second_factor/auth_manager.rb b/lib/second_factor/auth_manager.rb new file mode 100644 index 00000000000..689973ba1f2 --- /dev/null +++ b/lib/second_factor/auth_manager.rb @@ -0,0 +1,201 @@ +# frozen_string_literal: true + +=begin +This class is responsible for managing any actions that require second factor +authentication before a user is allowed to perform them. Such actions include +granting another user admin access, changing password and signing in. In a more +technical sense, an action is the logic encapsulated in a Rails controller +action without the logic related to 2fa enforcement/handling. + +When a user attempts to perform a 2fa-protected action, there are 3 possible +outcomes: + +1. the user doesn't have any suitable 2fa methods enabled, so they should be +allowed to perform the action right away. + +2. the user has a suitable 2fa method enabled, in which case there are 2 +possibilities: + a. the user hasn't done 2fa for the action so they should be redirected to + the 2fa page and complete the 2fa before they are allowed to proceed. + b. the user has done 2fa for the action so they should be allowed to perform + the action. + +This class, the auth manager, contains the logic for deciding which outcome +should happen and performing it. + +To use the auth manager for requiring 2fa for an action, it needs to be invoked +from the controller action using the `run_second_factor!` method which is +available in all controllers. This method takes a single argument which is a +class that inherits from the `SecondFactor::Actions::Base` class and implements +the following methods: + +1. no_second_factors_enabled!(params): + This method corresponds to outcome (1) above, i.e. it's called when the user + performing the action has no suitable 2fa method enabled. It receives the + request params of the controller action. Return value is insignificant. + +2. second_factor_auth_required!(params): + This method corresponds to outcome (2a) above. It also receives the request + params of the controller action. The purpose of this method is to keep track + of the params that are needed to perform the action and where they should be + redirected after the user completes the 2fa. + + To communicate this information to the auth manager, the return value of this + method is utilized for this purpose. This method must return a Hash that + should have 2 keys: + + :callback_params => another Hash containing the params that are needed to + finish the action once 2fa is completed. Everything in this Hash must be + serializable to JSON. + + :redirect_path => relative subfolder-aware path that the user should be + redirected to after the action is finished. When this key is omitted, the + redirect path is set to the homepage (/). + + 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 + client/frontend will need to complete the 2fa. More on this later. + +3. second_factor_auth_completed!(callback_params): + This method corresponds to outcome (2b) above. It's called after the user has + successfully completed the 2fa for the 2fa-protected action and the purpose + of this method is to actually perform that action. + + The `callback_params` param of this method is the `callback_params` Hash from + the return value of the previous method. + +If there are permission/security checks that the current user must pass in +order to perform the 2fa-protected action, it's important to run the checks in +all of the 3 methods of the action class and raise errors if the user doesn't +pass the checks. + +Rendering a response to the client in the outcomes (1) and (2b) is a task for +the controller action. The return value of the `run_second_factor!` method, +which is an instance of `SecondFactor::AuthManagerResult`, can be used to know +which outcome the auth manager has picked and render a different response based +on the outcome. + +For a real example where the auth manager is used, please refer to: + +* `SecondFactor::Actions::GrantAdmin` action class. This is a class that + inherits `SecondFactor::Actions::Base` and implements the 3 methods mentioned + above. + +* `Admin::UsersController#grant_admin` controller action. + +=end + +class SecondFactor::AuthManager + MAX_CHALLENGE_AGE = 5.minutes + + class SecondFactorRequired < StandardError + attr_reader :nonce + + def initialize(nonce:) + @nonce = nonce + end + end + + attr_reader :allowed_methods + + def self.find_second_factor_challenge(nonce, secure_session) + challenge_json = secure_session["current_second_factor_auth_challenge"] + if challenge_json.blank? + raise SecondFactor::BadChallenge.new( + "second_factor_auth.challenge_not_found", + status_code: 404 + ) + end + + challenge = JSON.parse(challenge_json).deep_symbolize_keys + if challenge[:nonce] != nonce + raise SecondFactor::BadChallenge.new( + "second_factor_auth.challenge_not_found", + status_code: 404 + ) + end + + generated_at = challenge[:generated_at] + if generated_at < MAX_CHALLENGE_AGE.ago.to_i + raise SecondFactor::BadChallenge.new( + "second_factor_auth.challenge_expired", + status_code: 401 + ) + end + challenge + end + + def initialize(guardian, action) + @guardian = guardian + @current_user = guardian.user + @action = action + @allowed_methods = Set.new([ + UserSecondFactor.methods[:totp], + UserSecondFactor.methods[:security_key], + ]).freeze + end + + def allow_backup_codes! + add_method(UserSecondFactor.methods[:backup_codes]) + end + + def run!(request, params, secure_session) + if !allowed_methods.any? { |m| @current_user.valid_second_factor_method_for_user?(m) } + @action.no_second_factors_enabled!(params) + create_result(:no_second_factor) + elsif nonce = params[:second_factor_nonce].presence + verify_second_factor_auth_completed(nonce, secure_session) + create_result(:second_factor_auth_completed) + else + nonce = initiate_second_factor_auth(params, secure_session, request) + raise SecondFactorRequired.new(nonce: nonce) + end + end + + private + + def initiate_second_factor_auth(params, secure_session, request) + config = @action.second_factor_auth_required!(params) + nonce = SecureRandom.alphanumeric(32) + callback_params = config[:callback_params] || {} + redirect_path = config[:redirect_path] || GlobalPath.path("").presence || "/" + challenge = { + nonce: nonce, + callback_method: request.request_method, + callback_path: request.path, + callback_params: callback_params, + redirect_path: redirect_path, + allowed_methods: allowed_methods.to_a, + generated_at: Time.zone.now.to_i + } + secure_session["current_second_factor_auth_challenge"] = challenge.to_json + nonce + end + + def verify_second_factor_auth_completed(nonce, secure_session) + challenge = self.class.find_second_factor_challenge(nonce, secure_session) + if !challenge[:successful] + raise SecondFactor::BadChallenge.new( + "second_factor_auth.challenge_not_completed", + status_code: 401 + ) + end + + secure_session["current_second_factor_auth_challenge"] = nil + callback_params = challenge[:callback_params] + @action.second_factor_auth_completed!(callback_params) + end + + def add_method(id) + if !@allowed_methods.include?(id) + @allowed_methods = Set.new(@allowed_methods) + @allowed_methods.add(id) + @allowed_methods.freeze + end + end + + def create_result(status) + SecondFactor::AuthManagerResult.new(status) + end +end diff --git a/lib/second_factor/auth_manager_result.rb b/lib/second_factor/auth_manager_result.rb new file mode 100644 index 00000000000..00e11653aa4 --- /dev/null +++ b/lib/second_factor/auth_manager_result.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class SecondFactor::AuthManagerResult + STATUSES = { + no_second_factor: 1, + second_factor_auth_completed: 2, + }.freeze + + private_constant :STATUSES + + def initialize(status) + if !STATUSES.key?(status) + raise ArgumentError.new("#{status.inspect} is not a valid status. Allowed statuses: #{STATUSES.inspect}") + end + @status_id = STATUSES[status] + end + + def no_second_factors_enabled? + @status_id == STATUSES[:no_second_factor] + end + + def second_factor_auth_completed? + @status_id == STATUSES[:second_factor_auth_completed] + end +end diff --git a/lib/second_factor/bad_challenge.rb b/lib/second_factor/bad_challenge.rb new file mode 100644 index 00000000000..59d533b42f0 --- /dev/null +++ b/lib/second_factor/bad_challenge.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class SecondFactor::BadChallenge < StandardError + attr_reader :error_translation_key, :status_code + + def initialize(error_translation_key, status_code:) + @error_translation_key = error_translation_key + @status_code = status_code + end +end diff --git a/spec/lib/second_factor/actions/grant_admin_spec.rb b/spec/lib/second_factor/actions/grant_admin_spec.rb new file mode 100644 index 00000000000..62d7951cfcb --- /dev/null +++ b/spec/lib/second_factor/actions/grant_admin_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe SecondFactor::Actions::GrantAdmin do + fab!(:admin) { Fabricate(:admin) } + fab!(:user) { Fabricate(:user) } + + def cleanup_admin_confirmation_redis_keys + keys = Discourse.redis.keys("admin-confirmation:*") + keys += Discourse.redis.keys("admin-confirmation-token:*") + Discourse.redis.del(keys) + end + + after do + cleanup_admin_confirmation_redis_keys + end + + def params(hash) + ActionController::Parameters.new(hash) + end + + def create_instance(user) + SecondFactor::Actions::GrantAdmin.new(Guardian.new(user)) + end + + describe "#no_second_factors_enabled!" do + it "sends new admin confirmation email" do + instance = create_instance(admin) + expect { + instance.no_second_factors_enabled!(params({ user_id: user.id })) + }.to change { AdminConfirmation.exists_for?(user.id) }.from(false).to(true) + end + + it "ensures the acting user is admin" do + instance = create_instance(user) + expect { + instance.no_second_factors_enabled!(params({ user_id: user.id })) + }.to raise_error(Discourse::InvalidAccess) + expect(AdminConfirmation.exists_for?(user.id)).to eq(false) + end + end + + describe "#second_factor_auth_required!" do + it "returns a hash with callback_params and redirect_path" 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}") + end + + it "ensures the acting user is admin" do + instance = create_instance(user) + expect { + instance.second_factor_auth_required!(params({ user_id: user.id })) + }.to raise_error(Discourse::InvalidAccess) + end + end + + describe "#second_factor_auth_completed!" do + it "grants the target user admin access and logs to staff action logs" do + instance = create_instance(admin) + expect { + instance.second_factor_auth_completed!(user_id: user.id) + }.to change { user.reload.admin }.from(false).to(true) + expect(UserHistory.exists?( + acting_user_id: admin.id, + target_user_id: user.id, + action: UserHistory.actions[:grant_admin] + )).to eq(true) + end + + it "ensures the acting user is admin" do + instance = create_instance(user) + expect { + instance.second_factor_auth_completed!(user_id: user.id) + }.to raise_error(Discourse::InvalidAccess) + end + end +end diff --git a/spec/lib/second_factor/auth_manager_spec.rb b/spec/lib/second_factor/auth_manager_spec.rb new file mode 100644 index 00000000000..ba8aa5d63bc --- /dev/null +++ b/spec/lib/second_factor/auth_manager_spec.rb @@ -0,0 +1,225 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe SecondFactor::AuthManager do + fab!(:user) { Fabricate(:user) } + fab!(:guardian) { Guardian.new(user) } + fab!(:user_totp) { Fabricate(:user_second_factor_totp, user: user) } + + def create_request(request_method: "GET", path: "/") + ActionDispatch::TestRequest.create({ + "REQUEST_METHOD" => request_method, + "PATH_INFO" => path + }) + end + + def create_manager(action) + SecondFactor::AuthManager.new(guardian, action) + end + + def create_action + TestSecondFactorAction.new(guardian) + end + + def stage_challenge(successful:) + action = create_action + action.expects(:no_second_factors_enabled!).never + action + .expects(:second_factor_auth_required!) + .with({ random_param: 'hello' }) + .returns({ callback_params: { call_me_back: 4314 } }) + .once + manager = create_manager(action) + request = create_request( + request_method: "POST", + path: "/abc/xyz" + ) + secure_session = {} + begin + manager.run!(request, { random_param: 'hello' }, secure_session) + rescue SecondFactor::AuthManager::SecondFactorRequired + # expected + end + + challenge = JSON + .parse(secure_session["current_second_factor_auth_challenge"]) + .deep_symbolize_keys + + challenge[:successful] = successful + secure_session["current_second_factor_auth_challenge"] = challenge.to_json + [challenge[:nonce], secure_session] + end + + describe '#allow_backup_codes!' do + it 'adds the backup codes method to the allowed methods set' do + manager = create_manager(create_action) + expect(manager.allowed_methods).not_to include( + UserSecondFactor.methods[:backup_codes] + ) + manager.allow_backup_codes! + expect(manager.allowed_methods).to include( + UserSecondFactor.methods[:backup_codes] + ) + end + end + + describe '#run!' do + context 'when the user does not have a suitable 2FA method' do + before do + user_totp.destroy! + end + + it 'calls the no_second_factors_enabled! method of the action' do + action = create_action + action.expects(:no_second_factors_enabled!).with({ hello_world: 331 }).once + action.expects(:second_factor_auth_required!).never + action.expects(:second_factor_auth_completed!).never + manager = create_manager(action) + manager.run!(create_request, { hello_world: 331 }, {}) + end + + it 'calls the no_second_factors_enabled! method of the action even if a nonce is present in the params' do + action = create_action + params = { second_factor_nonce: SecureRandom.hex } + action.expects(:no_second_factors_enabled!).with(params).once + action.expects(:second_factor_auth_required!).never + action.expects(:second_factor_auth_completed!).never + manager = create_manager(action) + manager.run!(create_request, params, {}) + end + end + + it "initiates the 2FA process and stages a challenge in secure session when there is no nonce in params" do + action = create_action + action.expects(:no_second_factors_enabled!).never + action + .expects(:second_factor_auth_required!) + .with({ expect_me: 131 }) + .returns({ callback_params: { call_me_back: 4314 }, redirect_path: "/gg" }) + .once + action.expects(:second_factor_auth_completed!).never + manager = create_manager(action) + request = create_request( + request_method: "POST", + path: "/abc/xyz" + ) + secure_session = {} + expect { + manager.run!(request, { expect_me: 131 }, secure_session) + }.to raise_error(SecondFactor::AuthManager::SecondFactorRequired) + json = secure_session["current_second_factor_auth_challenge"] + challenge = JSON.parse(json).deep_symbolize_keys + expect(challenge[:nonce]).to be_present + expect(challenge[:callback_method]).to eq("POST") + expect(challenge[:callback_path]).to eq("/abc/xyz") + 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 }) + end + + it "sets the redirect_path to the root path if second_factor_auth_required! doesn't specify a redirect_path" do + action = create_action + action.expects(:no_second_factors_enabled!).never + action + .expects(:second_factor_auth_required!) + .with({ expect_me: 131 }) + .returns({ callback_params: { call_me_back: 4314 } }) + .once + action.expects(:second_factor_auth_completed!).never + manager = create_manager(action) + request = create_request( + request_method: "POST", + path: "/abc/xyz" + ) + secure_session = {} + expect { + manager.run!(request, { expect_me: 131 }, secure_session) + }.to raise_error(SecondFactor::AuthManager::SecondFactorRequired) + json = secure_session["current_second_factor_auth_challenge"] + challenge = JSON.parse(json).deep_symbolize_keys + expect(challenge[:redirect_path]).to eq("/") + + set_subfolder("/community") + action = create_action + action.expects(:no_second_factors_enabled!).never + action + .expects(:second_factor_auth_required!) + .with({ expect_me: 131 }) + .returns({ callback_params: { call_me_back: 4314 } }) + .once + action.expects(:second_factor_auth_completed!).never + manager = create_manager(action) + request = create_request( + request_method: "POST", + path: "/abc/xyz" + ) + secure_session = {} + expect { + manager.run!(request, { expect_me: 131 }, secure_session) + }.to raise_error(SecondFactor::AuthManager::SecondFactorRequired) + json = secure_session["current_second_factor_auth_challenge"] + challenge = JSON.parse(json).deep_symbolize_keys + expect(challenge[:redirect_path]).to eq("/community") + end + + it "calls the second_factor_auth_completed! method of the action if the challenge is successful and not expired" do + nonce, secure_session = stage_challenge(successful: true) + + action = create_action + + action.expects(:no_second_factors_enabled!).never + action.expects(:second_factor_auth_required!).never + action + .expects(:second_factor_auth_completed!) + .with({ call_me_back: 4314 }) + .once + manager = create_manager(action) + request = create_request( + request_method: "POST", + path: "/abc/xyz" + ) + manager.run!(request, { second_factor_nonce: nonce }, secure_session) + end + + it "does not call the second_factor_auth_completed! method of the action if the challenge is not marked successful" do + nonce, secure_session = stage_challenge(successful: false) + + action = create_action + action.expects(:no_second_factors_enabled!).never + action.expects(:second_factor_auth_required!).never + action.expects(:second_factor_auth_completed!).never + manager = create_manager(action) + request = create_request( + request_method: "POST", + path: "/abc/xyz" + ) + expect { + manager.run!(request, { second_factor_nonce: nonce }, secure_session) + }.to raise_error(SecondFactor::BadChallenge) do |ex| + expect(ex.error_translation_key).to eq("second_factor_auth.challenge_not_completed") + end + end + + it "does not call the second_factor_auth_completed! method of the action if the challenge is expired" do + nonce, secure_session = stage_challenge(successful: true) + + action = create_action + action.expects(:no_second_factors_enabled!).never + action.expects(:second_factor_auth_required!).never + action.expects(:second_factor_auth_completed!).never + manager = create_manager(action) + request = create_request( + request_method: "POST", + path: "/abc/xyz" + ) + + freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now + expect { + manager.run!(request, { second_factor_nonce: nonce }, secure_session) + }.to raise_error(SecondFactor::BadChallenge) do |ex| + expect(ex.error_translation_key).to eq("second_factor_auth.challenge_expired") + end + end + end +end diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index 7ece24fd80d..c39be0e371c 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -346,7 +346,15 @@ RSpec.describe Admin::UsersController do Discourse.redis.flushdb end - it "raises an error when the user doesn't have permission" do + it "returns a 404 when the acting user doesn't have permission" do + sign_in(user) + put "/admin/users/#{another_user.id}/grant_admin.json" + expect(response.status).to eq(404) + expect(AdminConfirmation.exists_for?(another_user.id)).to eq(false) + end + + it "returns a 404 when the acting user doesn't have permission even if they have 2FA enabled" do + Fabricate(:user_second_factor_totp, user: user) sign_in(user) put "/admin/users/#{another_user.id}/grant_admin.json" expect(response.status).to eq(404) @@ -358,33 +366,121 @@ RSpec.describe Admin::UsersController do expect(response.status).to eq(404) end - it 'updates the admin flag' do + it 'sends a confirmation email if the acting admin does not have a second factor method enabled' do expect(AdminConfirmation.exists_for?(another_user.id)).to eq(false) put "/admin/users/#{another_user.id}/grant_admin.json" expect(response.status).to eq(200) expect(AdminConfirmation.exists_for?(another_user.id)).to eq(true) end - it 'asks user for second factor if it is enabled' do - user_second_factor = Fabricate(:user_second_factor_totp, user: admin) + it 'asks the acting admin for second factor if it is enabled' do + Fabricate(:user_second_factor_totp, user: admin) put "/admin/users/#{another_user.id}/grant_admin.json" - expect(response.parsed_body["failed"]).to eq("FAILED") + expect(response.parsed_body["second_factor_challenge_nonce"]).to be_present expect(another_user.reload.admin).to eq(false) end it 'grants admin if second factor is correct' do user_second_factor = Fabricate(:user_second_factor_totp, user: admin) - put "/admin/users/#{another_user.id}/grant_admin.json", params: { + put "/admin/users/#{another_user.id}/grant_admin.json" + nonce = response.parsed_body["second_factor_challenge_nonce"] + expect(nonce).to be_present + expect(another_user.reload.admin).to eq(false) + + post "/session/2fa.json", params: { + nonce: nonce, second_factor_token: ROTP::TOTP.new(user_second_factor.data).now, second_factor_method: UserSecondFactor.methods[:totp] } + res = response.parsed_body + expect(response.status).to eq(200) + expect(res["ok"]).to eq(true) + expect(res["callback_method"]).to eq("PUT") + expect(res["callback_path"]).to eq("/admin/users/#{another_user.id}/grant_admin.json") + expect(res["redirect_path"]).to eq("/admin/users/#{another_user.id}/#{another_user.username}") + expect(another_user.reload.admin).to eq(false) - expect(response.parsed_body["success"]).to eq("OK") + put res["callback_path"], params: { + second_factor_nonce: nonce + } + expect(response.status).to eq(200) expect(another_user.reload.admin).to eq(true) end + + it 'does not grant admin if second factor auth is not successful' do + user_second_factor = Fabricate(:user_second_factor_totp, user: admin) + + put "/admin/users/#{another_user.id}/grant_admin.json" + nonce = response.parsed_body["second_factor_challenge_nonce"] + expect(nonce).to be_present + expect(another_user.reload.admin).to eq(false) + + token = ROTP::TOTP.new(user_second_factor.data).now.to_i + token = (token == 999_999 ? token - 1 : token + 1).to_s + post "/session/2fa.json", params: { + nonce: nonce, + second_factor_token: token, + second_factor_method: UserSecondFactor.methods[:totp] + } + expect(response.status).to eq(400) + expect(another_user.reload.admin).to eq(false) + + put "/admin/users/#{another_user.id}/grant_admin.json", params: { + second_factor_nonce: nonce + } + expect(response.status).to eq(401) + expect(another_user.reload.admin).to eq(false) + end + + it 'does not grant admin if the acting admin loses permission in the middle of the process' do + user_second_factor = Fabricate(:user_second_factor_totp, user: admin) + + put "/admin/users/#{another_user.id}/grant_admin.json" + nonce = response.parsed_body["second_factor_challenge_nonce"] + expect(nonce).to be_present + expect(another_user.reload.admin).to eq(false) + + post "/session/2fa.json", params: { + nonce: nonce, + second_factor_token: ROTP::TOTP.new(user_second_factor.data).now, + second_factor_method: UserSecondFactor.methods[:totp] + } + res = response.parsed_body + expect(response.status).to eq(200) + expect(res["ok"]).to eq(true) + expect(res["callback_method"]).to eq("PUT") + expect(res["callback_path"]).to eq("/admin/users/#{another_user.id}/grant_admin.json") + expect(res["redirect_path"]).to eq("/admin/users/#{another_user.id}/#{another_user.username}") + expect(another_user.reload.admin).to eq(false) + + admin.update!(admin: false) + put res["callback_path"], params: { + second_factor_nonce: nonce + } + expect(response.status).to eq(404) + expect(another_user.reload.admin).to eq(false) + end + + it 'does not accept backup codes' do + Fabricate(:user_second_factor_totp, user: admin) + Fabricate(:user_second_factor_backup, user: admin) + + put "/admin/users/#{another_user.id}/grant_admin.json" + nonce = response.parsed_body["second_factor_challenge_nonce"] + expect(nonce).to be_present + expect(another_user.reload.admin).to eq(false) + + post "/session/2fa.json", params: { + nonce: nonce, + second_factor_token: "iAmValidBackupCode", + second_factor_method: UserSecondFactor.methods[:backup_codes] + } + expect(response.status).to eq(403) + expect(another_user.reload.admin).to eq(false) + end end describe '#add_group' do diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index be65aec5eb3..13ed81e20dc 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -2260,4 +2260,164 @@ describe SessionController do end end end + + describe '#second_factor_auth_show' do + let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) } + + before do + sign_in(user) + end + + it 'returns 404 if there is no challenge for the given nonce' do + get "/session/2fa.json", params: { nonce: 'asdasdsadsad' } + expect(response.status).to eq(404) + expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_not_found")) + end + + it 'returns 404 if the nonce does not match the challenge nonce' do + post "/session/2fa/test-action" + get "/session/2fa.json", params: { nonce: 'wrongnonce' } + expect(response.status).to eq(404) + expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_not_found")) + end + + it 'returns 401 if the challenge nonce has expired' do + post "/session/2fa/test-action" + nonce = response.parsed_body["second_factor_challenge_nonce"] + get "/session/2fa.json", params: { nonce: nonce } + expect(response.status).to eq(200) + + freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now + get "/session/2fa.json", params: { nonce: nonce } + expect(response.status).to eq(401) + expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_expired")) + end + + it 'responds with challenge data' do + post "/session/2fa/test-action" + nonce = response.parsed_body["second_factor_challenge_nonce"] + get "/session/2fa.json", params: { nonce: nonce } + expect(response.status).to eq(200) + challenge_data = response.parsed_body + expect(challenge_data["totp_enabled"]).to eq(true) + expect(challenge_data["backup_enabled"]).to eq(false) + expect(challenge_data["security_keys_enabled"]).to eq(false) + expect(challenge_data["allowed_methods"]).to contain_exactly( + UserSecondFactor.methods[:totp], + UserSecondFactor.methods[:security_key], + ) + + Fabricate( + :user_security_key_with_random_credential, + user: user, + name: 'Enabled YubiKey', + enabled: true + ) + Fabricate(:user_second_factor_backup, user: user) + post "/session/2fa/test-action", params: { allow_backup_codes: true } + nonce = response.parsed_body["second_factor_challenge_nonce"] + get "/session/2fa.json", params: { nonce: nonce } + expect(response.status).to eq(200) + challenge_data = response.parsed_body + expect(challenge_data["totp_enabled"]).to eq(true) + expect(challenge_data["backup_enabled"]).to eq(true) + expect(challenge_data["security_keys_enabled"]).to eq(true) + expect(challenge_data["allowed_credential_ids"]).to be_present + expect(challenge_data["challenge"]).to be_present + expect(challenge_data["allowed_methods"]).to contain_exactly( + UserSecondFactor.methods[:totp], + UserSecondFactor.methods[:security_key], + UserSecondFactor.methods[:backup_codes], + ) + end + end + + describe '#second_factor_auth_perform' do + let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) } + + before do + sign_in(user) + end + + it 'returns 401 if the challenge nonce has expired' do + post "/session/2fa/test-action" + nonce = response.parsed_body["second_factor_challenge_nonce"] + + freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now + token = ROTP::TOTP.new(user_second_factor.data).now + post "/session/2fa.json", params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:totp], + second_factor_token: token + } + expect(response.status).to eq(401) + expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_expired")) + end + + it 'returns 403 if the 2FA method is not allowed' do + Fabricate(:user_second_factor_backup, user: user) + post "/session/2fa/test-action" + nonce = response.parsed_body["second_factor_challenge_nonce"] + post "/session/2fa.json", params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:backup_codes], + second_factor_token: "iAmValidBackupCode" + } + expect(response.status).to eq(403) + end + + it 'returns 403 if the user disables the 2FA method in the middle of the 2FA process' do + post "/session/2fa/test-action" + nonce = response.parsed_body["second_factor_challenge_nonce"] + token = ROTP::TOTP.new(user_second_factor.data).now + user_second_factor.destroy! + post "/session/2fa.json", params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:totp], + second_factor_token: token + } + expect(response.status).to eq(403) + end + + it 'marks the challenge as successful if the 2fa succeeds' do + post "/session/2fa/test-action", params: { redirect_path: "/ggg" } + nonce = response.parsed_body["second_factor_challenge_nonce"] + + token = ROTP::TOTP.new(user_second_factor.data).now + post "/session/2fa.json", params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:totp], + second_factor_token: token + } + expect(response.status).to eq(200) + expect(response.parsed_body["ok"]).to eq(true) + expect(response.parsed_body["callback_method"]).to eq("POST") + expect(response.parsed_body["callback_path"]).to eq("/session/2fa/test-action") + expect(response.parsed_body["redirect_path"]).to eq("/ggg") + + post "/session/2fa/test-action", params: { second_factor_nonce: nonce } + expect(response.status).to eq(200) + expect(response.parsed_body["result"]).to eq("second_factor_auth_completed") + end + + it 'does not mark the challenge as successful if the 2fa fails' do + post "/session/2fa/test-action", params: { redirect_path: "/ggg" } + nonce = response.parsed_body["second_factor_challenge_nonce"] + + token = ROTP::TOTP.new(user_second_factor.data).now.to_i + token += token == 999999 ? -1 : 1 + post "/session/2fa.json", params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:totp], + second_factor_token: token.to_s + } + expect(response.status).to eq(400) + expect(response.parsed_body["ok"]).to eq(false) + expect(response.parsed_body["reason"]).to eq("invalid_second_factor") + expect(response.parsed_body["error"]).to eq(I18n.t("login.invalid_second_factor_code")) + + post "/session/2fa/test-action", params: { second_factor_nonce: nonce } + expect(response.status).to eq(401) + end + end end diff --git a/spec/support/session_controller_helper_routes.rb b/spec/support/session_controller_helper_routes.rb new file mode 100644 index 00000000000..0cb2e191dff --- /dev/null +++ b/spec/support/session_controller_helper_routes.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module SessionControllerExtension + def self.included(base) + base.skip_before_action :check_xhr, only: %i(test_second_factor_restricted_route) + end + + def test_second_factor_restricted_route + result = run_second_factor!(TestSecondFactorAction) do |manager| + manager.allow_backup_codes! if params[:allow_backup_codes] + end + if result.no_second_factors_enabled? + render json: { result: 'no_second_factors_enabled' } + else + render json: { result: 'second_factor_auth_completed' } + end + end +end + +SessionController.class_eval { include SessionControllerExtension } diff --git a/spec/support/test_second_factor_action.rb b/spec/support/test_second_factor_action.rb new file mode 100644 index 00000000000..713a7f01481 --- /dev/null +++ b/spec/support/test_second_factor_action.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class TestSecondFactorAction < SecondFactor::Actions::Base + def no_second_factors_enabled!(params) + end + + def second_factor_auth_required!(params) + { + redirect_path: params[:redirect_path], + callback_params: { + saved_param_1: params[:saved_param_1], + saved_param_2: params[:saved_param_2] + } + } + end + + def second_factor_auth_completed!(callback_params) + end +end