From b03af8142db04a75062dab8a26ab1f50ab39392b Mon Sep 17 00:00:00 2001 From: Peter Mayer Date: Thu, 17 Sep 2026 00:41:20 +0200 Subject: [PATCH 1/4] MBS-11338: Add multirow query results webservice for monitoring --- classes/external.php | 80 ++++++++++++++++++++++++++++++ db/services.php | 15 ++++++ tests/webservice_test.php | 101 ++++++++++++++++++++++++++++++++++++++ version.php | 2 +- 4 files changed, 197 insertions(+), 1 deletion(-) diff --git a/classes/external.php b/classes/external.php index 7cdf60e..55ce2f1 100644 --- a/classes/external.php +++ b/classes/external.php @@ -36,6 +36,86 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class report_customsql_external extends core_external\external_api { + /** + * Describe the parameters for a complete query result. + * + * @return external_function_parameters + */ + public static function get_query_result_parameters() { + return new external_function_parameters([ + 'queryname' => new external_value(PARAM_TEXT, 'Name of the query.'), + ]); + } + + /** + * Execute a predefined query without the report display row limit. + * + * @param string $queryname Name of the saved query. + * @return array Column names and rows containing nullable string values. + */ + public static function get_query_result($queryname) { + global $DB; + + $params = self::validate_parameters(self::get_query_result_parameters(), ['queryname' => $queryname]); + $context = \context_system::instance(); + self::validate_context($context); + require_capability('report/customsql:view', $context); + + $report = $DB->get_record('report_customsql_queries', ['displayname' => $params['queryname']]); + if (!$report) { + throw new \moodle_exception('invalidreportid', 'report_customsql'); + } + if (!empty($report->capability)) { + require_capability($report->capability, $context); + } + + $queryparams = []; + if (!empty($report->queryparams)) { + $queryparams = json_decode($report->queryparams, true); + if (!is_array($queryparams)) { + $queryparams = unserialize($report->queryparams, ['allowed_classes' => false]); + } + if (!is_array($queryparams)) { + throw new \invalid_parameter_exception('Invalid saved query parameters.'); + } + } + + $sql = report_customsql_prepare_sql($report, time()); + $recordset = report_customsql_execute_query($sql, $queryparams, 0); + $result = ['columns' => [], 'rows' => []]; + try { + foreach ($recordset as $record) { + if (!$result['columns']) { + $result['columns'] = array_keys((array) $record); + } + $values = []; + foreach ($record as $value) { + $values[] = $value === null ? null : (string) $value; + } + $result['rows'][] = ['values' => $values]; + } + } finally { + $recordset->close(); + } + return $result; + } + + /** + * Describe the complete query result. + * + * @return external_single_structure + */ + public static function get_query_result_returns() { + return new external_single_structure([ + 'columns' => new external_multiple_structure(new external_value(PARAM_RAW, 'SQL column name.')), + 'rows' => new external_multiple_structure(new external_single_structure([ + 'values' => new external_multiple_structure( + new external_value(PARAM_RAW, 'Cell value as text, or null for SQL NULL.', VALUE_REQUIRED, null, NULL_ALLOWED) + ), + ])), + ]); + } + /** * Returns description of method parameters * diff --git a/db/services.php b/db/services.php index 5926a45..8e198fc 100644 --- a/db/services.php +++ b/db/services.php @@ -25,6 +25,15 @@ defined('MOODLE_INTERNAL') || die(); $functions = [ + 'report_customsql_get_query_result' => [ + 'classname' => 'report_customsql_external', + 'methodname' => 'get_query_result', + 'classpath' => 'report/customsql/classes/external.php', + 'description' => 'Execute a predefined query and return all result rows.', + 'capabilities' => 'report/customsql:view', + 'type' => 'read', + 'ajax' => false, + ], 'report_customsql_get_users' => [ 'classname' => 'report_customsql\external\get_users', 'methodname' => 'execute', @@ -45,6 +54,12 @@ ]; $services = [ + 'Custom SQL Query Results API' => [ + 'functions' => ['report_customsql_get_query_result'], + 'shortname' => 'report_customsql_query_results', + 'restrictedusers' => 1, + 'enabled' => 1, + ], 'Custom SQL Report API' => [ 'functions' => [ 'report_customsql_get_simple_value', diff --git a/tests/webservice_test.php b/tests/webservice_test.php index 0a7efb4..9378c78 100644 --- a/tests/webservice_test.php +++ b/tests/webservice_test.php @@ -37,6 +37,107 @@ * @covers \report_customsql_external */ final class webservice_test extends \advanced_testcase { + /** + * All rows, duplicate first-column values and SQL NULL survive the external schema. + * + * @runInSeparateProcess + */ + public function test_get_query_result(): void { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + $reportid = $this->create_a_database_row(); + $DB->update_record('report_customsql_queries', (object) [ + 'id' => $reportid, + 'querysql' => 'SELECT 1 AS repeated, :value AS content, NULL AS optionalvalue ' + . 'UNION ALL SELECT 1, :other, NULL', + 'queryparams' => serialize(['value' => 'First & second', 'other' => '0']), + 'querylimit' => 1, + ]); + + $result = \report_customsql_external::get_query_result('number_of_custom_sql_queries'); + $cleaned = \report_customsql_external::clean_returnvalue( + \report_customsql_external::get_query_result_returns(), + $result + ); + $this->assertSame([ + 'columns' => ['repeated', 'content', 'optionalvalue'], + 'rows' => [ + ['values' => ['1', 'First & second', null]], + ['values' => ['1', '0', null]], + ], + ], $cleaned); + } + + /** + * JSON parameters and the legacy table prefix work for an empty result. + * + * @runInSeparateProcess + */ + public function test_get_query_result_empty(): void { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + $reportid = $this->create_a_database_row(); + $DB->update_record('report_customsql_queries', (object) [ + 'id' => $reportid, + 'querysql' => 'SELECT id FROM prefix_report_customsql_queries WHERE id = :reportid', + 'queryparams' => json_encode(['reportid' => -1]), + ]); + $this->assertSame( + ['columns' => [], 'rows' => []], + \report_customsql_external::get_query_result('number_of_custom_sql_queries') + ); + } + + /** + * Unknown queries are rejected. + * + * @runInSeparateProcess + */ + public function test_get_query_result_missing(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + $this->expectException(\moodle_exception::class); + $this->expectExceptionMessage(get_string('invalidreportid', 'report_customsql')); + \report_customsql_external::get_query_result('missing'); + } + + /** + * The global report capability is required before looking up a query. + * + * @runInSeparateProcess + */ + public function test_get_query_result_requires_view(): void { + $this->resetAfterTest(); + $this->setUser($this->getDataGenerator()->create_user()); + $this->expectException(\required_capability_exception::class); + \report_customsql_external::get_query_result('missing'); + } + + /** + * A reader still needs the report-specific capability. + * + * @runInSeparateProcess + */ + public function test_get_query_result_requires_report_capability(): void { + global $DB; + + $this->resetAfterTest(); + $user = $this->getDataGenerator()->create_user(); + $roleid = $this->getDataGenerator()->create_role(); + $context = \context_system::instance(); + assign_capability('report/customsql:view', CAP_ALLOW, $roleid, $context->id); + role_assign($roleid, $user->id, $context->id); + $reportid = $this->create_a_database_row(); + $DB->set_field('report_customsql_queries', 'capability', 'moodle/site:config', ['id' => $reportid]); + $this->setUser($user); + $this->expectException(\required_capability_exception::class); + \report_customsql_external::get_query_result('number_of_custom_sql_queries'); + } + /** * Test getting a simple value via webservice. * diff --git a/version.php b/version.php index 2647290..cd37e11 100644 --- a/version.php +++ b/version.php @@ -24,7 +24,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2025102101; +$plugin->version = 2025102102; $plugin->requires = 2024042200; $plugin->component = 'report_customsql'; $plugin->maturity = MATURITY_STABLE; From 0b6b2ae295831729b3e11ff67ba219786b0cb01d Mon Sep 17 00:00:00 2001 From: Peter Mayer Date: Thu, 17 Sep 2026 07:33:48 +0200 Subject: [PATCH 2/4] MBS-11338: Move query discovery webservice to report_customsql --- classes/external.php | 68 +++++++++++++++++++++++ db/access.php | 7 +++ db/services.php | 11 +++- db/upgrade.php | 12 +++++ lang/en/report_customsql.php | 9 ++++ settings.php | 40 ++++++++++++++ tests/webservice_test.php | 102 +++++++++++++++++++++++++++++++++++ version.php | 2 +- 8 files changed, 249 insertions(+), 2 deletions(-) diff --git a/classes/external.php b/classes/external.php index 55ce2f1..55128f5 100644 --- a/classes/external.php +++ b/classes/external.php @@ -36,6 +36,74 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class report_customsql_external extends core_external\external_api { + /** + * Describe the optional category filter for query discovery. + * + * @return external_function_parameters + */ + public static function get_queries_parameters() { + return new external_function_parameters([ + 'categoryids' => new external_multiple_structure( + new external_value(PARAM_INT, 'Category ID'), + 'Category IDs to filter by', + VALUE_DEFAULT, + [] + ), + ]); + } + + /** + * List query names and Graphite paths from explicitly allowed categories. + * + * @param array $categoryids Optional category IDs to filter by. + * @return array Query names and category Graphite paths. + */ + public static function get_queries($categoryids = []) { + global $DB; + + $params = self::validate_parameters(self::get_queries_parameters(), ['categoryids' => $categoryids]); + $context = \context_system::instance(); + self::validate_context($context); + require_capability('report/customsql:usecustomsql', $context); + + $allowedcategories = get_config('report_customsql', 'customsqlcategories'); + if (empty($allowedcategories)) { + return []; + } + $categoryids = array_unique(array_map('intval', explode(',', $allowedcategories))); + if (!empty($params['categoryids'])) { + $categoryids = array_intersect($categoryids, $params['categoryids']); + } + + $queries = []; + foreach ($categoryids as $categoryid) { + if (!$DB->record_exists('report_customsql_categories', ['id' => $categoryid])) { + continue; + } + $graphitepath = get_config('report_customsql', 'customsqlgraphitepath_' . $categoryid) ?: ''; + $categoryqueries = $DB->get_records('report_customsql_queries', ['categoryid' => $categoryid], 'id', 'id, displayname'); + foreach ($categoryqueries as $query) { + $queries[] = [ + 'displayname' => $query->displayname, + 'graphitepath' => $graphitepath, + ]; + } + } + return $queries; + } + + /** + * Describe the query discovery response. + * + * @return external_multiple_structure + */ + public static function get_queries_returns() { + return new external_multiple_structure(new external_single_structure([ + 'displayname' => new external_value(PARAM_TEXT, 'Display name'), + 'graphitepath' => new external_value(PARAM_PATH, 'Graphite path for this category'), + ])); + } + /** * Describe the parameters for a complete query result. * diff --git a/db/access.php b/db/access.php index f82b84a..b857cc6 100644 --- a/db/access.php +++ b/db/access.php @@ -26,6 +26,13 @@ $capabilities = [ + 'report/customsql:usecustomsql' => [ + 'captype' => 'read', + 'contextlevel' => CONTEXT_SYSTEM, + 'archetypes' => [], + 'clonepermissionsfrom' => 'local/mbs:usecustomsql', + ], + // People who can view the reports at all. 'report/customsql:view' => [ 'riskbitmask' => RISK_PERSONAL, diff --git a/db/services.php b/db/services.php index 8e198fc..f3e0f4d 100644 --- a/db/services.php +++ b/db/services.php @@ -25,6 +25,15 @@ defined('MOODLE_INTERNAL') || die(); $functions = [ + 'report_customsql_get_queries' => [ + 'classname' => 'report_customsql_external', + 'methodname' => 'get_queries', + 'classpath' => 'report/customsql/classes/external.php', + 'description' => 'Get CustomSQL queries from allowed categories.', + 'type' => 'read', + 'capabilities' => 'report/customsql:usecustomsql', + 'ajax' => false, + ], 'report_customsql_get_query_result' => [ 'classname' => 'report_customsql_external', 'methodname' => 'get_query_result', @@ -55,7 +64,7 @@ $services = [ 'Custom SQL Query Results API' => [ - 'functions' => ['report_customsql_get_query_result'], + 'functions' => ['report_customsql_get_query_result', 'report_customsql_get_queries'], 'shortname' => 'report_customsql_query_results', 'restrictedusers' => 1, 'enabled' => 1, diff --git a/db/upgrade.php b/db/upgrade.php index 744ab2e..5dc51fe 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -340,5 +340,17 @@ function xmldb_report_customsql_upgrade($oldversion) { upgrade_plugin_savepoint(true, 2025101800, 'report', 'customsql'); } + if ($oldversion < 2025102103) { + $legacyconfig = get_config('local_mbs'); + foreach ((array) $legacyconfig as $name => $value) { + if ($name === 'customsqlcategories' || preg_match('/^customsqlgraphitepath_[0-9]+$/', $name)) { + if (get_config('report_customsql', $name) === false) { + set_config($name, $value, 'report_customsql'); + } + } + } + upgrade_plugin_savepoint(true, 2025102103, 'report', 'customsql'); + } + return true; } diff --git a/lang/en/report_customsql.php b/lang/en/report_customsql.php index f89fbd9..d857d23 100644 --- a/lang/en/report_customsql.php +++ b/lang/en/report_customsql.php @@ -75,8 +75,17 @@ $string['customsql:definequeries'] = 'Define custom queries'; $string['customsql:executebackground'] = 'Execute queries in background'; $string['customsql:managecategories'] = 'Define custom categories'; +$string['customsql:usecustomsql'] = 'Discover queries through the Custom SQL webservice'; $string['customsql:view'] = 'View custom queries report'; $string['customsql:viewallexecutions'] = 'View all query executions'; +$string['customsqlcategories'] = 'Available Custom SQL categories'; +$string['customsqlcategoriesdesc'] = 'Select the categories whose query names can be retrieved through the webservice.'; +$string['customsqlgraphitepath'] = 'Graphite path for {$a}'; +$string['customsqlgraphitepathdesc'] = 'The Graphite path returned for queries in category "{$a}".'; +$string['customsqlgraphitepaths'] = 'Graphite paths for Custom SQL categories'; +$string['customsqlgraphitepathsdesc'] = 'Save the category selection first, then configure the Graphite path for each selected category.'; +$string['customsqlwebservice'] = 'Custom SQL query discovery webservice'; +$string['customsqlwebservicedesc'] = 'Settings for the webservice that lists query names and their category Graphite paths.'; $string['defaultcategory'] = 'Miscellaneous'; $string['deleteareyousure'] = 'Are you sure you want to delete this query?'; $string['deletecategoryareyousure'] = '

Are you sure you want to delete this category?

It cannot contain any queries.

'; diff --git a/settings.php b/settings.php index bed7e8e..bca42a1 100644 --- a/settings.php +++ b/settings.php @@ -25,6 +25,46 @@ defined('MOODLE_INTERNAL') || die(); if ($ADMIN->fulltree) { + require_once($CFG->dirroot . '/report/customsql/locallib.php'); + + $settings->add(new admin_setting_heading( + 'report_customsql/customsqlwebservice', + get_string('customsqlwebservice', 'report_customsql'), + get_string('customsqlwebservicedesc', 'report_customsql') + )); + $customsqlcategories = []; + if (!during_initial_install() && get_config('report_customsql', 'version')) { + $customsqlcategories = report_customsql_category_options(); + } + $settings->add(new admin_setting_configmulticheckbox( + 'report_customsql/customsqlcategories', + get_string('customsqlcategories', 'report_customsql'), + get_string('customsqlcategoriesdesc', 'report_customsql'), + [], + $customsqlcategories + )); + $settings->add(new admin_setting_heading( + 'report_customsql/customsqlgraphitepaths', + get_string('customsqlgraphitepaths', 'report_customsql'), + get_string('customsqlgraphitepathsdesc', 'report_customsql') + )); + $selectedcategories = get_config('report_customsql', 'customsqlcategories'); + if (!empty($selectedcategories)) { + foreach (explode(',', $selectedcategories) as $categoryid) { + $categoryid = (int) $categoryid; + if (isset($customsqlcategories[$categoryid])) { + $categoryname = $customsqlcategories[$categoryid]; + $settings->add(new admin_setting_configtext( + 'report_customsql/customsqlgraphitepath_' . $categoryid, + get_string('customsqlgraphitepath', 'report_customsql', $categoryname), + get_string('customsqlgraphitepathdesc', 'report_customsql', $categoryname), + 'mebis.lern.count.', + PARAM_PATH + )); + } + } + } + // Start of week, used for the day to run weekly reports. $days = ['sunday', 'monday', 'tuesday', 'wednesday', 'thursday', 'friday', 'saturday']; $days = array_map(function ($day) { diff --git a/tests/webservice_test.php b/tests/webservice_test.php index 9378c78..0721697 100644 --- a/tests/webservice_test.php +++ b/tests/webservice_test.php @@ -37,6 +37,108 @@ * @covers \report_customsql_external */ final class webservice_test extends \advanced_testcase { + /** + * Upgrade copies legacy settings without overwriting target settings. + */ + public function test_upgrade_query_discovery_settings(): void { + global $CFG; + + require_once($CFG->libdir . '/upgradelib.php'); + require_once($CFG->dirroot . '/report/customsql/db/upgrade.php'); + $this->resetAfterTest(); + set_config('version', 2025102102, 'report_customsql'); + set_config('customsqlcategories', '1,2', 'local_mbs'); + set_config('customsqlgraphitepath_1', 'legacy.first.', 'local_mbs'); + set_config('customsqlgraphitepath_2', 'legacy.second.', 'local_mbs'); + set_config('customsqlgraphitepath_1', 'existing.', 'report_customsql'); + set_config('unrelated', 'unchanged', 'local_mbs'); + + $this->assertTrue(xmldb_report_customsql_upgrade(2025102102)); + $this->assertSame('1,2', get_config('report_customsql', 'customsqlcategories')); + $this->assertSame('existing.', get_config('report_customsql', 'customsqlgraphitepath_1')); + $this->assertSame('legacy.second.', get_config('report_customsql', 'customsqlgraphitepath_2')); + $this->assertFalse(get_config('report_customsql', 'unrelated')); + $this->assertSame('1,2', get_config('local_mbs', 'customsqlcategories')); + } + + /** + * Discovery only returns allowed categories and preserves Graphite paths. + * + * @runInSeparateProcess + */ + public function test_get_queries(): void { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + $allowedid = $DB->insert_record('report_customsql_categories', (object) ['name' => 'Allowed']); + $blockedid = $DB->insert_record('report_customsql_categories', (object) ['name' => 'Blocked']); + $reportid = $this->create_a_database_row('Allowed query'); + $DB->set_field('report_customsql_queries', 'categoryid', $allowedid, ['id' => $reportid]); + $reportid = $this->create_a_database_row('Blocked query'); + $DB->set_field('report_customsql_queries', 'categoryid', $blockedid, ['id' => $reportid]); + set_config('customsqlcategories', $allowedid . ',999999', 'report_customsql'); + set_config('customsqlgraphitepath_' . $allowedid, 'mebis.lern.count.', 'report_customsql'); + set_config('customsqlcategories', $blockedid, 'local_mbs'); + + $expected = [['displayname' => 'Allowed query', 'graphitepath' => 'mebis.lern.count.']]; + $result = \report_customsql_external::get_queries(); + $this->assertSame($expected, \report_customsql_external::clean_returnvalue( + \report_customsql_external::get_queries_returns(), + $result + )); + $this->assertSame($expected, \report_customsql_external::get_queries([$allowedid, $blockedid])); + $this->assertSame([], \report_customsql_external::get_queries([$blockedid])); + $this->assertSame([], \report_customsql_external::get_queries([999999])); + unset_config('customsqlgraphitepath_' . $allowedid, 'report_customsql'); + $this->assertSame( + [['displayname' => 'Allowed query', 'graphitepath' => '']], + \report_customsql_external::get_queries([$allowedid]) + ); + set_config('customsqlcategories', '', 'report_customsql'); + $this->assertSame([], \report_customsql_external::get_queries()); + } + + /** + * Query discovery requires its dedicated capability. + * + * @runInSeparateProcess + */ + public function test_get_queries_requires_capability(): void { + $this->resetAfterTest(); + $this->setUser($this->getDataGenerator()->create_user()); + $this->expectException(\required_capability_exception::class); + \report_customsql_external::get_queries(); + } + + /** + * A user with the discovery capability can list queries without admin access. + * + * @runInSeparateProcess + */ + public function test_get_queries_with_capability(): void { + $this->resetAfterTest(); + $user = $this->getDataGenerator()->create_user(); + $roleid = $this->getDataGenerator()->create_role(); + $context = \context_system::instance(); + assign_capability('report/customsql:usecustomsql', CAP_ALLOW, $roleid, $context->id); + role_assign($roleid, $user->id, $context->id); + $this->setUser($user); + $this->assertSame([], \report_customsql_external::get_queries()); + } + + /** + * Invalid category filters are rejected. + * + * @runInSeparateProcess + */ + public function test_get_queries_invalid_category(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + $this->expectException(\invalid_parameter_exception::class); + \report_customsql_external::get_queries(['invalid']); + } + /** * All rows, duplicate first-column values and SQL NULL survive the external schema. * diff --git a/version.php b/version.php index cd37e11..e40ff3c 100644 --- a/version.php +++ b/version.php @@ -24,7 +24,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2025102102; +$plugin->version = 2025102103; $plugin->requires = 2024042200; $plugin->component = 'report_customsql'; $plugin->maturity = MATURITY_STABLE; From 6f07f5524e4d013b2ff2092adbe2e9be97a1a4cd Mon Sep 17 00:00:00 2001 From: Tobias Garske Date: Thu, 8 Oct 2026 13:54:21 +0000 Subject: [PATCH 3/4] MBS-11338: remove local_mbs references --- db/access.php | 1 - db/upgrade.php | 12 ------------ tests/webservice_test.php | 25 ------------------------- 3 files changed, 38 deletions(-) diff --git a/db/access.php b/db/access.php index b857cc6..44d5c30 100644 --- a/db/access.php +++ b/db/access.php @@ -30,7 +30,6 @@ 'captype' => 'read', 'contextlevel' => CONTEXT_SYSTEM, 'archetypes' => [], - 'clonepermissionsfrom' => 'local/mbs:usecustomsql', ], // People who can view the reports at all. diff --git a/db/upgrade.php b/db/upgrade.php index 5dc51fe..744ab2e 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -340,17 +340,5 @@ function xmldb_report_customsql_upgrade($oldversion) { upgrade_plugin_savepoint(true, 2025101800, 'report', 'customsql'); } - if ($oldversion < 2025102103) { - $legacyconfig = get_config('local_mbs'); - foreach ((array) $legacyconfig as $name => $value) { - if ($name === 'customsqlcategories' || preg_match('/^customsqlgraphitepath_[0-9]+$/', $name)) { - if (get_config('report_customsql', $name) === false) { - set_config($name, $value, 'report_customsql'); - } - } - } - upgrade_plugin_savepoint(true, 2025102103, 'report', 'customsql'); - } - return true; } diff --git a/tests/webservice_test.php b/tests/webservice_test.php index 0721697..954e985 100644 --- a/tests/webservice_test.php +++ b/tests/webservice_test.php @@ -37,30 +37,6 @@ * @covers \report_customsql_external */ final class webservice_test extends \advanced_testcase { - /** - * Upgrade copies legacy settings without overwriting target settings. - */ - public function test_upgrade_query_discovery_settings(): void { - global $CFG; - - require_once($CFG->libdir . '/upgradelib.php'); - require_once($CFG->dirroot . '/report/customsql/db/upgrade.php'); - $this->resetAfterTest(); - set_config('version', 2025102102, 'report_customsql'); - set_config('customsqlcategories', '1,2', 'local_mbs'); - set_config('customsqlgraphitepath_1', 'legacy.first.', 'local_mbs'); - set_config('customsqlgraphitepath_2', 'legacy.second.', 'local_mbs'); - set_config('customsqlgraphitepath_1', 'existing.', 'report_customsql'); - set_config('unrelated', 'unchanged', 'local_mbs'); - - $this->assertTrue(xmldb_report_customsql_upgrade(2025102102)); - $this->assertSame('1,2', get_config('report_customsql', 'customsqlcategories')); - $this->assertSame('existing.', get_config('report_customsql', 'customsqlgraphitepath_1')); - $this->assertSame('legacy.second.', get_config('report_customsql', 'customsqlgraphitepath_2')); - $this->assertFalse(get_config('report_customsql', 'unrelated')); - $this->assertSame('1,2', get_config('local_mbs', 'customsqlcategories')); - } - /** * Discovery only returns allowed categories and preserves Graphite paths. * @@ -79,7 +55,6 @@ public function test_get_queries(): void { $DB->set_field('report_customsql_queries', 'categoryid', $blockedid, ['id' => $reportid]); set_config('customsqlcategories', $allowedid . ',999999', 'report_customsql'); set_config('customsqlgraphitepath_' . $allowedid, 'mebis.lern.count.', 'report_customsql'); - set_config('customsqlcategories', $blockedid, 'local_mbs'); $expected = [['displayname' => 'Allowed query', 'graphitepath' => 'mebis.lern.count.']]; $result = \report_customsql_external::get_queries(); From 894f43afde1c3513b6cb19a3e3e0cf98795e05d6 Mon Sep 17 00:00:00 2001 From: Tobias Garske Date: Thu, 8 Oct 2026 14:03:56 +0000 Subject: [PATCH 4/4] MBS-11338: add category check on query execution --- classes/external.php | 24 +++++++++++++++++------- db/services.php | 2 +- tests/webservice_test.php | 18 ++++++++++++++++++ 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/classes/external.php b/classes/external.php index 55128f5..a92c2bb 100644 --- a/classes/external.php +++ b/classes/external.php @@ -66,11 +66,7 @@ public static function get_queries($categoryids = []) { self::validate_context($context); require_capability('report/customsql:usecustomsql', $context); - $allowedcategories = get_config('report_customsql', 'customsqlcategories'); - if (empty($allowedcategories)) { - return []; - } - $categoryids = array_unique(array_map('intval', explode(',', $allowedcategories))); + $categoryids = self::get_allowed_category_ids(); if (!empty($params['categoryids'])) { $categoryids = array_intersect($categoryids, $params['categoryids']); } @@ -92,6 +88,19 @@ public static function get_queries($categoryids = []) { return $queries; } + /** + * Get the category IDs enabled for the monitoring webservices. + * + * @return int[] + */ + private static function get_allowed_category_ids(): array { + $allowedcategories = get_config('report_customsql', 'customsqlcategories'); + if (empty($allowedcategories)) { + return []; + } + return array_unique(array_map('intval', explode(',', $allowedcategories))); + } + /** * Describe the query discovery response. * @@ -116,7 +125,7 @@ public static function get_query_result_parameters() { } /** - * Execute a predefined query without the report display row limit. + * Execute a predefined query from an allowed category without the report display row limit. * * @param string $queryname Name of the saved query. * @return array Column names and rows containing nullable string values. @@ -130,7 +139,8 @@ public static function get_query_result($queryname) { require_capability('report/customsql:view', $context); $report = $DB->get_record('report_customsql_queries', ['displayname' => $params['queryname']]); - if (!$report) { + // Same error for disallowed categories so query names outside them are not disclosed. + if (!$report || !in_array((int) $report->categoryid, self::get_allowed_category_ids(), true)) { throw new \moodle_exception('invalidreportid', 'report_customsql'); } if (!empty($report->capability)) { diff --git a/db/services.php b/db/services.php index f3e0f4d..68aba35 100644 --- a/db/services.php +++ b/db/services.php @@ -38,7 +38,7 @@ 'classname' => 'report_customsql_external', 'methodname' => 'get_query_result', 'classpath' => 'report/customsql/classes/external.php', - 'description' => 'Execute a predefined query and return all result rows.', + 'description' => 'Execute a predefined query from an allowed category and return all result rows.', 'capabilities' => 'report/customsql:view', 'type' => 'read', 'ajax' => false, diff --git a/tests/webservice_test.php b/tests/webservice_test.php index 954e985..9e7669a 100644 --- a/tests/webservice_test.php +++ b/tests/webservice_test.php @@ -132,6 +132,7 @@ public function test_get_query_result(): void { 'queryparams' => serialize(['value' => 'First & second', 'other' => '0']), 'querylimit' => 1, ]); + set_config('customsqlcategories', '1', 'report_customsql'); $result = \report_customsql_external::get_query_result('number_of_custom_sql_queries'); $cleaned = \report_customsql_external::clean_returnvalue( @@ -163,6 +164,7 @@ public function test_get_query_result_empty(): void { 'querysql' => 'SELECT id FROM prefix_report_customsql_queries WHERE id = :reportid', 'queryparams' => json_encode(['reportid' => -1]), ]); + set_config('customsqlcategories', '1', 'report_customsql'); $this->assertSame( ['columns' => [], 'rows' => []], \report_customsql_external::get_query_result('number_of_custom_sql_queries') @@ -182,6 +184,21 @@ public function test_get_query_result_missing(): void { \report_customsql_external::get_query_result('missing'); } + /** + * Queries outside the allowed categories are rejected like unknown queries. + * + * @runInSeparateProcess + */ + public function test_get_query_result_category_not_allowed(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + $this->create_a_database_row(); + set_config('customsqlcategories', '2', 'report_customsql'); + $this->expectException(\moodle_exception::class); + $this->expectExceptionMessage(get_string('invalidreportid', 'report_customsql')); + \report_customsql_external::get_query_result('number_of_custom_sql_queries'); + } + /** * The global report capability is required before looking up a query. * @@ -210,6 +227,7 @@ public function test_get_query_result_requires_report_capability(): void { role_assign($roleid, $user->id, $context->id); $reportid = $this->create_a_database_row(); $DB->set_field('report_customsql_queries', 'capability', 'moodle/site:config', ['id' => $reportid]); + set_config('customsqlcategories', '1', 'report_customsql'); $this->setUser($user); $this->expectException(\required_capability_exception::class); \report_customsql_external::get_query_result('number_of_custom_sql_queries');