WordPress.org

Make WordPress Core

Opened 8 weeks ago

Last modified 5 weeks ago

#54516 reopened task (blessed)

Full site editing/REST-API: modify permission checks to use post type.

Reported by: peterwilsoncc Owned by: spacedmonkey
Milestone: 6.0 Priority: normal
Severity: normal Version: trunk
Component: REST API Keywords: needs-patch
Focuses: rest-api Cc:

Description

The new wp_global_styles post type is registered to use edit_theme_options in the capability settings.

The WP_REST_Global_Styles_Controller class's permission checks methods use the capability in a hard coded form rather than using $post_type->cap->edit_posts, etc, for the primitives and edit_post, $post_id for the meta caps.

To allow theme and plugin developers to modify the capability used for editing global styles via a filter, it would be good to defer to the post types setting. At the moment, such code would cause a conflict between the permission checks in the API and those in wp_insert_post().

I'll put this on the 5.9 milestone for visibility as the endpoint was introduced during the current cycle.

Change History (34)

#1 @peterwilsoncc
8 weeks ago

It looks like the same applies to WP_REST_Templates_Controller -- it was added in the last cycle as part of the FSE infrastructure.

Based on my review, these are the only two end points using post types that this applies to. It would be helpful to have a logic check on this though.

This ticket was mentioned in PR #1950 on WordPress/wordpress-develop by peterwilsoncc.


8 weeks ago

  • Keywords has-patch added; needs-patch removed

Unlinked to the ticket while I work a few things out: mainly that I am not about to break everything.

cc @spacedmonkey @TimothyBJacobs @costdev

#4 @spacedmonkey
8 weeks ago

I have created my own patch at #1955, it is untested, but it copied from the post controller for the most part.

I think we should make this change in 5.9 cycle, if we change it later it would be a backward compatibility issue.

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


8 weeks ago

#6 @prbot
8 weeks ago

adamziel commented on PR #1955:

LGTM and tests well, although it would still be nice to run Gutenberg e2e tests against it.

#7 @prbot
7 weeks ago

peterwilsoncc commented on PR #1950:

Thanks @costdev I'll close this down in favor of Jonny's in #1955 as it seems much closer to completion.

#8 @peterwilsoncc
7 weeks ago

I've added some notes to PR1955 (@spacedmonkey's version) with a few change requests but the bot didn't pick them up.

#9 @hellofromTonya
7 weeks ago

  • Keywords needs-refresh added

Marking for refresh as there's a merge conflict with the PR.

There are open questions in the PR. As a reminder, 5.9 Beta 1 is in < 18.5 hours. Let's see if the PR can get to a committable state before Beta 1 release party starts.

#10 @prbot
7 weeks ago

peterwilsoncc commented on PR #1955:

@spacedmonkey I've resolved a merged conflict in a292d82ffa25fe5b98fb3f908b6c6a64025835b1, are you able to double check that everything is dandy.

#11 @peterwilsoncc
7 weeks ago

  • Keywords needs-refresh removed

I've resolved the merge conflict so removing the needs-refresh keyword. The open questions Tonya refers to are mine so unable to help answer them.

#12 @hellofromTonya
7 weeks ago

  • Type changed from defect (bug) to task (blessed)

As this was introduced in 5.9 and Beta 1 is happening within the hour, changing this to blessed task. But let's try to finish it before Beta 2.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


7 weeks ago

#14 @prbot
7 weeks ago

spacedmonkey commented on PR #1955:

I need to add some unit tests to this.

This ticket was mentioned in Slack in #core-restapi by hellofromtonya. View the logs.


6 weeks ago

#16 @prbot
6 weeks ago

hellofromtonya commented on PR #1955:

@spacedmonkey Tests look good 👍

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 weeks ago

#18 @prbot
6 weeks ago

hellofromtonya commented on PR #1955:

Testing: The PR is currently causing a WSOD in the Site Editor interface. Here's the error:

[07-Dec-2021 18:27:31 UTC] PHP Fatal error:  Uncaught Error: Call to undefined method WP_REST_Global_Styles_Controller::permissions_check() in /var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php:504
Stack trace:
#0 /var/www/src/wp-includes/rest-api/class-wp-rest-server.php(1108): WP_REST_Global_Styles_Controller->get_theme_item_permissions_check(Object(WP_REST_Request))
#1 /var/www/src/wp-includes/rest-api/class-wp-rest-server.php(988): WP_REST_Server->respond_to_request(Object(WP_REST_Request), '/wp/v2/global-s...', Array, NULL)
#2 /var/www/src/wp-includes/rest-api/class-wp-rest-server.php(414): WP_REST_Server->dispatch(Object(WP_REST_Request))
#3 /var/www/src/wp-includes/rest-api.php(386): WP_REST_Server->serve_request('/wp/v2/global-s...')
#4 /var/www/src/wp-includes/class-wp-hook.php(307): rest_api_loaded(Object(WP))
#5 /var/www/src/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters('', Array)
#6 /var/www/src/wp-includes/plugin.php(522): WP_Hook->do_action(Array)
#7 /var/www/src/wp-includes/class-wp.php(396 in /var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php on line 504

#19 @prbot
6 weeks ago

hellofromtonya commented on PR #1955:

The fatal error is happening the get_theme_item_permissions_check() method as it's calling the permissions_check() which was renamed I think to check_read_permission(). I assume this needs similar treatment that get_item_permissions_check()` got. Thoughts?

@spacedmonkey

#20 @prbot
6 weeks ago

hellofromtonya commented on PR #1955:

The fatal error is happening the get_theme_item_permissions_check() method as it's calling the permissions_check() which was renamed I think to check_read_permission(). I assume this needs similar treatment that get_item_permissions_check()` got. Thoughts?

@spacedmonkey

#21 @prbot
6 weeks ago

hellofromtonya commented on PR #1955:

Alrighty, I see the hotfix that restores the permissions check for the theme. Retesting.

#22 @spacedmonkey
6 weeks ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#23 @spacedmonkey
6 weeks ago

@peterwilsoncc Sorry I missed this feedback.

It looks like the same applies to WP_REST_Templates_Controller -- it was added in the last cycle as part of the FSE infrastructure.

Sadly WP_REST_Templates_Controller was added in WordPress 5.8, meaning making changes to these endpoints much harder. @TimothyBlynJacobs Do you think we can safely change these endpoints?

#24 @prbot
6 weeks ago

TimothyBJacobs commented on PR #1955:

As long as we aren't removing those old methods we can. You can see how we introduced granular capabilities for App Passwords in 5.7 for an example.

Moving the capability check should also be fine because it maps to the same capability, we're just giving developers more flexibility.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 weeks ago

This ticket was mentioned in Slack in #core by spacedmonkey. View the logs.


6 weeks ago

#28 @spacedmonkey
6 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 52342:

REST API: Improve permission handling in global style endpoint.

The new wp_global_styles post type is registered to use edit_theme_options in the capability settings. The WP_REST_Global_Styles_Controller class's permission checks methods use the capability in a hard coded form rather than looking up the capability via the post type object. Changing the permission callbacks to lookup capabilities via the post type object, allows theme and plugin developers to modify the capability used for editing global styles via a filter and these values to be respected via the Global Styles REST API.

Props Spacedmonkey, peterwilsoncc, hellofromTonya , antonvlasenko, TimothyBlynJacobs, costdev, zieladam.
Fixes #54516.

#29 @hellofromTonya
6 weeks ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the PR 2026.

This ticket was mentioned in Slack in #core-restapi by hellofromtonya. View the logs.


5 weeks ago

#32 @spacedmonkey
5 weeks ago

Also reported here.

#33 @TimothyBlynJacobs
5 weeks ago

I'd like to suggest we punt this ticket to 6.0. This code was introduced in 5.8 not 5.9 so I don't think this is a must solve in this release in general.

Additionally, there is a lot of complexity to handling this. Because templates are backed by both posts and files, the REST API controller needs to do a lot of additional logic checks to be able to determine whether a template can be edited based on it's type.

Instead, I think we should approach this in 6.0 by introducing specific meta capabilities like edit_template or similar that would handle whether this is a template backed by a file or by a post object in the permission handling itself. That way developers will have the full context available when utilizing the map_meta_cap and other filters.

Our REST API controllers can then perform logic like current_user_can( 'edit_template', 'twentytwentytwo//single' ) instead.

#34 @hellofromTonya
5 weeks ago

  • Milestone changed from 5.9 to 6.0

Given Timothy's comments about complexity and this being a problem introduced in 5.8, I agree. Moving the ongoing work in this ticket to 6.0.

Note: See TracTickets for help on using tickets.