diff --git a/api/requests/permissions.py b/api/requests/permissions.py index 6811323c058..9e4a240ac7d 100644 --- a/api/requests/permissions.py +++ b/api/requests/permissions.py @@ -81,7 +81,7 @@ def has_permission(self, request, view): if not institution.institutional_request_access_enabled: raise exceptions.PermissionDenied({'institution': 'Institutional request access is not enabled.'}) - if get_user_auth(request).user.is_institutional_admin(institution): + 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.'}) diff --git a/api/requests/serializers.py b/api/requests/serializers.py index ab99d36ba1e..8ad450936c0 100644 --- a/api/requests/serializers.py +++ b/api/requests/serializers.py @@ -177,6 +177,8 @@ def create(self, validated_data) -> NodeRequest: 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')) diff --git a/api/users/permissions.py b/api/users/permissions.py index 00510ec8b30..a8cc446be10 100644 --- a/api/users/permissions.py +++ b/api/users/permissions.py @@ -79,6 +79,6 @@ def has_permission(self, request, view) -> bool: message_type = request.data.get('message_type') if message_type == MessageTypes.INSTITUTIONAL_REQUEST: - return user.is_institutional_admin(institution) and institution.institutional_request_access_enabled + 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 c1b8f49fc89..a3bd4d1cdb4 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -760,7 +760,7 @@ def create(self, validated_data: Dict[str, Any]) -> UserMessage: 'institution', ) - if not sender.is_institutional_admin(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): 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/osf/migrations/0025_noderequest_requested_permissions_and_more.py b/osf/migrations/0025_contributor_is_curator_and_more.py similarity index 98% rename from osf/migrations/0025_noderequest_requested_permissions_and_more.py rename to osf/migrations/0025_contributor_is_curator_and_more.py index d936e1393e7..0d72e062b99 100644 --- a/osf/migrations/0025_noderequest_requested_permissions_and_more.py +++ b/osf/migrations/0025_contributor_is_curator_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.15 on 2025-01-08 12:24 +# Generated by Django 4.2.13 on 2025-01-09 19:19 from django.conf import settings from django.db import migrations, models diff --git a/osf/migrations/0026_contributor_is_curator_and_more.py b/osf/migrations/0026_contributor_is_curator_and_more.py new file mode 100644 index 00000000000..84dbbb32bf1 --- /dev/null +++ b/osf/migrations/0026_contributor_is_curator_and_more.py @@ -0,0 +1,28 @@ +# Generated by Django 4.2.13 on 2025-01-10 19:27 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0025_contributor_is_curator_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='contributor', + name='is_curator', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='noderequest', + name='is_institutional_request', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='preprintrequest', + name='is_institutional_request', + field=models.BooleanField(default=False), + ), + ] 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/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 ed50c29f226..cab7469b724 100644 --- a/osf/models/request.py +++ b/osf/models/request.py @@ -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): diff --git a/osf/models/user.py b/osf/models/user.py index 411381b0687..26cfd93b8dc 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -644,9 +644,33 @@ 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(self, institution): - group_name = institution.format_group('institutional_admins') - return self.groups.filter(name=group_name).exists() + 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): """ diff --git a/osf/utils/machines.py b/osf/utils/machines.py index 8e4b8f07338..9687e19749d 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] 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/website/profile/utils.py b/website/profile/utils.py index e24ca9b1ed9..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,11 +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/project/contributors.mako b/website/templates/project/contributors.mako index c194884d7c8..c3284ebb99d 100644 --- a/website/templates/project/contributors.mako +++ b/website/templates/project/contributors.mako @@ -188,7 +188,7 @@