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

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

Merged
merged 7 commits into from Nov 15, 2018

Conversation

@imath
Contributor

imath commented Dec 23, 2017

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

This comment was marked as outdated.

Contributor

imath commented Dec 23, 2017

I'll update the PR to fix the failing tests if you think it's a good idea.

@youknowriad youknowriad requested a review from mtias Dec 28, 2017

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

This comment was marked as outdated.

@mtias

mtias Jan 1, 2018

Contributor

Not sure we should apply the capability to the whole block in this case. It should be ok for a user to pick an existing image from the library without uploading. Seems the components themselves (media upload button, draggable upload area) are the ones that should check for the capability and change how they are displayed.

This comment has been minimized.

@imath

imath Jan 1, 2018

Contributor

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...

This comment has been minimized.

@aduth

aduth Jan 4, 2018

Member

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 comment has been minimized.

@mtias

mtias Jan 4, 2018

Contributor

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?

This comment was marked as outdated.

@aduth

aduth Jan 16, 2018

Member

how should we handle editing an existing post that has images? Or a post that has an image placeholder?

@imath Is there anything which needs to be considered here?

This comment has been minimized.

@imath

imath Jan 17, 2018

Contributor

@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..)

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

This comment was marked as outdated.

@aduth

aduth Jan 4, 2018

Member

We shouldn't assume that _wpAPIDataPreload exists here, or at all. I'm not totally sure we'd want the registration to be handling the capabilities check anyways, or if it's fine to allow the block to be registered, but then check the capability at the time that it's relevant (i.e. forbid in the inserter, etc).

You might also imagine an existing post which had been authored by another user with the capability. If we don't allow the block to be registered, it may appear as broken.

This comment was marked as outdated.

@imath

imath Jan 4, 2018

Contributor

You might also imagine an existing post which had been authored by another user with the capability. If we don't allow the block to be registered, it may appear as broken.

Good Point. In the classic editor this can only happen if the upper roles (author, editor, administrator), keep the post as pending. As soon as the post is published, the contributor cannot edit it anymore.

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

This comment has been minimized.

@aduth

aduth Jan 4, 2018

Member

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

This comment has been minimized.

Contributor

imath commented Jan 4, 2018

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
gallery
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 imath force-pushed the imath:add/blocks-capability branch from 8844bd6 to 1216849 Jan 7, 2018

@imath

This comment has been minimized.

Contributor

imath commented Jan 7, 2018

@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:

disable

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).

toolbars

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.

more-more

@aduth

This comment was marked as outdated.

Member

aduth commented Jan 9, 2018

Good stuff!

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:

Do you think it will be clear to the user what's happening here? Wondering if it would be much more effort to include a tooltip explaining the disabling (which may or may not be difficult, since mouse events tend not to be fired for disabled elements).

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.

Perhaps related to #4097 ?

@imath imath force-pushed the imath:add/blocks-capability branch from 1216849 to 738e5d0 Jan 10, 2018

@imath

This comment was marked as outdated.

Contributor

imath commented Jan 10, 2018

Thanks :)

About the tooltip, i've tried to use the Tooltip component but i'm stuck as it is not supporting disabled elements. I think the user will understand he can't use the blocks, but i'm not sure he will figure out why, especially users who are not too aware of WordPress roles and capabilities. So I agree it's a bit too bad.

It looks like I have 2 lint errors, but i wasn't able to replicate them on my local environment, so I'll give it another look soon.

About the Autocompleter, yes i think it's best to wait for #4097 to be fixed, before adding the capability check to it.

@aduth

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

if ( isLocked || ! allowedBlocks.length ) {
return null;
}
const userAllowedBlocks = allowedBlocks.filter( b => ! b.capability || userCapabilities[ b.capability ] );

This comment was marked as outdated.

@aduth

aduth Jan 16, 2018

Member

I think there's a chance userCapabilities could be undefined here; likely not observed in general usage only because we preload the /users/me request but something which could easily break in future refactoring.

One option is to provide the third argument to Lodash's _.get, which is a default value in case capabilities is not available. We could assign this to an empty object.

Or: We could bypass the filter altogether by wrapping this in an if condition or including userCapabilities in the above condition to return null;.

Aside: I have a personal vendetta against abbreviated variable names. I'd rather we assign a full variable name block and just split this into multiple lines. The single line doesn't necessarily help readability, or at least not as much as I think the abbreviated variable name harms it.

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

This comment was marked as outdated.

@aduth

aduth Jan 16, 2018

Member

how should we handle editing an existing post that has images? Or a post that has an image placeholder?

@imath Is there anything which needs to be considered here?

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

This comment was marked as outdated.

@aduth

aduth Jan 16, 2018

Member

This is a long line, and I think we might consider splitting it across multiple:

<VisualEditorInserter
	insertBlock={ insertBlock }
	mostFrequentlyUsedBlocks={ mostFrequentlyUsedBlocks }
	user={ user }
/>

@imath imath force-pushed the imath:add/blocks-capability branch from 738e5d0 to 1897ae2 Jan 17, 2018

@imath

This comment was marked as outdated.

Contributor

imath commented Jan 17, 2018

Thanks for your review @aduth.

1897ae2 is applying them.

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

This comment was marked as outdated.

@noisysocks

noisysocks Jan 17, 2018

Member

Heads up: this logic has moved to selectors.buildInserterItemFromBlockType (see #4497).

@imath imath force-pushed the imath:add/blocks-capability branch from 1897ae2 to 77e56da Jan 21, 2018

@imath

This comment was marked as outdated.

Contributor

imath commented Jan 21, 2018

Hi @noisysocks thanks for your alert about this logic move.

@aduth I just updated the PR accordingly.

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

This comment was marked as outdated.

@noisysocks

noisysocks Jan 22, 2018

Member

We should be able to avoid this mutating side effect by passing userCapabilities into buildInserterItemFromBlockType and performing the check there.

@imath imath force-pushed the imath:add/blocks-capability branch from 77e56da to d43bbb6 Jan 23, 2018

@imath

This comment was marked as outdated.

Contributor

imath commented Jan 23, 2018

Thanks for your feedback @noisysocks d43bbb6 should avoid the mutating side effect.

@youknowriad

This comment was marked as outdated.

Contributor

youknowriad commented Mar 6, 2018

Sorry, we kind of forgot about this PR :) Do you think you can rebase it @imath so we can do a last check before merging. I know you're probably busy with WordCamp Paris, it can probably wait for the next week.

@imath

This comment was marked as outdated.

Contributor

imath commented Mar 6, 2018

Hi @youknowriad To be honest so do I! No problem, i'll rebase it this coming week-end, once the WordCamp Paris rush hours are behind us 😅

@gziolo gziolo self-requested a review Mar 7, 2018

@imath imath force-pushed the imath:add/blocks-capability branch from d43bbb6 to 6612b66 Mar 18, 2018

@imath

This comment was marked as outdated.

Contributor

imath commented Mar 18, 2018

Hi @youknowriad it took a while longer than i thought! Sorry for the extra delay. I've just fixed merge conflicts, rebased and tested everything again 😉

@gziolo

This comment was marked as outdated.

Member

gziolo commented Mar 18, 2018

It looks like that API returns an array with capabilities. Is it possible that block would require a check for more than one capability? At the moment we introduce only check for images, but I wonder if it should be represented as an array in the case when a user needs to have more permissions to edit a given block.
I'm also not convinced that the name capability is the best choice in here. It might be confusing for the plugin developers because it is a user's capability, not block's. It would be nice if it were clear that it is a required user's capability.

@imath

This comment was marked as outdated.

Contributor

imath commented Mar 19, 2018

Hi @gziolo

Thanks for your feedback. I can't think of an example where WordPress would require more than one capability to be able to do something. I think using an array to check for more than one capability would introduce some complexity eg: how should values in the array be treated: the user needs to have one of listed capability or all of them ? If a developer needs more than one regular capability, he could use a custom capability like use_my_custom_block and use the map_meta_cap filter to deal with it during the REST API request.

About the name: userCan, userCapability or something else ? I'm fine with whatever you think is best 😉

@gziolo

This comment was marked as outdated.

Member

gziolo commented Mar 22, 2018

@imath what is your opinion about performing this kind of check on the server instead? We are looking into moving the block's registration to the server and having non-UI properties provided there. See related issue: #2751.

imath added some commits Oct 25, 2018

Use OPTIONS Rest API Media Request to get user `upload_file` capability
Edit the preloaded path and adapt the way apiFetch is preloading the requests using the OPTIONS method.
Add a hasUploadPermissions property to core/data
Introduce the MediaUploadCheck component
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.
Display a message to the contributor about the featured image
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.
Enforce strict format of the preloaded path when an array is used
This is to bring into Gutenberg the improvement @danielbachhuber committed in WordPress 5.0 branch
See https://core.trac.wordpress.org/changeset/43833

@imath imath force-pushed the imath:add/blocks-capability branch from c6f922c to 9238c48 Nov 13, 2018

@imath imath force-pushed the imath:add/blocks-capability branch from 9238c48 to 80ffbf6 Nov 13, 2018

@imath

This comment has been minimized.

Contributor

imath commented Nov 13, 2018

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.

@mtias mtias referenced this pull request Nov 14, 2018

Closed

"upload_files" capability and Image relative blocks #3672

0 of 2 tasks complete
@antpb

This comment has been minimized.

Contributor

antpb commented Nov 14, 2018

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

@imath

This comment has been minimized.

Contributor

imath commented Nov 15, 2018

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 ?

@talldan talldan removed their assignment Nov 15, 2018

@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.

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

This comment has been minimized.

@youknowriad

youknowriad Nov 15, 2018

Contributor

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.

This comment has been minimized.

@youknowriad

youknowriad Nov 15, 2018

Contributor

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.
export default withSelect( ( select ) => {
let hasUploadPermissions = false;
if ( undefined !== select( 'core' ) ) {

This comment has been minimized.

@youknowriad

youknowriad Nov 15, 2018

Contributor

Same comment as above.

It had several iterations since the time Andrew reviewed it

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 15, 2018

@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.

@gziolo gziolo merged commit bcf55ff into WordPress:master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@imath

This comment has been minimized.

Contributor

imath commented Nov 15, 2018

Thanks a lot for your help everyone 😍🎉

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

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

This comment has been minimized.

Contributor

imath commented Nov 15, 2018

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

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

Oh good thanks @imath

@imath imath deleted the imath:add/blocks-capability branch Nov 15, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Verify 'upload_files' capability when displaying upload UI in media b…
…locks (WordPress#4155)

* Use OPTIONS Rest API Media Request to get user `upload_file` capability

Edit the preloaded path and adapt the way apiFetch is preloading the requests using the OPTIONS method.
Add a hasUploadPermissions property to core/data

* Introduce the MediaUploadCheck component

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.

* Display a message to the contributor about the featured image

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.

* Enforce strict format of the preloaded path when an array is used

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

* Make sure the Inline Image is only available to users with the upload_files cap

* Update generated data-core docs

* Apply @gziolo recommandations and fix a failing unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment