Makes sure a SuperAdministrator can publish on any sites of the network. #3895

imath · 2017-12-09T23:19:21Z

Description

After preloading the current user from the /wp/v2/users/me REST request, a Super Administrator on a site he's not a user of has no caps/roles on it, as a result the major action button is set to Submit for review. The changes are making sure a Super Administrator with no caps/roles on the site can publish a post from the Gutenberg editor just like it's the case within the Classic Editor.

Steps to reproduce the bug on a multisite configuration

Log in as a SuperAdmin
Create a user
Create a site for this user.
Try to add a post on the created site.

How Has This Been Tested?

I've tested this on a Multisite configuration of WordPress version 5.0-alpha-42125-src
My testing environment: regular macOS HighSierra AMP - PHP 7.1.7
I've checked changes had no effects on the contributor role of the site.

Screenshots (jpeg or gifs if applicable):

Without the patch

With the patch

Types of changes

Bug fix

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

youknowriad · 2017-12-11T08:35:51Z

Thanks for this fix @imath I wonder if this could be considered a REST API bug.

Could this fix #2702 cc @aduth

imath · 2017-12-11T13:34:42Z

You're welcome @youknowriad I'm going to improve this PR because, now i've read #2702 i don't think it's the right way to do it.

First i've hesitated about the fact it might be related to the REST API. Now my ideas are clearer, i'm pretty sure it's not.

I think Gutenberg shouldn't rely on the values that are stored into the database for capabilities. The reason is you can have plugins containing capability mapping. And you won't have this information into the wp_{blog_id}_capabilities meta field. The REST API basically returns the value of this meta field.

A better way to handle this imho is to get the post type object capabilities for the post type being edited and to loop in it for the current user checking current_user_can() to be sure to trigger the map_meta_cap filter and avoid the super admin thing i've found in this PR and other troubles that may be the origin of #2702.

If you look at the post_submit_meta_box(), you'll see at the beginning that Core uses current_user_can() at line 27 of wp-admin/includes/meta-boxes.php

I'll update the PR in this direction.

aduth · 2017-12-11T17:46:10Z

I think Gutenberg shouldn't rely on the values that are stored into the database for capabilities.

Wouldn't this mean that, more broadly than just Gutenberg, the capabilities field on the REST API user entity cannot be trusted?

aduth · 2017-12-11T17:49:32Z

Enumerated at #2702 (comment), there are quite a few places we test for capabilities to determine whether something should be rendered.

Update branch to upstream master

imath · 2017-12-11T19:50:04Z

Wouldn't this mean that, more broadly than just Gutenberg, the capabilities field on the REST API user entity cannot be trusted?

To be sure a user can do a specific cap, you need to use current_user_can() so that the map_meta_cap filter is fired & the Super Admin check is performed.

Saving a cap for a specific user is not always done, so yes there's a good chance (and it's more likely to be the case when dealing with custom post types as they can set custom caps / caps mapping) the meta field does not reflect precisely what a user can do.

About the User controller of the REST API, i'm not sure its goal is to get what a user can do with a specific post type, but more to get/create/update/delete a user. If it was the case, the post type name would have been one of the available query arguments i think.

imath · 2017-12-11T21:04:41Z

@youknowriad & @aduth

I've updated the PR the way i've explained in this comment. It's fixing the issue i've first met with a SuperAdmin with no role on a child site, it should also fix possible issues for Custom Post Types having custom capabilities.

I think it shouldn't need any changes on the JavaScript part, as i kind of map back custom capabilities to post capabilities.

youknowriad · 2017-12-12T08:51:32Z

lib/client-assets.php

+
+				$response->set_data(
+					array(
+						'capabilities' => $post_type_caps,


Really awesome work here Thanks.

As far as I understand, these capabilities are "post type" specific capabilities only. Do you think it makes more sense to use a more descriptive key post_type_capabilties => array( $post_type => $post_type_caps ) to avoid confusion on the client-side?

Granted this means we need to update some client code as well.

Also this raises the question of whether this should be a separate endpoint /users/me/post_type_caps?postType=post

You're very welcome @youknowriad

I agree it makes a lot of sense. As I lack some knowledge of React, I took no risk adventuring myself into this part of Gutenberg's code 😃

As you said, it would require some updates into the client code so that thepost_type_capabilties are used if available.

About the new endpoint, yes it's a possibility. You can alternatively use a specific query parameter /users/me/?context=edit&post_type=post and temporarly use the rest_prepare_user filter to include the post_type_capabilties into the response before editing the WP_REST_Users_Controller::prepare_item_for_response() method once Gutenberg is merged into core.

About the new endpoint, yes it's a possibility. You can alternatively use a specific query parameter /users/me/?context=edit&post_type=post and temporarly use the rest_prepare_user filter to include the post_type_capabilties into the response before editing the WP_REST_Users_Controller::prepare_item_for_response() method once Gutenberg is merged into core.

That sounds like a good solution for now. We'll probably have to bring this issue to the REST API team later. Might be good to have @aduth's insights

Also, the client changes should be small enough, I can help with those if necessary ;)

Update branch to latest upstream edits.

…of user. Edit the query parameters used for the users/me endpoint on the server and client side. Merge capabilities with post type capabilities into the client side before checking the user capabilities

Add spaces into inline comments. Add missing "gutenberg" textdomain. Remove quotes in JS object keys.

imath · 2017-12-13T09:31:39Z

editor/components/post-publish-button/label.js

+	const userCaps = user.data ?
+		{ ...user.data.capabilities, ...user.data.post_type_capabilities } :
+		{ publish_posts: false };
+	const isContributor = ! userCaps.publish_posts;


Note to self: the test is probably failing because i don't test user.data anymore when setting isContributor.

youknowriad · 2017-12-13T10:12:54Z

editor/components/post-author/check.js

-	if ( ! user.data || ! user.data.capabilities.publish_posts || authors.length < 2 ) {
+	const userCaps = user.data ?
+		{ ...user.data.capabilities, ...user.data.post_type_capabilities } :
+		{ 'publish_posts': false };


Do we need if this will be always a "post type capability" or not? In which case we coud avoid using user.data.capabilities entirely?

Minor: maybe we could avoid creating a separate object, by leveraging lodash get

const canPublishPosts = get( user, 'data.post_type_capabilities.publish_posts', false )

Good point, i think we don't need it, i'll update the PR in this direction + thanks for the tip about lodash 👍

youknowriad · 2017-12-13T10:15:23Z

editor/components/post-author/check.js

 		return null;
 	}

 	return children;
 }

 const applyWithAPIData = withAPIData( () => {
+	const postTypeSlug = window._wpGutenbergPost.type;


We should avoid using the global here, and rely on the getCurrentPostType selector. Example here
https://github.com/WordPress/gutenberg/blob/master/editor/components/page-attributes/check.js#L32-L51

Thanks for your feedback. I've tried to use getCurrentPostType but unfortunately the Post Type was not defined yet at the time the request was sent. I'll give it another look asap.

imath · 2017-12-14T02:23:24Z

@youknowriad

I improved the PR the way you advised (use lodash/ stop using the global to get the post type / only use post type capabilities).

I still need to update the tests so that they also use the post_type_capabilities. I think it's the reason why many failed during my last update.

imath · 2017-12-14T10:07:28Z

I confirm. I just updated tests so that they use post_type_capabilities instead of capabilites and they are now passing.

imath · 2017-12-17T12:33:37Z

Hi @youknowriad @aduth

As 3 days passed and this issue hasn't been updated, I guess i can't do anything more on my side. So I'm leaving you deal with it, hoping the Gutenberg UI will soon be consistent with the Post Type capabilities of a user.

Thanks for your consideration and advices.

youknowriad · 2017-12-18T07:59:46Z

Sorry for the delay @imath I was AFK, this looks solid to me. Might be good to have a second opinion @aduth before merging?

aduth

Speaking to the REST API changes specifically, the post_type_capabilities property appears at first glance redundant with the existing capabilities property. It's a bit strange to specify a post type parameter on a user endpoint, though more understandable with the endpoint returning capabilities information.

Curious though, would another option here have been to add an is_super_admin property to the endpoint to allow all actions?

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

aduth · 2017-12-18T13:50:52Z

editor/components/post-author/check.js

 export function PostAuthorCheck( { user, users, children } ) {
 	const authors = filter( users.data, ( { capabilities } ) => capabilities.level_1 );
-	if ( ! user.data || ! user.data.capabilities.publish_posts || authors.length < 2 ) {
+	const userCanPublishPosts = get( user, 'data.post_type_capabilities.publish_posts', false );


Minor: At one point I'd benchmarked the two different ways to call get (with . and as an array) and found the array signature to perform much better, and arguably be more readable:

Also, we could assume that withAPIData will guarantee that user will at least be passed as an object, meaning we don't need to guard against access on user (see also the line prior).

get( user.data, [ 'post_type_capabilities', 'publish_posts' ], false )

aduth · 2017-12-18T13:55:19Z

lib/client-assets.php

 	// Preload common data.
 	$preload_paths = array(
-		'/wp/v2/users/me?context=edit',
+		sprintf( '/wp/v2/users/me?post_type=%s&context=edit', sanitize_key( $post_type ) ),


How did you land on using sanitize_key? Can custom post type slugs have non-alphanumeric characters?

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

Sorry, it's probably my habit to always sanitize when i use sprintf. I'll remove it.

Sorry, it's probably my habit to always sanitize when i use sprintf. I'll remove it.

I think sanitization is sensible, just making sure that it aligns with what's permitted when registering a custom post type.

aduth · 2017-12-18T14:55:40Z

Or, alternatively, should we be running the capabilities property on the users endpoint through relevant filters rather than sending raw?

imath · 2017-12-18T15:52:17Z

@aduth

Curious though, would another option here have been to add an is_super_admin property to the endpoint to allow all actions?

I should probably edit the Title of the PR. It does concerned any user having caps to edit a post type not only the Super Administrator.

You can have specific roles in WordPress to edit/publish etc.. a specific custom post type.

Or, alternatively, should we be running the capabilities property on the users endpoint through relevant filters rather than sending raw?

Sorry i am not sure to understand. Could you explain in a more detailed way ?

aduth · 2017-12-18T16:54:41Z

Sorry i am not sure to understand. Could you explain in a more detailed way ?

I mean instead of naively returning $user->allcaps here:

https://github.com/WordPress/wordpress-develop/blob/bc35d72a6363fbf67f65bc96f22fd758e9495e2a/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L893-L895

...that we'd iterate through known capabilities and call user_can to ensure that the relevant filters are applied†. Though, in the case of a super admin, I expect allcaps would be an incomplete array of capabilities, since they're not explicitly assigned for the user.

† Might be a non-negligible performance impact to this.

youknowriad · 2017-12-18T16:56:36Z

.Might be a non-negligible performance impact to this.

Also, the capabilities may change per CPT if I understand properly. So we'll have to loop over the CPTs and enforce a per CPT structure in this case.

…nberg into try/improved-mobile-tools

…ription Improve paragraph block description.

Try improving mobile controls

youknowriad · 2017-12-20T14:46:01Z

Since @aduth is AFK, I don't want to let this PR stale. In opinion, this is good enough to ship. It's certainly not idea to add this post_type_capabilities attribute but it's also not idea to iterate over all the post types and compute the capabilities for each one of them.

A separate endpoint /user/me/post_type_catabilities might make sense here but I'm ok with moving forward and revisit. This fixes several bugs in the multisite installations.

I'd appreciate another review, maybe @mcsf @pento @gziolo before merging.

Also, this needs a rebase :)

mcsf

This is a nice fix; thank you, @imath. As mentioned, this should be rebased.

There's room to smooth the edges (e.g. refactor the common patterns), but we can address that later.

mcsf · 2017-12-20T17:24:26Z

lib/register.php

+ * @param WP_REST_Request $request Full details about the REST API request.
+ * @return object The Post Type capabilities.
+ */
+function gutenberg_get_post_type_capabities( $user, $name, $request ) {


Typo at capabities

mcsf · 2017-12-20T17:26:57Z

lib/register.php

+/**
+ * Includes the value for the custom field `post_type_capabities` inside the REST API response of user.
+ *
+ * TODO: This is a temporary solution. Next step would be to edit theWP_REST_Users_Controller,


Missing space at theWP_REST_Users_Controller.

Wondering if we should be grouping these things that are expected to be fixed directly in core. Until then, does this function, as well as the next, make more sense in lib/compat.php? It might make them more visible.

@jasmussen

Rename "Classic Text" to "Classic". Props @jasmussen

3 column inserter

…of user. Edit the query parameters used for the users/me endpoint on the server and client side. Merge capabilities with post type capabilities into the client side before checking the user capabilities

Add spaces into inline comments. Add missing "gutenberg" textdomain. Remove quotes in JS object keys.

Call get using an array to improve performance. Sanitizing the post type slug is not necessary

Fix a missing space in two docblocks Fix a typo in a function name Move temporary functions to lib/compat.php to improve their visibility

@mcsf

These functions are now in lib/compat.php to follow @mcsf recommandations.

imath · 2017-12-21T03:42:56Z

@mcsf Hi,

yw & thanks a lot for your recommandations, i've applied them and "rebased". As it was the first time i was using this command, i hope i didn't put a mess 😰

youknowriad · 2017-12-21T08:53:06Z

Mmmm! the rebase look broken :( Might be simpler to copy the code and push another PR? You can probably try cherry-picking the right commits as well?

Do you think you can open another PR to fix this?

imath · 2017-12-21T10:20:26Z

@youknowriad i was fearing it, so i saved a diff before 😉 it will be easier for me to create another PR, i think i still need to train to be a good rebaser! I’ll do it tonight (office hours for me!) and i’ll add it to this PR. Sorry for the « contretemps » 😊

youknowriad · 2017-12-21T11:06:14Z

No worries, Git can be scary sometimes :)

#3895 showed there was an issue with SuperAdmins not having a role on a site of a multisite network. After some discussions, we think it is best to check the post type capabilities for the connected user and the current post type being edited. A new parameter is added to the users/me REST API route containing the post type name. This parameter is used to get the corresponding post type capabilities. Capability checks have been updated accordingly into Gutenberg editor components

youknowriad · 2017-12-21T13:31:57Z

superseded by #4122

Makes sure a SuperAdministrator can publish on any sites of the network.

d078621

youknowriad added the Core REST API Task Task for Core REST API efforts label Dec 11, 2017

Merge branch 'master' into fix/superadmin-caps

b9a12a6

Update branch to upstream master

imath added 2 commits December 11, 2017 21:43

Validate the User's capabilities for the Post Type being edited

5f498ee

Add missing full-stops to inline comments

a9d59f8

youknowriad reviewed Dec 12, 2017

View reviewed changes

imath added 3 commits December 13, 2017 08:37

Merge branch 'master' into fix/superadmin-caps

3eef17c

Update branch to latest upstream edits.

Add a 'post_type_capabilities' custom field to the REST API response …

7d454c0

…of user. Edit the query parameters used for the users/me endpoint on the server and client side. Merge capabilities with post type capabilities into the client side before checking the user capabilities

Fix failing travis tests

246c9cc

Add spaces into inline comments. Add missing "gutenberg" textdomain. Remove quotes in JS object keys.

imath commented Dec 13, 2017

View reviewed changes

youknowriad reviewed Dec 13, 2017

View reviewed changes

Stop using a global to get the post type name

0ff8681

Update tests so that they use post_type_capabilities

dcf93dd

Merge branch 'master' into fix/superadmin-caps

45b3ba9

aduth reviewed Dec 18, 2017

View reviewed changes

youknowriad and others added 5 commits December 20, 2017 15:33

Block Mobile Toolbar: Extract as a separate component

6b51d23

Merge branch 'try/improved-mobile-tools' of github.com:WordPress/gute…

7c1cfae

…nberg into try/improved-mobile-tools

Factory: Fix block creation with falsey default

6378974

Merge pull request #4103 from WordPress/update/improve-paragraph-desc…

76ddebf

…ription Improve paragraph block description.

Merge pull request #4101 from WordPress/try/improved-mobile-tools

c89e226

Try improving mobile controls

Define setAttributes before using it.

9721009

mcsf reviewed Dec 20, 2017

View reviewed changes

jorgefilipecosta and others added 14 commits December 20, 2017 17:37

Removed duplicate of buildTermsTree in HierarchicalTermSelector. (#4111)

e5bafaa

Merge pull request #4104 from WordPress/update/rename-classic-text

a510f3c

Rename "Classic Text" to "Classic". Props @jasmussen

Merge pull request #4095 from WordPress/try/three-column-inserter

5b85f85

3 column inserter

Makes sure a SuperAdministrator can publish on any sites of the network.

387faa0

Validate the User's capabilities for the Post Type being edited

3ec3139

Add missing full-stops to inline comments

88b2061

Add a 'post_type_capabilities' custom field to the REST API response …

3f8c457

…of user. Edit the query parameters used for the users/me endpoint on the server and client side. Merge capabilities with post type capabilities into the client side before checking the user capabilities

Fix failing travis tests

fa7ba6f

Add spaces into inline comments. Add missing "gutenberg" textdomain. Remove quotes in JS object keys.

Stop using a global to get the post type name

e54f194

Update tests so that they use post_type_capabilities

631e1bf

Apply @aduth recommandations

2066975

Call get using an array to improve performance. Sanitizing the post type slug is not necessary

Apply @mcsf recommandations

8f3bd04

Fix a missing space in two docblocks Fix a typo in a function name Move temporary functions to lib/compat.php to improve their visibility

Fix some conflicts which appeared after rebasing

8459e42

Remove new functions from lib/register.php

8f82b7d

These functions are now in lib/compat.php to follow @mcsf recommandations.

imath mentioned this pull request Dec 21, 2017

Use Post Type capabilities instead of All capabilities. #4122

Merged

3 tasks

youknowriad closed this Dec 21, 2017

Makes sure a SuperAdministrator can publish on any sites of the network. #3895

Makes sure a SuperAdministrator can publish on any sites of the network. #3895

Conversation

imath commented Dec 9, 2017

Description

Steps to reproduce the bug on a multisite configuration

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Without the patch

With the patch

Types of changes

Checklist:

youknowriad commented Dec 11, 2017

imath commented Dec 11, 2017 • edited Loading

aduth commented Dec 11, 2017

aduth commented Dec 11, 2017

imath commented Dec 11, 2017

imath commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imath commented Dec 14, 2017

imath commented Dec 14, 2017

imath commented Dec 17, 2017

youknowriad commented Dec 18, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Dec 18, 2017

imath commented Dec 18, 2017

aduth commented Dec 18, 2017

youknowriad commented Dec 18, 2017

youknowriad commented Dec 20, 2017

mcsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imath commented Dec 21, 2017

youknowriad commented Dec 21, 2017

imath commented Dec 21, 2017

youknowriad commented Dec 21, 2017

youknowriad commented Dec 21, 2017

imath commented Dec 11, 2017 •

edited

Loading