Problem/Motivation

ListUsageController::checkAccess() checks the entity access in boolean mode and returns a new access result:

if (!$entity || !$entity->access('view')) {
  return AccessResult::forbidden();
}
return AccessResult::allowed();

The access result of the entity is thrown away, so the returned result has no cache tags and no cache contexts. It no longer depends on the entity, nor on what the entity access check varies by, such as the node grants.

Both usage routes use this callback, entity_usage.usage_list directly and the per entity type routes through LocalTaskUsageController::checkAccessLocalTask(). The _permission part of the routes is fine on its own.

Steps to reproduce

Check the access of the usage route of a node and inspect the result:

\Drupal::service('access_manager')
  ->checkNamedRoute('entity.node.entity_usage', ['node' => 1], $account, TRUE);

It carries no cache tag for the node, so it is not invalidated when the node is unpublished.

Proposed resolution

Return the access result of the entity:

if (!$entity) {
  return AccessResult::forbidden();
}
return $entity->access('view', NULL, TRUE);

Remaining tasks

Decide what to return when the entity does not exist, the current result is cacheable forever.
Test coverage.

User interface changes

None.

API changes

None.

Data model changes

None.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

herved created an issue. See original summary.

alexpott’s picture

Decide what to return when the entity does not exist, the current result is cacheable forever.

We should ensure this is not cacheable.

andres alvarez made their first commit to this issue’s fork.

andres alvarez’s picture

Confirmed the root cause. checkAccess() calls $entity->access('view') without the third $return_as_object parameter, so it gets back a plain boolean and discards the real AccessResultInterface entirely, including its cache tags and contexts. The method then builds a fresh AccessResult::forbidden()/AccessResult::allowed() with no cacheability metadata at all, so the route's cached access decision never invalidates when the entity is unpublished, republished, or the user's permissions change.

Fix: return $entity->access('view', NULL, TRUE) directly instead of building a new result. Matches the fix already proposed in this issue.

Added a Kernel test (real node, real controller instance, no mocks) asserting the returned AccessResultInterface carries the node's own cache tag and the user.permissions context, for both the allowed and forbidden cases. Confirmed it fails against the unpatched code (zero cacheability metadata on the result) and passes with the fix.

MR to follow.

andres alvarez’s picture

Status: Active » Needs review
csakiistvan’s picture

Assigned: Unassigned » csakiistvan
StatusFileSize
new140.9 KB
csakiistvan’s picture

StatusFileSize
new140.9 KB

✅ Tested and works — applied MR !242, ran ddev drush cr and re-checked the access of the usage routes for a non-admin user with access entity usage statistics: the result now carries the node's own cache tags and the user.permissions context, where before the patch it carried none. The dropped cacheability is fixed for both routes.

csakiistvan’s picture

Assigned: csakiistvan » Unassigned
Status: Needs review » Reviewed & tested by the community

  • alexpott committed 67ad88f3 on 5.x
    fix: #3620563 ListUsageController::checkAccess() drops the cacheability...
alexpott’s picture

Version: 5.x-dev » 8.x-2.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I've merged the MR in 5.x - if someone can make an 8.x-2.x version I'll merge it there too. Nice work everyone.