From c84365f714c7b584c70f13aa2f11a252d83b4806 Mon Sep 17 00:00:00 2001
From: catch <catch@35733.no-reply.drupal.org>
Date: Fri, 3 May 2024 09:36:18 +0100
Subject: [PATCH] Issue #3443761 by amateescu: Workspace negotiators shouldn't
be responsible for loading the negotiated workspace entity
---
.../QueryParameterWorkspaceNegotiator.php | 37 ++++--
.../Negotiator/SessionWorkspaceNegotiator.php | 52 ++------
.../WorkspaceIdNegotiatorInterface.php | 23 ++++
.../WorkspaceNegotiatorInterface.php | 19 +--
.../workspaces/src/WorkspaceManager.php | 19 ++-
.../Negotiator/TestWorkspaceNegotiator.php | 12 +-
.../ActiveWorkspaceUpdateTest.php | 4 +
.../WorkspaceQueryParameterNegotiatorTest.php | 123 ++++++++++++++++++
8 files changed, 220 insertions(+), 69 deletions(-)
create mode 100644 core/modules/workspaces/src/Negotiator/WorkspaceIdNegotiatorInterface.php
create mode 100644 core/modules/workspaces/tests/src/Kernel/WorkspaceQueryParameterNegotiatorTest.php
diff --git a/core/modules/workspaces/src/Negotiator/QueryParameterWorkspaceNegotiator.php b/core/modules/workspaces/src/Negotiator/QueryParameterWorkspaceNegotiator.php
index a2917cd46b2f..1f0b688541ff 100644
--- a/core/modules/workspaces/src/Negotiator/QueryParameterWorkspaceNegotiator.php
+++ b/core/modules/workspaces/src/Negotiator/QueryParameterWorkspaceNegotiator.php
@@ -2,6 +2,8 @@
namespace Drupal\workspaces\Negotiator;
+use Drupal\Component\Utility\Crypt;
+use Drupal\Core\Site\Settings;
use Symfony\Component\HttpFoundation\Request;
/**
@@ -13,21 +15,38 @@ class QueryParameterWorkspaceNegotiator extends SessionWorkspaceNegotiator {
* {@inheritdoc}
*/
public function applies(Request $request) {
- return is_string($request->query->get('workspace')) && parent::applies($request);
+ return is_string($request->query->get('workspace'))
+ && is_string($request->query->get('token'))
+ && parent::applies($request);
}
/**
* {@inheritdoc}
*/
- public function getActiveWorkspace(Request $request) {
- $workspace_id = $request->query->get('workspace');
-
- if ($workspace_id && ($workspace = $this->workspaceStorage->load($workspace_id))) {
- $this->setActiveWorkspace($workspace);
- return $workspace;
- }
+ public function getActiveWorkspaceId(Request $request): ?string {
+ $workspace_id = (string) $request->query->get('workspace');
+ $token = (string) $request->query->get('token');
+ $is_valid_token = hash_equals($this->getQueryToken($workspace_id), $token);
+
+ // This negotiator receives a workspace ID from user input, so a minimal
+ // validation is needed to ensure that we protect against fake input before
+ // the workspace manager fully validates the negotiated workspace ID.
+ // @see \Drupal\workspaces\WorkspaceManager::getActiveWorkspace()
+ return $is_valid_token ? $workspace_id : NULL;
+ }
- return NULL;
+ /**
+ * Calculates a token based on a workspace ID.
+ *
+ * @param string $workspace_id
+ * The workspace ID.
+ *
+ * @return string
+ * An 8 char token based on the given workspace ID.
+ */
+ protected function getQueryToken(string $workspace_id): string {
+ // Return the first 8 characters.
+ return substr(Crypt::hmacBase64($workspace_id, Settings::getHashSalt()), 0, 8);
}
}
diff --git a/core/modules/workspaces/src/Negotiator/SessionWorkspaceNegotiator.php b/core/modules/workspaces/src/Negotiator/SessionWorkspaceNegotiator.php
index 0a0550b4cd7d..8b2e0b9a3812 100644
--- a/core/modules/workspaces/src/Negotiator/SessionWorkspaceNegotiator.php
+++ b/core/modules/workspaces/src/Negotiator/SessionWorkspaceNegotiator.php
@@ -11,60 +11,36 @@
/**
* Defines the session workspace negotiator.
*/
-class SessionWorkspaceNegotiator implements WorkspaceNegotiatorInterface {
+class SessionWorkspaceNegotiator implements WorkspaceNegotiatorInterface, WorkspaceIdNegotiatorInterface {
- /**
- * The current user.
- *
- * @var \Drupal\Core\Session\AccountInterface
- */
- protected $currentUser;
-
- /**
- * The session.
- *
- * @var \Symfony\Component\HttpFoundation\Session\Session
- */
- protected $session;
-
- /**
- * The workspace storage handler.
- *
- * @var \Drupal\Core\Entity\EntityStorageInterface
- */
- protected $workspaceStorage;
+ public function __construct(
+ protected readonly AccountInterface $currentUser,
+ protected readonly Session $session,
+ protected readonly EntityTypeManagerInterface $entityTypeManager
+ ) {}
/**
- * Constructor.
- *
- * @param \Drupal\Core\Session\AccountInterface $current_user
- * The current user.
- * @param \Symfony\Component\HttpFoundation\Session\Session $session
- * The session.
- * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
- * The entity type manager.
+ * {@inheritdoc}
*/
- public function __construct(AccountInterface $current_user, Session $session, EntityTypeManagerInterface $entity_type_manager) {
- $this->currentUser = $current_user;
- $this->session = $session;
- $this->workspaceStorage = $entity_type_manager->getStorage('workspace');
+ public function applies(Request $request) {
+ // This negotiator only applies if the current user is authenticated.
+ return $this->currentUser->isAuthenticated();
}
/**
* {@inheritdoc}
*/
- public function applies(Request $request) {
- // This negotiator only applies if the current user is authenticated.
- return $this->currentUser->isAuthenticated();
+ public function getActiveWorkspaceId(Request $request): ?string {
+ return $this->session->get('active_workspace_id');
}
/**
* {@inheritdoc}
*/
public function getActiveWorkspace(Request $request) {
- $workspace_id = $this->session->get('active_workspace_id');
+ $workspace_id = $this->getActiveWorkspaceId($request);
- if ($workspace_id && ($workspace = $this->workspaceStorage->load($workspace_id))) {
+ if ($workspace_id && ($workspace = $this->entityTypeManager->getStorage('workspace')->load($workspace_id))) {
return $workspace;
}
diff --git a/core/modules/workspaces/src/Negotiator/WorkspaceIdNegotiatorInterface.php b/core/modules/workspaces/src/Negotiator/WorkspaceIdNegotiatorInterface.php
new file mode 100644
index 000000000000..1afe25be6001
--- /dev/null
+++ b/core/modules/workspaces/src/Negotiator/WorkspaceIdNegotiatorInterface.php
@@ -0,0 +1,23 @@
+<?php
+
+namespace Drupal\workspaces\Negotiator;
+
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ * Interface for workspace negotiators that return only the negotiated ID.
+ */
+interface WorkspaceIdNegotiatorInterface {
+
+ /**
+ * Performs workspace negotiation.
+ *
+ * @param \Symfony\Component\HttpFoundation\Request $request
+ * The HTTP request.
+ *
+ * @return string|null
+ * A valid workspace ID if the negotiation was successful, NULL otherwise.
+ */
+ public function getActiveWorkspaceId(Request $request): ?string;
+
+}
diff --git a/core/modules/workspaces/src/Negotiator/WorkspaceNegotiatorInterface.php b/core/modules/workspaces/src/Negotiator/WorkspaceNegotiatorInterface.php
index caf6c788fc2a..e35e4835e3c0 100644
--- a/core/modules/workspaces/src/Negotiator/WorkspaceNegotiatorInterface.php
+++ b/core/modules/workspaces/src/Negotiator/WorkspaceNegotiatorInterface.php
@@ -25,25 +25,10 @@ interface WorkspaceNegotiatorInterface {
public function applies(Request $request);
/**
- * Gets the negotiated workspace, if any.
- *
- * Note that it is the responsibility of each implementation to check whether
- * the negotiated workspace actually exists in the storage.
- *
- * @param \Symfony\Component\HttpFoundation\Request $request
- * The HTTP request.
- *
- * @return \Drupal\workspaces\WorkspaceInterface|null
- * The negotiated workspace or NULL if the negotiator could not determine a
- * valid workspace.
- */
- public function getActiveWorkspace(Request $request);
-
- /**
- * Sets the negotiated workspace.
+ * Notifies the negotiator that the workspace ID returned has been accepted.
*
* @param \Drupal\workspaces\WorkspaceInterface $workspace
- * The workspace entity.
+ * The negotiated workspace entity.
*/
public function setActiveWorkspace(WorkspaceInterface $workspace);
diff --git a/core/modules/workspaces/src/WorkspaceManager.php b/core/modules/workspaces/src/WorkspaceManager.php
index e94909b64ef5..d6f2e3327c47 100644
--- a/core/modules/workspaces/src/WorkspaceManager.php
+++ b/core/modules/workspaces/src/WorkspaceManager.php
@@ -42,21 +42,34 @@ public function hasActiveWorkspace() {
public function getActiveWorkspace() {
if (!isset($this->activeWorkspace)) {
$request = $this->requestStack->getCurrentRequest();
+
foreach ($this->negotiatorIds as $negotiator_id) {
+ /** @var \Drupal\workspaces\Negotiator\WorkspaceIdNegotiatorInterface $negotiator */
$negotiator = $this->classResolver->getInstanceFromDefinition($negotiator_id);
+
if ($negotiator->applies($request)) {
+ if ($workspace_id = $negotiator->getActiveWorkspaceId($request)) {
+ /** @var \Drupal\workspaces\WorkspaceInterface $negotiated_workspace */
+ $negotiated_workspace = $this->entityTypeManager
+ ->getStorage('workspace')
+ ->load($workspace_id);
+ }
+
// By default, 'view' access is checked when a workspace is activated,
// but it should also be checked when retrieving the currently active
// workspace.
- if (($negotiated_workspace = $negotiator->getActiveWorkspace($request)) && $negotiated_workspace->access('view')) {
+ if (isset($negotiated_workspace) && $negotiated_workspace->access('view')) {
+ // Notify the negotiator that its workspace has been selected.
+ $negotiator->setActiveWorkspace($negotiated_workspace);
+
$active_workspace = $negotiated_workspace;
break;
}
}
}
- // If no negotiator was able to determine the active workspace, default to
- // the live version of the site.
+ // If no negotiator was able to provide a valid workspace, default to the
+ // live version of the site.
$this->activeWorkspace = $active_workspace ?? FALSE;
}
diff --git a/core/modules/workspaces/tests/modules/workspace_update_test/src/Negotiator/TestWorkspaceNegotiator.php b/core/modules/workspaces/tests/modules/workspace_update_test/src/Negotiator/TestWorkspaceNegotiator.php
index 72a727bf5e06..83e8f8805071 100644
--- a/core/modules/workspaces/tests/modules/workspace_update_test/src/Negotiator/TestWorkspaceNegotiator.php
+++ b/core/modules/workspaces/tests/modules/workspace_update_test/src/Negotiator/TestWorkspaceNegotiator.php
@@ -3,6 +3,7 @@
namespace Drupal\workspace_update_test\Negotiator;
use Drupal\workspaces\Entity\Workspace;
+use Drupal\workspaces\Negotiator\WorkspaceIdNegotiatorInterface;
use Drupal\workspaces\Negotiator\WorkspaceNegotiatorInterface;
use Drupal\workspaces\WorkspaceInterface;
use Symfony\Component\HttpFoundation\Request;
@@ -10,7 +11,7 @@
/**
* Defines a workspace negotiator used for testing.
*/
-class TestWorkspaceNegotiator implements WorkspaceNegotiatorInterface {
+class TestWorkspaceNegotiator implements WorkspaceNegotiatorInterface, WorkspaceIdNegotiatorInterface {
/**
* {@inheritdoc}
@@ -19,11 +20,18 @@ public function applies(Request $request) {
return TRUE;
}
+ /**
+ * {@inheritdoc}
+ */
+ public function getActiveWorkspaceId(Request $request): ?string {
+ return 'test';
+ }
+
/**
* {@inheritdoc}
*/
public function getActiveWorkspace(Request $request) {
- return Workspace::create(['id' => 'test', 'label' => 'Test']);
+ return Workspace::load($this->getActiveWorkspaceId($request));
}
/**
diff --git a/core/modules/workspaces/tests/src/Functional/UpdateSystem/ActiveWorkspaceUpdateTest.php b/core/modules/workspaces/tests/src/Functional/UpdateSystem/ActiveWorkspaceUpdateTest.php
index 61c69112c3a2..17e6fa5af07d 100644
--- a/core/modules/workspaces/tests/src/Functional/UpdateSystem/ActiveWorkspaceUpdateTest.php
+++ b/core/modules/workspaces/tests/src/Functional/UpdateSystem/ActiveWorkspaceUpdateTest.php
@@ -7,6 +7,7 @@
use Drupal\Tests\BrowserTestBase;
use Drupal\Tests\UpdatePathTestTrait;
use Drupal\Tests\user\Traits\UserCreationTrait;
+use Drupal\workspaces\Entity\Workspace;
/**
* Tests that there is no active workspace during database updates.
@@ -45,6 +46,9 @@ protected function setUp(): void {
$index = array_search('workspace_update_test_post_update_check_active_workspace', $existing_updates);
unset($existing_updates[$index]);
\Drupal::keyValue('post_update')->set('existing_updates', $existing_updates);
+
+ // Create a valid workspace that can be used for testing.
+ Workspace::create(['id' => 'test', 'label' => 'Test'])->save();
}
/**
diff --git a/core/modules/workspaces/tests/src/Kernel/WorkspaceQueryParameterNegotiatorTest.php b/core/modules/workspaces/tests/src/Kernel/WorkspaceQueryParameterNegotiatorTest.php
new file mode 100644
index 000000000000..6f310832ed04
--- /dev/null
+++ b/core/modules/workspaces/tests/src/Kernel/WorkspaceQueryParameterNegotiatorTest.php
@@ -0,0 +1,123 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\workspaces\Kernel;
+
+use Drupal\Component\Utility\Crypt;
+use Drupal\KernelTests\KernelTestBase;
+use Drupal\Tests\user\Traits\UserCreationTrait;
+use Drupal\workspaces\Entity\Workspace;
+
+/**
+ * Tests the query parameter workspace negotiator.
+ *
+ * @coversDefaultClass \Drupal\workspaces\Negotiator\QueryParameterWorkspaceNegotiator
+ * @group workspaces
+ */
+class WorkspaceQueryParameterNegotiatorTest extends KernelTestBase {
+
+ use UserCreationTrait;
+
+ /**
+ * {@inheritdoc}
+ */
+ protected static $modules = [
+ 'user',
+ 'system',
+ 'workspaces',
+ ];
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp(): void {
+ parent::setUp();
+
+ $this->installEntitySchema('user');
+ $this->installEntitySchema('workspace');
+ $this->installSchema('workspaces', ['workspace_association']);
+
+ // Create a new workspace for testing.
+ Workspace::create(['id' => 'stage', 'label' => 'Stage'])->save();
+
+ $this->setCurrentUser($this->createUser(['administer workspaces']));
+
+ // Reset the internal state of the workspace manager so that checking for an
+ // active workspace in the test is not influenced by previous actions.
+ \Drupal::getContainer()->set('workspaces.manager', NULL);
+ }
+
+ /**
+ * @covers ::getActiveWorkspaceId
+ * @dataProvider providerTestWorkspaceQueryParameter
+ */
+ public function testWorkspaceQueryParameter(?string $workspace, ?string $token, ?string $negotiated_workspace, bool $has_active_workspace): void {
+ // We can't access the settings service in the data provider method, so we
+ // generate a good token here.
+ if ($token === 'good_token') {
+ $hash_salt = $this->container->get('settings')->get('hash_salt');
+ $token = substr(Crypt::hmacBase64($workspace, $hash_salt), 0, 8);
+ }
+
+ $request = \Drupal::request();
+ $request->query->set('workspace', $workspace);
+ $request->query->set('token', $token);
+
+ /** @var \Drupal\workspaces\Negotiator\QueryParameterWorkspaceNegotiator $negotiator */
+ $negotiator = $this->container->get('workspaces.negotiator.query_parameter');
+
+ $this->assertSame($negotiated_workspace, $negotiator->getActiveWorkspaceId($request));
+ $this->assertSame($has_active_workspace, \Drupal::service('workspaces.manager')->hasActiveWorkspace());
+ }
+
+ /**
+ * Data provider for testWorkspaceQueryParameter.
+ */
+ public static function providerTestWorkspaceQueryParameter(): array {
+ return [
+ 'no workspace, no token' => [
+ 'workspace' => NULL,
+ 'token' => NULL,
+ 'negotiated_workspace' => NULL,
+ 'has_active_workspace' => FALSE,
+ ],
+ 'fake workspace, no token' => [
+ 'workspace' => 'fake_id',
+ 'token' => NULL,
+ 'negotiated_workspace' => NULL,
+ 'has_active_workspace' => FALSE,
+ ],
+ 'fake workspace, fake token' => [
+ 'workspace' => 'fake_id',
+ 'token' => 'fake_token',
+ 'negotiated_workspace' => NULL,
+ 'has_active_workspace' => FALSE,
+ ],
+ 'good workspace, fake token' => [
+ 'workspace' => 'stage',
+ 'token' => 'fake_token',
+ 'negotiated_workspace' => NULL,
+ 'has_active_workspace' => FALSE,
+ ],
+ // The fake workspace will be accepted by the negotiator in this case, but
+ // the workspace manager will try to load and check access for it, and
+ // won't set it as the active workspace. Note that "fake" can also mean a
+ // workspace that existed at some point, then it was deleted and the user
+ // is just accessing a stale link.
+ 'fake workspace, good token' => [
+ 'workspace' => 'fake_id',
+ 'token' => 'good_token',
+ 'negotiated_workspace' => 'fake_id',
+ 'has_active_workspace' => FALSE,
+ ],
+ 'good workspace, good token' => [
+ 'workspace' => 'stage',
+ 'token' => 'good_token',
+ 'negotiated_workspace' => 'stage',
+ 'has_active_workspace' => TRUE,
+ ],
+ ];
+ }
+
+}
--
GitLab