WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 8 months ago

#17981 closed defect (bug) (fixed)

XML-RPC wp.getComments should work for non-admins

Reported by: koke Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.2
Component: XML-RPC Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

Right now, if the caller doesn't have the moderate_comments permission, the XML-RPC call returns a 401 error.

A more graceful alternative would be to return the approved comments. The user may not be able to moderate, but still should be able to read/reply

Attachments (7)

patch-core-17981.diff (1.9 KB) - added by koke 5 years ago.
patch-core-17981-2.diff (1.9 KB) - added by koke 5 years ago.
Second patch closer to dashboard behavior
commentAPIfixes.patch (3.9 KB) - added by nprasath002 4 years ago.
Fixes cap checks
patch-gp-157-strings-3.diff (3.8 KB) - added by koke 4 years ago.
patch-core-17981-3.diff (3.1 KB) - added by koke 4 years ago.
17981-refresh.diff (3.5 KB) - added by ericmann 4 years ago.
Refresh patch to match latest version of trunk.
patch-core-17981-ut.diff (6.6 KB) - added by koke 4 years ago.
Unit Tests for wp.getComments and wp.editComment

Download all attachments as: .zip

Change History (28)

#1 @koke
5 years ago

  • Keywords has-patch added; needs-patch removed

Added patch for wp.getComments and wp.getComment. If user can't moderate comments, in only returns approved comments for wp.getComments, and returns an error if comment isn't approved for wp.getComment

@koke
5 years ago

Second patch closer to dashboard behavior

#2 @koke
5 years ago

I should have done a trac search before writing a second patch but I feel it's better ;)

Dashboard also shows unapproved comments to authors, so we check for (edit_posts OR moderate_comments) instead of just moderate_comments. The current checks for edit_comment should take care of the rest.

Also, added better error descriptions.

#3 @koke
5 years ago

  • Cc jbernal@… added

#4 @Anton Torvald
5 years ago

Added patch for wp.getComments and wp.getComment

Version 0, edited 5 years ago by Anton Torvald (next)

#6 @daniloercoli
4 years ago

  • Cc ercoli@… added

#7 @josephscott
4 years ago

Have you compared this with #19916 yet?

@nprasath002
4 years ago

Fixes cap checks

#8 @nprasath002
4 years ago

The patch fixes the issues in cap checks.
Also added validation for post_id

#9 @koke
4 years ago

Added a new patch (sorry about the wrong one). Tested with an Author user:

  • wp.getComments shows every comment (as dashboard does)
  • Added a new field 'can_edit' to show if the user has permission to edit/delete that specific comment. That'll allow to customize the UI for it
  • Can edit/delete comments in own posts
  • Can't edit/delete comments in others posts

I think this also solves #19916. Some questions about it:

  • Should wp.deleteComment error be "You are not allowed to delete..." instead of moderate?
  • I was going to change the error in wp.getComments to match wp.getComment, but it's a different error code (401 and 403) and could break something

@ericmann
4 years ago

Refresh patch to match latest version of trunk.

#10 @ericmann
4 years ago

  • Keywords commit added

#11 @westi
4 years ago

Before we commit these changes I would like us to have some unit tests around the expected behaviour here.

Highest priority are tests for all the cap checks to ensure we have expected behaviour - writing the tests out makes you think a lot about this :)

Second priority are tests for the data returned especially datatypes so we can ensure continuing consistency over time.

I asked koke to see if he could write some of these in IRC today.

@koke
4 years ago

Unit Tests for wp.getComments and wp.editComment

#12 @c3mdigital
3 years ago

  • Keywords needs-testing needs-refresh added; commit removed

#13 @jeremyfelt
3 years ago

  • Keywords mobile removed

#14 follow-up: @markoheijnen
3 years ago

Not sure why you remove the mobile keyword. It's used by the mobile team.

#15 in reply to: ↑ 14 ; follow-up: @dd32
3 years ago

  • Keywords mobile added

Replying to markoheijnen:

Not sure why you remove the mobile keyword. It's used by the mobile team.

Since it's not a 'supported' keyword in the drop down, non-privledged users can't set it, so I expect it's probably getting removed without the users intention.

#16 in reply to: ↑ 15 ; follow-up: @nacin
3 years ago

I'm fine with this patch — anything to make it more like how edit-comments.php actually works.

Replying to dd32:

Since it's not a 'supported' keyword in the drop down, non-privledged users can't set it, so I expect it's probably getting removed without the users intention.

Anyone can click "manual" and set additional keywords, but since it isn't listed, gardeners might not realize it is a pseudo-official keyword and remove it. I'll look into getting it listed.

#17 in reply to: ↑ 16 @jeremyfelt
3 years ago

Replying to nacin:

Anyone can click "manual" and set additional keywords, but since it isn't listed, gardeners might not realize it is a pseudo-official keyword and remove it. I'll look into getting it listed.

That's my fault. Assumed that mobile was an old tag. :)

#18 @dd32
3 years ago

Anyone can click "manual" and set additional keywords, but since it isn't listed, gardeners might not realize it is a pseudo-official keyword and remove it. I'll look into getting it listed.

oops, I thought the Manual was limited to gardeners. Ignore me :)

#19 @chriscct7
9 months ago

  • Keywords mobile removed

Mobile doesn't seem to apply to this ticket.

#20 @wonderboymusic
8 months ago

  • Keywords needs-unit-tests added; needs-testing needs-refresh removed
  • Milestone changed from Awaiting Review to 4.4
  • Owner set to wonderboymusic
  • Status changed from new to assigned

#21 @wonderboymusic
8 months ago

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

In 34570:

XML-RPC: wp.getComments should be allowed to return approved comments to those without the 'moderate_comments' cap.

Adds (rewrites) unit tests from 4 years ago that we never committed because....

Props wonderboymusic, koke, ericmann, nprasath002.
Fixes #17981.

Note: See TracTickets for help on using tickets.