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

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.