Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hitting otp_required as a user without device results in infinite login loop #499

Closed
MWeesenaar opened this issue May 12, 2022 · 7 comments

Comments

@MWeesenaar
Copy link
Contributor

During implementation of this module, I noticed that in some use cases, I got stuck in an infinite login loop.

Expected Behavior

For step 4, I would expect a message stating that the page the user tries to access requires OTP and without the user having an OTP device, that this is not possible. OR) the user is being redirected to the OTP setup page, and after setting up the OTP, have the possibility to get back to the page where the user initially wanted to go to.
Note that this might be troublesome for body-related requests (e.g. POST/PUT), but I have the feeling (not confirmed from my side) that this is already the case for the current implementation.

Current Behavior

When going to an otp_required-view, after signing in with a user without an OTP device, the user gets stuck in an infinite login-loop.

Possible Solution

I would say that an else-clause could be useful in core.py#L160.
I will create a pull request with the suggested change and updated tests.
The only question I have, right now, is the following: since this change will introduce a 'breaking change' (in the sense of user-flow), please advise whether it should be configureable or not. I tend to say no, since this feels like a user-workflow related bug; but I prefer the maintainers to have an opinion on this.

Steps to Reproduce (for bugs)

  1. Install the module as per instructions, configure appropriately and make sure you got atleast one user without an otp-device linked and @otp_required view - for making sure that it is not related to my Django instance, I could reproduce it on a clean-freshly installed Django project
  2. Make sure you're not logged in on the Django app and go to the otp_required-view
  3. Validate that you're being redirected to /account/login/?next={view_url}
  4. Log in with your user credentials
  5. You are again, directed to the /account/login/ page, with the same ?next={view_url}

Context

It affects me because I have a few dozen users of my app, who are now in a loop of logging in, coming to me, and I need to explain them to go to the OTP setup page.

Your Environment

  • Browser and version: Chrome 103.0.5052.0 (Official Build) canary (arm64)
  • Python version: 3.10 (and initially found on 3.6)
  • Django version: 4.0.4 (and reproducible on 3.2.12)
  • django-otp version: 1.1.3
  • django-two-factor-auth version: 1.13.2
  • Link to your project: N/a
@alstr
Copy link

alstr commented Aug 24, 2022

I'm getting this problem even when I have a device set up, regardless of otp_required.

@claudep
Copy link
Contributor

claudep commented Aug 24, 2022

This should hopefully be fixed in master with 4bd592c

@claudep claudep closed this as completed Aug 24, 2022
@PetrDlouhy
Copy link
Contributor

@MWeesenaar @claudep
On new master branch my application starts to behave differently - user is redirected to create OTP page. I bisected the issue down to the commit applied for this issue.

I don't have otp_required for any of my pages and I don't want OTP to be required for all users.

Is this intentional? If so, shouldn't the change be documented?

@claudep
Copy link
Contributor

claudep commented Sep 28, 2022

I don't think this was intentional, looks like a regression. Thanks for noticing.

@MWeesenaar, do you have a plan to avoid this regression?

@MWeesenaar
Copy link
Contributor Author

@PetrDlouhy , I am sorry that this might introduced regression.
In all fairness, I have not touched Django as of this PR any more - since I finished the application where I needed this repo for. So if I can help, I would need to get digging in this repo again, with some what of a reproduction scenario / example application with described expected behaviour.

So, @claudep , future regression will simply not occur any more, since I have no plans as of now to submit further PRs.

@claudep
Copy link
Contributor

claudep commented Sep 28, 2022

@PetrDlouhy If you have an idea about how to fix this without reverting, it's welcome. Otherwise, I'm afraid we'll have to revert this as it seems the regression is at least as serious as the fix.

@PetrDlouhy
Copy link
Contributor

I tried to fix this in #558. I don't understand the original issue fully, but this seems to be working.
Please review thoroughly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants