s3: implemented SignedURL

s3: implemented SignedURL

This also cleans up the querying logic a bit to make
the implementation of SignedURL more reasonble.

https://code.launchpad.net/~niemeyer/goamz/signed-urls/+merge/106928

(do not edit description out of merge proposal)

[niemeyer, 2012-05-23 02:42:50]

Please take a look.

[rog, 2012-05-23 09:24:49]

LGTM. thanks for doing this so quickly!

https://codereview.appspot.com/6231044/diff/1/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode99
s3/s3.go:99: req := &request{
much nicer.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode269
s3/s3.go:269: }
nice

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode285
s3/s3.go:285: // URL returns a non-signed URL that allows retriving path.
// URL returns a URL that allows retrieving
// the object at path.  It only works if the path is publicly
// readable (see SignedURL for an alternative).

?

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode290
s3/s3.go:290: path: path,
go fmt

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode304
s3/s3.go:304: // SignedURL returns a signed URL that allows retriving path until
expires.
// SignedURL returns a URL that allows anyone to retrieve
// the object at path, until it expires.

?

(alternative:
// SignedURL returns a URL, expiring at the given time, that
// allows anyone to retrieve the object at path.
)

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode322
s3/s3.go:322: //
----------------------------------------------------------------------------
i'd be happy if we deleted all of these

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode357
s3/s3.go:357: func (s3 *S3) prepare(req *request) error {
i wonder if this might not read marginally better as a method on *request.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode377
s3/s3.go:377: for _, c := range req.bucket {
if strings.IndexAny(req.bucket, "/:@") >= 0 {
    return fmt.Errorf("bad S3 bucket: %q", req.bucket)
}
?

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode433
s3/s3.go:433: httpresp, err := http.DefaultClient.Do(&httpreq)
FWIW, i was happy with "r" here. httpResp would be better than httpresp
regardless.

https://codereview.appspot.com/6231044/diff/1/s3/s3i_test.go
File s3/s3i_test.go (right):

https://codereview.appspot.com/6231044/diff/1/s3/s3i_test.go#newcode123
s3/s3i_test.go:123: if !s.authIsBroken{
go fmt

https://codereview.appspot.com/6231044/diff/1/s3/s3i_test.go#newcode124
s3/s3i_test.go:124: data, err = get(b.SignedURL("name2",
time.Now().Add(-time.Hour)))
perhaps a test here that the unadorned b.URL("name2") fails also?

https://codereview.appspot.com/6231044/diff/1/s3/s3t_test.go
File s3/s3t_test.go (right):

https://codereview.appspot.com/6231044/diff/1/s3/s3t_test.go#newcode41
s3/s3t_test.go:41: // TODO Sadly the fake server ignores auth completely right
now. :-(
it'll happen!

https://codereview.appspot.com/6231044/diff/1/s3/sign.go
File s3/sign.go (right):

https://codereview.appspot.com/6231044/diff/1/s3/sign.go#newcode40
s3/sign.go:40: func sign(auth aws.Auth, method, path string, params, headers
map[string][]string) {
i wonder if things might look simpler if you used url.Values instead of
map[string][]string

[rog, 2012-05-23 10:04:37]

https://codereview.appspot.com/6231044/diff/1/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode309
s3/s3.go:309: params: url.Values{"Expires": {strconv.FormatInt(expires.Unix(),
10)}},
i wonder if we should have a way of specifying a URL with no expiry date.

[niemeyer, 2012-05-23 13:54:16]

Please take a look.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode285
s3/s3.go:285: // URL returns a non-signed URL that allows retriving path.
On 2012/05/23 09:24:49, rog wrote:
> // URL returns a URL that allows retrieving
> // the object at path.  It only works if the path is publicly
> // readable (see SignedURL for an alternative).
> 
> ?

I've merged both. Hopefully it looks better.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode304
s3/s3.go:304: // SignedURL returns a signed URL that allows retriving path until
expires.
On 2012/05/23 09:24:49, rog wrote:
> // SignedURL returns a URL that allows anyone to retrieve
> // the object at path, until it expires.
> 
> ?
> 
> (alternative:
> // SignedURL returns a URL, expiring at the given time, that
> // allows anyone to retrieve the object at path.
> )

Also merged both.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode309
s3/s3.go:309: params: url.Values{"Expires": {strconv.FormatInt(expires.Unix(),
10)}},
On 2012/05/23 10:04:37, rog wrote:
> i wonder if we should have a way of specifying a URL with no expiry date.

Maybe. Not sure about what's the int size the server would be using, though. We
can have the zero time expiring at the limit of the 32bit epoch, perhaps. Let's
please keep that for a future change, though.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode322
s3/s3.go:322: //
----------------------------------------------------------------------------
On 2012/05/23 09:24:49, rog wrote:
> i'd be happy if we deleted all of these

Done.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode357
s3/s3.go:357: func (s3 *S3) prepare(req *request) error {
On 2012/05/23 09:24:49, rog wrote:
> i wonder if this might not read marginally better as a method on *request.

I've actually tried that. Doesn't improve the code.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode377
s3/s3.go:377: for _, c := range req.bucket {
On 2012/05/23 09:24:49, rog wrote:
> if strings.IndexAny(req.bucket, "/:@") >= 0 {
>     return fmt.Errorf("bad S3 bucket: %q", req.bucket)
> }
> ?

Nice one. Done.

https://codereview.appspot.com/6231044/diff/1/s3/s3.go#newcode433
s3/s3.go:433: httpresp, err := http.DefaultClient.Do(&httpreq)
On 2012/05/23 09:24:49, rog wrote:
> FWIW, i was happy with "r" here. httpResp would be better than httpresp
> regardless.

Went to the middle ground and used hreq and hresp.

https://codereview.appspot.com/6231044/diff/1/s3/s3i_test.go
File s3/s3i_test.go (right):

https://codereview.appspot.com/6231044/diff/1/s3/s3i_test.go#newcode124
s3/s3i_test.go:124: data, err = get(b.SignedURL("name2",
time.Now().Add(-time.Hour)))
On 2012/05/23 09:24:49, rog wrote:
> perhaps a test here that the unadorned b.URL("name2") fails also?

Corrupting the returned URL is testing the behavior of something that is not
what the method produces. We're already verifying that we can access a *private*
value with a good expiration date, and can't access it with one that has timed
out, and both of these verify the real URL.

https://codereview.appspot.com/6231044/diff/1/s3/sign.go
File s3/sign.go (right):

https://codereview.appspot.com/6231044/diff/1/s3/sign.go#newcode40
s3/sign.go:40: func sign(auth aws.Auth, method, path string, params, headers
map[string][]string) {
On 2012/05/23 09:24:49, rog wrote:
> i wonder if things might look simpler if you used url.Values instead of
> map[string][]string

headers is not a url.Values, so probably not.

[niemeyer, 2012-05-23 13:59:04]

*** Submitted:

s3: implemented SignedURL

This also cleans up the querying logic a bit to make
the implementation of SignedURL more reasonble.

R=rog
CC=
https://codereview.appspot.com/6231044