feat: new AuthZ permissions for import/export courses and export tags#38173
feat: new AuthZ permissions for import/export courses and export tags#38173rodmgwgu wants to merge 4 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @rodmgwgu! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
8531587 to
4208e9d
Compare
| path( | ||
| "object_tags/<str:context_id>/export/", | ||
| views.ObjectTagExportView.as_view(), | ||
| name="taxonomy-object-tag-export", |
There was a problem hiding this comment.
Named this so we can refer to it on tests with the reverse method.
| except InvalidKeyError: | ||
| log.warning("Invalid course key %s", object_id) | ||
|
|
||
| if course_key and core_toggles.enable_authz_course_authoring(course_key) and not authz_api.is_user_allowed( |
There was a problem hiding this comment.
I'm still letting the old checks (line 35) to run even when the authz check gets done, as these are different from the rest of the course authoring checks and I'm not familiar with this logic.
@bmtcril are you familiar with this?
There was a problem hiding this comment.
I put this comment in the wrong line, I'm referring to the checks on line 35
There was a problem hiding this comment.
I know roughly what it is: https://github.com/dfunckt/django-rules but I'm not familiar with our implementation for tagging. I'll ask around to see if anyone else here knows more.
There was a problem hiding this comment.
Maybe this can help? https://github.com/openedx/openedx-platform/pull/38071/changes#diff-15db056809a5257a6081c33072c7d430098eb751b7bea826c8e1325702b4c9c4
It's the implementation we did for library tags, maybe something we learned there can be reused here FYI @BryanttV
There was a problem hiding this comment.
What I can add here is that user.has_perm() in this case calls the functions can_view_object_tag_taxonomy and can_view_object_tag_objectid, but since taxonomy=None, it only goes into the validation in the second function. There, it checks whether the user has read permissions in Studio, or if the user is an org_admin or org_user. I’m not sure whether we want to keep that legacy validation or remove it and simply rely on authz.
| try: | ||
| course_key = CourseKey.from_string(object_id) | ||
| except InvalidKeyError: | ||
| log.warning("Invalid course key %s", object_id) |
There was a problem hiding this comment.
Should we re-raise this exception? The logic below depends on it, and not doing so might cause silent errors
There was a problem hiding this comment.
So my though process here was:
I see that this function can be used for things other than courses, so in that case we don't want to affect the existing behavior.
So, if CourseKey.from_string fails, course_key will remain None
Then when we get to the if, because course_key is None, we don't validate for Authz
This means that we fallback to the code below that is the old validation check.
There was a problem hiding this comment.
I added a comment to clarify this
4208e9d to
5513880
Compare
Description
Implement new AuthZ permission checks over endpoints related with import/export courses functionality, and export tags from courses.
The new AuthZ permission checks only apply when the enable_authz_course_authoring feature flag is enabled for the specific course, or globally, otherwise existing behavior persist.
The following AuthZ permissions are being used:
The following endpoints were updated:
Also updated tests and existing test utilities for being more flexible to test AuthZ use cases across the codebase.
Supporting information
Closes openedx/openedx-authz#201
Testing
Verified that:
Running relevant tests manually:
On a cms container (run with
tutor dev exec cms bash), do:Deadline
Verawood
Other information
Depends on #38156