Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 26 additions & 13 deletions common/djangoapps/student/roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
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 RoleAssignmentData, CourseOverviewData
from openedx_authz.constants import roles as authz_roles

from common.djangoapps.student.models import CourseAccessRole
Expand Down Expand Up @@ -73,6 +74,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 isinstance(assignment.scope, CourseOverviewData)
]
return filtered_assignments

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the Libraries V2 key parsing as it's not needed, but I've kept the function as I think it improves legibility on the implementation, what do you think?

"""
Get the org from a course key.
"""
parsed_key = CourseKey.from_string(key)
return parsed_key.org

def register_access_role(cls):
"""
Expand Down Expand Up @@ -136,13 +155,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 +843,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 +863,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 +895,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 +905,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
20 changes: 20 additions & 0 deletions common/djangoapps/student/tests/test_roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@


import ddt
from unittest.mock import patch
from django.contrib.auth.models import Permission
from django.test import TestCase
from edx_toggles.toggles.testutils import override_waffle_flag
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locator import LibraryLocator

from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
from openedx_authz.api.data import ContentLibraryData, RoleAssignmentData, RoleData, UserData
from openedx_authz.engine.enforcer import AuthzEnforcer

from common.djangoapps.student.admin import CourseAccessRoleHistoryAdmin
Expand All @@ -32,6 +34,7 @@
OrgInstructorRole,
OrgStaffRole,
RoleCache,
get_authz_compat_course_access_roles_for_user,
get_role_cache_key_for_course,
ROLE_CACHE_UNGROUPED_ROLES__KEY
)
Expand Down Expand Up @@ -236,6 +239,23 @@ def test_get_orgs_for_user(self):
role_second_org.add_users(self.student)
assert len(role.get_orgs_for_user(self.student)) == 2

def test_get_authz_compat_course_access_roles_for_user(self):
"""
Thest that get_authz_compat_course_access_roles_for_user doesn't crash when the user
has Libraries V2 or other non-course roles in their assignments.
"""
lib_assignment = RoleAssignmentData(
subject=UserData(external_key=self.student.username),
roles=[RoleData(external_key='test-role')],
scope=ContentLibraryData(external_key='lib:edX:test-lib'),
)
with patch(
'openedx_authz.api.users.get_subject_role_assignments',
return_value=[lib_assignment],
):
result = get_authz_compat_course_access_roles_for_user(self.student)
assert result == set()


@ddt.ddt
class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-class-docstring
Expand Down
Loading