From 1c275682a66c38dfc213c02a94a07a7d197fa9dd Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Thu, 23 Jun 2022 09:34:37 -0400 Subject: [PATCH] refactor: remove admin monkey patching - rename admin site classes to use the module namespace. BREAKING CHANGE: Sites will need to explicitly use TwoFactorAdminSite or extend the TwoFactorAdminSiteMixin --- CHANGELOG.md | 29 +++++++++ docs/class-reference.rst | 4 +- docs/configuration.rst | 9 +-- docs/installation.rst | 3 +- example/urls.py | 4 +- tests/test_admin.py | 124 ++++++++++++++++++++++++--------------- tests/urls_admin.py | 5 +- tests/urls_otp_admin.py | 11 ---- two_factor/admin.py | 37 +++++------- two_factor/apps.py | 6 -- 10 files changed, 131 insertions(+), 101 deletions(-) delete mode 100644 tests/urls_otp_admin.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cde32a7d..e09b23d1e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/class-reference.rst b/docs/class-reference.rst index 6f2332475..213247254 100644 --- a/docs/class-reference.rst +++ b/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 ---------- diff --git a/docs/configuration.rst b/docs/configuration.rst index 738a833c0..aafa36494 100644 --- a/docs/configuration.rst +++ b/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)), ... diff --git a/docs/installation.rst b/docs/installation.rst index 5ae666176..51989e0b8 100644 --- a/docs/installation.rst +++ b/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:: diff --git a/example/urls.py b/example/urls.py index 8e7531f09..63258bfe0 100644 --- a/example/urls.py +++ b/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: diff --git a/tests/test_admin.py b/tests/test_admin.py index 57d11225f..13c9c7a74 100644 --- a/tests/test_admin.py +++ b/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) diff --git a/tests/urls_admin.py b/tests/urls_admin.py index 64a073faa..881215767 100644 --- a/tests/urls_admin.py +++ b/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), ] diff --git a/tests/urls_otp_admin.py b/tests/urls_otp_admin.py deleted file mode 100644 index 32210cb68..000000000 --- a/tests/urls_otp_admin.py +++ /dev/null @@ -1,11 +0,0 @@ -from django.urls import path - -from two_factor.admin import AdminSiteOTPRequired - -from .urls import urlpatterns - -otp_admin_site = AdminSiteOTPRequired() - -urlpatterns += [ - path('otp_admin/', otp_admin_site.urls), -] diff --git a/two_factor/admin.py b/two_factor/admin.py index 7d9b29513..e1e7cb7d7 100644 --- a/two_factor/admin.py +++ b/two_factor/admin.py @@ -1,20 +1,20 @@ +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: +except ImportError: # Django < 3.0 from django.utils.http import ( is_safe_url as url_has_allowed_host_and_scheme, ) -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 diff --git a/two_factor/apps.py b/two_factor/apps.py index e627f2a8f..41e7dff34 100644 --- a/two_factor/apps.py +++ b/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()