Skip to content

E
edx-platform
Commit fac73e88 by Carlos Andrés Rocha
Options

Cache user course privileges during OpenID Connect authorization.

parent dfd00a02
Showing with 109 additions and 54 deletions
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 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):
......
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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment