diff --git a/core/lib/Drupal/Core/Access/AccessException.php b/core/lib/Drupal/Core/Access/AccessException.php new file mode 100644 index 00000000000..a44738e5ec2 --- /dev/null +++ b/core/lib/Drupal/Core/Access/AccessException.php @@ -0,0 +1,14 @@ +checks[$service_id]->access($route, $request, $account); + + if (!in_array($service_access, array(AccessInterface::ALLOW, AccessInterface::DENY, AccessInterface::KILL), TRUE)) { + throw new AccessException("Access error in $service_id. Access services can only return AccessInterface::ALLOW, AccessInterface::DENY, or AccessInterface::KILL constants."); + } + if ($service_access === AccessInterface::ALLOW) { $access = TRUE; } @@ -299,6 +304,11 @@ class AccessManager extends ContainerAware { } $service_access = $this->checks[$service_id]->access($route, $request, $account); + + if (!in_array($service_access, array(AccessInterface::ALLOW, AccessInterface::DENY, AccessInterface::KILL), TRUE)) { + throw new AccessException("Access error in $service_id. Access services can only return AccessInterface::ALLOW, AccessInterface::DENY, or AccessInterface::KILL constants."); + } + if ($service_access === AccessInterface::ALLOW) { $access = TRUE; } diff --git a/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php index 3322c9edc2f..23920ee12cc 100644 --- a/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php @@ -67,6 +67,8 @@ class FormModeAccessCheck implements StaticAccessCheckInterface { return $account->hasPermission($permission) ? static::ALLOW : static::DENY; } } + + return static::DENY; } } diff --git a/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php index 5e69daf8647..c1e653e8408 100644 --- a/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php @@ -67,6 +67,8 @@ class ViewModeAccessCheck implements StaticAccessCheckInterface { return $account->hasPermission($permission) ? static::ALLOW : static::DENY; } } + + return static::DENY; } } diff --git a/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php b/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php index 568c3891d64..2d3281bf9bc 100644 --- a/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php +++ b/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php @@ -136,7 +136,8 @@ class NodeRevisionAccessCheck implements AccessCheckInterface { if (!isset($this->access[$cid])) { // Perform basic permission checks first. if (!$account->hasPermission($map[$op]) && !$account->hasPermission($type_map[$op]) && !$account->hasPermission('administer nodes')) { - return $this->access[$cid] = FALSE; + $this->access[$cid] = FALSE; + return FALSE; } // There should be at least two revisions. If the vid of the given node diff --git a/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php b/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php index 18498f944e2..11f42c19337 100644 --- a/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php +++ b/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php @@ -30,7 +30,7 @@ class ViewOwnTrackerAccessCheck implements StaticAccessCheckInterface { public function access(Route $route, Request $request, AccountInterface $account) { // The user object from the User ID in the path. $user = $request->attributes->get('user'); - return $user && $account->isAuthenticated() && ($user->id() == $account->id()); + return ($user && $account->isAuthenticated() && ($user->id() == $account->id())) ? static::ALLOW : static::DENY; } } diff --git a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php index c7ae50a9b57..740191c4a37 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php @@ -11,10 +11,8 @@ use Drupal\Core\Access\AccessCheckInterface; use Drupal\Core\Routing\Access\AccessInterface; use Drupal\Core\Access\AccessManager; use Drupal\Core\Access\DefaultAccessCheck; -use Drupal\system\Tests\Routing\MockRouteProvider; use Drupal\Tests\UnitTestCase; use Drupal\router_test\Access\DefinedTestAccessCheck; -use Symfony\Cmf\Component\Routing\RouteObjectInterface; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Routing\Exception\RouteNotFoundException; @@ -468,6 +466,70 @@ class AccessManagerTest extends UnitTestCase { $this->assertFalse($this->accessManager->checkNamedRoute('test_route_1', array(), $this->account), 'A non existing route lead to access.'); } + /** + * Tests that an access checker throws an exception for not allowed values. + * + * @dataProvider providerCheckException + * + * @expectedException \Drupal\Core\Access\AccessException + */ + public function testCheckException($return_value, $access_mode) { + $route_provider = $this->getMock('Drupal\Core\Routing\RouteProviderInterface'); + + // Setup a test route for each access configuration. + $requirements = array( + '_test_incorrect_value' => 'TRUE', + ); + $options = array( + '_access_mode' => $access_mode, + '_access_checks' => array( + 'test_incorrect_value', + ), + ); + $route = new Route('', array(), $requirements, $options); + + $route_provider->expects($this->any()) + ->method('getRouteByName') + ->will($this->returnValue($route)); + + $request = new Request(); + + $container = new ContainerBuilder(); + + // Register a service that will return an incorrect value. + $access_check = $this->getMock('Drupal\Core\Routing\Access\AccessInterface'); + $access_check->expects($this->any()) + ->method('access') + ->will($this->returnValue($return_value)); + $container->set('test_incorrect_value', $access_check); + + $access_manager = new AccessManager($route_provider, $this->urlGenerator, $this->paramConverter); + $access_manager->setContainer($container); + $access_manager->addCheckService('test_incorrect_value'); + + $access_manager->checkNamedRoute('test_incorrect_value', array(), $this->account, $request); + } + + /** + * Data provider for testCheckException. + * + * @return array + */ + public function providerCheckException() { + return array( + array(array(), 'ALL'), + array(array(), 'ANY'), + array(array(1), 'ALL'), + array(array(1), 'ANY'), + array('string', 'ALL'), + array('string', 'ANY'), + array(0, 'ALL'), + array(0, 'ANY'), + array(1, 'ALL'), + array(1, 'ANY'), + ); + } + /** * Converts AccessCheckInterface constants to a string. * diff --git a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php index 7a5676b8c64..8144c237ec0 100644 --- a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php @@ -95,13 +95,13 @@ class CustomAccessCheckTest extends UnitTestCase { $route = new Route('/test-route', array(), array('_custom_access' => '\Drupal\Tests\Core\Access\TestController::accessDeny')); $account = $this->getMock('Drupal\Core\Session\AccountInterface'); - $this->assertNull($this->accessChecker->access($route, $request, $account)); + $this->assertSame(AccessInterface::DENY, $this->accessChecker->access($route, $request, $account)); $route = new Route('/test-route', array(), array('_custom_access' => '\Drupal\Tests\Core\Access\TestController::accessAllow')); - $this->assertTrue($this->accessChecker->access($route, $request, $account)); + $this->assertSame(AccessInterface::ALLOW, $this->accessChecker->access($route, $request, $account)); $route = new Route('/test-route', array('parameter' => 'TRUE'), array('_custom_access' => '\Drupal\Tests\Core\Access\TestController::accessParameter')); - $this->assertTrue($this->accessChecker->access($route, $request, $account)); + $this->assertSame(AccessInterface::ALLOW, $this->accessChecker->access($route, $request, $account)); } } diff --git a/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php index 89c845a51ea..a2fce189b86 100644 --- a/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php @@ -7,6 +7,7 @@ namespace Drupal\Tests\Core\Access; +use Drupal\Core\Access\AccessInterface; use Drupal\Core\Access\DefaultAccessCheck; use Drupal\Tests\UnitTestCase; use Symfony\Component\HttpFoundation\Request; @@ -66,13 +67,13 @@ class DefaultAccessCheckTest extends UnitTestCase { $request = new Request(array()); $route = new Route('/test-route', array(), array('_access' => 'NULL')); - $this->assertNull($this->accessChecker->access($route, $request, $this->account)); + $this->assertSame(AccessInterface::DENY, $this->accessChecker->access($route, $request, $this->account)); $route = new Route('/test-route', array(), array('_access' => 'FALSE')); - $this->assertFalse($this->accessChecker->access($route, $request, $this->account)); + $this->assertSame(AccessInterface::KILL, $this->accessChecker->access($route, $request, $this->account)); $route = new Route('/test-route', array(), array('_access' => 'TRUE')); - $this->assertTrue($this->accessChecker->access($route, $request, $this->account)); + $this->assertSame(AccessInterface::ALLOW, $this->accessChecker->access($route, $request, $this->account)); } }