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

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

Usually it's faster/easier to squash commits and create new MR or patch

OK, I read once again the CR and it seems pretty clear for me. Is just saying that a menu administrator can manage 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.

The last submitted patch, 106: drupal-n2463753-106.patch, failed testing. View results →

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!

acbramley → changed the visibility of the branch 2463753-menu to hidden.

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 🐛 "Login" link shows up for logged-in users as well Needs work .

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!

quietone → closed merge request !3118

Automatically closed - issue fixed for 2 weeks with no activity.