Merge pull request #5749 from rocha/cache-oauth2-handler-course-values

Cache user course privileges during OpenID Connect authorization.

lms/djangoapps/oauth2_handler/handlers.py

 """ 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.
+        Find all courses for which the user has the specified access type. If
+        `values` is specified, check only the courses from `values`.
 
-        The field `data` follows the handler specification in:
+        """
 
-            `oauth2_provider/oidc/handlers.py`
+        # 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.
 
-        """
+        key = (user.id, access_type)
+        if key in self._course_cache:
+            course_ids = self._course_cache[key]
+        else:
+            course_ids = self._get_courses_with_access_type(user, access_type)
+            self._course_cache[key] = course_ids
 
-        user = data['user']
-        values = set(data.get('values', []))
+        # If values was specified, filter out other courses.
+        if values is not None:
+            course_ids = list(set(course_ids) & set(values))
 
-        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)
+        return course_ids
 
-        # If values was provided, return only the requested authorized courses
-        if values:
-            return [c for c in course_ids if c in values]
-        else:
-            return [c for c in 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.
 
-class IDTokenHandler(OpenIDHandler, ProfileHandler, CourseAccessHandler):
-    """
-    Configure the ID Token handler for the LMS.
+        key = '-'.join([str(self.__class__), str(user.id), access_type])
+        course_ids = cache.get(key)
 
-    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.
+        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. """
     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

lms/djangoapps/oauth2_handler/tests.py

 # pylint: disable=missing-docstring
+from django.core.cache import cache
 from django.test.utils import override_settings
 
 from courseware.tests.tests import TEST_DATA_MIXED_MODULESTORE
@@ -35,8 +36,14 @@ class BaseTestMixin(ModuleStoreTestCase):
 
 
 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']
@@ -45,7 +52,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)
@@ -55,14 +62,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)
 
@@ -70,8 +77,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)
 
@@ -81,7 +87,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
@@ -89,11 +95,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):

requirements/edx/github.txt

@@ -32,5 +32,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