From 6a7ea66670dbb9b2b3f331c8d610d9efed879bfe Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Tue, 14 Sep 2021 15:19:28 +0300 Subject: [PATCH] FEATURE: Use second factor for admin confirmation (#14293) Administrators can use second factor to confirm granting admin access without using email. The old method of confirmation via email is still used as a fallback when second factor is unavailable. --- .../addon/controllers/admin-user-index.js | 11 ++- .../admin/addon/models/admin-user.js | 13 ++- .../admin/addon/templates/user-index.hbs | 2 +- .../controllers/grant-admin-second-factor.js | 83 +++++++++++++++++++ .../modal/grant-admin-second-factor.hbs | 33 ++++++++ .../tests/acceptance/admin-user-index-test.js | 73 ++++++++++++++++ app/controllers/admin/users_controller.rb | 21 ++++- config/locales/client.en.yml | 1 + spec/requests/admin/users_controller_spec.rb | 22 +++++ 9 files changed, 253 insertions(+), 6 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/controllers/grant-admin-second-factor.js create mode 100644 app/assets/javascripts/discourse/app/templates/modal/grant-admin-second-factor.hbs 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 93ed9c306f1..22345e8b829 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-user-index.js +++ b/app/assets/javascripts/admin/addon/controllers/admin-user-index.js @@ -218,8 +218,15 @@ export default Controller.extend(CanCheckEmails, { grantAdmin() { return this.model .grantAdmin() - .then(() => { - bootbox.alert(I18n.t("admin.user.grant_admin_confirm")); + .then((result) => { + if (result.email_confirmation_required) { + bootbox.alert(I18n.t("admin.user.grant_admin_confirm")); + } else { + const controller = showModal("grant-admin-second-factor", { + model: this.model, + }); + controller.setResult(result); + } }) .catch(popupAjaxError); }, diff --git a/app/assets/javascripts/admin/addon/models/admin-user.js b/app/assets/javascripts/admin/addon/models/admin-user.js index cfd34b87e37..3fb3e28dad4 100644 --- a/app/assets/javascripts/admin/addon/models/admin-user.js +++ b/app/assets/javascripts/admin/addon/models/admin-user.js @@ -99,9 +99,20 @@ const AdminUser = User.extend({ }); }, - grantAdmin() { + grantAdmin(data) { return ajax(`/admin/users/${this.id}/grant_admin`, { type: "PUT", + data, + }).then((resp) => { + if (resp.success && !resp.email_confirmation_required) { + this.setProperties({ + admin: true, + can_grant_admin: false, + can_revoke_admin: true, + }); + } + + return resp; }); }, diff --git a/app/assets/javascripts/admin/addon/templates/user-index.hbs b/app/assets/javascripts/admin/addon/templates/user-index.hbs index d81aeb21f46..df6c4c84062 100644 --- a/app/assets/javascripts/admin/addon/templates/user-index.hbs +++ b/app/assets/javascripts/admin/addon/templates/user-index.hbs @@ -334,7 +334,7 @@ {{/if}} {{#if model.can_grant_admin}} {{d-button - class="btn-default" + class="btn-default grant-admin" action=(action "grantAdmin") icon="shield-alt" label="admin.user.grant_admin"}} diff --git a/app/assets/javascripts/discourse/app/controllers/grant-admin-second-factor.js b/app/assets/javascripts/discourse/app/controllers/grant-admin-second-factor.js new file mode 100644 index 00000000000..2debd00feb6 --- /dev/null +++ b/app/assets/javascripts/discourse/app/controllers/grant-admin-second-factor.js @@ -0,0 +1,83 @@ +import Controller from "@ember/controller"; +import { action } from "@ember/object"; +import discourseComputed from "discourse-common/utils/decorators"; +import { getWebauthnCredential } from "discourse/lib/webauthn"; +import ModalFunctionality from "discourse/mixins/modal-functionality"; +import { SECOND_FACTOR_METHODS } from "discourse/models/user"; +import I18n from "I18n"; + +export default Controller.extend(ModalFunctionality, { + showSecondFactor: false, + secondFactorMethod: SECOND_FACTOR_METHODS.TOTP, + secondFactorToken: null, + securityKeyCredential: null, + + inProgress: false, + + onShow() { + this.setProperties({ + showSecondFactor: false, + secondFactorMethod: SECOND_FACTOR_METHODS.TOTP, + secondFactorToken: null, + securityKeyCredential: null, + }); + }, + + @discourseComputed("inProgress", "securityKeyCredential", "secondFactorToken") + disabled(inProgress, securityKeyCredential, secondFactorToken) { + return inProgress || (!securityKeyCredential && !secondFactorToken); + }, + + setResult(result) { + this.setProperties({ + otherMethodAllowed: result.multiple_second_factor_methods, + secondFactorRequired: true, + showLoginButtons: false, + backupEnabled: result.backup_enabled, + showSecondFactor: result.totp_enabled, + showSecurityKey: result.security_key_enabled, + secondFactorMethod: result.security_key_enabled + ? SECOND_FACTOR_METHODS.SECURITY_KEY + : SECOND_FACTOR_METHODS.TOTP, + securityKeyChallenge: result.challenge, + securityKeyAllowedCredentialIds: result.allowed_credential_ids, + }); + }, + + @action + authenticateSecurityKey() { + getWebauthnCredential( + this.securityKeyChallenge, + this.securityKeyAllowedCredentialIds, + (credentialData) => { + this.set("securityKeyCredential", credentialData); + this.send("authenticate"); + }, + (errorMessage) => { + this.flash(errorMessage, "error"); + } + ); + }, + + @action + authenticate() { + this.set("inProgress", true); + this.model + .grantAdmin({ + second_factor_token: + this.securityKeyCredential || this.secondFactorToken, + second_factor_method: this.secondFactorMethod, + timezone: moment.tz.guess(), + }) + .then((result) => { + if (result.success) { + this.send("closeModal"); + bootbox.alert(I18n.t("admin.user.grant_admin_success")); + } else { + this.flash(result.error, "error"); + this.setResult(result); + } + }) + .finally(() => this.set("inProgress", false)); + }, +}); diff --git a/app/assets/javascripts/discourse/app/templates/modal/grant-admin-second-factor.hbs b/app/assets/javascripts/discourse/app/templates/modal/grant-admin-second-factor.hbs new file mode 100644 index 00000000000..3c49f73034c --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/modal/grant-admin-second-factor.hbs @@ -0,0 +1,33 @@ +{{#d-modal-body title="admin.user.grant_admin"}} + {{#second-factor-form + secondFactorMethod=secondFactorMethod + secondFactorToken=secondFactorToken + class=secondFactorClass + backupEnabled=backupEnabled + }} + {{#if showSecurityKey}} + {{#security-key-form + allowedCredentialIds=securityKeyAllowedCredentialIds + challenge=securityKeyChallenge + showSecurityKey=showSecurityKey + showSecondFactor=showSecondFactor + secondFactorMethod=secondFactorMethod + otherMethodAllowed=otherMethodAllowed + action=(action "authenticateSecurityKey")}} + {{/security-key-form}} + {{else}} + {{second-factor-input value=secondFactorToken inputId="second-factor-confirmation" secondFactorMethod=secondFactorMethod backupEnabled=backupEnabled}} + {{/if}} + {{/second-factor-form}} + + {{#unless showSecurityKey}} +
+ {{d-button + action=(action "authenticate") + icon="shield-alt" + label="admin.user.grant_admin" + disabled=disabled + class="btn btn-primary"}} +
+ {{/unless}} +{{/d-modal-body}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-user-index-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-user-index-test.js index 3c399d2a819..c1403fa8609 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/admin-user-index-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/admin-user-index-test.js @@ -1,11 +1,13 @@ import { acceptance, exists, + query, queryAll, } from "discourse/tests/helpers/qunit-helpers"; import { click, currentURL, fillIn, visit } from "@ember/test-helpers"; import selectKit from "discourse/tests/helpers/select-kit-helper"; import { test } from "qunit"; +import I18n from "I18n"; acceptance("Admin - User Index", function (needs) { needs.user(); @@ -40,6 +42,60 @@ acceptance("Admin - User Index", function (needs) { server.put("/users/sam/preferences/username", () => { return helper.response({ id: 2, username: "new-sam" }); }); + + server.get("/admin/users/3.json", () => { + return helper.response({ + id: 3, + username: "user1", + name: null, + avatar_template: "/letter_avatar_proxy/v4/letter/b/f0a364/{size}.png", + active: true, + admin: false, + moderator: false, + can_grant_admin: true, + can_revoke_admin: false, + can_grant_moderation: true, + can_revoke_moderation: false, + }); + }); + + server.put("/admin/users/3/grant_admin", () => { + return helper.response({ + success: "OK", + email_confirmation_required: true, + }); + }); + + server.get("/admin/users/4.json", () => { + return helper.response({ + id: 4, + username: "user2", + name: null, + avatar_template: "/letter_avatar_proxy/v4/letter/b/f0a364/{size}.png", + active: true, + admin: false, + moderator: false, + can_grant_admin: true, + can_revoke_admin: false, + can_grant_moderation: true, + can_revoke_moderation: false, + }); + }); + + server.put("/admin/users/4/grant_admin", () => { + return helper.response({ + failed: "FAILED", + ok: false, + error: "The selected two-factor method is invalid.", + reason: "invalid_second_factor_method", + backup_enabled: true, + security_key_enabled: true, + totp_enabled: true, + multiple_second_factor_methods: true, + allowed_credential_ids: ["allowed_credential_ids"], + challenge: "challenge", + }); + }); }); test("can edit username", async function (assert) { @@ -124,4 +180,21 @@ acceptance("Admin - User Index", function (needs) { "group should not be set" ); }); + + test("grant admin - shows the confirmation bootbox", async function (assert) { + await visit("/admin/users/3/user1"); + await click(".grant-admin"); + assert.ok(exists(".bootbox")); + assert.equal( + I18n.t("admin.user.grant_admin_confirm"), + query(".modal-body").textContent.trim() + ); + await click(".bootbox .btn-primary"); + }); + + test("grant admin - shows the second factor modal", async function (assert) { + await visit("/admin/users/4/user2"); + await click(".grant-admin"); + assert.ok(exists(".grant-admin-second-factor-modal")); + }); }); diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index cd8b2f7f3ae..637172fa8a0 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -191,8 +191,25 @@ class Admin::UsersController < Admin::AdminController end def grant_admin - AdminConfirmation.new(@user, current_user).create_confirmation - render json: success_json + 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 + render json: success_json.merge(email_confirmation_required: true) + end end def revoke_moderation diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index ae1294b14cf..f057a37af44 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -4960,6 +4960,7 @@ en: logged_out: "User was logged out on all devices" revoke_admin: "Revoke Admin" grant_admin: "Grant Admin" + grant_admin_success: "New administrator was confirmed." grant_admin_confirm: "We've sent you an email to verify the new administrator. Please open it and follow the instructions." revoke_moderation: "Revoke Moderation" grant_moderation: "Grant Moderation" diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index 1ad1867d152..19b41802705 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -2,6 +2,7 @@ require 'rails_helper' require 'discourse_ip_info' +require 'rotp' RSpec.describe Admin::UsersController do fab!(:admin) { Fabricate(:admin) } @@ -362,6 +363,27 @@ RSpec.describe Admin::UsersController do 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) + + put "/admin/users/#{another_user.id}/grant_admin.json" + + expect(response.parsed_body["failed"]).to eq("FAILED") + 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: { + second_factor_token: ROTP::TOTP.new(user_second_factor.data).now, + second_factor_method: UserSecondFactor.methods[:totp] + } + + expect(response.parsed_body["success"]).to eq("OK") + expect(another_user.reload.admin).to eq(true) + end end describe '#add_group' do