Harden sponsors admin actions: require change permission and make lock POST-only (#3035)

Author: tonghuaroot

apps/sponsors/templates/sponsors/admin/lock.html

@@ -0,0 +1,35 @@
+{% extends 'admin/base_site.html' %}
+{% load i18n static sponsors %}
+
+{% block extrastyle %}{{ block.super }}<link rel="stylesheet" type="text/css" href="{% static "admin/css/forms.css" %}">{% endblock %}
+
+{% block title %}Lock Sponsorship {{ sponsorship }} | python.org{% endblock %}
+
+{% block breadcrumbs %}
+<div class="breadcrumbs">
+  <a href="{% url 'admin:index' %}">{% trans 'Home' %}</a> &gt
+  <a href="{% url 'admin:app_list' app_label='sponsors' %}">{% trans 'Sponsors' %}</a> &gt
+  <a href="{% url 'admin:sponsors_sponsorship_changelist' %}">{% trans 'Sponsorship' %}</a> &gt
+  <a href="{% url 'admin:sponsors_sponsorship_change' sponsorship.pk %}">{{ sponsorship }}</a> &gt
+  {% trans 'Lock Sponsorship' %}
+</div>
+{% endblock %}
+
+{% block content %}
+<h1>Lock Sponsorship</h1>
+<p>Please review the sponsorship application and click in the Lock button if you want to proceed.</p>
+<div id="content-main">
+<form action="" method="post">
+{% csrf_token %}
+
+<pre>{% full_sponsorship sponsorship display_fee=True %}</pre>
+
+<input name="confirm" value="yes" style="display:none">
+
+<div class="submit-row">
+    <input type="submit" value="Lock" class="default">
+</div>
+
+</form>
+<div>
+</div>{% endblock %}

apps/sponsors/tests/test_views_admin.py

@@ -205,6 +205,87 @@ def test_message_user_if_rejecting_invalid_sponsorship(self):
         assert_message(msg, "Can't reject a Finalized sponsorship.", messages.ERROR)
 
 
+class LockSponsorshipAdminViewTests(TestCase):
+    def setUp(self):
+        self.user = baker.make(settings.AUTH_USER_MODEL, is_staff=True, is_superuser=True)
+        self.client.force_login(self.user)
+        self.sponsorship = baker.make(
+            Sponsorship,
+            status=Sponsorship.APPLIED,
+            submited_by=self.user,
+            _fill_optional=True,
+        )
+        self.url = reverse("admin:sponsors_sponsorship_lock", args=[self.sponsorship.pk])
+
+    def test_display_confirmation_form_on_get(self):
+        response = self.client.get(self.url)
+        self.sponsorship.refresh_from_db()
+
+        self.assertTemplateUsed(response, "sponsors/admin/lock.html")
+        self.assertEqual(response.context["sponsorship"], self.sponsorship)
+        self.assertFalse(self.sponsorship.locked)  # GET must not lock
+
+    def test_lock_sponsorship_on_post(self):
+        response = self.client.post(self.url, data={"confirm": "yes"})
+        self.sponsorship.refresh_from_db()
+
+        expected_url = reverse("admin:sponsors_sponsorship_change", args=[self.sponsorship.pk])
+        self.assertRedirects(response, expected_url, fetch_redirect_response=True)
+        self.assertTrue(self.sponsorship.locked)
+        msg = next(iter(get_messages(response.wsgi_request)))
+        assert_message(msg, "Sponsorship is now locked!", messages.SUCCESS)
+
+    def test_do_not_lock_if_invalid_post(self):
+        response = self.client.post(self.url, data={})
+        self.sponsorship.refresh_from_db()
+        self.assertTemplateUsed(response, "sponsors/admin/lock.html")
+        self.assertFalse(self.sponsorship.locked)  # did not lock
+
+        response = self.client.post(self.url, data={"confirm": "invalid"})
+        self.sponsorship.refresh_from_db()
+        self.assertTemplateUsed(response, "sponsors/admin/lock.html")
+        self.assertFalse(self.sponsorship.locked)
+
+    def test_404_if_sponsorship_does_not_exist(self):
+        self.sponsorship.delete()
+        response = self.client.get(self.url)
+        self.assertEqual(response.status_code, 404)
+
+    def test_login_required(self):
+        login_url = reverse("admin:login")
+        redirect_url = f"{login_url}?next={self.url}"
+        self.client.logout()
+
+        r = self.client.get(self.url)
+
+        self.assertRedirects(r, redirect_url)
+
+    def test_staff_required(self):
+        login_url = reverse("admin:login")
+        redirect_url = f"{login_url}?next={self.url}"
+        self.user.is_staff = False
+        self.user.save()
+        self.client.force_login(self.user)
+
+        r = self.client.get(self.url)
+
+        self.assertRedirects(r, redirect_url, fetch_redirect_response=False)
+
+    def test_change_permission_required(self):
+        # A staff account without the sponsorship change permission must not
+        # reach the action, and a GET must never lock the sponsorship.
+        staff = baker.make(settings.AUTH_USER_MODEL, is_staff=True, is_superuser=False)
+        self.client.force_login(staff)
+
+        get_response = self.client.get(self.url)
+        post_response = self.client.post(self.url, data={"confirm": "yes"})
+        self.sponsorship.refresh_from_db()
+
+        self.assertEqual(get_response.status_code, 403)
+        self.assertEqual(post_response.status_code, 403)
+        self.assertFalse(self.sponsorship.locked)
+
+
 class ApproveSponsorshipAdminViewTests(TestCase):
     def setUp(self):
         self.user = baker.make(settings.AUTH_USER_MODEL, is_staff=True, is_superuser=True)

apps/sponsors/views_admin.py

@@ -2,8 +2,10 @@
 
 import io
 import zipfile
+from functools import wraps
 
 from django.contrib import messages
+from django.core.exceptions import PermissionDenied
 from django.db import transaction
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404, redirect, render
@@ -22,6 +24,24 @@
 from apps.sponsors.models import BenefitFeature, EmailTargetable, SponsorshipCurrentYear
 
 
+def require_change_permission(view):
+    """Require the model's change permission for a custom admin view.
+
+    ``AdminSite.admin_view`` only checks that the user is active staff, so the
+    custom action URLs registered in ``admin.py`` must enforce the per-model
+    permission themselves.
+    """
+
+    @wraps(view)
+    def wrapper(model_admin, request, *args, **kwargs):
+        if not model_admin.has_change_permission(request):
+            raise PermissionDenied
+        return view(model_admin, request, *args, **kwargs)
+
+    return wrapper
+
+
+@require_change_permission
 def preview_contract_view(model_admin, request, pk):
     """Render a contract preview as PDF or DOCX based on the format query parameter."""
     contract = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -34,6 +54,7 @@ def preview_contract_view(model_admin, request, pk):
     return response
 
 
+@require_change_permission
 def reject_sponsorship_view(model_admin, request, pk):
     """Handle rejection of a sponsorship application with confirmation."""
     sponsorship = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -53,6 +74,7 @@ def reject_sponsorship_view(model_admin, request, pk):
     return render(request, "sponsors/admin/reject_application.html", context=context)
 
 
+@require_change_permission
 def approve_sponsorship_view(model_admin, request, pk):
     """Approves a sponsorship and create an empty contract."""
     sponsorship = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -88,6 +110,7 @@ def approve_sponsorship_view(model_admin, request, pk):
     return render(request, "sponsors/admin/approve_application.html", context=context)
 
 
+@require_change_permission
 def approve_signed_sponsorship_view(model_admin, request, pk):
     """Approves a sponsorship and execute contract for existing file."""
     sponsorship = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -123,6 +146,7 @@ def approve_signed_sponsorship_view(model_admin, request, pk):
     return render(request, "sponsors/admin/approve_application.html", context=context)
 
 
+@require_change_permission
 def send_contract_view(model_admin, request, pk):
     """Send a finalized contract to the sponsor for signature."""
     contract = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -147,6 +171,7 @@ def send_contract_view(model_admin, request, pk):
     return render(request, "sponsors/admin/send_contract.html", context=context)
 
 
+@require_change_permission
 def rollback_to_editing_view(model_admin, request, pk):
     """Roll back a sponsorship to editing status with confirmation."""
     sponsorship = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -170,6 +195,7 @@ def rollback_to_editing_view(model_admin, request, pk):
     )
 
 
+@require_change_permission
 def unlock_view(model_admin, request, pk):
     """Unlock a sponsorship to allow editing with confirmation."""
     sponsorship = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -193,17 +219,28 @@ def unlock_view(model_admin, request, pk):
     )
 
 
+@require_change_permission
 def lock_view(model_admin, request, pk):
-    """Lock a sponsorship to prevent further editing."""
+    """Lock a sponsorship to prevent further editing with confirmation."""
     sponsorship = get_object_or_404(model_admin.get_queryset(request), pk=pk)
 
-    sponsorship.locked = True
-    sponsorship.save()
+    if request.method.upper() == "POST" and request.POST.get("confirm") == "yes":
+        sponsorship.locked = True
+        sponsorship.save(update_fields=["locked"])
+        model_admin.message_user(request, "Sponsorship is now locked!", messages.SUCCESS)
 
-    redirect_url = reverse("admin:sponsors_sponsorship_change", args=[sponsorship.pk])
-    return redirect(redirect_url)
+        redirect_url = reverse("admin:sponsors_sponsorship_change", args=[sponsorship.pk])
+        return redirect(redirect_url)
+
+    context = {"sponsorship": sponsorship}
+    return render(
+        request,
+        "sponsors/admin/lock.html",
+        context=context,
+    )
 
 
+@require_change_permission
 def execute_contract_view(model_admin, request, pk):
     """Execute a contract by uploading the signed document."""
     contract = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -234,6 +271,7 @@ def execute_contract_view(model_admin, request, pk):
     return render(request, "sponsors/admin/execute_contract.html", context=context)
 
 
+@require_change_permission
 def nullify_contract_view(model_admin, request, pk):
     """Nullify a contract with confirmation."""
     contract = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -258,6 +296,7 @@ def nullify_contract_view(model_admin, request, pk):
     return render(request, "sponsors/admin/nullify_contract.html", context=context)
 
 
+@require_change_permission
 @transaction.atomic
 def update_related_sponsorships(model_admin, request, pk):
     """Update all related SponsorBenefit from a SponsorshipBenefit.
@@ -288,6 +327,7 @@ def update_related_sponsorships(model_admin, request, pk):
     return render(request, "sponsors/admin/update_related_sponsorships.html", context=context)
 
 
+@require_change_permission
 def list_uploaded_assets(model_admin, request, pk):
     """List and export assets uploaded by the user."""
     sponsorship = get_object_or_404(model_admin.get_queryset(request), pk=pk)
@@ -296,6 +336,7 @@ def list_uploaded_assets(model_admin, request, pk):
     return render(request, "sponsors/admin/list_uploaded_assets.html", context=context)
 
 
+@require_change_permission
 def clone_application_config(model_admin, request):
     """Clone sponsorship application configuration from one year to another."""
     form = CloneApplicationConfigForm()