Issue #2463753 by claudiu.cristea, acbramley, gg24, anairamzap,...

Issue #2463753 by claudiu.cristea, acbramley, gg24, anairamzap, dhirendra.mishra, andypost, dawehner, swatichouhan012, smustgrave, quietone, Hardik_Patel_12, douggreen, mohrerao, gcb, kalistos, Wim Leers, pfrenssen, dww, larowlan: [regression] Do not bypass route access with 'link to any page' permissions for menu links

core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php

@@ -212,22 +212,14 @@ protected function collectNodeLinks(array &$tree, array &$node_links) {
    *   The access result.
    */
   protected function menuLinkCheckAccess(MenuLinkInterface $instance) {
-    $access_result = NULL;
-    if ($this->account->hasPermission('link to any page')) {
-      $access_result = AccessResult::allowed();
-    }
-    else {
-      $url = $instance->getUrlObject();
+    $url = $instance->getUrlObject();
 
-      // When no route name is specified, this must be an external link.
-      if (!$url->isRouted()) {
-        $access_result = AccessResult::allowed();
-      }
-      else {
-        $access_result = $this->accessManager->checkNamedRoute($url->getRouteName(), $url->getRouteParameters(), $this->account, TRUE);
-      }
+    if ($url->isRouted()) {
+      return $this->accessManager->checkNamedRoute($url->getRouteName(), $url->getRouteParameters(), $this->account, TRUE);
     }
-    return $access_result->cachePerPermissions();
+
+    // Must be an external link.
+    return AccessResult::allowed();
   }
 
   /**

core/modules/menu_ui/menu_ui.services.yml

+services:
+  menu_ui.menu_tree_manipulators:
+    class: Drupal\menu_ui\Menu\MenuUiMenuTreeManipulators
+  Drupal\menu_ui\Menu\MenuUiMenuTreeManipulators: '@menu_ui.menu_tree_manipulators'

core/modules/menu_ui/src/Menu/MenuUiMenuTreeManipulators.php

+<?php
+
+namespace Drupal\menu_ui\Menu;
+
+use Drupal\Core\Access\AccessResult;
+
+/**
+ * Provides menu tree manipulators to be used when managing menu links.
+ */
+class MenuUiMenuTreeManipulators {
+
+  /**
+   * Grants access to a menu tree when used in the menu management form.
+   *
+   * This manipulator allows access to menu links with inaccessible routes.
+   *
+   * Example use cases:
+   * - A login menu link, using the `user.login` route, is not accessible to a
+   *   logged-in user, but the site builder still needs to configure the menu
+   *   link.
+   * - A site builder wants to create a menu item for a Views page that has not
+   *   been created. In this case, there is no access to the route because it
+   *   does not exist.
+   *
+   * @param \Drupal\Core\Menu\MenuLinkTreeElement[] $tree
+   *   The menu link tree to manipulate.
+   *
+   * @return \Drupal\Core\Menu\MenuLinkTreeElement[]
+   *   The manipulated menu link tree.
+   *
+   * @internal
+   * This menu tree manipulator is intended for use only in the context of
+   * MenuForm because the user permissions to administer links is already
+   * checked. Don't use this manipulator in other places.
+   *
+   * @see \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkAccess()
+   * @see \Drupal\menu_ui\MenuForm
+   */
+  public function checkAccess(array $tree): array {
+    foreach ($tree as $element) {
+      $element->access = AccessResult::allowed();
+      if ($element->subtree) {
+        $element->subtree = $this->checkAccess($element->subtree);
+      }
+    }
+    return $tree;
+  }
+
+}

core/modules/menu_ui/src/MenuForm.php

@@ -231,7 +231,13 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
     // We indicate that a menu administrator is running the menu access check.
     $this->getRequest()->attributes->set('_menu_admin', TRUE);
     $manipulators = [
-      ['callable' => 'menu.default_tree_manipulators:checkAccess'],
+      // Use a dedicated menu tree access check manipulator as users editing
+      // this form, granted with 'administer menu' permission, should be able to
+      // access menu links with inaccessible routes. The default menu tree
+      // manipulator only allows the access to menu links with accessible routes.
+      // @see \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkAccess()
+      // @see \Drupal\menu_ui\Menu\MenuUiMenuTreeManipulators::checkAccess()
+      ['callable' => 'menu_ui.menu_tree_manipulators:checkAccess'],
       ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
     ];
     $tree = $this->menuTree->transform($tree, $manipulators);

core/modules/menu_ui/tests/src/Functional/MenuUiTest.php

@@ -727,10 +727,15 @@ public function testUnpublishedNodeMenuItem() {
     $this->drupalLogout();
     $this->drupalLogin($this->adminUser);
     $this->drupalGet('admin/structure/menu/manage/' . $item->getMenuName());
-    $this->assertSession()->pageTextNotContains($item->getTitle());
+    $this->assertSession()->linkExists($item->getTitle());
     // The cache contexts associated with the (in)accessible menu links are
     // bubbled. See DefaultMenuLinkTreeManipulators::menuLinkCheckAccess().
     $this->assertSession()->responseHeaderContains('X-Drupal-Cache-Contexts', 'user.permissions');
+
+    // The menu link admin is able to administer the link but cannot access the
+    // route as is not granted with 'bypass node access' permission.
+    $this->clickLink($item->getTitle());
+    $this->assertSession()->statusCodeEquals(403);
   }
 
   /**
@@ -1229,4 +1234,47 @@ public function testMenuUiWithPendingRevisions() {
     $this->assertSession()->elementNotExists('xpath', '//div[contains(@class, "messages--error")]');
   }
 
+  /**
+   * Tests the user login/logout links.
+   */
+  public function testUserLoginUserLogoutLinks() {
+    MenuLinkContent::create([
+      'menu' => 'tools',
+      'link' => [
+        'uri' => 'internal:/user/login',
+      ],
+      'title' => 'Login',
+    ])->save();
+    MenuLinkContent::create([
+      'menu' => 'tools',
+      'link' => [
+        'uri' => 'internal:/user/logout',
+      ],
+      'title' => 'Logout',
+    ])->save();
+
+    $assert = $this->assertSession();
+
+    $block = $this->drupalPlaceBlock('system_menu_block:tools');
+    $this->drupalGet('<front>');
+    $assert->linkExists('Login');
+    $assert->linkNotExists('Logout');
+
+    $this->drupalLogin($this->createUser());
+    $this->drupalGet('<front>');
+    $assert->linkNotExists('Login');
+    $assert->linkExists('Logout');
+
+    // Delete the block, we're now checking the Menu UI form.
+    $block->delete();
+
+    $this->drupalLogin($this->createUser(['administer menu']));
+    $this->drupalGet('admin/structure/menu/manage/tools');
+
+    $assert->linkExists('Logout');
+    // Check that the login link is accessible even the route is not.
+    $this->assertFalse(Url::fromRoute('user.login')->access($this->loggedInUser));
+    $assert->linkExists('Login');
+  }
+
 }

core/modules/system/tests/src/Functional/Menu/LinksetControllerTest.php

@@ -280,12 +280,10 @@ public function testUserAccountMenu() {
     $this->enableEndpoint(TRUE);
     $expected_cacheability = new CacheableMetadata();
     $expected_cacheability->addCacheContexts([
-      'user.permissions',
       'user.roles:authenticated',
     ]);
     $expected_cacheability->addCacheTags([
       'config:system.menu.account',
-      'config:user.role.anonymous',
       'http_response',
     ]);
     $response = $this->doRequest('GET', Url::fromUri('base:/system/menu/account/linkset'));

core/modules/system/tests/src/Functional/System/AdminTest.php

@@ -65,7 +65,14 @@ public function testAdminPages() {
 
     // Verify that all visible, top-level administration links are listed on
     // the main administration page.
-    foreach ($this->getTopLevelMenuLinks() as $item) {
+    foreach ($this->getTopLevelMenuLinks() as $element) {
+      $item = $element->link;
+      if (!$element->access->isAllowed()) {
+        // If the link is not accessible, it should not be rendered.
+        // @see \Drupal\Core\Menu\MenuLinkTree::buildItems().
+        $this->assertSession()->linkNotExists($item->getTitle());
+        continue;
+      }
       $this->assertSession()->linkExists($item->getTitle());
       $this->assertSession()->linkByHrefExists($item->getUrlObject()->toString());
       // The description should appear below the link.
@@ -124,7 +131,7 @@ public function testAdminPages() {
   /**
    * Returns all top level menu links.
    *
-   * @return \Drupal\Core\Menu\MenuLinkInterface[]
+   * @return \Drupal\Core\Menu\MenuLinkTreeElement[]
    */
   protected function getTopLevelMenuLinks() {
     $menu_tree = \Drupal::menuTree();
@@ -137,15 +144,7 @@ protected function getTopLevelMenuLinks() {
       ['callable' => 'menu.default_tree_manipulators:checkAccess'],
       ['callable' => 'menu.default_tree_manipulators:flatten'],
     ];
-    $tree = $menu_tree->transform($tree, $manipulators);
-
-    // Transform the tree to a list of menu links.
-    $menu_links = [];
-    foreach ($tree as $element) {
-      $menu_links[] = $element->link;
-    }
-
-    return $menu_links;
+    return $menu_tree->transform($tree, $manipulators);
   }
 
   /**

core/modules/views_ui/tests/src/Functional/DisplayPathTest.php

@@ -217,6 +217,7 @@ public function testDefaultMenuTabRegression() {
       'administer menu',
       'link to any page',
       'access toolbar',
+      'access administration pages',
     ]);
     $this->drupalLogin($admin_user);
 

core/tests/Drupal/KernelTests/Core/Menu/MenuLinkTreeTest.php

@@ -2,8 +2,11 @@
 
 namespace Drupal\KernelTests\Core\Menu;
 
+use Drupal\Core\Menu\InaccessibleMenuLink;
 use Drupal\Core\Menu\MenuLinkTreeElement;
 use Drupal\Core\Menu\MenuTreeParameters;
+use Drupal\Core\Session\AnonymousUserSession;
+use Drupal\Core\Session\UserSession;
 use Drupal\KernelTests\KernelTestBase;
 use Drupal\Tests\Core\Menu\MenuLinkMock;
 
@@ -133,4 +136,42 @@ public function testCreateLinksInMenu() {
     $this->assertEquals(3, $height);
   }
 
+  /**
+   * Tests user/login and user/logout links.
+   */
+  public function testUserLoginAndUserLogoutLinks() {
+    $account_switcher = $this->container->get('account_switcher');
+
+    $login_menu_link = MenuLinkMock::create(['id' => 'user_login_example', 'route_name' => 'user.login']);
+    $logout_menu_link = MenuLinkMock::create(['id' => 'user_logout_example', 'route_name' => 'user.logout']);
+
+    $this->menuLinkManager->addDefinition($login_menu_link->getPluginId(), $login_menu_link->getPluginDefinition());
+    $this->menuLinkManager->addDefinition($logout_menu_link->getPluginId(), $logout_menu_link->getPluginDefinition());
+
+    // Returns the accessible links from transformed 'mock' menu tree.
+    $get_accessible_links = function () {
+      $parameters = new MenuTreeParameters();
+      $manipulators = [
+        ['callable' => 'menu.default_tree_manipulators:checkAccess'],
+      ];
+
+      $tree = $this->linkTree->load('mock', $parameters);
+      $this->linkTree->transform($tree, $manipulators);
+
+      return array_keys(
+        array_filter($tree, function (MenuLinkTreeElement $element) {
+          return !$element->link instanceof InaccessibleMenuLink;
+        })
+      );
+    };
+
+    // Check that anonymous can only access the login link.
+    $account_switcher->switchTo(new AnonymousUserSession());
+    $this->assertSame(['user_login_example'], $get_accessible_links());
+
+    // Ensure that also user 1 does not see the login link.
+    $account_switcher->switchTo(new UserSession(['uid' => 1]));
+    $this->assertSame(['user_logout_example'], $get_accessible_links());
+  }
+
 }

core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php

@@ -207,15 +207,12 @@ public function testCheckAccess() {
     $this->cacheContextManager->assertValidTokens(['user'])->shouldBeCalled()->willReturn(TRUE);
     $this->originalTree[8]->access = AccessResult::allowed()->cachePerUser();
 
-    // Since \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkAccess()
-    // allows access to any link if the user has the 'link to any page'
-    // permission, *every* single access result is varied by permissions.
     $tree = $this->defaultMenuTreeManipulators->checkAccess($this->originalTree);
 
     // Menu link 1: route without parameters, access forbidden, but at level 0,
     // hence kept.
     $element = $tree[1];
-    $this->assertEquals(AccessResult::forbidden()->cachePerPermissions(), $element->access);
+    $this->assertEquals(AccessResult::forbidden(), $element->access);
     $this->assertInstanceOf('\Drupal\Core\Menu\InaccessibleMenuLink', $element->link);
     // Menu link 2: route with parameters, access granted.
     $element = $tree[2];
@@ -223,21 +220,19 @@ public function testCheckAccess() {
     $this->assertNotInstanceOf('\Drupal\Core\Menu\InaccessibleMenuLink', $element->link);
     // Menu link 3: route with parameters, AccessResult::neutral(), top-level
     // inaccessible link, hence kept for its cacheability metadata.
-    // Note that the permissions cache context is added automatically, because
-    // we always check the "link to any page" permission.
     $element = $tree[2]->subtree[3];
-    $this->assertEquals(AccessResult::neutral()->cachePerPermissions(), $element->access);
+    $this->assertEquals(AccessResult::neutral(), $element->access);
     $this->assertInstanceOf('\Drupal\Core\Menu\InaccessibleMenuLink', $element->link);
     // Menu link 4: child of menu link 3, which was AccessResult::neutral(),
     // hence menu link 3's subtree is removed, of which this menu link is one.
     $this->assertArrayNotHasKey(4, $tree[2]->subtree[3]->subtree);
     // Menu link 5: no route name, treated as external, hence access granted.
     $element = $tree[5];
-    $this->assertEquals(AccessResult::allowed()->cachePerPermissions(), $element->access);
+    $this->assertEquals(AccessResult::allowed(), $element->access);
     $this->assertNotInstanceOf('\Drupal\Core\Menu\InaccessibleMenuLink', $element->link);
     // Menu link 6: external URL, hence access granted.
     $element = $tree[6];
-    $this->assertEquals(AccessResult::allowed()->cachePerPermissions(), $element->access);
+    $this->assertEquals(AccessResult::allowed(), $element->access);
     $this->assertNotInstanceOf('\Drupal\Core\Menu\InaccessibleMenuLink', $element->link);
     // Menu link 7: 'access' already set: AccessResult::neutral(), top-level
     // inaccessible link, hence kept for its cacheability metadata.
@@ -247,31 +242,31 @@ public function testCheckAccess() {
     $element = $tree[5]->subtree[7];
     $this->assertEquals(AccessResult::neutral(), $element->access);
     $this->assertInstanceOf('\Drupal\Core\Menu\InaccessibleMenuLink', $element->link);
-    // Menu link 8: 'access' already set, note that 'per permissions' caching
-    // is not added.
+    // Menu link 8: 'access' already set.
     $element = $tree[8];
     $this->assertEquals(AccessResult::allowed()->cachePerUser(), $element->access);
     $this->assertNotInstanceOf('\Drupal\Core\Menu\InaccessibleMenuLink', $element->link);
   }
 
   /**
-   * Tests checkAccess() tree manipulator with 'link to any page' permission.
+   * Tests checkAccess() tree manipulator.
    *
    * @covers ::checkAccess
    * @covers ::menuLinkCheckAccess
    */
-  public function testCheckAccessWithLinkToAnyPagePermission() {
+  public function testCheckAccessTreeManipulator(): void {
     $this->mockTree();
-    $this->currentUser->expects($this->exactly(9))
-      ->method('hasPermission')
-      ->with('link to any page')
-      ->willReturn(TRUE);
+    // There are 9 checks but one is on an external link, so the route access
+    // checker should be called only 8 times.
+    // @see \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::menuLinkCheckAccess()
+    $this->accessManager->expects($this->exactly(8))
+      ->method('checkNamedRoute')
+      ->willReturn(AccessResult::allowed());
 
     $this->mockTree();
-    $this->cacheContextManager->assertValidTokens(['user.permissions'])->shouldBeCalled()->willReturn(TRUE);
     $this->defaultMenuTreeManipulators->checkAccess($this->originalTree);
 
-    $expected_access_result = AccessResult::allowed()->cachePerPermissions();
+    $expected_access_result = AccessResult::allowed();
     $this->assertEquals($expected_access_result, $this->originalTree[1]->access);
     $this->assertEquals($expected_access_result, $this->originalTree[2]->access);
     $this->assertEquals($expected_access_result, $this->originalTree[2]->subtree[3]->access);
@@ -366,8 +361,8 @@ public function testCheckNodeAccess() {
     $this->assertEquals($node_access_result, $tree[1]->access);
     $this->assertEquals($node_access_result, $tree[2]->access);
     $this->assertEquals(AccessResult::neutral(), $tree[2]->subtree[3]->access);
-    $this->assertEquals(AccessResult::allowed()->cachePerPermissions(), $tree[5]->access);
-    $this->assertEquals(AccessResult::neutral()->cachePerPermissions(), $tree[5]->subtree[6]->access);
+    $this->assertEquals(AccessResult::allowed(), $tree[5]->access);
+    $this->assertEquals(AccessResult::neutral(), $tree[5]->subtree[6]->access);
   }
 
   /**