-
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 1 commit
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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="course-v1:*"), | ||||||
| ) | ||||||
| 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="course-v1:*"), | ||||||
|
Contributor
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.
Suggested change
Contributor
Author
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. Applied — wrapped it in |
||||||
| ) | ||||||
| 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) | ||||||
|
|
||||||
| for org in self.orgs: | ||||||
| self.assertTrue(self.student._roles.has_role("instructor", None, org)) # noqa: PT009 | ||||||
|
Contributor
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. Could we use a Python
Suggested change
The same applies to the other tests. To avoid adding the
Contributor
Author
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. Good call, done — switched to plain |
||||||
| self.assertTrue(OrgInstructorRole(org).has_user(self.student)) # noqa: PT009 | ||||||
| self.assertTrue(self.student.has_perm(instructor_permissions.VIEW_DASHBOARD, marvel_course_key)) # noqa: PT009 | ||||||
| self.assertTrue(self.student.has_perm(instructor_permissions.VIEW_DASHBOARD, dc_course_key)) # noqa: PT009 | ||||||
|
|
||||||
|
|
||||||
| @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.
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.
Same as above — wrapped in
PlatformCourseOverviewGlobData(...)to keepscopea ScopeData instance. Fixed in 793f2ca.