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/v0/tests/test_course_optimizer_permissions.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer_permissions.py new file mode 100644 index 000000000000..5c21e7d56d2b --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer_permissions.py @@ -0,0 +1,151 @@ +""" +Integration tests verifying authz permissions for v0 course optimizer REST API views. +""" +from unittest.mock import patch + +from django.urls import reverse +from openedx_authz.constants.roles import COURSE_AUDITOR, COURSE_EDITOR, COURSE_STAFF + +from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin + +VIEWS_MODULE = 'cms.djangoapps.contentstore.rest_api.v0.views.course_optimizer' + + +class CourseOptimizerV0AuthzTest(CourseAuthoringAuthzTestMixin, CourseTestCase): + """ + Integration tests for v0 course optimizer API authz permissions. + + All endpoints require courses.edit_course_content. + """ + + def setUp(self): + super().setUp() + self.link_check_url = reverse( + 'cms.djangoapps.contentstore:v0:link_check', + kwargs={'course_id': self.course.id}, + ) + self.link_check_status_url = reverse( + 'cms.djangoapps.contentstore:v0:link_check_status', + kwargs={'course_id': self.course.id}, + ) + self.rerun_link_update_url = reverse( + 'cms.djangoapps.contentstore:v0:rerun_link_update', + kwargs={'course_id': self.course.id}, + ) + self.rerun_link_update_status_url = reverse( + 'cms.djangoapps.contentstore:v0:rerun_link_update_status', + kwargs={'course_id': self.course.id}, + ) + + prev_run_links_patcher = patch( + f'{VIEWS_MODULE}.enable_course_optimizer_check_prev_run_links', + return_value=True, + ) + prev_run_links_patcher.start() + self.addCleanup(prev_run_links_patcher.stop) + + # --- LinkCheckView (POST) --- + + @patch(f'{VIEWS_MODULE}.check_broken_links') + def test_editor_can_start_link_check(self, mock_task): + """Test that a course editor can start a link check.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_EDITOR.external_key, self.course.id) + resp = self.authorized_client.post(self.link_check_url) + assert resp.status_code == 200 + mock_task.delay.assert_called_once() + + @patch(f'{VIEWS_MODULE}.check_broken_links') + def test_staff_can_start_link_check(self, mock_task): + """Test that course staff can start a link check.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_STAFF.external_key, self.course.id) + resp = self.authorized_client.post(self.link_check_url) + assert resp.status_code == 200 + mock_task.delay.assert_called_once() + + @patch(f'{VIEWS_MODULE}.check_broken_links') + def test_auditor_cannot_start_link_check(self, mock_task): + """Test that a course auditor cannot start a link check.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_AUDITOR.external_key, self.course.id) + resp = self.authorized_client.post(self.link_check_url) + assert resp.status_code == 403 + mock_task.delay.assert_not_called() + + @patch(f'{VIEWS_MODULE}.check_broken_links') + def test_unauthorized_cannot_start_link_check(self, mock_task): + """Test that a user without a role in the course cannot start a link check.""" + resp = self.unauthorized_client.post(self.link_check_url) + assert resp.status_code == 403 + mock_task.delay.assert_not_called() + + # --- LinkCheckStatusView (GET) --- + + def test_editor_can_get_link_check_status(self): + """Test that a course editor can read the link check status.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_EDITOR.external_key, self.course.id) + resp = self.authorized_client.get(self.link_check_status_url) + assert resp.status_code == 200 + + def test_auditor_cannot_get_link_check_status(self): + """Test that a course auditor cannot read the link check status.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_AUDITOR.external_key, self.course.id) + resp = self.authorized_client.get(self.link_check_status_url) + assert resp.status_code == 403 + + def test_unauthorized_cannot_get_link_check_status(self): + """Test that a user without a role in the course cannot read the link check status.""" + resp = self.unauthorized_client.get(self.link_check_status_url) + assert resp.status_code == 403 + + # --- RerunLinkUpdateView (POST) --- + + @patch(f'{VIEWS_MODULE}.update_course_rerun_links') + def test_editor_can_start_rerun_link_update(self, mock_task): + """Test that a course editor can start a rerun link update.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_EDITOR.external_key, self.course.id) + resp = self.authorized_client.post(self.rerun_link_update_url, data={'action': 'all'}, format='json') + assert resp.status_code == 200 + mock_task.delay.assert_called_once() + + @patch(f'{VIEWS_MODULE}.update_course_rerun_links') + def test_auditor_cannot_start_rerun_link_update(self, mock_task): + """Test that a course auditor cannot start a rerun link update.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_AUDITOR.external_key, self.course.id) + resp = self.authorized_client.post(self.rerun_link_update_url, data={'action': 'all'}, format='json') + assert resp.status_code == 403 + mock_task.delay.assert_not_called() + + @patch(f'{VIEWS_MODULE}.update_course_rerun_links') + def test_unauthorized_cannot_start_rerun_link_update(self, mock_task): + """Test that a user without a role in the course cannot start a rerun link update.""" + resp = self.unauthorized_client.post(self.rerun_link_update_url, data={'action': 'all'}, format='json') + assert resp.status_code == 403 + mock_task.delay.assert_not_called() + + # --- RerunLinkUpdateStatusView (GET) --- + + def test_editor_can_get_rerun_link_update_status(self): + """Test that a course editor can read the rerun link update status.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_EDITOR.external_key, self.course.id) + resp = self.authorized_client.get(self.rerun_link_update_status_url) + assert resp.status_code == 200 + + def test_auditor_cannot_get_rerun_link_update_status(self): + """Test that a course auditor cannot read the rerun link update status.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_AUDITOR.external_key, self.course.id) + resp = self.authorized_client.get(self.rerun_link_update_status_url) + assert resp.status_code == 403 + + def test_unauthorized_cannot_get_rerun_link_update_status(self): + """Test that a user without a role in the course cannot read the rerun link update status.""" + resp = self.unauthorized_client.get(self.rerun_link_update_status_url) + assert resp.status_code == 403 + + # --- Superuser bypass --- + + @patch(f'{VIEWS_MODULE}.check_broken_links') + def test_superuser_can_start_link_check(self, mock_task): + """Test that a superuser bypasses the permission check and can start a link check.""" + resp = self.super_client.post(self.link_check_url) + assert resp.status_code == 200 + mock_task.delay.assert_called_once() diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py index 9b012d918e06..077ba1bd9aec 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py @@ -3,6 +3,7 @@ import edx_api_doc_tools as apidocs from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from openedx_authz.constants.permissions import COURSES_EDIT_COURSE_CONTENT from rest_framework import status from rest_framework.request import Request from rest_framework.response import Response @@ -21,8 +22,9 @@ ) from cms.djangoapps.contentstore.tasks import check_broken_links, update_course_rerun_links from cms.djangoapps.contentstore.toggles import enable_course_optimizer_check_prev_run_links -from common.djangoapps.student.auth import has_course_author_access, has_studio_read_access from common.djangoapps.util.json_request import JsonResponse +from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission +from openedx.core.djangoapps.authz.decorators import user_has_course_permission from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, verify_course_exists, view_auth_classes @@ -58,7 +60,12 @@ def post(self, request: Request, course_id: str): """ course_key = CourseKey.from_string(course_id) - if not has_studio_read_access(request.user, course_key): + if not user_has_course_permission( + request.user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + LegacyAuthoringPermission.READ, + ): self.permission_denied(request) check_broken_links.delay(request.user.id, course_id, request.LANGUAGE_CODE) @@ -206,7 +213,12 @@ def get(self, request: Request, course_id: str): } """ course_key = CourseKey.from_string(course_id) - if not has_course_author_access(request.user, course_key): + if not user_has_course_permission( + request.user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + LegacyAuthoringPermission.WRITE, + ): self.permission_denied(request) link_check_data = get_link_check_data(request, course_id) @@ -280,8 +292,12 @@ def post(self, request: Request, course_id: str): status=status.HTTP_404_NOT_FOUND, ) - # Check course author permissions - if not has_course_author_access(request.user, course_key): + if not user_has_course_permission( + request.user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + LegacyAuthoringPermission.WRITE, + ): self.permission_denied(request) if not enable_course_optimizer_check_prev_run_links(course_key): @@ -401,8 +417,12 @@ def get(self, request: Request, course_id: str): status=status.HTTP_404_NOT_FOUND, ) - # Check course author permissions - if not has_course_author_access(request.user, course_key): + if not user_has_course_permission( + request.user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + LegacyAuthoringPermission.WRITE, + ): self.permission_denied(request) if not enable_course_optimizer_check_prev_run_links(course_key): 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/serializers/group_configurations.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/group_configurations.py index fefeac8e748e..7b7fe132beca 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/group_configurations.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/group_configurations.py @@ -57,3 +57,4 @@ class CourseGroupConfigurationsSerializer(serializers.Serializer): ) should_show_enrollment_track = serializers.BooleanField() should_show_experiment_groups = serializers.BooleanField() + 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/group_configurations.py b/cms/djangoapps/contentstore/rest_api/v1/views/group_configurations.py index 4de2dd8a16b7..f6373d3febaa 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/group_configurations.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/group_configurations.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_GROUP_CONFIGURATIONS +from openedx_authz.constants.permissions import COURSES_MANAGE_GROUP_CONFIGURATIONS, COURSES_VIEW_GROUP_CONFIGURATIONS from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView @@ -10,7 +10,7 @@ from cms.djangoapps.contentstore.rest_api.v1.serializers import CourseGroupConfigurationsSerializer from cms.djangoapps.contentstore.utils import get_group_configurations_context from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission -from openedx.core.djangoapps.authz.decorators import authz_permission_required +from openedx.core.djangoapps.authz.decorators import authz_permission_required, user_has_course_permission from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, verify_course_exists, view_auth_classes from xmodule.modulestore.django import modulestore @@ -36,7 +36,7 @@ class CourseGroupConfigurationsView(DeveloperErrorViewMixin, APIView): ) @verify_course_exists() @authz_permission_required( - authz_permission=COURSES_MANAGE_GROUP_CONFIGURATIONS.identifier, + authz_permission=COURSES_VIEW_GROUP_CONFIGURATIONS.identifier, legacy_permission=LegacyAuthoringPermission.READ ) def get(self, request: Request, course_key: CourseKey): @@ -144,5 +144,11 @@ def get(self, request: Request, course_key: CourseKey): with store.bulk_operations(course_key): course = modulestore().get_course(course_key) group_configurations_context = get_group_configurations_context(course, store) + group_configurations_context['can_manage'] = user_has_course_permission( + request.user, + COURSES_MANAGE_GROUP_CONFIGURATIONS.identifier, + course_key, + LegacyAuthoringPermission.WRITE + ) serializer = CourseGroupConfigurationsSerializer(group_configurations_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 diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py index e12c3f6e7770..e7a2c88d463b 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py @@ -5,15 +5,17 @@ from django.test import RequestFactory from django.urls import reverse from edx_toggles.toggles.testutils import override_waffle_flag -from openedx_authz.constants.roles import COURSE_EDITOR +from openedx_authz.constants.roles import COURSE_EDITOR, COURSE_STAFF from rest_framework import status from cms.djangoapps.contentstore.config.waffle import CUSTOM_RELATIVE_DATES from cms.djangoapps.contentstore.rest_api.v1.mixins import PermissionAccessMixin -from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient, CourseTestCase from cms.djangoapps.contentstore.utils import get_lms_link_for_item, get_pages_and_resources_url from cms.djangoapps.contentstore.views.course import _course_outline_json +from common.djangoapps.student.roles import CourseStaffRole from common.djangoapps.student.tests.factories import UserFactory +from openedx.core import toggles as core_toggles from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES from xmodule.modulestore.tests.factories import BlockFactory, check_mongo_calls @@ -224,3 +226,72 @@ def test_superuser_can_access_without_authz_role(self): assert response.status_code == status.HTTP_200_OK assert "course_structure" in response.data + + +class CourseIndexViewReindexLinkAuthzTest(CourseAuthoringAuthzTestMixin, CourseTestCase): + """ + Regression tests: with AuthZ course authoring enabled, `reindex_link` (and therefore the + Reindex button in the course-authoring MFE) must follow the same access rules as the actual + reindex action, not be restricted to Global Staff only. + """ + + def setUp(self): + super().setUp() + self.url = reverse("cms.djangoapps.contentstore:v1:course_index", kwargs={"course_id": self.course.id}) + + def test_reindex_link_present_for_non_global_staff_with_authz_permission(self): + """A non-global-staff user granted the AuthZ course-staff role sees the reindex link.""" + self.add_user_to_role_in_course(self.authorized_user, COURSE_STAFF.external_key, self.course.id) + + response = self.authorized_client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + assert not self.authorized_user.is_staff + assert response.data["reindex_link"] == f"/course/{self.course.id}/search_reindex" + + def test_reindex_link_absent_without_reindex_permission(self): + """ + A user with enough AuthZ permission to view the course (COURSE_EDITOR) but without the + publish-content permission required to reindex does not see the reindex link. + """ + self.add_user_to_role_in_course(self.authorized_user, COURSE_EDITOR.external_key, self.course.id) + + response = self.authorized_client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + assert response.data["reindex_link"] is None + + +class CourseIndexViewReindexLinkLegacyTest(CourseTestCase): + """ + Regression tests: with AuthZ course authoring disabled, `reindex_link` must preserve the + legacy behavior of being restricted to Global Staff, even for users with legacy course-staff + write access. + """ + + def setUp(self): + super().setUp() + self.url = reverse("cms.djangoapps.contentstore:v1:course_index", kwargs={"course_id": self.course.id}) + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=False) + def test_reindex_link_absent_for_course_staff_without_global_staff(self): + """A course-staff member who isn't Global Staff does not see the reindex link.""" + course_staff_user, password = self.create_non_staff_user() + CourseStaffRole(self.course.id).add_users(course_staff_user) + + client = AjaxEnabledTestClient() + client.login(username=course_staff_user.username, password=password) + + response = client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + assert not course_staff_user.is_staff + assert response.data["reindex_link"] is None + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=False) + def test_reindex_link_present_for_global_staff(self): + """Global Staff still sees the reindex link (legacy behavior preserved).""" + response = self.client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + assert response.data["reindex_link"] == f"/course/{self.course.id}/search_reindex" diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_group_configurations.py index 496a50437104..f6f98d675899 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_group_configurations.py @@ -2,7 +2,7 @@ Unit tests for the course's setting group configuration. """ from django.urls import reverse -from openedx_authz.constants.roles import COURSE_DATA_RESEARCHER, COURSE_STAFF +from openedx_authz.constants.roles import COURSE_AUDITOR, COURSE_DATA_RESEARCHER, COURSE_EDITOR, COURSE_STAFF from rest_framework import status from rest_framework.test import APIClient @@ -111,3 +111,31 @@ def test_non_staff_user_cannot_access(self): resp = non_staff_client.get(self.get_url(self.course_key)) self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) # noqa: PT009 + + def test_staff_has_can_manage_true(self): + """User with COURSE_STAFF role gets can_manage=True in response.""" + resp = self.authorized_client.get(self.get_url(self.course_key)) + assert resp.status_code == status.HTTP_200_OK + assert resp.data["can_manage"] is True + + def test_editor_can_view_group_configurations(self): + """User with COURSE_EDITOR role can view group configurations.""" + editor_user = UserFactory() + editor_client = APIClient() + self.add_user_to_role(editor_user, COURSE_EDITOR.external_key) + editor_client.force_authenticate(user=editor_user) + + resp = editor_client.get(self.get_url(self.course_key)) + assert resp.status_code == status.HTTP_200_OK + assert resp.data["can_manage"] is True # editor has manage_group_configurations + + def test_auditor_can_view_group_configurations(self): + """User with COURSE_AUDITOR role can view group configurations.""" + auditor_user = UserFactory() + auditor_client = APIClient() + self.add_user_to_role(auditor_user, COURSE_AUDITOR.external_key) + auditor_client.force_authenticate(user=auditor_user) + + resp = auditor_client.get(self.get_url(self.course_key)) + assert resp.status_code == status.HTTP_200_OK + assert resp.data["can_manage"] is False # auditor has view only 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/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/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..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 @@ -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,134 @@ 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 + + +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/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/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/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 35818125efab..2c061fcab54f 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1898,14 +1898,14 @@ def _get_course_index_context(request, course_key, course_block): _course_outline_json, _deprecated_blocks_info, course_outline_initial_state, + user_can_reindex_course, ) from openedx.core.djangoapps.content_staging import api as content_staging_api lms_link = get_lms_link_for_item(course_block.location) reindex_link = None - if settings.FEATURES.get('ENABLE_COURSEWARE_INDEX', False): - if GlobalStaff().has_user(request.user): - reindex_link = f"/course/{str(course_key)}/search_reindex" + if settings.FEATURES.get('ENABLE_COURSEWARE_INDEX', False) and user_can_reindex_course(course_key, request.user): + reindex_link = f"/course/{str(course_key)}/search_reindex" sections = course_block.get_children() course_structure = _course_outline_json(request, course_block) locator_to_show = request.GET.get('show', None) diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index 4d60640cb2cf..0727b26235ed 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 @@ -26,7 +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.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 @@ -129,10 +133,63 @@ def xblock_handler(request, usage_key_string=None): return handle_xblock(request, usage_key_string) +def _user_can_edit_course_content(user, course_key): + """ + 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. + """ + return user_has_course_permission( + user, + COURSES_EDIT_COURSE_CONTENT.identifier, + course_key, + legacy_permission=LegacyAuthoringPermission.WRITE, + ) + + +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, + ) + + @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. @@ -142,7 +199,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") @@ -198,7 +260,13 @@ def xblock_view_handler(request, usage_key_string, view_name): 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) + + # 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 @@ -242,6 +310,8 @@ 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, + "can_manage_tags": can_manage_tags, + "can_edit_title": can_edit_title, "root_xblock": xblock if (view_name == "container_preview") else None, @@ -299,7 +369,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 +446,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/course.py b/cms/djangoapps/contentstore/views/course.py index 2ee588195210..ebae8b2c740e 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -42,6 +42,7 @@ COURSES_PUBLISH_COURSE_CONTENT, COURSES_VIEW_COURSE, COURSES_VIEW_COURSE_UPDATES, + COURSES_VIEW_GROUP_CONFIGURATIONS, COURSES_VIEW_PAGES_AND_RESOURCES, ) from organizations.api import add_organization_course, ensure_organization @@ -194,16 +195,33 @@ def get_course_and_check_manage_group_configurations_access(course_key, user, de return _get_course_block(course_key, depth) -def reindex_course_and_check_access(course_key, user): +def user_can_reindex_course(course_key, user): """ - Internal method used to restart indexing on a course. + Returns True if `user` is allowed to trigger a search reindex for `course_key`. + + Mirrors the access rules enforced by `course_search_index_handler`, so callers that only + need to know whether the action is allowed (e.g. to decide whether to show a reindex link) + stay in sync with the rules enforced when the reindex is actually triggered. """ - if not user_has_course_permission( + is_authz_enabled = core_toggles.AUTHZ_COURSE_AUTHORING_FLAG.is_enabled(course_key) + if not is_authz_enabled and not GlobalStaff().has_user(user): + # When AuthZ is disabled, restrict to global staff (legacy behavior). + # When AuthZ is enabled, access control is enforced by the AuthZ layer, + # which includes staff/superuser checks and course-level permissions. + return False + return user_has_course_permission( user=user, authz_permission=COURSES_PUBLISH_COURSE_CONTENT.identifier, course_key=course_key, legacy_permission=LegacyAuthoringPermission.WRITE - ): + ) + + +def reindex_course_and_check_access(course_key, user): + """ + Internal method used to restart indexing on a course. + """ + if not user_can_reindex_course(course_key, user): raise PermissionDenied() return CoursewareSearchIndexer.do_course_reindex(modulestore(), course_key) @@ -380,12 +398,6 @@ def course_search_index_handler(request, course_key_string): json: return status of indexing task """ course_key = CourseKey.from_string(course_key_string) - is_authz_enabled = core_toggles.AUTHZ_COURSE_AUTHORING_FLAG.is_enabled(course_key) - if not is_authz_enabled and not GlobalStaff().has_user(request.user): - # When AuthZ is disabled, restrict to global staff (legacy behavior). - # When AuthZ is enabled, access control is enforced by the AuthZ layer, - # which includes staff/superuser checks and course-level permissions. - raise PermissionDenied() content_type = request.META.get('CONTENT_TYPE', None) if content_type is None: content_type = "application/json; charset=utf-8" @@ -429,19 +441,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 +1346,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']) @@ -1961,6 +1953,20 @@ def group_configurations_list_handler(request, course_key_string): json: create new group configuration """ course_key = CourseKey.from_string(course_key_string) + + if request.method == 'GET': + # GET only redirects to the MFE (html) or returns 406 (json) + if not user_has_course_permission( + user=request.user, + authz_permission=COURSES_VIEW_GROUP_CONFIGURATIONS.identifier, + course_key=course_key, + legacy_permission=LegacyAuthoringPermission.READ, + ): + raise PermissionDenied() + if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): + return redirect(get_group_configurations_url(course_key)) + return HttpResponse(status=406) + store = modulestore() with store.bulk_operations(course_key): course = get_course_and_check_manage_group_configurations_access(course_key, request.user) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index d3ca840c01fd..420def3e4f44 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -318,6 +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 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 ( @@ -334,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') @@ -362,7 +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 + # 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 15468c63bd9f..274317971bc8 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,466 @@ 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`` and ``courses.manage_tags`` permissions. + + ``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 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 (all three default to permissive) + or when the flag is on and the user has edit or tag-management access. + """ + + AUTHZ_FLAG_PATH = ( + "openedx.core.djangoapps.authz.decorators.enable_authz_course_authoring" + ) + # Patch user_has_course_permission at the block.py binding so the + # 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 + # 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"' + # 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): + """ + 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 _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, 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): + 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, + 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( + 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 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, 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. + + ``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 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. + + 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 = ( + "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, can_edit_course_content=True): + """ + Build a ``user_has_course_permission`` side effect that always grants + ``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 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 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, 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() + + 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, + 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, + 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 + + 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): """Tests for '/xblock' DELETE url.""" @@ -3628,6 +4093,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. diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index 83c20f3c4fbb..b6f3aee46ae1 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -10,7 +10,7 @@ import ddt from django.test import Client from edx_toggles.toggles.testutils import override_waffle_flag -from openedx_authz.constants.roles import COURSE_DATA_RESEARCHER, COURSE_STAFF +from openedx_authz.constants.roles import COURSE_AUDITOR, COURSE_DATA_RESEARCHER, COURSE_EDITOR, COURSE_STAFF from rest_framework import status from cms.djangoapps.contentstore import toggles @@ -1253,6 +1253,7 @@ def test_update_usage_info_no_message(self): """ self.verify_validation_update_usage_info(None, None) # pylint: disable=no-value-for-parameter +@ddt.ddt class PostGroupConfigurationsListHandlerAuthzTest(CourseAuthzTestMixin, BaseCourseViewTest): """ Tests endpoint used to create new Course Group Configurations @@ -1361,6 +1362,43 @@ def test_non_staff_user_cannot_access(self): ) self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) # noqa: PT009 + def test_editor_can_write(self): + """ + Course editor has manage_group_configurations and can POST (create). + This is the write-side counterpart to the editor's view access. + """ + editor = UserFactory(password=self.password) + self.add_user_to_role(editor, COURSE_EDITOR.external_key) + client = Client() + client.login(username=editor.username, password=self.password) + + resp = client.post( + self.get_url(self.course_key), + data=json.dumps(GROUP_CONFIGURATION_JSON), + content_type='application/json', + HTTP_ACCEPT='application/json', + ) + assert resp.status_code == status.HTTP_201_CREATED + + def test_auditor_cannot_write(self): + """ + Course auditor has only view_group_configurations (no manage), so it can + read the list but must be denied POST (create). This is the write-side + counterpart to the auditor's view access. + """ + auditor = UserFactory(password=self.password) + self.add_user_to_role(auditor, COURSE_AUDITOR.external_key) + client = Client() + client.login(username=auditor.username, password=self.password) + + resp = client.post( + self.get_url(self.course_key), + data=json.dumps(GROUP_CONFIGURATION_JSON), + content_type='application/json', + HTTP_ACCEPT='application/json', + ) + assert resp.status_code == status.HTTP_403_FORBIDDEN + class PutGroupConfigurationsDetailHandlerAuthzTest(CourseAuthzTestMixin, BaseCourseViewTest): """ 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) diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 5dcafb96e02a..24dbcd03e19a 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -165,6 +165,7 @@ % endif + % if can_edit or can_manage_tags or can_edit_title: