WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 8 weeks ago

#24153 closed defect (bug) (fixed)

Sticky flag gets unset if author doesn't have publish_posts permission

Reported by: archon810 Owned by: obenland
Milestone: 4.3 Priority: normal
Severity: normal Version: 3.5
Component: Role/Capability Keywords: has-patch has-tests
Focuses: administration Cc:

Description

I'm observing a bug with the sticky flag. I set up a special user with a role of "Grammar Nazi" who should only have access to editing of other people's posts, but not publishing his own.

The permissions given to this role are:

  • edit_published_posts
  • edit_others_posts
  • edit_posts
  • read
  • read_private_posts

This user works out great - he's limited to only editing errors in other authors' posts.

However, there is a bug with sticky posts. If a grammar nazi edits a stickied post, the sticky flag gets unset. As a possibly related observation, there's no Edit button on the post edit page next to the Visibility area.

This bug is worked around by adding the publish_posts permission. However, this permission is unwanted in this case as grammar nazis shouldn't be able to post their own posts. Adding publish_posts enables the Edit button next to Visibility, and saves retain the sticky bit correctly.

So, in short: the sticky bit should be retained even when users without the publish_posts permission update a post.

Attachments (3)

24153.patch (455 bytes) - added by chriscct7 4 months ago.
24153.2.patch (504 bytes) - added by chriscct7 3 months ago.
current_user_can( $ptype->cap->edit_others_posts ) && current_user_can( $ptype->cap->publish_posts ) implemented
24153.3.diff (3.9 KB) - added by ericmann 2 months ago.
Updated patch file, with tests

Download all attachments as: .zip

Change History (26)

comment:1 @archon8102 years ago

  • Cc admin@… added

comment:2 @SergeyBiryukov2 years ago

Sticky flag indeed requires both publish_posts and edit_others_posts capabilities:
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-admin/includes/meta-boxes.php#L127

Related: [8546], [8577], ticket:7457:19.

comment:3 @archon8102 years ago

Thanks for tracking down the source references and the original ticket with relevant comments. It's clear that this is a bug at this point, right?

comment:4 @chriscct74 months ago

  • Focuses administration added
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.3
  • Owner set to chriscct7
  • Severity changed from major to normal
  • Status changed from new to accepted
  • Version changed from 3.5.1 to 3.5

Confirmed this still exists. Since the setting requires publish_post to edit, requiring publish_post when checking whether to change the status when saving the post solves the bug.

@chriscct74 months ago

comment:5 @chriscct74 months ago

  • Keywords commit added

comment:6 @archon8104 months ago

Wow, I forgot all about this bug. Been 2 years now. :o

comment:7 @chriscct74 months ago

@archon810 Yep, I'm part of a group working on reducing the length tickets are staying open without a response (called ancient tickets ). We've got it down from 4 1/2 years to 2 years so far. You just happened to be next on the queue :)

Thanks for your work on reporting this :-)

comment:8 follow-up: @obenland3 months ago

  • Keywords dev-feedback added; commit removed

We should discuss the patch a it more before it's ready for commit. By default Authors can't edit others posts, but they can publish posts.

comment:9 in reply to: ↑ 8 @chriscct73 months ago

Replying to obenland:

We should discuss the patch a it more before it's ready for commit. By default Authors can't edit others posts, but they can publish posts.

That actually works fine as is, and the patch doesn't affect it. When mentioning "author", we're not talking about the author role but rather an author of a post who can't publish a post. A better description would be "if user has edit_post but not publish_post, and edits a post, sticky gets unset"

comment:10 follow-up: @obenland3 months ago

If we change the cap from edit_others_posts, to publish_posts, users with the Author role can pass a true value for sticky in the post array to make that post sticky. Admittedly an edge case, but hey, devtools FTW!

comment:11 in reply to: ↑ 10 ; follow-up: @chriscct73 months ago

Replying to obenland:

If we change the cap from edit_others_posts, to publish_posts, users with the Author role can pass a true value for sticky in the post array to make that post sticky. Admittedly an edge case, but hey, devtools FTW!

how about

current_user_can( $ptype->cap->publish_posts ) && current_user_can( $ptype->cap->edit_others_posts)

then?

Last edited 3 months ago by chriscct7 (previous) (diff)

@chriscct73 months ago

current_user_can( $ptype->cap->edit_others_posts ) && current_user_can( $ptype->cap->publish_posts ) implemented

comment:12 @chriscct73 months ago

  • Keywords dev-feedback removed

@obenland thoughts on 24153.2.patch?

comment:13 in reply to: ↑ 11 ; follow-up: @obenland3 months ago

Replying to chriscct7:

how about

current_user_can( $ptype->cap->publish_posts ) && current_user_can( $ptype->cap->edit_others_posts)

then?

Would that fix it for the scenario outlined in the ticket description?

comment:14 in reply to: ↑ 13 @chriscct73 months ago

Replying to obenland:

Replying to chriscct7:

how about

current_user_can( $ptype->cap->publish_posts ) && current_user_can( $ptype->cap->edit_others_posts)

then?

Would that fix it for the scenario outlined in the ticket description?

Yes because the updated patch does not allow the stickiness status to be updated without both caps.

comment:15 follow-ups: @obenland3 months ago

But Grammar Nazi doesn't have publish_posts, only edit_others_posts.

The problem lies in the Publish meta box, where the sticky option only gets added when the user has both caps. If they lack publish_post the sticky input is never added to the form. The cap check in edit_post() goes through, but 'sticky' is not set in the $_POST array, hence removing the sticky flag.

comment:16 in reply to: ↑ 15 @chriscct73 months ago

Replying to obenland:

But Grammar Nazi doesn't have publish_posts, only edit_others_posts.

That's correct. The problem is in the save routine, right now Grammar Nazi can change the stickiness of the post because since he doesn't have both caps, the save routine will assume the post was unstickied. By wrapping the check in a cap check for both capabilities, Gramar Nazi cannot change the stickiness of the post by editing the post because the part that checks whether or not the post is sticky or not is circumvented by the capability check

comment:17 in reply to: ↑ 15 ; follow-up: @chriscct73 months ago

Replying to obenland:

The problem lies in the Publish meta box, where the sticky option only gets added when the user has both caps. If they lack publish_post the sticky input is never added to the form. The cap check in edit_post() goes through, but 'sticky' is not set in the $_POST array, hence removing the sticky flag.

We should do a permissions check before changing as the patch does as opposed to adding a hidden field or something can be manipulated by editing the source output of a page or using a POST replay

comment:18 in reply to: ↑ 17 @obenland3 months ago

  • Keywords needs-unit-tests added

Replying to chriscct7:

We should do a permissions check before changing as the patch does as opposed to adding a hidden field or something can be manipulated by editing the source output of a page or using a POST replay

Agreed. Could you check the other instances where we stick/unstick posts for permissions? This would also benefit from getting unit tests.

comment:19 @ericmann2 months ago

  • Keywords has-tests added; needs-unit-tests removed

I wrote a test that replicates the reported behavior with both wp_update_post and edit_post:

  • User with edit_others_posts but lacking publish_posts edits a sticky post
  • Make sure the post is still sticky after it's been edited

As things turn out, the wp_update_post routine worked just fine without any changes - it's merely the presence of the $_POST array that mucked things up in edit_post. I've added tests for both update/save routines just to be sure nothing breaks.

My new patch also refreshes the existing patch.

@ericmann2 months ago

Updated patch file, with tests

comment:20 @slackbot2 months ago

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

comment:21 @chriscct72 months ago

@ericmann Thanks!

Completely forgot about this ticket, but nevertheless, Eric's patch 3 looks good

comment:22 @obenland8 weeks ago

  • Owner changed from chriscct7 to obenland

comment:23 @obenland8 weeks ago

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

In 33096:

Check for all required caps before (un)sticking a post.

In cases where a user has the edit_others_posts capability but not
publish_posts, it was possible for that user to unstick a post after editing,
since the input field was never made available in that context.

Props ericmann, chriscct7.
Fixes #24153.

Note: See TracTickets for help on using tickets.