Repository: google/go-cloud
Pull request #2861 (2020-09-12 12:44:02)

blob/all: adding BeforeSign support to SignedURL operation
`BeforeSign` is similar to `BeforeRead` and friends allowing users to hook into SignedURL operation. In my case it's needed to set `ContentDisposition` before signing s3blob get request.


codecov[bot]: # [Codecov](https://codecov.io/gh/google/go-cloud/pull/2861?src=pr&el=h1) Report
> Merging [#2861](https://codecov.io/gh/google/go-cloud/pull/2861?src=pr&el=desc) into [master](https://codecov.io/gh/google/go-cloud/commit/df6dfebf8f4775455934ab0660f50b32a7e7c08b?el=desc) will **decrease** coverage by `0.23%`.
> The diff coverage is `16.98%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-cloud/pull/2861/graphs/tree.svg?width=650&height=150&src=pr&token=y514B216UB)](https://codecov.io/gh/google/go-cloud/pull/2861?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #2861      +/-   ##
==========================================
- Coverage   68.54%   68.30%   -0.24%     
==========================================
  Files         116      116              
  Lines       13481    13516      +35     
==========================================
- Hits         9240     9232       -8     
- Misses       3582     3620      +38     
- Partials      659      664       +5     
```


| [Impacted Files](https://codecov.io/gh/google/go-cloud/pull/2861?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [blob/driver/driver.go](https://codecov.io/gh/google/go-cloud/pull/2861/diff?src=pr&el=tree#diff-YmxvYi9kcml2ZXIvZHJpdmVyLmdv) | `90.32% <ø> (ø)` | |
| [blob/fileblob/fileblob.go](https://codecov.io/gh/google/go-cloud/pull/2861/diff?src=pr&el=tree#diff-YmxvYi9maWxlYmxvYi9maWxlYmxvYi5nbw==) | `80.61% <0.00%> (-0.69%)` | :arrow_down: |
| [blob/gcsblob/gcsblob.go](https://codecov.io/gh/google/go-cloud/pull/2861/diff?src=pr&el=tree#diff-YmxvYi9nY3NibG9iL2djc2Jsb2IuZ28=) | `86.07% <0.00%> (-1.97%)` | :arrow_down: |
| [blob/azureblob/azureblob.go](https://codecov.io/gh/google/go-cloud/pull/2861/diff?src=pr&el=tree#diff-YmxvYi9henVyZWJsb2IvYXp1cmVibG9iLmdv) | `87.82% <18.18%> (-1.58%)` | :arrow_down: |
| [blob/s3blob/s3blob.go](https://codecov.io/gh/google/go-cloud/pull/2861/diff?src=pr&el=tree#diff-YmxvYi9zM2Jsb2IvczNibG9iLmdv) | `84.54% <20.00%> (-5.21%)` | :arrow_down: |
| [blob/blob.go](https://codecov.io/gh/google/go-cloud/pull/2861/diff?src=pr&el=tree#diff-YmxvYi9ibG9iLmdv) | `90.18% <100.00%> (+0.02%)` | :arrow_up: |
| [server/health/sqlhealth/sqlhealth.go](https://codecov.io/gh/google/go-cloud/pull/2861/diff?src=pr&el=tree#diff-c2VydmVyL2hlYWx0aC9zcWxoZWFsdGgvc3FsaGVhbHRoLmdv) | `61.90% <0.00%> (-19.05%)` | :arrow_down: |
| [docstore/driver/actionkind\_string.go](https://codecov.io/gh/google/go-cloud/pull/2861/diff?src=pr&el=tree#diff-ZG9jc3RvcmUvZHJpdmVyL2FjdGlvbmtpbmRfc3RyaW5nLmdv) | `0.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-cloud/pull/2861?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-cloud/pull/2861?src=pr&el=footer). Last update [df6dfeb...2421b74](https://codecov.io/gh/google/go-cloud/pull/2861?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/blob/azureblob/azureblob.go b/blob/azureblob/azureblob.go
index 56deba00..c840b361 100644
--- a/blob/azureblob/azureblob.go
+++ b/blob/azureblob/azureblob.go
@@ -62,6 +62,7 @@
 //  - Attributes: azblob.BlobGetPropertiesResponse
 //  - CopyOptions.BeforeCopy: azblob.Metadata, *azblob.ModifiedAccessConditions, *azblob.BlobAccessConditions
 //  - WriterOptions.BeforeWrite: *azblob.UploadStreamToBlockBlobOptions
+//  - SignedURLOptions.BeforeSign: *azblob.BlobSASSignatureValues
 package azureblob
 
 import (
@@ -749,15 +750,27 @@ func (b *bucket) SignedURL(ctx context.Context, key string, opts *driver.SignedU
 	default:
 		return "", fmt.Errorf("unsupported Method %s", opts.Method)
 	}
-	var err error
-	srcBlobParts.SAS, err = azblob.BlobSASSignatureValues{
+	signVals := &azblob.BlobSASSignatureValues{
 		Protocol:      azblob.SASProtocolHTTPS,
 		ExpiryTime:    time.Now().UTC().Add(opts.Expiry),
 		ContainerName: b.name,
 		BlobName:      srcBlobParts.BlobName,
 		Permissions:   perms.String(),
-	}.NewSASQueryParameters(b.opts.Credential)
-	if err != nil {
+	}
+	if opts.BeforeSign != nil {
+		asFunc := func(i interface{}) bool {
+			v, ok := i.(**azblob.BlobSASSignatureValues)
+			if ok {
+				*v = signVals
+			}
+			return ok
+		}
+		if err := opts.BeforeSign(asFunc); err != nil {
+			return "", err
+		}
+	}
+	var err error
+	if srcBlobParts.SAS, err = signVals.NewSASQueryParameters(b.opts.Credential); err != nil {
 		return "", err
 	}
 	srcBlobURLWithSAS := srcBlobParts.URL()
diff --git a/blob/blob.go b/blob/blob.go
index a20eefcf..d87fd24d 100644
--- a/blob/blob.go
+++ b/blob/blob.go
@@ -945,6 +945,7 @@ func (b *Bucket) SignedURL(ctx context.Context, key string, opts *SignedURLOptio
 	}
 	dopts.ContentType = opts.ContentType
 	dopts.EnforceAbsentContentType = opts.EnforceAbsentContentType
+	dopts.BeforeSign = opts.BeforeSign
 	b.mu.RLock()
 	defer b.mu.RUnlock()
 	if b.closed {
@@ -999,6 +1000,12 @@ type SignedURLOptions struct {
 	//
 	// Must be false for non-PUT requests.
 	EnforceAbsentContentType bool
+
+	// BeforeSign is a callback that will be called before each call to the
+	// the underlying service's sign functionality.
+	// asFunc converts its argument to driver-specific types.
+	// See https://gocloud.dev/concepts/as/ for background information.
+	BeforeSign func(asFunc func(interface{}) bool) error
 }
 
 // ReaderOptions sets options for NewReader and NewRangeReader.
diff --git a/blob/driver/driver.go b/blob/driver/driver.go
index e78969a4..0a296634 100644
--- a/blob/driver/driver.go
+++ b/blob/driver/driver.go
@@ -330,6 +330,12 @@ type SignedURLOptions struct {
 	//
 	// This field will always be false for non-PUT requests.
 	EnforceAbsentContentType bool
+
+	// BeforeSign is a callback that will be called before each call to the
+	// the underlying service's sign functionality.
+	// asFunc converts its argument to driver-specific types.
+	// See https://gocloud.dev/concepts/as/ for background information.
+	BeforeSign func(asFunc func(interface{}) bool) error
 }
 
 // prefixedBucket implements Bucket by prepending prefix to all keys.
diff --git a/blob/fileblob/fileblob.go b/blob/fileblob/fileblob.go
index 7ece2587..7979fa01 100644
--- a/blob/fileblob/fileblob.go
+++ b/blob/fileblob/fileblob.go
@@ -651,6 +651,11 @@ func (b *bucket) SignedURL(ctx context.Context, key string, opts *driver.SignedU
 	if b.opts.URLSigner == nil {
 		return "", errors.New("sign fileblob url: bucket does not have an Options.URLSigner")
 	}
+	if opts.BeforeSign != nil {
+		if err := opts.BeforeSign(func(interface{}) bool { return false }); err != nil {
+			return "", err
+		}
+	}
 	surl, err := b.opts.URLSigner.URLFromKey(ctx, key, opts)
 	if err != nil {
 		return "", err
diff --git a/blob/gcsblob/gcsblob.go b/blob/gcsblob/gcsblob.go
index ac975eb8..48d40ee1 100644
--- a/blob/gcsblob/gcsblob.go
+++ b/blob/gcsblob/gcsblob.go
@@ -52,6 +52,7 @@
 //  - Attributes: storage.ObjectAttrs
 //  - CopyOptions.BeforeCopy: *CopyObjectHandles, *storage.Copier (if accessing both, must be in that order)
 //  - WriterOptions.BeforeWrite: **storage.ObjectHandle, *storage.Writer (if accessing both, must be in that order)
+//  - SignedURLOptions.BeforeSign: *storage.SignedURLOptions
 package gcsblob // import "gocloud.dev/blob/gcsblob"
 
 import (
@@ -712,6 +713,18 @@ func (b *bucket) SignedURL(ctx context.Context, key string, dopts *driver.Signed
 	if b.opts.MakeSignBytes != nil {
 		opts.SignBytes = b.opts.MakeSignBytes(ctx)
 	}
+	if dopts.BeforeSign != nil {
+		asFunc := func(i interface{}) bool {
+			v, ok := i.(**storage.SignedURLOptions)
+			if ok {
+				*v = opts
+			}
+			return ok
+		}
+		if err := dopts.BeforeSign(asFunc); err != nil {
+			return "", err
+		}
+	}
 	return storage.SignedURL(b.name, key, opts)
 }
 
diff --git a/blob/s3blob/s3blob.go b/blob/s3blob/s3blob.go
index 1f2aab11..d8be65ac 100644
--- a/blob/s3blob/s3blob.go
+++ b/blob/s3blob/s3blob.go
@@ -51,6 +51,10 @@
 //  - Attributes: s3.HeadObjectOutput
 //  - CopyOptions.BeforeCopy: *s3.CopyObjectInput
 //  - WriterOptions.BeforeWrite: *s3manager.UploadInput, *s3manager.Uploader
+//  - SignedURLOptions.BeforeSign:
+//      *s3.GetObjectInput when Options.Method == http.MethodGet, or
+//      *s3.PutObjectInput when Options.Method == http.MethodPut, or
+//      *s3.DeleteObjectInput when Options.Method == http.MethodDelete
 package s3blob // import "gocloud.dev/blob/s3blob"
 
 import (
@@ -70,6 +74,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/aws/client"
+	"github.com/aws/aws-sdk-go/aws/request"
 	"github.com/aws/aws-sdk-go/service/s3"
 	"github.com/aws/aws-sdk-go/service/s3/s3manager"
 	"github.com/google/wire"
@@ -725,32 +730,67 @@ func (b *bucket) Delete(ctx context.Context, key string) error {
 	return err
 }
 
-func (b *bucket) SignedURL(ctx context.Context, key string, opts *driver.SignedURLOptions) (string, error) {
+func (b *bucket) SignedURL(_ context.Context, key string, opts *driver.SignedURLOptions) (string, error) {
 	key = escapeKey(key)
+	var req *request.Request
 	switch opts.Method {
 	case http.MethodGet:
 		in := &s3.GetObjectInput{
 			Bucket: aws.String(b.name),
 			Key:    aws.String(key),
 		}
-		req, _ := b.client.GetObjectRequest(in)
-		return req.Presign(opts.Expiry)
+		if opts.BeforeSign != nil {
+			asFunc := func(i interface{}) bool {
+				v, ok := i.(**s3.GetObjectInput)
+				if ok {
+					*v = in
+				}
+				return ok
+			}
+			if err := opts.BeforeSign(asFunc); err != nil {
+				return "", err
+			}
+		}
+		req, _ = b.client.GetObjectRequest(in)
 	case http.MethodPut:
 		in := &s3.PutObjectInput{
 			Bucket:      aws.String(b.name),
 			Key:         aws.String(key),
 			ContentType: aws.String(opts.ContentType),
 		}
-		req, _ := b.client.PutObjectRequest(in)
-		return req.Presign(opts.Expiry)
+		if opts.BeforeSign != nil {
+			asFunc := func(i interface{}) bool {
+				v, ok := i.(**s3.PutObjectInput)
+				if ok {
+					*v = in
+				}
+				return ok
+			}
+			if err := opts.BeforeSign(asFunc); err != nil {
+				return "", err
+			}
+		}
+		req, _ = b.client.PutObjectRequest(in)
 	case http.MethodDelete:
 		in := &s3.DeleteObjectInput{
 			Bucket: aws.String(b.name),
 			Key:    aws.String(key),
 		}
-		req, _ := b.client.DeleteObjectRequest(in)
-		return req.Presign(opts.Expiry)
+		if opts.BeforeSign != nil {
+			asFunc := func(i interface{}) bool {
+				v, ok := i.(**s3.DeleteObjectInput)
+				if ok {
+					*v = in
+				}
+				return ok
+			}
+			if err := opts.BeforeSign(asFunc); err != nil {
+				return "", err
+			}
+		}
+		req, _ = b.client.DeleteObjectRequest(in)
 	default:
 		return "", fmt.Errorf("unsupported Method %q", opts.Method)
 	}
+	return req.Presign(opts.Expiry)
 }


Test patch:
diff --git a/blob/azureblob/azureblob_test.go b/blob/azureblob/azureblob_test.go
index 10d344ac..d1b3b6f6 100644
--- a/blob/azureblob/azureblob_test.go
+++ b/blob/azureblob/azureblob_test.go
@@ -221,6 +221,14 @@ func (verifyContentLanguage) BeforeList(as func(interface{}) bool) error {
 	return nil
 }
 
+func (verifyContentLanguage) BeforeSign(as func(interface{}) bool) error {
+	var azOpts *azblob.BlobSASSignatureValues
+	if !as(&azOpts) {
+		return errors.New("BeforeSign.As failed")
+	}
+	return nil
+}
+
 func (verifyContentLanguage) AttributesCheck(attrs *blob.Attributes) error {
 	var resp azblob.BlobGetPropertiesResponse
 	if !attrs.As(&resp) {
diff --git a/blob/drivertest/drivertest.go b/blob/drivertest/drivertest.go
index 1317f799..697a101f 100644
--- a/blob/drivertest/drivertest.go
+++ b/blob/drivertest/drivertest.go
@@ -75,6 +75,8 @@ type HarnessMaker func(ctx context.Context, t *testing.T) (Harness, error)
 //    on the single blob entry returned.
 // 7. Tries to read a non-existent blob, and calls ErrorCheck with the error.
 // 8. Makes a copy of the blob, using BeforeCopy as a CopyOption.
+// 9. Calls SignedURL using BeforeSign as a SignedURLOption for each supported
+//    signing method (i.e. GET, PUT and DELETE).
 //
 // For example, an AsTest might set a driver-specific field to a custom
 // value in BeforeWrite, and then verify the custom value was returned in
@@ -98,6 +100,9 @@ type AsTest interface {
 	// BeforeList will be passed directly to ListOptions as part of listing the
 	// test blob.
 	BeforeList(as func(interface{}) bool) error
+	// BeforeSign will be passed directly to SignedURLOptions as part of
+	// generating a signed URL to the test blob.
+	BeforeSign(as func(interface{}) bool) error
 	// AttributesCheck will be called after fetching the test blob's attributes.
 	// It should call attrs.As and verify the results.
 	AttributesCheck(attrs *blob.Attributes) error
@@ -160,6 +165,13 @@ func (verifyAsFailsOnNil) BeforeList(as func(interface{}) bool) error {
 	return nil
 }
 
+func (verifyAsFailsOnNil) BeforeSign(as func(interface{}) bool) error {
+	if as(nil) {
+		return errors.New("want BeforeSign's As to return false when passed nil")
+	}
+	return nil
+}
+
 func (verifyAsFailsOnNil) AttributesCheck(attrs *blob.Attributes) error {
 	if attrs.As(nil) {
 		return errors.New("want Attributes.As to return false when passed nil")
@@ -2343,6 +2355,13 @@ func testAs(t *testing.T, newHarness HarnessMaker, st AsTest) {
 	} else {
 		defer func() { _ = b.Delete(ctx, copyKey) }()
 	}
+
+	for _, method := range []string{http.MethodGet, http.MethodPut, http.MethodDelete} {
+		_, err = b.SignedURL(ctx, key, &blob.SignedURLOptions{Method: method, BeforeSign: st.BeforeSign})
+		if err != nil && gcerrors.Code(err) != gcerrors.Unimplemented {
+			t.Errorf("got err %v when signing url with method %q", err, method)
+		}
+	}
 }
 
 func benchmarkRead(b *testing.B, bkt *blob.Bucket) {
diff --git a/blob/fileblob/fileblob_test.go b/blob/fileblob/fileblob_test.go
index 0536d7fe..6484b702 100644
--- a/blob/fileblob/fileblob_test.go
+++ b/blob/fileblob/fileblob_test.go
@@ -214,6 +214,7 @@ func (verifyPathError) BeforeRead(as func(interface{}) bool) error   { return ni
 func (verifyPathError) BeforeWrite(as func(interface{}) bool) error  { return nil }
 func (verifyPathError) BeforeCopy(as func(interface{}) bool) error   { return nil }
 func (verifyPathError) BeforeList(as func(interface{}) bool) error   { return nil }
+func (verifyPathError) BeforeSign(as func(interface{}) bool) error   { return nil }
 func (verifyPathError) AttributesCheck(attrs *blob.Attributes) error { return nil }
 func (verifyPathError) ReaderCheck(r *blob.Reader) error             { return nil }
 func (verifyPathError) ListObjectCheck(o *blob.ListObject) error     { return nil }
diff --git a/blob/gcsblob/gcsblob_test.go b/blob/gcsblob/gcsblob_test.go
index b4e22c7b..5baf67ff 100644
--- a/blob/gcsblob/gcsblob_test.go
+++ b/blob/gcsblob/gcsblob_test.go
@@ -196,6 +196,15 @@ func (verifyContentLanguage) BeforeList(as func(interface{}) bool) error {
 	return nil
 }
 
+func (verifyContentLanguage) BeforeSign(as func(interface{}) bool) error {
+	var opts *storage.SignedURLOptions
+	if !as(&opts) {
+		return errors.New("BeforeSign.As failed")
+	}
+	// Nothing to do.
+	return nil
+}
+
 func (verifyContentLanguage) AttributesCheck(attrs *blob.Attributes) error {
 	var oa storage.ObjectAttrs
 	if !attrs.As(&oa) {
diff --git a/blob/s3blob/s3blob_test.go b/blob/s3blob/s3blob_test.go
index 540ccfbc..b3380487 100644
--- a/blob/s3blob/s3blob_test.go
+++ b/blob/s3blob/s3blob_test.go
@@ -169,6 +169,18 @@ func (v verifyContentLanguage) BeforeList(as func(interface{}) bool) error {
 	return nil
 }
 
+func (v verifyContentLanguage) BeforeSign(as func(interface{}) bool) error {
+	var (
+		get *s3.GetObjectInput
+		put *s3.PutObjectInput
+		del *s3.DeleteObjectInput
+	)
+	if as(&get) || as(&put) || as(&del) {
+		return nil
+	}
+	return errors.New("BeforeSign.As failed")
+}
+
 func (verifyContentLanguage) AttributesCheck(attrs *blob.Attributes) error {
 	var hoo s3.HeadObjectOutput
 	if !attrs.As(&hoo) {