From b19edd0b74eb8dd1d08ce36ddadb2802877e08b1 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:33:26 -0700 Subject: [PATCH] Fix: dedupe permission-visible documents when combined with multi-tag ALL filtering (#13331) (#13345) The permission filter OR'd three querysets together on top of a queryset that could already carry two independent tags__id__all joins, letting a document that matched more than one branch (e.g. unowned + group-permissioned) come back twice. Replaced it with a single id__in filter against the existing permitted_document_ids helper, which is join-free and can't hit this. --- src/documents/filters.py | 27 +++++++ src/documents/permissions.py | 6 +- src/documents/tests/test_api_documents.py | 88 +++++++++++++++++++++++ src/documents/views.py | 3 +- 4 files changed, 120 insertions(+), 4 deletions(-) diff --git a/src/documents/filters.py b/src/documents/filters.py index 427745424..d5dac705c 100644 --- a/src/documents/filters.py +++ b/src/documents/filters.py @@ -37,6 +37,7 @@ from drf_spectacular.utils import extend_schema_field from guardian.utils import get_group_obj_perms_model from guardian.utils import get_user_obj_perms_model from rest_framework import serializers +from rest_framework.filters import BaseFilterBackend from rest_framework.filters import OrderingFilter from rest_framework_guardian.filters import ObjectPermissionsFilter @@ -50,6 +51,7 @@ from documents.models import ShareLink from documents.models import ShareLinkBundle from documents.models import StoragePath from documents.models import Tag +from documents.permissions import permitted_document_ids if TYPE_CHECKING: from collections.abc import Callable @@ -1038,6 +1040,31 @@ class ObjectOwnedOrGrantedPermissionsFilter(ObjectPermissionsFilter): return objects_with_perms | objects_owned | objects_unowned +class DocumentPermissionsFilter(BaseFilterBackend): + """ + A filter backend limiting Document results to those the requesting user + owns, are unowned, or has explicit (user- or group-level) view + permission on. + + Unlike ``ObjectOwnedOrGrantedPermissionsFilter``, this does not build an + ``objects_with_perms | objects_owned | objects_unowned`` union of + querysets derived from the same base queryset. When that base queryset + already carries independent joins on a multi-valued relation (e.g. two + separate joins from ``tags__id__all`` filtering on two tags), each + OR-ed branch can end up pairing those joins' aliases differently, + letting more than one row out of the join's cross product satisfy the + combined WHERE -- returning the same document more than once. Filtering + via a single ``id__in`` against ``permitted_document_ids`` (a plain + subquery, not a join) sidesteps that entirely and is also cheaper than + guardian's join-based permission check. + """ + + def filter_queryset(self, request, queryset, view): + if request.user.is_superuser: + return queryset + return queryset.filter(id__in=permitted_document_ids(request.user)) + + class ObjectOwnedPermissionsFilter(ObjectPermissionsFilter): """ A filter backend that limits results to those where the requesting user diff --git a/src/documents/permissions.py b/src/documents/permissions.py index adfaaec1a..006b77a81 100644 --- a/src/documents/permissions.py +++ b/src/documents/permissions.py @@ -163,7 +163,7 @@ def set_permissions_for_object( ) -def _permitted_document_ids(user): +def permitted_document_ids(user): """ Return a queryset of document IDs the user may view, limited to non-deleted documents. This intentionally avoids ``get_objects_for_user`` to keep the @@ -220,7 +220,7 @@ def get_document_count_filter_for_user(user, related_name: str = "documents"): # Superuser: no permission filtering needed return Q(**{f"{related_name}__deleted_at__isnull": True}) - permitted_ids = _permitted_document_ids(user) + permitted_ids = permitted_document_ids(user) return Q(**{f"{related_name}__id__in": permitted_ids}) @@ -311,7 +311,7 @@ def annotate_document_count_for_related_queryset( queryset, through_model=through_model, related_object_field=related_object_field, - document_ids=_permitted_document_ids(user), + document_ids=permitted_document_ids(user), target_field=target_field, ) diff --git a/src/documents/tests/test_api_documents.py b/src/documents/tests/test_api_documents.py index 2e3e9cbfc..edd9f3df3 100644 --- a/src/documents/tests/test_api_documents.py +++ b/src/documents/tests/test_api_documents.py @@ -14,6 +14,7 @@ from unittest import mock import celery from dateutil import parser from django.conf import settings +from django.contrib.auth.models import Group from django.contrib.auth.models import Permission from django.contrib.auth.models import User from django.core import mail @@ -47,6 +48,8 @@ from documents.models import Workflow from documents.models import WorkflowAction from documents.models import WorkflowTrigger from documents.signals.handlers import run_workflows +from documents.tests.factories import DocumentFactory +from documents.tests.factories import TagFactory from documents.tests.utils import ConsumeTaskMixin from documents.tests.utils import DirectoriesMixin from documents.tests.utils import read_streaming_response @@ -1212,6 +1215,91 @@ class TestDocumentApi(DirectoriesMixin, ConsumeTaskMixin, APITestCase): [u1_doc1.id], ) + def test_document_owned_and_group_shared_not_duplicated_when_filtering_by_tags( + self, + ) -> None: + """ + GIVEN: + - A document owned by a user and also shared with a group the user belongs to + WHEN: + - The user filters documents by more than one tag (tags__id__all) + THEN: + - The document is returned exactly once, not once per permission path + (regression test for https://github.com/paperless-ngx/paperless-ngx/issues/13331) + """ + user = User.objects.create_user("user1") + user.user_permissions.add(*Permission.objects.filter(codename="view_document")) + group = Group.objects.create(name="group1") + user.groups.add(group) + + tag1 = TagFactory() + tag2 = TagFactory() + doc = DocumentFactory(title="shared", owner=user) + doc.tags.add(tag1, tag2) + assign_perm("view_document", group, doc) + + self.client.force_authenticate(user=user) + response = self.client.get( + f"/api/documents/?tags__id__all={tag1.id},{tag2.id}", + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data["count"], 1) + self.assertEqual(response.data["results"][0]["id"], doc.id) + + def test_document_permission_filter_excludes_unrelated_documents(self) -> None: + """ + GIVEN: + - A document owned by one user, with no permission granted to another user + WHEN: + - The unrelated user requests the document list + THEN: + - The document does not appear in their results + """ + owner = User.objects.create_user("owner1") + stranger = User.objects.create_user("stranger1") + stranger.user_permissions.add( + *Permission.objects.filter(codename="view_document"), + ) + + DocumentFactory(title="private", owner=owner) + + self.client.force_authenticate(user=stranger) + response = self.client.get("/api/documents/") + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data["count"], 0) + + def test_document_permission_filter_only_visible_to_group_members(self) -> None: + """ + GIVEN: + - A document shared with a group via object permissions + WHEN: + - A group member and a non-member both request the document list + THEN: + - Only the group member sees the document + """ + owner = User.objects.create_user("owner2") + member = User.objects.create_user("member1") + non_member = User.objects.create_user("nonmember1") + for u in (member, non_member): + u.user_permissions.add(*Permission.objects.filter(codename="view_document")) + + group = Group.objects.create(name="group2") + member.groups.add(group) + + doc = DocumentFactory(title="shared2", owner=owner) + assign_perm("view_document", group, doc) + + self.client.force_authenticate(user=member) + response = self.client.get("/api/documents/") + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data["count"], 1) + self.assertEqual(response.data["results"][0]["id"], doc.id) + + self.client.force_authenticate(user=non_member) + response = self.client.get("/api/documents/") + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data["count"], 0) + def test_pagination_results(self) -> None: """ GIVEN: diff --git a/src/documents/views.py b/src/documents/views.py index a024dd3c8..528d6ced5 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -133,6 +133,7 @@ from documents.file_handling import format_filename from documents.filters import CorrespondentFilterSet from documents.filters import CustomFieldFilterSet from documents.filters import DocumentFilterSet +from documents.filters import DocumentPermissionsFilter from documents.filters import DocumentsOrderingFilter from documents.filters import DocumentTypeFilterSet from documents.filters import ObjectOwnedOrGrantedPermissionsFilter @@ -986,7 +987,7 @@ class DocumentViewSet( DjangoFilterBackend, SearchFilter, DocumentsOrderingFilter, - ObjectOwnedOrGrantedPermissionsFilter, + DocumentPermissionsFilter, ) filterset_class = DocumentFilterSet search_fields = ("title", "correspondent__name", "effective_content")