Updated: Comment #0

Problem/Motivation

After custom block is placed in some region with "node condition" there's no way to remove or reconfigure one.
That's caused by EntityListController::getOperations() code

    if ($entity->access('update')) {
      $operations['edit'] = array(
        'title' => $this->t('Edit'),
        'href' => $uri['path'] . '/edit',
        'options' => $uri['options'],
        'weight' => 10,
      );
    }

User could not have access to update/delete a custom block because node_block_access() fires first and blocks execution of entity access callback in EntityAccessController::access()

    $access = array_merge(
      $this->moduleHandler->invokeAll('entity_access', array($entity, $operation, $account, $langcode)),
      $this->moduleHandler->invokeAll($entity->entityType() . '_access', array($entity, $operation, $account, $langcode))
    );

    if (($return = $this->processAccessHookResults($access)) === NULL) {
      // No module had an opinion about the access, so let's the access
      // controller check create access.
      $return = (bool) $this->checkAccess($entity, $operation, $langcode, $account);
    }
    return $this->setCache($return, $entity->uuid(), $operation, $langcode, $account);

The comment makes sense so ...

Proposed resolution

Use BlockListController::access() method to fire it before standard controller

Remaining tasks

tbd

User interface changes

UI should work

API changes

not sure

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, block-access.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.42 KB

Another approach, do not allow to fire hooks for operations other then 'view'

dawehner’s picture

FileSize
4.24 KB
3.88 KB

I think it is wrong to not fire those hooks on other cases than 'view'.

Let's use the proper interfaces, if we touch all the instances anyway.

andypost’s picture

Title: Custom block have neither Configure no Delete links after placement » BlockAccessController::checkAccess() is not executed if any hook_block_access() returns data
Issue tags: +Blocks-Layouts

Re-title, actually we need to think more about block access

I think it is wrong to not fire those hooks on other cases than 'view'

Agree here, so seems that first patch was right - we need to override access() method and move visibility logic to special method to make it extendable in contrib.

Currently access to view block is hardcoded in (page-url, role) but node-type access check is implemented via node_block_access() and overlay removes unneeded blocks via the same hook

Status: Needs review » Needs work

The last submitted patch, 2: 2139141-block-access-2.patch, failed testing.

larowlan’s picture

Issue tags: +Needs tests

We should be testing that the links are visible

andypost’s picture

Probably better to refactor the way like visibility works, related #1880274: Reimplement block visibility settings

Suppose current implementation is weird and will bring a lot of troubles in contrib because each contrib implementation have to add check for $operation == 'view' in hook_block_access()

So I'd prefer #0 approach to override BlockAccessController::access() to execute hooks after BlockBase::access() to allow override visibility of the block

tim.plunkett’s picture

This actually causes a bug in overlay, see #2099503: No operations for some blocks at Block layout page in overlay

If that is marked a duplicate, the tests should be moved here.

tim.plunkett’s picture

andypost’s picture

Overlay hunk could be skipped now, maybe better to postpone the issue on #2124599: node_block_access affects access to editing blocks Removes "operations" links for Block layout page. or just mark as duplicate?

andypost’s picture

mr.baileys’s picture

Assigned: Unassigned » mr.baileys

Working on this as part of the Drupal Developer Days sprint in Szeged.

mr.baileys’s picture

Assigned: mr.baileys » Unassigned
Status: Needs work » Closed (duplicate)