REST-API prevents users with edit_published_posts capability updating published posts

[derweili]

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() ) );
}

Related

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

I am concerned about introducing security risk with this changes.

[derweili]

Patch V1

[peterwilsoncc]

@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.

[apieschel]

slight change with unit test

[apieschel]

Uploaded a 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 new unit test.

[apieschel]

reverts to first proposed solution, but keeps unit test

[apieschel]

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?

[peterwilsoncc]

@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.

[peterwilsoncc]

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.

[TimothyBlynJacobs]

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.

[kadamwhite]

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

[peterwilsoncc]

@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
}

[peterwilsoncc]

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]

[davidbaumwald]

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.

Fixes: https://core.trac.wordpress.org/ticket/47443

Trac ticket:

[derweili]

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.

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

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

Rebased to Master, implemented @TimothyBJacobs recommended changes.

I implemented all recommended changes in the newly created 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?

@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.

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.

[TimothyBlynJacobs]

#48764 was marked as a duplicate.

[TimothyBlynJacobs]

#47679 was marked as a duplicate.

A new try to fix trac ticket 47443

• Added a function to compare the current and new post status when editing existing posts and checking for users edit_published_posts capability.
• Added a function to check for current post status and user edit_post capability when setting publish post action links

Added unit tests for both changes:

• Unit Test for "editing published posts without publish post capability"
• Unit Test to ensure "publish action link exists for user without publish post capability"

Trac ticket: https://core.trac.wordpress.org/ticket/47443

Closes #538

[derweili]

@TimothyBlynJacobs I created a new PR on GitHub addressing this ticket.

As requested in the previous PRs, I created unit tests and additionally tested the behavior manually with the Block Editor.

I also changed the behavior of the action links so they check for the current post_id. This was missing in the previous PR. I also created a Unit Test for this action link behavior.