Conversation
…ion identifier check
|
Thanks for the pull request, @BryanttV! 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. |
There was a problem hiding this comment.
Thank you so much for this! I haven't tested this so this first review is only a static scan of the code. My main concern is how to maintain the extensibility we've built so far with multiple classes, and whether we could leverage that to include this (globs) as a new scope. Curious to know what you think!
| if GLOBAL_SCOPE_WILDCARD in external_key: | ||
| return external_key.startswith(cls.NAMESPACE) and external_key.endswith(GLOBAL_SCOPE_WILDCARD) | ||
|
|
There was a problem hiding this comment.
Can glob be a new scope type so we can support it natively with the mechanisms we already have in place (inheritance, etc) like any other scope?
| Raises: | ||
| ValueError: If the scope string contains invalid glob patterns. | ||
| """ | ||
| validate_scope_with_glob(scope) |
There was a problem hiding this comment.
Shouldn't this be part of validating the key of the scope? As when validating whether a course id is valid.
| Examples: | ||
| Valid scopes: | ||
| - CourseOverviewData(external_key="course-v1:OpenedX*") # org-level wildcard |
There was a problem hiding this comment.
So if I have OpenedXV2 as an organization and I assign a role in the scope OpenedX* then I'd also be assigning roles in OpenedXV2?
There was a problem hiding this comment.
Good catch, perhaps we would need to include the separator to make sure this doesn't happen, like:
lib:OpenedX:*
course-v1:OpenedX+*
| @@ -0,0 +1,149 @@ | |||
| """Validation utilities for OpenedX AuthZ API. | |||
There was a problem hiding this comment.
I wonder if all this validations should be part of our data structures instead of be independent here.
| parts = scope_prefix.split(EXTERNAL_KEY_SEPARATOR) | ||
|
|
||
| if len(parts) != 2 or parts[1] == "": |
There was a problem hiding this comment.
Can we add an inline comment here? What kind of examples will raise the value error?
| def _course_org_exists(org: str) -> bool: | ||
| """Check if there is at least one course with the given org. | ||
| Args: | ||
| org (str): Organization identifier extracted from the course scope | ||
| Returns: | ||
| bool: True if there is at least one CourseOverview whose org field matches | ||
| the provided identifier in a case-sensitive way, False otherwise. | ||
| """ | ||
| from openedx_authz.models.scopes import CourseOverview # pylint: disable=import-outside-toplevel | ||
|
|
||
| course_obj = CourseOverview.objects.filter(org=org).only("org").last() | ||
| return course_obj is not None and course_obj.org == org | ||
|
|
||
|
|
||
| def _library_org_exists(org: str) -> bool: | ||
| """Check if there is at least one content library with the given org. | ||
| Args: | ||
| org (str): Organization identifier extracted from the library scope | ||
| Returns: | ||
| bool: True if there is at least one ContentLibrary whose related | ||
| organization's short_name matches the provided identifier in a | ||
| case-sensitive way, False otherwise. | ||
| """ | ||
| from openedx_authz.models.scopes import ContentLibrary # pylint: disable=import-outside-toplevel | ||
|
|
||
| lib_obj = ContentLibrary.objects.filter(org__short_name=org).only("org").last() | ||
| return lib_obj is not None and lib_obj.org.short_name == org |
There was a problem hiding this comment.
I thought we already had something similar in data.py? https://github.com/eduNEXT/openedx-authz/blob/200cf5ca6499c1e94414f21e33e3c3472f851659/openedx_authz/api/data.py#L459, what would be the difference between these two?
| # For glob scopes we don't create a Scope object since | ||
| # they don't represent a specific content library | ||
| if GLOBAL_SCOPE_WILDCARD in scope.external_key: | ||
| return None | ||
|
|
There was a problem hiding this comment.
Is there a way, by even creating a glob scope data structure or using a mixing, of not having to include these lines each time we have to implement a new scope?
|
Hi @mariajgrimaldi, thanks for the review!
I definitely agree with your approach. I think we could handle new scopes specifically at the glob level, for example, in this case, something like |
Closes: #172
Description
This PR implements organization-level glob pattern matching for role assignments in the Casbin enforcer, enabling administrators to assign roles across all courses or libraries within a specific organization using a single policy rule.
Previously, the AuthZ system only supported:
lib:WGU:CSPROBorcourse-v1:OpenedX+DemoX+DemoCourse)Now, administrators can assign roles using organization-level glob patterns:
lib:DemoX*(matches all libraries in DemoX org)course-v1:OpenedX*(matches all courses in OpenedX org)Changes
Core Functionality
Domain Matching Function: Added
key_match_functo the Casbin enforcer to enable glob-like pattern matching at the end of scope stringsAuthzEnforcer._initialize_enforcer()to register the matching function viaenforcer.add_named_domain_matching_func("g", key_match_func)course-v1:OpenedX*Validation Rules:
course-v1:OpenedX*,lib:DemoX*(org-level wildcards)course-v1*(wildcard before org)course-v1:*(wildcard without org prefix)course-v1:OpenedX+Course*(wildcard at course level)lib:DemoX:Slug*(wildcard at slug level)Example Use Case
Before (required one role assignment per course):
After (single org-level assignment):
Permission Check:
Technical Details
Casbin Model Behavior
The
key_match_funcfunction fromcasbin.utilenables pattern matching where:course-v1:OpenedX*matches any string starting withcourse-v1:OpenedXlib:DemoX*matches any string starting withlib:DemoXThis is applied to the domain (scope) parameter of the "g" grouping function in the Casbin model, allowing role assignments with glob patterns to match concrete resource scopes during permission checks.
Security Considerations
Test Plan
Related Documentation
Merge checklist
Check off if complete or not applicable: