From 307366f3f3efdb6d7311bb4b56ffe2ca5edd1231 Mon Sep 17 00:00:00 2001 From: Daniel Chiquito Date: Mon, 5 Apr 2021 14:59:26 -0400 Subject: [PATCH 1/2] Use Django method_decorator instead of manually testing permissions --- dandiapi/api/views/asset.py | 35 ++++++++++++---------------------- dandiapi/api/views/dandiset.py | 10 +++------- dandiapi/api/views/version.py | 14 +++++--------- 3 files changed, 20 insertions(+), 39 deletions(-) diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 32b8087bf..409e86a54 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -14,10 +14,11 @@ from django.http import HttpResponseRedirect from django.shortcuts import get_object_or_404 from django.urls import reverse +from django.utils.decorators import method_decorator from django_filters import rest_framework as filters from drf_yasg import openapi from drf_yasg.utils import swagger_auto_schema -from guardian.utils import get_40x_or_None +from guardian.decorators import permission_required_or_403 from rest_framework import serializers, status from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticatedOrReadOnly @@ -25,7 +26,7 @@ from rest_framework.viewsets import ReadOnlyModelViewSet from rest_framework_extensions.mixins import DetailSerializerMixin, NestedViewSetMixin -from dandiapi.api.models import Asset, AssetBlob, AssetMetadata, Version +from dandiapi.api.models import Asset, AssetBlob, AssetMetadata, Dandiset, Version from dandiapi.api.views.common import DandiPagination from dandiapi.api.views.serializers import AssetDetailSerializer, AssetSerializer @@ -89,7 +90,9 @@ def retrieve(self, request, versions__dandiset__pk, versions__version, asset_id) 404: 'If a blob with the given checksum has not been validated', }, ) - # @permission_required_or_403('owner', (Dandiset, 'pk', 'version__dandiset__pk')) + @method_decorator( + permission_required_or_403('owner', (Dandiset, 'pk', 'versions__dandiset__pk')) + ) def create(self, request, versions__dandiset__pk, versions__version): version: Version = get_object_or_404( Version, @@ -97,12 +100,6 @@ def create(self, request, versions__dandiset__pk, versions__version): version=versions__version, ) - # TODO @permission_required doesn't work on methods - # https://github.com/django-guardian/django-guardian/issues/723 - response = get_40x_or_None(request, ['owner'], version.dandiset, return_403=True) - if response: - return response - serializer = AssetRequestSerializer(data=request.data) serializer.is_valid(raise_exception=True) @@ -134,7 +131,9 @@ def create(self, request, versions__dandiset__pk, versions__version): request_body=AssetRequestSerializer(), responses={200: AssetDetailSerializer()}, ) - # @permission_required_or_403('owner', (Dandiset, 'pk', 'version__dandiset__pk')) + @method_decorator( + permission_required_or_403('owner', (Dandiset, 'pk', 'versions__dandiset__pk')) + ) def update(self, request, versions__dandiset__pk, versions__version, **kwargs): """Update the metadata of an asset.""" old_asset = self.get_object() @@ -143,12 +142,6 @@ def update(self, request, versions__dandiset__pk, versions__version, **kwargs): version=versions__version, ) - # TODO @permission_required doesn't work on methods - # https://github.com/django-guardian/django-guardian/issues/723 - response = get_40x_or_None(request, ['owner'], version.dandiset, return_403=True) - if response: - return response - serializer = AssetRequestSerializer(data=request.data) serializer.is_valid(raise_exception=True) @@ -182,19 +175,15 @@ def update(self, request, versions__dandiset__pk, versions__version, **kwargs): serializer = AssetDetailSerializer(instance=new_asset) return Response(serializer.data, status=status.HTTP_200_OK) - # @permission_required_or_403('owner', (Dandiset, 'pk', 'version__dandiset__pk')) + @method_decorator( + permission_required_or_403('owner', (Dandiset, 'pk', 'versions__dandiset__pk')) + ) def destroy(self, request, versions__dandiset__pk, versions__version, **kwargs): asset = self.get_object() version = Version.objects.get( dandiset__pk=versions__dandiset__pk, version=versions__version ) - # TODO @permission_required doesn't work on methods - # https://github.com/django-guardian/django-guardian/issues/723 - response = get_40x_or_None(request, ['owner'], version.dandiset, return_403=True) - if response: - return response - version.assets.remove(asset) return Response(None, status=status.HTTP_204_NO_CONTENT) diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 89ebe37a6..ea92c0f99 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -3,7 +3,9 @@ from django.db.utils import IntegrityError from django.http import Http404 from django.shortcuts import get_object_or_404 +from django.utils.decorators import method_decorator from drf_yasg.utils import swagger_auto_schema +from guardian.decorators import permission_required_or_403 from guardian.shortcuts import assign_perm, get_objects_for_user from guardian.utils import get_40x_or_None from rest_framework import filters, status @@ -127,16 +129,10 @@ def create(self, request): serializer = DandisetDetailSerializer(instance=dandiset) return Response(serializer.data, status=status.HTTP_200_OK) - # @permission_required_or_403('owner', (Dandiset, 'dandiset__pk')) + @method_decorator(permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk'))) def destroy(self, request, dandiset__pk): dandiset: Dandiset = get_object_or_404(Dandiset, pk=dandiset__pk) - # TODO @permission_required doesn't work on methods - # https://github.com/django-guardian/django-guardian/issues/723 - response = get_40x_or_None(request, ['owner'], dandiset, return_403=True) - if response: - return response - dandiset.delete() return Response(None, status=status.HTTP_204_NO_CONTENT) diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index 9851b02f1..43292a715 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -1,4 +1,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 status from rest_framework.decorators import action @@ -8,7 +10,7 @@ from rest_framework_extensions.mixins import DetailSerializerMixin, NestedViewSetMixin from dandiapi.api import doi -from dandiapi.api.models import Version, VersionMetadata +from dandiapi.api.models import Dandiset, Version, VersionMetadata from dandiapi.api.tasks import write_yamls from dandiapi.api.views.common import DandiPagination from dandiapi.api.views.serializers import ( @@ -34,7 +36,7 @@ class VersionViewSet(NestedViewSetMixin, DetailSerializerMixin, ReadOnlyModelVie request_body=VersionMetadataSerializer(), responses={200: VersionDetailSerializer()}, ) - # @permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk')) + @method_decorator(permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk'))) def update(self, request, **kwargs): """Update the metadata of a version.""" version: Version = self.get_object() @@ -64,16 +66,10 @@ def update(self, request, **kwargs): @swagger_auto_schema(request_body=no_body, responses={200: VersionSerializer()}) @action(detail=True, methods=['POST']) - # @permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk')) + @method_decorator(permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk'))) def publish(self, request, **kwargs): old_version = self.get_object() - # TODO @permission_required doesn't work on methods - # https://github.com/django-guardian/django-guardian/issues/723 - response = get_40x_or_None(request, ['owner'], old_version.dandiset, return_403=True) - if response: - return response - new_version = Version.copy(old_version) new_version.doi = doi.create_doi(new_version) From 4bb46db92640b2648c1fa447608b6a660992c815 Mon Sep 17 00:00:00 2001 From: Daniel Chiquito Date: Mon, 5 Apr 2021 14:59:35 -0400 Subject: [PATCH 2/2] Add permissions tests for version view --- dandiapi/api/tests/test_version.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index 1d7a98ae9..abbf0db8d 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -174,6 +174,23 @@ def test_version_rest_update_large(api_client, user, version): assert version.metadata.name == new_name +@pytest.mark.django_db +def test_version_rest_update_not_an_owner(api_client, user, version): + api_client.force_authenticate(user=user) + + new_name = 'A unique and special name!' + new_metadata = {'foo': 'bar', 'num': 123, 'list': ['a', 'b', 'c']} + + assert ( + api_client.put( + f'/api/dandisets/{version.dandiset.identifier}/versions/{version.version}/', + {'metadata': new_metadata, 'name': new_name}, + format='json', + ).status_code + == 403 + ) + + @pytest.mark.django_db def test_version_rest_publish(api_client, user, version, asset): assign_perm('owner', user, version.dandiset) @@ -204,3 +221,14 @@ def test_version_rest_publish(api_client, user, version, asset): assert asset == version.assets.get() assert asset == published_version.assets.get() assert asset.versions.count() == 2 + + +@pytest.mark.django_db +def test_version_rest_publish_not_an_owner(api_client, user, version, asset): + api_client.force_authenticate(user=user) + version.assets.add(asset) + + resp = api_client.post( + f'/api/dandisets/{version.dandiset.identifier}/versions/{version.version}/publish/' + ) + assert resp.status_code == 403