From fac73e88d090b752b505dd56319de63cee29bfcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Andr=C3=A9s=20Rocha?= Date: Mon, 27 Oct 2014 16:52:10 -0400 Subject: [PATCH] Cache user course privileges during OpenID Connect authorization. --- lms/djangoapps/oauth2_handler/handlers.py | 124 +++++++++++++--------- lms/djangoapps/oauth2_handler/tests.py | 43 ++++++-- requirements/edx/github.txt | 2 +- 3 files changed, 112 insertions(+), 57 deletions(-) diff --git a/lms/djangoapps/oauth2_handler/handlers.py b/lms/djangoapps/oauth2_handler/handlers.py index 8ea7a6b54c..60591ec62e 100644 --- a/lms/djangoapps/oauth2_handler/handlers.py +++ b/lms/djangoapps/oauth2_handler/handlers.py @@ -1,6 +1,7 @@ """ Handlers for OpenID Connect provider. """ from django.conf import settings +from django.core.cache import cache from courseware.access import has_access from student.models import anonymous_id_for_user @@ -68,33 +69,34 @@ class CourseAccessHandler(object): valid only if the user is instructor or staff of at least one course. Each new scope has a corresponding claim: `instructor_courses` and - `staff_courses` that lists the course_ids for which the user as instructor + `staff_courses` that lists the course_ids for which the user has instructor or staff privileges. - The claims support claim request values. In other words, if no claim is - requested it returns all the courses for the corresponding privileges. If a - claim request is used, then it only returns the from the list of requested - values that have the corresponding privileges. + The claims support claim request values: if there is no claim request, the + value of the claim is the list all the courses for which the user has the + corresponding privileges. If a claim request is used, then the value of the + claim the list of courses from the requested values that have the + corresponding privileges. For example, if the user is staff of course_a and course_b but not - course_c, the request: + course_c, the claim corresponding to the scope request: scope = openid course_staff - will return: + has the value: {staff_courses: [course_a, course_b] } - If the request is: + For the claim request: - claims = {userinfo: {staff_courses=[course_b, course_d]}} + claims = {userinfo: {staff_courses: {values=[course_b, course_d]}}} - the result will be: + the corresponding claim will have the value: {staff_courses: [course_b] }. - This is useful to quickly determine if a user has the right - privileges for a given course. + This is useful to quickly determine if a user has the right privileges for a + given course. For a description of the function naming and arguments, see: @@ -102,6 +104,11 @@ class CourseAccessHandler(object): """ + COURSE_CACHE_TIMEOUT = getattr(settings, 'OIDC_COURSE_HANDLER_CACHE_TIMEOUT', 60) # In seconds. + + def __init__(self, *_args, **_kwargs): + self._course_cache = {} + def scope_course_instructor(self, data): """ Scope `course_instructor` valid only if the user is an instructor @@ -109,7 +116,10 @@ class CourseAccessHandler(object): """ - course_ids = self._courses_with_access_type(data, 'instructor') + # TODO: unfortunately there is not a faster and still correct way to + # check if a user is instructor of at least one course other than + # checking the access type against all known courses. + course_ids = self.find_courses(data['user'], 'instructor') return ['instructor_courses'] if course_ids else None def scope_course_staff(self, data): @@ -118,8 +128,9 @@ class CourseAccessHandler(object): least one course. """ + # TODO: see :method:CourseAccessHandler.scope_course_instructor + course_ids = self.find_courses(data['user'], 'staff') - course_ids = self._courses_with_access_type(data, 'staff') return ['staff_courses'] if course_ids else None def claim_instructor_courses(self, data): @@ -128,7 +139,8 @@ class CourseAccessHandler(object): user has instructor privileges. """ - return self._courses_with_access_type(data, 'instructor') + + return self.find_courses(data['user'], 'instructor', data.get('values')) def claim_staff_courses(self, data): """ @@ -136,52 +148,66 @@ class CourseAccessHandler(object): has staff privileges. """ - return self._courses_with_access_type(data, 'staff') - def _courses_with_access_type(self, data, access_type): + return self.find_courses(data['user'], 'staff', data.get('values')) + + def find_courses(self, user, access_type, values=None): """ - Utility function to list all courses for a user according to the - access type. - - The field `data` follows the handler specification in: - - `oauth2_provider/oidc/handlers.py` + Find all courses for which the user has the specified access type. If + `values` is specified, check only the courses from `values`. """ - user = data['user'] - values = set(data.get('values', [])) + # Check the instance cache and update if not present. The instance + # cache is useful since there are multiple scope and claims calls in the + # same request. - courses = _get_all_courses() - courses = (c for c in courses if has_access(user, access_type, c)) - course_ids = (unicode(c.id) for c in courses) - - # If values was provided, return only the requested authorized courses - if values: - return [c for c in course_ids if c in values] + key = (user.id, access_type) + if key in self._course_cache: + course_ids = self._course_cache[key] else: - return [c for c in course_ids] + course_ids = self._get_courses_with_access_type(user, access_type) + self._course_cache[key] = course_ids + + # If values was specified, filter out other courses. + if values is not None: + course_ids = list(set(course_ids) & set(values)) + + return course_ids + + # pylint: disable=missing-docstring + def _get_courses_with_access_type(self, user, access_type): + + # 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 endpoins. + + key = '-'.join([str(self.__class__), str(user.id), access_type]) + course_ids = cache.get(key) + + if course_ids is None: + course_ids = [unicode(course.id) for course in _get_all_courses() + if has_access(user, access_type, course)] + cache.set(key, course_ids, self.COURSE_CACHE_TIMEOUT) + + return course_ids class IDTokenHandler(OpenIDHandler, ProfileHandler, CourseAccessHandler): - """ - Configure the ID Token handler for the LMS. - - Note that the values of the claims `instructor_courses` and - `staff_courses` are not included in the ID Token. The rationale is - that for global staff, the list of courses returned could be very - large. Instead they could check for specific courses using the - UserInfo endpoint. - - """ - + """ Configure the ID Token handler for the LMS. """ def claim_instructor_courses(self, data): - # Don't return list of courses in ID Tokens - return None + # Don't return list of courses unless they are requested as essential. + if data.get('essential'): + return super(IDTokenHandler, self).claim_instructor_courses(data) + else: + return None def claim_staff_courses(self, data): - # Don't return list of courses in ID Tokens - return None + # Don't return list of courses unless they are requested as essential. + if data.get('essential'): + return super(IDTokenHandler, self).claim_staff_courses(data) + else: + return None class UserInfoHandler(OpenIDHandler, ProfileHandler, CourseAccessHandler): @@ -194,6 +220,8 @@ def _get_all_courses(): Utitilty function to list all available courses. """ + ms_courses = modulestore().get_courses() courses = [c for c in ms_courses if isinstance(c, CourseDescriptor)] + return courses diff --git a/lms/djangoapps/oauth2_handler/tests.py b/lms/djangoapps/oauth2_handler/tests.py index b358e2648c..989b95dd18 100644 --- a/lms/djangoapps/oauth2_handler/tests.py +++ b/lms/djangoapps/oauth2_handler/tests.py @@ -1,4 +1,5 @@ # pylint: disable=missing-docstring +from django.core.cache import cache from django.test.utils import override_settings from django.test import TestCase @@ -34,8 +35,14 @@ class BaseTestMixin(TestCase): class IDTokenTest(BaseTestMixin, IDTokenTestCase): + def setUp(self): + super(IDTokenTest, self).setUp() + + # CourseAccessHandler uses the application cache. + cache.clear() + def test_sub_claim(self): - scopes, claims = self.get_new_id_token_values('openid') + scopes, claims = self.get_id_token_values('openid') self.assertIn('openid', scopes) sub = claims['sub'] @@ -44,7 +51,7 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): self.assertEqual(sub, expected_sub) def test_user_name_claim(self): - _scopes, claims = self.get_new_id_token_values('openid profile') + _scopes, claims = self.get_id_token_values('openid profile') claim_name = claims['name'] user_profile = UserProfile.objects.get(user=self.user) @@ -54,14 +61,14 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): @override_settings(LANGUAGE_CODE='en') def test_user_without_locale_claim(self): - scopes, claims = self.get_new_id_token_values('openid profile') + scopes, claims = self.get_id_token_values('openid profile') self.assertIn('profile', scopes) self.assertEqual(claims['locale'], 'en') def test_user_with_locale_claim(self): language = 'en' UserPreference.set_preference(self.user, LANGUAGE_KEY, language) - scopes, claims = self.get_new_id_token_values('openid profile') + scopes, claims = self.get_id_token_values('openid profile') self.assertIn('profile', scopes) @@ -69,8 +76,7 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): self.assertEqual(language, locale) def test_no_special_course_access(self): - scopes, claims = self.get_new_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) @@ -80,7 +86,7 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): def test_course_staff_courses(self): CourseStaffRole(self.course_key).add_users(self.user) - scopes, claims = self.get_new_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 @@ -88,11 +94,32 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): def test_course_instructor_courses(self): CourseInstructorRole(self.course_key).add_users(self.user) - scopes, claims = self.get_new_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 + def test_course_staff_courses_with_claims(self): + CourseStaffRole(self.course_key).add_users(self.user) + + course_id = unicode(self.course_key) + nonexistent_course_id = 'some/other/course' + + claims = { + 'staff_courses': { + 'values': [course_id, nonexistent_course_id], + 'essential': True, + } + } + + scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims) + + self.assertIn('course_staff', scopes) + self.assertIn('staff_courses', claims) + self.assertEqual(len(claims['staff_courses']), 1) + self.assertIn(course_id, claims['staff_courses']) + self.assertNotIn(nonexistent_course_id, claims['staff_courses']) + class UserInfoTest(BaseTestMixin, UserInfoTestCase): def token_for_scope(self, scope): diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index db2516943b..3eb71b052b 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -33,5 +33,5 @@ -e git+https://github.com/edx/opaque-keys.git@0.1.2#egg=opaque-keys -e git+https://github.com/edx/ease.git@97de68448e5495385ba043d3091f570a699d5b5f#egg=ease -e git+https://github.com/edx/i18n-tools.git@56f048af9b6868613c14aeae760548834c495011#egg=i18n-tools --e git+https://github.com/edx/edx-oauth2-provider.git@0.2.2#egg=oauth2-provider +-e git+https://github.com/edx/edx-oauth2-provider.git@0.3.1#egg=oauth2-provider -e git+https://github.com/edx/edx-val.git@a3c54afe30375f7a5755ba6f6412a91de23c3b86#egg=edx-val