Skip to content

Fixes and improvement to LTI. #1987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
olmar merged 1 commit into from
Dec 27, 2013
Merged

Fixes and improvement to LTI. #1987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions common/lib/xmodule/xmodule/lti_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,23 +486,26 @@ def grade_handler(self, request, dispatch):

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.'
except Exception as e:
error_message = "Request body XML parsing error: " + escape(e.message)
log.debug("[LTI]: " + error_message)
failure_values['imsx_description'] = error_message
return Response(response_xml_template.format(**failure_values), content_type="application/xml")

# Verify OAuth signing.
try:
self.verify_oauth_body_sign(request)
except (ValueError, LTIError):
except (ValueError, LTIError) as e:
failure_values['imsx_messageIdentifier'] = escape(imsx_messageIdentifier)
failure_values['imsx_description'] = 'OAuth verification error.'
error_message = "OAuth verification error: " + escape(e.message)
failure_values['imsx_description'] = error_message
log.debug("[LTI]: " + error_message)
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.'
failure_values['imsx_description'] = "User not found."
return Response(response_xml_template.format(**failure_values), content_type="application/xml")

if action == 'replaceResultRequest':
Expand Down Expand Up @@ -554,8 +557,7 @@ def parse_grade_xml_body(cls, body):
# 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
raise LTIError('score value outside the permitted range of 0-1.')

return imsx_messageIdentifier, sourcedId, score, action

Expand All @@ -576,7 +578,6 @@ def verify_oauth_body_sign(self, request):
"""

client_key, client_secret = self.get_client_key_secret()

headers = {
'Authorization':unicode(request.headers.get('Authorization')),
'Content-Type': 'application/x-www-form-urlencoded',
Expand All @@ -597,11 +598,9 @@ def verify_oauth_body_sign(self, request):
signature=oauth_signature
)
if oauth_body_hash != oauth_headers.get('oauth_body_hash'):
log.debug("[LTI]: OAuth body hash verification is failed.")
raise LTIError
raise LTIError("OAuth body hash verification is failed.")
if not signature.verify_hmac_sha1(mock_request, client_secret):
log.debug("[LTI]: OAuth signature verification is failed.")
raise LTIError
raise LTIError("OAuth signature verification is failed.")

def get_client_key_secret(self):
"""
Expand Down
14 changes: 8 additions & 6 deletions common/lib/xmodule/xmodule/tests/test_lti_unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ def get_response_values(self, response):
'action': action
}

def test_authorization_header_not_present(self):
@patch('xmodule.lti_module.LTIModule.get_client_key_secret', return_value=('test_client_key', u'test_client_secret'))
def test_authorization_header_not_present(self, get_key_secret):
"""
Request has no Authorization header.

Expand All @@ -112,14 +113,15 @@ def test_authorization_header_not_present(self):
expected_response = {
'action': None,
'code_major': 'failure',
'description': 'OAuth verification error.',
'description': 'OAuth verification error: Malformed authorization header',
'messageIdentifier': self.DEFAULTS['messageIdentifier'],
}

self.assertEqual(response.status_code, 200)
self.assertDictEqual(expected_response, real_response)

def test_authorization_header_empty(self):
@patch('xmodule.lti_module.LTIModule.get_client_key_secret', return_value=('test_client_key', u'test_client_secret'))
def test_authorization_header_empty(self, get_key_secret):

@auraz auraz Dec 19, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add doc string to every test that checks exception rising.

"""
Request Authorization header has no value.

Expand All @@ -133,7 +135,7 @@ def test_authorization_header_empty(self):
expected_response = {
'action': None,
'code_major': 'failure',
'description': 'OAuth verification error.',
'description': 'OAuth verification error: Malformed authorization header',
'messageIdentifier': self.DEFAULTS['messageIdentifier'],
}
self.assertEqual(response.status_code, 200)
Expand Down Expand Up @@ -171,7 +173,7 @@ def test_grade_not_in_range(self):
expected_response = {
'action': None,
'code_major': 'failure',
'description': 'Request body XML parsing error.',
'description': 'Request body XML parsing error: score value outside the permitted range of 0-1.',
'messageIdentifier': 'unknown',
}
self.assertEqual(response.status_code, 200)
Expand All @@ -189,7 +191,7 @@ def test_bad_grade_decimal(self):
expected_response = {
'action': None,
'code_major': 'failure',
'description': 'Request body XML parsing error.',
'description': 'Request body XML parsing error: invalid literal for float(): 0,5',
'messageIdentifier': 'unknown',
}
self.assertEqual(response.status_code, 200)
Expand Down
5 changes: 2 additions & 3 deletions lms/djangoapps/courseware/features/lti_setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,8 @@ def setup_mock_lti_server():
'lti_endpoint': 'correct_lti_endpoint'
}

# Flag for acceptance tests used for creating right callback_url and sending
# graded result. Used in MockLTIRequestHandler.
server.test_mode = True
# For testing on localhost make callback url using referer host.

@auraz auraz Dec 25, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: referrer

@olmar olmar Dec 25, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not typo :) http://en.wikipedia.org/wiki/HTTP_referer

@auraz auraz Dec 25, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL, HTTP referer (originally a misspelling of referrer)

you are right)

Sincerely,
Alexander Kryklia
Отправлено при помощи Sparrow (http://www.sparrowmailapp.com/?sig)

среда, 25 декабря 2013 г. в 16:35, Oleg Marshev написал:

In lms/djangoapps/courseware/features/lti_setup.py:

@@ -38,9 +38,10 @@ def setup_mock_lti_server(): > 'lti_endpoint': 'correct_lti_endpoint' > } > > - # Flag for acceptance tests used for creating right callback_url and sending > - # graded result. Used in MockLTIRequestHandler. > - server.test_mode = True > + # For testing on localhost make callback url using referer host.
not typo :) http://en.wikipedia.org/wiki/HTTP_referer

—
Reply to this email directly or view it on GitHub (https://github.com/edx/edx-platform/pull/1987/files#r8550943).

server.real_callback_url_on = False

@auraz auraz Dec 25, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rename to use_real_callback_url

@olmar olmar Dec 25, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


# Store the server instance in lettuce's world
# so that other steps can access it
Expand Down
20 changes: 12 additions & 8 deletions lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def do_POST(self):
'''
if 'grade' in self.path and self._send_graded_result().status_code == 200:
status_message = 'LTI consumer (edX) responded with XML content:<br>' + self.server.grade_data['TC answer']
self.server.grade_data['callback_url'] = None
self.server.grade_data = None
self._send_response(status_message, 200)
# Respond to request with correct lti endpoint:
elif self._is_correct_lti_request():
Expand Down Expand Up @@ -152,12 +152,14 @@ def _send_graded_result(self):
</imsx_POXEnvelopeRequest>
""")
data = payload.format(**values)
# get relative part, because host name is different in a) manual tests b) acceptance tests c) demos
if getattr(self.server, 'test_mode', None):
if getattr(self.server, 'use_real_callback_url', None):
# Use exact URL that was sent from TC when using this Stub LTI server
# as TP in real standalone environment.
url = self.server.grade_data['callback_url']
else:
# Use relative URL when using TP locally for manual testing or jenkins.
relative_url = urlparse.urlparse(self.server.grade_data['callback_url']).path
url = self.server.referer_host + relative_url
else:
url = self.server.grade_data['callback_url']

headers = {'Content-Type': 'application/xml', 'X-Requested-With': 'XMLHttpRequest'}
headers['Authorization'] = self.oauth_sign(url, data)
Expand All @@ -167,11 +169,12 @@ def _send_graded_result(self):
if getattr(self.server, 'run_inside_unittest_flag', None):
response = mock.Mock(status_code=200, url=url, data=data, headers=headers)
return response

# Send request ignoring verification of SSL certificate
response = requests.post(
url,
data=data,
headers=headers
headers=headers,
verify=False

@auraz auraz Dec 19, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comment here, why we need it

)
self.server.grade_data['TC answer'] = response.content
return response
Expand All @@ -182,6 +185,7 @@ def _send_response(self, message, status_code):
'''
self._send_head(status_code)
if getattr(self.server, 'grade_data', False): # lti can be graded
url = "//{}:{}".format(self.server.server_host, self.server.server_port)
response_str = textwrap.dedent("""
<html>
<head>
Expand All @@ -198,7 +202,7 @@ def _send_response(self, message, status_code):
</form>
</body>
</html>
""").format(message, url="http://%s:%s" % self.server.server_address)
""").format(message, url=url)
else: # lti can't be graded
response_str = textwrap.dedent("""
<html>
Expand Down
6 changes: 3 additions & 3 deletions lms/djangoapps/courseware/mock_lti_server/server_start.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@
'lti_endpoint': 'correct_lti_endpoint'
}
server.server_host = server_host
server.server_port = server_port

# If in test mode mock lti server will make callback url using referer host.
# Used in MockLTIRequestHandler when sending graded result.
server.test_mode = True
# For testing on localhost make callback url using referer host.

@auraz auraz Dec 25, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add some helpful defaults:

# for manual testing on localhost:
server.use_real_callback=False
server.use_ssl = False

# running standalone against sandbox:
#server.use_real_callback_url = True
#server.use_ssl = False

# running standalone against stage 
#server,use_real_callback_url = True
#server.use_ssl = True

server.use_real_callback_url = False

try:
server.serve_forever()
Expand Down
3 changes: 3 additions & 0 deletions lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ def setUp(self):
#flag for creating right callback_url
self.server.test_mode = True

self.server.server_host = server_host
self.server.server_port = server_port

# Start the server in a separate daemon thread
server_thread = threading.Thread(target=self.server.serve_forever)
server_thread.daemon = True
Expand Down