diff --git a/src/documents/permissions.py b/src/documents/permissions.py index 70c48b6cc..05f203b8c 100644 --- a/src/documents/permissions.py +++ b/src/documents/permissions.py @@ -14,6 +14,7 @@ from django.db.models import QuerySet from django.db.models import Value from django.db.models import When from django.db.models.functions import Cast +from django.db.models.functions import Coalesce from guardian.core import ObjectPermissionChecker from guardian.models import GroupObjectPermission from guardian.models import UserObjectPermission @@ -474,6 +475,29 @@ def permitted_document_ids( 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]: + """ + 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, + ), + ) + + def get_document_count_filter_for_user(user, related_name: str = "documents"): """ Return the Q object used to filter document counts for the given user. diff --git a/src/documents/tests/test_share_link_bundles.py b/src/documents/tests/test_share_link_bundles.py index 55cc61bde..3c1320c9f 100644 --- a/src/documents/tests/test_share_link_bundles.py +++ b/src/documents/tests/test_share_link_bundles.py @@ -90,10 +90,13 @@ class ShareLinkBundleAPITests(DirectoriesMixin, APITestCase): self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.assertIn("document_ids", response.data) - @mock.patch("documents.views.permitted_document_ids", return_value=set()) - def test_create_bundle_rejects_insufficient_permissions(self, perms_mock) -> None: + def test_create_bundle_rejects_insufficient_permissions(self) -> None: + requester = UserFactory(username="bundle_creator") + grant_global(requester, "add_sharelinkbundle", "view_document") + self.client.force_authenticate(requester) + document = DocumentFactory(owner=UserFactory(username="document_owner")) payload = { - "document_ids": [self.document.pk], + "document_ids": [self.document.pk, document.pk], "file_version": ShareLink.FileVersion.ARCHIVE, "expiration_days": 7, } @@ -101,8 +104,8 @@ class ShareLinkBundleAPITests(DirectoriesMixin, APITestCase): response = self.client.post(self.ENDPOINT, payload, format="json") self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn("document_ids", response.data) - perms_mock.assert_called() + self.assertIn(str(document.pk), str(response.data["document_ids"])) + self.assertFalse(ShareLinkBundle.objects.exists()) @mock.patch("documents.views.build_share_link_bundle.apply_async") def test_rebuild_bundle_resets_state(self, delay_mock) -> None: diff --git a/src/documents/views.py b/src/documents/views.py index 87fe2ced6..2e5960d65 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -174,6 +174,7 @@ 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 @@ -2113,13 +2114,12 @@ class DocumentViewSet( message = validated_data.get("message") use_archive_version = validated_data.get("use_archive_version", True) - documents = Document.objects.filter(pk__in=document_ids).select_related( - "root_document__owner", - ) - if request.user is not None: - permitted_ids = set(permitted_document_ids(request.user)) - if any(get_root_document(doc).pk not in permitted_ids for doc in documents): - return HttpResponseForbidden("Insufficient permissions") + documents = Document.objects.filter(pk__in=document_ids) + if ( + request.user is not None + and documents_without_permitted_root(documents, request.user).exists() + ): + return HttpResponseForbidden("Insufficient permissions") try: attachments: list[EmailAttachment] = [] @@ -3624,13 +3624,11 @@ class SelectionDataView(DocumentSelectionMixin, GenericAPIView[Any]): user=request.user, validated_data=serializer.validated_data, ) - # Versions are authorized by their root document - permitted_documents = Document.objects.annotate( - root_id=Coalesce("root_document_id", "id"), - ).filter( - root_id__in=permitted_document_ids(request.user), - ) - if permitted_documents.filter(pk__in=ids).count() != len(ids): + documents = Document.objects.filter(pk__in=ids) + if ( + documents.count() != len(ids) + or documents_without_permitted_root(documents, request.user).exists() + ): return HttpResponseForbidden("Insufficient permissions") correspondents = Correspondent.objects.annotate( @@ -4799,7 +4797,6 @@ class ShareLinkBundleViewSet(PassUserMixin, ModelViewSet[ShareLinkBundle]): document_ids = serializer.validated_data["document_ids"] documents_qs = Document.objects.filter(pk__in=document_ids).select_related( "owner", - "root_document__owner", ) found_ids = set(documents_qs.values_list("pk", flat=True)) missing = sorted(set(document_ids) - found_ids) @@ -4813,19 +4810,23 @@ class ShareLinkBundleViewSet(PassUserMixin, ModelViewSet[ShareLinkBundle]): }, ) - documents = list(documents_qs) - permitted_ids = set(permitted_document_ids(request.user)) - for document in documents: - if get_root_document(document).pk not in permitted_ids: - raise ValidationError( - { - "document_ids": _( - "Insufficient permissions to share document %(id)s.", - ) - % {"id": document.pk}, - }, - ) + denied_id = ( + documents_without_permitted_root(documents_qs, request.user) + .order_by("pk") + .values_list("pk", flat=True) + .first() + ) + if denied_id is not None: + raise ValidationError( + { + "document_ids": _( + "Insufficient permissions to share document %(id)s.", + ) + % {"id": denied_id}, + }, + ) + documents = list(documents_qs) document_map = {document.pk: document for document in documents} ordered_documents = [document_map[doc_id] for doc_id in document_ids] @@ -5656,18 +5657,12 @@ class TrashView(ListModelMixin, PassUserMixin): if doc_ids is not None else self.filter_queryset(self.get_queryset()).all() ) - # Versions are authorized by their root document - if ( - docs.annotate(root_id=Coalesce("root_document_id", "id")) - .exclude( - root_id__in=permitted_document_ids( - request.user, - perm="delete_document", - include_deleted=True, - ), - ) - .exists() - ): + if documents_without_permitted_root( + docs, + request.user, + perm="delete_document", + include_deleted=True, + ).exists(): return HttpResponseForbidden("Insufficient permissions") action = serializer.validated_data.get("action") if action == "restore":