From 67079257e4142b4bb079202dda22b37eb5fcef20 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 6 Oct 2026 17:59:40 -0500 Subject: [PATCH 01/16] chore: upgrade openedx-authz==1.23.0 (#39001) --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index eefde1711f73..b7ded5e08ae7 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -834,7 +834,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==1.21.0 +openedx-authz==1.23.0 # via -r requirements/edx/kernel.in openedx-calc==5.0.0 # via diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index c6a407ff78dc..86683cd8ef28 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1374,7 +1374,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==1.21.0 +openedx-authz==1.23.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index c39abd009e40..25bbf523f806 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1012,7 +1012,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==1.21.0 +openedx-authz==1.23.0 # via -r requirements/edx/base.txt openedx-calc==5.0.0 # via diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 02ebe71f678d..a7a387cda1a2 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1052,7 +1052,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==1.21.0 +openedx-authz==1.23.0 # via -r requirements/edx/base.txt openedx-calc==5.0.0 # via From 6aeb002e77ac091e8f2d4d9e684b799154dbb63d Mon Sep 17 00:00:00 2001 From: KEVYN SUAREZ <91025555+efortish@users.noreply.github.com> Date: Wed, 19 Aug 2026 12:14:08 -0500 Subject: [PATCH 02/16] revert: Cannot rerun courses - authz role assignment expects rerun to already exist (#38973) (cherry picked from commit 07f09868c1f851dc038988ca190f915db4c81438) --- cms/djangoapps/contentstore/tasks.py | 10 +--- .../contentstore/tests/test_clone_course.py | 55 ++----------------- .../tests/test_course_create_rerun.py | 27 +++------ cms/djangoapps/contentstore/views/course.py | 28 ++-------- 4 files changed, 18 insertions(+), 102 deletions(-) diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index c97d3cba4209..b73800053288 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -56,7 +56,6 @@ from cms.djangoapps.contentstore.toggles import enable_course_optimizer_check_prev_run_links from cms.djangoapps.contentstore.utils import ( IMPORTABLE_FILE_TYPES, - add_instructor, contains_course_reference, create_course_info_usage_key, create_or_update_xblock_upstream_link, @@ -189,14 +188,7 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i update_unit_discussion_state_from_discussion_blocks(destination_course_key, user_id) # set initial permissions for the user to access the course. - # NOTE: add_instructor is called here (after clone_course) because when - # authz.enable_course_authoring is enabled, it cannot be called pre-task - # (CourseOverview doesn't exist yet). This is a temporary workaround until - # openedx/openedx-authz#352 is implemented. Once resolved, add_instructor - # can move back to the pre-task call site unconditionally. - user = User.objects.get(id=user_id) - add_instructor(destination_course_key, user, user) - initialize_permissions(destination_course_key, user) + initialize_permissions(destination_course_key, User.objects.get(id=user_id)) # update state: Succeeded CourseRerunState.objects.succeeded(course_key=destination_course_key) diff --git a/cms/djangoapps/contentstore/tests/test_clone_course.py b/cms/djangoapps/contentstore/tests/test_clone_course.py index de9860a80b5e..2449c9269288 100644 --- a/cms/djangoapps/contentstore/tests/test_clone_course.py +++ b/cms/djangoapps/contentstore/tests/test_clone_course.py @@ -14,11 +14,10 @@ from common.djangoapps.course_action_state.managers import CourseRerunUIStateManager from common.djangoapps.course_action_state.models import CourseRerunState from common.djangoapps.student.auth import has_course_author_access -from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole -from xmodule.contentstore.content import StaticContent # pylint: disable=wrong-import-order -from xmodule.contentstore.django import contentstore # pylint: disable=wrong-import-order -from xmodule.modulestore import EdxJSONEncoder, ModuleStoreEnum # pylint: disable=wrong-import-order -from xmodule.modulestore.tests.factories import CourseFactory # pylint: disable=wrong-import-order +from xmodule.contentstore.content import StaticContent # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.contentstore.django import contentstore # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore import EdxJSONEncoder, ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT @@ -142,49 +141,3 @@ def test_rerun_course(self): course_key=split_course4_id, state=CourseRerunUIStateManager.State.FAILED ) - - - def test_rerun_course_grants_instructor_access(self): - """ - Test that the rerun_course task grants instructor and staff access - to the user after cloning. This verifies add_instructor is called - inside the task (needed when authz.enable_course_authoring is enabled - and add_instructor cannot be called pre-task). - - TODO: This test covers a temporary workaround until openedx/openedx-authz#352 - is implemented. Once authz supports pre-assigning roles without a CourseOverview, - add_instructor can move back to the pre-task call site and this test can be - simplified. - """ - org = 'edX' - course_number = 'CS101' - course_run = '2025_Q1' - display_name = 'rerun_instructor_test' - fields = {'display_name': display_name} - - # Create a source course - source_course = CourseFactory.create( - org=org, - number=course_number, - run=course_run, - display_name=display_name, - default_store=ModuleStoreEnum.Type.split, - ) - - dest_course_id = CourseLocator(org=org, course=course_number, run="instructor_rerun") - CourseRerunState.objects.initiated( - source_course.id, dest_course_id, self.user, fields['display_name'] - ) - - result = rerun_course.delay( - str(source_course.id), - str(dest_course_id), - self.user.id, - json.dumps(fields, cls=EdxJSONEncoder), - ) - assert result.get() == "succeeded" - - # Verify the user has instructor and staff access on the new course - assert has_course_author_access(self.user, dest_course_id) - assert CourseInstructorRole(dest_course_id).has_user(self.user) - assert CourseStaffRole(dest_course_id).has_user(self.user) diff --git a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py index 52359dd7c27a..b9a22cfaf46c 100644 --- a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py +++ b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py @@ -426,18 +426,10 @@ class TestCourseRerunAuthz( """ Tests for course rerun behavior when authz.enable_course_authoring is enabled. - Verifies that: - - Rerun succeeds without calling add_instructor pre-task (which would fail - because CourseOverview doesn't exist yet). - - add_instructor is called in the task after clone_course, when CourseOverview - can be resolved. - - The initiating user can see in-process rerun status via created_user check. - - TODO: These tests cover a temporary workaround needed while (1) the authz system - doesn't support pre-assigning roles without a CourseOverview, and (2) we support - both authz and legacy systems simultaneously. Once openedx/openedx-authz#352 is - implemented, this class can be simplified — the conditional skip of add_instructor - and the created_user visibility fallback will no longer be needed. + Verifies that rerun succeeds and grants proper access even though add_instructor + is called for the destination course key before its CourseOverview exists + (openedx-authz#352 makes role assignment succeed ahead of the object, backfilling + the link once clone_course creates it). """ def setUp(self): @@ -467,8 +459,8 @@ def setUp(self): def test_rerun_succeeds_with_authz_enabled(self): """ Test that course rerun completes successfully when authz.enable_course_authoring - is enabled. Previously this would fail with CourseOverview.DoesNotExist because - add_instructor was called before the course was cloned. + is enabled, granting the initiating user instructor/staff access on the + destination course before it's cloned. """ response = self.staff_client.ajax_post(self.url, { 'source_course_key': str(self.source_course_key), @@ -516,8 +508,7 @@ def test_rerun_does_not_create_legacy_roles_with_authz_enabled(self): def test_rerun_grants_authz_permissions_after_clone(self): """ Test that after a successful rerun with authz enabled, the user has proper - permissions via the authz layer (add_instructor is called in the task after - clone_course completes and CourseOverview is available). + permissions via the authz layer on the newly created destination course. """ response = self.staff_client.ajax_post(self.url, { 'source_course_key': str(self.source_course_key), @@ -537,8 +528,8 @@ def test_rerun_grants_authz_permissions_after_clone(self): def test_in_process_rerun_visible_to_initiating_user(self): """ Test that the user who initiated the rerun can see the in-process status - via the created_user check in get_in_process_course_actions, even when - authz permissions haven't been fully established yet. + in get_in_process_course_actions, via the instructor/staff access already + granted on the destination course. """ dest_course_key = CourseLocator(org='testorg', course='101', run='in_progress_rerun') CourseRerunState.objects.initiated( diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 2ee588195210..3c9c36bb00b9 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -429,19 +429,8 @@ def get_in_process_course_actions(request): exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED}, should_display=True, ) - if ( - # The user who initiated the rerun can always see its status. - # This is needed because when the authz flag is enabled, permission - # checks require a CourseOverview which doesn't exist until the - # rerun task clones the course. - # TODO: This created_user fallback is a temporary workaround until - # openedx/openedx-authz#352 is implemented. Once authz supports - # pre-assigning roles without a CourseOverview, this check can be removed - # and the standard permission check will suffice. - course.created_user == request.user - or user_has_course_permission( - request.user, COURSES_VIEW_COURSE.identifier, course.course_key, LegacyAuthoringPermission.READ - ) + if user_has_course_permission( + request.user, COURSES_VIEW_COURSE.identifier, course.course_key, LegacyAuthoringPermission.READ ) ] @@ -1345,17 +1334,8 @@ def rerun_course(user, source_course_key, org, number, run, fields, background=T raise PermissionDenied() # Make sure user has instructor and staff access to the destination course - # so the user can see the updated status for that course. - # When authz is enabled, we skip this because the authz layer requires a - # CourseOverview (which doesn't exist until the course is cloned in the task). - # In that case, visibility of the rerun status is granted by checking - # created_user on CourseRerunState instead. - # TODO: This conditional is a temporary workaround until openedx/openedx-authz#352 - # is implemented (pre-assigning roles without a CourseOverview). Once resolved, - # add_instructor can be called unconditionally here and the created_user fallback - # in get_in_process_course_actions can be removed. - if not enable_authz_course_authoring(destination_course_key): - add_instructor(destination_course_key, user, user) + # so the user can see the updated status for that course + add_instructor(destination_course_key, user, user) # Mark the action as initiated CourseRerunState.objects.initiated(source_course_key, destination_course_key, user, fields['display_name']) From 2ee9dd11db04f52f45c71a088ae584ee700d1432 Mon Sep 17 00:00:00 2001 From: Taylor Payne Date: Fri, 28 Aug 2026 12:10:43 -0600 Subject: [PATCH 03/16] feat: split read vs write authz checks for advanced settings (#39007) (cherry picked from commit 0310eb2173d58748e5074a1e8f73c4e1832f85c1) --- .../v0/tests/test_advanced_settings.py | 37 ++++++++++++++++++- .../v1/views/tests/test_proctoring.py | 16 ++++++++ common/djangoapps/student/auth.py | 16 ++++++-- 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v0/tests/test_advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_advanced_settings.py index 8c6392109eb2..636b947a57f3 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/tests/test_advanced_settings.py +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_advanced_settings.py @@ -11,7 +11,7 @@ from django.urls import reverse from milestones.tests.utils import MilestonesTestCaseMixin from openedx_authz.api.users import assign_role_to_user_in_scope -from openedx_authz.constants.roles import COURSE_STAFF +from openedx_authz.constants.roles import COURSE_AUDITOR, COURSE_EDITOR, COURSE_STAFF from openedx_authz.engine.enforcer import AuthzEnforcer from openedx_authz.engine.utils import migrate_policy_between_enforcers from rest_framework.test import APIClient @@ -103,6 +103,7 @@ def test_disabled_fetch_all_query_param(self, setting, excluded_field): assert excluded_field not in resp.data +@ddt.ddt @patch.object(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, 'is_enabled', return_value=True) class AdvancedSettingsAuthzTest(CourseTestCase): """ @@ -203,3 +204,37 @@ def test_patch_unauthorized_for_specific_course(self, mock_flag): content_type="application/json" ) self.assertEqual(response.status_code, 403) # noqa: PT009 + + @ddt.data(COURSE_EDITOR, COURSE_AUDITOR) + def test_view_only_role_can_read(self, role, mock_flag): + """Course editor/auditor (view-only for advanced settings) can GET.""" + view_only_user = UserFactory() + assign_role_to_user_in_scope( + view_only_user.username, + role.external_key, + str(self.course.id), + ) + + view_only_client = APIClient() + view_only_client.force_authenticate(user=view_only_user) + response = view_only_client.get(self.url) + assert response.status_code == 200 + + @ddt.data(COURSE_EDITOR, COURSE_AUDITOR) + def test_view_only_role_cannot_write(self, role, mock_flag): + """Course editor/auditor cannot PATCH advanced settings (requires manage permission).""" + view_only_user = UserFactory() + assign_role_to_user_in_scope( + view_only_user.username, + role.external_key, + str(self.course.id), + ) + + view_only_client = APIClient() + view_only_client.force_authenticate(user=view_only_user) + response = view_only_client.patch( + self.url, + {"display_name": {"value": "Test"}}, + content_type="application/json", + ) + assert response.status_code == 403 diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py index 79f65c7f5dd3..c2e18a7cd41b 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py @@ -8,6 +8,7 @@ from django.test.utils import override_settings from django.urls import reverse from edx_toggles.toggles.testutils import override_waffle_flag +from openedx_authz.constants.permissions import COURSES_VIEW_ADVANCED_SETTINGS from rest_framework import status from rest_framework.test import APITestCase @@ -489,3 +490,18 @@ def test_authz_with_disable_advanced_settings_non_staff_denied(self, mock_is_use response = self.non_staff_client.get(self.url) self.assertEqual(response.status_code, 403) # noqa: PT009 mock_is_user_allowed.assert_not_called() + + @patch.object(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, 'is_enabled', return_value=True) + @patch('common.djangoapps.student.auth.authz_api.is_user_allowed') + def test_authz_feature_restricted_checks_view_permission(self, mock_is_user_allowed, mock_flag): + """ + The proctoring errors endpoint uses the 'feature_restricted' access type, which should + check the view (read) permission so that view-only roles (editor/auditor) can access it. + """ + mock_is_user_allowed.return_value = True + response = self.non_staff_client.get(self.url) + assert response.status_code == 200 + mock_is_user_allowed.assert_called_once() + # The permission identifier is the second positional arg to is_user_allowed. + called_permission = mock_is_user_allowed.call_args[0][1] + assert called_permission == COURSES_VIEW_ADVANCED_SETTINGS.identifier diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index d4b7ec7f97ba..e4b66e95db00 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -11,7 +11,10 @@ from django.core.exceptions import PermissionDenied from opaque_keys.edx.locator import LibraryLocator from openedx_authz import api as authz_api -from openedx_authz.constants.permissions import COURSES_MANAGE_ADVANCED_SETTINGS +from openedx_authz.constants.permissions import ( + COURSES_MANAGE_ADVANCED_SETTINGS, + COURSES_VIEW_ADVANCED_SETTINGS, +) from common.djangoapps.student.roles import ( CourseBetaTesterRole, @@ -209,8 +212,15 @@ def check_course_advanced_settings_access(user, course_key, access_type='read'): ): # When feature is disabled, only staff/superuser can access (bypass authz) return user.is_staff or user.is_superuser - # Otherwise check authz permission - return authz_api.is_user_allowed(user.username, COURSES_MANAGE_ADVANCED_SETTINGS.identifier, str(course_key)) + + # Read and feature_restricted require only the view permission; + # write operations require the manage permission. + if access_type == 'write': + permission = COURSES_MANAGE_ADVANCED_SETTINGS.identifier + else: + permission = COURSES_VIEW_ADVANCED_SETTINGS.identifier + + return authz_api.is_user_allowed(user.username, permission, str(course_key)) # Legacy permission checks if access_type == 'read': From 780ca83c29fb1d0265e8c132e3bb72683467fde6 Mon Sep 17 00:00:00 2001 From: Taylor Payne Date: Fri, 28 Aug 2026 12:11:18 -0600 Subject: [PATCH 04/16] feat: split read vs write authz checks for certificates (#39008) (cherry picked from commit b13ef66cc0c08e6817673735bcbc401c8db4d835) --- .../rest_api/v1/serializers/certificates.py | 1 + .../rest_api/v1/views/certificates.py | 17 ++++++--- .../v1/views/tests/test_certificates.py | 35 +++++++++++++------ 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/certificates.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/certificates.py index 9e536efa4550..32e8c16f1d1f 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/certificates.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/certificates.py @@ -50,3 +50,4 @@ class CourseCertificatesSerializer(serializers.Serializer): course_number = serializers.CharField(source="context_course.number") course_title = serializers.CharField(source="context_course.display_name_with_default") course_number_override = serializers.CharField(source="context_course.display_coursenumber") + can_manage = serializers.BooleanField(default=False, read_only=True) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/certificates.py b/cms/djangoapps/contentstore/rest_api/v1/views/certificates.py index 43069553a067..adb835d30c2c 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/certificates.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/certificates.py @@ -2,7 +2,7 @@ import edx_api_doc_tools as apidocs from opaque_keys.edx.keys import CourseKey -from openedx_authz.constants.permissions import COURSES_MANAGE_CERTIFICATES +from openedx_authz.constants.permissions import COURSES_MANAGE_CERTIFICATES, COURSES_VIEW_CERTIFICATES from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView @@ -85,7 +85,8 @@ def get(self, request: Request, course_id: str): "mfe_proctored_exam_settings_url": "", "course_number": "DemoX", "course_title": "Demonstration Course", - "course_number_override": "Course Number Display String" + "course_number_override": "Course Number Display String", + "can_manage": true } ``` """ @@ -94,14 +95,22 @@ def get(self, request: Request, course_id: str): if not user_has_course_permission( request.user, - COURSES_MANAGE_CERTIFICATES.identifier, + COURSES_VIEW_CERTIFICATES.identifier, course_key, - LegacyAuthoringPermission.WRITE + LegacyAuthoringPermission.READ ): self.permission_denied(request) + can_manage = user_has_course_permission( + request.user, + COURSES_MANAGE_CERTIFICATES.identifier, + course_key, + LegacyAuthoringPermission.WRITE + ) + with store.bulk_operations(course_key): course = modulestore().get_course(course_key) certificates_context = get_certificates_context(course, request.user) + certificates_context['can_manage'] = can_manage serializer = CourseCertificatesSerializer(certificates_context) return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_certificates.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_certificates.py index 9ae3ba84d8bc..8c0e0e79b4d2 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_certificates.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_certificates.py @@ -1,8 +1,9 @@ """ Unit tests for the course's certificate. """ +import ddt from django.urls import reverse -from openedx_authz.constants.roles import COURSE_EDITOR, COURSE_STAFF +from openedx_authz.constants.roles import COURSE_AUDITOR, COURSE_EDITOR, COURSE_STAFF from rest_framework import status from cms.djangoapps.contentstore.tests.utils import CourseTestCase @@ -40,6 +41,7 @@ def test_success_response(self): self.assertEqual(response_data["course_number"], self.course.number) # noqa: PT009 +@ddt.ddt class CourseCertificatesAuthzViewTest( CourseAuthoringAuthzTestMixin, CourseTestCase, PermissionAccessMixin, HelperMethods ): @@ -59,14 +61,27 @@ def test_authorized_user_can_access(self): self._add_course_certificates(count=2, signatory_count=2) self.add_user_to_role_in_course(self.authorized_user, COURSE_STAFF.external_key, self.course.id) resp = self.authorized_client.get(self.url) - self.assertEqual(resp.status_code, status.HTTP_200_OK) # noqa: PT009 + assert resp.status_code == status.HTTP_200_OK - def test_non_staff_user_cannot_access(self): - """ - User without permissions should be denied. - This case validates that a non-staff user cannot access. - """ - self._add_course_certificates(count=2, signatory_count=2) - self.add_user_to_role_in_course(self.authorized_user, COURSE_EDITOR.external_key, self.course.id) + def test_staff_role_has_can_manage_true(self): + """User with COURSE_STAFF role gets can_manage=True in response.""" + self._add_course_certificates(count=1, signatory_count=1) + self.add_user_to_role_in_course(self.authorized_user, COURSE_STAFF.external_key, self.course.id) resp = self.authorized_client.get(self.url) - self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) # noqa: PT009 + assert resp.status_code == status.HTTP_200_OK + assert resp.data["can_manage"] is True + + @ddt.data(COURSE_EDITOR, COURSE_AUDITOR) + def test_view_only_role_can_view_without_manage(self, role): + """Course editor/auditor can view certificates but gets can_manage=False.""" + self._add_course_certificates(count=1, signatory_count=1) + self.add_user_to_role_in_course(self.authorized_user, role.external_key, self.course.id) + resp = self.authorized_client.get(self.url) + assert resp.status_code == status.HTTP_200_OK + assert resp.data["can_manage"] is False + + def test_unauthorized_user_cannot_access(self): + """User without any role cannot access.""" + self._add_course_certificates(count=1, signatory_count=1) + resp = self.unauthorized_client.get(self.url) + assert resp.status_code == status.HTTP_403_FORBIDDEN From facdfeb0422fe1b025217d9ad3a68107271fa4f0 Mon Sep 17 00:00:00 2001 From: KEVYN SUAREZ <91025555+efortish@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:23:05 -0500 Subject: [PATCH 05/16] fix: Course Auditor gets 403 navigating to a course unit (#38986) (cherry picked from commit 7510e57aeaf8ba708c55b082e643cbe1ea46b0bb) --- .../v1/views/tests/test_vertical_block.py | 37 ++++++++ cms/djangoapps/contentstore/views/block.py | 21 +++- .../contentstore/views/component.py | 16 +++- .../contentstore/views/tests/test_block.py | 95 +++++++++++++++++++ 4 files changed, 164 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py index 260ed4732320..83b47b638e6b 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py @@ -4,12 +4,16 @@ from urllib.parse import quote +import ddt from django.urls import reverse from edx_toggles.toggles.testutils import override_waffle_flag +from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_AUDITOR, COURSE_EDITOR, COURSE_STAFF from rest_framework import status from xblock.validation import ValidationMessage from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin from openedx.core.djangoapps.content_libraries.tests import ContentLibrariesRestApiTest from openedx.core.djangoapps.content_tagging.toggles import DISABLE_TAGGING_FEATURE from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order @@ -201,6 +205,39 @@ def test_not_valid_usage_key_string(self): self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) # noqa: PT009 +@ddt.ddt +class ContainerHandlerViewAuthzTest(CourseAuthoringAuthzTestMixin, BaseXBlockContainer): + """ + Regression test for openedx-authz#384: ContainerHandlerView (the endpoint the + Authoring MFE's unit page calls to render a unit) required legacy write access + via _get_item_in_course(), so AuthZ-native roles with no legacy equivalent + (course_auditor, course_editor) got a 403 despite holding COURSES_VIEW_COURSE. + """ + + view_name = "container_handler" + + @ddt.data( + COURSE_STAFF.external_key, + COURSE_ADMIN.external_key, + COURSE_AUDITOR.external_key, + COURSE_EDITOR.external_key, + ) + def test_course_roles_can_view_unit_container(self, role_key): + role_user = UserFactory(password=self.password) + self.add_user_to_role_in_course(role_user, role_key, self.course.id) + + self.client.login(username=role_user.username, password=self.password) + response = self.client.get(self.get_reverse_url(self.vertical.location)) + + assert response.status_code == status.HTTP_200_OK + + def test_unauthorized_user_gets_permission_denied(self): + self.client.login(username=self.unauthorized_user.username, password=self.password) + response = self.client.get(self.get_reverse_url(self.vertical.location)) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + class ContainerVerticalViewTest(BaseXBlockContainer): """ Unit tests for the ContainerVerticalViewTest. diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index 4d60640cb2cf..ae9005a546f4 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -142,7 +142,12 @@ def xblock_view_handler(request, usage_key_string, view_name): the second is the resource description """ usage_key = usage_key_with_run(usage_key_string) - if not has_studio_read_access(request.user, usage_key.course_key): + if not user_has_course_permission( + request.user, + COURSES_VIEW_COURSE.identifier, + usage_key.course_key, + LegacyAuthoringPermission.READ, + ): raise PermissionDenied() accept_header = request.META.get("HTTP_ACCEPT", "application/json") @@ -299,7 +304,12 @@ def xblock_edit_view(request, usage_key_string): Allows editing of an XBlock specified by the usage key. """ usage_key = usage_key_with_run(usage_key_string) - if not has_studio_read_access(request.user, usage_key.course_key): + if not user_has_course_permission( + request.user, + COURSES_VIEW_COURSE.identifier, + usage_key.course_key, + LegacyAuthoringPermission.READ, + ): raise PermissionDenied() store = modulestore() @@ -371,7 +381,12 @@ def xblock_container_handler(request, usage_key_string): """ usage_key = usage_key_with_run(usage_key_string) - if not has_studio_read_access(request.user, usage_key.course_key): + if not user_has_course_permission( + request.user, + COURSES_VIEW_COURSE.identifier, + usage_key.course_key, + LegacyAuthoringPermission.READ, + ): raise PermissionDenied() response_format = request.GET.get("format", "html") diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 880cb9057795..eba16ad6ac38 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -15,6 +15,7 @@ from django.views.decorators.http import require_GET from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey +from openedx_authz.constants.permissions import COURSES_VIEW_COURSE from xblock.core import XBlock from xblock.django.request import django_to_webob_request, webob_to_django_response from xblock.exceptions import NoSuchHandlerError @@ -28,6 +29,8 @@ from common.djangoapps.student.auth import has_course_author_access from common.djangoapps.xblock_django.api import authorable_xblocks, disabled_xblocks from common.djangoapps.xblock_django.models import XBlockStudioConfigurationFlag +from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission +from openedx.core.djangoapps.authz.decorators import user_has_course_permission from openedx.core.djangoapps.content_tagging.api import get_object_tags from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration from openedx.core.lib.xblock_utils import get_aside_from_xblock, is_xblock_aside @@ -527,7 +530,11 @@ def _get_item_in_course(request, usage_key): Helper method for getting the old location, containing course, item, lms_link, and preview_lms_link for a given locator. - Verifies that the caller has permission to access this item. + Verifies that the caller has permission to view this item. All current callers + (container_handler, container_embed_handler, xblock_edit_view, and the REST API v1 + ContainerHandlerView) are read-only, so this only requires view access, not write + access — actual mutations are gated separately (e.g. component_handler's own + has_course_author_access check before persisting). """ from ..utils import get_lms_link_for_item @@ -537,7 +544,12 @@ def _get_item_in_course(request, usage_key): course_key = usage_key.course_key - if not has_course_author_access(request.user, course_key): + if not user_has_course_permission( + request.user, + COURSES_VIEW_COURSE.identifier, + course_key, + LegacyAuthoringPermission.READ, + ): raise PermissionDenied() course = modulestore().get_course(course_key) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 15468c63bd9f..6884dd86ab19 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -3628,6 +3628,101 @@ def test_unauthorized_chapter_outline(self): assert resp.status_code == 403 +@ddt.ddt +class TestXBlockContainerAndViewHandlerAuthz(CourseAuthoringAuthzTestMixin, ItemTest): + """ + Unit tests for xblock_container_handler and xblock_view_handler authorization. + + Regression test for openedx-authz#384: navigating to a unit (the container page, + which in turn renders each child block via the view handler) returned 403 for + roles like course_auditor that have no legacy role equivalent, because these two + handlers checked the legacy-only has_studio_read_access instead of the AuthZ-aware + user_has_course_permission already used by xblock_outline_handler. + """ + + def setUp(self): + super().setUp() + user_id = self.user.id + self.chapter = BlockFactory.create( + parent_location=self.course.location, + category="chapter", + display_name="Week 1", + user_id=user_id, + ) + self.sequential = BlockFactory.create( + parent_location=self.chapter.location, + category="sequential", + display_name="Lesson 1", + user_id=user_id, + ) + self.vertical = BlockFactory.create( + parent_location=self.sequential.location, + category="vertical", + display_name="Unit 1", + user_id=user_id, + ) + + @ddt.data( + COURSE_STAFF.external_key, + COURSE_ADMIN.external_key, + COURSE_AUDITOR.external_key, + COURSE_EDITOR.external_key, + ) + def test_course_roles_can_view_unit_container(self, role_key): + """ + Any role with COURSES_VIEW_COURSE, including the legacy-less course_auditor + and course_editor roles, can open the unit (container) page. + """ + role_user = UserFactory(password=self.password) + self.add_user_to_role_in_course(role_user, role_key, self.course.id) + + container_url = reverse_usage_url("xblock_container_handler", self.vertical.location) + self.client.login(username=role_user.username, password=self.password) + resp = self.client.get(container_url, HTTP_ACCEPT="application/json") + + assert resp.status_code == 200 + + @ddt.data( + COURSE_STAFF.external_key, + COURSE_ADMIN.external_key, + COURSE_AUDITOR.external_key, + COURSE_EDITOR.external_key, + ) + def test_course_roles_can_render_unit_preview(self, role_key): + """ + Any role with COURSES_VIEW_COURSE can render the unit's preview fragment, + which is what the frontend fetches for each block shown on the unit page. + """ + role_user = UserFactory(password=self.password) + self.add_user_to_role_in_course(role_user, role_key, self.course.id) + + preview_url = reverse_usage_url( + "xblock_view_handler", self.vertical.location, {"view_name": "container_preview"} + ) + self.client.login(username=role_user.username, password=self.password) + resp = self.client.get(preview_url, HTTP_ACCEPT="application/json") + + assert resp.status_code == 200 + + def test_unauthorized_user_gets_permission_denied_on_unit_container(self): + container_url = reverse_usage_url("xblock_container_handler", self.vertical.location) + + self.client.login(username=self.unauthorized_user.username, password=self.password) + resp = self.client.get(container_url, HTTP_ACCEPT="application/json") + + assert resp.status_code == 403 + + def test_unauthorized_user_gets_permission_denied_on_unit_preview(self): + preview_url = reverse_usage_url( + "xblock_view_handler", self.vertical.location, {"view_name": "container_preview"} + ) + + self.client.login(username=self.unauthorized_user.username, password=self.password) + resp = self.client.get(preview_url, HTTP_ACCEPT="application/json") + + assert resp.status_code == 403 + + class TestGetMetadataWithProblemDefaults(ModuleStoreTestCase): """ Unit tests for _get_metadata_with_problem_defaults. From b219bf373a18b293e531c691dba6209de926ab9f Mon Sep 17 00:00:00 2001 From: KEVYN SUAREZ <91025555+efortish@users.noreply.github.com> Date: Thu, 3 Sep 2026 07:35:43 -0500 Subject: [PATCH 06/16] fix: RoleCache legacy compat layer ignores platform-wide glob role assignments (#38984) (cherry picked from commit 05fa2e1fc0bff787924dd549f844564e37062446) --- common/djangoapps/student/roles.py | 89 +++++++++++-------- common/djangoapps/student/tests/test_roles.py | 59 ++++++++++++ 2 files changed, 110 insertions(+), 38 deletions(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index dc942dec022c..4b0dae64f6b9 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -15,8 +15,14 @@ from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import CourseLocator from openedx_authz.api import users as authz_api -from openedx_authz.api.data import CourseOverviewData, OrgCourseOverviewGlobData, RoleAssignmentData +from openedx_authz.api.data import ( + CourseOverviewData, + OrgCourseOverviewGlobData, + PlatformCourseOverviewGlobData, + RoleAssignmentData, +) from openedx_authz.constants import roles as authz_roles +from organizations.api import get_organizations from common.djangoapps.student.models import CourseAccessRole from common.djangoapps.student.signals.signals import emit_course_access_role_added, emit_course_access_role_removed @@ -159,44 +165,54 @@ class AuthzCompatCourseAccessRole: role: str -def _get_org_and_course_id_from_authz_scope( - scope: CourseOverviewData | OrgCourseOverviewGlobData, -) -> tuple[str, str | None] | None: +def _get_orgs_and_course_ids_from_authz_scope( + scope: CourseOverviewData | OrgCourseOverviewGlobData | PlatformCourseOverviewGlobData, +) -> list[tuple[str, str | None]]: """ - Extract the org and course key from an AuthZ course assignment scope. + Extract the (org, course_id) pairs an AuthZ course assignment scope maps to. - Course-scoped assignments return ``(org, course_external_key)``. - Org-wide assignments return ``(org, None)``. + Course-scoped assignments map to a single ``(org, course_external_key)`` pair. + Org-wide assignments map to a single ``(org, None)`` pair. - Returns ``None`` when the org cannot be determined. For org-wide scopes, - ``OrgGlobData.org`` is typed as ``str | None`` because it is parsed from - ``external_key`` and returns ``None`` for malformed glob patterns. + Platform-wide assignments (``course-v1:*``) apply to every org, not just one, so + they map to ``(org, None)`` for *every registered org* — the same shape an org-wide + grant already produces, just repeated per org. This lets a platform-wide grant be + picked up by the existing OrgRole-based legacy checks (e.g. ``has_staff_roles``, + ``get_user_permissions``, which already check org-level and course-level access + separately) with no changes to ``RoleCache``/``OrgRole``/``CourseRole``. + + Returns an empty list when the scope type isn't one of the above (e.g. a content + library scope). """ if isinstance(scope, CourseOverviewData): course_id = scope.external_key - return get_org_from_key(course_id), course_id + return [(get_org_from_key(course_id), course_id)] + if isinstance(scope, PlatformCourseOverviewGlobData): + return [(org["short_name"], None) for org in get_organizations()] if isinstance(scope, OrgCourseOverviewGlobData): - return scope.org, None - return None + return [(scope.org, None)] + return [] def authz_get_all_course_assignments_for_user(user: User) -> list[RoleAssignmentData]: """ Return AuthZ role assignments for a user that apply to courses. - Includes assignments scoped to a specific course (``CourseOverviewData``) and - assignments scoped to all courses in an organization (``OrgCourseOverviewGlobData``). - Assignments for other resource types, such as content libraries, are excluded. + Includes assignments scoped to a specific course (``CourseOverviewData``), to all + courses in an organization (``OrgCourseOverviewGlobData``), and to all courses on + the platform (``PlatformCourseOverviewGlobData``). Assignments for other resource + types, such as content libraries, are excluded. Args: user (User): The user whose AuthZ role assignments should be retrieved. Returns: - list[RoleAssignmentData]: Role assignments whose scope is course-level or org-wide + list[RoleAssignmentData]: Role assignments whose scope is course-level, + org-wide, or platform-wide. """ return authz_api.get_user_role_assignments_per_scope_type( user_external_key=user.username, - scope_types=(CourseOverviewData, OrgCourseOverviewGlobData), + scope_types=(CourseOverviewData, OrgCourseOverviewGlobData, PlatformCourseOverviewGlobData), ) @@ -207,9 +223,10 @@ def _compat_roles_from_authz_assignment( """ Convert an AuthZ role assignment into legacy-compatible course access roles. - Course-scoped assignments produce roles tied to a specific course key. - Org-wide assignments produce org-level roles with no course key (``course_id`` - is ``None``), matching legacy ``OrgStaffRole`` / ``OrgInstructorRole`` behavior. + Course-scoped assignments produce roles tied to a specific course key. Org-wide + and platform-wide assignments produce org-level roles with no course key + (``course_id`` is ``None``), matching legacy ``OrgStaffRole`` / ``OrgInstructorRole`` + behavior — a platform-wide assignment produces one such role per registered org. AuthZ roles without a legacy mapping are skipped. Args: @@ -222,25 +239,21 @@ def _compat_roles_from_authz_assignment( assignment. Returns an empty set if the org cannot be determined from the scope or no roles could be mapped. """ - org_and_course_id = _get_org_and_course_id_from_authz_scope(assignment.scope) - if org_and_course_id is None: - return set() - org, course_id = org_and_course_id - compat_roles = set() - for role in assignment.roles: - legacy_role = get_legacy_role_from_authz_role(authz_role=role.external_key) - if legacy_role is None: - continue - compat_roles.add( - AuthzCompatCourseAccessRole( - user_id=user.id, - username=user.username, - org=org, - course_id=course_id, - role=legacy_role, + for org, course_id in _get_orgs_and_course_ids_from_authz_scope(assignment.scope): + for role in assignment.roles: + legacy_role = get_legacy_role_from_authz_role(authz_role=role.external_key) + if legacy_role is None: + continue + compat_roles.add( + AuthzCompatCourseAccessRole( + user_id=user.id, + username=user.username, + org=org, + course_id=course_id, + role=legacy_role, + ) ) - ) return compat_roles diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index a6f5c7aa27b2..d3d734baa6d0 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -14,13 +14,16 @@ ContentLibraryData, CourseOverviewData, OrgCourseOverviewGlobData, + PlatformCourseOverviewGlobData, RoleAssignmentData, RoleData, ScopeData, UserData, ) +from openedx_authz.api.users import assign_role_to_user_in_scope from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_STAFF from openedx_authz.engine.enforcer import AuthzEnforcer +from organizations.api import add_organization from common.djangoapps.student.admin import CourseAccessRoleHistoryAdmin from common.djangoapps.student.models import CourseAccessRoleHistory, User @@ -380,6 +383,62 @@ def test_org_scope_authz_role_grants_instructor_dashboard_permissions(self): self.assertTrue(self.student.has_perm(instructor_permissions.VIEW_DASHBOARD, course_key)) # noqa: PT009 self.assertTrue(self.student.has_perm(instructor_permissions.SHOW_TASKS, course_key)) # noqa: PT009 + def test_get_authz_compat_course_access_roles_for_user_platform_glob(self): + """ + A platform-wide (course-v1:*) AuthZ assignment should map to one legacy + org-level course access role per registered org, since it applies to all of them. + """ + for org in self.orgs: + add_organization({"name": org, "short_name": org, "description": ""}) + + assign_role_to_user_in_scope( + self.student.username, + COURSE_ADMIN.external_key, + PlatformCourseOverviewGlobData.build_external_key(), + ) + AuthzEnforcer.get_enforcer().load_policy() + + result = get_authz_compat_course_access_roles_for_user(self.student) + + assert result == { + AuthzCompatCourseAccessRole( + user_id=self.student.id, + username=self.student.username, + org=org, + course_id=None, + role="instructor", + ) + for org in self.orgs + } + + def test_platform_glob_authz_role_grants_instructor_dashboard_permissions(self): + """ + A platform-wide (course-v1:*) AuthZ course_admin should grant legacy instructor + access for courses in *any* org, the same way an org-wide grant does for its org. + """ + # pylint: disable=protected-access + for org in self.orgs: + add_organization({"name": org, "short_name": org, "description": ""}) + marvel_course_key = CourseKey.from_string(f"course-v1:{self.orgs[0]}+DemoX+DemoCourse") + dc_course_key = CourseKey.from_string(f"course-v1:{self.orgs[1]}+DemoX+DemoCourse") + + assign_role_to_user_in_scope( + self.student.username, + COURSE_ADMIN.external_key, + PlatformCourseOverviewGlobData.build_external_key(), + ) + AuthzEnforcer.get_enforcer().load_policy() + + if hasattr(self.student, "_roles"): + del self.student._roles + self.student._roles = RoleCache(self.student) + + for org in self.orgs: + assert self.student._roles.has_role("instructor", None, org) + assert OrgInstructorRole(org).has_user(self.student) + assert self.student.has_perm(instructor_permissions.VIEW_DASHBOARD, marvel_course_key) + assert self.student.has_perm(instructor_permissions.VIEW_DASHBOARD, dc_course_key) + @ddt.ddt class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-class-docstring From 81820806a2244f148c294892f600bf2549f7692f Mon Sep 17 00:00:00 2001 From: KEVYN SUAREZ <91025555+efortish@users.noreply.github.com> Date: Tue, 8 Sep 2026 05:19:37 -0500 Subject: [PATCH 07/16] fix: platform-wide glob role assignments crash get_orgs_for_user/has_org_for_user (#38980) (cherry picked from commit 8f802e8ffdfc2e69de12b201d0e65ed2cbaee798) --- .../contentstore/tests/test_course_listing.py | 12 ++-- common/djangoapps/student/roles.py | 5 ++ common/djangoapps/student/tests/test_roles.py | 56 +++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 47865d326cbd..21e9d1817130 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -936,7 +936,9 @@ def test_get_course_keys_from_scopes_with_platform_scope(self): "is_enabled", side_effect=self._mock_authz_toggle(enabled_keys), ): - course_keys = _get_course_keys_from_scopes([PlatformCourseOverviewGlobData(external_key="course-v1:*")]) + course_keys = _get_course_keys_from_scopes([ + PlatformCourseOverviewGlobData(external_key=PlatformCourseOverviewGlobData.build_external_key()) + ]) assert course_keys == set(authz_keys) | set(legacy_keys) @@ -953,7 +955,9 @@ def test_get_course_keys_from_scopes_with_platform_scope_global_flag_enabled(sel "is_enabled", side_effect=self._mock_authz_toggle(enabled_keys, global_enabled=True), ): - course_keys = _get_course_keys_from_scopes([PlatformCourseOverviewGlobData(external_key="course-v1:*")]) + course_keys = _get_course_keys_from_scopes([ + PlatformCourseOverviewGlobData(external_key=PlatformCourseOverviewGlobData.build_external_key()) + ]) assert course_keys == set(CourseOverview.get_all_courses().values_list("id", flat=True)) @@ -972,8 +976,8 @@ def test_get_course_keys_from_scopes_platform_scope_short_circuits(self): ): course_keys = _get_course_keys_from_scopes( [ - OrgCourseOverviewGlobData(external_key="course-v1:Org1+*"), - PlatformCourseOverviewGlobData(external_key="course-v1:*"), + OrgCourseOverviewGlobData(external_key=OrgCourseOverviewGlobData.build_external_key("Org1")), + PlatformCourseOverviewGlobData(external_key=PlatformCourseOverviewGlobData.build_external_key()), ] ) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 4b0dae64f6b9..4ea2f577b5e2 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -645,6 +645,11 @@ def _authz_get_orgs_for_user(self, user) -> list[str]: user_external_key=user.username, role_external_key=role, ) + # A platform-wide grant (course-v1:*, lib:*) covers every org, not just the ones + # with a concrete assignment. Platform-glob scopes have no .org attribute at all + # (unlike org-glob/course/library scopes, where it's a real field that can be None). + if any(assignment.scope.IS_PLATFORM_GLOB for assignment in assignments): + return [org["short_name"] for org in get_organizations()] orgs = {assignment.scope.org for assignment in assignments if assignment.scope.org is not None} return list(orgs) diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index d3d734baa6d0..172a153d3cde 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -24,6 +24,7 @@ from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_STAFF from openedx_authz.engine.enforcer import AuthzEnforcer from organizations.api import add_organization +from organizations.tests.factories import OrganizationFactory from common.djangoapps.student.admin import CourseAccessRoleHistoryAdmin from common.djangoapps.student.models import CourseAccessRoleHistory, User @@ -316,6 +317,61 @@ def test_get_orgs_for_user_authz(self): result = role.get_orgs_for_user(self.student) self.assertCountEqual(result, [self.course_key.org, other_org]) # noqa: PT009 + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + def test_get_orgs_for_user_authz_platform_glob(self): + """ + A platform-wide glob assignment (course-v1:*) has no `.org` attribute, unlike + course/org-glob scopes. get_orgs_for_user must special-case it and return every + registered org instead of crashing with an AttributeError. + """ + role = CourseStaffRole(self.course_key) + + for org in self.orgs: + OrganizationFactory(short_name=org, name=org) + + assign_role_to_user_in_scope( + self.student.username, + COURSE_STAFF.external_key, + PlatformCourseOverviewGlobData.build_external_key(), + ) + AuthzEnforcer.get_enforcer().load_policy() + + result = role.get_orgs_for_user(self.student) + assert sorted(result) == sorted(self.orgs) + assert role.has_org_for_user(self.student) + assert role.has_org_for_user(self.student, org=self.orgs[0]) + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + def test_get_orgs_for_user_authz_platform_glob_vs_org_scoped(self): + """ + Side-by-side check that the platform-glob branch (return every registered org) + and the regular branch (return only the orgs with a concrete assignment) produce + the same list[str] shape, over the same pool of registered orgs: an org-scoped + grant returns a subset, a platform-wide grant returns all of them. + """ + role = CourseStaffRole(self.course_key) + third_org = "Universal" + all_orgs = [*self.orgs, third_org] + + for org in all_orgs: + OrganizationFactory(short_name=org, name=org) + + subset_user = UserFactory() + assign_role_to_user_in_scope( + subset_user.username, + COURSE_STAFF.external_key, + OrgCourseOverviewGlobData.build_external_key(self.orgs[0]), + ) + assign_role_to_user_in_scope( + self.student.username, + COURSE_STAFF.external_key, + PlatformCourseOverviewGlobData.build_external_key(), + ) + AuthzEnforcer.get_enforcer().load_policy() + + assert sorted(role.get_orgs_for_user(subset_user)) == [self.orgs[0]] + assert sorted(role.get_orgs_for_user(self.student)) == sorted(all_orgs) + def test_get_authz_compat_course_access_roles_for_user(self): """ Test that get_authz_compat_course_access_roles_for_user doesn't crash when the user From 47dd8eb90ec6bed66571740ae9d129d3ec63f62e Mon Sep 17 00:00:00 2001 From: KEVYN SUAREZ <91025555+efortish@users.noreply.github.com> Date: Thu, 10 Sep 2026 09:23:41 -0500 Subject: [PATCH 08/16] fix: course editor gets 403 copying a unit to the clipboard (#39075) (cherry picked from commit fef9dd5e6ae9ee05b62b0d0c68f09c79e668a583) --- .../content_staging/tests/test_clipboard.py | 41 +++++++++++++++++++ .../core/djangoapps/content_staging/views.py | 11 ++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py b/openedx/core/djangoapps/content_staging/tests/test_clipboard.py index 4c26f2c97cca..083125870113 100644 --- a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py +++ b/openedx/core/djangoapps/content_staging/tests/test_clipboard.py @@ -6,8 +6,12 @@ from typing import cast from xml.etree import ElementTree +import ddt +from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_AUDITOR, COURSE_EDITOR, COURSE_STAFF from rest_framework.test import APIClient +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin from openedx.core.djangoapps.content_staging import api as python_api from xmodule.contentstore.django import contentstore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course @@ -378,3 +382,40 @@ def assertXmlEqual(self, xml_str_a: str, xml_str_b: str) -> None: a = ElementTree.canonicalize(xml_str_a, strip_text=True) b = ElementTree.canonicalize(xml_str_b, strip_text=True) assert a == b + + +@ddt.ddt +class ClipboardAuthzTest(CourseAuthoringAuthzTestMixin, ModuleStoreTestCase): + """ + Regression test for openedx-authz#403: ClipboardEndpoint.post() required legacy read + access via has_studio_read_access(), so AuthZ-native roles with no legacy equivalent + (course_auditor, course_editor) got a 403 when copying a unit to the clipboard despite + holding COURSES_VIEW_COURSE. + """ + + @ddt.data( + COURSE_STAFF.external_key, + COURSE_ADMIN.external_key, + COURSE_AUDITOR.external_key, + COURSE_EDITOR.external_key, + ) + def test_course_roles_can_copy_unit_to_clipboard(self, role_key): + course_key = ToyCourseFactory.create().id + html_key = course_key.make_usage_key("html", "toyhtml") + + role_user = UserFactory(password=self.password) + self.add_user_to_role_in_course(role_user, role_key, course_key) + + client = APIClient() + client.force_authenticate(user=role_user) + response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(html_key)}, format="json") + + assert response.status_code == 200 + + def test_unauthorized_user_gets_permission_denied(self): + course_key = ToyCourseFactory.create().id + html_key = course_key.make_usage_key("html", "toyhtml") + + with self.allow_transaction_exception(): + response = self.unauthorized_client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(html_key)}, format="json") + assert response.status_code == 403 diff --git a/openedx/core/djangoapps/content_staging/views.py b/openedx/core/djangoapps/content_staging/views.py index b7cf60b38c51..7071105aec05 100644 --- a/openedx/core/djangoapps/content_staging/views.py +++ b/openedx/core/djangoapps/content_staging/views.py @@ -12,11 +12,13 @@ from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import CourseLocator, LibraryLocatorV2 from openedx_authz.constants import permissions as authz_permissions +from openedx_authz.constants.permissions import COURSES_VIEW_COURSE from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError from rest_framework.response import Response from rest_framework.views import APIView -from common.djangoapps.student.auth import has_studio_read_access +from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission +from openedx.core.djangoapps.authz.decorators import user_has_course_permission from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.lib.api.view_utils import view_auth_classes from xmodule.modulestore.django import modulestore @@ -102,7 +104,12 @@ def post(self, request): try: if isinstance(course_key, CourseLocator): # Make sure the user has permission on that course - if not has_studio_read_access(request.user, course_key): + if not user_has_course_permission( + request.user, + COURSES_VIEW_COURSE.identifier, + course_key, + LegacyAuthoringPermission.READ, + ): raise PermissionDenied( "You must be a member of the course team in Studio to export OLX using this API." ) From 6d1ad8483e31166ac062e85b1157e6a2b0a1b59b Mon Sep 17 00:00:00 2001 From: Taylor Payne Date: Mon, 7 Sep 2026 06:41:43 -0600 Subject: [PATCH 09/16] feat: split read vs write authz checks for library updates (#39009) (cherry picked from commit 34e4d6dc3d43bd8989961854d82f7700c8d6ef92) --- .../rest_api/v2/views/downstreams.py | 33 ++++++-- .../v2/views/tests/test_downstreams.py | 76 ++++++++++++++++++- 2 files changed, 101 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 8a669a782ed6..66cdb67b9192 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -90,7 +90,10 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 -from openedx_authz.constants.permissions import COURSES_VIEW_COURSE +from openedx_authz.constants.permissions import ( + COURSES_MANAGE_LIBRARY_UPDATES, + COURSES_VIEW_LIBRARY_UPDATES, +) from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError from rest_framework.fields import BooleanField from rest_framework.request import Request @@ -115,7 +118,6 @@ ) from cms.lib.xblock.upstream_sync_block import fetch_customizable_fields_from_block from cms.lib.xblock.upstream_sync_container import fetch_customizable_fields_from_container -from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access from openedx.core.djangoapps.authz.decorators import LegacyAuthoringPermission, user_has_course_permission from openedx.core.djangoapps.content_libraries import api as lib_api from openedx.core.djangoapps.video_config.transcripts_utils import clear_transcripts @@ -198,7 +200,12 @@ def get(self, request: _AuthenticatedRequest): except InvalidKeyError as exc: raise ValidationError(detail=f"Malformed course key: {course_key_string}") from exc - if not has_studio_read_access(request.user, course_key): + if not user_has_course_permission( + request.user, + COURSES_VIEW_LIBRARY_UPDATES.identifier, + course_key, + LegacyAuthoringPermission.READ + ): raise PermissionDenied if ready_to_sync is not None: link_filter["ready_to_sync"] = BooleanField().to_internal_value(ready_to_sync) @@ -306,7 +313,7 @@ def get(self, request: _AuthenticatedRequest, course_key_string: str): if not user_has_course_permission( request.user, - COURSES_VIEW_COURSE.identifier, + COURSES_VIEW_LIBRARY_UPDATES.identifier, course_key, LegacyAuthoringPermission.READ ): @@ -568,10 +575,22 @@ def _load_accessible_block(user: User, usage_key_string: str, *, require_write_a usage_key = UsageKey.from_string(usage_key_string) except InvalidKeyError as exc: raise ValidationError(detail=f"Malformed block usage key: {usage_key_string}") from exc - if require_write_access and not has_studio_write_access(user, usage_key.context_key): - raise not_found - if not has_studio_read_access(user, usage_key.context_key): + + context_key = usage_key.context_key + if not isinstance(context_key, CourseKey): raise not_found + + if require_write_access: + if not user_has_course_permission( + user, COURSES_MANAGE_LIBRARY_UPDATES.identifier, context_key, LegacyAuthoringPermission.WRITE + ): + raise not_found + else: + if not user_has_course_permission( + user, COURSES_VIEW_LIBRARY_UPDATES.identifier, context_key, LegacyAuthoringPermission.READ + ): + raise not_found + try: block = modulestore().get_item(usage_key) except ItemNotFoundError as exc: diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index 149732437215..0f3ba29673e6 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -11,7 +11,7 @@ from freezegun import freeze_time from opaque_keys.edx.keys import ContainerKey, UsageKey from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 -from openedx_authz.constants.roles import COURSE_EDITOR +from openedx_authz.constants.roles import COURSE_AUDITOR, COURSE_EDITOR from openedx_content import models_api as content_models from organizations.models import Organization from rest_framework import status @@ -1686,3 +1686,77 @@ def test_delete_component_should_be_ready_to_sync(self): } self.assertDictEqual(data[0], expected_results) # noqa: PT009 + + +class GetDownstreamListAuthzViewTest( + CourseAuthoringAuthzTestMixin, + _BaseDownstreamViewTestMixin, + ImmediateOnCommitMixin, + SharedModuleStoreTestCase, +): + """ + AuthZ tests for: + GET /api/contentstore/v2/downstreams/?course_id=... + + Validates that view_library_updates grants read access and + manage_library_updates is required for sync operations. + """ + + def call_list_api(self, client, course_id): + return client.get("/api/contentstore/v2/downstreams/", data={"course_id": str(course_id)}) + + def call_sync_api(self, client, usage_key): + return client.post( + f"/api/contentstore/v2/downstreams/{usage_key}/sync", + content_type="application/json", + ) + + def test_editor_can_list_downstreams(self): + """Course editor (has view_library_updates) can list downstream links.""" + self.add_user_to_role_in_course( + self.authorized_user, COURSE_EDITOR.external_key, self.course.id + ) + response = self.call_list_api(self.authorized_client, self.course.id) + assert response.status_code == status.HTTP_200_OK + + def test_auditor_can_list_downstreams(self): + """Course auditor (has view_library_updates) can list downstream links.""" + self.add_user_to_role_in_course( + self.authorized_user, COURSE_AUDITOR.external_key, self.course.id + ) + response = self.call_list_api(self.authorized_client, self.course.id) + assert response.status_code == status.HTTP_200_OK + + def test_unauthorized_user_cannot_list_downstreams(self): + """User without any course role cannot list downstream links.""" + response = self.call_list_api(self.unauthorized_client, self.course.id) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_editor_can_sync_downstream(self): + """Course editor passes the manage_library_updates gate for sync. + + Note: a full 200 sync additionally requires library-level access on the + upstream, which is out of scope for this PR and tracked in + openedx-authz#419. This asserts only that the course-level write gate is + satisfied — i.e. the request is not the 404 that _load_accessible_block + raises on permission denial. + """ + self.add_user_to_role_in_course( + self.authorized_user, COURSE_EDITOR.external_key, self.course.id + ) + response = self.call_sync_api(self.authorized_client, str(self.downstream_video_key)) + assert response.status_code != status.HTTP_404_NOT_FOUND + + def test_auditor_cannot_sync_downstream(self): + """Course auditor (only view_library_updates) cannot sync a downstream block.""" + self.add_user_to_role_in_course( + self.authorized_user, COURSE_AUDITOR.external_key, self.course.id + ) + response = self.call_sync_api(self.authorized_client, str(self.downstream_video_key)) + # This 404 is permission-based, not "block missing": test_editor_can_sync_downstream + # hits the SAME downstream_video_key with a role that holds manage_library_updates and + # gets a non-404 response, so the block demonstrably exists and is reachable. The only + # variable between the two tests is the role, so the auditor's 404 is the + # _load_accessible_block permission denial (which returns 404, not 403, to avoid + # leaking block existence). + assert response.status_code == status.HTTP_404_NOT_FOUND From e2e81b8ddf5d6d0ab8960c8aa90b55e1a346a412 Mon Sep 17 00:00:00 2001 From: Carlos Marquez <125318182+carlos-marquez-wgu@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:13:49 -0600 Subject: [PATCH 10/16] fix: allow Course Admin/Staff/Editor to sync library updates into a course (#39055) (cherry picked from commit 9a257961c93998e4413037c7c189649227ab6c9a) --- .../v2/views/tests/test_downstreams.py | 57 +++++++++++++++++++ cms/lib/xblock/test/test_upstream_sync.py | 56 ++++++++++++++++++ cms/lib/xblock/upstream_sync_block.py | 21 ++++++- cms/lib/xblock/upstream_sync_container.py | 22 +++++-- 4 files changed, 151 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index 0f3ba29673e6..3b0f7d5d763c 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -1760,3 +1760,60 @@ def test_auditor_cannot_sync_downstream(self): # _load_accessible_block permission denial (which returns 404, not 403, to avoid # leaking block existence). assert response.status_code == status.HTTP_404_NOT_FOUND + + +class PostDownstreamSyncAuthzViewTest( + CourseAuthoringAuthzTestMixin, + _BaseDownstreamViewTestMixin, + ImmediateOnCommitMixin, + SharedModuleStoreTestCase, +): + """ + AuthZ tests for: + POST /api/contentstore/v2/downstreams/{usage_key}/sync + + Verifies that a user with the ``course_staff`` authz role (which includes + ``courses.manage_library_updates``) can sync a downstream container even + when the user has **no** permissions on the source library. + """ + + def call_api(self, usage_key_string): + return self.authorized_client.post( + f"/api/contentstore/v2/downstreams/{usage_key_string}/sync" + ) + + def test_course_staff_can_sync_container_without_library_access(self): + """ + A user with Course Staff role (which carries + ``courses.manage_library_updates``) should be able to sync a + downstream container from its upstream library, even when the user + has no explicit permissions on the library. + """ + # Give the user Course Staff in authz so they get manage_library_updates + from openedx_authz.constants.roles import COURSE_STAFF + self.add_user_to_role_in_course( + self.authorized_user, + COURSE_STAFF.external_key, + self.course.id, + ) + + # Confirm the user has NO explicit permissions on the library. + assert lib_api.get_library_user_permissions( + self.library_key, self.authorized_user, + ) is None + + # The downstream_unit_key is linked to a container upstream in self.library. + # The unit was updated (display_name changed + republished) in setUp, + # so it is ready to sync. + response = self.call_api(self.downstream_unit_key) + + assert response.status_code == 200, ( + f"Expected 200 but got {response.status_code}: {getattr(response, 'data', '')}" + ) + + # Same test but for a block sync instead of a container one + response = self.call_api(self.downstream_html_key) + + assert response.status_code == 200, ( + f"Expected 200 but got {response.status_code}: {getattr(response, 'data', '')}" + ) diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index 9966a1379f5b..cbae3d45c121 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -2,8 +2,10 @@ Test CMS's upstream->downstream syncing system """ import datetime +from unittest.mock import patch import ddt +from openedx_content.models_api import Unit from organizations.api import ensure_organization from organizations.models import Organization @@ -17,10 +19,12 @@ sever_upstream_link, ) from cms.lib.xblock.upstream_sync_block import fetch_customizable_fields_from_block, sync_from_upstream_block +from cms.lib.xblock.upstream_sync_container import sync_from_upstream_container from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as libs from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangoapps.xblock import api as xblock +from openedx.core.djangoapps.xblock.data import CheckPerm from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory @@ -651,3 +655,55 @@ def test_sync_keep_customizaton_option(self): # data is overridden assert downstream.data == "Upstream content V2" assert downstream.downstream_customized == ["display_name"] + + def test_load_upstream_block_legacy_does_not_bypass_library_permission(self): + """ + When AuthZ is not enabled, _load_upstream_block falls through to the + library-level CAN_READ_AS_AUTHOR check. + """ + downstream = BlockFactory.create( + category="html", parent=self.unit, upstream=str(self.upstream_key) + ) + + # Get upstream xblock before patching + real_upstream = xblock.load_block(self.upstream_key, self.user) + + with patch("openedx.core.djangoapps.xblock.api.load_block") as mock_load_block: + mock_load_block.return_value = real_upstream + sync_from_upstream_block(downstream, self.user) + + mock_load_block.assert_called_once() + _, lb_kwargs = mock_load_block.call_args + assert lb_kwargs["check_permission"] == CheckPerm.CAN_READ_AS_AUTHOR, ( + "When the course-level permission is denied, the library block " + "should be loaded with CAN_READ_AS_AUTHOR, not with check_permission=None" + ) + + def test_sync_container_legacy_does_not_bypass_library_permission(self): + """ + When AuthZ is not enabled, sync_from_upstream_container falls through + to the library-level CAN_VIEW_THIS_CONTENT_LIBRARY check. + """ + upstream_container = libs.create_container( + self.library.key, Unit, "test-container", "Test Container Title", self.user.id, + ) + libs.publish_changes(self.library.key, self.user.id) + + downstream = BlockFactory.create( + category="vertical", + parent=self.unit, + upstream=str(upstream_container.container_key), + ) + + with patch( + "cms.lib.xblock.upstream_sync_container.lib_api.require_permission_for_library_key" + ) as mock_require_perm: + sync_from_upstream_container(downstream, self.user) + + mock_require_perm.assert_called_once() + _, rp_kwargs = mock_require_perm.call_args + assert rp_kwargs.get("permission") == libs.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, ( + "When the course-level permission is denied, the container sync " + "should enforce CAN_VIEW_THIS_CONTENT_LIBRARY via " + "require_permission_for_library_key" + ) diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py index 7099080511ef..170ed15525fb 100644 --- a/cms/lib/xblock/upstream_sync_block.py +++ b/cms/lib/xblock/upstream_sync_block.py @@ -12,10 +12,13 @@ from django.core.exceptions import PermissionDenied from django.utils.translation import gettext_lazy as _ from opaque_keys.edx.locator import LibraryUsageLocatorV2 +from openedx_authz.constants.permissions import COURSES_MANAGE_LIBRARY_UPDATES from rest_framework.exceptions import NotFound from xblock.core import XBlock from xblock.fields import Scope +from openedx.core.djangoapps.authz.decorators import user_has_course_permission + from .upstream_sync import BadDownstream, BadUpstream, UpstreamLink if t.TYPE_CHECKING: @@ -94,6 +97,10 @@ def _load_upstream_block(downstream: XBlock, user: User) -> XBlock: library. This assumption may need to be relaxed in the future (see module docstring). If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + + If the user holds ``courses.manage_library_updates`` for the course that + owns ``downstream``, the library-level permission check is bypassed. + Otherwise the default ``CAN_READ_AS_AUTHOR`` check is applied. """ # We import load_block here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. from openedx.core.djangoapps.xblock.api import ( # pylint: disable=wrong-import-order @@ -101,11 +108,23 @@ def _load_upstream_block(downstream: XBlock, user: User) -> XBlock: LatestVersion, load_block, ) + + # Try course-level permission first; fall back to library-level check. + course_key = downstream.usage_key.context_key + if course_key and user_has_course_permission( + user, + COURSES_MANAGE_LIBRARY_UPDATES.identifier, + course_key, + ): + check_perm = None + else: + check_perm = CheckPerm.CAN_READ_AS_AUTHOR + try: lib_block: XBlock = load_block( LibraryUsageLocatorV2.from_string(downstream.upstream), user, - check_permission=CheckPerm.CAN_READ_AS_AUTHOR, + check_permission=check_perm, version=LatestVersion.PUBLISHED, ) except (NotFound, PermissionDenied) as exc: diff --git a/cms/lib/xblock/upstream_sync_container.py b/cms/lib/xblock/upstream_sync_container.py index d6117509d238..3148eb1ce6ee 100644 --- a/cms/lib/xblock/upstream_sync_container.py +++ b/cms/lib/xblock/upstream_sync_container.py @@ -10,8 +10,10 @@ from django.utils.translation import gettext_lazy as _ # noqa: F401 from opaque_keys.edx.locator import LibraryContainerLocator +from openedx_authz.constants.permissions import COURSES_MANAGE_LIBRARY_UPDATES from xblock.core import XBlock +from openedx.core.djangoapps.authz.decorators import user_has_course_permission from openedx.core.djangoapps.content_libraries import api as lib_api from .upstream_sync import UpstreamLink @@ -37,15 +39,27 @@ def sync_from_upstream_container( Should children be handled in here? Maybe if sync_from_upstream_block were updated to handle static assets and also save changes to modulestore. + + The library-level permission check is skipped when the user holds + ``courses.manage_library_updates`` for ``downstream``'s course (derived + from ``downstream.usage_key.context_key``). """ link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException if not isinstance(link.upstream_key, LibraryContainerLocator): raise TypeError("sync_from_upstream_container() only supports Container upstreams, not containers") - lib_api.require_permission_for_library_key( # TODO: should permissions be checked at this low level? - link.upstream_key.lib_key, + + # Try course-level permission first; fall back to library-level check. + course_key = downstream.usage_key.context_key + if not (course_key and user_has_course_permission( user, - permission=lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, - ) + COURSES_MANAGE_LIBRARY_UPDATES.identifier, + course_key, + )): + lib_api.require_permission_for_library_key( # TODO: should permissions be checked at this low level? + link.upstream_key.lib_key, + user, + permission=lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, + ) upstream_meta = lib_api.get_container(link.upstream_key) upstream_children = lib_api.get_container_children(link.upstream_key, published=True) _update_customizable_fields(upstream=upstream_meta, downstream=downstream, only_fetch=False) From 8e3dab5a33938098a06434c15ace90809b2cbcd3 Mon Sep 17 00:00:00 2001 From: Jacobo Dominguez Date: Fri, 4 Sep 2026 12:16:08 -0600 Subject: [PATCH 11/16] feat: authz edit permission gate for XBlock component card action menu (#39013) (cherry picked from commit f3a87b132a301789659fc9ae565c95fb85dee48d) --- cms/djangoapps/contentstore/views/block.py | 60 +++- cms/djangoapps/contentstore/views/preview.py | 7 + .../contentstore/views/tests/test_block.py | 261 ++++++++++++++++++ cms/templates/studio_xblock_wrapper.html | 4 +- 4 files changed, 329 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index ae9005a546f4..ec5a1b972cda 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -12,7 +12,11 @@ from django.views.decorators.clickjacking import xframe_options_exempt from django.views.decorators.http import require_http_methods from opaque_keys.edx.keys import CourseKey -from openedx_authz.constants.permissions import COURSES_VIEW_COURSE +from openedx_authz.constants.permissions import ( + COURSES_EDIT_COURSE_CONTENT, + COURSES_MANAGE_TAGS, + COURSES_VIEW_COURSE, +) from web_fragments.fragment import Fragment from cms.djangoapps.contentstore.utils import load_services_for_studio @@ -27,6 +31,7 @@ from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW from common.djangoapps.edxmako.shortcuts import render_to_response, render_to_string from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access +from common.djangoapps.student.roles import enable_authz_course_authoring from common.djangoapps.util.json_request import JsonResponse, expect_json from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission from openedx.core.djangoapps.authz.decorators import user_has_course_permission @@ -129,10 +134,49 @@ def xblock_handler(request, usage_key_string=None): return handle_xblock(request, usage_key_string) +def _get_authz_permissions_flags(user, course_key): + """ + Return the RBAC-authoring flags used to gate portions of the XBlock + component card template (the header-actions div and the "Manage Tags" + action). + + When ``authz.enable_course_authoring`` is off for the course all flags + default to values that preserve existing (pre-RBAC) behaviour: + - ``is_authz_authoring_enabled = False`` → template always shows the div + and the "Manage Tags" action. + - ``authz_can_edit_course_content = False`` → unused while flag is off. + - ``authz_can_manage_tags = False`` → unused while flag is off. + + When the flag is on: + - ``authz_can_edit_course_content`` reflects whether the requesting user + holds the ``courses.edit_course_content`` permission. + - ``authz_can_manage_tags`` reflects whether the requesting user holds the + ``courses.manage_tags`` permission. + + Returns: + tuple[bool, bool, bool]: (is_authz_authoring_enabled, + authz_can_edit_course_content, authz_can_manage_tags) + """ + if not enable_authz_course_authoring(course_key): + return False, False, False + can_edit = user_has_course_permission( + user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + legacy_permission=LegacyAuthoringPermission.WRITE, + ) + can_manage_tags = user_has_course_permission( + user, + COURSES_MANAGE_TAGS.identifier, + course_key, + ) + return True, can_edit, can_manage_tags + + @require_http_methods("GET") @login_required @expect_json -def xblock_view_handler(request, usage_key_string, view_name): +def xblock_view_handler(request, usage_key_string, view_name): # pylint: disable=too-many-statements """ The restful handler for requests for rendered xblock views. @@ -205,6 +249,15 @@ def xblock_view_handler(request, usage_key_string, view_name): ) # Only the "Pages" view uses student view in Studio can_edit = has_studio_write_access(request.user, usage_key.course_key) + # Gate the header-actions div on courses.edit_course_content and the + # "Manage Tags" action on courses.manage_tags when the authz flag is + # on. See _get_authz_permissions_flags for details. + ( + is_authz_authoring_enabled, + authz_can_edit_course_content, + authz_can_manage_tags, + ) = _get_authz_permissions_flags(request.user, usage_key.course_key) + # Determine the items to be shown as reorderable. Note that the view # 'reorderable_container_child_preview' is only rendered for xblocks that # are being shown in a reorderable container, so the xblock is automatically @@ -247,6 +300,9 @@ def xblock_view_handler(request, usage_key_string, view_name): "is_pages_view": is_pages_view or view_name == AUTHOR_VIEW, "is_unit_page": is_unit(xblock), "can_edit": can_edit, + "is_authz_authoring_enabled": is_authz_authoring_enabled, + "authz_can_edit_course_content": authz_can_edit_course_content, + "authz_can_manage_tags": authz_can_manage_tags, "root_xblock": xblock if (view_name == "container_preview") else None, diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index d3ca840c01fd..b77b520f9a29 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -318,6 +318,10 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): can_edit = context.get('can_edit', True) can_add = context.get('can_add', True) can_move = context.get('can_move', True) + # Set by block.py; default False so callers that don't set it are unaffected. + is_authz_authoring_enabled = context.get('is_authz_authoring_enabled', False) + authz_can_edit_course_content = context.get('authz_can_edit_course_content', True) + authz_can_manage_tags = context.get('authz_can_manage_tags', True) root_upstream_link = UpstreamLink.try_get_for_block(root_xblock, log_error=False) upstream_link = UpstreamLink.try_get_for_block(xblock, log_error=False) if ( @@ -363,6 +367,9 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): 'is_course': is_course, 'tags_count': tags_count, 'can_edit_title': True, # This is always true even for imported components + 'is_authz_authoring_enabled': is_authz_authoring_enabled, + 'authz_can_edit_course_content': authz_can_edit_course_content, + 'authz_can_manage_tags': authz_can_manage_tags, } add_webpack_js_to_fragment(frag, "js/factories/xblock_validation") diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 6884dd86ab19..77b4e174930f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -19,6 +19,11 @@ from opaque_keys.edx.asides import AsideUsageKeyV2 from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from openedx_authz.constants.permissions import ( + COURSES_EDIT_COURSE_CONTENT, + COURSES_MANAGE_TAGS, + COURSES_VIEW_COURSE, +) from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_AUDITOR, COURSE_EDITOR, COURSE_STAFF from openedx_events.content_authoring.data import DuplicatedXBlockData from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED @@ -545,6 +550,262 @@ def assert_xblock_info(xblock, xblock_info): self.assertEqual(xblock_info, response) # noqa: PT009 +class TestXBlockViewHandlerHeaderActionsAuthz(ItemTest): + """ + Regression tests for the ``header-actions`` div gating introduced to + conditionally render the component card action menu based on the RBAC + ``courses.edit_course_content`` permission. + + The gate uses two independent context flags: + - ``is_authz_authoring_enabled``: True when enable_authz_course_authoring + is on for the course. + - ``authz_can_edit_course_content``: True when the user holds + courses.edit_course_content (only evaluated when the flag is on). + + The template condition is: + ``not is_authz_authoring_enabled or authz_can_edit_course_content`` + + So the div is shown when the flag is off (preserving existing behaviour) + or when the flag is on and the user has the permission. + """ + + AUTHZ_FLAG_PATH = ( + "cms.djangoapps.contentstore.views.block.enable_authz_course_authoring" + ) + # Patch user_has_course_permission at the block.py binding so the + # authz_can_edit_course_content value is fully controlled by the test. + # + # NOTE: xblock_view_handler gates the *whole* request on + # ``courses.view_course`` via this same binding before the template is ever + # rendered. A blanket return_value=False would make that view-access gate + # fail and the handler would raise PermissionDenied (403) before reaching + # the header-actions logic. We therefore drive the mock with a + # permission-aware side effect that always grants view access and only + # toggles the ``courses.edit_course_content`` permission. + AUTHZ_PERMISSION_PATH = ( + "cms.djangoapps.contentstore.views.block.user_has_course_permission" + ) + HEADER_ACTIONS_DIV = 'class="header-actions"' + + @staticmethod + def _permission_side_effect(*, can_edit_course_content, can_manage_tags=True): + """ + Build a ``user_has_course_permission`` side effect that always grants + ``courses.view_course`` (so the handler returns 200), returns + ``can_edit_course_content`` for ``courses.edit_course_content`` and + ``can_manage_tags`` for ``courses.manage_tags``. + + The permission identifier is passed as the second positional argument + by every call site in ``block.py``. + """ + def _side_effect(_user, permission_identifier, *_args, **_kwargs): + if permission_identifier == COURSES_VIEW_COURSE.identifier: + return True + if permission_identifier == COURSES_EDIT_COURSE_CONTENT.identifier: + return can_edit_course_content + if permission_identifier == COURSES_MANAGE_TAGS.identifier: + return can_manage_tags + return False + + return _side_effect + + def _get_container_preview_html(self): + """ + Return the rendered HTML for a child vertical card inside a parent vertical. + + ``header-actions`` only appears on non-root blocks (``is_root=False``). + We replicate the setup used by ``test_draft_container_preview_html`` in + ``test_container_page.py``: create a parent vertical, add a child + vertical inside it, then request ``reorderable_container_child_preview`` + for that child. A vertical renders cleanly in the test environment + without needing any external services, and its card includes the full + ``header-actions`` section. + """ + parent_usage_key = self._create_vertical() + child_usage_key = self._create_vertical(parent_usage_key=parent_usage_key) + + preview_url = reverse_usage_url( + "xblock_view_handler", + child_usage_key, + {"view_name": "reorderable_container_child_preview"}, + ) + resp = self.client.get(preview_url, HTTP_ACCEPT="application/json") + assert resp.status_code == 200 + return json.loads(resp.content.decode("utf-8"))["html"] + + def test_header_actions_visible_when_flag_off(self): + """ + When enable_authz_course_authoring is off, is_authz_authoring_enabled + is False and the template condition ``not False or *`` is always True, + so the div must be present regardless of any permission value. + Preserves existing behaviour for courses not yet on the authz rollout. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=False): + html = self._get_container_preview_html() + + assert self.HEADER_ACTIONS_DIV in html + + def test_header_actions_visible_when_flag_on_and_user_allowed(self): + """ + When the flag is on and the user holds courses.edit_course_content, + is_authz_authoring_enabled=True and authz_can_edit_course_content=True, + so the template condition is True and the div must be rendered. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect(can_edit_course_content=True), + ): + html = self._get_container_preview_html() + + assert self.HEADER_ACTIONS_DIV in html + + def test_header_actions_hidden_when_flag_on_and_user_denied(self): + """ + When the flag is on and the user does NOT hold courses.edit_course_content, + is_authz_authoring_enabled=True and authz_can_edit_course_content=False, + so the template condition is False and the entire header-actions div + must be absent from the rendered HTML. + This is the core regression test: without the fix the div would always + render even for read-only users when the authz flag is on. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect(can_edit_course_content=False), + ): + html = self._get_container_preview_html() + + assert self.HEADER_ACTIONS_DIV not in html + + +class TestXBlockViewHandlerManageTagsAuthz(ItemTest): + """ + Regression tests for the "Manage Tags" action-menu item gating based on the + RBAC ``courses.manage_tags`` permission. + + The gate uses two independent context flags: + - ``is_authz_authoring_enabled``: True when enable_authz_course_authoring + is on for the course. + - ``authz_can_manage_tags``: True when the user holds courses.manage_tags + (only evaluated when the flag is on). + + The template condition is: + ``use_tagging and (not is_authz_authoring_enabled or authz_can_manage_tags)`` + + So the "Manage Tags" link is shown when tagging is enabled and either the + authz flag is off (preserving existing behaviour) or the flag is on and the + user holds the permission. + + Because the outer ``header-actions`` div is itself gated on + ``courses.edit_course_content``, these tests always grant that permission so + the menu renders and only the "Manage Tags" item is toggled. + """ + + AUTHZ_FLAG_PATH = ( + "cms.djangoapps.contentstore.views.block.enable_authz_course_authoring" + ) + AUTHZ_PERMISSION_PATH = ( + "cms.djangoapps.contentstore.views.block.user_has_course_permission" + ) + MANAGE_TAGS_LINK = 'class="manage-tags-button"' + + @staticmethod + def _permission_side_effect(*, can_manage_tags): + """ + Build a ``user_has_course_permission`` side effect that always grants + ``courses.view_course`` (200 response) and ``courses.edit_course_content`` + (so the header-actions menu renders), and returns ``can_manage_tags`` for + ``courses.manage_tags``. + """ + def _side_effect(_user, permission_identifier, *_args, **_kwargs): + if permission_identifier == COURSES_VIEW_COURSE.identifier: + return True + if permission_identifier == COURSES_EDIT_COURSE_CONTENT.identifier: + return True + if permission_identifier == COURSES_MANAGE_TAGS.identifier: + return can_manage_tags + return False + + return _side_effect + + def _get_container_preview_html(self): + """ + Return the rendered HTML for a leaf ``html`` component card inside a + parent vertical. + + The "Manage Tags" action lives inside the ``% if not show_inline:`` + block of the card template, where ``show_inline = xblock.has_children + and not xblock_url``. A vertical has children and no studio URL, so it + is rendered inline and its "Manage Tags" item is never emitted. A leaf + ``html`` component has no children and does have a studio URL, so + ``show_inline`` is False and the action menu (including "Manage Tags") + is rendered. Requesting ``reorderable_container_child_preview`` for the + component yields a non-root card that includes the full action menu. + """ + parent_usage_key = self._create_vertical() + resp = self.create_xblock( + parent_usage_key=parent_usage_key, category="html" + ) + assert resp.status_code == 200 + child_usage_key = self.response_usage_key(resp) + + preview_url = reverse_usage_url( + "xblock_view_handler", + child_usage_key, + {"view_name": "reorderable_container_child_preview"}, + ) + resp = self.client.get(preview_url, HTTP_ACCEPT="application/json") + assert resp.status_code == 200 + return json.loads(resp.content.decode("utf-8"))["html"] + + def test_manage_tags_visible_when_flag_off(self): + """ + When enable_authz_course_authoring is off, is_authz_authoring_enabled is + False and the ``not False or *`` clause is always True, so the "Manage + Tags" link must be present (tagging is enabled in the test environment). + Preserves existing behaviour for courses not yet on the authz rollout. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=False): + html = self._get_container_preview_html() + + assert self.MANAGE_TAGS_LINK in html + + def test_manage_tags_visible_when_flag_on_and_user_allowed(self): + """ + When the flag is on and the user holds courses.manage_tags, + is_authz_authoring_enabled=True and authz_can_manage_tags=True, so the + template condition is True and the "Manage Tags" link must be rendered. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect(can_manage_tags=True), + ): + html = self._get_container_preview_html() + + assert self.MANAGE_TAGS_LINK in html + + def test_manage_tags_hidden_when_flag_on_and_user_denied(self): + """ + When the flag is on and the user does NOT hold courses.manage_tags, + is_authz_authoring_enabled=True and authz_can_manage_tags=False, so the + template condition is False and the "Manage Tags" link must be absent + from the rendered HTML, even though the surrounding header-actions menu + still renders (the user retains courses.edit_course_content). + This is the core regression test: without the fix the link would always + render for any user who can see the actions menu when the flag is on. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect(can_manage_tags=False), + ): + html = self._get_container_preview_html() + + assert self.MANAGE_TAGS_LINK not in html + + @ddt.ddt class DeleteItem(ItemTest): """Tests for '/xblock' DELETE url.""" diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 5dcafb96e02a..af59c20575be 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -165,6 +165,7 @@ % endif + % if not is_authz_authoring_enabled or authz_can_edit_course_content:
    % if can_edit_title and not can_edit: @@ -217,7 +218,7 @@ ${_("Move")} % endif - % if use_tagging: + % if use_tagging and (not is_authz_authoring_enabled or authz_can_manage_tags):
  • ${_("Manage Tags")}
    • @@ -259,6 +260,7 @@ % endif
+ % endif % if not is_root:
From 325e25fcdb8b14a3a1bbc1448f871c6d2e7d6911 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Tue, 22 Sep 2026 18:06:37 +0400 Subject: [PATCH 12/16] feat: make can_tag_object's course check authorization-service aware (#39112) (cherry picked from commit ecb9f5ef4b599051cd6c817f8042c4769cd89b8c) --- .../content_tagging/rest_api/v1/views.py | 20 +-- .../core/djangoapps/content_tagging/rules.py | 13 +- .../content_tagging/tests/test_rules.py | 126 ++++++++++++++++++ 3 files changed, 138 insertions(+), 21 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py index 556ea06cfe1e..ef01b6930e52 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -9,7 +9,7 @@ from django.db.models import Count from django.http import StreamingHttpResponse from openedx_authz import api as authz_api -from openedx_authz.constants.permissions import COURSES_MANAGE_TAGS, COURSES_VIEW_COURSE +from openedx_authz.constants.permissions import COURSES_VIEW_COURSE from openedx_events.content_authoring.data import ContentObjectChangedData, ContentObjectData from openedx_events.content_authoring.signals import CONTENT_OBJECT_ASSOCIATIONS_CHANGED, CONTENT_OBJECT_TAGS_CHANGED from openedx_tagging import rules as oel_tagging_rules @@ -207,24 +207,6 @@ def ensure_has_view_object_tag_permission(self, user, taxonomy, object_id): # Fall back to parent implementation super().ensure_has_view_object_tag_permission(user, taxonomy, object_id) - def ensure_user_has_can_tag_object_permissions(self, user, tags_data, object_id): - """ - Check if user has permission to tag object for each taxonomy in tags_data. - - This method is overridden to conditionally use openedx-authz when the toggle is enabled. - - When using openedx-authz, if the user has manage tags permission for the course, - they can tag the object regardless of the taxonomy. - """ - should_use_authz, course_key = self._authz_check - if should_use_authz and not authz_api.is_user_allowed( - user.username, COURSES_MANAGE_TAGS.identifier, str(course_key) - ): - raise PermissionDenied("You do not have permission to manage object tags.") - if not should_use_authz: - # Fall back to parent implementation - super().ensure_user_has_can_tag_object_permissions(user, tags_data, object_id) - def update(self, request, *args, **kwargs) -> Response: """ Extend the update method to fire CONTENT_OBJECT_ASSOCIATIONS_CHANGED event diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 3b70076189ef..33dd13c18fe9 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -23,6 +23,7 @@ OrgStaffRole, ) +from .auth import should_use_course_authz_for_object from .models import TaxonomyOrg from .utils import check_taxonomy_context_key_org, get_context_key_from_key_string, rules_cache @@ -225,8 +226,10 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: For Content Libraries V2, this requires either explicit library tagging permission (MANAGE_LIBRARY_TAGS) or org-level admin access for the library's org. - For other contexts (courses, xblocks, etc.), this requires studio write access or - org-level admin access for the object's org. + For a course (or object within one) switched to openedx-authz, this requires + courses.manage_tags through that service alone, with no fallback to studio write or + org-admin access; for any other course, xblock, etc. not yet switched, those checks + still apply exactly as before. """ if not object_id: return True @@ -246,6 +249,12 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: ): return True + should_use_authz, course_key = should_use_course_authz_for_object(object_id) + if should_use_authz: + return authz_api.is_user_allowed( + user.username, authz_permissions.COURSES_MANAGE_TAGS.identifier, str(course_key) + ) + # For other contexts (courses, xblocks, etc.), use general write or org-admin access if has_studio_write_access(user, context_key): return True diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 4432310147c7..e8f81f604f1e 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -5,6 +5,7 @@ import ddt from django.contrib.auth import get_user_model from django.test import TestCase +from edx_toggles.toggles.testutils import override_waffle_flag from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocatorV2 from openedx_authz.constants import permissions as authz_permissions from openedx_tagging.models import Tag, UserSystemDefinedTaxonomy @@ -12,6 +13,7 @@ from common.djangoapps.student.auth import add_users, update_org_role from common.djangoapps.student.roles import CourseStaffRole, OrgStaffRole +from openedx.core.toggles import AUTHZ_COURSE_AUTHORING_FLAG from .. import api from ..rules import can_change_object_tag_objectid, can_remove_object_tag_objectid @@ -743,3 +745,127 @@ def test_empty_object_id(self): with self.assertRaises(ValueError): # noqa: PT027 can_remove_object_tag_objectid(self.library_user, "") + + +class TestRulesCourseAuthzPermissions(TestTaxonomyMixin, TestCase): + """ + Tests for can_change_object_tag_objectid's course-authz-aware branch. + """ + + def setUp(self): + super().setUp() + + self.superuser = User.objects.create( + username="superuser", + email="superuser@example.com", + is_superuser=True, + ) + self.authz_user = User.objects.create( + username="authz_user", + email="authz_user@example.com", + ) + self.legacy_user = User.objects.create( + username="legacy_user", + email="legacy_user@example.com", + ) + self.org_admin_user = User.objects.create( + username="org_admin_user", + email="org_admin_user@example.com", + ) + + self.course_key = CourseLocator.from_string("course-v1:OeX+DemoX+Demo_Course") + self.xblock_key = BlockUsageLocator( + course_key=self.course_key, + block_type='problem', + block_id='block_id', + ) + # Granted while the waffle flag is off, so this goes through the legacy role path. + add_users(self.superuser, CourseStaffRole(self.course_key), self.legacy_user) + # org1's short_name ("OeX") matches self.course_key's org, so this is an org-level + # admin for the course's org, not merely a course-level role. + update_org_role(self.superuser, OrgStaffRole, self.org_admin_user, [self.org1.short_name]) + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + @patch("openedx_authz.api.is_user_allowed") + def test_course_switched_authz_only_role_allowed(self, mock_is_user_allowed): + """ + A course switched to openedx-authz grants access via authz_api.is_user_allowed alone. + """ + mock_is_user_allowed.return_value = True + + result = can_change_object_tag_objectid(self.authz_user, str(self.course_key)) + + self.assertTrue(result) # noqa: PT009 + mock_is_user_allowed.assert_called_once_with( + self.authz_user.username, + authz_permissions.COURSES_MANAGE_TAGS.identifier, + str(self.course_key), + ) + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + @patch("openedx_authz.api.is_user_allowed") + def test_course_switched_legacy_only_role_denied(self, mock_is_user_allowed): + """ + A switched course does not fall back to a legacy role: the switch is exclusive, not + an OR with the legacy check. + """ + mock_is_user_allowed.return_value = False + + result = can_change_object_tag_objectid(self.legacy_user, str(self.course_key)) + + self.assertFalse(result) # noqa: PT009 + mock_is_user_allowed.assert_called_once_with( + self.legacy_user.username, + authz_permissions.COURSES_MANAGE_TAGS.identifier, + str(self.course_key), + ) + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + @patch("openedx_authz.api.is_user_allowed") + def test_xblock_in_switched_course_authz_only_role_allowed(self, mock_is_user_allowed): + """ + An xblock inside a switched course resolves through the same authz-only branch as + the course itself: should_use_course_authz_for_object resolves the xblock's usage + key down to its course key. + """ + mock_is_user_allowed.return_value = True + + result = can_change_object_tag_objectid(self.authz_user, str(self.xblock_key)) + + self.assertTrue(result) # noqa: PT009 + mock_is_user_allowed.assert_called_once_with( + self.authz_user.username, + authz_permissions.COURSES_MANAGE_TAGS.identifier, + str(self.course_key), + ) + + @patch("openedx_authz.api.is_user_allowed") + def test_course_not_switched_legacy_role_allowed(self, mock_is_user_allowed): + """ + A course that hasn't been switched to openedx-authz keeps resolving through the + legacy role check, unaffected by this change. + """ + result = can_change_object_tag_objectid(self.legacy_user, str(self.course_key)) + + self.assertTrue(result) # noqa: PT009 + mock_is_user_allowed.assert_not_called() + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + @patch("openedx_authz.api.is_user_allowed") + def test_library_object_unchanged(self, mock_is_user_allowed): + """ + Library objects are unaffected by this branch: should_use_course_authz_for_object + only recognizes CourseKeys, so the library check still resolves through + MANAGE_LIBRARY_TAGS even with the course-authz flag on. + """ + mock_is_user_allowed.return_value = True + library_key = LibraryLocatorV2.from_string(f"lib:{self.org1.short_name}:test_library") + + result = can_change_object_tag_objectid(self.legacy_user, str(library_key)) + + self.assertTrue(result) # noqa: PT009 + mock_is_user_allowed.assert_called_once_with( + self.legacy_user.username, + authz_permissions.MANAGE_LIBRARY_TAGS.identifier, + str(library_key), + ) From 4f21a403f9f17ff6ce00bd27e54dc176100147af Mon Sep 17 00:00:00 2001 From: Jacobo Dominguez Date: Wed, 30 Sep 2026 12:09:25 -0600 Subject: [PATCH 13/16] fix: gate xblock action menu on authz edit/manage_tags permissions (#39050) (cherry picked from commit b91a89e561fe2115f1f90c0f643507cae6cc2e34) --- cms/djangoapps/contentstore/views/block.py | 89 +++--- cms/djangoapps/contentstore/views/preview.py | 24 +- .../contentstore/views/tests/test_block.py | 300 +++++++++++++++--- cms/templates/studio_xblock_wrapper.html | 6 +- openedx/core/djangoapps/authz/decorators.py | 9 +- .../djangoapps/authz/tests/test_decorators.py | 88 ++++- 6 files changed, 415 insertions(+), 101 deletions(-) diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index ec5a1b972cda..0727b26235ed 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -30,8 +30,7 @@ from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_tags_count, usage_key_with_run from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW from common.djangoapps.edxmako.shortcuts import render_to_response, render_to_string -from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access -from common.djangoapps.student.roles import enable_authz_course_authoring +from common.djangoapps.student.auth import has_studio_read_access from common.djangoapps.util.json_request import JsonResponse, expect_json from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission from openedx.core.djangoapps.authz.decorators import user_has_course_permission @@ -134,43 +133,57 @@ def xblock_handler(request, usage_key_string=None): return handle_xblock(request, usage_key_string) -def _get_authz_permissions_flags(user, course_key): +def _user_can_edit_course_content(user, course_key): """ - Return the RBAC-authoring flags used to gate portions of the XBlock - component card template (the header-actions div and the "Manage Tags" - action). - - When ``authz.enable_course_authoring`` is off for the course all flags - default to values that preserve existing (pre-RBAC) behaviour: - - ``is_authz_authoring_enabled = False`` → template always shows the div - and the "Manage Tags" action. - - ``authz_can_edit_course_content = False`` → unused while flag is off. - - ``authz_can_manage_tags = False`` → unused while flag is off. - - When the flag is on: - - ``authz_can_edit_course_content`` reflects whether the requesting user - holds the ``courses.edit_course_content`` permission. - - ``authz_can_manage_tags`` reflects whether the requesting user holds the - ``courses.manage_tags`` permission. - - Returns: - tuple[bool, bool, bool]: (is_authz_authoring_enabled, - authz_can_edit_course_content, authz_can_manage_tags) + Return whether the user may edit course content, as a single final boolean. + + This delegates entirely to ``user_has_course_permission`` which already + encapsulates the flag logic: when ``authz.enable_course_authoring`` is on + for the course the ``courses.edit_course_content`` AuthZ permission is + checked and legacy access is ignored; when the flag is off it falls back to + the legacy studio WRITE permission. No separate legacy check is OR'd in. """ - if not enable_authz_course_authoring(course_key): - return False, False, False - can_edit = user_has_course_permission( + return user_has_course_permission( user, COURSES_EDIT_COURSE_CONTENT.identifier, course_key, legacy_permission=LegacyAuthoringPermission.WRITE, ) - can_manage_tags = user_has_course_permission( + + +def _user_can_manage_tags(user, course_key): + """ + Return whether the user may manage tags, as a single final boolean. + + Tag management has no legacy-permission concept, so when + ``authz.enable_course_authoring`` is off for the course we preserve the + pre-RBAC behaviour and return ``True``. When the flag is on we check the + ``courses.manage_tags`` AuthZ permission. + """ + return user_has_course_permission( user, COURSES_MANAGE_TAGS.identifier, course_key, + default_fallback=True, + ) + + +def _user_can_edit_title(user, course_key): + """ + Return whether the user may edit an xblock title, as a single final boolean. + + Editing a title is a content-authoring action, so when + ``authz.enable_course_authoring`` is off for the course we preserve the + pre-RBAC behaviour and return ``True`` (the "Edit Title" affordance was + historically always available). When the flag is on it tracks the + ``courses.edit_course_content`` AuthZ permission. + """ + return user_has_course_permission( + user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + default_fallback=True, ) - return True, can_edit, can_manage_tags @require_http_methods("GET") @@ -247,16 +260,13 @@ def xblock_view_handler(request, usage_key_string, view_name): # pylint: disable is_pages_view = ( view_name == STUDENT_VIEW ) # Only the "Pages" view uses student view in Studio - can_edit = has_studio_write_access(request.user, usage_key.course_key) - # Gate the header-actions div on courses.edit_course_content and the - # "Manage Tags" action on courses.manage_tags when the authz flag is - # on. See _get_authz_permissions_flags for details. - ( - is_authz_authoring_enabled, - authz_can_edit_course_content, - authz_can_manage_tags, - ) = _get_authz_permissions_flags(request.user, usage_key.course_key) + # Resolve the final gating booleans server-side. Each helper + # encapsulates its own "authz flag off" default, so the template + # only needs these two already-final values. + can_edit = _user_can_edit_course_content(request.user, usage_key.course_key) + can_manage_tags = _user_can_manage_tags(request.user, usage_key.course_key) + can_edit_title = _user_can_edit_title(request.user, usage_key.course_key) # Determine the items to be shown as reorderable. Note that the view # 'reorderable_container_child_preview' is only rendered for xblocks that @@ -300,9 +310,8 @@ def xblock_view_handler(request, usage_key_string, view_name): # pylint: disable "is_pages_view": is_pages_view or view_name == AUTHOR_VIEW, "is_unit_page": is_unit(xblock), "can_edit": can_edit, - "is_authz_authoring_enabled": is_authz_authoring_enabled, - "authz_can_edit_course_content": authz_can_edit_course_content, - "authz_can_manage_tags": authz_can_manage_tags, + "can_manage_tags": can_manage_tags, + "can_edit_title": can_edit_title, "root_xblock": xblock if (view_name == "container_preview") else None, diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index b77b520f9a29..420def3e4f44 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -318,10 +318,9 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): can_edit = context.get('can_edit', True) can_add = context.get('can_add', True) can_move = context.get('can_move', True) - # Set by block.py; default False so callers that don't set it are unaffected. - is_authz_authoring_enabled = context.get('is_authz_authoring_enabled', False) - authz_can_edit_course_content = context.get('authz_can_edit_course_content', True) - authz_can_manage_tags = context.get('authz_can_manage_tags', True) + # Set by block.py as an already-final boolean. Default True so callers + # that don't set it preserve pre-RBAC behaviour (matching can_edit). + can_manage_tags = context.get('can_manage_tags', True) root_upstream_link = UpstreamLink.try_get_for_block(root_xblock, log_error=False) upstream_link = UpstreamLink.try_get_for_block(xblock, log_error=False) if ( @@ -338,6 +337,13 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): if upstream_link.error_message is None and upstream_link.upstream_ref: can_edit = xblock.category in editable_library_components + # All content-modifying actions require edit access. A user may reach + # the actions menu with only tag-management rights (can_manage_tags), + # so gate the edit-type flags on can_edit to keep Move/Add/Delete and + # (via can_edit_visibility below) Manage Access out of their reach. + can_add = can_add and can_edit + can_move = can_move and can_edit + # Is this a course or a library? is_course = xblock.context_key.is_course tags_count_map = context.get('tags_count_map') @@ -366,10 +372,12 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): 'language': getattr(course, 'language', None), 'is_course': is_course, 'tags_count': tags_count, - 'can_edit_title': True, # This is always true even for imported components - 'is_authz_authoring_enabled': is_authz_authoring_enabled, - 'authz_can_edit_course_content': authz_can_edit_course_content, - 'authz_can_manage_tags': authz_can_manage_tags, + # Set by block.py as an already-final boolean. Defaults True so + # callers that don't set it preserve pre-RBAC behaviour (the "Edit + # Title" affordance was historically always available, including for + # imported components). + 'can_edit_title': context.get('can_edit_title', True), + 'can_manage_tags': can_manage_tags, } add_webpack_js_to_fragment(frag, "js/factories/xblock_validation") diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 77b4e174930f..274317971bc8 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -554,26 +554,32 @@ class TestXBlockViewHandlerHeaderActionsAuthz(ItemTest): """ Regression tests for the ``header-actions`` div gating introduced to conditionally render the component card action menu based on the RBAC - ``courses.edit_course_content`` permission. + ``courses.edit_course_content`` and ``courses.manage_tags`` permissions. - The gate uses two independent context flags: - - ``is_authz_authoring_enabled``: True when enable_authz_course_authoring - is on for the course. - - ``authz_can_edit_course_content``: True when the user holds - courses.edit_course_content (only evaluated when the flag is on). + ``block.py`` resolves two already-final booleans server-side: + - ``can_edit``: whether the user may edit course content. Delegates to + ``user_has_course_permission`` which checks AuthZ when the flag is on and + falls back to the legacy studio WRITE permission when the flag is off. + - ``can_manage_tags``: whether the user may manage tags. True when the flag + is off (tags have no legacy-permission concept), otherwise the + ``courses.manage_tags`` AuthZ permission. - The template condition is: - ``not is_authz_authoring_enabled or authz_can_edit_course_content`` + The template opens the div on: + ``can_edit or can_manage_tags or can_edit_title`` + + ``can_edit_title`` is also resolved server-side: True when the flag is off + (preserving the historically-always-available "Edit Title" affordance) and + equal to ``courses.edit_course_content`` when the flag is on. - So the div is shown when the flag is off (preserving existing behaviour) - or when the flag is on and the user has the permission. + So the div is shown when the flag is off (all three default to permissive) + or when the flag is on and the user has edit or tag-management access. """ AUTHZ_FLAG_PATH = ( - "cms.djangoapps.contentstore.views.block.enable_authz_course_authoring" + "openedx.core.djangoapps.authz.decorators.enable_authz_course_authoring" ) # Patch user_has_course_permission at the block.py binding so the - # authz_can_edit_course_content value is fully controlled by the test. + # can_edit value is fully controlled by the test. # # NOTE: xblock_view_handler gates the *whole* request on # ``courses.view_course`` via this same binding before the template is ever @@ -586,6 +592,13 @@ class TestXBlockViewHandlerHeaderActionsAuthz(ItemTest): "cms.djangoapps.contentstore.views.block.user_has_course_permission" ) HEADER_ACTIONS_DIV = 'class="header-actions"' + # The component (content) "Edit" button is rendered only when + # ``not show_inline and can_edit`` in studio_xblock_wrapper.html. Match on + # its full class string so this does NOT collide with the separate + # "Edit Title" button (``title-edit-button``), which is rendered on the + # opposite condition (``can_edit_title and not can_edit``) and would + # otherwise match a bare ``edit-button`` substring. + CONTENT_EDIT_BUTTON = 'class="btn-default edit-button action-button"' @staticmethod def _permission_side_effect(*, can_edit_course_content, can_manage_tags=True): @@ -633,11 +646,40 @@ def _get_container_preview_html(self): assert resp.status_code == 200 return json.loads(resp.content.decode("utf-8"))["html"] + def _get_leaf_component_preview_html(self): + """ + Return the rendered HTML for a leaf ``html`` component card. + + The component (content) "Edit" button is gated on + ``not show_inline and can_edit`` in studio_xblock_wrapper.html, where + ``show_inline = xblock.has_children and not xblock_url``. A vertical has + children, so its card is rendered inline and never shows that edit + button regardless of ``can_edit``. We therefore create a leaf ``html`` + component (no children -> ``show_inline`` is False) inside a vertical and + request ``container_child_preview`` for it, which is the branch that the + ``can_edit`` fix actually controls. + """ + parent_usage_key = self._create_vertical() + resp = self.create_xblock( + parent_usage_key=parent_usage_key, category="html" + ) + assert resp.status_code == 200 + child_usage_key = self.response_usage_key(resp) + + preview_url = reverse_usage_url( + "xblock_view_handler", + child_usage_key, + {"view_name": "container_child_preview"}, + ) + resp = self.client.get(preview_url, HTTP_ACCEPT="application/json") + assert resp.status_code == 200 + return json.loads(resp.content.decode("utf-8"))["html"] + def test_header_actions_visible_when_flag_off(self): """ - When enable_authz_course_authoring is off, is_authz_authoring_enabled - is False and the template condition ``not False or *`` is always True, - so the div must be present regardless of any permission value. + When enable_authz_course_authoring is off, both can_edit (legacy WRITE + fallback) and can_manage_tags (defaults True) are permissive, so the + ``can_edit or can_manage_tags`` gate is True and the div must be present. Preserves existing behaviour for courses not yet on the authz rollout. """ with patch(self.AUTHZ_FLAG_PATH, return_value=False): @@ -648,8 +690,8 @@ def test_header_actions_visible_when_flag_off(self): def test_header_actions_visible_when_flag_on_and_user_allowed(self): """ When the flag is on and the user holds courses.edit_course_content, - is_authz_authoring_enabled=True and authz_can_edit_course_content=True, - so the template condition is True and the div must be rendered. + can_edit is True, so the ``can_edit or can_manage_tags`` gate is True and + the div must be rendered. """ with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ patch( @@ -662,67 +704,156 @@ def test_header_actions_visible_when_flag_on_and_user_allowed(self): def test_header_actions_hidden_when_flag_on_and_user_denied(self): """ - When the flag is on and the user does NOT hold courses.edit_course_content, - is_authz_authoring_enabled=True and authz_can_edit_course_content=False, - so the template condition is False and the entire header-actions div - must be absent from the rendered HTML. - This is the core regression test: without the fix the div would always + When the flag is on and the user holds NEITHER courses.edit_course_content + NOR courses.manage_tags, can_edit, can_manage_tags AND can_edit_title are + all False (can_edit_title tracks courses.edit_course_content when the flag + is on), so the ``can_edit or can_manage_tags or can_edit_title`` gate is + False and the entire header-actions div must be absent from the rendered + HTML. + This is a core regression test: without the fix the div would always render even for read-only users when the authz flag is on. """ with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ patch( self.AUTHZ_PERMISSION_PATH, - side_effect=self._permission_side_effect(can_edit_course_content=False), + side_effect=self._permission_side_effect( + can_edit_course_content=False, can_manage_tags=False + ), ): html = self._get_container_preview_html() assert self.HEADER_ACTIONS_DIV not in html + def test_header_actions_visible_when_flag_on_and_only_manage_tags(self): + """ + Regression test for the "Manage Tags" nesting bug. + + The header-actions div previously opened only on the edit permission, so + a role granted courses.manage_tags but NOT courses.edit_course_content + could never reach the "Manage Tags" item nested inside. The div now + opens on ``can_edit or can_manage_tags``, so with only manage_tags + granted the div must still render. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect( + can_edit_course_content=False, can_manage_tags=True + ), + ): + html = self._get_container_preview_html() + + assert self.HEADER_ACTIONS_DIV in html + + def test_can_edit_true_via_authz_without_legacy_write_access(self): + """ + Regression test for the ``can_edit`` fix in xblock_view_handler. + + ``can_edit`` delegates entirely to ``user_has_course_permission`` with a + legacy WRITE fallback. When the authz flag is on that helper checks the + ``courses.edit_course_content`` AuthZ permission and ignores legacy + access entirely. A user granted the AuthZ permission through the + rollout (but without legacy studio write access) must therefore still + get ``can_edit=True`` and see the per-block "Edit" button. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect(can_edit_course_content=True), + ): + html = self._get_leaf_component_preview_html() + + assert self.CONTENT_EDIT_BUTTON in html + + def test_can_edit_false_without_legacy_write_access_or_authz_permission(self): + """ + When the authz flag is on and the user is not granted + ``courses.edit_course_content``, ``user_has_course_permission`` returns + False without consulting legacy access. ``can_edit`` must therefore be + False and the per-block "Edit" button must be absent. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect(can_edit_course_content=False), + ): + html = self._get_leaf_component_preview_html() + + assert self.CONTENT_EDIT_BUTTON not in html + + def test_can_edit_true_via_legacy_write_access_when_flag_off(self): + """ + Legacy fallback path for ``can_edit``. + + When enable_authz_course_authoring is off, ``user_has_course_permission`` + ignores AuthZ and falls back to the legacy studio WRITE permission. The + course-author test user (``CourseTestCase.user``) holds that access, so + ``can_edit`` must be True and the per-block "Edit" button must render. + + This is the complement of ``test_can_edit_true_via_authz_...``: together + they cover both branches of the ``can_edit`` computation (AuthZ-on and + AuthZ-off/legacy). Nothing is patched on ``user_has_course_permission`` + here so the real legacy check runs end to end. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=False): + html = self._get_leaf_component_preview_html() + + assert self.CONTENT_EDIT_BUTTON in html + class TestXBlockViewHandlerManageTagsAuthz(ItemTest): """ Regression tests for the "Manage Tags" action-menu item gating based on the RBAC ``courses.manage_tags`` permission. - The gate uses two independent context flags: - - ``is_authz_authoring_enabled``: True when enable_authz_course_authoring - is on for the course. - - ``authz_can_manage_tags``: True when the user holds courses.manage_tags - (only evaluated when the flag is on). + ``block.py`` resolves ``can_manage_tags`` server-side as a single final + boolean: True when enable_authz_course_authoring is off (tags have no + legacy-permission concept), otherwise the ``courses.manage_tags`` AuthZ + permission. The template condition is: - ``use_tagging and (not is_authz_authoring_enabled or authz_can_manage_tags)`` + ``use_tagging and can_manage_tags`` So the "Manage Tags" link is shown when tagging is enabled and either the authz flag is off (preserving existing behaviour) or the flag is on and the user holds the permission. - Because the outer ``header-actions`` div is itself gated on - ``courses.edit_course_content``, these tests always grant that permission so - the menu renders and only the "Manage Tags" item is toggled. + The outer ``header-actions`` div opens on ``can_edit or can_manage_tags``. + These tests grant ``courses.edit_course_content`` so the menu renders for + the visible/hidden cases and only the "Manage Tags" item is toggled; the + edit-independent case is covered by + ``TestXBlockViewHandlerHeaderActionsAuthz`` and by + ``test_manage_tags_visible_without_edit_permission`` below. """ AUTHZ_FLAG_PATH = ( - "cms.djangoapps.contentstore.views.block.enable_authz_course_authoring" + "openedx.core.djangoapps.authz.decorators.enable_authz_course_authoring" ) AUTHZ_PERMISSION_PATH = ( "cms.djangoapps.contentstore.views.block.user_has_course_permission" ) MANAGE_TAGS_LINK = 'class="manage-tags-button"' + # Edit-type action-menu links. A tags-only user (can_manage_tags without + # can_edit) reaches the actions menu but must NOT see any of these. + MANAGE_ACCESS_LINK = 'class="access-button"' + MOVE_LINK = 'class="move-button"' + COPY_LINK = 'class="copy-button"' + DUPLICATE_LINK = 'class="duplicate-button"' + DELETE_LINK = 'class="delete-button"' @staticmethod - def _permission_side_effect(*, can_manage_tags): + def _permission_side_effect(*, can_manage_tags, can_edit_course_content=True): """ Build a ``user_has_course_permission`` side effect that always grants - ``courses.view_course`` (200 response) and ``courses.edit_course_content`` - (so the header-actions menu renders), and returns ``can_manage_tags`` for - ``courses.manage_tags``. + ``courses.view_course`` (200 response), returns ``can_edit_course_content`` + for ``courses.edit_course_content`` (defaulting True so the header-actions + menu renders), and returns ``can_manage_tags`` for ``courses.manage_tags``. """ def _side_effect(_user, permission_identifier, *_args, **_kwargs): if permission_identifier == COURSES_VIEW_COURSE.identifier: return True if permission_identifier == COURSES_EDIT_COURSE_CONTENT.identifier: - return True + return can_edit_course_content if permission_identifier == COURSES_MANAGE_TAGS.identifier: return can_manage_tags return False @@ -761,10 +892,11 @@ def _get_container_preview_html(self): def test_manage_tags_visible_when_flag_off(self): """ - When enable_authz_course_authoring is off, is_authz_authoring_enabled is - False and the ``not False or *`` clause is always True, so the "Manage - Tags" link must be present (tagging is enabled in the test environment). - Preserves existing behaviour for courses not yet on the authz rollout. + When enable_authz_course_authoring is off, can_manage_tags defaults to + True, so the ``use_tagging and can_manage_tags`` clause is True and the + "Manage Tags" link must be present (tagging is enabled in the test + environment). Preserves existing behaviour for courses not yet on the + authz rollout. """ with patch(self.AUTHZ_FLAG_PATH, return_value=False): html = self._get_container_preview_html() @@ -774,8 +906,8 @@ def test_manage_tags_visible_when_flag_off(self): def test_manage_tags_visible_when_flag_on_and_user_allowed(self): """ When the flag is on and the user holds courses.manage_tags, - is_authz_authoring_enabled=True and authz_can_manage_tags=True, so the - template condition is True and the "Manage Tags" link must be rendered. + can_manage_tags=True, so the template condition is True and the "Manage + Tags" link must be rendered. """ with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ patch( @@ -789,10 +921,10 @@ def test_manage_tags_visible_when_flag_on_and_user_allowed(self): def test_manage_tags_hidden_when_flag_on_and_user_denied(self): """ When the flag is on and the user does NOT hold courses.manage_tags, - is_authz_authoring_enabled=True and authz_can_manage_tags=False, so the - template condition is False and the "Manage Tags" link must be absent - from the rendered HTML, even though the surrounding header-actions menu - still renders (the user retains courses.edit_course_content). + can_manage_tags=False, so the template condition is False and the + "Manage Tags" link must be absent from the rendered HTML, even though + the surrounding header-actions menu still renders (the user retains + courses.edit_course_content). This is the core regression test: without the fix the link would always render for any user who can see the actions menu when the flag is on. """ @@ -805,6 +937,78 @@ def test_manage_tags_hidden_when_flag_on_and_user_denied(self): assert self.MANAGE_TAGS_LINK not in html + def test_manage_tags_visible_without_edit_permission(self): + """ + Regression test for the "Manage Tags" nesting bug. + + A role granted courses.manage_tags but NOT courses.edit_course_content + must still see the "Manage Tags" link. Previously the item was nested + inside a div gated solely on the edit permission, so such a role could + never reach it. The div now opens on ``can_edit or can_manage_tags`` + and the item itself is gated on ``use_tagging and can_manage_tags``, so + the link must be present even with edit access denied. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect( + can_manage_tags=True, can_edit_course_content=False + ), + ): + html = self._get_container_preview_html() + + assert self.MANAGE_TAGS_LINK in html + + def test_edit_actions_hidden_for_tags_only_user(self): + """ + A user granted courses.manage_tags but NOT courses.edit_course_content + reaches the actions menu (to use "Manage Tags"), but every + content-modifying action must be absent. Opening the menu on + ``can_edit or can_manage_tags`` must not leak edit-type items: Manage + Access, Move, Copy to Clipboard, and Duplicate are all gated on + ``can_edit`` (directly in the template or via the ``can_edit``-derived + ``can_add`` / ``can_move`` / ``can_edit_visibility`` flags). + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=True), \ + patch( + self.AUTHZ_PERMISSION_PATH, + side_effect=self._permission_side_effect( + can_manage_tags=True, can_edit_course_content=False + ), + ): + html = self._get_container_preview_html() + + # The menu is reachable via Manage Tags... + assert self.MANAGE_TAGS_LINK in html + # ...but no edit-type action leaks through. + assert self.MANAGE_ACCESS_LINK not in html + assert self.MOVE_LINK not in html + assert self.COPY_LINK not in html + assert self.DUPLICATE_LINK not in html + assert self.DELETE_LINK not in html + + def test_all_actions_visible_for_full_edit_user(self): + """ + Non-RBAC regression: gating the edit-type flags on ``can_edit`` must not + over-restrict a user who *does* have edit access. + + With enable_authz_course_authoring off, ``can_edit`` resolves via the + legacy WRITE fallback, which the course-author test user holds. Every + edit-type action (Manage Access, Move, Copy, Duplicate, Delete) must + still render alongside Manage Tags, confirming the ``can_add and + can_edit`` / ``can_move and can_edit`` guards left the full-access path + untouched. + """ + with patch(self.AUTHZ_FLAG_PATH, return_value=False): + html = self._get_container_preview_html() + + assert self.MANAGE_TAGS_LINK in html + assert self.MANAGE_ACCESS_LINK in html + assert self.MOVE_LINK in html + assert self.COPY_LINK in html + assert self.DUPLICATE_LINK in html + assert self.DELETE_LINK in html + @ddt.ddt class DeleteItem(ItemTest): diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index af59c20575be..24dbcd03e19a 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -165,7 +165,7 @@ % endif
- % if not is_authz_authoring_enabled or authz_can_edit_course_content: + % if can_edit or can_manage_tags or can_edit_title:
    % if can_edit_title and not can_edit: @@ -218,12 +218,12 @@ ${_("Move")} % endif - % if use_tagging and (not is_authz_authoring_enabled or authz_can_manage_tags): + % if use_tagging and can_manage_tags:
  • ${_("Manage Tags")}
    • % endif - % if is_course: + % if is_course and can_edit: