Make WordPress Core

Opened 9 years ago

Last modified 5 years ago

#30452 new defect (bug)

_wp_translate_postdata() redundantly+destructively checks current_user_can( $ptype->cap->edit_others_posts ) on update

Reported by: danielbachhuber's profile danielbachhuber Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: dev-feedback
Focuses: Cc:

Description

The product use-case is that the creator of a post should always be able to edit the post, even when they've assigned someone else as a byline. To do so, I'm storing their user ID to post meta:

public function action_wp_insert_post_persist_author( $post_id, $post, $update ) {
	$post_obj = Post::get_by_post_id( $post_id );
	if ( $update
		|| ! in_array( $post->post_type, Fusion()->get_post_types() )
		|| ! $post_obj
		|| ! $post->post_author ) {
		return;
	}
	$post_obj->set_first_author_id( $post->post_author );
}

And then filtering map_meta_cap:

/**
 * Filter map meta cap to do whatever custom caps we need
 */
public function filter_map_meta_cap( $caps, $cap, $user_id, $args ) {

	switch ( $cap ) {
		case 'edit_post':

			$post_obj = Post::get_by_post_id( $args[0] );
			if ( ! $post_obj ) {
				break;
			}

			$post_type = get_post_type_object( $post_obj->get_post_type() );

			// Allow first authors to always edit the post
			if ( $post_obj->get_first_author_id() && $user_id == $post_obj->get_first_author_id() ) {
				// Don't require editing others' posts
				if ( false !== ( $key = array_search( $post_type->cap->edit_others_posts, $caps ) ) ) {
					unset( $caps[ $key ] );
				}
				// If the post is published...
				if ( 'publish' == $post_obj->get_status() ) {
					$caps[] = $post_type->cap->edit_published_posts;
				} elseif ( 'trash' == $post_obj->get_status() ) {
					if ( 'publish' == get_post_meta( $post_obj->get_id(), '_wp_trash_meta_status', true ) ) {
						$caps[] = $post_type->cap->edit_published_posts;
					}
				} else {
					// If the post is draft...
					$caps[] = $post_type->cap->edit_posts;
				}
			}
			break;

	}
	return $caps;
}

This approach generally works — except the original author isn't able to save an update to a post because _wp_translate_postdata() aggressively checks current_user_can( $ptype->cap->edit_others_posts ) (ref).

A check for current_user_can( $ptype->cap->edit_post, $post_data['ID'] ) was added to _wp_translate_postdata() in r22950, but the changeset also leaves in the edit_others_posts check.

Given current_user_can( 'edit_post' ) falls back to edit_others_posts behind the scenes (ref), I'd expect we could remove the second check.

Change History (5)

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


9 years ago

#2 follow-up: @jeremyclarke
9 years ago

+1 this bug breaks my (IMHO legit) filter on map_meta_cap to allow anyone subscribed via. Edit Flow editing rights to a post.

Daniel do you have a solution to make the filter work in the meantime?

#3 in reply to: ↑ 2 @danielbachhuber
9 years ago

Replying to jeremyclarke:

+1 this bug breaks my (IMHO legit) filter on map_meta_cap to allow anyone subscribed via. Edit Flow editing rights to a post.

Daniel do you have a solution to make the filter work in the meantime?

Here's the code we're using internally with an even more hacky fix: https://gist.github.com/danielbachhuber/18850d571c5dce419f8b

It's not something I can support, but feel free to use as you wish.

#4 @jeremyclarke
9 years ago

Okay after further investigation I now understand Daniel's original comment and can say I agree completely with his judgement. Since this apparently hasn't gained any traction I'll take a stab at re-explaning the problem to emphasize that the second check is probably unnecessary and completely destroys the usefulness of the edit_posts map_meta_cap system.

The check for edit_posts works great and does what we need

_wp_translate_postdata essentially starts with a check for whether the current user is allowed to edit the post in question:

if ( $update && ! current_user_can( 'edit_post', $post_data['ID'] ) ) {...}

This will run a barrage of permissions tests depending on context. Most importantly for this bug, edit_post includes a map_meta_cap for edit_others_posts which is run as-necessary if the editing user is not the "author" of the post.

This is great! One check with tons of intelligence behind it that covers lots of ground. Even better, the map_meta_cap system allows plugins (like Edit Flow) to filter the outcome with the context of the author_id and post_id. In the case of Daniel and me this lets us delegate conditional edit_others_posts to author/contributor accounts if they meet some criteria in relation to particular posts (for me it's if they are subscribed to notifications on the post in Edit Flow).

The second check for edit_others_posts is useless and breaks map_meta_cap

For some reason (I can imagine many causes as this code matured) the author testing algorithm from the edit_post is duplicated in _wp_translate_postdata including testing if current user is the author. This is unnecessary because the test would have already been done in edit_posts.

Just looking at the function you can see the many hints that the two stanzas are redundant: They both run current_user_can, they both branch for post_type=page and they both have 4 instances of the same error message ('You are not allowed to create posts as this user.')

The big problem is that this second check provides no context for the edit_others_posts test:

if ( isset( $post_data['user_ID'] ) 
&& ( $post_data['post_author'] != $post_data['user_ID'] ) 
&& ! current_user_can( $ptype->cap->edit_others_posts ) ) {

This means that if a user doesn't explicitly have edit_others_posts they won't be able to save the post regardless of how the map_meta_cap value is filtered.

The worst part is that it makes the UI horribly inconsistent

This bug affects _wp_translate_postdata(), but that function isn't run ever time the map_meta_cap for the edit_post capability is checked, and it seems that in all the other cases map_meta_cap works correctly, this means some things work and others don't which is super confusing and can cause all kinds of bugs.

Specifically, a user given conditional edit_posts privileges for a post is able to log in, view the post in the editor, change it's content and push the "Update" button to reload the page, but when they do so they get an error and all their changes are lost because saving the post triggers _wp_translate_postdata() which fails. This is totally unnacceptable, since it results in lost data and a frustration we can all relate to at having to rewrite your changes (potentially multiple times before you figure out that it's never going to work).

It also means that many people trying to use map_meta_cap this way are likely to think it works when it really doesn't. We should be able to assume that if a user can see a post's edit screen they can edit it, but this bug completely breaks that logic. I found at least one other dev while searching who, like me, thought it was working and only realized later this bug was breaking things.

At bare minimum the check for edit_others_posts should be replaced with a second edit_posts + post_id check

I suspect that whole second stanza can be removed by reworking the function, but I don't know the effect of the post_author_override section between the two capability checks, so maybe we still need to to the second check.

If so it should still be changed to match the first one:

current_user_can( 'edit_post', $post_data['ID'] )

This will have the desired effect of testing the case where the editing user is not the post author while still honoring the map_meta_cap filter.


#5 @jeremyclarke
9 years ago

  • Summary changed from _wp_translate_postdata() aggressively checks current_user_can( $ptype->cap->edit_others_posts ) on update to _wp_translate_postdata() redundantly+destructively checks current_user_can( $ptype->cap->edit_others_posts ) on update
Note: See TracTickets for help on using tickets.