EntityCreateAccessCheck needs to enforce a bundle is set by the route for entity types that support bundles

Created on 23 October 2017, about 7 years ago
Updated 8 May 2024, 6 months ago

EntityCreateAccessCheck provides a route access checker which can be used in two ways:

For an entity type that does not have bundles, you just give the entity type ID:

  requirements:
    _entity_create_access: 'menu'

For an entity type that does have bundles, you need to give the entity type ID and a bundle, which can be loaded from a route placeholder:

  requirements:
    _entity_create_access: 'taxonomy_term:{taxonomy_vocabulary}'

If you use it without the bundle, but with an entity type that does have bundles, nothing breaks, until a contrib modules implements hook_entity_create_access() and expects to find a $bundle parameter, and makes API calls that pass $bundle to APIs that expect it. Then everything breaks and it's really hard to debug. (For example, this problem caused a crash that was apparently in OG, but was caused by the incorrect use of this route access in Commerce - #2911542: "Add payment" and "Add order" routes need to use _entity_create_any_access, not _entity_create_access β†’ .)

The use without the bundle value with an entity type that does have bundles is incorrect because:

1. The _entity_create_access router check is handled by EntityCreateAccessCheck, which calls:

    return $this->entityManager->getAccessControlHandler($entity_type)->createAccess($bundle, $account, [], TRUE);

2. EntityAccessControlHandlerInterface::createAccess() says:

   * @param string $entity_bundle
   *   (optional) The bundle of the entity. Required if the entity supports
   *   bundles, defaults to NULL otherwise.

I think EntityCreateAccessCheck should simply throw an exception if the bundle value isn't available for an entity type that has bundles.

πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
EntityΒ  β†’

Last updated 1 day ago

Created by

πŸ‡¬πŸ‡§United Kingdom joachim

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡ΈUnited States thomas.fleming

    thomas.fleming β†’ made their first commit to this issue’s fork.

  • Merge request !7981Convert patch to MR. β†’ (Open) created by thomas.fleming
  • Pipeline finished with Failed
    6 months ago
    Total: 575s
    #167943
  • Status changed to Needs review 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States thomas.fleming

    I converted the patch to a merge request and also updated the call to entityManager to use entityTypeManager. Also confirmed that this is still an issue.

  • Pipeline finished with Failed
    6 months ago
    Total: 663s
    #168018
  • Status changed to Needs work 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Thanks for reviving this one and verifying still an issue

    Tagging for issue summary as it should follow the standard issue template.

    Also tagging for tests since the change appeared to break a number of tests.

    Thanks!

Production build 0.71.5 2024