Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENG-6668] Add Contributor Page Improvements for Institutional Access #10825

2 changes: 1 addition & 1 deletion api/requests/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.'})
Expand Down
2 changes: 1 addition & 1 deletion api/users/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.')
2 changes: 1 addition & 1 deletion api/users/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
67 changes: 67 additions & 0 deletions api_tests/nodes/views/test_node_contributor_insti_admin.py
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Expand Down
18 changes: 18 additions & 0 deletions osf/migrations/0026_contributor_is_curator.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 4.2.13 on 2025-01-09 21:22

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),
),
]
10 changes: 9 additions & 1 deletion osf/models/contributor.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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):
Expand All @@ -41,6 +42,13 @@ class Meta:
# NOTE: Adds an _order column
order_with_respect_to = 'node'

def save(self, *args, **kwargs):
if self.user.is_institutional_admin():
if 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)
Expand Down
7 changes: 6 additions & 1 deletion osf/models/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
16 changes: 14 additions & 2 deletions osf/models/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

@Johnetordoff Johnetordoff Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overwriting save doesn't always cut it, but this looks like that only place we .update visibility, arguably we want to retain the ability to change the visibility which leans away from placing in DB constraints. However I think this is all visibility updates in the existing code.

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')
Expand Down
30 changes: 27 additions & 3 deletions osf/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down
24 changes: 17 additions & 7 deletions osf/utils/machines.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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,
Expand All @@ -19,13 +19,16 @@
APPROVAL_TRANSITIONS,
CollectionSubmissionStates,
COLLECTION_SUBMISSION_TRANSITIONS,
NodeRequestTypes
)
from website.mails import mails
from website.reviews import signals as reviews_signals
from website.settings import DOMAIN, OSF_SUPPORT_EMAIL, OSF_CONTACT_EMAIL

from osf.utils import notifications as notify

from api.base.exceptions import Conflict

class BaseMachine(Machine):

action = None
Expand Down Expand Up @@ -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]
Expand Down
75 changes: 75 additions & 0 deletions osf_tests/test_institutional_admin_contributors.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
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

@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_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
7 changes: 6 additions & 1 deletion website/profile/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ 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_institutional_admin': user.is_institutional_admin(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this for this view? It looks like it's being used, but is it being used properly? See below for more.

'is_curator': user.is_institutional_curator(node),
'profile_image_url': user.profile_image_url(size=settings.PROFILE_IMAGE_MEDIUM),
'active': user.is_active,
}
Expand Down Expand Up @@ -199,7 +201,10 @@ def serialize_access_requests(node):
'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')
]
Loading
Loading