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..5b03b32557fc 100644 --- a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py +++ b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py @@ -15,25 +15,17 @@ from django.urls import reverse from edx_toggles.toggles.testutils import override_waffle_flag from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locator import CourseLocator from organizations.api import add_organization, get_course_organizations, get_organization_by_short_name from organizations.exceptions import InvalidOrganizationException from organizations.models import Organization from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient, parse_json -from cms.djangoapps.contentstore.views.course import ( - get_allowed_organizations, - get_in_process_course_actions, - user_can_create_organizations, -) +from cms.djangoapps.contentstore.views.course import get_allowed_organizations, user_can_create_organizations from cms.djangoapps.course_creators.admin import CourseCreatorAdmin from cms.djangoapps.course_creators.models import CourseCreator -from common.djangoapps.course_action_state.models import CourseRerunState -from common.djangoapps.student.auth import has_course_author_access, update_org_role -from common.djangoapps.student.models import CourseAccessRole +from common.djangoapps.student.auth import update_org_role from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, OrgContentCreatorRole from common.djangoapps.student.tests.factories import AdminFactory, UserFactory -from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin from openedx.core.toggles import AUTHZ_COURSE_AUTHORING_FLAG from xmodule.course_block import CourseFields from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -417,234 +409,3 @@ def test_create_course_staff(self, authz_enabled): }) assert response.status_code == 200 - - -class TestCourseRerunAuthz( - CourseAuthoringAuthzTestMixin, - ModuleStoreTestCase, -): - """ - 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. - """ - - def setUp(self): - super().setUp() - - self.url = reverse("course_handler") - - # Create a source course - self.source_course = CourseFactory.create( - org='testorg', - number='101', - run='2025_Spring', - display_name='Source Course', - ) - self.source_course_key = self.source_course.id - - # Give the staff user instructor/staff access to the source course - for role in [CourseInstructorRole, CourseStaffRole]: - role(self.source_course_key).add_users(self.staff_user) - - self.staff_client = AjaxEnabledTestClient() - self.staff_client.login( - username=self.staff_user.username, - password=self.password, - ) - - 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. - """ - response = self.staff_client.ajax_post(self.url, { - 'source_course_key': str(self.source_course_key), - 'org': self.source_course_key.org, - 'course': self.source_course_key.course, - 'run': 'rerun_authz', - 'display_name': 'Rerun with AuthZ', - }) - - assert response.status_code == 200 - data = parse_json(response) - dest_course_key = CourseKey.from_string(data['destination_course_key']) - - # Verify the rerun completed (task runs eagerly in tests) - dest_course = self.store.get_course(dest_course_key) - assert dest_course is not None - assert dest_course.display_name == 'Rerun with AuthZ' - - def test_rerun_does_not_create_legacy_roles_with_authz_enabled(self): - """ - Test that when authz.enable_course_authoring is enabled, the rerun does not - create legacy CourseAccessRole records pre-task. Legacy roles should not exist - when authz is enabled to avoid database inconsistencies. - """ - response = self.staff_client.ajax_post(self.url, { - 'source_course_key': str(self.source_course_key), - 'org': self.source_course_key.org, - 'course': self.source_course_key.course, - 'run': 'rerun_no_legacy', - 'display_name': 'Rerun No Legacy Roles', - }) - - assert response.status_code == 200 - data = parse_json(response) - dest_course_key = CourseKey.from_string(data['destination_course_key']) - - # Verify no legacy CourseAccessRole records were created for the destination course. - # When authz is enabled, permissions are managed entirely through the authz layer. - legacy_roles = CourseAccessRole.objects.filter( - user=self.staff_user, - course_id=dest_course_key, - ) - assert not legacy_roles.exists() - - 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). - """ - response = self.staff_client.ajax_post(self.url, { - 'source_course_key': str(self.source_course_key), - 'org': self.source_course_key.org, - 'course': self.source_course_key.course, - 'run': 'rerun_perms', - 'display_name': 'Rerun Permissions Test', - }) - - assert response.status_code == 200 - data = parse_json(response) - dest_course_key = CourseKey.from_string(data['destination_course_key']) - - # Verify the user has author access to the new course via the role system - assert has_course_author_access(self.staff_user, dest_course_key) - - 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. - """ - dest_course_key = CourseLocator(org='testorg', course='101', run='in_progress_rerun') - CourseRerunState.objects.initiated( - self.source_course_key, dest_course_key, self.staff_user, 'In Progress Rerun' - ) - - # Build a request for the staff user who initiated the rerun - request = RequestFactory().get('/') - request.user = self.staff_user - - # The user should see the in-process action even without explicit course permissions - in_process_actions = get_in_process_course_actions(request) - action_course_keys = [action.course_key for action in in_process_actions] - assert dest_course_key in action_course_keys - - def test_in_process_rerun_not_visible_to_other_users(self): - """ - Test that a user who did NOT initiate the rerun cannot see the in-process - status (unless they have course permissions). - """ - dest_course_key = CourseLocator(org='testorg', course='101', run='other_user_rerun') - CourseRerunState.objects.initiated( - self.source_course_key, dest_course_key, self.staff_user, 'Other User Rerun' - ) - - # Build a request for a different user who did not initiate the rerun - request = RequestFactory().get('/') - request.user = self.unauthorized_user - - # The other user should NOT see the in-process action - in_process_actions = get_in_process_course_actions(request) - action_course_keys = [action.course_key for action in in_process_actions] - assert dest_course_key not in action_course_keys - - -class TestCourseRerunLegacy(ModuleStoreTestCase): - """ - Tests for course rerun behavior when authz.enable_course_authoring is DISABLED - (legacy mode). Verifies the original behavior is preserved. - """ - - def setUp(self): - super().setUp() - - self.url = reverse("course_handler") - self.user = UserFactory() - self.client = AjaxEnabledTestClient() - self.client.login(username=self.user.username, password=self.TEST_PASSWORD) - - # Create a source course and give user access - self.source_course = CourseFactory.create( - org='legacyorg', - number='201', - run='2025_Fall', - display_name='Legacy Source Course', - ) - self.source_course_key = self.source_course.id - - for role in [CourseInstructorRole, CourseStaffRole]: - role(self.source_course_key).add_users(self.user) - - def test_rerun_creates_legacy_roles_pre_task(self): - """ - Test that when authz is disabled (legacy mode), add_instructor is called - pre-task and creates CourseAccessRole records immediately. - """ - response = self.client.ajax_post(self.url, { - 'source_course_key': str(self.source_course_key), - 'org': self.source_course_key.org, - 'course': self.source_course_key.course, - 'run': 'legacy_rerun', - 'display_name': 'Legacy Rerun', - }) - - assert response.status_code == 200 - data = parse_json(response) - dest_course_key = CourseKey.from_string(data['destination_course_key']) - - # Verify legacy CourseAccessRole records exist for the destination course - legacy_roles = CourseAccessRole.objects.filter( - user=self.user, - course_id=dest_course_key, - ) - assert legacy_roles.filter(role='instructor').exists() - assert legacy_roles.filter(role='staff').exists() - - def test_rerun_succeeds_with_authz_disabled(self): - """ - Test that course rerun still works correctly when authz is disabled. - """ - response = self.client.ajax_post(self.url, { - 'source_course_key': str(self.source_course_key), - 'org': self.source_course_key.org, - 'course': self.source_course_key.course, - 'run': 'legacy_rerun_success', - 'display_name': 'Legacy Rerun Success', - }) - - assert response.status_code == 200 - data = parse_json(response) - dest_course_key = CourseKey.from_string(data['destination_course_key']) - - # Verify the course was created - dest_course = self.store.get_course(dest_course_key) - assert dest_course is not None - assert dest_course.display_name == 'Legacy Rerun Success' - - # Verify author access - assert has_course_author_access(self.user, dest_course_key) 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