diff --git a/src/documents/bulk_edit.py b/src/documents/bulk_edit.py index 363193d48..a56eb014a 100644 --- a/src/documents/bulk_edit.py +++ b/src/documents/bulk_edit.py @@ -28,7 +28,7 @@ from documents.models import DocumentType from documents.models import PaperlessTask from documents.models import StoragePath from documents.models import Tag -from documents.permissions import set_permissions_for_object +from documents.permissions import set_permissions_for_objects from documents.plugins.helpers import DocumentsStatusManager from documents.tasks import bulk_update_documents from documents.tasks import consume_file @@ -433,10 +433,13 @@ def set_permissions( else: qs.update(owner=owner) - for doc in qs: - set_permissions_for_object(permissions=set_permissions, object=doc, merge=merge) - affected_docs = list(qs.values_list("pk", flat=True)) + set_permissions_for_objects( + permissions=set_permissions, + model=Document, + pks=affected_docs, + merge=merge, + ) bulk_update_documents.apply_async( kwargs={"document_ids": affected_docs}, diff --git a/src/documents/permissions.py b/src/documents/permissions.py index d22882753..dfa530516 100644 --- a/src/documents/permissions.py +++ b/src/documents/permissions.py @@ -173,6 +173,179 @@ def set_permissions_for_object( ) +def _resolve_permissions(codenames: set[str], ctype: ContentType) -> list[Permission]: + """ + Resolves `codenames` to Permission rows, raising like the single-object + assign_perm() this bulk path replaces does (via a `.get()` internally) + if any codename doesn't exist -- e.g. a client-supplied action name that + was never validated (BulkEditObjectsSerializer._validate_permissions + calls validate_set_permissions() only for its side-effecting id checks + and discards the filtered dict it returns, so an unrecognized action key + reaches this function as-is). A plain `.filter()` with no existence + check would otherwise silently build zero rows and no-op instead of + reporting the bad input. + """ + permission_objs = list( + Permission.objects.filter(content_type=ctype, codename__in=codenames), + ) + missing = codenames - {p.codename for p in permission_objs} + if missing: + raise Permission.DoesNotExist( + f"Permission matching query does not exist for codename(s): " + f"{', '.join(sorted(missing))}", + ) + return permission_objs + + +def _apply_bulk_permission_entry( + *, + perm_model: type[UserObjectPermission] | type[GroupObjectPermission], + identity_model: type[User] | type[Group], + identity_field: str, + ids: list[int], + codename: str, + permission_objs: list[Permission], + ctype: ContentType, + object_pks: list[str], + merge: bool, +) -> None: + # Only the ids are needed to build permission rows (via `_id=`), + # so avoid fetching full User/Group rows for identities that may not + # even end up being granted anything new. + add_ids = set( + identity_model.objects.filter(id__in=ids).values_list("id", flat=True), + ) + + if not merge: + existing_ids = set( + perm_model.objects.filter( + content_type=ctype, + object_pk__in=object_pks, + permission__codename=codename, + ) + .values_list(f"{identity_field}_id", flat=True) + .distinct(), + ) + remove_ids = existing_ids - add_ids + if remove_ids: + perm_model.objects.filter( + content_type=ctype, + object_pk__in=object_pks, + permission__codename=codename, + **{f"{identity_field}_id__in": remove_ids}, + ).delete() + + if not add_ids: + return + + rows = [ + perm_model( + content_type=ctype, + object_pk=pk, + permission=permission_obj, + **{f"{identity_field}_id": identity_id}, + ) + for permission_obj in permission_objs + for pk in object_pks + for identity_id in add_ids + ] + # ignore_conflicts skips only rows that already exist as an exact + # (identity, permission, object) match -- the same de-dup the + # underlying (user|group, permission, object_pk) unique constraint + # already enforces for the single-object assign_perm() this replaces, + # so it doesn't change what counts as "already granted". batch_size + # caps how many rows go into a single INSERT statement. + perm_model.objects.bulk_create(rows, ignore_conflicts=True, batch_size=1000) + + +def set_permissions_for_objects( + permissions: dict, + model: type[Model], + pks: QuerySet | list, + *, + merge: bool = False, +) -> None: + """ + Bulk equivalent of set_permissions_for_object: applies the same + permission changes to every object identified by `pks` at once. + + Takes a model + pks (rather than model instances) deliberately -- the + permission rows built below only ever need `pk`, `content_type`, and + identity ids, so callers shouldn't have to fetch full rows (with every + other field) just to hand them to this function. + + Deliberately does not use guardian's queryset/list-aware assign_perm: + passing a list as the object routes to bulk_assign_perm, which skips + creating a direct permission row for anyone who already has the + permission via ANY group membership (it checks + ObjectPermissionChecker.has_perm, which is group-inheritance-aware) -- + unlike the single-object assign_perm this replaces, which always + ensures a direct row via get_or_create regardless of group-derived + access. Losing that guarantee would mean a later revocation of the + group's grant silently strips access an admin explicitly asked to be + direct. Bulk-creating rows straight against the permission models + instead (see _apply_bulk_permission_entry) preserves the original + always-create-a-direct-row semantics while still batching every object + and every identity into one query per action, rather than one query per + (object, user) pair. + """ + object_pks = [str(pk) for pk in pks] + if not object_pks: # pragma: no cover + return + + model_name = model.__name__.lower() + ctype = ContentType.objects.get_for_model(model) + + # Every action is resolved up front, before anything is written, so an + # unrecognized action name (see _resolve_permissions) aborts the whole + # call instead of leaving the actions ahead of it already applied -- + # BulkEditObjectsSerializer lets unknown keys through and its view turns + # the exception into a 400, so a half-applied change would otherwise be + # reported to the client as a failure. + permissions_by_action: dict[str, list[Permission]] = {} + for action, entry in permissions.items(): + if "users" not in entry and "groups" not in entry: + continue + implied_codenames = {f"{action}_{model_name}"} + if action == "change": + # change gives view too + implied_codenames.add(f"view_{model_name}") + permissions_by_action[action] = _resolve_permissions( + implied_codenames, + ctype, + ) + + for action, entry in permissions.items(): + codename = f"{action}_{model_name}" + permission_objs = permissions_by_action.get(action, []) + + if "users" in entry: + _apply_bulk_permission_entry( + perm_model=UserObjectPermission, + identity_model=User, + identity_field="user", + ids=entry["users"], + codename=codename, + permission_objs=permission_objs, + ctype=ctype, + object_pks=object_pks, + merge=merge, + ) + + if "groups" in entry: + _apply_bulk_permission_entry( + perm_model=GroupObjectPermission, + identity_model=Group, + identity_field="group", + ids=entry["groups"], + codename=codename, + permission_objs=permission_objs, + ctype=ctype, + object_pks=object_pks, + merge=merge, + ) + + def permitted_object_ids( user: User | None, model: type[Model], diff --git a/src/documents/tests/test_api_objects.py b/src/documents/tests/test_api_objects.py index 05488ba47..88184cb84 100644 --- a/src/documents/tests/test_api_objects.py +++ b/src/documents/tests/test_api_objects.py @@ -2,10 +2,15 @@ import datetime import json from unittest import mock +from django.contrib.auth.models import Group from django.contrib.auth.models import Permission from django.contrib.auth.models import User +from django.db import connection from django.test import override_settings +from django.test.utils import CaptureQueriesContext from guardian.shortcuts import assign_perm +from guardian.shortcuts import get_groups_with_perms +from guardian.shortcuts import get_users_with_perms from rest_framework import status from rest_framework.test import APITestCase @@ -842,6 +847,66 @@ class TestBulkEditObjects(APITestCase): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(StoragePath.objects.count(), 0) + def test_bulk_objects_set_permissions_batched_across_object_count( + self, + ) -> None: + """ + GIVEN: + - Many tags are being bulk-edited to set permissions at once + WHEN: + - bulk_edit_objects API endpoint is called with set_permissions + operation over a small batch vs. a much larger one + THEN: + - Permissions are applied correctly at both scales + - Query count does not grow with the number of tags, i.e. each + user/group is applied across all tags with one batched call + rather than one call per (tag, identity) pair + """ + group1 = Group.objects.create(name="perm-group") + permissions = { + "view": {"users": [self.user1.id, self.user2.id], "groups": [group1.id]}, + "change": {"users": [self.user1.id], "groups": [group1.id]}, + } + + def run_with_n_tags(n: int) -> int: + tags = [Tag.objects.create(name=f"perm-tag-{n}-{i}") for i in range(n)] + with CaptureQueriesContext(connection) as ctx: + response = self.client.post( + "/api/bulk_edit_objects/", + json.dumps( + { + "objects": [t.id for t in tags], + "object_type": "tags", + "operation": "set_permissions", + "permissions": permissions, + "merge": False, + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + for tag in tags: + self.assertEqual(get_users_with_perms(tag).count(), 2) + self.assertEqual(get_groups_with_perms(tag).count(), 1) + return len(ctx.captured_queries) + + small_batch_queries = run_with_n_tags(5) + large_batch_queries = run_with_n_tags(50) + + # A tolerance rather than equality, matching the N+1 check in + # test_views.py: bulk_create's batch_size caps rows per INSERT, so a + # large enough selection does legitimately add statements, and the + # per-process ContentType cache makes the first run carry an extra + # query. Neither can hide a regression to per-object assignment, + # which would be ~10x the small-batch count here. + self.assertLessEqual( + large_batch_queries, + small_batch_queries + 5, + "Permission assignment appears to scale with object count: " + f"{small_batch_queries} queries for 5 tags vs. " + f"{large_batch_queries} for 50", + ) + def test_bulk_objects_delete_all_filtered(self) -> None: """ GIVEN: diff --git a/src/documents/tests/test_bulk_edit.py b/src/documents/tests/test_bulk_edit.py index 1315dd84b..6df2a9c53 100644 --- a/src/documents/tests/test_bulk_edit.py +++ b/src/documents/tests/test_bulk_edit.py @@ -5,8 +5,11 @@ from unittest import mock import pikepdf from django.contrib.auth.models import Group +from django.contrib.auth.models import Permission from django.contrib.auth.models import User +from django.db import connection from django.test import TestCase +from django.test.utils import CaptureQueriesContext from guardian.shortcuts import assign_perm from guardian.shortcuts import get_groups_with_perms from guardian.shortcuts import get_users_with_perms @@ -19,6 +22,7 @@ from documents.models import Document from documents.models import DocumentType from documents.models import StoragePath from documents.models import Tag +from documents.permissions import set_permissions_for_objects from documents.tests.utils import DirectoriesMixin @@ -515,6 +519,178 @@ class TestBulkEdit(DirectoriesMixin, TestCase): ) self.assertEqual(groups_with_perms.count(), 2) + @mock.patch("documents.tasks.bulk_update_documents.apply_async") + def test_set_permissions_batched_across_document_count( + self, + m, + ) -> None: + """ + GIVEN: + - Many documents are being bulk-edited to set permissions at once + WHEN: + - set_permissions runs over a small batch vs. a much larger one + THEN: + - Permissions are applied correctly at both scales + - Query count does not grow with the number of documents, i.e. + each user/group is applied across all documents with one + batched call rather than one call per (document, identity) + pair + """ + permissions = { + "view": { + "users": [self.user1.id, self.user2.id], + "groups": [self.group2.id], + }, + "change": { + "users": [self.user1.id], + "groups": [self.group2.id], + }, + } + + def run_with_n_documents(n: int) -> int: + docs = [ + Document.objects.create(checksum=f"perm-{n}-{i}", title=f"perm-{n}-{i}") + for i in range(n) + ] + with CaptureQueriesContext(connection) as ctx: + bulk_edit.set_permissions( + [doc.id for doc in docs], + set_permissions=permissions, + owner=self.owner, + merge=False, + ) + for doc in docs: + self.assertEqual(get_users_with_perms(doc).count(), 2) + self.assertEqual(get_groups_with_perms(doc).count(), 1) + return len(ctx.captured_queries) + + small_batch_queries = run_with_n_documents(5) + large_batch_queries = run_with_n_documents(50) + + # A tolerance rather than equality, matching the N+1 check in + # test_views.py: bulk_create's batch_size caps rows per INSERT, so a + # large enough selection does legitimately add statements, and the + # per-process ContentType cache makes the first run carry an extra + # query. Neither can hide a regression to per-document assignment, + # which would be ~10x the small-batch count here. + self.assertLessEqual( + large_batch_queries, + small_batch_queries + 5, + "Permission assignment appears to scale with document count: " + f"{small_batch_queries} queries for 5 documents vs. " + f"{large_batch_queries} for 50", + ) + + @mock.patch("documents.tasks.bulk_update_documents.apply_async") + def test_set_permissions_grants_direct_perm_even_if_already_granted_via_group( + self, + m, + ) -> None: + """ + GIVEN: + - A user already has view access to a document via group + membership, with no direct grant of their own + WHEN: + - set_permissions explicitly grants that same user direct view + access via bulk_edit + THEN: + - A direct permission grant is created for the user, not skipped + because they already have equivalent access via the group + + Regression test: guardian's queryset-aware assign_perm() (routed to + when the target is a list/queryset) skips creating a direct row for + anyone whose ObjectPermissionChecker.has_perm() already returns True + -- which includes group-derived access. The single-object assign_perm + this bulk path replaces has no such check; it always ensures a + direct row via get_or_create. Losing that guarantee would mean + revoking the group's grant later silently strips access that was + supposed to be explicit. + """ + self.doc1.owner = self.user1 + self.doc1.save() + self.user1.groups.add(self.group1) + assign_perm("view_document", self.group1, self.doc1) + + bulk_edit.set_permissions( + [self.doc1.id], + set_permissions={ + "view": {"users": [self.user1.id], "groups": []}, + }, + merge=True, + ) + + direct_users = get_users_with_perms( + self.doc1, + only_with_perms_in=["view_document"], + with_group_users=False, + ) + self.assertIn(self.user1, direct_users) + + def test_set_permissions_for_objects_raises_for_unknown_action(self) -> None: + """ + GIVEN: + - An unrecognized permission action name with users to grant it + to + WHEN: + - set_permissions_for_objects is called + THEN: + - Permission.DoesNotExist is raised, not a silent no-op + + Regression test: the endpoint that calls this + (BulkEditObjectPermissionsView) never actually validates action + names against the raw client-supplied permissions dict -- + BulkEditObjectsSerializer._validate_permissions calls + validate_set_permissions() only for its side-effecting user/group id + checks and discards the filtered dict it returns -- so a bogus + action key reaches this function as-is. Resolving the Permission via + a bare `.filter()` (which returns empty instead of raising) would + silently drop the grant and report success. + """ + with self.assertRaises(Permission.DoesNotExist): + set_permissions_for_objects( + {"not_a_real_action": {"users": [self.user1.id], "groups": []}}, + Document, + [self.doc1.pk], + ) + + def test_set_permissions_for_objects_unknown_action_applies_nothing( + self, + ) -> None: + """ + GIVEN: + - A permissions dict with a valid action ordered ahead of an + unrecognized one + WHEN: + - set_permissions_for_objects is called + THEN: + - Permission.DoesNotExist is raised + - The valid action ahead of it is not applied either + + Every action is resolved before any row is written, so a bad action + name cannot leave a half-applied change behind. That matters because + BulkEditObjectsView turns this exception into a 400: without the + up-front resolution the client would be told the request failed + while the leading action had already been committed. + """ + with self.assertRaises(Permission.DoesNotExist): + set_permissions_for_objects( + { + "view": {"users": [self.user1.id], "groups": []}, + "not_a_real_action": {"users": [self.user1.id], "groups": []}, + }, + Document, + [self.doc1.pk], + ) + + self.assertNotIn( + self.user1, + get_users_with_perms( + self.doc1, + only_with_perms_in=["view_document"], + with_group_users=False, + ), + ) + @mock.patch("documents.models.Document.delete") def test_delete_documents_old_uuid_field(self, m) -> None: m.side_effect = Exception("Data too long for column 'transaction_id' at row 1") diff --git a/src/documents/views.py b/src/documents/views.py index 04bd68d2e..f18d17c1e 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -180,7 +180,7 @@ from documents.permissions import has_perms_owner_aware from documents.permissions import has_system_status_permission from documents.permissions import permitted_document_ids from documents.permissions import permitted_object_ids -from documents.permissions import set_permissions_for_object +from documents.permissions import set_permissions_for_objects from documents.permissions import user_is_unrestricted from documents.plugins.date_parsing import get_date_parser from documents.schema import generate_object_with_permissions_schema @@ -5019,12 +5019,12 @@ class BulkEditObjectsView(PassUserMixin): qs_owner_update.update(owner=owner) if "permissions" in serializer.validated_data: - for obj in qs: - set_permissions_for_object( - permissions=permissions, - object=obj, - merge=merge, - ) + set_permissions_for_objects( + permissions=permissions, + model=object_class, + pks=qs.values_list("pk", flat=True), + merge=merge, + ) except Exception as e: logger.warning(