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 ?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Version: 7.14 » 8.x-dev
Priority: Major » Critical
Issue tags: +Needs backport to D7

Bumping to critical.

catch’s picture

Status: Active » Closed (duplicate)

Hmm actually I think this is a duplicate of #997394: field_sql_query hardcodes a 'node_access' tag on EFQs with a fieldCondition, although it's possible 7.14 introduced a change that made this even worse. Marking duplicate.

Dave.Ingram’s picture

Title: Can't access non-node entities with EntityFetchQuery after updating form 7.12 to 7.14 » Can't access non-node entities with EntityFieldQuery after updating form 7.12 to 7.14

I agree this is a duplicate--just fixing the title.

lkiss80’s picture

Why is this bug marked as a duplicate bug for a completely different version of Drupal? This is a critical issue and will receive less attention if it's associated with Drupal 8, since drupal 8 is in development. Everywhere that EntityFieldQuery is used, within my codebase, within all the contributed modules, and within core, are affected. A simple, yet effective example is that any image attached to an entity will not load unless the administrator is logged in. While this example doesn't portray the critical nature of the issue, it does show that even other core modules are affected.

Does someone have a good workaround? If not, I imagine reverting to 7.12 is in my near future.

Mark Trapp’s picture

See the Backport Policy: bugs are fixed in the development branch (i.e., 8.x) and then backported, if needed, to the release/maintenance branch (7.x). Subsequently, most development eyes are on the 8.x branch because that's where all the active development is.

lkiss80’s picture

Thanks for the response Mark. I understand what you're saying and why they have this type of policy in place. However, I don't completely agree that this policy should apply to this bug. The reason is because the proposed solution for drupal 8 that would solve this issue is a redesign of an entire system. Waiting for an entire system redesign, and then a backport, seems overkill, for a small bug introduced in a released product. If the solution for drupal 8 were a simple few lines of code, that could easily be backported, I would agree with this policy.

What's frustrating is that this bug was introduced to drupal 7 in either 7.13 or 7.14, which was very recent. However, the "duplicate" bug was opened in Dec 2010. It just seems odd that a bug recently introduced in drupal 7, has been marked as a duplicate of a bug found in drupal 8 over 15 months ago and won't get fixed for drupal 7 (a released product) before drupal 8 (an unreleased product).

I think reverting to 7.12 is probably my best bet instead of trying to fight the system. Just wanted to throw my $0.02 in!!

lkiss80’s picture

After reviewing the Backport Policy, I believe this bug falls under the following statement:

"Sometimes, the ideal way to fix the bug in Drupal 8 may involve far-reaching changes or refactoring that would not be appropriate to backport. In those cases, it is best to create a separate Drupal 8 issue for the refactoring and cross-link it to the original issue. This allows the minimal fix to be committed to Drupal 8 and backported as quickly as possible, while work on the more far-reaching fix for Drupal 8 can continue at its own pace."

Can someone recommend what the next steps are for me? Should I create a separate Drupal 8 issue for the minimal fix that could be backported? Even if the hotfix posted in the duplicate bug would work, there are numerous contributed modules that I would have to hack in order to get this working. That is not something I'm comfortable doing, especially because I don't have the means to test each individual module to ensure no security holes were created.

Any advice would be appreciated.

Damien Tournoud’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (duplicate) » Active
FileSize
421 bytes

This is clearly not a duplicate of the other issue. This issue is about the node access query being broken (again). The other issue is about when/how to tag the different kinds of EFQ queries.

If I read the original post properly, this is the query that is generated:

EXISTS(node access) AND ( (entity_type = 'node') OR (entity_type <> 'node') )

while it obviously needs to be:

( EXISTS(node access) AND (entity_type = 'node') ) OR (entity_type <> 'node')

The patch should be an simple one-liner (untested Drupal 7 version attached).

Damien Tournoud’s picture

Status: Active » Needs review
Damien Tournoud’s picture

Version: 7.x-dev » 8.x-dev

Status: Needs review » Needs work

The last submitted patch, 1571104-node-access-entity.patch, failed testing.

Dave.Ingram’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
1002 bytes

It seems like we only need to make the change in #8 for entity queries. This patch adds the subquery to $entity_conditions only if it's an entity query. Otherwise, the subquery is added to the main $query object.

Dave.Ingram’s picture

ah, botched that last patch. Let's try that again.

salvis’s picture

Marked #1588550: Regression in 7.14: Cannot download files attached to comments in protected forums as a duplicate of this issue. The patch in #13 fixes it.

BTW, reverting to 7.13 fixes it, too. No need to go back to 7.12.

salvis’s picture

Version: 7.x-dev » 7.14
Berdir’s picture

Version: 7.14 » 8.x-dev
Issue tags: +Needs tests

I guess this needs a test then. Also, this needs to be fixed in 8.x first and then backported. The entity field query system has not changed much as far as I know in 8.x, the clause from #7 does not apply here IMHO.

Berdir’s picture

Dave.Ingram’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: -Needs tests
FileSize
1.7 KB

There is some ambiguity in what's happening in this function because of the name of $entity_conditions. It appears we can better name that $node_conditions as this is the set that eventually gets grouped with "entity_type = 'node'". I've updated the code comments here and renamed the variable in this patch to hopefully make this clearer.

On a different note, we will need a test to check against this problem in the future, but I'm not sure if it is best placed in node.test or elsewhere since the thing that's broken doesn't have anything to do with nodes, it's just that this part of the node module is breaking it.

Finally, as per comment #2 and #7 above, I believe that #2 may hold the keys to the solution for D8, but not for D7, so hopefully this issue can be used to resolve the immediate and potentially production stopping bug, and a more robust solution can be considered for D8.

Dave.Ingram’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs tests

Sorry Berdir, cross-posted there.

Dave.Ingram’s picture

Title: Can't access non-node entities with EntityFieldQuery after updating form 7.12 to 7.14 » Can't access non-node entities with EntityFieldQuery
FileSize
1.72 KB

OK, I've rerolled the patch for D8 so it finds the core directory. Everything else is the same as in #18.

chx’s picture

Thanks for fixing this. This obviously got broken in #681760: Try to improve performance and eliminate duplicates caused by node_access table joins :/ pity. Anyways, we really need tests. How can we test this?

Dave.Ingram’s picture

I'm thinking that the patch from http://drupal.org/node/997394#comment-5995802 has relevance here, but I ran it locally against my patch above and it still fails the tests.

Also, this test won't be appropriate for a D7 solution as it runs off of the Entity module which we don't have in D7 core. So I'm not certain what the correct approach would be there.

lkiss80’s picture

Patch in #18 worked for me. Thank you very much for this.

sun’s picture

Cleaned up coding style and docs. Still needs tests.

webchick’s picture

Issue tags: +7.15 release blocker

Hm. Rats.

BTMash’s picture

Another version of this issue exists (also as critical) at #1126000: [meta] hook_field_*() called on fields not set in the $entity object (partial updates). So I'm adding my test case here which showed how the current revision failed. Combining it with the work done by everyone else in this issue shows that the issue now gets resolved.

BTMash’s picture

FileSize
5.67 KB

argh...that was stupid. so I didn't try it with the combined patch. Time to try again. And it looks like it still fails? I'll debug on my end on what is going on.

BTMash’s picture

Status: Needs review » Needs work

The test is not quite correct (something happened between monday and today that 1 fail has somehow become 2 fails. The tests are returning 500 errors locally so just trying to figure out what it could be.

BTMash’s picture

Status: Needs work » Needs review
FileSize
5.83 KB
4.06 KB

My error was not including the PSR changes that went into EntityFieldQuery. The patch seems to now work as intended (both the testonly and the test + patch). Attaching for testbot.

Niklas Fiekas’s picture

Thank you. Here's a superficial coding style review:

+++ b/core/modules/entity/tests/entity_query.testundefined
@@ -1526,6 +1526,25 @@ class EntityFieldQueryTestCase extends WebTestBase {
+    // Test as a user that does not have ability to bypass node access or view all nodes.

This comment should be wrapped at 80 characters.

+++ b/core/modules/entity/tests/modules/entity_query_access_test/entity_query_access_test.moduleundefined
@@ -0,0 +1,57 @@
+ * Helper function for testing EntityFieldQuery access.

Helper functions. And hook implementations?

+++ b/core/modules/entity/tests/modules/entity_query_access_test/entity_query_access_test.moduleundefined
@@ -0,0 +1,57 @@
+ * Return the results from an Example EntityFieldQuery.

Should start with a 3rd person verb like "Returns". "Example" should not be capital."

+++ b/core/modules/entity/tests/modules/entity_query_access_test/entity_query_access_test.moduleundefined
@@ -0,0 +1,57 @@
+      $results['items'][] = t('Found entity of type @entity_type with id @entity_id', array('@entity_type' => $entity_type, '@entity_id' => $entity_id));

Since this is a testing module only, we should not use t(). format_string() should be fine, instead. Or is t() needed to match an existing string or something?

+++ b/core/modules/entity/tests/modules/entity_query_access_test/entity_query_access_test.moduleundefined
@@ -0,0 +1,57 @@
+    $output = t('No results found with EntityFieldQuery.');

Again, avoid t() if not needed.

tim.plunkett’s picture

Status: Needs review » Needs work

Either the assertion strings should be wrapped in t() or the messages should, but they need to agree. It's fine to use t() in those cases.

However,

+++ b/core/modules/entity/tests/entity_query.testundefined
@@ -1526,6 +1526,25 @@ class EntityFieldQueryTestCase extends WebTestBase {
+    $this->assertText('Found entity', t('Returned access response with entities.'));
+++ b/core/modules/entity/tests/entity_query.testundefined
@@ -1526,6 +1526,25 @@ class EntityFieldQueryTestCase extends WebTestBase {
+    $this->assertText('Found entity', t('Returned access response with entities.'));

Don't use t() for assertion messages themselves (the "Returned access" part).

BTMash’s picture

FileSize
5.88 KB

Thank @Niklas Fiekas and @tim.plunkett. The review is much appreciated. I did not know about format_string so I've replaced it with that where it made sense and removed t() otherwise. I replaced

+ * Helper function for testing EntityFieldQuery access.

with

+ * Helper module for testing EntityFieldQuery access on any type of entity.
Berdir’s picture

Status: Needs work » Needs review
+++ b/core/modules/entity/tests/modules/entity_query_access_test/entity_query_access_test.moduleundefined
@@ -0,0 +1,57 @@
+ * Returns the results from an Example EntityFieldQuery.

Example should be lowercase.

BTMash’s picture

FileSize
5.88 KB

Doh!

Attaching patch with fix.

mradcliffe’s picture

FileSize
3.36 KB

Backport from #36.

I moved the tests to modules/simpletest/tests.

Edit: oh, crap. I forgot to add/diff the newly created files :(

Status: Needs review » Needs work

The last submitted patch, 1571104.34_0-d7.patch, failed testing.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
5.58 KB

Sorry for spam. Bad patch.

Status: Needs review » Needs work

The last submitted patch, 1571104.34_1-d7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

When uploading patches for a different major version, add a -do-not-test.patch suffix to avoid confusing the testbot. Also, it's usually recommended to wait with the backport because you will have to re-do any changes made to the 8.x patch.

Setting back to needs review for #36.

ZenDoodles’s picture

#36: 1571104.34.patch queued for re-testing.

lliss’s picture

FileSize
5.97 KB

Rerolling because psr-0 changes caused the last one to no longer apply.

aspilicious’s picture

Can someone tests this? :s

smk-ka’s picture

Patch from #39 tested on D7.14, works as expected.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing this bug. However, renaming entity_conditions to node_conditions within a critical bugfix seems unnecessary and makes it a lot harder to review. To make it easier to review, here is the only piece of code actually added:

      // For an entity query, attach the subquery to entity conditions.
      if ($type == 'entity') {
        $entity_conditions->exists($subquery);
      }
      // Otherwise attach it to the node query itself.
      else {
        $query->exists($subquery);
      }

whereas previous we only ran $query->exists($subquery);. So we change only in the entity case and it is correct per #8 .For the sake of progress, I will not argue the variable rename and instead name this RTBC.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs tests

Thanks for the bug fix and the test.

Committed and pushed to 8.x. This will need a re-roll for 7.x due to PSR-0.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.58 KB

Rerolled.

mgifford’s picture

The patch in #48 applies nicely to our local D7.14 install.

@smk-ka have you tried the patch above?

August1914’s picture

Status: Needs review » Reviewed & tested by the community

Applies cleanly to 7.14, node testcases all pass, code and comments are good. Sweet.

Dave Reid’s picture

Confirmed this fixes my issues with EntityFieldQuery with non-node entities. This one looks good to go for D7.

acbramley’s picture

Awesome, patch #48 fixed my issues on 7.14 with private files and hook_file_download trying to grant access to authenticated users using file_get_file_references, cheers!

mradcliffe’s picture

#48: drupal-1571104-48.patch queued for re-testing.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.15 release notes

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

wwgrey’s picture

Not sure if I need to open a new issue, but my issue seems related.

Since my ugrade from 7.14 -> 7.15 the following EFQ statement gives an empty result, while before I got the list of entities 'partners'.

  $query = new EntityFieldQuery();
  $query->entityCondition('entity_type', 'partner')
    ->entityCondition('bundle', 'partner')
    ->range(0, $limit )
    ->propertyOrderBy('partner_name');
  $entities = $query->execute();

In my understanding, a relevant modification on EFQ in 7.15 is access control. This could cause badly written legacy code in access control functions, to become active and cause failures. To exclude this, I executed the code with admin rights and with the 'partner_access' function (see hereunder) return TRUE on first line. But this didn't resolve the issue .

For completeness , hereunder the entity_info.

function uc_partner_entity_info() {

  $return = array(
    'partner' => array(
      'label' => t('Partner'),
      'entity class' => 'Partner',
      'controller class' => 'PartnerController',
      'base table' => 'uc_partner',
      'fieldable' => TRUE,
      'entity keys' => array(
        'id' => 'partner_id',
      ),
      'bundles' => array(
        'partner' => array(
          'label' => t('Partner'),
          'admin' => array(
            'path' => 'admin/store/settings/partners',
            'access arguments' => array('administer partners'),
          ),
        ),
      ),
      // define hook to be called when partner entity is loaded to add data
      'load hook' => 'partner_load',
      'view modes' => array(
        'full' => array(
          'label' => t('Admin view'),
          'custom settings' => FALSE,
        ),
      ),
      // standard Entity API label callback
      'label callback' => 'entity_class_label',
      // standard Entity API call back for the URI
      'uri callback' => 'entity_class_uri',
      // the module providing the entity type
      'module' => 'uc_partner',
      // this function will/can be used for access control
      // when in 'access callback' => 'entity_access', is used, this function will be called
      'access callback' => 'partner_access',
    ),
wwgrey’s picture

Found a work-around: removed the entity condition on 'bundle'. My EFQ becomes:

  $query = new EntityFieldQuery();
  $query->entityCondition('entity_type', 'partner')
    //   entityCondition('bundle', 'partner'   ---> remove this line 
    ->range(0, $limit )
    ->propertyOrderBy('partner_name');
  $entities = $query->execute();

Got the idea , looking at the query in the mySql query log: the where clause could obviously not give any results, and the only condition I have used is on the 'bundle', so removing it, resolved it.

Query	SELECT uc_partner.partner_id AS entity_id, 'partner' AS entity_type, NULL AS revision_id, 'partner' AS bundle
FROM 
uc_partner uc_partner
WHERE  (1 = 0)      ---> this is a strange where clause, isn't it ? 
ORDER BY uc_partner.partner_name ASC
LIMIT 11 OFFSET 0

I'm just wondering if this is a new bug in drupal core 7.15 or an error in my code ?

In the latter case , documentation should be modified.
See http://drupal.org/node/1343708 . It tells

not to use 'bundle' for taxonomy_term and comments

It's my own defined entity with a 'bundle' defined in my entity_info, see #56, so I would expect it to be supported...

Dave Reid’s picture

@wwwgrey: I believe your problem is due to the fact that you do not define which column in your table stores the entities' bundle. It should be stored as $info['partner']['entity keys']['bundle'] = 'column_name'. If your entity has no more than one bundle type, then you are running into the other core bug #1054168: EntityFieldQuery fails for entities that have no bundle.

wwgrey’s picture

@Dave Reid: you are right, it's an error in my code, which didn't cause any problems in the previous Drupals releases 7.13 and 7.14. Thanks for you help.

dpi’s picture

Issue tags: -Needs backport to D7

removing d7 backport tag

dpi’s picture

Issue summary: View changes