Skip to content

fix: authz compat layer was failing on libraries v2 keys #38131

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
rodmgwgu wants to merge 3 commits into
base: master
Choose a base branch
from
+28 −14
Open

fix: authz compat layer was failing on libraries v2 keys #38131

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03e26b1
fix: authz compat layer was failing on libraries v2 keys
rodmgwgu Mar 9, 2026
3fd02f5
squash!: Filter only courses
rodmgwgu Mar 9, 2026
2a0515f
squash!: Fix import
rodmgwgu Mar 9, 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
42 changes: 28 additions & 14 deletions common/djangoapps/student/roles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@
from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user
from common.djangoapps.student.signals.signals import emit_course_access_role_added, emit_course_access_role_removed
from opaque_keys.edx.django.models import CourseKeyField
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locator import CourseLocator
from opaque_keys.edx.locator import CourseLocator, LibraryLocatorV2
from openedx_authz.api import users as authz_api
from openedx_authz.api.data import RoleAssignmentData
from openedx_authz.constants import roles as authz_roles

from common.djangoapps.student.models import CourseAccessRole
Expand Down Expand Up @@ -73,6 +75,24 @@ def authz_add_role(user: User, authz_role: str, course_key: str):
legacy_role = get_legacy_role_from_authz_role(authz_role)
emit_course_access_role_added(user, course_locator, course_locator.org, legacy_role)

def authz_get_all_course_assignments_for_user(user: User) -> list[RoleAssignmentData]:
"""
Get all course assignments for a user.
"""
assignments = authz_api.get_user_role_assignments(user_external_key=user.username)
# filter courses only
filtered_assignments = [assignment for assignment in assignments if assignment.scope.NAMESPACE == 'course-v1']
Copy link
Contributor

@wgu-taylor-payne wgu-taylor-payne Mar 10, 2026

Choose a reason for hiding this comment

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

I think I would lean towards checking the class instead. What do you think?

Suggested change
filtered_assignments = [assignment for assignment in assignments if assignment.scope.NAMESPACE == 'course-v1']
filtered_assignments = [assignment for assignment in assignments if isinstance(assignment.scope, CourseOverviewData)]

return filtered_assignments

def get_org_from_key(key: str) -> str:
Copy link
Contributor

@wgu-taylor-payne wgu-taylor-payne Mar 10, 2026

Choose a reason for hiding this comment

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

Since we filter by the course-v1 namespace before calling this function in each case, is it worth keeping?

"""
Get the org from a course or library key.
"""
try:
parsed_key = CourseKey.from_string(key)
except InvalidKeyError:
parsed_key = LibraryLocatorV2.from_string(key)
return parsed_key.org

def register_access_role(cls):
"""
Expand Down Expand Up @@ -136,13 +156,12 @@ def get_authz_compat_course_access_roles_for_user(user: User) -> set[AuthzCompat
Retrieve all CourseAccessRole objects for a given user and convert them to AuthzCompatCourseAccessRole objects.
"""
compat_role_assignments = set()
assignments = authz_api.get_user_role_assignments(user_external_key=user.username)
assignments = authz_get_all_course_assignments_for_user(user)
for assignment in assignments:
for role in assignment.roles:
legacy_role = get_legacy_role_from_authz_role(authz_role=role.external_key)
course_key = assignment.scope.external_key
parsed_key = CourseKey.from_string(course_key)
org = parsed_key.org
org = get_org_from_key(course_key)
compat_role = AuthzCompatCourseAccessRole(
user_id=user.id,
username=user.username,
Expand Down Expand Up @@ -825,9 +844,7 @@ def courses_with_role(self) -> set[AuthzCompatCourseAccessRole]:

# Get all assignments for a user to a role
new_authz_roles = [get_authz_role_from_legacy_role(role) for role in roles]
all_authz_user_assignments = authz_api.get_user_role_assignments(
user_external_key=self.user.username
)
all_authz_user_assignments = authz_get_all_course_assignments_for_user(self.user)

all_assignments = set()

Expand All @@ -847,8 +864,7 @@ def courses_with_role(self) -> set[AuthzCompatCourseAccessRole]:
continue
legacy_role = get_legacy_role_from_authz_role(authz_role=role.external_key)
course_key = assignment.scope.external_key
parsed_key = CourseKey.from_string(course_key)
org = parsed_key.org
org = get_org_from_key(course_key)
all_assignments.add(AuthzCompatCourseAccessRole(
user_id=self.user.id,
username=self.user.username,
Expand Down Expand Up @@ -880,9 +896,7 @@ def has_courses_with_role(self, org: str | None = None) -> bool:

# Then check for authz assignments
new_authz_roles = [get_authz_role_from_legacy_role(role) for role in roles]
all_authz_user_assignments = authz_api.get_user_role_assignments(
user_external_key=self.user.username
)
all_authz_user_assignments = authz_get_all_course_assignments_for_user(self.user)

for assignment in all_authz_user_assignments:
for role in assignment.roles:
Expand All @@ -892,7 +906,7 @@ def has_courses_with_role(self, org: str | None = None) -> bool:
# There is at least one assignment, short circuit
return True
course_key = assignment.scope.external_key
parsed_key = CourseKey.from_string(course_key)
if org == parsed_key.org:
parsed_org = get_org_from_key(course_key)
if org == parsed_org:
return True
return False
Loading