From c271adbb22f28a66ff4321b2fb7f86f20fe0c838 Mon Sep 17 00:00:00 2001 From: Alex Pott <alex.a.pott@googlemail.com> Date: Fri, 5 Jan 2024 00:16:52 +0000 Subject: [PATCH] Issue #1349080 by Nephele, freelock, Spokje, quietone, Lendude, agentrickard, tic2000, jofitz, RunePhilosof, manuelBS, AndreyMaximov, aleevas, dmsmidt, ChristianAdamski, ducktape, lokapujya, kalistos, ekes, maximpodorov, jordan.jamous, mgifford, fuerst, daffie, markhalliwell, emorency, thetwentyseven, damiankloip, mohit1604, rensingh99, gilsbert, xjm, andypost, tim.plunkett, Renee S, TwoD, rbruhn, mxr576, MustangGB, alexpott, dawehner, smustgrave, catch, lauriii, larowlan, merlinofchaos, Alan D., TBarina, jhedstrom, neclimdul, pookmish, danflanagan8, Les Lim, cherabomb, Christopher Riley, chx: node_access filters out accessible nodes when node is left joined --- .../node/src/NodeGrantDatabaseStorage.php | 17 +- .../views.view.test_node_access_join.yml | 337 +++++++++++++++++ .../src/Functional/NodeAccessJoinTest.php | 354 ++++++++++++++++++ .../node/tests/src/Kernel/NodeAccessTest.php | 31 ++ 4 files changed, 737 insertions(+), 2 deletions(-) create mode 100644 core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_access_join.yml create mode 100644 core/modules/node/tests/src/Functional/NodeAccessJoinTest.php diff --git a/core/modules/node/src/NodeGrantDatabaseStorage.php b/core/modules/node/src/NodeGrantDatabaseStorage.php index a8289edb7edf..a2e63b305bde 100644 --- a/core/modules/node/src/NodeGrantDatabaseStorage.php +++ b/core/modules/node/src/NodeGrantDatabaseStorage.php @@ -151,7 +151,7 @@ public function alterQuery($query, array $tables, $operation, AccountInterface $ $langcode = FALSE; } - // Find all instances of the base table being joined -- could appear + // Find all instances of the base table being joined which could appear // more than once in the query, and could be aliased. Join each one to // the node_access table. $grants = node_access_grants($operation, $account); @@ -191,7 +191,20 @@ public function alterQuery($query, array $tables, $operation, AccountInterface $ // Now handle entities. $subquery->where("[$table_alias].[$field] = [na].[nid]"); - $query->exists($subquery); + if (empty($tableinfo['join type'])) { + $query->exists($subquery); + } + else { + // If this is a join, add the node access check to the join condition. + // This requires using $query->getTables() to alter the table + // information. + $join_cond = $query + ->andConditionGroup() + ->exists($subquery); + $join_cond->where($tableinfo['condition'], $query->getTables()[$table_alias]['arguments']); + $query->getTables()[$table_alias]['arguments'] = []; + $query->getTables()[$table_alias]['condition'] = $join_cond; + } } } } diff --git a/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_access_join.yml b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_access_join.yml new file mode 100644 index 000000000000..766f656fc2bf --- /dev/null +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_access_join.yml @@ -0,0 +1,337 @@ +langcode: en +status: true +dependencies: + config: + - node.type.page + module: + - node + - user +id: test_node_access_join +label: 'Test Node Access Join' +module: views +description: '' +tag: '' +base_table: node_field_data +base_field: nid +display: + default: + display_plugin: default + id: default + display_title: Master + position: 0 + display_options: + access: + type: perm + options: + perm: 'access content' + cache: + type: tag + options: { } + query: + type: views_query + options: + disable_sql_rewrite: false + distinct: false + replica: false + query_comment: '' + query_tags: { } + exposed_form: + type: basic + options: + submit_button: Apply + reset_button: false + reset_button_label: Reset + exposed_sorts_label: 'Sort by' + expose_sort_order: true + sort_asc_label: Asc + sort_desc_label: Desc + pager: + type: full + options: + items_per_page: 1000 + offset: 0 + id: 0 + total_pages: null + expose: + items_per_page: false + items_per_page_label: 'Items per page' + items_per_page_options: '5, 10, 25, 50' + items_per_page_options_all: false + items_per_page_options_all_label: '- All -' + offset: false + offset_label: Offset + tags: + previous: '‹ Previous' + next: 'Next ›' + first: '« First' + last: 'Last »' + quantity: 9 + style: + type: table + row: + type: fields + fields: + title: + id: title + table: node_field_data + field: title + entity_type: node + entity_field: title + alter: + alter_text: false + make_link: false + absolute: false + trim: false + word_boundary: false + ellipsis: false + strip_tags: false + html: false + hide_empty: false + empty_zero: false + settings: + link_to_entity: true + plugin_id: field + relationship: none + group_type: group + admin_label: '' + label: Title + exclude: false + element_type: '' + element_class: '' + element_label_type: '' + element_label_class: '' + element_label_colon: true + element_wrapper_type: '' + element_wrapper_class: '' + element_default_classes: true + empty: '' + hide_alter_empty: true + click_sort_column: value + type: string + group_column: value + group_columns: { } + group_rows: true + delta_limit: 0 + delta_offset: 0 + delta_reversed: false + delta_first_last: false + multi_type: separator + separator: ', ' + field_api_classes: false + title_1: + id: title_1 + table: node_field_data + field: title + relationship: related_article + group_type: group + admin_label: '' + label: Article + exclude: false + alter: + alter_text: false + text: '' + make_link: false + path: '' + absolute: false + external: false + replace_spaces: false + path_case: none + trim_whitespace: false + alt: '' + rel: '' + link_class: '' + prefix: '' + suffix: '' + target: '' + nl2br: false + max_length: 0 + word_boundary: true + ellipsis: true + more_link: false + more_link_text: '' + more_link_path: '' + strip_tags: false + trim: false + preserve_tags: '' + html: false + element_type: '' + element_class: '' + element_label_type: '' + element_label_class: '' + element_label_colon: true + element_wrapper_type: '' + element_wrapper_class: '' + element_default_classes: true + empty: '' + hide_empty: false + empty_zero: false + hide_alter_empty: true + click_sort_column: value + type: string + settings: + link_to_entity: true + group_column: value + group_columns: { } + group_rows: true + delta_limit: 0 + delta_offset: 0 + delta_reversed: false + delta_first_last: false + multi_type: separator + separator: ', ' + field_api_classes: false + entity_type: node + entity_field: title + plugin_id: field + title_2: + id: title_2 + table: node_field_data + field: title + relationship: related_article_2 + group_type: group + admin_label: '' + label: 'Article 2' + exclude: false + alter: + alter_text: false + text: '' + make_link: false + path: '' + absolute: false + external: false + replace_spaces: false + path_case: none + trim_whitespace: false + alt: '' + rel: '' + link_class: '' + prefix: '' + suffix: '' + target: '' + nl2br: false + max_length: 0 + word_boundary: true + ellipsis: true + more_link: false + more_link_text: '' + more_link_path: '' + strip_tags: false + trim: false + preserve_tags: '' + html: false + element_type: '' + element_class: '' + element_label_type: '' + element_label_class: '' + element_label_colon: true + element_wrapper_type: '' + element_wrapper_class: '' + element_default_classes: true + empty: '' + hide_empty: false + empty_zero: false + hide_alter_empty: true + click_sort_column: value + type: string + settings: + link_to_entity: true + group_column: value + group_columns: { } + group_rows: true + delta_limit: 0 + delta_offset: 0 + delta_reversed: false + delta_first_last: false + multi_type: separator + separator: ', ' + field_api_classes: false + entity_type: node + entity_field: title + plugin_id: field + filters: + status: + value: '1' + table: node_field_data + field: status + plugin_id: boolean + entity_type: node + entity_field: status + id: status + expose: + operator: '' + group: 1 + type: + id: type + table: node_field_data + field: type + value: + page: page + entity_type: node + entity_field: type + plugin_id: bundle + sorts: + title: + id: title + table: node_field_data + field: title + relationship: none + group_type: group + admin_label: '' + order: ASC + exposed: false + expose: + label: '' + entity_type: node + entity_field: title + plugin_id: standard + title: 'Test Reference Access' + header: { } + footer: { } + empty: { } + relationships: + related_article: + id: related_article + table: node__related_article + field: related_article + relationship: none + group_type: group + admin_label: 'Page related article' + required: false + plugin_id: standard + related_article_2: + id: related_article_2 + table: node__related_article + field: related_article + relationship: related_article + group_type: group + admin_label: 'Article related article' + required: false + plugin_id: standard + arguments: { } + display_extenders: { } + cache_metadata: + max-age: 0 + contexts: + - 'languages:language_content' + - 'languages:language_interface' + - url.query_args + - 'user.node_grants:view' + - user.permissions + tags: { } + page_1: + display_plugin: page + id: page_1 + display_title: Page + position: 1 + display_options: + display_extenders: { } + path: test-node-access-join + cache_metadata: + max-age: 0 + contexts: + - 'languages:language_content' + - 'languages:language_interface' + - url.query_args + - 'user.node_grants:view' + - user.permissions + tags: { } diff --git a/core/modules/node/tests/src/Functional/NodeAccessJoinTest.php b/core/modules/node/tests/src/Functional/NodeAccessJoinTest.php new file mode 100644 index 000000000000..06ec97007bb0 --- /dev/null +++ b/core/modules/node/tests/src/Functional/NodeAccessJoinTest.php @@ -0,0 +1,354 @@ +<?php + +namespace Drupal\Tests\node\Functional; + +use Drupal\field\Entity\FieldConfig; +use Drupal\field\Entity\FieldStorageConfig; +use Drupal\node\Entity\NodeType; +use Drupal\user\UserInterface; +use Drupal\views\Tests\ViewTestData; + +/** + * Tests Node Access on join. + * + * @group views + */ +class NodeAccessJoinTest extends NodeTestBase { + + /** + * {@inheritdoc} + */ + protected static $modules = ['node_access_test', 'node_test_views', 'views']; + + /** + * {@inheritdoc} + */ + protected $defaultTheme = 'stark'; + + /** + * The user that will create the articles. + * + * @var \Drupal\user\UserInterface + */ + protected UserInterface $authorUser; + + /** + * Another user that will create articles. + * + * @var \Drupal\user\UserInterface + */ + protected UserInterface $otherUser; + + /** + * A user with just access content permissions. + * + * @var \Drupal\user\UserInterface + */ + protected UserInterface $regularUser; + + /** + * A user with access to private articles. + * + * @var \Drupal\user\UserInterface + */ + protected UserInterface $accessUser; + + /** + * Articles. + * + * @var array + */ + protected array $articles; + + /** + * Views used by this test. + * + * @var array + */ + public static array $testViews = ['test_node_access_join']; + + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + + node_access_test_add_field(NodeType::load('article')); + + $field_storage = FieldStorageConfig::create([ + 'field_name' => 'related_article', + 'entity_type' => 'node', + 'translatable' => FALSE, + 'entity_types' => [], + 'settings' => [ + 'target_type' => 'node', + ], + 'type' => 'entity_reference', + ]); + $field_storage->save(); + $field = FieldConfig::create([ + 'field_name' => 'related_article', + 'entity_type' => 'node', + 'bundle' => 'page', + 'label' => 'Related Article', + 'settings' => [ + 'handler' => 'default', + 'handler_settings' => [ + // Reference a single vocabulary. + 'target_bundles' => [ + 'article', + ], + ], + ], + ]); + $field->save(); + + $entity_display = \Drupal::service('entity_display.repository'); + $entity_display->getViewDisplay('node', 'page', 'default') + ->setComponent('related_article') + ->save(); + $entity_display->getFormDisplay('node', 'page', 'default') + ->setComponent('related_article', [ + 'type' => 'entity_reference_autocomplete', + ]) + ->save(); + + $field = FieldConfig::create([ + 'field_name' => 'related_article', + 'entity_type' => 'node', + 'bundle' => 'article', + 'label' => 'Related Article', + 'settings' => [ + 'handler' => 'default', + 'handler_settings' => [ + // Reference a single vocabulary. + 'target_bundles' => [ + 'article', + ], + ], + ], + ]); + $field->save(); + + $entity_display->getViewDisplay('node', 'article', 'default') + ->setComponent('related_article') + ->save(); + $entity_display->getFormDisplay('node', 'article', 'default') + ->setComponent('related_article', [ + 'type' => 'entity_reference_autocomplete', + ]) + ->save(); + + node_access_rebuild(); + \Drupal::state()->set('node_access_test.private', TRUE); + } + + /** + * Tests the accessibility of joined nodes. + * + * - Create two users with "access content" and "create article" permissions + * who can each access their own private articles but not others'. + * - Create article-type nodes with and without references to other articles. + * The articles and references represent all possible combinations of the + * tested access types. + * - Create page-type nodes referencing each of the articles, as well as a + * page with no reference. + * - Use a custom view that creates two joins between nodes and has a + * node_access tag. The view lists the page nodes, the article + * referenced by each page node, and the article referenced by each + * article. + * + * - Login with the author user and check that user does not have access to + * private nodes created by other users. Test access using total row + * count as well as checking for presence of individual page titles. + * - Repeat tests using a user with only the "access content" permission, + * confirming this user does not have access to any private nodes. + * - Repeat tests using a user with "access content" and "node test view" + * permissions, confirming this user sees the complete view. + */ + public function testNodeAccessJoin(): void { + + $permissions = ['access content', 'create article content']; + // User to add articles and test author access. + $this->authorUser = $this->drupalCreateUser($permissions); + // Another user to add articles whose private articles can not be accessed + // by authorUser. + $this->otherUser = $this->drupalCreateUser($permissions); + + // Create the articles. The articles are stored in an array keyed by + // $article and $reference2, where $article is the access type of the + // article itself, and $reference2 is the access type of the reference + // linked to by the article. 'public' articles are created by otherUser with + // private=0. 'private' articles are created by otherUser with private=1. + // 'author_public' articles are created by authorUser with private=0. + // 'author_private' articles are created by authorUser with private=1. + // 'no_reference' is used for references when there is no related article. + $access_type = ['public', 'private', 'author_public', 'author_private']; + $reference_access_type = array_merge(['no_reference'], $access_type); + + foreach ($reference_access_type as $reference2) { + foreach ($access_type as $article) { + $is_author = (str_starts_with($article, 'author')); + $is_private = (str_ends_with($article, 'private')); + $edit = [ + 'type' => 'article', + 'uid' => $is_author ? $this->authorUser->id() : $this->otherUser->id(), + ]; + $edit['private'][0]['value'] = $is_private; + // The article names provide the access status of the article and the + // access status of the related article, if any. The naming system + // ensures that the text 'Article $article' will only appear in the view + // if an article with that access type is displayed in the view. The + // text '$article' alone will appear in the titles of other nodes that + // reference an article. + $edit['title'] = "Article $article - $reference2"; + if ($reference2 !== 'no_reference') { + $edit['related_article'][0]['target_id'] = $this->articles[$reference2]['no_reference']; + } + $node = $this->drupalCreateNode($edit); + $this->articles[$article][$reference2] = $node->id(); + + $this->assertEquals((int) $is_private, (int) $node->private->value, 'The private status of the article node was properly set in the node_access_test table.' . $node->uid->target_id); + if ($reference2 !== 'no_reference') { + $this->assertEquals((int) $this->articles[$reference2]['no_reference'], (int) $node->related_article->target_id, 'Proper article attached to article.'); + } + } + } + + // Add a blank 'no_reference' entry to the article list, so that a page with + // no reference gets created. + $this->articles['no_reference']['no_reference'] = NULL; + + $total = 0; + $count_s_total = $count_s2_total = 0; + $count_s_public = $count_s2_public = 0; + $count_s_author = $count_s2_author = 0; + $total_public = $total_author = 0; + + // Create page nodes referencing each article, as a page without reference. + foreach ($this->articles as $reference => $list) { + foreach ($list as $reference2 => $article_nid) { + $title = "Page - $reference"; + if ($reference !== 'no_reference') { + $title .= " - $reference2"; + } + $edit = [ + 'type' => 'page', + 'title' => $title, + ]; + if ($article_nid) { + $edit['related_article'][0]['target_id'] = $article_nid; + } + $node = $this->drupalCreateNode($edit); + if ($article_nid) { + $this->assertEquals((int) $article_nid, (int) $node->related_article->target_id, 'Proper article attached to page.'); + } + + // Calculate totals expected for each user type. + $total++; + // Total number of primary and secondary references. + if ($reference !== 'no_reference') { + $count_s_total++; + if ($reference2 !== 'no_reference') { + $count_s2_total++; + } + } + // Public users only see 'public' and 'author_public' articles. + if (str_ends_with($reference, 'public')) { + $count_s_public++; + if (str_ends_with($reference2, 'public')) { + $count_s2_public++; + } + } + // authorUser sees 'public','author_public', 'author_private' articles. + if (str_ends_with($reference, 'public') || str_starts_with($reference, 'author')) { + $count_s_author++; + if (str_ends_with($reference2, 'public') || str_starts_with($reference2, 'author')) { + $count_s2_author++; + } + } + + // $total_public and $total_author are not currently in use -- but + // represent the totals when joins are handled by adding an is-null + // check (i.e., if inaccessible references caused the entire row to be + // hidden from view, instead of hiding just one cell of the table). + // Count of pages where all related articles are accessible by + // public users. + if (!str_ends_with($reference, 'private') && !str_ends_with($reference2, 'private')) { + $total_public++; + } + // Count of pages where all related articles are accessible by + // authorUser. + if ($reference !== 'private' && $reference2 !== 'private') { + $total_author++; + } + } + } + + // Generate a view listing all the pages, and check the view's content for + // users with three different access levels. + ViewTestData::createTestViews(get_class($this), ['node_test_views']); + + // Check the author of the 'author' articles. + $this->drupalLogin($this->authorUser); + $this->drupalGet('test-node-access-join'); + $chk_total = count($this->xpath("//td[@headers='view-title-table-column']")); + $this->assertEquals($chk_total, $total, 'Author should see ' . $total . ' rows. Actual: ' . $chk_total); + $chk_total = count($this->xpath("//td[@headers='view-title-1-table-column']/a")); + $this->assertEquals($chk_total, $count_s_author, 'Author should see ' . $count_s_author . ' primary references. Actual: ' . $chk_total); + $chk_total = count($this->xpath("//td[@headers='view-title-2-table-column']/a")); + $this->assertEquals($chk_total, $count_s2_author, 'Author should see ' . $count_s2_author . ' secondary references. Actual: ' . $chk_total); + + $session = $this->assertSession(); + $session->pageTextContains('Page - no_reference'); + $session->pageTextContains('Page - public - no_reference'); + $session->pageTextContains('Page - public - public'); + $session->pageTextContains('Page - author_private - no_reference'); + $session->pageTextContains('Article public'); + $session->pageTextNotContains('Article private'); + $session->pageTextContains('Article author_public'); + $session->pageTextContains('Article author_private'); + + // Check a regular user who did not author any articles. + $this->regularUser = $this->drupalCreateUser(['access content']); + $this->drupalLogin($this->regularUser); + $this->drupalGet('test-node-access-join'); + $chk_total = count($this->xpath("//td[@headers='view-title-table-column']")); + $this->assertEquals($chk_total, $total, 'Public user should see ' . $total . ' rows. Actual: ' . $chk_total); + $chk_total = count($this->xpath("//td[@headers='view-title-1-table-column']/a")); + $this->assertEquals($chk_total, $count_s_public, 'Public user should see ' . $count_s_public . ' primary references. Actual: ' . $chk_total); + $chk_total = count($this->xpath("//td[@headers='view-title-2-table-column']/a")); + $this->assertEquals($chk_total, $count_s2_public, 'Public user should see ' . $count_s2_public . ' secondary references. Actual: ' . $chk_total); + $session->pageTextContains('Page - no_reference'); + $session->pageTextContains('Page - public - no_reference'); + $session->pageTextContains('Page - public - public'); + $session->pageTextContains('Article public'); + $session->pageTextNotContains('Article private'); + $session->pageTextContains('Article author_public'); + $session->pageTextNotContains('Article author_private'); + + // Check that a user with 'node test view' permission, can view all pages + // and articles. + $this->accessUser = $this->drupalCreateUser([ + 'access content', + 'node test view', + ]); + $this->drupalLogin($this->accessUser); + $this->drupalGet('test-node-access-join'); + $chk_total = count($this->xpath("//td[@headers='view-title-table-column']")); + $this->assertEquals($chk_total, $total, 'Full-access user should see ' . $total . ' rows. Actual: ' . $chk_total); + $chk_total = count($this->xpath("//td[@headers='view-title-1-table-column']/a")); + $this->assertEquals($chk_total, $count_s_total, 'Full-access user should see ' . $count_s_total . ' primary references. Actual: ' . $chk_total); + $chk_total = count($this->xpath("//td[@headers='view-title-2-table-column']/a")); + $this->assertEquals($chk_total, $count_s2_total, 'Full-access user should see ' . $count_s2_total . ' secondary references. Actual: ' . $chk_total); + $session->pageTextContains('Page - no_reference'); + $session->pageTextContains('Page - public - no_reference'); + $session->pageTextContains('Page - public - public'); + $session->pageTextContains('Page - author_private - no_reference'); + $session->pageTextContains('Article public'); + $session->pageTextContains('Article private'); + $session->pageTextContains('Article author_public'); + $session->pageTextContains('Article author_private'); + } + +} diff --git a/core/modules/node/tests/src/Kernel/NodeAccessTest.php b/core/modules/node/tests/src/Kernel/NodeAccessTest.php index 8d6b178c7fc1..4426eb0b4c20 100644 --- a/core/modules/node/tests/src/Kernel/NodeAccessTest.php +++ b/core/modules/node/tests/src/Kernel/NodeAccessTest.php @@ -123,4 +123,35 @@ public function testUnsupportedOperation() { $this->assertNodeAccess(['random_operation' => FALSE], $node, $web_user); } + /** + * Tests node grants for queries with node access checks and base table join. + */ + public function testQueryWithBaseTableJoin(): void { + $this->enableModules(['node_access_test_empty']); + $this->drupalCreateNode(['type' => 'page']); + $this->drupalCreateNode(['type' => 'page']); + + $container = \Drupal::getContainer(); + $container->get('current_user')->setAccount($this->drupalCreateUser()); + + $query = \Drupal::database()->select('node_field_data', 'n'); + // Intentionally add a left join of the base table on the base table with a + // failing condition. This can, for example, happen in views with non + // required relations. + $query->leftJoin('node_field_data', 'nc', 'n.changed = nc.nid'); + $query->addTag('node_access'); + + $this->assertEquals(2, $query->countQuery()->execute()->fetchField()); + + $query = \Drupal::database()->select('node_field_data', 'n'); + // Use a Condition object to do the left join to test that this is handled + // correctly. + $join_cond = (\Drupal::database()->condition('AND'))->where('[n].[changed] = [n].[changed]'); + $join_cond->compile(\Drupal::database(), $query); + $query->leftJoin('node_field_data', 'nc', (string) $join_cond); + $query->addTag('node_access'); + + $this->assertEquals(4, $query->countQuery()->execute()->fetchField()); + } + } -- GitLab