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

Adding bucket_as_host flag to generate signed url #156

Merged
merged 10 commits into from
Jan 25, 2022

Conversation

gr8Adakron
Copy link
Contributor

@gr8Adakron gr8Adakron commented Jan 18, 2022

The following PR will allow us to create signed url in the following nomenclature: https://custom.domain.com/image.jpg? which wasn't feasible before with this library. This allow us to be independent of amazon host. As in real time scenario when generating signed url for clients side application the IP whitelisting is a well known problem. Certainly this resolve the prior issue as one get better control over custom domain IPs.
Further benefits with been custom domain as host - provision us to configure authentication layer on access of presigned url too.

Inspired from the following issues:

@gr8Adakron
Copy link
Contributor Author

@bernardd FYI

fixing formatting issue
fixing formatting issue for s3.ex
@bernardd
Copy link
Contributor

bernardd commented Jan 23, 2022

Hi @gr8Adakron - the change itself looks good; just a couple of little requests:

  • I think the s3_bucket_endpoint option might be better named something like bucket_as_host (unless AWS use the former nomenclature somewhere).
  • Could you add the new option, whatever it ends up being called, to the presigned_url_opts() type spec
  • Could you also update the docs for presigned_url() to reflect the new option (I just noticed the existing docs for virtual_host are slightly wrong, too, which is probably what caused the confusion over in Create Signed URL with custom domain #154).

Thanks!

@gr8Adakron
Copy link
Contributor Author

gr8Adakron commented Jan 23, 2022

@bernardd I choose this s3_bucket_endpoint argument as it was already been considered in other libraries.

  • I was thinking to add separate PR for documentation update - to keep things simple.

@bernardd
Copy link
Contributor

Also it would be great to have a test case to cover this use.

@gr8Adakron gr8Adakron changed the title Adding s3_bucket_endpoint flag to generate signed url Adding bucket_as_host flag to generate signed url Jan 23, 2022
@gr8Adakron
Copy link
Contributor Author

@bernardd

  • Renamed flag to bucket_as_host - as its easy to understand.
  • Adding REAME.md for bucket_as_host flag
  • Adding code level doc for bucket_as_host flag
  • Fixing code level doc for virtual_host flag
  • Adding test cases for bucket_as_host flag in coupled with virtual_host flag.

Please review.

@bernardd
Copy link
Contributor

Thanks @gr8Adakron - that all looks good. One tiny little request. The docs say:

When option param :bucket_as_host is true, the bucket name will be used as the hostname, which will look like <bucket>.<domain>.com host.

but actually bucket is the full hostname, there's no additional domain or .com parts. Could you reword it to something like

When option param :bucket_as_host is true, the bucket name will be used as the full hostname. In this case, bucket must be set to a full hostname, for example mybucket.example.com.

If you can make that tweak, I'll get this all merged. Thanks for all your work on it :)

@gr8Adakron
Copy link
Contributor Author

@bernardd Updated for the mentioned tweak.

@bernardd
Copy link
Contributor

Fantastic, thanks heaps for your work on this @gr8Adakron!

@bernardd bernardd merged commit 6b375da into ex-aws:main Jan 25, 2022
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