Problem/Motivation
Sites might want to give users access to the toolbar that aren't actually admins. For example, to provide easy access to shortcuts, a more slick user menu experience, etc. Toolbar doesn't have to just be for admins.
However, as currently written, toolbar.module assumes that anyone seeing the toolbar must be an admin, and it always provides a "Manage" tray, even if the user cannot access any admin links and the tray is empty.
Note: although the machine name for the permission to use the toolbar is just access toolbar
, the label in the admin UI is unfortunately called "Use the administration toolbar". Fixing that is at #3025839: Change toolbar's permission label to 'Use the toolbar'. A more complete solution for this will be at #1044090: Enable toolbar for authenticated users also, so that non-admin users can use shortcuts as well. This issue is only about fixing the bug in the toolbar that renders an empty tray, not about "rebranding" the toolbar to stop assuming only "admins" use it.
Steps to reproduce
- Disable shortcut module (to avoid confusion).
- Create a user with
access toolbar
permission but nothing admin-related. In particular, notaccess administration pages
. - Login as the non-admin auth user.
- You should see an empty "Manage" tray in the toolbar (along with the "User" tray).
- If you apply the latest patch, clear all caches, and reload a page, the "Manage" tray should disappear and you should just have the User tray.
Proposed resolution
Test if the current user has at least one of the following permissions before doing any work to create/render the "Manage" link and tray in the toolbar:
access administration pages
access content overview
access media overview
administer blocks
administer comments
administer nodes
It'd be "better" to see if the admin menu is in fact empty for the user, but thanks to lazy loading, #pre_render, #2301317: MenuLinkNG part4: Conversion and all sorts of other unfathomable complication, that code is now far removed from where we actually register the 'Manage' toolbar link/tray.
Remaining tasks
Write and verify test (comment #27)Write code that fixes failing test (comment #39)- Review
- Commit
Rejoice(and unblock [#1044090])
User interface changes
Fixes a UI bug where an empty "Manage" tray appears for users who don't have access to any administration pages.
Before
After
API changes
None.
Original report by @jessebeach
Almost anyone developing on Drupal 8 has admin privileges to their local site. We rarely see that Toolbar module under a permission set that doesn't have god-access.
We need to do some permission combo tests and verify that the Toolbar renders and displays well when a user does not have bypass privileges, but simple privilege sets that someone like a content editor might have.
Permission Combinations
Standard authenticated user with use administration toolbar permission
The Menu tab is visible and the tray is empty. The tab should not be visible.
Test the Toolbar against reduced permission sets that simulate non-admin users: content producers, content editors, site builders, etc.
Comment | File | Size | Author |
---|---|---|---|
#93 | Screenshot 2022-01-11 at 10.15.22 AM.png | 229.96 KB | ankithashetty |
#80 | 2135445.77_80.interdiff.txt | 1.48 KB | dww |
#80 | 2135445-80.patch | 3.49 KB | dww |
#58 | 2135445.39_58.interdiff.txt | 845 bytes | dww |
#58 | 2135445-58.no-empty-manage-tray.patch | 3.74 KB | dww |
Comments
Comment #1
webchickCan I one-up this by saying that if we do discover any misnomers, we should try and write automated tests that capture this as well?
I'm hoping this is possible with straight-up simpletest and just some assertLink() action.
Comment #2
jessebeach CreditAttribution: jessebeach commentedComment #3
jessebeach CreditAttribution: jessebeach commentedI looked through all the existing Toolbar issues and I could not find any about permissions issues.
I also tested various permission combinations and the only problem I could find is the empty Admin menu. I think the resolution is quite clear. When the admin menu link list comes back empty, because the various access checks for individual links returned false, then don't render the Admin tab.
I added a test to make sure that the Admin tab is not rendered for an authenticated user who happens to only have the 'access toolbar' permission. I provided a 'fail' patch to show how the current code fails before the patch.
Comment #5
Sam152 CreditAttribution: Sam152 commented3: toolbar-permissions-2135445-1.patch queued for re-testing.
Comment #6
Sam152 CreditAttribution: Sam152 commentedLooks like the tests pass for #3 now, not sure what was going on before.
I spooled this up and it worked exactly as advertised. The only minor issue was busting the 80 gutter. I'm not that familiar with the toolbar code yet, but the rest looks good to me.
Re-rolled the patch with the fixed comment.
Comment #8
Sam152 CreditAttribution: Sam152 commentedMessed up the patch. Doing everything described in #6 again.
Comment #9
Sam152 CreditAttribution: Sam152 commentedComment #10
aspilicious CreditAttribution: aspilicious commentedOh yes, thnx for opening this issue :)
Comment #11
Sam152 CreditAttribution: Sam152 commentedI believe this patch also resolves #1938448: Empty toolbar sub-menu looks funny; Don't render empty trays.
Comment #12
Sam152 CreditAttribution: Sam152 commentedNo longer applied. Re-rolled against HEAD.
Comment #13
martin107 CreditAttribution: martin107 commented12: toolbar-permissions-2135445-12.patch queued for re-testing.
Comment #15
martin107 CreditAttribution: martin107 commentedComment #16
Sam152 CreditAttribution: Sam152 commented12: toolbar-permissions-2135445-12.patch queued for re-testing.
Comment #18
Sam152 CreditAttribution: Sam152 commentedRerolled.
Comment #19
Sam152 CreditAttribution: Sam152 commentedComment #20
SweetchuckThe patch #18 is applicable to the latest 8.x 0a8e34cf15f237c0672dd6ea7776d46393467ce1 Sat Mar 29 12:40:21 2014 +0100
Comment #27
Mile23Setting to needs review only to run the test. It'll fail and need work.
I updated the test but the patch to toolbar module is pretty crusty and doesn't apply, and looks like a nightmare to rebase.
Comment #29
Mile23Comment #32
dwwThis is a bug, not a task. We shouldn't be generating an empty 'Manage' tray. It's really confusing if you give users access to the toolbar (so they have easy access to shortcuts) but they're not site admins. This bug will be much more visible if/when #1044090: Enable toolbar for authenticated users also, so that non-admin users can use shortcuts as well ever happens. That said, I don't think it's fair to call this "major", since it's a bit edge-casey.
Comment #33
Wim LeersWould love to see this fixed :)
Comment #34
dww@Wim Leers and I tried to make sense of this mess in Slack. A few notes:
#pre_render
callback now. So, insidetoolbar_toolbar()
itself, there's no good/clean way to check if the menu tree is empty or not. :(But ugh, that seems like even more churn and complication.
Here's a totally new, somewhat naive, but very simple solution. Inside
toolbar_toolbar()
before we start doing anything about the admin menu and the manage tray, we test if the current user has the 'access administration pages' permission. If not, bail early and save a huge ton of complicated code we don't need. Yes, a site could still screw this up, give that perm to a role that doesn't really need it, and users in that role would still get the empty manage tray if they didn't actually have access to any admin menu items. But, that's basically a site configuration bug. So, I'm pretty sure this is all we need.What about cacheability?
toolbar_page_top()
already does this, so the entire toolbar is already cached based on user.permissions:So, I think we're cool on that front.
Yes, I'm introducing another return inside
toolbar_toolbar()
but that makes the patch way more understandable than indenting another ~100 lines of the function 2 spaces just to be inside anif ()
. If everyone wants a much bigger and harder to read patch (making the rest of the code in that function harder to read, too), we can obviously do that, instead. I'm aiming for the smallest possible change to get this working. ;)I'm including the test from #27 (with very minor fixes to the code comments). That test still applies cleanly, so consider comment #27 the test-only version of this patch. Interdiff against #27 attached, showing the changes to the test comments and the actual fix that should make this green (it passes locally).
Thoughts?
Thanks!
-Derek
Comment #35
dwwPreemptively attaching a version of the same fix without the early return and indenting the rest of the function, instead. As promised, it's an ugly mess. :)
I'd much rather we review/commit #34 (I don't want this whole function git blaming me after this lands), but if core committers think this is better, here you go. ;)
Cheers,
-Derek
Comment #36
dwwUpdated the IS to match the current state, added before/after screenshots of an auth user with 'access toolbar' and 'access shortcuts' but not 'access administration pages'.
Comment #39
dwwAdding 'access administration pages' to the necessary test classes. ;)
Comment #40
dwwp.s. I had re-queued the test-only in #27 against 8.6.x and as expected it a) still applies cleanly and b) fails as intended. Hence, no new test-only patch of my own.
p.p.s. My "it passes locally" comment at the end of #34 was because I had only run
php core/scripts/run-tests.sh --url http://localhost/drupal-8_6 --verbose --class "Drupal\Tests\toolbar\Functional\ToolbarAdminMenuTest"
, not the entire test suite. Apologies for the noise.Comment #41
Kristen PolI used simplytest.me to create a site with the patch from #39 and created a new user that was not an admin but I didn't see the toolbar area at all. I assume I did something wrong when testing. I might be able to try again tomorrow.
Comment #42
Kristen PolI forgot to upload the screenshots.
Comment #43
dww@Kristen Pol: Thanks for testing this! However, for a user to see the toolbar at all, you must give (one of) its role(s) the 'access toolbar' permission (which sadly is still labeled "Use the administration toolbar" in the admin UI on the permissions page). A more complete solution to all of this will have to happen at #1044090: Enable toolbar for authenticated users also, so that non-admin users can use shortcuts as well. This issue is only about fixing the bug that for users that can access the toolbar, but not any admin* pages, they get an empty 'Manage' icon/tray per my screenshots in the summary. Updated the summary to clarify.
Cheers,
-Derek
Comment #44
dwwSee also #3025839: Change toolbar's permission label to 'Use the toolbar'
Comment #45
Kristen PolDoh! My apologies for the oversight. I'll try to test again soon.
Comment #46
Kristen PolOk, I see the toolbar and can see the shortcuts if the auth user can administer shortcuts.
Comment #47
Kristen PolComment #48
Kristen PolSigh... uploaded the wrong file... trying again.
Comment #49
dww@Kristen Pol: Here is the one case that needs to be tested to see this bug:
A user with
access toolbar
andaccess shortcuts
but nothing admin-related. In particular, notaccess administration pages
.Before the patch, you should see the empty "Manage" tray from the before screenshot in the summary. That's the (only) bug this issue is fixing. After the patch, the entire "Manage" tray should go away for this user (since the tray is empty). See the after screenshot.
Any user with
access administration pages
will be able to see the "Manage" link, and it'll have some menu items, both before and after the patch. If you add anyadmin*
permissions to your test "authenticated" user, you're no longer in the case where the bug is visible.You shouldn't touch
administer shortcuts
. That's irrelevant to this issue. That falls under the "granting some admin* perms to the user" case, which would invalidate the conditions for seeing the bug in the first place.Re: #48: if the user itself doesn't have a shortcut set defined, you won't see any shortcuts in the shortcut tray. That's arguably a bug in shortcut module, but it's mostly a site configuration bug, and it's out of scope for this issue. This issue is only about the "Manage" tray. In fact, you can see the bug (and the toolbar) with the shortcut module completely disabled (in which case you'd only have the "User" tray in the toolbar after the patch is applied). Perhaps it'd be more clear for your testing and screenshots to disable shortcut.module.
Hope that all makes sense.
I'm going to hide all the screenshots from the files table, since they're off topic for this bug.
Thanks again,
-Derek
Comment #50
dwwAdded a Step to reproduce section to the summary, cleaned up a few typos, and fixed some formatting.
Comment #51
Kristen PolOk, sorry about my confusion. Late night peer reviews are maybe not a good idea. :)
I have followed the steps in the steps to reproduce. Thanks for adding those, and for your patience.
Marking RTBC!
Here's without patch:
Admin user:
Non-admin user:
Here's with patch:
Admin user:
Non-admin user:
Comment #52
dwwThanks! To be extra clear, your "Non-admin user" screenshot from before the patch should show what happens when you click on "Manage", not the User tray. That would visually reveal the bug this issue fixes. But whatever, I already have that screenshot in the summary (since comment #36), so it probably doesn't matter.
Cheers,
-Derek
Comment #53
Wim Leers#34: totally agreed
toolbar.module
is not in a great state. It was written early in the D8 cycle (2012–2013) based on https://www.drupal.org/project/admin_menu. D8's terrible performance at the time masked performance problems with it, which then had to be fixed urgently and made the code confusing/hard to maintain. It then was sporadically updated in the subsequent years. Which has left it in a very confusing state overall. If you'd like to, I think you'd be welcomed with open arms to become an official maintainer of it to get us to a better place. 😊 Thanks for also doing #3025839: Change toolbar's permission label to 'Use the toolbar'!Seems reasonable! It's not perfect, but it allows a very very simple patch to make a huge difference. I like your style 😀
Comment #54
dwwRe: #53: Thanks! Glad you like the simple approach. :)
Duly noted on the history of toolbar. I get it, I really do. And thanks but no thanks for the offer to become an official co-maintainer. ;) I'm trying to avoid burnout by having reasonable expectations about my time and contributions around here. Gone are the days of countless hours of unpaid labor Just Because It's The Right Thing To Do(tm). If someone wants to hire me to become a toolbar maintainer, let's talk. Otherwise, I'm trying to be a responsible contributor, but mostly focus on what I actually need to get my projects unblocked...
Cheers,
-Derek
Comment #55
Wim Leers👌 — I did not mean to force you of course, I just wanted to make sure you know it is an option, if it happened to be something you'd like to do :)
The right thing to do is to live a happy life!
👍 — you've been more than just responsible here 😉
Comment #56
Kristen Pol+100 to Wim's comments :)
Comment #57
larowlanCan we also add a positive assert too, e.g. if the whole page is broken, this test will still pass, because we only have a negative assert.
The fact we had to change a couple of tests here makes me feel that there is some minor disruption here, so can we get a change record to communicate the change?
Also, needs to go into 8.7.x first.
Other than that, looks great.
Comment #58
dwwRe: test: Fixed.
Re: CR: Should we expand The permission to use the toolbar has been renamed from "Use the administration toolbar" to "Use the toolbar" and have a single CR, or do you want a separate one for this?
Thanks,
-Derek
Comment #59
dww@larowlan via slack: "Second cr if possible, cause different audience".
Viewing the toolbar's "Manage" tray now requires the 'access administration pages' permission
Comment #60
Kristen PolThanks for the update. The interdiff in #58 seems fine to me. I retested with the new patch and it worked as expected. Marking RTBC.
(Update: Sorry for re-testing. @larowlan pointed out in chat that it wasn't actually needed since the update was to a test. Makes sense!)
Comment #61
larowlanCommitted 211c664 and pushed to 8.7.x. Thanks!
This is eligible for backport to 8.6.x but we're in a commit freeze at the moment.
Comment #63
dwwRe: #61 - thanks!
Is this a temp freeze? For 8.6.8 release? Anything I should follow-up about, or y'all will get back to this as RTBC for 8.6.x?
Meanwhile, I re-queued #58 to test with 8.6.x. Should be all green, but just in case...
Cheers,
-Derek
Comment #64
larowlanyeh 48 hr freeze for 8.6.8, so we'll come back to this once that's done
Comment #65
dwwSweet, makes sense. Meanwhile, bot came back happy and green for #58 against 8.6.x.
Cheers,
-Derek
Comment #67
dwwNo idea what the bot is thinking with #66. Please ignore. See current test results against 8.6.x in #58. Still RTBC.
Thanks,
-Derek
Comment #68
Wim LeersGreat job, @dww! 👏
Comment #70
larowlanCherry-picked as f458f5a290 and pushed to 8.6.x
Freeze is well and truly over, but was warranted as we had an emergency release pretty much two days after the point release.
Thanks for your work folks
Comment #71
larowlanPublished the change record
Comment #72
dwwYay, thanks!
Cheers,
-Derek
Comment #74
BerdirDoesn't this break access to admin/content?
access administration pages grants access to a lot of configuration pages, like admin/reports/status/rebuild, admin/structure, tons of pages below admin/config like media, regional and so on.
Until now, I could give editors access to the toolbar and they just saw "Content" (which does use the same permission but only when node.module is not enabled, so never). I could make a shortcut I guess, but that doesn't show for example the links below like add content (especially when using admin_toolbar).
> Due to other limitations and assumptions, existing sites probably already grant this permission to any user configured to see the toolbar.
In short, I don't think this assumption is correct.
Comment #75
larowlanI agree, so I think we have to either roll this back or quickly create a follow-up that makes it check 'access content overview' permission too, but it might also be there are other permissions.
As this isn't yet in a tagged release
git tag --contains f458f5a290
I think rolling it back is the safest move.I will check with other committers.
Comment #76
larowlanDiscussed with @xjm and she agreed revert is the best move given this didn't make it into a tagged release yet.
Have done that :sob:
Comment #77
dwwAlas. :( Sorry about the false assumption.
I suspect y'all will hate this, but here's a version that loops over a few of the more common admin perms (based on absolutely no hard data) ;) instead of just 'access administration pages'. It means we don't have to touch any other tests, a sign of reduced or eliminated disruption.
Added a test for a user with only 'access toolbar' and 'access content overview' to ensure they still see a 'Manage' tray.
Comment #78
dwwUpdating the proposed resolution and remaining tasks.
Comment #79
dwwAlso, put the CR back to draft state and updated it to reflect the current proposed solution:
https://www.drupal.org/node/3030597/revisions/view/11297934/11331387
Comment #80
dwwCombining the test methods to avoid reinstalling all of Drupal unnecessarily. Shaves 14 seconds off the time for Drupal\Tests\toolbar\Functional\ToolbarAdminMenuTest on a local test run.
Comment #81
larowlanthis works, but I think a hook_toolbar_permissions_alter would be a worthwhile addition instead of a todo.
Then the permissions could live in the respective modules
thoughts?
Comment #82
BerdirHm, a hook isn't completely free either and the list is fairly arbitrary. Another idea would be to add a new permission for this (maybe additionally to to access administration pages), then we could even have an update function that grants that permission to all roles that are allowed to use the toolbar but don't have the administration pages permission. Then we can guarantee that all sites keep their existing behavior and if you don't want to see it, they can remove that permission.
Comment #83
dwwWhoops, I completely forgot this got reverted. I've been happily running my patch on my own sites. Eek. Might be too late to get this into 8.8.0, not sure. Alas for getting busy!
Comment #84
andypostComment #85
BerdirNote that #2914110: Shortcut hook_toolbar implementation makes all pages uncacheable now has a similar but likely rarer scenario. If a user does have access to use shortcuts, but doesn't have access to any actual shortcut, then the Shortcut tab was hidden, now that we make it a lazy builder, it becomes visible again. I think that's less of an issue, because you simply shouldn't give that user access shorcuts then.
However, I was thinking if maybe a generic, client-side solution might be an option, where we hide the tabs with JS if there's nothing inside might be an option? Not sure though, as I might lead to flickering, especially since Shortcuts would likely be initially shown, then hidden and then shown again once the lazy builder completes. Or we make it hidden by default until that completed.
Unless we can make that work in a useful way, I still think that #82, with an extra new permission is the easiest solution here?
Comment #86
dwwI think all the rendering / hiding / showing client-side is already fairly problematic, and I'd rather not add any more steps to that mess. So yeah, something like #82 and a new permission is probably the best solution.
Meanwhile, I assume this needs to be 8.9.x now, right? At least for main development, then we can sort out if this is backportable to 8.8.x or not.
Thanks,
-Derek
Comment #91
nod_Patch exists that needs to be updated
Comment #92
SweetchuckComment #93
ankithashettyPatch in #80 still applies successfully, hence doesn't need a reroll. But yeah, it might need to be updated with the reviews made after #80.
Thanks!