Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: Use second factor for admin confirmation #14293

Merged
merged 3 commits into from Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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"));
});
});
21 changes: 19 additions & 2 deletions app/controllers/admin/users_controller.rb
Expand Up @@ -191,8 +191,25 @@ def revoke_admin
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?
ZogStriP marked this conversation as resolved.
Show resolved Hide resolved
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
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
22 changes: 22 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,27 @@
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
Expand Down