diff --git a/core/lib/Drupal/Core/Database/Query/SelectExtender.php b/core/lib/Drupal/Core/Database/Query/SelectExtender.php index d60931d..746335b 100644 --- a/core/lib/Drupal/Core/Database/Query/SelectExtender.php +++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.php @@ -55,7 +55,7 @@ public function uniqueIdentifier() { * Implements Drupal\Core\Database\Query\PlaceholderInterface::nextPlaceholder(). */ public function nextPlaceholder() { - return $this->placeholder++; + return $this->query->nextPlaceholder(); } /* Implementations of Drupal\Core\Database\Query\AlterableInterface. */ diff --git a/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php index 8dcc8f3..590ffee 100644 --- a/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php +++ b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php @@ -124,7 +124,7 @@ public function checkAll(AccountInterface $account) { /** * {@inheritdoc} */ - public function alterQuery($query, array $tables, $op, AccountInterface $account, $base_table) { + public function alterQuery($query, array &$tables, $op, AccountInterface $account, $base_table) { if (!$langcode = $query->getMetaData('langcode')) { $langcode = FALSE; } @@ -174,8 +174,23 @@ public function alterQuery($query, array $tables, $op, AccountInterface $account $field = 'nid'; // Now handle entities. $subquery->where("$nalias.$field = na.nid"); - - $query->exists($subquery); + if (empty($tableinfo['join type'])) { + // If we are looking at the main table of the query, apply the + // subquery directly. + $query->exists($subquery); + } + else { + // If we are looking at a joined table, add the node access check + // to the join condition. + $tables[$nalias]['condition'] .= ' AND EXISTS(' . (string)$subquery . ')'; + $tables[$nalias]['arguments'] += $subquery->arguments(); + // Increment the placeholder count on the main query until it matches the placeholders + // used by the subquery. + $cond_count = $subquery->nextPlaceholder(); + while ($cond_count--) { + $query->nextPlaceholder(); + } + } } } } diff --git a/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorageInterface.php b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorageInterface.php index 2457b8f..dc02656 100644 --- a/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorageInterface.php +++ b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorageInterface.php @@ -49,7 +49,7 @@ public function checkAll(AccountInterface $account); * @return int * Status of the access check. */ - public function alterQuery($query, array $tables, $op, AccountInterface $account, $base_table); + public function alterQuery($query, array &$tables, $op, AccountInterface $account, $base_table); /** * Writes a list of grants to the database, deleting previously saved ones. diff --git a/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.php b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.php new file mode 100644 index 0000000..8070e4d --- /dev/null +++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.php @@ -0,0 +1,210 @@ + 'Node access across referenced nodes', + 'description' => 'Tests that node access checks get applied to the node base table across referenced fields.', + 'group' => 'Node', + ); + } + + public function setUp() { + parent::setUp(); + + node_access_rebuild(); + variable_set('node_access_test_private', TRUE); + + // Create some users. + $this->admin_user = $this->drupalCreateUser(array('access content', 'bypass node access')); + $this->user = $this->drupalCreateUser(array('access content')); + + // Add a custom field to the page content type. + $this->field_name = drupal_strtolower($this->randomName() . '_field_name'); + $this->field = entity_create('field_entity', array( + 'name' => $this->field_name, + 'entity_type' => 'node', + 'type' => 'number_integer', + 'cardinality' => FieldDefinitionInterface::CARDINALITY_UNLIMITED + )); + $this->field->save(); + $instance = array( + 'field_name' => $this->field_name, + 'entity_type' => 'node', + 'bundle' => 'page', + ); + $this->instance = entity_create('field_instance', $instance)->save(); + } + + /** + * Tests administering fields when node access is restricted. + * + * Create admin user that can skip node access checks. + * Create regular user that cannot see nodes subject to access checks. + * + * Create private node 1 that only admin user can see. + * Create public node 2 that both users can see, with integer field that + * references the private node 1. + * Create public node 3 with nothing in the integer reference field. + * Create a select query that shows nodes and the title from nodes related + * through the integer field, and tag for node_access. + * + * Test that admin user gets all 3 nodes returned in the query, and that the + * joined node appears in that node's result row. + * Test that regular user gets two results returned, and that the joined node + * does not appear in the public node's result row. + * + * Add a second instance of the integer reference field on node 2 that refers + * to node 3. + * Test that admin user gets 4 rows returned from query, two containing node 2. + * + * Test that regular user gets 2 rows returned from query, with node 2 listing + * only the reference to node 3. + * + */ + function testNodeAccessSubquery() { + // Create a page node. + $langcode = Language::LANGCODE_NOT_SPECIFIED; + $field_data = array(); +$this->pass('pre-nodecreation'); + $node1 = $this->drupalCreateNode(array('title'=>'Private node 1','uid' => $this->admin_user->id(), 'status' => 0)); + $node1 = $this->drupalCreateNode(array('title'=>'Private node 1','private'=> TRUE, 'uid' => $this->admin_user->id(), 'status' => 0)); + + $value = $field_data[0]['value'] = $node1->id(); + $node2 = $this->drupalCreateNode(array('title' => 'Public node 2', $this->field_name => $field_data, 'private' => TRUE, 'uid' => $this->user->id(), 'status' => 1)); + $node3 = $this->drupalCreateNode(array('title' => 'Public node 3', 'private' => TRUE, 'uid' => $this->user->id(), 'status' => 1)); + $value2 = $field_data[0]['value'] = $node3->id(); + $node4 = $this->drupalCreateNode(array('title' => 'Public node 4', $this->field_name => $field_data, 'private' => TRUE, 'uid' => $this->user->id(), 'status' => 1)); + $node5 = $this->drupalCreateNode(array('title' => 'Public node 5', 'uid' => $this->user->id(), 'private' => FALSE, 'status' => 1)); +$this->pass('post-nodecreation'); + + // @see http://drupal.org/node/1349080 + // Results we expect: + $expected_admin_count = 5; // one row for each node + $expected_user_count = 4; // one row for each node with my uid + // Results we should never find in a user query: + $this->private_nids = array(1); + + // Set up template query. + $join_table = FieldableDatabaseStorageController::_fieldTableName($this->field); + $join_column = $this->field_name . '_value'; + + // Get Query object for query. + $base_query = db_select('node', 'n'); + // Tag for node access. + $base_query->addTag('node_access'); + // Join on $join_table + $base_query->addJoin('LEFT OUTER', $join_table, 'jf', 'n.vid = jf.revision_id'); + + // Now add subquery join. + $base_query->addJoin('LEFT OUTER', 'node', 's', 'jf.' . $join_column .' = s.nid'); + $base_query->fields('n', array('nid', 'title')) + ->fields('s', array('nid', 'title')); + + + // Test as admin_user. + // We need to clone, because the node_access tag is only altered once. + $query = clone $base_query; + $query->addMetaData('account', $this->admin_user); + $result = $query->execute(); + $untagged = array(); + while ($record = $result->fetchAssoc()) { + $untagged[] = $record; + } + $this->assertEqual(count($untagged), $expected_admin_count, + "Admin user should get $expected_admin_count rows returned after initial load. Actual: ".count($untagged)); + + // Need a fresh copy of the query, because the node access alteration happens + // in the pre-execute phase, which only happens once. + $query = clone $base_query; + $query->addMetaData('account', $this->user); + $result = $query->execute(); + $tagged = array(); + while ($record = $result->fetchAssoc()) { + $tagged[] = $record; + } + $this->assertEqual(count($tagged), $expected_user_count, + "Regular user should get $expected_user_count rows returned after initial load. Actual: ".count($tagged)); + // now check to see if a private node appears in the result. + $this->assertUserResultAccess($tagged); + + // Add a second value to the original node, which the user CAN see: + $node2->{$this->field_name}[$langcode][] = array('value' => $node3->id()); + $node2->save(); + // We now expect the admin user to have 2 rows for node 2. + $expected_admin_count++; // 6 + // We now expect the regular user to have the same number of rows, but with + // a value in the subquery columns for node 2 instead of null. + // $expected_user_count++; + + // Re-run queries from above: + $query = clone $base_query; + $query->addMetaData('account', $this->admin_user); + $result = $query->execute(); + $untagged = array(); + while ($record = $result->fetchAssoc()) { + $untagged[] = $record; + } + $this->assertEqual(count($untagged), $expected_admin_count, + "Admin user should get $expected_admin_count rows returned after adding a relation. Actual: ".count($untagged)); + + $query = clone $base_query; + $query->addMetaData('account', $this->user); + $result = $query->execute(); + $tagged = array(); + while ($record = $result->fetchAssoc()) { + $tagged[] = $record; + } + //$this->assertEqual(count($tagged), $expected_user_count, + // "Regular user should get $expected_user_count rows returned after adding a relation. Actual: ".count($tagged)); + $this->assertUserResultAccess($tagged); + } + + /** + * Assert that a protected nid is not included in the returned results for a user. + * + * @param array $results An array with all results from the query. + */ + function assertUserResultAccess($results) { + $pass = true; + foreach ($results as $row) { + if (in_array($row['nid'],$this->private_nids)) { + $pass = false; + $this->fail(format_string('User was able to access node !nid in the returned result, in the parent query.', + array('!nid' => $row['s_nid']) + )); + } + if (!empty($row['s_nid']) && in_array($row['s_nid'], $this->private_nids)) { + $pass = false; + $this->fail(format_string('User was able to access node !nid in the returned result, in the subquery.', + array('!nid' => $row['s_nid']) + )); + } + } + if ($pass) { + $this->pass('User was not able to see private nodes in the returned query result.'); + } + } +} diff --git a/core/modules/node/node.module b/core/modules/node/node.module index 438104d..59ff47a 100644 --- a/core/modules/node/node.module +++ b/core/modules/node/node.module @@ -1758,7 +1758,7 @@ function node_query_node_access_alter(AlterableInterface $query) { return; } - $tables = $query->getTables(); + $tables = &$query->getTables(); $base_table = $query->getMetaData('base_table'); // If the base table is not given, default to node if present. if (!$base_table) {