From c640a9820f57981c4f476f331cb4d0c5d2a12043 Mon Sep 17 00:00:00 2001 From: UltraBot05 <170253496+UltraBot05@users.noreply.github.com> Date: Tue, 24 Feb 2026 09:10:39 +0000 Subject: [PATCH 1/4] [fix] Allow cascade deletions in ReadOnlyAdmin ReadOnlyAdmin.has_delete_permission previously returned False unconditionally, which blocked cascade deletions from parent models (like Organization). Updated the method to check request.resolver_match.url_name. It now correctly blocks direct deletes on the model's own views while delegating to the standard Django permission check for cascade deletes. --- openwisp_utils/admin.py | 11 ++++++++- tests/test_project/tests/test_admin.py | 34 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/openwisp_utils/admin.py b/openwisp_utils/admin.py index bfaba7e2..7af390c4 100644 --- a/openwisp_utils/admin.py +++ b/openwisp_utils/admin.py @@ -34,7 +34,16 @@ def has_add_permission(self, request): return False def has_delete_permission(self, request, obj=None): - return False + 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 ( + f"{opts.app_label}_{opts.model_name}_delete", + f"{opts.app_label}_{opts.model_name}_change", + f"{opts.app_label}_{opts.model_name}_changelist", + ): + return False + return super().has_delete_permission(request, obj) def save_model(self, request, obj, form, change): # pragma: nocover pass diff --git a/tests/test_project/tests/test_admin.py b/tests/test_project/tests/test_admin.py index 73ccf22f..f2155edf 100644 --- a/tests/test_project/tests/test_admin.py +++ b/tests/test_project/tests/test_admin.py @@ -119,6 +119,40 @@ class TestReadOnlyAdmin(ReadOnlyAdmin): ["id", "session_id", "username", "start_time", "stop_time"], ) + def test_readonlyadmin_has_delete_permission(self): + modeladmin = ReadOnlyAdmin(RadiusAccounting, AdminSite()) + + with self.subTest("changelist URL returns False"): + request = self.client.get( + reverse("admin:test_project_radiusaccounting_changelist") + ).wsgi_request + self.assertFalse(modeladmin.has_delete_permission(request)) + + with self.subTest("change URL returns False"): + obj = self._create_radius_accounting(username="test", session_id="1") + request = self.client.get( + reverse("admin:test_project_radiusaccounting_change", args=[obj.pk]) + ).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. + request = self.client.get( + reverse("admin:test_project_radiusaccounting_changelist") + ).wsgi_request + mock_resolver = MagicMock() + mock_resolver.url_name = "index" + request.resolver_match = mock_resolver + self.assertTrue(modeladmin.has_delete_permission(request)) + + with self.subTest("no resolver_match returns True"): + request = self.client.get( + reverse("admin:test_project_radiusaccounting_changelist") + ).wsgi_request + request.resolver_match = None + self.assertTrue(modeladmin.has_delete_permission(request)) + def test_context_processor(self): url = reverse("admin:index") response = self.client.get(url) From de1376f9d99ac13527d4bb35aefcb16316c63a2f Mon Sep 17 00:00:00 2001 From: UltraBot05 <170253496+UltraBot05@users.noreply.github.com> Date: Sun, 8 Mar 2026 16:47:40 +0000 Subject: [PATCH 2/4] [fix] Added negative case for cascade delete permission #256 Added a subTest to verify that cascade deletes still respect child-model permissions. Fixes #256 --- tests/test_project/tests/test_admin.py | 27 ++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/tests/test_project/tests/test_admin.py b/tests/test_project/tests/test_admin.py index f2155edf..3ac86d27 100644 --- a/tests/test_project/tests/test_admin.py +++ b/tests/test_project/tests/test_admin.py @@ -1,8 +1,7 @@ from unittest.mock import MagicMock, patch from django.contrib.admin.sites import AdminSite -from django.contrib.auth import get_user_model -from django.contrib.auth.models import Permission +from django.contrib.auth.models import Permission, User from django.core.exceptions import ImproperlyConfigured from django.test import TestCase from django.urls import reverse @@ -22,8 +21,6 @@ ) from . import AdminTestMixin, CreateMixin -User = get_user_model() - class TestAdmin(AdminTestMixin, CreateMixin, TestCase): TEST_KEY = "w1gwJxKaHcamUw62TQIPgYchwLKn3AA0" @@ -135,6 +132,13 @@ def test_readonlyadmin_has_delete_permission(self): ).wsgi_request self.assertFalse(modeladmin.has_delete_permission(request)) + with self.subTest("delete URL returns False"): + obj = self._create_radius_accounting(username="delete-test", session_id="2") + request = self.client.get( + reverse("admin:test_project_radiusaccounting_delete", args=[obj.pk]) + ).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. @@ -153,6 +157,21 @@ def test_readonlyadmin_has_delete_permission(self): request.resolver_match = None self.assertTrue(modeladmin.has_delete_permission(request)) + with self.subTest("cascade delete without child permission returns False"): + user = User.objects.create( + username="readonly-staff", + password="pass", + is_staff=True, + is_superuser=False, + ) + self.client.force_login(user) + request = self.client.get(reverse("admin:index")).wsgi_request + + mock_resolver = MagicMock() + mock_resolver.url_name = "index" + request.resolver_match = mock_resolver + self.assertFalse(modeladmin.has_delete_permission(request)) + def test_context_processor(self): url = reverse("admin:index") response = self.client.get(url) From b0afbc66f843339ee5e9133479344438f9ae5eb2 Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Mon, 18 May 2026 20:22:05 -0300 Subject: [PATCH 3/4] [fix] Tighten ReadOnlyAdmin delete permissions --- openwisp_utils/admin.py | 17 ++++++-- tests/test_project/tests/test_admin.py | 55 +++++++++++++++++++++----- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/openwisp_utils/admin.py b/openwisp_utils/admin.py index ea27d5c6..7baddd40 100644 --- a/openwisp_utils/admin.py +++ b/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 diff --git a/tests/test_project/tests/test_admin.py b/tests/test_project/tests/test_admin.py index b54ddfa3..5ac1a139 100644 --- a/tests/test_project/tests/test_admin.py +++ b/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) From cf695e5885a1c302782c4a27c500d94a8f6c451c Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Wed, 20 May 2026 17:36:53 -0300 Subject: [PATCH 4/4] [chores] Improved implementation --- openwisp_utils/admin.py | 58 ++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/openwisp_utils/admin.py b/openwisp_utils/admin.py index 7baddd40..82ecf8a9 100644 --- a/openwisp_utils/admin.py +++ b/openwisp_utils/admin.py @@ -12,7 +12,40 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) -class ReadOnlyAdmin(ModelAdmin): +class BlockDeleteAllowCascadeMixin: + """A mixin that allows cascade/bulk deletions while blocking single-row deletion in the change view.""" + + def is_admin_cascade_delete_request(self, request): + """Return True when another admin model is checking cascade deletion.""" + model = self.model + opts = model._meta + resolver_match = getattr(request, "resolver_match", None) + url_name = getattr(resolver_match, "url_name", None) + if not url_name: + return False + 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 + is_parent_delete = url_name.endswith("_delete") + is_parent_bulk_delete = ( + url_name.endswith("_changelist") + and getattr(request, "POST", {}).get("action") == "delete_selected" + ) + return is_parent_delete or is_parent_bulk_delete + + def has_delete_permission(self, request, obj=None): + # Django calls child admins during parent delete confirmations; + # allow only those cascade checks to use normal delete permissions. + if self.is_admin_cascade_delete_request(request): + return super().has_delete_permission(request, obj) + return False + + +class ReadOnlyAdmin(BlockDeleteAllowCascadeMixin, ModelAdmin): """Disables all editing capabilities.""" exclude = tuple() @@ -33,29 +66,6 @@ def get_actions(self, request): def has_add_permission(self, request): return False - 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) - 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 - # 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