Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip irrelevant capabilities filtering in filter_user_has_cap() with whitelist #543

Merged
merged 3 commits into from
Jun 27, 2018
Merged

Skip irrelevant capabilities filtering in filter_user_has_cap() with whitelist #543

merged 3 commits into from
Jun 27, 2018

Conversation

TheCrowned
Copy link
Contributor

This fixes #489.

CAP needs to filter the Core has_cap() function to allow co-authors to edit posts, even if they are not their primary author. However, the desired behavior is to filter only the edit_posts-like caps, while leaving alone all the others. Right now this happens, but in a very sub-optimal way:

function filter_user_has_cap( $allcaps, $caps, $args ) {
	$cap = $args[0];
	$user_id = isset( $args[1] ) ? $args[1] : 0;
	$post_id = isset( $args[2] ) ? $args[2] : 0;
	
	$obj = get_post_type_object( get_post_type( $post_id ) );
	if ( ! $obj || 'revision' == $obj->name ) {
		return $allcaps;
	}

The problem lies in calling get_post_type( $post_id ) with no certainty that $post_id is indeed a post ID. Indeed, it is a post ID when the cap is a post related one, but can be something else altogether for other caps. For example, for users-related caps it holds the user ID.

So the issue is that get_post_type( $post_id ) is most often called with $post_id being either 0 or a random int. get_post_type() calls get_post(), which will end up loading the wrong post, or no post at all. If no post is loaded, that call never gets cached and will run on every request. Considering that has_cap() is called hundreds of times per request, this can result in a major slow down.

One would be tempted to just bail if the filtered cap is not edit_posts or edit_others_posts. However, CAP can be active on any post type, and thus other caps need to be taken into account. For example, one could have edit_downloads, or edit_pages caps, and CAP has to filter them.

My solution is thus to get all the post types on which CAP is active, retrieve the edit-like caps and build an array with them. That is the whitelist it is then used to check whether the filtered cap is an interesting one, or we should bail out quickly. The whitelist is built by a separate function, that also caches the result in a class variables. Thus, the whitelist is built once per request and then used to quickly bail out in filter_user_has_cap() if needed.

Building the whitelist is pretty quick - basically as quick as reading from a global WP var, since that is what get_post_type_object() ultimately does.

Unfortunately, we still have to retain the original check that bailed out in the sub-optimal way. This is because the whitelist can be built only after the post types have been initialized and, although this happens pretty early, there are still a handful of has_cap() calls that run before that happens, and we must have a way to bail out if needed. However, the too-early has_cap() calls are really just a few, so the whitelist ends up being used 99% of the times.

--

A fix had already been proposed in #490, but it uses a blacklist of three caps that are often called and that we should bail out quickly, but a) there are probably more caps; b) it does not bail out when $post_id === 0. This PR makes #490 obsolete.

Copy link
Contributor

@sboisvert sboisvert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this code. Well done!

* Builds list of capabilities that CAP should filter.
*
* Will only work after $this->supported_post_types has been populated.
* Will only run once per request, and then cache the result.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really the explanation here. I would add something like:
Cache the result in $to_be_filtered_caps since CoAuthors_Plus is only instantiated once and stored as a global
or something like that to make it even more clear why this is cached for the whole request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - done! :)

@sboisvert
Copy link
Contributor

@rebeccahum If this looks good with you let's merge it :)

cc @philipjohn in case you want input.

@rebeccahum rebeccahum merged commit 604b3f0 into Automattic:master Jun 27, 2018
rebeccahum added a commit that referenced this pull request Mar 26, 2019
…relevant-caps

Skip irrelevant capabilities filtering in filter_user_has_cap() with whitelist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip cap (capabilities) filtering on irrelevant cap checks
3 participants