Automatically assign user cache contexts/tags when using current_user service

#2 originates from #2556889-42: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation.

Fabianx already posted a review over at #2556889-53: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation, which I need to address.

At least some of the failures are because CacheabilityBubblingAccountProxy is missing use DependencySerializationTrait. Which made me realize that this change actually forces everything using the current_user service to initialize the render and theme system (because current_user -> renderer -> theme.manager).

So the proper solution is to not inject the renderer. Otherwise even REST responses may get the theme system initialized.

Stupid me, I'm of course still getting a service injected. So I still need DependencySerializationTrait.

1. Hah! Fixed.
2. We should! This is exactly what Fabian suggested in #53 at the other issue, but which I still needed to address. I first wanted to start reducing the number of failures. Done now.
3. Because otherwise there's a circular dependency. Except that that is no longer the case as of #6!

Now really on this again.

First, a rebased patch.

#13: done.

Apparently AccountInterface was changed in #2112679: getUsername() should return the username getDisplayName() for the formatted user name; it now has two additional required methods. Updated accordingly.

Fixing test failures caused by excessively added/bubbled cacheability metadata…

Sadly, to achieve that, we need to use the current_user.non_bubbling service in many places. For example, in all cache contexts and in all places where we do access checking.

It seems that as a consequence, we'll end up with a majority of services consuming current_user.non_bubbling instead of current_user. Which means developers looking at core have a good chance of copy/pasting something that will in fact still be insecure…

However, I can reduce the pain there by stopping to inject the current user into many services; many services don't actually need it. Doing that next.

Refactored a bunch of services to not pass the current user service to the access manager. That significantly cuts down on the problem described in #18.

Missed one spot.

Fixed the failures in CommentDefaultFormatterCacheTagsTest and ToolbarCacheContextsTest.

AFAICT getTimeZone() should not bubble the user cache context, but the timezone cache context.

While working on #22, I noticed that the comment form now has more cache tags (see the changes in CommentDefaultFormatterCacheTagsTest).

This is because filter_formats() (which is used to create the text format dropdown and the filter tips) uses the current user service to personalize it to the list of formats the current user can use. But this list is of course not specific to the user; it depends on the user's permissions. So it actually varies by user.permissions, not user. But, making this work correctly requires modifying filter_formats(), which 1) is a soft BC break at best, an actual BC break at worst, 2) is therefore in any case out of scope here.

So, opening a separate issue for that.

This patch also fixes CommentTranslationUITest's cacheability expectations.

Issue+patch created: #2611924: filter_formats() returns different value based on access, lacks cacheability metadata.

Note that with that patch, we could revert the changes here to CommentDefaultFormatterCacheTagsTest (in #22) and CommentTranslationUITest (in #23), because the cacheability metadata would become more accurate/more refined thanks to that.

One less fail and one less unnecessary @current_user injection.

+++ b/core/core.services.yml
@@ -804,7 +804,7 @@ services:
   router:
     class: Drupal\Core\Routing\AccessAwareRouter
-    arguments: ['@router.no_access_checks', '@access_manager', '@current_user']
+    arguments: ['@router.no_access_checks', '@access_manager', '@current_user.non_bubbling']

I noticed this is actually just like the three contextual link/local action/local task managers: having both the access manager and the current user services injected is pointless, since the access manager already has the current user service injected. So this one can be simplified also, which means again one less occurrence of @current_user.non_bubbling.

Again one less fail.

And another one. Turns out HEAD is injecting \Drupal::currentUser() into \Drupal::currentUser(), LOL.

The majority of the remaining failures are in REST tests. They're mainly caused by two bits of cacheability metadata being bubbled:

1. \Drupal\node\Entity\Node::getCurrentUserId(), which is called for the Node entity type's uid base field, to set a default value; the mere act of instantiating a Node object does not mean it will also be rendered, so this should use the non-bubbling service
2. AccountProxy::setAccount() (this is the class used for the current_user.non_bubbling service) does
   

   date_default_timezone_set(drupal_get_user_timezone());
   

   and drupal_get_user_timezone() calls \Drupal::currentUser()->getTimezone(). The latter then bubbles the timezone cache context (since #22). But the mere act of ensuring the right timezone for the duration of this request does not mean that any date or time will actually be rendered. In other words: it does not make sense for \Drupal\Core\Session\CacheabilityBubblingAccountProxy::getTimeZone() to do any bubbling at all.

This fixes 5 failures.

P.S.: debugging the second point has unveiled lots of bugs: we seem to call setAccount() twice whenever a user logs in, once with a User entity, once with a UserSession; in getAccount() when $this->account is not set and we default either to the initial account or the anonymous user session, we don't set the timezone; the AnonymousUserSession's timezone property is never set to a value, and so on. None of those seem to be causing any problems for now, but we should consider cleaning that up in future releases.

One less failure.

(This time: filter_formats() again, called by \Drupal\filter\Plugin\DataType\FilterFormat::getSettableOptions(), called by \Drupal\Core\Validation\Plugin\Validation\Constraint\AllowedValuesConstraintValidator(), which uses the bubbling current_user service, and should really use the non-bubbling one. Form validation doesn't need render bubbling.)

#41 caused us to go from 12 to 8 fails. But I said it fixed 5. It did. It also caused one regression, because one place (NodeBlockFunctionalTest) had been updated to expect the timezone cache context that no longer appears since #41.

This fixes that.

Together with #42, this should be down to 6 failures.

Just like #42 fixed one validator to use the non-bubbling current user service, so should all validators be updated.

That, together with two more "save entity"-related things (in User::postSave() and Comment::preSave()) and one completely different thing that's completely independent from rendering (user_logout()), this should bring the number of failures down to zero.

Next (and final) steps for the patch:

1. import the "cacheability safeguard" mechanism/architecture from #2557815-88: Automatically bubble the "user.node_grants:$op" cache context in node_query_node_access_alter()
2. tests that show it works as expected

Then it's purely down to reviews. And specifically, review whether this is worth doing, because I think it's clear this is impossible to make disruption-free. Also read #18.

It's described in #18:

However, I can reduce the pain there by stopping to inject the current user into many services; many services don't actually need it. Doing that next.

The access_manager already gets the current_user passed in. So, any service that calls the access_manager doesn't need to pass in the current_user: that's pointless by definition, because the access_manager already has it.

So I don't see at all what's confusing about those changes; in fact, I find it much cleaner. In HEAD, it's confusing why you need to pass the current_user again to the access_manager.

Did that make sense? If you disagree, can you explain why you find this less clear?

One unit test had to be updated. Actually green now.

Discussed with @dawehner in IRC. He felt very strongly that we should be explicit, and keep injecting the current_user service into the contextual link/local task/location action managers.

So, reverting the changes in #20, #21 and #34.

Side benefit: this hugely reduces the size of the patch :)

import the "cacheability safeguard" mechanism/architecture from #2557815-88: Automatically bubble the "user.node_grants:$op" cache context in node_query_node_access_alter()

I tried to do this, but failed miserably, because Symfony very much insists on making the inner/decorated service (current_user.non_bubbling in our case) private, and even trying tricks using aliases, I am not able to figure out how to convince Symfony to not do that.

Therefore, leaving the patch as-is for now.

Also, the most relevant snippet from an IRC conversation:

14:33:25 <catch> I really don't know about the patch in general.
14:33:51 <WimLeers> yeah me neither
14:34:26 <WimLeers> catch: dawehner So, one part that will *definitely* be affected in a negative manner are non-HTML routes
14:34:37 <WimLeers> Which you can tell by the number of test failures in REST routes
14:34:41 <WimLeers> which took the longest to debug
14:35:00 <WimLeers> Anything in contrib there using the current_user  service today will basically break
14:35:14 <WimLeers> because REST responses don't implement CacheableResponseInterface
14:35:28 <WimLeers> Which means EarlyRenderingControllerWrapperSubscriber will throw an exception for them
14:35:54 <WimLeers> We could in theory make EarlyRenderingControllerWrapperSubscriber put a *subclassed* RenderContext on there, and detect that in the bubbling current_user service, and if so, NOT bubble the cacheability metadata
14:36:02 <WimLeers> But then certain use cases *won't* be protected
14:36:09 <catch> WimLeers: tbh I think it is 8.1
14:36:18 <catch> And if we've had issues before then, then we can focus on it.
14:36:21 <WimLeers> i.e. any controller calling \Drupal::currentUser()
14:36:25 <catch> And if not, then leave it alone.
14:36:28 <WimLeers> okay
14:36:30 <WimLeers> thanks
14:36:33 <WimLeers> that's very helpful

#55 was around for >24 hours.

Moving to 8.1 per that IRC excerpt.

EntityAccessControlHandler's "current user" service is used by UserAccessControlHandler to compare the user ID (for "edit/view own"-style permissions). Which means that we simply must use the non-bubbling service in EntityAccessControlHandler. Which is the right thing to do anyway, because:

1. access checks may very well run independently of rendering, so it does not make sense to bubble cacheability metadata in that case
2. access checks are already expected to specify cacheability metadata explicitly

This should make the patch green again.

#77 AFAICT means that I should be able to revert all the getStaticCacheKey() stuff. Let's see if I'm right.

I'm afraid I've got lots of bad news, news that in my opinion shows that we really can't do this.

(And news that unfortunately demonstrates how unfortunate it is that we didn't have (Refinable)CacheableDependencyInterface and the cache tags/contexts/max-age concepts since the very beginning of the Drupal 8 cycle.)

I firmly believe that the getStaticCacheKey() stuff added by @effulgentsia in #57 and later is the wrong direction. We'd be adding a public method without an interface. A method that should not even be necessary. Just to mask the bubbling in situations where bubbling should not happen in the first place.

The clearest validation possible can be found right in the consequences (test failures) of #78: even if we remove the static caching from filter_formats(), then we still pass in the bubbling current_user service to the access() method. By chance, it so happens that its id() method is also called only for static caching. But if the specific entity access logic would call id(), then it would still bubble the cacheability metadata!

So, it is absolutely and solely thanks to chance that #77 is green: if FilterFormatAccessControlHandler called id(), then it would be red! So any contrib module adding more advanced entity access logic will then still cause the cacheability metadata to be bubbled (inappropriately so!), despite not doing anything wrong whatsoever!

Similarly, all the reverts @effulgentsia made since #57, including the ones to the constraint validators and the Comment class (and to a lesser extent those in the access_manager's injected service)… they all just happen to only cause test failures for REST tests. So, sure, our tests are green. But that doesn't mean it is correct.

This also explains why @effulgentsia's surgical change to EarlyRenderingControllerWrapperSubscriber works: because validation happens on POST requests… hence it's possible to revert the validation changes I made.

Another example, that shows an even more painful/deeper example: the Node class's uid base field, which has

->setDefaultValueCallback('Drupal\node\Entity\Node::getCurrentUserId')

The default value callback customizes a Node object to vary by the current user. And we already have the infrastructure to solve this: RefinableCacheableDependencyInterface! But we don't use it here — it didn't exist yet at the time when Field API was designed.

In other words: we cannot ever cache the "add node" form correctly, because we fail to see that the Node object it depends on actually varies by user.

In conclusion, Drupal 8 has gotten much better about being aware about its dependencies. Without knowing about dependencies, it is impossible to correctly cache/invalidate things.

But… it's nowhere near done yet. We see problems like this at all layers, in all APIs: from the "default value callback" for a field definition, to the current_user service being injected not allowing us to conclude that the thing it is injected to must also vary by user, because the current_user service also allows you to vary something just by permission, by roles, by authenticated/anonymous.

Far too many objects still conflate many concerns into a single thing.

Far too much logic still assumes it's okay to access global state to figure out the current user/permissions/roles/….

Consequently, the only way I see something like this patch being workable/understandable, is by doing something like my patch was doing: inject the non-bubbling service when in fact we don't want bubbling to happen at all! My patch avoids doing bubbling cacheability metadata onto a render context when the thing happening is completely unrelated to rendering. This patch does not. But that approach also is brittle/confusing. Hence #55+#56 concluding this should be moved to 8.1.

I updated all places that use current_user.non_bubbling to have an explanation as to why they need it. They further support my explanation/analysis above.

1. +++ b/core/core.services.yml
   @@ -1395,8 +1395,19 @@ services:
   +  # In most cases, use the 'current_user' service. The non_bubbling version
   +  # should only be used within code that must bypass the per-user cache
   +  # context getting added when a user-specific method is called. Such code
   +  # should be rare, and commented with an explanation for why the bypass is
   +  # necessary and safe.
   

   This is a bit dishonest; it doesn't mention at all this bubbling is specifically for rendering.

2. +++ b/core/lib/Drupal/Core/Logger/LoggerChannelFactory.php
   @@ -43,7 +43,10 @@ public function get($channel) {
   +        // Loggers should be able to log the user who triggered a certain
   +        // condition during a cache miss, even for code execution that can
   +        // be cached across users, so use the non-bubbling service.
   

   This is a far too complex explanation IMO. Logs are not cached. There's simply no point in caring about cacheability metadata *at all*.

3. +++ b/core/modules/node/src/Entity/Node.php
   @@ -500,7 +500,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    // This method is only called for applying a default value to the uid
   +    // field. That is triggered even during requests that do not use the
   +    // field. Therefore use the non-bubbling service to prevent an unnecessary
   +    // cache context.
   +    // @todo Switch this to the regular 'current_user' service if/when default
   +    //   value callbacks are refactored to run lazily.
   

   This is only half the truth, see my comment for the full truth.