Problem/Motivation

Why issue is filed:
EntityFieldQuery (EFQ) processing is not consistent and will cause confusion.

Example / Reproduction:

  1. if EFQ has a FieldCondition, then node_access_grants is checked
  2. if EFQ has only EntityCondition, NO node_access_grant check occurs

Root cause(s):
field_sql_storage_field_storage_query() hardcodes an 'entity_field_access' tag on the query, which then triggers _node_query_node_access_alter().

Considerations while implementing solution:

  • Impact to the Field UI vi field_has_data(). Why, you ask? field_has_data() relies on EFQ
  • When implementing the EFQ consistency fix, See related issue with tested patches and recreation steps to test if the fix will break field_has_data() logic.: http://drupal.org/node/1302228
  • entity.inc may need to check if node or not. This may be separate issue from entity field access consistency

Proposed resolution

Proposed HOTFIX:
Add 'account' in metadata. See comment: http://drupal.org/node/997394#comment-5096664

Proposed FULL SOLUTION:
Rationale: Create a consistent and flexible processing logic for EntityFieldQuery and its interaction with node_access. Benefit: it will provide a predictable interface for other operations leveraging the EFQ logic.

The solution proposal includes the following changes:

  1. Remove hardcoded node_access check in EFQ
  2. Support the node_access check in EFQ. It is up to the caller add the tag
  3. The tag should be removed from field_sql_storage_field_storage_query()
  4. Support for the tag should be added for EFQs without field conditions (does this need to be supported for all cases? w w/o field conditions.

Remaining tasks

  1. cradle to grave impact analysis of *where* the fix needs to happen and what may break when implementing solution
  2. identify the interaction behavior between entity.inc and EFQ. define if any work to entity.inc needs to be added in the proposed solution
  3. identify proposed solution, review with those on the comment list for feedback.
  4. fix it, please. :-)

User interface changes

No obvious changes.

API changes

No obvious changes.

Original report by [yched]

Currently, an EntityFieldQuery with a FieldCondition checks node_access grants, while an EFQ with only EntityConditions performs no check. That's inconsistent and confusing.

That's because field_sql_storage_field_storage_query() hardcodes an 'entity_field_access' tag on the query, which triggers _node_query_node_access_alter().

Side issue: the impact on the query is then hardcoded in _node_query_node_access_alter(), no matter what the field storage engine (currently, messes with the infamous {field_data_table}.etid column, which makes this etid construct de-facto compulsory for any other storage backend that want to store fields in SQL).

Discussing this with Damz, we agreed that :
- node_access check should be supported but not *hardcoded* in EFQ. It's up to the caller to add the tag himself.
--> The tag should be removed from field_sql_storage_field_storage_query().
--> Support for the tag should be added for EFQs without field conditions

- Ideally, the field storage engine should also handle dealing with the 'entity_field_access' tag if it was set.
That's probably for a separate issue, though ?

CommentFileSizeAuthor
#11 997394.testonly.patch3.93 KBBTMash
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Subscribing.

yched’s picture

#1302228: field_has_data() returns inconsistent results – possible data loss was marked as a duplicate.
However, this latter issue outlines a nasty consequence : field_has_data() relies on EFQ and thus gets affected by node_access rules. It can then incorrectly return "nope, the field has no data".
This badly affects field_update_field() and the Field UI, which use field_has_data() to only allow some updates if the field is empty.

Damien Tournoud’s picture

An easy fix for those queries is to add an 'account' in the metadata:

$query->addMetadata('account', user_load(1));
catch’s picture

Priority: Normal » Critical

The other issue was critical, bumping this one as well, account metadata sounds like a good quick fix.

yched’s picture

The EFQ behavior described in the OP (sometimes node_access, sometimes not) sounds broken to me, but I'm not sure we can change that in D7 now (i.e. removing hardcoded node_access on some existing EFQs) ?
In that case, #3 might be our only workaround for D7. But for D8 ? Do we really want to leave EFQ that way ?

smk-ka’s picture

Subscribing.

yched’s picture

Damien's workaround in #3 works as a hotfix for #1302228: field_has_data() returns inconsistent results – possible data loss, so I reopened that issue and posted a patch over there.

Leaving this issue open for the more general - IMO broken - behavior of EFQ regarding node_access.

chx’s picture

This must be an oversight in the entity.inc code -- there we could add a check whether we are on node or not. fields are trickier because the entity type can change result row by result row.

tim.plunkett’s picture

Tagging.

aspilicious’s picture

summary is written

BTMash’s picture

Status: Needs work » Needs review
FileSize
3.93 KB

As mentioned above, currently, writing out entity field queries will return empty resultsets when dealing with field conditions. As far as I have see, the the specific scenario where this happens is when:

  • You have an entity field query on non-node entities (user, comment, taxonomy in core; relation, media, eck, and a slew of others out in contrib).
  • if you have a node access module enabled (such as OG, domain access, workbench access, and a slew of others out in contrib).
  • You are using core's Field SQL Storage module.
  • You have content that some users are able to see and some are unable to see (based on the node access module)

I'm attaching a test that shows how this fails in D8 but if you want to replicate it in D7, here is one (somewhat lengthy) example process.

  • Download and install Drupal 7.
  • Download and enable a content access module - like Content Access.
  • Create content and allow some of that content to be *not* be viewable by certain non-admin users (like anonymous).
  • Download eck module.
  • Create an arbitrary type of entity (lets say 'sample') and bundle (lets say 'sample_bundle') and create some fields to attach to the bundle.
  • Create 'sample_bundle' content.
  • Download CTools, Views, and EntityFieldQuery Views
  • Set up a view for your entity that is filtered by a condition on a field on that 'sample_entity' bundle and make sure it works for you admin user (ie - it returns back results) and save as a page.
  • Visit said page as an anonymous user.

By this lengthy point, you should end up with being able to see a resultset as an admin user (or a user that has the ability to bypass node access rules / view all nodes) and being able to see no results as an anonymous user.

The cause of this stems from a check in hook_field_storage_query() where it adds the tag entity_field_access. The node module implements this tag for query alters and thus a check against non-node entities ends up here whereby the query gets joined with node_access tables.

On a side note, since this was mentioned in #5 and #7, there seems to be a generic issue on this in #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() though that issue also seems to be somewhat stuck.

Status: Active » Needs work

The last submitted patch, 997394.testonly.patch, failed testing.

BTMash’s picture

Status: Needs review » Closed (duplicate)

This issue seems to be a duplicate of #1571104: Can't access non-node entities with EntityFieldQuery so I'm marking this as a duplicate as that issue seems to have more activity.

Berdir’s picture

It might be a duplicate of what you're reporting in #11, but I'm not sure the issues actual purpose is a duplicate. As per @yched's comment in #7:

Leaving this issue open for the more general - IMO broken - behavior of EFQ regarding node_access.

Maybe re-open as a non-critical issue (not sure if bug or task)? I'm quite confused by the current behavior, as stated before.

Berdir’s picture

Issue summary: View changes

Addition of Issue Summary at DrupalCon Denver Core sprint

m4olivei’s picture

For those of us led to this issue and looking for a practical solution, would it suffice to simply always add addTag('node_access') to an EFQ (if it's a node query)? That way you don't have to remember that it's OK to leave it off if you do have field conditions.

Thanks a bunch!