#access should support AccessResultInterface objects or better has to always use it

#2495171: Block access results' cacheability metadata is not applied to the render arrays would also benefit from this.

It is enough to set max-age = 0 when encountering a non-access object.

Bad for performance, but good for security.

Yep, and CacheableMetadata::createFromObject() does exactly that.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -787,7 +788,7 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +      if (isset($element['#access']) && (($element['#access'] instanceof AccessResultInterface && !$element['#access']->isAllowed()) || !$element['#access'])) {
            $element[$key]['#access'] = FALSE;
          }
   

   If it's an instance of the object, let's pass the object instead?

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -138,7 +139,10 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    if (isset($elements['#access']) && (($elements['#access'] instanceof AccessResultInterface && !$elements['#access']->isAllowed()) || !$elements['#access'])) {
          return '';
        }
   

   This should apply the cacheability metadata to $elements.

About 1., could it be like this ? I moved the access calculation out of the loop since it belongs to the element, not its children, so remains constant during the loop.

No idea how to do 2., yet.

For point 2:

$this->addCacheableDependency($elements, $elements['#access']);

First, straight reroll — patch no longer applied cleanly.

Just applying #10, and moving the if (empty($elements)) condition to be the very first thing that is checked.

Next steps:

1. debugging the test failures
2. adding test coverage — to be added in \Drupal\Tests\Core\Render\RendererTest, specifically ::testRenderWithPresetAccess(), ::testRenderWithAccessCallbackCallable(), ::testRenderWithAccessPropertyAndCallback() and ::testRenderWithAccessControllerResolved() should be expanded to also assert the cacheability metadata, and ::providerBoolean() (which is the data provider for all of them) should be updated to return not only booleans, but also AccessResultInterface objects with cacheability metadata (e.g. AccessResult::allowed()->cachePerUser()).

Bumping to critical, discussed at the European D8 criticals meeting, we have agreement that this is critical.

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -416,7 +417,7 @@ public function flagErrors(FieldItemListInterface $items, ConstraintViolationLis
          // Only set errors if the element is accessible.
   -      if (!isset($element['#access']) || $element['#access']) {
   +      if (Element::isVisibleElement($element)) {
   

   Nit: Comment no longer matches the code.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -829,6 +830,15 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +        $child_denied = $access;
   

   s/$child_denied/$child_access/

   This is also the reason for e.g. the RssTest failure. Likely more.

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -843,8 +853,8 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
          // Deny access to child elements if parent is denied.
   

   Nit: comment outdated.

All fixed.

Now test coverage is the only remaining todo.

Awesome, thanks! Will review.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -829,6 +830,15 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
+    if (isset($element['#access'])) {
+      $access = $element['#access'];
+      if ($access instanceof AccessResultInterface || $access === FALSE) {
+        $child_access = $access;
+      }
+      else {
+        $child_access = NULL;
+      }
+    }

@@ -842,9 +852,9 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
+      // Children inherit #access from parent.
+      if (isset($child_access)) {
+        $element[$key]['#access'] = $child_access;
       }

I think this variable is storing the parent's access result so labelling it as $child_access is confusing. Also the creation of $access looks unnecessary.

This looks pretty close, couple of candidates for the Uber-nitpick award and a question about a possible DX improvement

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -829,6 +830,13 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +      if (($access instanceof AccessResultInterface && !$access->isAllowed()) || $access === FALSE) {
   

   Do you think it is worth creating a utility method on Element for this? Element::isVisibleElement could call it too.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -176,8 +181,18 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +      // If #access is an AccessResultInterface object, we must apply its
   

   nitpick: should be it's

3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -419,14 +421,44 @@ public function testRenderWithAccessPropertyAndCallback($access) {
   +        break;
   ...
   +        break;
   ...
   +        break;
   ...
   +        break;
   

   pretty sure phpcs says we should have empty line after each case breaking statement

4. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -454,7 +486,9 @@ public function testRenderTwice() {
   +      [AccessResult::forbidden()],
   +      [AccessResult::allowed()],
   

   Nice

Indeed, this looks *very* close. IS updated; the test coverage is complete.

I think this variable is storing the parent's access result so labelling it as $child_access is confusing. Also the creation of $access looks unnecessary.

+1

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -832,11 +832,9 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +      $inherited_access = NULL;
   

   Better :)

2. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -449,6 +452,167 @@ public function testExceededFileSize() {
   +   * @covers ::buildFOrm
   

   Nit: s/FOrm/Form/

3. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -449,6 +452,167 @@ public function testExceededFileSize() {
   +  public function testChildAccessInheritance($element, $access_checks) {
   ...
   +  public function providerTestChildAccessInheritance() {
   

   Looks very solid :)

4. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -419,14 +421,44 @@ public function testRenderWithAccessPropertyAndCallback($access) {
   +      case AccessResult::allowed():
   

   Oh wow, I didn't know PHP's switch statement supported objects! :D

5. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
   @@ -454,7 +486,9 @@ public function testRenderTwice() {
      public function providerBoolean() {
        return [
          [FALSE],
   -      [TRUE]
   +      [TRUE],
   +      [AccessResult::forbidden()],
   +      [AccessResult::allowed()],
        ];
   

   The provider's name is now a misnomer.

You basically use ==, so this is the reason why the boolean values are checked later.

¯\(°_o)/¯ — this is fine because it's in a test.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -182,7 +182,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+      // If #access is an AccessResultInterface object, we must apply it's

Nit: s/it's/its/ — can be fixed on commit.

#32.2:

nitpick: should be it's

No: http://www.elearnenglishlanguage.com/blog/english-mistakes/its/ :) Is the rule perhaps different in Australia?

Yeah I'm just plain wrong, Rtbc +1

Couple of nits/questions, generally looks good.

1. +++ b/core/lib/Drupal/Core/Render/Element.php
   @@ -162,7 +158,9 @@ public static function getVisibleChildren(array $elements) {
   +        || (($element['#access'] instanceof AccessResultInterface && $element['#access']->isAllowed()) || ($element['#access'] === TRUE)));
   

   Nit: shouldn't that be indented the same as the line above?

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -168,6 +169,10 @@ public function render(&$elements, $is_root_call = FALSE) {
      protected function doRender(&$elements, $is_root_call = FALSE) {
   +    if (empty($elements)) {
   +      return '';
   +    }
   

   So that make sense, but why's it necessary to add here?

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -176,8 +181,18 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +      }
   +      elseif ($elements['#access'] === FALSE) {
   +        return '';
   +      }
        }
   

   We need a follow-up to update https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen... in addition to the change record.

4. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -449,6 +452,167 @@ public function testExceededFileSize() {
   +
   +    $form = $this->formBuilder->buildForm($form_arg, $form_state);
   +
   +
   +    $actual_access_structure = [];
   +    $expected_access_structure = [];
   +
   

   Nit, extra blank line.

5. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -449,6 +452,167 @@ public function testExceededFileSize() {
   +
   +  public function providerTestChildAccessInheritance() {
   +    $data = [];
   +
   

   Needs phpdoc.

#38.2: See the #14 interdiff. Because that code block was affected and the existing code in HEAD doesn't make any sense (checking emptiness not as the first thing), that interdiff just moved it higher so that it actually makes sense again.

Yes, we need a change record for this, for sure. :)

Needs work for #38 and #40

Done: https://www.drupal.org/node/2514588.

#40 is done, #38.2 is also done. Just other points in #38 need to be addressed now (nits + follow-up filed).

+++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
@@ -482,6 +481,11 @@ public function testChildAccessInheritance($element, $access_checks) {
+   * Data provider for testChildAccessInheritance

Missing trailing period. Can be fixed on commit.

+++ b/core/lib/Drupal/Core/Render/Element.php
@@ -160,7 +160,7 @@ public static function getVisibleChildren(array $elements) {
   }

Isn't that further the other direction?

edit: failed to select right bit in dreditor and can fix on commit anyway.

Committed/pushed to 8.0.x, thanks!

Published CR: https://www.drupal.org/node/2514588.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -176,8 +181,18 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+    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 '';
+      }
     }

Post-Commit review:

Same as #2375695-102: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed I think this clashes very much with #cache.

I think what would need to happen here is (as we cannot add it to a higher-up region), we would need to create an artificial child element containing everything except access.

e.g.

$original = $elements;
$elements = [];
$this->addCacheableDependency($elements, $original['#access']);
unset($original['#access']);
$elements['access_child'] = $original;

OR push the access to the stack, so that it is available after the element was rendered and cached ...

But caching with the #access information is wrong. (though probably only a major bug)

Please ignore #50:

Discussed in IRC:

- Adding #access metadata that is affecting the element itself to cacheable metadata is correct, because in case we would be doing the access check inside some #pre_render function, we would also bubble it up and create a cache redirect.

=> No bug here.

This is fixed in a correct way.