[fix] Tighten ReadOnlyAdmin delete permissions

Author: nemesifier

openwisp_utils/admin.py

@@ -37,13 +37,24 @@ def has_delete_permission(self, request, obj=None):
         opts = self.model._meta
         resolver_match = getattr(request, "resolver_match", None)
         url_name = getattr(resolver_match, "url_name", None)
-        if url_name and url_name in (
+        own_admin_urls = (
             f"{opts.app_label}_{opts.model_name}_delete",
             f"{opts.app_label}_{opts.model_name}_change",
             f"{opts.app_label}_{opts.model_name}_changelist",
-        ):
+        )
+        if url_name in own_admin_urls:
             return False
-        return super().has_delete_permission(request, obj)
+        # Django calls child admins during parent delete confirmations;
+        # allow only those cascade checks to use normal delete permissions.
+        is_parent_delete = url_name and url_name.endswith("_delete")
+        is_parent_bulk_delete = (
+            url_name
+            and url_name.endswith("_changelist")
+            and request.POST.get("action") == "delete_selected"
+        )
+        if is_parent_delete or is_parent_bulk_delete:
+            return super().has_delete_permission(request, obj)
+        return False
 
     def save_model(self, request, obj, form, change):  # pragma: nocover
         pass

tests/test_project/tests/test_admin.py

@@ -1,7 +1,9 @@
 from unittest.mock import MagicMock, patch
 
+from django.contrib import admin
 from django.contrib.admin.sites import AdminSite
-from django.contrib.auth.models import Permission, User
+from django.contrib.auth import get_user_model
+from django.contrib.auth.models import Permission
 from django.core.exceptions import ImproperlyConfigured
 from django.test import TestCase
 from django.urls import reverse
@@ -21,6 +23,8 @@
 )
 from . import AdminTestMixin, CreateMixin
 
+User = get_user_model()
+
 
 class TestAdmin(AdminTestMixin, CreateMixin, TestCase):
     TEST_KEY = "w1gwJxKaHcamUw62TQIPgYchwLKn3AA0"
@@ -118,6 +122,8 @@ class TestReadOnlyAdmin(ReadOnlyAdmin):
 
     def test_readonlyadmin_has_delete_permission(self):
         modeladmin = ReadOnlyAdmin(RadiusAccounting, AdminSite())
+        # The Django test client keeps the resolved request on the response;
+        # these assertions call the admin permission method directly.
 
         with self.subTest("changelist URL returns False"):
             request = self.client.get(
@@ -139,23 +145,32 @@ def test_readonlyadmin_has_delete_permission(self):
             ).wsgi_request
             self.assertFalse(modeladmin.has_delete_permission(request))
 
-        with self.subTest("cascade delete from unrelated URL returns True"):
-            # Simulate being called from a parent model's delete
-            # confirmation (cascade), not from the model's own views.
+        with self.subTest("cascade delete from parent delete URL returns True"):
             request = self.client.get(
                 reverse("admin:test_project_radiusaccounting_changelist")
             ).wsgi_request
             mock_resolver = MagicMock()
-            mock_resolver.url_name = "index"
+            mock_resolver.url_name = "test_project_project_delete"
             request.resolver_match = mock_resolver
             self.assertTrue(modeladmin.has_delete_permission(request))
 
-        with self.subTest("no resolver_match returns True"):
+        with self.subTest("parent bulk delete returns True"):
+            request = self.client.post(
+                reverse("admin:test_project_project_changelist"),
+                data={"action": "delete_selected"},
+            ).wsgi_request
+            self.assertTrue(modeladmin.has_delete_permission(request))
+
+        with self.subTest("unrelated admin URL returns False"):
+            request = self.client.get(reverse("admin:index")).wsgi_request
+            self.assertFalse(modeladmin.has_delete_permission(request))
+
+        with self.subTest("no resolver_match returns False"):
             request = self.client.get(
                 reverse("admin:test_project_radiusaccounting_changelist")
             ).wsgi_request
             request.resolver_match = None
-            self.assertTrue(modeladmin.has_delete_permission(request))
+            self.assertFalse(modeladmin.has_delete_permission(request))
 
         with self.subTest("cascade delete without child permission returns False"):
             user = User.objects.create(
@@ -166,12 +181,34 @@ def test_readonlyadmin_has_delete_permission(self):
             )
             self.client.force_login(user)
             request = self.client.get(reverse("admin:index")).wsgi_request
-
             mock_resolver = MagicMock()
-            mock_resolver.url_name = "index"
+            mock_resolver.url_name = "test_project_project_delete"
             request.resolver_match = mock_resolver
             self.assertFalse(modeladmin.has_delete_permission(request))
 
+    def test_readonlyadmin_allows_parent_cascade_delete(self):
+        original_admin = admin.site._registry[Operator].__class__
+        admin.site.unregister(Operator)
+        admin.site.register(Operator, ReadOnlyAdmin)
+        try:
+            project = Project.objects.create(name="test-parent-delete")
+            operator = Operator.objects.create(
+                first_name="Jane", last_name="Doe", project=project
+            )
+            path = reverse("admin:test_project_project_delete", args=[project.pk])
+            response = self.client.get(path)
+            self.assertEqual(response.status_code, 200)
+            self.assertNotContains(
+                response, "your account doesn't have permission to delete"
+            )
+            response = self.client.post(path, data={"post": "yes"}, follow=True)
+            self.assertEqual(response.status_code, 200)
+            self.assertFalse(Project.objects.filter(pk=project.pk).exists())
+            self.assertFalse(Operator.objects.filter(pk=operator.pk).exists())
+        finally:
+            admin.site.unregister(Operator)
+            admin.site.register(Operator, original_admin)
+
     def test_context_processor(self):
         url = reverse("admin:index")
         response = self.client.get(url)