WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 2 days ago

#47443 new defect (bug)

REST-API prevents users with edit_published_posts capability updating published posts

Reported by: derweili Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.2.1
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

When a user has edit_posts and edit_published_posts capabilities but not publish_posts capability and they edit a published post they they get following error:

'Sorry, you are not allowed to publish posts in this post type.'

Because the Block Editor relies on the REST-API, you can see this behavior in the Gutenberg Editor as well.
In Gutenberg they don't see the above error. Instead you see a "Submit for Review" button instead of an Update Button.

To Reproduce

  1. Create a user with edit_pages and edit_published_pages capabilities but not publish_pages capability
  2. Login as that user and edit a published page in the Classic Editor
  3. See that the primary action button is "Update"
  4. Switch to the Block Editor and see that the primary action button is "Submit for Review"

I think there are two changes that need to be done:

1. in wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php:1825

The if statement should be changed to:

<?php
if ( 'attachment' !== $this->post_type && ( ( 'publish' == $post->post_status && current_user_can( $post_type->cap->edit_published_posts ) ) || current_user_can( $post_type->cap->publish_posts ) )  ) {

After this first change you will have the "Update" Button back in the editor, but you still can't update the post. You will receive the above Sorry, you are not allowed to publish posts in this post type. answer from the REST-API. A additional change must be done:

2. in wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php:1148

The if statement should be changed to

<?php
if ( ! current_user_can( $post_type->cap->publish_posts ) && ! current_user_can( $post_type->cap->edit_published_posts ) ) {
    return new WP_Error( 'rest_cannot_publish', __( 'Sorry, you are not allowed to publish posts in this post type.' ), array( 'status' => rest_authorization_required_code() ) );
}

Github Issue for Gutenberg: https://github.com/WordPress/gutenberg/issues/13342

I am concerned about introducing security risk with this changes.

Attachments (3)

47443.diff (1.3 KB) - added by derweili 17 months ago.
Patch V1
47443.2.diff (4.1 KB) - added by apieschel 9 months ago.
slight change with unit test
47443.3.diff (3.8 KB) - added by apieschel 9 months ago.
reverts to first proposed solution, but keeps unit test

Download all attachments as: .zip

Change History (36)

@derweili
17 months ago

Patch V1

#1 @derweili
17 months ago

  • Keywords has-patch added; needs-patch removed

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


16 months ago

This ticket was mentioned in Slack in #docs by pbiron. View the logs.


16 months ago

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


16 months ago

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


12 months ago

#6 @kadamwhite
12 months ago

  • Milestone changed from Awaiting Review to 5.4

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


10 months ago

#8 @peterwilsoncc
10 months ago

  • Keywords needs-unit-tests added

@derweili

Hello and welcome to trac and thank you for the ticket.

As this modifies permissions, I've added the needs-unit-tests keyword to the ticket; they'll be handy for both the pass and failure use case.

If you've written unit tests in the past it would be great if you could help out but there are plenty of other contributors with experience writing tests so don't worry if you haven't got time or need some assistance to help out.

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


9 months ago

@apieschel
9 months ago

slight change with unit test

#10 @apieschel
9 months ago

Uploaded a new patch with a slight modification to the switch statement in wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php. I found that it was necessary to distinguish between the "publish" and "future" cases in order for all previous unit tests to pass. Otherwise, test_create_post_publish_without_permission() (line 2303 in tests/phpunit/tests/rest-api/rest-posts-controller.php) fails. In this scenario with the previous patch, the role would still be allowed to publish posts, even though it loses the "publish_posts" capability, because it still retains the "edit_published_posts" capability. Also added a unit test.

Last edited 9 months ago by apieschel (previous) (diff)

@apieschel
9 months ago

reverts to first proposed solution, but keeps unit test

#11 @apieschel
9 months ago

Nevermind, thinking it over again, since "future" means "to be published in the future" it doesn't really make sense to have different permissions for the "publish" and "future" cases. The original edit to wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php is better. Added a new patch that reverts the change to this file, but keeps my new unit test.

But is it a problem that the patch would allow a user to publish posts via the Rest API, even if they only had the "edit_published_posts" capability? This seems tricky. If it's not a problem, then I could simply edit the unit test (test_create_post_publish_without_permission) that's failing. It would pass if I removed the "edit_published_posts" capability in addition to the "publish_posts" capability. What is the best practice for adjusting previous unit tests in a case like this?

Last edited 9 months ago by apieschel (previous) (diff)

#12 @peterwilsoncc
9 months ago

  • Keywords needs-refresh added

@kadamwhite asked me to take a quick look at this with regard to the permissions and unit test changes.

@apieschel
In terms of modifying existing tests, it's best to only modify them if the existing assertions are wrong. Otherwise they should remain unchanged.

The assertion in test_create_post_publish_without_permission() looks correct to me, in that the user is having the publish_posts permission removed, which prevents them from creating a published post.

This suggests the permissions change needs some work to ensure it works as expected. Accordingly I've labeled this as needing a refresh.

#13 @peterwilsoncc
8 months ago

I've been turning this over in my head.

Am I correct in thinking the WP_REST_Posts_Controller::handle_status_param() function is used for both creating and updating post objects?

If that's the case, I think that's the underlying problem. For updates the post ID needs to be included when determining the permitted statuses so the correct meta capabilities are calculated for the individual post.

#14 @TimothyBlynJacobs
8 months ago

  • Milestone changed from 5.4 to 5.5

Am I correct in thinking the WP_REST_Posts_Controller::handle_status_param() function is used for both creating and updating post objects?

That's correct.

If that's the case, I think that's the underlying problem. For updates the post ID needs to be included when determining the permitted statuses so the correct meta capabilities are calculated for the individual post.

So when we are editing an existing post, we'd use current_user_can( 'publish_post', $id )?


Given the complexity of this, going to punt to 5.5 for now.

#15 @kadamwhite
7 months ago

@peterwilsoncc tagging you to make sure you've seen and have the opportunity to respond to Timothy's reply above

#16 @peterwilsoncc
7 months ago

@kadamwhite @TimothyBlynJacobs Sorry, I'd missed the comment.

Yes, when editing an existing post the correct check is the meta capability; current_user_can( 'publish_post', $id ) for the generic post type post.

You'll need to fork the function for existing and new posts, in terms of code I think you'll want the following

<?php

// EXISTING POST

switch ( $post_status ) {
        case 'draft':
        case 'pending':
                if ( ! current_user_can( $post_type->cap->edit_post, $post_id ) ) {
                        // Error
                }
                break;
        case 'publish':
        case 'future':
        case 'private':
                if ( ! current_user_can( 'publish_post', $post_id ) ) {
                        // Error (same meta cap string for all post types).
                }
                break;
        default:
                // I'm really not sure what this is for, sorry. Custom statuses????
}


// NEW POST

switch ( $post_status ) {
        case 'draft':
        case 'pending':
                if ( ! current_user_can( $post_type->cap->edit_posts ) ) {
                        // Error
                }
                break;
        case 'publish':
        case 'future':
        case 'private':
        default:
                // Unchanged
                break
}

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


4 months ago

#18 @peterwilsoncc
4 months ago

Following up my comment above, the first permission check for editing an existing post should be current_user_can( 'edit_post', $post_id ) -- the meta capability checks in the API were recently modified to match those in core for edit_post, delete_post and read_post.

See #50128, [47850]

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


3 months ago

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


3 months ago

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


3 months ago

#22 @davidbaumwald
3 months ago

  • Milestone changed from 5.5 to Future Release

This was discussed during today's scrub. With 5.5 RC1 approaching, this is being moved to Future Release. If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, please update the milestone accordingly.

This ticket was mentioned in PR #532 on WordPress/wordpress-develop by derweili.


6 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests needs-refresh removed

#24 @derweili
6 weeks ago

I modified the mentioned handle_status_param method to first check if we are editing an existing post or a new post.
Secondly, if we are editing an existing post there is an additional check for the current status when the requested status is future or publish

So basically the the approach from @peterwilsoncc with another check for the current status.

The unit tests now have no more errors.

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


6 weeks ago

#26 @prbot
6 weeks ago

TimothyBJacobs commented on PR #532:

@derweili can you also rebase this against the latest master?

This ticket was mentioned in PR #538 on WordPress/wordpress-develop by derweili.


6 weeks ago

A new try for:
Fixes: https://core.trac.wordpress.org/ticket/47443
Closes #532

Rebased to Master, implemented @TimothyBJacobs recommended changes.

#28 @prbot
6 weeks ago

derweili commented on PR #532:

I implemented all recommended changes in the newly created PR #538

#29 @prbot
6 weeks ago

TimothyBJacobs commented on PR #538:

Thanks for rebasing @derweili! I noticed this PR doesn't have the changes to get_available_actions. I think the publish action for that still needs to be updated to pass the post id to the permission check?

#30 @prbot
5 weeks ago

derweili commented on PR #538:

@TimothyBJacobs I modified the get_available_actions to check if the post has a published or future status and if the user has edit_published_posts capability.

I did not add the $post_id check to this function because none of the other checks in this function use meta-capabilities to check if a specific post (post_id) can be edited by a user.

#31 @prbot
3 days ago

TimothyBJacobs commented on PR #538:

Hm. Have you tested this with Gutenberg? Gutenberg uses those available actions to build the proper submit buttons for the user. So I think those checks need to take the current post id into account. It not doing that currently is probably part of this bug.

#32 @TimothyBlynJacobs
3 days ago

#48764 was marked as a duplicate.

#33 @TimothyBlynJacobs
2 days ago

#47679 was marked as a duplicate.

Note: See TracTickets for help on using tickets.