-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix: RoleCache legacy compat layer ignores platform-wide glob role assignments #38984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,8 +15,14 @@ | |
| from opaque_keys.edx.keys import CourseKey | ||
| from opaque_keys.edx.locator import CourseLocator | ||
| from openedx_authz.api import users as authz_api | ||
| from openedx_authz.api.data import CourseOverviewData, OrgCourseOverviewGlobData, RoleAssignmentData | ||
| from openedx_authz.api.data import ( | ||
| CourseOverviewData, | ||
| OrgCourseOverviewGlobData, | ||
| PlatformCourseOverviewGlobData, | ||
| RoleAssignmentData, | ||
| ) | ||
| from openedx_authz.constants import roles as authz_roles | ||
| from organizations.api import get_organizations | ||
|
|
||
| from common.djangoapps.student.models import CourseAccessRole | ||
| from common.djangoapps.student.signals.signals import emit_course_access_role_added, emit_course_access_role_removed | ||
|
|
@@ -159,44 +165,55 @@ class AuthzCompatCourseAccessRole: | |
| role: str | ||
|
|
||
|
|
||
| def _get_org_and_course_id_from_authz_scope( | ||
| scope: CourseOverviewData | OrgCourseOverviewGlobData, | ||
| ) -> tuple[str, str | None] | None: | ||
| def _get_orgs_and_course_ids_from_authz_scope( | ||
| scope: CourseOverviewData | OrgCourseOverviewGlobData | PlatformCourseOverviewGlobData, | ||
| ) -> list[tuple[str, str | None]]: | ||
| """ | ||
| Extract the org and course key from an AuthZ course assignment scope. | ||
| Extract the (org, course_id) pairs an AuthZ course assignment scope maps to. | ||
|
|
||
| Course-scoped assignments return ``(org, course_external_key)``. | ||
| Org-wide assignments return ``(org, None)``. | ||
| Course-scoped assignments map to a single ``(org, course_external_key)`` pair. | ||
| Org-wide assignments map to a single ``(org, None)`` pair. | ||
|
|
||
| Returns ``None`` when the org cannot be determined. For org-wide scopes, | ||
| ``OrgGlobData.org`` is typed as ``str | None`` because it is parsed from | ||
| ``external_key`` and returns ``None`` for malformed glob patterns. | ||
| Platform-wide assignments (``course-v1:*``) apply to every org, not just one, so | ||
| they map to ``(org, None)`` for *every registered org* — the same shape an org-wide | ||
| grant already produces, just repeated per org. This lets a platform-wide grant be | ||
| picked up by the existing OrgRole-based legacy checks (e.g. ``has_staff_roles``, | ||
| ``get_user_permissions``, which already check org-level and course-level access | ||
| separately) with no changes to ``RoleCache``/``OrgRole``/``CourseRole``. | ||
|
|
||
| Returns an empty list when the org cannot be determined (e.g. a malformed org-glob | ||
| external_key, where ``OrgGlobData.org`` is ``None``) or the scope type isn't one of | ||
| the above. | ||
| """ | ||
| if isinstance(scope, CourseOverviewData): | ||
| course_id = scope.external_key | ||
| return get_org_from_key(course_id), course_id | ||
| if isinstance(scope, OrgCourseOverviewGlobData): | ||
| return scope.org, None | ||
| return None | ||
| return [(get_org_from_key(course_id), course_id)] | ||
| if isinstance(scope, PlatformCourseOverviewGlobData): | ||
| return [(org["short_name"], None) for org in get_organizations()] | ||
| if isinstance(scope, OrgCourseOverviewGlobData) and scope.org is not None: | ||
| return [(scope.org, None)] | ||
| return [] | ||
|
|
||
|
|
||
| def authz_get_all_course_assignments_for_user(user: User) -> list[RoleAssignmentData]: | ||
| """ | ||
| Return AuthZ role assignments for a user that apply to courses. | ||
|
|
||
| Includes assignments scoped to a specific course (``CourseOverviewData``) and | ||
| assignments scoped to all courses in an organization (``OrgCourseOverviewGlobData``). | ||
| Assignments for other resource types, such as content libraries, are excluded. | ||
| Includes assignments scoped to a specific course (``CourseOverviewData``), to all | ||
| courses in an organization (``OrgCourseOverviewGlobData``), and to all courses on | ||
| the platform (``PlatformCourseOverviewGlobData``). Assignments for other resource | ||
| types, such as content libraries, are excluded. | ||
|
|
||
| Args: | ||
| user (User): The user whose AuthZ role assignments should be retrieved. | ||
|
|
||
| Returns: | ||
| list[RoleAssignmentData]: Role assignments whose scope is course-level or org-wide | ||
| list[RoleAssignmentData]: Role assignments whose scope is course-level, | ||
| org-wide, or platform-wide. | ||
| """ | ||
| return authz_api.get_user_role_assignments_per_scope_type( | ||
| user_external_key=user.username, | ||
| scope_types=(CourseOverviewData, OrgCourseOverviewGlobData), | ||
| scope_types=(CourseOverviewData, OrgCourseOverviewGlobData, PlatformCourseOverviewGlobData), | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -207,9 +224,10 @@ def _compat_roles_from_authz_assignment( | |
| """ | ||
| Convert an AuthZ role assignment into legacy-compatible course access roles. | ||
|
|
||
| Course-scoped assignments produce roles tied to a specific course key. | ||
| Org-wide assignments produce org-level roles with no course key (``course_id`` | ||
| is ``None``), matching legacy ``OrgStaffRole`` / ``OrgInstructorRole`` behavior. | ||
| Course-scoped assignments produce roles tied to a specific course key. Org-wide | ||
| and platform-wide assignments produce org-level roles with no course key | ||
| (``course_id`` is ``None``), matching legacy ``OrgStaffRole`` / ``OrgInstructorRole`` | ||
| behavior — a platform-wide assignment produces one such role per registered org. | ||
| AuthZ roles without a legacy mapping are skipped. | ||
|
|
||
| Args: | ||
|
|
@@ -222,25 +240,21 @@ def _compat_roles_from_authz_assignment( | |
| assignment. Returns an empty set if the org cannot be determined from | ||
| the scope or no roles could be mapped. | ||
| """ | ||
| org_and_course_id = _get_org_and_course_id_from_authz_scope(assignment.scope) | ||
| if org_and_course_id is None: | ||
| return set() | ||
| org, course_id = org_and_course_id | ||
|
|
||
| compat_roles = set() | ||
| for role in assignment.roles: | ||
| legacy_role = get_legacy_role_from_authz_role(authz_role=role.external_key) | ||
| if legacy_role is None: | ||
| continue | ||
| compat_roles.add( | ||
| AuthzCompatCourseAccessRole( | ||
| user_id=user.id, | ||
| username=user.username, | ||
| org=org, | ||
| course_id=course_id, | ||
| role=legacy_role, | ||
| for org, course_id in _get_orgs_and_course_ids_from_authz_scope(assignment.scope): | ||
| for role in assignment.roles: | ||
| legacy_role = get_legacy_role_from_authz_role(authz_role=role.external_key) | ||
| if legacy_role is None: | ||
| continue | ||
| compat_roles.add( | ||
| AuthzCompatCourseAccessRole( | ||
| user_id=user.id, | ||
| username=user.username, | ||
| org=org, | ||
| course_id=course_id, | ||
| role=legacy_role, | ||
| ) | ||
|
Comment on lines
+244
to
+256
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not pretty sure I follow this change, also I don't understand how this is related to the issue at hand. Is it only a refactor to avoid checking if the return is empty? |
||
| ) | ||
| ) | ||
| return compat_roles | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,13 +14,15 @@ | |
| ContentLibraryData, | ||
| CourseOverviewData, | ||
| OrgCourseOverviewGlobData, | ||
| PlatformCourseOverviewGlobData, | ||
| RoleAssignmentData, | ||
| RoleData, | ||
| ScopeData, | ||
| UserData, | ||
| ) | ||
| from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_STAFF | ||
| from openedx_authz.engine.enforcer import AuthzEnforcer | ||
| from organizations.api import add_organization | ||
|
|
||
| from common.djangoapps.student.admin import CourseAccessRoleHistoryAdmin | ||
| from common.djangoapps.student.models import CourseAccessRoleHistory, User | ||
|
|
@@ -380,6 +382,63 @@ def test_org_scope_authz_role_grants_instructor_dashboard_permissions(self): | |
| self.assertTrue(self.student.has_perm(instructor_permissions.VIEW_DASHBOARD, course_key)) # noqa: PT009 | ||
| self.assertTrue(self.student.has_perm(instructor_permissions.SHOW_TASKS, course_key)) # noqa: PT009 | ||
|
|
||
| def test_get_authz_compat_course_access_roles_for_user_platform_glob(self): | ||
| """ | ||
| A platform-wide (course-v1:*) AuthZ assignment should map to one legacy | ||
| org-level course access role per registered org, since it applies to all of them. | ||
| """ | ||
| for org in self.orgs: | ||
| add_organization({"name": org, "short_name": org, "description": ""}) | ||
|
|
||
| assignment = RoleAssignmentData( | ||
| subject=UserData(external_key=self.student.username), | ||
| roles=[RoleData(external_key=COURSE_ADMIN.external_key)], | ||
| scope=PlatformCourseOverviewGlobData(external_key=PlatformCourseOverviewGlobData.build_external_key()), | ||
| ) | ||
| with patch("openedx_authz.api.users.get_user_role_assignments", return_value=[assignment]): | ||
| result = get_authz_compat_course_access_roles_for_user(self.student) | ||
|
|
||
| self.assertCountEqual( # noqa: PT009 | ||
| result, | ||
| { | ||
| AuthzCompatCourseAccessRole( | ||
| user_id=self.student.id, | ||
| username=self.student.username, | ||
| org=org, | ||
| course_id=None, | ||
| role="instructor", | ||
| ) | ||
| for org in self.orgs | ||
| }, | ||
| ) | ||
|
|
||
| def test_platform_glob_authz_role_grants_instructor_dashboard_permissions(self): | ||
| """ | ||
| A platform-wide (course-v1:*) AuthZ course_admin should grant legacy instructor | ||
| access for courses in *any* org, the same way an org-wide grant does for its org. | ||
| """ | ||
| # pylint: disable=protected-access | ||
| for org in self.orgs: | ||
| add_organization({"name": org, "short_name": org, "description": ""}) | ||
| marvel_course_key = CourseKey.from_string(f"course-v1:{self.orgs[0]}+DemoX+DemoCourse") | ||
| dc_course_key = CourseKey.from_string(f"course-v1:{self.orgs[1]}+DemoX+DemoCourse") | ||
|
|
||
| assignment = RoleAssignmentData( | ||
| subject=UserData(external_key=self.student.username), | ||
| roles=[RoleData(external_key=COURSE_ADMIN.external_key)], | ||
| scope=PlatformCourseOverviewGlobData(external_key=PlatformCourseOverviewGlobData.build_external_key()), | ||
| ) | ||
| with patch("openedx_authz.api.users.get_user_role_assignments", return_value=[assignment]): | ||
| if hasattr(self.student, "_roles"): | ||
| del self.student._roles | ||
| self.student._roles = RoleCache(self.student) | ||
|
Comment on lines
+432
to
+434
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why we're doing this manually, shouldn't we be testing that the cache is working correctly (out-of-the-box) for the PlatformCourseOverviewGlobData? |
||
|
|
||
| for org in self.orgs: | ||
| assert self.student._roles.has_role("instructor", None, org) | ||
| assert OrgInstructorRole(org).has_user(self.student) | ||
| assert self.student.has_perm(instructor_permissions.VIEW_DASHBOARD, marvel_course_key) | ||
| assert self.student.has_perm(instructor_permissions.VIEW_DASHBOARD, dc_course_key) | ||
|
|
||
|
|
||
| @ddt.ddt | ||
| class RoleCacheTestCase(TestCase): # pylint: disable=missing-class-docstring | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't scope.org be always non empty?