Opened 3 years ago
Last modified 4 weeks ago
#54516 assigned defect (bug)
Full site editing/REST-API: modify permission checks to use post type.
Reported by: | 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 (52)
This ticket was mentioned in PR #1950 on WordPress/wordpress-develop by peterwilsoncc.
3 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
This ticket was mentioned in PR #1955 on WordPress/wordpress-develop by spacedmonkey.
3 years ago
#3
Trac ticket: https://core.trac.wordpress.org/ticket/54516
#4
@
3 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.
3 years ago
3 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:
3 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
@
3 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
@
3 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:
3 years ago
#10
@spacedmonkey I've resolved a merged conflict in a292d82ffa25fe5b98fb3f908b6c6a64025835b1, are you able to double check that everything is dandy.
#11
@
3 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
@
3 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.
3 years ago
spacedmonkey commented on PR #1955:
3 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.
3 years ago
hellofromtonya commented on PR #1955:
3 years ago
#16
@spacedmonkey Tests look good 👍
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
3 years ago
hellofromtonya commented on PR #1955:
3 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:
3 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:
3 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:
3 years ago
#21
Alrighty, I see the hotfix that restores the permissions check for the theme. Retesting.
#23
@
3 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:
3 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.
3 years ago
This ticket was mentioned in PR #2026 on WordPress/wordpress-develop by spacedmonkey.
3 years ago
#26
Trac ticket: https://core.trac.wordpress.org/ticket/54516
This ticket was mentioned in Slack in #core by spacedmonkey. View the logs.
3 years ago
#29
@
3 years ago
- Keywords needs-patch added; has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for the PR 2026.
peterwilsoncc commented on PR #1955:
3 years ago
#30
Committed in https://core.trac.wordpress.org/changeset/52342
This ticket was mentioned in Slack in #core-restapi by hellofromtonya. View the logs.
3 years ago
#33
follow-up:
↓ 36
@
3 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
@
3 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.
3 years ago
#35
- Keywords has-patch added; needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/54516
#36
in reply to:
↑ 33
@
3 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 themap_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
@
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
@
2 years 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 toedit_template
rather thanedit_post
. This may not be a problem if it is understood thatedit_post
should not be used for templates, but again, thoughts are welcome.
#40
@
2 years 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.
#42
@
21 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
@
20 months ago
- Milestone changed from 6.2 to 6.3
Punting as we have no progress on this ticket.
#44
@
16 months ago
- Milestone changed from 6.3 to 6.4
- Owner spacedmonkey deleted
- Status changed from reopened to assigned
This ticket was mentioned in Slack in #core by chaion07. View the logs.
8 months ago
#47
@
8 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
@
8 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
@
8 months ago
Just notifying there has recently been some related work on the permission checks at https://github.com/WordPress/gutenberg/pull/58301
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.