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.

Sign up for GitHub

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

Jump to bottom

AdminSiteOTPRequiredMixin modified #370

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

AdminSiteOTPRequiredMixin modified #370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65030e7
AdminSiteOTPRequiredMixin modified
aseem-hegshetye Aug 5, 2020
ff34354
Ordered imports and modified tests
aseem-hegshetye Aug 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions tests/test_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
from django.conf import settings
from django.shortcuts import resolve_url
from django.shortcuts import resolve_url, reverse
from django.test import TestCase
from django.test.utils import override_settings

from two_factor.admin import patch_admin, unpatch_admin

from .utils import UserMixin


Expand Down Expand Up @@ -44,21 +43,30 @@ def test_default_admin(self):

@override_settings(ROOT_URLCONF='tests.urls_otp_admin')
class OTPAdminSiteTest(UserMixin, TestCase):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I test this class alone, all tests pass.
When I run entire test suit, 2 tests in this class fail

  1. test_otp_admin_without_otp
  2. test_otp_admin_without_otp_named_url

When running entire test suite, self.client.get('/otp_admin/', follow=True) returns login page url /account/login/?next=%2Fotp_admin%2F, when its supposed to return a OTP setup url /account/two_factor/setup/.
I was unable to find why this happens.
Any help is appreciated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I just test this class: AdminSiteOTPRequiredMixin().login() func executes.
When I run entire test suite - AdminSiteOTPRequiredMixin().login() func never executes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've not had a chance to look at this yet and I'm not overly familiar with the admin code.

I would imagine that our tests aren't quite as isolated from eachother as we'd actually want. Have you tried running just the test_admin module and then the class those two tests belong to? That might help narrow down where the problem is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept AdminSiteTest and OTPAdminSiteTest in test_admin.py and tried to test make test TARGET=tests.test_admin
They work fine as long as there is no get request made in AdminSiteTest class.
If I make that get request in AdminSiteTest.test_default_admin I get error in OTPAdminSiteTest class.

class AdminSiteTest(UserMixin, TestCase):

    def setUp(self):
        super().setUp()
        self.user = self.create_superuser()
        self.login_user()
        
    def test_default_admin(self):
        with self.settings(ROOT_URLCONF='tests.urls_admin'):
            # if I comment the line below, test_admin.py tests successfully.
            response = self.client.get('/admin/')


class OTPAdminSiteTest(UserMixin, TestCase):
    def setUp(self):
        super().setUp()
        self.user = self.create_superuser()
        self.login_user()

    def test_otp_admin_without_otp(self):
        with self.settings(ROOT_URLCONF='tests.urls_otp_admin'):
            response2 = self.client.get('/otp_admin/', follow=True)
            redirect_to = reverse('two_factor:setup')
            self.assertRedirects(response2, redirect_to)

Error:
Response redirected to '/admin/', expected '/account/two_factor/setup/'

So in conclusion, error in OTPAdminSiteTest class depends on which test was run previously. If I were to substitute AdminSiteTest with AdminPatchTest in the above code, my error will change to following:
Response redirected to '/account/login/?next=/otp_admin/', expected '/account/two_factor/setup/

"""
otp_admin is admin console that needs OTP for access.
Only admin users (is_staff and is_active)
with OTP can access it.
"""

def setUp(self):
super().setUp()
self.user = self.create_superuser()
self.login_user()

def test_otp_admin_without_otp(self):
"""
if user has admin permissions (is_staff and is_active)
but doesnt have OTP setup, redirect the user to OTP setup page
"""
response = self.client.get('/otp_admin/', follow=True)
redirect_to = '%s?next=/otp_admin/' % resolve_url(settings.LOGIN_URL)
redirect_to = reverse('two_factor:setup')
self.assertRedirects(response, redirect_to)

@override_settings(LOGIN_URL='two_factor:login')
def test_otp_admin_without_otp_named_url(self):
response = self.client.get('/otp_admin/', follow=True)
redirect_to = '%s?next=/otp_admin/' % resolve_url(settings.LOGIN_URL)
redirect_to = reverse('two_factor:setup')
self.assertRedirects(response, redirect_to)

def test_otp_admin_with_otp(self):
Expand Down
19 changes: 19 additions & 0 deletions two_factor/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@

from django.conf import settings
from django.contrib import admin
from django.contrib.admin import AdminSite
from django.contrib.auth import REDIRECT_FIELD_NAME
from django.contrib.auth.views import redirect_to_login
from django.http import HttpResponseRedirect
from django.shortcuts import resolve_url
from django.urls import reverse
from django.utils.http import is_safe_url

from .models import PhoneDevice
Expand All @@ -30,9 +33,25 @@ def has_permission(self, request):
def login(self, request, extra_context=None):
"""
Redirects to the site login page for the given HttpRequest.
If user has admin permissions but 2FA not setup, then redirect to
2FA setup page.
"""
# redirect to admin page after login
redirect_to = request.POST.get(REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME))

# if user (is_active and is_staff)
if request.method == "GET" and AdminSite().has_permission(request):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not self.has_permission(request)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.has_permission() func checks if user (is_staff, is_active and is_verified) .
AdminSite().has_permission() checks if user (is_staff, is_active ). Then based on if user is not verified (OTP not setup) we want to redirect to OTP setup page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

super() should be used instead then. This is a mixin and we don't know what other checks users might have/want so there might be other classes involved, not just AdminSite.


# if user has 2FA setup, go to admin homepage
if request.user.is_verified():
index_path = reverse("admin:index", current_app=self.name)

# 2FA not setup. redirect to 2FA setup page
else:
index_path = reverse("two_factor:setup", current_app=self.name)

return HttpResponseRedirect(index_path)

if not redirect_to or not is_safe_url(url=redirect_to, allowed_hosts=[request.get_host()]):
redirect_to = resolve_url(settings.LOGIN_REDIRECT_URL)

Expand Down