Enforcing a redirect to setup of otp device when none available for user

[MWeesenaar]

This pull request will enforce a user, without OTP device, to be redirected to the OTP setup page keeping the redirected-from in mind.

Description

Please refer to issue #499 for more details and context.

Motivation and Context

Please refer to issue #499 for more details and context.

How Has This Been Tested?

I created a scenario test: go to a protected page as anonymous user, log in, setup OTP and validate whether you get the option to be redirected to the page you meant to visit.
The current change has impact on the existing flow for users without OTP devices configured.

Screenshots (if appropriate):

Currently I have not made any. I might be able to create a short movie about the workings.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[codecov]

Codecov Report

Merging #500 (6144f37) into master (51b7fc2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #500      +/-   ##
==========================================
+ Coverage   98.51%   98.53%   +0.02%     
==========================================
  Files          60       60              
  Lines        2621     2659      +38     
  Branches      276      278       +2     
==========================================
+ Hits         2582     2620      +38     
  Misses         24       24              
  Partials       15       15              

Impacted Files	Coverage Δ
tests/test_views_login.py	100.00% <100.00%> (ø)
tests/test_views_mixins.py	100.00% <100.00%> (ø)
two_factor/views/core.py	97.09% <100.00%> (+0.11%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51b7fc2...6144f37. Read the comment docs.

[MWeesenaar]

Optionally the documentation about Enforcing two-factor could be updated or extended. I leave that up to the maintainers to determine whether that would be helpful, and if so, I will be pleased to do so.

[dopry]

@MWeesenaar Does this conflict with #491? I think in #491 I was automatically redirecting the user if next was set. This code looks like it waits until the "setup complete" view and then requires the user to click a link to get back to their intended destination. I kind of prefer saving the user the action if we know a destination. Do you think there is a good reason to require the user to take another action is this point?

[ghost]

Hi @dopry ,

Thanks for your message. Please consider the scenario I described in #499. It seems, that the redirection works too well when you do not have MFA setup yet.
I do not think that #491 really conflicts with this PR; where you solved it for Admin specifically where my main focus was to fix the login-loop.
I can validate my scenario with your branch/PR to validate if your solution fixes the problem I was facing; would that help?

PS. Sorry for mixed usage of Github accounts... Work (mandatory for employer) vs personal ... 😅

[dopry]

@MWeesenaar I was addressing the same issue, but for admins. They ended up in the same scenario you are describing in #499. I think they're compatible. We're both redirecting to the setup, but from different entry points in the UX Flow. We both intercept and redirect to OTP Setup. The only conflict I saw was handling the ?next after the user completes the setup. I feel like we should reconcile the UX flows of our implementations. I'd rather not create churn by implementing one flow and then immediately changing it with the final behavior dependent on the order of merges.

This is how your implementation in behaves

flowchart TD
   START((user requests\notp required page))-->CHECK_OTP{Is the user\nOTP Verified}
   CHECK_OTP-->|YES|NEXT[Requested Page is Delivered]
   CHECK_OTP-->|NO|OTP_SETUP[OTP Setup Page]
   OTP_SETUP-->|Complete OTP SETUP|OTP_SETUP_COMPLETE[OTP Setup  Complete]
   OTP_SETUP_COMPLETE-->|User clicks link to goto requested page|NEXT

Loading

This is how my implementation behaves, but only on the admin.

flowchart TD
   START((user requests\notp required page))-->CHECK_OTP{Is the user\nOTP Verified}
   CHECK_OTP-->|YES|NEXT[Requested Page is Delivered]
   CHECK_OTP-->|NO|OTP_SETUP[OTP Setup Page]
   OTP_SETUP-->|Complete OTP SETUP|NEXT

Loading

My implementation removes the additional user interaction of needing to click the next link on the SetupComplete view in favor of redirecting the user once the setup is complete. This is modeled on the same flow as a login intercept, where the user is automatically returned to their intended destination after login.

What is your reason for sending the user to the Setup Complete page and asking them to perform an additional interaction?

If you want to adopt the redirect to next behavior in your branch the code can be found at https://github.com/jazzband/django-two-factor-auth/pull/491/files#diff-47e9cf2bbbe1952606db449365a94f378e479389cc4c53d52310ceed0605646dR413. I added a get_success_url modeled on the one from django.contrib.auth.views.LoginView

[MWeesenaar]

Thanks for your feedback @dopry !
I must say that I do not have a very clear answer to your question why I think that manual interaction is better than redirecting automatically.
Would you suggest me removing the manual interaction?

[dopry]

Let's update it to automate the redirect. I worry that users will get further distracted from what they are doing if we add additional interactions to get them back to their original task.

[MWeesenaar]

I understand your point of view.
I will try to fix this somewhere this weekend. As you indicated, the code is there to do so, so it should not take too much time to actually implement it. :)

[dopry]

@MWeesenaar I rebased your PR on master. I updated to use RedirectURLMixin in place of SuccessURLAllowedHostsMixin for Django 4.1 compatibility that had been added upstream.

[dopry]

I've tested locally and this looks good to me. It even works for the admin. It looks like it will supersede my work in #491.