twilio / authy-devise

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

Bad decision #151

Closed rubygitflow closed 2 years ago

rubygitflow commented 3 years ago

https://github.com/twilio/authy-devise/blob/5f233c7af959b40fb9bec3faa2d663226efacd23/app/controllers/devise/devise_authy_controller.rb#L209 This is bad because we can only access verify_authy_installation when resource.authy_enabled = false. This is not logical from the point of view of an application programmer. Apparently it should be like this:

if resource.authy_id && resource.last_sign_in_with_authy
  redirect_to after_authy_verified_path_for(resource)
end
philnash commented 3 years ago

Hi @rubygitflow, thanks for raising an issue.

Before a resource has verified the authy installation by entering a code from the app or that was sent to them, they have resource.authy_enabled set to false. Only once they have successfully verified is resource.authy_enabled set to true, on this line.

So, if a resource is already authy_enabled then we know that the resource has already verified the authy installation and if they try to access GET_verify_authy_installation or POST_verify_authy_installation we redirect them on to the step beyond that.

Does that make sense? Or are you seeing something different here?

rubygitflow commented 3 years ago

From the point of view of the inherent meanings in the names of variables, you're right. Actually, your recommendation to set resource.auto_enabled to false was implemented in my solution. However, it entails creating crutches around the situation when the user refuses to send an SMS to himself on the user_verify_authy_installation_path page. I was very stressed when I could't open user_verify_auto_installation_path until I figured out the source code. I don't know yet what the solution should look like from a logical point of view, but the current solution is definitely not ideal, unless I'm missing something in the global two-factor authentication strategy for user registration. Where can I see an event diagram describing the behavior of the authy-devise library? Because I would just have to ask the user for a phone number, check that this number is real, and then finish the user registration.

philnash commented 3 years ago

Hi @rubygitflow, I am not sure what you mean here. Perhaps you could describe the specific issue that you are having?

I don't have a diagram of events, but I can describe the behaviour if that helps?

A resource that is logged in can enable two factor authentication by visiting user_enable_authy_path. This route runs the Devise::DeviseAuthyController#GET_enable_authy action which, by default, shows the resource a form in which they enter their phone number and country code. If the resource has already enabled 2FA (resource.authy_id is present and resource.authy_enabled is false) they are redirected to the after_authy_enabled_path_for(resource).

When they submit the form, it runs the Devise::DeviseAuthyController#POST_enable_authy action which registers the resource with the Authy API using their email, country code and phone number. If this is a success, Authy will either send an SMS with a verification code or will send a push notification to the user's Authy application. Also, the resource.authy_id is set to the Authy ID that is returned from the API call and the resource is redirected to verify they have enabled Authy.

The verification action is Devise::DeviseAuthyController#GET_verify_authy_installation and that displays a form for the user to enter the code they received by SMS or through the app. It also renders a link that the user can click to request another SMS (this is forced to be SMS this time, not a push notification to the app). As you already know, if a resource has already enabled Authy, they will be redirected away from this action to after_authy_verified_path_for(resource).

When they submit the code, it is POSTed to the Devise::DeviseAuthyController#POST_verify_authy_installation. The code is then verified using the Authy API and the resource's authy_id. If this is successful, the resource.authy_enabled is set to true, the resource's last_sign_in_with_authy is set to the current time, a session cookies is set to say that the resource's Authy token was set and the user is redirected to after_authy_verified_path_for(resource). If the code is not correct, the form is rendered again.

There is an example of this gem in action as part of a Rails application on GitHub here. You can download and run that application and see everything in action.

Let me know if this helps at all, or if there is more I can explain.

rubygitflow commented 3 years ago

Hi, Phil. I made a fork of the demo app and posted my own implementation for it https://github.com/rubygitflow/authy-devise-demo/commit/a68c98bb3a04ca3456c01a017891155496b50dbd. I'm sure that the new implementation of app/controllers/welcome_controller.rb reduces control over the app.

philnash commented 3 years ago

I feel like a lot of this has stemmed from the fact that you changed the default for the authy_enabled column from false to true. Can you explain why you have done that?

I'm not sure you ever explained what you were trying to do in the first place. Can we take a step back and you explain what brought you to start this issue?

rubygitflow commented 3 years ago

Your question made me realize that I misused the ill-fated parameter. However, my task is the opposite to the causal relationships embedded in the authy-devise gem. I added a description of my example of using two-factor user authentication. https://github.com/rubygitflow/authy-devise-demo Perhaps it will remove questions regarding my expectations from the authy-devise gem. Then my question changes perspective. How should I solve my problem using the authy-device gem?

philnash commented 3 years ago

So, your description of the problem is?

You have content on the resource that can only be accessed after a single user identification by phone number. During registration, you confirm your phone number by receiving a code from an SMS. During subsequent authorizations, you no longer ask the user for a confirmation code. If necessary, you still have the option to re-identify the user with two-factor authentication.

In this case it sounds like you are verifying that a user owns a phone number and can receive a message to that number.

That is a slightly different use case to what the Authy API provides. The Authy API, and subsequently this gem, is very much based around providing a second factor of authentication, via sms, voice, the Authy app or a generic authenticator app, on each authentication or on the first authentication before the "remember me" box is checked until the cookie expires.

So, I wouldn't recommend using this gem for your use case. Instead, I would recommend you take a look at Twilio's Verify API. Verify is intended for the one time verification use case that it sounded like you were describing. You can check out this implementation of Verify in a Rails application that should give you an idea of how it works.

I would also avoid trying to get the verification too muddled up with Devise and the authentication flow. Instead, you might look to override after_sign_in_path_for in your controller and send the user to your verification flow if they haven't previously verified their number. Something like:

def after_sign_in_path_for(resource)
  if resource.has_verified_phone_number?
    super(resource)
  else 
    new_resource_verification_path(resource)
  end
end

And then ensure that you gate any other actions that require the user to have verified their phone number with a similar redirect.

What do you think?

rubygitflow commented 3 years ago

Thank You. I need more time to study the new material. But your solution was very close to my needs.

philnash commented 3 years ago

Ok, sure. Let me know how you get on with Verify and if that is more useful for you.

I do just want to point out that, for the intended use of this gem, I don't believe there have been "bad decisions" here. The gem is built for a specific use case and while you seem to have bent it to your will now, I haven't seen anything that I should change about it yet. If you still feel something is up, after the discussion above, do let me know.

rubygitflow commented 3 years ago

Of course, I'll let you know if I have any additional insight. You don't need to change anything in your gem yet.

philnash commented 2 years ago

This library is no longer actively maintained. The Authy API has been replaced with the Twilio Verify API. Twilio will support the Authy API through November 1, 2022 for SMS/Voice. After this date, we’ll start to deprecate the service for SMS/Voice. Any requests sent to the API after May 1, 2023, will automatically receive an error. Push and TOTP will continue to be supported through July 2023.

Learn more about migrating from Authy to Verify.

Please visit the Twilio Docs for:

Please direct any questions to Twilio Support. Thank you!