diff --git a/core/modules/jsonapi/tests/src/Functional/NodeTest.php b/core/modules/jsonapi/tests/src/Functional/NodeTest.php index a8d18a5ac04..9a1200f9161 100644 --- a/core/modules/jsonapi/tests/src/Functional/NodeTest.php +++ b/core/modules/jsonapi/tests/src/Functional/NodeTest.php @@ -346,13 +346,17 @@ class NodeTest extends ResourceTestBase { ['4xx-response', 'http_response', 'node:1'], ['url.query_args', 'url.site', 'user.permissions'], 'UNCACHEABLE (request policy)', - TRUE + 'MISS' ); // 200 after granting permission. $this->grantPermissionsToTestedRole(['view own unpublished content']); $response = $this->request('GET', $url, $request_options); - $this->assertResourceResponse(200, FALSE, $response, $this->getExpectedCacheTags(), $this->getExpectedCacheContexts(), 'UNCACHEABLE (request policy)', TRUE); + // 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)'); } /** @@ -409,29 +413,6 @@ class NodeTest extends ResourceTestBase { }); } - /** - * {@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} */ diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php index 13020cea47b..6d3bbbb55e3 100644 --- a/core/modules/node/src/NodeAccessControlHandler.php +++ b/core/modules/node/src/NodeAccessControlHandler.php @@ -3,8 +3,6 @@ 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; @@ -128,15 +126,15 @@ class NodeAccessControlHandler extends EntityAccessControlHandler implements Nod * {@inheritdoc} */ protected function checkAccess(EntityInterface $node, $operation, AccountInterface $account) { - assert($node instanceof NodeInterface); - $cacheability = new CacheableMetadata(); - /** @var \Drupal\node\NodeInterface $node */ - if ($operation === 'view') { - $result = $this->checkViewAccess($node, $account, $cacheability); - if ($result !== NULL) { - return $result; - } + + // Fetch information from the node object if possible. + $status = $node->isPublished(); + $uid = $node->getOwnerId(); + + // 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); } [$revision_permission_operation, $entity_operation] = static::REVISION_OPERATION_MAP[$operation] ?? [ @@ -146,28 +144,25 @@ class NodeAccessControlHandler extends EntityAccessControlHandler implements Nod // 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()->addCacheableDependency($cacheability); + return AccessResult::neutral()->cachePerPermissions(); } // 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()->addCacheableDependency($cacheability); + return AccessResult::allowed()->cachePerPermissions(); } // 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($cacheability); + return AccessResult::forbidden()->addCacheableDependency($node); } elseif ($account->hasPermission('administer nodes')) { - return AccessResult::allowed()->addCacheableDependency($cacheability); + return AccessResult::allowed()->cachePerPermissions(); } // First check the access to the default revision and finally, if the @@ -178,59 +173,22 @@ class NodeAccessControlHandler extends EntityAccessControlHandler implements Nod if (!$node->isDefaultRevision()) { $access = $access->andIf($this->access($node, $entity_operation, $account, TRUE)); } - return $access->addCacheableDependency($cacheability); + return $access->cachePerPermissions()->addCacheableDependency($node); } // Evaluate node grants. $access_result = $this->grantStorage->access($node, $operation, $account); - if ($access_result instanceof RefinableCacheableDependencyInterface) { - $access_result->addCacheableDependency($cacheability); + 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); } 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} */ diff --git a/core/modules/node/src/NodeGrantDatabaseStorage.php b/core/modules/node/src/NodeGrantDatabaseStorage.php index eea6cc10012..1b6d2085191 100644 --- a/core/modules/node/src/NodeGrantDatabaseStorage.php +++ b/core/modules/node/src/NodeGrantDatabaseStorage.php @@ -66,19 +66,15 @@ class NodeGrantDatabaseStorage implements NodeGrantDatabaseStorageInterface { // 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->isNew()) { + if (!$this->moduleHandler->hasImplementations('node_grants') || !$node->id()) { // Return the equivalent of the default grant, defined by // self::writeDefault(). if ($operation === 'view') { - $result = AccessResult::allowedIf($node->isPublished()); - if (!$node->isNew()) { - $result->addCacheableDependency($node); - } - - return $result; + return AccessResult::allowedIf($node->isPublished()); + } + else { + return AccessResult::neutral(); } - - return AccessResult::neutral(); } // Check the database for potential access grants. diff --git a/core/modules/node/tests/node_access_test_auto_bubbling/node_access_test_auto_bubbling.routing.yml b/core/modules/node/tests/node_access_test_auto_bubbling/node_access_test_auto_bubbling.routing.yml index 36a00034fa3..34fd420b3f6 100644 --- a/core/modules/node/tests/node_access_test_auto_bubbling/node_access_test_auto_bubbling.routing.yml +++ b/core/modules/node/tests/node_access_test_auto_bubbling/node_access_test_auto_bubbling.routing.yml @@ -4,15 +4,3 @@ 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 diff --git a/core/modules/node/tests/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php b/core/modules/node/tests/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php index 6ed17c95ca3..feb40c60e1f 100644 --- a/core/modules/node/tests/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php +++ b/core/modules/node/tests/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php @@ -2,7 +2,6 @@ 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; @@ -28,28 +27,4 @@ class NodeAccessTestAutoBubblingController extends ControllerBase implements Con 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; - } - } diff --git a/core/modules/node/tests/src/Functional/NodeAccessUnpublishedCacheabilityTest.php b/core/modules/node/tests/src/Functional/NodeAccessUnpublishedCacheabilityTest.php deleted file mode 100644 index 7874196660b..00000000000 --- a/core/modules/node/tests/src/Functional/NodeAccessUnpublishedCacheabilityTest.php +++ /dev/null @@ -1,52 +0,0 @@ -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()); - } - -} diff --git a/core/modules/system/tests/src/Functional/Menu/LinksetControllerTestBase.php b/core/modules/system/tests/src/Functional/Menu/LinksetControllerTestBase.php index 1987ca1ecc8..83279917652 100644 --- a/core/modules/system/tests/src/Functional/Menu/LinksetControllerTestBase.php +++ b/core/modules/system/tests/src/Functional/Menu/LinksetControllerTestBase.php @@ -95,7 +95,7 @@ abstract class LinksetControllerTestBase extends BrowserTestBase { */ 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(\Drupal::service('cache_contexts_manager')->optimizeTokens($expected_metadata->getCacheContexts()), explode(' ', $response->getHeaderLine('X-Drupal-Cache-Contexts'))); + $this->assertSame($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) {