Cleaned up impersonation code. Fixes: #12

backend/mixins.py

@@ -22,30 +22,18 @@ class OverrideView(Exception):
 
 class VisitorMixin(NM2LayoutMixin):
     """
-    Add self.visitor and self.impersonator to the View for the person visiting
-    the site
+    Add self.visitor to the View for the person visiting the site
     """
     # Define to "dd" "am" or "admin" to raise PermissionDenied if the
     # given test on the visitor fails
     require_visitor = None
 
     def set_visitor_info(self):
-        self.impersonator = None
-
         if not self.request.user.is_authenticated:
             self.visitor = None
         else:
             self.visitor = self.request.user
 
-            # Implement impersonation if requested in session
-            if self.visitor.is_admin:
-                key = self.request.session.get("impersonate", None)
-                if key is not None:
-                    p = bmodels.Person.lookup(key)
-                    if p is not None:
-                        self.impersonator = self.visitor
-                        self.visitor = p
-
     def load_objects(self):
         """
         Hook to set self.* members from request parameters, so that they are
@@ -79,7 +67,6 @@ class VisitorMixin(NM2LayoutMixin):
     def get_context_data(self, **kw):
         ctx = super(VisitorMixin, self).get_context_data(**kw)
         ctx["visitor"] = self.visitor
-        ctx["impersonator"] = self.impersonator
         return ctx
 
 
@@ -121,11 +108,6 @@ class VisitPersonMixin(VisitorMixin):
                 if "am_candidate" in self.person.perms:
                     res.append(NavLink(
                         reverse("admin:backend_am_add") + f"?person={self.person.id}", _("Make AM")))
-                if self.visitor != self.person:
-                    res.append(NavLink(
-                        reverse("impersonate", kwargs={
-                            "key": self.person.lookup_key}) + f"?url={self.request.build_absolute_uri()}",
-                        _("Impersonate"), "random"))
                 if self.person.is_dd:
                     res.append(NavLink(
                         reverse("mia_wat_ping", kwargs={"key": self.person.lookup_key}), _("WAT ping"), "heartbeat"))

backend/models.py

@@ -49,6 +49,7 @@ class PersonManager(BaseUserManager):
 
     def create_superuser(self, email, **other_fields):
         other_fields["is_superuser"] = True
+        other_fields["is_staff"] = True
         return self.create_user(email, **other_fields)
 
     def get_or_none(self, *args, **kw):

backend/unittest.py

@@ -103,7 +103,8 @@ class TestBase(nm2.lib.unittest.TestBase):
             else:
                 raise NotImplementedError(f"{identity.issuer} not supported as identity during testing")
 
-        person = self.persons[person]
+        if isinstance(person, str):
+            person = self.persons[person]
         if person is not None:
             client.force_login(person, backend=self.TEST_AUTH_BACKEND)
         client.visitor = person
@@ -128,7 +129,8 @@ class TestBase(nm2.lib.unittest.TestBase):
             else:
                 raise NotImplementedError(f"{identity.issuer} not supported as identity during testing")
 
-        person = self.persons[person]
+        if isinstance(person, str):
+            person = self.persons[person]
         if person is not None:
             client.force_login(person, backend=self.TEST_AUTH_BACKEND)
         client.visitor = person

impersonate/admin.py

+from django.contrib import admin
+
+# Register your models here.

impersonate/apps.py

+from django.apps import AppConfig
+
+
+class ImpersonateConfig(AppConfig):
+    name = 'impersonate'

impersonate/middleware.py

+from __future__ import annotations
+from django.core.exceptions import ImproperlyConfigured
+from django.contrib.auth import get_user_model
+
+
+class ImpersonateMiddleware:
+    def __init__(self, get_response):
+        self.get_response = get_response
+        self.User = get_user_model()
+
+    def __call__(self, request):
+        # AuthenticationMiddleware is required so that request.user exists.
+        if not hasattr(request, 'user'):
+            raise ImproperlyConfigured(
+                "The impersonator middleware requires the authentication middleware"
+                " to be installed.  Edit your MIDDLEWARE setting to insert"
+                " 'django.contrib.auth.middleware.AuthenticationMiddleware'"
+                " before the ImpersonateMiddleware class.")
+
+        if request.user.is_authenticated:
+            # Implement impersonation if requested in session
+            if request.user.is_staff:
+                pk = request.session.get("impersonate", None)
+                if pk is not None:
+                    try:
+                        user = self.User.objects.get(pk=pk)
+                    except self.User.DoesNotExist:
+                        user = None
+
+                    if user is not None:
+                        request.impersonator = request.user
+                        request.user = user
+
+        return self.get_response(request)

impersonate/models.py

+from django.db import models
+
+# Create your models here.

impersonate/tests.py

+from __future__ import annotations
+from django.test import TestCase
+from django.urls import reverse
+from backend.unittest import TestBase
+from django.contrib.auth import get_user_model
+
+
+class TestPermissions(TestBase, TestCase):
+    def test_impersonate_staff(self):
+        User = get_user_model()
+        visitor = User.objects.create_superuser(email="admin@example.org", fullname="Admin", audit_skip=True)
+        visited = User.objects.create_user(email="user@example.org", fullname="User", audit_skip=True)
+        client = self.make_test_client(visitor)
+
+        response = client.get(reverse("impersonate:whoami"))
+        self.assertJSONEqual(response.content, {
+            'impersonator': None,
+            'impersonator_desc': None,
+            'user': visitor.pk,
+            'user_desc': str(visitor),
+        })
+
+        response = client.post(reverse("impersonate:impersonate"), data={"pk": visited.pk, "next": "/"})
+        self.assertRedirectMatches(response, "^/$")
+
+        response = client.get(reverse("impersonate:whoami"))
+        self.assertJSONEqual(response.content, {
+            'impersonator': visitor.pk,
+            'impersonator_desc': str(visitor),
+            'user': visited.pk,
+            'user_desc': str(visited),
+        })
+
+    def test_impersonate_user(self):
+        User = get_user_model()
+        visitor = User.objects.create_user(email="user@example.org", fullname="User", audit_skip=True)
+        visited = User.objects.create_user(email="user1@example.org", fullname="User1", audit_skip=True)
+        client = self.make_test_client(visitor)
+        response = client.post(reverse("impersonate:impersonate"), data={"pk": visited.pk})
+        self.assertPermissionDenied(response)
+
+        response = client.get(reverse("impersonate:whoami"))
+        self.assertJSONEqual(response.content, {
+            'impersonator': None,
+            'impersonator_desc': None,
+            'user': visitor.pk,
+            'user_desc': str(visitor),
+        })
+
+    def test_impersonate_anonymous(self):
+        User = get_user_model()
+        visited = User.objects.create_user(email="user@example.org", fullname="User", audit_skip=True)
+        client = self.make_test_client(None)
+        response = client.post(reverse("impersonate:impersonate"), data={"pk": visited.pk})
+        self.assertPermissionDenied(response)
+
+        response = client.get(reverse("impersonate:whoami"))
+        self.assertJSONEqual(response.content, {
+            'impersonator': None,
+            'impersonator_desc': None,
+            'user': None,
+            'user_desc': "AnonymousUser",
+        })

impersonate/urls.py

+from __future__ import annotations
+from django.urls import path
+from . import views
+
+app_name = "impersonate"
+urlpatterns = [
+    # Impersonate a user
+    path('impersonate/', views.Impersonate.as_view(), name="impersonate"),
+    path('whoami/', views.Whoami.as_view(), name="whoami"),
+]

impersonate/views.py

+from __future__ import annotations
+from django.utils.translation import ugettext as _
+from django.views.generic import View
+from django.shortcuts import redirect
+from django.core.exceptions import PermissionDenied
+from django.contrib import messages
+from django.contrib.auth import get_user_model
+from django import http
+
+
+class Impersonate(View):
+    def post(self, request, *args, **kw):
+        User = get_user_model()
+        effective_user = getattr(request, "impersonator", None)
+        if effective_user is None:
+            effective_user = request.user
+        if not effective_user.is_authenticated or not effective_user.is_staff:
+            raise PermissionDenied
+        pk = request.POST.get("pk")
+        if pk is None:
+            del request.session["impersonate"]
+            messages.add_message(request, messages.INFO, _("Impersonation canceled"))
+            user = effective_user
+        else:
+            try:
+                user = User.objects.get(pk=pk)
+            except User.DoesNotExist:
+                raise PermissionDenied
+            request.session["impersonate"] = user.pk
+            messages.info(request, _("Impersonating {}").format(user))
+
+        url = request.POST.get("next", None)
+        if url is None:
+            return redirect(user.get_absolute_url())
+        else:
+            return redirect(url)
+
+
+class Whoami(View):
+    def get(self, request, *args, **kw):
+        impersonator = getattr(request, "impersonator", None)
+        return http.JsonResponse({
+            "user": request.user.pk,
+            "user_desc": str(request.user),
+            "impersonator": None if impersonator is None else impersonator.pk,
+            "impersonator_desc": None if impersonator is None else str(impersonator),
+        })

nm2/settings.py

@@ -74,6 +74,7 @@ INSTALLED_APPS = [
     'sitechecks',
     'deploy',
     'signon',
+    'impersonate',
 ]
 
 MIDDLEWARE = [
@@ -87,6 +88,7 @@ MIDDLEWARE = [
     'signon.middleware.SignonMiddleware',
     'django.contrib.messages.middleware.MessageMiddleware',
     'django.middleware.clickjacking.XFrameOptionsMiddleware',
+    'impersonate.middleware.ImpersonateMiddleware',
 ]
 
 AUTHENTICATION_BACKENDS = [

nm2/urls.py

@@ -44,10 +44,9 @@ urlpatterns = [
     path('minechangelogs/', include("minechangelogs.urls")),
     path('sitechecks/', include("sitechecks.urls")),
     path('deploy/', include("deploy.urls")),
-
     path('rest/api/', include(router.urls)),
-
     path('signon/', include("signon.urls")),
+    path('impersonate/', include("impersonate.urls")),
 
     # Uncomment the admin/doc line below to enable admin documentation:
     path('admin/doc/', include('django.contrib.admindocs.urls')),

nmlayout/templates/nm2-base.html

@@ -37,6 +37,13 @@ window.nm2.url_api_people = "{% url 'api_people' %}";
     {% for link in navbar.person %}
       <a class="dropdown-item" href="{{link.url}}">{% if link.icon %}<span class="fa fa-{{link.icon}}"></span> {% endif %}{{link.label}}</a>
     {% endfor %}
+    {% if visitor != person %}
+    <form class="form-inline" method="POST" action="{% url 'impersonate:impersonate' %}">{% csrf_token %}
+      <input type="hidden" name="pk" value="{{person.pk}}">
+      <input type="hidden" name="next" value="{{request.build_absolute_uri}}">
+      <button class="dropdown-item btn btn-link" type="submit"><span class="fa fa-random"></span> {% trans "Impersonate" %}</button>
+    </form>
+    {% endif %}
     </div>
   </li>
   {% endif %}
@@ -71,8 +78,11 @@ window.nm2.url_api_people = "{% url 'api_people' %}";
   </a>
   <div class="dropdown-menu dropdown-menu-right" role="menu" aria-labelledby="navbarDropdown">
   {% block visitor_menu %}
-    {% if impersonator %}
-      <a class="dropdown-item" href="{% url 'impersonate' %}?url={{request.build_absolute_uri}}"><span class="fa fa-random"></span> (really {{impersonator.lookup_key}})</a>
+    {% if request.impersonator %}
+    <form class="form-inline" method="POST" action="{% url 'impersonate:impersonate' %}">{% csrf_token %}
+      <input type="hidden" name="next" value="{{request.build_absolute_uri}}">
+      <button class="dropdown-item btn btn-link" type="submit"><span class="fa fa-random"></span> (really {{request.impersonator.lookup_key}})</button>
+    </form>
     {% endif %}
     {% if user.is_anonymous %}
       <a class="dropdown-item" href="{% url 'dm_claim' %}">{% trans "claim account" %}</a>
@@ -109,11 +119,13 @@ window.nm2.url_api_people = "{% url 'api_people' %}";
 
 {% block page_lead %}
 {{super}}
-{% if impersonator %}
+{% if request.impersonator %}
 <div id="impersonation" class="container-fluid bg-warning">
-<span class="badge badge-pill badge-light"><span class="fa fa-random"></span> {{impersonator.a_link}} as {{visitor.a_link}}</span>
-<a class="badge badge-pill badge-primary" href="{% url 'impersonate' %}?url={{request.build_absolute_uri}}">{% trans "Cancel" %}</a>
-<br>
+<span class="badge badge-pill badge-light"><span class="fa fa-random"></span> {{request.impersonator.a_link}} as {{visitor.a_link}}</span>
+<form class="form-inline" method="POST" action="{% url 'impersonate:impersonate' %}">{% csrf_token %}
+  <input type="hidden" name="next" value="{{request.build_absolute_uri}}">
+  <button class="badge badge-pill badge-primary" type="submit">{% trans "Cancel" %}</button>
+</form>
 {% for perm in visit_perms %}<span class="badge badge-pill badge-info mr-1">{{perm}}</span>{% endfor %}
 </div>
 {% endif %}

restricted/tests/test_permissions.py

+from __future__ import annotations
 from django.test import TestCase
 from django.urls import reverse
 from backend.unittest import PersonFixtureMixin
@@ -7,33 +8,12 @@ import json
 class TestPermissions(PersonFixtureMixin, TestCase):
     @classmethod
     def __add_extra_tests__(cls):
-        non_fd = ["pending", "dc", "dc_ga", "dm", "dm_ga", "dd_nu", "dd_u", "dd_e", "dd_r", "activeam", "oldam"]
-        fd = ["fd", "dam"]
-
-        for visitor in [None] + non_fd:
-            for visited in non_fd + fd:
-                cls._add_method(cls._test_impersonate_fail, visitor, visited)
-
-        for visitor in fd:
-            for visited in non_fd + fd:
-                cls._add_method(cls._test_impersonate_success, visitor, visited)
-
         for visitor in None, "pending", "dc", "dc_ga", "dm", "dm_ga", "dd_e", "dd_r":
             cls._add_method(cls._test_export_fail, visitor)
 
         for visitor in "dd_nu", "dd_u", "activeam", "oldam", "fd", "dam":
             cls._add_method(cls._test_export_success, visitor)
 
-    def _test_impersonate_success(self, visitor, visited):
-        client = self.make_test_client(visitor)
-        response = client.get(reverse("impersonate", kwargs={"key": self.persons[visited].lookup_key}))
-        self.assertRedirectMatches(response, "^/$")
-
-    def _test_impersonate_fail(self, visitor, visited):
-        client = self.make_test_client(visitor)
-        response = client.get(reverse("impersonate", kwargs={"key": self.persons[visited].lookup_key}))
-        self.assertPermissionDenied(response)
-
     def _test_export_success(self, visitor):
         self.persons.dc.email = "private@example.org"
 

restricted/urls.py

-from django.conf.urls import url
-from django.views.generic import RedirectView
+from django.urls import path
 from . import views
 
 urlpatterns = [
-    # Impersonate a user
-    url(r'^impersonate/(?P<key>[^/]+)?$', views.Impersonate.as_view(), name="impersonate"),
     # Export database
-    url(r'^db-export$', views.DBExport.as_view(), name="restricted_db_export"),
+    path('db-export/', views.DBExport.as_view(), name="restricted_db_export"),
     # Mailbox stats
-    url(r'^mailbox-stats$', views.MailboxStats.as_view(), name="mailbox_stats"),
+    path('mailbox-stats/', views.MailboxStats.as_view(), name="mailbox_stats"),
     # Export membership information for salsa
-    url(r'^salsa-export$', views.SalsaExport.as_view(), name="export_salsa"),
-
-    # Compatibility
-    url(r'^ammain$', RedirectView.as_view(url="/process/am-dashboard", permanent=True)),
-    url(r'^amprofile(?:/(?P<key>[^/]+))?$', RedirectView.as_view(url="/person/%(key)s/amprofile", permanent=True)),
-    url(r'^minechangelogs/(?P<key>[^/]+)?$', RedirectView.as_view(url="/minechangelogs/search/%(key)s", permanent=True)),
-    url(r'^mail-archive/(?P<key>[^/]+)$', RedirectView.as_view(url="/legacy/mail-archive/%(key)s", permanent=True)),
-    url(r'^display-mail-archive/(?P<key>[^/]+)$', RedirectView.as_view(url="/legacy/display-mail-archive/%(key)s", permanent=True)),
+    path('salsa-export/', views.SalsaExport.as_view(), name="export_salsa"),
 ]

restricted/views.py

@@ -2,11 +2,8 @@ from __future__ import annotations
 from django import http
 from django.core.exceptions import ImproperlyConfigured
 from django.conf import settings
-from django.shortcuts import redirect
-from django.utils.translation import ugettext as _
 from django.core.exceptions import PermissionDenied
 from django.views.generic import View
-from django.contrib import messages
 from django.contrib.sites.shortcuts import get_current_site
 import backend.models as bmodels
 import backend.const as const
@@ -114,26 +111,6 @@ class SalsaExport(VisitorMixin, View):
             return res
 
 
-class Impersonate(View):
-    def get(self, request, key=None, *args, **kw):
-        visitor = request.user
-        if not visitor.is_authenticated or not visitor.is_admin:
-            raise PermissionDenied
-        if key is None:
-            del request.session["impersonate"]
-            messages.add_message(request, messages.INFO, _("Impersonation canceled"))
-        else:
-            person = bmodels.Person.lookup_or_404(key)
-            request.session["impersonate"] = person.lookup_key
-            messages.info(request, _("Impersonating {}").format(person.lookup_key))
-
-        url = request.GET.get("url", None)
-        if url is None:
-            return redirect('home')
-        else:
-            return redirect(url)
-
-
 class MailboxStats(VisitorTemplateView):
     template_name = "restricted/mailbox-stats.html"
     require_visitor = "admin"

signon/templates/signon/login.html

@@ -6,7 +6,7 @@
 
 <h1>nm.debian.org login</h1>
 
-{% if not impersonator %}
+{% if not request.impersonator %}
 {% if not providers_active and not providers_inactive %}
   <p class="lead">
   {% blocktrans %}