Commit 7b12ed7a authored by Mickaël Desfrênes's avatar Mickaël Desfrênes
Browse files

Enforce resource access when listing collection annotations 

Reading a collection previously exposed every contained resource and its
annotations, including resources with explicit read denials.

Reuse one UserAccess instance and check each resource before serialization.
Fetch annotations and their owners together, restricted to readable resource
IDs. Batch membership and ancestor loading to preserve existing ACL inheritance
without a database walk per resource, and invalidate the ancestry cache when
roles change.

Add regression coverage for denied resources and files, inherited permissions,
deleted memberships, project boundaries, output ordering, and cache invalidation.
Verify that query counts remain unchanged between 1 and 21 resources at the
same collection depth, with both global and scoped permissions.

Validation: all 183 resources, rpc, and annotations tests passed; Ruff and
git diff --check passed.
parent b4c2211d

src/annotations/tests.py

+212 −0
Changes for src/annotations/tests.py: 212 added lines, 0 removed lines.
Original line number Diff line number Diff line 
from django.test import TestCase

from django.db import connection

from django.test.utils import CaptureQueriesContext

from django.utils import timezone

from resources.acl import UserAccess, set_permission_on_role

from resources import models

from rpc import methods as rpc_methods

from resources.management.commands.loadfixtures import load_fixtures

from django.contrib.auth.models import User

from annotations.models import Annotation

from rpc.methods import ServiceException

from unittest.mock import patch



test_json = {"pouet": "tagada"}
@@ -153,3 +158,210 @@ class AnnotationTestCase(TestCase): 


        self.assertTrue(serialized_annotation.get("public"))

        self.assertFalse(fetched_annotation.public)





class CollectionAnnotationsTestCase(TestCase):

    @classmethod

    def setUpTestData(cls):

        cls.user = User.objects.create(username="reader")

        cls.owner = User.objects.create(username="annotator")

        cls.project = models.Project.objects.create(label="annotations", description="")

        cls.root = models.Collection.objects.create(project=cls.project, title="Root")

        cls.collection = models.Collection.objects.create(

            project=cls.project, parent=cls.root, title="Requested collection"

        )

        cls.role = models.Role.objects.create(project=cls.project, label="reader")

        models.ProjectAccess.objects.create(

            project=cls.project, user=cls.user, role=cls.role

        )

        set_permission_on_role(cls.role, "collection", can_read=True)

        set_permission_on_role(cls.role, "resource", can_read=True)

        cls.file_type = models.FileType.objects.create(title="Text", mime="text/plain")



    def _resource(self, title: str, *, file_hash: str | None = None) -> models.Resource:

        if file_hash:

            resource = models.File.objects.create(

                project=self.project,

                title=title,

                original_name=f"{title}.txt",

                hash=file_hash,

                file_type=self.file_type,

                size=1,

                denormalized_image_width=1,

                denormalized_image_height=1,

            )

        else:

            resource = models.Resource.objects.create(

                ptr_project=self.project, title=title

            )

        self.collection.resources.add(resource)

        return resource



    def _annotation(

        self, resource: models.Resource, *, public: bool = False

    ) -> Annotation:

        return Annotation.objects.create(

            resource=resource,

            owner=self.owner,

            data={"title": resource.title},

            public=public,

        )



    def test_excludes_denied_resources_and_their_annotations(self):

        allowed = self._resource("allowed")

        allowed_annotation = self._annotation(allowed)

        for title, file_hash in (("denied resource", None), ("denied file", "a" * 64)):

            denied = self._resource(title, file_hash=file_hash)

            set_permission_on_role(self.role, denied, can_read=False)

            self._annotation(denied)

            self._annotation(denied, public=True)



        with patch.object(

            rpc_methods.serializers, "resource", wraps=rpc_methods.serializers.resource

        ) as serialize:

            results = rpc_methods.list_collection_annotations(

                self.user, self.collection.pk

            )



        self.assertEqual([result["id"] for result in results], [allowed.pk])

        self.assertEqual(results[0]["annotations"][0]["id"], allowed_annotation.pk)

        self.assertEqual(results[0]["annotations"][0]["owner"], self.owner.username)

        self.assertFalse(results[0]["annotations"][0]["public"])

        self.assertEqual(serialize.call_count, 1)



    def test_preserves_resource_and_annotation_order(self):

        first = self._resource("Z", file_hash="a" * 64)

        second = self._resource("A")

        models.CollectionMembership.objects.filter(resource=first).update(rank=1)

        models.CollectionMembership.objects.filter(resource=second).update(rank=2)

        first_annotation = self._annotation(first, public=True)

        second_annotation = self._annotation(first)



        results = rpc_methods.list_collection_annotations(self.user, self.collection.pk)



        self.assertEqual([result["id"] for result in results], [first.pk, second.pk])

        self.assertEqual(

            [annotation["id"] for annotation in results[0]["annotations"]],

            [first_annotation.pk, second_annotation.pk],

        )

        self.assertEqual(results[1]["annotations"], [])



    def test_respects_inherited_permissions_across_memberships(self):

        set_permission_on_role(self.role, "resource", can_read=False)

        allowed_parent = models.Collection.objects.create(

            project=self.project, parent=self.root, title="Allowed parent"

        )

        allowed_branch = models.Collection.objects.create(

            project=self.project, parent=allowed_parent, title="Z allowed branch"

        )

        set_permission_on_role(self.role, allowed_parent, can_read=True)

        denied_branch = models.Collection.objects.create(

            project=self.project, parent=self.root, title="A denied branch"

        )

        set_permission_on_role(self.role, denied_branch, can_read=False)

        inherited_allow = self._resource("inherited allow")

        allowed_branch.resources.add(inherited_allow)

        inherited_deny = self._resource("inherited deny")

        allowed_branch.resources.add(inherited_deny)

        denied_branch.resources.add(inherited_deny)

        explicit_allow = self._resource("explicit allow")

        denied_branch.resources.add(explicit_allow)

        set_permission_on_role(self.role, explicit_allow, can_read=True)

        for resource in (inherited_allow, inherited_deny, explicit_allow):

            self._annotation(resource)



        access = UserAccess(self.user, self.project)

        self.assertTrue(access.can_read(inherited_allow))

        self.assertFalse(access.can_read(inherited_deny))

        self.assertTrue(access.can_read(explicit_allow))

        results = rpc_methods.list_collection_annotations(self.user, self.collection.pk)



        self.assertEqual(

            {result["id"] for result in results},

            {inherited_allow.pk, explicit_allow.pk},

        )



    def test_deleted_memberships_do_not_grant_access(self):

        set_permission_on_role(self.role, "resource", can_read=False)

        deleted_collection = models.Collection.objects.create(

            project=self.project, title="Deleted", deleted_at=timezone.now()

        )

        set_permission_on_role(self.role, deleted_collection, can_read=True)

        resource = self._resource("hidden")

        deleted_collection.resources.add(resource)

        self._annotation(resource)



        self.assertEqual(

            rpc_methods.list_collection_annotations(self.user, self.collection.pk), []

        )



    def test_rejects_resources_from_another_project(self):

        other_project = models.Project.objects.create(label="other", description="")

        resource = models.Resource.objects.create(

            ptr_project=other_project, title="other"

        )

        self.collection.resources.add(resource)

        self._annotation(resource)



        self.assertEqual(

            rpc_methods.list_collection_annotations(self.user, self.collection.pk), []

        )



    def test_requires_collection_read_access(self):

        resource = self._resource("visible")

        self._annotation(resource)

        set_permission_on_role(self.role, self.collection, can_read=False)



        with self.assertRaises(ServiceException):

            rpc_methods.list_collection_annotations(self.user, self.collection.pk)



    def test_prefetched_access_is_invalidated_when_roles_change(self):

        resource = self._resource("visible")

        access = UserAccess(self.user, self.project)

        access.prefetch_resource_ancestors([resource])

        self.assertTrue(access.can_read(resource))

        restricted_role = models.Role.objects.create(

            project=self.project, label="restricted"

        )

        set_permission_on_role(restricted_role, self.root, can_read=False)



        access.add_role_to_user(restricted_role)

        self.assertFalse(access.can_read(resource))

        access.remove_role_from_user(restricted_role)

        self.assertTrue(access.can_read(resource))



    def test_queries_do_not_grow_with_resource_count(self):

        for scoped in (False, True):

            with self.subTest(scoped=scoped):

                self.collection.resources.clear()

                models.ObjectPermission.objects.filter(

                    role=self.role, object_pk__isnull=False

                ).delete()

                if scoped:

                    set_permission_on_role(self.role, self.root, can_read=True)

                resource = self._resource(

                    f"first {scoped}", file_hash=("b" if scoped else "a") * 64

                )

                self._annotation(resource)

                rpc_methods.list_collection_annotations(self.user, self.collection.pk)

                with CaptureQueriesContext(connection) as small_queries:

                    small_results = rpc_methods.list_collection_annotations(

                        self.user, self.collection.pk

                    )

                for index in range(20):

                    resource = self._resource(f"extra {scoped} {index}")

                    self._annotation(resource)

                with CaptureQueriesContext(connection) as large_queries:

                    large_results = rpc_methods.list_collection_annotations(

                        self.user, self.collection.pk

                    )



                self.assertEqual(len(small_results), 1)

                self.assertEqual(len(large_results), 21)

                self.assertEqual(len(small_queries), len(large_queries))

                annotation_queries = [

                    query

                    for query in large_queries.captured_queries

                    if 'FROM "annotations_annotation"' in query["sql"]

                ]

                self.assertEqual(len(annotation_queries), 1)

src/resources/acl.py

+95 −3
Changes for src/resources/acl.py: 95 added lines, 3 removed lines.
Original line number Diff line number Diff line
@@ -11,8 +11,11 @@ from resources.models import ( 
    MetadataSet,

    Tag,

    ProjectProperty,

    CollectionMembership,

)

from typing import Iterator, Union, List

from collections import defaultdict

from itertools import chain

from typing import Iterable, Iterator, Union, List

from rpc.errors import ServiceException

from rpc.const import NO_ACCESS

import logging
@@ -131,6 +134,8 @@ def has_crud_access( 
    user_permissions: List[ObjectPermission],

    object_instance: AccessControlledObject,

    crud_access: str,

    *,

    ancestors: Iterable[Collection] | None = None,

) -> bool:

    crud_access = _normalize_crud_verb(crud_access)

    # Only Resource and Collection are tested for instance-level permissions
@@ -141,7 +146,12 @@ def has_crud_access( 
            object_instance, Collection

        ):

            # node is either a Resource or a Collection instance

            for node in _climb_tree(object_instance):

            nodes = (

                _climb_tree(object_instance)

                if ancestors is None

                else chain((object_instance,), ancestors)

            )

            for node in nodes:

                tested_permissions_boolean_values = []

                for permission in user_permissions:

                    if (
@@ -261,6 +271,79 @@ class UserAccess: 
        self.user = user

        self.project = project

        self._permissions = None

        self._resource_ancestors: dict[int, list[Collection]] = {}



    def prefetch_resource_ancestors(self, resources: List[Resource]) -> None:

        """Cache ACL ancestry for a resource list without a walk per resource."""

        if not resources:

            return

        self._resource_ancestors.update({resource.pk: [] for resource in resources})

        if not any(

            permission.object_pk is not None

            and permission.object_class in (PERM_CLASS_RESOURCE, PERM_CLASS_COLLECTION)

            for permission in self.permissions

        ):

            return



        collections_by_resource_id: dict[int, list[Collection]] = defaultdict(list)

        collections_by_id: dict[int, Collection] = {}

        for membership in (

            CollectionMembership.objects.filter(

                resource_id__in=[resource.pk for resource in resources],

                collection__deleted_at__isnull=True,

            )

            .select_related("collection")

            .only(

                "resource_id",

                "collection_id",

                "collection__id",

                "collection__parent_id",

                "collection__project_id",

            )

            .order_by("collection__title")

        ):

            collection = collections_by_id.setdefault(

                membership.collection_id, membership.collection

            )

            collections_by_resource_id[membership.resource_id].append(collection)



        missing_parent_ids = {

            collection.parent_id

            for collection in collections_by_id.values()

            if collection.parent_id and collection.parent_id not in collections_by_id

        }

        while missing_parent_ids:

            parents = {

                collection.pk: collection

                for collection in Collection.objects.filter(

                    pk__in=missing_parent_ids

                ).only("id", "parent_id", "project_id")

            }

            collections_by_id.update(parents)

            missing_parent_ids = {

                collection.parent_id

                for collection in parents.values()

                if collection.parent_id

                and collection.parent_id not in collections_by_id

            }



        ancestor_chains: dict[int, list[Collection]] = {}

        for resource_id, collections in collections_by_resource_id.items():

            for collection in collections:

                if collection.pk not in ancestor_chains:

                    ancestors = []

                    current_collection = collection

                    seen_ids = set()

                    while current_collection and current_collection.pk not in seen_ids:

                        seen_ids.add(current_collection.pk)

                        ancestors.append(current_collection)

                        current_collection = collections_by_id.get(

                            current_collection.parent_id

                        )

                    ancestor_chains[collection.pk] = ancestors

                self._resource_ancestors[resource_id].extend(

                    ancestor_chains[collection.pk]

                )



    @property

    def permissions(self) -> List[ObjectPermission]:
@@ -287,7 +370,14 @@ class UserAccess: 
        if any(project_id != self.project.pk for project_id in project_ids):

            return False

        return (

            has_crud_access(self.permissions, object_or_class, crud_access)

            has_crud_access(

                self.permissions,

                object_or_class,

                crud_access,

                ancestors=self._resource_ancestors.get(object_or_class.pk)

                if isinstance(object_or_class, Resource)

                else None,

            )

            and self.user.is_active

        )
@@ -392,6 +482,7 @@ class UserAccess: 
            project=self.project, user=self.user, role=role

        )

        self._permissions = None

        self._resource_ancestors.clear()

        return access



    def remove_role_from_user(self, role: Role):
@@ -401,3 +492,4 @@ class UserAccess: 
            project=self.project, user=self.user, role=role

        ).delete()

        self._permissions = None

        self._resource_ancestors.clear()

src/rpc/methods.py

+25 −9
Changes for src/rpc/methods.py: 25 added lines, 9 removed lines.
Original line number Diff line number Diff line 
import logging

import os

import re

from collections import defaultdict

from datetime import timedelta

from functools import wraps as _wraps

from glob import glob as _glob
@@ -3599,17 +3600,21 @@ def list_collection_annotations(user: User, collection_id: int) -> List[dict]: 
    )

    if not collection_instance:

        raise ServiceException("no such collection")

    UserAccess(user, collection_instance.project).check_read(collection_instance)

    resources_list = []

    for resource_instance in _resources_with_serialization_data(

    acl = UserAccess(user, collection_instance.project)

    acl.check_read(collection_instance)

    resources = list(

        _resources_with_serialization_data(

            collection_instance.available_resources(), include_metas=False

    ):

        serialized_resource = serializers.resource(

            resource_instance, include_metas=False

        )

        serialized_resource["annotations"] = []

    )

    acl.prefetch_resource_ancestors(resources)

    readable_resources = [resource for resource in resources if acl.can_read(resource)]

    annotations_by_resource_id: dict[int, list[dict]] = defaultdict(list)

    if readable_resources:

        for annotation in (

            Annotation.objects.filter(resource=resource_instance)

            Annotation.objects.filter(

                resource_id__in=[resource.pk for resource in readable_resources]

            )

            .select_related("owner")

            .only(

                "id",
@@ -3624,7 +3629,18 @@ def list_collection_annotations(user: User, collection_id: int) -> List[dict]: 
            )

            .order_by("created_at")

        ):

            serialized_resource["annotations"].append(_serialize_annotation(annotation))

            annotations_by_resource_id[annotation.resource_id].append(

                _serialize_annotation(annotation)

            )

    serializer_cache = SerializerCache()

    resources_list = []

    for resource_instance in readable_resources:

        serialized_resource = serializers.resource(

            resource_instance, include_metas=False, cache=serializer_cache

        )

        serialized_resource["annotations"] = annotations_by_resource_id[

            resource_instance.pk

        ]

        resources_list.append(serialized_resource)

    return resources_list