node_access filters out accessible nodes when node is left joined

hey! i came across this same behavior this weekend, it delayed the launch of a website as my views depended on this great module.

Has anyone found out what is causing this? it is strange to have a not required check box that worked only as admin... unless i am missing something. I've tried filtering out the empty references but still only display as admin.

thanks.

It looks like content access module is the culprit. once i disable the module, everything works fine. I now uninstall the module and looking for a access control module that works well with this great module.

Entity Reference and Views both respect access control now. Check your access control configuration.

I'm having this issue, but only when using content access module. I am using entity relationship 7.x-1.x-dev and views 3.0, content access dev.

I have a topic type and alert type. Alert types reference topic types. The exported view below shows topic content types and uses the "Entity Reference: Referencing entity" so I can get a count of alerts referencing the topic.

When a non-user1 user looks at this view: http://pastebin.com/1BBBeg6j , the user does NOT see all the topics he should. He only see's topics that have alerts referencing them. If the topic doesn't have alerts, it should still show (as it does when using user1) but with a blank in the number of alerts column, but instead that topic just isn't shown.

If I remove the relationship from the view. It will show all the topics to non-user1 users.

Anyone found a solution? I'm not using the Content Access module but am experiencing the same problem using OG-7.x-.2.x

You can try in your view -> advanced setting -> query settings - to check to box “Disable SQL rewriting“.

I have the same problem here when using Domain Access. The 'Disable SQL rewriting' option gave me the desired results.

What about if I want a view in a organic group context and the group content visibility ( public | private ) to be respected? It's not if sql rewrite option is marked (as it's adviced) ... I think it's working forever as "require this relationship".

Actually, AFAICK, this is a bug report. However, if it is for this module, views or every module that implements the access control is another matter.

An optional relationship should have (X IS NULL OR X IN (a,b,c)).

'Disable SQL rewriting' is not a solution.

It is interesting to note that the core Term references (basic forward relationships) does not use a views relationship and the access control is not triggered.

I've just confirmed this bug. Steps to reproduce:

1. Install D7.14 with the standard profile.
2. Install ctools, views, views_ui, entity, entityreference.
3. Add an entityreference field to the article node type that references nodes, page bundle, with other settings as defaults.
4. Create a page node.
5. Create two article nodes, one that references the the page node, and the other with the reference field blank.
6. Create a view of article nodes with a page display.
7. Add a relationship for Entityreference: Referenced entity. Do not check "require this relationship". No need to add any fields using the relationship.
   • Visit the page display as uid 1. Both articles are listed. Query:
     

     /* DEBUG VIEW QUERY */ SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created FROM node node LEFT JOIN field_data_field_page field_data_field_page ON node.nid = field_data_field_page.entity_id AND (field_data_field_page.entity_type = :views_join_condition_0 AND field_data_field_page.deleted = :views_join_condition_1) LEFT JOIN node node_field_data_field_page ON field_data_field_page.field_page_target_id = node_field_data_field_page.nid WHERE (( (node.status = :db_condition_placeholder_2) AND (node.type IN (:db_condition_placeholder_3)) )) ORDER BY node_created DESC LIMIT 10 OFFSET 0
     

   • Visit the page display as an anonymous user. Both articles are listed. Query:
     

     /* DEBUG VIEW QUERY */ SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created FROM node node LEFT JOIN field_data_field_page field_data_field_page ON node.nid = field_data_field_page.entity_id AND (field_data_field_page.entity_type = :views_join_condition_0 AND field_data_field_page.deleted = :views_join_condition_1) LEFT JOIN node node_field_data_field_page ON field_data_field_page.field_page_target_id = node_field_data_field_page.nid WHERE (( (node.status = :db_condition_placeholder_2) AND (node.type IN (:db_condition_placeholder_3)) )) ORDER BY node_created DESC LIMIT 10 OFFSET 0
     

8. Install any node access module, e.g. TAC, and rebuild permissions. No need to configure it; just allow full access to everything.
   • Visit the page display as uid 1. Both articles are listed. Query:
     

     /* DEBUG VIEW QUERY */ SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created FROM node node LEFT JOIN field_data_field_page field_data_field_page ON node.nid = field_data_field_page.entity_id AND (field_data_field_page.entity_type = :views_join_condition_0 AND field_data_field_page.deleted = :views_join_condition_1) LEFT JOIN node node_field_data_field_page ON field_data_field_page.field_page_target_id = node_field_data_field_page.nid WHERE (( (node.status = :db_condition_placeholder_2) AND (node.type IN (:db_condition_placeholder_3)) )) ORDER BY node_created DESC LIMIT 10 OFFSET 0
     

   • Visit the page display as an anonymous user. The article with an empty reference field has disappeared. Query:
     

     /* DEBUG VIEW QUERY */ SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created FROM node node LEFT JOIN field_data_field_page field_data_field_page ON node.nid = field_data_field_page.entity_id AND (field_data_field_page.entity_type = :views_join_condition_0 AND field_data_field_page.deleted = :views_join_condition_1) LEFT JOIN node node_field_data_field_page ON field_data_field_page.field_page_target_id = node_field_data_field_page.nid WHERE (( (node.status = :db_condition_placeholder_2) AND (node.type IN (:db_condition_placeholder_3)) ))AND ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) )OR( (na.gid = :db_condition_placeholder_6) AND (na.realm = :db_condition_placeholder_7) ))AND (na.grant_view >= :db_condition_placeholder_8) AND (node.nid = na.nid) )) AND ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = :db_condition_placeholder_9) AND (na.realm = :db_condition_placeholder_10) )OR( (na.gid = :db_condition_placeholder_11) AND (na.realm = :db_condition_placeholder_12) ))AND (na.grant_view >= :db_condition_placeholder_13) AND (node_field_data_field_page.nid = na.nid) )) ORDER BY node_created DESC LIMIT 10 OFFSET 0
     

9. Edit the view and remove the relationship.
   • Visit the page display as uid 1. Both articles are listed. Query:
     

     /* DEBUG VIEW QUERY */ SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created FROM node node WHERE (( (node.status = :db_condition_placeholder_0) AND (node.type IN (:db_condition_placeholder_1)) )) ORDER BY node_created DESC LIMIT 10 OFFSET 0
     

   • Visit the page display as an anonymous user. Both articles are listed. Query:
     

     /* DEBUG VIEW QUERY */ SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created FROM node node WHERE (( (node.status = :db_condition_placeholder_0) AND (node.type IN (:db_condition_placeholder_1)) ))AND ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = :db_condition_placeholder_2) AND (na.realm = :db_condition_placeholder_3) )OR( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) ))AND (na.grant_view >= :db_condition_placeholder_6) AND (node.nid = na.nid) )) ORDER BY node_created DESC LIMIT 10 OFFSET 0
     

Might be a bug in Views. I'm using the References module, not Entity reference, and I have the same problem. I'm also using Node Access User Reference and Node Access Node Reference for access control.

We've hit this bug, too. Twice in two weeks, actually -- and the first time was in Drupal 6/Views 2! I just confirmed through debugging that it's a result of any node access module getting enabled -- the views query gets altered to look in the node_access table for the base table and the table of any relationship.

I think Alan D has the correct solution -- Disable SQL rewriting is a poor workaround that might open up other problems, the real answer is to make the node access query use an OR X IS NULL query when checking node access on optional relationship tables.

And here's where it's getting mis-applied (Drupal 7.14):

node.module, in function _node_query_node_access_alter($query, $type) (definition starts at line 3183).

Line 3311 seems to be where we need to fix:

      $subquery->where("$nalias.$field = na.nid");
      $query->exists($subquery);

At that point, $tableinfo['join type'] is set to 'LEFT' whereas the base table has that set to null. I'm not familiar with the EXISTS SQL statement -- I think this needs to get rewritten to not use exists and add an or condition where ("$nalias.$field" IS NULL).

Moving to Views based on #11 and #12. No hacking node access; we need to fix this in Views. :)

Oh, and I am not considering this a security issue because we are displaying less data than we should, not more data than we should.

And maybe this is a duplicate of the never-dying #1222324: Fix query access control on relationships (comments) → .

@xjm I believe this is a core bug, not a views bug -- Views just happens to trigger it. Moving issue to Drupal core.

The problem is that _node_query_node_access_alter in node.module does not properly apply node access permissions to related tables, when the join type is set to LEFT -- it should allow null values as well as values in the node_access table.

Views is properly setting the join type.

I've ran into this issue as well and the only access control module I'm using is OG.
Logging MySql queries I receive the following:

SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created
FROM 
drupal_node node
LEFT JOIN drupal_field_data_standard_image field_data_standard_image ON node.nid = field_data_standard_image.entity_id AND (field_data_standard_image.entity_type = 'node' AND field_data_standard_image.deleted = '0')
LEFT JOIN drupal_node node_field_data_standard_image ON field_data_standard_image.standard_image_target_id = node_field_data_standard_image.nid
WHERE (( (node.nid = '2343' ) )AND(( (node.status = '1') AND (node.type IN  ('bir_album')) )))AND ( EXISTS  (SELECT na.nid AS nid
FROM 
drupal_node_access na
WHERE (( (na.gid = '0') AND (na.realm = 'all') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) AND ( EXISTS  (SELECT na.nid AS nid
FROM 
drupal_node_access na
WHERE (( (na.gid = '0') AND (na.realm = 'all') ))AND (na.grant_view >= '1') AND (node_field_data_standard_image.nid = na.nid) )) 
ORDER BY node_created DESC

This is a simple view using node Album with Entity Reference (standard_image) to an Image node. As can be seen, the node access checks the permissions for the standard_image node. The problem is, there are times the standard_image is non-existent for a given Album. The only way to make it work in my case would be to make the standard_image field required. As soon as I add a standard_image, it works fine.

The problem is that _node_query_node_access_alter in node.module does not properly apply node access permissions to related tables, when the join type is set to LEFT -- it should allow null values as well as values in the node_access table.

This sounds quite dangerous to me; a little voice in the back of my head is muttering "access bypass". I'm not convinced that is a correct solution. The query is tagged and views can alter it as needed.

If it's a core bug, let's get a test case demonstrating a non-views path to reproduce this.

@rbruhn your query illustrates the problem, right here:

 EXISTS  (SELECT na.nid AS nid
FROM 
drupal_node_access na
WHERE (( (na.gid = '0') AND (na.realm = 'all') ))AND (na.grant_view >= '1') AND (node_field_data_standard_image.nid = na.nid) )

If there are no image nodes returned in the main query, node_field_data_standard_image.nid will be null, and this subquery won't return any rows. This will filter out the album row.

The query object passed into the _node_query_node_access_alter has a join type set to LEFT for that table, but that's ignored and the access check basically makes it so the relationship is not optional.

@xjm fixing this shouldn't lead to an access bypass -- as you stated earlier, the current behavior is to filter too much out, and addressing this in the node access system should make it work correctly.

The node access query should only be changed for subqueries with a join type of LEFT -- the base table has a null join type, and for that case you would expect a row in the node_access table.

Building a test case for this looks like involves creating a PDO query with a left joined table, and tagging it for node_access, which will invoke the hook_query_TAG_alter for core node_access, node_query_node_access_alter.

Looking into a test case demonstrating this.

@agentrickard pointed me at #1422040: Use leftJoin when base table uses a leftJoin → also.

I'm not sure how core can be expected to adapt to this kind of complex, dynamic query. Looking at the MySQL JOIN documentation, I think the burden here is in the JOIN condition, not the WHERE clause behavior:

The conditional_expr used with ON is any conditional expression of the form that can be used in a WHERE clause. Generally, you should use the ON clause for conditions that specify how to join tables, and the WHERE clause to restrict which rows you want in the result set.

See: https://dev.mysql.com/doc/refman/5.0/en/join.html

By that logic, the proper fix here is to have Views add an ON condition when using a relationship:

/* DEBUG VIEW QUERY */ SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created FROM node node LEFT JOIN field_data_field_page field_data_field_page ON node.nid = field_data_field_page.entity_id AND (field_data_field_page.entity_type = :views_join_condition_0 AND field_data_field_page.deleted = :views_join_condition_1) LEFT JOIN node node_field_data_field_page ON (field_data_field_page.field_page_target_id = node_field_data_field_page.nid OR field_data_field_page.field_page_target_id IS NULL) WHERE (( (node.status = :db_condition_placeholder_2) AND (node.type IN (:db_condition_placeholder_3)) ))AND ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) )OR( (na.gid = :db_condition_placeholder_6) AND (na.realm = :db_condition_placeholder_7) ))AND (na.grant_view >= :db_condition_placeholder_8) AND (node.nid = na.nid) )) AND ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = :db_condition_placeholder_9) AND (na.realm = :db_condition_placeholder_10) )OR( (na.gid = :db_condition_placeholder_11) AND (na.realm = :db_condition_placeholder_12) ))AND (na.grant_view >= :db_condition_placeholder_13) AND (node_field_data_field_page.nid = na.nid) )) ORDER BY node_created DESC LIMIT 10 OFFSET 0

And given that this query is built largely by Views, it would be exceedingly onerous for core to try to unpack the query structure in order to insert the proper ON conditional.

That said, the above query may still return zero rows.

And we have debunked that approach in IRC. The problem is that both NID references are being run against node access, and one of them is returning NULL from the LEFT JOIN.

@agentrickard - You maybe right in that it can't be handled.

A relationship that is not required in Views creates a left join. If the value does not exist, and some node_access check is done, the row will not be returned due to the where clause. If the check on node_access is a left join, it's not really going to give you anything worthwhile. Meaning, it's not restricting anything even if there is a value existing in the relationship.

I imagine if someone creates an entity relationship, and that value exists, of course they would want to know if the user viewing has access to it. I'm not sure if there is a way in a where clause to say, "do not return this row if a user does not have access base on that left join... but oh ya... if the value in the left join is null go ahead and show the row."

Here's the only query variant that I could make work. I am replicating xjm's test, using Domain Access.

* 2 Article nodes
* 1 node is referenced to a Page node

We want 2 rows to be returned.

Initial query from Views: returns zero rows

SELECT node.created AS node_created, node.nid AS nid, node_field_data_field_reference.nid AS field 
  FROM node node 
  LEFT JOIN field_data_field_reference field_data_field_reference ON node.nid = field_data_field_reference.entity_id AND (field_data_field_reference.entity_type = 'node' AND field_data_field_reference.deleted = '0') 
  LEFT JOIN node node_field_data_field_reference ON field_data_field_reference.field_reference_target_id = node_field_data_field_reference.nid 
  WHERE (
    ( 
      (node.status = '1') AND (node.type IN ('article')) ))
    AND 
    ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) 
    AND 
    ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE  (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node_field_data_field_reference.nid = na.nid) ))
    ORDER BY node_created DESC LIMIT 10 OFFSET 0

Working query: returns 2 rows

SELECT node.created AS node_created, node.nid AS nid, node_field_data_field_reference.nid AS field 
  FROM node node 
  LEFT JOIN field_data_field_reference field_data_field_reference ON node.nid = field_data_field_reference.entity_id AND (field_data_field_reference.entity_type = 'node' AND field_data_field_reference.deleted = '0') 
  LEFT JOIN node node_field_data_field_reference ON field_data_field_reference.field_reference_target_id = node_field_data_field_reference.nid 
  WHERE (
    ( 
      (node.status = '1') AND (node.type IN ('article')) ))
    AND 
    ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) 
    AND
    ( EXISTS (SELECT na.nid AS nid FROM node_access na LEFT JOIN field_data_field_reference ON na.nid = field_data_field_reference.entity_id WHERE  field_data_field_reference.entity_id IS NULL OR (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node_field_data_field_reference.nid = na.nid) ))
    ORDER BY node_created DESC LIMIT 10 OFFSET 0

This suggests that any {node_access} subquery has to be LEFT JOINed any related table, so that we can check IS NULL.

And the problem with that approach is that query_alter has no knowledge of how to JOIN the table to the node_access table.

Brief chat with @freelock and it turns out my query was failing because the _referenced node_ was not visible to the current user. So this query "works" now that it is:

SELECT node.created AS node_created, node.nid AS nid, node_field_data_field_reference.nid AS field 
  FROM node node 
  LEFT JOIN field_data_field_reference field_data_field_reference ON node.nid = field_data_field_reference.entity_id AND (field_data_field_reference.entity_type = 'node' AND field_data_field_reference.deleted = '0') 
  LEFT JOIN node node_field_data_field_reference ON field_data_field_reference.field_reference_target_id = node_field_data_field_reference.nid 
  WHERE (
    ( 
      (node.status = '1') AND (node.type IN ('article')) ))
    AND 
    ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) 
    AND (node_field_data_field_reference.nid IS NULL OR 
    ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE  (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node_field_data_field_reference.nid = na.nid) )))
    ORDER BY node_created DESC LIMIT 10 OFFSET 0

However, I see no valid reason why we should be checking the node access status of the referenced entity when building a list of nodes.

Ok, as discussed on IRC, the test case in #32 is not working quite as expected, because the page node being referenced did not have an entry in the node_access table. So the expected result should be 1 row returned -- the row with no referenced page.

This illustrates a corner case that I'm not sure we want to solve, because we don't have enough information in the query object to do a join any other way.

So let me restate a couple different test case scenarios:

Case A:
* 2 article nodes, nid 9 and 12
* 1 page node, nid 13, related to nid 12
* User has access (entries in node_access table) for all 3 nodes
* Expected result: 2 rows, the row for nid 9 having null values for the referenced fields
* Actual result, currently: 1 row, row 12 with the related fields from node 13

Case B: (AgentRickard's original results)
* 2 article nodes, nid 9 and 12
* 1 page node, nid 13, related to nid 12
* User only has access to article nodes, but does not have access to node 13
* Expected result: ? (I would expect 2 rows with null columns for both rows, but don't see how we can get there without entirely rewriting the parent query).
* Actual result: 0 rows (row with optional relationship blocked by null value, row with denied page relationship blocked by node access)
* Proposed result: 1 row (node 9, with null values for the referenced fields -- if we allow the row with node 12 to come through, it would be an access bypass vulnerability exposing data from the blocked node 13)

I think this query will achieve the proposed result, and address the issues in case A:

SELECT node.created AS node_created, node.nid AS nid, node_field_data_field_reference.nid AS field 
  FROM node node 
  LEFT JOIN field_data_field_reference field_data_field_reference ON node.nid = field_data_field_reference.entity_id AND (field_data_field_reference.entity_type = 'node' AND field_data_field_reference.deleted = '0') 
  LEFT JOIN node node_field_data_field_reference ON field_data_field_reference.field_reference_target_id = node_field_data_field_reference.nid 
  WHERE (
    ( 
      (node.status = '1') AND (node.type IN ('article')) ))
    AND 
    ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) 
    AND
    ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node_field_data_field_reference.nid = na.nid) ) OR field_data_field_reference.entity_id IS NULL)
    ORDER BY node_created DESC LIMIT 10 OFFSET 0

That last OR clause should only be added if the join type of the table in the query is LEFT.

@agentrickard in #35, I think it's reasonable in a node_access rewrite to block any access to a node a user does not have permission to view -- here's a use case:

Project management tool

* Project A
** Case AA <- Client can see this
** Case AB <- Private case hidden from client

* Project B
(no cases)

* Project C
** Case CA <- private case

In this case, a view of all open cases across projects, that uses projects as the base type and cases as a relationship, this access check would filter Case AB out of the result set when clients view it.

Before a patch, the client would see only Case AA under Project A, and would not see Project B or C.

After correcting the node access as proposed, the client would see Case AA under Project A and Project B with no cases. They would not see project C until a case was added to it that the client could access, or the private case removed.

Well, here's a version that eliminates the secondary access query on the related node. Possibly not the final approach.

Also attached is a screen cap of the data passed to the alter query for each table, which shows we have two tables identified as $base_table, which I think is wrong. We should only be checking the node access status of the parent node. The fact that you can attached child nodes and see their fields in Views is not, perhaps, a core issue.

Here it is against Drupal 8.

Ok, patch against 7.14, still haven't tested, not sure this is correct syntax for chaining together the conditions.

$table['join type'] needs to be $tableinfo['join type']. If it is, I get this query (and one record):

SELECT node.title AS node_title, node.nid AS nid, node.language AS node_language, node_field_data_field_reference.title AS node_field_data_field_reference_title, node_field_data_field_reference.nid AS node_field_data_field_reference_nid, node_field_data_field_reference.language AS node_field_data_field_reference_language, node.created AS node_created FROM node node LEFT JOIN field_data_field_reference field_data_field_reference ON node.nid = field_data_field_reference.entity_id AND (field_data_field_reference.entity_type = 'node' AND field_data_field_reference.deleted = '0') LEFT JOIN node node_field_data_field_reference ON field_data_field_reference.field_reference_target_id = node_field_data_field_reference.nid WHERE (( (node.status = '1') AND (node.type IN ('article')) ))AND ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) AND( (node_field_data_field_reference.nid IS NULL ) OR ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '1') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') AND (node_field_data_field_reference.nid = na.nid) )) ) ORDER BY node_created DESC LIMIT 10 OFFSET 0

Here is @freelock's patch for D8.

Re-roll of @freelock's patch against HEAD.

Re-Roll of my patch against d8 HEAD.

#41 with the $tableinfo fix from #42 works in my particular circumstance.

Oopsie, 8.x HEAD is still broken, so we need to wait for #1445224: Add new HTML5 FAPI element: color → to be fixed (again).

#46: 1349080-d8-do-not-double-join.patch queued for re-testing.

@rbruhn "works" is a pretty subjective statement here and needs more detail.

The problem with the patch in 41/42/45 is that if you cannot access the attached node, it will deny access to the main node. I think that behavior is wrong.

@agentrickard - I was referring to the problem I saw and posted about earlier in the discussion. In that, when a relationship points to a non-existent node, the null value stops any results being returned. The patch fixed that.

In regards to still showing a result even if the user does not have access to the related node, I agree with you in that is the behavior desired. I've been playing around with queries to see what I can figure out.

@agentrickard, rbruhn, note that the query does not block access to the parent node entirely.

It only blocks access to the parent node iff all child nodes have blocked access. If there is another child node the user can access, the parent node will show up.

@freelock That is an interesting distinction, but we have to consider the implications before changing core.

@agentrickard in my view this is fixing a bug, core node access is not working the way it's intended. And the corner case this does not address is easily understandable from a database point of view...

That's where we disagree. I don't think core node access was ever intended to check the condition of two separate nodes in order to return a single node record. That's what the multiple $base_table invocations does.

The fact that not running the second check allows a JOIN to attach unrestricted data is, perhaps, a Views problem.

[EDIT: additional]

I would also say that if this were any module other than Views, my answer would be "Of course this fails. The queries are wrong. You can't check access to both the parent node and its child data in one query. You have to load the child data on separately."

In fact, if you use a 'content' display mode and not a 'field' display mode, this is what Views does, and it works correctly when using the "only allow a single node access JOIN" rule. The problem here is that Views is rather blindly adding field data to a query that is really asking "show me a list of nodes of condition X." It should not be asking "show me a list of nodes of condition X where an attached child node also meets condition X."

Well I would say that Views here is acting as a query builder, nothing more.

If you're developing with a database-oriented view, rather than an object oriented view, it's inane that a LEFT JOIN is not properly handled. Especially when it's specified in the query object passed in.

Looking at: http://api.drupal.org/api/drupal/includes%21database%21select.inc/functi... ... the database API explicitly supports doing LEFT OUTER queries. (Though I notice that Views is specifying this as LEFT, not LEFT OUTER).

It's perfectly valid sql to join a table multiple times, even as a self-join (though with Drupal's schema in practice there's nearly always an intervening table). In many cases you might want to explicitly look for a null value in the return of such a query.

The alternative here that you're asking module developers to do, is to do extra queries in code, most likely inside a loop.

If we're talking about something as small as 100 rows of parent objects, joined to 1,000 rows of child objects, without having a proper left join syntax, you're turning what could be done in a single query to now doing 101 queries. And if you have a thousand parents and 100 children, now you would have to do 1001 queries to get the same results as could be done in a single query for a left join. Because if this node_access function only handles the parents, you would need to do an additional query on each parent object to find the children your user can access.

Having node_access properly rewrite sql to block access to a node whenever it sees the node table seems like the entire purpose of this function.

If we only do that SOME of the time, then you're putting the burden on less-experienced contrib module developers to know that this function does not fully protect them, even if they tag their query with node_access. And so not only are you making it much easier to have a whole slew of access bypass issues, you are also forcing module developers to do multiple queries inside a loop to properly protect from that, making Drupal a horrible framework for doing basic fundamental database work. When the fix is 4 lines of code that puts the heavy lifting in the database where it belongs, this seems like a no-brainer to me.

It should not be asking "show me a list of nodes of condition X where an attached child node also meets condition X."

Why not?

That seems like a perfectly legitimate thing to do. And that case works perfectly well, right now.

The case that's broken is:

"show me a list of nodes of condition X where an attached child node also meets condition X OR there is no attached child node."

And that's where we disagree, and rather strongly. The node access restriction should be applied to the question:

* Show me a list of nodes I can view. (e.g. All "article" nodes of access group "foo").

Not the question:

* Show me a list of nodes I can view that might also contain attached data from nodes that I can view. (e.g. All "article" nodes of access group "foo" that either have no attached data "bar" or whose "bar" is also in access group "foo".)

The second condition breaks the logic of generating the list. That is the core problem here, not JOIN syntax. The original query breaks because it tries to apply node access conditions twice, which is invalid because the logic is not trustworthy.

In security terms, we should never grant access to a node that does not have a record in the {node_access} table. The fact that the secondary condition adds this possibility means that the query logic is malformed, because it either a) routes around proper node access to attach data or b) improperly restricts valid access based on a secondary (and irrelevant) condition.

Check my last note which indicates this is only a problem with Views that rip Field data out of context. If you use a proper node_load() / entity_load() structure, yes, you get extra queries, but you get proper access controls. Extra queries are a side-effect of having granular access control rules; and a side-effect we've been comfortable with for a long time.

But I fear we're just arguing past each other at this point. We need to get more opinions here.

For reference, look at entityreference_field_formatter_prepare_view() and how it checks access to an entity before attaching it to a standard node view. Views bypasses this step when using field output (but not when using content output.)

The node access restriction should be applied to the question:

* Show me a list of nodes I can view. (e.g. All "article" nodes of access group "foo").

Not the question:

* Show me a list of nodes I can view that might also contain attached data from nodes that I can view. (e.g. All "article" nodes of access group "foo" that either have no attached data "bar" or whose "bar" is also in access group "foo".)

Yes, this. I agree 100%.

Eww. Really? entity_load the entire set and entity_access on each loaded entity inside a loop? No wonder Drupal is getting slower...

In security terms, we should never grant access to a node that does not have a record in the {node_access} table. The fact that the secondary condition adds this possibility means that the query logic is malformed, because it either a) routes around proper node access to attach data or b) improperly restricts valid access based on a secondary (and irrelevant) condition.

Hmm. Your fix causes a. My fix causes b. The status quo is an even more extreme version of b). And eliminating b entirely would entail a lot more query mangling than I think is reasonable to do.

Can you provide an example of how my fix would result in broken logic, aside from the known corner condition?

By my reading, this function is providing a valid sql clause that should properly restrict rows containing node data I cannot view.

Your fix would return rows with data I am not supposed to view -- if we're talking about security, your solution clearly reveals more information, returns more rows than mine does.

The current function additionally blocks rows with no child objects, even when they have no restricted data -- that's what my patch fixes.

Hmm. I wonder if there's enough data in the query object to move that access clause to the join? If that's in the query, it should be possible to apply the base node access clause to the entire query, and the secondary node access clause to the join clause, and solve both a and b.

The real world case I'm hitting does not seem uncommon -- organic groups, where generally people with access to the parent have access to all the children -- and a desire for some aggregate reports on hierarchical data. You don't want leaves of the hierarchy just disappearing on you, and generally the access rules are the same for the entire tree.

This function already does a good job of only allowing access to node data you can view. Why can't we fix the case where it goes too far? Why burden module developers with having to load entities and run them through access checks when this function is entirely capable of doing it for them, far more reliably?

Hmm. I wonder if there's enough data in the query object to move that access clause to the join? If that's in the query, it should be possible to apply the base node access clause to the entire query, and the secondary node access clause to the join clause, and solve both a and b.

At first glance, this appears to work, and address @agentrickard's a and b scenarios in #62. Needs more checks to make sure we're adding to an existing condition, possibly, and also to the entity section... this against 7.14:

diff --git a/modules/node/node.module b/modules/node/node.module
index 57133c6..3c49380 100644
--- a/modules/node/node.module
+++ b/modules/node/node.module
@@ -3309,7 +3309,11 @@ function _node_query_node_access_alter($query, $type) {
         $field = 'entity_id';
       }
       $subquery->where("$nalias.$field = na.nid");
-      $query->exists($subquery);
+      if (empty($tableinfo['join type'])) {
+        $query->exists($subquery);
+      } else {
+        $tables[$nalias]['condition'] .= ' AND ' . (string)$subquery;
+      }
     }
   }

I have not yet examined the generated sql, just dropped it into my current project and it's returning desired results.

I'm sure that needs a bit of polishing, but it should make this function now:

* Show me a list of nodes I can view. If there are any attached nodes I can view, show additional data from those nodes.

Parent node should no longer be dependent on child node.

The condition you are outlining is something Views has to fix. Not core.

there is a string of issues (some reaching back several years/versions) that finally seem to lead to this one being the root cause.

Can you elaborate on why this is not something that should be fixed in core?

Also, just wanted to point out where this is being tracked from a views perspective #1222324: Fix query access control on relationships (comments) →

I support the method in #66. Ken points out there is still a potential false deny but the rarity of that denial is sufficiently low that I believe we can document that possibility and provide a workaround for it. It is better than allowing unfiltered data that is more difficult to remove.

The kind of joins that we are doing in Views is one of the key features that the transition from Views 1 to 2 allowed us and it is not something that could be removed or done differently.

In theory Views could add more access control to entity field rendering, however, but that is a very big job and not one that could be undertaken lightly.

@merlinofchaos and I just walked through the two possible solutions here at DrupalCamp NYC. I'm ok with the approach in #66 now, given that it is the "least bad" option.

We would need to document (for Views users, primarily) the workaround for handling advanced use-cases as described in #62.

do we need a patch from #66 to move this forward?

Also, any pointers on how to resolve this in D7?

FYI - the code in #66 is againt 7.14 and I have tested it and it seems to work great! Thanks @freelock.

I have copies of a D8 and D7 patch lying around that I will post shortly. The big step is tests.

Patch for Drupal 8.

Patch for Drupal 7.

The patch in #77 is ready for tests.

Glad to see some progress here! Had on my list to create some tests around this, but don't currently have a test environment set up, will do later this week if somebody doesn't beat me to it.

@merlinofchaos in #71, curious what condition would trigger a false deny?

There was a false deny in my initial algorithm, in #36 - #45 above. In that query, I was adding an "OR (aliased table.id is null)" clause. This would cause a false deny if all rows in the joined table were denied by the node_access subquery. In #66, I'm adding as an AND clause to the join condition. This would fail with an SQL error if there is no previous join condition... otherwise if the join type is an outer join, the joined table would return null values if there was no value as well as no valid access in the node_access table, and the base table should still return its results.

What am I overlooking?

Oh, just read the patches... @agentrickard, those are the original patches. Need to rewrite with the join condition instead of isNull.

D7 patch.

D8 Patch.

Note: I have not gone through the code block that follows this patch -- if the object type is an entity, it looks like this might add an or condition for particular fields. This section probably also needs a patch, but since I'm not sure offhand when it applies, I'm not sure how to patch that section.

Ok, try again... Looks like my local git clone of drupal wasn't getting fresh updates from HEAD... This update assumes the query object structure and api hasn't changed drastically since 7, haven't yet spun up a copy of 8 to make sure this is functional...

Ah putting the check in the join should remove the false deny so that is a win. It might be worth testing any potential false deny scenarios that we can think of.

Ok, looks like D8 has changed enough that the query isn't necessarily the same structure, at least in the book and search tests.

I've rewritten the patch from comment #77 in order to be able to apply it to the latest version, 7.15.

Please see http://drupal.org/node/767608

Hello I don't know is someone find a solution.

I test Drupal 7.15 and Views 7x 3.5.

Now with the administration account you can get reverse reference with no cases

Father_Node 1
- Child_Reference_Node 1
- Child_Reference_Node 2

Father_Node 2
- No childs nodes have been created.

And if you seleted that the reverse reference is requiered you only get.

Father_Node 1
- Child_Reference_Node 1
- Child_Reference_Node 2

That is correct!! ; )

But other users only get.
Father_Node 1
- Child_Reference_Node 1
- Child_Reference_Node 2

No mather if the relationship is requiered or not. : (

Thanks for your help

I tried with the patch in post 89 and it WORKS for me!!!!
Thanks emorency

@Tim - views is not in core yet, so lets address this in contrib space while we can.

This is filed against Drupal Core, not Views. Either way, the backport policy still applies.

Patch at #89 worked for me!

So if I'm reading things correctly, this cannot be fixed in D7 until it is fixed in D8? (since it is a core patch and there is the backport policy)

So where does D8 patch at #85 stand? Is this still an issue in D8?

It would be great if this can be put into D7 for a future release. Some of us forget to re-apply core patches after we install security upgrades to core :)

Any suggestions for getting to to move forward with D7 or do we have to wait for the D8 fix no matter what?

Yes, it will be fixed in D8 and then in D7. The fastest way to get it fixed in D7 is to help it get fixed in D8. :)

Hi,

I'm part way through creating a test case for this. Finally got a chance to set up a test environment, find what's changed in D8, learn what needs to happen to set up the failing case, etc.

I was trying to leverage the node_access_test module to assign these permissions, but I'm not getting them invoked correctly in my test case -- hoping to get another chance to get the test working correctly in 8, and then update the patch.

Attached is what I've done so far. Right now the test does not appear to invoke the node access hooks in node_access_test -- if somebody could point out what I'm doing wrong, I'd appreciate it -- if not I think I'll whip up another test support module with a simpler node_access invocation...

Ok. I have a working test that fails because of this bug. Still needs to be run through coder, but I think this tests for the correct behavior.

Can somebody please review?

I still have too much on my plate, but I believe the last test above is testing for this issue correctly. However, my last patch fails the test.

What I'm trying to do is add additional conditions to the JOIN clause on the query object. However, by the time it gets to the node_access function, the condition on the join clause is already converted to a string -- and what I'm trying to append is still an object. I'm basically looking for a SQL fragment for this clause to append to the existing join condition string -- if I cast the object to a string, I get a full-blown SELECT query when all I want is the WHERE condition.

I can continue digging on this when I get time, but if somebody who knows SelectQuery objects well can give me a pointer, I'm sure I can get this working much quicker...

At long last, I have a test and a patch.

This patch does work -- it returns rows where there are no matches in the subquery, and it prevents information disclosure/access bypass, blocking access to rows the user does not have access to.

However, I am getting one slightly unexpected result -- when a subquery matches two rows, but the user only has access to one, there are two rows returned, with the blocked one returning null values for the right hand table columns. I've commented out a test for the expected number of rows (lines 192-3 in the test file). I'm not quite following why -- I would expect this case to return a single row, with the subquery values for the node the user has access to.

Can somebody please review?

Thanks!

Ok. At long last, I finally have a working patch!

Definitely need some feedback on this, whether this will break anything. Using query->nextPlaceholder() to increment the placeholder values here seems a bit hackish, but is working pretty well.

The challenge I had to overcome in this is when the base node table is joined to the query, I wanted to add the condition to the join clause. Since join conditions are strings, there does not seem to be a proper way to do this -- I had to generate the proper query conditions, and add the compiled version of that along with the appropriate arguments.

When the main query already had arguments, this made it so there was a name collision on the arguments.

And after incrementing the query->placeholder value appropriately to avoid these collisions, I hit another case where there were placeholder collisions -- when the PagerSelectExtender (or presumably any SelectExtender) is used.

So this might actually fix another set of bugs related to placeholder naming collisions. I think this is a safe way to fix the issue. And short of having a better way in the API to alter table join collisions, this seems like the best way to fix...

Feedback?

Thank you for working on this issue!

+++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.phpundefined
@@ -55,6 +55,7 @@ public function uniqueIdentifier() {
+    $this->query->nextPlaceholder();

Couldn't we just return $this->query->nextPlaceholder(), just wondering.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+   * ¶
...
+    ¶
...
+    ¶

Some trailing whitespace is left.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+    // Test #1349080

We could instead of this set a // @see http://drupal.org/node/1349080

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+    // Join on $join_table
...
+    // Now add subquery join

A trailing dot should be added here.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+    // test as admin_user.

Uppercase T please.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+      "Admin user should get $expected_admin_count rows returned after initial load. Actual: ".count($untagged));
+    $this->verbose('Admin query result: '.
+      theme('table',array('header'=>array('nid','title','subquery_nid','subquery title'),
+        'rows'=>$untagged)
+      )
+    );
+    $this->verbose('Db Query used: '.print_r($result->queryString,1));

If we have something with a dynamic number we can use format_string() for the message.

In general this needs some small changes for codestyle.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+    $this->assertEqual(count($tagged), $expected_user_count,
+      "Regular user should get $expected_user_count rows returned after initial load. Actual: ".count($tagged));
+    $this->verbose('Regular user query result: ' .
+      theme('table',array('header'=>array('nid','title','subquery_nid','subquery title'),
+        'rows'=>$tagged)
+      )
+    );
+    $this->verbose('Db Result: '.print_r($result->queryString,1));

I guess we can remove the verbose as it's just used for testing?

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+   * @return null
+   *
+   * Assertions are made from this function, so no return value necessary.

We probably don't have to document that.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+        $this->fail('User was able to access a node in the returned result, in the parent query.');
...
+        $this->fail('User was able to access a node in the returned result, in the subquery.');

I guess it would be also helpful to include somehow the actual values so it can be tracked down more easy once this breaks.

+++ b/core/modules/node/node.moduleundefined
@@ -3092,8 +3092,22 @@ function node_query_node_access_alter(AlterableInterface $query) {
+      } else {

The codestyle says: Add a new line for an else{}

+++ b/core/modules/node/node.moduleundefined
@@ -3092,8 +3092,22 @@ function node_query_node_access_alter(AlterableInterface $query) {
+        // Increment the placeholder count on the main query until it matches the placeholders
+        // used by the subquery.
+        $cond_count = $subquery->nextPlaceholder();
+        while ($cond_count--) {
+          $query->nextPlaceholder();

This seems to be really hacky, can't we request a new placeholder and use that?

@dawehner, thanks for the review! I'll clean up the style issue and repost the patch in the next couple days. I do think the two code comments bear more widespread review:

+++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.phpundefined
@@ -55,6 +55,7 @@ public function uniqueIdentifier() {
+    $this->query->nextPlaceholder();

Couldn't we just return $this->query->nextPlaceholder(), just wondering.

I don't know this API thoroughly, but I would think that's an improvement. I see no reason to keep two placeholder values in the same query object -- just confuses things and leads to bugs.

+++ b/core/modules/node/node.moduleundefined
@@ -3092,8 +3092,22 @@ function node_query_node_access_alter(AlterableInterface $query) {
+        // Increment the placeholder count on the main query until it matches the placeholders
+        // used by the subquery.
+        $cond_count = $subquery->nextPlaceholder();
+        while ($cond_count--) {
+          $query->nextPlaceholder();

This seems to be really hacky, can't we request a new placeholder and use that?

I agree, this seems hacky, but it's the only method I could find to access the protected member placeholder property. I was thinking a different placeholder name (before the incrementing value) might be appropriate, but that's set in the ->compile method.

So unless I'm missing something, this is the only way to avoid argument naming collisions when assembling a query object with multiple subqueries compiled at different times. A shortcoming in the API?

I think the best, less hacky solution would be to support using a Condition object as a $table['condition']. Then this alter could move an existing table join condition into a query object, and the whole thing would get compiled at the same time, avoiding name collisions. It looks like the $condition argument of all the join methods expect a string, and only do %alias expansions if it is a string. And while I'm not seeing an explicit prohibition on using condition objects here (I thought I had seen it specified as a string earlier, but can't find that documented), it's certainly not working if I change the patch to replace the string with a condition object.

The other less hacky approach might be to check for any existing placeholder arguments recursively through the query at compile or string casting time. I do see it explicitly documented that a placeholder can only be used once within a given query. If this is checked when converting conditions to argument placeholders, that would be a better solution, and likely catch other corner cases that currently fail.

I'm thinking specifically in Condtion::compile(), where the :db_condition_placeholder label is applied -- perhaps move that text into the Query::nextPlaceholder() function, change that function's return value to a string, and have it check against the current Query arguments() for a collision.

Obviously both of those involve much bigger API changes... and it seems a bit late in the release cycle for that...

Any other thoughts?

It seams to me that 7.x patch http://drupal.org/node/1349080#comment-6384002 #89 has a security problem because when I show entities in a view that reference other entities and I check "require this relationship", i see the entities even if i don't have access to the referenced entities.

Views is not in Core in Drupal 7.

Thank you manuel for the feedback, I guess the following line is problematic.

Instead of checking for an empty join type we should probably check for an INNER join type, as then we want to handle it the strict way.

if (empty($tableinfo['join type'])) {

In order to get this into Drupal the process is to first get it into Drupal8 and then backport which is pretty straighforward once there is a good test coverage.

@dawehner I don't think this matters. If there is any join, we can add the conditions to the join condition (inner join or outer join). If there is no join, or if it's an inner join, we can add to the main where condition. When it's an inner join, the net effect should be the same whether the condition is on the join or on the where condition. It's an outer join we need to specifically add to the join condition.

@manuelBS, the patch has not yet been updated for D7, and the earlier test failures did find a logic flaw. So I'm not surprised you were able to find a test case that breaks in D7. It would be very useful to create an automated test to replicate the scenario you outlined.

@tim.plunkett this is not a patch for views, it's a patch for node access (and also the DB abstraction layer, SelectExtender interface). Views just depends upon this behavior working correctly.

@tim.plunkett this is not a patch for views, it's a patch for node access (and also the DB abstraction layer, SelectExtender interface). Views just depends upon this behavior working correctly.

Core patches also need to go into D8 first. :)

@freelock

Would it be possible to fix at least the code-style issues, maybe this will trigger people with knowledge to node access to review it.

Ok! Here's an updated patch, fixing the code style issues identified. No coder for d8 yet!

I can confirm #89 works for my D7 test case (node reference from a term view) - thanks :)

@Alice, the patch for D7 is incomplete, and can trigger SQL errors in certain conditions -- I haven't yet backported the corrected patch from D8.

Can someone provide a good review of the patch from #119?

Now I tried to backport the patch from #119 to D7. But there still seams to be a problem whit entities in a view in the fallowing use case. Before I create a test case, I want to make sure that the problem is related to this issue:

I have a "self made" entity that references a node just be putting the nodes nid into the entity tabel (no reference field). Then I added this relationship in hook_views_data.
In my view I show the "self made" entities and I added the relation to the referenced node, the relation is required.
I make sure that I dont have access to all referenced nodes. If I apply the patch, I see all the "self made" entities in the view. If I don't apply the patch, these entities that reference a node where I don't have access to, are not shown in the view (but of course then the initial issue occurs again). But as I understand, the query should check access to related nodes if "the relationship" is required.

Instead of checking for an empty join type we should probably check for an INNER join type, as then we want to handle it the strict way.

as @dawehner wrote works form me. And in my opinion this makes sence to check

if (empty($tableinfo['join type']) || $tableinfo['join type'] == 'INNER') {

cause in case of an inner join (when for axample a relationship is required) we need to have access check with

$query->exists($subquery);

To ensure that the user has access to the referenced nodes.
I added tha patch to this comment

#126: d7_move_access_to_join_condition-1349080.patch queued for re-testing.

@manuelBS the patch is failing to apply because you need to create the entire patch, not a patch of my patch ;-)

But I am not understanding the case you're hitting -- when I spent time analyzing this problem, adding the condition to the join clause if there was one seemed like a total wash if it was an inner join.

Can you provide the SQL that is failing with my patch? Maybe we can come up with a clear test for that case?

Thanks!

Sorry bad fault ;-) Here is the patch again. I hope it works now @freelock thanks for your help.
Does http://drupal.org/node/1349080#comment-7141442 not explain my thoughts or do you think these thought are not always right and open other issues?

@manuelBS, I think your addition is unnecessary -- your statement in #126 that you need to have an access check when there's an inner join implies that there isn't one. But there is -- it is just moved to the join condition rather than being in a subselect in the where clause. When there is an inner join clause in the SQL, adding the condition to the join clause should return exactly the same set of rows as adding the condition as an exists subquery in the main where clause. But I could be missing something.

That's why I'm asking for some SQL that my patch is generating -- to prove me wrong.

If you can provide a very specific test case that breaks with my patch, then let's see if your version works (in my debugging on this, the "join type" values have been very inconsistent -- I've seen INNER, INNER JOIN, LEFT, LEFT JOIN, LEFT OUTER JOIN iirc, so at a minimum we would need to check more carefully the contents to cover all cases). If you can post some SQL and a pretty clear step-by-step of how you generated that SQL, I can write the test case which we need anyway for each patch.

If your backport is failing in D7, I do notice your patch is missing a couple key things -- the reference to the tables returned from the query object, and the fix to the pager query to prevent name collisions of the database placeholders...

Didn't realize this had gone back to Needs Work -- the patch for D8 is complete as far as I know, and needs review before backporting to D7.

Can we get this reviewed and committed? :-)

Reposting patch from #119.

Update test for changes to NodeTestBase::drupalCreateNode, which now adds the language code and breaks if one is used.

Why this patch isn't in Drupal 7.22 ? I have on 7.22 still the same problem. Or is there a other way to fix that?

I can confirm #89 works for my D7 test case.

Hi,

I can probably find some time this week to backport the patch to D7. But how do I post this for the testbot? And is there anyone who can review the D8 patch further?

I don't think the patch in #89 is correct -- it can trigger some name collisions that can result in failed SQL queries.

Cheers,
John

Agree with John, #89 is not a fix. It would be great to backport to D7. Have not had a chance to look into the D8 patch but if it is acceptable it would be great to move forward with these issues.

Hi.

I would like to know if the patch for D7 will be released.

Regards,
Gilsberty

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.php
@@ -0,0 +1,204 @@
+    $this->field = field_create_field(
+      array(
+        'field_name' => $this->field_name,
+        'type' => 'number_integer',
+        'cardinality' => FIELD_CARDINALITY_UNLIMITED
+      )
+    );

Testbot failed on this with a Call to undefined function Drupal\node\Tests\field_create_field()

New patch will be up soon

HEAD had a lot of new things, I have updated a good deal of the test, but I didn't have time to make it work completely.
The sql in base_query needs to be updated.

I managed to finish a working backport to D7, including the test.

Hi.

I tested patch #149 without success.

That doesn't mean the patch is not correct but perhaps it is not the correct solution for the issues: https://drupal.org/node/2012250 and https://drupal.org/node/2113919.

Do you think that two issues are directly related with this one?

Regards,
Gilsberty

Same issue here. I tested patch #149 partly successfully: what I now can see is the proper list of objects in a view even if there is no relationship on those rows, but what is missing is the pager: if I have multiple results, I do not see them unless I manually change the URL to show the next page.

Do you have any clue on what we should look into? Maybe the views module?

Thank you,
Marco

Hi, Gilsbert,

After a quick review of #2012250: entity reference in profile field permissions → and [2113919] I suspect this issue is the underlying cause of both of them. And so if you're not getting the expected result, the patch is incomplete.

What triggers this issue is having some sort of node_access module enabled -- when a query gets tagged with node_access, the query gets rewritten incorrectly. What we need is a test case to replicate the failure.

I have not yet reviewed RunePhilisof's patches, will take a look over our holiday weekend. But if they are failing for you, you've probably hit some case where this patch is incomplete. Can you provide more details? e.g. which node access module, what configurations are still failing with the patch?

@olivo.marco I would think if you're getting correct results back but having pager trouble, this sounds like a pager issue -- are you using grouping?

One site I work on showed something similar, that site uses https://drupal.org/project/view_unpublished. Have not verified that this might be the cause of this but it fiddles with node access.

What I am using as a content access module is tac_lite. But no grouping is involved: the pager has just disappeared when there is a relationship (non-required), but "manual paging" via URL-modification works.

@olivo Hmm. That suggests that the count query is not getting altered correctly. That does suggest a new test case we'll need to build for this.

I'm sorry for taking two days for my answer.
Yes I will make a detailed test.
Working on it right now.

Version of drupal core and extra modules

Drupal Core - 7.24
Chaos Tools - 7.x-1.3
Entity API - 7.x-1.2
Entity Reference - 7.x-1.1
Views & Views UI - 7.x-3.7

With this setup the issue doesn't happen as freelock commented at #152 (no node_access module enabled).

The following modules were turned on individually and the issue happened on each of them.

views unpublished - 7.x-1.1
content access - 7.x-1.2-BETA2+0-dev

I didn't test others because I know/use only those two.

The issue is happening even after applying the patch #149.

Steps to reproduce the issue

1 - create a new content type "parent a" (fields title and body are enough)

2 - create a new content type "parent b" (fields title and body are enough)

3 - create a new content type "children" with fields
a) title and body
b) field_parent_a: entity reference to "parent a", cardinality unlimited
c) field_parent_b: entity reference to "parent b", cardinality unlimited

Create the contents as following:
- parent a: "a1" and "a2"
- parent b: "b1" and "b2"
- children: "c" and point for field_parent_a the nodes "a2" and "a1" and for field_parent_b the node "b1"

Now create a view with the objective to list all nodes of type "parent a" or "parent b" following the order gave by the node of type "children", listing first parent a then parent b and finally the rest of the nodes by the update date.
That way you would get a list like this:
a2
a1
b2
b1

This view works perfectly for superuser but not for anonymous user!
(P.S.: please remember to turn on at least one of the node_access modules)

Below there is the exported view and following it the SQL generated by the view.

View
====

$view = new view();
$view->name = 'v1';
$view->description = '';
$view->tag = 'default';
$view->base_table = 'node';
$view->human_name = 'v1';
$view->core = 7;
$view->api_version = '3.0';
$view->disabled = FALSE; /* Edit this to true to make a default view disabled initially */

/* Display: Master */
$handler = $view->new_display('default', 'Master', 'default');
$handler->display->display_options['title'] = 'v1';
$handler->display->display_options['use_more_always'] = FALSE;
$handler->display->display_options['access']['type'] = 'perm';
$handler->display->display_options['cache']['type'] = 'none';
$handler->display->display_options['query']['type'] = 'views_query';
$handler->display->display_options['exposed_form']['type'] = 'basic';
$handler->display->display_options['exposed_form']['options']['reset_button_label'] = 'Reiniciar';
$handler->display->display_options['pager']['type'] = 'full';
$handler->display->display_options['pager']['options']['items_per_page'] = '10';
$handler->display->display_options['pager']['options']['tags']['first'] = '« início';
$handler->display->display_options['pager']['options']['tags']['previous'] = '‹ anterior';
$handler->display->display_options['pager']['options']['tags']['next'] = 'próximo ›';
$handler->display->display_options['pager']['options']['tags']['last'] = 'fim »';
$handler->display->display_options['style_plugin'] = 'default';
$handler->display->display_options['row_plugin'] = 'fields';
/* Relationship: Entity Reference: Referencing entity */
$handler->display->display_options['relationships']['reverse_field_parent_a_node']['id'] = 'reverse_field_parent_a_node';
$handler->display->display_options['relationships']['reverse_field_parent_a_node']['table'] = 'node';
$handler->display->display_options['relationships']['reverse_field_parent_a_node']['field'] = 'reverse_field_parent_a_node';
/* Relationship: Entity Reference: Referencing entity */
$handler->display->display_options['relationships']['reverse_field_parent_b_node']['id'] = 'reverse_field_parent_b_node';
$handler->display->display_options['relationships']['reverse_field_parent_b_node']['table'] = 'node';
$handler->display->display_options['relationships']['reverse_field_parent_b_node']['field'] = 'reverse_field_parent_b_node';
/* Campo: Conteúdo: Título */
$handler->display->display_options['fields']['title']['id'] = 'title';
$handler->display->display_options['fields']['title']['table'] = 'node';
$handler->display->display_options['fields']['title']['field'] = 'title';
$handler->display->display_options['fields']['title']['label'] = 'title';
/* Campo: Conteúdo: Body */
$handler->display->display_options['fields']['body']['id'] = 'body';
$handler->display->display_options['fields']['body']['table'] = 'field_data_body';
$handler->display->display_options['fields']['body']['field'] = 'body';
$handler->display->display_options['fields']['body']['label'] = 'body';
/* Sort criterion: Conteúdo: parent_a (field_parent_a:delta) */
$handler->display->display_options['sorts']['delta']['id'] = 'delta';
$handler->display->display_options['sorts']['delta']['table'] = 'field_data_field_parent_a';
$handler->display->display_options['sorts']['delta']['field'] = 'delta';
/* Sort criterion: Conteúdo: parent_b (field_parent_b:delta) */
$handler->display->display_options['sorts']['delta_1']['id'] = 'delta_1';
$handler->display->display_options['sorts']['delta_1']['table'] = 'field_data_field_parent_b';
$handler->display->display_options['sorts']['delta_1']['field'] = 'delta';
/* Sort criterion: Conteúdo: Updated date */
$handler->display->display_options['sorts']['changed']['id'] = 'changed';
$handler->display->display_options['sorts']['changed']['table'] = 'node';
$handler->display->display_options['sorts']['changed']['field'] = 'changed';
$handler->display->display_options['sorts']['changed']['order'] = 'DESC';
/* Filter criterion: Conteúdo: Publicado */
$handler->display->display_options['filters']['status']['id'] = 'status';
$handler->display->display_options['filters']['status']['table'] = 'node';
$handler->display->display_options['filters']['status']['field'] = 'status';
$handler->display->display_options['filters']['status']['value'] = 1;
$handler->display->display_options['filters']['status']['group'] = 1;
$handler->display->display_options['filters']['status']['expose']['operator'] = FALSE;
/* Filter criterion: Conteúdo: Tipo */
$handler->display->display_options['filters']['type']['id'] = 'type';
$handler->display->display_options['filters']['type']['table'] = 'node';
$handler->display->display_options['filters']['type']['field'] = 'type';
$handler->display->display_options['filters']['type']['value'] = array(
'parent_a' => 'parent_a',
'parent_b' => 'parent_b',
);

/* Display: Page */
$handler = $view->new_display('page', 'Page', 'page');
$handler->display->display_options['path'] = 'v1';
$translatables['v1'] = array(
t('Master'),
t('v1'),
t('more'),
t('Apply'),
t('Reiniciar'),
t('Sort by'),
t('Asc'),
t('Desc'),
t('Items per page'),
t('- All -'),
t('Offset'),
t('« início'),
t('‹ anterior'),
t('próximo ›'),
t('fim »'),
t('Conteúdo referencing Conteúdo from field_parent_a'),
t('Conteúdo referencing Conteúdo from field_parent_b'),
t('title'),
t('body'),
t('Page'),
);

SQL
===

SELECT node.title AS node_title, node.nid AS nid, field_data_field_parent_a.delta AS field_data_field_parent_a_delta, field_data_field_parent_b.delta AS field_data_field_parent_b_delta, node.changed AS node_changed, 'node' AS field_data_body_node_entity_type
FROM
{node} node
LEFT JOIN {field_data_field_parent_a} field_data_field_parent_a ON node.nid = field_data_field_parent_a.field_parent_a_target_id AND (field_data_field_parent_a.entity_type = 'node' AND field_data_field_parent_a.deleted = '0')
LEFT JOIN {node} field_parent_a_node ON field_data_field_parent_a.entity_id = field_parent_a_node.nid
LEFT JOIN {field_data_field_parent_b} field_data_field_parent_b ON node.nid = field_data_field_parent_b.field_parent_b_target_id AND (field_data_field_parent_b.entity_type = 'node' AND field_data_field_parent_b.deleted = '0')
LEFT JOIN {node} field_parent_b_node ON field_data_field_parent_b.entity_id = field_parent_b_node.nid
WHERE (( (node.status = '1') AND (node.type IN ('parent_a', 'parent_b')) ))
ORDER BY field_data_field_parent_a_delta ASC, field_data_field_parent_b_delta ASC, node_changed DESC
LIMIT 10 OFFSET 0

If you need more information please let me know!

Regards,
Gilsberty

Hi,

@gilsbert, I spun up a pretty vanilla test install of D7, following your instructions and importing your view. I confirmed that with no access modules enabled, it worked for anonymous users, but with either content_access or view_unpublished it failed for anonymous users.

However, after applying the patch in #149, I got the expected behavior with the same nodes available for anonymous users as user 1. So this patch appears to be working, here...

I've left both view_unpublished and content_access set up. Are you sure you applied the patch correctly? If so, there's something missing from your recipe. I'll leave this install up for a week or so if you want to see if you can break the node access again ;-) http://d7.dotest.freelock.net , admin/admin and test/test accounts set up.

@olivo would be great if you could set up some test data illustrating your pager issue on this instance...

Hi @freelock.

Thank you very much for allowing me to help you with this issue.

Unfortunately I confirm that the patch #149 doesn't worked for me.
But I got more details that might help us find why this is happening.
In a summary I believe the patch is OK but we might have found another bug (related or not with this one) restricted to postgresql database.

So before I continue let me explain: I use postgresql on my Drupal installation and that would not be the first time I face an issue restricted to it.

Is the site posted by you using mysql or postgresql?

Now let me explain why I'm having this conclusion.

1 - I turned on "devel" information for anonymous user on both sites (mine and yours).
--> You can access my site by http://www.ifch.unicamp.br/beto/v1
--> Yours is at http://d7.dotest.freelock.net//v1

2 - In the list of queries located in the footer of each page I found in your site the following 3 queries:

a) - node_access_view_all_nodes
SELECT COUNT(*) AS expression FROM node_access node_access WHERE (nid = '0') AND (grant_view >= '1') AND(( (gid = '0') AND (realm = 'all') )OR( (gid = '0') AND (realm = 'content_access_author') )OR( (gid = '1') AND (realm = 'content_access_rid') ))

b) - views_plugin_pager::execute_count_query
* I didn't paste the SQL here because I believe it is not relevant.

c) - views_plugin_query_default::execute
SELECT node.title AS node_title, node.nid AS nid, field_data_field_parent_a.delta AS field_data_field_parent_a_delta, field_data_field_parent_b.delta AS field_data_field_parent_b_delta, node.changed AS node_changed, 'node' AS field_data_body_node_entity_type FROM node node LEFT JOIN field_data_field_parent_a field_data_field_parent_a ON node.nid = field_data_field_parent_a.field_parent_a_target_id AND (field_data_field_parent_a.entity_type = 'node' AND field_data_field_parent_a.deleted = '0') LEFT JOIN node field_parent_a_node ON field_data_field_parent_a.entity_id = field_parent_a_node.nid AND EXISTS(SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'content_access_author') )OR( (na.gid = '1') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (field_parent_a_node.nid = na.nid) ) LEFT JOIN field_data_field_parent_b field_data_field_parent_b ON node.nid = field_data_field_parent_b.field_parent_b_target_id AND (field_data_field_parent_b.entity_type = 'node' AND field_data_field_parent_b.deleted = '0') LEFT JOIN node field_parent_b_node ON field_data_field_parent_b.entity_id = field_parent_b_node.nid AND EXISTS(SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'content_access_author') )OR( (na.gid = '1') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (field_parent_b_node.nid = na.nid) ) WHERE (( (node.status = '1') AND (node.type IN ('parent_a', 'parent_b')) ))AND ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'content_access_author') )OR( (na.gid = '1') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) ORDER BY field_data_field_parent_a_delta ASC, field_data_field_parent_b_delta ASC, node_changed DESC LIMIT 10 OFFSET 0

3 - In the list of queries on my site I found only:

a) - node_access_view_all_nodes
* Same querie as yours.

The queries (b) and (c) are not being executed on my site.
There are not any warning/error messages so I can't explain why they are not being executed.
But I can confirm those informations:

- the query (a) is returning zero as the result for "count(*)" on my site (is that correct? is the same result for you?)

- the query (c) is correctly returning the rows that we need to show in the view when I paste it and execute it directly on my database (thats why I believe the patch #149 is OK)

So there is something happening after the query (a) that is not allowing the queries (b) and (c) to be executed on my site. What it could be? Where/how can I look deeper?

Regards,
Gilsberty

P.S.: me and @drunken_monkey found an issue with complex queries and postgresql database related here: https://drupal.org/node/2142107 . Do you think the problem with "v1" view on my site might be related with the issue #2142107?

Hi, @gilsbert,

Thanks for the follow-up! Yes, it does seem quite likely that you're hitting the same issue as #2142107: Complex cloned query dependent on __toString() call → -- the change with the patch does involve casting a subquery object to a string, so if the Postgres driver has trouble in certain cases doing that, it could very well be the source of this bug.

I do get a count of 0 when running query (a) as shown, so that's not the issue.

I would say the next step is going through the code in a debugger, and finding out exactly where it's failing. Do you have a debugger/IDE set up?

I've got a pretty full plate this week, but I'll see if I can carve out a couple hours to step through this on postgres in a few days...

P.S. We need to see if this is an issue in D8, too. And yes, this server is using MySQL -- well, MariaDB, actually.

Hi @freelock.

I dont have a debugger/IDE on my site :-(
I would also need first to debugg it in a site where the workflow is working to compare with mine.

I can edit the module files and manually debugg using "echo". I would just need a hint where to start.
I guess I should start in views module or in a workflow control module that calls views and check why it is not being called.

Regards,
Gilsberty

In my specific case
A calendar show nodes where some have a reference and some don't, #149 worked great

I tested the patch at #149 and it also worked great for me. Thanks!

Hi @freelock.

May we try to solve the postgresql issue?

Regards,
Gilsberty

Hi, @gilsbert,

Been slammed around here. I did swap out the dotest.freelock.net instance with a postgres database, but haven't had a chance to replay your steps to reproduce. I did run the automated node access tests, and they passed on Postgres, so we definitely do not have test coverage that identifies your issue.

Can you go ahead and get the scenario you described up and running on that instance? User admin/pw admin should get you in as user 1, and the add-on modules for content access, etc should be available to enable. I do have xdebug installed on this server, but have not (yet) configured it -- once we have a test case I can probably get that configured and then step through it with a debugger and find the problem...

Thanks,
John

Hi John.

The view "v1" is avaliable in your site.
http://d7.dotest.freelock.net/v1

It works for superuser and doesn't work for anonymous.

I gave to anonymous and authenticated users permission to see published content and to see devel information.
I tested the permission to see contents visiting the page http://d7.dotest.freelock.net/content/c .

Going back to the view the behavior is equal what I reported at #160.
The queries "views_plugin_pager::execute_count_query" and "views_plugin_query_default::execute" are not being executed.

So I believe we reproduced the full scenario with sucess.

Let me know if I can help further.

Regards,
Gilsberty

Ok, I've confirmed that this is an issue, and ran out of steam before getting to the bottom of it.

One thing I've found is that subselect handling of placeholders is still broken -- what's different about this scenario is that there are two subqueries getting added to the query -- the node table appears in this query 3 times, and the second two use joins with subqueries. Both of these end up with the same db_placeholder keys. I think this works in MySQL because they are exactly the same in both subqueries, and so the replacement happens to work anyway. Something in Postgres must be interfering with this token replacement.

It could be related to #886970: DB API putting wrong db placeholders in complex queries → , specifically @Crell's comment #9, item 2.

If this is the case, we might be able to get around it by calling $subquery->get_arguments() before casting the subquery to a string.

Also, getting back into this again, I think this patch is incomplete, as it does not cover when node_access is called with a type of "entity". I don't know of any examples where that is used, or a good way to build a test case for it -- I would suggest we deal with that in a separate issue.

I'll come back to this later, but I need to run for now...

I also tested #149,

My view has a pager. After applying the patch the result for anonymous users changed from nothing to show the first page, but the pager disappeared. In the meaning time, Root user can see the pager with no problem though.

As patch #149 works for my use cases perfectly in D7 on mysql but cannot be applied to the latest 7.25 release, here is the patch rerolled if anybody else needs it.

#174 worked for me! I'm using D7 latest version and after clearing cache, the views that were not displaying to the anonymous user are now showing up. Thanks a lot!

I've also tested patch in #174 and confirm that is works like a charm! Thank you.

Change branch to 7.x, since patch in #174 is for 7.x and needs to be tested by a testbot.

I think for this to be tested by testbot for d7, this needs to be an issue in the views contrib project.

@rcross but why in the view project? This is really related to core.

Hello friends.

After a deserved vacation I'm back.

Patch #174 doesn't work on postgresql environment.
After testing it I got the same results reported at #160.

But it might be related with the postgresql driver...

I hope to get an answer at https://drupal.org/node/2142107 and then I will retest this patch.

Regards,
Gilsberty

@manuelBS I thought this was related to the views project. Views is only in core for D8.

The bot didn't run so re-attaching the same patch.

It applies nicely to my local install.

Testbot, where are you?
The patch applies and solves the problem.

Reattaching the same file with the name which asks the bot to test the patch.

185: 1349080-174-d7-move-access-to-join-condition-update-please-test.patch queued for re-testing.

I can't see why that patch fails with this test:

  /**
   * Tests that search returns results with punctuation in the search phrase.
   */
  function testPhraseSearchPunctuation() {
    $node = $this->drupalCreateNode(array('body' => array(LANGUAGE_NONE => array(array('value' => "The bunny's ears were fuzzy.")))));

    // Update the search index.
    module_invoke_all('update_index');
    search_update_totals();

    // Refresh variables after the treatment.
    $this->refreshVariables();

    // Submit a phrase wrapped in double quotes to include the punctuation.
    $edit = array('keys' => '"bunny\'s"');
    $this->drupalPost('search/node', $edit, t('Search'));
    $this->assertText($node->title);
  }

trying to replicate it here, it seems to work - http://s4daf11e38186177.s3.simplytest.me/search/node/bunny%27s

The patch deals with placeholders incorrectly. The resulting query has multiple placeholders having the same name.

Fix placeholder numbering

Attaching correct interdiff

Another one try.

I'm not sure this is right way.

Very nice! #195 did a good Job for me and works!

1) there's a same issue in 8.x - so should be fixed first
2) #115 is not addressed! but test is written for outer join
3) the approach is fragile because assumes that the order of placeholders and joins would be the same

1. +++ b/modules/node/node.module
   @@ -3402,6 +3402,13 @@ function _node_query_node_access_alter($query, $type) {
   +      $np = count($query->getArguments());
   +      while ($np--) {
   

   please use more readable $placeholer_count

2. +++ b/modules/node/node.module
   @@ -3434,7 +3441,22 @@ function _node_query_node_access_alter($query, $type) {
   +          $tables[$nalias]['condition'] .= ' AND EXISTS(' . (string)$subquery . ')';
   +          $tables[$nalias]['arguments'] += $subquery->arguments();
   

   not sure that $subquery will always get right placeholders

1. https://drupal.org/node/767608
2. Any time the version is changed back to D7, you're delaying the patch for both D7 and D8.
3. A patch for D7 will be made when the D8 patch is committed to Core.

Is it acceptable to post patches for D7 here?

@maximpodorov - yes.

At the moment you can only pick one version for each issue, but they often go back/forth as needed.

If you want it tested, then you can switch it back to D7, run the bot, then when that's passed or failed, switch it back to D8.

@andypost in #198, thank you for a clear summary here of what still needs to happen.

Really your item #3 is the big concern here.

I think #2, the issue in #115, is not an issue -- if there is any kind of join, moving the conditions to the join results in the desired result. So it does not matter if it is an inner or an outer join -- if it's an outer join, the conditions MUST be on the join, whereas if it's an inner join, you get the same result regardless of whether the conditions are on the join or in the where clause.

But the placeholder brittleness is a big concern -- I'm not quite sure how to get a test case to illustrate this, or a better approach for this problem. Do you (or anyone else) have any suggestions for that? The test cases we have (and even the previous placeholder hackery) arose from conflicts running other modules that resulted in placeholder name collisions -- the solutions have worked for me, but I'm not confident they work everywhere.

Also, there is one other issue, raised by @gilsbert -- a specific scenario built up that fails in Postgres but passes in MySQL. Would like to get a test written for that. I don't understand what that problem is.

#195 did the job for me. Thank you Andrey.

Hi @freelock.

I will try to give more information about the postgresql's scenario.

I found an error message in the database log that explain why statements (b) and (c) reported at #160 were not being executed in postgresql database when the site was visited by an anonymous user (for superuser everything was working).

FIRST TRY (after applying the patch #174)

The following SQL was the (b) statement for an anonymous user:

SELECT COUNT(*) AS expression
FROM
(SELECT 1 AS expression
FROM
node node
LEFT JOIN field_data_field_parent_a field_data_field_parent_a ON node.nid = field_data_field_parent_a.field_parent_a_target_id AND (field_data_field_parent_a.entity_type = 'node' AND field_data_field_parent_a.deleted = '0')
LEFT JOIN node field_parent_a_node ON field_data_field_parent_a.entity_id = field_parent_a_node.nid AND EXISTS(SELECT na.nid AS nid
FROM
node_access na
WHERE (( (na.gid = '1') AND (na.realm = 'parent_a') )OR( (na.gid = 'parent_b') AND (na.realm = '0') )OR( (na.gid = 'all') AND (na.realm = '0') ))AND (na.grant_view >= 'content_access_author') AND (field_parent_a_node.nid = na.nid) )
LEFT JOIN field_data_field_parent_b field_data_field_parent_b ON node.nid = field_data_field_parent_b.field_parent_b_target_id AND (field_data_field_parent_b.entity_type = 'node' AND field_data_field_parent_b.deleted = '0')
LEFT JOIN node field_parent_b_node ON field_data_field_parent_b.entity_id = field_parent_b_node.nid AND EXISTS(SELECT na.nid AS nid
FROM
node_access na
WHERE (( (na.gid = '1') AND (na.realm = 'parent_a') )OR( (na.gid = 'parent_b') AND (na.realm = '0') )OR( (na.gid = 'all') AND (na.realm = '0') ))AND (na.grant_view >= 'content_access_author') AND (field_parent_b_node.nid = na.nid) )
WHERE (( (node.status = '1') AND (node.type IN ('parent_a', 'parent_b')) ))AND ( EXISTS (SELECT na.nid AS nid
FROM
node_access na
WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'content_access_author') )OR( (na.gid = '1') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) ) subquery

You will find comparations with field "gid" in table "node_access" using a string value ('parent_b' and 'all').
This will not work on postgres because the gid field is a bigint and the database can't convert the strings into an integer value!

In mysql it might be working and returning "false" as result on those comparations.
In postgresql it generates an error and the statement fails.

SECOND TRY (after applying the patch #195)

EUREKA!

The patch did the work and fixed the issue!
The view is working and correctly showing the nodes for superuser and anonymous!

The following SQL is the new statement (b):

SELECT COUNT(*) AS expression FROM (SELECT 1 AS expression FROM node node LEFT JOIN field_data_field_parent_a field_data_field_parent_a ON node.nid = field_data_field_parent_a.field_parent_a_target_id AND (field_data_field_parent_a.entity_type = 'node' AND field_data_field_parent_a.deleted = '0') LEFT JOIN node field_parent_a_node ON field_data_field_parent_a.entity_id = field_parent_a_node.nid AND EXISTS(SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'content_access_author') )OR( (na.gid = '1') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (field_parent_a_node.nid = na.nid) ) LEFT JOIN field_data_field_parent_b field_data_field_parent_b ON node.nid = field_data_field_parent_b.field_parent_b_target_id AND (field_data_field_parent_b.entity_type = 'node' AND field_data_field_parent_b.deleted = '0') LEFT JOIN node field_parent_b_node ON field_data_field_parent_b.entity_id = field_parent_b_node.nid AND EXISTS(SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'content_access_author') )OR( (na.gid = '1') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (field_parent_b_node.nid = na.nid) ) WHERE (( (node.status = '1') AND (node.type IN ('parent_a', 'parent_b')) ))AND ( EXISTS (SELECT na.nid AS nid FROM node_access na WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'content_access_author') )OR( (na.gid = '1') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) ) subquery

The field gid is now only compared with integer values!
Everything working perfectly.

Thank you very much.

Will this change be commited to Drupal or is there more work to do?

Regards,
Gilsberty

P.S.: I believe this issue is not related with https://drupal.org/node/2142107 since the patch #195 fixed the issue!

Hi.

I believe the patch #195 introduced a new issue with the testing module (file node.test).

After applying the patch the "simpletest" stops to work.

PHP's log shows the message: PHP Fatal error: Cannot redeclare class NodeAccessSubqueryTest in /srv/www/drupal/versao-atual/modules/node/node.test on line 3153.

May we fix this?

Regards,
Gilsberty

Hello,

For me, after apply the patch #195,
all my views with a "Filter criteria" like "Domain Access: Available on current domain (True)" disappear, on anonymous acces, superuser is ok.
:-(

I too have a View with an Entity Reference in the Relationships area. All was fine until I started using the Views Unpublished module which caused the View to disappear for those users who didn't have the rights to see unpublished nodes that appeared in that View. Applying the patch in #195 works for me. Thank you all for your work on this, sticking with it, and getting us here.

This patch worked for me!

My use-case: I had a View that was supposed to use a non-required relationship-provided field as a filter, ORed with some other filter conditions on fields that didn't share the relationship. Nothing that wasn't in the relationship was showing up.

Patch fixed it, and no side-effects that I can tell. Thanks!

It sounds like the postgre SQL problem is fixed, and there are quite a few reviews of the patch here saying it worked. What else do we need to get this committed to the next release? Is there an error in the test but not the patch?

Hi @Renee S.

Yes. There is a small problem with the test as I reported at #206.

Regards,
Gilsberty

#195 worked great for me too.

Similar context: View with an non-required entityreference field relationship to access additional field from the referenced entity.

Anonymous users did'n't get the rows with an empty referenced entity.

Patched fixed the issue, thanks, no side-effects for the moment.

As alexanderpas said in comment #199, setting back to D7 slows the process down.

Re-rolled D7 is great, just upload the D7 patch ending in "do-not-test.patch" and leave the version alone ;)

WARNING: #195 solves the issue, yet it does cause another issue.

After applying patch #195, anonymous users can't search any contents on the site using drupal's default search.

Patch needs more work!

# 195 works for me, untill now without side effects.

#195 works for me on d7.

#195 (with or without) Doesn't work for me for this query:

I have to turn Disable SQL rewriting on for Anonymous to get any view.

SELECT file_managed_field_data_field_image.fid AS file_managed_field_data_field_image_fid, commerce_product_field_data_commerce_product.product_id AS commerce_product_field_data_commerce_product_product_id, 'commerce_product' AS field_data_field_image_commerce_product_entity_type
FROM 
{commerce_product} commerce_product
LEFT JOIN {field_data_field_product} field_data_field_product ON commerce_product.product_id = field_data_field_product.field_product_product_id
LEFT JOIN {node} field_product_commerce_product ON field_data_field_product.entity_id = field_product_commerce_product.nid
LEFT JOIN {field_data_field_cards} field_data_field_cards ON commerce_product.product_id = field_data_field_cards.entity_id AND (field_data_field_cards.entity_type = 'commerce_product' AND field_data_field_cards.deleted = '0')
LEFT JOIN {commerce_line_item} commerce_line_item_field_data_field_cards ON field_data_field_cards.field_cards_line_item_id = commerce_line_item_field_data_field_cards.line_item_id
LEFT JOIN {field_data_commerce_product} commerce_line_item_field_data_field_cards__field_data_commerce_product ON commerce_line_item_field_data_field_cards.line_item_id = commerce_line_item_field_data_field_cards__field_data_commerce_product.entity_id AND (commerce_line_item_field_data_field_cards__field_data_commerce_product.entity_type = 'commerce_line_item' AND commerce_line_item_field_data_field_cards__field_data_commerce_product.deleted = '0')
LEFT JOIN {commerce_product} commerce_product_field_data_commerce_product ON commerce_line_item_field_data_field_cards__field_data_commerce_product.commerce_product_product_id = commerce_product_field_data_commerce_product.product_id
LEFT JOIN {field_data_field_image} commerce_product_field_data_commerce_product__field_data_field_image ON commerce_product_field_data_commerce_product.product_id = commerce_product_field_data_commerce_product__field_data_field_image.entity_id AND (commerce_product_field_data_commerce_product__field_data_field_image.entity_type = 'commerce_product' AND commerce_product_field_data_commerce_product__field_data_field_image.deleted = '0')
LEFT JOIN {file_managed} file_managed_field_data_field_image ON commerce_product_field_data_commerce_product__field_data_field_image.field_image_fid = file_managed_field_data_field_image.fid
WHERE (( (field_product_commerce_product.nid = '2412' ) )AND(( (commerce_product.type IN  ('card_package')) )))

I have this problem after enabling View Unpublished module.

The rows of the views with a node entity_reference empty are not visible for any user. Just user-1 can see them.
Disabling this module it works for all the users with privileges.

I applied the patch #195 . With the patch applied the rows are shown for all the users with privileges, but all the other views (without node entity_reference) shows no results.

Any suggestion?

Hi @dobe,

Can you turn SQL rewriting back on, and post a copy of the generated SQL *without* the patch in #195 and also *with* the patch in #195?

The patch from #195 prevents Views to substitute (at least) the ***CURRENT_TIME*** placeholder (see views_views_query_substitutions() and views_query_views_alter()). I tracked that down to the call to $query->getArguments() which in turn calls $this->compile(). There it somehow will use the query conditions before Views can substitute them using its hook_query_TAG_alter().

This can be reproduced by creating a simple node based View with default settings. Add the filter criteria Content: Has new content. As soon as Node Access kicks in the problem occurs. I triggered it using the Content Access modul (default settings) and accessing the Views path as standard authorized user. The resulting SQL code was:

SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created
FROM node node
LEFT JOIN history history ON node.nid = history.nid AND history.uid = '3'
INNER JOIN node_comment_statistics node_comment_statistics ON node.nid = node_comment_statistics.nid
WHERE (( (node.status = '1') AND ((history.timestamp IS NULL AND (node.changed > (***CURRENT_TIME*** - 2592000) OR node_comment_statistics.last_comment_timestamp > (***CURRENT_TIME*** - 2592000))) OR history.timestamp < node.changed OR history.timestamp < node_comment_statistics.last_comment_timestamp) ))AND ( EXISTS  (SELECT na.nid AS nid
FROM node_access na
WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '3') AND (na.realm = 'content_access_author') )OR( (na.gid = '2') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) 
ORDER BY node_creat

Query without patch looks like this:

SELECT node.title AS node_title, node.nid AS nid, node.created AS node_created
FROM node node
LEFT JOIN history history ON node.nid = history.nid AND history.uid = '3'
INNER JOIN node_comment_statistics node_comment_statistics ON node.nid = node_comment_statistics.nid
WHERE (( (node.status = '1') AND ((history.timestamp IS NULL AND (node.changed > (1412939965 - 2592000) OR node_comment_statistics.last_comment_timestamp > (1412939965 - 2592000))) OR history.timestamp < node.changed OR history.timestamp < node_comment_statistics.last_comment_timestamp) ))AND ( EXISTS  (SELECT na.nid AS nid
FROM node_access na
WHERE (( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '3') AND (na.realm = 'content_access_author') )OR( (na.gid = '2') AND (na.realm = 'content_access_rid') ))AND (na.grant_view >= '1') AND (node.nid = na.nid) )) 
ORDER BY node_created DESC
LIMIT 10 OFFSET 0

SelectQuery::getArguments() calls SelectQuery::arguments() so using arguments() instead of getArguments() does fix the problem described in #224. Modified patch from #195 for Drupal 7.31 is attached.

Problems mentioned in #198 remaining, of course.

I think I have the same problem here with Drupal 7.31 (running over MySQL), Search By Page 7.x-1.3 and Profile 2 7.x-1.3 modules. If you create a profile for a user wich contains the word X and a create a node with the same word and then index all the content on the site, if you search for the word X with uid=1, then you get two search results (the node and the user profile), but if you search for the word X as anonymous user, then you get only one result (the node).

The problem here is that the Search By Page module tags the query with the node_access tag, so Drupal core rewrites the query to check for node access permissions. This works ok with nodes, but not when searching for users because the node_access table shouldn't be checked for users. This is the SQL query once Drupal core has rewritten it:

SELECT i.type AS type, i.sid AS sid, SUM((11.1543620469 * i.score * t.count)) AS calculated_score
FROM 
search_index i
INNER JOIN sbp_path sp ON i.sid = sp.pid
LEFT OUTER JOIN node sbpn_n ON sbpn_n.nid = sp.modid
LEFT OUTER JOIN db_temporary_0 sbpp_p ON sbpp_p.pid = sp.modid
LEFT OUTER JOIN users sbpu_u ON sbpu_u.uid = sp.modid
INNER JOIN search_total t ON i.word = t.word
INNER JOIN search_dataset d ON i.sid = d.sid AND i.type = d.type
WHERE  (sp.environment = :db_condition_placeholder_0) AND (sp.language = :db_condition_placeholder_1) AND( (0=1) OR( (sbpn_n.status = :db_condition_placeholder_2) AND (sp.from_module = :db_condition_placeholder_3) )OR( (sp.from_module = :db_condition_placeholder_4) )OR( (sbpu_u.status = :db_condition_placeholder_5) AND (sp.from_module = :db_condition_placeholder_6) ))AND( (i.word = :db_condition_placeholder_7) )AND (i.type = :db_condition_placeholder_8) AND ( EXISTS  (SELECT na.nid AS nid
FROM 
node_access na
WHERE (( (na.gid = :db_condition_placeholder_9) AND (na.realm = :db_condition_placeholder_10) ))AND (na.grant_view >= :db_condition_placeholder_11) AND (sbpn_n.nid = na.nid) )) AND( (d.data LIKE :db_condition_placeholder_12 ESCAPE '\\') )
GROUP BY i.type, i.sid
HAVING  (COUNT(*) >= :matches) 
ORDER BY calculated_score DESC
LIMIT 10 OFFSET 0

As you can see, the problem is in the " EXISTS (SELECT na.nid AS nid..." subquery when the "LEFT OUTER JOIN users" is present.

Hope this helps to catch this annoying bug in Drupal Core.

GoddamnNoise, have you tried to apply patch from this issue?

Yes, i've tried to apply the patch in #225 with no success.

I have a couple of views that didn't work with standard core node.module (Drupal v. 7.32).

The first view is a query that has a node table appearing twice with a left join. Patch #174 fixed the issue.

Then I built another view (a completely different one) with an Entity Reference in the Relationship area. The view has an exposed filter based on a field belonging to the referenced Entity.
Patch #174 caused a problem since the pager of the view disappeared each time I filled in the filter field even though there were more rows matching the specified criteria than those displayed in the first page.

After applying patch #195 the issue is fixed. The SQL code generated by the view is the same both under patch #174 and #195. But now the pager works correctly. I noticed that under patch #195 the internal "count_query" (see code in function execute() in module views_plugin_query_default.inc) now returns the correct number of total rows and thus the pager is properly displayed.

#195 did a great job for me for both views. Thanks a lot.

Similar issue. Drove me crazy for while. Applied patch in #195 → to Drupal 7.32 and I'm happy to report that it works!

Thanks @AndreyMaximov and thank you all!

p.s. the issue that lead me here → .

We use #195 in our project, to make entity references and views and "view_unpublished" modules work together.

However: search stopped working, after adding the "search_config" module.

Problem: view_unpublised and search_config both alter query_node_access. The above patch tries to handle several of these, by counting and increasing "query->nextPlaceholder()". The idea being, that the separate subqueries use :db_condition_placeholder_XX and the "XX" part counting up through the subqueries.

That does not work.

The internal counter for subqueries is increased, but it seems to me it's also ignored or reset. Instead it seems the arguments are just counted, which causes each subquery to start at the same value and causes collisions between the subquery placeholders.

So instead, I altered #195 (and #225) to not use $query->condition at all, but ->where() instead, which allows me to name the placeholders myself.
I also removed the placeholder counting part added by above patches.

Patch in #231 with 7.34 worked like a charm.

What is the latest D8 version of this patch? Lots of recent focus on D7, but we need it fixed in Drupal 8. What's the last D8 patch?

I agree with #239, it worked me too.

Issue #2213729 → has been marked as duplicate of this issue (and patch in #231 solved the problem).

Patch #231 works for me. Is this a confirmed bug in D8? It's been a number of years since this was first reported, and it seems like a number of folks have reviewed this for D7.

Confirmed #231 for D7.36 (I had been using the prior patch; this one also works.)

W00t! Test passed for D7.

Patch in #231 with 7.35 worked like a charm.

yes it works, when will this patch go into the official release ?, it is marked as Major and I think it is important enough to make it into the next 7.36 release...

Added related issue #2213729: Items with empty entity reference field not shown in view when entity-reference based relationship is included →

Also, #231 works for 7.37

Just tried #231 on Drupal 7.38 and it appears to have fixed a longstanding bug on a View on one of our websites... fix would be nice in 7.x core...

Also just tried #231 on Drupal 7.38 and it solves a bug with dissappering rows I had with a View using a not required relationship. Hope this goes into core asap.

Patched Drupal 7.38 with #231 and I can confirm that this fixed the missing nodes on views under non-administrators.

Had a non-required node reference relation in a view. Wasn't working for non-administrators.
Patch #231 on 7.38 fixed the issue.

Patch #231 works! My setup is Drupal 7.38 - Views 7.x-3.11 - Taxonomy Access Control Lite 7.x-1.2.

Patch #231 worked for me on Drupal 7.38 with Entity Reference 7.x-1.1 and Views 7.x-3.11

When will patch #231 join next Drupal core release and fix this blocking bug? I have no idea how does it work.

Even with patch 231 I am still running into an issue where $subquery is not being generated. What are the odds that we could get a wrapper in place that tests to see if it exists before running the:

if ($type == 'entity' && count($subquery->conditions())) at the end f the function.

Where I am butting my head against it is a add another date field to a drupal commerce item.

Thanks in advance.

Patch #231 solved my issue with this in Drupal 7.39. Would be nice to see it rolled in...it's unclear why this isn't in it yet and there hasn't been much discussion beyond the patch working for various versions.

I've flipped this to RTBC, because, I believe it is. (Yes, it works for me under 7.39 also)

I've added a needs-maintainer-reply tag to this, hopefully a core maintainer can take a look and advise, since this is so helpful and has been around for so long and was passing tests, and needs a helping hand...

There are a couple things that need work on this issue:

1) Patch Drupal 8. If the problem does not exist in Drupal 8, update the issue summary to explain why this does not need to be fixed for D8.

2) The reason why the patches pass tests is because no tests exist to catch the original bug. Any patch that would be a candidate for committing to D7 will need to include a test that verifies that the bug exists and that the patch fixes it.

I'll try to do this by early next week.

1.) Provide a D7 Test
2.) Check D8 for this and include a patch if necessary.

Ok, I added a simplified version of the test from the earlier patches here.
It works in that a non patched version will fail for not retrieving the correct amount of nodes.

I have to point out though, that I am pretty sure, that this does not fix the underlying issue.
The placeholders in queries or more specifically subqueries, get mixed up. Two or more subqueries with conditions that require a variable to be included in the condition, will cause collisions, because variables from the later subqueries are replacing the ones from the earlier subquery. This is in turn is a somewhat correct behavior, because the naming of the placeholders is incorrect. The database-engine expects each placeholder to be unique through the entire query, but within the subqueries, you get duplicates.
This points to issues in the actual database engine.

The earlier patches (#195, #225) not working any more might (just might) be related to Drupalgeddon and the changes of the replacement-pattern handling there.

@ChristianAdamski: Looking at the diff between #231 and #269 I see that, in addition to the added test, this part was removed. Was that intentional?

-  $tables = $query->getTables();
+  $tables = &$query->getTables();

Changing the version/status to test that last patch.

@Michelle: I honestly do not know anymore. But we are altering $tables. And that pass by reference variant "$tables = &$query->getTables();" is used at at least one other occasion in that file. So I think I actually removed that "&" from the patch by accident...

Maybe add another patch to test that? Sadly don't know when I get around to check that. I'll try though.

I have been trying to use some of my contrib time to test this patch but I can't reproduce the problem. I don't know why the patch was on the client site so I can't test it there and I can't reproduce this in a fresh D7 install. I've tried both the repro steps and also following along with the original report and both times all the nodes show up whether I am logged in or not.

Hopefully someone who is having this problem can confirm your patch works. All I can confirm is that it didn't cause any visible problems on the client site when I tested it there.

This is affecting my current project. Applied the patch from #269 and all the nodes re-appeared in the view.

Patch 231 fixed the following problem on our site, currently running Drupal 7.41. Steps to reproduce problem:

1. Create node type Article with Taxonomy field "Target user group". Create node type Employee and use the Entity Reference module to add an "Author" field on the Article type that references the Employee entity. The "Author" field is *not* required.
2. Create a couple articles, some linked to a "All users including guests" group and some linked to a "Registered users" group. Make sure one article linked to the "Registered users" group does not have a value for the Author field.
3. Use Taxonomy Access Lite Control to restrict access to nodes linked to the "Registered users" taxonomy term to authenticated users.
4. Now create a view of Articles including their Authors. Make sure the Author relationship is not required.
5. As an administrator, it will appear the view is working correctly. As an anonymous user, it will appear the view is working correctly. Log in as a standard registered user and you will be unable to see the Article without an Author.... until you apply path 231 ;)

#269 seems to correct the problem for me (d7)

Just to state what I am aware is still missing here:

- Review of the patch and especially the test by smarter people than me.
- Addition of another test following #272++. Copied from earlier patch versions here, my latest patch version also adds the node-access-check for joined tables. But as pointed out in #272, I accidentally disabled that, by not passing by reference the $tables variable. What I do not really get, is what scenario exactly is prevented by this. I guess a joined table could be restricted and therefor should be checked, but I don't know, why this should happen here. So this leaves us with 3 options:
-- Remove the addition of the node access check for joined tables here. It's not working anyway right now.
-- Restore the missing pass-by-reference line and assume this is doing a good thing.
-- Wait for somebody who can describe a scenario where those checks are actually needed and build a test based on that.

Patch 231 worked for me Drupal version 7.39

Patch 269 worked for me Drupal version 7.41
Thanks

Patch 269 does NOT work for me -- because I need the node_access check on joined tables. And therefore the deleted '&' discussed in #272 and #274 needs to be re-inserted.

Re: #279. Disabling node_access checks for joined tables is not what the OP requested -- plus it would disable checks for inner joins that are currently functional and therefore presumably being used. So, if a scenario and test are needed to make it happen, here goes.

A variant of #277 provides a scenario:

1. Create node type Article with Taxonomy field "Target user group". Create node type Employee and use the Entity Reference module to add an "Author" field on the Article type that references the Employee entity. The "Author" field is *not* required.
2. Create a couple articles, some linked to a "All users including guests" group and some linked to a "Registered users" group. Assign different authors to each article.
3. Use Taxonomy Access Lite Control to restrict access to nodes linked to the "Registered users" taxonomy term to authenticated users.
4. Now create a view listing the Employee nodes. Add a non-required relationship to Articles. Add the Article node title as another column in the viewed table.
5. The resulting table should list all employees, for all users. The node access check should only affect the article-title column, ensuring that only titles visible to the current user are displayed. The bug in the original code is that employees with non-visible articles were removed from the table completely instead of being restricted. The bug in patch 269 is that restricted titles are visible to all users.

Finally, the patch I'm uploading reinserts the '&', plus expands the test -- the test now fails in all identified situations.

(I moved it back to Drupal 8 since the Drupal 7 testbot run is finished - we still need to get an up-to-date Drupal 8 patch here so it can be fixed there first.)

I just saw this and I think it's related to #1969208: Query node access alter filtering out accesible nodes → which also has a D8 patch waiting for review in #1969208-83: Query node access alter filtering out accesible nodes →

I marked #1969208: Query node access alter filtering out accesible nodes → as a duplicate of this one.

The patch is a little bit different from the one provided here. I would need steps to reproduce why the count is required for gids to see if it affects D8 or not. Note that in D8 the code is different for that part.

From the duplicate issue steps to reproduce this on D8

Reproduction and testing steps

1. Install module attached in #1349080-286: node_access filters out accessible nodes when node is left joined →
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

+++ b/core/modules/node/src/Tests/NodeAccessJoinTest.php
@@ -0,0 +1,179 @@
+    $node = $this->drupalCreateNode($edit);

variable $node does not get used anywhere.

Also, since this is a new file and we are using the shorthand array style in some places, we might as well use it consistently.

Also, since this is a new file and we are using the shorthand array style in some places, we might as well use it consistently.

Were exactly do we use that? I looked at NodeAccessBaseTableTest, NodeAccessFieldTest, NodeTestBase and WebTestBase and I don't see that.

Actually that's the first time I see that in Drupal.

The use of PHP 5.4+ short array syntax for Drupal 8.x is being discussed and is used in some patches already. When used, try to apply it consistently to at least a whole method or function. Short array syntax should not be used in Drupal 7.x or 6.x core or contributed modules

So that's only discussed.

+++ b/core/modules/node/src/Tests/NodeAccessJoinTest.php
@@ -0,0 +1,179 @@
+    $this->regularUser = $this->drupalCreateUser(['access content']);

For example, It was used here within the new file. So we might as well use it in all places.

We could remove some of the unneeded settings in the view to make it simpler, but since that is what the configuration system exported, maybe we should just leave it that way?

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_access_join.yml
@@ -0,0 +1,264 @@
+      fields:
+        title:
+          id: title
+          table: node_field_data
+          field: title
+          entity_type: node
+          entity_field: title
+          alter:
+            alter_text: false
+            make_link: false
+            absolute: false
+            trim: false
+            word_boundary: false
+            ellipsis: false
+            strip_tags: false
+            html: false
+          hide_empty: false
+          empty_zero: false

so many settings...

Also, it's interesting the last D8 patch (#148 maybe) in this issue did not use a view just a db_select(). But I think I like the view better because it provides more test coverage.

The array stuff is OK.

In the module I created and uploaded I don't use a view. But since for D8 we do use views we should use that.

When porting to D7 we will need a custom page.

From the duplicate issue, Credit should also be attributed to: benjifisher, TechNikh, Ericmaster

(Deleting tag from #266 -- effectively answered by #267)

After reading/digesting this entire issue, I've realized that the coding approach used in the patch is somewhat more contentious than I had thought. Adopting the patch from #1969208: Query node access alter filtering out accesible nodes → effectively reverses an earlier decision, and therefore I think the change needs to be justified.

The two coding approaches under consideration are:

1. Add an is-null-check (as in #289, and in much older patches such as #41)
2. Move the node-access conditions into the join conditions (first proposed in #65, most recently used in #282, e.g. the latest d7 patch).

The original change from approach #1 to approach #2 was made following a lot of discussion (e.g., #36, #62, #65, #132), and approach #2 was endorsed by multiple contributors (see #71, #72).

In short, my vote is for approach #1 -- but only if it's possible to prove through tests that it can correctly handle various problems identified in earlier discussions. (At which point the d7 patch would need to be revised).

To explain more fully, I believe that approach #2 is perhaps the more elegant solution. However, implementing approach #2 cleanly isn't possible (at least not in d7 -- I don't know d8 well enough yet), so it ends up requiring some hacky and fragile coding. The issue being that DatabaseCondition objects (such as $subquery) can not be used as join conditions. Rather, the join condition has to be a string, and any placeholders need to be manually created and added to the join arguments. In other words the subquery can't be moved using simple code like:

$tables[$nalias]['condition'][] = $subquery;

Instead the code has to be:

$tables[$nalias]['condition'] .= ' AND EXISTS(' . (string)$subquery . ')';
$tables[$nalias]['arguments'] += $subquery->arguments();

Which involves casting $subquery into a string, as well as manually naming the placeholders used in the arguments. (See #111). This has caused numerous problems: postgresql fails when casting $subquery into a string (#161); generating unique placeholder names is not straightforward (#190). Which leads me to conclude that approach #1 is more robust and therefore preferable.

However, concerns have been raised about whether approach #1 works correctly in all situations. I've been able to identify two primary concerns that motivated the switch away from approach #1:

1. Is access granted to nodes that have no entries in the node_access table (see #36, #62)?
2. Is access granted to nodes that have multiple entries in the node_access table, when all of the entries deny access (see #80)?

The earlier discussions imply that adding an is-null-check will incorrectly grant access in these two cases. While that would be true if the patch added isNull("node_access.nid"), all of the submitted patches instead add isNull("$nalias.$nfield") -- testing the variable in the base table, not the joined table. In other words, the code change allows entries where the value of an entity reference field is null; it is impossible for such an entry to match any node, therefore it's impossible for it to grant access to any nodes.

Nevertheless, if we are going to switch away from an endorsed approach, I think tests have to be added to explicitly address all previously-raised concerns, and confirm that the patch behaves correctly in all identified scenarios. Which means I think at least three more tests are needed:

1. Test missing node_access entries: After creating a node and referencing it (e.g., adding it as a related_article), manually delete all entries for it in the node_access table. Confirm that the reference is hidden from all non-admin users.
2. Test multiple node_access entries: For example, need to have a list of permitted users for each private article, and create a node with at least two permitted users. Confirm that a reference to the node is hidden to non-permitted users.
3. Test multiple joins (see #160): For example, the related_articles need to also have related_articles. And/or there needs to be a second field, e.g. second_article. The view then needs to have three columns, to show all the related articles.

I may be able to put together some d7 code implementing these tests (or at least more clearly demonstrating them), but I haven't worked in d8 yet.

The D8 test can be changed to test for point and 3. I think that 2 is already covered by the test.

I also did a manual test and after deleting the entries for a private article I can't see the listing of the page in the table. Note that deleting directly from table should not happen and when I delete the node from the UI it also deletes it's reference from the page that was referencing it.

I also tested point 3 by adding another reference field to a page and it shows the results as expected. If I don't have access to any of the references I don't see the entry. If I have no access to one and the second is empty I don't see the entry. If I have access to one and the other is empty I see the entry.

I modified the test with the following:

1. Changed article to also allow the related article reference.
2. Change the view to display a 3rd column with the reference attached to the article.
3. In the test I create articles for both a selected user (author) and anonymous user with combination of private and public references.
4. I create pages with more combination, private, public, references both by the "author" and by the anonymous user.
5. I calculate the total of rows each user should see.
6. Some more text based check to be sure users don't see rows they shouldn't see. Regular should not see the word "private" at all, while "author" can not see "- private" which denotes a private article he is not author of. This test are to be sure the totals check are not just a fluke.
7. Check for the total number of rows each should see.

I attach screenshots with how the tables look for each user and let me know what other text checks I should include.

Changes base on @chx comment on IRC

One more patch. The previous didn't have 2 cases:

• page > private article > author private
• page > author private article > private

I added code to create those 2 situations and also I change the assertTrue() used for row count check to use assertEqual()

I complete agree and really, really like applying There Is No Kill Like Overkill to testing security related aspects of Drupal core.

Thanks for putting in the extra tests. I agree that the patch seems to have a pretty comprehensive set of tests now.

Re: #297. Yes, upon examining the node_access_test module more closely I confirmed that point 2 is already covered by the tests, i.e., private nodes already have 2-3 entries each in the node_access table. And given that you manually confirmed point 1, i.e., that the case with 0 node_access entries behaves properly, that seems like enough (since the basic node_access tests don't even check this case, and technically it should never occur).

My apologies, but somehow I messed up in my initial evaluation of whether an is-null check is equivalent to moving the condition into the join. I suppose the good news is that the tests in patch #302 (which I started to port into d7) do demonstrate the differences between the two approaches.

When I run the tests from #302 against the d7 code from #282 (where the node-access condition is moved into the join), all users see a table with 15 rows -- because all of the 'page' nodes are public. What changes is the contents of the 'Article' and 'Article 1' columns -- all inaccessible 'article' nodes are hidden, based on the current user's permissions.

And that's what a left join should do. Adding a left join to a query shouldn't make any of the original rows (e.g., the page nodes) disappear; it should just add extra information whenever it is available. The node-access condition places more limits upon whether the extra information is indeed available, but if it also makes the original rows disappear, then it's behaving like an inner join.

So unfortunately I think the patch needs to be rewritten.

And more bad news is that d8 still does not have a clean way of moving the node_access subquery into the join -- there's just a feature request at #2275519: Unable to use Condition objects with joins → .

From the IS:

When two node tables are left joined then access checks prevent access to the node when nothing is matched in the table on the right-hand side of the join.
Access should be granted when nothing is matched in the right-hand side (and the node on the left-hand side is permitted).

Exactly that is what the patch solves.

You want to change the last phrase to

Access should be granted when nothing is matched in the right-hand side (and the node on the left-hand side is permitted), but in case something is matched on right side check access on right side and show the left side while hiding the right side if access to right side is not permitted.

That I think is for another issue which can be opened after the issue here is fixed.

I move this back to RTBC. If the maintainers think we should expand the scope of this they can move it back to needs work.

I do not agree that this issue is anywhere near RTBC.

For four years all the development in this queue was on a patch where the condition was moved into the join. That is the approach that was endorsed by multiple early contributors (#71, #72), who spent a lot of time debating the merits of the different approaches. The dozens of different contributors to this issue over the last four years who have said that a patch works for them were all using a patch in which the condition was moved into the join.

The change in approach dates back only 2 weeks, and was reviewed primarily in another issue ( #1969208: Query node access alter filtering out accesible nodes → ), where alternatives were never discussed. The is-null-test approach was explicitly criticized by the early contributors to this issue.

In terms of the desired behaviour, the behaviour I'm advocating was requested by multiple contributors: For example #54:

The problem with the patch in 41/42/45 is that if you cannot access the attached node, it will deny access to the main node. I think that behavior is wrong.

#55:

In regards to still showing a result even if the user does not have access to the related node, I agree with you in that is the behavior desired.

#66:

Show me a list of nodes I can view. If there are any attached nodes I can view, show additional data from those nodes.

The summary that you quote was added in #143, after all of the previous comments had been incorporated into the patch design. So my interpretation is that the summary is incorrect. I'm willing to take a stab at revising the summary, but preferably only after we reach a consensus on what the summary should say.

Changing to Needs Work because The description in the comment of the test needs some re-working anyway. For example:

1. +++ b/core/modules/node/src/Tests/NodeAccessJoinTest.php
   @@ -0,0 +1,322 @@
   +   * - Create user with "access content" and "create article" permissions which
   +   *   will be author having access to some private articles but not others.
   

   be an author having

2. +++ b/core/modules/node/src/Tests/NodeAccessJoinTest.php
   @@ -0,0 +1,322 @@
   +   * - Create pages with articles as reference for any combination of article
   

   ??

3. +++ b/core/modules/node/src/Tests/NodeAccessJoinTest.php
   @@ -0,0 +1,322 @@
   +   *   he is not the author of.
   

   he => the user

The problem with the patch in 41/42/45 is that if you cannot access the attached node, it will deny access to the main node. I think that behavior is wrong.

I don't think we should fix this. That sounds like a feature request.

What might be helpful is if we could publicize the test case in the issue descriptions in simplest form possible so that we agree on how it should work. This would eventually become part of the documentation.
For example:

Left      Right   Role    Access
Public    Public  normal  granted
Public    [NULL]  normal  granted
Public    Private normal  ??

Admittedly, the problem on my site that led me here involves null-values. Which is why adding an is-null-check on my site appeared to work, and I didn't notice the differences between the two approaches initially.

My primary concern is that I don't want previous discussions / consensus /decisions in this issue to be overlooked, and especially not if it's based on my mistakes parsing those discussions.

The fact that the tests in #302 fail when used with the previously-accepted patch (#286) is objective evidence that the various symptoms are part of a single bug. I doubt tests should be added to core when there's disagreement about the expected results of the test.

Furthermore, reading through those discussions and examining the differing results has led me to conclude that the behaviour when the condition is moved into the join is correct. I think dropping null-value entries is the most visible symptom of this node_access bug -- because it will even affect views involving only public content, and most content on most sites is public. But hiding a public node because it is left-joined to a restricted node is another symptom of the same line of code, just one that affects fewer views and fewer users. Restricted content should only hide rows when it is required (i.e., inner-joined), not when it is optional (i.e., left-joined). And tagging a query with node_access should only remove restricted nodes, not public nodes.

In fact, I now know that my site has views where this an issue. My site has some complex views with multiple layers of left-joined nodes where access restrictions are critical. But missing data on those views had not been brought to my attention because it only affects a small subset of users, and those users didn't realize a handful of valid rows were missing. So just as with the tests, I can't support a patch now that I know it doesn't fix all the issues.

So we need to go with:

Show me a list of nodes I can view. If there are any attached nodes I can view, show additional data from those nodes.

which is the #282 fix? The tests will probably need to be updated. Are there any scenarios where this doesn't work and someone would intend the view to work the like the #302 fix? But first, we need an issue summary update because that's not what the issue summary reads.

I've made some revisions to the title/summary to bring it up-to-date, and I also tried to pull in some wording from #1969208: Query node access alter filtering out accesible nodes → .

To help facilitate a comparison, I'm uploading a patch that should basically be a d8 port of #282.

It's not identical to #282 -- I've pulled in a patch from #2275519: Unable to use Condition objects with joins → because it allows cleaner code. For now I'm uploading it without the new tests, because I know those will fail.

Apologies, I'm writing d8 code blindly because my about-to-be-replaced test machine can't run php 5.5.

Take three.

The last submitted patch, 317: node_access_for_left_joins_d8-1349080-317-without_tests.patch, failed testing.