search

twilio / authy-devise

Authy Devise plugin to add Two-Factor Authentication
MIT License
201 stars 84 forks source link

Option to disable 2FA without removing the User from Authy #143

Closed dpflucas closed 4 years ago

dpflucas commented 4 years ago

The Authy API docs state the following:

A user may have multiple email addresses but only one phone is associated with each authy_id. Two separate API calls to register a user with the same device and different emails will return the same authy_id and store both emails for that user.

This means that is valid to have multiple devise resources in an application that share the same authy_id.

However, when one of the devise resource disables 2FA, this action will remove the user from Authy:

https://github.com/twilio/authy-devise/blob/ec6cb1839196d9f949c903d124210af0b4f756d9/app/controllers/devise/devise_authy_controller.rb#L83

This means that all the other devise resources that share the same authy_id will still have the authy_enabled attribute set to true, however the associated user in Authy doesn't exist anymore, which will lead to errors.

Does it make sense to add an option to disable 2FA without actually removing the User from Authy?

PS: I can fix this in my application by overriding the POST_disable_authy controller action to only remove the user from Authy if there are no more devise resources enabled with the same authy_id. But would like to know if you think it makes sense to add this to the gem.

Thanks!

philnash commented 4 years ago

Hey @dpflucas, thanks for opening the issue. This is an interesting question about where the responsibility lies for this. It's definitely the case that you could end up with two users with the same authy_id and disabling it for one would then break it for the other.

Rather than making an option, what do you think of this plan?

When a user disables 2FA, remove their authy_id and set authy_enabled to false and save the user. Then, look for any other user with the same authy_id (there is already an index on the column from the generated migration) and if there are others remaining complete the action. If there are no others remaining, make the delete_user call to the API.

This should keep things as clean as possible without causing login problems.

dpflucas commented 4 years ago

Rather than making an option, what do you think of this plan?

Makes sense, this way both scenarios will be handled without the developer having to pass an option explicitly đź‘Ť

philnash commented 4 years ago

@dpflucas I've made this a PR #144, please take a look and let me know what you think.

philnash commented 4 years ago

This has been released as version 2.2.0. Thanks!