Skip to content

Conversation

@shawnz
Copy link
Contributor

@shawnz shawnz commented Apr 11, 2025

When using AdminSiteOTPRequiredMixin, users who don't have 2FA set up will face an infinite login redirect, similar to the behaviour prior to #499/#500.

In #558, a change was made to fix a regression introduced by #500 by restricting when the 2FA setup login redirect will apply. But, the restriction logic only considers the use of OTPRequiredMixin. It doesn't consider the use of AdminSiteOTPRequiredMixin. Thus, users of AdminSiteOTPRequiredMixin on their admin site will still see the same broken infinite redirect behaviour from before.

This changes the logic added in #558 to also consider AdminSiteOTPRequiredMixin in addition to OTPRequiredMixin.

Description

Update the is_otp_view logic to additionally consider views that are part of an admin site with the AdminSiteOTPRequiredMixin as OTP views.

Motivation and Context

Fixes the infinite login redirect for admin sites, first described in #499, fixed in #500, then regressed for admin sites specifically in #558.

How Has This Been Tested?

I applied this change to my workplace's Django 4.2.20 app and tried it out by hand to confirm that the infinite redirect is fixed for our admin site. Additionally, I copied the test from #500 which tests the redirect flow for logged out users without OTP on OTP required views but applied it to the OTPAdminSite.

Before the change:

FAIL: test_otp_admin_login_redirect_without_otp (tests.test_admin.OTPAdminSiteTest.test_otp_admin_login_redirect_without_otp)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/shawnz/Documents/GitHub/django-two-factor-auth/.venv/lib/python3.13/site-packages/django/test/utils.py", line 456, in inner
    return func(*args, **kwargs)
  File "/Users/shawnz/Documents/GitHub/django-two-factor-auth/tests/test_admin.py", line 91, in test_otp_admin_login_redirect_without_otp
    self.assertRedirects(response, reverse('two_factor:setup'))
    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 302 != 200 : Couldn't retrieve redirection page '/otp_admin/': response code was 302 (expected 200)

----------------------------------------------------------------------
Ran 165 tests in 2.713s

FAILED (failures=1, skipped=1)

After the change:

----------------------------------------------------------------------
Ran 165 tests in 2.709s

OK (skipped=1)

Screenshots (if appropriate):

N/A

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.

Copy link
Collaborator

@moggers87 moggers87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good but I think it needs tests otherwise we will run into the same problem again in the future.

@shawnz
Copy link
Contributor Author

shawnz commented Apr 12, 2025

Agreed, working on them as we speak!

Thanks for looking at this

@shawnz
Copy link
Contributor Author

shawnz commented Apr 13, 2025

@moggers87 I copied the test from #500 which validates the login and setup redirect flow but applied it to the OTPAdminSite. Let me know if you feel that's sufficient.

Before the change:

FAIL: test_otp_admin_login_redirect_without_otp (tests.test_admin.OTPAdminSiteTest.test_otp_admin_login_redirect_without_otp)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/shawnz/Documents/GitHub/django-two-factor-auth/.venv/lib/python3.13/site-packages/django/test/utils.py", line 456, in inner
    return func(*args, **kwargs)
  File "/Users/shawnz/Documents/GitHub/django-two-factor-auth/tests/test_admin.py", line 91, in test_otp_admin_login_redirect_without_otp
    self.assertRedirects(response, reverse('two_factor:setup'))
    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 302 != 200 : Couldn't retrieve redirection page '/otp_admin/': response code was 302 (expected 200)

----------------------------------------------------------------------
Ran 165 tests in 2.713s

FAILED (failures=1, skipped=1)

After the change:

----------------------------------------------------------------------
Ran 165 tests in 2.709s

OK (skipped=1)

Thanks, Shawn

Copy link
Collaborator

@moggers87 moggers87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moggers87 moggers87 merged commit 8f02f48 into jazzband:master Apr 16, 2025
4 checks passed
@mightym
Copy link

mightym commented May 25, 2025

@moggers87 I just ran into the same issue and saw that its already fixed <3 . But the last release on https://pypi.org/project/django-two-factor-auth/ is from August 2024. Will there be a new release with that fix soon?

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

Successfully merging this pull request may close these issues.

3 participants