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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix presigned uploads #23

Merged
merged 4 commits into from
Feb 13, 2018
Merged

Conversation

janko
Copy link
Contributor

@janko janko commented Feb 13, 2018

Shrine's presign endpoint returns URL, fields and headers that the client (browser) needs to upload to. The Shrine::Storage::GoogleCloudStorage#presign method, which the presign endpoint calls, never returns any headers, which means that the presign endpoint always returns "headers": {} in the response. When the browser then tries to upload the file to the URL, but because the request headers required by GCS weren't included, the upload fails.

The solution is for Shrine::Storage::GoogleCloudStorage#presign to return appropriate headers when either :content_type, content_md5 or headers options were given, which will make the presign endpoint automatically return them, and then the client (browser) can then include them in the upload request.

I also updated the :method to be PUT (the default is GET), because presigned URLs are meant for uploading, not downloading. Uploading doesn't work if method: "PUT" is not specified.

Btw, I also switched from Net::HTTP to HTTP.rb, because Net::HTTP has a really horrible API 😛, while HTTP.rb has a really nice and intuitive API.

Fixes #22

@janko
Copy link
Contributor Author

janko commented Feb 13, 2018

Sorry, I just copied the explanation from the google-cloud-ruby issue, but I didn't provide the relevant context here. The reason for #22 is that when :content_type, :content_md5 and :headers presign options are passed in, then the upload request also needs to include the corresponding request headers.

screen shot 2018-02-13 at 02 01 10

This PR modifies #presign to also add the headers field, which will automatically get picked up by the presign_endpoint and returned in the response, to be picked up by the client for the upload.

@renchap renchap merged commit 023e7a2 into renchap:master Feb 13, 2018
@renchap
Copy link
Owner

renchap commented Feb 13, 2018

Thanks @janko-m, I planned to dig into this today but you beat me on it.
I indeed missed the fact that those headers needs to be returned by presign. I also thought that presign could be used to generate signed download URLs, but it seems that url with signing parameters should be use instead, so you are right again.

And HTTP.rb is definitely better than net/http, thanks :)

@janko janko deleted the fix-presigned-uploads branch February 13, 2018 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants