Skip to content

Commit

Permalink
FEATURE: Use second factor for admin confirmation
Browse files Browse the repository at this point in the history
If second_factor_confirmation is true, then administrators can confirm
other administrators using a second factor. This is an alternative to
clicking a URL from an email.
  • Loading branch information
nbianca committed Sep 9, 2021
1 parent 34ff7bf commit 8e6a361
Show file tree
Hide file tree
Showing 11 changed files with 260 additions and 6 deletions.
Expand Up @@ -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);
},
Expand Down
13 changes: 12 additions & 1 deletion app/assets/javascripts/admin/addon/models/admin-user.js
Expand Up @@ -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;
});
},

Expand Down
Expand Up @@ -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"}}
Expand Down
@@ -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));
},
});
@@ -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}}
<div class="modal-footer">
{{d-button
action=(action "authenticate")
icon="shield-alt"
label="admin.user.grant_admin"
disabled=disabled
class="btn btn-primary"}}
</div>
{{/unless}}
{{/d-modal-body}}
@@ -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();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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"));
});
});
22 changes: 20 additions & 2 deletions app/controllers/admin/users_controller.rb
Expand Up @@ -191,8 +191,26 @@ def revoke_admin
end

def grant_admin
AdminConfirmation.new(@user, current_user).create_confirmation
render json: success_json
if SiteSetting.second_factor_confirmation && 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
guardian.ensure_can_grant_admin!(@user)
@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
Expand Down
1 change: 1 addition & 0 deletions config/locales/client.en.yml
Expand Up @@ -4923,6 +4923,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"
Expand Down
1 change: 1 addition & 0 deletions config/locales/server.en.yml
Expand Up @@ -1541,6 +1541,7 @@ en:
email_subject: "Customizable subject format for standard emails. See <a href='https://meta.discourse.org/t/customize-subject-format-for-standard-emails/20801' target='_blank'>https://meta.discourse.org/t/customize-subject-format-for-standard-emails/20801</a>"
detailed_404: "Provides more details to users about why they can’t access a particular topic. Note: This is less secure because users will know if a URL links to a valid topic."
enforce_second_factor: "Forces users to enable two-factor authentication. Select 'all' to enforce it to all users. Select 'staff' to enforce it to staff users only."
second_factor_confirmation: "Allow administrators to use two-factor authentication to confirm new administrators instead of using email confirmation."
force_https: "Force your site to use HTTPS only. WARNING: do NOT enable this until you verify HTTPS is fully set up and working absolutely everywhere! Did you check your CDN, all social logins, and any external logos / dependencies to make sure they are all HTTPS compatible, too?"
same_site_cookies: "Use same site cookies, they eliminate all Cross Site Request Forgery vectors on supported browsers (Lax or Strict). Warning: Strict will only work on sites that force login and use an external auth method."
summary_score_threshold: "The minimum score required for a post to be included in 'Summarize This Topic'"
Expand Down
2 changes: 2 additions & 0 deletions config/site_settings.yml
Expand Up @@ -1573,6 +1573,8 @@ security:
- "no"
- "staff"
- "all"
second_factor_confirmation:
default: false
force_https:
default: false
same_site_cookies:
Expand Down
25 changes: 25 additions & 0 deletions spec/requests/admin/users_controller_spec.rb
Expand Up @@ -2,6 +2,7 @@

require 'rails_helper'
require 'discourse_ip_info'
require 'rotp'

RSpec.describe Admin::UsersController do
fab!(:admin) { Fabricate(:admin) }
Expand Down Expand Up @@ -362,6 +363,30 @@
expect(response.status).to eq(200)
expect(AdminConfirmation.exists_for?(another_user.id)).to eq(true)
end

context 'second_factor_confirmation is true' do
before do
SiteSetting.second_factor_confirmation = true
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

it 'falls back to admin if second factor is not set' do
put "/admin/users/#{another_user.id}/grant_admin.json"
expect(response.parsed_body["email_confirmation_required"]).to eq(true)
expect(another_user.reload.admin).to eq(false)
end
end
end

describe '#add_group' do
Expand Down

0 comments on commit 8e6a361

Please sign in to comment.