WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 5 years ago

#24916 closed defect (bug) (fixed)

XML-RPC "wp_author_id" ignored when changing author to self

Reported by: redsweater Owned by: johnbillion
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.8
Component: XML-RPC Keywords: has-patch 2nd-opinion reporter-feedback
Focuses: Cc:

Description

When using the MetaWeblog mw_editPost method to change the author of an existing post from another user to the logged in user, the wp_author_id" field is not read from the content_struct because of a logic error in mw_editPost().

The logic error is in this line:

if ( isset($content_struct['wp_author_id']) && ($user->ID != $content_struct['wp_author_id']) )

Here it is assumed that if the supplied author ID is the same as the logged in user, then the supplied ID does not need to be read. However, this is not true when the post being edited is in fact not currently in the logged in user's authorship.

The attached patch fixes the problem and also clarifies the permissions test so that it will reject any effort to change the author ID either to another user's ID or from another user's ID, unless the logged in user has permission to editor others' posts.

Attachments (5)

FixMetaWeblogChangeAuthorToSelf.patch (1.7 KB) - added by redsweater 7 years ago.
Patches mw_editPost to correctly convey the supplied author ID even if it matches the logged in user's ID
TestMetaWeblogChangeAuthorToSelf.patch (2.1 KB) - added by redsweater 7 years ago.
Tests to confirm the fix to mw_editPost and to confirm the existing correct behavior of wp_editPost
24916.diff (3.9 KB) - added by markoheijnen 6 years ago.
24916.2.diff (3.9 KB) - added by markoheijnen 6 years ago.
Refreshed patch
24916.3.diff (3.9 KB) - added by DrewAPicture 6 years ago.

Download all attachments as: .zip

Change History (20)

@redsweater
7 years ago

Patches mw_editPost to correctly convey the supplied author ID even if it matches the logged in user's ID

@redsweater
7 years ago

Tests to confirm the fix to mw_editPost and to confirm the existing correct behavior of wp_editPost

#1 @redsweater
7 years ago

I confirmed that the existing behavior of wp_editPost is correct, because the author field for the content is implied as the logged-in user, and doesn't need to be mapped from the legacy MetaWeblog-style content struct to the internal $post_datapost_author? field.

The attached tests TestMetaWeblogChangeAuthorToSelf.patch not only confirm my fix to mw_editPost but also add a test to wp_editPost to confirm the existing correct functionality for the modern wp_editPost API.

#2 @markoheijnen
7 years ago

In general only use wp.* methods.

#3 @redsweater
7 years ago

markoheijnen - I understand that the wp.* methods are preferred, but as long as the older methods are present I assume there is not opposition to having them behave correctly?

For what it's worth, as far as I can tell even WordPress's official iOS app still uses metaWeblog.editPost. It doesn't look like it currently supports changing the author of a post but for example, if it tried to add that feature without updating to use wp.editPost, it would run into this failure.

#4 @markoheijnen
7 years ago

Yes, they should. You can also see it as they missing that feature.
If you want to have this support then you need to move to wp.editPost anyway. So you support WordPress 3.4 > instead of a possible 3.7.

I guess the mobile apps will move to wp.* methods in the near future.

#5 @markoheijnen
7 years ago

  • Owner set to markoheijnen
  • Status changed from new to assigned

@markoheijnen
6 years ago

#6 @markoheijnen
6 years ago

  • Milestone changed from Awaiting Review to 4.1

Refreshed the patch and cleanup the code a bit. Moving to 4.1. It's either commit the whole patch or add only the wp.editPost unit tests.

#7 @johnbillion
6 years ago

  • Keywords 4.2-early added
  • Milestone changed from 4.1 to Future Release

#8 @iseulde
6 years ago

  • Milestone changed from Future Release to 4.2

has-patch 4.2-early so moving to 4.2.

#9 @DrewAPicture
6 years ago

  • Keywords needs-refresh added

@markoheijnen: Any interest in pursuing a fix for this in 4.2? If so, the patch needs a refresh in class-wp-xmlrpc-server.php.

#10 @markoheijnen
6 years ago

I will check it out later today. Thanks for the reminder. Didn't saw that this was moved to 4.2

Last edited 6 years ago by markoheijnen (previous) (diff)

@markoheijnen
6 years ago

Refreshed patch

#11 @markoheijnen
6 years ago

  • Keywords needs-refresh removed

Little bit later then planned but here is the refreshed patch. After reading comments and code It still looks fine for me.

@DrewAPicture
6 years ago

#12 @DrewAPicture
6 years ago

  • Owner changed from markoheijnen to johnbillion
  • Status changed from assigned to reviewing

24916.3.diff applies and unit tests pass.

@johnbillion: What do you think? Should we get this in for 4.2 or push to 4.3-early?

#13 follow-up: @johnbillion
5 years ago

  • Keywords 2nd-opinion reporter-feedback added; 4.2-early removed

This change makes sense, but I'm struggling to understand why the two edit_others_* permissions checks are in place here. AFAICT they are redundant due to the if ( ! current_user_can( 'edit_post', $post_ID ) ) check near the beginning of wp_xmlrpc_server::mw_editPost().

Thoughts?

#14 in reply to: ↑ 13 @redsweater
5 years ago

Replying to johnbillion:

This change makes sense, but I'm struggling to understand why the two edit_others_* permissions checks are in place here. AFAICT they are redundant due to the if ( ! current_user_can( 'edit_post', $post_ID ) ) check near the beginning of wp_xmlrpc_server::mw_editPost().

Thoughts?

In my original patch I just took it for granted that code was correct (and the intent of this bug and patch is to avoid reaching that code in the case the current user is simply supplying their own author ID for their own post).

I assume that although it has been confirmed the user can edit the post, that doesn't necessarily give them the right to change the authorship on the post to e.g. another author. Consider a scenario where the post is currently in the current user's name, and they want to change the authorship to another person. In that case it makes sense that a higher level of permission than just "edit_post" should be required to proceed.

#15 @johnbillion
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 31983:

Correctly set the post author in wp_xmlrpc_server::mw_editPost() when the current user is not the author of the post.

Props redsweater, markoheijnen, DrewAPicture
Fixes #24916

Note: See TracTickets for help on using tickets.