From 724662c86fa603eb330a3ce6ec92dfb3d6b1f63c Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Tue, 17 Feb 2026 21:10:27 -0600 Subject: [PATCH 01/18] feat: add course authoring migration and rollback scripts - Add `authz_migrate_course_authoring` command to migrate legacy CourseAccessRole data to the new Authz (Casbin-based) system - Add `authz_rollback_course_authoring` command to rollback Authz roles back to legacy CourseAccessRole - Support optional `--delete` flag for controlled cleanup of source permissions after successful migration - Add `migrate_legacy_course_roles_to_authz` and `migrate_authz_to_legacy_course_roles` service functions - Add unit tests to verify migration and command behavior --- openedx_authz/constants/roles.py | 5 + openedx_authz/engine/utils.py | 159 +++++- .../authz_migrate_course_authoring.py | 68 +++ .../authz_rollback_course_authoring.py | 75 +++ .../0008_migrate_course_roles_to_authz.py | 56 ++ openedx_authz/tests/stubs/models.py | 54 ++ openedx_authz/tests/test_migrations.py | 516 +++++++++++++++++- 7 files changed, 926 insertions(+), 7 deletions(-) create mode 100644 openedx_authz/management/commands/authz_migrate_course_authoring.py create mode 100644 openedx_authz/management/commands/authz_rollback_course_authoring.py create mode 100644 openedx_authz/migrations/0008_migrate_course_roles_to_authz.py diff --git a/openedx_authz/constants/roles.py b/openedx_authz/constants/roles.py index 4ac6f941..e7c81212 100644 --- a/openedx_authz/constants/roles.py +++ b/openedx_authz/constants/roles.py @@ -160,6 +160,11 @@ permissions.COURSES_EXPORT_TAGS, ] +COURSE_LIMITED_STAFF_PERMISSIONS = [] + +COURSE_DATA_RESEARCHER_PERMISSIONS = [] + +COURSE_ADMIN = RoleData(external_key="course_admin", permissions=COURSE_ADMIN_PERMISSIONS) COURSE_STAFF = RoleData(external_key="course_staff", permissions=COURSE_STAFF_PERMISSIONS) COURSE_LIMITED_STAFF_PERMISSIONS = [ diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 5d15fd65..6b2c2176 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -8,8 +8,21 @@ from casbin import Enforcer -from openedx_authz.api.users import assign_role_to_user_in_scope, batch_assign_role_to_users_in_scope -from openedx_authz.constants.roles import LIBRARY_ADMIN, LIBRARY_AUTHOR, LIBRARY_USER +from openedx_authz.api.users import ( + assign_role_to_user_in_scope, + batch_assign_role_to_users_in_scope, + batch_unassign_role_from_users, + get_user_role_assignments, +) +from openedx_authz.constants.roles import ( + COURSE_ADMIN, + COURSE_DATA_RESEARCHER, + COURSE_LIMITED_STAFF, + COURSE_STAFF, + LIBRARY_ADMIN, + LIBRARY_AUTHOR, + LIBRARY_USER, +) logger = logging.getLogger(__name__) @@ -151,3 +164,145 @@ def migrate_legacy_permissions(ContentLibraryPermission): ) return permissions_with_errors + + +def migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration): + """ + Migrate legacy course role data to the new Casbin-based authorization model. + This function reads legacy permissions from the CourseAccessRole model + and assigns equivalent roles in the new authorization system. + + The old Course permissions are stored in the CourseAccessRole model, it consists of the following columns: + + - user: FK to User + - org: optional Organization string + - course_id: optional CourseKeyField of Course + - role: 'instructor' | 'staff' | 'limited_staff' | 'data_researcher' + + In the new Authz model, this would roughly translate to: + + - course_id: scope + - user: subject + - role: role + + param CourseAccessRole: The CourseAccessRole model to use. + """ + + legacy_permissions = CourseAccessRole.objects.select_related("user").all() + + # List to keep track of any permissions that could not be migrated + permissions_with_errors = [] + permissions_with_no_errors = [] + + for permission in legacy_permissions: + # Migrate the permission to the new model + + # Derive equivalent role based on access level + map_legacy_role = { + "instructor": COURSE_ADMIN, + "staff": COURSE_STAFF, + "limited_staff": COURSE_LIMITED_STAFF, + "data_researcher": COURSE_DATA_RESEARCHER, + } + + role = map_legacy_role.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) + continue + + # Permission applied to individual user + logger.info( + f"Migrating permission for User: {permission.user.username} " + f"to Role: {role.external_key} in Scope: {permission.course_id}" + ) + + assign_role_to_user_in_scope( + user_external_key=permission.user.username, + role_external_key=role.external_key, + scope_external_key=str(permission.course_id), + ) + permissions_with_no_errors.append(permission) + + if delete_after_migration: + CourseAccessRole.objects.filter(id__in=[p.id for p in permissions_with_no_errors]).delete() + + return permissions_with_errors + + +def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_after_migration): + """ + 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 + CourseAccessRole model. + + This is essentially the reverse of migrate_legacy_course_roles_to_authz and is intended + for rollback purposes in case of migration issues. + """ + # 1. Get all users with course-related permissions in the new model by filtering + # UserSubjects that are linked to CourseScopes with a valid course overview. + course_subjects = ( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .select_related("user") + .distinct() + ) + + roles_with_errors = [] + + for course_subject in course_subjects: + user = course_subject.user + user_external_key = user.username + + # 2. Get all role assignments for the user + role_assignments = get_user_role_assignments(user_external_key=user_external_key) + + for assignment in role_assignments: + scope = assignment.scope.external_key + + course_overview = assignment.scope.get_object() + + for role in assignment.roles: + # We are only interested in course-related scopes and roles + if not scope.startswith("course-v1:"): + continue + + # Map new roles back to legacy roles + role_to_legacy_role = { + COURSE_ADMIN.external_key: "instructor", + COURSE_STAFF.external_key: "staff", + COURSE_LIMITED_STAFF.external_key: "limited_staff", + COURSE_DATA_RESEARCHER.external_key: "data_researcher", + } + + legacy_role = role_to_legacy_role.get(role.external_key) + if legacy_role is None: + logger.error(f"Unknown role: {role} for User: {user_external_key}") + roles_with_errors.append((user_external_key, role.external_key, scope)) + continue + + try: + # Create legacy CourseAccessRole entry + CourseAccessRole.objects.get_or_create( + user=user, + org=course_overview.org, + course_id=scope, + role=legacy_role, + ) + except Exception as e: # pylint: disable=broad-exception-caught + logger.error( + f"Error creating CourseAccessRole for User: " + f"{user_external_key}, Role: {legacy_role}, Course: {scope}: {e}" + ) + roles_with_errors.append((user_external_key, role.external_key, scope)) + continue + + # If we successfully created the legacy role, we can unassign the new role + if delete_after_migration: + batch_unassign_role_from_users( + users=[user_external_key], + role_external_key=role.external_key, + scope_external_key=scope, + ) + return roles_with_errors diff --git a/openedx_authz/management/commands/authz_migrate_course_authoring.py b/openedx_authz/management/commands/authz_migrate_course_authoring.py new file mode 100644 index 00000000..4d95fa4f --- /dev/null +++ b/openedx_authz/management/commands/authz_migrate_course_authoring.py @@ -0,0 +1,68 @@ +""" +Django management command to migrate legacy course authoring roles to the new Authz (Casbin-based) authorization system. +""" + +from django.core.management.base import BaseCommand +from django.db import transaction + +from openedx_authz.engine.utils import migrate_legacy_course_roles_to_authz + +try: + from common.djangoapps.student.models import CourseAccessRole +except ImportError: + CourseAccessRole = None # type: ignore + + +class Command(BaseCommand): + """ + Django command to migrate legacy CourseAccessRole data + to the new Authz (Casbin-based) authorization system. + """ + + help = "Migrate legacy course authoring roles to the new Authz system." + + def add_arguments(self, parser): + parser.add_argument( + "--delete", + action="store_true", + help="Delete legacy CourseAccessRole records after successful migration.", + ) + + def handle(self, *args, **options): + delete_after_migration = options["delete"] + + self.stdout.write(self.style.WARNING("Starting legacy → Authz migration...")) + + try: + with transaction.atomic(): + errors = migrate_legacy_course_roles_to_authz( + CourseAccessRole=CourseAccessRole, + delete_after_migration=False, # control deletion here instead + ) + + if errors: + self.stdout.write(self.style.ERROR(f"Migration completed with {len(errors)} errors.")) + else: + self.stdout.write(self.style.SUCCESS("Migration completed successfully with no errors.")) + + # Handle deletion separately for safety + if delete_after_migration: + confirm = input( + "Are you sure you want to delete successfully migrated legacy roles? Type 'yes' to continue: " + ) + + if confirm != "yes": + self.stdout.write(self.style.WARNING("Deletion aborted.")) + return + + migrated_ids = [p.id for p in CourseAccessRole.objects.all() if p not in errors] + + CourseAccessRole.objects.filter(id__in=migrated_ids).delete() + + self.stdout.write(self.style.SUCCESS("Legacy roles deleted successfully.")) + + except Exception as exc: + self.stdout.write(self.style.ERROR(f"Migration failed due to unexpected error: {exc}")) + raise + + self.stdout.write(self.style.SUCCESS("Done.")) diff --git a/openedx_authz/management/commands/authz_rollback_course_authoring.py b/openedx_authz/management/commands/authz_rollback_course_authoring.py new file mode 100644 index 00000000..d43a05dc --- /dev/null +++ b/openedx_authz/management/commands/authz_rollback_course_authoring.py @@ -0,0 +1,75 @@ +""" +Django management command to rollback course authoring roles from the new Authz (Casbin-based) +authorization system back to the legacy CourseAccessRole model. +""" + +from django.core.management.base import BaseCommand +from django.db import transaction + +from openedx_authz.engine.utils import migrate_authz_to_legacy_course_roles +from openedx_authz.models.subjects import UserSubject + +try: + from common.djangoapps.student.models import CourseAccessRole +except ImportError: + CourseAccessRole = None # type: ignore + + +class Command(BaseCommand): + """ + Django command to rollback course authoring roles + from the new Authz system back to legacy CourseAccessRole. + """ + + help = "Rollback Authz course authoring roles to legacy CourseAccessRole." + + def add_arguments(self, parser): + parser.add_argument( + "--delete", + action="store_true", + help="Delete Authz role assignments after successful rollback.", + ) + + def handle(self, *args, **options): + delete_after_migration = options["delete"] + + self.stdout.write(self.style.WARNING("Starting Authz → Legacy rollback migration...")) + + try: + with transaction.atomic(): + errors = migrate_authz_to_legacy_course_roles( + CourseAccessRole=CourseAccessRole, + UserSubject=UserSubject, + delete_after_migration=False, # control deletion here + ) + + if errors: + self.stdout.write(self.style.ERROR(f"Rollback completed with {len(errors)} errors.")) + else: + self.stdout.write(self.style.SUCCESS("Rollback completed successfully with no errors.")) + + # Handle deletion separately for safety + if delete_after_migration: + confirm = input( + "Are you sure you want to remove the new Authz role " + "assignments after rollback? Type 'yes' to continue: " + ) + + if confirm != "yes": + self.stdout.write(self.style.WARNING("Deletion aborted.")) + return + + # Re-run with deletion enabled + migrate_authz_to_legacy_course_roles( + CourseAccessRole=CourseAccessRole, + UserSubject=UserSubject, + delete_after_migration=True, + ) + + self.stdout.write(self.style.SUCCESS("Authz role assignments removed successfully.")) + + except Exception as exc: + self.stdout.write(self.style.ERROR(f"Rollback failed due to unexpected error: {exc}")) + raise + + self.stdout.write(self.style.SUCCESS("Done.")) diff --git a/openedx_authz/migrations/0008_migrate_course_roles_to_authz.py b/openedx_authz/migrations/0008_migrate_course_roles_to_authz.py new file mode 100644 index 00000000..11ff1bd7 --- /dev/null +++ b/openedx_authz/migrations/0008_migrate_course_roles_to_authz.py @@ -0,0 +1,56 @@ +# Generated by Django 4.2.24 on 2026-02-17 22:31 + +import logging + +from django.db import migrations + +from openedx_authz.engine.utils import migrate_legacy_course_roles_to_authz + +logger = logging.getLogger(__name__) + + +def _log_migration_errors(permissions_with_errors: list) -> None: + """ + Log the permissions that could not be migrated during the migration process. + Args: + permissions_with_errors (list): List of CourseAccessRole instances that failed to migrate. + """ + logger.error( + f"Migration completed with errors for {len(permissions_with_errors)} permissions.\n" + "The following permissions could not be migrated:" + ) + for permission in permissions_with_errors: + logger.error( + "Role: %s, %sCourse: %s", + permission.role, + f"User: {permission.user.username}, " if permission.user else "", + permission.course_id, + ) + + +def apply_migrate_legacy_course_roles_to_authz(apps, schema_editor): + """ + Wrapper to run the migration using the historical version of the CourseAccessRole model. + """ + # CourseAccessRole model from the student app, here is where the legacy course roles are stored + try: + CourseAccessRole = apps.get_model("student", "CourseAccessRole") + except LookupError: + # Don't run the migration where the student app is not installed, like during development. + logger.warning("CourseAccessRole model not found. Skipping migration.") + return + + permissions_with_errors = migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration=True) + + if permissions_with_errors: + _log_migration_errors(permissions_with_errors) + + +class Migration(migrations.Migration): + dependencies = [ + ("openedx_authz", "0007_coursescope"), + ] + + operations = [ + migrations.RunPython(apply_migrate_legacy_course_roles_to_authz), + ] diff --git a/openedx_authz/tests/stubs/models.py b/openedx_authz/tests/stubs/models.py index c64995aa..86c39079 100644 --- a/openedx_authz/tests/stubs/models.py +++ b/openedx_authz/tests/stubs/models.py @@ -125,3 +125,57 @@ def get_from_id(cls, course_key): key = str(course_key) obj, _ = cls.objects.get_or_create(id=key) return obj + + +class NoneToEmptyQuerySet(models.query.QuerySet): + """ + A :class:`django.db.query.QuerySet` that replaces `None` values passed to `filter` and `exclude` + with the corresponding `Empty` value for all fields with an `Empty` attribute. + + This is to work around Django automatically converting `exact` queries for `None` into + `isnull` queries before the field has a chance to convert them to queries for it's own + empty value. + """ + + def _filter_or_exclude(self, *args, **kwargs): + for field_object in self.model._meta.get_fields(): + direct = not field_object.auto_created or field_object.concrete + if direct and hasattr(field_object, "Empty"): + for suffix in ("", "_exact"): + key = f"{field_object.name}{suffix}" + if key in kwargs and kwargs[key] is None: + kwargs[key] = field_object.Empty + + return super()._filter_or_exclude(*args, **kwargs) + + +class NoneToEmptyManager(models.Manager): + """ + A :class:`django.db.models.Manager` that has a :class:`NoneToEmptyQuerySet` + as its `QuerySet`, initialized with a set of specified `field_names`. + """ + + def get_queryset(self): + """ + Returns the result of NoneToEmptyQuerySet instead of a regular QuerySet. + """ + return NoneToEmptyQuerySet(self.model, using=self._db) + + +class CourseAccessRole(models.Model): + """ + Maps users to org, courses, and roles. Used by student.roles.CourseRole and OrgRole. + To establish a user as having a specific role over all courses in the org, create an entry + without a course_id. + + .. no_pii: + """ + + objects = NoneToEmptyManager() + + user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE) + # blank org is for global group based roles such as course creator (may be deprecated) + org = models.CharField(max_length=64, db_index=True, blank=True) + # 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) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 5d612d01..7a7b79f6 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -1,14 +1,35 @@ """Unit Tests for openedx_authz migrations.""" +from unittest.mock import patch + from django.contrib.auth import get_user_model from django.contrib.auth.models import Group +from django.core.management import call_command from django.test import TestCase from openedx_authz.api.users import batch_unassign_role_from_users, get_user_role_assignments_in_scope -from openedx_authz.constants.roles import LIBRARY_ADMIN, LIBRARY_USER +from openedx_authz.constants.roles import ( + COURSE_ADMIN, + COURSE_DATA_RESEARCHER, + COURSE_LIMITED_STAFF, + COURSE_STAFF, + LIBRARY_ADMIN, + LIBRARY_USER, +) from openedx_authz.engine.enforcer import AuthzEnforcer -from openedx_authz.engine.utils import migrate_legacy_permissions -from openedx_authz.tests.stubs.models import ContentLibrary, ContentLibraryPermission, Organization +from openedx_authz.engine.utils import ( + migrate_authz_to_legacy_course_roles, + migrate_legacy_course_roles_to_authz, + migrate_legacy_permissions, +) +from openedx_authz.models.subjects import UserSubject +from openedx_authz.tests.stubs.models import ( + ContentLibrary, + ContentLibraryPermission, + CourseAccessRole, + CourseOverview, + Organization, +) User = get_user_model() @@ -26,8 +47,8 @@ empty_group_name = f"{OBJECT_PREFIX}empty_group" -class TestLegacyPermissionsMigration(TestCase): - """Test cases for migrating legacy permissions.""" +class TestLegacyContentLibraryPermissionsMigration(TestCase): + """Test cases for migrating legacy content library permissions.""" def setUp(self): """ @@ -146,3 +167,488 @@ def test_migration(self): self.assertEqual(assignments[0].roles[0], LIBRARY_USER) self.assertEqual(len(permissions_with_errors), 2) + + +class TestLegacyCourseAuthoringPermissionsMigration(TestCase): + """Test cases for migrating legacy course authoring permissions.""" + + def setUp(self): + """ + Set up test data: + + What this does: + 1. Defines an Org and a CourseKey for the test course + 2. Create Users for each legacy role and an additional user for testing invalid permissions + 3. Assign legacy permissions using CourseAccessRole for each user and role combination + 4. Create invalid permissions for user to test error logging + - Notes: + - CourseAccessRole does not have a group concept, so we are only assigning + permissions to individual users in this test. + - The only roles we are migrating are instructor, staff, limited_staff and data_researcher, + any other role in CourseAccessRole will be considered invalid for the purpose of this test. + """ + + # Defining course identifiers + self.org = org_short_name + self.course_id = f"course-v1:{self.org}+{OBJECT_PREFIX}course+2024" + default_course_fields = { + "org": self.org, + "course_id": self.course_id, + } + self.course_overview = CourseOverview.objects.create( + id=self.course_id, org=self.org, display_name=f"{OBJECT_PREFIX} Course" + ) + + # Create lists to hold legacy role objects for cleanup and verification purposes + self.admin_legacy_roles = [] + self.staff_legacy_roles = [] + self.limited_staff_legacy_roles = [] + self.data_researcher_legacy_roles = [] + + # Create users for each legacy role and an additional user for testing invalid permissions + self.admin_users = [ + User.objects.create_user(username=f"admin_{user_name}", email=f"admin_{user_name}@example.com") + for user_name in user_names + ] + + self.staff_users = [ + User.objects.create_user(username=f"staff_{user_name}", email=f"staff_{user_name}@example.com") + for user_name in user_names + ] + + self.limited_staff = [ + User.objects.create_user( + username=f"limited_staff_{user_name}", email=f"limited_staff_{user_name}@example.com" + ) + for user_name in user_names + ] + + self.data_researcher = [ + User.objects.create_user( + username=f"data_researcher_{user_name}", email=f"data_researcher_{user_name}@example.com" + ) + for user_name in user_names + ] + + self.error_user = User.objects.create_user(username=error_user_name, email=f"{error_user_name}@example.com") + + # Assign legacy permissions for users based on their role + for user in self.admin_users: + leg_role = CourseAccessRole.objects.create( + **default_course_fields, + user=user, + role="instructor", + ) + self.admin_legacy_roles.append(leg_role) + + for user in self.staff_users: + leg_role = CourseAccessRole.objects.create( + **default_course_fields, + user=user, + role="staff", + ) + self.staff_legacy_roles.append(leg_role) + + for user in self.limited_staff: + leg_role = CourseAccessRole.objects.create( + **default_course_fields, + user=user, + role="limited_staff", + ) + self.limited_staff_legacy_roles.append(leg_role) + + for user in self.data_researcher: + leg_role = CourseAccessRole.objects.create( + **default_course_fields, + user=user, + role="data_researcher", + ) + self.data_researcher_legacy_roles.append(leg_role) + + # Create invalid permission for testing error logging + CourseAccessRole.objects.create( + **default_course_fields, + user=self.error_user, + role="invalid-legacy-role", + ) + + def tearDown(self): + """ + Clean up test data created for the migration test. + """ + super().tearDown() + AuthzEnforcer.get_enforcer().load_policy() + + admin_users_names = [user.username for user in self.admin_users] + staff_users_names = [user.username for user in self.staff_users] + limited_staff_users_names = [user.username for user in self.limited_staff] + data_researcher_users_names = [user.username for user in self.data_researcher] + + batch_unassign_role_from_users( + users=admin_users_names, + role_external_key=COURSE_ADMIN.external_key, + scope_external_key=self.course_id, + ) + batch_unassign_role_from_users( + users=staff_users_names, + role_external_key=COURSE_STAFF.external_key, + scope_external_key=self.course_id, + ) + batch_unassign_role_from_users( + users=limited_staff_users_names, + role_external_key=COURSE_LIMITED_STAFF.external_key, + scope_external_key=self.course_id, + ) + batch_unassign_role_from_users( + users=data_researcher_users_names, + role_external_key=COURSE_DATA_RESEARCHER.external_key, + scope_external_key=self.course_id, + ) + + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): + """Test the migration of legacy permissions from CourseAccessRole to the new Casbin-based model + and the rollback functionality without deletion. + + 1. Run the migration to migrate legacy permissions from CourseAccessRole to the + new model with delete_after_migration set to False. + - Notes: + - The migration function should correctly map legacy roles to + the new roles based on the defined mapping in the migration function. + - Any legacy role that does not have a defined mapping should be logged as an error + and not migrated. + - After migration, all legacy CourseAccessRole entries should not be deleted + since we set delete_after_migration to False. + 2. Check that each user has the expected role in the new model. + 3. Check that invalid permissions were identified correctly as errors. + 4. Rollback the migration and check that each user has the expected legacy role and + that all migrated permissions were rolled back successfully. + """ + + # Capture the old state of permissions for rollback testing + original_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + self.assertEqual( + len(user_names), 3 + ) # Sanity check to ensure we have the expected number of users for each role + self.assertEqual( + len(original_state_access_roles), 13 + ) # 3 users for each of the 4 roles + 1 invalid role = 13 total entries + + # Migrate from legacy CourseAccessRole to new Casbin-based model + permissions_with_errors = migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration=False) + AuthzEnforcer.get_enforcer().load_policy() + 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), 1) + self.assertEqual(assignments[0].roles[0], COURSE_ADMIN) + for user in self.staff_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_STAFF) + for user in self.limited_staff: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_LIMITED_STAFF) + for user in self.data_researcher: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + 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].role, "invalid-legacy-role") + + after_migrate_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + # 3 users for each of the 4 roles + 1 invalid role = 13 total entries + self.assertEqual(len(after_migrate_state_access_roles), 13) + # Must be the same before and after migration since we set delete_after_migration to False + self.assertEqual(original_state_access_roles, after_migrate_state_access_roles) + + # Now let's rollback + + # Capture the state of permissions before rollback to verify that rollback restores the original state + original_state_user_subjects = list( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .distinct() + .order_by("id") + .values("id", "user_id") + ) + original_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + permissions_with_errors = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, delete_after_migration=False + ) + + # Check that each user has the expected legacy role after rollback and that errors + # are logged for any permissions that could not be rolled back + 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), 1) + self.assertEqual(assignments[0].roles[0], COURSE_ADMIN) + for user in self.staff_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_STAFF) + for user in self.limited_staff: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_LIMITED_STAFF) + for user in self.data_researcher: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_DATA_RESEARCHER) + self.assertEqual(len(permissions_with_errors), 0) + + state_after_migration_user_subjects = list( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .distinct() + .order_by("id") + .values("id", "user_id") + ) + after_migrate_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + # The number of CourseAccessRole entries should be the same as the original state + # since we are not deleting any entries in this test. + self.assertEqual(len(original_state_access_roles), 13) + + # All original entries should still be there since we are not deleting any entries + # and when creating new entries for the users that were migrated back to legacy roles, + # we are creating them with get_or_create which will not create duplicates if an entry + # with the same user, org, course_id and role already exists. + self.assertEqual(len(after_migrate_state_access_roles), 13) + + # Sanity check to ensure we have the expected number of UserSubjects related to + # the course permissions before migration (3 users * 4 roles = 12) + self.assertEqual(len(original_state_user_subjects), 12) + + # After rollback, we should have the same 12 UserSubjects related to the course permissions + # since we are not deleting any entries in this test, + self.assertEqual(len(state_after_migration_user_subjects), 12) + + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): + """Test the migration of legacy permissions from CourseAccessRole to + the new Casbin-based model with deletion of legacy permissions after migration. + + 1. Run the migration to migrate legacy permissions from CourseAccessRole to the + new model with delete_after_migration set to True. + - Notes: + - The migration function should correctly map legacy roles to + the new roles based on the defined mapping in the migration function. + - Any legacy role that does not have a defined mapping should be logged as an error + and not migrated. + - After migration, all legacy CourseAccessRole entries should be deleted + since we set delete_after_migration to True. + 2. Check that each user has the expected role in the new model. + 3. Check that invalid permissions were identified correctly as errors. + 4. Rollback the migration and check that each user has the expected legacy role and + that all migrated permissions were rolled back successfully. + """ + + # Capture the old state of permissions for rollback testing + original_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + self.assertEqual( + len(user_names), 3 + ) # Sanity check to ensure we have the expected number of users for each role + self.assertEqual( + len(original_state_access_roles), 13 + ) # 3 users for each of the 4 roles + 1 invalid role = 13 total entries + + # Migrate from legacy CourseAccessRole to new Casbin-based model + permissions_with_errors = migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration=True) + AuthzEnforcer.get_enforcer().load_policy() + 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), 1) + self.assertEqual(assignments[0].roles[0], COURSE_ADMIN) + for user in self.staff_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_STAFF) + for user in self.limited_staff: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_LIMITED_STAFF) + for user in self.data_researcher: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + 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].role, "invalid-legacy-role") + + after_migrate_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + self.assertEqual(len(original_state_access_roles), 13) + + # Only the invalid role entry should remain since we set delete_after_migration to True + self.assertEqual(len(after_migrate_state_access_roles), 1) + + # Must be different before and after migration since we set delete_after_migration + # to True and we are deleting all + self.assertNotEqual(original_state_access_roles, after_migrate_state_access_roles) + + # Now let's rollback + + # Capture the state of permissions before rollback to verify that rollback restores the original state + original_state_user_subjects = list( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .distinct() + .order_by("id") + .values("id", "user_id") + ) + original_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + permissions_with_errors = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, delete_after_migration=True + ) + + # Check that each user has the expected legacy role after rollback + # and that errors are logged for any permissions that could not be rolled back + 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) + for user in self.staff_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 0) + for user in self.limited_staff: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 0) + for user in self.data_researcher: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 0) + self.assertEqual(len(permissions_with_errors), 0) + + state_after_migration_user_subjects = list( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .distinct() + .order_by("id") + .values("id", "user_id") + ) + after_migrate_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + # Before the rollback, we should only have the 1 invalid role entry + # since we set delete_after_migration to True in the migration. + self.assertEqual(len(original_state_access_roles), 1) + + # All original entries + 3 users * 4 roles = 12 + # plus the original invalid entry = 1 + 12 = 13 total entries + self.assertEqual(len(after_migrate_state_access_roles), 1 + 12) + + # Sanity check to ensure we have the expected number of UserSubjects related to + # the course permissions before migration (3 users * 4 roles = 12) + self.assertEqual(len(original_state_user_subjects), 12) + + # After rollback, we should have 0 UserSubjects related to the course permissions + self.assertEqual(len(state_after_migration_user_subjects), 0) + + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") + def test_authz_migrate_course_authoring_command(self, mock_migrate): + """ + Verify that the authz_migrate_course_authoring command + calls migrate_legacy_course_roles_to_authz with the correct arguments. + """ + + mock_migrate.return_value = [] + + # Run without --delete + call_command("authz_migrate_course_authoring") + + mock_migrate.assert_called_once() + args, kwargs = mock_migrate.call_args + + self.assertEqual(kwargs["delete_after_migration"], False) + + mock_migrate.reset_mock() + + # Run with --delete + with patch("builtins.input", return_value="yes"): + call_command("authz_migrate_course_authoring", "--delete") + + mock_migrate.assert_called_once() + args, kwargs = mock_migrate.call_args + + self.assertEqual(kwargs["delete_after_migration"], False) + + @patch("openedx_authz.management.commands.authz_rollback_course_authoring.CourseAccessRole", CourseAccessRole) + @patch("openedx_authz.management.commands.authz_rollback_course_authoring.migrate_authz_to_legacy_course_roles") + def test_authz_rollback_course_authoring_command(self, mock_rollback): + """ + Verify that the authz_rollback_course_authoring command + calls migrate_authz_to_legacy_course_roles correctly. + """ + + mock_rollback.return_value = [] + + # Run without --delete + call_command("authz_rollback_course_authoring") + + mock_rollback.assert_called_once() + args, kwargs = mock_rollback.call_args + + self.assertEqual(kwargs["delete_after_migration"], False) + + mock_rollback.reset_mock() + + # Run with --delete + with patch("builtins.input", return_value="yes"): + call_command("authz_rollback_course_authoring", "--delete") + + # First call (normal) + # Second call (with deletion=True) + self.assertEqual(mock_rollback.call_count, 2) + + first_call_kwargs = mock_rollback.call_args_list[0][1] + second_call_kwargs = mock_rollback.call_args_list[1][1] + + self.assertEqual(first_call_kwargs["delete_after_migration"], False) + self.assertEqual(second_call_kwargs["delete_after_migration"], True) From bf5d76c8a6d4125acdb5b6a365309f1365c7c5f2 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 18 Feb 2026 16:43:17 -0600 Subject: [PATCH 02/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/engine/utils.py | 17 ++-- .../0008_migrate_course_roles_to_authz.py | 56 ----------- openedx_authz/tests/test_migrations.py | 93 +++++++++++++++++++ 3 files changed, 102 insertions(+), 64 deletions(-) delete mode 100644 openedx_authz/migrations/0008_migrate_course_roles_to_authz.py diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 6b2c2176..89ccb438 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -29,6 +29,15 @@ GROUPING_POLICY_PTYPES = ["g", "g2", "g3", "g4", "g5", "g6"] +# Map new roles back to legacy roles +role_to_legacy_role = { + COURSE_ADMIN.external_key: "instructor", + COURSE_STAFF.external_key: "staff", + COURSE_LIMITED_STAFF.external_key: "limited_staff", + COURSE_DATA_RESEARCHER.external_key: "data_researcher", +} + + def migrate_policy_between_enforcers( source_enforcer: Enforcer, target_enforcer: Enforcer, @@ -268,14 +277,6 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_a if not scope.startswith("course-v1:"): continue - # Map new roles back to legacy roles - role_to_legacy_role = { - COURSE_ADMIN.external_key: "instructor", - COURSE_STAFF.external_key: "staff", - COURSE_LIMITED_STAFF.external_key: "limited_staff", - COURSE_DATA_RESEARCHER.external_key: "data_researcher", - } - legacy_role = role_to_legacy_role.get(role.external_key) if legacy_role is None: logger.error(f"Unknown role: {role} for User: {user_external_key}") diff --git a/openedx_authz/migrations/0008_migrate_course_roles_to_authz.py b/openedx_authz/migrations/0008_migrate_course_roles_to_authz.py deleted file mode 100644 index 11ff1bd7..00000000 --- a/openedx_authz/migrations/0008_migrate_course_roles_to_authz.py +++ /dev/null @@ -1,56 +0,0 @@ -# Generated by Django 4.2.24 on 2026-02-17 22:31 - -import logging - -from django.db import migrations - -from openedx_authz.engine.utils import migrate_legacy_course_roles_to_authz - -logger = logging.getLogger(__name__) - - -def _log_migration_errors(permissions_with_errors: list) -> None: - """ - Log the permissions that could not be migrated during the migration process. - Args: - permissions_with_errors (list): List of CourseAccessRole instances that failed to migrate. - """ - logger.error( - f"Migration completed with errors for {len(permissions_with_errors)} permissions.\n" - "The following permissions could not be migrated:" - ) - for permission in permissions_with_errors: - logger.error( - "Role: %s, %sCourse: %s", - permission.role, - f"User: {permission.user.username}, " if permission.user else "", - permission.course_id, - ) - - -def apply_migrate_legacy_course_roles_to_authz(apps, schema_editor): - """ - Wrapper to run the migration using the historical version of the CourseAccessRole model. - """ - # CourseAccessRole model from the student app, here is where the legacy course roles are stored - try: - CourseAccessRole = apps.get_model("student", "CourseAccessRole") - except LookupError: - # Don't run the migration where the student app is not installed, like during development. - logger.warning("CourseAccessRole model not found. Skipping migration.") - return - - permissions_with_errors = migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration=True) - - if permissions_with_errors: - _log_migration_errors(permissions_with_errors) - - -class Migration(migrations.Migration): - dependencies = [ - ("openedx_authz", "0007_coursescope"), - ] - - operations = [ - migrations.RunPython(apply_migrate_legacy_course_roles_to_authz), - ] diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 7a7b79f6..5d6d7685 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -590,6 +590,99 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(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_and_rollback_with_no_new_role_equivalent(self): + """Test the migration of legacy course roles to the new Casbin-based model + and the rollback when there is no equivalent new role. + """ + + # Migrate from legacy CourseAccessRole to new Casbin-based model + permissions_with_errors = migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration=True) + AuthzEnforcer.get_enforcer().load_policy() + + # Now let's rollback + + # Capture the state of permissions before rollback to verify that rollback restores the original state + original_state_user_subjects = list( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .distinct() + .order_by("id") + .values("id", "user_id") + ) + original_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + # Mock the role_to_legacy_role mapping to only include a mapping + # for COURSE_ADMIN to simulate the scenario where the staff, limited_staff + # and data_researcher roles do not have a legacy role equivalent and + # therefore cannot be migrated back to legacy roles during the rollback. + with patch( + "openedx_authz.engine.utils.role_to_legacy_role", + {COURSE_ADMIN.external_key: "instructor"}, + ): + permissions_with_errors = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, delete_after_migration=True + ) + + # Check that each user has the expected legacy role after rollback + # and that errors are logged for any permissions that could not be rolled back + 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) + for user in self.staff_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + # Since we are mocking the role_to_legacy_role mapping to only include a mapping for COURSE_ADMIN, + # the staff role will not have a legacy role equivalent and therefore should not be migrated back + self.assertEqual(len(assignments), 1) + for user in self.limited_staff: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + # Since we are mocking the role_to_legacy_role mapping to only include a mapping for COURSE_ADMIN, + # the limited_staff role will not have a legacy role equivalent and therefore should not be migrated back + self.assertEqual(len(assignments), 1) + for user in self.data_researcher: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + # Since we are mocking the role_to_legacy_role mapping to only include a mapping for COURSE_ADMIN, + # the data_researcher role will not have a legacy role equivalent and therefore should not be migrated back + self.assertEqual(len(assignments), 1) + + # 3 staff + 3 limited_staff + 3 data_researcher = 9 entries with no legacy role equivalent + self.assertEqual(len(permissions_with_errors), 9) + + state_after_migration_user_subjects = list( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .distinct() + .order_by("id") + .values("id", "user_id") + ) + after_migrate_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + # Before the rollback, we should only have the 1 invalid role entry + # since we set delete_after_migration to True in the migration. + self.assertEqual(len(original_state_access_roles), 1) + + # All original entries (1) + 3 users * 1 roles = 4 + self.assertEqual(len(after_migrate_state_access_roles), 1 + 3) + + # Before the rollback, we should have the 12 UserSubjects related to the course permissions + # since we had 3 users with 4 roles each in the original state. + self.assertEqual(len(original_state_user_subjects), 12) + + # After rollback, we should have 9 UserSubjects related to the course permissions + # since the users with staff, limited_staff and data_researcher roles will not be + # migrated back to legacy roles due to our mocked role_to_legacy_role mapping. + self.assertEqual(len(state_after_migration_user_subjects), 9) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") def test_authz_migrate_course_authoring_command(self, mock_migrate): From ac7acef227d12f87690e9da347dbe1cd6d04ff47 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Thu, 19 Feb 2026 09:49:33 -0600 Subject: [PATCH 03/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/engine/utils.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 89ccb438..e62d6c27 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -273,10 +273,6 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_a course_overview = assignment.scope.get_object() for role in assignment.roles: - # We are only interested in course-related scopes and roles - if not scope.startswith("course-v1:"): - continue - legacy_role = role_to_legacy_role.get(role.external_key) if legacy_role is None: logger.error(f"Unknown role: {role} for User: {user_external_key}") From e627dd78ac3fabc717412af0d7f88893af21174f Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Thu, 19 Feb 2026 12:15:54 -0600 Subject: [PATCH 04/18] fixup! feat: add course authoring migration and rollback scripts --- CHANGELOG.rst | 12 ++++++++++++ openedx_authz/__init__.py | 2 +- openedx_authz/constants/roles.py | 5 ----- openedx_authz/engine/utils.py | 12 ++++-------- openedx_authz/tests/test_migrations.py | 23 +++++++++++++++++------ 5 files changed, 34 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0804b100..51d6cc57 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,18 @@ Change Log Unreleased ********** +0.23.0 - 2026-02-18 +******************** + +Added +===== + +* Add authz_migrate_course_authoring command to migrate legacy CourseAccessRole data to the new Authz (Casbin-based) system +* Add authz_rollback_course_authoring command to rollback Authz roles back to legacy CourseAccessRole +* Support optional --delete flag for controlled cleanup of source permissions after successful migration +* Add migrate_legacy_course_roles_to_authz and migrate_authz_to_legacy_course_roles service functions +* Add unit tests to verify migration and command behavior + Added ===== diff --git a/openedx_authz/__init__.py b/openedx_authz/__init__.py index de3fced5..86dbaad4 100644 --- a/openedx_authz/__init__.py +++ b/openedx_authz/__init__.py @@ -4,6 +4,6 @@ import os -__version__ = "0.22.0" +__version__ = "0.23.0" ROOT_DIRECTORY = os.path.dirname(os.path.abspath(__file__)) diff --git a/openedx_authz/constants/roles.py b/openedx_authz/constants/roles.py index e7c81212..4ac6f941 100644 --- a/openedx_authz/constants/roles.py +++ b/openedx_authz/constants/roles.py @@ -160,11 +160,6 @@ permissions.COURSES_EXPORT_TAGS, ] -COURSE_LIMITED_STAFF_PERMISSIONS = [] - -COURSE_DATA_RESEARCHER_PERMISSIONS = [] - -COURSE_ADMIN = RoleData(external_key="course_admin", permissions=COURSE_ADMIN_PERMISSIONS) COURSE_STAFF = RoleData(external_key="course_staff", permissions=COURSE_STAFF_PERMISSIONS) COURSE_LIMITED_STAFF_PERMISSIONS = [ diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index e62d6c27..d99cc854 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -19,6 +19,7 @@ COURSE_DATA_RESEARCHER, COURSE_LIMITED_STAFF, COURSE_STAFF, + LEGACY_COURSE_ROLE_EQUIVALENCES, LIBRARY_ADMIN, LIBRARY_AUTHOR, LIBRARY_USER, @@ -29,13 +30,8 @@ GROUPING_POLICY_PTYPES = ["g", "g2", "g3", "g4", "g5", "g6"] -# Map new roles back to legacy roles -role_to_legacy_role = { - COURSE_ADMIN.external_key: "instructor", - COURSE_STAFF.external_key: "staff", - COURSE_LIMITED_STAFF.external_key: "limited_staff", - COURSE_DATA_RESEARCHER.external_key: "data_researcher", -} +# Map new roles back to legacy roles for rollback purposes +COURSE_ROLE_EQUIVALENCES = {v: k for k, v in LEGACY_COURSE_ROLE_EQUIVALENCES.items()} def migrate_policy_between_enforcers( @@ -273,7 +269,7 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_a course_overview = assignment.scope.get_object() for role in assignment.roles: - legacy_role = role_to_legacy_role.get(role.external_key) + legacy_role = COURSE_ROLE_EQUIVALENCES.get(role.external_key) if legacy_role is None: logger.error(f"Unknown role: {role} for User: {user_external_key}") roles_with_errors.append((user_external_key, role.external_key, scope)) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 5d6d7685..2a5623c1 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -13,6 +13,7 @@ COURSE_DATA_RESEARCHER, COURSE_LIMITED_STAFF, COURSE_STAFF, + LEGACY_COURSE_ROLE_EQUIVALENCES, LIBRARY_ADMIN, LIBRARY_USER, ) @@ -305,6 +306,16 @@ def tearDown(self): scope_external_key=self.course_id, ) + def test_legacy_course_role_equivalences_mapping(self): + """Test that the LEGACY_COURSE_ROLE_EQUIVALENCES mapping contains no duplicate values.""" + legacy_roles = LEGACY_COURSE_ROLE_EQUIVALENCES.keys() + new_roles = LEGACY_COURSE_ROLE_EQUIVALENCES.values() + + # Check that there are no duplicate values in the mapping + self.assertEqual( + len(legacy_roles), len(set(new_roles)), "LEGACY_COURSE_ROLE_EQUIVALENCES contains duplicate values" + ) + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): """Test the migration of legacy permissions from CourseAccessRole to the new Casbin-based model @@ -613,12 +624,12 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") ) - # Mock the role_to_legacy_role mapping to only include a mapping + # Mock the COURSE_ROLE_EQUIVALENCES mapping to only include a mapping # for COURSE_ADMIN to simulate the scenario where the staff, limited_staff # and data_researcher roles do not have a legacy role equivalent and # therefore cannot be migrated back to legacy roles during the rollback. with patch( - "openedx_authz.engine.utils.role_to_legacy_role", + "openedx_authz.engine.utils.COURSE_ROLE_EQUIVALENCES", {COURSE_ADMIN.external_key: "instructor"}, ): permissions_with_errors = migrate_authz_to_legacy_course_roles( @@ -636,21 +647,21 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi assignments = get_user_role_assignments_in_scope( user_external_key=user.username, scope_external_key=self.course_id ) - # Since we are mocking the role_to_legacy_role mapping to only include a mapping for COURSE_ADMIN, + # Since we are mocking the COURSE_ROLE_EQUIVALENCES mapping to only include a mapping for COURSE_ADMIN, # the staff role will not have a legacy role equivalent and therefore should not be migrated back self.assertEqual(len(assignments), 1) for user in self.limited_staff: assignments = get_user_role_assignments_in_scope( user_external_key=user.username, scope_external_key=self.course_id ) - # Since we are mocking the role_to_legacy_role mapping to only include a mapping for COURSE_ADMIN, + # Since we are mocking the COURSE_ROLE_EQUIVALENCES mapping to only include a mapping for COURSE_ADMIN, # the limited_staff role will not have a legacy role equivalent and therefore should not be migrated back self.assertEqual(len(assignments), 1) for user in self.data_researcher: assignments = get_user_role_assignments_in_scope( user_external_key=user.username, scope_external_key=self.course_id ) - # Since we are mocking the role_to_legacy_role mapping to only include a mapping for COURSE_ADMIN, + # Since we are mocking the COURSE_ROLE_EQUIVALENCES mapping to only include a mapping for COURSE_ADMIN, # the data_researcher role will not have a legacy role equivalent and therefore should not be migrated back self.assertEqual(len(assignments), 1) @@ -680,7 +691,7 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi # After rollback, we should have 9 UserSubjects related to the course permissions # since the users with staff, limited_staff and data_researcher roles will not be - # migrated back to legacy roles due to our mocked role_to_legacy_role mapping. + # migrated back to legacy roles due to our mocked COURSE_ROLE_EQUIVALENCES mapping. self.assertEqual(len(state_after_migration_user_subjects), 9) @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) From ec9785f261cc25e835d97aa308d7d1a6383c7c29 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Fri, 20 Feb 2026 11:44:23 -0600 Subject: [PATCH 05/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/admin.py | 17 +++++++++++ openedx_authz/engine/utils.py | 20 +++++++++++-- .../authz_migrate_course_authoring.py | 23 ++++++--------- .../authz_rollback_course_authoring.py | 28 +++++++------------ openedx_authz/models/core.py | 9 ++++++ openedx_authz/tests/test_migrations.py | 12 +++----- 6 files changed, 67 insertions(+), 42 deletions(-) diff --git a/openedx_authz/admin.py b/openedx_authz/admin.py index e2285158..52470867 100644 --- a/openedx_authz/admin.py +++ b/openedx_authz/admin.py @@ -5,6 +5,8 @@ from django.contrib import admin from openedx_authz.models import ExtendedCasbinRule +from openedx_authz.models.scopes import CourseScope +from openedx_authz.models.subjects import UserSubject class CasbinRuleForm(forms.ModelForm): @@ -48,3 +50,18 @@ 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(ExtendedCasbinRule) +class ExtendedCasbinRuleAdmin(admin.ModelAdmin): + pass + + +@admin.register(UserSubject) +class UserSubjectAdmin(admin.ModelAdmin): + pass + + +@admin.register(CourseScope) +class CourseScopeAdmin(admin.ModelAdmin): + list_display = ("id", "course_overview") diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index d99cc854..133a9859 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -8,6 +8,7 @@ from casbin import Enforcer +from openedx_authz.api.data import CourseOverviewData from openedx_authz.api.users import ( assign_role_to_user_in_scope, batch_assign_role_to_users_in_scope, @@ -193,7 +194,9 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migratio param CourseAccessRole: The CourseAccessRole model to use. """ - legacy_permissions = CourseAccessRole.objects.select_related("user").all() + legacy_permissions = ( + CourseAccessRole.objects.filter(course_id__startswith="course-v1:").select_related("user").all() + ) # List to keep track of any permissions that could not be migrated permissions_with_errors = [] @@ -224,11 +227,20 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migratio f"to Role: {role.external_key} in Scope: {permission.course_id}" ) - assign_role_to_user_in_scope( + is_user_added = assign_role_to_user_in_scope( user_external_key=permission.user.username, role_external_key=role.external_key, scope_external_key=str(permission.course_id), ) + + if not is_user_added: + logger.error( + f"Failed to migrate permission for User: {permission.user.username} " + f"to Role: {role.external_key} in Scope: {permission.course_id}" + ) + permissions_with_errors.append(permission) + continue + permissions_with_no_errors.append(permission) if delete_after_migration: @@ -264,6 +276,10 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_a role_assignments = get_user_role_assignments(user_external_key=user_external_key) for assignment in role_assignments: + if not isinstance(assignment.scope, CourseOverviewData): + logger.error(f"Skipping role assignment for User: {user_external_key} due to missing course scope.") + continue + scope = assignment.scope.external_key course_overview = assignment.scope.get_object() diff --git a/openedx_authz/management/commands/authz_migrate_course_authoring.py b/openedx_authz/management/commands/authz_migrate_course_authoring.py index 4d95fa4f..1deed9b4 100644 --- a/openedx_authz/management/commands/authz_migrate_course_authoring.py +++ b/openedx_authz/management/commands/authz_migrate_course_authoring.py @@ -34,10 +34,18 @@ def handle(self, *args, **options): self.stdout.write(self.style.WARNING("Starting legacy → Authz migration...")) try: + if delete_after_migration: + confirm = input( + "Are you sure you want to delete successfully migrated legacy roles? Type 'yes' to continue: " + ) + + if confirm != "yes": + self.stdout.write(self.style.WARNING("Deletion aborted.")) + return with transaction.atomic(): errors = migrate_legacy_course_roles_to_authz( CourseAccessRole=CourseAccessRole, - delete_after_migration=False, # control deletion here instead + delete_after_migration=delete_after_migration, ) if errors: @@ -45,20 +53,7 @@ def handle(self, *args, **options): else: self.stdout.write(self.style.SUCCESS("Migration completed successfully with no errors.")) - # Handle deletion separately for safety if delete_after_migration: - confirm = input( - "Are you sure you want to delete successfully migrated legacy roles? Type 'yes' to continue: " - ) - - if confirm != "yes": - self.stdout.write(self.style.WARNING("Deletion aborted.")) - return - - migrated_ids = [p.id for p in CourseAccessRole.objects.all() if p not in errors] - - CourseAccessRole.objects.filter(id__in=migrated_ids).delete() - self.stdout.write(self.style.SUCCESS("Legacy roles deleted successfully.")) except Exception as exc: diff --git a/openedx_authz/management/commands/authz_rollback_course_authoring.py b/openedx_authz/management/commands/authz_rollback_course_authoring.py index d43a05dc..0b4334a8 100644 --- a/openedx_authz/management/commands/authz_rollback_course_authoring.py +++ b/openedx_authz/management/commands/authz_rollback_course_authoring.py @@ -36,11 +36,20 @@ def handle(self, *args, **options): self.stdout.write(self.style.WARNING("Starting Authz → Legacy rollback migration...")) try: + if delete_after_migration: + confirm = input( + "Are you sure you want to remove the new Authz role " + "assignments after rollback? Type 'yes' to continue: " + ) + + if confirm != "yes": + self.stdout.write(self.style.WARNING("Deletion aborted.")) + return with transaction.atomic(): errors = migrate_authz_to_legacy_course_roles( CourseAccessRole=CourseAccessRole, UserSubject=UserSubject, - delete_after_migration=False, # control deletion here + delete_after_migration=delete_after_migration, # control deletion here ) if errors: @@ -48,24 +57,7 @@ def handle(self, *args, **options): else: self.stdout.write(self.style.SUCCESS("Rollback completed successfully with no errors.")) - # Handle deletion separately for safety if delete_after_migration: - confirm = input( - "Are you sure you want to remove the new Authz role " - "assignments after rollback? Type 'yes' to continue: " - ) - - if confirm != "yes": - self.stdout.write(self.style.WARNING("Deletion aborted.")) - return - - # Re-run with deletion enabled - migrate_authz_to_legacy_course_roles( - CourseAccessRole=CourseAccessRole, - UserSubject=UserSubject, - delete_after_migration=True, - ) - self.stdout.write(self.style.SUCCESS("Authz role assignments removed successfully.")) except Exception as exc: diff --git a/openedx_authz/models/core.py b/openedx_authz/models/core.py index 326ecbc3..4089996a 100644 --- a/openedx_authz/models/core.py +++ b/openedx_authz/models/core.py @@ -114,6 +114,9 @@ class Scope(BaseRegistryModel): class Meta: abstract = False + def __str__(self): + return f"Scope (namespace={self.NAMESPACE})" + class Subject(BaseRegistryModel): """Model representing a subject in the authorization system. @@ -132,6 +135,9 @@ class Subject(BaseRegistryModel): class Meta: abstract = False + def __str__(self): + return f"Subject (namespace={self.NAMESPACE})" + class ExtendedCasbinRule(models.Model): """Extended model for Casbin rules to store additional metadata. @@ -182,6 +188,9 @@ class Meta: verbose_name = "Extended Casbin Rule" verbose_name_plural = "Extended Casbin Rules" + def __str__(self): + return f"ExtendedCasbinRule for {self.casbin_rule}" + @classmethod def create_based_on_policy( cls, diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 2a5623c1..0a9d2c55 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -721,7 +721,7 @@ def test_authz_migrate_course_authoring_command(self, mock_migrate): mock_migrate.assert_called_once() args, kwargs = mock_migrate.call_args - self.assertEqual(kwargs["delete_after_migration"], False) + self.assertEqual(kwargs["delete_after_migration"], True) @patch("openedx_authz.management.commands.authz_rollback_course_authoring.CourseAccessRole", CourseAccessRole) @patch("openedx_authz.management.commands.authz_rollback_course_authoring.migrate_authz_to_legacy_course_roles") @@ -747,12 +747,8 @@ def test_authz_rollback_course_authoring_command(self, mock_rollback): with patch("builtins.input", return_value="yes"): call_command("authz_rollback_course_authoring", "--delete") - # First call (normal) - # Second call (with deletion=True) - self.assertEqual(mock_rollback.call_count, 2) + self.assertEqual(mock_rollback.call_count, 1) - first_call_kwargs = mock_rollback.call_args_list[0][1] - second_call_kwargs = mock_rollback.call_args_list[1][1] + call_kwargs = mock_rollback.call_args_list[0][1] - self.assertEqual(first_call_kwargs["delete_after_migration"], False) - self.assertEqual(second_call_kwargs["delete_after_migration"], True) + self.assertEqual(call_kwargs["delete_after_migration"], True) From 1cda364cf11668e1ec3f7da89af74b66bbdb9ff2 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Fri, 20 Feb 2026 11:56:50 -0600 Subject: [PATCH 06/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/models/core.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/openedx_authz/models/core.py b/openedx_authz/models/core.py index 4089996a..326ecbc3 100644 --- a/openedx_authz/models/core.py +++ b/openedx_authz/models/core.py @@ -114,9 +114,6 @@ class Scope(BaseRegistryModel): class Meta: abstract = False - def __str__(self): - return f"Scope (namespace={self.NAMESPACE})" - class Subject(BaseRegistryModel): """Model representing a subject in the authorization system. @@ -135,9 +132,6 @@ class Subject(BaseRegistryModel): class Meta: abstract = False - def __str__(self): - return f"Subject (namespace={self.NAMESPACE})" - class ExtendedCasbinRule(models.Model): """Extended model for Casbin rules to store additional metadata. @@ -188,9 +182,6 @@ class Meta: verbose_name = "Extended Casbin Rule" verbose_name_plural = "Extended Casbin Rules" - def __str__(self): - return f"ExtendedCasbinRule for {self.casbin_rule}" - @classmethod def create_based_on_policy( cls, From c8f1c7371ec76a613dbd4e540b9d31a1adb89ae3 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Fri, 20 Feb 2026 15:52:12 -0600 Subject: [PATCH 07/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/engine/utils.py | 73 ++++++++++++------- .../authz_migrate_course_authoring.py | 32 +++++++- .../authz_rollback_course_authoring.py | 34 ++++++++- openedx_authz/tests/test_migrations.py | 47 ++++++++---- 4 files changed, 135 insertions(+), 51 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 133a9859..814f6a89 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -16,10 +16,6 @@ get_user_role_assignments, ) from openedx_authz.constants.roles import ( - COURSE_ADMIN, - COURSE_DATA_RESEARCHER, - COURSE_LIMITED_STAFF, - COURSE_STAFF, LEGACY_COURSE_ROLE_EQUIVALENCES, LIBRARY_ADMIN, LIBRARY_AUTHOR, @@ -172,7 +168,7 @@ def migrate_legacy_permissions(ContentLibraryPermission): return permissions_with_errors -def migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration): +def migrate_legacy_course_roles_to_authz(CourseAccessRole, course_id_list, org_id, delete_after_migration): """ Migrate legacy course role data to the new Casbin-based authorization model. This function reads legacy permissions from the CourseAccessRole model @@ -193,10 +189,23 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migratio param CourseAccessRole: The CourseAccessRole model to use. """ + if not course_id_list and not org_id: + raise ValueError( + "At least one of course_id_list or org_id must be provided to limit the scope of the rollback migration." + ) + course_access_role_filter = { + "course_id__startswith": "course-v1:", + } + + if org_id: + course_access_role_filter["org"] = org_id + + if course_id_list and not org_id: + # Only filter by course_id if org_id is not provided, + # otherwise we will filter by org_id which is more efficient + course_access_role_filter["course_id__in"] = course_id_list - legacy_permissions = ( - CourseAccessRole.objects.filter(course_id__startswith="course-v1:").select_related("user").all() - ) + legacy_permissions = CourseAccessRole.objects.filter(**course_access_role_filter).select_related("user").all() # List to keep track of any permissions that could not be migrated permissions_with_errors = [] @@ -205,15 +214,7 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migratio for permission in legacy_permissions: # Migrate the permission to the new model - # Derive equivalent role based on access level - map_legacy_role = { - "instructor": COURSE_ADMIN, - "staff": COURSE_STAFF, - "limited_staff": COURSE_LIMITED_STAFF, - "data_researcher": COURSE_DATA_RESEARCHER, - } - - role = map_legacy_role.get(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 @@ -224,19 +225,19 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migratio # Permission applied to individual user logger.info( f"Migrating permission for User: {permission.user.username} " - f"to Role: {role.external_key} in Scope: {permission.course_id}" + f"to Role: {role} in Scope: {permission.course_id}" ) is_user_added = assign_role_to_user_in_scope( user_external_key=permission.user.username, - role_external_key=role.external_key, + role_external_key=role, scope_external_key=str(permission.course_id), ) if not is_user_added: logger.error( f"Failed to migrate permission for User: {permission.user.username} " - f"to Role: {role.external_key} in Scope: {permission.course_id}" + f"to Role: {role} in Scope: {permission.course_id}" ) permissions_with_errors.append(permission) continue @@ -244,12 +245,13 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migratio permissions_with_no_errors.append(permission) if delete_after_migration: + # Only delete permissions that were successfully migrated to avoid data loss. CourseAccessRole.objects.filter(id__in=[p.id for p in permissions_with_no_errors]).delete() - return permissions_with_errors + return permissions_with_errors, permissions_with_no_errors -def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_after_migration): +def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, course_id_list, org_id, delete_after_migration): """ 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 @@ -258,15 +260,29 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_a This is essentially the reverse of migrate_legacy_course_roles_to_authz and is intended for rollback purposes in case of migration issues. """ + if not course_id_list and not org_id: + raise ValueError( + "At least one of course_id_list or org_id must be provided to limit the scope of the rollback migration." + ) + # 1. Get all users with course-related permissions in the new model by filtering # UserSubjects that are linked to CourseScopes with a valid course overview. - course_subjects = ( - UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) - .select_related("user") - .distinct() - ) + course_subject_filter = { + "casbin_rules__scope__coursescope__course_overview__isnull": False, + } + + if org_id: + course_subject_filter["casbin_rules__scope__coursescope__course_overview__org"] = org_id + + if course_id_list and not org_id: + # Only filter by course_id if org_id is not provided, + # otherwise we will filter by org_id which is more efficient + course_subject_filter["casbin_rules__scope__coursescope__course_overview__id__in"] = course_id_list + + course_subjects = UserSubject.objects.filter(**course_subject_filter).select_related("user").distinct() roles_with_errors = [] + roles_with_no_errors = [] for course_subject in course_subjects: user = course_subject.user @@ -299,6 +315,7 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_a course_id=scope, role=legacy_role, ) + roles_with_no_errors.append((user_external_key, role.external_key, scope)) except Exception as e: # pylint: disable=broad-exception-caught logger.error( f"Error creating CourseAccessRole for User: " @@ -314,4 +331,4 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, delete_a role_external_key=role.external_key, scope_external_key=scope, ) - return roles_with_errors + return roles_with_errors, roles_with_no_errors diff --git a/openedx_authz/management/commands/authz_migrate_course_authoring.py b/openedx_authz/management/commands/authz_migrate_course_authoring.py index 1deed9b4..2437770f 100644 --- a/openedx_authz/management/commands/authz_migrate_course_authoring.py +++ b/openedx_authz/management/commands/authz_migrate_course_authoring.py @@ -2,7 +2,7 @@ Django management command to migrate legacy course authoring roles to the new Authz (Casbin-based) authorization system. """ -from django.core.management.base import BaseCommand +from django.core.management.base import BaseCommand, CommandError from django.db import transaction from openedx_authz.engine.utils import migrate_legacy_course_roles_to_authz @@ -27,9 +27,29 @@ def add_arguments(self, parser): action="store_true", help="Delete legacy CourseAccessRole records after successful migration.", ) + parser.add_argument( + "--course-id-list", + nargs="*", + type=str, + help="Optional list of course IDs to filter the migration.", + ) + + parser.add_argument( + "--org-id", + type=str, + help="Optional organization ID to filter the migration.", + ) def handle(self, *args, **options): delete_after_migration = options["delete"] + course_id_list = options.get("course_id_list") + org_id = options.get("org_id") + + if not course_id_list and not org_id: + raise CommandError("You must specify either --course-id-list or --org-id to filter the rollback.") + + if course_id_list and org_id: + raise CommandError("You cannot use --course-id-list and --org-id together.") self.stdout.write(self.style.WARNING("Starting legacy → Authz migration...")) @@ -43,18 +63,22 @@ def handle(self, *args, **options): self.stdout.write(self.style.WARNING("Deletion aborted.")) return with transaction.atomic(): - errors = migrate_legacy_course_roles_to_authz( + errors, success = migrate_legacy_course_roles_to_authz( CourseAccessRole=CourseAccessRole, + course_id_list=course_id_list, + org_id=org_id, delete_after_migration=delete_after_migration, ) if errors: self.stdout.write(self.style.ERROR(f"Migration completed with {len(errors)} errors.")) else: - self.stdout.write(self.style.SUCCESS("Migration completed successfully with no errors.")) + self.stdout.write( + self.style.SUCCESS(f"Migration completed successfully with {len(success)} roles migrated.") + ) if delete_after_migration: - self.stdout.write(self.style.SUCCESS("Legacy roles deleted successfully.")) + self.stdout.write(self.style.SUCCESS(f"{len(success)} Legacy roles deleted successfully.")) except Exception as exc: self.stdout.write(self.style.ERROR(f"Migration failed due to unexpected error: {exc}")) diff --git a/openedx_authz/management/commands/authz_rollback_course_authoring.py b/openedx_authz/management/commands/authz_rollback_course_authoring.py index 0b4334a8..bab89d58 100644 --- a/openedx_authz/management/commands/authz_rollback_course_authoring.py +++ b/openedx_authz/management/commands/authz_rollback_course_authoring.py @@ -3,7 +3,7 @@ authorization system back to the legacy CourseAccessRole model. """ -from django.core.management.base import BaseCommand +from django.core.management.base import BaseCommand, CommandError from django.db import transaction from openedx_authz.engine.utils import migrate_authz_to_legacy_course_roles @@ -29,9 +29,29 @@ def add_arguments(self, parser): action="store_true", help="Delete Authz role assignments after successful rollback.", ) + parser.add_argument( + "--course-id-list", + nargs="*", + type=str, + help="Optional list of course IDs to filter the migration.", + ) + + parser.add_argument( + "--org-id", + type=str, + help="Optional organization ID to filter the migration.", + ) def handle(self, *args, **options): delete_after_migration = options["delete"] + course_id_list = options.get("course_id_list") + org_id = options.get("org_id") + + if not course_id_list and not org_id: + raise CommandError("You must specify either --course-id-list or --org-id to filter the rollback.") + + if course_id_list and org_id: + raise CommandError("You cannot use --course-id-list and --org-id together.") self.stdout.write(self.style.WARNING("Starting Authz → Legacy rollback migration...")) @@ -46,19 +66,25 @@ def handle(self, *args, **options): self.stdout.write(self.style.WARNING("Deletion aborted.")) return with transaction.atomic(): - errors = migrate_authz_to_legacy_course_roles( + errors, success = migrate_authz_to_legacy_course_roles( CourseAccessRole=CourseAccessRole, UserSubject=UserSubject, + course_id_list=course_id_list, + org_id=org_id, delete_after_migration=delete_after_migration, # control deletion here ) if errors: self.stdout.write(self.style.ERROR(f"Rollback completed with {len(errors)} errors.")) else: - self.stdout.write(self.style.SUCCESS("Rollback completed successfully with no errors.")) + self.stdout.write( + self.style.SUCCESS(f"Rollback completed successfully with {len(success)} roles rolled back.") + ) if delete_after_migration: - self.stdout.write(self.style.SUCCESS("Authz role assignments removed successfully.")) + self.stdout.write( + self.style.SUCCESS(f"{len(success)} Authz role assignments removed successfully.") + ) except Exception as exc: self.stdout.write(self.style.ERROR(f"Rollback failed due to unexpected error: {exc}")) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 0a9d2c55..1f26cbcc 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -348,7 +348,10 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): ) # 3 users for each of the 4 roles + 1 invalid role = 13 total entries # Migrate from legacy CourseAccessRole to new Casbin-based model - permissions_with_errors = migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration=False) + course_id_list = [self.course_id] + permissions_with_errors, permissions_with_no_errors = migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=course_id_list, org_id=None, delete_after_migration=False + ) AuthzEnforcer.get_enforcer().load_policy() for user in self.admin_users: assignments = get_user_role_assignments_in_scope( @@ -374,10 +377,13 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_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].role, "invalid-legacy-role") + self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries + after_migrate_state_access_roles = list( CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") ) @@ -400,8 +406,8 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") ) - permissions_with_errors = migrate_authz_to_legacy_course_roles( - CourseAccessRole, UserSubject, delete_after_migration=False + permissions_with_errors, permissions_with_no_errors = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, course_id_list=course_id_list, org_id=None, delete_after_migration=False ) # Check that each user has the expected legacy role after rollback and that errors @@ -430,7 +436,9 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): ) self.assertEqual(len(assignments), 1) self.assertEqual(assignments[0].roles[0], COURSE_DATA_RESEARCHER) + self.assertEqual(len(permissions_with_errors), 0) + self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries state_after_migration_user_subjects = list( UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) @@ -491,8 +499,11 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): len(original_state_access_roles), 13 ) # 3 users for each of the 4 roles + 1 invalid role = 13 total entries + course_id_list = [self.course_id] # Migrate from legacy CourseAccessRole to new Casbin-based model - permissions_with_errors = migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration=True) + permissions_with_errors, permissions_with_no_errors = migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=course_id_list, org_id=None, delete_after_migration=True + ) AuthzEnforcer.get_enforcer().load_policy() for user in self.admin_users: assignments = get_user_role_assignments_in_scope( @@ -521,6 +532,7 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): 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].role, "invalid-legacy-role") + self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries after_migrate_state_access_roles = list( CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") @@ -548,8 +560,8 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") ) - permissions_with_errors = migrate_authz_to_legacy_course_roles( - CourseAccessRole, UserSubject, delete_after_migration=True + permissions_with_errors, permissions_with_no_errors = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, course_id_list=course_id_list, org_id=None, delete_after_migration=True ) # Check that each user has the expected legacy role after rollback @@ -574,7 +586,9 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): user_external_key=user.username, scope_external_key=self.course_id ) self.assertEqual(len(assignments), 0) + self.assertEqual(len(permissions_with_errors), 0) + self.assertEqual(len(permissions_with_no_errors), 12) state_after_migration_user_subjects = list( UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) @@ -608,7 +622,10 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi """ # Migrate from legacy CourseAccessRole to new Casbin-based model - permissions_with_errors = migrate_legacy_course_roles_to_authz(CourseAccessRole, delete_after_migration=True) + course_id_list = [self.course_id] + permissions_with_errors, _ = migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=course_id_list, org_id=None, delete_after_migration=True + ) AuthzEnforcer.get_enforcer().load_policy() # Now let's rollback @@ -632,8 +649,8 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi "openedx_authz.engine.utils.COURSE_ROLE_EQUIVALENCES", {COURSE_ADMIN.external_key: "instructor"}, ): - permissions_with_errors = migrate_authz_to_legacy_course_roles( - CourseAccessRole, UserSubject, delete_after_migration=True + permissions_with_errors, _ = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, course_id_list=course_id_list, org_id=None, delete_after_migration=True ) # Check that each user has the expected legacy role after rollback @@ -702,10 +719,10 @@ def test_authz_migrate_course_authoring_command(self, mock_migrate): calls migrate_legacy_course_roles_to_authz with the correct arguments. """ - mock_migrate.return_value = [] + mock_migrate.return_value = [], [] # Return empty lists for permissions with and without errors # Run without --delete - call_command("authz_migrate_course_authoring") + call_command("authz_migrate_course_authoring", "--course-id-list", self.course_id) mock_migrate.assert_called_once() args, kwargs = mock_migrate.call_args @@ -716,7 +733,7 @@ def test_authz_migrate_course_authoring_command(self, mock_migrate): # Run with --delete with patch("builtins.input", return_value="yes"): - call_command("authz_migrate_course_authoring", "--delete") + call_command("authz_migrate_course_authoring", "--delete", "--course-id-list", self.course_id) mock_migrate.assert_called_once() args, kwargs = mock_migrate.call_args @@ -731,10 +748,10 @@ def test_authz_rollback_course_authoring_command(self, mock_rollback): calls migrate_authz_to_legacy_course_roles correctly. """ - mock_rollback.return_value = [] + mock_rollback.return_value = [], [] # Return empty lists for permissions with and without errors # Run without --delete - call_command("authz_rollback_course_authoring") + call_command("authz_rollback_course_authoring", "--course-id-list", self.course_id) mock_rollback.assert_called_once() args, kwargs = mock_rollback.call_args @@ -745,7 +762,7 @@ def test_authz_rollback_course_authoring_command(self, mock_rollback): # Run with --delete with patch("builtins.input", return_value="yes"): - call_command("authz_rollback_course_authoring", "--delete") + call_command("authz_rollback_course_authoring", "--delete", "--course-id-list", self.course_id) self.assertEqual(mock_rollback.call_count, 1) From 37149a85a4c15d9a70e99438020c6e9f580f0d58 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 25 Feb 2026 16:29:17 -0700 Subject: [PATCH 08/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/tests/test_migrations.py | 69 ++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 1f26cbcc..0b7c469e 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -711,6 +711,75 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi # migrated back to legacy roles due to our mocked COURSE_ROLE_EQUIVALENCES mapping. self.assertEqual(len(state_after_migration_user_subjects), 9) + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_legacy_course_roles_to_authz_using_org_id(self): + """Test the migration of legacy course roles to the new Casbin-based model + and the rollback when there is no equivalent new role. + """ + + # Migrate from legacy CourseAccessRole to new Casbin-based model + permissions_with_errors, permissions_with_no_errors = migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=None, org_id=self.org, delete_after_migration=True + ) + AuthzEnforcer.get_enforcer().load_policy() + 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), 1) + self.assertEqual(assignments[0].roles[0], COURSE_ADMIN) + for user in self.staff_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_STAFF) + for user in self.limited_staff: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_LIMITED_STAFF) + for user in self.data_researcher: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + 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].role, "invalid-legacy-role") + self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries + + after_migrate_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + # self.assertEqual(len(original_state_access_roles), 13) + + # Only the invalid role entry should remain since we set delete_after_migration to True + self.assertEqual(len(after_migrate_state_access_roles), 1) + + # Must be different before and after migration since we set delete_after_migration + # to True and we are deleting all + + @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 + + with self.assertRaises(ValueError): + migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, course_id_list=None, org_id=None, delete_after_migration=True + ) + + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_legacy_course_roles_to_authz_with_no_org_and_courses(self): + # Migrate from legacy CourseAccessRole to new Casbin-based model + with self.assertRaises(ValueError): + migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=None, org_id=None, delete_after_migration=True + ) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") def test_authz_migrate_course_authoring_command(self, mock_migrate): From 1753f8ed2e26fa3daa9328b57c09c7a5184cb549 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Thu, 26 Feb 2026 09:16:36 -0700 Subject: [PATCH 09/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/tests/test_migrations.py | 91 +++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 0b7c469e..4f1a8ffe 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -4,7 +4,7 @@ from django.contrib.auth import get_user_model from django.contrib.auth.models import Group -from django.core.management import call_command +from django.core.management import CommandError, call_command from django.test import TestCase from openedx_authz.api.users import batch_unassign_role_from_users, get_user_role_assignments_in_scope @@ -762,6 +762,73 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): # Must be different before and after migration since we set delete_after_migration # to True and we are deleting all + # Now let's rollback + + # Capture the state of permissions before rollback to verify that rollback restores the original state + original_state_user_subjects = list( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .distinct() + .order_by("id") + .values("id", "user_id") + ) + original_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + permissions_with_errors, permissions_with_no_errors = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, course_id_list=None, org_id=self.org, delete_after_migration=True + ) + + # Check that each user has the expected legacy role after rollback + # and that errors are logged for any permissions that could not be rolled back + 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) + for user in self.staff_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 0) + for user in self.limited_staff: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 0) + for user in self.data_researcher: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 0) + + self.assertEqual(len(permissions_with_errors), 0) + self.assertEqual(len(permissions_with_no_errors), 12) + + state_after_migration_user_subjects = list( + UserSubject.objects.filter(casbin_rules__scope__coursescope__course_overview__isnull=False) + .distinct() + .order_by("id") + .values("id", "user_id") + ) + after_migrate_state_access_roles = list( + CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") + ) + + # Before the rollback, we should only have the 1 invalid role entry + # since we set delete_after_migration to True in the migration. + self.assertEqual(len(original_state_access_roles), 1) + + # All original entries + 3 users * 4 roles = 12 + # plus the original invalid entry = 1 + 12 = 13 total entries + self.assertEqual(len(after_migrate_state_access_roles), 1 + 12) + + # Sanity check to ensure we have the expected number of UserSubjects related to + # the course permissions before migration (3 users * 4 roles = 12) + self.assertEqual(len(original_state_user_subjects), 12) + + # 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_authz_to_legacy_course_roles_with_no_org_and_courses(self): @@ -838,3 +905,25 @@ def test_authz_rollback_course_authoring_command(self, mock_rollback): call_kwargs = mock_rollback.call_args_list[0][1] self.assertEqual(call_kwargs["delete_after_migration"], True) + + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") + def test_authz_migrate_course_authoring_command_no_org_and_courses(self): + """ + Verify that the authz_migrate_course_authoring command raises an error + when neither course_id_list nor org_id is provided. + """ + + with self.assertRaises(CommandError): + call_command("authz_migrate_course_authoring") + + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") + def test_authz_migrate_course_authoring_command_with_org_and_courses(self): + """ + Verify that the authz_migrate_course_authoring command raises an error + when both course_id_list and org_id are provided. + """ + + with self.assertRaises(CommandError): + call_command("authz_migrate_course_authoring", "--course-id-list", self.course_id, "--org-id", self.org) From 0a21eb5bf21728e968f73af1b6721dece0205d78 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Thu, 26 Feb 2026 10:13:05 -0700 Subject: [PATCH 10/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/tests/test_migrations.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 4f1a8ffe..fbc91ff5 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -907,7 +907,6 @@ def test_authz_rollback_course_authoring_command(self, mock_rollback): self.assertEqual(call_kwargs["delete_after_migration"], True) @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) - @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") def test_authz_migrate_course_authoring_command_no_org_and_courses(self): """ Verify that the authz_migrate_course_authoring command raises an error @@ -918,7 +917,6 @@ def test_authz_migrate_course_authoring_command_no_org_and_courses(self): call_command("authz_migrate_course_authoring") @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) - @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") def test_authz_migrate_course_authoring_command_with_org_and_courses(self): """ Verify that the authz_migrate_course_authoring command raises an error From 82aed9e564c0f81f951e291566d1c774feb70189 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Mon, 2 Mar 2026 11:04:41 -0600 Subject: [PATCH 11/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/tests/test_migrations.py | 68 ++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index fbc91ff5..17a305f6 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -2,6 +2,7 @@ from unittest.mock import patch +import pytest from django.contrib.auth import get_user_model from django.contrib.auth.models import Group from django.core.management import CommandError, call_command @@ -925,3 +926,70 @@ def test_authz_migrate_course_authoring_command_with_org_and_courses(self): with self.assertRaises(CommandError): call_command("authz_migrate_course_authoring", "--course-id-list", self.course_id, "--org-id", self.org) + + +@pytest.fixture +def mock_course_access_role(): + """Fixture to mock the CourseAccessRole model and its queryset + for testing the migration functions without relying on the actual database model.""" + + class MockPermission: + """A simple class to represent a permission with user, role, course_id and id attributes.""" + + def __init__(self, user, role, course_id, id_in): + self.user = user + self.role = role + self.course_id = course_id + self.id = id_in + + class MockUser: + """A simple class to represent a user with a username attribute.""" + + def __init__(self, username): + self.username = username + + class MockQuerySet: + """A simple class to represent a queryset of permissions.""" + + def __init__(self, permissions): + self.permissions = permissions + + def filter(self, **kwargs): + return self + + def select_related(self, *args, **kwargs): + return self + + def all(self): + return self.permissions + + class MockCourseAccessRole: + """A simple class to represent the CourseAccessRole model.""" + + objects = MockQuerySet( + [ + MockPermission(MockUser("testuser"), "instructor", "course-v1:test", 1), + ] + ) + + def __init__(self): + pass + + return MockCourseAccessRole + + +# pylint: disable=redefined-outer-name +def test_migrate_legacy_course_roles_to_authz_user_not_added(monkeypatch, mock_course_access_role): + # Patch assign_role_to_user_in_scope to always return False + monkeypatch.setattr( + "openedx_authz.engine.utils.assign_role_to_user_in_scope", + lambda user_external_key, role_external_key, scope_external_key: False, + ) + # Patch LEGACY_COURSE_ROLE_EQUIVALENCES + monkeypatch.setattr("openedx_authz.engine.utils.LEGACY_COURSE_ROLE_EQUIVALENCES", {"instructor": "instructor-role"}) + errors, successes = migrate_legacy_course_roles_to_authz( + mock_course_access_role, course_id_list=["course-v1:test"], org_id=None, delete_after_migration=False + ) + assert len(errors) == 1 + assert len(successes) == 0 + assert errors[0].user.username == "testuser" From f2900798347133bf7ff6dcb0cb32e2209c9dcd69 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Mon, 2 Mar 2026 13:53:59 -0600 Subject: [PATCH 12/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/tests/test_migrations.py | 171 +++++++++++++++++-------- 1 file changed, 116 insertions(+), 55 deletions(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 17a305f6..a7f27faf 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -2,7 +2,6 @@ from unittest.mock import patch -import pytest from django.contrib.auth import get_user_model from django.contrib.auth.models import Group from django.core.management import CommandError, call_command @@ -274,6 +273,46 @@ def setUp(self): role="invalid-legacy-role", ) + class MockPermission: + """Mock class to simulate CourseAccessRole entries for testing the rollback migration.""" + def __init__(self, user, role, course_id, id_in): + self.user = user + self.role = role + self.course_id = course_id + self.id = id_in + + class MockUser: + """Mock class to simulate User objects for testing the rollback migration.""" + def __init__(self, username): + self.username = username + + class MockQuerySet: + """Mock class to simulate QuerySet behavior for testing the rollback migration.""" + def __init__(self, permissions): + self.permissions = permissions + + def filter(self, **kwargs): + return self + + def select_related(self, *args, **kwargs): + return self + + def all(self): + return self.permissions + + def get_or_create(self): + raise Exception("Unexpected error mock") + + class MockCourseAccessRole: + """Mock class to simulate CourseAccessRole manager for testing the rollback migration.""" + objects = MockQuerySet( + [ + MockPermission(MockUser("testuser"), "instructor", "course-v1:test", 1), + ] + ) + + self.mock_course_access_role = MockCourseAccessRole + def tearDown(self): """ Clean up test data created for the migration test. @@ -927,69 +966,91 @@ def test_authz_migrate_course_authoring_command_with_org_and_courses(self): with self.assertRaises(CommandError): call_command("authz_migrate_course_authoring", "--course-id-list", self.course_id, "--org-id", self.org) + @patch("openedx_authz.engine.utils.assign_role_to_user_in_scope", return_value=False) + @patch("openedx_authz.engine.utils.LEGACY_COURSE_ROLE_EQUIVALENCES", {"instructor": "instructor-role"}) + def test_migrate_legacy_course_roles_to_authz_user_not_added( + self, + _, # comes from patch + ): + errors, successes = migrate_legacy_course_roles_to_authz( + self.mock_course_access_role, + course_id_list=["course-v1:test"], + org_id=None, + delete_after_migration=False, + ) -@pytest.fixture -def mock_course_access_role(): - """Fixture to mock the CourseAccessRole model and its queryset - for testing the migration functions without relying on the actual database model.""" - - class MockPermission: - """A simple class to represent a permission with user, role, course_id and id attributes.""" - - def __init__(self, user, role, course_id, id_in): - self.user = user - self.role = role - self.course_id = course_id - self.id = id_in - - class MockUser: - """A simple class to represent a user with a username attribute.""" + self.assertEqual(len(errors), 1) + self.assertEqual(len(successes), 0) + self.assertEqual(errors[0].user.username, "testuser") - def __init__(self, username): - self.username = username + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_authz_to_legacy_course_roles_user_not_added(self): + permissions_with_errors, permissions_with_no_errors = migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=[self.course_id], org_id=None, delete_after_migration=False + ) + self.assertEqual(len(permissions_with_errors), 1) + self.assertEqual(len(permissions_with_no_errors), 12) + errors, successes = migrate_authz_to_legacy_course_roles( + self.mock_course_access_role, + UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) - class MockQuerySet: - """A simple class to represent a queryset of permissions.""" + # 3 users for each of the 4 roles = 12 total entries that will + # fail to migrate back to legacy roles due to our mock + self.assertEqual(len(errors), 12) + self.assertEqual(len(successes), 0) - def __init__(self, permissions): - self.permissions = permissions + def create_library_env(self): + """Helper method to create a ContentLibrary environment for testing the migration of legacy permissions + related to ContentLibraryPermission to the new Casbin-based model. + """ - def filter(self, **kwargs): - return self + # Create ContentLibrary + org = Organization.objects.create(name=org_name, short_name=org_short_name) + library = ContentLibrary.objects.create(org=org, slug=lib_name) - def select_related(self, *args, **kwargs): - return self + # Create Users and Groups + users = [ + User.objects.create_user(username=user_name, email=f"lib_{user_name}@example.com") + for user_name in user_names + ] - def all(self): - return self.permissions + group_users = [ + User.objects.create_user(username=user_name, email=f"lib_{user_name}@example.com") + for user_name in group_user_names + ] + group = Group.objects.create(name=group_name) + group.user_set.set(group_users) - class MockCourseAccessRole: - """A simple class to represent the CourseAccessRole model.""" + # Assign legacy permissions for users and group + for user in users: + ContentLibraryPermission.objects.create( + user=user, + library=library, + access_level=ContentLibraryPermission.ADMIN_LEVEL, + ) - objects = MockQuerySet( - [ - MockPermission(MockUser("testuser"), "instructor", "course-v1:test", 1), - ] + ContentLibraryPermission.objects.create( + group=group, + library=library, + access_level=ContentLibraryPermission.READ_LEVEL, ) - def __init__(self): - pass - - return MockCourseAccessRole - + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_authz_to_legacy_course_roles_with_no_course_scopes(self): + self.create_library_env() + migrate_legacy_permissions(ContentLibraryPermission) + permissions_with_errors, permissions_with_no_errors = migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=[self.course_id], org_id=None, delete_after_migration=False + ) + self.assertEqual(len(permissions_with_errors), 1) + self.assertEqual(len(permissions_with_no_errors), 12) + errors, successes = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, course_id_list=[self.course_id], org_id=None, delete_after_migration=False + ) -# pylint: disable=redefined-outer-name -def test_migrate_legacy_course_roles_to_authz_user_not_added(monkeypatch, mock_course_access_role): - # Patch assign_role_to_user_in_scope to always return False - monkeypatch.setattr( - "openedx_authz.engine.utils.assign_role_to_user_in_scope", - lambda user_external_key, role_external_key, scope_external_key: False, - ) - # Patch LEGACY_COURSE_ROLE_EQUIVALENCES - monkeypatch.setattr("openedx_authz.engine.utils.LEGACY_COURSE_ROLE_EQUIVALENCES", {"instructor": "instructor-role"}) - errors, successes = migrate_legacy_course_roles_to_authz( - mock_course_access_role, course_id_list=["course-v1:test"], org_id=None, delete_after_migration=False - ) - assert len(errors) == 1 - assert len(successes) == 0 - assert errors[0].user.username == "testuser" + self.assertEqual(len(errors), 0) + self.assertEqual(len(successes), 12) From bd4f1a3c5d9560841ca79c4514b2da14ee9bb178 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Mon, 2 Mar 2026 16:42:18 -0600 Subject: [PATCH 13/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/tests/test_migrations.py | 94 +++++++++++++++++++++++--- 1 file changed, 83 insertions(+), 11 deletions(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index a7f27faf..3f0d4eae 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -275,6 +275,7 @@ def setUp(self): class MockPermission: """Mock class to simulate CourseAccessRole entries for testing the rollback migration.""" + def __init__(self, user, role, course_id, id_in): self.user = user self.role = role @@ -283,11 +284,13 @@ def __init__(self, user, role, course_id, id_in): class MockUser: """Mock class to simulate User objects for testing the rollback migration.""" + def __init__(self, username): self.username = username class MockQuerySet: """Mock class to simulate QuerySet behavior for testing the rollback migration.""" + def __init__(self, permissions): self.permissions = permissions @@ -305,6 +308,7 @@ def get_or_create(self): class MockCourseAccessRole: """Mock class to simulate CourseAccessRole manager for testing the rollback migration.""" + objects = MockQuerySet( [ MockPermission(MockUser("testuser"), "instructor", "course-v1:test", 1), @@ -946,6 +950,54 @@ def test_authz_rollback_course_authoring_command(self, mock_rollback): self.assertEqual(call_kwargs["delete_after_migration"], True) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") + def test_authz_migrate_course_authoring_command_delete_confirmation_no(self, mock_migrate): + """ + Verify that the authz_migrate_course_authoring command does NOT perform deletion + when user provides an answer other than 'yes' during delete confirmation. + """ + mock_migrate.return_value = [], [] + with patch("builtins.input", return_value="no"): + call_command("authz_migrate_course_authoring", "--delete", "--course-id-list", self.course_id) + mock_migrate.assert_not_called() + + @patch("openedx_authz.management.commands.authz_rollback_course_authoring.CourseAccessRole", CourseAccessRole) + @patch("openedx_authz.management.commands.authz_rollback_course_authoring.migrate_authz_to_legacy_course_roles") + def test_authz_rollback_course_authoring_command_delete_confirmation_no(self, mock_rollback): + """ + Verify that the authz_rollback_course_authoring command does NOT perform deletion + when user provides an answer other than 'yes' during delete confirmation. + """ + mock_rollback.return_value = [], [] + with patch("builtins.input", return_value="no"): + call_command("authz_rollback_course_authoring", "--delete", "--course-id-list", self.course_id) + mock_rollback.assert_not_called() + + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.migrate_legacy_course_roles_to_authz") + def test_authz_migrate_course_authoring_command_migration_exception(self, mock_migrate): + """ + Verify that the authz_migrate_course_authoring command handles exceptions raised + by migrate_legacy_course_roles_to_authz. + """ + mock_migrate.side_effect = Exception("Migration error") + with self.assertRaises(Exception) as exc: + call_command("authz_migrate_course_authoring", "--course-id-list", self.course_id) + self.assertIn("Migration error", str(exc.exception)) + + @patch("openedx_authz.management.commands.authz_rollback_course_authoring.CourseAccessRole", CourseAccessRole) + @patch("openedx_authz.management.commands.authz_rollback_course_authoring.migrate_authz_to_legacy_course_roles") + def test_authz_rollback_course_authoring_command_rollback_exception(self, mock_rollback): + """ + Verify that the authz_rollback_course_authoring command handles exceptions raised + by migrate_authz_to_legacy_course_roles. + """ + mock_rollback.side_effect = Exception("Rollback error") + with self.assertRaises(Exception) as exc: + call_command("authz_rollback_course_authoring", "--course-id-list", self.course_id) + self.assertIn("Rollback error", str(exc.exception)) + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) def test_authz_migrate_course_authoring_command_no_org_and_courses(self): """ @@ -956,6 +1008,16 @@ def test_authz_migrate_course_authoring_command_no_org_and_courses(self): with self.assertRaises(CommandError): call_command("authz_migrate_course_authoring") + @patch("openedx_authz.management.commands.authz_rollback_course_authoring.CourseAccessRole", CourseAccessRole) + def test_authz_rollback_course_authoring_command_no_org_and_courses(self): + """ + Verify that the authz_rollback_course_authoring command raises an error + when neither course_id_list nor org_id is provided. + """ + + with self.assertRaises(CommandError): + call_command("authz_rollback_course_authoring") + @patch("openedx_authz.management.commands.authz_migrate_course_authoring.CourseAccessRole", CourseAccessRole) def test_authz_migrate_course_authoring_command_with_org_and_courses(self): """ @@ -966,6 +1028,16 @@ def test_authz_migrate_course_authoring_command_with_org_and_courses(self): with self.assertRaises(CommandError): call_command("authz_migrate_course_authoring", "--course-id-list", self.course_id, "--org-id", self.org) + @patch("openedx_authz.management.commands.authz_rollback_course_authoring.CourseAccessRole", CourseAccessRole) + def test_authz_rollback_course_authoring_command_with_org_and_courses(self): + """ + Verify that the authz_rollback_course_authoring command raises an error + when both course_id_list and org_id are provided. + """ + + with self.assertRaises(CommandError): + call_command("authz_rollback_course_authoring", "--course-id-list", self.course_id, "--org-id", self.org) + @patch("openedx_authz.engine.utils.assign_role_to_user_in_scope", return_value=False) @patch("openedx_authz.engine.utils.LEGACY_COURSE_ROLE_EQUIVALENCES", {"instructor": "instructor-role"}) def test_migrate_legacy_course_roles_to_authz_user_not_added( @@ -1004,29 +1076,24 @@ def test_migrate_authz_to_legacy_course_roles_user_not_added(self): self.assertEqual(len(successes), 0) def create_library_env(self): - """Helper method to create a ContentLibrary environment for testing the migration of legacy permissions - related to ContentLibraryPermission to the new Casbin-based model. + """ + Helper method to create a ContentLibrary environment for testing. """ # Create ContentLibrary org = Organization.objects.create(name=org_name, short_name=org_short_name) library = ContentLibrary.objects.create(org=org, slug=lib_name) - # Create Users and Groups - users = [ - User.objects.create_user(username=user_name, email=f"lib_{user_name}@example.com") - for user_name in user_names - ] - + # Create Groups group_users = [ - User.objects.create_user(username=user_name, email=f"lib_{user_name}@example.com") + User.objects.create_user(username=user_name, email=f"{user_name}@example.com") for user_name in group_user_names ] group = Group.objects.create(name=group_name) group.user_set.set(group_users) # Assign legacy permissions for users and group - for user in users: + for user in self.admin_users + self.staff_users + self.limited_staff + self.data_researcher: ContentLibraryPermission.objects.create( user=user, library=library, @@ -1040,7 +1107,12 @@ def create_library_env(self): ) @patch("openedx_authz.api.data.CourseOverview", CourseOverview) - def test_migrate_authz_to_legacy_course_roles_with_no_course_scopes(self): + def test_migrate_authz_to_legacy_course_roles_with_library_env(self): + """Test the migration of permissions from the new Casbin-based model back to the legacy CourseAccessRole model + in a ContentLibrary environment to ensure that the migration functions correctly identify the relevant + permissions based on the scope and do not attempt to migrate permissions that are outside + of the specified scope (i.e. permissions related to the ContentLibrary in this case). + """ self.create_library_env() migrate_legacy_permissions(ContentLibraryPermission) permissions_with_errors, permissions_with_no_errors = migrate_legacy_course_roles_to_authz( From af13f30bed5fce52903300fcceb37a2bc924a45f Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Mon, 2 Mar 2026 19:55:58 -0600 Subject: [PATCH 14/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/engine/utils.py | 30 ++++++++++++++----- .../authz_migrate_course_authoring.py | 2 +- .../authz_rollback_course_authoring.py | 4 +-- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 814f6a89..e11011dd 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -168,7 +168,7 @@ def migrate_legacy_permissions(ContentLibraryPermission): return permissions_with_errors -def migrate_legacy_course_roles_to_authz(CourseAccessRole, course_id_list, org_id, delete_after_migration): +def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_list, org_id, delete_after_migration): """ Migrate legacy course role data to the new Casbin-based authorization model. This function reads legacy permissions from the CourseAccessRole model @@ -187,7 +187,11 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, course_id_list, org_i - user: subject - role: role - param CourseAccessRole: The CourseAccessRole model to use. + param course_access_role_model: The CourseAccessRole model to use. This is passed in because the function + 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 delete successfully migrated legacy permissions after migration. """ if not course_id_list and not org_id: raise ValueError( @@ -205,7 +209,9 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, course_id_list, org_i # otherwise we will filter by org_id which is more efficient course_access_role_filter["course_id__in"] = course_id_list - legacy_permissions = CourseAccessRole.objects.filter(**course_access_role_filter).select_related("user").all() + legacy_permissions = ( + course_access_role_model.objects.filter(**course_access_role_filter).select_related("user").all() + ) # List to keep track of any permissions that could not be migrated permissions_with_errors = [] @@ -246,12 +252,14 @@ def migrate_legacy_course_roles_to_authz(CourseAccessRole, course_id_list, org_i if delete_after_migration: # Only delete permissions that were successfully migrated to avoid data loss. - CourseAccessRole.objects.filter(id__in=[p.id for p in permissions_with_no_errors]).delete() + course_access_role_model.objects.filter(id__in=[p.id for p in permissions_with_no_errors]).delete() return permissions_with_errors, permissions_with_no_errors -def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, course_id_list, org_id, delete_after_migration): +def migrate_authz_to_legacy_course_roles( + course_access_role_model, user_subject_model, course_id_list, org_id, delete_after_migration +): """ 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 @@ -259,6 +267,14 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, course_i This is essentially the reverse of migrate_legacy_course_roles_to_authz and is intended for rollback purposes in case of migration issues. + + param course_access_role_model: The CourseAccessRole model to use. This is passed in because the function + is intended to run within a Django migration context, where direct model imports can cause issues. + param user_subject_model: The UserSubject model to query for users with course-related permissions. + 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. """ if not course_id_list and not org_id: raise ValueError( @@ -279,7 +295,7 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, course_i # otherwise we will filter by org_id which is more efficient course_subject_filter["casbin_rules__scope__coursescope__course_overview__id__in"] = course_id_list - course_subjects = UserSubject.objects.filter(**course_subject_filter).select_related("user").distinct() + course_subjects = user_subject_model.objects.filter(**course_subject_filter).select_related("user").distinct() roles_with_errors = [] roles_with_no_errors = [] @@ -309,7 +325,7 @@ def migrate_authz_to_legacy_course_roles(CourseAccessRole, UserSubject, course_i try: # Create legacy CourseAccessRole entry - CourseAccessRole.objects.get_or_create( + course_access_role_model.objects.get_or_create( user=user, org=course_overview.org, course_id=scope, diff --git a/openedx_authz/management/commands/authz_migrate_course_authoring.py b/openedx_authz/management/commands/authz_migrate_course_authoring.py index 2437770f..30e4d748 100644 --- a/openedx_authz/management/commands/authz_migrate_course_authoring.py +++ b/openedx_authz/management/commands/authz_migrate_course_authoring.py @@ -64,7 +64,7 @@ def handle(self, *args, **options): return with transaction.atomic(): errors, success = migrate_legacy_course_roles_to_authz( - CourseAccessRole=CourseAccessRole, + course_access_role_model=CourseAccessRole, course_id_list=course_id_list, org_id=org_id, delete_after_migration=delete_after_migration, diff --git a/openedx_authz/management/commands/authz_rollback_course_authoring.py b/openedx_authz/management/commands/authz_rollback_course_authoring.py index bab89d58..555757a4 100644 --- a/openedx_authz/management/commands/authz_rollback_course_authoring.py +++ b/openedx_authz/management/commands/authz_rollback_course_authoring.py @@ -67,8 +67,8 @@ def handle(self, *args, **options): return with transaction.atomic(): errors, success = migrate_authz_to_legacy_course_roles( - CourseAccessRole=CourseAccessRole, - UserSubject=UserSubject, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, course_id_list=course_id_list, org_id=org_id, delete_after_migration=delete_after_migration, # control deletion here From bc7624de5d9e05de3299a50620ce2d09f6810d35 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Tue, 3 Mar 2026 10:45:15 -0600 Subject: [PATCH 15/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/engine/utils.py | 13 ++++++++++--- openedx_authz/tests/test_migrations.py | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index e11011dd..3765db20 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -187,7 +187,7 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis - user: subject - role: role - param course_access_role_model: The CourseAccessRole model to use. This is passed in because the function + param course_access_role_model: It should be the CourseAccessRole model. This is passed in because the function 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. @@ -253,6 +253,8 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis 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() + 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 @@ -268,9 +270,10 @@ def migrate_authz_to_legacy_course_roles( This is essentially the reverse of migrate_legacy_course_roles_to_authz and is intended for rollback purposes in case of migration issues. - param course_access_role_model: The CourseAccessRole model to use. This is passed in because the function + param course_access_role_model: It should be the CourseAccessRole model. This is passed in because the function + is intended to run within a Django migration context, where direct model imports can cause issues. + param user_subject_model: It should be the UserSubject model. This is passed in because the function is intended to run within a Django migration context, where direct model imports can cause issues. - param user_subject_model: The UserSubject model to query for users with course-related permissions. 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 @@ -347,4 +350,8 @@ def migrate_authz_to_legacy_course_roles( role_external_key=role.external_key, scope_external_key=scope, ) + logger.info( + f"Unassigned Role: {role.external_key} from User: {user_external_key} in Scope: {scope}" + f" after successful rollback migration." + ) return roles_with_errors, roles_with_no_errors diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 3f0d4eae..795ccbba 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -1042,7 +1042,7 @@ def test_authz_rollback_course_authoring_command_with_org_and_courses(self): @patch("openedx_authz.engine.utils.LEGACY_COURSE_ROLE_EQUIVALENCES", {"instructor": "instructor-role"}) def test_migrate_legacy_course_roles_to_authz_user_not_added( self, - _, # comes from patch + _, ): errors, successes = migrate_legacy_course_roles_to_authz( self.mock_course_access_role, From 9fbc920721159197bc288b47a98ba0717328f9a9 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Tue, 3 Mar 2026 11:27:47 -0600 Subject: [PATCH 16/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/engine/utils.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 3765db20..17402e92 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -188,7 +188,7 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis - role: role param course_access_role_model: It should be the CourseAccessRole model. This is passed in because the function - is intended to run within a Django migration context, where direct model imports can cause issues. + 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 delete successfully migrated legacy permissions after migration. @@ -271,13 +271,13 @@ def migrate_authz_to_legacy_course_roles( for rollback purposes in case of migration issues. param course_access_role_model: It should be the CourseAccessRole model. This is passed in because the function - is intended to run within a Django migration context, where direct model imports can cause issues. + is intended to run within a Django migration context, where direct model imports can cause issues. param user_subject_model: It should be the UserSubject model. This is passed in because the function - is intended to run within a Django migration context, where direct model imports can cause issues. + 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. + from the new model after migration. """ if not course_id_list and not org_id: raise ValueError( From ccfea4dd526ce3dc9272254236469c1e5bdcbf69 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Tue, 3 Mar 2026 17:08:37 -0600 Subject: [PATCH 17/18] fixup! feat: add course authoring migration and rollback scripts --- .../management/commands/authz_migrate_course_authoring.py | 4 ++-- .../management/commands/authz_rollback_course_authoring.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx_authz/management/commands/authz_migrate_course_authoring.py b/openedx_authz/management/commands/authz_migrate_course_authoring.py index 30e4d748..0515e26e 100644 --- a/openedx_authz/management/commands/authz_migrate_course_authoring.py +++ b/openedx_authz/management/commands/authz_migrate_course_authoring.py @@ -46,7 +46,7 @@ def handle(self, *args, **options): org_id = options.get("org_id") if not course_id_list and not org_id: - raise CommandError("You must specify either --course-id-list or --org-id to filter the rollback.") + raise CommandError("You must specify either --course-id-list or --org-id to filter the migration.") if course_id_list and org_id: raise CommandError("You cannot use --course-id-list and --org-id together.") @@ -60,7 +60,7 @@ def handle(self, *args, **options): ) if confirm != "yes": - self.stdout.write(self.style.WARNING("Deletion aborted.")) + self.stdout.write(self.style.WARNING("Migration aborted.")) return with transaction.atomic(): errors, success = migrate_legacy_course_roles_to_authz( diff --git a/openedx_authz/management/commands/authz_rollback_course_authoring.py b/openedx_authz/management/commands/authz_rollback_course_authoring.py index 555757a4..d57d4995 100644 --- a/openedx_authz/management/commands/authz_rollback_course_authoring.py +++ b/openedx_authz/management/commands/authz_rollback_course_authoring.py @@ -63,7 +63,7 @@ def handle(self, *args, **options): ) if confirm != "yes": - self.stdout.write(self.style.WARNING("Deletion aborted.")) + self.stdout.write(self.style.WARNING("Rollback aborted.")) return with transaction.atomic(): errors, success = migrate_authz_to_legacy_course_roles( From d830f3ca0632663573e810fd03da22832bce82c5 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 4 Mar 2026 15:26:41 -0600 Subject: [PATCH 18/18] fixup! feat: add course authoring migration and rollback scripts --- openedx_authz/admin.py | 17 ---------- openedx_authz/engine/utils.py | 34 +++++++++++++------ .../authz_migrate_course_authoring.py | 2 +- .../authz_rollback_course_authoring.py | 2 +- 4 files changed, 25 insertions(+), 30 deletions(-) diff --git a/openedx_authz/admin.py b/openedx_authz/admin.py index 52470867..e2285158 100644 --- a/openedx_authz/admin.py +++ b/openedx_authz/admin.py @@ -5,8 +5,6 @@ from django.contrib import admin from openedx_authz.models import ExtendedCasbinRule -from openedx_authz.models.scopes import CourseScope -from openedx_authz.models.subjects import UserSubject class CasbinRuleForm(forms.ModelForm): @@ -50,18 +48,3 @@ 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(ExtendedCasbinRule) -class ExtendedCasbinRuleAdmin(admin.ModelAdmin): - pass - - -@admin.register(UserSubject) -class UserSubjectAdmin(admin.ModelAdmin): - pass - - -@admin.register(CourseScope) -class CourseScopeAdmin(admin.ModelAdmin): - list_display = ("id", "course_overview") diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 17402e92..8823d4c6 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -5,6 +5,7 @@ """ import logging +from collections import defaultdict from casbin import Enforcer @@ -195,7 +196,7 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis """ if not course_id_list and not org_id: raise ValueError( - "At least one of course_id_list or org_id must be provided to limit the scope of the rollback migration." + "At least one of course_id_list or org_id must be provided to limit the scope of the migration." ) course_access_role_filter = { "course_id__startswith": "course-v1:", @@ -302,6 +303,7 @@ def migrate_authz_to_legacy_course_roles( roles_with_errors = [] roles_with_no_errors = [] + unassignments = defaultdict(list) for course_subject in course_subjects: user = course_subject.user @@ -343,15 +345,25 @@ def migrate_authz_to_legacy_course_roles( roles_with_errors.append((user_external_key, role.external_key, scope)) continue - # If we successfully created the legacy role, we can unassign the new role + # If we successfully created the legacy role, we can add this role assignment + # to the unassignment list if delete_after_migration is True if delete_after_migration: - batch_unassign_role_from_users( - users=[user_external_key], - role_external_key=role.external_key, - scope_external_key=scope, - ) - logger.info( - f"Unassigned Role: {role.external_key} from User: {user_external_key} in Scope: {scope}" - f" after successful rollback migration." - ) + unassignments[(role.external_key, scope)].append(user_external_key) + + # Once the loop is done, we can log summary of unassignments + # and perform batch unassignment if delete_after_migration is True + if delete_after_migration: + total_unassignments = sum(len(users) for users in unassignments.values()) + 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"in Scope: {scope} after successful rollback migration." + ) + batch_unassign_role_from_users( + users=users, + role_external_key=role_external_key, + scope_external_key=scope, + ) + return roles_with_errors, roles_with_no_errors diff --git a/openedx_authz/management/commands/authz_migrate_course_authoring.py b/openedx_authz/management/commands/authz_migrate_course_authoring.py index 0515e26e..1968f5fa 100644 --- a/openedx_authz/management/commands/authz_migrate_course_authoring.py +++ b/openedx_authz/management/commands/authz_migrate_course_authoring.py @@ -29,7 +29,7 @@ def add_arguments(self, parser): ) parser.add_argument( "--course-id-list", - nargs="*", + nargs="+", type=str, help="Optional list of course IDs to filter the migration.", ) diff --git a/openedx_authz/management/commands/authz_rollback_course_authoring.py b/openedx_authz/management/commands/authz_rollback_course_authoring.py index d57d4995..7d323998 100644 --- a/openedx_authz/management/commands/authz_rollback_course_authoring.py +++ b/openedx_authz/management/commands/authz_rollback_course_authoring.py @@ -31,7 +31,7 @@ def add_arguments(self, parser): ) parser.add_argument( "--course-id-list", - nargs="*", + nargs="+", type=str, help="Optional list of course IDs to filter the migration.", )