From 942ecfae2263f5a7363ee749c9194a21832b4e29 Mon Sep 17 00:00:00 2001
From: "kunal.sachdev" <kunal.sachdev@3685163.no-reply.drupal.org>
Date: Thu, 3 Mar 2022 19:42:09 +0000
Subject: [PATCH] Issue #3266092 by kunal.sachdev, tedbow, phenaproxima: Make
sure staging root is unique for each Drupal site
---
automatic_updates.services.yml | 3 +-
package_manager/src/Stage.php | 57 ++++++++++++-------
.../src/ApiController.php | 1 +
.../Kernel/PackageManagerKernelTestBase.php | 3 +-
.../tests/src/Kernel/StageTest.php | 20 ++++++-
src/CronUpdater.php | 13 +----
.../Kernel/AutomaticUpdatesKernelTestBase.php | 8 +--
7 files changed, 66 insertions(+), 39 deletions(-)
diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml
index 0e50d2b22d..f0c7f25211 100644
--- a/automatic_updates.services.yml
+++ b/automatic_updates.services.yml
@@ -14,6 +14,7 @@ services:
automatic_updates.updater:
class: Drupal\automatic_updates\Updater
arguments:
+ - '@config.factory'
- '@package_manager.path_locator'
- '@package_manager.beginner'
- '@package_manager.stager'
@@ -25,8 +26,8 @@ services:
automatic_updates.cron_updater:
class: Drupal\automatic_updates\CronUpdater
arguments:
- - '@config.factory'
- '@logger.factory'
+ - '@config.factory'
- '@package_manager.path_locator'
- '@package_manager.beginner'
- '@package_manager.stager'
diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php
index b93a3bcf7f..0a906eda06 100644
--- a/package_manager/src/Stage.php
+++ b/package_manager/src/Stage.php
@@ -5,6 +5,7 @@ namespace Drupal\package_manager;
use Drupal\Component\Datetime\TimeInterface;
use Drupal\Component\FileSystem\FileSystem;
use Drupal\Component\Utility\Crypt;
+use Drupal\Core\Config\ConfigFactoryInterface;
use Drupal\Core\File\Exception\FileException;
use Drupal\Core\File\FileSystemInterface;
use Drupal\Core\TempStore\SharedTempStoreFactory;
@@ -39,6 +40,16 @@ use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
* operations on the staging area, and the stage must be "claimed" by its owner
* before any such operations are done. A stage is claimed by presenting a
* unique token that is generated when the stage is created.
+ *
+ * Although a site can only have one staging area, it is possible for privileged
+ * users to destroy a stage created by another user. To prevent such actions
+ * from putting the file system into an uncertain state (for example, if a stage
+ * is destroyed by another user while it is still being created), the staging
+ * directory has a randomly generated name. For additional cleanliness, all
+ * staging directories created by a specific site live in a single directory,
+ * called the "staging root" and identified by the UUID of the current site
+ * (e.g. `/tmp/.package_managerSITE_UUID`), which is deleted when any stage
+ * created by that site is destroyed.
*/
class Stage {
@@ -66,6 +77,13 @@ class Stage {
*/
private const TEMPSTORE_APPLY_TIME_KEY = 'apply_time';
+ /**
+ * The config factory service.
+ *
+ * @var \Drupal\Core\Config\ConfigFactoryInterface
+ */
+ protected $configFactory;
+
/**
* The path locator service.
*
@@ -134,6 +152,8 @@ class Stage {
/**
* Constructs a new Stage object.
*
+ * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+ * The config factory service.
* @param \Drupal\package_manager\PathLocator $path_locator
* The path locator service.
* @param \PhpTuf\ComposerStager\Domain\BeginnerInterface $beginner
@@ -151,7 +171,8 @@ class Stage {
* @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, TimeInterface $time) {
+ public function __construct(ConfigFactoryInterface $config_factory, PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore, TimeInterface $time) {
+ $this->configFactory = $config_factory;
$this->pathLocator = $path_locator;
$this->beginner = $beginner;
$this->stager = $stager;
@@ -337,20 +358,17 @@ class Stage {
}
$this->dispatch(new PreDestroyEvent($this));
- // Delete all directories in parent staging directory.
- $parent_stage_dir = static::getStagingRoot();
- if (is_dir($parent_stage_dir)) {
- try {
- $this->fileSystem->deleteRecursive($parent_stage_dir, function (string $path): void {
- $this->fileSystem->chmod($path, 0777);
- });
- }
- catch (FileException $e) {
- // Deliberately swallow the exception so that the stage will be marked
- // as available and the post-destroy event will be fired, even if the
- // staging area can't actually be deleted. The file system service logs
- // the exception, so we don't need to do anything else here.
- }
+ // Delete the staging root and everything in it.
+ try {
+ $this->fileSystem->deleteRecursive($this->getStagingRoot(), function (string $path): void {
+ $this->fileSystem->chmod($path, 0777);
+ });
+ }
+ catch (FileException $e) {
+ // Deliberately swallow the exception so that the stage will be marked
+ // as available and the post-destroy event will be fired, even if the
+ // staging area can't actually be deleted. The file system service logs
+ // the exception, so we don't need to do anything else here.
}
$this->markAsAvailable();
$this->dispatch(new PostDestroyEvent($this));
@@ -498,14 +516,12 @@ class Stage {
*
* @throws \LogicException
* If this method is called before the stage has been created or claimed.
- *
- * @todo Make this method public in https://www.drupal.org/i/3251972.
*/
public function getStageDirectory(): string {
if (!$this->lock) {
throw new \LogicException(__METHOD__ . '() cannot be called because the stage has not been created or claimed.');
}
- return static::getStagingRoot() . DIRECTORY_SEPARATOR . $this->lock[0];
+ return $this->getStagingRoot() . DIRECTORY_SEPARATOR . $this->lock[0];
}
/**
@@ -515,8 +531,9 @@ class Stage {
* The absolute path of the directory containing the staging areas managed
* by this class.
*/
- protected static function getStagingRoot(): string {
- return FileSystem::getOsTemporaryDirectory() . DIRECTORY_SEPARATOR . '.package_manager';
+ protected function getStagingRoot(): string {
+ $site_id = $this->configFactory->get('system.site')->get('uuid');
+ return FileSystem::getOsTemporaryDirectory() . DIRECTORY_SEPARATOR . '.package_manager' . $site_id;
}
}
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 9c237bc000..cf6c28efa6 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
@@ -35,6 +35,7 @@ class ApiController extends ControllerBase {
*/
public static function create(ContainerInterface $container) {
$stage = new Stage(
+ $container->get('config.factory'),
$container->get('package_manager.path_locator'),
$container->get('package_manager.beginner'),
$container->get('package_manager.stager'),
diff --git a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
index 8bce0be1a8..5b636ff355 100644
--- a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
+++ b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
@@ -72,6 +72,7 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
*/
protected function createStage(): TestStage {
return new TestStage(
+ $this->container->get('config.factory'),
$this->container->get('package_manager.path_locator'),
$this->container->get('package_manager.beginner'),
$this->container->get('package_manager.stager'),
@@ -291,7 +292,7 @@ class TestStage extends Stage {
/**
* {@inheritdoc}
*/
- public static function getStagingRoot(): string {
+ public function getStagingRoot(): string {
return static::$stagingRoot ?: parent::getStagingRoot();
}
diff --git a/package_manager/tests/src/Kernel/StageTest.php b/package_manager/tests/src/Kernel/StageTest.php
index 4d8e419ceb..b08b47d8c7 100644
--- a/package_manager/tests/src/Kernel/StageTest.php
+++ b/package_manager/tests/src/Kernel/StageTest.php
@@ -26,11 +26,29 @@ class StageTest extends PackageManagerKernelTestBase {
/**
* @covers ::getStageDirectory
+ * @covers ::getStagingRoot
*/
public function testGetStageDirectory(): void {
+ $this->container->get('module_installer')->install(['system']);
+ // Ensure we have an up-to-date-container.
+ $this->container = $this->container->get('kernel')->getContainer();
+
+ // Ensure that a site ID was generated.
+ // @see system_install()
+ $site_id = $this->config('system.site')->get('uuid');
+ $this->assertNotEmpty($site_id);
+
$stage = $this->createStage();
$id = $stage->create();
- $this->assertStringEndsWith("/.package_manager/$id", $stage->getStageDirectory());
+ $this->assertStringEndsWith("/.package_manager$site_id/$id", $stage->getStageDirectory());
+ $stage->destroy();
+
+ $stage = $this->createStage();
+ $another_id = $stage->create();
+ // The new stage ID should be unique, but the parent directory should be
+ // unchanged.
+ $this->assertNotSame($id, $another_id);
+ $this->assertStringEndsWith("/.package_manager$site_id/$another_id", $stage->getStageDirectory());
}
/**
diff --git a/src/CronUpdater.php b/src/CronUpdater.php
index 03ba20fc24..69d1fb1872 100644
--- a/src/CronUpdater.php
+++ b/src/CronUpdater.php
@@ -2,7 +2,6 @@
namespace Drupal\automatic_updates;
-use Drupal\Core\Config\ConfigFactoryInterface;
use Drupal\Core\Logger\LoggerChannelFactoryInterface;
use Drupal\package_manager\Exception\StageValidationException;
@@ -36,13 +35,6 @@ class CronUpdater extends Updater {
*/
public const ALL = 'patch';
- /**
- * The config factory service.
- *
- * @var \Drupal\Core\Config\ConfigFactoryInterface
- */
- protected $configFactory;
-
/**
* The logger.
*
@@ -53,16 +45,13 @@ class CronUpdater extends Updater {
/**
* Constructs a CronUpdater object.
*
- * @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(ConfigFactoryInterface $config_factory, LoggerChannelFactoryInterface $logger_factory, ...$arguments) {
+ public function __construct(LoggerChannelFactoryInterface $logger_factory, ...$arguments) {
parent::__construct(...$arguments);
- $this->configFactory = $config_factory;
$this->logger = $logger_factory->get('automatic_updates');
}
diff --git a/tests/src/Kernel/AutomaticUpdatesKernelTestBase.php b/tests/src/Kernel/AutomaticUpdatesKernelTestBase.php
index 454ac6bb5d..277027b7cb 100644
--- a/tests/src/Kernel/AutomaticUpdatesKernelTestBase.php
+++ b/tests/src/Kernel/AutomaticUpdatesKernelTestBase.php
@@ -138,8 +138,8 @@ class TestUpdater extends Updater {
/**
* {@inheritdoc}
*/
- protected static function getStagingRoot(): string {
- return TestStage::getStagingRoot();
+ public function getStagingRoot(): string {
+ return TestStage::$stagingRoot ?: parent::getStagingRoot();
}
}
@@ -152,8 +152,8 @@ class TestCronUpdater extends CronUpdater {
/**
* {@inheritdoc}
*/
- protected static function getStagingRoot(): string {
- return TestStage::getStagingRoot();
+ public function getStagingRoot(): string {
+ return TestStage::$stagingRoot ?: parent::getStagingRoot();
}
/**
--
GitLab