-
Notifications
You must be signed in to change notification settings - Fork 327
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
[ENG-2814] Allow Read-only and Read/Write contributors to view a project's draft registrations #10660
Changes from 12 commits
b91536e
1ae8f29
c0a70bb
b2e8268
f507af1
9037d7f
a2268f8
0d854bb
75e5d7d
3ebaba0
2f623d2
eea4c16
d8e9d0f
974dc34
59fdd3e
18dc873
6a45c15
bfdffe0
da8a4da
ae233ad
e961bf4
3897e7f
930e25d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ | |
OSFUser, | ||
) | ||
from api.nodes.permissions import ContributorDetailPermissions | ||
from osf.utils.permissions import READ, WRITE, ADMIN | ||
|
||
|
||
class IsContributorOrAdminContributor(permissions.BasePermission): | ||
""" | ||
|
@@ -57,3 +59,27 @@ 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 request.method in permissions.SAFE_METHODS: | ||
if isinstance(obj, DraftRegistration): | ||
return obj.is_contributor(auth.user) or obj.has_permission(auth.user, READ) | ||
return obj.has_permission(auth.user, READ) | ||
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) | ||
return obj.has_permission(auth.user, ADMIN) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
else: | ||
if isinstance(obj, DraftRegistration): | ||
return obj.is_contributor(auth.user) and obj.has_permission(auth.user, WRITE) | ||
return obj.has_permission(auth.user, WRITE) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,13 @@ | ||
from rest_framework import permissions as drf_permissions, exceptions | ||
from django.db.models import Q | ||
|
||
from framework.auth.oauth_scopes import CoreScopes | ||
|
||
from api.base import permissions as base_permissions | ||
from api.base.pagination import DraftRegistrationContributorPagination | ||
from api.draft_registrations.permissions import ( | ||
DraftContributorDetailPermissions, | ||
IsContributorOrAdminContributor, | ||
DraftRegistrationPermission, | ||
IsAdminContributor, | ||
) | ||
from api.draft_registrations.serializers import ( | ||
|
@@ -27,7 +28,7 @@ | |
) | ||
from api.nodes.permissions import ContributorOrPublic, AdminDeletePermissions | ||
from api.subjects.views import SubjectRelationshipBaseView, BaseResourceSubjectsList | ||
from osf.models import DraftRegistrationContributor | ||
from osf.models import DraftRegistrationContributor, DraftRegistration | ||
|
||
class DraftRegistrationMixin(DraftMixin): | ||
""" | ||
|
@@ -50,9 +51,9 @@ def check_resource_permissions(self, resource): | |
|
||
class DraftRegistrationList(NodeDraftRegistrationsList): | ||
permission_classes = ( | ||
IsContributorOrAdminContributor, | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
base_permissions.TokenHasScope, | ||
DraftRegistrationPermission, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still a noop, since DraftRegistrationPermission only defines There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
) | ||
|
||
view_category = 'draft_registrations' | ||
|
@@ -68,12 +69,15 @@ def get_queryset(self): | |
if user.is_anonymous: | ||
raise exceptions.NotAuthenticated() | ||
# Returns DraftRegistrations for which a user is a contributor | ||
return user.draft_registrations_active | ||
|
||
return DraftRegistration.objects.filter( | ||
Q(_contributors=user) & | ||
Q(deleted__isnull=True) & | ||
(Q(registered_node__isnull=True) | Q(registered_node__deleted__isnull=False)), | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the explicit query instead of reusing the |
||
|
||
class DraftRegistrationDetail(NodeDraftRegistrationDetail, DraftRegistrationMixin): | ||
permission_classes = ( | ||
ContributorOrPublic, | ||
DraftRegistrationPermission, | ||
AdminDeletePermissions, | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
base_permissions.TokenHasScope, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -75,7 +76,6 @@ | |
from api.nodes.filters import NodesFilterMixin | ||
from api.nodes.permissions import ( | ||
IsAdmin, | ||
IsAdminContributor, | ||
IsPublic, | ||
AdminOrPublic, | ||
WriteAdmin, | ||
|
@@ -625,7 +625,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No | |
Use DraftRegistrationsList endpoint instead. | ||
""" | ||
permission_classes = ( | ||
IsAdminContributor, | ||
DraftRegistrationPermission, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't comment on the actually relevant line: i.e., the queryset should return We should probably get some additional clarification from Product about a couple of things: I'm happy to own this communication if you would like. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I discussed the questions you raised with Mark.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( Two fixes to get the correct behavior: Second, you'll need to update your Permission class to use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some unaddressed comments here regarding the queryset. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
) | ||
|
@@ -659,9 +659,9 @@ class NodeDraftRegistrationDetail(JSONAPIBaseView, generics.RetrieveUpdateDestro | |
Use DraftRegistrationDetail endpoint instead. | ||
""" | ||
permission_classes = ( | ||
DraftRegistrationPermission, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like this to be more specific. Maybe add an
isinstance(obj, NodeModel)
here, since we are checking the permission of the node?