Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed: Insufficient access to Taxonomy Block on Dashboard #5198

Closed
stpaultim opened this issue Sep 5, 2021 · 14 comments · Fixed by backdrop/backdrop#3737
Closed

Fixed: Insufficient access to Taxonomy Block on Dashboard #5198

stpaultim opened this issue Sep 5, 2021 · 14 comments · Fixed by backdrop/backdrop#3737

Comments

@stpaultim
Copy link
Member

stpaultim commented Sep 5, 2021

Description of the bug

With the addition of #382, we need to refine access to the "Taxonomy" block on the dashboard for roles to create or delete terms.

Currently, only those with permission to administer vocabularies or edit terms have access to this block, when this block is actually useful for anyone with permission to do anything with taxonomies. This was not the case prior to #382.

ALSO - user with "Administer Terms and Vocabularies" permission had a link to "configure" but it took them to the "list terms" page instead of the "configure" page.

Steps To Reproduce

To reproduce the behavior:

  1. Login to a new site as site editor (with default permission "Tags: Create terms")
  2. Visit the dashboard and see that the taxonomy block is not visible.
  3. Add "Tags: Edit terms" permission and view dashboard again and you will see that the block is available.

Actual behavior

Even with edit and delete terms, these are the only options you get with the block.

dashboard-terms

Expected behavior

Since the only two links that are provided by this block directly are "List Terms" and "Add New Terms", this block is useful to editors with any taxonomy permission. The ability to see available taxonomies and list the terms is valuable.

@olafgrabienski and @herbdool

@stpaultim
Copy link
Member Author

To test - create a TEST EDITOR account (Maybe use same password as admin). Give various taxonomy permissions to editor and see if the Dashboard block behaves as expected.

The following permissions should give access to the following on the dashboard

Administer vocabularies and terms

  • Ability to access the Taxonomy block
  • "List Terms" Link
  • "Add New Term" Link
  • "Configure" Link

Tags: Create terms

  • Ability to access the Taxonomy block
  • "List Terms" Link
  • "Add New Term" Link

Tags: Edit terms

  • Ability to access the Taxonomy block
  • "List Terms" Link

Tags: Delete terms

  • Ability to access the Taxonomy block
  • "List Terms" Link

No Permissions

  • NOT able to access the block at all

@stpaultim
Copy link
Member Author

I've updated the code in the PR based on some feedback from @klonos, but it still works the same.

@klonos klonos modified the milestones: 1.19.4, 1.20.0 Sep 5, 2021
@klonos
Copy link
Member

klonos commented Sep 5, 2021

Since this relies on a set of changes that went in only in 1.x, I've set the milestone to 1.20.

@stpaultim
Copy link
Member Author

@klonos - I've made requested changes to PR. Take a look again if you get the chance. Thanks.

@olafgrabienski
Copy link

olafgrabienski commented Sep 12, 2021

I've tested the PR sandbox site and got results as expected - described in #5198 (comment). I've also created a new vocabulary and played around with different permission combinations. Everything worked as expected!

@klonos
Copy link
Member

klonos commented Sep 12, 2021

Sorry @stpaultim and @olafgrabienski there are a few issues with the approach in this PR, so I'm gonna move this back to "needs work". Too many things to list now, so I'll file an alternative PR shortly...

@stpaultim
Copy link
Member Author

@klonos - I look forward to seeing your alternative. ;-)

@klonos
Copy link
Member

klonos commented Sep 12, 2021

OK, here's the PR: backdrop/backdrop#3737

  • Before, we were blindly loading all vocabularies with taxonomy_get_vocabularies(), and then iterating through all of them, in order to determine if they are in the list of vocabularies that have been configured to be shown in the block settings. Now, we compile the list of vocabularies that the block has been configured to show, and we then use taxonomy_vocabulary_load_multiple() to load only those vocabularies.
  • Before, we were checking access and creating the operations links for the block from scratch. Now, we reuse the _vocabulary_get_operations() helper function that was introduced in [D8][UX] Add Better Taxonomy permissions #382. Less code duplication + if we change anything re permissions/access in the future, those changes will be reflected in the operations links in the Dashboard block as well (so less likely to have the problem this issue was originally created for).
  • Tiny fix: changed the links from '#type' => 'dropbutton' to '#type' => 'operations' instead.
  • Instead of having custom logic add the There are no vocabularies to display. message, we now use the '#empty' variable that is passed to theme_table().
  • Tiny improvement: moved the $header array after the if ($no_access) { return array(); }, so that that bit of code is not executed if we bail earlier.

@jenlampton
Copy link
Member

I gave the PR test, and it works great!

Here's the dashboard from the editor's perspective:
Screen Shot 2021-09-12 at 7 41 34 PM

And here's the dashboard from the admins perspective:
Screen Shot 2021-09-12 at 7 41 46 PM

And when I added a new vocab with more limited permissions for editor, that vocab appears on the dashboard with a more limited set of operations 🎊
Screen Shot 2021-09-12 at 7 43 30 PM

@stpaultim
Copy link
Member Author

there are a few issues with the approach in this PR, so I'm gonna move this back to "needs work". Too many things to list now, so I'll file an alternative PR shortly

@klonos - Looks like there were issues with the code before the PR. ;-) Thanks for stepping in and cleaning this one up.

@klonos
Copy link
Member

klonos commented Sep 13, 2021

Yes @stpaultim, preexisting issues + we have not thought of the taxonomy Dashboard block when implementing #382 ...the main thing was (re)using _vocabulary_get_operations() for the operations links, but you know me ...if I start afixin' things, I gots to fix 'em all 😅

@stpaultim
Copy link
Member Author

I closed my PR in favor of the one by @klonos and reviewed by @jenlampton.

@klonos
Copy link
Member

klonos commented Sep 13, 2021

Rebased after the recent core commits that fix the random failures, and tests are green! 🎉

Thanks @indigoxela and @quicksketch 🙏🏼

@laryn
Copy link
Contributor

laryn commented Sep 13, 2021

Looks great! Thanks @stpaultim for filing and for the initial PR, and @klonos for doing some of the clean up work in your alternative PR. Thanks to @olafgrabienski and @jenlampton for testing and code review. I've merged the backdrop/backdrop#3737 PR in backdrop/backdrop#3737 for 1.x so it will be included in 1.20.

@jenlampton jenlampton changed the title Insufficient access to Taxonomy Block on Dashboard Fixed: Insufficient access to Taxonomy Block on Dashboard Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment