From 94be9637965a89169e15fb582cc2c30b585011f3 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 21 Jan 2025 14:59:30 -0500 Subject: [PATCH] Feature/institutional access (#10884) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Description: Update role permissions to enable institutional administrators to make metadata revisions. This is critical to enabling increased control for institutional admins, supporting researchers in metadata curation and meeting data sharing policies. Enable reviews workflow that allows institutional admins to add or remove affiliations from OSF content if appropriate. This is critical to enabling increased control for institutional admins as they monitor data sharing compliance. Impact: OSFI members are facing new requirements on data sharing and need workflows to support compliance. Renewals could decline as other tools enable these solutions and OSF doesn’t; new revenue potential with solutions that meet data sharing compliance needs. Commitment: We have 50+ OSFI members that haven’t had new features on OSF since joining, we need to listen to their needs and help make it easy and possible for them to enact open scholarship practices with their researchers. [ENG-4980] --- api/institutions/serializers.py | 1 + api/nodes/views.py | 3 +- api/requests/permissions.py | 46 +- api/requests/serializers.py | 133 +++++- api/users/permissions.py | 39 +- api/users/serializers.py | 124 +++++- api/users/urls.py | 1 + api/users/views.py | 22 + .../test_node_contributor_insti_admin.py | 67 +++ .../registries_moderation/test_submissions.py | 6 +- .../test_node_request_institutional_access.py | 397 ++++++++++++++++++ ...st_create.py => test_node_request_list.py} | 78 +--- .../views/test_preprint_request_list.py | 72 ++++ .../test_user_message_institutional_access.py | 288 +++++++++++++ framework/auth/oauth_scopes.py | 8 +- framework/email/tasks.py | 162 +++++-- .../0025_contributor_is_curator_and_more.py | 67 +++ osf/models/__init__.py | 2 + osf/models/contributor.py | 9 +- osf/models/institution.py | 1 + osf/models/mixins.py | 14 +- osf/models/node.py | 16 +- osf/models/request.py | 17 +- osf/models/user.py | 28 ++ osf/models/user_message.py | 122 ++++++ osf/utils/machines.py | 42 +- osf/utils/workflows.py | 6 + osf_tests/factories.py | 8 + .../test_institutional_admin_contributors.py | 116 +++++ tests/framework_tests/test_email.py | 45 +- website/mails/mails.py | 36 +- website/profile/utils.py | 10 +- website/project/views/contributor.py | 8 + website/static/js/accessRequestManager.js | 5 +- website/static/js/contribManager.js | 2 + website/static/js/project.js | 8 +- ...est_institutional_access_request.html.mako | 35 ++ ...age_institutional_access_request.html.mako | 26 ++ website/templates/project/contributors.mako | 68 ++- 39 files changed, 1931 insertions(+), 207 deletions(-) create mode 100644 api_tests/nodes/views/test_node_contributor_insti_admin.py create mode 100644 api_tests/requests/views/test_node_request_institutional_access.py rename api_tests/requests/views/{test_request_list_create.py => test_node_request_list.py} (58%) create mode 100644 api_tests/requests/views/test_preprint_request_list.py create mode 100644 api_tests/users/views/test_user_message_institutional_access.py create mode 100644 osf/migrations/0025_contributor_is_curator_and_more.py create mode 100644 osf/models/user_message.py create mode 100644 osf_tests/test_institutional_admin_contributors.py create mode 100644 website/templates/emails/node_request_institutional_access_request.html.mako create mode 100644 website/templates/emails/user_message_institutional_access_request.html.mako diff --git a/api/institutions/serializers.py b/api/institutions/serializers.py index 1d1e0761715..0262f9dddd7 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.BooleanField(read_only=True) links = LinksField({ 'self': 'get_api_url', 'html': 'get_absolute_html_url', diff --git a/api/nodes/views.py b/api/nodes/views.py index 93879e2d40a..2dcf7aa541e 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -125,7 +125,7 @@ RegistrationSerializer, RegistrationCreateSerializer, ) -from api.requests.permissions import NodeRequestPermission +from api.requests.permissions import NodeRequestPermission, InstitutionalAdminRequestTypePermission from api.requests.serializers import NodeRequestSerializer, NodeRequestCreateSerializer from api.requests.views import NodeRequestMixin from api.resources import annotations as resource_annotations @@ -2239,6 +2239,7 @@ class NodeRequestListCreate(JSONAPIBaseView, generics.ListCreateAPIView, ListFil drf_permissions.IsAuthenticatedOrReadOnly, base_permissions.TokenHasScope, NodeRequestPermission, + InstitutionalAdminRequestTypePermission, ) required_read_scopes = [CoreScopes.NODE_REQUESTS_READ] diff --git a/api/requests/permissions.py b/api/requests/permissions.py index 09c97f012ba..9e4a240ac7d 100644 --- a/api/requests/permissions.py +++ b/api/requests/permissions.py @@ -1,11 +1,15 @@ -from rest_framework import permissions as drf_permissions +from rest_framework import exceptions, permissions as drf_permissions from api.base.utils import get_user_auth -from osf.models.action import NodeRequestAction, PreprintRequestAction +from osf.models import ( + Node, + NodeRequestAction, + PreprintRequestAction, + Preprint, + Institution, +) from osf.models.mixins import NodeRequestableMixin, PreprintRequestableMixin -from osf.models.node import Node -from osf.models.preprint import Preprint -from osf.utils.workflows import DefaultTriggers +from osf.utils.workflows import DefaultTriggers, NodeRequestTypes from osf.utils import permissions as osf_permissions @@ -32,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) @@ -52,7 +56,35 @@ def has_object_permission(self, request, view, obj): # Requesters may not be contributors # Requesters may edit their comment or submit their request return is_requester and auth.user not in node.contributors - return False + + +class InstitutionalAdminRequestTypePermission(drf_permissions.BasePermission): + """ + Permission class for handling object permissions related to Node requests and actions. + """ + + def has_permission(self, request, view): + # Skip if not institutional_request request_type + request_type = request.data.get('request_type') + if request_type != NodeRequestTypes.INSTITUTIONAL_REQUEST.value: + return True + + institution_id = request.data.get('institution') + if not institution_id: + raise exceptions.ValidationError({'institution': 'Institution is required.'}) + + try: + institution = Institution.objects.get(_id=institution_id) + 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_at(institution): + return True + else: + raise exceptions.PermissionDenied({'institution': 'You do not have permission to perform this action for this institution.'}) class PreprintRequestPermission(drf_permissions.BasePermission): diff --git a/api/requests/serializers.py b/api/requests/serializers.py index 71ef190b50d..3723d8984e8 100644 --- a/api/requests/serializers.py +++ b/api/requests/serializers.py @@ -4,10 +4,24 @@ from api.base.exceptions import Conflict from api.base.utils import absolute_reverse, get_user_auth -from api.base.serializers import JSONAPISerializer, LinksField, VersionedDateTimeField, RelationshipField -from osf.models import NodeRequest, PreprintRequest -from osf.utils.workflows import DefaultStates, RequestTypes +from api.base.serializers import ( + JSONAPISerializer, + LinksField, + VersionedDateTimeField, + RelationshipField, +) +from osf.models import ( + NodeRequest, + PreprintRequest, + Institution, + OSFUser, +) +from osf.utils.workflows import DefaultStates, RequestTypes, NodeRequestTypes from osf.utils import permissions as osf_permissions +from website import settings +from website.mails import send_mail, NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST + +from rest_framework.exceptions import PermissionDenied, ValidationError class RequestSerializer(JSONAPISerializer): @@ -56,6 +70,8 @@ def create(self, validated_data): raise NotImplementedError() class NodeRequestSerializer(RequestSerializer): + request_type = ser.ChoiceField(read_only=True, required=False, choices=NodeRequestTypes.choices()) + class Meta: type_ = 'node-requests' @@ -66,6 +82,13 @@ class Meta: source='target__guids___id', ) + requested_permissions = ser.ChoiceField( + help_text='These are the default permission suggested when the Node admin sees users ' + 'listed in an `Request Access` list.', + choices=osf_permissions.API_CONTRIBUTOR_PERMISSIONS, + required=False, + ) + def get_target_url(self, obj): return absolute_reverse('nodes:node-detail', kwargs={'node_id': obj.target._id, 'version': self.context['request'].parser_context['kwargs']['version']}) @@ -89,40 +112,116 @@ def get_target_url(self, obj): }, ) -class NodeRequestCreateSerializer(NodeRequestSerializer): - request_type = ser.ChoiceField(required=True, choices=RequestTypes.choices()) - def create(self, validated_data): - auth = get_user_auth(self.context['request']) - if not auth.user: - raise exceptions.PermissionDenied +class NodeRequestCreateSerializer(NodeRequestSerializer): + request_type = ser.ChoiceField(read_only=False, required=False, choices=NodeRequestTypes.choices()) + message_recipient = RelationshipField( + help_text='An optional user who will receive an email explaining the nature of the request.', + required=False, + related_view='users:user-detail', + related_view_kwargs={'user_id': ''}, + ) + bcc_sender = ser.BooleanField( + required=False, + default=False, + help_text='If true, BCCs the sender, giving them a copy of the email message they sent.', + ) + reply_to = ser.BooleanField( + default=False, + help_text='Whether to set the sender\'s username as the `Reply-To` header in the email.', + ) + def to_internal_value(self, data): + """ + Retrieves the id value from `RelationshipField` fields + """ + institution_id = data.pop('institution', None) + message_recipient_id = data.pop('message_recipient', None) + data = super().to_internal_value(data) + + if institution_id: + data['institution'] = institution_id + + if message_recipient_id: + data['message_recipient'] = message_recipient_id + return data + + def get_node_and_validate_non_contributor(self, auth): + """ + Ensures request user isn't already a contributor. + """ try: - node = self.context['view'].get_target() + return self.context['view'].get_target() except exceptions.PermissionDenied: node = self.context['view'].get_target(check_object_permissions=False) if auth.user in node.contributors: raise exceptions.PermissionDenied('You cannot request access to a node you contribute to.') raise - comment = validated_data.pop('comment', '') - request_type = validated_data.pop('request_type', None) + def create(self, validated_data) -> NodeRequest: + auth = get_user_auth(self.context['request']) + if not auth.user: + raise exceptions.PermissionDenied - if request_type != RequestTypes.ACCESS.value: - raise exceptions.ValidationError('You must specify a valid request_type.') + node = self.get_node_and_validate_non_contributor(auth) + + request_type = validated_data.get('request_type') + match request_type: + case NodeRequestTypes.ACCESS.value: + return self._create_node_request(node, validated_data) + case NodeRequestTypes.INSTITUTIONAL_REQUEST.value: + return self.make_node_institutional_access_request(node, validated_data) + case _: + raise ValidationError('You must specify a valid request_type.') + + def make_node_institutional_access_request(self, node, validated_data) -> NodeRequest: + sender = self.context['request'].user + node_request = self._create_node_request(node, validated_data) + node_request.is_institutional_request = True + node_request.save() + institution = Institution.objects.get(_id=validated_data['institution']) + recipient = OSFUser.load(validated_data.get('message_recipient')) + + if recipient: + if not recipient.is_affiliated_with_institution(institution): + raise PermissionDenied(f"User {recipient._id} is not affiliated with the institution.") + + if validated_data['comment']: + send_mail( + to_addr=recipient.username, + mail=NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST, + user=recipient, + sender=sender, + bcc_addr=[sender.username] if validated_data['bcc_sender'] else None, + reply_to=sender.username if validated_data['reply_to'] else None, + recipient=recipient, + comment=validated_data['comment'], + institution=institution, + osf_url=settings.DOMAIN, + node=node_request.target, + ) + return node_request + + def _create_node_request(self, node, validated_data) -> NodeRequest: + creator = self.context['request'].user + request_type = validated_data['request_type'] + comment = validated_data.get('comment', '') + requested_permissions = validated_data.get('requested_permissions') try: node_request = NodeRequest.objects.create( target=node, - creator=auth.user, + creator=creator, comment=comment, machine_state=DefaultStates.INITIAL.value, request_type=request_type, + requested_permissions=requested_permissions, ) node_request.save() except IntegrityError: - raise Conflict(f'Users may not have more than one {request_type} request per node.') - node_request.run_submit(auth.user) + raise Conflict(f"Users may not have more than one {request_type} request per node.") + + node_request.run_submit(creator) return node_request class PreprintRequestSerializer(RequestSerializer): diff --git a/api/users/permissions.py b/api/users/permissions.py index a9186a4efd5..a8cc446be10 100644 --- a/api/users/permissions.py +++ b/api/users/permissions.py @@ -1,5 +1,7 @@ -from osf.models import OSFUser -from rest_framework import permissions +from rest_framework import permissions, exceptions + +from osf.models import OSFUser, Institution +from osf.models.user_message import MessageTypes class ReadOnlyOrCurrentUser(permissions.BasePermission): @@ -47,3 +49,36 @@ def has_permission(self, request, view): def has_object_permission(self, request, view, obj): assert isinstance(obj, OSFUser), f'obj must be a User, got {obj}' return not obj.is_registered + + +class UserMessagePermissions(permissions.BasePermission): + """ + Custom permission to allow only institutional admins to create certain types of UserMessages. + """ + def has_permission(self, request, view) -> bool: + """ + Validate if the user has permission to perform the requested action. + Args: + request: The HTTP request. + view: The view handling the request. + Returns: + bool: True if the user has the required permission, False otherwise. + """ + user = request.user + if not user or user.is_anonymous: + return False + + institution_id = request.data.get('institution') + if not institution_id: + raise exceptions.ValidationError({'institution': 'Institution is required.'}) + + try: + institution = Institution.objects.get(_id=institution_id) + except Institution.DoesNotExist: + raise exceptions.ValidationError({'institution': 'Specified institution does not exist.'}) + + message_type = request.data.get('message_type') + if message_type == MessageTypes.INSTITUTIONAL_REQUEST: + return user.is_institutional_admin_at(institution) and institution.institutional_request_access_enabled + else: + raise exceptions.ValidationError('Not valid message type.') diff --git a/api/users/serializers.py b/api/users/serializers.py index 5e8ca59d9cf..2707a4141c0 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -1,3 +1,5 @@ +from typing import Any, Dict + from django.utils import timezone from jsonschema import validate, Draft7Validator, ValidationError as JsonSchemaValidationError from rest_framework import exceptions @@ -19,16 +21,26 @@ JSONAPIListField, ShowIfCurrentUser, ) -from api.base.utils import absolute_reverse, default_node_list_queryset, get_user_auth, is_deprecated, hashids +from api.base.utils import ( + absolute_reverse, + default_node_list_queryset, + get_user_auth, + is_deprecated, + hashids, + get_object_or_error, +) + from api.base.versioning import get_kebab_snake_case_field from api.nodes.serializers import NodeSerializer, RegionRelationshipField from framework.auth.views import send_confirm_email_async from osf.exceptions import ValidationValueError, ValidationError, BlockedEmailError -from osf.models import Email, Node, OSFUser, Preprint, Registration +from osf.models import Email, Node, OSFUser, Preprint, Registration, UserMessage, Institution +from osf.models.user_message import MessageTypes from osf.models.provider import AbstractProviderGroupObjectPermission from osf.utils.requests import string_type_request_headers from website.profile.views import update_osf_help_mails_subscription, update_mailchimp_subscription from website.settings import MAILCHIMP_GENERAL_LIST, OSF_HELP_LIST, CONFIRM_REGISTRATIONS_BY_EMAIL +from website.util import api_v2_url class SocialField(ser.DictField): @@ -657,3 +669,111 @@ def update(self, instance, validated_data): class UserNodeSerializer(NodeSerializer): filterable_fields = NodeSerializer.filterable_fields | {'current_user_permissions'} + + +class UserMessageSerializer(JSONAPISerializer): + """ + Serializer for creating and managing `UserMessage` instances. + + Attributes: + message_text (CharField): The text content of the message. + message_type (ChoiceField): The type of message being sent, restricted to `MessageTypes`. + institution (RelationshipField): The institution related to the message. Required. + user (RelationshipField): The recipient of the message. + """ + id = IDField(read_only=True, source='_id') + message_text = ser.CharField( + required=True, + help_text='The content of the message to be sent.', + ) + message_type = ser.ChoiceField( + choices=MessageTypes.choices, + required=True, + help_text='The type of message being sent. Must match one of the defined `MessageTypes`.', + ) + institution = RelationshipField( + related_view='institutions:institution-detail', + related_view_kwargs={'institution_id': ''}, + help_text='The institution associated with this message.', + ) + bcc_sender = ser.BooleanField( + required=False, + default=False, + help_text='If true, BCCs the sender, giving them a copy of the email message they sent.', + ) + reply_to = ser.BooleanField( + default=False, + help_text='Whether to set the sender\'s username as the `Reply-To` header in the email.', + ) + + def get_absolute_url(self, obj: UserMessage) -> str: + return api_v2_url( + 'users:user-messages', + params={ + 'user_id': self.context['request'].parser_context['kwargs']['user_id'], + 'version': self.context['request'].parser_context['kwargs']['version'], + }, + ) + + def to_internal_value(self, data): + institution_id = data.pop('institution', None) + data = super().to_internal_value(data) + if institution_id: + data['institution'] = institution_id + return data + + class Meta: + type_ = 'user-messages' + + def create(self, validated_data: Dict[str, Any]) -> UserMessage: + """ + Creates a `UserMessage` instance based on validated data. + + Args: + validated_data (Dict[str, Any]): The data validated by the serializer. + + Raises: + ValidationError: If required validations fail (e.g., sender not an institutional admin, + or recipient not affiliated with the institution). + + Returns: + UserMessage: The created `UserMessage` instance. + """ + request = self.context['request'] + sender = request.user + + recipient = get_object_or_error( + OSFUser, + self.context['view'].kwargs['user_id'], + request, + 'user', + ) + + institution_id = validated_data.get('institution') + if not institution_id: + raise exceptions.ValidationError({'institution': 'Institution is required.'}) + + institution = get_object_or_error( + Institution, + institution_id, + request, + 'institution', + ) + + if not sender.is_institutional_admin_at(institution): + raise Conflict({'sender': 'Only institutional administrators can create messages.'}) + + if not recipient.is_affiliated_with_institution(institution): + raise Conflict( + {'user': 'Cannot send to a recipient that is not affiliated with the provided institution.'}, + ) + + return UserMessage.objects.create( + sender=sender, + recipient=recipient, + institution=institution, + message_type=MessageTypes.INSTITUTIONAL_REQUEST, + message_text=validated_data['message_text'], + is_sender_BCCed=validated_data['bcc_sender'], + reply_to=validated_data['reply_to'], + ) diff --git a/api/users/urls.py b/api/users/urls.py index cf9bd0bb7b9..ef53094a121 100644 --- a/api/users/urls.py +++ b/api/users/urls.py @@ -19,6 +19,7 @@ re_path(r'^(?P\w+)/draft_preprints/$', views.UserDraftPreprints.as_view(), name=views.UserDraftPreprints.view_name), re_path(r'^(?P\w+)/registrations/$', views.UserRegistrations.as_view(), name=views.UserRegistrations.view_name), re_path(r'^(?P\w+)/settings/$', views.UserSettings.as_view(), name=views.UserSettings.view_name), + re_path(r'^(?P\w+)/messages/$', views.UserMessageView.as_view(), name=views.UserMessageView.view_name), re_path(r'^(?P\w+)/quickfiles/$', views.UserQuickFiles.as_view(), name=views.UserQuickFiles.view_name), re_path(r'^(?P\w+)/relationships/institutions/$', views.UserInstitutionsRelationship.as_view(), name=views.UserInstitutionsRelationship.view_name), re_path(r'^(?P\w+)/settings/emails/$', views.UserEmailsList.as_view(), name=views.UserEmailsList.view_name), diff --git a/api/users/views.py b/api/users/views.py index 927b5dc2f9b..314969d0c4c 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -7,6 +7,7 @@ from api.addons.views import AddonSettingsMixin from api.base import permissions as base_permissions +from api.users.permissions import UserMessagePermissions from api.base.waffle_decorators import require_flag from api.base.exceptions import Conflict, UserGone, Gone from api.base.filters import ListFilterMixin, PreprintFilterMixin @@ -55,6 +56,7 @@ UserAccountExportSerializer, ReadEmailUserDetailSerializer, UserChangePasswordSerializer, + UserMessageSerializer, ) from django.contrib.auth.models import AnonymousUser from django.http import JsonResponse @@ -957,3 +959,23 @@ def perform_destroy(self, instance): else: user.remove_unconfirmed_email(email) user.save() + + +class UserMessageView(JSONAPIBaseView, generics.CreateAPIView): + """ + List and create UserMessages for a user. + """ + permission_classes = ( + drf_permissions.IsAuthenticated, + base_permissions.TokenHasScope, + UserMessagePermissions, + ) + + required_read_scopes = [CoreScopes.NULL] + required_write_scopes = [CoreScopes.USERS_MESSAGE_WRITE_EMAIL] + parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON) + throttle_classes = [BurstRateThrottle, SendEmailThrottle] + serializer_class = UserMessageSerializer + + view_category = 'users' + view_name = 'user-messages' diff --git a/api_tests/nodes/views/test_node_contributor_insti_admin.py b/api_tests/nodes/views/test_node_contributor_insti_admin.py new file mode 100644 index 00000000000..8e95ddef658 --- /dev/null +++ b/api_tests/nodes/views/test_node_contributor_insti_admin.py @@ -0,0 +1,67 @@ +import pytest +from osf.models import Contributor +from osf_tests.factories import ( + AuthUserFactory, + ProjectFactory, + InstitutionFactory, +) +from api.base.settings.defaults import API_BASE + + +@pytest.mark.django_db +class TestChangeInstitutionalAdminContributor: + + @pytest.fixture() + def project_admin(self): + return AuthUserFactory() + + @pytest.fixture() + def project_admin2(self): + return AuthUserFactory() + + @pytest.fixture() + def institution(self): + return InstitutionFactory() + + @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 project(self, project_admin, project_admin2, institutional_admin): + project = ProjectFactory(creator=project_admin) + project.add_contributor(project_admin2, permissions='admin', visible=False) + project.add_contributor(institutional_admin, visible=False, make_curator=True) + return project + + @pytest.fixture() + def url_contrib(self, project, institutional_admin): + return f'/{API_BASE}nodes/{project._id}/contributors/{institutional_admin._id}/' + + def test_cannot_set_institutional_admin_contributor_bibliographic(self, app, project_admin, project, + institutional_admin, url_contrib): + res = app.put_json_api( + url_contrib, + { + 'data': { + 'id': f'{project._id}-{institutional_admin._id}', + 'type': 'contributors', + 'attributes': { + 'bibliographic': True, + } + } + }, + auth=project_admin.auth, + expect_errors=True + ) + + assert res.status_code == 409 + assert res.json['errors'][0]['detail'] == 'Curators cannot be made bibliographic contributors' + + contributor = Contributor.objects.get( + node=project, + user=institutional_admin + ) + assert not contributor.visible diff --git a/api_tests/registries_moderation/test_submissions.py b/api_tests/registries_moderation/test_submissions.py index a7e0b3dbb6c..532f71d93fe 100644 --- a/api_tests/registries_moderation/test_submissions.py +++ b/api_tests/registries_moderation/test_submissions.py @@ -4,7 +4,7 @@ from api.base.settings.defaults import API_BASE from api.providers.workflows import Workflows -from osf.utils.workflows import RequestTypes, RegistrationModerationTriggers, RegistrationModerationStates +from osf.utils.workflows import NodeRequestTypes, RegistrationModerationTriggers, RegistrationModerationStates from osf_tests.factories import ( @@ -66,7 +66,7 @@ def registration_with_withdraw_request(self, provider): registration = RegistrationFactory(provider=provider) NodeRequest.objects.create( - request_type=RequestTypes.WITHDRAWAL.value, + request_type=NodeRequestTypes.WITHDRAWAL.value, target=registration, creator=registration.creator ) @@ -75,7 +75,7 @@ def registration_with_withdraw_request(self, provider): @pytest.fixture() def access_request(self, provider): - request = NodeRequestFactory(request_type=RequestTypes.ACCESS.value) + request = NodeRequestFactory(request_type=NodeRequestTypes.ACCESS.value) request.target.provider = provider request.target.save() 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..c3fdc4e111b --- /dev/null +++ b/api_tests/requests/views/test_node_request_institutional_access.py @@ -0,0 +1,397 @@ +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(institutional_request_access_enabled=True) + + @pytest.fixture() + def institution_without_access(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_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() + + @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 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 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() + 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_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. + """ + 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, 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 + """ + 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 'Institutional request access is not enabled.' in res.json['errors'][0]['detail'] + + @mock.patch('api.requests.serializers.send_mail') + @mock.patch('osf.utils.machines.mails.send_mail') + def test_email_send_institutional_request_specific_email( + self, + mock_send_mail_machines, + mock_send_mail_serializers, + user_with_affiliation, + app, + project, + url, + create_payload, + institutional_admin, + institution + ): + """ + Test that the institutional request triggers email notifications to appropriate recipients. + """ + # Set up mock behaviors + project.is_public = True + project.save() + + # Perform the action + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + + # Ensure response is successful + assert res.status_code == 201 + + assert mock_send_mail_serializers.call_count == 1 + assert mock_send_mail_machines.call_count == 0 + + # Check calls for osf.utils.machines.mails.send_mail + mock_send_mail_serializers.assert_called_once_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_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_request_list_create.py b/api_tests/requests/views/test_node_request_list.py similarity index 58% rename from api_tests/requests/views/test_request_list_create.py rename to api_tests/requests/views/test_node_request_list.py index fdd96b2cd02..f94d3b4acf2 100644 --- a/api_tests/requests/views/test_request_list_create.py +++ b/api_tests/requests/views/test_node_request_list.py @@ -1,10 +1,12 @@ from unittest import mock import pytest -from osf.utils import workflows from api.base.settings.defaults import API_BASE -from api_tests.requests.mixins import NodeRequestTestMixin, PreprintRequestTestMixin +from api_tests.requests.mixins import NodeRequestTestMixin + from osf_tests.factories import NodeFactory, NodeRequestFactory +from osf.utils.workflows import DefaultStates, NodeRequestTypes + @pytest.mark.django_db class TestNodeRequestListCreate(NodeRequestTestMixin): @@ -18,7 +20,7 @@ def create_payload(self): 'data': { 'attributes': { 'comment': 'ASDFG', - 'request_type': 'access' + 'request_type': NodeRequestTypes.ACCESS.value }, 'type': 'node-requests' } @@ -112,8 +114,8 @@ def test_filter_by_machine_state(self, app, project, noncontrib, url, admin, nod initial_node_request = NodeRequestFactory( creator=noncontrib, target=project, - request_type=workflows.RequestTypes.ACCESS.value, - machine_state=workflows.DefaultStates.INITIAL.value + request_type=NodeRequestTypes.ACCESS.value, + machine_state=DefaultStates.INITIAL.value ) filtered_url = f'{url}?filter[machine_state]=pending' res = app.get(filtered_url, auth=admin.auth) @@ -121,69 +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 TestPreprintRequestListCreate(PreprintRequestTestMixin): - def url(self, preprint): - return f'/{API_BASE}preprints/{preprint._id}/requests/' - - @pytest.fixture() - def create_payload(self): - return { - 'data': { - 'attributes': { - 'comment': 'ASDFG', - 'request_type': 'withdrawal' - }, - 'type': 'preprint-requests' - } - } - - def test_noncontrib_cannot_submit(self, app, noncontrib, create_payload, pre_mod_preprint, post_mod_preprint, none_mod_preprint): - for preprint in [pre_mod_preprint, post_mod_preprint, none_mod_preprint]: - res = app.post_json_api(self.url(preprint), create_payload, auth=noncontrib.auth, expect_errors=True) - assert res.status_code == 403 - - def test_unauth_cannot_submit(self, app, create_payload, pre_mod_preprint, post_mod_preprint, none_mod_preprint): - for preprint in [pre_mod_preprint, post_mod_preprint, none_mod_preprint]: - res = app.post_json_api(self.url(preprint), create_payload, expect_errors=True) - assert res.status_code == 401 - - def test_write_contributor_cannot_submit(self, app, write_contrib, create_payload, pre_mod_preprint, post_mod_preprint, none_mod_preprint): - for preprint in [pre_mod_preprint, post_mod_preprint, none_mod_preprint]: - res = app.post_json_api(self.url(preprint), create_payload, auth=write_contrib.auth, expect_errors=True) - assert res.status_code == 403 - - def test_admin_can_submit(self, app, admin, create_payload, pre_mod_preprint, post_mod_preprint, none_mod_preprint): - for preprint in [pre_mod_preprint, post_mod_preprint, none_mod_preprint]: - res = app.post_json_api(self.url(preprint), create_payload, auth=admin.auth) - assert res.status_code == 201 - - def test_admin_can_view_requests(self, app, admin, pre_request, post_request, none_request): - for request in [pre_request, post_request, none_request]: - res = app.get(self.url(request.target), auth=admin.auth) - assert res.status_code == 200 - assert res.json['data'][0]['id'] == request._id - - def test_noncontrib_and_write_contrib_cannot_view_requests(self, app, noncontrib, write_contrib, pre_request, post_request, none_request): - for request in [pre_request, post_request, none_request]: - for user in [noncontrib, write_contrib]: - res = app.get(self.url(request.target), auth=user.auth, expect_errors=True) - assert res.status_code == 403 - - def test_unauth_cannot_view_requests(self, app, noncontrib, write_contrib, pre_request, post_request, none_request): - for request in [pre_request, post_request, none_request]: - res = app.get(self.url(request.target), expect_errors=True) - assert res.status_code == 401 - - def test_requester_cannot_submit_again(self, app, admin, create_payload, pre_mod_preprint, pre_request): - res = app.post_json_api(self.url(pre_mod_preprint), create_payload, auth=admin.auth, expect_errors=True) - assert res.status_code == 409 - assert res.json['errors'][0]['detail'] == 'Users may not have more than one withdrawal request per preprint.' - - @pytest.mark.skip('TODO: IN-284 -- add emails') - @mock.patch('website.reviews.listeners.mails.send_mail') - def test_email_sent_to_moderators_on_submit(self, mock_mail, app, admin, create_payload, moderator, post_mod_preprint): - res = app.post_json_api(self.url(post_mod_preprint), create_payload, auth=admin.auth) - assert res.status_code == 201 - assert mock_mail.call_count == 1 diff --git a/api_tests/requests/views/test_preprint_request_list.py b/api_tests/requests/views/test_preprint_request_list.py new file mode 100644 index 00000000000..d23736aa312 --- /dev/null +++ b/api_tests/requests/views/test_preprint_request_list.py @@ -0,0 +1,72 @@ +from unittest import mock +import pytest + +from api.base.settings.defaults import API_BASE +from api_tests.requests.mixins import PreprintRequestTestMixin + + +@pytest.mark.django_db +class TestPreprintRequestListCreate(PreprintRequestTestMixin): + def url(self, preprint): + return f'/{API_BASE}preprints/{preprint._id}/requests/' + + @pytest.fixture() + def create_payload(self): + return { + 'data': { + 'attributes': { + 'comment': 'ASDFG', + 'request_type': 'withdrawal' + }, + 'type': 'preprint-requests' + } + } + + def test_noncontrib_cannot_submit(self, app, noncontrib, create_payload, pre_mod_preprint, post_mod_preprint, none_mod_preprint): + for preprint in [pre_mod_preprint, post_mod_preprint, none_mod_preprint]: + res = app.post_json_api(self.url(preprint), create_payload, auth=noncontrib.auth, expect_errors=True) + assert res.status_code == 403 + + def test_unauth_cannot_submit(self, app, create_payload, pre_mod_preprint, post_mod_preprint, none_mod_preprint): + for preprint in [pre_mod_preprint, post_mod_preprint, none_mod_preprint]: + res = app.post_json_api(self.url(preprint), create_payload, expect_errors=True) + assert res.status_code == 401 + + def test_write_contributor_cannot_submit(self, app, write_contrib, create_payload, pre_mod_preprint, post_mod_preprint, none_mod_preprint): + for preprint in [pre_mod_preprint, post_mod_preprint, none_mod_preprint]: + res = app.post_json_api(self.url(preprint), create_payload, auth=write_contrib.auth, expect_errors=True) + assert res.status_code == 403 + + def test_admin_can_submit(self, app, admin, create_payload, pre_mod_preprint, post_mod_preprint, none_mod_preprint): + for preprint in [pre_mod_preprint, post_mod_preprint, none_mod_preprint]: + res = app.post_json_api(self.url(preprint), create_payload, auth=admin.auth) + assert res.status_code == 201 + + def test_admin_can_view_requests(self, app, admin, pre_request, post_request, none_request): + for request in [pre_request, post_request, none_request]: + res = app.get(self.url(request.target), auth=admin.auth) + assert res.status_code == 200 + assert res.json['data'][0]['id'] == request._id + + def test_noncontrib_and_write_contrib_cannot_view_requests(self, app, noncontrib, write_contrib, pre_request, post_request, none_request): + for request in [pre_request, post_request, none_request]: + for user in [noncontrib, write_contrib]: + res = app.get(self.url(request.target), auth=user.auth, expect_errors=True) + assert res.status_code == 403 + + def test_unauth_cannot_view_requests(self, app, noncontrib, write_contrib, pre_request, post_request, none_request): + for request in [pre_request, post_request, none_request]: + res = app.get(self.url(request.target), expect_errors=True) + assert res.status_code == 401 + + def test_requester_cannot_submit_again(self, app, admin, create_payload, pre_mod_preprint, pre_request): + res = app.post_json_api(self.url(pre_mod_preprint), create_payload, auth=admin.auth, expect_errors=True) + assert res.status_code == 409 + assert res.json['errors'][0]['detail'] == 'Users may not have more than one withdrawal request per preprint.' + + @pytest.mark.skip('TODO: IN-284 -- add emails') + @mock.patch('website.reviews.listeners.mails.send_mail') + def test_email_sent_to_moderators_on_submit(self, mock_mail, app, admin, create_payload, moderator, post_mod_preprint): + res = app.post_json_api(self.url(post_mod_preprint), create_payload, auth=admin.auth) + assert res.status_code == 201 + assert mock_mail.call_count == 1 diff --git a/api_tests/users/views/test_user_message_institutional_access.py b/api_tests/users/views/test_user_message_institutional_access.py new file mode 100644 index 00000000000..36f2a59e252 --- /dev/null +++ b/api_tests/users/views/test_user_message_institutional_access.py @@ -0,0 +1,288 @@ +from unittest import mock +import pytest +from osf.models.user_message import MessageTypes, UserMessage +from api.base.settings.defaults import API_BASE +from osf_tests.factories import ( + AuthUserFactory, + InstitutionFactory +) +from website.mails import USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST +from webtest import AppError + + +@pytest.mark.django_db +class TestUserMessageInstitutionalAccess: + """ + Tests for `UserMessage`. + """ + + @pytest.fixture() + def institution(self): + return InstitutionFactory(institutional_request_access_enabled=True) + + @pytest.fixture() + def institution_without_access(self): + return InstitutionFactory() + + @pytest.fixture() + def user(self): + return AuthUserFactory() + + @pytest.fixture() + def noncontrib(self): + return AuthUserFactory() + + @pytest.fixture() + def user_with_affiliation(self, institution): + user = AuthUserFactory() + 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/' + + @pytest.fixture() + def payload(self, institution, user): + return { + 'data': { + 'attributes': { + 'message_text': 'Requesting user access for collaboration', + 'message_type': MessageTypes.INSTITUTIONAL_REQUEST.value, + }, + 'relationships': { + 'institution': { + 'data': {'id': institution._id, 'type': 'institutions'}, + }, + }, + 'type': 'user-message' + } + } + + @mock.patch('osf.models.user_message.send_mail') + def test_institutional_admin_can_create_message(self, mock_send_mail, app, institutional_admin, institution, url_with_affiliation, payload): + """ + Ensure an institutional admin can create a `UserMessage` with a `message` and `institution`. + """ + mock_send_mail.return_value = mock.MagicMock() + + res = app.post_json_api( + url_with_affiliation, + payload, + auth=institutional_admin.auth + ) + assert res.status_code == 201 + data = res.json['data'] + + user_message = UserMessage.objects.get(sender=institutional_admin) + + assert user_message.message_text == payload['data']['attributes']['message_text'] + assert user_message.institution == institution + + mock_send_mail.assert_called_once() + assert mock_send_mail.call_args[1]['to_addr'] == user_message.recipient.username + 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`. + """ + res = app.post_json_api(url_with_affiliation, payload, expect_errors=True) + assert res.status_code == 401 + assert 'Authentication credentials were not provided' in res.json['errors'][0]['detail'] + + def test_non_institutional_admin_cannot_create_message(self, app, noncontrib, user, url_with_affiliation, payload): + """ + Ensure a non-institutional admin cannot create a `UserMessage`, even with valid data. + """ + res = app.post_json_api(url_with_affiliation, payload, auth=noncontrib.auth, expect_errors=True) + assert res.status_code == 403 + + def test_request_without_institution(self, app, institutional_admin, user, url_with_affiliation, payload): + """ + Test that a `UserMessage` can be created without specifying an institution, and `institution` is None. + """ + del payload['data']['relationships']['institution'] + + res = app.post_json_api(url_with_affiliation, payload, auth=institutional_admin.auth, expect_errors=True) + assert res.status_code == 400 + error = res.json['errors'] + assert error == [ + { + 'source': { + 'pointer': '/data/relationships/institution' + }, + 'detail': 'Institution is required.' + } + ] + + def test_missing_message_fails(self, app, institutional_admin, user, url_with_affiliation, payload): + """ + Ensure a `UserMessage` cannot be created without a `message` attribute. + """ + del payload['data']['attributes']['message_text'] + + res = app.post_json_api(url_with_affiliation, payload, auth=institutional_admin.auth, expect_errors=True) + assert res.status_code == 400 + error = res.json['errors'] + assert error == [ + { + 'source': { + 'pointer': '/data/attributes/message_text' + }, + 'detail': 'This field is required.', + } + ] + + def test_admin_cannot_message_user_outside_institution( + self, + app, + institutional_admin, + url_without_affiliation, + payload, + user + ): + """ + Ensure that an institutional admin cannot create a `UserMessage` for a user who is not affiliated with their institution. + """ + res = app.post_json_api(url_without_affiliation, payload, auth=institutional_admin.auth, expect_errors=True) + assert res.status_code == 409 + assert ('Cannot send to a recipient that is not affiliated with the provided institution.' + in res.json['errors'][0]['detail']['user']) + + @mock.patch('osf.models.user_message.send_mail') + def test_cc_institutional_admin( + self, + mock_send_mail, + app, + institutional_admin, + institution, + url_with_affiliation, + user_with_affiliation, + payload + ): + """ + Ensure CC option works as expected, sending messages to all institutional admins except the sender. + """ + mock_send_mail.return_value = mock.MagicMock() + + # Enable CC in the payload + payload['data']['attributes']['bcc_sender'] = True + + # Perform the API request + res = app.post_json_api( + url_with_affiliation, + payload, + auth=institutional_admin.auth, + ) + assert res.status_code == 201 + user_message = UserMessage.objects.get() + + assert user_message.is_sender_BCCed + # Two emails are sent during the CC but this is how the mock works `send_email` is called once. + mock_send_mail.assert_called_once_with( + to_addr=user_with_affiliation.username, + bcc_addr=[institutional_admin.username], + reply_to=None, + message_text='Requesting user access for collaboration', + mail=USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + sender=institutional_admin, + recipient=user_with_affiliation, + institution=institution, + ) + + @mock.patch('osf.models.user_message.send_mail') + def test_cc_field_defaults_to_false(self, mock_send_mail, app, institutional_admin, url_with_affiliation, user_with_affiliation, institution, payload): + """ + Ensure the `cc` field defaults to `false` when not provided in the payload. + """ + res = app.post_json_api(url_with_affiliation, payload, auth=institutional_admin.auth) + assert res.status_code == 201 + + user_message = UserMessage.objects.get(sender=institutional_admin) + assert user_message.message_text == payload['data']['attributes']['message_text'] + mock_send_mail.assert_called_once_with( + to_addr=user_with_affiliation.username, + bcc_addr=None, + reply_to=None, + message_text='Requesting user access for collaboration', + mail=USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + sender=institutional_admin, + recipient=user_with_affiliation, + institution=institution, + ) + + @mock.patch('osf.models.user_message.send_mail') + def test_reply_to_header_set(self, mock_send_mail, app, institutional_admin, user_with_affiliation, institution, url_with_affiliation, payload): + """ + Ensure that the 'Reply-To' header is correctly set to the sender's email address. + """ + payload['data']['attributes']['reply_to'] = True + + res = app.post_json_api( + url_with_affiliation, + payload, + auth=institutional_admin.auth, + ) + assert res.status_code == 201 + + mock_send_mail.assert_called_once_with( + to_addr=user_with_affiliation.username, + bcc_addr=None, + reply_to=institutional_admin.username, + message_text='Requesting user access for collaboration', + mail=USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + sender=institutional_admin, + recipient=user_with_affiliation, + institution=institution, + ) diff --git a/framework/auth/oauth_scopes.py b/framework/auth/oauth_scopes.py index c18084fbb9a..67644050383 100644 --- a/framework/auth/oauth_scopes.py +++ b/framework/auth/oauth_scopes.py @@ -29,6 +29,8 @@ class CoreScopes: USERS_READ = 'users_read' USERS_WRITE = 'users_write' + USERS_MESSAGE_READ_EMAIL = 'users_message_read_email' + USERS_MESSAGE_WRITE_EMAIL = 'users_message_write_email' USERS_CREATE = 'users_create' USER_SETTINGS_READ = 'user.settings_read' @@ -323,7 +325,8 @@ class ComposedScopes: CoreScopes.MEETINGS_READ, CoreScopes.INSTITUTION_READ, CoreScopes.SEARCH, - CoreScopes.SCOPES_READ + CoreScopes.SCOPES_READ, + CoreScopes.USERS_MESSAGE_READ_EMAIL )\ + ( CoreScopes.READ_COLLECTION_SUBMISSION, @@ -342,7 +345,8 @@ class ComposedScopes: + ( CoreScopes.CEDAR_METADATA_RECORD_WRITE, CoreScopes.WRITE_COLLECTION_SUBMISSION_ACTION, - CoreScopes.WRITE_COLLECTION_SUBMISSION + CoreScopes.WRITE_COLLECTION_SUBMISSION, + CoreScopes.USERS_MESSAGE_WRITE_EMAIL ) # Admin permissions- includes functionality not intended for third-party use diff --git a/framework/email/tasks.py b/framework/email/tasks.py index bb2528fd1d3..cf43395222e 100644 --- a/framework/email/tasks.py +++ b/framework/email/tasks.py @@ -5,7 +5,21 @@ from io import BytesIO from sendgrid import SendGridAPIClient -from sendgrid.helpers.mail import Attachment, Mail, FileContent, Category +from sendgrid.helpers.mail import ( + Mail, + Bcc, + ReplyTo, + Category, + Attachment, + FileContent, + Email, + To, + Personalization, + Cc, + FileName, + Disposition, +) + from framework import sentry from framework.celery_tasks import app @@ -20,8 +34,10 @@ def send_email( to_addr: str, subject: str, message: str, + reply_to: bool = False, ttls: bool = True, login: bool = True, + bcc_addr: [] = None, username: str = None, password: str = None, categories=None, @@ -57,6 +73,8 @@ def send_email( categories=categories, attachment_name=attachment_name, attachment_content=attachment_content, + reply_to=reply_to, + bcc_addr=bcc_addr, ) else: return _send_with_smtp( @@ -68,35 +86,58 @@ def send_email( login=login, username=username, password=password, + reply_to=reply_to, + bcc_addr=bcc_addr, ) -def _send_with_smtp(from_addr, to_addr, subject, message, ttls=True, login=True, username=None, password=None): +def _send_with_smtp( + from_addr, + to_addr, + subject, + message, + ttls=True, + login=True, + username=None, + password=None, + bcc_addr=None, + reply_to=None, +): username = username or settings.MAIL_USERNAME password = password or settings.MAIL_PASSWORD if login and (username is None or password is None): logger.error('Mail username and password not set; skipping send.') - return + return False - msg = MIMEText(message, 'html', _charset='utf-8') + msg = MIMEText( + message, + 'html', + _charset='utf-8', + ) msg['Subject'] = subject msg['From'] = from_addr msg['To'] = to_addr - s = smtplib.SMTP(settings.MAIL_SERVER) - s.ehlo() - if ttls: - s.starttls() - s.ehlo() - if login: - s.login(username, password) - s.sendmail( - from_addr=from_addr, - to_addrs=[to_addr], - msg=msg.as_string(), - ) - s.quit() + if reply_to: + msg['Reply-To'] = reply_to + + # Combine recipients for SMTP + recipients = [to_addr] + (bcc_addr or []) + + # Establish SMTP connection and send the email + with smtplib.SMTP(settings.MAIL_SERVER) as server: + server.ehlo() + if ttls: + server.starttls() + server.ehlo() + if login: + server.login(username, password) + server.sendmail( + from_addr=from_addr, + to_addrs=recipients, + msg=msg.as_string() + ) return True @@ -108,39 +149,72 @@ def _send_with_sendgrid( categories=None, attachment_name: str = None, attachment_content=None, + cc_addr=None, + bcc_addr=None, + reply_to=None, client=None, ): - if ( - settings.SENDGRID_WHITELIST_MODE and to_addr in settings.SENDGRID_EMAIL_WHITELIST - ) or settings.SENDGRID_WHITELIST_MODE is False: - client = client or SendGridAPIClient(settings.SENDGRID_API_KEY) - mail = Mail(from_email=from_addr, html_content=message, to_emails=to_addr, subject=subject) - if categories: - mail.category = [Category(x) for x in categories] - if attachment_name and attachment_content: - content_bytes = _content_to_bytes(attachment_content) - content_bytes = FileContent(b64encode(content_bytes).decode()) - attachment = Attachment(file_content=content_bytes, file_name=attachment_name) - mail.add_attachment(attachment) - - response = client.send(mail) - if response.status_code >= 400: - sentry.log_message( - f'{response.status_code} error response from sendgrid.' - f'from_addr: {from_addr}\n' - f'to_addr: {to_addr}\n' - f'subject: {subject}\n' - 'mimetype: html\n' - f'message: {response.body[:30]}\n' - f'categories: {categories}\n' - f'attachment_name: {attachment_name}\n' - ) - return response.status_code < 400 - else: + in_allowed_list = to_addr in settings.SENDGRID_EMAIL_WHITELIST + if settings.SENDGRID_WHITELIST_MODE and not in_allowed_list: sentry.log_message( f'SENDGRID_WHITELIST_MODE is True. Failed to send emails to non-whitelisted recipient {to_addr}.' ) + return False + + client = client or SendGridAPIClient(settings.SENDGRID_API_KEY) + mail = Mail( + from_email=Email(from_addr), + html_content=message, + subject=subject, + ) + + # Personalization to handle To, CC, and BCC sendgrid client concept + personalization = Personalization() + + personalization.add_to(To(to_addr)) + + if cc_addr: + if isinstance(cc_addr, str): + cc_addr = [cc_addr] + for email in cc_addr: + personalization.add_cc(Cc(email)) + + if bcc_addr: + if isinstance(bcc_addr, str): + bcc_addr = [bcc_addr] + for email in bcc_addr: + personalization.add_bcc(Bcc(email)) + + if reply_to: + mail.reply_to = ReplyTo(reply_to) + + mail.add_personalization(personalization) + + if categories: + mail.add_category([Category(x) for x in categories]) + + if attachment_name and attachment_content: + attachment = Attachment( + file_content=FileContent(b64encode(attachment_content).decode()), + file_name=FileName(attachment_name), + disposition=Disposition('attachment') + ) + mail.add_attachment(attachment) + response = client.send(mail) + if response.status_code not in (200, 201, 202): + sentry.log_message( + f'{response.status_code} error response from sendgrid.' + f'from_addr: {from_addr}\n' + f'to_addr: {to_addr}\n' + f'subject: {subject}\n' + 'mimetype: html\n' + f'message: {response.body[:30]}\n' + f'categories: {categories}\n' + f'attachment_name: {attachment_name}\n' + ) + else: + return True def _content_to_bytes(attachment_content: BytesIO | str | bytes) -> bytes: if isinstance(attachment_content, bytes): diff --git a/osf/migrations/0025_contributor_is_curator_and_more.py b/osf/migrations/0025_contributor_is_curator_and_more.py new file mode 100644 index 00000000000..d1ce4bd6432 --- /dev/null +++ b/osf/migrations/0025_contributor_is_curator_and_more.py @@ -0,0 +1,67 @@ +# Generated by Django 4.2.13 on 2025-01-16 20:36 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django_extensions.db.fields +import osf.models.base + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0024_institution_link_to_external_reports_archive'), + ] + + operations = [ + migrations.AddField( + model_name='contributor', + name='is_curator', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='institution', + name='institutional_request_access_enabled', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='noderequest', + name='is_institutional_request', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='noderequest', + name='requested_permissions', + field=models.CharField(blank=True, choices=[('read', 'read'), ('write', 'write'), ('admin', 'admin')], help_text='The permissions being requested for the node (e.g., read, write, admin).', max_length=31, null=True), + ), + migrations.AddField( + model_name='preprintrequest', + name='is_institutional_request', + field=models.BooleanField(default=False), + ), + migrations.AlterField( + model_name='noderequest', + name='request_type', + field=models.CharField(choices=[('access', 'Access'), ('withdrawal', 'Withdrawal'), ('institutional_request', 'Institutional_Request')], help_text='The specific type of node request (e.g., access request).', max_length=31), + ), + migrations.CreateModel( + name='UserMessage', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', django_extensions.db.fields.CreationDateTimeField(auto_now_add=True, verbose_name='created')), + ('modified', django_extensions.db.fields.ModificationDateTimeField(auto_now=True, verbose_name='modified')), + ('_id', models.CharField(db_index=True, default=osf.models.base.generate_object_id, max_length=24, unique=True)), + ('message_text', models.TextField(help_text='The content of the message. The custom text of a formatted email.')), + ('message_type', models.CharField(choices=[('institutional_request', 'INSTITUTIONAL_REQUEST')], help_text='The type of message being sent, as defined in MessageTypes.', max_length=50)), + ('is_sender_BCCed', models.BooleanField(default=False, help_text='The boolean value that indicates whether other institutional admins were BCCed')), + ('reply_to', models.BooleanField(default=False, help_text="Whether to set the sender's username as the `Reply-To` header in the email.")), + ('institution', models.ForeignKey(help_text='The institution associated with this message.', on_delete=django.db.models.deletion.CASCADE, to='osf.institution')), + ('recipient', models.ForeignKey(help_text='The recipient of this message.', on_delete=django.db.models.deletion.CASCADE, related_name='received_user_messages', to=settings.AUTH_USER_MODEL)), + ('sender', models.ForeignKey(help_text='The user who sent this message.', on_delete=django.db.models.deletion.CASCADE, related_name='sent_user_messages', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'abstract': False, + }, + bases=(models.Model, osf.models.base.QuerySetExplainMixin), + ), + ] diff --git a/osf/models/__init__.py b/osf/models/__init__.py index cad31ea323f..4dbcb4d42ff 100644 --- a/osf/models/__init__.py +++ b/osf/models/__init__.py @@ -108,3 +108,5 @@ Email, OSFUser, ) +from .user_message import UserMessage + diff --git a/osf/models/contributor.py b/osf/models/contributor.py index 4c5807f3ee2..a427a7e50f6 100644 --- a/osf/models/contributor.py +++ b/osf/models/contributor.py @@ -1,4 +1,4 @@ -from django.db import models +from django.db import models, IntegrityError from osf.utils.fields import NonNaiveDateTimeField from osf.utils import permissions @@ -30,6 +30,7 @@ def permission(self): class Contributor(AbstractBaseContributor): node = models.ForeignKey('AbstractNode', on_delete=models.CASCADE) + is_curator = models.BooleanField(default=False) @property def _id(self): @@ -41,6 +42,12 @@ class Meta: # NOTE: Adds an _order column order_with_respect_to = 'node' + def save(self, *args, **kwargs): + if self.is_curator and self.visible: + raise IntegrityError('Curators cannot be made bibliographic contributors') + + return super().save(*args, **kwargs) + class PreprintContributor(AbstractBaseContributor): preprint = models.ForeignKey('Preprint', on_delete=models.CASCADE) 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/models/mixins.py b/osf/models/mixins.py index 17c35db04a6..54e945a9f47 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1327,7 +1327,7 @@ def _get_admin_contributors_query(self, users, require_active=True): return qs def add_contributor(self, contributor, permissions=None, visible=True, - send_email=None, auth=None, log=True, save=False): + send_email=None, auth=None, log=True, save=False, make_curator=False): """Add a contributor to the project. :param User contributor: The contributor to be added @@ -1338,6 +1338,7 @@ def add_contributor(self, contributor, permissions=None, visible=True, :param Auth auth: All the auth information including user, API key :param bool log: Add log to self :param bool save: Save after adding contributor + :param bool make_curator incicates whether the user should be an institituional curator :returns: Whether contributor was added """ send_email = send_email or self.contributor_email_template @@ -1393,6 +1394,11 @@ def add_contributor(self, contributor, permissions=None, visible=True, if getattr(self, 'get_identifier_value', None) and self.get_identifier_value('doi'): request, user_id = get_request_and_user_id() self.update_or_enqueue_on_resource_updated(user_id, first_save=False, saved_fields=['contributors']) + + if make_curator: + contributor_obj.is_curator = True + contributor_obj.save() + return contrib_to_add def add_contributors(self, contributors, auth=None, log=True, save=False): @@ -1839,7 +1845,11 @@ def set_visible(self, user, visible, log=True, auth=None, save=False): if visible and not self.contributor_class.objects.filter(**kwargs).exists(): set_visible_kwargs = kwargs set_visible_kwargs['visible'] = False - self.contributor_class.objects.filter(**set_visible_kwargs).update(visible=True) + contribs = self.contributor_class.objects.filter(**set_visible_kwargs) + if self.guardian_object_type == 'node' and contribs.filter(is_curator=True).exists(): + raise ValueError('Curators cannot be made bibliographic contributors') + contribs.update(visible=True) + elif not visible and self.contributor_class.objects.filter(**kwargs).exists(): num_visible_kwargs = self.contributor_kwargs num_visible_kwargs['visible'] = True diff --git a/osf/models/node.py b/osf/models/node.py index 62925966e2e..6aff5126abe 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -15,7 +15,7 @@ from django.core.exceptions import ImproperlyConfigured from django.core.paginator import Paginator from django.urls import reverse -from django.db import models, connection +from django.db import models, connection, IntegrityError from django.db.models.signals import post_save from django.dispatch import receiver from django.utils import timezone @@ -77,6 +77,7 @@ from website.util.metrics import OsfSourceTags, CampaignSourceTags from website.util import api_url_for, api_v2_url, web_url_for from .base import BaseModel, GuidMixin, GuidMixinQuerySet +from api.base.exceptions import Conflict from api.caching.tasks import update_storage_usage from api.caching import settings as cache_settings from api.caching.utils import storage_usage_cache @@ -1079,7 +1080,18 @@ def set_visible(self, user, visible, log=True, auth=None, save=False): if not self.is_contributor(user): raise ValueError(f'User {user} not in contributors') if visible and not Contributor.objects.filter(node=self, user=user, visible=True).exists(): - Contributor.objects.filter(node=self, user=user, visible=False).update(visible=True) + contributor = Contributor.objects.get( + node=self, + user=user, + visible=False + ) + try: + contributor.visible = True + contributor.save() + except IntegrityError as e: + if 'Curators cannot be made bibliographic contributors' in str(e): + raise Conflict(str(e)) from e + raise e elif not visible and Contributor.objects.filter(node=self, user=user, visible=True).exists(): if Contributor.objects.filter(node=self, visible=True).count() == 1: raise ValueError('Must have at least one visible contributor') diff --git a/osf/models/request.py b/osf/models/request.py index d91837cbc7c..cab7469b724 100644 --- a/osf/models/request.py +++ b/osf/models/request.py @@ -1,8 +1,8 @@ from django.db import models - from .base import BaseModel, ObjectIDMixin -from osf.utils.workflows import RequestTypes +from osf.utils.workflows import RequestTypes, NodeRequestTypes from .mixins import NodeRequestableMixin, PreprintRequestableMixin +from osf.utils.permissions import API_CONTRIBUTOR_PERMISSIONS class AbstractRequest(BaseModel, ObjectIDMixin): @@ -12,6 +12,7 @@ class Meta: request_type = models.CharField(max_length=31, choices=RequestTypes.choices()) creator = models.ForeignKey('OSFUser', related_name='submitted_%(class)s', on_delete=models.CASCADE) comment = models.TextField(null=True, blank=True) + is_institutional_request = models.BooleanField(default=False) @property def target(self): @@ -22,6 +23,18 @@ class NodeRequest(AbstractRequest, NodeRequestableMixin): """ Request for Node Access """ target = models.ForeignKey('AbstractNode', related_name='requests', on_delete=models.CASCADE) + request_type = models.CharField( + max_length=31, + choices=NodeRequestTypes.choices(), + help_text='The specific type of node request (e.g., access request).' + ) + requested_permissions = models.CharField( + max_length=31, + choices=((perm.lower(), perm) for perm in API_CONTRIBUTOR_PERMISSIONS), + null=True, + blank=True, + help_text='The permissions being requested for the node (e.g., read, write, admin).' + ) class PreprintRequest(AbstractRequest, PreprintRequestableMixin): diff --git a/osf/models/user.py b/osf/models/user.py index bb0f97f91a9..26cfd93b8dc 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -644,6 +644,34 @@ def osf_groups(self): OSFGroup = apps.get_model('osf.OSFGroup') return get_objects_for_user(self, 'member_group', OSFGroup, with_superuser=False) + def is_institutional_admin_at(self, institution): + """ + Checks if user is admin of a specific institution. + """ + return self.has_perms( + institution.groups['institutional_admins'], + institution + ) + + def is_institutional_admin(self): + """ + Checks if user is admin of any institution. + """ + return self.groups.filter( + name__startswith='institution_', + name__endswith='_institutional_admins' + ).exists() + + def is_institutional_curator(self, node): + """ + Checks if user is user has curator permissions for a node. + """ + return Contributor.objects.filter( + node=node, + user=self, + is_curator=True, + ).exists() + def group_role(self, group): """ For the given OSFGroup, return the user's role - either member or manager diff --git a/osf/models/user_message.py b/osf/models/user_message.py new file mode 100644 index 00000000000..ac77cefe629 --- /dev/null +++ b/osf/models/user_message.py @@ -0,0 +1,122 @@ +from typing import Type +from django.db import models +from django.db.models.signals import post_save +from django.dispatch import receiver + +from .base import BaseModel, ObjectIDMixin +from website.mails import send_mail, USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST + + +class MessageTypes(models.TextChoices): + """ + Enumeration of the different user-to-user message types supported by UserMessage. + + Notes: + Message types should be limited to direct communication between two users. + These may include cases where the sender represents an organization or group, + but they must not involve bulk messaging or group-wide notifications. + """ + # Admin-to-member communication within an institution. + INSTITUTIONAL_REQUEST = ('institutional_request', 'INSTITUTIONAL_REQUEST') + + @classmethod + def get_template(cls: Type['MessageTypes'], message_type: str) -> str: + """ + Retrieve the email template associated with a specific message type. + + Args: + message_type (str): The type of the message. + + Returns: + str: The email template string for the specified message type. + """ + return { + cls.INSTITUTIONAL_REQUEST: USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST + }[message_type] + + +class UserMessage(BaseModel, ObjectIDMixin): + """ + Represents a user-to-user message, potentially sent on behalf of an organization or group. + + Attributes: + sender (OSFUser): The user who initiated the message. + recipient (OSFUser): The intended recipient of the message. + message_text (str): The content of the message being sent. + message_type (str): The type of message, e.g., 'institutional_request'. + institution (Institution): The institution linked to the message, if applicable. + """ + sender = models.ForeignKey( + 'OSFUser', + on_delete=models.CASCADE, + related_name='sent_user_messages', + help_text='The user who sent this message.' + ) + recipient = models.ForeignKey( + 'OSFUser', + on_delete=models.CASCADE, + related_name='received_user_messages', + help_text='The recipient of this message.' + ) + message_text = models.TextField( + help_text='The content of the message. The custom text of a formatted email.' + ) + message_type = models.CharField( + max_length=50, + choices=MessageTypes.choices, + help_text='The type of message being sent, as defined in MessageTypes.' + ) + institution = models.ForeignKey( + 'Institution', + on_delete=models.CASCADE, + help_text='The institution associated with this message.' + ) + is_sender_BCCed = models.BooleanField( + default=False, + help_text='The boolean value that indicates whether other institutional admins were BCCed', + ) + reply_to = models.BooleanField( + default=False, + help_text='Whether to set the sender\'s username as the `Reply-To` header in the email.' + ) + + def send_institution_request(self) -> None: + """ + Sends an institutional access request email to the recipient of the message. + """ + send_mail( + mail=MessageTypes.get_template(self.message_type), + to_addr=self.recipient.username, + bcc_addr=[self.sender.username] if self.is_sender_BCCed else None, + reply_to=self.sender.username if self.reply_to else None, + user=self.recipient, + **{ + 'sender': self.sender, + 'recipient': self.recipient, + 'message_text': self.message_text, + 'institution': self.institution, + }, + ) + + +@receiver(post_save, sender=UserMessage) +def user_message_created(sender: Type[UserMessage], instance: UserMessage, created: bool, **kwargs) -> None: + """ + Signal handler executed after a UserMessage instance is saved. + + Args: + sender (Type[UserMessage]): The UserMessage model class. + instance (UserMessage): The newly created instance of the UserMessage. + created (bool): Whether this is the first save of the instance. + + Notes: + If the message type is 'INSTITUTIONAL_REQUEST', it triggers sending an + institutional request email. Raises an error for unsupported message types. + """ + if not created: + return # Ignore subsequent saves. + + if instance.message_type == MessageTypes.INSTITUTIONAL_REQUEST: + instance.send_institution_request() + else: + raise NotImplementedError(f'Unsupported message type: {instance.message_type}') diff --git a/osf/utils/machines.py b/osf/utils/machines.py index 8e4b8f07338..ac63b1b7894 100644 --- a/osf/utils/machines.py +++ b/osf/utils/machines.py @@ -1,4 +1,5 @@ from django.utils import timezone +from django.db import IntegrityError from transitions import Machine, MachineError from api.providers.workflows import Workflows @@ -7,7 +8,6 @@ from osf.exceptions import InvalidTransitionError from osf.models.preprintlog import PreprintLog from osf.models.action import ReviewAction, NodeRequestAction, PreprintRequestAction - from osf.utils import permissions from osf.utils.workflows import ( DefaultStates, @@ -19,6 +19,7 @@ APPROVAL_TRANSITIONS, CollectionSubmissionStates, COLLECTION_SUBMISSION_TRANSITIONS, + NodeRequestTypes ) from website.mails import mails from website.reviews import signals as reviews_signals @@ -26,6 +27,8 @@ from osf.utils import notifications as notify +from api.base.exceptions import Conflict + class BaseMachine(Machine): action = None @@ -199,12 +202,19 @@ def save_changes(self, ev): if ev.event.name == DefaultTriggers.ACCEPT.value: if not self.machineable.target.is_contributor(self.machineable.creator): contributor_permissions = ev.kwargs.get('permissions', permissions.READ) - self.machineable.target.add_contributor( - self.machineable.creator, - auth=Auth(ev.kwargs['user']), - permissions=contributor_permissions, - visible=ev.kwargs.get('visible', True), - send_email=f'{self.machineable.request_type}_request') + try: + self.machineable.target.add_contributor( + self.machineable.creator, + auth=Auth(ev.kwargs['user']), + permissions=contributor_permissions, + visible=ev.kwargs.get('visible', True), + send_email=f'{self.machineable.request_type}_request', + make_curator=self.machineable.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value, + ) + except IntegrityError as e: + if 'Curators cannot be made bibliographic contributors' in str(e): + raise Conflict(str(e)) from e + raise e def resubmission_allowed(self, ev): # TODO: [PRODUCT-395] @@ -216,15 +226,15 @@ def notify_submit(self, ev): context = self.get_context() context['contributors_url'] = f'{self.machineable.target.absolute_url}contributors/' context['project_settings_url'] = f'{self.machineable.target.absolute_url}settings/' - - for admin in self.machineable.target.get_users_with_perm(permissions.ADMIN): - mails.send_mail( - admin.username, - mails.ACCESS_REQUEST_SUBMITTED, - admin=admin, - osf_contact_email=OSF_CONTACT_EMAIL, - **context - ) + if not self.machineable.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value: + for admin in self.machineable.target.get_users_with_perm(permissions.ADMIN): + mails.send_mail( + admin.username, + mails.ACCESS_REQUEST_SUBMITTED, + admin=admin, + osf_contact_email=OSF_CONTACT_EMAIL, + **context + ) def notify_resubmit(self, ev): """ Notify admins that someone is requesting access again diff --git a/osf/utils/workflows.py b/osf/utils/workflows.py index 43d21659bde..b054de25452 100644 --- a/osf/utils/workflows.py +++ b/osf/utils/workflows.py @@ -515,3 +515,9 @@ def db_name(self): class RequestTypes(ChoiceEnum): ACCESS = 'access' WITHDRAWAL = 'withdrawal' + +@unique +class NodeRequestTypes(ChoiceEnum): + ACCESS = 'access' + WITHDRAWAL = 'withdrawal' + INSTITUTIONAL_REQUEST = 'institutional_request' diff --git a/osf_tests/factories.py b/osf_tests/factories.py index 0bd1664977d..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 @@ -1021,6 +1022,13 @@ class Meta: comment = factory.Faker('text') + +class UserMessageFactory(DjangoModelFactory): + class Meta: + model = models.UserMessage + + comment = factory.Faker('text') + osfstorage_settings = apps.get_app_config('addons_osfstorage') diff --git a/osf_tests/test_institutional_admin_contributors.py b/osf_tests/test_institutional_admin_contributors.py new file mode 100644 index 00000000000..7cfd9044ce2 --- /dev/null +++ b/osf_tests/test_institutional_admin_contributors.py @@ -0,0 +1,116 @@ +import pytest + +from osf.models import Contributor +from osf_tests.factories import ( + AuthUserFactory, + ProjectFactory, + InstitutionFactory, + NodeRequestFactory +) +from django.db.utils import IntegrityError + +@pytest.mark.django_db +class TestContributorModel: + + @pytest.fixture() + def user(self): + return AuthUserFactory() + + @pytest.fixture() + def user_with_institutional_request(self, project): + user = AuthUserFactory() + NodeRequestFactory( + target=project, + creator=user, + is_institutional_request=True, + ) + return user + + @pytest.fixture() + def user_with_non_institutional_request(self, project): + user = AuthUserFactory() + NodeRequestFactory( + target=project, + creator=user, + is_institutional_request=False, + ) + return user + + @pytest.fixture() + def project(self): + return ProjectFactory() + + @pytest.fixture() + def institution(self): + return InstitutionFactory() + + @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 curator(self, institutional_admin, project): + return Contributor( + user=institutional_admin, + node=project, + visible=False, + is_curator=True + ) + + def test_contributor_with_visible_and_pending_request_raises_error(self, user_with_institutional_request, project, institution): + user_with_institutional_request.save() + user_with_institutional_request.visible = True + user_with_institutional_request.refresh_from_db() + assert user_with_institutional_request.visible + + try: + project.add_contributor(user_with_institutional_request, make_curator=True) + except IntegrityError as e: + assert e.args == ('Curators cannot be made bibliographic contributors',) + + def test_contributor_with_visible_and_valid_request(self, user_with_non_institutional_request, project, institution): + user_with_non_institutional_request.save() + user_with_non_institutional_request.visible = True + user_with_non_institutional_request.save() + + user_with_non_institutional_request.refresh_from_db() + assert user_with_non_institutional_request.visible + + def test_contributor_with_visible_and_institutional_admin_raises_error(self, curator, project, institution): + curator.save() + curator.visible = True + with pytest.raises(IntegrityError, match='Curators cannot be made bibliographic contributors'): + curator.save() + + assert curator.visible + curator.refresh_from_db() + assert not curator.visible + + # save completes when valid + curator.visible = False + curator.save() + + def test_regular_visible_contributor_is_saved(self, user, project): + contributor = Contributor( + user=user, + node=project, + visible=True, + is_curator=False + ) + contributor.save() + saved_contributor = Contributor.objects.get(pk=contributor.pk) + assert saved_contributor.user == user + assert saved_contributor.node == project + assert saved_contributor.visible is True + assert saved_contributor.is_curator is False + + def test_invisible_curator_is_saved(self, institutional_admin, curator, project): + curator.save() + saved_curator = Contributor.objects.get(pk=curator.pk) + assert curator == saved_curator + assert saved_curator.user == institutional_admin + assert saved_curator.node == project + assert saved_curator.visible is False + assert saved_curator.is_curator is True diff --git a/tests/framework_tests/test_email.py b/tests/framework_tests/test_email.py index 1dde2ada9ad..c19596b7ed8 100644 --- a/tests/framework_tests/test_email.py +++ b/tests/framework_tests/test_email.py @@ -6,7 +6,7 @@ import sendgrid from sendgrid import SendGridAPIClient -from sendgrid.helpers.mail import Category +from sendgrid.helpers.mail import Mail, Email, To, Category from framework.email.tasks import send_email, _send_with_sendgrid from website import settings @@ -55,17 +55,26 @@ def test_send_with_sendgrid_success(self, mock_mail: MagicMock): categories=(category1, category2) ) assert ret - mock_mail.assert_called_once_with( - from_email=from_addr, - to_emails=to_addr, - subject=subject, - html_content=message, - ) - assert len(mock_mail.return_value.category) == 2 - assert mock_mail.return_value.category[0].get() == category1 - assert mock_mail.return_value.category[1].get() == category2 - mock_client.send.assert_called_once_with(mock_mail.return_value) + # Check Mail object arguments + mock_mail.assert_called_once() + kwargs = mock_mail.call_args.kwargs + assert kwargs['from_email'].email == from_addr + assert kwargs['subject'] == subject + assert kwargs['html_content'] == message + + mock_mail.return_value.add_personalization.assert_called_once() + + # Capture the categories added via add_category + mock_mail.return_value.add_category.assert_called_once() + added_categories = mock_mail.return_value.add_category.call_args.args[0] + assert len(added_categories) == 2 + assert isinstance(added_categories[0], Category) + assert isinstance(added_categories[1], Category) + assert added_categories[0].get() == category1 + assert added_categories[1].get() == category2 + + mock_client.send.assert_called_once_with(mock_mail.return_value) @mock.patch(f'{_send_with_sendgrid.__module__}.sentry.log_message', autospec=True) @mock.patch(f'{_send_with_sendgrid.__module__}.Mail', autospec=True) @@ -84,12 +93,14 @@ def test_send_with_sendgrid_failure_returns_false(self, mock_mail, sentry_mock): ) assert not ret sentry_mock.assert_called_once() - mock_mail.assert_called_once_with( - from_email=from_addr, - to_emails=to_addr, - subject=subject, - html_content=message, - ) + + # Check Mail object arguments + mock_mail.assert_called_once() + kwargs = mock_mail.call_args.kwargs + assert kwargs['from_email'].email == from_addr + assert kwargs['subject'] == subject + assert kwargs['html_content'] == message + mock_client.send.assert_called_once_with(mock_mail.return_value) diff --git a/website/mails/mails.py b/website/mails/mails.py index afca9e78f03..1a9d675c06d 100644 --- a/website/mails/mails.py +++ b/website/mails/mails.py @@ -76,17 +76,29 @@ def render_message(tpl_name, **context): def send_mail( - to_addr, mail, from_addr=None, mailer=None, celery=True, - username=None, password=None, callback=None, attachment_name=None, - attachment_content=None, **context): - """Send an email from the OSF. - Example: :: - + to_addr, + mail, + from_addr=None, + bcc_addr=None, + reply_to=None, + mailer=None, + celery=True, + username=None, + password=None, + callback=None, + attachment_name=None, + attachment_content=None, + **context): + """ + Send an email from the OSF. + Example: from website import mails mails.send_email('foo@bar.com', mails.TEST, name="Foo") :param str to_addr: The recipient's email address + :param str bcc_addr: The BCC senders's email address (or list of addresses) + :param str reply_to: The sender's email address will appear in the reply-to header :param Mail mail: The mail object :param str mimetype: Either 'plain' or 'html' :param function callback: celery task to execute after send_mail completes @@ -119,6 +131,8 @@ def send_mail( categories=mail.categories, attachment_name=attachment_name, attachment_content=attachment_content, + bcc_addr=bcc_addr, + reply_to=reply_to, ) logger.debug('Preparing to send...') @@ -595,3 +609,13 @@ def get_english_article(word): 'addons_boa_job_failure', subject='Your Boa job has failed' ) + +NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST = Mail( + 'node_request_institutional_access_request', + subject='Institutional Access Project Request' +) + +USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST = Mail( + 'user_message_institutional_access_request', + subject='Message from Institutional Admin' +) diff --git a/website/profile/utils.py b/website/profile/utils.py index 3ccff69a164..5b1e22e8916 100644 --- a/website/profile/utils.py +++ b/website/profile/utils.py @@ -51,7 +51,8 @@ def serialize_user(user, node=None, admin=False, full=False, is_profile=False, i is_contributor_obj = isinstance(contrib, Contributor) flags = { 'visible': contrib.visible if is_contributor_obj else node.contributor_set.filter(user=user, visible=True).exists(), - 'permission': contrib.permission if is_contributor_obj else None + 'permission': contrib.permission if is_contributor_obj else None, + 'is_curator': contrib.is_curator, } ret.update(flags) if user.is_registered: @@ -195,10 +196,15 @@ def serialize_access_requests(node): return [ { 'user': serialize_user(access_request.creator), + 'is_institutional_request': access_request.is_institutional_request, 'comment': access_request.comment, + 'requested_permissions': access_request.requested_permissions, 'id': access_request._id } for access_request in node.requests.filter( - request_type=workflows.RequestTypes.ACCESS.value, + request_type__in=[ + workflows.NodeRequestTypes.ACCESS.value, + workflows.NodeRequestTypes.INSTITUTIONAL_REQUEST.value + ], machine_state=workflows.DefaultStates.PENDING.value ).select_related('creator') ] diff --git a/website/project/views/contributor.py b/website/project/views/contributor.py index 56b6802d2a6..0b36b494c34 100644 --- a/website/project/views/contributor.py +++ b/website/project/views/contributor.py @@ -3,6 +3,7 @@ from flask import request from django.core.exceptions import ValidationError from django.utils import timezone +from django.db import IntegrityError from framework import forms, status from framework.auth import cas @@ -287,6 +288,13 @@ def project_manage_contributors(auth, node, **kwargs): node.manage_contributors(contributors, auth=auth, save=True) except (ValueError, NodeStateError) as error: raise HTTPError(http_status.HTTP_400_BAD_REQUEST, data={'message_long': error.args[0]}) + except IntegrityError as error: + status.push_status_message( + 'You can not make an institutional curator a bibliographic contributor.', + kind='error', + trust=False + ) + raise HTTPError(http_status.HTTP_409_CONFLICT, data={'message_long': error.args[0]}) # If user has removed herself from project, alert; redirect to # node summary if node is public, else to user's dashboard page diff --git a/website/static/js/accessRequestManager.js b/website/static/js/accessRequestManager.js index 04616df41a8..862f86ca597 100644 --- a/website/static/js/accessRequestManager.js +++ b/website/static/js/accessRequestManager.js @@ -22,8 +22,9 @@ var AccessRequestModel = function(accessRequest, pageOwner, isRegistration, isPa self.permissionText = ko.observable(self.options.permissionMap[self.permission()]); - self.visible = ko.observable(true); - + self.is_curator = ko.observable(accessRequest.user.is_curator || false); + self.is_institutional_request = ko.observable(accessRequest.is_institutional_request || false); + self.visible = ko.observable(!accessRequest.is_institutional_request); self.pageOwner = pageOwner; self.expanded = ko.observable(false); diff --git a/website/static/js/contribManager.js b/website/static/js/contribManager.js index a93174c07cd..b0537a1d9ef 100644 --- a/website/static/js/contribManager.js +++ b/website/static/js/contribManager.js @@ -59,6 +59,8 @@ var ContributorModel = function(contributor, currentUserCanEdit, pageOwner, isRe self.permission = ko.observable(contributor.permission); + self.is_curator = ko.observable(contributor.is_curator || false); + self.permissionText = ko.observable(self.options.permissionMap[self.permission()]); self.visible = ko.observable(contributor.visible); diff --git a/website/static/js/project.js b/website/static/js/project.js index b32cc77592c..91453024c1b 100644 --- a/website/static/js/project.js +++ b/website/static/js/project.js @@ -217,12 +217,18 @@ $(document).ready(function() { 'in the Contributors list and in project citations. Non-bibliographic contributors ' + 'can read and modify the project as normal.'; - $('.visibility-info').attr( + $('.visibility-info-contrib').attr( 'data-content', bibliographicContribInfoHtml ).popover({ trigger: 'hover' }); + $('.visibility-info-curator').attr( + 'data-content', 'An administrator designated by your affiliated institution to curate your project.' + ).popover({ + trigger: 'hover' + }); + //////////////////// // Event Handlers // //////////////////// diff --git a/website/templates/emails/node_request_institutional_access_request.html.mako b/website/templates/emails/node_request_institutional_access_request.html.mako new file mode 100644 index 00000000000..d49b2811ea1 --- /dev/null +++ b/website/templates/emails/node_request_institutional_access_request.html.mako @@ -0,0 +1,35 @@ +<%inherit file="notify_base.mako" /> + +<%def name="content()"> + + + <%!from website import settings%> + Hello ${recipient.fullname}, +

+ ${sender.fullname} has requested access to ${node.title}. +

+ % if comment: +

+ ${comment} +

+ % endif +

+ To review the request, click here to allow or deny access and configure permissions. +

+

+ This request is being sent to you because your project has the “Request Access” feature enabled. + This allows potential collaborators to request to be added to your project or to disable this feature, click + here +

+ +

+ Want more information? Visit ${settings.DOMAIN} to learn about OSF, or + https://cos.io/ for information about its supporting organization, the Center + for Open Science. +

+

+ Questions? Email ${settings.OSF_CONTACT_EMAIL} +

+ + + diff --git a/website/templates/emails/user_message_institutional_access_request.html.mako b/website/templates/emails/user_message_institutional_access_request.html.mako new file mode 100644 index 00000000000..1e314f91e4e --- /dev/null +++ b/website/templates/emails/user_message_institutional_access_request.html.mako @@ -0,0 +1,26 @@ +<%inherit file="notify_base.mako" /> + +<%def name="content()"> + + + <%!from website import settings%> + Hello ${recipient.fullname}, +

+ This message is coming from an Institutional administrator within your Institution. +

+ % if message_text: +

+ ${message_text} +

+ % endif +

+ Want more information? Visit ${settings.DOMAIN} to learn about OSF, or + https://cos.io/ for information about its supporting organization, the Center + for Open Science. +

+

+ Questions? Email ${settings.OSF_CONTACT_EMAIL} +

+ + + diff --git a/website/templates/project/contributors.mako b/website/templates/project/contributors.mako index 65954d3aced..c3284ebb99d 100644 --- a/website/templates/project/contributors.mako +++ b/website/templates/project/contributors.mako @@ -188,7 +188,7 @@ Bibliographic Contributor - + + Curator + + @@ -242,6 +252,17 @@ data-html="true" > + + Curator + + + @@ -307,12 +328,30 @@ -
-
+
+
+
+ +
+
+
+ type='checkbox' + aria-label='Curator Disabled Bibliographic User Checkbox' + style='background-color: lightgray;' + disabled + > +
+ + +
+ +
+
+
@@ -356,7 +395,7 @@
@@ -366,11 +405,26 @@
+
+
+ +
+ + +
+
+
+ +
+
+ +
+