[ENG-2814] Allow Read-only and Read/Write contributors to view a project's draft registrations #10660

uditijmehta · 2024-07-05T18:53:29Z

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

#10468

QA Notes

Ticket

<(https://openscience.atlassian.net/browse/ENG-2814)>

jwalz

Sorry, there's some history to this ticket that isn't well documented (I did the dumb thing of just assuming that I'd own this whenever it came back up) >_<

jwalz · 2024-07-05T21:00:42Z

api_tests/nodes/views/test_node_draft_registration_list.py

+ data = res.json['data']
+ assert len(data) == 1
+
+ @pytest.mark.skip('Old OSF Groups code')


Feel free to just delete all of the tests for osf groups

jwalz · 2024-07-10T14:28:18Z

api/draft_registrations/views.py

@@ -50,7 +50,7 @@ def check_resource_permissions(self, resource):

 class DraftRegistrationList(NodeDraftRegistrationsList):
 permission_classes = (
- IsContributorOrAdminContributor,
+ DraftRegistrationPermission,


I don't think the Permissions class actually does anything here. It only defines has_object_permission, which is only invoked by default on detail views. That makes sense here, considering the endpoint specifically surfaces the active drafts for the requesting user.

jwalz · 2024-07-10T14:48:01Z

api/draft_registrations/permissions.py

+ if request.method in permissions.SAFE_METHODS:
+ if isinstance(obj, DraftRegistration):
+ node_permission = obj.branched_from.has_permission(auth.user, READ)
+ return obj.has_permission(auth.user, READ) or node_permission
+ elif request.method == 'POST': # Only Admin can create a draft registration
+ if isinstance(obj, DraftRegistration):
+ node_permission = obj.branched_from.has_permission(auth.user, ADMIN)
+ return obj.has_permission(auth.user, ADMIN) or node_permission
+ else:
+ if isinstance(obj, DraftRegistration):
+ node_permission = obj.branched_from.has_permission(auth.user, WRITE)
+ return obj.has_permission(auth.user, WRITE) or node_permission


Making all permissions inheritable from the branched_from node can lead to some confusing situations down the line -- notably, when a Draft is Registered and suddenly a user who thought they had a certain permission level doesn't have those permissions anymore, but there are probably also other edge cases just because there are lots of skeletons in the registration workflow.

There is a plan to some day kill off DraftRegistrationContributors (which were only created due to a confusion in requirements when No Project Registratsions were implemented).

For now, let's constrain this PR to the specific issue identified in the ticket:
Users who view a Project's DraftRegistrations should see the Drafts on which they are contributors.

jwalz · 2024-07-10T15:12:04Z

api/nodes/views.py

@@ -625,7 +625,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No
 Use DraftRegistrationsList endpoint instead.
 """
 permission_classes = (
- IsAdminContributor,
+ DraftRegistrationPermission,


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.

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

I discussed the questions you raised with Mark.

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.

For now, we'll wait to implement a feature that gives project admins implicit read permissions on all related drafts.

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.

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.

(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 are some unaddressed comments here regarding the queryset.

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

jwalz · 2024-07-10T15:13:03Z

api/nodes/views.py

@@ -659,9 +659,9 @@ class NodeDraftRegistrationDetail(JSONAPIBaseView, generics.RetrieveUpdateDestro
 Use DraftRegistrationDetail endpoint instead.
 """
 permission_classes = (
+ DraftRegistrationPermission,


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

Do we still need to update the permission of the detail view?

…enforce correct permissions

jwalz · 2024-07-16T15:46:28Z

api/draft_registrations/views.py

+ return DraftRegistration.objects.filter(
+ Q(_contributors=user) &
+ Q(deleted__isnull=True) &
+ (Q(registered_node__isnull=True) | Q(registered_node__deleted__isnull=False)),
+ )


Why the explicit query instead of reusing the user.draft_registrations_active?

jwalz · 2024-07-16T15:49:10Z

api/nodes/views.py

@@ -625,7 +625,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No
 Use DraftRegistrationsList endpoint instead.
 """
 permission_classes = (
- IsAdminContributor,
+ DraftRegistrationPermission,


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

jwalz · 2024-07-19T14:02:44Z

api_tests/nodes/views/test_node_draft_registration_list.py

- def test_admin_can_view_draft_list(
- self, app, user, draft_registration, project_public,
- schema, url_draft_registrations):
+ def test_draft_admin_can_view_draft_list(


user here is also a Project admin, as they are the creator of project_public.

jwalz · 2024-07-19T14:04:45Z

api_tests/nodes/views/test_node_draft_registration_list.py

- group = OSFGroupFactory(creator=group_mem)
- project_public.add_osf_group(group, permissions.ADMIN)
- res = app.get(url_draft_registrations, auth=group_mem.auth, expect_errors=True)
+ def test_node_admin_can_view_draft_list(


This test is confirming that a Project Admin can view the draft list even if they are not a contributor on the draft -- and, in fact, confirms that they can see the Draft in the list, even if they are not a contributor on the Draft, which was identified as work for a different ticket.

jwalz · 2024-07-19T14:06:46Z

api_tests/nodes/views/test_node_draft_registration_list.py

- url_draft_registrations,
- auth=user_non_contrib.auth,
- expect_errors=True)
+ def test_logged_in_non_contributor_cannot_view_draft_list(self, app, user_non_contrib, url_draft_registrations):


Non-contributor on the project who is a contributor on the Draft should still see the draft in the list. This shows that they will 403 instead (because we're checking has_permission(user, READ) instead of can_view(user))

jwalz · 2024-07-19T14:07:27Z

api_tests/nodes/views/test_node_draft_registration_list.py

- user_read_contrib, user_non_contrib,
- project_public, payload, group,
- url_draft_registrations, group_mem):
+ def test_write_only_contributor_cannot_create_draft(


Thank you for breaking this into distinct tests!!

jwalz · 2024-07-19T14:10:02Z

api_tests/nodes/views/test_node_draft_registration_list.py

+ )
+
+ @pytest.fixture()
+ def metaschema_open_ended(self):


nit/honest question: why the separate schema?

I think the separate metaschema_open_ended fixture is there to ensure draft registrations are tested with the right schema for consistency.

jwalz · 2024-07-19T14:30:50Z

api/nodes/views.py

@@ -625,7 +625,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No
 Use DraftRegistrationsList endpoint instead.
 """
 permission_classes = (
- IsAdminContributor,
+ DraftRegistrationPermission,


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.

jwalz · 2024-07-19T14:33:06Z

api/nodes/views.py

@@ -625,7 +625,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No
 Use DraftRegistrationsList endpoint instead.
 """
 permission_classes = (
- IsAdminContributor,
+ DraftRegistrationPermission,


(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)

jwalz · 2024-07-19T14:40:45Z

api/draft_registrations/views.py

 drf_permissions.IsAuthenticatedOrReadOnly,
 base_permissions.TokenHasScope,
+ DraftRegistrationPermission,


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)

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

adlius

Some nitpicks and unaddressed comments from @jwalz . Also there seems to be some merge conflicts.

adlius · 2024-07-24T02:27:54Z

api/draft_registrations/permissions.py

+ 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)


I'd like this to be more specific. Maybe add an isinstance(obj, NodeModel) here, since we are checking the permission of the node?

adlius · 2024-07-24T02:28:03Z

api/draft_registrations/permissions.py

+ 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)


adlius · 2024-07-24T02:28:09Z

api/draft_registrations/permissions.py

+ 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)


adlius · 2024-07-24T02:28:44Z

api/nodes/views.py

@@ -625,7 +625,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No
 Use DraftRegistrationsList endpoint instead.
 """
 permission_classes = (
- IsAdminContributor,
+ DraftRegistrationPermission,


There are some unaddressed comments here regarding the queryset.

… refined querysets and permissions handling.

adlius

LGTM. Just one small question.

adlius · 2024-07-27T01:36:46Z

api/nodes/views.py

@@ -659,9 +659,9 @@ class NodeDraftRegistrationDetail(JSONAPIBaseView, generics.RetrieveUpdateDestro
 Use DraftRegistrationDetail endpoint instead.
 """
 permission_classes = (
+ DraftRegistrationPermission,


Do we still need to update the permission of the detail view?

…nd-i-24-14

jwalz

lgtm

…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

John Tordoff and others added 5 commits October 24, 2023 07:51

change draft registration permissions and clean-up tests

b91536e

use exists instead of evaluating the whole queryset

1ae8f29

Resolve conflicts between draft-reg-permission and develop

c0a70bb

Resolve conflicts between draft-reg-permission and develop

b2e8268

Resolve conflicts between draft-reg-permission and develop

f507af1

uditijmehta changed the title ~~Draft reg permission~~ [ENG-2814] Allow Read-only and Read/Write contributors to view a project's draft registrations Jul 9, 2024

jwalz reviewed Jul 10, 2024

View reviewed changes

Uditi Mehta added 7 commits July 15, 2024 10:24

refine visibility rules and remove redundant tests

9037d7f

fix flake8

a2268f8

fix flake8

0d854bb

fix tests

75e5d7d

Include DraftRegistrationPermission in DraftRegistrationList view to …

3ebaba0

…enforce correct permissions

Include DraftRegistrationPermission in DraftRegistrationList view to …

2f623d2

…enforce correct permissions

Include DraftRegistrationPermission in DraftRegistrationList view to …

eea4c16

…enforce correct permissions

jwalz requested changes Jul 16, 2024

View reviewed changes

Uditi Mehta added 3 commits July 16, 2024 12:11

remove redundant query

d8e9d0f

remove redundant query

974dc34

remove redundant query

59fdd3e

jwalz requested changes Jul 19, 2024

View reviewed changes

adlius requested changes Jul 24, 2024

View reviewed changes

uditijmehta changed the base branch from develop to feature/b-and-i-24-14 July 25, 2024 12:21

Uditi Mehta added 6 commits July 25, 2024 08:42

Update NodeDraftRegistrationsList and DraftRegistrationPermission for…

18dc873

… refined querysets and permissions handling.

Update tests to allow draft contributors to view draft list.

6a45c15

Update tests to allow draft contributors to view draft list.

bfdffe0

Update tests to allow draft contributors to view draft list.

da8a4da

Update tests to allow draft contributors to view draft list.

ae233ad

Update tests to allow draft contributors to view draft list.

e961bf4

adlius approved these changes Jul 27, 2024

View reviewed changes

Uditi Mehta added 2 commits July 31, 2024 15:28

Resolved merge conflicts between draft-reg-permission and feature/b-a…

3897e7f

…nd-i-24-14

Resolved merge conflicts between draft-reg-permission and feature/b-a…

930e25d

…nd-i-24-14

jwalz approved these changes Aug 1, 2024

View reviewed changes

brianjgeiger merged commit 0b6a3da into CenterForOpenScience:feature/b-and-i-24-14 Aug 1, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[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

uditijmehta commented Jul 5, 2024

jwalz left a comment

jwalz Jul 5, 2024

jwalz Jul 10, 2024

jwalz Jul 10, 2024

jwalz Jul 10, 2024

jwalz Jul 16, 2024

uditijmehta Jul 16, 2024

jwalz Jul 19, 2024

jwalz Jul 19, 2024

adlius Jul 24, 2024

uditijmehta Jul 25, 2024

jwalz Jul 10, 2024

adlius Jul 27, 2024

jwalz Jul 16, 2024

jwalz Jul 16, 2024

jwalz Jul 19, 2024

jwalz Jul 19, 2024

jwalz Jul 19, 2024

jwalz Jul 19, 2024

jwalz Jul 19, 2024

uditijmehta Jul 23, 2024

jwalz Jul 19, 2024

jwalz Jul 19, 2024

jwalz Jul 19, 2024

uditijmehta Jul 23, 2024

adlius left a comment •

edited

Loading

adlius Jul 24, 2024

adlius Jul 24, 2024

adlius Jul 24, 2024

adlius Jul 24, 2024

adlius left a comment

adlius Jul 27, 2024

jwalz left a comment

[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

Conversation

uditijmehta commented Jul 5, 2024

Purpose

Changes

QA Notes

Ticket

jwalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlius left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwalz left a comment

Choose a reason for hiding this comment

adlius left a comment •

edited

Loading