- Issue created by @mxh
- 🇩🇪Germany jurgenhaas Gottmadingen
the way the user object as sessionUser property is being set is not quite right, because it's then always referencing to one entity.
I'm not sure, I do understand the problem. Would it be possible to write a test which demonstrates how this can go wrong?
- 🇩🇪Germany mxh Offenburg
This is setting a static property once, using an entity object that won't get exchanged once the current user is being switched:
self::$sessionUser = $storage->load($current_user->id());
On the other hand, the purpose of an
AccountProxyInterface
object would automatically use the currently logged in user. - 🇩🇪Germany jurgenhaas Gottmadingen
The
sessionUser
is the one user that's currently logged in. That should never change, so isn't what you're describing on purpose? - 🇩🇪Germany mxh Offenburg
The "session user" is a representation of the current state of the "current user". And the "current user" can be switched using core's
account_switcher
service. That may happen, otherwise that service wouldn't exist. I'm using it for a access report page, where I check access for various user accounts and show the results on one page. But that report currently does not produce correct results when ECA is involved in any way. - 🇩🇪Germany jurgenhaas Gottmadingen
I don't think, that's correct. The description of that token reads like this:
The user account that dispatched the event, regardless if ECA is processing models under a different account. This is only available if ECA is configured to always run under a specific account.
So, that token holds the user at the point when ECA starts processing, before being switched to the user defined in the settings to always process under a different user account.
- 🇩🇪Germany mxh Offenburg
My explanations are correct, especially as analyzed in #3: It's a static property, being initialized once for the whole runtime. When switching the current user with account_switcher service, an AccountProxyInterface object automatically refers to the correct current user.
- 🇩🇪Germany jurgenhaas Gottmadingen
My explanations are correct, especially as analyzed in #3: It's a static property, being initialized once for the whole runtime.
Yes, that's how it should be. The session user never changes. At least, that's how I understand the purpose of it. No matter how often the user gets switched by ECA, the session user has to stay what the runtime session has been at the very beginning.
I probably find myself in a deep dark tunnel? I'm not seeing the issue, sorry.
- 🇩🇪Germany mxh Offenburg
I wouldn't have created an issue if I wouldn't have a problem. I took the time to analyze this, give an explanation and re-reading my explanations I think they makes it obvious to make sense what the problem is about.
Also, the very first ECA execution on runtime may happen within a place where a component may have switched to a different current user on purpose. Creating an overview for access reports is a good example for that where you may check access for a privileged user and another time for a non-privileged user.
- 🇩🇪Germany mxh Offenburg
Maybe the following code part makes it more obvious: https://git.drupalcode.org/project/eca/-/blob/2.1.x/src/EventSubscriber/...
public function initializeUser(ConfigFactoryInterface $config_factory, AccountInterface $current_user): void { if (self::$initialized) { // Already initialized. return; } self::$initialized = TRUE; // ... }
So this logic is applied only once for the whole runtime. Given my possible case in #9 this may even be dangerous as the current user at the very first execution may be a privileged user and this would remain the same one on all subsequent processes on the same runtime.
- 🇩🇪Germany jurgenhaas Gottmadingen
I trust you, of course, I would never expect you to open an issue for no good reason.
But I still don't understand the problem. The
session_user
token was created to only be available, if a global user is defined ineca.settings
; otherwise that token will not be defined. At it gets initialized by the event subscriber, I don't see how anything else could switch to a different user before this gets initialized with that user of the current PHP process.I understand what you're describing what you want to achieve, but that's most likely the problem with that global user to run ECA processes, that it just doesn't allow any sort of proper permission checks. We've discussed this recently at various places, and that's why we recommend to no longer use this global user.
If you still think I'm not getting the point, then please provide a model that demonstrates where this is going wrong.
It's not about the user switch being done by ECA, it's about anything else that may switch the user on purpose.
Also, the very first ECA execution on runtime may happen within a place where a component may have switched to a different current user on purpose. Creating an overview for access reports is a good example for that where you may check access for a privileged user and another time for a non-privileged user.
What "else" would switch the user account on purpose?
- 🇩🇪Germany mxh Offenburg
What "else" would switch the user account on purpose?
Access Policy API exactly does this within
Drupal\Core\Session\AccessPolicyProcessor::processAccessPolicies
:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co... - 🇩🇪Germany jurgenhaas Gottmadingen
OK, that's a new thing. And how would be ever get a chance to get to the original user account in that context? Because that's what the documented purpose of the
session_user
token is: knowing the original session user. Does Access Policy API expose that? - 🇩🇪Germany jurgenhaas Gottmadingen
I just realized, there is an
AccountEvents::SET_USER
which is dispatched when the current user is being set. We may want to subscribe to that event and safely grab the session user from there, which should be happening very early before anything else like Access Policy API can alter anything. - 🇩🇪Germany jurgenhaas Gottmadingen
I've tested the
AccountEvents::SET_USER
event, and it gets dispatches early in the process. This may be more reliable to set the static variable\Drupal\eca\EventSubscriber\EcaExecutionSwitchAccountSubscriber::$sessionUser
to the real authenticated user, which is what this variable was designed to be there for.@mxh would you please test that in your scenario together with the Access Policy API to see if you then get the correct session user?
- 🇩🇪Germany mxh Offenburg
This issue is clearly a bug, I'm wondering why it got changed to a feature request.
Because that's what the documented purpose of the session_user token is: knowing the original session user. Does Access Policy API expose that?
This may be more reliable to set the static variable \Drupal\eca\EventSubscriber\EcaExecutionSwitchAccountSubscriber::$sessionUser to the real authenticated user, which is what this variable was designed to be there for.
The only thing ECA can ever know, is the currently set user before ECA itself is executing anything. Which includes switching the user inside of ECA process execution. There is no safe way to always guarantee that we have the "real session user" at that point.
For continuation I try to summarize this issue so far:
The current behavior of ECA is:
- When ECA is being executed the very first time, it sets the current user's entity object assession_user
token. On every ECA execution at the same runtime later on, the same instance of that initially determinedsession_user
is being used.
This behavior is wrong as described in #9.To resolve this, my previous assumption to just use the AccountProxy object was wrong. Because the
AccountProxy
may then of course hold the "current user" set by ECA, which is not what we want to be stored there.I think the correct behavior would be:
- When ECA is being executed on a root-level process (= execution is not a nested subprocess), it sets the current user's entity object assession_user
token. When the last ECA process on a root level finished execution, thesession_user
token may get removed from the token variables environment to avoid further confusion. - 🇩🇪Germany jurgenhaas Gottmadingen
The only thing ECA can ever know, is the currently set user before ECA itself is executing anything. Which includes switching the user inside of ECA process execution. There is no safe way to always guarantee that we have the "real session user" at that point.
The
\Drupal\eca\EventSubscriber\EcaExecutionSwitchAccountSubscriber::initializeUser
method is called when the event subscribers get registered. So, that's not bound to the switch user action. Or am I wrong in that understanding?Regardless,
AccountEvents::SET_USER
seems the better approach, as it will register the session user right when the current user session is being set by core. And that's what documentation tells, what this token is supposed to provide. Or do you disagree with that statement? - 🇩🇪Germany mxh Offenburg
Regardless, AccountEvents::SET_USER seems the better approach, as it will register the session user right when the current user session is being set by core.
Whichever component in the system comes first, would "win". We have no guaranteed way that this is always the very first thing happening when setting the current user.
And that's what documentation tells, what this token is supposed to provide. Or do you disagree with that statement?
I don't necessarily disagree with it. I'd also like to have the "real session user" all the time if we could get it in a guaranteed manner. But we simply don't have it as described above.
Any other component could decide else what is supposed to be the "session user". Example: The Masquerade module
I think ECA should do what's possible within its scope, and I think the described way in #16 is still the "most" safe one I can currently think of. To recap it's the following way:
When ECA is being executed on a root-level process (= execution is not a nested subprocess), it sets the current user's entity object as session_user token. When the last ECA process on a root level finished execution, the session_user token may get removed from the token variables environment to avoid further confusion.
But I'd be happy to discover better available options.
- 🇩🇪Germany jurgenhaas Gottmadingen
Whichever component in the system comes first, would "win". We have no guaranteed way that this is always the very first thing happening when setting the current user.
OK, we have to analyse this. As the
current_user
service is being used almost everywhere, it must be set very early in the process by Drupal core. Here is where this happens\Drupal\Core\EventSubscriber\AuthenticationSubscriber::getSubscribedEvents
:// The priority for authentication must be higher than the highest event // subscriber accessing the current user. Especially it must be higher than // LanguageRequestSubscriber as LanguageManager accesses the current user if // the language module is enabled. $events[KernelEvents::REQUEST][] = ['onKernelRequestAuthenticate', 300];
Now, do we know anything that beats that and comes sooner?