Automatically bubble the "user.node_grants:$op" cache context in node_query_node_access_alter()

Uploading the PoC I posted at #2556889-40: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation.

Makes sense!

And here's test coverage.

Green after re-testing, so PIFR & DrupalCI agree again.

I haven't reviewed the tests yet, but the node.module change looks great. Meanwhile, how's this for docs explaining the rationale for having this in 8.x, but removing in 9?

index 0000000..0e4fdfe
Binary files /dev/null and b/core/.DS_Store differ

Oh, Macs and your silly files. Removed.

+1, thanks for writing that comment!

1. +++ b/core/modules/node/node.module
   @@ -1064,6 +1064,21 @@ function node_query_node_access_alter(AlterableInterface $query) {
   +  $renderer = \Drupal::service('renderer');
   +  if ($renderer->hasRenderContext()) {
   +    $build = ['#cache' => ['contexts' => ['user.node_grants:' . $op]]];
   +    $renderer->render($build);
   +  }
   

   For all those automatic magic ... is there a way that we can toggle them off, so we can find problematic parts of our code easier?

2. +++ b/core/modules/node/tests/modules/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php
   @@ -0,0 +1,59 @@
   +/**
   + * Returns a node ID listing.
   + */
   

   missing proper doc

Re #13.1, we have:

1. 'user.permissions' added as a required cache context in core.services.yml.
2. MetadataBubblingUrlGenerator used as the url_generator implementation in core.services.yml.
3. This patch's addition to node_query_node_access_alter().
4. Anything else?

Should we move all of that to something like a cache_context_safeguards.module? And if so, should we do something to prevent it from being disabled without massive warnings? Or are you thinking of some other approach to toggling off?

Wow, #14 is a very interesting idea! We could even make that module hidden.

Here's a reroll to address #13.2.

I think addressing #13.1 will need broader changes. I like the idea in #14. But to do that for this particular patch (which changes node_query_node_access_alter()), I think that's going to be difficult. Fortunately, we can apply the same tactic as in other places: add a decorating service that does the cacheability bubbling.

Given those changes that I made in this reroll, I think the answer to #13.1 then becomes "service aliases", and we can do that in a separate issue, just like #14 proposes.

Here's a PoC patch implementing #14. Incomplete, but shows that moving the URL + node access bubbling can indeed be moved to a separate module that can be disabled.

Wim--

Again, Wim--

I'm making mistake after mistake. Clearly I can't multitask. Sorry for all the noise.

Per #2556889-65: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation, this should not handle MetadataBubblingUrlGenerator. #20 partially reverted that, so that's why #20 should also fail badly.

This makes the module required and adds an update hook, per #2556889-65: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation.

cache_context_safeguards implies it's only about cache contexts. But in case of #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated, we know that it's also about cache tags (the current user's cache tag). So, renaming to cacheability_safeguards.

Also moved the test coverage out of the node module and into this module.

The failures that started appearing in #23 are because they're kernel tests, which means they don't get the cacheability_safeguards module, which means they no longer get the user.permissions cache context (unless they'd install cacheability_safeguards).

I don't think the module should be required. It's fine that we want it to be un-disable-able in Standard profile - or even in Minimal, that's fine with me - however, Drupal as a framework should not be making the assumption that you need such a special-purpose module in every single application. Instead we can implement hook_system_info_alter() in Standard profile. In a perfect world we would have actual dependencies for installation profiles, but lacking that I think that would be the best option.

#29: it needs to be required, because otherwise it's too easy for custom distributions/install profiles to forget to add this module and hence not have these safeguards. That concern was explicitly raised by Berdir. See #2556889-65: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation.

This reroll allows sites to opt out by specifying a cacheability_safeguards: false container parameter. A "no safeguards" module that undoes everything doesn't work: what if something else already is adding the user.permissions cache context? We'd then be incorrectly removing it in the "no safeguards" module. Therefore, we must have some way to choose to not enable all these safeguards. An empty module to do so would be rather silly. Hence a container parameter seems the logical choice.

This concludes this PoC. It's a working PoC, just needs some polish and more tests. This is now blocked on reviews. This now blocks #2558599: Automatically assign user cache contexts/tags when using current_user service.

@Fabianx remarked in IRC it'd be better if those kernel tests simply used the required_cache_contexts specified in the container. Done.

+
+/**
+ * Install the cache_context_safeguards module.
+ */
+function system_update_8005() {
+  \Drupal::service('module_installer')->install(['cache_context_safeguards']);
+}

Module name needs to be updated.

Yes, and tests that would've caught that. Before continuing I want a +1 or -1 on this direction.

Re #30: Meh, yeah I guess that's valid. We really need inheritance for profiles and then this could be part of a base install profile....

Is an $op cache context sufficient? Doesn't the content vary by the grants for the current user (which is determined dynamically - ugh).

The context cache token will then be replaced with an actual value that represents the node access grants for that user. yes, it works.

The context cache token will then be replaced with an actual value that represents the node access grants for that user. yes, it works.

Indeed. And we have test coverage for that: \Drupal\node\Tests\NodeAccessGrantsCacheContextTest.

Before continuing I want a +1 or -1 on this direction.

I like the patch's use of a container parameter for toggling the safeguards. If we have that, then I don't think we need it to also be a separate module. Perhaps the provider that is being safeguarded can also provide the safeguard? I.e., if \Drupal\Core\Cache is the provider of 'user.permissions', perhaps it should also contain the classes needed to safeguard it, and same for \Drupal\node with respect to 'user.node_grants'? Although that would mean the safeguard implementations aren't all centralized in the same namespace, it would save on needing a hidden required module, and it would setup a pattern for contrib: e.g., a contrib LDAP integration module could provide both a cache context for some LDAP info and a safeguard for it.

I struggled for a while on whether to promote this issue to Critical, either as a blocker for #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) or a blocker for RC once that issue lands. But I don't think it should be either. I think it's a great security hardening, so tagging as such, and would be very valuable to land near the same time as SmartCache itself, so tagging as "sprint", but I also think it would be acceptable to commit after 8.0.0 (either in a minor or patch release).

If there are concerns about the security of Drupal without this patch, then I think those security concerns should already exist with HEAD, independent of SmartCache, because of node_access queries performed within non-controller code that is already being render cached in HEAD: blocks, hook_node_view(), Views plugins, etc.

If, however, we find real security vulnerabilities related to this either before or after committing SmartCache, then we might need to promote this to Critical.

Discussed yesterday with webchick and effulgentsia. The conclusion was that this should not be a separate (and hidden+required) module. The decorators should live in the modules providing this functionality, and should simply be possible to disable by setting a container cacheability_safeguards: true container parameter to false.

This patch does that.

This direction also means that AFAICT no additional test coverage is necessary, beyond the (already existing) tests that ensure that the automatic bubbling of the cache context for using the node access query alter.

EDIT: only after writing this, I saw #38, which says pretty much the same.

1. +++ b/core/core.services.yml
   @@ -9,11 +9,12 @@ parameters:
   +  cacheability_safeguards: true
   

   I think it would be better to have this fine-granular for each thing:

   e.g.

   cacheability_safeguards:
   node_grants: true
   user_permissions: true

   etc.

   On the other hand Symfony makes it hard to extend a parameter, so contrib/ could not use it ...

   So I guess this is fine :/

2. +++ b/core/modules/node/src/NodeServiceProvider.php
   @@ -0,0 +1,38 @@
   +    if ($container->hasParameter('cacheability_safeguards') && $container->getParameter('cacheability_safeguards') === FALSE) {
   +      return;
   +    }
   

   The non_bubbling version should always be available, so one can depend on it in an edge case.

   In that case it would just be an alias.

   $container->setAlias('node.grant_storage.non_bubbling', ...);

   ( Syntax not sure, but pseudo code. )

1. Exactly…

2. The non_bubbling version should always be available, so one can depend on it in an edge case.

   Good point! Let's hear if others agree with that. Will do that in my next reroll, but looking for more feedback in the mean time. (I will likely be able to reroll late tonight, i.e. within 3–5 hours.)

+1 to #42.2. I don't know what the best implementation is (e.g., what should be an alias of what, see point one below), but I agree that services that know what they're doing should be able to ask for the non_bubbling service as a dependency. For node grants, the use cases for that might be edge case, but as a general pattern, it's not so edge case. For example, in #2558599: Automatically assign user cache contexts/tags when using current_user service, any module that wants to provide a context for a specific field of the current user will need to have a way to get that value without triggering the autobubbling of the 'user' context, and doing that via a dependency on current_user.non_bubbling seems like the likely way to do that.

1. +++ b/core/modules/node/src/NodeServiceProvider.php
   @@ -0,0 +1,38 @@
   +    $container->setDefinition('node.grant_storage.non_bubbling', $container->getDefinition('node.grant_storage'))
   +      ->setPublic(FALSE);
   +    $container->register('node.grant_storage')
   +      ->setClass('\Drupal\node\CacheabilityBubblingNodeGrantStorage')
   

   A plus side of this approach (as opposed to defining node.grant_storage.non_bubbling in node.services.yml) is that existing modules that implement providers that alter node.grant_storage (e.g., with a MongoDb implementation) can continue to work without a BC break, and then NodeServiceProvider just comes in and takes the altered definition, renames it to node.grant_storage.non_bubbling, and then decorates it. I like this over breaking existing contrib modules that alter node.grant_storage. However, this seems like it would be sensitive on module list order. I.e., contrib modules that have service providers that run after NodeServiceProvider will then end up replacing the decorator, which is not what we want. I don't know if there's a way to solve this cleanly. If not, perhaps we'll be required to make the BC break of putting node.grant_storage.non_bubbling into node.services.yml and publishing a CR for contrib modules to alter that one instead of node.grant_storage?

2. +++ b/core/modules/node/tests/node_access_test_auto_bubbling/src/Controller/NodeAccessTestAutoBubblingController.php
   @@ -0,0 +1,62 @@
   +    return ['#markup' => $this->t('The three latest nodes are: !nids.', ['!nids' => implode(', ', $nids)])];
   

   s/!nids/@nids/. Also, should the test be expanded to test for this message (in particular, which nids)?

+++ b/core/core.services.yml
@@ -9,11 +9,12 @@ parameters:
+  cacheability_safeguards: true

Not sure about this name. It looks naked without a namespace. Perhaps renderer.cacheability_safeguards?

#44:

1. (e.g., with a MongoDb implementation) can continue to work without a BC break, and then NodeServiceProvider just comes in and takes the altered definition

   Exactly.

   However, this seems like it would be sensitive on module list order. I.e., contrib modules that have service providers that run after NodeServiceProvider will then end up replacing the decorator, which is not what we want. I don't know if there's a way to solve this cleanly.

   Ugh :( Excellent point!

   If not, perhaps we'll be required to make the BC break of putting node.grant_storage.non_bubbling into node.services.yml and publishing a CR for contrib modules to alter that one instead of node.grant_storage?

   I don't see an alternative. :/

2. No, we don't need test coverage for this. It's solely about the fact that an entity query using entity access was used.

#45: Works for me.

Keeping at NR particularly for #44.1. Will reroll tomorrow.

Addressed everything except the ordering problem pointed out by @effulgentsia in #44.1.

@dawehner mentioned http://symfony.com/doc/current/components/dependency_injection/advanced.... in IRC. Currently investigating that, it may address #44.1 :)

#48: Yes, indeed that is a great way to decorate services as you can always depend on the inner state. Should have thought about that.

This is a pure container builder thing, but defines this in a way nicer way.

So, sadly, Symfony has used a very strange interpretation of the word "decorator" (which has a very specific meaning in the CS world, and they assign a very different one). They're not actually decorating a service, but replacing a service while keeping an alias to the replaced service.

http://www.php-schulung.de/2014/replace-service-keep-reference/ explains it more accurately (and the URL already captures better what it does than http://symfony.com/doc/current/components/dependency_injection/advanced.... extremely misleadingly describes).

EDIT: I forgot to mention the worst part: when decorating a service, you have to give it a different name. So it's only for providing "extended versions of a service" (that's also what Symfony's test coverage indicates), which you must choose to explicitly access. Rather than actually decorating a service.

As always things are an ordering issue. These services would use the the storage mechanism anyway which is executed after the aliasing so no matter which module order we choose for the modifiers, the fact that we use

    $container->addCompilerPass(new ModifyServiceDefinitionsPass());

    $container->addCompilerPass(new BackendCompilerPass());

will result into issues.

+++ b/core/modules/node/src/CacheabilityBubblingNodeGrantStorage.php
@@ -0,0 +1,123 @@
+ * The renderer is not injected to avoid initializing the render and theme
+ * system for REST routes. Instead, this service is container-aware.
...
+class CacheabilityBubblingNodeGrantStorage implements NodeGrantDatabaseStorageInterface, ContainerAwareInterface {
...
+    /** @var \Drupal\Core\Render\RendererInterface $renderer */
+    $renderer = $this->container->get('renderer');

This makes me quite sad. We can decouple that by providing a renderer proxy and inject that manually.

I think the solution to #50 is quite simple. Just introduce our own compiler pass that runs very late.

That also makes it much easier for contrib to provide additional cacheability safeguards:

  node.grant_storage:
    class: Drupal\node\NodeGrantDatabaseStorage
    arguments: ['@database', '@module_handler', '@language_manager']
    tags:
      - { name: backend_overridable }
      - { name: cacheability_safeguard, class: 'Drupal\node\CacheabilityBubblingNodeGrantStorage' }

(See the last line there.)

Next: addressing #51.

The renderer.lazy thing in #51 can be done in a follow-up, it'd be useful in other places too, and doesn't need to block this critical.

This reroll just removes the container-awareness and injects the renderer. So @dawehner's concerns are addressed, it'll just make things a bit slower when node.grants_storage is used in e.g. a JSON response. But that was probably premature optimization on my behalf anyway.

RTBC, looks great to me now!

All my concerns are addressed.

@effulgentsia and I pair-programmed on an improved implementation, that does use Symfony's decorators. This improves clarity for people familiar with Symfony decorators and ensures that the service name swapping happens at the expected time, for example before ResolveReferencesToAliasesPass.

We also added unit test coverage for the compiler pass :)

Please also credit @effulgentsia.

Great work! Back to RTBC!

My credit box is already checked. Also adding credit to Fabianx and dawehner for their thorough reviews.

Since I pair programmed with Wim on the latest patch, assigning to @catch for final review/commit.

Added beta evaluation.

+++ b/core/lib/Drupal/Core/CoreServiceProvider.php
index ac9a468..c53ba11 100644
--- a/core/lib/Drupal/Core/DependencyInjection/Compiler/CacheabilitySafeguardsPass.php

--- a/core/lib/Drupal/Core/DependencyInjection/Compiler/CacheabilitySafeguardsPass.php
+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CacheabilitySafeguardsPass.php

Ah so the pass ensures that it will be late, nice!

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CacheabilitySafeguardsPass.php
   @@ -0,0 +1,54 @@
   + * Contains \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass.
   

   Wrong docs

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CacheabilitySafeguardsPass.php
   @@ -0,0 +1,54 @@
   +/**
   + * Decorates the 'cacheability_safeguard'-tagged services.
   + */
   +class CacheabilitySafeguardsPass implements CompilerPassInterface {
   

   We should document here why we use the pass ... because its later than a ServiceModifier ...

Why are language and theme not safeguards too?

Theme is a required cache context because the Renderer itself calls out to theme(). I think a follow-up to move it from a required cache context to something added by the renderer only where it calls theme() would be a reasonable thing to explore, but out of scope for this issue.

Interface language is a required cache context because a massive amount of core code calls t(). In theory, we could decorate TranslationManager, but that would add several stack calls to every invocation of t() for little gain: we'd still end up varying every rendered cache item by language. And even if we did do that, we'd want it always in effect, not conditional on the cacheability_safeguards parameter, for the same reason as #22 gives with respect to url().

Decorate the non-bubbling service with a non-bubbling implementation

The 2nd "non-" should be removed.

So we keep the original name but append a hash. Is there any chance this makes errors less googleable etc?

Right, the way Symfony service decorators work, the original service ID is what ends up holding the decorator service. I.e., after the container is compiled, node.grant_storage contains the definition for the bubbling implementation. Unfortunately, Symfony requires an ID to be passed to $container->register() even when setDecoratedService() will be called on it, and therefore, that ID never used. If there's a container compilation error, I suppose that could lead to an error message exposing the temporary ID. Perhaps we should make it $id . '.decorator_alias_do_not_use.' . strtolower($random->name()) to help with googling?

#60: addressed both points.

#61:

1. Language and theme aren't security-sensitive, they (their contexts being absent when they should be present) would merely cause bugs, not information disclosure.
2. :)
3. No, because you never ever can or should use this service directly, and this just encourages that. Symfony's decorated services always ensure that the decorating service is marked private. Which means you can still use constructor/container injection. So, then you'd still be able to access it in theory. But we want to discourage that: we want you to use the original service ID (which transparently uses the bubbling or non-bubbling version depending on whether you have your guards down or up), or the non-bubbling service ID (which always is the original one). We don't ever want developers using the decorating/bubbling one specifically. The privateness plus the randomness ensure that even when they want to, they can't, so they can't shoot themselves in the foot.

Keeping RTBC because only docs changes.

Unfortunately, Symfony requires an ID to be passed to $container->register() even when setDecoratedService() will be called on it, and therefore, that ID never used.

To clarify, what I should have said is "that ID is never legitimately used." But, if we don't randomize it, but make it something predictable, such as node.grant_storage.bubbling, then people will be able to write *.services.yml files that have @node.grant_storage.bubbling as a dependency. But that dependency would fail when cacheability_safeguards is turned off. So randomizing the ID helps make it clear that it's not part of the API.

Haha, #62's reply to #61.3 was what I was writing first, and then changed, because I felt that was the less important rationale :) Now catch has both aspects! :) Also, #64++ for that additional clarification :)

The 2nd "non-" should be removed.

Good catch, fixed.

Perhaps we should make it $id . '.decorator_alias_do_not_use.' . strtolower($random->name()) to help with googling?

I don't think that's necessary. Because how could you ever see that service ID? It's all automated transformation with well-defined inputs. The only additional input, and hence also the only way to trigger an error, is if you specify a non-existing class name. I tried doing that, and this is what I got:

Fatal error: Class 'Drupal\node\CacheabilityBubblingNodeGrantStoragessss' not found in /Users/wim.leers/Work/drupal-tres/core/lib/Drupal/Component/DependencyInjection/Container.php on line 269

… exactly the same kind of error you get when you specify a non-existing class for any regular service definition in a *.services.yml file.

#64: To be precise that random name never occurs anywhere, it is just a dummy as can be seen here:

https://gist.github.com/LionsAd/a10b7d8bb17cebe35d92#file-container-deco...

Maybe we should move making user_permissions a safe guard guarded thing to a follow-up - or at least leave the parameter change - the code itself does not hurt.

Reasoning:

- Since adding that we do not know if core is still secure if you turned safe guards off, so that change is more invasive.

- Since adding that we do not know if core is still secure if you turned safe guards off, so that change is more invasive.

At first I disagreed, but then I saw what you meant: all of core now was written/updated/tested with the assumption that the permissions cache context is added automatically. But, the same is not true for the node access cache context. As this patch shows, no additional responses with test coverage were found to be missing the node access cache context.

That's why they're different.

So, yes, let's do that in a follow-up.

Maybe I missed this somewhere, but can someone explain why we'd ever want the safeguards off? I mean, this is the code that makes entity queries (or at least node queries) not be an information disclosure. Why in Druplicon's name would I ever want it off?

The only reason I even could turn it off is if I am manually adding the appropriate cache info myself... but I my node-futzing module has no idea if a node grants module is enabled. That's by design. And even then, how would I know to add the cache info if in "dev mode" or whatever it's being added for me, so I don't know I need to add it?

Either I'm completely missing something here, or everyone else is. I give it 50/50 odds. :-)

The only reason I even could turn it off is if I am manually adding the appropriate cache info myself... but I my node-futzing module has no idea if a node grants module is enabled. That's by design. And even then, how would I know to add the cache info if in "dev mode" or whatever it's being added for me, so I don't know I need to add it?

This is exactly why.

It doesn't matter that no node grants hook implementations exist on a certain site. As long as you're doing a node_access-tagged query, you must associate that cache context. The node access cache context ensures that in case no node grants hook implementations exist, it always returns the same value, because only a single variation exists. (Single variation, single return value ⇒ correct.) You can check the test coverage if you want :)

#68: I am pretty sure no sane person ever wants to turn off the safe guards in production (I would not want to do that) - except it is very useful for testing, because as dawehner stated earlier:

- Explicit is better than implicit

Yes, nagging the developer is the "holy grail" and tell you as developer exactly what you need to do, but that needs a (non-BC breaking, purely internal) refactor in Renderer first.

I think this is the aspect that catch felt was missing in the IS?

I also wrote a handbook page: https://www.drupal.org/developing/api/8/cache/safeguards

This looks great! I updated the IS to mention why MetadataBubblingUrlGenerator is not a safeguard.

+++ b/core/modules/node/src/CacheabilityBubblingNodeGrantStorage.php
@@ -0,0 +1,128 @@
+    if ($this->renderer->hasRenderContext()) {
+      $build = ['#cache' => ['contexts' => ['user.node_grants:' . $op]]];
+      $this->renderer->render($build);
+    }
+

Would it make sense to statically cache this, so bubbling is performed exactly once for each $op?

#74: No, it must not be statically cached, the reason being:

Consider two blocks which both show something with node_grants, but without adding the necessary context.

If Block A and B are not cached then and Block A is executed first then Block A gets the cache context added to their render array, while Block B does not => Information disclosure.

While currently no new RenderContext is opened, one can still semantically think of it like that:

- Whenever Renderer encounters a #cache element then (at least) it will be in a new context and hence all data that is added is part of the #cache entry there.

So the beauty of the Render Context and Render Stack system is that as long as #pre_render is used, things automatically end up within the right render element automatically, even if they are added out-of-bounds.

#74 is fully answered by #76 — thanks Fabianx :)

#75:

-when you write an entity query, it automatically handles the node access query tag unless you explicitly disable it, this is the opposite of using the database API directly

It took some searching to find where this is happening:

  /**
   * Whether access check is requested or not. Defaults to TRUE.
   *
   * @var bool
   */
  protected $accessCheck = TRUE;

…

    if ($this->accessCheck) {
      $this->sqlQuery->addTag($this->entityTypeId . '_access');
    }

- therefore it is not reasonable to expect people writing entity queries to think about node grants - unless they were explicitly trying to avoid them.

Agreed. But the current patch already only does things to queries that have the node_access tag. Therefore, when developers are using entity queries, they will get the cache context. And when they explicitly opt out, they will not get the cache context. So, this is already working :)

I just didn't realize that that tag was being added by default. So all that is necessary, is to expand the test coverage here to test the three possible cases:

1. default (accessCheck === TRUE => node_access tag added automatically)/li>
2. opt out (accessCheck === FALSE => node_access tag NOT added)/li>
3. manual (accessCheck === TRUE || FALSE, node_access tag added manually))/li>

- therefore we might want the grants cache context to be added by entity queries, whether or not safeguards are on or not.

I don't think this is a good idea. Because that means:

• injecting the Renderer into the entity query class
• embedding node-specific logic into the entity query class

Both of those seem like a terrible conflating of concerns. I think the above solution, where a query tag is added automatically, is already sufficient. The cache context concern should not be handled by the query logic.

- if we did that, can we additionally send some information via the entity query (and possibly Views), to disable the safeguard version from running? That gives us the opportunity to throw E_DEPRECATED in here one day.

If the previous doesn't follow, then neither does this.

@Fabianx:

Thanks for the explanation, makes totally sense :)

Wim: I'm still not following. What is the use case for disabling auto-tagging? What's the User Story for that? That's what I still don't get.

While awaiting a reply from @catch in #77, here's a straight reroll of #67 against HEAD.

The failures in #80 were legitimate, caused by #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends.

I'll work on addressing #77 + #82 tomorrow.

Now really on this again.

First, a rebased patch.

#82:

We don't need to do either of those things, because we have hook_entity_query_alter().

By working on this, I sadly discovered that hook_entity_query_alter() is dead. There's only hook_query_TAG_alter(). Which seems to have been sufficient, since hook_entity_query_alter() has been dead for three years without anybody noticing. So, filed an issue to update the docs accordingly: #2611638: hook_entity_query_alter() is dead, remove it from entity.api.php.

So with @catch's great insight in #75, this becomes vastly simpler. In fact, this then doesn't need a cacheability safeguard. Instead, this then simply means that the necessary cache context for node access queries is added automatically.

Which means there is nothing to safeguard, because it is always safe.

Retitling accordingly.

There were also some hunks in there that made sense in earlier iterations of the patch, but now are out-of-scope, nice-to-have clean-ups. Removing those, which cuts the patch size in half and makes it much simpler to review.

Clean-up.

I think this is ready for final review.

#75 also says:

can we additionally send some information via the entity query (and possibly Views), to disable the safeguard version from running? That gives us the opportunity to throw E_DEPRECATED in here one day.

I don't see that implemented in the patch. Was there a decision somewhere that we don't want that?

@effulgentsia chatted with me about #92 yesterday, and I gave him the same reply as #93, so that's great :)

@catch and I agree on this being rc target, but let's update the issue summary to reflect #93.

IS updated.

Thanks. I reviewed the patch again and it looks great, so RTBC.

RTBC + 1