![Announcement icon Announcement icon](https://www.drupal.org/files/styles/grid-2-2x-square/public/announcements/drupal-evergreen-logo-280X280px%20%281%29_0.jpg?itok=PHpn6rCb)
Problem/Motivation
Follow-up from #3345929: Let "Token: set value" use new objects when referring to existing ones:
Regarding action "Token: set value": Another thing that may need to be considered is checking for field access.
Plus if we need to filter out any loaded referenced entity that is not accessible for the current user (using "view" operation). If so, we also need to correct that for GetFieldValue and possibly other places where we automatically load referenced entities.
When loading entities with "Entity: load" and "Entity: load via reference" we perform an access check whether the current user can view the targeted entity.
When using "Entity: get field value" we check for field access. However, this action automatically loads referenced entities but does not check access on every referenced entity that got loaded.
tldr; We need to consider whether we need to add more access checks, especially at places where fields are being used (field access) and entities are being loaded (e.g. with ->referencedEntities()
). It currently looks like it's not yet consistent.
Comments
Comment #2
berdirJust a drive-by comment because the load action and access checking didn't work like I expected it to.
As a developer, I defined an action to load an entity that references one that I was acting on and wanted to know if it exists or not. I did that like I write code. I loaded the entity, then did conditions to see whether the resulting token was set or not. Only when that didn't work I realized that the action has a built-in access check to see if the entity exists and I have access to it, and was failed/aborted without a way to act on that. So I had to rewrite it to first have a condition to see if it exists, and then load it if it does. From a performance standpoint, that's not optimal because it's running the fairly expensive entity query twice.
A similar problem with the access check, I'm working on an internal site where content is not accessible to anonymous users and there are cron jobs that acton content and change it, which then triggers ECA stuff. That didn't work again because the anonymous user wasn't allowed to view that entity.
When writing code, you only want to/need to check access when you display those entities/fields to the user.
I would expect that quite often, you do stuff with ECA that the current user wouldn't be allowed to do, so you don't want access checks to happen. My workaround was to just set the executing user to 1 through the global setting. Entity queries now force you to make a decision whether or not access checks should happen and I think maybe the same should be the case here, meaning a setting whether access should be checked or not, defaulting to on.
Comment #3
jurgenhaasThis sounds like a great suggestion to enable entity access check by default but allow the site owner to turn that off globally. Maybe, like with switch user, we then also could provide an action to disable access check for a specific model only.
However, this raises the question about sharing models across sites or even between agencies: if models were build under the assumption that they always run as user 1 or with entity access check being disabled, how does such a model propagate that "requirement" when being shared?
We could add that as a property to models. Which makes me think if we even wanted to think about a third level of setting: run as user 1 or without access check on a model base, i.e. we could provide those 2 settings globally, per model and as an action for only selected parts of the model.
Comment #4
berdirFor configuration, I meant a per-action configuration option, the user 1 trick is already mostly a global way to switch it off.
Comment #5
jurgenhaasComment #6
jurgenhaasComment #7
jurgenhaasComment #8
jurgenhaasI've evaluated this issue again and removed it now from the 2.0 target release for these reasons:
For the "Get field value" action, as an example, we would have to re-implement almost all of the execute code for the access control. This feels like a lot of extra processing while, it isn't entirely clear, if checking access at that point is really appropriate. One could argue that subsequent actions will be checking the access for that entity already.
To achieve consistency, a deep research is being required across all action plugins. Once a specific and complete list of candidates is available, the evaluation can only even get started. Compiling such a list reliably is something that needs extra effort.
And for the suggested improvements by @Berdir in #2 and #4 I've created a separate issue in #3449091: Toggle access control per model and per action as they can be implemented regardless.
Comment #9
jurgenhaas