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.