From 46192ca77e0fb11f52121837de642a8b41b0f322 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Fri, 24 Nov 2023 10:44:07 +0000 Subject: [PATCH] MDL-80254 reportbuilder: better handling of deleted users in schedules. We should verify that schedule and audience users are active accounts, and that we don't try to send report schedules either from/to them if they aren't. --- .../classes/local/helpers/audience.php | 7 ++- .../classes/local/helpers/schedule.php | 20 +++++---- .../systemreports/report_access_list.php | 29 ++++-------- reportbuilder/classes/task/send_schedule.php | 24 +++++++++- .../tests/local/helpers/audience_test.php | 9 ++++ .../tests/local/helpers/schedule_test.php | 9 ++++ .../tests/task/send_schedule_test.php | 44 +++++++++++++++++++ 7 files changed, 108 insertions(+), 34 deletions(-) diff --git a/reportbuilder/classes/local/helpers/audience.php b/reportbuilder/classes/local/helpers/audience.php index 65290e0951f..0adf2fb43c3 100644 --- a/reportbuilder/classes/local/helpers/audience.php +++ b/reportbuilder/classes/local/helpers/audience.php @@ -84,14 +84,17 @@ class audience { // Generate audience SQL based on those for the current report. [$wheres, $params] = self::user_audience_sql($audiences); - $allwheres = implode(' OR ', $wheres); + if (count($wheres) === 0) { + continue; + } $paramuserid = database::generate_param_name(); $params[$paramuserid] = $userid; $sql = "SELECT DISTINCT(u.id) FROM {user} u - WHERE ({$allwheres}) + WHERE (" . implode(' OR ', $wheres) . ") + AND u.deleted = 0 AND u.id = :{$paramuserid}"; // If we have a matching record, user can view the report. diff --git a/reportbuilder/classes/local/helpers/schedule.php b/reportbuilder/classes/local/helpers/schedule.php index 3d6942619ad..0940afee914 100644 --- a/reportbuilder/classes/local/helpers/schedule.php +++ b/reportbuilder/classes/local/helpers/schedule.php @@ -111,19 +111,21 @@ class schedule { $audienceids = (array) json_decode($schedule->get('audiences')); // Retrieve all selected audience records for the schedule. - [$audienceselect, $audienceparams] = $DB->get_in_or_equal($audienceids, SQL_PARAMS_NAMED, 'aid', true, true); + [$audienceselect, $audienceparams] = $DB->get_in_or_equal($audienceids, SQL_PARAMS_NAMED, 'aid', true, null); $audiences = audience_model::get_records_select("id {$audienceselect}", $audienceparams); - if (count($audiences) === 0) { - return []; - } // Now convert audiences to SQL for user retrieval. [$wheres, $params] = audience::user_audience_sql($audiences); + if (count($wheres) === 0) { + return []; + } + [$userorder] = users_order_by_sql('u'); $sql = 'SELECT u.* FROM {user} u - WHERE ' . implode(' OR ', $wheres) . ' + WHERE (' . implode(' OR ', $wheres) . ') + AND u.deleted = 0 ORDER BY ' . $userorder; return $DB->get_records_sql($sql, $params); @@ -151,7 +153,8 @@ class schedule { * @return stored_file */ public static function get_schedule_report_file(model $schedule): stored_file { - global $CFG; + global $CFG, $USER; + require_once("{$CFG->libdir}/filelib.php"); $table = custom_report_table_view::create($schedule->get('reportid')); @@ -166,10 +169,9 @@ class schedule { $exportclass = new table_dataformat_export_format($table, $table->download); ob_end_clean(); - // Create our schedule report stored file. - $context = context_user::instance($schedule->get('usercreated')); + // Create our schedule report stored file temporarily in user draft. $filerecord = [ - 'contextid' => $context->id, + 'contextid' => context_user::instance($USER->id)->id, 'component' => 'user', 'filearea' => 'draft', 'itemid' => file_get_unused_draft_itemid(), diff --git a/reportbuilder/classes/local/systemreports/report_access_list.php b/reportbuilder/classes/local/systemreports/report_access_list.php index a8c2cace3b0..9a180de3f16 100644 --- a/reportbuilder/classes/local/systemreports/report_access_list.php +++ b/reportbuilder/classes/local/systemreports/report_access_list.php @@ -41,22 +41,22 @@ class report_access_list extends system_report { protected function initialise(): void { $userentity = new user(); $userentityalias = $userentity->get_table_alias('user'); + $this->set_main_table('user', $userentityalias); $this->add_entity($userentity); - $reportid = $this->get_parameter('id', 0, PARAM_INT); - // Find users allowed to view the report thru the report audiences. - [$wheres, $params] = self::get_users_by_audience_sql($reportid, $userentityalias); + $audiences = audience::get_records(['reportid' => $this->get_parameter('id', 0, PARAM_INT)]); + [$wheres, $params] = audience_helper::user_audience_sql($audiences, $userentityalias); - if (!empty($wheres)) { - // Wrap each OR condition into brackets. - $allwheres = '(' . implode(') OR (', $wheres) . ')'; + if (count($wheres) > 0) { + $select = '(' . implode(' OR ', $wheres) . ')'; } else { - $allwheres = "1=0"; + $select = "1=0"; } - $this->add_base_condition_sql("($allwheres)", $params); + $this->add_base_condition_sql($select, $params); + $this->add_base_condition_simple("{$userentityalias}.deleted", 0); $this->add_columns(); $this->add_filters(); @@ -105,17 +105,4 @@ class report_access_list extends system_report { $this->add_filter($userentity->get_identity_filter($identityfield)); } } - - /** - * Find users who can access this report based on the audience and add them to the report. - * - * @param int $reportid - * @param string $usertablealias - * @return array - */ - protected static function get_users_by_audience_sql(int $reportid, string $usertablealias): array { - $audiences = audience::get_records(['reportid' => $reportid]); - - return audience_helper::user_audience_sql($audiences, $usertablealias); - } } diff --git a/reportbuilder/classes/task/send_schedule.php b/reportbuilder/classes/task/send_schedule.php index 86430795f92..79eda5a9527 100644 --- a/reportbuilder/classes/task/send_schedule.php +++ b/reportbuilder/classes/task/send_schedule.php @@ -22,6 +22,7 @@ use core_user; use core\task\adhoc_task; use core_reportbuilder\local\helpers\schedule as helper; use core_reportbuilder\local\models\schedule; +use moodle_exception; /** * Ad-hoc task for sending a single report schedule @@ -70,8 +71,17 @@ class send_schedule extends adhoc_task { $scheduleattachment = null; $originaluser = $USER; + // Get the schedule creator, ensure it's an active account. + try { + $schedulecreator = core_user::get_user($schedule->get('usercreated'), '*', MUST_EXIST); + core_user::require_active_user($schedulecreator); + } catch (moodle_exception $exception) { + $this->log('Invalid schedule creator: ' . $exception->getMessage(), 0); + return; + } + // Switch to schedule creator, and retrieve list of recipient users. - \core\cron::setup_user(core_user::get_user($schedule->get('usercreated'))); + \core\cron::setup_user($schedulecreator); $users = helper::get_schedule_report_users($schedule); if (count($users) > 0) { @@ -83,7 +93,17 @@ class send_schedule extends adhoc_task { if ($scheduleuserviewas === schedule::REPORT_VIEWAS_CREATOR) { $scheduleattachment = helper::get_schedule_report_file($schedule); } else if ($scheduleuserviewas !== schedule::REPORT_VIEWAS_RECIPIENT) { - \core\cron::setup_user(core_user::get_user($scheduleuserviewas)); + + // Get the user to view the schedule report as, ensure it's an active account. + try { + $scheduleviewas = core_user::get_user($scheduleuserviewas, '*', MUST_EXIST); + core_user::require_active_user($scheduleviewas); + } catch (moodle_exception $exception) { + $this->log('Invalid schedule view as user: ' . $exception->getMessage(), 0); + return; + } + + \core\cron::setup_user($scheduleviewas); $scheduleattachment = helper::get_schedule_report_file($schedule); } diff --git a/reportbuilder/tests/local/helpers/audience_test.php b/reportbuilder/tests/local/helpers/audience_test.php index db4966d3206..d0fa0069e7a 100644 --- a/reportbuilder/tests/local/helpers/audience_test.php +++ b/reportbuilder/tests/local/helpers/audience_test.php @@ -155,6 +155,15 @@ class audience_test extends advanced_testcase { // User2 can access report1 and report2. $reports = audience::get_allowed_reports((int) $user2->id); $this->assertEqualsCanonicalizing([$report1->get('id'), $report2->get('id')], $reports); + + // Purge cache, to ensure allowed reports are re-calculated. + audience::purge_caches(); + + // Now delete one of our users, ensure they no longer have any allowed reports. + delete_user($user2); + + $reports = audience::get_allowed_reports((int) $user2->id); + $this->assertEmpty($reports); } /** diff --git a/reportbuilder/tests/local/helpers/schedule_test.php b/reportbuilder/tests/local/helpers/schedule_test.php index c1d9451e6ef..e25dffaf5ac 100644 --- a/reportbuilder/tests/local/helpers/schedule_test.php +++ b/reportbuilder/tests/local/helpers/schedule_test.php @@ -225,6 +225,15 @@ class schedule_test extends advanced_testcase { 'Henrietta', 'Zoe', ], array_column($users, 'firstname')); + + // Now delete one of our users, ensure they are no longer returned. + delete_user($manualuserone); + + $users = schedule::get_schedule_report_users($schedule); + $this->assertEquals([ + 'Henrietta', + 'Zoe', + ], array_column($users, 'firstname')); } /** diff --git a/reportbuilder/tests/task/send_schedule_test.php b/reportbuilder/tests/task/send_schedule_test.php index 7f53bd7ea00..57958a1ce09 100644 --- a/reportbuilder/tests/task/send_schedule_test.php +++ b/reportbuilder/tests/task/send_schedule_test.php @@ -146,6 +146,32 @@ class send_schedule_test extends advanced_testcase { $this->assertStringEndsWith("Username\n{$usertwosees}\n", $messagetwoattachment); } + /** + * Test executing task where the schedule "View as user" is an inactive account + */ + public function test_execute_report_viewas_user_invalid(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + + $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $audience = $generator->create_audience(['reportid' => $report->get('id'), 'configdata' => []]); + + $schedule = $generator->create_schedule([ + 'reportid' => $report->get('id'), + 'name' => 'My schedule', + 'userviewas' => 42, + 'audiences' => json_encode([$audience->get_persistent()->get('id')]), + ]); + + $this->expectOutputRegex("/^Sending schedule: My schedule\nInvalid schedule view as user: Invalid user/"); + $sendschedule = new send_schedule(); + $sendschedule->set_custom_data(['reportid' => $report->get('id'), 'scheduleid' => $schedule->get('id')]); + $sendschedule->execute(); + } + /** * Test executing task for a schedule that is configured to not send empty reports */ @@ -203,6 +229,24 @@ class send_schedule_test extends advanced_testcase { $sendschedule->execute(); } + /** + * Test executing task where the schedule creator is an inactive account + */ + public function test_execute_schedule_creator_invalid(): void { + $this->resetAfterTest(); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + + $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $schedule = $generator->create_schedule(['reportid' => $report->get('id'), 'name' => 'My schedule', 'usercreated' => 42]); + + $this->expectOutputRegex("/^Sending schedule: My schedule\nInvalid schedule creator: Invalid user/"); + $sendschedule = new send_schedule(); + $sendschedule->set_custom_data(['reportid' => $report->get('id'), 'scheduleid' => $schedule->get('id')]); + $sendschedule->execute(); + } + /** * Test executing task given invalid schedule data */