feat: introduce authz_permission_required decorator#38156
feat: introduce authz_permission_required decorator#38156dwong2708 wants to merge 7 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @dwong2708! 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. |
bce4a4a to
1c4fe82
Compare
rodmgwgu
left a comment
There was a problem hiding this comment.
Looking good, just one logic change and some minor comments.
- Add decorator to enforce AuthZ course permissions - Apply it to the Course Quality view
b4cdb3f to
582c8bc
Compare
rodmgwgu
left a comment
There was a problem hiding this comment.
Looks good, thanks! Tested in my local and works as expected.
bmtcril
left a comment
There was a problem hiding this comment.
Tested locally and works well for me 👍
mariajgrimaldi
left a comment
There was a problem hiding this comment.
A few comments for you to review and apply if they make sense! Thanks so much for this. Also, some time ago, there was this discussion about when to implement a new Django app. We settled on trying to justify their creation via an ADR. Do you think we can write something for this app? Here's an example: https://github.com/openedx/openedx-platform/blob/831af2be3774eb73d3d2a4affcb6f577ae26e61e/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst. A good middle ground to avoid writing an ADR is to write a README for the app, so others understand our reasoning behind this. Thank you so much!
| from django.apps import AppConfig | ||
|
|
||
|
|
||
| class AuthzConfig(AppConfig): |
There was a problem hiding this comment.
A docstring here describing the app would be useful.
| class AuthzConfig(AppConfig): | ||
| default_auto_field = 'django.db.models.BigAutoField' | ||
| name = 'openedx.core.djangoapps.authz' | ||
| verbose_name = "Authz" |
There was a problem hiding this comment.
What about:
| verbose_name = "Authz" | |
| verbose_name = "Open edX Authorization Framework" |
|
|
||
|
|
||
| legacy_permission_handler_map = { | ||
| "read": has_studio_read_access, | ||
| "write": has_studio_write_access, | ||
| } |
There was a problem hiding this comment.
I think a middle ground between using "read" and "write" literals without much context could be this:
| legacy_permission_handler_map = { | |
| "read": has_studio_read_access, | |
| "write": has_studio_write_access, | |
| } | |
| class LegacyAuthoringPermissions: | |
| # Thorough docstring explaining what this is for reference | |
| READ = "read" | |
| WRITE = "write" | |
| legacy_permission_handler_map = { | |
| LegacyAuthoringPermissions.READ: has_studio_read_access, | |
| LegacyAuthoringPermissions.WRITE: has_studio_write_access, | |
| } | |
We can use the LegacyAuthoringPermissions:
@authz_permission_required(COURSES_VIEW_COURSE.identifier, legacy_permission=LegacyAuthoringPermissions.READ)
| Decorator enforcing course author permissions via AuthZ | ||
| with optional legacy fallback. |
There was a problem hiding this comment.
What about?
| Decorator enforcing course author permissions via AuthZ | |
| with optional legacy fallback. | |
| Decorator enforcing course author permissions via AuthZ with optional legacy fallback. | |
| This decorator checks if the requesting user has the specified AuthZ permission for the course. | |
| If AuthZ is not enabled for the course, and a legacy_permission is provided, it falls back to checking | |
| the legacy permission. | |
| Raises: | |
| PermissionDenied: If the user does not have the required permissions. |
| } | ||
|
|
||
|
|
||
| def authz_permission_required(authz_permission, legacy_permission=None): |
There was a problem hiding this comment.
I think we should keep on using types for better control:
| def authz_permission_required(authz_permission, legacy_permission=None): | |
| def authz_permission_required(authz_permission: str, legacy_permission: Optional[str] = None) -> callable: |
| ): | ||
| raise DeveloperErrorViewMixin.api_error( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| developer_message="The requesting user does not have course author permissions.", |
There was a problem hiding this comment.
Should we be this specific with this error? I've always seen "You do not have permission to perform this action" like in DRF, this way we avoid changing the error with every implementation in case we adopt this approach in other places of the code.
| def user_has_course_permission(user, authz_permission, course_key, legacy_permission=None): | ||
| """ | ||
| Core authorization logic. | ||
| """ |
There was a problem hiding this comment.
| def user_has_course_permission(user, authz_permission, course_key, legacy_permission=None): | |
| """ | |
| Core authorization logic. | |
| """ | |
| def user_has_course_permission(user: User, authz_permission: str, course_key: CourseKey, legacy_permission: Optional[str] = None): | |
| """ | |
| Checks if the user has the specified AuthZ permission for the course, with optional fallback to legacy permissions. | |
| """ |
There was a problem hiding this comment.
This could help us reduce descriptive inline comments and keep them for explaining why we are doing something.
|
|
||
| # If AuthZ is not enabled for this course, fall back to legacy course author | ||
| # access check if legacy_permission is provided. | ||
| has_legacy_permission = legacy_permission_handler_map.get(legacy_permission) |
There was a problem hiding this comment.
| has_legacy_permission = legacy_permission_handler_map.get(legacy_permission) | |
| has_legacy_permission: Optional[callable] = legacy_permission_handler_map.get(legacy_permission) |
| return False | ||
|
|
||
|
|
||
| def get_course_key(course_id): |
There was a problem hiding this comment.
| def get_course_key(course_id): | |
| def get_course_key(course_id: str) -> CourseKey: | |
| 'openedx.core.djangoapps.notifications', | ||
|
|
||
| # Authz | ||
| 'openedx.core.djangoapps.authz.apps.AuthzConfig', |
There was a problem hiding this comment.
Shouldn't this be in cms/envs/common.py as well? Also, Django should automatically discover apps.AuthzConfig, so we can just add:
| 'openedx.core.djangoapps.authz.apps.AuthzConfig', | |
| 'openedx.core.djangoapps.authz', |
Resolves openedx/openedx-authz#200
Description
This PR introduces a new decorator to enforce permissions using the new AuthZ system and applies it to the Course Quality and Course Validations endpoints.
The decorator centralizes the authorization logic for course authoring APIs by checking permissions through the AuthZ service, with an optional fallback to the legacy permission system during the migration period.
As part of implementing the new permissions for the Checklist section of the course, the decorator is now applied to the following endpoints:
This ensures that access to checklist-related functionality is properly controlled by the new permission model.
Changes
Testing
Context
This change is part of the work to support RBAC/AuthZ permissions for the Checklist section in course authoring.