-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[FC-0099] feat: filter libraries based on user-role scopes #37564
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
Changes from all commits
ce3faea
db198a8
d26cbc8
b2a5c45
f06062e
7816254
af8a2e4
c614b34
d1d98e3
621c07c
0b6c3ee
d3d35ab
e2d6a97
0ac96bc
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 |
|---|---|---|
|
|
@@ -2,8 +2,12 @@ | |
| Permissions for Content Libraries (v2, Learning-Core-based) | ||
| """ | ||
| from bridgekeeper import perms, rules | ||
| from bridgekeeper.rules import Attribute, ManyRelation, Relation, blanket_rule, in_current_groups | ||
| from bridgekeeper.rules import Attribute, ManyRelation, Relation, blanket_rule, in_current_groups, Rule | ||
| from django.conf import settings | ||
| from django.db.models import Q | ||
|
|
||
| from openedx_authz import api as authz_api | ||
| from openedx_authz.constants.permissions import VIEW_LIBRARY | ||
|
|
||
| from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission | ||
|
|
||
|
|
@@ -54,6 +58,154 @@ def is_course_creator(user): | |
|
|
||
| return get_course_creator_status(user) == 'granted' | ||
|
|
||
|
|
||
| class HasPermissionInContentLibraryScope(Rule): | ||
| """Bridgekeeper rule that checks content library permissions via the openedx-authz system. | ||
|
|
||
| This rule integrates the openedx-authz authorization system (backed by Casbin) with | ||
| Bridgekeeper's declarative permission system. It checks if a user has been granted a | ||
| specific permission (action) through their role assignments in the authorization system. | ||
|
|
||
| The rule works by: | ||
| 1. Querying the authorization system to find library scopes where the user has this permission | ||
| 2. Parsing the library keys (org/slug) from the scopes | ||
| 3. Building database filters to match ContentLibrary models with those org/slug combinations | ||
|
|
||
| Attributes: | ||
| permission (PermissionData): The permission object representing the action to check | ||
| (e.g., 'view', 'edit'). This is used to look up scopes in the authorization system. | ||
|
|
||
| filter_keys (list[str]): The Django model fields to use when building QuerySet filters. | ||
| Defaults to ['org', 'slug'] for ContentLibrary models. | ||
|
|
||
| These fields are used to construct the Q object filters that match libraries | ||
| based on the parsed components from library keys in authorization scopes. | ||
|
|
||
| For ContentLibrary, library keys have the format 'lib:ORG:SLUG', which maps to: | ||
| - 'org' -> filters on org__short_name (related Organization model) | ||
| - 'slug' -> filters on slug field | ||
|
|
||
| If filtering by different fields is needed, pass a custom list. For example: | ||
| - ['org', 'slug'] - default for ContentLibrary (filters by org and slug) | ||
| - ['id'] - filter by primary key (for other models) | ||
|
|
||
| Examples: | ||
| Basic usage with default filter_keys: | ||
| >>> from bridgekeeper import perms | ||
| >>> from openedx.core.djangoapps.content_libraries.permissions import HasPermissionInContentLibraryScope | ||
| >>> | ||
| >>> # Uses default filter_keys=['org', 'slug'] for ContentLibrary | ||
| >>> can_view = HasPermissionInContentLibraryScope('view_library') | ||
| >>> perms['libraries.view_library'] = can_view | ||
|
|
||
| Compound permissions with boolean operators: | ||
| >>> from bridgekeeper.rules import Attribute | ||
| >>> | ||
| >>> is_active = Attribute('is_active', True) | ||
| >>> is_staff = Attribute('is_staff', True) | ||
| >>> can_view = HasPermissionInContentLibraryScope('view_library') | ||
| >>> | ||
| >>> # User must be active AND (staff OR have explicit permission) | ||
| >>> perms['libraries.view_library'] = is_active & (is_staff | can_view) | ||
|
|
||
| QuerySet filtering (efficient, database-level): | ||
| >>> from openedx.core.djangoapps.content_libraries.models import ContentLibrary | ||
| >>> | ||
| >>> # Gets all libraries user can view in a single SQL query | ||
| >>> visible_libraries = perms['libraries.view_library'].filter( | ||
| ... request.user, | ||
| ... ContentLibrary.objects.all() | ||
| ... ) | ||
|
|
||
| Individual object checks: | ||
| >>> library = ContentLibrary.objects.get(org__short_name='DemoX', slug='CSPROB') | ||
| >>> if perms['libraries.view_library'].check(request.user, library): | ||
| ... # User can view this specific library | ||
|
|
||
| Note: | ||
| The library keys in authorization scopes must have the format 'lib:ORG:SLUG' | ||
| to match the ContentLibrary model's org.short_name and slug fields. | ||
| For example, scope 'lib:DemoX:CSPROB' matches a library with | ||
| org.short_name='DemoX' and slug='CSPROB'. | ||
| """ | ||
|
|
||
| def __init__(self, permission: authz_api.PermissionData, filter_keys: list[str] | None = None): | ||
| """Initialize the rule with the action and filter keys to filter on. | ||
|
|
||
| Args: | ||
| permission (PermissionData): The permission to check (e.g., 'view', 'edit'). | ||
| filter_keys (list[str]): The model fields to filter on when building QuerySet filters. | ||
| Defaults to ['org', 'slug'] for ContentLibrary. | ||
| """ | ||
| self.permission = permission | ||
| self.filter_keys = filter_keys if filter_keys is not None else ["org", "slug"] | ||
|
|
||
| def query(self, user): | ||
| """Convert this rule to a Django Q object for QuerySet filtering. | ||
|
|
||
| Args: | ||
| user: The Django user object (must have a 'username' attribute). | ||
|
|
||
| Returns: | ||
| Q: A Django Q object that can be used to filter a QuerySet. | ||
| The Q object combines multiple conditions using OR (|) operators, | ||
| where each condition matches a library's org and slug fields: | ||
| Q(org__short_name='OrgA' & slug='lib-a') | Q(org__short_name='OrgB' & slug='lib-b') | ||
|
|
||
| Example: | ||
| >>> # User has 'view' permission in scopes: ['lib:OrgA:lib-a', 'lib:OrgB:lib-b'] | ||
| >>> rule = HasPermissionInContentLibraryScope('view', filter_keys=['org', 'slug']) | ||
| >>> q = rule.query(user) | ||
| >>> # Results in: Q(org__short_name='OrgA', slug='lib-a') | Q(org__short_name='OrgB', slug='lib-b') | ||
| >>> | ||
| >>> # Apply to queryset | ||
| >>> libraries = ContentLibrary.objects.filter(q) | ||
| >>> # SQL: SELECT * FROM content_library | ||
| >>> # WHERE (org.short_name='OrgA' AND slug='lib-a') | ||
| >>> # OR (org.short_name='OrgB' AND slug='lib-b') | ||
| """ | ||
| scopes = authz_api.get_scopes_for_user_and_permission( | ||
| user.username, | ||
| self.permission.identifier | ||
| ) | ||
|
|
||
| library_keys = [scope.library_key for scope in scopes] | ||
|
|
||
| if not library_keys: | ||
| return Q(pk__in=[]) # No access, return Q that matches nothing | ||
|
|
||
| # Build Q object: OR together (org AND slug) conditions for each library | ||
| query = Q() | ||
| for library_key in library_keys: | ||
| query |= Q(org__short_name=library_key.org, slug=library_key.slug) | ||
|
|
||
| return query | ||
|
|
||
| def check(self, user, instance, *args, **kwargs): # pylint: disable=arguments-differ | ||
| """Check if user has permission for a specific object instance. | ||
|
|
||
| This method is used for checking permission on individual objects rather | ||
| than filtering a QuerySet. It extracts the scope from the object and | ||
| checks if the user has the required permission in that scope via Casbin. | ||
|
|
||
| Args: | ||
| user: The Django user object (must have a 'username' attribute). | ||
| instance: The Django model instance to check permission for. | ||
| *args: Additional positional arguments (for compatibility with parent signature). | ||
| **kwargs: Additional keyword arguments (for compatibility with parent signature). | ||
|
|
||
| Returns: | ||
| bool: True if the user has the permission in the object's scope, | ||
| False otherwise. | ||
|
|
||
| Example: | ||
| >>> rule = HasPermissionInContentLibraryScope('view') | ||
| >>> can_view = rule.check(user, library) | ||
| >>> # Checks if user has 'view' permission in scope 'lib:DemoX:CSPROB' | ||
| """ | ||
| return authz_api.is_user_allowed(user.username, self.permission.identifier, str(instance.library_key)) | ||
|
|
||
|
|
||
| ########################### Permissions ########################### | ||
|
|
||
| # Is the user allowed to view XBlocks from the specified content library | ||
|
|
@@ -87,7 +239,9 @@ def is_course_creator(user): | |
| is_global_staff | | ||
| # Libraries with "public read" permissions can be accessed only by course creators | ||
| (Attribute('allow_public_read', True) & is_course_creator) | | ||
| # Otherwise the user must be part of the library's team | ||
| # Users can access libraries within their authorized scope (via Casbin/role-based permissions) | ||
| HasPermissionInContentLibraryScope(VIEW_LIBRARY) | | ||
| # Fallback to: the user must be part of the library's team (legacy permission system) | ||
|
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. Do we still want this? I thought we were not going to be using the legacy permissions any more since there will be no UI to revoke them?
Member
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. That's what I think should happen, though I'm not sure it should be part of this PR. It would also mean removing
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 think the states will diverge either way, but I'm ok with leaving this as long as we have an issue to remove it that has a list of places we need to update 👍
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. @mariajgrimaldi @bmtcril, in #37501 I was maintaining the old check for prevention. But in a DEPR WG meeting they mention the concept of expand contract pattern, that it is related. For Ulmo we are only expanding (adding the new functionality withouth removing the old one), and then we can remove it.
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. Yes, I think as long as we're not actually using the old permissions that's fine and gives instances the opportunity to roll back if necessary. I just want to make sure we're not in a position where we are checking old permissions that cannot be changed. |
||
| has_explicit_read_permission_for_library | ||
| ) | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.