diff --git a/docs/decisions/0015-course-authoring-flag-visibility-in-rest-api.rst b/docs/decisions/0015-course-authoring-flag-visibility-in-rest-api.rst new file mode 100644 index 00000000..9ac686cc --- /dev/null +++ b/docs/decisions/0015-course-authoring-flag-visibility-in-rest-api.rst @@ -0,0 +1,193 @@ +0015: Course Authoring Flag Visibility in the REST API +####################################################### + +Status +****** + +**Draft** + +Context +******* + +``authz.enable_course_authoring`` is a three-tier flag (`ADR 0010`_), where a course override wins over an org override, which in turn wins over the platform default. + +A migration is supposed to keep Casbin's role assignments in sync with this flag (`ADR 0011`_, `ADR 0013`_), but in practice it often doesn't, since it's off by default and never runs at all for platform-wide flag changes. That means Casbin can go on holding course data for a course whose flag is off, for as long as nobody runs the migration by hand. + +Because of that lag, only the flag itself can answer whether course authoring is on for a given course. Casbin's contents cannot. This is exactly how openedx-platform's own code already treats it, checking the flag directly before it ever touches Casbin. + +The flag never governs libraries either, since it only applies to courses; library access keeps using its own separate legacy path, unaffected by it. + +The rule this ADR needs to enforce, that course authoring content is visible only when the user has permission and the flag resolves to on, is captured in this truth table, where "None" means no override exists for that tier: + +.. list-table:: + :header-rows: 1 + + * - Platform Flag + - Org Override + - Course Override + - Effective Authoring State + - User Has Permission? + - Show Authoring Roles? + * - Off + - None + - None + - Off + - No + - No + * - Off + - None + - None + - Off + - Yes + - No + * - On + - None + - None + - On + - No + - No + * - On + - None + - None + - On + - Yes + - Yes + * - Off + - Force On + - None + - On + - No + - No + * - Off + - Force On + - None + - On + - Yes + - Yes + * - On + - Force On + - None + - On + - No + - No + * - On + - Force On + - None + - On + - Yes + - Yes + * - Off + - Force Off + - None + - Off + - No + - No + * - Off + - Force Off + - None + - Off + - Yes + - No + * - On + - Force Off + - None + - Off + - No + - No + * - On + - Force Off + - None + - Off + - Yes + - No + * - Off + - None + - Force On + - On + - No + - No + * - Off + - None + - Force On + - On + - Yes + - Yes + * - On + - None + - Force On + - On + - No + - No + * - On + - None + - Force On + - On + - Yes + - Yes + * - Off + - None + - Force Off + - Off + - No + - No + * - Off + - None + - Force Off + - Off + - Yes + - No + * - On + - None + - Force Off + - Off + - No + - No + * - On + - None + - Force Off + - Off + - Yes + - No + +This exact cascade is already implemented by ``enable_authz_course_authoring(course_key)`` in ``common.djangoapps.student.roles``, so nothing new needs to be built for it. Only the direct check for org-overrides needs to be added, since that function only accepts a course key and has no public API for checking an org alone. + +Decision +******** + +1. Course-scoped REST API endpoints became flag-aware, starting with ``PermissionValidationMeView``, ahead of and independent from the separate effort to reduce this app's openedx-platform dependencies (`issue #360`_). +2. Staff and superusers get no bypass for flag visibility. The flag's effective state applies the same way to every user. +3. Every permission check backed by Casbin data should be short-circuited by the flag's effective state for that scope, so a stale Casbin grant never surfaces flag-disabled content. +4. Make openedx-platform a temporary dependency of this repo, so that the flag's effective state can be read directly from the same models that openedx-platform itself uses. This is a stopgap until the app is fully decoupled from openedx-platform. + +This is implemented by ``is_scope_visible``/``has_visible_scope`` in ``openedx_authz.rest_api.utils``, and applied so far only to ``PermissionValidationMeView``. ``AssignmentsAPIView``, ``RoleUserAPIView``, ``TeamMembersAPIView``, ``TeamMemberAssignmentsAPIView``, ``RoleListView``, and ``ScopesAPIView`` read the same kind of Casbin data and carry the same staleness risk, but wiring them up is follow-up work, not part of this change. + +Consequences +************ + +1. **One place defines flag visibility**, reused by every call site instead of each view re-deriving it or trusting stale Casbin data. +2. **REST API behavior now matches openedx-platform's own enforcement points**, which already treat the flag as the source of truth. +3. **No staff/superuser bypass.** Once an endpoint is wired up, a flag-disabled course stays hidden or denied for every user, regardless of role. +4. **Wired-up endpoints stop returning whatever Casbin holds.** A course-scoped result can now be hidden or denied even though Casbin still has a matching row, whenever the flag is off and a rollback migration hasn't run yet. The REST API and Django Admin's migration status can visibly disagree during that window; this should be called out in user-facing docs about the flag. +5. **Per-row checks cost up to one flag resolution per distinct course/org in a response.** ``enable_authz_course_authoring`` reads models that are ``@request_cached()`` in openedx-platform, so repeat calls for the same course/org within one request are cheap, but a listing spanning N courses across M orgs still costs up to N + M + 1 lookups. +6. **These views depend on** ``enable_authz_course_authoring`` **and** ``WaffleFlagOrgOverrideModel``, guarded by the same standalone-import pattern already used in ``handlers.py`` for ``CourseAccessRole``, so the app keeps loading outside openedx-platform. There's no fail-open fallback: this repo runs as an openedx-platform plugin, so these imports are always available at runtime. + +Rejected Alternatives +********************** + +**Gating each endpoint as a whole (e.g. 404 for the entire endpoint when the flag is off)**: Every one of these endpoints legitimately continues to serve library data regardless of this flag's state. An endpoint-level gate would hide that data too. + +**Relying on Casbin data presence as a proxy for flag state**: ADR 0013 establishes that Casbin content and the flag's effective state can diverge by design (opt-in automatic migration, never triggered for global flag changes). Therefore, treating "the row exists in Casbin" as equivalent to "the flag is on" is wrong and would break the truth table above. + +References +********** + +* `ADR 0010`_ +* `ADR 0011`_ +* `ADR 0013`_ +* `issue #360`_ + +.. _ADR 0010: 0010-course-authoring-flag.rst +.. _ADR 0011: 0011-course-authoring-migration-process.rst +.. _ADR 0013: 0013-course-authoring-automatic-migration.rst +.. _0013: 0013-course-authoring-automatic-migration.rst +.. _issue #360: https://github.com/openedx/openedx-authz/issues/360 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 96686338..77b0cb05 100644 --- a/openedx_authz/rest_api/v1/views.py +++ b/openedx_authz/rest_api/v1/views.py @@ -46,6 +46,7 @@ from openedx_authz.rest_api.utils import ( filter_users, get_generic_scope, + has_visible_scope, sort_users, ) from openedx_authz.rest_api.v1.filters import ( @@ -148,8 +149,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} ] """ @@ -174,12 +175,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 9bb236f6..080ab2d7 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, @@ -668,6 +669,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 e7632817..7d07aa90 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 @@ -30,6 +31,7 @@ from openedx_authz.rest_api.v1.permissions import AnyScopePermission, DynamicScopePermission from openedx_authz.rest_api.v1.views import ScopesAPIView, UserValidationAPIView from openedx_authz.tests.api.test_roles import BaseRolesTestCase +from openedx_authz.tests.rest_api.test_utils import CourseWaffleFlagMock from openedx_authz.tests.stubs.models import LearningPackage ContentLibrary = get_content_library_model() @@ -315,11 +317,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) ] @@ -328,19 +330,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") @@ -377,7 +387,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"}], @@ -388,6 +398,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."""