Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#52094 new defect (bug)

Queries with perm readable/editable will not work for multiple post types and status queries

Reported by: leogermani's profile leogermani Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

This ticket is a continuation of #48556, which fixes permission checks when querying for multiple post types.

If you make a query for multiple post types, and also specifying one or more post statuses, there will be a couple of problems in the results if your query also includes perm=readable or perm=editable.

perm=readable is supposed to do a check to see if the current user has permission to read the queried post statuses before adding it to the SQL statement. It works fine if you're querying for only one post type, but will fail for multiple post types.

The problem is the same we found in #48556. When multiple post types are queried, the permission checked will be read_private_multiple_post_types, instead of individually check for each post type's capabilities.

(The same happens for perm=editable, which will check for edit_others_multiple_post_types)

Another problem: Custom post statuses

When querying for multiple post types and perm=readable, WP_Query also will not handle custom post statuses properly.

Any custom post status will be treated as a public post status, ignoring how it was registered. You can see it happening here: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-wp-query.php#L2478

Fix

The fix should be similar to what we are doing in #48556, and I think we should work on the patch after we finish #48556.

Reproducing

I'm going to attach a test file that reproduces all the bugs described here.

Attachments (1)

statusPerm.php (3.4 KB) - added by leogermani 4 years ago.
Tests that reproduces the bugs described in this ticket

Download all attachments as: .zip

Change History (9)

@leogermani
4 years ago

Tests that reproduces the bugs described in this ticket

#1 @leogermani
4 years ago

Here's a first proposed fix -> https://github.com/leogermani/wordpress-develop/pull/2

This is a PR that depends on the PR that fixes #48556 and a patch should be properly created once #48556 is merged. (or we should turn both in one single patch)

This patch also addresses the bug described in #51094 and adds a specific test for that.

What this patch does:

  1. It fixed #51094 by checking if at least one valid post status was found in the query
  1. It fixes support for custom post statuses in the block of code that goes:
<?php
foreach ( get_post_stati( array(), 'objects' ) as $status => $status_object ) {
        if ( in_array( $status, $q_status, true ) ) {
                if ( $status_object->private ) {

  1. It adds the current post Status logic inside a Loop and treat each post type differently, depending on the specific capabilities for each of them

Note

This patch introduces the same logic introduced by #48556 to the status query, so there's a bit of duplicate logic.

I considered doing everything in the same block of code and simplifying the whole thing, but this would "fix" some current inconsistencies and, therefore, change some behavior I'm not sure we want to change. It could also end up being a bit more complicated to read. But I can give it a go if anyone thinks it's worth it. It's not very difficult

The main "inconsistency" is that when we query for multiple post types, we do not consider the perm parameter. We basically assume perm=readable but there's no support for perm=editable for example. So we would start supporting it.

Also there is how we handle "protected states that should show in the admin all list", that are not considered when querying for post statuses.

This ticket was mentioned in PR #1183 on WordPress/wordpress-develop by leogermani.


4 years ago
#2

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

#3 @leogermani
4 years ago

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

Edit: Here's a PR that joins everything in a single block, fixing both this and #48556

https://github.com/WordPress/wordpress-develop/pull/1183

#4 @leogermani
4 years ago

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

This ticket was mentioned in PR #2810 on WordPress/wordpress-develop by leogermani.


3 years ago
#5

Add tests and refactor the creation of status and post types query

Trac ticket: https://core.trac.wordpress.org/ticket/52094

#7 @leogermani
3 years ago

I've created a brand new patch for this ticket: https://github.com/WordPress/wordpress-develop/pull/2810

cc @peterwilsoncc

What this patch does is it joins the situations where post_status query is used with the multiple post type logic fixed in #48556.

So now it's possible to query for multiple post types and multiple post_status at the same time and it will work as expected.

# Bug or feature

There's one thing that I'm not fixing here because I'm not sure it's a bug or feature.

If you query for post_status=any, the perm=readable param is ignored. Meaning that if you query for any post status, all post statuses will be returned regardless of the permission the current user has. Protected statuses will be removed, but private statuses will be included. This is how it behaves now and I didn't change it.

Should we change it? Or should we just update the docs to make it more explicit?

#8 @leogermani
3 years ago

One thing I'd like to add about the latest patch is that, even though it looks like a lot of changes at first sight, it is mostly moving code blocks around and making sure most of the behavior is kept just the same and triggered under the same circumstances.

Note: See TracTickets for help on using tickets.