Issue #1064954 by xjm, Dave Reid, becw: Fixed _node_revision_access() static usage.

2 files changed, 147 insertions, 14 deletions

diff --git a/modules/node/node.module b/modules/node/node.module
index fe8ee5169..461021bcd 100644
--- a/modules/node/node.module
+++ b/modules/node/node.module
@@ -1814,38 +1814,72 @@ function theme_node_search_admin($variables) {
   return $output;
 }
 
-function _node_revision_access($node, $op = 'view') {
+/**
+ * Access callback: Checks node revision access.
+ *
+ * @param $node
+ *   The node to check.
+ * @param $op
+ *   (optional) The specific operation being checked. Defaults to 'view.'
+ * @param object $account
+ *   (optional) A user object representing the user for whom the operation is
+ *   to be performed. Determines access for a user other than the current user.
+ *
+ * @return
+ *   TRUE if the operation may be performed, FALSE otherwise.
+ *
+ * @see node_menu()
+ */
+function _node_revision_access($node, $op = 'view', $account = NULL) {
   $access = &drupal_static(__FUNCTION__, array());
-  if (!isset($access[$node->vid])) {
-    // To save additional calls to the database, return early if the user
-    // doesn't have the required permissions.
-    $map = array('view' => 'view revisions', 'update' => 'revert revisions', 'delete' => 'delete revisions');
-    if (isset($map[$op]) && (!user_access($map[$op]) && !user_access('administer nodes'))) {
-      $access[$node->vid] = FALSE;
-      return FALSE;
+
+  $map = array(
+    'view' => 'view revisions',
+    'update' => 'revert revisions',
+    'delete' => 'delete revisions',
+  );
+
+  if (!$node || !isset($map[$op])) {
+    // If there was no node to check against, or the $op was not one of the
+    // supported ones, we return access denied.
+    return FALSE;
+  }
+
+  if (!isset($account)) {
+    $account = $GLOBALS['user'];
+  }
+
+  // Statically cache access by revision ID, user account ID, and operation.
+  $cid = $node->vid . ':' . $account->uid . ':' . $op;
+
+  if (!isset($access[$cid])) {
+    // Perform basic permission checks first.
+    if (!user_access($map[$op], $account) && !user_access('administer nodes', $account)) {
+      return $access[$cid] = FALSE;
     }
 
     $node_current_revision = node_load($node->nid);
     $is_current_revision = $node_current_revision->vid == $node->vid;
 
     // There should be at least two revisions. If the vid of the given node
-    // and the vid of the current revision differs, then we already have two
+    // and the vid of the current revision differ, then we already have two
     // different revisions so there is no need for a separate database check.
     // Also, if you try to revert to or delete the current revision, that's
     // not good.
     if ($is_current_revision && (db_query('SELECT COUNT(vid) FROM {node_revision} WHERE nid = :nid', array(':nid' => $node->nid))->fetchField() == 1 || $op == 'update' || $op == 'delete')) {
-      $access[$node->vid] = FALSE;
+      $access[$cid] = FALSE;
     }
-    elseif (user_access('administer nodes')) {
-      $access[$node->vid] = TRUE;
+    elseif (user_access('administer nodes', $account)) {
+      $access[$cid] = TRUE;
     }
     else {
       // First check the access to the current revision and finally, if the
       // node passed in is not the current revision then access to that, too.
-      $access[$node->vid] = node_access($op, $node_current_revision) && ($is_current_revision || node_access($op, $node));
+      $access[$cid] = node_access($op, $node_current_revision, $account) && ($is_current_revision || node_access($op, $node, $account));
     }
   }
-  return $access[$node->vid];
+
+  return $access[$cid];
 }
 
 function _node_add_access() {
diff --git a/modules/node/node.test b/modules/node/node.test
index 636b9be64..834960271 100644
--- a/modules/node/node.test
+++ b/modules/node/node.test
@@ -2306,3 +2306,102 @@ class NodeTokenReplaceTestCase extends DrupalWebTestCase {
     }
   }
 }
+
+/**
+ * Tests user permissions for node revisions.
+ */
+class NodeRevisionPermissionsTestCase extends DrupalWebTestCase {
+  protected $node_revisions = array();
+  protected $accounts = array();
+
+  // Map revision permission names to node revision access ops.
+  protected $map = array(
+    'view' => 'view revisions',
+    'update' => 'revert revisions',
+    'delete' => 'delete revisions',
+  );
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Node revision permissions',
+      'description' => 'Tests user permissions for node revision operations.',
+      'group' => 'Node',
+    );
+  }
+
+  function setUp() {
+    parent::setUp();
+
+    // Create a node with several revisions.
+    $node = $this->drupalCreateNode();
+    $this->node_revisions[] = $node;
+
+    for ($i = 0; $i < 3; $i++) {
+      // Create a revision for the same nid and settings with a random log.
+      $revision = clone $node;
+      $revision->revision = 1;
+      $revision->log = $this->randomName(32);
+      node_save($revision);
+      $this->node_revisions[] = $revision;
+    }
+
+    // Create three users, one with each revision permission.
+    foreach ($this->map as $op => $permission) {
+      // Create the user.
+      $account = $this->drupalCreateUser(
+        array(
+          'access content',
+          'edit any page content',
+          'delete any page content',
+          $permission,
+        )
+      );
+      $account->op = $op;
+      $this->accounts[] = $account;
+    }
+
+    // Create an admin account (returns TRUE for all revision permissions).
+    $admin_account = $this->drupalCreateUser(array('access content', 'administer nodes'));
+    $admin_account->is_admin = TRUE;
+    $this->accounts['admin'] = $admin_account;
+
+    // Create a normal account (returns FALSE for all revision permissions).
+    $normal_account = $this->drupalCreateUser();
+    $normal_account->op = FALSE;
+    $this->accounts[] = $normal_account;
+  }
+
+  /**
+   * Tests the _node_revision_access() function.
+   */
+  function testNodeRevisionAccess() {
+    $revision = $this->node_revisions[1];
+
+    $parameters = array(
+      'op' => array_keys($this->map),
+      'account' => $this->accounts,
+    );
+
+    $permutations = $this->generatePermutations($parameters);
+    foreach ($permutations as $case) {
+      if (!empty($case['account']->is_admin) || $case['op'] == $case['account']->op) {
+        $this->assertTrue(_node_revision_access($revision, $case['op'], $case['account']), "{$this->map[$case['op']]} granted.");
+      }
+      else {
+        $this->assertFalse(_node_revision_access($revision, $case['op'], $case['account']), "{$this->map[$case['op']]} not granted.");
+      }
+    }
+
+    // Test that access is FALSE for a node administrator with an invalid $node
+    // or $op parameters.
+    $admin_account = $this->accounts['admin'];
+    $this->assertFalse(_node_revision_access(FALSE, 'view', $admin_account), '_node_revision_access() returns FALSE with an invalid node.');
+    $this->assertFalse(_node_revision_access($revision, 'invalid-op', $admin_account), '_node_revision_access() returns FALSE with an invalid op.');
+
+    // Test that the $account parameter defaults to the "logged in" user.
+    $original_user = $GLOBALS['user'];
+    $GLOBALS['user'] = $admin_account;
+    $this->assertTrue(_node_revision_access($revision, 'view'), '_node_revision_access() returns TRUE when used with global user.');
+    $GLOBALS['user'] = $original_user;
+  }
+}