From 6b0a1cacb8dfd6d42982f699df852aaddb98848c Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 19 Mar 2026 13:25:12 -0400 Subject: [PATCH 1/2] Return 404 on orphaned assets --- dandiapi/api/tests/test_asset.py | 13 +++++++++++++ dandiapi/api/views/asset.py | 6 +++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index a23b7565a..59bfe0214 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -1840,6 +1840,9 @@ def test_asset_direct_download_head(api_client, version, asset): @pytest.mark.django_db def test_asset_direct_metadata(api_client, asset): + draft_version = DraftVersionFactory.create() + draft_version.assets.add(asset) + assert ( json.loads(api_client.get(f'/api/assets/{asset.asset_id}/').content) == asset.full_metadata ) @@ -1847,6 +1850,9 @@ def test_asset_direct_metadata(api_client, asset): @pytest.mark.django_db def test_asset_direct_info(api_client, asset): + draft_version = DraftVersionFactory.create() + draft_version.assets.add(asset) + assert api_client.get(f'/api/assets/{asset.asset_id}/info/').json() == { 'asset_id': str(asset.asset_id), 'blob': str(asset.blob.blob_id), @@ -1859,6 +1865,13 @@ def test_asset_direct_info(api_client, asset): } +@pytest.mark.django_db +def test_asset_direct_orphaned(api_client, asset): + assert api_client.get(f'/api/assets/{asset.asset_id}/').status_code == 404 + assert api_client.get(f'/api/assets/{asset.asset_id}/info/').status_code == 404 + assert api_client.get(f'/api/assets/{asset.asset_id}/download/').status_code == 404 + + @pytest.mark.django_db @pytest.mark.parametrize( ('glob_pattern', 'expected_paths'), diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index de3a2350f..4d085ea0b 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -80,7 +80,11 @@ def raise_if_unauthorized(self): if asset_id is None: return - asset = get_object_or_404(Asset.objects.select_related('blob', 'zarr'), asset_id=asset_id) + asset = get_object_or_404( + # Filter out assets that don't have any versions (orphaned assets) + Asset.objects.filter(versions__isnull=False).select_related('blob', 'zarr'), + asset_id=asset_id, + ) if not asset.is_embargoed: return From 1cfd65b269499c9ff343cb1630acc7420ef83c7a Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 19 Mar 2026 16:38:52 -0400 Subject: [PATCH 2/2] Throw exception if embargoed asset is missing embargo end date --- dandiapi/api/models/asset.py | 15 +++++++++------ dandiapi/api/tests/test_asset.py | 12 ++++++------ .../api/tests/test_asset_access_metadata.py | 17 +++++++++++++---- dandiapi/api/tests/test_unembargo.py | 14 +++++++------- dandiapi/api/tests/test_version.py | 8 ++++---- 5 files changed, 39 insertions(+), 27 deletions(-) diff --git a/dandiapi/api/models/asset.py b/dandiapi/api/models/asset.py index 79dd9fd25..6d74c1523 100644 --- a/dandiapi/api/models/asset.py +++ b/dandiapi/api/models/asset.py @@ -27,6 +27,10 @@ class EmbargoedAssetWithinOpenDandisetError(Exception): """Raised when an embargoed asset exists in an open dandiset.""" +class EmbargoedAssetWithoutEndDateError(Exception): + """Raised when an embargoed asset exists without an embargo end date.""" + + ASSET_CHARS_REGEX = r'[A-z0-9(),&\s#+~_=-]' ASSET_PATH_REGEX = rf'^({ASSET_CHARS_REGEX}?\/?\.?{ASSET_CHARS_REGEX})+$' ASSET_COMPUTED_FIELDS = [ @@ -281,13 +285,12 @@ def access_metadata(self): 'min_embargo_end_date' ] - # The only way embargo_end_date can be None here is if asset isn't associated with any - # versions (most likely due to being updated). Even so, sometimes these assets are accessed - # directly, so we need to handle that case. - # TODO: Update once https://github.com/dandi/dandi-archive/issues/2733 is addressed - if embargo_end_date is not None: - access['embargoedUntil'] = embargo_end_date.isoformat() + # The only way embargo_end_date can be None here is if asset has been orphaned. Access to + # these assets is prohibited, so if this occurs, it's an error. + if embargo_end_date is None: + raise EmbargoedAssetWithoutEndDateError + access['embargoedUntil'] = embargo_end_date.isoformat() return access @property diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index 59bfe0214..35cc3e802 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -486,7 +486,7 @@ def test_nested_asset_ordering_with_authenticated_user(api_client, asset_factory @pytest.mark.django_db def test_nested_asset_ordering_with_embargoed_assets( - api_client, asset_factory, embargoed_asset_blob + api_client, draft_asset_factory, embargoed_asset_blob ): """Test that ordering works with embargoed assets.""" from dandiapi.api.models.dandiset import Dandiset @@ -498,9 +498,9 @@ def test_nested_asset_ordering_with_embargoed_assets( api_client.force_authenticate(user=user) # Create assets with different paths - asset1 = asset_factory(path='a_first.txt', blob=embargoed_asset_blob) - asset2 = asset_factory(path='c_last.txt', blob=embargoed_asset_blob) - asset3 = asset_factory(path='b_middle.txt', blob=embargoed_asset_blob) + asset1 = draft_asset_factory(path='a_first.txt', blob=embargoed_asset_blob) + asset2 = draft_asset_factory(path='c_last.txt', blob=embargoed_asset_blob) + asset3 = draft_asset_factory(path='b_middle.txt', blob=embargoed_asset_blob) draft_version.assets.add(asset1) draft_version.assets.add(asset2) @@ -1744,7 +1744,7 @@ def test_asset_download(api_client, version, asset): @pytest.mark.django_db def test_asset_download_embargo( api_client, - asset_factory, + draft_asset_factory, embargoed_asset_blob, ): user = UserFactory.create() @@ -1753,7 +1753,7 @@ def test_asset_download_embargo( dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED, dandiset__owners=[user] ) # Generate assets and blobs - asset = asset_factory(blob=embargoed_asset_blob) + asset = draft_asset_factory(blob=embargoed_asset_blob) version.assets.add(asset) response = api_client.get( diff --git a/dandiapi/api/tests/test_asset_access_metadata.py b/dandiapi/api/tests/test_asset_access_metadata.py index ea39fa83c..aa89284f6 100644 --- a/dandiapi/api/tests/test_asset_access_metadata.py +++ b/dandiapi/api/tests/test_asset_access_metadata.py @@ -25,19 +25,28 @@ def test_asset_full_metadata_access( 'foo': 'bar', 'schemaVersion': DANDI_SCHEMA_VERSION, } + + # Embargoed embargoed_zarr_asset: Asset = draft_asset_factory( metadata=raw_metadata, blob=None, zarr=embargoed_zarr_archive_factory() ) - open_zarr_asset: Asset = draft_asset_factory( - metadata=raw_metadata, blob=None, zarr=zarr_archive_factory() - ) - embargoed_blob_asset: Asset = draft_asset_factory( metadata=raw_metadata, blob=asset_blob_factory(embargoed=True), zarr=None ) + embargoed_draft_version = DraftVersionFactory.create( + dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED + ) + embargoed_draft_version.assets.add(embargoed_zarr_asset, embargoed_blob_asset) + + # Open + open_zarr_asset: Asset = draft_asset_factory( + metadata=raw_metadata, blob=None, zarr=zarr_archive_factory() + ) open_blob_asset: Asset = draft_asset_factory( metadata=raw_metadata, blob=asset_blob_factory(embargoed=False), zarr=None ) + open_draft_version = DraftVersionFactory.create() + open_draft_version.assets.add(open_zarr_asset, open_blob_asset) # Test that access is correctly inferred from embargo status for embargoed_asset in [embargoed_zarr_asset, embargoed_blob_asset]: diff --git a/dandiapi/api/tests/test_unembargo.py b/dandiapi/api/tests/test_unembargo.py index 1d789b57c..3a3fa3883 100644 --- a/dandiapi/api/tests/test_unembargo.py +++ b/dandiapi/api/tests/test_unembargo.py @@ -126,7 +126,7 @@ def test_unembargo_dandiset_uploads_exist(upload_factory, api_client): @pytest.mark.django_db def test_remove_dandiset_embargo_tags_chunks( - asset_factory, + draft_asset_factory, embargoed_asset_blob_factory, mocker, ): @@ -140,7 +140,7 @@ def test_remove_dandiset_embargo_tags_chunks( ) ds: Dandiset = draft_version.dandiset for _ in range(chunk_size + 1): - asset = asset_factory(blob=embargoed_asset_blob_factory()) + asset = draft_asset_factory(blob=embargoed_asset_blob_factory()) draft_version.assets.add(asset) remove_dandiset_embargo_tags(dandiset=ds) @@ -152,7 +152,7 @@ def test_remove_dandiset_embargo_tags_chunks( @pytest.mark.django_db def test_remove_dandiset_embargo_tags_fails_remove_tags( - asset_factory, + draft_asset_factory, embargoed_asset_blob_factory, mocker, ): @@ -165,7 +165,7 @@ def test_remove_dandiset_embargo_tags_fails_remove_tags( ) ds: Dandiset = draft_version.dandiset for _ in range(2): - asset = asset_factory(blob=embargoed_asset_blob_factory()) + asset = draft_asset_factory(blob=embargoed_asset_blob_factory()) draft_version.assets.add(asset) # Remove tags @@ -226,7 +226,7 @@ def test_remove_dandiset_manifest_tags(): @pytest.mark.django_db def test_unembargo_dandiset( - asset_factory, + draft_asset_factory, embargoed_asset_blob_factory, embargoed_zarr_archive_factory, zarr_file_factory, @@ -240,7 +240,7 @@ def test_unembargo_dandiset( dandiset: Dandiset = draft_version.dandiset embargoed_blob: AssetBlob = embargoed_asset_blob_factory() - blob_asset = asset_factory(blob=embargoed_blob, status=Asset.Status.VALID) + blob_asset = draft_asset_factory(blob=embargoed_blob, status=Asset.Status.VALID) draft_version.assets.add(blob_asset) zarr_archive: ZarrArchive = embargoed_zarr_archive_factory( @@ -251,7 +251,7 @@ def test_unembargo_dandiset( ] ingest_zarr_archive(zarr_id=zarr_archive.zarr_id) zarr_archive.refresh_from_db() - zarr_asset = asset_factory(zarr=zarr_archive, blob=None, status=Asset.Status.VALID) + zarr_asset = draft_asset_factory(zarr=zarr_archive, blob=None, status=Asset.Status.VALID) draft_version.assets.add(zarr_asset) write_manifest_files(draft_version.id) diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index 69e2b09d6..847db934e 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -407,14 +407,14 @@ def test_version_aggregate_assets_summary_metadata_modified(draft_asset_factory) @pytest.mark.django_db def test_version_size( version, - asset_factory, + draft_asset_factory, asset_blob_factory, embargoed_asset_blob_factory, zarr_archive_factory, ): - version.assets.add(asset_factory(blob=asset_blob_factory(size=100))) - version.assets.add(asset_factory(blob=embargoed_asset_blob_factory(size=200))) - version.assets.add(asset_factory(blob=None, zarr=zarr_archive_factory(size=400))) + version.assets.add(draft_asset_factory(blob=asset_blob_factory(size=100))) + version.assets.add(draft_asset_factory(blob=embargoed_asset_blob_factory(size=200))) + version.assets.add(draft_asset_factory(blob=None, zarr=zarr_archive_factory(size=400))) add_version_asset_paths(version=version) assert version.size == 700