Make WordPress Core

Opened 2 years ago

Last modified 2 months ago

#54516 assigned defect (bug)

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

Reported by: peterwilsoncc's profile peterwilsoncc Owned by:
Milestone: Future Release 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 (51)

#1 @peterwilsoncc
2 years 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.


2 years 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
2 years 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.


2 years ago

adamziel commented on PR #1955:


2 years ago
#6

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

peterwilsoncc commented on PR #1950:


2 years 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
2 years 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
2 years 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:


2 years ago
#10

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

#11 @peterwilsoncc
2 years 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
2 years 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.


2 years ago

spacedmonkey commented on PR #1955:


2 years 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.


2 years ago

hellofromtonya commented on PR #1955:


2 years ago
#16

@spacedmonkey Tests look good 👍

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


2 years ago

hellofromtonya commented on PR #1955:


2 years 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:


2 years 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:


2 years 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:


2 years ago
#21

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

#22 @spacedmonkey
2 years ago

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

#23 @spacedmonkey
2 years 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:


2 years 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.


2 years ago

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


2 years ago

#28 @spacedmonkey
2 years 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
2 years 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.


2 years ago

#32 @spacedmonkey
2 years ago

Also reported here.

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


2 years ago
#35

  • Keywords has-patch added; needs-patch removed

#36 in reply to: ↑ 33 @manfcarlo
2 years 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.


2 years ago

#38 @costdev
2 years 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
22 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
19 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
19 months ago

  • Milestone changed from 6.1 to 6.2

Punting, as no movement on this.

#42 @manfcarlo
16 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
15 months ago

  • Milestone changed from 6.2 to 6.3

Punting as we have no progress on this ticket.

#44 @spacedmonkey
10 months ago

  • Milestone changed from 6.3 to 6.4
  • Owner spacedmonkey deleted
  • Status changed from reopened to assigned

#45 @spacedmonkey
7 months ago

  • Milestone changed from 6.4 to 6.5

Punting because of no progress.

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


3 months ago

#47 @mukesh27
3 months ago

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

This ticket was discussed in today's scrub.

As it's a follow-up bug, it will need to be addressed. I update the type to bug instead of task.

What are the next steps for this ticket? @spacedmonkey @TimothyBlynJacobs, if you have any thoughts?

#48 @chaion07
3 months ago

Thanks @peterwilsoncc for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback we are changing the ticket type to bug since the original patch has already been shipped, and the ticket was reopened for follow-up work.

Cheers!

Props to @mukesh27

#49 @manfcarlo
3 months ago

Just notifying there has recently been some related work on the permission checks at https://github.com/WordPress/gutenberg/pull/58301

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


2 months ago

#51 @audrasjb
2 months ago

  • Milestone changed from 6.5 to Future Release

As per today's bugscrub:
As noted in the ticket, there is a related PR opened upstream on Gutenberg side.
Anyway, without noticeable movement during this cycle, let’s move this ticket to Future release.

Note: See TracTickets for help on using tickets.