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

Separates url building and signing #5

Merged
merged 2 commits into from Apr 29, 2015

Conversation

Projects
None yet
4 participants
@leonelgalan

leonelgalan commented Apr 21, 2015

How

To be used in signing cookies. From within example.com or a subdomain of example.com (this is important, because the browser won't allow Rails to set cookies for a domain other than the one serving the page.)

# In a controller, before serving the images
before_action :set_cookie

def set_cookie
  signed_params = AWS::CF::Signer.sign(
                    nil,
                    resource: 'http://images.example.com/*',
                    expires: 1.day.from_now
                  )

  signed_params.each do |name, value|
    cookies["CloudFront-#{name}"] = { value: value,
                                      domain: :all,
                                      httponly: true }
  end
end

A custom policy is built allowing the user access to all images inside http://images.example.com/:

<img src="http://images.example.com/image1.jpg" alt="Image 1" />
<img src="http://images.example.com/image2.jpg" alt="Image 2" />
<img src="http://images.example.com/business/image1.jpg" alt="Image 1" />

Why

This is important when:

You want to provide access to multiple restricted files, for example, all of the files for a video in HLS format or all of the files in the subscribers' area of a website.

As described by Choosing Between Signed URLs and Signed Cookies


This PR breaks background compatibility of those using sign directly, but not sign_url, sign_url_safe, sign_path and sign_path_safe. We could simply rename the new method to keep background compatibility, but I thought this made the most sense, semantically:

  • sign gets all required parts of the signature as described by CloudFront documents. And can be used independently to support signed cookies.
  • build_url uses sign to build a signed url.
Separates url building and signing
To be used in signing cookies. See PR for details.
@tennantje

This comment has been minimized.

tennantje commented Apr 25, 2015

I've just tried this out and I think it's an awesome addition - I'd just written my own but when it wasn't working as expected - then I found this.

@58bits

This comment has been minimized.

Owner

58bits commented Apr 26, 2015

Thanks for the contribution @leonelgalan - I'll accept the pull request, and create a new release. Since there's the potential for breaking backwards compatibility, I may release this as 3.0.0

@subdigital

This comment has been minimized.

subdigital commented Apr 26, 2015

Looks like a good change, however why break backwards compatibility? Couldn't you just use a different method than sign?

@leonelgalan

This comment has been minimized.

leonelgalan commented Apr 27, 2015

@subdigital, as I expressed earlier, I did it because semantics. The current sign is doing too much, signing and building the url. I think the result is better code, even at the cost of breaking backwards compatibility. With @58bits bumping the major version number it shouldn't be a problem for anyone used to semver.

@tennantje

This comment has been minimized.

tennantje commented Apr 28, 2015

So long as this is being considered for a v3.0 release, is it also worth a separate PR to move this out of the AWS namespace (which is used by the official AWS v1 Gem)? I recognise that v2 is out and uses the Aws namespace instead.

@58bits

This comment has been minimized.

Owner

58bits commented Apr 28, 2015

Hi All. First, a confession of sorts. I haven't written a line of Ruby code in close to two years, and so this gem could probably do with a different maintainer. It's also making it difficult for me to 'reason' about the code here, and decide on the best approach.

Secondly, since this gem is a complete re-write from Dylan Vaughn's effort over five years ago (and I was never able to successfully contact Dylan), I think at this point, it's okay (with a hat tip to Dylan) for this project to be moved to a new repo so that issues can be raised (issues can't be raised on a fork). Can a repo that maintains 'head' be destroyed here on Github, and then re-pushed from a local repo, maintaining HEAD?

Since we're talking about a new major rev, new namespace and potentially a new public interface, I'd be happy for this project to go to a new home or new repo, with the docs updated to point to the new repository and new major version. I'd also be happy to continue to 'curate' the gem here, and release the new version if there's a consensus here on how to proceed.

Thoughts or suggestions welcome.

@leonelgalan

This comment has been minimized.

leonelgalan commented Apr 28, 2015

I wouldn't mind helping on the maintaining duties on either role, helping you out or flying solo.
I have the time (paid by my employer) and interest to keep this gem up-to-date. This gem is well documented, well written and has good tests.

As for the best approach, I think that we could rename the method on this PR and bump the MINOR version number and, later, rename methods and namespace for a MAJOR bump. Reconsidering, @subdigital was right, and we could easily add this feature to the current release, and still keep my "semantic" concerns for a MAJOR version change, immediately after.

Renames build_url back to sign
This allows it to be backwards compatible with the existing gem, as
discussed in #5

## Usage:

```ruby
# In a controller, before serving the images
before_action :set_cookie

def set_cookie
  signed_params = AWS::CF::Signer.signed_params(
                    nil,
                    resource: 'http://images.example.com/*',
                    expires: 1.day.from_now
                  )

  signed_params.each do |name, value|
    cookies["CloudFront-#{name}"] = { value: value,
                                      domain: :all,
                                      httponly: true }
  end
end
```
@subdigital

This comment has been minimized.

subdigital commented Apr 28, 2015

+1, thanks @leonelgalan.

In general I agree with your first PR, but keeping backward compatibility is worth it IMO.

@58bits

This comment has been minimized.

Owner

58bits commented Apr 28, 2015

Thanks @leonelgalan . So how about I close this PR, and wait for the update? Once you submit the new PR, I'll then publish the gem with the MINOR rev. As for the future, I'm fine with whatever works best. If giving this a new home under @leonelgalan works - then that's great (although a hat tip for the initial re-write would be greatly appreciated ;))

@leonelgalan

This comment has been minimized.

leonelgalan commented Apr 29, 2015

The updates are already on this PR, do you want me to squash the commits into one? What's the best way to communicate with you so I can publish future versions of the gem?

You and Dylan Vaugh, will always have my recognition! I'll push my local repo up, so it's a full featured repo that accepts issues and such.

58bits added a commit that referenced this pull request Apr 29, 2015

@58bits 58bits merged commit a80b220 into 58bits:master Apr 29, 2015

@58bits

This comment has been minimized.

Owner

58bits commented Apr 29, 2015

Got it and accepted. Will publish the new gem shortly. You can reach me on tony at 58bits dot com.

leonelgalan added a commit to leonelgalan/cloudfront-signer that referenced this pull request May 14, 2015

Renames module to Aws (from AWS)
- Matches the namespace used by the latest
_https://github.com/aws/aws-sdk-ruby_, this improvement was proposed by
@tennantje on 58bits/cloudfront-signer#5

leonelgalan added a commit to leonelgalan/cloudfront-signer that referenced this pull request May 18, 2015

Renames module to Aws (from AWS)
- Matches the namespace used by the latest
_https://github.com/aws/aws-sdk-ruby_, this improvement was proposed by
@tennantje on 58bits/cloudfront-signer#5

leonelgalan added a commit to leonelgalan/cloudfront-signer that referenced this pull request May 18, 2015

Renames module to Aws (from AWS)
- Matches the namespace used by the latest
_https://github.com/aws/aws-sdk-ruby_, this improvement was proposed by
@tennantje on 58bits/cloudfront-signer#5
- Adds missing `s` to `options`. Fixes unreported bug when using ip_range to build a custom policy.

leonelgalan added a commit to leonelgalan/cloudfront-signer that referenced this pull request May 18, 2015

Renames module to Aws (from AWS)
- Matches the namespace used by the latest
_https://github.com/aws/aws-sdk-ruby_, this improvement was proposed by
@tennantje on 58bits/cloudfront-signer#5
- Adds missing `s` to `options`. Fixes unreported bug when using ip_range to build a custom policy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment