diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index d16e941b09e1..4ef303496fd9 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 bf2e6c63a2ad..1b70e02b55ec 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 a02e7551fc3a..ff6cf355ff70 100644 --- a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py +++ b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py @@ -426,18 +426,10 @@ class TestCourseRerunAuthz( """ Tests for course rerun behavior when authz.enable_course_authoring is enabled. - Verifies that: - - Rerun succeeds without calling add_instructor pre-task (which would fail - because CourseOverview doesn't exist yet). - - add_instructor is called in the task after clone_course, when CourseOverview - can be resolved. - - The initiating user can see in-process rerun status via created_user check. - - TODO: These tests cover a temporary workaround needed while (1) the authz system - doesn't support pre-assigning roles without a CourseOverview, and (2) we support - both authz and legacy systems simultaneously. Once openedx/openedx-authz#352 is - implemented, this class can be simplified — the conditional skip of add_instructor - and the created_user visibility fallback will no longer be needed. + Verifies that rerun succeeds and grants proper access even though add_instructor + is called for the destination course key before its CourseOverview exists + (openedx-authz#352 makes role assignment succeed ahead of the object, backfilling + the link once clone_course creates it). """ def setUp(self): @@ -467,8 +459,8 @@ def setUp(self): def test_rerun_succeeds_with_authz_enabled(self): """ Test that course rerun completes successfully when authz.enable_course_authoring - is enabled. Previously this would fail with CourseOverview.DoesNotExist because - add_instructor was called before the course was cloned. + is enabled, granting the initiating user instructor/staff access on the + destination course before it's cloned. """ response = self.staff_client.ajax_post(self.url, { 'source_course_key': str(self.source_course_key), @@ -516,8 +508,7 @@ def test_rerun_does_not_create_legacy_roles_with_authz_enabled(self): def test_rerun_grants_authz_permissions_after_clone(self): """ Test that after a successful rerun with authz enabled, the user has proper - permissions via the authz layer (add_instructor is called in the task after - clone_course completes and CourseOverview is available). + permissions via the authz layer on the newly created destination course. """ response = self.staff_client.ajax_post(self.url, { 'source_course_key': str(self.source_course_key), @@ -537,8 +528,8 @@ def test_rerun_grants_authz_permissions_after_clone(self): def test_in_process_rerun_visible_to_initiating_user(self): """ Test that the user who initiated the rerun can see the in-process status - via the created_user check in get_in_process_course_actions, even when - authz permissions haven't been fully established yet. + in get_in_process_course_actions, via the instructor/staff access already + granted on the destination course. """ dest_course_key = CourseLocator(org='testorg', course='101', run='in_progress_rerun') CourseRerunState.objects.initiated( diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 90eaddf06576..1e3238a6f89b 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -420,19 +420,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 ) ] @@ -1340,17 +1329,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']) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index a02051a1943b..5c41156d917d 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -835,7 +835,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==1.21.1 +openedx-authz==1.22.0 # via -r requirements/edx/kernel.in openedx-calc==5.0.0 # via diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 12d2fa586a07..d0b7a8d69b05 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1376,7 +1376,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==1.21.1 +openedx-authz==1.22.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 9bd0bfdd7a16..9233434eaa4d 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1014,7 +1014,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==1.21.1 +openedx-authz==1.22.0 # via -r requirements/edx/base.txt openedx-calc==5.0.0 # via diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index ef101a0fdbfb..da2372abb410 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1053,7 +1053,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==1.21.1 +openedx-authz==1.22.0 # via -r requirements/edx/base.txt openedx-calc==5.0.0 # via