diff --git a/.gitignore b/.gitignore index 7cd853fbb..80de2fd45 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,5 @@ example/settings_private.py .eggs/ .idea/ + +venv/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a64f2231..843d78e41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ### Added - Python 3.10 support +- If OTP is required and the user doesn't have OTP setup, the user will be redirected to the + OTP Setup page after initial login. ### Changed - default_device utility function now caches the found device on the given user object 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/installation.rst b/docs/installation.rst index 6f1b28b9b..a45b41a7b 100644 --- a/docs/installation.rst +++ b/docs/installation.rst @@ -64,9 +64,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/requirements_dev.txt b/requirements_dev.txt index a03692141..9ce66fe8d 100644 --- a/requirements_dev.txt +++ b/requirements_dev.txt @@ -14,7 +14,6 @@ django-bootstrap-form django-user-sessions # Testing - coverage flake8 tox diff --git a/tests/test_admin.py b/tests/test_admin.py index 57d11225f..d29e8dd95 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -1,5 +1,7 @@ -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 @@ -9,7 +11,7 @@ @override_settings(ROOT_URLCONF='tests.urls_admin') -class AdminPatchTest(TestCase): +class TwoFactorAdminSiteTest(UserMixin, TestCase): def setUp(self): patch_admin() @@ -19,50 +21,98 @@ def tearDown(self): def test(self): response = self.client.get('/admin/', follow=True) - redirect_to = '%s?next=/admin/' % resolve_url(settings.LOGIN_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) + redirect_to = '%s?next=/admin/' % reverse('admin:login') self.assertRedirects(response, redirect_to) @override_settings(ROOT_URLCONF='tests.urls_admin') -class AdminSiteTest(UserMixin, TestCase): - - def setUp(self): - super().setUp() - self.user = self.create_superuser() - self.login_user() +class AdminPatchTest(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 test_default_admin(self): - response = self.client.get('/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) -@override_settings(ROOT_URLCONF='tests.urls_otp_admin') -class OTPAdminSiteTest(UserMixin, TestCase): - - def setUp(self): - super().setUp() + 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() - - 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) + 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_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) + 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) - def test_otp_admin_with_otp(self): - self.enable_otp() + 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() - response = self.client.get('/otp_admin/') + response = self.client.get(index_url) self.assertEqual(response.status_code, 200) + + 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_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(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..3a7fa05f6 100644 --- a/two_factor/admin.py +++ b/two_factor/admin.py @@ -1,20 +1,20 @@ -from django.conf import settings +import warnings +from functools import update_wrapper + 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 +from django.http import HttpResponseRedirect +from django.urls import reverse +from django.utils.decorators import method_decorator +from django.utils.translation import gettext as _ +from django.views.decorators.cache import never_cache +from django.views.decorators.csrf import csrf_protect -try: - from django.utils.http import url_has_allowed_host_and_scheme -except ImportError: - from django.utils.http import ( - is_safe_url as url_has_allowed_host_and_scheme, - ) +from .utils import default_device -class AdminSiteOTPRequiredMixin: +class TwoFactorAdminSiteMixin: """ Mixin for enforcing OTP verified staff users. @@ -22,50 +22,169 @@ class AdminSiteOTPRequiredMixin: use :meth:`has_permission` in order to secure those views. """ + def has_admin_permission(self, request): + return super().has_permission(request) + def has_permission(self, request): """ Returns True if the given HttpRequest has permission to view *at least one* page in the admin site. """ - if not super().has_permission(request): - return False - return request.user.is_verified() - + return self.has_admin_permission(request) and request.user.is_verified() + + def has_mfa_setup(self, request): + otp_device = default_device(request.user) + return otp_device is not None + + def redirect_to_mfa_setup(self, request): + # Already logged-in, but they did not login with MFA and do not + # have MFA enabled on their account. We're going to redirect them + # to the MFA setup. + + # TODO: Add redirect_to functionality to MFA setup. + # TODO: Add message indicating why the user was directed or setup and MFA required + # interstitial page to explain to the user they need to setup MFA. + setup_url = reverse('two_factor:setup') + next = request.GET.get(REDIRECT_FIELD_NAME, reverse('admin:index')) + + # the name redirect_to_login is a little misleading, the function actually + # redirects to the specified view setting the REDIRECT_FIELD_NAME to the + # next value. We're not logging in here, we're just sending the user to the + # MFA setup screen. + return redirect_to_login(next, setup_url) + + @method_decorator(never_cache) def login(self, request, extra_context=None): """ - Redirects to the site login page for the given HttpRequest. + Display the login form for the given HttpRequest. """ - redirect_to = request.POST.get(REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME)) + if request.method == "GET": + if self.has_admin_permission(request) and not self.has_mfa_setup(request): + return self.redirect_to_mfa_setup(request) + + if self.has_permission(request): + # Already logged-in with OTP verification. redirect to admin index + index_path = reverse("admin:index", current_app=self.name) + return HttpResponseRedirect(index_path) + + # Since this module gets imported in the application's root package, + # it cannot import models from other applications at the module level, + # and django.contrib.admin.forms eventually imports User. + from two_factor.views.core import LoginView + + context = { + **self.each_context(request), + "title": _("Log in"), + "subtitle": None, + "app_path": request.get_full_path(), + "username": request.user.get_username(), + } + if ( + REDIRECT_FIELD_NAME not in request.GET + and REDIRECT_FIELD_NAME not in request.POST + ): + context[REDIRECT_FIELD_NAME] = reverse("admin:index", current_app=self.name) + context.update(extra_context or {}) + defaults = { + 'extra_context': context, + } + + request.current_app = self.name + return LoginView.as_view(**defaults)(request, context) + + def admin_view(self, view, cacheable=False): + """ + Decorator to create an admin view attached to this ``AdminSite``. This + wraps the view and provides permission checking by calling + ``self.has_permission``. + + You'll want to use this from within ``AdminSite.get_urls()``: - 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) + class MyAdminSite(AdminSite): - return redirect_to_login(redirect_to) + def get_urls(self): + from django.urls import path + urls = super().get_urls() + urls += [ + path('my_view/', self.admin_view(some_view)) + ] + return urls -class AdminSiteOTPRequired(AdminSiteOTPRequiredMixin, AdminSite): + By default, admin_views are marked non-cacheable using the + ``never_cache`` decorator. If the view can be safely cached, set + cacheable=True. + """ + def inner(request, *args, **kwargs): + if self.has_admin_permission(request) and not self.has_mfa_setup(request): + return self.redirect_to_mfa_setup(request) + + if not self.has_permission(request): + if request.path == reverse('admin:logout', current_app=self.name): + index_path = reverse('admin:index', current_app=self.name) + return HttpResponseRedirect(index_path) + + # Inner import to prevent django.contrib.admin (app) from + # importing django.contrib.auth.models.User (unrelated model). + from django.contrib.auth.views import redirect_to_login + return redirect_to_login( + request.get_full_path(), + reverse('%s:login' % self.name, current_app=self.name) + ) + return view(request, *args, **kwargs) + + if not cacheable: + inner = never_cache(inner) + # We add csrf_protect here so this function can be used as a utility + # function for any view, without having to repeat 'csrf_protect'. + if not getattr(view, 'csrf_exempt', False): + inner = csrf_protect(inner) + return update_wrapper(inner, view) + + +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)) +class AdminSiteOTPRequiredMixin(TwoFactorAdminSiteMixin): + warnings.warn('AdminSiteOTPRequiredMixin is deprecated by TwoFactorAdminSiteMixin, please update.', + category=DeprecationWarning) + pass - 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) +class AdminSiteOTPRequired(TwoFactorAdminSite): + warnings.warn('AdminSiteOTPRequired is deprecated by TwoFactorAdminSite, please update.', + category=DeprecationWarning) + pass + + +def patch_admin(): + warnings.warn('two-factor admin patching will be removed, use TwoFactorAdminSite or TwoFactorAdminSiteMixin.', + category=DeprecationWarning) + # overrides + setattr(AdminSite, 'login', TwoFactorAdminSiteMixin.login) + setattr(AdminSite, 'admin_view', TwoFactorAdminSiteMixin.admin_view) + setattr(AdminSite, 'has_permission', TwoFactorAdminSiteMixin.has_permission) + # additions + setattr(AdminSite, 'has_admin_permission', original_has_permission) + setattr(AdminSite, 'has_mfa_setup', TwoFactorAdminSiteMixin.has_mfa_setup) + setattr(AdminSite, 'redirect_to_mfa_setup', TwoFactorAdminSiteMixin.redirect_to_mfa_setup) def unpatch_admin(): + warnings.warn('django-two-factor admin patching is deprecated, use TwoFactorAdminSite or TwoFactorAdminSiteMixin.', + category=DeprecationWarning) + # we really only need unpatching in our tests so this can be a noop. + # overrides setattr(AdminSite, 'login', original_login) + setattr(AdminSite, 'admin_view', original_admin_view) + setattr(AdminSite, 'has_permission', original_has_permission) + # NOTE: this unpatching doesn't really work, but becuase it just patches in our mixin it isn't harmful. original_login = AdminSite.login +original_admin_view = AdminSite.admin_view +original_has_permission = AdminSite.has_permission diff --git a/two_factor/views/core.py b/two_factor/views/core.py index cbb6a24cc..e5c2fb33d 100644 --- a/two_factor/views/core.py +++ b/two_factor/views/core.py @@ -387,7 +387,7 @@ def dispatch(self, request, *args, **kwargs): @class_view_decorator(never_cache) @class_view_decorator(login_required) -class SetupView(IdempotentSessionWizardView): +class SetupView(SuccessURLAllowedHostsMixin, IdempotentSessionWizardView): """ View for handling OTP setup using a wizard. @@ -410,6 +410,27 @@ class SetupView(IdempotentSessionWizardView): # Other forms are dynamically added in get_form_list() ) + # Copied from django.conrib.auth.views.LoginView (Branch: stable/1.11.x) + # https://github.com/django/django/blob/58df8aa40fe88f753ba79e091a52f236246260b3/django/contrib/auth/views.py#L63 + def get_success_url(self): + url = self.get_redirect_url() + return url or reverse('two_factor:setup_complete') + + # Copied from django.conrib.auth.views.LoginView (Branch: stable/1.11.x) + # https://github.com/django/django/blob/58df8aa40fe88f753ba79e091a52f236246260b3/django/contrib/auth/views.py#L67 + def get_redirect_url(self): + """Return the user-originating redirect URL if it's safe.""" + redirect_to = self.request.POST.get( + REDIRECT_FIELD_NAME, + self.request.GET.get(REDIRECT_FIELD_NAME, '') + ) + url_is_safe = url_has_allowed_host_and_scheme( + url=redirect_to, + allowed_hosts=self.get_success_url_allowed_hosts(), + require_https=self.request.is_secure(), + ) + return redirect_to if url_is_safe else '' + def get_method(self): method_data = self.storage.validated_step_data.get('method', {}) method_key = method_data.get('method', None) @@ -420,7 +441,7 @@ def get(self, request, *args, **kwargs): Start the setup wizard. Redirect if already enabled. """ if default_device(self.request.user): - return redirect(self.success_url) + return redirect(self.get_success_url()) return super().get(request, *args, **kwargs) def get_form(self, step=None, **kwargs): @@ -489,7 +510,7 @@ def done(self, form_list, **kwargs): raise NotImplementedError("Unknown method '%s'" % method.code) django_otp.login(self.request, device) - return redirect(self.success_url) + return redirect(self.get_success_url()) def get_form_kwargs(self, step=None): kwargs = {}