From 5ff1dd8426aea403d03c92effe6a0012b36b0cca Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 17 Sep 2020 16:24:53 +0200 Subject: [PATCH] core: move impersonation to core, add tests, add better permission checks --- passbook/admin/middleware.py | 26 --------- passbook/admin/settings.py | 5 -- .../templates/administration/user/list.html | 2 +- passbook/core/middleware.py | 26 +++++++++ .../migrations/0010_auto_20200917_1021.py | 24 ++++++++ passbook/core/models.py | 5 +- passbook/core/templates/base/skeleton.html | 4 +- passbook/core/tests/test_impersonation.py | 55 +++++++++++++++++++ passbook/core/urls.py | 13 ++++- passbook/core/views/impersonate.py | 55 +++++++++++++++++++ passbook/root/settings.py | 1 + 11 files changed, 180 insertions(+), 36 deletions(-) delete mode 100644 passbook/admin/middleware.py delete mode 100644 passbook/admin/settings.py create mode 100644 passbook/core/middleware.py create mode 100644 passbook/core/migrations/0010_auto_20200917_1021.py create mode 100644 passbook/core/tests/test_impersonation.py create mode 100644 passbook/core/views/impersonate.py diff --git a/passbook/admin/middleware.py b/passbook/admin/middleware.py deleted file mode 100644 index 0be230711..000000000 --- a/passbook/admin/middleware.py +++ /dev/null @@ -1,26 +0,0 @@ -"""passbook admin Middleware to impersonate users""" - -from passbook.core.models import User - - -def impersonate(get_response): - """Middleware to impersonate users""" - - def middleware(request): - """Middleware to impersonate users""" - - # User is superuser and has __impersonate ID set - if request.user.is_superuser and "__impersonate" in request.GET: - request.session["impersonate_id"] = request.GET["__impersonate"] - # user wants to stop impersonation - elif "__unimpersonate" in request.GET and "impersonate_id" in request.session: - del request.session["impersonate_id"] - - # Actually impersonate user - if request.user.is_superuser and "impersonate_id" in request.session: - request.user = User.objects.get(pk=request.session["impersonate_id"]) - - response = get_response(request) - return response - - return middleware diff --git a/passbook/admin/settings.py b/passbook/admin/settings.py deleted file mode 100644 index 09bb5c9f7..000000000 --- a/passbook/admin/settings.py +++ /dev/null @@ -1,5 +0,0 @@ -"""passbook admin settings""" - -MIDDLEWARE = [ - "passbook.admin.middleware.impersonate", -] diff --git a/passbook/admin/templates/administration/user/list.html b/passbook/admin/templates/administration/user/list.html index 770d903cd..78a6e9dc0 100644 --- a/passbook/admin/templates/administration/user/list.html +++ b/passbook/admin/templates/administration/user/list.html @@ -55,7 +55,7 @@ {% trans 'Edit' %} {% trans 'Delete' %} {% trans 'Reset Password' %} - {% trans 'Impersonate' %} + {% trans 'Impersonate' %} {% endfor %} diff --git a/passbook/core/middleware.py b/passbook/core/middleware.py new file mode 100644 index 000000000..f1631beb9 --- /dev/null +++ b/passbook/core/middleware.py @@ -0,0 +1,26 @@ +"""passbook admin Middleware to impersonate users""" + +from typing import Callable + +from django.http import HttpRequest, HttpResponse + +SESSION_IMPERSONATE_USER = "passbook_impersonate_user" +SESSION_IMPERSONATE_ORIGINAL_USER = "passbook_impersonate_original_user" + + +class ImpersonateMiddleware: + """Middleware to impersonate users""" + + get_response: Callable[[HttpRequest], HttpResponse] + + def __init__(self, get_response: Callable[[HttpRequest], HttpResponse]): + self.get_response = get_response + + def __call__(self, request: HttpRequest) -> HttpResponse: + # No permission checks are done here, they need to be checked before + # SESSION_IMPERSONATE_USER is set. + + if SESSION_IMPERSONATE_USER in request.session: + request.user = request.session[SESSION_IMPERSONATE_USER] + + return self.get_response(request) diff --git a/passbook/core/migrations/0010_auto_20200917_1021.py b/passbook/core/migrations/0010_auto_20200917_1021.py new file mode 100644 index 000000000..29c3ae7a2 --- /dev/null +++ b/passbook/core/migrations/0010_auto_20200917_1021.py @@ -0,0 +1,24 @@ +# Generated by Django 3.1.1 on 2020-09-17 10:21 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("passbook_core", "0009_group_is_superuser"), + ] + + operations = [ + migrations.AlterModelOptions( + name="user", + options={ + "permissions": ( + ("reset_user_password", "Reset Password"), + ("impersonate", "Can impersonate other users"), + ), + "verbose_name": "User", + "verbose_name_plural": "Users", + }, + ), + ] diff --git a/passbook/core/models.py b/passbook/core/models.py index d58a53856..1b4945c2e 100644 --- a/passbook/core/models.py +++ b/passbook/core/models.py @@ -98,7 +98,10 @@ class User(GuardianUserMixin, AbstractUser): class Meta: - permissions = (("reset_user_password", "Reset Password"),) + permissions = ( + ("reset_user_password", "Reset Password"), + ("impersonate", "Can impersonate other users"), + ) verbose_name = _("User") verbose_name_plural = _("Users") diff --git a/passbook/core/templates/base/skeleton.html b/passbook/core/templates/base/skeleton.html index 68f9c158e..8ed64ebf9 100644 --- a/passbook/core/templates/base/skeleton.html +++ b/passbook/core/templates/base/skeleton.html @@ -21,13 +21,13 @@ {% endblock %} - {% if 'impersonate_id' in request.session %} + {% if 'passbook_impersonate_user' in request.session %}
{% blocktrans with user=user %}You're currently impersonating {{ user }}.{% endblocktrans %} - {% trans 'Stop impersonation' %} + {% trans 'Stop impersonation' %}
diff --git a/passbook/core/tests/test_impersonation.py b/passbook/core/tests/test_impersonation.py new file mode 100644 index 000000000..ba365e421 --- /dev/null +++ b/passbook/core/tests/test_impersonation.py @@ -0,0 +1,55 @@ +"""impersonation tests""" +from django.shortcuts import reverse +from django.test.testcases import TestCase + +from passbook.core.models import User + + +class TestImpersonation(TestCase): + """impersonation tests""" + + def setUp(self) -> None: + super().setUp() + self.other_user = User.objects.create(username="to-impersonate") + self.pbadmin = User.objects.get(username="pbadmin") + + def test_impersonate_simple(self): + """test simple impersonation and un-impersonation""" + self.client.force_login(self.pbadmin) + + self.client.get( + reverse( + "passbook_core:impersonate-init", kwargs={"user_id": self.other_user.pk} + ) + ) + + response = self.client.get(reverse("passbook_core:overview")) + self.assertIn(self.other_user.username, response.content.decode()) + self.assertNotIn(self.pbadmin.username, response.content.decode()) + + self.client.get(reverse("passbook_core:impersonate-end")) + + response = self.client.get(reverse("passbook_core:overview")) + self.assertNotIn(self.other_user.username, response.content.decode()) + self.assertIn(self.pbadmin.username, response.content.decode()) + + def test_impersonate_denied(self): + """test impersonation without permissions""" + self.client.force_login(self.other_user) + + self.client.get( + reverse( + "passbook_core:impersonate-init", kwargs={"user_id": self.pbadmin.pk} + ) + ) + + response = self.client.get(reverse("passbook_core:overview")) + self.assertIn(self.other_user.username, response.content.decode()) + self.assertNotIn(self.pbadmin.username, response.content.decode()) + + def test_un_impersonate_empty(self): + """test un-impersonation without impersonating first""" + self.client.force_login(self.other_user) + + response = self.client.get(reverse("passbook_core:impersonate-end")) + self.assertRedirects(response, reverse("passbook_core:overview")) diff --git a/passbook/core/urls.py b/passbook/core/urls.py index df29adde5..881b2e021 100644 --- a/passbook/core/urls.py +++ b/passbook/core/urls.py @@ -1,11 +1,22 @@ """passbook URL Configuration""" from django.urls import path -from passbook.core.views import overview, user +from passbook.core.views import impersonate, overview, user urlpatterns = [ # User views path("-/user/", user.UserSettingsView.as_view(), name="user-settings"), # Overview path("", overview.OverviewView.as_view(), name="overview"), + # Impersonation + path( + "-/impersonation//", + impersonate.ImpersonateInitView.as_view(), + name="impersonate-init", + ), + path( + "-/impersonation/end/", + impersonate.ImpersonateEndView.as_view(), + name="impersonate-end", + ), ] diff --git a/passbook/core/views/impersonate.py b/passbook/core/views/impersonate.py new file mode 100644 index 000000000..009c74d2a --- /dev/null +++ b/passbook/core/views/impersonate.py @@ -0,0 +1,55 @@ +"""passbook impersonation views""" + +from django.http import HttpRequest, HttpResponse +from django.shortcuts import get_object_or_404, redirect +from django.views import View +from structlog import get_logger + +from passbook.core.middleware import ( + SESSION_IMPERSONATE_ORIGINAL_USER, + SESSION_IMPERSONATE_USER, +) +from passbook.core.models import User + +LOGGER = get_logger() + + +class ImpersonateInitView(View): + """Initiate Impersonation""" + + def get(self, request: HttpRequest, user_id: int) -> HttpResponse: + """Impersonation handler, checks permissions""" + if not request.user.has_perm("impersonate"): + LOGGER.debug( + "User attempted to impersonate without permissions", user=request.user + ) + return HttpResponse("Unauthorized", status=401) + + user_to_be = get_object_or_404(User, pk=user_id) + + request.session[SESSION_IMPERSONATE_ORIGINAL_USER] = request.user + request.session[SESSION_IMPERSONATE_USER] = user_to_be + + # TODO Audit log entry + + return redirect("passbook_core:overview") + + +class ImpersonateEndView(View): + """End User impersonation""" + + def get(self, request: HttpRequest) -> HttpResponse: + """End Impersonation handler""" + if ( + SESSION_IMPERSONATE_USER not in request.session + or SESSION_IMPERSONATE_ORIGINAL_USER not in request.session + ): + LOGGER.debug("Can't end impersonation", user=request.user) + return redirect("passbook_core:overview") + + del request.session[SESSION_IMPERSONATE_USER] + del request.session[SESSION_IMPERSONATE_ORIGINAL_USER] + + # TODO: Audit log entry + + return redirect("passbook_core:overview") diff --git a/passbook/root/settings.py b/passbook/root/settings.py index 928eb0f62..fa48db253 100644 --- a/passbook/root/settings.py +++ b/passbook/root/settings.py @@ -179,6 +179,7 @@ MIDDLEWARE = [ "django.middleware.csrf.CsrfViewMiddleware", "django.contrib.messages.middleware.MessageMiddleware", "django.middleware.clickjacking.XFrameOptionsMiddleware", + "passbook.core.middleware.ImpersonateMiddleware", "django_prometheus.middleware.PrometheusAfterMiddleware", ]