From efc5dd200aa2b380a8a8917d81b5a9a50582d40d Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Mon, 16 Dec 2024 11:34:27 -0500 Subject: [PATCH] split off node request file again, added descriptive message when node request access is turned off and added test case --- api/requests/permissions.py | 2 +- .../test_node_request_institutional_access.py | 300 ++++++++++++++++++ .../requests/views/test_node_request_list.py | 272 +--------------- 3 files changed, 302 insertions(+), 272 deletions(-) create mode 100644 api_tests/requests/views/test_node_request_institutional_access.py diff --git a/api/requests/permissions.py b/api/requests/permissions.py index 469c7ffa150..cb08a6a94e5 100644 --- a/api/requests/permissions.py +++ b/api/requests/permissions.py @@ -36,7 +36,7 @@ def has_object_permission(self, request, view, obj): raise ValueError(f'Not a request-related model: {obj}') if not node.access_requests_enabled: - return False + raise exceptions.PermissionDenied(f'{node._id} does not have Access Requests enabled') is_requester = target is not None and target.creator == auth.user or trigger == DefaultTriggers.SUBMIT.value is_node_admin = node.has_permission(auth.user, osf_permissions.ADMIN) diff --git a/api_tests/requests/views/test_node_request_institutional_access.py b/api_tests/requests/views/test_node_request_institutional_access.py new file mode 100644 index 00000000000..e989ccbc469 --- /dev/null +++ b/api_tests/requests/views/test_node_request_institutional_access.py @@ -0,0 +1,300 @@ +from unittest import mock +import pytest + +from api.base.settings.defaults import API_BASE +from api_tests.requests.mixins import NodeRequestTestMixin + +from osf_tests.factories import NodeFactory, InstitutionFactory, AuthUserFactory +from osf.utils.workflows import DefaultStates, NodeRequestTypes +from website.mails import NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST + + +@pytest.mark.django_db +class TestNodeRequestListInstitutionalAccess(NodeRequestTestMixin): + + @pytest.fixture() + def url(self, project): + return f'/{API_BASE}nodes/{project._id}/requests/' + + @pytest.fixture() + def institution(self): + return InstitutionFactory() + + @pytest.fixture() + def institution2(self): + return InstitutionFactory() + + @pytest.fixture() + def user_with_affiliation(self, institution): + user = AuthUserFactory() + user.add_or_update_affiliated_institution(institution) + return user + + @pytest.fixture() + def user_without_affiliation(self): + return AuthUserFactory() + + @pytest.fixture() + def institutional_admin(self, institution): + admin_user = AuthUserFactory() + institution.get_group('institutional_admins').user_set.add(admin_user) + return admin_user + + @pytest.fixture() + def create_payload(self, institution, user_with_affiliation): + return { + 'data': { + 'attributes': { + 'comment': 'Wanna Philly Philly?', + 'request_type': NodeRequestTypes.INSTITUTIONAL_REQUEST.value, + }, + 'relationships': { + 'institution': { + 'data': { + 'id': institution._id, + 'type': 'institutions' + } + }, + 'message_recipient': { + 'data': { + 'id': user_with_affiliation._id, + 'type': 'users' + } + } + }, + 'type': 'node-requests' + } + } + + @pytest.fixture() + def node_with_disabled_access_requests(self, institution): + node = NodeFactory() + node.access_requests_enabled = False + creator = node.creator + creator.add_or_update_affiliated_institution(institution) + creator.save() + node.add_affiliated_institution(institution, creator) + node.save() + return node + + def test_institutional_admin_can_make_institutional_request(self, app, project, institutional_admin, url, create_payload): + """ + Test that an institutional admin can make an institutional access request. + """ + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + assert res.status_code == 201 + + # Verify the NodeRequest object is created + node_request = project.requests.get(creator=institutional_admin) + assert node_request.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value + assert node_request.comment == 'Wanna Philly Philly?' + assert node_request.machine_state == DefaultStates.PENDING.value + + def test_non_admin_cant_make_institutional_request(self, app, project, noncontrib, url, create_payload): + """ + Test that a non-institutional admin cannot make an institutional access request. + """ + res = app.post_json_api(url, create_payload, auth=noncontrib.auth, expect_errors=True) + assert res.status_code == 403 + assert 'You do not have permission to perform this action for this institution.' in res.json['errors'][0]['detail'] + + def test_institutional_admin_can_add_requested_permission(self, app, project, institutional_admin, url, create_payload): + """ + Test that an institutional admin can make an institutional access request with requested_permissions. + """ + create_payload['data']['attributes']['requested_permissions'] = 'admin' + + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + assert res.status_code == 201 + + # Verify the NodeRequest object is created with the correct requested_permissions + node_request = project.requests.get(creator=institutional_admin) + assert node_request.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value + assert node_request.requested_permissions == 'admin' + + def test_institutional_admin_needs_institution(self, app, project, institutional_admin, url, create_payload): + """ + Test that the payload needs the institution relationship and gives the correct error message. + """ + del create_payload['data']['relationships']['institution'] + + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth, expect_errors=True) + assert res.status_code == 400 + assert 'Institution is required.' in res.json['errors'][0]['detail'] + + def test_institutional_admin_invalid_institution(self, app, project, institutional_admin, url, create_payload): + """ + Test that the payload validates the institution relationship and gives the correct error message when it's + invalid. + """ + create_payload['data']['relationships']['institution']['data']['id'] = 'invalid_id' + + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth, expect_errors=True) + assert res.status_code == 400 + assert 'Institution is does not exist.' in res.json['errors'][0]['detail'] + + def test_institutional_admin_unauth_institution(self, app, project, institution2, institutional_admin, url, create_payload): + """ + Test that the view authenticates the relationship between the institution and the user and gives the correct + error message when it's unauthorized.' + """ + create_payload['data']['relationships']['institution']['data']['id'] = institution2._id + + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth, expect_errors=True) + assert res.status_code == 403 + assert 'You do not have permission to perform this action for this institution.' in res.json['errors'][0]['detail'] + + @mock.patch('api.requests.serializers.send_mail') + def test_email_not_sent_without_recipient(self, mock_mail, app, project, institutional_admin, url, + create_payload, institution): + """ + Test that an email is not sent when no recipient is listed when an institutional access request is made, + but the request is still made anyway without email. + """ + del create_payload['data']['relationships']['message_recipient'] + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + assert res.status_code == 201 + + # Check that an email is sent + assert not mock_mail.called + + @mock.patch('api.requests.serializers.send_mail') + def test_email_not_sent_outside_institution(self, mock_mail, app, project, institutional_admin, url, + create_payload, user_without_affiliation, institution): + """ + Test that you are prevented from requesting a user with the correct institutional affiliation. + """ + create_payload['data']['relationships']['message_recipient']['data']['id'] = user_without_affiliation._id + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth, expect_errors=True) + assert res.status_code == 403 + assert f'User {user_without_affiliation._id} is not affiliated with the institution.' in res.json['errors'][0]['detail'] + + # Check that an email is sent + assert not mock_mail.called + + @mock.patch('api.requests.serializers.send_mail') + def test_email_sent_on_creation( + self, + mock_mail, + app, + project, + institutional_admin, + url, + user_with_affiliation, + create_payload, + institution + ): + """ + Test that an email is sent to the appropriate recipients when an institutional access request is made. + """ + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + assert res.status_code == 201 + + assert mock_mail.call_count == 1 + + mock_mail.assert_called_with( + to_addr=user_with_affiliation.username, + mail=NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + bcc_addr=None, + reply_to=None, + **{ + 'sender': institutional_admin, + 'recipient': user_with_affiliation, + 'comment': create_payload['data']['attributes']['comment'], + 'institution': institution, + 'osf_url': mock.ANY, + 'node': project, + } + ) + + @mock.patch('api.requests.serializers.send_mail') + def test_bcc_institutional_admin( + self, + mock_mail, + app, + project, + institutional_admin, + url, + user_with_affiliation, + create_payload, + institution + ): + """ + Ensure BCC option works as expected, sending messages to sender giving them a copy for themselves. + """ + create_payload['data']['attributes']['bcc_sender'] = True + + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + assert res.status_code == 201 + + assert mock_mail.call_count == 1 + + mock_mail.assert_called_with( + to_addr=user_with_affiliation.username, + mail=NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + bcc_addr=[institutional_admin.username], + reply_to=None, + **{ + 'sender': institutional_admin, + 'recipient': user_with_affiliation, + 'comment': create_payload['data']['attributes']['comment'], + 'institution': institution, + 'osf_url': mock.ANY, + 'node': project, + } + ) + + @mock.patch('api.requests.serializers.send_mail') + def test_reply_to_institutional_admin( + self, + mock_mail, + app, + project, + institutional_admin, + url, + user_with_affiliation, + create_payload, + institution + ): + """ + Ensure reply-to option works as expected, allowing a reply to header be added to the email. + """ + create_payload['data']['attributes']['reply_to'] = True + + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + assert res.status_code == 201 + + assert mock_mail.call_count == 1 + + mock_mail.assert_called_with( + to_addr=user_with_affiliation.username, + mail=NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + bcc_addr=None, + reply_to=institutional_admin.username, + **{ + 'sender': institutional_admin, + 'recipient': user_with_affiliation, + 'comment': create_payload['data']['attributes']['comment'], + 'institution': institution, + 'osf_url': mock.ANY, + 'node': project, + } + ) + + def test_access_requests_disabled_raises_permission_denied( + self, app, node_with_disabled_access_requests, user_with_affiliation, institutional_admin, create_payload + ): + """ + Ensure that when `access_requests_enabled` is `False`, a PermissionDenied error is raised. + """ + res = app.post_json_api( + f'/{API_BASE}nodes/{node_with_disabled_access_requests._id}/requests/', + create_payload, + auth=institutional_admin.auth, + expect_errors=True + ) + assert res.status_code == 403 + assert f"{node_with_disabled_access_requests._id} does not have Access Requests enabled" in res.json['errors'][0]['detail'] diff --git a/api_tests/requests/views/test_node_request_list.py b/api_tests/requests/views/test_node_request_list.py index 668428a1632..f94d3b4acf2 100644 --- a/api_tests/requests/views/test_node_request_list.py +++ b/api_tests/requests/views/test_node_request_list.py @@ -4,14 +4,8 @@ from api.base.settings.defaults import API_BASE from api_tests.requests.mixins import NodeRequestTestMixin -from osf_tests.factories import ( - NodeFactory, - NodeRequestFactory, - InstitutionFactory, - AuthUserFactory -) +from osf_tests.factories import NodeFactory, NodeRequestFactory from osf.utils.workflows import DefaultStates, NodeRequestTypes -from website.mails import NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST @pytest.mark.django_db @@ -129,267 +123,3 @@ def test_filter_by_machine_state(self, app, project, noncontrib, url, admin, nod ids = [result['id'] for result in res.json['data']] assert initial_node_request._id not in ids assert node_request.machine_state == 'pending' and node_request._id in ids - -@pytest.mark.django_db -class TestNodeRequestListInstitutionalAccess(NodeRequestTestMixin): - - @pytest.fixture() - def url(self, project): - return f'/{API_BASE}nodes/{project._id}/requests/' - - @pytest.fixture() - def institution(self): - return InstitutionFactory() - - @pytest.fixture() - def institution2(self): - return InstitutionFactory() - - @pytest.fixture() - def user_with_affiliation(self, institution): - user = AuthUserFactory() - user.add_or_update_affiliated_institution(institution) - return user - - @pytest.fixture() - def user_without_affiliation(self): - return AuthUserFactory() - - @pytest.fixture() - def institutional_admin(self, institution): - admin_user = AuthUserFactory() - institution.get_group('institutional_admins').user_set.add(admin_user) - return admin_user - - @pytest.fixture() - def create_payload(self, institution, user_with_affiliation): - return { - 'data': { - 'attributes': { - 'comment': 'Wanna Philly Philly?', - 'request_type': NodeRequestTypes.INSTITUTIONAL_REQUEST.value, - }, - 'relationships': { - 'institution': { - 'data': { - 'id': institution._id, - 'type': 'institutions' - } - }, - 'message_recipient': { - 'data': { - 'id': user_with_affiliation._id, - 'type': 'users' - } - } - }, - 'type': 'node-requests' - } - } - - def test_institutional_admin_can_make_institutional_request(self, app, project, institutional_admin, url, create_payload): - """ - Test that an institutional admin can make an institutional access request. - """ - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) - assert res.status_code == 201 - - # Verify the NodeRequest object is created - node_request = project.requests.get(creator=institutional_admin) - assert node_request.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value - assert node_request.comment == 'Wanna Philly Philly?' - assert node_request.machine_state == DefaultStates.PENDING.value - - def test_non_admin_cant_make_institutional_request(self, app, project, noncontrib, url, create_payload): - """ - Test that a non-institutional admin cannot make an institutional access request. - """ - res = app.post_json_api(url, create_payload, auth=noncontrib.auth, expect_errors=True) - assert res.status_code == 403 - assert 'You do not have permission to perform this action for this institution.' in res.json['errors'][0]['detail'] - - def test_institutional_admin_can_add_requested_permission(self, app, project, institutional_admin, url, create_payload): - """ - Test that an institutional admin can make an institutional access request with requested_permissions. - """ - create_payload['data']['attributes']['requested_permissions'] = 'admin' - - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) - assert res.status_code == 201 - - # Verify the NodeRequest object is created with the correct requested_permissions - node_request = project.requests.get(creator=institutional_admin) - assert node_request.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value - assert node_request.requested_permissions == 'admin' - - def test_institutional_admin_needs_institution(self, app, project, institutional_admin, url, create_payload): - """ - Test that the payload needs the institution relationship and gives the correct error message. - """ - del create_payload['data']['relationships']['institution'] - - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth, expect_errors=True) - assert res.status_code == 400 - assert 'Institution is required.' in res.json['errors'][0]['detail'] - - def test_institutional_admin_invalid_institution(self, app, project, institutional_admin, url, create_payload): - """ - Test that the payload validates the institution relationship and gives the correct error message when it's - invalid. - """ - create_payload['data']['relationships']['institution']['data']['id'] = 'invalid_id' - - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth, expect_errors=True) - assert res.status_code == 400 - assert 'Institution is does not exist.' in res.json['errors'][0]['detail'] - - def test_institutional_admin_unauth_institution(self, app, project, institution2, institutional_admin, url, create_payload): - """ - Test that the view authenticates the relationship between the institution and the user and gives the correct - error message when it's unauthorized.' - """ - create_payload['data']['relationships']['institution']['data']['id'] = institution2._id - - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth, expect_errors=True) - assert res.status_code == 403 - assert 'You do not have permission to perform this action for this institution.' in res.json['errors'][0]['detail'] - - @mock.patch('api.requests.serializers.send_mail') - def test_email_not_sent_without_recipient(self, mock_mail, app, project, institutional_admin, url, - create_payload, institution): - """ - Test that an email is not sent when no recipient is listed when an institutional access request is made, - but the request is still made anyway without email. - """ - del create_payload['data']['relationships']['message_recipient'] - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) - assert res.status_code == 201 - - # Check that an email is sent - assert not mock_mail.called - - @mock.patch('api.requests.serializers.send_mail') - def test_email_not_sent_outside_institution(self, mock_mail, app, project, institutional_admin, url, - create_payload, user_without_affiliation, institution): - """ - Test that you are prevented from requesting a user with the correct institutional affiliation. - """ - create_payload['data']['relationships']['message_recipient']['data']['id'] = user_without_affiliation._id - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth, expect_errors=True) - assert res.status_code == 403 - assert f'User {user_without_affiliation._id} is not affiliated with the institution.' in res.json['errors'][0]['detail'] - - # Check that an email is sent - assert not mock_mail.called - - @mock.patch('api.requests.serializers.send_mail') - def test_email_sent_on_creation( - self, - mock_mail, - app, - project, - institutional_admin, - url, - user_with_affiliation, - create_payload, - institution - ): - """ - Test that an email is sent to the appropriate recipients when an institutional access request is made. - """ - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) - assert res.status_code == 201 - - assert mock_mail.call_count == 1 - - mock_mail.assert_called_with( - to_addr=user_with_affiliation.username, - mail=NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST, - user=user_with_affiliation, - bcc_addr=None, - reply_to=None, - **{ - 'sender': institutional_admin, - 'recipient': user_with_affiliation, - 'comment': create_payload['data']['attributes']['comment'], - 'institution': institution, - 'osf_url': mock.ANY, - 'node': project, - } - ) - - @mock.patch('api.requests.serializers.send_mail') - def test_bcc_institutional_admin( - self, - mock_mail, - app, - project, - institutional_admin, - url, - user_with_affiliation, - create_payload, - institution - ): - """ - Ensure BCC option works as expected, sending messages to sender giving them a copy for themselves. - """ - create_payload['data']['attributes']['bcc_sender'] = True - - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) - assert res.status_code == 201 - - assert mock_mail.call_count == 1 - - mock_mail.assert_called_with( - to_addr=user_with_affiliation.username, - mail=NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST, - user=user_with_affiliation, - bcc_addr=[institutional_admin.username], - reply_to=None, - **{ - 'sender': institutional_admin, - 'recipient': user_with_affiliation, - 'comment': create_payload['data']['attributes']['comment'], - 'institution': institution, - 'osf_url': mock.ANY, - 'node': project, - } - ) - - @mock.patch('api.requests.serializers.send_mail') - def test_reply_to_institutional_admin( - self, - mock_mail, - app, - project, - institutional_admin, - url, - user_with_affiliation, - create_payload, - institution - ): - """ - Ensure reply-to option works as expected, allowing a reply to header be added to the email. - """ - create_payload['data']['attributes']['reply_to'] = True - - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) - assert res.status_code == 201 - - assert mock_mail.call_count == 1 - - mock_mail.assert_called_with( - to_addr=user_with_affiliation.username, - mail=NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST, - user=user_with_affiliation, - bcc_addr=None, - reply_to=institutional_admin.username, - **{ - 'sender': institutional_admin, - 'recipient': user_with_affiliation, - 'comment': create_payload['data']['attributes']['comment'], - 'institution': institution, - 'osf_url': mock.ANY, - 'node': project, - } - )