Access result caching per user(.permissions) does not check for correct user

.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -235,22 +239,64 @@ public function setCacheMaxAge($max_age) {
   +  public function cachePerPermissions(AccountInterface $account = NULL) {
   

   This could use comments reflecting the discussion that led up to this. The cacheable dependency vs. cache contexts switch here encapsulates a lot of 8.x caching concepts in a couple of lines.

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -235,22 +239,64 @@ public function setCacheMaxAge($max_age) {
   +  public function cachePerUserIfCurrent(AccountInterface $account = NULL) {
   

   If I wasn't familiar with 'current user' as a Drupal concept, I'd be wondering if the user account is current (has it been logged in recently or something). Could we make it cachePerCurrentUser($account).

   It's really cachePerUserIfCurrentUser($account) but that might be overdoing things.

3. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -235,22 +239,64 @@ public function setCacheMaxAge($max_age) {
   +  protected function isCurrentUser(AccountInterface $account = NULL) {
   

   The call out to \Drupal:: is a bit unavoidable, but this then muddies the role of this class as a value object - we don't want to be injecting stuff into a value object either.

   This might be the thing that makes #2628840: Separate access checks for current_user and $account not a duplicate of this issue - so we know which it is before we get here.

4. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   @@ -398,9 +398,31 @@ public function testCacheContexts() {
   +    // static method and using the non-current user.
   

   'an arbitrary account' might be clearer? Also this appears to be setting the current user? Doesn't look like it's testing what it says it's testing.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -235,22 +240,62 @@ public function setCacheMaxAge($max_age) {
   +      $this->addCacheableDependency($account);
   

   This definitely needs to also add the role cache tags, e.g. those cache tags and contexts returned when user.permissions is reduced to 'user':

   https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21C...

   So in reality this needs also in addition to the user object:

   $this->addCacheableDependency((new \Drupal\Core\Cache\Context\AccountPermissionsCacheContext($account))->getCacheableMetadata());

   because it is a permission that is varied here, not only the user itself.

   So whenever:

   - the user gains or looses a role
   - The general permissions hash changes (not used in core, but per user permissions are possible in general)
   - A role of that user changes

   this items needs to be re-generated.

   Fortunately reduction of permissions to user is already implemented, so re-using this is probably the simplest.

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -235,22 +240,62 @@ public function setCacheMaxAge($max_age) {
   +  /**
   +   * Convenience method, adds the "user" cache context for the current user.
   +   *
   +   * @param \Drupal\Core\Session\AccountInterface|NULL $account
   +   *   An optional account. If specified and it is not the current account then
   +   *   nothing will happen.
   ...
   +  public function cachePerCurrentUser(AccountInterface $account = NULL) {
   

   This feels really strange to pass an account into this ...

3. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -235,22 +240,62 @@ public function setCacheMaxAge($max_age) {
   +   * This checks whether an account is the current user.
   ...
   +  protected function isCurrentUser(AccountInterface $account = NULL) {
   +    return !isset($account) || ($account instanceof CurrentUserInterface && $account->isCurrentUser());
   +  }
   

   Clever, I like that.

#16: Yes, you need to add the cache tags of all the roles of the user for which you want the invalidation.

$tags = [];
$tags[] = 'user:' . $account->id();
foreach ($account->getRoles() as $rid) {
$tags[] = "config:user.role.$rid";
}

$this->addCacheTags($tags);

This is a pure invalidation problem can be ignored here as the output does not vary based on any contextual input, but is hard-coded ($account is passed in).

If the application uses the phase of the moon to determine those 3 users, then obviously it would need to add the '[moon]' cache context, but that is independent of the access implementation.

Rule of thumb:

- Whenever something is derived automatically from a certain independent context (request, session) even if indirectly (theme,user) then a cache context needs to be added.
- Whenever something is passed in as a parameter, cache tags (and potentially max-age) need to be added for invalidation.

So long story short:

Yes, just needs some tags added in addition to the dependency on account and likely the $tags[] = 'user:' . $account->id() is even enough.

#15:

What I meant is:

- Have the new cachePerCurrentUser() not accept any arguments but always cache per current user.
- Have cachePerUser accept an optional $account argument and do the check for current user there.

So you can do:

$access->cachePerUser(); // BC - falls back to current user, behavior will be removed in 9.0.0

New API:

$access->cachePerUser($account);
$access->cachePerCurrentUser();

chx asked me to take a look at this (more generally, people that aren't already intimately familiar with all the fancy new caching stuff in D8). This all pretty much makes sense and is understandable (esp with the latest changes), so +1 from me. Holding off on RTBC so that someone else that better understands the big picture can take another look first.

Okay, so chx asked me to write down my train of thought, so doing so:

- Given our scenario from above with the 3 users in the sidebar, whose access to the content is checked:

If we are one of those users, then the 'user' cache context is added - as one of the users is the current user.

However in this case where the user ids for that block came from elsewhere, this is actually wrong.

As for cache contexts it depends where something came from - not what it contains.

However - given that checking access for another user is likely an edge-case anyway - lets see what will happen if we add that and how bad it will break:

For simplicity lets assume we have users 2,3 and 42. And I am 42.

- We will add the user:2 and user:3 cache tags and also the cache tags of the roles.
- We will not add the user:42 cache tag and not the roles.
- We will however add the 'user' cache context.

The block is now "poisoned" for all cache entries to be per 'user', so will be placeholdered and rendered later.

So for a fictive user 23, they would get to the block, get a cache miss, then get back:

- user:2,user:3,user:42 cache tags and corresponding roles.

per our merge strategy, would also merge 'user' in.

So while the block is unnecessarily per user, it won't break and still be correct for other users.

Ironically it would not be correct for the user:42 as those cache tags are missing.

---

While all the interfaces support passing in an arbitrary account parameter, lets see why we want this anyway.

The only use case I can think of is that I want to check access for another user to be able to generate previews of things.

However in this case, we would want whatever that preview is to be cached per 'user', so a masquerade like approach is needed to be used.

---

So checking for currentUser is wrong - except when said user was gotten from the current_user service.

There is a difference between:

user:23 and user:[current_user] - semantically - even if user:23 at this moment happens to be the current user.

This can not be simplified.

The biggest problem we ran into with that before in the other issue where Wim tried to automatically add the 'user' cache context when something was retrieved from current_user service, was exactly the use case that someone did:

- Get current_user service $account
- User::load($account->id());

This looses the precious information on where the user ID originally came from.

UNLESS we change the account service to give back an ID of a constant with 'X_DRUPAL_CURRENT_USER_CONSTANT' or such.

Then User::load would be forced to load the user object by getting the rawId() [internal use only] and then set currentUser -> TRUE.

That would solve that use case.

---

TL;DR:

- For cache contexts it depends where something came from - not what it contains.
- We cannot do if ($this->currentUser == $account) { return TRUE } in isCurrentUser(), because the current user is depending on the context how it is used.
- The scenario above would make for a good test case for distinguishing the user matching accidentally and the user being indeed the current user.

Quick update from IRC discussion:

- AccountInterface::id() will return something like "[current_user:12345]" for the current user (from the current user service).

This is unfortunately a needed BC break.

- User::load() will learn how to load this format, so that User::load($account->id()) still works.
- In addition User::load($account) might be provided.

User::load sets $user->isCurrentUser = TRUE accordingly. (default: FALSE)

The actual bug here is the opposite of the title/summary so tagging for issue summary update.

Reviving with a comment from an issue closed as a duplicate of this one:

Knowing what I know now compared to three years ago, I still think this is a tricky issue.

Suppose you have a page where the visibility depends on a permission of the author, not the visitor. Something like: "Owner can display advanced information to the public". Then if a general audience member visits this page when the owner has that permission, the advanced version would get cached and displayed to all audience members. But if later on that owner has a change of permissions, retracting said permission (let's say their premium subscription expired), then said page would still be leaking the advanced information to the general public.

This would be alleviated by only varying by user.permissions if the passed in account is the current user's account and otherwise adding the user's roles cache tags (like we see when user.permissions gets optimized away). Which is exactly what chx's patch in #2628870: Access result caching per user(.permissions) does not check for correct user does.

Considering the example where I want to show the access to node FOO for user X, Y and Z in a block:

• We shouldn't vary this block by anything, because it doesn't. Not even 'user'
• We need to add the cache tags that may invalidate one's permissions, these are found in AccountPermissionsCacheContext::getCacheableMetadata(), which right now is only called when the context is optimized away.

chx's patch addresses the above, but fails where it checks for the current user and then adds the user.permissions cache context rather than the cacheable metadata. In order to make the above block work with AccessResult, it would need to somehow inform AccessResult that we care about either a user in specific or the current user, regardless of UID.

This is the conclusion @Fabianx came to in #29 by suggesting the current user gets a special ID.

Now there is also a reusability problem: chx's patch copies code from AccountPermissionsCacheContext in order to add the right cache tags. This is silly and unsustainable because we might need to do it for all cache contexts that start with 'user.' in case we want to cache something that depends on the user's roles, underwear size, etc.

It would be great if the user.permissions cache context et al were a wrapper around a reusable "engine" class which allows you to pass in any account. This way we can simply call said engine here and have the right cache tags applied with a oneliner.

In closing, considering AccessResult: It is horribly wrong to ask for any account and then cache whatever result came from that account for the currently logged in user. This is a potential security issue that luckily is extremely difficult to exploit because it only shows up in edge cases such as "view page as user X" that are almost always solved in other ways (such as Masquerade).

I'm frankly a bit stumped as to why this issue has been public for so long with no action taken.

Let me try to resurrects this issue... I am coming from #3238556

I have tried to add reroll of patch #24 on Drupal 9.4.x. There are lots of things got changed since the last patch #25, Please review.

Fixed failed commands on #47
Attached interdiff

@mxr576
Huh, this is scary. Good you bumped this!
Alas, there are some variants of this issue out there, let's collect the pieces.

IMHBSO, the current approach is doomed, and the way to go is #2628840: Separate access checks for current_user and $account for the reasons outlined there.

+++ b/core/lib/Drupal/Core/Access/AccessResult.php
@@ -259,24 +260,73 @@ public function setCacheMaxAge($max_age) {
+    // When the current user is access checked then we can use the cache
+    // contexts mechanism for more fine grained caching. Otherwise, the right
+    // tags needs to be added manually.
+    if ($this->isCurrentUser($account)) {
+      $this->addCacheContexts(['user.permissions']);
+    }
+    else {
+      $this->addCacheableDependency($account);
+      $this->addCacheTags(array_map(function ($rid) {
+        return "config:user.role.$rid";
+      },
...
+    }

I still think this is flawed and it's problematic to offer an API that can not actually deal with this.

no cache tag can ever replace user.permissions or any other cache context, they have a completely different purpose.

the only thing that this can accomplish is to invalidate caches if the accout or those roles are changed, not have different variations based on the given account and their permission.

"$this->addCacheableDependency($account);" *might* help if the system that passes through the account adds a cache context to tell others where this account is coming from and what it depends on.

A good example where this is working is the plugin contexts for a node: Blocks want a node context and that plugin context object is added as a cacheable dependency for their output. \Drupal\node\ContextProvider\NodeRouteContext then knows that its node is from the current route, so it adds that as a cache context. You could implement another node context provider that would return a certain node based on configuration and return that cache tag. the block doesn't need to know that detail.

But I have no idea how to apply that here.

The only feasible and safe option I can think of would be to disallow caching completely whenever access is checked for for any user other than the current one. I'm not sure if the logic for that should be in AccessResult or it's the responsibility of the code checking access for that user.

And to add to the fun. With the account switcher, even if it's the "current" user, it still wouldn't be correct to cache it then as it has been altered.

Let me copy the relevant part of my comment from a related issue that might be useful to address @berdir's concern:

Totally agree with this, but to say it otherwise. user.permissions cache context SHOULD be set in the presentation layer (e.g.: controller) when the result is made/calculated for the current user (e.g.: user is not a route parameter), lower layers MAY set that cache context to ensure it gets bubbled up, but only if they ensured that the user parameter they get as argument and the current user are the same.

My journey for this conclusion was started with building a custom access control handler (but we can also consider a traditional entity access handler) that gets the "acting user" as a parameter. Technically speaking, the (access) result returned by that handler could be stored in a persistent cache once it is calculated and with proper cacheability information it can be invalidated when it is necessary. This is the expectation that cannot be fulfilled when there is anything on the result that depends on a global state, like current user.

So @berdir wrote in #50:

no cache tag can ever replace user.permissions or any other cache context, they have a completely different purpose

That is not the goal here, rather using (render) cache contexts in the rendering (presentation layer) and not in lower layers.

not have different variations based on the given account and their permission.

Correct me if I am wrong, but the decision of whether a variation is needed based on a given account and their permission is a presentation layer-based decision, it is a decision that is based on the current (render) context. Considering an entity listing that is rendered as HTML or a JSONAPI response; depending on to whom it is rendered and for what purpose should define whether the result varies per account or permissions (a just made-up example: "My assigned tickets" vs. "all published articles"), the entity access control handlers must not flag that because they cannot know for sure.

A good example where this is working is the plugin contexts for a node: Blocks want a node context and that plugin context object is added as a cacheable dependency for their output. \Drupal\node\ContextProvider\NodeRouteContext then knows that its node is from the current route, so it adds that as a cache context. You could implement another node context provider that would return a certain node based on configuration and return that cache tag. the block doesn't need to know that detail.

A block is also a presentation layer solution and it rightfully depends on something global, something external, but implementations in a lower layer (application/domain - I am using Hexagonal arch references from time to time) MUST get contextual dependencies as parameters (like a user) and work with that information, nothing that comes from an external/global state anyhow (like the current user, the current route, etc.).

Marked #3238556: Wrong cacheability of AccessResult::allowedIfHasPermission() in case the provided account argument is not the current user as a duplicate, transferring credit

The Flexible Permissions module actually fixes this problem already as far as guessing the cache tags is concerned.

It calculates your permissions once by consulting all declared calculators and then caches the result. Any calculator can inform the system of what their calculations vary by (i.e. which cache contexts they use). If a user-dependent cache context is detected, it swaps the account, resolves the cache contexts and calculates the permissions, after which it swaps back to the original account.

All of this is cached using the aggregate of cache contexts (including user variants) and the VariationCache module, which allows for cache redirects to be used anywhere. So anytime we try to load someone's flexible permissions, we consult the cache contexts used to store them and again switch accounts if need be during retrieval, after which we switch back.

Because of VariationCache translating the cache contexts into cache tags, it would be far easier to write the above patch without Berdir's concerns in #50 as all you'd need to do is query the calculator for the cache tags it would set if you were to calculate permissions for another user.

Finally, Flexible Permissions was written in a way that core could implement it to replace the current permission layer. As soon as that happens, it can make full advantage of what I wrote above and the patch here would become fully viable.

Edit: All of the above, while invented for different reasons, is basically a workaround for something I've been asking about for a while: Why can't we simply fold cache contexts for any given value? If we had a system that said "Hey, please fold the 'user' cache context with this account for me" then we'd immediately have the cache tags available that we'd otherwise incorrectly hard-code as pointed out in #50.

@kristiaanvandeneynde

> If we had a system that said "Hey, please fold the 'user' cache context with this account for me" then we'd immediately have the cache tags available

Hmm, i was not around when you asked that, can you elaborate? Or better: Probably it deserves a separate issue to sort out.

It was brought up a few times in the issues leading up to the creation of the VariationCache module.

In a nutshell:

• Drupal caches only support cache tags
• The render cache supports cache contexts, which get evaluated to a specific cache ID
• For optimization purposes, a cache context can be folded when its parent is also present
• Therefore, cache contexts have to specify their cacheable metadata (mainly tags) when optimized away
• These tags (and max-age) are then applied to the regular Drupal cache, which as mentioned above only understands tags

This leads to cache contexts basically being able to translate themselves to a max-age and bunch of tags for a given contextual value. So the code from #50 is basically copying that logic from the user.permissions cache context, which we really shouldn't be doing.

If we were to be able to ask the user.permissions cache context the cacheable metadata for a given value, not the current context's value, then we can reuse its logic safely and make sure we get new cacheable metadata if it ever gets introduced.

Long story short: We have good logic locked away in cache contexts and no way to reuse it. The workaround is quite verbose by manually setting up a context (i.e. swapping account), folding the cache contexts and then getting the cache tags out of that result. Which is what the Flexible Permissions module ended up doing with assistance from VariationCache.

Should we postpone this on #2551419: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way, or do a partial fix here then open a new issue for a more complete one?

Re #58:
> Therefore, cache contexts have to specify their cacheable metadata (mainly tags) when optimized away. These tags (and max-age) are then applied to the regular Drupal cache, which as mentioned above only understands tags

Ah, that was the point i needed. Yes, now i completely grok it.

> Long story short: We have good logic locked away in cache contexts and no way to reuse it.

+100 for that.

Re #59:
Imho let's do one small step after the other:
- Create a new task "Add ability to get cache context cacheability for a specific context value"
- Postpone this issue on that.

@kristiaanvandeneynde What u think?
Also, the proposal of #56 imho really is worth a meta issue.

Adding an issue for being able to reuse cache context cacheabilty would definitely get my vote

Also opened #3340246: Node module must not set user.node_grants:* context if current user != acting user

2024 update and bump :)

So Re: #56: Both VariationCache and Flexible Permissions is in Drupal core \o/ (The latter is called Access Policy API)

If I am not mistaken the "cache context fold" solution atm locked inside \Drupal\Core\Session\AccessPolicyProcessor::processAccessPolicies() but that can be easily abstracted if necessary for solving this task. (Which #61 refers to)

#3452181: VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects may also come handy when we start refactoring code for solving this one.

    // Sadly, there is currently no way to reuse the cache context logic outside
    // of the caching layer. If we every get a system that allows us to process
    // cache contexts with a provided environmental value (such as the current
    // user), then we should update the logic below to use that instead.

Before we would go for a "Wild hunt" with defining an almighty API that would allow to fold ANY cache context with any contextual value, do we really need that to solve this issue? Would not we only need a generic, or worst case a copy-pasted solution from \Drupal\Core\Session\AccessPolicyProcessor::processAccessPolicies(), for translating user.* cache contexts t actual cacheability data? (I am asking this to balance between increasing scope vs. delivering solution.)

Alternative approach

How about introducing "self-contained" cache contexts? Because our actual problem that existing cache contexts depends on a global context (current user, current route, etc.).

I built this years ago in a module that may becomes public soon (no strict commitment).

/**
 * Defines a cache context "per acting user's node grants" caching.
 *
 * Cache context ID: 'node_grants_by_acting_user:||%uid' (to vary by all
 * operations' grants).
 * Calculated cache context ID: 'node_grants_by_acting_user:%operation||%uid',
 * e.g., 'user.node_grants:view||%uid' (to vary by the view operation's grants).
 *
 * The built-in NodeAccessGrantsCacheContext always calculates grants for the
 * current user that might not be the same as the acting user. This cache
 * context SHOULD BE only used in that case otherwise it is redundant.
 */

The only drawback of this approach that I see is implementation details (e.g., route id) and internal ids (e.g., UID) get exposed by cache context ids, but we could also find a solution for those.

The thing is, and I believe Berdir hinted at such a thing earlier in the comments, all systems need to know about your custom value you are trying to pass to the cache context.

With the access policy processor it's fine because the processor itself receives an account and we can use the account switcher there if someone uses a user-based cache context. It also passes the account object around so the individual access policies don't have to rely on the current user service (but could, if they wanted to). But we don't have this global representation of a value like "current user" for everything and neither do we have switchers for all the things.

Another thing you would end up with is that pages would become completely unchacheable or bogged down if we start using cache contexts that do not rely on a global context value. Keep in mind that cache contexts need to be fast. If we start bubbling up a bunch of cache contexts that need to load a few entities to get their return value, we'd be getting really slow pages.

Having said that, I am in favor of trying to rethink the cache contexts we have to allow for better reusability. Currently, what we're seeing in this issue is that we have a system which is given an account and then caches by a cache context which does not know what that account is and assumes the current user. The APP can work around this because it controls the caching, but the AccessResult does not so it can't.

So what we would need here is a calculated cache context called user.permissions:%uid, which would default to current_user if nothing was given and the permission hash of the account if provided. Then the cache context could have an optimization where it compares the passed in account ID to the current user and runs with the latter if the IDs match.

The issue is that if you have user.permissions and user.permissions:%uid on the same page, they will get folded into user.permissions and that crucial piece of information (the UID) gets lost.

Which brings us to the next possible solution: current_user.permissions and user.permissions, but that would require all of core and contrib to rename their user.foo cache contexts to current_user.foo. And you'd still risk folding issues as I'm not sure CacheContextsManager::optimizeTokens() currently knows how to deal with user:13,user.permissions:14 or user:13,user.permissions:13. At a glance it seems it would not work.

So hopefully it is starting to become clear what a pain in the ass it can be to tell a cache context which specific value to use for all ancestors of a given cache context.

Which brings us back to reusability (again): If we could somehow say: "I need to know the cacheability of a given context, provided I swap out its global value for something else.", we'd be in better shape. For user.foo we can using the account switcher, sure, but we need a universal way for all cache contexts like route, url, etc...

If we allowed cache contexts to declare what they depend on, we could write a service which allows you to call said cache context's methods while in a mocked environment so to speak (different route, user, url, ...), without actually changing the environment, unlike the workaround in APP.

Solve that, and you solve this whole issue and more. But keep in mind the most preferred option is the one that doesn't require all cache contexts to be rewritten, even though I see no other way around it.

Adding a note here that resolving this would also make it possible to remove the account switching in AccessPolicyProcessor::processAccessPolicies(), which doesn't play well with Fibers: #3576074: Current user is changed unexpectedly