From d43ffd3a5683b55be901e4f5d0888ad3988531c9 Mon Sep 17 00:00:00 2001 From: Phil McGachey Date: Wed, 20 May 2015 08:44:58 -0400 Subject: [PATCH 1/2] [LTI Provider] Refactoring and clean-up --- .../0001_create_lti_consumer_model.py | 37 ++++++++++++++++ .../lti_provider/migrations/__init__.py | 0 lms/djangoapps/lti_provider/models.py | 12 +++++- .../lti_provider/signature_validator.py | 4 +- .../tests/test_signature_validator.py | 4 +- .../lti_provider/tests/test_views.py | 37 +++++++--------- lms/djangoapps/lti_provider/views.py | 43 ++++++++++++++----- 7 files changed, 100 insertions(+), 37 deletions(-) create mode 100644 lms/djangoapps/lti_provider/migrations/0001_create_lti_consumer_model.py create mode 100644 lms/djangoapps/lti_provider/migrations/__init__.py diff --git a/lms/djangoapps/lti_provider/migrations/0001_create_lti_consumer_model.py b/lms/djangoapps/lti_provider/migrations/0001_create_lti_consumer_model.py new file mode 100644 index 000000000000..0ad7ce6c6ef6 --- /dev/null +++ b/lms/djangoapps/lti_provider/migrations/0001_create_lti_consumer_model.py @@ -0,0 +1,37 @@ +# -*- coding: utf-8 -*- +# pylint: disable=invalid-name, missing-docstring, unused-argument, unused-import, line-too-long +from south.utils import datetime_utils as datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding model 'LtiConsumer' + db.create_table('lti_provider_lticonsumer', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('consumer_name', self.gf('django.db.models.fields.CharField')(max_length=255)), + ('consumer_key', self.gf('django.db.models.fields.CharField')(unique=True, max_length=32, db_index=True)), + ('consumer_secret', self.gf('django.db.models.fields.CharField')(unique=True, max_length=32)), + )) + db.send_create_signal('lti_provider', ['LtiConsumer']) + + + def backwards(self, orm): + # Deleting model 'LtiConsumer' + db.delete_table('lti_provider_lticonsumer') + + + models = { + 'lti_provider.lticonsumer': { + 'Meta': {'object_name': 'LtiConsumer'}, + 'consumer_key': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32', 'db_index': 'True'}), + 'consumer_name': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'consumer_secret': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + } + } + + complete_apps = ['lti_provider'] \ No newline at end of file diff --git a/lms/djangoapps/lti_provider/migrations/__init__.py b/lms/djangoapps/lti_provider/migrations/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py index 8b0e92965968..50eec8f25449 100644 --- a/lms/djangoapps/lti_provider/models.py +++ b/lms/djangoapps/lti_provider/models.py @@ -1,5 +1,12 @@ """ Database models for the LTI provider feature. + +This app uses migrations. If you make changes to this model, be sure to create +an appropriate migration file and check it in at the same time as your model +changes. To do that, + +1. Go to the edx-platform dir +2. ./manage.py lms schemamigration lti_provider --auto "description" --settings=devstack """ from django.db import models from django.dispatch import receiver @@ -13,8 +20,9 @@ class LtiConsumer(models.Model): specific settings, such as the OAuth key/secret pair and any LTI fields that must be persisted. """ - key = models.CharField(max_length=32, unique=True, db_index=True) - secret = models.CharField(max_length=32, unique=True) + consumer_name = models.CharField(max_length=255) + consumer_key = models.CharField(max_length=32, unique=True, db_index=True) + consumer_secret = models.CharField(max_length=32, unique=True) @receiver(SCORE_CHANGED) diff --git a/lms/djangoapps/lti_provider/signature_validator.py b/lms/djangoapps/lti_provider/signature_validator.py index 7376666cb9e9..59207ea9e0c6 100644 --- a/lms/djangoapps/lti_provider/signature_validator.py +++ b/lms/djangoapps/lti_provider/signature_validator.py @@ -79,7 +79,7 @@ def validate_client_key(self, client_key, request): :return: True if the key is valid, False if it is not. """ - return LtiConsumer.objects.filter(key=client_key).count() == 1 + return LtiConsumer.objects.filter(consumer_key=client_key).count() == 1 def get_client_secret(self, client_key, request): """ @@ -90,7 +90,7 @@ def get_client_secret(self, client_key, request): present, or None if the key does not exist in the database. """ try: - return LtiConsumer.objects.get(key=client_key).secret + return LtiConsumer.objects.get(consumer_key=client_key).consumer_secret except ObjectDoesNotExist: return None diff --git a/lms/djangoapps/lti_provider/tests/test_signature_validator.py b/lms/djangoapps/lti_provider/tests/test_signature_validator.py index 9b19c6896f83..4e0e9c03bfc8 100644 --- a/lms/djangoapps/lti_provider/tests/test_signature_validator.py +++ b/lms/djangoapps/lti_provider/tests/test_signature_validator.py @@ -78,7 +78,7 @@ def test_validate_existing_key(self): Verify that validate_client_key succeeds if the client key exists in the database """ - LtiConsumer.objects.create(key='client_key', secret='client_secret') + LtiConsumer.objects.create(consumer_key='client_key', consumer_secret='client_secret') self.assertTrue(SignatureValidator().validate_client_key('client_key', None)) def test_validate_missing_key(self): @@ -93,7 +93,7 @@ def test_get_existing_client_secret(self): Verify that get_client_secret returns the right value if the key is in the database """ - LtiConsumer.objects.create(key='client_key', secret='client_secret') + LtiConsumer.objects.create(consumer_key='client_key', consumer_secret='client_secret') secret = SignatureValidator().get_client_secret('client_key', None) self.assertEqual(secret, 'client_secret') diff --git a/lms/djangoapps/lti_provider/tests/test_views.py b/lms/djangoapps/lti_provider/tests/test_views.py index 10c622bd2866..add8b13e146e 100644 --- a/lms/djangoapps/lti_provider/tests/test_views.py +++ b/lms/djangoapps/lti_provider/tests/test_views.py @@ -8,6 +8,7 @@ from lti_provider import views from lti_provider.signature_validator import SignatureValidator +from opaque_keys.edx.keys import CourseKey, UsageKey from student.tests.factories import UserFactory @@ -22,10 +23,11 @@ 'oauth_nonce': u'OAuth Nonce', } - +COURSE_KEY = CourseKey.from_string('some/course/id') +USAGE_KEY = UsageKey.from_string('i4x://some/course/problem/uuid').map_into_course(COURSE_KEY) COURSE_PARAMS = { - 'course_id': 'CourseID', - 'usage_id': 'UsageID' + 'course_key': COURSE_KEY, + 'usage_key': USAGE_KEY } @@ -72,7 +74,7 @@ def test_valid_launch(self, render): Verifies that the LTI launch succeeds when passed a valid request. """ request = build_launch_request() - views.lti_launch(request, COURSE_PARAMS['course_id'], COURSE_PARAMS['usage_id']) + views.lti_launch(request, str(COURSE_PARAMS['course_key']), str(COURSE_PARAMS['usage_key'])) render.assert_called_with(request, ALL_PARAMS) def launch_with_missing_parameter(self, missing_param): @@ -112,10 +114,10 @@ def test_session_contents_after_launch(self, _run): properly stored in the session """ request = build_launch_request() - views.lti_launch(request, COURSE_PARAMS['course_id'], COURSE_PARAMS['usage_id']) + views.lti_launch(request, str(COURSE_PARAMS['course_key']), str(COURSE_PARAMS['usage_key'])) session = request.session[views.LTI_SESSION_KEY] - self.assertEqual(session['course_id'], 'CourseID', 'Course ID not set in the session') - self.assertEqual(session['usage_id'], 'UsageID', 'Usage ID not set in the session') + self.assertEqual(session['course_key'], COURSE_KEY, 'Course key not set in the session') + self.assertEqual(session['usage_key'], USAGE_KEY, 'Usage key not set in the session') for key in views.REQUIRED_PARAMETERS: self.assertEqual(session[key], request.POST[key], key + ' not set in the session') @@ -126,7 +128,9 @@ def test_redirect_for_non_authenticated_user(self): URL """ request = build_launch_request(False) - response = views.lti_launch(request, None, None) + response = views.lti_launch( + request, str(COURSE_PARAMS['course_key']), str(COURSE_PARAMS['usage_key']) + ) self.assertEqual(response.status_code, 302) self.assertEqual(response['Location'], '/accounts/login?next=/lti_provider/lti_run') @@ -170,7 +174,7 @@ def test_forbidden_if_session_incomplete(self): Verifies that the lti_run view returns a Forbidden status if the session is missing any of the required LTI parameters or course information. """ - extra_keys = ['course_id', 'usage_id'] + extra_keys = ['course_key', 'usage_key'] for key in views.REQUIRED_PARAMETERS + extra_keys: request = build_run_request() del request.session[views.LTI_SESSION_KEY][key] @@ -208,7 +212,6 @@ def setUp(self): self.module_mock = self.setup_patch('lti_provider.views.get_module_by_usage_id', (self.module_instance, None)) self.access_mock = self.setup_patch('lti_provider.views.has_access', 'StaffAccess') self.course_mock = self.setup_patch('lti_provider.views.get_course_with_access', 'CourseWithAccess') - self.key_mock = self.setup_patch('lti_provider.views.CourseKey.from_string', 'CourseKey') def setup_patch(self, function_name, return_value): """ @@ -228,21 +231,13 @@ def test_valid_launch(self): response = views.render_courseware(request, ALL_PARAMS.copy()) self.assertEqual(response, 'Rendered page') - def test_course_key(self): - """ - Verify that the correct course key is requested - """ - request = build_run_request() - views.render_courseware(request, ALL_PARAMS.copy()) - self.key_mock.assert_called_with(ALL_PARAMS['course_id']) - def test_course_with_access(self): """ Verify that get_course_with_access is called with the right parameters """ request = build_run_request() views.render_courseware(request, ALL_PARAMS.copy()) - self.course_mock.assert_called_with(request.user, 'load', 'CourseKey') + self.course_mock.assert_called_with(request.user, 'load', COURSE_KEY) def test_has_access(self): """ @@ -258,7 +253,7 @@ def test_get_module(self): """ request = build_run_request() views.render_courseware(request, ALL_PARAMS.copy()) - self.module_mock.assert_called_with(request, ALL_PARAMS['course_id'], ALL_PARAMS['usage_id']) + self.module_mock.assert_called_with(request, str(ALL_PARAMS['course_key']), str(ALL_PARAMS['usage_key'])) def test_render(self): """ @@ -278,7 +273,7 @@ def test_context(self): 'disable_footer': True, 'disable_tabs': True, 'staff_access': 'StaffAccess', - 'xqa_server': 'http://your_xqa_server.com', + 'xqa_server': 'http://example.com/xqa', } request = build_run_request() views.render_courseware(request, ALL_PARAMS.copy()) diff --git a/lms/djangoapps/lti_provider/views.py b/lms/djangoapps/lti_provider/views.py index cf49289af79f..bd7d73600c0c 100644 --- a/lms/djangoapps/lti_provider/views.py +++ b/lms/djangoapps/lti_provider/views.py @@ -6,7 +6,7 @@ from django.contrib.auth.decorators import login_required from django.contrib.auth.views import redirect_to_login from django.core.urlresolvers import reverse -from django.http import HttpResponseBadRequest, HttpResponseForbidden +from django.http import HttpResponseBadRequest, HttpResponseForbidden, Http404 from django.views.decorators.csrf import csrf_exempt from courseware.access import has_access @@ -14,7 +14,9 @@ from courseware.module_render import get_module_by_usage_id from edxmako.shortcuts import render_to_response from lti_provider.signature_validator import SignatureValidator -from opaque_keys.edx.keys import CourseKey +from lms_xblock.runtime import unquote_slashes +from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys import InvalidKeyError # LTI launch parameters that must be present for a successful launch REQUIRED_PARAMETERS = [ @@ -64,8 +66,11 @@ def lti_launch(request, course_id, usage_id): # Store the course, and usage ID in the session to prevent privilege # escalation if a staff member in one course tries to access material in # another. - params['course_id'] = course_id - params['usage_id'] = usage_id + course_key, usage_key = parse_course_and_usage_keys(course_id, usage_id) + if not course_key: + raise Http404('Invalid course or usage key') + params['course_key'] = course_key + params['usage_key'] = usage_key request.session[LTI_SESSION_KEY] = params if not request.user.is_authenticated(): @@ -140,7 +145,7 @@ def restore_params_from_session(request): if LTI_SESSION_KEY not in request.session: return None session_params = request.session[LTI_SESSION_KEY] - additional_params = ['course_id', 'usage_id'] + additional_params = ['course_key', 'usage_key'] return get_required_parameters(session_params, additional_params) @@ -154,13 +159,12 @@ def render_courseware(request, lti_params): :return: an HttpResponse object that contains the template and necessary context to render the courseware. """ - usage_id = lti_params['usage_id'] - course_id = lti_params['course_id'] - course_key = CourseKey.from_string(course_id) + usage_key = lti_params['usage_key'] + course_key = lti_params['course_key'] user = request.user course = get_course_with_access(user, 'load', course_key) staff = has_access(request.user, 'staff', course) - instance, _ = get_module_by_usage_id(request, course_id, usage_id) + instance, _ = get_module_by_usage_id(request, str(course_key), str(usage_key)) fragment = instance.render('student_view', context=request.GET) @@ -173,7 +177,26 @@ def render_courseware(request, lti_params): 'disable_footer': True, 'disable_tabs': True, 'staff_access': staff, - 'xqa_server': settings.FEATURES.get('XQA_SERVER', 'http://your_xqa_server.com'), + 'xqa_server': settings.FEATURES.get('XQA_SERVER', 'http://example.com/xqa'), } return render_to_response('courseware/courseware.html', context) + + +def parse_course_and_usage_keys(course_id, usage_id): + """ + Convert course and usage ID strings into key objects. Return a tuple of + (course_key, usage_key), or (None, None) if the translation fails. + """ + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError: + return None, None + if not course_key: + return None, None + try: + usage_id = unquote_slashes(usage_id) + usage_key = UsageKey.from_string(usage_id).map_into_course(course_key) + except InvalidKeyError: + return None, None + return course_key, usage_key From 901acda946b53c060c29c6669f7aade151f9a1de Mon Sep 17 00:00:00 2001 From: Phil McGachey Date: Fri, 22 May 2015 09:53:45 -0400 Subject: [PATCH 2/2] [LTI Provider] WIP: Basic user authentication --- .../migrations/0002_create_lti_user_model.py | 43 +++++++++++++++++++ lms/djangoapps/lti_provider/models.py | 5 +++ lms/djangoapps/lti_provider/users.py | 42 ++++++++++++++++++ lms/djangoapps/lti_provider/views.py | 23 +++++++--- 4 files changed, 108 insertions(+), 5 deletions(-) create mode 100644 lms/djangoapps/lti_provider/migrations/0002_create_lti_user_model.py create mode 100644 lms/djangoapps/lti_provider/users.py diff --git a/lms/djangoapps/lti_provider/migrations/0002_create_lti_user_model.py b/lms/djangoapps/lti_provider/migrations/0002_create_lti_user_model.py new file mode 100644 index 000000000000..c111f80f4bbf --- /dev/null +++ b/lms/djangoapps/lti_provider/migrations/0002_create_lti_user_model.py @@ -0,0 +1,43 @@ +# -*- coding: utf-8 -*- +from south.utils import datetime_utils as datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding model 'LtiUser' + db.create_table('lti_provider_ltiuser', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('lti_user_id', self.gf('django.db.models.fields.CharField')(unique=True, max_length=255, db_index=True)), + ('edx_user_id', self.gf('django.db.models.fields.CharField')(unique=True, max_length=30)), + ('edx_password', self.gf('django.db.models.fields.CharField')(max_length=255)), + )) + db.send_create_signal('lti_provider', ['LtiUser']) + + + def backwards(self, orm): + # Deleting model 'LtiUser' + db.delete_table('lti_provider_ltiuser') + + + models = { + 'lti_provider.lticonsumer': { + 'Meta': {'object_name': 'LtiConsumer'}, + 'consumer_key': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32', 'db_index': 'True'}), + 'consumer_name': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'consumer_secret': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'lti_provider.ltiuser': { + 'Meta': {'object_name': 'LtiUser'}, + 'edx_password': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'edx_user_id': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'lti_user_id': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255', 'db_index': 'True'}) + } + } + + complete_apps = ['lti_provider'] \ No newline at end of file diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py index 50eec8f25449..f8e7617f1977 100644 --- a/lms/djangoapps/lti_provider/models.py +++ b/lms/djangoapps/lti_provider/models.py @@ -25,6 +25,11 @@ class LtiConsumer(models.Model): consumer_secret = models.CharField(max_length=32, unique=True) +class LtiUser(models.Model): + lti_user_id = models.CharField(max_length=255, unique=True, db_index=True) + edx_user_id = models.CharField(max_length=30, unique=True) + + @receiver(SCORE_CHANGED) def score_changed_handler(sender, **kwargs): # pylint: disable=unused-argument """ diff --git a/lms/djangoapps/lti_provider/users.py b/lms/djangoapps/lti_provider/users.py new file mode 100644 index 000000000000..7aa3d7ee99b6 --- /dev/null +++ b/lms/djangoapps/lti_provider/users.py @@ -0,0 +1,42 @@ +import string +import random +import uuid + +from django.contrib.auth import authenticate, login, get_backends +from django.contrib.auth.models import User +from lti_provider.models import LtiUser + + +def create_lti_user(params): + lti_user_id = params['user_id'] + edx_user_id = generate_random_edx_username() + edx_password = uuid.uuid4() + + edx_user = User.objects.create_user( + username=edx_user_id, + password=edx_password, + email='{}@lti.example.com'.format(edx_user_id) + ) + edx_user.save() + + lti_user = LtiUser( + lti_user_id=lti_user_id, + edx_user_id=edx_user_id, + ) + lti_user.save() + return lti_user + + +def switch_user(request, lti_user): + user = User.objects.get(username=lti_user.edx_user_id) + user.backend = 'nobody' + if user: + login(request, user) + + +def generate_random_edx_username(): + allowable_chars = string.ascii_letters + string.digits + username = '' + for _i in range(30): + username = username + random.SystemRandom().choice(allowable_chars) + return username diff --git a/lms/djangoapps/lti_provider/views.py b/lms/djangoapps/lti_provider/views.py index bd7d73600c0c..2029b917acac 100644 --- a/lms/djangoapps/lti_provider/views.py +++ b/lms/djangoapps/lti_provider/views.py @@ -4,7 +4,9 @@ from django.conf import settings from django.contrib.auth.decorators import login_required +from django.contrib.auth.models import User from django.contrib.auth.views import redirect_to_login +from django.core.exceptions import ObjectDoesNotExist from django.core.urlresolvers import reverse from django.http import HttpResponseBadRequest, HttpResponseForbidden, Http404 from django.views.decorators.csrf import csrf_exempt @@ -13,8 +15,10 @@ from courseware.courses import get_course_with_access from courseware.module_render import get_module_by_usage_id from edxmako.shortcuts import render_to_response +from lti_provider.models import LtiUser from lti_provider.signature_validator import SignatureValidator from lms_xblock.runtime import unquote_slashes +from lti_provider.users import create_lti_user, lti_to_edx_user_id, switch_user from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys import InvalidKeyError @@ -22,7 +26,7 @@ REQUIRED_PARAMETERS = [ 'roles', 'context_id', 'oauth_version', 'oauth_consumer_key', 'oauth_signature', 'oauth_signature_method', 'oauth_timestamp', - 'oauth_nonce' + 'oauth_nonce', 'user_id' ] LTI_SESSION_KEY = 'lti_provider_parameters' @@ -63,6 +67,19 @@ def lti_launch(request, course_id, usage_id): params = get_required_parameters(request.POST) if not params: return HttpResponseBadRequest() + + try: + lti_user = LtiUser.objects.get(lti_user_id=params['user_id']) + except ObjectDoesNotExist: + # This is the first time that the user has been here. Create an account. + lti_user = create_lti_user(params) + pass + edx_user_id = lti_user.edx_user_id + if not (request.user.is_authenticated() and + request.user.username == edx_user_id): + # The user is logged in as somebody else. Switch them to the LTI user + switch_user(request, lti_user) + # Store the course, and usage ID in the session to prevent privilege # escalation if a staff member in one course tries to access material in # another. @@ -73,10 +90,6 @@ def lti_launch(request, course_id, usage_id): params['usage_key'] = usage_key request.session[LTI_SESSION_KEY] = params - if not request.user.is_authenticated(): - run_url = reverse('lti_provider.views.lti_run') - return redirect_to_login(run_url, settings.LOGIN_URL) - return lti_run(request)