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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add media upload capability check #9301

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
4 participants
@talldan
Copy link
Contributor

commented Aug 24, 2018

Description

Changes

  • Introduces a MediaUploadCheck component, which can be used to wrap other components and prevent them from rendering if a user does not have adequate permissions (e.g. Contributor roles) to upload files/media. This is implemented in our MediaPlaceholder component and several media blocks (image, video, audio etc.)
  • Adds store state hasUploadPermissions, a boolean value which indicates whether the user can upload files/media
  • Adds a new resolver that makes an OPTIONS request to the media endpoint. If this returns 'POST' in its Allow header, the user id deemed able to upload media.
  • Preloads the OPTIONS request to the media endpoint.
  • Prevents users without upload permissions from making requests to the media endpoint with the context of 'edit' (since that's not allowed)

These changes were originally implemented in #4155, but have been split into a separate PR to make it easier to review since the history on that PR is very long - I've moved across any outstanding review comments. Would be great to get some fresh 馃憖 on this.

Closes #3672

Massive thanks to @imath for all the hard work he's put in developing this. 馃帀

How has this been tested?

  • Added unit tests
  • Created a user with the role 'contributor'. Tested that when adding blocks they're unable to upload media.
  • Also tested that a contributor cannot manipulate media already uploaded by an admin (which is unfortunately also forbidden)

Screenshots

Screenshot of how media blocks appear to contributors
screen shot 2018-08-24 at 2 45 07 pm

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@@ -1,13 +0,0 @@
/**

This comment has been minimized.

Copy link
@talldan

talldan Aug 24, 2018

Author Contributor

Seems a shame to remove these tests. I spent a while trying to find a way to mock 'select' and the hasUploadPermissions selector, but couldn't find a suitable way. Any pointers would be appreciated if there is a way to do this.

But I also thought that perhaps it would be better to spend the effort introducing e2e tests that cover the initial state of these blocks?

// array_reduce() doesn't support passing an array in PHP 5.2
// so we need to make sure we start with one.
if ( ! is_array( $memo ) ) {
$memo = array();
}
if ( empty( $path ) ) {
if ( empty( $args ) || empty( $args['path'] ) || empty( $args['method'] ) ) {

This comment has been minimized.

Copy link
@talldan

talldan Aug 24, 2018

Author Contributor

Comment from old PR
https://github.com/WordPress/gutenberg/pull/4155/files#r201324762

Aside: GET will certainly be the majority usage here. I'd expect it to be reasonable as a default value. Maybe even to the extreme (i.e. debatable) of supporting mixed value arrays like...

[
	'/get/to/path/1',
	'/get/to/path/2',
	[
		'path'   => '/options/to/path',
		'method' => 'OPTIONS',
	],
]

But this is less part of a public API than I originally expected, so I don't feel strongly about it at this point.

This comment has been minimized.

Copy link
@talldan

talldan Aug 24, 2018

Author Contributor

I'm not a fan of the idea of mixing types in the array. I feel like at least this way it's obvious what's happening.

I would maybe say that this could throw an error if the args are not provided, at the moment it just skips preloading the request when really it is an obvious user error if they're not provided.

Also not sure if we'd consider splitting some code out of client-assets, we might be able to make this a bit more expressive.

if ( 'GET' === method && preloadedData[ path ] ) {
return Promise.resolve( preloadedData[ path ].body );
if ( preloadedData[ method ] && preloadedData[ method ][ path ] ) {
const response = 'OPTIONS' === method ? preloadedData[ method ][ path ] : preloadedData[ method ][ path ].body;

This comment has been minimized.

Copy link
@talldan

talldan Aug 24, 2018

Author Contributor

Coment from old PR
#4155 (comment)

We should have a unit test for this. I'm also not sure why we ought to need to special-case OPTIONS requests. Who's to say we don't want other headers from the request? Just seems like we're making inconsistency for not an entirely valid reason.

This comment has been minimized.

Copy link
@talldan

talldan Aug 24, 2018

Author Contributor

I can see that this would be quite a big refactor, so maybe worth splitting into a separate PR?

I will add the tests though.

@@ -14,10 +14,13 @@ import apiFetch from '@wordpress/api-fetch';
import { getEntitiesByKind } from './selectors';
import { addEntities } from './actions';

const getEditContext = () => 'edit';
const getMediaContext = ( state ) => state.hasUploadPermissions ? 'edit' : 'view';

This comment has been minimized.

Copy link
@talldan

talldan Aug 24, 2018

Author Contributor

This code needs some work. If anyone has an ideas on how we can make entities 'permissions aware', that would be great!

One of the challenges is that there are race conditions (is hasUploadPermissions set by the time this is called). Also some issues around separation of concerns - entity declaration shouldn't really need to know about state structure.

* Requests Upload Permissions from the REST API.
*/
export async function* hasUploadPermissions() {
const response = await apiFetch( { path: '/wp/v2/media', method: 'OPTIONS', parse: false } );

This comment has been minimized.

Copy link
@talldan

talldan Aug 24, 2018

Author Contributor

I'm not really clear on why we prefer the approach of making an options request here over that of fetching all the users capabilities from the /users/me?context=edit endpoint. The latter would remove the need for all the code that specially handles OPTIONS requests.

@danielbachhuber if you're able to let me know some background behind that decision it would be much appreciated - seems like this is the relevant comment #4155 (comment)

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Aug 24, 2018

Member

I'm not really clear on why we prefer the approach of making an options request here over that of fetching all the users capabilities from the /users/me?context=edit endpoint.

See #6361

This comment has been minimized.

Copy link
@talldan

talldan Aug 27, 2018

Author Contributor

Thanks @danielbachhuber for the info.

That helps me understand why capabilities aren't used across Gutenberg.

Worth noting that for this PR, the permissions check is preloaded server-side during page load and cached. I don't think there's anything that invalidates the cache, the user would have to reload the page if they gained file upload permissions.

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Aug 27, 2018

Member

I don't think there's anything that invalidates the cache, the user would have to reload the page if they gained file upload permissions.

Yep, this is a reasonable assumption.

@talldan talldan changed the title WIP Add media upload capability check Add media upload capability check Aug 24, 2018

@talldan talldan requested review from WordPress/gutenberg-core and aduth Aug 24, 2018

@joemcgill

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

This is a really nice change, however, the MediaUpload component should remain available for someone without upload_files capabilities so they can select images from the media library even if they can't upload them.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

Hi @joemcgill Thanks for the feedback. There's a conversation about that on the original PR:
#4155 (comment)

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.

talldan and others added some commits Aug 22, 2018

Add MediaUploadCheck component and unit tests
- Hardcode hasUploadPermissions value to true for now until implementation of
server-side and redux state has been finalised

Co-authored-by: imath <math.viet@gmail.com>
Use MediaUploadCheck in MediaPlaceholder
MediaUploadCheck component will conditionally show/hide parts of the UI
related to file uploads dependent on whether the user has the capability
to upload files.

Co-authored-by: imath <math.viet@gmail.com>
Simplify rendering logic of PostFeaturedImage
Use early return when no featuredImageId is set. Reduce boolean checks around
component rendering.

Co-authored-by: imath <math.viet@gmail.com>
Hard code hasUploadPermissions in redux state instead of MediaUploadC鈥
鈥eck component

Co-authored-by: imath <math.viet@gmail.com>
Implement MediaUploadCheck wherever a user might interact with file u鈥
鈥loads

Co-authored-by: imath <math.viet@gmail.com>
Add state handling for upload permissions
Co-authored-by: imath <math.viet@gmail.com>
Preload OPTIONS request to media endpoint used to determine if user h鈥
鈥s upload permissions

Co-authored-by: imath <math.viet@gmail.com>

@talldan talldan force-pushed the add/media-upload-capability-check branch from dfcad5f to 4b4b4d6 Sep 25, 2018

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

Rebased this:

Prevents users without upload permissions from making requests to the media endpoint with the context of 'edit' (since that's not allowed)

This bit wasn't possible with master due to recent changes to resolvers, so it'll have to be reimplemented.

@mikeselander

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

@talldan I am really looking forward to this change - this will be a solid addition to G'berg.

I do have the same concern as @joemcgill though - this logic applies to Contributors and un-customized sites, but what about a situation where a developer would want to lock down the permissions for users to upload but not to select from existing media? I can see many cases where administrators would not want new uploads but would want existing media to be selectable. I, myself, have this very need and this change would wipe out all usability of the media library within G'berg media blocks.

Could we instead lock down only the FormFileUploader here and not block all access to media in the blocks themselves? https://github.com/WordPress/gutenberg/pull/9301/files#diff-56d6af193779467f7a4cb10e85c26ef3R135

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

Hi @mikeselander. The media library is already unusable for users with a contributor role, so this PR doesn't change that. It does at least introduce messaging to explain why that's the case, making the experience much less broken, as well as removing the UI for uploading.

My understanding is that if we wanted to allow contributors to select media from the media library, that would have to be introduced in core, here's a (fairly old) ticket that discusses that - https://core.trac.wordpress.org/ticket/23391

@mikeselander

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

@talldan I see that and it makes sense in the current context for Contributors.

However, we have a (albeit rather unique) need to customize permissions for regular users for a client. We will remove the permission to upload images but those user should still see and be able to select from the media library for usage within posts. The way that this PR is currently written those users won't have access to insert images like they should. Does that make sense? I can outline in more detail if not.

Additionally, and I'm going to speak out of term a bit here, but @joemcgill is the maintainer for media in core and I'm confident that we can push through the necessary changes to allow contributors to properly view media or limit their permissions in another fashion that would allow this UI to make sense.

@joemcgill

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@mikeselander @joemcgill This isn't something we can solve within the gutenberg project - I think it'd be beneficial to try to generate some traction in core trac tickets instead.

The changes here are in no way a permanent thing, if the core changes can be made then it should be pretty straightforward to expose the Media Library again for contributors.

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

@joemcgill @mikeselander I'm happy to help find a middle ground. I think we'd need to:

  1. Introduce a new capability around viewing the media library (independent of upload_files but defaulting to upload_files).
  2. Audit core for references to upload_files that could be switched to the new capability.
  3. Update Gutenberg once the implementation is agreed upon for core.

Feel free to pull me into conversations wherever needed.

@joemcgill

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

I don鈥檛 think we need a new capability, but instead need to check whether a person without upload caps can view others posts and if we can specifically check the attachment post type, that would be best.

The advantage of introducing a new capability is that it makes it possible for the site owner to modify the behavior. Here's existing prior art for plugins: https://core.trac.wordpress.org/changeset/41290

Is there prior art for the scenario you describe? And, in the scenario you describe, how would the site owner modify the behavior in a way that worked consistently with the rest of WordPress?

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@joemcgill @danielbachhuber politely - please could you keep the comments on topic and in the right place. I've pointed out the core trac issue a few times now, can you please relocate the conversation over to there.

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

@talldan Yep, sorry about that.

array(
'path' => '/wp/v2/media',
'method' => 'OPTIONS',
),

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Oct 1, 2018

Member

Related #10268

If we change the function signature, there may be some compatibility concerns.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

closing in favor of #4155

@youknowriad youknowriad deleted the add/media-upload-capability-check branch Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.