Make 'user.permissions' a required cache context

+++ b/core/modules/comment/src/Tests/CommentCacheTagsTest.php
@@ -86,11 +86,7 @@ protected function createEntity() {
-    return [
-      // Field access for the user picture rendered as part of the node that
-      // this comment is created on.
-      'user.permissions',
-    ];
+    return [];

We should keep that still I think.

We discussed that a site builder could possibly remove user.permissions, this makes that impossible.

So hmm, not sure, but I think core should treat itself as if that cache context was not present by default ...

+++ b/core/core.services.yml
@@ -2,7 +2,7 @@ parameters:
-    required_cache_contexts: ['languages:language_interface', 'theme']
+    required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']

Should we document it here, why we are doing it?

I still like the hardened security of the user.permissions, but we also see some issues with that obviously, e.g. contrib could forget to add it (as its already there), which pretty much makes ripping it out more difficult.

As Daniels concerns have been mainly about caching controllers (where you least expect it), I think that is a good compromise.

We could still allow this as an optional security hardening, but that can be decided on a site-by-site basis and maybe we need to make it easy to enable.

It is pretty clear we need to run tests without it being a required cache context - as else we don't find missing cache contexts.

@effulgentsia made an interesting remark: what if we don't make user.permissions a required cache context (which affects all render cache items and SmartCache items), but just a cache context that a required SmartCache cache context?

I think the problem not just exists for smart cache but also every other render cache entry. From block caching, over views row caching etc.

I still hope we start with thinking of security first in our mind

Agreed, adding user.permissions always seems like a good "security first, performance second" decision.

A while ago, we removed user.permission from rendered entities by default, making modules that add data responsible to add that. That resulted security issues in a few contrib modules that we're working on, for example simplenews and flag, which are adding links in hook_node/user_view() and similar places, based on permissions.

The majority of users on most sites will have a the same set of permissions, and the number of variations are usually not *that* big.

If not everywhere, then maybe at least in places that are very dynamic/commonly altered, like entities, blocks, views?

I think it makes sense to have user permissions as a required cache context.

I'm trying to figure out if there are good cases to allow people to opt-out. All I can think of is expensive database queries with no variation, or external http requests to web services. For web service requests you might have to limit how many calls are done - and four roles could mean four times as many cache misses (i.e. even if there are 20,000 cache hits, there might be 400 cache misses instead of 100).

However for cases like that, I think it's OK to say you have to cache the expensive/rate-limited stuff yourself, with whatever cache key is appropriate, then that cache item is good for however many different render misses.

The only other case would be a site where everything depending on permissions has been moved into post render cache / placeholders. Or a block that is cached per-page that has no variation. For these it would be nice to offer an opt-out, but I can't think of a way to do that except for an #exclude_cache_contexts key which is quite ugly.

So my biggest problem with the patch atm. is that like in #14 the user.permissions is not easily excludable again.

I think what would be nice is to have the default be different from the user.permissions 'real' one. So an alias so to speak.

user.permissions.secure_by_default?

Then if both user.permissions and user.permissions.secure_by_default are present it would be collapsed to user.permissions, but if user.permissions or user or whatever is not present, then it would just be user.permissions.secure_by_default be present and hence be secure.

That kind of distinction is important IMHO as we still want to ensure that things correctly declare what they vary on - despite it being a default context.

Yes, however it has several advantages:

- e.g. msonnabaum having 20 roles (with distinct permissions hashes) could for the secure_by_default cache context, decide to use a custom cache context service, which distinguishes just:

4 categories (super-admin, admin, end-user, anon)

instead of 20 using a custom site specific algorithm.

He could still use user.permissions to automatically placeholder e.g. and leave 20 partitions for the normal user.permissions.

--

So I think it still gives us the best out of both worlds.

I noticed that as user 1 looking at user/1 (although page doesn't matter), we do 5 cache redirects in HEAD:

This is because the block entity render cache misses the cache contexts added by the plugins or what they render:

entity_view:block:bartik_account_menu:en:menu_trail.account|:bartik
Array ( [#cache_redirect] => 1 [#cache] => Array ( [keys] => Array ( [0] => entity_view [1] => block [2] => bartik_account_menu ) [contexts] => Array ( [0] => languages:language_interface [1] => route.menu_active_trails:account [2] => theme [3] => user.permissions [4] => user.roles:authenticated ) [tags] => Array ( [0] => block_view [1] => config:block.block.bartik_account_menu [2] => config:system.menu.account ) [bin] => render ) )
entity_view:block:bartik_main_menu:en:menu_trail.main|:bartik
Array ( [#cache_redirect] => 1 [#cache] => Array ( [keys] => Array ( [0] => entity_view [1] => block [2] => bartik_main_menu ) [contexts] => Array ( [0] => languages:language_interface [1] => route.menu_active_trails:main [2] => theme [3] => user.permissions ) [tags] => Array ( [0] => block_view [1] => config:block.block.bartik_main_menu [2] => config:system.menu.main ) [bin] => render ) )
entity_view:block:bartik_tools:en:menu_trail.tools|:bartik
Array ( [#cache_redirect] => 1 [#cache] => Array ( [keys] => Array ( [0] => entity_view [1] => block [2] => bartik_tools ) [contexts] => Array ( [0] => languages:language_interface [1] => route.menu_active_trails:tools [2] => theme [3] => user.permissions ) [tags] => Array ( [0] => block_view [1] => config:block.block.bartik_tools [2] => config:system.menu.tools ) [bin] => render ) )
entity_view:block:bartik_footer:en:menu_trail.footer|:bartik
Array ( [#cache_redirect] => 1 [#cache] => Array ( [keys] => Array ( [0] => entity_view [1] => block [2] => bartik_footer ) [contexts] => Array ( [0] => languages:language_interface [1] => route.menu_active_trails:footer [2] => theme [3] => user.permissions ) [tags] => Array ( [0] => block_view [1] => config:block.block.bartik_footer [2] => config:system.menu.footer ) [bin] => render ) )

user.roles.authenticated cache context is due to the logout menu link I think - that's fine.

All the rest we'd save redirecting if we had user.permissions by default - which is 4 cache gets in HEAD.

So small performance improvement here too.

@catch: Yes. Similar, related note that we could optimize: At least one of the redirects I saw only had user.roles:anonymous in addition to user.permissions. At least in our implementation in core, we could optimize that away.

I think we found the reason for those test fails. The patch didn't have the change for default.services.yml which is used for new test environments.

This should fix most of the fails.

+1 to this issue. However:

We discussed that a site builder could possibly remove user.permissions, this makes that impossible.

Contrib could make it possible via the same kind of service decorator around hasPermission() as is being done in #2351015: URL generation does not bubble cache contexts around URL generation. But it can only do so if there's a service to decorate. Therefore: #2526514: Make User(Session)::hasPermission() a service.

This makes the new default cache tags/contexts a bit more flexible/intelligent. Adding user to ViewsUnitTestBase uncovered the broken create() method in the RolesRid plugin. Fixed that. We should consider enabling a lot more modules in PluginInstanceTest so we are testin more of those plugins, in a follow-up.

Wrong patch.

This should fix that last fail.

#22:

The making that pluggable move is great!

My concern is that people forget to use:

- $access->cachePerPermissions(), because it works anyway.

I guess that trade-off is okay and if someone has 16-20 roles they might have the development resources to either replace the PermissionsCacheContext with a simpler version (and hence also the permissions hash) or be able to audit the site for things wrong when that required context is removed.

Easy reroll, conflicted with #2516802: FilterProcessResult->merge() results in PHP warning: Missing argument 1 for FilterProcessResult::__construct() which also added user.permissions in FilterApiTest.

I am very happy about #31, but we should at least discuss the implications of #28 and #15 (even if the secure_by_default is not a good idea, so ignore that part).

For me, #22 is the answer to that. In other words, we make it ok for people to leave off the 'user.permissions' context in their render arrays / access results / etc., because if a contrib module wants to remove 'user.permissions' as a required context, it can decorate the service proposed in that other issue, such that any call to ->hasPermission() adds the context to the render stack, just like we're doing in core for URL generation.

RTBC + 1, I just wanted to have it discussed :).

It however means we need to be extra-careful in core, too ;) - though I guess that is acceptable.

Proposed commit message:

Issue #2493033 by Berdir, Wim Leers, Fabianx, dawehner, catch, effulgentsia, msonnabaum, Crell, webchick: Make 'user.permissions' a required cache context

Note:

msonnabaum, Crell, webchick have participated and contributed to that in the discussion around this that led to the decision to do this at DrupalCon LA.

Fixed failing tests

Interdiff looks good, so back to RTBC.

RTBC + 1 (again)

--

New proposed commit message:

Issue #2493033 by Berdir, Wim Leers, lauriii, Fabianx, effulgentsia, dawehner, catch, msonnabaum, Crell, webchick: Make 'user.permissions' a required cache context

msonnabaum, Crell, webchick have participated in the discussion at LA around this issue, where we already discussed IRL a lot of the pro's and con's.

+++ b/core/modules/block/src/Tests/BlockCacheTest.php
@@ -119,9 +119,11 @@ function testCachePerRole() {
+    // user.permissions is a required context, so a different user will see an
+    // always different block.

Should be a user with different permissions no?

+++ b/core/modules/block/src/Tests/BlockCacheTest.php
@@ -134,9 +136,12 @@ function testCacheGlobal() {
+    $this->assertText($current_content, 'Block content served from cache.');

Shouldn't this message be 'block not served from cache'?

I'd fix these on commit, but I want to double check I'm not reading the test change incorrectly here.

Committed/pushed to 8.0.x, thanks!