lynndylanhurley / lynndylanhurley/devise_token_auth
Suggestion: 2FA - New credentials validation method | Warden strategy
- Dominant language
- Ruby
- Stars
- 3.6k
- Forks
- 1.1k
- PR merge metrics
- No merged PRs in 30d
Description
Hi guys,
I'm adding a 2FA with this gem but I have to completely copy the `create` action in the `sessions_controller` and it's dangerous because one may forget this in a future when updating `devise_token_auth` gem without updating our own copied code, and so a possibility of security vulnerabilities.
I read #1171 and #1280 but I think there could be another solution about implementing a 2FA with this gem.
### Option 1
https://github.com/lynndylanhurley/devise_token_auth/blob/eeed6422a025813010ac1dcb922661d05266e36e/app/controllers/devise_token_auth/sessions_controller.rb#L22-L30
I think before `create_and_assign_token`, there should have a `yield` block with the `@resource` to add another layer of credentials validation, but we cannot have more then 1 `yield` block per method. So maybe there should have a new method being called with `@resource` returning `true` (or `@resource`) by default and then we have the possibility to override it in our `sessions_controller` without overriding the whole controller but just that part. And then, if we want, we can render à `bad_credentials` if it fails to our 2FA.
### Option 2
https://github.com/lynndylanhurley/devise_token_auth/blob/eeed6422a025813010ac1dcb922661d05266e36e/app/controllers/devise_token_auth/sessions_controller.rb#L26-L28
I don't know if it really makes sense but we could swap `create_and_assign_token` and `sign_in(@resource, scope: :user, store: false, bypass: false)` positions and so we have the possibility of adding our own Warden strategy to be effective when calling `sign_in`. So if it fails, no tokens are created and it's also compatible with `devise` gem. Because right now, if we do add our Warden strategy, tokens are created for nothing and if it fails and that we have reached of max count tokens (e.i. 10 tokens per session), then it will badly remove the older one.
What do you think?
Contributor guide
Research direction
Start with app/controllers/devise_token_auth/sessions_controller.rb around create_and_assign_token and sign_in, then review issues #1171 and #1280. The issue presents two alternatives without selecting an implementation; progress requires deciding on an extension point that supports 2FA or Warden validation without copying the whole create action and avoids creating tokens before validation succeeds.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rails, ruby
- Domain
- api, authentication
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100