feat: split read vs write authz checks for advanced settings - #39007
feat: split read vs write authz checks for advanced settings#39007wgu-taylor-payne wants to merge 1 commit into
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @wgu-taylor-payne! 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. |
19f312d to
6173a9f
Compare
Use COURSES_VIEW_ADVANCED_SETTINGS for read access and COURSES_MANAGE_ADVANCED_SETTINGS for write access when authz is enabled. This allows Course Editors and Auditors to view advanced settings in read-only mode while keeping write access restricted. ENG45-699
6173a9f to
e59713e
Compare
BryanttV
left a comment
BryanttV
left a comment
There was a problem hiding this comment.
LGTM! I tested this on my local and it works as expected.
| ) | ||
| self.assertEqual(response.status_code, 403) # noqa: PT009 | ||
|
|
||
| def test_editor_can_view_advanced_settings(self, mock_flag): |
There was a problem hiding this comment.
Editor and Auditor tests are very similar. Could we use ddt to avoid code repetition?
There was a problem hiding this comment.
While running some other tests, I accessed the URL http://apps.local.openedx.io:2001/authoring/course/course-v1:OpenedX+DemoX+DemoCourse/settings/advanced directly as a Course Editor to see which endpoints were being called.
I found that in addition to the advanced_settings endpoint updated in the PR, the /api/contentstore/v1/proctoring_errors/course-v1:OpenedX+DemoX+DemoCourse endpoint is also called.
The second endpoint is returning a 403. Should we also update the permission validation on that endpoint so that it can retrieve the proctoring errors?
UPDATE:
Perhaps the solution would be that for the write access_type we use COURSES_MANAGE_ADVANCED_SETTINGS, and for read and feature_restricted we check against COURSES_VIEW_ADVANCED_SETTINGS?
3 participants
Description
Splits the advanced settings permission check so that read access uses
courses.view_advanced_settingsand write access usescourses.manage_advanced_settings.Previously, the backend used
COURSES_MANAGE_ADVANCED_SETTINGSfor both read and write access when authz was enabled. This PR splits the check so that GET usesCOURSES_VIEW_ADVANCED_SETTINGSand PATCH usesCOURSES_MANAGE_ADVANCED_SETTINGS. A corresponding frontend change is needed to checkview_advanced_settingsfor page access (instead of onlymanage_advanced_settings) and render read-only mode when the user lacks the manage permission. Per the design decisions in openedx/openedx-authz#283, users with any course role should be able to see advanced settings in read-only mode.User roles impacted: Course Editor, Course Auditor — can now view (but not edit) advanced settings when authz is enabled.
Changes:
common/djangoapps/student/auth.py: ImportCOURSES_VIEW_ADVANCED_SETTINGS, use it foraccess_type="read"in the authz branchRollback: Gated behind
AUTHZ_COURSE_AUTHORING_FLAG— disabling the flag reverts to legacy behavior.Supporting information
courses.view_advanced_settingsenforcement in openedx-platform openedx-authz#392courses.view_advanced_settingspermission openedx-authz#328, Design: Course Editor & Course Auditor screen restrictions openedx-authz#283Testing instructions
AUTHZ_COURSE_AUTHORING_FLAGfor a coursecourse_editorrole on that courseGET /api/contentstore/v0/advanced_settings/{course_id}→ should return 200PATCH /api/contentstore/v0/advanced_settings/{course_id}→ should return 403course_auditor— same expected resultscourse_staff— both GET and PATCH should return 200Deadline
None
AI Usage
Kiro was used as a development partner throughout this PR. I directed the implementation approach, defined scope, reviewed generated code, and made design decisions — Kiro researched the codebase, wrote the implementation and tests, ran lint/type checks, and iterated on fixes. I verified the changes manually against a local Tutor dev environment and reviewed the final diffs before submitting.