🇺🇸United States @Nephele

Updating tags

I've converted the patch into a merge request (effectively rerolling against 11.x) and added a line of code to fix the phpstan errors that made the latest tests fail.

Nephele → made their first commit to this issue’s fork.

@burnsjeremy: Patch #231 is functionally equivalent to #332 (and passes all the tests). Theoretically, however, #231 is less robust than #332 -- meaning that #231 is less likely to work on excessively complex queries.

Re: D7 patch testing in #332. I wanted to test the patch against other databases (given earlier problems reported for PostgreSQL), so I queued up some extra tests. But the alternate database tests all use PHP5.5 instead of PHP5.3. As far as I can tell, the test failures are not caused by the patch. Instead, I think core D7 can't pass tests under PHP5.5.

I'm also revising the issue summary based on my comments in #330; I think the summary now more accurately reflects the issue's status.

[Fixing some PHP5.5-specific syntax].

In the absence of any other feedback, I've been trying on my own to find ways to resolve the question of whether patch#326 → or patch#302 → is the better solution. For anyone starting at the end, both cover the minimum requirements (all node_access-restrictions are enforced; null-entry rows are not hidden), but patch#302 hides entire rows instead of just cells when there is restricted content.

I've created a new d7 patch derived line-for-line from (d8) patch#326, including all the tests. I need the patch for my own website; hopefully there are others who will also find it easier to use and provide feedback given a d7 patch.

Using these d7 tests, I've been able to evaluate many of the patches previously uploaded to this issue. The results were:

My conclusion is that patch#326 is what comments #1-#281 were working to achieve; the alternative patch#302 is not. By my count, that includes 17 different users reporting that patch#231 worked for them, all of whom should get identical results with patch#326 (or the d7 version added here).

Finally, the tie-breaker for me is that patch#326 can satisfy everyone's needs, but patch#302 cannot. If patch#302 is adopted, it is impossible for a query to generate results comparable to patch#326 -- patch#302 forcibly excludes entire rows, even though the rows contain publicly visible information. On the other hand, patch#326 will work for users who prefer patch#302, e.g., users who want to exclude public nodes because those nodes have non-visible links to private nodes. Those users can just add appropriate filters/conditions to their query (e.g., "where field_related_article.target_id is not null and related_article.id is null").

To help visualize the tests in #326 I'm uploading screenshots of the views generated by the tests. Note that the displayed node titles are of the format "Node_type node_status - linked_node_status" -- so whether a user can access a node is based solely on the information before the dash (-).

Three of the screenshots are the views generated by #326. For comparison I've also provided one screenshot if the code from #302 is substituted in, and two screenshots for non-patched code.

The only difference between 321 and 326 is that tests were added. Nevertheless, I'm uploading an interdiff.

I've now added tests to this version of the code. The tests are derived from the tests in #302, but I ended up doing a fair bit of refactoring. Plus of course changing the details so that the approach I've been using passes the tests.

There are a few commented-out bits of code that can be enabled to make the tests more similar to those in #302 and #299 -- they're not intended to be part of the final code, but I left them in place in case anyone else is trying to make more direct comparisons with the previous tests.

[Also added a link to #106721: Optimize node access query building → -- which is making edits to the same function. Earlier patches had overlap issues, but the approaches used in #302 and #321 are overlap-free (even if/when a D7 backport is created).]

Re: API change. This is actually a slightly odd situation; in some ways my edit is more like a documentation fix than an API change. The edit isn't changing the capabilities of the alterQuery function; it's just making the function definition provide correct information about whether arguments may change.

In the original code, alterQuery already has the ability to make changes to $tables. This is because $tables is actually a redundant argument -- it's just a reference to an array stored within $query (i.e., the object provided as the first function argument). alterQuery can make any changes it wants to $query, and therefore it can make any changes it wants to $tables. To demonstrate, I'm uploading a new version of the patch which has the exact same functionality, but does it without an API change.

Personally, I think the code in #318 is better, but I realize that in the context of a large project, a stable API may be more important than good code. Does anyone have any feedback on which one is preferable?

Take three.

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

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.

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

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.

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.

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

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.

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

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.

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

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.