From 1e669d977010aadd380634593b188a6de1c17de4 Mon Sep 17 00:00:00 2001 From: Javier Romero Castro Date: Tue, 3 Oct 2023 17:30:29 +0200 Subject: [PATCH] files: add check for deleted record * closes https://github.com/inveniosoftware/invenio-rdm-records/issues/1479 --- invenio_rdm_records/ext.py | 10 +- invenio_rdm_records/resources/config.py | 31 +++++ .../services/files/__init__.py | 11 ++ invenio_rdm_records/services/files/service.py | 34 ++++++ invenio_rdm_records/services/permissions.py | 2 + tests/resources/test_files.py | 106 +++++++++++++++++ tests/resources/test_media_files.py | 110 ++++++++++++++++++ .../files/test_deleted_record_files.py | 105 +++++++++++++++++ 8 files changed, 404 insertions(+), 5 deletions(-) create mode 100644 invenio_rdm_records/services/files/__init__.py create mode 100644 invenio_rdm_records/services/files/service.py create mode 100644 tests/services/files/test_deleted_record_files.py diff --git a/invenio_rdm_records/ext.py b/invenio_rdm_records/ext.py index 7eb10b449..0892b1d63 100644 --- a/invenio_rdm_records/ext.py +++ b/invenio_rdm_records/ext.py @@ -14,7 +14,6 @@ from flask_iiif import IIIF from flask_principal import identity_loaded from invenio_records_resources.resources.files import FileResource -from invenio_records_resources.services import FileService from . import config from .oaiserver.resources.config import OAIPMHServerResourceConfig @@ -62,6 +61,7 @@ RDMMediaFileRecordServiceConfig, RDMRecordMediaFilesServiceConfig, ) +from .services.files import RDMFileService from .services.pids import PIDManager, PIDsService from .services.review.service import ReviewService from .utils import verify_token @@ -147,8 +147,8 @@ def init_services(self, app): # Services self.records_service = RDMRecordService( service_configs.record, - files_service=FileService(service_configs.file), - draft_files_service=FileService(service_configs.file_draft), + files_service=RDMFileService(service_configs.file), + draft_files_service=RDMFileService(service_configs.file_draft), access_service=RecordAccessService(service_configs.record), pids_service=PIDsService(service_configs.record, PIDManager), review_service=ReviewService(service_configs.record), @@ -156,8 +156,8 @@ def init_services(self, app): self.records_media_files_service = RDMRecordService( service_configs.record_with_media_files, - files_service=FileService(service_configs.media_file), - draft_files_service=FileService(service_configs.media_file_draft), + files_service=RDMFileService(service_configs.media_file), + draft_files_service=RDMFileService(service_configs.media_file_draft), pids_service=PIDsService(service_configs.record, PIDManager), ) diff --git a/invenio_rdm_records/resources/config.py b/invenio_rdm_records/resources/config.py index a093d2dea..744dc01a6 100644 --- a/invenio_rdm_records/resources/config.py +++ b/invenio_rdm_records/resources/config.py @@ -26,6 +26,7 @@ from invenio_drafts_resources.resources import RecordResourceConfig from invenio_i18n import lazy_gettext as _ from invenio_records.systemfields.relations import InvalidRelationValue +from invenio_records_resources.resources.errors import ErrorHandlersMixin from invenio_records_resources.resources.files import FileResourceConfig from invenio_records_resources.resources.records.headers import etag_headers from invenio_records_resources.services.base.config import ConfiguratorMixin, FromConfig @@ -233,6 +234,21 @@ class RDMRecordFilesResourceConfig(FileResourceConfig, ConfiguratorMixin): blueprint_name = "record_files" url_prefix = "/records/" + error_handlers = { + **ErrorHandlersMixin.error_handlers, + RecordDeletedException: create_error_handler( + lambda e: ( + HTTPJSONException(code=404, description=_("Record not found")) + if not e.record.tombstone.is_visible + else HTTPJSONException( + code=410, + description=_("Record deleted"), + tombstone=e.record.tombstone.dump(), + ) + ) + ), + } + # # Draft files @@ -260,6 +276,21 @@ class RDMRecordMediaFilesResourceConfig(FileResourceConfig, ConfiguratorMixin): "list-archive": "/media-files-archive", } + error_handlers = { + **ErrorHandlersMixin.error_handlers, + RecordDeletedException: create_error_handler( + lambda e: ( + HTTPJSONException(code=404, description=_("Record not found")) + if not e.record.tombstone.is_visible + else HTTPJSONException( + code=410, + description=_("Record deleted"), + tombstone=e.record.tombstone.dump(), + ) + ) + ), + } + # # Draft files diff --git a/invenio_rdm_records/services/files/__init__.py b/invenio_rdm_records/services/files/__init__.py new file mode 100644 index 000000000..14e56f117 --- /dev/null +++ b/invenio_rdm_records/services/files/__init__.py @@ -0,0 +1,11 @@ +# -*- coding: utf-8 -*- +# +# Copyright (C) 2023 CERN. +# +# Invenio-RDM-Records is free software; you can redistribute it and/or modify +# it under the terms of the MIT License; see LICENSE file for more details. + +"""File Service API.""" +from invenio_rdm_records.services.files.service import RDMFileService + +__all__ = ("RDMFileService",) diff --git a/invenio_rdm_records/services/files/service.py b/invenio_rdm_records/services/files/service.py new file mode 100644 index 000000000..24251bea6 --- /dev/null +++ b/invenio_rdm_records/services/files/service.py @@ -0,0 +1,34 @@ +# -*- coding: utf-8 -*- +# +# Copyright (C) 2023 CERN. +# +# Invenio-RDM-Records is free software; you can redistribute it and/or modify +# it under the terms of the MIT License; see LICENSE file for more details. + +"""File Service API.""" +from invenio_records_resources.services import FileService +from invenio_records_resources.services.errors import FileKeyNotFoundError + +from invenio_rdm_records.services.errors import RecordDeletedException + + +class RDMFileService(FileService): + """A service for adding files support to records.""" + + def _check_record_deleted_permissions(self, record, identity): + """Ensure that the record exists (not deleted) or raise.""" + if record.is_draft: + return + if record.deletion_status.is_deleted: + can_read_deleted = self.check_permission( + identity, "read_deleted_files", record=record + ) + if not can_read_deleted: + raise RecordDeletedException(record) + + def _get_record(self, id_, identity, action, file_key=None): + """Get the associated record.""" + record = super()._get_record(id_, identity, action, file_key) + self._check_record_deleted_permissions(record, identity) + + return record diff --git a/invenio_rdm_records/services/permissions.py b/invenio_rdm_records/services/permissions.py index c1ef3d013..e682efdd4 100644 --- a/invenio_rdm_records/services/permissions.py +++ b/invenio_rdm_records/services/permissions.py @@ -111,6 +111,8 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): else_=can_read, ) ] + can_read_deleted_files = can_read_deleted + can_media_read_deleted_files = can_read_deleted_files # Allow reading the files of a record can_read_files = [ IfRestricted("files", then_=can_view, else_=can_all), diff --git a/tests/resources/test_files.py b/tests/resources/test_files.py index a4398566b..938eac077 100644 --- a/tests/resources/test_files.py +++ b/tests/resources/test_files.py @@ -23,6 +23,13 @@ def create_draft(client, record, headers): return response.json +def delete_record(client, recid, headers): + """Soft delete a record.""" + response = client.delete(f"/records/{recid}/delete", json={}, headers=headers) + assert response.status_code == 204 + response.close() + + def attach_file(client, recid, key, headers): """Attach a file to a record.""" @@ -108,3 +115,102 @@ def test_published_record_files_deny_edit( # the upload of a file is the one that is affected when we lock the bucket assert response.status_code == 403 logout_user(client) + + +def test_files_api_flow_for_deleted_record( + client, headers, running_app, minimal_record, users, location, superuser +): + login_user(client, users[0]) + + draft = create_draft(client, minimal_record, headers) + recid = draft["id"] + + attach_file(client, recid, "test.pdf", headers) + + # publish the draft + response = client.post(link(draft["links"]["publish"]), headers=headers) + assert response.status_code == 202 + assert response.data + + logout_user(client) + + url = f"/records/{recid}/files" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/files/test.pdf" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/files/test.pdf/content" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/files-archive" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + login_user(client, superuser.user) + + # We delete the record + delete_record(client, recid, headers) + + # Superuser has access to record + url = f"/records/{recid}/files" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/files/test.pdf" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/files/test.pdf/content" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/files-archive" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + logout_user(client) + + # Non superuser users have no access to the record + url = f"/records/{recid}/files" + + response = client.get(url, headers=headers) + assert 410 == response.status_code + assert response.data + + url = f"/records/{recid}/files/test.pdf" + + response = client.get(url, headers=headers) + assert 410 == response.status_code + assert response.data + + url = f"/records/{recid}/files/test.pdf/content" + + response = client.get(url, headers=headers) + assert 410 == response.status_code + assert response.data + + url = f"/records/{recid}/files-archive" + + response = client.get(url, headers=headers) + assert 410 == response.status_code + assert response.data diff --git a/tests/resources/test_media_files.py b/tests/resources/test_media_files.py index bd3f4eb62..65ee0764f 100644 --- a/tests/resources/test_media_files.py +++ b/tests/resources/test_media_files.py @@ -25,6 +25,21 @@ def create_draft(client, record, headers): return response.json["id"] +def publish_draft(client, recid, headers): + """Publish a draft and returns the id.""" + response = client.post(f"/records/{recid}/draft/actions/publish", headers=headers) + assert response.status_code == 202 + assert response.data + return recid + + +def delete_record(client, recid, headers): + """Soft delete a record.""" + response = client.delete(f"/records/{recid}/delete", json={}, headers=headers) + assert response.status_code == 204 + response.close() + + def init_file(client, recid, headers): """Init a file for draft with given recid.""" return client.post( @@ -160,3 +175,98 @@ def test_only_owners_can_list_draft_w_public_files( login_user(client, users[0]) response = client.get(url, headers=headers) assert 200 == response.status_code + + +def test_list_media_files_for_deleted_record( + client, headers, running_app, minimal_record, users, location, superuser +): + login_user(client, users[0]) + + recid = create_draft(client, minimal_record, headers) + init_file(client, recid, headers) + upload_file(client, recid) + # upload file and commit + response = commit_file(client, recid, headers) + assert response.status_code == 200 + assert response.data + publish_draft(client, recid, headers) + logout_user(client) + + url = f"/records/{recid}/media-files" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files/test.pdf" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files/test.pdf/content" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files-archive" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + login_user(client, superuser.user) + + # We delete the record + delete_record(client, recid, headers) + + url = f"/records/{recid}/media-files" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files/test.pdf?include_deleted=1" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files/test.pdf/content" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files-archive?include_deleted=1" + + response = client.get(url, headers=headers) + assert 200 == response.status_code + assert response.data + + logout_user(client) + + url = f"/records/{recid}/media-files" + + response = client.get(url, headers=headers) + assert 410 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files/test.pdf" + + response = client.get(url, headers=headers) + assert 410 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files/test.pdf/content" + + response = client.get(url, headers=headers) + assert 410 == response.status_code + assert response.data + + url = f"/records/{recid}/media-files-archive" + + response = client.get(url, headers=headers) + assert 410 == response.status_code + assert response.data diff --git a/tests/services/files/test_deleted_record_files.py b/tests/services/files/test_deleted_record_files.py new file mode 100644 index 000000000..5cc6ebd6f --- /dev/null +++ b/tests/services/files/test_deleted_record_files.py @@ -0,0 +1,105 @@ +# -*- coding: utf-8 -*- +# +# Copyright (C) 2023 CERN. +# +# Invenio-RDM-Records is free software; you can redistribute it and/or modify +# it under the terms of the MIT License; see LICENSE file for more details. + +"""Test some permissions on deleted record files.""" + +from io import BytesIO + +import pytest +from invenio_records_resources.services.errors import PermissionDeniedError + +from invenio_rdm_records.proxies import current_rdm_records_service +from invenio_rdm_records.services.errors import RecordDeletedException + + +def attach_file(service, user, recid, key): + """Attach a file to a record.""" + service.draft_files.init_files(user, recid, data=[{"key": key}]) + service.draft_files.set_file_content(user, recid, key, BytesIO(b"test file")) + service.draft_files.commit_file(user, recid, key) + + +def test_deleted_records_file_flow( + location, minimal_record, identity_simple, superuser_identity, running_app +): + """Test the lifecycle of a deleted record file .""" + data = minimal_record.copy() + data["files"] = {"enabled": True} + service = current_rdm_records_service + + file_service = service.files + + # Create + draft = service.create(identity_simple, data) + + # Add a file + attach_file(service, identity_simple, draft.id, "test.pdf") + + # Publish + record = service.publish(identity_simple, draft.id) + recid = record["id"] + + # Delete record + service.delete_record(superuser_identity, record["id"], {}) + + # List files + with pytest.raises(RecordDeletedException): + file_service.list_files(identity_simple, recid) + + result = file_service.list_files(superuser_identity, recid) + assert result.to_dict()["entries"][0]["key"] == "test.pdf" + assert result.to_dict()["entries"][0]["storage_class"] == "L" + + # Read file metadata + with pytest.raises(RecordDeletedException): + file_service.read_file_metadata(identity_simple, recid, "test.pdf") + + result = file_service.read_file_metadata(superuser_identity, recid, "test.pdf") + assert result.to_dict()["key"] == "test.pdf" + assert result.to_dict()["storage_class"] == "L" + + # Retrieve file + with pytest.raises(RecordDeletedException): + file_service.get_file_content(identity_simple, recid, "test.pdf") + + result = file_service.get_file_content(superuser_identity, recid, "test.pdf") + assert result.file_id == "test.pdf" + + # Delete file + with pytest.raises(PermissionDeniedError): + file_service.delete_file(identity_simple, recid, "test.pdf") + + recid = record["id"] + file_to_initialise = [ + { + "key": "article.txt", + "checksum": "md5:c785060c866796cc2a1708c997154c8e", + "size": 17, # 2kB + "metadata": { + "description": "Published article PDF.", + }, + } + ] + + # Initialize file saving + with pytest.raises(PermissionDeniedError): + file_service.init_files(identity_simple, recid, file_to_initialise) + + # Update file content + with pytest.raises(PermissionDeniedError): + content = BytesIO(b"test file content") + file_service.set_file_content( + identity_simple, + recid, + file_to_initialise[0]["key"], + content, + content.getbuffer().nbytes, + ) + + # Commit file + with pytest.raises(PermissionDeniedError): + file_service.commit_file(identity_simple, recid, "article.txt")