Query for multiple post types not considering user permission to retrieve private posts

[leogermani]

When you query for posts not informing a specific post_status, WordPress will return posts that the current user can read (if there is a user logged in).

However, if you query for multiple post types, passing an array, or if you query for any post type, WordPress will ignore this behavior and won't return any private posts at all.

Expected behavior is that it would return posts with private status if they belong to a post type for which the user has the read_private_posts capability.

An existing, and rather undocumented, workaround is to grant the user the read_multiple_post_types capability. But this, again, will not check the permission current user have in each queried post type and will simply return all private posts for all queried post types.

Proposal

The proposed solution for this is to change the SQL query when querying for multiple post types without informing a post status, and combining the post_status and post_type WHERE clauses, checking user capability for each post type and returning the appropriate query in the very same way WordPress already does when you query for only one post type.

Sample Query when querying for posts and pages, for a user that HAS read_private_posts cap but DOES NOT HAVE read_private_pages:

SELECT SQL_CALC_FOUND_ROWS  wptests_posts.ID FROM wptests_posts  WHERE 1=1  AND 
(
  (wptests_posts.post_type = 'post' AND 
    (wptests_posts.post_status = 'publish' OR wptests_posts.post_status = 'private')
  ) 
  OR 
  (wptests_posts.post_type = 'page' AND 
    (wptests_posts.post_status = 'publish' 
     OR wptests_posts.post_author = 4 
     AND wptests_posts.post_status = 'private'
    )
  )
)  ORDER BY wptests_posts.post_date DESC LIMIT 0, 10 }}}

[leogermani]

Attached a patch with the change and some tests.

I made an additional elseif statement so it do not touch all other cases and acts only in this very specific case.

Would love to have some feedback.

git branch: ​https://github.com/leogermani/wordpress-develop/tree/48556

[leogermani]

This is a related ticket: #44737

If either of these tickets are merged, the other have to be updated

[leogermani]

Few observations on this ticket:

1) It changes the query results, so it needs Dev Notes

2) It also changes the SQL query, so it might break things for people filtering the query with "posts_where" filter, for example. Also needs to be in Dev Notes

3) I made it in a way it only changes the query in the very specific case of this ticket. The code could be cleaner if we changed the way the query is built in every case, but I thought this would bring less problems for developers

[leogermani]

There is another case where the same situation described in this ticket happens.

If you query for multiple post_types, informing a private post_status, WP_Query will not check permissions for each post type and again rely on the read_multiple_post_types capability.

All of this only if perm=readable.

The expected behavior is for the query to check read_private_posts permission for each post type consider the informed post_status only when current user has permission.

I don't think it is the case of opening another ticket because both things should be done together and the ticket title still applies. I'll edit the description.

[leogermani]

As a last note for today, I will have a look at all the tests we have for WP_Query. I think we should write more tests to cover as many situations as possible before doing such changes.

[audrasjb]

Hi,

Looks like this ticket is still awaiting review.

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[boonebgorges]

Hi @leogermani - Thanks for working on this ticket. Your description of the problem, and your general approach to fixing it, seem correct to me.

3) I made it in a way it only changes the query in the very specific case of this ticket. The code could be cleaner if we changed the way the query is built in every case, but I thought this would bring less problems for developers

This decision means that there are two places where the post_status logic is built: one for single post types, and one for multiple post types and/or 'any'. This will create a maintenance burden in the future. Could you please rework the patch so that the multiple-post-type logic is part of this block https://core.trac.wordpress.org/browser/tags/5.5/src/wp-includes/class-wp-query.php#L2514? This also means that we should have tests covering the existing behavior - there are some in tests/phpunit/tests/query/postStatus.php, but this should be reviewed for completeness.

[leogermani]

Thanks for the feedback @boonebgorges

This decision means that there are two places where the post_status logic is built: one for single post types, and one for multiple post types and/or 'any'.

Yes, just clarify that with "less problems for developers" I meant backward compatibility for anyone doing fancy transformations in the query using the posts_where filter.

If we change the query in all cases we will basically break things for anyone using this filter, since they usually rely on the query structure they expect from core using a regex or similar approach.

Is that ok? Something we let everyone know with a Dev note?

[boonebgorges]

If we change the query in all cases we will basically break things for anyone using this filter, since they usually rely on the query structure they expect from core using a regex or similar approach.

I'm not sure this has to happen. Your logic could be adapted in such a way that the "default" case generates the very same SQL that we currently generate. I guess there could be differences in whitespace or in parentheses, due to the way you're assembling $typewheres? Is that what you have in mind?

In the past, we've found it acceptable to make these kinds of changes to underlying SQL. (a) We can write a dev note that explains the change, as you note. And (b) it may be that developers have written their filters in such a way that these changes will break their customizations. On the latter point, we might be able to run some sort of search on the public wordpress.org plugin repository - perhaps, first matching plugins that filter 'posts_where' and then narrowing it down to those that do something with post_status? - that can give us some concrete examples of what may or may not break. If I had to bet, I'd say that most plugins are not, in fact, doing regex/string manipulation on the query, but instead are rebuilding the query based on the parameters passed to WP_Query. But we can only know if we check :-D

[leogermani]

I guess there could be differences in whitespace or in parentheses, due to the way you're assembling $typewheres? Is that what you have in mind?

Exactly!

In the past, we've found it acceptable to make these kinds of changes to underlying SQL. (a) We can write a dev note that explains the change, as you note. And (b) it may be that developers have written their filters in such a way that these changes will break their customizations. On the latter point, we might be able to run some sort of search on the public wordpress.org plugin repository - perhaps, first matching plugins that filter 'posts_where' and then narrowing it down to those that do something with post_status? - that can give us some concrete examples of what may or may not break.

Cool! Let's do it. I can work in the patch and do a search in the plugin repo. Let's see what we find.

Query for multiple post types not considering user permission to retrieve private posts

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

[leogermani]

hi @boonebgorges ,

I've linked a new PR to this ticket with the changes we spoke about. Please check it out whenever you can.

There's still another case to take care, which is when we also query for multiple post statuses. I'm wondering if that will be too much for a single diff and maybe it's better to wait for this to move forward and do this in a following one. Anyway, I already started looking into it and the solution will be similar.