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

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

2024 update and bump :)

So Re: #56: Both VariationCache and Flexible Permissions is in Drupa core \o/ (The latter is accalled 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)

🐛 VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Fixed 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 is depending 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 like 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.