Do not show Search if user does not have the correct permission

I tried the patch with an user with the 'administer site configuration' permission and is not working, are you sure this is the only permission needed?

I think that we face two different problems here:
- do we need a specific permission to use the search feature?
- to access the result page, we also need specific permission (for example the 'Administer users' to access the users overview page).

IMO we should not need to have the 'Administer site configuration' to access the search feature. In any case the result pages do have their specific permissions.

Suscribe to this issue, not user friendly at all. Will keep an eye on this progress.

Some feedback from my point of view:

1. The admin_toolbar.search route should not require the administer site configuration permission, as stated by @romainj. access toolbar should do just fine, until we make the search feature its own module (see #3090760: Move search feature to a submodule).
2. The patch is ignoring cacheability, consider adding $items['administration_search']['#cache']['contexts'][] = 'user.permissions';
3. The access check itself could also be done using a single line of code: $items['administration_search']['#access'] = \Drupal::currentUser()->hasPermission('access toolbar'); (or whichever permission we end up using)

It would actually be great to be able to remove the search from the admin toolbar. It confuses many of our site users, who think it would let them search the site's content. Perhaps this requires a separate permission rather than relying on "access toolbar" or "Administer Search". Something like "Use Toolbar Search" would make sense to me.

I know work is being done on moving search into a submodule, which would address this.

I agree that 'administer site configuration' is too broad (and overly restrictive). Many users may have admin_toolbar access and not whole-site admin who could use the built-in toolbar search.

@ckaotik provides great suggestions in #6. I'd add that each $links[] = .. could be wrapped with an access check.

Patch-attached:

This patch only provides the Admin Toolbar search item when the user has the new permission (provided by this patch) 'admin toolbar tools use search' "Use the toolbar search" (so namespaced to fit the module, so labeled so it fits core Toolbar nomenclature) by preventing the adding of the entire set of $items related to this search, because merely blocking access like @ckaotik cleverly proposed only blocks the parent item-- its sub elements like 'tray' and '#attached' were still added, causing a garbled-looking toolbar.

Earlier, I had also suggested $links[] = .. could be wrapped with an access check. These are under _additional_ links beyond what the clientside js already discerns from admin toolbar links from dom query (pretty clever -- so this part, at least, already has built-in access check by menu item). I don't know about these getExtraLinks() so I let it be :)

I also secured the ajax search callback endpoint /admin/admin-toolbar-search with this new user permission.

Ooh -- sorry. Same comment and patch as before but includes the missing permissions yml this time.

I don't think we should list the module 'admin toolbar tools' in the permission, as we're considering moving the search feature to its own submodule:

--- a/admin_toolbar_tools/admin_toolbar_tools.routing.yml
+++ b/admin_toolbar_tools/admin_toolbar_tools.routing.yml
@@ -93,4 +93,4 @@ admin_toolbar.search:
   defaults:
     _controller: '\Drupal\admin_toolbar_tools\Controller\ToolbarController::search'
   requirements:
-    _permission: 'administer site configuration'
+    _permission: 'admin toolbar tools use search'

Yep, I gave that consideration: If and when that occurs, things like this will need to be updated at that time-- if it's imminent, ok, please advise what that new submodule's name will be and we could crowbar it in with this commit. Not knowing that issue's real status, it made sense to me to stick to Drupal naming convention with this modest contribution.

Updated patch + test coverage. Lots of good points in #6.

Sorry to highjack, but the menu item ‘Manage’ should also be removed for non-admin roles :)

@liliplanet, please open a separate issue for this.

Hi @oknate, do you think we need to update the patch now that #3090760: Move search feature to a submodule was committed?

Yes, I think so. Here's a reroll. I moved the permission and test into the submodule.

Oops, fixing the other test.

I think this is good to go, if someone can review it.

Merci ;-)

#3117860: Add a permission to the Manage menu to discuss about the permission for the Manage menu item

Hello, closed issue. Is it something I'm doing wrong that with the latest release (which includes this patch), I see Search for an unprivileged user (unexpected), but it's unusable (as expected)?

and

It was with this observation that I advocated for my #10 (#9 desc) patch. It might be my theme or structure-sync'd menu. Just wanted to float it by anyone else who might see what I see or can advise on what's going on.
Disabling the new admin_toolbar_search module removes it cleanly.

We're seeing the same issue as @texas-bronius where when the permission is not granted, the search elements still appear and in a broken state. Should a new issue be created since this issue is closed and released?

Hi, @texas-bronius yes, open a new issue using the Issue Summary Template and add the steps to reproduce the issue.