Direct MFA Required users to edit settings page #3902
Conversation
ericherscovich
force-pushed
the
eric/mfa-required-options
branch
from
662d06c
to
efa914f
Compare
Codecov Report
@@ Coverage Diff @@
## master #3902 +/- ##
=======================================
Coverage 98.79% 98.80%
=======================================
Files 217 217
Lines 5414 5417 +3
=======================================
+ Hits 5349 5352 +3
Misses 65 65
|
| @@ -4,8 +4,14 @@ module ApplicationMultifactorMethods | |||
| included do | |||
| def redirect_to_new_mfa | |||
| message = t("multifactor_auths.setup_required_html") | |||
|
|
|||
| if request.path_info == edit_settings_path | |||
There was a problem hiding this comment.
Maybe current_page? could be used in here.
There was a problem hiding this comment.
Wasn't able to get current_page to work here, I believe it's because this is a concern and doesn't have direct access to ActiovView perhaps 🤔
There was a problem hiding this comment.
True you would need to include the ActionView::Helpers::UrlHelper module into the concern.
module ApplicationMultifactorMethods
include ActionView::Helpers::UrlHelper
There was a problem hiding this comment.
Gave this another attempt. Unfortunately given that this concern is used in many places, including ActionView::Helpers::UrlHelper causes some bad behaviour in other controllers relying on it. So given that the current request.path_info == edit_settings_path does do what we need it do, will keep it as is.
Feel free to ping me on Slack if help would be welcomed, I can try to prepare testing skeleton for those cases. |
ericherscovich
force-pushed
the
eric/mfa-required-options
branch
from
2b516fb
to
cf494ca
Compare
Update test Use current_page? Remove current_page
ericherscovich
force-pushed
the
eric/mfa-required-options
branch
from
9043cbd
to
d6edc00
Compare
Contributes to #3800
What problem are you solving?
Currently, on an MFA Required user, they are directed to the TOTP setup page:
Given that TOTP or webauthn can both be used, it makes more sense to redirect them to the edit settings page where they can choose TOTP or webauthn.
What approach did you choose and why?
Where a user would previously have hit to TOTP setup page as shown above, they will now instead be redirected to the edit settings page, and will be given the relevant flash warning.
Testing
Set up a user that has a gem with over 180M downloads. You can do this by creating a gem, and running something along the lines of the below in your Rails Console.
my_gem = GemDownload.last my_gem.count = 180000123 my_gem.save!After this, you can log in to the web app, and validate that the correct functionality is occuring.
Future Considerations
I noticed that we lack test suites for a few of our concerns, including the one I touched in this PR,
ApplicationMultifactorMethods. These are somewhat funky to set up, but it would perhaps be beneficial to backfill these tests in the future.