Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag

+++ b/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
@@ -237,6 +239,57 @@ function testPageCache() {
+    $config = $this->config('system.performance');
+    $config->set('cache.page.use_internal', 1);
+    $config->set('cache.page.max_age', 300);
+    $config->set('response.gzip', 1);
+    $config->save();

I think we should have helpers for those calls (enable / disable) in general in WebTestBase.

Why gzip here?

Can all be follow-up though.

--

RTBC, test changes look great!

#606840: Enable internal page cache by default is currently marked critical, so if this is a hard blocker, then it's critical as well.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -139,6 +139,22 @@ public function onRespond(FilterResponseEvent $event) {
+    // The 'user.permissions' cache context ensures that if the permissions for
+    // a role are modified, users aren't served stale render cache content. But,
+    // when entire responses are cached in reverse proxies, the value for the
+    // cache context is never calculated, causing the stale response to not be
+    // invalidated. Therefore, when varying by permissions and the current user
+    // is the anonymous user, also add the cache tag for the 'anonymous' role.
+    $cache_contexts = $response->headers->get('X-Drupal-Cache-Contexts');
+    if ($cache_contexts && in_array('user.permissions', explode(' ', $cache_contexts)) && \Drupal::currentUser()->isAnonymous()) {
+      $cache_tags = ['config:user.role.anonymous'];

Its a bit odd that we introduce a special case in the FinishResponseSubscriber which seems to be for me a pretty generic place. Don't we actually need an API to provide additional cache tags given you have some cache contexts?

I actually had the same thought. I would hate for contrib modules to have to write that sort of shenanigans a lot... seems like it's really easy to get that wrong.

Yes and No:

It _is_ a very special case, because we don't want external proxies / internal page cache to depend on the user.permissions cache context, but use the cache tags instead.

However:

a) This shows there is a bug with the PageCacheMiddleware not taking contexts into account from the header as context.permissions should work and the parent 'enable page cache' should not fail. Fixing that is the real fix for the 'enable page cache issue' (and this can be demoted to major again as it is an optimization).

b) This should be more an alter cache contexts alter event / alter hook.

What should be happening here instead is:

- Call alterCacheMetadata event with a BubbleableMetadata or CacheableDependenyInterface object (whatever we want).
- User module implements the event / hook (whatever is appropriate)
- If cache_context user.permissions is present AND user.isAnonymous(), add the cache tag and _remove_ the context (that is important to distinguish from authenticated user processing)

a) However is not easy to fix as we need all the #cache_redirect logic of the renderer for that. Opened #2466585: Decouple cache implementation from the renderer and expose as renderCache service for that.

Sorry for the ping-pong, I still think its better to support a first alteration of the cache contexts header, because even if the page cache supports cache contexts, it is important to have in the ideal case only the 'url' cache context at the top level to avoid expensive service invocations.

If we want we can include the API here - however I am also okay to put that to a follow-up.

I am totally fine with just moving the current logic to a different subscriber as written, however I would ask that we also remove the user.permissions cache context while adding the tag as it is no longer needed.

And back to RTBC any API to simplify that can be follow-up.

Fixed a couple of nitpicks in comments so I guess it's still RTBC.

+++ b/core/lib/Drupal/Core/EventSubscriber/AnonymousUserResponseSubscriber.php
@@ -0,0 +1,84 @@
+    if ($event->getRequestType() !== HttpKernelInterface::MASTER_REQUEST) {
+      return;
+    }

Quick tip: You can use $event->isMasterRequest()

Thank you for fixing that nitpick.

Looks right here. Committed/pushed to 8.0.x, thanks!

The system_test.permissions.yml added in this issue didn't do anything. #2517114: Remove needless, wrongly-placed system_test.permissions.yml file