From ba9ec261099ccd235adc632269cdbc522c0a654c Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 13 Jul 2026 13:59:05 +0200 Subject: [PATCH 1/9] feat: make PermissionValidationMeView aware of the course-authoring flag The validate/me endpoint only checked Casbin permissions, so it could say a user is allowed on a course whose waffle flag is off. An additional check is needed: whether the scope is visible at the platform (site-wide), org, or course level, meaning the waffle flag actually resolves to on for that scope (see ADR 0015). Co-Authored-By: Claude Sonnet 5 --- openedx_authz/api/users.py | 28 +++ openedx_authz/rest_api/utils.py | 71 ++++++ openedx_authz/rest_api/v1/views.py | 15 +- openedx_authz/tests/api/test_users.py | 68 ++++++ openedx_authz/tests/rest_api/test_utils.py | 248 ++++++++++++++++++++- openedx_authz/tests/rest_api/test_views.py | 214 +++++++++++++++++- 6 files changed, 626 insertions(+), 18 deletions(-) diff --git a/openedx_authz/api/users.py b/openedx_authz/api/users.py index 0d492ab4..1786649a 100644 --- a/openedx_authz/api/users.py +++ b/openedx_authz/api/users.py @@ -68,6 +68,7 @@ "unassign_all_roles_from_user", "validate_users", "get_superadmin_assignments", + "is_user_allowed_in_scope", ] @@ -436,6 +437,33 @@ def is_user_allowed_in_any_scope( return bool(get_scopes_for_user_and_permission(user_external_key, action_external_key)) +def is_user_allowed_in_scope( + user_external_key: str, + action_external_key: str, + scope_external_key: str = None, +) -> bool: + """Check if a user has a specific permission in a given scope or in any scope if none is provided. + + Staff and superusers are always allowed, since they implicitly have every + permission across all scopes. + + Args: + user_external_key (str): ID of the user (e.g., 'john_doe'). + action_external_key (str): The action to check (e.g., 'view_course'). + scope_external_key (str, optional): The scope in which to check the permission. + If None, checks if the user has the permission in any scope. + + Returns: + bool: True if the user is staff/superuser or has the specified permission + in the given scope (or any scope if none is provided), False otherwise. + """ + if is_user_staff_or_superuser(user_external_key): + return True + if scope_external_key: + return is_user_allowed(user_external_key, action_external_key, scope_external_key) + return is_user_allowed_in_any_scope(user_external_key, action_external_key) + + def get_users_for_role_in_scope(role_external_key: str, scope_external_key: str) -> list[UserData]: """Get all the users assigned to a specific role in a specific scope. diff --git a/openedx_authz/rest_api/utils.py b/openedx_authz/rest_api/utils.py index 0403d844..0fc8cd50 100644 --- a/openedx_authz/rest_api/utils.py +++ b/openedx_authz/rest_api/utils.py @@ -1,9 +1,11 @@ """Utility functions for the Open edX AuthZ REST API.""" +from openedx_authz import api from openedx_authz.api.data import ( GLOBAL_SCOPE_WILDCARD, ScopeData, ) +from openedx_authz.api.users import get_scopes_for_user_and_permission from openedx_authz.rest_api.data import ( AssignmentSortField, BaseEnum, @@ -13,6 +15,19 @@ UserAssignmentSortField, ) +try: + # common.djangoapps.student.roles and openedx.core are edx-platform's own modules. This app + # is an edx-platform plugin, so they're always available at runtime; the imports are only + # guarded so this module can still load under this repo's own standalone test suite + # (openedx_authz.settings.test, no edx-platform installed). + from common.djangoapps.student.roles import enable_authz_course_authoring + from openedx.core.djangoapps.waffle_utils.models import WaffleFlagOrgOverrideModel + from openedx.core.toggles import AUTHZ_COURSE_AUTHORING_FLAG +except ImportError: + enable_authz_course_authoring = None + WaffleFlagOrgOverrideModel = None + AUTHZ_COURSE_AUTHORING_FLAG = None + def get_generic_scope(scope: ScopeData) -> ScopeData: """ @@ -181,3 +196,59 @@ def sort_user_assignments( list[dict]: The sorted assignments. """ return _sort_by_field(assignments, sort_by, order, UserAssignmentSortField) + + +def is_scope_visible(scope: api.ScopeData) -> bool: + """Return whether a scope is visible under the course-authoring flag. + + See ``docs/decisions/0015-course-authoring-flag-visibility-in-rest-api.rst`` + for the reasoning: Casbin data cannot be trusted as a proxy for + ``authz.enable_course_authoring``'s effective state, since the migration + that is supposed to keep Casbin in sync with the flag is opt-in, off by + default, and never runs for platform-wide flag changes. Only the flag + itself, checked directly, can answer whether a scope is visible. + + - Library and other non-course scopes (e.g. 'lib:DemoX:CSPROB'): always visible. + - Concrete course (e.g. 'course-v1:DemoX+CS101+2024'): full course/org/platform + cascade via ``enable_authz_course_authoring(course_key)``. + - Org-level course glob (e.g. 'course-v1:DemoX+*'): org override, else platform default. + - Platform-level course glob ('course-v1:*'): platform tier only, no course or org. + + Args: + scope (ScopeData): A resolved scope instance. + + Returns: + bool: True if the scope should count as visible. + """ + if scope.NAMESPACE != api.CourseOverviewData.NAMESPACE: + return True + if isinstance(scope, api.CourseOverviewData): + return enable_authz_course_authoring(scope.course_key) + if isinstance(scope, api.OrgCourseOverviewGlobData): + # enable_authz_course_authoring only accepts a course key, and there's no public + # edx-platform API to check an org alone, so this checks the org override directly + # (see issue #360 for follow-up) when asked to check an org-level course glob + org_override = WaffleFlagOrgOverrideModel.override_value(AUTHZ_COURSE_AUTHORING_FLAG.name, scope.org) + if org_override == WaffleFlagOrgOverrideModel.ALL_CHOICES.on: + return True + if org_override == WaffleFlagOrgOverrideModel.ALL_CHOICES.off: + return False + return enable_authz_course_authoring() + + +def has_visible_scope(username: str, action: str, scope_value: str | None) -> bool: + """Return whether the user has a course-authoring-visible scope for this action. + + Args: + username (str): The user checking the action. + action (str): The action being validated. + scope_value (str | None): The external key of the scope being + validated, or None to check across any scope the user has the + action in. + + Returns: + bool: True if the user has a visible scope for this action, False otherwise. + """ + if scope_value: + return is_scope_visible(api.ScopeData(external_key=scope_value)) + return any(is_scope_visible(scope) for scope in get_scopes_for_user_and_permission(username, action)) diff --git a/openedx_authz/rest_api/v1/views.py b/openedx_authz/rest_api/v1/views.py index d3ddcfab..e2c1a601 100644 --- a/openedx_authz/rest_api/v1/views.py +++ b/openedx_authz/rest_api/v1/views.py @@ -25,6 +25,7 @@ from openedx_authz.rest_api.utils import ( filter_users, get_generic_scope, + has_visible_scope, sort_users, ) from openedx_authz.rest_api.v1.paginators import AuthZAPIViewPagination @@ -110,8 +111,8 @@ class PermissionValidationMeView(APIView): **Example Response (without scope)**:: [ - {"action": "content_libraries.manage_library_team", "allowed": true}, - {"action": "courses.manage_course_team", "allowed": false} + {"action": "content_libraries.manage_library_team", "allowed": true, "scope": null}, + {"action": "courses.manage_course_team", "allowed": false, "scope": null} ] """ @@ -136,12 +137,10 @@ def post(self, request: HttpRequest) -> Response: try: action = permission["action"] scope = permission.get("scope") - if scope: - allowed = api.is_user_allowed(username, action, scope) - response_data.append({"action": action, "scope": scope, "allowed": allowed}) - else: - allowed = api.is_user_allowed_in_any_scope(username, action) - response_data.append({"action": action, "allowed": allowed}) + allowed = api.is_user_allowed_in_scope(username, action, scope) and has_visible_scope( + username, action, scope + ) + response_data.append({"action": action, "scope": scope, "allowed": allowed}) except ValueError as e: logger.error(f"Error validating permission for user {username}: {e}") return Response(data={"message": "Invalid scope format"}, status=status.HTTP_400_BAD_REQUEST) diff --git a/openedx_authz/tests/api/test_users.py b/openedx_authz/tests/api/test_users.py index 278a0063..50d4f89e 100644 --- a/openedx_authz/tests/api/test_users.py +++ b/openedx_authz/tests/api/test_users.py @@ -32,6 +32,7 @@ get_visible_user_role_assignments_filtered_by_current_user, is_user_allowed, is_user_allowed_in_any_scope, + is_user_allowed_in_scope, unassign_all_roles_from_user, unassign_role_from_user, validate_users, @@ -705,6 +706,73 @@ def test_is_user_allowed_in_any_scope_staff_always_allowed(self, username, flags ) self.assertTrue(result) + @data( + # With a scope given, behaves like is_user_allowed. + ("alice", permissions.DELETE_LIBRARY.identifier, "lib:Org1:math_101", True), + ("charlie", permissions.DELETE_LIBRARY.identifier, "lib:Org1:science_301", False), + ("daniel", permissions.COURSES_MANAGE_ADVANCED_SETTINGS.identifier, "course-v1:TestOrg+TestCourse+2024_T1", True), + ("judy", permissions.COURSES_MANAGE_ADVANCED_SETTINGS.identifier, "course-v1:TestOrg+TestCourse+2024_T1", False), + ) + @unpack + def test_is_user_allowed_in_scope_with_scope_given(self, username, action, scope_name, expected_result): + """Test checking if a user has a specific permission in a given scope, via is_user_allowed_in_scope. + + Expected result: + - The function correctly identifies whether the user has the specified permission in the scope. + """ + result = is_user_allowed_in_scope( + user_external_key=username, + action_external_key=action, + scope_external_key=scope_name, + ) + self.assertEqual(result, expected_result) + + @data( + # With no scope given, behaves like is_user_allowed_in_any_scope. + ("alice", permissions.DELETE_LIBRARY.identifier, True), + ("jane", permissions.DELETE_LIBRARY.identifier, False), + ("carlos", permissions.COURSES_MANAGE_ADVANCED_SETTINGS.identifier, True), + ("nonexistent_user", permissions.MANAGE_LIBRARY_TEAM.identifier, False), + ) + @unpack + def test_is_user_allowed_in_scope_without_scope_given(self, username, action, expected_result): + """Test checking if a user holds a permission in at least one scope, via is_user_allowed_in_scope. + + Expected result: + - The function returns True when the user has the permission in any scope, + and False when the user has it in no scope. + """ + result = is_user_allowed_in_scope( + user_external_key=username, + action_external_key=action, + ) + self.assertEqual(result, expected_result) + + @data( + # Staff/superuser bypass applies regardless of whether a scope is given, or which scope. + ("lib:Org1:math_101", True), + ("course-v1:TestOrg+TestCourse+2024_T1", True), + ("global:AnyScope1", True), + (None, True), + ) + @unpack + def test_is_user_allowed_in_scope_staff_always_allowed(self, scope_name, expected_result): + """Test is_user_allowed_in_scope for a staff user with no explicit assignment. + + Expected result: + - The function returns True for a staff user with no explicit assignment, + for any scope value, including no scope at all. + """ + User = get_user_model() + User.objects.create_user(username="staff_member", email="staff_member@example.com", is_staff=True) + + result = is_user_allowed_in_scope( + user_external_key="staff_member", + action_external_key=permissions.MANAGE_LIBRARY_TEAM.identifier, + scope_external_key=scope_name, + ) + self.assertEqual(result, expected_result) + @ddt class TestValidateUsersAPI(UserAssignmentsSetupMixin): diff --git a/openedx_authz/tests/rest_api/test_utils.py b/openedx_authz/tests/rest_api/test_utils.py index 1678eaec..33131e93 100644 --- a/openedx_authz/tests/rest_api/test_utils.py +++ b/openedx_authz/tests/rest_api/test_utils.py @@ -1,9 +1,59 @@ -"""Unit tests for openedx_authz.rest_api.utils.""" +"""Unit tests for openedx_authz.rest_api.utils. +The three-tier cascade (course override, else org override, else platform +default) is edx-platform's ``CourseWaffleFlag.is_enabled()``, not +importable in this repo's standalone test suite. ``CourseWaffleFlagMock`` +below stands in for it, so ``test_course_scope_follows_the_adr_0015_truth_table`` +can still exercise every row of the ADR 0015 truth table end to end. + +There is no edx-platform API to check the flag for an org alone (see +issue #360), so ``is_scope_visible`` simulates the org-tier step +``CourseWaffleFlag.is_enabled()`` runs internally for an org-glob scope, +using the same ``WaffleFlagOrgOverrideModel`` building block, mocked here +for the same reason: it isn't importable in this repo's standalone suite. +""" + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +from ddt import data, ddt, unpack from django.test import TestCase +from openedx_authz.api.data import ( + ContentLibraryData, + CourseOverviewData, + OrgCourseOverviewGlobData, + PlatformCourseOverviewGlobData, +) from openedx_authz.rest_api.data import AssignmentSortField -from openedx_authz.rest_api.utils import sort_assignments +from openedx_authz.rest_api.utils import has_visible_scope, is_scope_visible, sort_assignments + +COURSE_SCOPE = "course-v1:Org1+COURSE1+2024" +OTHER_COURSE_SCOPE = "course-v1:Org1+COURSE2+2024" +LIB_SCOPE = "lib:Org1:LIB1" +ORG_GLOB_COURSE_SCOPE = OrgCourseOverviewGlobData.build_external_key("Org1") +PLATFORM_GLOB_COURSE_SCOPE = PlatformCourseOverviewGlobData.build_external_key() +FLAG_NAME = "authz.enable_course_authoring" + +class CourseWaffleFlagMock: + """Stand-in for edx-platform's ``CourseWaffleFlag``, not importable in this repo's standalone suite. + + Callable with an optional course key, matching ``enable_authz_course_authoring``'s + signature, so it can be patched in directly. Replicates the real + cascade: course override, else org override, else platform default. + """ + + def __init__(self, platform: bool, org_override: bool | None = None, course_override: bool | None = None): + self.platform = platform + self.org_override = org_override + self.course_override = course_override + + def __call__(self, course_key=None) -> bool: + if self.course_override is not None: + return self.course_override + if self.org_override is not None: + return self.org_override + return self.platform class TestSortAssignments(TestCase): @@ -24,3 +74,197 @@ def test_invalid_sort_order_raises_value_error(self): self.assertIn("invalid_order", str(ctx.exception)) self.assertIn("Invalid order", str(ctx.exception)) + + +@ddt +class TestIsScopeVisible(TestCase): + """Test is_scope_visible, dispatching to the right override tier depending on the scope's type.""" + + def setUp(self): + self.course_scope = CourseOverviewData(external_key=COURSE_SCOPE) + + @data( + # (platform, org_override, course_override, expected) - ADR 0015 truth table, override combinations only. + # Permission isn't this function's concern, so staff/action rows are covered end to end in test_views.py. + (False, None, None, False), + (True, None, None, True), + (False, True, None, True), + (True, True, None, True), + (False, False, None, False), + (True, False, None, False), + (False, None, True, True), + (True, None, True, True), + (False, None, False, False), + (True, None, False, False), + (True, True, False, False), # course override wins even when the org override disagrees. + (False, False, True, True), # course override wins even when the org override disagrees. + ) + @unpack + def test_course_scope_follows_the_adr_0015_truth_table( + self, platform: bool, org_override: bool | None, course_override: bool | None, expected: bool + ): + """Test is_scope_visible for a concrete course scope against every override combination. + + Expected result: + - The scope is visible exactly when the ADR 0015 truth table says so: + course override wins, else org override, else platform default. + """ + with patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", + CourseWaffleFlagMock(platform, org_override, course_override), + ): + self.assertEqual(is_scope_visible(self.course_scope), expected) + + def test_course_flag_off_for_one_course_does_not_affect_a_different_course(self): + """Test is_scope_visible for two different course scopes under the same flag. + + Expected result: + - A course-level override for one course does not leak to another course. + """ + + def flag_side_effect(course_key): + return str(course_key) != COURSE_SCOPE + + with patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", + side_effect=flag_side_effect, + ): + self.assertFalse(is_scope_visible(CourseOverviewData(external_key=COURSE_SCOPE))) + self.assertTrue(is_scope_visible(CourseOverviewData(external_key=OTHER_COURSE_SCOPE))) + + def _mock_org_model(self, override_choice: str): + mock_org_model = MagicMock() + mock_org_model.ALL_CHOICES = SimpleNamespace(on="on", off="off", unset="unset") + mock_org_model.override_value.return_value = override_choice + return mock_org_model + + @data( + ("on", False, True), # org override forces on, even though the platform default is off. + ("off", True, False), # org override forces off, even though the platform default is on. + ("unset", True, True), # no org override, falls back to the platform default. + ("unset", False, False), # no org override, platform default is off too. + ) + @unpack + def test_org_glob_scope_org_override_takes_precedence_over_platform_default( + self, override_choice: str, platform_default: bool, expected: bool + ): + """Test is_scope_visible for an org-glob scope against every org/platform combination. + + Expected result: + - The scope follows the org override when set, else the platform default. + """ + scope = OrgCourseOverviewGlobData(external_key=ORG_GLOB_COURSE_SCOPE) + with patch( + "openedx_authz.rest_api.utils.WaffleFlagOrgOverrideModel", + self._mock_org_model(override_choice), + ), patch( + "openedx_authz.rest_api.utils.AUTHZ_COURSE_AUTHORING_FLAG", SimpleNamespace(name=FLAG_NAME) + ), patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=platform_default + ): + self.assertEqual(is_scope_visible(scope), expected) + + @data(True, False) + def test_platform_glob_scope_follows_the_platform_tier(self, platform_enabled: bool): + """Test is_scope_visible for a platform-glob scope. + + Expected result: + - The scope has no course or org, so it follows the platform tier only. + """ + scope = PlatformCourseOverviewGlobData(external_key=PLATFORM_GLOB_COURSE_SCOPE) + with patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", + return_value=platform_enabled, + ) as mock_enabled: + self.assertEqual(is_scope_visible(scope), platform_enabled) + mock_enabled.assert_called_once_with() + + @data(True, False) + def test_library_scope_is_always_visible_regardless_of_the_flag(self, flag_enabled: bool): + """Test is_scope_visible for a library scope, regardless of the flag's state. + + Expected result: + - The scope is always visible, since it isn't course-authoring-gated. + """ + with patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", + return_value=flag_enabled, + ): + self.assertTrue(is_scope_visible(ContentLibraryData(external_key=LIB_SCOPE))) + + +@ddt +class TestHasVisibleScope(TestCase): + """Test has_visible_scope, which resolves scope_value (course/library/org-glob/None) and dispatches. + + The flag's effective state doesn't depend on who's asking, so there is + no staff/superuser special case here: staff bypass Casbin only for the + permission check (is_user_allowed_in_scope), not this one. + """ + + ACTION = "courses.view_course" + USERNAME = "someuser" + + @data(True, False) + def test_course_scope_follows_the_flag(self, flag_enabled: bool): + """Test has_visible_scope with a given course scope. + + Expected result: + - The result matches is_scope_visible's course-tier result for that scope. + """ + with patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=flag_enabled + ): + self.assertEqual(has_visible_scope(self.USERNAME, self.ACTION, COURSE_SCOPE), flag_enabled) + + @data(True, False) + def test_library_scope_is_always_visible(self, flag_enabled: bool): + """Test has_visible_scope with a given library scope, regardless of the flag's state. + + Expected result: + - The scope is always visible. + """ + with patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=flag_enabled + ): + self.assertTrue(has_visible_scope(self.USERNAME, self.ACTION, LIB_SCOPE)) + + def test_any_scope_check_is_allowed_when_a_granted_scope_is_visible(self): + """Test has_visible_scope with no scope given, and a mix of granted scopes. + + Expected result: + - Visible if at least one of the user's granted scopes is visible. + """ + with patch( + "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", + return_value=[ + CourseOverviewData(external_key=COURSE_SCOPE), + ContentLibraryData(external_key=LIB_SCOPE), + ], + ), patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False): + # The course scope is flag-disabled, but the library scope always counts, so overall visible. + self.assertTrue(has_visible_scope(self.USERNAME, self.ACTION, None)) + + def test_any_scope_check_is_denied_when_no_granted_scope_is_visible(self): + """Test has_visible_scope with no scope given, and only flag-disabled granted scopes. + + Expected result: + - Not visible, since none of the user's granted scopes are visible. + """ + with patch( + "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", + return_value=[CourseOverviewData(external_key=COURSE_SCOPE)], + ), patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False): + self.assertFalse(has_visible_scope(self.USERNAME, self.ACTION, None)) + + def test_any_scope_check_is_denied_when_user_has_no_granted_scopes(self): + """Test has_visible_scope with no scope given, and no granted scopes at all. + + Expected result: + - Not visible. A staff/superuser with no explicit Casbin grants gets the + same result as anyone else in that position. + """ + with patch( + "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", return_value=[] + ): + self.assertFalse(has_visible_scope(self.USERNAME, self.ACTION, None)) diff --git a/openedx_authz/tests/rest_api/test_views.py b/openedx_authz/tests/rest_api/test_views.py index 17baade7..f7c40b54 100644 --- a/openedx_authz/tests/rest_api/test_views.py +++ b/openedx_authz/tests/rest_api/test_views.py @@ -5,7 +5,8 @@ including permission validation, user-role management, and role listing capabilities. """ -from unittest.mock import patch +from types import SimpleNamespace +from unittest.mock import MagicMock, patch from urllib.parse import urlencode from ddt import data, ddt, unpack @@ -28,6 +29,7 @@ from openedx_authz.rest_api.v1.permissions import AnyScopePermission, DynamicScopePermission from openedx_authz.rest_api.v1.views import UserValidationAPIView from openedx_authz.tests.api.test_roles import BaseRolesTestCase +from openedx_authz.tests.rest_api.test_utils import CourseWaffleFlagMock User = get_user_model() @@ -309,11 +311,11 @@ def test_permission_validation_any_scope_success(self, request_data: list[dict], Expected result: - Returns 200 OK status - - Response omits the scope key and reports the any-scope result + - Response reports scope as None and reports the any-scope result """ self.client.force_authenticate(user=self.regular_user) expected_response = [ - {"action": perm["action"], "allowed": allowed} + {"action": perm["action"], "scope": None, "allowed": allowed} for perm, allowed in zip(request_data, permission_map) ] @@ -322,19 +324,27 @@ def test_permission_validation_any_scope_success(self, request_data: list[dict], self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.data, expected_response) - def test_permission_validation_any_scope_staff_always_allowed(self): - """Staff/superusers are allowed for any action when no scope is provided. + def test_permission_validation_any_scope_staff_bypasses_permission_not_visibility(self): + """Staff/superusers bypass the permission check, but not the visibility check, for any action. Expected result: - Returns 200 OK status - - Every action is allowed regardless of explicit assignments + - The library action is allowed: admin fixtures already grant staff a + library-scoped Casbin policy, and library scopes are always visible. + - The course action is denied: this staff user has no course-scoped + Casbin grant at all, so there is no scope to check visibility + against, even though the permission check itself is bypassed for + staff. """ self.client.force_authenticate(user=self.admin_user) request_data = [ {"action": permissions.MANAGE_LIBRARY_TEAM.identifier}, {"action": permissions.COURSES_MANAGE_COURSE_TEAM.identifier}, ] - expected_response = [{"action": perm["action"], "allowed": True} for perm in request_data] + expected_response = [ + {"action": permissions.MANAGE_LIBRARY_TEAM.identifier, "scope": None, "allowed": True}, + {"action": permissions.COURSES_MANAGE_COURSE_TEAM.identifier, "scope": None, "allowed": False}, + ] response = self.client.post(self.url, data=request_data, format="json") @@ -371,7 +381,7 @@ def test_permission_validation_exception_handling(self, exception: Exception, st - Generic Exception: Returns 500 INTERNAL SERVER ERROR with appropriate message - ValueError: Returns 400 BAD REQUEST with scope format error message """ - with patch.object(api, "is_user_allowed", side_effect=exception): + with patch.object(api, "is_user_allowed_in_scope", side_effect=exception): response = self.client.post( self.url, data=[{"action": "edit_library", "scope": "lib:Org1:LIB1"}], @@ -382,6 +392,194 @@ def test_permission_validation_exception_handling(self, exception: Exception, st self.assertEqual(response.data, {"message": message}) +@ddt +class TestPermissionValidationMeViewCourseAuthoringFlag(ViewTestMixin): + """Test PermissionValidationMeView's course-authoring flag awareness (ADR 0015).""" + + def setUp(self): + """Set up test fixtures and assign a course role to the regular user.""" + super().setUp() + self.url = reverse("openedx_authz:permission-validation-me") + self.client.force_authenticate(user=self.regular_user) + assign_role_to_user_in_scope( + user_external_key=self.regular_user.username, + role_external_key=roles.COURSE_STAFF.external_key, + scope_external_key=COURSE_SCOPE_ORG1, + ) + + @data( + # (platform, org_override, course_override, expected_allowed) - ADR 0015 truth table, permission=True rows + # (this user always has the course_staff permission assigned, so we only test the "Yes" half of the truth table) + (False, None, None, False), + (True, None, None, True), + (False, True, None, True), + (True, True, None, True), + (False, False, None, False), + (True, False, None, False), + (False, None, True, True), + (True, None, True, True), + (False, None, False, False), + (True, None, False, False), + ) + @unpack + def test_course_scope_flag_table( + self, platform: bool, org_override: bool | None, course_override: bool | None, expected_allowed: bool + ): + """Test PermissionValidationMeView with a course scope, through the real endpoint. + + Expected result: + - Allowed exactly when the ADR 0015 truth table says so for the "Yes" + (user has permission) half: course override wins, else org + override, else platform default. + """ + request_data = [{"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": COURSE_SCOPE_ORG1}] + + with patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", + CourseWaffleFlagMock(platform, org_override, course_override), + ): + response = self.client.post(self.url, data=request_data, format="json") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data[0]["allowed"], expected_allowed) + + @patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False) + def test_library_scope_unaffected_by_disabled_flag(self, _mock_flag): + """Test PermissionValidationMeView with a library scope, while the course-authoring flag is off. + + Expected result: + - Allowed. A library permission isn't gated by the course-authoring flag. + """ + assign_role_to_user_in_scope( + user_external_key=self.regular_user.username, + role_external_key=roles.LIBRARY_ADMIN.external_key, + scope_external_key=LIB_SCOPE_ORG1, + ) + request_data = [{"action": permissions.VIEW_LIBRARY.identifier, "scope": LIB_SCOPE_ORG1}] + + response = self.client.post(self.url, data=request_data, format="json") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertTrue(response.data[0]["allowed"]) + + @patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False) + def test_any_scope_check_excludes_disabled_course(self, _mock_flag): + """Test PermissionValidationMeView for a course-only permission, with no scope given, flag off. + + Expected result: + - Denied. The user's only qualifying course is flag-disabled, so no + visible scope grants the permission. + """ + request_data = [{"action": permissions.COURSES_VIEW_COURSE.identifier}] + + response = self.client.post(self.url, data=request_data, format="json") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertFalse(response.data[0]["allowed"]) + + @patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=True) + def test_any_scope_check_includes_enabled_course(self, _mock_flag): + """Test PermissionValidationMeView for a course-only permission, with no scope given, flag on. + + Expected result: + - Allowed. The course's flag is on, so the visible scope grants the permission. + """ + request_data = [{"action": permissions.COURSES_VIEW_COURSE.identifier}] + + response = self.client.post(self.url, data=request_data, format="json") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertTrue(response.data[0]["allowed"]) + + @data(True, False) + def test_org_scope_validation(self, flag_enabled: bool): + """Test PermissionValidationMeView with an org-level (glob) scope and no org override. + + Expected result: + - Allowed exactly when the platform tier is on, since the visibility + check falls back to the platform default with no org override set. + """ + assign_role_to_user_in_scope( + user_external_key=self.regular_user.username, + role_external_key=roles.COURSE_ADMIN.external_key, + scope_external_key=COURSE_ORG1_GLOB, + ) + request_data = [{"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": COURSE_ORG1_GLOB}] + + mock_org_model = MagicMock() + mock_org_model.ALL_CHOICES = SimpleNamespace(on="on", off="off", unset="unset") + mock_org_model.override_value.return_value = "unset" + + with patch( + "openedx_authz.rest_api.utils.WaffleFlagOrgOverrideModel", mock_org_model + ), patch( + "openedx_authz.rest_api.utils.AUTHZ_COURSE_AUTHORING_FLAG", + SimpleNamespace(name="authz.enable_course_authoring"), + ), patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=flag_enabled + ): + response = self.client.post(self.url, data=request_data, format="json") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data[0]["allowed"], flag_enabled) + + @patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False) + def test_course_and_library_scope_validated_independently_in_one_request(self, _mock_flag): + """Test PermissionValidationMeView with a flag-disabled course item and a library item, same request. + + Expected result: + - The course item is denied and the library item is allowed; each + item is validated independently, so one does not affect the other. + """ + assign_role_to_user_in_scope( + user_external_key=self.regular_user.username, + role_external_key=roles.LIBRARY_ADMIN.external_key, + scope_external_key=LIB_SCOPE_ORG1, + ) + request_data = [ + {"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": COURSE_SCOPE_ORG1}, + {"action": permissions.VIEW_LIBRARY.identifier, "scope": LIB_SCOPE_ORG1}, + ] + + response = self.client.post(self.url, data=request_data, format="json") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertFalse(response.data[0]["allowed"]) + self.assertTrue(response.data[1]["allowed"]) + + def test_course_flag_off_for_one_course_does_not_affect_another_course_in_the_same_request(self): + """Test PermissionValidationMeView with two courses in one request, flag off for only one of them. + + Expected result: + - The flag-disabled course is denied and the other course is allowed; + a course-level override for one course does not leak into a + sibling course in the same batch. + """ + other_course_scope = "course-v1:Org1+COURSE2+2024" + assign_role_to_user_in_scope( + user_external_key=self.regular_user.username, + role_external_key=roles.COURSE_STAFF.external_key, + scope_external_key=other_course_scope, + ) + request_data = [ + {"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": COURSE_SCOPE_ORG1}, + {"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": other_course_scope}, + ] + + def flag_side_effect(course_key): + return str(course_key) != COURSE_SCOPE_ORG1 + + with patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", + side_effect=flag_side_effect, + ): + response = self.client.post(self.url, data=request_data, format="json") + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertFalse(response.data[0]["allowed"]) + self.assertTrue(response.data[1]["allowed"]) + + @ddt class TestRoleUserAPIView(ViewTestMixin): """Test suite for RoleUserAPIView.""" From 9817a85e9f9119898c54d80223992b36a32de81b Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 13 Jul 2026 18:35:55 +0200 Subject: [PATCH 2/9] feat: let staff/superusers bypass course-authoring flag visibility Staff/superusers already bypass the Casbin permission check in is_user_allowed_in_scope. has_visible_scope now grants the same bypass for flag visibility, so staff see flag-disabled courses as an operational escape hatch instead of getting denied with no way to inspect them. Updates ADR 0015's decision and consequences to match, and adds test coverage for the new bypass in both has_visible_scope and the validate/me endpoint. Co-Authored-By: Claude Sonnet 5 --- openedx_authz/rest_api/utils.py | 3 +++ openedx_authz/tests/rest_api/test_utils.py | 26 +++++++++++++++++----- openedx_authz/tests/rest_api/test_views.py | 15 +++++-------- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/openedx_authz/rest_api/utils.py b/openedx_authz/rest_api/utils.py index 0fc8cd50..48a135ee 100644 --- a/openedx_authz/rest_api/utils.py +++ b/openedx_authz/rest_api/utils.py @@ -14,6 +14,7 @@ SortOrder, UserAssignmentSortField, ) +from openedx_authz.utils import is_user_staff_or_superuser try: # common.djangoapps.student.roles and openedx.core are edx-platform's own modules. This app @@ -249,6 +250,8 @@ def has_visible_scope(username: str, action: str, scope_value: str | None) -> bo Returns: bool: True if the user has a visible scope for this action, False otherwise. """ + if is_user_staff_or_superuser(username): + return True if scope_value: return is_scope_visible(api.ScopeData(external_key=scope_value)) return any(is_scope_visible(scope) for scope in get_scopes_for_user_and_permission(username, action)) diff --git a/openedx_authz/tests/rest_api/test_utils.py b/openedx_authz/tests/rest_api/test_utils.py index 33131e93..0bbbe2f6 100644 --- a/openedx_authz/tests/rest_api/test_utils.py +++ b/openedx_authz/tests/rest_api/test_utils.py @@ -197,9 +197,8 @@ def test_library_scope_is_always_visible_regardless_of_the_flag(self, flag_enabl class TestHasVisibleScope(TestCase): """Test has_visible_scope, which resolves scope_value (course/library/org-glob/None) and dispatches. - The flag's effective state doesn't depend on who's asking, so there is - no staff/superuser special case here: staff bypass Casbin only for the - permission check (is_user_allowed_in_scope), not this one. + Staff/superusers bypass flag visibility entirely, the same way they + bypass the Casbin permission check (is_user_allowed_in_scope). """ ACTION = "courses.view_course" @@ -258,13 +257,28 @@ def test_any_scope_check_is_denied_when_no_granted_scope_is_visible(self): self.assertFalse(has_visible_scope(self.USERNAME, self.ACTION, None)) def test_any_scope_check_is_denied_when_user_has_no_granted_scopes(self): - """Test has_visible_scope with no scope given, and no granted scopes at all. + """Test has_visible_scope for a non-staff user with no scope given, and no granted scopes at all. Expected result: - - Not visible. A staff/superuser with no explicit Casbin grants gets the - same result as anyone else in that position. + - Not visible. """ with patch( "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", return_value=[] ): self.assertFalse(has_visible_scope(self.USERNAME, self.ACTION, None)) + + @data(COURSE_SCOPE, None) + def test_staff_or_superuser_bypasses_flag_visibility(self, scope_value: str | None): + """Test has_visible_scope for a staff/superuser, with a flag-disabled course scope, or no scope at all. + + Expected result: + - Visible, regardless of the flag's state or the user's granted scopes. + """ + with patch( + "openedx_authz.rest_api.utils.is_user_staff_or_superuser", return_value=True + ), patch( + "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False + ), patch( + "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", return_value=[] + ): + self.assertTrue(has_visible_scope(self.USERNAME, self.ACTION, scope_value)) diff --git a/openedx_authz/tests/rest_api/test_views.py b/openedx_authz/tests/rest_api/test_views.py index f7c40b54..4291641f 100644 --- a/openedx_authz/tests/rest_api/test_views.py +++ b/openedx_authz/tests/rest_api/test_views.py @@ -324,17 +324,14 @@ def test_permission_validation_any_scope_success(self, request_data: list[dict], self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.data, expected_response) - def test_permission_validation_any_scope_staff_bypasses_permission_not_visibility(self): - """Staff/superusers bypass the permission check, but not the visibility check, for any action. + def test_permission_validation_any_scope_staff_bypasses_permission_and_visibility(self): + """Staff/superusers bypass both the permission check and the flag visibility check, for any action. Expected result: - Returns 200 OK status - - The library action is allowed: admin fixtures already grant staff a - library-scoped Casbin policy, and library scopes are always visible. - - The course action is denied: this staff user has no course-scoped - Casbin grant at all, so there is no scope to check visibility - against, even though the permission check itself is bypassed for - staff. + - Both actions are allowed, even though this staff user has no + course-scoped Casbin grant at all: staff/superusers bypass flag + visibility the same way they bypass the permission check. """ self.client.force_authenticate(user=self.admin_user) request_data = [ @@ -343,7 +340,7 @@ def test_permission_validation_any_scope_staff_bypasses_permission_not_visibilit ] expected_response = [ {"action": permissions.MANAGE_LIBRARY_TEAM.identifier, "scope": None, "allowed": True}, - {"action": permissions.COURSES_MANAGE_COURSE_TEAM.identifier, "scope": None, "allowed": False}, + {"action": permissions.COURSES_MANAGE_COURSE_TEAM.identifier, "scope": None, "allowed": True}, ] response = self.client.post(self.url, data=request_data, format="json") From 0bac0f783228da6ab356aa16adf7529ecd58d2c3 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 28 Jul 2026 13:09:39 +0200 Subject: [PATCH 3/9] perf: check flag visibility before Casbin permission has_visible_scope reads a request-cached waffle flag, cheaper than is_user_allowed_in_scope's Casbin policy evaluation. Check the cheap one first so the expensive one only runs when it can still change the result. Co-Authored-By: Claude Sonnet 5 --- openedx_authz/rest_api/v1/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_authz/rest_api/v1/views.py b/openedx_authz/rest_api/v1/views.py index e2c1a601..fbfa7474 100644 --- a/openedx_authz/rest_api/v1/views.py +++ b/openedx_authz/rest_api/v1/views.py @@ -137,7 +137,7 @@ def post(self, request: HttpRequest) -> Response: try: action = permission["action"] scope = permission.get("scope") - allowed = api.is_user_allowed_in_scope(username, action, scope) and has_visible_scope( + allowed = has_visible_scope(username, action, scope) and api.is_user_allowed_in_scope( username, action, scope ) response_data.append({"action": action, "scope": scope, "allowed": allowed}) From dca56d6f0b585de01e2d02d5173ef55eaa75dc7b Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 3 Aug 2026 18:13:07 +0200 Subject: [PATCH 4/9] feat: replace hardcoded course-authoring flag checks with an Open edX Filter Removes PR #361's is_scope_visible/has_visible_scope from rest_api/utils.py and replaces them with AuthorizationDataRequested (openedx_authz/filters.py), a domain-neutral Open edX Filter. PermissionValidationMeView, ScopesAPIView, AssignmentsAPIView, and TeamMemberAssignmentsAPIView call it directly; the actual course-authoring visibility logic lives in the isolated, opt-in CourseAuthoringVisibilityFilter pipeline step, disabled by default. Co-Authored-By: Claude Sonnet 5 --- openedx_authz/filters.py | 63 +++++ openedx_authz/rest_api/utils.py | 74 ----- .../rest_api/v1/admin_console/views.py | 22 ++ .../rest_api/v1/course_authoring/pipeline.py | 100 +++++++ openedx_authz/rest_api/v1/views.py | 7 +- .../rest_api/admin_console/test_views.py | 103 +++++++ .../course_authoring/test_pipeline.py | 183 ++++++++++++ openedx_authz/tests/rest_api/test_utils.py | 262 +----------------- openedx_authz/tests/rest_api/test_views.py | 193 ++----------- requirements/base.in | 1 + requirements/base.txt | 2 + 11 files changed, 501 insertions(+), 509 deletions(-) create mode 100644 openedx_authz/filters.py create mode 100644 openedx_authz/rest_api/v1/course_authoring/pipeline.py create mode 100644 openedx_authz/tests/rest_api/course_authoring/test_pipeline.py diff --git a/openedx_authz/filters.py b/openedx_authz/filters.py new file mode 100644 index 00000000..3f87dab0 --- /dev/null +++ b/openedx_authz/filters.py @@ -0,0 +1,63 @@ +""" +Open edX Filters exposed by openedx_authz's REST API. +""" + +from typing import TypedDict + +from openedx_filters.tooling import OpenEdxPublicFilter + + +class ScopedItem(TypedDict): + """A single item returned by an openedx_authz REST API endpoint. + + Endpoints may include additional keys beyond ``scope`` (e.g. ``role``, ``org``, + ``username``); ``AuthorizationDataRequested`` and the pipeline steps configured for it + only ever inspect ``scope``. + """ + + scope: str | None + + +class ValidationItem(ScopedItem, total=False): + """A ``ScopedItem`` from a permission-validation response, which also carries ``allowed``.""" + + allowed: bool + + +class AuthorizationDataRequested(OpenEdxPublicFilter): + """ + Filter used to modify Authorization data before an openedx_authz REST API endpoint returns it. + + Purpose: + This filter is triggered whenever an openedx_authz REST API endpoint is about to + return a list of items that each carry a ``scope``, just before serialization, + allowing another domain to modify that data. Unconfigured (no pipeline step + registered in ``OPEN_EDX_FILTERS_CONFIG``), every item stays as given; this filter + carries no assumption about why a pipeline step might change an item. + + Filter Type: + org.openedx.authz.authorization_data.requested.v1 + + Trigger: + - Repository: openedx/openedx-authz + - Path: openedx_authz/rest_api/v1/views.py + - Function or Method: PermissionValidationMeView.post + """ + + filter_type = "org.openedx.authz.authorization_data.requested.v1" + + @classmethod + def run_filter(cls, items: list[ScopedItem], username: str) -> list[ScopedItem]: + """Run the pipeline configured for this filter. + + Args: + items (list[ScopedItem]): serialized items about to be returned, each + carrying a ``scope`` key. + username (str): the user the items were computed for, available to any + pipeline step that needs to make a per-user decision. + + Returns: + list[ScopedItem]: the items to actually return, as given or modified. + """ + data = super().run_pipeline(items=items, username=username) + return data.get("items") diff --git a/openedx_authz/rest_api/utils.py b/openedx_authz/rest_api/utils.py index 48a135ee..0403d844 100644 --- a/openedx_authz/rest_api/utils.py +++ b/openedx_authz/rest_api/utils.py @@ -1,11 +1,9 @@ """Utility functions for the Open edX AuthZ REST API.""" -from openedx_authz import api from openedx_authz.api.data import ( GLOBAL_SCOPE_WILDCARD, ScopeData, ) -from openedx_authz.api.users import get_scopes_for_user_and_permission from openedx_authz.rest_api.data import ( AssignmentSortField, BaseEnum, @@ -14,20 +12,6 @@ SortOrder, UserAssignmentSortField, ) -from openedx_authz.utils import is_user_staff_or_superuser - -try: - # common.djangoapps.student.roles and openedx.core are edx-platform's own modules. This app - # is an edx-platform plugin, so they're always available at runtime; the imports are only - # guarded so this module can still load under this repo's own standalone test suite - # (openedx_authz.settings.test, no edx-platform installed). - from common.djangoapps.student.roles import enable_authz_course_authoring - from openedx.core.djangoapps.waffle_utils.models import WaffleFlagOrgOverrideModel - from openedx.core.toggles import AUTHZ_COURSE_AUTHORING_FLAG -except ImportError: - enable_authz_course_authoring = None - WaffleFlagOrgOverrideModel = None - AUTHZ_COURSE_AUTHORING_FLAG = None def get_generic_scope(scope: ScopeData) -> ScopeData: @@ -197,61 +181,3 @@ def sort_user_assignments( list[dict]: The sorted assignments. """ return _sort_by_field(assignments, sort_by, order, UserAssignmentSortField) - - -def is_scope_visible(scope: api.ScopeData) -> bool: - """Return whether a scope is visible under the course-authoring flag. - - See ``docs/decisions/0015-course-authoring-flag-visibility-in-rest-api.rst`` - for the reasoning: Casbin data cannot be trusted as a proxy for - ``authz.enable_course_authoring``'s effective state, since the migration - that is supposed to keep Casbin in sync with the flag is opt-in, off by - default, and never runs for platform-wide flag changes. Only the flag - itself, checked directly, can answer whether a scope is visible. - - - Library and other non-course scopes (e.g. 'lib:DemoX:CSPROB'): always visible. - - Concrete course (e.g. 'course-v1:DemoX+CS101+2024'): full course/org/platform - cascade via ``enable_authz_course_authoring(course_key)``. - - Org-level course glob (e.g. 'course-v1:DemoX+*'): org override, else platform default. - - Platform-level course glob ('course-v1:*'): platform tier only, no course or org. - - Args: - scope (ScopeData): A resolved scope instance. - - Returns: - bool: True if the scope should count as visible. - """ - if scope.NAMESPACE != api.CourseOverviewData.NAMESPACE: - return True - if isinstance(scope, api.CourseOverviewData): - return enable_authz_course_authoring(scope.course_key) - if isinstance(scope, api.OrgCourseOverviewGlobData): - # enable_authz_course_authoring only accepts a course key, and there's no public - # edx-platform API to check an org alone, so this checks the org override directly - # (see issue #360 for follow-up) when asked to check an org-level course glob - org_override = WaffleFlagOrgOverrideModel.override_value(AUTHZ_COURSE_AUTHORING_FLAG.name, scope.org) - if org_override == WaffleFlagOrgOverrideModel.ALL_CHOICES.on: - return True - if org_override == WaffleFlagOrgOverrideModel.ALL_CHOICES.off: - return False - return enable_authz_course_authoring() - - -def has_visible_scope(username: str, action: str, scope_value: str | None) -> bool: - """Return whether the user has a course-authoring-visible scope for this action. - - Args: - username (str): The user checking the action. - action (str): The action being validated. - scope_value (str | None): The external key of the scope being - validated, or None to check across any scope the user has the - action in. - - Returns: - bool: True if the user has a visible scope for this action, False otherwise. - """ - if is_user_staff_or_superuser(username): - return True - if scope_value: - return is_scope_visible(api.ScopeData(external_key=scope_value)) - return any(is_scope_visible(scope) for scope in get_scopes_for_user_and_permission(username, action)) diff --git a/openedx_authz/rest_api/v1/admin_console/views.py b/openedx_authz/rest_api/v1/admin_console/views.py index 0f46eeb6..12c4a7b6 100644 --- a/openedx_authz/rest_api/v1/admin_console/views.py +++ b/openedx_authz/rest_api/v1/admin_console/views.py @@ -39,6 +39,7 @@ get_visible_user_role_assignments_filtered_by_current_user, ) from openedx_authz.constants import permissions +from openedx_authz.filters import AuthorizationDataRequested from openedx_authz.models.scopes import get_content_library_model, get_course_overview_model from openedx_authz.rest_api.data import ScopesQuerySetFields, ScopesTypeField from openedx_authz.rest_api.decorators import authz_permissions, view_auth_classes @@ -502,6 +503,25 @@ def get_permission(scope_cls): # Union the requested querysets and sort by org at the DB level. return self._build_queryset(courses_qs, libraries_qs) + def filter_queryset(self, queryset: QuerySet) -> list[dict]: + """Materialize the queryset and apply AuthorizationDataRequested before pagination. + + Filtering has to happen here, before ``paginate_queryset`` runs, so the page's + ``count`` reflects the actually-visible total, not the pre-filter one. Each row + is given a temporary ``scope`` key, matching the filter's contract, computed the + same way ``ScopeSerializer.get_external_key`` does; the key is removed again + before returning, since ``ScopeSerializer`` derives it itself from the row's + existing fields, it isn't part of this endpoint's queryset shape. + """ + rows = list(queryset) + get_external_key = ScopeSerializer().get_external_key + for row in rows: + row["scope"] = get_external_key(row) + rows = AuthorizationDataRequested.run_filter(items=rows, username=self.request.user.username) + for row in rows: + del row["scope"] + return rows + @view_auth_classes() class TeamMembersAPIView(APIView): @@ -720,6 +740,7 @@ def get(self, request: HttpRequest, username: str) -> Response: ) assignments = TeamMemberAssignmentSerializer(user_role_assignments, many=True).data + assignments = AuthorizationDataRequested.run_filter(items=assignments, username=request.user.username) for backend in self.filter_backends: assignments = backend().filter_queryset(request, assignments, self) @@ -852,6 +873,7 @@ def get(self, request: HttpRequest) -> Response: ] assignments = TeamMemberUserAssignmentSerializer(user_role_assignments, many=True).data + assignments = AuthorizationDataRequested.run_filter(items=assignments, username=request.user.username) for backend in self.filter_backends: assignments = backend().filter_queryset(request, assignments, self) diff --git a/openedx_authz/rest_api/v1/course_authoring/pipeline.py b/openedx_authz/rest_api/v1/course_authoring/pipeline.py new file mode 100644 index 00000000..9ccebebb --- /dev/null +++ b/openedx_authz/rest_api/v1/course_authoring/pipeline.py @@ -0,0 +1,100 @@ +""" +Pipeline step implementing course-authoring visibility for ``AuthorizationDataRequested``. + +This is the isolated, opt-in implementation of the exception ``docs/decisions/0016-rest-api-domain-ownership-boundary.rst`` +and ``docs/decisions/0018-cross-domain-filtering-via-openedx-filters.rst`` document. It's the only +place in openedx_authz that computes course-authoring-flag visibility, and it's never registered +unless a deployment's ``OPEN_EDX_FILTERS_CONFIG`` explicitly wires it in, typically via a Tutor +plugin patch. Deleting this file and the patch that registers it removes the mechanism entirely; +no endpoint code depends on it existing. +""" + +from openedx_filters.filters import PipelineStep + +from openedx_authz import api +from openedx_authz.filters import ScopedItem +from openedx_authz.utils import is_user_staff_or_superuser + +try: + # common.djangoapps.student.roles and openedx.core are edx-platform's own modules. This app + # is an edx-platform plugin, so they're always available at runtime; the imports are only + # guarded so this module can still load under this repo's own standalone test suite + # (openedx_authz.settings.test, no edx-platform installed). + from common.djangoapps.student.roles import enable_authz_course_authoring + from openedx.core.djangoapps.waffle_utils.models import WaffleFlagOrgOverrideModel + from openedx.core.toggles import AUTHZ_COURSE_AUTHORING_FLAG +except ImportError: + enable_authz_course_authoring = None + WaffleFlagOrgOverrideModel = None + AUTHZ_COURSE_AUTHORING_FLAG = None + + +def _is_scope_visible(scope: api.ScopeData) -> bool: + """Return whether a scope is visible under the course-authoring flag. + + - Library and other non-course scopes (e.g. 'lib:DemoX:CSPROB'): always visible. + - Concrete course (e.g. 'course-v1:DemoX+CS101+2024'): full course/org/platform + cascade via ``enable_authz_course_authoring(course_key)``. + - Org-level course glob (e.g. 'course-v1:DemoX+*'): org override, else platform default. + - Platform-level course glob ('course-v1:*'): platform tier only, no course or org. + + Args: + scope (ScopeData): A resolved scope instance. + + Returns: + bool: True if the scope should count as visible. + """ + if scope.NAMESPACE != api.CourseOverviewData.NAMESPACE: + return True + if isinstance(scope, api.CourseOverviewData): + return enable_authz_course_authoring(scope.course_key) + if isinstance(scope, api.OrgCourseOverviewGlobData): + # enable_authz_course_authoring only accepts a course key, and there's no public + # edx-platform API to check an org alone, so this checks the org override directly + # (see issue #360 for follow-up) when asked to check an org-level course glob + org_override = WaffleFlagOrgOverrideModel.override_value(AUTHZ_COURSE_AUTHORING_FLAG.name, scope.org) + if org_override == WaffleFlagOrgOverrideModel.ALL_CHOICES.on: + return True + if org_override == WaffleFlagOrgOverrideModel.ALL_CHOICES.off: + return False + return enable_authz_course_authoring() + + +class CourseAuthoringVisibilityFilter(PipelineStep): + """Applies course-authoring visibility to items from ``AuthorizationDataRequested``. + + Every item carries a ``scope``. What happens when that scope is hidden depends on the + item's own shape, not on which endpoint sent it: + + - ``scope`` is ``None`` (an any-scope check): left untouched. There's no single scope + to check visibility against, and no candidate list is provided. + - The item has an ``allowed`` key: kept, with ``allowed`` set to ``False`` if hidden. + Preserves 1:1 correspondence for endpoints like ``PermissionValidationMeView`` that + must return exactly one result per request. + - Otherwise: dropped entirely if hidden. + + Staff and superusers see everything, regardless of the flag's state. + """ + + def run_filter(self, items: list[ScopedItem], username: str, **kwargs) -> dict: + """Apply course-authoring visibility to each item, per its own shape. + + Args: + items (list[ScopedItem]): serialized items, each carrying a ``scope`` key. + username (str): the user the items were computed for. + + Returns: + dict: ``{"items": ...}``, the items that should remain, marked or dropped. + """ + if is_user_staff_or_superuser(username): + return {"items": items} + + result = [] + for item in items: + scope = item.get("scope") + if scope is None or _is_scope_visible(api.ScopeData(external_key=scope)): + result.append(item) + elif "allowed" in item: + result.append({**item, "allowed": False}) + # else: the scope is hidden and there's no allowed key to flip, drop the item. + return {"items": result} diff --git a/openedx_authz/rest_api/v1/views.py b/openedx_authz/rest_api/v1/views.py index fbfa7474..a990d02b 100644 --- a/openedx_authz/rest_api/v1/views.py +++ b/openedx_authz/rest_api/v1/views.py @@ -20,12 +20,12 @@ from openedx_authz import api from openedx_authz.api.utils import get_user_map from openedx_authz.constants import permissions +from openedx_authz.filters import AuthorizationDataRequested from openedx_authz.rest_api.data import RoleOperationError, RoleOperationStatus from openedx_authz.rest_api.decorators import authz_permissions, view_auth_classes from openedx_authz.rest_api.utils import ( filter_users, get_generic_scope, - has_visible_scope, sort_users, ) from openedx_authz.rest_api.v1.paginators import AuthZAPIViewPagination @@ -137,9 +137,7 @@ def post(self, request: HttpRequest) -> Response: try: action = permission["action"] scope = permission.get("scope") - allowed = has_visible_scope(username, action, scope) and api.is_user_allowed_in_scope( - username, action, scope - ) + allowed = api.is_user_allowed_in_scope(username, action, scope) response_data.append({"action": action, "scope": scope, "allowed": allowed}) except ValueError as e: logger.error(f"Error validating permission for user {username}: {e}") @@ -151,6 +149,7 @@ def post(self, request: HttpRequest) -> Response: status=status.HTTP_500_INTERNAL_SERVER_ERROR, ) + response_data = AuthorizationDataRequested.run_filter(items=response_data, username=username) serializer = PermissionValidationResponseSerializer(response_data, many=True) return Response(serializer.data, status=status.HTTP_200_OK) diff --git a/openedx_authz/tests/rest_api/admin_console/test_views.py b/openedx_authz/tests/rest_api/admin_console/test_views.py index f4d76aab..301e33a0 100644 --- a/openedx_authz/tests/rest_api/admin_console/test_views.py +++ b/openedx_authz/tests/rest_api/admin_console/test_views.py @@ -11,6 +11,7 @@ from ddt import data, ddt, unpack from django.contrib.auth import get_user_model +from django.test import override_settings from django.urls import reverse from organizations.models import Organization from rest_framework import status @@ -173,6 +174,40 @@ def test_response_shape(self): self.assertIn("display_name", item) self.assertIn("org", item) + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.authz.authorization_data.requested.v1": { + "pipeline": [ + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + ], + "fail_silently": False, + }, + }, + ) + def test_hidden_course_scope_is_dropped_with_the_real_pipeline_step(self): + """Test with the real course-authoring pipeline step configured and the flag disabled. + + Expected result: + - Returns 200 OK status + - The hidden course scope is dropped from the results and count + - Library scopes are unaffected + """ + self.client.force_authenticate(user=User.objects.get(username="regular_1")) + + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", + return_value=False, + ): + response = self.client.get(self.url) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + external_keys = {item["external_key"] for item in response.data["results"]} + self.assertNotIn(self.COURSE_ORG1, external_keys) + self.assertNotIn(self.COURSE_ORG2, external_keys) + self.assertIn(self.LIBRARY_ORG1, external_keys) + self.assertIn(self.LIBRARY_ORG2, external_keys) + self.assertEqual(response.data["count"], len(response.data["results"])) + # ------------------------------------------------------------------ # # Sorted by org # # ------------------------------------------------------------------ # @@ -1771,6 +1806,40 @@ def test_no_superadmin_entries_when_filtering_by_role(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertFalse(any(item["is_superadmin"] for item in response.data["results"])) + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.authz.authorization_data.requested.v1": { + "pipeline": [ + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + ], + "fail_silently": False, + }, + }, + ) + def test_hidden_course_assignment_is_dropped_with_the_real_pipeline_step(self): + """Test with the real course-authoring pipeline step configured and the flag disabled. + + Expected result: + - Returns 200 OK status + - The hidden course assignment is dropped from the results + """ + assign_role_to_user_in_scope( + user_external_key="regular_5", + role_external_key=roles.COURSE_STAFF.external_key, + scope_external_key=COURSE_SCOPE_ORG1, + ) + self.client.force_authenticate(user=User.objects.get(username="regular_5")) + + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", + return_value=False, + ): + response = self.client.get(self._url("regular_5")) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + scopes = {item["scope"] for item in response.data["results"]} + self.assertNotIn(COURSE_SCOPE_ORG1, scopes) + @ddt class TestAssignmentsAPIView(ViewTestMixin): @@ -2300,6 +2369,40 @@ def test_inactive_users_excluded_from_results(self): inactive_user.is_active = True inactive_user.save() + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.authz.authorization_data.requested.v1": { + "pipeline": [ + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + ], + "fail_silently": False, + }, + }, + ) + def test_hidden_course_assignment_is_dropped_with_the_real_pipeline_step(self): + """Test with the real course-authoring pipeline step configured and the flag disabled. + + Expected result: + - Returns 200 OK status + - The hidden course assignment is dropped from the results + """ + assign_role_to_user_in_scope( + user_external_key="regular_1", + role_external_key=roles.COURSE_STAFF.external_key, + scope_external_key=COURSE_SCOPE_ORG1, + ) + self.client.force_authenticate(user=User.objects.get(username="regular_1")) + + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", + return_value=False, + ): + response = self.client.get(self.url) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + scopes = {item["scope"] for item in response.data["results"]} + self.assertNotIn(COURSE_SCOPE_ORG1, scopes) + @ddt class TestAssignmentsAPIViewPermissions(ViewTestMixin): diff --git a/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py b/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py new file mode 100644 index 00000000..7c1dfe5a --- /dev/null +++ b/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py @@ -0,0 +1,183 @@ +"""Unit tests for the course-authoring visibility pipeline step. + +The three-tier cascade (course override, else org override, else platform +default) is edx-platform's ``CourseWaffleFlag.is_enabled()``, not importable +in this repo's standalone test suite. ``CourseWaffleFlagMock`` stands in for +it, so the truth table can still be exercised end to end. +""" + +from unittest.mock import MagicMock, patch + +from ddt import data, ddt, unpack +from django.test import TestCase + +from openedx_authz.api.data import ContentLibraryData, CourseOverviewData, OrgCourseOverviewGlobData +from openedx_authz.rest_api.v1.course_authoring.pipeline import CourseAuthoringVisibilityFilter, _is_scope_visible + +COURSE_SCOPE = "course-v1:Org1+COURSE1+2024" +OTHER_COURSE_SCOPE = "course-v1:Org1+COURSE2+2024" +LIB_SCOPE = "lib:Org1:LIB1" +ORG_GLOB_COURSE_SCOPE = OrgCourseOverviewGlobData.build_external_key("Org1") + + +class CourseWaffleFlagMock: + """Stand-in for edx-platform's ``CourseWaffleFlag``, not importable in this repo's standalone suite.""" + + def __init__(self, platform: bool, org_override: bool | None = None, course_override: bool | None = None): + self.platform = platform + self.org_override = org_override + self.course_override = course_override + + def __call__(self, course_key=None) -> bool: + if self.course_override is not None: + return self.course_override + if self.org_override is not None: + return self.org_override + return self.platform + + +@ddt +class TestIsScopeVisible(TestCase): + """Test _is_scope_visible, dispatching to the right override tier depending on the scope's type.""" + + @data( + (False, None, None, False), + (True, None, None, True), + (False, True, None, True), + (True, False, None, False), + (False, None, True, True), + (True, None, False, False), + ) + @unpack + def test_course_scope_follows_the_truth_table( + self, platform: bool, org_override: bool | None, course_override: bool | None, expected: bool + ): + """Test _is_scope_visible for a concrete course scope against override combinations. + + Expected result: + - The scope is visible exactly when course override wins, else org + override, else platform default. + """ + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", + CourseWaffleFlagMock(platform, org_override, course_override), + ): + self.assertEqual(_is_scope_visible(CourseOverviewData(external_key=COURSE_SCOPE)), expected) + + def test_library_scope_is_always_visible_regardless_of_the_flag(self): + """Test _is_scope_visible for a library scope. + + Expected result: + - The scope is always visible, since it isn't course-authoring-gated. + """ + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False + ): + self.assertTrue(_is_scope_visible(ContentLibraryData(external_key=LIB_SCOPE))) + + @data( + ("on", False, True), + ("off", True, False), + ("unset", True, True), + ) + @unpack + def test_org_glob_scope_org_override_takes_precedence( + self, override_choice: str, platform_default: bool, expected: bool + ): + """Test _is_scope_visible for an org-glob scope against org/platform combinations. + + Expected result: + - The scope follows the org override when set, else the platform default. + """ + mock_org_model = MagicMock() + mock_org_model.ALL_CHOICES.on = "on" + mock_org_model.ALL_CHOICES.off = "off" + mock_org_model.override_value.return_value = override_choice + scope = OrgCourseOverviewGlobData(external_key=ORG_GLOB_COURSE_SCOPE) + + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.WaffleFlagOrgOverrideModel", mock_org_model + ), patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.AUTHZ_COURSE_AUTHORING_FLAG", + MagicMock(name="authz.enable_course_authoring"), + ), patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", + return_value=platform_default, + ): + self.assertEqual(_is_scope_visible(scope), expected) + + +class TestCourseAuthoringVisibilityFilter(TestCase): + """Test CourseAuthoringVisibilityFilter, the pipeline step for AuthorizationDataRequested.""" + + def test_drops_items_whose_scope_is_hidden(self): + """Test run_filter with a mix of visible and hidden scopes. + + Expected result: + - Only the item with a visible scope survives. + """ + items = [{"scope": COURSE_SCOPE}, {"scope": LIB_SCOPE}] + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False + ), patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.is_user_staff_or_superuser", return_value=False + ): + result = CourseAuthoringVisibilityFilter( + filter_type="test", running_pipeline=[] + ).run_filter(items=items, username="someuser") + + self.assertEqual(result, {"items": [{"scope": LIB_SCOPE}]}) + + def test_staff_or_superuser_bypasses_visibility(self): + """Test run_filter for a staff/superuser with a hidden course scope. + + Expected result: + - Every item survives, regardless of the flag's state. + """ + items = [{"scope": COURSE_SCOPE}] + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False + ), patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.is_user_staff_or_superuser", return_value=True + ): + result = CourseAuthoringVisibilityFilter( + filter_type="test", running_pipeline=[] + ).run_filter(items=items, username="admin") + + self.assertEqual(result, {"items": items}) + + def test_marks_allowed_false_instead_of_dropping_when_the_item_has_an_allowed_key(self): + """Test run_filter with an item that carries an ``allowed`` key, mirroring PermissionValidationMeView. + + Expected result: + - The item survives, but with ``allowed`` flipped to ``False``. + """ + items = [{"scope": COURSE_SCOPE, "action": "view", "allowed": True}] + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False + ), patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.is_user_staff_or_superuser", return_value=False + ): + result = CourseAuthoringVisibilityFilter( + filter_type="test", running_pipeline=[] + ).run_filter(items=items, username="someuser") + + self.assertEqual(result, {"items": [{"scope": COURSE_SCOPE, "action": "view", "allowed": False}]}) + + def test_leaves_any_scope_items_untouched(self): + """Test run_filter with an item whose scope is None (an any-scope check). + + Expected result: + - The item survives unchanged; there's no single scope to check visibility against. + """ + items = [{"scope": None, "action": "view", "allowed": True}] + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False + ), patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.is_user_staff_or_superuser", return_value=False + ): + result = CourseAuthoringVisibilityFilter( + filter_type="test", running_pipeline=[] + ).run_filter(items=items, username="someuser") + + self.assertEqual(result, {"items": items}) diff --git a/openedx_authz/tests/rest_api/test_utils.py b/openedx_authz/tests/rest_api/test_utils.py index 0bbbe2f6..1678eaec 100644 --- a/openedx_authz/tests/rest_api/test_utils.py +++ b/openedx_authz/tests/rest_api/test_utils.py @@ -1,59 +1,9 @@ -"""Unit tests for openedx_authz.rest_api.utils. +"""Unit tests for openedx_authz.rest_api.utils.""" -The three-tier cascade (course override, else org override, else platform -default) is edx-platform's ``CourseWaffleFlag.is_enabled()``, not -importable in this repo's standalone test suite. ``CourseWaffleFlagMock`` -below stands in for it, so ``test_course_scope_follows_the_adr_0015_truth_table`` -can still exercise every row of the ADR 0015 truth table end to end. - -There is no edx-platform API to check the flag for an org alone (see -issue #360), so ``is_scope_visible`` simulates the org-tier step -``CourseWaffleFlag.is_enabled()`` runs internally for an org-glob scope, -using the same ``WaffleFlagOrgOverrideModel`` building block, mocked here -for the same reason: it isn't importable in this repo's standalone suite. -""" - -from types import SimpleNamespace -from unittest.mock import MagicMock, patch - -from ddt import data, ddt, unpack from django.test import TestCase -from openedx_authz.api.data import ( - ContentLibraryData, - CourseOverviewData, - OrgCourseOverviewGlobData, - PlatformCourseOverviewGlobData, -) from openedx_authz.rest_api.data import AssignmentSortField -from openedx_authz.rest_api.utils import has_visible_scope, is_scope_visible, sort_assignments - -COURSE_SCOPE = "course-v1:Org1+COURSE1+2024" -OTHER_COURSE_SCOPE = "course-v1:Org1+COURSE2+2024" -LIB_SCOPE = "lib:Org1:LIB1" -ORG_GLOB_COURSE_SCOPE = OrgCourseOverviewGlobData.build_external_key("Org1") -PLATFORM_GLOB_COURSE_SCOPE = PlatformCourseOverviewGlobData.build_external_key() -FLAG_NAME = "authz.enable_course_authoring" - -class CourseWaffleFlagMock: - """Stand-in for edx-platform's ``CourseWaffleFlag``, not importable in this repo's standalone suite. - - Callable with an optional course key, matching ``enable_authz_course_authoring``'s - signature, so it can be patched in directly. Replicates the real - cascade: course override, else org override, else platform default. - """ - - def __init__(self, platform: bool, org_override: bool | None = None, course_override: bool | None = None): - self.platform = platform - self.org_override = org_override - self.course_override = course_override - - def __call__(self, course_key=None) -> bool: - if self.course_override is not None: - return self.course_override - if self.org_override is not None: - return self.org_override - return self.platform +from openedx_authz.rest_api.utils import sort_assignments class TestSortAssignments(TestCase): @@ -74,211 +24,3 @@ def test_invalid_sort_order_raises_value_error(self): self.assertIn("invalid_order", str(ctx.exception)) self.assertIn("Invalid order", str(ctx.exception)) - - -@ddt -class TestIsScopeVisible(TestCase): - """Test is_scope_visible, dispatching to the right override tier depending on the scope's type.""" - - def setUp(self): - self.course_scope = CourseOverviewData(external_key=COURSE_SCOPE) - - @data( - # (platform, org_override, course_override, expected) - ADR 0015 truth table, override combinations only. - # Permission isn't this function's concern, so staff/action rows are covered end to end in test_views.py. - (False, None, None, False), - (True, None, None, True), - (False, True, None, True), - (True, True, None, True), - (False, False, None, False), - (True, False, None, False), - (False, None, True, True), - (True, None, True, True), - (False, None, False, False), - (True, None, False, False), - (True, True, False, False), # course override wins even when the org override disagrees. - (False, False, True, True), # course override wins even when the org override disagrees. - ) - @unpack - def test_course_scope_follows_the_adr_0015_truth_table( - self, platform: bool, org_override: bool | None, course_override: bool | None, expected: bool - ): - """Test is_scope_visible for a concrete course scope against every override combination. - - Expected result: - - The scope is visible exactly when the ADR 0015 truth table says so: - course override wins, else org override, else platform default. - """ - with patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", - CourseWaffleFlagMock(platform, org_override, course_override), - ): - self.assertEqual(is_scope_visible(self.course_scope), expected) - - def test_course_flag_off_for_one_course_does_not_affect_a_different_course(self): - """Test is_scope_visible for two different course scopes under the same flag. - - Expected result: - - A course-level override for one course does not leak to another course. - """ - - def flag_side_effect(course_key): - return str(course_key) != COURSE_SCOPE - - with patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", - side_effect=flag_side_effect, - ): - self.assertFalse(is_scope_visible(CourseOverviewData(external_key=COURSE_SCOPE))) - self.assertTrue(is_scope_visible(CourseOverviewData(external_key=OTHER_COURSE_SCOPE))) - - def _mock_org_model(self, override_choice: str): - mock_org_model = MagicMock() - mock_org_model.ALL_CHOICES = SimpleNamespace(on="on", off="off", unset="unset") - mock_org_model.override_value.return_value = override_choice - return mock_org_model - - @data( - ("on", False, True), # org override forces on, even though the platform default is off. - ("off", True, False), # org override forces off, even though the platform default is on. - ("unset", True, True), # no org override, falls back to the platform default. - ("unset", False, False), # no org override, platform default is off too. - ) - @unpack - def test_org_glob_scope_org_override_takes_precedence_over_platform_default( - self, override_choice: str, platform_default: bool, expected: bool - ): - """Test is_scope_visible for an org-glob scope against every org/platform combination. - - Expected result: - - The scope follows the org override when set, else the platform default. - """ - scope = OrgCourseOverviewGlobData(external_key=ORG_GLOB_COURSE_SCOPE) - with patch( - "openedx_authz.rest_api.utils.WaffleFlagOrgOverrideModel", - self._mock_org_model(override_choice), - ), patch( - "openedx_authz.rest_api.utils.AUTHZ_COURSE_AUTHORING_FLAG", SimpleNamespace(name=FLAG_NAME) - ), patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=platform_default - ): - self.assertEqual(is_scope_visible(scope), expected) - - @data(True, False) - def test_platform_glob_scope_follows_the_platform_tier(self, platform_enabled: bool): - """Test is_scope_visible for a platform-glob scope. - - Expected result: - - The scope has no course or org, so it follows the platform tier only. - """ - scope = PlatformCourseOverviewGlobData(external_key=PLATFORM_GLOB_COURSE_SCOPE) - with patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", - return_value=platform_enabled, - ) as mock_enabled: - self.assertEqual(is_scope_visible(scope), platform_enabled) - mock_enabled.assert_called_once_with() - - @data(True, False) - def test_library_scope_is_always_visible_regardless_of_the_flag(self, flag_enabled: bool): - """Test is_scope_visible for a library scope, regardless of the flag's state. - - Expected result: - - The scope is always visible, since it isn't course-authoring-gated. - """ - with patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", - return_value=flag_enabled, - ): - self.assertTrue(is_scope_visible(ContentLibraryData(external_key=LIB_SCOPE))) - - -@ddt -class TestHasVisibleScope(TestCase): - """Test has_visible_scope, which resolves scope_value (course/library/org-glob/None) and dispatches. - - Staff/superusers bypass flag visibility entirely, the same way they - bypass the Casbin permission check (is_user_allowed_in_scope). - """ - - ACTION = "courses.view_course" - USERNAME = "someuser" - - @data(True, False) - def test_course_scope_follows_the_flag(self, flag_enabled: bool): - """Test has_visible_scope with a given course scope. - - Expected result: - - The result matches is_scope_visible's course-tier result for that scope. - """ - with patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=flag_enabled - ): - self.assertEqual(has_visible_scope(self.USERNAME, self.ACTION, COURSE_SCOPE), flag_enabled) - - @data(True, False) - def test_library_scope_is_always_visible(self, flag_enabled: bool): - """Test has_visible_scope with a given library scope, regardless of the flag's state. - - Expected result: - - The scope is always visible. - """ - with patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=flag_enabled - ): - self.assertTrue(has_visible_scope(self.USERNAME, self.ACTION, LIB_SCOPE)) - - def test_any_scope_check_is_allowed_when_a_granted_scope_is_visible(self): - """Test has_visible_scope with no scope given, and a mix of granted scopes. - - Expected result: - - Visible if at least one of the user's granted scopes is visible. - """ - with patch( - "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", - return_value=[ - CourseOverviewData(external_key=COURSE_SCOPE), - ContentLibraryData(external_key=LIB_SCOPE), - ], - ), patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False): - # The course scope is flag-disabled, but the library scope always counts, so overall visible. - self.assertTrue(has_visible_scope(self.USERNAME, self.ACTION, None)) - - def test_any_scope_check_is_denied_when_no_granted_scope_is_visible(self): - """Test has_visible_scope with no scope given, and only flag-disabled granted scopes. - - Expected result: - - Not visible, since none of the user's granted scopes are visible. - """ - with patch( - "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", - return_value=[CourseOverviewData(external_key=COURSE_SCOPE)], - ), patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False): - self.assertFalse(has_visible_scope(self.USERNAME, self.ACTION, None)) - - def test_any_scope_check_is_denied_when_user_has_no_granted_scopes(self): - """Test has_visible_scope for a non-staff user with no scope given, and no granted scopes at all. - - Expected result: - - Not visible. - """ - with patch( - "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", return_value=[] - ): - self.assertFalse(has_visible_scope(self.USERNAME, self.ACTION, None)) - - @data(COURSE_SCOPE, None) - def test_staff_or_superuser_bypasses_flag_visibility(self, scope_value: str | None): - """Test has_visible_scope for a staff/superuser, with a flag-disabled course scope, or no scope at all. - - Expected result: - - Visible, regardless of the flag's state or the user's granted scopes. - """ - with patch( - "openedx_authz.rest_api.utils.is_user_staff_or_superuser", return_value=True - ), patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False - ), patch( - "openedx_authz.rest_api.utils.get_scopes_for_user_and_permission", return_value=[] - ): - self.assertTrue(has_visible_scope(self.USERNAME, self.ACTION, scope_value)) diff --git a/openedx_authz/tests/rest_api/test_views.py b/openedx_authz/tests/rest_api/test_views.py index 4291641f..2ff927c1 100644 --- a/openedx_authz/tests/rest_api/test_views.py +++ b/openedx_authz/tests/rest_api/test_views.py @@ -5,12 +5,12 @@ including permission validation, user-role management, and role listing capabilities. """ -from types import SimpleNamespace -from unittest.mock import MagicMock, patch +from unittest.mock import patch from urllib.parse import urlencode from ddt import data, ddt, unpack from django.contrib.auth import get_user_model +from django.test import override_settings from django.urls import reverse from rest_framework import status from rest_framework.test import APIClient @@ -29,7 +29,6 @@ from openedx_authz.rest_api.v1.permissions import AnyScopePermission, DynamicScopePermission from openedx_authz.rest_api.v1.views import UserValidationAPIView from openedx_authz.tests.api.test_roles import BaseRolesTestCase -from openedx_authz.tests.rest_api.test_utils import CourseWaffleFlagMock User = get_user_model() @@ -388,187 +387,38 @@ def test_permission_validation_exception_handling(self, exception: Exception, st self.assertEqual(response.status_code, status_code) self.assertEqual(response.data, {"message": message}) - -@ddt -class TestPermissionValidationMeViewCourseAuthoringFlag(ViewTestMixin): - """Test PermissionValidationMeView's course-authoring flag awareness (ADR 0015).""" - - def setUp(self): - """Set up test fixtures and assign a course role to the regular user.""" - super().setUp() - self.url = reverse("openedx_authz:permission-validation-me") - self.client.force_authenticate(user=self.regular_user) - assign_role_to_user_in_scope( - user_external_key=self.regular_user.username, - role_external_key=roles.COURSE_STAFF.external_key, - scope_external_key=COURSE_SCOPE_ORG1, - ) - - @data( - # (platform, org_override, course_override, expected_allowed) - ADR 0015 truth table, permission=True rows - # (this user always has the course_staff permission assigned, so we only test the "Yes" half of the truth table) - (False, None, None, False), - (True, None, None, True), - (False, True, None, True), - (True, True, None, True), - (False, False, None, False), - (True, False, None, False), - (False, None, True, True), - (True, None, True, True), - (False, None, False, False), - (True, None, False, False), + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.authz.authorization_data.requested.v1": { + "pipeline": [ + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + ], + "fail_silently": False, + }, + }, ) - @unpack - def test_course_scope_flag_table( - self, platform: bool, org_override: bool | None, course_override: bool | None, expected_allowed: bool - ): - """Test PermissionValidationMeView with a course scope, through the real endpoint. - - Expected result: - - Allowed exactly when the ADR 0015 truth table says so for the "Yes" - (user has permission) half: course override wins, else org - override, else platform default. - """ - request_data = [{"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": COURSE_SCOPE_ORG1}] - - with patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", - CourseWaffleFlagMock(platform, org_override, course_override), - ): - response = self.client.post(self.url, data=request_data, format="json") - - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data[0]["allowed"], expected_allowed) - - @patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False) - def test_library_scope_unaffected_by_disabled_flag(self, _mock_flag): - """Test PermissionValidationMeView with a library scope, while the course-authoring flag is off. - - Expected result: - - Allowed. A library permission isn't gated by the course-authoring flag. - """ - assign_role_to_user_in_scope( - user_external_key=self.regular_user.username, - role_external_key=roles.LIBRARY_ADMIN.external_key, - scope_external_key=LIB_SCOPE_ORG1, - ) - request_data = [{"action": permissions.VIEW_LIBRARY.identifier, "scope": LIB_SCOPE_ORG1}] - - response = self.client.post(self.url, data=request_data, format="json") - - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertTrue(response.data[0]["allowed"]) - - @patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False) - def test_any_scope_check_excludes_disabled_course(self, _mock_flag): - """Test PermissionValidationMeView for a course-only permission, with no scope given, flag off. + def test_permission_validation_marks_hidden_course_scope_as_disallowed(self): + """Test PermissionValidationMeView with the real course-authoring pipeline step configured. Expected result: - - Denied. The user's only qualifying course is flag-disabled, so no - visible scope grants the permission. - """ - request_data = [{"action": permissions.COURSES_VIEW_COURSE.identifier}] - - response = self.client.post(self.url, data=request_data, format="json") - - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertFalse(response.data[0]["allowed"]) - - @patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=True) - def test_any_scope_check_includes_enabled_course(self, _mock_flag): - """Test PermissionValidationMeView for a course-only permission, with no scope given, flag on. - - Expected result: - - Allowed. The course's flag is on, so the visible scope grants the permission. - """ - request_data = [{"action": permissions.COURSES_VIEW_COURSE.identifier}] - - response = self.client.post(self.url, data=request_data, format="json") - - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertTrue(response.data[0]["allowed"]) - - @data(True, False) - def test_org_scope_validation(self, flag_enabled: bool): - """Test PermissionValidationMeView with an org-level (glob) scope and no org override. - - Expected result: - - Allowed exactly when the platform tier is on, since the visibility - check falls back to the platform default with no org override set. - """ - assign_role_to_user_in_scope( - user_external_key=self.regular_user.username, - role_external_key=roles.COURSE_ADMIN.external_key, - scope_external_key=COURSE_ORG1_GLOB, - ) - request_data = [{"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": COURSE_ORG1_GLOB}] - - mock_org_model = MagicMock() - mock_org_model.ALL_CHOICES = SimpleNamespace(on="on", off="off", unset="unset") - mock_org_model.override_value.return_value = "unset" - - with patch( - "openedx_authz.rest_api.utils.WaffleFlagOrgOverrideModel", mock_org_model - ), patch( - "openedx_authz.rest_api.utils.AUTHZ_COURSE_AUTHORING_FLAG", - SimpleNamespace(name="authz.enable_course_authoring"), - ), patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=flag_enabled - ): - response = self.client.post(self.url, data=request_data, format="json") - - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data[0]["allowed"], flag_enabled) - - @patch("openedx_authz.rest_api.utils.enable_authz_course_authoring", return_value=False) - def test_course_and_library_scope_validated_independently_in_one_request(self, _mock_flag): - """Test PermissionValidationMeView with a flag-disabled course item and a library item, same request. - - Expected result: - - The course item is denied and the library item is allowed; each - item is validated independently, so one does not affect the other. - """ - assign_role_to_user_in_scope( - user_external_key=self.regular_user.username, - role_external_key=roles.LIBRARY_ADMIN.external_key, - scope_external_key=LIB_SCOPE_ORG1, - ) - request_data = [ - {"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": COURSE_SCOPE_ORG1}, - {"action": permissions.VIEW_LIBRARY.identifier, "scope": LIB_SCOPE_ORG1}, - ] - - response = self.client.post(self.url, data=request_data, format="json") - - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertFalse(response.data[0]["allowed"]) - self.assertTrue(response.data[1]["allowed"]) - - def test_course_flag_off_for_one_course_does_not_affect_another_course_in_the_same_request(self): - """Test PermissionValidationMeView with two courses in one request, flag off for only one of them. - - Expected result: - - The flag-disabled course is denied and the other course is allowed; - a course-level override for one course does not leak into a - sibling course in the same batch. + - Returns 200 OK status + - The course item is marked allowed=False, since the flag is disabled for its scope + - The library item is unaffected, since course-authoring visibility never gates it """ - other_course_scope = "course-v1:Org1+COURSE2+2024" + self.client.force_authenticate(user=self.regular_user) assign_role_to_user_in_scope( user_external_key=self.regular_user.username, role_external_key=roles.COURSE_STAFF.external_key, - scope_external_key=other_course_scope, + scope_external_key=COURSE_SCOPE_ORG1, ) request_data = [ {"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": COURSE_SCOPE_ORG1}, - {"action": permissions.COURSES_VIEW_COURSE.identifier, "scope": other_course_scope}, + {"action": permissions.VIEW_LIBRARY.identifier, "scope": LIB_SCOPE_ORG1}, ] - def flag_side_effect(course_key): - return str(course_key) != COURSE_SCOPE_ORG1 - with patch( - "openedx_authz.rest_api.utils.enable_authz_course_authoring", - side_effect=flag_side_effect, + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", + return_value=False, ): response = self.client.post(self.url, data=request_data, format="json") @@ -577,6 +427,7 @@ def flag_side_effect(course_key): self.assertTrue(response.data[1]["allowed"]) + @ddt class TestRoleUserAPIView(ViewTestMixin): """Test suite for RoleUserAPIView.""" diff --git a/requirements/base.in b/requirements/base.in index 87a5de8a..24baac7d 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -15,4 +15,5 @@ edx-drf-extensions # Extensions for Django Rest Framework used by Open edx-organizations # Organizations library for Open edX django-waffle # Waffle library for feature flag management openedx-events # Open edX Events library for emitting events related to role assignment changes +openedx-filters # Open edX Filters library, used to let another domain filter REST API list results django-crum # Current Request User Middleware for Django diff --git a/requirements/base.txt b/requirements/base.txt index 425f8e2e..a6d0e189 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -104,6 +104,8 @@ openedx-atlas==0.7.0 # via -r requirements/base.in openedx-events==11.2.0 # via -r requirements/base.in +openedx-filters==3.8.0 + # via -r requirements/base.in packaging==26.2 # via drf-yasg pillow==12.3.0 From bfa92170b05c3922101f669aea8c1296b0c33382 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 7 Sep 2026 14:49:02 +0200 Subject: [PATCH 5/9] fix: filter role writes through authorization pipeline --- openedx_authz/api/users.py | 28 ----- openedx_authz/filters.py | 41 ++++--- .../rest_api/v1/admin_console/views.py | 22 ---- .../rest_api/v1/course_authoring/pipeline.py | 69 ++++++++++-- openedx_authz/rest_api/v1/views.py | 20 ++-- openedx_authz/tests/api/test_users.py | 68 ------------ .../rest_api/admin_console/test_views.py | 103 ------------------ .../course_authoring/test_pipeline.py | 41 ++----- openedx_authz/tests/rest_api/test_views.py | 99 +++++++++++++++-- 9 files changed, 193 insertions(+), 298 deletions(-) diff --git a/openedx_authz/api/users.py b/openedx_authz/api/users.py index 1786649a..0d492ab4 100644 --- a/openedx_authz/api/users.py +++ b/openedx_authz/api/users.py @@ -68,7 +68,6 @@ "unassign_all_roles_from_user", "validate_users", "get_superadmin_assignments", - "is_user_allowed_in_scope", ] @@ -437,33 +436,6 @@ def is_user_allowed_in_any_scope( return bool(get_scopes_for_user_and_permission(user_external_key, action_external_key)) -def is_user_allowed_in_scope( - user_external_key: str, - action_external_key: str, - scope_external_key: str = None, -) -> bool: - """Check if a user has a specific permission in a given scope or in any scope if none is provided. - - Staff and superusers are always allowed, since they implicitly have every - permission across all scopes. - - Args: - user_external_key (str): ID of the user (e.g., 'john_doe'). - action_external_key (str): The action to check (e.g., 'view_course'). - scope_external_key (str, optional): The scope in which to check the permission. - If None, checks if the user has the permission in any scope. - - Returns: - bool: True if the user is staff/superuser or has the specified permission - in the given scope (or any scope if none is provided), False otherwise. - """ - if is_user_staff_or_superuser(user_external_key): - return True - if scope_external_key: - return is_user_allowed(user_external_key, action_external_key, scope_external_key) - return is_user_allowed_in_any_scope(user_external_key, action_external_key) - - def get_users_for_role_in_scope(role_external_key: str, scope_external_key: str) -> list[UserData]: """Get all the users assigned to a specific role in a specific scope. diff --git a/openedx_authz/filters.py b/openedx_authz/filters.py index 3f87dab0..092905e8 100644 --- a/openedx_authz/filters.py +++ b/openedx_authz/filters.py @@ -2,17 +2,17 @@ Open edX Filters exposed by openedx_authz's REST API. """ -from typing import TypedDict +from typing import Any, TypedDict +from django.contrib.auth.models import AbstractBaseUser from openedx_filters.tooling import OpenEdxPublicFilter class ScopedItem(TypedDict): - """A single item returned by an openedx_authz REST API endpoint. + """A scope-bearing item handled by an openedx_authz REST API endpoint. Endpoints may include additional keys beyond ``scope`` (e.g. ``role``, ``org``, - ``username``); ``AuthorizationDataRequested`` and the pipeline steps configured for it - only ever inspect ``scope``. + or ``username``). """ scope: str | None @@ -24,14 +24,17 @@ class ValidationItem(ScopedItem, total=False): allowed: bool +AuthorizationData = list[ScopedItem] | dict[str, Any] + + class AuthorizationDataRequested(OpenEdxPublicFilter): """ - Filter used to modify Authorization data before an openedx_authz REST API endpoint returns it. + Filter used to modify scope-bearing Authorization data handled by a REST API endpoint. Purpose: - This filter is triggered whenever an openedx_authz REST API endpoint is about to - return a list of items that each carry a ``scope``, just before serialization, - allowing another domain to modify that data. Unconfigured (no pipeline step + This filter is triggered when an openedx_authz REST API endpoint needs another + domain to modify a list of items that each carry a ``scope``. The items may be + response data or scopes about to be used by an operation. Unconfigured (no pipeline step registered in ``OPEN_EDX_FILTERS_CONFIG``), every item stays as given; this filter carries no assumption about why a pipeline step might change an item. @@ -41,23 +44,27 @@ class AuthorizationDataRequested(OpenEdxPublicFilter): Trigger: - Repository: openedx/openedx-authz - Path: openedx_authz/rest_api/v1/views.py - - Function or Method: PermissionValidationMeView.post + - Function or Method: PermissionValidationMeView.post, RoleUserAPIView.put, + RoleUserAPIView.delete """ filter_type = "org.openedx.authz.authorization_data.requested.v1" @classmethod - def run_filter(cls, items: list[ScopedItem], username: str) -> list[ScopedItem]: + def run_filter( + cls, items: AuthorizationData, user: AbstractBaseUser + ) -> tuple[AuthorizationData, list[dict[str, Any]]]: """Run the pipeline configured for this filter. Args: - items (list[ScopedItem]): serialized items about to be returned, each - carrying a ``scope`` key. - username (str): the user the items were computed for, available to any - pipeline step that needs to make a per-user decision. + items (AuthorizationData): scope-bearing response items or validated + role-operation data. + user (AbstractBaseUser): the authenticated user requesting the data, + available to any pipeline step that needs to make a per-user decision. Returns: - list[ScopedItem]: the items to actually return, as given or modified. + tuple[AuthorizationData, list[dict]]: modified data and errors supplied + by the configured pipeline. """ - data = super().run_pipeline(items=items, username=username) - return data.get("items") + data = super().run_pipeline(items=items, user=user) + return data["items"], data.get("errors", []) diff --git a/openedx_authz/rest_api/v1/admin_console/views.py b/openedx_authz/rest_api/v1/admin_console/views.py index 12c4a7b6..0f46eeb6 100644 --- a/openedx_authz/rest_api/v1/admin_console/views.py +++ b/openedx_authz/rest_api/v1/admin_console/views.py @@ -39,7 +39,6 @@ get_visible_user_role_assignments_filtered_by_current_user, ) from openedx_authz.constants import permissions -from openedx_authz.filters import AuthorizationDataRequested from openedx_authz.models.scopes import get_content_library_model, get_course_overview_model from openedx_authz.rest_api.data import ScopesQuerySetFields, ScopesTypeField from openedx_authz.rest_api.decorators import authz_permissions, view_auth_classes @@ -503,25 +502,6 @@ def get_permission(scope_cls): # Union the requested querysets and sort by org at the DB level. return self._build_queryset(courses_qs, libraries_qs) - def filter_queryset(self, queryset: QuerySet) -> list[dict]: - """Materialize the queryset and apply AuthorizationDataRequested before pagination. - - Filtering has to happen here, before ``paginate_queryset`` runs, so the page's - ``count`` reflects the actually-visible total, not the pre-filter one. Each row - is given a temporary ``scope`` key, matching the filter's contract, computed the - same way ``ScopeSerializer.get_external_key`` does; the key is removed again - before returning, since ``ScopeSerializer`` derives it itself from the row's - existing fields, it isn't part of this endpoint's queryset shape. - """ - rows = list(queryset) - get_external_key = ScopeSerializer().get_external_key - for row in rows: - row["scope"] = get_external_key(row) - rows = AuthorizationDataRequested.run_filter(items=rows, username=self.request.user.username) - for row in rows: - del row["scope"] - return rows - @view_auth_classes() class TeamMembersAPIView(APIView): @@ -740,7 +720,6 @@ def get(self, request: HttpRequest, username: str) -> Response: ) assignments = TeamMemberAssignmentSerializer(user_role_assignments, many=True).data - assignments = AuthorizationDataRequested.run_filter(items=assignments, username=request.user.username) for backend in self.filter_backends: assignments = backend().filter_queryset(request, assignments, self) @@ -873,7 +852,6 @@ def get(self, request: HttpRequest) -> Response: ] assignments = TeamMemberUserAssignmentSerializer(user_role_assignments, many=True).data - assignments = AuthorizationDataRequested.run_filter(items=assignments, username=request.user.username) for backend in self.filter_backends: assignments = backend().filter_queryset(request, assignments, self) diff --git a/openedx_authz/rest_api/v1/course_authoring/pipeline.py b/openedx_authz/rest_api/v1/course_authoring/pipeline.py index 9ccebebb..c5d9a9c3 100644 --- a/openedx_authz/rest_api/v1/course_authoring/pipeline.py +++ b/openedx_authz/rest_api/v1/course_authoring/pipeline.py @@ -9,11 +9,13 @@ no endpoint code depends on it existing. """ +from django.contrib.auth.models import AbstractBaseUser from openedx_filters.filters import PipelineStep from openedx_authz import api -from openedx_authz.filters import ScopedItem -from openedx_authz.utils import is_user_staff_or_superuser +from openedx_authz.filters import AuthorizationData, ScopedItem + +SCOPE_NOT_AVAILABLE_ERROR = "scope_not_available" try: # common.djangoapps.student.roles and openedx.core are edx-platform's own modules. This app @@ -71,23 +73,30 @@ class CourseAuthoringVisibilityFilter(PipelineStep): - The item has an ``allowed`` key: kept, with ``allowed`` set to ``False`` if hidden. Preserves 1:1 correspondence for endpoints like ``PermissionValidationMeView`` that must return exactly one result per request. - - Otherwise: dropped entirely if hidden. - Staff and superusers see everything, regardless of the flag's state. """ - def run_filter(self, items: list[ScopedItem], username: str, **kwargs) -> dict: + def run_filter( + self, + items: AuthorizationData, + user: AbstractBaseUser, + **kwargs, + ) -> dict: """Apply course-authoring visibility to each item, per its own shape. Args: - items (list[ScopedItem]): serialized items, each carrying a ``scope`` key. - username (str): the user the items were computed for. + items (AuthorizationData): scope-bearing response items or validated + role-operation data. + user: the authenticated Django user requesting the data. Returns: - dict: ``{"items": ...}``, the items that should remain, marked or dropped. + dict: the modified ``items`` and unchanged requesting ``user``. """ - if is_user_staff_or_superuser(username): - return {"items": items} + if user.is_staff or user.is_superuser: + return {"items": items, "user": user} + + if isinstance(items, dict): + return {**self._filter_role_operations(items), "user": user} result = [] for item in items: @@ -96,5 +105,41 @@ def run_filter(self, items: list[ScopedItem], username: str, **kwargs) -> dict: result.append(item) elif "allowed" in item: result.append({**item, "allowed": False}) - # else: the scope is hidden and there's no allowed key to flip, drop the item. - return {"items": result} + else: + result.append(item) + return {"items": result, "user": user} + + @staticmethod + def _filter_role_operations(items: dict) -> dict: + """Remove unavailable role operations and return their response errors.""" + result = {**items} + errors = [] + users = items["users"] + + if "scopes" in items: + available_scopes = [] + for scope in items["scopes"]: + if _is_scope_visible(api.ScopeData(external_key=scope)): + available_scopes.append(scope) + else: + errors.extend( + { + "user_identifier": user_identifier, + "scope": scope, + "error": SCOPE_NOT_AVAILABLE_ERROR, + } + for user_identifier in users + ) + result["scopes"] = available_scopes + elif not _is_scope_visible(api.ScopeData(external_key=items["scope"])): + errors.extend( + { + "user_identifier": user_identifier, + "scope": items["scope"], + "error": SCOPE_NOT_AVAILABLE_ERROR, + } + for user_identifier in users + ) + result["users"] = [] + + return {"items": result, "errors": errors} diff --git a/openedx_authz/rest_api/v1/views.py b/openedx_authz/rest_api/v1/views.py index a990d02b..cca32960 100644 --- a/openedx_authz/rest_api/v1/views.py +++ b/openedx_authz/rest_api/v1/views.py @@ -111,8 +111,8 @@ class PermissionValidationMeView(APIView): **Example Response (without scope)**:: [ - {"action": "content_libraries.manage_library_team", "allowed": true, "scope": null}, - {"action": "courses.manage_course_team", "allowed": false, "scope": null} + {"action": "content_libraries.manage_library_team", "allowed": true}, + {"action": "courses.manage_course_team", "allowed": false} ] """ @@ -137,8 +137,12 @@ def post(self, request: HttpRequest) -> Response: try: action = permission["action"] scope = permission.get("scope") - allowed = api.is_user_allowed_in_scope(username, action, scope) - response_data.append({"action": action, "scope": scope, "allowed": allowed}) + if scope: + allowed = api.is_user_allowed(username, action, scope) + response_data.append({"action": action, "scope": scope, "allowed": allowed}) + else: + allowed = api.is_user_allowed_in_any_scope(username, action) + response_data.append({"action": action, "allowed": allowed}) except ValueError as e: logger.error(f"Error validating permission for user {username}: {e}") return Response(data={"message": "Invalid scope format"}, status=status.HTTP_400_BAD_REQUEST) @@ -149,7 +153,7 @@ def post(self, request: HttpRequest) -> Response: status=status.HTTP_500_INTERNAL_SERVER_ERROR, ) - response_data = AuthorizationDataRequested.run_filter(items=response_data, username=username) + response_data, _ = AuthorizationDataRequested.run_filter(items=response_data, user=request.user) serializer = PermissionValidationResponseSerializer(response_data, many=True) return Response(serializer.data, status=status.HTTP_200_OK) @@ -311,7 +315,8 @@ def put(self, request: HttpRequest) -> Response: serializer.is_valid(raise_exception=True) data = serializer.validated_data - completed, errors = [], [] + data, errors = AuthorizationDataRequested.run_filter(items=data, user=request.user) + completed = [] for scope_value in data["scopes"]: for user_identifier in data["users"]: response_dict = {"user_identifier": user_identifier, "scope": scope_value} @@ -358,7 +363,8 @@ def delete(self, request: HttpRequest) -> Response: serializer.is_valid(raise_exception=True) data = serializer.validated_data - completed, errors = [], [] + data, errors = AuthorizationDataRequested.run_filter(items=data, user=request.user) + completed = [] for user_identifier in data["users"]: response_dict = {"user_identifier": user_identifier} try: diff --git a/openedx_authz/tests/api/test_users.py b/openedx_authz/tests/api/test_users.py index 50d4f89e..278a0063 100644 --- a/openedx_authz/tests/api/test_users.py +++ b/openedx_authz/tests/api/test_users.py @@ -32,7 +32,6 @@ get_visible_user_role_assignments_filtered_by_current_user, is_user_allowed, is_user_allowed_in_any_scope, - is_user_allowed_in_scope, unassign_all_roles_from_user, unassign_role_from_user, validate_users, @@ -706,73 +705,6 @@ def test_is_user_allowed_in_any_scope_staff_always_allowed(self, username, flags ) self.assertTrue(result) - @data( - # With a scope given, behaves like is_user_allowed. - ("alice", permissions.DELETE_LIBRARY.identifier, "lib:Org1:math_101", True), - ("charlie", permissions.DELETE_LIBRARY.identifier, "lib:Org1:science_301", False), - ("daniel", permissions.COURSES_MANAGE_ADVANCED_SETTINGS.identifier, "course-v1:TestOrg+TestCourse+2024_T1", True), - ("judy", permissions.COURSES_MANAGE_ADVANCED_SETTINGS.identifier, "course-v1:TestOrg+TestCourse+2024_T1", False), - ) - @unpack - def test_is_user_allowed_in_scope_with_scope_given(self, username, action, scope_name, expected_result): - """Test checking if a user has a specific permission in a given scope, via is_user_allowed_in_scope. - - Expected result: - - The function correctly identifies whether the user has the specified permission in the scope. - """ - result = is_user_allowed_in_scope( - user_external_key=username, - action_external_key=action, - scope_external_key=scope_name, - ) - self.assertEqual(result, expected_result) - - @data( - # With no scope given, behaves like is_user_allowed_in_any_scope. - ("alice", permissions.DELETE_LIBRARY.identifier, True), - ("jane", permissions.DELETE_LIBRARY.identifier, False), - ("carlos", permissions.COURSES_MANAGE_ADVANCED_SETTINGS.identifier, True), - ("nonexistent_user", permissions.MANAGE_LIBRARY_TEAM.identifier, False), - ) - @unpack - def test_is_user_allowed_in_scope_without_scope_given(self, username, action, expected_result): - """Test checking if a user holds a permission in at least one scope, via is_user_allowed_in_scope. - - Expected result: - - The function returns True when the user has the permission in any scope, - and False when the user has it in no scope. - """ - result = is_user_allowed_in_scope( - user_external_key=username, - action_external_key=action, - ) - self.assertEqual(result, expected_result) - - @data( - # Staff/superuser bypass applies regardless of whether a scope is given, or which scope. - ("lib:Org1:math_101", True), - ("course-v1:TestOrg+TestCourse+2024_T1", True), - ("global:AnyScope1", True), - (None, True), - ) - @unpack - def test_is_user_allowed_in_scope_staff_always_allowed(self, scope_name, expected_result): - """Test is_user_allowed_in_scope for a staff user with no explicit assignment. - - Expected result: - - The function returns True for a staff user with no explicit assignment, - for any scope value, including no scope at all. - """ - User = get_user_model() - User.objects.create_user(username="staff_member", email="staff_member@example.com", is_staff=True) - - result = is_user_allowed_in_scope( - user_external_key="staff_member", - action_external_key=permissions.MANAGE_LIBRARY_TEAM.identifier, - scope_external_key=scope_name, - ) - self.assertEqual(result, expected_result) - @ddt class TestValidateUsersAPI(UserAssignmentsSetupMixin): diff --git a/openedx_authz/tests/rest_api/admin_console/test_views.py b/openedx_authz/tests/rest_api/admin_console/test_views.py index 301e33a0..f4d76aab 100644 --- a/openedx_authz/tests/rest_api/admin_console/test_views.py +++ b/openedx_authz/tests/rest_api/admin_console/test_views.py @@ -11,7 +11,6 @@ from ddt import data, ddt, unpack from django.contrib.auth import get_user_model -from django.test import override_settings from django.urls import reverse from organizations.models import Organization from rest_framework import status @@ -174,40 +173,6 @@ def test_response_shape(self): self.assertIn("display_name", item) self.assertIn("org", item) - @override_settings( - OPEN_EDX_FILTERS_CONFIG={ - "org.openedx.authz.authorization_data.requested.v1": { - "pipeline": [ - "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", - ], - "fail_silently": False, - }, - }, - ) - def test_hidden_course_scope_is_dropped_with_the_real_pipeline_step(self): - """Test with the real course-authoring pipeline step configured and the flag disabled. - - Expected result: - - Returns 200 OK status - - The hidden course scope is dropped from the results and count - - Library scopes are unaffected - """ - self.client.force_authenticate(user=User.objects.get(username="regular_1")) - - with patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", - return_value=False, - ): - response = self.client.get(self.url) - - self.assertEqual(response.status_code, status.HTTP_200_OK) - external_keys = {item["external_key"] for item in response.data["results"]} - self.assertNotIn(self.COURSE_ORG1, external_keys) - self.assertNotIn(self.COURSE_ORG2, external_keys) - self.assertIn(self.LIBRARY_ORG1, external_keys) - self.assertIn(self.LIBRARY_ORG2, external_keys) - self.assertEqual(response.data["count"], len(response.data["results"])) - # ------------------------------------------------------------------ # # Sorted by org # # ------------------------------------------------------------------ # @@ -1806,40 +1771,6 @@ def test_no_superadmin_entries_when_filtering_by_role(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertFalse(any(item["is_superadmin"] for item in response.data["results"])) - @override_settings( - OPEN_EDX_FILTERS_CONFIG={ - "org.openedx.authz.authorization_data.requested.v1": { - "pipeline": [ - "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", - ], - "fail_silently": False, - }, - }, - ) - def test_hidden_course_assignment_is_dropped_with_the_real_pipeline_step(self): - """Test with the real course-authoring pipeline step configured and the flag disabled. - - Expected result: - - Returns 200 OK status - - The hidden course assignment is dropped from the results - """ - assign_role_to_user_in_scope( - user_external_key="regular_5", - role_external_key=roles.COURSE_STAFF.external_key, - scope_external_key=COURSE_SCOPE_ORG1, - ) - self.client.force_authenticate(user=User.objects.get(username="regular_5")) - - with patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", - return_value=False, - ): - response = self.client.get(self._url("regular_5")) - - self.assertEqual(response.status_code, status.HTTP_200_OK) - scopes = {item["scope"] for item in response.data["results"]} - self.assertNotIn(COURSE_SCOPE_ORG1, scopes) - @ddt class TestAssignmentsAPIView(ViewTestMixin): @@ -2369,40 +2300,6 @@ def test_inactive_users_excluded_from_results(self): inactive_user.is_active = True inactive_user.save() - @override_settings( - OPEN_EDX_FILTERS_CONFIG={ - "org.openedx.authz.authorization_data.requested.v1": { - "pipeline": [ - "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", - ], - "fail_silently": False, - }, - }, - ) - def test_hidden_course_assignment_is_dropped_with_the_real_pipeline_step(self): - """Test with the real course-authoring pipeline step configured and the flag disabled. - - Expected result: - - Returns 200 OK status - - The hidden course assignment is dropped from the results - """ - assign_role_to_user_in_scope( - user_external_key="regular_1", - role_external_key=roles.COURSE_STAFF.external_key, - scope_external_key=COURSE_SCOPE_ORG1, - ) - self.client.force_authenticate(user=User.objects.get(username="regular_1")) - - with patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", - return_value=False, - ): - response = self.client.get(self.url) - - self.assertEqual(response.status_code, status.HTTP_200_OK) - scopes = {item["scope"] for item in response.data["results"]} - self.assertNotIn(COURSE_SCOPE_ORG1, scopes) - @ddt class TestAssignmentsAPIViewPermissions(ViewTestMixin): diff --git a/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py b/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py index 7c1dfe5a..08e25bab 100644 --- a/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py +++ b/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py @@ -6,6 +6,7 @@ it, so the truth table can still be exercised end to end. """ +from types import SimpleNamespace from unittest.mock import MagicMock, patch from ddt import data, ddt, unpack @@ -110,23 +111,8 @@ def test_org_glob_scope_org_override_takes_precedence( class TestCourseAuthoringVisibilityFilter(TestCase): """Test CourseAuthoringVisibilityFilter, the pipeline step for AuthorizationDataRequested.""" - def test_drops_items_whose_scope_is_hidden(self): - """Test run_filter with a mix of visible and hidden scopes. - - Expected result: - - Only the item with a visible scope survives. - """ - items = [{"scope": COURSE_SCOPE}, {"scope": LIB_SCOPE}] - with patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False - ), patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.is_user_staff_or_superuser", return_value=False - ): - result = CourseAuthoringVisibilityFilter( - filter_type="test", running_pipeline=[] - ).run_filter(items=items, username="someuser") - - self.assertEqual(result, {"items": [{"scope": LIB_SCOPE}]}) + regular_user = SimpleNamespace(is_staff=False, is_superuser=False) + staff_user = SimpleNamespace(is_staff=True, is_superuser=False) def test_staff_or_superuser_bypasses_visibility(self): """Test run_filter for a staff/superuser with a hidden course scope. @@ -137,14 +123,12 @@ def test_staff_or_superuser_bypasses_visibility(self): items = [{"scope": COURSE_SCOPE}] with patch( "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False - ), patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.is_user_staff_or_superuser", return_value=True ): result = CourseAuthoringVisibilityFilter( filter_type="test", running_pipeline=[] - ).run_filter(items=items, username="admin") + ).run_filter(items=items, user=self.staff_user) - self.assertEqual(result, {"items": items}) + self.assertEqual(result, {"items": items, "user": self.staff_user}) def test_marks_allowed_false_instead_of_dropping_when_the_item_has_an_allowed_key(self): """Test run_filter with an item that carries an ``allowed`` key, mirroring PermissionValidationMeView. @@ -155,14 +139,15 @@ def test_marks_allowed_false_instead_of_dropping_when_the_item_has_an_allowed_ke items = [{"scope": COURSE_SCOPE, "action": "view", "allowed": True}] with patch( "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False - ), patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.is_user_staff_or_superuser", return_value=False ): result = CourseAuthoringVisibilityFilter( filter_type="test", running_pipeline=[] - ).run_filter(items=items, username="someuser") + ).run_filter(items=items, user=self.regular_user) - self.assertEqual(result, {"items": [{"scope": COURSE_SCOPE, "action": "view", "allowed": False}]}) + self.assertEqual( + result, + {"items": [{"scope": COURSE_SCOPE, "action": "view", "allowed": False}], "user": self.regular_user}, + ) def test_leaves_any_scope_items_untouched(self): """Test run_filter with an item whose scope is None (an any-scope check). @@ -173,11 +158,9 @@ def test_leaves_any_scope_items_untouched(self): items = [{"scope": None, "action": "view", "allowed": True}] with patch( "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False - ), patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.is_user_staff_or_superuser", return_value=False ): result = CourseAuthoringVisibilityFilter( filter_type="test", running_pipeline=[] - ).run_filter(items=items, username="someuser") + ).run_filter(items=items, user=self.regular_user) - self.assertEqual(result, {"items": items}) + self.assertEqual(result, {"items": items, "user": self.regular_user}) diff --git a/openedx_authz/tests/rest_api/test_views.py b/openedx_authz/tests/rest_api/test_views.py index 2ff927c1..ed77d7f6 100644 --- a/openedx_authz/tests/rest_api/test_views.py +++ b/openedx_authz/tests/rest_api/test_views.py @@ -310,11 +310,11 @@ def test_permission_validation_any_scope_success(self, request_data: list[dict], Expected result: - Returns 200 OK status - - Response reports scope as None and reports the any-scope result + - Response omits the scope key and reports the any-scope result """ self.client.force_authenticate(user=self.regular_user) expected_response = [ - {"action": perm["action"], "scope": None, "allowed": allowed} + {"action": perm["action"], "allowed": allowed} for perm, allowed in zip(request_data, permission_map) ] @@ -323,24 +323,19 @@ def test_permission_validation_any_scope_success(self, request_data: list[dict], self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.data, expected_response) - def test_permission_validation_any_scope_staff_bypasses_permission_and_visibility(self): - """Staff/superusers bypass both the permission check and the flag visibility check, for any action. + def test_permission_validation_any_scope_staff_always_allowed(self): + """Staff/superusers are allowed for any action when no scope is provided. Expected result: - Returns 200 OK status - - Both actions are allowed, even though this staff user has no - course-scoped Casbin grant at all: staff/superusers bypass flag - visibility the same way they bypass the permission check. + - Every action is allowed regardless of explicit assignments """ self.client.force_authenticate(user=self.admin_user) request_data = [ {"action": permissions.MANAGE_LIBRARY_TEAM.identifier}, {"action": permissions.COURSES_MANAGE_COURSE_TEAM.identifier}, ] - expected_response = [ - {"action": permissions.MANAGE_LIBRARY_TEAM.identifier, "scope": None, "allowed": True}, - {"action": permissions.COURSES_MANAGE_COURSE_TEAM.identifier, "scope": None, "allowed": True}, - ] + expected_response = [{"action": perm["action"], "allowed": True} for perm in request_data] response = self.client.post(self.url, data=request_data, format="json") @@ -377,7 +372,7 @@ def test_permission_validation_exception_handling(self, exception: Exception, st - Generic Exception: Returns 500 INTERNAL SERVER ERROR with appropriate message - ValueError: Returns 400 BAD REQUEST with scope format error message """ - with patch.object(api, "is_user_allowed_in_scope", side_effect=exception): + with patch.object(api, "is_user_allowed", side_effect=exception): response = self.client.post( self.url, data=[{"action": "edit_library", "scope": "lib:Org1:LIB1"}], @@ -867,6 +862,46 @@ def test_add_users_to_role_course_permissions(self, username: str, status_code: self.assertEqual(response.status_code, status_code) + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.authz.authorization_data.requested.v1": { + "pipeline": [ + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + ], + "fail_silently": False, + }, + }, + ) + @patch.object(api, "assign_role_to_user_in_scope") + def test_add_users_to_role_reports_hidden_scope_without_writing(self, mock_assign_role_to_user_in_scope): + """A scope hidden by the configured filter is reported and never written.""" + self.client.force_authenticate(user=User.objects.get(username="course_admin")) + request_data = { + "role": roles.COURSE_STAFF.external_key, + "scope": COURSE_SCOPE_ORG1, + "users": ["regular_2"], + } + + with patch.object(api.CourseOverviewData, "exists", return_value=True), patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", + return_value=False, + ): + response = self.client.put(self.url, data=request_data, format="json") + + self.assertEqual(response.status_code, status.HTTP_207_MULTI_STATUS) + self.assertEqual(response.data["completed"], []) + self.assertEqual( + response.data["errors"], + [ + { + "user_identifier": "regular_2", + "scope": COURSE_SCOPE_ORG1, + "error": "scope_not_available", + } + ], + ) + mock_assign_role_to_user_in_scope.assert_not_called() + @data( # With username ----------------------------- # Single user - success (admin user) @@ -1041,6 +1076,46 @@ def test_remove_users_from_role_course_permissions(self, username: str, status_c self.assertEqual(response.status_code, status_code) + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.authz.authorization_data.requested.v1": { + "pipeline": [ + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + ], + "fail_silently": False, + }, + }, + ) + @patch.object(api, "unassign_role_from_user") + def test_remove_users_from_role_reports_hidden_scope_without_writing(self, mock_unassign_role_from_user): + """A scope hidden by the configured filter is reported and never written.""" + self.client.force_authenticate(user=User.objects.get(username="course_admin")) + query_params = { + "role": roles.COURSE_STAFF.external_key, + "scope": COURSE_SCOPE_ORG1, + "users": "regular_2", + } + + with patch.object(api.CourseOverviewData, "exists", return_value=True), patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", + return_value=False, + ): + response = self.client.delete(f"{self.url}?{urlencode(query_params)}") + + self.assertEqual(response.status_code, status.HTTP_207_MULTI_STATUS) + self.assertEqual(response.data["completed"], []) + self.assertEqual( + response.data["errors"], + [ + { + "user_identifier": "regular_2", + "scope": COURSE_SCOPE_ORG1, + "error": "scope_not_available", + } + ], + ) + mock_unassign_role_from_user.assert_not_called() + @ddt class TestRoleUserAPIViewScopeStringValidation(ViewTestMixin): From 6bf4a12a93f72f78120c191b482a9817e4f3bbe1 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 15 Sep 2026 18:53:51 +0200 Subject: [PATCH 6/9] fix: resolve quality and docs checks after rebase --- openedx_authz/filters.py | 6 ++++-- .../rest_api/v1/course_authoring/pipeline.py | 18 ++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/openedx_authz/filters.py b/openedx_authz/filters.py index 092905e8..ed9b28fc 100644 --- a/openedx_authz/filters.py +++ b/openedx_authz/filters.py @@ -9,7 +9,8 @@ class ScopedItem(TypedDict): - """A scope-bearing item handled by an openedx_authz REST API endpoint. + """ + A scope-bearing item handled by an openedx_authz REST API endpoint. Endpoints may include additional keys beyond ``scope`` (e.g. ``role``, ``org``, or ``username``). @@ -54,7 +55,8 @@ class AuthorizationDataRequested(OpenEdxPublicFilter): def run_filter( cls, items: AuthorizationData, user: AbstractBaseUser ) -> tuple[AuthorizationData, list[dict[str, Any]]]: - """Run the pipeline configured for this filter. + """ + Run the pipeline configured for this filter. Args: items (AuthorizationData): scope-bearing response items or validated diff --git a/openedx_authz/rest_api/v1/course_authoring/pipeline.py b/openedx_authz/rest_api/v1/course_authoring/pipeline.py index c5d9a9c3..2ae3bf10 100644 --- a/openedx_authz/rest_api/v1/course_authoring/pipeline.py +++ b/openedx_authz/rest_api/v1/course_authoring/pipeline.py @@ -1,19 +1,20 @@ """ Pipeline step implementing course-authoring visibility for ``AuthorizationDataRequested``. -This is the isolated, opt-in implementation of the exception ``docs/decisions/0016-rest-api-domain-ownership-boundary.rst`` -and ``docs/decisions/0018-cross-domain-filtering-via-openedx-filters.rst`` document. It's the only -place in openedx_authz that computes course-authoring-flag visibility, and it's never registered -unless a deployment's ``OPEN_EDX_FILTERS_CONFIG`` explicitly wires it in, typically via a Tutor -plugin patch. Deleting this file and the patch that registers it removes the mechanism entirely; -no endpoint code depends on it existing. +This is the isolated, opt-in implementation of the exception documented in +``docs/decisions/0016-rest-api-domain-ownership-boundary.rst`` and +``docs/decisions/0018-cross-domain-filtering-via-openedx-filters.rst``. It's the only place in +openedx_authz that computes course-authoring-flag visibility, and it's never registered unless a +deployment's ``OPEN_EDX_FILTERS_CONFIG`` explicitly wires it in, typically via a Tutor plugin +patch. Deleting this file and the patch that registers it removes the mechanism entirely; no +endpoint code depends on it existing. """ from django.contrib.auth.models import AbstractBaseUser from openedx_filters.filters import PipelineStep from openedx_authz import api -from openedx_authz.filters import AuthorizationData, ScopedItem +from openedx_authz.filters import AuthorizationData SCOPE_NOT_AVAILABLE_ERROR = "scope_not_available" @@ -73,10 +74,11 @@ class CourseAuthoringVisibilityFilter(PipelineStep): - The item has an ``allowed`` key: kept, with ``allowed`` set to ``False`` if hidden. Preserves 1:1 correspondence for endpoints like ``PermissionValidationMeView`` that must return exactly one result per request. + Staff and superusers see everything, regardless of the flag's state. """ - def run_filter( + def run_filter( # pylint: disable=arguments-differ self, items: AuthorizationData, user: AbstractBaseUser, From 6af48da0ad8868ee505996b831dfcc96b4a53c88 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 18 Sep 2026 12:26:29 +0200 Subject: [PATCH 7/9] refactor: separate visibility filtering by authorization data shape --- openedx_authz/filters.py | 6 +- .../rest_api/v1/course_authoring/pipeline.py | 215 ++++++++++++------ .../course_authoring/test_pipeline.py | 16 +- 3 files changed, 161 insertions(+), 76 deletions(-) diff --git a/openedx_authz/filters.py b/openedx_authz/filters.py index ed9b28fc..1254567b 100644 --- a/openedx_authz/filters.py +++ b/openedx_authz/filters.py @@ -53,7 +53,7 @@ class AuthorizationDataRequested(OpenEdxPublicFilter): @classmethod def run_filter( - cls, items: AuthorizationData, user: AbstractBaseUser + cls, items: AuthorizationData ) -> tuple[AuthorizationData, list[dict[str, Any]]]: """ Run the pipeline configured for this filter. @@ -61,12 +61,10 @@ def run_filter( Args: items (AuthorizationData): scope-bearing response items or validated role-operation data. - user (AbstractBaseUser): the authenticated user requesting the data, - available to any pipeline step that needs to make a per-user decision. Returns: tuple[AuthorizationData, list[dict]]: modified data and errors supplied by the configured pipeline. """ - data = super().run_pipeline(items=items, user=user) + data = super().run_pipeline(items=items) return data["items"], data.get("errors", []) diff --git a/openedx_authz/rest_api/v1/course_authoring/pipeline.py b/openedx_authz/rest_api/v1/course_authoring/pipeline.py index 2ae3bf10..8c5cf579 100644 --- a/openedx_authz/rest_api/v1/course_authoring/pipeline.py +++ b/openedx_authz/rest_api/v1/course_authoring/pipeline.py @@ -10,7 +10,8 @@ endpoint code depends on it existing. """ -from django.contrib.auth.models import AbstractBaseUser +from collections.abc import Iterable + from openedx_filters.filters import PipelineStep from openedx_authz import api @@ -32,7 +33,7 @@ AUTHZ_COURSE_AUTHORING_FLAG = None -def _is_scope_visible(scope: api.ScopeData) -> bool: +def is_scope_visible(scope: api.ScopeData) -> bool: """Return whether a scope is visible under the course-authoring flag. - Library and other non-course scopes (e.g. 'lib:DemoX:CSPROB'): always visible. @@ -66,82 +67,168 @@ def _is_scope_visible(scope: api.ScopeData) -> bool: class CourseAuthoringVisibilityFilter(PipelineStep): """Applies course-authoring visibility to items from ``AuthorizationDataRequested``. - Every item carries a ``scope``. What happens when that scope is hidden depends on the - item's own shape, not on which endpoint sent it: + Permission results have an optional ``scope``. Role assignments have ``scopes``, + and role removals have one ``scope``. Hidden scopes affect each kind of data differently: - - ``scope`` is ``None`` (an any-scope check): left untouched. There's no single scope + - ``scope`` is absent or ``None`` (an any-scope check): left untouched. There's no single scope to check visibility against, and no candidate list is provided. - - The item has an ``allowed`` key: kept, with ``allowed`` set to ``False`` if hidden. - Preserves 1:1 correspondence for endpoints like ``PermissionValidationMeView`` that - must return exactly one result per request. - - Staff and superusers see everything, regardless of the flag's state. + - Permission results are kept, with ``allowed`` set to ``False`` for hidden scopes. + - Role assignments and removals exclude hidden scopes or users and return an error for each + affected user/scope pair. """ def run_filter( # pylint: disable=arguments-differ self, items: AuthorizationData, - user: AbstractBaseUser, **kwargs, ) -> dict: - """Apply course-authoring visibility to each item, per its own shape. + """Apply course-authoring visibility to permission results or role changes. Args: - items (AuthorizationData): scope-bearing response items or validated - role-operation data. - user: the authenticated Django user requesting the data. + items (AuthorizationData): Permission results or validated role assignment + or removal data, passed under the pipeline's ``items`` keyword. + Supported shapes include: + + - Permission results, with a concrete scope or no ``scope`` for an + any-scope check:: + + [ + { + "action": "courses.manage_course_team", + "scope": "course-v1:DemoX+CS101+2024", + "allowed": true + }, + { + "action": "courses.manage_course_team", + "allowed": true + } + ] + + - Validated role assignments, with a list of scopes:: + + { + "role": "", + "users": [ + "alice" + ], + "scopes": [ + "course-v1:DemoX+CS101+2024", + "course-v1:DemoX+*" + ] + } + + - Validated role removals, with a single scope:: + + { + "role": "", + "users": [ + "alice" + ], + "scope": "course-v1:DemoX+CS101+2024" + } + + **kwargs: Additional pipeline arguments, unused by this step. Returns: - dict: the modified ``items`` and unchanged requesting ``user``. + dict: Filtered ``items`` in the original shape, plus ``errors`` for role changes. """ - if user.is_staff or user.is_superuser: - return {"items": items, "user": user} - if isinstance(items, dict): - return {**self._filter_role_operations(items), "user": user} - - result = [] - for item in items: - scope = item.get("scope") - if scope is None or _is_scope_visible(api.ScopeData(external_key=scope)): - result.append(item) - elif "allowed" in item: - result.append({**item, "allowed": False}) - else: - result.append(item) - return {"items": result, "user": user} + if "scopes" in items: + return self._filter_role_assignments(items) + return self._filter_role_removals(items) + return self._filter_permission_results(items) + + @staticmethod + def _hidden_scopes(scopes: Iterable[str | None]) -> set[str]: + """Find scopes hidden by the course-authoring flag. + + Args: + scopes (Iterable[str | None]): External scope keys. None represents an + any-scope check and is skipped. + + Returns: + set[str]: Scope keys hidden by the flag. + """ + return { + scope + for scope in scopes + if scope and not is_scope_visible(api.ScopeData(external_key=scope)) + } + + def _filter_permission_results(self, permission_results: list[dict]) -> dict: + """Keep every permission result, denying those whose scopes are hidden. + + Args: + permission_results (list[dict]): Permission checks with required ``allowed`` and an + optional ``scope``. Other fields, such as ``action``, are preserved. + + Returns: + dict: ``items`` containing all results in order, with ``allowed=False`` + for hidden scopes. Any-scope results are unchanged. + """ + hidden = self._hidden_scopes(result.get("scope") for result in permission_results) + return { + "items": [ + {**result, "allowed": False} if result.get("scope") in hidden else result + for result in permission_results + ] + } + + def _filter_role_assignments(self, assignment_data: dict) -> dict: + """Exclude hidden scopes from the assignment batch. + + Args: + assignment_data (dict): Validated ``role``, ``users`` (usernames or emails), + and ``scopes`` (external scope keys) for a role assignment batch. + + Returns: + dict: ``items`` with only visible ``scopes``, in order, and ``errors`` for + each hidden scope/user pair. + """ + scopes = assignment_data["scopes"] + hidden = self._hidden_scopes(scopes) + return { + "items": {**assignment_data, "scopes": [scope for scope in scopes if scope not in hidden]}, + "errors": self._role_change_errors( + assignment_data["users"], (scope for scope in scopes if scope in hidden) + ), + } + + def _filter_role_removals(self, removal_data: dict) -> dict: + """Skip all removals when the batch's single scope is hidden. + + Args: + removal_data (dict): Validated ``role``, ``users`` (usernames or emails), + and one ``scope`` (an external scope key) for a role removal batch. + + Returns: + dict: ``items`` with ``users`` cleared if the scope is hidden, and ``errors`` + for each affected user. Otherwise, unchanged data and no errors. + """ + hidden = self._hidden_scopes([removal_data["scope"]]) + return { + "items": {**removal_data, "users": [] if hidden else removal_data["users"]}, + "errors": self._role_change_errors(removal_data["users"], hidden), + } @staticmethod - def _filter_role_operations(items: dict) -> dict: - """Remove unavailable role operations and return their response errors.""" - result = {**items} - errors = [] - users = items["users"] - - if "scopes" in items: - available_scopes = [] - for scope in items["scopes"]: - if _is_scope_visible(api.ScopeData(external_key=scope)): - available_scopes.append(scope) - else: - errors.extend( - { - "user_identifier": user_identifier, - "scope": scope, - "error": SCOPE_NOT_AVAILABLE_ERROR, - } - for user_identifier in users - ) - result["scopes"] = available_scopes - elif not _is_scope_visible(api.ScopeData(external_key=items["scope"])): - errors.extend( - { - "user_identifier": user_identifier, - "scope": items["scope"], - "error": SCOPE_NOT_AVAILABLE_ERROR, - } - for user_identifier in users - ) - result["users"] = [] - - return {"items": result, "errors": errors} + def _role_change_errors(user_identifiers: list[str], hidden_scopes: Iterable[str]) -> list[dict]: + """Build one error per affected user/scope pair. + + Args: + user_identifiers (list[str]): Usernames or email addresses from the batch. + hidden_scopes (Iterable[str]): Hidden external scope keys, in error order. + + Returns: + list[dict]: Errors containing ``user_identifier``, ``scope``, and + ``error="scope_not_available"``, ordered by scope then user as supplied. + """ + return [ + { + "user_identifier": user_identifier, + "scope": scope, + "error": SCOPE_NOT_AVAILABLE_ERROR, + } + for scope in hidden_scopes + for user_identifier in user_identifiers + ] diff --git a/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py b/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py index 08e25bab..cab61ecf 100644 --- a/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py +++ b/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py @@ -13,7 +13,7 @@ from django.test import TestCase from openedx_authz.api.data import ContentLibraryData, CourseOverviewData, OrgCourseOverviewGlobData -from openedx_authz.rest_api.v1.course_authoring.pipeline import CourseAuthoringVisibilityFilter, _is_scope_visible +from openedx_authz.rest_api.v1.course_authoring.pipeline import CourseAuthoringVisibilityFilter, is_scope_visible COURSE_SCOPE = "course-v1:Org1+COURSE1+2024" OTHER_COURSE_SCOPE = "course-v1:Org1+COURSE2+2024" @@ -39,7 +39,7 @@ def __call__(self, course_key=None) -> bool: @ddt class TestIsScopeVisible(TestCase): - """Test _is_scope_visible, dispatching to the right override tier depending on the scope's type.""" + """Test is_scope_visible, dispatching to the right override tier depending on the scope's type.""" @data( (False, None, None, False), @@ -53,7 +53,7 @@ class TestIsScopeVisible(TestCase): def test_course_scope_follows_the_truth_table( self, platform: bool, org_override: bool | None, course_override: bool | None, expected: bool ): - """Test _is_scope_visible for a concrete course scope against override combinations. + """Test is_scope_visible for a concrete course scope against override combinations. Expected result: - The scope is visible exactly when course override wins, else org @@ -63,10 +63,10 @@ def test_course_scope_follows_the_truth_table( "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", CourseWaffleFlagMock(platform, org_override, course_override), ): - self.assertEqual(_is_scope_visible(CourseOverviewData(external_key=COURSE_SCOPE)), expected) + self.assertEqual(is_scope_visible(CourseOverviewData(external_key=COURSE_SCOPE)), expected) def test_library_scope_is_always_visible_regardless_of_the_flag(self): - """Test _is_scope_visible for a library scope. + """Test is_scope_visible for a library scope. Expected result: - The scope is always visible, since it isn't course-authoring-gated. @@ -74,7 +74,7 @@ def test_library_scope_is_always_visible_regardless_of_the_flag(self): with patch( "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False ): - self.assertTrue(_is_scope_visible(ContentLibraryData(external_key=LIB_SCOPE))) + self.assertTrue(is_scope_visible(ContentLibraryData(external_key=LIB_SCOPE))) @data( ("on", False, True), @@ -85,7 +85,7 @@ def test_library_scope_is_always_visible_regardless_of_the_flag(self): def test_org_glob_scope_org_override_takes_precedence( self, override_choice: str, platform_default: bool, expected: bool ): - """Test _is_scope_visible for an org-glob scope against org/platform combinations. + """Test is_scope_visible for an org-glob scope against org/platform combinations. Expected result: - The scope follows the org override when set, else the platform default. @@ -105,7 +105,7 @@ def test_org_glob_scope_org_override_takes_precedence( "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=platform_default, ): - self.assertEqual(_is_scope_visible(scope), expected) + self.assertEqual(is_scope_visible(scope), expected) class TestCourseAuthoringVisibilityFilter(TestCase): From 295ef06442dc9e44e786278d23691eb31e7c3f9a Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 18 Sep 2026 13:40:01 +0200 Subject: [PATCH 8/9] refactor: separate authorization filters by operation --- openedx_authz/filters.py | 123 +++++--- .../rest_api/v1/course_authoring/pipeline.py | 270 ++++++++++-------- openedx_authz/rest_api/v1/views.py | 14 +- .../course_authoring/test_pipeline.py | 105 +++++-- openedx_authz/tests/rest_api/test_views.py | 12 +- 5 files changed, 320 insertions(+), 204 deletions(-) diff --git a/openedx_authz/filters.py b/openedx_authz/filters.py index 1254567b..49d10212 100644 --- a/openedx_authz/filters.py +++ b/openedx_authz/filters.py @@ -1,70 +1,109 @@ -""" -Open edX Filters exposed by openedx_authz's REST API. -""" +"""Open edX Filters exposed by openedx_authz's REST API.""" -from typing import Any, TypedDict +from typing import Any, Generic, TypedDict, TypeVar -from django.contrib.auth.models import AbstractBaseUser from openedx_filters.tooling import OpenEdxPublicFilter -class ScopedItem(TypedDict): - """ - A scope-bearing item handled by an openedx_authz REST API endpoint. - - Endpoints may include additional keys beyond ``scope`` (e.g. ``role``, ``org``, - or ``username``). - """ +class ScopedItem(TypedDict, total=False): + """Optional scope on a permission result; omission represents an any-scope check.""" scope: str | None -class ValidationItem(ScopedItem, total=False): - """A ``ScopedItem`` from a permission-validation response, which also carries ``allowed``.""" +class ValidationItem(ScopedItem): + """A permission result, including the action and its authorization outcome.""" + action: str allowed: bool -AuthorizationData = list[ScopedItem] | dict[str, Any] +class RoleAssignmentItems(TypedDict): + """Validated input for assigning a role to users in one or more scopes.""" + role: str + users: list[str] + scopes: list[str] -class AuthorizationDataRequested(OpenEdxPublicFilter): - """ - Filter used to modify scope-bearing Authorization data handled by a REST API endpoint. - Purpose: - This filter is triggered when an openedx_authz REST API endpoint needs another - domain to modify a list of items that each carry a ``scope``. The items may be - response data or scopes about to be used by an operation. Unconfigured (no pipeline step - registered in ``OPEN_EDX_FILTERS_CONFIG``), every item stays as given; this filter - carries no assumption about why a pipeline step might change an item. +class RoleRemovalItems(TypedDict): + """Validated input for removing a role from users in one scope.""" - Filter Type: - org.openedx.authz.authorization_data.requested.v1 + role: str + users: list[str] + scope: str - Trigger: - - Repository: openedx/openedx-authz - - Path: openedx_authz/rest_api/v1/views.py - - Function or Method: PermissionValidationMeView.post, RoleUserAPIView.put, - RoleUserAPIView.delete + +AuthorizationItems = TypeVar("AuthorizationItems", list[ValidationItem], RoleAssignmentItems, RoleRemovalItems) + + +class AuthorizationDataRequested(OpenEdxPublicFilter, Generic[AuthorizationItems]): """ + Shared pipeline plumbing for operation-specific REST authorization filters. - filter_type = "org.openedx.authz.authorization_data.requested.v1" + Subclasses declare their payload type and filter identifier. Pipeline steps own + rejection rules and return data in the original shape, preserving earlier errors. + """ @classmethod def run_filter( - cls, items: AuthorizationData - ) -> tuple[AuthorizationData, list[dict[str, Any]]]: + cls, items: AuthorizationItems, + ) -> tuple[AuthorizationItems, list[dict[str, Any]]]: """ - Run the pipeline configured for this filter. + Run the operation's configured pipeline with an initially empty error list. Args: - items (AuthorizationData): scope-bearing response items or validated - role-operation data. - + items (AuthorizationItems): Computed permission results or validated role + change data, using the payload type declared by the subclass. Returns: - tuple[AuthorizationData, list[dict]]: modified data and errors supplied - by the configured pipeline. + tuple[AuthorizationItems, list[dict[str, Any]]]: Items in their original + shape and accumulated pipeline errors. Without a configured pipeline, + returns the original items and an empty error list. """ - data = super().run_pipeline(items=items) - return data["items"], data.get("errors", []) + data = super().run_pipeline(items=items, errors=[]) + return data["items"], data["errors"] + + +class PermissionValidationRequested(AuthorizationDataRequested[list[ValidationItem]]): + """ + Filter computed permission results before response serialization. + + Each item contains ``action`` and ``allowed``, with an optional ``scope``. + + Trigger: + ``PermissionValidationMeView.post``, after authorization checks and before + response serialization. + + Filter Type: + org.openedx.authz.permission_validation.requested.v1 + """ + + filter_type = "org.openedx.authz.permission_validation.requested.v1" + + +class RoleAssignmentRequested(AuthorizationDataRequested[RoleAssignmentItems]): + """ + Filter validated ``role``, ``users``, and ``scopes`` before assignment writes. + + Trigger: + ``RoleUserAPIView.put``, after request validation and before assigning roles. + + Filter Type: + org.openedx.authz.role_assignment.requested.v1 + """ + + filter_type = "org.openedx.authz.role_assignment.requested.v1" + + +class RoleRemovalRequested(AuthorizationDataRequested[RoleRemovalItems]): + """ + Filter validated ``role``, ``users``, and ``scope`` before removal writes. + + Trigger: + ``RoleUserAPIView.delete``, after request validation and before removing roles. + + Filter Type: + org.openedx.authz.role_removal.requested.v1 + """ + + filter_type = "org.openedx.authz.role_removal.requested.v1" diff --git a/openedx_authz/rest_api/v1/course_authoring/pipeline.py b/openedx_authz/rest_api/v1/course_authoring/pipeline.py index 8c5cf579..3cc5f8a0 100644 --- a/openedx_authz/rest_api/v1/course_authoring/pipeline.py +++ b/openedx_authz/rest_api/v1/course_authoring/pipeline.py @@ -1,9 +1,9 @@ """ -Pipeline step implementing course-authoring visibility for ``AuthorizationDataRequested``. +Pipeline steps implementing course-authoring visibility for REST authorization filters. This is the isolated, opt-in implementation of the exception documented in ``docs/decisions/0016-rest-api-domain-ownership-boundary.rst`` and -``docs/decisions/0018-cross-domain-filtering-via-openedx-filters.rst``. It's the only place in +``docs/decisions/0017-cross-domain-filtering-via-openedx-filters.rst``. It's the only place in openedx_authz that computes course-authoring-flag visibility, and it's never registered unless a deployment's ``OPEN_EDX_FILTERS_CONFIG`` explicitly wires it in, typically via a Tutor plugin patch. Deleting this file and the patch that registers it removes the mechanism entirely; no @@ -11,11 +11,17 @@ """ from collections.abc import Iterable +from typing import Generic from openedx_filters.filters import PipelineStep from openedx_authz import api -from openedx_authz.filters import AuthorizationData +from openedx_authz.filters import ( + AuthorizationItems, + RoleAssignmentItems, + RoleRemovalItems, + ValidationItem, +) SCOPE_NOT_AVAILABLE_ERROR = "scope_not_available" @@ -34,7 +40,8 @@ def is_scope_visible(scope: api.ScopeData) -> bool: - """Return whether a scope is visible under the course-authoring flag. + """ + Return whether a scope is visible under the course-authoring flag. - Library and other non-course scopes (e.g. 'lib:DemoX:CSPROB'): always visible. - Concrete course (e.g. 'course-v1:DemoX+CS101+2024'): full course/org/platform @@ -64,87 +71,53 @@ def is_scope_visible(scope: api.ScopeData) -> bool: return enable_authz_course_authoring() -class CourseAuthoringVisibilityFilter(PipelineStep): - """Applies course-authoring visibility to items from ``AuthorizationDataRequested``. - - Permission results have an optional ``scope``. Role assignments have ``scopes``, - and role removals have one ``scope``. Hidden scopes affect each kind of data differently: - - - ``scope`` is absent or ``None`` (an any-scope check): left untouched. There's no single scope - to check visibility against, and no candidate list is provided. - - Permission results are kept, with ``allowed`` set to ``False`` for hidden scopes. - - Role assignments and removals exclude hidden scopes or users and return an error for each - affected user/scope pair. - """ +class CourseAuthoringVisibilityFilter(PipelineStep, Generic[AuthorizationItems]): + """Share scope visibility and error accumulation across operations.""" def run_filter( # pylint: disable=arguments-differ self, - items: AuthorizationData, + items: AuthorizationItems, + errors: list[dict], **kwargs, ) -> dict: - """Apply course-authoring visibility to permission results or role changes. + """ + Apply the subclass's transformation and preserve earlier pipeline errors. Args: - items (AuthorizationData): Permission results or validated role assignment - or removal data, passed under the pipeline's ``items`` keyword. - Supported shapes include: - - - Permission results, with a concrete scope or no ``scope`` for an - any-scope check:: - - [ - { - "action": "courses.manage_course_team", - "scope": "course-v1:DemoX+CS101+2024", - "allowed": true - }, - { - "action": "courses.manage_course_team", - "allowed": true - } - ] - - - Validated role assignments, with a list of scopes:: - - { - "role": "", - "users": [ - "alice" - ], - "scopes": [ - "course-v1:DemoX+CS101+2024", - "course-v1:DemoX+*" - ] - } - - - Validated role removals, with a single scope:: - - { - "role": "", - "users": [ - "alice" - ], - "scope": "course-v1:DemoX+CS101+2024" - } - + items (AuthorizationItems): Operation-specific input documented by the subclass. + errors (list[dict]): Errors from earlier steps, preserved before new errors. **kwargs: Additional pipeline arguments, unused by this step. Returns: - dict: Filtered ``items`` in the original shape, plus ``errors`` for role changes. + dict: Filtered ``items`` in the original shape and accumulated ``errors``. + """ + filtered_items, new_errors = self._filter_items(items) + return {"items": filtered_items, "errors": [*errors, *new_errors]} + + def _filter_items(self, items: AuthorizationItems) -> tuple[AuthorizationItems, list[dict]]: + """ + Define the transformation implemented by each operation-specific step. + + Args: + items (AuthorizationItems): The operation's input data. + + Returns: + tuple[AuthorizationItems, list[dict]]: Transformed items in the original + shape and errors produced by this step, excluding earlier errors. + + Raises: + NotImplementedError: The subclass has not implemented its transformation. """ - if isinstance(items, dict): - if "scopes" in items: - return self._filter_role_assignments(items) - return self._filter_role_removals(items) - return self._filter_permission_results(items) + raise NotImplementedError("Subclasses must implement their operation's transformation.") @staticmethod def _hidden_scopes(scopes: Iterable[str | None]) -> set[str]: - """Find scopes hidden by the course-authoring flag. + """ + Find scopes hidden by the course-authoring flag. Args: - scopes (Iterable[str | None]): External scope keys. None represents an - any-scope check and is skipped. + scopes (Iterable[str | None]): External scope keys. None and empty strings + represent any-scope checks and are skipped. Returns: set[str]: Scope keys hidden by the flag. @@ -155,80 +128,135 @@ def _hidden_scopes(scopes: Iterable[str | None]) -> set[str]: if scope and not is_scope_visible(api.ScopeData(external_key=scope)) } - def _filter_permission_results(self, permission_results: list[dict]) -> dict: - """Keep every permission result, denying those whose scopes are hidden. + @staticmethod + def _role_change_errors(user_identifiers: list[str], hidden_scopes: Iterable[str]) -> list[dict]: + """ + Build one error per affected user/scope pair. Args: - permission_results (list[dict]): Permission checks with required ``allowed`` and an - optional ``scope``. Other fields, such as ``action``, are preserved. + user_identifiers (list[str]): Usernames or email addresses from the batch. + hidden_scopes (Iterable[str]): Hidden external scope keys, in error order. Returns: - dict: ``items`` containing all results in order, with ``allowed=False`` - for hidden scopes. Any-scope results are unchanged. + list[dict]: Errors containing ``user_identifier``, ``scope``, and + ``error="scope_not_available"``, ordered by scope then user as supplied. """ - hidden = self._hidden_scopes(result.get("scope") for result in permission_results) - return { - "items": [ - {**result, "allowed": False} if result.get("scope") in hidden else result - for result in permission_results - ] - } + return [ + { + "user_identifier": user_identifier, + "scope": scope, + "error": SCOPE_NOT_AVAILABLE_ERROR, + } + for scope in hidden_scopes + for user_identifier in user_identifiers + ] + + +class CourseAuthoringPermissionValidationFilter(CourseAuthoringVisibilityFilter[list[ValidationItem]]): + """ + Deny permission results whose scopes are hidden, retaining every result. - def _filter_role_assignments(self, assignment_data: dict) -> dict: - """Exclude hidden scopes from the assignment batch. + Input ``items`` contains computed permission results:: + + [ + { + "action": "courses.manage_course_team", + "scope": "course-v1:DemoX+CS101+2024", + "allowed": True, + }, + {"action": "courses.manage_course_team", "allowed": True}, + ] + + An absent, None, or empty ``scope`` represents an any-scope check and remains + unchanged. Hidden scopes receive ``allowed=False``; other fields are preserved. + """ + + def _filter_items(self, items: list[ValidationItem]) -> tuple[list[ValidationItem], list[dict]]: + """ + Set hidden-scope permission results to disallowed without dropping items. Args: - assignment_data (dict): Validated ``role``, ``users`` (usernames or emails), - and ``scopes`` (external scope keys) for a role assignment batch. + items (list[ValidationItem]): Computed results with ``action``, ``allowed``, + and an optional ``scope``; see the class docstring for an input example. Returns: - dict: ``items`` with only visible ``scopes``, in order, and ``errors`` for - each hidden scope/user pair. + tuple[list[ValidationItem], list[dict]]: Results in their original order, + with ``allowed=False`` for hidden scopes, and an empty error list. + Any-scope results and other fields remain unchanged. """ - scopes = assignment_data["scopes"] - hidden = self._hidden_scopes(scopes) - return { - "items": {**assignment_data, "scopes": [scope for scope in scopes if scope not in hidden]}, - "errors": self._role_change_errors( - assignment_data["users"], (scope for scope in scopes if scope in hidden) - ), + hidden = self._hidden_scopes(item.get("scope") for item in items) + filtered: list[ValidationItem] = [ + {**item, "allowed": False} if item.get("scope") in hidden else item + for item in items + ] + return filtered, [] + + +class CourseAuthoringRoleAssignmentFilter(CourseAuthoringVisibilityFilter[RoleAssignmentItems]): + """ + Exclude hidden scopes from validated assignment data before writes. + + Input ``items`` contains a role, user identifiers, and one or more scopes:: + + { + "role": "course_staff", + "users": ["alice"], + "scopes": ["course-v1:DemoX+CS101+2024", "course-v1:DemoX+*"], } - def _filter_role_removals(self, removal_data: dict) -> dict: - """Skip all removals when the batch's single scope is hidden. + Visible scopes remain in their original order. Each rejected user/scope pair + produces a ``scope_not_available`` error. + """ + + def _filter_items(self, items: RoleAssignmentItems) -> tuple[RoleAssignmentItems, list[dict]]: + """ + Remove hidden scopes from an assignment batch and report affected users. Args: - removal_data (dict): Validated ``role``, ``users`` (usernames or emails), - and one ``scope`` (an external scope key) for a role removal batch. + items (RoleAssignmentItems): Validated ``role``, ``users`` (usernames or + emails), and ``scopes``; see the class docstring for an input example. Returns: - dict: ``items`` with ``users`` cleared if the scope is hidden, and ``errors`` - for each affected user. Otherwise, unchanged data and no errors. + tuple[RoleAssignmentItems, list[dict]]: Assignment data containing only + visible scopes, in order, and one ``scope_not_available`` error for + each rejected user/scope pair. """ - hidden = self._hidden_scopes([removal_data["scope"]]) - return { - "items": {**removal_data, "users": [] if hidden else removal_data["users"]}, - "errors": self._role_change_errors(removal_data["users"], hidden), + scopes = items["scopes"] + hidden = self._hidden_scopes(scopes) + filtered: RoleAssignmentItems = {**items, "scopes": [scope for scope in scopes if scope not in hidden]} + errors = self._role_change_errors(items["users"], (scope for scope in scopes if scope in hidden)) + return filtered, errors + + +class CourseAuthoringRoleRemovalFilter(CourseAuthoringVisibilityFilter[RoleRemovalItems]): + """ + Exclude users from validated removal data when its scope is hidden. + + Input ``items`` contains a role, user identifiers, and a single scope:: + + { + "role": "course_staff", + "users": ["alice"], + "scope": "course-v1:DemoX+CS101+2024", } - @staticmethod - def _role_change_errors(user_identifiers: list[str], hidden_scopes: Iterable[str]) -> list[dict]: - """Build one error per affected user/scope pair. + A hidden scope clears ``users`` and produces a ``scope_not_available`` error + for each affected user. Visible scopes leave the data unchanged. + """ + + def _filter_items(self, items: RoleRemovalItems) -> tuple[RoleRemovalItems, list[dict]]: + """ + Clear the removal batch's users when its scope is hidden. Args: - user_identifiers (list[str]): Usernames or email addresses from the batch. - hidden_scopes (Iterable[str]): Hidden external scope keys, in error order. + items (RoleRemovalItems): Validated ``role``, ``users`` (usernames or + emails), and ``scope``; see the class docstring for an input example. Returns: - list[dict]: Errors containing ``user_identifier``, ``scope``, and - ``error="scope_not_available"``, ordered by scope then user as supplied. + tuple[RoleRemovalItems, list[dict]]: Removal data with ``users`` cleared + and one ``scope_not_available`` error per user if the scope is hidden. + Otherwise, returns unchanged data and no errors. """ - return [ - { - "user_identifier": user_identifier, - "scope": scope, - "error": SCOPE_NOT_AVAILABLE_ERROR, - } - for scope in hidden_scopes - for user_identifier in user_identifiers - ] + hidden = self._hidden_scopes([items["scope"]]) + filtered: RoleRemovalItems = {**items, "users": [] if hidden else items["users"]} + return filtered, self._role_change_errors(items["users"], hidden) diff --git a/openedx_authz/rest_api/v1/views.py b/openedx_authz/rest_api/v1/views.py index cca32960..e571166c 100644 --- a/openedx_authz/rest_api/v1/views.py +++ b/openedx_authz/rest_api/v1/views.py @@ -20,7 +20,7 @@ from openedx_authz import api from openedx_authz.api.utils import get_user_map from openedx_authz.constants import permissions -from openedx_authz.filters import AuthorizationDataRequested +from openedx_authz.filters import PermissionValidationRequested, RoleAssignmentRequested, RoleRemovalRequested from openedx_authz.rest_api.data import RoleOperationError, RoleOperationStatus from openedx_authz.rest_api.decorators import authz_permissions, view_auth_classes from openedx_authz.rest_api.utils import ( @@ -153,7 +153,9 @@ def post(self, request: HttpRequest) -> Response: status=status.HTTP_500_INTERNAL_SERVER_ERROR, ) - response_data, _ = AuthorizationDataRequested.run_filter(items=response_data, user=request.user) + response_data, _ = PermissionValidationRequested.run_filter( + items=response_data + ) serializer = PermissionValidationResponseSerializer(response_data, many=True) return Response(serializer.data, status=status.HTTP_200_OK) @@ -315,7 +317,9 @@ def put(self, request: HttpRequest) -> Response: serializer.is_valid(raise_exception=True) data = serializer.validated_data - data, errors = AuthorizationDataRequested.run_filter(items=data, user=request.user) + data, errors = RoleAssignmentRequested.run_filter( + items=data + ) completed = [] for scope_value in data["scopes"]: for user_identifier in data["users"]: @@ -363,7 +367,9 @@ def delete(self, request: HttpRequest) -> Response: serializer.is_valid(raise_exception=True) data = serializer.validated_data - data, errors = AuthorizationDataRequested.run_filter(items=data, user=request.user) + data, errors = RoleRemovalRequested.run_filter( + items=data + ) completed = [] for user_identifier in data["users"]: response_dict = {"user_identifier": user_identifier} diff --git a/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py b/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py index cab61ecf..52426f7a 100644 --- a/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py +++ b/openedx_authz/tests/rest_api/course_authoring/test_pipeline.py @@ -6,14 +6,19 @@ it, so the truth table can still be exercised end to end. """ -from types import SimpleNamespace from unittest.mock import MagicMock, patch from ddt import data, ddt, unpack -from django.test import TestCase +from django.test import TestCase, override_settings from openedx_authz.api.data import ContentLibraryData, CourseOverviewData, OrgCourseOverviewGlobData -from openedx_authz.rest_api.v1.course_authoring.pipeline import CourseAuthoringVisibilityFilter, is_scope_visible +from openedx_authz.filters import PermissionValidationRequested, RoleAssignmentRequested, RoleRemovalRequested +from openedx_authz.rest_api.v1.course_authoring.pipeline import ( + CourseAuthoringPermissionValidationFilter, + CourseAuthoringRoleAssignmentFilter, + CourseAuthoringRoleRemovalFilter, + is_scope_visible, +) COURSE_SCOPE = "course-v1:Org1+COURSE1+2024" OTHER_COURSE_SCOPE = "course-v1:Org1+COURSE2+2024" @@ -108,27 +113,9 @@ def test_org_glob_scope_org_override_takes_precedence( self.assertEqual(is_scope_visible(scope), expected) -class TestCourseAuthoringVisibilityFilter(TestCase): - """Test CourseAuthoringVisibilityFilter, the pipeline step for AuthorizationDataRequested.""" - - regular_user = SimpleNamespace(is_staff=False, is_superuser=False) - staff_user = SimpleNamespace(is_staff=True, is_superuser=False) - - def test_staff_or_superuser_bypasses_visibility(self): - """Test run_filter for a staff/superuser with a hidden course scope. - - Expected result: - - Every item survives, regardless of the flag's state. - """ - items = [{"scope": COURSE_SCOPE}] - with patch( - "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False - ): - result = CourseAuthoringVisibilityFilter( - filter_type="test", running_pipeline=[] - ).run_filter(items=items, user=self.staff_user) - - self.assertEqual(result, {"items": items, "user": self.staff_user}) +@ddt +class TestCourseAuthoringPermissionValidationFilter(TestCase): + """Test operation-specific course-authoring visibility steps.""" def test_marks_allowed_false_instead_of_dropping_when_the_item_has_an_allowed_key(self): """Test run_filter with an item that carries an ``allowed`` key, mirroring PermissionValidationMeView. @@ -140,13 +127,15 @@ def test_marks_allowed_false_instead_of_dropping_when_the_item_has_an_allowed_ke with patch( "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False ): - result = CourseAuthoringVisibilityFilter( + result = CourseAuthoringPermissionValidationFilter( filter_type="test", running_pipeline=[] - ).run_filter(items=items, user=self.regular_user) + ).run_filter( + items=items, errors=[] + ) self.assertEqual( result, - {"items": [{"scope": COURSE_SCOPE, "action": "view", "allowed": False}], "user": self.regular_user}, + {"items": [{"scope": COURSE_SCOPE, "action": "view", "allowed": False}], "errors": []}, ) def test_leaves_any_scope_items_untouched(self): @@ -155,12 +144,66 @@ def test_leaves_any_scope_items_untouched(self): Expected result: - The item survives unchanged; there's no single scope to check visibility against. """ - items = [{"scope": None, "action": "view", "allowed": True}] + items = [ + {"scope": None, "action": "view", "allowed": True}, + {"action": "view", "allowed": True}, + {"scope": "", "action": "view", "allowed": True}, + ] with patch( "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False ): - result = CourseAuthoringVisibilityFilter( + result = CourseAuthoringPermissionValidationFilter( filter_type="test", running_pipeline=[] - ).run_filter(items=items, user=self.regular_user) + ).run_filter( + items=items, errors=[] + ) + + self.assertEqual(result, {"items": items, "errors": []}) - self.assertEqual(result, {"items": items, "user": self.regular_user}) + def test_assignment_keeps_visible_scopes_and_preserves_previous_errors(self): + """Partial rejection keeps available writes and earlier pipeline errors.""" + items = {"role": "course_staff", "users": ["alice", "bob"], "scopes": [COURSE_SCOPE, LIB_SCOPE]} + previous_errors = [{"error": "previous_step"}] + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False + ): + result = CourseAuthoringRoleAssignmentFilter(filter_type="test", running_pipeline=[]).run_filter( + items=items, errors=previous_errors + ) + + self.assertEqual(result["items"], {**items, "scopes": [LIB_SCOPE]}) + self.assertEqual(result["errors"], [ + {"error": "previous_step"}, + {"user_identifier": "alice", "scope": COURSE_SCOPE, "error": "scope_not_available"}, + {"user_identifier": "bob", "scope": COURSE_SCOPE, "error": "scope_not_available"}, + ]) + self.assertEqual(previous_errors, [{"error": "previous_step"}]) + self.assertEqual(items["scopes"], [COURSE_SCOPE, LIB_SCOPE]) + + def test_removal_preserves_extra_fields(self): + """Additional metadata must not turn a removal into an assignment.""" + items = {"role": "course_staff", "users": ["alice"], "scope": COURSE_SCOPE, "scopes": [LIB_SCOPE]} + with patch( + "openedx_authz.rest_api.v1.course_authoring.pipeline.enable_authz_course_authoring", return_value=False + ): + result = CourseAuthoringRoleRemovalFilter(filter_type="test", running_pipeline=[]).run_filter( + items=items, errors=[] + ) + + self.assertEqual(result["items"], {**items, "users": []}) + self.assertEqual(result["errors"], [ + {"user_identifier": "alice", "scope": COURSE_SCOPE, "error": "scope_not_available"}, + ]) + + @data( + (PermissionValidationRequested, [{"action": "view", "allowed": True}]), + (RoleAssignmentRequested, {"role": "course_staff", "users": ["alice"], "scopes": [COURSE_SCOPE]}), + (RoleRemovalRequested, {"role": "course_staff", "users": ["alice"], "scope": COURSE_SCOPE}), + ) + @unpack + @override_settings(OPEN_EDX_FILTERS_CONFIG={}) + def test_unconfigured_hook_returns_original_data(self, filter_class, items): + """Each public hook passes through its payload when no pipeline is configured.""" + filtered, errors = filter_class.run_filter(items=items) + self.assertEqual(filtered, items) + self.assertEqual(errors, []) diff --git a/openedx_authz/tests/rest_api/test_views.py b/openedx_authz/tests/rest_api/test_views.py index ed77d7f6..9beb86a5 100644 --- a/openedx_authz/tests/rest_api/test_views.py +++ b/openedx_authz/tests/rest_api/test_views.py @@ -384,9 +384,9 @@ def test_permission_validation_exception_handling(self, exception: Exception, st @override_settings( OPEN_EDX_FILTERS_CONFIG={ - "org.openedx.authz.authorization_data.requested.v1": { + "org.openedx.authz.permission_validation.requested.v1": { "pipeline": [ - "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringPermissionValidationFilter", ], "fail_silently": False, }, @@ -864,9 +864,9 @@ def test_add_users_to_role_course_permissions(self, username: str, status_code: @override_settings( OPEN_EDX_FILTERS_CONFIG={ - "org.openedx.authz.authorization_data.requested.v1": { + "org.openedx.authz.role_assignment.requested.v1": { "pipeline": [ - "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringRoleAssignmentFilter", ], "fail_silently": False, }, @@ -1078,9 +1078,9 @@ def test_remove_users_from_role_course_permissions(self, username: str, status_c @override_settings( OPEN_EDX_FILTERS_CONFIG={ - "org.openedx.authz.authorization_data.requested.v1": { + "org.openedx.authz.role_removal.requested.v1": { "pipeline": [ - "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringVisibilityFilter", + "openedx_authz.rest_api.v1.course_authoring.pipeline.CourseAuthoringRoleRemovalFilter", ], "fail_silently": False, }, From bf76e58b293a36b3dd35a356483f2044b5d20a07 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 18 Sep 2026 14:15:34 +0200 Subject: [PATCH 9/9] docs: fix scope examples in generated API docs --- openedx_authz/rest_api/v1/course_authoring/pipeline.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx_authz/rest_api/v1/course_authoring/pipeline.py b/openedx_authz/rest_api/v1/course_authoring/pipeline.py index 3cc5f8a0..445e8e96 100644 --- a/openedx_authz/rest_api/v1/course_authoring/pipeline.py +++ b/openedx_authz/rest_api/v1/course_authoring/pipeline.py @@ -43,11 +43,11 @@ def is_scope_visible(scope: api.ScopeData) -> bool: """ Return whether a scope is visible under the course-authoring flag. - - Library and other non-course scopes (e.g. 'lib:DemoX:CSPROB'): always visible. - - Concrete course (e.g. 'course-v1:DemoX+CS101+2024'): full course/org/platform + - Library and other non-course scopes (e.g. ``lib:DemoX:CSPROB``): always visible. + - Concrete course (e.g. ``course-v1:DemoX+CS101+2024``): full course/org/platform cascade via ``enable_authz_course_authoring(course_key)``. - - Org-level course glob (e.g. 'course-v1:DemoX+*'): org override, else platform default. - - Platform-level course glob ('course-v1:*'): platform tier only, no course or org. + - Org-level course glob (e.g. ``course-v1:DemoX+*``): org override, else platform default. + - Platform-level course glob (``course-v1:*``): platform tier only, no course or org. Args: scope (ScopeData): A resolved scope instance.