Per vocabulary permissions do not appear to do anything

[laz0rama]

giving a role permission to edit terms in a particular vocabulary does not allow users with that role to edit terms in the vocabulary UNLESS they also have the "administer vocabularies and terms" permission, which defeats the purpose of having per-vocabulary permissions.

my scenario:

there are permissions defined for editing and deleting each defined vocabulary, on the permissions page (/admin/config/people/permissions).

i have checked the edit permission on a vocabulary for a defined role "content editor" (i do NOT want to give them access to any other vocabularies), and saved. i have cleared all caches.

but when i log in as a user with the "content editor" role, and i try to access the term edit form for a term in that vocabulary (/taxonomy/term/47/edit), i get an access denied from backdrop.

hopefully this is enough information. if not, please let me know what more i can provide.

---

PR by @indigoxela: backdrop/backdrop#3052 (rename permission, closed in favor of the more lightweight change)
PR by @quicksketch: backdrop/backdrop#3067 (update access check)

[olafgrabienski]

Thanks for your report, @laz0rama ! The issue could be a duplicate of #382. Can you have a look at it?

[laz0rama]

that issue (#382) definitely looks related. though i have to admit i am not clear if that issue is about the "permissions per vocabulary" functionality not working at all. there is discussion there about specific details of how it functions for different roles/permissions, i didn't see anything about the feature generally not working. so i don't know if this would be a duplicate or not. i am happy to do whatever makes sense for the project. just let me know if i should close this issue, or whatever.

[olafgrabienski]

@laz0rama I guess #382 is not only about issues with "per vocabulary" permissions but the suggested module seems to address that topic:

[The the Taxonomy access fix module] adds additional permissions for accessing Vocabulary pages and adding new terms (currently to have to give users the 'Administer vocabularies and terms' permission to do this).

@BWPanda What do you think: Is this issue a duplicate of #382, or is the latter more broad in scope?

[ghost]

giving a role permission to edit terms in a particular vocabulary does not allow users with that role to edit terms in the vocabulary

I haven't tested this, but if that is indeed the case then this is a bug with existing functionality and not merely missing functionality like the other issue.

[indigoxela]

giving a role permission to edit terms in a particular vocabulary does not allow users with that role to edit terms in the vocabulary

Confirmed. Even if the role has permission to edit/delete terms in that vocabulary, no tabs appear on the term page and trying to access taxonomy/term/[NUM]/edit directly results in "Access denied".

The problem is "edit terms in" vs. "update terms in". See this issue for further information.

[olafgrabienski]

The problem is "edit terms in" vs. "update terms in". See this issue for further information.

This seems to be the respective change record: https://api.backdropcms.org/change-records/taxonomy_term_access-and-comment_access-op-edit-parameter-renamed-update

[indigoxela]

Here's a quick and dirty workaround:

Edit files/config_[the hash]/active/user.role.[role name].json
And change "edit terms in YOURVOCAB" to "update terms in YOURVOCAB"

The problem arises here:
https://github.com/backdrop/backdrop/blob/9ed127933df9994ec3e96f9110520359451a183d/core/modules/taxonomy/taxonomy.module#L68

As the permission is still saved as "edit terms in...", while the class itself only handles "update terms in..." ($op = 'update').

We need to change the string that creates the permission and we also need to update existing role configs to switch from edit to update.

[indigoxela]

Here's a PR

Please test and review.

[ghost]

I confirmed the bug by creating an authenticated user with permission to edit and delete terms, but not administer them. They were able to view a term page, and access the 'Delete' tab on that page, but there was no 'Edit' tab. Manually going to the edit URL showed an 'access denied' message.

I then applied the patch from the PR, cleared caches and updated the database. I refreshed the authenticated user's screen, and was able to see and access the 'Edit' tab for the term.

I've left a comment on the PR suggesting a change (or rather, suggesting not to change the human-readable name of the permission), otherwise the PR works and the code looks good!

[indigoxela]

@BWPanda many thanks for your review. PR has been updated accordingly.

[ghost]

Code looks good, thanks @indigoxela! I'll let someone else do a final review/test before marking as RTBC.

[klonos]

Code LGTM 👍

[quicksketch]

Thanks folks! Although I don't like the discrepancy between "edit" and "update" here, other permissions like nodes use "edit" even though the access parameter is "update". e.g. The permission name edit own post content currently matches edit terms in tags. Unless we also want to update the permissions for nodes for full consistency, perhaps we just leave the permission name for terms as-is and update TaxonomyTerm::access() so that it checks the current permission name.

[quicksketch]

I filed an alternative approach that updates the permission checking code to use the existing permission name, rather than renaming it. PR: backdrop/backdrop#3067

I like this approach because it keeps the same permission name as before, which is less change and more compatibility with previous versions (and D7).

[indigoxela]

I can confirm that the alternate patch works fine.

I created an unprivileged user and gave role "authenticated" the permission to edit and delete tags, then created a tag.
Before the patch only the delete tab was visible and accessing taxonomy/term/1/edit resulted in "Access denied". After applying 3067.patch the edit page was accessible.
Removing the permission for role authenticated also removed the access to edit page.

[indigoxela]

I agree that the lightweight change is preferable here.

[klonos]

Indeed! Much simpler implementation, and no need for an update hook.

[quicksketch]

Thanks folks! Merged backdrop/backdrop#3067 into 1.x and 1.15.x.