diff --git a/src/documents/permissions.py b/src/documents/permissions.py index a97df8ec4..2cc3a744c 100644 --- a/src/documents/permissions.py +++ b/src/documents/permissions.py @@ -163,30 +163,40 @@ def set_permissions_for_object( ) -def permitted_document_ids( +def permitted_object_ids( user, + model, + perm: str, *, - perm: str = "view_document", include_deleted: bool = False, ): """ - 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. + Generic version of ``permitted_document_ids`` for any model with an + ``owner`` field and guardian object-level permissions. ``include_deleted`` + only has an effect for models exposing a ``global_objects``/``deleted_at`` + 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. """ - - manager = Document.global_objects if include_deleted else Document.objects - base_docs = manager.all() - base_docs = base_docs.only("id", "owner") + has_soft_delete = hasattr(model, "global_objects") + manager = ( + model.global_objects if include_deleted and has_soft_delete else model.objects + ) + if has_soft_delete: + base_qs = ( + manager.all() + if include_deleted + else manager.filter(deleted_at__isnull=True) + ) + else: + base_qs = manager.all() + base_qs = base_qs.only("id", "owner") if user is None or not getattr(user, "is_authenticated", False): - # Just Anonymous user e.g. for drf-spectacular - return base_docs.filter(owner__isnull=True).values_list("id", flat=True) + return base_qs.filter(owner__isnull=True).values_list("id", flat=True) if getattr(user, "is_superuser", False): - return base_docs.values_list("id", flat=True) + return base_qs.values_list("id", flat=True) # Guardian's UserObjectPermission/GroupObjectPermission always store a bare # codename, but has_perm()-style callers commonly pass the qualified @@ -194,31 +204,46 @@ def permitted_document_ids( # codename, so just drop any prefix rather than silently under-permitting. perm = perm.rsplit(".", 1)[-1] - document_ct = ContentType.objects.get_for_model(Document) + content_type = ContentType.objects.get_for_model(model) perm_filter = { "permission__codename": perm, - "permission__content_type": document_ct, + "permission__content_type": content_type, } - user_perm_docs = ( + 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_docs = ( + 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) - permitted_documents = user_perm_docs.union(group_perm_docs) - - return base_docs.filter( - Q(owner=user) | Q(owner__isnull=True) | Q(id__in=permitted_documents), + return base_qs.filter( + Q(owner=user) | Q(owner__isnull=True) | Q(id__in=permitted_ids), ).values_list("id", flat=True) +def permitted_document_ids( + user, + *, + perm: str = "view_document", + include_deleted: bool = False, +): + """ + Document-specific convenience wrapper around ``permitted_object_ids``. + 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. + """ + return permitted_object_ids(user, Document, 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_permission_filtering_security.py b/src/documents/tests/test_permission_filtering_security.py index f7a55f2f2..aeb8be168 100644 --- a/src/documents/tests/test_permission_filtering_security.py +++ b/src/documents/tests/test_permission_filtering_security.py @@ -12,9 +12,18 @@ from django.test import override_settings from guardian.shortcuts import assign_perm from rest_framework.test import APIClient +from documents.models import Correspondent +from documents.models import DocumentType +from documents.models import StoragePath +from documents.models import Tag from documents.permissions import permitted_document_ids +from documents.permissions import permitted_object_ids from documents.serialisers import _get_viewable_duplicates +from documents.tests.factories import CorrespondentFactory from documents.tests.factories import DocumentFactory +from documents.tests.factories import DocumentTypeFactory +from documents.tests.factories import StoragePathFactory +from documents.tests.factories import TagFactory def assert_visible_document_ids(actual_ids, *, expected_visible, expected_hidden): @@ -431,3 +440,92 @@ class TestTrashRestorePermissionBoundary: format="json", ) assert response.status_code == HTTPStatus.OK + + +@pytest.mark.django_db +@pytest.mark.parametrize( + ("model", "factory", "perm"), + [ + (Tag, TagFactory, "view_tag"), + (Correspondent, CorrespondentFactory, "view_correspondent"), + (DocumentType, DocumentTypeFactory, "view_documenttype"), + (StoragePath, StoragePathFactory, "view_storagepath"), + ], +) +class TestPermittedObjectIdsGenericModels: + def test_owner_sees_own_object(self, model, factory, perm): + owner = User.objects.create_user(username=f"owner_{model.__name__}") + stranger = User.objects.create_user(username=f"stranger_{model.__name__}") + owned = factory(owner=owner) + strangers = factory(owner=stranger) + + assert_visible_document_ids( + permitted_object_ids(owner, model, perm), + expected_visible=[owned.pk], + expected_hidden=[strangers.pk], + ) + + def test_unowned_object_visible_to_everyone(self, model, factory, perm): + user = User.objects.create_user(username=f"user_{model.__name__}") + unowned = factory(owner=None) + + assert_visible_document_ids( + permitted_object_ids(user, model, perm), + expected_visible=[unowned.pk], + expected_hidden=[], + ) + + def test_explicit_permission_grants_visibility(self, model, factory, perm): + owner = User.objects.create_user(username=f"owner2_{model.__name__}") + grantee = User.objects.create_user(username=f"grantee_{model.__name__}") + stranger = User.objects.create_user(username=f"stranger2_{model.__name__}") + shared = factory(owner=owner) + not_shared = factory(owner=owner) + assign_perm(perm, grantee, shared) + + assert_visible_document_ids( + permitted_object_ids(grantee, model, perm), + expected_visible=[shared.pk], + expected_hidden=[not_shared.pk], + ) + assert_visible_document_ids( + permitted_object_ids(stranger, model, perm), + expected_visible=[], + expected_hidden=[shared.pk, not_shared.pk], + ) + + def test_group_permission_grants_visibility_to_members_only( + self, + model, + factory, + perm, + ): + owner = User.objects.create_user(username=f"owner3_{model.__name__}") + member = User.objects.create_user(username=f"member_{model.__name__}") + non_member = User.objects.create_user(username=f"nonmember_{model.__name__}") + group = Group.objects.create(name=f"group_{model.__name__}") + member.groups.add(group) + shared = factory(owner=owner) + assign_perm(perm, group, shared) + + assert_visible_document_ids( + permitted_object_ids(member, model, perm), + expected_visible=[shared.pk], + expected_hidden=[], + ) + assert_visible_document_ids( + permitted_object_ids(non_member, model, perm), + expected_visible=[], + expected_hidden=[shared.pk], + ) + + def test_superuser_sees_everything(self, model, factory, perm): + superuser = User.objects.create_superuser(username=f"root_{model.__name__}") + owner = User.objects.create_user(username=f"owner4_{model.__name__}") + obj = factory(owner=owner) + + assert_visible_document_ids( + permitted_object_ids(superuser, model, perm), + expected_visible=[obj.pk], + expected_hidden=[], + )