Merge pull request #1954 from dandi/dont-allow-set-embargo-status

Restrict updates to metadata `access` field

dandiapi/api/models/dandiset.py

@@ -58,6 +58,10 @@ def identifier(self) -> str:
         # Compare against None, to allow id 0
         return f'{self.id:06}' if self.id is not None else ''
 
+    @property
+    def embargoed(self) -> bool:
+        return self.embargo_status == self.EmbargoStatus.EMBARGOED
+
     @property
     def most_recent_published_version(self):
         return self.versions.exclude(version='draft').order_by('modified').last()

dandiapi/api/models/version.py

@@ -3,6 +3,7 @@
 import datetime
 import logging
 
+from dandischema.models import AccessType
 from django.conf import settings
 from django.contrib.postgres.indexes import HashIndex
 from django.core.validators import RegexValidator
@@ -165,7 +166,41 @@ def strip_metadata(cls, metadata):
             'publishedBy',
             'manifestLocation',
         ]
-        return {key: metadata[key] for key in metadata if key not in computed_fields}
+        stripped = {key: metadata[key] for key in metadata if key not in computed_fields}
+
+        # Strip the status and schemaKey fields, as modifying them is not supported
+        if (
+            'access' in stripped
+            and isinstance(stripped['access'], list)
+            and len(stripped['access'])
+            and isinstance(stripped['access'][0], dict)
+        ):
+            stripped['access'][0].pop('schemaKey', None)
+            stripped['access'][0].pop('status', None)
+
+        return stripped
+
+    def _populate_access_metadata(self):
+        default_access = [{}]
+        access = self.metadata.get('access', default_access)
+
+        # Ensure access is a non-empty list
+        if not (isinstance(access, list) and access):
+            access = default_access
+
+        # Ensure that every item in access is a dict
+        access = [x for x in access if isinstance(x, dict)] or default_access
+
+        # Set first access item
+        access[0] = {
+            **access[0],
+            'schemaKey': 'AccessRequirements',
+            'status': AccessType.EmbargoedAccess.value
+            if self.dandiset.embargoed
+            else AccessType.OpenAccess.value,
+        }
+
+        return access
 
     def _populate_metadata(self):
         from dandiapi.api.manifests import manifest_location
@@ -187,6 +222,7 @@ def _populate_metadata(self):
                 f'{self.dandiset.identifier}/{self.version}'
             ),
             'dateCreated': self.dandiset.created.isoformat(),
+            'access': self._populate_access_metadata(),
         }
 
         if 'assetsSummary' not in metadata:

dandiapi/api/services/embargo/__init__.py

@@ -48,12 +48,6 @@ def _unembargo_dandiset(dandiset: Dandiset):
     embargoed_assets: QuerySet[Asset] = draft_version.assets.filter(blob__embargoed=True)
     AssetBlob.objects.filter(assets__in=embargoed_assets).update(embargoed=False)
 
-    # Update draft version metadata
-    draft_version.metadata['access'] = [
-        {'schemaKey': 'AccessRequirements', 'status': 'dandi:OpenAccess'}
-    ]
-    draft_version.save()
-
     # Set access on dandiset
     dandiset.embargo_status = Dandiset.EmbargoStatus.OPEN
     dandiset.save()

dandiapi/api/services/version/metadata.py

@@ -32,13 +32,6 @@ def _normalize_version_metadata(
                 'includeInCitation': True,
             },
         ],
-        # TODO: move this into dandischema
-        'access': [
-            {
-                'schemaKey': 'AccessRequirements',
-                'status': 'dandi:EmbargoedAccess' if embargo else 'dandi:OpenAccess',
-            }
-        ],
         **version_metadata,
     }
     # Run the version_metadata through the pydantic model to automatically include any boilerplate

dandiapi/api/tests/factories.py

@@ -5,6 +5,7 @@
 
 from allauth.socialaccount.models import SocialAccount
 from dandischema.digests.dandietag import DandiETag
+from dandischema.models import AccessType
 from django.conf import settings
 from django.contrib.auth.models import User
 from django.core import files as django_files
@@ -95,6 +96,14 @@ def metadata(self):
             'schemaVersion': settings.DANDI_SCHEMA_VERSION,
             'schemaKey': 'Dandiset',
             'description': faker.Faker().sentence(),
+            'access': [
+                {
+                    'schemaKey': 'AccessRequirements',
+                    'status': AccessType.EmbargoedAccess.value
+                    if self.dandiset.embargoed
+                    else AccessType.OpenAccess.value,
+                }
+            ],
             'contributor': [
                 {
                     'name': f'{faker.Faker().last_name()}, {faker.Faker().first_name()}',

dandiapi/api/tests/test_version.py

@@ -4,6 +4,7 @@
 from time import sleep
 from typing import TYPE_CHECKING
 
+from dandischema.models import AccessType
 from django.conf import settings
 from freezegun import freeze_time
 from guardian.shortcuts import assign_perm
@@ -84,6 +85,7 @@ def test_draft_version_metadata_computed(draft_version: Version):
         ),
         'repository': settings.DANDI_WEB_APP_URL,
         'dateCreated': draft_version.dandiset.created.isoformat(),
+        'access': [{'schemaKey': 'AccessRequirements', 'status': AccessType.OpenAccess.value}],
         '@context': f'https://raw.githubusercontent.com/dandi/schema/master/releases/{settings.DANDI_SCHEMA_VERSION}/context.json',
         'assetsSummary': {
             'numberOfBytes': 0,
@@ -127,6 +129,7 @@ def test_published_version_metadata_computed(published_version: Version):
         ),
         'repository': settings.DANDI_WEB_APP_URL,
         'dateCreated': published_version.dandiset.created.isoformat(),
+        'access': [{'schemaKey': 'AccessRequirements', 'status': AccessType.OpenAccess.value}],
         '@context': f'https://raw.githubusercontent.com/dandi/schema/master/releases/{settings.DANDI_SCHEMA_VERSION}/context.json',
         'assetsSummary': {
             'numberOfBytes': 0,
@@ -556,6 +559,7 @@ def test_version_rest_update(api_client, user, draft_version):
         'url': url,
         'repository': settings.DANDI_WEB_APP_URL,
         'dateCreated': UTC_ISO_TIMESTAMP_RE,
+        'access': [{'schemaKey': 'AccessRequirements', 'status': AccessType.OpenAccess.value}],
         'citation': f'{new_name} ({year}). (Version draft) [Data set]. DANDI archive. {url}',
         'assetsSummary': {
             'numberOfBytes': 0,
@@ -635,6 +639,94 @@ def test_version_rest_update_not_an_owner(api_client, user, version):
     )
 
 
+@pytest.mark.parametrize(
+    ('access'),
+    [
+        'some value',
+        123,
+        None,
+        [],
+        ['a', 'b'],
+        ['a', 'b', {}],
+        [{'schemaKey': 'AccessRequirements', 'status': 'foobar'}],
+    ],
+)
+@pytest.mark.django_db()
+def test_version_rest_update_access_values(api_client, user, draft_version, access):
+    assign_perm('owner', user, draft_version.dandiset)
+    api_client.force_authenticate(user=user)
+
+    new_metadata = {**draft_version.metadata, 'access': access}
+    resp = api_client.put(
+        f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/',
+        {'metadata': new_metadata, 'name': draft_version.name},
+        format='json',
+    )
+    assert resp.status_code == 200
+    draft_version.refresh_from_db()
+
+    access = draft_version.metadata['access']
+    assert access != new_metadata['access']
+    assert access[0]['schemaKey'] == 'AccessRequirements'
+    assert (
+        access[0]['status'] == AccessType.EmbargoedAccess.value
+        if draft_version.dandiset.embargoed
+        else AccessType.OpenAccess.value
+    )
+
+
+@pytest.mark.django_db()
+def test_version_rest_update_access_missing(api_client, user, draft_version):
+    assign_perm('owner', user, draft_version.dandiset)
+    api_client.force_authenticate(user=user)
+
+    # Check that the field missing entirely is also okay
+    new_metadata = {**draft_version.metadata}
+    new_metadata.pop('access', None)
+
+    resp = api_client.put(
+        f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/',
+        {'metadata': new_metadata, 'name': draft_version.name},
+        format='json',
+    )
+    assert resp.status_code == 200
+    draft_version.refresh_from_db()
+    assert 'access' in draft_version.metadata
+    access = draft_version.metadata['access']
+    assert access[0]['schemaKey'] == 'AccessRequirements'
+    assert (
+        access[0]['status'] == AccessType.EmbargoedAccess.value
+        if draft_version.dandiset.embargoed
+        else AccessType.OpenAccess.value
+    )
+
+
+@pytest.mark.django_db()
+def test_version_rest_update_access_valid(api_client, user, draft_version):
+    assign_perm('owner', user, draft_version.dandiset)
+    api_client.force_authenticate(user=user)
+
+    # Check that extra fields persist
+    new_metadata = {**draft_version.metadata, 'access': [{'extra': 'field'}]}
+    resp = api_client.put(
+        f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/',
+        {'metadata': new_metadata, 'name': draft_version.name},
+        format='json',
+    )
+    assert resp.status_code == 200
+    draft_version.refresh_from_db()
+    access = draft_version.metadata['access']
+    assert access != new_metadata['access']
+    assert access[0]['schemaKey'] == 'AccessRequirements'
+    assert (
+        access[0]['status'] == AccessType.EmbargoedAccess.value
+        if draft_version.dandiset.embargoed
+        else AccessType.OpenAccess.value
+    )
+
+    assert access[0]['extra'] == 'field'
+
+
 @pytest.mark.django_db()
 def test_version_rest_publish(
     api_client: APIClient,