Repository navigation
feat: add configurable challenge store for WebAuthn challenges - #150
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e4e111b to
8b7958b
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f59cbd8 to
3de9e63
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c41508e to
77331d8
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
77331d8 to
a541487
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a541487 to
176c904
Compare
Controllers and strategies now read and write challenges through `Devise::Webauthn.challenge_store`. The default session store keeps the existing session keys, so behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Controllers and strategies include `Devise::Webauthn::ChallengeStoreAccess` instead of calling `Devise::Webauthn.challenge_store_for(request)`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The challenge is consumed inside `verify_and_save_*`, which is skipped when parsing the credential raises or when an app overrides it. The `ensure` restores the previous guarantee that the challenge is always cleared. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
176c904 to
05e8ad1
Compare
santiagorodriguez96
left a comment
There was a problem hiding this comment.
Looking really good!!!
| @session[KEYS.fetch(purpose)] = challenge | ||
| end | ||
|
|
||
| def pending?(purpose, _credential_json) |
There was a problem hiding this comment.
Why the _credential_json param if we are not going to use it?
There was a problem hiding this comment.
Hmmmm I see...
In that case I think I would hold off these changes and instead include them in #152 as I think they can be confusing here. I think that would give clarity about what we are including those params in this methods. What's more, I'd be able to explore different approaches and discuss them in that PR to avoid having to do that.
What do you think?
Rename the verified challenge to expected_challenge and the unused store argument to _credential. Move the custom store specs into the passkey authentication spec and extract MemoryChallengeStore to spec/support. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| @session[KEYS.fetch(purpose)] = challenge | ||
| end | ||
|
|
||
| def pending?(purpose, _credential_json) |
There was a problem hiding this comment.
Hmmmm I see...
In that case I think I would hold off these changes and instead include them in #152 as I think they can be confusing here. I think that would give clarity about what we are including those params in this methods. What's more, I'd be able to explore different approaches and discuss them in that PR to avoid having to do that.
What do you think?
| end | ||
| end | ||
|
|
||
| describe "sign-in with passkeys through a custom challenge store" do |
There was a problem hiding this comment.
I've been thinking about the new sign-in with passkeys through a custom challenge store block, and I wonder if we could fold it into the existing specs 🤔
The existing specs still pass, but only because :session is the default. They read session directly, so they test the session store's internals, not the challenge store interface.
What if we merge the new block with the existing examples, like this?
- Set the specs to use
MemoryChallengeStoreexplicitly, so nothing passes just because of the default store. - Check challenges through
challenge_storeinstead of the session
There was a problem hiding this comment.
Done in 46aaa6c. The passkey authentication examples now run against MemoryChallengeStore and read challenges through the store.
…face Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
santiagorodriguez96
left a comment
There was a problem hiding this comment.
Overall looks good to me!
Let's keep iterating over this 🙂
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
…request Stores already receive the request, so callers no longer pass the credential and the store interface stays as merged in #150. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What: Adds
Devise::Webauthn.challenge_storeso WebAuthn challenges can be kept somewhere other than the session. The default session store keeps today's keys and behaviour.Why: This is the first step toward passkey sign-in from API clients such as React Native with devise-jwt, which have no cookie session. A cache-backed store follows in the next PR.
How to test:
bundle exec rspec.passkey_authentication_spec.rbsigns in throughMemoryChallengeStore, a store that doesn't use the session.