From 6da5271303290d04336cb5b8f8cecf5504b207e4 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 7 Apr 2026 13:11:10 +0200 Subject: [PATCH 01/19] feat: consider org-level - org scope match access when migrating to new model --- openedx_authz/api/data.py | 18 ++++++++++++++++++ openedx_authz/engine/utils.py | 29 ++++++++++++++++++++++------- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/openedx_authz/api/data.py b/openedx_authz/api/data.py index e6e18f41..8caa450f 100644 --- a/openedx_authz/api/data.py +++ b/openedx_authz/api/data.py @@ -695,6 +695,24 @@ def get_admin_view_permission(cls) -> PermissionData: """ raise NotImplementedError("Subclasses must implement get_admin_view_permission method.") + @classmethod + def build_external_key(cls, org: str) -> str: + """Build the external key for all resources within the given organization. + + Args: + org (str): The organization identifier (e.g., ``DemoX``). + + Returns: + str: The external key for the org-level glob (e.g., ``course-v1:DemoX+*``). + + Examples: + >>> OrgCourseOverviewGlobData.build_external_key('DemoX') + 'course-v1:DemoX+*' + >>> OrgContentLibraryGlobData.build_external_key('DemoX') + 'lib:DemoX:*' + """ + return f"{cls.NAMESPACE}{EXTERNAL_KEY_SEPARATOR}{org}{cls.ID_SEPARATOR}{GLOBAL_SCOPE_WILDCARD}" + @classmethod def get_org(cls, external_key: str) -> str | None: """Extract the organization identifier from the glob pattern. diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index e844dcf8..54256323 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -9,7 +9,7 @@ from casbin import Enforcer -from openedx_authz.api.data import CourseOverviewData +from openedx_authz.api.data import CourseOverviewData, OrgCourseOverviewGlobData from openedx_authz.api.users import ( assign_role_to_user_in_scope, batch_assign_role_to_users_in_scope, @@ -204,6 +204,11 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis - user: subject - role: role + The scope assigned per row depends on which fields are set: + - course_id set: course-level scope (e.g. "course-v1:OpenedX+CS101+2024"). + - course_id blank, org set: org-level glob scope (e.g. "course-v1:OpenedX+*"). + - both set: course_id takes precedence as the more specific scope. + 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. @@ -211,10 +216,8 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis param delete_after_migration: Whether to delete successfully migrated legacy permissions after migration. """ _validate_migration_input(course_id_list, org_id) - - course_access_role_filter = { - "course_id__startswith": "course-v1:", - } +W + course_access_role_filter = {} if org_id: course_access_role_filter["org"] = org_id @@ -243,16 +246,28 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis permissions_with_errors.append(permission) continue + if permission.course_id: + scope_external_key = str(permission.course_id) + elif permission.org: + scope_external_key = OrgCourseOverviewGlobData.build_external_key(permission.org) + else: + # This should not happen as either course_id or org should be defined for each permission, log and skip + logger.error( + f"Permission for User: {permission.user.username} has neither course_id nor org defined, skipping." + ) + 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} in Scope: {permission.course_id}" + f"to Role: {role} in Scope: {scope_external_key}" ) is_user_added = assign_role_to_user_in_scope( user_external_key=permission.user.username, role_external_key=role, - scope_external_key=str(permission.course_id), + scope_external_key=scope_external_key, ) if not is_user_added: From 32517541662f92c04b71722f1821c0fc2c98b7e2 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 7 Apr 2026 16:52:18 +0200 Subject: [PATCH 02/19] refactor: use only runtime utilities to filter by course namespace (glob or specific key) --- openedx_authz/api/roles.py | 20 +++ openedx_authz/engine/utils.py | 121 ++++++++++-------- openedx_authz/tests/test_migrations.py | 167 +++---------------------- 3 files changed, 101 insertions(+), 207 deletions(-) diff --git a/openedx_authz/api/roles.py b/openedx_authz/api/roles.py index 1215b79b..489fb52c 100644 --- a/openedx_authz/api/roles.py +++ b/openedx_authz/api/roles.py @@ -44,6 +44,7 @@ "get_subject_role_assignments", "get_subject_role_assignments_for_role_in_scope", "get_subject_role_assignments_in_scope", + "get_all_role_assignments_per_scope_type", "unassign_role_from_subject_in_scope", "unassign_subject_from_all_roles", ] @@ -553,3 +554,22 @@ def unassign_subject_from_all_roles(subject: SubjectData) -> bool: """ enforcer = AuthzEnforcer.get_enforcer() return enforcer.remove_filtered_grouping_policy(GroupingPolicyIndex.SUBJECT.value, subject.namespaced_key) + + +def get_all_role_assignments_per_scope_type(scope_type: type[ScopeData]) -> list[RoleAssignmentData]: + """Get all role assignments for a specific scope type. + + Loads all grouping policies from the enforcer and filters in Python. Casbin policies + store full scope keys (e.g., 'course-v1^course-v1:Org+Course+Run'), so there is no + way to query by scope type directly so the filtering must happen here. + + Args: + scope_type: A ScopeData subclass (not an instance) used to match by NAMESPACE. + + Returns: + list[RoleAssignmentData]: All assignments whose scope matches the given scope type. + """ + return [ + role_assignment for role_assignment in get_role_assignments() + if role_assignment.scope.NAMESPACE == scope_type.NAMESPACE + ] diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 54256323..6e49310c 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -10,11 +10,11 @@ from casbin import Enforcer from openedx_authz.api.data import CourseOverviewData, OrgCourseOverviewGlobData +from openedx_authz.api.roles import get_all_role_assignments_per_scope_type 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 ( LEGACY_COURSE_ROLE_EQUIVALENCES, @@ -216,7 +216,7 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis param delete_after_migration: Whether to delete successfully migrated legacy permissions after migration. """ _validate_migration_input(course_id_list, org_id) -W + course_access_role_filter = {} if org_id: @@ -301,6 +301,12 @@ 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. + To build each CourseAccessRole entry, the function needs: + - A user: resolved from role assignments in scopes linked to courses. + - A scope: either a CourseOverviewData (course-level) or OrgCourseOverviewGlobData (org-level glob), + filtered by course_id or org_id if provided. + - A role: a role external key that maps to a legacy role in COURSE_ROLE_EQUIVALENCES. + 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 @@ -312,70 +318,77 @@ def migrate_authz_to_legacy_course_roles( """ _validate_migration_input(course_id_list, org_id) - # 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_subject_filter = { - "casbin_rules__scope__coursescope__course_overview__isnull": False, - } + # CourseOverviewData and OrgCourseOverviewGlobData share the same namespace, + # so filtering by CourseOverviewData captures both course-level and org-level glob assignments. + role_assignments = get_all_role_assignments_per_scope_type(scope_type=CourseOverviewData) + # Two cases here: + # 1. If org_id is provided, we filter by org_id which will include both org-level glob scopes and course-level scopes linked to that org + # 2. If only course_id_list is provided, we filter by course_id which will include only course-level scopes linked to those course_ids since + # org-level glob scopes don't have course_id in their scope object if org_id: - course_subject_filter["casbin_rules__scope__coursescope__course_overview__org"] = org_id + role_assignments = [ + role_assignment + for role_assignment in role_assignments + if role_assignment.scope.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 = user_subject_model.objects.filter(**course_subject_filter).select_related("user").distinct() + role_assignments = [ + role_assignment + for role_assignment in role_assignments + if isinstance(role_assignment.scope, CourseOverviewData) + and role_assignment.scope.external_key in course_id_list + ] roles_with_errors = [] roles_with_no_errors = [] unassignments = defaultdict(list) - for course_subject in course_subjects: - user = course_subject.user - user_external_key = user.username + for role_assignment in role_assignments: + + # Per valid role assignment, create corresponding CourseAccessRole entry + try: + user_external_key = role_assignment.subject.external_key + role_external_key = role_assignment.roles[0].external_key + scope_external_key = role_assignment.scope.external_key + + course_access_role_kwargs = { + "user": user_subject_model.objects.get(user__username=user_external_key).user, + "role": COURSE_ROLE_EQUIVALENCES[role_external_key], + } + + # Here we prioritize course_id over org for scope since course-level scope is more specific + # and also both are not needed to create a valid CourseAccessRole entry + if isinstance(role_assignment.scope, CourseOverviewData): + course_access_role_kwargs["course_id"] = scope_external_key + elif isinstance(role_assignment.scope, OrgCourseOverviewGlobData): + course_access_role_kwargs["org"] = role_assignment.scope.org + else: + logger.error( + f"Unexpected scope type: {type(role_assignment.scope)} for RoleAssignment with scope: {scope_external_key}" + ) + roles_with_errors.append(role_assignment) + continue + + course_access_role_model.objects.create(**course_access_role_kwargs) + roles_with_no_errors.append(role_assignment) - # 2. Get all role assignments for the user - role_assignments = get_user_role_assignments(user_external_key=user_external_key) + logger.info( + f"Successfully rolled back RoleAssignment for User: {user_external_key} " + f"in Role: {role_external_key} and Scope: {scope_external_key} " + f"to legacy CourseAccessRole entry." + ) - 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 + if delete_after_migration: + unassignments[(role_external_key, scope_external_key)].append(user_external_key) - scope = assignment.scope.external_key - - course_overview = assignment.scope.get_object() - - for role in assignment.roles: - 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)) - continue - - try: - # Create legacy CourseAccessRole entry - course_access_role_model.objects.get_or_create( - user=user, - org=course_overview.org, - 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: " - 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 add this role assignment - # to the unassignment list if delete_after_migration is True - if delete_after_migration: - unassignments[(role.external_key, scope)].append(user_external_key) + except Exception as e: + logger.error( + f"Error rolling back RoleAssignment for User: {role_assignment.subject.external_key} " + f"in Role: {role_assignment.roles[0].external_key} and Scope: {role_assignment.scope.external_key}: {e}" + ) + roles_with_errors.append(role_assignment) # Once the loop is done, we can log summary of unassignments # and perform batch unassignment if delete_after_migration is True diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 17450ebb..d3dcfbf7 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -295,16 +295,16 @@ class MockQuerySet: def __init__(self, permissions): self.permissions = permissions - def filter(self, **kwargs): + def filter(self, **_): return self - def select_related(self, *args, **kwargs): + def select_related(self, *_, **__): return self def all(self): return self.permissions - def get_or_create(self): + def get_or_create(self, **_): raise Exception("Unexpected error mock") class MockCourseAccessRole: @@ -440,23 +440,11 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): # 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=course_id_list, org_id=None, 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 + # Casbin assignments are intact since delete_after_migration is False for user in self.admin_users: assignments = get_user_role_assignments_in_scope( user_external_key=user.username, scope_external_key=self.course_id @@ -485,34 +473,6 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): 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) - .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 @@ -594,23 +554,11 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): # 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=course_id_list, org_id=None, 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 + # Casbin assignments are removed since delete_after_migration is True for user in self.admin_users: assignments = get_user_role_assignments_in_scope( user_external_key=user.username, scope_external_key=self.course_id @@ -635,30 +583,12 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): 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) + # 3 users * 4 roles = 12 recreated entries, plus the original invalid entry = 13 total + self.assertEqual(len(after_migrate_state_access_roles), 13) @patch("openedx_authz.api.data.CourseOverview", CourseOverview) def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equivalent(self): @@ -675,17 +605,6 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi # 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 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 @@ -698,8 +617,7 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi 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 - # and that errors are logged for any permissions that could not be rolled back + # Admin assignments are removed; the rest remain since they had no legacy equivalent for user in self.admin_users: assignments = get_user_role_assignments_in_scope( user_external_key=user.username, scope_external_key=self.course_id @@ -709,53 +627,28 @@ 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 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 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 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) # 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 + # 1 original invalid entry + 3 admin users rolled back = 4 total 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 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 @@ -805,27 +698,13 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): # 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 # 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 + # Casbin assignments are removed since delete_after_migration is True for user in self.admin_users: assignments = get_user_role_assignments_in_scope( user_external_key=user.username, scope_external_key=self.course_id @@ -850,31 +729,13 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): 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 + # 3 users * 4 roles = 12 recreated entries, plus the original invalid entry = 13 total 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): # Migrate from legacy CourseAccessRole to new Casbin-based model @@ -927,7 +788,7 @@ def test_authz_migrate_course_authoring_command(self, mock_migrate): call_command("authz_migrate_course_authoring", "--course-id-list", self.course_id) mock_migrate.assert_called_once() - args, kwargs = mock_migrate.call_args + _, kwargs = mock_migrate.call_args self.assertEqual(kwargs["delete_after_migration"], False) @@ -938,7 +799,7 @@ def test_authz_migrate_course_authoring_command(self, mock_migrate): call_command("authz_migrate_course_authoring", "--delete", "--course-id-list", self.course_id) mock_migrate.assert_called_once() - args, kwargs = mock_migrate.call_args + _, kwargs = mock_migrate.call_args self.assertEqual(kwargs["delete_after_migration"], True) @@ -1001,7 +862,7 @@ def test_authz_rollback_course_authoring_command(self, mock_rollback): call_command("authz_rollback_course_authoring", "--course-id-list", self.course_id) mock_rollback.assert_called_once() - args, kwargs = mock_rollback.call_args + _, kwargs = mock_rollback.call_args self.assertEqual(kwargs["delete_after_migration"], False) From 543c3d2b39b5f1a5b6d4819e9881bf93bf5e0607 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 7 Apr 2026 17:20:04 +0200 Subject: [PATCH 03/19] refactor: go back to previous tests --- openedx_authz/tests/test_migrations.py | 38 ++++++++++++++++++-------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index d3dcfbf7..1e423942 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -295,16 +295,16 @@ class MockQuerySet: def __init__(self, permissions): self.permissions = permissions - def filter(self, **_): + def filter(self, **kwargs): return self - def select_related(self, *_, **__): + def select_related(self, *args, **kwargs): return self def all(self): return self.permissions - def get_or_create(self, **_): + def get_or_create(self): raise Exception("Unexpected error mock") class MockCourseAccessRole: @@ -444,7 +444,8 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): CourseAccessRole, UserSubject, course_id_list=course_id_list, org_id=None, delete_after_migration=False ) - # Casbin assignments are intact since delete_after_migration is 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 @@ -558,7 +559,8 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): CourseAccessRole, UserSubject, course_id_list=course_id_list, org_id=None, delete_after_migration=True ) - # Casbin assignments are removed since delete_after_migration is 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 @@ -587,7 +589,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") ) - # 3 users * 4 roles = 12 recreated entries, plus the original invalid entry = 13 total + # 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), 13) @patch("openedx_authz.api.data.CourseOverview", CourseOverview) @@ -617,7 +620,8 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi CourseAccessRole, UserSubject, course_id_list=course_id_list, org_id=None, delete_after_migration=True ) - # Admin assignments are removed; the rest remain since they had no legacy equivalent + # 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 @@ -627,16 +631,22 @@ 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 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 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 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) # 3 staff + 3 limited_staff + 3 data_researcher = 9 entries with no legacy role equivalent @@ -646,7 +656,7 @@ 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") ) - # 1 original invalid entry + 3 admin users rolled back = 4 total + # All original entries (1) + 3 users * 1 roles = 4 self.assertEqual(len(after_migrate_state_access_roles), 1 + 3) @patch("openedx_authz.api.data.CourseOverview", CourseOverview) @@ -698,13 +708,16 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): # 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 # Now let's rollback 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 ) - # Casbin assignments are removed since delete_after_migration is 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 @@ -733,7 +746,8 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): CourseAccessRole.objects.all().order_by("id").values("id", "user_id", "org", "course_id", "role") ) - # 3 users * 4 roles = 12 recreated entries, plus the original invalid entry = 13 total + # 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) @patch("openedx_authz.api.data.CourseOverview", CourseOverview) @@ -788,7 +802,7 @@ def test_authz_migrate_course_authoring_command(self, mock_migrate): call_command("authz_migrate_course_authoring", "--course-id-list", self.course_id) mock_migrate.assert_called_once() - _, kwargs = mock_migrate.call_args + args, kwargs = mock_migrate.call_args self.assertEqual(kwargs["delete_after_migration"], False) @@ -799,7 +813,7 @@ def test_authz_migrate_course_authoring_command(self, mock_migrate): call_command("authz_migrate_course_authoring", "--delete", "--course-id-list", self.course_id) mock_migrate.assert_called_once() - _, kwargs = mock_migrate.call_args + args, kwargs = mock_migrate.call_args self.assertEqual(kwargs["delete_after_migration"], True) From ba20d275a06a7e426d95d3d295c75a7994949b51 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 7 Apr 2026 18:18:03 +0200 Subject: [PATCH 04/19] refactor: consider course_id when migrating backward for course-level roles --- openedx_authz/engine/utils.py | 3 +-- openedx_authz/tests/test_migrations.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 6e49310c..f55ec672 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -358,9 +358,8 @@ def migrate_authz_to_legacy_course_roles( "role": COURSE_ROLE_EQUIVALENCES[role_external_key], } - # Here we prioritize course_id over org for scope since course-level scope is more specific - # and also both are not needed to create a valid CourseAccessRole entry if isinstance(role_assignment.scope, CourseOverviewData): + course_access_role_kwargs["org"] = role_assignment.scope.org course_access_role_kwargs["course_id"] = scope_external_key elif isinstance(role_assignment.scope, OrgCourseOverviewGlobData): course_access_role_kwargs["org"] = role_assignment.scope.org diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 1e423942..4c3a6687 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -876,7 +876,7 @@ def test_authz_rollback_course_authoring_command(self, mock_rollback): call_command("authz_rollback_course_authoring", "--course-id-list", self.course_id) mock_rollback.assert_called_once() - _, kwargs = mock_rollback.call_args + args, kwargs = mock_rollback.call_args self.assertEqual(kwargs["delete_after_migration"], False) From d369fe009e9c6af6488429d87f9e4f7c86cb9a05 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 8 Apr 2026 10:45:14 +0200 Subject: [PATCH 05/19] test: include test cases for org-level migrations --- openedx_authz/engine/utils.py | 6 ++- openedx_authz/tests/test_migrations.py | 58 ++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index f55ec672..90ead244 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -348,6 +348,7 @@ def migrate_authz_to_legacy_course_roles( for role_assignment in role_assignments: # Per valid role assignment, create corresponding CourseAccessRole entry + # depending on whether the scope is course-level or org-level glob try: user_external_key = role_assignment.subject.external_key role_external_key = role_assignment.roles[0].external_key @@ -365,12 +366,13 @@ def migrate_authz_to_legacy_course_roles( course_access_role_kwargs["org"] = role_assignment.scope.org else: logger.error( - f"Unexpected scope type: {type(role_assignment.scope)} for RoleAssignment with scope: {scope_external_key}" + f"Unexpected scope type: {type(role_assignment.scope)} for RoleAssignment with " + f"scope: {scope_external_key}, user: {user_external_key} and role: {role_external_key}, skipping." ) roles_with_errors.append(role_assignment) continue - course_access_role_model.objects.create(**course_access_role_kwargs) + course_access_role_model.objects.get_or_create(**course_access_role_kwargs) roles_with_no_errors.append(role_assignment) logger.info( diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 4c3a6687..1d47ca38 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -7,6 +7,7 @@ from django.core.management import CommandError, call_command from django.test import TestCase +from openedx_authz.api.data import OrgCourseOverviewGlobData from openedx_authz.api.users import batch_unassign_role_from_users, get_user_role_assignments_in_scope from openedx_authz.constants.roles import ( COURSE_ADMIN, @@ -1114,3 +1115,60 @@ def test_migrate_authz_to_legacy_course_roles_with_library_env(self): self.assertEqual(len(errors), 0) self.assertEqual(len(successes), 12) + + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_org_level_scope_creates_org_glob_assignment(self): + """A CourseAccessRole with org set and blank course_id maps to an OrgCourseOverviewGlobData scope. + + Expected result: + User has a COURSE_ADMIN assignment under the org-level glob scope. + """ + org_short_name_new = f"{OBJECT_PREFIX}org2" + Organization.objects.create(name=f"{OBJECT_PREFIX}org2_full", short_name=org_short_name_new) + user = User.objects.create_user( + username=f"org_user_{OBJECT_PREFIX}", email=f"org_user_{OBJECT_PREFIX}@example.com" + ) + CourseAccessRole.objects.create(user=user, org=org_short_name_new, course_id="", role="instructor") + + _, _ = migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=None, org_id=org_short_name_new, delete_after_migration=True + ) + AuthzEnforcer.get_enforcer().load_policy() + + org_scope = OrgCourseOverviewGlobData.build_external_key(org_short_name_new) + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=org_scope + ) + self.assertEqual(len(assignments), 1) + self.assertEqual(assignments[0].roles[0], COURSE_ADMIN) + + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_rollback_org_level_scope_creates_org_only_course_access_role(self): + """Rollback of an OrgCourseOverviewGlobData assignment recreates a CourseAccessRole with org only. + + Expected result: + The recreated entry has org set and course_id is None (org-wide, not course-specific). + """ + org_short_name_new = f"{OBJECT_PREFIX}org2" + Organization.objects.create(name=f"{OBJECT_PREFIX}org2_full", short_name=org_short_name_new) + user = User.objects.create_user( + username=f"org_user_{OBJECT_PREFIX}", email=f"org_user_{OBJECT_PREFIX}@example.com" + ) + CourseAccessRole.objects.create(user=user, org=org_short_name_new, course_id="", role="instructor") + + migrate_legacy_course_roles_to_authz( + CourseAccessRole, course_id_list=None, org_id=org_short_name_new, delete_after_migration=True + ) + AuthzEnforcer.get_enforcer().load_policy() + + errors, successes = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, course_id_list=None, org_id=org_short_name_new, delete_after_migration=True + ) + + self.assertEqual(len(errors), 0) + self.assertEqual(len(successes), 1) + + recreated = CourseAccessRole.objects.filter(user=user, org=org_short_name_new).first() + self.assertIsNotNone(recreated) + self.assertEqual(recreated.org, org_short_name_new) + self.assertIsNone(recreated.course_id) From e255e0d41c41e07eea46c747f0c2eb99c785fa5a Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 8 Apr 2026 10:57:10 +0200 Subject: [PATCH 06/19] refactor: revert to previous tests that MUST pass with latest changes if compatible --- openedx_authz/tests/test_migrations.py | 127 ++++++++++++++++++++++++- 1 file changed, 126 insertions(+), 1 deletion(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 1d47ca38..fd22fe67 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -441,6 +441,17 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): # 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=course_id_list, org_id=None, delete_after_migration=False ) @@ -475,6 +486,34 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): 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) + .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 @@ -556,6 +595,17 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): # 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=course_id_list, org_id=None, delete_after_migration=True ) @@ -586,13 +636,30 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): 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), 13) + 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_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equivalent(self): @@ -609,6 +676,17 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi # 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 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 @@ -653,13 +731,32 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_no_new_role_equi # 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 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 @@ -713,6 +810,17 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): # 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 ) @@ -743,14 +851,31 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): 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): # Migrate from legacy CourseAccessRole to new Casbin-based model From 9f0a35cba6c4c774aff28e7e69c45da14f445c76 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 8 Apr 2026 11:25:53 +0200 Subject: [PATCH 07/19] refactor: address quality issues --- openedx_authz/engine/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 90ead244..8ef313e4 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -303,8 +303,7 @@ def migrate_authz_to_legacy_course_roles( To build each CourseAccessRole entry, the function needs: - A user: resolved from role assignments in scopes linked to courses. - - A scope: either a CourseOverviewData (course-level) or OrgCourseOverviewGlobData (org-level glob), - filtered by course_id or org_id if provided. + - A scope: a CourseOverviewData or OrgCourseOverviewGlobData instance, optionally filtered by course_id or org_id. - A role: a role external key that maps to a legacy role in COURSE_ROLE_EQUIVALENCES. param course_access_role_model: It should be the CourseAccessRole model. This is passed in because the function From e4b50c39049118e0573629a0606fb66e2047c4c0 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 8 Apr 2026 17:12:56 +0200 Subject: [PATCH 08/19] fix: address quality errors --- openedx_authz/engine/utils.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 8ef313e4..b4fd474e 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -322,9 +322,8 @@ def migrate_authz_to_legacy_course_roles( role_assignments = get_all_role_assignments_per_scope_type(scope_type=CourseOverviewData) # Two cases here: - # 1. If org_id is provided, we filter by org_id which will include both org-level glob scopes and course-level scopes linked to that org - # 2. If only course_id_list is provided, we filter by course_id which will include only course-level scopes linked to those course_ids since - # org-level glob scopes don't have course_id in their scope object + # 1. org_id provided: filter by org — includes org-level glob and course-level scopes for that org. + # 2. only course_id_list provided: filter by course_id — org-level glob scopes are excluded (no course_id). if org_id: role_assignments = [ role_assignment @@ -383,7 +382,7 @@ def migrate_authz_to_legacy_course_roles( if delete_after_migration: unassignments[(role_external_key, scope_external_key)].append(user_external_key) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logger.error( f"Error rolling back RoleAssignment for User: {role_assignment.subject.external_key} " f"in Role: {role_assignment.roles[0].external_key} and Scope: {role_assignment.scope.external_key}: {e}" From 10b7c533b914e30747d4970b2a7af7ad4ea64727 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 8 Apr 2026 17:55:21 +0200 Subject: [PATCH 09/19] refactor: index users to avoid additional queries & improve inline comments --- openedx_authz/engine/utils.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index b4fd474e..ad8d87a6 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -317,8 +317,10 @@ def migrate_authz_to_legacy_course_roles( """ _validate_migration_input(course_id_list, org_id) - # CourseOverviewData and OrgCourseOverviewGlobData share the same namespace, - # so filtering by CourseOverviewData captures both course-level and org-level glob assignments. + # CourseOverviewData and OrgCourseOverviewGlobData share the same NAMESPACE ("course-v1"), + # and get_all_role_assignments_per_scope_type matches by NAMESPACE. Passing CourseOverviewData + # therefore captures both course-level and org-level glob assignments. The exact scope type + # is narrowed per-assignment via isinstance checks in the loop below. role_assignments = get_all_role_assignments_per_scope_type(scope_type=CourseOverviewData) # Two cases here: @@ -343,6 +345,14 @@ def migrate_authz_to_legacy_course_roles( roles_with_no_errors = [] unassignments = defaultdict(list) + user_external_keys = {assignment.subject.external_key for assignment in role_assignments} + users_by_username = { + subject.user.username: subject.user + for subject in user_subject_model.objects.filter( + user__username__in=user_external_keys + ).select_related("user") + } + for role_assignment in role_assignments: # Per valid role assignment, create corresponding CourseAccessRole entry @@ -353,7 +363,7 @@ def migrate_authz_to_legacy_course_roles( scope_external_key = role_assignment.scope.external_key course_access_role_kwargs = { - "user": user_subject_model.objects.get(user__username=user_external_key).user, + "user": users_by_username[user_external_key], "role": COURSE_ROLE_EQUIVALENCES[role_external_key], } @@ -363,6 +373,8 @@ def migrate_authz_to_legacy_course_roles( elif isinstance(role_assignment.scope, OrgCourseOverviewGlobData): course_access_role_kwargs["org"] = role_assignment.scope.org else: + # This would only happen for course roles assigned instance-wide + # which is not yet supported logger.error( f"Unexpected scope type: {type(role_assignment.scope)} for RoleAssignment with " f"scope: {scope_external_key}, user: {user_external_key} and role: {role_external_key}, skipping." From 28791a6ce0b8e03adb0cd860c34d4fc50f1deba2 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 9 Apr 2026 15:11:36 +0200 Subject: [PATCH 10/19] refactor: address PR reviews --- openedx_authz/engine/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index ad8d87a6..57f3ff8b 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -337,8 +337,7 @@ def migrate_authz_to_legacy_course_roles( role_assignments = [ role_assignment for role_assignment in role_assignments - if isinstance(role_assignment.scope, CourseOverviewData) - and role_assignment.scope.external_key in course_id_list + if role_assignment.scope.course_id in course_id_list ] roles_with_errors = [] From 666629f6aa74b1d89bee78fb74ee16bc1a32fa82 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 9 Apr 2026 15:37:44 +0200 Subject: [PATCH 11/19] refactor: use is instance instead of matching per namespace --- openedx_authz/api/roles.py | 11 ++++++----- openedx_authz/engine/utils.py | 8 +++----- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/openedx_authz/api/roles.py b/openedx_authz/api/roles.py index 489fb52c..f56ee8e6 100644 --- a/openedx_authz/api/roles.py +++ b/openedx_authz/api/roles.py @@ -556,20 +556,21 @@ def unassign_subject_from_all_roles(subject: SubjectData) -> bool: return enforcer.remove_filtered_grouping_policy(GroupingPolicyIndex.SUBJECT.value, subject.namespaced_key) -def get_all_role_assignments_per_scope_type(scope_type: type[ScopeData]) -> list[RoleAssignmentData]: - """Get all role assignments for a specific scope type. +def get_all_role_assignments_per_scope_type(scope_types: list[type[ScopeData]]) -> list[RoleAssignmentData]: + """Get all role assignments matching any of the given scope types. Loads all grouping policies from the enforcer and filters in Python. Casbin policies store full scope keys (e.g., 'course-v1^course-v1:Org+Course+Run'), so there is no way to query by scope type directly so the filtering must happen here. Args: - scope_type: A ScopeData subclass (not an instance) used to match by NAMESPACE. + scope_types: A list of ScopeData subclasses (not instances). Assignments matching + any of the given types are returned. Returns: - list[RoleAssignmentData]: All assignments whose scope matches the given scope type. + list[RoleAssignmentData]: All assignments whose scope is an instance of any of the given scope types. """ return [ role_assignment for role_assignment in get_role_assignments() - if role_assignment.scope.NAMESPACE == scope_type.NAMESPACE + if isinstance(role_assignment.scope, tuple(scope_types)) ] diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 57f3ff8b..0bf589a8 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -317,11 +317,9 @@ def migrate_authz_to_legacy_course_roles( """ _validate_migration_input(course_id_list, org_id) - # CourseOverviewData and OrgCourseOverviewGlobData share the same NAMESPACE ("course-v1"), - # and get_all_role_assignments_per_scope_type matches by NAMESPACE. Passing CourseOverviewData - # therefore captures both course-level and org-level glob assignments. The exact scope type - # is narrowed per-assignment via isinstance checks in the loop below. - role_assignments = get_all_role_assignments_per_scope_type(scope_type=CourseOverviewData) + role_assignments = get_all_role_assignments_per_scope_type( + scope_types=[CourseOverviewData, OrgCourseOverviewGlobData] + ) # Two cases here: # 1. org_id provided: filter by org — includes org-level glob and course-level scopes for that org. From 7ad67d049141311f357d1083364b489a223c0820 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 10 Apr 2026 11:37:39 +0200 Subject: [PATCH 12/19] refactor: address PR reviews to inlcude only org/course wide access --- openedx_authz/engine/utils.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 0bf589a8..8d88b901 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -7,6 +7,8 @@ import logging from collections import defaultdict +from django.db.models import Q + from casbin import Enforcer from openedx_authz.api.data import CourseOverviewData, OrgCourseOverviewGlobData @@ -228,7 +230,10 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis course_access_role_filter["course_id__in"] = course_id_list legacy_permissions = ( - course_access_role_model.objects.filter(**course_access_role_filter).select_related("user").all() + course_access_role_model.objects.filter(**course_access_role_filter) + .filter(Q(course_id="") | Q(course_id__startswith=CourseOverviewData.NAMESPACE)) + .select_related("user") + .all() ) # List to keep track of any permissions that could not be migrated From e4f4eeecbb9fccaeeaaf0f3d39c98844465a4e35 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 10 Apr 2026 11:59:09 +0200 Subject: [PATCH 13/19] fix: address quality issues --- openedx_authz/engine/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 8d88b901..debf0b8a 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -7,9 +7,8 @@ import logging from collections import defaultdict -from django.db.models import Q - from casbin import Enforcer +from django.db.models import Q from openedx_authz.api.data import CourseOverviewData, OrgCourseOverviewGlobData from openedx_authz.api.roles import get_all_role_assignments_per_scope_type From df9f70ff3b6004f7084cf82032c79e2743333cb0 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 10 Apr 2026 13:27:07 +0200 Subject: [PATCH 14/19] test: include test case for instance wide cases --- openedx_authz/engine/utils.py | 5 +-- openedx_authz/tests/test_migrations.py | 44 +++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index debf0b8a..0707206f 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -9,6 +9,7 @@ from casbin import Enforcer from django.db.models import Q +from opaque_keys.edx.django.models import CourseKeyField from openedx_authz.api.data import CourseOverviewData, OrgCourseOverviewGlobData from openedx_authz.api.roles import get_all_role_assignments_per_scope_type @@ -230,7 +231,7 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis legacy_permissions = ( course_access_role_model.objects.filter(**course_access_role_filter) - .filter(Q(course_id="") | Q(course_id__startswith=CourseOverviewData.NAMESPACE)) + .filter(Q(course_id=CourseKeyField.Empty) | Q(course_id__startswith=CourseOverviewData.NAMESPACE)) .select_related("user") .all() ) @@ -255,7 +256,7 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis elif permission.org: scope_external_key = OrgCourseOverviewGlobData.build_external_key(permission.org) else: - # This should not happen as either course_id or org should be defined for each permission, log and skip + # Instance-wide roles (no course_id, no org) are not supported by this migration, log and skip. logger.error( f"Permission for User: {permission.user.username} has neither course_id nor org defined, skipping." ) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index fd22fe67..3212666f 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -1,6 +1,6 @@ """Unit Tests for openedx_authz migrations.""" -from unittest.mock import patch +from unittest.mock import MagicMock, patch from django.contrib.auth import get_user_model from django.contrib.auth.models import Group @@ -278,11 +278,12 @@ 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): + def __init__(self, user, role, course_id, id_in, *, org=""): self.user = user self.role = role self.course_id = course_id self.id = id_in + self.org = org class MockUser: """Mock class to simulate User objects for testing the rollback migration.""" @@ -296,7 +297,7 @@ class MockQuerySet: def __init__(self, permissions): self.permissions = permissions - def filter(self, **kwargs): + def filter(self, *args, **kwargs): return self def select_related(self, *args, **kwargs): @@ -1169,6 +1170,41 @@ def test_migrate_legacy_course_roles_to_authz_user_not_added( self.assertEqual(len(successes), 0) self.assertEqual(errors[0].user.username, "testuser") + @patch("openedx_authz.engine.utils.LEGACY_COURSE_ROLE_EQUIVALENCES", {"instructor": "instructor-role"}) + def test_migrate_legacy_course_roles_to_authz_instance_wide_role_is_error(self): + """Instance-wide roles (no course_id and no org) are logged as errors and skipped. + + Expected result: + A CourseAccessRole with both course_id and org blank is logged as an error and + returned in permissions_with_errors, but not migrated. + """ + instance_wide_permission = MagicMock() + instance_wide_permission.user.username = "instance_user" + instance_wide_permission.role = "instructor" + instance_wide_permission.course_id = "" + instance_wide_permission.org = "" + + mock_qs = MagicMock() + mock_qs.filter.return_value = mock_qs + mock_qs.select_related.return_value = mock_qs + mock_qs.all.return_value = [instance_wide_permission] + + mock_model = MagicMock() + mock_model.objects.filter.return_value = mock_qs + + with self.assertLogs("openedx_authz.engine.utils", level="ERROR") as log: + errors, successes = migrate_legacy_course_roles_to_authz( + mock_model, + course_id_list=["course-v1:test"], + org_id=None, + delete_after_migration=False, + ) + + self.assertEqual(len(errors), 1) + self.assertEqual(len(successes), 0) + self.assertEqual(errors[0].user.username, "instance_user") + self.assertTrue(any("instance_user" in msg for msg in log.output)) + @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( @@ -1279,7 +1315,7 @@ def test_rollback_org_level_scope_creates_org_only_course_access_role(self): user = User.objects.create_user( username=f"org_user_{OBJECT_PREFIX}", email=f"org_user_{OBJECT_PREFIX}@example.com" ) - CourseAccessRole.objects.create(user=user, org=org_short_name_new, course_id="", role="instructor") + CourseAccessRole.objects.create(user=user, org=org_short_name_new, role="instructor") migrate_legacy_course_roles_to_authz( CourseAccessRole, course_id_list=None, org_id=org_short_name_new, delete_after_migration=True From 4b08cf14fa2eb3dd0d96ed445b9954a33beff412 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 10 Apr 2026 17:58:50 +0200 Subject: [PATCH 15/19] refactor: address PR reviews --- openedx_authz/engine/utils.py | 3 +- openedx_authz/tests/test_migrations.py | 71 +++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 0707206f..ebf49034 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -340,7 +340,8 @@ def migrate_authz_to_legacy_course_roles( role_assignments = [ role_assignment for role_assignment in role_assignments - if role_assignment.scope.course_id in course_id_list + if isinstance(role_assignment.scope, CourseOverviewData) and + role_assignment.scope.course_id in course_id_list ] roles_with_errors = [] diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 3212666f..c48b1597 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -8,7 +8,11 @@ from django.test import TestCase from openedx_authz.api.data import OrgCourseOverviewGlobData -from openedx_authz.api.users import batch_unassign_role_from_users, get_user_role_assignments_in_scope +from openedx_authz.api.users import ( + assign_role_to_user_in_scope, + batch_unassign_role_from_users, + get_user_role_assignments_in_scope, +) from openedx_authz.constants.roles import ( COURSE_ADMIN, COURSE_DATA_RESEARCHER, @@ -1333,3 +1337,68 @@ def test_rollback_org_level_scope_creates_org_only_course_access_role(self): self.assertIsNotNone(recreated) self.assertEqual(recreated.org, org_short_name_new) self.assertIsNone(recreated.course_id) + + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_org_id_filter_includes_glob_and_excludes_other_orgs(self): + """Rolling back with org_id includes org-level glob scopes and excludes assignments from other orgs. + + Expected result: + The user with a glob scope in self.org is in successes; the user with a course-level + assignment in a different org is not. + """ + glob_scope = f"course-v1:{self.org}+*" + other_org_course_scope = f"course-v1:{OBJECT_PREFIX}filter_org2+FilterCourse+2024" + + user_glob = User.objects.create_user( + username=f"{OBJECT_PREFIX}filter_glob_user", email=f"{OBJECT_PREFIX}filter_glob@example.com" + ) + user_other_org = User.objects.create_user( + username=f"{OBJECT_PREFIX}filter_org2_user", email=f"{OBJECT_PREFIX}filter_org2@example.com" + ) + assign_role_to_user_in_scope(user_glob.username, COURSE_STAFF.external_key, glob_scope) + assign_role_to_user_in_scope(user_other_org.username, COURSE_STAFF.external_key, other_org_course_scope) + AuthzEnforcer.get_enforcer().load_policy() + + errors, successes = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, course_id_list=None, org_id=self.org, delete_after_migration=False + ) + + migrated_users = {assignment.subject.external_key for assignment in successes} + # glob assignment for self.org is included + self.assertIn(user_glob.username, migrated_users) + # assignment from the other org is excluded + self.assertNotIn(user_other_org.username, migrated_users) + self.assertEqual(len(errors), 0) + + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_course_id_list_filter_excludes_glob_and_other_courses(self): + """Rolling back with course_id_list excludes org-level glob scopes and assignments from other courses. + + Expected result: + The user with a glob scope and the user with a course-level assignment not in the list + are both absent from successes. + """ + glob_scope = f"course-v1:{self.org}+*" + other_course_scope = f"course-v1:{self.org}+FilterOtherCourse+2024" + + user_glob = User.objects.create_user( + username=f"{OBJECT_PREFIX}filter_glob_user", email=f"{OBJECT_PREFIX}filter_glob@example.com" + ) + user_other_course = User.objects.create_user( + username=f"{OBJECT_PREFIX}filter_other_course_user", email=f"{OBJECT_PREFIX}filter_other@example.com" + ) + assign_role_to_user_in_scope(user_glob.username, COURSE_STAFF.external_key, glob_scope) + assign_role_to_user_in_scope(user_other_course.username, COURSE_STAFF.external_key, other_course_scope) + AuthzEnforcer.get_enforcer().load_policy() + + errors, successes = migrate_authz_to_legacy_course_roles( + CourseAccessRole, UserSubject, + course_id_list=[self.course_id], org_id=None, delete_after_migration=False + ) + + migrated_users = {assignment.subject.external_key for assignment in successes} + # org-level glob is excluded even though it belongs to the same org + self.assertNotIn(user_glob.username, migrated_users) + # course not in the list is excluded + self.assertNotIn(user_other_course.username, migrated_users) + self.assertEqual(len(errors), 0) From 573ec57398a69d4794611bfabdd31bc88881e126 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 13 Apr 2026 11:33:33 +0200 Subject: [PATCH 16/19] refactor: drop all in tests --- openedx_authz/engine/utils.py | 1 - openedx_authz/tests/test_migrations.py | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index ebf49034..b88761a8 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -233,7 +233,6 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis course_access_role_model.objects.filter(**course_access_role_filter) .filter(Q(course_id=CourseKeyField.Empty) | Q(course_id__startswith=CourseOverviewData.NAMESPACE)) .select_related("user") - .all() ) # List to keep track of any permissions that could not be migrated diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index c48b1597..e9b5357c 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -301,15 +301,15 @@ class MockQuerySet: def __init__(self, permissions): self.permissions = permissions + def __iter__(self): + return iter(self.permissions) + def filter(self, *args, **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") @@ -1191,7 +1191,7 @@ def test_migrate_legacy_course_roles_to_authz_instance_wide_role_is_error(self): mock_qs = MagicMock() mock_qs.filter.return_value = mock_qs mock_qs.select_related.return_value = mock_qs - mock_qs.all.return_value = [instance_wide_permission] + mock_qs.__iter__ = MagicMock(return_value=iter([instance_wide_permission])) mock_model = MagicMock() mock_model.objects.filter.return_value = mock_qs From 8a8d27104224d82d097493d0b8e22441c6d9efba Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 13 Apr 2026 11:41:16 +0200 Subject: [PATCH 17/19] refactor: include tests with lib scopes to ensure filtering happens --- openedx_authz/tests/test_migrations.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index e9b5357c..508ef1dc 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -1344,10 +1344,11 @@ def test_org_id_filter_includes_glob_and_excludes_other_orgs(self): Expected result: The user with a glob scope in self.org is in successes; the user with a course-level - assignment in a different org is not. + assignment in a different org and the user with a library assignment in self.org are not. """ glob_scope = f"course-v1:{self.org}+*" other_org_course_scope = f"course-v1:{OBJECT_PREFIX}filter_org2+FilterCourse+2024" + lib_scope = f"lib:{self.org}:*" user_glob = User.objects.create_user( username=f"{OBJECT_PREFIX}filter_glob_user", email=f"{OBJECT_PREFIX}filter_glob@example.com" @@ -1355,8 +1356,12 @@ def test_org_id_filter_includes_glob_and_excludes_other_orgs(self): user_other_org = User.objects.create_user( username=f"{OBJECT_PREFIX}filter_org2_user", email=f"{OBJECT_PREFIX}filter_org2@example.com" ) + user_lib = User.objects.create_user( + username=f"{OBJECT_PREFIX}filter_lib_user", email=f"{OBJECT_PREFIX}filter_lib@example.com" + ) assign_role_to_user_in_scope(user_glob.username, COURSE_STAFF.external_key, glob_scope) assign_role_to_user_in_scope(user_other_org.username, COURSE_STAFF.external_key, other_org_course_scope) + assign_role_to_user_in_scope(user_lib.username, LIBRARY_ADMIN.external_key, lib_scope) AuthzEnforcer.get_enforcer().load_policy() errors, successes = migrate_authz_to_legacy_course_roles( @@ -1368,6 +1373,8 @@ def test_org_id_filter_includes_glob_and_excludes_other_orgs(self): self.assertIn(user_glob.username, migrated_users) # assignment from the other org is excluded self.assertNotIn(user_other_org.username, migrated_users) + # library assignment in self.org is excluded — library scopes are not course scopes + self.assertNotIn(user_lib.username, migrated_users) self.assertEqual(len(errors), 0) @patch("openedx_authz.api.data.CourseOverview", CourseOverview) @@ -1375,11 +1382,12 @@ def test_course_id_list_filter_excludes_glob_and_other_courses(self): """Rolling back with course_id_list excludes org-level glob scopes and assignments from other courses. Expected result: - The user with a glob scope and the user with a course-level assignment not in the list - are both absent from successes. + The user with a glob scope, the user with a course-level assignment not in the list, + and the user with a library assignment are all absent from successes. """ glob_scope = f"course-v1:{self.org}+*" other_course_scope = f"course-v1:{self.org}+FilterOtherCourse+2024" + lib_scope = f"lib:{self.org}:*" user_glob = User.objects.create_user( username=f"{OBJECT_PREFIX}filter_glob_user", email=f"{OBJECT_PREFIX}filter_glob@example.com" @@ -1387,8 +1395,12 @@ def test_course_id_list_filter_excludes_glob_and_other_courses(self): user_other_course = User.objects.create_user( username=f"{OBJECT_PREFIX}filter_other_course_user", email=f"{OBJECT_PREFIX}filter_other@example.com" ) + user_lib = User.objects.create_user( + username=f"{OBJECT_PREFIX}filter_lib_user", email=f"{OBJECT_PREFIX}filter_lib@example.com" + ) assign_role_to_user_in_scope(user_glob.username, COURSE_STAFF.external_key, glob_scope) assign_role_to_user_in_scope(user_other_course.username, COURSE_STAFF.external_key, other_course_scope) + assign_role_to_user_in_scope(user_lib.username, LIBRARY_ADMIN.external_key, lib_scope) AuthzEnforcer.get_enforcer().load_policy() errors, successes = migrate_authz_to_legacy_course_roles( @@ -1401,4 +1413,6 @@ def test_course_id_list_filter_excludes_glob_and_other_courses(self): self.assertNotIn(user_glob.username, migrated_users) # course not in the list is excluded self.assertNotIn(user_other_course.username, migrated_users) + # library assignment in self.org is excluded — library scopes are not course scopes + self.assertNotIn(user_lib.username, migrated_users) self.assertEqual(len(errors), 0) From 04f62e0823706c1ae4cc5ff1285fccc0a8cbfd56 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 13 Apr 2026 12:00:05 +0200 Subject: [PATCH 18/19] refactor: address PR reviews --- openedx_authz/api/roles.py | 4 ++-- openedx_authz/engine/utils.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx_authz/api/roles.py b/openedx_authz/api/roles.py index f56ee8e6..809e0ddb 100644 --- a/openedx_authz/api/roles.py +++ b/openedx_authz/api/roles.py @@ -556,7 +556,7 @@ def unassign_subject_from_all_roles(subject: SubjectData) -> bool: return enforcer.remove_filtered_grouping_policy(GroupingPolicyIndex.SUBJECT.value, subject.namespaced_key) -def get_all_role_assignments_per_scope_type(scope_types: list[type[ScopeData]]) -> list[RoleAssignmentData]: +def get_all_role_assignments_per_scope_type(scope_types: tuple[type[ScopeData], ...]) -> list[RoleAssignmentData]: """Get all role assignments matching any of the given scope types. Loads all grouping policies from the enforcer and filters in Python. Casbin policies @@ -572,5 +572,5 @@ def get_all_role_assignments_per_scope_type(scope_types: list[type[ScopeData]]) """ return [ role_assignment for role_assignment in get_role_assignments() - if isinstance(role_assignment.scope, tuple(scope_types)) + if isinstance(role_assignment.scope, scope_types) ] diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index b88761a8..e9091d6f 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -322,7 +322,7 @@ def migrate_authz_to_legacy_course_roles( _validate_migration_input(course_id_list, org_id) role_assignments = get_all_role_assignments_per_scope_type( - scope_types=[CourseOverviewData, OrgCourseOverviewGlobData] + scope_types=(CourseOverviewData, OrgCourseOverviewGlobData,) ) # Two cases here: From 10158acb336880faa423f398f39cb1a9e6dcc7c7 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 14 Apr 2026 14:44:57 +0200 Subject: [PATCH 19/19] chore: update changelog and release --- CHANGELOG.rst | 8 ++++++++ openedx_authz/__init__.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 5cc0060a..084075e1 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,14 @@ Change Log Unreleased ********** +1.6.0 - 2026-04-10 +****************** + +Added +===== + +* Add org-wide support to migration commands for forward and backward migration of course authoring permissions. + 1.5.0 - 2026-04-09 ****************** diff --git a/openedx_authz/__init__.py b/openedx_authz/__init__.py index f729fdf5..192ac7e1 100644 --- a/openedx_authz/__init__.py +++ b/openedx_authz/__init__.py @@ -4,6 +4,6 @@ import os -__version__ = "1.5.0" +__version__ = "1.6.0" ROOT_DIRECTORY = os.path.dirname(os.path.abspath(__file__))