node_node_access() is too eager in adding the user cache context — fixing this makes some pages faster

Added a test which asserts the correct cache contexts and fixed the failing ones.

Re-uploading the test only patch, because it was corrupted.

https://www.drupal.org/project/drupal/issues/2883553
This issue was committed yesterday, so rerolling my patch to make it work with the latest changes. Otherwise nothing changed in the patch.

Made all the changes according to your feedback.

The issue summary

node_node_access() always adds the user cache context for the update/delete operations.

This is not true, because:

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

If you enter the first branch, it cannot possibly vary by the user cache context.

You're right though that in the second branch it always vary by user, even when the user doesn't have that permission! Great find! 👏 😀

Still, the claim in the very first sentence of the issue summary is incorrect. Explaining this more clearly will help this issue get committed much faster.

The patch

#2: There's so much broken about this function... Wow!

1. +++ b/core/modules/node/tests/src/Functional/NodeCacheContextsTest.php
   @@ -0,0 +1,54 @@
   + * Tests the Node entity's cache contexts.
   

   That's not what this is testing.

   We could fix this by just adding the sole test method of this class to \Drupal\Tests\node\Functional\NodeAccessAutoBubblingTest and perhaps renaming that test class, to NodeAccessCacheabilityTest.

2. +++ b/core/modules/node/node.module
   @@ -946,22 +946,22 @@ function node_node_access(NodeInterface $node, $op, $account) {
   +      elseif ($account->hasPermission('edit own ' . $type . ' content')) {
   +        return AccessResult::allowedIf($account->id() == $node->getOwnerId())->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
          }
   +      break;
   ...
   +  // No opinion.
   +  return AccessResult::neutral();
   

   This means that the user.permissions cache context will incorrectly be missing if we don't enter the elseif branch.

3. +++ b/core/modules/node/tests/src/Functional/NodeCacheContextsTest.php
   @@ -0,0 +1,54 @@
   +    $this->assertNoCacheContext('user');
   +    $this->drupalGet('node/' . $node2->id() . '/delete');
   +    $this->assertNoCacheContext('user');
   

   This should assert that the user.permissions cache context is present.

4. +++ b/core/profiles/standard/tests/src/FunctionalJavascript/StandardJavascriptTest.php
   @@ -37,12 +37,11 @@ public function testBigPipe() {
   -    // 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);
   

   NICE!!! This means fewer things to render on every page load, and therefore a slightly faster Drupal!

   It'd be nice if we'd have benchmarks for this and preferably even profiling, but that's not a requirement. It'd make for a cool release notes snippet :)

Great work! 🎉

Actually, for #9.3 / #12.4, I'm bumping this to Major 🤘

It's not a major improvement in and of itself … but it has a performance (and a small scalability) impact on just about every Drupal 8 site. Which is why it'd be great to benchmark it.

Agree with Wim on the user.permissions cache context needing to be set in case of 'update' or 'delete'.

So we can actually keep the default case returning a plain AccessResult::neutral(), but the delete and update case need to look like this:

if has any permission
  return access result cached by permissions
else if has own permission
  return access result cached by user (and permissions, but meh)
else
  return access result neutral cached by permissions

If we don't implement the fallback return value in the else statement and the first person to have their access checked happens to not have access, then we will cache a neutral result for everyone. So the tests definitely need to check this, as Wim said, to avoid regressions further down the line.

Also, I'd just change it to store the the access result in $access and then always call

+1

Moved the added test method into the suggested class and made sure that the user.permissions cache contexts are always set, even if we don't enter the elseif.

One very minor comment. Leaving RTBC (and also for Wim to have a last look).

+++ b/core/modules/node/node.module
@@ -933,33 +933,36 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
+      $access->addCacheContexts(['user.permissions']);

Do we explicitly need this given the allowedIfHasPermission() just above? I guess we do in case that code changes but maybe a comment for people copying the logic?

+++ b/core/modules/node/node.module
@@ -933,33 +933,36 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
-    default:
-      // No opinion.
-      return AccessResult::neutral();

This change reminds me of #3016038: Unrecognised entity operation passed to Menu Link Content throws exceptions.

dispatcher issues. returning to rtbc

@berdir OK that's a good point, but then why not use AccessResult::allowedIfHasPermission() in these cases too?

+++ b/core/modules/node/node.module
@@ -933,33 +933,36 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
     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();
+        $access = AccessResult::allowed();
       }
-      else {
-        return AccessResult::allowedIf($account->hasPermission('edit own ' . $type . ' content') && ($account->id() == $node->getOwnerId()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
+      elseif ($account->hasPermission('edit own ' . $type . ' content')) {
+        $access = AccessResult::allowedIf($account->id() == $node->getOwnerId())->cachePerUser()->addCacheableDependency($node);
       }

OK I'm not sure if I'm not explaining my idea well, or if I am and you just think it's a bad one. So here it is. Passes the new test.

This one should apply.

1. +++ b/core/modules/editor/tests/src/Functional/QuickEditIntegrationLoadingTest.php
   @@ -103,11 +103,11 @@ public function testUsersWithoutPermission() {
   -        $message = '';
   +        $message = "The 'edit any article content' permission is required.";
   

   🎉 — this is a nice side effect of using AccessResult and friends in the intended way :)

2. +++ b/core/modules/node/node.module
   @@ -974,33 +974,30 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
   +      $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;
   

   Excellent! Thanks @catch :) Like @Berdir, I like this too!

   Excellent pattern to follow elsewhere too.

3. +++ b/core/modules/node/tests/src/Functional/NodeAccessCacheabilityTest.php
   @@ -12,7 +12,7 @@
   -class NodeAccessAutoBubblingTest extends NodeTestBase {
   +class NodeAccessCacheabilityTest extends NodeTestBase {
   

   👍— much more accurate name indeed.

4. +++ b/core/modules/node/tests/src/Functional/NodeAccessCacheabilityTest.php
   @@ -67,4 +67,45 @@ public function testNodeAccessCacheabilitySafeguard() {
   +    // Create a user, with edit/delete own content permission.
   ...
   +    $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.
   ...
   +    $this->drupalGet('node/' . $node2->id() . '/edit');
   +    $this->assertCacheContext('user.permissions');
   +    $this->drupalGet('node/' . $node2->id() . '/delete');
   +    $this->assertCacheContext('user.permissions');
   

   👌

5. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php
   @@ -71,7 +71,6 @@ public function testPageCacheTags() {
   -      'user',
   
   @@ -81,6 +80,8 @@ public function testPageCacheTags() {
   +      'user.roles:anonymous',
   +      'user.roles:authenticated',
   
   @@ -107,7 +108,6 @@ public function testPageCacheTags() {
   -      'user:0',
   
   @@ -164,7 +164,6 @@ public function testPageCacheTags() {
   -      'user:0',
   
   +++ b/core/modules/views/tests/src/Kernel/Plugin/RowRenderCacheTest.php
   @@ -188,11 +188,10 @@ protected function doTestRenderedOutput(AccountInterface $account, $check_cache
   -        $user_context = !$account->hasPermission('edit any test content') ? 'user' : 'user.permissions';
   ...
   -            'contexts' => ['languages:language_interface', 'theme', $user_context],
   +            'contexts' => ['languages:language_interface', 'theme', 'user.permissions'],
   
   +++ b/core/profiles/standard/tests/src/FunctionalJavascript/StandardJavascriptTest.php
   @@ -37,12 +37,11 @@ public function testBigPipe() {
   -    // 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);
   

   🥳— this is showing all the big cacheability wins!

Committed and pushed 039eae1719 to 8.8.x and cd253e6860 to 8.7.x. Thanks!