Return 404 on orphaned assets

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

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(
@@ -1840,13 +1840,19 @@ 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
     )
 
 
 @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'),

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]:

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)

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

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