From 3ce43cc2b110e110e4d77aff160c7ca012be92e6 Mon Sep 17 00:00:00 2001
From: Eric Smith <ericsmith@catalyst.net.nz>
Date: Tue, 4 Feb 2025 09:45:09 +1300
Subject: [PATCH 1/8] Issue #3504114: Add WIP test to show error
---
.../NodeAccessCacheRedirectWarning.php | 70 +++++++++++++++++++
1 file changed, 70 insertions(+)
create mode 100644 core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
diff --git a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
new file mode 100644
index 000000000000..461d96f2ba37
--- /dev/null
+++ b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
@@ -0,0 +1,70 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\node\Functional;
+
+use Drupal\Core\Cache\CacheableMetadata;
+
+/**
+ * Tests the node access grants cache context service.
+ *
+ * @group node
+ * @group Cache
+ */
+class NodeAccessCacheRedirectWarning extends NodeTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ protected static $modules = ['block', 'node_access_test_empty'];
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $defaultTheme = 'stark';
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp(): void {
+ parent::setUp();
+
+ node_access_rebuild();
+ }
+
+ /**
+ * Quick demonstration of the differences in cache contexts.
+ *
+ * The intent here was to visit the nodes to view the error but for whatever
+ * reason I can't seem to trigger the redirect warning this way. Needs work.
+ */
+ public function testNodeAccessCacheRedirectWarning(): void {
+ $this->drupalPlaceBlock('local_tasks_block');
+
+ $this->assertTrue(\Drupal::moduleHandler()->hasImplementations('node_grants'));
+
+ $author = $this->drupalCreateUser([
+ 'create page content',
+ 'edit own page content',
+ 'view own unpublished content',
+ ]);
+ $this->drupalLogin($author);
+
+ $node = $this->drupalCreateNode(['uid' => $author->id(), 'status' => 0]);
+
+ $access = $node->access('view', $author, TRUE);
+ $unpublished_metadata = CacheableMetadata::createFromObject($access);
+
+ // Reset the node access cache to check the published node.
+ \Drupal::entityTypeManager()->getAccessControlHandler('node')->resetCache();
+
+ $node->setPublished();
+ $node->save();
+ $access = $node->access('view', $author, TRUE);
+ $published_metadata = CacheableMetadata::createFromObject($access);
+
+ $this->assertSame($unpublished_metadata->getCacheContexts(), $published_metadata->getCacheContexts());
+ }
+
+}
--
GitLab
From 7c76520ea3264c996ab967687ae63892f26e25af Mon Sep 17 00:00:00 2001
From: Eric Smith <ericsmith@catalyst.net.nz>
Date: Thu, 6 Mar 2025 18:08:15 +1300
Subject: [PATCH 2/8] Issue #3504114: If edit own content is added, then the
cache context user is used, if edit any permissions then only user.permission
is added
---
.../NodeAccessCacheRedirectWarning.php | 22 ++++++++++---------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
index 461d96f2ba37..5438ed7680c5 100644
--- a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
+++ b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
@@ -4,8 +4,6 @@
namespace Drupal\Tests\node\Functional;
-use Drupal\Core\Cache\CacheableMetadata;
-
/**
* Tests the node access grants cache context service.
*
@@ -46,25 +44,29 @@ public function testNodeAccessCacheRedirectWarning(): void {
$author = $this->drupalCreateUser([
'create page content',
- 'edit own page content',
+ 'edit any page content',
'view own unpublished content',
]);
$this->drupalLogin($author);
$node = $this->drupalCreateNode(['uid' => $author->id(), 'status' => 0]);
- $access = $node->access('view', $author, TRUE);
- $unpublished_metadata = CacheableMetadata::createFromObject($access);
+ $this->drupalGet($node->toUrl());
+
+ $node->setPublished();
+ $node->save();
+
+ $this->drupalGet($node->toUrl());
+
+ $node->setUnpublished();
+ $node->save();
- // Reset the node access cache to check the published node.
- \Drupal::entityTypeManager()->getAccessControlHandler('node')->resetCache();
+ $this->drupalGet($node->toUrl());
$node->setPublished();
$node->save();
- $access = $node->access('view', $author, TRUE);
- $published_metadata = CacheableMetadata::createFromObject($access);
- $this->assertSame($unpublished_metadata->getCacheContexts(), $published_metadata->getCacheContexts());
+ $this->drupalGet($node->toUrl());
}
}
--
GitLab
From 9ad37e776a53bf5a44398941f231a21e189f86a3 Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 09:32:11 +0000
Subject: [PATCH 3/8] Rename test.
---
...directWarning.php => NodeAccessCacheRedirectWarningTest.php} | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
rename core/modules/node/tests/src/Functional/{NodeAccessCacheRedirectWarning.php => NodeAccessCacheRedirectWarningTest.php} (95%)
diff --git a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarningTest.php
similarity index 95%
rename from core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
rename to core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarningTest.php
index 5438ed7680c5..8946ca75fefe 100644
--- a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
+++ b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarningTest.php
@@ -10,7 +10,7 @@
* @group node
* @group Cache
*/
-class NodeAccessCacheRedirectWarning extends NodeTestBase {
+class NodeAccessCacheRedirectWarningTest extends NodeTestBase {
/**
* {@inheritdoc}
--
GitLab
From f9f1f2ba0f4b6534d80a7ce9c50dbb4fc5106846 Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 10:20:41 +0000
Subject: [PATCH 4/8] Add extra cache context.
---
core/modules/node/src/NodeGrantDatabaseStorage.php | 1 +
1 file changed, 1 insertion(+)
diff --git a/core/modules/node/src/NodeGrantDatabaseStorage.php b/core/modules/node/src/NodeGrantDatabaseStorage.php
index eea6cc100127..9a4c77d2d9ab 100644
--- a/core/modules/node/src/NodeGrantDatabaseStorage.php
+++ b/core/modules/node/src/NodeGrantDatabaseStorage.php
@@ -121,6 +121,7 @@ public function access(NodeInterface $node, $operation, AccountInterface $accoun
// know it for a fact.
$set_cacheability = function (AccessResult $access_result) use ($operation) {
$access_result->addCacheContexts(['user.node_grants:' . $operation]);
+ $access_result->addCacheContexts(['user.roles.authenticated']);
if ($operation !== 'view') {
$access_result->setCacheMaxAge(0);
}
--
GitLab
From 711552bd9e03eed2ddc041f484ffb57ded6e85ce Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 18:13:14 +0000
Subject: [PATCH 5/8] Move user.roles:authenticated cache context around.
---
core/modules/node/src/NodeAccessControlHandler.php | 9 ++++++++-
core/modules/node/src/NodeGrantDatabaseStorage.php | 1 -
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php
index 963ab53ded41..f4811bdb21dd 100644
--- a/core/modules/node/src/NodeAccessControlHandler.php
+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -207,6 +207,14 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
// we need to add the node as a cacheable dependency.
$cacheability->addCacheableDependency($node);
+ // Due to the check below, it is not possible to rely only on account
+ // permissions to determine whether the 'view own unpublished content'
+ // permission can be checked, instead we also need to check if the user has
+ // the authenticated role. Just in case anonymous and authenticated users
+ // are both granted the 'view own unpublished content' permission and also
+ // have otherwise identical permissions.
+ $cacheability->addCacheContext(['user.roles.authenticated']);
+
if ($node->isPublished()) {
return NULL;
}
@@ -216,7 +224,6 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
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()) {
diff --git a/core/modules/node/src/NodeGrantDatabaseStorage.php b/core/modules/node/src/NodeGrantDatabaseStorage.php
index 9a4c77d2d9ab..eea6cc100127 100644
--- a/core/modules/node/src/NodeGrantDatabaseStorage.php
+++ b/core/modules/node/src/NodeGrantDatabaseStorage.php
@@ -121,7 +121,6 @@ public function access(NodeInterface $node, $operation, AccountInterface $accoun
// know it for a fact.
$set_cacheability = function (AccessResult $access_result) use ($operation) {
$access_result->addCacheContexts(['user.node_grants:' . $operation]);
- $access_result->addCacheContexts(['user.roles.authenticated']);
if ($operation !== 'view') {
$access_result->setCacheMaxAge(0);
}
--
GitLab
From 025c6e98df936d134ec599b031133cfdbab9515b Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 20:21:50 +0000
Subject: [PATCH 6/8] Add the node grants cache context in exactly one
additional situation.
---
core/modules/node/src/NodeAccessControlHandler.php | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php
index f4811bdb21dd..2ee7d4965d20 100644
--- a/core/modules/node/src/NodeAccessControlHandler.php
+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -213,12 +213,12 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
// the authenticated role. Just in case anonymous and authenticated users
// are both granted the 'view own unpublished content' permission and also
// have otherwise identical permissions.
- $cacheability->addCacheContext(['user.roles.authenticated']);
if ($node->isPublished()) {
return NULL;
}
$cacheability->addCacheContexts(['user.permissions']);
+ $cacheability->addCacheContexts(['user.roles:authenticated']);
if (!$account->hasPermission('view own unpublished content')) {
return NULL;
@@ -230,7 +230,17 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
return NULL;
}
+ // When access is granted due to the 'view own unpublished content'
+ // permission and for no other reason, node grants are bypassed. However,
+ // to ensure the full set of cacheable metadata is available to variation
+ // cache, additionally add the node_grants cache context so that if the
+ // status or the owner of the node changes, cache redirects will continue to
+ // reflect the latest state without needing to be invalidated.
$cacheability->addCacheContexts(['user']);
+ if ($this->moduleHandler->hasImplementations('node_grants')) {
+ $cacheability->addCacheContexts(['user.node_grants:view']);
+ }
+
if ($account->id() != $node->getOwnerId()) {
return NULL;
}
--
GitLab
From e4be93b0d7a393b80abc3662dce11e93f97ada9b Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 20:57:58 +0000
Subject: [PATCH 7/8] Move things back where they were, but keep comment.
---
.../node/src/NodeAccessControlHandler.php | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php
index 2ee7d4965d20..743f614e61c8 100644
--- a/core/modules/node/src/NodeAccessControlHandler.php
+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -207,23 +207,23 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
// we need to add the node as a cacheable dependency.
$cacheability->addCacheableDependency($node);
- // Due to the check below, it is not possible to rely only on account
- // permissions to determine whether the 'view own unpublished content'
- // permission can be checked, instead we also need to check if the user has
- // the authenticated role. Just in case anonymous and authenticated users
- // are both granted the 'view own unpublished content' permission and also
- // have otherwise identical permissions.
-
if ($node->isPublished()) {
return NULL;
}
$cacheability->addCacheContexts(['user.permissions']);
- $cacheability->addCacheContexts(['user.roles:authenticated']);
if (!$account->hasPermission('view own unpublished content')) {
return NULL;
}
+ // Due to the check below, it is not possible to rely only on account
+ // permissions to determine whether the 'view own unpublished content'
+ // permission can be checked, instead we also need to check if the user has
+ // the authenticated role. Just in case anonymous and authenticated users
+ // are both granted the 'view own unpublished content' permission and also
+ // have otherwise identical permissions.
+ $cacheability->addCacheContexts(['user.roles:authenticated']);
+
// The "view own unpublished content" permission must not be granted
// to anonymous users for security reasons.
if (!$account->isAuthenticated()) {
--
GitLab
From 916ff04d83093ead8482698d60b77e788acba911 Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 21:09:45 +0000
Subject: [PATCH 8/8] Alternate approach - merge the access results.
---
core/modules/node/src/NodeAccessControlHandler.php | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php
index 743f614e61c8..11561df302fe 100644
--- a/core/modules/node/src/NodeAccessControlHandler.php
+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -131,12 +131,11 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
assert($node instanceof NodeInterface);
$cacheability = new CacheableMetadata();
+ $view_access_result = NULL;
+
/** @var \Drupal\node\NodeInterface $node */
if ($operation === 'view') {
- $result = $this->checkViewAccess($node, $account, $cacheability);
- if ($result !== NULL) {
- return $result;
- }
+ $view_access_result = $this->checkViewAccess($node, $account, $cacheability);
}
[$revision_permission_operation, $entity_operation] = static::REVISION_OPERATION_MAP[$operation] ?? [
@@ -185,6 +184,9 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
$access_result = $this->grantStorage->access($node, $operation, $account);
if ($access_result instanceof RefinableCacheableDependencyInterface) {
$access_result->addCacheableDependency($cacheability);
+ if ($view_access_result) {
+ $access_result->addCacheableDependency($view_access_result);
+ }
}
return $access_result;
}
--
GitLab