Skip to content

Commit

Permalink
[ENG-2814] Allow Read-only and Read/Write contributors to view a proj…
Browse files Browse the repository at this point in the history
…ect's draft registrations (CenterForOpenScience#10660)

## Purpose

Change permissions for DraftRegistrations so Read/Write contributors can view all listed DraftRegistrations for a node.

GET for reading, POST for DR creation and PATCH for editing

## Changes

- Add new permissions class NodeDraftRegistrationsListPermission to NodeDraftRegistrationsList
- Break up test runner into separate classes
- Split apart large single test functions into individual cases
- add more to testing matrix test_<user_permission>_draft_not_node has all cases

CenterForOpenScience#10468

## QA Notes


## Ticket

https://openscience.atlassian.net/browse/ENG-2814
  • Loading branch information
uditijmehta authored and Uditi Mehta committed Aug 9, 2024
1 parent 056452a commit 68e1f55
Show file tree
Hide file tree
Showing 9 changed files with 566 additions and 556 deletions.
33 changes: 33 additions & 0 deletions api/draft_registrations/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
OSFUser,
)
from api.nodes.permissions import ContributorDetailPermissions
from osf.utils.permissions import WRITE, ADMIN


class IsContributorOrAdminContributor(permissions.BasePermission):
"""
Expand Down Expand Up @@ -57,3 +59,34 @@ class DraftContributorDetailPermissions(ContributorDetailPermissions):

def load_resource(self, context, view):
return DraftRegistration.load(context['draft_id'])


class DraftRegistrationPermission(permissions.BasePermission):
"""
Check permissions for draft and node, Admin can create (POST) or edit (PATCH, PUT) to a DraftRegistration, but write
users can only edit them. Node permissions are inherited by the DraftRegistration when they are higher.
"""
acceptable_models = (DraftRegistration, AbstractNode)

def has_object_permission(self, request, view, obj):
auth = get_user_auth(request)

if not auth.user:
return False

if request.method in permissions.SAFE_METHODS:
if isinstance(obj, DraftRegistration):
return obj.can_view(auth)
elif isinstance(obj, AbstractNode):
return obj.can_view(auth)
elif request.method == 'POST': # Only Admin can create a draft registration
if isinstance(obj, DraftRegistration):
return obj.is_contributor(auth.user) and obj.has_permission(auth.user, ADMIN)
elif isinstance(obj, AbstractNode):
return obj.has_permission(auth.user, ADMIN)
else:
if isinstance(obj, DraftRegistration):
return obj.is_contributor(auth.user) and obj.has_permission(auth.user, WRITE)
elif isinstance(obj, AbstractNode):
return obj.has_permission(auth.user, WRITE)
return False
7 changes: 3 additions & 4 deletions api/draft_registrations/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from api.base.pagination import DraftRegistrationContributorPagination
from api.draft_registrations.permissions import (
DraftContributorDetailPermissions,
IsContributorOrAdminContributor,
DraftRegistrationPermission,
IsAdminContributor,
)
from api.draft_registrations.serializers import (
Expand Down Expand Up @@ -50,9 +50,9 @@ def check_resource_permissions(self, resource):

class DraftRegistrationList(NodeDraftRegistrationsList):
permission_classes = (
IsContributorOrAdminContributor,
drf_permissions.IsAuthenticatedOrReadOnly,
base_permissions.TokenHasScope,
DraftRegistrationPermission,
)

view_category = 'draft_registrations'
Expand All @@ -70,10 +70,9 @@ def get_queryset(self):
# Returns DraftRegistrations for which a user is a contributor
return user.draft_registrations_active


class DraftRegistrationDetail(NodeDraftRegistrationDetail, DraftRegistrationMixin):
permission_classes = (
ContributorOrPublic,
DraftRegistrationPermission,
AdminDeletePermissions,
drf_permissions.IsAuthenticatedOrReadOnly,
base_permissions.TokenHasScope,
Expand Down
13 changes: 8 additions & 5 deletions api/nodes/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from django.db.models import F, Max, Q, Subquery
from django.utils import timezone
from django.contrib.contenttypes.models import ContentType
from rest_framework import generics, permissions as drf_permissions
from rest_framework import generics, permissions as drf_permissions, exceptions
from rest_framework.exceptions import PermissionDenied, ValidationError, NotFound, MethodNotAllowed, NotAuthenticated
from rest_framework.response import Response
from rest_framework.status import HTTP_202_ACCEPTED, HTTP_204_NO_CONTENT
Expand Down Expand Up @@ -66,6 +66,7 @@
NodeCommentSerializer,
)
from api.draft_registrations.serializers import DraftRegistrationSerializer, DraftRegistrationDetailSerializer
from api.draft_registrations.permissions import DraftRegistrationPermission
from api.files.serializers import FileSerializer, OsfStorageFileSerializer
from api.files import annotations as file_annotations
from api.identifiers.serializers import NodeIdentifierSerializer
Expand All @@ -75,7 +76,6 @@
from api.nodes.filters import NodesFilterMixin
from api.nodes.permissions import (
IsAdmin,
IsAdminContributor,
IsPublic,
AdminOrPublic,
WriteAdmin,
Expand Down Expand Up @@ -626,7 +626,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No
Use DraftRegistrationsList endpoint instead.
"""
permission_classes = (
IsAdminContributor,
DraftRegistrationPermission,
drf_permissions.IsAuthenticatedOrReadOnly,
base_permissions.TokenHasScope,
)
Expand All @@ -649,8 +649,11 @@ def get_serializer_class(self):

# overrides ListCreateAPIView
def get_queryset(self):
user = self.request.user
node = self.get_node()
return node.draft_registrations_active
if user.is_anonymous:
raise exceptions.NotAuthenticated()
return user.draft_registrations_active.filter(branched_from=node)


class NodeDraftRegistrationDetail(JSONAPIBaseView, generics.RetrieveUpdateDestroyAPIView, DraftMixin):
Expand All @@ -660,9 +663,9 @@ class NodeDraftRegistrationDetail(JSONAPIBaseView, generics.RetrieveUpdateDestro
Use DraftRegistrationDetail endpoint instead.
"""
permission_classes = (
DraftRegistrationPermission,
drf_permissions.IsAuthenticatedOrReadOnly,
base_permissions.TokenHasScope,
IsAdminContributor,
)
parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON)

Expand Down
132 changes: 75 additions & 57 deletions api_tests/draft_registrations/views/test_draft_registration_detail.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@

from api.base.settings.defaults import API_BASE
from api_tests.nodes.views.test_node_draft_registration_detail import (
TestDraftRegistrationDetail,
TestDraftRegistrationUpdate,
TestDraftRegistrationPatch,
TestDraftRegistrationDelete,
AbstractDraftRegistrationTestCase
)
from osf.models import DraftNode, Node, NodeLicense, RegistrationSchema
from osf.utils.permissions import ADMIN, READ, WRITE
Expand All @@ -16,58 +16,34 @@
SubjectFactory,
ProjectFactory,
)
from website.settings import API_DOMAIN


@pytest.mark.django_db
class TestDraftRegistrationDetailEndpoint(TestDraftRegistrationDetail):
class TestDraftRegistrationDetailEndpoint(AbstractDraftRegistrationTestCase):
@pytest.fixture()
def url_draft_registrations(self, project_public, draft_registration):
return '/{}draft_registrations/{}/'.format(
API_BASE, draft_registration._id)

# Overrides TestDraftRegistrationDetail
def test_admin_group_member_can_view(self, app, user, draft_registration, project_public,
schema, url_draft_registrations, group_mem):

res = app.get(url_draft_registrations, auth=group_mem.auth, expect_errors=True)
assert res.status_code == 403
return f'/{API_BASE}draft_registrations/{draft_registration._id}/'

def test_can_view_draft(
self, app, user_write_contrib, project_public,
user_read_contrib, user_non_contrib,
url_draft_registrations, group, group_mem):

# test_read_only_contributor_can_view_draft
res = app.get(
url_draft_registrations,
auth=user_read_contrib.auth,
expect_errors=False)
def test_read_only_contributor_can_view_draft(self, app, user_read_contrib, url_draft_registrations):
res = app.get(url_draft_registrations, auth=user_read_contrib.auth)
assert res.status_code == 200

# test_read_write_contributor_can_view_draft
res = app.get(
url_draft_registrations,
auth=user_write_contrib.auth,
expect_errors=False)
def test_read_write_contributor_can_view_draft(self, app, user_write_contrib, url_draft_registrations):
res = app.get(url_draft_registrations, auth=user_write_contrib.auth)
assert res.status_code == 200

def test_cannot_view_draft(
self, app, project_public,
user_non_contrib, url_draft_registrations):

# test_logged_in_non_contributor_cannot_view_draft
res = app.get(
url_draft_registrations,
auth=user_non_contrib.auth,
expect_errors=True)
def test_logged_in_non_contributor_cannot_view_draft(self, app, user_non_contrib, url_draft_registrations):
res = app.get(url_draft_registrations, auth=user_non_contrib.auth, expect_errors=True)
assert res.status_code == 403

# test_unauthenticated_user_cannot_view_draft
def test_unauthenticated_user_cannot_view_draft(self, app, url_draft_registrations):
res = app.get(url_draft_registrations, expect_errors=True)
assert res.status_code == 401

def test_detail_view_returns_editable_fields(self, app, user, draft_registration,
url_draft_registrations, project_public):
def test_detail_view_returns_editable_fields(
self, app, user, draft_registration, url_draft_registrations, project_public
):

res = app.get(url_draft_registrations, auth=user.auth, expect_errors=True)
attributes = res.json['data']['attributes']
Expand All @@ -77,7 +53,7 @@ def test_detail_view_returns_editable_fields(self, app, user, draft_registration
assert attributes['category'] == project_public.category
assert attributes['has_project']

res.json['data']['links']['self'] == url_draft_registrations
assert res.json['data']['links']['self'] == f'{API_DOMAIN}{url_draft_registrations.lstrip("/")}'

relationships = res.json['data']['relationships']
assert Node.load(relationships['branched_from']['data']['id']) == draft_registration.branched_from
Expand All @@ -89,8 +65,7 @@ def test_detail_view_returns_editable_fields(self, app, user, draft_registration
def test_detail_view_returns_editable_fields_no_specified_node(self, app, user):

draft_registration = DraftRegistrationFactory(initiator=user, branched_from=None)
url = '/{}draft_registrations/{}/'.format(
API_BASE, draft_registration._id)
url = f'{API_DOMAIN}{API_BASE}draft_registrations/{draft_registration._id}/'

res = app.get(url, auth=user.auth, expect_errors=True)
attributes = res.json['data']['attributes']
Expand All @@ -101,7 +76,7 @@ def test_detail_view_returns_editable_fields_no_specified_node(self, app, user):
assert attributes['node_license'] is None
assert not attributes['has_project']

res.json['data']['links']['self'] == url
assert res.json['data']['links']['self'] == url
relationships = res.json['data']['relationships']

assert 'affiliated_institutions' in relationships
Expand All @@ -112,44 +87,88 @@ def test_detail_view_returns_editable_fields_no_specified_node(self, app, user):
res = app.get(draft_node_link, auth=user.auth)
assert DraftNode.load(res.json['data']['id']) == draft_registration.branched_from

def test_draft_registration_perms_checked_on_draft_not_node(self, app, user, project_public,
draft_registration, url_draft_registrations):

# Admin on node and draft
def test_admin_node_and_draft(self, app, user, project_public, draft_registration, url_draft_registrations):
assert project_public.has_permission(user, ADMIN) is True
assert draft_registration.has_permission(user, ADMIN) is True
res = app.get(url_draft_registrations, auth=user.auth)
assert res.status_code == 200

# Admin on node but not draft
def test_admin_node_not_draft(self, app, user, project_public, draft_registration, url_draft_registrations):
node_admin = AuthUserFactory()
project_public.add_contributor(node_admin, ADMIN)
assert project_public.has_permission(node_admin, ADMIN) is True
assert draft_registration.has_permission(node_admin, ADMIN) is False
res = app.get(url_draft_registrations, auth=node_admin.auth, expect_errors=True)
assert res.status_code == 403

# Admin on draft but not node
def test_admin_draft_not_node(self, app, user, project_public, draft_registration, url_draft_registrations):
draft_admin = AuthUserFactory()
draft_registration.add_contributor(draft_admin, ADMIN)
assert project_public.has_permission(draft_admin, ADMIN) is False
assert draft_registration.has_permission(draft_admin, ADMIN) is True
res = app.get(url_draft_registrations, auth=draft_admin.auth)
assert res.status_code == 200

# Overwrites TestDraftRegistrationDetail
def test_can_view_after_added(
self, app, schema, draft_registration, url_draft_registrations):
# Draft Registration permissions are no longer based on the branched from project
def test_write_node_and_draft(self, app, user, project_public, draft_registration, url_draft_registrations):
assert project_public.has_permission(user, WRITE) is True
assert draft_registration.has_permission(user, WRITE) is True
res = app.get(url_draft_registrations, auth=user.auth)
assert res.status_code == 200

def test_write_node_not_draft(self, app, user, project_public, draft_registration, url_draft_registrations):
node_admin = AuthUserFactory()
project_public.add_contributor(node_admin, WRITE)
assert project_public.has_permission(node_admin, WRITE) is True
assert draft_registration.has_permission(node_admin, WRITE) is False
res = app.get(url_draft_registrations, auth=node_admin.auth, expect_errors=True)
assert res.status_code == 403

def test_write_draft_not_node(self, app, user, project_public, draft_registration, url_draft_registrations):
draft_admin = AuthUserFactory()
draft_registration.add_contributor(draft_admin, WRITE)
assert project_public.has_permission(draft_admin, WRITE) is False
assert draft_registration.has_permission(draft_admin, WRITE) is True
res = app.get(url_draft_registrations, auth=draft_admin.auth)
assert res.status_code == 200

def test_read_node_and_draft(self, app, user, project_public, draft_registration, url_draft_registrations):
assert project_public.has_permission(user, READ) is True
assert draft_registration.has_permission(user, READ) is True
res = app.get(url_draft_registrations, auth=user.auth)
assert res.status_code == 200

def test_read_node_not_draft(self, app, user, project_public, draft_registration, url_draft_registrations):
node_admin = AuthUserFactory()
project_public.add_contributor(node_admin, READ)
assert project_public.has_permission(node_admin, READ) is True
assert draft_registration.has_permission(node_admin, READ) is False
res = app.get(url_draft_registrations, auth=node_admin.auth, expect_errors=True)
assert res.status_code == 403

def test_read_draft_not_node(self, app, user, project_public, draft_registration, url_draft_registrations):
draft_admin = AuthUserFactory()
draft_registration.add_contributor(draft_admin, READ)
assert project_public.has_permission(draft_admin, READ) is False
assert draft_registration.has_permission(draft_admin, READ) is True
res = app.get(url_draft_registrations, auth=draft_admin.auth)
assert res.status_code == 200

def test_can_view_after_added(self, app, schema, draft_registration, url_draft_registrations):
"""
Ensure Draft Registration permissions are no longer based on the branched from project
"""
user = AuthUserFactory()
project = draft_registration.branched_from
project.add_contributor(user, ADMIN)
res = app.get(url_draft_registrations, auth=user.auth, expect_errors=True)
assert res.status_code == 403
draft_registration.add_contributor(user, ADMIN)
res = app.get(url_draft_registrations, auth=user.auth)
assert res.status_code == 200

def test_current_permissions_field(self, app, user_read_contrib,
user_write_contrib, user, draft_registration, url_draft_registrations):
def test_current_permissions_field(
self, app, user_read_contrib, user_write_contrib, user, draft_registration, url_draft_registrations
):
res = app.get(url_draft_registrations, auth=user_read_contrib.auth, expect_errors=False)
assert res.json['data']['attributes']['current_user_permissions'] == [READ]

Expand Down Expand Up @@ -548,9 +567,8 @@ def test_write_contributor_can_update_draft(
assert data['attributes']['registration_metadata'] == payload['data']['attributes']['registration_metadata']


class TestDraftRegistrationDelete(TestDraftRegistrationDelete):
class TestDraftRegistrationDeleteDetail(TestDraftRegistrationDelete):
@pytest.fixture()
def url_draft_registrations(self, project_public, draft_registration):
# Overrides TestDraftRegistrationDelete
return '/{}draft_registrations/{}/'.format(
API_BASE, draft_registration._id)
return f'/{API_BASE}draft_registrations/{draft_registration._id}/'
Loading

0 comments on commit 68e1f55

Please sign in to comment.