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
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.
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?
| ) | ||
| 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.
Are we testing the editor can write code branch as well?
There was a problem hiding this comment.
Yes, there is a test ensuring that a course editor cannot modify advanced settings (ref - see explicit exclusions).
mphilbrick211
added
the
mao-onboarding
I see that the proctoring_errors endpoint returns the following: { I'm not sure what's that used for, perhaps to show an error banner is there are any proctored errors? Anyway, as it's a GET request, I think a COURSES_VIEW_ADVANCED_SETTINGS permission should be enough to enable it, so we do need to update that endpoint to allow that permission. |
|
When accessing the Advanced Settings, the Proctoring Errors endpoint is also called. This endpoint is not related to reviewing student exam attempts or anything similar. It is simply a configuration validator for the proctoring fields that live within the course's Advanced Settings. It checks the following fields:
and generates warnings depending on the configuration. This endpoint is used by the frontend to render an alert banner if there are any errors. The endpoint ( def has_studio_advanced_settings_access(user):
"""
If DISABLE_ADVANCED_SETTINGS feature is enabled, only Django Superuser
or Django Staff can access "Advanced Settings".
By default, this feature is disabled.
"""
return (
not getattr(settings, 'DISABLE_ADVANCED_SETTINGS', False)
or user.is_staff
or user.is_superuser
)In other words, any authenticated user would have permission to query the proctoring errors if the What I think (and I agree with Rodrigo) we should do in this case is allow users with roles that have the |
rodmgwgu
left a comment
rodmgwgu
left a comment
There was a problem hiding this comment.
Code looks good, also tested in my local for the auditor, editor and staff roles and all worked as advertised. Thanks!
Just let's decide on what to do with the proctoring_error endpoint, should we include the change to that one here? or do it as a separate PR?
Oh that's great, yeah looks good to me, let's just get approval from product and then we should be good to go. |
@gviedma-aulasneo approved this approach in our weekly meeting today. |
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. The 'feature_restricted' access type also checks COURSES_VIEW_ADVANCED_SETTINGS (not manage), so view-only roles can reach the proctoring errors endpoint that the advanced settings page calls on load; previously this returned 403 for editors/auditors.
ec30500 to
e6260b7
Compare
6 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.