Issue #2487600 by dawehner, Wim Leers, fgm: #access should support...

Issue #2487600 by dawehner, Wim Leers, fgm: #access should support AccessResultInterface objects or better has to always use it

core/includes/common.inc

@@ -1036,7 +1036,7 @@ function drupal_pre_render_links($element) {
     $child = &$element[$key];
     // If the child has links which have not been printed yet and the user has
     // access to it, merge its links in to the parent.
-    if (isset($child['#links']) && empty($child['#printed']) && (!isset($child['#access']) || $child['#access'])) {
+    if (isset($child['#links']) && empty($child['#printed']) && Element::isVisibleElement($child)) {
       $element['#links'] += $child['#links'];
       // Mark the child as having been printed already (so that its links
       // cannot be mistakenly rendered twice).

core/lib/Drupal/Core/Field/WidgetBase.php

@@ -12,6 +12,7 @@
 use Drupal\Component\Utility\SortArray;
 use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Form\FormStateInterface;
+use Drupal\Core\Render\Element;
 use Symfony\Component\Validator\ConstraintViolationInterface;
 use Symfony\Component\Validator\ConstraintViolationListInterface;
 
@@ -415,8 +416,8 @@ public function flagErrors(FieldItemListInterface $items, ConstraintViolationLis
         }
       }
 
-      // Only set errors if the element is accessible.
-      if (!isset($element['#access']) || $element['#access']) {
+      // Only set errors if the element is visible.
+      if (Element::isVisibleElement($element)) {
         $handles_multiple = $this->handlesMultipleValues();
 
         $violations_by_delta = array();

core/lib/Drupal/Core/Form/FormBuilder.php

@@ -12,6 +12,7 @@
 use Drupal\Component\Utility\NestedArray;
 use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\UrlHelper;
+use Drupal\Core\Access\AccessResultInterface;
 use Drupal\Core\Access\CsrfTokenGenerator;
 use Drupal\Core\DependencyInjection\ClassResolverInterface;
 use Drupal\Core\EventSubscriber\MainContentViewSubscriber;
@@ -829,6 +830,13 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
 
     // Recurse through all child elements.
     $count = 0;
+    if (isset($element['#access'])) {
+      $access = $element['#access'];
+      $inherited_access = NULL;
+      if (($access instanceof AccessResultInterface && !$access->isAllowed()) || $access === FALSE) {
+        $inherited_access = $access;
+      }
+    }
     foreach (Element::children($element) as $key) {
       // Prior to checking properties of child elements, their default
       // properties need to be loaded.
@@ -842,9 +850,9 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
         $element[$key]['#tree'] = $element['#tree'];
       }
 
-      // Deny access to child elements if parent is denied.
-      if (isset($element['#access']) && !$element['#access']) {
-        $element[$key]['#access'] = FALSE;
+      // Children inherit #access from parent.
+      if (isset($inherited_access)) {
+        $element[$key]['#access'] = $inherited_access;
       }
 
       // Make child elements inherit their parent's #disabled and #allow_focus

core/lib/Drupal/Core/Render/Element.php

@@ -8,6 +8,7 @@
 namespace Drupal\Core\Render;
 
 use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Core\Access\AccessResultInterface;
 
 /**
  * Provides helper methods for Drupal render elements.
@@ -136,11 +137,6 @@ public static function getVisibleChildren(array $elements) {
     foreach (static::children($elements) as $key) {
       $child = $elements[$key];
 
-      // Skip un-accessible children.
-      if (isset($child['#access']) && !$child['#access']) {
-        continue;
-      }
-
       // Skip value and hidden elements, since they are not rendered.
       if (!static::isVisibleElement($child)) {
         continue;
@@ -162,7 +158,9 @@ public static function getVisibleChildren(array $elements) {
    *   TRUE if the element is visible, otherwise FALSE.
    */
   public static function isVisibleElement($element) {
-    return (!isset($element['#type']) || !in_array($element['#type'], ['value', 'hidden', 'token'])) && (!isset($element['#access']) || $element['#access']);
+    return (!isset($element['#type']) || !in_array($element['#type'], ['value', 'hidden', 'token']))
+      && (!isset($element['#access'])
+      || (($element['#access'] instanceof AccessResultInterface && $element['#access']->isAllowed()) || ($element['#access'] === TRUE)));
   }
 
   /**

core/lib/Drupal/Core/Render/Renderer.php

@@ -10,6 +10,7 @@
 use Drupal\Component\Utility\NestedArray;
 use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\UrlHelper;
+use Drupal\Core\Access\AccessResultInterface;
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Controller\ControllerResolverInterface;
@@ -168,6 +169,10 @@ public function render(&$elements, $is_root_call = FALSE) {
    * See the docs for ::render().
    */
   protected function doRender(&$elements, $is_root_call = FALSE) {
+    if (empty($elements)) {
+      return '';
+    }
+
     if (!isset($elements['#access']) && isset($elements['#access_callback'])) {
       if (is_string($elements['#access_callback']) && strpos($elements['#access_callback'], '::') === FALSE) {
         $elements['#access_callback'] = $this->controllerResolver->getControllerFromDefinition($elements['#access_callback']);
@@ -176,8 +181,18 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
     }
 
     // Early-return nothing if user does not have access.
-    if (empty($elements) || (isset($elements['#access']) && !$elements['#access'])) {
-      return '';
+    if (isset($elements['#access'])) {
+      // If #access is an AccessResultInterface object, we must apply it's
+      // cacheability metadata to the render array.
+      if ($elements['#access'] instanceof AccessResultInterface) {
+        $this->addCacheableDependency($elements, $elements['#access']);
+        if (!$elements['#access']->isAllowed()) {
+          return '';
+        }
+      }
+      elseif ($elements['#access'] === FALSE) {
+        return '';
+      }
     }
 
     // Do not print elements twice.

core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php

@@ -7,6 +7,9 @@
 
 namespace Drupal\Tests\Core\Form;
 
+use Drupal\Component\Utility\NestedArray;
+use Drupal\Core\Access\AccessResult;
+use Drupal\Core\Access\AccessResultForbidden;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
 use Drupal\Core\Form\EnforcedResponseException;
 use Drupal\Core\Form\FormBuilderInterface;
@@ -449,6 +452,171 @@ public function testExceededFileSize() {
     $this->formBuilder->buildForm($form_arg, $form_state);
   }
 
+  /**
+   * @covers ::buildForm
+   *
+   * @dataProvider providerTestChildAccessInheritance
+   */
+  public function testChildAccessInheritance($element, $access_checks) {
+    $form_arg = new TestFormWithPredefinedForm();
+    $form_arg->setForm($element);
+
+    $form_state = new FormState();
+
+    $form = $this->formBuilder->buildForm($form_arg, $form_state);
+
+    $actual_access_structure = [];
+    $expected_access_structure = [];
+
+    // Ensure that the expected access checks are set.
+    foreach ($access_checks as $access_check) {
+      $parents = $access_check[0];
+      $parents[] = '#access';
+
+      $actual_access = NestedArray::getValue($form, $parents);
+      $actual_access_structure[] = [$parents, $actual_access];
+      $expected_access_structure[] = [$parents, $access_check[1]];
+    }
+
+    $this->assertEquals($expected_access_structure, $actual_access_structure);
+  }
+
+  /**
+   * Data provider for testChildAccessInheritance.
+   *
+   * @return array
+   */
+  public function providerTestChildAccessInheritance() {
+    $data = [];
+
+    $element = [
+      'child0' => [
+        '#type' => 'checkbox',
+      ],
+      'child1' => [
+        '#type' => 'checkbox',
+      ],
+      'child2' => [
+        '#type' => 'fieldset',
+        'child2.0' => [
+          '#type' => 'checkbox',
+        ],
+        'child2.1' => [
+          '#type' => 'checkbox',
+        ],
+        'child2.2' => [
+          '#type' => 'checkbox',
+        ],
+      ],
+    ];
+
+    // Sets access FALSE on the root level, this should be inherited completely.
+    $clone = $element;
+    $clone['#access'] = FALSE;
+
+    $expected_access = [];
+    $expected_access[] = [[], FALSE];
+    $expected_access[] = [['child0'], FALSE];
+    $expected_access[] = [['child1'], FALSE];
+    $expected_access[] = [['child2'], FALSE];
+    $expected_access[] = [['child2', 'child2.0'], FALSE];
+    $expected_access[] = [['child2', 'child2.1'], FALSE];
+    $expected_access[] = [['child2', 'child2.2'], FALSE];
+
+    $data['access-false-root'] = [$clone, $expected_access];
+
+    $clone = $element;
+    $access_result = AccessResult::forbidden()->addCacheContexts(['user']);
+    $clone['#access'] = $access_result;
+
+    $expected_access = [];
+    $expected_access[] = [[], $access_result];
+    $expected_access[] = [['child0'], $access_result];
+    $expected_access[] = [['child1'], $access_result];
+    $expected_access[] = [['child2'], $access_result];
+    $expected_access[] = [['child2', 'child2.0'], $access_result];
+    $expected_access[] = [['child2', 'child2.1'], $access_result];
+    $expected_access[] = [['child2', 'child2.2'], $access_result];
+
+    $data['access-forbidden-root'] = [$clone, $expected_access];
+
+    // Allow access on the most outer level but set FALSE otherwise.
+    $clone = $element;
+    $clone['#access'] = TRUE;
+    $clone['child0']['#access'] = FALSE;
+
+    $expected_access = [];
+    $expected_access[] = [[], TRUE];
+    $expected_access[] = [['child0'], FALSE];
+    $expected_access[] = [['child1'], NULL];
+    $expected_access[] = [['child2'], NULL];
+    $expected_access[] = [['child2', 'child2.0'], NULL];
+    $expected_access[] = [['child2', 'child2.1'], NULL];
+    $expected_access[] = [['child2', 'child2.2'], NULL];
+
+    $data['access-true-root'] = [$clone, $expected_access];
+
+    // Allow access on the most outer level but forbid otherwise.
+    $clone = $element;
+    $access_result_allowed = AccessResult::allowed()
+      ->addCacheContexts(['user']);
+    $clone['#access'] = $access_result_allowed;
+    $access_result_forbidden = AccessResult::forbidden()
+      ->addCacheContexts(['user']);
+    $clone['child0']['#access'] = $access_result_forbidden;
+
+    $expected_access = [];
+    $expected_access[] = [[], $access_result_allowed];
+    $expected_access[] = [['child0'], $access_result_forbidden];
+    $expected_access[] = [['child1'], NULL];
+    $expected_access[] = [['child2'], NULL];
+    $expected_access[] = [['child2', 'child2.0'], NULL];
+    $expected_access[] = [['child2', 'child2.1'], NULL];
+    $expected_access[] = [['child2', 'child2.2'], NULL];
+
+    $data['access-allowed-root'] = [$clone, $expected_access];
+
+    // Allow access on the most outer level, deny access on a parent, and allow
+    // on a child. The denying should be inherited.
+    $clone = $element;
+    $clone['#access'] = TRUE;
+    $clone['child2']['#access'] = FALSE;
+    $clone['child2.0']['#access'] = TRUE;
+    $clone['child2.1']['#access'] = TRUE;
+    $clone['child2.2']['#access'] = TRUE;
+
+    $expected_access = [];
+    $expected_access[] = [[], TRUE];
+    $expected_access[] = [['child0'], NULL];
+    $expected_access[] = [['child1'], NULL];
+    $expected_access[] = [['child2'], FALSE];
+    $expected_access[] = [['child2', 'child2.0'], FALSE];
+    $expected_access[] = [['child2', 'child2.1'], FALSE];
+    $expected_access[] = [['child2', 'child2.2'], FALSE];
+
+    $data['access-mixed-parents'] = [$clone, $expected_access];
+
+    $clone = $element;
+    $clone['#access'] = $access_result_allowed;
+    $clone['child2']['#access'] = $access_result_forbidden;
+    $clone['child2.0']['#access'] = $access_result_allowed;
+    $clone['child2.1']['#access'] = $access_result_allowed;
+    $clone['child2.2']['#access'] = $access_result_allowed;
+
+    $expected_access = [];
+    $expected_access[] = [[], $access_result_allowed];
+    $expected_access[] = [['child0'], NULL];
+    $expected_access[] = [['child1'], NULL];
+    $expected_access[] = [['child2'], $access_result_forbidden];
+    $expected_access[] = [['child2', 'child2.0'], $access_result_forbidden];
+    $expected_access[] = [['child2', 'child2.1'], $access_result_forbidden];
+    $expected_access[] = [['child2', 'child2.2'], $access_result_forbidden];
+
+    $data['access-mixed-parents-object'] = [$clone, $expected_access];
+
+    return $data;
+  }
+
 }
 
 class TestForm implements FormInterface {
@@ -467,3 +635,21 @@ public static function create(ContainerInterface $container) {
     return new static();
   }
 }
+
+
+class TestFormWithPredefinedForm extends TestForm {
+
+  /**
+   * @var array
+   */
+  protected $form;
+
+  public function setForm($form) {
+    $this->form = $form;
+  }
+
+  public function buildForm(array $form, FormStateInterface $form_state) {
+    return $this->form;
+  }
+
+}

core/tests/Drupal/Tests/Core/Render/ElementTest.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\Tests\Core\Render;
 
+use Drupal\Core\Access\AccessResult;
 use Drupal\Tests\UnitTestCase;
 use Drupal\Core\Render\Element;
 
@@ -151,6 +152,8 @@ public function providerVisibleChildren() {
       array(array('#property1' => '', 'child1' => array()), array('child1')),
       array(array('#property1' => '', 'child1' => array(), 'child2' => array('#access' => TRUE)), array('child1', 'child2')),
       array(array('#property1' => '', 'child1' => array(), 'child2' => array('#access' => FALSE)), array('child1')),
+      'access_result_object_allowed' => array(array('#property1' => '', 'child1' => array(), 'child2' => array('#access' => AccessResult::allowed())), array('child1', 'child2')),
+      'access_result_object_forbidden' => array(array('#property1' => '', 'child1' => array(), 'child2' => array('#access' => AccessResult::forbidden())), array('child1')),
       array(array('#property1' => '', 'child1' => array(), 'child2' => array('#type' => 'textfield')), array('child1', 'child2')),
       array(array('#property1' => '', 'child1' => array(), 'child2' => array('#type' => 'value')), array('child1')),
       array(array('#property1' => '', 'child1' => array(), 'child2' => array('#type' => 'hidden')), array('child1')),

core/tests/Drupal/Tests/Core/Render/RendererTest.php

@@ -7,6 +7,8 @@
 
 namespace Drupal\Tests\Core\Render;
 
+use Drupal\Core\Access\AccessResult;
+use Drupal\Core\Access\AccessResultInterface;
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\CacheableDependencyInterface;
 use Drupal\Core\Render\Element;
@@ -367,7 +369,7 @@ public function testRenderSortingWithSetHashSorted() {
    * @covers ::render
    * @covers ::doRender
    *
-   * @dataProvider providerBoolean
+   * @dataProvider providerAccessValues
    */
   public function testRenderWithPresetAccess($access) {
     $build = [
@@ -381,7 +383,7 @@ public function testRenderWithPresetAccess($access) {
    * @covers ::render
    * @covers ::doRender
    *
-   * @dataProvider providerBoolean
+   * @dataProvider providerAccessValues
    */
   public function testRenderWithAccessCallbackCallable($access) {
     $build = [
@@ -399,7 +401,7 @@ public function testRenderWithAccessCallbackCallable($access) {
    * @covers ::render
    * @covers ::doRender
    *
-   * @dataProvider providerBoolean
+   * @dataProvider providerAccessValues
    */
   public function testRenderWithAccessPropertyAndCallback($access) {
     $build = [
@@ -416,16 +418,49 @@ public function testRenderWithAccessPropertyAndCallback($access) {
    * @covers ::render
    * @covers ::doRender
    *
-   * @dataProvider providerBoolean
+   * @dataProvider providerAccessValues
    */
   public function testRenderWithAccessControllerResolved($access) {
+
+    switch ($access) {
+      case AccessResult::allowed():
+        $method = 'accessResultAllowed';
+        break;
+
+      case AccessResult::forbidden():
+        $method = 'accessResultForbidden';
+        break;
+
+      case FALSE:
+        $method = 'accessFalse';
+        break;
+
+      case TRUE:
+        $method = 'accessTrue';
+        break;
+    }
+
     $build = [
-      '#access_callback' => 'Drupal\Tests\Core\Render\TestAccessClass::' . ($access ? 'accessTrue' : 'accessFalse'),
+      '#access_callback' => 'Drupal\Tests\Core\Render\TestAccessClass::' . $method,
     ];
 
     $this->assertAccess($build, $access);
   }
 
+  /**
+   * @covers ::render
+   * @covers ::doRender
+   */
+  public function testRenderAccessCacheablityDependencyInheritance() {
+    $build = [
+      '#access' => AccessResult::allowed()->addCacheContexts(['user']),
+    ];
+
+    $this->renderer->renderPlain($build);
+
+    $this->assertEquals(['languages:language_interface', 'theme', 'user'], $build['#cache']['contexts']);
+  }
+
   /**
    * Tests that a first render returns the rendered output and a second doesn't.
    *
@@ -451,10 +486,12 @@ public function testRenderTwice() {
    *
    * @return array
    */
-  public function providerBoolean() {
+  public function providerAccessValues() {
     return [
       [FALSE],
-      [TRUE]
+      [TRUE],
+      [AccessResult::forbidden()],
+      [AccessResult::allowed()],
     ];
   }
 
@@ -469,7 +506,7 @@ public function providerBoolean() {
   protected function assertAccess($build, $access) {
     $sensitive_content = $this->randomContextValue();
     $build['#markup'] = $sensitive_content;
-    if ($access) {
+    if (($access instanceof AccessResultInterface && $access->isAllowed()) || $access === TRUE) {
       $this->assertSame($sensitive_content, $this->renderer->renderRoot($build));
     }
     else {
@@ -784,6 +821,14 @@ public static function accessFalse() {
     return FALSE;
   }
 
+  public static function accessResultAllowed() {
+    return AccessResult::allowed();
+  }
+
+  public static function accessResultForbidden() {
+    return AccessResult::forbidden();
+  }
+
 }
 
 class TestCallables {