Problem/Motivation
Why issue is filed:
EntityFieldQuery (EFQ) processing is not consistent and will cause confusion.
Example / Reproduction:
- if EFQ has a FieldCondition, then node_access_grants is checked
- 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:
- Remove hardcoded node_access check in EFQ
- Support the node_access check in EFQ. It is up to the caller add the tag
- The tag should be removed from field_sql_storage_field_storage_query()
- 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
- cradle to grave impact analysis of *where* the fix needs to happen and what may break when implementing solution
- identify the interaction behavior between entity.inc and EFQ. define if any work to entity.inc needs to be added in the proposed solution
- identify proposed solution, review with those on the comment list for feedback.
- 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 ?
Comment | File | Size | Author |
---|---|---|---|
#48 | drupal-1571104-48.patch | 5.58 KB | tim.plunkett |
#43 | 1571104-43.patch | 5.97 KB | lliss |
#39 | 1571104.34_1-d7.patch | 5.58 KB | mradcliffe |
#37 | 1571104.34_0-d7.patch | 3.36 KB | mradcliffe |
#36 | 1571104.34.patch | 5.88 KB | BTMash |
Comments
Comment #1
catchBumping to critical.
Comment #2
catchHmm 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.
Comment #3
Dave.Ingram CreditAttribution: Dave.Ingram commentedI agree this is a duplicate--just fixing the title.
Comment #4
lkiss80 CreditAttribution: lkiss80 commentedWhy 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.
Comment #5
Mark TrappSee 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.
Comment #6
lkiss80 CreditAttribution: lkiss80 commentedThanks 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!!
Comment #7
lkiss80 CreditAttribution: lkiss80 commentedAfter reviewing the Backport Policy, I believe this bug falls under the following statement:
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.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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:
while it obviously needs to be:
The patch should be an simple one-liner (untested Drupal 7 version attached).
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #12
Dave.Ingram CreditAttribution: Dave.Ingram commentedIt 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.
Comment #13
Dave.Ingram CreditAttribution: Dave.Ingram commentedah, botched that last patch. Let's try that again.
Comment #14
salvisMarked #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.
Comment #15
salvisComment #16
BerdirI 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.
Comment #17
Berdir#13: node-access-entity-1571104-13.patch queued for re-testing.
Comment #18
Dave.Ingram CreditAttribution: Dave.Ingram commentedThere 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.
Comment #19
Dave.Ingram CreditAttribution: Dave.Ingram commentedSorry Berdir, cross-posted there.
Comment #20
Dave.Ingram CreditAttribution: Dave.Ingram commentedOK, I've rerolled the patch for D8 so it finds the core directory. Everything else is the same as in #18.
Comment #23
chx CreditAttribution: chx commentedThanks 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?
Comment #24
Dave.Ingram CreditAttribution: Dave.Ingram commentedI'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.
Comment #25
lkiss80 CreditAttribution: lkiss80 commentedPatch in #18 worked for me. Thank you very much for this.
Comment #26
sunCleaned up coding style and docs. Still needs tests.
Comment #27
webchickHm. Rats.
Comment #28
BTMash CreditAttribution: BTMash commentedAnother 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.
Comment #29
BTMash CreditAttribution: BTMash commentedargh...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.
Comment #30
BTMash CreditAttribution: BTMash commentedThe 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.
Comment #31
BTMash CreditAttribution: BTMash commentedMy 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.
Comment #32
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank you. Here's a superficial coding style review:
This comment should be wrapped at 80 characters.
Helper functions. And hook implementations?
Should start with a 3rd person verb like "Returns". "Example" should not be capital."
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?
Again, avoid t() if not needed.
Comment #33
tim.plunkettEither 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,
Don't use t() for assertion messages themselves (the "Returned access" part).
Comment #34
BTMash CreditAttribution: BTMash commentedThank @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
with
Comment #35
BerdirExample should be lowercase.
Comment #36
BTMash CreditAttribution: BTMash commentedDoh!
Attaching patch with fix.
Comment #37
mradcliffeBackport from #36.
I moved the tests to modules/simpletest/tests.
Edit: oh, crap. I forgot to add/diff the newly created files :(
Comment #39
mradcliffeSorry for spam. Bad patch.
Comment #41
BerdirWhen 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.
Comment #42
ZenDoodles CreditAttribution: ZenDoodles commented#36: 1571104.34.patch queued for re-testing.
Comment #43
lliss CreditAttribution: lliss commentedRerolling because psr-0 changes caused the last one to no longer apply.
Comment #44
aspilicious CreditAttribution: aspilicious commentedCan someone tests this? :s
Comment #45
smk-ka CreditAttribution: smk-ka commentedPatch from #39 tested on D7.14, works as expected.
Comment #46
chx CreditAttribution: chx commentedThanks 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:
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.Comment #47
webchickThanks 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.
Comment #48
tim.plunkettRerolled.
Comment #49
mgiffordThe patch in #48 applies nicely to our local D7.14 install.
@smk-ka have you tried the patch above?
Comment #50
August1914 CreditAttribution: August1914 commentedApplies cleanly to 7.14, node testcases all pass, code and comments are good. Sweet.
Comment #51
Dave ReidConfirmed this fixes my issues with EntityFieldQuery with non-node entities. This one looks good to go for D7.
Comment #52
acbramley CreditAttribution: acbramley commentedAwesome, 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!
Comment #53
mradcliffe#48: drupal-1571104-48.patch queued for re-testing.
Comment #54
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/bada576
Comment #56
wwgrey CreditAttribution: wwgrey commentedNot 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'.
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.
Comment #57
wwgrey CreditAttribution: wwgrey commentedFound a work-around: removed the entity condition on 'bundle'. My EFQ becomes:
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.
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
It's my own defined entity with a 'bundle' defined in my entity_info, see #56, so I would expect it to be supported...
Comment #58
Dave Reid@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.
Comment #59
wwgrey CreditAttribution: wwgrey commented@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.
Comment #60
dpiremoving d7 backport tag
Comment #60.0
dpiUpdated issue summary with work by tim.plunkett from #997394: field_sql_query hardcodes a 'node_access' tag on EFQs with a fieldCondition