From d9540f3c27db1d2ad58aaa71af090974bfdc81c5 Mon Sep 17 00:00:00 2001 From: bodintsov Date: Thu, 9 Jan 2025 21:13:40 +0200 Subject: [PATCH] Changes for "can_request_access" feature (#10877) Add to admin "can_request_access" option to the institution API Changes for "can_request_access" feature Done 2 tasks (ENG-6779) and (ENG-6780) within one PR, due to tickets being related to each other --- api/institutions/serializers.py | 1 + api/requests/permissions.py | 3 + api/users/permissions.py | 2 +- .../test_node_request_institutional_access.py | 62 +++++++++++++++++-- .../test_user_message_institutional_access.py | 41 ++++++++++++ ...erequest_requested_permissions_and_more.py | 7 ++- osf/models/institution.py | 1 + osf_tests/factories.py | 1 + 8 files changed, 110 insertions(+), 8 deletions(-) diff --git a/api/institutions/serializers.py b/api/institutions/serializers.py index 1d1e0761715..46a2f4f0bff 100644 --- a/api/institutions/serializers.py +++ b/api/institutions/serializers.py @@ -41,6 +41,7 @@ class InstitutionSerializer(JSONAPISerializer): ser.CharField(read_only=True), permission='view_institutional_metrics', ) + institutional_request_access_enabled = ser.CharField(read_only=True) links = LinksField({ 'self': 'get_api_url', 'html': 'get_absolute_html_url', diff --git a/api/requests/permissions.py b/api/requests/permissions.py index cb08a6a94e5..6811323c058 100644 --- a/api/requests/permissions.py +++ b/api/requests/permissions.py @@ -78,6 +78,9 @@ def has_permission(self, request, view): except Institution.DoesNotExist: raise exceptions.ValidationError({'institution': 'Institution is does not exist.'}) + if not institution.institutional_request_access_enabled: + raise exceptions.PermissionDenied({'institution': 'Institutional request access is not enabled.'}) + if get_user_auth(request).user.is_institutional_admin(institution): return True else: diff --git a/api/users/permissions.py b/api/users/permissions.py index 7ca5e3ef862..00510ec8b30 100644 --- a/api/users/permissions.py +++ b/api/users/permissions.py @@ -79,6 +79,6 @@ def has_permission(self, request, view) -> bool: message_type = request.data.get('message_type') if message_type == MessageTypes.INSTITUTIONAL_REQUEST: - return user.is_institutional_admin(institution) + return user.is_institutional_admin(institution) and institution.institutional_request_access_enabled else: raise exceptions.ValidationError('Not valid message type.') diff --git a/api_tests/requests/views/test_node_request_institutional_access.py b/api_tests/requests/views/test_node_request_institutional_access.py index e989ccbc469..72b0fcce03b 100644 --- a/api_tests/requests/views/test_node_request_institutional_access.py +++ b/api_tests/requests/views/test_node_request_institutional_access.py @@ -18,10 +18,10 @@ def url(self, project): @pytest.fixture() def institution(self): - return InstitutionFactory() + return InstitutionFactory(institutional_request_access_enabled=True) @pytest.fixture() - def institution2(self): + def institution_without_access(self): return InstitutionFactory() @pytest.fixture() @@ -30,6 +30,12 @@ def user_with_affiliation(self, institution): user.add_or_update_affiliated_institution(institution) return user + @pytest.fixture() + def user_with_affiliation_on_institution_without_access(self, institution_without_access): + user = AuthUserFactory() + user.add_or_update_affiliated_institution(institution_without_access) + return user + @pytest.fixture() def user_without_affiliation(self): return AuthUserFactory() @@ -40,6 +46,12 @@ def institutional_admin(self, institution): institution.get_group('institutional_admins').user_set.add(admin_user) return admin_user + @pytest.fixture() + def institutional_admin_on_institution_without_access(self, institution_without_access): + admin_user = AuthUserFactory() + institution_without_access.get_group('institutional_admins').user_set.add(admin_user) + return admin_user + @pytest.fixture() def create_payload(self, institution, user_with_affiliation): return { @@ -66,6 +78,32 @@ def create_payload(self, institution, user_with_affiliation): } } + @pytest.fixture() + def create_payload_on_institution_without_access(self, institution_without_access, user_with_affiliation_on_institution_without_access): + return { + 'data': { + 'attributes': { + 'comment': 'Wanna Philly Philly?', + 'request_type': NodeRequestTypes.INSTITUTIONAL_REQUEST.value, + }, + 'relationships': { + 'institution': { + 'data': { + 'id': institution_without_access._id, + 'type': 'institutions' + } + }, + 'message_recipient': { + 'data': { + 'id': user_with_affiliation_on_institution_without_access._id, + 'type': 'users' + } + } + }, + 'type': 'node-requests' + } + } + @pytest.fixture() def node_with_disabled_access_requests(self, institution): node = NodeFactory() @@ -112,6 +150,18 @@ def test_institutional_admin_can_add_requested_permission(self, app, project, in assert node_request.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value assert node_request.requested_permissions == 'admin' + def test_institutional_admin_can_not_add_requested_permission(self, app, project, institutional_admin_on_institution_without_access, url, create_payload_on_institution_without_access): + """ + Test that an institutional admin can not make an institutional access request on institution with disabled access . + """ + create_payload_on_institution_without_access['data']['attributes']['requested_permissions'] = 'admin' + + res = app.post_json_api( + url, create_payload_on_institution_without_access, auth=institutional_admin_on_institution_without_access.auth, expect_errors=True + ) + + assert res.status_code == 403 + 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. @@ -133,16 +183,16 @@ def test_institutional_admin_invalid_institution(self, app, project, institution 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): + def test_institutional_admin_unauth_institution(self, app, project, institution_without_access, 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.' + error message when it's unauthorized """ - create_payload['data']['relationships']['institution']['data']['id'] = institution2._id + create_payload['data']['relationships']['institution']['data']['id'] = institution_without_access._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'] + assert 'Institutional request access is not enabled.' 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, diff --git a/api_tests/users/views/test_user_message_institutional_access.py b/api_tests/users/views/test_user_message_institutional_access.py index c058a1f135d..36f2a59e252 100644 --- a/api_tests/users/views/test_user_message_institutional_access.py +++ b/api_tests/users/views/test_user_message_institutional_access.py @@ -7,6 +7,7 @@ InstitutionFactory ) from website.mails import USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST +from webtest import AppError @pytest.mark.django_db @@ -17,6 +18,10 @@ class TestUserMessageInstitutionalAccess: @pytest.fixture() def institution(self): + return InstitutionFactory(institutional_request_access_enabled=True) + + @pytest.fixture() + def institution_without_access(self): return InstitutionFactory() @pytest.fixture() @@ -33,16 +38,32 @@ def user_with_affiliation(self, institution): user.add_or_update_affiliated_institution(institution) return user + @pytest.fixture() + def user_with_affiliation_on_institution_without_access(self, institution_without_access): + user = AuthUserFactory() + user.add_or_update_affiliated_institution(institution_without_access) + return user + @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 institutional_admin_on_institution_without_access(self, institution_without_access): + admin_user = AuthUserFactory() + institution_without_access.get_group('institutional_admins').user_set.add(admin_user) + return admin_user + @pytest.fixture() def url_with_affiliation(self, user_with_affiliation): return f'/{API_BASE}users/{user_with_affiliation._id}/messages/' + @pytest.fixture() + def url_with_affiliation_on_institution_without_access(self, user_with_affiliation_on_institution_without_access): + return f'/{API_BASE}users/{user_with_affiliation_on_institution_without_access._id}/messages/' + @pytest.fixture() def url_without_affiliation(self, user): return f'/{API_BASE}users/{user._id}/messages/' @@ -89,6 +110,26 @@ def test_institutional_admin_can_create_message(self, mock_send_mail, app, insti assert 'Requesting user access for collaboration' in mock_send_mail.call_args[1]['message_text'] assert user_message._id == data['id'] + @mock.patch('osf.models.user_message.send_mail') + def test_institutional_admin_can_not_create_message(self, mock_send_mail, app, institutional_admin_on_institution_without_access, + institution_without_access, url_with_affiliation_on_institution_without_access, + payload): + """ + Ensure an institutional admin cannot create a `UserMessage` with a `message` and `institution` witch has 'institutional_request_access_enabled' as False + """ + mock_send_mail.return_value = mock.MagicMock() + + # Use pytest.raises to explicitly expect the 403 error + with pytest.raises(AppError) as exc_info: + app.post_json_api( + url_with_affiliation_on_institution_without_access, + payload, + auth=institutional_admin_on_institution_without_access.auth + ) + + # Assert that the raised error contains the 403 Forbidden status + assert '403 Forbidden' in str(exc_info.value) + def test_unauthenticated_user_cannot_create_message(self, app, user, url_with_affiliation, payload): """ Ensure that unauthenticated users cannot create a `UserMessage`. diff --git a/osf/migrations/0025_noderequest_requested_permissions_and_more.py b/osf/migrations/0025_noderequest_requested_permissions_and_more.py index 32183df5067..d936e1393e7 100644 --- a/osf/migrations/0025_noderequest_requested_permissions_and_more.py +++ b/osf/migrations/0025_noderequest_requested_permissions_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.13 on 2024-12-12 19:37 +# Generated by Django 4.2.15 on 2025-01-08 12:24 from django.conf import settings from django.db import migrations, models @@ -14,6 +14,11 @@ class Migration(migrations.Migration): ] operations = [ + migrations.AddField( + model_name='institution', + name='institutional_request_access_enabled', + field=models.BooleanField(default=False), + ), migrations.AddField( model_name='noderequest', name='requested_permissions', diff --git a/osf/models/institution.py b/osf/models/institution.py index d0ce38eacf4..5dce3c1df36 100644 --- a/osf/models/institution.py +++ b/osf/models/institution.py @@ -103,6 +103,7 @@ class Institution(DirtyFieldsMixin, Loggable, ObjectIDMixin, BaseModel, Guardian related_name='institutions' ) + institutional_request_access_enabled = models.BooleanField(default=False) is_deleted = models.BooleanField(default=False, db_index=True) deleted = NonNaiveDateTimeField(null=True, blank=True) deactivated = NonNaiveDateTimeField(null=True, blank=True) diff --git a/osf_tests/factories.py b/osf_tests/factories.py index ca3d2dacce4..7df749c0f72 100644 --- a/osf_tests/factories.py +++ b/osf_tests/factories.py @@ -257,6 +257,7 @@ class InstitutionFactory(DjangoModelFactory): email_domains = FakeList('domain_name', n=1) orcid_record_verified_source = '' delegation_protocol = '' + institutional_request_access_enabled = False class Meta: model = models.Institution