Issue #2204363 by Berdir, Wim Leers, tim.plunkett, chx: [sechole] Returning...

Issue #2204363 by Berdir, Wim Leers, tim.plunkett, chx: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess()

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);

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.
    */

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);

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');