diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d9fcd9e..165ea41 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,7 +16,7 @@ jobs: strategy: matrix: python: ['3.10', '3.11', '3.12'] - django: ['3.2', 4.2'] + django: ['3.2', '4.2'] exclude: - python: '3.12' django: '3.2' diff --git a/README.rst b/README.rst index 8978f10..4c291dd 100644 --- a/README.rst +++ b/README.rst @@ -23,6 +23,7 @@ Features * Multi-factor authentication is enforced for admin users, but... * Allows marking certain authentication backends (like Single-Sign-On solutions) as exempt from this rule +* Works with django-hijack out of the box * Does not get in the way of using django-two-factor-auth for your public UI * Commitment to support (at least) maintained Django LTS versions diff --git a/docs/index.rst b/docs/index.rst index a29bcbe..2b0d4c2 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -20,6 +20,7 @@ Features * Multi-factor authentication is enforced for admin users, but... * Allows marking certain authentication backends (like Single-Sign-On solutions) as exempt from this rule +* Works with django-hijack out of the box * Does not get in the way of using django-two-factor-auth for your public UI * Commitment to support (at least) maintained Django LTS versions diff --git a/docs/reference.rst b/docs/reference.rst index dbec56b..2a08a70 100644 --- a/docs/reference.rst +++ b/docs/reference.rst @@ -65,3 +65,20 @@ Test helpers .. automodule:: maykin_2fa.test :members: + +Django-hijack integration +========================= + +Maykin 2FA works out of the box with django-hijack. It subscribes to the +``hijack_started`` and ``hijack_ended`` signals to install a temporary hijack TOTP +device. + +If you don't specify the setting ``HIJACK_PERMISSION_CHECK`` yet, you should update +this to: + +.. code-block:: python + + HIJACK_PERMISSION_CHECK = "maykin_2fa.hijack.superusers_only_and_is_verified" + +Alternatively, if you have a custom check, make sure to also check +``hijacker.is_verified()``. diff --git a/maykin_2fa/apps.py b/maykin_2fa/apps.py index dfc8fba..948b79f 100644 --- a/maykin_2fa/apps.py +++ b/maykin_2fa/apps.py @@ -1,4 +1,4 @@ -from django.apps import AppConfig +from django.apps import AppConfig, apps class Maykin2FaConfig(AppConfig): @@ -7,3 +7,7 @@ class Maykin2FaConfig(AppConfig): def ready(self): from . import checks # noqa from . import signals # noqa + + # enable django-hijack integration if it's installed + if apps.is_installed("hijack"): + from . import hijack_signals # noqa diff --git a/maykin_2fa/hijack.py b/maykin_2fa/hijack.py new file mode 100644 index 0000000..9a6e110 --- /dev/null +++ b/maykin_2fa/hijack.py @@ -0,0 +1,11 @@ +from hijack.permissions import superusers_only # the library default + +from .typing import VerifiableUser + + +def superusers_only_and_is_verified( + *, hijacker: VerifiableUser, hijacked: VerifiableUser +): + return hijacker.is_verified() and superusers_only( + hijacker=hijacker, hijacked=hijacked + ) diff --git a/maykin_2fa/hijack_signals.py b/maykin_2fa/hijack_signals.py new file mode 100644 index 0000000..de0142a --- /dev/null +++ b/maykin_2fa/hijack_signals.py @@ -0,0 +1,93 @@ +""" +Support for django-hijack in the django admin interface when MFA is enforced. + +Originally authored by @Bartvaderkin as part of +https://github.com/open-formulieren/open-forms. +""" + +from typing import Any + +from django.core.exceptions import SuspiciousOperation +from django.dispatch import receiver +from django.http import HttpRequest + +from django_otp import login as otp_login +from django_otp.plugins.otp_totp.models import TOTPDevice +from hijack.signals import hijack_ended, hijack_started +from two_factor.utils import default_device + +from .typing import VerifiableUser + +HIJACK_DEVICE_NAME = "hijack_device" + + +@receiver(hijack_started, dispatch_uid="maykin_2fa.hijack_started.manage_totp_device") +def handle_hijack_start( + sender: None, + hijacker: VerifiableUser, + hijacked: VerifiableUser, + request: HttpRequest, + **kwargs: Any, +): + """ + Create a hijack device if needed. + + If a staff user gets hijacked, a multi-factor device needs to be set to be able + to use the admin interface as that user. Set a temporary hijack device to achieve + this. + """ + # Crash if the hijacker was not MFA verified. This should prevent the hijack too + # because the hijack views are wrapped in atomic transactions. An explicit + # request.session.save() *may* still make the hijack go through since the signal + # only fires *after* the hijack is performed. + if not hijacker.is_verified(): + raise SuspiciousOperation( + f"User {hijacker.get_username()} hijacked user " + f"{hijacked.get_username()} without being two-factor verified." + ) + + # XXX possibly we can create our own plugin/device type for this? + hijack_device, _ = TOTPDevice.objects.get_or_create( + user=hijacked, + name=HIJACK_DEVICE_NAME, + ) + otp_login(request, hijack_device) + + +@receiver(hijack_ended, dispatch_uid="maykin_2fa.hijack_ended.manage_totp_device") +def handle_hijack_end( + sender: None, + hijacker: VerifiableUser, + hijacked: VerifiableUser, + request: HttpRequest, + **kwargs: Any, +): + """ + 1. Remove any dummy OTP devices for the hijacked user. + 2. Restore the original OTP device for the hijacker. + + Determining the 'original' OTP device for the hijacker is not trivial - we can not + simply store a reference to ``hijacker.otp_device`` in the session data in the + :func:`maykin_2fa.hijack_signals.handle_hijack_start` handler because releasing the + user calls django's :func:`django.contrib.auth.login`, which flushes the + session. Flushing the session causes us to lose that information *before* the + ``hijack_ended`` signal fires. + + Instead, we grab the default device from the hijacker and restore that - at the + time of writing it does not seem to be possible to use multiple devices (see also + https://github.com/maykinmedia/maykin-2fa/issues/11) - so that the hijacker is + verified again. + + * django-hijack validates that a release can only be done after an acquire + * therefore, enforcing a verified user during hijack implies that only verified + users can release + """ + TOTPDevice.objects.filter(user=hijacked, name=HIJACK_DEVICE_NAME).delete() + + # restore 'original' device. See the docstring for why this is not guaranteed to be + # the original device. + original_device = default_device(hijacker) + if original_device is not None: + otp_login(request, original_device) + else: + hijacker.otp_device = None diff --git a/maykin_2fa/typing.py b/maykin_2fa/typing.py new file mode 100644 index 0000000..0a1c692 --- /dev/null +++ b/maykin_2fa/typing.py @@ -0,0 +1,19 @@ +from typing import Protocol + +from django_otp.models import Device + + +class VerifiableUser(Protocol): + """ + The `is_verified` callback gets added through the OTP middleware. + """ + + is_active: bool + is_anonymous: bool + is_authenticated: bool + + otp_device: Device | None + + def get_username(self) -> str: ... + + def is_verified(self) -> bool: ... diff --git a/tests/conftest.py b/tests/conftest.py index 6d92f4b..1f2caa4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,18 @@ +from django.test import Client +from django.urls import reverse + import pytest from django_otp.plugins.otp_static.models import StaticToken from django_otp.util import random_hex +from maykin_2fa.test import get_valid_totp_token + + +def _create_totp_device(user): + # See https://github.com/jazzband/django-two-factor-auth/blob/ + # 3c4888c79e37dc4c137bbccafb5680c1e4b74eaa/tests/test_views_login.py#L225 + return user.totpdevice_set.create(name="default", key=random_hex()) + @pytest.fixture def user(db: None, django_user_model): @@ -32,9 +43,7 @@ def totp_device(user): """ Set up a TOTP generator device for the user fixture. """ - # See https://github.com/jazzband/django-two-factor-auth/blob/ - # 3c4888c79e37dc4c137bbccafb5680c1e4b74eaa/tests/test_views_login.py#L225 - return user.totpdevice_set.create(name="default", key=random_hex()) + return _create_totp_device(user) @pytest.fixture @@ -48,3 +57,41 @@ def recovery_codes(user) -> list[str]: ] StaticToken.objects.bulk_create(tokens) return [token.token for token in tokens] + + +@pytest.fixture +def mfa_admin_user(admin_user): + _create_totp_device(admin_user) + return admin_user + + +@pytest.fixture +def mfa_verified_client(mfa_admin_user): + """ + Log in the MFA admin user and verify with token generator. + """ + # use a separate client instance instead of the default fixture so that both + # can be used in the same test + client = Client() + + admin_login_url = reverse("admin:login") + # login superuser who can hijack + client.post( + admin_login_url, + data={ + "admin_login_view-current_step": "auth", + "auth-username": "admin", + "auth-password": "password", + }, + ) + client.post( + admin_login_url, + data={ + "admin_login_view-current_step": "token", + "token-otp_token": get_valid_totp_token(mfa_admin_user), + }, + ) + # sanity check that we are actually fully verified + admin_index = client.get(reverse("admin:index")) + assert admin_index.status_code == 200 + return client diff --git a/tests/hijack_urls.py b/tests/hijack_urls.py new file mode 100644 index 0000000..4017cf0 --- /dev/null +++ b/tests/hijack_urls.py @@ -0,0 +1,8 @@ +from django.urls import include, path + +from testapp.urls import urlpatterns as base_urlpatterns + +urlpatterns = [ + *base_urlpatterns, + path("hijack/", include("hijack.urls")), +] diff --git a/tests/test_hijack_integration.py b/tests/test_hijack_integration.py new file mode 100644 index 0000000..ed9dfb0 --- /dev/null +++ b/tests/test_hijack_integration.py @@ -0,0 +1,157 @@ +from django.apps import apps +from django.contrib.auth.models import User +from django.test import Client +from django.urls import reverse + +import pytest +from two_factor.utils import default_device + +try: + import hijack +except ImportError: + hijack = None + +pytestmark = [ + pytest.mark.skipif( + hijack is None, reason="Skipping hijack tests, dependency not installed." + ), + pytest.mark.django_db, + pytest.mark.urls("tests.hijack_urls"), +] + + +@pytest.fixture(autouse=True) +def hijack_settings(settings): + settings.MAYKIN_2FA_ALLOW_MFA_BYPASS_BACKENDS = [] + settings.INSTALLED_APPS = settings.INSTALLED_APPS + ["hijack"] + settings.MIDDLEWARE = settings.MIDDLEWARE + [ + "hijack.middleware.HijackUserMiddleware" + ] + return settings + + +def test_hijack_enabled(): + # smoke test for invalid test fixtures/configuration + is_installed = apps.is_installed("hijack") + + assert is_installed + + +def test_can_hijack_staff_user( + mfa_admin_user, client: Client, mfa_verified_client: Client +): + admin_index_url = reverse("admin:index") + # set up other staff user + other_user = User.objects.create_user( + username="other", + password="password", + is_staff=True, + is_superuser=False, + ) + + # can't log in by themselves with mfa enabled + assert default_device(other_user) is None + client.login(username="other", password="password") + other_user_response = client.get(admin_index_url) + assert other_user_response.status_code == 302 + client.logout() + + # do the hijack + response = mfa_verified_client.post( + reverse("hijack:acquire"), + data={ + "user_pk": other_user.pk, + "next": admin_index_url, + }, + ) + assert response.status_code == 302 + + admin_index = mfa_verified_client.get(admin_index_url) + assert admin_index.status_code == 200 + assert admin_index.context["request"].user == other_user + + # and release the user again + mfa_verified_client.post(reverse("hijack:release")) + + admin_index = mfa_verified_client.get(admin_index_url) + assert admin_index.status_code == 200 + assert admin_index.context["request"].user == mfa_admin_user + + +def test_release_without_hijack_not_possible(mfa_admin_user, client): + # Test that MFA cannot be bypassed by username/password login + a release call + client.login(username="admin", password="password") + + response = client.post(reverse("hijack:release")) + + assert response.status_code == 403 + + +def test_hijack_without_verification_not_possible(mfa_admin_user, client): + other_user = User.objects.create_user(username="other", password="password") + client.login(username="admin", password="password") + + # SuspiciousOperation gets converted to HTTP 400 by Django + response = client.post( + reverse("hijack:acquire"), + data={"user_pk": other_user.pk}, + ) + + assert response.status_code == 400 + + +def test_hijack_staff_user_with_bypass_enabled(admin_user, client: Client, settings): + settings.MAYKIN_2FA_ALLOW_MFA_BYPASS_BACKENDS = settings.AUTHENTICATION_BACKENDS + admin_index_url = reverse("admin:index") + # set up other staff user + other_user = User.objects.create_user( + username="other", + password="password", + is_staff=True, + is_superuser=False, + ) + # log in + client.login(username="admin", password="password") + admin_index = client.get(reverse("admin:index")) + assert admin_index.status_code == 200 + + response = client.post( + reverse("hijack:acquire"), + data={ + "user_pk": other_user.pk, + "next": admin_index_url, + }, + ) + assert response.status_code == 302 + + admin_index = client.get(admin_index_url) + assert admin_index.status_code == 200 + assert admin_index.context["request"].user == other_user + + # and release the user again + client.post(reverse("hijack:release")) + + admin_index = client.get(admin_index_url) + assert admin_index.status_code == 200 + assert admin_index.context["request"].user == admin_user + + +def test_custom_permission_check(settings, admin_user, client: Client): + settings.HIJACK_PERMISSION_CHECK = ( + "maykin_2fa.hijack.superusers_only_and_is_verified" + ) + # set up other staff user + other_user = User.objects.create_user( + username="other", + password="password", + is_staff=True, + is_superuser=False, + ) + client.login(username="admin", password="password") + + # do the hijack + response = client.post( + reverse("hijack:acquire"), + data={"user_pk": other_user.pk}, + ) + assert response.status_code == 403 diff --git a/tox.ini b/tox.ini index 4494340..dade556 100644 --- a/tox.ini +++ b/tox.ini @@ -2,6 +2,7 @@ envlist = py{310,311}-django32 py{310,311,312}-django{42} + py310-django42-hijack isort black flake8 @@ -29,6 +30,7 @@ extras = deps = django32: Django~=3.2.0 django42: Django~=4.2.0 + hijack: django-hijack commands = py.test tests \ --cov --cov-report xml:reports/coverage-{envname}.xml \