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