Issue #3266092 by kunal.sachdev, tedbow, phenaproxima: Make sure staging root...

Issue #3266092 by kunal.sachdev, tedbow, phenaproxima: Make sure staging root is unique for each Drupal site

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'

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;
   }
 
 }

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'),

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();
   }
 

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());
   }
 
   /**

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');
   }
 

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();
   }
 
   /**