From f0d1772da39e9c3324a5d331743419571927f146 Mon Sep 17 00:00:00 2001 From: Dennis Jen Date: Mon, 27 Jul 2015 11:38:02 -0400 Subject: [PATCH] Optimize OpenID Course Claims for non-global-staff users. --- lms/djangoapps/oauth2_handler/handlers.py | 27 +++++++---- lms/djangoapps/oauth2_handler/tests.py | 58 +++++++++++++++++------ 2 files changed, 60 insertions(+), 25 deletions(-) diff --git a/lms/djangoapps/oauth2_handler/handlers.py b/lms/djangoapps/oauth2_handler/handlers.py index 5989dfb2fe..4d08519fa7 100644 --- a/lms/djangoapps/oauth2_handler/handlers.py +++ b/lms/djangoapps/oauth2_handler/handlers.py @@ -4,12 +4,11 @@ from django.conf import settings from django.core.cache import cache from xmodule.modulestore.django import modulestore -from courseware.access import has_access from openedx.core.djangoapps.user_api.models import UserPreference from student.models import anonymous_id_for_user from student.models import UserProfile from lang_pref import LANGUAGE_KEY -from student.roles import GlobalStaff, CourseStaffRole, CourseInstructorRole +from student.roles import (GlobalStaff, CourseStaffRole, CourseInstructorRole, UserBasedRole) class OpenIDHandler(object): @@ -190,23 +189,31 @@ class CourseAccessHandler(object): return course_ids - # pylint: disable=missing-docstring def _get_courses_with_access_type(self, user, access_type): + """ + If global staff, returns all courses. Otherwise, returns list of courses + based on role access (e.g. courses that course staff has access to). + """ + # Check the application cache and update if not present. The application # cache is useful since there are calls to different endpoints in close # succession, for example the id_token and user_info endpoints. - key = '-'.join([str(self.__class__), str(user.id), access_type]) course_ids = cache.get(key) if not course_ids: - courses = _get_all_courses() - # Global staff have access to all courses. Filter courses for non-global staff. - if not GlobalStaff().has_user(user): - courses = [course for course in courses if has_access(user, access_type, course)] - - course_ids = [unicode(course.id) for course in courses] + if GlobalStaff().has_user(user): + # TODO: This code should be optimized in the future to caching + # the list of all courses in the system. + # The modulestore has all courses, while the roles table only has courses + # with roles. Thus, we'll use the modulestore to fetch all courses. + courses = _get_all_courses() + course_ids = [unicode(course.id) for course in courses] + else: + # Getting courses based on roles avoid querying mongo and thus faster + courses = UserBasedRole(user, access_type).courses_with_role() + course_ids = [unicode(course.course_id) for course in courses] cache.set(key, course_ids, self.COURSE_CACHE_TIMEOUT) diff --git a/lms/djangoapps/oauth2_handler/tests.py b/lms/djangoapps/oauth2_handler/tests.py index d34d24072c..e83d3f4c1a 100644 --- a/lms/djangoapps/oauth2_handler/tests.py +++ b/lms/djangoapps/oauth2_handler/tests.py @@ -5,9 +5,10 @@ from lang_pref import LANGUAGE_KEY from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_TOY_MODULESTORE +from xmodule.modulestore.tests.factories import check_mongo_calls, check_mongo_calls_range from student.models import anonymous_id_for_user from student.models import UserProfile -from student.roles import CourseStaffRole, CourseInstructorRole +from student.roles import CourseStaffRole, CourseInstructorRole, GlobalStaff from student.tests.factories import UserFactory, UserProfileFactory from openedx.core.djangoapps.user_api.preferences.api import set_user_preference from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -77,7 +78,8 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): self.assertEqual(language, locale) def test_no_special_course_access(self): - scopes, claims = self.get_id_token_values('openid course_instructor course_staff') + with check_mongo_calls(0): + scopes, claims = self.get_id_token_values('openid course_instructor course_staff') self.assertNotIn('course_staff', scopes) self.assertNotIn('staff_courses', claims) @@ -86,17 +88,15 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): def test_course_staff_courses(self): CourseStaffRole(self.course_key).add_users(self.user) - - scopes, claims = self.get_id_token_values('openid course_staff') - + with check_mongo_calls(0): + scopes, claims = self.get_id_token_values('openid course_staff') self.assertIn('course_staff', scopes) self.assertNotIn('staff_courses', claims) # should not return courses in id_token def test_course_instructor_courses(self): CourseInstructorRole(self.course_key).add_users(self.user) - - scopes, claims = self.get_id_token_values('openid course_instructor') - + with check_mongo_calls(0): + scopes, claims = self.get_id_token_values('openid course_instructor') self.assertIn('course_instructor', scopes) self.assertNotIn('instructor_courses', claims) # should not return courses in id_token @@ -113,7 +113,8 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): } } - scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims) + with check_mongo_calls(0): + scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims) self.assertIn('course_staff', scopes) self.assertIn('staff_courses', claims) @@ -133,6 +134,11 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): class UserInfoTest(BaseTestMixin, UserInfoTestCase): + def setUp(self): + super(UserInfoTest, self).setUp() + # clear the course ID cache + cache.clear() + def token_for_scope(self, scope): full_scope = 'openid %s' % scope self.set_access_token_scope(full_scope) @@ -158,27 +164,48 @@ class UserInfoTest(BaseTestMixin, UserInfoTestCase): self.assertEqual(result.status_code, 200) return claims + def test_request_global_staff_courses_using_scope(self): + GlobalStaff().add_users(self.user) + with check_mongo_calls_range(min_finds=1): + claims = self.get_with_scope('course_staff') + courses = claims['staff_courses'] + self.assertIn(self.course_id, courses) + self.assertEqual(len(courses), 1) + def test_request_staff_courses_using_scope(self): CourseStaffRole(self.course_key).add_users(self.user) - claims = self.get_with_scope('course_staff') - + with check_mongo_calls(0): + claims = self.get_with_scope('course_staff') courses = claims['staff_courses'] self.assertIn(self.course_id, courses) self.assertEqual(len(courses), 1) def test_request_instructor_courses_using_scope(self): CourseInstructorRole(self.course_key).add_users(self.user) - claims = self.get_with_scope('course_instructor') - + with check_mongo_calls(0): + claims = self.get_with_scope('course_instructor') courses = claims['instructor_courses'] self.assertIn(self.course_id, courses) self.assertEqual(len(courses), 1) + def test_request_global_staff_courses_with_claims(self): + GlobalStaff().add_users(self.user) + + values = [self.course_id, 'some_invalid_course'] + with check_mongo_calls_range(min_finds=1): + claims = self.get_with_claim_value('course_staff', 'staff_courses', values) + self.assertEqual(len(claims), 2) + + courses = claims['staff_courses'] + self.assertIn(self.course_id, courses) + self.assertEqual(len(courses), 1) + def test_request_staff_courses_with_claims(self): CourseStaffRole(self.course_key).add_users(self.user) values = [self.course_id, 'some_invalid_course'] - claims = self.get_with_claim_value('course_staff', 'staff_courses', values) + with check_mongo_calls(0): + claims = self.get_with_claim_value('course_staff', 'staff_courses', values) self.assertEqual(len(claims), 2) courses = claims['staff_courses'] @@ -189,7 +216,8 @@ class UserInfoTest(BaseTestMixin, UserInfoTestCase): CourseInstructorRole(self.course_key).add_users(self.user) values = ['edX/toy/TT_2012_Fall', self.course_id, 'invalid_course_id'] - claims = self.get_with_claim_value('course_instructor', 'instructor_courses', values) + with check_mongo_calls(0): + claims = self.get_with_claim_value('course_instructor', 'instructor_courses', values) self.assertEqual(len(claims), 2) courses = claims['instructor_courses']