Problem/Motivation

Administrative task links provided by Admin Toolbar Extra Tools (admin_toolbar_tools) show up for users who do not have access to the toolbar at

/admin/index

Steps to reproduce

  • Visit /admin/index as a user with "Use the administration pages" permission and "Administer menus and menu links" permission but not "Use the toolbar" or "Use Admin Toolbar search" permissions.
  • See a bunch of "Add link", "Add link"… links among others under the "Admin Toolbar Extra Tools" heading, even though these are not useful here without context for any user and especially not for you without ever having the chance to see them in context in the admin toolbar.

Proposed resolution

Don't show anything from "Admin Toolbar Extra Tools" on the Administration Index page.

Remaining tasks

User interface changes

API changes

Data model changes

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mlncn created an issue. See original summary.

arunsahijpal’s picture

Assigned: Unassigned » arunsahijpal

Working on it.

arunsahijpal’s picture

Assigned: arunsahijpal » Unassigned
StatusFileSize
new207.44 KB

Hi @mlncn,
I've tried to reproduce this issue with these steps :

Enabled Admin toolbar 3.6.1
Loged in as a user with:
1. “Use the administration pages”
2. “Administer menus and menu links”
❌ “Use the toolbar”
❌ “Use Admin Toolbar search”
3. Visited /admin/index

But there was no option like "Add link" or "Admin Toolbar Extra Tools"
attaching ss for reference.
Please let me know if I am missing something here.

Thanks,
Arun

mlncn’s picture

Thanks Arun!

Did you enable Admin Toolbar Extra Tools? drush -y en admin_toolbar_tools

arunsahijpal’s picture

Status: Active » Needs review

Thanks @mlncn,
I forgot to enable it but when I enabled it the issue appeared and I've raised the MR for the solution please check.
Also the phpunit is failing, I think it can be fixed by updating the existing test cases.

divya.sejekan’s picture

StatusFileSize
new126.15 KB
new132.86 KB

Applied Patch MR!16. The issue looks resolved . Extra Add links are removed now
Enabled Admin toolbar 3.6.1
Loged in as a user with:
1. “Use the administration pages”
2. “Administer menus and menu links”
❌ “Use the toolbar”
❌ “Use Admin Toolbar search”

RTBC ++ , Keeping on hold due to MR merge issue and code review

dydave’s picture

Status: Needs review » Needs work

Thanks a lot everyone for raising this issue and for the initial work on the merge request, it's greatly appreciated.

I think a few things would need to be clarified here :

The access to the routes added by the admin_toolbar_tools module can be controlled with the permission 'administer site configuration', see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

However, there is currently nothing allowing to really control access to all the links added by the module in class ExtraLinks, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
For example the links Add link, as mentioned in the IS:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

The solution suggested in the MR is probably worth considering and testing a bit more carefully.

But I'm wondering whether we could just add a check for the permission at the top of the method adding the extra links and just exit if the access condition is not met.
It would make sense to prevent any access to admin_toolbar_tools extra links (in general), if the user does not have access to the toolbar and thus the admin_toolbar features, something like:
In https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

  public function getDerivativeDefinitions($base_plugin_definition) {
    $links = [];

    // Only users with 'use toolbar' permission will see links.
    if (!$this->currentUser->hasPermission('use toolbar')) {
      return $links;
    }

Would we be hiding too many links ? Could there be any links we would still want to display on the index page ?
Could this make sense or correspond to the problem described in the IS ?

I'm not completely clear right now on the links we would like to keep and the ones that should be hidden.
But if the changes suggested in this comment could help fixing this issue, then it would probably be preferable to go down this road.

Once the necessary changes will have been clarified, we should be able adjust the PHPUnit Tests accordingly, so the MR goes back to green.

I'm switching this back to Needs work as an attempt to get more information on the links that should be hidden and reviews or tests of the implementation suggested in this comment.

Any feedback, comments, suggestions or reviews would be greatly appreciated.
Thanks in advance!