Current user is changed unexpectedly

MR 14937 has a failing kernel test that I think shows what the issue is.

For a fix that can go in a patch release, we'd probably need to look at a fiber trap again or similar mechanism. Ideally, either the account isn't switched at all in AccessPolicyProcessor, or it can be switched back before all the calculations. But in the case of the first, #2628870-70: Access result caching per user(.permissions) does not check for correct user explains the difficult in providing a cache context for a provided user instead of the current user. For the second, the cache must take the correct user data into account, so that the account can't be switched back before then.

So instead, I looked at whether it's possible to provide a way to make current user, or the current language as in #3569172: Weird language negotiation behavior inside getLanguageSwitchLinks leading to incorrect languages being used to work contextually within the scope of the fiber hierarchy they run in. A POC of this is MR 14941, and the general idea is this:
Introduce a new class/service called FiberStack, which tracks the hierarchy of instantiated fibers. The name is actually quite bad, since it's not a stack at all and more of a tree, but gonna put a pin in that

• Every time a fiber is instantiated, a call to FiberStack::addFiber() should be called, and that creates a record in an array, indexed by the fibers' spl_object_id. In addition, if the fiber is instantiated within a running fiber, then a parent property for the item in the array is set to the value of the parent fiber's spl_object_id
• Then in AccountProxy and AccountSwitcher, any sets/gets and account switches are tracked using the fiber stack. For example, if the account switcher switches to an account running in a fiber, then the value of the current user is set in a tracking array indexed by the current fiber's spl_object_id. For calls to AccountProxy::get(), first it's checked whether there's a running fiber, and whether a user was registered for the specific fiber. If not, then iterate through all the fiber's ancestors until an account is found

Since this is new API, this solution wouldn't work for a patch release. It's also turned out to be more complex to implement than anticipated. Tests are still not passing, and I think a lot of that has to do with the fiber stack not being reconstituted correctly after a container rebuild or when services are serialized. I tried addressing some with static properties instead, which raises the question of whether the fiber stack should be a static class instead of a service, but there are still outstanding issues.

Anyway, I'm not too sure going further with this idea will be fruitful, but I'm also not too sure what alternatives there would be better to explore.

Closed MR 14941, because it should have been obviously silly to me and that not every current user change should automatically be limited to the scope of the current fiber. If a global scope mutation needs to be contained to the current fiber, that needs to be handled intentionally where the mutation is done.

Added a fix to 14937.

There should definitely be investigation on how to avoid the account switch, but this addresses the bug for now.

Could we have a follow-up for this investigation? Or if one already exists, could you, please, link it here?

It is essentially #2628870: Access result caching per user(.permissions) does not check for correct user, with #2628870-70: Access result caching per user(.permissions) does not check for correct user: providing some possibilities/challenges:

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.

Once that's done, then the account switching can be removed in AccessPolicyProcessor, either directly in that issue or in a follow up.

Updated the IS, and proposed some ideas for general workarounds for Fibers issues that maybe could go in to 11.4/12.0 while harder solutions are worked on.

This looks good. I wonder whether we want to add some kind of FiberTrap class with a static method that we can call from places like this (method could either take a callable or a fiber). We currently only have two places though so it's not quite rule of three yet.

It would theoretically have been possible to do something like this too: https://git.drupalcode.org/project/drupal/-/commit/df7ee1dbac085c3898ab8... - e.g. keep switching the user back and forth so that we can re-suspend the fiber, but this use case is sufficiently rare I don't think we'd get much benefit for a lot more complexity. It was bad enough implementing that once.

Committed/pushed to main, 11.x, and 11.3.x, thanks!

Stubbed a follow up: #3576594: [PP-1] Avoid account switching when calculating permissions in AccessPolicyProcessor::processAccessPolicies()

While perf hunting on a project, Claude suggested this as a possible improvement to ::processAccessPolicies to avoid creating Fibers unnecessarily:

if (!\Fiber::getCurrent() || $account->id() === $this->currentUser->id()) {
  return $this->doProcessAccessPolicies($account, $scope);
}

Since access check can run thousands of times on a single page load it seems to save a few ms, but it's not huge, so IDK if it's worth reporting a dedicated issue...

Edit: I eventually created it #3593008: AccessPolicyProcessor::processAccessPolicies() creates unnecessary Fibers when checking permissions for the current user