From 72a51148e976f95ca6ea9feec6bd18e1a55addb2 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 a0d37334de2..21a71603b75 100644 --- a/lib/cronlib.php +++ b/lib/cronlib.php @@ -242,6 +242,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(); @@ -325,6 +329,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 22ac74f686cceab3dd1b92ec9e867e378493435d 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 41a7da01b1e..7420ce40ab6 100644 --- a/lib/phpunit/classes/advanced_testcase.php +++ b/lib/phpunit/classes/advanced_testcase.php @@ -702,7 +702,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 525325d2a38..769072a5516 100644 --- a/lib/phpunit/tests/advanced_test.php +++ b/lib/phpunit/tests/advanced_test.php @@ -33,8 +33,13 @@ defined('MOODLE_INTERNAL') || die(); * @category phpunit * @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 core_phpunit_advanced_testcase extends advanced_testcase { + public static function setUpBeforeClass(): void { + global $CFG; + require_once(__DIR__ . '/fixtures/adhoc_test_task.php'); + } public function test_debugging() { global $CFG; @@ -680,4 +685,53 @@ class core_phpunit_advanced_testcase 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 d1f085a771da29d223e5377f288f1f8774eb5797 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 6 Apr 2023 16:26: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 59e2e6aa51f..e465d7dff2c 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.