Core Data: Add support for fetching permissions of custom actions

[aduth]

Related: https://core.trac.wordpress.org/ticket/44287

This pull request seeks to enhance the canUser core data selector to handle custom user capabilities defined on REST endpoints via JSON Hyper Schema. This will allow a consistent interface to resolve user capabilities, removing the need to manually reference the _links property on a post object.

There are performance implications to this as well, since as of #16932, the result of getCurrentPost selector will update on every change to post content (every keystroke). Before these changes, any mounted component which depended on the current post (notably from checking these user permissions) would rerender on every key press.

Open Questions:

• Does it make sense to allow these "actions" to coexist in the same space, i.e. those verbs returned in the Allow response headers of an OPTIONS request, and those added as keys to a _links body? The changes here operate on an assumption that these can be equally considered as "action capability" keys.
• Currently, the REST API only adds these to the response of a singular post, and only in the edit context (source). To me, it seems mostly incidental, and that any resource could include these _links keys, including for requests on the root path (vs. providing an ID of a resource). For this reason, I didn't consider to include any guards to prevent an attempt to request canUser without an ID for a custom action, or to verify that the resource is "posts".
• Could we reuse getEntityRecord here to avoid an additional request for the post? In existing editor usage, the proposed changes shouldn't regress, because the request should resolve immediately since it is preloaded. But for non-preloaded data, there may be redundant requests with getEntityRecord. However, it is difficult (impossible?) to map a "resource" parameter to an equivalent "kind" and "name" parameter pair, and furthermore this would only support reuse of singular entities, not potential support for requesting capabilities on the root resource path (i.e. omitting the id argument).

Testing Instructions:

Verify there are no regressions in the behavior of capability testing for affected components:

A few examples:

• Contributors should see "Submit for Review" when hitting Publish; Administrators should see "Publish"
• Contributors should not have the option to assign an author to a post; Administrators should
• Contributors should be capable of assigning a category to a post, but not create a new one; Administrators can do both

Verify that the behavior of the canUserUseUnfilteredHTML selector still returns the same expected value, but logs a deprecation warning:

wp.data.select( 'core/editor' ).canUserUseUnfilteredHTML();
// `wp.data.select( 'core/editor' ).canUserUseUnfilteredHTML` is deprecated. Please use `wp.data.select( 'core' ).canUser( 'unfiltered-html', 'posts', wp.data.select( 'core/editor' ).getCurrentPostId() )` instead.
// false
wp.data.select( 'core' ).canUser( 'unfiltered-html', 'posts', wp.data.select( 'core/editor' ).getCurrentPostId() )
// false

[epiqueras]

Really nice PR here 😄!

Does it make sense to allow these "actions" to coexist in the same space, i.e. those verbs returned in the Allow response headers of an OPTIONS request, and those added as keys to a _links body? The changes here operate on an assumption that these can be equally considered as "action capability" keys.

It makes sense to me, but probably worth to verify with one of the REST API folk.

Currently, the REST API only adds these to the response of a singular post, and only in the edit context (source). To me, it seems mostly incidental, and that any resource could include these _links keys, including for requests on the root path (vs. providing an ID of a resource). For this reason, I didn't consider to include any guards to prevent an attempt to request canUser without an ID for a custom action, or to verify that the resource is "posts".

Nice, this is similar to how I removed the need for an id for the entity selectors so that you can request things like kind: 'root', name: 'site'.

Could we reuse getEntityRecord here to avoid an additional request for the post? In existing editor usage, the proposed changes shouldn't regress, because the request should resolve immediately since it is preloaded. But for non-preloaded data, there may be redundant requests with getEntityRecord. However, it is difficult (impossible?) to map a "resource" parameter to an equivalent "kind" and "name" parameter pair, and furthermore this would only support reuse of singular entities, not potential support for requesting capabilities on the root resource path (i.e. omitting the id argument).

The id argument can already be omitted in getEntityRecord, see the Site Title block implementation.

I think this is a good idea. What if canUser takes a "kind" and "name" and we resolve the resource/path from the entity config/getEntity?

[epiqueras]

We don't want to add "undefined" when resource and/or id are undefined.

[aduth]

resource should be considered a required parameter. id is optional†, but this concatenation occurs in a condition to consider it as being set.

† The JSDoc should be updated from the nullable {?string} type to the correct [id] optional parameter notation.

[epiqueras]

What if I want to request the root/index resource?

[aduth]

You would omit the id parameter.

[aduth]

I think I might not be understanding what you're asking. There will always be a resource.

When I say it doesn't necessarily need to be a singular resource, I consider it the same as how it might map to a use of current_user_can in PHP:

current_user_can( 'publish_posts' );
current_user_can( 'publish_post', $post->ID );

https://developer.wordpress.org/reference/functions/current_user_can/

I see this mapping to canUser as something like:

canUser( 'publish', 'posts' );
canUser( 'publish', 'posts', postId );

The caveat mentioned in the original post is that, currently, the REST API only supports the latter of these two, since it only includes action _links on the response of an item:

https://github.com/WordPress/wordpress-develop/blob/47e38ea/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L1675-L1683

My thinking is that, to represent the first of these (without an ID), it could be possible to include the action _links properties on the collection response as well:

https://github.com/WordPress/wordpress-develop/blob/47e38ea/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L155

[epiqueras]

Right, but we also have a base resource:

f6ce79c#diff-c5eaec560b039f2dc76f93fa76ba89da

Aren't there capabilities for /wp/v2/ or /?

I still think that canUser should have the same API as getEntityRecord with kind and name and we can just grab the resource from the entity config as I outlined here: #18956 (review).

[aduth]

Aren't there capabilities for /wp/v2/ or /?

Doesn't seem to me like there would be.

I still think that canUser should have the same API as getEntityRecord with kind and name and we can just grab the resource from the entity config as I outlined here: #18956 (review).

I agree this should be a goal for reuse. Not entirely sure yet how I feel about the overloaded signature, or if it's something to reevaluate whether "resource" is ever a sufficient parameter (i.e. might be more inclined to deprecate this parameter in favor of kind, name, if it came to it, or perhaps vice versa).

[epiqueras]

Oh, I never meant to have an overloaded signature. I was suggesting to deprecate "resource".

[epiqueras]

Suggested change

	// actions. Non-custom actions derive permissions from the response
	// action. Non-custom actions derive permissions from the response

[epiqueras]

Nice!

[epiqueras]

And the resource would be an empty string?

…

On Thu, Dec 5, 2019 at 6:32 PM Andrew Duthie ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/core-data/src/resolvers.js <#18956 (comment)>: > + let path = '/wp/v2/' + resource; + + if ( id ) { + path += '/' + id; You would omit the id parameter. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#18956?email_source=notifications&email_token=AESFA2FJQI6ZLPELFMOCW3TQXG2SLA5CNFSM4JWDWU52YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOGGPBI#discussion_r354641806>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AESFA2FNWCLQIG3SSA5SWZTQXG2SLANCNFSM4JWDWU5Q> .

[aduth]

I think this is a good idea. What if canUser takes a "kind" and "name" and we resolve the resource/path from the entity config/getEntity?

While I agree we should be moving toward this, do you think it's a blocker for what's proposed here? The way I see it, it's been an issue already, since there was never any reuse of the resource checking for the existing permissions. Nothing here should regress further on that front. It might be wise to explore this enhancement independently.

[epiqueras]

Agreed, it will be easier to review in a follow up.

[epiqueras]

It looks like some endpoints are missing though? See the 404s in the failed tests.

[aduth]

It looks like some endpoints are missing though? See the 404s in the failed tests.

I suspect the current iteration of this pull request is failing to account for post types other than 'post'.

Going back to your earlier point:

Oh, I never meant to have an overloaded signature. I was suggesting to deprecate "resource".

To make this handle post type would be difficult in its current form, since not only would we need to get the post type for the current post, we would need to map that post type to its corresponding rest_base in order to know the value to pass as the resource argument.

I think it reinforces that we need to reconcile how we reference entities within this module. Elsewhere, we could get an entity by its post type using the [ 'postType', 'post' ] pair. If we had this for canUser, it would solve avoid the need to try to map the post type to its rest_base.

A few things we'll want to keep in mind:

• We need to keep backwards compatibility
• As mentioned a few places elsewhere, the [ 'postType', postType ] pair is cumbersome to work with. We should want to retain the expressiveness of canUser as much as possible.
• And to that point, considering the best balance of usability and consistency with this function.

As things stand in the data module, I think the most consistent signature for this function would be something like:

canUser( action, kind, name, key )
// Example:
// canUser( 'publish', 'postType', 'post', 123 );

To make this compatible with existing usage, we could consider a few options:

• Create a separate function for the entity-specific signature (canUserForEntity, canUserForEntityRecord)
• If we can assume that key is numeric, we could do some testing against the number and types of the arguments to infer the intended signature (overload the function)
• For the entities usage, we could accept the [ kind, name ] or [ kind, name, key ] as an argument(s). In other words, the existing resource could be passed as a rest_base like it is today (e.g. 'posts') or as a entity record type pair (e.g. [ 'postType', 'post' ]).

Do you have any thoughts on whether any of the above sounds reasonable, and/or if a specific approach sounds best?

[epiqueras]

I suspect the current iteration of this pull request is failing to account for post types other than 'post'.

Ah, yeah, that's common.

Create a separate function for the entity-specific signature (canUserForEntity, canUserForEntityRecord)

This would be the most consistent with the rest of the code. We can't assume keys are numeric and passing [ kind, name, key ] in an array is not done elsewhere.

[aristath]

Is this still on the radar?
If yes then it's in need of a rebase... 😅 If not then maybe close?

[youknowriad]

Unless someone want to own this again, let's just close it for now.