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__)) 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/api/roles.py b/openedx_authz/api/roles.py index 1215b79b..809e0ddb 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,23 @@ 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_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 + 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_types: A list of ScopeData subclasses (not instances). Assignments matching + any of the given types are returned. + + Returns: + 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 isinstance(role_assignment.scope, scope_types) + ] diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index e844dcf8..e9091d6f 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -8,13 +8,15 @@ from collections import defaultdict 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 +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, @@ -204,6 +206,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. @@ -212,9 +219,7 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis """ _validate_migration_input(course_id_list, org_id) - course_access_role_filter = { - "course_id__startswith": "course-v1:", - } + course_access_role_filter = {} if org_id: course_access_role_filter["org"] = org_id @@ -225,7 +230,9 @@ 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=CourseKeyField.Empty) | Q(course_id__startswith=CourseOverviewData.NAMESPACE)) + .select_related("user") ) # List to keep track of any permissions that could not be migrated @@ -243,16 +250,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: + # 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." + ) + 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: @@ -286,6 +305,11 @@ 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: 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 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 @@ -297,70 +321,87 @@ 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, - } + 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. + # 2. only course_id_list provided: filter by course_id — org-level glob scopes are excluded (no course_id). 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.course_id 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 - - # 2. Get all role assignments for the user - role_assignments = get_user_role_assignments(user_external_key=user_external_key) + 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 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.") + 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 + scope_external_key = role_assignment.scope.external_key + + course_access_role_kwargs = { + "user": users_by_username[user_external_key], + "role": COURSE_ROLE_EQUIVALENCES[role_external_key], + } + + 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 + 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." + ) + roles_with_errors.append(role_assignment) continue - 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) + course_access_role_model.objects.get_or_create(**course_access_role_kwargs) + roles_with_no_errors.append(role_assignment) + + 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." + ) + + if delete_after_migration: + unassignments[(role_external_key, scope_external_key)].append(user_external_key) + + 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}" + ) + 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..508ef1dc 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -1,13 +1,18 @@ """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 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 +from openedx_authz.api.data import OrgCourseOverviewGlobData +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, @@ -277,11 +282,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.""" @@ -295,15 +301,15 @@ class MockQuerySet: def __init__(self, permissions): self.permissions = permissions - def filter(self, **kwargs): + 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") @@ -1168,6 +1174,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.__iter__ = MagicMock(return_value=iter([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( @@ -1239,3 +1280,139 @@ 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, 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) + + @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 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" + ) + 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( + 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) + # 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) + 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, 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" + ) + 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( + 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) + # 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)