-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: authorize advanced settings endpoints via openedx-authz when fl… #38009
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,10 @@ | |
| from django.conf import settings | ||
| from django.core.exceptions import PermissionDenied | ||
| from opaque_keys.edx.locator import LibraryLocator | ||
| from openedx_authz import api as authz_api | ||
| from openedx_authz.constants.permissions import MANAGE_ADVANCED_SETTINGS | ||
|
|
||
| from openedx.core import toggles as core_toggles | ||
| from common.djangoapps.student.roles import ( | ||
| CourseBetaTesterRole, | ||
| CourseCreatorRole, | ||
|
|
@@ -166,6 +169,34 @@ def has_studio_advanced_settings_access(user): | |
| ) | ||
|
|
||
|
|
||
| def check_course_advanced_settings_access(user, course_key, access_type='read'): | ||
| """ | ||
| Check if user has access to advanced settings for a course. | ||
|
|
||
| Uses openedx-authz when AUTHZ_COURSE_AUTHORING_FLAG is enabled, | ||
| otherwise falls back to legacy permission checks. | ||
|
|
||
| Args: | ||
| user: Django user object | ||
| course_key: CourseKey for the course | ||
| access_type: Type of access to check. Options: | ||
| - 'read': Check read access (default) | ||
| - 'write': Check write access | ||
| - 'advanced_settings': Check advanced settings access (DISABLE_ADVANCED_SETTINGS feature) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this comment is correct at this point?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indirectly via
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make the access_type option more descriptive? perhaps 'legacy_disable_advanced_settings_flag_check' or something like that? |
||
|
|
||
| Returns: | ||
| bool: True if user has permission, False otherwise | ||
| """ | ||
| if core_toggles.AUTHZ_COURSE_AUTHORING_FLAG.is_enabled(course_key): | ||
| return authz_api.is_user_allowed(user.username, MANAGE_ADVANCED_SETTINGS.identifier, str(course_key)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous version of this check also checked whether the DISABLE_ADVANCED_SETTINGS feature flag was set. Is that no longer necessary?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only path that checks this feature flag is when the proctoring errors view calls has_studio_advanced_settings_access. This path still executes, as I've wrapped So, the check still happens if the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking through the permission check for the proctoring errors view. Right now this is how the permission check works: The first thing to note here is that if Secondly, if
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mmm so the idea is that with MANAGE_ADVANCED_SETTINGS, we should be able to have the granularity to enable or disable advanced settings for specific users, so DISABLE_ADVANCED_SETTINGS won't be necessary anymore. I would vote then to only check for MANAGE_ADVANCED_SETTINGS if the flag is enabled. |
||
| if access_type == 'read': | ||
| return has_studio_read_access(user, course_key) | ||
| if access_type == 'advanced_settings': | ||
| return has_studio_advanced_settings_access(user) | ||
| if access_type == 'write': | ||
| return has_studio_write_access(user, course_key) | ||
|
|
||
|
|
||
| def has_studio_read_access(user, course_key): | ||
| """ | ||
| Return True if user is allowed to view this course/library in studio. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is being used for more than just advanced settings checks, should it be renamed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the advanced settings page, the frontend calls the
AdvancedCourseSettingsView(get/patch) and theProctoringErrorsView(get). I wrapped all of those checks into this function. Since they are all advanced settings related, and since theProctoringErrorsViewcheck already called a function with "advanced settings" in the name, I thought this would be appropriate.My first approach was to replace the current checks with something like:
but I didn't love the repetition, nesting, and having to import everything needed in both modules, so I wrapped all the checks.