From 20429eb9408fc3696d8b2c775bc6b7020a56eaf8 Mon Sep 17 00:00:00 2001 From: Rich Lott Date: Sat, 14 Mar 2020 14:29:55 +0000 Subject: [PATCH] Scheduled jobs belonging to extensions cannot be disabled (#16430) Authored-by: Rich Lott / Artful Robot --- CRM/Core/ManagedEntities.php | 26 +++- CRM/Extension/Manager.php | 134 +++++++++++++++++- .../phpunit/CRM/Core/ManagedEntitiesTest.php | 94 ++++++++++++ 3 files changed, 245 insertions(+), 9 deletions(-) diff --git a/CRM/Core/ManagedEntities.php b/CRM/Core/ManagedEntities.php index cf47fa6918..434c7a3376 100644 --- a/CRM/Core/ManagedEntities.php +++ b/CRM/Core/ManagedEntities.php @@ -251,7 +251,7 @@ class CRM_Core_ManagedEntities { } /** - * Update an entity which (a) is believed to exist and which (b) ought to be active. + * Update an entity which is believed to exist. * * @param CRM_Core_DAO_Managed $dao * @param array $todo @@ -262,12 +262,26 @@ class CRM_Core_ManagedEntities { $doUpdate = ($policy == 'always'); if ($doUpdate) { - $defaults = [ - 'id' => $dao->entity_id, - // FIXME: test whether is_active is valid - 'is_active' => 1, - ]; + $defaults = ['id' => $dao->entity_id, 'is_active' => 1]; $params = array_merge($defaults, $todo['params']); + + $manager = CRM_Extension_System::singleton()->getManager(); + if ($dao->entity_type === 'Job' && !$manager->extensionIsBeingInstalledOrEnabled($dao->module)) { + // Special treatment for scheduled jobs: + // + // If we're being called as part of enabling/installing a module then + // we want the default behaviour of setting is_active = 1. + // + // However, if we're just being called by a normal cache flush then we + // should not re-enable a job that an administrator has decided to disable. + // + // Without this logic there was a problem: site admin might disable + // a job, but then when there was a flush op, the job was re-enabled + // which can cause significant embarrassment, depending on the job + // ("Don't worry, sending mailings is disabled right now..."). + unset($params['is_active']); + } + $result = civicrm_api($dao->entity_type, 'create', $params); if ($result['is_error']) { $this->onApiError($dao->entity_type, 'create', $params, $result); diff --git a/CRM/Extension/Manager.php b/CRM/Extension/Manager.php index 1040ea119e..ce4bca1550 100644 --- a/CRM/Extension/Manager.php +++ b/CRM/Extension/Manager.php @@ -13,6 +13,10 @@ * The extension manager handles installing, disabling enabling, and * uninstalling extensions. * + * You should obtain a singleton of this class via + * + * $manager = CRM_Extension_Manager::singleton()->getManager(); + * * @package CRM * @copyright CiviCRM LLC https://civicrm.org/licensing */ @@ -94,6 +98,34 @@ class CRM_Extension_Manager { */ public $statuses; + /** + * Live process(es) per extension. + * + * @var array + * + * Format is: { + * extensionKey => [ + * ['operation' => 'install|enable|uninstall|disable', 'phase' => 'queued|live|completed' + * ... + * ], + * ... + * } + * + * The inner array is a stack, so the most recent current operation is the + * last entry. As this manager handles multiple extensions at once, here's + * the flow for an install operation. + * + * $manager->install(['ext1', 'ext2']); + * + * 0. {} + * 1. { ext1: ['install'], ext2: ['install'] } + * 2. { ext1: ['install', 'installing'], ext2: ['install'] } + * 3. { ext1: ['install'], ext2: ['install', 'installing'] } + * 4. { ext1: ['install'], ext2: ['install'] } + * 5. {} + */ + protected $processes = []; + /** * Class constructor. * @@ -201,9 +233,10 @@ class CRM_Extension_Manager { * * @param string|array $keys * One or more extension keys. + * @param string $mode install|enable * @throws CRM_Extension_Exception */ - public function install($keys) { + public function install($keys, $mode = 'install') { $keys = (array) $keys; $origStatuses = $this->getStatuses(); @@ -221,6 +254,9 @@ class CRM_Extension_Manager { throw new CRM_Extension_Exception('Cannot install incompatible extension: ' . implode(', ', $incompatible)); } + // Keep state for these operations. + $this->addProcess($keys, $mode); + foreach ($keys as $key) { /** @var CRM_Extension_Info $info */ /** @var CRM_Extension_Manager_Base $typeManager */ @@ -228,11 +264,17 @@ class CRM_Extension_Manager { switch ($origStatuses[$key]) { case self::STATUS_INSTALLED: - // ok, nothing to do + // ok, nothing to do. As such the status of this process is no longer + // 'install' install was the intent, which might have resulted in + // changes but these changes will not be happening, so processes that + // are sensitive to installs (like the managed entities reconcile + // operation) should not assume that these changes have happened. + $this->popProcess([$key]); break; case self::STATUS_DISABLED: // re-enable it + $this->addProcess([$key], 'enabling'); $typeManager->onPreEnable($info); $this->_setExtensionActive($info, 1); $typeManager->onPostEnable($info); @@ -241,10 +283,13 @@ class CRM_Extension_Manager { // later extensions to access classes from earlier extensions. $this->statuses = NULL; $this->mapper->refresh(); + + $this->popProcess([$key]); break; case self::STATUS_UNINSTALLED: // install anew + $this->addProcess([$key], 'installing'); $typeManager->onPreInstall($info); $this->_createExtensionEntry($info); $typeManager->onPostInstall($info); @@ -253,6 +298,8 @@ class CRM_Extension_Manager { // later extensions to access classes from earlier extensions. $this->statuses = NULL; $this->mapper->refresh(); + + $this->popProcess([$key]); break; case self::STATUS_UNKNOWN: @@ -264,6 +311,7 @@ class CRM_Extension_Manager { $this->statuses = NULL; $this->mapper->refresh(); CRM_Core_Invoke::rebuildMenuAndCaches(TRUE); + $schema = new CRM_Logging_Schema(); $schema->fixSchemaDifferences(); @@ -282,7 +330,9 @@ class CRM_Extension_Manager { case self::STATUS_UNINSTALLED: // install anew + $this->addProcess([$key], 'installing'); $typeManager->onPostPostInstall($info); + $this->popProcess([$key]); break; case self::STATUS_UNKNOWN: @@ -291,6 +341,8 @@ class CRM_Extension_Manager { } } + // All processes for these keys + $this->popProcess($keys); } /** @@ -301,7 +353,7 @@ class CRM_Extension_Manager { * @throws CRM_Extension_Exception */ public function enable($keys) { - $this->install($keys); + $this->install($keys, 'enable'); } /** @@ -326,14 +378,18 @@ class CRM_Extension_Manager { throw new CRM_Extension_Exception_DependencyException("Cannot disable extension due to dependencies. Consider disabling all these: " . implode(',', $disableRequirements)); } + $this->addProcess($keys, 'disable'); + foreach ($keys as $key) { switch ($origStatuses[$key]) { case self::STATUS_INSTALLED: + $this->addProcess([$key], 'disabling'); // throws Exception list ($info, $typeManager) = $this->_getInfoTypeHandler($key); $typeManager->onPreDisable($info); $this->_setExtensionActive($info, 0); $typeManager->onPostDisable($info); + $this->popProcess([$key]); break; case self::STATUS_INSTALLED_MISSING: @@ -348,6 +404,8 @@ class CRM_Extension_Manager { case self::STATUS_DISABLED_MISSING: case self::STATUS_UNINSTALLED: // ok, nothing to do + // Remove the 'disable' process as we're not doing that. + $this->popProcess([$key]); break; case self::STATUS_UNKNOWN: @@ -359,6 +417,8 @@ class CRM_Extension_Manager { $this->statuses = NULL; $this->mapper->refresh(); CRM_Core_Invoke::rebuildMenuAndCaches(TRUE); + + $this->popProcess($keys); } /** @@ -375,6 +435,8 @@ class CRM_Extension_Manager { // TODO: to mitigate the risk of crashing during installation, scan // keys/statuses/types before doing anything + $this->addProcess($keys, 'uninstall'); + foreach ($keys as $key) { switch ($origStatuses[$key]) { case self::STATUS_INSTALLED: @@ -382,6 +444,7 @@ class CRM_Extension_Manager { throw new CRM_Extension_Exception("Cannot uninstall extension; disable it first: $key"); case self::STATUS_DISABLED: + $this->addProcess([$key], 'uninstalling'); // throws Exception list ($info, $typeManager) = $this->_getInfoTypeHandler($key); $typeManager->onPreUninstall($info); @@ -399,6 +462,8 @@ class CRM_Extension_Manager { case self::STATUS_UNINSTALLED: // ok, nothing to do + // remove the 'uninstall' process since we're not doing that. + $this->popProcess([$key]); break; case self::STATUS_UNKNOWN: @@ -410,6 +475,7 @@ class CRM_Extension_Manager { $this->statuses = NULL; $this->mapper->refresh(); CRM_Core_Invoke::rebuildMenuAndCaches(TRUE); + $this->popProcess($keys); } /** @@ -491,6 +557,34 @@ class CRM_Extension_Manager { $this->mapper->refresh(); } + /** + * Return current processes for given extension. + * + * @param String $key extension key + * + * @return array + */ + public function getActiveProcesses(string $key) :Array { + return $this->processes[$key] ?? []; + } + + /** + * Determine if the extension specified is currently involved in an install + * or enable process. Just sugar code to make things more readable. + * + * @param String $key extension key + * + * @return bool + */ + public function extensionIsBeingInstalledOrEnabled($key) :bool { + foreach ($this->getActiveProcesses($key) as $process) { + if (in_array($process, ['install', 'installing', 'enable', 'enabling'])) { + return TRUE; + } + } + return FALSE; + } + // ---------------------- /** @@ -711,6 +805,15 @@ class CRM_Extension_Manager { return $sorter->sort(); } + /** + * Provides way to set processes property for phpunit tests - not for general use. + * + * @param $processes + */ + public function setProcessesForTesting(array $processes) { + $this->processes = $processes; + } + /** * @param $infos * @param $filterStatuses @@ -726,4 +829,29 @@ class CRM_Extension_Manager { return $matches; } + /** + * Add a process to the stacks for the extensions. + * + * @param array $keys extensionKey + * @param string $process one of: install|uninstall|enable|disable|installing|uninstalling|enabling|disabling + */ + protected function addProcess(array $keys, string $process) { + foreach ($keys as $key) { + $this->processes[$key][] = $process; + } + } + + /** + * Pop the top op from the stacks for the extensions. + * + * @param array $keys extensionKey + */ + protected function popProcess(array $keys) { + foreach ($keys as $key) { + if (!empty($this->process[$key])) { + array_pop($this->process[$key]); + } + } + } + } diff --git a/tests/phpunit/CRM/Core/ManagedEntitiesTest.php b/tests/phpunit/CRM/Core/ManagedEntitiesTest.php index dc73e15f4a..a8e4c2801f 100644 --- a/tests/phpunit/CRM/Core/ManagedEntitiesTest.php +++ b/tests/phpunit/CRM/Core/ManagedEntitiesTest.php @@ -78,6 +78,20 @@ class CRM_Core_ManagedEntitiesTest extends CiviUnitTestCase { ], ]; + $this->fixtures['com.example.one-Job'] = [ + 'module' => 'com.example.one', + 'name' => 'Job', + 'entity' => 'Job', + 'params' => [ + 'version' => 3, + 'name' => 'test_job', + 'run_frequency' => 'Daily', + 'api_entity' => 'Job', + 'api_action' => 'Get', + 'parameters' => '', + ], + ]; + $this->apiKernel = \Civi::service('civi_api_kernel'); $this->adhocProvider = new \Civi\API\Provider\AdhocProvider(3, 'CustomSearch'); $this->apiKernel->registerApiProvider($this->adhocProvider); @@ -361,9 +375,13 @@ class CRM_Core_ManagedEntitiesTest extends CiviUnitTestCase { * module */ public function testDeactivateReactivateModule() { + $manager = CRM_Extension_System::singleton()->getManager(); + // create first managed entity ('foo') $decls = []; $decls[] = $this->fixtures['com.example.one-foo']; + // Mock the contextual process info that would be added by CRM_Extension_Manager::install + $manager->setProcessesForTesting(['com.example.one' => ['install']]); $me = new CRM_Core_ManagedEntities($this->modules, $decls); $me->reconcile(); $foo = $me->get('com.example.one', 'foo'); @@ -373,6 +391,8 @@ class CRM_Core_ManagedEntitiesTest extends CiviUnitTestCase { // now deactivate module, which has empty decls and which cascades to managed object $this->modules['one']->is_active = FALSE; + // Mock the contextual process info that would be added by CRM_Extension_Manager::disable + $manager->setProcessesForTesting(['com.example.one' => ['disable']]); $me = new CRM_Core_ManagedEntities($this->modules, []); $me->reconcile(); $foo = $me->get('com.example.one', 'foo'); @@ -382,12 +402,86 @@ class CRM_Core_ManagedEntitiesTest extends CiviUnitTestCase { // and reactivate module, which again provides decls and which cascades to managed object $this->modules['one']->is_active = TRUE; + // Mock the contextual process info that would be added by CRM_Extension_Manager::enable + $manager->setProcessesForTesting(['com.example.one' => ['enable']]); $me = new CRM_Core_ManagedEntities($this->modules, $decls); $me->reconcile(); $foo = $me->get('com.example.one', 'foo'); $this->assertEquals(1, $foo['is_active']); $this->assertEquals('CRM_Example_One_Foo', $foo['name']); $this->assertDBQuery(1, 'SELECT is_active FROM civicrm_option_value WHERE name = "CRM_Example_One_Foo"'); + + // Special case: Job entities. + // + // First we repeat the above steps, but adding the context that + // CRM_Extension_Manager adds when installing/enabling extensions. + // + // The behaviour should be as above. + $decls = [$this->fixtures['com.example.one-Job']]; + // Mock the contextual process info that would be added by CRM_Extension_Manager::install + $manager->setProcessesForTesting(['com.example.one' => ['install']]); + $me = new CRM_Core_ManagedEntities($this->modules, $decls); + $me->reconcile(); + $job = $me->get('com.example.one', 'Job'); + $this->assertEquals(1, $job['is_active']); + $this->assertEquals('test_job', $job['name']); + $this->assertDBQuery(1, 'SELECT is_active FROM civicrm_job WHERE name = "test_job"'); + // Reset context. + $manager->setProcessesForTesting([]); + + // now deactivate module, which has empty decls and which cascades to managed object + $this->modules['one']->is_active = FALSE; + // Mock the contextual process info that would be added by CRM_Extension_Manager::disable + $manager->setProcessesForTesting(['com.example.one' => ['disable']]); + $me = new CRM_Core_ManagedEntities($this->modules, []); + $me->reconcile(); + $job = $me->get('com.example.one', 'Job'); + $this->assertEquals(0, $job['is_active']); + $this->assertEquals('test_job', $job['name']); + $this->assertDBQuery(0, 'SELECT is_active FROM civicrm_job WHERE name = "test_job"'); + + // and reactivate module, which again provides decls and which cascades to managed object + $this->modules['one']->is_active = TRUE; + $me = new CRM_Core_ManagedEntities($this->modules, $decls); + // Mock the contextual process info that would be added by CRM_Extension_Manager::enable + $manager->setProcessesForTesting(['com.example.one' => ['enable']]); + $me->reconcile(); + $job = $me->get('com.example.one', 'Job'); + $this->assertEquals(1, $job['is_active']); + $this->assertEquals('test_job', $job['name']); + $this->assertDBQuery(1, 'SELECT is_active FROM civicrm_job WHERE name = "test_job"'); + + // Currently: module enabled, job enabled. + // Test that if we now manually disable the job, calling reconcile in a + // normal flush situation does NOT re-enable it. + // ... manually disable job. + $this->callAPISuccess('Job', 'create', ['id' => $job['id'], 'is_active' => 0]); + + // ... now call reconcile in the context of a normal flush operation. + // Mock the contextual process info - there would not be any + $manager->setProcessesForTesting([]); + $me = new CRM_Core_ManagedEntities($this->modules, $decls); + $me->reconcile(); + $job = $me->get('com.example.one', 'Job'); + $this->assertEquals(0, $job['is_active'], "Job that was manually set inactive should not have been set active again, but it was."); + $this->assertDBQuery(0, 'SELECT is_active FROM civicrm_job WHERE name = "test_job"'); + + // Now call reconcile again, but in the context of the job's extension being installed/enabled. This should re-enable the job. + foreach (['enable', 'install'] as $process) { + // Manually disable the job + $this->callAPISuccess('Job', 'create', ['id' => $job['id'], 'is_active' => 0]); + // Mock the contextual process info that would be added by CRM_Extension_Manager::enable + $manager->setProcessesForTesting(['com.example.one' => [$process]]); + $me = new CRM_Core_ManagedEntities($this->modules, $decls); + $me->reconcile(); + $job = $me->get('com.example.one', 'Job'); + $this->assertEquals(1, $job['is_active']); + $this->assertEquals('test_job', $job['name']); + $this->assertDBQuery(1, 'SELECT is_active FROM civicrm_job WHERE name = "test_job"'); + } + + // Reset context. + $manager->setProcessesForTesting([]); } /** -- 2.25.1