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

feat: add object tag view permissions [FC-0036] #94

Conversation

rpenido
Copy link
Contributor

@rpenido rpenido commented Oct 9, 2023

Description

Add view permission for ObjectTag

  • The oel_tagging.view_objecttag_objectid rule is used in the rest API view to check if the user has permission on the object_id to see its tags

Supporting Information

Testing instructions

  • Please ensure that the tests cover the expected behavior of the view

Private-ref: FAL-3518

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 9, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 9, 2023

Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from ed87a26 to 447c7ba Compare October 9, 2023 13:22
@rpenido rpenido marked this pull request as ready for review October 9, 2023 13:23
@rpenido rpenido changed the title feat: add ObjectTag view permissions and filter feat: add ObjectTag view permissions and filters Oct 9, 2023
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch 2 times, most recently from 3087faa to 9b886cf Compare October 9, 2023 13:53
@pomegranited
Copy link
Contributor

@rpenido I don't think this PR is necessary -- We're anticipating functionality that's not specified yet, and that's gotten us into trouble before. I think it's enough for now to have this restriction in the content_tagging app.

CC @bradenmacdonald

@rpenido
Copy link
Contributor Author

rpenido commented Oct 10, 2023

Hi @pomegranited! I agree with you regarding the filter, but I think the permission belongs here:

  • We are overriding the retrieve method of the view to return a list. This is not the default behavior, so the DjangoModelPermissions will not work out of the box. We don't have a Model to check permission against, so we need to make explicit permission calls in the view code (like we did in the update):
    perm_obj = ChangeObjectTagPermissionItem(
    taxonomy=taxonomy,
    object_id=object_id,
    )
    if not request.user.has_perm(perm, perm_obj):
    raise PermissionDenied(
    "You do not have permission to change object tags for this taxonomy or object_id."
    )
  • The View is implemented here. To change the permission check to edx-platform we will need to reimplement the view code there.

I'll change the code to make it minimal. Let me know what you think!

@rpenido rpenido changed the title feat: add ObjectTag view permissions and filters feat: add ObjectTag view permissions Oct 10, 2023
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from 9b886cf to 4a95e0a Compare October 10, 2023 13:59
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from b8f113b to c1502d0 Compare October 10, 2023 14:23
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

I agree with you regarding the filter, but I think the permission belongs here:

Ok yes, I get your reasoning for putting the rules check and view change here. 👍

But why make oel_tagging.view_objecttag_objectid False by default?

openedx_tagging/core/tagging/rules.py Outdated Show resolved Hide resolved
tests/openedx_tagging/core/tagging/test_views.py Outdated Show resolved Hide resolved
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from b727ed2 to 11c6f46 Compare October 11, 2023 11:50
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from 11c6f46 to d7f78c1 Compare October 11, 2023 12:21
@rpenido rpenido changed the title feat: add ObjectTag view permissions feat: add object tag view permissions Oct 11, 2023
rpenido and others added 2 commits October 12, 2023 13:26
The "view objecttag" rule now checks permissions for the taxonomy and
object_id if provided.
@rpenido
Copy link
Contributor Author

rpenido commented Oct 12, 2023

Hi @pomegranited! I think we are close! I made the requested changes.
Let me know if I miss something! If everything is right, I will squash it and send it to CC review.

@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from d85ee7f to 677260d Compare October 12, 2023 16:57
@bradenmacdonald bradenmacdonald changed the title feat: add object tag view permissions feat: add object tag view permissions [FC-0036] Oct 13, 2023
@bradenmacdonald bradenmacdonald added the FC Relates to an Axim Funded Contribution project label Oct 13, 2023
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 This is looking good, thank you @rpenido , and ready for CC review!

Could you make sure there's a version bump before it gets merged?

@rpenido
Copy link
Contributor Author

rpenido commented Oct 15, 2023

Thank you for the review @pomegranited!
@bradenmacdonald , can you make a CC review here?

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Nice! Just one minor question / change request, then I'll approve and merge.

openedx_tagging/core/tagging/rules.py Show resolved Hide resolved
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from f43f2ad to c082514 Compare October 16, 2023 18:51
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from c082514 to 6b91980 Compare October 16, 2023 18:55
@bradenmacdonald
Copy link
Contributor

@rpenido Would you like me to merge this now?

@rpenido
Copy link
Contributor Author

rpenido commented Oct 16, 2023

Sure @bradenmacdonald! I think we are good here!
We also need to tag the new version (I already bumped the version to 0.2.5

@bradenmacdonald bradenmacdonald merged commit 38da66b into openedx:main Oct 16, 2023
7 checks passed
@openedx-webhooks
Copy link

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@bradenmacdonald bradenmacdonald deleted the rpenido/fal-3518-permissions-for-taxonomies branch October 16, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants