diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml index 0e50d2b22d0d06fbd7274e31908f91db84115d89..f0c7f2521161b43ecbbe850393b22a6a314033ee 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 b93a3bcf7f6efadadb885a4fb7a62be3d95e552c..0a906eda0625393d54d5b755b901f73c69b0ae31 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 9c237bc0006bec751e3304e6a67259e589a92544..cf6c28efa633edab946f1b31ad7ac19c3f6b5f48 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 8bce0be1a87a2f0d46818c01a939e73b909a0300..5b636ff35541341aa3a6e1c786f666a983022b95 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 4d8e419cebd97727fbd4d9dd67800fcbf6e36755..b08b47d8c7422c945292a4207b258500a4bdfd42 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 03ba20fc2416739cd7d8734981f59f448423cf4c..69d1fb18720189c8f2d32423ffe42d11bbea11f5 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 454ac6bb5d6b7129c1bbe4129149e934849d5e48..277027b7cb81b5aaf26106c7ba012d95d3046052 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(); } /**