Problem/Motivation

BlockPluginInterface::access() now returns an AccessResult object.
While this object is really nice and helpful for caching of access checking, it makes code potentially more fragile, when you check
access and forget that you don't get back a boolean value.

Proposed resolution

By default return a boolean value again, but make it possible for request for a result object, much like a lot of other apis.

Remaining tasks

User interface changes

API changes

interface BlockPluginInterface {

-  public function access(AccountInterface $account) {
+  public function access(AccountInterface $account, $return_as_object = FALSE) {

}

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because there is no actual bug out there.
Issue priority Major because it could easily happen that contrib introduces a bug, like page_manager had for a while
Prioritized changes The main goal of this issue is security.
Disruption Disruptive is not nothing, given that all contrib/custom blocks have to be adapted, but its worth for the security improvement.
CommentFileSizeAuthor
#26 block-2342057-26.patch6.97 KBwim leers
#21 block-2342057-21.patch6.48 KBhussainweb
#21 interdiff-17-21.txt600 byteshussainweb
#17 block-2342057-17.patch6.5 KBhussainweb
#13 block-2342057-13.patch8.2 KBdawehner
#11 2342057-PASS.patch7.8 KBdawehner
#11 2342057-FAIL.patch3.68 KBdawehner
#4 block-2342057-4.patch7.03 KBdawehner
#4 interdiff.txt5.42 KBdawehner
#1 2342057.patch2.1 KBdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2342057.patch2.1 KB

.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Do we need to update existing calls to that method?

Status: Needs review » Needs work

The last submitted patch, 1: 2342057.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Title: Expand BlockPluginInterface to take into account $return_as_object » [sechole] Expand BlockPluginInterface to take into account $return_as_object
Category: Task » Bug report
Priority: Normal » Major
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt5.42 KB
new block-2342057-4.patch7.03 KB

Ha, now we do have one sechole!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Some commented out tests in there :)

Nice, so core had the same bug as page_manager? Why are there no tests failing on this?

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Critical
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Wow. Very poor test coverage for block access, then :(

Status: Needs review » Needs work

The last submitted patch, 4: block-2342057-4.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented 
+++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
@@ -120,7 +120,7 @@ public function build() {
-        if ($block->access('view')) {
+        if ($block->access('view', NULL, FALSE)) {

This should be the default, no? I.e. specifying NULL and FALSE explicitly, should not be necessary?!

dawehner’s picture

dawehner
Primary language German
commented

This should be the default, no? I.e. specifying NULL and FALSE explicitly, should not be necessary?!

Right this is the default, though I prefer to make this implicit, to improve readability. Maybe we should even use TRUE and call isAllowed()?

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2342057-FAIL.patch3.68 KB
new 2342057-PASS.patch7.8 KB

Let's write a proper test.

Status: Needs review » Needs work

The last submitted patch, 11: 2342057-PASS.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Title: [sechole] Expand BlockPluginInterface to take into account $return_as_object » Expand BlockPluginInterface to take into account $return_as_object
Priority: Critical » Major
Status: Needs work » Needs review
StatusFileSize
new block-2342057-13.patch8.2 KB

As often said, better don't trust me.

    $return = $this->processAccessHookResults($access);
    if ($return->isNeutral()) {
      // No module had an opinion about the access, so let's the access
      // handler check access.
      $return = $return->orIf($this->checkAccess($entity, $operation, $langcode, $account));
    }
    $result = $this->setCache($return, $entity->uuid(), $operation, $langcode, $account);
    return $return_as_object ? $result : $result->isAllowed();
  }

Just to be clear, so the code is secure, but we just page_manager has a sechole here. I still would love to have dedicated testcoverage here.

Fixed the unittest.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
  1. +++ b/core/lib/Drupal/Core/Block/BlockPluginInterface.php
@@ -47,13 +47,19 @@ public function label();
+   * @param bool $return_as_object
+   *   (optional) Defaults to FALSE.

    Is this the same elsewhere? It's not too useful either way but we afaik stopped putting the default in the description? Maybe something like "Controls if the return value is an AccessResult object or a bollean" ? 

  2. +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestAccessBlock.php
@@ -0,0 +1,82 @@
+   * Tests the test access block.
+   *
+   *
+   * @param array $configuration

    Unnecessary empty line. 

  3. +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestAccessBlock.php
@@ -0,0 +1,82 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function isCacheable() {
+    return TRUE;
+  }

    Why bother implementing this?

I like how the FAIL test in #11 is green and PASS failed :) Can you provide an updated test-only patch? Then this is good to go I think.

Uh, me much stupid. There's obviously no failing test because there is no bug :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Setting to needs work for the minor stuff.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Novice

Tagging novice to fix the minor things from my review.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new block-2342057-17.patch6.5 KB

Doing a reroll first.

Status: Needs review » Needs work

The last submitted patch, 17: block-2342057-17.patch, failed testing.

Status: Needs work » Needs review

rpayanm queued 17: block-2342057-17.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: block-2342057-17.patch, failed testing.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-17-21.txt600 bytes
new block-2342057-21.patch6.48 KB

Let's see if this works.

dawehner’s picture

dawehner
Primary language German
commented
Category: Bug report » Task
Issue summary: View changes

Adding a IS and BA

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Assigned: Unassigned » wim leers

Skimmed through it, looks good to me. Page manager is AFAIK still broken, so would be nice to have this fixed.

Maybe Wim can review/RTBC this? :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

Looks good to me too.

+++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestAccessBlock.php
@@ -0,0 +1,82 @@
+   * Tests the test access block.
+   *
+   *
+   * @param array $configuration

Two blank lines; should be one. Can be fixed on commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: block-2342057-21.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new block-2342057-26.patch6.97 KB

Rebased.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed b8f63ed and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed b8f63ed on 8.0.x 
    Issue #2342057 by dawehner, hussainweb, Wim Leers: Expand...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.