FIX: Show a nicer error if name/code missing for TOTP/Security Keys (#9124)

Meta: https://meta.discourse.org/t/improve-error-message-when-not-including-name-setting-up-totp/143339

* when the user creates a TOTP second factor method we want
to show them a nicer error if they forget to add a name
or the code from the app, instead of the param missing error
* also add a client-side check for this and for security key name,
no need to bother the server if we can help it

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: {

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

app/assets/javascripts/discourse/templates/modal/second-factor-add-security-key.hbs

@@ -16,7 +16,7 @@
 
     <div class="control-group">
       <div class="controls">
-        {{input value=securityKeyName id='test' placeholder='security key name'}}
+        {{input value=securityKeyName id='security-key-name' placeholder='security key name'}}
       </div>
     </div>
 
@@ -24,7 +24,7 @@
       <div class="controls">
         {{#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}}

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}"]

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"

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"

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

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"
   );
 });