#701744 by jhodgdon and Crell: Remove assumptions from node_query_node_access_alter() (with tests).

4 files changed, 288 insertions, 15 deletions

diff --git a/modules/node/node.module b/modules/node/node.module
index 7e1c4272f..7b04d24d1 100644
--- a/modules/node/node.module
+++ b/modules/node/node.module
@@ -2905,25 +2905,52 @@ function node_access_view_all_nodes() {
 
 /**
  * Implements hook_query_TAG_alter().
+ *
+ * This is the hook_query_alter() for queries tagged with 'node_access'.
+ * It adds node access checks for the user account given by the 'account'
+ * meta-data (or global $user if not provided), for an operation given by
+ * the 'op' meta-data (or 'view' if not provided; other possible values are
+ * 'update' and 'delete').
  */
 function node_query_node_access_alter(QueryAlterableInterface $query) {
-  // Skip the extra expensive alterations if site has no node access control
-  // modules.
-  if (!node_access_view_all_nodes()) {
-    // Prevent duplicate records.
-    $query->distinct();
-    // The recognized operations are 'view', 'update', 'delete'.
-    if (!$op = $query->getMetaData('op')) {
-      $op = 'view';
-    }
-    // Skip the extra joins and conditions for node admins.
-    if (!user_access('bypass node access')) {
+  global $user;
+
+  // Read meta-data from query, if provided.
+  if (!$account = $query->getMetaData('account')) {
+    $account = $user;
+  }
+  if (!$op = $query->getMetaData('op')) {
+    $op = 'view';
+  }
+  
+  // If $account can bypass node access, or there are no node access
+  // modules, we don't need to alter the query.
+  if (user_access('bypass node access', $account)) {
+    return;
+  }
+  if (!count(module_implements('node_grants'))) {
+    return;
+  }
+
+  // Prevent duplicate records.
+  $query->distinct();
+
+  // Find all instances of the {node} table being joined -- could appear
+  // more than once in the query, and could be aliased. Join each one to
+  // the node_access table.
+
+  $tables = $query->getTables();
+  $grants = node_access_grants($op, $account);
+  foreach ($tables as $nalias => $tableinfo) {
+    $table = $tableinfo['table'];
+    if (!($table instanceof SelectQueryInterface) && $table == 'node') {
+
       // The node_access table has the access grants for any given node.
-      $access_alias = $query->join('node_access', 'na', 'na.nid = n.nid');
+      $access_alias = $query->join('node_access', 'na', "na.nid = {$nalias}.nid");
       $or = db_or();
-      // If any grant exists for the specified user, then user has access to the
-      // node for the specified operation.
-      foreach (node_access_grants($op, $query->getMetaData('account')) as $realm => $gids) {
+      // If any grant exists for the specified user, then user has access
+      // to the node for the specified operation.
+      foreach ($grants as $realm => $gids) {
         foreach ($gids as $gid) {
           $or->condition(db_and()
             ->condition("{$access_alias}.gid", $gid)
diff --git a/modules/node/node.test b/modules/node/node.test
index 31404a6b8..82428f848 100644
--- a/modules/node/node.test
+++ b/modules/node/node.test
@@ -713,6 +713,12 @@ class NodeRSSContentTestCase extends DrupalWebTestCase {
   function setUp() {
     // Enable dummy module that implements hook_node_view.
     parent::setUp('node_test');
+
+    // Use bypass node access permission here, because the test class uses
+    // hook_grants_alter() to deny access to everyone on node_access
+    // queries.
+    $user = $this->drupalCreateUser(array('bypass node access', 'access content', 'create article content'));
+    $this->drupalLogin($user);
   }
 
   /**
@@ -747,6 +753,7 @@ class NodeRSSContentTestCase extends DrupalWebTestCase {
     // viewing node.
     $this->drupalGet("node/$node->nid");
     $this->assertNoText($rss_only_content, t('Node content designed for RSS doesn\'t appear when viewing node.'));
+
   }
 }
 
@@ -1438,3 +1445,132 @@ class NodeBuildContent extends DrupalWebTestCase {
     $this->assertFalse(isset($content['test_content_property']), t('Node content was emptied prior to being built.'));
   }
 }
+
+/**
+ * Tests node_query_node_access_alter().
+ */
+class NodeQueryAlter extends DrupalWebTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Node query alter',
+      'description' => 'Test that node access queries are properly altered by the node module.',
+      'group' => 'Node',
+    );
+  }
+
+  /**
+   * User with permission to view content.
+   */
+  protected $accessUser;
+
+  /**
+   * User without permission to view content.
+   */
+  protected $noAccessUser;
+
+  function setUp() {
+    parent::setUp('node_access_test');
+    node_access_rebuild();
+
+    // Create some content.
+    $this->drupalCreateNode();
+    $this->drupalCreateNode();
+    $this->drupalCreateNode();
+    $this->drupalCreateNode();
+
+    // Create user with simple node access permission.
+    $this->accessUser = $this->drupalCreateUser(array('access content', 'node test view'));
+    $this->noAccessUser = $this->drupalCreateUser(array('access content'));
+  }
+
+  /**
+   * Tests that node access permissions are followed.
+   */
+  function testNodeQueryAlterWithUI() {
+    // Verify that a user with access permission can see at least one node.
+
+    $this->drupalLogin($this->accessUser);
+    $this->drupalGet('node_access_test_page');
+    $this->assertText('Yes, 4 nodes', "4 nodes were found for access user");
+    $this->assertNoText('Exception', "No database exception");
+
+    // Verify that a user with no access permission cannot see nodes.
+
+    $this->drupalLogin($this->noAccessUser);
+    $this->drupalGet('node_access_test_page');
+    $this->assertText('No nodes', "No nodes were found for no access user");
+    $this->assertNoText('Exception', "No database exception");
+  }
+
+  /**
+   * Lower-level test of 'node_access' query alter, for user with access.
+   *
+   * Verifies that a non-standard table alias can be used, and that a
+   * user with node access can view the nodes.
+   */
+  function testNodeQueryAlterLowLevelWithAccess() {   
+    // User with access should be able to view 4 nodes.
+    try {
+      $query = db_select('node', 'mytab')
+        ->fields('mytab');
+      $query->addTag('node_access');
+      $query->addMetaData('op', 'view');
+      $query->addMetaData('account', $this->accessUser);
+
+      $result = $query->execute()->fetchAll();
+      $this->assertEqual(count($result), 4, t('User with access can see correct nodes'));
+    }
+    catch (Exception $e) {
+      $this->fail(t('Altered query is malformed'));
+    }
+  }
+
+  /**
+   * Lower-level test of 'node_access' query alter, for user without access.
+   *
+   * Verifies that a non-standard table alias can be used, and that a
+   * user without node access cannot view the nodes.
+   */
+  function testNodeQueryAlterLowLevelNoAccess() {
+    // User without access should be able to view 0 nodes.
+    try {
+      $query = db_select('node', 'mytab')
+        ->fields('mytab');
+      $query->addTag('node_access');
+      $query->addMetaData('op', 'view');
+      $query->addMetaData('account', $this->noAccessUser);
+
+      $result = $query->execute()->fetchAll();
+      $this->assertEqual(count($result), 0, t('User with no access cannot see nodes'));
+    }
+    catch (Exception $e) {
+      $this->fail(t('Altered query is malformed'));
+    }
+  }
+
+  /**
+   * Lower-level test of 'node_access' query alter, for edit access.
+   *
+   * Verifies that a non-standard table alias can be used, and that a
+   * user with view-only node access cannot edit the nodes.
+   */
+  function testNodeQueryAlterLowLevelEditAccess() {
+    // User with view-only access should not be able to edit nodes.
+    try {
+      $query = db_select('node', 'mytab')
+        ->fields('mytab');
+      $query->addTag('node_access');
+      $query->addMetaData('op', 'update');
+      $query->addMetaData('account', $this->accessUser);
+
+      $result = $query->execute()->fetchAll();
+      $this->assertEqual(count($result), 0, t('User with view-only access cannot edit nodes'));
+    }
+    catch (Exception $e) {
+      $this->fail($e->getMessage());
+      $this->fail((string)$query);
+      $this->fail(t('Altered query is malformed'));
+    }
+  }
+}
diff --git a/modules/node/tests/node_access_test.info b/modules/node/tests/node_access_test.info
new file mode 100644
index 000000000..e973c8fed
--- /dev/null
+++ b/modules/node/tests/node_access_test.info
@@ -0,0 +1,8 @@
+; $Id$
+name = "Node module access tests"
+description = "Support module for node permission testing."
+package = Testing
+version = VERSION
+core = 7.x
+files[] = node_access_test.module
+hidden = TRUE
diff --git a/modules/node/tests/node_access_test.module b/modules/node/tests/node_access_test.module
new file mode 100644
index 000000000..eaae6b80a
--- /dev/null
+++ b/modules/node/tests/node_access_test.module
@@ -0,0 +1,102 @@
+<?php
+// $Id$
+
+/**
+ * @file
+ * Dummy module implementing node access related hooks to test API interaction
+ * with the Node module. This module restricts view permission to those with
+ * a special 'node test view' permission.
+ */
+
+/**
+ * Implements hook_node_grants().
+ */
+function node_access_test_node_grants($account, $op) {
+  $grants = array();
+  if ($op == 'view' && user_access('node test view', $account)) {
+    $grants['node_access_test'] = array(888);
+  }
+  return $grants;
+}
+
+/**
+ * Implements hook_node_access_records().
+ */
+function node_access_test_node_access_records($node) {
+  $grants = array();
+  $grants[] = array(
+    'realm' => 'node_access_test',
+    'gid' => 888,
+    'grant_view' => 1,
+    'grant_update' => 0,
+    'grant_delete' => 0,
+    'priority' => 999,
+    );
+
+  return $grants;
+}
+
+/**
+ * Implements hook_permission().
+ *
+ * Sets up permissions for this module.
+ */
+function node_access_test_permission() {
+  return array('node test view' => array('title' => 'View content'));
+}
+
+/**
+ * Implements hook_menu().
+ *
+ * Sets up a page that lists nodes.
+ */
+function node_access_test_menu() {
+  $items = array();
+  $items['node_access_test_page'] = array(
+    'title' => 'Node access test',
+    'page callback' => 'node_access_test_page',
+    'access arguments' => array('access content'),
+    'type' => MENU_SUGGESTED_ITEM,
+  );
+  return $items;
+}
+
+/**
+ * Page callback for node access test page.
+ *
+ * Page should say "No nodes" if there are no nodes, and "Yes, # nodes" (with
+ * the number filled in) if there were nodes the user could access. Also, the
+ * database query is shown, and a list of the node IDs, for debugging purposes.
+ * And if there is a query exception, the page says "Exception" and gives the
+ * error.
+ */
+function node_access_test_page() {
+  $output = '';
+
+  try {
+    $query = db_select('node', 'mytab')
+      ->fields('mytab');
+    $query->addTag('node_access');
+    $result = $query->execute()->fetchAll();
+
+    if (count($result)) {
+      $output .= '<p>Yes, ' . count($result) . ' nodes</p>';
+      $output .= '<ul>';
+      foreach ($result as $item) {
+        $output .= '<li>' . $item->nid . '</li>';
+      }
+      $output .= '</ul>';
+    }
+    else {
+      $output .= '<p>No nodes</p>';
+    }
+
+    $output .= '<p>' . ((string) $query ) . '</p>';
+  }
+  catch (Exception $e) {
+    $output = '<p>Exception</p>';
+    $output .= '<p>' . $e->getMessage() . '</p>';
+  }
+
+  return $output;
+}