diff --git a/core/lib/Drupal/Core/Access/AccessCheckInterface.php b/core/lib/Drupal/Core/Access/AccessCheckInterface.php index 9622295..172e095 100644 --- a/core/lib/Drupal/Core/Access/AccessCheckInterface.php +++ b/core/lib/Drupal/Core/Access/AccessCheckInterface.php @@ -16,19 +16,29 @@ interface AccessCheckInterface { /** - * @TODO + * Grant access. + * + * A checker should return this value to indicate that it grants access to a + * route. */ - const ACCEPT = TRUE; + const ALLOW = TRUE; /** - * @TODO + * Deny access. + * + * A checker should return this value to indicate it does not grant access to + * a route. */ - const KILL = FALSE; + const DENY = NULL; /** - * @TODO + * 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 DENY = NULL; + const KILL = FALSE; /** * Declares whether the access check applies to a specific route or not. diff --git a/core/lib/Drupal/Core/Access/AccessManager.php b/core/lib/Drupal/Core/Access/AccessManager.php index 8743664..3618631 100644 --- a/core/lib/Drupal/Core/Access/AccessManager.php +++ b/core/lib/Drupal/Core/Access/AccessManager.php @@ -139,7 +139,11 @@ protected function checkAnd(array $checks, Route $route, Request $request) { $access = FALSE; break; } - if ($service_access === AccessCheckInterface::ACCEPT) { + if ($service_access === AccessCheckInterface::DENY) { + $access = FALSE; + break; + } + if ($service_access === AccessCheckInterface::ALLOW) { $access = TRUE; } } @@ -170,8 +174,8 @@ protected function checkOr(array $checks, $route, $request) { } $service_access = $this->checks[$service_id]->access($route, $request); - if ($service_access === AccessCheckinterface::ACCEPT) { - return TRUE; + if ($service_access === AccessCheckinterface::ALLOW) { + $access = TRUE; } if ($service_access === AccessCheckInterface::KILL) { return FALSE; diff --git a/core/lib/Drupal/Core/Access/DefaultAccessCheck.php b/core/lib/Drupal/Core/Access/DefaultAccessCheck.php index 20f2f16..647bae5 100644 --- a/core/lib/Drupal/Core/Access/DefaultAccessCheck.php +++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.php @@ -27,7 +27,7 @@ public function applies(Route $route) { */ public function access(Route $route, Request $request) { if ($route->getRequirement('_access') === 'TRUE') { - return static::ACCEPT; + return static::ALLOW; } elseif ($route->getRequirement('_access') === 'FALSE') { return static::KILL; diff --git a/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/Access/DefinedTestAccessCheck.php b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/Access/DefinedTestAccessCheck.php index 77248dc..f2cc4d9 100644 --- a/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/Access/DefinedTestAccessCheck.php +++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/Access/DefinedTestAccessCheck.php @@ -27,7 +27,15 @@ public function applies(Route $route) { * {@inheritdoc} */ public function access(Route $route, Request $request) { - return $route->getRequirement('_test_access') === 'TRUE' ?: NULL; + if ($route->getRequirement('_test_access') === 'TRUE') { + return static::ALLOW; + } + elseif ($route->getRequirement('_test_access') === 'FALSE') { + return static::KILL; + } + else { + return static::DENY; + } } } diff --git a/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php index 2b3fc59..519ba86 100644 --- a/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php +++ b/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) ? static::ACCEPT : static::DENY; + return user_access($permission) ? static::ALLOW : static::DENY; } } diff --git a/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php index 9e50de7..729f4b8 100644 --- a/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php +++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php @@ -42,18 +42,18 @@ public function access(Route $route, Request $request) { if (count($explode_and) > 1) { $diff = array_diff($explode_and, array_keys($account->roles)); if (empty($diff)) { - return static::ACCEPT; + return static::ALLOW; } } else { $explode_or = array_filter(array_map('trim', explode(',', $rid_string))); $intersection = array_intersect($explode_or, array_keys($account->roles)); if (!empty($intersection)) { - return static::ACCEPT; + return static::ALLOW; } } - // If there is no allowed role, return DENY to give other checks a chance. + // Do not allow access if the user does not have the necessary role. return static::DENY; } diff --git a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php index 9ee2852..5a952e7 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php @@ -7,6 +7,7 @@ namespace Drupal\Tests\Core\Access; +use Drupal\Core\Access\AccessCheckInterface; use Drupal\Core\Access\AccessManager; use Drupal\Core\Access\DefaultAccessCheck; use Drupal\Tests\UnitTestCase; @@ -126,57 +127,107 @@ public function testCheckConjunctions() { $request = new Request(); - $this->routeCollection->add('test_route_4', new Route('/test-route-4', array(), array( - '_access' => 'TRUE', - '_test_access' => 'TRUE', - ))); - - $this->routeCollection->add('test_route_5', new Route('/test-route-5', array(), array( - '_access' => 'TRUE', - '_test_access' => 'TRUE', - )), array('_access_conjunction' => 'OR')); - - $this->routeCollection->add('test_route_6', new Route('/test-route-6', array(), array( - '_access' => 'TRUE', - '_test_access' => 'TRUE', - )), array('_access_conjunction' => 'AND')); - - $this->routeCollection->add('test_route_7', new Route('/test-route-7', array(), array( - '_access' => 'FALSE', - '_test_access' => 'TRUE', - )), array('_access_conjunction' => 'OR')); - - $this->routeCollection->add('test_route_8', new Route('/test-route-8', array(), array( - '_access' => 'FALSE', - '_test_access' => 'TRUE', - )), array('_access_conjunction' => 'AND')); - - $this->routeCollection->add('test_route_9', new Route('/test-route-9', array(), array( - '_access' => 'NULL', - '_test_access' => 'TRUE', - )), array('_access_conjunction' => 'AND')); - - $this->routeCollection->add('test_route_10', new Route('/test-route-10', array(), array( - '_access' => 'NULL', - '_test_access' => 'TRUE', - )), array('_access_conjunction' => 'OR')); - - $this->routeCollection->add('test_route_11', new Route('/test-route-11', array(), array( - '_access' => 'NULL', - '_test_access' => 'FALSE', - )), array('_access_conjunction' => 'OR')); - - $this->accessManager->setChecks($this->routeCollection); + $access_configurations = array(); + $access_configurations[] = array( + 'conjunction' => 'AND', + 'name' => 'test_route_4', + 'condition_one' => AccessCheckInterface::ALLOW, + 'condition_two' => AccessCheckInterface::KILL, + 'expected' => FALSE, + ); + $access_configurations[] = array( + 'conjunction' => 'AND', + 'name' => 'test_route_5', + 'condition_one' => AccessCheckInterface::ALLOW, + 'condition_two' => AccessCheckInterface::DENY, + 'expected' => FALSE, + ); + $access_configurations[] = array( + 'conjunction' => 'AND', + 'name' => 'test_route_6', + 'condition_one' => AccessCheckInterface::KILL, + 'condition_two' => AccessCheckInterface::DENY, + 'expected' => FALSE, + ); + $access_configurations[] = array( + 'conjunction' => 'AND', + 'name' => 'test_route_7', + 'condition_one' => AccessCheckInterface::ALLOW, + 'condition_two' => AccessCheckInterface::ALLOW, + 'expected' => TRUE, + ); + $access_configurations[] = array( + 'conjunction' => 'AND', + 'name' => 'test_route_8', + 'condition_one' => AccessCheckInterface::KILL, + 'condition_two' => AccessCheckInterface::KILL, + 'expected' => FALSE, + ); + $access_configurations[] = array( + 'conjunction' => 'AND', + 'name' => 'test_route_9', + 'condition_one' => AccessCheckInterface::DENY, + 'condition_two' => AccessCheckInterface::DENY, + 'expected' => FALSE, + ); + $access_configurations[] = array( + 'conjunction' => 'OR', + 'name' => 'test_route_10', + 'condition_one' => AccessCheckInterface::ALLOW, + 'condition_two' => AccessCheckInterface::KILL, + 'expected' => FALSE, + ); + $access_configurations[] = array( + 'conjunction' => 'OR', + 'name' => 'test_route_11', + 'condition_one' => AccessCheckInterface::ALLOW, + 'condition_two' => AccessCheckInterface::DENY, + 'expected' => TRUE, + ); + $access_configurations[] = array( + 'conjunction' => 'OR', + 'name' => 'test_route_12', + 'condition_one' => AccessCheckInterface::KILL, + 'condition_two' => AccessCheckInterface::DENY, + 'expected' => FALSE, + ); + $access_configurations[] = array( + 'conjunction' => 'OR', + 'name' => 'test_route_13', + 'condition_one' => AccessCheckInterface::ALLOW, + 'condition_two' => AccessCheckInterface::ALLOW, + 'expected' => TRUE, + ); + $access_configurations[] = array( + 'conjunction' => 'OR', + 'name' => 'test_route_14', + 'condition_one' => AccessCheckInterface::KILL, + 'condition_two' => AccessCheckInterface::KILL, + 'expected' => FALSE, + ); + $access_configurations[] = array( + 'conjunction' => 'OR', + 'name' => 'test_route_15', + 'condition_one' => AccessCheckInterface::DENY, + 'condition_two' => AccessCheckInterface::DENY, + 'expected' => FALSE, + ); $expected = array(); - $expected['test_route_4'] = TRUE; - $expected['test_route_5'] = TRUE; - $expected['test_route_6'] = TRUE; - $expected['test_route_7'] = FALSE; - $expected['test_route_8'] = FALSE; - $expected['test_route_9'] = TRUE; - $expected['test_route_10'] = TRUE; - $expected['test_route_11'] = FALSE; + foreach ($access_configurations as $config) { + // Setup a test route for each access configuration. + $name = $config['name']; + $requirements = array( + '_access' => static::convertAccessCheckInterfaceToString($config['condition_one']), + '_test_access' => static::convertAccessCheckInterfaceToString($config['condition_two']), + ); + $options = array('_access_conjunction' => $config['conjunction']); + $this->routeCollection->add($name, new Route($name, array(), $requirements, $options)); + + $expected[$name] = $config['expected']; + } + + $this->accessManager->setChecks($this->routeCollection); foreach ($expected as $route_name => $expected_access) { $this->assertSame($this->accessManager->check($this->routeCollection->get($route_name), $request), $expected_access); @@ -184,6 +235,30 @@ public function testCheckConjunctions() { } /** + * Little helper function to convert AccessCheckInterface constants to 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() {