Make WordPress Core

Opened 19 months ago

Last modified 4 months ago

#54516 reopened task (blessed)

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

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.9
Component: REST API Keywords: has-patch needs-unit-tests needs-testing
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 (43)

#1 @peterwilsoncc
19 months 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.


19 months ago
#2

  • 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
19 months 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.


19 months ago

adamziel commented on PR #1955:


19 months ago
#6

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

peterwilsoncc commented on PR #1950:


18 months ago
#7

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

#8 @peterwilsoncc
18 months 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
18 months 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.

peterwilsoncc commented on PR #1955:


18 months ago
#10

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

#11 @peterwilsoncc
18 months 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
18 months 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.


18 months ago

spacedmonkey commented on PR #1955:


18 months ago
#14

I need to add some unit tests to this.

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


18 months ago

hellofromtonya commented on PR #1955:


18 months ago
#16

@spacedmonkey Tests look good 👍

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


18 months ago

hellofromtonya commented on PR #1955:


18 months ago
#18

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

hellofromtonya commented on PR #1955:


18 months ago
#19

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

hellofromtonya commented on PR #1955:


18 months ago
#20

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

hellofromtonya commented on PR #1955:


18 months ago
#21

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

#22 @spacedmonkey
18 months ago

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

#23 @spacedmonkey
18 months 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?

TimothyBJacobs commented on PR #1955:


18 months ago
#24

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.


18 months ago

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


18 months ago

#28 @spacedmonkey
18 months 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
18 months 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.


18 months ago

#32 @spacedmonkey
18 months ago

Also reported here.

#33 follow-up: @TimothyBlynJacobs
18 months 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
18 months 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.

This ticket was mentioned in PR #2342 on WordPress/wordpress-develop by carlomanf.


16 months ago
#35

  • Keywords has-patch added; needs-patch removed

#36 in reply to: ↑ 33 @manfcarlo
16 months ago

Replying to TimothyBlynJacobs:

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.

Here is a suggested implementation. There appear to be two test failures, one is just stating that the new meta capabilities don't have their own tests yet, the other I believe is happening because the permission check is failing when the delete method is called on a non-existing template. Not sure what is the best way to handle that.

Another limitation I can see is the example code in this issue would still not work, due to the map_meta_cap filter ultimately being applied to edit_template rather than edit_post. This may not be a problem if it is understood that edit_post should not be used for templates, but again, thoughts are welcome.

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


13 months ago

#38 @costdev
13 months ago

  • Milestone changed from 6.0 to 6.1

Per the discussion in the bug scrub, I'm moving this ticket to the 6.1 milestone.

#39 @manfcarlo
11 months ago

Updated the pull request based on discussions happening at https://github.com/WordPress/gutenberg/issues/27597

I expect the update solves this problem:

Another limitation I can see is the example code in this issue would still not work, due to the map_meta_cap filter ultimately being applied to edit_template rather than edit_post. This may not be a problem if it is understood that edit_post should not be used for templates, but again, thoughts are welcome.

#40 @manfcarlo
9 months ago

I noticed that the bug scrubs are beginning for 6.1, so I thought it would help to give a summary of the current status.

  • The ticket was originally opened for 5.9 and has already been postponed twice to 6.0 and 6.1. It would be disappointing for it to be postponed again, since full-site editing is considered a major focus at the moment.
  • The last time the ticket was postponed, it was done so on the basis that the patch had test failures. The test failures are trivial ones and should be possible to resolve very quickly.
  • Since the last postponement of the ticket, a core committer @peterwilsoncc reviewed the patch and identified only one major issue. It was essentially the same issue I identified myself when submitting the patch. I ended up changing the patch and I believe the issue has been resolved. No one has reviewed the patch since I made the change.
  • I am biased but I believe the patch in its current state is near enough to be ready in time for 6.1, with enough eyes on it.

#41 @spacedmonkey
8 months ago

  • Milestone changed from 6.1 to 6.2

Punting, as no movement on this.

#42 @manfcarlo
5 months ago

  • Keywords needs-unit-tests needs-testing added

Adding some keywords with the hope this will get addressed in the current cycle. The previous summary is still current.

#43 @spacedmonkey
4 months ago

  • Milestone changed from 6.2 to 6.3

Punting as we have no progress on this ticket.

Note: See TracTickets for help on using tickets.