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

  1. Install module attached in #1969208-85: Query node access alter filtering out accesible nodes
  2. Create 2 regular users (user1 and user2)
  3. Give authenticated users the permission to create articles
  4. 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
    
  5. Attach an entity reference to the page content type named field_related_article. It should allow node of type article
  6. 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
  7. 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
  8. 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
  9. 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
  10. Login with an admin and enable the "View private content" permission for authenticated users
  11. 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
  12. Login with an admin and disable the "View private content" permission for authenticated users
  13. Apply the patch you wish to test
  14. 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

#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"

CommentFileSizeAuthor
#88 node_access_for_left_joins_d7-1969208-89.patch7.34 KBNephele
#85 test_reference_access.tgz2.55 KBtic2000
#83 1969208-83.tests-fix.patch14.89 KBtic2000
#83 1969208-78-83.interdiff.txt1.29 KBtic2000
#78 1969208-78.tests-fix.patch14.87 KBtic2000
#78 1969208-78.tests_.patch13.89 KBtic2000
#70 1969208-70.patch12.82 KBlokapujya
#70 1969208-70-test-only.patch11.9 KBlokapujya
#70 interdiff-1969208-70.txt2.65 KBlokapujya
#66 interdiff-1969208-66.txt4.02 KBlokapujya
#66 1969208-66.patch14.21 KBlokapujya
#66 1969208-66-test-only.patch13.29 KBlokapujya
#62 interdiff-1969208-59-62.txt3.42 KBlokapujya
#62 1969208-62.patch13.82 KBlokapujya
#59 interdiff-1969208-59.txt1.3 KBlokapujya
#59 1969208-59.patch13.68 KBlokapujya
#58 interdiff-1969208-58.txt23.67 KBlokapujya
#58 1969208-58.patch13.47 KBlokapujya
#56 interdiff-1969208-56.txt899 byteslokapujya
#56 1969208-56.patch12.1 KBlokapujya
node-sql_rewrite_access_entity_reference.patch817 bytesEricmaster
#2 node-sql_rewrite_access_entity_reference.patch797 bytesEricmaster
#25 node-sql-rewrite-1969208-25.patch1.05 KBbenjifisher
#28 node-sql-rewrite-1969208-28.patch806 bytesbenjifisher
#34 node-sql-rewrite-1969208-34.patch1.32 KBTechNikh
#35 drupal8_1969208_test_view.txt5.9 KBTechNikh
#36 nodeaccess.zip1.72 KBbenjifisher
#41 node-sql-rewrite-1969208-40.patch943 bytesEricmaster
#47 1969208-47-NodeAccessTest-test-only.patch1.88 KBlokapujya
#49 interdiff-1969208-49-test-only.txt3.65 KBlokapujya
#49 1969208-48-test-only.patch4.78 KBlokapujya
#52 interdiff-1969208-53.txt620 byteslokapujya
#52 1969208-53-test-only.patch12.15 KBlokapujya
#54 1969208-54-test-only.patch12.32 KBlokapujya
#54 interdiff-1969208-54.txt1.58 KBlokapujya
#54 interdiff-1969208-49-52.txt8.78 KBlokapujya
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, node-sql_rewrite_access_entity_reference.patch, failed testing.

Ericmaster’s picture

Sorry, I created the patch from outside of the drupal directory root, here is with the path updated

Ericmaster’s picture

Status: Needs work » Needs review

forgot to update the status

Berdir’s picture

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

Thanks for starting to work on this. This needs a test and has to be fixed in 8.x first and then backported.

Berdir’s picture

Issue tags: +Needs backport to D7

Adding backport tag.

Berdir’s picture

Status: Needs review » Needs work
fenda’s picture

Patch in #2 worked for me in D7.

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

BrockBoland’s picture

Needs an issue summary: http://drupal.org/node/1427826

jasonyarrington’s picture

I am working on a summary for this at Drupalcon Portland

jasonyarrington’s picture

Removing Needs Issue Summary tag

markdorison’s picture

This was an interesting issue for me to identify and track down. The patch in #2 seems to work as expected in D7.

hansfn’s picture

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

hansfn’s picture

Issue summary: View changes

Added an issue summary

hansfn’s picture

Just for the record: The patch in comment #2 works for the newly released Drupal 7.24 too.

shandman’s picture

Still works in 7.28.

spacer’s picture

I'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?

ajmantis’s picture

This patch works in 7.27

Thanks!

benjifisher’s picture

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

benjifisher’s picture

flocondetoile’s picture

Thanks @Ericmaster
Patch #2 solved my issue in D7.28 with content_access, views and entity reference.

weekbeforenext’s picture

Patch #2 solved my issue in Drupal 7.31.

Leeteq’s picture

Version: 8.0.x-dev » 7.x-dev
Priority: Major » Critical
Status: Needs work » Reviewed & tested by the community

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

dcam’s picture

Version: 7.x-dev » 8.0.x-dev
Priority: Critical » Major
Status: Reviewed & tested by the community » Needs work

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

hansfn’s picture

I 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?

benjifisher’s picture

Version: 8.0.x-dev » 7.x-dev
FileSize
1.05 KB

@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():

/**
 * Queries tagged with 'node_access' that are not against the {node} table
 * should add the base table as metadata. For example:
 * @code
 *   $query
 *     ->addTag('node_access')
 *     ->addMetaData('base_table', 'taxonomy_index');
 * @endcode
 * If the query is not against the {node} table, an attempt is made to guess
 * the table, but is not recommended to rely on this as it is deprecated and not
 * allowed in Drupal 8. It is always safer to provide the table.
 */

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.

benjifisher’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 25: node-sql-rewrite-1969208-25.patch, failed testing.

benjifisher’s picture

Status: Needs work » Needs review
FileSize
806 bytes

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

benjifisher’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Needs review » Needs work
weekbeforenext’s picture

Patch #28 worked for me in Drupal 7.32.

Thanks!

bforchhammer’s picture

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

LGLC’s picture

#28 worked for me on Drupal 7.34. Thanks!

benjifisher’s picture

Issue tags: +SprintWeekend2015

@TechNikh and I are looking at this today (Boston instance of the global sprint weekend).

TechNikh’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

rewritten the patch in #28 for Drupal8

Query for admin:

SELECT node_field_data.title AS node_field_data_title, node.nid AS nid, node_node__field_related_pages__node_field_data.title AS node_node__field_related_pages__node_field_data_title, node_node__field_related_pages.nid AS node_node__field_related_pages_nid, node_node__field_related_pages_1.nid AS node_node__field_related_pages_1_nid
FROM 
{node} node
LEFT JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
LEFT JOIN {node__field_related_pages} node__field_related_pages ON node_field_data.nid = node__field_related_pages.entity_id AND (node__field_related_pages.deleted = '0' AND node__field_related_pages.langcode = node_field_data.langcode)
LEFT JOIN {node} node_node__field_related_pages ON node__field_related_pages.field_related_pages_target_id = node_node__field_related_pages.nid
LEFT JOIN {node} node_node__field_related_pages_1 ON node__field_related_pages.field_related_pages_target_id = node_node__field_related_pages_1.nid
LEFT JOIN {node_field_data} node_node__field_related_pages__node_field_data ON node_node__field_related_pages.nid = node_node__field_related_pages__node_field_data.nid
WHERE (( (node_field_data.status = '1') AND (node.type IN  ('article')) ))
LIMIT 10 OFFSET 0

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:

SELECT node_field_data.title AS node_field_data_title, node.nid AS nid, node_node__field_related_pages__node_field_data.title AS node_node__field_related_pages__node_field_data_title, node_node__field_related_pages.nid AS node_node__field_related_pages_nid, node_node__field_related_pages_1.nid AS node_node__field_related_pages_1_nid
FROM 
{node} node
LEFT JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
LEFT JOIN {node__field_related_pages} node__field_related_pages ON node_field_data.nid = node__field_related_pages.entity_id AND (node__field_related_pages.deleted = '0' AND node__field_related_pages.langcode = node_field_data.langcode)
LEFT JOIN {node} node_node__field_related_pages ON node__field_related_pages.field_related_pages_target_id = node_node__field_related_pages.nid
LEFT JOIN {node} node_node__field_related_pages_1 ON node__field_related_pages.field_related_pages_target_id = node_node__field_related_pages_1.nid
LEFT JOIN {node_field_data} node_node__field_related_pages__node_field_data ON node_node__field_related_pages.nid = node_node__field_related_pages__node_field_data.nid
WHERE (( (node_field_data.status = '1') AND (node.type IN  ('article')) ))AND ( EXISTS  (SELECT na.nid AS nid
FROM 
{node_access} na
WHERE (( (gid IN  ('0')) AND (realm = 'all') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) AND ( EXISTS  (SELECT na.nid AS nid
FROM 
{node_access} na
WHERE (( (gid IN  ('0')) AND (realm = 'all') ))AND (na.grant_view >= '1') AND (node_node__field_related_pages.nid = na.nid) )) AND ( EXISTS  (SELECT na.nid AS nid
FROM 
{node_access} na
WHERE (( (gid IN  ('0')) AND (realm = 'all') ))AND (na.grant_view >= '1') AND (node_node__field_related_pages_1.nid = na.nid) )) 
LIMIT 10 OFFSET 0

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:

SELECT node_field_data.title AS node_field_data_title, node.nid AS nid, node_node__field_related_pages__node_field_data.title AS node_node__field_related_pages__node_field_data_title, node_node__field_related_pages.nid AS node_node__field_related_pages_nid, node_node__field_related_pages_1.nid AS node_node__field_related_pages_1_nid
FROM 
{node} node
LEFT JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
LEFT JOIN {node__field_related_pages} node__field_related_pages ON node_field_data.nid = node__field_related_pages.entity_id AND (node__field_related_pages.deleted = '0' AND node__field_related_pages.langcode = node_field_data.langcode)
LEFT JOIN {node} node_node__field_related_pages ON node__field_related_pages.field_related_pages_target_id = node_node__field_related_pages.nid
LEFT JOIN {node} node_node__field_related_pages_1 ON node__field_related_pages.field_related_pages_target_id = node_node__field_related_pages_1.nid
LEFT JOIN {node_field_data} node_node__field_related_pages__node_field_data ON node_node__field_related_pages.nid = node_node__field_related_pages__node_field_data.nid
WHERE (( (node_field_data.status = '1') AND (node.type IN  ('article')) ))AND ( EXISTS  (SELECT na.nid AS nid
FROM 
{node_access} na
WHERE (( (gid IN  ('0')) AND (realm = 'all') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) AND( ( EXISTS  (SELECT na.nid AS nid
FROM 
{node_access} na
WHERE (( (gid IN  ('0')) AND (realm = 'all') ))AND (na.grant_view >= '1') AND (node_node__field_related_pages.nid = na.nid) )) OR (node_node__field_related_pages.nid IS NULL ) )AND( ( EXISTS  (SELECT na.nid AS nid
FROM 
{node_access} na
WHERE (( (gid IN  ('0')) AND (realm = 'all') ))AND (na.grant_view >= '1') AND (node_node__field_related_pages_1.nid = na.nid) )) OR (node_node__field_related_pages_1.nid IS NULL ) )
LIMIT 10 OFFSET 0
TechNikh’s picture

Steps 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

function modulename_views_data_alter(array &$data) {
  unset($data['node_field_data']['table']['join']['node']['type']);
}
/**
 * Implements hook_node_grants().
 */
function modulename_node_grants($account, $op) {
  // Give everyone full grants
  return array(
    'all' => array(0),
  );
}

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.

benjifisher’s picture

FileSize
1.72 KB

I 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 and mynodeaccess) in your modules/ (not core/modules) directory. This makes it easier to test the problem.

Here is a simplified version of the instructions in #35 for reproducing the bug:

  1. Add a field ("Related pages", machine name field_related_pages) to the Article type, an entity reference pointing to a "Basic page" node.
  2. Create at least one Basic page.
  3. Create at least one Article with a reference to a Basic page node, and at least one Article with no reference.
  4. Import the view (admin/config/development/configuration/single/import) attached to #35.
  5. If #2273849: Convert INNER joins to LEFT joins for relationships without "Require this relationship" has not been resolved, then enable the viewsfix module.
  6. Admin and anonymous users should see the same thing at /articles: all the Articles will be listed, with or without the linked Basic page.
  7. Enable the mynodeaccess module.
  8. Rebuild node-access permissions at admin/reports/status/rebuild
  9. Revisit /articles: admin still sees all articles, but anonymous users will only see those with an entity reference.
  10. Apply the patch from #34.
  11. Revisit /articles: admin and anonymous users will again see the same thing.
benjifisher’s picture

Issue summary: View changes
Ericmaster’s picture

Issue tags: +LatinAmerica2015
FerConde’s picture

I 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

constrict0r’s picture

I 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

Ericmaster’s picture

Just re-rolled the patch since it applied with an offset

constrict0r’s picture

I just tested the new patch in #41 and it works too, cool.

argosbass’s picture

Status: Needs review » Reviewed & tested by the community

I applied and tested patch on #41 and it's works. Changing status to "reviewed & tested by the community"

Greetings from DrupalCon Latin America.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Given that this is access related we definitely need to add a test.

benjifisher’s picture

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

Jānis Bebrītis’s picture

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

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.88 KB

Need to start somewhere. Lets just use the "viewsfix" module until that other issue is fixed.

Status: Needs review » Needs work

The last submitted patch, 47: 1969208-47-NodeAccessTest-test-only.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -SprintWeekend2015, -LatinAmerica2015
FileSize
3.65 KB
4.78 KB

A little more work.

Status: Needs review » Needs work

The last submitted patch, 49: 1969208-48-test-only.patch, failed testing.

benjifisher’s picture

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

lokapujya’s picture

Status: Needs work » Needs review
FileSize
620 bytes
12.15 KB

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

Status: Needs review » Needs work

The last submitted patch, 52: 1969208-53-test-only.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
8.78 KB
1.58 KB
12.32 KB

Fix some bugs, create the article content type.

Also, the last interdiff wasn't complete, so I redid the interdiff.

The last submitted patch, 54: 1969208-54-test-only.patch, failed testing.

lokapujya’s picture

FileSize
12.1 KB
899 bytes

Just adjusting the view so that it will load.

lokapujya’s picture

+++ b/core/modules/views/src/Tests/NodeAccessTest.php
@@ -0,0 +1,114 @@
+    // Log in as admin user.
+
+    // Execute the view.
+
+    // Assert the row count of the view.
+
+    // Enable the mynodeaccess module.
+
+    // Rebuild node-access permissions at admin/reports/status/rebuild
+    ¶
+    // Execute the view: admin and anonymous users will again see the same thing.
+  }

Good, the test is almost working. Next we need this part.

lokapujya’s picture

FileSize
13.47 KB
23.67 KB

The test is a lot closer now to actually testing the issue now.

lokapujya’s picture

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

Status: Needs review » Needs work

The last submitted patch, 59: 1969208-59.patch, failed testing.

mgifford’s picture

lokapujya’s picture

Status: Needs work » Needs review
FileSize
13.82 KB
3.42 KB

Rerolling the view.

lokapujya’s picture

I manually tested the issue, and the bug problem 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?

valthebald queued 62: 1969208-62.patch for re-testing.

cattmote’s picture

The patch in #28 solved the issue for me as well without any other side effects.

lokapujya’s picture

Changed test to use drupalGet() instead of view->execute() so that access will be considered.

The last submitted patch, 66: 1969208-66-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 66: 1969208-66.patch, failed testing.

lokapujya’s picture

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

lokapujya’s picture

removed views_test_remove_inner_joins.

The last submitted patch, 70: 1969208-70-test-only.patch, failed testing.

lokapujya’s picture

Issue summary: View changes
tic2000’s picture

Status: Needs review » Needs work

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

lokapujya’s picture

@tic2000 What is a private article?

tic2000’s picture

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

lokapujya’s picture

Access is not controlled per node. So, would this test require having 2 different content types?

tic2000’s picture

Access is not controlled per node

If 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()

tic2000’s picture

Assigned: Unassigned » tic2000
Status: Needs work » Needs review
FileSize
13.89 KB
14.87 KB

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

lokapujya’s picture

+++ b/core/modules/node/src/Tests/NodeAccessJoinTest.php
@@ -0,0 +1,179 @@
+    // User to check if private articles are hidden.

hidden => visible?

The last submitted patch, 78: 1969208-78.tests_.patch, failed testing.

lokapujya’s picture

+++ b/core/modules/views/tests/modules/views_test_mynodeaccess/views_test_mynodeaccess.module
@@ -0,0 +1,30 @@
+function views_test_mynodeaccess_node_grants($account, $op) {
+  return array(
+    'all' => array(0),
+  );
+}

To my understanding, we needed to implement hook_node_access() (see #36) to reproduce the bug. Did you find a different way reproduce it?

tic2000’s picture

That comment is correct.
The tests for that user are:

    $this->assertText('Page with no article', 'Node with null reference visible');
    $this->assertNoText('Page with private article', "Node with private reference hidden");
    $this->assertText('Page with public article', 'Node with public reference visible');

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.

tic2000’s picture

Only comments changed.

lokapujya’s picture

The access code is in the already existing module node_access_test.

Good, much better than creating a new module.

RTBC for me.

tic2000’s picture

Issue summary: View changes
FileSize
2.55 KB

Added reproduction steps and a module to test the issue.

tic2000’s picture

Issue summary: View changes
tic2000’s picture

Issue summary: View changes
Nephele’s picture

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

Status: Needs review » Needs work

The last submitted patch, 88: node_access_for_left_joins_d7-1969208-89.patch, failed testing.

tic2000’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Needs work » Needs review

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

tic2000’s picture

Status: Needs review » Closed (duplicate)
sirragh’s picture

Patch in #88 worked for me in D7.43

cgv’s picture

Patch node_access_for_left_joins_d7-1969208-89.patch works for me using 7.50. Thank you very much.