From 93d8111efc26b8e01057c9110839de60eac3f9b2 Mon Sep 17 00:00:00 2001 From: Kevyn Suarez Date: Fri, 16 Oct 2026 21:10:55 -0500 Subject: [PATCH] fix: UnboundLocalError in BulkRoleCache.prefetch for authz compat roles BulkRoleCache.prefetch has two loops: one over legacy CourseAccessRole rows, which builds and caches a `compat_role` object, and one over openedx-authz compat roles, which iterates with `role` but was adding `compat_role` to the set instead. If the legacy loop never ran for any user, compat_role was unbound and the whole call raised UnboundLocalError. If it did run, the authz loop silently cached the last legacy role instead of the actual authz one. This broke the Gradebook (BulkRoleCache.prefetch is called from bulk_gradebook_view_context) for any course where a listed user had an authz compat role but no legacy CourseAccessRole, and any other caller of prefetch was equally affected. Add it instead of compat_role, and add regression tests covering a user with only an authz role and a mix of legacy/authz roles across different users in the same prefetch call. Fixes openedx-authz#514. --- common/djangoapps/student/roles.py | 2 +- common/djangoapps/student/tests/test_roles.py | 87 +++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index c628e7db3b73..96d4ee132765 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -329,7 +329,7 @@ def prefetch(cls, users): # pylint: disable=missing-function-docstring for role in compat_roles: course_id = get_role_cache_key_for_course(role.course_id) user_roles_set_for_course = roles_by_user[user.id][course_id] - user_roles_set_for_course.add(compat_role) + user_roles_set_for_course.add(role) users_without_roles = [u for u in users if u.id not in roles_by_user] for user in users_without_roles: diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index de576ede58f0..658192b5af41 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -7,6 +7,7 @@ import ddt from django.contrib.auth.models import Permission from django.test import TestCase +from edx_django_utils.cache import RequestCache from edx_toggles.toggles.testutils import override_waffle_flag from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocator @@ -32,6 +33,7 @@ from common.djangoapps.student.roles import ( ROLE_CACHE_UNGROUPED_ROLES__KEY, AuthzCompatCourseAccessRole, + BulkRoleCache, CourseAccessRole, CourseBetaTesterRole, CourseCreatorRole, @@ -618,6 +620,91 @@ def test_role_cache_roles_by_course_id(self): assert roles_dict.get('course-v1:edX+toy2+2013_Fall').pop().course_id.course == 'toy2' +class BulkRoleCachePrefetchTestCase(TestCase): + """ + Regression tests for BulkRoleCache.prefetch mixing legacy CourseAccessRole rows + with openedx-authz compat roles. The authz loop used to add the previous + iteration's `compat_role` (left over from the legacy loop above it) instead of + its own `role`, raising UnboundLocalError when no legacy role existed at all, + and silently caching the wrong role when one did. + + These mock get_authz_compat_course_access_roles_for_user directly rather than + assigning real AuthZ roles, since that goes through the global casbin enforcer, + whose in-memory policy isn't rolled back between tests the way the database is. + + prefetch() also stores its result in a RequestCache, which outside of an actual + request (as here) isn't cleared automatically between tests, so it's cleared in + tearDown to avoid leaking cached roles into unrelated tests that construct a + RoleCache for a user this class never prefetched. + """ + + def setUp(self): + super().setUp() + self.course_key = CourseKey.from_string('course-v1:edX+toy+2012_Fall') + + def tearDown(self): + super().tearDown() + RequestCache(BulkRoleCache.CACHE_NAMESPACE).clear() + + def test_prefetch_with_only_authz_roles_does_not_crash(self): + """ + A user with an authz compat role and no legacy CourseAccessRole entry used to + crash prefetch with UnboundLocalError, since compat_role was never assigned. + """ + user = UserFactory() + authz_role = AuthzCompatCourseAccessRole( + user_id=user.id, + username=user.username, + course_id=str(self.course_key), + org=self.course_key.org, + role='staff', + ) + + with patch( + 'common.djangoapps.student.roles.get_authz_compat_course_access_roles_for_user', + return_value={authz_role}, + ): + BulkRoleCache.prefetch([user]) + + cached_roles = BulkRoleCache.get_user_roles(user) + assert cached_roles[get_role_cache_key_for_course(self.course_key)] == {authz_role} + + def test_prefetch_with_legacy_and_authz_roles_for_different_users(self): + """ + Each user's authz compat role must be cached as itself, not as whichever + legacy CourseAccessRole happened to be the last one scanned by the loop above. + """ + legacy_user = UserFactory() + CourseAccessRole.objects.create( + user=legacy_user, org=self.course_key.org, course_id=self.course_key, role='staff', + ) + + authz_user = UserFactory() + authz_role = AuthzCompatCourseAccessRole( + user_id=authz_user.id, + username=authz_user.username, + course_id=str(self.course_key), + org=self.course_key.org, + role='instructor', + ) + + def fake_authz_roles(user): + return {authz_role} if user.id == authz_user.id else set() + + with patch( + 'common.djangoapps.student.roles.get_authz_compat_course_access_roles_for_user', + side_effect=fake_authz_roles, + ): + BulkRoleCache.prefetch([legacy_user, authz_user]) + + course_cache_key = get_role_cache_key_for_course(self.course_key) + legacy_roles = BulkRoleCache.get_user_roles(legacy_user)[course_cache_key] + authz_roles = BulkRoleCache.get_user_roles(authz_user)[course_cache_key] + + assert {r.role for r in legacy_roles} == {'staff'} + assert authz_roles == {authz_role} + + class CourseAccessRoleHistoryTest(TestCase): """ Tests for the CourseAccessRoleHistory model and associated signals/admin actions.