Issue #2461049 by Arla, agentrickard, webflo, rteijeiro, xjm, Berdir: Node...

Issue #2461049 by Arla, agentrickard, webflo, rteijeiro, xjm, Berdir: Node module permissions are broken if hook_node_grants is implemented

core/modules/node/src/NodeAccessControlHandler.php

@@ -109,21 +109,8 @@ protected function checkAccess(EntityInterface $node, $operation, $langcode, Acc
       return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->cacheUntilEntityChanges($node);
     }
 
-    // If no module specified either ALLOW or KILL, we fall back to the
-    // node_access table.
-    $grants = $this->grantStorage->access($node, $operation, $langcode, $account);
-    if ($grants->isAllowed() || $grants->isForbidden()) {
-      return $grants;
-    }
-
-    // If no modules implement hook_node_grants(), the default behavior is to
-    // allow all users to view published nodes, so reflect that here.
-    if ($operation === 'view') {
-      return AccessResult::allowedIf($status)->cacheUntilEntityChanges($node);
-    }
-
-    // No opinion.
-    return AccessResult::neutral();
+    // Evaluate node grants.
+    return $this->grantStorage->access($node, $operation, $langcode, $account);
   }
 
   /**

core/modules/node/src/NodeGrantDatabaseStorage.php

@@ -69,8 +69,14 @@ public function access(NodeInterface $node, $operation, $langcode, AccountInterf
     // If no module implements the hook or the node does not have an id there is
     // no point in querying the database for access grants.
     if (!$this->moduleHandler->getImplementations('node_grants') || !$node->id()) {
-      // No opinion.
-      return AccessResult::neutral();
+      // Return the equivalent of the default grant, defined by
+      // self::writeDefault().
+      if ($operation === 'view') {
+        return AccessResult::allowedIf($node->getTranslation($langcode)->isPublished())->cacheUntilEntityChanges($node);
+      }
+      else {
+        return AccessResult::neutral();
+      }
     }
 
     // Check the database for potential access grants.
@@ -117,7 +123,7 @@ public function access(NodeInterface $node, $operation, $langcode, AccountInterf
       return $set_cacheability(AccessResult::allowed());
     }
     else {
-      return $set_cacheability(AccessResult::forbidden());
+      return $set_cacheability(AccessResult::neutral());
     }
   }
 

core/modules/node/src/NodeGrantDatabaseStorageInterface.php

@@ -106,7 +106,12 @@ public function writeDefault();
    *   The user for which to check access.
    *
    * @return \Drupal\Core\Access\AccessResultInterface
-   *   The access result.
+   *   The access result, either allowed or neutral. If there are no node
+   *   grants, the default grant defined by writeDefault() is applied.
+   *
+   * @see hook_node_grants()
+   * @see hook_node_access_records()
+   * @see \Drupal\node\NodeGrantDatabaseStorageInterface::writeDefault()
    */
   public function access(NodeInterface $node, $operation, $langcode, AccountInterface $account);
 

core/modules/node/src/Tests/NodeAccessGrantsTest.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\node\Tests\NodeAccessGrantsTest.
+ */
+
+namespace Drupal\node\Tests;
+
+/**
+ * Tests basic node_access functionality with hook_node_grants().
+ *
+ * This test just wraps the existing default permissions test while a module
+ * that implements hook_node_grants() is enabled.
+ *
+ * @see \Drupal\node\NodeGrantDatabaseStorage
+ *
+ * @group node
+ */
+class NodeAccessGrantsTest extends NodeAccessTest {
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = array('node_access_test_empty');
+
+}

core/modules/node/src/Tests/NodeAccessTest.php

@@ -58,6 +58,20 @@ function testNodeAccess() {
     // Tests the default access provided for a published node.
     $node5 = $this->drupalCreateNode();
     $this->assertNodeAccess(array('view' => TRUE, 'update' => FALSE, 'delete' => FALSE), $node5, $web_user3);
+
+    // Tests the "edit any BUNDLE" and "delete any BUNDLE" permissions.
+    $web_user6 = $this->drupalCreateUser(array('access content', 'edit any page content', 'delete any page content'));
+    $node6 = $this->drupalCreateNode(array('type' => 'page'));
+    $this->assertNodeAccess(array('view' => TRUE, 'update' => TRUE, 'delete' => TRUE), $node6, $web_user6);
+
+    // Tests the "edit own BUNDLE" and "delete own BUNDLE" permission.
+    $web_user7 = $this->drupalCreateUser(array('access content', 'edit own page content', 'delete own page content'));
+    // User should not be able to edit or delete nodes they do not own.
+    $this->assertNodeAccess(array('view' => TRUE, 'update' => FALSE, 'delete' => FALSE), $node6, $web_user7);
+
+    // User should be able to edit or delete nodes they own.
+    $node7 = $this->drupalCreateNode(array('type' => 'page', 'uid' => $web_user7->id()));
+    $this->assertNodeAccess(array('view' => TRUE, 'update' => TRUE, 'delete' => TRUE), $node7, $web_user7);
   }
 
 }

core/modules/node/src/Tests/Views/BulkFormAccessTest.php

@@ -105,7 +105,7 @@ public function testNodeEditAccess() {
 
     // Create an account that may view the private node, but can update the
     // status.
-    $account = $this->drupalCreateUser(array('administer nodes', 'edit any article content', 'node test view'));
+    $account = $this->drupalCreateUser(array('administer nodes', 'node test view'));
     $this->drupalLogin($account);
 
     // Ensure the node is published.

core/modules/node/tests/modules/node_access_test_empty/node_access_test_empty.info.yml

+name: 'Node module empty access tests'
+type: module
+description: 'Support module for node permission testing. Provides empty grants hook implementations.'
+package: Testing
+version: VERSION
+core: 8.x

core/modules/node/tests/modules/node_access_test_empty/node_access_test_empty.module

+<?php
+
+/**
+ * @file
+ * Empty node access hook implementations.
+ */
+
+use Drupal\node\NodeInterface;
+
+/**
+ * Implements hook_node_grants().
+ */
+function node_access_test_empty_node_grants($account, $op) {
+  return array();
+}
+
+/**
+ * Implements hook_node_access_records().
+ */
+function node_access_test_empty_node_access_records(NodeInterface $node) {
+  return array();
+}