diff --git a/app/assets/javascripts/discourse/controllers/second-factor-add-security-key.js.es6 b/app/assets/javascripts/discourse/controllers/second-factor-add-security-key.js.es6 index 54f41fc4054..2cb03f968ff 100644 --- a/app/assets/javascripts/discourse/controllers/second-factor-add-security-key.js.es6 +++ b/app/assets/javascripts/discourse/controllers/second-factor-add-security-key.js.es6 @@ -53,6 +53,13 @@ export default Controller.extend(ModalFunctionality, { actions: { registerSecurityKey() { + if (!this.securityKeyName) { + this.set( + "errorMessage", + I18n.t("user.second_factor.security_key.name_required_error") + ); + return; + } const publicKeyCredentialCreationOptions = { challenge: Uint8Array.from(this.challenge, c => c.charCodeAt(0)), rp: { diff --git a/app/assets/javascripts/discourse/controllers/second-factor-add-totp.js.es6 b/app/assets/javascripts/discourse/controllers/second-factor-add-totp.js.es6 index eab4f2943f7..10752e41c76 100644 --- a/app/assets/javascripts/discourse/controllers/second-factor-add-totp.js.es6 +++ b/app/assets/javascripts/discourse/controllers/second-factor-add-totp.js.es6 @@ -45,7 +45,13 @@ export default Controller.extend(ModalFunctionality, { }, enableSecondFactor() { - if (!this.secondFactorToken) return; + if (!this.secondFactorToken || !this.secondFactorName) { + this.set( + "errorMessage", + I18n.t("user.second_factor.totp.name_and_code_required_error") + ); + return; + } this.set("loading", true); this.model diff --git a/app/assets/javascripts/discourse/templates/modal/second-factor-add-security-key.hbs b/app/assets/javascripts/discourse/templates/modal/second-factor-add-security-key.hbs index d63b5fe8eb9..36ede3f634c 100644 --- a/app/assets/javascripts/discourse/templates/modal/second-factor-add-security-key.hbs +++ b/app/assets/javascripts/discourse/templates/modal/second-factor-add-security-key.hbs @@ -16,7 +16,7 @@
- {{input value=securityKeyName id='test' placeholder='security key name'}} + {{input value=securityKeyName id='security-key-name' placeholder='security key name'}}
@@ -24,7 +24,7 @@
{{#unless webauthnUnsupported}} {{d-button - class="btn-primary add-totp" + class="btn-primary add-security-key" action=(action "registerSecurityKey") label="user.second_factor.security_key.register"}} {{/unless}} diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index e96a79c9532..33e7bdce6c2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1211,8 +1211,12 @@ class UsersController < ApplicationController end def enable_second_factor_totp - params.require(:second_factor_token) - params.require(:name) + if params[:second_factor_token].blank? + return render json: failed_json.merge(error: I18n.t("login.missing_second_factor_code")) + end + if params[:name].blank? + return render json: failed_json.merge(error: I18n.t("login.missing_second_factor_name")) + end auth_token = params[:second_factor_token] totp_data = secure_session["staged-totp-#{current_user.id}"] diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 127c964d041..a18720a2930 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1023,6 +1023,7 @@ en: title: "Token-Based Authenticators" add: "New Authenticator" default_name: "My Authenticator" + name_and_code_required_error: "You must provide a name and the code from your authenticator app." security_key: register: "Register" title: "Security Keys" @@ -1033,6 +1034,7 @@ en: edit: "Edit Security Key" edit_description: "Security Key Name" delete: "Delete" + name_required_error: "You must provide a name for your security key." change_about: title: "Change About Me" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c4db42fb1ca..3705e98f010 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2381,6 +2381,8 @@ en: second_factor_backup_title: "Two Factor Backup Code" invalid_second_factor_code: "Invalid authentication code. Each code can only be used once." invalid_security_key: "Invalid security key." + missing_second_factor_name: "Please provide a name." + missing_second_factor_code: "Please provide a code." second_factor_toggle: totp: "Use an authenticator app or security key instead" backup_code: "Use a backup code instead" diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index ed28cab6504..13aebdfd2f4 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -3522,6 +3522,58 @@ describe UsersController do end end + describe "#enable_second_factor_totp" do + before do + sign_in(user) + end + + def create_totp + stub_secure_session_confirmed + post "/users/create_second_factor_totp.json" + end + + it "creates a totp for the user successfully" do + create_totp + staged_totp_key = read_secure_session["staged-totp-#{user.id}"] + token = ROTP::TOTP.new(staged_totp_key).now + + post "/users/enable_second_factor_totp.json", params: { name: "test", second_factor_token: token } + + expect(response.status).to eq(200) + expect(user.user_second_factors.count).to eq(1) + end + + context "when an incorrect token is provided" do + before do + create_totp + post "/users/enable_second_factor_totp.json", params: { name: "test", second_factor_token: "123456" } + end + it "shows a helpful error message to the user" do + expect(JSON.parse(response.body)['error']).to eq(I18n.t("login.invalid_second_factor_code")) + end + end + + context "when a name is not provided" do + before do + create_totp + post "/users/enable_second_factor_totp.json", params: { second_factor_token: "123456" } + end + it "shows a helpful error message to the user" do + expect(JSON.parse(response.body)['error']).to eq(I18n.t("login.missing_second_factor_name")) + end + end + + context "when a token is not provided" do + before do + create_totp + post "/users/enable_second_factor_totp.json", params: { name: "test" } + end + it "shows a helpful error message to the user" do + expect(JSON.parse(response.body)['error']).to eq(I18n.t("login.missing_second_factor_code")) + end + end + end + describe '#update_second_factor' do let(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) } @@ -3554,7 +3606,7 @@ describe UsersController do context 'when token is valid' do before do - ApplicationController.any_instance.stubs(:secure_session).returns("confirmed-password-#{user.id}" => "true") + stub_secure_session_confirmed end it 'should allow second factor for the user to be renamed' do put "/users/second_factor.json", params: { @@ -4062,7 +4114,11 @@ describe UsersController do def create_second_factor_security_key sign_in(user) - UsersController.any_instance.stubs(:secure_session_confirmed?).returns(true) + stub_secure_session_confirmed post "/u/create_second_factor_security_key.json" end + + def stub_secure_session_confirmed + UsersController.any_instance.stubs(:secure_session_confirmed?).returns(true) + end end diff --git a/test/javascripts/acceptance/preferences-test.js.es6 b/test/javascripts/acceptance/preferences-test.js.es6 index ce5d8f68b11..c651c678022 100644 --- a/test/javascripts/acceptance/preferences-test.js.es6 +++ b/test/javascripts/acceptance/preferences-test.js.es6 @@ -20,6 +20,16 @@ acceptance("User Preferences", { }); }); + server.post("/u/create_second_factor_security_key.json", () => { + return helper.response({ + challenge: + "a6d393d12654c130b2273e68ca25ca232d1d7f4c2464c2610fb8710a89d4", + rp_id: "localhost", + rp_name: "Discourse", + supported_algoriths: [-7, -257] + }); + }); + server.post("/u/enable_second_factor_totp.json", () => { return helper.response({ error: "invalid token" }); }); @@ -211,7 +221,7 @@ QUnit.test("connected accounts", async assert => { .indexOf("Connect") > -1; }); -QUnit.test("second factor", async assert => { +QUnit.test("second factor totp", async assert => { await visit("/u/eviltrout/preferences/second-factor"); assert.ok(exists("#password"), "it has a password input"); @@ -223,14 +233,36 @@ QUnit.test("second factor", async assert => { await click(".new-totp"); assert.ok(exists("#test-qr"), "shows qr code"); - await fillIn("#second-factor-token", "111111"); await click(".add-totp"); assert.ok( find(".alert-error") .html() - .indexOf("invalid token") > -1, - "shows server validation error message" + .indexOf("provide a name and the code") > -1, + "shows name/token missing error message" + ); +}); + +QUnit.test("second factor security keys", async assert => { + await visit("/u/eviltrout/preferences/second-factor"); + + assert.ok(exists("#password"), "it has a password input"); + + await fillIn("#password", "secrets"); + await click(".user-preferences .btn-primary"); + assert.notOk(exists("#password"), "it hides the password input"); + + await click(".new-security-key"); + assert.ok(exists("#security-key-name"), "shows security key name input"); + + fillIn("#security-key-name", ""); + await click(".add-security-key"); + + assert.ok( + find(".alert-error") + .html() + .indexOf("provide a name") > -1, + "shows name missing error message" ); });