From 4f35a3d73b4316f130de2e7e636a6e3b34f6a574 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 15 Jan 2024 15:21:29 +1000 Subject: [PATCH] MDL-80598 bigbluebutton: Gracefully handle invalid customdata --- .../classes/task/base_send_notification.php | 16 ++- .../classes/task/completion_update_state.php | 19 +++ .../task/base_send_notification_test.php | 47 ++++++- .../completion_update_state_task_test.php | 122 ++++++++++++++++++ 4 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 mod/bigbluebuttonbn/tests/task/completion_update_state_task_test.php diff --git a/mod/bigbluebuttonbn/classes/task/base_send_notification.php b/mod/bigbluebuttonbn/classes/task/base_send_notification.php index d3bad42d0f7..1a77648cf8c 100644 --- a/mod/bigbluebuttonbn/classes/task/base_send_notification.php +++ b/mod/bigbluebuttonbn/classes/task/base_send_notification.php @@ -69,9 +69,14 @@ abstract class base_send_notification extends adhoc_task { /** * Get the bigbluebutton instance that this notification is for. * - * @return instance + * @return instance|null null if the instance could not be loaded. */ - protected function get_instance(): instance { + protected function get_instance(): ?instance { + // This means the customdata is broken, and needs to be fixed. + if (empty($this->get_custom_data()->instanceid)) { + throw new \coding_exception("Task custom data was missing instanceid"); + } + if ($this->instance === null) { $this->instance = instance::get_from_instanceid($this->get_custom_data()->instanceid); } @@ -180,6 +185,13 @@ abstract class base_send_notification extends adhoc_task { */ protected function send_all_notifications(): void { $instance = $this->get_instance(); + + // Cannot do anything without a valid instance. + if (empty($instance)) { + mtrace("Instance was empty, skipping"); + return; + } + foreach ($this->get_recipients() as $recipient) { try { \core_user::require_active_user($recipient, true, true); diff --git a/mod/bigbluebuttonbn/classes/task/completion_update_state.php b/mod/bigbluebuttonbn/classes/task/completion_update_state.php index acef35cf633..8ca5b8ea919 100644 --- a/mod/bigbluebuttonbn/classes/task/completion_update_state.php +++ b/mod/bigbluebuttonbn/classes/task/completion_update_state.php @@ -17,6 +17,7 @@ namespace mod_bigbluebuttonbn\task; use core\task\adhoc_task; +use core_user; use mod_bigbluebuttonbn\local\proxy\bigbluebutton_proxy; /** @@ -33,6 +34,24 @@ class completion_update_state extends adhoc_task { public function execute() { // Get the custom data. $data = $this->get_custom_data(); + + // Ensure the customdata structure is corect. + if (empty($data->bigbluebuttonbn->id) || empty($data->userid)) { + throw new \coding_exception("Task customdata was missing bigbluebuttonbn->id or userid"); + } + + // If coursemodule does not exist, ignore (likely has been deleted). + if (get_coursemodule_from_instance('bigbluebuttonbn', $data->bigbluebuttonbn->id) === false) { + mtrace("Course module does not exist, ignoring."); + return; + } + + // If user does not exist, ignore (likely has been deleted). + if (core_user::get_user($data->userid) === false) { + mtrace("User does not exist, ignoring."); + return; + } + mtrace("Task completion_update_state running for user {$data->userid}"); // Process the completion. diff --git a/mod/bigbluebuttonbn/tests/task/base_send_notification_test.php b/mod/bigbluebuttonbn/tests/task/base_send_notification_test.php index 2515ea008c0..5ef3850081a 100644 --- a/mod/bigbluebuttonbn/tests/task/base_send_notification_test.php +++ b/mod/bigbluebuttonbn/tests/task/base_send_notification_test.php @@ -28,13 +28,14 @@ use advanced_testcase; * @coversDefaultClass \mod_bigbluebuttonbn\task\base_send_notification */ class base_send_notification_test extends advanced_testcase { + /** - * Check if set instance ID works correctly + * Returns mock base_send_notification class * + * @return base_send_notification */ - public function test_set_instance_id(): void { - $this->resetAfterTest(); - $stub = $this->getMockForAbstractClass( + private function get_mock(): base_send_notification { + return $this->getMockForAbstractClass( base_send_notification::class, [], '', @@ -43,7 +44,26 @@ class base_send_notification_test extends advanced_testcase { true, [] ); + } + /** + * Returns reflection method for base_send_notification->get_instance + * + * @return \ReflectionMethod + */ + private function get_instance_reflection(): \ReflectionMethod { + $rc = new \ReflectionClass(base_send_notification::class); + $rcm = $rc->getMethod('get_instance'); + $rcm->setAccessible(true); + return $rcm; + } + + /** + * Check if set instance ID works correctly + */ + public function test_set_instance_id(): void { + $this->resetAfterTest(); + $stub = $this->get_mock(); $generator = $this->getDataGenerator(); $course = $generator->create_course(); $instancedata = $generator->create_module('bigbluebuttonbn', [ @@ -52,10 +72,23 @@ class base_send_notification_test extends advanced_testcase { $stub->set_instance_id($instancedata->id); - $rc = new \ReflectionClass(base_send_notification::class); - $rcm = $rc->getMethod('get_instance'); + $rcm = $this->get_instance_reflection(); $instance = $rcm->invoke($stub); - + $this->assertNotNull($instance); $this->assertEquals($instancedata->id, $instance->get_instance_id()); } + + /** + * Check if instanceid missing is checked and handled. + */ + public function test_set_instanceid_missing(): void { + $this->resetAfterTest(); + $stub = $this->get_mock(); + $rcm = $this->get_instance_reflection(); + + // This should throw a coding exception since there is no instanceid set. + $this->expectException(\coding_exception::class); + $this->expectExceptionMessage("Task custom data was missing instanceid"); + $rcm->invoke($stub); + } } diff --git a/mod/bigbluebuttonbn/tests/task/completion_update_state_task_test.php b/mod/bigbluebuttonbn/tests/task/completion_update_state_task_test.php new file mode 100644 index 00000000000..bf1ef50578d --- /dev/null +++ b/mod/bigbluebuttonbn/tests/task/completion_update_state_task_test.php @@ -0,0 +1,122 @@ +. + +namespace mod_bigbluebuttonbn\task; + +use advanced_testcase; +use mod_bigbluebuttonbn\task\completion_update_state; + +/** + * Completion_update_state task tests. + * + * @package mod_bigbluebuttonbn + * @copyright 2024 Catalyst IT + * @author Matthew Hilton + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \mod_bigbluebuttonbn\task\completion_update_state + */ +final class completion_update_state_task_test extends advanced_testcase { + + /** + * Providers data to test_invalid_customdata test + * @return array + */ + public static function invalid_customdata_provider(): array { + return [ + 'empty' => [ + 'customdata' => [], + 'expectoutput' => "", + 'expectexceptionmessage' => "Task customdata was missing bigbluebuttonbn->id or userid", + ], + 'bbb id set but is invalid' => [ + 'customdata' => [ + 'bigbluebuttonbn' => -1, + ], + 'expectoutput' => "", + 'expectexceptionmessage' => "Task customdata was missing bigbluebuttonbn->id or userid", + ], + 'bbb id is valid, but there is no user' => [ + 'customdata' => [ + 'bigbluebuttonbn' => ':bbb', + ], + 'expectoutput' => "", + 'expectexceptionmessage' => "Task customdata was missing bigbluebuttonbn->id or userid", + ], + 'bbb id is valid, but the user is not given' => [ + 'customdata' => [ + 'bigbluebuttonbn' => ':bbb', + ], + 'expectoutput' => "", + 'expectexceptionmessage' => "Task customdata was missing bigbluebuttonbn->id or userid", + ], + 'bbb id is valid, but the user given is invalid' => [ + 'customdata' => [ + 'bigbluebuttonbn' => ':bbb', + 'userid' => -1, + ], + 'expectoutput' => "User does not exist, ignoring.\n", + 'expectexceptionmessage' => "", + ], + 'bbb and userid is valid' => [ + 'customdata' => [ + 'bigbluebuttonbn' => ':bbb', + 'userid' => ':userid', + ], + // Expects this output, since all the necessary data is there. + 'expectoutput' => "Task completion_update_state running for user :userid\nCompletion not enabled\n", + 'expectexceptionmessage' => "", + ], + ]; + } + /** + * Tests the task handles an invalid cmid gracefully. + * @param array $customdata customdata to set (with placeholders to replace with real data). + * @param string $expectoutput any output expected from the test, or empty to not expect output. + * @param string $expectexceptionmessage exception message expected from test, or empty to expect nothing. + * @dataProvider invalid_customdata_provider + */ + public function test_invalid_customdata(array $customdata, string $expectoutput, string $expectexceptionmessage): void { + $this->resetAfterTest(); + $customdata = (object) $customdata; + + // Replace any placeholders in the customdata. + if (!empty($customdata->bigbluebuttonbn) && $customdata->bigbluebuttonbn == ':bbb') { + $course = $this->getDataGenerator()->create_course(); + $module = $this->getDataGenerator()->create_module('bigbluebuttonbn', ['course' => $course->id]); + $customdata->bigbluebuttonbn = $module; + } + + $user = $this->getDataGenerator()->create_user(); + + // Replace userid placeholders. + if (!empty($customdata->userid) && $customdata->userid == ':userid') { + $customdata->userid = $user->id; + } + + $task = new completion_update_state(); + $task->set_custom_data($customdata); + + if (!empty($expectoutput)) { + $expectoutput = str_replace(':userid', $user->id, $expectoutput); + $this->expectOutputString($expectoutput); + } + + if (!empty($expectexceptionmessage)) { + $this->expectExceptionMessage($expectexceptionmessage); + } + $task->execute(); + } +}