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.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 3620563.png | 140.9 KB | csakiistvan |
Issue fork entity_usage-3620563
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:
- 3620563-listusagecontrollercheckaccess-drops-the
changes, plain diff MR !242
Comments
Comment #2
alexpottWe should ensure this is not cacheable.
Comment #5
andres alvarez commentedConfirmed the root cause.
checkAccess()calls$entity->access('view')without the third$return_as_objectparameter, so it gets back a plain boolean and discards the realAccessResultInterfaceentirely, including its cache tags and contexts. The method then builds a freshAccessResult::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
AccessResultInterfacecarries the node's own cache tag and theuser.permissionscontext, 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.
Comment #6
andres alvarez commentedComment #7
csakiistvanComment #8
csakiistvan✅ Tested and works — applied MR !242, ran
ddev drush crand re-checked the access of the usage routes for a non-admin user withaccess entity usage statistics: the result now carries the node's own cache tags and theuser.permissionscontext, where before the patch it carried none. The dropped cacheability is fixed for both routes.Comment #9
csakiistvanComment #11
alexpottI'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.