Steps to reproduce:

- Enable content access (any module implementing hook_node_grants should work)
- Enable views and entity-reference modules
- Create two content types, let the first one reference the other through a entity-reference field
- Create some content of the first content type, but don't bother adding any relations
- Create a view with a relation through an entity-reference, don't mark this relation as required (LEFT JOIN)

Without the 'bybass node access' privilege, users will not get any results in this view.

It's easier to illustrate with an example.

Something like could for example be appended to the node query:

... 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_field_data_field_person.nid = na.nid))

where node_field_data_field_person is the referenced base table of a "person" content type referenced through an entity-refrence field. node_field_data_field_person.nid will in the case where no such relation exists be NULL (because of the LEFT JOIN from the node table). This will cause the EXISTS condition to evaluate to FALSE, and the row to be excluded.

To remedy this the query should be written like this to protect against the case where node_field_data_field_person.nid IS NULL.

... 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_field_data_field_person.nid IS NULL ) OR (node_field_data_field_person.nid = na.nid) )))

I will attach a patch with the needed changes.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gnucifer’s picture

gnucifer’s picture

Status: Active » Needs review
gnucifer’s picture

Have been running this in production on about 100 sites for a couple of weeks now, no issues. Any chance of it being reviewed and hopefully committed?

gnucifer’s picture

Any chance this patch can get some love? I'm sure there are many people affected by this bug, in our production environment there are about 4-5 sites that will bail out without the fix.

fengtan’s picture

We are also experiencing this problem.
Tested patch #1, confirmed it fixes the problem. Thanks.
The code makes sense to me. Would be great if someone from the core team could have a look.

spheresh’s picture

Patch was applied to my site. It works great.
Thanks.

spheresh’s picture

Priority: Major » Critical

Hi, guys. The required patch have been waiting to be approved already year

Does No one not use this functionality? Have you not using this functionality?

mgifford’s picture

Assigned: gnucifer » Unassigned

@spheresh want to mark it RTBC?

Does this also fail in D8? If so it needs to be fixed in D8 first and then backported.

David_Rothstein’s picture

Priority: Critical » Major

I am not positive, but I think this might be a duplicate of #1969208: Query node access alter filtering out accesible nodes?

mgifford’s picture

Version: 7.x-dev » 8.0.x-dev

We should test if this also fails in D8.

mgifford’s picture

Issue tags: +Needs backport to D7
benjifisher’s picture

I agree with the comment in #10: this is a duplicate. The patches on the two issues are similar, and we should decide which is the better fix.

There has already been some discussion on #1969208: Query node access alter filtering out accesible nodes of whether the problem still exists in D8. I think the answer is that it does, but there is another bug in Views that will have to be fixed before we can address this issue in D8.

Like others posting on this issue, I have patched Drupal core on a live site (just one) because of this problem. But I also know the policy: since we do not want to introduce regressions when moving to D8, we have to fix it there before the change will be accepted into Drupal core.

valthebald’s picture

Status: Needs review » Closed (duplicate)

It seems that everyone has agreed this is a duplicate of #1969208: Query node access alter filtering out accesible nodes (and the latter is more actively discussed)