diff --git a/src/documents/permissions.py b/src/documents/permissions.py index 05f203b8c..bc9ea82cd 100644 --- a/src/documents/permissions.py +++ b/src/documents/permissions.py @@ -6,7 +6,9 @@ from django.contrib.auth.models import Permission from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType from django.db.models import Case +from django.db.models import CharField from django.db.models import Count +from django.db.models import F from django.db.models import IntegerField from django.db.models import Model from django.db.models import Q @@ -349,6 +351,7 @@ def permitted_object_ids( perm: str, *, include_deleted: bool = False, + parent_field: str | None = None, ) -> QuerySet[int]: """ Generic version of ``permitted_document_ids`` for any model with an @@ -357,6 +360,24 @@ def permitted_object_ids( soft-delete pattern (currently only ``Document``); for every other model it is accepted but has no effect, since those models have no soft-delete concept. + + ``parent_field`` names a self-referencing foreign key whose target + authorizes the row (``Document.root_document``). A row with a parent is + visible exactly when its parent is, judged by the parent's owner and + grants, so the row's own owner and grants are ignored. + + Guardian stores ``object_pk`` as a string, so the row key is cast to a + string and tested against the user's and groups' grants with a single + uncorrelated ``IN``. Postgres and SQLite build that set once. MariaDB + evaluates it as an index probe per row, which is cheap because the + lookups use guardian's unique indexes. Casting every ``object_pk`` to an + integer instead cannot use an index, and MariaDB cannot materialize it + inside the owner ``OR``, so it re-scans the user's grants for every row. + A correlated ``EXISTS`` per grant fixes MariaDB too, but Postgres and + SQLite re-run it for every row and end up slower than the original. The + user's groups are matched with an ``IN`` subquery rather than a join + through the membership table, which SQLite plans badly once the grant + tables grow. """ has_soft_delete = hasattr(model, "global_objects") manager = ( @@ -364,8 +385,21 @@ def permitted_object_ids( ) base_qs = manager.all().only("id", "owner") + owner_field, key_field = "owner", "pk" + if parent_field is not None: + owner_field, key_field = "authorizing_owner", "authorizing_id" + base_qs = base_qs.annotate( + authorizing_id=Coalesce(f"{parent_field}_id", "id"), + authorizing_owner=Case( + When(**{f"{parent_field}_id__isnull": True}, then=F("owner_id")), + default=F(f"{parent_field}__owner_id"), + output_field=IntegerField(), + ), + ) + unowned = Q(**{f"{owner_field}__isnull": True}) + if user is None or not getattr(user, "is_authenticated", False): - return base_qs.filter(owner__isnull=True).values_list("id", flat=True) + return base_qs.filter(unowned).values_list("id", flat=True) # Deactivated users get nothing, deactivated superusers included, so this # has to come before the superuser shortcut. guardian's @@ -389,21 +423,26 @@ def permitted_object_ids( "permission__content_type": content_type, } - user_perm_ids = ( - UserObjectPermission.objects.filter(user=user, **perm_filter) - .annotate(object_pk_int=Cast("object_pk", IntegerField())) - .values_list("object_pk_int", flat=True) - ) - group_perm_ids = ( - GroupObjectPermission.objects.filter(group__user=user, **perm_filter) - .annotate(object_pk_int=Cast("object_pk", IntegerField())) - .values_list("object_pk_int", flat=True) - ) - permitted_ids = user_perm_ids.union(group_perm_ids) + # Both grant sets are compared to the row key as strings, exactly as + # guardian stores them, and are uncorrelated, so each engine can build the + # set once instead of probing per row. + user_keys = UserObjectPermission.objects.filter( + user=user, + **perm_filter, + ).values_list("object_pk", flat=True) + group_keys = GroupObjectPermission.objects.filter( + group_id__in=user.groups.values("id"), + **perm_filter, + ).values_list("object_pk", flat=True) + permitted_keys = user_keys.union(group_keys, all=True) - return base_qs.filter( - Q(owner=user) | Q(owner__isnull=True) | Q(id__in=permitted_ids), - ).values_list("id", flat=True) + return ( + base_qs.annotate(permitted_key=Cast(key_field, CharField(max_length=64))) + .filter( + Q(**{owner_field: user.pk}) | unowned | Q(permitted_key__in=permitted_keys), + ) + .values_list("id", flat=True) + ) ModelT = TypeVar("ModelT", bound=Model) @@ -471,30 +510,16 @@ def permitted_document_ids( ``include_deleted=True`` for callers that need to check permission on soft-deleted documents (e.g. trash restore). This intentionally avoids ``get_objects_for_user`` to keep the subquery small and index-friendly. - """ - return permitted_object_ids(user, Document, perm, include_deleted=include_deleted) - -def documents_without_permitted_root( - documents: QuerySet[Document], - user: User | None, - *, - perm: str = "view_document", - include_deleted: bool = False, -) -> QuerySet[Document]: + A version is authorized by its root document, so a version's own owner and + grants never matter. """ - The documents the user lacks ``perm`` on. Versions are authorized by their - root document, so a version's own owner is ignored. A single query, without - loading the documents or joining the root. - """ - return documents.annotate( - root_id=Coalesce("root_document_id", "id"), - ).exclude( - root_id__in=permitted_document_ids( - user, - perm=perm, - include_deleted=include_deleted, - ), + return permitted_object_ids( + user, + Document, + perm, + include_deleted=include_deleted, + parent_field="root_document", ) diff --git a/src/documents/tests/test_permission_filtering_security.py b/src/documents/tests/test_permission_filtering_security.py index f48095d48..774c67803 100644 --- a/src/documents/tests/test_permission_filtering_security.py +++ b/src/documents/tests/test_permission_filtering_security.py @@ -32,6 +32,8 @@ from paperless_testing.permissions import grant_global from paperless_testing.permissions import grant_object if TYPE_CHECKING: + from django.contrib.auth.models import User + from paperless_testing.dirs import PaperlessDirs @@ -178,6 +180,296 @@ class TestPermittedDocumentIdsIncludeDeleted: ) +@pytest.mark.django_db +class TestPermittedDocumentIdsVersions: + """ + A version is authorized by its root document: the version's own owner and + grants never matter. + """ + + @pytest.mark.parametrize( + ("root_owner", "version_owner", "expected_visible"), + [ + pytest.param( + "other", + "nobody", + False, + id="unowned-version-of-private-root", + ), + pytest.param("other", "user", False, id="own-version-of-private-root"), + pytest.param("user", "other", True, id="foreign-version-of-own-root"), + pytest.param("user", "nobody", True, id="unowned-version-of-own-root"), + pytest.param("nobody", "other", True, id="private-version-of-unowned-root"), + ], + ) + def test_version_follows_root_owner( + self, + root_owner: str, + version_owner: str, + *, + expected_visible: bool, + ) -> None: + """ + GIVEN: + - A root document and a version with differing owners + WHEN: + - The permitted document ids are resolved for the user + THEN: + - The version is visible exactly when its root is + """ + user = UserFactory() + owners = {"user": user, "other": UserFactory(), "nobody": None} + root = DocumentFactory(owner=owners[root_owner]) + version = DocumentFactory(root_document=root, owner=owners[version_owner]) + + visible = set(permitted_document_ids(user)) + + assert (version.pk in visible) is expected_visible + assert (root.pk in visible) is expected_visible + + @staticmethod + def grantee(user: User, kind: str) -> User | Group: + """The user itself, or a new group the user belongs to.""" + if kind == "user": + return user + group = Group.objects.create(name="shared") + user.groups.add(group) + return group + + @pytest.mark.parametrize( + "grantee_kind", + [pytest.param("user", id="user"), pytest.param("group", id="group")], + ) + def test_grant_on_root_applies_to_version(self, grantee_kind: str) -> None: + """ + GIVEN: + - A private root document shared with a user or one of their groups + - A version of it owned by someone else + WHEN: + - The permitted document ids are resolved for the user + THEN: + - Both the root and the version are visible + - A user without the grant sees neither + """ + user = UserFactory() + stranger = UserFactory() + root = DocumentFactory(owner=UserFactory()) + version = DocumentFactory(root_document=root, owner=UserFactory()) + grant_object(self.grantee(user, grantee_kind), root, "view_document") + + assert_visible_document_ids( + permitted_document_ids(user), + expected_visible=[root.pk, version.pk], + expected_hidden=[], + ) + assert_visible_document_ids( + permitted_document_ids(stranger), + expected_visible=[], + expected_hidden=[root.pk, version.pk], + ) + + @pytest.mark.parametrize( + "grantee_kind", + [pytest.param("user", id="user"), pytest.param("group", id="group")], + ) + def test_grant_on_version_is_ignored(self, grantee_kind: str) -> None: + """ + GIVEN: + - A private root document + - A version with an explicit grant for the user or one of their groups + WHEN: + - The permitted document ids are resolved for the user + THEN: + - Neither the root nor the version is visible + """ + user = UserFactory() + root = DocumentFactory(owner=UserFactory()) + version = DocumentFactory(root_document=root, owner=UserFactory()) + grant_object(self.grantee(user, grantee_kind), version, "view_document") + + assert_visible_document_ids( + permitted_document_ids(user), + expected_visible=[], + expected_hidden=[root.pk, version.pk], + ) + + def test_grant_on_one_root_does_not_reach_another_roots_version(self) -> None: + """ + GIVEN: + - Two private roots, each with a version + - The user may view only the first root + WHEN: + - The permitted document ids are resolved for the user + THEN: + - Only the first root and its version are visible + """ + user = UserFactory() + first = DocumentFactory(owner=UserFactory()) + first_version = DocumentFactory(root_document=first, owner=UserFactory()) + second = DocumentFactory(owner=UserFactory()) + second_version = DocumentFactory(root_document=second, owner=user) + grant_object(user, first, "view_document") + + assert_visible_document_ids( + permitted_document_ids(user), + expected_visible=[first.pk, first_version.pk], + expected_hidden=[second.pk, second_version.pk], + ) + + def test_user_in_several_groups(self) -> None: + """ + GIVEN: + - A user in two groups + - Two private roots shared with one group each, and a third shared with nobody + - A version of each root + WHEN: + - The permitted document ids are resolved for the user + THEN: + - The two shared roots and their versions are visible + - The third root and its version are not + """ + user = UserFactory() + groups = [Group.objects.create(name=f"group{i}") for i in range(2)] + user.groups.add(*groups) + shared = [DocumentFactory(owner=UserFactory()) for _ in groups] + for root, group in zip(shared, groups, strict=True): + grant_object(group, root, "view_document") + unshared = DocumentFactory(owner=UserFactory()) + shared_versions = [ + DocumentFactory(root_document=root, owner=UserFactory()) for root in shared + ] + unshared_version = DocumentFactory(root_document=unshared, owner=None) + + assert_visible_document_ids( + permitted_document_ids(user), + expected_visible=[ + *(root.pk for root in shared), + *(version.pk for version in shared_versions), + ], + expected_hidden=[unshared.pk, unshared_version.pk], + ) + + def test_permission_is_resolved_through_the_root(self) -> None: + """ + GIVEN: + - A private root document where the user may view and change + WHEN: + - The permitted ids are resolved for view, change and delete + THEN: + - The version is visible for view and change only + """ + user = UserFactory() + root = DocumentFactory(owner=UserFactory()) + version = DocumentFactory(root_document=root, owner=UserFactory()) + grant_object(user, root, "view_document", "change_document") + + assert version.pk in set(permitted_document_ids(user)) + assert version.pk in set(permitted_document_ids(user, perm="change_document")) + assert version.pk in set( + permitted_document_ids(user, perm="documents.change_document"), + ) + assert version.pk not in set( + permitted_document_ids(user, perm="delete_document"), + ) + + def test_anonymous_sees_versions_of_unowned_roots_only(self) -> None: + """ + GIVEN: + - A version owned by nobody under a private root + - A version owned by someone under an unowned root + WHEN: + - The permitted document ids are resolved for an anonymous user + THEN: + - Only the version of the unowned root is visible + """ + private_root = DocumentFactory(owner=UserFactory()) + private_version = DocumentFactory(root_document=private_root, owner=None) + open_root = DocumentFactory(owner=None) + open_version = DocumentFactory(root_document=open_root, owner=UserFactory()) + + assert_visible_document_ids( + permitted_document_ids(AnonymousUser()), + expected_visible=[open_root.pk, open_version.pk], + expected_hidden=[private_root.pk, private_version.pk], + ) + + def test_deleted_versions_follow_their_deleted_root(self) -> None: + """ + GIVEN: + - A soft-deleted root document and its version, which deleting the + root soft-deletes too; the version is owned by someone else + WHEN: + - The permitted document ids are resolved with and without deleted + documents + THEN: + - Nothing is visible by default + - With deleted documents included, the version is visible to the + root's owner and not to the version's own owner + """ + owner = UserFactory() + version_owner = UserFactory() + root = DocumentFactory(owner=owner) + version = DocumentFactory(root_document=root, owner=version_owner) + root.delete() + + assert not {root.pk, version.pk} & set(permitted_document_ids(owner)) + assert_visible_document_ids( + permitted_document_ids(owner, include_deleted=True), + expected_visible=[root.pk, version.pk], + expected_hidden=[], + ) + assert_visible_document_ids( + permitted_document_ids(version_owner, include_deleted=True), + expected_visible=[], + expected_hidden=[root.pk, version.pk], + ) + + @pytest.mark.parametrize( + "is_superuser", + [ + pytest.param(False, id="regular-user"), + pytest.param(True, id="superuser"), + ], + ) + def test_inactive_user_sees_no_versions(self, *, is_superuser: bool) -> None: + """ + GIVEN: + - An inactive user, possibly a superuser, who owns a root and its version + WHEN: + - The permitted document ids are resolved for them + THEN: + - Nothing is visible + """ + user = UserFactory(is_active=False, is_superuser=is_superuser) + root = DocumentFactory(owner=user) + version = DocumentFactory(root_document=root, owner=user) + + assert_visible_document_ids( + permitted_document_ids(user), + expected_visible=[], + expected_hidden=[root.pk, version.pk], + ) + + def test_superuser_sees_all_versions(self) -> None: + """ + GIVEN: + - A private root owned by someone else, with a version + WHEN: + - The permitted document ids are resolved for a superuser + THEN: + - Both the root and the version are visible + """ + superuser = UserFactory(superuser=True) + root = DocumentFactory(owner=UserFactory()) + version = DocumentFactory(root_document=root, owner=UserFactory()) + + assert_visible_document_ids( + permitted_document_ids(superuser), + expected_visible=[root.pk, version.pk], + expected_hidden=[], + ) + + @pytest.mark.django_db class TestAiChatAllDocumentsPermissionBoundary: """ diff --git a/src/documents/views.py b/src/documents/views.py index 2e5960d65..0fefaa331 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -174,7 +174,6 @@ from documents.permissions import TrashPermissions from documents.permissions import ViewDocumentsPermissions from documents.permissions import annotate_document_count_by_ids from documents.permissions import annotate_document_count_for_related_queryset -from documents.permissions import documents_without_permitted_root from documents.permissions import get_document_count_filter_for_user from documents.permissions import get_objects_for_user_owner_aware from documents.permissions import has_global_statistics_permission @@ -2117,7 +2116,7 @@ class DocumentViewSet( documents = Document.objects.filter(pk__in=document_ids) if ( request.user is not None - and documents_without_permitted_root(documents, request.user).exists() + and documents.exclude(id__in=permitted_document_ids(request.user)).exists() ): return HttpResponseForbidden("Insufficient permissions") @@ -3009,12 +3008,8 @@ class DocumentOperationPermissionMixin(PassUserMixin, DocumentSelectionMixin): user.has_perm( "documents.change_document", ) - and not Document.global_objects.filter( - pk__in=[doc.pk for doc in root_docs], - ) - .exclude( - pk__in=permitted_document_ids(user, perm="change_document"), - ) + and not Document.global_objects.filter(pk__in=documents) + .exclude(pk__in=permitted_document_ids(user, perm="change_document")) .exists() ) @@ -3627,7 +3622,7 @@ class SelectionDataView(DocumentSelectionMixin, GenericAPIView[Any]): documents = Document.objects.filter(pk__in=ids) if ( documents.count() != len(ids) - or documents_without_permitted_root(documents, request.user).exists() + or documents.exclude(id__in=permitted_document_ids(request.user)).exists() ): return HttpResponseForbidden("Insufficient permissions") @@ -4124,21 +4119,16 @@ class BulkDownloadView(DocumentSelectionMixin, GenericAPIView[Any]): validated_data=serializer.validated_data, ) documents = Document.objects.filter(pk__in=ids) - versioned_documents = [] compression = serializer.validated_data.get("compression") content = serializer.validated_data.get("content") follow_filename_format = serializer.validated_data.get("follow_formatting") - permitted_ids = set(permitted_document_ids(request.user)) - for document in documents: - root_doc = get_root_document(document) - if root_doc.pk not in permitted_ids: - return HttpResponseForbidden("Insufficient permissions") - versioned_documents.append( - get_latest_version_for_root( - root_doc, - ), - ) + if documents.exclude(id__in=permitted_document_ids(request.user)).exists(): + return HttpResponseForbidden("Insufficient permissions") + versioned_documents = [ + get_latest_version_for_root(get_root_document(document)) + for document in documents + ] if content == "both": strategy_class = OriginalAndArchiveStrategy @@ -4811,7 +4801,7 @@ class ShareLinkBundleViewSet(PassUserMixin, ModelViewSet[ShareLinkBundle]): ) denied_id = ( - documents_without_permitted_root(documents_qs, request.user) + documents_qs.exclude(id__in=permitted_document_ids(request.user)) .order_by("pk") .values_list("pk", flat=True) .first() @@ -5657,11 +5647,12 @@ class TrashView(ListModelMixin, PassUserMixin): if doc_ids is not None else self.filter_queryset(self.get_queryset()).all() ) - if documents_without_permitted_root( - docs, - request.user, - perm="delete_document", - include_deleted=True, + if docs.exclude( + id__in=permitted_document_ids( + request.user, + perm="delete_document", + include_deleted=True, + ), ).exists(): return HttpResponseForbidden("Insufficient permissions") action = serializer.validated_data.get("action")