From f86bc5788009dbc601713c87fea3af46ea6c938e Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:02:46 -0700 Subject: [PATCH] Fix: batch document user_can_change checks to avoid per-row N+1 (#13204) * 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 * Fix (beta): don't batch permissions for tantivy search results UnifiedSearchViewSet.list() returns SearchHit/dict-like objects for text/title/query/more_like_id search requests, not Document ORM instances. Adding BulkPermissionMixin to DocumentViewSet (previous commit) meant its get_serializer_context() ran for search responses too, and its _get_object_perms() -- which expects real model instances with .pk -- crashed on the dict-like hits with AttributeError, turning every search request into a 400. Skip the batching specifically for search requests (existing _is_search_request() check) by calling past BulkPermissionMixin in the MRO; non-search list() calls (which return a real Document queryset) are unaffected and still get the batching. Co-authored-by: Claude Sonnet 5 --------- Co-authored-by: Claude Sonnet 5 --- src/documents/serialisers.py | 35 ++++++++++++++++----- src/documents/tests/test_api_permissions.py | 24 ++++++++++++++ src/documents/views.py | 22 ++++++++----- 3 files changed, 66 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..10eaae00c 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, @@ -2291,6 +2290,15 @@ class UnifiedSearchViewSet(DocumentViewSet): return SearchResultSerializer return DocumentSerializer + def get_serializer_context(self): + if self._is_search_request(): + # BulkPermissionMixin.get_serializer_context() (inherited via + # DocumentViewSet) assumes it's batching permissions for a page of + # real Document instances. Tantivy search results are SearchHit/ + # dict-like objects instead, so skip straight past it here. + return super(BulkPermissionMixin, self).get_serializer_context() + return super().get_serializer_context() + def _get_active_search_params(self, request: Request | None = None) -> list[str]: request = request or self.request return [