Support creation of API driven content entities

Created on 20 July 2023, over 1 year ago
Updated 12 June 2024, 5 months ago

Problem/Motivation

Currently it is not possible to create content entities, which don't implement a custom access handler. If so, the following code will prevent the entity from being created:
NewEntity.php:147

      elseif (!$this->entityTypeManager->hasHandler($entity_type_id, 'access')) {
        $access_result = AccessResult::forbidden('Cannot determine access without an access handler.');
      }

Modules like message are using a content entity, which has no user interface and no custom access handler - and even though they would have, the next part of the NewEntity's access method will check the "createAccess" which by default checks for a permission, not existing in such modules.

Steps to reproduce

- Install the message and message_example module
- Create an ECA workflow which creates a message for instance after creating a node. Caution, the message_example module does this aswell via hook_node_insert
- Verify that the message entity by ECA is not created, the one by message_example on the other side is

Proposed resolution

Change default behaviour when checking access when creating content entities. Instead of denying the access, we should grant it since this seems to be the default behaviour in drupal core wen using plain entity API to create such entiites.

Remaining tasks

Discuss how to support content entities without custom access handlers.

Feature request
Status

Active

Version

2.1

Component

Code

Created by

🇩🇪Germany Hydra

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • 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 be access 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.

  • 🇩🇪Germany jurgenhaas Gottmadingen
  • 🇩🇪Germany jurgenhaas Gottmadingen
Production build 0.71.5 2024