Problem/Motivation
The _node_query_node_access_alter()
helper function, which is called from node_query_entity_field_access_alter()
, seems to be blocking access to nodes that are left joined to another entity table where the latter entity does not exists. If node table is left joined, then access shouldn't be performed on "null" referred nodes which will cause to filter out nodes on the base table (like using inner join).
This issue was verified when trying to use views module in conjunction with the entity reference module and creating a view with a relationship through an entity reference. I tracked down the caused based on what codisman (http://drupal.org/user/2305632) mentioned and ended up in the function mentioned above which seems to be responsible for the issue mentioned. The issue was found in Drupal core 7.21 (and several later versions up to at least 7.34), views 7.x-3.5 and entity_reference 7.x-1.0. Some more info on how to reproduce this can be found in a post in the entity reference issue queue #1808876: SQL Rewrite blocking access to nodes that have a Referencing entity relationship in views.
Proposed resolution
Patch for Drupal core 7.x attached in comment 28. (The original patch in #2 works, but does not follow Drupal coding standards.) Needs to be fixed in 8.x and backported.
Reproduction and testing steps
- Install module attached in #1969208-85: Query node access alter filtering out accesible nodes
- Create 2 regular users (user1 and user2)
- Give authenticated users the permission to create articles
- Attach a "field_private" field of type "List (integer)" to the article content type which allows only one value. The allowed values for it should be:
0|Public 1|Private
- Attach an entity reference to the page content type named field_related_article. It should allow node of type article
- Login with "user1". Create 2 articles. Leave one as default and the second one make it private by selecting the private value in the field_private select box
- Login with the admin and create 3 pages
- One with no related article
- one using the public article as reference
- one using the private article
- Login with "user1" and go to /test/access page.
- Expected result: a table with 3 rows, one for each page
- Actual result: a table with 2 rows, missing the page with no related article referenced
- Login with "user2" and go to /test/access page.
- Expected result: a table with 2 rows, one for each page
- Actual result: a table with 1 row, missing the page with no related article referenced
- Login with an admin and enable the "View private content" permission for authenticated users
- Login with "user2" and go to /test/access page.
- Expected result: a table with 3 rows, one for each page
- Actual result: a table with 2 rows, missing the page with no related article referenced
- Login with an admin and disable the "View private content" permission for authenticated users
- Apply the patch you wish to test
- Repeat steps 8 - 11 and now the actual result should mach the expected results
Remaining tasks
- Backport to 7.x
User interface changes
None
API changes
None
Related Issues
#1808876: SQL Rewrite blocking access to nodes that have a Referencing entity relationship in views and #2273849: Convert INNER joins to LEFT joins for relationships without "Require this relationship"
Comment | File | Size | Author |
---|---|---|---|
#88 | node_access_for_left_joins_d7-1969208-89.patch | 7.34 KB | Nephele |
| |||
#85 | test_reference_access.tgz | 2.55 KB | tic2000 |
#83 | 1969208-83.tests-fix.patch | 14.89 KB | tic2000 |
#83 | 1969208-78-83.interdiff.txt | 1.29 KB | tic2000 |
#78 | 1969208-78.tests-fix.patch | 14.87 KB | tic2000 |
Comments
Comment #2
Ericmaster CreditAttribution: Ericmaster commentedSorry, I created the patch from outside of the drupal directory root, here is with the path updated
Comment #3
Ericmaster CreditAttribution: Ericmaster commentedforgot to update the status
Comment #4
BerdirThanks for starting to work on this. This needs a test and has to be fixed in 8.x first and then backported.
Comment #5
BerdirAdding backport tag.
Comment #6
BerdirComment #7
fenda CreditAttribution: fenda commentedPatch in #2 worked for me in D7.
Comment #8
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
Comment #9
BrockBoland CreditAttribution: BrockBoland commentedNeeds an issue summary: http://drupal.org/node/1427826
Comment #10
jasonyarringtonI am working on a summary for this at Drupalcon Portland
Comment #11
jasonyarringtonRemoving Needs Issue Summary tag
Comment #12
markdorisonThis was an interesting issue for me to identify and track down. The patch in #2 seems to work as expected in D7.
Comment #13
hansfn CreditAttribution: hansfn commentedThis was an unpleasant issue to track down ;-) Anyway, the patch in comment #2 works for both Drupal 7.22 and 7.23.
So, basically this issue is now RTBC, but nothing will happen since there is no D8 patch ... I'll see what I can do.
Comment #13.0
hansfn CreditAttribution: hansfn commentedAdded an issue summary
Comment #14
hansfn CreditAttribution: hansfn commentedJust for the record: The patch in comment #2 works for the newly released Drupal 7.24 too.
Comment #15
shandman CreditAttribution: shandman commentedStill works in 7.28.
Comment #16
spacer CreditAttribution: spacer commentedI've spent most of the day trying to figure this out - until I came across this post.
The patch fixed the issue - thanks Ericmaster.
Any news on if/when this is likely to roll into core?
Comment #17
ajmantis CreditAttribution: ajmantis commentedThis patch works in 7.27
Thanks!
Comment #18
benjifisher@spacer: The policy is to fix problems in the development version (currently Drupal 8) first, then backport the fix to the production version (currently Drupal 7). (See @Berdir's comment, #4 above, and the issue summary.)
If you want to help move this issue forward, then the first step is to reproduce the bug on a D8 site. In D8, Views and Entity API are in core; not sure about Entity reference, and I do not think Content access (or any other node-access module) is available in D8 core.
Unfortunately, the documentation of
_node_query_node_access_alter()
is pretty skimpy, so it is hard to say whether the observed behavior is actually a bug. Maybe we could invoke the principle that all undocumented features should be considered bugs.Comment #19
benjifisherComment #20
flocondetoileThanks @Ericmaster
Patch #2 solved my issue in D7.28 with content_access, views and entity reference.
Comment #21
weekbeforenextPatch #2 solved my issue in Drupal 7.31.
Comment #22
Leeteq CreditAttribution: Leeteq commentedIs it completely out of the question to commit this to D7 before we have a D8 version of this simple patch?
This is important, especially on D7 as more and more depends on Entity Reference.
Comment #23
dcam CreditAttribution: dcam commentedYes, it is. The 7.x committer isn't going to commit anything that will knowingly introduce a regression. That 7.x patch isn't even complete anyway. It would need tests before it could be committed.
Maybe you could help create an 8.x patch. That would help get this issue backported to 7.x faster.
Comment #24
hansfn CreditAttribution: hansfn commentedI can write the patch for D8, but we need to create a test too.
Does anyone have a test case (that could be codified as a simpletest for D7) that I can port to D8?
Comment #25
benjifisher@hansfn:
The last time I tried, I could not figure out how to reproduce the problem in D8. Even though Views and Entity API (and Entity reference?) are in core, I could not figure out how to make a view that illustrates the problem. After that, you will still have to (at least) write a module that implements
hook_node_grants()
, or whatever the analogue is for D8.I just noticed that Drupal 7.28 added some comments to the doc block on
_node_query_node_access_alter()
:See #1885420: Document the use of metadata to indicate a base table for node access queries and the git commit.
Best case scenario: this problem has already been fixed in D8, in which case we are free to work on this issue in D7.
I have re-rolled the patch from #2 to match Drupal coding standards. Nothing complicated: limit comment lines to 80 characters, and use standard whitespace in control structures. I have tested on Drupal 7.31, but I have not studied the code enough to have an opinion on whether this is the right way to fix the problem. I do plan to use the patch on a live site.
I am temporarily setting the version to 7.x-dev so that the testbot will check my patch.
For coding standards, see https://www.drupal.org/coding-standards#controlstruct and https://www.drupal.org/node/1354#drupal.
Comment #26
benjifisherI still want the testbot to check my patch, so I am setting this to NR. The issue should be returned to 8.0.x/NW once the testbot is done.
Looking at the code, I see that the lines being patched are now the end of the
alterQuery()
method in NodeGrantDatabaseStorage (in core/modules/node/src/NodeGrantDatabaseStorage.php). I do not have time to work on a patch for D8, but maybe this will save some time for someone else.Comment #28
benjifisherOuch, my patch included some stuff from my theme folder. Let me try again: still trying for a D7 version of @Ericmaster's patch, with a few changes to comply with Drupal coding standards.
Comment #29
benjifisherComment #30
weekbeforenextPatch #28 worked for me in Drupal 7.32.
Thanks!
Comment #31
bforchhammer CreditAttribution: bforchhammer commentedI can say that this problem still very much exists in the current code-base for D8. However, views currently more or less forces relationships to be "required" as soon as you use any kind of field data (because `node_field_data` is always joined using an
INNER JOIN
at the moment), which means that having "empty references" is not possible anyway. For more details see #2273849: Convert INNER joins to LEFT joins for relationships without "Require this relationship".FWIW, we just ran into this issue and I can say that the solution outlined in #28 still works very well!
Comment #32
LGLC CreditAttribution: LGLC commented#28 worked for me on Drupal 7.34. Thanks!
Comment #33
benjifisher@TechNikh and I are looking at this today (Boston instance of the global sprint weekend).
Comment #34
TechNikh CreditAttribution: TechNikh commentedrewritten the patch in #28 for Drupal8
Query for admin:
Query for non-admin who doesn't have permission to node_access_view_all_nodes function but has permission through node_access before the fix in the patch:
Query for non-admin who doesn't have permission to node_access_view_all_nodes function but has permission through node_access after the fix in the patch:
Comment #35
TechNikh CreditAttribution: TechNikh commentedSteps to reproduce & test in Drupal8:
Add below code snippets to a test module replacing hook "modulename" with appropriate module name.
required for bug with INNER joins. More at https://www.drupal.org/node/2273849#comment-9383433
After adding the code, clear cache at /admin/config/development/performance and "Rebuild permissions" at /admin/reports/status/rebuild
Add entity reference field "field_related_pages" on article content type referencing content of type page. (non required field)
create a page and two articles. Only one article references the page. Leave the entity reference field empty for the other article.
Import the attached view at /admin/config/development/configuration/single/import
Look at the view page as anonymous user. Before the patch, you will only see one article which referenced the page even though anonymous user has access to all articles.
Apply the patch and now you should see both the articles.
Comment #36
benjifisherI agree with @TechNikh in #34 and #35: the bug is reproducible in D8, and the same change fixes the problem (at least in simple tests).
I have attached a zip file: expand it in your Drupal root directory, and it will create two modules (
viewsfix
andmynodeaccess
) in yourmodules/
(notcore/modules)
directory. This makes it easier to test the problem.Here is a simplified version of the instructions in #35 for reproducing the bug:
field_related_pages
) to the Article type, an entity reference pointing to a "Basic page" node.admin/config/development/configuration/single/import
) attached to #35.viewsfix
module./articles
: all the Articles will be listed, with or without the linked Basic page.mynodeaccess
module.admin/reports/status/rebuild
/articles
: admin still sees all articles, but anonymous users will only see those with an entity reference./articles
: admin and anonymous users will again see the same thing.Comment #37
benjifisherComment #38
Ericmaster CreditAttribution: Ericmaster commentedComment #39
FerConde CreditAttribution: FerConde commentedI been testing the patch in #34 and after applying it the issue was still there, I'm gonna try to fix it right now. Greetings from DrupalCon Latin America
Comment #40
constrict0r CreditAttribution: constrict0r commentedI tested the patch on #34 with another two friends (using different computers) and it works correctly. One of the guys has to clear the cache a lot of times but it works.
We've tested it on Debian Wheezy, Ubuntu 14.04 and Ubuntu mint 17.
Greetings from DrupalCon Latin America.
Pura vida mae
Comment #41
Ericmaster CreditAttribution: Ericmaster commentedJust re-rolled the patch since it applied with an offset
Comment #42
constrict0r CreditAttribution: constrict0r commentedI just tested the new patch in #41 and it works too, cool.
Comment #43
argosbass CreditAttribution: argosbass commentedI applied and tested patch on #41 and it's works. Changing status to "reviewed & tested by the community"
Greetings from DrupalCon Latin America.
Comment #44
alexpottGiven that this is access related we definitely need to add a test.
Comment #45
benjifisherI would love to have this issue fixed. Like a lot of other people, I have patched D7 on a live site. BUT I think that it cannot properly be tested until #2273849: Convert INNER joins to LEFT joins for relationships without "Require this relationship" is fixed. See @TechNikh's comments in #35 and mine in #36.
Comment #46
Jānis Bebrītis CreditAttribution: Jānis Bebrītis commented#2 patch helped with D7.
I had exact problem with non-existent referenced content + node grants - admin could see all, but other users couldn't.
Thanks @Ericmaster !
Comment #47
lokapujyaNeed to start somewhere. Lets just use the "viewsfix" module until that other issue is fixed.
Comment #49
lokapujyaA little more work.
Comment #51
benjifisher@lokapujya , since this is a test-only patch, is it a good thing that it is failing? That is, are you trying to write a test that will fail until this issue is fixed? Looking at the test results, I think it is an unrelated test that is failing.
You mention the viewsfix module; for the sake of others reading this, I assume you mean the one attached to #36 above.
Comment #52
lokapujyaYes, it should fail. But, it still needs a lot of work before it will fail for the right reason. I don't know why it failed a migration test, haha.
Yeah, viewsfix is the one in #36 that we need in order to fix the joins. Of course, it is a theoretical fix since that is assuming the joins get fixed that way.
In this interdiff:
- added an entity_reference field.
- added the field to article.
- added the view in #35.
I'm making slow improvements because I only have a little amounts of time to work on this.
Todo:
Actually load the view from #35 in the test.
Load the view with different user roles.
Fix any missing dependencies in loading the view.
Write some assertions.
Simplify / Clean up the view and test; Remove any code/configuration that isn't needed.
Comment #54
lokapujyaFix some bugs, create the article content type.
Also, the last interdiff wasn't complete, so I redid the interdiff.
Comment #56
lokapujyaJust adjusting the view so that it will load.
Comment #57
lokapujyaGood, the test is almost working. Next we need this part.
Comment #58
lokapujyaThe test is a lot closer now to actually testing the issue now.
Comment #59
lokapujyaA couple more fixes. The last assert should fail but it doesn't; I think for this test to work as intended that it would need to do a drupalGet() instead of view->execute().
Comment #61
mgiffordComment #62
lokapujyaRerolling the view.
Comment #63
lokapujyaI manually tested the issue, and the
bugproblem is still there. We have a fix in #34 (and the related issue.) I was last working on the test. The comment in #59 still stands. Since the last patch was a test only patch, it should actually be failing. Once the test properly fails -- the fix can be added back and the patch will be complete. Any takers to try to fix the test so that it properly fails?Comment #65
cattmote CreditAttribution: cattmote as a volunteer commentedThe patch in #28 solved the issue for me as well without any other side effects.
Comment #66
lokapujyaChanged test to use drupalGet() instead of view->execute() so that access will be considered.
Comment #69
lokapujyaShould have been 1 fail and 0 fails, but: views_test_remove_inner_joins can be probably be removed completely (based on the last comment in https://www.drupal.org/node/2273849). Documentation needs fixing in views_test_mynodeaccess module.
Comment #70
lokapujyaremoved views_test_remove_inner_joins.
Comment #72
lokapujyaComment #73
tic2000 CreditAttribution: tic2000 commentedThe test should also provide a test for a user that doesn't have access to the reference and can't see the entry in the view.
I don't think we need to test before enabling the module that alters access. I'm sure that case is covered by other tests.
The test needs:
- 2 articles. one private and one public.
- 3 pages. One with no reference, one with reference to the public article, one with a reference to the private article.
- 2 regular users (admin user doesn't count cause in that case access is not even checked by the view). One with access to the private article, one without.
- A view to list pages with relationship to article and showing the article title
- Tests that the user with access to the private article can see all 3 entries and that the user without access can see only 2 entries, also that those are the entries he is supposed to see.
Comment #74
lokapujya@tic2000 What is a private article?
Comment #75
tic2000 CreditAttribution: tic2000 commentedAn article which a user doesn't have access to view. Because right now the test only covers the fact that a view will hide entries with null reference, but doesn't make sure that also a user that should not see a reference will not see it.
Comment #76
lokapujyaAccess is not controlled per node. So, would this test require having 2 different content types?
Comment #77
tic2000 CreditAttribution: tic2000 commentedIf I understand this correctly you refer to the fact that in this case the issue is a NULL value.
But even if this is the reported bug, we still need to make sure access on a node is checked when there is a non null value in a left join. Using the test as is and commenting out everything inside
if ($tableinfo['join type'] == 'LEFT') {
the tests will pass. They shouldn't pass, we should have an error telling us we see a node we shouldn't see. Right now your test doesn't provide such a node and doesn't test for it.Three more remarks:
1.
} else {
Else should be on a new line.
2. I think the tests should be inside the node module. Even if the original report came from an issue in a view, the problem originates from the node module. While we can use a view to test it, the same issue would appear if we used a custom page with a db_select query with 'node_access' tag.
3.
$tableinfo['join type']
should also be checked for "LEFT OUTER", that being the default if we do a$query->leftJoin()
Comment #78
tic2000 CreditAttribution: tic2000 as a volunteer and at Intellix commentedI redid the patch as per my previous comments.
Tests moved to the node module.
It adds tests for public, private and null references.
It uses the already existing node_access_test.module and ads the views to the existing node_test_views module.
The change to node_access_test module is because without it the tests give warnings for trying to get a property of a non-object since the page node type doesn't have the private field.
No interdiff provided since this is rewritten and an interdiff would be bigger than the actual patch.
Comment #79
lokapujyahidden => visible?
Comment #81
lokapujyaTo my understanding, we needed to implement hook_node_access() (see #36) to reproduce the bug. Did you find a different way reproduce it?
Comment #82
tic2000 CreditAttribution: tic2000 as a volunteer and at Intellix commentedThat comment is correct.
The tests for that user are:
He should see the page with a null reference and that with a public reference and not see the page with a private reference.
The access code is in the already existing module node_access_test. As you can see from the test results there were 3 fails for the code without the fix. All 3 users didn't see the entry for the page with null reference.
Edit: Oh, I see now, I have the same comment twice.
Comment #83
tic2000 CreditAttribution: tic2000 as a volunteer and at Intellix commentedOnly comments changed.
Comment #84
lokapujyaGood, much better than creating a new module.
RTBC for me.
Comment #85
tic2000 CreditAttribution: tic2000 as a volunteer and at Intellix commentedAdded reproduction steps and a module to test the issue.
Comment #86
tic2000 CreditAttribution: tic2000 as a volunteer and at Intellix commentedComment #87
tic2000 CreditAttribution: tic2000 as a volunteer and at Intellix commentedComment #88
Nephele CreditAttribution: Nephele commentedGood catch, tlc2000: #1349080: node_access filters out accessible nodes when node is left joined is essentially an even older issue that is a duplicate of this issue -- but with a d7 patch instead of a d8 patch.
The d8 and d7 patches use slightly different approaches, but should both yield the same result. In fact, on my (d7) test site I had implemented a more d8-style fix before finding this issue, then switched to the d7 patch -- I got the same results and had similar query execution times. The tests in the two patches are also functionally the same although written completely differently (in particular the d8 tests already cover one situation that was missing from the d7 tests until yesterday).
I'm not sure what the protocol is supposed to be in this situation, but under the assumption that the powers-that-be would prefer all the patches to be part of a single queue, I'm uploading the d7 patch that I uploaded yesterday to #1349080-282: node_access filters out accessible nodes when node is left joined (only change being the file name).
Comment #90
tic2000 CreditAttribution: tic2000 as a volunteer and at Intellix commentedWell, the first step would be to get this reviewed and commited to D8, I do think one issue should be marked as a duplicate of the other one (this should be the duplicate since the other one is older), but is not my job to decide that.
I looked at this path and I see the fix is used for all joins. Maybe I should do the same for the D8 patch since inner joins would not have a problem with this since they don't select the row with a null reference in the first place.
Comment #91
tic2000 CreditAttribution: tic2000 as a volunteer and at Intellix commentedI mark this as duplicate of #1349080: node_access filters out accessible nodes when node is left joined
Comment #92
sirragh CreditAttribution: sirragh commentedPatch in #88 worked for me in D7.43
Comment #93
cgv CreditAttribution: cgv commentedPatch node_access_for_left_joins_d7-1969208-89.patch works for me using 7.50. Thank you very much.