Use Devise configuration for auth_keys, instead of email

heliocola commented 1 year ago

Goal

Use the configuration value for Devise authentication_keys, instead of assuming :email

Included while reviewing `bundle exec rubocop`

Fixed a few rubocop issues
Added documentation to surrounding methods

Why

This allows apps that uses username (or any, other than email) as the Devise authentication_key to work

Outside of the scope of this PR

Add support to multiple authentication keys ([:username, :subdomain], [:email, :account_id], ...)

Still TODO

[ ] Add tests for Devise auth_key username
[x] Remove unnecessary resource_class method as Devise already provide one!
[ ] Add a default implementation of default_passkey_name (ref: https://github.com/ruby-passkeys/devise-passkeys/pull/50#discussion_r1314277788)

tcannonfodder commented 1 year ago

Oo a great find! Will review as soon as I'm able!

tcannonfodder commented 1 year ago

I left some comments, but the from my perspective, we'll need to tweak this PR to only add a default implementation for default_passkey_name, which will be used when registering a new passkey for an existing user. Registration of a new user is a different flow altogether, which I left some thoughts on in: https://github.com/ruby-passkeys/devise-passkeys/pull/50#discussion_r1314277788.

Thanks for digging into this problem! I hadn't thought to figure out how the gem fits with non-email Devise.authentication_keys configurations.

heliocola commented 1 year ago

@tcannonfodder : thank you for your comments, information, and suggestion.

Please check this PR: https://github.com/heliocola/devise-passkeys/pull/1 Is this inline of the changes you want for comment here ==> https://github.com/ruby-passkeys/devise-passkeys/pull/50#issuecomment-1704335778 ?

I left some comments, but the from my perspective, we'll need to tweak this PR to only add a default implementation for default_passkey_name, which will be used when registering a new passkey for an existing user. Registration of a new user is a different flow altogether, which I left some thoughts on in: #50 (comment).

Thanks for digging into this problem! I hadn't thought to figure out how the gem fits with non-email Devise.authentication_keys configurations.

If that is what you were looking for I can merge that PR and update this one. If not, we can push this to another PR as my initial intent for this PR is to allow devise-passkeys gem to use the auth key from devise, instead of hardcode to :email.

(side note: my initial comment was about the new user registration, which is a different code [like you mentioned])

After that I will use this PR to write some tests for an User with auth_key [:username] or [:account_id, :email]

tcannonfodder commented 1 year ago

Your questions actually led me to rethink the potential use of Devise.authentication_keys, see: https://github.com/ruby-passkeys/devise-passkeys/pull/50#discussion_r1322284848

tcannonfodder commented 11 months ago

Closing this PR since it seems like we actually won't use Devise.authentication_keys, see: https://github.com/ruby-passkeys/devise-passkeys/pull/50#discussion_r1326486875

ruby-passkeys / devise-passkeys