From ce3faea137eca729b5d8f14b51550683cc7a4c0b Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 28 Oct 2025 16:45:09 +0100 Subject: [PATCH 01/14] feat: use filter based on scopes alongside permissions dict (bridgekeeper) --- .../content_libraries/permissions.py | 180 +++++++++++- .../tests/test_content_libraries.py | 260 ++++++++++++++++++ 2 files changed, 438 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index 4e72381986ed..d13eb2126597 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -2,8 +2,11 @@ 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.api.users import is_user_allowed, get_scopes_for_user_and_permission from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission @@ -54,6 +57,177 @@ def is_course_creator(user): return get_course_creator_status(user) == 'granted' + +class HasPermissionInContentLibraryScope(Rule): + """Bridgekeeper rule that checks permissions via Casbin's policy engine. + + This rule integrates Casbin's role-based authorization with Bridgekeeper's + declarative permission system. It checks if a user has been granted a specific + permission (action) through their role assignments in Casbin. + + The rule works by: + 1. Querying Casbin grouping policies to find the user's role assignments + 2. Querying Casbin permission policies to find which roles grant the action + 3. Matching role assignments with scopes to determine where the user has permission + + This enables both individual object permission checks and efficient QuerySet + filtering - a key feature that allows database-level filtering instead of + checking each object individually. + + Attributes: + action_external_key (str): The action/permission to check (e.g., 'view', 'edit'). + This should be the external key WITHOUT the namespace prefix. + For example, use 'view' not 'act^view'. + + scope_field (str): The Django model field/property that contains the scope identifier. + This tells the rule WHERE to find the scope value in your model. + Defaults to 'id'. + + **IMPORTANT**: This can be a model property (like `library_key`) or a field. + For ContentLibrary, use 'library_key' which is a @property that returns + the LibraryLocatorV2 string representation. + + The scope_field serves two purposes: + - **For QuerySet filtering**: Builds SQL like `WHERE scope_field IN (...)` + - **For object checks**: Extracts the scope from `instance.scope_field` + + Supports Django ORM field lookups for nested fields: + - 'library_key' - a @property on the model (ContentLibrary case) + - 'id' - direct field on the model + - 'library__id' - field on a related model + - 'course__org__key' - multi-level relationship + + Examples: + Basic usage with default scope_field: + >>> from bridgekeeper import perms + >>> from openedx_authz.permissions import HasPermissionInScope + >>> + >>> # Assumes the model's 'id' field contains the scope + >>> can_view = HasPermissionInScope('view') + >>> perms['libraries.view'] = can_view + + Specifying a custom scope_field: + >>> # When scope is in a field named 'library_id' + >>> can_view = HasPermissionInScope('view', scope_field='library_id') + >>> + >>> # When scope is in a related model + >>> can_manage = HasPermissionInScope('manage', scope_field='library__key') + + Compound permissions with boolean operators: + >>> from bridgekeeper.rules import Attribute + >>> + >>> is_active = Attribute('is_active', True) + >>> is_staff = Attribute('is_staff', True) + >>> can_view = HasPermissionInScope('view', scope_field='library_id') + >>> + >>> # User must be active AND (staff OR have explicit permission) + >>> perms['libraries.view'] = 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'].filter( + ... request.user, + ... ContentLibrary.objects.all() + ... ) + + Individual object checks: + >>> library = ContentLibrary.objects.get(library_id='lib:DemoX:CSPROB') + >>> if perms['libraries.view'].check(request.user, library): + ... # User can view this specific library + ... return render_library(library) + + Note: + The scope identifiers in Casbin policies must match the values in your + Django model's scope_field. For example, if Casbin stores + 'lib:DemoX:CSPROB' and your model has library_id='lib:DemoX:CSPROB', + they must match exactly (including format and casing). + """ + + def __init__(self, action_external_key: str, filter_keys: list[str] = ["org", "slug"]): + """Initialize the rule with the action and filter keys to filter on. + + Args: + action_external_key (str): The action/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.action_external_key = action_external_key + self.filter_keys = filter_keys + + def query(self, user): + """Convert this rule to a Django Q object for QuerySet filtering. + + This method enables efficient database-level filtering by: + 1. Querying the authorization system to get ALL library scopes where the user has this permission + 2. Parsing the library keys (org/slug pairs) from the scopes + 3. Building a Django Q object that filters for libraries matching those org/slug combinations + + This avoids N+1 query problems by filtering at the database level rather + than checking permission for each object individually. + + 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 = get_scopes_for_user_and_permission( + user.username, + self.action_external_key + ) + + 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): + """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. + + Returns: + bool: True if the user has the permission in the object's scope, + False otherwise. + + Example: + >>> rule = HasPermissionInScope('view') + >>> can_view = rule.check(user, library) + >>> # Checks if user has 'view' permission in scope 'lib:DemoX:CSPROB' + """ + return is_user_allowed(user.username, self.action_external_key, str(instance.library_key)) + + ########################### Permissions ########################### # Is the user allowed to view XBlocks from the specified content library @@ -87,7 +261,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) has_explicit_read_permission_for_library ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index c4f61f47e254..8c446d294d29 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -12,8 +12,10 @@ import ddt import tomlkit +from bridgekeeper import perms from django.core.files.uploadedfile import SimpleUploadedFile from django.contrib.auth.models import Group +from django.db.models import Q from django.test import override_settings from django.test.client import Client from freezegun import freeze_time @@ -37,6 +39,7 @@ from openedx.core.djangolib.testing.utils import skip_unless_cms from ..models import ContentLibrary +from ..permissions import CAN_VIEW_THIS_CONTENT_LIBRARY, HasPermissionInContentLibraryScope @skip_unless_cms @@ -1216,6 +1219,263 @@ def test_uncaught_error_creates_error_log(self): self.assertEqual(task_data, expected) + def test_authz_scope_filters_by_authorized_libraries(self): + """ + Test that HasPermissionInContentLibraryScope rule filters libraries + based on authorized org/slug combinations. + + Given: + - 3 libraries: lib1 (org1), lib2 (org2), lib3 (org1) + - User authorized for lib1 and lib2 only + + Expected: + - Filter returns exactly 2 libraries (lib1 and lib2) + - lib3 is excluded (same org as lib1, but different slug) + - Correct org/slug combinations are matched + """ + user = UserFactory.create(username="scope_user", is_staff=False) + + Organization.objects.get_or_create(short_name="org1", defaults={"name": "Org 1"}) + Organization.objects.get_or_create(short_name="org2", defaults={"name": "Org 2"}) + + with self.as_user(self.admin_user): + lib1 = self._create_library(slug="lib1", org="org1", title="Library 1") + lib2 = self._create_library(slug="lib2", org="org2", title="Library 2") + lib3 = self._create_library(slug="lib3", org="org1", title="Library 3") + + with patch('openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission') as mock_get_scopes: + # Mock: User authorized for lib1 (org1:lib1) and lib2 (org2:lib2) only, NOT lib3 + mock_scope1 = type('Scope', (), {'library_key': LibraryLocatorV2.from_string(lib1['id'])})() + mock_scope2 = type('Scope', (), {'library_key': LibraryLocatorV2.from_string(lib2['id'])})() + mock_get_scopes.return_value = [mock_scope1, mock_scope2] + + all_libs = ContentLibrary.objects.filter(slug__in=['lib1', 'lib2', 'lib3']) + filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, all_libs) + + # TEST: Verify exactly 2 libraries returned (lib1 and lib2, not lib3) + self.assertEqual(filtered.count(), 2, "Should return exactly 2 authorized libraries") + + # TEST: Verify correct libraries are included/excluded + slugs = set(filtered.values_list('slug', flat=True)) + self.assertIn('lib1', slugs, "lib1 (org1:lib1) should be included") + self.assertIn('lib2', slugs, "lib2 (org2:lib2) should be included") + self.assertNotIn('lib3', slugs, "lib3 (org1:lib3) should be excluded") + + # TEST: Verify the org/slug combinations match + lib1_result = filtered.get(slug='lib1') + lib2_result = filtered.get(slug='lib2') + self.assertEqual(lib1_result.org.short_name, 'org1') + self.assertEqual(lib2_result.org.short_name, 'org2') + + def test_authz_scope_individual_check_with_permission(self): + """ + Test that HasPermissionInContentLibraryScope.check() returns True + when authorization is granted. + + Given: + - Non-staff user + - Library exists + - Authorization system grants permission (mocked) + + Expected: + - check() returns True + """ + user = UserFactory.create(username="check_user", is_staff=False) + + with self.as_user(self.admin_user): + lib = self._create_library(slug="check-lib", org=self.org_short_name, title="Check Library") + + library_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib["id"])) + + with patch("openedx.core.djangoapps.content_libraries.permissions.is_user_allowed", return_value=True): + result = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].check(user, library_obj) + + self.assertTrue(result, "Should return True when user is authorized") + + def test_authz_scope_individual_check_without_permission(self): + """ + Test that HasPermissionInContentLibraryScope.check() returns False + when authorization is denied. + + Given: + - Non-staff user + - Non-public library + - Authorization system denies permission (mocked) + + Expected: + - check() returns False + """ + user = UserFactory.create(username="no_perm_user", is_staff=False) + + with self.as_user(self.admin_user): + lib = self._create_library(slug="no-perm-lib", org=self.org_short_name, title="No Permission Library") + + library_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib['id'])) + + with patch('openedx.core.djangoapps.content_libraries.permissions.is_user_allowed', return_value=False): + result = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].check(user, library_obj) + + self.assertFalse(result, "Should return False when user is not authorized") + + self.assertFalse(library_obj.allow_public_read) + self.assertFalse(user.is_staff) + + def test_authz_scope_handles_empty_scopes(self): + """ + Test that HasPermissionInContentLibraryScope.query() returns empty + result when user has no authorized scopes. + + Given: + - Non-staff user + - Library exists in database + - Authorization system returns empty scope list (mocked) + + Expected: + - Filter returns 0 libraries + - Library exists in database but is not accessible + """ + user = UserFactory.create(username="empty_user", is_staff=False) + + with self.as_user(self.admin_user): + lib = self._create_library(slug="empty-lib", title="Empty Scopes Test") + + with patch('openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission', return_value=[]): + filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter( + user, + ContentLibrary.objects.filter(slug="empty-lib") + ) + + self.assertEqual(filtered.count(), 0, + "Should return 0 libraries when user has no authorized scopes") + + self.assertTrue(ContentLibrary.objects.filter(slug="empty-lib").exists(), + "Library should exist in database") + + def test_authz_scope_q_object_has_correct_structure(self): + """ + Test that HasPermissionInContentLibraryScope.query() generates Q object + with structure: Q(org__short_name='X') & Q(slug='Y') for each scope. + + Multiple scopes should be OR'd: + (Q(org__short_name='org1') & Q(slug='lib1')) | (Q(org__short_name='org2') & Q(slug='lib2')) + """ + user = UserFactory.create(username="q_user") + rule = HasPermissionInContentLibraryScope('view_library', filter_keys=['org', 'slug']) + + with patch("openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission") as mock_get_scopes: + # Create scopes with specific org/slug values we can verify + mock_scope1 = type("Scope", (), { + "library_key": type("Key", (), {"org": "specific-org1", "slug": "specific-slug1"})() + })() + mock_scope2 = type("Scope", (), { + "library_key": type("Key", (), {"org": "specific-org2", "slug": "specific-slug2"})() + })() + mock_get_scopes.return_value = [mock_scope1, mock_scope2] + + q_obj = rule.query(user) + + # Test 1: Verify it returns a Q object + self.assertIsInstance(q_obj, Q) + + # Test 2: Verify Q object uses OR connector (for multiple scopes) + self.assertEqual(q_obj.connector, 'OR', + "Should use OR to combine different library scopes") + + # Test 3: Verify the Q object string contains the exact fields and values + q_str = str(q_obj) + + # Should filter by org__short_name field + self.assertIn("org__short_name", q_str, + "Q object must filter by org__short_name field") + + # Should filter by slug field + self.assertIn("slug", q_str, + "Q object must filter by slug field") + + # Should contain exact org values + self.assertIn("specific-org1", q_str, + "Q object must include 'specific-org1'") + self.assertIn("specific-org2", q_str, + "Q object must include 'specific-org2'") + + # Should contain exact slug values + self.assertIn("specific-slug1", q_str, + "Q object must include 'specific-slug1'") + self.assertIn('specific-slug2', q_str, + "Q object must include 'specific-slug2'") + + def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): + """ + Test that the Q object filters by EXACT (org, slug) pairs, not just org OR slug. + + Critical test: Verifies the rule generates: + Q(org__short_name='org1' AND slug='lib1') OR Q(org__short_name='org2' AND slug='lib2') + + NOT just: + Q(org__short_name IN ['org1', 'org2']) OR Q(slug IN ['lib1', 'lib2']) + + Creates scenario: + - lib1: org1 + lib1 (authorized) + - lib2: org2 + lib2 (authorized) + - lib3: org1 + lib3 (NOT authorized - same org, different slug) + - lib4: org3 + lib1 (NOT authorized - same slug, different org) + """ + user = UserFactory.create(username="exact_pair_user") + rule = HasPermissionInContentLibraryScope('view_library', filter_keys=['org', 'slug']) + + Organization.objects.get_or_create(short_name="pair-org1", defaults={"name": "Pair Org 1"}) + Organization.objects.get_or_create(short_name="pair-org2", defaults={"name": "Pair Org 2"}) + Organization.objects.get_or_create(short_name="pair-org3", defaults={"name": "Pair Org 3"}) + + with self.as_user(self.admin_user): + lib1 = self._create_library(slug="pair-lib1", org="pair-org1", title="Pair Lib 1") + lib2 = self._create_library(slug="pair-lib2", org="pair-org2", title="Pair Lib 2") + lib3 = self._create_library(slug="pair-lib3", org="pair-org1", title="Pair Lib 3") # Same org as lib1 + lib4 = self._create_library(slug="pair-lib1", org="pair-org3", title="Pair Lib 4") # Same slug as lib1 + + with patch('openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission') as mock_get_scopes: + # Authorize ONLY (pair-org1, pair-lib1) and (pair-org2, pair-lib2) + lib1_key = LibraryLocatorV2.from_string(lib1['id']) + lib2_key = LibraryLocatorV2.from_string(lib2['id']) + + mock_get_scopes.return_value = [ + type('Scope', (), {'library_key': lib1_key})(), + type('Scope', (), {'library_key': lib2_key})(), + ] + + q_obj = rule.query(user) + filtered = ContentLibrary.objects.filter(q_obj) + + # TEST: Verify EXACTLY 2 libraries match (lib1 and lib2 only) + self.assertEqual(filtered.count(), 2, + "Must match EXACTLY 2 libraries - only those with authorized (org, slug) pairs") + + # TEST: Verify lib1 matches (pair-org1, pair-lib1) + lib1_result = filtered.filter(slug='pair-lib1', org__short_name='pair-org1') + self.assertEqual(lib1_result.count(), 1, + "Must match lib1: (pair-org1, pair-lib1) - this exact pair is authorized") + + # TEST: Verify lib2 matches (pair-org2, pair-lib2) + lib2_result = filtered.filter(slug='pair-lib2', org__short_name='pair-org2') + self.assertEqual(lib2_result.count(), 1, + "Must match lib2: (pair-org2, pair-lib2) - this exact pair is authorized") + + # TEST: Verify lib3 does NOT match (pair-org1, pair-lib3) + lib3_result = filtered.filter(slug='pair-lib3', org__short_name='pair-org1') + self.assertEqual(lib3_result.count(), 0, + "Must NOT match lib3: (pair-org1, pair-lib3) - only pair-lib1 is authorized for pair-org1") + + # TEST: Verify lib4 does NOT match (pair-org3, pair-lib1) + lib4_result = filtered.filter(slug='pair-lib1', org__short_name='pair-org3') + self.assertEqual(lib4_result.count(), 0, + "Must NOT match lib4: (pair-org3, pair-lib1) - only pair-org1 is authorized for pair-lib1") + + # TEST: Verify the result set contains exactly the right libraries + result_pairs = set(filtered.values_list('org__short_name', 'slug')) + expected_pairs = {('pair-org1', 'pair-lib1'), ('pair-org2', 'pair-lib2')} + self.assertEqual(result_pairs, expected_pairs, + f"Result must contain exactly {expected_pairs}, got {result_pairs}") + @ddt.ddt class ContentLibraryXBlockValidationTest(APITestCase): From db198a8d170dcc4c100243de2ea3b8b6eb640607 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 28 Oct 2025 17:32:32 +0100 Subject: [PATCH 02/14] docs: update docstring considering latest changes --- .../content_libraries/permissions.py | 77 ++++++++----------- 1 file changed, 33 insertions(+), 44 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index d13eb2126597..baceffa26234 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -59,90 +59,79 @@ def is_course_creator(user): class HasPermissionInContentLibraryScope(Rule): - """Bridgekeeper rule that checks permissions via Casbin's policy engine. + """Bridgekeeper rule that checks content library permissions via the openedx-authz system. - This rule integrates Casbin's role-based authorization with Bridgekeeper's - declarative permission system. It checks if a user has been granted a specific - permission (action) through their role assignments in Casbin. + 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 Casbin grouping policies to find the user's role assignments - 2. Querying Casbin permission policies to find which roles grant the action - 3. Matching role assignments with scopes to determine where the user has permission + 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 This enables both individual object permission checks and efficient QuerySet filtering - a key feature that allows database-level filtering instead of checking each object individually. Attributes: - action_external_key (str): The action/permission to check (e.g., 'view', 'edit'). + action_external_key (str): The action/permission to check (e.g., 'view_library', 'edit_library'). This should be the external key WITHOUT the namespace prefix. - For example, use 'view' not 'act^view'. + For example, use 'view_library' not 'act^view_library'. - scope_field (str): The Django model field/property that contains the scope identifier. - This tells the rule WHERE to find the scope value in your model. - Defaults to 'id'. + filter_keys (list[str]): The Django model fields to use when building QuerySet filters. + Defaults to ['org', 'slug'] for ContentLibrary models. - **IMPORTANT**: This can be a model property (like `library_key`) or a field. - For ContentLibrary, use 'library_key' which is a @property that returns - the LibraryLocatorV2 string representation. + These fields are used to construct the Q object filters that match libraries + based on the parsed components from library keys in authorization scopes. - The scope_field serves two purposes: - - **For QuerySet filtering**: Builds SQL like `WHERE scope_field IN (...)` - - **For object checks**: Extracts the scope from `instance.scope_field` + 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 - Supports Django ORM field lookups for nested fields: - - 'library_key' - a @property on the model (ContentLibrary case) - - 'id' - direct field on the model - - 'library__id' - field on a related model - - 'course__org__key' - multi-level relationship + 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 scope_field: + Basic usage with default filter_keys: >>> from bridgekeeper import perms - >>> from openedx_authz.permissions import HasPermissionInScope + >>> from openedx.core.djangoapps.content_libraries.permissions import HasPermissionInContentLibraryScope >>> - >>> # Assumes the model's 'id' field contains the scope - >>> can_view = HasPermissionInScope('view') - >>> perms['libraries.view'] = can_view - - Specifying a custom scope_field: - >>> # When scope is in a field named 'library_id' - >>> can_view = HasPermissionInScope('view', scope_field='library_id') - >>> - >>> # When scope is in a related model - >>> can_manage = HasPermissionInScope('manage', scope_field='library__key') + >>> # 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 = HasPermissionInScope('view', scope_field='library_id') + >>> can_view = HasPermissionInContentLibraryScope('view_library') >>> >>> # User must be active AND (staff OR have explicit permission) - >>> perms['libraries.view'] = is_active & (is_staff | can_view) + >>> 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'].filter( + >>> visible_libraries = perms['libraries.view_library'].filter( ... request.user, ... ContentLibrary.objects.all() ... ) Individual object checks: - >>> library = ContentLibrary.objects.get(library_id='lib:DemoX:CSPROB') - >>> if perms['libraries.view'].check(request.user, library): + >>> 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 ... return render_library(library) Note: - The scope identifiers in Casbin policies must match the values in your - Django model's scope_field. For example, if Casbin stores - 'lib:DemoX:CSPROB' and your model has library_id='lib:DemoX:CSPROB', - they must match exactly (including format and casing). + 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, action_external_key: str, filter_keys: list[str] = ["org", "slug"]): From d26cbc8e680b808857dc758a3181856e40a3111f Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 3 Nov 2025 14:05:00 +0100 Subject: [PATCH 03/14] refactor: address quality issues --- .../djangoapps/content_libraries/permissions.py | 8 +++++--- .../tests/test_content_libraries.py | 17 +++++++++++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index baceffa26234..892a6fb4a93f 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -134,7 +134,7 @@ class HasPermissionInContentLibraryScope(Rule): org.short_name='DemoX' and slug='CSPROB'. """ - def __init__(self, action_external_key: str, filter_keys: list[str] = ["org", "slug"]): + def __init__(self, action_external_key: str, filter_keys: list[str] | None = None): """Initialize the rule with the action and filter keys to filter on. Args: @@ -143,7 +143,7 @@ def __init__(self, action_external_key: str, filter_keys: list[str] = ["org", "s Defaults to ['org', 'slug'] for ContentLibrary. """ self.action_external_key = action_external_key - self.filter_keys = filter_keys + 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. @@ -194,7 +194,7 @@ def query(self, user): return query - def check(self, user, instance): + 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 @@ -204,6 +204,8 @@ def check(self, user, instance): 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, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 8c446d294d29..623df593b2cd 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -1243,7 +1243,9 @@ def test_authz_scope_filters_by_authorized_libraries(self): lib2 = self._create_library(slug="lib2", org="org2", title="Library 2") lib3 = self._create_library(slug="lib3", org="org1", title="Library 3") - with patch('openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission') as mock_get_scopes: + with patch( + 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission' + ) as mock_get_scopes: # Mock: User authorized for lib1 (org1:lib1) and lib2 (org2:lib2) only, NOT lib3 mock_scope1 = type('Scope', (), {'library_key': LibraryLocatorV2.from_string(lib1['id'])})() mock_scope2 = type('Scope', (), {'library_key': LibraryLocatorV2.from_string(lib2['id'])})() @@ -1339,7 +1341,10 @@ def test_authz_scope_handles_empty_scopes(self): with self.as_user(self.admin_user): lib = self._create_library(slug="empty-lib", title="Empty Scopes Test") - with patch('openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission', return_value=[]): + with patch( + 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission', + return_value=[] + ): filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter( user, ContentLibrary.objects.filter(slug="empty-lib") @@ -1362,7 +1367,9 @@ def test_authz_scope_q_object_has_correct_structure(self): user = UserFactory.create(username="q_user") rule = HasPermissionInContentLibraryScope('view_library', filter_keys=['org', 'slug']) - with patch("openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission") as mock_get_scopes: + with patch( + "openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission" + ) as mock_get_scopes: # Create scopes with specific org/slug values we can verify mock_scope1 = type("Scope", (), { "library_key": type("Key", (), {"org": "specific-org1", "slug": "specific-slug1"})() @@ -1433,7 +1440,9 @@ def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): lib3 = self._create_library(slug="pair-lib3", org="pair-org1", title="Pair Lib 3") # Same org as lib1 lib4 = self._create_library(slug="pair-lib1", org="pair-org3", title="Pair Lib 4") # Same slug as lib1 - with patch('openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission') as mock_get_scopes: + with patch( + 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission' + ) as mock_get_scopes: # Authorize ONLY (pair-org1, pair-lib1) and (pair-org2, pair-lib2) lib1_key = LibraryLocatorV2.from_string(lib1['id']) lib2_key = LibraryLocatorV2.from_string(lib2['id']) From b2a5c45dfda5914e36a1f98d6d7d0037ddc17939 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 3 Nov 2025 14:39:29 +0100 Subject: [PATCH 04/14] refactor: use constants instead of action plain string --- .../djangoapps/content_libraries/permissions.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index 892a6fb4a93f..e23cc5992879 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -6,7 +6,9 @@ from django.conf import settings from django.db.models import Q +from openedx_authz.api.data import PermissionData from openedx_authz.api.users import is_user_allowed, get_scopes_for_user_and_permission +from openedx_authz.constants.permissions import VIEW_LIBRARY from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission @@ -134,15 +136,15 @@ class HasPermissionInContentLibraryScope(Rule): org.short_name='DemoX' and slug='CSPROB'. """ - def __init__(self, action_external_key: str, filter_keys: list[str] | None = None): + def __init__(self, permission: PermissionData, filter_keys: list[str] | None = None): """Initialize the rule with the action and filter keys to filter on. Args: - action_external_key (str): The action/permission to check (e.g., 'view', 'edit'). + 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.action_external_key = action_external_key + self.permission = permission self.filter_keys = filter_keys if filter_keys is not None else ["org", "slug"] def query(self, user): @@ -179,7 +181,7 @@ def query(self, user): """ scopes = get_scopes_for_user_and_permission( user.username, - self.action_external_key + self.permission.identifier ) library_keys = [scope.library_key for scope in scopes] @@ -216,7 +218,7 @@ def check(self, user, instance, *args, **kwargs): # pylint: disable=arguments-d >>> can_view = rule.check(user, library) >>> # Checks if user has 'view' permission in scope 'lib:DemoX:CSPROB' """ - return is_user_allowed(user.username, self.action_external_key, str(instance.library_key)) + return is_user_allowed(user.username, self.permission.identifier, str(instance.library_key)) ########################### Permissions ########################### @@ -253,7 +255,7 @@ def check(self, user, instance, *args, **kwargs): # pylint: disable=arguments-d # Libraries with "public read" permissions can be accessed only by course creators (Attribute('allow_public_read', True) & is_course_creator) | # Users can access libraries within their authorized scope (via Casbin/role-based permissions) - HasPermissionInContentLibraryScope("view_library") | + HasPermissionInContentLibraryScope(VIEW_LIBRARY) | # Fallback to: the user must be part of the library's team (legacy permission system) has_explicit_read_permission_for_library ) From f06062e1dd63721be06ce600cff290c9b5d9a518 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 3 Nov 2025 14:46:44 +0100 Subject: [PATCH 05/14] docs: update current docstrings --- .../content_libraries/permissions.py | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index e23cc5992879..8b6c41b8fb88 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -72,14 +72,9 @@ class HasPermissionInContentLibraryScope(Rule): 2. Parsing the library keys (org/slug) from the scopes 3. Building database filters to match ContentLibrary models with those org/slug combinations - This enables both individual object permission checks and efficient QuerySet - filtering - a key feature that allows database-level filtering instead of - checking each object individually. - Attributes: - action_external_key (str): The action/permission to check (e.g., 'view_library', 'edit_library'). - This should be the external key WITHOUT the namespace prefix. - For example, use 'view_library' not 'act^view_library'. + 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. @@ -127,7 +122,6 @@ class HasPermissionInContentLibraryScope(Rule): >>> 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 - ... return render_library(library) Note: The library keys in authorization scopes must have the format 'lib:ORG:SLUG' @@ -150,14 +144,6 @@ def __init__(self, permission: PermissionData, filter_keys: list[str] | None = N def query(self, user): """Convert this rule to a Django Q object for QuerySet filtering. - This method enables efficient database-level filtering by: - 1. Querying the authorization system to get ALL library scopes where the user has this permission - 2. Parsing the library keys (org/slug pairs) from the scopes - 3. Building a Django Q object that filters for libraries matching those org/slug combinations - - This avoids N+1 query problems by filtering at the database level rather - than checking permission for each object individually. - Args: user: The Django user object (must have a 'username' attribute). From 781625454cd28dfe84fa16790072d9d668675a28 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 3 Nov 2025 15:35:21 +0100 Subject: [PATCH 06/14] refactor: use view library object instead of string in tests --- .../content_libraries/tests/test_content_libraries.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 623df593b2cd..42951c8584da 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -37,6 +37,7 @@ ) from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx_authz.constants.permissions import VIEW_LIBRARY from ..models import ContentLibrary from ..permissions import CAN_VIEW_THIS_CONTENT_LIBRARY, HasPermissionInContentLibraryScope @@ -1365,7 +1366,7 @@ def test_authz_scope_q_object_has_correct_structure(self): (Q(org__short_name='org1') & Q(slug='lib1')) | (Q(org__short_name='org2') & Q(slug='lib2')) """ user = UserFactory.create(username="q_user") - rule = HasPermissionInContentLibraryScope('view_library', filter_keys=['org', 'slug']) + rule = HasPermissionInContentLibraryScope(VIEW_LIBRARY, filter_keys=['org', 'slug']) with patch( "openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission" @@ -1428,7 +1429,7 @@ def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): - lib4: org3 + lib1 (NOT authorized - same slug, different org) """ user = UserFactory.create(username="exact_pair_user") - rule = HasPermissionInContentLibraryScope('view_library', filter_keys=['org', 'slug']) + rule = HasPermissionInContentLibraryScope(VIEW_LIBRARY, filter_keys=['org', 'slug']) Organization.objects.get_or_create(short_name="pair-org1", defaults={"name": "Pair Org 1"}) Organization.objects.get_or_create(short_name="pair-org2", defaults={"name": "Pair Org 2"}) From af8a2e4ebfcec7f0e5aeda946852afb4bd78d367 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 3 Nov 2025 20:02:52 +0100 Subject: [PATCH 07/14] refactor: address quality issues --- .../tests/test_content_libraries.py | 104 +++++++++++++----- 1 file changed, 74 insertions(+), 30 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 42951c8584da..94a78759e54e 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -1351,11 +1351,16 @@ def test_authz_scope_handles_empty_scopes(self): ContentLibrary.objects.filter(slug="empty-lib") ) - self.assertEqual(filtered.count(), 0, - "Should return 0 libraries when user has no authorized scopes") + self.assertEqual( + filtered.count(), + 0, + "Should return 0 libraries when user has no authorized scopes", + ) - self.assertTrue(ContentLibrary.objects.filter(slug="empty-lib").exists(), - "Library should exist in database") + self.assertTrue( + ContentLibrary.objects.filter(slug="empty-lib").exists(), + "Library should exist in database", + ) def test_authz_scope_q_object_has_correct_structure(self): """ @@ -1386,31 +1391,52 @@ def test_authz_scope_q_object_has_correct_structure(self): self.assertIsInstance(q_obj, Q) # Test 2: Verify Q object uses OR connector (for multiple scopes) - self.assertEqual(q_obj.connector, 'OR', - "Should use OR to combine different library scopes") + self.assertEqual( + q_obj.connector, + 'OR', + "Should use OR to combine different library scopes", + ) # Test 3: Verify the Q object string contains the exact fields and values q_str = str(q_obj) # Should filter by org__short_name field - self.assertIn("org__short_name", q_str, - "Q object must filter by org__short_name field") + self.assertIn( + "org__short_name", + q_str, + "Q object must filter by org__short_name field", + ) # Should filter by slug field - self.assertIn("slug", q_str, - "Q object must filter by slug field") + self.assertIn( + "slug", + q_str, + "Q object must filter by slug field", + ) # Should contain exact org values - self.assertIn("specific-org1", q_str, - "Q object must include 'specific-org1'") - self.assertIn("specific-org2", q_str, - "Q object must include 'specific-org2'") + self.assertIn( + "specific-org1", + q_str, + "Q object must include 'specific-org1'", + ) + self.assertIn( + "specific-org2", + q_str, + "Q object must include 'specific-org2'", + ) # Should contain exact slug values - self.assertIn("specific-slug1", q_str, - "Q object must include 'specific-slug1'") - self.assertIn('specific-slug2', q_str, - "Q object must include 'specific-slug2'") + self.assertIn( + "specific-slug1", + q_str, + "Q object must include 'specific-slug1'", + ) + self.assertIn( + 'specific-slug2', + q_str, + "Q object must include 'specific-slug2'", + ) def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): """ @@ -1457,34 +1483,52 @@ def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): filtered = ContentLibrary.objects.filter(q_obj) # TEST: Verify EXACTLY 2 libraries match (lib1 and lib2 only) - self.assertEqual(filtered.count(), 2, - "Must match EXACTLY 2 libraries - only those with authorized (org, slug) pairs") + self.assertEqual( + filtered.count(), + 2, + "Must match EXACTLY 2 libraries - only those with authorized (org, slug) pairs", + ) # TEST: Verify lib1 matches (pair-org1, pair-lib1) lib1_result = filtered.filter(slug='pair-lib1', org__short_name='pair-org1') - self.assertEqual(lib1_result.count(), 1, - "Must match lib1: (pair-org1, pair-lib1) - this exact pair is authorized") + self.assertEqual( + lib1_result.count(), + 1, + "Must match lib1: (pair-org1, pair-lib1) - this exact pair is authorized", + ) # TEST: Verify lib2 matches (pair-org2, pair-lib2) lib2_result = filtered.filter(slug='pair-lib2', org__short_name='pair-org2') - self.assertEqual(lib2_result.count(), 1, - "Must match lib2: (pair-org2, pair-lib2) - this exact pair is authorized") + self.assertEqual( + lib2_result.count(), + 1, + "Must match lib2: (pair-org2, pair-lib2) - this exact pair is authorized", + ) # TEST: Verify lib3 does NOT match (pair-org1, pair-lib3) lib3_result = filtered.filter(slug='pair-lib3', org__short_name='pair-org1') - self.assertEqual(lib3_result.count(), 0, - "Must NOT match lib3: (pair-org1, pair-lib3) - only pair-lib1 is authorized for pair-org1") + self.assertEqual( + lib3_result.count(), + 0, + "Must NOT match lib3: (pair-org1, pair-lib3) - only pair-lib1 is authorized for pair-org1", + ) # TEST: Verify lib4 does NOT match (pair-org3, pair-lib1) lib4_result = filtered.filter(slug='pair-lib1', org__short_name='pair-org3') - self.assertEqual(lib4_result.count(), 0, - "Must NOT match lib4: (pair-org3, pair-lib1) - only pair-org1 is authorized for pair-lib1") + self.assertEqual( + lib4_result.count(), + 0, + "Must NOT match lib4: (pair-org3, pair-lib1) - only pair-org1 is authorized for pair-lib1", + ) # TEST: Verify the result set contains exactly the right libraries result_pairs = set(filtered.values_list('org__short_name', 'slug')) expected_pairs = {('pair-org1', 'pair-lib1'), ('pair-org2', 'pair-lib2')} - self.assertEqual(result_pairs, expected_pairs, - f"Result must contain exactly {expected_pairs}, got {result_pairs}") + self.assertEqual( + result_pairs, + expected_pairs, + f"Result must contain exactly {expected_pairs}, got {result_pairs}", + ) @ddt.ddt From c614b34144698ca574bff6aeacb9e4f26ace966b Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 4 Nov 2025 12:02:13 +0100 Subject: [PATCH 08/14] refactor: test ONLY authz filtering by removing legacy permissions --- .../tests/test_content_libraries.py | 47 ++++++++++++++++++- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 94a78759e54e..b99b43023809 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -39,7 +39,7 @@ from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx_authz.constants.permissions import VIEW_LIBRARY -from ..models import ContentLibrary +from ..models import ContentLibrary, ContentLibraryPermission from ..permissions import CAN_VIEW_THIS_CONTENT_LIBRARY, HasPermissionInContentLibraryScope @@ -1220,6 +1220,28 @@ def test_uncaught_error_creates_error_log(self): self.assertEqual(task_data, expected) + +@skip_unless_cms +class ContentLibrariesAuthZTestCase(ContentLibrariesRestApiTest): + """ + Tests for Content Libraries AuthZ integration via openedx-authz. + + These tests verify the HasPermissionInContentLibraryScope Bridgekeeper rule + integrates correctly with the openedx-authz authorization system (Casbin). + See: https://github.com/openedx/openedx-authz/ + + IMPORTANT: These tests explicitly remove legacy ContentLibraryPermission grants + to ensure ONLY the AuthZ system is being tested, not the legacy fallback. + """ + + def setUp(self): + super().setUp() + # The parent class provides self.user (a staff user) and self.organization + # Set up admin_user as an alias to self.user for test readability + self.admin_user = self.user + # Set up org_short_name for convenience + self.org_short_name = self.organization.short_name + def test_authz_scope_filters_by_authorized_libraries(self): """ Test that HasPermissionInContentLibraryScope rule filters libraries @@ -1227,7 +1249,7 @@ def test_authz_scope_filters_by_authorized_libraries(self): Given: - 3 libraries: lib1 (org1), lib2 (org2), lib3 (org1) - - User authorized for lib1 and lib2 only + - User authorized for lib1 and lib2 only via AuthZ (NO legacy permissions) Expected: - Filter returns exactly 2 libraries (lib1 and lib2) @@ -1244,6 +1266,9 @@ def test_authz_scope_filters_by_authorized_libraries(self): lib2 = self._create_library(slug="lib2", org="org2", title="Library 2") lib3 = self._create_library(slug="lib3", org="org1", title="Library 3") + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ filtering) + ContentLibraryPermission.objects.filter(user=user).delete() + with patch( 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission' ) as mock_get_scopes: @@ -1279,6 +1304,7 @@ def test_authz_scope_individual_check_with_permission(self): - Non-staff user - Library exists - Authorization system grants permission (mocked) + - NO legacy permissions Expected: - check() returns True @@ -1290,6 +1316,9 @@ def test_authz_scope_individual_check_with_permission(self): library_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib["id"])) + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) + ContentLibraryPermission.objects.filter(user=user).delete() + with patch("openedx.core.djangoapps.content_libraries.permissions.is_user_allowed", return_value=True): result = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].check(user, library_obj) @@ -1304,6 +1333,7 @@ def test_authz_scope_individual_check_without_permission(self): - Non-staff user - Non-public library - Authorization system denies permission (mocked) + - NO legacy permissions Expected: - check() returns False @@ -1315,6 +1345,9 @@ def test_authz_scope_individual_check_without_permission(self): library_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib['id'])) + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) + ContentLibraryPermission.objects.filter(user=user).delete() + with patch('openedx.core.djangoapps.content_libraries.permissions.is_user_allowed', return_value=False): result = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].check(user, library_obj) @@ -1332,6 +1365,7 @@ def test_authz_scope_handles_empty_scopes(self): - Non-staff user - Library exists in database - Authorization system returns empty scope list (mocked) + - NO legacy permissions Expected: - Filter returns 0 libraries @@ -1342,6 +1376,9 @@ def test_authz_scope_handles_empty_scopes(self): with self.as_user(self.admin_user): lib = self._create_library(slug="empty-lib", title="Empty Scopes Test") + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) + ContentLibraryPermission.objects.filter(user=user).delete() + with patch( 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission', return_value=[] @@ -1369,6 +1406,9 @@ def test_authz_scope_q_object_has_correct_structure(self): Multiple scopes should be OR'd: (Q(org__short_name='org1') & Q(slug='lib1')) | (Q(org__short_name='org2') & Q(slug='lib2')) + + Note: This test focuses on Q object structure, not filtering behavior, + so legacy permissions don't affect the outcome. """ user = UserFactory.create(username="q_user") rule = HasPermissionInContentLibraryScope(VIEW_LIBRARY, filter_keys=['org', 'slug']) @@ -1467,6 +1507,9 @@ def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): lib3 = self._create_library(slug="pair-lib3", org="pair-org1", title="Pair Lib 3") # Same org as lib1 lib4 = self._create_library(slug="pair-lib1", org="pair-org3", title="Pair Lib 4") # Same slug as lib1 + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ filtering) + ContentLibraryPermission.objects.filter(user=user).delete() + with patch( 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission' ) as mock_get_scopes: From d1d98e36dc8cbe9eeb0f11c88719ab9f47cf66a8 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 11 Nov 2025 14:29:39 +0100 Subject: [PATCH 09/14] refactor: get distinct objects when querying content libraries --- openedx/core/djangoapps/content_libraries/api/libraries.py | 2 +- .../content_libraries/tests/test_content_libraries.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 8d32e4dbc015..e85b9047f352 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -267,7 +267,7 @@ def get_libraries_for_user(user, org=None, text_search=None, order=None) -> Quer Q(learning_package__description__icontains=text_search) ) - filtered = permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs) + filtered = permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs).distinct() if order: order_query = 'learning_package__' diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index b99b43023809..db423a3ad865 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -1278,7 +1278,7 @@ def test_authz_scope_filters_by_authorized_libraries(self): mock_get_scopes.return_value = [mock_scope1, mock_scope2] all_libs = ContentLibrary.objects.filter(slug__in=['lib1', 'lib2', 'lib3']) - filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, all_libs) + filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, all_libs).distinct() # TEST: Verify exactly 2 libraries returned (lib1 and lib2, not lib3) self.assertEqual(filtered.count(), 2, "Should return exactly 2 authorized libraries") @@ -1386,7 +1386,7 @@ def test_authz_scope_handles_empty_scopes(self): filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter( user, ContentLibrary.objects.filter(slug="empty-lib") - ) + ).distinct() self.assertEqual( filtered.count(), From 621c07c2673895b6a8caa1762cebc92011058ac9 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 12 Nov 2025 17:03:21 +0100 Subject: [PATCH 10/14] docs: add inline comment explaining why to use distinct --- openedx/core/djangoapps/content_libraries/api/libraries.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index e85b9047f352..ee0618898a9a 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -267,6 +267,10 @@ def get_libraries_for_user(user, org=None, text_search=None, order=None) -> Quer Q(learning_package__description__icontains=text_search) ) + # Using distinct() temporarily to avoid duplicate results caused by overlapping permission checks + # between Bridgekeeper and the new authorization framework. This ensures correct results for now, + # but it should be removed once Bridgekeeper support is fully dropped and all permission logic + # is handled through openedx-authz. filtered = permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs).distinct() if order: From 0b6c3eefada8879a3b949f20acec65e2f7a7ffdb Mon Sep 17 00:00:00 2001 From: "Maria Grimaldi (Majo)" Date: Wed, 12 Nov 2025 17:06:53 +0100 Subject: [PATCH 11/14] refactor: apply suggestions from code review Co-authored-by: Bryann Valderrama --- openedx/core/djangoapps/content_libraries/permissions.py | 2 +- .../content_libraries/tests/test_content_libraries.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index 8b6c41b8fb88..cb304e52b2e1 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -200,7 +200,7 @@ def check(self, user, instance, *args, **kwargs): # pylint: disable=arguments-d False otherwise. Example: - >>> rule = HasPermissionInScope('view') + >>> rule = HasPermissionInContentLibraryScope('view') >>> can_view = rule.check(user, library) >>> # Checks if user has 'view' permission in scope 'lib:DemoX:CSPROB' """ diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index db423a3ad865..b08ae9af86cc 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -1264,7 +1264,7 @@ def test_authz_scope_filters_by_authorized_libraries(self): with self.as_user(self.admin_user): lib1 = self._create_library(slug="lib1", org="org1", title="Library 1") lib2 = self._create_library(slug="lib2", org="org2", title="Library 2") - lib3 = self._create_library(slug="lib3", org="org1", title="Library 3") + self._create_library(slug="lib3", org="org1", title="Library 3") # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ filtering) ContentLibraryPermission.objects.filter(user=user).delete() @@ -1374,7 +1374,7 @@ def test_authz_scope_handles_empty_scopes(self): user = UserFactory.create(username="empty_user", is_staff=False) with self.as_user(self.admin_user): - lib = self._create_library(slug="empty-lib", title="Empty Scopes Test") + self._create_library(slug="empty-lib", title="Empty Scopes Test") # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) ContentLibraryPermission.objects.filter(user=user).delete() @@ -1504,8 +1504,8 @@ def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): with self.as_user(self.admin_user): lib1 = self._create_library(slug="pair-lib1", org="pair-org1", title="Pair Lib 1") lib2 = self._create_library(slug="pair-lib2", org="pair-org2", title="Pair Lib 2") - lib3 = self._create_library(slug="pair-lib3", org="pair-org1", title="Pair Lib 3") # Same org as lib1 - lib4 = self._create_library(slug="pair-lib1", org="pair-org3", title="Pair Lib 4") # Same slug as lib1 + self._create_library(slug="pair-lib3", org="pair-org1", title="Pair Lib 3") # Same org as lib1 + self._create_library(slug="pair-lib1", org="pair-org3", title="Pair Lib 4") # Same slug as lib1 # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ filtering) ContentLibraryPermission.objects.filter(user=user).delete() From d3d35ab99e6283d5f141ddb2e141320015396c87 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 13 Nov 2025 12:49:42 +0100 Subject: [PATCH 12/14] test: add test case for combined frameworks --- .../tests/test_content_libraries.py | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index b08ae9af86cc..c97f7d15155f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -1573,6 +1573,109 @@ def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): f"Result must contain exactly {expected_pairs}, got {result_pairs}", ) + def test_authz_scope_with_combined_authz_and_legacy_permissions(self): + """ + Test that the filter returns libraries when user has BOTH AuthZ AND legacy permissions. + + The CAN_VIEW_THIS_CONTENT_LIBRARY permission uses OR logic: + is_user_active & ( + is_global_staff | + (allow_public_read & is_course_creator) | + HasPermissionInContentLibraryScope(VIEW_LIBRARY) | # AuthZ + has_explicit_read_permission_for_library # Legacy + ) + + This means a user with BOTH types of permissions should get access through EITHER system. + + Test scenario: + - lib1: User has AuthZ permission only + - lib2: User has legacy permission only + - lib3: User has BOTH AuthZ AND legacy permissions + - lib4: User has NO permissions + + Expected behavior: + - Filter returns lib1, lib2, and lib3 (NOT lib4) + - Having both permission types doesn't break filtering + - Each permission system contributes its authorized libraries + """ + user = UserFactory.create(username="combined_perm_user", is_staff=False) + + Organization.objects.get_or_create(short_name="comb-org", defaults={"name": "Combined Org"}) + + with self.as_user(self.admin_user): + lib1 = self._create_library(slug="comb-lib1", org="comb-org", title="AuthZ Only Library") + lib2 = self._create_library(slug="comb-lib2", org="comb-org", title="Legacy Only Library") + lib3 = self._create_library(slug="comb-lib3", org="comb-org", title="Both AuthZ and Legacy Library") + lib4 = self._create_library(slug="comb-lib4", org="comb-org", title="No Permissions Library") + + # Retrieve library objects for permission assignment + lib1_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib1['id'])) + lib2_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib2['id'])) + lib3_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib3['id'])) + + # Set up legacy permissions: lib2 (legacy only), lib3 (both) + ContentLibraryPermission.objects.create( + library=lib2_obj, + user=user, + access_level=ContentLibraryPermission.READ_LEVEL, + ) + ContentLibraryPermission.objects.create( + library=lib3_obj, + user=user, + access_level=ContentLibraryPermission.READ_LEVEL, + ) + + with patch( + 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission' + ) as mock_get_scopes: + # Set up AuthZ permissions: lib1 (AuthZ only), lib3 (both) + lib1_key = LibraryLocatorV2.from_string(lib1['id']) + lib3_key = LibraryLocatorV2.from_string(lib3['id']) + + mock_get_scopes.return_value = [ + type('Scope', (), {'library_key': lib1_key})(), + type('Scope', (), {'library_key': lib3_key})(), + ] + + all_libs = ContentLibrary.objects.filter(slug__in=['comb-lib1', 'comb-lib2', 'comb-lib3', 'comb-lib4']) + filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, all_libs).distinct() + + # TEST: Verify exactly 3 libraries returned (lib1, lib2, lib3 - NOT lib4) + self.assertEqual( + filtered.count(), + 3, + "Should return exactly 3 libraries: AuthZ-only, legacy-only, and both", + ) + + # TEST: Verify correct libraries are included + slugs = set(filtered.values_list('slug', flat=True)) + self.assertIn('comb-lib1', slugs, "lib1 should be accessible via AuthZ permission") + self.assertIn('comb-lib2', slugs, "lib2 should be accessible via legacy permission") + self.assertIn('comb-lib3', slugs, "lib3 should be accessible via BOTH AuthZ and legacy permissions") + self.assertNotIn('comb-lib4', slugs, "lib4 should NOT be accessible (no permissions)") + + # TEST: Verify lib3 doesn't get duplicated despite having both permission types + lib3_results = filtered.filter(slug='comb-lib3') + self.assertEqual( + lib3_results.count(), + 1, + "lib3 should appear exactly once despite having both AuthZ and legacy permissions", + ) + + # TEST: Verify the permission sources work independently + # This demonstrates the OR logic: user gets access if EITHER permission type grants it + result_pairs = set(filtered.values_list('org__short_name', 'slug')) + expected_pairs = { + ('comb-org', 'comb-lib1'), # AuthZ only + ('comb-org', 'comb-lib2'), # Legacy only + ('comb-org', 'comb-lib3'), # Both + } + self.assertEqual( + result_pairs, + expected_pairs, + f"Should get exactly the 3 authorized libraries via OR logic, got {result_pairs}", + ) + @ddt.ddt class ContentLibraryXBlockValidationTest(APITestCase): From e2d6a97c3adccd56260234e07cc72db2cc5de19e Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 13 Nov 2025 15:41:19 +0100 Subject: [PATCH 13/14] refactor: import from api instead --- openedx/core/djangoapps/content_libraries/permissions.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index cb304e52b2e1..c3a8b68c947c 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -6,8 +6,7 @@ from django.conf import settings from django.db.models import Q -from openedx_authz.api.data import PermissionData -from openedx_authz.api.users import is_user_allowed, get_scopes_for_user_and_permission +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 @@ -130,7 +129,7 @@ class HasPermissionInContentLibraryScope(Rule): org.short_name='DemoX' and slug='CSPROB'. """ - def __init__(self, permission: PermissionData, filter_keys: list[str] | None = None): + 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: @@ -165,7 +164,7 @@ def query(self, user): >>> # WHERE (org.short_name='OrgA' AND slug='lib-a') >>> # OR (org.short_name='OrgB' AND slug='lib-b') """ - scopes = get_scopes_for_user_and_permission( + scopes = authz_api.get_scopes_for_user_and_permission( user.username, self.permission.identifier ) @@ -204,7 +203,7 @@ def check(self, user, instance, *args, **kwargs): # pylint: disable=arguments-d >>> can_view = rule.check(user, library) >>> # Checks if user has 'view' permission in scope 'lib:DemoX:CSPROB' """ - return is_user_allowed(user.username, self.permission.identifier, str(instance.library_key)) + return authz_api.is_user_allowed(user.username, self.permission.identifier, str(instance.library_key)) ########################### Permissions ########################### From 0ac96bc141c685b8850ab8bd71ef10a37772c233 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 13 Nov 2025 16:04:13 +0100 Subject: [PATCH 14/14] refactor: import from api instead in tests --- .../tests/test_content_libraries.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index c97f7d15155f..61bc8012640f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -1270,7 +1270,7 @@ def test_authz_scope_filters_by_authorized_libraries(self): ContentLibraryPermission.objects.filter(user=user).delete() with patch( - 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission' + 'openedx_authz.api.get_scopes_for_user_and_permission' ) as mock_get_scopes: # Mock: User authorized for lib1 (org1:lib1) and lib2 (org2:lib2) only, NOT lib3 mock_scope1 = type('Scope', (), {'library_key': LibraryLocatorV2.from_string(lib1['id'])})() @@ -1319,7 +1319,7 @@ def test_authz_scope_individual_check_with_permission(self): # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) ContentLibraryPermission.objects.filter(user=user).delete() - with patch("openedx.core.djangoapps.content_libraries.permissions.is_user_allowed", return_value=True): + with patch("openedx_authz.api.is_user_allowed", return_value=True): result = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].check(user, library_obj) self.assertTrue(result, "Should return True when user is authorized") @@ -1348,7 +1348,7 @@ def test_authz_scope_individual_check_without_permission(self): # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) ContentLibraryPermission.objects.filter(user=user).delete() - with patch('openedx.core.djangoapps.content_libraries.permissions.is_user_allowed', return_value=False): + with patch('openedx_authz.api.is_user_allowed', return_value=False): result = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].check(user, library_obj) self.assertFalse(result, "Should return False when user is not authorized") @@ -1380,7 +1380,7 @@ def test_authz_scope_handles_empty_scopes(self): ContentLibraryPermission.objects.filter(user=user).delete() with patch( - 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission', + 'openedx_authz.api.get_scopes_for_user_and_permission', return_value=[] ): filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter( @@ -1414,7 +1414,7 @@ def test_authz_scope_q_object_has_correct_structure(self): rule = HasPermissionInContentLibraryScope(VIEW_LIBRARY, filter_keys=['org', 'slug']) with patch( - "openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission" + "openedx_authz.api.get_scopes_for_user_and_permission" ) as mock_get_scopes: # Create scopes with specific org/slug values we can verify mock_scope1 = type("Scope", (), { @@ -1511,7 +1511,7 @@ def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): ContentLibraryPermission.objects.filter(user=user).delete() with patch( - 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission' + 'openedx_authz.api.get_scopes_for_user_and_permission' ) as mock_get_scopes: # Authorize ONLY (pair-org1, pair-lib1) and (pair-org2, pair-lib2) lib1_key = LibraryLocatorV2.from_string(lib1['id']) @@ -1626,7 +1626,7 @@ def test_authz_scope_with_combined_authz_and_legacy_permissions(self): ) with patch( - 'openedx.core.djangoapps.content_libraries.permissions.get_scopes_for_user_and_permission' + 'openedx_authz.api.get_scopes_for_user_and_permission' ) as mock_get_scopes: # Set up AuthZ permissions: lib1 (AuthZ only), lib3 (both) lib1_key = LibraryLocatorV2.from_string(lib1['id'])