Issue #3317232 by phenaproxima, tedbow: Stages can be owned by someone other than their creator

automatic_updates.services.yml

@@ -98,6 +98,8 @@ services:
       - { name: event_subscriber }
   automatic_updates.config_subscriber:
     class: Drupal\automatic_updates\EventSubscriber\ConfigSubscriber
+    arguments:
+      - '@automatic_updates.status_checker'
     tags:
       - { name: event_subscriber }
   automatic_updates.validator.scaffold_file_permissions:

package_manager/src/Stage.php

@@ -148,6 +148,13 @@ class Stage implements LoggerAwareInterface {
    */
   protected $eventDispatcher;
 
+  /**
+   * The shared temp store factory.
+   *
+   * @var \Drupal\Core\TempStore\SharedTempStoreFactory
+   */
+  protected $tempStoreFactory;
+
   /**
    * The shared temp store.
    *
@@ -202,7 +209,7 @@ class Stage implements LoggerAwareInterface {
    *   The file system service.
    * @param \Symfony\Contracts\EventDispatcher\EventDispatcherInterface $event_dispatcher
    *   The event dispatcher service.
-   * @param \Drupal\Core\TempStore\SharedTempStoreFactory $shared_tempstore
+   * @param \Drupal\Core\TempStore\SharedTempStoreFactory $temp_store_factory
    *   The shared tempstore factory.
    * @param \Drupal\Component\Datetime\TimeInterface $time
    *   The time service.
@@ -211,7 +218,7 @@ class Stage implements LoggerAwareInterface {
    * @param \Drupal\package_manager\FailureMarker $failure_marker
    *   The failure marker service.
    */
-  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, PathFactoryInterface $path_factory = NULL, FailureMarker $failure_marker = NULL) {
+  public function __construct(ConfigFactoryInterface $config_factory, PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $temp_store_factory, TimeInterface $time, PathFactoryInterface $path_factory = NULL, FailureMarker $failure_marker = NULL) {
     $this->configFactory = $config_factory;
     $this->pathLocator = $path_locator;
     $this->beginner = $beginner;
@@ -220,7 +227,8 @@ class Stage implements LoggerAwareInterface {
     $this->fileSystem = $file_system;
     $this->eventDispatcher = $event_dispatcher;
     $this->time = $time;
-    $this->tempStore = $shared_tempstore->get('package_manager_stage');
+    $this->tempStoreFactory = $temp_store_factory;
+    $this->tempStore = $temp_store_factory->get('package_manager_stage');
     if (empty($path_factory)) {
       @trigger_error('Calling ' . __METHOD__ . '() without the $path_factory argument is deprecated in automatic_updates:8.x-2.3 and will be required before automatic_updates:3.0.0. See https://www.drupal.org/node/3310706.', E_USER_DEPRECATED);
       $path_factory = new PathFactory();
@@ -259,7 +267,7 @@ class Stage implements LoggerAwareInterface {
   protected function getMetadata(string $key) {
     $this->checkOwnership();
 
-    $metadata = $this->tempStore->getIfOwner(static::TEMPSTORE_METADATA_KEY) ?: [];
+    $metadata = $this->tempStore->get(static::TEMPSTORE_METADATA_KEY) ?: [];
     return $metadata[$key] ?? NULL;
   }
 
@@ -317,6 +325,12 @@ class Stage implements LoggerAwareInterface {
     // while the event is being processed, the stage is marked as available.
     // @see ::dispatch()
     $id = Crypt::randomBytesBase64();
+    // Re-acquire the tempstore to ensure that the lock is written by whoever is
+    // actually logged in (or not) right now, since it's possible that the stage
+    // was instantiated (i.e., __construct() was called) by a different session,
+    // which would result in the lock having the wrong owner and the stage not
+    // being claimable by whoever is actually creating it.
+    $this->tempStore = $this->tempStoreFactory->get('package_manager_stage');
     $this->tempStore->set(static::TEMPSTORE_LOCK_KEY, [$id, static::class]);
     $this->claim($id);
 
@@ -610,7 +624,7 @@ class Stage implements LoggerAwareInterface {
       throw new StageException('Cannot claim the stage because no stage has been created.');
     }
 
-    $stored_lock = $this->tempStore->getIfOwner(self::TEMPSTORE_LOCK_KEY);
+    $stored_lock = $this->tempStore->getIfOwner(static::TEMPSTORE_LOCK_KEY);
     if (!$stored_lock) {
       throw new StageOwnershipException('Cannot claim the stage because it is not owned by the current user or session.');
     }

package_manager/tests/src/Kernel/StageOwnershipTest.php

@@ -127,6 +127,20 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
     }
   }
 
+  /**
+   * Tests that the stage is owned by the person who calls create() on it.
+   */
+  public function testStageOwnedByCreator(): void {
+    // Even if the stage is instantiated before anyone is logged in, it should
+    // still be owned (and claimable) by the user who called create() on it.
+    $stage = $this->createStage();
+
+    $account = $this->createUser([], NULL, FALSE, ['uid' => 2]);
+    $this->setCurrentUser($account);
+    $id = $stage->create();
+    $this->createStage()->claim($id);
+  }
+
   /**
    * Tests behavior of claiming a stage.
    */
@@ -253,8 +267,8 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
         // Normally, the stage will call this method as it tries to make
         // everything in the staging area writable so it can be deleted. We
         // don't wan't to do that in this test, since we're specifically testing
-        // what happens when we try to delete a staging area with unwritable
-        // files.
+        // what happens when we try to delete a staging area with
+        // write-protected files.
       }
 
     });
@@ -264,8 +278,8 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
     $stage->create();
     $this->assertFalse($stage->isAvailable());
 
-    // Make the stage directory unwritable, which should prevent files in it
-    // from being deleted.
+    // Write-protect the stage directory, which should prevent files in it from
+    // being deleted.
     $dir = $stage->getStageDirectory();
     chmod($dir, 0400);
     $this->assertDirectoryIsNotWritable($dir);

src/CronUpdater.php

@@ -283,11 +283,7 @@ class CronUpdater extends Updater {
     $owner = $this->tempStore->getMetadata(static::TEMPSTORE_LOCK_KEY)
       ->getOwnerId();
     // Reload the tempstore with the correct owner ID so we can claim the stage.
-    // We use \Drupal::service() here because, given what we inherit from the
-    // parent class, there's no clean way to inject the shared tempstore
-    // factory.
-    $this->tempStore = \Drupal::service('tempstore.shared')
-      ->get('package_manager_stage', $owner);
+    $this->tempStore = $this->tempStoreFactory->get('package_manager_stage', $owner);
 
     $this->claim($stage_id);
 

src/EventSubscriber/ConfigSubscriber.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\automatic_updates\EventSubscriber;
 
+use Drupal\automatic_updates\Validation\StatusChecker;
 use Drupal\Core\Config\ConfigCrudEvent;
 use Drupal\Core\Config\ConfigEvents;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -19,6 +20,23 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
  */
 final class ConfigSubscriber implements EventSubscriberInterface {
 
+  /**
+   * The status checker service.
+   *
+   * @var \Drupal\automatic_updates\Validation\StatusChecker
+   */
+  protected $statusChecker;
+
+  /**
+   * Constructs a ConfigSubscriber object.
+   *
+   * @param \Drupal\automatic_updates\Validation\StatusChecker $status_checker
+   *   The status checker service.
+   */
+  public function __construct(StatusChecker $status_checker) {
+    $this->statusChecker = $status_checker;
+  }
+
   /**
    * {@inheritdoc}
    */
@@ -36,8 +54,7 @@ final class ConfigSubscriber implements EventSubscriberInterface {
    */
   public function onConfigSave(ConfigCrudEvent $event): void {
     if ($event->getConfig()->getName() === 'package_manager.settings' && $event->isChanged('executables.composer')) {
-      \Drupal::service('automatic_updates.status_checker')
-        ->clearStoredResults();
+      $this->statusChecker->clearStoredResults();
     }
   }