From 4b2656695a3f4dbea35962fccb540fbd3fb91d2e Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Fri, 8 Apr 2016 13:47:07 +0100 Subject: [PATCH] Issue #2696669 by mbovan, Berdir, sanja_m: Incomplete/broken access checking in EntityController::addPage() --- core/core.services.yml | 5 + .../Entity/Controller/EntityController.php | 2 +- .../Entity/EntityCreateAnyAccessCheck.php | 109 ++++++++++++++++++ .../Routing/DefaultHtmlRouteProvider.php | 12 +- .../src/Tests/Entity/EntityAddUITest.php | 75 +++++++++--- .../entity_test/entity_test.permissions.yml | 5 + .../src/Entity/EntityTestBundle.php | 2 +- .../src/Entity/EntityTestWithBundle.php | 2 +- .../src/EntityTestAccessControlHandler.php | 7 +- .../entity_test/src/EntityTestPermissions.php | 57 +++++++++ .../Routing/DefaultHtmlRouteProviderTest.php | 12 +- 11 files changed, 264 insertions(+), 24 deletions(-) create mode 100644 core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php create mode 100644 core/modules/system/tests/modules/entity_test/src/EntityTestPermissions.php diff --git a/core/core.services.yml b/core/core.services.yml index 5e63508f6c..6a31a44f61 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -1069,6 +1069,11 @@ services: arguments: ['@entity.manager'] tags: - { name: access_check, applies_to: _entity_create_access } + access_check.entity_create_any: + class: Drupal\Core\Entity\EntityCreateAnyAccessCheck + arguments: ['@entity_type.manager', '@entity_type.bundle.info'] + tags: + - { name: access_check, applies_to: _entity_create_any_access } access_check.theme: class: Drupal\Core\Theme\ThemeAccessCheck arguments: ['@theme_handler'] diff --git a/core/lib/Drupal/Core/Entity/Controller/EntityController.php b/core/lib/Drupal/Core/Entity/Controller/EntityController.php index 5ae33ef626..0392fcf23e 100644 --- a/core/lib/Drupal/Core/Entity/Controller/EntityController.php +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php @@ -140,7 +140,7 @@ public function addPage($entity_type_id) { '@add_link' => Link::createFromRoute($link_text, $link_route_name)->toString(), ]); // Filter out the bundles the user doesn't have access to. - $access_control_handler = $this->entityTypeManager->getAccessControlHandler($bundle_entity_type_id); + $access_control_handler = $this->entityTypeManager->getAccessControlHandler($entity_type_id); foreach ($bundles as $bundle_name => $bundle_info) { $access = $access_control_handler->createAccess($bundle_name, NULL, [], TRUE); if (!$access->isAllowed()) { diff --git a/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php new file mode 100644 index 0000000000..7ca49c0fa0 --- /dev/null +++ b/core/lib/Drupal/Core/Entity/EntityCreateAnyAccessCheck.php @@ -0,0 +1,109 @@ +entityTypeManager = $entity_type_manager; + $this->entityTypeBundleInfo = $entity_type_bundle_info; + } + + /** + * Checks access to create an entity of any bundle for the given route. + * + * @param \Symfony\Component\Routing\Route $route + * The route to check against. + * @param \Drupal\Core\Routing\RouteMatchInterface $route_match + * The parameterized route. + * @param \Drupal\Core\Session\AccountInterface $account + * The currently logged in account. + * + * @return \Drupal\Core\Access\AccessResultInterface + * The access result. + */ + public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account) { + $entity_type_id = $route->getRequirement($this->requirementsKey); + $entity_type = $this->entityTypeManager->getDefinition($entity_type_id); + $access_control_handler = $this->entityTypeManager->getAccessControlHandler($entity_type_id); + + // In case there is no "bundle" entity key, check create access with no + // bundle specified. + if (!$entity_type->hasKey('bundle')) { + return $access_control_handler->createAccess(NULL, $account, [], TRUE); + } + + $access = AccessResult::neutral(); + $bundles = array_keys($this->entityTypeBundleInfo->getBundleInfo($entity_type_id)); + + // Include list cache tag as access might change if more bundles are added. + if ($entity_type->getBundleEntityType()) { + $access->addCacheTags($this->entityTypeManager->getDefinition($entity_type->getBundleEntityType())->getListCacheTags()); + + // Check if the user is allowed to create new bundles. If so, allow + // access, so the add page can show a link to create one. + // @see \Drupal\Core\Entity\Controller\EntityController::addPage() + $bundle_access_control_handler = $this->entityTypeManager->getAccessControlHandler($entity_type->getBundleEntityType()); + $access = $access->orIf($bundle_access_control_handler->createAccess(NULL, $account, [], TRUE)); + if ($access->isAllowed()) { + return $access; + } + } + + // Check whether an entity of any bundle may be created. + foreach ($bundles as $bundle) { + $access = $access->orIf($access_control_handler->createAccess($bundle, $account, [], TRUE)); + // In case there is a least one bundle user can create entities for, + // access is allowed. + if ($access->isAllowed()) { + break; + } + } + + return $access; + } + +} diff --git a/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php index 26258bd638..6b9c29fce0 100644 --- a/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php @@ -120,7 +120,7 @@ protected function getAddPageRoute(EntityTypeInterface $entity_type) { $route->setDefault('_controller', EntityController::class . '::addPage'); $route->setDefault('_title_callback', EntityController::class . '::addTitle'); $route->setDefault('entity_type_id', $entity_type->id()); - $route->setRequirement('_entity_create_access', $entity_type->id()); + $route->setRequirement('_entity_create_any_access', $entity_type->id()); return $route; } @@ -151,7 +151,11 @@ protected function getAddFormRoute(EntityTypeInterface $entity_type) { // If the entity has bundles, we can provide a bundle-specific title // and access requirements. - if ($bundle_key = $entity_type->getKey('bundle')) { + $expected_parameter = $entity_type->getBundleEntityType() ?: $entity_type->getKey('bundle'); + // @todo: We have to check if a route contains a bundle in its path as + // test entities have inconsistent usage of "add-form" link templates. + // Fix it in https://www.drupal.org/node/2699959. + if (($bundle_key = $entity_type->getKey('bundle')) && strpos($route->getPath(), '{' . $expected_parameter . '}') !== FALSE) { $route->setDefault('_title_callback', EntityController::class . '::addBundleTitle'); // If the bundles are entities themselves, we can add parameter // information to the route options. @@ -162,7 +166,7 @@ protected function getAddFormRoute(EntityTypeInterface $entity_type) { // The title callback uses the value of the bundle parameter to // fetch the respective bundle at runtime. ->setDefault('bundle_parameter', $bundle_entity_type_id) - ->setRequirement('_entity_create_access', "{$entity_type_id}:{$bundle_entity_type_id}"); + ->setRequirement('_entity_create_access', $entity_type_id . ':{' . $bundle_entity_type_id . '}'); // Entity types with serial IDs can specify this in their route // requirements, improving the matching process. @@ -185,7 +189,7 @@ protected function getAddFormRoute(EntityTypeInterface $entity_type) { // route parameter name directly. $route ->setDefault('bundle_parameter', $bundle_key) - ->setRequirement('_entity_create_access', "{$entity_type_id}:{$bundle_key}"); + ->setRequirement('_entity_create_access', $entity_type_id . ':{' . $bundle_key . '}'); } } else { diff --git a/core/modules/system/src/Tests/Entity/EntityAddUITest.php b/core/modules/system/src/Tests/Entity/EntityAddUITest.php index 0df247a47e..c5fe9691b9 100644 --- a/core/modules/system/src/Tests/Entity/EntityAddUITest.php +++ b/core/modules/system/src/Tests/Entity/EntityAddUITest.php @@ -25,24 +25,28 @@ class EntityAddUITest extends WebTestBase { public static $modules = ['entity_test']; /** - * {@inheritdoc} + * Tests the add page for an entity type using bundle entities. */ - protected function setUp() { - parent::setUp(); + public function testAddPageWithBundleEntities() { + $admin_user = $this->drupalCreateUser([ + 'administer entity_test_with_bundle content', + ]); + $this->drupalLogin($admin_user); - $web_user = $this->drupalCreateUser([ - "administer entity_test_with_bundle content", - "administer entity_test content", + // Users without create access for bundles do not have access to the add + // page if there are no bundles. + $this->drupalGet('/entity_test_with_bundle/add'); + $this->assertResponse(403); + + $bundle_admin_user = $this->drupalCreateUser([ + 'administer entity_test_with_bundle content', + 'administer entity_test_bundle content', ]); - $this->drupalLogin($web_user); - } + $this->drupalLogin($bundle_admin_user); - /** - * Tests the add page for an entity type using bundle entities. - */ - public function testAddPageWithBundleEntities() { + // No bundles exist, the add bundle message should be present as the user + // has the necessary permissions. $this->drupalGet('/entity_test_with_bundle/add'); - // No bundles exist, the add bundle message should be present. $this->assertText('There is no test entity bundle yet.'); $this->assertLink('Add a new test entity bundle.'); @@ -74,12 +78,57 @@ public function testAddPageWithBundleEntities() { $this->drupalPostForm(NULL, ['name[0][value]' => 'test name'], t('Save')); $entity = EntityTestWithBundle::load(1); $this->assertEqual('test name', $entity->label()); + + // Create a new user that only has bundle specific permissions. + $user = $this->drupalCreateUser([ + 'create test entity_test_with_bundle entities', + 'create test2 entity_test_with_bundle entities', + ]); + $this->drupalLogin($user); + + // Create a bundle that the user does not have create permissions for. + EntityTestBundle::create([ + 'id' => 'test3', + 'label' => 'Test3 label', + 'description' => 'My test3 description', + ])->save(); + $this->drupalGet('/entity_test_with_bundle/add'); + $this->assertLink('Test label'); + $this->assertLink('Test2 label'); + $this->assertNoLink('Test3 label'); + $this->clickLink(t('Test label')); + $this->assertResponse(200); + + // Without any permissions, access must be denied. + $this->drupalLogout(); + $this->drupalGet('/entity_test_with_bundle/add'); + $this->assertResponse(403); + + // Create a new user that has bundle create permissions. + $user = $this->drupalCreateUser([ + 'administer entity_test_bundle content', + ]); + $this->drupalLogin($user); + + // 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'); + $this->assertNoLink('Test3 label'); + $this->assertLink('Add a new test entity bundle.'); } /** * Tests the add page for an entity type not using bundle entities. */ public function testAddPageWithoutBundleEntities() { + $admin_user = $this->drupalCreateUser([ + 'administer entity_test content', + ]); + $this->drupalLogin($admin_user); + entity_test_create_bundle('test', 'Test label', 'entity_test_mul'); // Delete the default bundle, so that we can rely on our own. entity_test_delete_bundle('entity_test_mul', 'entity_test_mul'); diff --git a/core/modules/system/tests/modules/entity_test/entity_test.permissions.yml b/core/modules/system/tests/modules/entity_test/entity_test.permissions.yml index eb0d974acd..ca04459ad9 100644 --- a/core/modules/system/tests/modules/entity_test/entity_test.permissions.yml +++ b/core/modules/system/tests/modules/entity_test/entity_test.permissions.yml @@ -10,3 +10,8 @@ view test entity field: administer entity_test_with_bundle content: title: 'administer entity_test_with_bundle content' description: 'administer entity_test_with_bundle content' +administer entity_test_bundle content: + title: 'administer entity_test_bundle content' + +permission_callbacks: + - \Drupal\entity_test\EntityTestPermissions::entityTestBundlePermissions diff --git a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBundle.php b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBundle.php index f9abd9b963..5d7557ddf4 100644 --- a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBundle.php +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBundle.php @@ -25,7 +25,7 @@ * "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider", * }, * }, - * admin_permission = "administer entity_test_with_bundle content", + * admin_permission = "administer entity_test_bundle content", * config_prefix = "entity_test_bundle", * bundle_of = "entity_test_with_bundle", * entity_keys = { diff --git a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestWithBundle.php b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestWithBundle.php index c06c8e37f6..c78c4f88a1 100644 --- a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestWithBundle.php +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestWithBundle.php @@ -20,7 +20,7 @@ * handlers = { * "list_builder" = "Drupal\entity_test\EntityTestListBuilder", * "view_builder" = "Drupal\entity_test\EntityTestViewBuilder", - * "access" = "\Drupal\Core\Entity\EntityAccessControlHandler", + * "access" = "Drupal\entity_test\EntityTestAccessControlHandler", * "form" = { * "default" = "\Drupal\Core\Entity\ContentEntityForm", * "delete" = "\Drupal\Core\Entity\EntityDeleteForm" diff --git a/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php b/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php index 7988060450..cd6dd3138f 100644 --- a/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php @@ -22,6 +22,7 @@ * @see \Drupal\entity_test\Entity\EntityTestMul * @see \Drupal\entity_test\Entity\EntityTestMulRev * @see \Drupal\entity_test\Entity\EntityTestRev + * @see \Drupal\entity_test\Entity\EntityTestWithBundle * @see \Drupal\entity_test\Entity\EntityTestStringId */ class EntityTestAccessControlHandler extends EntityAccessControlHandler { @@ -68,7 +69,11 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter * {@inheritdoc} */ protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) { - return AccessResult::allowedIfHasPermission($account, 'administer entity_test content'); + return AccessResult::allowedIfHasPermissions($account, [ + 'administer entity_test content', + 'administer entity_test_with_bundle content', + 'create ' . $entity_bundle . ' entity_test_with_bundle entities', + ], 'OR'); } } diff --git a/core/modules/system/tests/modules/entity_test/src/EntityTestPermissions.php b/core/modules/system/tests/modules/entity_test/src/EntityTestPermissions.php new file mode 100644 index 0000000000..ab9f1faee3 --- /dev/null +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestPermissions.php @@ -0,0 +1,57 @@ +buildPermissions($type); + } + + return $perms; + } + + /** + * Returns a list of entity test permissions for a given entity test bundle. + * + * @param EntityTestBundle $type + * The entity test bundle. + * + * @return array + * An associative array of permission names and descriptions. + */ + protected function buildPermissions(EntityTestBundle $type) { + $type_id = $type->id(); + $type_params = ['%type_name' => $type->label()]; + + return [ + "create $type_id entity_test_with_bundle entities" => [ + 'title' => $this->t('%type_name: Create new entity', $type_params), + ], + ]; + } + +} diff --git a/core/tests/Drupal/Tests/Core/Entity/Routing/DefaultHtmlRouteProviderTest.php b/core/tests/Drupal/Tests/Core/Entity/Routing/DefaultHtmlRouteProviderTest.php index 54393d4450..7196652760 100644 --- a/core/tests/Drupal/Tests/Core/Entity/Routing/DefaultHtmlRouteProviderTest.php +++ b/core/tests/Drupal/Tests/Core/Entity/Routing/DefaultHtmlRouteProviderTest.php @@ -90,7 +90,7 @@ public function providerTestGetAddPageRoute() { '_title_callback' => 'Drupal\Core\Entity\Controller\EntityController::addTitle', 'entity_type_id' => 'the_entity_type_id', ]); - $route->setRequirement('_entity_create_access', 'the_entity_type_id'); + $route->setRequirement('_entity_create_any_access', 'the_entity_type_id'); $data['add_page'] = [clone $route, $entity_type3->reveal()]; return $data; @@ -119,9 +119,11 @@ public function providerTestGetAddFormRoute() { $entity_type1 = $this->getEntityType(); $entity_type1->hasLinkTemplate('add-form')->willReturn(FALSE); + $data['no_add_form_link_template'] = [NULL, $entity_type1->reveal()]; $entity_type2 = $this->getEntityType(); + $entity_type2->getBundleEntityType()->willReturn(NULL); $entity_type2->hasLinkTemplate('add-form')->willReturn(TRUE); $entity_type2->id()->willReturn('the_entity_type_id'); $entity_type2->getLinkTemplate('add-form')->willReturn('/the/add/form/link/template'); @@ -145,19 +147,23 @@ public function providerTestGetAddFormRoute() { $entity_type4 = $this->getEntityType($entity_type3); $entity_type4->getKey('bundle')->willReturn('the_bundle_key'); $entity_type4->getBundleEntityType()->willReturn(NULL); + $entity_type4->getLinkTemplate('add-form')->willReturn('/the/add/form/link/template/{the_bundle_key}'); + $route->setPath('/the/add/form/link/template/{the_bundle_key}'); $route ->setDefault('_title_callback', 'Drupal\Core\Entity\Controller\EntityController::addBundleTitle') ->setDefault('bundle_parameter', 'the_bundle_key') - ->setRequirement('_entity_create_access', 'the_entity_type_id:the_bundle_key'); + ->setRequirement('_entity_create_access', 'the_entity_type_id:{the_bundle_key}'); $data['add_form_bundle_static'] = [clone $route, $entity_type4->reveal()]; $entity_type5 = $this->getEntityType($entity_type4); $entity_type5->getBundleEntityType()->willReturn('the_bundle_entity_type_id'); + $entity_type5->getLinkTemplate('add-form')->willReturn('/the/add/form/link/template/{the_bundle_entity_type_id}'); $bundle_entity_type = $this->getEntityType(); $bundle_entity_type->isSubclassOf(FieldableEntityInterface::class)->willReturn(FALSE); + $route->setPath('/the/add/form/link/template/{the_bundle_entity_type_id}'); $route ->setDefault('bundle_parameter', 'the_bundle_entity_type_id') - ->setRequirement('_entity_create_access', 'the_entity_type_id:the_bundle_entity_type_id') + ->setRequirement('_entity_create_access', 'the_entity_type_id:{the_bundle_entity_type_id}') ->setOption('parameters', ['the_bundle_entity_type_id' => [ 'type' => 'entity:the_bundle_entity_type_id', ]]); -- GitLab