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

Proposal: robust permissions system for FSE theme templates #27597

Closed
Tracked by #41241
carlomanf opened this issue Dec 9, 2020 · 19 comments
Closed
Tracked by #41241

Proposal: robust permissions system for FSE theme templates #27597

carlomanf opened this issue Dec 9, 2020 · 19 comments
Assignees
Labels
[Status] In Progress Tracking issues with work in progress

Comments

@carlomanf
Copy link

Is your feature request related to a problem? Please describe.
The usual post type capabilities (edit_posts, publish_posts etc) will not work for FSE theme templates. If you grant someone the permission to edit and publish their own posts, they can not affect anyone else's posts. If you do the same with templates, they would be able to publish a single template where a singular already exists, and cause drastic changes to much of the site.

However, it still makes sense that you might want to let someone control the templates for a particular post or a particular post type, without being able to affect the rest of the site.

Describe the solution you'd like
The idea is the system would recognise capabilities such as edit_single_templates or delete_others_archive_templates etc, with keys from the template hierarchy inserted into the usual post type capability patterns.

For example, if a user has the capability edit_page-$id_templates, they can submit their own template for a particular page, but it will need to be approved by an admin unless they also have the publish capability. If a user has the capabilities edit_single-$posttype_templates, edit_published_single-$posttype_templates and publish_single-$posttype_templates, they will have full and independent control over the templates for that post type (including single-$posttype-$slug) but not any other post types.

Note that they would be automatically granted permission for more specific templates than the one specified in the capability. This means edit_index_templates is effectively the same as edit_templates, delete_others_index_templates is the same as delete_others_templates etc.

By default, the only capabilities granted would be the general ones to admin roles. The more specific capabilities would just be available for optional use.

Describe alternatives you've considered
I've already written some exploratory code to test out this approach, and it looks viable to me as long as there is agreement that it's the best approach. I'm open to hearing alternative approaches.

@mtias
Copy link
Member

mtias commented Jan 18, 2021

Also partially related: #27941

@carlomanf
Copy link
Author

I am seeing this issue being referenced a lot, so I thought I should give an update. These days, I think the best way to achieve it is through this ticket: https://core.trac.wordpress.org/ticket/54516

What this will do is enable plugins to easily implement the solution described in this issue, using familiar PHP filters.

@peterwilsoncc
Copy link
Contributor

This is great! It will be especially useful for the bigger enterprise type sites.

The current_user_can() function can accept as many or few arguments required for checking meta capabilities.

In most cases the meta capability is something like current_user_can( edit_template, $post_id ) but it would be possible to expand it to include items like post types, etc: either with current_user_can( edit_template_for_post_type, $post_type ) or current_user_can( edit_template_for_post_type, $post_id, $post_type ) if you wanted to be really verbose.

As a rule, I try to keep the primitive capabilities with a plural name (edit_templates) and the meta capabilities with a singular (edit_template) -- the former is for editing, creating, or deleting all the objects; the latter is for editing, creating or deleting or single object.

I know way too much about roles and capabilities, so let me know if you need anything clarified or other assistance.

@carlomanf
Copy link
Author

@peterwilsoncc If you are interested to review the open pull request WordPress/wordpress-develop#2342 I would much appreciate it. If that one (or an equivalent) can be merged, I would consider closing this issue, because plugins would be able to handle the rest.

@peterwilsoncc
Copy link
Contributor

@carlomanf I'm happy to do so but have run out of time today. I am on leave next week so will take a look once I return in the week starting June 6, 2022.

@mtias
Copy link
Member

mtias commented May 27, 2022

This would be useful as well for #37943.

@peterwilsoncc
Copy link
Contributor

@carlomanf I've taken a look at WordPress/wordpress-develop#2342 but think there are a few architectural things to work out before this is ready for core.

Rather than discussing these on the pull request I think it's better to discuss them here or on WP#53131 to avoid multiple threads.

At the moment the WordPress capabilities use edit_post, read_post and delete_post for the meta capabilities for all post types (see WP#50128) so it would be best to maintain this practice if at all possible.

As get_block_template() may not exist when it falls back to get_block_file_template() I am still trying to figure out how this might be achieved. I'll share some thoughts once I have something more considered to propose.

@carlomanf
Copy link
Author

At the moment the WordPress capabilities use edit_post, read_post and delete_post for the meta capabilities for all post types (see WP#50128) so it would be best to maintain this practice if at all possible.

As get_block_template() may not exist when it falls back to get_block_file_template() I am still trying to figure out how this might be achieved. I'll share some thoughts once I have something more considered to propose.

What do you think of something like this:

case 'edit_template':
    if ( $template->wp_id ) {
        $caps = array_merge( $caps, map_meta_cap( 'edit_post', $user_id, $template->wp_id ) );
    }

This is a little bit similar to what happens with the privacy policy check.

@peterwilsoncc
Copy link
Contributor

@carlomanf Sorry for the delay, I was quietly pondering rather than completely ignoring this issue.

To further inform how permissions might be considered, I have another question.

  • A theme includes a default version of the template
  • As yet, no one has edited the template so it only exists in the theme's folder, eg 404.html
  • A user decides to edit the template, are they
    • editing the template, ie the action requires the edit_templates permission
    • creating the template, ie the action requires the create_templates permission
    • both
    • neither

The reason for my question is that it helps inform how the meta capabilities might work, eg current_user_can( 'edit_template', 'twentytwentytwo//404' ) or whatever the second part might be.

@carlomanf
Copy link
Author

@peterwilsoncc For the action you describe, they would need only the editing permission and not the create permission. This already had a strong precedent that the API endpoint used for the action is the editing one and not the create one, so I didn't need to spend much time deciding how to handle the permission.

Furthermore, the pull request actually denies the create permission in this case because the template ID is already taken.

Somewhat related, it must be decided what happens when the user tries to delete a template that doesn't exist. The pull request also denies the permission in this case, which is causing some of the test failures.

@peterwilsoncc
Copy link
Contributor

Thanks Carlo, I appreciate your patience as I try to understand similarities and differences between the existing roles and caps paradigm.

As the edit_post meta capability expects the second parameter to be a number but there isn't one available in this circumstance, I'll consider how to handle this and come back to you.

The meta capability will need to use the post type object's capability settings to determine if a user is allowed to do something and work for items that both have and do not have a wp_id.

@carlomanf
Copy link
Author

If the template exists but does not have a wp_id, then there is no post to be edited and edit_post doesn't apply. Perhaps in this case the capabilities should be merged with the post type object's create_posts capability setting, because it's the creation of a post but not the creation of the template.

carlomanf added a commit to carlomanf/wordpress-develop that referenced this issue Jun 28, 2022
Update capabilities.php based on discussions at WordPress/gutenberg#27597
@carlomanf
Copy link
Author

Updated WordPress/wordpress-develop#2342 based on our discussions here.

@carlomanf
Copy link
Author

@peterwilsoncc Did you get a chance to review the updates to the pull request? Do they resolve your concerns?

@carlomanf
Copy link
Author

This ticket should have been closed a long time ago. It was opened in 2020 when the FSE infrastructure was not yet stable and only describes the issue vaguely. Now, there exist other open tickets that more precisely describe the task ahead. This ticket is not tracked as part of the core release cycle, meaning that leaving it open diverts discussion away from core committers' attention.

@jeremyfelt
Copy link
Member

This might be pedantic of me, but closing this has marked the corresponding "task" as complete in the FSE roadmap, which may leave someone skimming progress on that page with the wrong impression.

I'm going to reopen, but feel free to tell me I'm wrong. 😄

I know some progress has been made on trac toward implementing more granular capability checks: https://core.trac.wordpress.org/ticket/54516

I believe work will still need to be done more directly in Gutenberg once those are implemented. Right now the error notices aren't very descriptive or consistent if you do find yourself hacking in a capabilities system. 😎

@jeremyfelt jeremyfelt reopened this Mar 16, 2023
@carlomanf
Copy link
Author

@jeremyfelt

I believe work will still need to be done more directly in Gutenberg once those are implemented. Right now the error notices aren't very descriptive or consistent if you do find yourself hacking in a capabilities system. 😎

I would argue that this issue doesn't cover the front-end work. It is really just an older version of the trac ticket from the "experimental" era. Most of the recent comments here should have been left on the trac ticket instead, because they exclusively reference back-end code that had already been removed from the plugin long ago.

Do you mind elaborating about the error notices you are seeing? Perhaps they would be covered by #37126 and/or #26573.

@carlomanf
Copy link
Author

Closing again if no objection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress
5 participants