From 00e270600f8644de409cd38d2cf05710dcb6bf19 Mon Sep 17 00:00:00 2001 From: Mark Nielsen Date: Fri, 25 Sep 2015 13:44:14 -0700 Subject: [PATCH] MDL-51571 mod_lti: Improve service error handling --- mod/lti/classes/service_exception_handler.php | 122 ++++++++++++++++++ mod/lti/locallib.php | 38 +++++- mod/lti/service.php | 38 +++--- mod/lti/servicelib.php | 33 ++--- .../tests/service_exception_handler_test.php | 85 ++++++++++++ mod/lti/tests/servicelib_test.php | 115 +++++++++++++++++ 6 files changed, 393 insertions(+), 38 deletions(-) create mode 100644 mod/lti/classes/service_exception_handler.php create mode 100644 mod/lti/tests/service_exception_handler_test.php create mode 100644 mod/lti/tests/servicelib_test.php diff --git a/mod/lti/classes/service_exception_handler.php b/mod/lti/classes/service_exception_handler.php new file mode 100644 index 00000000000..38643fdf385 --- /dev/null +++ b/mod/lti/classes/service_exception_handler.php @@ -0,0 +1,122 @@ +. + +/** + * Exception handler for LTI services + * + * @package mod_lti + * @copyright Copyright (c) 2015 Moodlerooms Inc. (http://www.moodlerooms.com) + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace mod_lti; + +defined('MOODLE_INTERNAL') || die(); + +require_once(__DIR__.'/../locallib.php'); +require_once(__DIR__.'/../servicelib.php'); + +/** + * Handles exceptions when handling incoming LTI messages. + * + * Ensures that LTI always returns a XML message that can be consumed by the caller. + * + * @package mod_lti + * @copyright Copyright (c) 2015 Moodlerooms Inc. (http://www.moodlerooms.com) + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class service_exception_handler { + /** + * Enable error response logging. + * + * @var bool + */ + protected $log = false; + + /** + * The LTI service message ID, if known. + * + * @var string + */ + protected $id = ''; + + /** + * The LTI service message type, if known. + * + * @var string + */ + protected $type = 'unknownRequest'; + + /** + * Constructor. + * + * @param boolean $log Enable error response logging. + */ + public function __construct($log) { + $this->log = $log; + } + + /** + * Set the LTI message ID being handled. + * + * @param string $id + */ + public function set_message_id($id) { + if (!empty($id)) { + $this->id = $id; + } + } + + /** + * Set the LTI message type being handled. + * + * @param string $type + */ + public function set_message_type($type) { + if (!empty($type)) { + $this->type = $type; + } + } + + /** + * Echo an exception message encapsulated in XML + * + * @param \Exception $exception The exception that was thrown + */ + public function handle(\Exception $exception) { + $message = $exception->getMessage(); + + // Add the exception backtrace for developers. + if (debugging('', DEBUG_DEVELOPER)) { + $message .= "\n".format_backtrace(get_exception_info($exception)->backtrace, true); + } + + // Switch to response. + $type = str_replace('Request', 'Response', $this->type); + + // Build the appropriate xml. + $response = lti_get_response_xml('failure', $message, $this->id, $type); + + $xml = $response->asXML(); + + // Log the request if necessary. + if ($this->log) { + lti_log_response($xml, $exception); + } + + echo $xml; + } +} \ No newline at end of file diff --git a/mod/lti/locallib.php b/mod/lti/locallib.php index 9840dba6181..3bf7b0f6b57 100644 --- a/mod/lti/locallib.php +++ b/mod/lti/locallib.php @@ -1913,7 +1913,43 @@ function lti_should_log_request($rawbody) { function lti_log_request($rawbody) { if ($tempdir = make_temp_directory('mod_lti', false)) { if ($tempfile = tempnam($tempdir, 'mod_lti_request'.date('YmdHis'))) { - file_put_contents($tempfile, $rawbody); + $content = "Request Headers:\n"; + foreach (moodle\mod\lti\OAuthUtil::get_headers() as $header => $value) { + $content .= "$header: $value\n"; + } + $content .= "Request Body:\n"; + $content .= $rawbody; + + file_put_contents($tempfile, $content); + chmod($tempfile, 0644); + } + } +} + +/** + * Log an LTI response + * + * @param string $responsexml The response XML + * @param Exception $e If there was an exception, pass that too + */ +function lti_log_response($responsexml, $e = null) { + if ($tempdir = make_temp_directory('mod_lti', false)) { + if ($tempfile = tempnam($tempdir, 'mod_lti_response'.date('YmdHis'))) { + $content = ''; + if ($e instanceof Exception) { + $info = get_exception_info($e); + + $content .= "Exception:\n"; + $content .= "Message: $info->message\n"; + $content .= "Debug info: $info->debuginfo\n"; + $content .= "Backtrace:\n"; + $content .= format_backtrace($info->backtrace, true); + $content .= "\n"; + } + $content .= "Response XML:\n"; + $content .= $responsexml; + + file_put_contents($tempfile, $content); chmod($tempfile, 0644); } } diff --git a/mod/lti/service.php b/mod/lti/service.php index 0075f8eba93..a00ce8886ad 100644 --- a/mod/lti/service.php +++ b/mod/lti/service.php @@ -31,11 +31,18 @@ require_once($CFG->dirroot.'/mod/lti/locallib.php'); require_once($CFG->dirroot.'/mod/lti/servicelib.php'); // TODO: Switch to core oauthlib once implemented - MDL-30149. +use mod_lti\service_exception_handler; use moodle\mod\lti as lti; $rawbody = file_get_contents("php://input"); -if (lti_should_log_request($rawbody)) { +$logrequests = lti_should_log_request($rawbody); +$errorhandler = new service_exception_handler($logrequests); + +// Register our own error handler so we can always send valid XML response. +set_exception_handler(array($errorhandler, 'handle')); + +if ($logrequests) { lti_log_request($rawbody); } @@ -73,20 +80,13 @@ foreach ($body->children() as $child) { $messagetype = $child->getName(); } +// We know more about the message, update error handler to send better errors. +$errorhandler->set_message_id(lti_parse_message_id($xml)); +$errorhandler->set_message_type($messagetype); + switch ($messagetype) { case 'replaceResultRequest': - try { - $parsed = lti_parse_grade_replace_message($xml); - } catch (Exception $e) { - $responsexml = lti_get_response_xml( - 'failure', - $e->getMessage(), - uniqid(), - 'replaceResultResponse'); - - echo $responsexml->asXML(); - break; - } + $parsed = lti_parse_grade_replace_message($xml); $ltiinstance = $DB->get_record('lti', array('id' => $parsed->instanceid)); @@ -99,8 +99,12 @@ switch ($messagetype) { $gradestatus = lti_update_grade($ltiinstance, $parsed->userid, $parsed->launchid, $parsed->gradeval); + if (!$gradestatus) { + throw new Exception('Grade replace response'); + } + $responsexml = lti_get_response_xml( - $gradestatus ? 'success' : 'failure', + 'success', 'Grade replace response', $parsed->messageid, 'replaceResultResponse' @@ -157,8 +161,12 @@ switch ($messagetype) { $gradestatus = lti_delete_grade($ltiinstance, $parsed->userid); + if (!$gradestatus) { + throw new Exception('Grade delete request'); + } + $responsexml = lti_get_response_xml( - $gradestatus ? 'success' : 'failure', + 'success', 'Grade delete request', $parsed->messageid, 'deleteResultResponse' diff --git a/mod/lti/servicelib.php b/mod/lti/servicelib.php index 438cea2d1cf..f1521a6f8b7 100644 --- a/mod/lti/servicelib.php +++ b/mod/lti/servicelib.php @@ -57,6 +57,10 @@ function lti_get_response_xml($codemajor, $description, $messageref, $messagetyp } function lti_parse_message_id($xml) { + if (empty($xml->imsx_POXHeader)) { + return ''; + } + $node = $xml->imsx_POXHeader->imsx_POXRequestHeaderInfo->imsx_messageIdentifier; $messageid = (string)$node; @@ -285,29 +289,14 @@ function lti_verify_sourcedid($ltiinstance, $parsed) { function lti_extend_lti_services($data) { $plugins = get_plugin_list_with_function('ltisource', $data->messagetype); if (!empty($plugins)) { - try { - // There can only be one. - if (count($plugins) > 1) { - throw new coding_exception('More than one ltisource plugin handler found'); - } - $data->xml = new SimpleXMLElement($data->body); - $callback = current($plugins); - call_user_func($callback, $data); - } catch (moodle_exception $e) { - $error = $e->getMessage(); - if (debugging('', DEBUG_DEVELOPER)) { - $error .= ' '.format_backtrace(get_exception_info($e)->backtrace); - } - $responsexml = lti_get_response_xml( - 'failure', - $error, - $data->messageid, - $data->messagetype - ); - - header('HTTP/1.0 400 bad request'); - echo $responsexml->asXML(); + // There can only be one. + if (count($plugins) > 1) { + throw new coding_exception('More than one ltisource plugin handler found'); } + $data->xml = new SimpleXMLElement($data->body); + $callback = current($plugins); + call_user_func($callback, $data); + return true; } return false; diff --git a/mod/lti/tests/service_exception_handler_test.php b/mod/lti/tests/service_exception_handler_test.php new file mode 100644 index 00000000000..cd7270f0aa0 --- /dev/null +++ b/mod/lti/tests/service_exception_handler_test.php @@ -0,0 +1,85 @@ +. + +/** + * Tests Exception handler for LTI services + * + * @package mod_lti + * @copyright Copyright (c) 2015 Moodlerooms Inc. (http://www.moodlerooms.com) + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +use mod_lti\service_exception_handler; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Tests Exception handler for LTI services + * + * @package mod_lti + * @copyright Copyright (c) 2015 Moodlerooms Inc. (http://www.moodlerooms.com) + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class mod_lti_service_exception_handler_testcase extends advanced_testcase { + /** + * Testing service error handling. + */ + public function test_handle() { + $handler = new service_exception_handler(false); + $handler->set_message_id('123'); + $handler->set_message_type('testRequest'); + $handler->handle(new Exception('Error happened')); + + $this->expectOutputRegex('/imsx_codeMajor>failure/'); + $this->expectOutputRegex('/imsx_description>Error happened/'); + $this->expectOutputRegex('/imsx_messageRefIdentifier>123/'); + $this->expectOutputRegex('/imsx_operationRefIdentifier>testRequest/'); + $this->expectOutputRegex('/imsx_POXBody>handle(new Exception('Error happened')); + + $this->expectOutputRegex('/imsx_codeMajor>failure/'); + $this->expectOutputRegex('/imsx_description>Error happened/'); + $this->expectOutputRegex('/imsx_messageRefIdentifier\/>/'); + $this->expectOutputRegex('/imsx_operationRefIdentifier>unknownRequest/'); + $this->expectOutputRegex('/imsx_POXBody>resetAfterTest(); + + $handler = new service_exception_handler(true); + + ob_start(); + $handler->handle(new Exception('Error happened')); + ob_end_clean(); + + $this->assertTrue(is_dir($CFG->dataroot.'/temp/mod_lti')); + $files = glob($CFG->dataroot.'/temp/mod_lti/mod_lti_response*'); + $this->assertEquals(1, count($files)); + } +} \ No newline at end of file diff --git a/mod/lti/tests/servicelib_test.php b/mod/lti/tests/servicelib_test.php new file mode 100644 index 00000000000..286fd63fe6e --- /dev/null +++ b/mod/lti/tests/servicelib_test.php @@ -0,0 +1,115 @@ +. + +/** + * Tests for servicelib.php + * + * @package mod_lti + * @copyright Copyright (c) 2015 Moodlerooms Inc. (http://www.moodlerooms.com) + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; + +require_once($CFG->dirroot.'/mod/lti/servicelib.php'); + +/** + * Tests for servicelib.php + * + * @package mod_lti + * @copyright Copyright (c) 2015 Moodlerooms Inc. (http://www.moodlerooms.com) + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class mod_lti_servicelib_testcase extends basic_testcase { + /** + * Test that lti_parse_message_id never fails with good and bad XML. + * + * @dataProvider message_id_provider + * @param mixed $expected Expected message ID. + * @param string $xml XML to parse. + */ + public function test_lti_parse_message_id($expected, $xml) { + $xml = simplexml_load_string($xml); + $this->assertEquals($expected, lti_parse_message_id($xml)); + } + + /** + * Test data provider for testing lti_parse_message_id + * + * @return array + */ + public function message_id_provider() { + $valid = << + + + + V1.0 + 9999 + + + + +XML; + + $noheader = << + + + + V1.0 + 9999 + + + + +XML; + + $noinfo = << + + + + V1.0 + 9999 + + + + +XML; + + $noidentifier = << + + + + V1.0 + + + + +XML; + + return array( + array(9999, $valid), + array('', $noheader), + array('', $noinfo), + array('', $noidentifier), + ); + } +} \ No newline at end of file