Issue #2847687 by jp.stacey, dawehner: EntityAccessCheck mandates instanceof...

Issue #2847687 by jp.stacey, dawehner: EntityAccessCheck mandates instanceof EntityInterface when it only needs AccessibleInterface

core/lib/Drupal/Core/Entity/EntityAccessCheck.php

@@ -16,28 +16,32 @@ class EntityAccessCheck implements AccessInterface {
   /**
    * Checks access to the entity operation on the given route.
    *
-   * The value of the '_entity_access' key must be in the pattern
-   * 'entity_slug_name.operation.' For example, this will check a node for
-   * 'update' access:
+   * The route's '_entity_access' requirement must follow the pattern
+   * 'entity_stub_name.operation', where available operations are:
+   * 'view', 'update', 'create', and 'delete'.
+   *
+   * For example, this route configuration invokes a permissions check for
+   * 'update' access to entities of type 'node':
    * @code
    * pattern: '/foo/{node}/bar'
    * requirements:
    *   _entity_access: 'node.update'
    * @endcode
-   * And this will check a dynamic entity type:
+   * And this will check 'delete' access to a dynamic entity type:
    * @code
    * example.route:
    *   path: foo/{entity_type}/{example}
    *   requirements:
-   *     _entity_access: example.update
+   *     _entity_access: example.delete
    *   options:
    *     parameters:
    *       example:
    *         type: entity:{entity_type}
    * @endcode
-   * @see \Drupal\Core\ParamConverter\EntityConverter
+   * The route match parameter corresponding to the stub name is checked to
+   * see if it is entity-like i.e. implements EntityInterface.
    *
-   * Available operations are 'view', 'update', 'create', and 'delete'.
+   * @see \Drupal\Core\ParamConverter\EntityConverter
    *
    * @param \Symfony\Component\Routing\Route $route
    *   The route to check against.
@@ -53,7 +57,7 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
     // Split the entity type and the operation.
     $requirement = $route->getRequirement('_entity_access');
     list($entity_type, $operation) = explode('.', $requirement);
-    // If there is valid entity of the given entity type, check its access.
+    // If $entity_type parameter is a valid entity, call its own access check.
     $parameters = $route_match->getParameters();
     if ($parameters->has($entity_type)) {
       $entity = $parameters->get($entity_type);

core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.php

@@ -9,8 +9,10 @@
 use Drupal\node\NodeInterface;
 use Symfony\Component\HttpFoundation\ParameterBag;
 use Symfony\Component\Routing\Route;
+use Drupal\Core\Access\AccessibleInterface;
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Entity\EntityAccessCheck;
+use Drupal\Core\Entity\EntityInterface;
 use Drupal\Tests\UnitTestCase;
 
 /**
@@ -70,13 +72,53 @@ public function testAccessWithTypePlaceholder() {
     $node = $node->reveal();
 
     /** @var \Drupal\Core\Routing\RouteMatchInterface|\Prophecy\Prophecy\ObjectProphecy $route_match */
-    $route_match = $this->prophesize(RouteMatchInterface::class);
-    $route_match->getRawParameters()->willReturn(new ParameterBag(['entity_type' => 'node', 'var_name' => 1]));
-    $route_match->getParameters()->willReturn(new ParameterBag(['entity_type' => 'node', 'var_name' => $node]));
-    $route_match = $route_match->reveal();
+    $route_match = $this->createRouteMatchForObject($node);
 
     $access_check = new EntityAccessCheck();
     $this->assertEquals(AccessResult::allowed(), $access_check->access($route, $route_match, $account));
   }
 
+  /**
+   * @covers ::access
+   */
+  public function testAccessWithDifferentRouteParameters() {
+    $route = new Route(
+      '/foo/{var_name}',
+      [],
+      ['_entity_access' => 'var_name.update'],
+      ['parameters' => ['var_name' => ['type' => 'entity:node']]]
+    );
+    /** @var \Drupal\Core\Session\AccountInterface $account */
+    $account = $this->prophesize(AccountInterface::class)->reveal();
+    $access_check = new EntityAccessCheck();
+
+    // Confirm an EntityInterface route parameter's ::access() is called.
+    /** @var \Drupal\Core\Entity\EntityInterface|\Prophecy\Prophecy\ObjectProphecy $node */
+    $node = $this->prophesize(EntityInterface::class);
+    $node->access('update', $account, TRUE)->willReturn(AccessResult::allowed());
+    $route_match = $this->createRouteMatchForObject($node->reveal());
+    $this->assertEquals(AccessResult::allowed(), $access_check->access($route, $route_match, $account));
+
+    // AccessibleInterface is not entity-like: ::access() should not be called.
+    /** @var \Drupal\Core\Access\AccessibleInterface|\Prophecy\Prophecy\ObjectProphecy $node */
+    $node = $this->prophesize(AccessibleInterface::class);
+    $node->access('update', $account, TRUE)->willReturn(AccessResult::allowed());
+    $route_match = $this->createRouteMatchForObject($node->reveal());
+    $this->assertEquals(AccessResult::neutral(), $access_check->access($route, $route_match, $account));
+  }
+
+  /**
+   * Wrap any object with a route match, and return that.
+   *
+   * @param \stdClass $object
+   *   Any object, including prophesized mocks based on interfaces.
+   * @return RouteMatchInterface
+   *   A prophesized RouteMatchInterface.
+   */
+  private function createRouteMatchForObject(\stdClass $object) {
+    $route_match = $this->prophesize(RouteMatchInterface::class);
+    $route_match->getRawParameters()->willReturn(new ParameterBag(['entity_type' => 'node', 'var_name' => 1]));
+    $route_match->getParameters()->willReturn(new ParameterBag(['entity_type' => 'node', 'var_name' => $object]));
+    return $route_match->reveal();
+  }
 }