feat: follow REST standard for getting security key options

Author: nicolastemciuc

app/controllers/devise/second_factor_webauthn_credentials_controller.rb

@@ -2,8 +2,7 @@
 
 module Devise
   class SecondFactorWebauthnCredentialsController < DeviseController
-    before_action :authenticate_scope!, only: %i[new create update destroy options_for_create]
-    before_action :set_resource, only: :options_for_get
+    before_action :authenticate_scope!
 
     def new; end
 
@@ -43,39 +42,6 @@ def destroy
       redirect_to after_destroy_path
     end
 
-    def options_for_get
-      security_key_authentication_options =
-        WebAuthn::Credential.options_for_get(
-          allow: @resource.webauthn_credentials.pluck(:external_id),
-          user_verification: "discouraged"
-        )
-
-      # Store challenge in session for later verification
-      session[:two_factor_authentication_challenge] = security_key_authentication_options.challenge
-
-      render json: security_key_authentication_options
-    end
-
-    def options_for_create
-      create_security_key_options =
-        WebAuthn::Credential.options_for_create(
-          user: {
-            id: resource.webauthn_id,
-            name: resource_human_palatable_identifier
-          },
-          exclude: resource.webauthn_credentials.pluck(:external_id),
-          authenticator_selection: {
-            resident_key: "discouraged",
-            user_verification: "discouraged"
-          }
-        )
-
-      # Store challenge in session for later verification
-      session[:webauthn_challenge] = create_security_key_options.challenge
-
-      render json: create_security_key_options
-    end
-
     private
 
     def authenticate_scope!
@@ -113,16 +79,5 @@ def after_update_path
     def after_destroy_path
       new_second_factor_webauthn_credential_path(resource_name)
     end
-
-    def resource_human_palatable_identifier
-      authentication_keys = resource.class.authentication_keys
-      authentication_keys = authentication_keys.keys if authentication_keys.is_a?(Hash)
-
-      authentication_keys.filter_map { |authentication_key| resource.public_send(authentication_key) }.first
-    end
-
-    def set_resource
-      @resource = resource_class.find(session[:current_authentication_resource_id])
-    end
   end
 end

app/controllers/devise/security_key_authentication_options_controller.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module Devise
+  class SecurityKeyAuthenticationOptionsController < DeviseController
+    before_action :set_resource
+
+    def index
+      security_key_authentication_options =
+        WebAuthn::Credential.options_for_get(
+          allow: @resource.webauthn_credentials.pluck(:external_id),
+          user_verification: "discouraged"
+        )
+
+      # Store challenge in session for later verification
+      session[:two_factor_authentication_challenge] = security_key_authentication_options.challenge
+
+      render json: security_key_authentication_options
+    end
+
+    private
+
+    def set_resource
+      @resource = resource_class.find(session[:current_authentication_resource_id])
+    end
+  end
+end

app/controllers/devise/security_key_registration_options_controller.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module Devise
+  class SecurityKeyRegistrationOptionsController < DeviseController
+    before_action :authenticate_scope!
+
+    def index
+      create_security_key_options =
+        WebAuthn::Credential.options_for_create(
+          user: {
+            id: resource.webauthn_id,
+            name: resource_human_palatable_identifier
+          },
+          exclude: resource.webauthn_credentials.pluck(:external_id),
+          authenticator_selection: {
+            resident_key: "discouraged",
+            user_verification: "discouraged"
+          }
+        )
+
+      # Store challenge in session for later verification
+      session[:webauthn_challenge] = create_security_key_options.challenge
+
+      render json: create_security_key_options
+    end
+
+    private
+
+    def authenticate_scope!
+      send(:"authenticate_#{resource_name}!", force: true)
+      self.resource = send(:"current_#{resource_name}")
+    end
+
+    def resource_human_palatable_identifier
+      authentication_keys = resource.class.authentication_keys
+      authentication_keys = authentication_keys.keys if authentication_keys.is_a?(Hash)
+
+      authentication_keys.filter_map { |authentication_key| resource.public_send(authentication_key) }.first
+    end
+  end
+end

lib/devise/webauthn/helpers/credentials_helper.rb

@@ -38,7 +38,7 @@ def security_key_creation_form_for(resource, form_classes: nil, &block)
           class: form_classes
         ) do |f|
           tag.webauthn_create(
-            data: { options_url: options_for_create_second_factor_webauthn_credentials_path(resource) }
+            data: { options_url: security_key_registration_options_path(resource) }
           ) do
             concat f.hidden_field(:public_key_credential, data: { webauthn_target: "response" })
             concat capture(f, &block)
@@ -52,7 +52,7 @@ def login_with_security_key_button(text = nil, resource:, button_classes: nil, f
           method: :post,
           class: form_classes
         ) do |f|
-          tag.webauthn_get(data: { options_url: options_for_get_second_factor_webauthn_credentials_path(resource) }) do
+          tag.webauthn_get(data: { options_url: security_key_authentication_options_path(resource) }) do
             concat f.hidden_field(:public_key_credential, data: { webauthn_target: "response" })
             concat f.button(text, type: "submit", class: button_classes, &block)
           end

lib/devise/webauthn/routes.rb

@@ -20,10 +20,11 @@ def devise_two_factor_authentication(_mapping, controllers)
 
         resources :second_factor_webauthn_credentials,
                   only: %i[new create update destroy],
-                  controller: controllers[:second_factor_webauthn_credentials] do
-                    get :options_for_get, on: :collection
-                    get :options_for_create, on: :collection
-                  end
+                  controller: controllers[:second_factor_webauthn_credentials]
+        resources :security_key_authentication_options, only: %i[index],
+                                                        controller: controllers[:security_key_authentication_options]
+        resources :security_key_registration_options, only: %i[index],
+                                                      controller: controllers[:security_key_registration_options]
       end
     end
   end

lib/devise/webauthn/url_helpers.rb

@@ -27,8 +27,10 @@ module UrlHelpers
         passkey_authentication_options: [nil],
         passkey_registration_options: [nil],
         two_factor_authentication: [nil, :new],
-        second_factor_webauthn_credentials: [nil, :options_for_get, :options_for_create],
-        second_factor_webauthn_credential: [nil, :new]
+        second_factor_webauthn_credentials: [nil],
+        second_factor_webauthn_credential: [nil, :new],
+        security_key_authentication_options: [nil],
+        security_key_registration_options: [nil]
       }.each do |route, actions|
         %i[path url].each do |path_or_url|
           actions.each do |action|

lib/generators/devise/webauthn/templates/controllers/security_key_authentication_options_controller.rb.tt

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class <%= @scope_prefix %>SecurityKeyAuthenticationOptionsController < Devise::SecurityKeyAuthenticationOptionsController
+  # GET /resource/security_key_authentication_options
+  # def index
+  #   super
+  # end
+end

lib/generators/devise/webauthn/templates/controllers/security_key_registration_options_controller.rb.tt

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class <%= @scope_prefix %>SecurityKeyRegistrationOptionsController < Devise::SecurityKeyRegistrationOptionsController
+  # GET /resource/securiy_key_registration_options
+  # def index
+  #   super
+  # end
+end

spec/requests/devise/second_factor_webauthn_credentials_controller_spec.rb

@@ -41,7 +41,7 @@
     context "when user is authenticated" do
       before do
         sign_in user
-        get options_for_create_account_second_factor_webauthn_credentials_path # To set the challenge in session
+        get account_security_key_registration_options_path # To set the challenge in session
       end
 
       context "with valid parameters" do
@@ -157,89 +157,4 @@
       end
     end
   end
-
-  # rubocop:disable RSpec/MultipleExpectations
-  describe "GET #options_for_get" do
-    before do
-      user.webauthn_credentials.create!(
-        external_id: "second-factor-id",
-        name: "Second Factor Key",
-        public_key: "pk",
-        sign_count: 0,
-        authentication_factor: :second_factor
-      )
-
-      # rubocop:disable RSpec/AnyInstance
-      allow_any_instance_of(described_class)
-        .to receive(:set_resource) do |instance|
-          instance.instance_variable_set(:@resource, user)
-        end
-      # rubocop:enable RSpec/AnyInstance
-    end
-
-    it "returns authentication options and stores the challenge in the session" do
-      get options_for_get_account_second_factor_webauthn_credentials_path
-
-      expect(response).to have_http_status(:ok)
-      expect(response.media_type).to eq("application/json")
-
-      body = response.parsed_body
-      expect(body).to include("challenge")
-      expect(body["userVerification"]).to eq("discouraged")
-
-      expect(body["allowCredentials"].size).to eq(1)
-
-      expect(session[:two_factor_authentication_challenge]).to be_present
-    end
-  end
-
-  describe "GET #options_for_create" do
-    context "when user is authenticated" do
-      before do
-        sign_in user, scope: :account
-      end
-
-      it "returns security key creation options and stores the challenge in the session" do
-        get options_for_create_account_second_factor_webauthn_credentials_path
-
-        expect(response).to have_http_status(:ok)
-        expect(response.media_type).to eq("application/json")
-
-        body = response.parsed_body
-        expect(body).to include("challenge")
-        expect(body.dig("user", "name")).to eq(user.email)
-
-        expect(body.dig("authenticatorSelection", "residentKey")).to eq("discouraged")
-        expect(body.dig("authenticatorSelection", "userVerification")).to eq("discouraged")
-
-        expect(session[:webauthn_challenge]).to be_present
-      end
-
-      it "includes existing first-factor credentials in the excludeCredentials list" do
-        user.webauthn_credentials.create!(
-          external_id: "existing-id",
-          name: "Existing Key",
-          public_key: "pk",
-          sign_count: 0,
-          authentication_factor: :first_factor
-        )
-
-        get options_for_create_account_second_factor_webauthn_credentials_path
-
-        body = response.parsed_body
-        exclude_credentials = body["excludeCredentials"] || []
-
-        expect(exclude_credentials.size).to eq(user.webauthn_credentials.count)
-      end
-    end
-
-    context "when user is not authenticated" do
-      it "redirects to the sign-in page" do
-        get options_for_create_account_second_factor_webauthn_credentials_path
-
-        expect(response).to redirect_to(new_account_session_path)
-      end
-    end
-  end
-  # rubocop:enable RSpec/MultipleExpectations
 end

spec/requests/devise/security_key_authentication_options_controller_spec.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe Devise::SecurityKeyAuthenticationOptionsController, type: :request do
+  let(:user) { Account.create!(email: "test@example.com", password: "password123") }
+
+  describe "GET #index" do
+    before do
+      sign_in user, scope: :account
+    end
+
+    it "returns authentication options and stores the challenge in the session" do
+      get account_security_key_authentication_options_path
+
+      expect(response).to have_http_status(:ok)
+
+      body = response.parsed_body
+      expect(body).to include("challenge")
+
+      expect(session[:two_factor_authentication_challenge]).to eq(body["challenge"])
+    end
+  end
+end