From 2d52693c15af986890d488d0c51bcd5b0f36e718 Mon Sep 17 00:00:00 2001
From: phenaproxima <phenaproxima@205645.no-reply.drupal.org>
Date: Fri, 18 Feb 2022 18:53:23 +0000
Subject: [PATCH] Issue #3248909 by phenaproxima, tedbow: Do not allow
destroying a stage during Apply
---
automatic_updates.services.yml | 2 +
package_manager/src/Stage.php | 59 +++++++++---
.../src/ApiController.php | 1 +
.../src/EventSubscriber/TestSubscriber.php | 16 ++++
.../Kernel/PackageManagerKernelTestBase.php | 7 +-
.../tests/src/Kernel/StageTest.php | 89 +++++++++++++++++++
src/Form/UpdateReady.php | 12 ++-
src/Form/UpdaterForm.php | 10 ++-
src/Updater.php | 4 +-
tests/src/Functional/UpdaterFormTest.php | 59 +++++++++++-
10 files changed, 235 insertions(+), 24 deletions(-)
diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml
index cbb5b41645..0e50d2b22d 100644
--- a/automatic_updates.services.yml
+++ b/automatic_updates.services.yml
@@ -21,6 +21,7 @@ services:
- '@file_system'
- '@event_dispatcher'
- '@tempstore.shared'
+ - '@datetime.time'
automatic_updates.cron_updater:
class: Drupal\automatic_updates\CronUpdater
arguments:
@@ -33,6 +34,7 @@ services:
- '@file_system'
- '@event_dispatcher'
- '@tempstore.shared'
+ - '@datetime.time'
automatic_updates.staged_projects_validator:
class: Drupal\automatic_updates\Validator\StagedProjectsValidator
arguments:
diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php
index 4c71210753..b93a3bcf7f 100644
--- a/package_manager/src/Stage.php
+++ b/package_manager/src/Stage.php
@@ -2,6 +2,7 @@
namespace Drupal\package_manager;
+use Drupal\Component\Datetime\TimeInterface;
use Drupal\Component\FileSystem\FileSystem;
use Drupal\Component\Utility\Crypt;
use Drupal\Core\File\Exception\FileException;
@@ -55,6 +56,16 @@ class Stage {
*/
protected const TEMPSTORE_METADATA_KEY = 'metadata';
+ /**
+ * The tempstore key under which to store the time that ::apply() was called.
+ *
+ * @var string
+ *
+ * @see ::apply()
+ * @see ::destroy()
+ */
+ private const TEMPSTORE_APPLY_TIME_KEY = 'apply_time';
+
/**
* The path locator service.
*
@@ -104,6 +115,13 @@ class Stage {
*/
protected $tempStore;
+ /**
+ * The time service.
+ *
+ * @var \Drupal\Component\Datetime\TimeInterface
+ */
+ protected $time;
+
/**
* The lock info for the stage.
*
@@ -130,14 +148,17 @@ class Stage {
* The event dispatcher service.
* @param \Drupal\Core\TempStore\SharedTempStoreFactory $shared_tempstore
* The shared tempstore factory.
+ * @param \Drupal\Component\Datetime\TimeInterface $time
+ * The time service.
*/
- public function __construct(PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore) {
+ public function __construct(PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore, TimeInterface $time) {
$this->pathLocator = $path_locator;
$this->beginner = $beginner;
$this->stager = $stager;
$this->committer = $committer;
$this->fileSystem = $file_system;
$this->eventDispatcher = $event_dispatcher;
+ $this->time = $time;
$this->tempStore = $shared_tempstore->get('package_manager_stage');
}
@@ -224,7 +245,9 @@ class Stage {
$stage_dir = $this->getStageDirectory();
$event = new PreCreateEvent($this);
- $this->dispatch($event);
+ // If an error occurs and we won't be able to create the stage, mark it as
+ // available.
+ $this->dispatch($event, [$this, 'markAsAvailable']);
$this->beginner->begin($active_dir, $stage_dir, $event->getExcludedPaths());
$this->dispatch(new PostCreateEvent($this));
@@ -277,9 +300,16 @@ class Stage {
$stage_dir = $this->getStageDirectory();
$event = new PreApplyEvent($this);
- $this->dispatch($event);
+ $this->tempStore->set(self::TEMPSTORE_APPLY_TIME_KEY, $this->time->getRequestTime());
+ // If an error occurs while dispatching the event, ensure that ::destroy()
+ // doesn't think we're in the middle of applying the staged changes to the
+ // active directory.
+ $this->dispatch($event, function (): void {
+ $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY);
+ });
$this->committer->commit($stage_dir, $active_dir, $event->getExcludedPaths());
+ $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY);
$this->dispatch(new PostApplyEvent($this));
}
@@ -290,14 +320,22 @@ class Stage {
* (optional) If TRUE, the staging area will be destroyed even if it is not
* owned by the current user or session. Defaults to FALSE.
*
- * @todo Do not allow the stage to be destroyed while it's being applied to
- * the active directory in https://www.drupal.org/i/3248909.
+ * @throws \Drupal\package_manager\Exception\StageException
+ * If the staged changes are being applied to the active directory.
*/
public function destroy(bool $force = FALSE): void {
if (!$force) {
$this->checkOwnership();
}
+ // If we started applying staged changes to the active directory less than
+ // an hour ago, prevent the stage from being destroyed.
+ // @see :apply()
+ $apply_time = $this->tempStore->get(self::TEMPSTORE_APPLY_TIME_KEY);
+ if (isset($apply_time) && $this->time->getRequestTime() - $apply_time < 3600) {
+ throw new StageException('Cannot destroy the staging area while it is being applied to the active directory.');
+ }
+
$this->dispatch(new PreDestroyEvent($this));
// Delete all directories in parent staging directory.
$parent_stage_dir = static::getStagingRoot();
@@ -332,13 +370,16 @@ class Stage {
*
* @param \Drupal\package_manager\Event\StageEvent $event
* The event object.
+ * @param callable $on_error
+ * (optional) A callback function to call if an error occurs, before any
+ * exceptions are thrown.
*
* @throws \Drupal\package_manager\Exception\StageValidationException
* If the event collects any validation errors.
* @throws \Drupal\package_manager\Exception\StageException
* If any other sort of error occurs.
*/
- protected function dispatch(StageEvent $event): void {
+ protected function dispatch(StageEvent $event, callable $on_error = NULL): void {
try {
$this->eventDispatcher->dispatch($event);
@@ -354,10 +395,8 @@ class Stage {
}
if (isset($error)) {
- // If we won't be able to create the staging area, mark it as available.
- // @see ::create()
- if ($event instanceof PreCreateEvent) {
- $this->markAsAvailable();
+ if ($on_error) {
+ $on_error();
}
throw $error;
}
diff --git a/package_manager/tests/modules/package_manager_test_api/src/ApiController.php b/package_manager/tests/modules/package_manager_test_api/src/ApiController.php
index a9f116ff2b..9c237bc000 100644
--- a/package_manager/tests/modules/package_manager_test_api/src/ApiController.php
+++ b/package_manager/tests/modules/package_manager_test_api/src/ApiController.php
@@ -42,6 +42,7 @@ class ApiController extends ControllerBase {
$container->get('file_system'),
$container->get('event_dispatcher'),
$container->get('tempstore.shared'),
+ $container->get('datetime.time')
);
return new static($stage);
}
diff --git a/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php b/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php
index e58267f1bb..0aeb5ede68 100644
--- a/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php
+++ b/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php
@@ -44,6 +44,19 @@ class TestSubscriber implements EventSubscriberInterface {
$this->state = $state;
}
+ /**
+ * Sets whether a specific event will call exit().
+ *
+ * This is useful for simulating an unrecoverable (fatal) error when handling
+ * the given event.
+ *
+ * @param string $event
+ * The event class.
+ */
+ public static function setExit(string $event): void {
+ \Drupal::state()->set(static::STATE_KEY . ".$event", 'exit');
+ }
+
/**
* Sets validation results for a specific event.
*
@@ -102,6 +115,9 @@ class TestSubscriber implements EventSubscriberInterface {
if ($results instanceof \Throwable) {
throw $results;
}
+ elseif ($results === 'exit') {
+ exit();
+ }
/** @var \Drupal\package_manager\ValidationResult $result */
foreach ($results as $result) {
if ($result->getSeverity() === SystemManager::REQUIREMENT_ERROR) {
diff --git a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
index 647878fcae..8bce0be1a8 100644
--- a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
+++ b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
@@ -78,7 +78,8 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
$this->container->get('package_manager.committer'),
$this->container->get('file_system'),
$this->container->get('event_dispatcher'),
- $this->container->get('tempstore.shared')
+ $this->container->get('tempstore.shared'),
+ $this->container->get('datetime.time')
);
}
@@ -297,9 +298,9 @@ class TestStage extends Stage {
/**
* {@inheritdoc}
*/
- protected function dispatch(StageEvent $event): void {
+ protected function dispatch(StageEvent $event, callable $on_error = NULL): void {
try {
- parent::dispatch($event);
+ parent::dispatch($event, $on_error);
}
catch (StageException $e) {
// Attach the event object to the exception so that test code can verify
diff --git a/package_manager/tests/src/Kernel/StageTest.php b/package_manager/tests/src/Kernel/StageTest.php
index 0ed5f56049..4d8e419ceb 100644
--- a/package_manager/tests/src/Kernel/StageTest.php
+++ b/package_manager/tests/src/Kernel/StageTest.php
@@ -2,6 +2,11 @@
namespace Drupal\Tests\package_manager\Kernel;
+use Drupal\Component\Datetime\Time;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\package_manager\Event\PreApplyEvent;
+use Drupal\package_manager\Exception\StageException;
+
/**
* @coversDefaultClass \Drupal\package_manager\Stage
*
@@ -9,6 +14,16 @@ namespace Drupal\Tests\package_manager\Kernel;
*/
class StageTest extends PackageManagerKernelTestBase {
+ /**
+ * {@inheritdoc}
+ */
+ public function register(ContainerBuilder $container) {
+ parent::register($container);
+
+ $container->getDefinition('datetime.time')
+ ->setClass(TestTime::class);
+ }
+
/**
* @covers ::getStageDirectory
*/
@@ -27,4 +42,78 @@ class StageTest extends PackageManagerKernelTestBase {
$this->createStage()->getStageDirectory();
}
+ /**
+ * Data provider for ::testDestroyDuringApply().
+ *
+ * @return array[]
+ * Sets of arguments to pass to the test method.
+ */
+ public function providerDestroyDuringApply(): array {
+ return [
+ 'force destroy, not stale' => [TRUE, 1, TRUE],
+ 'regular destroy, not stale' => [FALSE, 1, TRUE],
+ 'force destroy, stale' => [TRUE, 7200, FALSE],
+ 'regular destroy, stale' => [FALSE, 7200, FALSE],
+ ];
+ }
+
+ /**
+ * Tests destroying a stage while applying it.
+ *
+ * @param bool $force
+ * Whether or not the stage should be force destroyed.
+ * @param int $time_offset
+ * How many simulated seconds should have elapsed between the PreApplyEvent
+ * being dispatched and the attempt to destroy the stage.
+ * @param bool $expect_exception
+ * Whether or not destroying the stage will raise an exception.
+ *
+ * @dataProvider providerDestroyDuringApply
+ */
+ public function testDestroyDuringApply(bool $force, int $time_offset, bool $expect_exception): void {
+ $listener = function (PreApplyEvent $event) use ($force, $time_offset): void {
+ // Simulate that a certain amount of time has passed since we started
+ // applying staged changes. After a point, it should be possible to
+ // destroy the stage even if it hasn't finished.
+ TestTime::$offset = $time_offset;
+
+ // No real-life event subscriber should try to destroy the stage while
+ // handling another event. The only reason we're doing it here is to
+ // simulate an attempt to destroy the stage while it's being applied, for
+ // testing purposes.
+ $event->getStage()->destroy($force);
+ };
+ $this->container->get('event_dispatcher')
+ ->addListener(PreApplyEvent::class, $listener);
+
+ $stage = $this->createStage();
+ $stage->create();
+ if ($expect_exception) {
+ $this->expectException(StageException::class);
+ $this->expectExceptionMessage('Cannot destroy the staging area while it is being applied to the active directory.');
+ }
+ $stage->apply();
+ }
+
+}
+
+/**
+ * A test-only implementation of the time service.
+ */
+class TestTime extends Time {
+
+ /**
+ * An offset to add to the request time.
+ *
+ * @var int
+ */
+ public static $offset = 0;
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getRequestTime() {
+ return parent::getRequestTime() + static::$offset;
+ }
+
}
diff --git a/src/Form/UpdateReady.php b/src/Form/UpdateReady.php
index 6f81bac387..6fd63c3fab 100644
--- a/src/Form/UpdateReady.php
+++ b/src/Form/UpdateReady.php
@@ -11,6 +11,7 @@ use Drupal\Core\Form\FormBase;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Messenger\MessengerInterface;
use Drupal\Core\State\StateInterface;
+use Drupal\package_manager\Exception\StageException;
use Drupal\package_manager\Exception\StageOwnershipException;
use Symfony\Component\DependencyInjection\ContainerInterface;
@@ -193,9 +194,14 @@ class UpdateReady extends FormBase {
* Cancels the in-progress update.
*/
public function cancel(array &$form, FormStateInterface $form_state): void {
- $this->updater->destroy();
- $this->messenger()->addStatus($this->t('The update was successfully cancelled.'));
- $form_state->setRedirect('automatic_updates.report_update');
+ try {
+ $this->updater->destroy();
+ $this->messenger()->addStatus($this->t('The update was successfully cancelled.'));
+ $form_state->setRedirect('automatic_updates.report_update');
+ }
+ catch (StageException $e) {
+ $this->messenger()->addError($e->getMessage());
+ }
}
}
diff --git a/src/Form/UpdaterForm.php b/src/Form/UpdaterForm.php
index d6e2daa4f0..e1ee4e2e9a 100644
--- a/src/Form/UpdaterForm.php
+++ b/src/Form/UpdaterForm.php
@@ -14,6 +14,7 @@ use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Link;
use Drupal\Core\State\StateInterface;
use Drupal\Core\Url;
+use Drupal\package_manager\Exception\StageException;
use Drupal\package_manager\Exception\StageOwnershipException;
use Drupal\system\SystemManager;
use Drupal\update\UpdateManagerInterface;
@@ -276,8 +277,13 @@ class UpdaterForm extends FormBase {
* Submit function to delete an existing in-progress update.
*/
public function deleteExistingUpdate(): void {
- $this->updater->destroy(TRUE);
- $this->messenger()->addMessage($this->t("Staged update deleted"));
+ try {
+ $this->updater->destroy(TRUE);
+ $this->messenger()->addMessage($this->t("Staged update deleted"));
+ }
+ catch (StageException $e) {
+ $this->messenger()->addError($e->getMessage());
+ }
}
/**
diff --git a/src/Updater.php b/src/Updater.php
index f8c3ca05e7..c333f6c376 100644
--- a/src/Updater.php
+++ b/src/Updater.php
@@ -91,9 +91,9 @@ class Updater extends Stage {
/**
* {@inheritdoc}
*/
- protected function dispatch(StageEvent $event): void {
+ protected function dispatch(StageEvent $event, callable $on_error = NULL): void {
try {
- parent::dispatch($event);
+ parent::dispatch($event, $on_error);
}
catch (StageValidationException $e) {
throw new UpdateException($e->getResults(), $e->getMessage() ?: "Unable to complete the update because of errors.", $e->getCode(), $e);
diff --git a/tests/src/Functional/UpdaterFormTest.php b/tests/src/Functional/UpdaterFormTest.php
index c68f84dcba..2e4f6ed525 100644
--- a/tests/src/Functional/UpdaterFormTest.php
+++ b/tests/src/Functional/UpdaterFormTest.php
@@ -3,6 +3,8 @@
namespace Drupal\Tests\automatic_updates\Functional;
use Drupal\automatic_updates\Event\ReadinessCheckEvent;
+use Drupal\automatic_updates_test\Datetime\TestTime;
+use Drupal\package_manager\Event\PreApplyEvent;
use Drupal\package_manager\Event\PreCreateEvent;
use Drupal\package_manager\ValidationResult;
use Drupal\automatic_updates_test\EventSubscriber\TestSubscriber1;
@@ -240,6 +242,7 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
*/
public function testDeleteExistingUpdate(): void {
$conflict_message = 'Cannot begin an update because another Composer operation is currently in progress.';
+ $cancelled_message = 'The update was successfully cancelled.';
$assert_session = $this->assertSession();
$page = $this->getSession()->getPage();
@@ -263,7 +266,7 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
// Delete the existing update.
$page->pressButton('Cancel update');
$assert_session->addressEquals('/admin/reports/updates/automatic-update');
- $assert_session->pageTextContains('The update was successfully cancelled.');
+ $assert_session->pageTextContains($cancelled_message);
$assert_session->pageTextNotContains($conflict_message);
// Ensure we can start another update after deleting the existing one.
$page->pressButton('Update');
@@ -281,12 +284,60 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
$this->drupalGet('/admin/reports/updates/automatic-update');
$assert_session->pageTextContains($conflict_message);
$assert_session->buttonNotExists('Update');
- // We should be able to delete the previous update, and then we should be
- // able to start a new one.
+ // We should be able to delete the previous update, then start a new one.
$page->pressButton('Delete existing update');
$assert_session->pageTextContains('Staged update deleted');
$assert_session->pageTextNotContains($conflict_message);
- $assert_session->buttonExists('Update');
+ $page->pressButton('Update');
+ $this->checkForMetaRefresh();
+ $this->assertUpdateReady();
+
+ // Stop execution during pre-apply. This should make Package Manager think
+ // the staged changes are being applied and raise an error if we try to
+ // cancel the update.
+ TestSubscriber1::setExit(PreApplyEvent::class);
+ $page->pressButton('Continue');
+ $this->checkForMetaRefresh();
+ $page->clickLink('the error page');
+ $page->pressButton('Cancel update');
+ // The exception should have been caught and displayed in the messages area.
+ $assert_session->statusCodeEquals(200);
+ $destroy_error = 'Cannot destroy the staging area while it is being applied to the active directory.';
+ $assert_session->pageTextContains($destroy_error);
+ $assert_session->pageTextNotContains($cancelled_message);
+
+ // We should get the same error if we log in as another user and try to
+ // delete the staged update.
+ $this->drupalLogin($this->rootUser);
+ $this->drupalGet('/admin/reports/updates/automatic-update');
+ $assert_session->pageTextContains($conflict_message);
+ $page->pressButton('Delete existing update');
+ $assert_session->statusCodeEquals(200);
+ $assert_session->pageTextContains($destroy_error);
+ $assert_session->pageTextNotContains('Staged update deleted');
+
+ // Two hours later, Package Manager should consider the stage to be stale,
+ // allowing the staged update to be deleted.
+ TestTime::setFakeTimeByOffset('+2 hours');
+ $this->getSession()->reload();
+ $assert_session->pageTextContains($conflict_message);
+ $page->pressButton('Delete existing update');
+ $assert_session->statusCodeEquals(200);
+ $assert_session->pageTextContains('Staged update deleted');
+
+ // If a legitimate error is raised during pre-apply, we should be able to
+ // delete the staged update right away.
+ $this->createTestValidationResults();
+ $results = $this->testResults['checker_1']['1 error'];
+ TestSubscriber1::setTestResult($results, PreApplyEvent::class);
+ $page->pressButton('Update');
+ $this->checkForMetaRefresh();
+ $this->assertUpdateReady();
+ $page->pressButton('Continue');
+ $this->checkForMetaRefresh();
+ $page->clickLink('the error page');
+ $page->pressButton('Cancel update');
+ $assert_session->pageTextContains($cancelled_message);
}
/**
--
GitLab