diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 49f07b03..297f6e93 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,14 @@ Change Log Unreleased ********** +1.12.0 - 2026-04-20 +******************* + +Added +===== + +* Add automatic course authoring migration mechanism triggered by the ``authz.enable_course_authoring`` waffle flag when it is toggled at course or organization scope. + 1.11.0 - 2026-04-16 ******************* diff --git a/openedx_authz/__init__.py b/openedx_authz/__init__.py index c6f61fe9..e04c85ea 100644 --- a/openedx_authz/__init__.py +++ b/openedx_authz/__init__.py @@ -4,6 +4,6 @@ import os -__version__ = "1.11.0" +__version__ = "1.12.0" ROOT_DIRECTORY = os.path.dirname(os.path.abspath(__file__)) diff --git a/openedx_authz/admin.py b/openedx_authz/admin.py index e2285158..da343691 100644 --- a/openedx_authz/admin.py +++ b/openedx_authz/admin.py @@ -1,10 +1,24 @@ """Admin configuration for openedx_authz.""" +import json + from casbin_adapter.models import CasbinRule from django import forms from django.contrib import admin +from django.utils.html import format_html + +from openedx_authz.models import AuthzCourseAuthoringMigrationRun, ExtendedCasbinRule -from openedx_authz.models import ExtendedCasbinRule + +def pretty_json(value) -> str: + """Return an indented JSON representation of a value.""" + if value is None: + return "-" + try: + formatted = json.dumps(value, indent=2, ensure_ascii=False) + except (TypeError, ValueError): + return str(value) + return format_html("
{}
", formatted) class CasbinRuleForm(forms.ModelForm): @@ -48,3 +62,28 @@ class CasbinRuleAdmin(admin.ModelAdmin): # TODO: In a future, possibly we should only show an inline for the rules that # have an extended rule, and show the subject and scope information in detail. inlines = [ExtendedCasbinRuleInline] + + +@admin.register(AuthzCourseAuthoringMigrationRun) +class AuthzCourseAuthoringMigrationRunAdmin(admin.ModelAdmin): + """Admin for AuthzCourseAuthoringMigrationRun to display additional metadata.""" + + list_display = ("id", "scope_type", "scope_key", "migration_type", "status", "created_at", "updated_at") + search_fields = ("scope_type", "scope_key", "migration_type", "status") + list_filter = ("scope_type", "migration_type", "status") + readonly_fields = ( + "scope_type", + "scope_key", + "migration_type", + "status", + "pretty_metadata", + "completed_at", + "created_at", + "updated_at", + ) + fields = readonly_fields + + @admin.display(description="Metadata") + def pretty_metadata(self, obj): + """Return formatted JSON for the metadata field.""" + return pretty_json(obj.metadata) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 8603934b..93f27e1c 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -6,8 +6,11 @@ import logging from collections import defaultdict +from dataclasses import dataclass, field +from enum import StrEnum, auto from casbin import Enforcer +from django.db import IntegrityError, transaction from django.db.models import Q from opaque_keys.edx.django.models import CourseKeyField @@ -24,6 +27,7 @@ LIBRARY_AUTHOR, LIBRARY_USER, ) +from openedx_authz.models.authz_migration import AuthzCourseAuthoringMigrationRun, MigrationType, ScopeType logger = logging.getLogger(__name__) @@ -34,6 +38,49 @@ COURSE_ROLE_EQUIVALENCES = {v: k for k, v in LEGACY_COURSE_ROLE_EQUIVALENCES.items()} +class MigrationErrorReason(StrEnum): + """String constants for categorising why a single role assignment failed during migration.""" + + # Forward (legacy → authz) reasons + UNKNOWN_ROLE = auto() + NO_SCOPE = auto() + ASSIGNMENT_FAILED = auto() + + # Rollback (authz → legacy) reasons + UNEXPECTED_SCOPE_TYPE = auto() + NO_LEGACY_EQUIVALENT = auto() + UNEXPECTED_ERROR = auto() + + # Forward and rollback reasons + SKIPPED_FOR_FLAG_OVERRIDE = auto() + + +@dataclass +class MigrationMetadata: + """Normalised representation of a single role-assignment outcome during migration. + + Can represent both successful and failed assignments. Populate ``reason`` / + ``details`` only for failures; leave them empty for successes. + + Attributes: + subject: External key of the user whose assignment was attempted. + role: Role external key (new-style for rollback, legacy key for forward). + scope: Scope external key, or empty string when not yet determined. + reason: One of the ``MigrationErrorReason`` constants; empty for successes. + details: Optional human-readable extra context (e.g. exception message). + """ + + subject: str + role: str + scope: str = field(default="") + reason: MigrationErrorReason | None = field(default=None) + details: str = field(default="") + + def to_dict(self) -> dict: + """Convert the migration metadata to a dictionary.""" + return {k: v for k, v in self.__dict__.items() if v} + + def migrate_policy_between_enforcers( source_enforcer: Enforcer, target_enforcer: Enforcer, @@ -187,7 +234,14 @@ def _validate_migration_input(course_id_list, org_id): ) -def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_list, org_id, delete_after_migration): +# pylint: disable=too-many-statements +def migrate_legacy_course_roles_to_authz( + course_access_role_model, + course_id_list, + org_id, + delete_after_migration, + excluded_course_ids: frozenset[str] = frozenset(), +) -> tuple[list[MigrationMetadata], list[MigrationMetadata]]: """ Migrate legacy course role data to the new Casbin-based authorization model. This function reads legacy permissions from the CourseAccessRole model @@ -216,6 +270,8 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis param course_id_list: Optional list of course IDs to filter the migration. param org_id: Optional organization ID to filter the migration. param delete_after_migration: Whether to delete successfully migrated legacy permissions after migration. + param excluded_course_ids: Course ids for which migration is skipped (course-level + override opposes the org-level transition). """ _validate_migration_input(course_id_list, org_id) @@ -235,19 +291,31 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis .select_related("user") ) - # List to keep track of any permissions that could not be migrated - permissions_with_errors = [] - permissions_with_no_errors = [] + permissions_with_errors: list[MigrationMetadata] = [] + permissions_with_no_errors: list[MigrationMetadata] = [] + permission_ids: list[int] = [] for permission in legacy_permissions: # Migrate the permission to the new model + migration_metadata = MigrationMetadata(subject=permission.user.username, role=permission.role) role = LEGACY_COURSE_ROLE_EQUIVALENCES.get(permission.role) if role is None: # This should not happen as there are no more access_levels defined # in CourseAccessRole, log and skip logger.error(f"Unknown access level: {permission.role} for User: {permission.user}") - permissions_with_errors.append(permission) + migration_metadata.reason = MigrationErrorReason.UNKNOWN_ROLE + migration_metadata.details = f"Unknown access level: {permission.role} for User: {permission.user.username}" + permissions_with_errors.append(migration_metadata) + continue + + if permission.course_id and str(permission.course_id) in excluded_course_ids: + migration_metadata.reason = MigrationErrorReason.SKIPPED_FOR_FLAG_OVERRIDE + migration_metadata.scope = str(permission.course_id) + migration_metadata.details = ( + "Course-level authoring flag override opposes this organization-level transition" + ) + permissions_with_errors.append(migration_metadata) continue if permission.course_id: @@ -259,7 +327,9 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis logger.error( f"Permission for User: {permission.user.username} has neither course_id nor org defined, skipping." ) - permissions_with_errors.append(permission) + migration_metadata.reason = MigrationErrorReason.NO_SCOPE + migration_metadata.details = f"User '{permission.user.username}' has neither course_id nor org defined" + permissions_with_errors.append(migration_metadata) continue # Permission applied to individual user @@ -279,23 +349,35 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis f"to Role: {role} in Scope: {permission.course_id} " "user may already have this permission assigned" ) - permissions_with_errors.append(permission) + migration_metadata.scope = scope_external_key + migration_metadata.reason = MigrationErrorReason.ASSIGNMENT_FAILED + migration_metadata.details = f"User '{permission.user.username}' may already have this permission assigned" + permissions_with_errors.append(migration_metadata) continue - permissions_with_no_errors.append(permission) + migration_metadata.scope = scope_external_key + permissions_with_no_errors.append(migration_metadata) + + permission_ids.append(permission.id) if delete_after_migration: # Only delete permissions that were successfully migrated to avoid data loss. - course_access_role_model.objects.filter(id__in=[p.id for p in permissions_with_no_errors]).delete() + course_access_role_model.objects.filter(id__in=permission_ids).delete() logger.info(f"Deleted {len(permissions_with_no_errors)} legacy permissions after successful migration.") logger.info(f"Retained {len(permissions_with_errors)} legacy permissions that had errors during migration.") return permissions_with_errors, permissions_with_no_errors +# pylint: disable=too-many-statements,too-many-positional-arguments def migrate_authz_to_legacy_course_roles( - course_access_role_model, user_subject_model, course_id_list, org_id, delete_after_migration -): + course_access_role_model, + user_subject_model, + course_id_list, + org_id, + delete_after_migration, + excluded_course_ids: frozenset[str] = frozenset(), +) -> tuple[list[MigrationMetadata], list[MigrationMetadata]]: """ Migrate permissions from the new Casbin-based authorization model back to the legacy CourseAccessRole model. This function reads permissions from the Casbin enforcer and creates equivalent entries in the @@ -315,16 +397,15 @@ def migrate_authz_to_legacy_course_roles( is intended to run within a Django migration context, where direct model imports can cause issues. param course_id_list: Optional list of course IDs to filter the migration. param org_id: Optional organization ID to filter the migration. - param delete_after_migration: Whether to unassign successfully migrated permissions - from the new model after migration. + param delete_after_migration: Whether to unassign successfully migrated permissions from + the new model after migration. + param excluded_course_ids: Course ids for which rollback is skipped (course-level override + opposes the org-level transition). """ _validate_migration_input(course_id_list, org_id) role_assignments = get_all_role_assignments_per_scope_type( - scope_types=( - CourseOverviewData, - OrgCourseOverviewGlobData, - ) + scope_types=(CourseOverviewData, OrgCourseOverviewGlobData) ) # Two cases here: @@ -343,8 +424,8 @@ def migrate_authz_to_legacy_course_roles( and role_assignment.scope.course_id in course_id_list ] - roles_with_errors = [] - roles_with_no_errors = [] + roles_with_errors: list[MigrationMetadata] = [] + roles_with_no_errors: list[MigrationMetadata] = [] unassignments = defaultdict(list) user_external_keys = {assignment.subject.external_key for assignment in role_assignments} @@ -361,9 +442,32 @@ def migrate_authz_to_legacy_course_roles( role_external_key = role_assignment.roles[0].external_key scope_external_key = role_assignment.scope.external_key + migration_metadata = MigrationMetadata( + subject=user_external_key, role=role_external_key, scope=scope_external_key + ) + + if isinstance(role_assignment.scope, CourseOverviewData) and scope_external_key in excluded_course_ids: + migration_metadata.reason = MigrationErrorReason.SKIPPED_FOR_FLAG_OVERRIDE + migration_metadata.details = ( + "Course-level authoring flag override opposes this organization-level transition" + ) + roles_with_errors.append(migration_metadata) + continue + + legacy_role = COURSE_ROLE_EQUIVALENCES.get(role_external_key) + if legacy_role is None: + logger.error( + f"No legacy equivalent found for role: {role_external_key}, " + f"user: {user_external_key}, scope: {scope_external_key}. Skipping." + ) + migration_metadata.reason = MigrationErrorReason.NO_LEGACY_EQUIVALENT + migration_metadata.details = f"Role '{role_external_key}' has no legacy equivalent." + roles_with_errors.append(migration_metadata) + continue + course_access_role_kwargs = { "user": users_by_username[user_external_key], - "role": COURSE_ROLE_EQUIVALENCES[role_external_key], + "role": legacy_role, } if isinstance(role_assignment.scope, CourseOverviewData): @@ -371,6 +475,7 @@ def migrate_authz_to_legacy_course_roles( course_access_role_kwargs["course_id"] = scope_external_key elif isinstance(role_assignment.scope, OrgCourseOverviewGlobData): course_access_role_kwargs["org"] = role_assignment.scope.org + course_access_role_kwargs["course_id"] = CourseKeyField.Empty else: # This would only happen for course roles assigned instance-wide # which is not yet supported @@ -378,11 +483,13 @@ def migrate_authz_to_legacy_course_roles( f"Unexpected scope type: {type(role_assignment.scope)} for RoleAssignment with " f"scope: {scope_external_key}, user: {user_external_key} and role: {role_external_key}, skipping." ) - roles_with_errors.append(role_assignment) + migration_metadata.reason = MigrationErrorReason.UNEXPECTED_SCOPE_TYPE + migration_metadata.details = f"Unexpected scope type: {type(role_assignment.scope).__name__}" + roles_with_errors.append(migration_metadata) continue course_access_role_model.objects.get_or_create(**course_access_role_kwargs) - roles_with_no_errors.append(role_assignment) + roles_with_no_errors.append(migration_metadata) logger.info( f"Successfully rolled back RoleAssignment for User: {user_external_key} " @@ -398,7 +505,9 @@ def migrate_authz_to_legacy_course_roles( f"Error rolling back RoleAssignment for User: {role_assignment.subject.external_key} " f"in Role: {role_assignment.roles[0].external_key} and Scope: {role_assignment.scope.external_key}: {e}" ) - roles_with_errors.append(role_assignment) + migration_metadata.reason = MigrationErrorReason.UNEXPECTED_ERROR + migration_metadata.details = str(e) + roles_with_errors.append(migration_metadata) # Once the loop is done, we can log summary of unassignments # and perform batch unassignment if delete_after_migration is True @@ -407,7 +516,7 @@ def migrate_authz_to_legacy_course_roles( logger.info(f"Total of {total_unassignments} role assignments unassigned after successful rollback migration.") for (role_external_key, scope), users in unassignments.items(): logger.info( - f"Unassigned Role: {role_external_key} from {len(users)} users \n" + f"Unassigned Role: {role_external_key} from {len(users)} users " f"in Scope: {scope} after successful rollback migration." ) batch_unassign_role_from_users( @@ -417,3 +526,120 @@ def migrate_authz_to_legacy_course_roles( ) return roles_with_errors, roles_with_no_errors + + +# pylint: disable=too-many-positional-arguments +def run_course_authoring_migration( + migration_type: MigrationType, + scope_type: ScopeType, + scope_key: str, + course_access_role_model, + user_subject_model, + course_id_list: list[str] | None, + org_id: str | None, + excluded_course_ids: frozenset[str], + delete_after_migration: bool, +) -> AuthzCourseAuthoringMigrationRun: + """ + Orchestrate a course authoring role migration with concurrency protection and lifecycle tracking. + + Wraps either :func:`migrate_legacy_course_roles_to_authz` (``FORWARD``) or + :func:`migrate_authz_to_legacy_course_roles` (``ROLLBACK``) with three guarantees: + + 1. Concurrency guard: an ``AuthzCourseAuthoringMigrationRun`` record is + created atomically before work begins. If an active run already exists for the + same ``(scope_type, scope_key)``, the call is skipped immediately to prevent + duplicate parallel runs. + 2. Lifecycle tracking: the run record is updated to ``COMPLETED``, + ``PARTIAL_SUCCESS``, or ``FAILED`` regardless of the outcome, with per-role + error details persisted in the record's ``metadata`` field. + 3. Transactional safety: data-migration work runs inside an inner + ``atomic()`` block so that an unexpected exception rolls back all data changes + while the tracking record (updated outside that block) is always persisted. + + Args: + migration_type (MigrationType): Direction of the migration (``FORWARD`` or ``ROLLBACK``). + scope_type (ScopeType): Granularity key dimension for the tracking record (e.g. course or org). + scope_key (str): Concrete identifier — a course-v1 key or an org name. + course_access_role_model: The ``CourseAccessRole`` model class. + user_subject_model: The ``UserSubject`` model class; required for ``ROLLBACK``, ignored otherwise. + course_id_list (list[str] | None): Restrict migration to these course-v1 keys. + org_id (str | None): Restrict migration to this org; takes precedence over ``course_id_list``. + excluded_course_ids (frozenset[str]): For org-scoped runs, course ids + whose course-level waffle override opposes the org transition + delete_after_migration (bool): Remove successfully migrated entries from the source system. + """ + try: + with transaction.atomic(): + run = AuthzCourseAuthoringMigrationRun.create_running(migration_type, scope_type, scope_key) + except IntegrityError: + logger.warning( + "Skipping %s migration for %s:%s — an active run already exists.", migration_type, scope_type, scope_key + ) + return AuthzCourseAuthoringMigrationRun.create_skipped(migration_type, scope_type, scope_key) + + logger.info("Started %s migration run [%s] for %s:%s", migration_type, run.id, scope_type, scope_key) + + try: + with transaction.atomic(): + if migration_type == MigrationType.FORWARD: + errors, successes = migrate_legacy_course_roles_to_authz( + course_access_role_model, + course_id_list, + org_id, + delete_after_migration, + excluded_course_ids, + ) + else: + errors, successes = migrate_authz_to_legacy_course_roles( + course_access_role_model, + user_subject_model, + course_id_list, + org_id, + delete_after_migration, + excluded_course_ids, + ) + except Exception as exc: # pylint: disable=broad-exception-caught + # The inner atomic block is rolled back on exception; mark_failed() runs + # outside it so the tracking record is always persisted. + logger.exception( + "Unexpected error in migration run [%s] for %s:%s", run.id, scope_type, scope_key, exc_info=exc + ) + return run.mark_failed(exception=exc) + + errors_by_reason: dict = defaultdict(list) + for entry in errors: + entry_dict = entry.to_dict() + errors_by_reason[entry_dict.pop("reason")].append(entry_dict) + + metadata_updates = { + "total": len(successes) + len(errors), + "success_count": len(successes), + "error_count": len(errors), + "successes": [entry.to_dict() for entry in successes], + "errors": dict(errors_by_reason), + } + + if errors: + run.mark_partial_success(metadata_updates=metadata_updates) + logger.warning( + "Partial success in %s migration run [%s] for %s:%s — successes=%d, errors=%d", + migration_type, + run.id, + scope_type, + scope_key, + len(successes), + len(errors), + ) + return run + else: + run.mark_completed(metadata_updates=metadata_updates) + logger.info( + "Completed %s migration run [%s] for %s:%s — successes=%d", + migration_type, + run.id, + scope_type, + scope_key, + len(successes), + ) + return run diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index 7701123e..755d485e 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -4,19 +4,35 @@ These handlers ensure proper cleanup and consistency when models are deleted. """ +from __future__ import annotations + import logging +from typing import Union from casbin_adapter.models import CasbinRule -from django.db.models.signals import post_delete +from django.conf import settings +from django.db.models.signals import post_delete, post_save from django.dispatch import receiver +from waffle.models import Flag from openedx_authz.api.users import unassign_all_roles_from_user +from openedx_authz.engine.utils import run_course_authoring_migration +from openedx_authz.models.authz_migration import MigrationType, ScopeType from openedx_authz.models.core import ExtendedCasbinRule +from openedx_authz.models.subjects import UserSubject try: + from common.djangoapps.student.models import CourseAccessRole from openedx.core.djangoapps.user_api.accounts.signals import USER_RETIRE_LMS_CRITICAL + from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel, WaffleFlagOrgOverrideModel + from openedx.core.toggles import AUTHZ_COURSE_AUTHORING_FLAG except ImportError: USER_RETIRE_LMS_CRITICAL = None + WaffleFlagCourseOverrideModel = None + WaffleFlagOrgOverrideModel = None + AUTHZ_COURSE_AUTHORING_FLAG = None + CourseAccessRole = None + logger = logging.getLogger(__name__) @@ -82,3 +98,210 @@ def unassign_roles_on_user_retirement(sender, user, **kwargs): # pylint: disabl # Only register the handler if the signal is available (i.e., running in Open edX) if USER_RETIRE_LMS_CRITICAL is not None: USER_RETIRE_LMS_CRITICAL.connect(unassign_roles_on_user_retirement) + + +def handle_course_waffle_flag_change(sender, instance, **kwargs) -> None: + """ + Handle changes to course-level waffle flags. + + When the authz.enable_course_authoring flag is changed for a course, + trigger the appropriate migration run. Only trigger if automatic migration + is enabled in the settings. + + Args: + sender: The model class (WaffleFlagCourseOverrideModel) + instance: The flag override instance being saved + **kwargs: Additional keyword arguments from the signal + """ + trigger_course_authoring_migration(sender=sender, instance=instance, scope_key=str(instance.course_id)) + + +def handle_org_waffle_flag_change(sender, instance, **kwargs) -> None: + """ + Handle changes to organization-level waffle flags. + + When the authz.enable_course_authoring flag is changed for an organization, + trigger the appropriate migration run. Only trigger if automatic migration + is enabled in the settings. + + Args: + sender: The model class (WaffleFlagOrgOverrideModel) + instance: The flag override instance being saved + **kwargs: Additional keyword arguments from the signal + """ + trigger_course_authoring_migration(sender=sender, instance=instance, scope_key=str(instance.org)) + + +# Only register the handlers if the models are available (i.e., running in Open edX) +if WaffleFlagCourseOverrideModel is not None: + post_save.connect(handle_course_waffle_flag_change, sender=WaffleFlagCourseOverrideModel) + +if WaffleFlagOrgOverrideModel is not None: + post_save.connect(handle_org_waffle_flag_change, sender=WaffleFlagOrgOverrideModel) + + +# Match ``WaffleFlagCourseOverrideModel.OVERRIDE_CHOICES`` / ``override_value`` in edx-platform: +# the flag is effectively forced on only when the row is enabled and ``override_choice`` is "on". +WAFFLE_OVERRIDE_FORCE_ON = "on" +WAFFLE_OVERRIDE_FORCE_OFF = "off" + + +# Type Alias for better readability +WaffleOverrideRecord = Union[WaffleFlagCourseOverrideModel, WaffleFlagOrgOverrideModel] + + +def get_effective_state(record: WaffleOverrideRecord | None, global_flag_enabled: bool) -> bool: + """ + Return whether the feature is effectively active for the override and global flag. + + An enabled override forces on or off, otherwise the result follows the global flag. + + Args: + record (WaffleOverrideRecord | None): The waffle flag record to evaluate. + global_flag_enabled (bool): The state of the global flag. + + Returns: + bool: True if the feature is active, False otherwise. + """ + # If there is no override, or the override is disabled, the state falls back to the global flag. + if not record or not record.enabled: + return global_flag_enabled + + # If there is an active override, it dictates the state, ignoring the global flag. + if record.override_choice == WAFFLE_OVERRIDE_FORCE_ON: + return True + if record.override_choice == WAFFLE_OVERRIDE_FORCE_OFF: + return False + + # Safety fallback (in case override_choice is corrupted or empty) + return global_flag_enabled + + +def get_migration_type( + current_record: WaffleOverrideRecord, + previous_record: WaffleOverrideRecord | None, + global_flag_enabled: bool, +) -> MigrationType | None: + """ + Determine the migration type by comparing the effective state before and after the transaction. + + This accounts for the global flag state, meaning a transition could be triggered by + removing a FORCE_OFF override when the global flag is ON. + + Args: + current_record (WaffleOverrideRecord): The state of the record in the current transaction. + previous_record (WaffleOverrideRecord | None): The state of the record prior to the current transaction. + global_flag_enabled (bool): The state of the global flag. + + Returns: + MigrationType.FORWARD: If the flag is newly forced on. + MigrationType.ROLLBACK: If the forced-on state is removed. + None: If there is no effective change in the flag's behavior. + """ + was_effectively_active = get_effective_state(previous_record, global_flag_enabled) + is_effectively_active = get_effective_state(current_record, global_flag_enabled) + + # If the effective behavior hasn't changed, we don't need to do anything + if is_effectively_active == was_effectively_active: + return None + + # If it is now effectively active (and wasn't before), migrate forward. Otherwise, rollback. + return MigrationType.FORWARD if is_effectively_active else MigrationType.ROLLBACK + + +def get_excluded_course_ids_for_org_migration(org_id: str, override_choice: str) -> frozenset[str]: + """ + Collect course-level authoring flag overrides for an org that oppose the new org-level state. + + When the org flag changes, we need to exclude course ids that have a course-level + authoring flag override that opposes the new org-level state. + + Args: + org_id (str): Organization short name. + override_choice (str): The override choice of the org waffle flag. + + Returns: + frozenset[str]: course ids excluded from org migration + """ + # We only need to check the current set (active flags). Opposing overrides are rows that + # force the opposite of the org transition (Force On vs Force Off), not merely inactive rows. + reverse_choice = ( + WAFFLE_OVERRIDE_FORCE_ON if override_choice == WAFFLE_OVERRIDE_FORCE_OFF else WAFFLE_OVERRIDE_FORCE_OFF + ) + filter_kwargs = { + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG.name, + "course_id__startswith": f"course-v1:{org_id}+", + "enabled": True, + "override_choice": reverse_choice, + } + qs = WaffleFlagCourseOverrideModel.objects.current_set().filter(**filter_kwargs) + return frozenset(map(str, qs.values_list("course_id", flat=True))) + + +def trigger_course_authoring_migration( + sender: type[WaffleOverrideRecord], + instance: WaffleOverrideRecord, + scope_key: str, +) -> None: + """ + Trigger a migration run in response to a waffle flag change. + + Determines the migration direction from the flag state, guards against + no-op saves, and delegates execution to ``run_course_authoring_migration`` + which handles tracking and concurrent-run protection. + + Args: + sender: The model class (WaffleOverrideRecord). + instance: The waffle flag instance that triggered the migration. + scope_key (str): Course ID or organization name. + """ + if instance.waffle_flag != AUTHZ_COURSE_AUTHORING_FLAG.name: + return + + if not settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION: + logger.info("ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION is set to False, skipping migration") + return + + course_id_list, org_id, scope_type = None, None, None + filter_kwargs = {"waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG.name} + if isinstance(instance, WaffleFlagCourseOverrideModel): + filter_kwargs["course_id"] = instance.course_id + course_id_list = [scope_key] + scope_type = ScopeType.COURSE + elif isinstance(instance, WaffleFlagOrgOverrideModel): + filter_kwargs["org"] = instance.org + org_id = scope_key + scope_type = ScopeType.ORG + else: + logger.error("Unsupported waffle flag instance: %s", instance) + return + + prev_record = sender.objects.filter(**filter_kwargs).exclude(id=instance.id).order_by("-change_date").first() + + global_flag = Flag.objects.filter(name=AUTHZ_COURSE_AUTHORING_FLAG.name).first() + global_flag_enabled = bool(global_flag and global_flag.everyone) + + migration_type = get_migration_type(instance, prev_record, global_flag_enabled) + if migration_type is None: + logger.info("No effective change in waffle flag behavior, skipping migration") + return + + excluded_course_ids = frozenset() + if isinstance(instance, WaffleFlagOrgOverrideModel): + excluded_course_ids = get_excluded_course_ids_for_org_migration( + org_id=scope_key, override_choice=instance.override_choice + ) + + logger.info("Triggering %s migration for %s:%s due to waffle flag change", migration_type, scope_type, scope_key) + + run_course_authoring_migration( + migration_type=migration_type, + scope_type=scope_type, + scope_key=scope_key, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=course_id_list, + org_id=org_id, + excluded_course_ids=excluded_course_ids, + delete_after_migration=True, + ) diff --git a/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py b/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py new file mode 100644 index 00000000..7eaa02f0 --- /dev/null +++ b/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py @@ -0,0 +1,78 @@ +# Generated by Django 5.2.12 on 2026-04-16 20:29 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("openedx_authz", "0007_coursescope"), + ] + + operations = [ + migrations.CreateModel( + name="AuthzCourseAuthoringMigrationRun", + fields=[ + ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), + ( + "migration_type", + models.CharField( + choices=[("forward", "Legacy to AuthZ"), ("rollback", "AuthZ to Legacy")], + help_text="Direction of migration: forward (legacy → authz) or rollback (authz → legacy)", + max_length=20, + ), + ), + ( + "scope_type", + models.CharField( + choices=[("course", "Course"), ("org", "Organization")], + help_text="Type of scope being migrated: course or organization", + max_length=20, + ), + ), + ( + "scope_key", + models.CharField( + help_text="Identifier for the scope (e.g., course-v1:edX+DemoX+DemoCourse or org name)", + max_length=255, + ), + ), + ( + "status", + models.CharField( + choices=[ + ("running", "Running"), + ("completed", "Completed"), + ("partial_success", "Partial Success"), + ("failed", "Failed"), + ("skipped", "Skipped"), + ], + default="running", + help_text="Current status of the migration run", + max_length=20, + ), + ), + ("created_at", models.DateTimeField(auto_now_add=True, help_text="When the migration run was created")), + ( + "updated_at", + models.DateTimeField(auto_now=True, help_text="When the migration run was last updated"), + ), + ( + "completed_at", + models.DateTimeField(blank=True, help_text="When the migration run was completed", null=True), + ), + ( + "metadata", + models.JSONField( + blank=True, + default=dict, + help_text="Additional metadata about the migration run (e.g., counts, warnings, errors)", + ), + ), + ], + options={ + "verbose_name": "Course Authoring Migration Run", + "verbose_name_plural": "Course Authoring Migration Runs", + "ordering": ["-created_at"], + }, + ), + ] diff --git a/openedx_authz/models/__init__.py b/openedx_authz/models/__init__.py index 4f0318a2..06b5d003 100644 --- a/openedx_authz/models/__init__.py +++ b/openedx_authz/models/__init__.py @@ -15,6 +15,7 @@ avoid circular dependencies. """ +from openedx_authz.models.authz_migration import * from openedx_authz.models.core import * from openedx_authz.models.scopes import * from openedx_authz.models.subjects import * diff --git a/openedx_authz/models/authz_migration.py b/openedx_authz/models/authz_migration.py new file mode 100644 index 00000000..1be9f8ce --- /dev/null +++ b/openedx_authz/models/authz_migration.py @@ -0,0 +1,170 @@ +"""Models for tracking migration runs between legacy and AuthZ systems.""" + +from django.db import IntegrityError, models, transaction +from django.utils import timezone + + +class MigrationType(models.TextChoices): + """Direction of migration.""" + + FORWARD = "forward", "Legacy to AuthZ" + ROLLBACK = "rollback", "AuthZ to Legacy" + + +class Status(models.TextChoices): + """Status of the migration task.""" + + RUNNING = "running", "Running" + COMPLETED = "completed", "Completed" + PARTIAL_SUCCESS = "partial_success", "Partial Success" + FAILED = "failed", "Failed" + SKIPPED = "skipped", "Skipped" + + +class ScopeType(models.TextChoices): + """Type of scope being migrated.""" + + COURSE = "course", "Course" + ORG = "org", "Organization" + + +class AuthzCourseAuthoringMigrationRun(models.Model): + """Track the status of course authoring migration tasks. + + This model is used to track async migrations between the legacy + CourseAccessRole system and the new AuthZ system. + + .. no_pii: + """ + + migration_type = models.CharField( + max_length=20, + choices=MigrationType, + help_text="Direction of migration: forward (legacy → authz) or rollback (authz → legacy)", + ) + + scope_type = models.CharField( + max_length=20, + choices=ScopeType, + help_text="Type of scope being migrated: course or organization", + ) + + scope_key = models.CharField( + max_length=255, + help_text="Identifier for the scope (e.g., course-v1:edX+DemoX+DemoCourse or org name)", + ) + + status = models.CharField( + max_length=20, + choices=Status, + default=Status.RUNNING, + help_text="Current status of the migration run", + ) + + created_at = models.DateTimeField( + auto_now_add=True, + help_text="When the migration run was created", + ) + + updated_at = models.DateTimeField( + auto_now=True, + help_text="When the migration run was last updated", + ) + + completed_at = models.DateTimeField( + null=True, + blank=True, + help_text="When the migration run was completed", + ) + + metadata = models.JSONField( + default=dict, + blank=True, + help_text="Additional metadata about the migration run (e.g., counts, warnings, errors)", + ) + + class Meta: + verbose_name = "Course Authoring Migration Run" + verbose_name_plural = "Course Authoring Migration Runs" + ordering = ["-created_at"] + + def save(self, *args, **kwargs) -> "AuthzCourseAuthoringMigrationRun": + """Enforce at most one RUNNING record per (scope_type, scope_key). + + MySQL does not support partial unique indexes, so this check is done at + the application level. select_for_update() is used to reduce (but not + fully eliminate) the race-condition window on concurrent inserts. + """ + with transaction.atomic(): + if self.status == Status.RUNNING and self.pk is None: + conflict = ( + self.__class__.objects.select_for_update() + .filter(scope_type=self.scope_type, scope_key=self.scope_key, status=Status.RUNNING) + .exists() + ) + if conflict: + raise IntegrityError( + f"Duplicate RUNNING migration run for scope {self.scope_type}:{self.scope_key}" + ) + super().save(*args, **kwargs) + return self + + # pylint: disable=too-many-positional-arguments + @classmethod + def _create( + cls, migration_type, scope_type, scope_key, status, metadata=None + ) -> "AuthzCourseAuthoringMigrationRun": + return cls.objects.create( + migration_type=migration_type, + scope_type=scope_type, + scope_key=scope_key, + status=status, + metadata=metadata or {}, + ) + + @classmethod + def create_running( + cls, + migration_type, + scope_type, + scope_key, + metadata=None, + ) -> "AuthzCourseAuthoringMigrationRun": + """Create a migration run in running state.""" + return cls._create(migration_type, scope_type, scope_key, Status.RUNNING, metadata) + + @classmethod + def create_skipped( + cls, + migration_type, + scope_type, + scope_key, + metadata=None, + ) -> "AuthzCourseAuthoringMigrationRun": + """Create a migration run in skipped state.""" + extra = {**(metadata or {}), "skip_reason": "A concurrent migration run is already active for this scope."} + return cls._create(migration_type, scope_type, scope_key, Status.SKIPPED, extra) + + def _finalize(self, status: str, metadata_updates: dict | None = None) -> "AuthzCourseAuthoringMigrationRun": + """Finalize the migration run.""" + self.status = status + self.completed_at = timezone.now() + if metadata_updates: + self.metadata = {**(self.metadata or {}), **metadata_updates} + return self.save(update_fields=["status", "completed_at", "updated_at", "metadata"]) + + def mark_partial_success(self, *, metadata_updates=None) -> "AuthzCourseAuthoringMigrationRun": + """Mark the migration run as partially successful.""" + return self._finalize(Status.PARTIAL_SUCCESS, metadata_updates) + + def mark_completed(self, *, metadata_updates=None) -> "AuthzCourseAuthoringMigrationRun": + """Mark the migration run as completed.""" + return self._finalize(Status.COMPLETED, metadata_updates) + + def mark_failed(self, *, exception=None) -> "AuthzCourseAuthoringMigrationRun": + """Mark the migration run as failed.""" + return self._finalize(Status.FAILED, {"error": str(exception)} if exception is not None else None) + + def __str__(self) -> str: + """Return a string representation of the migration run.""" + return f"[{self.id}] {self.migration_type} {self.scope_type}:{self.scope_key} {self.status}" diff --git a/openedx_authz/settings/common.py b/openedx_authz/settings/common.py index 81e060b8..c6cabc24 100644 --- a/openedx_authz/settings/common.py +++ b/openedx_authz/settings/common.py @@ -54,3 +54,8 @@ def plugin_settings(settings): # This setting defines the logging level for the Casbin enforcer. if not hasattr(settings, "CASBIN_LOG_LEVEL"): settings.CASBIN_LOG_LEVEL = "WARNING" + + # Set default ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION if not already set. + # This setting defines whether to enable automatic course migration. See ADR-0013 for more details. + if not hasattr(settings, "ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION"): + settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = False diff --git a/openedx_authz/settings/test.py b/openedx_authz/settings/test.py index ba449092..bd6d698d 100644 --- a/openedx_authz/settings/test.py +++ b/openedx_authz/settings/test.py @@ -39,6 +39,7 @@ def plugin_settings(settings): # pylint: disable=unused-argument "openedx_authz.apps.OpenedxAuthzConfig", "openedx_authz.tests.stubs.apps.StubsConfig", "organizations", + "waffle", ) MIDDLEWARE = [ @@ -78,3 +79,6 @@ def plugin_settings(settings): # pylint: disable=unused-argument # Use stub model for testing instead of the real content_libraries app OPENEDX_AUTHZ_CONTENT_LIBRARY_MODEL = "stubs.ContentLibrary" OPENEDX_AUTHZ_COURSE_OVERVIEW_MODEL = "stubs.CourseOverview" + +# Migration settings +ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = True diff --git a/openedx_authz/tests/stubs/models.py b/openedx_authz/tests/stubs/models.py index d443a612..8bbe8b42 100644 --- a/openedx_authz/tests/stubs/models.py +++ b/openedx_authz/tests/stubs/models.py @@ -205,3 +205,39 @@ class CourseAccessRole(models.Model): # blank course_id implies org wide role course_id = CourseKeyField(max_length=255, db_index=True, blank=True) role = models.CharField(max_length=64, db_index=True) + + +# Waffle flag models +class _WaffleStubManager(models.Manager): + """Minimal stand-in for ``ConfigurationModel`` managers exposing ``current_set()``.""" + + def current_set(self): + return self.get_queryset() + + +class WaffleFlagCourseOverrideModel(models.Model): + """Stub model representing a waffle flag course override for testing purposes. + + .. no_pii: + """ + + course_id = CourseKeyField(max_length=255, db_index=True) + waffle_flag = models.CharField(max_length=255, db_index=True, default="") + enabled = models.BooleanField(default=False) + override_choice = models.CharField(max_length=3, default="on") + change_date = models.DateTimeField(auto_now_add=True) + objects = _WaffleStubManager() + + +class WaffleFlagOrgOverrideModel(models.Model): + """Stub model representing a waffle flag org override for testing purposes. + + .. no_pii: + """ + + org = models.CharField(max_length=64, db_index=True) + waffle_flag = models.CharField(max_length=255, db_index=True, default="") + enabled = models.BooleanField(default=False) + override_choice = models.CharField(max_length=3, default="on") + change_date = models.DateTimeField(auto_now_add=True) + objects = _WaffleStubManager() diff --git a/openedx_authz/tests/test_handlers.py b/openedx_authz/tests/test_handlers.py index 1b31e1f9..81879f64 100644 --- a/openedx_authz/tests/test_handlers.py +++ b/openedx_authz/tests/test_handlers.py @@ -1,16 +1,36 @@ -"""Behavioral tests for the ExtendedCasbinRule deletion signal. +"""Tests for ``openedx_authz.handlers`` Coverage confirms direct deletions, cascades, bulk operations, and resilience when foreign keys -are missing so that the signal stays aligned with the cleanup guarantees in -``openedx_authz.handlers``. +are missing so that the signal stays aligned with the cleanup guarantees. + +Also covers ``trigger_course_authoring_migration`` using stub waffle model classes (Open edX +waffle models are not imported in the test environment). """ +from types import SimpleNamespace from unittest.mock import patch from casbin_adapter.models import CasbinRule -from django.test import TestCase - +from ddt import data, ddt, unpack +from django.test import TestCase, override_settings + +from openedx_authz.handlers import ( + WAFFLE_OVERRIDE_FORCE_OFF, + WAFFLE_OVERRIDE_FORCE_ON, + get_migration_type, + trigger_course_authoring_migration, +) +from openedx_authz.models.authz_migration import MigrationType, ScopeType from openedx_authz.models.core import ExtendedCasbinRule, Scope, Subject +from openedx_authz.models.subjects import UserSubject +from openedx_authz.tests.stubs.models import ( + CourseAccessRole, + WaffleFlagCourseOverrideModel, + WaffleFlagOrgOverrideModel, +) + +AUTHZ_COURSE_AUTHORING_FLAG_NAME = "authz.enable_course_authoring" +OTHER_WAFFLE_FLAG_NAME = "some.other.flag" def create_casbin_rule_with_extended( # pylint: disable=too-many-positional-arguments @@ -220,3 +240,452 @@ def test_cascade_deletion_with_scope_deletion(self): self.assertFalse(CasbinRule.objects.filter(id=casbin_rule_id).exists()) self.assertFalse(Scope.objects.filter(id=scope_id).exists()) self.assertTrue(Subject.objects.filter(id=subject_id).exists()) + + +@ddt +@patch("openedx_authz.handlers.run_course_authoring_migration") +@patch.multiple( + "openedx_authz.handlers", + AUTHZ_COURSE_AUTHORING_FLAG=SimpleNamespace(name=AUTHZ_COURSE_AUTHORING_FLAG_NAME), + WaffleFlagCourseOverrideModel=WaffleFlagCourseOverrideModel, + WaffleFlagOrgOverrideModel=WaffleFlagOrgOverrideModel, + CourseAccessRole=CourseAccessRole, +) +class TestTriggerCourseAuthoringMigration(TestCase): + """ + Runs tests for ``trigger_course_authoring_migration`` with stub waffle models. + """ + + COURSE_KEY = "course-v1:test_org+course+run_mm" + ORG_KEY = "test_org" + + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": COURSE_KEY, + "waffle_flag": OTHER_WAFFLE_FLAG_NAME, + "enabled": True, + }, + COURSE_KEY, + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": ORG_KEY, + "waffle_flag": OTHER_WAFFLE_FLAG_NAME, + "enabled": True, + }, + ORG_KEY, + ), + ) + @unpack + def test_skips_when_waffle_flag_name_mismatch(self, sender_model, instance_kwargs, scope_key, mock_run): + """Only the authz course authoring flag triggers migration (course and org overrides).""" + instance = sender_model(**instance_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + mock_run.assert_not_called() + + @override_settings(ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION=False) + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + }, + COURSE_KEY, + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + }, + ORG_KEY, + ), + ) + @unpack + @patch("openedx_authz.handlers.logger") + def test_skips_when_automatic_migration_setting_disabled( + self, sender_model, instance_kwargs, scope_key, mock_logger, mock_run + ): # pylint: disable=too-many-positional-arguments + """When the setting is off, the handler returns before scheduling work (course and org).""" + instance = sender_model(**instance_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + mock_run.assert_not_called() + mock_logger.info.assert_called_once_with( + "ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION is set to False, skipping migration" + ) + + @patch("openedx_authz.handlers.logger") + def test_logs_error_for_unsupported_instance_type(self, mock_logger, mock_run): + """Instances that are neither course nor org overrides are rejected.""" + unsupported = SimpleNamespace(waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, enabled=True, id=9) + + trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, unsupported, "ignored") + + mock_run.assert_not_called() + mock_logger.error.assert_called_once_with("Unsupported waffle flag instance: %s", unsupported) + + @data( + (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD, True), + (WAFFLE_OVERRIDE_FORCE_OFF, None, False), + ) + @unpack + @patch("openedx_authz.handlers.logger") + def test_course_scope_migration_depends_on_override_choice( + self, override_choice, expected_migration_type, expect_migration, mock_logger, mock_run + ): # pylint: disable=too-many-positional-arguments + """Course override runs forward only when forced on, force-off is a no-op for migration.""" + instance = WaffleFlagCourseOverrideModel.objects.create( + course_id=self.COURSE_KEY, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=True, + override_choice=override_choice, + ) + + trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, instance, self.COURSE_KEY) + + if expect_migration: + mock_run.assert_called_once_with( + migration_type=expected_migration_type, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_KEY, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_KEY], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=True, + ) + else: + mock_run.assert_not_called() + mock_logger.info.assert_called_once_with("No effective change in waffle flag behavior, skipping migration") + + @data( + (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD, True), + (WAFFLE_OVERRIDE_FORCE_OFF, None, False), + ) + @unpack + @patch("openedx_authz.handlers.logger") + def test_org_scope_migration_depends_on_override_choice( + self, override_choice, expected_migration_type, expect_migration, mock_logger, mock_run + ): # pylint: disable=too-many-positional-arguments + """Org override runs forward only when forced on, force-off is a no-op for migration.""" + instance = WaffleFlagOrgOverrideModel.objects.create( + org=self.ORG_KEY, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=True, + override_choice=override_choice, + ) + + trigger_course_authoring_migration(WaffleFlagOrgOverrideModel, instance, self.ORG_KEY) + + if expect_migration: + mock_run.assert_called_once_with( + migration_type=expected_migration_type, + scope_type=ScopeType.ORG, + scope_key=self.ORG_KEY, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.ORG_KEY, + excluded_course_ids=frozenset(), + delete_after_migration=True, + ) + else: + mock_run.assert_not_called() + mock_logger.info.assert_called_once_with("No effective change in waffle flag behavior, skipping migration") + + def test_org_scope_passes_excluded_course_ids_when_course_overrides_oppose_org(self, mock_run): + """Org forward migration excludes active course rows whose override opposes force-on.""" + WaffleFlagCourseOverrideModel.objects.create( + course_id=self.COURSE_KEY, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=True, + override_choice=WAFFLE_OVERRIDE_FORCE_OFF, + ) + instance = WaffleFlagOrgOverrideModel.objects.create( + org=self.ORG_KEY, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=True, + override_choice=WAFFLE_OVERRIDE_FORCE_ON, + ) + + trigger_course_authoring_migration(WaffleFlagOrgOverrideModel, instance, self.ORG_KEY) + + mock_run.assert_called_once_with( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.ORG, + scope_key=self.ORG_KEY, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.ORG_KEY, + excluded_course_ids=frozenset({str(self.COURSE_KEY)}), + delete_after_migration=True, + ) + + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + COURSE_KEY, + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + ORG_KEY, + ), + ) + @unpack + @patch("openedx_authz.handlers.logger") + def test_skips_when_previous_enabled_record_has_same_override_choice( + self, sender_model, row_kwargs, scope_key, mock_logger, mock_run + ): # pylint: disable=too-many-positional-arguments + """Repeated history rows with the same active override choice do not trigger migration.""" + sender_model.objects.create(**row_kwargs) + instance = sender_model.objects.create(**row_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + mock_run.assert_not_called() + mock_logger.info.assert_called_once_with("No effective change in waffle flag behavior, skipping migration") + + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": False, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + COURSE_KEY, + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": False, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + ORG_KEY, + ), + ) + @unpack + def test_runs_when_previous_record_disabled_even_if_same_override_choice( + self, sender_model, prev_kwargs, instance_kwargs, scope_key, mock_run + ): # pylint: disable=too-many-positional-arguments + """If the prior row was inactive, a new active row still triggers migration (course and org).""" + sender_model.objects.create(**prev_kwargs) + instance = sender_model.objects.create(**instance_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + common = { + "migration_type": MigrationType.FORWARD, + "scope_key": scope_key, + "course_access_role_model": CourseAccessRole, + "user_subject_model": UserSubject, + "excluded_course_ids": frozenset(), + "delete_after_migration": True, + } + if sender_model is WaffleFlagCourseOverrideModel: + mock_run.assert_called_once_with( + **common, + scope_type=ScopeType.COURSE, + course_id_list=[scope_key], + org_id=None, + ) + else: + mock_run.assert_called_once_with( + **common, + scope_type=ScopeType.ORG, + course_id_list=None, + org_id=scope_key, + ) + + +class TestGetMigrationType(TestCase): + """Tests for ``get_migration_type`` (effective state includes ``global_flag_enabled``).""" + + def create_mock_record(self, enabled: bool, choice: str): + """Helper to create a mock record object.""" + return SimpleNamespace(enabled=enabled, override_choice=choice) + + def test_creation_new_record_enabled_global_off(self): + """No prior row: enabling FORCE_ON migrates forward when the global flag is off.""" + current = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(current, None, global_flag_enabled=False) + + self.assertEqual(result, MigrationType.FORWARD) + + def test_creation_new_record_enabled_global_on(self): + """No prior row: FORCE_ON is already the effective state when global is on — no migration.""" + current = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(current, None, global_flag_enabled=True) + + self.assertIsNone(result) + + def test_creation_new_record_disabled_matches_global(self): + """Disabled row defers to global, no previous row means same effective state as global.""" + current = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + self.assertIsNone(get_migration_type(current, None, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(current, None, global_flag_enabled=True)) + + def test_no_change_stay_active_force_on(self): + """Both enabled FORCE_ON — effective stays on.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=True)) + + def test_no_change_stay_active_force_off(self): + """Both enabled FORCE_OFF — effective stays off.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=True)) + + def test_no_change_stay_inactive(self): + """Both rows disabled — both follow global, so no effective change.""" + prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=True)) + + def test_transition_disabled_to_enabled_force_on_global_off(self): + """Row becomes active FORCE_ON while global is off — effective off → on.""" + prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev, global_flag_enabled=False) + + self.assertEqual(result, MigrationType.FORWARD) + + def test_transition_disabled_to_enabled_force_on_global_on(self): + """Previously inactive row followed global (on), turning FORCE_ON on stays on — no op.""" + prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev, global_flag_enabled=True) + + self.assertIsNone(result) + + def test_transition_enabled_force_on_to_disabled_global_off(self): + """FORCE_ON row disabled, global off — effective on → off (rollback).""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev, global_flag_enabled=False) + + self.assertEqual(result, MigrationType.ROLLBACK) + + def test_transition_enabled_force_on_to_disabled_global_on(self): + """FORCE_ON row disabled but global still on — effective stays on, no migration.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev, global_flag_enabled=True) + + self.assertIsNone(result) + + def test_change_choice_force_on_to_force_off(self): + """Enabled FORCE_ON → FORCE_OFF — effective on → off.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + + self.assertEqual( + get_migration_type(curr, prev, global_flag_enabled=False), + MigrationType.ROLLBACK, + ) + self.assertEqual( + get_migration_type(curr, prev, global_flag_enabled=True), + MigrationType.ROLLBACK, + ) + + def test_change_choice_force_off_to_force_on(self): + """Enabled FORCE_OFF → FORCE_ON — effective off → on.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + self.assertEqual( + get_migration_type(curr, prev, global_flag_enabled=False), + MigrationType.FORWARD, + ) + self.assertEqual( + get_migration_type(curr, prev, global_flag_enabled=True), + MigrationType.FORWARD, + ) + + def test_remove_force_off_override_when_global_on(self): + """Deleting FORCE_OFF behavior by disabling row restores global on — forward migration.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_OFF) + + result = get_migration_type(curr, prev, global_flag_enabled=True) + + self.assertEqual(result, MigrationType.FORWARD) + + def test_remove_force_off_override_when_global_off(self): + """Disabling FORCE_OFF row while global off — still off, no migration.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_OFF) + + result = get_migration_type(curr, prev, global_flag_enabled=False) + + self.assertIsNone(result) + + def test_add_force_off_override_when_global_on(self): + """New active FORCE_OFF while global on — effective on → off.""" + prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + + result = get_migration_type(curr, prev, global_flag_enabled=True) + + self.assertEqual(result, MigrationType.ROLLBACK) + + def test_unknown_override_choice_follows_global(self): + """Non on/off choice falls back to global — toggling only matters vs that baseline.""" + prev = self.create_mock_record(True, "unset") + curr = self.create_mock_record(True, "unset") + + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=True)) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index fb82908e..53b63a66 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -2,12 +2,20 @@ from unittest.mock import MagicMock, patch +from ddt import data, ddt, unpack from django.contrib.auth import get_user_model from django.contrib.auth.models import Group from django.core.management import CommandError, call_command from django.test import TestCase - -from openedx_authz.api.data import OrgCourseOverviewGlobData +from opaque_keys.edx.django.models import CourseKeyField + +from openedx_authz.api.data import ( + CourseOverviewData, + OrgCourseOverviewGlobData, + RoleAssignmentData, + RoleData, + UserData, +) from openedx_authz.api.users import ( assign_role_to_user_in_scope, batch_unassign_role_from_users, @@ -16,6 +24,7 @@ from openedx_authz.constants.roles import ( COURSE_ADMIN, COURSE_DATA_RESEARCHER, + COURSE_EDITOR, COURSE_LIMITED_STAFF, COURSE_STAFF, LEGACY_COURSE_ROLE_EQUIVALENCES, @@ -24,9 +33,20 @@ ) from openedx_authz.engine.enforcer import AuthzEnforcer from openedx_authz.engine.utils import ( + MigrationErrorReason, + MigrationMetadata, migrate_authz_to_legacy_course_roles, migrate_legacy_course_roles_to_authz, migrate_legacy_permissions, + run_course_authoring_migration, +) +from openedx_authz.models.authz_migration import ( + AuthzCourseAuthoringMigrationRun as MigrationRun, +) +from openedx_authz.models.authz_migration import ( + MigrationType, + ScopeType, + Status, ) from openedx_authz.models.subjects import UserSubject from openedx_authz.tests.stubs.models import ( @@ -430,7 +450,7 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): self.assertEqual(assignments[0].roles[0], COURSE_DATA_RESEARCHER) self.assertEqual(len(permissions_with_errors), 1) - self.assertEqual(permissions_with_errors[0].user.username, self.error_user.username) + self.assertEqual(permissions_with_errors[0].subject, self.error_user.username) self.assertEqual(permissions_with_errors[0].role, "invalid-legacy-role") self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries @@ -581,7 +601,7 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): self.assertEqual(len(assignments), 1) self.assertEqual(assignments[0].roles[0], COURSE_DATA_RESEARCHER) self.assertEqual(len(permissions_with_errors), 1) - self.assertEqual(permissions_with_errors[0].user.username, self.error_user.username) + self.assertEqual(permissions_with_errors[0].subject, self.error_user.username) self.assertEqual(permissions_with_errors[0].role, "invalid-legacy-role") self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries @@ -798,7 +818,7 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): self.assertEqual(len(assignments), 1) self.assertEqual(assignments[0].roles[0], COURSE_DATA_RESEARCHER) self.assertEqual(len(permissions_with_errors), 1) - self.assertEqual(permissions_with_errors[0].user.username, self.error_user.username) + self.assertEqual(permissions_with_errors[0].subject, self.error_user.username) self.assertEqual(permissions_with_errors[0].role, "invalid-legacy-role") self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries @@ -881,6 +901,39 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): # After rollback, we should have 0 UserSubjects related to the course permissions self.assertEqual(len(state_after_migration_user_subjects), 0) + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_legacy_course_roles_to_authz_skips_excluded_course_ids(self): + """Org-scoped migration skips course-level rows whose course_id is in excluded_course_ids.""" + errors, successes = migrate_legacy_course_roles_to_authz( + CourseAccessRole, + course_id_list=None, + org_id=self.org, + delete_after_migration=False, + excluded_course_ids=frozenset({self.course_id}), + ) + + self.assertEqual(len(successes), 0) + self.assertEqual(len(errors), 13) + + skipped = [entry for entry in errors if entry.reason == MigrationErrorReason.SKIPPED_FOR_FLAG_OVERRIDE] + self.assertEqual(len(skipped), 12) + for entry in skipped: + self.assertEqual(entry.scope, self.course_id) + self.assertEqual( + entry.details, + "Course-level authoring flag override opposes this organization-level transition", + ) + + unknown_role = [entry for entry in errors if entry.reason == MigrationErrorReason.UNKNOWN_ROLE] + self.assertEqual(len(unknown_role), 1) + self.assertEqual(unknown_role[0].subject, self.error_user.username) + + for user in self.admin_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 0) + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) def test_migrate_authz_to_legacy_course_roles_with_no_org_and_courses(self): # Migrate from legacy CourseAccessRole to new Casbin-based model @@ -1172,7 +1225,7 @@ def test_migrate_legacy_course_roles_to_authz_user_not_added( self.assertEqual(len(errors), 1) self.assertEqual(len(successes), 0) - self.assertEqual(errors[0].user.username, "testuser") + self.assertEqual(errors[0].subject, "testuser") @patch("openedx_authz.engine.utils.LEGACY_COURSE_ROLE_EQUIVALENCES", {"instructor": "instructor-role"}) def test_migrate_legacy_course_roles_to_authz_instance_wide_role_is_error(self): @@ -1206,7 +1259,7 @@ def test_migrate_legacy_course_roles_to_authz_instance_wide_role_is_error(self): self.assertEqual(len(errors), 1) self.assertEqual(len(successes), 0) - self.assertEqual(errors[0].user.username, "instance_user") + self.assertEqual(errors[0].subject, "instance_user") self.assertTrue(any("instance_user" in msg for msg in log.output)) @patch("openedx_authz.api.data.CourseOverview", CourseOverview) @@ -1229,6 +1282,35 @@ def test_migrate_authz_to_legacy_course_roles_user_not_added(self): self.assertEqual(len(errors), 12) self.assertEqual(len(successes), 0) + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_authz_to_legacy_course_roles_skips_excluded_course_ids(self): + """Course-level assignments whose scope is listed in excluded_course_ids are skipped.""" + migrate_legacy_course_roles_to_authz( + CourseAccessRole, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + errors, successes = migrate_authz_to_legacy_course_roles( + CourseAccessRole, + UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + excluded_course_ids=frozenset({self.course_id}), + ) + + self.assertEqual(len(successes), 0) + self.assertEqual(len(errors), 12) + for entry in errors: + self.assertEqual(entry.reason, MigrationErrorReason.SKIPPED_FOR_FLAG_OVERRIDE) + self.assertEqual( + entry.details, + "Course-level authoring flag override opposes this organization-level transition", + ) + self.assertEqual(entry.scope, self.course_id) + def create_library_env(self): """ Helper method to create a ContentLibrary environment for testing. @@ -1366,7 +1448,7 @@ def test_org_id_filter_includes_glob_and_excludes_other_orgs(self): CourseAccessRole, UserSubject, course_id_list=None, org_id=self.org, delete_after_migration=False ) - migrated_users = {assignment.subject.external_key for assignment in successes} + migrated_users = {assignment.subject for assignment in successes} # glob assignment for self.org is included self.assertIn(user_glob.username, migrated_users) # assignment from the other org is excluded @@ -1405,7 +1487,7 @@ def test_course_id_list_filter_excludes_glob_and_other_courses(self): CourseAccessRole, UserSubject, course_id_list=[self.course_id], org_id=None, delete_after_migration=False ) - migrated_users = {assignment.subject.external_key for assignment in successes} + migrated_users = {assignment.subject for assignment in successes} # org-level glob is excluded even though it belongs to the same org self.assertNotIn(user_glob.username, migrated_users) # course not in the list is excluded @@ -1413,3 +1495,318 @@ def test_course_id_list_filter_excludes_glob_and_other_courses(self): # library assignment in self.org is excluded — library scopes are not course scopes self.assertNotIn(user_lib.username, migrated_users) self.assertEqual(len(errors), 0) + + +@ddt +class TestRunCourseAuthoringMigration(TestCase): + """Exercise ``run_course_authoring_migration`` lifecycle and outcomes using stub ``CourseAccessRole``.""" + + COURSE_ID = "course-v1:TestOrg+TestCourse+2024" + ORG_ID = "TestOrg" + + @data( + (MigrationType.FORWARD, ScopeType.COURSE, None, [COURSE_ID]), + (MigrationType.ROLLBACK, ScopeType.COURSE, None, [COURSE_ID]), + (MigrationType.FORWARD, ScopeType.ORG, ORG_ID, None), + (MigrationType.ROLLBACK, ScopeType.ORG, ORG_ID, None), + ) + @unpack + def test_skipped_when_another_run_is_already_running_for_scope( + self, migration_type, scope_type, org_id, course_id_list + ): + """Second call for the same scope creates a SKIPPED run and returns without migrating.""" + scope_key = org_id if scope_type == ScopeType.ORG else course_id_list[0] + MigrationRun.create_running(migration_type, scope_type, scope_key) + + run_course_authoring_migration( + migration_type=migration_type, + scope_type=scope_type, + scope_key=scope_key, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=course_id_list, + org_id=org_id, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=scope_key, status=Status.SKIPPED) + self.assertEqual(run.migration_type, migration_type) + self.assertEqual(run.scope_type, scope_type) + self.assertEqual(run.scope_key, scope_key) + self.assertEqual(run.metadata, {"skip_reason": "A concurrent migration run is already active for this scope."}) + + @data( + (MigrationType.FORWARD, ScopeType.COURSE, None, ["ccx-v1:Org+Course+Run"]), + (MigrationType.ROLLBACK, ScopeType.COURSE, None, ["ccx-v1:Org+Course+Run"]), + ) + @unpack + def test_marks_failed_when_migration_input_invalid(self, migration_type, scope_type, org_id, course_id_list): + """``_validate_migration_input`` raises (e.g. non-course-v1 keys); orchestration marks FAILED.""" + scope_key = org_id if scope_type == ScopeType.ORG else course_id_list[0] if course_id_list else None + + run_course_authoring_migration( + migration_type=migration_type, + scope_type=scope_type, + scope_key=scope_key, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=course_id_list, + org_id=org_id, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=scope_key, status=Status.FAILED) + self.assertIn("error", run.metadata) + self.assertEqual(run.migration_type, migration_type) + self.assertEqual(run.scope_type, scope_type) + self.assertEqual(run.scope_key, scope_key) + + def test_forward_partial_success_when_legacy_role_unknown(self): + """Unknown legacy role strings are reported as errors; run ends PARTIAL_SUCCESS.""" + user = User.objects.create_user(username="legacy_unknown_user", email="legacy_unknown_user@example.com") + role = "not_a_defined_legacy_role" + CourseAccessRole.objects.create(user=user, org=self.ORG_ID, course_id=self.COURSE_ID, role=role) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_ID], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertGreaterEqual(run.metadata.get("error_count"), 1) + self.assertEqual(run.metadata.get("success_count"), 0) + self.assertIn(MigrationErrorReason.UNKNOWN_ROLE, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.UNKNOWN_ROLE][0] + self.assertEqual(error["subject"], user.username) + self.assertEqual(error["role"], role) + self.assertEqual(error["details"], f"Unknown access level: {role} for User: {user.username}") + + def test_rollback_partial_success_when_authz_role_unknown(self): + """Roles with no legacy ``CourseAccessRole`` mapping (e.g. ``course_editor``) → PARTIAL_SUCCESS.""" + user = User.objects.create_user(username="rb_no_legacy_user", email="rb_no_legacy_user@example.com") + role = COURSE_EDITOR.external_key + assign_role_to_user_in_scope(user.username, role, self.COURSE_ID) + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_ID], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertGreaterEqual(run.metadata.get("error_count"), 1) + self.assertEqual(run.metadata.get("success_count"), 0) + self.assertIn(MigrationErrorReason.NO_LEGACY_EQUIVALENT, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.NO_LEGACY_EQUIVALENT][0] + self.assertEqual(error["subject"], user.username) + self.assertEqual(error["role"], role) + self.assertEqual(error["details"], f"Role '{role}' has no legacy equivalent.") + + def test_forward_partial_success_when_assignment_failed_on_duplicate(self): + """Second FORWARD over the same legacy row hits duplicate AuthZ assignment → ``ASSIGNMENT_FAILED``.""" + user = User.objects.create_user(username="dup_assign_user", email="dup_assign_user@example.com") + CourseAccessRole.objects.create(user=user, org=self.ORG_ID, course_id=self.COURSE_ID, role="staff") + common = { + "migration_type": MigrationType.FORWARD, + "scope_type": ScopeType.COURSE, + "scope_key": self.COURSE_ID, + "course_access_role_model": CourseAccessRole, + "user_subject_model": UserSubject, + "course_id_list": [self.COURSE_ID], + "org_id": None, + "excluded_course_ids": frozenset(), + "delete_after_migration": False, + } + + run_course_authoring_migration(**common) + run_obj = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.COMPLETED) + self.assertEqual(run_obj.metadata.get("success_count"), 1) + + run_course_authoring_migration(**common) + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertEqual(run.metadata.get("success_count"), 0) + self.assertGreaterEqual(run.metadata.get("error_count"), 1) + self.assertIn(MigrationErrorReason.ASSIGNMENT_FAILED, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.ASSIGNMENT_FAILED][0] + self.assertEqual(error["subject"], user.username) + self.assertEqual(error["role"], "staff") + self.assertEqual(error["details"], f"User '{user.username}' may already have this permission assigned") + + @patch("openedx_authz.engine.utils.migrate_legacy_course_roles_to_authz") + def test_forward_partial_success_when_no_scope(self, mock_migrate): + """``NO_SCOPE`` is surfaced when the migration body reports neither course nor org (orchestration path).""" + mock_migrate.return_value = ( + [ + MigrationMetadata( + subject="no_scope_user", + role="staff", + reason=MigrationErrorReason.NO_SCOPE, + details="No scope", + ) + ], + [], + ) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_ID], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.NO_SCOPE, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.NO_SCOPE][0] + self.assertEqual(error["subject"], "no_scope_user") + self.assertEqual(error["role"], "staff") + self.assertEqual(error["details"], "No scope") + + @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") + def test_rollback_partial_success_when_unexpected_scope_type(self, mock_migrate): + """``UNEXPECTED_SCOPE_TYPE`` from rollback migration is grouped in run metadata.""" + mock_migrate.return_value = ( + [ + MigrationMetadata( + subject="u", + role="course_staff", + scope=self.COURSE_ID, + reason=MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, + details="UnexpectedScope", + ) + ], + [], + ) + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_ID], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.UNEXPECTED_SCOPE_TYPE][0] + self.assertEqual(error["subject"], "u") + self.assertEqual(error["role"], "course_staff") + self.assertEqual(error["scope"], self.COURSE_ID) + self.assertEqual(error["details"], "UnexpectedScope") + + @patch("openedx_authz.engine.utils.get_all_role_assignments_per_scope_type") + def test_rollback_partial_success_when_unexpected_error(self, mock_role_assignments): + """Exceptions inside the rollback loop become ``UNEXPECTED_ERROR`` entries in metadata.""" + missing_username = "missing_user_no_subject" + mock_role_assignments.return_value = [ + RoleAssignmentData( + subject=UserData(external_key=missing_username), + roles=[RoleData(external_key=COURSE_STAFF.external_key)], + scope=CourseOverviewData(external_key=self.COURSE_ID), + ) + ] + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_ID], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.UNEXPECTED_ERROR, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.UNEXPECTED_ERROR][0] + self.assertEqual(error["subject"], missing_username) + self.assertEqual(error["role"], COURSE_STAFF.external_key) + self.assertEqual(error["scope"], self.COURSE_ID) + self.assertEqual(error["details"], str(KeyError(missing_username))) + + def test_forward_completes_org_wide_legacy_when_org_id(self): + """FORWARD with ``org_id`` migrates org-level (no ``course_id``) legacy roles for that org.""" + user = User.objects.create_user(username="rcam_org_fwd_user", email="rcam_org_fwd_user@example.com") + CourseAccessRole.objects.create( + user=user, + org=self.ORG_ID, + course_id=CourseKeyField.Empty, + role="instructor", + ) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.ORG, + scope_key=self.ORG_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.ORG_ID, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.ORG_ID, status=Status.COMPLETED) + self.assertEqual(run.metadata.get("success_count"), 1) + self.assertEqual(run.metadata.get("error_count"), 0) + self.assertListEqual( + run.metadata.get("successes"), + [ + { + "subject": user.username, + "role": "instructor", + "scope": OrgCourseOverviewGlobData.build_external_key(self.ORG_ID), + } + ], + ) + + def test_rollback_completes_org_glob_assignment_when_org_id(self): + """ROLLBACK with ``org_id`` processes org glob / course scopes for that organization.""" + user = User.objects.create_user(username="rcam_org_rb_user", email="rcam_org_rb_user@example.com") + glob_scope = OrgCourseOverviewGlobData.build_external_key(self.ORG_ID) + assign_role_to_user_in_scope(user.username, COURSE_STAFF.external_key, glob_scope) + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.ORG, + scope_key=self.ORG_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.ORG_ID, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.ORG_ID, status=Status.COMPLETED) + self.assertGreaterEqual(run.metadata.get("success_count"), 1) + self.assertEqual(run.metadata.get("error_count"), 0) + self.assertListEqual( + run.metadata.get("successes"), + [{"subject": user.username, "role": COURSE_STAFF.external_key, "scope": glob_scope}], + ) diff --git a/openedx_authz/tests/test_models.py b/openedx_authz/tests/test_models.py index 94e41a43..60371347 100644 --- a/openedx_authz/tests/test_models.py +++ b/openedx_authz/tests/test_models.py @@ -3,6 +3,7 @@ This test suite verifies the functionality of the authorization models including: - ExtendedCasbinRule model with metadata and relationships - Cascade deletion behavior across model hierarchies +- AuthzCourseAuthoringMigrationRun (migration tracking and uniqueness on RUNNING inserts) These tests use the stub ContentLibrary model from openedx_authz.tests.stubs.models instead of the real ContentLibrary model, allowing them to run without the full @@ -18,12 +19,19 @@ from casbin_adapter.models import CasbinRule from django.contrib.auth import get_user_model +from django.db import IntegrityError from django.test import TestCase from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_authz.api.data import ContentLibraryData, CourseOverviewData, UserData from openedx_authz.models import ExtendedCasbinRule, Scope, Subject +from openedx_authz.models.authz_migration import ( + AuthzCourseAuthoringMigrationRun, + MigrationType, + ScopeType, + Status, +) from openedx_authz.models.engine import PolicyCacheControl from openedx_authz.tests.stubs.models import ContentLibrary, CourseOverview @@ -311,3 +319,114 @@ def test_singleton_behavior(self): instance1.save() all_instances = PolicyCacheControl.objects.all() self.assertEqual(all_instances.count(), 1) + + +class TestAuthzCourseAuthoringMigrationRun(TestCase): + """Tests for ``AuthzCourseAuthoringMigrationRun`` lifecycle and uniqueness rules.""" + + def setUp(self): + self.scope_key = "course-v1:TestOrg+AuthzMigrationRun+2024" + + def test_create_running(self): + """``create_running`` stores RUNNING and optional metadata.""" + meta = {"batch": 1} + + run = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, self.scope_key, metadata=meta + ) + + run.refresh_from_db() + self.assertEqual(run.migration_type, MigrationType.FORWARD) + self.assertEqual(run.scope_type, ScopeType.COURSE) + self.assertEqual(run.scope_key, self.scope_key) + self.assertEqual(run.status, Status.RUNNING) + self.assertEqual(run.metadata, meta) + + def test_create_skipped_merges_skip_reason(self): + """``create_skipped`` records SKIPPED and documents why.""" + run = AuthzCourseAuthoringMigrationRun.create_skipped( + MigrationType.ROLLBACK, ScopeType.ORG, "test_org", metadata={"note": "extra"} + ) + + self.assertEqual(run.status, Status.SKIPPED) + self.assertEqual(run.metadata["note"], "extra") + self.assertIn("skip_reason", run.metadata) + + def test_str_representation(self): + """``__str__`` includes id, migration type, scope, and status.""" + run = AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.scope_key) + + text = str(run) + self.assertIn(str(run.id), text) + self.assertIn(MigrationType.FORWARD.value, text) + self.assertIn(self.scope_key, text) + self.assertIn(Status.RUNNING.value, text) + + def test_second_running_insert_same_scope_raises_integrity_error(self): + """At most one RUNNING row per (scope_type, scope_key) on insert.""" + AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.scope_key) + + with self.assertRaises(IntegrityError): + AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.scope_key) + + def test_running_allowed_after_previous_run_finished(self): + """A new RUNNING row is allowed once the prior run for that scope is no longer RUNNING.""" + first = AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.scope_key) + first.mark_completed() + + second = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, self.scope_key + ) + + self.assertEqual(second.status, Status.RUNNING) + self.assertNotEqual(first.pk, second.pk) + + def test_distinct_scope_type_allows_parallel_running_same_key_string(self): + """RUNNING uniqueness is per (scope_type, scope_key), not key alone.""" + key = "same-key-string" + + AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, key) + other = AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.ORG, key) + + self.assertEqual(other.status, Status.RUNNING) + + def test_mark_completed_and_partial_success_merge_metadata(self): + """Finalizers set status, ``completed_at``, and merge metadata.""" + run = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, self.scope_key, metadata={"a": 1} + ) + run.mark_completed(metadata_updates={"success_count": 3}) + + run.refresh_from_db() + self.assertEqual(run.status, Status.COMPLETED) + self.assertIsNotNone(run.completed_at) + self.assertEqual(run.metadata["a"], 1) + self.assertEqual(run.metadata["success_count"], 3) + + run2 = AuthzCourseAuthoringMigrationRun.create_running(MigrationType.ROLLBACK, ScopeType.ORG, "org_partial") + run2.mark_partial_success(metadata_updates={"error_count": 2}) + + run2.refresh_from_db() + self.assertEqual(run2.status, Status.PARTIAL_SUCCESS) + self.assertEqual(run2.metadata["error_count"], 2) + + def test_mark_failed_with_and_without_exception(self): + """``mark_failed`` records FAILED, optional exception string is stored when provided.""" + run = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, f"{self.scope_key}_fail" + ) + run.mark_failed(exception=ValueError("boom")) + + run.refresh_from_db() + self.assertEqual(run.status, Status.FAILED) + self.assertEqual(run.metadata["error"], "boom") + + run2 = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, f"{self.scope_key}_fail2" + ) + prior_meta = dict(run2.metadata) + run2.mark_failed() + + run2.refresh_from_db() + self.assertEqual(run2.status, Status.FAILED) + self.assertEqual(run2.metadata, prior_meta) diff --git a/requirements/base.in b/requirements/base.in index 99d7082a..11b452c6 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -13,3 +13,4 @@ edx-api-doc-tools # Tools for API documentation edx-django-utils # Used for RequestCache edx-drf-extensions # Extensions for Django Rest Framework used by Open edX edx-organizations # Organizations library for Open edX +django-waffle # Waffle library for feature flag management diff --git a/requirements/base.txt b/requirements/base.txt index 830c1607..1be06872 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -48,6 +48,7 @@ django-simple-history==3.11.0 # via edx-organizations django-waffle==5.0.0 # via + # -r requirements/base.in # edx-django-utils # edx-drf-extensions djangorestframework==3.17.1 @@ -84,7 +85,7 @@ edx-opaque-keys==4.0.0 # edx-organizations edx-organizations==8.0.0 # via -r requirements/base.in -idna==3.11 +idna==3.12 # via requests inflection==0.5.1 # via drf-yasg @@ -106,7 +107,7 @@ pyjwt[crypto]==2.12.1 # via # drf-jwt # edx-drf-extensions -pymongo==4.16.0 +pymongo==4.17.0 # via edx-opaque-keys pynacl==1.6.2 # via edx-django-utils diff --git a/requirements/ci.txt b/requirements/ci.txt index fb94db11..441e26a3 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -4,7 +4,7 @@ # # make upgrade # -cachetools==7.0.5 +cachetools==7.0.6 # via tox colorama==0.4.6 # via tox diff --git a/requirements/dev.txt b/requirements/dev.txt index 98a0f9ed..19a8d456 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -23,7 +23,7 @@ build==1.4.3 # via # -r requirements/pip-tools.txt # pip-tools -cachetools==7.0.5 +cachetools==7.0.6 # via # -r requirements/ci.txt # tox @@ -169,7 +169,7 @@ filelock==3.29.0 # python-discovery # tox # virtualenv -idna==3.11 +idna==3.12 # via # -r requirements/quality.txt # requests @@ -287,7 +287,7 @@ pylint-plugin-utils==0.9.0 # -r requirements/quality.txt # pylint-celery # pylint-django -pymongo==4.16.0 +pymongo==4.17.0 # via # -r requirements/quality.txt # edx-opaque-keys diff --git a/requirements/doc.txt b/requirements/doc.txt index babb2ad5..6b91f191 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -143,7 +143,7 @@ edx-organizations==8.0.0 # via -r requirements/test.txt id==1.6.1 # via twine -idna==3.11 +idna==3.12 # via # -r requirements/test.txt # requests @@ -236,7 +236,7 @@ pyjwt[crypto]==2.12.1 # -r requirements/test.txt # drf-jwt # edx-drf-extensions -pymongo==4.16.0 +pymongo==4.17.0 # via # -r requirements/test.txt # edx-opaque-keys diff --git a/requirements/quality.txt b/requirements/quality.txt index 0432e279..91afebf2 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -133,7 +133,7 @@ edx-opaque-keys==4.0.0 # edx-organizations edx-organizations==8.0.0 # via -r requirements/test.txt -idna==3.11 +idna==3.12 # via # -r requirements/test.txt # requests @@ -212,7 +212,7 @@ pylint-plugin-utils==0.9.0 # via # pylint-celery # pylint-django -pymongo==4.16.0 +pymongo==4.17.0 # via # -r requirements/test.txt # edx-opaque-keys diff --git a/requirements/test.txt b/requirements/test.txt index 7ea01398..f48ebd0c 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -116,7 +116,7 @@ edx-opaque-keys==4.0.0 # edx-organizations edx-organizations==8.0.0 # via -r requirements/base.txt -idna==3.11 +idna==3.12 # via # -r requirements/base.txt # requests @@ -164,7 +164,7 @@ pyjwt[crypto]==2.12.1 # -r requirements/base.txt # drf-jwt # edx-drf-extensions -pymongo==4.16.0 +pymongo==4.17.0 # via # -r requirements/base.txt # edx-opaque-keys