From 71102452d5235a78d439db0fc5a84bb634962af3 Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Sat, 28 Sep 2024 16:27:36 +0200 Subject: [PATCH 1/9] Do Not Create User If Registration Fails --- app/controllers/registrations_controller.rb | 4 ++-- test/controllers/registrations_controller_test.rb | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 9ac6fe9..b2fe38b 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -29,7 +29,7 @@ def create end def callback - user = User.create!(session[:current_registration][:user_attributes]) + user = User.new(session[:current_registration][:user_attributes]) begin webauthn_credential = relying_party.verify_registration( @@ -45,7 +45,7 @@ def callback sign_count: webauthn_credential.sign_count ) - if credential.save + if user.save sign_in(user) render json: { status: "ok" }, status: :ok diff --git a/test/controllers/registrations_controller_test.rb b/test/controllers/registrations_controller_test.rb index ef8aadf..a76db94 100644 --- a/test/controllers/registrations_controller_test.rb +++ b/test/controllers/registrations_controller_test.rb @@ -55,10 +55,12 @@ class RegistrationsControllerTest < ActionDispatch::IntegrationTest ] ) - post( - callback_registration_url, - params: { credential_nickname: "USB Key" }.merge(public_key_credential) - ) + assert_no_difference -> { User.count } do + post( + callback_registration_url, + params: { credential_nickname: "USB Key" }.merge(public_key_credential) + ) + end assert_response :unprocessable_entity assert_equal "Couldn't register your Security Key", response.body From 15c660bd6cf84d83c64e755ada661746986a2803 Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Sat, 28 Sep 2024 17:32:06 +0200 Subject: [PATCH 2/9] Check the Creation of User & Credential When Registered Successfully --- .../registrations_controller_test.rb | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/controllers/registrations_controller_test.rb b/test/controllers/registrations_controller_test.rb index a76db94..e8a7a53 100644 --- a/test/controllers/registrations_controller_test.rb +++ b/test/controllers/registrations_controller_test.rb @@ -65,4 +65,49 @@ class RegistrationsControllerTest < ActionDispatch::IntegrationTest assert_response :unprocessable_entity assert_equal "Couldn't register your Security Key", response.body end + + test "should register successfully" do + authenticator = WebAuthn::FakeAuthenticator.new + + fake_rp = WebAuthn::RelyingParty.new( + origin: "https://fake.relying_party.test", + id: "fake.relying_party.test", + name: "Fake RelyingParty" + ) + + fake_client = WebAuthn::FakeClient.new("https://fake.relying_party.test", authenticator:) + + user = User.new(username: "John Doe") + + raw_challenge = SecureRandom.random_bytes(32) + challenge = WebAuthn.configuration.encoder.encode(raw_challenge) + + webauthn_credential = fake_client.create(challenge:, rp_id: fake_rp.id, user_verified: true) + + session_data = { + current_registration: { + challenge:, + user_attributes: user.attributes + } + } + + any_instance_of(RegistrationsController) do |klass| + stub(klass).session { session_data } + end + + any_instance_of(ApplicationController) do |klass| + stub(klass).relying_party { fake_rp } + end + + assert_difference 'User.count', +1 do + assert_difference 'Credential.count', +1 do + post( + callback_registration_url, + params: { credential_nickname: "USB Key" }.merge(webauthn_credential) + ) + end + end + + assert_response :success + end end From 1cfe78eb3a8f8749d72e824b4ca1f3b04fc457a5 Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Sat, 28 Sep 2024 17:35:40 +0200 Subject: [PATCH 3/9] Add RR Gem --- Gemfile | 1 + Gemfile.lock | 2 ++ 2 files changed, 3 insertions(+) diff --git a/Gemfile b/Gemfile index 8f9d780..a093b51 100644 --- a/Gemfile +++ b/Gemfile @@ -37,4 +37,5 @@ group :test do gem 'capybara', '~> 3.26' gem 'minitest-stub_any_instance', '~> 1.0' gem 'selenium-webdriver', '~> 4.25' + gem 'rr' end diff --git a/Gemfile.lock b/Gemfile.lock index bad58a3..016a789 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -241,6 +241,7 @@ GEM io-console (~> 0.5) rexml (3.3.7) rollbar (3.6.0) + rr (3.1.1) rubocop (1.66.1) json (~> 2.3) language_server-protocol (>= 3.17.0) @@ -342,6 +343,7 @@ DEPENDENCIES rack-mini-profiler (~> 3.3) rails (~> 7.1.3) rollbar (~> 3.6) + rr rubocop (~> 1.66) rubocop-rails (~> 2.26) sassc-rails (~> 2.0) From c50438f910bf52391626c2e094bfd1963d4e5ac6 Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Sat, 28 Sep 2024 20:03:39 +0200 Subject: [PATCH 4/9] Revert "Add RR Gem" This reverts commit 1cfe78eb3a8f8749d72e824b4ca1f3b04fc457a5. --- Gemfile | 1 - Gemfile.lock | 2 -- 2 files changed, 3 deletions(-) diff --git a/Gemfile b/Gemfile index a093b51..8f9d780 100644 --- a/Gemfile +++ b/Gemfile @@ -37,5 +37,4 @@ group :test do gem 'capybara', '~> 3.26' gem 'minitest-stub_any_instance', '~> 1.0' gem 'selenium-webdriver', '~> 4.25' - gem 'rr' end diff --git a/Gemfile.lock b/Gemfile.lock index 016a789..bad58a3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -241,7 +241,6 @@ GEM io-console (~> 0.5) rexml (3.3.7) rollbar (3.6.0) - rr (3.1.1) rubocop (1.66.1) json (~> 2.3) language_server-protocol (>= 3.17.0) @@ -343,7 +342,6 @@ DEPENDENCIES rack-mini-profiler (~> 3.3) rails (~> 7.1.3) rollbar (~> 3.6) - rr rubocop (~> 1.66) rubocop-rails (~> 2.26) sassc-rails (~> 2.0) From 1afd495b0857bb69b167822a98bc4d2c2d5e47f9 Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Sat, 28 Sep 2024 20:04:01 +0200 Subject: [PATCH 5/9] Use minitest-stub_any_instance --- .../registrations_controller_test.rb | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/test/controllers/registrations_controller_test.rb b/test/controllers/registrations_controller_test.rb index e8a7a53..f16d190 100644 --- a/test/controllers/registrations_controller_test.rb +++ b/test/controllers/registrations_controller_test.rb @@ -91,20 +91,16 @@ class RegistrationsControllerTest < ActionDispatch::IntegrationTest } } - any_instance_of(RegistrationsController) do |klass| - stub(klass).session { session_data } - end - - any_instance_of(ApplicationController) do |klass| - stub(klass).relying_party { fake_rp } - end - - assert_difference 'User.count', +1 do - assert_difference 'Credential.count', +1 do - post( - callback_registration_url, - params: { credential_nickname: "USB Key" }.merge(webauthn_credential) - ) + ApplicationController.stub_any_instance(:relying_party, fake_rp) do + RegistrationsController.stub_any_instance(:session, session_data) do + assert_difference 'User.count', +1 do + assert_difference 'Credential.count', +1 do + post( + callback_registration_url, + params: { credential_nickname: "USB Key" }.merge(webauthn_credential) + ) + end + end end end From e42c372fbc18e9f73b48e4fe7ea65d7a3a22e634 Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Fri, 11 Oct 2024 13:06:01 +0200 Subject: [PATCH 6/9] Do No Use Mix Hash Access --- app/controllers/registrations_controller.rb | 5 +++-- app/controllers/sessions_controller.rb | 7 ++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index f10537f..86c0867 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -29,12 +29,13 @@ def create end def callback - user = User.new(session[:current_registration]["user_attributes"]) + current_registration = session[:current_registration].deep_symbolize_keys + user = User.new(current_registration[:user_attributes]) begin webauthn_credential = relying_party.verify_registration( params, - session[:current_registration]["challenge"], + current_registration[:challenge], user_verification: true, ) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 8ecc458..7a4fc46 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -26,13 +26,14 @@ def create end def callback - user = User.find_by(username: session[:current_authentication]["username"]) - raise "user #{session[:current_authentication]["username"]} never initiated sign up" unless user + current_registration = session[:current_registration].deep_symbolize_keys + user = User.find_by(username: current_registration[:username]) + raise "user #{current_registration[:username]} never initiated sign up" unless user begin verified_webauthn_credential, stored_credential = relying_party.verify_authentication( params, - session[:current_authentication]["challenge"], + current_registration[:challenge], user_verification: true, ) do |webauthn_credential| user.credentials.find_by(external_id: Base64.strict_encode64(webauthn_credential.raw_id)) From 3f0f587506c0d5815eaa99411b352057b7105a7a Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Fri, 11 Oct 2024 13:06:09 +0200 Subject: [PATCH 7/9] Fix Rubocop --- app/controllers/registrations_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 86c0867..f7c5703 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -39,7 +39,7 @@ def callback user_verification: true, ) - credential = user.credentials.build( + user.credentials.build( external_id: Base64.strict_encode64(webauthn_credential.raw_id), nickname: params[:credential_nickname], public_key: webauthn_credential.public_key, From e7dea69d3c7a55a7acbc44ba87caec19899e47fc Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Fri, 11 Oct 2024 13:09:28 +0200 Subject: [PATCH 8/9] Make Test More Consistent --- .../registrations_controller_test.rb | 46 +++++++------------ 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/test/controllers/registrations_controller_test.rb b/test/controllers/registrations_controller_test.rb index f16d190..fafc832 100644 --- a/test/controllers/registrations_controller_test.rb +++ b/test/controllers/registrations_controller_test.rb @@ -67,40 +67,26 @@ class RegistrationsControllerTest < ActionDispatch::IntegrationTest end test "should register successfully" do - authenticator = WebAuthn::FakeAuthenticator.new - - fake_rp = WebAuthn::RelyingParty.new( - origin: "https://fake.relying_party.test", - id: "fake.relying_party.test", - name: "Fake RelyingParty" - ) + raw_challenge = SecureRandom.random_bytes(32) + challenge = WebAuthn.configuration.encoder.encode(raw_challenge) - fake_client = WebAuthn::FakeClient.new("https://fake.relying_party.test", authenticator:) + WebAuthn::PublicKeyCredential::CreationOptions.stub_any_instance(:raw_challenge, raw_challenge) do + post registration_url, params: { registration: { username: "alice" }, format: :json } - user = User.new(username: "John Doe") + assert_response :success + end - raw_challenge = SecureRandom.random_bytes(32) - challenge = WebAuthn.configuration.encoder.encode(raw_challenge) + public_key_credential = + WebAuthn::FakeClient + .new(Rails.configuration.webauthn_origin) + .create(challenge:, user_verified: true) - webauthn_credential = fake_client.create(challenge:, rp_id: fake_rp.id, user_verified: true) - - session_data = { - current_registration: { - challenge:, - user_attributes: user.attributes - } - } - - ApplicationController.stub_any_instance(:relying_party, fake_rp) do - RegistrationsController.stub_any_instance(:session, session_data) do - assert_difference 'User.count', +1 do - assert_difference 'Credential.count', +1 do - post( - callback_registration_url, - params: { credential_nickname: "USB Key" }.merge(webauthn_credential) - ) - end - end + assert_difference 'User.count', +1 do + assert_difference 'Credential.count', +1 do + post( + callback_registration_url, + params: { credential_nickname: "USB Key" }.merge(public_key_credential) + ) end end From 726e3acf4d3bc7f09ed17b5fe47fe162941019b6 Mon Sep 17 00:00:00 2001 From: Joel AZEMAR Date: Mon, 14 Oct 2024 16:35:15 +0200 Subject: [PATCH 9/9] Revert "Do No Use Mix Hash Access" This reverts commit e42c372fbc18e9f73b48e4fe7ea65d7a3a22e634. --- app/controllers/registrations_controller.rb | 5 ++--- app/controllers/sessions_controller.rb | 7 +++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index f7c5703..607ec8d 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -29,13 +29,12 @@ def create end def callback - current_registration = session[:current_registration].deep_symbolize_keys - user = User.new(current_registration[:user_attributes]) + user = User.new(session[:current_registration]["user_attributes"]) begin webauthn_credential = relying_party.verify_registration( params, - current_registration[:challenge], + session[:current_registration]["challenge"], user_verification: true, ) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 7a4fc46..8ecc458 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -26,14 +26,13 @@ def create end def callback - current_registration = session[:current_registration].deep_symbolize_keys - user = User.find_by(username: current_registration[:username]) - raise "user #{current_registration[:username]} never initiated sign up" unless user + user = User.find_by(username: session[:current_authentication]["username"]) + raise "user #{session[:current_authentication]["username"]} never initiated sign up" unless user begin verified_webauthn_credential, stored_credential = relying_party.verify_authentication( params, - current_registration[:challenge], + session[:current_authentication]["challenge"], user_verification: true, ) do |webauthn_credential| user.credentials.find_by(external_id: Base64.strict_encode64(webauthn_credential.raw_id))