Issue #1984528 by dawehner, ParisLiakos: Allow for more robust access checking.

core/lib/Drupal/Core/Access/AccessCheckInterface.php

@@ -15,6 +15,31 @@
  */
 interface AccessCheckInterface {
 
+  /**
+   * Grant access.
+   *
+   * A checker should return this value to indicate that it grants access to a
+   * route.
+   */
+  const ALLOW = TRUE;
+
+  /**
+   * Deny access.
+   *
+   * A checker should return this value to indicate it does not grant access to
+   * a route.
+   */
+  const DENY = NULL;
+
+  /**
+   * Block access.
+   *
+   * A checker should return this value to indicate that it wants to completely
+   * block access to this route, regardless of any other access checkers. Most
+   * checkers should prefer DENY.
+   */
+  const KILL = FALSE;
+
   /**
    * Declares whether the access check applies to a specific route or not.
    *

core/lib/Drupal/Core/Access/AccessManager.php

@@ -14,6 +14,8 @@
 
 /**
  * Attaches access check services to routes and runs them on request.
+ *
+ * @see \Drupal\Tests\Core\Access\AccessManagerTest
  */
 class AccessManager extends ContainerAware {
 
@@ -22,7 +24,7 @@ class AccessManager extends ContainerAware {
    *
    * @var array
    */
-  protected $checkIds;
+  protected $checkIds = array();
 
   /**
    * Array of access check objects keyed by service id.
@@ -89,32 +91,92 @@ protected function applies(Route $route) {
    *
    * @param \Symfony\Component\Routing\Route $route
    *   The route to check access to.
-   * @param \Symfony\Commponent\HttpFoundation\Request $request
+   * @param \Symfony\Component\HttpFoundation\Request $request
    *   The incoming request object.
    *
+   * @return bool
+   *  Returns TRUE if the user has access to the route, otherwise FALSE.
+   *
    * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    *   If any access check denies access or none explicitly approve.
    */
   public function check(Route $route, Request $request) {
-    $access = FALSE;
     $checks = $route->getOption('_access_checks') ?: array();
 
-    // No checks == deny by default.
+    $conjunction = $route->getOption('_access_mode') ?: 'ANY';
+
+    if ($conjunction == 'ALL') {
+      return $this->checkAll($checks, $route, $request);
+    }
+    else {
+      return $this->checkAny($checks, $route, $request);
+    }
+  }
+
+  /**
+   * Checks access so that every checker should allow access.
+   *
+   * @param array $checks
+   *   Contains the list of checks on the route definition.
+   * @param \Symfony\Component\Routing\Route $route
+   *   The route to check access to.
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The incoming request object.
+   *
+   * @return bool
+   *  Returns TRUE if the user has access to the route, else FALSE.
+   */
+  protected function checkAll(array $checks, Route $route, Request $request) {
+    $access = FALSE;
+
     foreach ($checks as $service_id) {
       if (empty($this->checks[$service_id])) {
         $this->loadCheck($service_id);
       }
 
       $service_access = $this->checks[$service_id]->access($route, $request);
-      if ($service_access === FALSE) {
-        // A check has denied access, no need to continue checking.
+      if ($service_access === AccessCheckInterface::ALLOW) {
+        $access = TRUE;
+      }
+      else {
+        // On both KILL and DENY stop.
         $access = FALSE;
         break;
       }
-      elseif ($service_access === TRUE) {
-        // A check has explicitly granted access, so we need to remember that.
+    }
+
+    return $access;
+  }
+
+  /**
+   * Checks access so that at least one checker should allow access.
+   *
+   * @param array $checks
+   *   Contains the list of checks on the route definition.
+   * @param \Symfony\Component\Routing\Route $route
+   *   The route to check access to.
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The incoming request object.
+   *
+   * @return bool
+   *  Returns TRUE if the user has access to the route, else FALSE.
+   */
+  protected function checkAny(array $checks, $route, $request) {
+    // No checks == deny by default.
+    $access = FALSE;
+
+    foreach ($checks as $service_id) {
+      if (empty($this->checks[$service_id])) {
+        $this->loadCheck($service_id);
+      }
+
+      $service_access = $this->checks[$service_id]->access($route, $request);
+      if ($service_access === AccessCheckinterface::ALLOW) {
         $access = TRUE;
       }
+      if ($service_access === AccessCheckInterface::KILL) {
+        return FALSE;
+      }
     }
 
     return $access;

core/lib/Drupal/Core/Access/DefaultAccessCheck.php

@@ -23,9 +23,18 @@ public function applies(Route $route) {
   }
 
   /**
-   * Implements AccessCheckInterface::access().
+   * {@inheritdoc}
    */
   public function access(Route $route, Request $request) {
-    return $route->getRequirement('_access') === 'TRUE';
+    if ($route->getRequirement('_access') === 'TRUE') {
+      return static::ALLOW;
+    }
+    elseif ($route->getRequirement('_access') === 'FALSE') {
+      return static::KILL;
+    }
+    else {
+      return static::DENY;
+    }
   }
+
 }

core/modules/system/tests/modules/router_test/lib/Drupal/router_test/Access/DefinedTestAccessCheck.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\router_test\Access\DefinedTestAccessCheck.
+ */
+
+namespace Drupal\router_test\Access;
+
+use Drupal\Core\Access\AccessCheckInterface;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\Route;
+
+/**
+ * Defines an access checker similar to DefaultAccessCheck
+ */
+class DefinedTestAccessCheck implements AccessCheckInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function applies(Route $route) {
+    return array_key_exists('_test_access', $route->getRequirements());
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function access(Route $route, Request $request) {
+    if ($route->getRequirement('_test_access') === 'TRUE') {
+      return static::ALLOW;
+    }
+    elseif ($route->getRequirement('_test_access') === 'FALSE') {
+      return static::KILL;
+    }
+    else {
+      return static::DENY;
+    }
+  }
+
+}

core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php

@@ -31,6 +31,6 @@ public function access(Route $route, Request $request) {
     // @todo Replace user_access() with a correctly injected and session-using
     //   alternative.
     // If user_access() fails, return NULL to give other checks a chance.
-    return user_access($permission) ? TRUE : NULL;
+    return user_access($permission) ? static::ALLOW : static::DENY;
   }
 }

core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php

@@ -42,19 +42,19 @@ public function access(Route $route, Request $request) {
     if (count($explode_and) > 1) {
       $diff = array_diff($explode_and, $account->roles);
       if (empty($diff)) {
-        return TRUE;
+        return static::ALLOW;
       }
     }
     else {
       $explode_or = array_filter(array_map('trim', explode(',', $rid_string)));
       $intersection = array_intersect($explode_or, $account->roles);
       if (!empty($intersection)) {
-        return TRUE;
+        return static::ALLOW;
       }
     }
 
     // If there is no allowed role, return NULL to give other checks a chance.
-    return NULL;
+    return static::DENY;
   }
 
 }

core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Tests\Core\Access\AccessManagerTest.
+ */
+
+namespace Drupal\Tests\Core\Access;
+
+use Drupal\Core\Access\AccessCheckInterface;
+use Drupal\Core\Access\AccessManager;
+use Drupal\Core\Access\DefaultAccessCheck;
+use Drupal\Tests\UnitTestCase;
+use Drupal\router_test\Access\DefinedTestAccessCheck;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\Route;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * Tests the access manager.
+ *
+ * @see \Drupal\Core\Access\AccessManager
+ */
+class AccessManagerTest extends UnitTestCase {
+
+  /**
+   * The dependency injection container.
+   *
+   * @var \Symfony\Component\DependencyInjection\ContainerBuilder
+   */
+  protected $container;
+
+  /**
+   * The collection of routes, which are tested.
+   *
+   * @var \Symfony\Component\Routing\RouteCollection
+   */
+  protected $routeCollection;
+
+  /**
+   * The access manager to test.
+   *
+   * @var \Drupal\Core\Access\AccessManager
+   */
+  protected $accessManager;
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Access manager tests',
+      'description' => 'Test for the AccessManager object.',
+      'group' => 'Routing',
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    $this->container = new ContainerBuilder();
+    $this->accessManager = new AccessManager();
+    $this->accessManager->setContainer($this->container);
+
+    $this->routeCollection = new RouteCollection();
+    $this->routeCollection->add('test_route_1', new Route('/test-route-1'));
+    $this->routeCollection->add('test_route_2', new Route('/test-route-2', array(), array('_access' => 'TRUE')));
+    $this->routeCollection->add('test_route_3', new Route('/test-route-3', array(), array('_access' => 'FALSE')));
+  }
+
+  /**
+   * Tests \Drupal\Core\Access\AccessManager::setChecks().
+   */
+  public function testSetChecks() {
+    // Check setChecks without any access checker defined yet.
+    $this->accessManager->setChecks($this->routeCollection);
+
+    foreach ($this->routeCollection->all() as $route) {
+      $this->assertNull($route->getOption('_access_checks'));
+    }
+
+
+    $this->setupAccessChecker();
+
+    $this->accessManager->setChecks($this->routeCollection);
+
+    $this->assertEquals($this->routeCollection->get('test_route_1')->getOption('_access_checks'), NULL);
+    $this->assertEquals($this->routeCollection->get('test_route_2')->getOption('_access_checks'), array('test_access_default'));
+    $this->assertEquals($this->routeCollection->get('test_route_3')->getOption('_access_checks'), array('test_access_default'));
+  }
+
+  /**
+   * Tests \Drupal\Core\Access\AccessManager::check().
+   */
+  public function testCheck() {
+    $request = new Request();
+
+    // Check check without any access checker defined yet.
+    foreach ($this->routeCollection->all() as $route) {
+      $this->assertFalse($this->accessManager->check($route, $request));
+    }
+
+    $this->setupAccessChecker();
+
+    // An access checker got setup, but the routes haven't been setup using
+    // setChecks.
+    foreach ($this->routeCollection->all() as $route) {
+      $this->assertFalse($this->accessManager->check($route, $request));
+    }
+
+    $this->accessManager->setChecks($this->routeCollection);
+
+    $this->assertFalse($this->accessManager->check($this->routeCollection->get('test_route_1'), $request));
+    $this->assertTrue($this->accessManager->check($this->routeCollection->get('test_route_2'), $request));
+    $this->assertFalse($this->accessManager->check($this->routeCollection->get('test_route_3'), $request));
+  }
+
+  /**
+   * Provides data for the conjunction test.
+   *
+   * @return array
+   *   An array of data for check conjunctions.
+   *
+   * @see \Drupal\Tests\Core\Access\AccessManagerTest::testCheckConjunctions()
+   */
+  public function providerTestCheckConjunctions() {
+    $access_configurations = array();
+    $access_configurations[] = array(
+      'conjunction' => 'ALL',
+      'name' => 'test_route_4',
+      'condition_one' => AccessCheckInterface::ALLOW,
+      'condition_two' => AccessCheckInterface::KILL,
+      'expected' => FALSE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ALL',
+      'name' => 'test_route_5',
+      'condition_one' => AccessCheckInterface::ALLOW,
+      'condition_two' => AccessCheckInterface::DENY,
+      'expected' => FALSE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ALL',
+      'name' => 'test_route_6',
+      'condition_one' => AccessCheckInterface::KILL,
+      'condition_two' => AccessCheckInterface::DENY,
+      'expected' => FALSE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ALL',
+      'name' => 'test_route_7',
+      'condition_one' => AccessCheckInterface::ALLOW,
+      'condition_two' => AccessCheckInterface::ALLOW,
+      'expected' => TRUE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ALL',
+      'name' => 'test_route_8',
+      'condition_one' => AccessCheckInterface::KILL,
+      'condition_two' => AccessCheckInterface::KILL,
+      'expected' => FALSE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ALL',
+      'name' => 'test_route_9',
+      'condition_one' => AccessCheckInterface::DENY,
+      'condition_two' => AccessCheckInterface::DENY,
+      'expected' => FALSE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ANY',
+      'name' => 'test_route_10',
+      'condition_one' => AccessCheckInterface::ALLOW,
+      'condition_two' => AccessCheckInterface::KILL,
+      'expected' => FALSE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ANY',
+      'name' => 'test_route_11',
+      'condition_one' => AccessCheckInterface::ALLOW,
+      'condition_two' => AccessCheckInterface::DENY,
+      'expected' => TRUE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ANY',
+      'name' => 'test_route_12',
+      'condition_one' => AccessCheckInterface::KILL,
+      'condition_two' => AccessCheckInterface::DENY,
+      'expected' => FALSE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ANY',
+      'name' => 'test_route_13',
+      'condition_one' => AccessCheckInterface::ALLOW,
+      'condition_two' => AccessCheckInterface::ALLOW,
+      'expected' => TRUE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ANY',
+      'name' => 'test_route_14',
+      'condition_one' => AccessCheckInterface::KILL,
+      'condition_two' => AccessCheckInterface::KILL,
+      'expected' => FALSE,
+    );
+    $access_configurations[] = array(
+      'conjunction' => 'ANY',
+      'name' => 'test_route_15',
+      'condition_one' => AccessCheckInterface::DENY,
+      'condition_two' => AccessCheckInterface::DENY,
+      'expected' => FALSE,
+    );
+
+    return $access_configurations;
+  }
+
+  /**
+   * Test \Drupal\Core\Access\AccessManager::check() with conjunctions.
+   *
+   * @dataProvider providerTestCheckConjunctions
+   */
+  public function testCheckConjunctions($conjunction, $name, $condition_one, $condition_two, $expected_access) {
+    $this->setupAccessChecker();
+    $access_check = new DefinedTestAccessCheck();
+    $this->container->register('test_access_defined', $access_check);
+    $this->accessManager->addCheckService('test_access_defined');
+
+    $request = new Request();
+
+    $route_collection = new RouteCollection();
+    // Setup a test route for each access configuration.
+    $requirements = array(
+      '_access' => static::convertAccessCheckInterfaceToString($condition_one),
+      '_test_access' => static::convertAccessCheckInterfaceToString($condition_two),
+    );
+    $options = array('_access_mode' => $conjunction);
+    $route = new Route($name, array(), $requirements, $options);
+    $route_collection->add($name, $route);
+
+    $this->accessManager->setChecks($route_collection);
+    $this->assertSame($this->accessManager->check($route, $request), $expected_access);
+  }
+
+  /**
+   * Converts AccessCheckInterface constants to a string.
+   *
+   * @param mixed $constant
+   *   The access constant which is tested, so either
+   *   AccessCheckInterface::ALLOW, AccessCheckInterface::DENY OR
+   *   AccessCheckInterface::KILL.
+   *
+   * @return string
+   *   The corresponding string used in route requirements, so 'TRUE', 'FALSE'
+   *   or 'NULL'.
+   */
+  protected static function convertAccessCheckInterfaceToString($constant) {
+    if ($constant === AccessCheckInterface::ALLOW) {
+      return 'TRUE';
+    }
+    if ($constant === AccessCheckInterface::DENY) {
+      return 'NULL';
+    }
+    if ($constant === AccessCheckInterface::KILL) {
+      return 'FALSE';
+    }
+  }
+
+  /**
+   * Adds a default access check service to the container and the access manager.
+   */
+  protected function setupAccessChecker() {
+    $access_check = new DefaultAccessCheck();
+    $this->container->register('test_access_default', $access_check);
+    $this->accessManager->addCheckService('test_access_default');
+  }
+
+}

core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php

@@ -62,13 +62,15 @@ public function testApplies() {
    * Test the access method.
    */
   public function testAccess() {
-    $route = new Route('/test-route');
     $request = new Request(array());
 
-    $route->addRequirements(array('_access' => 'FALSE'));
+    $route = new Route('/test-route', array(), array('_access' => 'NULL'));
+    $this->assertNull($this->accessChecker->access($route, $request));
+
+    $route = new Route('/test-route', array(), array('_access' => 'FALSE'));
     $this->assertFalse($this->accessChecker->access($route, $request));
 
-    $route->addRequirements(array('_access' => 'TRUE'));
+    $route = new Route('/test-route', array(), array('_access' => 'TRUE'));
     $this->assertTrue($this->accessChecker->access($route, $request));
   }
 

core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\Tests\Core\Route;
 
+use Drupal\Core\Access\AccessCheckInterface;
 use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\Tests\UnitTestCase;
 use Drupal\user\Access\RoleAccessCheck;
@@ -163,7 +164,7 @@ public function testRoleAccess($path, $grant_accounts, $deny_accounts) {
 
       $subrequest = Request::create($path, 'GET');
       $message = sprintf('Access granted for user with the roles %s on path: %s', implode(', ', $account->roles), $path);
-      $this->assertTrue($role_access_check->access($collection->get($path), $subrequest), $message);
+      $this->assertSame(AccessCheckInterface::ALLOW, $role_access_check->access($collection->get($path), $subrequest), $message);
     }
 
     // Check all users which don't have access.
@@ -173,7 +174,7 @@ public function testRoleAccess($path, $grant_accounts, $deny_accounts) {
       $subrequest = Request::create($path, 'GET');
       $message = sprintf('Access denied for user %s with the roles %s on path: %s', $account->uid, implode(', ', $account->roles), $path);
       $has_access = $role_access_check->access($collection->get($path), $subrequest);
-      $this->assertEmpty($has_access , $message);
+      $this->assertSame(AccessCheckInterface::DENY, $has_access , $message);
     }
   }