Verify 'upload_files' capability when displaying upload UI in media blocks #4155

imath · 2017-12-23T18:20:35Z

Closes #1536
Closes #3672

I've been looking for a way to solve this issue #3672 and I think one possible way is to add a capability attribute to blocks and check the current user has this cap before registering the block.

Description

In the classic editor, the upload button is disabled if the current user does not have the upload_files capability (eg: contributor). But when a contributor uses Gutenberg he can use "file" relative blocks. When he tries to use one of them as this role hasn't got the upload_files capability, the REST reply to site.url/wp-json/wp/v2/media is rest_cannot_create (403).

How Has This Been Tested?

I have tested this PR for the contributor role. A contributor does not have the upload_files capability and the PR is making sure he can't use blocks involving a file upload (eg: image, core-image, audio, video, gallery).

Screenshots (jpeg or gifs if applicable):

See #3672 for a video of the issue.

Types of changes

I think this PR is first a bug fix, but i also think it could be interesting to be able to restrict block usage according to the user's role.

Checklist:

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

imath · 2018-01-01T23:47:36Z

blocks/library/image/index.js

@@ -59,6 +59,8 @@ registerBlockType( 'core/image', {
 		},
 	},

+	capability: 'upload_files',


Thanks a lot for your review @mtias

On one hand I totally agree a contributor should be able to pick an existing media from the WordPress Media Library.

On the other hand, he can't even do that 😢 Being logged in as a contributor, when you click on the button to pick an image, a video or an audio file, you get an empty library as the ajax action that performs the attachments query is checking for the upload_files capability very early. See /wp-admin/includes/ajax-actions.php.

So today these "media" blocks are pretty useless for contributors, that's why I suggested to remove them as it's an experience that is a bit frustrating 😬

To only allow picking to contributors would require a WordPress core change that would be a bit more difficult than simply allowing uploading and picking just like the other roles, imho. I can think of UI/feature changes to remove the Uploader & the Upload tab of the WordPress Media Editor for instance.
Adding the upload_files capability to the contributor role would be much easier, but it would probably require some wide communication to WordPress users as I can imagine some are using this role for the users they wan't to restrict from uploading files...

Being logged in as a contributor, when you click on the button to pick an image, a video or an audio file, you get an empty library as the ajax action that performs the attachments query is checking for the upload_files capability very early.

I hadn't realized this, and it's interesting, especially since the new media REST API endpoints don't have the same restriction, it'd be entirely possible to create a media explorer based on the REST API given current permissions checks. This hints to me that perhaps in the interim to get to that point, perhaps these restrictions ought to be loosened (i.e. those in admin-ajax.php and in the current media library implementation).

This makes sense. I think it'd be ok to remove them from the inserter, but how should we handle editing an existing post that has images? Or a post that has an image placeholder?

@aduth This is handled into the PR actually. Images or any other media in existing post will be displayed. Just like in the classic editor the contributor can remove the image from the post content (but not from the Media Library) or edit display attributes (alignment, etc..)

blocks/api/registration.js

+ * @type {Object}
+ */
+const currentUser = get(
+	Object.values( pickBy( window._wpAPIDataPreload, ( value, key ) => key.indexOf( '/wp/v2/users/me' ) !== -1 ) ),


aduth · 2018-01-03T13:38:23Z

blocks/library/image/index.js

@@ -59,6 +59,8 @@ registerBlockType( 'core/image', {
 		},
 	},

+	capability: 'upload_files',


Being logged in as a contributor, when you click on the button to pick an image, a video or an audio file, you get an empty library as the ajax action that performs the attachments query is checking for the upload_files capability very early.

I hadn't realized this, and it's interesting, especially since the new media REST API endpoints don't have the same restriction, it'd be entirely possible to create a media explorer based on the REST API given current permissions checks. This hints to me that perhaps in the interim to get to that point, perhaps these restrictions ought to be loosened (i.e. those in admin-ajax.php and in the current media library implementation).

imath · 2018-01-04T22:23:00Z

FWIW i've tested in the classic editor the situation where a contributor can edit a post containing an image. In this case, the contributor can edit the markup (alignment, caption, link, classes, rel attribute) and can remove the image from the content.

When it's a gallery of images, the contributor cannot see it. He sees this placeholder instead

When he tries to edit it, he has the same issue we have here: he can't add images to the gallery or list the images of the gallery. But he can still edit the markup and remove the gallery.
As in the existing 90 tickets about gallery none are talking about this "improvable" experience, it makes me think the possibility for a contributor to edit a pending post containing an image might be an edge case.

Anyway & imho, a possible approach could be to hide the media blocks from the block pickers, so that a contributor cannot be the one who inserts one of these blocks. If another user with the upload_files capability added a media block and kept the post as pending. The media should be displayed to the contributor and he should be able to edit the markup, alignment or even remove the block. So this means the only change to the block there would be in the block's toolbar where the edit button shouldn't be displayed so that the contributor cannot open the Media Editor.

@aduth about WP_REST_Attachments_Controller: creating items checks for the same capability (upload_files). Getting media is more permissive, but do you know if it would be that permissive if it was used in the context of the Media Editor ?

imath · 2018-01-07T21:02:54Z

@mtias @aduth

In 1216849 I'm using the approach I've described in my previous comment. But instead of not showing the blocks, I'm using the feature that disables them when the property useOnce is true (eg: the more block).

So when the contributor is using the inserter he sees:

When another user with an upper role added an image or a gallery and kept the post as pending.. The contributor can see the corresponding block, remove it or modify markup, caption, link, alignment etc.. But he can't use the Edit button to open the WP Media Editor and can't transform it to another type (as the other types also require the upload_files capability).

I haven't edited the Block autocompleter because i think there's another issue to fix in blockAutocompleter function as it doesn't seem to check for disabled blocks. As you can see below it's possible to include more than once more block using the autocompleter.

aduth

Looping in @noisysocks , who's been working on relevant Inserter refactoring that might overlap with some of the changes proposed here (#4497).

editor/components/block-switcher/index.js


 	if ( isLocked || ! allowedBlocks.length ) {
 		return null;
 	}

+	const userAllowedBlocks = allowedBlocks.filter( b => ! b.capability || userCapabilities[ b.capability ] );


blocks/library/image/index.js

@@ -59,6 +59,8 @@ registerBlockType( 'core/image', {
 		},
 	},

+	capability: 'upload_files',


editor/edit-post/modes/visual-editor/test/inserter.js

 		const wrapper = shallow(
-			<VisualEditorInserter insertBlock={ insertBlock } mostFrequentlyUsedBlocks={ mostFrequentlyUsedBlocks } />
+			<VisualEditorInserter insertBlock={ insertBlock } mostFrequentlyUsedBlocks={ mostFrequentlyUsedBlocks } user={ user } />


editor/components/inserter/menu.js

+
+		if ( ! isDisabled && requiredCapability ) {
+			isDisabled = ! get( this.props.user.data, [ 'capabilities', requiredCapability ] );
+		}


editor/store/selectors.js

+		 * make sure he cannot use it disabling it.
+		 */
+		if ( userCapabilities && blockType.capability && ! get( userCapabilities, blockType.capability, false ) ) {
+			blockType.isDisabled = true;


imath · 2018-03-24T12:48:22Z

@gziolo I think the user capability check needs to be done in JavaScript because as we've seen working on this issue, it's not just about not loading the block if the user does not have the capability to upload_files. We need to let the block load and disable it in JavaScript. The reason is while the post is in draft mode, there's a probability a user with a higher role edits the post and adds a new media block in it, then leave it as a draft. To preserve the experience of the contributor when he comes back and edit the same draft, the inserted media block is doing his job (eg: displaying the image or the gallery) but some actions are disabled: transformation, replacement of media etc.. Just like in the classic editor the contributor can still edit the html, change the alignment or link or remove the media from the content.

That being said, I can see a real interest about doing some stuff on the server side to add this capability feature to any custom block.

User capabilities are fetched using this Rest Request: /wp/v2/users/me?post_type=${ postType }&context=edit

As upload_files is a core capability, it's fetched but let's say a custom block choose a custom capability like can_use_mycustom_block, then it's probably needed to edit the users/me endpoint so that it performs a current_user_can() check on all the custom blocks custom capabilities to make sure the map_meta_cap filter is fired.

We had to deal with this for Multisite configs for the super admins who don't have the capabilities for a subsite (see https://github.com/WordPress/gutenberg/blob/master/lib/compat.php#L187-L232)

So having the capability attribute available into the WP_Block_Type would probably be helpful to pluck all the custom capabilities from the list of registered blocks to check them all and eventually add them to the capabilities parameter of the users/me endpoint json reply.

This way, what has been done in this PR for the media blocks would work for any block.

gziolo · 2018-03-26T10:44:56Z

@karmatosed proposed new UX for the media blocks in here: #5456 (comment). Now, the question is if with custom URL for the external images, we still should disable those blocks, or rather do something the action buttons?

It would be nice to figure out what is our overall strategy related to templates, block's locking, user capabilities and roles. In this particular case, it's been proposed to lock the media block when you can’t upload files. While it seems to be a good solution from the media management perspective, it isn’t necessarily the best approach for someone editing the post who wants to change alignment, add a custom class or change the number of columns displayed. We plan to allow providing the custom urls soon, so it’s another thing that doesn’t fall under upload files category (assuming those urls are always external).

imath · 2018-03-26T19:29:12Z

Awesome ! Absolutely only Upload/Media Library buttons should be disable then. Let's wait for it!

About the second part Maybe I wasn't clear or used a bad english, a contributor can still edit alignment/html or remove the media in the PR.

So far strategy was to disable the possibility to add the block, just like the core/more block (once added). But the media url field is showing it's probably not the best one.

danielbachhuber · 2018-05-10T13:21:14Z

@imath We have a path forward for the capabilities part of this established in #6361 (comment) You can see it in action with #6529

Do you want to try to recover this pull request, or does it make sense to start fresh? I've filed #3672 against the Merge Proposal: REST API milestone to make sure it retains visibility.

Start using it into the MediaPlaceHolder to inform the contributor role he cannot upload media. If the block supports selecting an URL, the instruction will inform the contributor role he can only add a link to a media.

As it is not possible to fetch the Featured image for this role, no matter the context of the REST request, display a message to inform him managing the featured image needs the upload files cap.

@danielbachhuber

This is to bring into Gutenberg the improvement @danielbachhuber committed in WordPress 5.0 branch See https://core.trac.wordpress.org/changeset/43833

…_files cap

imath · 2018-11-13T20:37:44Z

Ouch sorry but I have a very old computer, i can't install docker so i'm not able to see why a e2e test is failing.. sysctl kern.hv_support is returning 0.

antpb · 2018-11-14T22:31:59Z

I think the tests are good now @imath . Maybe someone kicked them off again. I've seen this in some of my PRs.

imath · 2018-11-15T05:23:01Z

Awesome !! Thanks for your comment @antpb.

@gziolo @aduth @youknowriad do we have a chance to finally have this fixed after almost a year working on it ? 😊

Or is there anything else that needs to be done ?

youknowriad

Really great work on this PR 👏

I left some minor comments. And not blocking here but I'd appreciate an e2e test (maybe as a follow-up PR) to ensure we don't regress.

youknowriad · 2018-11-15T08:39:06Z

packages/editor/src/components/media-placeholder/index.js

-export default withFilters( 'editor.MediaPlaceholder' )( MediaPlaceholder );
+const applyWithSelect = withSelect( ( select ) => {
+	let hasUploadPermissions = false;
+	if ( undefined !== select( 'core' ) ) {


I think this check is not necessary, to be consistent with other withSelect usage, we should assume the "core" store to be available as it's a dependency of the editor package.

I think I saw in the comments that it's because of the unit tests. Two suggestions:

Avoid running this code in unit tests (test the inner component)

Import the @wordpress/core-data in the unit tests to ensure that the store is available.

youknowriad · 2018-11-15T08:39:43Z

packages/editor/src/components/media-upload/check.js

+
+export default withSelect( ( select ) => {
+	let hasUploadPermissions = false;
+	if ( undefined !== select( 'core' ) ) {


Same comment as above.

It had several iterations since the time Andrew reviewed it

gziolo · 2018-11-15T09:17:53Z

@gziolo @aduth @youknowriad do we have a chance to finally have this fixed after almost a year working on it ? 😊

Congrats on keeping this PR updated for so long and ensuring it gets merged 💯 Thanks for this contributions. I will address comments raised by Riad in the follow-up PR.

imath · 2018-11-15T11:09:05Z

Thanks a lot for your help everyone 😍🎉

youknowriad · 2018-11-15T11:12:35Z

This would require some changes to be back-ported in the 5.0 branch (for the preloading changes in PHP). Up for a ticket+commit @gziolo :P

imath · 2018-11-15T11:15:29Z

@youknowriad the preloading part is already in Core thanks to https://core.trac.wordpress.org/changeset/43833

youknowriad · 2018-11-15T11:16:31Z

Oh good thanks @imath