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

  1. Disable shortcut module (to avoid confusion).
  2. Create a user with access toolbar permission but nothing admin-related. In particular, not access administration pages.
  3. Login as the non-admin auth user.
  4. You should see an empty "Manage" tray in the toolbar (along with the "User" tray).
  5. 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

  1. Write and verify test (comment #27)
  2. Write code that fixes failing test (comment #39)
  3. Review
  4. Commit
  5. 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

Screenshot before fix showing empty 'Manage' tray

After

Screenshot after fix showing no 'Manage' tray at all

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.

CommentFileSizeAuthor
#93 Screenshot 2022-01-11 at 10.15.22 AM.png229.96 KBankithashetty
#80 2135445.77_80.interdiff.txt1.48 KBdww
#80 2135445-80.patch3.49 KBdww
#77 2135445.58_77.interdiff.txt3.54 KBdww
#77 2135445-77.patch3.79 KBdww
#60 Screen Shot 2019-02-04 at 5.23.49 PM.png157.39 KBKristen Pol
#60 Screen Shot 2019-02-04 at 5.22.44 PM.png249.07 KBKristen Pol
#60 Screen Shot 2019-02-04 at 5.22.21 PM.png133.07 KBKristen Pol
#60 Screen Shot 2019-02-04 at 5.24.13 PM.png187.14 KBKristen Pol
#58 2135445.39_58.interdiff.txt845 bytesdww
#58 2135445-58.no-empty-manage-tray.patch3.74 KBdww
#51 Screen Shot 2019-01-15 at 9.09.36 AM.png163.05 KBKristen Pol
#51 Screen Shot 2019-01-15 at 9.08.54 AM.png186.83 KBKristen Pol
#51 Screen Shot 2019-01-15 at 9.04.45 AM.png130.52 KBKristen Pol
#51 Screen Shot 2019-01-15 at 9.04.26 AM.png123.08 KBKristen Pol
#51 Screen Shot 2019-01-15 at 9.03.23 AM.png95.3 KBKristen Pol
#51 Screen Shot 2019-01-15 at 8.59.39 AM.png74.44 KBKristen Pol
#51 Screen Shot 2019-01-15 at 8.59.19 AM.png118.5 KBKristen Pol
#48 Screen Shot 2019-01-15 at 12.02.19 AM.png131.22 KBKristen Pol
#47 Screen Shot 2019-01-15 at 12.03.31 AM.png224.44 KBKristen Pol
#47 Screen Shot 2019-01-14 at 11.55.53 PM.png155.51 KBKristen Pol
#46 Screen Shot 2019-01-14 at 11.58.00 PM.png186.84 KBKristen Pol
#46 Screen Shot 2019-01-14 at 11.55.37 PM.png165.81 KBKristen Pol
#46 Screen Shot 2019-01-14 at 11.55.53 PM.png155.51 KBKristen Pol
#42 Screen Shot 2019-01-13 at 6.38.55 PM.png100.07 KBKristen Pol
#42 Screen Shot 2019-01-13 at 10.00.59 PM.png86.24 KBKristen Pol
#42 Screen Shot 2019-01-13 at 10.00.46 PM.png124.56 KBKristen Pol
#39 2135445.34_39.interdiff.txt955 bytesdww
#39 2135445-39.no-empty-manage-tray.patch3.48 KBdww
#36 2135445-36.toolbar-after.png30.95 KBdww
#36 2135445-36.toolbar-before.png34.52 KBdww
#35 2135445.34_35.interdiff.txt5 KBdww
#35 2135445-35.no-empty-manage-tray-with-massive-indenting.patch5.63 KBdww
#34 2135445.27_34.interdiff.txt1.92 KBdww
#34 2135445-34.no-empty-manage-tray.patch2.19 KBdww
#27 2135445_27_test_only.patch1.02 KBMile23
#18 toolbar-permissions-2135445-18.patch5.66 KBSam152
#12 toolbar-permissions-2135445-12.patch5.36 KBSam152
#8 interdiff.txt610 bytesSam152
#8 toolbar-permissions-2135445-8.patch5.35 KBSam152
#6 toolbar-permissions-2135445-6.patch5.86 KBSam152
#6 interdiff.txt610 bytesSam152
#3 toolbar-permissions-2135445-1.patch5.99 KBjessebeach
#3 toolbar-permissions-2135445-1-fail.patch1.5 KBjessebeach
#2 user-admin-toolbar-2.png15.66 KBjessebeach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Can 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.

jessebeach’s picture

Issue summary: View changes
FileSize
15.66 KB
jessebeach’s picture

Status: Active » Needs review
FileSize
1.5 KB
5.99 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, 3: toolbar-permissions-2135445-1-fail.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
Sam152’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
610 bytes
5.86 KB

Looks 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: toolbar-permissions-2135445-6.patch, failed testing.

Sam152’s picture

Messed up the patch. Doing everything described in #6 again.

Sam152’s picture

Status: Needs work » Needs review
aspilicious’s picture

Oh yes, thnx for opening this issue :)

Sam152’s picture

Sam152’s picture

No longer applied. Re-rolled against HEAD.

martin107’s picture

Status: Needs review » Needs work

The last submitted patch, 12: toolbar-permissions-2135445-12.patch, failed testing.

martin107’s picture

Issue tags: +Needs reroll
Sam152’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: toolbar-permissions-2135445-12.patch, failed testing.

Sam152’s picture

Rerolled.

Sam152’s picture

Status: Needs work » Needs review
Sweetchuck’s picture

Issue tags: -Needs reroll

The patch #18 is applicable to the latest 8.x 0a8e34cf15f237c0672dd6ea7776d46393467ce1 Sat Mar 29 12:40:21 2014 +0100

Status: Needs review » Needs work

The last submitted patch, 18: toolbar-permissions-2135445-18.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

Setting 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.

Status: Needs review » Needs work

The last submitted patch, 27: 2135445_27_test_only.patch, failed testing. View results

Mile23’s picture

Title: Manually test the Toolbar module under various permission combinations; try to break it » Toolbar displays Manage tab even if the user is not permitted to see it

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dww’s picture

Category: Task » Bug report
Priority: Major » Normal

This 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.

Wim Leers’s picture

Would love to see this fixed :)

dww’s picture

@Wim Leers and I tried to make sense of this mess in Slack. A few notes:

  1. Holy crap, toolbar.module is hard to follow. :/ It's got @see comments pointing to functions that no longer exist. It's a real tangled web of pain for anyone like me trying to make any sense of it for the first time.
  2. Something like #18 would be the right approach, but thanks to #2301317: MenuLinkNG part4: Conversion the rendering of the admin menu tree actually happens in the #pre_render callback now. So, inside toolbar_toolbar() itself, there's no good/clean way to check if the menu tree is empty or not. :(
  3. Wim suggested in Slack:
    We _could_ just:
    - make the `Manage` toolbar tab hidden by default
    - modify `toolbar.menu.es6.js` to show it _if_ the lazy-loaded subtrees are in fact non-empty

    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:

    '#cache' => [
      'keys' => ['toolbar'],
      'contexts' => ['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 an if (). 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

dww’s picture

Preemptively 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

dww’s picture

Updated 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'.

The last submitted patch, 34: 2135445-34.no-empty-manage-tray.patch, failed testing. View results

Status: Needs review » Needs work
dww’s picture

Adding 'access administration pages' to the necessary test classes. ;)

dww’s picture

p.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.

Kristen Pol’s picture

I 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.

Kristen Pol’s picture

dww’s picture

Issue summary: View changes

@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

dww’s picture

Kristen Pol’s picture

Doh! My apologies for the oversight. I'll try to test again soon.

Kristen Pol’s picture

Ok, I see the toolbar and can see the shortcuts if the auth user can administer shortcuts.

Kristen Pol’s picture

Kristen Pol’s picture

Sigh... uploaded the wrong file... trying again.

dww’s picture

@Kristen Pol: Here is the one case that needs to be tested to see this bug:

A user with access toolbar and access shortcuts but nothing admin-related. In particular, not access 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 any admin* 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

dww’s picture

Issue summary: View changes

Added a Step to reproduce section to the summary, cleaned up a few typos, and fixed some formatting.

Kristen Pol’s picture

Ok, 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:

dww’s picture

Thanks! 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

Wim Leers’s picture

#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'!

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

Seems reasonable! It's not perfect, but it allows a very very simple patch to make a huge difference. I like your style 😀

dww’s picture

Re: #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

Wim Leers’s picture

I'm trying to avoid burnout by having reasonable expectations about my time and contributions around here.

👌 — 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 :)

Gone are the days of countless hours of unpaid labor Just Because It's The Right Thing To Do(tm).

The right thing to do is to live a happy life!

. Otherwise, I'm trying to be a responsible contributor, but mostly focus on what I actually need to get my projects unblocked...

👍 — you've been more than just responsible here 😉

Kristen Pol’s picture

+100 to Wim's comments :)

larowlan’s picture

Version: 8.6.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
+++ b/core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php
@@ -395,6 +395,19 @@ public function testExternalLink() {
+    $this->assertSession()->elementNotExists('css', 'a[id=toolbar-item-administration]');

Can 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.

dww’s picture

Issue tags: -Needs change record

@larowlan via slack: "Second cr if possible, cause different audience".

Viewing the toolbar's "Manage" tray now requires the 'access administration pages' permission

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
187.14 KB
133.07 KB
249.07 KB
157.39 KB

Thanks 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!)

larowlan’s picture

Version: 8.7.x-dev » 8.6.x-dev

Committed 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.

  • larowlan committed 211c664 on 8.7.x
    Issue #2135445 by dww, Sam152, jessebeach, Mile23, Kristen Pol, Wim...
dww’s picture

Re: #61 - thanks!

This is eligible for backport to 8.6.x but we're in a commit freeze at the moment.

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

larowlan’s picture

yeh 48 hr freeze for 8.6.8, so we'll come back to this once that's done

dww’s picture

Sweet, makes sense. Meanwhile, bot came back happy and green for #58 against 8.6.x.

Cheers,
-Derek

The last submitted patch, 39: 2135445-39.no-empty-manage-tray.patch, failed testing. View results

dww’s picture

No idea what the bot is thinking with #66. Please ignore. See current test results against 8.6.x in #58. Still RTBC.

Thanks,
-Derek

Wim Leers’s picture

Great job, @dww! 👏

  • larowlan committed f458f5a on 8.6.x
    Issue #2135445 by dww, Sam152, jessebeach, Mile23, Kristen Pol, Wim...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Cherry-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

larowlan’s picture

Published the change record

dww’s picture

Yay, thanks!

Cheers,
-Derek

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Doesn'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.

larowlan’s picture

I 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.

larowlan’s picture

Version: 8.6.x-dev » 8.7.x-dev
Status: Closed (fixed) » Needs work

Discussed 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:

dww’s picture

Alas. :( 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.

dww’s picture

Issue summary: View changes

Updating the proposed resolution and remaining tasks.

dww’s picture

Also, 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

dww’s picture

FileSize
3.49 KB
1.48 KB

Combining 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.

larowlan’s picture

+++ b/core/modules/toolbar/toolbar.module
@@ -159,6 +159,35 @@ function toolbar_toolbar() {
+  // @todo Make this array configurable somehow?
+  $admin_permissions = [
+    'access administration pages',
+    'access content overview',
+    'access media overview',
+    'administer blocks',
+    'administer comments',
+    'administer nodes',
+  ];

this 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?

Berdir’s picture

Version: 8.7.x-dev » 8.6.x-dev
Status: Needs review » Active

Hm, 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.

dww’s picture

Version: 8.6.x-dev » 8.8.x-dev

Whoops, 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!

andypost’s picture

Berdir’s picture

Note 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?

dww’s picture

Version: 8.8.x-dev » 8.9.x-dev

I 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

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nod_’s picture

Status: Active » Needs work

Patch exists that needs to be updated

Sweetchuck’s picture

Issue tags: +Needs reroll
ankithashetty’s picture

Issue summary: View changes
Issue tags: -Needs reroll
FileSize
229.96 KB

Patch 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!

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.