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/serialisers.py b/src/documents/serialisers.py index c2c875029..ec0cf2812 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -90,6 +90,7 @@ from documents.templating.utils import convert_format_str_to_template_format from documents.templating.workflows import validate_workflow_template from documents.validators import uri_validator from documents.validators import url_validator +from documents.versioning import get_root_document from documents.versioning import has_prefetched_effective_content from documents.versioning import sort_versions_newest_first @@ -2894,7 +2895,7 @@ class ShareLinkSerializer(OwnedObjectSerializer): and has_perms_owner_aware( self.user, "view_document", - document, + get_root_document(document), ) ): return document diff --git a/src/documents/tests/test_api_document_versions.py b/src/documents/tests/test_api_document_versions.py index 6fce8fa49..b1eb3c737 100644 --- a/src/documents/tests/test_api_document_versions.py +++ b/src/documents/tests/test_api_document_versions.py @@ -8,6 +8,7 @@ from auditlog.models import LogEntry # type: ignore[import-untyped] from django.contrib.contenttypes.models import ContentType from django.core.files.uploadedfile import SimpleUploadedFile from django.test import TestCase as DjangoTestCase +from django.test import override_settings from django.utils import timezone from rest_framework import status from rest_framework.test import APITestCase @@ -16,13 +17,17 @@ from documents.data_models import DocumentSource from documents.filters import EffectiveContentFilter from documents.filters import TitleContentFilter from documents.models import Document +from documents.models import Note +from documents.models import ShareLink from documents.versioning import annotate_effective_content from documents.views import DocumentSelectionMixin from paperless_testing.dirs import DirectoriesMixin from paperless_testing.factories import DocumentFactory from paperless_testing.factories import UserFactory from paperless_testing.http import read_streaming_response +from paperless_testing.permissions import grant_all_global from paperless_testing.permissions import grant_global +from paperless_testing.permissions import grant_object if TYPE_CHECKING: from pathlib import Path @@ -1043,3 +1048,152 @@ class TestBulkSelectionExcludesVersions(DjangoTestCase): ) self.assertEqual(selected, [root.id]) + + +class TestVersionActionPermissions(DirectoriesMixin, APITestCase): + def setUp(self): + super().setUp() + self.user = UserFactory() + grant_all_global(self.user) + self.client.force_authenticate(self.user) + self.root = DocumentFactory(owner=UserFactory()) + self.version = DocumentFactory(root_document=self.root, owner=None) + + @override_settings(AUDIT_LOG_ENABLED=True) + def test_actions_reject_stale_version_ownership(self): + note = Note.objects.create(document=self.version, note="Version note") + for owner in (None, self.user): + self.version.owner = owner + self.version.save(update_fields=["owner"]) + for action in ( + "notes", + "suggestions", + "ai_suggestions", + "history", + "share_links", + ): + with self.subTest(owner=owner, action=action): + response = self.client.get( + f"/api/documents/{self.version.pk}/{action}/", + ) + self.assertEqual(response.status_code, 403) + response = self.client.post( + f"/api/documents/{self.version.pk}/notes/", + {"note": "New note"}, + ) + self.assertEqual(response.status_code, 403) + response = self.client.delete( + f"/api/documents/{self.version.pk}/notes/?id={note.pk}", + ) + self.assertEqual(response.status_code, 403) + response = self.client.post( + "/api/share_links/", + {"document": self.version.pk, "file_version": "original"}, + ) + self.assertEqual(response.status_code, 403) + response = self.client.post( + "/api/share_link_bundles/", + {"document_ids": [self.version.pk], "file_version": "original"}, + format="json", + ) + self.assertEqual(response.status_code, 400) + response = self.client.post( + "/api/documents/email/", + { + "documents": [self.version.pk], + "addresses": "recipient@example.com", + "subject": "Version", + "message": "Version", + }, + format="json", + ) + self.assertEqual(response.status_code, 403) + with ( + mock.patch("documents.views.AIConfig") as ai_config, + mock.patch("documents.views.stream_chat_with_documents") as chat, + ): + ai_config.return_value.ai_enabled = True + response = self.client.post( + "/api/documents/chat/", + {"q": "Version?", "document_id": self.version.pk}, + format="json", + ) + self.assertEqual(response.status_code, 403) + chat.assert_not_called() + self.assertTrue(Note.objects.filter(pk=note.pk).exists()) + self.assertFalse(ShareLink.objects.exists()) + + @mock.patch("documents.views.build_share_link_bundle.apply_async") + def test_root_permissions_allow_sharing_a_private_version(self, build_mock): + self.version.owner = UserFactory() + self.version.save(update_fields=["owner"]) + grant_object(self.user, self.root, "view_document", "change_document") + note = Note.objects.create(document=self.version, note="Version note") + response = self.client.get(f"/api/documents/{self.version.pk}/notes/") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data[0]["id"], note.pk) + response = self.client.post( + "/api/share_links/", + {"document": self.version.pk, "file_version": "original"}, + ) + self.assertEqual(response.status_code, 201) + self.assertEqual(ShareLink.objects.get().document_id, self.version.pk) + response = self.client.get(f"/api/documents/{self.version.pk}/share_links/") + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data), 1) + response = self.client.post( + "/api/share_link_bundles/", + {"document_ids": [self.version.pk], "file_version": "original"}, + format="json", + ) + self.assertEqual(response.status_code, 201) + build_mock.assert_called_once() + + def test_root_view_permission_does_not_allow_note_changes(self): + grant_object(self.user, self.root, "view_document") + note = Note.objects.create(document=self.version, note="Version note") + response = self.client.get(f"/api/documents/{self.version.pk}/notes/") + self.assertEqual(response.status_code, 200) + response = self.client.post( + f"/api/documents/{self.version.pk}/notes/", + {"note": "New note"}, + ) + self.assertEqual(response.status_code, 403) + response = self.client.delete( + f"/api/documents/{self.version.pk}/notes/?id={note.pk}", + ) + self.assertEqual(response.status_code, 403) + self.assertTrue(Note.objects.filter(pk=note.pk).exists()) + + @override_settings(AUDIT_LOG_ENABLED=True) + def test_history_uses_root_ownership(self): + self.root.owner = self.user + self.root.save(update_fields=["owner"]) + self.version.owner = UserFactory() + self.version.save(update_fields=["owner"]) + response = self.client.get(f"/api/documents/{self.version.pk}/history/") + self.assertEqual(response.status_code, 200) + + def test_selection_data_rejects_stale_version_ownership(self): + for owner in (None, self.user): + self.version.owner = owner + self.version.save(update_fields=["owner"]) + with self.subTest(owner=owner): + response = self.client.post( + "/api/documents/selection_data/", + {"documents": [self.version.pk]}, + format="json", + ) + self.assertEqual(response.status_code, 403) + + def test_selection_data_allows_private_version_of_permitted_root(self): + self.version.owner = UserFactory() + self.version.save(update_fields=["owner"]) + grant_object(self.user, self.root, "view_document") + other = DocumentFactory(owner=self.user) + response = self.client.post( + "/api/documents/selection_data/", + {"documents": [self.version.pk, other.pk]}, + format="json", + ) + self.assertEqual(response.status_code, 200) diff --git a/src/documents/tests/test_api_trash.py b/src/documents/tests/test_api_trash.py index 792f9ca00..8dbdee2db 100644 --- a/src/documents/tests/test_api_trash.py +++ b/src/documents/tests/test_api_trash.py @@ -6,6 +6,7 @@ from rest_framework.test import APITestCase from documents.models import Document from paperless_testing.dirs import DirectoriesMixin +from paperless_testing.factories import DocumentFactory from paperless_testing.factories import UserFactory from paperless_testing.permissions import grant_all_global @@ -279,3 +280,53 @@ class TestTrashAPI(DirectoriesMixin, APITestCase): Document.objects.filter(root_document=root).values_list("id", flat=True), [version.pk for version in versions], ) + + def test_api_trash_version_follows_root_owner(self) -> None: + """ + GIVEN: + - A deleted version of user2's document, owned by nobody + - A deleted version of the user's document, owned by user2 + WHEN: + - The user lists the trash and tries to restore or empty the versions + THEN: + - Only the version of the user's own document is listed + - The other version can't be restored or emptied + - The version of the user's own document can be restored + """ + user2 = UserFactory(username="user2") + other_version = DocumentFactory( + root_document=DocumentFactory(owner=user2), + version_index=1, + ) + other_version.delete() + own_version = DocumentFactory( + owner=user2, + root_document=DocumentFactory(owner=self.user), + version_index=1, + ) + own_version.delete() + + resp = self.client.get("/api/trash/") + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual( + [doc["id"] for doc in resp.data["results"]], + [own_version.pk], + ) + + for action in ("restore", "empty"): + with self.subTest(action=action): + resp = self.client.post( + "/api/trash/", + {"action": action, "documents": [other_version.pk]}, + ) + self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + self.assertTrue( + Document.deleted_objects.filter(pk=other_version.pk).exists(), + ) + + resp = self.client.post( + "/api/trash/", + {"action": "restore", "documents": [own_version.pk]}, + ) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertTrue(Document.objects.filter(pk=own_version.pk).exists()) 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 515fd33d9..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 @@ -1550,13 +1551,16 @@ class DocumentViewSet( ) def suggestions(self, request, pk=None): doc = get_object_or_404( - Document.objects.select_related("owner").prefetch_related("versions"), + Document.objects.select_related( + "owner", + "root_document__owner", + ).prefetch_related("versions"), pk=pk, ) if request.user is not None and not has_perms_owner_aware( request.user, "change_document", - doc, + get_root_document(doc), ): return HttpResponseForbidden("Insufficient permissions") @@ -1610,13 +1614,16 @@ class DocumentViewSet( @method_decorator(cache_control(no_cache=True)) def ai_suggestions(self, request, pk=None): doc = get_object_or_404( - Document.objects.select_related("owner").prefetch_related("versions"), + Document.objects.select_related( + "owner", + "root_document__owner", + ).prefetch_related("versions"), pk=pk, ) if request.user is not None and not has_perms_owner_aware( request.user, "change_document", - doc, + get_root_document(doc), ): return HttpResponseForbidden("Insufficient permissions") @@ -1856,15 +1863,20 @@ class DocumentViewSet( currentUser = request.user try: doc = ( - Document.objects.select_related("owner") + Document.objects.select_related("owner", "root_document__owner") .prefetch_related("notes") - .only("pk", "owner__id") + .only( + "pk", + "owner__id", + "root_document__id", + "root_document__owner__id", + ) .get(pk=pk) ) if currentUser is not None and not has_perms_owner_aware( currentUser, "view_document", - doc, + get_root_document(doc), ): return HttpResponseForbidden("Insufficient permissions to view notes") except Document.DoesNotExist: @@ -1886,7 +1898,7 @@ class DocumentViewSet( if currentUser is not None and not has_perms_owner_aware( currentUser, "change_document", - doc, + get_root_document(doc), ): return HttpResponseForbidden( "Insufficient permissions to create notes", @@ -1929,7 +1941,7 @@ class DocumentViewSet( if currentUser is not None and not has_perms_owner_aware( currentUser, "change_document", - doc, + get_root_document(doc), ): return HttpResponseForbidden("Insufficient permissions to delete notes") @@ -1973,11 +1985,13 @@ class DocumentViewSet( def share_links(self, request, pk=None): currentUser = request.user try: - doc = Document.objects.select_related("owner").get(pk=pk) + doc = Document.objects.select_related("owner", "root_document__owner").get( + pk=pk, + ) if currentUser is not None and not has_perms_owner_aware( currentUser, "change_document", - doc, + get_root_document(doc), ): return HttpResponseForbidden( "Insufficient permissions to add share link", @@ -2008,10 +2022,11 @@ class DocumentViewSet( if not settings.AUDIT_LOG_ENABLED: return HttpResponseBadRequest("Audit log is disabled") try: - doc = Document.objects.get(pk=pk) + doc = Document.objects.select_related("root_document__owner").get(pk=pk) + root_doc = get_root_document(doc) if not request.user.has_perm("auditlog.view_logentry") or ( - doc.owner is not None - and doc.owner != request.user + root_doc.owner is not None + and root_doc.owner != request.user and not request.user.is_superuser ): return HttpResponseForbidden( @@ -2102,9 +2117,7 @@ class DocumentViewSet( documents = Document.objects.filter(pk__in=document_ids) if ( request.user is not None - and documents.exclude( - pk__in=permitted_document_ids(request.user), - ).exists() + and documents_without_permitted_root(documents, request.user).exists() ): return HttpResponseForbidden("Insufficient permissions") @@ -2430,11 +2443,17 @@ class ChatStreamingView(GenericAPIView[Any]): if doc_id: try: - document = Document.objects.get(id=doc_id) + document = Document.objects.select_related( + "root_document__owner", + ).get(id=doc_id) except Document.DoesNotExist: return HttpResponseBadRequest("Document not found") - if not has_perms_owner_aware(request.user, "view_document", document): + if not has_perms_owner_aware( + request.user, + "view_document", + get_root_document(document), + ): return HttpResponseForbidden("Insufficient permissions") documents = Document.objects.filter(pk=document.pk) @@ -3605,10 +3624,11 @@ class SelectionDataView(DocumentSelectionMixin, GenericAPIView[Any]): user=request.user, validated_data=serializer.validated_data, ) - permitted_documents = Document.objects.filter( - 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( @@ -4790,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 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] @@ -5587,6 +5611,23 @@ class TrashView(ListModelMixin, PassUserMixin): class _TrashPermittedObjectsFilter(PermittedObjectsFilter): include_granted = False + def filter_queryset(self, request, queryset, view): + if request.user.is_superuser or not request.user.is_active: + return super().filter_queryset(request, queryset, view) + + # A version belongs to whoever owns its root + def owned_or_unowned(prefix: str) -> Q: + return Q(**{f"{prefix}owner": request.user}) | Q( + **{f"{prefix}owner__isnull": True}, + ) + + return queryset.filter( + (Q(root_document__isnull=True) & owned_or_unowned("")) + | ( + Q(root_document__isnull=False) & owned_or_unowned("root_document__") + ), + ) + filter_backends = (_TrashPermittedObjectsFilter,) pagination_class = StandardPagination @@ -5616,12 +5657,11 @@ class TrashView(ListModelMixin, PassUserMixin): if doc_ids is not None else self.filter_queryset(self.get_queryset()).all() ) - if docs.exclude( - pk__in=permitted_document_ids( - request.user, - perm="delete_document", - include_deleted=True, - ), + 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")