Incorrect permission check in views node access filter

This was reported and discussed in private and deemed to be OK to be worked on in public given it requires the "attacker" to have administer nodes.

I was looking at writing a test for this, but even when the node table is not the base table, node access is checked anyway. Adding this filter to the test view didn't change anything.

So is there a scenario where this filter might actually be useful?

So is there a scenario where this filter might actually be useful?

Its useful for checking edit access.

+++ b/core/modules/node/src/Tests/Views/FilterNodeAccess.php
@@ -0,0 +1,112 @@
+ * Tests the node_access filter handler.
+ *
+ * @group node
+ * @see \Drupal\node\Plugin\views\filter\Status
+ */
+class FilterNodeAccess extends NodeTestBase {

not sure but I think we still actually require tests to be suffixed with *Test, no?

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_filter_node_access.yml
@@ -0,0 +1,250 @@
+          disable_sql_rewrite: false

I think you need to disable this for this to make sense.

@dawehner: the filter is hardcoded to view, so you can't really use it for edit checking?

I'd say a use case would to combine it with OR conditions. And for some reason, you only want to respect node access for one of the cases.

Addressing #5 and #6

I think you need to disable this for this to make sense.

Ah yeah ok that makes sense, you want to disable all rewriting but still want to use the grant table.

Ok so now with the access filter added to the view and the correct permission set in the fix.

1. +++ b/core/modules/node/src/Tests/Views/FilterNodeAccessTest.php
   @@ -0,0 +1,112 @@
   +    foreach ($this->users as $web_user) {
   +      $this->drupalLogin($web_user);
   +      foreach (array(0 => 'Public', 1 => 'Private') as $is_private => $type) {
   +        $edit = array(
   +          'title[0][value]' => t('@private_public Article created by @user', array('@private_public' => $type, '@user' => $web_user->getUsername())),
   +        );
   +        if ($is_private) {
   +          $edit['private[0][value]'] = TRUE;
   +          $edit['body[0][value]'] = 'private node';
   +        }
   +        else {
   +          $edit['body[0][value]'] = 'public node';
   +        }
   +
   +        $this->drupalPostForm('node/add/article', $edit, t('Save'));
   +        $node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
   +        $this->assertEqual($is_private, (int)$node->private->value, 'The private status of the node was properly set in the node_access_test table.');
   +      }
   

   Is there a reason we shouldn't use the API to create those nodes?

2. +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_filter_node_access.yml
   @@ -0,0 +1,288 @@
   +base_table: users_field_data
   +base_field: uid
   

   Wait, this is confusing, why are we creating a view of users?

13.1 lazy copy/pasting existing tests, so no GOOD reason :) Changed
13.2 Well the help text for the handler says "Not necessary if you are using node as your base table.", so I started with a different base table when I wanted to test this. But the help text is obviously wrong, so now I changed it to a straight up node view. Should we change the help text here too or do a follow up?

Some nitpicks :)

1. +++ b/core/modules/node/src/Tests/Views/FilterNodeAccessTest.php
   @@ -0,0 +1,111 @@
   +          'body'      => [[
   +            'value' => $type . ' node',
   +            'format' => filter_default_format(),
   +          ]],
   +          'title'     => t('@private_public Article created by @user', ['@private_public' => $type, '@user' => $web_user->getUsername()]),
   +          'type'      => 'article',
   +          'uid'       => $web_user->id(),
   +          'private'   => (bool) $is_private,
   

   Let's get rid of all that whitespace.

2. +++ b/core/modules/node/src/Tests/Views/FilterNodeAccessTest.php
   @@ -0,0 +1,111 @@
   +        $this->assertEqual($is_private, (int)$node->private->value, 'The private status of the node was properly set in the node_access_test table.');
   

   Nitpick: one more space between (int) and $node

3. +++ b/core/modules/node/src/Tests/Views/FilterNodeAccessTest.php
   @@ -0,0 +1,111 @@
   +    $admin_user = $this->drupalCreateUser(['access content', 'bypass node access']);
   +    $this->drupalLogin($admin_user);
   

   It is weird to reuse an existing variable name

Thanks for the review as always.

Fixed all the feedback.

Ok time for bed obviously.....now a patch with some actual content...

Those were just nitpicks, so I think this is good to go?

+1 for the RTBC

Uploading a test only patch to see the test fail.

Berdir correctly pointed out the other patch wasn't a patch.

Back to RTBC then...

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!