Expose node grants as cache context

@catch pointed out that this has a lot in common with #2099137: Entity/field access and node grants not taken into account with core cache contexts.

node_access_view_all_nodes() already checks if any module implements node grants, so we might be able to skip that check?

Neutral on whether we define it dynamically or not, it's a no-op if there's no node grant modules which seems OK.

Where to apply it is trickier.

We don't have a good way to bubble up cache contexts from child elements yet and it's not really possible in the render system.

To generate the cache key, you need all the cache contexts, to get cache contexts from child elements, you'd need a full render array, which you don't have, because all the hard work is done in #pre_render callbacks.

Individual subsystems may be able to do this if they know the rules for child elements - for example when rendering an entity, we know the bundle and view mode. That allows us to get the display, then ask every formatter configured for cache contexts, without actually rendering the fields at all. An entity reference formatter or views_embed-style formatter could also do that for the referenced view or entity, so while it's not simple, can handle multiple levels of nesting that way.

However rendering entities inside views, views would need to figure out all the possible bundles x view mode combinations for a view and get cache contexts from them.

Any custom caching (and other contrib modules like panels) would need to be able to ask the view or the entity displays what their cache contexts are.

The other option is to give up trying to do that, and add hook_default_cache_contexts_alter() - then at least access modules to be safe they just implement that. If a site doesn't like it they can add a custom alter.

This is equally a problem for both this issue and the entity/field access ones.

Regardless of the above I think we should be setting this in code, not in the UI anywhere.

Bumping this to critical. The only place in core this would be an issue is in Views if it used native block caching.

However any contrib or custom module caching lists of entities is going to run into this, so we at least need to add the context, even if we defer implementation of it to the formatter/access issue.

The code in #3 looks good overall.

But catch already said everything I could've wanted to say. The problem is indeed where/how to set this. I suspect it might be wiser to postpone this on #2099137: Entity/field access and node grants not taken into account with core cache contexts? Though we could definitely already commit the cache context, and then #2099137 can solve it all in one go. That'd also make sure that we take node access-specific trickiness into account while doing that one.

catch, what do you think?

Yeah I think adding it here then applying it there is good.

Alright, thanks catch. Since Berdir is already in several other, much bigger patches, and it's already assigned to me. I'll take this on (in the morning) unless somebody beats me to it.

Marking NW to finish it; this is not blocked on review anymore.

Sorry I misread #6. I'd also be fine with postponing this on that issue - we know the cache context is pretty much fine here, so either way works.

Tests added. Code cleaned up.

Ideally this would be a KernelTestBase test, but I think that might be tricky. This is good enough for now.

1. +++ b/core/modules/node/src/Cache/NodeAccessViewGrantsCacheContext.php
   @@ -0,0 +1,76 @@
   +    if ($this->user->hasPermission('bypass node access')) {
   +      return 'bypass';
   +    }
   +    if (!count(\Drupal::moduleHandler()->getImplementations('node_grants'))) {
   +      return 'n/a';
   +    }
   +
   +    if (node_access_view_all_nodes($this->user)) {
   +      return 'all';
   +    }
   +
   

   Does it really make sense to have more cache entries (due to a different CID) even you basically just have the same level of access checking anyway? Note: node_access_view_all_nodes() already partially includes the logic of the module handler, so we would not need that. (so you also cannot longer complain about the missing injection)

2. +++ b/core/modules/node/src/Cache/NodeAccessViewGrantsCacheContext.php
   @@ -0,0 +1,76 @@
   +    foreach ($grants as $realm => $gids) {
   +      $grants_context_parts[] = $realm . ':' . implode(',', $gids);
   +    }
   +    return implode(';', $grants_context_parts);
   

   It is odd that we don't have some generic API function which given an array it converts it into a deterministic string. I have seen similar things in MenuTreeStorage ... there we use UrlHelper::buildQuery() instead

The tests itself are fine, ... depending on the first comment for sure

1. Oh, you're right! We can merge these:
   

   +    if (!count(\Drupal::moduleHandler()->getImplementations('node_grants'))) {
   +      return 'n/a';
   +    }
   +
   +    if (node_access_view_all_nodes($this->user)) {
   +      return 'all';
   +    }
   

   Great catch :)

2. I guess, but I suspect they're all just slightly different, and it's simple enough to just write it wherever you need it?

... depending on the first comment for sure

What do you mean by this? :)

Well ... the tests have to be changed in case we merge together these special cases.

Oh, yes, of course. I thought you meant something else.

+++ b/core/modules/node/node.services.yml
@@ -39,3 +39,8 @@ services:
+      - { name: cache.context}

nitpick alarm: just in case someone wants to fix it, put a space before the }

Easy enough

This patch looks good to me, but should we also change NodeGrantDatabaseStorage::access() to use it as part of this issue, because currently, it says, // Node grants currently don't have any cacheability metadata., which won't be true once this patch is in. I think the points in #4 are all valid, but I don't think that invalidates using it in that access() method, but rather are issues that affect the callers of that method, which for menu links will be covered in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees and for entity display will be covered in #2099137: Entity/field access and node grants not taken into account with core cache contexts.

[…] because currently, it says, // Node grants currently don't have any cacheability metadata., which won't be true once this patch is in […]

AWESOME point! Exciting! :) Sadly only for the 'view' operation though. But that still helps the 99% use case. Consequently, the code still looks kinda kludgy, but it's definitely a step forward :)

That was stupid of me.

It helps when I try to run a test locally. (Sadly, no PHPUnit tests for this, otherwise I'd have found it.)

A bug in testbot, because the only exception is due to this:
rename(/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/588671/files/php/service_container/service_container_prod/d90f89e38b76ef8c46eb4b1e7f0ceed1e7e9a2b9a3c93adba930a24b6aabd1cd.php,/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/588671/files/php/service_container/.AkLCaS): No such file or directory

Re-testing.

Now it's green again.

Just a nitpick ...

+++ b/core/modules/node/src/Cache/NodeAccessViewGrantsCacheContext.php
@@ -0,0 +1,72 @@
+ *
+ * node_query_node_access_alter().

Is this considereed to be an @see ? ... Maybe though this is also a valid english sentence, you never know :P

:P No idea what happened there. Fixed :)

+++ b/core/modules/node/src/NodeGrantDatabaseStorage.php
@@ -88,16 +88,26 @@ public function access(NodeInterface $node, $operation, $langcode, AccountInterf
+    $set_cacheability = function (AccessResult $access_result) use ($operation) {
+      if ($operation === 'view') {
+        return $access_result->addCacheContexts(['cache_context.node_view_grants']);
+      }
+      else {
+        return $access_result->setCacheable(FALSE);
+      }
+    };
+
     if ($query->execute()->fetchField()) {
-      return AccessResult::allowed()->setCacheable(FALSE);
+      return $set_cacheability(AccessResult::allowed());
     }
     else {
-      return AccessResult::forbidden()->setCacheable(FALSE);
+      return $set_cacheability(AccessResult::forbidden());
     }

I'm not clear on why we need to do this with an anonymous function rather than changing the final if/else to set a local $access_result variable and then inline the body of that anonymous function directly after that, but AFAIK, we don't have Drupal coding standards that discourage anonymous functions, so then it's a stylistic choice, and not one worth holding up a critical issue for.

Therefore, RTBC.

+++ b/core/modules/node/src/Cache/NodeAccessViewGrantsCacheContext.php
@@ -0,0 +1,72 @@
+    // current user.
+    if ($this->user->hasPermission('bypass node access')) {
+      return 'bypass';
+    }
+    if (node_access_view_all_nodes($this->user)) {
+      return 'all';
+    }
+

Still a bit confused why 'bypass' and 'all' are different. If there's a good reason this could use inline docs. Especially confusing given the comment above suggests these are the same.

They are different to simplify debugging. Only users with the right permission get the 'bypass' 'shortcut'. After that, if there are no hook_node_grants() implementations or if there is a global grant, the 'all' 'shortcut' applies.
This is very clear in the test coverage, where UID 1 always gets 'bypass', other users get either 'all' if applicable, or otherwise the "full" value.

BUT! Having written the above, I realized that this indeed makes no sense, because it effectively causes multiple cache items to be created, and hence a lower cache hit ratio.

Given that this is such a simple change, keeping at RTBC.

Part of me wonders why we don't just check 'bypass node access' in node_access_view_all_nodes(), but that's out of scope here.

Looks much better and yes no reason to pollute cache due to debugging. Committed/pushed to 8.0.x, thanks!

The patch is great, how does it work, when a node is rendered in a block?

How will the block know that cache context?

Or is that access check something that is always applied?

@Fabianx none of that works yet, we left that for #2099137: Entity/field access and node grants not taken into account with core cache contexts to solve.

Yes, this is a pure addition, to make available this cache context; it's not used anywhere yet. You could consider this a child issue of the issue catch linked to.

The part of #34 not covered by #35 is the specific question about blocks. The answer depends on how the block renders the node. For example, for Views, we have #2318377: Determine whether a view is cacheable and its required contexts, store this i/t config entity and a possible follow-up to that that will be needed to ensure that Views blocks use that information. But another possibility is a entity reference field in a custom block, for which I just now opened #2396333: BlockContentBlock ignores cache contexts required by the block_content entity.

Also, in the meantime, until #35 and #37 are fixed, as a site builder you can manually configure the block to use the new cache context. See attached screenshot.