From f4108f88bff84e742a634d647adcf4c6e3a60bf7 Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Sat, 13 Dec 2014 17:29:19 +0100
Subject: [PATCH] Issue #2204363 by Berdir, Wim Leers, tim.plunkett, chx:
[sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access()
must not bypass EntityAccessController::checkAccess()
---
.../Entity/EntityAccessControlHandler.php | 14 ++++++----
.../Entity/EntityAccessControlHandlerTest.php | 28 +++++++++++++++++++
.../modules/entity_test/entity_test.module | 7 +++++
.../src/EntityTestAccessControlHandler.php | 6 ++++
4 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
index 3def84ac573c..0994cba0aa10 100644
--- a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
+++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
@@ -76,9 +76,10 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
);
$return = $this->processAccessHookResults($access);
- if ($return->isNeutral()) {
- // No module had an opinion about the access, so let's the access
- // handler check access.
+
+ // Also execute the default access check except when the access result is
+ // already forbidden, as in that case, it can not be anything else.
+ if (!$return->isForbidden()) {
$return = $return->orIf($this->checkAccess($entity, $operation, $langcode, $account));
}
$result = $this->setCache($return, $entity->uuid(), $operation, $langcode, $account);
@@ -231,9 +232,10 @@ public function createAccess($entity_bundle = NULL, AccountInterface $account =
);
$return = $this->processAccessHookResults($access);
- if ($return->isNeutral()) {
- // No module had an opinion about the access, so let's the access
- // handler check create access.
+
+ // Also execute the default access check except when the access result is
+ // already forbidden, as in that case, it can not be anything else.
+ if (!$return->isForbidden()) {
$return = $return->orIf($this->checkCreateAccess($account, $context, $entity_bundle));
}
$result = $this->setCache($return, $cid, 'create', $context['langcode'], $account);
diff --git a/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php b/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
index 76e87f4ebd6d..d11a9454fd55 100644
--- a/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
+++ b/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
@@ -66,6 +66,34 @@ function testEntityAccess() {
), $entity, $custom_user);
}
+ /**
+ * Ensures default entity access is checked when necessary.
+ *
+ * This ensures that the default checkAccess() implementation of the
+ * entity access control handler is considered if hook_entity_access() has not
+ * explicitly forbidden access. Therefore the default checkAccess()
+ * implementation can forbid access, even after access was already explicitly
+ * allowed by hook_entity_access().
+ *
+ * @see \Drupal\entity_test\EntityTestAccessControlHandler::checkAccess()
+ * @see entity_test_entity_access()
+ */
+ function testDefaultEntityAccess() {
+ // Set up a non-admin user that is allowed to view test entities.
+ \Drupal::currentUser()->setAccount($this->createUser(array('uid' => 2), array('view test entity')));
+ $entity = entity_create('entity_test', array(
+ 'name' => 'forbid_access',
+ ));
+
+ // The user is denied access to the entity.
+ $this->assertEntityAccess(array(
+ 'create' => FALSE,
+ 'update' => FALSE,
+ 'delete' => FALSE,
+ 'view' => FALSE,
+ ), $entity);
+ }
+
/**
* Ensures that the default handler is used as a fallback.
*/
diff --git a/core/modules/system/tests/modules/entity_test/entity_test.module b/core/modules/system/tests/modules/entity_test/entity_test.module
index 3e7cb73f2354..08fa56444074 100644
--- a/core/modules/system/tests/modules/entity_test/entity_test.module
+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -497,6 +497,13 @@ function entity_test_entity_prepare_view($entity_type, array $entities, array $d
function entity_test_entity_access(EntityInterface $entity, $operation, AccountInterface $account, $langcode) {
\Drupal::state()->set('entity_test_entity_access', TRUE);
+ // Attempt to allow access to entities with the title forbid_access,
+ // this will be overridden by
+ // \Drupal\entity_test\EntityTestAccessControlHandler::checkAccess().
+ if ($entity->label() == 'forbid_access') {
+ return AccessResult::allowed();
+ }
+
// Uncacheable because the access result depends on a State key-value pair and
// might therefore change at any time.
$condition = \Drupal::state()->get("entity_test_entity_access.{$operation}." . $entity->id(), FALSE);
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 2ae39ea556ff..14141273ad2b 100644
--- a/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php
+++ b/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php
@@ -30,6 +30,12 @@ class EntityTestAccessControlHandler extends EntityAccessControlHandler {
* {@inheritdoc}
*/
protected function checkAccess(EntityInterface $entity, $operation, $langcode, AccountInterface $account) {
+ // Always forbid access to entities with the label 'forbid_access', used for
+ // \Drupal\system\Tests\Entity\EntityAccessHControlandlerTest::testDefaultEntityAccess().
+ if ($entity->label() == 'forbid_access') {
+ return AccessResult::forbidden();
+ }
+
if ($operation === 'view') {
if ($langcode != LanguageInterface::LANGCODE_DEFAULT) {
return AccessResult::allowedIfHasPermission($account, 'view test entity translations');
--
GitLab