From 75c089ff0ba6d5de9ea5b36c1a0a6d280975e064 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Wed, 20 Jul 2022 12:59:30 -0400 Subject: [PATCH 1/8] OutputList POST functionality and tests --- api/resources/permissions.py | 8 + api/resources/serializers.py | 14 +- api/resources/urls.py | 4 + api/resources/views.py | 36 ++++- api_tests/resources/utils.py | 70 ++++++++ .../resources/views/test_resource_detail.py | 70 +------- .../resources/views/test_resource_list.py | 150 ++++++++++++++++++ osf/models/outcome_artifacts.py | 3 +- 8 files changed, 284 insertions(+), 71 deletions(-) create mode 100644 api_tests/resources/utils.py create mode 100644 api_tests/resources/views/test_resource_list.py diff --git a/api/resources/permissions.py b/api/resources/permissions.py index bcfd0358bab..c972f71e570 100644 --- a/api/resources/permissions.py +++ b/api/resources/permissions.py @@ -43,6 +43,14 @@ def has_permission(self, request, view): return proxy_object.has_permission(auth.user, self.REQUIRED_PERMISSIONS[request.method]) +class ResourceListPermission(ResourcesPermission, permissions.BasePermission): + '''Permissions for the top-level ResourceList endpoint. + + ResourceList only supports POST + ''' + REQUIRED_PERMISSIONS = {'POST': 'admin'} + + class ResourceDetailPermission(ResourcesPermission, permissions.BasePermission): '''Permissions for the top-level ResourcesDetail endpoint. diff --git a/api/resources/serializers.py b/api/resources/serializers.py index b892cb968a7..4978f84de06 100644 --- a/api/resources/serializers.py +++ b/api/resources/serializers.py @@ -1,5 +1,6 @@ from rest_framework import serializers as ser +from api.base.exceptions import Conflict from api.base.serializers import ( EnumField, JSONAPISerializer, @@ -9,7 +10,8 @@ VersionedDateTimeField, ) from api.base.utils import absolute_reverse -from osf.utils.outcomes import ArtifactTypes +from osf.models import Outcome, OutcomeArtifact, Registration +from osf.utils.outcomes import ArtifactTypes, NoPIDError class ResourceSerializer(JSONAPISerializer): @@ -60,3 +62,13 @@ def get_absolute_url(self, obj): 'resource_id': obj._id, }, ) + + def create(self, validated_data): + primary_registration = Registration.load(self.context['request'].data['registration']) + + try: + root_outcome = Outcome.objects.for_registration(primary_registration, create=True) + except NoPIDError: + raise Conflict('Cannot add Resrouces to a Registration without a DOI') + + return OutcomeArtifact.objects.create(outcome=root_outcome) diff --git a/api/resources/urls.py b/api/resources/urls.py index 9a50bedd761..d46f7c1eb7f 100644 --- a/api/resources/urls.py +++ b/api/resources/urls.py @@ -5,6 +5,10 @@ app_name = 'osf' urlpatterns = [ + url( + r'^$', views.ResourceList.as_view(), name=views.ResourceList.view_name, + ), + url( r'^(?P\w+)/$', views.ResourceDetail.as_view(), diff --git a/api/resources/views.py b/api/resources/views.py index 3da4a02a3f3..d46486b1ca0 100644 --- a/api/resources/views.py +++ b/api/resources/views.py @@ -4,18 +4,50 @@ from rest_framework.exceptions import NotFound from api.base import permissions as base_permissions +from api.base.exceptions import JSONAPIException from api.base.parsers import ( JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON, ) from api.base.views import JSONAPIBaseView -from api.resources.permissions import ResourceDetailPermission +from api.resources.permissions import ResourceDetailPermission, ResourceListPermission from api.resources.serializers import ResourceSerializer from framework.auth.oauth_scopes import CoreScopes -from osf.models import Guid, OutcomeArtifact +from osf.models import Guid, OutcomeArtifact, Registration logger = logging.getLogger(__name__) + +class ResourceList(JSONAPIBaseView, generics.ListCreateAPIView): + + permission_classes = ( + ResourceListPermission, + drf_permissions.IsAuthenticatedOrReadOnly, + base_permissions.TokenHasScope, + ) + + required_read_scopes = [CoreScopes.READ_REGISTRATION_RESOURCES] + required_write_scopes = [CoreScopes.WRITE_REGISTRATION_RESOURCES] + + view_category = 'resources' + view_name = 'resource-list' + + serializer_class = ResourceSerializer + + parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON) + + def get_permissions_proxy(self): + try: + registration_guid = self.request.data['registration'] + except KeyError: + raise JSONAPIException('Must provide "registration" relationship in payload"') + + registration = Registration.load(registration_guid) + if not registration: + raise NotFound + return registration + + class ResourceDetail(JSONAPIBaseView, generics.RetrieveUpdateDestroyAPIView): permission_classes = ( diff --git a/api_tests/resources/utils.py b/api_tests/resources/utils.py new file mode 100644 index 00000000000..ecac7503e06 --- /dev/null +++ b/api_tests/resources/utils.py @@ -0,0 +1,70 @@ +from django.utils import timezone + +from api.providers.workflows import Workflows as ModerationWorkflows +from api_tests.utils import UserRoles +from osf.models import Outcome +from osf.utils.outcomes import ArtifactTypes +from osf.utils.workflows import RegistrationModerationStates as RegStates +from osf_tests.factories import ( + AuthUserFactory, + RegistrationFactory, + RegistrationProviderFactory +) + +TEST_EXTERNAL_PID = 'This is a doi' + +# Omitted the following redundant states: +# PENDING_EMBARGO_TERMINATION (overlaps EMBARGO) +# PENDING_WITHDRAW_REQESST and PENDING_WITHDRAW (ovrlaps ACCEPTED) +# REVERTED (overlaps REJECTED) +# +# Techncically PENDING and EMBARGO overlap as well, but worth confirming EMBARGO behavior +STATE_VISIBILITY_MAPPINGS = { + RegStates.INITIAL: {'public': False, 'deleted': False}, + RegStates.PENDING: {'public': False, 'deleted': False}, + RegStates.EMBARGO: {'public': False, 'deleted': False}, + RegStates.ACCEPTED: {'public': True, 'deleted': False}, + RegStates.WITHDRAWN: {'public': True, 'deleted': False}, + RegStates.REJECTED: {'public': False, 'deleted': True}, + # Use the generally unreachable UNDEFINED value for the edge case of deleted and public + RegStates.UNDEFINED: {'public': True, 'deleted': True}, +} + + +def configure_test_preconditions(registration_state=RegStates.ACCEPTED, user_role=UserRoles.ADMIN_USER): + provider = RegistrationProviderFactory() + provider.update_group_permissions() + provider.reviews_workflow = ModerationWorkflows.PRE_MODERATION.value + provider.save() + + state_settings = STATE_VISIBILITY_MAPPINGS[registration_state] + registration = RegistrationFactory( + provider=provider, + is_public=state_settings['public'], + has_doi=True + ) + registration.moderation_state = registration_state.db_name + registration.deleted = timezone.now() if state_settings['deleted'] else None + registration.save() + + outcome = Outcome.objects.for_registration(registration, create=True) + test_artifact = outcome.add_artifact_by_pid( + pid=TEST_EXTERNAL_PID, artifact_type=ArtifactTypes.DATA, create_identifier=True + ) + + test_auth = configure_test_auth(registration, user_role) + + return test_artifact, test_auth, registration + + +def configure_test_auth(registration, user_role): + if user_role is UserRoles.UNAUTHENTICATED: + return None + + user = AuthUserFactory() + if user_role is UserRoles.MODERATOR: + registration.provider.get_group('moderator').user_set.add(user) + elif user_role in UserRoles.contributor_roles(): + registration.add_contributor(user, user_role.get_permissions_string()) + + return user.auth diff --git a/api_tests/resources/views/test_resource_detail.py b/api_tests/resources/views/test_resource_detail.py index 1553855b0f6..cece33d6282 100644 --- a/api_tests/resources/views/test_resource_detail.py +++ b/api_tests/resources/views/test_resource_detail.py @@ -1,38 +1,12 @@ import pytest -from django.utils import timezone - from api.providers.workflows import Workflows as ModerationWorkflows from api_tests.utils import UserRoles -from osf.models import Outcome +from api_tests.resources.utils import configure_test_preconditions + from osf.utils.workflows import RegistrationModerationStates as RegStates -from osf_tests.factories import ( - AuthUserFactory, - PrivateLinkFactory, - RegistrationFactory, - RegistrationProviderFactory -) from osf.utils.outcomes import ArtifactTypes -from api.resources import urls, permissions, views, serializers # noqa - -TEST_EXTERNAL_PID = 'This is a doi' - -# Omitted the following redundant states: -# PENDING_EMBARGO_TERMINATION (overlaps EMBARGO) -# PENDING_WITHDRAW_REQESST and PENDING_WITHDRAW (ovrlaps ACCEPTED) -# REVERTED (overlaps REJECTED) -# -# Techncically PENDING and EMBARGO overlap as well, but worth confirming EMBARGO behavior -STATE_VISIBILITY_MAPPINGS = { - RegStates.INITIAL: {'public': False, 'deleted': False}, - RegStates.PENDING: {'public': False, 'deleted': False}, - RegStates.EMBARGO: {'public': False, 'deleted': False}, - RegStates.ACCEPTED: {'public': True, 'deleted': False}, - RegStates.WITHDRAWN: {'public': True, 'deleted': False}, - RegStates.REJECTED: {'public': False, 'deleted': True}, - # Use the generally unreachable UNDEFINED value for the edge case of deleted and public - RegStates.UNDEFINED: {'public': True, 'deleted': True}, -} +from osf_tests.factories import PrivateLinkFactory def make_api_url(resource, vol_key=None): @@ -41,44 +15,6 @@ def make_api_url(resource, vol_key=None): return f'{base_url}?view_only={vol_key}' return base_url -def configure_test_preconditions(registration_state=RegStates.ACCEPTED, user_role=UserRoles.ADMIN_USER): - provider = RegistrationProviderFactory() - provider.update_group_permissions() - provider.reviews_workflow = ModerationWorkflows.PRE_MODERATION.value - provider.save() - - state_settings = STATE_VISIBILITY_MAPPINGS[registration_state] - registration = RegistrationFactory( - provider=provider, - is_public=state_settings['public'], - has_doi=True - ) - registration.moderation_state = registration_state.db_name - registration.deleted = timezone.now() if state_settings['deleted'] else None - registration.save() - - outcome = Outcome.objects.for_registration(registration, create=True) - test_artifact = outcome.add_artifact_by_pid( - pid=TEST_EXTERNAL_PID, artifact_type=ArtifactTypes.DATA, create_identifier=True - ) - - test_auth = _configure_permissions_test_auth(registration, user_role) - - return test_artifact, test_auth, registration - - -def _configure_permissions_test_auth(registration, user_role): - if user_role is UserRoles.UNAUTHENTICATED: - return None - - user = AuthUserFactory() - if user_role is UserRoles.MODERATOR: - registration.provider.get_group('moderator').user_set.add(user) - elif user_role in UserRoles.contributor_roles(): - registration.add_contributor(user, user_role.get_permissions_string()) - - return user.auth - @pytest.mark.django_db class TestResourceDetailGETPermissions: diff --git a/api_tests/resources/views/test_resource_list.py b/api_tests/resources/views/test_resource_list.py new file mode 100644 index 00000000000..303f7d15b8f --- /dev/null +++ b/api_tests/resources/views/test_resource_list.py @@ -0,0 +1,150 @@ +import pytest + +from django.utils import timezone + +from api_tests.resources.utils import configure_test_auth +from api_tests.utils import UserRoles +from osf.models import Outcome, OutcomeArtifact +from osf.utils.outcomes import ArtifactTypes +from osf.utils.workflows import RegistrationModerationStates as RegStates +from osf_tests.factories import AuthUserFactory, RegistrationFactory + +POST_URL = '/v2/resources/' + +@pytest.fixture +def admin_user(): + return AuthUserFactory() + + +@pytest.fixture +def registration(admin_user): + return RegistrationFactory(creator=admin_user, is_public=True, has_doi=True) + + +@pytest.fixture +def payload(registration): + return { + 'data': { + 'type': 'resources', + 'relationships': { + 'registration': { + 'data': { + 'id': registration._id, + 'type': 'registrations' + } + } + }, + } + } + +@pytest.mark.django_db +class TestResourceListPOSTPermissions: + + def test_status_code__admin(self, app, registration, admin_user, payload): + resp = app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) + assert resp.status_code == 201 + + @pytest.mark.parametrize('user_role', [role for role in UserRoles if role is not UserRoles.ADMIN_USER]) + def test_status_code__non_admin(self, app, registration, payload, user_role): + test_auth = configure_test_auth(registration, UserRoles.ADMIN_USER) + resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) + assert resp.status_code == 403 if test_auth else 401 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_status_code__withdrawn_registration(self, app, registration, payload, user_role): + registration.moderation_state = RegStates.WITHDRAWN.db_name + registration.save() + + test_auth = configure_test_auth(registration, UserRoles.ADMIN_USER) + resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) + assert resp.status_code == 403 if test_auth else 401 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_status_code__deleted_registration(self, app, registration, payload, user_role): + registration.deleted = timezone.now() + registration.save() + + test_auth = configure_test_auth(registration, UserRoles.ADMIN_USER) + resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) + assert resp.status_code == 410 + + +@pytest.mark.django_db +class TestResourceListPOSTBehavior: + + @pytest.fixture + def outcome(self, registration): + return Outcome.objects.for_registration(registration, create=True) + + @pytest.fixture + def alternate_outcome(self, registration): + alternate_outcome = Outcome.objects.for_registration( + registration=RegistrationFactory(has_doi=True) + ) + alternate_outcome.artifact_metadata.create(identifier=registration.get_identifier(category='doi')) + return alternate_outcome + + def test_post_adds_artifact_to_existing_outcome(self, app, outcome, registration, admin_user, payload): + assert outcome.artifact_metadata.count() == 1 + + app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) + + assert outcome.artifact_metadata.count() == 2 + + def test_post_creates_outcome_and_primary_artifact_if_none_exists(self, app, registration, admin_user, payload): + assert not OutcomeArtifact.objects.exists() + assert not Outcome.objects.exists() + + app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) + + created_outcome = Outcome.objects.for_registration(registration, create=False) + assert created_outcome is not None + assert created_outcome.artifact_metadata.count() == 2 + + primary_artifact = created_outcome.artifact_metadata.get(artifact_type=ArtifactTypes.PRIMARY) + assert primary_artifact.identifier == registration.get_identifier(category='doi') + + def test_post_created_artifact_has_default_metadata(self, app, outcome, registration, admin_user, payload): + app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) + + created_artifact = outcome.artifact_metadata.order_by('-created').first() + + assert not created_artifact.title + assert not created_artifact.description + assert not created_artifact.identifier + assert not created_artifact.pid + assert created_artifact.artifact_type == ArtifactTypes.UNDEFINED + assert created_artifact.primary_resource_guid == registration._id + + def test_post_adds_artifact_to_primary_outcome_for_registration( + self, app, outcome, registration, admin_user, payload + ): + # Create an outcome where the Registration is *an* artifact but not the *primary* artifact + alternate_outcome = Outcome.objects.for_registration( + registration=RegistrationFactory(has_doi=True), create=True + ) + alternate_outcome.artifact_metadata.create(identifier=registration.get_identifier(category='doi')) + + assert alternate_outcome.artifact_metadata.count() == 2 + assert outcome.artifact_metadata.count() == 1 + + app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) + + assert alternate_outcome.artifact_metadata.count() == 2 + assert outcome.artifact_metadata.count() == 2 + + def test_post_fails_if_no_registration(self, app): + payload = {'data': {'type': 'resources'}} + resp = app.post_json_api(POST_URL, payload, auth=None, expect_errors=True) + assert resp.status_code == 400 + + def test_post_fails_with_404_if_registration_does_not_exist(self, app): + payload = { + 'data': { + 'type': 'resources', + 'relationships': {'registration': {'data': {'id': 'QWERT', 'type': 'registrations'}}} + } + } + + resp = app.post_json_api(POST_URL, payload, auth=None, expect_errors=True) + assert resp.status_code == 404 diff --git a/osf/models/outcome_artifacts.py b/osf/models/outcome_artifacts.py index 201b3cf68df..a61a2bf87a0 100644 --- a/osf/models/outcome_artifacts.py +++ b/osf/models/outcome_artifacts.py @@ -82,7 +82,8 @@ class OutcomeArtifact(ObjectIDMixin, BaseModel): identifier = models.ForeignKey( 'osf.identifier', on_delete=models.CASCADE, - related_name='artifact_metadata' + related_name='artifact_metadata', + null=True, ) artifact_type = models.IntegerField( From 714f3faf5810456a17b4621a98fb3a8397b4f718 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Wed, 20 Jul 2022 15:59:32 -0400 Subject: [PATCH 2/8] Add constraints around Registration state --- api/resources/serializers.py | 8 +++++- .../resources/views/test_resource_list.py | 25 +++++++++++++++---- osf/models/registrations.py | 13 +++++++--- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/api/resources/serializers.py b/api/resources/serializers.py index 4978f84de06..382bc246903 100644 --- a/api/resources/serializers.py +++ b/api/resources/serializers.py @@ -64,7 +64,13 @@ def get_absolute_url(self, obj): ) def create(self, validated_data): - primary_registration = Registration.load(self.context['request'].data['registration']) + # Already loaded by view, so no need to error check + guid = self.context['request'].data['registration'] + primary_registration = Registration.load(guid) + if not primary_registration.is_registration_approved: + raise Conflict( + f'Registration {guid} is pending approval and cannot have associated resources', + ) try: root_outcome = Outcome.objects.for_registration(primary_registration, create=True) diff --git a/api_tests/resources/views/test_resource_list.py b/api_tests/resources/views/test_resource_list.py index 303f7d15b8f..2431f6e7182 100644 --- a/api_tests/resources/views/test_resource_list.py +++ b/api_tests/resources/views/test_resource_list.py @@ -18,7 +18,10 @@ def admin_user(): @pytest.fixture def registration(admin_user): - return RegistrationFactory(creator=admin_user, is_public=True, has_doi=True) + registration = RegistrationFactory(creator=admin_user, is_public=True, has_doi=True) + registration.moderation_state = RegStates.ACCEPTED.db_name + registration.save() + return registration @pytest.fixture @@ -46,18 +49,20 @@ def test_status_code__admin(self, app, registration, admin_user, payload): @pytest.mark.parametrize('user_role', [role for role in UserRoles if role is not UserRoles.ADMIN_USER]) def test_status_code__non_admin(self, app, registration, payload, user_role): - test_auth = configure_test_auth(registration, UserRoles.ADMIN_USER) + test_auth = configure_test_auth(registration, user_role) resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) - assert resp.status_code == 403 if test_auth else 401 + expected_code = 403 if user_role is not UserRoles.UNAUTHENTICATED else 401 + assert resp.status_code == expected_code @pytest.mark.parametrize('user_role', UserRoles) def test_status_code__withdrawn_registration(self, app, registration, payload, user_role): registration.moderation_state = RegStates.WITHDRAWN.db_name registration.save() - test_auth = configure_test_auth(registration, UserRoles.ADMIN_USER) + test_auth = configure_test_auth(registration, user_role) resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) - assert resp.status_code == 403 if test_auth else 401 + expected_code = 403 if user_role is not UserRoles.UNAUTHENTICATED else 401 + assert resp.status_code == expected_code @pytest.mark.parametrize('user_role', UserRoles) def test_status_code__deleted_registration(self, app, registration, payload, user_role): @@ -148,3 +153,13 @@ def test_post_fails_with_404_if_registration_does_not_exist(self, app): resp = app.post_json_api(POST_URL, payload, auth=None, expect_errors=True) assert resp.status_code == 404 + + @pytest.mark.parametrize('registration_state', [RegStates.INITIAL, RegStates.PENDING]) + def test_post_fails_with_409_if_registration_not_approved( + self, app, registration, admin_user, payload, registration_state + ): + registration.moderation_state = registration_state.db_name + registration.save() + + resp = app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) + assert resp.status_code == 409 diff --git a/osf/models/registrations.py b/osf/models/registrations.py index 1fed97ffdfe..35ae000998f 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -249,10 +249,15 @@ def sanction(self): @property def is_registration_approved(self): - root = self._dirty_root - if root.registration_approval is None: - return False - return root.registration_approval.is_approved + pending_states = [ + RegistrationModerationStates.INITIAL.db_name, + RegistrationModerationStates.PENDING.db_name + ] + return self.moderation_state not in pending_states +# root = self._dirty_root +# if root.registration_approval is None: +# return False +# return root.registration_approval.is_approved @property def is_pending_embargo(self): From 99209a6d4efe9ee124e484b7138f9d7f0ccb3cdc Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Wed, 20 Jul 2022 16:30:21 -0400 Subject: [PATCH 3/8] Add tests for unsupported methods and clean up --- api/resources/permissions.py | 2 -- api_tests/resources/utils.py | 2 +- .../resources/views/test_resource_list.py | 27 +++++++++++++++++++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/api/resources/permissions.py b/api/resources/permissions.py index c972f71e570..45e834c8d32 100644 --- a/api/resources/permissions.py +++ b/api/resources/permissions.py @@ -1,10 +1,8 @@ - from rest_framework import exceptions from rest_framework import permissions from api.base.exceptions import Gone from api.base.utils import get_user_auth, assert_resource_type - from osf.models import Registration diff --git a/api_tests/resources/utils.py b/api_tests/resources/utils.py index ecac7503e06..787d67c62d0 100644 --- a/api_tests/resources/utils.py +++ b/api_tests/resources/utils.py @@ -15,7 +15,7 @@ # Omitted the following redundant states: # PENDING_EMBARGO_TERMINATION (overlaps EMBARGO) -# PENDING_WITHDRAW_REQESST and PENDING_WITHDRAW (ovrlaps ACCEPTED) +# PENDING_WITHDRAW_REQUEST and PENDING_WITHDRAW (overlaps ACCEPTED) # REVERTED (overlaps REJECTED) # # Techncically PENDING and EMBARGO overlap as well, but worth confirming EMBARGO behavior diff --git a/api_tests/resources/views/test_resource_list.py b/api_tests/resources/views/test_resource_list.py index 2431f6e7182..6c3e2986ef0 100644 --- a/api_tests/resources/views/test_resource_list.py +++ b/api_tests/resources/views/test_resource_list.py @@ -163,3 +163,30 @@ def test_post_fails_with_409_if_registration_not_approved( resp = app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) assert resp.status_code == 409 + +@pytest.mark.django_db +class TestResourceListUnsupportedMethods: + + @pytest.mark.parametrize('user_role', UserRoles) + def test_cannot_GET(self, app, registration, user_role): + test_auth = configure_test_auth(registration, user_role) + resp = app.get(POST_URL, auth=test_auth, expect_errors=True) + assert resp.status_code == 405 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_cannot_PUT(self, app, registration, user_role): + test_auth = configure_test_auth(registration, user_role) + resp = app.put_json_api(POST_URL, auth=test_auth, expect_errors=True) + assert resp.status_code == 405 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_cannot_PATCH(self, app, registration, user_role): + test_auth = configure_test_auth(registration, user_role) + resp = app.patch_json_api(POST_URL, auth=test_auth, expect_errors=True) + assert resp.status_code == 405 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_cannot_DELETE(self, app, registration, user_role): + test_auth = configure_test_auth(registration, user_role) + resp = app.delete_json_api(POST_URL, auth=test_auth, expect_errors=True) + assert resp.status_code == 405 From 89e1a9f44da9b19ef9ceea6319425642332ed762 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Thu, 21 Jul 2022 10:29:05 -0400 Subject: [PATCH 4/8] Undo changes around registration state checking in favor of future identifier workflow changes --- api/resources/serializers.py | 4 ---- api_tests/resources/views/test_resource_list.py | 9 +++------ osf/models/registrations.py | 13 ++++--------- 3 files changed, 7 insertions(+), 19 deletions(-) diff --git a/api/resources/serializers.py b/api/resources/serializers.py index 382bc246903..bdd578319db 100644 --- a/api/resources/serializers.py +++ b/api/resources/serializers.py @@ -67,10 +67,6 @@ def create(self, validated_data): # Already loaded by view, so no need to error check guid = self.context['request'].data['registration'] primary_registration = Registration.load(guid) - if not primary_registration.is_registration_approved: - raise Conflict( - f'Registration {guid} is pending approval and cannot have associated resources', - ) try: root_outcome = Outcome.objects.for_registration(primary_registration, create=True) diff --git a/api_tests/resources/views/test_resource_list.py b/api_tests/resources/views/test_resource_list.py index 6c3e2986ef0..0dd906ad268 100644 --- a/api_tests/resources/views/test_resource_list.py +++ b/api_tests/resources/views/test_resource_list.py @@ -154,12 +154,9 @@ def test_post_fails_with_404_if_registration_does_not_exist(self, app): resp = app.post_json_api(POST_URL, payload, auth=None, expect_errors=True) assert resp.status_code == 404 - @pytest.mark.parametrize('registration_state', [RegStates.INITIAL, RegStates.PENDING]) - def test_post_fails_with_409_if_registration_not_approved( - self, app, registration, admin_user, payload, registration_state - ): - registration.moderation_state = registration_state.db_name - registration.save() + def test_post_fails_with_409_if_no_registration_identifier(self, app, admin_user, payload): + registration = RegistrationFactory(creator=admin_user, has_doi=False) + payload['data']['relationships']['registration']['data']['id'] = registration._id resp = app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) assert resp.status_code == 409 diff --git a/osf/models/registrations.py b/osf/models/registrations.py index 35ae000998f..1fed97ffdfe 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -249,15 +249,10 @@ def sanction(self): @property def is_registration_approved(self): - pending_states = [ - RegistrationModerationStates.INITIAL.db_name, - RegistrationModerationStates.PENDING.db_name - ] - return self.moderation_state not in pending_states -# root = self._dirty_root -# if root.registration_approval is None: -# return False -# return root.registration_approval.is_approved + root = self._dirty_root + if root.registration_approval is None: + return False + return root.registration_approval.is_approved @property def is_pending_embargo(self): From 3ffdf190b0462b0c73245de591ac6fde53d6ce63 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Thu, 21 Jul 2022 11:46:49 -0400 Subject: [PATCH 5/8] moar cleanup --- osf/models/outcome_artifacts.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/osf/models/outcome_artifacts.py b/osf/models/outcome_artifacts.py index 932a6c8fcbf..b167a8e0216 100644 --- a/osf/models/outcome_artifacts.py +++ b/osf/models/outcome_artifacts.py @@ -83,8 +83,7 @@ class OutcomeArtifact(ObjectIDMixin, BaseModel): 'osf.identifier', null=True, on_delete=models.CASCADE, - related_name='artifact_metadata', - null=True, + related_name='artifact_metadata' ) artifact_type = models.IntegerField( From 4563673a5aad323844b60c99f63730b626515a8f Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Thu, 21 Jul 2022 13:12:14 -0400 Subject: [PATCH 6/8] [ENG-3895][ENG-3897]ResourceSerializer and ResourceDetail GET implementation (#9980) ResourceSerializer and GET functionality on ResourceDetail Co-authored-by: Jon Walz --- api/base/serializers.py | 17 ++ api/base/urls.py | 3 +- api/resources/permissions.py | 50 ++++++ api/resources/serializers.py | 62 +++++++ api/resources/urls.py | 13 ++ api/resources/views.py | 44 +++++ api_tests/resources/utils.py | 70 ++++++++ .../resources/views/test_resource_detail.py | 158 ++++++++++++++++++ api_tests/utils.py | 30 ++++ framework/auth/oauth_scopes.py | 6 +- osf/models/outcome_artifacts.py | 16 +- osf/utils/outcomes.py | 24 +++ osf_tests/factories.py | 6 +- osf_tests/test_outcomes.py | 5 + tasks/__init__.py | 1 + 15 files changed, 497 insertions(+), 8 deletions(-) create mode 100644 api/resources/permissions.py create mode 100644 api/resources/serializers.py create mode 100644 api/resources/urls.py create mode 100644 api/resources/views.py create mode 100644 api_tests/resources/utils.py create mode 100644 api_tests/resources/views/test_resource_detail.py diff --git a/api/base/serializers.py b/api/base/serializers.py index 5d3f7e996f7..a2e487f1bee 100644 --- a/api/base/serializers.py +++ b/api/base/serializers.py @@ -1993,3 +1993,20 @@ def __init__(self, switch_name: str, field: ser.Field, hide_if: bool = False, ** """ super(DisableIfSwitch, self).__init__(switch_name, field, hide_if, **kwargs) self.validators.append(SwitchValidator(self.switch_name)) + + +class EnumField(ser.ChoiceField): + """ + Custom field for using Int enums internally in a human-readable way. + """ + def __init__(self, enum_class, **kwargs): + self._enum_class = enum_class + # We usually define this on the Enum class + choices = tuple((entry.value, entry.name.lower()) for entry in enum_class) + super().__init__(choices=choices, **kwargs) + + def to_representation(self, value): + return self._enum_class(value).name.lower() + + def to_internal_value(self, data): + return self._enum_class[data.upper()].value diff --git a/api/base/urls.py b/api/base/urls.py index 1e97ce11c2a..52c79178ad6 100644 --- a/api/base/urls.py +++ b/api/base/urls.py @@ -57,10 +57,11 @@ url(r'^nodes/', include('api.nodes.urls', namespace='nodes')), url(r'^preprints/', include('api.preprints.urls', namespace='preprints')), url(r'^preprint_providers/', include('api.preprint_providers.urls', namespace='preprint_providers')), - url(r'^regions/', include('api.regions.urls', namespace='regions')), url(r'^providers/', include('api.providers.urls', namespace='providers')), + url(r'^regions/', include('api.regions.urls', namespace='regions')), url(r'^registrations/', include('api.registrations.urls', namespace='registrations')), url(r'^requests/', include(('api.requests.urls', 'requests'), namespace='requests')), + url(r'^resources/', include('api.resources.urls', namespace='resources')), url(r'^scopes/', include('api.scopes.urls', namespace='scopes')), url(r'^search/', include('api.search.urls', namespace='search')), url(r'^sparse/', include('api.sparse.urls', namespace='sparse')), diff --git a/api/resources/permissions.py b/api/resources/permissions.py new file mode 100644 index 00000000000..3073647a76a --- /dev/null +++ b/api/resources/permissions.py @@ -0,0 +1,50 @@ +from rest_framework import exceptions +from rest_framework import permissions + +from api.base.exceptions import Gone +from api.base.utils import get_user_auth, assert_resource_type +from osf.models import Registration + + +class ResourcesPermission: + '''Base permissions class for acting on Resources. + + To GET an Resource (when the method is available), the user must have `can_view` access + to the proxy_object designated by the view (i.e. the Primary Registration). + + For all other methods, the user must have the permissions designated by the + REQUIRED_PERMISSIONS dictionary for the Permission subclass + + Note: SchemaResponses for deleted parent resources should appear to be deleted, + while access should be denied to SchemaResponses on withdrawn parent resources. + ''' + acceptable_models = (Registration, ) + REQUIRED_PERMISSIONS = {} + + def has_permission(self, request, view): + if request.method not in self.REQUIRED_PERMISSIONS.keys(): + raise exceptions.MethodNotAllowed(request.method) + + proxy_object = view.get_permissions_proxy() + assert_resource_type(proxy_object, self.acceptable_models) + if proxy_object.deleted: + # Mimics get_object_or_error logic + raise Gone + if getattr(proxy_object, 'is_retracted', False): + # Mimics behavior of ExcludeWithdrawals for Registration Resources + return False + + auth = get_user_auth(request) + if request.method in permissions.SAFE_METHODS: + return proxy_object.is_public or proxy_object.can_view(auth) + + return proxy_object.has_permission(auth.user, self.REQUIRED_PERMISSIONS[request.method]) + + +class ResourceDetailPermission(ResourcesPermission, permissions.BasePermission): + '''Permissions for the top-level ResourcesDetail endpoint. + + ResourcesDetail supports GET, PATCH, and DELETE methods. + ''' + # TODO(ENG-3898, ENG-3899) Implement PATCH and DELETE + REQUIRED_PERMISSIONS = {'GET': None} diff --git a/api/resources/serializers.py b/api/resources/serializers.py new file mode 100644 index 00000000000..b892cb968a7 --- /dev/null +++ b/api/resources/serializers.py @@ -0,0 +1,62 @@ +from rest_framework import serializers as ser + +from api.base.serializers import ( + EnumField, + JSONAPISerializer, + LinksField, + RelationshipField, + TypeField, + VersionedDateTimeField, +) +from api.base.utils import absolute_reverse +from osf.utils.outcomes import ArtifactTypes + +class ResourceSerializer(JSONAPISerializer): + + non_anonymized_fields = frozenset([ + 'id', + 'type', + 'date_created', + 'date_modified', + 'name', + 'description', + 'registration', + 'links', + ]) + + class Meta: + type_ = 'resources' + + id = ser.CharField(source='_id', read_only=True, required=False) + type = TypeField() + + date_created = VersionedDateTimeField(source='created', required=False) + date_modified = VersionedDateTimeField(source='modified', required=False) + + name = ser.CharField(allow_null=False, allow_blank=True, required=False) + description = ser.CharField(allow_null=False, allow_blank=True, required=False) + resource_type = EnumField(ArtifactTypes, source='artifact_type', allow_null=False, required=False) + finalized = ser.BooleanField(required=False) + + # Reference to obj.identifier.value, populated via annotation on default manager + pid = ser.CharField(allow_null=False, allow_blank=True, required=False) + + # primary_resource_guid is populated via annotation on the default manager + registration = RelationshipField( + related_view='registrations:registration-detail', + related_view_kwargs={'node_id': ''}, + read_only=True, + required=False, + allow_null=True, + ) + + links = LinksField({'self': 'get_absolute_url'}) + + def get_absolute_url(self, obj): + return absolute_reverse( + 'resources:resource-detail', + kwargs={ + 'version': self.context['request'].parser_context['kwargs']['version'], + 'resource_id': obj._id, + }, + ) diff --git a/api/resources/urls.py b/api/resources/urls.py new file mode 100644 index 00000000000..9a50bedd761 --- /dev/null +++ b/api/resources/urls.py @@ -0,0 +1,13 @@ +from django.conf.urls import url + +from api.resources import views + +app_name = 'osf' + +urlpatterns = [ + url( + r'^(?P\w+)/$', + views.ResourceDetail.as_view(), + name=views.ResourceDetail.view_name, + ), +] diff --git a/api/resources/views.py b/api/resources/views.py new file mode 100644 index 00000000000..3da4a02a3f3 --- /dev/null +++ b/api/resources/views.py @@ -0,0 +1,44 @@ +import logging + +from rest_framework import generics, permissions as drf_permissions +from rest_framework.exceptions import NotFound + +from api.base import permissions as base_permissions +from api.base.parsers import ( + JSONAPIMultipleRelationshipsParser, + JSONAPIMultipleRelationshipsParserForRegularJSON, +) +from api.base.views import JSONAPIBaseView +from api.resources.permissions import ResourceDetailPermission +from api.resources.serializers import ResourceSerializer +from framework.auth.oauth_scopes import CoreScopes +from osf.models import Guid, OutcomeArtifact + +logger = logging.getLogger(__name__) + +class ResourceDetail(JSONAPIBaseView, generics.RetrieveUpdateDestroyAPIView): + + permission_classes = ( + ResourceDetailPermission, + drf_permissions.IsAuthenticatedOrReadOnly, + base_permissions.TokenHasScope, + ) + + required_read_scopes = [CoreScopes.READ_REGISTRATION_RESOURCES] + required_write_scopes = [CoreScopes.WRITE_REGISTRATION_RESOURCES] + + view_category = 'resources' + view_name = 'resource-detail' + + serializer_class = ResourceSerializer + + parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON) + + def get_object(self): + try: + return OutcomeArtifact.objects.get(_id=self.kwargs['resource_id']) + except OutcomeArtifact.DoesNotExist: + raise NotFound + + def get_permissions_proxy(self): + return Guid.load(self.get_object().primary_resource_guid).referent diff --git a/api_tests/resources/utils.py b/api_tests/resources/utils.py new file mode 100644 index 00000000000..c411177a8ae --- /dev/null +++ b/api_tests/resources/utils.py @@ -0,0 +1,70 @@ +from django.utils import timezone + +from api.providers.workflows import Workflows as ModerationWorkflows +from api_tests.utils import UserRoles +from osf.models import Outcome +from osf.utils.workflows import RegistrationModerationStates as RegStates +from osf_tests.factories import ( + AuthUserFactory, + RegistrationFactory, + RegistrationProviderFactory +) +from osf.utils.outcomes import ArtifactTypes + +TEST_EXTERNAL_PID = 'This is a doi' + +# Omitted the following redundant states: +# PENDING_EMBARGO_TERMINATION (overlaps EMBARGO) +# PENDING_WITHDRAW_REQUEST and PENDING_WITHDRAW (overlaps ACCEPTED) +# REVERTED (overlaps REJECTED) +# +# Techncically PENDING and EMBARGO overlap as well, but worth confirming EMBARGO behavior +STATE_VISIBILITY_MAPPINGS = { + RegStates.INITIAL: {'public': False, 'deleted': False}, + RegStates.PENDING: {'public': False, 'deleted': False}, + RegStates.EMBARGO: {'public': False, 'deleted': False}, + RegStates.ACCEPTED: {'public': True, 'deleted': False}, + RegStates.WITHDRAWN: {'public': True, 'deleted': False}, + RegStates.REJECTED: {'public': False, 'deleted': True}, + # Use the generally unreachable UNDEFINED value for the edge case of deleted and public + RegStates.UNDEFINED: {'public': True, 'deleted': True}, +} + + +def configure_test_preconditions(registration_state=RegStates.ACCEPTED, user_role=UserRoles.ADMIN_USER): + provider = RegistrationProviderFactory() + provider.update_group_permissions() + provider.reviews_workflow = ModerationWorkflows.PRE_MODERATION.value + provider.save() + + state_settings = STATE_VISIBILITY_MAPPINGS[registration_state] + registration = RegistrationFactory( + provider=provider, + is_public=state_settings['public'], + has_doi=True + ) + registration.moderation_state = registration_state.db_name + registration.deleted = timezone.now() if state_settings['deleted'] else None + registration.save() + + outcome = Outcome.objects.for_registration(registration, create=True) + test_artifact = outcome.add_artifact_by_pid( + pid=TEST_EXTERNAL_PID, artifact_type=ArtifactTypes.DATA, create_identifier=True + ) + + test_auth = configure_test_auth(registration, user_role) + + return test_artifact, test_auth, registration + + +def configure_test_auth(registration, user_role): + if user_role is UserRoles.UNAUTHENTICATED: + return None + + user = AuthUserFactory() + if user_role is UserRoles.MODERATOR: + registration.provider.get_group('moderator').user_set.add(user) + elif user_role in UserRoles.contributor_roles(): + registration.add_contributor(user, user_role.get_permissions_string()) + + return user.auth diff --git a/api_tests/resources/views/test_resource_detail.py b/api_tests/resources/views/test_resource_detail.py new file mode 100644 index 00000000000..fe8bebb04e2 --- /dev/null +++ b/api_tests/resources/views/test_resource_detail.py @@ -0,0 +1,158 @@ +import pytest + +from api.providers.workflows import Workflows as ModerationWorkflows +from api_tests.resources.utils import configure_test_preconditions +from api_tests.utils import UserRoles +from osf.utils.workflows import RegistrationModerationStates as RegStates +from osf_tests.factories import PrivateLinkFactory +from osf.utils.outcomes import ArtifactTypes + + +def make_api_url(resource, vol_key=None): + base_url = f'/v2/resources/{resource._id}/' + if vol_key: + return f'{base_url}?view_only={vol_key}' + return base_url + + +@pytest.mark.django_db +class TestResourceDetailGETPermissions: + + @pytest.mark.parametrize('user_role', UserRoles) + def test_status_code__public(self, app, user_role): + test_artifact, test_auth, _ = configure_test_preconditions( + registration_state=RegStates.ACCEPTED, user_role=user_role + ) + resp = app.get(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 200 + + @pytest.mark.parametrize('user_role', UserRoles.contributor_roles()) + def test_status_code__unapproved__contributor(self, app, user_role): + test_artifact, test_auth, _ = configure_test_preconditions( + registration_state=RegStates.INITIAL, user_role=user_role + ) + resp = app.get(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 200 + + @pytest.mark.parametrize('user_role', UserRoles.noncontributor_roles()) + def test_status_code__unapproved__non_contributor(self, app, user_role): + test_artifact, test_auth, _ = configure_test_preconditions( + registration_state=RegStates.INITIAL, user_role=user_role + ) + resp = app.get(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 403 if test_auth else 401 + + @pytest.mark.parametrize('registration_state', [RegStates.PENDING, RegStates.EMBARGO]) + @pytest.mark.parametrize('user_role', UserRoles.contributor_roles(include_moderator=True)) + def test_status_code__pending_or_embargoed__contributor_or_moderator(self, app, registration_state, user_role): + test_artifact, test_auth, registration = configure_test_preconditions( + registration_state=registration_state, user_role=user_role + ) + resp = app.get(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 200 + + @pytest.mark.parametrize('registration_state', [RegStates.PENDING, RegStates.EMBARGO]) + def test_status_code__pending_or_embargoed__moderator__unmoderated(self, app, registration_state): + test_artifact, test_auth, registration = configure_test_preconditions( + registration_state=registration_state, user_role=UserRoles.MODERATOR + ) + provider = registration.provider + provider.reviews_workflow = ModerationWorkflows.NONE.value + provider.save() + + resp = app.get(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 403 + + @pytest.mark.parametrize('registration_state', [RegStates.PENDING, RegStates.EMBARGO]) + @pytest.mark.parametrize('user_role', [UserRoles.UNAUTHENTICATED, UserRoles.NONCONTRIB]) + def test_status_code__pending_or_embargoed__noncontrib(self, app, registration_state, user_role): + test_artifact, test_auth, _ = configure_test_preconditions( + registration_state=registration_state, user_role=user_role + ) + resp = app.get(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 403 if test_auth else 401 + + @pytest.mark.parametrize('registration_state', [RegStates.REJECTED, RegStates.UNDEFINED]) + @pytest.mark.parametrize('user_role', UserRoles) + def test_status_code__deleted(self, app, registration_state, user_role): + test_artifact, test_auth, _ = configure_test_preconditions( + registration_state=registration_state, user_role=user_role + ) + resp = app.get(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 410 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_status_code__withdrawn(self, app, user_role): + test_artifact, test_auth, _ = configure_test_preconditions( + registration_state=RegStates.WITHDRAWN, user_role=user_role + ) + resp = app.get(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 403 if test_auth else 401 + + @pytest.mark.parametrize('registration_state', [RegStates.INITIAL, RegStates.PENDING, RegStates.EMBARGO]) + @pytest.mark.parametrize('user_role', UserRoles.noncontributor_roles()) + def test_status_code__vol(self, app, registration_state, user_role): + test_artifact, test_auth, registration = configure_test_preconditions( + registration_state=registration_state, user_role=user_role + ) + provider = registration.provider + provider.reviews_workflow = ModerationWorkflows.NONE.value + provider.save() + + vol = PrivateLinkFactory(anonymous=False) + vol.nodes.add(registration) + resp = app.get(make_api_url(test_artifact, vol_key=vol.key), auth=test_auth, expect_errors=True) + assert resp.status_code == 200 + + +@pytest.mark.django_db +class TestResourceDetailGETBehavior: + + def test_serialized_data(self, app): + test_artifact, test_auth, _ = configure_test_preconditions() + + resp = app.get(make_api_url(test_artifact), auth=test_auth) + data = resp.json['data'] + + assert data['id'] == test_artifact._id + assert data['type'] == 'resources' + assert data['attributes']['resource_type'] == ArtifactTypes(test_artifact.artifact_type).name.lower() + assert data['attributes']['pid'] == test_artifact.identifier.value + assert data['relationships']['registration']['data']['id'] == test_artifact.outcome.primary_osf_resource._id + + def test_anonymized_data(self, app): + test_artifact, test_auth, registration = configure_test_preconditions() + avol = PrivateLinkFactory(anonymous=True) + avol.nodes.add(registration) + + resp = app.get(make_api_url(test_artifact, vol_key=avol.key), auth=None) + data = resp.json['data'] + assert 'pid' not in data['attributes'] + + +@pytest.mark.django_db +class TestResourceDetailUnsupportedMethods: + + @pytest.mark.parametrize('user_role', UserRoles) + def test_cannot_POST(self, app, user_role): + test_artifact, test_auth, _ = configure_test_preconditions() + resp = app.post_json_api(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 405 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_cannot_PUT(self, app, user_role): + test_artifact, test_auth, _ = configure_test_preconditions() + resp = app.put_json_api(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 405 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_cannot_PATCH(self, app, user_role): + test_artifact, test_auth, _ = configure_test_preconditions() + resp = app.patch_json_api(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 405 + + @pytest.mark.parametrize('user_role', UserRoles) + def test_cannot_DELETE(self, app, user_role): + test_artifact, test_auth, _ = configure_test_preconditions() + resp = app.delete_json_api(make_api_url(test_artifact), auth=test_auth, expect_errors=True) + assert resp.status_code == 405 diff --git a/api_tests/utils.py b/api_tests/utils.py index 279f682c561..05878b28044 100644 --- a/api_tests/utils.py +++ b/api_tests/utils.py @@ -1,4 +1,5 @@ from blinker import ANY +from enum import Enum from future.moves.urllib.parse import urlparse from contextlib import contextmanager from addons.osfstorage import settings as osfstorage_settings @@ -64,3 +65,32 @@ def only_supports_methods(view, expected_methods): view = view() expected_methods.append('OPTIONS') return set(expected_methods) == set(view.allowed_methods) + + +class UserRoles(Enum): + UNAUTHENTICATED = 0 + NONCONTRIB = 1 + MODERATOR = 2 + READ_USER = 3 + WRITE_USER = 4 + ADMIN_USER = 5 + + @classmethod + def contributor_roles(cls, include_moderator=False): + base_roles = [cls.READ_USER, cls.WRITE_USER, cls.ADMIN_USER] + if include_moderator: + return [cls.MODERATOR, *base_roles] + return base_roles + + @classmethod + def noncontributor_roles(cls): + return [cls.UNAUTHENTICATED, cls.NONCONTRIB, cls.MODERATOR] + + def get_permissions_string(self): + if self is UserRoles.READ_USER: + return 'read' + if self is UserRoles.WRITE_USER: + return 'write' + if self is UserRoles.ADMIN_USER: + return 'admin' + return None diff --git a/framework/auth/oauth_scopes.py b/framework/auth/oauth_scopes.py index 65a1bc1abf0..126bd08a981 100644 --- a/framework/auth/oauth_scopes.py +++ b/framework/auth/oauth_scopes.py @@ -189,6 +189,8 @@ class CoreScopes(object): READ_SCHEMA_RESPONSES = 'read_schema_responses' WRITE_SCHEMA_RESPONSES = 'write_schema_responses' + READ_REGISTRATION_RESOURCES = 'read_registration_resources' + WRITE_REGISTRATION_RESOURCES = 'write_registration_resources' class ComposedScopes(object): """ @@ -241,12 +243,12 @@ class ComposedScopes(object): CoreScopes.NODE_CITATIONS_READ, CoreScopes.NODE_COMMENTS_READ, CoreScopes.NODE_LOG_READ, CoreScopes.NODE_FORKS_READ, CoreScopes.WIKI_BASE_READ, CoreScopes.LICENSE_READ, CoreScopes.IDENTIFIERS_READ, CoreScopes.NODE_PREPRINTS_READ, CoreScopes.PREPRINT_REQUESTS_READ, - CoreScopes.REGISTRATION_REQUESTS_READ, CoreScopes.READ_SCHEMA_RESPONSES) + CoreScopes.REGISTRATION_REQUESTS_READ, CoreScopes.READ_SCHEMA_RESPONSES, CoreScopes.READ_REGISTRATION_RESOURCES) NODE_METADATA_WRITE = NODE_METADATA_READ + \ (CoreScopes.NODE_BASE_WRITE, CoreScopes.NODE_CHILDREN_WRITE, CoreScopes.NODE_LINKS_WRITE, CoreScopes.IDENTIFIERS_WRITE, CoreScopes.NODE_CITATIONS_WRITE, CoreScopes.NODE_COMMENTS_WRITE, CoreScopes.NODE_FORKS_WRITE, CoreScopes.NODE_PREPRINTS_WRITE, CoreScopes.PREPRINT_REQUESTS_WRITE, CoreScopes.WIKI_BASE_WRITE, - CoreScopes.REGISTRATION_REQUESTS_WRITE, CoreScopes.WRITE_SCHEMA_RESPONSES) + CoreScopes.REGISTRATION_REQUESTS_WRITE, CoreScopes.WRITE_SCHEMA_RESPONSES, CoreScopes.WRITE_REGISTRATION_RESOURCES) # Preprints collection # TODO: Move Metrics scopes to their own restricted composed scope once the Admin app can manage scopes on tokens/apps diff --git a/osf/models/outcome_artifacts.py b/osf/models/outcome_artifacts.py index 87b73a7df55..b167a8e0216 100644 --- a/osf/models/outcome_artifacts.py +++ b/osf/models/outcome_artifacts.py @@ -2,7 +2,7 @@ from osf.models.base import BaseModel, ObjectIDMixin from osf.models.identifiers import Identifier -from osf.utils.outcomes import ArtifactTypes, NoPIDError +from osf.utils import outcomes as outcome_utils ''' @@ -13,11 +13,17 @@ for the research effort described by the Outcome. ''' + +ArtifactTypes = outcome_utils.ArtifactTypes + + class ArtifactManager(models.Manager): def get_queryset(self): - return super().get_queryset().annotate( - pid=models.F('identifier__value') + base_queryset = super().get_queryset().select_related('identifier') + return base_queryset.annotate( + pid=models.F('identifier__value'), + primary_resource_guid=outcome_utils.make_primary_resource_guid_annotation(base_queryset) ) def create_for_identifier_value( @@ -33,7 +39,9 @@ def create_for_identifier_value( value=pid_value, category=pid_type ) except Identifier.DoesNotExist: - raise NoPIDError('No PID with value {pid_value} found for PID type {pid_type}') + raise outcome_utils.NoPIDError( + 'No PID with value {pid_value} found for PID type {pid_type}' + ) return self.create(outcome=outcome, identifier=identifier, **kwargs) diff --git a/osf/utils/outcomes.py b/osf/utils/outcomes.py index f3f67ec1a51..e5399f5837a 100644 --- a/osf/utils/outcomes.py +++ b/osf/utils/outcomes.py @@ -1,5 +1,7 @@ from enum import IntEnum +from django.db.models import CharField, OuterRef, Subquery + class NoPIDError(Exception): pass @@ -24,3 +26,25 @@ class ArtifactTypes(IntEnum): @classmethod def choices(cls): return tuple((entry.value, entry.name) for entry in cls) + + +def make_primary_resource_guid_annotation(base_queryset): + from osf.models import Guid + primary_artifacts_and_guids = base_queryset.filter( + artifact_type=ArtifactTypes.PRIMARY + ).annotate( + resource_guid=Subquery( + Guid.objects.filter( + content_type=OuterRef('identifier__content_type'), + object_id=OuterRef('identifier__object_id') + ).order_by('-created').values('_id')[:1], + output_field=CharField(), + ) + ) + + return Subquery( + primary_artifacts_and_guids.filter( + outcome_id=OuterRef('outcome_id') + ).values('resource_guid')[:1], + output_field=CharField() + ) diff --git a/osf_tests/factories.py b/osf_tests/factories.py index e5fdd2241a3..96ff58976be 100644 --- a/osf_tests/factories.py +++ b/osf_tests/factories.py @@ -392,7 +392,7 @@ def _build(cls, target_class, *args, **kwargs): def _create(cls, target_class, project=None, is_public=False, schema=None, draft_registration=None, archive=False, embargo=None, registration_approval=None, retraction=None, - provider=None, + provider=None, has_doi=False, *args, **kwargs): user = None if project: @@ -468,6 +468,10 @@ def add_approval_step(reg): draft_registration.registered_node = reg draft_registration.save() reg.save() + + if has_doi: + IdentifierFactory(referent=reg, category='doi') + return reg diff --git a/osf_tests/test_outcomes.py b/osf_tests/test_outcomes.py index 87fc74f30d6..ab0fd5a8466 100644 --- a/osf_tests/test_outcomes.py +++ b/osf_tests/test_outcomes.py @@ -65,6 +65,7 @@ def test_outcome_for_registration__create_creates_primary_artifact( assert primary_artifact.identifier == registration_doi assert primary_artifact.pid == registration_doi.value assert primary_artifact.artifact_type == ArtifactTypes.PRIMARY + assert primary_artifact.primary_resource_guid == registration._id def test_outcome_for_registration__create_copies_metadata(self, registration, registration_doi): outcome = Outcome.objects.for_registration(registration, create=True) @@ -163,6 +164,10 @@ def test_get_artifacts_for_registration(self, outcome, registration, project_doi retrieved_project_artifact = registration_artifacts.get(identifier=project_doi) assert retrieved_project_artifact == project_artifact + assert retrieved_project_artifact.pid == TEST_PROJECT_DOI + assert retrieved_project_artifact.primary_resource_guid == registration._id retrieved_external_artifact = registration_artifacts.get(identifier=external_doi) assert retrieved_external_artifact == external_artifact + assert retrieved_external_artifact.pid == TEST_EXTERNAL_DOI + assert retrieved_external_artifact.primary_resource_guid == registration._id diff --git a/tasks/__init__.py b/tasks/__init__.py index 414f3de7a4e..ab596a0ed9b 100755 --- a/tasks/__init__.py +++ b/tasks/__init__.py @@ -354,6 +354,7 @@ def test_module(ctx, module=None, numprocesses=None, nocapture=False, params=Non 'api_tests/nodes', 'api_tests/osf_groups', 'api_tests/requests', + 'api_tests/resources', 'api_tests/schema_responses', 'api_tests/subscriptions', 'api_tests/waffle', From fd0dcc355adaaf31554cf4c4b725f5059a7af919 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Mon, 25 Jul 2022 09:26:31 -0400 Subject: [PATCH 7/8] Make Resource POST callable by WRITE user --- api/resources/permissions.py | 2 +- api/resources/serializers.py | 2 +- api/resources/views.py | 5 ++++- api_tests/resources/views/test_resource_list.py | 8 +++++--- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/api/resources/permissions.py b/api/resources/permissions.py index 45e834c8d32..140f2774435 100644 --- a/api/resources/permissions.py +++ b/api/resources/permissions.py @@ -46,7 +46,7 @@ class ResourceListPermission(ResourcesPermission, permissions.BasePermission): ResourceList only supports POST ''' - REQUIRED_PERMISSIONS = {'POST': 'admin'} + REQUIRED_PERMISSIONS = {'POST': 'write'} class ResourceDetailPermission(ResourcesPermission, permissions.BasePermission): diff --git a/api/resources/serializers.py b/api/resources/serializers.py index bdd578319db..7bcb5c60056 100644 --- a/api/resources/serializers.py +++ b/api/resources/serializers.py @@ -71,6 +71,6 @@ def create(self, validated_data): try: root_outcome = Outcome.objects.for_registration(primary_registration, create=True) except NoPIDError: - raise Conflict('Cannot add Resrouces to a Registration without a DOI') + raise Conflict('Cannot add Resources to a Registration that does not have a DOI') return OutcomeArtifact.objects.create(outcome=root_outcome) diff --git a/api/resources/views.py b/api/resources/views.py index d46486b1ca0..d7aa2e09825 100644 --- a/api/resources/views.py +++ b/api/resources/views.py @@ -40,7 +40,10 @@ def get_permissions_proxy(self): try: registration_guid = self.request.data['registration'] except KeyError: - raise JSONAPIException('Must provide "registration" relationship in payload"') + raise JSONAPIException( + detail='Must provide "registration" relationship in payload"', + source={'pointer': '/data/relationships/registration/data/id'}, + ) registration = Registration.load(registration_guid) if not registration: diff --git a/api_tests/resources/views/test_resource_list.py b/api_tests/resources/views/test_resource_list.py index 0dd906ad268..ee909aed35d 100644 --- a/api_tests/resources/views/test_resource_list.py +++ b/api_tests/resources/views/test_resource_list.py @@ -43,11 +43,13 @@ def payload(registration): @pytest.mark.django_db class TestResourceListPOSTPermissions: - def test_status_code__admin(self, app, registration, admin_user, payload): - resp = app.post_json_api(POST_URL, payload, auth=admin_user.auth, expect_errors=True) + @pytest.mark.parametrize('user_role', UserRoles.write_users()) + def test_status_code__write_user(self, app, registration, payload, user_role): + test_auth = configure_test_auth(registration, user_role) + resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) assert resp.status_code == 201 - @pytest.mark.parametrize('user_role', [role for role in UserRoles if role is not UserRoles.ADMIN_USER]) + @pytest.mark.parametrize('user_role', UserRoles.excluding(*UserRoles.write_users())) def test_status_code__non_admin(self, app, registration, payload, user_role): test_auth = configure_test_auth(registration, user_role) resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) From 49a6fbd34e9dc548b4c4b881e48f02777e53bc86 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Mon, 25 Jul 2022 09:43:12 -0400 Subject: [PATCH 8/8] Extend UserRoles functionality --- api_tests/resources/views/test_resource_list.py | 4 ++-- api_tests/utils.py | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/api_tests/resources/views/test_resource_list.py b/api_tests/resources/views/test_resource_list.py index ee909aed35d..ff6796d232b 100644 --- a/api_tests/resources/views/test_resource_list.py +++ b/api_tests/resources/views/test_resource_list.py @@ -43,13 +43,13 @@ def payload(registration): @pytest.mark.django_db class TestResourceListPOSTPermissions: - @pytest.mark.parametrize('user_role', UserRoles.write_users()) + @pytest.mark.parametrize('user_role', UserRoles.write_roles()) def test_status_code__write_user(self, app, registration, payload, user_role): test_auth = configure_test_auth(registration, user_role) resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) assert resp.status_code == 201 - @pytest.mark.parametrize('user_role', UserRoles.excluding(*UserRoles.write_users())) + @pytest.mark.parametrize('user_role', UserRoles.excluding(*UserRoles.write_roles())) def test_status_code__non_admin(self, app, registration, payload, user_role): test_auth = configure_test_auth(registration, user_role) resp = app.post_json_api(POST_URL, payload, auth=test_auth, expect_errors=True) diff --git a/api_tests/utils.py b/api_tests/utils.py index 05878b28044..db1b95cf5d2 100644 --- a/api_tests/utils.py +++ b/api_tests/utils.py @@ -86,6 +86,14 @@ def contributor_roles(cls, include_moderator=False): def noncontributor_roles(cls): return [cls.UNAUTHENTICATED, cls.NONCONTRIB, cls.MODERATOR] + @classmethod + def write_roles(cls): + return [cls.WRITE_USER, cls.ADMIN_USER] + + @classmethod + def excluding(cls, *excluded_roles): + return [role for role in cls if role not in excluded_roles] + def get_permissions_string(self): if self is UserRoles.READ_USER: return 'read'