From d57e505246865f38b2127760a9c7fe35b06c1d80 Mon Sep 17 00:00:00 2001 From: tonghuaroot Date: Sun, 28 Jun 2026 09:36:00 +0800 Subject: [PATCH] Harden sponsors admin actions: require change permission and make lock POST-only AdminSite.admin_view only checks that the user is active staff, so the custom sponsorship/contract action URLs in apps/sponsors/admin.py did not enforce the per-model permission. Add a require_change_permission decorator applied to each views_admin action. Also give lock_view the same POST + confirmation flow unlock_view already uses, so locking a sponsorship is no longer performed on a plain GET. A new lock.html template mirrors unlock.html. --- .../templates/sponsors/admin/lock.html | 35 ++++++++ apps/sponsors/tests/test_views_admin.py | 81 +++++++++++++++++++ apps/sponsors/views_admin.py | 51 ++++++++++-- 3 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 apps/sponsors/templates/sponsors/admin/lock.html diff --git a/apps/sponsors/templates/sponsors/admin/lock.html b/apps/sponsors/templates/sponsors/admin/lock.html new file mode 100644 index 000000000..39e403146 --- /dev/null +++ b/apps/sponsors/templates/sponsors/admin/lock.html @@ -0,0 +1,35 @@ +{% extends 'admin/base_site.html' %} +{% load i18n static sponsors %} + +{% block extrastyle %}{{ block.super }}{% endblock %} + +{% block title %}Lock Sponsorship {{ sponsorship }} | python.org{% endblock %} + +{% block breadcrumbs %} +
+ {% trans 'Home' %} > + {% trans 'Sponsors' %} > + {% trans 'Sponsorship' %} > + {{ sponsorship }} > + {% trans 'Lock Sponsorship' %} +
+{% endblock %} + +{% block content %} +

Lock Sponsorship

+

Please review the sponsorship application and click in the Lock button if you want to proceed.

+
+
+{% csrf_token %} + +
{% full_sponsorship sponsorship display_fee=True %}
+ + + +
+ +
+ +
+
+
{% endblock %} diff --git a/apps/sponsors/tests/test_views_admin.py b/apps/sponsors/tests/test_views_admin.py index 680c23698..2d211e83c 100644 --- a/apps/sponsors/tests/test_views_admin.py +++ b/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) diff --git a/apps/sponsors/views_admin.py b/apps/sponsors/views_admin.py index 940fe41ae..dfd4566ab 100644 --- a/apps/sponsors/views_admin.py +++ b/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()