From 02ca3176ead562c9e6175c4e7789b3a34296524b Mon Sep 17 00:00:00 2001 From: catch Date: Mon, 26 Feb 2024 12:46:31 +0000 Subject: [PATCH] Issue #3381929 by tedbow, yash.rode, kunal.sachdev, tim.plunkett, quietone, hooroomoo, Utkarsh_33: Restrict access to empty top level administration pages for overview controller (cherry picked from commit 535d1bc282bba87717122305b94373283f39c4b3) --- .../SystemAdminMenuBlockAccessCheck.php | 24 ++- .../AccessRouteAlterSubscriber.php | 12 +- core/modules/system/system.services.yml | 5 + .../menu_test/menu_test.links.menu.yml | 38 ++-- .../menu_test/menu_test.permissions.yml | 15 +- .../modules/menu_test/menu_test.routing.yml | 49 +++-- .../src/Functional/Menu/MenuAccessTest.php | 171 ++++++++++++------ 7 files changed, 218 insertions(+), 96 deletions(-) diff --git a/core/modules/system/src/Access/SystemAdminMenuBlockAccessCheck.php b/core/modules/system/src/Access/SystemAdminMenuBlockAccessCheck.php index a527605ea73..f9dda50ca62 100644 --- a/core/modules/system/src/Access/SystemAdminMenuBlockAccessCheck.php +++ b/core/modules/system/src/Access/SystemAdminMenuBlockAccessCheck.php @@ -55,8 +55,23 @@ class SystemAdminMenuBlockAccessCheck implements AccessInterface { */ public function access(RouteMatchInterface $route_match, AccountInterface $account): AccessResultInterface { $parameters = $route_match->getParameters()->all(); - // Load links in the 'admin' menu matching this route. + $route = $route_match->getRouteObject(); + // Load links in the 'admin' menu matching this route. First, try to find + // the menu link using all specified parameters. $links = $this->menuLinkManager->loadLinksByRoute($route_match->getRouteName(), $parameters, 'admin'); + // If the menu link was not found, try finding it without the parameters + // that match the route defaults. Depending on whether the parameter is + // specified in the menu item with a value matching the default, or not + // specified at all, will change how it is stored in the menu_tree table. In + // both cases the route match parameters will always include the default + // parameters. This fallback method of finding the menu item is needed so + // that menu items will work in either case. + // @todo Remove this fallback in https://drupal.org/i/3359511. + if (empty($links)) { + + $parameters_without_defaults = array_filter($parameters, fn ($key) => !$route->hasDefault($key) || $route->getDefault($key) !== $parameters[$key], ARRAY_FILTER_USE_KEY); + $links = $this->menuLinkManager->loadLinksByRoute($route_match->getRouteName(), $parameters_without_defaults, 'admin'); + } if (empty($links)) { // If we did not find a link then we have no opinion on access. return AccessResult::neutral(); @@ -65,7 +80,7 @@ class SystemAdminMenuBlockAccessCheck implements AccessInterface { } /** - * Check that the given route has access to one of it's child routes. + * Check that the given route has access to child routes. * * @param \Drupal\Core\Menu\MenuLinkInterface $link * The menu link. @@ -87,7 +102,10 @@ class SystemAdminMenuBlockAccessCheck implements AccessInterface { if (empty($tree)) { $route = $this->router->getRouteCollection()->get($link->getRouteName()); if ($route) { - return AccessResult::allowedIf(empty($route->getRequirement('_access_admin_menu_block_page'))); + return AccessResult::allowedIf( + empty($route->getRequirement('_access_admin_menu_block_page')) + && empty($route->getRequirement('_access_admin_overview_page')) + ); } return AccessResult::neutral(); } diff --git a/core/modules/system/src/EventSubscriber/AccessRouteAlterSubscriber.php b/core/modules/system/src/EventSubscriber/AccessRouteAlterSubscriber.php index 5bc438442bc..7de3bab0bb5 100644 --- a/core/modules/system/src/EventSubscriber/AccessRouteAlterSubscriber.php +++ b/core/modules/system/src/EventSubscriber/AccessRouteAlterSubscriber.php @@ -33,8 +33,16 @@ class AccessRouteAlterSubscriber implements EventSubscriberInterface { foreach ($routes as $route) { // Do not use a leading slash when comparing to the _controller string // because the leading slash in a fully-qualified method name is optional. - if ($route->hasDefault('_controller') && ltrim($route->getDefault('_controller'), '\\') === 'Drupal\system\Controller\SystemController::systemAdminMenuBlockPage') { - $route->setRequirement('_access_admin_menu_block_page', 'TRUE'); + if ($route->hasDefault('_controller')) { + switch (ltrim($route->getDefault('_controller'), '\\')) { + case 'Drupal\system\Controller\SystemController::systemAdminMenuBlockPage': + $route->setRequirement('_access_admin_menu_block_page', 'TRUE'); + break; + + case 'Drupal\system\Controller\SystemController::overview': + $route->setRequirement('_access_admin_overview_page', 'TRUE'); + break; + } } } } diff --git a/core/modules/system/system.services.yml b/core/modules/system/system.services.yml index e6380fbf138..b56f401f9f0 100644 --- a/core/modules/system/system.services.yml +++ b/core/modules/system/system.services.yml @@ -14,6 +14,11 @@ services: arguments: ['@access_manager', '@menu.link_tree', '@router', '@plugin.manager.menu.link'] tags: - { name: access_check, applies_to: _access_admin_menu_block_page } + access_check.admin_overview_page: + class: Drupal\system\Access\SystemAdminMenuBlockAccessCheck + arguments: [ '@access_manager', '@menu.link_tree', '@router', '@plugin.manager.menu.link' ] + tags: + - { name: access_check, applies_to: _access_admin_overview_page } system.manager: class: Drupal\system\SystemManager arguments: ['@module_handler', '@request_stack', '@menu.link_tree', '@menu.active_trail'] diff --git a/core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml b/core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml index d694e419b85..503cbb6622a 100644 --- a/core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml +++ b/core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml @@ -22,7 +22,7 @@ menu_test.parent_test: parent: system.admin menu_test.parent_test.child1_test: title: 'Menu child1' - description: 'Menu item description child1' + description: 'Menu child1: uses SystemController::overview' route_name: menu_test.child1_test parent: menu_test.parent_test menu_test.parent_test.child2_test: @@ -33,23 +33,33 @@ menu_test.parent_test.child2_test: menu_test.parent_test.child3_test: title: 'Menu child3' description: 'Menu item description child3' - route_name: menu_test.child3_test + route_name: menu_test.child3_test_block parent: menu_test.parent_test -menu_test.parent_test.child_test.super_child1_test: - title: 'Menu super child1' - description: 'Menu item description super child1' - route_name: menu_test.super_child1_test +menu_test.parent_test.child4_test: + title: 'Menu child4' + description: 'Menu item description child4' + route_name: menu_test.child4_test_overview + parent: menu_test.parent_test +menu_test.parent_test.child_test.grand_child1_test: + title: 'Menu grand child1' + description: 'Menu grand child1: direct parent uses SystemController::overview' + route_name: menu_test.grand_child1_test parent: menu_test.parent_test.child1_test -menu_test.parent_test.child_test.super_child2_test: - title: 'Menu super child2' - description: 'Menu item description super child2' - route_name: menu_test.super_child2_test +menu_test.parent_test.child_test.grand_child2_test: + title: 'Menu grand child2' + description: 'Menu item description grand child2' + route_name: menu_test.grand_child2_test parent: menu_test.parent_test.child2_test -menu_test.parent_test.child_test.super_child3_test: - title: 'Menu super child3' - description: 'Menu item description super child3' - route_name: menu_test.super_child3_test +menu_test.parent_test.child_test.grand_child3_test: + title: 'Menu grand child3' + description: 'Menu item description grand child3' + route_name: menu_test.grand_child3_test parent: menu_test.parent_test.child2_test +menu_test.parent_test.child_test.great_grand_child1_test: + title: 'Menu great grand child1' + description: 'Menu great grand child1: grand parent uses SystemController::overview' + route_name: menu_test.great_grand_child1_test + parent: menu_test.parent_test.child_test.grand_child1_test menu_test.menu_parent_test_param: title: 'Menu Parent Param' description: 'Menu item description parent' diff --git a/core/modules/system/tests/modules/menu_test/menu_test.permissions.yml b/core/modules/system/tests/modules/menu_test/menu_test.permissions.yml index 47c528f0125..a175145d3b7 100644 --- a/core/modules/system/tests/modules/menu_test/menu_test.permissions.yml +++ b/core/modules/system/tests/modules/menu_test/menu_test.permissions.yml @@ -7,12 +7,15 @@ access child1 test page: access child2 test page: title: 'Access child2 test page' restrict access: true -access super child1 test page: - title: 'Access super child1 test page' +access grand child1 test page: + title: 'Access grand child1 test page' restrict access: true -access super child2 test page: - title: 'Access super child2 test page' +access grand child2 test page: + title: 'Access grand child2 test page' restrict access: true -access super child3 test page: - title: 'Access super child3 test page' +access grand child3 test page: + title: 'Access grand child3 test page' + restrict access: true +access great grand child1 test page: + title: 'Access great grand child1 test page' restrict access: true diff --git a/core/modules/system/tests/modules/menu_test/menu_test.routing.yml b/core/modules/system/tests/modules/menu_test/menu_test.routing.yml index 5b83aeb2af6..d88f9f21774 100644 --- a/core/modules/system/tests/modules/menu_test/menu_test.routing.yml +++ b/core/modules/system/tests/modules/menu_test/menu_test.routing.yml @@ -48,7 +48,9 @@ menu_test.parent_test: menu_test.child1_test: path: '/parent_test/child1_test' defaults: - _controller: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage' + _controller: '\Drupal\system\Controller\SystemController::overview' + link_id: 'menu_test.parent_test.child1_test' + _title: 'Configuration' requirements: _permission: 'access child1 test page' @@ -59,32 +61,49 @@ menu_test.child2_test: requirements: _permission: 'access child2 test page' -menu_test.child3_test: +menu_test.child3_test_block: path: '/parent_test/child3_test' defaults: _controller: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage' requirements: _access: 'TRUE' -menu_test.super_child1_test: - path: '/parent_test/child_test/super_child1_test' +menu_test.child4_test_overview: + path: '/parent_test/child4_test' defaults: - _controller: '\Drupal\test_page_test\Controller\TestPageTestController::testPage' + _controller: '\Drupal\system\Controller\SystemController::overview' + link_id: 'menu_test.parent_test.child4_test' + _title: 'Configuration' requirements: - _permission: 'access super child1 test page' -menu_test.super_child2_test: - path: '/parent_test/child_test/super_child2_test' - defaults: - _controller: '\Drupal\test_page_test\Controller\TestPageTestController::testPage' - requirements: - _permission: 'access super child2 test page' + _access: 'TRUE' -menu_test.super_child3_test: - path: '/parent_test/child_test/super_child3_test' +menu_test.grand_child1_test: + path: '/parent_test/child_test/grand_child1_test' + defaults: + _controller: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage' + requirements: + _permission: 'access grand child1 test page' + +menu_test.great_grand_child1_test: + path: '/parent_test/child_test/great_grand_child1_test' defaults: _controller: '\Drupal\test_page_test\Controller\TestPageTestController::testPage' requirements: - _permission: 'access super child3 test page' + _permission: 'access great grand child1 test page' + +menu_test.grand_child2_test: + path: '/parent_test/child_test/grand_child2_test' + defaults: + _controller: '\Drupal\test_page_test\Controller\TestPageTestController::testPage' + requirements: + _permission: 'access grand child2 test page' + +menu_test.grand_child3_test: + path: '/parent_test/child_test/grand_child3_test' + defaults: + _controller: '\Drupal\test_page_test\Controller\TestPageTestController::testPage' + requirements: + _permission: 'access grand child3 test page' menu_test.parent_test_param: path: '/parent_test_param/{param}' diff --git a/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php b/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php index 307fbf36130..b1a83af3cb7 100644 --- a/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php +++ b/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php @@ -18,7 +18,7 @@ class MenuAccessTest extends BrowserTestBase { * * @var array */ - protected static $modules = ['block', 'filter', 'menu_test', 'toolbar']; + protected static $modules = ['block', 'filter', 'toolbar', 'menu_ui']; /** * {@inheritdoc} @@ -40,6 +40,7 @@ class MenuAccessTest extends BrowserTestBase { * @see \Drupal\menu_test\Access\AccessCheck::access() */ public function testMenuBlockLinksAccessCheck() { + $this->container->get('module_installer')->install(['menu_test']); $this->drupalPlaceBlock('system_menu_block:account'); // Test that there's link rendered on the route. $this->drupalGet('menu_test_access_check_session'); @@ -118,33 +119,40 @@ class MenuAccessTest extends BrowserTestBase { // This user doesn't have access to any of the child pages, so the parent // pages should not be accessible. $this->drupalLogin($webUser); - $this->assertMenuItemRoutesAccess(403, 'admin/structure', 'admin/people'); - // As menu_test adds a menu link under config. - $this->assertMenuItemRoutesAccess(200, 'admin/config'); + $this->assertMenuItemRoutesAccess(403, 'admin/structure', 'admin/people', 'admin/config'); + // The test cases below depend on routes, menu items and permissions added + // by the menu_test module. It is not enabled before this to ensure that any + // other configuration it provides that we don't need for these test cases + // does not affect the assertions above. + $this->container->get('module_installer')->install(['menu_test']); // Test access to routes in the admin menu. The routes are in a menu tree // of the hierarchy: // menu_test.parent_test // -menu_test.child1_test - // --menu_test.super_child1_test + // --menu_test.grand_child1_test // -menu_test.child2_test - // --menu_test.super_child2_test - // --menu_test.super_child3_test - // -menu_test.child3_test - // All routes in this tree except the "super_child" routes should have the + // --menu_test.grand_child2_test + // --menu_test.grand_child3_test + // -menu_test.child3_test_block + // -menu_test.child4_test_overview + // All routes in this tree except the "grand_child" routes should have the // '_access_admin_menu_block_page' requirement which denies access unless // the user has access to a menu item under that route. Route - // 'menu_test.child3_test' has no menu items underneath it so no user should - // have access to this route even though it has the requirement + // 'menu_test.child3_test_block' and 'menu_test.child4_test_overview' have + // no menu items underneath it so no user should have access to these routes + // even though they have the requirement: // `_access: 'TRUE'`. $tree_routes = [ 'menu_test.parent_test', 'menu_test.child1_test', 'menu_test.child2_test', - 'menu_test.child3_test', - 'menu_test.super_child1_test', - 'menu_test.super_child2_test', - 'menu_test.super_child3_test', + 'menu_test.child3_test_block', + 'menu_test.child4_test_overview', + 'menu_test.grand_child1_test', + 'menu_test.grand_child2_test', + 'menu_test.grand_child3_test', + 'menu_test.great_grand_child1_test', ]; // Create a user with access to only the top level parent. @@ -152,72 +160,92 @@ class MenuAccessTest extends BrowserTestBase { 'access parent test page', ]); // Create a user with access to the parent and child routes but none of the - // super child routes. + // grand child routes. $childOnlyUser = $this->drupalCreateUser([ 'access parent test page', 'access child1 test page', 'access child2 test page', ]); - // Create 3 users all with access the parent and child but only 1 super + // Create 3 users all with access the parent and child but only 1 grand // child route. - $superChild1User = $this->drupalCreateUser([ + $grandChild1User = $this->drupalCreateUser([ 'access parent test page', 'access child1 test page', 'access child2 test page', - 'access super child1 test page', + 'access grand child1 test page', ]); - $superChild2User = $this->drupalCreateUser([ + $grandChild2User = $this->drupalCreateUser([ 'access parent test page', 'access child1 test page', 'access child2 test page', - 'access super child2 test page', + 'access grand child2 test page', ]); - $superChild3User = $this->drupalCreateUser([ + $grandChild3User = $this->drupalCreateUser([ 'access parent test page', 'access child1 test page', 'access child2 test page', - 'access super child3 test page', + 'access grand child3 test page', + ]); + $greatGrandChild1User = $this->drupalCreateUser([ + 'access parent test page', + 'access child1 test page', + 'access grand child1 test page', + 'access great grand child1 test page', ]); $noParentAccessUser = $this->drupalCreateUser([ 'access child1 test page', 'access child2 test page', - 'access super child1 test page', - 'access super child2 test page', - 'access super child3 test page', + 'access grand child1 test page', + 'access grand child2 test page', + 'access grand child3 test page', + 'access great grand child1 test page', ]); - // Users that do not have access to any of the 'super_child' routes will + // Users that do not have access to any of the 'grand_child' routes will // not have access to any of the routes in the tree. - $this->assertUserRoutesAccess($parentUser, [], ...$tree_routes); - $this->assertUserRoutesAccess($childOnlyUser, [], ...$tree_routes); - + $this->assertUserRoutesAccess($parentUser, [], $tree_routes); + $this->assertUserRoutesAccess($childOnlyUser, [], $tree_routes); // A user that does not have access to the top level parent but has access // to all the other routes will have access to all routes except the parent - // and 'menu_test.child3_test', because it has no items underneath in the - // menu. + // and 'menu_test.child3_test_block', because it has no items underneath in + // the menu. $this->assertUserRoutesAccess( $noParentAccessUser, - array_diff($tree_routes, ['menu_test.parent_test', 'menu_test.child3_test']), - ...$tree_routes + array_diff($tree_routes, [ + 'menu_test.parent_test', + 'menu_test.child3_test_block', + 'menu_test.child4_test_overview', + ]), + $tree_routes ); - // Users who have only access to one super child route should have access + + // Route using overview should have access to the grand child to access the + // current route. + $this->assertUserRoutesAccess( + $grandChild1User, + [], + $tree_routes); + $this->assertUserRoutesAccess( + $greatGrandChild1User, [ + 'menu_test.parent_test', + 'menu_test.child1_test', + 'menu_test.grand_child1_test', + 'menu_test.great_grand_child1_test', + ], + $tree_routes); + // Users who have only access to one grand child route should have access // only to that route and its parents. $this->assertUserRoutesAccess( - $superChild1User, - ['menu_test.parent_test', 'menu_test.child1_test', 'menu_test.super_child1_test'], - ...$tree_routes); + $grandChild2User, + ['menu_test.parent_test', 'menu_test.child2_test', 'menu_test.grand_child2_test'], + $tree_routes); $this->assertUserRoutesAccess( - $superChild2User, - ['menu_test.parent_test', 'menu_test.child2_test', 'menu_test.super_child2_test'], - ...$tree_routes); - $this->assertUserRoutesAccess( - $superChild3User, - // The 'menu_test.super_child3_test' menu item is nested under + $grandChild3User, + // The 'menu_test.grand_child3_test' menu item is nested under // 'menu_test.child2_test' to ensure access is correct when there are // multiple items nested at the same level. - ['menu_test.parent_test', 'menu_test.child2_test', 'menu_test.super_child3_test'], - ...$tree_routes); - + ['menu_test.parent_test', 'menu_test.child2_test', 'menu_test.grand_child3_test'], + $tree_routes); // Test a route that has parameter defined in the menu item. $this->drupalLogin($parentUser); $this->assertMenuItemRoutesAccess(403, Url::fromRoute('menu_test.parent_test_param', ['param' => 'param-in-menu'])); @@ -285,29 +313,60 @@ class MenuAccessTest extends BrowserTestBase { private function assertMenuItemRoutesAccess(int $expected_status, string|Url ...$paths): void { foreach ($paths as $path) { $this->drupalGet($path); - $this->assertSession()->statusCodeEquals($expected_status); - $this->assertSession()->pageTextNotContains('You do not have any administrative items.'); + if (!is_string($path)) { + $path = $path->toString(); + } + // We don't use \Behat\Mink\WebAssert::statusCodeEquals() here because it + // would not allow us to know which path failed. + $this->assertSame($expected_status, $this->getSession()->getStatusCode(), "Route $path has expected status code"); } } + /** + * {@inheritdoc} + */ + protected function drupalGet($path, array $options = [], array $headers = []) { + $return = parent::drupalGet($path, $options, $headers); + $this->assertSession()->pageTextNotContains('You do not have any administrative items.'); + return $return; + } + /** * Asserts which routes a user has access to. * * @param \Drupal\Core\Session\AccountInterface $user * The user account for which to check access. - * @param array $accessibleRoutes + * @param array $expectedAccessibleRoutes * The routes the user should have access to. - * @param string ...$allRoutes + * @param array $allRoutes * The routes to check. */ - private function assertUserRoutesAccess(AccountInterface $user, array $accessibleRoutes, string ...$allRoutes): void { + private function assertUserRoutesAccess(AccountInterface $user, array $expectedAccessibleRoutes, array $allRoutes): void { $this->drupalLogin($user); + $expectedInaccessibleRoutes = array_diff($allRoutes, $expectedAccessibleRoutes); + $this->assertEmpty(array_diff($expectedAccessibleRoutes, $allRoutes)); + $actualAccessibleRoutes = []; + $actualInaccessibleRoutes = []; foreach ($allRoutes as $route) { - $this->assertMenuItemRoutesAccess( - in_array($route, $accessibleRoutes, TRUE) ? 200 : 403, - Url::fromRoute($route) - ); + $this->drupalGet(Url::fromRoute($route)); + switch ($this->getSession()->getStatusCode()) { + case 200: + $actualAccessibleRoutes[] = $route; + break; + + case 403: + $actualInaccessibleRoutes[] = $route; + break; + + default: + throw new \UnexpectedValueException("Unexpected status code {$this->getStatus()} for route $route"); + + } } + $debug = fn($accessibleRoutes, $inaccessibleRoutes) => "\nAccessible routes: " . implode(', ', $accessibleRoutes) . "\nInaccessible routes: " . implode(', ', $inaccessibleRoutes); + $expected = $debug($expectedAccessibleRoutes, $expectedInaccessibleRoutes); + $actual = $debug($actualAccessibleRoutes, $actualInaccessibleRoutes); + $this->assertSession()->assert($expected === $actual, "Routes do not match. \nExpected routes:$expected\nActual routes: $actual"); } }