twilio / authy-devise

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

Suggestion: Allow "soft disable" #134

Closed DannyBen closed 4 years ago

DannyBen commented 4 years ago

Right now the disable route deletes the user from Authy. It would be nice if I can configure some option like DeviseAuthy.soft_disable and it cause the disable operation to just set the local flags to false, instead of hitting the Authy API.

If I want to do this myself - please correct me if I am wrong: I only need to set User.authy_enabled to false yes? (can I keep the User.authy_id?).

philnash commented 4 years ago

You are right, to soft disable a user in your own application without removing them in the API you would just set authy_enabled to false. You could then re-enable the user by just setting it back to true rather than putting the user back through the verification flow.

The potential issue here is that if your user is using the Authy app and has disabled 2FA for themselves, your application is still going to appear in the Authy app for them. If you do the full remove via the API, their tokens will be revoked immediately and the application will be removed from the app. That's why the docs describe this as the best practice.

Can you give me the use case for not removing them through the API, so I can consider whether to add this to the gem?

DannyBen commented 4 years ago

Thank you for the quick response.

The use case I have is this: We have several different "instances" of the same application, all using the same Authy API key. Although I know I can easily use a different key for each, I intentionally picked the same key, since the majority of my users (clients) only use their own instance, where we - the "super admins" - use all instances.

So - my options were to have many keys (with many Authy "accounts"), or this one key for all instances. Of course, I realize each of these options has its downside, but for now I chose to use one key.

Perhaps a better suggestion than "providing a config option", would be for the controller to look for params[;soft]? This is an easy change no?

Of course, I completely understand if this use case is not enough to justify such a change.

philnash commented 4 years ago

Hmm, I'll have a think about this. I'm not sure what you're doing is the best idea, but I can understand why you are doing it (having to have many accounts within the authy app to sort between over just the one).

In the meantime you could override the devise authy controllers to implement this soft removal yourself.

DannyBen commented 4 years ago

The more I think about it, the more it sounds like a bad idea. I will go ahead and switch to the best practice and avoid all that uncertainty.