Skip to content
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

Remove default add_module for passkey strategy #48

Merged

Conversation

Billiam
Copy link
Contributor

@Billiam Billiam commented Aug 18, 2023

Proposed change

Remove the default call to Devise.add_module for the :passkey_authenticatable strategy.

Reasoning

The default call adds no_input: true for the strategy which effectively bypasses sign_in, as well as after_action controller filters.

Details

With no_input: true enabled for the passkey devise strategy it will be considered when checking for already-authenticated sessions in DeviseController::require_no_authentication.

When submitting valid passkey credentials to Sessions#create, require_no_authentication runs before the action and tries to authenticate against all of the strategies flagged with no_input. If it finds one, it halts, devise adds an already logged in flash message, and redirects to the after_sign_in_path_for(resource), instead of going through the session create action.

This bypasses any user-created after_action filters, and more importantly, the sign_in method, both of which may be important for rails apps.

The no_input option can't be changed in userland without manually altering devise constants, because the default Devise.add_module here:

Devise.add_module :passkey_authenticatable,
model: "devise/passkeys/model",
strategy: true,
no_input: true
mutates the Devise::NO_INPUT constant and a couple of others.

Since the readme currently calls out needing to call Devise.add_module manually during setup, I've just removed the offending add_module and changed the readme recommendation to not include no_input.

Tests

Tests pass using warden-webauthn = 0.2.1, but 0.3.0 adds a new default value (resident_key: required) that affects some tests.

@tcannonfodder
Copy link
Contributor

Fixing the tests in #49! Reviewing this now 💪

@tcannonfodder tcannonfodder self-requested a review August 19, 2023 02:02
Copy link
Contributor

@tcannonfodder tcannonfodder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks so much!

@tcannonfodder tcannonfodder merged commit 6940cc5 into ruby-passkeys:main Aug 19, 2023
1 of 2 checks passed
@heliocola
Copy link
Contributor

@tcannonfodder @Billiam : is having no_input: true a good default option?
From reading the description above it looks like that isn't.

The template app is still using (ref: https://github.com/ruby-passkeys/devise-passkeys-template/blob/main/app/models/user.rb#L18-L23) but the README.MD on devise-passkeys repo (ref https://github.com/ruby-passkeys/devise-passkeys/blob/main/README.md#reimplement-the-passkey_authenticatable-module) don't have it.

@tcannonfodder : I've removed no_input: true in an app I've been working for a few months, run all the regressions tests (on my devise app), and all the passkeys scenarios works the same way.

@tcannonfodder
Copy link
Contributor

Ah, the template app does need to be updated; thanks for catching that!

tcannonfodder added a commit to ruby-passkeys/devise-passkeys-template that referenced this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants