Tools menu link displays for users who shouldn't see it

Ah, wait -- this probably doesn't work because it means the cron permission can't be used separately. So I guess the fix would be to add a new general permission?

Thank you for reporting this. I agree it is a problem.

I think the problem is that there can be a number of dynamically created items under the Tools menu, for example, if the Devel module is enabled.

So setting the permission for the Tools menu statically won't work. Currently the permissions to see it are somewhat lenient because it seems better for some users to see an empty Tools menu than for users to be denied access to items under it.

I think the solution is to implement a custom access check, by adding a class that implements Drupal\Core\Session\AccountInterface, and referencing it under requirements as _custom_access in the routing file.

It could check which children are enabled, and if none are, deny access to the Tools menu.

So I've added a custom access check /src/AccessChecks/ToolsAccess.php, which recursively checks access to all items in the Tools menu for the current user. If any item is visible, the Tools menu will be granted access.

This is working well for low privileged users like content creators, who have access to the navigation menu, but not to any admin functions. Previously, they were seeing an empty Tools menu. Now this is hidden.

There are a couple of cases that still need work.

First, if the user has access to some functions, e.g. Run Cron, but not any Clear Cache options, they will see the Tools menu, but also the empty "Clear individual cache". This because permissions are based on the route, and I don't currently have a unique route for the Cache Clear container item. I need to create a unique route for this menu option to fix this.

The second is that if the Devel module is enabled, and the user has access devel information permission, but not administer site configuration, the Tools menu shows, but the Devel menu does not. This is because the parent Devel menu requires administer site configuration permission, preventing it from displaying, but some of its only require access devel information, causing the Tools menu to show, while the Devel menu remains hidden.

I have tried implementing a Route Subscriber, /src/Routing/ToolsRouteSubscriber.php to alter the Devel parent menu, and make it use the scame custom check as the Tools menu. However, at present this is not working correctly, and the Devel menu is still not showing.

I have also added a test case to test that the Tools menu is visible for an admin user, but not shown for a user with only access to Navigation menu.

I will continue to work on the above problems, but if there isn't a resolution soon, I will merge to fix for most users, and fix outlying cases in a follow-on issue.

I have added handlers for the menus with children to the Controller, and set their permissions accordingly.

Have added test cases to verify correct permissions.

Needs review.

I see, wow, I was way off thinking thinking initially this was a simple fix! For our case, we were able to patch by just making the change I proposed because we won't have any users with access to only some of the tools.

I will try to find time to test this proper fix during the week.

Rebased after merging 🐛 JS error caused by toolbar library when toolbar is not present Active .

Thanks @pameeela, I wouldn't say you were way off, just that it's one of those issues that initially looks straightforward, but gets a lot more involved when you look into the details. I think this solution should set the module up for possible additions to the Tools menu in future.

If you have time to review, I'd be grateful. It would be nice to get wrapped up into a release.

Tested the patch and it works to hide the menu link. I'm not the best person to do a code review of this as it's outside of my expertise, but I am not sure about the empty pages. The top level page is reachable via the side navigation:

For now, the second level pages don't link but they may in future.

lostcarpark → credited ultimike → .

After reviewing with @ultimike, I have made some changes:

• I've made both the main tools menu and the Devel menu call the same access function, and use the route name to select which check gets used.
• I was fetching the whole Admin menu, and then selecting the Tools submenu from within it. I've figured out I can use setRoute in the MenuTreeParameters to select the specific submenu I want. I've also told it to only select one level of the menu.
• I was traversing the whole Tools menu tree to find all the items the user has access to. I've realised that once I find one item the user has access to, I know they can see the Tools menu, so I don't need to look any further, so I can return from checkTreeAccess as soon as an item is found.
• I was recursively checking the whole tree under Tools menu, but I've realised any submenus under Tools will have their own permission, so I only need to check for access to items on the first level below Tools. That allows me to drop the recursion, so makes the check simpler.

Would be very grateful if anyone has time to test.

Suggested test steps:

1. Install current stable version (1.1.x branch) of Navigation Extra Tools.
2. Check that admin user can see Tools menu on sidebar, with cache clearing options under it.
3. Create user with access to Navigation, but no admin functions, and verify that Tools menu displays, and empty "Flush individual cache" submenu shown, but no other options visible..
4. Install this issue fork, and either uninstall/reinstall, or clear caches.
5. Check that Tools no longer shows for user with only access to Navigation.
6. Grant user access to "Navigation Extra Tools Clear Cache", and verify Tools menu visible (and cache clearing options under it).
7. For bonus points, take away their cache clearing privilege, but then install the Devel module, and give user "Access Devel Information" privilege. Check they can see Tools again, and only option under it is "Development".

Obviously will give contribution credit for testers.

Another small update.

I figured out the controller core "parent" menus use, which displays the child items as a list.

Using the same controller for the "Tools", "Clear individual cache", and "Development" menus that if a user ends up on one of those pages, they will see available options in a list (or a message letting them know they don't have access to any menu items).

Although this should not happen in normal circumstances, it means that if the user does land on the Tools menu page, the menu will be displayed consistently with other menu pages.

This also means the individual controllers for those pages are no longer required.

One more small change. In the NavigationExtraToolsAccessTest test, I had hardcoded the strings the test asserts against, which are used multiple times. I have moved these into constants, which reduces duplication, and makes the code more readable.

Works like a charm. Thanks!

Went through with @ultimike, and he suggested removing <front> from the access check, as we're no longer using it as a route.

Thanks for the review, @ant1.

Going ahead with a merge.

Merged into 1.1 and 1.2 branches, moving to fixed.

Excellent! I didn't get around to testing it until today but working well, thanks :)

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