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-2814] Allow Read-only and Read/Write contributors to view a project's draft registrations #10660

Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b91536e
change draft registration permissions and clean-up tests
Oct 17, 2023
1ae8f29
use `exists` instead of evaluating the whole queryset
Oct 24, 2023
c0a70bb
Resolve conflicts between draft-reg-permission and develop
Jul 5, 2024
b2e8268
Resolve conflicts between draft-reg-permission and develop
Jul 5, 2024
f507af1
Resolve conflicts between draft-reg-permission and develop
Jul 8, 2024
9037d7f
refine visibility rules and remove redundant tests
Jul 15, 2024
a2268f8
fix flake8
Jul 15, 2024
0d854bb
fix flake8
Jul 15, 2024
75e5d7d
fix tests
Jul 16, 2024
3ebaba0
Include DraftRegistrationPermission in DraftRegistrationList view to …
Jul 16, 2024
2f623d2
Include DraftRegistrationPermission in DraftRegistrationList view to …
Jul 16, 2024
eea4c16
Include DraftRegistrationPermission in DraftRegistrationList view to …
Jul 16, 2024
d8e9d0f
remove redundant query
Jul 16, 2024
974dc34
remove redundant query
Jul 16, 2024
59fdd3e
remove redundant query
Jul 16, 2024
18dc873
Update NodeDraftRegistrationsList and DraftRegistrationPermission for…
Jul 25, 2024
6a45c15
Update tests to allow draft contributors to view draft list.
Jul 25, 2024
bfdffe0
Update tests to allow draft contributors to view draft list.
Jul 25, 2024
da8a4da
Update tests to allow draft contributors to view draft list.
Jul 25, 2024
ae233ad
Update tests to allow draft contributors to view draft list.
Jul 25, 2024
e961bf4
Update tests to allow draft contributors to view draft list.
Jul 25, 2024
3897e7f
Resolved merge conflicts between draft-reg-permission and feature/b-a…
Jul 31, 2024
930e25d
Resolved merge conflicts between draft-reg-permission and feature/b-a…
Aug 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still a noop, since DraftRegistrationPermission only defines has_object_permission -- and this endpoint doesn't need any extra permissions, since it's just surfacing the current user's drafts (https://media1.tenor.com/m/j5TI9U8hISQAAAAC/emperors-new-groove-why.gif)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it, a bunch of our tests are failing because it is letting unauthorized users create draft registrations.

The following tests failed when it was removed:

test_write_only_contributor_cannot_create_draft
test_read_only_contributor_cannot_create_draft
test_draft_registration_attributes_copied_from_node
test_logged_in_non_contributor_cannot_create_draft

)

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 @@ -625,7 +625,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No
Use DraftRegistrationsList endpoint instead.
"""
permission_classes = (
IsAdminContributor,
DraftRegistrationPermission,
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't comment on the actually relevant line:
My instinct is that the get_queryset should change to support the ticket requirements. Instead of listing all of the Drafts, this should list the Drafts on which the user is a contributor.

i.e., the queryset should return user.draft_registractions_active.filter(branched_from=node)
Existing behavior for the anonymous user would be to 401 -- which would redirect them to the login page -- but

We should probably get some additional clarification from Product about a couple of things:
First: Should users who are contributors on the Draft but not the branched_from Project be able to see their Drafts on the list of the Project's Drafts (so long as the Project is public)?
Second: Should Project Admins be able to view all Drafts, even if they aren't contributors, or should that wait until the related ticket to give branched_from Project Admins implicit read permissions on Drafts even when they aren't contributors?
Third: What should the behavior be for Unauthenticated users? Should we 401, implicitly redirecting them to the login page? Or should we just return the Empty queryset?

I'm happy to own this communication if you would like.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is unaddressed. Let me know if you want a quick Zoom to clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discussed the questions you raised with Mark.

  1. If a user is a contributor to a draft registration but not to the main project, they should still be able to see their drafts if the project is public.
  2. For now, we'll wait to implement a feature that gives project admins implicit read permissions on all related drafts.
  3. Users who are not logged in should be redirected to the login page if they try to view draft registrations. If they don't have an account, they should be sent to a create an account page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for sorting this out!

All of that being true, there's a bit of work left to do. Mostly, this endpoint (NodeDraftRegistrationList) is currently returning the full list of DraftRegistrations to all Node contributors.

Two fixes to get the correct behavior:
First, you'll need to update this get_queryset function to return something like user.active_draft_registrations.filter(branched_from=node).

Second, you'll need to update your Permission class to use can_view instead of has_permission(READ) so that non-contributors on the Project can still see their associated Drafts.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I added some redundant-ish comments on tests to highlight where these requirements were being missed; we'll want/need to expand the test coverage, too)

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are some unaddressed comments here regarding the queryset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I had missed this comment. I have updated the PR.

drf_permissions.IsAuthenticatedOrReadOnly,
base_permissions.TokenHasScope,
)
Expand All @@ -648,8 +648,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 @@ -659,9 +662,9 @@ class NodeDraftRegistrationDetail(JSONAPIBaseView, generics.RetrieveUpdateDestro
Use DraftRegistrationDetail endpoint instead.
"""
permission_classes = (
DraftRegistrationPermission,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given everything else I've said, I think the Detail views can go untouched for now.

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 still need to update the permission of the detail view?

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
Loading