Skip to content

revert: Cannot rerun courses - authz role assignment expects rerun to already exist [verawood] #39283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
BryanttV wants to merge 1 commit into
base: release/verawood
Choose a base branch
from
+18 −102
Draft

revert: Cannot rerun courses - authz role assignment expects rerun to already exist [verawood] #39283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions cms/djangoapps/contentstore/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
55 changes: 4 additions & 51 deletions cms/djangoapps/contentstore/tests/test_clone_course.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
27 changes: 9 additions & 18 deletions cms/djangoapps/contentstore/tests/test_course_create_rerun.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand All @@ -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(
Expand Down
28 changes: 4 additions & 24 deletions cms/djangoapps/contentstore/views/course.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,19 +429,8 @@ def get_in_process_course_actions(request):
exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED},
should_display=True,
)
if (
# The user who initiated the rerun can always see its status.
# This is needed because when the authz flag is enabled, permission
# checks require a CourseOverview which doesn't exist until the
# rerun task clones the course.
# TODO: This created_user fallback is a temporary workaround until
# openedx/openedx-authz#352 is implemented. Once authz supports
# pre-assigning roles without a CourseOverview, this check can be removed
# and the standard permission check will suffice.
course.created_user == request.user
or user_has_course_permission(
request.user, COURSES_VIEW_COURSE.identifier, course.course_key, LegacyAuthoringPermission.READ
)
if user_has_course_permission(
request.user, COURSES_VIEW_COURSE.identifier, course.course_key, LegacyAuthoringPermission.READ
)
]

Expand Down Expand Up @@ -1345,17 +1334,8 @@ def rerun_course(user, source_course_key, org, number, run, fields, background=T
raise PermissionDenied()

# Make sure user has instructor and staff access to the destination course
# so the user can see the updated status for that course.
# When authz is enabled, we skip this because the authz layer requires a
# CourseOverview (which doesn't exist until the course is cloned in the task).
# In that case, visibility of the rerun status is granted by checking
# created_user on CourseRerunState instead.
# TODO: This conditional is a temporary workaround until openedx/openedx-authz#352
# is implemented (pre-assigning roles without a CourseOverview). Once resolved,
# add_instructor can be called unconditionally here and the created_user fallback
# in get_in_process_course_actions can be removed.
if not enable_authz_course_authoring(destination_course_key):
add_instructor(destination_course_key, user, user)
# so the user can see the updated status for that course
add_instructor(destination_course_key, user, user)

# Mark the action as initiated
CourseRerunState.objects.initiated(source_course_key, destination_course_key, user, fields['display_name'])
Expand Down
Loading