From 72e6b8aaeb92f0af2d6ff2e8e12af8701fbde20b Mon Sep 17 00:00:00 2001 From: Dennis Jen Date: Tue, 11 Aug 2015 17:06:22 -0400 Subject: [PATCH] Revert "Optimize OpenID Course Claims for non-global-staff users." This reverts commit f0d1772da39e9c3324a5d331743419571927f146. --- lms/djangoapps/oauth2_handler/handlers.py | 27 ++++------- lms/djangoapps/oauth2_handler/tests.py | 58 ++++++----------------- 2 files changed, 25 insertions(+), 60 deletions(-) diff --git a/lms/djangoapps/oauth2_handler/handlers.py b/lms/djangoapps/oauth2_handler/handlers.py index 4d08519fa7..5989dfb2fe 100644 --- a/lms/djangoapps/oauth2_handler/handlers.py +++ b/lms/djangoapps/oauth2_handler/handlers.py @@ -4,11 +4,12 @@ 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, UserBasedRole) +from student.roles import GlobalStaff, CourseStaffRole, CourseInstructorRole class OpenIDHandler(object): @@ -189,31 +190,23 @@ 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() - 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] + # 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] 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 e83d3f4c1a..d34d24072c 100644 --- a/lms/djangoapps/oauth2_handler/tests.py +++ b/lms/djangoapps/oauth2_handler/tests.py @@ -5,10 +5,9 @@ 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, GlobalStaff +from student.roles import CourseStaffRole, CourseInstructorRole 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 @@ -78,8 +77,7 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): self.assertEqual(language, locale) def test_no_special_course_access(self): - with check_mongo_calls(0): - scopes, claims = self.get_id_token_values('openid course_instructor course_staff') + scopes, claims = self.get_id_token_values('openid course_instructor course_staff') self.assertNotIn('course_staff', scopes) self.assertNotIn('staff_courses', claims) @@ -88,15 +86,17 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): def test_course_staff_courses(self): CourseStaffRole(self.course_key).add_users(self.user) - with check_mongo_calls(0): - scopes, claims = self.get_id_token_values('openid course_staff') + + 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) - with check_mongo_calls(0): - scopes, claims = self.get_id_token_values('openid course_instructor') + + 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,8 +113,7 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): } } - with check_mongo_calls(0): - scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims) + scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims) self.assertIn('course_staff', scopes) self.assertIn('staff_courses', claims) @@ -134,11 +133,6 @@ 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) @@ -164,48 +158,27 @@ 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) - with check_mongo_calls(0): - claims = self.get_with_scope('course_staff') + 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) - with check_mongo_calls(0): - claims = self.get_with_scope('course_instructor') + 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'] - with check_mongo_calls(0): - claims = self.get_with_claim_value('course_staff', 'staff_courses', values) + claims = self.get_with_claim_value('course_staff', 'staff_courses', values) self.assertEqual(len(claims), 2) courses = claims['staff_courses'] @@ -216,8 +189,7 @@ class UserInfoTest(BaseTestMixin, UserInfoTestCase): CourseInstructorRole(self.course_key).add_users(self.user) values = ['edX/toy/TT_2012_Fall', self.course_id, 'invalid_course_id'] - with check_mongo_calls(0): - claims = self.get_with_claim_value('course_instructor', 'instructor_courses', values) + claims = self.get_with_claim_value('course_instructor', 'instructor_courses', values) self.assertEqual(len(claims), 2) courses = claims['instructor_courses']