Issue #3443761 by amateescu: Workspace negotiators shouldn't be responsible...

Issue #3443761 by amateescu: Workspace negotiators shouldn't be responsible for loading the negotiated workspace entity

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

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

core/modules/workspaces/src/Negotiator/WorkspaceIdNegotiatorInterface.php

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

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

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

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

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

core/modules/workspaces/tests/src/Kernel/WorkspaceQueryParameterNegotiatorTest.php

+<?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,
+      ],
+    ];
+  }
+
+}