From 346cb39cff2e6ac2da427299671e7bfcf1931379 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Tue, 4 Apr 2023 09:25:04 +0800 Subject: [PATCH 1/3] MDL-77837 cron: Ensure user is set when running tasks We should be proactive in ensuring that the environment is clean when running a task. We already ensure that we have a clean renderer and other parts of the output chain, but we were not setting a clean user. This change adds a call to setup the cron user before each task is actually executed. --- lib/cronlib.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/cronlib.php b/lib/cronlib.php index 2e98a654050..3cf3ccc65da 100644 --- a/lib/cronlib.php +++ b/lib/cronlib.php @@ -248,6 +248,10 @@ function cron_run_inner_scheduled_task(\core\task\task_base $task) { $predbqueries = null; $predbqueries = $DB->perf_get_queries(); $pretime = microtime(1); + + // Ensure that we have a clean session with the correct cron user. + cron_setup_user(); + try { get_mailer('buffer'); cron_prepare_core_renderer(); @@ -346,6 +350,10 @@ function cron_run_inner_adhoc_task(\core\task\adhoc_task $task) { } cron_setup_user($user); + } else { + // No user specified, ensure that we have a clean session with the correct cron user. + cron_setup_user(); + } try { From 44d734147aadb67b598c84858c157921e12d5306 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Tue, 4 Apr 2023 09:26:34 +0800 Subject: [PATCH 2/3] MDL-77837 phpunit: Ensure that the cron user setter is used When running an adhoc task in a unit test we should use the cron variant of the set user method to mimic the behaviour of a real cron run. --- lib/phpunit/classes/advanced_testcase.php | 2 +- lib/phpunit/tests/advanced_test.php | 54 +++++++++++++++++++ .../tests/fixtures/adhoc_test_task.php | 35 ++++++++++++ 3 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 lib/phpunit/tests/fixtures/adhoc_test_task.php diff --git a/lib/phpunit/classes/advanced_testcase.php b/lib/phpunit/classes/advanced_testcase.php index 15de8395ab9..c6b71329335 100644 --- a/lib/phpunit/classes/advanced_testcase.php +++ b/lib/phpunit/classes/advanced_testcase.php @@ -733,7 +733,7 @@ abstract class advanced_testcase extends base_testcase { } cron_prepare_core_renderer(); - $this->setUser($user); + cron_setup_user($user); $task->execute(); \core\task\manager::adhoc_task_complete($task); diff --git a/lib/phpunit/tests/advanced_test.php b/lib/phpunit/tests/advanced_test.php index 8ee2388c63f..4b67b169bac 100644 --- a/lib/phpunit/tests/advanced_test.php +++ b/lib/phpunit/tests/advanced_test.php @@ -23,8 +23,13 @@ namespace core; * @category test * @copyright 2012 Petr Skoda {@link http://skodak.org} * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \advanced_testcase */ class advanced_test extends \advanced_testcase { + public static function setUpBeforeClass(): void { + global $CFG; + require_once(__DIR__ . '/fixtures/adhoc_test_task.php'); + } public function test_debugging() { global $CFG; @@ -697,4 +702,53 @@ class advanced_test extends \advanced_testcase { self::resetAllData(false); self::assertFalse(\core_useragent::get_user_agent_string(), 'It should not be set again, data was reset.'); } + + /** + * @covers ::runAdhocTasks + */ + public function test_runadhoctasks_no_tasks_queued(): void { + $this->runAdhocTasks(); + $this->expectOutputRegex('/^$/'); + } + + /** + * @covers ::runAdhocTasks + */ + public function test_runadhoctasks_tasks_queued(): void { + $this->resetAfterTest(true); + $admin = get_admin(); + \core\task\manager::queue_adhoc_task(new \core_phpunit\adhoc_test_task()); + $this->runAdhocTasks(); + $this->expectOutputRegex("/Task was run as {$admin->id}/"); + } + + /** + * @covers ::runAdhocTasks + */ + public function test_runadhoctasks_with_existing_user_change(): void { + $this->resetAfterTest(true); + $admin = get_admin(); + + $this->setGuestUser(); + \core\task\manager::queue_adhoc_task(new \core_phpunit\adhoc_test_task()); + $this->runAdhocTasks(); + $this->expectOutputRegex("/Task was run as {$admin->id}/"); + } + + /** + * @covers ::runAdhocTasks + */ + public function test_runadhoctasks_with_existing_user_change_and_specified(): void { + global $USER; + + $this->resetAfterTest(true); + $user = $this->getDataGenerator()->create_user(); + + $this->setGuestUser(); + $task = new \core_phpunit\adhoc_test_task(); + $task->set_userid($user->id); + \core\task\manager::queue_adhoc_task($task); + $this->runAdhocTasks(); + $this->expectOutputRegex("/Task was run as {$user->id}/"); + } } diff --git a/lib/phpunit/tests/fixtures/adhoc_test_task.php b/lib/phpunit/tests/fixtures/adhoc_test_task.php new file mode 100644 index 00000000000..090650fdc0c --- /dev/null +++ b/lib/phpunit/tests/fixtures/adhoc_test_task.php @@ -0,0 +1,35 @@ +. + +namespace core_phpunit; + +/** + * Fixtures for task tests. + * + * @package core + * @category phpunit + * @copyright 2023 Andrew Lyons + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class adhoc_test_task extends \core\task\adhoc_task { + /** + * Execute. + */ + public function execute() { + global $USER; + mtrace("Task was run as {$USER->id}"); + } +} From 202718f968247943fcd38d4f3862e9c428251b89 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 6 Apr 2023 16:27:38 +0800 Subject: [PATCH 3/3] MDL-77837 core: Improve usage docs for cron_setup_user --- lib/sessionlib.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/sessionlib.php b/lib/sessionlib.php index af4c0a6f6bf..9752431a9df 100644 --- a/lib/sessionlib.php +++ b/lib/sessionlib.php @@ -170,7 +170,11 @@ function get_moodle_cookie() { /** * Sets up current user and course environment (lang, etc.) in cron. - * Do not use outside of cron script! + * Note: This function is intended only for use in: + * - the cron runner scripts + * - individual tasks which extend the adhoc_task and scheduled_task classes + * - unit tests related to tasks + * - other parts of the cron/task system * * @param stdClass $user full user object, null means default cron user (admin), * value 'reset' means reset internal static caches.