sausin / laravel-ovh

Wrapper for OVH Object Storage integration with laravel
MIT License
37 stars 12 forks source link

Add form post signature #58

Closed sausin closed 4 years ago

sausin commented 4 years ago

First iteration on implementation as discussed in #48

iksaku commented 4 years ago

Looking good! This is more or less what I developed previously in a test.

One thing left to test (at least for me) is to check whether you can use Custom Domains to submit Form Posts, or if you would need to use OVH’s URL format for that.

sausin commented 4 years ago

One thing, I am thinking that the expires field should not be optional. The user will have no way of guessing the correct time when using the actual form. Unless it's specified, there is maybe no point in generating a signature for the sake of it.

Also, one thing that should change is that the defaults should maybe come from config. The config options are really become too large for something simple, but I suppose it would be nice to have the possibility.

The server resources will be used when uploading using the custom endpoint. Maybe it's better to use the regular upload when doing that? It should probably work though. Would you be okay to test it?

iksaku commented 4 years ago

I think it’s okay to enforce expiration parameter for the generation function. User should be able to truly know what they’re doing.

I’ll be on my way to keep testing against custom endpoints, hopefully they’ll work. Otherwise, traditional url should be used. The main reason to use Form Post option is to relax the stress of using a server as a middleman for file uploading.

iksaku commented 4 years ago

I've encountered a weird situation regarding CORS... If you upload via Javascript (axios) you can hit an error regarding Access-Control-Allow-Origin not being allowed. This happens when uploading directly to OVH's url scheme (No custom domain). A 'simple' workaround is to set the X-Container-Meta-Access-Control-Allow-Origin header in the container to allow all (*) domains.

EDIT: Javascript upload works fine without setting up the corresponding CORS headers, but will throw an annoying error on browser's console.

From CLI, the command would be:

swift post <container> -H "X-Container-Meta-Access-Control-Allow-Origin:*"

I'm not an expert with CORS, but allowing every domain doesn't seem harmful while using Signed petitions.

Read more about CORS and Openstack

iksaku commented 4 years ago

FormPost using Custom Endpoints works flawlessly! Tested against Cloudflare DNS using Flexible SSL.

Below is the code I used to test the From Upload.

web.php file ```php Route::get('/', function () { return view('welcome'); }); Route::get('signature', function () { $path = ''; $redirect = ''; $expires = now()->addMinutes(5)->getTimestamp(); $max_file_size = 1 * 1024 * 1024; $max_file_count = 5; $options = compact('path', 'redirect', 'expires', 'max_file_count', 'max_file_size'); $adapter = Storage::cloud()->getAdapter(); $options['signature'] = $adapter->getFormPostSignature('', $options); $options['url'] = $adapter->getUrl($path); return response()->json($options); }); ```
welcome.blade.php file ```blade OVH Playground
```
sausin commented 4 years ago

I've encountered a weird situation regarding CORS... If you upload via Javascript (axios) you can hit an error regarding Access-Control-Allow-Origin not being allowed. This happens when uploading directly to OVH's url scheme (No custom domain). A 'simple' workaround is to set the X-Container-Meta-Access-Control-Allow-Origin header in the container to allow all (*) domains.

EDIT: Javascript upload works fine without setting up the corresponding CORS headers, but will throw an annoying error on browser's console.

I suppose the right thing to do would be to have a command option in the package to set these headers. I'll add separate PR for the command, but the release can be together with the command in place.

Thanks for testing :+1:

iksaku commented 4 years ago

Talking about configuration, I think we should not provide a configuration proxy for this, as it should be intended for the developer to place stricter rules on the data submission by end users.

Config defaults may let some developers relax the limits about what their users upload, this way, we encourage them to keep an eye on default values each time they implement a FormPost

iksaku commented 4 years ago

Once this is merged, I would like to send another PR before tagging the release, looking into full param list and a way to abstract signature logic with TempUrlKey (If possible, if not, just params)

sausin commented 4 years ago

Talking about configuration, I think we should not provide a configuration proxy for this, as it should be intended for the developer to place stricter rules on the data submission by end users.

Config defaults may let some developers relax the limits about what their users upload, this way, we encourage them to keep an eye on default values each time they implement a FormPost

Agree!