[D8][UX] Add Better Taxonomy permissions

[ghost]

Since discovering it 8 months ago, I have been installing the Taxonomy access fix module on all my sites. It is a great, simple module that fixes the lacking Taxonomy permissions in Drupal core. I recommend implementing it (or something similar) in Backdrop.

Basically, it adds additional permissions for accessing Vocabulary pages and adding new terms (currently you have to give users the broader 'Administer vocabularies and terms' permission to do this).

There is an issue for doing the same thing in Drupal 8 core here: https://www.drupal.org/node/1848686 (which might be helpful if we decide to do the same).

---

PR by @BWPanda: backdrop/backdrop#508 (merge conflicts)
PR by @herbdool backdrop/backdrop#3319
PR by @docwilmot backdrop/backdrop#3716

[quicksketch]

I was a bit baffled looking at the Taxonomy permissions recently for this exact reason (related to your term cancel links at #371). This seems like a good set of changes to me. Could we review exactly what changes we want to port over from Taxonomy access fix module, or if there's anything we don't want from it?

[ghost]

Ok, from what I can see by reading through the code, TAF does the following:

1. Adds new permission: 'Add terms in [vocabulary]' (for each vocabulary)
2. Edits 'taxonomy_overview_vocabularies' form if user doesn't have 'Administer vocabularies and terms' permission:
   • Removes weight elements
   • Removes 'Edit vocabulary' links
   • Removes 'Save' button
   • Removes 'Add terms' links (if user doesn't have new 'Add terms in [vocabulary]' permission)
   • Removes vocabularies (if user doesn't have any add/edit/delete access)
3. Updates the role_permission table when vocabulary machine-name changes
4. Edits 'taxonomy_overview_terms' form if user doesn't have 'Administer vocabularies and terms' permission:
   • Removes 'Edit' links (if user doesn't have 'Edit terms in [vocabulary]' permission)
   • Hides Save & Reset buttons (if user doesn't have any add/edit/delete access)
5. Changes access of:
   • 'admin/structure/taxonomy' to anyone with any 'add/edit/delete' permission for any vocabulary
   • 'admin/structure/taxonomy/%' to anyone with any 'add/edit/delete' permission for that vocabulary
   • 'admin/structure/taxonomy/%/add' to anyone with 'add' permission for that vocabulary
6. Implements hook_admin_menu_map() to add vocabulary links to admin bar when user doesn't have 'Administer vocabularies and terms' permission

[quicksketch]

Wow, this all sounds like great improvements. A lot of these sounds border-line bugs:

Updates the role_permission table when vocabulary machine-name changes

😮

All of these changes sound good to me. It has me a little worried that some of these updates may make converting the term listings to views more difficult, but I think built-in draggable views may be a ways off yet, so converting the taxonomy term listing pages isn't going to happen any time soon anyway.

I'd like to see tests for each of these things added. Different behaviors for different levels of permissions is one of those things that's hard to reproduce accurately and reasonably easy to test.

[ghost]

My PR addresses pretty much all this:

1. New 'add' permission added
2. I added a bunch of access checks so that elements were only added to the form if the user was allowed to see/use them.
3. Wasn't necessary as core uses vocabulary IDs in permissions (not sure why TAF decided to use the machine name...)
4. Same as 2.
5. Added some new access callbacks to allow for this.
6. A simple removal of 3 lines in the admin bar module fixed this.

[ghost]

Oh, BTW, no tests as I'm not familiar with them...

[quicksketch]

Wasn't necessary as core uses vocabulary IDs in permissions (not sure why TAF decided to use the machine name...)

That's perfectly fine, as we're renaming permissions as part of the CMI conversion for Taxonomy Terms, since auto-increment IDs aren't compatible with config files. The work over in #174 does this conversion and uses machine names everywhere.

Oh, BTW, no tests as I'm not familiar with them...

This PR overall looks great, but really can't add this expansion of permission granularity without tests to ensure that the permissions work as expected for the different roles with different permission levels. The existing TaxonomyTermTestCase in taxonomy.test is a pretty good start to show how testing is done on the Taxonomy UI currently. We could probably add another test within that class to check the permissions for users with each level of permissions.

[quicksketch]

I love this feature and the PR is close. For now I've closed it as it needs more work, but let's pick this up again if we can gets tests on the new functionality. The last PR was at backdrop/backdrop#508.

[jenlampton]

Yes, please! I also want better UX for permissions. See related: #900 and #38

[jenlampton]

edit: whoops, changing milestone on wrong issue.

[klonos]

...part of #378

[klonos]

The d.org project page for https://www.drupal.org/project/taxonomy_access_fix links to another related, very useful module: https://www.drupal.org/project/taxonomy_autocomplete_permission (less than 100 lines of code).

Defines which roles can add new taxonomy terms when using the autocomplete widget.

Adds 1 permission per vocabulary: "Add terms in %term via autocomplete"

Now with #38 implemented, it would be a great addition to merge the functionality of these modules.

[klonos]

Proposing milestone to be set for 1.13

[jenlampton]

Code in newest PR looks great - thanks for resolving the conflicts @docwilmot :)

I took it for another spin in the sandbox, but after a fresh install, the Editor role has the Edit and Delete permission on the Tags vocabulary, but not Create, which seems odd. I'd recommend we do it the other way around, and set them to Create, but not Edit or Delete.

I'm doing more testing, but wanted to note this here right away.

[jenlampton]

Ah, I see, the editor role previously had Administer taxonomy as well as Edit and Delete, so these permissions are unchanged.

For existing sites, I recommend that we add the Create permission for Editors (Even though they have Administer taxonomy so this isn't really an issue, it does seem strange when you view only the taxonomy page, where the "Administer taxonomy" permission does not appear).

For new installs, I'd prefer that we remove the existing permissions and add only Create terms permission. Thoughts on this?

[laryn]

For existing sites, I recommend that we add the Create permission for Editors.

Maybe check first to make sure the existing Editor role still has "Administer taxonomy" permissions? Should we also check if any other roles have been given that permission and do the same for them? (#382 (comment))

For new installs, I'd prefer that we remove the existing permissions and add only Create terms permission.

Sounds good to me.

[stpaultim]

I agree with Jen's suggestions for permissions. I was going to delay the discussion of default permissions to a follow-up issue, but if you are able to get those change in here too, great.

[docwilmot]

Done.

[jenlampton]

Testing again now! :)

Perfect:

term list without edit permission on vocab:

term list with edit permission on vocab:

term list with edit/delete permission, but not add:

[ghost]

I've merged backdrop/backdrop#3716 into 1.x. Thanks to @docwilmot, @quicksketch, @jenlampton, @stpaultim, @klonos, @herbdool, @olafgrabienski & @laryn for your work on this!

[laryn]

And thanks to @bwpanda for getting the whole issue rolling way back in 2014! 😉

[klonos]

@laryn or @BWPanda follow-up PR with some small fixes: backdrop/backdrop#3718