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 0000000000..c3d313afaa --- /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'] diff --git a/lms/djangoapps/lti_provider/migrations/__init__.py b/lms/djangoapps/lti_provider/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py index 8b0e929659..50eec8f254 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 7376666cb9..59207ea9e0 100644 --- a/lms/djangoapps/lti_provider/signature_validator.py +++ b/lms/djangoapps/lti_provider/signature_validator.py @@ -79,7 +79,7 @@ class SignatureValidator(RequestValidator): :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 @@ class SignatureValidator(RequestValidator): 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 9b19c6896f..4e0e9c03bf 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 @@ class SignatureValidatorTest(TestCase): 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 @@ class SignatureValidatorTest(TestCase): 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 10c622bd28..2127807897 100644 --- a/lms/djangoapps/lti_provider/tests/test_views.py +++ b/lms/djangoapps/lti_provider/tests/test_views.py @@ -8,6 +8,7 @@ from mock import patch, MagicMock 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 @@ LTI_DEFAULT_PARAMS = { '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 @@ class LtiLaunchTest(TestCase): 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, unicode(COURSE_KEY), unicode(USAGE_KEY)) render.assert_called_with(request, ALL_PARAMS) def launch_with_missing_parameter(self, missing_param): @@ -112,10 +114,10 @@ class LtiLaunchTest(TestCase): 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, unicode(COURSE_KEY), unicode(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,7 @@ class LtiLaunchTest(TestCase): URL """ request = build_launch_request(False) - response = views.lti_launch(request, None, None) + response = views.lti_launch(request, unicode(COURSE_KEY), unicode(USAGE_KEY)) self.assertEqual(response.status_code, 302) self.assertEqual(response['Location'], '/accounts/login?next=/lti_provider/lti_run') @@ -139,6 +141,7 @@ class LtiLaunchTest(TestCase): request = build_launch_request() response = views.lti_launch(request, None, None) self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 403) class LtiRunTest(TestCase): @@ -170,7 +173,7 @@ class LtiRunTest(TestCase): 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 +211,6 @@ class RenderCoursewareTest(TestCase): 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 +230,13 @@ class RenderCoursewareTest(TestCase): 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 +252,7 @@ class RenderCoursewareTest(TestCase): """ 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, unicode(COURSE_KEY), unicode(USAGE_KEY)) def test_render(self): """ @@ -278,7 +272,7 @@ class RenderCoursewareTest(TestCase): '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 cf49289af7..8e0cb0925c 100644 --- a/lms/djangoapps/lti_provider/views.py +++ b/lms/djangoapps/lti_provider/views.py @@ -6,15 +6,21 @@ from django.conf import settings 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 +import logging from courseware.access import has_access 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.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 + +log = logging.getLogger("edx.lti_provider") + # LTI launch parameters that must be present for a successful launch REQUIRED_PARAMETERS = [ @@ -64,8 +70,18 @@ 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 + try: + course_key, usage_key = parse_course_and_usage_keys(course_id, usage_id) + except InvalidKeyError: + log.error( + 'Invalid course key %s or usage key %s from request %s', + course_id, + usage_id, + request + ) + raise Http404() + params['course_key'] = course_key + params['usage_key'] = usage_key request.session[LTI_SESSION_KEY] = params if not request.user.is_authenticated(): @@ -140,7 +156,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 +170,16 @@ 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, _dummy = get_module_by_usage_id( + request, + unicode(course_key), + unicode(usage_key) + ) fragment = instance.render('student_view', context=request.GET) @@ -173,7 +192,19 @@ 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 throw an InvalidKeyError if the translation + fails. + """ + course_key = CourseKey.from_string(course_id) + usage_id = unquote_slashes(usage_id) + usage_key = UsageKey.from_string(usage_id).map_into_course(course_key) + return course_key, usage_key