Skip to content

fix: platform-wide glob role assignments crash get_orgs_for_user/has_org_for_user #38980

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.

Sign up for GitHub

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

Open
efortish wants to merge 7 commits into
base: master
Choose a base branch
from
+72 −4
Open

fix: platform-wide glob role assignments crash get_orgs_for_user/has_org_for_user #38980

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
65b57d9
fix: platform-wide glob role assignments crash get_orgs_for_user/has_…
efortish Aug 12, 2026
526ea04
chore: retrigger CI (flaky JS video-volume-control test)
efortish Aug 12, 2026
8b92fc6
Merge branch 'master' into ks/issue-380-platform-glob-org
efortish Aug 12, 2026
ace8a06
refactor: use build_external_key() instead of hardcoded glob key strings
efortish Aug 14, 2026
d32aa52
Merge remote-tracking branch 'ednx/ks/issue-380-platform-glob-org' in…
efortish Aug 14, 2026
795de56
refactor: use assign_role_to_user_in_scope instead of mocking assignm…
efortish Aug 18, 2026
fbb2726
test: add subset-vs-all-orgs coverage for _authz_get_orgs_for_user
efortish Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions common/djangoapps/student/roles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from openedx_authz.api import users as authz_api
from openedx_authz.api.data import CourseOverviewData, OrgCourseOverviewGlobData, 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
Expand Down Expand Up @@ -632,6 +633,11 @@ def _authz_get_orgs_for_user(self, user) -> list[str]:
user_external_key=user.username,
role_external_key=role,
)
# A platform-wide grant (course-v1:*, lib:*) covers every org, not just the ones
# with a concrete assignment. Platform-glob scopes have no .org attribute at all
# (unlike org-glob/course/library scopes, where it's a real field that can be None).
if any(assignment.scope.IS_PLATFORM_GLOB for assignment in assignments):
return [org["short_name"] for org in get_organizations()]

@mariajgrimaldi mariajgrimaldi Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being late to this review. Does this return the same structure as in L641? Can we make sure of this with a test - like a test case that for a user returns a subset and for another all orgs? Not sure if we're doing that already. Thanks

@efortish efortish Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries about the timing! Yes — both branches return the same shape, a plain list[str] of org short names (L640 is a list comprehension over get_organizations(), L641 is list(a set) of assignment.scope.org values). Added test_get_orgs_for_user_authz_platform_glob_vs_org_scoped: it registers a third org that's never assigned to anyone, then asserts an org-scoped grant returns only its own org while a platform-wide grant returns all three, against that same shared pool of orgs — so the two branches are actually distinguished instead of just coincidentally returning the same numbers (which is what my original platform_glob test alone couldn't rule out, since it only ever registered exactly the orgs it expected back). Verified locally against a real devstack. Fixed in fbb2726.

orgs = {assignment.scope.org for assignment in assignments if assignment.scope.org is not None}
return list(orgs)

Expand Down
29 changes: 29 additions & 0 deletions common/djangoapps/student/tests/test_roles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -313,6 +315,33 @@ def test_get_orgs_for_user_authz(self):
result = role.get_orgs_for_user(self.student)
self.assertCountEqual(result, [self.course_key.org, other_org]) # noqa: PT009

@override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True)
def test_get_orgs_for_user_authz_platform_glob(self):
"""
A platform-wide glob assignment (course-v1:*) has no `.org` attribute, unlike
course/org-glob scopes. get_orgs_for_user must special-case it and return every
registered org instead of crashing with an AttributeError.
"""
role = CourseStaffRole(self.course_key)

for org in self.orgs:
add_organization({"name": org, "short_name": org, "description": ""})

staff_authz_role = RoleData(external_key=COURSE_STAFF)
assignments = [
RoleAssignmentData(
subject=UserData(external_key=self.student.username),
roles=[staff_authz_role],
scope=PlatformCourseOverviewGlobData(external_key="course-v1:*"),

@BryanttV BryanttV Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
scope=PlatformCourseOverviewGlobData(external_key="course-v1:*"),
scope=PlatformCourseOverviewGlobData.build_external_key(),

And could we please update test_course_listing.py module to use the build_external_key method? I forgot to do that in some of the tests.

@efortish efortish Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied — wrapped it in PlatformCourseOverviewGlobData(...) since the literal suggestion would assign a bare string to scope (RoleAssignmentData.scope expects a ScopeData instance). Also updated test_course_listing.py's platform/org-glob tests to use build_external_key() as you asked. Fixed in d32aa52.

@BryanttV BryanttV Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. Now that I'm taking a closer look at the test, instead of mocking the assignments, could we use the assign_role_to_user_in_scope function? Something like this:

assign_role_to_user_in_scope(
    self.student.username,
    COURSE_STAFF.external_key,
    PlatformCourseOverviewGlobData.build_external_key(),
)

@efortish efortish Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea — this is actually stronger, since it exercises the real Casbin assignment + policy load instead of mocking get_user_role_assignments_filtered. It also caught a latent bug in my mock: I had RoleData(external_key=COURSE_STAFF) (the RoleData object itself) instead of COURSE_STAFF.external_key, which the mock silently tolerated since it bypassed the real filtering. Switched to assign_role_to_user_in_scope + load_policy() in 795de56.

),
]

with patch("openedx_authz.api.users.get_user_role_assignments_filtered", return_value=assignments):
result = role.get_orgs_for_user(self.student)
self.assertCountEqual(result, self.orgs) # noqa: PT009
assert role.has_org_for_user(self.student)
assert role.has_org_for_user(self.student, org=self.orgs[0])

def test_get_authz_compat_course_access_roles_for_user(self):
"""
Test that get_authz_compat_course_access_roles_for_user doesn't crash when the user
Expand Down
Loading