Add a variant which sets up 2FA with devise #77
Conversation
|
The basics are in this now but we need to add:
My goal here is that this should generate everything you need except CSS styling for the feature |
|
I now think this is big enough to be a whole separate template so I'm going to close this. |
|
This work has to be paused until devise-two-factor supports Rails 7. I'm trying to help make that happen on devise-two-factor/devise-two-factor#206 |
|
Edit: this is outdated, latest changes are in this PR now |
eoinkelly
changed the title
eoinkelly
changed the title
|
copy improvements from another project which doesn't need js and is instead an actual 2-step process with separate pages |
|
I have copied the files from ackama/rails-template-demo-devise-2fa#11 which incorporates changes made on client projects. This PR is still very WIP. |
| private | ||
|
|
||
| def otp_param | ||
| params.require(:otp_attempt).gsub(/\A[^\d+]\z/, "") |
There was a problem hiding this comment.
The OTP parameter sanitization regex appears incorrect for typical OTP codes composed solely of digits. Consider replacing it with a method like .delete("^0-9") to reliably remove unwanted characters.
| params.require(:otp_attempt).gsub(/\A[^\d+]\z/, "") | |
| params.require(:otp_attempt).delete("^0-9") |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is a real bug. My regex had anchors for start and end of string (\A and \z respectively) which prevents it from matching all non digit characters. A correct regex would be /[^\d+]/ but the suggestion from copilot is also correct and honestly a bit more readable.
# irb session to demonstrate
irb(main):001> x = "abc123%*"
=> "abc123%*"
# my broken regex
irb(main):002> x.gsub(/\A[^\d+]\z/, "")
=> "abc123%*"
# copilot suggestion
irb(main):003> x.delete("^0-9")
=> "123"
# my fixed regex
irb(main):004> x.gsub(/[^\d+]/, "")
=> "123"| @@ -45,6 +45,10 @@ def apply_variant_devise? | |||
| @yaml_config.fetch("apply_variant_devise") | |||
| end | |||
|
|
|||
| def apply_variant_devise_mfa? | |||
| @yaml_config.fetch("apply_variant_devise") | |||
There was a problem hiding this comment.
The apply_variant_devise_mfa? method is fetching the 'apply_variant_devise' key instead of 'apply_variant_devise_mfa', which may lead to incorrect behavior. Please update the key to ensure it reflects the intended variant.
| @yaml_config.fetch("apply_variant_devise") | |
| @yaml_config.fetch("apply_variant_devise_mfa") |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is a real bug and the suggested fix does fix it.
Current status
WIP. This branch can be used as inspiration for a devise MFA feature. The design doc included in this PR is particularly useful for understanding the feature.
Background
These changes were initially sketched in ackama/rails-template-demo-devise-2fa#11 and refined in some real client apps. Hat tip to @joshmcarthur who greatly improved my first pass at this. Fixes #61
TODO
TODOitems in the code