From c4cebd8f519a83d8222264671b77e603bfbc6683 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Mon, 30 Dec 2024 12:30:21 +0200 Subject: [PATCH 1/7] Changes for "can_request_access" feature --- api/institutions/serializers.py | 1 + .../0026_institution_can_request_access.py | 18 ++++++++++++++++++ osf/models/institution.py | 1 + osf/models/user.py | 2 +- 4 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 osf/migrations/0026_institution_can_request_access.py diff --git a/api/institutions/serializers.py b/api/institutions/serializers.py index 1d1e0761715..039d8fe6379 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', ) + can_request_access = ser.CharField(read_only=True) links = LinksField({ 'self': 'get_api_url', 'html': 'get_absolute_html_url', diff --git a/osf/migrations/0026_institution_can_request_access.py b/osf/migrations/0026_institution_can_request_access.py new file mode 100644 index 00000000000..355a4334182 --- /dev/null +++ b/osf/migrations/0026_institution_can_request_access.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.15 on 2024-12-27 14:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0025_noderequest_requested_permissions_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='institution', + name='can_request_access', + field=models.BooleanField(db_index=True, default=False), + ), + ] diff --git a/osf/models/institution.py b/osf/models/institution.py index d0ce38eacf4..4e3d25ad52f 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' ) + can_request_access = models.BooleanField(default=False, db_index=True) 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/models/user.py b/osf/models/user.py index 411381b0687..311378c9642 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -646,7 +646,7 @@ def osf_groups(self): def is_institutional_admin(self, institution): group_name = institution.format_group('institutional_admins') - return self.groups.filter(name=group_name).exists() + return self.groups.filter(name=group_name).exists() and institution.can_request_access def group_role(self, group): """ From b9c92b8d1b4db569c1e6fe9b67878e624fb3d7eb Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Tue, 31 Dec 2024 14:02:23 +0200 Subject: [PATCH 2/7] fixed tests, added new tests, removed index on `can_request_access` --- .../test_node_request_institutional_access.py | 2 +- .../test_user_message_institutional_access.py | 41 +++++++++++++++++++ osf/models/institution.py | 2 +- osf_tests/factories.py | 4 ++ 4 files changed, 47 insertions(+), 2 deletions(-) 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..d7d2bcd4065 100644 --- a/api_tests/requests/views/test_node_request_institutional_access.py +++ b/api_tests/requests/views/test_node_request_institutional_access.py @@ -18,7 +18,7 @@ def url(self, project): @pytest.fixture() def institution(self): - return InstitutionFactory() + return InstitutionFactory(can_request_access=True) @pytest.fixture() def institution2(self): 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..074befa62f7 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(can_request_access=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 'can_request_access' 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/models/institution.py b/osf/models/institution.py index 4e3d25ad52f..0c04c73e3bc 100644 --- a/osf/models/institution.py +++ b/osf/models/institution.py @@ -103,7 +103,7 @@ class Institution(DirtyFieldsMixin, Loggable, ObjectIDMixin, BaseModel, Guardian related_name='institutions' ) - can_request_access = models.BooleanField(default=False, db_index=True) + can_request_access = 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..1ed47c0f090 100644 --- a/osf_tests/factories.py +++ b/osf_tests/factories.py @@ -257,10 +257,14 @@ class InstitutionFactory(DjangoModelFactory): email_domains = FakeList('domain_name', n=1) orcid_record_verified_source = '' delegation_protocol = '' + can_request_access = False class Meta: model = models.Institution + @classmethod + def _create(cls, *args, **kwargs): + return super()._create(*args, **kwargs) class NodeLicenseRecordFactory(DjangoModelFactory): year = factory.Faker('year') From edbde188d43e97f18fc4e5ef9af90d602220022f Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Thu, 2 Jan 2025 16:42:25 +0200 Subject: [PATCH 3/7] fixed tests --- .../users/views/test_user_message_institutional_access.py | 2 +- framework/email/tasks.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) 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 074befa62f7..37baa0de1f2 100644 --- a/api_tests/users/views/test_user_message_institutional_access.py +++ b/api_tests/users/views/test_user_message_institutional_access.py @@ -112,7 +112,7 @@ def test_institutional_admin_can_create_message(self, mock_send_mail, app, insti @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, + 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 'can_request_access' as False diff --git a/framework/email/tasks.py b/framework/email/tasks.py index cf43395222e..76f04f056a5 100644 --- a/framework/email/tasks.py +++ b/framework/email/tasks.py @@ -163,7 +163,8 @@ def _send_with_sendgrid( client = client or SendGridAPIClient(settings.SENDGRID_API_KEY) mail = Mail( - from_email=Email(from_addr), + from_email=from_addr, + to_emails=to_addr, html_content=message, subject=subject, ) @@ -191,7 +192,7 @@ def _send_with_sendgrid( mail.add_personalization(personalization) if categories: - mail.add_category([Category(x) for x in categories]) + mail.category = [Category(x) for x in categories] if attachment_name and attachment_content: attachment = Attachment( From 33193929d4eeafc0c5acd894052b0c3ba0b9d5c5 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Thu, 2 Jan 2025 17:32:26 +0200 Subject: [PATCH 4/7] remove unsued import --- framework/email/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/framework/email/tasks.py b/framework/email/tasks.py index 76f04f056a5..67d08d28254 100644 --- a/framework/email/tasks.py +++ b/framework/email/tasks.py @@ -12,7 +12,6 @@ Category, Attachment, FileContent, - Email, To, Personalization, Cc, From aad97170daf984cee0a138a1891dde104f75d1ab Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Fri, 3 Jan 2025 15:25:24 +0200 Subject: [PATCH 5/7] reverted changes in tasks --- framework/email/tasks.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/framework/email/tasks.py b/framework/email/tasks.py index 67d08d28254..cf43395222e 100644 --- a/framework/email/tasks.py +++ b/framework/email/tasks.py @@ -12,6 +12,7 @@ Category, Attachment, FileContent, + Email, To, Personalization, Cc, @@ -162,8 +163,7 @@ def _send_with_sendgrid( client = client or SendGridAPIClient(settings.SENDGRID_API_KEY) mail = Mail( - from_email=from_addr, - to_emails=to_addr, + from_email=Email(from_addr), html_content=message, subject=subject, ) @@ -191,7 +191,7 @@ def _send_with_sendgrid( mail.add_personalization(personalization) if categories: - mail.category = [Category(x) for x in categories] + mail.add_category([Category(x) for x in categories]) if attachment_name and attachment_content: attachment = Attachment( From c8e345ca384c5aa50d615b847f6c16f54aa857e4 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Wed, 8 Jan 2025 20:55:04 +0200 Subject: [PATCH 6/7] fixed comments --- api/institutions/serializers.py | 2 +- api/requests/permissions.py | 3 ++ api/users/permissions.py | 2 +- .../test_node_request_institutional_access.py | 54 ++++++++++++++++++- .../test_user_message_institutional_access.py | 4 +- ...erequest_requested_permissions_and_more.py | 7 ++- .../0026_institution_can_request_access.py | 18 ------- osf/models/institution.py | 2 +- osf/models/user.py | 2 +- osf_tests/factories.py | 5 +- 10 files changed, 68 insertions(+), 31 deletions(-) delete mode 100644 osf/migrations/0026_institution_can_request_access.py diff --git a/api/institutions/serializers.py b/api/institutions/serializers.py index 039d8fe6379..46a2f4f0bff 100644 --- a/api/institutions/serializers.py +++ b/api/institutions/serializers.py @@ -41,7 +41,7 @@ class InstitutionSerializer(JSONAPISerializer): ser.CharField(read_only=True), permission='view_institutional_metrics', ) - can_request_access = ser.CharField(read_only=True) + 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..61a9ab229f4 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 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 d7d2bcd4065..8801d80bffc 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(can_request_access=True) + 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, institution2): + user = AuthUserFactory() + user.add_or_update_affiliated_institution(institution2) + 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, institution2): + admin_user = AuthUserFactory() + institution2.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. 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 37baa0de1f2..36f2a59e252 100644 --- a/api_tests/users/views/test_user_message_institutional_access.py +++ b/api_tests/users/views/test_user_message_institutional_access.py @@ -18,7 +18,7 @@ class TestUserMessageInstitutionalAccess: @pytest.fixture() def institution(self): - return InstitutionFactory(can_request_access=True) + return InstitutionFactory(institutional_request_access_enabled=True) @pytest.fixture() def institution_without_access(self): @@ -115,7 +115,7 @@ def test_institutional_admin_can_not_create_message(self, mock_send_mail, app, i 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 'can_request_access' as False + 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() 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/migrations/0026_institution_can_request_access.py b/osf/migrations/0026_institution_can_request_access.py deleted file mode 100644 index 355a4334182..00000000000 --- a/osf/migrations/0026_institution_can_request_access.py +++ /dev/null @@ -1,18 +0,0 @@ -# Generated by Django 4.2.15 on 2024-12-27 14:08 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('osf', '0025_noderequest_requested_permissions_and_more'), - ] - - operations = [ - migrations.AddField( - model_name='institution', - name='can_request_access', - field=models.BooleanField(db_index=True, default=False), - ), - ] diff --git a/osf/models/institution.py b/osf/models/institution.py index 0c04c73e3bc..5dce3c1df36 100644 --- a/osf/models/institution.py +++ b/osf/models/institution.py @@ -103,7 +103,7 @@ class Institution(DirtyFieldsMixin, Loggable, ObjectIDMixin, BaseModel, Guardian related_name='institutions' ) - can_request_access = models.BooleanField(default=False) + 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/models/user.py b/osf/models/user.py index 311378c9642..411381b0687 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -646,7 +646,7 @@ def osf_groups(self): def is_institutional_admin(self, institution): group_name = institution.format_group('institutional_admins') - return self.groups.filter(name=group_name).exists() and institution.can_request_access + return self.groups.filter(name=group_name).exists() def group_role(self, group): """ diff --git a/osf_tests/factories.py b/osf_tests/factories.py index 1ed47c0f090..7df749c0f72 100644 --- a/osf_tests/factories.py +++ b/osf_tests/factories.py @@ -257,14 +257,11 @@ class InstitutionFactory(DjangoModelFactory): email_domains = FakeList('domain_name', n=1) orcid_record_verified_source = '' delegation_protocol = '' - can_request_access = False + institutional_request_access_enabled = False class Meta: model = models.Institution - @classmethod - def _create(cls, *args, **kwargs): - return super()._create(*args, **kwargs) class NodeLicenseRecordFactory(DjangoModelFactory): year = factory.Faker('year') From bdf304ec2daaf0c32e9f3211c6b849cce034b14b Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Wed, 8 Jan 2025 23:39:51 +0200 Subject: [PATCH 7/7] fixed test --- .../test_node_request_institutional_access.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 8801d80bffc..c38fcfcafd3 100644 --- a/api_tests/requests/views/test_node_request_institutional_access.py +++ b/api_tests/requests/views/test_node_request_institutional_access.py @@ -31,9 +31,9 @@ def user_with_affiliation(self, institution): return user @pytest.fixture() - def user_with_affiliation_on_institution_without_access(self, institution2): + def user_with_affiliation_on_institution_without_access(self, institution_without_access): user = AuthUserFactory() - user.add_or_update_affiliated_institution(institution2) + user.add_or_update_affiliated_institution(institution_without_access) return user @pytest.fixture() @@ -47,9 +47,9 @@ def institutional_admin(self, institution): return admin_user @pytest.fixture() - def institutional_admin_on_institution_without_access(self, institution2): + def institutional_admin_on_institution_without_access(self, institution_without_access): admin_user = AuthUserFactory() - institution2.get_group('institutional_admins').user_set.add(admin_user) + institution_without_access.get_group('institutional_admins').user_set.add(admin_user) return admin_user @pytest.fixture() @@ -183,12 +183,12 @@ 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