[wp-trac] [WordPress Trac] #30452: _wp_translate_postdata() aggressively checks current_user_can( $ptype->cap->edit_others_posts ) on update

[WordPress Trac]

#30452: _wp_translate_postdata() aggressively checks current_user_can(
$ptype->cap->edit_others_posts ) on update
-------------------------------+-----------------------------
 Reporter:  danielbachhuber    |       Owner:
     Type:  defect (bug)       |      Status:  new
 Priority:  normal             |   Milestone:  Future Release
Component:  Posts, Post Types  |     Version:
 Severity:  normal             |  Resolution:
 Keywords:  dev-feedback       |     Focuses:
-------------------------------+-----------------------------

Comment (by jeremyclarke):

 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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/30452#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform