Node grant access check missing cacheable dependency on node

The formatter adds cacheability metadata to the render array. The access implementation is responsible for adding the entity as a cacheable dependency to the access result if changing it can result in a change of the access result.

Node is doing that, are you sure you really had this problem with nodes and not with another entity type?

I just recently wrote tests for paragraph (library) that relies on that working: #2943601: Make Paragraphs Library items publishable.

I also tested it manually and it works fine for me on a standard D8 site with a reference field on page.

But I just noticed a problem in \Drupal\node\NodeAccessControlHandler::checkAccess(), you can see that it adds the addCacheableDependency() call but only *if* it grants access.

If access isn't allowed, it continues to the grant storage, and then in \Drupal\node\NodeGrantDatabaseStorage::access() does it correctly:

return AccessResult::allowedIf($node->isPublished())->addCacheableDependency($node);

It always adds the cacheable dependency, but only if you are not using node grants. So my wild guess would be that you are using access grants?

So there is a problem, but it's a node specific problem of not always adding the cacheable dependency on the node. Not a problem with entity reference formatters or even the cache system. Whether or not the cache tag needs to be added depends on the access logic. E.g. for entity types without a status, saving them can't change whether you have access or not, so we don't want to add unnecessary cache tags.

Yes, I agree it should be fixed in the node module. I'd try to do it in NodeAccessControlHandler, possibly by merging multiple access results together, like I did in the referenced paragraphs issue.

I'll also ping Wim about this.

We have the same issue with view_unpublished enabled. It appears to be an issue when the hook 'hook_node_grants' is used. At that point the access function in NodeGrantDatabaseStorage doesn't add the node as cacheable dependency anymore.

I've used patch #11 and it appears to be adding the correct tags to the cache entries and it works!

Updating title and summary to better reflect what's going on here. I also came across this bug when debugging cacheability issues related to the access denied exception cacheability data missing the node:[nid] cache tag. This is the cause.

I don't understand the comment in #13, but this patch shouldn't cause all your nodes to become uncacheable? The patch in #13 looks correct to me. So this just needs a regression test I think.

#11 looks functionally fine but Berdir's suggestions make sense. I'm not really clear _how_ he was suggesting it though. Maybe this?

Here's a test that acts also as interdiff.

Fix coding standards issue.

Note that node_test module is enabled in test because is implementing hook_node_grants().

I also noticed the node_test. Thanks for clearing that out @claudiu.cristea. However, I think we should have a comment in the $modules array since if someone comes across it and sees it as a needless dependency and removes it, the test will still be green without it but will be a false positive.

@idimopoulos that is a good remark. In fact I want that this test fails if someone is removing that module. Better that any explanation.

Improving the comment.

The test provides good cover and the fix is proved. Moving to RTBC.

+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -104,7 +105,11 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
 
     // Evaluate node grants.
-    return $this->grantStorage->access($node, $operation, $account);
+    $access_result = $this->grantStorage->access($node, $operation, $account);
+    if ($operation === 'view' && $access_result instanceof RefinableCacheableDependencyInterface) {
+      $access_result->addCacheableDependency($node);
+    }
+    return $access_result;
   }
 

This could use a comment to explain why we only add the cacheable dependency on the view operation and not the edit or delete operations.

Also moving the version back to 8.9.x since this could go into a patch release.

Documented.

Setting this as RTBC again then if everyone is ok.

Committed/pushed to 9.0.x and cherry-picked back to 8.9.x, thanks!