refactor: remove admin monkey patching

CHANGELOG.md

@@ -1,7 +1,36 @@
+# Change Log
 ## Unreleased
 
 ### Added
 - Enforcing a redirect to setup of otp device when none available for user [#550](https://github.com/jazzband/django-two-factor-auth/pull/500)
+
+### Changed
+
+### Removed
+
+- Admin Monkey Patching
+
+  The Admin UI will not longer be automatically patched. The `TwoFactorSiteAdmin` will need to be explicitly
+  configured in urls.py.
+
+  ```py
+  # urls.py
+  from django.urls import path
+  from two_factor.admin import TwoFactorAdminSite
+  url_patterns = [
+    path('admin/', TwoFactorAdminSite().urls),
+  ]
+  ```
+
+  Custom admin sites can extend `TwoFactorSiteAdmin` or `TwoFactorSideAdminMixin` to inherit the behavior.
+
+  ```py
+  # admin.py
+  class MyCustomAdminSite(TwoFactorSiteAdminMixin, AdminSite):
+    # implement your customizations here.
+    pass
+  ```
+
 
 ## 1.14.0
 

docs/class-reference.rst

@@ -3,8 +3,8 @@ Class Reference
 
 Admin Site
 ----------
-.. autoclass:: two_factor.admin.AdminSiteOTPRequired
-.. autoclass:: two_factor.admin.AdminSiteOTPRequiredMixin
+.. autoclass:: two_factor.admin.TwoFactorAdminSite
+.. autoclass:: two_factor.admin.TwoFactorAdminSiteMixin
 
 Decorators
 ----------

docs/configuration.rst

@@ -4,13 +4,6 @@ Configuration
 General Settings
 ----------------
 
-``TWO_FACTOR_PATCH_ADMIN`` (default: ``True``)
-  Whether the Django admin is patched to use the default login view.
-
-  .. warning::
-     The admin currently does not enforce one-time passwords being set for
-     admin users.
-
 ``LOGIN_URL``
   Should point to the login view provided by this application as described in
   setup. This login view handles password authentication followed by a one-time
@@ -123,7 +116,7 @@ Next, add additional urls to your config:
 
     # urls.py
     from two_factor.gateways.twilio.urls import urlpatterns as tf_twilio_urls
-    
+
     urlpatterns = [
         path('', include(tf_twilio_urls)),
         ...

docs/installation.rst

@@ -66,9 +66,10 @@ Add the routes to your project url configuration:
 .. code-block:: python
 
     from two_factor.urls import urlpatterns as tf_urls
+    from two_factor.admin import TwoFactorAdminSite
     urlpatterns = [
        path('', include(tf_urls)),
-        ...
+       path('admin', TwoFactorAdminSite().urls)
     ]
 
 .. warning::

example/urls.py

@@ -1,8 +1,8 @@
 from django.conf import settings
-from django.contrib import admin
 from django.contrib.auth.views import LogoutView
 from django.urls import include, path
 
+from two_factor.admin import TwoFactorAdminSite
 from two_factor.gateways.twilio.urls import urlpatterns as tf_twilio_urls
 from two_factor.urls import urlpatterns as tf_urls
 
@@ -39,7 +39,7 @@
     path('', include(tf_urls)),
     path('', include(tf_twilio_urls)),
     path('', include('user_sessions.urls', 'user_sessions')),
-    path('admin/', admin.site.urls),
+    path('admin/', TwoFactorAdminSite().urls),
 ]
 
 if settings.DEBUG:

tests/test_admin.py

@@ -1,68 +1,100 @@
-from django.conf import settings
-from django.shortcuts import resolve_url
+
+from unittest import mock
+
+from django.shortcuts import 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
 
 
 @override_settings(ROOT_URLCONF='tests.urls_admin')
-class AdminPatchTest(TestCase):
-
-    def setUp(self):
-        patch_admin()
+class TwoFactorAdminSiteTest(UserMixin, TestCase):
+    """
+    otp_admin is admin console that needs OTP for access.
+    Only admin users (is_staff and is_active)
+        with OTP can access it.
+    """
+
+    def test_anonymous_get_admin_index_redirects_to_admin_login(self):
+        index_url = reverse('admin:index')
+        login_url = reverse('admin:login')
+        response = self.client.get(index_url, follow=True)
+        redirect_to = '%s?next=%s' % (login_url, index_url)
+        self.assertRedirects(response, redirect_to)
 
-    def tearDown(self):
-        unpatch_admin()
+    def test_anonymous_get_admin_logout_redirects_to_admin_index(self):
+        # see: django.tests.admin_views.test_client_logout_url_can_be_used_to_login
+        index_url = reverse('admin:index')
+        logout_url = reverse('admin:logout')
+        response = self.client.get(logout_url)
+        self.assertEqual(
+            response.status_code, 302
+        )
+        self.assertEqual(response.get('Location'), index_url)
+
+    def test_anonymous_get_admin_login(self):
+        login_url = reverse('admin:login')
+        response = self.client.get(login_url, follow=True)
+        self.assertEqual(response.status_code, 200)
 
-    def test(self):
-        response = self.client.get('/admin/', follow=True)
-        redirect_to = '%s?next=/admin/' % resolve_url(settings.LOGIN_URL)
+    def test_is_staff_not_verified_not_setup_get_admin_index_redirects_to_setup(self):
+        """
+        admins without MFA setup should be redirected to the setup page.
+        """
+        index_url = reverse('admin:index')
+        setup_url = reverse('two_factor:setup')
+        self.user = self.create_superuser()
+        self.login_user()
+        response = self.client.get(index_url, follow=True)
+        redirect_to = '%s?next=%s' % (setup_url, index_url)
         self.assertRedirects(response, redirect_to)
 
-    @override_settings(LOGIN_URL='two_factor:login')
-    def test_named_url(self):
-        response = self.client.get('/admin/', follow=True)
-        redirect_to = '%s?next=/admin/' % resolve_url(settings.LOGIN_URL)
+    def test_is_staff_not_verified_not_setup_get_admin_login_redirects_to_setup(self):
+        index_url = reverse('admin:index')
+        login_url = reverse('admin:login')
+        setup_url = reverse('two_factor:setup')
+        self.user = self.create_superuser()
+        self.login_user()
+        response = self.client.get(login_url, follow=True)
+        redirect_to = '%s?next=%s' % (setup_url, index_url)
         self.assertRedirects(response, redirect_to)
 
-
-@override_settings(ROOT_URLCONF='tests.urls_admin')
-class AdminSiteTest(UserMixin, TestCase):
-
-    def setUp(self):
-        super().setUp()
+    def test_is_staff_is_verified_get_admin_index(self):
+        index_url = reverse('admin:index')
         self.user = self.create_superuser()
+        self.enable_otp(self.user)
         self.login_user()
-
-    def test_default_admin(self):
-        response = self.client.get('/admin/')
+        response = self.client.get(index_url)
         self.assertEqual(response.status_code, 200)
 
-
-@override_settings(ROOT_URLCONF='tests.urls_otp_admin')
-class OTPAdminSiteTest(UserMixin, TestCase):
-
-    def setUp(self):
-        super().setUp()
+    def test_is_staff_is_verified_get_admin_password_change(self):
+        password_change_url = reverse('admin:password_change')
         self.user = self.create_superuser()
+        self.enable_otp(self.user)
         self.login_user()
+        response = self.client.get(password_change_url)
+        self.assertEqual(response.status_code, 200)
 
-    def test_otp_admin_without_otp(self):
-        response = self.client.get('/otp_admin/', follow=True)
-        redirect_to = '%s?next=/otp_admin/' % resolve_url(settings.LOGIN_URL)
-        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)
-        self.assertRedirects(response, redirect_to)
-
-    def test_otp_admin_with_otp(self):
-        self.enable_otp()
+    def test_is_staff_is_verified_get_admin_login_redirects_to_admin_index(self):
+        login_url = reverse('admin:login')
+        index_url = reverse('admin:index')
+        self.user = self.create_superuser()
+        self.enable_otp(self.user)
         self.login_user()
-        response = self.client.get('/otp_admin/')
+        response = self.client.get(login_url)
+        self.assertEqual(response.get('Location'), index_url)
+
+    @mock.patch('two_factor.views.core.signals.user_verified.send')
+    def test_valid_login(self, mock_signal):
+        login_url = reverse('admin:login')
+        self.user = self.create_user()
+        self.enable_otp(self.user)
+        data = {'auth-username': 'bouke@example.com',
+                'auth-password': 'secret',
+                'login_view-current_step': 'auth'}
+        response = self.client.post(login_url, data=data)
         self.assertEqual(response.status_code, 200)
+
+        # No signal should be fired for non-verified user logins.
+        self.assertFalse(mock_signal.called)

tests/urls_admin.py

@@ -1,8 +1,9 @@
-from django.contrib import admin
 from django.urls import path
 
+from two_factor.admin import TwoFactorAdminSite
+
 from .urls import urlpatterns
 
 urlpatterns += [
-    path('admin/', admin.site.urls),
+    path('admin/', TwoFactorAdminSite().urls),
 ]

two_factor/admin.py

@@ -1,11 +1,11 @@
+import warnings
+
 from django.conf import settings
 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.shortcuts import resolve_url
 
-from .utils import monkeypatch_method
-
 try:
     from django.utils.http import url_has_allowed_host_and_scheme
 except ImportError:
@@ -14,7 +14,7 @@
     )
 
 
-class AdminSiteOTPRequiredMixin:
+class TwoFactorAdminSiteMixin:
     """
     Mixin for enforcing OTP verified staff users.
 
@@ -43,29 +43,20 @@ def login(self, request, extra_context=None):
         return redirect_to_login(redirect_to)
 
 
-class AdminSiteOTPRequired(AdminSiteOTPRequiredMixin, AdminSite):
+class TwoFactorAdminSite(TwoFactorAdminSiteMixin, AdminSite):
     """
-    AdminSite enforcing OTP verified staff users.
+    AdminSite with MFA Support.
     """
     pass
 
 
-def patch_admin():
-    @monkeypatch_method(AdminSite)
-    def login(self, request, extra_context=None):
-        """
-        Redirects to the site login page for the given HttpRequest.
-        """
-        redirect_to = request.POST.get(REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME))
-
-        if not redirect_to or not url_has_allowed_host_and_scheme(url=redirect_to, allowed_hosts=[request.get_host()]):
-            redirect_to = resolve_url(settings.LOGIN_REDIRECT_URL)
-
-        return redirect_to_login(redirect_to)
-
-
-def unpatch_admin():
-    setattr(AdminSite, 'login', original_login)
+class AdminSiteOTPRequiredMixin(TwoFactorAdminSiteMixin):
+    warnings.warn('AdminSiteOTPRequiredMixin is deprecated by TwoFactorAdminSiteMixin, please update.',
+                  category=DeprecationWarning)
+    pass
 
 
-original_login = AdminSite.login
+class AdminSiteOTPRequired(TwoFactorAdminSite):
+    warnings.warn('AdminSiteOTPRequired is deprecated by TwoFactorAdminSite, please update.',
+                  category=DeprecationWarning)
+    pass

two_factor/apps.py

@@ -1,12 +1,6 @@
 from django.apps import AppConfig
-from django.conf import settings
 
 
 class TwoFactorConfig(AppConfig):
     name = 'two_factor'
     verbose_name = "Django Two Factor Authentication"
-
-    def ready(self):
-        if getattr(settings, 'TWO_FACTOR_PATCH_ADMIN', True):
-            from .admin import patch_admin
-            patch_admin()