Make OTP delivery independent of 2FA gem #452
Conversation
monfresh
force-pushed
the
refactor-otp-delivery
branch
from
3cd2a7b
to
ddb5fd7
Compare
| # Present the TOTP entry screen to users who are TOTP enabled, | ||
| # unless the user explictly selects SMS or voice | ||
| current_user.totp_enabled? && !use_sms_or_voice_otp_delivery? | ||
| end |
There was a problem hiding this comment.
I removed this because a TOTP-enabled user can never select any other option before they land on the show view, which is the only place this method was called. When a TOTP-enabled user signs in, they are automatically asked for their Authenticator app code, with the option to use SMS or voice instead. If they choose one of those options, they go to the otp/send path, which never calls this use_totp? method.
| job = "#{current_otp_method.to_s.capitalize}SenderOtpJob".constantize | ||
|
|
||
| job.perform_later(confirmation_code, unconfirmed_phone) | ||
|
|
There was a problem hiding this comment.
This change adheres to the Open/Closed principle because it allows us to add new delivery options without changing the controller code, whereas before, we would need to add a new elsif statement to the conditional in the controller.
Here's a quick article on SOLID principles: https://robots.thoughtbot.com/back-to-basics-solid
I also highly recommend Sandi Metz's Practical Object-Oriented Design in Ruby: An Agile Primer, which is available on Safari Books Online.
There was a problem hiding this comment.
Generating references at runtime using .constantize is also pretty horrible though :( It makes it really hard to grep for references and understand there things are using the codebase. I wish there was a nicer solution.
There was a problem hiding this comment.
This is a common way of implementing the "Replace Conditional with Polymorphism" refactoring pattern. At least here, you can grep for most of the class name, the SenderOtpJob portion, which is common to all the classes related to sending an OTP, because we have agreed to this naming convention.
If Sandi Metz uses this pattern, it makes it acceptable in my opinion. Search for const_get in this article:
http://www.sandimetz.com/blog/2014/9/9/shape-at-the-bottom-of-all-things
thoughtbot, a well-respected Ruby/Rails firm, also uses this pattern on page 45 of their Ruby Science book.
The other option would be to create a Class or Module that defines a hash that maps the delivery method as submitted by the user to the Active Job class, and then the class can be obtained from the hash. This would not be my first choice though because you have to remember to update the hash every time you add a new delivery method and corresponding Active Job class.
There was a problem hiding this comment.
I get Sam's point, but using this approach in moderation is not going to create any confusion, imo.
| @@ -268,5 +268,4 @@ | |||
| config.otp_length = 6 | |||
| config.direct_otp_length = 6 | |||
| config.direct_otp_valid_for = 5.minutes | |||
| config.enable_auto_send_otp = false # disable auto-sending of OTP after authentication | |||
**Why**: We should not have to modify the `send_two_factor_authentication_code` method in the `two_factor_authentication` gem in order to accommodate multiple delivery mechanisms. This violates the "O" in the SOLID object-oriented design principles. The "O" stands for Open for extension, Closed for modification. **How**: We can work around the gem's undesirable design choices by making a few sensible changes: - Make the `send_two_factor_authentication_code` method a no-op method, i.e. it doesn't do anything. This means that even though the gem calls this method right after Warden signs the user in, nothing will happen, which is what we want. - Instead of relying on the `send_two_factor_authentication_code` method and the `UserOtpSender` class to invoke the proper background job, we do this work in the controller, and use the delivery method chosen by the user to determine which job to invoke. This allows us to continue our work without having to fork the gem or wait for our PRs to be approved and merged. We can work on our code the way we want and propose changes to the gem in parallel, without one depending on the other. Note that these are the smallest changes I could come up with to remove the dependency on Amos's fork, without changing behavior. Due to preexisting duplication, new duplication was introduced, but it will be cleaned up in subsequent PRs, which are part of a larger refactor I am working on.
monfresh
force-pushed
the
refactor-otp-delivery
branch
from
ddb5fd7
to
8418fb1
Compare
|
Comment added. Let me know how it looks. |
|
Is this good to go now, or still reviewing? |
Why: We should not have to modify the
send_two_factor_authentication_codemethod in thetwo_factor_authenticationgem in order to accommodate multipledelivery mechanisms. This violates the "O" in the SOLID object-oriented
design principles. The "O" stands for Open for extension, Closed
for modification.
How:
We can work around the gem's undesirable design choices by making a few
sensible changes:
send_two_factor_authentication_codemethod a no-op method,i.e. it doesn't do anything. This means that even though the gem calls
this method right after Warden signs the user in, nothing will happen,
which is what we want.
send_two_factor_authentication_codemethod and the
UserOtpSenderclass to invoke the proper backgroundjob, we do this work in the controller, and use the delivery method
chosen by the user to determine which job to invoke.
This allows us to continue our work without having to fork the gem or
wait for our PRs to be approved and merged. We can work on our code
the way we want and propose changes to the gem in parallel, without
one depending on the other.
Note that these are the smallest changes I could come up with to remove
the dependency on Amos's fork, without changing behavior. Due to
preexisting duplication, new duplication was introduced, but it will be
cleaned up in subsequent PRs, which are part of a larger refactor I am
working on.