From 1f1725ba567cd6c63a93571591eab2041904e083 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Tue, 4 Aug 2026 08:01:24 -0700 Subject: [PATCH] feat: add perm param to permitted_document_ids for change/delete checks (#13508) * feat: add perm param to permitted_document_ids for change/delete checks Widens permitted_document_ids(user, *, include_deleted=False) to permitted_document_ids(user, *, perm="view_document", include_deleted=False) so Stage 2 callers can check change_document/delete_document permissions instead of the hardcoded view_document codename. Default is unchanged for every existing call site. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GRp4kf1mdn9ruv81zWAmh2 * test: strengthen delete-permission include_deleted test to discriminate from other grants test_delete_permission_with_include_deleted_for_trash_restore only checked an owner and a fully-ungranted stranger, so it never proved perm= actually discriminates delete_document from other permission grants. Add a view_only user with view_document (but not delete_document) granted on the same doc and assert they remain excluded, mirroring the pattern in test_change_document_permission_is_distinct_from_view. * fix: normalize qualified permission strings in permitted_document_ids Guardian's UserObjectPermission/GroupObjectPermission always store a bare codename, but has_perm()-style callers commonly pass the qualified "app_label.codename" form. Passing that qualified form here previously matched zero rows, silently under-permitting. content_type already disambiguates the codename, so just strip any prefix instead. Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: Claude Sonnet 5 --- src/documents/permissions.py | 25 +++++-- .../test_permission_filtering_security.py | 67 +++++++++++++++++++ 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/documents/permissions.py b/src/documents/permissions.py index 5936f405a..a97df8ec4 100644 --- a/src/documents/permissions.py +++ b/src/documents/permissions.py @@ -163,13 +163,18 @@ def set_permissions_for_object( ) -def permitted_document_ids(user, *, include_deleted: bool = False): +def permitted_document_ids( + user, + *, + perm: str = "view_document", + include_deleted: bool = False, +): """ - Return a queryset of document IDs the user may view. By default limited - to non-deleted documents; pass ``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 a queryset of document IDs the user has ``perm`` on (default + ``"view_document"``). By default limited to non-deleted documents; pass + ``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. """ manager = Document.global_objects if include_deleted else Document.objects @@ -183,9 +188,15 @@ def permitted_document_ids(user, *, include_deleted: bool = False): if getattr(user, "is_superuser", False): return base_docs.values_list("id", flat=True) + # Guardian's UserObjectPermission/GroupObjectPermission always store a bare + # codename, but has_perm()-style callers commonly pass the qualified + # "app_label.codename" form. content_type already disambiguates the + # codename, so just drop any prefix rather than silently under-permitting. + perm = perm.rsplit(".", 1)[-1] + document_ct = ContentType.objects.get_for_model(Document) perm_filter = { - "permission__codename": "view_document", + "permission__codename": perm, "permission__content_type": document_ct, } diff --git a/src/documents/tests/test_permission_filtering_security.py b/src/documents/tests/test_permission_filtering_security.py index ba5522cb4..4ec6cf13c 100644 --- a/src/documents/tests/test_permission_filtering_security.py +++ b/src/documents/tests/test_permission_filtering_security.py @@ -217,3 +217,70 @@ class TestDuplicateDocumentsPermissionBoundary: result_stranger = _get_viewable_duplicates(original, stranger) assert {d.pk for d in result_stranger} == {dup_visible.pk} + + +@pytest.mark.django_db +class TestPermittedDocumentIdsArbitraryPermission: + def test_change_document_permission_is_distinct_from_view(self): + owner = User.objects.create_user(username="owner") + viewer_only = User.objects.create_user(username="viewer") + editor = User.objects.create_user(username="editor") + doc = DocumentFactory(owner=owner) + assign_perm("view_document", viewer_only, doc) + assign_perm("change_document", editor, doc) + assign_perm("view_document", editor, doc) + + assert_visible_document_ids( + permitted_document_ids(editor, perm="change_document"), + expected_visible=[doc.pk], + expected_hidden=[], + ) + assert_visible_document_ids( + permitted_document_ids(viewer_only, perm="change_document"), + expected_visible=[], + expected_hidden=[doc.pk], + ) + + def test_qualified_permission_string_is_normalized_to_codename(self): + owner = User.objects.create_user(username="owner") + editor = User.objects.create_user(username="editor") + doc = DocumentFactory(owner=owner) + assign_perm("change_document", editor, doc) + + assert_visible_document_ids( + permitted_document_ids(editor, perm="documents.change_document"), + expected_visible=[doc.pk], + expected_hidden=[], + ) + + def test_delete_permission_with_include_deleted_for_trash_restore(self): + owner = User.objects.create_user(username="owner") + stranger = User.objects.create_user(username="mallory") + view_only = User.objects.create_user(username="viewer") + doc = DocumentFactory(owner=owner) + assign_perm("view_document", view_only, doc) + doc.delete() + + assert_visible_document_ids( + permitted_document_ids(owner, perm="delete_document", include_deleted=True), + expected_visible=[doc.pk], + expected_hidden=[], + ) + assert_visible_document_ids( + permitted_document_ids( + stranger, + perm="delete_document", + include_deleted=True, + ), + expected_visible=[], + expected_hidden=[doc.pk], + ) + assert_visible_document_ids( + permitted_document_ids( + view_only, + perm="delete_document", + include_deleted=True, + ), + expected_visible=[], + expected_hidden=[doc.pk], + )