Drupal
/
drupal
mirror of https://git.drupalcode.org/project/drupal.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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

This reverts commit 7102b46b35.
merge-requests/10501/head
catch 2024-12-04 10:42:55 +00:00
parent d3892f34c8
commit 7479a46b71
7 changed files with 32 additions and 186 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
core/modules/jsonapi/tests/src/Functional/NodeTest.php
View File

@ -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}
*/

82
core/modules/node/src/NodeAccessControlHandler.php
View File

@ -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}
*/

14
core/modules/node/src/NodeGrantDatabaseStorage.php
View File

@ -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.

12
core/modules/node/tests/node_access_test_auto_bubbling/node_access_test_auto_bubbling.routing.yml
View File

@ -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

25
core/modules/node/tests/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php
View File

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

52
core/modules/node/tests/src/Functional/NodeAccessUnpublishedCacheabilityTest.php
View File

@ -1,52 +0,0 @@
<?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());
}
}

2
core/modules/system/tests/src/Functional/Menu/LinksetControllerTestBase.php
View File

@ -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) {