Problem/Motivation
The performance pattern load_by_properties (modules/audit_performance/src/Plugin/AuditAnalyzer/PerformanceAnalyzer.php) reports "loadByProperties() bypasses entity access control" and recommends an entity query with ->accessCheck(TRUE), whenever isUserFacingContext() considers the class user-facing (which is the default for any class that is not named like a service and does not implement an interface).
EntityStorageBase::loadByProperties() does run its entity query with accessCheck(FALSE). But accessCheck() does not check entity access: it only adds the '_access' tag to the query, which has an effect only when something alters queries with that tag. Verified:
- In core, the only implementation is node_query_node_access_alter(), and it returns early when no module implements hook_node_grants() (node.module).
- taxonomy adds the taxonomy_term_access tag in some queries, but no core module alters it.
- The contrib Entity API module applies 'query_access' handler conditions only to queries with that tag (entity_query_entity_query_alter()). It gives EVERY entity type without a handler an EventOnlyQueryAccessHandler, which adds conditions only when something subscribes to the entity.query_access or entity.query_access. events; real QueryAccessHandler classes (Commerce products, orders, stores, profiles...) always add them.
So on most sites loadByProperties() and an entity query with accessCheck(TRUE) return exactly the same entities, and the warning only adds noise. On a real site with Commerce, Profile and Entity API, only commerce_order, commerce_product, commerce_store and profile have active query access rules; nodes do not (no node grants module).
Steps to reproduce
1. On a site without node access modules, add $this->entityTypeManager()->getStorage('node')->loadByProperties(['title' => $title]); to a controller.
2. Run drush audit:run performance --format=json.
3. LOAD_BY_PROPERTIES is reported, although an entity query with accessCheck(TRUE) would return the same nodes.
Proposed resolution
Add a requires_query_access flag to the pattern, checked before the existing user-facing context check (which is unchanged):
- New getQueryAccessEntityTypes() computes, once per run, the entity types whose queries are altered for access on this site: a module implements hook_query__access_alter() (for node, only if a module also implements hook_node_grants()), or the entity type has an Entity API query_access handler that adds conditions (any handler except EventOnlyQueryAccessHandler, which counts only when its events have subscribers).
- New storageCallHasQueryAccess() resolves the entity type of the storage with the resolver from #3630468 (getStorage('type') chained to the call, or the assignment of the storage variable or property). The issue is reported when that entity type has query access rules, or when the storage cannot be resolved and at least one entity type has them. When no entity type has query access rules, nothing is reported.
- Inject event_dispatcher (for the subscriber check) into the analyzer.
- Reword the message ("loadByProperties() skips the query access rules that this site applies to this entity type (for example node grants)") and the recommendation.
Remaining tasks
- Implement the flag, the helpers and the dependency.
- Unit test with mocked hooks, handlers and event subscribers, in a controller: no query access => not reported (node and unresolved storage); node grants => node and unresolved storage reported, terms not; Entity API QueryAccessHandler on products => reported; event-only handler with a subscriber for terms => terms reported, products not.
- Kernel smoke test still instantiates the analyzer through the container.
User interface changes
LOAD_BY_PROPERTIES is only reported for entity types with query access rules on the audited site, with a more precise message.
API changes
None.
Data model changes
None.
Comments
Comment #3
trebormc