diff --git a/core/common/mixins.py b/core/common/mixins.py index 3feb5291e..4b18afa10 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -1058,7 +1058,9 @@ def get(self, request, *args, **kwargs): # pylint: disable=unused-argument if version.has_export(): export_path = version.get_export_path() - export_url = get_export_service().url_for(export_path) + download_name = version.get_export_download_name(export_path) + logger.info('Export %s downloads as %s', export_path, download_name) + export_url = get_export_service().url_for(export_path, filename=download_name) if export_url: return redirect(export_url) logger.error( diff --git a/core/common/models.py b/core/common/models.py index bdbd6a45b..ea96027d7 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -1,4 +1,5 @@ import logging +import re import time from celery.result import AsyncResult @@ -1400,6 +1401,30 @@ def get_export_path(self): service = get_export_service() return service.get_last_key_from_path(self.get_version_export_path(suffix=None)) or self.version_export_path + def get_export_download_name(self, export_path): + """ + Name a client saves the export under, set on the signed URL: + [owner-type]_[owner]_[repo-type]_[repo]_[repo-version]_[expansion]_[lastUpdated].zip + lastUpdated is read from the storage key, so the name matches the export's content. A key without one + right after this version's prefix gets no name (None), and the download keeps its storage name. Characters + that aren't safe in a filename become '-'. + """ + # current (2026-09-30_123456) or legacy (20260930123456) key timestamp, anchored to the prefix because a key + # listed under it can be another version's (1.8.24.1's under 1.8.24's) + match = re.fullmatch( + re.escape(self.get_version_export_path(suffix=None)) + r'(\d{4})-?(\d{2})-?(\d{2})_?(\d{6})\.zip', + export_path) + if not match: + return None + + parts = (get(self, 'expansion_uri') or self.uri).strip('/').split('/') + # the uri percent-encodes the version, HEAD's uri has none and an expansion uri follows it with 'expansions', + # so the version comes from the model + parts[4:6] = [self.version] + year, month, day, time_of_day = match.groups() + parts.append(f'{year}-{month}-{day}_{time_of_day}') + return '_'.join(re.sub(r'[^A-Za-z0-9._@-]+', '-', part) for part in parts) + '.zip' + def has_export(self): service = get_export_service() if self.is_head: diff --git a/core/integration_tests/tests_collections.py b/core/integration_tests/tests_collections.py index a8e7d6f8e..71e9fcdae 100644 --- a/core/integration_tests/tests_collections.py +++ b/core/integration_tests/tests_collections.py @@ -2700,7 +2700,10 @@ def test_get_302_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, self.assertEqual(response['Location'], 'https://signed.example/coll-v1.zip') s3_has_path_mock.assert_called_once_with("users/username/username_coll_v1.") s3_get_last_key_from_path_mock.assert_called_once_with("users/username/username_coll_v1.") - s3_url_for_mock.assert_called_once_with(f'users/username/username_coll_v1.{self.v1_updated_at}.zip') + s3_url_for_mock.assert_called_once_with( + f'users/username/username_coll_v1.{self.v1_updated_at}.zip', + filename=f'users_username_collections_coll_v1_{self.v1_updated_at}.zip' + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.exists') @@ -2717,7 +2720,10 @@ def test_get_302_head(self, s3_exists_mock, s3_url_for_mock): self.assertEqual(response.status_code, 302) self.assertEqual(response['Location'], 'https://signed.example/coll-head.zip') s3_exists_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip") - s3_url_for_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip") + s3_url_for_mock.assert_called_once_with( + f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip", + filename=f"users_username_collections_coll_HEAD_{self.HEAD_updated_at}.zip" + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.exists') @@ -2734,7 +2740,10 @@ def test_get_500_head_when_signed_url_generation_fails(self, s3_exists_mock, s3_ self.assertEqual(response.status_code, 500) self.assertEqual(response.data, {'detail': 'Export exists but could not generate a download URL.'}) s3_exists_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip") - s3_url_for_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip") + s3_url_for_mock.assert_called_once_with( + f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip", + filename=f"users_username_collections_coll_HEAD_{self.HEAD_updated_at}.zip" + ) def test_get_405(self): random_user = UserProfileFactory() diff --git a/core/integration_tests/tests_sources.py b/core/integration_tests/tests_sources.py index b7343468b..82b21a13f 100644 --- a/core/integration_tests/tests_sources.py +++ b/core/integration_tests/tests_sources.py @@ -1169,7 +1169,10 @@ def test_get_302_head(self, s3_exists_mock, s3_url_for_mock): self.assertEqual(response.status_code, 302) self.assertEqual(response['Location'], 'https://signed.example/head.zip') s3_exists_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip") - s3_url_for_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip") + s3_url_for_mock.assert_called_once_with( + f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip", + filename=f"users_username_sources_source1_HEAD_{self.HEAD_updated_at}.zip" + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.get_last_key_from_path') @@ -1179,17 +1182,26 @@ def test_get_302_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, s3_has_path_mock.return_value = True s3_get_last_key_from_path_mock.return_value = f'users/username/username_source1_v1.{self.v1_updated_at}.zip' - response = self.client.get( - self.source_v1.uri + 'export/', - HTTP_AUTHORIZATION='Token ' + self.token, - format='json' - ) + with self.assertLogs('oclapi', level='INFO') as logs: + response = self.client.get( + self.source_v1.uri + 'export/', + HTTP_AUTHORIZATION='Token ' + self.token, + format='json' + ) self.assertEqual(response.status_code, 302) self.assertEqual(response['Location'], 'https://signed.example/v1.zip') s3_has_path_mock.assert_called_once_with("users/username/username_source1_v1.") s3_get_last_key_from_path_mock.assert_called_once_with("users/username/username_source1_v1.") - s3_url_for_mock.assert_called_once_with(f'users/username/username_source1_v1.{self.v1_updated_at}.zip') + s3_url_for_mock.assert_called_once_with( + f'users/username/username_source1_v1.{self.v1_updated_at}.zip', + filename=f'users_username_sources_source1_v1_{self.v1_updated_at}.zip' + ) + self.assertIn( + f'INFO:oclapi:Export users/username/username_source1_v1.{self.v1_updated_at}.zip downloads as ' + f'users_username_sources_source1_v1_{self.v1_updated_at}.zip', + logs.output + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.exists') @@ -1206,7 +1218,10 @@ def test_get_500_head_when_signed_url_generation_fails(self, s3_exists_mock, s3_ self.assertEqual(response.status_code, 500) self.assertEqual(response.data, {'detail': 'Export exists but could not generate a download URL.'}) s3_exists_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip") - s3_url_for_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip") + s3_url_for_mock.assert_called_once_with( + f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip", + filename=f"users_username_sources_source1_HEAD_{self.HEAD_updated_at}.zip" + ) @patch('core.sources.models.Source.is_exporting', new_callable=PropertyMock) @patch('core.services.storages.cloud.aws.S3.exists') diff --git a/core/repos/tests.py b/core/repos/tests.py index 3a222fbc9..98b25d498 100644 --- a/core/repos/tests.py +++ b/core/repos/tests.py @@ -7,7 +7,7 @@ from core.collections.documents import CollectionDocument from core.collections.models import Collection -from core.collections.tests.factories import OrganizationCollectionFactory, UserCollectionFactory +from core.collections.tests.factories import OrganizationCollectionFactory, UserCollectionFactory, ExpansionFactory from core.common.constants import HEAD from core.common.tests import OCLAPITestCase, OCLTestCase from core.orgs.tests.factories import OrganizationFactory @@ -115,6 +115,108 @@ def test_get_external_export_path_normalizes_invalid_filename_characters(self): ) +class RepoExportDownloadNameTest(OCLTestCase): + def test_org_source_version(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.8.24') + export_path = source_v1.get_version_export_path(suffix='2026-09-30_123456.zip') + + self.assertEqual(export_path, 'orgs/PIH/PIH_PIH_v1.8.24.2026-09-30_123456.zip') + self.assertEqual( + source_v1.get_export_download_name(export_path), 'orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip') + + def test_user_collection_version_with_expansion(self): + collection_v1 = UserCollectionFactory( + user=UserProfileFactory(username='jdoe'), mnemonic='Concepts', version='1.0') + expansion = ExpansionFactory(collection_version=collection_v1, mnemonic='autoexpand-1.0') + collection_v1.expansion_uri = expansion.uri + collection_v1.save() + export_path = collection_v1.get_version_export_path(suffix='2026-09-30_123456.zip') + + self.assertEqual( + collection_v1.get_export_download_name(export_path), + 'users_jdoe_collections_Concepts_1.0_autoexpand-1.0_2026-09-30_123456.zip' + ) + + def test_source_head(self): + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH') + last_update = source.last_child_update.strftime('%Y-%m-%d_%H%M%S') + + self.assertEqual( + source.get_export_download_name(source.version_export_path), + f'orgs_PIH_sources_PIH_HEAD_{last_update}.zip' + ) + + def test_collection_head_with_expansion(self): + collection = OrganizationCollectionFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIHEMR_Concepts') + expansion = ExpansionFactory(collection_version=collection, mnemonic='autoexpand-HEAD') + collection.expansion_uri = expansion.uri + collection.save() + last_update = collection.last_child_update.strftime('%Y-%m-%d_%H%M%S') + + self.assertEqual( + collection.get_export_download_name(collection.version_export_path), + f'orgs_PIH_collections_PIHEMR_Concepts_HEAD_autoexpand-HEAD_{last_update}.zip' + ) + + def test_legacy_key_timestamp(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='CIEL'), mnemonic='CIEL', version='v2016-08-22') + + self.assertEqual( + source_v1.get_export_download_name('orgs/CIEL/CIEL_CIEL_v2016-08-22.20150516122820.zip'), + 'orgs_CIEL_sources_CIEL_v2016-08-22_2015-05-16_122820.zip' + ) + + def test_key_without_timestamp_gets_no_name(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.8.24') + + self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.8.24.zip')) + + def test_date_like_version(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.20260930123456') + + self.assertEqual( + source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.20260930123456.2026-10-01_090000.zip'), + 'orgs_PIH_sources_PIH_1.20260930123456_2026-10-01_090000.zip' + ) + self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.20260930123456.zip')) + self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.20260930123456.1.zip')) + + def test_key_of_another_version_gets_no_name(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.8.24') + + # get_export_path() lists keys by prefix, so 1.8.24's lookup can return 1.8.24.1's export + self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.8.24.1.2026-10-01_090000.zip')) + + def test_mnemonics_named_expansions(self): + collection_v1 = OrganizationCollectionFactory( + organization=OrganizationFactory(mnemonic='expansions'), mnemonic='expansions', version='1.0') + expansion = ExpansionFactory(collection_version=collection_v1, mnemonic='expansions') + collection_v1.expansion_uri = expansion.uri + collection_v1.save() + export_path = collection_v1.get_version_export_path(suffix='2026-09-30_123456.zip') + + self.assertEqual( + collection_v1.get_export_download_name(export_path), + 'orgs_expansions_collections_expansions_1.0_expansions_2026-09-30_123456.zip' + ) + + def test_unsafe_version_characters_become_hyphens(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.0 beta: [draft] "x";100%') + export_path = source_v1.get_version_export_path(suffix='2026-09-30_123456.zip') + + self.assertEqual( + source_v1.get_export_download_name(export_path), + 'orgs_PIH_sources_PIH_1.0-beta-draft-x-100-_2026-09-30_123456.zip' + ) + + class UserOrganizationRepoListViewTest(OCLAPITestCase): def test_get(self): CollectionDocument._index.delete() # pylint: disable=protected-access diff --git a/core/services/storages/cloud/aws.py b/core/services/storages/cloud/aws.py index b89a35364..2b0f592e4 100644 --- a/core/services/storages/cloud/aws.py +++ b/core/services/storages/cloud/aws.py @@ -5,6 +5,7 @@ from botocore.exceptions import ClientError, NoCredentialsError, WaiterError from django.conf import settings from django.core.files.base import ContentFile +from django.utils.http import content_disposition_header from pydash import get from core.services.storages.cloud.core import CloudStorageServiceInterface @@ -64,8 +65,9 @@ def upload_base64( # pylint: disable=too-many-arguments,inconsistent-return-sta return file_name_with_ext - def url_for(self, file_path): - return self._generate_signed_url(self.GET, file_path) if file_path else None + def url_for(self, file_path, filename=None): + metadata = {'ResponseContentDisposition': content_disposition_header(True, filename)} if filename else None + return self._generate_signed_url(self.GET, file_path, metadata) if file_path else None def public_url_for(self, file_path): url = f"http://{settings.AWS_STORAGE_BUCKET_NAME}.s3.amazonaws.com/{file_path}" diff --git a/core/services/storages/cloud/azure.py b/core/services/storages/cloud/azure.py index 524df1d4e..f60792e17 100644 --- a/core/services/storages/cloud/azure.py +++ b/core/services/storages/cloud/azure.py @@ -19,7 +19,8 @@ def __init__(self): def public_url_for(self, file_path): return f"https://{self.account_name}.blob.core.windows.net/{self.container_name}/{file_path}" - def url_for(self, file_path): + def url_for(self, file_path, filename=None): # pylint: disable=unused-argument + # an unsigned blob URL can't carry a download name, so filename is ignored return self.public_url_for(file_path) def exists(self, key): diff --git a/core/services/storages/cloud/core.py b/core/services/storages/cloud/core.py index 22dd0edc1..c467a497e 100644 --- a/core/services/storages/cloud/core.py +++ b/core/services/storages/cloud/core.py @@ -21,9 +21,9 @@ def upload_base64(self, doc_base64, file_name, append_extension=True, public_rea Uploads base64 file content to file_name """ - def url_for(self, file_path): + def url_for(self, file_path, filename=None): """ - Returns signed url for file_path + Returns signed url for file_path; filename, if given, is the download name the response carries """ def public_url_for(self, file_path): diff --git a/core/services/storages/cloud/minio.py b/core/services/storages/cloud/minio.py index 6a278874e..f41c75e0a 100644 --- a/core/services/storages/cloud/minio.py +++ b/core/services/storages/cloud/minio.py @@ -2,6 +2,7 @@ import mimetypes from io import BytesIO +from django.utils.http import content_disposition_header from minio import Minio, S3Error from minio.deleteobjects import DeleteObject from pydash import get @@ -85,15 +86,19 @@ def upload_base64(self, doc_base64, file_name, append_extension=True, public_rea except S3Error as e: raise Exception(f"Could not upload base64 file {file_name_with_ext} to MinIO. Error: {e}") from e # pylint: disable=broad-exception-raised - def url_for(self, file_path): + def url_for(self, file_path, filename=None): """ Generates a presigned URL for the given file using the external client so the signature is computed with the publicly accessible host (MINIO_EXTERNAL_ENDPOINT). Falls back to the internal client when no external endpoint is configured. + filename, if given, is the download name the response carries. """ + response_headers = { + 'response-content-disposition': content_disposition_header(True, filename) + } if filename else None try: return self._presign_client.get_presigned_url( - method='GET', bucket_name=self.bucket_name, object_name=file_path + method='GET', bucket_name=self.bucket_name, object_name=file_path, response_headers=response_headers ) if file_path else None except S3Error as e: raise Exception(f"Could not generate presigned URL for file {file_path}. Error: {e}") from e # pylint: disable=broad-exception-raised diff --git a/core/services/storages/cloud/tests.py b/core/services/storages/cloud/tests.py index 255a12218..2228229db 100644 --- a/core/services/storages/cloud/tests.py +++ b/core/services/storages/cloud/tests.py @@ -189,6 +189,17 @@ def test_url_for(self): 'X-Amz-Expires=' in _url ) + @mock_aws + def test_url_for_with_filename(self): + _url = S3().url_for('some/path', filename='orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip') + + self.assertIn( + 'response-content-disposition=attachment%3B%20filename%3D%22' + 'orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip%22', + _url + ) + self.assertIn('&X-Amz-Signature=', _url) + def test_public_url_for(self): self.assertEqual( S3().public_url_for('some/path').replace('https://', 'http://'), @@ -451,7 +462,23 @@ def test_url_for(self, mock_minio_client): # Assert that the URL is generated correctly self.assertEqual(url, "http://mock_url") mock_client.get_presigned_url.assert_called_once_with(method='GET', bucket_name="bucket", - object_name=file_key) + object_name=file_key, response_headers=None) + + @patch("core.services.storages.cloud.minio.Minio") + def test_url_for_with_filename(self, mock_minio_client): + mock_client = mock_minio_client.return_value + mock_client.get_presigned_url = MagicMock(return_value="http://mock_url") + + url = MinIO().url_for('test-file.zip', filename='orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip') + + self.assertEqual(url, "http://mock_url") + mock_client.get_presigned_url.assert_called_once_with( + method='GET', bucket_name="bucket", object_name='test-file.zip', + response_headers={ + 'response-content-disposition': + 'attachment; filename="orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip"' + } + ) @patch("core.services.storages.cloud.minio.Minio", Mock()) def test_public_url_for(self):