fix: include ancestor scopes when filtering assignments by scope - #483
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @carlos-marquez-wgu! 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. |
702426b to
18be5b2
Compare
|
Hi @openedx/committers-openedx-authz, this PR is ready for review |
mphilbrick211
added
the
mao-onboarding
|
Hi @carlos-marquez-wgu, please, could you resolve the conflicts? |
18be5b2 to
9a75355
Compare
Done @BryanttV , 😃 |
|
About the concern with AssignmentsAPIView.get, in my opinion this would be a desired behavior also there, so it should be ok. |
|
nit: In the PR description the example request uses "scope" for the query param, but it should be "scopes" (pural). |
rodmgwgu
left a comment
rodmgwgu
left a comment
There was a problem hiding this comment.
Tested in my local, working as expected, thanks!
| return get_all_subject_role_assignments_in_scope(ScopeData(external_key=scope_external_key)) | ||
|
|
||
|
|
||
| def _expand_scopes_with_ancestors(scopes: list[str]) -> set[str]: |
There was a problem hiding this comment.
nit: Not sure if this is too late to change, but I wonder if this helper would make more sense as a method on the scope class itself, since it’s essentially responsible for recursively finding the ancestors of a given scope and associating it with the proper definitions. It would also help avoid adding more and more loose helper methods as this logic grows.
There was a problem hiding this comment.
Yeah it makes sense, thanks for the suggestion! I refactored it to place the method inside the scope class
The scope filter in _filter_candidate_assignments_by_params matched only exact scope keys, so querying a specific course or library did not return assignments granted at the org or platform level. Add _expand_scopes_with_ancestors to derive org-glob and platform-glob parent keys for each queried scope using the existing ScopeMeta registries, and expand the match set before filtering. Hierarchy: specific → org glob → platform glob - course-v1:Org+Course+Run → course-v1:Org+* → course-v1:* - lib:Org:Slug → lib:Org:* → lib:*
…s and inherited classes
9a75355 to
1952afd
Compare
6 participants
Description
The scope filter in
_filter_candidate_assignments_by_paramsmatched only exact scope keys, so querying a specific course or library did not return assignments granted at the org or platform level, this PR addresses this.Changes
Add
_expand_scopes_with_ancestorsto deriveorg-globandplatform-globparent keys for each queried scope using the existingScopeMetaregistries, and expand the match set before filtering.Hierarchy:
specific→org glob→platform globcourse-v1:Org+Course+Run→course-v1:Org+*→course-v1:*lib:Org:Slug→lib:Org:*→lib:*Manual testing
Example
Request
GET /api/authz/v1/users/?scopes=course-v1:OpenedX+DemoX+DemoCourse,lib%3ACORG%3ALIB101Response body (before)
{ "count": 1, "next": null, "previous": null, "results": [ { "username": "lord.admin", "full_name": "", "email": "admin@example.com", "assignment_count": 1, "assignments": [ { "role": "library_admin", "org": "CORG", "scope": "lib:CORG:LIB101", "permission_count": 11, "scope_display_name": "MyLibrary" } ] } ] }Response body (after)
{ "count": 2, "next": null, "previous": null, "results": [ { "username": "lord.admin", "full_name": "", "email": "admin@example.com", "assignment_count": 2, "assignments": [ { "role": "library_admin", "org": "CORG", "scope": "lib:CORG:LIB101", "permission_count": 11, "scope_display_name": "MyLibrary" }, { "role": "course_editor", "org": "*", "scope": "course-v1:*", "permission_count": 22, "scope_display_name": "" } ] }, { "username": "test.user.course_editor", "full_name": "", "email": "course_editor@example.com", "assignment_count": 2, "assignments": [ { "role": "library_user", "org": "*", "scope": "lib:*", "permission_count": 3, "scope_display_name": "" }, { "role": "course_staff", "org": "OpenedX", "scope": "course-v1:OpenedX+*", "permission_count": 31, "scope_display_name": "" } ] } ] }Screenshots
Note: using @dcoa work from PR openedx/frontend-app-admin-console#222
Before
After
Concerns
This change also affects the endpoint
AssignmentsAPIView.getsince it ultimately uses the same filtering function. Question, are we okay with this or should this path be isolated?I didn't isolated this yet since this could be a desired effect due to the current Team Members figma design
(see is filtered by scope but it still shows higher scopes too)
Merge checklist:
Check off if complete or not applicable:
AI Usage
Kiro + Claude were used to assist with the creation of the tests and code implementations while throughly and carefully guided, everything was reviewed and corrected manually by an actual person and ensured the changes were up to standard and met the closing issue requirements.
Closes #457