Issue #3278759 by mxr576, kristiaanvandeneynde, acbramley, danflanagan8,...

Issue #3278759 by mxr576, kristiaanvandeneynde, acbramley, danflanagan8, larowlan, bbrala: Access cacheability is not correct when "view own unpublished content" is in use

core/modules/jsonapi/tests/src/Functional/NodeTest.php

@@ -346,17 +346,13 @@ public function testGetIndividual(): void {
       ['4xx-response', 'http_response', 'node:1'],
       ['url.query_args', 'url.site', 'user.permissions'],
       'UNCACHEABLE (request policy)',
-      'MISS'
+      TRUE
     );
 
     // 200 after granting permission.
     $this->grantPermissionsToTestedRole(['view own unpublished content']);
     $response = $this->request('GET', $url, $request_options);
-    // The response varies by 'user', causing the 'user.permissions' cache
-    // context to be optimized away.
-    $expected_cache_contexts = Cache::mergeContexts($this->getExpectedCacheContexts(), ['user']);
-    $expected_cache_contexts = array_diff($expected_cache_contexts, ['user.permissions']);
-    $this->assertResourceResponse(200, FALSE, $response, $this->getExpectedCacheTags(), $expected_cache_contexts, 'UNCACHEABLE (request policy)', 'UNCACHEABLE (poor cacheability)');
+    $this->assertResourceResponse(200, FALSE, $response, $this->getExpectedCacheTags(), $this->getExpectedCacheContexts(), 'UNCACHEABLE (request policy)', TRUE);
   }
 
   /**
@@ -413,6 +409,29 @@ protected function assertNormalizedFieldsAreCached(array $field_names): void {
     });
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function getExpectedCacheContexts(?array $sparse_fieldset = NULL) {
+    // \Drupal\Tests\jsonapi\Functional\ResourceTestBase::testRevisions()
+    // loads different revisions via query parameters, we do our best
+    // here to react to those directly, or indirectly.
+    $cache_contexts = parent::getExpectedCacheContexts($sparse_fieldset);
+
+    // This is bubbled up by
+    // \Drupal\node\NodeAccessControlHandler::checkAccess() directly.
+    if ($this->entity->isPublished()) {
+      return $cache_contexts;
+    }
+    if (!\Drupal::currentUser()->isAuthenticated()) {
+      return Cache::mergeContexts($cache_contexts, ['user.roles:authenticated']);
+    }
+    if (\Drupal::currentUser()->hasPermission('view own unpublished content')) {
+      return Cache::mergeContexts($cache_contexts, ['user']);
+    }
+    return $cache_contexts;
+  }
+
   /**
    * {@inheritdoc}
    */

core/modules/node/src/NodeAccessControlHandler.php

@@ -3,6 +3,8 @@
 namespace Drupal\node;
 
 use Drupal\Core\Access\AccessResult;
+use Drupal\Core\Access\AccessResultInterface;
+use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Cache\RefinableCacheableDependencyInterface;
 use Drupal\Core\Entity\EntityHandlerInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
@@ -126,15 +128,15 @@ public function createAccess($entity_bundle = NULL, ?AccountInterface $account =
    * {@inheritdoc}
    */
   protected function checkAccess(EntityInterface $node, $operation, AccountInterface $account) {
-    /** @var \Drupal\node\NodeInterface $node */
-
-    // Fetch information from the node object if possible.
-    $status = $node->isPublished();
-    $uid = $node->getOwnerId();
+    assert($node instanceof NodeInterface);
+    $cacheability = new CacheableMetadata();
 
-    // Check if authors can view their own unpublished nodes.
-    if ($operation === 'view' && !$status && $account->hasPermission('view own unpublished content') && $account->isAuthenticated() && $account->id() == $uid) {
-      return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
+    /** @var \Drupal\node\NodeInterface $node */
+    if ($operation === 'view') {
+      $result = $this->checkViewAccess($node, $account, $cacheability);
+      if ($result !== NULL) {
+        return $result;
+      }
     }
 
     [$revision_permission_operation, $entity_operation] = static::REVISION_OPERATION_MAP[$operation] ?? [
@@ -144,25 +146,28 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
 
     // Revision operations.
     if ($revision_permission_operation) {
+      $cacheability->addCacheContexts(['user.permissions']);
       $bundle = $node->bundle();
+
       // If user doesn't have any of these then quit.
       if (!$account->hasPermission("$revision_permission_operation all revisions") && !$account->hasPermission("$revision_permission_operation $bundle revisions") && !$account->hasPermission('administer nodes')) {
-        return AccessResult::neutral()->cachePerPermissions();
+        return AccessResult::neutral()->addCacheableDependency($cacheability);
       }
 
       // If the user has the view all revisions permission and this is the view
       // all revisions operation then we can allow access.
       if ($operation === 'view all revisions') {
-        return AccessResult::allowed()->cachePerPermissions();
+        return AccessResult::allowed()->addCacheableDependency($cacheability);
       }
 
       // If this is the default revision, return access denied for revert or
       // delete operations.
+      $cacheability->addCacheableDependency($node);
       if ($node->isDefaultRevision() && ($operation === 'revert revision' || $operation === 'delete revision')) {
-        return AccessResult::forbidden()->addCacheableDependency($node);
+        return AccessResult::forbidden()->addCacheableDependency($cacheability);
       }
       elseif ($account->hasPermission('administer nodes')) {
-        return AccessResult::allowed()->cachePerPermissions();
+        return AccessResult::allowed()->addCacheableDependency($cacheability);
       }
 
       // First check the access to the default revision and finally, if the
@@ -173,22 +178,59 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
       if (!$node->isDefaultRevision()) {
         $access = $access->andIf($this->access($node, $entity_operation, $account, TRUE));
       }
-      return $access->cachePerPermissions()->addCacheableDependency($node);
+      return $access->addCacheableDependency($cacheability);
     }
 
     // Evaluate node grants.
     $access_result = $this->grantStorage->access($node, $operation, $account);
-    if ($operation === 'view' && $access_result instanceof RefinableCacheableDependencyInterface) {
-      // Node variations can affect the access to the node. For instance, the
-      // access result cache varies on the node's published status. Only the
-      // 'view' node grant can currently be cached. The 'update' and 'delete'
-      // grants are already marked as uncacheable in the node grant storage.
-      // @see \Drupal\node\NodeGrantDatabaseStorage::access()
-      $access_result->addCacheableDependency($node);
+    if ($access_result instanceof RefinableCacheableDependencyInterface) {
+      $access_result->addCacheableDependency($cacheability);
     }
     return $access_result;
   }
 
+  /**
+   * Performs view access checks.
+   *
+   * @param \Drupal\node\NodeInterface $node
+   *   The node for which to check access.
+   * @param \Drupal\Core\Session\AccountInterface $account
+   *   The user for which to check access.
+   * @param \Drupal\Core\Cache\CacheableMetadata $cacheability
+   *   Allows cacheability information bubble up from this method.
+   *
+   * @return \Drupal\Core\Access\AccessResultInterface|null
+   *   The calculated access result or null when no opinion.
+   */
+  protected function checkViewAccess(NodeInterface $node, AccountInterface $account, CacheableMetadata $cacheability): ?AccessResultInterface {
+    // If the node status changes, so does the outcome of the check below, so
+    // we need to add the node as a cacheable dependency.
+    $cacheability->addCacheableDependency($node);
+
+    if ($node->isPublished()) {
+      return NULL;
+    }
+    $cacheability->addCacheContexts(['user.permissions']);
+
+    if (!$account->hasPermission('view own unpublished content')) {
+      return NULL;
+    }
+
+    $cacheability->addCacheContexts(['user.roles:authenticated']);
+    // The "view own unpublished content" permission must not be granted
+    // to anonymous users for security reasons.
+    if (!$account->isAuthenticated()) {
+      return NULL;
+    }
+
+    $cacheability->addCacheContexts(['user']);
+    if ($account->id() != $node->getOwnerId()) {
+      return NULL;
+    }
+
+    return AccessResult::allowed()->addCacheableDependency($cacheability);
+  }
+
   /**
    * {@inheritdoc}
    */

core/modules/node/src/NodeGrantDatabaseStorage.php

@@ -66,15 +66,19 @@ public function access(NodeInterface $node, $operation, AccountInterface $accoun
 
     // If no module implements the hook or the node does not have an id there is
     // no point in querying the database for access grants.
-    if (!$this->moduleHandler->hasImplementations('node_grants') || !$node->id()) {
+    if (!$this->moduleHandler->hasImplementations('node_grants') || $node->isNew()) {
       // Return the equivalent of the default grant, defined by
       // self::writeDefault().
       if ($operation === 'view') {
-        return AccessResult::allowedIf($node->isPublished());
-      }
-      else {
-        return AccessResult::neutral();
+        $result = AccessResult::allowedIf($node->isPublished());
+        if (!$node->isNew()) {
+          $result->addCacheableDependency($node);
+        }
+
+        return $result;
       }
+
+      return AccessResult::neutral();
     }
 
     // Check the database for potential access grants.

core/modules/node/tests/node_access_test_auto_bubbling/node_access_test_auto_bubbling.routing.yml

@@ -4,3 +4,15 @@ node_access_test_auto_bubbling:
     _controller: '\Drupal\node_access_test_auto_bubbling\Controller\NodeAccessTestAutoBubblingController::latest'
   requirements:
     _access: 'TRUE'
+node_access_test_auto_bubbling.node_access:
+  path: '/node_access_test_auto_bubbling_node_access/{node}'
+  defaults:
+    _controller: '\Drupal\node_access_test_auto_bubbling\Controller\NodeAccessTestAutoBubblingController::nodeAccessCacheability'
+  requirements:
+    # Access checking intentionally happens in the controller instead of here.
+    _access: 'TRUE'
+    node: \d+
+  options:
+    parameters:
+      node:
+        type: entity:node

core/modules/node/tests/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php

@@ -4,6 +4,7 @@
 
 namespace Drupal\node_access_test_auto_bubbling\Controller;
 
+use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Controller\ControllerBase;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
 use Drupal\node\NodeInterface;
@@ -29,4 +30,28 @@ public function latest() {
     return ['#markup' => $this->t('The three latest nodes are: @nids.', ['@nids' => implode(', ', $nids)])];
   }
 
+  /**
+   * Exposes the node label when a user has access to view a node.
+   *
+   * @param \Drupal\node\NodeInterface $node
+   *   A node entity.
+   *
+   * @return array
+   *   A render array.
+   */
+  public function nodeAccessCacheability(NodeInterface $node) {
+    $build = [];
+    $cacheability = new CacheableMetadata();
+    $access = $node->access('view', return_as_object: TRUE);
+    $cacheability->addCacheableDependency($access);
+    if ($access->isAllowed()) {
+      $build[] = [
+        '#markup' => $node->label(),
+      ];
+      $cacheability->addCacheableDependency($node);
+    }
+    $cacheability->applyTo($build);
+    return $build;
+  }
+
 }

core/modules/node/tests/src/Functional/NodeAccessUnpublishedCacheabilityTest.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\node\Functional;
+
+use Drupal\node\NodeInterface;
+
+/**
+ * Tests cacheability on unpublished nodes inherited from node access.
+ *
+ * @group node
+ * @group Cache
+ */
+class NodeAccessUnpublishedCacheabilityTest extends NodeTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = [
+    'node_access_test_auto_bubbling',
+  ];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
+  /**
+   * Tests correct cacheability information bubbles up from node access.
+   */
+  public function testNodeAccessCacheabilityBubbleUpOnUnpublishedContent(): void {
+    $rid = $this->drupalCreateRole([
+      'access content',
+      'view own unpublished content',
+    ]);
+    $test_user1 = $this->drupalCreateUser(values: ['roles' => [$rid]]);
+    $test_user2 = $this->drupalCreateUser(values: ['roles' => [$rid]]);
+
+    $unpublished_node_by_test_user1 = $this->createNode(['type' => 'page', 'uid' => $test_user1->id(), 'status' => NodeInterface::NOT_PUBLISHED]);
+
+    $this->drupalLogin($test_user2);
+    $this->drupalGet('node_access_test_auto_bubbling_node_access/' . $unpublished_node_by_test_user1->id());
+    $this->assertSession()->pageTextNotContains($unpublished_node_by_test_user1->label());
+
+    // The author of the unpublished node must have access.
+    $this->drupalLogin($test_user1);
+    $this->drupalGet('node_access_test_auto_bubbling_node_access/' . $unpublished_node_by_test_user1->id());
+    $this->assertSession()->pageTextContains($unpublished_node_by_test_user1->label());
+  }
+
+}

core/modules/system/tests/src/Functional/Menu/LinksetControllerTestBase.php

@@ -95,7 +95,7 @@ protected function doRequest(string $method, Url $url, $expected_status = 200, ?
    */
   protected function assertDrupalResponseCacheability($expect_cache, CacheableDependencyInterface $expected_metadata, Response $response) {
     $this->assertTrue(in_array($expect_cache, ['HIT', 'MISS', FALSE], TRUE), 'Cache is HIT, MISS, FALSE.');
-    $this->assertSame($expected_metadata->getCacheContexts(), explode(' ', $response->getHeaderLine('X-Drupal-Cache-Contexts')));
+    $this->assertSame(\Drupal::service('cache_contexts_manager')->optimizeTokens($expected_metadata->getCacheContexts()), explode(' ', $response->getHeaderLine('X-Drupal-Cache-Contexts')));
     $this->assertSame($expected_metadata->getCacheTags(), explode(' ', $response->getHeaderLine('X-Drupal-Cache-Tags')));
     $max_age_message = $expected_metadata->getCacheMaxAge();
     if ($max_age_message === 0) {