From cd184f3eb07aa0badd756b0e8da1a3a963110161 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Fri, 20 Oct 2023 01:05:03 +0800 Subject: [PATCH] more test clean-up --- api/draft_registrations/permissions.py | 22 ++ api/draft_registrations/views.py | 7 +- api/nodes/permissions.py | 15 -- api/nodes/views.py | 7 +- .../views/test_draft_registration_detail.py | 135 ++++++----- .../views/test_draft_registration_list.py | 83 ++++--- .../test_node_draft_registration_detail.py | 222 +++++++----------- .../test_node_draft_registration_list.py | 154 ++++++++---- .../test_user_draft_registration_list.py | 4 +- 9 files changed, 337 insertions(+), 312 deletions(-) diff --git a/api/draft_registrations/permissions.py b/api/draft_registrations/permissions.py index 73d7f71fdd42..08ee9dfe492e 100644 --- a/api/draft_registrations/permissions.py +++ b/api/draft_registrations/permissions.py @@ -8,6 +8,8 @@ OSFUser, ) from api.nodes.permissions import ContributorDetailPermissions +from osf.utils.permissions import READ, WRITE, ADMIN + class IsContributorOrAdminContributor(permissions.BasePermission): """ @@ -51,9 +53,29 @@ def has_object_permission(self, request, view, obj): else: return obj.is_admin_contributor(auth.user) + class DraftContributorDetailPermissions(ContributorDetailPermissions): acceptable_models = (DraftRegistration, OSFUser, DraftRegistrationContributor,) def load_resource(self, context, view): return DraftRegistration.load(context['draft_id']) + + +class DraftRegistrationPermission(permissions.BasePermission): + """ + Check permissions for draft and node + """ + acceptable_models = (DraftRegistration, AbstractNode) + + def has_object_permission(self, request, view, obj): + auth = get_user_auth(request) + + if request.method in permissions.SAFE_METHODS: + draft_read_permissions = isinstance(obj, DraftRegistration) and \ + obj.branched_from.has_permission(auth.user, READ) + return obj.has_permission(auth.user, READ) or draft_read_permissions + else: + draft_write_permissions = isinstance(obj, DraftRegistration) and \ + obj.branched_from.has_permission(auth.user, WRITE) + return obj.has_permission(auth.user, WRITE) or draft_write_permissions diff --git a/api/draft_registrations/views.py b/api/draft_registrations/views.py index f664c45c862a..31a7378bee94 100644 --- a/api/draft_registrations/views.py +++ b/api/draft_registrations/views.py @@ -6,7 +6,7 @@ from api.base.pagination import DraftRegistrationContributorPagination from api.draft_registrations.permissions import ( DraftContributorDetailPermissions, - IsContributorOrAdminContributor, + DraftRegistrationPermission, IsAdminContributor, ) from api.draft_registrations.serializers import ( @@ -29,6 +29,7 @@ from api.subjects.views import SubjectRelationshipBaseView, BaseResourceSubjectsList from osf.models import DraftRegistrationContributor + class DraftRegistrationMixin(DraftMixin): """ Old DraftMixin was built under the assumption that a node was provided from the start. @@ -50,7 +51,7 @@ def check_resource_permissions(self, resource): class DraftRegistrationList(NodeDraftRegistrationsList): permission_classes = ( - IsContributorOrAdminContributor, + DraftRegistrationPermission, drf_permissions.IsAuthenticatedOrReadOnly, base_permissions.TokenHasScope, ) @@ -73,7 +74,7 @@ def get_queryset(self): class DraftRegistrationDetail(NodeDraftRegistrationDetail, DraftRegistrationMixin): permission_classes = ( - ContributorOrPublic, + DraftRegistrationPermission, AdminDeletePermissions, drf_permissions.IsAuthenticatedOrReadOnly, base_permissions.TokenHasScope, diff --git a/api/nodes/permissions.py b/api/nodes/permissions.py index 94195d05b8a4..45636819f72b 100644 --- a/api/nodes/permissions.py +++ b/api/nodes/permissions.py @@ -161,21 +161,6 @@ def has_object_permission(self, request, view, obj): return obj.is_admin_contributor(auth.user) -class NodeDraftRegistrationsListPermission(permissions.BasePermission): - acceptable_models = (AbstractNode, DraftRegistration,) - - def has_object_permission(self, request, view, obj): - """ - To make changes, user must be an admin contributor. Viewing information is acceptable for any READ contributor. - """ - assert_resource_type(obj, self.acceptable_models) - auth = get_user_auth(request) - if request.method in permissions.SAFE_METHODS: - return obj.has_permission(auth.user, osf_permissions.READ) - else: - return obj.is_admin_contributor(auth.user) - - class ExcludeWithdrawals(permissions.BasePermission): def has_object_permission(self, request, view, obj): diff --git a/api/nodes/views.py b/api/nodes/views.py index d6f1a2e9c7cb..8caab3ccadcd 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -64,6 +64,7 @@ NodeCommentSerializer, ) from api.draft_registrations.serializers import DraftRegistrationSerializer, DraftRegistrationDetailSerializer +from api.draft_registrations.permissions import DraftRegistrationPermission from api.files.serializers import FileSerializer, OsfStorageFileSerializer from api.files import annotations as file_annotations from api.identifiers.serializers import NodeIdentifierSerializer @@ -73,7 +74,6 @@ from api.nodes.filters import NodesFilterMixin from api.nodes.permissions import ( IsAdmin, - IsAdminContributor, IsPublic, AdminOrPublic, WriteAdmin, @@ -89,7 +89,6 @@ ExcludeWithdrawals, NodeLinksShowIfVersion, ReadOnlyIfWithdrawn, - NodeDraftRegistrationsListPermission, ) from api.nodes.serializers import ( NodeSerializer, @@ -623,7 +622,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No Use DraftRegistrationsList endpoint instead. """ permission_classes = ( - NodeDraftRegistrationsListPermission, + DraftRegistrationPermission, drf_permissions.IsAuthenticatedOrReadOnly, base_permissions.TokenHasScope, ) @@ -657,9 +656,9 @@ class NodeDraftRegistrationDetail(JSONAPIBaseView, generics.RetrieveUpdateDestro Use DraftRegistrationDetail endpoint instead. """ permission_classes = ( + DraftRegistrationPermission, drf_permissions.IsAuthenticatedOrReadOnly, base_permissions.TokenHasScope, - IsAdminContributor, ) parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON,) diff --git a/api_tests/draft_registrations/views/test_draft_registration_detail.py b/api_tests/draft_registrations/views/test_draft_registration_detail.py index d7e7626ac132..7a2bdf23c4dc 100644 --- a/api_tests/draft_registrations/views/test_draft_registration_detail.py +++ b/api_tests/draft_registrations/views/test_draft_registration_detail.py @@ -2,10 +2,10 @@ from api.base.settings.defaults import API_BASE from api_tests.nodes.views.test_node_draft_registration_detail import ( - TestDraftRegistrationDetail, TestDraftRegistrationUpdate, TestDraftRegistrationPatch, TestDraftRegistrationDelete, + AbstractDraftRegistrationTestCase ) from osf.models import DraftNode, Node, NodeLicense, RegistrationSchema from osf.utils.permissions import ADMIN, READ, WRITE @@ -16,58 +16,34 @@ SubjectFactory, ProjectFactory, ) +from website.settings import API_DOMAIN @pytest.mark.django_db -class TestDraftRegistrationDetailEndpoint(TestDraftRegistrationDetail): +class TestDraftRegistrationDetailEndpoint(AbstractDraftRegistrationTestCase): @pytest.fixture() def url_draft_registrations(self, project_public, draft_registration): - return '/{}draft_registrations/{}/'.format( - API_BASE, draft_registration._id) - - # Overrides TestDraftRegistrationDetail - def test_admin_group_member_can_view(self, app, user, draft_registration, project_public, - schema, url_draft_registrations, group_mem): - - res = app.get(url_draft_registrations, auth=group_mem.auth, expect_errors=True) - assert res.status_code == 403 + return f'/{API_BASE}draft_registrations/{draft_registration._id}/' - def test_can_view_draft( - self, app, user_write_contrib, project_public, - user_read_contrib, user_non_contrib, - url_draft_registrations, group, group_mem): - - # test_read_only_contributor_can_view_draft - res = app.get( - url_draft_registrations, - auth=user_read_contrib.auth, - expect_errors=False) + def test_read_only_contributor_can_view_draft(self, app, user_read_contrib, url_draft_registrations): + res = app.get(url_draft_registrations, auth=user_read_contrib.auth) assert res.status_code == 200 - # test_read_write_contributor_can_view_draft - res = app.get( - url_draft_registrations, - auth=user_write_contrib.auth, - expect_errors=False) + def test_read_write_contributor_can_view_draft(self, app, user_write_contrib, url_draft_registrations): + res = app.get(url_draft_registrations, auth=user_write_contrib.auth) assert res.status_code == 200 - def test_cannot_view_draft( - self, app, project_public, - user_non_contrib, url_draft_registrations): - - # test_logged_in_non_contributor_cannot_view_draft - res = app.get( - url_draft_registrations, - auth=user_non_contrib.auth, - expect_errors=True) + def test_logged_in_non_contributor_cannot_view_draft(self, app, user_non_contrib, url_draft_registrations): + res = app.get(url_draft_registrations, auth=user_non_contrib.auth, expect_errors=True) assert res.status_code == 403 - # test_unauthenticated_user_cannot_view_draft + def test_unauthenticated_user_cannot_view_draft(self, app, url_draft_registrations): res = app.get(url_draft_registrations, expect_errors=True) assert res.status_code == 401 - def test_detail_view_returns_editable_fields(self, app, user, draft_registration, - url_draft_registrations, project_public): + def test_detail_view_returns_editable_fields( + self, app, user, draft_registration, url_draft_registrations, project_public + ): res = app.get(url_draft_registrations, auth=user.auth, expect_errors=True) attributes = res.json['data']['attributes'] @@ -77,7 +53,7 @@ def test_detail_view_returns_editable_fields(self, app, user, draft_registration assert attributes['category'] == project_public.category assert attributes['has_project'] - res.json['data']['links']['self'] == url_draft_registrations + assert res.json['data']['links']['self'] == f'{API_DOMAIN}{url_draft_registrations.lstrip("/")}' relationships = res.json['data']['relationships'] assert Node.load(relationships['branched_from']['data']['id']) == draft_registration.branched_from @@ -89,8 +65,7 @@ def test_detail_view_returns_editable_fields(self, app, user, draft_registration def test_detail_view_returns_editable_fields_no_specified_node(self, app, user): draft_registration = DraftRegistrationFactory(initiator=user, branched_from=None) - url = '/{}draft_registrations/{}/'.format( - API_BASE, draft_registration._id) + url = f'{API_DOMAIN}{API_BASE}draft_registrations/{draft_registration._id}/' res = app.get(url, auth=user.auth, expect_errors=True) attributes = res.json['data']['attributes'] @@ -101,7 +76,7 @@ def test_detail_view_returns_editable_fields_no_specified_node(self, app, user): assert attributes['node_license'] is None assert not attributes['has_project'] - res.json['data']['links']['self'] == url + assert res.json['data']['links']['self'] == url relationships = res.json['data']['relationships'] assert 'affiliated_institutions' in relationships @@ -112,24 +87,21 @@ def test_detail_view_returns_editable_fields_no_specified_node(self, app, user): res = app.get(draft_node_link, auth=user.auth) assert DraftNode.load(res.json['data']['id']) == draft_registration.branched_from - def test_draft_registration_perms_checked_on_draft_not_node(self, app, user, project_public, - draft_registration, url_draft_registrations): - - # Admin on node and draft + def test_admin_node_and_draft(self, app, user, project_public, draft_registration, url_draft_registrations): assert project_public.has_permission(user, ADMIN) is True assert draft_registration.has_permission(user, ADMIN) is True res = app.get(url_draft_registrations, auth=user.auth) assert res.status_code == 200 - # Admin on node but not draft + def test_admin_node_not_draft(self, app, user, project_public, draft_registration, url_draft_registrations): node_admin = AuthUserFactory() project_public.add_contributor(node_admin, ADMIN) assert project_public.has_permission(node_admin, ADMIN) is True assert draft_registration.has_permission(node_admin, ADMIN) is False - res = app.get(url_draft_registrations, auth=node_admin.auth, expect_errors=True) - assert res.status_code == 403 + res = app.get(url_draft_registrations, auth=node_admin.auth) + assert res.status_code == 200 - # Admin on draft but not node + def test_admin_draft_not_node(self, app, user, project_public, draft_registration, url_draft_registrations): draft_admin = AuthUserFactory() draft_registration.add_contributor(draft_admin, ADMIN) assert project_public.has_permission(draft_admin, ADMIN) is False @@ -137,19 +109,63 @@ def test_draft_registration_perms_checked_on_draft_not_node(self, app, user, pro res = app.get(url_draft_registrations, auth=draft_admin.auth) assert res.status_code == 200 - # Overwrites TestDraftRegistrationDetail - def test_can_view_after_added( - self, app, schema, draft_registration, url_draft_registrations): - # Draft Registration permissions are no longer based on the branched from project + def test_write_node_and_draft(self, app, user, project_public, draft_registration, url_draft_registrations): + assert project_public.has_permission(user, WRITE) is True + assert draft_registration.has_permission(user, WRITE) is True + res = app.get(url_draft_registrations, auth=user.auth) + assert res.status_code == 200 + + def test_write_node_not_draft(self, app, user, project_public, draft_registration, url_draft_registrations): + node_admin = AuthUserFactory() + project_public.add_contributor(node_admin, WRITE) + assert project_public.has_permission(node_admin, WRITE) is True + assert draft_registration.has_permission(node_admin, WRITE) is False + res = app.get(url_draft_registrations, auth=node_admin.auth) + assert res.status_code == 200 + + def test_write_draft_not_node(self, app, user, project_public, draft_registration, url_draft_registrations): + draft_admin = AuthUserFactory() + draft_registration.add_contributor(draft_admin, WRITE) + assert project_public.has_permission(draft_admin, WRITE) is False + assert draft_registration.has_permission(draft_admin, WRITE) is True + res = app.get(url_draft_registrations, auth=draft_admin.auth) + assert res.status_code == 200 + + def test_read_node_and_draft(self, app, user, project_public, draft_registration, url_draft_registrations): + assert project_public.has_permission(user, READ) is True + assert draft_registration.has_permission(user, READ) is True + res = app.get(url_draft_registrations, auth=user.auth) + assert res.status_code == 200 + def test_read_node_not_draft(self, app, user, project_public, draft_registration, url_draft_registrations): + node_admin = AuthUserFactory() + project_public.add_contributor(node_admin, READ) + assert project_public.has_permission(node_admin, READ) is True + assert draft_registration.has_permission(node_admin, READ) is False + res = app.get(url_draft_registrations, auth=node_admin.auth) + assert res.status_code == 200 + + def test_read_draft_not_node(self, app, user, project_public, draft_registration, url_draft_registrations): + draft_admin = AuthUserFactory() + draft_registration.add_contributor(draft_admin, READ) + assert project_public.has_permission(draft_admin, READ) is False + assert draft_registration.has_permission(draft_admin, READ) is True + res = app.get(url_draft_registrations, auth=draft_admin.auth) + assert res.status_code == 200 + + def test_can_view_after_added(self, app, schema, draft_registration, url_draft_registrations): + """ + Ensure Draft Registration permissions are no longer based on the branched from project + """ user = AuthUserFactory() project = draft_registration.branched_from project.add_contributor(user, ADMIN) res = app.get(url_draft_registrations, auth=user.auth, expect_errors=True) - assert res.status_code == 403 + assert res.status_code == 200 - def test_current_permissions_field(self, app, user_read_contrib, - user_write_contrib, user, draft_registration, url_draft_registrations): + def test_current_permissions_field( + self, app, user_read_contrib, user_write_contrib, user, draft_registration, url_draft_registrations + ): res = app.get(url_draft_registrations, auth=user_read_contrib.auth, expect_errors=False) assert res.json['data']['attributes']['current_user_permissions'] == [READ] @@ -548,9 +564,8 @@ def test_write_contributor_can_update_draft( assert data['attributes']['registration_metadata'] == payload['data']['attributes']['registration_metadata'] -class TestDraftRegistrationDelete(TestDraftRegistrationDelete): +class TestDraftRegistrationDeleteDetail(TestDraftRegistrationDelete): @pytest.fixture() def url_draft_registrations(self, project_public, draft_registration): # Overrides TestDraftRegistrationDelete - return '/{}draft_registrations/{}/'.format( - API_BASE, draft_registration._id) + return f'/{API_BASE}draft_registrations/{draft_registration._id}/' diff --git a/api_tests/draft_registrations/views/test_draft_registration_list.py b/api_tests/draft_registrations/views/test_draft_registration_list.py index 21691600faa7..5a0fde8a01a0 100644 --- a/api_tests/draft_registrations/views/test_draft_registration_list.py +++ b/api_tests/draft_registrations/views/test_draft_registration_list.py @@ -3,7 +3,7 @@ from framework.auth.core import Auth from django.utils import timezone -from api_tests.nodes.views.test_node_draft_registration_list import TestDraftRegistrationCreate +from api_tests.nodes.views.test_node_draft_registration_list import AbstractDraftRegistrationTestCase from api.base.settings.defaults import API_BASE from osf.migrations import ensure_invisible_and_inactive_schema @@ -37,7 +37,7 @@ def invisible_and_inactive_schema(): class TestDraftRegistrationListTopLevelEndpoint: @pytest.fixture() - def url_draft_registrations(self, project): + def url_draft_registrations(self): return f'/{API_BASE}draft_registrations/' @pytest.fixture() @@ -101,17 +101,6 @@ def test_read_write_contributor_can_view_draft_list( assert res.status_code == 200 assert len(res.json['data']) == 1 - def test_logged_in_non_contributor_has_empty_list( - self, app, user_non_contrib, url_draft_registrations - ): - res = app.get(url_draft_registrations, auth=user_non_contrib.auth) - assert res.status_code == 200 - assert len(res.json['data']) == 0 - - def test_unauthenticated_user_cannot_view_draft_list(self, app, url_draft_registrations): - res = app.get(url_draft_registrations, expect_errors=True) - assert res.status_code == 401 - def test_admin_can_view_draft_list(self, app, user_admin_contrib, draft_registration, schema, url_draft_registrations): res = app.get(url_draft_registrations, auth=user_admin_contrib.auth) @@ -123,6 +112,17 @@ def test_admin_can_view_draft_list(self, app, user_admin_contrib, draft_registra assert data[0]['id'] == draft_registration._id assert data[0]['attributes']['registration_metadata'] == {} + def test_logged_in_non_contributor_has_empty_list( + self, app, user_non_contrib, url_draft_registrations + ): + res = app.get(url_draft_registrations, auth=user_non_contrib.auth) + assert res.status_code == 200 + assert len(res.json['data']) == 0 + + def test_unauthenticated_user_cannot_view_draft_list(self, app, url_draft_registrations): + res = app.get(url_draft_registrations, expect_errors=True) + assert res.status_code == 401 + def test_logged_in_non_contributor_cannot_view_draft_list(self, app, user_non_contrib, url_draft_registrations): res = app.get(url_draft_registrations, auth=user_non_contrib.auth) assert res.status_code == 200 @@ -164,11 +164,10 @@ def test_draft_with_deleted_registered_node_shows_up_in_draft_list( assert data[0]['attributes']['registration_metadata'] == {} -class TestDraftRegistrationCreateWithNode(TestDraftRegistrationCreate): +class TestDraftRegistrationCreateWithNode(AbstractDraftRegistrationTestCase): - # Overrides `url_draft_registrations` in `TestDraftRegistrationCreate` @pytest.fixture() - def url_draft_registrations(self, project_public): + def url_draft_registrations(self): return f'/{API_BASE}draft_registrations/' # Overrides `payload` in TestDraftRegistrationCreate` @@ -217,6 +216,7 @@ def payload_alt(self, payload, provider_alt): new_payload = payload.copy() new_payload['data']['relationships']['provider']['data']['id'] = provider_alt._id return new_payload + def test_cannot_create_draft_from_a_registration(self, app, user, payload_alt, project_public, url_draft_registrations): registration = RegistrationFactory( project=project_public, @@ -340,6 +340,7 @@ def test_logged_in_non_contributor_cannot_create_draft( ) assert res.status_code == 403 + @pytest.mark.skip('Old OSF Groups code') def test_group_admin_cannot_create_draft( self, app, user_write_contrib, payload_alt, url_draft_registrations, group_mem ): @@ -352,6 +353,7 @@ def test_group_admin_cannot_create_draft( ) assert res.status_code == 201 + @pytest.mark.skip('Old OSF Groups code') def test_group_write_contrib_cannot_create_draft( self, app, user_read_contrib, project_public, payload_alt, group, url_draft_registrations, group_mem ): @@ -366,11 +368,9 @@ def test_group_write_contrib_cannot_create_draft( ) assert res.status_code == 201 - def test_create_project_based_draft_does_not_email_initiator( - self, app, user, url_draft_registrations, payload): - post_url = url_draft_registrations + 'embed=branched_from&embed=initiator' + def test_create_project_based_draft_does_not_email_initiator(self, app, user, url_draft_registrations, payload): with mock.patch.object(mails, 'send_mail') as mock_send_mail: - app.post_json_api(post_url, payload, auth=user.auth) + app.post_json_api(f'{url_draft_registrations}?embed=branched_from&embed=initiator', payload, auth=user.auth) assert not mock_send_mail.called @@ -397,17 +397,17 @@ def test_affiliated_institutions_are_copied_from_user( assert not draft_registration.affiliated_institutions.all() == user.get_affiliated_institutions() -class TestDraftRegistrationCreateWithoutNode(TestDraftRegistrationCreate): +class TestDraftRegistrationCreateWithoutNode(AbstractDraftRegistrationTestCase): @pytest.fixture() def url_draft_registrations(self): - return '/{}draft_registrations/?'.format(API_BASE) + return f'/{API_BASE}draft_registrations/' - # Overrides TestDraftRegistrationList - def test_admin_can_create_draft( - self, app, user, url_draft_registrations, - payload, metaschema_open_ended): - url = '{}embed=branched_from&embed=initiator'.format(url_draft_registrations) - res = app.post_json_api(url, payload, auth=user.auth) + def test_admin_can_create_draft(self, app, user, url_draft_registrations, payload, metaschema_open_ended): + res = app.post_json_api( + f'{url_draft_registrations}?embed=branched_from&embed=initiator', + payload, + auth=user.auth + ) assert res.status_code == 201 data = res.json['data'] @@ -423,13 +423,14 @@ def test_admin_can_create_draft( assert draft.creator == user assert draft.has_permission(user, ADMIN) is True - def test_create_no_project_draft_emails_initiator( - self, app, user, url_draft_registrations, payload): - post_url = url_draft_registrations + 'embed=branched_from&embed=initiator' - + def test_create_no_project_draft_emails_initiator(self, app, user, url_draft_registrations, payload): # Intercepting the send_mail call from website.project.views.contributor.notify_added_contributor with mock.patch.object(mails, 'send_mail') as mock_send_mail: - resp = app.post_json_api(post_url, payload, auth=user.auth) + resp = app.post_json_api( + f'{url_draft_registrations}?embed=branched_from&embed=initiator', + payload, + auth=user.auth + ) assert mock_send_mail.called # Python 3.6 does not support mock.call_args.args/kwargs @@ -440,7 +441,9 @@ def test_create_no_project_draft_emails_initiator( assert mock_send_kwargs['user'] == user assert mock_send_kwargs['node'] == DraftRegistration.load(resp.json['data']['id']) - def test_create_draft_with_provider(self, app, user, url_draft_registrations, non_default_provider, payload_with_non_default_provider): + def test_create_draft_with_provider( + self, app, user, url_draft_registrations, non_default_provider, payload_with_non_default_provider + ): res = app.post_json_api(url_draft_registrations, payload_with_non_default_provider, auth=user.auth) assert res.status_code == 201 data = res.json['data'] @@ -453,7 +456,6 @@ def test_create_draft_with_provider(self, app, user, url_draft_registrations, no def test_write_contrib(self, app, user, project_public, payload, url_draft_registrations, user_write_contrib): """(no node supplied, so any logged in user can create) """ - assert user_write_contrib in project_public.contributors.all() res = app.post_json_api( url_draft_registrations, @@ -462,7 +464,8 @@ def test_write_contrib(self, app, user, project_public, payload, url_draft_regis assert res.status_code == 201 def test_read_only(self, app, user, url_draft_registrations, user_read_contrib, project_public, payload): - '''(no node supplied, so any logged in user can create) ''' + '''(no node supplied, so any logged in user can create) + ''' assert user_read_contrib in project_public.contributors.all() res = app.post_json_api( url_draft_registrations, @@ -479,8 +482,7 @@ def test_non_authenticated_user_cannot_create_draft(self, app, user, url_draft_r assert res.status_code == 401 def test_logged_in_non_contributor(self, app, user, url_draft_registrations, user_non_contrib, payload): - ''' - (no node supplied, so any logged in user can create) + '''(no node supplied, so any logged in user can create) ''' res = app.post_json_api( url_draft_registrations, @@ -489,11 +491,6 @@ def test_logged_in_non_contributor(self, app, user, url_draft_registrations, use ) assert res.status_code == 201 - # Overrides TestDraftRegistrationList - def test_cannot_create_draft_errors(self): - # The original test assumes a node is being passed in - return - def test_draft_registration_attributes_not_copied_from_node(self, app, project_public, url_draft_registrations, user, payload): diff --git a/api_tests/nodes/views/test_node_draft_registration_detail.py b/api_tests/nodes/views/test_node_draft_registration_detail.py index 068c325eacad..1f02177c1940 100644 --- a/api_tests/nodes/views/test_node_draft_registration_detail.py +++ b/api_tests/nodes/views/test_node_draft_registration_detail.py @@ -11,6 +11,7 @@ ) from osf.utils.permissions import WRITE, READ, ADMIN from api_tests.nodes.views.test_node_draft_registration_list import AbstractDraftRegistrationTestCase +from framework.auth.core import Auth SCHEMA_VERSION = 2 @@ -22,26 +23,7 @@ class TestDraftRegistrationDetail(AbstractDraftRegistrationTestCase): def url_draft_registrations(self, project_public, draft_registration): return f'/{API_BASE}nodes/{project_public._id}/draft_registrations/{draft_registration._id}/?version=2.19' - @pytest.fixture() - def schema(self): - return RegistrationSchema.objects.get( - name='OSF-Standard Pre-Data Collection Registration', - schema_version=SCHEMA_VERSION - ) - - @pytest.fixture() - def draft_registration(self, user, project_public, schema): - return DraftRegistrationFactory( - initiator=user, - registration_schema=schema, - branched_from=project_public - ) - - @pytest.fixture() - def project_other(self, user): - return ProjectFactory(creator=user) - - def test_admin_can_view_draft(self, app, user, draft_registration, schema, url_draft_registrations): + def test_node_admin_can_view_draft(self, app, user, draft_registration, schema, url_draft_registrations): res = app.get(url_draft_registrations, auth=user.auth) assert res.status_code == 200 data = res.json['data'] @@ -49,18 +31,24 @@ def test_admin_can_view_draft(self, app, user, draft_registration, schema, url_d assert data['id'] == draft_registration._id assert data['attributes']['registration_metadata'] == {} + @pytest.mark.skip('Old OSF Groups code') def test_admin_group_member_can_view(self, app, user, url_draft_registrations, group_mem): - res = app.get(url_draft_registrations, auth=group_mem.auth) assert res.status_code == 200 - def test_read_only_contributor_cannot_view_draft(self, app, user_read_contrib, url_draft_registrations): - res = app.get(url_draft_registrations, auth=user_read_contrib.auth, expect_errors=True) - assert res.status_code == 403 + def test_read_contributor_can_view_draft(self, app, user_read_contrib, url_draft_registrations): + """ + Note this is the Node permissions not DraftRegistration permission + """ + res = app.get(url_draft_registrations, auth=user_read_contrib.auth) + assert res.status_code == 200 - def test_read_write_contributor_cannot_view_draft(self, app, user_write_contrib, url_draft_registrations): - res = app.get(url_draft_registrations, auth=user_write_contrib.auth, expect_errors=True) - assert res.status_code == 403 + def test_write_contributor_can_view_draft(self, app, user_write_contrib, url_draft_registrations): + """ + Note this is the Node permissions not DraftRegistration permission + """ + res = app.get(url_draft_registrations, auth=user_write_contrib.auth) + assert res.status_code == 200 def test_logged_in_non_contributor_cannot_view_draft(self, app, user_non_contrib, url_draft_registrations): res = app.get(url_draft_registrations, auth=user_non_contrib.auth, expect_errors=True) @@ -70,13 +58,14 @@ def test_unauthenticated_user_cannot_view_draft(self, app, url_draft_registratio res = app.get(url_draft_registrations, expect_errors=True) assert res.status_code == 401 + @pytest.mark.skip('Old OSF Groups code') def test_group_mem_read_cannot_view(self, app, group, group_mem, project_public, url_draft_registrations): project_public.remove_osf_group(group) project_public.add_osf_group(group, READ) res = app.get(url_draft_registrations, auth=group_mem.auth, expect_errors=True) assert res.status_code == 403 - def test_cannot_view_deleted_draft( self, app, user, url_draft_registrations): + def test_cannot_view_deleted_draft(self, app, user, url_draft_registrations): res = app.delete_json_api(url_draft_registrations, auth=user.auth) assert res.status_code == 204 @@ -123,18 +112,8 @@ def test_can_view_after_added(self, app, schema, draft_registration, url_draft_r class TestDraftRegistrationUpdate(AbstractDraftRegistrationTestCase): @pytest.fixture() - def schema(self): - return RegistrationSchema.objects.get( - name='OSF-Standard Pre-Data Collection Registration', - schema_version=SCHEMA_VERSION) - - @pytest.fixture() - def draft_registration(self, user, project_public, schema): - return DraftRegistrationFactory( - initiator=user, - registration_schema=schema, - branched_from=project_public - ) + def url_draft_registrations(self, project_public, draft_registration): + return f'/{API_BASE}nodes/{project_public._id}/draft_registrations/{draft_registration._id}/?version=2.19' @pytest.fixture() def reg_schema(self): @@ -152,20 +131,13 @@ def draft_registration_prereg(self, user, project_public, reg_schema): ) @pytest.fixture() - def metadata_registration( - self, metadata, - draft_registration_prereg): + def metadata_registration(self, metadata, draft_registration_prereg): return metadata(draft_registration_prereg) @pytest.fixture() def project_other(self, user): return ProjectFactory(creator=user) - @pytest.fixture() - def url_draft_registrations(self, project_public, draft_registration): - return '/{}nodes/{}/draft_registrations/{}/?{}'.format( - API_BASE, project_public._id, draft_registration._id, 'version=2.19') - @pytest.fixture() def payload(self, draft_registration): return { @@ -249,12 +221,10 @@ def test_draft_must_be_branched_from_node( errors = res.json['errors'][0] assert errors['detail'] == 'This draft registration is not created from the given node.' - def test_cannot_update_draft( - self, app, user_write_contrib, project_public, - user_read_contrib, user_non_contrib, - payload, url_draft_registrations, group, group_mem): + def test_read_only_contributor_cannot_update_draft( + self, app, user_read_contrib, payload, url_draft_registrations, + ): - # test_read_only_contributor_cannot_update_draft res = app.put_json_api( url_draft_registrations, payload, @@ -262,34 +232,38 @@ def test_cannot_update_draft( expect_errors=True) assert res.status_code == 403 - # test_logged_in_non_contributor_cannot_update_draft + def test_logged_in_non_contributor_cannot_update_draft( + self, app, user_non_contrib, payload, url_draft_registrations, + ): res = app.put_json_api( url_draft_registrations, payload, auth=user_non_contrib.auth, - expect_errors=True) + expect_errors=True + ) assert res.status_code == 403 - # test_unauthenticated_user_cannot_update_draft - res = app.put_json_api( - url_draft_registrations, - payload, expect_errors=True) + def test_unauthenticated_user_cannot_update_draft(self, app, payload, url_draft_registrations): + res = app.put_json_api(url_draft_registrations, payload, expect_errors=True) assert res.status_code == 401 - # test_osf_group_member_admin_cannot_update_draft - res = app.put_json_api( - url_draft_registrations, - payload, expect_errors=True, - auth=group_mem.auth - ) + @pytest.mark.skip('Old OSF Groups code') + def test_osf_group_member_admin_cannot_update_draft( + self, app, project_public, payload, url_draft_registrations, group_mem + ): + res = app.put_json_api(url_draft_registrations, payload, expect_errors=True, auth=group_mem.auth) assert res.status_code == 403 - # test_osf_group_member_write_cannot_update_draft + @pytest.mark.skip('Old OSF Groups code') + def test_osf_group_member_write_cannot_update_draft( + self, app, project_public, payload, url_draft_registrations, group, group_mem + ): project_public.remove_osf_group(group) project_public.add_osf_group(group, WRITE) res = app.put_json_api( url_draft_registrations, - payload, expect_errors=True, + payload, + expect_errors=True, auth=group_mem.auth ) assert res.status_code == 403 @@ -512,20 +486,6 @@ def test_multiple_choice_question_value_in_registration_responses_must_match_val @pytest.mark.django_db class TestDraftRegistrationPatch(AbstractDraftRegistrationTestCase): - @pytest.fixture() - def schema(self): - return RegistrationSchema.objects.get( - name='OSF-Standard Pre-Data Collection Registration', - schema_version=SCHEMA_VERSION) - - @pytest.fixture() - def draft_registration(self, user, project_public, schema): - return DraftRegistrationFactory( - initiator=user, - registration_schema=schema, - branched_from=project_public - ) - @pytest.fixture() def reg_schema(self): return RegistrationSchema.objects.get( @@ -544,10 +504,6 @@ def draft_registration_prereg(self, user, project_public, reg_schema): def metadata_registration(self, metadata, draft_registration_prereg): return metadata(draft_registration_prereg) - @pytest.fixture() - def project_other(self, user): - return ProjectFactory(creator=user) - @pytest.fixture() def url_draft_registrations(self, project_public, draft_registration): return '/{}nodes/{}/draft_registrations/{}/?{}'.format( @@ -586,106 +542,90 @@ def test_admin_can_update_draft( assert schema._id in data['relationships']['registration_schema']['links']['related']['href'] assert data['attributes']['registration_metadata'] == payload['data']['attributes']['registration_metadata'] - def test_cannot_update_draft( - self, app, user_write_contrib, - user_read_contrib, user_non_contrib, - payload, url_draft_registrations, group_mem): - - # test_read_only_contributor_cannot_update_draft + def test_read_only_contributor_cannot_update_draft( + self, app, user_read_contrib, payload, url_draft_registrations + ): res = app.patch_json_api( url_draft_registrations, payload, auth=user_read_contrib.auth, - expect_errors=True) + expect_errors=True + ) assert res.status_code == 403 - # test_logged_in_non_contributor_cannot_update_draft + def test_logged_in_non_contributor_cannot_update_draft( + self, app, user_non_contrib, payload, url_draft_registrations + ): res = app.patch_json_api( url_draft_registrations, payload, auth=user_non_contrib.auth, - expect_errors=True) + expect_errors=True + ) assert res.status_code == 403 - # test_unauthenticated_user_cannot_update_draft + def test_unauthenticated_user_cannot_update_draft( + self, app, user_non_contrib, payload, url_draft_registrations + ): res = app.patch_json_api( url_draft_registrations, - payload, expect_errors=True) + payload, + expect_errors=True + ) assert res.status_code == 401 - # group admin cannot update draft + @pytest.mark.skip('Old OSF Groups code') + def test_group_admin_cannot_update_draft( + self, app, user_non_contrib, payload, url_draft_registrations, group_mem + ): res = app.patch_json_api( url_draft_registrations, payload, auth=group_mem.auth, - expect_errors=True) + expect_errors=True + ) assert res.status_code == 403 + @pytest.mark.django_db class TestDraftRegistrationDelete(AbstractDraftRegistrationTestCase): - @pytest.fixture() - def schema(self): - return RegistrationSchema.objects.get( - name='OSF-Standard Pre-Data Collection Registration', - schema_version=SCHEMA_VERSION) - - @pytest.fixture() - def draft_registration(self, user, project_public, schema): - return DraftRegistrationFactory( - initiator=user, - registration_schema=schema, - branched_from=project_public - ) - - @pytest.fixture() - def project_other(self, user): - return ProjectFactory(creator=user) - @pytest.fixture() def url_draft_registrations(self, project_public, draft_registration): - return '/{}nodes/{}/draft_registrations/{}/?{}'.format( - API_BASE, project_public._id, draft_registration._id, 'version=2.19') + return f'/{API_BASE}nodes/{project_public._id}/draft_registrations/{draft_registration._id}/?version=2.19' def test_admin_can_delete_draft(self, app, user, url_draft_registrations, project_public): res = app.delete_json_api(url_draft_registrations, auth=user.auth) assert res.status_code == 204 - def test_cannot_delete_draft( - self, app, user_write_contrib, project_public, - user_read_contrib, user_non_contrib, - url_draft_registrations, group, group_mem): - - # test_read_only_contributor_cannot_delete_draft - res = app.delete_json_api( - url_draft_registrations, - auth=user_read_contrib.auth, - expect_errors=True) + def test_read_only_contributor_cannot_delete_draft(self, app, user_read_contrib, url_draft_registrations): + res = app.delete_json_api(url_draft_registrations, auth=user_read_contrib.auth, expect_errors=True) assert res.status_code == 403 - # test_read_write_contributor_cannot_delete_draft - res = app.delete_json_api( - url_draft_registrations, - auth=user_write_contrib.auth, - expect_errors=True) + def test_read_write_draft_contributor_cannot_delete_draft( + self, app, user_write_contrib, url_draft_registrations, project_public + ): + project_public.remove_contributor(user_write_contrib, Auth(user_write_contrib)) # Draft contributor only + res = app.delete_json_api(url_draft_registrations, auth=user_write_contrib.auth, expect_errors=True) assert res.status_code == 403 - # test_logged_in_non_contributor_cannot_delete_draft - res = app.delete_json_api( - url_draft_registrations, - auth=user_non_contrib.auth, - expect_errors=True) + def test_logged_in_non_contributor_cannot_delete_draft(self, app, user_non_contrib, url_draft_registrations): + res = app.delete_json_api(url_draft_registrations, auth=user_non_contrib.auth, expect_errors=True) assert res.status_code == 403 - # test_unauthenticated_user_cannot_delete_draft + def test_unauthenticated_user_cannot_delete_draft(self, app, url_draft_registrations): res = app.delete_json_api(url_draft_registrations, expect_errors=True) assert res.status_code == 401 - # test_group_member_admin_cannot_delete_draft + @pytest.mark.skip('Old OSF Groups code') + def test_group_member_admin_cannot_delete_draft(self, app, url_draft_registrations, group, group_mem): res = app.delete_json_api(url_draft_registrations, expect_errors=True, auth=group_mem.auth) assert res.status_code == 403 - # test_group_member_write_cannot_delete_draft + @pytest.mark.skip('Old OSF Groups code') + def test_group_member_write_cannot_delete_draft( + self, app, url_draft_registrations, group, group_mem, project_public + ): project_public.remove_osf_group(group) project_public.add_osf_group(group, WRITE) res = app.delete_json_api(url_draft_registrations, expect_errors=True, auth=group_mem.auth) diff --git a/api_tests/nodes/views/test_node_draft_registration_list.py b/api_tests/nodes/views/test_node_draft_registration_list.py index 28544d7603ef..45528bf134df 100644 --- a/api_tests/nodes/views/test_node_draft_registration_list.py +++ b/api_tests/nodes/views/test_node_draft_registration_list.py @@ -34,6 +34,10 @@ class AbstractDraftRegistrationTestCase: def user(self): return AuthUserFactory() + @pytest.fixture() + def user_admin_contrib(self, user): + return AuthUserFactory() + @pytest.fixture() def user_write_contrib(self): return AuthUserFactory() @@ -55,7 +59,7 @@ def group(self, group_mem): return OSFGroupFactory(creator=group_mem) @pytest.fixture() - def project_public(self, user, user_write_contrib, user_read_contrib, group, group_mem): + def project_public(self, user, user_admin_contrib, user_write_contrib, user_read_contrib, group, group_mem): project_public = ProjectFactory(is_public=True, creator=user) project_public.add_contributor( user_write_contrib, @@ -63,11 +67,22 @@ def project_public(self, user, user_write_contrib, user_read_contrib, group, gro project_public.add_contributor( user_read_contrib, permissions=permissions.READ) + project_public.add_contributor( + user_admin_contrib, + permissions=permissions.ADMIN) project_public.save() project_public.add_osf_group(group, permissions.ADMIN) project_public.add_tag('hello', Auth(user), save=True) return project_public + @pytest.fixture() + def draft_registration(self, user, project_public, schema): + return DraftRegistrationFactory( + initiator=user, + registration_schema=schema, + branched_from=project_public + ) + @pytest.fixture() def metadata(self): def metadata(draft): @@ -90,6 +105,81 @@ def metadata(draft): return test_metadata return metadata + @pytest.fixture() + def schema(self): + return RegistrationSchema.objects.get( + name='OSF-Standard Pre-Data Collection Registration', + schema_version=SCHEMA_VERSION + ) + + @pytest.fixture() + def metaschema_open_ended(self): + return RegistrationSchema.objects.get( + name='Open-Ended Registration', + schema_version=OPEN_ENDED_SCHEMA_VERSION + ) + + @pytest.fixture() + def project_other(self, user): + return ProjectFactory(creator=user) + + @pytest.fixture() + def payload(self, metaschema_open_ended, provider): + return { + 'data': { + 'type': 'draft_registrations', + 'attributes': {}, + 'relationships': { + 'registration_schema': { + 'data': { + 'type': 'registration_schema', + 'id': metaschema_open_ended._id + } + }, + 'provider': { + 'data': { + 'type': 'registration-providers', + 'id': provider._id, + } + } + } + } + } + + @pytest.fixture() + def provider(self): + return RegistrationProvider.get_default() + + @pytest.fixture() + def non_default_provider(self, metaschema_open_ended): + non_default_provider = RegistrationProviderFactory() + non_default_provider.schemas.add(metaschema_open_ended) + non_default_provider.save() + return non_default_provider + + @pytest.fixture() + def payload_with_non_default_provider(self, metaschema_open_ended, non_default_provider): + return { + 'data': { + 'type': 'draft_registrations', + 'attributes': {}, + 'relationships': { + 'registration_schema': { + 'data': { + 'type': 'registration_schema', + 'id': metaschema_open_ended._id + } + }, + 'provider': { + 'data': { + 'type': 'registration-providers', + 'id': non_default_provider._id, + } + } + } + } + } + @pytest.mark.django_db class TestDraftRegistrationList(AbstractDraftRegistrationTestCase): @@ -114,7 +204,7 @@ def draft_registration(self, user, project_public, schema): branched_from=project_public ) - def test_admin_can_view_draft_list( + def test_draft_admin_can_view_draft_list( self, app, user, draft_registration, project_public, schema, url_draft_registrations ): res = app.get(url_draft_registrations, auth=user.auth) @@ -126,6 +216,22 @@ def test_admin_can_view_draft_list( assert data[0]['id'] == draft_registration._id assert data[0]['attributes']['registration_metadata'] == {} + def test_node_admin_can_view_draft_list( + self, app, user, draft_registration, project_public, user_admin_contrib, schema, url_draft_registrations + ): + """ + Project Admins should be implicitly granted READ permissions on Drafts even if not explicitly a contributor. + """ + draft_registration.remove_contributor(user_admin_contrib, Auth(user_admin_contrib), _force=True) + assert user_admin_contrib not in draft_registration.contributors.all() + assert project_public.has_permission(user_admin_contrib, permissions.ADMIN) + + res = app.get(url_draft_registrations, auth=user_admin_contrib.auth) + assert res.status_code == 200 + data = res.json['data'] + assert len(data) == 1 + + @pytest.mark.skip('Old OSF Groups code') def test_osf_group_with_admin_permissions_can_view( self, app, user, draft_registration, project_public, schema, url_draft_registrations ): @@ -224,23 +330,6 @@ class TestDraftRegistrationCreate(AbstractDraftRegistrationTestCase): def url_draft_registrations(self, project_public): return f'/{API_BASE}nodes/{project_public._id}/draft_registrations/?version=2.19' - @pytest.fixture() - def provider(self): - return RegistrationProvider.get_default() - - @pytest.fixture() - def non_default_provider(self, metaschema_open_ended): - non_default_provider = RegistrationProviderFactory() - non_default_provider.schemas.add(metaschema_open_ended) - non_default_provider.save() - return non_default_provider - - @pytest.fixture() - def metaschema_open_ended(self): - return RegistrationSchema.objects.get( - name='Open-Ended Registration', - schema_version=OPEN_ENDED_SCHEMA_VERSION) - @pytest.fixture() def payload(self, metaschema_open_ended, provider): return { @@ -264,32 +353,7 @@ def payload(self, metaschema_open_ended, provider): } } - @pytest.fixture() - def payload_with_non_default_provider(self, metaschema_open_ended, non_default_provider): - return { - 'data': { - 'type': 'draft_registrations', - 'attributes': {}, - 'relationships': { - 'registration_schema': { - 'data': { - 'type': 'registration_schema', - 'id': metaschema_open_ended._id - } - }, - 'provider': { - 'data': { - 'type': 'registration-providers', - 'id': non_default_provider._id, - } - } - } - } - } - - def test_type_is_draft_registrations( - self, app, user, metaschema_open_ended, - url_draft_registrations): + def test_type_is_draft_registrations(self, app, user, metaschema_open_ended, url_draft_registrations): draft_data = { 'data': { 'type': 'nodes', diff --git a/api_tests/users/views/test_user_draft_registration_list.py b/api_tests/users/views/test_user_draft_registration_list.py index b2412e98620d..a002ffe9a7ee 100644 --- a/api_tests/users/views/test_user_draft_registration_list.py +++ b/api_tests/users/views/test_user_draft_registration_list.py @@ -58,7 +58,9 @@ def test_non_contrib_view_permissions( assert data[0]['id'] == draft_registration._id assert data[0]['attributes']['registration_metadata'] == {} - def test_read_only_contributor_can_view_draft_list(self, app, user_read_contrib, url_draft_registrations): + def test_read_only_contributor_can_view_draft_list( + self, app, draft_registration, user_read_contrib, url_draft_registrations + ): res = app.get( url_draft_registrations, auth=user_read_contrib.auth