- Patch #920614 by agentrickard, chx, Damien Tournoud: unpublished content visible when a Node Access module is enabled.

4 files changed, 79 insertions, 17 deletions

diff --git a/modules/node/node.api.php b/modules/node/node.api.php
index 55c64dbbd..7d4f3996f 100644
--- a/modules/node/node.api.php
+++ b/modules/node/node.api.php
@@ -138,7 +138,10 @@
  * an array of the list IDs that this user is a member of.
  *
  * A node access module may implement as many realms as necessary to
- * properly define the access privileges for the nodes.
+ * properly define the access privileges for the nodes. Note that the system
+ * makes no distinction between published and unpublished nodes. It is the
+ * module's responsibility to provide appropriate realms to limit access to
+ * unpublished content.
  *
  * @param $account
  *   The user object whose grants are requested.
@@ -169,6 +172,12 @@ function hook_node_grants($account, $op) {
  * interested, it must respond with an array of permissions arrays for that
  * node.
  *
+ * Node access grants apply regardless of the published or unpublished status
+ * of the node. Implementations must make sure not to grant access to
+ * unpublished nodes if they don't want to change the standard access control
+ * behavior. Your module may need to create a separate access realm to handle
+ * access to unpublished nodes.
+ *
  * Note that the grant values in the return value from your hook must be
  * integers and not boolean TRUE and FALSE.
  *
@@ -177,7 +186,9 @@ function hook_node_grants($account, $op) {
  *   hook_node_grants().
  * - 'gid': A 'grant ID' from hook_node_grants().
  * - 'grant_view': If set to 1 a user that has been identified as a member
- *   of this gid within this realm can view this node.
+ *   of this gid within this realm can view this node. This should usually be
+ *   set to $node->status. Failure to do so may expose unpublished content
+ *   to some users.
  * - 'grant_update': If set to 1 a user that has been identified as a member
  *   of this gid within this realm can edit this node.
  * - 'grant_delete': If set to 1 a user that has been identified as a member
@@ -187,6 +198,35 @@ function hook_node_grants($account, $op) {
  *   priority will not be written. If there is any doubt, it is best to
  *   leave this 0.
  *
+ *
+ * When an implementation is interested in a node but want to deny access to
+ * everyone, it may return a "deny all" grant:
+ *
+ * @code
+ * $grants[] = array(
+ *   'realm' => 'all',
+ *   'gid' => 0,
+ *   'grant_view' => 0,
+ *   'grant_update' => 0,
+ *   'grant_delete' => 0,
+ *   'priority' => 1,
+ * );
+ * @endcode
+ *
+ * Setting the priority should cancel out other grants. In the case of a
+ * conflict between modules, it is safer to use hook_node_access_records_alter()
+ * to return only the deny grant.
+ *
+ * Note: a deny all grant is not written to the database; denies are implicit.
+ *
+ * @see node_access_write_grants()
+ *
+ * @param $node
+ *   The node that has just been saved.
+ *
+ * @return
+ *   An array of grants as defined above.
+ *
  * @ingroup node_access
  */
 function hook_node_access_records($node) {
@@ -194,17 +234,22 @@ function hook_node_access_records($node) {
   // treated just like any other node and we completely ignore it.
   if ($node->private) {
     $grants = array();
-    $grants[] = array(
-      'realm' => 'example',
-      'gid' => 1,
-      'grant_view' => 1,
-      'grant_update' => 0,
-      'grant_delete' => 0,
-      'priority' => 0,
-    );
-
+    // Only published nodes should be viewable to all users. If we allow access
+    // blindly here, then all users could view an unpublished node.
+    if ($node->status) {
+      $grants[] = array(
+        'realm' => 'example',
+        'gid' => 1,
+        'grant_view' => 1,
+        'grant_update' => 0,
+        'grant_delete' => 0,
+        'priority' => 0,
+      );
+    }
     // For the example_author array, the GID is equivalent to a UID, which
-    // means there are many many groups of just 1 user.
+    // means there are many groups of just 1 user.
+    // Note that an author can always view his or her nodes, even if they
+    // have status unpublished.
     $grants[] = array(
       'realm' => 'example_author',
       'gid' => $node->uid,
@@ -213,6 +258,7 @@ function hook_node_access_records($node) {
       'grant_delete' => 1,
       'priority' => 0,
     );
+
     return $grants;
   }
 }
@@ -234,6 +280,8 @@ function hook_node_access_records($node) {
  * user must have one or more matching permissions in order to complete the
  * requested operation.
  *
+ * A module may deny all access to a node by setting $grants to an empty array.
+ *
  * @see hook_node_grants()
  * @see hook_node_grants_alter()
  *
@@ -277,6 +325,8 @@ function hook_node_access_records_alter(&$grants, $node) {
  * The resulting grants are then checked against the records stored in the
  * {node_access} table to determine if the operation may be completed.
  *
+ * A module may deny all access to a user by setting $grants to an empty array.
+ *
  * @see hook_node_access_records()
  * @see hook_node_access_records_alter()
  *
diff --git a/modules/node/node.module b/modules/node/node.module
index b491e7469..a572d494b 100644
--- a/modules/node/node.module
+++ b/modules/node/node.module
@@ -3156,8 +3156,8 @@ function node_access_acquire_grants($node, $delete = TRUE) {
   $grants = module_invoke_all('node_access_records', $node);
   // Let modules alter the grants.
   drupal_alter('node_access_records', $grants, $node);
-  // If no grants are set, then use the default grant.
-  if (empty($grants)) {
+  // If no grants are set and the node is published, then use the default grant.
+  if (empty($grants) && !empty($node->status)) {
     $grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0);
   }
   else {
@@ -3206,7 +3206,7 @@ function node_access_write_grants($node, $grants, $realm = NULL, $delete = TRUE)
   }
 
   // Only perform work when node_access modules are active.
-  if (count(module_implements('node_grants'))) {
+  if (!empty($grants) && count(module_implements('node_grants'))) {
     $query = db_insert('node_access')->fields(array('nid', 'realm', 'gid', 'grant_view', 'grant_update', 'grant_delete'));
     foreach ($grants as $grant) {
       if ($realm && $realm != $grant['realm']) {
diff --git a/modules/node/node.test b/modules/node/node.test
index a31d1ae66..109b6735a 100644
--- a/modules/node/node.test
+++ b/modules/node/node.test
@@ -770,7 +770,7 @@ class NodeRSSContentTestCase extends DrupalWebTestCase {
 
 /**
  * Test case to verify basic node_access functionality.
- * @todo Cover hook_access in a separate test class.
+ * @todo Cover hook_node_access in a separate test class.
  * hook_node_access_records is covered in another test class.
  */
 class NodeAccessUnitTest extends DrupalWebTestCase {
@@ -821,6 +821,9 @@ class NodeAccessUnitTest extends DrupalWebTestCase {
     $node3 = $this->drupalCreateNode(array('status' => 0, 'uid' => $web_user3->uid));
     $this->assertNodeAccess(array('view' => FALSE), $node3, $web_user3);
 
+    // User cannot create content without permission.
+    $this->assertNodeAccess(array('create' => FALSE), 'page', $web_user3);
+
     // User can 'view own unpublished content', but another user cannot.
     $web_user4 = $this->drupalCreateUser(array('access content', 'view own unpublished content'));
     $web_user5 = $this->drupalCreateUser(array('access content', 'view own unpublished content'));
@@ -830,7 +833,6 @@ class NodeAccessUnitTest extends DrupalWebTestCase {
 
     // Tests the default access provided for a published node.
     $node5 = $this->drupalCreateNode();
-    $this->assertNodeAccess(array('create' => FALSE), 'page', $web_user3);
     $this->assertNodeAccess(array('view' => TRUE, 'update' => FALSE, 'delete' => FALSE), $node5, $web_user3);
   }
 }
@@ -909,6 +911,12 @@ class NodeAccessRecordsUnitTest extends DrupalWebTestCase {
       drupal_alter('node_grants', $altered_grants, $web_user, $op);
       $this->assertNotEqual($grants, $altered_grants, t('Altered the %op grant for a user.', array('%op' => $op)));
     }
+
+    // Check that core does not grant access to an unpublished node when an
+    // empty $grants array is returned.
+    $node6 = $this->drupalCreateNode(array('status' => 0, 'disable_node_access' => TRUE));
+    $records = db_query('SELECT realm, gid FROM {node_access} WHERE nid = :nid', array(':nid' => $node6->nid))->fetchAll();
+    $this->assertEqual(count($records), 0, t('Returned no records for unpublished node.'));
   }
 }
 
diff --git a/modules/node/tests/node_test.module b/modules/node/tests/node_test.module
index 3e8cdae04..cfc503da0 100644
--- a/modules/node/tests/node_test.module
+++ b/modules/node/tests/node_test.module
@@ -52,6 +52,10 @@ function node_test_node_grants($account, $op) {
  * Implements hook_node_access_records().
  */
 function node_test_node_access_records($node) {
+  // Return nothing when testing for empty responses.
+  if (!empty($node->disable_node_access)) {
+    return;
+  }
   $grants = array();
   if ($node->type == 'article') {
     // Create grant in arbitrary article_realm for article nodes.