From 37cdb89a5e0d21e89516ce0b9ba540c09de0081e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 28 Aug 2015 14:18:42 -0400 Subject: [PATCH 1/3] Allow LtiConsumer.instance_guid to be blank. The instructions say that this should be left blank until the initial launch, but Django Admin kicks that out because blanks are not allowed at the model level. --- lms/djangoapps/lti_provider/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py index 61c024a107..dc5305bfe5 100644 --- a/lms/djangoapps/lti_provider/models.py +++ b/lms/djangoapps/lti_provider/models.py @@ -26,7 +26,7 @@ class LtiConsumer(models.Model): consumer_name = models.CharField(max_length=255, unique=True) consumer_key = models.CharField(max_length=32, unique=True, db_index=True) consumer_secret = models.CharField(max_length=32, unique=True) - instance_guid = models.CharField(max_length=255, null=True, unique=True) + instance_guid = models.CharField(max_length=255, blank=True, null=True, unique=True) @staticmethod def get_or_supplement(instance_guid, consumer_key): From 664544035a74a91dbfe3005d153b3c32acf22b27 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 29 Aug 2015 20:22:27 -0400 Subject: [PATCH 2/3] Add explicit name for LTI provider outcome celery task. --- lms/djangoapps/lti_provider/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/lti_provider/tasks.py b/lms/djangoapps/lti_provider/tasks.py index cf11489f5d..2db49f8c0d 100644 --- a/lms/djangoapps/lti_provider/tasks.py +++ b/lms/djangoapps/lti_provider/tasks.py @@ -44,7 +44,7 @@ def score_changed_handler(sender, **kwargs): # pylint: disable=unused-argument ) -@CELERY_APP.task +@CELERY_APP.task(name='lti_provider.tasks.send_outcome') def send_outcome(points_possible, points_earned, user_id, course_id, usage_id): """ Calculate the score for a given user in a problem and send it to the From f577a6d2781736ecc2c6bfe8fb7e1d80547ae54e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 31 Aug 2015 15:22:37 -0400 Subject: [PATCH 3/3] Add body hash to LTI outcome message. This is necessary to properly implement the LTI outcome passback spec. It was not included previously because it was causing problems with Canvas, but Blackboard will not accept outcomes unless they are properly signed. The requests_oauthlib doesn't support the body hash spec out of the box, so BodyHashClient needed to be made. Fortunately, it's a pretty simple spec: https://oauth.googlecode.com/svn/spec/ext/body_hash/1.0/oauth-bodyhash.html --- lms/djangoapps/lti_provider/outcomes.py | 32 ++++++++++++- .../lti_provider/tests/test_outcomes.py | 47 ++++++++++++++++++- 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/lti_provider/outcomes.py b/lms/djangoapps/lti_provider/outcomes.py index bcfd37a883..bc02bcb3ec 100644 --- a/lms/djangoapps/lti_provider/outcomes.py +++ b/lms/djangoapps/lti_provider/outcomes.py @@ -3,18 +3,39 @@ Helper functions for managing interactions with the LTI outcomes service defined in LTI v1.1. """ +from hashlib import sha1 +from base64 import b64encode import logging +import uuid + from lxml import etree from lxml.builder import ElementMaker +from oauthlib.oauth1 import Client +from oauthlib.common import to_unicode import requests import requests_oauthlib -import uuid from lti_provider.models import GradedAssignment, OutcomeService log = logging.getLogger("edx.lti_provider") +class BodyHashClient(Client): + """ + OAuth1 Client that adds body hash support (required by LTI). + + The default Client doesn't support body hashes, so we have to add it ourselves. + The spec: + https://oauth.googlecode.com/svn/spec/ext/body_hash/1.0/oauth-bodyhash.html + """ + def get_oauth_params(self, request): + """Override get_oauth_params to add the body hash.""" + params = super(BodyHashClient, self).get_oauth_params(request) + digest = b64encode(sha1(request.body.encode('UTF-8')).digest()) + params.append((u'oauth_body_hash', to_unicode(digest))) + return params + + def store_outcome_parameters(request_params, user, lti_consumer): """ Determine whether a set of LTI launch parameters contains information about @@ -112,7 +133,13 @@ def sign_and_send_replace_result(assignment, xml): # message. Testing with Canvas throws an error when this field is included. # This code may need to be revisited once we test with other LMS platforms, # and confirm whether there's a bug in Canvas. - oauth = requests_oauthlib.OAuth1(consumer_key, consumer_secret) + oauth = requests_oauthlib.OAuth1( + consumer_key, + consumer_secret, + signature_method='HMAC-SHA1', + client_class=BodyHashClient, + force_include_body=True + ) headers = {'content-type': 'application/xml'} response = requests.post( @@ -121,6 +148,7 @@ def sign_and_send_replace_result(assignment, xml): auth=oauth, headers=headers ) + return response diff --git a/lms/djangoapps/lti_provider/tests/test_outcomes.py b/lms/djangoapps/lti_provider/tests/test_outcomes.py index 981a56a704..4845e7ff3e 100644 --- a/lms/djangoapps/lti_provider/tests/test_outcomes.py +++ b/lms/djangoapps/lti_provider/tests/test_outcomes.py @@ -1,16 +1,20 @@ """ Tests for the LTI outcome service handlers, both in outcomes.py and in tasks.py """ +import unittest from django.test import TestCase from lxml import etree from mock import patch, MagicMock, ANY +import requests_oauthlib +import requests + +from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator from student.tests.factories import UserFactory from lti_provider.models import GradedAssignment, LtiConsumer, OutcomeService import lti_provider.outcomes as outcomes import lti_provider.tasks as tasks -from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator class StoreOutcomeParametersTest(TestCase): @@ -363,3 +367,44 @@ class XmlHandlingTest(TestCase): major_code='failure' ) self.assertFalse(outcomes.check_replace_result_response(response)) + + +class TestBodyHashClient(unittest.TestCase): + """ + Test our custom BodyHashClient + + This Client should do everything a normal oauthlib.oauth1.Client would do, + except it also adds oauth_body_hash to the Authorization headers. + """ + def test_simple_message(self): + oauth = requests_oauthlib.OAuth1( + '1000000000000000', # fake consumer key + '2000000000000000', # fake consumer secret + signature_method='HMAC-SHA1', + client_class=outcomes.BodyHashClient, + force_include_body=True + ) + headers = {'content-type': 'application/xml'} + req = requests.Request( + 'POST', + "http://example.edx.org/fake", + data="Hello world!", + auth=oauth, + headers=headers + ) + prepped_req = req.prepare() + + # Make sure that our body hash is now part of the test... + self.assertIn( + 'oauth_body_hash="00hq6RNueFa8QiEjhep5cJRHWAI%3D"', + prepped_req.headers['Authorization'] + ) + + # But make sure we haven't wiped out any of the other oauth values + # that we would expect to be in the Authorization header as well + expected_oauth_headers = [ + "oauth_nonce", "oauth_timestamp", "oauth_version", + "oauth_signature_method", "oauth_consumer_key", "oauth_signature", + ] + for oauth_header in expected_oauth_headers: + self.assertIn(oauth_header, prepped_req.headers['Authorization'])