MFA Setup can be skipped via users back button

[lawrence-berry]

We have noticed a number of issue regarding the flow of the MFA sign-up process when attempting to add devise-mfa to an existing Rails application using devise for authentication. for example:

Current Behavior

1. Create a new user & set password from email link
2. When greeted with the MFA setup screen press the back button
3. User is logged in

Expected Behavior

1. Create a new user & set password from email link
2. When greeted with the MFA setup screen press the back button
3. User is taken back to the main login screen /admin/login

There is also a potentially related issue when clicking back from the 2nd MFA setup screen results in the user being presented with an MFA challenge on subsequent login, when they have not completed MFA setup.

Finally, it seems as if the trackable columns within devise are updated after successful password authentication, rather than after successful MFA challenge completion, where this is mandatory for the user. Is this configurable in any way, or expected behavior?

Apologies for lumpiong these all in together, let me know if you would rather they were raised as separate issues, I've kept them as a single issue for now as I get the feeling they are all related.

[strouptl]

I have confirmed the OP's main issue:

1. Configure MFA to be mandatory
2. Create a new user from the command line
3. Sign in as the new user
4. When presented with the MFA setup screen, visit any other URL (either by pressing back, clicking a navigation link, or entering the URL manually into the browser)
5. Result: User is still logged in

Underlying issue: the "create_otp_session" method will redirect an user to the MFA setup screen if MFA is configured to be mandatory, but this is a one time redirect, and it is not enforced at the controller level (as authenticate_user! would do).

@strzibny, have there been any discussions on this issue? I suppose the gem is still usable with mandatory MFA disabled, but this would be a serious issue for anyone wanting to use the gem for mandatory MFA.

[strzibny]

@strouptl I welcome a PR for this. It's true I only use volantary MFA so I haven't noticed it.

@lawrence-berry let's create a separate issue for trackable

[strouptl]

Working on a solution for these...

[strzibny]

I am thinking about it now. Maybe we need to adjust another hook here https://github.com/wmlele/devise-otp/blob/master/lib/devise_otp_authenticatable/hooks/sessions.rb

[strouptl]

@strzibny, I have a new PR which eliminates the need for the artificial login/logout session here by utilizing the built-in warden method. I will go ahead and push that one up so you can take a look.

It is "breaking" at the moment, as I have not reimplemented the mandatory redirection yet, but I believe this is the way to go.

[strouptl]

Just pushed up Pull Request #75. Let me know your thoughts!

[strouptl]

Sorry to pile on the PR's! We are looking to add this to a production system, so I am trying to bring it up to speed as quickly a s possible.

[strouptl]

Hi @strzibny, I have submitted two new PR's related to this issue:

• Mandatory OTP Issue: Fix Mandatory OTP Issue #78
• Trackable fields (preparation): Refresh Credentials Hook #79

Once you have checked these, I have one final PR which uses the native Warden redirect! method. This should resolve the issue with the Trackable fields (along with some other benefits).

[strouptl]

UPDATED

Hi @strzibny, I have submitted three new PR's related to the issues above:

• Mandatory OTP Issue: Fix Mandatory OTP Issue #78
• Trackable fields Issue: Refresh Credentials Hook #79 and Utilize Warden Redirect #80

[strzibny]

This is closed by #78

[lawrence-berry]

Sorry to re-raise a dead issue (let me know if opening a new issue is better), but after to switching to the main branch of this repo, I'm not seeing any change in behavior, have confirmed i have V0.7.0 installed. This may well be user error on my part, I just wanted to check there isn't any additional steps I may need to take, beyond upgrading the Gem, to resolve the issue?

[strzibny]

I am now running 0.7.0 from RubyGems. I think you should point to and fetch the release version. That said, master should work.