From b89d7a793cd852ef6078e33b7d951589a30b1508 Mon Sep 17 00:00:00 2001 From: Thomas Cannon Date: Fri, 23 Jun 2023 23:45:21 -0400 Subject: [PATCH 1/8] `Sessions/ReauthenticationControllerConcern`: use `WebAuthn::RackHelper` * In order to prevent a bleed-through between `warden-webauthn` and `devise-passkeys`, we need to use the new `Warden::WebAuthn::RackHelper` in order to get the `relying_party_key` that it defines * This prevents confusion & accidental over-inclusion that was caused by including the entire `Warden::WebAuthn::StrategyHelpers` module --- .../passkeys/controllers/reauthentication_controller_concern.rb | 2 +- lib/devise/passkeys/controllers/sessions_controller_concern.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb b/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb index 8d1aed7..99f82fc 100644 --- a/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb +++ b/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb @@ -10,7 +10,7 @@ module ReauthenticationControllerConcern include Devise::Passkeys::Controllers::Concerns::Reauthentication include Devise::Passkeys::Controllers::Concerns::ReauthenticationChallenge include Warden::WebAuthn::AuthenticationInitiationHelpers - include Warden::WebAuthn::StrategyHelpers + include Warden::WebAuthn::RackHelpers prepend_before_action :authenticate_scope! diff --git a/lib/devise/passkeys/controllers/sessions_controller_concern.rb b/lib/devise/passkeys/controllers/sessions_controller_concern.rb index 9f4e5ed..845f5ec 100644 --- a/lib/devise/passkeys/controllers/sessions_controller_concern.rb +++ b/lib/devise/passkeys/controllers/sessions_controller_concern.rb @@ -8,7 +8,7 @@ module SessionsControllerConcern included do include Warden::WebAuthn::AuthenticationInitiationHelpers - include Warden::WebAuthn::StrategyHelpers + include Warden::WebAuthn::RackHelpers # Prepending is crucial to ensure that the relying party is set in the # request.env before the strategy is executed From bc2d4c71cd4f8e5aa2c4f25336c8b7578bc9f006 Mon Sep 17 00:00:00 2001 From: Thomas Cannon Date: Fri, 23 Jun 2023 23:47:18 -0400 Subject: [PATCH 2/8] Refactor `PasskeysControllerConcern` to handle bad `parsed_credential` * In order to prevent a bleed-through between `warden-webauthn` and `devise-passkeys`, we need to add error-handling inside of `verify_passkey_challenge` instead of including the entire `Warden::WebAuthn::StrategyHelpers` * This prevents confusion & accidental over-inclusion that was caused by including the entire `Warden::WebAuthn::StrategyHelpers` module --- .../controllers/passkeys_controller_concern.rb | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/devise/passkeys/controllers/passkeys_controller_concern.rb b/lib/devise/passkeys/controllers/passkeys_controller_concern.rb index ae1e59e..cf866a9 100644 --- a/lib/devise/passkeys/controllers/passkeys_controller_concern.rb +++ b/lib/devise/passkeys/controllers/passkeys_controller_concern.rb @@ -11,7 +11,6 @@ module PasskeysControllerConcern include Devise::Passkeys::Controllers::Concerns::ReauthenticationChallenge include Warden::WebAuthn::AuthenticationInitiationHelpers include Warden::WebAuthn::RegistrationHelpers - include Warden::WebAuthn::StrategyHelpers prepend_before_action :authenticate_scope! before_action :ensure_at_least_one_passkey, only: %i[new_destroy_challenge destroy] @@ -94,11 +93,14 @@ def user_details_for_registration end def verify_passkey_challenge - if parsed_credential.nil? - render json: { message: find_message(:credential_missing_or_could_not_be_parsed) }, status: :bad_request - delete_registration_challenge - return false + begin + if parsed_credential.nil? + return render_credential_missing_or_could_not_be_parsed_error + end + rescue JSON::JSONError, TypeError + return render_credential_missing_or_could_not_be_parsed_error end + begin @webauthn_credential = verify_registration(relying_party: relying_party) rescue ::WebAuthn::Error => e @@ -134,6 +136,12 @@ def verify_reauthentication_token def reauthentication_params params.require(:passkey).permit(:reauthentication_token) end + + def render_credential_missing_or_could_not_be_parsed_error + render json: { message: find_message(:credential_missing_or_could_not_be_parsed) }, status: :bad_request + delete_registration_challenge + return false + end end end end From 258a40c28e05a995f8575b19a73a1419061c39d0 Mon Sep 17 00:00:00 2001 From: Thomas Cannon Date: Fri, 23 Jun 2023 23:52:17 -0400 Subject: [PATCH 3/8] Update `CHANGELOG.md` --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 022b07b..d6543cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## [Unreleased] +- Refactor Controller concerns to not use `Warden::WebAuthn::StrategyHelpers` + - https://github.com/ruby-passkeys/devise-passkeys/pull/29 - Rename `Devise::Passkeys::Controllers::Concerns::PasskeyReauthentication` => `Devise::Passkeys::Controllers::Concerns::Reauthentication` - https://github.com/ruby-passkeys/devise-passkeys/pull/7/ - Bump `Devise` requirement to `>= 4.7.1` From 7b7d50129ebe83b0a224d0ace0e4cff8ea407f4a Mon Sep 17 00:00:00 2001 From: Thomas Cannon Date: Sat, 24 Jun 2023 00:07:09 -0400 Subject: [PATCH 4/8] Use `Warden::WebAuthn::RackHelper.set_relying_party_in_request_env` * The definitions from the README were added directly to `warden-webauthn`'s `Warden::WebAuthn::RackHelper` * see: https://github.com/ruby-passkeys/warden-webauthn/pull/5/commits/80d2101cdd50e58cd623c8ce3b17ff47b1c9d85f * Therefore, we can remove this code and rely on the `RackHelper` to define the method for us --- README.md | 12 ------------ .../reauthentication_controller_concern.rb | 2 +- .../controllers/sessions_controller_concern.rb | 2 +- .../controllers/test_passkeys_controller_concern.rb | 4 ---- .../test_reauthentication_controller_concern.rb | 4 ---- .../test_registrations_controller_concern.rb | 4 ---- .../controllers/test_sessions_controller_concern.rb | 8 -------- 7 files changed, 2 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 113b1a5..174e9b1 100644 --- a/README.md +++ b/README.md @@ -96,10 +96,6 @@ class Users::SessionsController < Devise::SessionsController def relying_party WebAuthn::RelyingParty.new(...) end - - def set_relying_party_in_request_env - request.env[relying_party_key] = relying_party - end end # frozen_string_literal: true @@ -111,10 +107,6 @@ class Users::ReauthenticationController < DeviseController def relying_party WebAuthn::RelyingParty.new(...) end - - def set_relying_party_in_request_env - request.env[relying_party_key] = relying_party - end end # frozen_string_literal: true @@ -126,10 +118,6 @@ class Users::PasskeysController < DeviseController def relying_party WebAuthn::RelyingParty.new(...) end - - def set_relying_party_in_request_env - request.env[relying_party_key] = relying_party - end end ``` diff --git a/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb b/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb index 99f82fc..c67a28d 100644 --- a/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb +++ b/lib/devise/passkeys/controllers/reauthentication_controller_concern.rb @@ -69,7 +69,7 @@ def delete_reauthentication_challenge session.delete(passkey_reauthentication_challenge_session_key) end - def set_relying_party_in_request_env + def relying_party raise "need to define relying_party for this SessionsController" end end diff --git a/lib/devise/passkeys/controllers/sessions_controller_concern.rb b/lib/devise/passkeys/controllers/sessions_controller_concern.rb index 845f5ec..7e62c05 100644 --- a/lib/devise/passkeys/controllers/sessions_controller_concern.rb +++ b/lib/devise/passkeys/controllers/sessions_controller_concern.rb @@ -29,7 +29,7 @@ def new_challenge protected - def set_relying_party_in_request_env + def relying_party raise "need to define relying_party for this SessionsController" end end diff --git a/test/devise/passkeys/controllers/test_passkeys_controller_concern.rb b/test/devise/passkeys/controllers/test_passkeys_controller_concern.rb index 206c5ad..6278f6d 100644 --- a/test/devise/passkeys/controllers/test_passkeys_controller_concern.rb +++ b/test/devise/passkeys/controllers/test_passkeys_controller_concern.rb @@ -18,10 +18,6 @@ def relying_party WebAuthn::RelyingParty.new(origin: "https://www.example.com") end - def set_relying_party_in_request_env - request.env[relying_party_key] = relying_party - end - def resource_name :user end diff --git a/test/devise/passkeys/controllers/test_reauthentication_controller_concern.rb b/test/devise/passkeys/controllers/test_reauthentication_controller_concern.rb index 5f4f98f..5961914 100644 --- a/test/devise/passkeys/controllers/test_reauthentication_controller_concern.rb +++ b/test/devise/passkeys/controllers/test_reauthentication_controller_concern.rb @@ -18,10 +18,6 @@ def relying_party WebAuthn::RelyingParty.new(origin: "https://www.example.com") end - def set_relying_party_in_request_env - request.env[relying_party_key] = relying_party - end - def resource_name :user end diff --git a/test/devise/passkeys/controllers/test_registrations_controller_concern.rb b/test/devise/passkeys/controllers/test_registrations_controller_concern.rb index 8e61ce9..2800a41 100644 --- a/test/devise/passkeys/controllers/test_registrations_controller_concern.rb +++ b/test/devise/passkeys/controllers/test_registrations_controller_concern.rb @@ -16,10 +16,6 @@ def relying_party WebAuthn::RelyingParty.new(origin: "https://www.example.com") end - def set_relying_party_in_request_env - request.env[relying_party_key] = relying_party - end - # Dummy action to setup reauthentication token def reauthenticate store_reauthentication_token_in_session diff --git a/test/devise/passkeys/controllers/test_sessions_controller_concern.rb b/test/devise/passkeys/controllers/test_sessions_controller_concern.rb index c353261..b77e86f 100644 --- a/test/devise/passkeys/controllers/test_sessions_controller_concern.rb +++ b/test/devise/passkeys/controllers/test_sessions_controller_concern.rb @@ -13,10 +13,6 @@ def relying_party WebAuthn::RelyingParty.new(origin: "test.host") end - def set_relying_party_in_request_env - request.env[relying_party_key] = relying_party - end - def resource_name :user end @@ -55,10 +51,6 @@ def relying_party WebAuthn::RelyingParty.new(origin: "test.host") end - def set_relying_party_in_request_env - request.env[relying_party_key] = relying_party - end - def resource_name "user" end From d825ffded91aa98801bdd5530442761aa60538f9 Mon Sep 17 00:00:00 2001 From: Thomas Cannon Date: Sat, 24 Jun 2023 15:18:34 -0400 Subject: [PATCH 5/8] Bump to `warden-webauthn` `0.2.1` --- Gemfile.lock | 4 ++-- devise-passkeys.gemspec | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 1ff91f6..a6738be 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -3,7 +3,7 @@ PATH specs: devise-passkeys (0.1.0) devise (>= 4.7.1) - warden-webauthn (>= 0.2.0) + warden-webauthn (>= 0.2.1) GEM remote: https://rubygems.org/ @@ -157,7 +157,7 @@ GEM unicode-display_width (2.4.2) warden (1.2.9) rack (>= 2.0.9) - warden-webauthn (0.2.0) + warden-webauthn (0.2.1) warden webauthn (>= 3) webauthn (3.0.0) diff --git a/devise-passkeys.gemspec b/devise-passkeys.gemspec index edf5d36..3a9b2bc 100644 --- a/devise-passkeys.gemspec +++ b/devise-passkeys.gemspec @@ -33,7 +33,7 @@ Gem::Specification.new do |spec| # Uncomment to register a new dependency of your gem spec.add_dependency "devise", ">= 4.7.1" - spec.add_dependency "warden-webauthn", ">= 0.2.0" + spec.add_dependency "warden-webauthn", ">= 0.2.1" # For more information and examples about making a new gem, check out our # guide at: https://bundler.io/guides/creating_gem.html From f1400cb4b217c20b9e74fda3f55f74284e373d25 Mon Sep 17 00:00:00 2001 From: Thomas Cannon Date: Sat, 24 Jun 2023 15:24:35 -0400 Subject: [PATCH 6/8] Refactor `PasskeysControllerConcern` to have clearer credential verify * Moving the preamble checks that the `parsed_credential` is valid into its own `verify_credential_integrity` `before_action` helps keep the `verify_passkey_challenge` focused, since verifying the credential's integrity is a separate step (making sure it's not blank and is parseable JSON) --- .../passkeys_controller_concern.rb | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/devise/passkeys/controllers/passkeys_controller_concern.rb b/lib/devise/passkeys/controllers/passkeys_controller_concern.rb index cf866a9..5a38dca 100644 --- a/lib/devise/passkeys/controllers/passkeys_controller_concern.rb +++ b/lib/devise/passkeys/controllers/passkeys_controller_concern.rb @@ -16,6 +16,7 @@ module PasskeysControllerConcern before_action :ensure_at_least_one_passkey, only: %i[new_destroy_challenge destroy] before_action :find_passkey, only: %i[new_destroy_challenge destroy] + before_action :verify_credential_integrity, only: [:create] before_action :verify_passkey_challenge, only: [:create] before_action :verify_reauthentication_token, only: %i[create destroy] @@ -92,21 +93,17 @@ def user_details_for_registration { id: resource.webauthn_id, name: resource.email } end - def verify_passkey_challenge - begin - if parsed_credential.nil? - return render_credential_missing_or_could_not_be_parsed_error - end - rescue JSON::JSONError, TypeError - return render_credential_missing_or_could_not_be_parsed_error - end + def verify_credential_integrity + return render_credential_missing_or_could_not_be_parsed_error if parsed_credential.nil? + rescue JSON::JSONError, TypeError + return render_credential_missing_or_could_not_be_parsed_error + end - begin - @webauthn_credential = verify_registration(relying_party: relying_party) - rescue ::WebAuthn::Error => e - error_key = Warden::WebAuthn::ErrorKeyFinder.webauthn_error_key(exception: e) - render json: { message: find_message(error_key) }, status: :bad_request - end + def verify_passkey_challenge + @webauthn_credential = verify_registration(relying_party: relying_party) + rescue ::WebAuthn::Error => e + error_key = Warden::WebAuthn::ErrorKeyFinder.webauthn_error_key(exception: e) + render json: { message: find_message(error_key) }, status: :bad_request end def passkey_params From 64e0b85815479c267fbc6938610b15cc8db1a483 Mon Sep 17 00:00:00 2001 From: Thomas Cannon Date: Sat, 24 Jun 2023 15:33:26 -0400 Subject: [PATCH 7/8] Update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6543cb..fc783b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## [Unreleased] +- [Bump to warden-webauthn 0.2.1](https://github.com/ruby-passkeys/devise-passkeys/pull/29/commits/d825ffded91aa98801bdd5530442761aa60538f9) +- [Refactor PasskeysControllerConcern to have clearer credential verify with `verify_credential_integrity`](https://github.com/ruby-passkeys/devise-passkeys/pull/29/commits/f1400cb4b217c20b9e74fda3f55f74284e373d25) - Refactor Controller concerns to not use `Warden::WebAuthn::StrategyHelpers` - https://github.com/ruby-passkeys/devise-passkeys/pull/29 - Rename `Devise::Passkeys::Controllers::Concerns::PasskeyReauthentication` => `Devise::Passkeys::Controllers::Concerns::Reauthentication` From a64f3ba946cd25795265423f6b16afc53893783b Mon Sep 17 00:00:00 2001 From: Thomas Cannon Date: Sat, 24 Jun 2023 15:37:26 -0400 Subject: [PATCH 8/8] Add missing change in `CHANGELOG` --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc783b0..0112d01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## [Unreleased] - [Bump to warden-webauthn 0.2.1](https://github.com/ruby-passkeys/devise-passkeys/pull/29/commits/d825ffded91aa98801bdd5530442761aa60538f9) +- [Use `Warden::WebAuthn::RackHelper.set_relying_party_in_request_env` to streamline setup](https://github.com/ruby-passkeys/devise-passkeys/pull/29/commits/7b7d50129ebe83b0a224d0ace0e4cff8ea407f4a) - [Refactor PasskeysControllerConcern to have clearer credential verify with `verify_credential_integrity`](https://github.com/ruby-passkeys/devise-passkeys/pull/29/commits/f1400cb4b217c20b9e74fda3f55f74284e373d25) - Refactor Controller concerns to not use `Warden::WebAuthn::StrategyHelpers` - https://github.com/ruby-passkeys/devise-passkeys/pull/29