Incomplete/broken access checking in EntityController::addPage()

Thanks for getting the ball rolling on this.

You should inject the entityTypeManager, not the entityManager (it's deprecated).

+++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
@@ -0,0 +1,75 @@
+   */
+  public function __construct(EntityManagerInterface $entity_manager) {
+    $this->entityManager = $entity_manager;
+  }

use entity type manager.

Don't hardcode that there is a bundle entity type. Instead, get the bundles from the EntityBundleInfo service. Also, check if there is a bundle entity key, if not, call createAccess() once, without a bundle.

Maybe we can extend the tests from the issue that added add-page, and e.g. extend the test entity type create permissions to be per-bundle optionally and then in that test, give the user only bundle specific permissions?

1. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,90 @@
   +/**
   + * Defines an access checker for any entity creation.
   + */
   

   "any entity creation" isn't very specific.

   not sure what a good short description would be. Maybe "for creating an entity of any bundle."'

2. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,90 @@
   +    $entity_type = $route->getRequirement($this->requirementsKey);
   

   $entity_type_id and then you can use $entity_type for the definition.

3. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,90 @@
   +    if (!$entity_type_definition->hasKey('bundle')) {
   +      return $access_control_handler->createAccess(NULL, $account, []);
   

   this should also pass TRUE to return an access result object.

4. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,90 @@
   +    foreach ($this->entityTypeBundleInfo->getBundleInfo($entity_type) as $bundle_id => $bundle_label) {
   

   IIRC, getBundleInfo() returns a bundle_info array. Either name it like that, but since you don't need it anyway, you could also do an array_keys() on getBundleInfo()

5. +++ b/core/modules/system/src/Tests/Entity/EntityAddUITest.php
   @@ -74,6 +74,25 @@ public function testAddPageWithBundleEntities() {
   +    // Confirm a new user has no permissions to see a new bundle.
   

   // Create a bundle that the user does not have create permissions for?

6. +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestPermissions.php
   @@ -0,0 +1,75 @@
   +      "create $type_id entities" => array(
   +        'title' => $this->t('%type_name: Create new entity', $type_params),
   +      ),
   +      "edit own $type_id entities" => array(
   +        'title' => $this->t('%type_name: Edit own entity', $type_params),
   +      ),
   +      "edit any $type_id entities" => array(
   +        'title' => $this->t('%type_name: Edit any entity', $type_params),
   +      ),
   +      "delete own $type_id entities" => array(
   +        'title' => $this->t('%type_name: Delete own entity', $type_params),
   +      ),
   +      "delete any $type_id entities" => array(
   +        'title' => $this->t('%type_name: Delete any entity', $type_params),
   

   I would limit it to the permissions we actually need, we can always extend it when necessary.

   Also, not sure if we should get the entity type in there to avoid clashes, especially since it is a test module. E.g. create test entity_test_with_bundle entities or so.

There is indeed another bug here.

I now finally understand why @mbovan tried to implement access checks on the bundle entity type in the core_crm issue that caused us to find this.

And that's that we check create access for the bundle entity type with the given bundle, instead of the entity type we want to create an entity of. That of course doesn't make any sense and caused it to fail.

After that, there was just one fail remaining. And thinking about this, I finally understand the administer content types permission check in NodeAddAccessCheck.

The question is, what should happen if there are no bundles yet. The test currently expects that you get a page that allows you to create one. But if there's no entity types, we had nothing to check and access was denied.

The answer is to check if the user is allowed to create bundle entities. If so, we allow access. That ties the access check to EntityController::addPage(), though, other pages might not want that behavior, so maybe we need to make it configurable with a special requirement flag?

Retitling.
We have two issues here:
- addPage() checks createAccess() for bundles of the wrong entity type, and the tests aren't complete enough to catch it.
- there is a missing access check to ensure the user can create an entity of at least one bundle, before showing addPage()

The patch fixes both and adds missing test coverage.

Nitpick: EntityTestPermissions is new code, but uses array(). If a reroll happens, let's use [].

The question is, what should happen if there are no bundles yet. The test currently expects that you get a page that allows you to create one. But if there's no entity types, we had nothing to check and access was denied.

The answer is to check if the user is allowed to create bundle entities. If so, we allow access. That ties the access check to EntityController::addPage(), though, other pages might not want that behavior, so maybe we need to make it configurable with a special requirement flag?

That makes sense. I don't see a need for a special flag though, the additional logic feels reasonable.
Let's say I have a content entity form where the bundle is selected with #ajax. If there are no bundles I still wouldn't want a 403, I'd want an opportunity to present my own message/UI logic, and the bundle entity type check makes sense again.

1. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,107 @@
   +   * The entity manager.
   

   This is entity *type* manager. Super nitpik.

2. +++ b/core/modules/system/src/Tests/Entity/EntityAddUITest.php
   @@ -74,12 +78,42 @@ public function testAddPageWithBundleEntities() {
   +    $this->clickLink(t('Test label'));
   +    $this->assertResponse(200);
   

   I quickly checked this and it seems we are passing "entity_test_bundle" instead of "test" as $entity_bundle to checkCreateAccess() on access control handler. This is wrong as this isn't id of the bundle but it's entity type id.

   Quickly poking deeper it seems that the value of _entity_create_access on the route might be wrong.

+++ b/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php
@@ -68,7 +69,11 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+    return AccessResult::allowedIfHasPermissions($account, [
+      'administer entity_test content',
+      'administer entity_test_with_bundle content',
+      'create ' . $entity_bundle . ' entity_test_with_bundle entities',
+    ], 'OR');

Nice, I wasn't aware that this is possible!

Do you think the test is wrong or is there some actual bug in the code still?

Oh wow

This fixes the unit test, the unit test now actually has to use a path that makes sense and there's now always a call getEntityBundleType().

+1 on the approach. I ran into this a while back but wasn't able to get my PR merged into entity in time. And this is a much cleaner solution.

Looks like we're good to go. I'm happy with punting the general entity_test route cleanup to a followup.

Mostly nits. I don't have good ideas for the naming, feels like we're running into the limit of terminology a bit.

Tagging this as rc target, also leaving it RTBC in case someone else wants to commit it before RC - but probably won't be me.

1. +++ b/core/core.services.yml
   @@ -1069,6 +1069,11 @@ services:
   +  access_check.entity_create_any:
   

   Not sure about the service name. It's not for creating any entity.

2. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,107 @@
   + * Contains \Drupal\Core\Entity\EntityCreateAnyAccessCheck.
   

   Similarly not sure about the classname - it's not for creating any access checks.

3. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,107 @@
   + * Defines an access checker for creating an entity of any bundle.
   

   Comment isn't 100% clear whether it's any bundle of an entity.

   Or any bundle entity.

4. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,107 @@
   +   * Checks access to create any entity type for the given route.
   

   Now we're not creating entities but entity types.

5. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,107 @@
   +   *   The parametrized route.
   

   typo.

6. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -151,7 +151,11 @@ protected function getAddFormRoute(EntityTypeInterface $entity_type) {
   +      //   test entities have incosistent usage of "add-form" link templates.
   

   inconsistent.

Comment changes look good to me.

1. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,107 @@
   +        break;
   

   Why this break? What if a particular bundle returns FORBIDDEN as its access result?

   Back to NW for this.

2. +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
   @@ -0,0 +1,107 @@
   +      // If there are no bundles, check if the user is allowed to create new
   +      // ones. If so, allow access, so the add page can show a link to create
   +      // one.
   +      // @see \Drupal\Core\Entity\Controller\EntityController::addPage()
   ...
   +        return $access->orIf($bundle_access_control_handler->createAccess(NULL, $account, [], TRUE));
   

   So say that you don't have any bundles for Node, this is saying that the user will be able to see a "Create node bundle" link?

   I don't see where that is happening in EntityController::addPage()?

1. Forbidden is fine. Just because one is forbidden doesn't mean that you can't create any others. This isn't combined access for a single thing. It's a real OR, not an AccessResult or.

2.

      // Build the message shown when there are no bundles.
      $link_text = $this->t('Add a new @entity_type.', ['@entity_type' => $bundle_entity_type_label]);
      $link_route_name = 'entity.' . $bundle_entity_type->id() . '.add_form';
      $build['#add_bundle_message'] = $this->t('There is no @entity_type yet. @add_link', [
        '@entity_type' => $bundle_entity_type_label,
        '@add_link' => Link::createFromRoute($link_text, $link_route_name)->toString(),
      ]);

Yes, it exists.

#37:

1. Ok, that makes sense! Good to have it documented here. Would be even better to have it documented in the code, though that's a nit.
2. Glad I just missed that.

LGTM.

+1 for RTBC

+++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php
@@ -0,0 +1,107 @@
+    // Check whether an entity of any bundle may be created.
+    foreach ($bundles as $bundle) {
+      $access = $access->orIf($access_control_handler->createAccess($bundle, $account, [], TRUE));
+      if ($access->isAllowed()) {
+        break;
+      }
+    }

Given Wim's comment I think we need to improve comment here.

Also I think we have another case to consider. What happens if a user can create bundles but does not has access to the any of the bundles already created?

Also I think we have another case to consider. What happens if a user can create bundles but does not has access to the any of the bundles already created?

That would be a new feature and I'd consider that to be highly irrelevant.

This is modelled after node/add and afaik, nobody ever cared about that scenario in the last 10 years :)

With node/add you get access if you have the ability to create bundles regardless of whether you have permissions to create nodes with any of the bundles.

Looks good, but we should expand the tests to also cover this.

Create yet another user, do not give him any permissions to create entity_test_with_bundle entities but give him permissions to create entity_test_bundle entities. He should be allowed to access the page and will not see any bundles but should see the message that he can create them.

Might need a change on EntityController::addPage() too, we need to check access before displaying the empty message.

Added tests mentioned in #44.

Great, thanks.

#43 and #44 look good. Let's get it in.

Thanks for addressing the odd edge case. This looks great.

Committed 180191f and pushed to 8.1.x and 8.2.x. Thanks!

diff --git a/core/modules/system/src/Tests/Entity/EntityAddUITest.php b/core/modules/system/src/Tests/Entity/EntityAddUITest.php
index 7d8cf9c..c5fe969 100644
--- a/core/modules/system/src/Tests/Entity/EntityAddUITest.php
+++ b/core/modules/system/src/Tests/Entity/EntityAddUITest.php
@@ -110,9 +110,9 @@ public function testAddPageWithBundleEntities() {
     ]);
     $this->drupalLogin($user);
 
-    // User has access to the add page but no bundles are shown because he does
-    // not have bundle specific permissions, the add bundle message is present
-    // as the user has bundle create permissions.
+    // User has access to the add page but no bundles are shown because the user
+    // does not have bundle specific permissions. The add bundle message is
+    // present as the user has bundle create permissions.
     $this->drupalGet('/entity_test_with_bundle/add');
     $this->assertNoLink('Test label');
     $this->assertNoLink('Test2 label');

I removed the gendered pronoun on commit as per our code documentation standards.

Quick followup patch.

@jibran I think it is best if we don't re-open issues for things like that. Also given the likelihood of re-introducing this codign standard regression I think we should just have a single issue were we re-file patches just to tidy this up so it doesn't become a commit credit thing.