fix: course editor gets 403 copying a unit to the clipboard - #39075
fix: course editor gets 403 copying a unit to the clipboard #39075efortish wants to merge 2 commits into
Conversation
ClipboardEndpoint.post() gated read access on has_studio_read_access(), a legacy-only check with no knowledge of AuthZ-native roles like course_editor and course_auditor. Switched to user_has_course_permission() with COURSES_VIEW_COURSE, falling back to the same legacy check when AuthZ course authoring isn't enabled for the course. Same pattern already used for the same class of bug in openedx-authz#384.
Matches the existing test_no_course_permission's pattern for this same endpoint: a PermissionDenied raised mid-request under transaction.non_atomic_requests leaves ModuleStoreTestCase's cleanup unable to run its own queries afterward, unless the assertion is wrapped in allow_transaction_exception().
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @efortish! 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. |
rodmgwgu
left a comment
rodmgwgu
left a comment
There was a problem hiding this comment.
Tested in my local, working as described, Thanks!
| if not has_studio_read_access(request.user, course_key): | ||
| if not user_has_course_permission( | ||
| request.user, | ||
| COURSES_VIEW_COURSE.identifier, |
There was a problem hiding this comment.
I'm not entirely sure if COURSES_VIEW_COURSE is the right permission to validate here, since the Course Auditor could also use the endpoint, and in theory, that role should only have view permissions. Maybe we should check for COURSES_EDIT_COURSE instead?
I'd like to get Product's perspective on this @gviedma-aulasneo
There was a problem hiding this comment.
@BryanttV @rodmgwgu
Once a user is able to view the course content, the “copy” action is really just a faster way to replicate that content. I don’t see much harm in allowing users with view access to copy it as well.
That said, it is a bit “dirty” to tie the action to a permission that technically represents something else. It sounds like a fairly niche permission to split out and validate separately, though.
So let’s proceed with the view permission for now, since it is the most basic permission across the authoring roles.
There was a problem hiding this comment.
Thanks @gviedma-aulasneo.
To clarify, this is only the copy endpoint. There's also an endpoint to paste the content into the course, which is currently restricted for the Course Auditor because they only have view permissions.
Similarly, on the frontend, all buttons and menus are currently being hidden so that the Course Auditor cannot perform any paste actions.
Considering this, I believe the correct permission to validate should be COURSES_EDIT_COURSE, rather than the view permission. Even though the pasted content is based on existing content, it still involves adding another unit/component, etc., to the course. The way I see it, the Course Auditor shouldn't be able to do that.
What do you think?
cc @rodmgwgu
5 participants
Description
Closes openedx-authz#403.
A user with the Course Editor role gets a
403 Forbiddenwhen trying to copy a unit to the clipboard in Studio, even though they can view and edit the course content normally elsewhere in Studio.The issue is in
ClipboardEndpoint.post()(openedx/core/djangoapps/content_staging/views.py), which useshas_studio_read_access()to check whether the user can read the course. That check only knows about the legacyCourseAccessRoleroles (staff, instructor, limited staff, etc.) and doesn't account for AuthZ-native roles such ascourse_editororcourse_auditor, which don't have a legacy equivalent.As a result, users whose course access comes only from one of these AuthZ roles are denied by the clipboard endpoint, even though they have the appropriate permissions to work with the course in Studio.
This is the same type of issue fixed in openedx-authz#384, where
_get_item_in_coursewas relying on a legacy write check for callers that only needed read access.The fix follows the same approach: use
user_has_course_permission()with the AuthZCOURSES_VIEW_COURSEpermission, and fall back to the existinghas_studio_read_accesscheck throughLegacyAuthoringPermission.READwhen AuthZ course authoring is not enabled for the course.This keeps the existing behavior unchanged for courses that have not been migrated to AuthZ.