- Issue created by @Hydra
- 🇩🇪Germany jurgenhaas Gottmadingen
This code is from December 2021 and hasn't changed since its introduction. We had made the assumption, that when an entity type has no access control handler, that we default to
access denied
. They may have been the wrong choice, and it looks like the default should beaccess allowed
. I mean, if there is no access control handler, there is nothing in place that ever denies access. So, not even ECA should step back and prevent that from happening.@mxh do you have any objections?
- 🇩🇪Germany mxh Offenburg
ECA has (partially) very restrictive access checks on action plugins. Just allowing access because of a missing access handler would make this even more inconsistent, maybe even dangerous. I'd suggest to at least make a "minimal" access check then when no access handler is available: Check whether current user has an admin role and if so grant access, otherwise deny it.
- 🇩🇪Germany jurgenhaas Gottmadingen
I follow that logic, and we really want to be careful.
However, if there is a content entity class which has neither any UI component nor any access control handler, it seems safe to assume that those entities are there to be CRUD programmatically only. And if the provider of that entity doesn't provide any access control handler, there isn't any access control at all. Why should ECA take more care than everything else?
In fact, if the absence of the access control handler means, there should be no access by default, how would anyone ever grant access then? That's why I suggested, that in such cases only, the default assumption we've made 18 months ago was probably wrong.
- 🇩🇪Germany mxh Offenburg
However, if there is a content entity class which has neither any UI component nor any access control handler, it seems safe to assume that those entities are there to be CRUD programmatically only.
I don't know, as long as there's no documentation source or anything like that, this seems too much of a subjective assumption. My assumption is: when an entity type is supposed to be always allowed to be accessed, its according access handler would just always grant access. But when the access handler is missing, I'd say that it is simply not defined, so we can't tell whether access should be granted or not.
Let's put in a worst case scenario: A developer who introduced a bug that accidentally removes the access handler from an entity type definition. Then ECA would (wrongly) grant access to that sort of entity. Maybe this is a bit of extreme edge case, but it's not impossible.
As ECA is not consistent regarding access checks (#3348425) this argument can be seen one way or the other, and this is just my opinion. You finally decide which direction ECA should follow among access checks. Finally at least it should be a consistent behavior that ECA users can trust on.
- 🇩🇪Germany jurgenhaas Gottmadingen
Thanks @mxh, your input is really much appreciated. Just one last feedback loop on this one:
But when the access handler is missing, I'd say that it is simply not defined, so we can't tell whether access should be granted or not.
If there is no access handler, and we came to the conclusion that there should NOT be any access, and if that was the intention of the developer, then there will never ever be any access to those entities. But why are those entities there in the first place? If they can never be accessed (i.e. created or updated), there is no point in having them.
This would only be a wrong assumption, if the entity design was meant to be accessible by the providing module. In that case, the solution to the IS would have to be to ask the provider of the entity to add an access handler. @Hydra would that be an option?
- 🇩🇪Germany mxh Offenburg
As mentioned in #3, the following compromise could work out for all: I'd suggest to at least make a "minimal" access check then when no access handler is available: Check whether current user has an admin role and if so grant access, otherwise deny it.
I think with that approach we at least did the best we can in such case (esp. for an extreme case as mentioned in #5).
Another approach could be to introduce a new option for skipping access checks, something like mentioned here: https://www.drupal.org/project/eca/issues/3348425#comment-15037117 📌 Evaluate consistency of access checks when using fields and loading referenced entities Active
- 🇩🇪Germany Hydra
@jurgenhaas Yeah sure, I could go with that. In fact This is what my work-around module does. It just adds an AccessHandler to the Message entity which does nothing but implementing createAccess and allowing it for authenticated users. It just felt so wrong doing this.
So do we need to address the UX issue mentioned here, that it is possible to interact with of those kind of entities in ECA but without ever getting Access to perform the the interaction?
- 🇩🇪Germany mxh Offenburg
Just noticed there is an "admin_permission" annotation available for simple access checks on entities without complex access control. The annotation is also available on the requested "message" entity type: https://git.drupalcode.org/project/message/-/blob/8.x-1.x/src/Entity/Mes...
We probably should use the defined admin_permission to check for access, in case we have no access control handler available.
Also see documentation of
Drupal\Core\Entity\EntityTypeInterface::getAdminPermission
:/** * Gets the name of the default administrative permission. * * The default \Drupal\Core\Entity\EntityAccessControlHandler class checks this * permission for all operations in its checkAccess() method. Entities with * more complex permissions can extend this class to do their own access * checks. * * @return string|bool */ public function getAdminPermission();
- 🇩🇪Germany jurgenhaas Gottmadingen
Oh, of course. I remember using that in the past, but never thought about it in the context of this issue. That sounds like a very reasonable approach, yes.