Amazon S3 CORS Upload like functionality.

Moving this over to flysystem_s3. There might be some CORS features that we can add to Flysystem proper, but it would make sense to implement it here and move over reusable parts later.

I don't have any immediate plans to implement this feature. But, I will gladly except patches.

Nothing in particular. I'm not terribly familiar with the CORS setup.

They should look at the existing modules:

1. https://www.drupal.org/project/amazons3_cors
2. https://www.drupal.org/project/s3fs_cors

We actually have a ticket to implement CORS support that we were planning on starting this week. If anyone starts work on this, please be sure to assign the ticket so we don't duplicate work. At the least, it sounds like we can tag-team the patch and testing process.

Here's the WIP from Friday, still working on making the widget itself work and the JS to upload. I've got a route that takes the form input and returns the POST data with the signed response with the PostObjectV4. A basic widget that only allows itself to be used if the file field is configured to upload to an S3 system.

More progress, getting it to upload to S3, but I'm getting the following response back from the POST upload:

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>Invalid according to Policy: Extra input fields: bucket</Message>...</Error>

Got upload working and a progress bar. Working on polishing it up.

Aha! The #flysystem_s3_cors_managed_file element is working now! There are some small bugs but we can start using it now. Still working on the field widget, but a lot of progress.

This is looking great and well on the way! I would say my biggest overall feedback is to try and reduce the conditional nesting in some of the methods, but all of that should be done after something is working anyways.

1. +++ b/flysystem_s3.js
   @@ -0,0 +1,192 @@
   +        var extensionPattern = drupalSettings.file.elements[field_id].replace(/,\s*/g, '|');
   

   It might be good to document what this regex is doing.

2. +++ b/flysystem_s3.js
   @@ -0,0 +1,192 @@
   +                /*$file.parent().find('input[name$="[filemime]"]').val(f.type);
   

   What's up with this?

3. +++ b/flysystem_s3.libraries.yml
   @@ -0,0 +1,7 @@
   +drupal.file:
   

   I think this library needs to be renamed? Or are you overriding drupal.file somehow?

4. +++ b/flysystem_s3.module
   @@ -0,0 +1,5 @@
   +  //dpm($form_id);
   

   Dead code here?

5. +++ b/flysystem_s3.routing.yml
   @@ -0,0 +1,14 @@
   +    _permission: 'access content'
   

   This seems super broad to me. Do we really want anonymous users to be able to upload content?

1. +++ b/flysystem_s3.routing.yml
   @@ -0,0 +1,14 @@
   +flysystem_s3.test:
   

   Should this be in the main module or in a testing module?

2. +++ b/src/Controller/S3CorsUploadAjaxController.php
   @@ -0,0 +1,82 @@
   +    // @todo Inject the request?
   

   Since this is in the controller, the last parameter of the controller callback should be Request automatically.

3. +++ b/src/Controller/S3CorsUploadAjaxController.php
   @@ -0,0 +1,82 @@
   +    $post['key'] = file_uri_target(file_create_filename($post['filename'], $post['destination']));
   

   Can we call OO code here?

4. +++ b/src/Element/S3CorsManagedFile.php
   @@ -0,0 +1,430 @@
   +    $class = get_called_class();
   

   I think since php 5.4 you can just use static::$class ?

5. +++ b/src/Element/S3CorsManagedFile.php
   @@ -0,0 +1,430 @@
   +      '#acl' => 'private',
   

   This should probably come from the stream wrapper config?

6. +++ b/src/Element/S3CorsManagedFile.php
   @@ -0,0 +1,430 @@
   +                  $token = NestedArray::getValue($form_state->getUserInput(), array_merge($element['#parents'], array('file_' . $file->id(), 'fid_token')));
   

   This is quite the set of nesting! Can we break this down at all into new methods to make it easier to follow?

7. +++ b/src/Form/S3CorsUploadTest.php
   @@ -0,0 +1,96 @@
   +      '#upload_location' => 's3://2016-08',
   

   Will this pass on the testbot and so on where the scheme isn't set?

8. +++ b/src/Plugin/Field/FieldWidget/S3CorsUploadWidget.php
   @@ -0,0 +1,108 @@
   +use Aws\S3\S3ClientInterface;
   

   Wow, this is a huge number of imports! Any way we can pare this down, or is this a good signal to split up the class?

9. +++ b/src/Plugin/Field/FieldWidget/S3CorsUploadWidget.php
   @@ -0,0 +1,108 @@
   +    $element['s3_acl'] = [
   

   We talked about how this can probably be removed since we have scheme settings.

It might be good to document what this regex is doing.

Yeah, I am puzzled because the built-in Drupal.file.validateExtension is supposed to run before uploading, but doesn't (even for regular file field widgets). I just basically copied what that code does because I cannot call it directly (it's assumed be be invoked from a change.fileValidate jQuery event, which is too late for us).

What's up with this?

Original code from the amazons3_cors module JS, I can just remove it for now (common theme here).

This seems super broad to me. Do we really want anonymous users to be able to upload content?

This actually matches what the file.ajax_progress route does, and it can only be a POST request. Note it doesn't actually upload, it just returns a signed response. I did waver on this if it's a security issue. I might need to add a token protection.

I think since php 5.4 you can just use static::$class ?

Nope, doesn't appear to work: https://3v4l.org/c24ea

This is quite the set of nesting! Can we break this down at all into new methods to make it easier to follow?

Literally copied from ManagedFile, so no. :)

This should probably come from the stream wrapper config?

I feel like it should be possible to override the #acl on the individual upload element (and widget), but we can definitely default to NULL and load in the default from the scheme config.

A much cleaner version now, including improved security by adding a new permission 'use S3 CORS upload'. If the user doesn't have that permission, than the file upload falls back to using the default file upload functionality.

I'm *almost* thinking that there might not be a need for a separate form element, why couldn't we just convert existing file uploads to use CORS, instead of needing separate widgets. It could be an option supported in the existing file field upload widget. I'll leave that for a little bit. Going to work on the field widget and make sure that works completely.

That wasn't actually the latest version.

Updated, cleaning more things up.

I'm working on a update to this that just alters the managed file element, instead of adding a new element. So S3 CORS uploading will "just work" for any managed file element that has an upload location set to an S3 file system.

Wanted to say thanks for working on this, and it looks great. Will do a thorough review when #17 arrives.

So there's no extra configuration, either on the AWS side, or the client side, that is necessary for CORS uploading to function?

Thanks for the work on this one.

Not sure what could go wrong, but the new "S3 CORS File Upload" widget in the most basic Content Type still won't allow to surpass the server file size limit. Even though files go straight to S3. Checked twice on a clean D8.1.8 with Flysystem, Flysystem - S3, and after applying #16; PHP 5.6.10. Will appreciate any hints.

@gravedanger: Yes, you'd need to bump up the file size limit. Maybe I can alter that requirement out for now.

WIP that switches to just altering the existing file_managed element.

Abandoning work on altering file_managed for now. I want to come back to it but this works for a client in the meantime.

I couldn't let it go. I GOT IT TO WORK BY JUST ALTERING managed_file ELEMENTS! This means I can also drop the extra Element and FieldWidget plugins that are no longer necessary!

Added a config option to enable the CORS support, documented in the README.

So I noticed that the ManagedFile::valueCallback() was still doing a PHP upload of the file, so I attempted to figure out how to block the AJAX submit handler from executing until we could complete the CORS upload. I've been testing this extensively and I think it's working, it registers a temporary file in the signing request which returns a file ID. When the CORS upload is complete, it adds the file ID to the hidden element, removes the actual selected file from the upload field, and clicks the submit button. I added more docs and some todos about the limitations of this support being only for single-value managed file elements.

Patch applies cleanly and works as expected.

I'm working on an improvement to just use the S3 API to determine if CORS can be used to POST from the current domain, rather than requiring it to be set in the flysystem config.

Any thoughts on if this should be moved to a separate flysystem_s3_cors sub-module so it could be optional?

Here is a small adjustment where we check that drupalSettings.file exists before diving into its elements. The reason why this is needed is that if none of the form file elements have field extension validation via file_validate_extensions, then drupalSettings.file is not set.

Here's a version that adds the extensions as a data attribute to the upload element, which gets rid of the brittleness with element IDs changing with AJAX, and using drupalSettings when the data should just be attached to the element itself.

#29 is working well for me. Here's a docs update that includes an example CORS configuration.

Also, the above fix is somewhat related to #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?).

Fixes removing the VERSION constant from the libraries YML.

+++ b/flysystem_s3.js
@@ -0,0 +1,215 @@
+          var extensionPattern = $fileElement.data('valid-extensions').replace(/,\s*/g, '|');
...
+            var acceptableMatch = new RegExp('\\.(' + extensionPattern + ')$', 'gi');

The regex seems to be not working in case there are multiple file extensions.
As it was copied from Core, the same applies there but it seems it's going to be fixed in #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?).

Other than this, #32 works great!

We also had a problem where CORS doesn't take path prefix into account.

Providing a fix.

Works great with those two fixes for us. Back to RTBC.

Actually I was wrong, that remove Path prefix call doesn't seem right.

Adding a new patch that fixes prefix handling.

I couldn't get this to work with an image element since template_preprocess_image_widget() blows away all attributes. Moved the selector to a data attribute.

Other minor style fixes.

I don't know why S3CorsManagedFileHelper was static caching settings. That shouldn't be needed.

Last question: Are we sure that S3CorsManagedFileHelper::getAcl() should default to private? Based on https://github.com/thephpleague/flysystem-aws-s3-v3/pull/85, we can set the ACL to null.

Switched to a data- attribute. Using closest() rather than parents().

In an effort to get this to work with multiple files, I started refactoring the JS.

It's still working, as far as my tests go, but I ran out of time for the day.

I'm fine fine with postponing that to another issue, but the Js code is a bit cleaner.

I did find a possible problem with file validation though:
S3CorsUploadAjaxController::signRequest() should do more to validate the file being uploaded and be able to return a failure if the file is invalid. Meaning invalid extension or file size. As it currently stands, you can upload a file, it will fail validation eventually, but it's still available from AWS until the temp file gets deleted.

I'm running into an issue when I apply this patch. I get "PHP Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 260833309 bytes) in... flysystem_s3.module line 9" the moment I enable the module after patching.
The offending line is - S3CorsManagedFileHelper::alterInfo($types);

Unfortunately I can't alter the memory settings of my host, however that seems like an unusually high amount of memory usage. The base module without the patch runs fine.

Is anybody able to provide suggestions/assistance? I'm running D8.2.5 If you need more info let me know. Any help is appreciated.

*Edit - Also on my localhost I set my file max size fairly low, 8MB, for testing purposes. When I upload a file that's larger than that it successfully uploads to S3 then immediately after finishing throws two of the same error - file is too large. If I hit the save button at the bottom of the form it fails to create the node because the file is too large.

Hi Again,
I wanted to check back in. You can disregard my previous message about the memory issue.
The major issue I'm seeing is still the file size limit being imposed. Am I correct in understanding that this patch is working? If so, is this error a misconfiguration on my part?
My use case is to avoid the file size limit set by my host (100MB) by uploading directly to S3. I tried looking through the S3FS CORS module but couldn't figure it out there. I kept running into the issue where Drupal uploads the file itself even if you clear the file field before hitting save, thus failing the upload because it goes to a temp directory.

*Edit: I was able to get around the first file size error by adding "$types['managed_file']['#upload_validators'] = [];" as a test snippet. It then uploads the file successfully and after that fails out because the file is too large.

**Edit 2: I figured out a workaround. It's a bit hackish but it works. Essentially created a new field within the module that extends the file field and then overwrote the "getUploadValidators" function to remove the php file size check (still has manually set file size check).

We now have a client who needs uploads to support files larger than 5GB, which this current patch doesn't support. Implementing that will require bringing in the AWS JS SDK so we can do multipart uploads. I'm on the fence if we should just iterate on this patch, or try to get this committed with <5GB support. Thoughts?

I'm fine with moving forward with this patch and supporting large files in another. Either way works for me.

I'm curious if the AWS SDK will fix @matt.schuh's issue as well.

Just a minor fix for a PHP Notice in the getAcl(), taking into consideration ACL = NULL based on comments in #38.

array_key_exists('ACL', $settings['config']['options']['ACL']))
ACL is a string.

I really would like to see this feature, but with the recent updates in the code, this need update.

#46 patch file is not applying to the 8.x-1.0-beta1 released 22 October 2018.
The problem was in the README file part.

I updated that patch file. Some of the functions will be soon deprecated. Now, this patch should be D9 compatible.

Added multiple files uploading support to this patch.

Fix for a case when files with the same names as the uploaded file are already in the bucket.

Thanks! I have merged this into the latest dev.

One more fix to the patch. In the last commit #52 I did one change too much.
@MiSc - Sorry but we should update also Dev.

So it was missing stream wrapper from the path eg foobar://. Can somebody add direct commit or is patch needed for dev branch?

Patch #52 was merged to the Dev. But this last change I did #54 was also needed to get Cors upload working.
Can this patch #54 be merge to the Dev release or I need to create a separate patch for that last changed based on Dev code-base?

It would be nice to get Cors upload functionality to merge to this module because I have worked already on new features based on the flysystem_s3 module and Cors upload functionality.

+1 @misc

Here's a patch derived from #54 which applies cleanly. Edit: whoops it doesn't :-/

HERE is a new patch containing the change from #54 derived from current dev

All uploaded files using CORS are being uploaded with mime type Content-Type: binary/octet-stream.

This makes it hard to host files like images when using option ['public' => TRUE] also any app that looks at the Content-Type header.

I have added changes from #59, this can be merged with 8.x-1.x and 2.0.x

What about https://www.drupal.org/project/flysystem_s3/issues/3131517:

• Files with mime application/* get downloaded and theyr name is what was originally uploaded (without Drupals suffix)
• All other mimes get content types set - trick was to add mime to $options and $post

What does starts-with do?

PS. We should close this and start new issues.

@mkhamash

Found an issue with this when using the new Claro core theme. Created a separate issue (with patch), linking to it from here.

Tagging another issue that also effects CORS uploads. The changes from that will supersede those from the patch on #60.

I've incorporated changes from #60 in #3248466: Duplicate file entities when uploaded through CORS [DATA LOSS], which has now been merged and released.