From cd253e68606ab8b404ae807362cf1440c0f2e85b Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Tue, 26 Mar 2019 13:22:06 +0000 Subject: [PATCH] =?UTF-8?q?Issue=20#3016781=20by=20arpad.rozsa,=20Berdir,?= =?UTF-8?q?=20catch,=20Wim=20Leers,=20kristiaanvandeneynde:=20node=5Fnode?= =?UTF-8?q?=5Faccess()=20is=20too=20eager=20in=20adding=20the=20user=20cac?= =?UTF-8?q?he=20context=20=E2=80=94=20fixing=20this=20makes=20some=20pages?= =?UTF-8?q?=20faster?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (cherry picked from commit 039eae17199140420096da61a7e415d0cd21ef33) --- .../QuickEditIntegrationLoadingTest.php | 4 +- core/modules/node/node.module | 29 ++++++------- ...est.php => NodeAccessCacheabilityTest.php} | 43 ++++++++++++++++++- .../PageCacheTagsIntegrationTest.php | 5 +-- .../src/Kernel/Plugin/RowRenderCacheTest.php | 3 +- .../StandardJavascriptTest.php | 7 ++- 6 files changed, 63 insertions(+), 28 deletions(-) rename core/modules/node/tests/src/Functional/{NodeAccessAutoBubblingTest.php => NodeAccessCacheabilityTest.php} (56%) diff --git a/core/modules/editor/tests/src/Functional/QuickEditIntegrationLoadingTest.php b/core/modules/editor/tests/src/Functional/QuickEditIntegrationLoadingTest.php index 6f96971bb73..396c30a8a6a 100644 --- a/core/modules/editor/tests/src/Functional/QuickEditIntegrationLoadingTest.php +++ b/core/modules/editor/tests/src/Functional/QuickEditIntegrationLoadingTest.php @@ -103,11 +103,11 @@ class QuickEditIntegrationLoadingTest extends BrowserTestBase { $message = "The 'access in-place editing' permission is required."; } else { - $message = ''; + $message = "The 'edit any article content' permission is required."; } $body = Json::decode($response->getBody()); - $this->assertIdentical($message, $body['message']); + $this->assertSame($message, $body['message']); } } diff --git a/core/modules/node/node.module b/core/modules/node/node.module index aa242e6f0c4..af0c16606f2 100644 --- a/core/modules/node/node.module +++ b/core/modules/node/node.module @@ -974,33 +974,30 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo /** * Implements hook_node_access(). */ -function node_node_access(NodeInterface $node, $op, $account) { +function node_node_access(NodeInterface $node, $op, AccountInterface $account) { $type = $node->bundle(); + $access = AccessResult::neutral(); switch ($op) { case 'create': - return AccessResult::allowedIfHasPermission($account, 'create ' . $type . ' content'); + $access = AccessResult::allowedIfHasPermission($account, 'create ' . $type . ' content'); case 'update': - if ($account->hasPermission('edit any ' . $type . ' content')) { - return AccessResult::allowed()->cachePerPermissions(); - } - else { - return AccessResult::allowedIf($account->hasPermission('edit own ' . $type . ' content') && ($account->id() == $node->getOwnerId()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($node); + $access = AccessResult::allowedIfHasPermission($account, 'edit any ' . $type . ' content'); + if (!$access->isAllowed() && $account->hasPermission('edit own ' . $type . ' content')) { + $access = $access->orIf(AccessResult::allowedIf($account->id() == $node->getOwnerId())->cachePerUser()->addCacheableDependency($node)); } + break; case 'delete': - if ($account->hasPermission('delete any ' . $type . ' content')) { - return AccessResult::allowed()->cachePerPermissions(); + $access = AccessResult::allowedIfHasPermission($account, 'delete any ' . $type . ' content'); + if (!$access->isAllowed() && $account->hasPermission('delete own ' . $type . ' content')) { + $access = $access->orIf(AccessResult::allowedIf($account->id() == $node->getOwnerId()))->cachePerUser()->addCacheableDependency($node); } - else { - return AccessResult::allowedIf($account->hasPermission('delete own ' . $type . ' content') && ($account->id() == $node->getOwnerId()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($node); - } - - default: - // No opinion. - return AccessResult::neutral(); + break; } + + return $access; } /** diff --git a/core/modules/node/tests/src/Functional/NodeAccessAutoBubblingTest.php b/core/modules/node/tests/src/Functional/NodeAccessCacheabilityTest.php similarity index 56% rename from core/modules/node/tests/src/Functional/NodeAccessAutoBubblingTest.php rename to core/modules/node/tests/src/Functional/NodeAccessCacheabilityTest.php index 20154553fc1..84af1ff18a6 100644 --- a/core/modules/node/tests/src/Functional/NodeAccessAutoBubblingTest.php +++ b/core/modules/node/tests/src/Functional/NodeAccessCacheabilityTest.php @@ -12,7 +12,7 @@ use Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait; * @group Cache * @group cacheability_safeguards */ -class NodeAccessAutoBubblingTest extends NodeTestBase { +class NodeAccessCacheabilityTest extends NodeTestBase { use AssertPageCacheContextsAndTagsTrait; @@ -67,4 +67,45 @@ class NodeAccessAutoBubblingTest extends NodeTestBase { $this->assertNoCacheContext('user.node_grants:view'); } + /** + * Tests that the user cache contexts are correctly set. + */ + public function testNodeAccessCacheContext() { + // Create a user, with edit/delete own content permission. + $test_user1 = $this->drupalCreateUser([ + 'access content', + 'edit own page content', + 'delete own page content', + ]); + + $this->drupalLogin($test_user1); + + $node1 = $this->createNode(['type' => 'page']); + + // User should be able to edit/delete their own content. + // Therefore after the access check in node_node_access the user cache + // context should be added. + $this->drupalGet('node/' . $node1->id() . '/edit'); + $this->assertCacheContext('user'); + $this->drupalGet('node/' . $node1->id() . '/delete'); + $this->assertCacheContext('user'); + + // Create a user without edit/delete permission. + $test_user2 = $this->drupalCreateUser([ + 'access content', + ]); + + $this->drupalLogin($test_user2); + + $node2 = $this->createNode(['type' => 'page']); + + // The user shouldn't have access to the node edit/delete pages. + // Therefore after the access check in node_node_access the user permissions + // cache context should be added. + $this->drupalGet('node/' . $node2->id() . '/edit'); + $this->assertCacheContext('user.permissions'); + $this->drupalGet('node/' . $node2->id() . '/delete'); + $this->assertCacheContext('user.permissions'); + } + } diff --git a/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php b/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php index d94e94c2c70..40cefba9528 100644 --- a/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php @@ -71,7 +71,6 @@ class PageCacheTagsIntegrationTest extends BrowserTestBase { 'route', 'theme', 'timezone', - 'user', // The placed block is only visible on certain URLs through a visibility // condition. 'url.path', @@ -81,6 +80,8 @@ class PageCacheTagsIntegrationTest extends BrowserTestBase { // These two cache contexts are added by BigPipe. 'cookies:big_pipe_nojs', 'session.exists', + 'user.roles:anonymous', + 'user.roles:authenticated', ]; // Full node page 1. @@ -107,7 +108,6 @@ class PageCacheTagsIntegrationTest extends BrowserTestBase { 'config:block.block.bartik_page_title', 'node_view', 'node:' . $node_1->id(), - 'user:0', 'user:' . $author_1->id(), 'config:filter.format.basic_html', 'config:color.theme.bartik', @@ -164,7 +164,6 @@ class PageCacheTagsIntegrationTest extends BrowserTestBase { // FinishResponseSubscriber adds this cache tag to responses that have the // 'user.permissions' cache context for anonymous users. 'config:user.role.anonymous', - 'user:0', ]); } diff --git a/core/modules/views/tests/src/Kernel/Plugin/RowRenderCacheTest.php b/core/modules/views/tests/src/Kernel/Plugin/RowRenderCacheTest.php index 6f10ce5f105..a87ede5ffc3 100644 --- a/core/modules/views/tests/src/Kernel/Plugin/RowRenderCacheTest.php +++ b/core/modules/views/tests/src/Kernel/Plugin/RowRenderCacheTest.php @@ -188,11 +188,10 @@ class RowRenderCacheTest extends ViewsKernelTestBase { if ($check_cache) { $keys = $cache_plugin->getRowCacheKeys($view->result[$index]); - $user_context = !$account->hasPermission('edit any test content') ? 'user' : 'user.permissions'; $cache = [ '#cache' => [ 'keys' => $keys, - 'contexts' => ['languages:language_interface', 'theme', $user_context], + 'contexts' => ['languages:language_interface', 'theme', 'user.permissions'], ], ]; $element = $render_cache->get($cache); diff --git a/core/profiles/standard/tests/src/FunctionalJavascript/StandardJavascriptTest.php b/core/profiles/standard/tests/src/FunctionalJavascript/StandardJavascriptTest.php index 13973e0c5dc..7a9370a90dc 100644 --- a/core/profiles/standard/tests/src/FunctionalJavascript/StandardJavascriptTest.php +++ b/core/profiles/standard/tests/src/FunctionalJavascript/StandardJavascriptTest.php @@ -37,12 +37,11 @@ class StandardJavascriptTest extends WebDriverTestBase { $this->drupalGet(''); $this->assertBigPipePlaceholderReplacementCount(1); - // Node page: 3 placeholders: + // Node page: 2 placeholders: // 1. messages - // 2. local tasks block - // 3. comment form + // 2. comment form $this->drupalGet($node->toUrl()); - $this->assertBigPipePlaceholderReplacementCount(3); + $this->assertBigPipePlaceholderReplacementCount(2); } /**