Issue #2108829 by damiankloip, dawehner: Make AccessManager stricter with values returned from access checkers.

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

@@ -0,0 +1,14 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Access\AccessException.
+ */
+
+namespace Drupal\Core\Access;
+
+/**
+ * An exception thrown for invalid access callback return values.
+ */
+class AccessException extends \RuntimeException {
+}

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

@@ -20,14 +20,14 @@ interface AccessInterface {
    *
    * A checker should return this value to indicate that it grants access.
    */
-  const ALLOW = TRUE;
+  const ALLOW = 'ALLOW';
 
   /**
    * Deny access.
    *
    * A checker should return this value to indicate it does not grant access.
    */
-  const DENY = NULL;
+  const DENY = 'DENY';
 
   /**
    * Block access.
@@ -36,6 +36,6 @@ interface AccessInterface {
    * block access, regardless of any other access checkers. Most checkers
    * should prefer DENY.
    */
-  const KILL = FALSE;
+  const KILL = 'KILL';
 
 }

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

@@ -261,6 +261,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;
       }
@@ -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;
       }

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

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

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

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

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

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

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