feat: make scope optional for user action validation#348
Conversation
|
Thanks for the pull request, @dcoa! 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. |
f8ae234 to
c131225
Compare
BryanttV
left a comment
There was a problem hiding this comment.
I tested this locally and it works as expected! Just a few minor comments. Thanks!
| scope; when omitted, the permission is validated across any scope. | ||
| """ | ||
|
|
||
| scope = serializers.CharField(max_length=255, required=False, allow_null=True) |
There was a problem hiding this comment.
| scope = serializers.CharField(max_length=255, required=False, allow_null=True) | |
| scope = serializers.CharField(max_length=255, required=False) |
| def to_representation(self, instance: dict) -> dict: | ||
| """Serialize the result, omitting ``scope`` when the request had no scope.""" | ||
| representation = super().to_representation(instance) | ||
| if instance.get("scope") is None: | ||
| representation.pop("scope", None) | ||
| return representation |
There was a problem hiding this comment.
Without the allow_null this override is not necessary.
| ("nonexistent_user", permissions.MANAGE_LIBRARY_TEAM.identifier, False), | ||
| ) | ||
| @unpack | ||
| def test_is_user_allowed_in_any_scope(self, username, action, expected_result): |
There was a problem hiding this comment.
Could we also add test cases for course permissions?
| ) | ||
| self.assertEqual(result, expected_result) | ||
|
|
||
| def test_is_user_allowed_in_any_scope_staff_always_allowed(self): |
There was a problem hiding this comment.
Could we add a test case for is_superuser=True?
|
@BryanttV thank you for the feedback, I addressed your comments. |
Description
The current PR adds the ability to validate whether the authenticated user holds a permission in any scope.
Changes
API layer (openedx_authz/api/users.py)
is_user_allowed_in_any_scope(user_external_key, action_external_key): returns True for staff/superusers, otherwise True if the user holds the permission in at least one scope.REST API (openedx_authz/rest_api/v1/)
/api/authz/v1/permissions/validate/mescope is now optional per permission object:is_user_allowed); the response echoes scope.is_user_allowed_in_any_scope); the response omits scope.Context
This supports the Admin Console's Roles and Permissions Management view, where the Assign Role button should only render when the user has at least one permission to manage library or course roles; a check that isn't tied to a single scope.
It follows the proposal in openedx/wg-build-test-release#603
Testing instructions
/api/authz/v1/permissions/validate/mewith scope-less items and confirm allowed reflects whether the user holds each action in any scope.allowed: true.Merge checklist:
Check off if complete or not applicable:
AI use acknowledgement
Supported by Claude Code