From f613082249204c24b373f08d4a4913e8aea46ebe Mon Sep 17 00:00:00 2001 From: stumpylog <797416+stumpylog@users.noreply.github.com> Date: Wed, 22 Jul 2026 10:25:34 -0700 Subject: [PATCH] Fix (beta): batch document user_can_change checks to avoid per-row N+1 DocumentSerializer.get_user_can_change() built a fresh ObjectPermissionChecker and issued a guardian permission-table query for every document row not owned by the requesting user -- correct, but O(N) per page load for any non-superuser viewing documents owned by others. BulkPermissionMixin already batches this exact lookup (2 queries total, regardless of page size) for Correspondent/Tag/DocumentType/CustomField, but was gated behind the rarely-used `full_perms` flag and DocumentViewSet didn't inherit it at all. Changed the gate to "any list action" (cheap: just two extra queries per page) and added BulkPermissionMixin to DocumentViewSet, then updated get_user_can_change to consult that batched context before falling back to a fresh guardian check. Preserves guardian's own superuser shortcut explicitly (has_perm() special- cases is_superuser without a query; the batched-context path doesn't, so it needed its own check) -- covered by a new regression test, since no existing test exercised a superuser viewing an other-owned document. Co-authored-by: Claude Sonnet 5 --- src/documents/serialisers.py | 35 ++++++++++++++++----- src/documents/tests/test_api_permissions.py | 24 ++++++++++++++ src/documents/views.py | 13 ++++---- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index 1770fc91b..add3610af 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -392,15 +392,34 @@ class OwnedObjectSerializer( } def get_user_can_change(self, obj) -> bool: - checker = ObjectPermissionChecker(self.user) if self.user is not None else None - return ( - obj.owner is None - or obj.owner == self.user - or ( - self.user is not None - and checker.has_perm(f"change_{obj.__class__.__name__.lower()}", obj) + if obj.owner is None or obj.owner == self.user: + return True + if self.user is None: + return False + if self.user.is_active and self.user.is_superuser: + # Mirrors guardian's own ObjectPermissionChecker.has_perm() shortcut -- + # superusers aren't necessarily granted explicit object permissions, + # so the batched context below would otherwise incorrectly say no. + return True + + # Prefer the page-level batch computed by BulkPermissionMixin + # (get_serializer_context) over a fresh per-object guardian check, + # which would otherwise query the permission tables once per row. + users_change_perms = self.context.get("users_change_perms") + groups_change_perms = self.context.get("groups_change_perms") + if users_change_perms is not None and groups_change_perms is not None: + if self.user.pk in users_change_perms.get(obj.pk, []): + return True + user_group_ids = getattr(self, "_user_group_ids", None) + if user_group_ids is None: + user_group_ids = set(self.user.groups.values_list("id", flat=True)) + self._user_group_ids = user_group_ids + return bool( + user_group_ids.intersection(groups_change_perms.get(obj.pk, [])), ) - ) + + checker = ObjectPermissionChecker(self.user) + return checker.has_perm(f"change_{obj.__class__.__name__.lower()}", obj) @staticmethod def get_shared_object_pks(objects: Iterable): diff --git a/src/documents/tests/test_api_permissions.py b/src/documents/tests/test_api_permissions.py index 8caf118d8..e693886a2 100644 --- a/src/documents/tests/test_api_permissions.py +++ b/src/documents/tests/test_api_permissions.py @@ -568,6 +568,30 @@ class TestApiAuth(DirectoriesMixin, APITestCase): self.assertNotIn("user_can_change", results[0]) self.assertNotIn("is_shared_by_requester", results[0]) + def test_superuser_user_can_change_without_explicit_grant(self) -> None: + """ + A superuser has implicit change access to every document, even one + owned by someone else with no explicit guardian grant -- mirrors + guardian's own ObjectPermissionChecker.has_perm() superuser shortcut. + """ + superuser = User.objects.create_superuser(username="admin") + other_user = User.objects.create_user(username="user2") + Document.objects.create( + title="Test", + content="content", + checksum="1", + owner=other_user, + ) + + self.client.force_authenticate(superuser) + + response = self.client.get("/api/documents/", format="json") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + results = response.json()["results"] + self.assertEqual(len(results), 1) + self.assertTrue(results[0]["user_can_change"]) + @mock.patch("allauth.mfa.adapter.DefaultMFAAdapter.is_mfa_enabled") def test_basic_auth_mfa_enabled(self, mock_is_mfa_enabled) -> None: """ diff --git a/src/documents/views.py b/src/documents/views.py index ac49ae769..1e4527018 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -431,14 +431,12 @@ class BulkPermissionMixin: This avoid fetching permissions object by object in database. """ context = super().get_serializer_context() - try: - full_perms = get_boolean( - str(self.request.query_params.get("full_perms", "false")), - ) - except ValueError: - full_perms = False - if not full_perms: + if getattr(self, "action", None) != "list": + # Batching only pays off across a page of objects; for single-object + # actions (retrieve, update, ...) the per-object fallback in + # get_user_can_change()/_get_perms() is cheap and avoids scanning + # the whole queryset here. return context # Check which objects are being paginated @@ -943,6 +941,7 @@ class EmailDocumentDetailSchema(EmailSerializer): ), ) class DocumentViewSet( + BulkPermissionMixin, PassUserMixin, RetrieveModelMixin, UpdateModelMixin,