From 671e237952abb5f635602938a03eb51354a9255a Mon Sep 17 00:00:00 2001
From: phenaproxima <phenaproxima@205645.no-reply.drupal.org>
Date: Mon, 24 Oct 2022 21:50:24 +0000
Subject: [PATCH] Issue #3317232 by phenaproxima, tedbow: Stages can be owned
by someone other than their creator
---
automatic_updates.services.yml | 2 ++
package_manager/src/Stage.php | 24 +++++++++++++++----
.../tests/src/Kernel/StageOwnershipTest.php | 22 +++++++++++++----
src/CronUpdater.php | 6 +----
src/EventSubscriber/ConfigSubscriber.php | 21 ++++++++++++++--
5 files changed, 59 insertions(+), 16 deletions(-)
diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml
index bdd24e6eda..3c3377cab9 100644
--- a/automatic_updates.services.yml
+++ b/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:
diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php
index f6e2032c26..7e5188ba00 100644
--- a/package_manager/src/Stage.php
+++ b/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.');
}
diff --git a/package_manager/tests/src/Kernel/StageOwnershipTest.php b/package_manager/tests/src/Kernel/StageOwnershipTest.php
index cfd06a1a6f..b945927b6f 100644
--- a/package_manager/tests/src/Kernel/StageOwnershipTest.php
+++ b/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);
diff --git a/src/CronUpdater.php b/src/CronUpdater.php
index 5af93a252d..3c00e94829 100644
--- a/src/CronUpdater.php
+++ b/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);
diff --git a/src/EventSubscriber/ConfigSubscriber.php b/src/EventSubscriber/ConfigSubscriber.php
index 439e86ed27..267f4177ff 100644
--- a/src/EventSubscriber/ConfigSubscriber.php
+++ b/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();
}
}
--
GitLab