[regression] Do not bypass route access with 'link to any page' permissions for menu links

I agree that there is no point in using link to any page for checking the menu tree entires themselves.

We had a regression, let's ensure that we have such a link which is hidden for the user 1.

If memory serves, that permission was added to allow site builders to build out their IA before a site is fully built. Eg, add links to /about, /products, /etc before the views/panels that provide those pages exist. The challenge is that you can't add a link to a page you can't view (as a security measure), but that in turn means that since you can't access a non-existent page you can't add that link. That regression was introduced in D6 but not fixed until D8.

The permission in question is primarily intended as a loophole for site builders to do the above. Would there be a better way of handling that? Removing that check would be a regression at this point (do we regress a regression?), so we'd need some other way to ensure that site builders have that ability.

If we don't know how to get the else, we also don't know how to get to the elseif before, don't we?

Working on tests for now.

The tests are now working pretty fine for the block page.

One open question is how we should handle the menu UI.
You want to show the user/login link there as well, but at the same time, given that 'administer menu' is not a restricted permission, we kinda have a problem here.
Maybe special casing user/login is the right thing to do.

#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees landed, this needs a reroll.

P.S.: Another benefit of this patch, is that it will remove the need to have the user.permissions cache context for every menu link. That makes zero sense for external links, but currently, due to the way the code is structured, that's kind of what needs to happen.

Patch is rerolled - got a conflict and reuploaded the patch after fix . find details below

First, rewinding head to replay your work on top of it...
Applying: Applying patch from issue 2463753 comment 9845375
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
CONFLICT (content): Merge conflict in core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
Auto-merging core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php
Auto-merging core/modules/menu_ui/src/Tests/MenuTest.php
Auto-merging core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
Failed to merge in the changes.
Patch failed at 0001 Applying patch from issue 2463753 comment 9845375

Patch is clean now ...

Thanks! But looks like we need another one :) You probably missed a small thing.

I uploaded an another patch , Hope i donot miss anything this time. Patch is correctly applied to the head though i have not done anything different this time as well just rerolled again.

If it still fails then i might need your help in understanding this where i am going wrong :) .

#16: the patch is now ~90 instead of ~30 KB. That seems wrong :) You had conflicts again, and did not resolve them this time.

In fact, #13 also contains unresolved conflicts. You may want to google "how to resolve git conflicts".

Thanks Wim Leers for helping me with this reroll - I have uploaded an another updated patch. Hope this time it will pass other wise i will try again :)

Great work! Now it's 16 KB, and only has 4 failures. That's more like it :)

1. index 0000000..c12f789
   --- /dev/null
   
   --- /dev/null
   +++ b/2463753-10.patch
   

   Your patch includes the entire contents of the previous patch :P :) You might want to fix that :)

2. +++ b/2463753-10.patch
   @@ -0,0 +1,200 @@
   ++++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   +@@ -197,21 +197,31 @@ public function testCheckAccess() {
   +    */
   +   public function testCheckAccessWithLinkToAnyPagePermission() {
   +     $this->mockTree();
   +-    $this->currentUser->expects($this->exactly(8))
   ++    $this->currentUser->expects($this->any())
   +       ->method('hasPermission')
   +       ->with('link to any page')
   +       ->willReturn(TRUE);
   ++    $this->accessManager->expects($this->any())
   ++      ->method('checkNamedRoute')
   ++      ->willReturnCallback(function ($name, $parameters) {
   ++        switch ($name) {
   ++          case 'example1':
   ++          case 'example2':
   ++            return TRUE;
   ++          default:
   ++            return FALSE;
   ++        }
   ++      });
   + ¶
   +     $this->mockTree();
   +     $this->defaultMenuTreeManipulators->checkAccess($this->originalTree);
   + ¶
   +     $this->assertTrue($this->originalTree[1]->access);
   +     $this->assertTrue($this->originalTree[2]->access);
   +-    $this->assertTrue($this->originalTree[2]->subtree[3]->access);
   +-    $this->assertTrue($this->originalTree[2]->subtree[3]->subtree[4]->access);
   +-    $this->assertTrue($this->originalTree[5]->subtree[7]->access);
   ++    $this->assertFalse(isset($this->originalTree[2]->subtree[3]));
   ++    $this->assertFalse($this->originalTree[5]->access);
   +     $this->assertTrue($this->originalTree[6]->access);
   +-    $this->assertTrue($this->originalTree[8]->access);
   ++    $this->assertFalse($this->originalTree[8]->access);
   +   }
   + ¶
   

   There's two levels of pluses there. That seems wrong. Can you open this file locally? You should see that there is one plus at the beginning of many of these lines.

   Those are likely the cause of the remaining failures.

Patch rerolled again

Looking in the testing log under file Drupal\menu_ui\Tests\MenuTest/MenuTest.php . the test is failing at line 989

$this->assertLink(t('Login')); I guess this has to be $this->assertNoLink(t('Login')); to make this test pass becuase it is testing against logged in user

Any thoughts?

Working on this

Patch reroll.

comment from #10 still applies:

One open question is how we should handle the menu UI.
You want to show the user/login link there as well, but at the same time, given that 'administer menu' is not a restricted permission, we kinda have a problem here.
Maybe special casing user/login is the right thing to do.

Indention fix

I'm going to fix the fatal error.

Didn't manage to locate the issue, but in the #29 patch, the \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::menuLinkCheckAccess() method it's returning return $access_result->cachePerPermissions(); giving this error:

PHP Fatal error: Call to a member function cachePerPermissions() on a non-object

I was able to identify that by testing the value returned by following line:

$access_result = $this->accessManager->checkNamedRoute($definition['route_name'], $definition['route_parameters'], $this->account, TRUE);

$this->assertLink(t('Login')); I guess this has to be $this->assertNoLink(t('Login')); to make this test pass becuase it is testing against logged in user

Yeah, the login link doesn't show up because the user is logged in.

#37 #24
The issue here is what is said in #10 and still needs to be discussed/addressed.

The administrator SHOULD see the login-link when editting the menu, as he should be able to manage it.

To be explicit (and to confirm my understanding), this same bug causes a major usability issue if a role is granted both "Link to any page" and "Use the administration pages and help"— users with that role will see links under, say Structure, which they do not have access to, and get access denied when clicking them.

Reroll #29 patch for 8.5.x-dev without tests.
I think it will be not actual soon as #540008: Add a container parameter that can remove the special behavior of UID#1 will be finished.

This is a straight reroll for 8.8.x to see what tests fail (if any). This is a huge UX improvement in removing menu items from the toolbar that shouldn't be there e.g Appearance, Extend, People, Reports. Along with Structure and Configuration for roles without "Use the administration pages and help"

Here are before and after screenshots for a user with the following permissions:

  - 'access content overview'
  - 'access toolbar'
  - 'administer nodes'
  - 'link to any page'
  - 'view the administration theme'

Before:

After:

This should fix the first failure.

Fixed the tests. And, given that is bug, it should land in 8.8.x.

I've added back the tests removed in #46. I also took a different approach to solve the issue that is mentioned in #5 and #10: Instead of trying to hack into DefaultMenuLinkTreeManipulators::checkAccess(), let's use a dedicated manipulator, only for Menu UI. Given that the user that manages the menu form is already granted with permissions to do that, they should be able to see the links but, if the route access is forbidden they will get 403 on trying to access them. This happens only in Menu UI.

I've also removed the user.permissions cache context from the access resulty checks, as is mentioned in #12.

We have tests now.

I've added some docs in MenuForm, explaining why we're using a dedicated menu tree access check manipulator and removed a cache context from the Menu UI manipulator.

Corrected the docs added in the previous patch.

I think the new approach makes a lot of sense, and addresses the issue raised in #2463753-5: [regression] Do not bypass route access with 'link to any page' permissions for menu links and #2463753-10: [regression] Do not bypass route access with 'link to any page' permissions for menu links in an elegant way. I am not sure though if this still counts as a bug fix since it alters the usage of the "link to any page" permission. This can be discussed, but I think this possibly needs a change record.

1. diff --git a/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   index 108236e774..3b3bb22747 100644
   --- a/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -185,7 +185,7 @@ public function testCheckAccess() {
        // 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];
   @@ -196,18 +196,18 @@ public function testCheckAccess() {
        // 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->assertFalse(array_key_exists(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.
   

   This test still needs to have the documentation updated, it still refers to the original implementation that relied on the "link to any page" permission. The following documentation sections need work:

   1.     // 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.
      

   2.     // Note that the permissions cache context is added automatically, because
          // we always check the "link to any page" permission.
      

   3.     // Menu link 8: 'access' already set, note that 'per permissions' caching
          // is not added.
      

2. @@ -232,15 +232,17 @@ public function testCheckAccess() {
       */
      public function testCheckAccessWithLinkToAnyPagePermission() {
        $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());
   

   The name of this test is testCheckAccessWithLinkToAnyPagePermission() but we are no longer using the link to any page permission, so the name of this method should be updated.

3. @@ -1035,4 +1040,47 @@ public function testUserLoginUserLogoutLinks() {
   +    $this->drupalLogin($this->createUser([
   +      'administer menu',
   +      'link to any page',
   +    ]));
   +    $this->drupalGet('admin/structure/menu/manage/tools');
   +
   +    $assert->linkExists('Login');
   +    $assert->linkExists('Logout');
   

   This test seems to be a bit misleading in that it assigns the link to any page permission to the user. This gives the impression that this permission plays a role in the ability to access the login and logout links. This is not the case in the updated implementation. I verified this by removing the line that adds the permission; the test still passes without it. Let's remove this link to any page permission from the test to clear up any confusion.

4. --- /dev/null
   +++ b/core/modules/menu_ui/src/Menu/MenuUiMenuTreeManipulators.php
   @@ -0,0 +1,51 @@
   +/**
   + * Provides a menu tree manipulators to be used when managing menu links.
   + */
   

   Nitpick: remove "a ", since "tree manipulators" is plural.

5. --- a/core/modules/menu_ui/src/MenuForm.php
   +++ b/core/modules/menu_ui/src/MenuForm.php
   @@ -229,7 +229,15 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
   +      // Use a dedicated menu tree access check manipulator as users editing
   +      // this form, granted with 'administer menu' permission, should be able to
   +      // access even the menu links with inaccessible routes. Such cases are the
   +      // logout menu link, whose route is not accessible by a logged in user, or
   +      // a link to a page not yet created. The main menu tree manipulator only
   +      // allows the access to menu links with accessible routes.
   

   This is 100% clear, but it feels to me that this is a functional change. This issue is on the borderline between a bug fix and a feature request. Beforehand a user needed the "link to any page" permission to do this, now any user with "administer menu" can handle it.

   I don't think there are any security implications (apart from possibly an internal path disclosure), since the pages are still inaccessible, but this possibly needs a change record. TBD.

6. --- a/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   +    // 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);
   

   Very good to see this tested. This 100% guarantees that we will not accidentally introduce a vulnerability here in the future.

i am assigning this issue to me to work on the points mentioned in comment #60.

@swatichouhan012 your patch cannot be reviewed without an interdiff. See how to create it https://www.drupal.org/documentation/git/interdiff

@claudiu.cristea i am adding interdiff for patch. i added patch for point , 1, 2, 3, 4 regarding comment #60.

@swatichouhan, the interdiff should show the diffs between your patch (#64) and the previous (#59).

The interdiff is against #59. Fixed remarks from #60.

Thanks @swatichouhan and @claudiu.cristea for the quick response!

It looks good now, I just have one small nitpick left:

1. --- a/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -177,37 +177,32 @@ public function testCheckAccess() {
        // 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.
   +    // inaccessible link.
   

   I think we should keep the part of the sentence that reads , hence kept for its cacheability metadata. This is significant since this is the main reason why we retain links that are inaccessible for certain users. If the cacheability metadata of the inaccessible links were not included, then a user that _has_ the right permissions to access the link would possibly be served a cached version that _doesn't_ include the link.

Good point, thank you!

Great, thanks! This looks good to go.

This all looks very good and promising. I haven't tested it manually. I don't have time right now to fully comprehend the implications of the changes about cachePerPermissions(), but a quick skim makes initial sense.

Quick drive-by review of the patch. Only minor nits and cosmetic documentation changes, so leaving at RTBC. But if anyone wanted to re-roll to address these, huzzah. Might save an iteration from a core committer saying any of this. ;)

1. +++ b/core/modules/menu_ui/src/Menu/MenuUiMenuTreeManipulators.php
   @@ -0,0 +1,51 @@
   +   * Performs access checks of a menu tree when used in menu management form.
   

   Maybe "Grants access to a menu tree when used in the menu management form." ? There's no checking, it just blindly returns AccessResult::allowed() for everything. ;)

2. +++ b/core/modules/menu_ui/src/Menu/MenuUiMenuTreeManipulators.php
   @@ -0,0 +1,51 @@
   +   * be still able to create and manage menu links with Menu UI even the menu
   

   Super tiny nit: missing an if here: "even _if_ the menu"...

3. +++ b/core/modules/menu_ui/src/Menu/MenuUiMenuTreeManipulators.php
   @@ -0,0 +1,51 @@
   +   * NOTE: This menu tree manipulator is intended to be used only in the context
   ...
   +   * @internal
   

   Maybe put this NOTE text under @internal? I've seen core committers prefer a comment with @internal declarations, and this note seems to perfectly explain why no one else should use this class.

4. +++ b/core/modules/menu_ui/src/MenuForm.php
   @@ -230,7 +230,15 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
   +      // @see \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkAccess()
   +      // @see \Drupal\menu_ui\Menu\MenuUiMenuTreeManipulators::checkAccess()
   

   I love these, but I think I've seen that @see isn't allowed in inline comments. :/ Maybe move a bunch of this comment to the docblock for the whole function?

I am doing changes mentioned in above comment.

Thanks! Unfortunately now we have a failure because the Drupal coding standards do not allow to inline /* ... */ comments.

Re the comment #4 from above:

I love these, but I think I've seen that @see isn't allowed in inline comments. :/ Maybe move a bunch of this comment to the docblock for the whole function?

I verified this and it is nowhere mentioned in the API documentation and comment standards that it is not allowed. Even for some other tags (such as "@todo") its use is encouraged. I also found 800+ instances in core where "@see" mentions are used in inline comments so it is definitely a standard pattern.

It is true though that these are not parsed in the autogenerated PHPDoc documentation, but that's not important in this case I suppose.

Anyway, since it is not disallowed let's just put them back.

The "NoPreExistingSchemaTest" failure is also happening in the main dev branch so this can be ignored for now.

Thanks @pfrenssen

Just updated the patch.Please review.

Rerolling the patch.

Rerolled #76 as #79 did not reroll completely.

Interdiff against #76.

Tagging with D8 cacheability as per #12

The patch in #81 worked for me prior to updating to Drupal 9.1.4, but now I am unable to apply the patch.

Rerolled

Rerolled the patch against 9.2.x. Please review.

Rerolled the patch against 9.2.x. Please review.

Rerolled the patch against 9.2.x. Please review.

Let's simplify by moving in D10 so that we don't have to deal with so many patch variants.

What about Drupal 9.5.0? The most current patch is not applying.

Reroll of the patch in 88 for Drupal 9.5.

@gcb, D9.5 version is in the last MR https://git.drupalcode.org/project/drupal/-/merge_requests/3118

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Reviewing change record - explains the behavior before and how this broke things. The background was good feature explaining

Hiding patches as fixes are being addressed in MR 3118
Removing credit for myself for the rebase to see if tests pass.

The MR is targeted against 9.5.x but did verify it applies cleanly to 10.1.x

Tested this out on Drupal 10.1.x with a standard install
Created a test editor with "administer menu" permission but not "link to any"
As admin user created a menu link under tools menu
Logged into separate browser and verified I don't see that link
Applied fix
In browser with editor I can see the link now but when I click edit I getAccess denied.

Based on the change record I should be able to edit the menu links. If that's not the change then this needs a change record update to be more clear.

FWIW with this applied to core the test coverage added in #3271001 passes as expected, that the link to end the visitor's masquerading session only shows when it's supposed to, instead of all the time for user 1.

@smustgrave, I think the test is self-explanatory

    // 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);

Yes, you should see the link but you should not be able to access it

Then think the CR will need updating.

Also the MR needs to be updated for 11.x

This is the MR in patch format for 11.x; there were a few line offsets but one adjustment for fuzz:

$ curl https://git.drupalcode.org/project/drupal/-/merge_requests/2978.diff|patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 17095    0 17095    0     0  13238      0 --:--:--  0:00:01 --:--:-- 13293
patching file core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
Hunk #1 succeeded at 212 (offset 17 lines).
patching file core/modules/menu_ui/menu_ui.services.yml
patching file core/modules/menu_ui/src/Menu/MenuUiMenuTreeManipulators.php
patching file core/modules/menu_ui/src/MenuForm.php
patching file core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
Hunk #1 succeeded at 712 (offset 44 lines).
Hunk #2 succeeded at 1190 (offset 44 lines).
patching file core/modules/views_ui/tests/src/Functional/DisplayPathTest.php
Hunk #1 succeeded at 216 (offset 2 lines).
patching file core/tests/Drupal/KernelTests/Core/Menu/MenuLinkTreeTest.php
patching file core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
Hunk #1 succeeded at 205 (offset 10 lines).
Hunk #2 succeeded at 218 (offset 10 lines).
Hunk #3 succeeded at 240 (offset 10 lines).
Hunk #4 succeeded at 366 with fuzz 1 (offset 10 lines).

I'm working now to rebase MR on 11.x. Please don't open new patches or MRs

OK, I read once again the CR and it seems pretty clear for me. Is just saying that a menu administrator can manage a menu link even they cannot access the link's underlying route. However, I've added a new phrase that might clear more the explanation. I'm not a native English speaker so, maybe the CR is confusing.

Tweaked the CR very slightly, otherwise it's reading well!

I'm uploading a patch based on the latest version of the MR to ensure it can be applied safety via composer. Tested and working fine in Drupal 10.1.7. Thanks!

Trying to help get this across the line - the final fail is

Drupal\Tests\system\Functional\System\AdminTest::testAdminPages
Behat\Mink\Exception\ExpectationException: Link with label Inaccessible not found.

This is due to the Content link not being visible on /admin, debugging through it's because the admin/content route is getting _access_admin_menu_block_page = TRUE added, which then gets access denied in SystemAdminMenuBlockAccessCheck::hasAccessToChildMenuItems

I'm not sure if this is expected behaviour and therefore the testAdminPages needs to filter out inaccessible pages, or we need some other modification to get the menu item showing again.

Updated IS a bit.

Discussed with @larowlan, we agreed that the link should not be rendered (this is by design and is the very intention of this issue). Updated test to test for just that.

Ran the test-only feature to verify coverage

https://git.drupalcode.org/issue/drupal-2463753/-/jobs/521505 which correctly shows.

From what I can tell open threads are addressed/questions.

All green.

Only concern is if this could land in D10 since it's a permission change, but may just be me and won't let be a blocker.

Oh, another old issue, so nice to see these at RTBC.

I read the issue summary and comments here. I did not find any unanswered questions.

I did not #109 where @claudiu.cristea commented that the CR was confusing. I then read the CR and yes, it is confusing to me as a native English speaker. I am tagging for change record updates. I suggest rewording it to use plain English. There is plenty of information online to learn about it. If it were not so late right now I would have a go at improving it.

I left some suggestions on the MR as well. Those are also about the complexity of language in the comments.

I did not do a code review.

@quietone, thank you for looking at this.

I’ve applied the suggestions from MR. Regarding the CR, in #109 I didn’t say that the CR is confusing. I said that it might be confusing. For me is not confusing at all but I might be wrong as I’m not a native English speaker. Then, in #111, @acbramley said that he tweaked the CR and it’s ”reading well”. What can I do more, you’re both native English speakers?

I will tentatively set back to RTBC, leaving you (or the core committers) to weight on the CR. I would fix it myself but not clear for me what’s confusing there

I've simplified the title of the CR even more, I don't really see how much more rewording is possible unless someone wants to completely rewrite it. It is a somewhat confusing thing to explain though as access related issues are :)

Hello, the last modifications to the MR include a syntax error on a comment :S

In the core/modules/menu_ui/src/MenuForm.php file line 234

diff --git a/core/modules/menu_ui/src/MenuForm.php b/core/modules/menu_ui/src/MenuForm.php
index 9d90ac5a607d90f27b5776b722ed9f4feb60fefb..86fa319c74c4c19ececbf74aea8e84308faedd38 100644
--- a/core/modules/menu_ui/src/MenuForm.php
+++ b/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'],
     ];

I'll try to fix that now on the MR

Thank you, @anairamzap

Hi, I've been using this patch on a Drupal 9.5.11 version and I cannot apply it anymore.

This was failing spell check so I rebased.

Oh yes, big +1 to this. I think it should solve #2267603: "Login" link shows up for logged-in users as well.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks! This is a big enough change I don't think we should try to cherry-pick/backport to 10.2.x

Agreed on keeping this for 10.3

It's fantastic to see this finally committed, thank you all!