search

ruby-passkeys / devise-passkeys

Devise extension to use passkeys instead of passwords
MIT License
172 stars 13 forks source link

Remove default `add_module` for passkey strategy #48

Closed Billiam closed 1 year ago

Billiam commented 1 year ago

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: https://github.com/ruby-passkeys/devise-passkeys/blob/9a57c20900aebeef93441badfbc6211ff6a77531/lib/devise/passkeys.rb#L48-L51 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 commented 1 year ago

Fixing the tests in #49! Reviewing this now 💪

heliocola commented 1 year ago

@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 commented 1 year ago

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