From efffd1a4d40a6b81b90c7c55f47afaa8d38d85b6 Mon Sep 17 00:00:00 2001 From: Artem Baranovskyi Date: Sun, 1 Dec 2024 20:43:12 +0200 Subject: [PATCH] Added post data sanitising before passing to the external service. Added user's capability check. Few plugin API logic optimizing. Added http curl setting script. Added proper exception naming. https://transfer.hft-stuttgart.de/gitlab/ulrike.pado/asyst-moodle-plugin/-/issues/4 --- api.php | 102 +++++++++++++++++++++-------------- classes/api/http_client.php | 2 + lang/en/local_asystgrade.php | 10 ++++ readme.md | 9 ++++ update_http_settings.php | 47 ++++++++++++++++ 5 files changed, 130 insertions(+), 40 deletions(-) create mode 100755 update_http_settings.php diff --git a/api.php b/api.php index 8dfb2a2..e6343d9 100755 --- a/api.php +++ b/api.php @@ -35,6 +35,7 @@ use local_asystgrade\utils; try { require_login(); + require_capability('mod/assign:grade', context_system::instance()); } catch (coding_exception | moodle_exception $e) { debugging($e->getMessage()); redirect( @@ -43,53 +44,74 @@ try { ); } -if ($_SERVER['REQUEST_METHOD'] === 'POST') { - $data = json_decode(file_get_contents('php://input'), true); +if ($_SERVER['REQUEST_METHOD'] !== 'POST') { + throw new moodle_exception('invalidmethod', 'local_asystgrade'); +} - if ($data) { - // Preparing Flask API. - try { - $apiendpoint = utils::get_api_endpoint(); - } catch (dml_exception $e) { - debugging('Failed to get API endpoint setting: ' . $e->getMessage()); - } +$data = json_decode(file_get_contents('php://input'), true); - $httpclient = new http_client(); - $apiclient = client::getInstance($apiendpoint, $httpclient); +if ($data) { + // Preparing Flask API. + try { + $apiendpoint = utils::get_api_endpoint(); + } catch (dml_exception $e) { + debugging('Failed to get API endpoint setting: ' . $e->getMessage()); + } - $maxretries = 3; - $attempts = 0; - $success = false; + $httpclient = new http_client(); + $apiclient = client::getInstance($apiendpoint, $httpclient); - while ($attempts < $maxretries && !$success) { - try { - // Sending data to Flask and obtaining an answer. - $response = $apiclient->send_data($data); - $success = true; - } catch (Exception $e) { - $attempts++; - debugging('API request error: ' . $e->getMessage()); - if ($attempts >= $maxretries) { - echo json_encode(['error' => 'A server error occurred. Please try again later.']); - exit; // Ensure to stop further processing. - } - } - } + $response = retry_api_request($apiclient, $data); + $grades = json_decode($response, true); + + // Check JSON validity. + if (json_last_error() !== JSON_ERROR_NONE) { + debugging('JSON decode error: ' . json_last_error_msg()); + throw new moodle_exception('invalidjson', 'local_asystgrade', '', json_last_error_msg()); + } else { + echo json_encode(['success' => true, 'grades' => $grades]); + } +} else { + echo json_encode(['error' => 'No data received']); +} - if ($success) { - $grades = json_decode($response, true); - // Check JSON validity. - if (json_last_error() === JSON_ERROR_NONE) { - echo json_encode(['success' => true, 'grades' => $grades]); - } else { - debugging('JSON decode error: ' . json_last_error_msg()); - echo json_encode(['error' => 'Invalid JSON from Flask API']); +/** + * Validates the provided request payload data array. + * + * @param array $data The data to validate. + * @return array The cleaned data. + * @throws moodle_exception If the data is invalid. + */ +function validate_data($data): array { + if (!isset($data['referenceAnswer'], $data['studentAnswers']) || !is_array($data['studentAnswers'])) { + throw new moodle_exception('invalidrequest', 'local_asystgrade'); + } + return [ + 'referenceAnswer' => clean_param($data['referenceAnswer'], PARAM_TEXT), + 'studentAnswers' => array_map(fn($answer) => clean_param($answer, PARAM_TEXT), $data['studentAnswers']), + ]; +} + +/** + * Retries an API request a specified number of times. + * + * @param object $apiclient The API client to use for the request. + * @param array $payload The data to send in the request. + * @param int $maxretries The maximum number of retry attempts. + * @return mixed The response from the API client. + * @throws moodle_exception If the API request fails after the maximum retries. + */ +function retry_api_request($apiclient, $payload, $maxretries = 3): mixed +{ + for ($attempts = 0; $attempts < $maxretries; $attempts++) { + try { + return $apiclient->send_data(validate_data($payload)); + } catch (Exception $e) { + debugging('API request error: ' . $e->getMessage()); + if ($attempts + 1 === $maxretries) { + throw new moodle_exception('apifailure', 'local_asystgrade'); } } - } else { - echo json_encode(['error' => 'No data received']); } -} else { - echo json_encode(['error' => 'Invalid request method']); } diff --git a/classes/api/http_client.php b/classes/api/http_client.php index fdc8e75..47ffb83 100755 --- a/classes/api/http_client.php +++ b/classes/api/http_client.php @@ -43,6 +43,8 @@ class http_client implements http_client_interface { * @throws Exception */ public function post(string $url, array $data): bool|string { + global $CFG; + require_once($CFG->libdir . '/filelib.php'); $curl = new curl(); $options = [ 'CURLOPT_HTTPHEADER' => ['Content-Type: application/json'], diff --git a/lang/en/local_asystgrade.php b/lang/en/local_asystgrade.php index 5bf056b..a7aab50 100755 --- a/lang/en/local_asystgrade.php +++ b/lang/en/local_asystgrade.php @@ -28,3 +28,13 @@ $string['apiendpoint'] = 'API Endpoint'; $string['apiendpoint_desc'] = 'The endpoint of the AsystGrade API should be changed if you set ML Backend at remote server.'; $string['pluginname'] = 'ASYST API Moodle integration plugin'; $string['privacy:metadata'] = 'The AsystGrade plugin does not store any personal data.'; + +// Error messages. +$string['loginerror'] = 'You must log in with sufficient permissions to access this page.'; +$string['invalidmethod'] = 'Invalid request method. Only POST requests are allowed.'; +$string['invalidrequest'] = 'Invalid request payload. Required fields are missing or improperly formatted.'; +$string['invalidanswers'] = 'Invalid student answers provided.'; +$string['invalidjson'] = 'Failed to parse JSON response from the server: {$a}'; +$string['apifailure'] = 'The grading API failed after multiple attempts. Please try again later.'; +$string['norequestdata'] = 'No data received from the client.'; + diff --git a/readme.md b/readme.md index a678762..f9362c3 100755 --- a/readme.md +++ b/readme.md @@ -172,6 +172,15 @@ In this case it is possible to change an API address from http://127.0.0.1:5001/ If ASYST ML microservice is running, the grade will appear at every student's answer. ![Grading result](https://transfer.hft-stuttgart.de/gitlab/ulrike.pado/asyst-moodle-plugin/-/raw/asyst-moodle-plugin/images/Grading%20result.png) +### Other important settings +Since Moodle's Curl wrapper is used, it is also necessary to set a few HTTP security properties at the page /admin/settings.php?section=httpsecurity: +- remove from cURL blocked hosts list 127.0.0.0/8 and localhost. +- add to cURL allowed ports list 5001 (or other one that you use for an external custom flask server). + +If you are using default flask local server, you could also just run update_http_settings script for that: +~~~php +php ./local/asystgrade/update_http_settings.php +~~~ The structure of request to ASYST ML Backend: ~~~JSON diff --git a/update_http_settings.php b/update_http_settings.php new file mode 100755 index 0000000..bb3d17f --- /dev/null +++ b/update_http_settings.php @@ -0,0 +1,47 @@ +. + +/** + * CLI Script for the local_asystgrade plugin to set HTTP Curl port and domain permissions. + * + * @package local_asystgrade + * @copyright 2024 Artem Baranovskyi + * @copyright based on work by 2023 Ulrike Pado , + * @copyright Yunus Eryilmaz & Larissa Kirschner + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * Define CLI_SCRIPT to indicate this script is being run from the command line. + */ +const CLI_SCRIPT = true; +require('config.php'); + +global $CFG, $DB; + +// Remove 127.0.0.0/8 and localhost from blocked hosts. +$blockedhosts = get_config('core', 'curlsecurityblockedhosts'); +$newblockedhosts = str_replace(['127.0.0.0/8', 'localhost'], '', $blockedhosts); +set_config('curlsecurityblockedhosts', trim($newblockedhosts, ',')); + +// Add 5001 to allowed ports. +$allowedports = get_config('core', 'curlsecurityallowedport'); +$newallowedports = $allowedports ? $allowedports . "\r\n5001" : '5001'; +set_config('curlsecurityallowedport', $newallowedports); + +echo "Settings updated.\n"; -- GitLab