From 77a916450713e1dee495668e21cf4ffff162dfa6 Mon Sep 17 00:00:00 2001
From: tedbow <tedbow@240860.no-reply.drupal.org>
Date: Fri, 10 Dec 2021 20:46:46 +0000
Subject: [PATCH] Issue #3252126 by tedbow, phenaproxima: Do not apply updates
during cron if there are DB updates in the staging area
---
automatic_updates.module | 4 +-
automatic_updates.services.yml | 20 ++
src/CronUpdater.php | 38 +---
.../StagedDatabaseUpdateValidator.php | 160 ++++++++++++++
tests/src/Kernel/CronUpdaterTest.php | 28 +--
.../StagedDatabaseUpdateValidatorTest.php | 205 ++++++++++++++++++
6 files changed, 409 insertions(+), 46 deletions(-)
create mode 100644 src/Validator/StagedDatabaseUpdateValidator.php
create mode 100644 tests/src/Kernel/ReadinessValidation/StagedDatabaseUpdateValidatorTest.php
diff --git a/automatic_updates.module b/automatic_updates.module
index ba246224e3..7ed25a0bd8 100644
--- a/automatic_updates.module
+++ b/automatic_updates.module
@@ -73,9 +73,7 @@ function automatic_updates_module_implements_alter(&$implementations, $hook) {
* Implements hook_cron().
*/
function automatic_updates_cron() {
- /** @var \Drupal\automatic_updates\CronUpdater $cron_updater */
- $cron_updater = \Drupal::classResolver(CronUpdater::class);
- $cron_updater->handleCron();
+ \Drupal::service('automatic_updates.cron_updater')->handleCron();
/** @var \Drupal\automatic_updates\Validation\ReadinessValidationManager $checker_manager */
$checker_manager = \Drupal::service('automatic_updates.readiness_validation_manager');
diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml
index a4cc7ecd94..d5b1c8c5a4 100644
--- a/automatic_updates.services.yml
+++ b/automatic_updates.services.yml
@@ -18,6 +18,18 @@ services:
- '@file_system'
- '@event_dispatcher'
- '@tempstore.shared'
+ automatic_updates.cron_updater:
+ class: Drupal\automatic_updates\CronUpdater
+ arguments:
+ - '@config.factory'
+ - '@logger.factory'
+ - '@package_manager.path_locator'
+ - '@package_manager.beginner'
+ - '@package_manager.stager'
+ - '@package_manager.committer'
+ - '@file_system'
+ - '@event_dispatcher'
+ - '@tempstore.shared'
automatic_updates.excluded_paths_subscriber:
class: Drupal\automatic_updates\Event\ExcludedPathsSubscriber
arguments:
@@ -86,3 +98,11 @@ services:
- '@string_translation'
tags:
- { name: event_subscriber }
+ automatic_updates.validator.staged_database_updates:
+ class: Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator
+ arguments:
+ - '@package_manager.path_locator'
+ - '@extension.list.module'
+ - '@string_translation'
+ tags:
+ - { name: event_subscriber }
diff --git a/src/CronUpdater.php b/src/CronUpdater.php
index f989c58556..d6ca031824 100644
--- a/src/CronUpdater.php
+++ b/src/CronUpdater.php
@@ -3,9 +3,7 @@
namespace Drupal\automatic_updates;
use Drupal\Core\Config\ConfigFactoryInterface;
-use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Logger\LoggerChannelFactoryInterface;
-use Symfony\Component\DependencyInjection\ContainerInterface;
/**
* Defines a service that updates via cron.
@@ -14,7 +12,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
* This class implements logic specific to Automatic Updates' cron hook
* implementation. It should not be called directly.
*/
-class CronUpdater implements ContainerInjectionInterface {
+class CronUpdater extends Updater {
/**
* All automatic updates are disabled.
@@ -37,13 +35,6 @@ class CronUpdater implements ContainerInjectionInterface {
*/
public const ALL = 'patch';
- /**
- * The updater service.
- *
- * @var \Drupal\automatic_updates\Updater
- */
- protected $updater;
-
/**
* The config factory service.
*
@@ -61,30 +52,19 @@ class CronUpdater implements ContainerInjectionInterface {
/**
* Constructs a CronUpdater object.
*
- * @param \Drupal\automatic_updates\Updater $updater
- * The updater service.
* @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
* The config factory service.
* @param \Drupal\Core\Logger\LoggerChannelFactoryInterface $logger_factory
* The logger channel factory.
+ * @param mixed ...$arguments
+ * Additional arguments to pass to the parent constructor.
*/
- public function __construct(Updater $updater, ConfigFactoryInterface $config_factory, LoggerChannelFactoryInterface $logger_factory) {
- $this->updater = $updater;
+ public function __construct(ConfigFactoryInterface $config_factory, LoggerChannelFactoryInterface $logger_factory, ...$arguments) {
+ parent::__construct(...$arguments);
$this->configFactory = $config_factory;
$this->logger = $logger_factory->get('automatic_updates');
}
- /**
- * {@inheritdoc}
- */
- public static function create(ContainerInterface $container) {
- return new static(
- $container->get('automatic_updates.updater'),
- $container->get('config.factory'),
- $container->get('logger.factory')
- );
- }
-
/**
* Handles updates during cron.
*/
@@ -127,12 +107,12 @@ class CronUpdater implements ContainerInjectionInterface {
// cron runs.
$recommended_version = $recommended_release->getVersion();
try {
- $this->updater->begin([
+ $this->begin([
'drupal' => $recommended_version,
]);
- $this->updater->stage();
- $this->updater->apply();
- $this->updater->destroy();
+ $this->stage();
+ $this->apply();
+ $this->destroy();
}
catch (\Throwable $e) {
$this->logger->error($e->getMessage());
diff --git a/src/Validator/StagedDatabaseUpdateValidator.php b/src/Validator/StagedDatabaseUpdateValidator.php
new file mode 100644
index 0000000000..0fea7e697c
--- /dev/null
+++ b/src/Validator/StagedDatabaseUpdateValidator.php
@@ -0,0 +1,160 @@
+<?php
+
+namespace Drupal\automatic_updates\Validator;
+
+use Drupal\automatic_updates\CronUpdater;
+use Drupal\Core\Extension\Extension;
+use Drupal\Core\Extension\ModuleExtensionList;
+use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\Core\StringTranslation\TranslationInterface;
+use Drupal\package_manager\Event\PreApplyEvent;
+use Drupal\package_manager\PathLocator;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+/**
+ * Validates that there are no database updates in a staged update.
+ */
+class StagedDatabaseUpdateValidator implements EventSubscriberInterface {
+
+ use StringTranslationTrait;
+
+ /**
+ * The path locator service.
+ *
+ * @var \Drupal\package_manager\PathLocator
+ */
+ protected $pathLocator;
+
+ /**
+ * The module list service.
+ *
+ * @var \Drupal\Core\Extension\ModuleExtensionList
+ */
+ protected $moduleList;
+
+ /**
+ * Constructs a StagedDatabaseUpdateValidator object.
+ *
+ * @param \Drupal\package_manager\PathLocator $path_locator
+ * The path locator service.
+ * @param \Drupal\Core\Extension\ModuleExtensionList $module_list
+ * The module list service.
+ * @param \Drupal\Core\StringTranslation\TranslationInterface $translation
+ * The string translation service.
+ */
+ public function __construct(PathLocator $path_locator, ModuleExtensionList $module_list, TranslationInterface $translation) {
+ $this->pathLocator = $path_locator;
+ $this->moduleList = $module_list;
+ $this->setStringTranslation($translation);
+ }
+
+ /**
+ * Checks that the staged update does not have changes to its install files.
+ *
+ * @param \Drupal\package_manager\Event\PreApplyEvent $event
+ * The event object.
+ */
+ public function checkUpdateHooks(PreApplyEvent $event): void {
+ $stage = $event->getStage();
+ if (!$stage instanceof CronUpdater) {
+ return;
+ }
+
+ $active_dir = $this->pathLocator->getActiveDirectory();
+ $stage_dir = $stage->getStageDirectory();
+
+ $web_root = $this->pathLocator->getWebRoot();
+ if ($web_root) {
+ $active_dir .= DIRECTORY_SEPARATOR . $web_root;
+ $stage_dir .= DIRECTORY_SEPARATOR . $web_root;
+ }
+
+ $invalid_modules = [];
+ // Although \Drupal\automatic_updates\Validator\StagedProjectsValidator
+ // should prevent non-core modules from being added, updated, or removed in
+ // the staging area, we check all installed modules so as not to rely on the
+ // presence of StagedProjectsValidator.
+ foreach ($this->moduleList->getAllInstalledInfo() as $name => $info) {
+ if ($this->hasStagedUpdates($active_dir, $stage_dir, $this->moduleList->get($name))) {
+ $invalid_modules[] = $info['name'];
+ }
+ }
+
+ if ($invalid_modules) {
+ $event->addError($invalid_modules, $this->t('The update cannot proceed because possible database updates have been detected in the following modules.'));
+ }
+ }
+
+ /**
+ * Determines if a staged extension has changed update functions.
+ *
+ * @param string $active_dir
+ * The path of the running Drupal code base.
+ * @param string $stage_dir
+ * The path of the staging area.
+ * @param \Drupal\Core\Extension\Extension $extension
+ * The extension to check.
+ *
+ * @return bool
+ * TRUE if the staged copy of the extension has changed update functions
+ * compared to the active copy, FALSE otherwise.
+ *
+ * @todo Use a more sophisticated method to detect changes in the staged
+ * extension. Right now, we just compare hashes of the .install and
+ * .post_update.php files in both copies of the given extension, but this
+ * will cause false positives for changes to comments, whitespace, or
+ * runtime code like requirements checks. It would be preferable to use a
+ * static analyzer to detect new or changed functions that are actually
+ * executed during an update. No matter what, this method must NEVER cause
+ * false negatives, since that could result in code which is incompatible
+ * with the current database schema being copied to the active directory.
+ *
+ * @see https://www.drupal.org/project/automatic_updates/issues/3253828
+ */
+ protected function hasStagedUpdates(string $active_dir, string $stage_dir, Extension $extension): bool {
+ $active_hashes = $this->getHashes($active_dir, $extension);
+ $staged_hashes = $this->getHashes($stage_dir, $extension);
+
+ return $active_hashes !== $staged_hashes;
+ }
+
+ /**
+ * Returns hashes of the .install and .post-update.php files for a module.
+ *
+ * @param string $root_dir
+ * The root directory of the Drupal code base.
+ * @param \Drupal\Core\Extension\Extension $module
+ * The module to check.
+ *
+ * @return string[]
+ * The hashes of the module's .install and .post_update.php files, in that
+ * order, if they exist. The array will be keyed by file extension.
+ */
+ protected function getHashes(string $root_dir, Extension $module): array {
+ $path = implode(DIRECTORY_SEPARATOR, [
+ $root_dir,
+ $module->getPath(),
+ $module->getName(),
+ ]);
+ $hashes = [];
+
+ foreach (['.install', '.post_update.php'] as $suffix) {
+ $file = $path . $suffix;
+
+ if (file_exists($file)) {
+ $hashes[$suffix] = hash_file('sha256', $file);
+ }
+ }
+ return $hashes;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public static function getSubscribedEvents() {
+ return [
+ PreApplyEvent::class => 'checkUpdateHooks',
+ ];
+ }
+
+}
diff --git a/tests/src/Kernel/CronUpdaterTest.php b/tests/src/Kernel/CronUpdaterTest.php
index 78dc4d7d13..413e42fbfe 100644
--- a/tests/src/Kernel/CronUpdaterTest.php
+++ b/tests/src/Kernel/CronUpdaterTest.php
@@ -4,8 +4,8 @@ namespace Drupal\Tests\automatic_updates\Kernel;
use Drupal\automatic_updates\CronUpdater;
use Drupal\Core\Form\FormState;
-use Drupal\package_manager\ComposerUtility;
use Drupal\update\UpdateSettingsForm;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
/**
* @covers \Drupal\automatic_updates\CronUpdater
@@ -21,6 +21,7 @@ class CronUpdaterTest extends AutomaticUpdatesKernelTestBase {
protected static $modules = [
'automatic_updates',
'package_manager',
+ 'package_manager_bypass',
];
/**
@@ -97,21 +98,20 @@ class CronUpdaterTest extends AutomaticUpdatesKernelTestBase {
$form_state->setValue('automatic_updates_cron', $setting);
$form_builder->submitForm(UpdateSettingsForm::class, $form_state);
- // Mock the updater so we can assert that its methods are called or bypassed
- // depending on configuration.
- $will_update = (int) $will_update;
- $updater = $this->prophesize('\Drupal\automatic_updates\Updater');
-
- $composer = ComposerUtility::createForDirectory(__DIR__ . '/../../fixtures/fake-site');
- $updater->getActiveComposer()->willReturn($composer);
-
- $updater->begin(['drupal' => '9.8.1'])->shouldBeCalledTimes($will_update);
- $updater->stage()->shouldBeCalledTimes($will_update);
- $updater->apply()->shouldBeCalledTimes($will_update);
- $updater->destroy()->shouldBeCalledTimes($will_update);
- $this->container->set('automatic_updates.updater', $updater->reveal());
+ // Since we're just trying to ensure that all of Package Manager's services
+ // are called as expected, disable validation by replacing the event
+ // dispatcher with a dummy version.
+ $event_dispatcher = $this->prophesize(EventDispatcherInterface::class);
+ $this->container->set('event_dispatcher', $event_dispatcher->reveal());
+ // Run cron and ensure that Package Manager's services were called or
+ // bypassed depending on configuration.
$this->container->get('cron')->run();
+
+ $will_update = (int) $will_update;
+ $this->assertCount($will_update, $this->container->get('package_manager.beginner')->getInvocationArguments());
+ $this->assertCount($will_update, $this->container->get('package_manager.stager')->getInvocationArguments());
+ $this->assertCount($will_update, $this->container->get('package_manager.committer')->getInvocationArguments());
}
}
diff --git a/tests/src/Kernel/ReadinessValidation/StagedDatabaseUpdateValidatorTest.php b/tests/src/Kernel/ReadinessValidation/StagedDatabaseUpdateValidatorTest.php
new file mode 100644
index 0000000000..5b0ce6962f
--- /dev/null
+++ b/tests/src/Kernel/ReadinessValidation/StagedDatabaseUpdateValidatorTest.php
@@ -0,0 +1,205 @@
+<?php
+
+namespace Drupal\Tests\automatic_updates\Kernel\ReadinessValidation;
+
+use Drupal\automatic_updates\CronUpdater;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\package_manager\Exception\StageValidationException;
+use Drupal\package_manager\ValidationResult;
+use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
+
+/**
+ * @covers \Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator
+ *
+ * @group automatic_updates
+ */
+class StagedDatabaseUpdateValidatorTest extends AutomaticUpdatesKernelTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ protected static $modules = [
+ 'automatic_updates',
+ 'package_manager',
+ 'package_manager_bypass',
+ ];
+
+ /**
+ * The suffixes of the files that can contain database updates.
+ *
+ * @var string[]
+ */
+ private const SUFFIXES = ['install', 'post_update.php'];
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp(): void {
+ parent::setUp();
+
+ TestCronUpdater::$stagingRoot = $this->vfsRoot->url();
+
+ /** @var \Drupal\Tests\automatic_updates\Kernel\ReadinessValidation\TestCronUpdater $updater */
+ $updater = $this->container->get('automatic_updates.cron_updater');
+ $updater->begin(['drupal' => '9.8.1']);
+ $updater->stage();
+
+ $stage_dir = $updater->getStageDirectory();
+ mkdir($stage_dir);
+
+ // To satisfy StagedProjectsValidator, copy the active Composer files into
+ // the staging area.
+ $active_dir = $this->getDrupalRoot();
+ @copy("$active_dir/composer.json", "$stage_dir/composer.json");
+ @copy("$active_dir/composer.lock", "$stage_dir/composer.lock");
+
+ // Copy the .install and .post_update.php files from every installed module
+ // into the staging directory.
+ $module_list = $this->container->get('module_handler')->getModuleList();
+ foreach ($module_list as $name => $module) {
+ $path = $module->getPath();
+ mkdir("$stage_dir/$path", 0777, TRUE);
+
+ foreach (static::SUFFIXES as $suffix) {
+ // If the source file doesn't exist, silence the warning it raises.
+ @copy("$active_dir/$path/$name.$suffix", "$stage_dir/$path/$name.$suffix");
+ }
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function register(ContainerBuilder $container) {
+ parent::register($container);
+
+ $container->getDefinition('automatic_updates.cron_updater')
+ ->setClass(TestCronUpdater::class);
+ }
+
+ /**
+ * Tests that no errors are raised if staged files have no DB updates.
+ */
+ public function testNoUpdates(): void {
+ // Since we're testing with a modified version of Views, it should not be
+ // installed.
+ $this->assertFalse($this->container->get('module_handler')->moduleExists('views'));
+
+ // Create bogus staged versions of Views' .install and .post_update.php
+ // files. Since it's not installed, the changes should not raise any
+ // validation errors.
+ $updater = $this->container->get('automatic_updates.cron_updater');
+ $module_dir = $updater->getStageDirectory() . '/core/modules/views';
+ mkdir($module_dir, 0777, TRUE);
+ foreach (static::SUFFIXES as $suffix) {
+ file_put_contents("$module_dir/views.$suffix", $this->randomString());
+ }
+
+ $updater->apply();
+ }
+
+ /**
+ * Data provider for ::testFileChanged().
+ *
+ * @return array[]
+ * Sets of arguments to pass to the test method.
+ */
+ public function providerFileChanged(): array {
+ $scenarios = [];
+ foreach (static::SUFFIXES as $suffix) {
+ $scenarios[] = [$suffix, FALSE];
+ $scenarios[] = [$suffix, TRUE];
+ }
+ return $scenarios;
+ }
+
+ /**
+ * Tests that an error is raised if install or post-update files are changed.
+ *
+ * @param string $suffix
+ * The suffix of the file to change. Can be either 'install' or
+ * 'post_update.php'.
+ * @param bool $delete
+ * Whether or not to delete the file.
+ *
+ * @dataProvider providerFileChanged
+ */
+ public function testFileChanged(string $suffix, bool $delete): void {
+ /** @var \Drupal\Tests\automatic_updates\Kernel\ReadinessValidation\TestCronUpdater $updater */
+ $updater = $this->container->get('automatic_updates.cron_updater');
+
+ $file = $updater->getStageDirectory() . "/core/modules/system/system.$suffix";
+ if ($delete) {
+ unlink($file);
+ }
+ else {
+ file_put_contents($file, $this->randomString());
+ }
+
+ $expected_results = [
+ ValidationResult::createError(['System'], t('The update cannot proceed because possible database updates have been detected in the following modules.')),
+ ];
+
+ try {
+ $updater->apply();
+ $this->fail('Expected a validation error.');
+ }
+ catch (StageValidationException $e) {
+ $this->assertValidationResultsEqual($expected_results, $e->getResults());
+ }
+ }
+
+ /**
+ * Tests that an error is raised if install or post-update files are added.
+ */
+ public function testUpdatesAddedInStage(): void {
+ $module = $this->container->get('module_handler')
+ ->getModule('package_manager_bypass');
+
+ /** @var \Drupal\Tests\automatic_updates\Kernel\ReadinessValidation\TestCronUpdater $updater */
+ $updater = $this->container->get('automatic_updates.cron_updater');
+
+ foreach (static::SUFFIXES as $suffix) {
+ $file = sprintf('%s/%s/%s.%s', $updater->getStageDirectory(), $module->getPath(), $module->getName(), $suffix);
+ // The file we're creating shouldn't already exist in the staging area
+ // unless it's a file we actually ship, which is a scenario covered by
+ // ::testFileChanged().
+ $this->assertFileNotExists($file);
+ file_put_contents($file, $this->randomString());
+ }
+
+ $expected_results = [
+ ValidationResult::createError(['Package Manager Bypass'], t('The update cannot proceed because possible database updates have been detected in the following modules.')),
+ ];
+
+ try {
+ $updater->apply();
+ $this->fail('Expected a validation error.');
+ }
+ catch (StageValidationException $e) {
+ $this->assertValidationResultsEqual($expected_results, $e->getResults());
+ }
+ }
+
+}
+
+/**
+ * A test-only version of the cron updater.
+ */
+class TestCronUpdater extends CronUpdater {
+
+ /**
+ * The directory where staging areas will be created.
+ *
+ * @var string
+ */
+ public static $stagingRoot;
+
+ /**
+ * {@inheritdoc}
+ */
+ protected static function getStagingRoot(): string {
+ return static::$stagingRoot ?: parent::getStagingRoot();
+ }
+
+}
--
GitLab