From 5b92f2c374627da2ea6d05a4837e887969ffb7b1 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 10:57:42 -0500 Subject: [PATCH 01/37] Add AuditRecord model --- dandiapi/api/models/__init__.py | 2 ++ dandiapi/api/models/audit.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 dandiapi/api/models/audit.py diff --git a/dandiapi/api/models/__init__.py b/dandiapi/api/models/__init__.py index fdc3bd2b5..ff0c21c61 100644 --- a/dandiapi/api/models/__init__.py +++ b/dandiapi/api/models/__init__.py @@ -2,6 +2,7 @@ from .asset import Asset, AssetBlob, EmbargoedAssetBlob from .asset_paths import AssetPath, AssetPathRelation +from .audit import AuditRecord from .dandiset import Dandiset from .oauth import StagingApplication from .upload import EmbargoedUpload, Upload @@ -13,6 +14,7 @@ 'AssetBlob', 'AssetPath', 'AssetPathRelation', + 'AuditRecord', 'Dandiset', 'EmbargoedAssetBlob', 'EmbargoedUpload', diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py new file mode 100644 index 000000000..27bff46da --- /dev/null +++ b/dandiapi/api/models/audit.py @@ -0,0 +1,16 @@ +from __future__ import annotations + +from django.db import models + + +class AuditRecord(models.Model): + timestamp = models.DateTimeField(auto_now_add=True) + dandiset_id = models.IntegerField() + username = models.CharField(max_length=39) + user_email = models.CharField(max_length=254) + user_fullname = models.CharField(max_length=301) + record_type = models.CharField(max_length=32, choices=AUDIT_RECORD_CHOICES) + details = models.JSONField(blank=True) + + def __str__(self): + return f'{self.record_type}/{self.dandiset_id:06}' From 5ad7856924509e87b24726cb8761563e75883604 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 10:58:05 -0500 Subject: [PATCH 02/37] Add admin model for AuditRecord --- dandiapi/api/admin.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/dandiapi/api/admin.py b/dandiapi/api/admin.py index 06abb2e85..b0dc23a7f 100644 --- a/dandiapi/api/admin.py +++ b/dandiapi/api/admin.py @@ -19,6 +19,7 @@ from dandiapi.api.models import ( Asset, AssetBlob, + AuditRecord, Dandiset, EmbargoedAssetBlob, Upload, @@ -240,3 +241,39 @@ class AssetAdmin(admin.ModelAdmin): class UploadAdmin(admin.ModelAdmin): list_display = ['id', 'upload_id', 'blob', 'etag', 'upload_id', 'size', 'created'] list_display_links = ['id', 'upload_id'] + + +@admin.register(AuditRecord) +class AuditRecordAdmin(admin.ModelAdmin): + actions = None + date_hierarchy = 'timestamp' + search_fields = [ + 'dandiset_id', + 'username', + 'record_type', + ] + list_display = [ + 'id', + 'timestamp', + 'dandiset', + 'record_type', + 'details', + 'username', + ] + + @admin.display(description='Dandiset', ordering='dandiset_id') + def dandiset(self, obj): + return format_html( + '{}', + reverse('admin:api_dandiset_change', args=(obj.dandiset_id,)), + f'{obj.dandiset_id:06}', + ) + + def has_add_permission(self, request): + return False + + def has_change_permission(self, request, obj=None): + return False + + def has_delete_permission(self, request, obj=None): + return False From 31472ee2836c4c40566f69d956090c18a7b4f6b6 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:00:12 -0500 Subject: [PATCH 03/37] Generate create_dandiset audit record --- dandiapi/api/models/audit.py | 38 ++++++++++++++++++++++ dandiapi/api/services/dandiset/__init__.py | 6 ++++ 2 files changed, 44 insertions(+) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 27bff46da..039911fa2 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -1,7 +1,22 @@ from __future__ import annotations +from typing import TYPE_CHECKING, Literal, get_args + from django.db import models +if TYPE_CHECKING: + from django.contrib.auth.models import User + + from dandiapi.zarr.models import ZarrArchive + + from .asset import Asset + from .dandiset import Dandiset + +AuditRecordType = Literal[ + 'create_dandiset', +] +AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] + class AuditRecord(models.Model): timestamp = models.DateTimeField(auto_now_add=True) @@ -14,3 +29,26 @@ class AuditRecord(models.Model): def __str__(self): return f'{self.record_type}/{self.dandiset_id:06}' + + @staticmethod + def make_audit_record( + *, dandiset: Dandiset, user: User, record_type: AuditRecordType, details: dict + ) -> AuditRecord: + return AuditRecord( + dandiset_id=dandiset.id, + username=user.username, + user_email=user.email, + user_fullname=f'{user.first_name} {user.last_name}', + record_type=record_type, + details=details, + ) + + @staticmethod + def create_dandiset(*, dandiset: Dandiset, user: User, metadata: dict, embargoed: bool): + details = { + 'metadata': metadata, + 'embargoed': embargoed, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='create_dandiset', details=details + ) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 0cf80d82c..9cc15d35d 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -2,6 +2,7 @@ from django.db import transaction +from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError @@ -45,6 +46,11 @@ def create_dandiset( draft_version.full_clean(validate_constraints=False) draft_version.save() + audit_record = AuditRecord.create_dandiset( + dandiset=dandiset, user=user, metadata=version_metadata, embargoed=embargo + ) + audit_record.save() + return dandiset, draft_version From eccedd9ee410d10bbf2b4017e0605ee4f0882e89 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:02:09 -0500 Subject: [PATCH 04/37] Generate change_owners audit record --- dandiapi/api/models/audit.py | 20 ++++++++++++++++++++ dandiapi/api/views/dandiset.py | 15 +++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 039911fa2..a790d280a 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -14,6 +14,7 @@ AuditRecordType = Literal[ 'create_dandiset', + 'change_owners', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -52,3 +53,22 @@ def create_dandiset(*, dandiset: Dandiset, user: User, metadata: dict, embargoed return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='create_dandiset', details=details ) + + @staticmethod + def change_owners( + *, dandiset: Dandiset, user: User, removed_owners: list[User], added_owners: list[User] + ): + def glean_user_info(user: User): + return { + 'username': user.username, + 'email': user.email, + 'name': f'{user.first_name} {user.last_name}', + } + + details = { + 'removed_owners': [glean_user_info(u) for u in removed_owners], + 'added_owners': [glean_user_info(u) for u in added_owners], + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='change_owners', details=details + ) diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 040395aef..1789ee7ad 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -4,6 +4,7 @@ from allauth.socialaccount.models import SocialAccount from django.contrib.auth.models import User +from django.db import transaction from django.db.models import Count, Max, OuterRef, Subquery, Sum from django.db.models.functions import Coalesce from django.db.models.query_utils import Q @@ -23,7 +24,7 @@ from dandiapi.api.asset_paths import get_root_paths_many from dandiapi.api.mail import send_ownership_change_emails -from dandiapi.api.models import Dandiset, Version +from dandiapi.api.models import AuditRecord, Dandiset, Version from dandiapi.api.services.dandiset import create_dandiset, delete_dandiset from dandiapi.api.services.dandiset.exceptions import UnauthorizedEmbargoAccessError from dandiapi.api.services.embargo import unembargo_dandiset @@ -408,7 +409,17 @@ def users(self, request, dandiset__pk): # All owners found owners = user_owners + [acc.user for acc in socialaccount_owners] removed_owners, added_owners = dandiset.set_owners(owners) - dandiset.save() + with transaction.atomic(): + dandiset.save() + + if removed_owners or added_owners: + audit_record = AuditRecord.change_owners( + dandiset=dandiset, + user=request.user, + removed_owners=removed_owners, + added_owners=added_owners, + ) + audit_record.save() send_ownership_change_emails(dandiset, removed_owners, added_owners) From c50290bbd2dfb4cbc6bc845a8fec067facfaa70c Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:03:56 -0500 Subject: [PATCH 05/37] Generate update_metadata audit record --- dandiapi/api/models/audit.py | 8 ++++++++ dandiapi/api/views/version.py | 7 ++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index a790d280a..9fddf8cc5 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -15,6 +15,7 @@ AuditRecordType = Literal[ 'create_dandiset', 'change_owners', + 'update_metadata', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -72,3 +73,10 @@ def glean_user_info(user: User): return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='change_owners', details=details ) + + @staticmethod + def update_metadata(*, dandiset: Dandiset, user: User, metadata: dict) -> AuditRecord: + details = {'metadata': metadata} + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='update_metadata', details=details + ) diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index b94592414..65678e747 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -13,7 +13,7 @@ from rest_framework.viewsets import ReadOnlyModelViewSet from rest_framework_extensions.mixins import DetailSerializerMixin, NestedViewSetMixin -from dandiapi.api.models import Dandiset, Version +from dandiapi.api.models import AuditRecord, Dandiset, Version from dandiapi.api.services.publish import publish_dandiset from dandiapi.api.tasks import delete_doi_task from dandiapi.api.views.common import DANDISET_PK_PARAM, VERSION_PARAM, DandiPagination @@ -116,6 +116,11 @@ def update(self, request, **kwargs): locked_version.status = Version.Status.PENDING locked_version.save() + audit_record = AuditRecord.update_metadata( + dandiset=locked_version.dandiset, user=request.user, metadata=new_metadata + ) + audit_record.save() + serializer = VersionDetailSerializer(instance=locked_version) return Response(serializer.data, status=status.HTTP_200_OK) From fdb49a5931e6c1f8c29029999f6a86cdabd5bac7 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:06:52 -0500 Subject: [PATCH 06/37] Generate [add|update|remove]_asset audit records --- dandiapi/api/models/audit.py | 45 +++++++++++++++++++++++++ dandiapi/api/services/asset/__init__.py | 22 ++++++++++-- 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 9fddf8cc5..798255b51 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -16,6 +16,9 @@ 'create_dandiset', 'change_owners', 'update_metadata', + 'add_asset', + 'update_asset', + 'remove_asset', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -80,3 +83,45 @@ def update_metadata(*, dandiset: Dandiset, user: User, metadata: dict) -> AuditR return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='update_metadata', details=details ) + + @staticmethod + def _asset_details(asset: Asset) -> dict: + checksum = ( + (asset.blob and asset.blob.etag) + or (asset.embargoed_blob and asset.embargoed_blob.etag) + or (asset.zarr and asset.zarr.checksum) + ) + + return { + 'path': asset.path, + 'asset_blob_id': asset.blob and asset.blob.id, + 'embargoed_asset_blob_id': asset.embargoed_blob and asset.embargoed_blob.id, + 'zarr_archive_id': asset.zarr and asset.zarr.id, + 'asset_id': asset.id, + 'checksum': checksum, + 'metadata': asset.metadata, + } + + @staticmethod + def add_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = AuditRecord._asset_details(asset) + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='add_asset', details=details + ) + + @staticmethod + def update_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = AuditRecord._asset_details(asset) + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='update_asset', details=details + ) + + @staticmethod + def remove_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = { + 'path': asset.path, + 'asset_id': asset.id, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='remove_asset', details=details + ) diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index 8cedcd739..23beadfd9 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -6,6 +6,7 @@ from dandiapi.api.asset_paths import add_asset_paths, delete_asset_paths, get_conflicting_paths from dandiapi.api.models.asset import Asset, AssetBlob, EmbargoedAssetBlob +from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.version import Version from dandiapi.api.services.asset.exceptions import ( AssetAlreadyExistsError, @@ -84,7 +85,7 @@ def change_asset( # noqa: PLR0913 raise AssetAlreadyExistsError with transaction.atomic(): - remove_asset_from_version(user=user, asset=asset, version=version) + remove_asset_from_version(user=user, asset=asset, version=version, audit=False) new_asset = add_asset_to_version( user=user, @@ -92,11 +93,15 @@ def change_asset( # noqa: PLR0913 asset_blob=new_asset_blob, zarr_archive=new_zarr_archive, metadata=new_metadata, + audit=False, ) # Set previous asset and save new_asset.previous = asset new_asset.save() + audit_record = AuditRecord.update_asset(dandiset=version.dandiset, user=user, asset=asset) + audit_record.save() + return new_asset, True @@ -107,6 +112,7 @@ def add_asset_to_version( asset_blob: AssetBlob | EmbargoedAssetBlob | None = None, zarr_archive: ZarrArchive | None = None, metadata: dict, + audit: bool = True, ) -> Asset: """Create an asset, adding it to a version.""" if not asset_blob and not zarr_archive: @@ -158,10 +164,16 @@ def add_asset_to_version( # Save the version so that the modified field is updated version.save() + if audit: + audit_record = AuditRecord.add_asset(dandiset=version.dandiset, user=user, asset=asset) + audit_record.save() + return asset -def remove_asset_from_version(*, user, asset: Asset, version: Version) -> Version: +def remove_asset_from_version( + *, user, asset: Asset, version: Version, audit: bool = True +) -> Version: if not user.has_perm('owner', version.dandiset): raise DandisetOwnerRequiredError if version.version != 'draft': @@ -177,4 +189,10 @@ def remove_asset_from_version(*, user, asset: Asset, version: Version) -> Versio # Save the version so that the modified field is updated version.save() + if audit: + audit_record = AuditRecord.remove_asset( + dandiset=version.dandiset, user=user, asset=asset + ) + audit_record.save() + return version From 1ad599d6a9a9103e69672923cacb89cd4673a393 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:09:52 -0500 Subject: [PATCH 07/37] Generate [create|upload|finalize|delete]_zarr audit records --- dandiapi/api/models/audit.py | 47 +++++++++++++++++++++++++++++++++ dandiapi/zarr/views/__init__.py | 30 ++++++++++++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 798255b51..6ffcfd30f 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -19,6 +19,10 @@ 'add_asset', 'update_asset', 'remove_asset', + 'create_zarr', + 'upload_zarr', + 'delete_zarr_chunks', + 'finalize_zarr', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -125,3 +129,46 @@ def remove_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='remove_asset', details=details ) + + @staticmethod + def create_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'name': zarr_archive.name, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='create_zarr', details=details + ) + + @staticmethod + def upload_zarr( + *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, urls: list[str] + ) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'num_urls': len(urls), + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='upload_zarr', details=details + ) + + @staticmethod + def delete_zarr_chunks( + *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] + ) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'num_chunks': len(paths), + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='delete_zarr_chunks', details=details + ) + + @staticmethod + def finalize_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='finalize_zarr', details=details + ) diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index 61a1057c6..19e34f58c 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -14,6 +14,7 @@ from rest_framework.utils.urls import replace_query_param from rest_framework.viewsets import ReadOnlyModelViewSet +from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.storage import get_boto_client from dandiapi.api.views.common import DandiPagination @@ -139,7 +140,13 @@ def create(self, request): raise ValidationError('Cannot add zarr to embargoed dandiset') zarr_archive: ZarrArchive = ZarrArchive(name=name, dandiset=dandiset) try: - zarr_archive.save() + with transaction.atomic(): + zarr_archive.save() + + audit_record = AuditRecord.create_zarr( + dandiset=dandiset, user=request.user, zarr_archive=zarr_archive + ) + audit_record.save() except IntegrityError as e: raise ValidationError('Zarr already exists') from e @@ -175,6 +182,11 @@ def finalize(self, request, zarr_id): zarr_archive.status = ZarrArchiveStatus.UPLOADED zarr_archive.save() + audit_record = AuditRecord.finalize_zarr( + dandiset=zarr_archive.dandiset, user=request.user, zarr_archive=zarr_archive + ) + audit_record.save() + # Dispatch task ingest_zarr_archive.delay(zarr_id=zarr_archive.zarr_id) return Response(None, status=status.HTTP_204_NO_CONTENT) @@ -289,6 +301,14 @@ def create_files(self, request, zarr_id): zarr_archive.mark_pending() zarr_archive.save() + audit_record = AuditRecord.upload_zarr( + dandiset=zarr_archive.dandiset, + user=request.user, + zarr_archive=zarr_archive, + urls=urls, + ) + audit_record.save() + # Return presigned urls logger.info( 'Presigned %d URLs to upload to zarr archive %s', len(urls), zarr_archive.zarr_id @@ -320,4 +340,12 @@ def delete_files(self, request, zarr_id): paths = [file['path'] for file in serializer.validated_data] zarr_archive.delete_files(paths) + audit_record = AuditRecord.delete_zarr_chunks( + dandiset=zarr_archive.dandiset, + user=request.user, + zarr_archive=zarr_archive, + paths=paths, + ) + audit_record.save() + return Response(None, status=status.HTTP_204_NO_CONTENT) From 7f0fc968c8fe3a44dc447275d169a4d63c4fda36 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:12:19 -0500 Subject: [PATCH 08/37] Generate unembargo_dandiset audit record --- dandiapi/api/models/audit.py | 7 +++++++ dandiapi/api/services/embargo/__init__.py | 9 ++++++--- dandiapi/api/tasks/__init__.py | 6 ++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 6ffcfd30f..0d2694d29 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -23,6 +23,7 @@ 'upload_zarr', 'delete_zarr_chunks', 'finalize_zarr', + 'unembargo_dandiset', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -172,3 +173,9 @@ def finalize_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='finalize_zarr', details=details ) + + @staticmethod + def unembargo_dandiset(*, dandiset: Dandiset, user: User) -> AuditRecord: + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='unembargo_dandiset', details={} + ) diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 9b3e4425b..0f3bb854f 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -5,7 +5,7 @@ from django.conf import settings from dandiapi.api.copy import copy_object_multipart -from dandiapi.api.models import Asset, AssetBlob, Dandiset, Upload, Version +from dandiapi.api.models import Asset, AssetBlob, AuditRecord, Dandiset, Upload, Version from dandiapi.api.services.asset.exceptions import DandisetOwnerRequiredError from dandiapi.api.tasks import unembargo_dandiset_task @@ -56,7 +56,7 @@ def _unembargo_asset(asset: Asset): asset.save() -def _unembargo_dandiset(dandiset: Dandiset): +def _unembargo_dandiset(dandiset: Dandiset, user: User): draft_version: Version = dandiset.draft_version embargoed_assets: QuerySet[Asset] = draft_version.assets.filter(embargoed_blob__isnull=False) @@ -74,6 +74,9 @@ def _unembargo_dandiset(dandiset: Dandiset): dandiset.embargo_status = Dandiset.EmbargoStatus.OPEN dandiset.save() + audit_record = AuditRecord.unembargo_dandiset(dandiset=dandiset, user=user) + audit_record.save() + def unembargo_dandiset(*, user: User, dandiset: Dandiset): """Unembargo a dandiset by copying all embargoed asset blobs to the public bucket.""" @@ -83,4 +86,4 @@ def unembargo_dandiset(*, user: User, dandiset: Dandiset): if not user.has_perm('owner', dandiset): raise DandisetOwnerRequiredError - unembargo_dandiset_task.delay(dandiset.id) + unembargo_dandiset_task.delay(dandiset.id, user.id) diff --git a/dandiapi/api/tasks/__init__.py b/dandiapi/api/tasks/__init__.py index 80f8ef87c..817f73e99 100644 --- a/dandiapi/api/tasks/__init__.py +++ b/dandiapi/api/tasks/__init__.py @@ -2,6 +2,7 @@ from celery import shared_task from celery.utils.log import get_task_logger +from django.contrib.auth.models import User from dandiapi.api.doi import delete_doi from dandiapi.api.manifests import ( @@ -68,11 +69,12 @@ def delete_doi_task(doi: str) -> None: @shared_task -def unembargo_dandiset_task(dandiset_id: int): +def unembargo_dandiset_task(dandiset_id: int, user_id: int): from dandiapi.api.services.embargo import _unembargo_dandiset dandiset = Dandiset.objects.get(id=dandiset_id) - _unembargo_dandiset(dandiset) + user = User.objects.get(id=user_id) + _unembargo_dandiset(dandiset, user) @shared_task From 2c9c6b38eddd4f470f0089d5b84b7a0b211d268c Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:17:23 -0500 Subject: [PATCH 09/37] Generate publish_dandiset audit record --- dandiapi/api/models/audit.py | 10 ++++++++++ dandiapi/api/services/publish/__init__.py | 14 ++++++++++---- dandiapi/api/tasks/__init__.py | 4 ++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 0d2694d29..848cbfa0b 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -24,6 +24,7 @@ 'delete_zarr_chunks', 'finalize_zarr', 'unembargo_dandiset', + 'publish_dandiset', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -179,3 +180,12 @@ def unembargo_dandiset(*, dandiset: Dandiset, user: User) -> AuditRecord: return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='unembargo_dandiset', details={} ) + + @staticmethod + def publish_dandiset(*, dandiset: Dandiset, user: User, version: str) -> AuditRecord: + details = { + 'version': version, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='publish_dandiset', details=details + ) diff --git a/dandiapi/api/services/publish/__init__.py b/dandiapi/api/services/publish/__init__.py index a41ead487..9b4938a45 100644 --- a/dandiapi/api/services/publish/__init__.py +++ b/dandiapi/api/services/publish/__init__.py @@ -4,12 +4,13 @@ from typing import TYPE_CHECKING from dandischema.metadata import aggregate_assets_summary, validate +from django.contrib.auth.models import User from django.db import transaction from more_itertools import ichunked from dandiapi.api import doi from dandiapi.api.asset_paths import add_version_asset_paths -from dandiapi.api.models import Asset, Dandiset, Version +from dandiapi.api.models import Asset, AuditRecord, Dandiset, Version from dandiapi.api.services.exceptions import NotAllowedError from dandiapi.api.services.publish.exceptions import ( DandisetAlreadyPublishedError, @@ -22,7 +23,6 @@ from dandiapi.api.tasks import write_manifest_files if TYPE_CHECKING: - from django.contrib.auth.models import User from django.db.models import QuerySet @@ -103,7 +103,7 @@ def _build_publishable_version_from_draft(draft_version: Version) -> Version: return publishable_version -def _publish_dandiset(dandiset_id: int) -> None: +def _publish_dandiset(dandiset_id: int, user_id: int) -> None: """ Publish a dandiset. @@ -189,10 +189,16 @@ def _create_doi(version_id: int): transaction.on_commit(lambda: _create_doi(new_version.id)) + user = User.objects.get(id=user_id) + audit_record = AuditRecord.publish_dandiset( + dandiset=new_version.dandiset, user=user, version=new_version.version + ) + audit_record.save() + def publish_dandiset(*, user: User, dandiset: Dandiset) -> None: from dandiapi.api.tasks import publish_dandiset_task with transaction.atomic(): _lock_dandiset_for_publishing(user=user, dandiset=dandiset) - transaction.on_commit(lambda: publish_dandiset_task.delay(dandiset.id)) + transaction.on_commit(lambda: publish_dandiset_task.delay(dandiset.id, user.id)) diff --git a/dandiapi/api/tasks/__init__.py b/dandiapi/api/tasks/__init__.py index 817f73e99..31cecc310 100644 --- a/dandiapi/api/tasks/__init__.py +++ b/dandiapi/api/tasks/__init__.py @@ -78,7 +78,7 @@ def unembargo_dandiset_task(dandiset_id: int, user_id: int): @shared_task -def publish_dandiset_task(dandiset_id: int): +def publish_dandiset_task(dandiset_id: int, user_id: int): from dandiapi.api.services.publish import _publish_dandiset - _publish_dandiset(dandiset_id=dandiset_id) + _publish_dandiset(dandiset_id=dandiset_id, user_id=user_id) From 01f1b85efe13f3b8418359addf3c067ed5e96710 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:19:55 -0500 Subject: [PATCH 10/37] Generate delete_dandiset audit record --- dandiapi/api/models/audit.py | 7 +++++++ dandiapi/api/services/dandiset/__init__.py | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 848cbfa0b..03f6d479b 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -25,6 +25,7 @@ 'finalize_zarr', 'unembargo_dandiset', 'publish_dandiset', + 'delete_dandiset', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -189,3 +190,9 @@ def publish_dandiset(*, dandiset: Dandiset, user: User, version: str) -> AuditRe return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='publish_dandiset', details=details ) + + @staticmethod + def delete_dandiset(*, dandiset: Dandiset, user: User): + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='delete_dandiset', details={} + ) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 9cc15d35d..3b28172ac 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -65,5 +65,10 @@ def delete_dandiset(*, user, dandiset: Dandiset) -> None: # Delete all versions first, so that AssetPath deletion is cascaded # through versions, rather than through zarrs directly with transaction.atomic(): + # Record the audit event first so that the AuditRecord instance has a + # chance to grab the Dandiset information before it is destroyed. + audit_record = AuditRecord.delete_dandiset(dandiset=dandiset, user=user) + audit_record.save() + dandiset.versions.all().delete() dandiset.delete() From 52ff47ebcb752f3c605deec06c5179fe6838b8b2 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:20:10 -0500 Subject: [PATCH 11/37] Add migration for AuditRecord --- dandiapi/api/migrations/0006_auditrecord.py | 51 +++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 dandiapi/api/migrations/0006_auditrecord.py diff --git a/dandiapi/api/migrations/0006_auditrecord.py b/dandiapi/api/migrations/0006_auditrecord.py new file mode 100644 index 000000000..4d64a0bff --- /dev/null +++ b/dandiapi/api/migrations/0006_auditrecord.py @@ -0,0 +1,51 @@ +# Generated by Django 4.1.13 on 2024-03-06 15:39 +from __future__ import annotations + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('api', '0005_null_charfield'), + ] + + operations = [ + migrations.CreateModel( + name='AuditRecord', + fields=[ + ( + 'id', + models.BigAutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name='ID' + ), + ), + ('timestamp', models.DateTimeField(auto_now_add=True)), + ('dandiset_id', models.IntegerField()), + ('username', models.CharField(max_length=39)), + ('user_email', models.CharField(max_length=254)), + ('user_fullname', models.CharField(max_length=301)), + ( + 'record_type', + models.CharField( + choices=[ + ('create_dandiset', 'create_dandiset'), + ('change_owners', 'change_owners'), + ('update_metadata', 'update_metadata'), + ('add_asset', 'add_asset'), + ('update_asset', 'update_asset'), + ('remove_asset', 'remove_asset'), + ('create_zarr', 'create_zarr'), + ('upload_zarr', 'upload_zarr'), + ('delete_zarr_chunks', 'delete_zarr_chunks'), + ('finalize_zarr', 'finalize_zarr'), + ('unembargo_dandiset', 'unembargo_dandiset'), + ('publish_dandiset', 'publish_dandiset'), + ('delete_dandiset', 'delete_dandiset'), + ], + max_length=32, + ), + ), + ('details', models.JSONField(blank=True)), + ], + ), + ] From 08b26c61b20810446e8067bdbed8fc14edad27bb Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 14:27:13 -0500 Subject: [PATCH 12/37] Fix tests broken by signature changes --- dandiapi/api/tests/test_asset_paths.py | 6 ++++-- dandiapi/api/tests/test_tasks.py | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/dandiapi/api/tests/test_asset_paths.py b/dandiapi/api/tests/test_asset_paths.py index 3d8f5b1ff..92c5430e5 100644 --- a/dandiapi/api/tests/test_asset_paths.py +++ b/dandiapi/api/tests/test_asset_paths.py @@ -295,7 +295,7 @@ def test_asset_path_search_asset_paths(draft_version_factory, asset_factory): @pytest.mark.django_db() -def test_asset_path_publish_version(draft_version_factory, asset_factory): +def test_asset_path_publish_version(draft_version_factory, asset_factory, user_factory): version: Version = draft_version_factory() asset = asset_factory(path='foo/bar.txt', status=Asset.Status.VALID) version.assets.add(asset) @@ -308,8 +308,10 @@ def test_asset_path_publish_version(draft_version_factory, asset_factory): version.status = Version.Status.PUBLISHING version.save() + user = user_factory() + # Publish - publish_dandiset_task(version.dandiset.id) + publish_dandiset_task(version.dandiset.id, user.id) # Get published version published_version = ( diff --git a/dandiapi/api/tests/test_tasks.py b/dandiapi/api/tests/test_tasks.py index bfffb13f0..e299486d7 100644 --- a/dandiapi/api/tests/test_tasks.py +++ b/dandiapi/api/tests/test_tasks.py @@ -305,6 +305,7 @@ def test_publish_task( draft_asset_factory, published_asset_factory, draft_version_factory, + user_factory, django_capture_on_commit_callbacks, ): # Create a draft_version in PUBLISHING state @@ -323,8 +324,10 @@ def test_publish_task( # Ensure that the number of versions increases by 1 after publishing starting_version_count = draft_version.dandiset.versions.count() + user = user_factory() + with django_capture_on_commit_callbacks(execute=True): - tasks.publish_dandiset_task(draft_version.dandiset.id) + tasks.publish_dandiset_task(draft_version.dandiset.id, user.id) assert draft_version.dandiset.versions.count() == starting_version_count + 1 From 7f21061e75ed57af72420c832e8844bc31161853 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Tue, 23 Jul 2024 21:31:35 -0400 Subject: [PATCH 13/37] Rename upload_zarr to upload_zarr_chunks --- dandiapi/api/migrations/0006_auditrecord.py | 51 --------------------- dandiapi/api/migrations/0010_auditrecord.py | 26 +++++++++++ dandiapi/api/models/audit.py | 6 +-- 3 files changed, 29 insertions(+), 54 deletions(-) delete mode 100644 dandiapi/api/migrations/0006_auditrecord.py create mode 100644 dandiapi/api/migrations/0010_auditrecord.py diff --git a/dandiapi/api/migrations/0006_auditrecord.py b/dandiapi/api/migrations/0006_auditrecord.py deleted file mode 100644 index 4d64a0bff..000000000 --- a/dandiapi/api/migrations/0006_auditrecord.py +++ /dev/null @@ -1,51 +0,0 @@ -# Generated by Django 4.1.13 on 2024-03-06 15:39 -from __future__ import annotations - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ('api', '0005_null_charfield'), - ] - - operations = [ - migrations.CreateModel( - name='AuditRecord', - fields=[ - ( - 'id', - models.BigAutoField( - auto_created=True, primary_key=True, serialize=False, verbose_name='ID' - ), - ), - ('timestamp', models.DateTimeField(auto_now_add=True)), - ('dandiset_id', models.IntegerField()), - ('username', models.CharField(max_length=39)), - ('user_email', models.CharField(max_length=254)), - ('user_fullname', models.CharField(max_length=301)), - ( - 'record_type', - models.CharField( - choices=[ - ('create_dandiset', 'create_dandiset'), - ('change_owners', 'change_owners'), - ('update_metadata', 'update_metadata'), - ('add_asset', 'add_asset'), - ('update_asset', 'update_asset'), - ('remove_asset', 'remove_asset'), - ('create_zarr', 'create_zarr'), - ('upload_zarr', 'upload_zarr'), - ('delete_zarr_chunks', 'delete_zarr_chunks'), - ('finalize_zarr', 'finalize_zarr'), - ('unembargo_dandiset', 'unembargo_dandiset'), - ('publish_dandiset', 'publish_dandiset'), - ('delete_dandiset', 'delete_dandiset'), - ], - max_length=32, - ), - ), - ('details', models.JSONField(blank=True)), - ], - ), - ] diff --git a/dandiapi/api/migrations/0010_auditrecord.py b/dandiapi/api/migrations/0010_auditrecord.py new file mode 100644 index 000000000..a929ca58c --- /dev/null +++ b/dandiapi/api/migrations/0010_auditrecord.py @@ -0,0 +1,26 @@ +# Generated by Django 4.1.13 on 2024-07-24 01:30 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0009_remove_embargoedassetblob_dandiset_and_more'), + ] + + operations = [ + migrations.CreateModel( + name='AuditRecord', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('timestamp', models.DateTimeField(auto_now_add=True)), + ('dandiset_id', models.IntegerField()), + ('username', models.CharField(max_length=39)), + ('user_email', models.CharField(max_length=254)), + ('user_fullname', models.CharField(max_length=301)), + ('record_type', models.CharField(choices=[('create_dandiset', 'create_dandiset'), ('change_owners', 'change_owners'), ('update_metadata', 'update_metadata'), ('add_asset', 'add_asset'), ('update_asset', 'update_asset'), ('remove_asset', 'remove_asset'), ('create_zarr', 'create_zarr'), ('upload_zarr_chunks', 'upload_zarr_chunks'), ('delete_zarr_chunks', 'delete_zarr_chunks'), ('finalize_zarr', 'finalize_zarr'), ('unembargo_dandiset', 'unembargo_dandiset'), ('publish_dandiset', 'publish_dandiset'), ('delete_dandiset', 'delete_dandiset')], max_length=32)), + ('details', models.JSONField(blank=True)), + ], + ), + ] diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 03f6d479b..ca691aca3 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -20,7 +20,7 @@ 'update_asset', 'remove_asset', 'create_zarr', - 'upload_zarr', + 'upload_zarr_chunks', 'delete_zarr_chunks', 'finalize_zarr', 'unembargo_dandiset', @@ -144,7 +144,7 @@ def create_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> ) @staticmethod - def upload_zarr( + def upload_zarr_chunks( *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, urls: list[str] ) -> AuditRecord: details = { @@ -152,7 +152,7 @@ def upload_zarr( 'num_urls': len(urls), } return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='upload_zarr', details=details + dandiset=dandiset, user=user, record_type='upload_zarr_chunks', details=details ) @staticmethod From 38d1a29a0ef66dd4c987b378d920a9fb8105910c Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Tue, 23 Jul 2024 21:40:56 -0400 Subject: [PATCH 14/37] Use the `user` fixture directly --- dandiapi/api/tests/test_asset_paths.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dandiapi/api/tests/test_asset_paths.py b/dandiapi/api/tests/test_asset_paths.py index 92c5430e5..9e1a31126 100644 --- a/dandiapi/api/tests/test_asset_paths.py +++ b/dandiapi/api/tests/test_asset_paths.py @@ -295,7 +295,7 @@ def test_asset_path_search_asset_paths(draft_version_factory, asset_factory): @pytest.mark.django_db() -def test_asset_path_publish_version(draft_version_factory, asset_factory, user_factory): +def test_asset_path_publish_version(draft_version_factory, asset_factory, user): version: Version = draft_version_factory() asset = asset_factory(path='foo/bar.txt', status=Asset.Status.VALID) version.assets.add(asset) @@ -308,8 +308,6 @@ def test_asset_path_publish_version(draft_version_factory, asset_factory, user_f version.status = Version.Status.PUBLISHING version.save() - user = user_factory() - # Publish publish_dandiset_task(version.dandiset.id, user.id) From 50d80e61f3397ce9912825c12218a2e4d066538a Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 10:59:14 -0400 Subject: [PATCH 15/37] Add explanatory comments for weird char field length limits --- dandiapi/api/models/audit.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index ca691aca3..b96cacc67 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -33,8 +33,20 @@ class AuditRecord(models.Model): timestamp = models.DateTimeField(auto_now_add=True) dandiset_id = models.IntegerField() + + # GitHub enforces a 39 character limit on usernames (see, e.g., + # https://docs.github.com/en/enterprise-cloud@latest/admin/managing-iam/iam-configuration-reference/username-considerations-for-external-authentication). username = models.CharField(max_length=39) + + # According to RFC 5321 (https://www.rfc-editor.org/rfc/rfc5321.txt), + # section 4.5.3.1.3, an email address "path" is limited to 256 octets, + # including the surrounding angle brackets. Without the brackets, that + # leaves 254 characters for the email address itself. user_email = models.CharField(max_length=254) + + # The signup questionnaire imposes a 150 character limit on both first and + # last names; together with a space to separate them, that makes a 301 + # character limit on the full name. user_fullname = models.CharField(max_length=301) record_type = models.CharField(max_length=32, choices=AUDIT_RECORD_CHOICES) details = models.JSONField(blank=True) From 6e204c5e8e19fdd69d1e9ae9dc4d949c340a4ee8 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 11:45:23 -0400 Subject: [PATCH 16/37] Apply formatting to new migration --- dandiapi/api/migrations/0010_auditrecord.py | 31 +++++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/dandiapi/api/migrations/0010_auditrecord.py b/dandiapi/api/migrations/0010_auditrecord.py index a929ca58c..a58f12eb3 100644 --- a/dandiapi/api/migrations/0010_auditrecord.py +++ b/dandiapi/api/migrations/0010_auditrecord.py @@ -1,10 +1,10 @@ # Generated by Django 4.1.13 on 2024-07-24 01:30 +from __future__ import annotations from django.db import migrations, models class Migration(migrations.Migration): - dependencies = [ ('api', '0009_remove_embargoedassetblob_dandiset_and_more'), ] @@ -13,13 +13,38 @@ class Migration(migrations.Migration): migrations.CreateModel( name='AuditRecord', fields=[ - ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ( + 'id', + models.BigAutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name='ID' + ), + ), ('timestamp', models.DateTimeField(auto_now_add=True)), ('dandiset_id', models.IntegerField()), ('username', models.CharField(max_length=39)), ('user_email', models.CharField(max_length=254)), ('user_fullname', models.CharField(max_length=301)), - ('record_type', models.CharField(choices=[('create_dandiset', 'create_dandiset'), ('change_owners', 'change_owners'), ('update_metadata', 'update_metadata'), ('add_asset', 'add_asset'), ('update_asset', 'update_asset'), ('remove_asset', 'remove_asset'), ('create_zarr', 'create_zarr'), ('upload_zarr_chunks', 'upload_zarr_chunks'), ('delete_zarr_chunks', 'delete_zarr_chunks'), ('finalize_zarr', 'finalize_zarr'), ('unembargo_dandiset', 'unembargo_dandiset'), ('publish_dandiset', 'publish_dandiset'), ('delete_dandiset', 'delete_dandiset')], max_length=32)), + ( + 'record_type', + models.CharField( + choices=[ + ('create_dandiset', 'create_dandiset'), + ('change_owners', 'change_owners'), + ('update_metadata', 'update_metadata'), + ('add_asset', 'add_asset'), + ('update_asset', 'update_asset'), + ('remove_asset', 'remove_asset'), + ('create_zarr', 'create_zarr'), + ('upload_zarr_chunks', 'upload_zarr_chunks'), + ('delete_zarr_chunks', 'delete_zarr_chunks'), + ('finalize_zarr', 'finalize_zarr'), + ('unembargo_dandiset', 'unembargo_dandiset'), + ('publish_dandiset', 'publish_dandiset'), + ('delete_dandiset', 'delete_dandiset'), + ], + max_length=32, + ), + ), ('details', models.JSONField(blank=True)), ], ), From f772fc5481f3b1a02f95da850bd618035f941311 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 12:04:23 -0400 Subject: [PATCH 17/37] Remove Dandiset add_owner() and remove_owner() methods This came up during development of the audit functionality. It turns out that add_owner() was only called during Dandiset creation to add the creating user as the only owner, and remove_owner() is never called. Instead, in the method that changes the owner set, assign_perm() is used in all cases to add and remove owners as needed. Since no other point of use exists for adding/removing users, it makes sense to remove these methods. --- dandiapi/api/models/dandiset.py | 10 ---------- dandiapi/api/services/dandiset/__init__.py | 3 ++- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/dandiapi/api/models/dandiset.py b/dandiapi/api/models/dandiset.py index 4d95b6db4..97cb8bff3 100644 --- a/dandiapi/api/models/dandiset.py +++ b/dandiapi/api/models/dandiset.py @@ -99,16 +99,6 @@ def set_owners(self, new_owners): # Return the owners added/removed so they can be emailed return removed_owners, added_owners - def add_owner(self, new_owner): - old_owners = get_users_with_perms(self, only_with_perms_in=['owner']) - if new_owner not in old_owners: - assign_perm('owner', new_owner, self) - - def remove_owner(self, owner): - owners = get_users_with_perms(self, only_with_perms_in=['owner']) - if owner in owners: - remove_perm('owner', owner, self) - @classmethod def published_count(cls): """Return the number of Dandisets with published Versions.""" diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index f3016734b..f575f7f63 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -1,6 +1,7 @@ from __future__ import annotations from django.db import transaction +from guardian.shortcuts import assign_perm from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.dandiset import Dandiset @@ -37,7 +38,7 @@ def create_dandiset( dandiset = Dandiset(id=identifier, embargo_status=embargo_status) dandiset.full_clean() dandiset.save() - dandiset.add_owner(user) + assign_perm('owner', user, dandiset) draft_version = Version( dandiset=dandiset, name=version_name, From 0337f41c67ff92bcde3bf20ea970328cde7d6124 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 12:27:00 -0400 Subject: [PATCH 18/37] Move "set new owners" operation inside of transaction --- dandiapi/api/views/dandiset.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 6d6f9fa48..c6c1867b4 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -413,9 +413,9 @@ def users(self, request, dandiset__pk): # noqa: C901 raise ValidationError(f'User {username} not found') # All owners found - owners = user_owners + [acc.user for acc in socialaccount_owners] - removed_owners, added_owners = dandiset.set_owners(owners) with transaction.atomic(): + owners = user_owners + [acc.user for acc in socialaccount_owners] + removed_owners, added_owners = dandiset.set_owners(owners) dandiset.save() if removed_owners or added_owners: From 751d8b5ec435866b34078a279288b20783f8af40 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 14:40:24 -0400 Subject: [PATCH 19/37] Report live metadata to audit records Retrieving the metadata fresh from the model enables two things: 1. To report all "computed" metadata values that are derived from the supplied metadata. 2. To enable a changed `name` field on the Dandiset model to be properly reported via the metadata. --- dandiapi/api/services/dandiset/__init__.py | 2 +- dandiapi/api/views/version.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index f575f7f63..3f8785c03 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -49,7 +49,7 @@ def create_dandiset( draft_version.save() audit_record = AuditRecord.create_dandiset( - dandiset=dandiset, user=user, metadata=version_metadata, embargoed=embargo + dandiset=dandiset, user=user, metadata=draft_version.metadata, embargoed=embargo ) audit_record.save() diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index dffec235b..0287dd5f3 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -121,7 +121,7 @@ def update(self, request, **kwargs): locked_version.save() audit_record = AuditRecord.update_metadata( - dandiset=locked_version.dandiset, user=request.user, metadata=new_metadata + dandiset=locked_version.dandiset, user=request.user, metadata=locked_version.metadata ) audit_record.save() From 6c125bdbc3557f84c19deb4be6524b5743281062 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 15:07:03 -0400 Subject: [PATCH 20/37] Report list of paths to zarr chunk audit records --- dandiapi/api/models/audit.py | 6 +++--- dandiapi/zarr/views/__init__.py | 7 ++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index b96cacc67..ba7d9ed0d 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -157,11 +157,11 @@ def create_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> @staticmethod def upload_zarr_chunks( - *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, urls: list[str] + *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] ) -> AuditRecord: details = { 'zarr_id': zarr_archive.id, - 'num_urls': len(urls), + 'paths': paths, } return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='upload_zarr_chunks', details=details @@ -173,7 +173,7 @@ def delete_zarr_chunks( ) -> AuditRecord: details = { 'zarr_id': zarr_archive.id, - 'num_chunks': len(paths), + 'paths': paths, } return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='delete_zarr_chunks', details=details diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index e106e888d..986a83401 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -292,20 +292,21 @@ def create_files(self, request, zarr_id): serializer = ZarrFileCreationSerializer(data=request.data, many=True) serializer.is_valid(raise_exception=True) + paths = serializer.validated_data # Generate presigned urls logger.info('Beginning upload to zarr archive %s', zarr_archive.zarr_id) - urls = zarr_archive.generate_upload_urls(serializer.validated_data) + urls = zarr_archive.generate_upload_urls(paths) # Set status back to pending, since with these URLs the zarr could have been changed zarr_archive.mark_pending() zarr_archive.save() - audit_record = AuditRecord.upload_zarr( + audit_record = AuditRecord.upload_zarr_chunks( dandiset=zarr_archive.dandiset, user=request.user, zarr_archive=zarr_archive, - urls=urls, + paths=[p['path'] for p in paths], ) audit_record.save() From fcd288cc5df05a1c861e291e90ab676586cabe63 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 15:16:58 -0400 Subject: [PATCH 21/37] Suppress ruff warnings for complexity and number of arguments The `add_asset_to_version()` service function represents essential complexity in the actual domain. We may be able to simplify it by refactoring the function into smaller parts, but we will look into that independently. --- dandiapi/api/services/asset/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index ae4509870..a99243d2e 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -105,7 +105,7 @@ def change_asset( # noqa: PLR0913 return new_asset, True -def add_asset_to_version( +def add_asset_to_version( # noqa: PLR0913, C901 *, user, version: Version, From cbff89003be0744bf0309812840ff367370b175b Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 15:20:01 -0400 Subject: [PATCH 22/37] Reformat long line --- dandiapi/api/views/version.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index 0287dd5f3..f065f62d5 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -121,7 +121,9 @@ def update(self, request, **kwargs): locked_version.save() audit_record = AuditRecord.update_metadata( - dandiset=locked_version.dandiset, user=request.user, metadata=locked_version.metadata + dandiset=locked_version.dandiset, + user=request.user, + metadata=locked_version.metadata, ) audit_record.save() From a6489b62870db815450d59213e7a5e0b65088c44 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 15:39:17 -0400 Subject: [PATCH 23/37] Use the `user` fixture directly --- dandiapi/api/tests/test_tasks.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/dandiapi/api/tests/test_tasks.py b/dandiapi/api/tests/test_tasks.py index 1df58defb..8e7e11d98 100644 --- a/dandiapi/api/tests/test_tasks.py +++ b/dandiapi/api/tests/test_tasks.py @@ -308,7 +308,6 @@ def test_publish_task( draft_asset_factory, published_asset_factory, draft_version_factory, - user_factory, django_capture_on_commit_callbacks, ): # Create a draft_version in PUBLISHING state @@ -327,8 +326,6 @@ def test_publish_task( # Ensure that the number of versions increases by 1 after publishing starting_version_count = draft_version.dandiset.versions.count() - user = user_factory() - with django_capture_on_commit_callbacks(execute=True): tasks.publish_dandiset_task(draft_version.dandiset.id, user.id) From e120f861a9f2cfad5024a1695eb9a8cfe4dece7b Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 20:56:53 -0400 Subject: [PATCH 24/37] Create a service layer module for audit --- dandiapi/api/models/audit.py | 165 +------------------- dandiapi/api/services/asset/__init__.py | 22 ++- dandiapi/api/services/audit/__init__.py | 168 +++++++++++++++++++++ dandiapi/api/services/dandiset/__init__.py | 6 +- dandiapi/api/services/embargo/__init__.py | 5 +- dandiapi/api/services/publish/__init__.py | 5 +- dandiapi/api/views/dandiset.py | 5 +- dandiapi/api/views/version.py | 5 +- 8 files changed, 194 insertions(+), 187 deletions(-) create mode 100644 dandiapi/api/services/audit/__init__.py diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index ba7d9ed0d..56d5254f7 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -1,17 +1,9 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Literal, get_args +from typing import Literal, get_args from django.db import models -if TYPE_CHECKING: - from django.contrib.auth.models import User - - from dandiapi.zarr.models import ZarrArchive - - from .asset import Asset - from .dandiset import Dandiset - AuditRecordType = Literal[ 'create_dandiset', 'change_owners', @@ -53,158 +45,3 @@ class AuditRecord(models.Model): def __str__(self): return f'{self.record_type}/{self.dandiset_id:06}' - - @staticmethod - def make_audit_record( - *, dandiset: Dandiset, user: User, record_type: AuditRecordType, details: dict - ) -> AuditRecord: - return AuditRecord( - dandiset_id=dandiset.id, - username=user.username, - user_email=user.email, - user_fullname=f'{user.first_name} {user.last_name}', - record_type=record_type, - details=details, - ) - - @staticmethod - def create_dandiset(*, dandiset: Dandiset, user: User, metadata: dict, embargoed: bool): - details = { - 'metadata': metadata, - 'embargoed': embargoed, - } - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='create_dandiset', details=details - ) - - @staticmethod - def change_owners( - *, dandiset: Dandiset, user: User, removed_owners: list[User], added_owners: list[User] - ): - def glean_user_info(user: User): - return { - 'username': user.username, - 'email': user.email, - 'name': f'{user.first_name} {user.last_name}', - } - - details = { - 'removed_owners': [glean_user_info(u) for u in removed_owners], - 'added_owners': [glean_user_info(u) for u in added_owners], - } - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='change_owners', details=details - ) - - @staticmethod - def update_metadata(*, dandiset: Dandiset, user: User, metadata: dict) -> AuditRecord: - details = {'metadata': metadata} - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='update_metadata', details=details - ) - - @staticmethod - def _asset_details(asset: Asset) -> dict: - checksum = ( - (asset.blob and asset.blob.etag) - or (asset.embargoed_blob and asset.embargoed_blob.etag) - or (asset.zarr and asset.zarr.checksum) - ) - - return { - 'path': asset.path, - 'asset_blob_id': asset.blob and asset.blob.id, - 'embargoed_asset_blob_id': asset.embargoed_blob and asset.embargoed_blob.id, - 'zarr_archive_id': asset.zarr and asset.zarr.id, - 'asset_id': asset.id, - 'checksum': checksum, - 'metadata': asset.metadata, - } - - @staticmethod - def add_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: - details = AuditRecord._asset_details(asset) - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='add_asset', details=details - ) - - @staticmethod - def update_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: - details = AuditRecord._asset_details(asset) - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='update_asset', details=details - ) - - @staticmethod - def remove_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: - details = { - 'path': asset.path, - 'asset_id': asset.id, - } - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='remove_asset', details=details - ) - - @staticmethod - def create_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: - details = { - 'zarr_id': zarr_archive.id, - 'name': zarr_archive.name, - } - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='create_zarr', details=details - ) - - @staticmethod - def upload_zarr_chunks( - *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] - ) -> AuditRecord: - details = { - 'zarr_id': zarr_archive.id, - 'paths': paths, - } - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='upload_zarr_chunks', details=details - ) - - @staticmethod - def delete_zarr_chunks( - *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] - ) -> AuditRecord: - details = { - 'zarr_id': zarr_archive.id, - 'paths': paths, - } - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='delete_zarr_chunks', details=details - ) - - @staticmethod - def finalize_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: - details = { - 'zarr_id': zarr_archive.id, - } - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='finalize_zarr', details=details - ) - - @staticmethod - def unembargo_dandiset(*, dandiset: Dandiset, user: User) -> AuditRecord: - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='unembargo_dandiset', details={} - ) - - @staticmethod - def publish_dandiset(*, dandiset: Dandiset, user: User, version: str) -> AuditRecord: - details = { - 'version': version, - } - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='publish_dandiset', details=details - ) - - @staticmethod - def delete_dandiset(*, dandiset: Dandiset, user: User): - return AuditRecord.make_audit_record( - dandiset=dandiset, user=user, record_type='delete_dandiset', details={} - ) diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index a99243d2e..d295517e8 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -6,9 +6,9 @@ from dandiapi.api.asset_paths import add_asset_paths, delete_asset_paths, get_conflicting_paths from dandiapi.api.models.asset import Asset, AssetBlob -from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version +from dandiapi.api.services import audit from dandiapi.api.services.asset.exceptions import ( AssetAlreadyExistsError, AssetPathConflictError, @@ -85,7 +85,7 @@ def change_asset( # noqa: PLR0913 raise AssetAlreadyExistsError with transaction.atomic(): - remove_asset_from_version(user=user, asset=asset, version=version, audit=False) + remove_asset_from_version(user=user, asset=asset, version=version, do_audit=False) new_asset = add_asset_to_version( user=user, @@ -93,13 +93,13 @@ def change_asset( # noqa: PLR0913 asset_blob=new_asset_blob, zarr_archive=new_zarr_archive, metadata=new_metadata, - audit=False, + do_audit=False, ) # Set previous asset and save new_asset.previous = asset new_asset.save() - audit_record = AuditRecord.update_asset(dandiset=version.dandiset, user=user, asset=asset) + audit_record = audit.update_asset(dandiset=version.dandiset, user=user, asset=asset) audit_record.save() return new_asset, True @@ -112,7 +112,7 @@ def add_asset_to_version( # noqa: PLR0913, C901 asset_blob: AssetBlob | None = None, zarr_archive: ZarrArchive | None = None, metadata: dict, - audit: bool = True, + do_audit: bool = True, ) -> Asset: """Create an asset, adding it to a version.""" if not asset_blob and not zarr_archive: @@ -164,8 +164,8 @@ def add_asset_to_version( # noqa: PLR0913, C901 # Save the version so that the modified field is updated version.save() - if audit: - audit_record = AuditRecord.add_asset(dandiset=version.dandiset, user=user, asset=asset) + if do_audit: + audit_record = audit.add_asset(dandiset=version.dandiset, user=user, asset=asset) audit_record.save() # Perform this after the above transaction has finished, to ensure we only @@ -177,7 +177,7 @@ def add_asset_to_version( # noqa: PLR0913, C901 def remove_asset_from_version( - *, user, asset: Asset, version: Version, audit: bool = True + *, user, asset: Asset, version: Version, do_audit: bool = True ) -> Version: if not user.has_perm('owner', version.dandiset): raise DandisetOwnerRequiredError @@ -194,10 +194,8 @@ def remove_asset_from_version( # Save the version so that the modified field is updated version.save() - if audit: - audit_record = AuditRecord.remove_asset( - dandiset=version.dandiset, user=user, asset=asset - ) + if do_audit: + audit_record = audit.remove_asset(dandiset=version.dandiset, user=user, asset=asset) audit_record.save() return version diff --git a/dandiapi/api/services/audit/__init__.py b/dandiapi/api/services/audit/__init__.py new file mode 100644 index 000000000..f595a8996 --- /dev/null +++ b/dandiapi/api/services/audit/__init__.py @@ -0,0 +1,168 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING + +from dandiapi.api.models.audit import AuditRecord + +if TYPE_CHECKING: + from django.contrib.auth.models import User + + from dandiapi.api.models.asset import Asset + from dandiapi.api.models.audit import AuditRecordType + from dandiapi.api.models.dandiset import Dandiset + from dandiapi.zarr.models import ZarrArchive + + +def _make_audit_record( + *, dandiset: Dandiset, user: User, record_type: AuditRecordType, details: dict +) -> AuditRecord: + return AuditRecord( + dandiset_id=dandiset.id, + username=user.username, + user_email=user.email, + user_fullname=f'{user.first_name} {user.last_name}', + record_type=record_type, + details=details, + ) + + +def create_dandiset(*, dandiset: Dandiset, user: User, metadata: dict, embargoed: bool): + details = { + 'metadata': metadata, + 'embargoed': embargoed, + } + return _make_audit_record( + dandiset=dandiset, user=user, record_type='create_dandiset', details=details + ) + + +def change_owners( + *, dandiset: Dandiset, user: User, removed_owners: list[User], added_owners: list[User] +): + def glean_user_info(user: User): + return { + 'username': user.username, + 'email': user.email, + 'name': f'{user.first_name} {user.last_name}', + } + + details = { + 'removed_owners': [glean_user_info(u) for u in removed_owners], + 'added_owners': [glean_user_info(u) for u in added_owners], + } + return _make_audit_record( + dandiset=dandiset, user=user, record_type='change_owners', details=details + ) + + +def update_metadata(*, dandiset: Dandiset, user: User, metadata: dict) -> AuditRecord: + details = {'metadata': metadata} + return _make_audit_record( + dandiset=dandiset, user=user, record_type='update_metadata', details=details + ) + + +def _asset_details(asset: Asset) -> dict: + checksum = ( + (asset.blob and asset.blob.etag) + or (asset.embargoed_blob and asset.embargoed_blob.etag) + or (asset.zarr and asset.zarr.checksum) + ) + + return { + 'path': asset.path, + 'asset_blob_id': asset.blob and asset.blob.id, + 'embargoed_asset_blob_id': asset.embargoed_blob and asset.embargoed_blob.id, + 'zarr_archive_id': asset.zarr and asset.zarr.id, + 'asset_id': asset.id, + 'checksum': checksum, + 'metadata': asset.metadata, + } + + +def add_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = _asset_details(asset) + return _make_audit_record( + dandiset=dandiset, user=user, record_type='add_asset', details=details + ) + + +def update_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = _asset_details(asset) + return _make_audit_record( + dandiset=dandiset, user=user, record_type='update_asset', details=details + ) + + +def remove_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = { + 'path': asset.path, + 'asset_id': asset.id, + } + return _make_audit_record( + dandiset=dandiset, user=user, record_type='remove_asset', details=details + ) + + +def create_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'name': zarr_archive.name, + } + return _make_audit_record( + dandiset=dandiset, user=user, record_type='create_zarr', details=details + ) + + +def upload_zarr_chunks( + *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] +) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'paths': paths, + } + return _make_audit_record( + dandiset=dandiset, user=user, record_type='upload_zarr_chunks', details=details + ) + + +def delete_zarr_chunks( + *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] +) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'paths': paths, + } + return _make_audit_record( + dandiset=dandiset, user=user, record_type='delete_zarr_chunks', details=details + ) + + +def finalize_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + } + return _make_audit_record( + dandiset=dandiset, user=user, record_type='finalize_zarr', details=details + ) + + +def unembargo_dandiset(*, dandiset: Dandiset, user: User) -> AuditRecord: + return _make_audit_record( + dandiset=dandiset, user=user, record_type='unembargo_dandiset', details={} + ) + + +def publish_dandiset(*, dandiset: Dandiset, user: User, version: str) -> AuditRecord: + details = { + 'version': version, + } + return _make_audit_record( + dandiset=dandiset, user=user, record_type='publish_dandiset', details=details + ) + + +def delete_dandiset(*, dandiset: Dandiset, user: User): + return _make_audit_record( + dandiset=dandiset, user=user, record_type='delete_dandiset', details={} + ) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 3f8785c03..273acdc57 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -3,9 +3,9 @@ from django.db import transaction from guardian.shortcuts import assign_perm -from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version +from dandiapi.api.services import audit from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError from dandiapi.api.services.exceptions import AdminOnlyOperationError, NotAllowedError @@ -48,7 +48,7 @@ def create_dandiset( draft_version.full_clean(validate_constraints=False) draft_version.save() - audit_record = AuditRecord.create_dandiset( + audit_record = audit.create_dandiset( dandiset=dandiset, user=user, metadata=draft_version.metadata, embargoed=embargo ) audit_record.save() @@ -71,7 +71,7 @@ def delete_dandiset(*, user, dandiset: Dandiset) -> None: with transaction.atomic(): # Record the audit event first so that the AuditRecord instance has a # chance to grab the Dandiset information before it is destroyed. - audit_record = AuditRecord.delete_dandiset(dandiset=dandiset, user=user) + audit_record = audit.delete_dandiset(dandiset=dandiset, user=user) audit_record.save() dandiset.versions.all().delete() diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 2dfe3429a..6afb8395c 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -10,7 +10,8 @@ from more_itertools import chunked from dandiapi.api.mail import send_dandiset_unembargoed_message -from dandiapi.api.models import AssetBlob, AuditRecord, Dandiset, Version +from dandiapi.api.models import AssetBlob, Dandiset, Version +from dandiapi.api.services import audit from dandiapi.api.services.asset.exceptions import DandisetOwnerRequiredError from dandiapi.api.services.exceptions import DandiError from dandiapi.api.storage import get_boto_client @@ -102,7 +103,7 @@ def unembargo_dandiset(ds: Dandiset, user: User): logger.info('...Done') - audit_record = AuditRecord.unembargo_dandiset(dandiset=ds, user=user) + audit_record = audit.unembargo_dandiset(dandiset=ds, user=user) audit_record.save() diff --git a/dandiapi/api/services/publish/__init__.py b/dandiapi/api/services/publish/__init__.py index e1a559458..b62da5090 100644 --- a/dandiapi/api/services/publish/__init__.py +++ b/dandiapi/api/services/publish/__init__.py @@ -10,7 +10,8 @@ from dandiapi.api import doi from dandiapi.api.asset_paths import add_version_asset_paths -from dandiapi.api.models import Asset, AuditRecord, Dandiset, Version +from dandiapi.api.models import Asset, Dandiset, Version +from dandiapi.api.services import audit from dandiapi.api.services.exceptions import NotAllowedError from dandiapi.api.services.publish.exceptions import ( DandisetAlreadyPublishedError, @@ -191,7 +192,7 @@ def _create_doi(version_id: int): transaction.on_commit(lambda: _create_doi(new_version.id)) user = User.objects.get(id=user_id) - audit_record = AuditRecord.publish_dandiset( + audit_record = audit.publish_dandiset( dandiset=new_version.dandiset, user=user, version=new_version.version ) audit_record.save() diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index c6c1867b4..cc1755476 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -24,7 +24,8 @@ from dandiapi.api.asset_paths import get_root_paths_many from dandiapi.api.mail import send_ownership_change_emails -from dandiapi.api.models import AuditRecord, Dandiset, Version +from dandiapi.api.models import Dandiset, Version +from dandiapi.api.services import audit from dandiapi.api.services.dandiset import create_dandiset, delete_dandiset from dandiapi.api.services.embargo import kickoff_dandiset_unembargo from dandiapi.api.services.embargo.exceptions import ( @@ -419,7 +420,7 @@ def users(self, request, dandiset__pk): # noqa: C901 dandiset.save() if removed_owners or added_owners: - audit_record = AuditRecord.change_owners( + audit_record = audit.change_owners( dandiset=dandiset, user=request.user, removed_owners=removed_owners, diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index f065f62d5..e24fd2c0a 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -13,7 +13,8 @@ from rest_framework.viewsets import ReadOnlyModelViewSet from rest_framework_extensions.mixins import DetailSerializerMixin, NestedViewSetMixin -from dandiapi.api.models import AuditRecord, Dandiset, Version +from dandiapi.api.models import Dandiset, Version +from dandiapi.api.services import audit from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError from dandiapi.api.services.publish import publish_dandiset from dandiapi.api.tasks import delete_doi_task @@ -120,7 +121,7 @@ def update(self, request, **kwargs): locked_version.status = Version.Status.PENDING locked_version.save() - audit_record = AuditRecord.update_metadata( + audit_record = audit.update_metadata( dandiset=locked_version.dandiset, user=request.user, metadata=locked_version.metadata, From 6e4c2bb964aa6b3ac55c8a567591cef06e40a23b Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 21:07:09 -0400 Subject: [PATCH 25/37] Eliminate the need for manually calling .save() Now that the audit records are generated within service functions, it makes less sense to leak the detail of an AuditRecord instance coming from the function that must then be saved to the database--the service itself can just take care of this. --- dandiapi/api/services/asset/__init__.py | 9 +++------ dandiapi/api/services/audit/__init__.py | 5 ++++- dandiapi/api/services/dandiset/__init__.py | 6 ++---- dandiapi/api/services/embargo/__init__.py | 3 +-- dandiapi/api/services/publish/__init__.py | 3 +-- dandiapi/api/views/dandiset.py | 3 +-- dandiapi/api/views/version.py | 3 +-- 7 files changed, 13 insertions(+), 19 deletions(-) diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index d295517e8..3025eafd2 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -99,8 +99,7 @@ def change_asset( # noqa: PLR0913 new_asset.previous = asset new_asset.save() - audit_record = audit.update_asset(dandiset=version.dandiset, user=user, asset=asset) - audit_record.save() + audit.update_asset(dandiset=version.dandiset, user=user, asset=asset) return new_asset, True @@ -165,8 +164,7 @@ def add_asset_to_version( # noqa: PLR0913, C901 version.save() if do_audit: - audit_record = audit.add_asset(dandiset=version.dandiset, user=user, asset=asset) - audit_record.save() + audit.add_asset(dandiset=version.dandiset, user=user, asset=asset) # Perform this after the above transaction has finished, to ensure we only # operate on unembargoed asset blobs @@ -195,7 +193,6 @@ def remove_asset_from_version( version.save() if do_audit: - audit_record = audit.remove_asset(dandiset=version.dandiset, user=user, asset=asset) - audit_record.save() + audit.remove_asset(dandiset=version.dandiset, user=user, asset=asset) return version diff --git a/dandiapi/api/services/audit/__init__.py b/dandiapi/api/services/audit/__init__.py index f595a8996..7cef6fda4 100644 --- a/dandiapi/api/services/audit/__init__.py +++ b/dandiapi/api/services/audit/__init__.py @@ -16,7 +16,7 @@ def _make_audit_record( *, dandiset: Dandiset, user: User, record_type: AuditRecordType, details: dict ) -> AuditRecord: - return AuditRecord( + audit_record = AuditRecord( dandiset_id=dandiset.id, username=user.username, user_email=user.email, @@ -24,6 +24,9 @@ def _make_audit_record( record_type=record_type, details=details, ) + audit_record.save() + + return audit_record def create_dandiset(*, dandiset: Dandiset, user: User, metadata: dict, embargoed: bool): diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 273acdc57..20067be60 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -48,10 +48,9 @@ def create_dandiset( draft_version.full_clean(validate_constraints=False) draft_version.save() - audit_record = audit.create_dandiset( + audit.create_dandiset( dandiset=dandiset, user=user, metadata=draft_version.metadata, embargoed=embargo ) - audit_record.save() return dandiset, draft_version @@ -71,8 +70,7 @@ def delete_dandiset(*, user, dandiset: Dandiset) -> None: with transaction.atomic(): # Record the audit event first so that the AuditRecord instance has a # chance to grab the Dandiset information before it is destroyed. - audit_record = audit.delete_dandiset(dandiset=dandiset, user=user) - audit_record.save() + audit.delete_dandiset(dandiset=dandiset, user=user) dandiset.versions.all().delete() dandiset.delete() diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 6afb8395c..4f6791d16 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -103,8 +103,7 @@ def unembargo_dandiset(ds: Dandiset, user: User): logger.info('...Done') - audit_record = audit.unembargo_dandiset(dandiset=ds, user=user) - audit_record.save() + audit.unembargo_dandiset(dandiset=ds, user=user) def remove_asset_blob_embargoed_tag(asset_blob: AssetBlob) -> None: diff --git a/dandiapi/api/services/publish/__init__.py b/dandiapi/api/services/publish/__init__.py index b62da5090..882fa0c7c 100644 --- a/dandiapi/api/services/publish/__init__.py +++ b/dandiapi/api/services/publish/__init__.py @@ -192,10 +192,9 @@ def _create_doi(version_id: int): transaction.on_commit(lambda: _create_doi(new_version.id)) user = User.objects.get(id=user_id) - audit_record = audit.publish_dandiset( + audit.publish_dandiset( dandiset=new_version.dandiset, user=user, version=new_version.version ) - audit_record.save() def publish_dandiset(*, user: User, dandiset: Dandiset) -> None: diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index cc1755476..282523d47 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -420,13 +420,12 @@ def users(self, request, dandiset__pk): # noqa: C901 dandiset.save() if removed_owners or added_owners: - audit_record = audit.change_owners( + audit.change_owners( dandiset=dandiset, user=request.user, removed_owners=removed_owners, added_owners=added_owners, ) - audit_record.save() send_ownership_change_emails(dandiset, removed_owners, added_owners) diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index e24fd2c0a..51a6542d2 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -121,12 +121,11 @@ def update(self, request, **kwargs): locked_version.status = Version.Status.PENDING locked_version.save() - audit_record = audit.update_metadata( + audit.update_metadata( dandiset=locked_version.dandiset, user=request.user, metadata=locked_version.metadata, ) - audit_record.save() serializer = VersionDetailSerializer(instance=locked_version) return Response(serializer.data, status=status.HTTP_200_OK) From 2c2ce5d74b82ab971aadf0a878e1cd49b8eb5852 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 24 Jul 2024 21:10:39 -0400 Subject: [PATCH 26/37] Use audit service in zarr views --- dandiapi/zarr/views/__init__.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index 986a83401..a6110107f 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -14,8 +14,8 @@ from rest_framework.utils.urls import replace_query_param from rest_framework.viewsets import ReadOnlyModelViewSet -from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.dandiset import Dandiset +from dandiapi.api.services import audit from dandiapi.api.storage import get_boto_client from dandiapi.api.views.pagination import DandiPagination from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus @@ -143,10 +143,7 @@ def create(self, request): with transaction.atomic(): zarr_archive.save() - audit_record = AuditRecord.create_zarr( - dandiset=dandiset, user=request.user, zarr_archive=zarr_archive - ) - audit_record.save() + audit.create_zarr(dandiset=dandiset, user=request.user, zarr_archive=zarr_archive) except IntegrityError as e: raise ValidationError('Zarr already exists') from e @@ -182,10 +179,9 @@ def finalize(self, request, zarr_id): zarr_archive.status = ZarrArchiveStatus.UPLOADED zarr_archive.save() - audit_record = AuditRecord.finalize_zarr( + audit.finalize_zarr( dandiset=zarr_archive.dandiset, user=request.user, zarr_archive=zarr_archive ) - audit_record.save() # Dispatch task ingest_zarr_archive.delay(zarr_id=zarr_archive.zarr_id) @@ -302,13 +298,12 @@ def create_files(self, request, zarr_id): zarr_archive.mark_pending() zarr_archive.save() - audit_record = AuditRecord.upload_zarr_chunks( + audit.upload_zarr_chunks( dandiset=zarr_archive.dandiset, user=request.user, zarr_archive=zarr_archive, paths=[p['path'] for p in paths], ) - audit_record.save() # Return presigned urls logger.info( @@ -341,12 +336,11 @@ def delete_files(self, request, zarr_id): paths = [file['path'] for file in serializer.validated_data] zarr_archive.delete_files(paths) - audit_record = AuditRecord.delete_zarr_chunks( + audit.delete_zarr_chunks( dandiset=zarr_archive.dandiset, user=request.user, zarr_archive=zarr_archive, paths=paths, ) - audit_record.save() return Response(None, status=status.HTTP_204_NO_CONTENT) From c5380dec75454479397242c3a3727d3929f2c2bc Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Thu, 25 Jul 2024 20:02:02 -0400 Subject: [PATCH 27/37] Remove references to deleted model fields The `blob`/`embargoed_blob` distinction was removed in the embargo redesign. --- dandiapi/api/services/audit/__init__.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/dandiapi/api/services/audit/__init__.py b/dandiapi/api/services/audit/__init__.py index 7cef6fda4..12f49d892 100644 --- a/dandiapi/api/services/audit/__init__.py +++ b/dandiapi/api/services/audit/__init__.py @@ -66,16 +66,11 @@ def update_metadata(*, dandiset: Dandiset, user: User, metadata: dict) -> AuditR def _asset_details(asset: Asset) -> dict: - checksum = ( - (asset.blob and asset.blob.etag) - or (asset.embargoed_blob and asset.embargoed_blob.etag) - or (asset.zarr and asset.zarr.checksum) - ) + checksum = (asset.blob and asset.blob.etag) or (asset.zarr and asset.zarr.checksum) return { 'path': asset.path, 'asset_blob_id': asset.blob and asset.blob.id, - 'embargoed_asset_blob_id': asset.embargoed_blob and asset.embargoed_blob.id, 'zarr_archive_id': asset.zarr and asset.zarr.id, 'asset_id': asset.id, 'checksum': checksum, From 9501e0331b4eb61d9dba1d5edb4055c35e527137 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Thu, 25 Jul 2024 20:03:04 -0400 Subject: [PATCH 28/37] Remove duplicate task launch line --- dandiapi/api/services/embargo/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 4f6791d16..0452174db 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -122,7 +122,6 @@ def kickoff_dandiset_unembargo(*, user: User, dandiset: Dandiset): if not user.has_perm('owner', dandiset): raise DandisetOwnerRequiredError - unembargo_dandiset_task.delay(dandiset.id, user.id) if dandiset.uploads.count(): raise DandisetActiveUploadsError From 9fa7b416ceeaaff8c0186f5eb5d76dfe780a7ccd Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Thu, 25 Jul 2024 20:04:13 -0400 Subject: [PATCH 29/37] Fix invocation of unembargo routines in tests --- dandiapi/api/services/embargo/__init__.py | 2 +- dandiapi/api/tests/test_unembargo.py | 25 +++++++++++++++-------- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 0452174db..565a518fe 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -129,4 +129,4 @@ def kickoff_dandiset_unembargo(*, user: User, dandiset: Dandiset): Dandiset.objects.filter(pk=dandiset.pk).update( embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) - transaction.on_commit(lambda: unembargo_dandiset_task.delay(dandiset.pk)) + transaction.on_commit(lambda: unembargo_dandiset_task.delay(dandiset.pk, user.id)) diff --git a/dandiapi/api/tests/test_unembargo.py b/dandiapi/api/tests/test_unembargo.py index 0d6eff3e8..3fd7a878e 100644 --- a/dandiapi/api/tests/test_unembargo.py +++ b/dandiapi/api/tests/test_unembargo.py @@ -94,28 +94,34 @@ def test_kickoff_dandiset_unembargo(api_client, user, draft_version_factory, mai # Check that unembargo dandiset task was delayed assert len(patched_task.mock_calls) == 1 - assert str(patched_task.mock_calls[0]) == f'call.delay({ds.pk})' + assert str(patched_task.mock_calls[0]) == f'call.delay({ds.pk}, {user.id})' @pytest.mark.django_db() -def test_unembargo_dandiset_not_unembargoing(draft_version_factory): +def test_unembargo_dandiset_not_unembargoing(draft_version_factory, user, api_client): draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) ds: Dandiset = draft_version.dandiset + assign_perm('owner', user, ds) + api_client.force_authenticate(user=user) + with pytest.raises(DandiError): - unembargo_dandiset(ds) + unembargo_dandiset(ds, user) @pytest.mark.django_db() -def test_unembargo_dandiset_uploads_exist(draft_version_factory, upload_factory): +def test_unembargo_dandiset_uploads_exist(draft_version_factory, upload_factory, user, api_client): draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) ds: Dandiset = draft_version.dandiset + assign_perm('owner', user, ds) + api_client.force_authenticate(user=user) + upload_factory(dandiset=ds) with pytest.raises(DandisetActiveUploadsError): - unembargo_dandiset(ds) + unembargo_dandiset(ds, user) @pytest.mark.django_db() @@ -191,7 +197,7 @@ def test_unembargo_dandiset( # Patch this function to check if it's been called, since we can't test the tagging directly patched = mocker.patch('dandiapi.api.services.embargo._delete_asset_blob_tags') - unembargo_dandiset(ds) + unembargo_dandiset(ds, owners[0]) patched.assert_called_once() embargoed_blob.refresh_from_db() @@ -218,13 +224,16 @@ def test_unembargo_dandiset( @pytest.mark.django_db() -def test_unembargo_dandiset_task_failure(draft_version_factory, mailoutbox): +def test_unembargo_dandiset_task_failure(draft_version_factory, mailoutbox, user, api_client): # Intentionally set the status to embargoed so the task will fail draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) ds: Dandiset = draft_version.dandiset + assign_perm('owner', user, ds) + api_client.force_authenticate(user=user) + with pytest.raises(DandiError): - unembargo_dandiset_task.delay(ds.pk) + unembargo_dandiset_task.delay(ds.pk, user.id) assert mailoutbox assert 'Unembargo failed' in mailoutbox[0].subject From 222a15f80a46d7afab059791bc5a918d5e762ebb Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Fri, 26 Jul 2024 15:23:37 -0400 Subject: [PATCH 30/37] Add tests --- dandiapi/api/tests/test_audit.py | 293 +++++++++++++++++++++++++++++++ 1 file changed, 293 insertions(+) create mode 100644 dandiapi/api/tests/test_audit.py diff --git a/dandiapi/api/tests/test_audit.py b/dandiapi/api/tests/test_audit.py new file mode 100644 index 000000000..deec0fa60 --- /dev/null +++ b/dandiapi/api/tests/test_audit.py @@ -0,0 +1,293 @@ +from __future__ import annotations + +import base64 +import hashlib +from typing import TYPE_CHECKING + +import pytest + +from dandiapi.api.models import AuditRecord, Dandiset +from dandiapi.api.storage import get_boto_client +from dandiapi.zarr.models import ZarrArchive + +if TYPE_CHECKING: + from django.contrib.auth import User + + from dandiapi.api.models.audit import AuditRecordType + + +def verify_model_properties(rec: AuditRecord, user: User): + """Assert the correct content of the common AuditRecord fields.""" + assert rec.username == user.username + assert rec.user_email == user.email + assert rec.user_fullname == f'{user.first_name} {user.last_name}' + + +def get_latest_audit_record(*, dandiset: Dandiset, record_type: AuditRecordType): + """Get the most recent AuditRecord associated with a given Dandiset and record type.""" + record = ( + AuditRecord.objects.filter(dandiset_id=dandiset.id, record_type=record_type) + .order_by('-timestamp') + .first() + ) + assert record is not None + + return record + + +def create_dandiset( + api_client, + *, + user: User, + name: str | None = None, + metadata: dict | None = None, + embargoed: bool = False, +) -> Dandiset: + """Create a Dandiset for testing through the REST API.""" + name = name or 'Test Dandiset' + metadata = metadata or {} + + api_client.force_authenticate(user=user) + resp = api_client.post( + f'/api/dandisets/{"?embargo=true" if embargoed else ""}', + {'name': name, 'metadata': metadata}, + format='json', + ) + assert resp.status_code == 200 + + dandiset_id = int(resp.json()['identifier']) + return Dandiset.objects.get(pk=dandiset_id) + + +@pytest.mark.django_db() +def test_audit_dandiset_lifecycle(user_factory, api_client): + """Test the audit records generated during the mainline Dandiset lifecycle.""" + # Create some users. + alice = user_factory() + bob = user_factory() + charlie = user_factory() + + # Create a Dandiset with specified name and metadata. + name = 'Dandiset Extraordinaire' + metadata = {'foo': 'bar'} + dandiset = create_dandiset(api_client, user=alice, name=name, metadata=metadata) + + # Verify the create_dandiset audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='create_dandiset') + verify_model_properties(rec, alice) + assert rec.details['embargoed'] is False + for key, value in metadata.items(): + assert rec.details['metadata'][key] == value + assert rec.details['metadata']['name'] == name + + # Change the owners. + new_owners = [bob, charlie] + resp = api_client.put( + f'/api/dandisets/{dandiset.identifier}/users/', + [{'username': u.username} for u in new_owners], + format='json', + ) + assert resp.status_code == 200 + + # Verify the change_owners audit record. + def user_info(u): + return { + 'username': u.username, + 'email': u.email, + 'name': f'{u.first_name} {u.last_name}', + } + + rec = get_latest_audit_record(dandiset=dandiset, record_type='change_owners') + verify_model_properties(rec, alice) + assert rec.details == { + 'added_owners': [user_info(u) for u in [bob, charlie]], + 'removed_owners': [user_info(u) for u in [alice]], + } + + # Edit the metadata. + metadata = dandiset.draft_version.metadata + metadata['foo'] = 'baz' + + api_client.force_authenticate(user=bob) + resp = api_client.put( + f'/api/dandisets/{dandiset.identifier}/versions/draft/', + { + 'name': 'bar', + 'metadata': metadata, + }, + format='json', + ) + assert resp.status_code == 200 + + # Verify the update_metadata audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='update_metadata') + verify_model_properties(rec, bob) + metadata = rec.details['metadata'] + assert metadata['name'] == 'bar' + assert metadata['foo'] == 'baz' + + # Delete the dandiset. + resp = api_client.delete(f'/api/dandisets/{dandiset.identifier}/') + assert resp.status_code == 204 + + # Verify the delete_dandiset audit record + rec = get_latest_audit_record(dandiset=dandiset, record_type='delete_dandiset') + verify_model_properties(rec, bob) + assert rec.details == {} + + +@pytest.mark.django_db(transaction=True) +def test_audit_unembargo(user, api_client): + """Test the unembargo audit record.""" + # Create an embargoed Dandiset. + dandiset = create_dandiset(api_client, user=user, embargoed=True) + + # Verify the create_dandiset record's embargoed field. + rec = get_latest_audit_record(dandiset=dandiset, record_type='create_dandiset') + verify_model_properties(rec, user) + assert rec.details['embargoed'] is True + + # Unembargo the Dandiset. + resp = api_client.post(f'/api/dandisets/{dandiset.identifier}/unembargo/') + assert resp.status_code == 200 + + # Verify the unembargo_dandiset audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='unembargo_dandiset') + verify_model_properties(rec, user) + assert rec.details == {} + + +@pytest.mark.django_db() +def test_audit_asset(user, api_client, asset_blob_factory): + """Test the blob-based asset audit records.""" + # Create a dandiset and some asset blobs. + dandiset = create_dandiset(api_client, user=user) + blob1 = asset_blob_factory() + blob2 = asset_blob_factory() + + # Create a new asset. + path = 'foo/bar.txt' + resp = api_client.post( + f'/api/dandisets/{dandiset.identifier}/versions/draft/assets/', + { + 'blob_id': str(blob1.blob_id), + 'metadata': { + 'path': path, + }, + }, + ) + assert resp.status_code == 200 + + # Verify add_asset audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='add_asset') + verify_model_properties(rec, user) + assert rec.details['path'] == path + assert rec.details['asset_blob_id'] == blob1.blob_id + + # Update the asset "in place". + asset_id = rec.details['asset_id'] + resp = api_client.put( + f'/api/dandisets/{dandiset.identifier}/versions/draft/assets/{asset_id}/', + { + 'blob_id': str(blob2.blob_id), + 'metadata': { + 'path': path, + }, + }, + ) + assert resp.status_code == 200 + + # Verify update_asset audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='update_asset') + verify_model_properties(rec, user) + assert rec.details['path'] == path + assert rec.details['asset_blob_id'] == blob2.blob_id + + # Delete the asset. + asset_id = rec.details['asset_id'] + resp = api_client.delete( + f'/api/dandisets/{dandiset.identifier}/versions/draft/assets/{rec.details["asset_id"]}/', + ) + assert resp.status_code == 204 + + # Verify remove_asset audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='remove_asset') + verify_model_properties(rec, user) + assert rec.details['path'] == path + assert rec.details['asset_id'] == asset_id + + +@pytest.mark.django_db() +def test_audit_zarr(user, api_client, storage, monkeypatch, settings): + """Test the Zarr-based audit records.""" + # Monkeypatch ZarrArchive so that foobar. + monkeypatch.setattr(ZarrArchive, 'storage', storage) + + # Create a Dandiset. + dandiset = create_dandiset(api_client, user=user) + + # Create a Zarr archive. + resp = api_client.post( + '/api/zarr/', + { + 'name': 'Test Zarr', + 'dandiset': dandiset.identifier, + }, + ) + assert resp.status_code == 200 + + # Verify the create_zarr audit record. + zarr_id = resp.json()['zarr_id'] + + rec = get_latest_audit_record(dandiset=dandiset, record_type='create_zarr') + verify_model_properties(rec, user) + assert rec.details['name'] == 'Test Zarr' + assert rec.details['zarr_id'] == zarr_id + + # Request some chunk uploads. + b64hash = base64.b64encode(hashlib.md5(b'a').hexdigest().encode()) + paths = ['a.txt', 'b.txt', 'c.txt'] + resp = api_client.post( + f'/api/zarr/{zarr_id}/files/', + [{'path': path, 'base64md5': b64hash} for path in paths], + ) + assert resp.status_code == 200 + + # Verify the upload_zarr_chunks audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='upload_zarr_chunks') + verify_model_properties(rec, user) + assert rec.details['zarr_id'] == zarr_id + assert rec.details['paths'] == paths + + # Upload to the presigned URLs. + boto = get_boto_client() + zarr_archive = ZarrArchive.objects.get(zarr_id=zarr_id) + for path in paths: + boto.put_object( + Bucket=settings.DANDI_DANDISETS_BUCKET_NAME, Key=zarr_archive.s3_path(path), Body=b'a' + ) + + # Finalize the zarr. + resp = api_client.post( + f'/api/zarr/{zarr_id}/finalize/', + ) + assert resp.status_code == 204 + + # Verify the finalize_zarr audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='finalize_zarr') + verify_model_properties(rec, user) + assert rec.details['zarr_id'] == zarr_id + + # Delete some zarr chunks. + deleted = ['b.txt', 'c.txt'] + resp = api_client.delete( + f'/api/zarr/{zarr_id}/files/', + [{'path': path} for path in deleted], + ) + assert resp.status_code == 204 + + # Verify the delete_zarr_chunks audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='delete_zarr_chunks') + verify_model_properties(rec, user) + assert rec.details['zarr_id'] == zarr_id + assert rec.details['paths'] == deleted From 3483e503a4b6854fe8362ee14fe49e5fbfe9860d Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Tue, 30 Jul 2024 15:00:01 -0400 Subject: [PATCH 31/37] bugfix: Include correct asset in update_asset record --- dandiapi/api/services/asset/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index 3025eafd2..7e10c7d3b 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -99,7 +99,7 @@ def change_asset( # noqa: PLR0913 new_asset.previous = asset new_asset.save() - audit.update_asset(dandiset=version.dandiset, user=user, asset=asset) + audit.update_asset(dandiset=version.dandiset, user=user, asset=new_asset) return new_asset, True From 4a33a6bbb21b6b9ed6a3e6ea36c1c6a5b732598c Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Tue, 30 Jul 2024 15:00:33 -0400 Subject: [PATCH 32/37] bugfix: Pass correct and stringified ID values in asset/zarr records --- dandiapi/api/services/audit/__init__.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dandiapi/api/services/audit/__init__.py b/dandiapi/api/services/audit/__init__.py index 12f49d892..adad72b1a 100644 --- a/dandiapi/api/services/audit/__init__.py +++ b/dandiapi/api/services/audit/__init__.py @@ -70,9 +70,9 @@ def _asset_details(asset: Asset) -> dict: return { 'path': asset.path, - 'asset_blob_id': asset.blob and asset.blob.id, - 'zarr_archive_id': asset.zarr and asset.zarr.id, - 'asset_id': asset.id, + 'asset_blob_id': asset.blob and str(asset.blob.blob_id), + 'zarr_archive_id': asset.zarr and str(asset.zarr.zarr_id), + 'asset_id': str(asset.asset_id), 'checksum': checksum, 'metadata': asset.metadata, } @@ -95,7 +95,7 @@ def update_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord def remove_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: details = { 'path': asset.path, - 'asset_id': asset.id, + 'asset_id': str(asset.asset_id), } return _make_audit_record( dandiset=dandiset, user=user, record_type='remove_asset', details=details @@ -104,7 +104,7 @@ def remove_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord def create_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: details = { - 'zarr_id': zarr_archive.id, + 'zarr_id': str(zarr_archive.zarr_id), 'name': zarr_archive.name, } return _make_audit_record( @@ -116,7 +116,7 @@ def upload_zarr_chunks( *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] ) -> AuditRecord: details = { - 'zarr_id': zarr_archive.id, + 'zarr_id': str(zarr_archive.zarr_id), 'paths': paths, } return _make_audit_record( @@ -128,7 +128,7 @@ def delete_zarr_chunks( *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] ) -> AuditRecord: details = { - 'zarr_id': zarr_archive.id, + 'zarr_id': str(zarr_archive.zarr_id), 'paths': paths, } return _make_audit_record( @@ -138,7 +138,7 @@ def delete_zarr_chunks( def finalize_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: details = { - 'zarr_id': zarr_archive.id, + 'zarr_id': str(zarr_archive.zarr_id), } return _make_audit_record( dandiset=dandiset, user=user, record_type='finalize_zarr', details=details From b5ad8b2ee8b0f8ac78f063546b612bf45c7bfa1b Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 7 Aug 2024 10:09:43 -0400 Subject: [PATCH 33/37] Split long tests up into individual tests --- dandiapi/api/tests/test_audit.py | 218 ++++++++++++++++++++++++------- 1 file changed, 169 insertions(+), 49 deletions(-) diff --git a/dandiapi/api/tests/test_audit.py b/dandiapi/api/tests/test_audit.py index deec0fa60..19fcd89f1 100644 --- a/dandiapi/api/tests/test_audit.py +++ b/dandiapi/api/tests/test_audit.py @@ -4,6 +4,7 @@ import hashlib from typing import TYPE_CHECKING +from guardian.shortcuts import assign_perm import pytest from dandiapi.api.models import AuditRecord, Dandiset @@ -60,28 +61,36 @@ def create_dandiset( @pytest.mark.django_db() -def test_audit_dandiset_lifecycle(user_factory, api_client): - """Test the audit records generated during the mainline Dandiset lifecycle.""" - # Create some users. - alice = user_factory() - bob = user_factory() - charlie = user_factory() - +def test_audit_create_dandiset(api_client, user): + """Test create_dandiset audit record.""" # Create a Dandiset with specified name and metadata. name = 'Dandiset Extraordinaire' metadata = {'foo': 'bar'} - dandiset = create_dandiset(api_client, user=alice, name=name, metadata=metadata) + dandiset = create_dandiset(api_client, user=user, name=name, metadata=metadata) # Verify the create_dandiset audit record. rec = get_latest_audit_record(dandiset=dandiset, record_type='create_dandiset') - verify_model_properties(rec, alice) + verify_model_properties(rec, user) assert rec.details['embargoed'] is False for key, value in metadata.items(): assert rec.details['metadata'][key] == value assert rec.details['metadata']['name'] == name + +@pytest.mark.django_db() +def test_audit_change_owners(api_client, user_factory, draft_version): + """Test the change_owners audit record.""" + # Create some users. + alice = user_factory() + bob = user_factory() + charlie = user_factory() + + dandiset = draft_version.dandiset + assign_perm('owner', alice, dandiset) + # Change the owners. new_owners = [bob, charlie] + api_client.force_authenticate(user=alice) resp = api_client.put( f'/api/dandisets/{dandiset.identifier}/users/', [{'username': u.username} for u in new_owners], @@ -104,15 +113,22 @@ def user_info(u): 'removed_owners': [user_info(u) for u in [alice]], } - # Edit the metadata. - metadata = dandiset.draft_version.metadata - metadata['foo'] = 'baz' - api_client.force_authenticate(user=bob) +@pytest.mark.django_db() +def test_audit_update_metadata(api_client, draft_version, user): + # Create a Dandiset. + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) + + # Edit its metadata. + metadata = draft_version.metadata + metadata['foo'] = 'bar' + + api_client.force_authenticate(user=user) resp = api_client.put( f'/api/dandisets/{dandiset.identifier}/versions/draft/', { - 'name': 'bar', + 'name': 'baz', 'metadata': metadata, }, format='json', @@ -121,23 +137,31 @@ def user_info(u): # Verify the update_metadata audit record. rec = get_latest_audit_record(dandiset=dandiset, record_type='update_metadata') - verify_model_properties(rec, bob) + verify_model_properties(rec, user) metadata = rec.details['metadata'] - assert metadata['name'] == 'bar' - assert metadata['foo'] == 'baz' + assert metadata['name'] == 'baz' + assert metadata['foo'] == 'bar' + + +@pytest.mark.django_db() +def test_audit_delete_dandiset(api_client, user, draft_version): + # Create a Dandiset. + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) # Delete the dandiset. + api_client.force_authenticate(user=user) resp = api_client.delete(f'/api/dandisets/{dandiset.identifier}/') assert resp.status_code == 204 # Verify the delete_dandiset audit record rec = get_latest_audit_record(dandiset=dandiset, record_type='delete_dandiset') - verify_model_properties(rec, bob) + verify_model_properties(rec, user) assert rec.details == {} @pytest.mark.django_db(transaction=True) -def test_audit_unembargo(user, api_client): +def test_audit_unembargo(api_client, user): """Test the unembargo audit record.""" # Create an embargoed Dandiset. dandiset = create_dandiset(api_client, user=user, embargoed=True) @@ -158,19 +182,19 @@ def test_audit_unembargo(user, api_client): @pytest.mark.django_db() -def test_audit_asset(user, api_client, asset_blob_factory): - """Test the blob-based asset audit records.""" - # Create a dandiset and some asset blobs. - dandiset = create_dandiset(api_client, user=user) - blob1 = asset_blob_factory() - blob2 = asset_blob_factory() - - # Create a new asset. +def test_audit_add_asset(api_client, user, draft_version, asset_blob_factory): + # Create a Dandiset. + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) + + # Add a new asset. + blob = asset_blob_factory() path = 'foo/bar.txt' + api_client.force_authenticate(user=user) resp = api_client.post( f'/api/dandisets/{dandiset.identifier}/versions/draft/assets/', { - 'blob_id': str(blob1.blob_id), + 'blob_id': str(blob.blob_id), 'metadata': { 'path': path, }, @@ -182,14 +206,29 @@ def test_audit_asset(user, api_client, asset_blob_factory): rec = get_latest_audit_record(dandiset=dandiset, record_type='add_asset') verify_model_properties(rec, user) assert rec.details['path'] == path - assert rec.details['asset_blob_id'] == blob1.blob_id + assert rec.details['asset_blob_id'] == blob.blob_id + + +@pytest.mark.django_db() +def test_audit_update_asset( + api_client, user, draft_version, asset_blob_factory, draft_asset_factory +): + # Create a Dandiset with an asset. + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) + + path = 'foo/bar.txt' + asset = draft_asset_factory(path=path) + asset.versions.add(draft_version) - # Update the asset "in place". - asset_id = rec.details['asset_id'] + # Replace the asset. + asset_id = asset.asset_id + blob = asset_blob_factory() + api_client.force_authenticate(user=user) resp = api_client.put( f'/api/dandisets/{dandiset.identifier}/versions/draft/assets/{asset_id}/', { - 'blob_id': str(blob2.blob_id), + 'blob_id': str(blob.blob_id), 'metadata': { 'path': path, }, @@ -201,12 +240,26 @@ def test_audit_asset(user, api_client, asset_blob_factory): rec = get_latest_audit_record(dandiset=dandiset, record_type='update_asset') verify_model_properties(rec, user) assert rec.details['path'] == path - assert rec.details['asset_blob_id'] == blob2.blob_id + assert rec.details['asset_blob_id'] == blob.blob_id + + +@pytest.mark.django_db() +def test_audit_remove_asset( + api_client, user, draft_version, asset_blob_factory, draft_asset_factory +): + # Create a Dandiset with an asset. + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) + + path = 'foo/bar.txt' + asset = draft_asset_factory(path=path) + asset.versions.add(draft_version) # Delete the asset. - asset_id = rec.details['asset_id'] + asset_id = asset.asset_id + api_client.force_authenticate(user=user) resp = api_client.delete( - f'/api/dandisets/{dandiset.identifier}/versions/draft/assets/{rec.details["asset_id"]}/', + f'/api/dandisets/{dandiset.identifier}/versions/draft/assets/{asset_id}/', ) assert resp.status_code == 204 @@ -214,19 +267,17 @@ def test_audit_asset(user, api_client, asset_blob_factory): rec = get_latest_audit_record(dandiset=dandiset, record_type='remove_asset') verify_model_properties(rec, user) assert rec.details['path'] == path - assert rec.details['asset_id'] == asset_id + assert rec.details['asset_id'] == str(asset_id) @pytest.mark.django_db() -def test_audit_zarr(user, api_client, storage, monkeypatch, settings): - """Test the Zarr-based audit records.""" - # Monkeypatch ZarrArchive so that foobar. - monkeypatch.setattr(ZarrArchive, 'storage', storage) - +def test_audit_zarr_create(api_client, user, draft_version): # Create a Dandiset. - dandiset = create_dandiset(api_client, user=user) + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) # Create a Zarr archive. + api_client.force_authenticate(user=user) resp = api_client.post( '/api/zarr/', { @@ -244,11 +295,22 @@ def test_audit_zarr(user, api_client, storage, monkeypatch, settings): assert rec.details['name'] == 'Test Zarr' assert rec.details['zarr_id'] == zarr_id + +@pytest.mark.django_db() +def test_audit_upload_zarr_chunks(api_client, user, draft_version, zarr_archive_factory, storage): + ZarrArchive.storage = storage + + # Create a Dandiset and a Zarr archive. + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) + zarr = zarr_archive_factory(dandiset=dandiset) + # Request some chunk uploads. b64hash = base64.b64encode(hashlib.md5(b'a').hexdigest().encode()) paths = ['a.txt', 'b.txt', 'c.txt'] + api_client.force_authenticate(user=user) resp = api_client.post( - f'/api/zarr/{zarr_id}/files/', + f'/api/zarr/{zarr.zarr_id}/files/', [{'path': path, 'base64md5': b64hash} for path in paths], ) assert resp.status_code == 200 @@ -256,12 +318,34 @@ def test_audit_zarr(user, api_client, storage, monkeypatch, settings): # Verify the upload_zarr_chunks audit record. rec = get_latest_audit_record(dandiset=dandiset, record_type='upload_zarr_chunks') verify_model_properties(rec, user) - assert rec.details['zarr_id'] == zarr_id + assert rec.details['zarr_id'] == zarr.zarr_id assert rec.details['paths'] == paths + +@pytest.mark.django_db() +def test_audit_finalize_zarr( + api_client, user, draft_version, zarr_archive_factory, storage, settings +): + ZarrArchive.storage = storage + + # Create a Dandiset and a Zarr archive. + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) + zarr = zarr_archive_factory(dandiset=dandiset) + + # Request some chunk uploads. + b64hash = base64.b64encode(hashlib.md5(b'a').hexdigest().encode()) + paths = ['a.txt', 'b.txt', 'c.txt'] + api_client.force_authenticate(user=user) + resp = api_client.post( + f'/api/zarr/{zarr.zarr_id}/files/', + [{'path': path, 'base64md5': b64hash} for path in paths], + ) + assert resp.status_code == 200 + # Upload to the presigned URLs. boto = get_boto_client() - zarr_archive = ZarrArchive.objects.get(zarr_id=zarr_id) + zarr_archive = ZarrArchive.objects.get(zarr_id=zarr.zarr_id) for path in paths: boto.put_object( Bucket=settings.DANDI_DANDISETS_BUCKET_NAME, Key=zarr_archive.s3_path(path), Body=b'a' @@ -269,19 +353,55 @@ def test_audit_zarr(user, api_client, storage, monkeypatch, settings): # Finalize the zarr. resp = api_client.post( - f'/api/zarr/{zarr_id}/finalize/', + f'/api/zarr/{zarr.zarr_id}/finalize/', ) assert resp.status_code == 204 # Verify the finalize_zarr audit record. rec = get_latest_audit_record(dandiset=dandiset, record_type='finalize_zarr') verify_model_properties(rec, user) - assert rec.details['zarr_id'] == zarr_id + assert rec.details['zarr_id'] == zarr.zarr_id + + +@pytest.mark.django_db() +def test_audit_delete_zarr_chunks( + api_client, user, draft_version, zarr_archive_factory, storage, settings +): + ZarrArchive.storage = storage + + # Create a Dandiset and a Zarr archive. + dandiset = draft_version.dandiset + assign_perm('owner', user, dandiset) + zarr = zarr_archive_factory(dandiset=dandiset) + + # Request some chunk uploads. + b64hash = base64.b64encode(hashlib.md5(b'a').hexdigest().encode()) + paths = ['a.txt', 'b.txt', 'c.txt'] + api_client.force_authenticate(user=user) + resp = api_client.post( + f'/api/zarr/{zarr.zarr_id}/files/', + [{'path': path, 'base64md5': b64hash} for path in paths], + ) + assert resp.status_code == 200 + + # Upload to the presigned URLs. + boto = get_boto_client() + zarr_archive = ZarrArchive.objects.get(zarr_id=zarr.zarr_id) + for path in paths: + boto.put_object( + Bucket=settings.DANDI_DANDISETS_BUCKET_NAME, Key=zarr_archive.s3_path(path), Body=b'a' + ) + + # Finalize the zarr. + resp = api_client.post( + f'/api/zarr/{zarr.zarr_id}/finalize/', + ) + assert resp.status_code == 204 # Delete some zarr chunks. deleted = ['b.txt', 'c.txt'] resp = api_client.delete( - f'/api/zarr/{zarr_id}/files/', + f'/api/zarr/{zarr.zarr_id}/files/', [{'path': path} for path in deleted], ) assert resp.status_code == 204 @@ -289,5 +409,5 @@ def test_audit_zarr(user, api_client, storage, monkeypatch, settings): # Verify the delete_zarr_chunks audit record. rec = get_latest_audit_record(dandiset=dandiset, record_type='delete_zarr_chunks') verify_model_properties(rec, user) - assert rec.details['zarr_id'] == zarr_id + assert rec.details['zarr_id'] == zarr.zarr_id assert rec.details['paths'] == deleted From 7e04c83b8d4c0e081f6cc7b0420dd721731ea014 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Thu, 8 Aug 2024 11:31:31 -0400 Subject: [PATCH 34/37] Use nested transaction to handle integrity error The notes in the "avoid catching exceptions" callout here (https://docs.djangoproject.com/en/5.0/topics/db/transactions/#controlling-transactions-explicitly) say to do something like this to isolate error handling without stepping on the operation of the transactions themselves. --- dandiapi/zarr/views/__init__.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index a6110107f..3b9ed48ff 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -139,13 +139,14 @@ def create(self, request): if dandiset.embargo_status != Dandiset.EmbargoStatus.OPEN: raise ValidationError('Cannot add zarr to embargoed dandiset') zarr_archive: ZarrArchive = ZarrArchive(name=name, dandiset=dandiset) - try: - with transaction.atomic(): - zarr_archive.save() + with transaction.atomic(): + try: + with transaction.atomic(): + zarr_archive.save() + except IntegrityError as e: + raise ValidationError('Zarr already exists') from e - audit.create_zarr(dandiset=dandiset, user=request.user, zarr_archive=zarr_archive) - except IntegrityError as e: - raise ValidationError('Zarr already exists') from e + audit.create_zarr(dandiset=dandiset, user=request.user, zarr_archive=zarr_archive) serializer = ZarrSerializer(instance=zarr_archive) return Response(serializer.data, status=status.HTTP_200_OK) From 1a0faf7a49ceafd1db19e0e3113d2c1c792213ad Mon Sep 17 00:00:00 2001 From: Roni Choudhury <2903332+waxlamp@users.noreply.github.com> Date: Thu, 8 Aug 2024 16:48:25 -0400 Subject: [PATCH 35/37] Add explanatory comment Co-authored-by: Jacob Nesbitt --- dandiapi/zarr/views/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index 3b9ed48ff..39510008d 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -140,6 +140,7 @@ def create(self, request): raise ValidationError('Cannot add zarr to embargoed dandiset') zarr_archive: ZarrArchive = ZarrArchive(name=name, dandiset=dandiset) with transaction.atomic(): + # Use nested transaction block to prevent zarr creation race condition try: with transaction.atomic(): zarr_archive.save() From 9a2a2b2084c7031783a6d0e547e467e13b336726 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Thu, 8 Aug 2024 20:06:10 -0400 Subject: [PATCH 36/37] Disable complexity warning on users view --- dandiapi/api/views/dandiset.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index f78c57b63..0e6790ee3 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -376,7 +376,7 @@ def unembargo(self, request, dandiset__pk): ) # TODO: move these into a viewset @action(methods=['GET', 'PUT'], detail=True) - def users(self, request, dandiset__pk): + def users(self, request, dandiset__pk): # noqa: C901 dandiset: Dandiset = self.get_object() if request.method == 'PUT': if dandiset.unembargo_in_progress: From 8af0957985aabdd2839c4fda9b91d2a850ed9e45 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Fri, 9 Aug 2024 15:18:25 -0400 Subject: [PATCH 37/37] Add test for publish_dandiset audit record --- dandiapi/api/tests/test_audit.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/dandiapi/api/tests/test_audit.py b/dandiapi/api/tests/test_audit.py index 19fcd89f1..ced98fc70 100644 --- a/dandiapi/api/tests/test_audit.py +++ b/dandiapi/api/tests/test_audit.py @@ -7,7 +7,9 @@ from guardian.shortcuts import assign_perm import pytest +from dandiapi.api.asset_paths import add_version_asset_paths from dandiapi.api.models import AuditRecord, Dandiset +from dandiapi.api.services.metadata import validate_asset_metadata, validate_version_metadata from dandiapi.api.storage import get_boto_client from dandiapi.zarr.models import ZarrArchive @@ -270,6 +272,36 @@ def test_audit_remove_asset( assert rec.details['asset_id'] == str(asset_id) +@pytest.mark.django_db(transaction=True) +def test_audit_publish_dandiset( + api_client, user, dandiset_factory, draft_version_factory, draft_asset_factory +): + # Create a Dandiset whose draft version has one asset. + dandiset = dandiset_factory() + assign_perm('owner', user, dandiset) + draft_version = draft_version_factory(dandiset=dandiset) + draft_asset = draft_asset_factory() + draft_version.assets.add(draft_asset) + add_version_asset_paths(draft_version) + + # Validate the asset and then the draft version (to make it publishable + # through the API). + validate_asset_metadata(asset=draft_asset) + validate_version_metadata(version=draft_version) + + # Publish the Dandiset. + api_client.force_authenticate(user=user) + resp = api_client.post( + f'/api/dandisets/{dandiset.identifier}/versions/draft/publish/', + ) + assert resp.status_code == 202 + + # Verify publish_dandiset audit record. + rec = get_latest_audit_record(dandiset=dandiset, record_type='publish_dandiset') + verify_model_properties(rec, user) + assert rec.details['version'] == dandiset.most_recent_published_version.version + + @pytest.mark.django_db() def test_audit_zarr_create(api_client, user, draft_version): # Create a Dandiset.