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 ?

Files: 
CommentFileSizeAuthor
#48 drupal-1571104-48.patch5.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,301 pass(es).
[ View ]
#43 1571104-43.patch5.97 KBlliss
PASSED: [[SimpleTest]]: [MySQL] 36,869 pass(es).
[ View ]
#39 1571104.34_1-d7.patch5.58 KBmradcliffe
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571104.34_1-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 1571104.34_0-d7.patch3.36 KBmradcliffe
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571104.34_0-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#36 1571104.34.patch5.88 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571104.34_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 1571104.34.patch5.88 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 36,685 pass(es).
[ View ]
#31 1571104.testonly.patch4.06 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 36,682 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#31 1571104.combined.patch5.83 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 36,692 pass(es).
[ View ]
#29 1571104.combined.patch5.67 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 36,698 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#28 1571104.testonly.patch3.93 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 36,697 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#28 1571104.combined.patch1.75 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 36,650 pass(es).
[ View ]
#26 drupal8.node-access-entity.26.patch1.75 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,652 pass(es).
[ View ]
#20 node-access-entity-1571104-20.patch1.72 KBDave.Ingram
PASSED: [[SimpleTest]]: [MySQL] 36,610 pass(es).
[ View ]
#18 node-access-entity-1571104-16.patch1.7 KBDave.Ingram
PASSED: [[SimpleTest]]: [MySQL] 39,111 pass(es).
[ View ]
#13 node-access-entity-1571104-13.patch624 bytesDave.Ingram
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-access-entity-1571104-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 node-access-entity-1571104-12.patch1002 bytesDave.Ingram
FAILED: [[SimpleTest]]: [MySQL] 38,803 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#8 1571104-node-access-entity.patch421 bytesDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] 38,524 pass(es), 145 fail(s), and 226 exception(s).
[ View ]

Comments

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

Bumping to critical.

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.

Title:Can't access non-node entities with EntityFetchQuery after updating form 7.12 to 7.14Can'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.

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.

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.

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!!

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.

Version:8.x-dev» 7.x-dev
Status:Closed (duplicate)» Active
StatusFileSize
new421 bytes
FAILED: [[SimpleTest]]: [MySQL] 38,524 pass(es), 145 fail(s), and 226 exception(s).
[ View ]

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).

Status:Active» Needs review

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

Status:Needs review» Needs work

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

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
StatusFileSize
new1002 bytes
FAILED: [[SimpleTest]]: [MySQL] 38,803 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new624 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-access-entity-1571104-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

Version:7.x-dev» 7.14

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.

#13: node-access-entity-1571104-13.patch queued for re-testing.

Version:8.x-dev» 7.x-dev
Issue tags:-Needs tests
StatusFileSize
new1.7 KB
PASSED: [[SimpleTest]]: [MySQL] 39,111 pass(es).
[ View ]

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.

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

Sorry Berdir, cross-posted there.

Title:Can't access non-node entities with EntityFieldQuery after updating form 7.12 to 7.14Can't access non-node entities with EntityFieldQuery
StatusFileSize
new1.72 KB
PASSED: [[SimpleTest]]: [MySQL] 36,610 pass(es).
[ View ]

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

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?

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.

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

StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 36,652 pass(es).
[ View ]

Cleaned up coding style and docs. Still needs tests.

Issue tags:+7.15 release blocker

Hm. Rats.

StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 36,650 pass(es).
[ View ]
new3.93 KB
FAILED: [[SimpleTest]]: [MySQL] 36,697 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Another version of this issue exists (also as critical) at #1126000: 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.

StatusFileSize
new5.67 KB
FAILED: [[SimpleTest]]: [MySQL] 36,698 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new5.83 KB
PASSED: [[SimpleTest]]: [MySQL] 36,692 pass(es).
[ View ]
new4.06 KB
FAILED: [[SimpleTest]]: [MySQL] 36,682 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

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).

StatusFileSize
new5.88 KB
PASSED: [[SimpleTest]]: [MySQL] 36,685 pass(es).
[ View ]

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.

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.

StatusFileSize
new5.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571104.34_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Doh!

Attaching patch with fix.

StatusFileSize
new3.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571104.34_0-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new5.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571104.34_1-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Sorry for spam. Bad patch.

Status:Needs review» Needs work

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

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.

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

StatusFileSize
new5.97 KB
PASSED: [[SimpleTest]]: [MySQL] 36,869 pass(es).
[ View ]

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

Can someone tests this? :s

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

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:

<?php
     
// 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.

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new5.58 KB
PASSED: [[SimpleTest]]: [MySQL] 39,301 pass(es).
[ View ]

Rerolled.

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

@smk-ka have you tried the patch above?

Status:Needs review» Reviewed & tested by the community

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

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

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!

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

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.

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',
    ),

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...

@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.

@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.

Issue tags:-needs backport to D7

removing d7 backport tag

Issue summary:View changes