diff --git a/api/requests/permissions.py b/api/requests/permissions.py index 6811323c058..77c7a76a7d3 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_or_curator(institution): return True else: raise exceptions.PermissionDenied({'institution': 'You do not have permission to perform this action for this institution.'}) diff --git a/api/users/permissions.py b/api/users/permissions.py index 00510ec8b30..eff3029095b 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_or_curator(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..b733909cdda 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_or_curator(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..80e4a3b9ec7 --- /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) + 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 93% rename from osf/migrations/0025_noderequest_requested_permissions_and_more.py rename to osf/migrations/0025_contributor_is_curator_and_more.py index d936e1393e7..a6702e32946 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 @@ -14,6 +14,11 @@ class Migration(migrations.Migration): ] operations = [ + migrations.AddField( + model_name='contributor', + name='is_curator', + field=models.BooleanField(default=False), + ), migrations.AddField( model_name='institution', name='institutional_request_access_enabled', diff --git a/osf/models/contributor.py b/osf/models/contributor.py index 4c5807f3ee2..70384527885 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,14 @@ class Meta: # NOTE: Adds an _order column order_with_respect_to = 'node' + def save(self, *args, **kwargs): + if not self.user.is_institutional_admin_or_curator(): + return super().save(*args, **kwargs) + elif self.visible: + raise IntegrityError('Curators cannot be made bibliographic contributors') + else: + 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..ed64ebe6457 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 @@ -1393,6 +1393,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): 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/user.py b/osf/models/user.py index 411381b0687..41ed77bb5aa 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -644,7 +644,23 @@ 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): + def is_institutional_admin_or_curator(self, institution=None, node=None): + """ + Checks if user is admin of any or of a specific institution, or and curator on a specific node. + """ + if node: + return Contributor.objects.filter( + node=node, + user=self, + is_curator=True, + ).exists() + + if not institution: + return self.groups.filter( + name__startswith='institution_', + name__endswith='_institutional_admins' + ).exists() + group_name = institution.format_group('institutional_admins') return self.groups.filter(name=group_name).exists() 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..ccb9d79cb1a --- /dev/null +++ b/osf_tests/test_institutional_admin_contributors.py @@ -0,0 +1,42 @@ +import pytest +from osf.models import Contributor +from osf_tests.factories import ( + AuthUserFactory, + ProjectFactory, + InstitutionFactory +) +from django.db.utils import IntegrityError + +@pytest.mark.django_db +class TestContributorModel: + + @pytest.fixture() + def user(self): + return AuthUserFactory() + + @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 + + def test_contributor_with_visible_and_institutional_admin_raises_error(self, institutional_admin, project, institution): + contributor = Contributor( + user=institutional_admin, + node=project, + visible=False, + ) + contributor.save() + + contributor.visible = True + + with pytest.raises(IntegrityError, match='Curators cannot be made bibliographic contributors'): + contributor.save() diff --git a/website/profile/utils.py b/website/profile/utils.py index e24ca9b1ed9..a8d4cb1637c 100644 --- a/website/profile/utils.py +++ b/website/profile/utils.py @@ -15,7 +15,7 @@ def get_profile_image_url(user, size=settings.PROFILE_IMAGE_MEDIUM): use_ssl=True, size=size) -def serialize_user(user, node=None, admin=False, full=False, is_profile=False, include_node_counts=False): +def serialize_user(user, node=None, admin=False, full=False, is_profile=False, include_node_counts=False, is_expected_contrib=True): """ Return a dictionary representation of a registered user. @@ -33,10 +33,11 @@ def serialize_user(user, node=None, admin=False, full=False, is_profile=False, i 'surname': user.family_name, 'fullname': fullname, 'shortname': fullname if len(fullname) < 50 else fullname[:23] + '...' + fullname[-23:], + 'is_curator': user.is_institutional_admin_or_curator(node=node), 'profile_image_url': user.profile_image_url(size=settings.PROFILE_IMAGE_MEDIUM), 'active': user.is_active, } - if node is not None: + if node is not None and is_expected_contrib: if admin: flags = { 'visible': False, @@ -194,12 +195,15 @@ def serialize_access_requests(node): """Serialize access requests for a node""" return [ { - 'user': serialize_user(access_request.creator), + 'user': serialize_user(access_request.creator, is_expected_contrib=False), '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..8bd78ce46ad 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 admin 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..c8fc25f817c 100644 --- a/website/static/js/accessRequestManager.js +++ b/website/static/js/accessRequestManager.js @@ -22,8 +22,8 @@ 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.visible = ko.observable(!accessRequest.user.is_curator); 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..1c84b3b8be1 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,27 @@ -
-
+
+
+
+ +
+
+
+ type='checkbox' + aria-label='Curator Disabled Bibliographic User Checkbox' + style='background-color: lightgray;' + disabled + > +
+ + +
+
@@ -366,10 +402,28 @@
+
+
+
+ +
+ + +
+
+
+ +
+
+ +