search

GoogleForCreators / web-stories-wp

Web Stories for WordPress
https://wp.stories.google
Apache License 2.0
767 stars 178 forks source link

Authors cannot crop images they haven't uploaded themselves #9241

Closed swissspidy closed 1 year ago

swissspidy commented 3 years ago

Bug Description

In WordPress, authors don't have capability to edit an attachment they haven't uploaded themselves.

That means they also cannot crop an attachment they haven't uploaded themselves.

Expected Behaviour

Don't offer cropping if lacking permission

Perhaps don't offer inserting non-cropped image at all in that case.

Steps to Reproduce

  1. Log in as author
  2. Create a new story
  3. Try setting a new publisher logo or story poster image and crop it
  4. See capability error

Screenshots

https://user-images.githubusercontent.com/237508/135874128-8f114b64-bc45-40e8-a6f6-d4e74d131bec.mov

Additional Context

swissspidy commented 3 years ago

cc @spacedmonkey

spacedmonkey commented 3 years ago

We have a couple of options here.

  1. Add a new nonce to the admin ajax output, so the cropping will work. https://github.com/google/web-stories-wp/blob/373ee1bbde7b433256c470e02ea31a8a6c5b93d4/packages/wp-story-editor/src/components/mediaUpload/mediaPicker/WordPressImageCropper.js#L57
  2. Compare the attachment's author id, with the currentUser and skip the cropping stage all together.
  3. Hide the media picker all together, if can not edit others attachments.
swissspidy commented 3 years ago

We have a couple of options here.

  1. Add a new nonce to the admin ajax output, so the cropping will work. https://github.com/google/web-stories-wp/blob/373ee1bbde7b433256c470e02ea31a8a6c5b93d4/packages/wp-story-editor/src/components/mediaUpload/mediaPicker/WordPressImageCropper.js#L57

Wait, just adding a nonce would work? 🤔 Doesn't wp_ajax_crop_image do any more capability checks?

I assume you would filter wp_prepare_attachment_for_js to add the nonce?

For reference, here's where the current nonce is set:

https://github.com/WordPress/wordpress-develop/blob/c69c34273a7537ba969c8e93d359735a1b58bb3a/src/wp-includes/media.php#L4047-L4051

If this really works, then I'd be tempted to give it a try.

Drawback: this allows authors to crop images, which they usually are not allowed in WP.

  1. Compare the attachment's author id, with the currentUser and skip the cropping stage all together.

We should leave this sort of check to WP and the logic in map_meta_cap.

Also, if we just skip the step it means they'll choose an image with incorrect sizes.

  1. Hide the media picker all together, if can not edit others attachments.

That is very drastic. It would mean authors could no longer pick an image even if it doesn't need cropping.

Perhaps another option:

  1. Just don't allow choosing an image if it needs cropping but user does not have capability. Skip cropping if image is already cropped perfectly.
spacedmonkey commented 3 years ago

Just don't allow choosing an image if it needs cropping but user does not have capability. Skip cropping if image is already cropped perfectly.

I don't think this is possible. We can't easily filter attachments listed, by either size or edit_post capability.

I think option 1, is the cleanest. It should be as simple as this,

Add the following lines here. 

if ( current_user_can( 'upload_files' )) {
    $response['nonces']['web_stories_edit']   = wp_create_nonce( 'image_editor-' . $attachment->ID );
}

And change this line to 

      nonce: attachment.get('nonces').['web_stories_edit'],

As we are using our own key in the object, it should not effect core image crop. To be extra careful we could also copy and paste, the wp_ajax_crop_image function into our code and change the nonce all together.

Another, not use admin ajax and just use REST API image edit endpoint that has a different permission model.

swissspidy commented 3 years ago

Just don't allow choosing an image if it needs cropping but user does not have capability. Skip cropping if image is already cropped perfectly.

I don't think this is possible. We can't easily filter attachments listed, by either size or edit_post capability.

I don't mean filtering attachments, but just disable the"Insert" button if the image needs cropping but the user doesn't have the capability.

I think option 1, is the cleanest. It should be as simple as this,

Add the following lines here.

if ( current_user_can( 'upload_files' )) {
  $response['nonces']['web_stories_edit']   = wp_create_nonce( 'image_editor-' . $attachment->ID );
}

And change this line to

      nonce: attachment.get('nonces').['web_stories_edit'],

Are you confident this works?

Doesn't this mean the user technically edits an image even though they don't have the capability?

spacedmonkey commented 3 years ago

As a talking point, I create a POC PR to get cropping work as a author. See what you think.

swissspidy commented 3 years ago

That is a lot of code. I really don't like just copying over wp_ajax_crop_image and want to avoid that if at all possible. Do we really need our own image_editor_web_story nonce? Can't we just use the image_editor- nonce and the wp_ajax_crop_image function from core?

Doesn't this mean the user technically edits an image even though they don't have the capability?

spacedmonkey commented 3 years ago

Just don't allow choosing an image if it needs cropping but user does not have capability. Skip cropping if image is already cropped perfectly.

Worth, noting, if the image is already the correct aspect ratio, it will skip the cropping stage all together. But without overriding a lot of the backbone code, it will hard to disable the selection button, when a image is selected. It might be do-able, but testing / implement hacks on backbone take a lot time.

Another idea, that came up in the testing. Add another capability check to the config object, to see if you edit others posts.

Then here https://github.com/google/web-stories-wp/blob/d6deee0f81f3b4080de1c5af5784eafcdbc3668c/packages/wp-story-editor/src/components/mediaUpload/mediaPicker/useMediaPicker.js#L205

Change it to 

library: wp.media.query({ type: author: currentUserId }),

This would only show image that the current user uploaded.

spacedmonkey commented 3 years ago

Can't we just use the image_editor- nonce and the wp_ajax_crop_image function from core?

No, sadly, I did not notice this line.

This does the edit_post check. Which is why I needed to copy the function, to omit this line.

Doesn't this mean the user technically edits an image even though they don't have the capability?

Yeah it does. But I honestly don't understand the capability check. The author user could, download the file locally, edit it and reupload it. You are not editing the file, you are copying it. So the check is invalid IMO.