From 3df4d04690b2e6a4dea180db87a082a10aa37a2c Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 12 Jan 2024 14:17:29 +1000 Subject: [PATCH 1/5] WIP check api --- classes/check/configuration.php | 26 ++++++++ classes/check/store_check.php | 75 ++++++++++++++++++++++ classes/local/manager.php | 3 +- classes/local/store/object_client.php | 1 + classes/local/store/object_client_base.php | 4 ++ classes/local/store/s3/client.php | 48 +++++++++----- lib.php | 12 ++-- 7 files changed, 149 insertions(+), 20 deletions(-) create mode 100644 classes/check/configuration.php create mode 100644 classes/check/store_check.php diff --git a/classes/check/configuration.php b/classes/check/configuration.php new file mode 100644 index 00000000..ab593066 --- /dev/null +++ b/classes/check/configuration.php @@ -0,0 +1,26 @@ +get_configuration_check_status(); + $status = $configstatus['ok'] ? result::OK : result::ERROR; + $summary = $configstatus['ok'] ? "OK" : "ERRORS"; // TODO lang. + $details = nl2br($configstatus['details']); + + return new result($status, $summary, $details); + } +} diff --git a/classes/check/store_check.php b/classes/check/store_check.php new file mode 100644 index 00000000..b5d052b9 --- /dev/null +++ b/classes/check/store_check.php @@ -0,0 +1,75 @@ +type = $type; + } + + public function get_id(): string { + return "store_check_" . $this->type; + } + + public function get_result(): result { + try { + // Check if configured first, and report NA if not configured. + if (!\tool_objectfs\local\manager::check_file_storage_filesystem()) { + return new result(result::NA, "todo lang Not configured."); + } + + // Load objectfs and run a test. + $config = \tool_objectfs\local\manager::get_objectfs_config(); + $client = \tool_objectfs\local\manager::get_client($config); + + if (empty($client)) { + return new result(result::UNKNOWN, "TODO lang client not configured"); + } + + $results = []; + + // TODO test delete. + switch($this->type) { + case self::TYPE_CONNECTION: + $results = $client->test_permissions(false); + break; + + case self::TYPE_RANGEREQUEST: + $results = $client->test_range_request(new $config->filesystem()); + break; + + case self::TYPE_PERMISSIONS: + $results = $client->test_permissions(false); + break; + } + + if (empty($results)) { + return new result(result::UNKNOWN, "TODO lang Test type " . $this->type . " was configured, but no test was executed."); + } + + $status = $results['success'] ? result::OK : result::ERROR; + return new result($status, $results['details']); + } catch (Throwable $e) { + return new result(result::CRITICAL, "TODO lang Error while executing store check type " . $this->type . ': ' . $e->getMessage()); + } + } +} diff --git a/classes/local/manager.php b/classes/local/manager.php index c746b9c2..1c8e94c1 100644 --- a/classes/local/manager.php +++ b/classes/local/manager.php @@ -26,6 +26,7 @@ namespace tool_objectfs\local; use stdClass; +use tool_objectfs\local\store\object_client; use tool_objectfs\local\store\object_file_system; defined('MOODLE_INTERNAL') || die(); @@ -115,7 +116,7 @@ public static function get_objectfs_config() { /** * @param $config - * @return bool + * @return bool|object_client */ public static function get_client($config) { $clientclass = self::get_client_classname_from_fs($config->filesystem); diff --git a/classes/local/store/object_client.php b/classes/local/store/object_client.php index 44bdc306..1d975603 100644 --- a/classes/local/store/object_client.php +++ b/classes/local/store/object_client.php @@ -41,6 +41,7 @@ public function test_connection(); public function test_permissions($testdelete); public function proxy_range_request(\stored_file $file, $ranges); public function test_range_request($filesystem); + public function get_configuration_check_status(); } diff --git a/classes/local/store/object_client_base.php b/classes/local/store/object_client_base.php index cda37840..30d287b9 100644 --- a/classes/local/store/object_client_base.php +++ b/classes/local/store/object_client_base.php @@ -81,6 +81,10 @@ public function generate_presigned_url($contenthash, $headers = array()) { throw new \coding_exception("Pre-signed URLs not supported"); } + public function get_configuration_check_status() { + return ['ok' => false, 'details' => '']; + } + /** * Moodle admin settings form to display connection details for the client service. * diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php index 31288859..1b28d901 100644 --- a/classes/local/store/s3/client.php +++ b/classes/local/store/s3/client.php @@ -92,26 +92,44 @@ protected function is_functional() { return isset($this->client); } + public function get_configuration_check_status() { + $ok = $this->is_configuration_valid($this->config); + + $configcheck = $this->check_configuration($this->config); + $details = ''; + + $lookup = [ // TODO lang strings + true => 'GOOD', + false => "Missing", + null => "N/A", + ]; + + foreach ($configcheck as $check => $result) { + $details .= $check . ":" . $lookup[$result] . "\n"; + } + return ['ok' => $ok, 'details' => $details]; + } + + /** + * Checks the configuration is valid. + * @return array key => value where the value is true if ok, false if bad, or null if n/a + */ + protected function check_configuration($config) { + return [ + 's3_bucket' => !empty($config->s3_bucket), + 's3_region' => !empty($config->s3_region), + 's3_usesdkcreds' => empty($config->s3_usesdkcreds) ? null : (!empty($config->s3_key) && !empty($config->s3_secret)), + ]; + } + /** * Check if the client configured properly. * * @param \stdClass $config Client config. * @return bool */ - protected function is_configured($config) { - if (empty($config->s3_bucket)) { - return false; - } - - if (empty($config->s3_region)) { - return false; - } - - if (empty($config->s3_usesdkcreds) && (empty($config->s3_key) || empty($config->s3_secret))) { - return false; - } - - return true; + protected function is_configuration_valid($config) { + return !in_array(false, $this->check_configuration($config)); } /** @@ -120,7 +138,7 @@ protected function is_configured($config) { * @param \stdClass $config Client config. */ public function set_client($config) { - if (!$this->is_configured($config)) { + if (!$this->is_configuration_valid($config)) { $this->client = null; return; } diff --git a/lib.php b/lib.php index 59159ebe..db1a77a2 100644 --- a/lib.php +++ b/lib.php @@ -117,11 +117,15 @@ function tool_objectfs_pluginfile($course, $cm, context $context, $filearea, arr * @return array */ function tool_objectfs_status_checks() { + $checks = [ + new tool_objectfs\check\configuration(), + new tool_objectfs\check\store_check(\tool_objectfs\check\store_check::TYPE_PERMISSIONS), + new tool_objectfs\check\store_check(\tool_objectfs\check\store_check::TYPE_CONNECTION), + ]; + if (get_config('tool_objectfs', 'proxyrangerequests')) { - return [ - new tool_objectfs\check\proxy_range_request() - ]; + new tool_objectfs\check\store_check(\tool_objectfs\check\store_check::TYPE_RANGEREQUEST); } - return []; + return $checks; } From 13b84975e82b5b194cd6a8295d35075160af501f Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 12 Jan 2024 15:01:42 +1000 Subject: [PATCH 2/5] More tweaks --- classes/check/configuration.php | 16 ++++++++------- classes/check/store_check.php | 24 ++++++++++++++-------- classes/local/store/object_client.php | 2 +- classes/local/store/object_client_base.php | 16 ++++++++++----- classes/local/store/s3/client.php | 22 ++++++++++++-------- lang/en/tool_objectfs.php | 19 +++++++++++++++++ 6 files changed, 69 insertions(+), 30 deletions(-) diff --git a/classes/check/configuration.php b/classes/check/configuration.php index ab593066..33ae237e 100644 --- a/classes/check/configuration.php +++ b/classes/check/configuration.php @@ -6,21 +6,23 @@ use core\check\result; class configuration extends check { - public function get_result(): result - { + public function get_result(): result { // Load objectfs and run a test. $config = \tool_objectfs\local\manager::get_objectfs_config(); $client = \tool_objectfs\local\manager::get_client($config); if (empty($client)) { - return new result(result::WARNING, "TODO lang Client was empty"); + return new result(result::UNKNOWN, get_string('check:configuration:empty', 'tool_objectfs')); } - $configstatus = $client->get_configuration_check_status(); - $status = $configstatus['ok'] ? result::OK : result::ERROR; - $summary = $configstatus['ok'] ? "OK" : "ERRORS"; // TODO lang. - $details = nl2br($configstatus['details']); + $configstatus = $client->test_configuration(); + $status = $configstatus->success ? result::OK : result::ERROR; + $summary = $configstatus->success ? get_string('check:configuration:ok', 'tool_objectfs') + : get_string('check:configuration:error', 'tool_objectfs'); + $details = nl2br($configstatus->details); return new result($status, $summary, $details); } + + // TODO action link } diff --git a/classes/check/store_check.php b/classes/check/store_check.php index b5d052b9..7fc66cf2 100644 --- a/classes/check/store_check.php +++ b/classes/check/store_check.php @@ -34,7 +34,7 @@ public function get_result(): result { try { // Check if configured first, and report NA if not configured. if (!\tool_objectfs\local\manager::check_file_storage_filesystem()) { - return new result(result::NA, "todo lang Not configured."); + return new result(result::NA, get_string('check:notenabled', 'tool_objectfs')); } // Load objectfs and run a test. @@ -42,15 +42,20 @@ public function get_result(): result { $client = \tool_objectfs\local\manager::get_client($config); if (empty($client)) { - return new result(result::UNKNOWN, "TODO lang client not configured"); + return new result(result::UNKNOWN, get_string('check:configuration:empty', 'tool_objectfs')); } - $results = []; + if (!$client->test_configuration()->success) { + // Not confingured yet, don't bother testing connection or permissions. + return new result(result::NA, get_string('check:storecheck:notconfiguredskip', 'tool_objectfs')); + } + + $results = (object) []; // TODO test delete. switch($this->type) { case self::TYPE_CONNECTION: - $results = $client->test_permissions(false); + $results = $client->test_connection(false); break; case self::TYPE_RANGEREQUEST: @@ -63,13 +68,16 @@ public function get_result(): result { } if (empty($results)) { - return new result(result::UNKNOWN, "TODO lang Test type " . $this->type . " was configured, but no test was executed."); + return new result(result::UNKNOWN, get_string('check:storecheck:nothingexecuted', 'tool_objectfs')); } - $status = $results['success'] ? result::OK : result::ERROR; - return new result($status, $results['details']); + $status = $results->success ? result::OK : result::ERROR; + return new result($status, $results->details ?? ''); } catch (Throwable $e) { - return new result(result::CRITICAL, "TODO lang Error while executing store check type " . $this->type . ': ' . $e->getMessage()); + return new result(result::CRITICAL, get_string('check:storecheck:error', 'tool_objectfs') + . $this->type . ': ' . $e->getMessage(), $e->getTraceAsString()); } } + + // TODO action link } diff --git a/classes/local/store/object_client.php b/classes/local/store/object_client.php index 1d975603..b104edc6 100644 --- a/classes/local/store/object_client.php +++ b/classes/local/store/object_client.php @@ -41,7 +41,7 @@ public function test_connection(); public function test_permissions($testdelete); public function proxy_range_request(\stored_file $file, $ranges); public function test_range_request($filesystem); - public function get_configuration_check_status(); + public function test_configuration(); } diff --git a/classes/local/store/object_client_base.php b/classes/local/store/object_client_base.php index 30d287b9..a2db4a79 100644 --- a/classes/local/store/object_client_base.php +++ b/classes/local/store/object_client_base.php @@ -81,10 +81,6 @@ public function generate_presigned_url($contenthash, $headers = array()) { throw new \coding_exception("Pre-signed URLs not supported"); } - public function get_configuration_check_status() { - return ['ok' => false, 'details' => '']; - } - /** * Moodle admin settings form to display connection details for the client service. * @@ -142,7 +138,7 @@ public function proxy_range_request(\stored_file $file, $ranges) { * @return object */ public function test_range_request($filesystem) { - return (object)['result' => false, 'error' => '']; + return (object)['result' => false, 'details' => '']; } /** @@ -165,4 +161,14 @@ public function test_connection() { public function test_permissions($testdelete) { return (object)['success' => false, 'details' => '']; } + + /** + * Tests configuration is OK. + * Override this method in client class. + * + * @return object + */ + public function test_configuration() { + return (object)['success' => false, 'details' => '']; + } } diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php index 1b28d901..6c6c774b 100644 --- a/classes/local/store/s3/client.php +++ b/classes/local/store/s3/client.php @@ -89,25 +89,29 @@ public function __wakeup() { * @return bool */ protected function is_functional() { - return isset($this->client); + return !empty($this->client); } - public function get_configuration_check_status() { + /** + * Tests that the configuration is ok. + * @return object with 'success' and 'details' values. + */ + public function test_configuration() { $ok = $this->is_configuration_valid($this->config); $configcheck = $this->check_configuration($this->config); $details = ''; - $lookup = [ // TODO lang strings - true => 'GOOD', - false => "Missing", - null => "N/A", + $lookup = [ + true => get_string('settings:config:exists', 'tool_objectfs'), + false => get_string('settings:config:missing', 'tool_objectfs'), + null => get_string('settings:config:na', 'tool_objectfs'), ]; foreach ($configcheck as $check => $result) { $details .= $check . ":" . $lookup[$result] . "\n"; } - return ['ok' => $ok, 'details' => $details]; + return (object) ['success' => $ok, 'details' => $details]; } /** @@ -129,7 +133,7 @@ protected function check_configuration($config) { * @return bool */ protected function is_configuration_valid($config) { - return !in_array(false, $this->check_configuration($config)); + return !in_array(false, $this->check_configuration($config), true); } /** @@ -304,7 +308,7 @@ public function test_permissions($testdelete) { $permissions->success = true; $permissions->messages = array(); - if ($this->is_functional()) { + if (!$this->is_functional()) { $permissions->success = false; $permissions->messages = array(); return $permissions; diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index e8f48876..c5dcd65f 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -269,3 +269,22 @@ $string['check:proxyrangerequestsdisabled'] = 'The proxy range request setting is disabled.'; $string['checkproxy_range_request'] = 'Pre-signed URL range request proxy'; + +$string['settings:config:missing'] = 'Missing'; +$string['settings:config:na'] = 'N/A'; +$string['settings:config:exists'] = 'Exists'; + +$string['check:notenabled'] = 'Object storage not enabled'; + +$string['checkconfiguration'] = 'Object storage configuration'; +$string['check:configuration:ok'] = 'Configuration exists'; +$string['check:configuration:error'] = 'Configuration error'; +$string['check:configuration:empty'] = 'Client created from config was empty'; + +$string['checkstore_check_connection'] = 'Object storage connection'; +$string['checkstore_check_permissions'] = 'Object storage permissions'; +$string['checkstore_check_rangerequest'] = 'Object storage range requests'; +$string['check:storecheck:error'] = 'Error while trying to run check: '; +$string['check:storecheck:notconfiguredskip'] = 'Object storage not configured - skipping'; +$string['check:storecheck:nothingexecuted'] = 'No results were returned from the tests'; + From 927a4f692c9f6c07d27445523a8209014c19efae Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 15 Jan 2024 11:30:05 +1000 Subject: [PATCH 3/5] WIP rework of internal methods to return check result instead --- classes/check/configuration.php | 52 +++++++-- classes/check/store.php | 122 +++++++++++++++++++++ classes/check/store_check.php | 83 -------------- classes/local/store/azure/client.php | 30 ++--- classes/local/store/object_client.php | 10 +- classes/local/store/object_client_base.php | 71 +++++++----- classes/local/store/s3/client.php | 122 ++++++++++++--------- classes/local/store/swift/client.php | 38 +++---- lang/en/tool_objectfs.php | 4 + lib.php | 6 +- 10 files changed, 324 insertions(+), 214 deletions(-) create mode 100644 classes/check/store.php delete mode 100644 classes/check/store_check.php diff --git a/classes/check/configuration.php b/classes/check/configuration.php index 33ae237e..d956c0e4 100644 --- a/classes/check/configuration.php +++ b/classes/check/configuration.php @@ -1,28 +1,60 @@ . namespace tool_objectfs\check; +use action_link; use core\check\check; use core\check\result; +use moodle_url; +use tool_objectfs\local\manager; +/** + * Configuration check. + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ class configuration extends check { + /** + * Gets the result of the check + * @return result + */ public function get_result(): result { // Load objectfs and run a test. - $config = \tool_objectfs\local\manager::get_objectfs_config(); - $client = \tool_objectfs\local\manager::get_client($config); + $config = manager::get_objectfs_config(); + $client = manager::get_client($config); + // Something is very wrong if this is false. if (empty($client)) { return new result(result::UNKNOWN, get_string('check:configuration:empty', 'tool_objectfs')); } - $configstatus = $client->test_configuration(); - $status = $configstatus->success ? result::OK : result::ERROR; - $summary = $configstatus->success ? get_string('check:configuration:ok', 'tool_objectfs') - : get_string('check:configuration:error', 'tool_objectfs'); - $details = nl2br($configstatus->details); - - return new result($status, $summary, $details); + return $client->test_configuration(); } - // TODO action link + /** + * Return action link + * @return action_link + */ + public function get_action_link(): ?action_link { + $str = get_string('check:settings', 'tool_objectfs'); + $url = new moodle_url('/admin/category.php', ['category' => 'tool_objectfs']); + return new action_link($url, $str); + } } diff --git a/classes/check/store.php b/classes/check/store.php new file mode 100644 index 00000000..1b2c6c30 --- /dev/null +++ b/classes/check/store.php @@ -0,0 +1,122 @@ +. + +namespace tool_objectfs\check; + +use action_link; +use coding_exception; +use core\check\check; +use core\check\result; +use moodle_url; +use Throwable; +use tool_objectfs\local\manager; + +/** + * Store check. + * + * @package tool_objectfs + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class store extends check { + /** @var string The selected type of store check **/ + private $type; + + /** @var string Connection test type **/ + public const TYPE_CONNECTION = 'connection'; + + /** @var string Permissions test type **/ + public const TYPE_PERMISSIONS = 'permissions'; + + /** @var string Range request test type **/ + public const TYPE_RANGEREQUEST = 'rangerequest'; + + /** @var array Available test types **/ + public const TYPES = [self::TYPE_CONNECTION, self::TYPE_PERMISSIONS, self::TYPE_RANGEREQUEST]; + + /** + * Create a store check for a given type + * @param string $type one of TYPES + */ + public function __construct(string $type) { + if (!in_array($type, self::TYPES)) { + throw new coding_exception("Given test type " . $type . " is not valid."); + } + + $this->type = $type; + } + + /** + * Returns the id - differs based on the type + * @return string + */ + public function get_id(): string { + return "store_check_" . $this->type; + } + + /** + * Return the result + * @return result + */ + public function get_result(): result { + try { + // Check if configured first, and report NA if not configured. + if (!\tool_objectfs\local\manager::check_file_storage_filesystem()) { + return new result(result::NA, get_string('check:notenabled', 'tool_objectfs')); + } + + // Load objectfs and run a test. + $config = manager::get_objectfs_config(); + $client = manager::get_client($config); + + // Something is very wrong if this is empty. + if (empty($client)) { + return new result(result::UNKNOWN, get_string('check:configuration:empty', 'tool_objectfs')); + } + + // If not configured yet, don't bother testing connection or permissions. + if ($client->test_configuration()->get_status() != result::OK) { + return new result(result::NA, get_string('check:storecheck:notconfiguredskip', 'tool_objectfs')); + } + + switch($this->type) { + case self::TYPE_CONNECTION: + return $client->test_connection(false); + + case self::TYPE_RANGEREQUEST: + return $client->test_range_request(new $config->filesystem()); + + case self::TYPE_PERMISSIONS: + return $client->test_permissions(false); + } + } catch (Throwable $e) { + // Usually the SDKs will throw exceptions if something doesn't work, so we want to catch these. + return new result(result::CRITICAL, get_string('check:storecheck:error', 'tool_objectfs') + . $this->type . ': ' . $e->getMessage(), $e->getTraceAsString()); + } + } + + /** + * Return action link + * @return action_link + */ + public function get_action_link(): ?action_link { + $str = get_string('check:settings', 'tool_objectfs'); + $url = new moodle_url('/admin/category.php', ['category' => 'tool_objectfs']); + return new action_link($url, $str); + } +} diff --git a/classes/check/store_check.php b/classes/check/store_check.php deleted file mode 100644 index 7fc66cf2..00000000 --- a/classes/check/store_check.php +++ /dev/null @@ -1,83 +0,0 @@ -type = $type; - } - - public function get_id(): string { - return "store_check_" . $this->type; - } - - public function get_result(): result { - try { - // Check if configured first, and report NA if not configured. - if (!\tool_objectfs\local\manager::check_file_storage_filesystem()) { - return new result(result::NA, get_string('check:notenabled', 'tool_objectfs')); - } - - // Load objectfs and run a test. - $config = \tool_objectfs\local\manager::get_objectfs_config(); - $client = \tool_objectfs\local\manager::get_client($config); - - if (empty($client)) { - return new result(result::UNKNOWN, get_string('check:configuration:empty', 'tool_objectfs')); - } - - if (!$client->test_configuration()->success) { - // Not confingured yet, don't bother testing connection or permissions. - return new result(result::NA, get_string('check:storecheck:notconfiguredskip', 'tool_objectfs')); - } - - $results = (object) []; - - // TODO test delete. - switch($this->type) { - case self::TYPE_CONNECTION: - $results = $client->test_connection(false); - break; - - case self::TYPE_RANGEREQUEST: - $results = $client->test_range_request(new $config->filesystem()); - break; - - case self::TYPE_PERMISSIONS: - $results = $client->test_permissions(false); - break; - } - - if (empty($results)) { - return new result(result::UNKNOWN, get_string('check:storecheck:nothingexecuted', 'tool_objectfs')); - } - - $status = $results->success ? result::OK : result::ERROR; - return new result($status, $results->details ?? ''); - } catch (Throwable $e) { - return new result(result::CRITICAL, get_string('check:storecheck:error', 'tool_objectfs') - . $this->type . ': ' . $e->getMessage(), $e->getTraceAsString()); - } - } - - // TODO action link -} diff --git a/classes/local/store/azure/client.php b/classes/local/store/azure/client.php index d4441249..974f7301 100644 --- a/classes/local/store/azure/client.php +++ b/classes/local/store/azure/client.php @@ -25,6 +25,7 @@ namespace tool_objectfs\local\store\azure; +use core\check\result; use SimpleXMLElement; use stdClass; use tool_objectfs\local\store\azure\stream_wrapper; @@ -202,25 +203,23 @@ protected function get_filepath_from_hash($contenthash) { return "$l1/$l2/$contenthash"; } - public function test_connection() { - $connection = new \stdClass(); - $connection->success = true; - $connection->details = ''; - + /** + * Tests the connection + * @return result + */ + public function test_connection(): result { try { $this->client->createBlockBlob($this->container, 'connection_check_file', 'connection_check_file'); } catch (\MicrosoftAzure\Storage\Common\Exceptions\ServiceException $e) { - $connection->success = false; - $connection->details = $this->get_exception_details($e); + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), $this->get_exception_details($e)); } catch (\GuzzleHttp\Exception\ConnectException $e) { - $connection->success = false; - $connection->details = $e->getMessage(); + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), $e->getMessage()); } - return $connection; + return new result(result::OK, get_string('check:passed', 'tool_objectfs')); } - public function test_permissions($testdelete) { + public function test_permissions($testdelete): result { $permissions = new \stdClass(); $permissions->success = true; $permissions->messages = array(); @@ -261,11 +260,12 @@ public function test_permissions($testdelete) { } } - if ($permissions->success) { - $permissions->messages[get_string('settings:permissioncheckpassed', 'tool_objectfs')] = 'notifysuccess'; - } + $status = $permissions->success ? result::OK : result::ERROR; + $summarystr = result::OK ? 'check:passed' : 'check:failed'; + $summary = get_string($summarystr, 'tool_objectfs'); + $details = implode("\n", $permissions->messages); - return $permissions; + return new result($status, $summary, $details); } protected function get_exception_details(\MicrosoftAzure\Storage\Common\Exceptions\ServiceException $exception) { diff --git a/classes/local/store/object_client.php b/classes/local/store/object_client.php index b104edc6..3fe3b635 100644 --- a/classes/local/store/object_client.php +++ b/classes/local/store/object_client.php @@ -25,6 +25,8 @@ namespace tool_objectfs\local\store; +use core\check\result; + interface object_client { public function __construct($config); public function register_stream_wrapper(); @@ -37,11 +39,11 @@ public function get_maximum_upload_size(); public function verify_object($contenthash, $localpath); public function generate_presigned_url($contenthash, $headers = array()); public function support_presigned_urls(); - public function test_connection(); - public function test_permissions($testdelete); + public function test_connection(): result; + public function test_permissions($testdelete): result; public function proxy_range_request(\stored_file $file, $ranges); - public function test_range_request($filesystem); - public function test_configuration(); + public function test_range_request($filesystem): result; + public function test_configuration(): result; } diff --git a/classes/local/store/object_client_base.php b/classes/local/store/object_client_base.php index a2db4a79..389a99d6 100644 --- a/classes/local/store/object_client_base.php +++ b/classes/local/store/object_client_base.php @@ -25,6 +25,8 @@ namespace tool_objectfs\local\store; +use core\check\result; + abstract class object_client_base implements object_client { protected $autoloader; @@ -89,25 +91,40 @@ public function generate_presigned_url($contenthash, $headers = array()) { */ public function define_client_check() { global $OUTPUT; + + // TODO use MDL check api admin setting if available ??? + + $checks = tool_objectfs_status_checks(); + $output = ''; - $connection = $this->test_connection(); - if ($connection->success) { - $output .= $OUTPUT->notification(get_string('settings:connectionsuccess', 'tool_objectfs'), 'notifysuccess'); - // Check permissions if we can connect. - $permissions = $this->test_permissions($this->testdelete); - if ($permissions->success) { - $output .= $OUTPUT->notification(key($permissions->messages), 'notifysuccess'); - } else { - foreach ($permissions->messages as $message => $type) { - $output .= $OUTPUT->notification($message, $type); - } + + foreach ($checks as $check) { + // This is fixed in 4.4 by MDL-67898. + // But until that is more common, the component must be set manually, + // as the default is incorrect when viewed in the admin tree. + $check->set_component('tool_objectfs'); + + $result = $check->get_result(); + + $status = $result->get_status(); + + // If status was N/A - ignore it. + if ($status == result::NA) { + continue; } - } else { - $output .= $OUTPUT->notification( - get_string('settings:connectionfailure', 'tool_objectfs', $connection->details), - 'notifyproblem' - ); + + $str = $check->get_name() . ": " . $result->get_summary(); + + // Only include details if failure. + if ($status != result::OK) { + $str .= "\n" . $result->get_details(); + } + + $type = $status == result::OK ? 'notifysuccess' : 'notifyerror'; + + $output .= $OUTPUT->notification(nl2br($str), $type); } + return $output; } @@ -135,20 +152,20 @@ public function proxy_range_request(\stored_file $file, $ranges) { * Test proxy range request. * * @param object $filesystem Filesystem to be tested. - * @return object + * @return result */ - public function test_range_request($filesystem) { - return (object)['result' => false, 'details' => '']; + public function test_range_request($filesystem): result { + return new result(result::UNKNOWN, ''); } /** * Tests connection to external storage. * Override this method in client class. * - * @return object + * @return result */ - public function test_connection() { - return (object)['success' => false, 'details' => '']; + public function test_connection(): result { + return new result(result::UNKNOWN, ''); } /** @@ -158,17 +175,17 @@ public function test_connection() { * @param bool $testdelete Test delete permission and fail the test if could delete object from the storage. * @return object */ - public function test_permissions($testdelete) { - return (object)['success' => false, 'details' => '']; + public function test_permissions($testdelete): result { + return new result(result::UNKNOWN, ''); } /** * Tests configuration is OK. * Override this method in client class. * - * @return object + * @return result */ - public function test_configuration() { - return (object)['success' => false, 'details' => '']; + public function test_configuration(): result { + return new result(result::UNKNOWN, ''); } } diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php index 6c6c774b..60e295e3 100644 --- a/classes/local/store/s3/client.php +++ b/classes/local/store/s3/client.php @@ -25,6 +25,7 @@ namespace tool_objectfs\local\store\s3; +use core\check\result; use tool_objectfs\local\manager; use tool_objectfs\local\store\object_client_base; use tool_objectfs\local\store\signed_url; @@ -48,6 +49,9 @@ class client extends object_client_base { protected $bucket; private $signingmethod; + /** @var string key prefix for the bucket **/ + private $bucketkeyprefix; + public function __construct($config) { global $CFG; $this->autoloader = $CFG->dirroot . '/local/aws/sdk/aws-autoloader.php'; @@ -94,11 +98,9 @@ protected function is_functional() { /** * Tests that the configuration is ok. - * @return object with 'success' and 'details' values. + * @return result */ - public function test_configuration() { - $ok = $this->is_configuration_valid($this->config); - + public function test_configuration(): result { $configcheck = $this->check_configuration($this->config); $details = ''; @@ -111,7 +113,14 @@ public function test_configuration() { foreach ($configcheck as $check => $result) { $details .= $check . ":" . $lookup[$result] . "\n"; } - return (object) ['success' => $ok, 'details' => $details]; + + $valid = $this->is_configuration_valid($this->config); + $status = $valid ? result::OK : result::ERROR; + $summary = $valid ? get_string('check:configuration:ok', 'tool_objectfs') + : get_string('check:configuration:error', 'tool_objectfs'); + $details = nl2br($details); + + return new result($status, $summary, $details); } /** @@ -266,33 +275,33 @@ protected function get_filepath_from_hash($contenthash) { * There is no check connection in the AWS API. * We use list buckets instead and check the bucket is in the list. * - * @return object + * @return result * @throws \coding_exception */ - public function test_connection() { - $connection = new \stdClass(); - $connection->success = true; - $connection->details = ''; + public function test_connection(): result { + $status = result::OK; + $details = ''; try { if (!$this->is_functional()) { - $connection->success = false; - $connection->details = get_string('settings:notconfigured', 'tool_objectfs'); + return new result(result::NA, get_string('settings:notconfigured', 'tool_objectfs')); } else { $this->client->headBucket(array('Bucket' => $this->bucket)); } } catch (\Aws\S3\Exception\S3Exception $e) { - $connection->success = false; - $connection->details = $this->get_exception_details($e); + $status = result::ERROR; + $details = $this->get_exception_details($e); } catch (\GuzzleHttp\Exception\InvalidArgumentException $e) { - $connection->success = false; - $connection->details = $this->get_exception_details($e); + $status = result::ERROR; + $details = $this->get_exception_details($e); } catch (\Aws\Exception\CredentialsException $e) { - $connection->success = false; - $connection->details = $this->get_exception_details($e); + $status = result::ERROR; + $details = $this->get_exception_details($e); } - return $connection; + $summarystr = result::OK ? 'check:passed' : 'check:failed'; + $summary = get_string($summarystr, 'tool_objectfs'); + return new result($status, $summary, $details); } /** @@ -300,66 +309,68 @@ public function test_connection() { * There is no check connection in the AWS API. * We use list buckets instead and check the bucket is in the list. * - * @return object + * @return result * @throws \coding_exception */ - public function test_permissions($testdelete) { - $permissions = new \stdClass(); - $permissions->success = true; - $permissions->messages = array(); - + public function test_permissions($testdelete): result { if (!$this->is_functional()) { - $permissions->success = false; - $permissions->messages = array(); - return $permissions; + return new result(result::NA, ''); } + $status = result::OK; + $details = ''; + + // Try to put an object. try { - $result = $this->client->putObject(array( + $this->client->putObject([ 'Bucket' => $this->bucket, 'Key' => $this->bucketkeyprefix . 'permissions_check_file', - 'Body' => 'test content')); + 'Body' => 'test content', + ]); } catch (\Aws\S3\Exception\S3Exception $e) { - $details = $this->get_exception_details($e); - $permissions->messages[get_string('settings:writefailure', 'tool_objectfs') . $details] = 'notifyproblem'; - $permissions->success = false; + $exdetails = $this->get_exception_details($e); + $details .= get_string('settings:writefailure', 'tool_objectfs') . $exdetails; + $status = result::ERROR; } + // Try to get an object. try { - $result = $this->client->getObject(array( + $this->client->getObject([ 'Bucket' => $this->bucket, - 'Key' => $this->bucketkeyprefix . 'permissions_check_file')); + 'Key' => $this->bucketkeyprefix . 'permissions_check_file', + ]); } catch (\Aws\S3\Exception\S3Exception $e) { $errorcode = $e->getAwsErrorCode(); // Write could have failed. if ($errorcode !== 'NoSuchKey') { - $details = $this->get_exception_details($e); - $permissions->messages[get_string('settings:permissionreadfailure', 'tool_objectfs') . $details] = 'notifyproblem'; - $permissions->success = false; + $exdetails = $this->get_exception_details($e); + $details .= get_string('settings:permissionreadfailure', 'tool_objectfs') . $exdetails; + $status = result::ERROR; } } + // Try to delete an object. if ($testdelete) { try { - $result = $this->client->deleteObject(array('Bucket' => $this->bucket, 'Key' => $this->bucketkeyprefix . 'permissions_check_file')); - $permissions->messages[get_string('settings:deletesuccess', 'tool_objectfs')] = 'warning'; - $permissions->success = false; + $this->client->deleteObject([ + 'Bucket' => $this->bucket, + 'Key' => $this->bucketkeyprefix . 'permissions_check_file', + ]); + $details .= get_string('settings:deletesuccess', 'tool_objectfs'); } catch (\Aws\S3\Exception\S3Exception $e) { $errorcode = $e->getAwsErrorCode(); // Something else went wrong. if ($errorcode !== 'AccessDenied') { $details = $this->get_exception_details($e); - $permissions->messages[get_string('settings:deleteerror', 'tool_objectfs') . $details] = 'notifyproblem'; - $permissions->success = false; + $details .= get_string('settings:deleteerror', 'tool_objectfs') . $details; + $status = result::ERROR; } } } - if ($permissions->success) { - $permissions->messages[get_string('settings:permissioncheckpassed', 'tool_objectfs')] = 'notifysuccess'; - } - - return $permissions; + $summarystr = result::OK ? 'settings:permissioncheckpassed' : 'settings:permissioncheckfailed'; + $summary = get_string($summarystr, 'tool_objectfs'); + return new result($status, $summary, $details); } protected function get_exception_details($exception) { @@ -802,11 +813,12 @@ public function curl_range_request_to_presigned_url($contenthash, $ranges, $head * Test proxy range request. * * @param object $filesystem Filesystem to be tested. - * @return object + * @return result * @throws \coding_exception */ - public function test_range_request($filesystem) { + public function test_range_request($filesystem): result { global $PAGE; + $output = $PAGE->get_renderer('tool_objectfs'); $testfiles = $output->presignedurl_tests_load_files($filesystem); foreach ($testfiles as $file) { @@ -816,13 +828,17 @@ public function test_range_request($filesystem) { $ranges, ['Expires' => time() + HOURSECS]); $httpcode = manager::get_header($response['responseheaders'], 'HTTP/1.1'); if ($response['content'] != '' && $httpcode == '206 Partial Content') { - return (object)['result' => true]; + // Range request OK. + return new result(result::OK, get_string('check:passed', 'tool_objectfs')); } else { + // Range request failed. $a = (object)['url' => $response['url'], 'httpcode' => $httpcode, 'details' => $response['content']]; - return (object)['result' => false, 'error' => get_string('rangerequestfailed', 'tool_objectfs', $a)]; + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), get_string('rangerequestfailed', 'tool_objectfs', $a)); } } } - return (object)['result' => false, 'error' => get_string('fixturefilemissing', 'tool_objectfs')]; + + // Unknown - fixture missing cannot test it. + return new result(result::UNKNOWN, get_string('fixturefilemissing', 'tool_objectfs')); } } diff --git a/classes/local/store/swift/client.php b/classes/local/store/swift/client.php index e95a0fe0..8876daca 100644 --- a/classes/local/store/swift/client.php +++ b/classes/local/store/swift/client.php @@ -24,6 +24,7 @@ namespace tool_objectfs\local\store\swift; +use core\check\result; use tool_objectfs\local\store\swift\stream_wrapper; use tool_objectfs\local\store\object_client_base; use tool_objectfs\local\manager; @@ -187,19 +188,15 @@ protected function get_filepath_from_hash($contenthash) { return "$l1/$l2/$contenthash"; } - - public function test_connection() { - - $connection = new \stdClass(); - $connection->success = true; - $connection->details = ''; - + /** + * Tests connection and returns result. + * @return result + */ + public function test_connection(): result { try { $container = $this->get_container(); } catch (\Exception $e) { - $connection->success = false; - $connection->details = $e->getMessage(); - return $connection; + return new result(result::ERROR, $e->getMessage()); } try { @@ -209,22 +206,20 @@ public function test_connection() { try { $container->createObject(['name' => 'connection_check_file', 'content' => 'connection_check_file']); } catch (\OpenStack\Common\Error\BadResponseError $e) { - $connection->success = false; - $connection->details = $this->get_exception_details($e); + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), $this->get_exception_details($e)); } catch (\Exception $e) { - $connection->success = false; + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), $e->getMessage()); } } else { - $details = $this->get_exception_details($e); - $connection->messages[get_string('settings:connectionreadfailure', 'tool_objectfs') . $details] = 'notifyproblem'; - $connection->success = false; + $details = get_string('settings:connectionreadfailure', 'tool_objectfs') . $this->get_exception_details($e); + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), $details); } } - return $connection; + return new result(result::OK, get_string('check:success', 'tool_objectfs')); } - public function test_permissions($testdelete) { + public function test_permissions($testdelete): result { $permissions = new \stdClass(); $permissions->success = true; $permissions->messages = array(); @@ -267,7 +262,12 @@ public function test_permissions($testdelete) { $permissions->messages[get_string('settings:permissioncheckpassed', 'tool_objectfs')] = 'notifysuccess'; } - return $permissions; + $status = $permissions->success ? result::OK : result::ERROR; + $summarystr = result::OK ? 'check:passed' : 'check:failed'; + $summary = get_string($summarystr, 'tool_objectfs'); + $details = implode("\n", $permissions->messages); + + return new result($status, $summary, $details); } protected function get_exception_details(\OpenStack\Common\Error\BadResponseError $e) { diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index c5dcd65f..57ec3fb6 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -257,6 +257,7 @@ $string['settings:deletesuccess'] = 'Could delete file from object storage - It is not recommended for the user to have delete permissions. '; $string['settings:deleteerror'] = 'Could not delete permissions check file from object storage. '; $string['settings:permissioncheckpassed'] = 'Permissions check passed.'; +$string['settings:permissioncheckfailed'] = 'Permissions check failed.'; $string['settings:handlernotset'] = '$CFG->alternative_file_system_class is not set, the file system will not be able to read from object storage. Background tasks can still function.'; $string['settings:testingheader'] = 'Test Settings'; @@ -275,6 +276,9 @@ $string['settings:config:exists'] = 'Exists'; $string['check:notenabled'] = 'Object storage not enabled'; +$string['check:settings'] = 'Objectfs settings'; +$string['check:passed'] = 'Check passed'; +$string['check:failed'] = 'Check failed'; $string['checkconfiguration'] = 'Object storage configuration'; $string['check:configuration:ok'] = 'Configuration exists'; diff --git a/lib.php b/lib.php index db1a77a2..765622a4 100644 --- a/lib.php +++ b/lib.php @@ -119,12 +119,12 @@ function tool_objectfs_pluginfile($course, $cm, context $context, $filearea, arr function tool_objectfs_status_checks() { $checks = [ new tool_objectfs\check\configuration(), - new tool_objectfs\check\store_check(\tool_objectfs\check\store_check::TYPE_PERMISSIONS), - new tool_objectfs\check\store_check(\tool_objectfs\check\store_check::TYPE_CONNECTION), + new tool_objectfs\check\store(\tool_objectfs\check\store::TYPE_PERMISSIONS), + new tool_objectfs\check\store(\tool_objectfs\check\store::TYPE_CONNECTION), ]; if (get_config('tool_objectfs', 'proxyrangerequests')) { - new tool_objectfs\check\store_check(\tool_objectfs\check\store_check::TYPE_RANGEREQUEST); + new tool_objectfs\check\store(\tool_objectfs\check\store::TYPE_RANGEREQUEST); } return $checks; From 78be6db43fb27ef8308d70271fddda60ae157a8f Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 15 Jan 2024 11:43:55 +1000 Subject: [PATCH 4/5] Some tweaks --- classes/local/store/s3/client.php | 4 ++-- settings.php | 21 --------------------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php index 60e295e3..2cbec0c8 100644 --- a/classes/local/store/s3/client.php +++ b/classes/local/store/s3/client.php @@ -299,7 +299,7 @@ public function test_connection(): result { $details = $this->get_exception_details($e); } - $summarystr = result::OK ? 'check:passed' : 'check:failed'; + $summarystr = $status == result::OK ? 'check:passed' : 'check:failed'; $summary = get_string($summarystr, 'tool_objectfs'); return new result($status, $summary, $details); } @@ -368,7 +368,7 @@ public function test_permissions($testdelete): result { } } - $summarystr = result::OK ? 'settings:permissioncheckpassed' : 'settings:permissioncheckfailed'; + $summarystr = $status == result::OK ? 'settings:permissioncheckpassed' : 'settings:permissioncheckfailed'; $summary = get_string($summarystr, 'tool_objectfs'); return new result($status, $summary, $details); } diff --git a/settings.php b/settings.php index a941e506..98829582 100644 --- a/settings.php +++ b/settings.php @@ -158,27 +158,6 @@ new lang_string('settings:presignedurl:header', 'tool_objectfs'), $warningtext)); if ($classexists) { - $connstatus = false; - if ($objectfspage) { - $testconn = $client->test_connection(); - $connstatus = $testconn->success; - } - - $warningtext = ''; - $methodexists = method_exists('file_system', 'xsendfile_file'); - if (!$methodexists) { - $warningtext .= $OUTPUT->notification(get_string('settings:presignedurl:xsendfilefile', 'tool_objectfs')); - } else if ($connstatus) { - // Range request tests can only work if there is a valid connection. - $range = $client->test_range_request(new $config->filesystem()); - if ($range->result) { - $warningtext .= $OUTPUT->notification(get_string('settings:presignedurl:testrangeok', 'tool_objectfs'), - 'notifysuccess'); - } else { - $warningtext .= $OUTPUT->notification(get_string('settings:presignedurl:testrangeerror', 'tool_objectfs')); - $warningtext .= $OUTPUT->notification($range->error); - } - } $settings->add(new admin_setting_configcheckbox('tool_objectfs/proxyrangerequests', new lang_string('settings:presignedurl:proxyrangerequests', 'tool_objectfs'), new lang_string('settings:presignedurl:proxyrangerequests_help', 'tool_objectfs') . $warningtext, '1')); From 0f691ddaa61ae3ce8e8fc05f4c34bf50c357b78a Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 16 Jan 2024 14:14:41 +1000 Subject: [PATCH 5/5] Fixups --- classes/check/proxy_range_request.php | 69 ---------------------- classes/check/store.php | 7 ++- classes/local/store/azure/client.php | 4 ++ classes/local/store/object_client_base.php | 8 +-- classes/local/store/s3/client.php | 42 +++++-------- classes/local/store/swift/client.php | 7 ++- lang/en/tool_objectfs.php | 2 + 7 files changed, 38 insertions(+), 101 deletions(-) delete mode 100644 classes/check/proxy_range_request.php diff --git a/classes/check/proxy_range_request.php b/classes/check/proxy_range_request.php deleted file mode 100644 index e9969503..00000000 --- a/classes/check/proxy_range_request.php +++ /dev/null @@ -1,69 +0,0 @@ -. - -namespace tool_objectfs\check; -use core\check\check; -use core\check\result; - -/** - * Status check for objectFS proxied range requests. - * - * @package tool_objectfs - * @author Peter Burnett - * @copyright Catalyst IT - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ -class proxy_range_request extends check { - /** - * Link to ObjectFS settings page. - * - * @return \action_link|null - */ - public function get_action_link(): ?\action_link { - $url = new \moodle_url('/admin/category.php', ['category' => 'tool_objectfs']); - return new \action_link($url, get_string('pluginname', 'tool_objectfs')); - } - - /** - * Check for the success of a proxied range request, if the setting is enabled. - * - * @return result - */ - public function get_result(): result { - $config = \tool_objectfs\local\manager::get_objectfs_config(); - $client = \tool_objectfs\local\manager::get_client($config); - - $signingsupport = false; - if (!empty($config->filesystem)) { - $signingsupport = (new $config->filesystem())->supports_presigned_urls(); - } - - $testconn = $client->test_connection(); - $connstatus = $testconn->success; - - if ($connstatus && $signingsupport) { - $range = $client->test_range_request(new $config->filesystem()); - - if ($range->result) { - return new result(result::OK, get_string('settings:presignedurl:testrangeok', 'tool_objectfs')); - } else { - return new result(result::WARNING, get_string('settings:presignedurl:testrangeerror', 'tool_objectfs')); - } - } - - return new result(result::UNKNOWN, get_string('check:proxyrangerequestsdisabled', 'tool_objectfs')); - } -} diff --git a/classes/check/store.php b/classes/check/store.php index 1b2c6c30..ed2421b7 100644 --- a/classes/check/store.php +++ b/classes/check/store.php @@ -98,6 +98,11 @@ public function get_result(): result { return $client->test_connection(false); case self::TYPE_RANGEREQUEST: + // Range requests require presigned url support. If not supported, return N/A. + if (!$client->support_presigned_urls()) { + return new result(result::NA, get_string('check:storecheck:unsupportedrangerequest', 'tool_objectfs')); + } + return $client->test_range_request(new $config->filesystem()); case self::TYPE_PERMISSIONS: @@ -105,7 +110,7 @@ public function get_result(): result { } } catch (Throwable $e) { // Usually the SDKs will throw exceptions if something doesn't work, so we want to catch these. - return new result(result::CRITICAL, get_string('check:storecheck:error', 'tool_objectfs') + return new result(result::ERROR, get_string('check:storecheck:error', 'tool_objectfs') . $this->type . ': ' . $e->getMessage(), $e->getTraceAsString()); } } diff --git a/classes/local/store/azure/client.php b/classes/local/store/azure/client.php index 974f7301..073dde62 100644 --- a/classes/local/store/azure/client.php +++ b/classes/local/store/azure/client.php @@ -219,6 +219,10 @@ public function test_connection(): result { return new result(result::OK, get_string('check:passed', 'tool_objectfs')); } + /** + * Tests permission and returns result + * @return result + */ public function test_permissions($testdelete): result { $permissions = new \stdClass(); $permissions->success = true; diff --git a/classes/local/store/object_client_base.php b/classes/local/store/object_client_base.php index 389a99d6..ce8f8cd1 100644 --- a/classes/local/store/object_client_base.php +++ b/classes/local/store/object_client_base.php @@ -155,7 +155,7 @@ public function proxy_range_request(\stored_file $file, $ranges) { * @return result */ public function test_range_request($filesystem): result { - return new result(result::UNKNOWN, ''); + return new result(result::INFO, get_string('check:notimplemented', 'tool_objectfs')); } /** @@ -165,7 +165,7 @@ public function test_range_request($filesystem): result { * @return result */ public function test_connection(): result { - return new result(result::UNKNOWN, ''); + return new result(result::INFO, get_string('check:notimplemented', 'tool_objectfs')); } /** @@ -176,7 +176,7 @@ public function test_connection(): result { * @return object */ public function test_permissions($testdelete): result { - return new result(result::UNKNOWN, ''); + return new result(result::INFO, get_string('check:notimplemented', 'tool_objectfs')); } /** @@ -186,6 +186,6 @@ public function test_permissions($testdelete): result { * @return result */ public function test_configuration(): result { - return new result(result::UNKNOWN, ''); + return new result(result::INFO, get_string('check:notimplemented', 'tool_objectfs')); } } diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php index 2cbec0c8..a3386f00 100644 --- a/classes/local/store/s3/client.php +++ b/classes/local/store/s3/client.php @@ -111,7 +111,7 @@ public function test_configuration(): result { ]; foreach ($configcheck as $check => $result) { - $details .= $check . ":" . $lookup[$result] . "\n"; + $details .= $check . ": " . $lookup[$result] . "\n"; } $valid = $this->is_configuration_valid($this->config); @@ -128,11 +128,14 @@ public function test_configuration(): result { * @return array key => value where the value is true if ok, false if bad, or null if n/a */ protected function check_configuration($config) { - return [ + $configcheck = [ 's3_bucket' => !empty($config->s3_bucket), 's3_region' => !empty($config->s3_region), - 's3_usesdkcreds' => empty($config->s3_usesdkcreds) ? null : (!empty($config->s3_key) && !empty($config->s3_secret)), + 's3_key' => !empty($config->s3_key), + 's3_secret' => !empty($config->s3_secret), ]; + + return $configcheck; } /** @@ -279,9 +282,6 @@ protected function get_filepath_from_hash($contenthash) { * @throws \coding_exception */ public function test_connection(): result { - $status = result::OK; - $details = ''; - try { if (!$this->is_functional()) { return new result(result::NA, get_string('settings:notconfigured', 'tool_objectfs')); @@ -289,19 +289,14 @@ public function test_connection(): result { $this->client->headBucket(array('Bucket' => $this->bucket)); } } catch (\Aws\S3\Exception\S3Exception $e) { - $status = result::ERROR; - $details = $this->get_exception_details($e); + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), $this->get_exception_details($e)); } catch (\GuzzleHttp\Exception\InvalidArgumentException $e) { - $status = result::ERROR; - $details = $this->get_exception_details($e); + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), $this->get_exception_details($e)); } catch (\Aws\Exception\CredentialsException $e) { - $status = result::ERROR; - $details = $this->get_exception_details($e); + return new result(result::ERROR, get_string('check:failed', 'tool_objectfs'), $this->get_exception_details($e)); } - $summarystr = $status == result::OK ? 'check:passed' : 'check:failed'; - $summary = get_string($summarystr, 'tool_objectfs'); - return new result($status, $summary, $details); + return new result(result::OK, get_string('check:passed', 'tool_objectfs')); } /** @@ -328,9 +323,8 @@ public function test_permissions($testdelete): result { 'Body' => 'test content', ]); } catch (\Aws\S3\Exception\S3Exception $e) { - $exdetails = $this->get_exception_details($e); - $details .= get_string('settings:writefailure', 'tool_objectfs') . $exdetails; - $status = result::ERROR; + $details = $this->get_exception_details($e); + return new result(result::ERROR, get_string('settings:writefailure', 'tool_objectfs'), $details); } // Try to get an object. @@ -343,9 +337,8 @@ public function test_permissions($testdelete): result { $errorcode = $e->getAwsErrorCode(); // Write could have failed. if ($errorcode !== 'NoSuchKey') { - $exdetails = $this->get_exception_details($e); - $details .= get_string('settings:permissionreadfailure', 'tool_objectfs') . $exdetails; - $status = result::ERROR; + $details = $this->get_exception_details($e); + return new result(result::ERROR, get_string('settings:permissionreadfailure', 'tool_objectfs'), $details); } } @@ -362,15 +355,12 @@ public function test_permissions($testdelete): result { // Something else went wrong. if ($errorcode !== 'AccessDenied') { $details = $this->get_exception_details($e); - $details .= get_string('settings:deleteerror', 'tool_objectfs') . $details; - $status = result::ERROR; + return new result(result::ERROR, get_string('settings:deleteerror', 'tool_objectfs'), $details); } } } - $summarystr = $status == result::OK ? 'settings:permissioncheckpassed' : 'settings:permissioncheckfailed'; - $summary = get_string($summarystr, 'tool_objectfs'); - return new result($status, $summary, $details); + return new result(result::OK , get_string('settings:permissioncheckpassed', 'tool_objectfs')); } protected function get_exception_details($exception) { diff --git a/classes/local/store/swift/client.php b/classes/local/store/swift/client.php index 8876daca..8328306e 100644 --- a/classes/local/store/swift/client.php +++ b/classes/local/store/swift/client.php @@ -216,9 +216,14 @@ public function test_connection(): result { } } - return new result(result::OK, get_string('check:success', 'tool_objectfs')); + return new result(result::OK, get_string('check:passed', 'tool_objectfs')); } + /** + * Tests permission and returns the result + * @param bool $testdelete If deletion should be tested + * @return result + */ public function test_permissions($testdelete): result { $permissions = new \stdClass(); $permissions->success = true; diff --git a/lang/en/tool_objectfs.php b/lang/en/tool_objectfs.php index 57ec3fb6..2013bcf2 100644 --- a/lang/en/tool_objectfs.php +++ b/lang/en/tool_objectfs.php @@ -279,6 +279,7 @@ $string['check:settings'] = 'Objectfs settings'; $string['check:passed'] = 'Check passed'; $string['check:failed'] = 'Check failed'; +$string['check:notimplemented'] = 'Check not implemented by storage client'; $string['checkconfiguration'] = 'Object storage configuration'; $string['check:configuration:ok'] = 'Configuration exists'; @@ -291,4 +292,5 @@ $string['check:storecheck:error'] = 'Error while trying to run check: '; $string['check:storecheck:notconfiguredskip'] = 'Object storage not configured - skipping'; $string['check:storecheck:nothingexecuted'] = 'No results were returned from the tests'; +$string['check:storecheck:unsupportedrangerequest'] = 'File storage does not support presigned urls, so cannot test range requests.';