From cc2e4bfeae325f1c7e893ecc0cf76110235ef674 Mon Sep 17 00:00:00 2001 From: Oleg Marshev Date: Thu, 5 Dec 2013 18:11:32 +0200 Subject: [PATCH] Fix lis_outcome_service_url sending. --- CHANGELOG.rst | 1 + common/djangoapps/student/models.py | 4 +- common/lib/xmodule/xmodule/lti_module.py | 25 +++- .../xmodule/xmodule/tests/test_lti_unit.py | 28 ++++- common/lib/xmodule/xmodule/x_module.py | 3 + .../mock_lti_server/mock_lti_server.py | 112 ++++++++---------- .../mock_lti_server/test_mock_lti_server.py | 17 ++- .../courseware/tests/test_lti_integration.py | 1 - 8 files changed, 107 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3297f426a0..cf288721fb 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,7 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Blades: Make LTI module not send grade_back_url if has_score=False. BLD-561. Blades: LTI additional Python tests. LTI must use HTTPS for lis_outcome_service_url. BLD-564. diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 8921a078ba..935abbdd2e 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -46,8 +46,8 @@ class AnonymousUserId(models.Model): Purpose of this table is to provide user by anonymous_user_id. - We are generating anonymous_user_id using md5 algorithm, so resulting length will always be 16 bytes. - http://docs.python.org/2/library/md5.html#md5.digest_size + We generate anonymous_user_id using md5 algorithm, + and use result in hex form, so its length is equal to 32 bytes. """ user = models.ForeignKey(User, db_index=True) anonymous_user_id = models.CharField(unique=True, max_length=32) diff --git a/common/lib/xmodule/xmodule/lti_module.py b/common/lib/xmodule/xmodule/lti_module.py index 45ec5b29bc..ff90d0525e 100644 --- a/common/lib/xmodule/xmodule/lti_module.py +++ b/common/lib/xmodule/xmodule/lti_module.py @@ -355,11 +355,15 @@ class LTIModule(LTIFields, XModule): # Parameters required for grading: u'resource_link_id': self.get_resource_link_id(), - u'lis_outcome_service_url': self.get_outcome_service_url(), u'lis_result_sourcedid': self.get_lis_result_sourcedid(), } + if self.has_score: + body.update({ + u'lis_outcome_service_url': self.get_outcome_service_url() + }) + # Appending custom parameter for signing. body.update(custom_parameters) @@ -483,6 +487,8 @@ oauth_consumer_key="", oauth_signature="frVp4JuvT1mVXlxktiAUjQ7%2F1cw%3D"'} try: imsx_messageIdentifier, sourcedId, score, action = self.parse_grade_xml_body(request.body) except Exception: + log.debug("[LTI]: Request body XML parsing error.") + failure_values['imsx_description'] = 'Request body XML parsing error.' return Response(response_xml_template.format(**failure_values), content_type="application/xml") # Verify OAuth signing. @@ -490,10 +496,15 @@ oauth_consumer_key="", oauth_signature="frVp4JuvT1mVXlxktiAUjQ7%2F1cw%3D"'} self.verify_oauth_body_sign(request) except (ValueError, LTIError): failure_values['imsx_messageIdentifier'] = escape(imsx_messageIdentifier) + failure_values['imsx_description'] = 'OAuth verification error.' return Response(response_xml_template.format(**failure_values), content_type="application/xml") - real_user = self.system.get_real_user(urllib.unquote(sourcedId.split(':')[-1])) + if not real_user: # that means we can't save to database, as we do not have real user id. + failure_values['imsx_messageIdentifier'] = escape(imsx_messageIdentifier) + failure_values['imsx_description'] = 'User not found.' + return Response(response_xml_template.format(**failure_values), content_type="application/xml") + if action == 'replaceResultRequest': self.system.publish( event={ @@ -510,9 +521,11 @@ oauth_consumer_key="", oauth_signature="frVp4JuvT1mVXlxktiAUjQ7%2F1cw%3D"'} 'imsx_messageIdentifier': escape(imsx_messageIdentifier), 'response': '' } + log.debug("[LTI]: Grade is saved.") return Response(response_xml_template.format(**values), content_type="application/xml") unsupported_values['imsx_messageIdentifier'] = escape(imsx_messageIdentifier) + log.debug("[LTI]: Incorrect action.") return Response(response_xml_template.format(**unsupported_values), content_type='application/xml') @@ -541,6 +554,7 @@ oauth_consumer_key="", oauth_signature="frVp4JuvT1mVXlxktiAUjQ7%2F1cw%3D"'} # Raise exception if score is not float or not in range 0.0-1.0 regarding spec. score = float(score) if not 0 <= score <= 1: + log.debug("[LTI]: Score not in range.") raise LTIError return imsx_messageIdentifier, sourcedId, score, action @@ -582,8 +596,11 @@ oauth_consumer_key="", oauth_signature="frVp4JuvT1mVXlxktiAUjQ7%2F1cw%3D"'} params=oauth_headers.items(), signature=oauth_signature ) - if (oauth_body_hash != oauth_headers.get('oauth_body_hash') or - not signature.verify_hmac_sha1(mock_request, client_secret)): + if oauth_body_hash != oauth_headers.get('oauth_body_hash'): + log.debug("[LTI]: OAuth body hash verification is failed.") + raise LTIError + if not signature.verify_hmac_sha1(mock_request, client_secret): + log.debug("[LTI]: OAuth signature verification is failed.") raise LTIError def get_client_key_secret(self): diff --git a/common/lib/xmodule/xmodule/tests/test_lti_unit.py b/common/lib/xmodule/xmodule/tests/test_lti_unit.py index 63a4e49f6c..0c25bbafd2 100644 --- a/common/lib/xmodule/xmodule/tests/test_lti_unit.py +++ b/common/lib/xmodule/xmodule/tests/test_lti_unit.py @@ -112,7 +112,7 @@ class LTIModuleTest(LogicTest): expected_response = { 'action': None, 'code_major': 'failure', - 'description': 'The request has failed.', + 'description': 'OAuth verification error.', 'messageIdentifier': self.DEFAULTS['messageIdentifier'], } @@ -133,7 +133,27 @@ class LTIModuleTest(LogicTest): expected_response = { 'action': None, 'code_major': 'failure', - 'description': 'The request has failed.', + 'description': 'OAuth verification error.', + 'messageIdentifier': self.DEFAULTS['messageIdentifier'], + } + self.assertEqual(response.status_code, 200) + self.assertDictEqual(expected_response, real_response) + + def test_real_user_is_none(self): + """ + If we have no real user, we should send back failure response. + """ + self.xmodule.verify_oauth_body_sign = Mock() + self.xmodule.has_score = True + self.system.get_real_user = Mock(return_value=None) + request = Request(self.environ) + request.body = self.get_request_body() + response = self.xmodule.grade_handler(request, '') + real_response = self.get_response_values(response) + expected_response = { + 'action': None, + 'code_major': 'failure', + 'description': 'User not found.', 'messageIdentifier': self.DEFAULTS['messageIdentifier'], } self.assertEqual(response.status_code, 200) @@ -151,7 +171,7 @@ class LTIModuleTest(LogicTest): expected_response = { 'action': None, 'code_major': 'failure', - 'description': 'The request has failed.', + 'description': 'Request body XML parsing error.', 'messageIdentifier': 'unknown', } self.assertEqual(response.status_code, 200) @@ -169,7 +189,7 @@ class LTIModuleTest(LogicTest): expected_response = { 'action': None, 'code_major': 'failure', - 'description': 'The request has failed.', + 'description': 'Request body XML parsing error.', 'messageIdentifier': 'unknown', } self.assertEqual(response.status_code, 200) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 9154952ace..8f8674b615 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1016,6 +1016,9 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable=abs error_descriptor_class - The class to use to render XModules with errors + get_real_user - function that takes `anonymous_student_id` and returns real user_id, + associated with `anonymous_student_id`. + """ # Right now, usage_store is unused, and field_data is always supplanted diff --git a/lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py b/lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py index 9d69ec4152..a788aed196 100644 --- a/lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py +++ b/lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py @@ -33,7 +33,6 @@ class MockLTIRequestHandler(BaseHTTPRequestHandler): protocol = "HTTP/1.0" callback_url = None - def log_message(self, format, *args): """Log an arbitrary message.""" # Code copied from BaseHTTPServer.py. Changed to write to sys.stdout @@ -49,18 +48,13 @@ class MockLTIRequestHandler(BaseHTTPRequestHandler): Used for checking LTI Provider started correctly. ''' - self.send_response(200, 'OK') self.send_header('Content-type', 'html') self.end_headers() - response_str = """TEST TITLE This is LTI Provider.""" - self.wfile.write(response_str) - - def do_POST(self): ''' Handle a POST request from the client and sends response back. @@ -72,38 +66,17 @@ class MockLTIRequestHandler(BaseHTTPRequestHandler): # Respond to request with correct lti endpoint: elif self._is_correct_lti_request(): self.post_dict = self._post_dict() - correct_keys = [ - 'user_id', - 'role', - 'oauth_nonce', - 'oauth_timestamp', - 'oauth_consumer_key', - 'lti_version', - 'oauth_signature_method', - 'oauth_version', - 'oauth_signature', - 'lti_message_type', - 'oauth_callback', - 'lis_outcome_service_url', - 'lis_result_sourcedid', - 'launch_presentation_return_url', - # 'lis_person_sourcedid', optional, not used now. - 'resource_link_id', - ] - if sorted(correct_keys) != sorted(self.post_dict.keys()): - status_message = "Incorrect LTI header" + params = {k: v for k, v in self.post_dict.items() if k != 'oauth_signature'} + if self.server.check_oauth_signature(params, self.post_dict.get('oauth_signature', "")): + status_message = "This is LTI tool. Success." + # set data for grades what need to be stored as server data + if 'lis_outcome_service_url' in self.post_dict: + self.server.grade_data = { + 'callback_url': self.post_dict.get('lis_outcome_service_url'), + 'sourcedId': self.post_dict.get('lis_result_sourcedid') + } else: - params = {k: v for k, v in self.post_dict.items() if k != 'oauth_signature'} - if self.server.check_oauth_signature(params, self.post_dict['oauth_signature']): - status_message = "This is LTI tool. Success." - else: - status_message = "Wrong LTI signature" - # set data for grades - # what need to be stored as server data - self.server.grade_data = { - 'callback_url': self.post_dict.get('lis_outcome_service_url'), - 'sourcedId': self.post_dict.get('lis_result_sourcedid') - } + status_message = "Wrong LTI signature" self._send_response(status_message, 200) else: status_message = "Invalid request URL" @@ -141,17 +114,17 @@ class MockLTIRequestHandler(BaseHTTPRequestHandler): self.server.cookie = {} referer = urlparse.urlparse(self.headers.getheader('referer')) self.server.referer_host = "{}://{}".format(referer.scheme, referer.netloc) - self.server.referer_netloc = referer.netloc return post_dict def _send_graded_result(self): - + """ + Send grade request. + """ values = { 'textString': 0.5, 'sourcedId': self.server.grade_data['sourcedId'], 'imsx_messageIdentifier': uuid4().hex, } - payload = textwrap.dedent(""" @@ -208,40 +181,53 @@ class MockLTIRequestHandler(BaseHTTPRequestHandler): Send message back to the client ''' self._send_head(status_code) - if self.server.grade_data['callback_url']: - response_str = """TEST TITLE - -

Graded IFrame loaded

\ -

Server response is:

\ -

{}

-
- -
+ if getattr(self.server, 'grade_data', False): # lti can be graded + response_str = textwrap.dedent(""" + + + TEST TITLE + + +
+

Graded IFrame loaded

+

Server response is:

+

{}

+
+
+ +
+ + + """).format(message, url="http://%s:%s" % self.server.server_address) + else: # lti can't be graded + response_str = textwrap.dedent(""" + + + TEST TITLE + + +
+

IFrame loaded

+

Server response is:

+

{}

+
+ + + """).format(message) - """.format(message, url="http://%s:%s" % self.server.server_address) - else: - response_str = """TEST TITLE - -

IFrame loaded

\ -

Server response is:

\ -

{}

- """.format(message) - - # Log the response logger.debug("LTI: sent response {}".format(response_str)) - self.wfile.write(response_str) def _is_correct_lti_request(self): - '''If url to LTI tool is correct.''' + ''' + If url to LTI tool is correct. + ''' return self.server.oauth_settings['lti_endpoint'] in self.path def oauth_sign(self, url, body): """ Signs request and returns signed body and headers. - """ - client = oauthlib.oauth1.Client( client_key=unicode(self.server.oauth_settings['client_key']), client_secret=unicode(self.server.oauth_settings['client_secret']) diff --git a/lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py b/lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py index 282cbcc039..3f6a7c57d2 100644 --- a/lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py +++ b/lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py @@ -49,10 +49,9 @@ class MockLTIServerTest(unittest.TestCase): def test_wrong_header(self): """ - Tests that LTI server processes request with right program - path and responses with wrong header. + Tests that LTI server processes request with right program path but with wrong header. """ - #wrong number of params + #wrong number of params and no signature payload = { 'user_id': 'default_user_id', 'role': 'student', @@ -62,7 +61,7 @@ class MockLTIServerTest(unittest.TestCase): uri = self.server.oauth_settings['lti_base'] + self.server.oauth_settings['lti_endpoint'] headers = {'referer': 'http://localhost:8000/'} response = requests.post(uri, data=payload, headers=headers) - self.assertIn('Incorrect LTI header', response.content) + self.assertIn('Wrong LTI signature', response.content) def test_wrong_signature(self): """ @@ -74,7 +73,7 @@ class MockLTIServerTest(unittest.TestCase): 'role': 'student', 'oauth_nonce': '', 'oauth_timestamp': '', - 'oauth_consumer_key': 'client_key', + 'oauth_consumer_key': 'test_client_key', 'lti_version': 'LTI-1p0', 'oauth_signature_method': 'HMAC-SHA1', 'oauth_version': '1.0', @@ -101,7 +100,7 @@ class MockLTIServerTest(unittest.TestCase): 'role': 'student', 'oauth_nonce': '', 'oauth_timestamp': '', - 'oauth_consumer_key': 'client_key', + 'oauth_consumer_key': 'test_client_key', 'lti_version': 'LTI-1p0', 'oauth_signature_method': 'HMAC-SHA1', 'oauth_version': '1.0', @@ -112,7 +111,6 @@ class MockLTIServerTest(unittest.TestCase): 'lis_outcome_service_url': '', 'lis_result_sourcedid': '', 'resource_link_id':'', - "lis_outcome_service_url": '', } self.server.check_oauth_signature = Mock(return_value=True) @@ -128,7 +126,7 @@ class MockLTIServerTest(unittest.TestCase): 'role': 'student', 'oauth_nonce': '', 'oauth_timestamp': '', - 'oauth_consumer_key': 'client_key', + 'oauth_consumer_key': 'test_client_key', 'lti_version': 'LTI-1p0', 'oauth_signature_method': 'HMAC-SHA1', 'oauth_version': '1.0', @@ -139,7 +137,6 @@ class MockLTIServerTest(unittest.TestCase): 'lis_outcome_service_url': '', 'lis_result_sourcedid': '', 'resource_link_id':'', - "lis_outcome_service_url": '', } self.server.check_oauth_signature = Mock(return_value=True) @@ -147,8 +144,8 @@ class MockLTIServerTest(unittest.TestCase): #this is the uri for sending grade from lti headers = {'referer': 'http://localhost:8000/'} response = requests.post(uri, data=payload, headers=headers) + self.assertIn('This is LTI tool. Success.', response.content) - self.assertTrue('This is LTI tool. Success.' in response.content) self.server.grade_data['TC answer'] = "Test response" graded_response = requests.post('http://127.0.0.1:8034/grade') self.assertIn('Test response', graded_response.content) diff --git a/lms/djangoapps/courseware/tests/test_lti_integration.py b/lms/djangoapps/courseware/tests/test_lti_integration.py index d792285f6e..178cef57f1 100644 --- a/lms/djangoapps/courseware/tests/test_lti_integration.py +++ b/lms/djangoapps/courseware/tests/test_lti_integration.py @@ -46,7 +46,6 @@ class TestLTI(BaseTestXmodule): u'role': u'student', u'resource_link_id': module_id, - u'lis_outcome_service_url': lis_outcome_service_url, u'lis_result_sourcedid': sourcedId, u'oauth_nonce': mocked_nonce,