From 7102b46b35e94fd70976991c46f14a858ae7258c Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Mon, 2 Dec 2024 07:19:30 +1000 Subject: [PATCH] Issue #3278759 by mxr576, kristiaanvandeneynde, acbramley, danflanagan8, larowlan, bbrala: Access cacheability is not correct when "view own unpublished content" is in use --- .../jsonapi/tests/src/Functional/NodeTest.php | 31 +++++-- .../node/src/NodeAccessControlHandler.php | 82 ++++++++++++++----- .../node/src/NodeGrantDatabaseStorage.php | 14 ++-- ...node_access_test_auto_bubbling.routing.yml | 12 +++ .../NodeAccessTestAutoBubblingController.php | 25 ++++++ .../NodeAccessUnpublishedCacheabilityTest.php | 52 ++++++++++++ .../Menu/LinksetControllerTestBase.php | 2 +- 7 files changed, 186 insertions(+), 32 deletions(-) create mode 100644 core/modules/node/tests/src/Functional/NodeAccessUnpublishedCacheabilityTest.php diff --git a/core/modules/jsonapi/tests/src/Functional/NodeTest.php b/core/modules/jsonapi/tests/src/Functional/NodeTest.php index 9a1200f9161..a8d18a5ac04 100644 --- a/core/modules/jsonapi/tests/src/Functional/NodeTest.php +++ b/core/modules/jsonapi/tests/src/Functional/NodeTest.php @@ -346,17 +346,13 @@ class NodeTest extends ResourceTestBase { ['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 @@ 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 6d3bbbb55e3..13020cea47b 100644 --- a/core/modules/node/src/NodeAccessControlHandler.php +++ b/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 @@ 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 */ - - // 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); + 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 @@ 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()->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 @@ class NodeAccessControlHandler extends EntityAccessControlHandler implements Nod 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} */ diff --git a/core/modules/node/src/NodeGrantDatabaseStorage.php b/core/modules/node/src/NodeGrantDatabaseStorage.php index 1b6d2085191..eea6cc10012 100644 --- a/core/modules/node/src/NodeGrantDatabaseStorage.php +++ b/core/modules/node/src/NodeGrantDatabaseStorage.php @@ -66,15 +66,19 @@ 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->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. 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 34fd420b3f6..36a00034fa3 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,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 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 feb40c60e1f..6ed17c95ca3 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,6 +2,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; @@ -27,4 +28,28 @@ 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 new file mode 100644 index 00000000000..7874196660b --- /dev/null +++ b/core/modules/node/tests/src/Functional/NodeAccessUnpublishedCacheabilityTest.php @@ -0,0 +1,52 @@ +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 83279917652..1987ca1ecc8 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($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) {