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

Patterns: update pattern category capabilities to prevent authors adding new categories #5510

Conversation

glendaviesnz
Copy link

@glendaviesnz glendaviesnz commented Oct 17, 2023

Currently in the 6.4 release Author users are able to add new pattern categories, but although pattern categories are a flat taxonomy they should behave like a hierarchical taxonomy and only allow creation by users with the manage taxonomy permissions.

Because the taxonomy capabilities settings are overridden when a taxonomy is not hierarchical the only way we could see to prevent Author level users from adding new categories was by adding a custom endpoint for pattern categories. Let us know if there is an alternative way of doing this that is preferable.

There is a Gutenberg PR open to fix this (WordPress/gutenberg#55379), and this PR copies the REST API changes to core.

Trac ticket: https://core.trac.wordpress.org/ticket/59660


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@TimothyBJacobs
Copy link
Member

Was it considered to make the taxonomy hierarchical? While this will fix the REST API, I believe the WP-Admin based management interface will still let authors create new terms. Additionally, any plugins that facilitate term management would be correct to let authors create new terms as well.

@glendaviesnz
Copy link
Author

Thanks for taking a look @TimothyBJacobs.

Was it considered to make the taxonomy hierarchical?

Yes, we did look at this, but this taxonomy needs to merge in with the core pattern categories that come from the pattern directory, etc. and this is a flat taxonomy. Our thinking was that making the user categories hierarchical in contrast to flat core/dir pattern categories opened the potential for users to end up with confusing duplicate categories at nested levels.

while this will fix the REST API, I believe the WP-Admin based management interface will still let authors create new terms.

In all of our testing the /wp-admin/edit-tags.php?taxonomy=wp_pattern_category wp-admin interface respects the default taxonomy capabilities and Author level users are not able to access this interface. It seems that only the REST API overrides the default capabilities for flat taxonomies.

Additionally, any plugins that facilitate term management would be correct to let authors create new terms as well.

My understanding based on the fact that wp-admin respects the default capabilities, and the change in this PR applies that to the REST API for the wp_pattern_category taxonomy that a plugin would need to explicitly change capabilities in order to allow Authors to add new terms to this taxonomy, and I don't think we need to prevent them from doing that if they have reasons for doing so. @talldan does that sound right to you?

@TimothyBJacobs
Copy link
Member

TimothyBJacobs commented Oct 20, 2023

It seems that only the REST API overrides the default capabilities for flat taxonomies.

This doesn't sound right to me. I believe the REST API is trying to emulate Core behavior. This was changed in https://core.trac.wordpress.org/ticket/44096

It looks like edit-tags.php is gated behind manage_terms, but I would be able to use edit_post().

For example, via the bulk edit UI ( as an author ):

Screenshot 2023-10-20 at 10 23 04 AM

@glendaviesnz
Copy link
Author

It looks like edit-tags.php is gated behind manage_terms, but I would be able to use edit_post().

Thanks for the extra detail @TimothyBJacobs. @talldan what are your thoughts on whether we need to try and block access to adding new terms here as well for 6.4?

@TimothyBJacobs
Copy link
Member

I don't think the blocking approach is safe from a security perspective. Fundamentally, Core and plugins are safe to assume that authors should be able to create new terms for that taxonomy.

I'd try and see if the hierarchical UX concerns can be alleviated.

@glendaviesnz
Copy link
Author

Thanks @TimothyBJacobs we will have a think about what is going to be the best approach here.

@talldan
Copy link
Contributor

talldan commented Oct 20, 2023

Thanks for your input @TimothyBJacobs.

It'd be a shame to make the pattern categories hierarchical just to satisfy the permissions model. That doesn't feel right to me, as the two things (hierarchical-ness and permissions) are separate and distinct qualities of a taxonomy.

From my point of view, the pattern categories are 'categories' as they're a top-down organisation structure (like putting things in folders), but we want to present them in a relatively simple flat fashion, so that's why they're non-hierarchical.

Going a bit deep here (and maybe this is unrelated), but I guess the main difference to post categories/tags is that those things exist as data that a user might surface on the front-end of their site, whereas pattern categories are more a backend organizational tool where we want to enforce some rigidity around what the user can do. Perhaps this is a misuse, not sure.

@SaxonF might have some further input here from a UX perspective.

@TimothyBJacobs
Copy link
Member

That doesn't feel right to me, as the two things (hierarchical-ness and permissions) are separate and distinct qualities of a taxonomy.

I agree, but that's how Core treats it. What we'd really want is a specific create_terms cap for taxonomies, but I think that's far too large of a change at this point in the cycle.

but we want to present them in a relatively simple flat fashion, so that's why they're non-hierarchical.

If the UI doesn't present a native way to create categories with parents, then for most users they'd just see that flat list.

whereas pattern categories are more a backend organizational tool where we want to enforce some rigidity around what the user can do.

Perhaps, but I think Media categories are the also a backend organizational tool, but I think users would expect to have hierarchy there.

@talldan
Copy link
Contributor

talldan commented Oct 21, 2023

I agree, but that's how Core treats it. What we'd really want is a specific create_terms cap for taxonomies, but I think that's far too large of a change at this point in the cycle.

Yep, definitely agree. It's good to know though that we're in alignment with our thinking (and that my understanding of the situation isn't completely wrong 😄 ).

If the UI doesn't present a native way to create categories with parents, then for most users they'd just see that flat list.

Is there a way to guarantee that? I worry about situations like plugins that might present alternate interfaces, a bit like you mentioned earlier.

Perhaps the most prudent route might be to keep it as a flat hierarchy and accept that anyone that can assign can also create pattern categories. That way it works as designed, and there are no unusual edge cases. It means it's not something that can easily change in the future though.

@hellofromtonya
Copy link
Contributor

Seems there's consensus to revert the change in Gutenberg WordPress/gutenberg#55532.

Any concerns of closing this PR and its Trac ticket? @glendaviesnz

@TimothyBJacobs
Copy link
Member

Is there a way to guarantee that? I worry about situations like plugins that might present alternate interfaces, a bit like you mentioned earlier.

No, I don't think so. Though the interface could probably always render a flat list for now, and treat is as a bug fix in a future release to display the list with hierarchy?

Perhaps the most prudent route might be to keep it as a flat hierarchy and accept that anyone that can assign can also create pattern categories.

Yeah this would definitely be the simplest.

It means it's not something that can easily change in the future though.

We could potentially look at initial_db_version to set the capabilities when registering the taxonomy to preserve old behavior on new installs, but that may be more confusing than it's worth.

@glendaviesnz
Copy link
Author

A decision has been made to not include this change in Gutenberg or 6.4. The GB change was reverted here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants