From 6fbec8fdbdbe9c306b367e5531364d5c954ccfb1 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 17 Dec 2024 18:00:15 -0500 Subject: [PATCH 01/11] Create permissions service layer * Move logic from DandisetManager.visible_to into permission service layer. This makes the DandisetManager class obsolete, and removes it. * Move Dandiset.set_owners to permission service layer. --- dandiapi/api/models/dandiset.py | 52 +------------ dandiapi/api/services/permissions/__init__.py | 0 dandiapi/api/services/permissions/dandiset.py | 78 +++++++++++++++++++ dandiapi/api/tests/test_dandiset.py | 12 ++- dandiapi/api/views/dandiset.py | 8 +- dandiapi/api/views/upload.py | 3 +- dandiapi/zarr/tests/test_zarr.py | 5 +- dandiapi/zarr/views/__init__.py | 5 +- 8 files changed, 102 insertions(+), 61 deletions(-) create mode 100644 dandiapi/api/services/permissions/__init__.py create mode 100644 dandiapi/api/services/permissions/dandiset.py diff --git a/dandiapi/api/models/dandiset.py b/dandiapi/api/models/dandiset.py index 97cb8bff3..f44a1bdd8 100644 --- a/dandiapi/api/models/dandiset.py +++ b/dandiapi/api/models/dandiset.py @@ -3,41 +3,12 @@ from django.db import models from django_extensions.db.models import TimeStampedModel from guardian.models import GroupObjectPermissionBase, UserObjectPermissionBase -from guardian.shortcuts import assign_perm, get_objects_for_user, get_users_with_perms, remove_perm - - -class DandisetManager(models.Manager): - def visible_to(self, user) -> models.QuerySet[Dandiset]: - """ - Return a queryset containing all dandisets visible to the given user. - - This is basically all dandisets except embargoed dandisets not owned by the given user. - """ - # We would like to do something like: - # Dandiset.filter(embargo_status=OPEN | permission__owned) - # but this is not possible with django-guardian; the `get_objects_for_user` shortcut must - # be used instead. - # We would like to do something like: - # queryset = Dandiset.objects.filter(embargo_status=OPEN).union(get_objects_for_user(...)) - # but you cannot filter or perform many other common queryset operations after using - # union(). - # Therefore, we resort to fetching the list of all primary keys for dandisets owned by the - # current user with `get_objects_for_user(...)`, then filter for either those dandisets or - # OPEN dandisets. There aren't very many dandisets and most users won't own very many of - # them, so there shouldn't be too many dandisets in the list. - owned_dandiset_pks = get_objects_for_user(user, 'owner', Dandiset).values('pk').all() - return self.filter( - models.Q(embargo_status=Dandiset.EmbargoStatus.OPEN) - | models.Q(pk__in=owned_dandiset_pks) - ).order_by('created') class Dandiset(TimeStampedModel): # Don't add beginning and end markers, so this can be embedded in larger regexes IDENTIFIER_REGEX = r'\d{6}' - objects: DandisetManager = DandisetManager() - class EmbargoStatus(models.TextChoices): EMBARGOED = 'EMBARGOED', 'Embargoed' UNEMBARGOING = 'UNEMBARGOING', 'Unembargoing' @@ -76,28 +47,9 @@ def draft_version(self): @property def owners(self): - return get_users_with_perms(self, only_with_perms_in=['owner']).order_by('date_joined') - - def set_owners(self, new_owners): - old_owners = get_users_with_perms(self, only_with_perms_in=['owner']) - - removed_owners = [] - added_owners = [] - - # Remove old owners - for old_owner in old_owners: - if old_owner not in new_owners: - remove_perm('owner', old_owner, self) - removed_owners.append(old_owner) - - # Add new owners - for new_owner in new_owners: - if new_owner not in old_owners: - assign_perm('owner', new_owner, self) - added_owners.append(new_owner) + from dandiapi.api.services.permissions.dandiset import get_dandiset_owners - # Return the owners added/removed so they can be emailed - return removed_owners, added_owners + return get_dandiset_owners(self) @classmethod def published_count(cls): diff --git a/dandiapi/api/services/permissions/__init__.py b/dandiapi/api/services/permissions/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/dandiapi/api/services/permissions/dandiset.py b/dandiapi/api/services/permissions/dandiset.py new file mode 100644 index 000000000..8fab66f6e --- /dev/null +++ b/dandiapi/api/services/permissions/dandiset.py @@ -0,0 +1,78 @@ +"""Abstracts over django guardian to provide an internal permission framework.""" + +from __future__ import annotations + +import typing + +from django.contrib.auth.models import AnonymousUser, User +from django.db import transaction +from django.db.models import Q, QuerySet +from guardian.shortcuts import assign_perm, get_objects_for_user, get_users_with_perms + +from dandiapi.api.models.dandiset import Dandiset, DandisetUserObjectPermission + +if typing.TYPE_CHECKING: + from django.contrib.auth.base_user import AbstractBaseUser + + +def get_dandiset_owners(dandiset: Dandiset) -> QuerySet[User]: + qs = typing.cast(QuerySet[User], get_users_with_perms(dandiset, only_with_perms_in=['owner'])) + return qs.order_by('date_joined') + + +@transaction.atomic +def replace_dandiset_owners(dandiset: Dandiset, users: list[User]): + existing_owners = get_dandiset_owners(dandiset) + existing_owner_set = set(existing_owners) + new_owner_set = set(users) + + # Delete all existing owners + DandisetUserObjectPermission.objects.filter( + content_object=dandiset.pk, permission__codename='owner' + ).delete() + + # Set owners to new list + for user in users: + assign_perm('owner', user, dandiset) + + # Return the owners added/removed so they can be emailed + removed_owners = existing_owner_set - new_owner_set + added_owners = new_owner_set - existing_owner_set + return removed_owners, added_owners + + +def is_dandiset_owner(dandiset: Dandiset, user: AbstractBaseUser | AnonymousUser) -> bool: + if isinstance(user, AnonymousUser): + return False + + user = typing.cast(User, user) + return user.has_perm('owner', dandiset) + + +def get_owned_dandisets(user: AbstractBaseUser | AnonymousUser) -> QuerySet[Dandiset]: + return get_objects_for_user(user, 'owner', Dandiset) + + +def get_visible_dandisets(user: AbstractBaseUser | AnonymousUser) -> QuerySet[Dandiset]: + """ + Return a queryset containing all dandisets visible to the given user. + + This is basically all dandisets except embargoed dandisets not owned by the given user. + """ + # We would like to do something like: + # Dandiset.filter(embargo_status=OPEN | permission__owned) + # but this is not possible with django-guardian; the `get_objects_for_user` shortcut must + # be used instead. + # We would like to do something like: + # queryset = Dandiset.objects.filter(embargo_status=OPEN).union(get_objects_for_user(...)) + # but you cannot filter or perform many other common queryset operations after using + # union(). + # Therefore, we resort to fetching the list of all primary keys for dandisets owned by the + # current user with `get_objects_for_user(...)`, then filter for either those dandisets or + # OPEN dandisets. There aren't very many dandisets and most users won't own very many of + # them, so there shouldn't be too many dandisets in the list. + + owned_dandiset_pks = get_owned_dandisets(user).values('pk').all() + return Dandiset.objects.filter( + Q(embargo_status=Dandiset.EmbargoStatus.OPEN) | Q(pk__in=owned_dandiset_pks) + ).order_by('created') diff --git a/dandiapi/api/tests/test_dandiset.py b/dandiapi/api/tests/test_dandiset.py index 899c2069d..fe213cee2 100644 --- a/dandiapi/api/tests/test_dandiset.py +++ b/dandiapi/api/tests/test_dandiset.py @@ -9,6 +9,10 @@ from dandiapi.api.asset_paths import add_asset_paths, add_version_asset_paths from dandiapi.api.models import Dandiset, Version +from dandiapi.api.services.permissions.dandiset import ( + get_visible_dandisets, + replace_dandiset_owners, +) from .fuzzy import DANDISET_ID_RE, DANDISET_SCHEMA_ID_RE, TIMESTAMP_RE, UTC_ISO_TIMESTAMP_RE @@ -53,15 +57,15 @@ def test_dandiset_published_count( ], ) @pytest.mark.django_db -def test_dandiset_manager_visible_to( +def test_dandiset_get_visible_dandisets( dandiset_factory, user_factory, embargo_status, user_status, visible ): dandiset = dandiset_factory(embargo_status=embargo_status) - user = AnonymousUser if user_status == 'anonymous' else user_factory() + user = AnonymousUser() if user_status == 'anonymous' else user_factory() if user_status == 'owner': assign_perm('owner', user, dandiset) - assert list(Dandiset.objects.visible_to(user)) == ([dandiset] if visible else []) + assert list(get_visible_dandisets(user)) == ([dandiset] if visible else []) @pytest.mark.django_db @@ -251,7 +255,7 @@ def test_dandiset_rest_retrieve_embargoed(api_client, dandiset_factory, user): resp = api_client.get(f'/api/dandisets/{dandiset.identifier}/') assert resp.status_code == 403 - dandiset.set_owners([user]) + replace_dandiset_owners(dandiset, [user]) resp = api_client.get(f'/api/dandisets/{dandiset.identifier}/') assert resp.status_code == 200 diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 867dd4319..6d863c8c0 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -33,6 +33,10 @@ DandisetUnembargoInProgressError, UnauthorizedEmbargoAccessError, ) +from dandiapi.api.services.permissions.dandiset import ( + get_visible_dandisets, + replace_dandiset_owners, +) from dandiapi.api.views.common import DANDISET_PK_PARAM from dandiapi.api.views.pagination import DandiPagination from dandiapi.api.views.serializers import ( @@ -118,7 +122,7 @@ def get_queryset(self): # Only include embargoed dandisets which belong to the current user queryset = Dandiset.objects if self.action in ['list', 'search']: - queryset = Dandiset.objects.visible_to(self.request.user).order_by('created') + queryset = get_visible_dandisets(self.request.user).order_by('created') query_serializer = DandisetQueryParameterSerializer(data=self.request.query_params) query_serializer.is_valid(raise_exception=True) @@ -427,7 +431,7 @@ def users(self, request, dandiset__pk): # noqa: C901 # All owners found with transaction.atomic(): owners = user_owners + [acc.user for acc in socialaccount_owners] - removed_owners, added_owners = dandiset.set_owners(owners) + removed_owners, added_owners = replace_dandiset_owners(dandiset, owners) dandiset.save() if removed_owners or added_owners: diff --git a/dandiapi/api/views/upload.py b/dandiapi/api/views/upload.py index 9ef08bfc4..861c0b890 100644 --- a/dandiapi/api/views/upload.py +++ b/dandiapi/api/views/upload.py @@ -18,6 +18,7 @@ from dandiapi.api.models import AssetBlob, Dandiset, Upload from dandiapi.api.permissions import IsApproved from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError +from dandiapi.api.services.permissions.dandiset import get_visible_dandisets from dandiapi.api.tasks import calculate_sha256 from dandiapi.api.views.serializers import AssetBlobSerializer @@ -131,7 +132,7 @@ def upload_initialize_view(request: Request) -> HttpResponseBase: etag = digest['value'] dandiset_id = request_serializer.validated_data['dandiset'] dandiset = get_object_or_404( - Dandiset.objects.visible_to(request.user), + get_visible_dandisets(request.user), id=dandiset_id, ) response = get_40x_or_None(request, ['owner'], dandiset, return_403=True) diff --git a/dandiapi/zarr/tests/test_zarr.py b/dandiapi/zarr/tests/test_zarr.py index b38813c8a..913349773 100644 --- a/dandiapi/zarr/tests/test_zarr.py +++ b/dandiapi/zarr/tests/test_zarr.py @@ -6,6 +6,7 @@ from zarr_checksum.checksum import EMPTY_CHECKSUM from dandiapi.api.models.dandiset import Dandiset +from dandiapi.api.services.permissions.dandiset import replace_dandiset_owners from dandiapi.api.tests.fuzzy import UUID_RE from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus from dandiapi.zarr.tasks import ingest_zarr_archive @@ -134,7 +135,7 @@ def test_zarr_rest_get_embargoed(authenticated_api_client, user, embargoed_zarr_ resp = authenticated_api_client.get(f'/api/zarr/{embargoed_zarr_archive.zarr_id}/') assert resp.status_code == 404 - embargoed_zarr_archive.dandiset.set_owners([user]) + replace_dandiset_owners(embargoed_zarr_archive.dandiset, [user]) resp = authenticated_api_client.get(f'/api/zarr/{embargoed_zarr_archive.zarr_id}/') assert resp.status_code == 200 @@ -150,7 +151,7 @@ def test_zarr_rest_list_embargoed(authenticated_api_client, user, dandiset, zarr assert sorted(z['zarr_id'] for z in zarrs) == sorted(z.zarr_id for z in open_zarrs) # Assert that all zarrs returned when user has access to embargoed zarrs - dandiset.set_owners([user]) + replace_dandiset_owners(dandiset, [user]) zarrs = authenticated_api_client.get('/api/zarr/').json()['results'] assert len(zarrs) == len(open_zarrs + embargoed_zarrs) assert sorted(z['zarr_id'] for z in zarrs) == sorted( diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index dd8a47c78..211107ee0 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -17,6 +17,7 @@ from dandiapi.api.models.dandiset import Dandiset, DandisetUserObjectPermission from dandiapi.api.services import audit +from dandiapi.api.services.permissions.dandiset import get_visible_dandisets, is_dandiset_owner from dandiapi.api.storage import get_boto_client from dandiapi.api.views.pagination import DandiPagination from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus @@ -149,9 +150,9 @@ def create(self, request): name = serializer.validated_data['name'] dandiset = get_object_or_404( - Dandiset.objects.visible_to(request.user), id=serializer.validated_data['dandiset'] + get_visible_dandisets(request.user), id=serializer.validated_data['dandiset'] ) - if not self.request.user.has_perm('owner', dandiset): + if not is_dandiset_owner(dandiset, request.user): raise PermissionDenied zarr_archive: ZarrArchive = ZarrArchive(name=name, dandiset=dandiset) with transaction.atomic(): From c763e79a31d285ec21a517a826bd41a6276152e9 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 17 Dec 2024 18:14:10 -0500 Subject: [PATCH 02/11] Replace all functional uses of assign_perm with add_dandiset_owner --- dandiapi/api/services/dandiset/__init__.py | 4 +- dandiapi/api/services/permissions/dandiset.py | 6 +- dandiapi/api/tests/test_asset.py | 60 +++++++++---------- dandiapi/api/tests/test_audit.py | 24 ++++---- dandiapi/api/tests/test_dandiset.py | 38 ++++++------ dandiapi/api/tests/test_tasks.py | 4 +- dandiapi/api/tests/test_unembargo.py | 18 +++--- dandiapi/api/tests/test_upload.py | 24 ++++---- dandiapi/api/tests/test_version.py | 30 +++++----- .../zarr/tests/test_ingest_zarr_archive.py | 4 +- dandiapi/zarr/tests/test_zarr.py | 19 +++--- dandiapi/zarr/tests/test_zarr_upload.py | 8 +-- 12 files changed, 121 insertions(+), 118 deletions(-) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 20067be60..2da3504c3 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -1,7 +1,6 @@ from __future__ import annotations from django.db import transaction -from guardian.shortcuts import assign_perm from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version @@ -9,6 +8,7 @@ from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError from dandiapi.api.services.exceptions import AdminOnlyOperationError, NotAllowedError +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner from dandiapi.api.services.version.metadata import _normalize_version_metadata @@ -38,7 +38,7 @@ def create_dandiset( dandiset = Dandiset(id=identifier, embargo_status=embargo_status) dandiset.full_clean() dandiset.save() - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) draft_version = Version( dandiset=dandiset, name=version_name, diff --git a/dandiapi/api/services/permissions/dandiset.py b/dandiapi/api/services/permissions/dandiset.py index 8fab66f6e..895464f13 100644 --- a/dandiapi/api/services/permissions/dandiset.py +++ b/dandiapi/api/services/permissions/dandiset.py @@ -20,6 +20,10 @@ def get_dandiset_owners(dandiset: Dandiset) -> QuerySet[User]: return qs.order_by('date_joined') +def add_dandiset_owner(dandiset: Dandiset, user: User): + assign_perm('owner', user, dandiset) + + @transaction.atomic def replace_dandiset_owners(dandiset: Dandiset, users: list[User]): existing_owners = get_dandiset_owners(dandiset) @@ -33,7 +37,7 @@ def replace_dandiset_owners(dandiset: Dandiset, users: list[User]): # Set owners to new list for user in users: - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) # Return the owners added/removed so they can be emailed removed_owners = existing_owner_set - new_owner_set diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index adba44319..061e81178 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -7,7 +7,6 @@ from django.conf import settings from django.db.utils import IntegrityError from django.urls import reverse -from guardian.shortcuts import assign_perm import pytest import requests @@ -17,6 +16,7 @@ from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.services.asset import add_asset_to_version from dandiapi.api.services.asset.exceptions import AssetPathConflictError +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner from dandiapi.api.services.publish import publish_asset from dandiapi.api.tasks.scheduled import validate_pending_asset_metadata from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus @@ -563,7 +563,7 @@ def test_asset_rest_validation(api_client, version, asset, status, validation_er @pytest.mark.django_db def test_asset_create(api_client, user, draft_version, asset_blob): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -637,7 +637,7 @@ def test_asset_create(api_client, user, draft_version, asset_blob): def test_asset_create_path_validation( api_client, user, draft_version, asset_blob, path, expected_status_code ): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) metadata = { @@ -658,7 +658,7 @@ def test_asset_create_path_validation( @pytest.mark.django_db def test_asset_create_conflicting_path(api_client, user, draft_version, asset_blob): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) # Add first asset @@ -704,7 +704,7 @@ def test_asset_create_embargo( dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) draft_version = draft_version_factory(dandiset=dandiset) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) assert draft_version.dandiset.embargo_status == Dandiset.EmbargoStatus.EMBARGOED @@ -746,7 +746,7 @@ def test_asset_create_unembargo_in_progress( dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING) draft_version = draft_version_factory(dandiset=dandiset) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -777,7 +777,7 @@ def test_asset_create_embargoed_asset_blob_open_dandiset( assert draft_version.dandiset.embargo_status == Dandiset.EmbargoStatus.OPEN assert embargoed_asset_blob.embargoed - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -813,7 +813,7 @@ def test_asset_create_embargoed_asset_blob_open_dandiset( @pytest.mark.django_db def test_asset_create_zarr(api_client, user, draft_version, zarr_archive): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -863,7 +863,7 @@ def test_asset_create_zarr(api_client, user, draft_version, zarr_archive): def test_asset_create_zarr_validated( api_client, user, draft_version, zarr_archive, zarr_file_factory ): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -912,7 +912,7 @@ def test_asset_create_zarr_validated( def test_asset_create_zarr_wrong_dandiset( api_client, user, draft_version, zarr_archive_factory, dandiset_factory ): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) zarr_dandiset = dandiset_factory() @@ -939,7 +939,7 @@ def test_asset_create_zarr_wrong_dandiset( @pytest.mark.django_db def test_asset_create_no_blob_or_zarr(api_client, user, draft_version): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -963,7 +963,7 @@ def test_asset_create_no_blob_or_zarr(api_client, user, draft_version): @pytest.mark.django_db def test_asset_create_blob_and_zarr(api_client, user, draft_version, asset_blob, zarr_archive): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -987,7 +987,7 @@ def test_asset_create_blob_and_zarr(api_client, user, draft_version, asset_blob, @pytest.mark.django_db def test_asset_create_no_valid_blob(api_client, user, draft_version): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -1005,7 +1005,7 @@ def test_asset_create_no_valid_blob(api_client, user, draft_version): @pytest.mark.django_db def test_asset_create_no_path(api_client, user, draft_version, asset_blob): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) metadata = {'meta': 'data', 'foo': ['bar', 'baz'], '1': 2} @@ -1036,7 +1036,7 @@ def test_asset_create_not_an_owner(api_client, user, version): @pytest.mark.django_db def test_asset_create_published_version(api_client, user, published_version, asset): - assign_perm('owner', user, published_version.dandiset) + add_dandiset_owner(published_version.dandiset, user) api_client.force_authenticate(user=user) published_version.assets.add(asset) @@ -1057,7 +1057,7 @@ def test_asset_create_published_version(api_client, user, published_version, ass @pytest.mark.django_db def test_asset_create_existing_path(api_client, user, draft_version, asset_blob, asset_factory): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) path = 'test/create/asset.txt' @@ -1090,7 +1090,7 @@ def test_asset_create_on_open_dandiset_embargoed_asset_blob( assert embargoed_asset_blob.embargoed - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user) path = 'test/create/asset.txt' @@ -1117,7 +1117,7 @@ def test_asset_create_on_open_dandiset_embargoed_asset_blob( @pytest.mark.django_db def test_asset_rest_rename(api_client, user, draft_version, asset_blob): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) # Create asset @@ -1143,7 +1143,7 @@ def test_asset_rest_rename(api_client, user, draft_version, asset_blob): @pytest.mark.django_db def test_asset_rest_update(api_client, user, draft_version, asset, asset_blob): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) draft_version.assets.add(asset) add_asset_paths(asset=asset, version=draft_version) @@ -1201,7 +1201,7 @@ def test_asset_rest_update(api_client, user, draft_version, asset, asset_blob): @pytest.mark.django_db def test_asset_rest_update_embargo(api_client, user, draft_version, asset, embargoed_asset_blob): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) draft_version.assets.add(asset) add_asset_paths(asset=asset, version=draft_version) @@ -1259,7 +1259,7 @@ def test_asset_rest_update_unembargo_in_progress( draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) draft_version.assets.add(asset) @@ -1290,7 +1290,7 @@ def test_asset_rest_update_zarr( zarr_archive, zarr_file_factory, ): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) asset = draft_asset_factory(blob=None, zarr=zarr_archive) @@ -1383,7 +1383,7 @@ def test_asset_rest_update_not_an_owner(api_client, user, draft_version, asset): @pytest.mark.django_db def test_asset_rest_update_published_version(api_client, user, published_version, asset): - assign_perm('owner', user, published_version.dandiset) + add_dandiset_owner(published_version.dandiset, user) api_client.force_authenticate(user=user) published_version.assets.add(asset) @@ -1406,7 +1406,7 @@ def test_asset_rest_update_to_existing(api_client, user, draft_version, asset_fa draft_version.assets.add(old_asset) draft_version.assets.add(existing_asset) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) # Set path so API request succeeds @@ -1422,7 +1422,7 @@ def test_asset_rest_update_to_existing(api_client, user, draft_version, asset_fa @pytest.mark.django_db def test_asset_rest_delete(api_client, user, draft_version, asset): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) draft_version.assets.add(asset) # Add paths @@ -1457,7 +1457,7 @@ def test_asset_rest_delete_unembargo_in_progress(api_client, user, draft_version draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) draft_version.assets.add(asset) # Make request @@ -1479,7 +1479,7 @@ def test_asset_rest_delete_zarr( zarr_file_factory, ): asset = draft_asset_factory(blob=None, zarr=zarr_archive) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) draft_version.assets.add(asset) # Add paths @@ -1515,7 +1515,7 @@ def test_asset_rest_delete_zarr_modified( # Assign perms and authenticate user dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) api_client.force_authenticate(user=user) # Ensure zarr is ingested @@ -1575,7 +1575,7 @@ def test_asset_rest_delete_not_an_owner(api_client, user, version, asset): @pytest.mark.django_db def test_asset_rest_delete_published_version(api_client, user, published_version, asset): api_client.force_authenticate(user=user) - assign_perm('owner', user, published_version.dandiset) + add_dandiset_owner(published_version.dandiset, user) published_version.assets.add(asset) response = api_client.delete( @@ -1632,7 +1632,7 @@ def test_asset_download_embargo( ) # Assign perms and set client - assign_perm('owner', user, version.dandiset) + add_dandiset_owner(version.dandiset, user) client = authenticated_api_client # Generate assets and blobs diff --git a/dandiapi/api/tests/test_audit.py b/dandiapi/api/tests/test_audit.py index 6562755b6..fd3f7c0d9 100644 --- a/dandiapi/api/tests/test_audit.py +++ b/dandiapi/api/tests/test_audit.py @@ -4,12 +4,12 @@ import hashlib from typing import TYPE_CHECKING -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.services.permissions.dandiset import add_dandiset_owner from dandiapi.api.storage import get_boto_client from dandiapi.zarr.models import ZarrArchive @@ -88,7 +88,7 @@ def test_audit_change_owners(api_client, user_factory, draft_version): charlie = user_factory() dandiset = draft_version.dandiset - assign_perm('owner', alice, dandiset) + add_dandiset_owner(dandiset, alice) # Change the owners. new_owners = [bob, charlie] @@ -120,7 +120,7 @@ def user_info(u): def test_audit_update_metadata(api_client, draft_version, user): # Create a Dandiset. dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) # Edit its metadata. metadata = draft_version.metadata @@ -149,7 +149,7 @@ def test_audit_update_metadata(api_client, draft_version, user): def test_audit_delete_dandiset(api_client, user, draft_version): # Create a Dandiset. dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) # Delete the dandiset. api_client.force_authenticate(user=user) @@ -187,7 +187,7 @@ def test_audit_unembargo(api_client, user): 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_dandiset_owner(dandiset, user) # Add a new asset. blob = asset_blob_factory() @@ -217,7 +217,7 @@ def test_audit_update_asset( ): # Create a Dandiset with an asset. dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) path = 'foo/bar.txt' asset = draft_asset_factory(path=path) @@ -251,7 +251,7 @@ def test_audit_remove_asset( ): # Create a Dandiset with an asset. dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) path = 'foo/bar.txt' asset = draft_asset_factory(path=path) @@ -278,7 +278,7 @@ def test_audit_publish_dandiset( ): # Create a Dandiset whose draft version has one asset. dandiset = dandiset_factory() - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) draft_version = draft_version_factory(dandiset=dandiset) draft_asset = draft_asset_factory() draft_version.assets.add(draft_asset) @@ -306,7 +306,7 @@ def test_audit_publish_dandiset( def test_audit_zarr_create(api_client, user, draft_version): # Create a Dandiset. dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) # Create a Zarr archive. api_client.force_authenticate(user=user) @@ -334,7 +334,7 @@ def test_audit_upload_zarr_chunks(api_client, user, draft_version, zarr_archive_ # Create a Dandiset and a Zarr archive. dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) zarr = zarr_archive_factory(dandiset=dandiset) # Request some chunk uploads. @@ -362,7 +362,7 @@ def test_audit_finalize_zarr( # Create a Dandiset and a Zarr archive. dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) zarr = zarr_archive_factory(dandiset=dandiset) # Request some chunk uploads. @@ -403,7 +403,7 @@ def test_audit_delete_zarr_chunks( # Create a Dandiset and a Zarr archive. dandiset = draft_version.dandiset - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) zarr = zarr_archive_factory(dandiset=dandiset) # Request some chunk uploads. diff --git a/dandiapi/api/tests/test_dandiset.py b/dandiapi/api/tests/test_dandiset.py index fe213cee2..fb2db2668 100644 --- a/dandiapi/api/tests/test_dandiset.py +++ b/dandiapi/api/tests/test_dandiset.py @@ -4,12 +4,12 @@ from django.conf import settings from django.contrib.auth.models import AnonymousUser -from guardian.shortcuts import assign_perm import pytest from dandiapi.api.asset_paths import add_asset_paths, add_version_asset_paths from dandiapi.api.models import Dandiset, Version from dandiapi.api.services.permissions.dandiset import ( + add_dandiset_owner, get_visible_dandisets, replace_dandiset_owners, ) @@ -63,7 +63,7 @@ def test_dandiset_get_visible_dandisets( dandiset = dandiset_factory(embargo_status=embargo_status) user = AnonymousUser() if user_status == 'anonymous' else user_factory() if user_status == 'owner': - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) assert list(get_visible_dandisets(user)) == ([dandiset] if visible else []) @@ -213,7 +213,7 @@ def test_dandiset_rest_list_for_user(api_client, user, dandiset_factory): # Create an extra dandiset that should not be included in the response dandiset_factory() api_client.force_authenticate(user=user) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) assert api_client.get('/api/dandisets/?user=me&draft=true&empty=true').data == { 'count': 1, 'next': None, @@ -272,7 +272,7 @@ def test_dandiset_rest_embargo_access( owner = user_factory() unauthorized_user = user_factory() dandiset = dandiset_factory(embargo_status=embargo_status) - assign_perm('owner', owner, dandiset) + add_dandiset_owner(dandiset, owner) # This is what authorized users should get from the retrieve endpoint expected_dandiset_serialization = { @@ -771,7 +771,7 @@ def test_dandiset_rest_delete(api_client, draft_version_factory, user, embargo_s # Ensure that open or embargoed dandisets can be deleted draft_version = draft_version_factory(dandiset__embargo_status=embargo_status) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) response = api_client.delete(f'/api/dandisets/{draft_version.dandiset.identifier}/') if success: @@ -791,7 +791,7 @@ def test_dandiset_rest_delete_with_zarrs( draft_asset_factory, ): api_client.force_authenticate(user=user) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) zarr = zarr_archive_factory(dandiset=draft_version.dandiset) asset = draft_asset_factory(blob=None, zarr=zarr) @@ -817,7 +817,7 @@ def test_dandiset_rest_delete_not_an_owner(api_client, draft_version, user): @pytest.mark.django_db def test_dandiset_rest_delete_published(api_client, published_version, user): api_client.force_authenticate(user=user) - assign_perm('owner', user, published_version.dandiset) + add_dandiset_owner(published_version.dandiset, user) response = api_client.delete(f'/api/dandisets/{published_version.dandiset.identifier}/') assert response.status_code == 403 @@ -839,7 +839,7 @@ def test_dandiset_rest_delete_published_admin(api_client, published_version, adm @pytest.mark.django_db def test_dandiset_rest_get_owners(api_client, dandiset, social_account): - assign_perm('owner', social_account.user, dandiset) + add_dandiset_owner(dandiset, social_account.user) resp = api_client.get(f'/api/dandisets/{dandiset.identifier}/users/') @@ -855,7 +855,7 @@ def test_dandiset_rest_get_owners(api_client, dandiset, social_account): @pytest.mark.django_db def test_dandiset_rest_get_owners_no_social_account(api_client, dandiset, user): - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) resp = api_client.get(f'/api/dandisets/{dandiset.identifier}/users/') @@ -887,7 +887,7 @@ def test_dandiset_rest_change_owner( user1 = user_factory() user2 = user_factory() social_account2 = social_account_factory(user=user2) - assign_perm('owner', user1, dandiset) + add_dandiset_owner(dandiset, user1) api_client.force_authenticate(user=user1) resp = api_client.put( @@ -929,7 +929,7 @@ def test_dandiset_rest_change_owners_unembargo_in_progress( user2 = user_factory() social_account1 = social_account_factory(user=user1) social_account2 = social_account_factory(user=user2) - assign_perm('owner', user1, dandiset) + add_dandiset_owner(dandiset, user1) api_client.force_authenticate(user=user1) resp = api_client.put( @@ -957,7 +957,7 @@ def test_dandiset_rest_add_owner( user2 = user_factory() social_account1 = social_account_factory(user=user1) social_account2 = social_account_factory(user=user2) - assign_perm('owner', user1, dandiset) + add_dandiset_owner(dandiset, user1) api_client.force_authenticate(user=user1) resp = api_client.put( @@ -1001,8 +1001,8 @@ def test_dandiset_rest_remove_owner( user1 = user_factory() user2 = user_factory() social_account1 = social_account_factory(user=user1) - assign_perm('owner', user1, dandiset) - assign_perm('owner', user2, dandiset) + add_dandiset_owner(dandiset, user1) + add_dandiset_owner(dandiset, user2) api_client.force_authenticate(user=user1) resp = api_client.put( @@ -1040,7 +1040,7 @@ def test_dandiset_rest_not_an_owner(api_client, dandiset, user): @pytest.mark.django_db def test_dandiset_rest_delete_all_owners_fails(api_client, dandiset, user): - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) api_client.force_authenticate(user=user) resp = api_client.put( @@ -1054,7 +1054,7 @@ def test_dandiset_rest_delete_all_owners_fails(api_client, dandiset, user): @pytest.mark.django_db def test_dandiset_rest_add_owner_does_not_exist(api_client, dandiset, user): - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) api_client.force_authenticate(user=user) fake_name = user.username + 'butnotreally' @@ -1069,7 +1069,7 @@ def test_dandiset_rest_add_owner_does_not_exist(api_client, dandiset, user): @pytest.mark.django_db def test_dandiset_rest_add_malformed(api_client, dandiset, user): - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) api_client.force_authenticate(user=user) resp = api_client.put( @@ -1147,7 +1147,7 @@ def test_dandiset_rest_list_active_uploads( ): ds = draft_version.dandiset - assign_perm('owner', user, ds) + add_dandiset_owner(ds, user) upload = upload_factory(dandiset=ds) response = authenticated_api_client.get(f'/api/dandisets/{ds.identifier}/uploads/') @@ -1189,7 +1189,7 @@ def test_dandiset_rest_clear_active_uploads( ): ds = draft_version.dandiset - assign_perm('owner', user, ds) + add_dandiset_owner(ds, user) upload_factory(dandiset=ds) response = authenticated_api_client.get(f'/api/dandisets/{ds.identifier}/uploads/').json() diff --git a/dandiapi/api/tests/test_tasks.py b/dandiapi/api/tests/test_tasks.py index 85020359b..b91dd6095 100644 --- a/dandiapi/api/tests/test_tasks.py +++ b/dandiapi/api/tests/test_tasks.py @@ -8,7 +8,6 @@ from django.conf import settings from django.forms.models import model_to_dict from django.utils import timezone -from guardian.shortcuts import assign_perm import pytest from rest_framework.test import APIClient from zarr_checksum import compute_zarr_checksum @@ -17,6 +16,7 @@ from dandiapi.api import tasks from dandiapi.api.models import Asset, AssetBlob, Version +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner from dandiapi.zarr.models import ZarrArchiveStatus from .fuzzy import URN_RE, UTC_ISO_TIMESTAMP_RE @@ -390,7 +390,7 @@ def test_publish_task( # Create a draft_version in PUBLISHING state draft_version: Version = draft_version_factory(status=Version.Status.PUBLISHING) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) old_draft_asset: Asset = draft_asset_factory(status=Asset.Status.VALID) diff --git a/dandiapi/api/tests/test_unembargo.py b/dandiapi/api/tests/test_unembargo.py index 8fa6149f0..03a77fdc5 100644 --- a/dandiapi/api/tests/test_unembargo.py +++ b/dandiapi/api/tests/test_unembargo.py @@ -3,7 +3,6 @@ from typing import TYPE_CHECKING import dandischema -from guardian.shortcuts import assign_perm import pytest from dandiapi.api.models.dandiset import Dandiset @@ -22,6 +21,7 @@ remove_dandiset_embargo_tags, ) from dandiapi.api.services.exceptions import DandiError +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner from dandiapi.api.storage import get_boto_client from dandiapi.api.tasks import unembargo_dandiset_task from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus, zarr_s3_path @@ -37,7 +37,7 @@ def test_kickoff_dandiset_unembargo_dandiset_not_embargoed( ): dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.OPEN) draft_version_factory(dandiset=dandiset) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) api_client.force_authenticate(user=user) resp = api_client.post(f'/api/dandisets/{dandiset.identifier}/unembargo/') @@ -62,7 +62,7 @@ def test_kickoff_dandiset_unembargo_active_uploads( ): dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) draft_version_factory(dandiset=dandiset) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) api_client.force_authenticate(user=user) # Test that active uploads prevent unembargp @@ -77,7 +77,7 @@ def test_kickoff_dandiset_unembargo(api_client, user, draft_version_factory, mai draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) ds: Dandiset = draft_version.dandiset - assign_perm('owner', user, ds) + add_dandiset_owner(ds, user) api_client.force_authenticate(user=user) # mock this task to check if called @@ -99,7 +99,7 @@ def test_unembargo_dandiset_not_unembargoing(draft_version_factory, user, api_cl draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) ds: Dandiset = draft_version.dandiset - assign_perm('owner', user, ds) + add_dandiset_owner(ds, user) api_client.force_authenticate(user=user) with pytest.raises(DandiError): @@ -113,7 +113,7 @@ def test_unembargo_dandiset_uploads_exist(draft_version_factory, upload_factory, ) ds: Dandiset = draft_version.dandiset - assign_perm('owner', user, ds) + add_dandiset_owner(ds, user) api_client.force_authenticate(user=user) upload_factory(dandiset=ds) @@ -246,7 +246,7 @@ def test_unembargo_dandiset( ds: Dandiset = draft_version.dandiset owners = [user_factory() for _ in range(5)] for user in owners: - assign_perm('owner', user, ds) + add_dandiset_owner(ds, user) embargoed_blob: AssetBlob = embargoed_asset_blob_factory() draft_version.assets.add(asset_factory(blob=embargoed_blob)) @@ -301,7 +301,7 @@ def test_unembargo_dandiset_validate_version_metadata( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) ds: Dandiset = draft_version.dandiset - assign_perm('owner', user, ds) + add_dandiset_owner(ds, user) draft_version.validation_errors = ['error ajhh'] draft_version.status = Version.Status.INVALID @@ -324,7 +324,7 @@ def test_unembargo_dandiset_task_failure(draft_version_factory, mailoutbox, user draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) ds: Dandiset = draft_version.dandiset - assign_perm('owner', user, ds) + add_dandiset_owner(ds, user) api_client.force_authenticate(user=user) with pytest.raises(DandiError): diff --git a/dandiapi/api/tests/test_upload.py b/dandiapi/api/tests/test_upload.py index 8e6eaa462..6a8d6e548 100644 --- a/dandiapi/api/tests/test_upload.py +++ b/dandiapi/api/tests/test_upload.py @@ -3,11 +3,11 @@ import uuid from django.core.files.base import ContentFile -from guardian.shortcuts import assign_perm import pytest import requests from dandiapi.api.models import AssetBlob, Dandiset, Upload +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner from .fuzzy import HTTP_URL_RE, UUID_RE, Re @@ -74,7 +74,7 @@ def test_upload_initialize(api_client, user, dandiset_factory, embargoed): else Dandiset.EmbargoStatus.OPEN ) api_client.force_authenticate(user=user) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) content_size = 123 @@ -113,7 +113,7 @@ def test_upload_initialize(api_client, user, dandiset_factory, embargoed): def test_upload_initialize_unembargo_in_progress(api_client, user, dandiset_factory): dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING) api_client.force_authenticate(user=user) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) content_size = 123 resp = api_client.post( @@ -131,7 +131,7 @@ def test_upload_initialize_unembargo_in_progress(api_client, user, dandiset_fact @pytest.mark.django_db def test_upload_initialize_existing_asset_blob(api_client, user, dandiset, asset_blob): api_client.force_authenticate(user=user) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) resp = api_client.post( '/api/uploads/initialize/', @@ -195,7 +195,7 @@ def test_upload_initialize_embargo_existing_asset_blob( ): api_client.force_authenticate(user=user) dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) # Embargoed assets that are already uploaded publicly don't need to be private resp = api_client.post( @@ -219,7 +219,7 @@ def test_upload_initialize_embargo_existing_embargoed_asset_blob( ): api_client.force_authenticate(user=user) dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) # This embargoed AssetBlob is in the same embargoed dandiset, so it should be deduplicated embargoed_asset_blob = embargoed_asset_blob_factory() @@ -272,7 +272,7 @@ def test_upload_complete(api_client, user, upload): def test_upload_complete_embargo(api_client, user, dandiset_factory, embargoed_upload_factory): api_client.force_authenticate(user=user) dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) embargoed_upload = embargoed_upload_factory(dandiset=dandiset) content_size = 123 @@ -328,7 +328,7 @@ def test_upload_complete_unauthorized(api_client, upload): @pytest.mark.parametrize('content_size', [10, mb(10), mb(12)], ids=['10B', '10MB', '12MB']) def test_upload_initialize_and_complete(api_client, user, dandiset, content_size): api_client.force_authenticate(user=user) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) # Get the presigned upload URL initialization = api_client.post( @@ -384,7 +384,7 @@ def test_upload_initialize_and_complete_embargo( api_client.force_authenticate(user=user) dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) # Get the presigned upload URL initialization = api_client.post( @@ -455,7 +455,7 @@ def test_upload_validate(api_client, user, upload): def test_upload_validate_embargo(api_client, user, dandiset_factory, embargoed_upload_factory): api_client.force_authenticate(user=user) dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) embargoed_upload = embargoed_upload_factory(dandiset=dandiset) resp = api_client.post(f'/api/uploads/{embargoed_upload.upload_id}/validate/') @@ -539,7 +539,7 @@ def test_upload_validate_embargo_existing_assetblob( ): api_client.force_authenticate(user=user) dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) embargoed_upload = embargoed_upload_factory(dandiset=dandiset) # The upload should recognize this preexisting AssetBlob and use it instead @@ -557,7 +557,7 @@ def test_upload_validate_embargo_existing_embargoed_assetblob( ): api_client.force_authenticate(user=user) dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) embargoed_upload = embargoed_upload_factory(dandiset=dandiset) # The upload should recognize this preexisting embargoed AssetBlob and use it instead diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index a85445164..bc935b40b 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -7,12 +7,12 @@ from dandischema.models import AccessType from django.conf import settings from freezegun import freeze_time -from guardian.shortcuts import assign_perm import pytest from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.services.metadata import version_aggregate_assets_summary from dandiapi.api.services.metadata.exceptions import VersionMetadataConcurrentlyModifiedError +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner if TYPE_CHECKING: from django.contrib.auth.models import User @@ -524,7 +524,7 @@ def test_version_rest_info_with_asset( @pytest.mark.django_db def test_version_rest_update(api_client, user, draft_version): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) new_name = 'A unique and special name!' @@ -614,7 +614,7 @@ def test_version_rest_update_unembargo_in_progress(api_client, user, draft_versi draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) new_name = 'A unique and special name!' @@ -637,7 +637,7 @@ def test_version_rest_update_unembargo_in_progress(api_client, user, draft_versi @pytest.mark.django_db def test_version_rest_update_published_version(api_client, user, published_version): - assign_perm('owner', user, published_version.dandiset) + add_dandiset_owner(published_version.dandiset, user) api_client.force_authenticate(user=user) new_name = 'A unique and special name!' @@ -684,7 +684,7 @@ def test_version_rest_update_not_an_owner(api_client, user, version): ) @pytest.mark.django_db def test_version_rest_update_access_values(api_client, user, draft_version, access): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) new_metadata = {**draft_version.metadata, 'access': access} @@ -708,7 +708,7 @@ def test_version_rest_update_access_values(api_client, user, draft_version, acce @pytest.mark.django_db def test_version_rest_update_access_missing(api_client, user, draft_version): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) # Check that the field missing entirely is also okay @@ -734,7 +734,7 @@ def test_version_rest_update_access_missing(api_client, user, draft_version): @pytest.mark.django_db def test_version_rest_update_access_valid(api_client, user, draft_version): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) # Check that extra fields persist @@ -766,7 +766,7 @@ def test_version_rest_publish( draft_asset_factory, published_asset_factory, ): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) old_draft_asset: Asset = draft_asset_factory() @@ -795,7 +795,7 @@ def test_version_rest_publish( @pytest.mark.django_db def test_version_rest_publish_embargo(api_client: APIClient, user: User, draft_version_factory): draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) resp = api_client.post( @@ -812,7 +812,7 @@ def test_version_rest_publish_unembargo_in_progress( draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) resp = api_client.post( @@ -831,7 +831,7 @@ def test_version_rest_publish_zarr( zarr_archive_factory, zarr_file_factory, ): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) # create and ingest zarr archive @@ -873,7 +873,7 @@ def test_version_rest_publish_not_an_owner(api_client, user, version, asset): @pytest.mark.django_db def test_version_rest_publish_not_a_draft(api_client, user, published_version, asset): - assign_perm('owner', user, published_version.dandiset) + add_dandiset_owner(published_version.dandiset, user) api_client.force_authenticate(user=user) published_version.assets.add(asset) @@ -912,7 +912,7 @@ def test_version_rest_publish_invalid( expected_data: str, expected_status_code: int, ): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) draft_version.assets.add(asset) @@ -951,7 +951,7 @@ def test_version_rest_update_no_changed_metadata( @pytest.mark.django_db def test_version_rest_delete_published_not_admin(api_client, user, published_version): - assign_perm('owner', user, published_version.dandiset) + add_dandiset_owner(published_version.dandiset, user) api_client.force_authenticate(user=user) response = api_client.delete( f'/api/dandisets/{published_version.dandiset.identifier}' @@ -975,7 +975,7 @@ def test_version_rest_delete_published_admin(api_client, admin_user, published_v @pytest.mark.django_db def test_version_rest_delete_draft_not_admin(api_client, user, draft_version): - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) api_client.force_authenticate(user=user) response = api_client.delete( f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/' diff --git a/dandiapi/zarr/tests/test_ingest_zarr_archive.py b/dandiapi/zarr/tests/test_ingest_zarr_archive.py index 5be56cc83..8c366ce5d 100644 --- a/dandiapi/zarr/tests/test_ingest_zarr_archive.py +++ b/dandiapi/zarr/tests/test_ingest_zarr_archive.py @@ -1,13 +1,13 @@ from __future__ import annotations from django.conf import settings -from guardian.shortcuts import assign_perm import pytest from zarr_checksum.checksum import EMPTY_CHECKSUM from dandiapi.api.models import AssetPath from dandiapi.api.models.version import Version from dandiapi.api.services.asset import add_asset_to_version +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus from dandiapi.zarr.tasks import ingest_dandiset_zarrs, ingest_zarr_archive @@ -109,7 +109,7 @@ def test_ingest_zarr_archive_assets(zarr_archive_factory, zarr_file_factory, dra @pytest.mark.django_db(transaction=True) def test_ingest_zarr_archive_modified(user, draft_version, zarr_archive_factory, zarr_file_factory): """Ensure that if the zarr associated to an asset is modified and then ingested, it succeeds.""" - assign_perm('owner', user, draft_version.dandiset) + add_dandiset_owner(draft_version.dandiset, user) # Ensure zarr is ingested with non-zero size zarr_archive = zarr_archive_factory( diff --git a/dandiapi/zarr/tests/test_zarr.py b/dandiapi/zarr/tests/test_zarr.py index 913349773..123865f3f 100644 --- a/dandiapi/zarr/tests/test_zarr.py +++ b/dandiapi/zarr/tests/test_zarr.py @@ -1,12 +1,11 @@ from __future__ import annotations from django.conf import settings -from guardian.shortcuts import assign_perm import pytest from zarr_checksum.checksum import EMPTY_CHECKSUM from dandiapi.api.models.dandiset import Dandiset -from dandiapi.api.services.permissions.dandiset import replace_dandiset_owners +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner, replace_dandiset_owners from dandiapi.api.tests.fuzzy import UUID_RE from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus from dandiapi.zarr.tasks import ingest_zarr_archive @@ -14,7 +13,7 @@ @pytest.mark.django_db def test_zarr_rest_create(authenticated_api_client, user, dandiset): - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) name = 'My Zarr File!' resp = authenticated_api_client.post( @@ -42,7 +41,7 @@ def test_zarr_rest_create(authenticated_api_client, user, dandiset): @pytest.mark.django_db def test_zarr_rest_dandiset_malformed(authenticated_api_client, user, dandiset): - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) resp = authenticated_api_client.post( '/api/zarr/', { @@ -70,7 +69,7 @@ def test_zarr_rest_create_not_an_owner(authenticated_api_client, zarr_archive): @pytest.mark.django_db def test_zarr_rest_create_duplicate(authenticated_api_client, user, zarr_archive): - assign_perm('owner', user, zarr_archive.dandiset) + add_dandiset_owner(zarr_archive.dandiset, user) resp = authenticated_api_client.post( '/api/zarr/', { @@ -91,7 +90,7 @@ def test_zarr_rest_create_embargoed_dandiset( dandiset_factory, ): dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) - assign_perm('owner', user, dandiset) + add_dandiset_owner(dandiset, user) resp = authenticated_api_client.post( '/api/zarr/', { @@ -252,7 +251,7 @@ def test_zarr_rest_delete_file( # Create zarr and assign user perms zarr_archive = zarr_archive_factory(status=ZarrArchiveStatus.UPLOADED) - assign_perm('owner', user, zarr_archive.dandiset) + add_dandiset_owner(zarr_archive.dandiset, user) # Upload file and ingest zarr_file = zarr_file_factory(zarr_archive=zarr_archive) @@ -301,7 +300,7 @@ def test_zarr_rest_delete_file_asset_metadata( ZarrArchive.storage = storage zarr_archive = zarr_archive_factory(status=ZarrArchiveStatus.UPLOADED) - assign_perm('owner', user, zarr_archive.dandiset) + add_dandiset_owner(zarr_archive.dandiset, user) asset = asset_factory(zarr=zarr_archive, blob=None) @@ -352,7 +351,7 @@ def test_zarr_rest_delete_multiple_files( zarr_archive: ZarrArchive, zarr_file_factory, ): - assign_perm('owner', user, zarr_archive.dandiset) + add_dandiset_owner(zarr_archive.dandiset, user) # Pretend like ZarrArchive was defined with the given storage ZarrArchive.storage = storage @@ -383,7 +382,7 @@ def test_zarr_rest_delete_missing_file( zarr_archive: ZarrArchive, zarr_file_factory, ): - assign_perm('owner', user, zarr_archive.dandiset) + add_dandiset_owner(zarr_archive.dandiset, user) # Pretend like ZarrArchive was defined with the given storage ZarrArchive.storage = storage diff --git a/dandiapi/zarr/tests/test_zarr_upload.py b/dandiapi/zarr/tests/test_zarr_upload.py index 83b0a4697..0ce4697e3 100644 --- a/dandiapi/zarr/tests/test_zarr_upload.py +++ b/dandiapi/zarr/tests/test_zarr_upload.py @@ -1,9 +1,9 @@ from __future__ import annotations -from guardian.shortcuts import assign_perm import pytest from zarr_checksum.checksum import EMPTY_CHECKSUM +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner from dandiapi.api.tests.fuzzy import HTTP_URL_RE from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus @@ -12,7 +12,7 @@ def test_zarr_rest_upload_start( authenticated_api_client, user, zarr_archive: ZarrArchive, storage, monkeypatch ): - assign_perm('owner', user, zarr_archive.dandiset) + add_dandiset_owner(zarr_archive.dandiset, user) # Pretend like our zarr was defined with the given storage monkeypatch.setattr(ZarrArchive, 'storage', storage) @@ -69,7 +69,7 @@ def test_zarr_rest_finalize( zarr_file_factory, monkeypatch, ): - assign_perm('owner', user, zarr_archive.dandiset) + add_dandiset_owner(zarr_archive.dandiset, user) # Pretend like our zarr was defined with the given storage monkeypatch.setattr(ZarrArchive, 'storage', storage) @@ -97,7 +97,7 @@ def test_zarr_rest_finalize_not_an_owner(authenticated_api_client, zarr_archive: def test_zarr_rest_finalize_already_ingested( authenticated_api_client, user, zarr_archive: ZarrArchive ): - assign_perm('owner', user, zarr_archive.dandiset) + add_dandiset_owner(zarr_archive.dandiset, user) authenticated_api_client.post(f'/api/zarr/{zarr_archive.zarr_id}/finalize/') resp = authenticated_api_client.post(f'/api/zarr/{zarr_archive.zarr_id}/finalize/') assert resp.status_code == 400 From 7f80d94e0e5f862ff1b83c23f5ba316a30195162 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 17 Dec 2024 18:14:52 -0500 Subject: [PATCH 03/11] Fix type import --- dandiapi/api/tests/test_audit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dandiapi/api/tests/test_audit.py b/dandiapi/api/tests/test_audit.py index fd3f7c0d9..74385e053 100644 --- a/dandiapi/api/tests/test_audit.py +++ b/dandiapi/api/tests/test_audit.py @@ -14,7 +14,7 @@ from dandiapi.zarr.models import ZarrArchive if TYPE_CHECKING: - from django.contrib.auth import User + from django.contrib.auth.models import User from dandiapi.api.models.audit import AuditRecordType From 71e32d74149b112d3e6a16c98f396b46476759a0 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 17 Dec 2024 18:28:24 -0500 Subject: [PATCH 04/11] Replace all uses of user.has_perm with is_dandiset_owner --- dandiapi/api/services/asset/__init__.py | 7 ++++--- dandiapi/api/services/dandiset/__init__.py | 4 ++-- dandiapi/api/services/embargo/__init__.py | 3 ++- dandiapi/api/services/publish/__init__.py | 3 ++- dandiapi/api/views/asset.py | 3 ++- dandiapi/api/views/dandiset.py | 3 ++- dandiapi/api/views/upload.py | 6 +++--- dandiapi/api/views/version.py | 3 ++- dandiapi/zarr/views/__init__.py | 6 +++--- 9 files changed, 22 insertions(+), 16 deletions(-) diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index 834e00520..b50dfa6c1 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -17,6 +17,7 @@ DraftDandisetNotModifiableError, ZarrArchiveBelongsToDifferentDandisetError, ) +from dandiapi.api.services.permissions.dandiset import is_dandiset_owner from dandiapi.api.tasks import remove_asset_blob_embargoed_tag_task if TYPE_CHECKING: @@ -98,7 +99,7 @@ def change_asset( # noqa: PLR0913 if 'path' not in new_metadata: raise ValueError('Path must be present in new_metadata') - if not user.has_perm('owner', version.dandiset): + if not is_dandiset_owner(version.dandiset, user): raise DandisetOwnerRequiredError if version.version != 'draft': raise DraftDandisetNotModifiableError @@ -148,7 +149,7 @@ def add_asset_to_version( if 'path' not in metadata: raise RuntimeError('Path must be present in metadata') - if not user.has_perm('owner', version.dandiset): + if not is_dandiset_owner(version.dandiset, user): raise DandisetOwnerRequiredError if version.version != 'draft': raise DraftDandisetNotModifiableError @@ -194,7 +195,7 @@ def add_asset_to_version( def remove_asset_from_version(*, user, asset: Asset, version: Version) -> Version: - if not user.has_perm('owner', version.dandiset): + if not is_dandiset_owner(version.dandiset, user): raise DandisetOwnerRequiredError if version.version != 'draft': raise DraftDandisetNotModifiableError diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 2da3504c3..3c75ca5e8 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -8,7 +8,7 @@ from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError from dandiapi.api.services.exceptions import AdminOnlyOperationError, NotAllowedError -from dandiapi.api.services.permissions.dandiset import add_dandiset_owner +from dandiapi.api.services.permissions.dandiset import add_dandiset_owner, is_dandiset_owner from dandiapi.api.services.version.metadata import _normalize_version_metadata @@ -56,7 +56,7 @@ def create_dandiset( def delete_dandiset(*, user, dandiset: Dandiset) -> None: - if not user.has_perm('owner', dandiset): + if not is_dandiset_owner(dandiset, user): raise NotAllowedError('Cannot delete dandisets which you do not own.') if dandiset.versions.exclude(version='draft').exists(): raise NotAllowedError('Cannot delete dandisets with published versions.') diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index e7a8841ca..714bd655f 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -12,6 +12,7 @@ from dandiapi.api.services.embargo.utils import _delete_object_tags, remove_dandiset_embargo_tags from dandiapi.api.services.exceptions import DandiError from dandiapi.api.services.metadata import validate_version_metadata +from dandiapi.api.services.permissions.dandiset import is_dandiset_owner from dandiapi.api.storage import get_boto_client from dandiapi.api.tasks import unembargo_dandiset_task from dandiapi.zarr.models import ZarrArchive @@ -95,7 +96,7 @@ def kickoff_dandiset_unembargo(*, user: User, dandiset: Dandiset): if dandiset.embargo_status != Dandiset.EmbargoStatus.EMBARGOED: raise DandisetNotEmbargoedError - if not user.has_perm('owner', dandiset): + if not is_dandiset_owner(dandiset, user): raise DandisetOwnerRequiredError if dandiset.uploads.count(): diff --git a/dandiapi/api/services/publish/__init__.py b/dandiapi/api/services/publish/__init__.py index f3a2ecb5b..62f810309 100644 --- a/dandiapi/api/services/publish/__init__.py +++ b/dandiapi/api/services/publish/__init__.py @@ -13,6 +13,7 @@ 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.permissions.dandiset import is_dandiset_owner from dandiapi.api.services.publish.exceptions import ( DandisetAlreadyPublishedError, DandisetAlreadyPublishingError, @@ -49,7 +50,7 @@ def _lock_dandiset_for_publishing(*, user: User, dandiset: Dandiset) -> None: # This function MUST be called before _publish_dandiset is called. """ - if not user.has_perm('owner', dandiset): + if not is_dandiset_owner(dandiset, user): raise NotAllowedError if dandiset.embargo_status != Dandiset.EmbargoStatus.OPEN: diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 062622c17..692f5a0a9 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -10,6 +10,7 @@ ) from dandiapi.api.services.asset.exceptions import DraftDandisetNotModifiableError from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError +from dandiapi.api.services.permissions.dandiset import is_dandiset_owner from dandiapi.zarr.models import ZarrArchive try: @@ -243,7 +244,7 @@ def raise_if_unauthorized(self): if not self.request.user.is_authenticated: # Clients must be authenticated to access it raise NotAuthenticated - if not self.request.user.has_perm('owner', version.dandiset): + if not is_dandiset_owner(version.dandiset, self.request.user): # The user does not have ownership permission raise PermissionDenied diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 6d863c8c0..d3dffdba0 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -35,6 +35,7 @@ ) from dandiapi.api.services.permissions.dandiset import ( get_visible_dandisets, + is_dandiset_owner, replace_dandiset_owners, ) from dandiapi.api.views.common import DANDISET_PK_PARAM @@ -171,7 +172,7 @@ def require_owner_perm(self, dandiset: Dandiset): # Raise 403 if unauthorized self.request.user = typing.cast(User, self.request.user) - if not self.request.user.has_perm('owner', dandiset): + if not is_dandiset_owner(dandiset, self.request.user): raise PermissionDenied def get_object(self): diff --git a/dandiapi/api/views/upload.py b/dandiapi/api/views/upload.py index 861c0b890..de4911614 100644 --- a/dandiapi/api/views/upload.py +++ b/dandiapi/api/views/upload.py @@ -18,7 +18,7 @@ from dandiapi.api.models import AssetBlob, Dandiset, Upload from dandiapi.api.permissions import IsApproved from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError -from dandiapi.api.services.permissions.dandiset import get_visible_dandisets +from dandiapi.api.services.permissions.dandiset import get_visible_dandisets, is_dandiset_owner from dandiapi.api.tasks import calculate_sha256 from dandiapi.api.views.serializers import AssetBlobSerializer @@ -191,7 +191,7 @@ def upload_complete_view(request: Request, upload_id: str) -> HttpResponseBase: parts: list[TransferredPart] = request_serializer.save() upload: Upload = get_object_or_404(Upload, upload_id=upload_id) - if upload.embargoed and not request.user.has_perm('owner', upload.dandiset): + if upload.embargoed and not is_dandiset_owner(upload.dandiset, request.user): raise Http404 from None completion = TransferredParts( @@ -228,7 +228,7 @@ def upload_validate_view(request: Request, upload_id: str) -> HttpResponseBase: Also starts the asynchronous checksum calculation process. """ upload = get_object_or_404(Upload, upload_id=upload_id) - if upload.embargoed and not request.user.has_perm('owner', upload.dandiset): + if upload.embargoed and not is_dandiset_owner(upload.dandiset, request.user): raise Http404 from None # Verify that the upload was successful diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index 51a6542d2..e45dbe164 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -16,6 +16,7 @@ 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.permissions.dandiset import is_dandiset_owner 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 @@ -56,7 +57,7 @@ def get_queryset(self): if not self.request.user.is_authenticated: # Clients must be authenticated to access it raise NotAuthenticated - if not self.request.user.has_perm('owner', dandiset): + if not is_dandiset_owner(dandiset, self.request.user): # The user does not have ownership permission raise PermissionDenied return super().get_queryset() diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index 211107ee0..ceef001d0 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -186,7 +186,7 @@ def finalize(self, request, zarr_id): queryset = self.get_queryset().select_for_update(of=['self']) with transaction.atomic(): zarr_archive: ZarrArchive = get_object_or_404(queryset, zarr_id=zarr_id) - if not self.request.user.has_perm('owner', zarr_archive.dandiset): + if not is_dandiset_owner(zarr_archive.dandiset, self.request.user): # The user does not have ownership permission raise PermissionDenied @@ -301,7 +301,7 @@ def create_files(self, request, zarr_id): return Response(ZarrArchive.INGEST_ERROR_MSG, status=status.HTTP_400_BAD_REQUEST) # Deny if the user doesn't have ownership permission - if not self.request.user.has_perm('owner', zarr_archive.dandiset): + if not is_dandiset_owner(zarr_archive.dandiset, self.request.user): raise PermissionDenied serializer = ZarrFileCreationSerializer(data=request.data, many=True) @@ -346,7 +346,7 @@ def delete_files(self, request, zarr_id): if zarr_archive.status in [ZarrArchiveStatus.UPLOADED, ZarrArchiveStatus.INGESTING]: return Response(ZarrArchive.INGEST_ERROR_MSG, status=status.HTTP_400_BAD_REQUEST) - if not self.request.user.has_perm('owner', zarr_archive.dandiset): + if not is_dandiset_owner(zarr_archive.dandiset, self.request.user): # The user does not have ownership permission raise PermissionDenied serializer = ZarrDeleteFileRequestSerializer(data=request.data, many=True) From c986ab91c1555a157292a3bc48f1c668aa3b0216 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Wed, 18 Dec 2024 11:58:55 -0500 Subject: [PATCH 05/11] Implement `is_owned_asset` permission function --- dandiapi/api/services/permissions/dandiset.py | 18 ++++++++++++++++++ dandiapi/api/views/asset.py | 16 +++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/dandiapi/api/services/permissions/dandiset.py b/dandiapi/api/services/permissions/dandiset.py index 895464f13..603d60948 100644 --- a/dandiapi/api/services/permissions/dandiset.py +++ b/dandiapi/api/services/permissions/dandiset.py @@ -14,6 +14,8 @@ if typing.TYPE_CHECKING: from django.contrib.auth.base_user import AbstractBaseUser + from dandiapi.api.models.asset import Asset + def get_dandiset_owners(dandiset: Dandiset) -> QuerySet[User]: qs = typing.cast(QuerySet[User], get_users_with_perms(dandiset, only_with_perms_in=['owner'])) @@ -53,6 +55,22 @@ def is_dandiset_owner(dandiset: Dandiset, user: AbstractBaseUser | AnonymousUser return user.has_perm('owner', dandiset) +def is_owned_asset(asset: Asset, user: AbstractBaseUser | AnonymousUser) -> bool: + """Return `True` if this asset belongs to a dandiset that the user is an owner of.""" + if user.is_anonymous: + return False + + user = typing.cast(User, user) + asset_dandisets = Dandiset.objects.filter(versions__in=asset.versions.all()) + asset_dandisets_owned_by_user = DandisetUserObjectPermission.objects.filter( + content_object__in=asset_dandisets, + user=user, + permission__codename='owner', + ) + + return asset_dandisets_owned_by_user.exists() + + def get_owned_dandisets(user: AbstractBaseUser | AnonymousUser) -> QuerySet[Dandiset]: return get_objects_for_user(user, 'owner', Dandiset) diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 692f5a0a9..1d97ee53d 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -1,6 +1,9 @@ from __future__ import annotations import re +import typing + +from django.contrib.auth.models import User from dandiapi.api.asset_paths import search_asset_paths from dandiapi.api.services.asset import ( @@ -10,7 +13,7 @@ ) from dandiapi.api.services.asset.exceptions import DraftDandisetNotModifiableError from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError -from dandiapi.api.services.permissions.dandiset import is_dandiset_owner +from dandiapi.api.services.permissions.dandiset import is_dandiset_owner, is_owned_asset from dandiapi.zarr.models import ZarrArchive try: @@ -42,7 +45,6 @@ from dandiapi.api.models import Asset, AssetBlob, Dandiset, Version from dandiapi.api.models.asset import validate_asset_path -from dandiapi.api.models.dandiset import DandisetUserObjectPermission from dandiapi.api.views.common import ( ASSET_ID_PARAM, VERSIONS_DANDISET_PK_PARAM, @@ -95,19 +97,15 @@ def raise_if_unauthorized(self): # Clients must be authenticated to access it if not self.request.user.is_authenticated: raise NotAuthenticated + self.request.user = typing.cast(User, self.request.user) # Admins are allowed to access any embargoed asset blob if self.request.user.is_superuser: return # User must be an owner on any of the dandisets this asset belongs to - asset_dandisets = Dandiset.objects.filter(versions__in=asset.versions.all()) - asset_dandisets_owned_by_user = DandisetUserObjectPermission.objects.filter( - content_object__in=asset_dandisets, - user=self.request.user, - permission__codename='owner', - ) - if not asset_dandisets_owned_by_user.exists(): + is_owned = is_owned_asset(asset, self.request.user) + if not is_owned: raise PermissionDenied def get_queryset(self): From b4156d48d3ee0a5ba7d8031ef406ab61799c01a2 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Wed, 18 Dec 2024 12:05:44 -0500 Subject: [PATCH 06/11] Replace all uses of `get_objects_for_user` with `get_owned_dandisets` --- dandiapi/api/services/permissions/dandiset.py | 7 +++++-- dandiapi/api/views/dandiset.py | 6 +++--- dandiapi/search/models.py | 4 ++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/dandiapi/api/services/permissions/dandiset.py b/dandiapi/api/services/permissions/dandiset.py index 603d60948..a5e406dae 100644 --- a/dandiapi/api/services/permissions/dandiset.py +++ b/dandiapi/api/services/permissions/dandiset.py @@ -71,8 +71,11 @@ def is_owned_asset(asset: Asset, user: AbstractBaseUser | AnonymousUser) -> bool return asset_dandisets_owned_by_user.exists() -def get_owned_dandisets(user: AbstractBaseUser | AnonymousUser) -> QuerySet[Dandiset]: - return get_objects_for_user(user, 'owner', Dandiset) +def get_owned_dandisets( + user: AbstractBaseUser | AnonymousUser, + include_superusers=True, # noqa: FBT002 +) -> QuerySet[Dandiset]: + return get_objects_for_user(user, 'owner', Dandiset, with_superuser=include_superusers) def get_visible_dandisets(user: AbstractBaseUser | AnonymousUser) -> QuerySet[Dandiset]: diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index d3dffdba0..f929efb04 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -13,7 +13,6 @@ from django.utils.decorators import method_decorator from drf_yasg.utils import no_body, swagger_auto_schema from guardian.decorators import permission_required_or_403 -from guardian.shortcuts import get_objects_for_user from guardian.utils import get_40x_or_None from rest_framework import filters, status from rest_framework.decorators import action @@ -34,6 +33,7 @@ UnauthorizedEmbargoAccessError, ) from dandiapi.api.services.permissions.dandiset import ( + get_owned_dandisets, get_visible_dandisets, is_dandiset_owner, replace_dandiset_owners, @@ -133,8 +133,8 @@ def get_queryset(self): user_kwarg = query_serializer.validated_data.get('user') if user_kwarg == 'me': # Replace the original, rather inefficient queryset with a more specific one - queryset = get_objects_for_user( - self.request.user, 'owner', Dandiset, with_superuser=False + queryset = get_owned_dandisets( + self.request.user, include_superusers=False ).order_by('created') show_draft: bool = query_serializer.validated_data['draft'] diff --git a/dandiapi/search/models.py b/dandiapi/search/models.py index 41eb4f68c..ce67e937f 100644 --- a/dandiapi/search/models.py +++ b/dandiapi/search/models.py @@ -4,9 +4,9 @@ from django.db import models from django.db.models import OuterRef, Q, Subquery -from guardian.shortcuts import get_objects_for_user from dandiapi.api.models import Dandiset +from dandiapi.api.services.permissions.dandiset import get_owned_dandisets if TYPE_CHECKING: from django.contrib.auth.models import User @@ -18,7 +18,7 @@ def visible_to(self, user: User) -> models.QuerySet[AssetSearch]: embargo_statuses_query = Dandiset.objects.filter(id=OuterRef('dandiset_id')).values( 'embargo_status' ) - owned_dandisets_query = get_objects_for_user(user, 'owner', Dandiset) + owned_dandisets_query = get_owned_dandisets(user) return self.alias(embargo_status=Subquery(embargo_statuses_query)).filter( Q(embargo_status=Dandiset.EmbargoStatus.OPEN) | Q(dandiset_id__in=owned_dandisets_query) From d300848e151d71d9bfa774dd82abbf1084182d94 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Wed, 18 Dec 2024 12:11:54 -0500 Subject: [PATCH 07/11] Use `get_owned_dandisets` instead of direct perm class query --- dandiapi/zarr/views/__init__.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index ceef001d0..19bba2571 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -15,9 +15,13 @@ from rest_framework.utils.urls import replace_query_param from rest_framework.viewsets import ReadOnlyModelViewSet -from dandiapi.api.models.dandiset import Dandiset, DandisetUserObjectPermission +from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.services import audit -from dandiapi.api.services.permissions.dandiset import get_visible_dandisets, is_dandiset_owner +from dandiapi.api.services.permissions.dandiset import ( + get_owned_dandisets, + get_visible_dandisets, + is_dandiset_owner, +) from dandiapi.api.storage import get_boto_client from dandiapi.api.views.pagination import DandiPagination from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus @@ -107,9 +111,9 @@ def get_queryset(self) -> QuerySet: # Filter zarrs to either open access or owned if self.request.user.is_authenticated: - user_owned_dandiset_ids = DandisetUserObjectPermission.objects.filter( - user=self.request.user, permission__codename='owner' - ).values_list('content_object_id', flat=True) + user_owned_dandiset_ids = get_owned_dandisets(self.request.user).values_list( + 'id', flat=True + ) queryset_filter |= Q(dandiset_id__in=user_owned_dandiset_ids) # Apply filter From 0f5b240baee1053f1c2f6b1d3b7e723d06fe0416 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Wed, 18 Dec 2024 12:39:50 -0500 Subject: [PATCH 08/11] Replace use of undocumented function with `is_dandiset_owner` --- dandiapi/api/tests/test_dandiset.py | 22 ++++++++++++++++++++++ dandiapi/api/views/dandiset.py | 7 +++---- dandiapi/api/views/upload.py | 7 +++---- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/dandiapi/api/tests/test_dandiset.py b/dandiapi/api/tests/test_dandiset.py index fb2db2668..669908911 100644 --- a/dandiapi/api/tests/test_dandiset.py +++ b/dandiapi/api/tests/test_dandiset.py @@ -989,6 +989,28 @@ def test_dandiset_rest_add_owner( assert mailoutbox[0].to == [user2.email] +@pytest.mark.django_db +def test_dandiset_rest_add_owner_not_allowed( + api_client, draft_version, user_factory, social_account_factory +): + dandiset = draft_version.dandiset + user1 = user_factory() + user2 = user_factory() + social_account1 = social_account_factory(user=user1) + social_account2 = social_account_factory(user=user2) + api_client.force_authenticate(user=user1) + + resp = api_client.put( + f'/api/dandisets/{dandiset.identifier}/users/', + [ + {'username': social_account1.extra_data['login']}, + {'username': social_account2.extra_data['login']}, + ], + format='json', + ) + assert resp.status_code == 403 + + @pytest.mark.django_db def test_dandiset_rest_remove_owner( api_client, diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index f929efb04..4a5f9e031 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -13,7 +13,6 @@ from django.utils.decorators import method_decorator from drf_yasg.utils import no_body, swagger_auto_schema from guardian.decorators import permission_required_or_403 -from guardian.utils import get_40x_or_None from rest_framework import filters, status from rest_framework.decorators import action from rest_framework.exceptions import NotAuthenticated, PermissionDenied @@ -32,6 +31,7 @@ DandisetUnembargoInProgressError, UnauthorizedEmbargoAccessError, ) +from dandiapi.api.services.exceptions import NotAllowedError from dandiapi.api.services.permissions.dandiset import ( get_owned_dandisets, get_visible_dandisets, @@ -399,9 +399,8 @@ def users(self, request, dandiset__pk): # noqa: C901 raise DandisetUnembargoInProgressError # Verify that the user is currently an owner - response = get_40x_or_None(request, ['owner'], dandiset, return_403=True) - if response: - return response + if not is_dandiset_owner(dandiset, request.user): + raise NotAllowedError serializer = UserSerializer(data=request.data, many=True) serializer.is_valid(raise_exception=True) diff --git a/dandiapi/api/views/upload.py b/dandiapi/api/views/upload.py index de4911614..fe984793a 100644 --- a/dandiapi/api/views/upload.py +++ b/dandiapi/api/views/upload.py @@ -7,7 +7,6 @@ from django.http.response import Http404, HttpResponseBase from django.shortcuts import get_object_or_404 from drf_yasg.utils import swagger_auto_schema -from guardian.utils import get_40x_or_None from rest_framework import serializers, status from rest_framework.decorators import api_view, parser_classes, permission_classes from rest_framework.exceptions import ValidationError @@ -18,6 +17,7 @@ from dandiapi.api.models import AssetBlob, Dandiset, Upload from dandiapi.api.permissions import IsApproved from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError +from dandiapi.api.services.exceptions import NotAllowedError from dandiapi.api.services.permissions.dandiset import get_visible_dandisets, is_dandiset_owner from dandiapi.api.tasks import calculate_sha256 from dandiapi.api.views.serializers import AssetBlobSerializer @@ -135,9 +135,8 @@ def upload_initialize_view(request: Request) -> HttpResponseBase: get_visible_dandisets(request.user), id=dandiset_id, ) - response = get_40x_or_None(request, ['owner'], dandiset, return_403=True) - if response: - return response + if not is_dandiset_owner(dandiset, request.user): + raise NotAllowedError # Ensure dandiset not in the process of unembargo if dandiset.unembargo_in_progress: From a57538aef9d15d7cc82b885fe4e1cbaa50033c70 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Wed, 18 Dec 2024 14:37:52 -0500 Subject: [PATCH 09/11] Replace `permission_required_or_403` with `require_dandiset_owner_or_403` --- dandiapi/api/services/permissions/dandiset.py | 15 ++++++++++++++ dandiapi/api/views/asset.py | 20 ++++++++----------- dandiapi/api/views/dandiset.py | 5 ++--- dandiapi/api/views/version.py | 11 +++++----- 4 files changed, 31 insertions(+), 20 deletions(-) diff --git a/dandiapi/api/services/permissions/dandiset.py b/dandiapi/api/services/permissions/dandiset.py index a5e406dae..682693bca 100644 --- a/dandiapi/api/services/permissions/dandiset.py +++ b/dandiapi/api/services/permissions/dandiset.py @@ -7,6 +7,8 @@ from django.contrib.auth.models import AnonymousUser, User from django.db import transaction from django.db.models import Q, QuerySet +from django.utils.decorators import method_decorator +from guardian.decorators import permission_required from guardian.shortcuts import assign_perm, get_objects_for_user, get_users_with_perms from dandiapi.api.models.dandiset import Dandiset, DandisetUserObjectPermission @@ -101,3 +103,16 @@ def get_visible_dandisets(user: AbstractBaseUser | AnonymousUser) -> QuerySet[Da return Dandiset.objects.filter( Q(embargo_status=Dandiset.EmbargoStatus.OPEN) | Q(pk__in=owned_dandiset_pks) ).order_by('created') + + +def require_dandiset_owner_or_403(pk_path: str): + """ + Decorate viewset methods to only allow access to dandiset owners. + + The `pk_path` argument is the Dandiset ID URL path variable that DRF passes into the request. + """ + return method_decorator( + permission_required( + perm='owner', lookup_variables=(Dandiset, 'pk', pk_path), return_403=True + ) + ) diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 1d97ee53d..fbd94a842 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -13,7 +13,11 @@ ) from dandiapi.api.services.asset.exceptions import DraftDandisetNotModifiableError from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError -from dandiapi.api.services.permissions.dandiset import is_dandiset_owner, is_owned_asset +from dandiapi.api.services.permissions.dandiset import ( + is_dandiset_owner, + is_owned_asset, + require_dandiset_owner_or_403, +) from dandiapi.zarr.models import ZarrArchive try: @@ -31,10 +35,8 @@ from django.conf import settings from django.db import transaction from django.http import HttpResponse, HttpResponseRedirect -from django.utils.decorators import method_decorator from django_filters import rest_framework as filters from drf_yasg.utils import swagger_auto_schema -from guardian.decorators import permission_required_or_403 from rest_framework import serializers, status from rest_framework.decorators import action from rest_framework.exceptions import NotAuthenticated, NotFound, PermissionDenied @@ -320,9 +322,7 @@ def validation(self, request, **kwargs): User must be an owner of the specified dandiset.\ New assets can only be attached to draft versions.', ) - @method_decorator( - permission_required_or_403('owner', (Dandiset, 'pk', 'versions__dandiset__pk')) - ) + @require_dandiset_owner_or_403('versions__dandiset__pk') def create(self, request, versions__dandiset__pk, versions__version): version: Version = get_object_or_404( Version.objects.select_related('dandiset'), @@ -356,9 +356,7 @@ def create(self, request, versions__dandiset__pk, versions__version): Only draft versions can be modified.\ Old asset is returned if no updates to metadata are made.', ) - @method_decorator( - permission_required_or_403('owner', (Dandiset, 'pk', 'versions__dandiset__pk')) - ) + @require_dandiset_owner_or_403('versions__dandiset__pk') def update(self, request, versions__dandiset__pk, versions__version, **kwargs): """Create an asset with updated metadata.""" version: Version = get_object_or_404( @@ -391,9 +389,7 @@ def update(self, request, versions__dandiset__pk, versions__version, **kwargs): serializer = AssetDetailSerializer(instance=asset) return Response(serializer.data, status=status.HTTP_200_OK) - @method_decorator( - permission_required_or_403('owner', (Dandiset, 'pk', 'versions__dandiset__pk')) - ) + @require_dandiset_owner_or_403('versions__dandiset__pk') @swagger_auto_schema( manual_parameters=[VERSIONS_DANDISET_PK_PARAM, VERSIONS_VERSION_PARAM], operation_summary='Remove an asset from a version.', diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 4a5f9e031..cfbe9095e 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -10,9 +10,7 @@ from django.db.models.functions import Coalesce from django.db.models.query_utils import Q from django.http import Http404 -from django.utils.decorators import method_decorator from drf_yasg.utils import no_body, swagger_auto_schema -from guardian.decorators import permission_required_or_403 from rest_framework import filters, status from rest_framework.decorators import action from rest_framework.exceptions import NotAuthenticated, PermissionDenied @@ -37,6 +35,7 @@ get_visible_dandisets, is_dandiset_owner, replace_dandiset_owners, + require_dandiset_owner_or_403, ) from dandiapi.api.views.common import DANDISET_PK_PARAM from dandiapi.api.views.pagination import DandiPagination @@ -364,7 +363,7 @@ def destroy(self, request, dandiset__pk): ), ) @action(methods=['POST'], detail=True) - @method_decorator(permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk'))) + @require_dandiset_owner_or_403('dandiset__pk') def unembargo(self, request, dandiset__pk): dandiset: Dandiset = get_object_or_404(Dandiset, pk=dandiset__pk) kickoff_dandiset_unembargo(user=request.user, dandiset=dandiset) diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index e45dbe164..42d617d32 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -1,10 +1,8 @@ from __future__ import annotations from django.db import transaction -from django.utils.decorators import method_decorator from django_filters import rest_framework as filters from drf_yasg.utils import no_body, swagger_auto_schema -from guardian.decorators import permission_required_or_403 from rest_framework import status from rest_framework.decorators import action from rest_framework.exceptions import NotAuthenticated, PermissionDenied @@ -16,7 +14,10 @@ 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.permissions.dandiset import is_dandiset_owner +from dandiapi.api.services.permissions.dandiset import ( + is_dandiset_owner, + require_dandiset_owner_or_403, +) 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 @@ -88,7 +89,7 @@ def info(self, request, **kwargs): responses={200: VersionDetailSerializer}, manual_parameters=[DANDISET_PK_PARAM, VERSION_PARAM], ) - @method_decorator(permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk'))) + @require_dandiset_owner_or_403('dandiset__pk') def update(self, request, **kwargs): """Update the metadata of a version.""" version: Version = self.get_object() @@ -137,7 +138,7 @@ def update(self, request, **kwargs): responses={200: VersionSerializer}, ) @action(detail=True, methods=['POST']) - @method_decorator(permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk'))) + @require_dandiset_owner_or_403('dandiset__pk') def publish(self, request, **kwargs): """Publish a version.""" if kwargs['version'] != 'draft': From 265f0f237bdf3dc2cd3fc20a753620a78417e795 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 2 Jan 2025 17:05:23 -0500 Subject: [PATCH 10/11] Use transaction.atomic in context manager form Co-authored-by: Mike VanDenburgh --- dandiapi/api/services/permissions/dandiset.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dandiapi/api/services/permissions/dandiset.py b/dandiapi/api/services/permissions/dandiset.py index 682693bca..7d9f12e91 100644 --- a/dandiapi/api/services/permissions/dandiset.py +++ b/dandiapi/api/services/permissions/dandiset.py @@ -28,20 +28,20 @@ def add_dandiset_owner(dandiset: Dandiset, user: User): assign_perm('owner', user, dandiset) -@transaction.atomic def replace_dandiset_owners(dandiset: Dandiset, users: list[User]): existing_owners = get_dandiset_owners(dandiset) existing_owner_set = set(existing_owners) new_owner_set = set(users) - # Delete all existing owners - DandisetUserObjectPermission.objects.filter( - content_object=dandiset.pk, permission__codename='owner' - ).delete() + with transaction.atomic(): + # Delete all existing owners + DandisetUserObjectPermission.objects.filter( + content_object=dandiset.pk, permission__codename='owner' + ).delete() - # Set owners to new list - for user in users: - add_dandiset_owner(dandiset, user) + # Set owners to new list + for user in users: + add_dandiset_owner(dandiset, user) # Return the owners added/removed so they can be emailed removed_owners = existing_owner_set - new_owner_set From 0579cce6ee5cd5a44df0a7155d7e3a81cb554365 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 2 Jan 2025 17:42:36 -0500 Subject: [PATCH 11/11] Replace dandiset owners property with get_dandiset_owners Co-authored-by: Mike VanDenburgh --- dandiapi/api/mail.py | 6 ++++-- .../api/management/commands/extract_metadata.py | 3 ++- dandiapi/api/models/dandiset.py | 6 ------ dandiapi/api/tests/test_create_dev_dandiset.py | 3 ++- dandiapi/api/tests/test_dandiset.py | 15 ++++++++------- dandiapi/api/views/dandiset.py | 3 ++- dandiapi/zarr/tests/test_zarr.py | 8 ++++++-- 7 files changed, 24 insertions(+), 20 deletions(-) diff --git a/dandiapi/api/mail.py b/dandiapi/api/mail.py index 7b0a5b4d1..8fddd78fd 100644 --- a/dandiapi/api/mail.py +++ b/dandiapi/api/mail.py @@ -8,6 +8,8 @@ from django.template.loader import render_to_string from django.utils.html import strip_tags +from dandiapi.api.services.permissions.dandiset import get_dandiset_owners + if TYPE_CHECKING: from collections.abc import Iterable @@ -207,7 +209,7 @@ def build_dandiset_unembargoed_message(dandiset: Dandiset): subject='Your Dandiset has been unembargoed!', message=strip_tags(html_message), html_message=html_message, - to=[owner.email for owner in dandiset.owners], + to=[owner.email for owner in get_dandiset_owners(dandiset)], ) @@ -229,7 +231,7 @@ def build_dandiset_unembargo_failed_message(dandiset: Dandiset): subject=f'DANDI: Unembargo failed for dandiset {dandiset.identifier}', message=strip_tags(html_message), html_message=html_message, - to=[owner.email for owner in dandiset.owners], + to=[owner.email for owner in get_dandiset_owners(dandiset)], bcc=[settings.DANDI_DEV_EMAIL], reply_to=[ADMIN_EMAIL], ) diff --git a/dandiapi/api/management/commands/extract_metadata.py b/dandiapi/api/management/commands/extract_metadata.py index 0a9a9ba79..aa45ecee5 100644 --- a/dandiapi/api/management/commands/extract_metadata.py +++ b/dandiapi/api/management/commands/extract_metadata.py @@ -15,6 +15,7 @@ from dandiapi.api.models import Asset, Dandiset, Version from dandiapi.api.services.asset import change_asset +from dandiapi.api.services.permissions.dandiset import get_dandiset_owners if TYPE_CHECKING: from django.db.models import QuerySet @@ -55,7 +56,7 @@ def extract_asset_metadata(asset: Asset, draft_version: Version): # Use dandiset owner, default to some admin user user = ( - draft_version.dandiset.owners.first() + get_dandiset_owners(draft_version.dandiset).first() or User.objects.filter(is_superuser=True, is_staff=True).first() ) diff --git a/dandiapi/api/models/dandiset.py b/dandiapi/api/models/dandiset.py index f44a1bdd8..13ed9a67d 100644 --- a/dandiapi/api/models/dandiset.py +++ b/dandiapi/api/models/dandiset.py @@ -45,12 +45,6 @@ def most_recent_published_version(self): def draft_version(self): return self.versions.filter(version='draft').get() - @property - def owners(self): - from dandiapi.api.services.permissions.dandiset import get_dandiset_owners - - return get_dandiset_owners(self) - @classmethod def published_count(cls): """Return the number of Dandisets with published Versions.""" diff --git a/dandiapi/api/tests/test_create_dev_dandiset.py b/dandiapi/api/tests/test_create_dev_dandiset.py index b3204870b..c32f49d90 100644 --- a/dandiapi/api/tests/test_create_dev_dandiset.py +++ b/dandiapi/api/tests/test_create_dev_dandiset.py @@ -4,6 +4,7 @@ from dandiapi.api.management.commands.create_dev_dandiset import create_dev_dandiset from dandiapi.api.models import Asset, AssetBlob, Dandiset, Version +from dandiapi.api.services.permissions.dandiset import get_dandiset_owners @pytest.mark.django_db @@ -12,7 +13,7 @@ def test_create_dev_dandiset(user): assert Dandiset.objects.count() == 1 dandiset = Dandiset.objects.get() - assert user in dandiset.owners + assert user in get_dandiset_owners(dandiset) assert Version.objects.count() == 1 version = Version.objects.get() diff --git a/dandiapi/api/tests/test_dandiset.py b/dandiapi/api/tests/test_dandiset.py index 669908911..19cc73379 100644 --- a/dandiapi/api/tests/test_dandiset.py +++ b/dandiapi/api/tests/test_dandiset.py @@ -10,6 +10,7 @@ from dandiapi.api.models import Dandiset, Version from dandiapi.api.services.permissions.dandiset import ( add_dandiset_owner, + get_dandiset_owners, get_visible_dandisets, replace_dandiset_owners, ) @@ -382,7 +383,7 @@ def test_dandiset_rest_create(api_client, user): # Creating a Dandiset has side affects. # Verify that the user is the only owner. dandiset = Dandiset.objects.get(id=dandiset_id) - assert list(dandiset.owners.all()) == [user] + assert list(get_dandiset_owners(dandiset).all()) == [user] # Verify that a draft Version and VersionMetadata were also created. assert dandiset.versions.count() == 1 @@ -476,7 +477,7 @@ def test_dandiset_rest_create_with_identifier(api_client, admin_user): # Creating a Dandiset has side affects. # Verify that the user is the only owner. dandiset = Dandiset.objects.get(id=identifier) - assert list(dandiset.owners.all()) == [admin_user] + assert list(get_dandiset_owners(dandiset).all()) == [admin_user] # Verify that a draft Version and VersionMetadata were also created. assert dandiset.versions.count() == 1 @@ -584,7 +585,7 @@ def test_dandiset_rest_create_with_contributor(api_client, admin_user): # Creating a Dandiset has side affects. # Verify that the user is the only owner. dandiset = Dandiset.objects.get(id=identifier) - assert list(dandiset.owners.all()) == [admin_user] + assert list(get_dandiset_owners(dandiset).all()) == [admin_user] # Verify that a draft Version and VersionMetadata were also created. assert dandiset.versions.count() == 1 @@ -675,7 +676,7 @@ def test_dandiset_rest_create_embargoed(api_client, user): # Creating a Dandiset has side affects. # Verify that the user is the only owner. dandiset = Dandiset.objects.get(id=dandiset_id) - assert list(dandiset.owners.all()) == [user] + assert list(get_dandiset_owners(dandiset).all()) == [user] # Verify that a draft Version and VersionMetadata were also created. assert dandiset.versions.count() == 1 @@ -904,7 +905,7 @@ def test_dandiset_rest_change_owner( 'email': social_account2.extra_data['email'], } ] - assert list(dandiset.owners) == [user2] + assert list(get_dandiset_owners(dandiset)) == [user2] assert len(mailoutbox) == 2 assert mailoutbox[0].subject == f'Removed from Dandiset "{dandiset.draft_version.name}"' @@ -982,7 +983,7 @@ def test_dandiset_rest_add_owner( 'email': social_account2.extra_data['email'], }, ] - assert list(dandiset.owners) == [user1, user2] + assert list(get_dandiset_owners(dandiset)) == [user1, user2] assert len(mailoutbox) == 1 assert mailoutbox[0].subject == f'Added to Dandiset "{dandiset.draft_version.name}"' @@ -1041,7 +1042,7 @@ def test_dandiset_rest_remove_owner( 'email': social_account1.extra_data['email'], } ] - assert list(dandiset.owners) == [user1] + assert list(get_dandiset_owners(dandiset)) == [user1] assert len(mailoutbox) == 1 assert mailoutbox[0].subject == f'Removed from Dandiset "{dandiset.draft_version.name}"' diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index cfbe9095e..3df6a975d 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -31,6 +31,7 @@ ) from dandiapi.api.services.exceptions import NotAllowedError from dandiapi.api.services.permissions.dandiset import ( + get_dandiset_owners, get_owned_dandisets, get_visible_dandisets, is_dandiset_owner, @@ -444,7 +445,7 @@ def users(self, request, dandiset__pk): # noqa: C901 send_ownership_change_emails(dandiset, removed_owners, added_owners) owners = [] - for owner_user in dandiset.owners: + for owner_user in get_dandiset_owners(dandiset): try: owner_account = SocialAccount.objects.get(user=owner_user) owner_dict = {'username': owner_account.extra_data['login']} diff --git a/dandiapi/zarr/tests/test_zarr.py b/dandiapi/zarr/tests/test_zarr.py index 123865f3f..da13aabe0 100644 --- a/dandiapi/zarr/tests/test_zarr.py +++ b/dandiapi/zarr/tests/test_zarr.py @@ -5,7 +5,11 @@ from zarr_checksum.checksum import EMPTY_CHECKSUM from dandiapi.api.models.dandiset import Dandiset -from dandiapi.api.services.permissions.dandiset import add_dandiset_owner, replace_dandiset_owners +from dandiapi.api.services.permissions.dandiset import ( + add_dandiset_owner, + get_dandiset_owners, + replace_dandiset_owners, +) from dandiapi.api.tests.fuzzy import UUID_RE from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus from dandiapi.zarr.tasks import ingest_zarr_archive @@ -129,7 +133,7 @@ def test_zarr_rest_get(authenticated_api_client, storage, zarr_archive_factory, @pytest.mark.django_db def test_zarr_rest_get_embargoed(authenticated_api_client, user, embargoed_zarr_archive): - assert user not in embargoed_zarr_archive.dandiset.owners + assert user not in get_dandiset_owners(embargoed_zarr_archive.dandiset) resp = authenticated_api_client.get(f'/api/zarr/{embargoed_zarr_archive.zarr_id}/') assert resp.status_code == 404