diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 88bfebdf..02d2add7 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,14 @@ Change Log Unreleased ********** +1.25.0 - 2026-09-24 +******************* + +Changed +======= + +* Filtering assignments by scope now respects the hierarchy: querying any course or library scope also returns assignments from its ancestor org-level and platform-level scopes. + 1.24.0 - 2026-09-14 ******************* diff --git a/src/openedx_authz/api/data.py b/src/openedx_authz/api/data.py index 20e3e153..48990d45 100644 --- a/src/openedx_authz/api/data.py +++ b/src/openedx_authz/api/data.py @@ -491,6 +491,75 @@ def IS_GLOB(self) -> bool: """Whether this scope represents a glob pattern (org- or platform-level).""" return self.IS_ORG_GLOB or self.IS_PLATFORM_GLOB + @property + def ancestors(self) -> set[str]: + """External keys of the scopes that hierarchically contain this one. + + The scope hierarchy is:: + + specific resource -> organization glob -> platform glob + + A concrete scope is contained by the org-level glob of its organization and by + the platform-level glob of its namespace. Subclasses that already sit higher in + the hierarchy override this property to report their own ancestors. + + Returns: + set[str]: The external keys of the scopes containing this one. Empty when the + scope has no ancestors. + + Examples: + >>> ScopeData(external_key='course-v1:DemoX+CS101+2024').ancestors + {'course-v1:DemoX+*', 'course-v1:*'} + >>> ScopeData(external_key='lib:DemoX:CSPROB').ancestors + {'lib:DemoX:*', 'lib:*'} + """ + namespace = type(self).NAMESPACE + ancestor_keys: set[str] = set() + + # The base ScopeData has no 'org'; only concrete resource scopes derive one. + org = getattr(self, "org", None) + if org: # pragma: no branch + org_glob_cls = type(self).org_glob_registry.get(namespace) + if org_glob_cls: + ancestor_keys.add(org_glob_cls.build_external_key(org)) + + platform_glob_cls = type(self).platform_glob_registry.get(namespace) + if platform_glob_cls: # pragma: no branch + ancestor_keys.add(platform_glob_cls.build_external_key()) + + return ancestor_keys + + @classmethod + def expand_keys_with_ancestors(cls, external_keys: list[str]) -> set[str]: + """Expand scope external keys to include their hierarchical ancestors. + + For each key, the org-level and platform-level ancestor scopes that also apply + according to the scope hierarchy are added to the result. Keys that cannot be + resolved to a registered scope type are kept as-is and not expanded, preserving + exact-match behaviour for unknown keys. + + Args: + external_keys (list[str]): The scope external keys to expand. + + Returns: + set[str]: The original keys plus all applicable ancestors. + + Examples: + >>> ScopeData.expand_keys_with_ancestors(['course-v1:DemoX+CS101+2024']) + {'course-v1:DemoX+CS101+2024', 'course-v1:DemoX+*', 'course-v1:*'} + """ + expanded: set[str] = set(external_keys) + + for external_key in external_keys: + try: + scope = ScopeData(external_key=external_key) + except ValueError: + # Unrecognised scope format - keep the original, skip expansion. + continue + expanded |= scope.ancestors + + return expanded + @classmethod def validate_external_key(cls, _: str) -> bool: """Validate the external_key format for ScopeData. @@ -864,6 +933,20 @@ def org(self) -> str | None: """ return self.get_org(self.external_key) + @property + def ancestors(self) -> set[str]: + """External keys of the scopes that hierarchically contain this one. + + An organization-level glob is contained only by the platform-level glob of its + namespace (e.g., ``lib:DemoX:*`` is contained by ``lib:*``). + + Returns: + set[str]: The platform-level glob external key, or an empty set when the + namespace has no registered platform-level glob. + """ + platform_glob_cls = type(self).platform_glob_registry.get(type(self).NAMESPACE) + return {platform_glob_cls.build_external_key()} if platform_glob_cls else set() + @classmethod def validate_external_key(cls, external_key: str) -> bool: """Validate the external_key format for organization-level glob patterns. @@ -1118,6 +1201,17 @@ class PlatformGlobData(ScopeData): NAMESPACE: ClassVar[str] = "platform" IS_PLATFORM_GLOB: ClassVar[bool] = True + @property + def ancestors(self) -> set[str]: + """External keys of the scopes that hierarchically contain this one. + + Platform-level globs sit at the top of the hierarchy, so they have no ancestors. + + Returns: + set[str]: Always an empty set. + """ + return set() + @classmethod def validate_external_key(cls, external_key: str) -> bool: """Validate the external_key format for platform-level glob patterns. diff --git a/src/openedx_authz/api/users.py b/src/openedx_authz/api/users.py index 0d492ab4..68e9744b 100644 --- a/src/openedx_authz/api/users.py +++ b/src/openedx_authz/api/users.py @@ -309,6 +309,11 @@ def _filter_candidate_assignments_by_params( and are applied only when provided. This runs before the scope-based authorization pass to avoid paying the DB cost for assignments that would be dropped anyway. + When filtering by scope, the hierarchy is respected: assignments at higher levels + (org-level and platform-level globs) that apply to the queried scope are also + included. For example, filtering by ``course-v1:OpenedX+DemoX+DemoCourse`` will + also keep assignments scoped to ``course-v1:OpenedX+*`` and ``course-v1:*``. + Args: assignments: The full assignment list to filter. Each entry has exactly one role (one policy line), as produced by get_role_assignments. @@ -320,7 +325,8 @@ def _filter_candidate_assignments_by_params( The filtered assignment list. """ if scopes: - assignments = [a for a in assignments if a.scope.external_key in scopes] + expanded_scopes = ScopeData.expand_keys_with_ancestors(scopes) + assignments = [a for a in assignments if a.scope.external_key in expanded_scopes] if orgs: assignments = [a for a in assignments if getattr(a.scope, "org", None) in orgs] if roles: diff --git a/src/openedx_authz/tests/api/test_users.py b/src/openedx_authz/tests/api/test_users.py index 278a0063..e9ee99a5 100644 --- a/src/openedx_authz/tests/api/test_users.py +++ b/src/openedx_authz/tests/api/test_users.py @@ -14,6 +14,7 @@ PlatformCourseOverviewGlobData, RoleAssignmentData, RoleData, + ScopeData, UserData, ) from openedx_authz.api.users import ( @@ -939,3 +940,19 @@ def test_prefilter_is_applied_before_authorization(self): for a in authorized: self.assertEqual(getattr(a.scope, "org", None), "Org1") self.assertTrue(any(r.external_key == "library_admin" for r in a.roles)) + + +class TestExpandScopesWithAncestors(UserAssignmentsSetupMixin): + """Unit tests for ScopeData.expand_keys_with_ancestors.""" + + def test_unrecognized_scope_format_is_kept_without_expansion(self): + """A scope key that cannot be resolved keeps the original and skips expansion. + + This covers the ``except ValueError`` branch in expand_keys_with_ancestors + where ``ScopeData(external_key=...)`` raises because the key format is + invalid or the namespace is unknown. + """ + bogus_scope = "unknown-namespace:some-value" + result = ScopeData.expand_keys_with_ancestors([bogus_scope]) + + self.assertEqual(result, {bogus_scope}) diff --git a/src/openedx_authz/tests/rest_api/test_views.py b/src/openedx_authz/tests/rest_api/test_views.py index 795aac83..bdc59ee2 100644 --- a/src/openedx_authz/tests/rest_api/test_views.py +++ b/src/openedx_authz/tests/rest_api/test_views.py @@ -4491,6 +4491,261 @@ def test_user_with_both_library_and_course_permissions(self): self.assertIn("course-v1", scope_types) +class TestScopeHierarchyFiltering(ViewTestMixin): + """Test that filtering assignments by scope includes hierarchical ancestors. + + The scope hierarchy is:: + + specific resource → organization glob → platform glob + + When querying by a specific scope (e.g. ``course-v1:Org1+COURSE1+2024``), the + response must also include assignments at the org level (``course-v1:Org1+*``) and + platform level (``course-v1:*``). Analogous rules apply to library scopes. + """ + + @classmethod + def setUpClass(cls): + """Create assignments at every hierarchy level for courses and libraries. + + Assignment matrix (each row is one assignment): + + ======= ========================= =========================== ==== + User Role Scope # + ======= ========================= =========================== ==== + Courses + regular_1 course_staff course-v1:Org1+COURSE1+2024 1 ← specific + regular_2 course_staff course-v1:Org1+* 1 ← org glob + regular_3 course_staff course-v1:* 1 ← platform glob + + Libraries + regular_4 library_user lib:Org1:LIB1 1 ← specific + regular_5 library_user lib:Org1:* 1 ← org glob + regular_6 library_user lib:* 1 ← platform glob + ======= ========================= =========================== ==== + + Note: the base ``ViewTestMixin.setUpClass`` also creates library assignments. + The hierarchy tests account for these additional assignments in their + expected counts. + """ + super().setUpClass() + + cls._assign_roles_to_users( + [ + # -- Course hierarchy: Org1 -- + { + "subject_name": "regular_1", + "role_name": roles.COURSE_STAFF.external_key, + "scope_name": COURSE_SCOPE_ORG1, + }, + { + "subject_name": "regular_2", + "role_name": roles.COURSE_STAFF.external_key, + "scope_name": COURSE_ORG1_GLOB, + }, + { + "subject_name": "regular_3", + "role_name": roles.COURSE_STAFF.external_key, + "scope_name": PLATFORM_COURSE_GLOB, + }, + # -- Library hierarchy: Org1 -- + { + "subject_name": "regular_4", + "role_name": roles.LIBRARY_USER.external_key, + "scope_name": LIB_SCOPE_ORG1, + }, + { + "subject_name": "regular_5", + "role_name": roles.LIBRARY_USER.external_key, + "scope_name": LIB_ORG1_GLOB, + }, + { + "subject_name": "regular_6", + "role_name": roles.LIBRARY_USER.external_key, + "scope_name": PLATFORM_LIBRARY_GLOB, + }, + ] + ) + + def setUp(self): + """Set up test fixtures.""" + super().setUp() + self.url = reverse("openedx_authz:user-list") + + # -- Helpers ----------------------------------------------------------- # + + def _get_response(self, scopes: str): + """Query the team-members endpoint filtered by scopes and assert 200 OK.""" + response = self.client.get(self.url, {"scopes": scopes}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + return response + + def _collect_scopes_from_response(self, scopes: str) -> set[str]: + """Return the distinct scope values across all users' assignments.""" + response = self._get_response(scopes) + result_scopes: set[str] = set() + for user_entry in response.data["results"]: + for assignment in user_entry.get("assignments", []): + result_scopes.add(assignment["scope"]) + return result_scopes + + def _get_user_count(self, scopes: str) -> int: + """Return the number of users (count) from the response.""" + response = self._get_response(scopes) + return response.data["count"] + + # ================================================================== # + # Course scopes # + # ================================================================== # + + def test_course_specific_scope_includes_org_and_platform_ancestors(self): + """Filtering by a specific course returns users from org-glob and platform-glob too. + + Querying ``course-v1:Org1+COURSE1+2024`` should include users with assignments at: + - course-v1:Org1+COURSE1+2024 (regular_1) + - course-v1:Org1+* (regular_2) + - course-v1:* (regular_3) + Total users: 3 + """ + result_scopes = self._collect_scopes_from_response(COURSE_SCOPE_ORG1) + + self.assertIn(COURSE_SCOPE_ORG1, result_scopes) + self.assertIn(COURSE_ORG1_GLOB, result_scopes) + self.assertIn(PLATFORM_COURSE_GLOB, result_scopes) + + self.assertEqual(self._get_user_count(COURSE_SCOPE_ORG1), 3) + + def test_course_org_glob_includes_platform_ancestor(self): + """Filtering by an org-glob course scope returns users from platform-glob too. + + Querying ``course-v1:Org1+*`` should include users with assignments at: + - course-v1:Org1+* (regular_2) + - course-v1:* (regular_3) + Total users: 2 + """ + result_scopes = self._collect_scopes_from_response(COURSE_ORG1_GLOB) + + self.assertIn(COURSE_ORG1_GLOB, result_scopes) + self.assertIn(PLATFORM_COURSE_GLOB, result_scopes) + # Must NOT include specific course assignment + self.assertNotIn(COURSE_SCOPE_ORG1, result_scopes) + + self.assertEqual(self._get_user_count(COURSE_ORG1_GLOB), 2) + + def test_course_platform_glob_returns_only_platform(self): + """Filtering by platform-glob returns only platform-level course users. + + Querying ``course-v1:*`` should include only: + - course-v1:* (regular_3) + Total users: 1 + """ + result_scopes = self._collect_scopes_from_response(PLATFORM_COURSE_GLOB) + + self.assertIn(PLATFORM_COURSE_GLOB, result_scopes) + self.assertNotIn(COURSE_SCOPE_ORG1, result_scopes) + self.assertNotIn(COURSE_ORG1_GLOB, result_scopes) + + self.assertEqual(self._get_user_count(PLATFORM_COURSE_GLOB), 1) + + # ================================================================== # + # Library scopes # + # ================================================================== # + + def test_library_specific_scope_includes_org_and_platform_ancestors(self): + """Filtering by a specific library returns users from org-glob and platform-glob too. + + Querying ``lib:Org1:LIB1`` should include users with assignments at: + - lib:Org1:LIB1 (specific — includes base ViewTestMixin users) + - lib:Org1:* (regular_5) + - lib:* (regular_6) + """ + result_scopes = self._collect_scopes_from_response(LIB_SCOPE_ORG1) + + self.assertIn(LIB_SCOPE_ORG1, result_scopes) + self.assertIn(LIB_ORG1_GLOB, result_scopes) + self.assertIn(PLATFORM_LIBRARY_GLOB, result_scopes) + + def test_library_org_glob_includes_platform_ancestor(self): + """Filtering by an org-glob library scope returns users from platform-glob too. + + Querying ``lib:Org1:*`` should include users with assignments at: + - lib:Org1:* (regular_5) + - lib:* (regular_6) + Total users: 2 + """ + result_scopes = self._collect_scopes_from_response(LIB_ORG1_GLOB) + + self.assertIn(LIB_ORG1_GLOB, result_scopes) + self.assertIn(PLATFORM_LIBRARY_GLOB, result_scopes) + # Must NOT include specific library assignments + self.assertNotIn(LIB_SCOPE_ORG1, result_scopes) + + self.assertEqual(self._get_user_count(LIB_ORG1_GLOB), 2) + + def test_library_platform_glob_returns_only_platform(self): + """Filtering by platform-glob returns only platform-level library users. + + Querying ``lib:*`` should include only: + - lib:* (regular_6) + Total users: 1 + """ + result_scopes = self._collect_scopes_from_response(PLATFORM_LIBRARY_GLOB) + + self.assertIn(PLATFORM_LIBRARY_GLOB, result_scopes) + self.assertNotIn(LIB_SCOPE_ORG1, result_scopes) + self.assertNotIn(LIB_ORG1_GLOB, result_scopes) + + self.assertEqual(self._get_user_count(PLATFORM_LIBRARY_GLOB), 1) + + # ================================================================== # + # Cross-namespace isolation # + # ================================================================== # + + def test_course_scope_does_not_include_library_ancestors(self): + """Filtering by a course scope does not pull in library-namespace users. + + Querying ``course-v1:Org1+COURSE1+2024`` must not return any ``lib:`` scopes. + """ + result_scopes = self._collect_scopes_from_response(COURSE_SCOPE_ORG1) + + lib_scopes = {s for s in result_scopes if s.startswith("lib:")} + self.assertEqual(lib_scopes, set()) + + def test_library_scope_does_not_include_course_ancestors(self): + """Filtering by a library scope does not pull in course-namespace users. + + Querying ``lib:Org1:LIB1`` must not return any ``course-v1:`` scopes. + """ + result_scopes = self._collect_scopes_from_response(LIB_SCOPE_ORG1) + + course_scopes = {s for s in result_scopes if s.startswith("course-v1:")} + self.assertEqual(course_scopes, set()) + + # ================================================================== # + # Multiple scopes combined # + # ================================================================== # + + def test_multiple_scopes_each_expand_independently(self): + """Providing multiple scopes expands each one independently. + + Querying ``course-v1:Org1+COURSE1+2024,lib:Org1:LIB1`` should include users + from both hierarchies: + - Course ancestors: course-v1:Org1+*, course-v1:* + - Library ancestors: lib:Org1:*, lib:* + """ + combined = f"{COURSE_SCOPE_ORG1},{LIB_SCOPE_ORG1}" + result_scopes = self._collect_scopes_from_response(combined) + + # Course hierarchy + self.assertIn(COURSE_SCOPE_ORG1, result_scopes) + self.assertIn(COURSE_ORG1_GLOB, result_scopes) + self.assertIn(PLATFORM_COURSE_GLOB, result_scopes) + + # Library hierarchy + self.assertIn(LIB_SCOPE_ORG1, result_scopes) + self.assertIn(LIB_ORG1_GLOB, result_scopes) + self.assertIn(PLATFORM_LIBRARY_GLOB, result_scopes) + + @ddt class TestBulkPutScopesAllLogic(ViewTestMixin): """Test that DynamicScopePermission enforces AND logic across scopes in bulk PUT.