Merge lp://staging/~rvb/gwacl/signed-access into lp://staging/gwacl

Nice branch, reasonably well tested, but I think we should improve some of that testing!

In particular, the new sign() method is not unit tested. I think you should add a test for it, and then we can use that function in other tests like TestComposeSharedSignature so that we don't have to hard-code the magic string "COJt8UagHJR2LBT1129bhDChtgfLGFqfZ0YQpBdF0=" which makes the test difficult to verify what it's actually testing.

Ok so on to the details:

16 +func composeSharedSignature(permission, signedExpiry, path, accountName, signedIdentifier, signedVersion, accountKey string) string {

Since you documented the other internal compose* funcs, can you do the same here please.

19 + stringToSign := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n%s",
20 + permission, "", signedExpiry, canonicalizedResource, signedIdentifier, signedVersion)

Gah I *really* miss Python's format() here. The second arg of "" is a bit magic, can you initalise a variable called 'signedStart' to "" and then use it instead of the bare "", it will make that Sprintf more readable (the "" sticks out like a pimple on a supermodel).

29 +func composeAccessQueryString(permission, signedExpiry, path, accountName, signedIdentifier, signedVersion, signedRessource, accountKey string) url.Values {

Given that this shares almost all of its params with composeSharedSignature it might be worth declaring a struct type which encapsulates the data in a shared access signature. I think once functions get beyond more than three params they get very unwieldy very quickly. The new type could well be useful in the future if we do more things with SASes.

Also this function is mis-named, it is not composing a string. May I suggest composeAcessQueryValues instead.

+func composeReadBlobValues(container, filename, accountName, accountKey string, expires time.Time) url.Values {
45 + expiryDateString := expires.UTC().Format(time.RFC3339)

Christ - why did they not make the date header format and this expiry stamp consistent? :/ (The other is RFC1123)

71 +func (*sharedSignatureSuite) TestComposeSharedSignature(c *C) {
72 + permission := "r"
73 + signedExpiry := "2015-02-12"
74 + path := "/path/to/file"
75 + accountName := "name"
76 + signedIdentifier := "identifier"
77 + signedVersion := "2012-02-12"
78 + accountKey := base64.StdEncoding.EncodeToString([]byte("key"))
79 +
80 + signature := composeSharedSignature(permission, signedExpiry, path, accountName, signedIdentifier, signedVersion, accountKey)
81 + c.Check(signature, Equals, "C/COJt8UagHJR2LBT1129bhDChtgfLGFqfZ0YQpBdF0=")

Here, once sign() is independently tested, you can use its return value instead of hard-coding this string here. The test will improve in readability for that.

103 +func (*sharedSignatureSuite) TestComposeReadBlobValues(c *C) {
104 + container := "container"
105 + filename := "/path/to/file"
106 + accountName := "name"
107 + accountKey := base64.StdEncoding.EncodeToString([]byte("key"))
108 + expires := time.Now()
109 + values := composeReadBlobValues(container, filename, accountName, accountKey, expires)
110 + c.Check(values.Get("sv"), Equals, "2012-02-12")
111 + ...

Early approval, I am sure you'll fix things :)

> Nice branch, reasonably well tested, but I think we should improve some of
> that testing!

Thanks for the review!

> In particular, the new sign() method is not unit tested. I think you should
> add a test for it, and then we can use that function in other tests like
> TestComposeSharedSignature so that we don't have to hard-code the magic string
> "COJt8UagHJR2LBT1129bhDChtgfLGFqfZ0YQpBdF0=" which makes the test difficult to
> verify what it's actually testing.

Like I said on IRC, this is not a new method, it's just extracted from somewhere but it was already there (and tested by a method which calls it)… but you're right, having a unit-test of the method itself is much better; done.

> 16 +func composeSharedSignature(permission, signedExpiry, path,
> accountName, signedIdentifier, signedVersion, accountKey string) string {
>
> Since you documented the other internal compose* funcs, can you do the same
> here please.

In this case, I felt the name of the method would be as good as any comment but okay, done.

> 19 + stringToSign := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n%s",
> 20 + permission, "", signedExpiry, canonicalizedResource,
> signedIdentifier, signedVersion)
>
> Gah I *really* miss Python's format() here. The second arg of "" is a bit
> magic, can you initalise a variable called 'signedStart' to "" and then use it
> instead of the bare "", it will make that Sprintf more readable (the "" sticks
> out like a pimple on a supermodel).

Very good point, done (I've added a 'signedStart' field to the structure).

> 29 +func composeAccessQueryString(permission, signedExpiry, path,
> accountName, signedIdentifier, signedVersion, signedRessource, accountKey
> string) url.Values {
>
> Given that this shares almost all of its params with composeSharedSignature it
> might be worth declaring a struct type which encapsulates the data in a shared
> access signature. I think once functions get beyond more than three params
> they get very unwieldy very quickly. The new type could well be useful in the
> future if we do more things with SASes.

Excellent point, done.

> Also this function is mis-named, it is not composing a string. May I suggest
> composeAcessQueryValues instead.

True, I started with a method which returned a string then changed the signature but not the name, thanks for spotting that!

> 71 +func (*sharedSignatureSuite) TestComposeSharedSignature(c *C) {
> 72 + permission := "r"
> 73 + signedExpiry := "2015-02-12"
> 74 + path := "/path/to/file"
> 75 + accountName := "name"
> 76 + signedIdentifier := "identifier"
> 77 + signedVersion := "2012-02-12"
> 78 + accountKey := base64.StdEncoding.EncodeToString([]byte("key"))
> 79 +
> 80 + signature := composeSharedSignature(permission, signedExpiry,
> path, accountName, signedIdentifier, signedVersion, accountKey)
> 81 + c.Check(signature, Equals,
> "C/COJt8UagHJR2LBT1129bhDChtgfLGFqfZ0YQpBdF0=")
>
> Here, once sign() is independently tested, you can use its return value
> instead of hard-coding this string here. The test will improve in readability
> for that.

That wou...

> Early approval, I am sure you'll fix things :)

I did, thanks for the review :)