Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?")

This blocks #2428837: Adding/updating interface translations should invalidate page & render caches, which is major.

This also soft-blocks #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view.

I've been thinking some more about this. Also discussed briefly/superficially with @effulgentsia.

I'm now more convinced than before that this is necessary. It's necessary for every cache context that itself depends on something configurable.

Clear examples:

1. user.roles: when the permissions for a role change (i.e. the role object is modified), then any output that varies by role should be invalidated
2. menu.active_trail: when a menu item is moved in the menu tree (i.e. the menu object is modified), then any output that varies by menu active trail should be invalidated
3. language (not including content language, language-type specific cache contexts will be available once #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles') lands): when a translation is modified, then any output that varies by that language should be invalidated
4. theme: when the settings of a theme change, then any output that varies by theme should be invalidated (this is effectively already the case because we decided in #2040135: Caches dependent on simple config are only invalidated on form submissions to make themes reuse the rendered cache tag)

But there are also clear examples where you might think cache tags should be associated, but that's not actually true:

1. user: when a user is modified, the variation doesn't actually change, since a user always remains the same user (the UID doesn't change), only properties of the user change, like the user name

And then the ones that are completely surpriseless are the ones that don't involve any calculations: e.g. url.

Initial patch, marked as do-not-test because this will cause lots of tests to fail, since this will cause more cache tags to be added automatically. We don't need a green patch, we need a discussion about this.

Oops.

What about reusing CacheDependencyInterface

Impossible, because:

• that interface is only meant for value objects, but we also have CalculatedCacheContextInterface, for which we need a parameter to know which cache tag(s) to return
• that interface also forces cache contexts to return a max-age (though I guess that could be useful for some cache contexts…)
• … and, hilariously it would also force cache contexts to return cache contexts :D

I don't follow how this is a counter example. […]

But when you're rendering something of the user, it's your responsibility to associate the corresponding cache tag.

Note that for all four clear examples I gave in #5, we're not associating cache tags in case the role label, menu name, language name or theme name is being displayed. We're associating the cache tag because we're showing things OTHER THAN role label/menu name/language name/theme name that depend on those objects. We're showing/hiding things based on a role, showing a different menu tree depending on the active menu trail, showing a different interface translation depending on the language (this one is the closest to your analogy) and showing a different logo depending on the theme settings.

As well as the role vs. user vs. permissions argument, language is also similar. We don't associate the language cache tag because the language is rendered, but because the translation is rendered.

True. But since #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles'), we have 'languages:' . LanguageInterface:TYPE_INTERFACE as a cache context. If we can associate a per-translation cache tag with that cache context. Together with this issue, we'd be able to fix that cleanly like that.
(See the related #2428837: Adding/updating interface translations should invalidate page & render caches.)

t() doesn't allow us to automatically assign a cache tag, but if it did we'd probably do it from there.

Interesting; this is related to something Fabianx was talking about WRT #2449459: [PP-1] Make URLs cache context aware: he thinks #2450993: Rendered Cache Metadata created during the main controller request gets lost could be a solution there. I think that if we'd solve that, then perhaps what you're suggesting (t() bubbling a cache context) would actually be possible.

If we look at contrib examples, if there's an 'organic group membership' cache context, do we add cache tags for each organic group with that or not?

When:

• rendering (something of) an OG (such as the name), we're rendering the thing itself, and thus we'd need the cache tag to be associated by whatever is rendering the OG
• rendering something that varies by OG (membership), such as only showing node X if the current user is in some OG, then we'd associate the cache context. This would automatically associate the cache tag too, in case it is possible to do something like "change OG membership conditions", which would affect which nodes are visible

I don't know enough about organic groups to know whether it makes sense to associate a cache tag with the corresponding cache context.

So agreed with the need for the actual tags, but not 100% on the API-level support.

I see that argument. But then I have to ask: What alternative do you see?. The above is the simplest possible thing that can work AFAICS.

I think you're saying we don't need this API; that it's always up to the developer to remember to associate both the context and necessary cache tags?

I think that for some of the most common use cases, that's feasible: "user has permission?" checking, for which the AccessResult class can automatically generate the necessary cache tags. #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()") will make it clean/simple to apply the cacheability of an access result to a render array. So that common use case could be taken care of.
That leaves 'languages:' . LanguageInterface::TYPE_INTERFACE as the biggest concern, since it's likely going to be used the widest. Especially when/if #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE lands in its current form (that issue will add the interface language cache context to every render array). The only alternative solution for that that I see is making t() bubble a cache tag (at which point it no longer makes sense to associate the interface language cache context by default, since using t() could automatically bubble it — that's be ideal, actually, if we can make it happen).

The consequences of choosing not to do this issue can now be seen for AccessResult::cachePerRole() and about half a dozen unit tests at #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view, interdiffs in comments 15 and 17.

With Organic groups - the cache context would be equivalent to user roles - so when the user's group membership changes, there's no need to invalidate the cached content - the user will just get end up seeing a different contextual version of it

… but when looking at a listing of things whose access is determined by role, and thus by permissions, and the permissions that a role has access to, then there is the need to invalidate the cached listing, since it is keyed by the role IDs, and those roles now have access to a different set of content.

But if the cache context is set on every render array, then why can't the tag also be set by default too? We already set the 'rendered' cache tag.

Well, then it's no longer a static list of cache tags (['rendered']), but one that depends on the current request: if the current interface language is French, then the Renderer would have to associate the cache tag for that translation. In other words: the cache tag to associate varies by cache context.

Which again points at how closely related cache contexts & tags can be!

So while that sounds nice at first sight, I'm not fully convinced. Any additional default cache context would then require the logic of Renderer to be modified.

I had an IRC conversation with catch.

We came to a few realizations:

1. What makes the user.roles cache context different from the others listed in #5, is the fact that it's an "invisible" cache context: it doesn't have a direct representation on the page itself. The interface language (translation), active menu trail (rendered menu) and theme (rendered templates) do.
2. It's also a level of indirection, since 99% of the time when varying per role, we're actually varying per permission.
3. A way to ensure that when a role (and thus a user) is granted or revoked a permission, is to not cache per role, but per permission(s). If the permissions for a role change, then so does the cache context. No cache tag necessary.

In other words: an alternative solution for this issue, is to replace user.roles with user.permissions (and potentially even support user.permissions:<permission>).

catch and I agreed it'd be beneficial for this discussion if we saw what that'd look like. So, working on a patch. Attached are the basic changes necessary. Now updating all of the AccessResult::cachePerRole() consumers.

EDIT: part of the next step snuck in:

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -27,6 +27,8 @@ class CommentAccessControlHandler extends EntityAccessControlHandler {
+    $access = AccessResult::allowedIfHasPermission('administer comments');

Please ignore this.

In this reroll, two access checkers using ::cachePerRole() have been converted.

• arguably the most complex one: CommentAccessControlHandler — this used ::cachePerRole() whenever permission-checking was combined with other things to check; a call to ::cachePerRole() ensured the same cacheability was applied as using ::allowedIfHasPermission(). But with the proposed shift (see #16 + #17) to user.permissions:<perm>, that no longer works. Either we'll have ::addCacheContexts(['user.permissions:' . $permission]) all over the place, or we convert to ::allowedIfHasPermission(). I chose the latter
• arguably the simplest one: ContactPageAccess

It's a bit annoying to do, and it's definitely annoying to review, but it seems doable. It's also easy to document: Whenever your access check is checking a permission, use ::(allowed|forbidden)IfHasPermission(s)().

Assigning to catch for feedback.

This seems potentially expensive though.

Yes, except that this is currently even happening for every single ::hasPermission() check, so it actually shouldn't be a significant extra cost:

  public function hasPermission($permission) {
    // User #1 has all privileges.
    if ((int) $this->id() === 1) {
      return TRUE;
    }

    return $this->getRoleStorage()->isPermissionInRoles($permission, $this->getRoles());
  }

  public function isPermissionInRoles($permission, array $rids) {
    $has_permission = FALSE;
    foreach ($this->loadMultiple($rids) as $role) {
      /** @var \Drupal\user\RoleInterface $role */
      if ($role->isAdmin() || $role->hasPermission($permission)) {
        $has_permission = TRUE;
        break;
      }
    }

    return $has_permission;
  }

+1 to #23. Less invasive, simpler conceptually and: no dozens of permission cache contexts per page load!

Implemented the direction outlined in #23/#24. (Took many rerolls, did those in #2459067: Testing issue for #2428703.)

Reverted all the changes to access checks & access control handlers from prior patches, instead, we're now replacing ::cachePerRole() with ::cachePerPermissions(). That makes for a much easier to review patch. Only RoleAccessCheck(Test) really depended on the role, all other usages depended on permissions, and have thus been replaced.

No interdiff, because it's almost completely different — an interdiff would make it harder to review.

Updating title + IS.

user.roles is still valid if you really genuinely have code that depends on the role. Not sure what, maybe something in environment status that shows only when the user has an administrator role assigned.

Core still uses it in two places:

1. +++ b/core/modules/user/src/Access/LoginStatusCheck.php
   @@ -31,7 +31,7 @@ class LoginStatusCheck implements AccessInterface {
      public function access(AccountInterface $account, Route $route) {
        $required_status = filter_var($route->getRequirement('_user_is_logged_in'), FILTER_VALIDATE_BOOLEAN);
        $actual_status = $account->isAuthenticated();
   -    return AccessResult::allowedIf($required_status === $actual_status)->cachePerRole();
   +    return AccessResult::allowedIf($required_status === $actual_status)->addCacheContexts(['user.roles:authenticated']);
   

   Here: an access check that checks whether the user has the authenticated role or not.

2. +++ b/core/modules/user/src/Access/RoleAccessCheck.php
   @@ -40,19 +40,19 @@ public function access(Route $route, AccountInterface $account) {
   +        return AccessResult::allowed()->addCacheContexts(['user.roles']);
   

   And here: an access check that checks whether the user has a certain set of roles or not.

However there's definitely potential for confusion there.

I think there actually isn't. 99% of the time, you're checking permissions, so you want to use user.permissions. So it's actually clearer, simpler.

Only when relying only whether the current user has a certain role or not, you need user.roles, i.e. when using role-based access, NOT permission-based access.

Of course, we are collectively so very used to varying per role, that that may get in the way. But to further communicate that you'd rarely need that, this also removes AccessResult::cachePerRole() in favor of AccessResult::cachePerPermissions().

CR created: https://www.drupal.org/node/2459373.

#31: thanks for adding that tag!

YAY!

CR published.

This unblocked:

• #2428837: Adding/updating interface translations should invalidate page & render caches
• #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view

Also updated the handbook: https://www.drupal.org/developing/api/8/cache/contexts