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 535d1bc282)

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();
     }

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') {
+      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;
+        }
       }
     }
   }

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']

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:
+menu_test.parent_test.child4_test:
-  title: 'Menu super child1'
+  title: 'Menu child4'
-  description: 'Menu item description super child1'
+  description: 'Menu item description child4'
-  route_name: menu_test.super_child1_test
+  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:
+menu_test.parent_test.child_test.grand_child2_test:
-  title: 'Menu super child2'
+  title: 'Menu grand child2'
-  description: 'Menu item description super child2'
+  description: 'Menu item description grand child2'
-  route_name: menu_test.super_child2_test
+  route_name: menu_test.grand_child2_test
   parent: menu_test.parent_test.child2_test
-menu_test.parent_test.child_test.super_child3_test:
+menu_test.parent_test.child_test.grand_child3_test:
-  title: 'Menu super child3'
+  title: 'Menu grand child3'
-  description: 'Menu item description super child3'
+  description: 'Menu item description grand child3'
-  route_name: menu_test.super_child3_test
+  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'

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:
+access grand child1 test page:
-  title: 'Access super child1 test page'
+  title: 'Access grand child1 test page'
   restrict access: true
-access super child2 test page:
+access grand child2 test page:
-  title: 'Access super child2 test page'
+  title: 'Access grand child2 test page'
   restrict access: true
-access super child3 test page:
+access grand child3 test page:
-  title: 'Access super 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

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:
+menu_test.child4_test_overview:
-  path: '/parent_test/child_test/super_child1_test'
+  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'
+    _access: 'TRUE'
-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'
 
-menu_test.super_child3_test:
+menu_test.grand_child1_test:
-  path: '/parent_test/child_test/super_child3_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}'

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');
+    $this->assertMenuItemRoutesAccess(403, 'admin/structure', 'admin/people', 'admin/config');
-    // As menu_test adds a menu link under config.
-    $this->assertMenuItemRoutesAccess(200, '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.grand_child2_test
-    // --menu_test.super_child3_test
+    // --menu_test.grand_child3_test
-    // -menu_test.child3_test
+    // -menu_test.child3_test_block
-    // All routes in this tree except the "super_child" routes should have the
+    // -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
+    // 'menu_test.child3_test_block' and 'menu_test.child4_test_overview' have
-    // have access to this route even though it has the requirement
+    // 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.child3_test_block',
-      'menu_test.super_child1_test',
+      'menu_test.child4_test_overview',
-      'menu_test.super_child2_test',
+      'menu_test.grand_child1_test',
-      'menu_test.super_child3_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 grand child1 test page',
-      'access super child2 test page',
+      'access grand child2 test page',
-      'access super child3 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($parentUser, [], $tree_routes);
-    $this->assertUserRoutesAccess($childOnlyUser, [], ...$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
+    // and 'menu_test.child3_test_block', because it has no items underneath in
-    // menu.
+    // the menu.
     $this->assertUserRoutesAccess(
       $noParentAccessUser,
-      array_diff($tree_routes, ['menu_test.parent_test', 'menu_test.child3_test']),
+      array_diff($tree_routes, [
-      ...$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,
+      $grandChild2User,
-      ['menu_test.parent_test', 'menu_test.child1_test', 'menu_test.super_child1_test'],
+      ['menu_test.parent_test', 'menu_test.child2_test', 'menu_test.grand_child2_test'],
-      ...$tree_routes);
+      $tree_routes);
     $this->assertUserRoutesAccess(
-      $superChild2User,
+      $grandChild3User,
-      ['menu_test.parent_test', 'menu_test.child2_test', 'menu_test.super_child2_test'],
+      // The 'menu_test.grand_child3_test' menu item is nested under
-      ...$tree_routes);
-    $this->assertUserRoutesAccess(
-      $superChild3User,
-      // The 'menu_test.super_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'],
+      ['menu_test.parent_test', 'menu_test.child2_test', 'menu_test.grand_child3_test'],
-      ...$tree_routes);
+      $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,9 +313,22 @@ 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);
+      if (!is_string($path)) {
-      $this->assertSession()->pageTextNotContains('You do not have any administrative items.');
+        $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;
   }
 
   /**
@@ -295,19 +336,37 @@ class MenuAccessTest extends BrowserTestBase {
    *
    * @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(
+      $this->drupalGet(Url::fromRoute($route));
-        in_array($route, $accessibleRoutes, TRUE) ? 200 : 403,
+      switch ($this->getSession()->getStatusCode()) {
-        Url::fromRoute($route)
+        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");
+  }
 
 }