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

Authorize collection restores before changing subtree state 

Collection restoration previously checked only the destination's update
permission, allowing callers to restore denied collections and resources.

Authorize the source and destination before loading descendants, then check
update access on every item whose state will change. Validate destinations,
project boundaries, cycles, root moves, and duplicate titles before saving.
Lock affected rows and wrap restoration in a transaction so failures roll back
both restored data and queued database tasks.

Load descendants by level and cache ACL ancestry in batches. Include deleted
collection memberships only when those collections are being restored, keeping
legitimate inherited grants and existing permission precedence intact. Restore
shared resources once, leave active descendants untouched, and retain the ARK
save hooks.

Add 15 regression tests covering denied updates, inherited permissions, invalid
moves, rollback, save hooks, unchanged active items, and read-query scaling.

Validation: all 207 resources, rpc, and annotations tests passed; Ruff and
git diff --check passed. A five-run in-memory SQLite benchmark with 21 children
and resources reduced total queries from 135 to 93 and median runtime from
about 47 ms to 30 ms, including save-hook work.
parent 218b70a7

src/resources/acl.py

+13 −3
Changes for src/resources/acl.py: 13 added lines, 3 removed lines.
Original line number Diff line number Diff line
@@ -15,6 +15,7 @@ from resources.models import ( 
)

from collections import defaultdict

from itertools import chain

from django.db.models import Q

from typing import Iterable, Iterator, Union, List

from rpc.errors import ServiceException

from rpc.const import NO_ACCESS
@@ -273,8 +274,13 @@ class UserAccess: 
        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."""

    def prefetch_resource_ancestors(

        self,

        resources: List[Resource],

        *,

        restoring_collection_ids: Iterable[int] = (),

    ) -> None:

        """Cache ACL ancestry, including memberships scheduled for restoration."""

        if not resources:

            return

        self._resource_ancestors.update({resource.pk: [] for resource in resources})
@@ -287,10 +293,14 @@ class UserAccess: 


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

        collections_by_id: dict[int, Collection] = {}

        available_collections = Q(collection__deleted_at__isnull=True)

        restoring_collection_ids = set(restoring_collection_ids)

        if restoring_collection_ids:

            available_collections |= Q(collection_id__in=restoring_collection_ids)

        for membership in (

            CollectionMembership.objects.filter(

                available_collections,

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

                collection__deleted_at__isnull=True,

            )

            .select_related("collection")

            .only(

src/rpc/methods.py

+116 −26
Changes for src/rpc/methods.py: 116 added lines, 26 removed lines.
Original line number Diff line number Diff line
@@ -2916,48 +2916,138 @@ def restore_resource( 




@_rpc_groups(["Collections"])

@transaction.atomic

def restore_collection(

    user: User, collection_id: int, destination_collection_id: int

) -> bool:

    """

    Restore a deleted collection from the recycle bin

    """

    collection_instance = Collection.objects.filter(pk=collection_id).first()

    if collection_id == destination_collection_id:

        raise ServiceException(NO_FOOTGUNS)

    locked_collections = {

        collection.pk: collection

        for collection in Collection.objects.select_for_update(of=("self",))

        .filter(pk__in=[collection_id, destination_collection_id])

        .select_related("project")

        .order_by("pk")

    }

    collection_instance = locked_collections.get(collection_id)

    if not collection_instance:

        raise ServiceException(NO_SUCH_COLLECTION)

    destination_collection_instance: Union[Collection, None] = (

        Collection.objects.filter(pk=destination_collection_id).first()

    )

    if not destination_collection_instance:

    destination_collection_instance = locked_collections.get(destination_collection_id)

    if (

        not destination_collection_instance

        or destination_collection_instance.deleted_at

    ):

        raise ServiceException(NO_SUCH_COLLECTION)

    if destination_collection_instance.project_id != collection_instance.project_id:

        raise ServiceException(PROJECT_MISMATCH)



    UserAccess(user, destination_collection_instance.project).check_update(

        destination_collection_instance

    moving = collection_instance.parent_id != destination_collection_id

    if moving and collection_instance.parent_id is None:

        raise ServiceException(CANT_MOVE_ROOT_COLLECTION)



    # Authorize the source and destination before loading their descendants.

    acl_collections = dict(locked_collections)

    missing_parent_ids = {

        collection.parent_id

        for collection in acl_collections.values()

        if collection.parent_id and collection.parent_id not in acl_collections

    }

    while missing_parent_ids:

        parents = {

            parent.pk: parent

            for parent in Collection.objects.filter(pk__in=missing_parent_ids).only(

                "id", "parent_id", "project_id", "title"

            )

        }

        acl_collections.update(parents)

        missing_parent_ids = {

            parent.parent_id

            for parent in parents.values()

            if parent.parent_id and parent.parent_id not in acl_collections

        }

    for collection in (collection_instance, destination_collection_instance):

        seen_ids = set()

        parent = collection

        while parent:

            if parent.pk in seen_ids:

                raise ServiceException(NO_FOOTGUNS)

            seen_ids.add(parent.pk)

            parent = acl_collections.get(parent.parent_id)

    for collection in acl_collections.values():

        collection.parent = acl_collections.get(collection.parent_id)



    def _restore(col_instance: Collection):

        col_instance.deleted_at = None

        col_instance.save()

        for resource_instance in col_instance.resource_set.filter(

            deleted_at__isnull=False

        ):

            # restore_resource(user, resource_instance.pk, col_instance.pk)

            resource_instance.deleted_at = None

            resource_instance.save()

        for child_collection_instance in Collection.objects.filter(parent=col_instance):

            _restore(child_collection_instance)

    acl = UserAccess(user, collection_instance.project)

    acl.check_update(destination_collection_instance)

    acl.check_update(collection_instance)

    if moving:

        acl.check_read(destination_collection_instance)

        if Collection.objects.filter(

            project_id=collection_instance.project_id,

            parent=destination_collection_instance,

            title=collection_instance.title,

        ).exists():

            raise ServiceException(COLLECTION_ALREADY_EXIST_IN_PARENT)



    # recurse collection, respawn all descendants

    _restore(collection_instance)

    if (

        collection_instance.parent

        and collection_instance.parent.pk != destination_collection_id

    ):

        move_collection(

            user, collection_instance.pk, destination_collection_instance.pk

    subtree = {collection_instance.pk: collection_instance}

    parent_ids = [collection_instance.pk]

    while parent_ids:

        children = list(

            Collection.objects.select_for_update()

            .filter(parent_id__in=parent_ids)

            .order_by("pk")

        )

        parent_ids = []

        for child in children:

            if child.pk in subtree or child.pk == destination_collection_id:

                raise ServiceException(NO_FOOTGUNS)

            if child.project_id != collection_instance.project_id:

                raise ServiceException(PROJECT_MISMATCH)

            child.parent = subtree[child.parent_id]

            subtree[child.pk] = child

            parent_ids.append(child.pk)



    changing_collections = [

        collection

        for collection in subtree.values()

        if collection.deleted_at or (collection.pk == collection_id and moving)

    ]

    for collection in changing_collections:

        acl.check_update(collection)



    resources = list(

        Resource.objects.select_for_update()

        .filter(

            pk__in=CollectionMembership.objects.filter(

                collection_id__in=subtree

            ).values("resource_id"),

            deleted_at__isnull=False,

        )

        .order_by("pk")

    )

    acl.prefetch_resource_ancestors(

        resources,

        restoring_collection_ids=[

            collection.pk

            for collection in changing_collections

            if collection.deleted_at

        ],

    )

    for resource in resources:

        if resource.ptr_project_id != collection_instance.project_id:

            raise ServiceException(PROJECT_MISMATCH)

        acl.check_update(resource)



    if moving:

        collection_instance.parent = destination_collection_instance

    for collection in changing_collections:

        collection.deleted_at = None

        collection.save()

    for resource in resources:

        resource.deleted_at = None

        resource.save()

    return True

src/rpc/tests.py

+339 −1
Changes for src/rpc/tests.py: 339 added lines, 1 removed line.
Original line number Diff line number Diff line 
from django.test import RequestFactory, TestCase

from django.db import connection

from django.test.utils import CaptureQueriesContext

from django.utils import timezone

from django_tasks_db.models import DBTaskResult

from django.contrib.auth.models import AbstractUser, AnonymousUser, User

from rpc import methods

from rpc import serializers
@@ -16,7 +18,8 @@ import tempfile 
from io import BytesIO

from django.urls import reverse

from openpyxl import load_workbook

from unittest.mock import patch

from unittest.mock import Mock, patch

from types import SimpleNamespace

import hashlib

import json

from jama.middleware import ApiKeyCsrfViewMiddleware
@@ -37,6 +40,341 @@ object_classes = [ 
crud = ["create", "read", "update", "delete"]





class RestoreCollectionAccessTestCase(TestCase):

    @classmethod

    def setUpTestData(cls):

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

        cls.project = models.Project.objects.create(

            label="restore access", description=""

        )

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

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

            project=cls.project, parent=cls.root, title="Destination"

        )

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

            project=cls.project,

            parent=cls.root,

            title="Source",

            deleted_at=timezone.now(),

        )

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

            project=cls.project,

            parent=cls.source,

            title="Child",

            deleted_at=timezone.now(),

        )

        cls.resource = models.Resource.objects.create(

            ptr_project=cls.project, title="Resource", deleted_at=timezone.now()

        )

        cls.child.resources.add(cls.resource)

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

        models.ProjectAccess.objects.create(

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

        )

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

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



    def _assert_unchanged(self) -> None:

        self.source.refresh_from_db()

        self.child.refresh_from_db()

        self.resource.refresh_from_db()

        self.assertIsNotNone(self.source.deleted_at)

        self.assertIsNotNone(self.child.deleted_at)

        self.assertIsNotNone(self.resource.deleted_at)

        self.assertEqual(self.source.parent_id, self.root.pk)



    def test_denied_source_cannot_be_restored_to_its_existing_parent(self):

        set_permission_on_role(self.role, self.source, can_read=True, can_update=False)



        with self.assertRaises(ServiceException):

            methods.restore_collection(self.user, self.source.pk, self.root.pk)



        self._assert_unchanged()



    def test_denied_descendant_or_resource_prevents_all_changes(self):

        for target in (self.child, self.resource):

            with self.subTest(target=str(target)):

                set_permission_on_role(

                    self.role, target, can_read=True, can_update=False

                )

                task_stub = SimpleNamespace(

                    set_ark_to_collection=SimpleNamespace(enqueue=Mock()),

                    set_ark_to_resource=SimpleNamespace(enqueue=Mock()),

                )

                with (

                    patch.object(models, "tasks", task_stub),

                    self.assertRaises(ServiceException),

                ):

                    methods.restore_collection(

                        self.user, self.source.pk, self.destination.pk

                    )

                self._assert_unchanged()

                task_stub.set_ark_to_collection.enqueue.assert_not_called()

                task_stub.set_ark_to_resource.enqueue.assert_not_called()

                models.ObjectPermission.objects.filter(

                    role=self.role, object_pk__isnull=False

                ).delete()



    def test_inherited_update_grant_on_deleted_collection_allows_restore(self):

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

        set_permission_on_role(self.role, self.source, can_read=True, can_update=True)

        self.assertFalse(UserAccess(self.user, self.project).can_update(self.resource))



        self.assertTrue(

            methods.restore_collection(self.user, self.source.pk, self.destination.pk)

        )



        for item in (self.source, self.child, self.resource):

            item.refresh_from_db()

            self.assertIsNone(item.deleted_at)

        self.assertEqual(self.source.parent_id, self.destination.pk)



    def test_unrelated_deleted_collection_cannot_grant_restore_access(self):

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

        outside = models.Collection.objects.create(

            project=self.project,

            parent=self.root,

            title="A deleted outside",

            deleted_at=timezone.now(),

        )

        set_permission_on_role(self.role, outside, can_update=True)

        outside.resources.add(self.resource)



        with self.assertRaises(ServiceException):

            methods.restore_collection(self.user, self.source.pk, self.destination.pk)



        self._assert_unchanged()



    def test_other_active_membership_denial_is_preserved(self):

        outside = models.Collection.objects.create(

            project=self.project, parent=self.root, title="A restricted outside"

        )

        set_permission_on_role(self.role, outside, can_update=False)

        outside.resources.add(self.resource)

        set_permission_on_role(self.role, self.source, can_read=True, can_update=True)



        with self.assertRaises(ServiceException):

            methods.restore_collection(self.user, self.source.pk, self.destination.pk)



        self._assert_unchanged()



    def test_active_descendants_are_not_modified(self):

        active_child = models.Collection.objects.create(

            project=self.project, parent=self.source, title="Active child"

        )

        active_resource = models.Resource.objects.create(

            ptr_project=self.project, title="Active resource"

        )

        active_child.resources.add(active_resource)

        set_permission_on_role(self.role, active_child, can_update=False)

        set_permission_on_role(self.role, active_resource, can_update=False)

        previous_updated_at = (active_child.updated_at, active_resource.updated_at)



        methods.restore_collection(self.user, self.source.pk, self.destination.pk)



        active_child.refresh_from_db()

        active_resource.refresh_from_db()

        self.assertEqual(

            (active_child.updated_at, active_resource.updated_at), previous_updated_at

        )



    def test_preserves_save_hooks_and_restores_shared_resource_once(self):

        self.source.resources.add(self.resource)

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

        file = models.File.objects.create(

            project=self.project,

            title="Deleted file",

            original_name="deleted.txt",

            hash="a" * 64,

            size=1,

            file_type=file_type,

            deleted_at=timezone.now(),

        )

        self.child.resources.add(file)

        task_stub = SimpleNamespace(

            set_ark_to_collection=SimpleNamespace(enqueue=Mock()),

            set_ark_to_resource=SimpleNamespace(enqueue=Mock()),

        )

        with patch.object(models, "tasks", task_stub):

            methods.restore_collection(self.user, self.source.pk, self.root.pk)



        self.assertEqual(task_stub.set_ark_to_collection.enqueue.call_count, 2)

        self.assertEqual(task_stub.set_ark_to_resource.enqueue.call_count, 2)

        self.assertEqual(

            {

                call.args[0]

                for call in task_stub.set_ark_to_resource.enqueue.call_args_list

            },

            {self.resource.pk, file.pk},

        )

        file.refresh_from_db()

        self.assertIsNone(file.deleted_at)



    def test_database_and_queued_tasks_roll_back_on_save_failure(self):

        queued_tasks_before = DBTaskResult.objects.count()

        with patch.object(

            models.Resource, "save", side_effect=RuntimeError("save failed")

        ):

            with self.assertRaisesRegex(RuntimeError, "save failed"):

                methods.restore_collection(

                    self.user, self.source.pk, self.destination.pk

                )



        self._assert_unchanged()

        self.assertEqual(DBTaskResult.objects.count(), queued_tasks_before)



    def test_invalid_destinations_do_not_restore_anything(self):

        deleted_destination = models.Collection.objects.create(

            project=self.project,

            parent=self.root,

            title="Deleted destination",

            deleted_at=timezone.now(),

        )

        other_project = models.Project.objects.create(

            label="other restore", description=""

        )

        foreign_destination = models.Collection.objects.create(

            project=other_project, title="Other root"

        )

        for destination in (

            self.source,

            self.child,

            deleted_destination,

            foreign_destination,

        ):

            with (

                self.subTest(destination=str(destination)),

                self.assertRaises(ServiceException),

            ):

                methods.restore_collection(self.user, self.source.pk, destination.pk)

            self._assert_unchanged()



    def test_active_descendant_cannot_be_the_destination(self):

        self.child.deleted_at = None

        self.child.save()



        with self.assertRaises(ServiceException) as error:

            methods.restore_collection(self.user, self.source.pk, self.child.pk)



        self.assertEqual(error.exception.message, methods.NO_FOOTGUNS)

        self.source.refresh_from_db()

        self.resource.refresh_from_db()

        self.assertIsNotNone(self.source.deleted_at)

        self.assertIsNotNone(self.resource.deleted_at)

        self.assertEqual(self.source.parent_id, self.root.pk)



    def test_project_root_cannot_be_reparented(self):

        self.root.soft_delete()



        with self.assertRaises(ServiceException) as error:

            methods.restore_collection(self.user, self.root.pk, self.destination.pk)



        self.assertEqual(error.exception.message, methods.CANT_MOVE_ROOT_COLLECTION)

        self.root.refresh_from_db()

        self.assertIsNotNone(self.root.deleted_at)

        self.assertIsNone(self.root.parent_id)

        self._assert_unchanged()



    def test_inactive_user_cannot_restore(self):

        self.user.is_active = False

        self.user.save()



        with self.assertRaises(ServiceException):

            methods.restore_collection(self.user, self.source.pk, self.root.pk)



        self._assert_unchanged()



    def test_denied_destination_or_duplicate_title_does_not_restore_anything(self):

        set_permission_on_role(

            self.role, self.destination, can_read=True, can_update=False

        )

        with self.assertRaises(ServiceException):

            methods.restore_collection(self.user, self.source.pk, self.destination.pk)

        self._assert_unchanged()

        set_permission_on_role(

            self.role, self.destination, can_read=True, can_update=True

        )

        models.Collection.objects.create(

            project=self.project, parent=self.destination, title=self.source.title

        )

        with self.assertRaises(ServiceException) as error:

            methods.restore_collection(self.user, self.source.pk, self.destination.pk)

        self.assertEqual(

            error.exception.message, methods.COLLECTION_ALREADY_EXIST_IN_PARENT

        )

        self._assert_unchanged()



    def test_foreign_descendant_or_resource_is_rejected(self):

        other_project = models.Project.objects.create(

            label="other restore", description=""

        )

        foreign_child = models.Collection.objects.create(

            project=other_project, parent=self.source, title="Foreign child"

        )

        with self.assertRaises(ServiceException):

            methods.restore_collection(self.user, self.source.pk, self.destination.pk)

        self._assert_unchanged()

        foreign_child.delete()

        foreign_resource = models.Resource.objects.create(

            ptr_project=other_project,

            title="Foreign resource",

            deleted_at=timezone.now(),

        )

        self.child.resources.add(foreign_resource)

        with self.assertRaises(ServiceException):

            methods.restore_collection(self.user, self.source.pk, self.destination.pk)

        self._assert_unchanged()

        foreign_resource.refresh_from_db()

        self.assertIsNotNone(foreign_resource.deleted_at)



    def test_select_queries_do_not_grow_with_subtree_width(self):

        set_permission_on_role(self.role, self.source, can_read=True, can_update=True)

        with CaptureQueriesContext(connection) as small_queries:

            methods.restore_collection(self.user, self.source.pk, self.root.pk)

        for item in (self.source, self.child, self.resource):

            item.deleted_at = timezone.now()

            item.save()

        for index in range(20):

            child = models.Collection.objects.create(

                project=self.project,

                parent=self.source,

                title=f"Extra {index}",

                deleted_at=timezone.now(),

            )

            resource = models.Resource.objects.create(

                ptr_project=self.project,

                title=f"Extra resource {index}",

                deleted_at=timezone.now(),

            )

            child.resources.add(resource)

        with CaptureQueriesContext(connection) as large_queries:

            methods.restore_collection(self.user, self.source.pk, self.root.pk)



        small_selects = [

            query

            for query in small_queries.captured_queries

            if query["sql"].lstrip().upper().startswith("SELECT")

        ]

        large_selects = [

            query

            for query in large_queries.captured_queries

            if query["sql"].lstrip().upper().startswith("SELECT")

        ]

        self.assertEqual(len(small_selects), len(large_selects))

        self.assertEqual(

            models.Collection.objects.filter(

                parent=self.source, deleted_at__isnull=True

            ).count(),

            21,

        )

        self.assertEqual(

            models.Resource.objects.filter(

                ptr_project=self.project, deleted_at__isnull=True

            ).count(),

            21,

        )





class XlsxExportAccessTestCase(TestCase):

    @classmethod

    def setUpTestData(cls):