Repository navigation
feat: add configurable challenge store for WebAuthn challenges #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
33d5583
2937591
936fbca
05e8ad1
dfbb8c2
316a49f
46aaa6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Devise | ||
| module Webauthn | ||
| module ChallengeStoreAccess | ||
| private | ||
|
|
||
| def challenge_store | ||
| store = Devise::Webauthn.challenge_store | ||
| store = Devise::Webauthn::ChallengeStores.const_get(store.to_s.camelize) if store.is_a?(Symbol) | ||
| store.new(request) | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Devise | ||
| module Webauthn | ||
| module ChallengeStores | ||
| class Session | ||
| KEYS = { | ||
| passkey_authentication: :authentication_challenge, | ||
| two_factor_authentication: :two_factor_authentication_challenge, | ||
| registration: :webauthn_challenge | ||
| }.freeze | ||
|
|
||
| def initialize(request) | ||
| @session = request.session | ||
| end | ||
|
|
||
| def write(purpose, challenge) | ||
| @session[KEYS.fetch(purpose)] = challenge | ||
| end | ||
|
|
||
| def pending?(purpose) | ||
| @session[KEYS.fetch(purpose)].present? | ||
| end | ||
|
|
||
| def consume(purpose) | ||
| @session.delete(KEYS.fetch(purpose)) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "spec_helper" | ||
|
|
||
| RSpec.describe Devise::Webauthn::ChallengeStoreAccess do | ||
| let(:request) { instance_double(ActionDispatch::Request, session: {}) } | ||
| let(:host) do | ||
| Class.new do | ||
| include Devise::Webauthn::ChallengeStoreAccess | ||
|
|
||
| attr_reader :request | ||
|
|
||
| def initialize(request) | ||
| @request = request | ||
| end | ||
|
|
||
| public :challenge_store | ||
| end.new(request) | ||
| end | ||
|
|
||
| around do |example| | ||
| original_store = Devise::Webauthn.challenge_store | ||
| example.run | ||
| ensure | ||
| Devise::Webauthn.challenge_store = original_store | ||
| end | ||
|
|
||
| it "uses the session store by default" do | ||
| expect(host.challenge_store).to be_a(Devise::Webauthn::ChallengeStores::Session) | ||
| end | ||
|
|
||
| it "resolves a symbol to the matching store" do | ||
| Devise::Webauthn.challenge_store = :session | ||
|
|
||
| expect(host.challenge_store).to be_a(Devise::Webauthn::ChallengeStores::Session) | ||
| end | ||
|
|
||
| it "uses a store class as given" do | ||
| Devise::Webauthn.challenge_store = MemoryChallengeStore | ||
|
|
||
| expect(host.challenge_store).to be_a(MemoryChallengeStore) | ||
| end | ||
|
|
||
| it "raises on an unknown symbol" do | ||
| Devise::Webauthn.challenge_store = :unknown | ||
|
|
||
| expect { host.challenge_store }.to raise_error(NameError) | ||
| end | ||
| end |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These aren't models, they're plain Ruby classes. Specs here mirror |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "spec_helper" | ||
|
|
||
| RSpec.describe Devise::Webauthn::ChallengeStores::Session do | ||
| let(:session) { {} } | ||
| let(:store) { described_class.new(instance_double(ActionDispatch::Request, session: session)) } | ||
|
|
||
| it "keeps each purpose under its own session key" do | ||
| store.write(:passkey_authentication, "passkey-challenge") | ||
| store.write(:two_factor_authentication, "2fa-challenge") | ||
| store.write(:registration, "registration-challenge") | ||
|
|
||
| expect(session).to eq( | ||
| authentication_challenge: "passkey-challenge", | ||
| two_factor_authentication_challenge: "2fa-challenge", | ||
| webauthn_challenge: "registration-challenge" | ||
| ) | ||
| end | ||
|
|
||
| it "reports a challenge as pending until it is consumed" do | ||
| store.write(:passkey_authentication, "challenge") | ||
|
|
||
| expect(store.pending?(:passkey_authentication)).to be(true) | ||
| expect(store.consume(:passkey_authentication)).to eq("challenge") | ||
| expect(store.pending?(:passkey_authentication)).to be(false) | ||
| end | ||
|
|
||
| it "returns nil when consuming a challenge that was never written" do | ||
| expect(store.consume(:registration)).to be_nil | ||
| end | ||
|
|
||
| it "raises on an unknown purpose" do | ||
| expect { store.write(:unknown, "challenge") }.to raise_error(KeyError) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a model spec? Should we move it to our
models/folder?Now that I think about it tho, shouldn't it be placed in
spec/models? Not to be done as part of this PR but just to spark the discussion :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't models, they're plain Ruby classes. Specs here mirror
lib/(spec/devise/webauthn/↔lib/devise/webauthn/,spec/devise/models/↔lib/devise/models/), so I'd leave them. Happy to discuss a different layout in a separate PR.