From the IS:
When two node tables are left joined then access checks prevent access to the node when nothing is matched in the table on the right-hand side of the join.
Access should be granted when nothing is matched in the right-hand side (and the node on the left-hand side is permitted).
Exactly that is what the patch solves.
You want to change the last phrase to
Access should be granted when nothing is matched in the right-hand side (and the node on the left-hand side is permitted), but in case something is matched on right side check access on right side and show the left side while hiding the right side if access to right side is not permitted.
That I think is for another issue which can be opened after the issue here is fixed.
I move this back to RTBC. If the maintainers think we should expand the scope of this they can move it back to needs work.
One more patch. The previous didn't have 2 cases:
- page > private article > author private
- page > author private article > private
I added code to create those 2 situations and also I change the assertTrue() used for row count check to use assertEqual()
Changes base on @chx comment on IRC
I modified the test with the following:
1. Changed article to also allow the related article reference.
2. Change the view to display a 3rd column with the reference attached to the article.
3. In the test I create articles for both a selected user (author) and anonymous user with combination of private and public references.
4. I create pages with more combination, private, public, references both by the "author" and by the anonymous user.
5. I calculate the total of rows each user should see.
6. Some more text based check to be sure users don't see rows they shouldn't see. Regular should not see the word "private" at all, while "author" can not see "- private" which denotes a private article he is not author of. This test are to be sure the totals check are not just a fluke.
7. Check for the total number of rows each should see.
I attach screenshots with how the tables look for each user and let me know what other text checks I should include.
I also tested point 3 by adding another reference field to a page and it shows the results as expected. If I don't have access to any of the references I don't see the entry. If I have no access to one and the second is empty I don't see the entry. If I have access to one and the other is empty I see the entry.
The D8 test can be changed to test for point and 3. I think that 2 is already covered by the test.
I also did a manual test and after deleting the entries for a private article I can't see the listing of the page in the table. Note that deleting directly from table should not happen and when I delete the node from the UI it also deletes it's reference from the page that was referencing it.
The array stuff is OK.
In the module I created and uploaded I don't use a view. But since for D8 we do use views we should use that.
When porting to D7 we will need a custom page.
Also, since this is a new file and we are using the shorthand array style in some places, we might as well use it consistently.
Were exactly do we use that? I looked at NodeAccessBaseTableTest, NodeAccessFieldTest, NodeTestBase and WebTestBase and I don't see that.
Actually that's the first time I see that in Drupal.
The use of PHP 5.4+ short array syntax for Drupal 8.x is being discussed and is used in some patches already. When used, try to apply it consistently to at least a whole method or function. Short array syntax should not be used in Drupal 7.x or 6.x core or contributed modules
So that's only discussed.
I marked #1969208: Query node access alter filtering out accesible nodes → as a duplicate of this one.
The patch is a little bit different from the one provided here. I would need steps to reproduce why the count is required for gids to see if it affects D8 or not. Note that in D8 the code is different for that part.
From the duplicate issue steps to reproduce this on D8
Reproduction and testing steps
- Install module attached in #1349080-286: node_access filters out accessible nodes when node is left joined →
- Create 2 regular users (user1 and user2)
- Give authenticated users the permission to create articles
- Attach a "field_private" field of type "List (integer)" to the article content type which allows only one value. The allowed values for it should be:
0|Public 1|Private - Attach an entity reference to the page content type named field_related_article. It should allow node of type article
- Login with "user1". Create 2 articles. Leave one as default and the second one make it private by selecting the private value in the field_private select box
- Login with the admin and create 3 pages
- One with no related article
- one using the public article as reference
- one using the private article
- Login with "user1" and go to /test/access page.
- Expected result: a table with 3 rows, one for each page
- Actual result: a table with 2 rows, missing the page with no related article referenced
- Login with "user2" and go to /test/access page.
- Expected result: a table with 2 rows, one for each page
- Actual result: a table with 1 row, missing the page with no related article referenced
- Login with an admin and enable the "View private content" permission for authenticated users
- Login with "user2" and go to /test/access page.
- Expected result: a table with 3 rows, one for each page
- Actual result: a table with 2 rows, missing the page with no related article referenced
- Login with an admin and disable the "View private content" permission for authenticated users
- Apply the patch you wish to test
- Repeat steps 8 - 11 and now the actual result should mach the expected results
I just saw this and I think it's related to #1969208: Query node access alter filtering out accesible nodes → which also has a D8 patch waiting for review in #1969208-83: Query node access alter filtering out accesible nodes →