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

Enforce file permissions when reusing uploads 

The upload hash shortcut previously required only resource creation access,
allowing callers to reuse denied files and restore or rename deleted files.
Restoration also happened before the attachment transaction, so a rejected
attachment could leave the file restored despite returning HTTP 403.

Require read access before reusing an existing file and update access before
restoring it. Lock the existing file and keep authorization, restoration,
and attachment in one transaction so failures roll back file changes and
queued database tasks.

Apply the same checks during completed-upload imports through an optional
authorization callback in handle_uploaded_file. This covers files that
appear after the initial hash lookup while preserving create-only access
for new uploads and existing behavior for trusted internal callers.

Add nine regression tests and update the existing attachment test to require
read access. Cover denied reuse and restoration, permitted reuse without
update access, successful restoration, attachment rollback, completed-upload
reuse, and create-only uploads.

Validation: Ruff passed for all changed Python files. All 216 resource, RPC,
and annotation tests passed using an isolated in-memory SQLite database.
parent fa639907

src/resources/helpers.py

+13 −2
Changes for src/resources/helpers.py: 13 added lines, 2 removed lines.
Original line number Diff line number Diff line
@@ -13,7 +13,7 @@ from shutil import copyfile 
from django.db.utils import IntegrityError

from unidecode import unidecode

import logging

from typing import Union

from typing import Callable, Union

from pathlib import Path

import subprocess
@@ -538,11 +538,20 @@ def handle_uploaded_file( 
    uploaded_file: File,

    project: models.Project,

    force_file_name=None,

    *,

    check_existing_access: Callable[[models.File], None] | None = None,

) -> int:

    """Import an upload, optionally authorizing reuse under a row lock.



    Callers supplying check_existing_access must hold a database transaction.

    """

    upload_hash = hash_upload(uploaded_file)

    title, extension = os.path.splitext(uploaded_file.name)

    # copy file to disk and create db file record if not already here

    file = models.File.objects.filter(hash=upload_hash, project=project).first()

    existing_files = models.File.objects.filter(hash=upload_hash, project=project)

    if check_existing_access is not None:

        existing_files = existing_files.select_for_update()

    file = existing_files.first()

    if not file:

        destination_file = models.hash_to_local_path(upload_hash)

        _copy_uploaded_file_to_destination(uploaded_file, destination_file)
@@ -558,6 +567,8 @@ def handle_uploaded_file( 
            return file.id

        except IntegrityError:

            raise ConcurrencyError()

    if check_existing_access is not None:

        check_existing_access(file)

    # file is respawned ?

    if file.deleted_at:

        file.deleted_at = None

src/rpc/tests.py

+306 −2
Changes for src/rpc/tests.py: 306 added lines, 2 removed lines.
Original line number Diff line number Diff line
@@ -1303,7 +1303,7 @@ class ServiceTestCase(TestCase): 
            models.Collection.objects.filter(title="readonly collection").exists()

        )



    def test_upload_partial_collection_id_skips_resource_read_access(self):

    def test_upload_partial_collection_id_requires_existing_resource_read_access(self):

        file_instance = self._public_download_test_file()

        collection = models.Collection.objects.create(

            title="target collection",
@@ -1323,8 +1323,312 @@ class ServiceTestCase(TestCase): 
            )

        )



        self.assertEqual(response.status_code, 403)

        self.assertFalse(collection.resources.filter(pk=file_instance.pk).exists())



    def test_upload_partial_hash_shortcut_rejects_unreadable_existing_file(self):

        file_instance = self._public_download_test_file()

        set_permission_on_role(self.admin_role, file_instance, can_update=True)



        for deleted_at in (None, timezone.now()):

            with self.subTest(deleted=deleted_at is not None):

                models.Resource.objects.filter(pk=file_instance.pk).update(

                    deleted_at=deleted_at

                )

                queued_tasks = DBTaskResult.objects.count()

                response = views.upload_partial(

                    self._upload_partial_request(file_instance.hash, "1/1", data=b"")

                )



                self.assertEqual(response.status_code, 403)

                file_instance.refresh_from_db()

                self.assertEqual(file_instance.deleted_at, deleted_at)

                self.assertEqual(file_instance.original_name, "test_public_file.png")

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



    def test_upload_partial_requires_update_to_restore_existing_file(self):

        file_instance = self._public_download_test_file()

        file_instance.deleted_at = timezone.now()

        file_instance.save()

        deleted_at = file_instance.deleted_at

        updated_at = file_instance.updated_at

        queued_tasks = DBTaskResult.objects.count()

        set_permission_on_role(self.admin_role, file_instance, can_read=True)



        response = views.upload_partial(

            self._upload_partial_request(file_instance.hash, "1/1", data=b"")

        )



        self.assertEqual(response.status_code, 403)

        file_instance.refresh_from_db()

        self.assertEqual(file_instance.deleted_at, deleted_at)

        self.assertEqual(file_instance.updated_at, updated_at)

        self.assertEqual(file_instance.title, "test public file")

        self.assertEqual(file_instance.original_name, "test_public_file.png")

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



    def test_upload_partial_reuses_readable_active_file_without_update_access(self):

        file_instance = self._public_download_test_file()

        updated_at = file_instance.updated_at

        queued_tasks = DBTaskResult.objects.count()

        set_permission_on_role(self.admin_role, file_instance, can_read=True)



        response = views.upload_partial(

            self._upload_partial_request(

                file_instance.hash,

                "1/1",

                data=b"",

                collection_id=self.test_project_root_collection.pk,

            )

        )



        self.assertEqual(response.status_code, 200)

        self.assertEqual(int(response.content), file_instance.pk)

        file_instance.refresh_from_db()

        self.assertEqual(file_instance.updated_at, updated_at)

        self.assertEqual(file_instance.title, "test public file")

        self.assertEqual(file_instance.original_name, "test_public_file.png")

        self.assertTrue(

            self.test_project_root_collection.resources.filter(

                pk=file_instance.pk

            ).exists()

        )

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



    def test_upload_partial_restores_and_attaches_permitted_existing_file(self):

        file_instance = self._public_download_test_file()

        file_instance.deleted_at = timezone.now()

        file_instance.save()

        queued_tasks = DBTaskResult.objects.count()

        set_permission_on_role(

            self.admin_role, file_instance, can_read=True, can_update=True

        )



        response = views.upload_partial(

            self._upload_partial_request(

                file_instance.hash,

                "1/1",

                collection_id=self.test_project_root_collection.pk,

            )

        )



        self.assertEqual(response.status_code, 200)

        self.assertEqual(int(response.content), file_instance.pk)

        file_instance.refresh_from_db()

        self.assertIsNone(file_instance.deleted_at)

        self.assertEqual(file_instance.title, "test.png")

        self.assertEqual(file_instance.original_name, "test.png")

        self.assertTrue(

            self.test_project_root_collection.resources.filter(

                pk=file_instance.pk

            ).exists()

        )

        self.assertEqual(DBTaskResult.objects.count(), queued_tasks + 1)



    def test_upload_partial_attachment_failure_rolls_back_existing_file_restore(self):

        file_instance = self._public_download_test_file()

        file_instance.deleted_at = timezone.now()

        file_instance.save()

        deleted_at = file_instance.deleted_at

        updated_at = file_instance.updated_at

        queued_tasks = DBTaskResult.objects.count()

        models.ObjectPermission.objects.filter(

            role=self.admin_role, object_class="collection", object_pk=None

        ).update(object_update=False)



        destinations = [

            {"collection_id": self.test_project_root_collection.pk},

            {"collection_id": self.test_project_root_collection.pk + 10000},

            {"origin_dir": "readonly collection"},

        ]

        for destination in destinations:

            with self.subTest(destination=destination):

                response = views.upload_partial(

                    self._upload_partial_request(

                        file_instance.hash, "1/1", **destination

                    )

                )



                self.assertEqual(response.status_code, 403)

                file_instance.refresh_from_db()

                self.assertEqual(file_instance.deleted_at, deleted_at)

                self.assertEqual(file_instance.updated_at, updated_at)

                self.assertEqual(file_instance.title, "test public file")

                self.assertEqual(file_instance.original_name, "test_public_file.png")

                self.assertFalse(file_instance.collections.exists())

                self.assertFalse(

                    models.Collection.objects.filter(

                        title="readonly collection"

                    ).exists()

                )

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



    def test_upload_partial_new_file_needs_only_create_resource_access(self):

        data = b"new file for create-only user"

        file_hash = hashlib.sha256(data).hexdigest()

        set_permission_on_role(self.admin_role, "resource", can_create=True)



        with tempfile.TemporaryDirectory() as tmpdir:

            with (

                patch.object(views.settings, "PARTIAL_UPLOADS_DIR", tmpdir),

                patch.object(views.settings, "MEDIA_FILES_DIR", tmpdir),

                patch.object(views, "_enqueue_file_tasks"),

            ):

                response = views.upload_partial(

                    self._upload_partial_request(

                        file_hash,

                        "1/1",

                        data,

                        collection_id=self.test_project_root_collection.pk,

                    )

                )



        self.assertEqual(response.status_code, 200)

        file_instance = models.File.objects.get(pk=int(response.content))

        self.assertEqual(file_instance.hash, file_hash)

        self.assertTrue(

            self.test_project_root_collection.resources.filter(

                pk=file_instance.pk

            ).exists()

        )



    def test_upload_partial_completion_checks_existing_file_permissions(self):

        data = b"test"

        file_instance = self._public_download_test_file()

        models.File.objects.filter(pk=file_instance.pk).update(

            hash=hashlib.sha256(data).hexdigest()

        )

        file_instance.refresh_from_db()



        cases = [(None, False), (timezone.now(), False), (timezone.now(), True)]

        for deleted_at, can_read in cases:

            with self.subTest(deleted=deleted_at is not None, can_read=can_read):

                models.Resource.objects.filter(pk=file_instance.pk).update(

                    deleted_at=deleted_at

                )

                set_permission_on_role(

                    self.admin_role, "resource", can_create=True, can_read=can_read

                )

                queued_tasks = DBTaskResult.objects.count()

                with tempfile.TemporaryDirectory() as tmpdir:

                    with (

                        patch.object(views.settings, "PARTIAL_UPLOADS_DIR", tmpdir),

                        # Simulate a file appearing after the initial hash lookup.

                        patch.object(

                            views, "_existing_upload_response", return_value=None

                        ),

                        patch.object(

                            views, "_enqueue_file_tasks"

                        ) as enqueue_file_tasks,

                    ):

                        response = views.upload_partial(

                            self._upload_partial_request(

                                file_instance.hash, "1/1", data

                            )

                        )



                self.assertEqual(response.status_code, 403)

                file_instance.refresh_from_db()

                self.assertEqual(file_instance.deleted_at, deleted_at)

                self.assertEqual(file_instance.title, "test public file")

                self.assertEqual(file_instance.original_name, "test_public_file.png")

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

                enqueue_file_tasks.assert_not_called()



    def test_upload_partial_completion_reuses_permitted_existing_file(self):

        data = b"test"

        file_instance = self._public_download_test_file()

        models.File.objects.filter(pk=file_instance.pk).update(

            hash=hashlib.sha256(data).hexdigest()

        )

        file_instance.refresh_from_db()



        for deleted_at in (None, timezone.now()):

            with self.subTest(deleted=deleted_at is not None):

                models.Resource.objects.filter(pk=file_instance.pk).update(

                    deleted_at=deleted_at

                )

                set_permission_on_role(

                    self.admin_role,

                    "resource",

                    can_create=True,

                    can_read=True,

                    can_update=deleted_at is not None,

                )

                queued_tasks = DBTaskResult.objects.count()

                with tempfile.TemporaryDirectory() as tmpdir:

                    with (

                        patch.object(views.settings, "PARTIAL_UPLOADS_DIR", tmpdir),

                        patch.object(

                            views, "_existing_upload_response", return_value=None

                        ),

                        patch.object(

                            views, "_enqueue_file_tasks"

                        ) as enqueue_file_tasks,

                    ):

                        response = views.upload_partial(

                            self._upload_partial_request(

                                file_instance.hash,

                                "1/1",

                                data,

                                collection_id=self.test_project_root_collection.pk,

                            )

                        )



                self.assertEqual(response.status_code, 200)

        self.assertTrue(collection.resources.filter(pk=file_instance.pk).exists())

                self.assertEqual(int(response.content), file_instance.pk)

                file_instance.refresh_from_db()

                self.assertIsNone(file_instance.deleted_at)

                expected_name = "test.png" if deleted_at else "test_public_file.png"

                self.assertEqual(file_instance.original_name, expected_name)

                self.assertTrue(

                    self.test_project_root_collection.resources.filter(

                        pk=file_instance.pk

                    ).exists()

                )

                self.assertEqual(

                    DBTaskResult.objects.count(),

                    queued_tasks + int(deleted_at is not None),

                )

                enqueue_file_tasks.assert_called_once_with(file_instance.pk)



    def test_upload_partial_completion_rolls_back_existing_file_restore(self):

        data = b"test"

        file_instance = self._public_download_test_file()

        file_instance.hash = hashlib.sha256(data).hexdigest()

        file_instance.deleted_at = timezone.now()

        file_instance.save()

        deleted_at = file_instance.deleted_at

        updated_at = file_instance.updated_at

        queued_tasks = DBTaskResult.objects.count()

        models.ObjectPermission.objects.filter(

            role=self.admin_role, object_class="collection", object_pk=None

        ).update(object_update=False)



        with tempfile.TemporaryDirectory() as tmpdir:

            with (

                patch.object(views.settings, "PARTIAL_UPLOADS_DIR", tmpdir),

                patch.object(views, "_existing_upload_response", return_value=None),

                patch.object(views, "_enqueue_file_tasks") as enqueue_file_tasks,

            ):

                response = views.upload_partial(

                    self._upload_partial_request(

                        file_instance.hash,

                        "1/1",

                        data,

                        collection_id=self.test_project_root_collection.pk,

                    )

                )



        self.assertEqual(response.status_code, 403)

        file_instance.refresh_from_db()

        self.assertEqual(file_instance.deleted_at, deleted_at)

        self.assertEqual(file_instance.updated_at, updated_at)

        self.assertEqual(file_instance.title, "test public file")

        self.assertEqual(file_instance.original_name, "test_public_file.png")

        self.assertFalse(file_instance.collections.exists())

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

        enqueue_file_tasks.assert_not_called()



    def test_upload_partial_collection_id_requires_collection_update_access(self):

        file_instance = self._public_download_test_file()

src/rpc/views.py

+17 −8
Changes for src/rpc/views.py: 17 added lines, 8 removed lines.
Original line number Diff line number Diff line
@@ -163,7 +163,6 @@ def _add_file_to_origin_collection( 
) -> models.Collection:

    collection = _collection_from_origin_dir(origin_dir, project, acl)

    acl.check_update(collection)

    # acl.check_read(file_instance)

    collection.resources.add(file_instance)

    return collection
@@ -418,6 +417,12 @@ def _parse_partial_upload_request(request: HttpRequest) -> PartialUploadRequest: 
    )





def _check_existing_upload_access(file_instance: models.File, acl: UserAccess) -> None:

    acl.check_read(file_instance)

    if file_instance.deleted_at:

        acl.check_update(file_instance)





def _restore_deleted_file_if_needed(

    file_instance: models.File,

    file_name: str,
@@ -493,17 +498,18 @@ def _existing_upload_response( 
) -> Union[HttpResponse, None]:

    """Return a response for an already-known file, or None if upload must continue."""

    try:

        file_instance = models.File.objects.get(

        with transaction.atomic():

            file_instance = models.File.objects.select_for_update().get(

                hash=upload.file_hash, project=context.project

            )

    except models.File.DoesNotExist:

        return None



    _restore_deleted_file_if_needed(file_instance, upload.file_name, context.user)

            _check_existing_upload_access(file_instance, context.acl)

            _restore_deleted_file_if_needed(

                file_instance, upload.file_name, context.user

            )

            if upload.collection_id is not None or upload.origin_dir:

        try:

            with transaction.atomic():

                _attach_file_to_requested_collection(upload, file_instance, context)

    except models.File.DoesNotExist:

        return None

    except (

        models.Collection.DoesNotExist,

        models.Resource.DoesNotExist,
@@ -585,6 +591,9 @@ def _import_completed_partial_upload( 
                File(f),

                context.project,

                force_file_name=upload.file_name,

                check_existing_access=lambda file_instance: (

                    _check_existing_upload_access(file_instance, context.acl)

                ),

            )

            _silent_rmdir(partials_dir)

            logger.info(