lynndylanhurley / devise_token_auth

Token based authentication for Rails JSON APIs. Designed to work with jToker and ng-token-auth.
Do What The F*ck You Want To Public License
3.52k stars 1.14k forks source link

Suggestion: 2FA - New credentials validation method | Warden strategy #1605

Open newfylox opened 11 months ago

newfylox commented 11 months ago

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?