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

Url with expires #15

Closed
rosskevin opened this issue Oct 11, 2017 · 11 comments
Closed

Url with expires #15

rosskevin opened this issue Oct 11, 2017 · 11 comments

Comments

@rosskevin
Copy link
Contributor

rosskevin commented Oct 11, 2017

I see in #7 that signed urls are implemented with presign.

I'm thinking about access to an expiring url from an existing activerecord model with a file, and the implementation here seems like a different pattern than that of the S3#url.

I'm new to shrine, so please point out what I'm missing.

Assuming a model Agreement and a shrine field document_data, with S3 docs it looks as though you should be able to agreement.document.url(expires_in: 300).

Shouldn't the expected usage here be agreement.document.url(expires: 300) based on the above pattern, the UploadedFile api, and the GCS Using signed urls?

  • Is that correct?
  • Should this gem match the usage (except making options align with GCS, in this case expires)?
  • Can I already exec presign from my model and I'm missing it?
@renchap
Copy link
Owner

renchap commented Oct 11, 2017

You can see an example here : https://github.com/renchap/shrine-google_cloud_storage/blob/master/test/gcs_test.rb#L139

GCS requires you to specify an issuer and a signing key when generating a presign URL, and those are different than your Google Cloud API key. So supporting the expires argument in url() means also adding the issuer and signing_key args, and this start to ressemble a lot to the presign method.

@janko-m any advice here on which API would be best to keep it coherent with other Shrine storages ?

@rosskevin
Copy link
Contributor Author

rosskevin commented Oct 11, 2017

So the current usage for an existing file is:

uploaded_file = agreement.document
presigned_url = uploaded_file.storage().presign(uploaded_file.id(), options)

It seems like we might want #url to delegate to #presign based on the presence of :issuer, so instead it would look like S3's implementation and become

agreement.document.url(options)

If you are good with that I can PR, unless you want to do it.

@rosskevin
Copy link
Contributor Author

rosskevin commented Oct 11, 2017

In my case I run within GKE (google container engine), so based on the API doc it looks like I don't need to provide any credentials because of default discovery (I think).

Generating a URL requires service account credentials, either by connecting with a service account when calling Google::Cloud.storage, or by passing in the service account issuer and signing_key values.

@renchap Am I missing something here?

That would also make the desire for #url to #presign behavior based on presence of :expires? or something else like public from the S3 api.

@rosskevin
Copy link
Contributor Author

rosskevin commented Oct 11, 2017

I notice now that you are not using gem 'google-cloud-storage', but gem 'google-api-client', which might change things regarding 1.6.0 and discovery of credentials. Any reason you went that direction? It looks to me that the google-api-client is no longer the prefered gem and is in "maintenance mode".

I'm going to work on a conversion to google-cloud-storage and I'll submit a PR.

@janko
Copy link
Contributor

janko commented Oct 11, 2017

@renchap Shrine doesn't have any convention for expiring URLs, :expires_in option in Storage::S3#url is forwarded directly to the aws-sdk-s3 gem. The reason why the separate #presign method exists is because expiring URLs generally don't have to be equal to URLs for direct uploads.

In GCS case that seems to be true, so it seems it might be a good idea for GoogleCloudStorage#url to support an :expires option, and in that case call #presign. Alternatively things could be simpler with the google-cloud-storage gem if it's feasible now. FYI, I noticed a PR was merged some time ago which adds support for uploading generic IO objects (in addition to files).

@rosskevin
Copy link
Contributor Author

rosskevin commented Oct 11, 2017

@janko-m the only thing I see missing at this point in 1.6 is batch delete. How important is it to retain batching here?

It appears that batching will not be supported: googleapis/google-cloud-ruby#1008

... batch is considered a legacy feature. It is not supported in new gRPC-native APIs.

@janko
Copy link
Contributor

janko commented Oct 11, 2017

@rosskevin Implementing the #multi_delete method is relatively unimportant. It's nice to have for performance when using versions without backgrounding, but users can just use backgrounding and/or the parallelize plugin.

Though it would be nice to still be able to implement #clear!, which is meant to be used when GCS is also used for temporary storage.

@renchap
Copy link
Owner

renchap commented Oct 12, 2017

@rosskevin If you want to have a look at switching to google-api-client, feel free to do so! When I started this project it was new and did not support handling IO objects. From what I remember this was the main reason not depending on it.

I am fine with allowing presigned urls in url(). I duplicated most of the presign method from google-cloud-storage so it should not be hard to tie back them together.

The SA used to presign the URLs needs to be allowed to be specified as a parameter to the presign/url call when you want to use a different account for presigning. It may be a good idea to allow to specify it when creating the storage too. When you run the code on your computer most of the time you want to use your gcloud credentials, but as you need a service account to presign URLs it could be nice if you can configure it when setting up the Shrine storage (like a presign_sa option on the uploader). Then you would not initialize it when running in production and it uses the discovered GCP credentials, as they will be a service account.

@rosskevin
Copy link
Contributor Author

PR submitted, I think it is complete/mergeable.

@rosskevin
Copy link
Contributor Author

Ah @renchap, I just saw your note.

You can definitely still sign with your own, I verified that. You can also override and specify your own project when instantiating storage. As far as the myriad of other credential options, I didn't try to do anything else as I don't know the use-case. I do not think it will be hard to expand my implementation once someone raises specifics though.

@renchap
Copy link
Owner

renchap commented Nov 19, 2017

Fixed by #16

@renchap renchap closed this as completed Nov 19, 2017
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

No branches or pull requests

3 participants