Repository: moov-io/customers
Pull request #207 (2020-09-29 18:47:26)

documents: fixup document storage and retrieval
Fixes: https://github.com/moov-io/customers/issues/129
Fixes: https://github.com/moov-io/customers/issues/198
Issue: https://github.com/moovfinancial/production/issues/42

codecov-commenter: # [Codecov](https://codecov.io/gh/moov-io/customers/pull/207?src=pr&el=h1) Report
> Merging [#207](https://codecov.io/gh/moov-io/customers/pull/207?src=pr&el=desc) into [master](https://codecov.io/gh/moov-io/customers/commit/dab1b83cf3ff07de8b828d5c44bbab5a023cd782?el=desc) will **decrease** coverage by `3.06%`.
> The diff coverage is `51.38%`.


```diff
@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   61.67%   58.61%   -3.07%     
==========================================
  Files          49       49              
  Lines        2586     2619      +33     
==========================================
- Hits         1595     1535      -60     
- Misses        685      785     +100     
+ Partials      306      299       -7     
```


Patch:
diff --git a/cmd/server/main.go b/cmd/server/main.go
index 9b328e1..9306152 100644
--- a/cmd/server/main.go
+++ b/cmd/server/main.go
@@ -119,7 +119,7 @@ func main() {
 	if cloudProvider == "" {
 		cloudProvider = "file"
 	}
-	signer := setupStorageBucket(logger, bucketName, cloudProvider)
+	signer := setupSigner(logger, bucketName, cloudProvider)
 
 	// Create our Watchman client
 	debugWatchmanCalls := util.Or(os.Getenv("WATCHMAN_DEBUG_CALLS"), "false")
@@ -165,6 +165,8 @@ func main() {
 		Repo: accountsRepo, WatchmanClient: watchmanClient,
 	}
 
+	bucket := storage.GetBucket(logger, os.Getenv("DOCUMENTS_BUCKET"), util.Or(os.Getenv("DOCUMENTS_PROVIDER"), "file"), signer)
+
 	// Setup business HTTP routes
 	router := mux.NewRouter()
 	moovhttp.AddCORSHandler(router)
@@ -173,7 +175,7 @@ func main() {
 	customers.AddCustomerRoutes(logger, router, customerRepo, customerSSNStorage, ofac)
 	customers.AddCustomerAddressRoutes(logger, router, customerRepo)
 	documents.AddDisclaimerRoutes(logger, router, disclaimerRepo)
-	documents.AddDocumentRoutes(logger, router, documentRepo, storage.GetBucket(bucketName, cloudProvider, signer))
+	documents.AddDocumentRoutes(logger, router, documentRepo, bucket)
 	customers.AddOFACRoutes(logger, router, customerRepo, ofac)
 
 	// Add Configuration routes
@@ -183,7 +185,7 @@ func main() {
 	// Optionally serve /files/ as our fileblob routes
 	// Note: FILEBLOB_BASE_URL needs to match something that's routed to /files/...
 	if cloudProvider == "file" {
-		storage.AddFileblobRoutes(logger, router, signer, storage.GetBucket(bucketName, cloudProvider, signer))
+		storage.AddFileblobRoutes(logger, router, signer, bucket)
 	}
 
 	// Start business HTTP server
@@ -240,7 +242,7 @@ func addPingRoute(r *mux.Router) {
 	})
 }
 
-func setupStorageBucket(logger log.Logger, bucketName, cloudProvider string) *fileblob.URLSignerHMAC {
+func setupSigner(logger log.Logger, bucketName, cloudProvider string) *fileblob.URLSignerHMAC {
 	if cloudProvider == "file" || cloudProvider == "" {
 		baseURL, secret := os.Getenv("FILEBLOB_BASE_URL"), os.Getenv("FILEBLOB_HMAC_SECRET")
 		if baseURL == "" {
diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md
index 47682b3..8c70f4c 100644
--- a/docs/CONFIGURATION.md
+++ b/docs/CONFIGURATION.md
@@ -96,8 +96,8 @@ Refer to the sqlite driver documentation for [connection parameters](https://git
 
 The following environment variables control which backend service is initialized for Document persistence. These all follow a similar ["blob storage"](https://gocloud.dev/ref/blob/) API provided by a library that Google [build and maintains](https://github.com/google/go-cloud).
 
-- `BUCKET_NAME`: The name of the bucket to use. Must be created outside of Customers if using a cloud provider. Make sure proper access and encryption controls are setup on this bucket to prevent exposure or unauthorized access. Example: `./storage/` (For `file` type backends)
-- `CLOUD_PROVIDER`: Provider name which determines which of the following environmental variables are used to initialize Customer's persistence.
+- `DOCUMENTS_BUCKET`: The name of the bucket to use. Must be created outside of Customers if using a cloud provider. Make sure proper access and encryption controls are setup on this bucket to prevent exposure or unauthorized access. Example: `./storage/` (For `file` type backends or `moov-customers-storage` for GCP/GCS)
+- `DOCUMENTS_PROVIDER`: Provider name which determines which of the following environmental variables are used to initialize Customer's persistence.
 
 ##### AWS S3 Storage
 
diff --git a/pkg/documents/documents.go b/pkg/documents/documents.go
index 73c0687..364b834 100644
--- a/pkg/documents/documents.go
+++ b/pkg/documents/documents.go
@@ -13,6 +13,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"path"
 	"strings"
 	"time"
 
@@ -30,19 +31,21 @@ import (
 
 var (
 	errNoDocumentID = errors.New("no Document ID found")
+)
 
+const (
 	maxDocumentSize int64 = 20 * 1024 * 1024 // 20MB
 )
 
 func AddDocumentRoutes(logger log.Logger, r *mux.Router, repo DocumentRepository, bucketFactory storage.BucketFunc) {
 	r.Methods("GET").Path("/customers/{customerID}/documents").HandlerFunc(getCustomerDocuments(logger, repo))
 	r.Methods("POST").Path("/customers/{customerID}/documents").HandlerFunc(uploadCustomerDocument(logger, repo, bucketFactory))
-	r.Methods("GET").Path("/customers/{customerID}/documents/{documentId}").HandlerFunc(retrieveRawDocument(logger, repo, bucketFactory))
-	r.Methods("DELETE").Path("/customers/{customerID}/documents/{documentId}").HandlerFunc(deleteCustomerDocument(logger, repo))
+	r.Methods("GET").Path("/customers/{customerID}/documents/{documentID}").HandlerFunc(retrieveRawDocument(logger, repo, bucketFactory))
+	r.Methods("DELETE").Path("/customers/{customerID}/documents/{documentID}").HandlerFunc(deleteCustomerDocument(logger, repo))
 }
 
 func getDocumentID(w http.ResponseWriter, r *http.Request) string {
-	v, ok := mux.Vars(r)["documentId"]
+	v, ok := mux.Vars(r)["documentID"]
 	if !ok || v == "" {
 		moovhttp.Problem(w, errNoDocumentID)
 		return ""
@@ -61,6 +64,7 @@ func getCustomerDocuments(logger log.Logger, repo DocumentRepository) http.Handl
 
 		docs, err := repo.getCustomerDocuments(customerID, organization)
 		if err != nil {
+			logger.Log("documents", fmt.Sprintf("failed to %v", err), "customerID", customerID)
 			moovhttp.Problem(w, err)
 			return
 		}
@@ -86,8 +90,11 @@ func readDocumentType(v string) (string, error) {
 func uploadCustomerDocument(logger log.Logger, repo DocumentRepository, bucketFactory storage.BucketFunc) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		w = route.Responder(logger, w, r)
-		// TODO(adam): should we store x-organization along with the Document?
 
+		customerID := route.GetCustomerID(w, r)
+		if customerID == "" {
+			return
+		}
 		documentType, err := readDocumentType(r.URL.Query().Get("type"))
 		if err != nil {
 			moovhttp.Problem(w, err)
@@ -104,6 +111,7 @@ func uploadCustomerDocument(logger log.Logger, repo DocumentRepository, bucketFa
 		// Detect the content type by reading the first 512 bytes of r.Body (read into file as we expect a multipart request)
 		buf := make([]byte, 512)
 		if _, err := file.Read(buf); err != nil && err != io.EOF {
+			logger.Log("documents", fmt.Sprintf("failed to peek: %v", err), "customerID", customerID)
 			moovhttp.Problem(w, err)
 			return
 		}
@@ -118,6 +126,7 @@ func uploadCustomerDocument(logger log.Logger, repo DocumentRepository, bucketFa
 		// Grab our cloud bucket before writing into our database
 		bucket, err := bucketFactory()
 		if err != nil {
+			logger.Log("documents", fmt.Sprintf("failed to create bucket: %v", err), "customerID", customerID)
 			moovhttp.Problem(w, err)
 			return
 		}
@@ -128,6 +137,7 @@ func uploadCustomerDocument(logger log.Logger, repo DocumentRepository, bucketFa
 			return
 		}
 		if err := repo.writeCustomerDocument(customerID, doc); err != nil {
+			logger.Log("documents", fmt.Sprintf("failed to %v", err), "customerID", customerID)
 			moovhttp.Problem(w, err)
 			return
 		}
@@ -149,10 +159,15 @@ func uploadCustomerDocument(logger log.Logger, repo DocumentRepository, bucketFa
 			moovhttp.Problem(w, err)
 			return
 		}
-		defer writer.Close()
 
 		// Concat our mime buffer back with the multipart file
 		n, err := io.Copy(writer, io.LimitReader(io.MultiReader(bytes.NewReader(buf), file), maxDocumentSize))
+
+		if err := writer.Close(); err != nil {
+			moovhttp.Problem(w, fmt.Errorf("documents: closing writer: %v", err))
+			return
+		}
+
 		if err != nil || n == 0 {
 			moovhttp.Problem(w, fmt.Errorf("documents: wrote %d bytes: %v", n, err))
 			return
@@ -168,11 +183,23 @@ func retrieveRawDocument(logger log.Logger, repo DocumentRepository, bucketFacto
 	return func(w http.ResponseWriter, r *http.Request) {
 		w = route.Responder(logger, w, r)
 
-		customerID, documentId := route.GetCustomerID(w, r), getDocumentID(w, r)
-		if customerID == "" || documentId == "" {
+		customerID, documentID := route.GetCustomerID(w, r), getDocumentID(w, r)
+		if customerID == "" || documentID == "" {
+			return
+		}
+		organization := route.GetOrganization(w, r)
+		if organization == "" {
+			return
+		}
+
+		// reject the request if the document is deleted
+		if exists, err := repo.exists(customerID, documentID, organization); !exists || err != nil {
+			if err != nil {
+				logger.Log("documents", fmt.Sprintf("failed to %v", err), "customerID", customerID, "documentID", documentID)
+			}
+			w.WriteHeader(http.StatusNotFound)
 			return
 		}
-		requestID := moovhttp.GetRequestID(r)
 
 		bucket, err := bucketFactory()
 		if err != nil {
@@ -184,21 +211,20 @@ func retrieveRawDocument(logger log.Logger, repo DocumentRepository, bucketFacto
 		ctx, cancelFn := context.WithTimeout(context.TODO(), 10*time.Second)
 		defer cancelFn()
 
-		documentKey := makeDocumentKey(customerID, documentId)
-		signedURL, err := bucket.SignedURL(ctx, documentKey, &blob.SignedURLOptions{
-			Expiry: 15 * time.Minute,
-		})
+		documentKey := makeDocumentKey(customerID, documentID)
+		rdr, err := bucket.NewReader(ctx, documentKey, nil)
 		if err != nil {
-			moovhttp.Problem(w, err)
+			moovhttp.Problem(w, fmt.Errorf("read documentID=%s: %v", documentKey, err))
 			return
 		}
-		if signedURL == "" {
-			moovhttp.Problem(w, fmt.Errorf("document=%s not found", documentId))
+		defer rdr.Close()
+
+		w.Header().Set("Content-Type", rdr.ContentType())
+		w.WriteHeader(http.StatusOK)
+		if n, err := io.Copy(w, rdr); n == 0 || err != nil {
+			moovhttp.Problem(w, fmt.Errorf("failed writing documentID=%s (bytes read: %d): %v", documentKey, n, err))
 			return
 		}
-
-		logger.Log("documents", fmt.Sprintf("redirecting for document=%s", documentKey), "requestID", requestID)
-		http.Redirect(w, r, signedURL, http.StatusFound)
 	}
 }
 
@@ -214,8 +240,8 @@ func deleteCustomerDocument(logger log.Logger, repo DocumentRepository) http.Han
 
 		err := repo.deleteCustomerDocument(customerID, documentID)
 		if err != nil {
-			moovhttp.Problem(w, fmt.Errorf("deleting document: %v", err))
-			logger.Log("documents", fmt.Sprintf("error deleting document=%s for customer=%s: %v", documentID, customerID, err), "requestID", requestID)
+			moovhttp.Problem(w, fmt.Errorf("failed to %v", err))
+			logger.Log("documents", fmt.Sprintf("deleting document=%s for customer=%s: %v", documentID, customerID, err), "requestID", requestID)
 			return
 		}
 
@@ -225,12 +251,14 @@ func deleteCustomerDocument(logger log.Logger, repo DocumentRepository) http.Han
 	}
 }
 
-func makeDocumentKey(customerID, documentId string) string {
-	return fmt.Sprintf("customer-%s-document-%s", customerID, documentId)
+func makeDocumentKey(customerID, documentID string) string {
+	return path.Join("customers", customerID, "documents", documentID)
 }
 
 type DocumentRepository interface {
+	exists(customerID string, documentID string, organization string) (bool, error)
 	getCustomerDocuments(customerID string, organization string) ([]*client.Document, error)
+
 	writeCustomerDocument(customerID string, doc *client.Document) error
 	deleteCustomerDocument(customerID string, documentID string) error
 }
@@ -246,26 +274,44 @@ func NewDocumentRepo(logger log.Logger, db *sql.DB) DocumentRepository {
 		logger: logger,
 	}
 }
+
+func (r *sqlDocumentRepository) exists(customerID string, documentID string, organization string) (bool, error) {
+	query := `select documents.document_id from documents
+inner join customers on customers.organization = ? where documents.customer_id = ? and documents.document_id = ? and documents.deleted_at is null
+limit 1;`
+	stmt, err := r.db.Prepare(query)
+	if err != nil {
+		return false, fmt.Errorf("prepare exists: %v", err)
+	}
+	defer stmt.Close()
+
+	var docID string
+	if err := stmt.QueryRow(organization, customerID, documentID).Scan(&docID); err != nil {
+		return false, err
+	}
+	return documentID == docID, nil
+}
+
 func (r *sqlDocumentRepository) getCustomerDocuments(customerID string, organization string) ([]*client.Document, error) {
 	query := `select document_id, documents.type, content_type, uploaded_at from documents join customers on customers.
 	organization = ? where documents.customer_id = ? and documents.deleted_at is null`
 	stmt, err := r.db.Prepare(query)
 	if err != nil {
-		return nil, fmt.Errorf("getCustomerDocuments: prepare %v", err)
+		return nil, fmt.Errorf("prepare listing documents: %v", err)
 	}
 	defer stmt.Close()
 
 	rows, err := stmt.Query(organization, customerID)
 	if err != nil {
-		return nil, fmt.Errorf("getCustomerDocuments: query %v", err)
+		return nil, fmt.Errorf("failed querying customer documents: %v", err)
 	}
 	defer rows.Close()
 
-	var docs []*client.Document
+	docs := make([]*client.Document, 0)
 	for rows.Next() {
 		var doc client.Document
 		if err := rows.Scan(&doc.DocumentID, &doc.Type, &doc.ContentType, &doc.UploadedAt); err != nil {
-			return nil, fmt.Errorf("getCustomerDocuments: scan: %v", err)
+			return nil, fmt.Errorf("scan customer documents: %v", err)
 		}
 		docs = append(docs, &doc)
 	}
@@ -276,12 +322,12 @@ func (r *sqlDocumentRepository) writeCustomerDocument(customerID string, doc *cl
 	query := `insert into documents (document_id, customer_id, type, content_type, uploaded_at) values (?, ?, ?, ?, ?);`
 	stmt, err := r.db.Prepare(query)
 	if err != nil {
-		return fmt.Errorf("writeCustomerDocument: prepare: %v", err)
+		return fmt.Errorf("prepare write: %v", err)
 	}
 	defer stmt.Close()
 
 	if _, err := stmt.Exec(doc.DocumentID, customerID, doc.Type, doc.ContentType, doc.UploadedAt); err != nil {
-		return fmt.Errorf("writeCustomerDocument: exec: %v", err)
+		return fmt.Errorf("write customer document: %v", err)
 	}
 	return nil
 }
@@ -290,7 +336,7 @@ func (r *sqlDocumentRepository) deleteCustomerDocument(customerID string, docume
 	query := `update documents set deleted_at = ? where customer_id = ? and document_id = ? and deleted_at is null;`
 	stmt, err := r.db.Prepare(query)
 	if err != nil {
-		return err
+		return fmt.Errorf("prepare delete: %v", err)
 	}
 	defer stmt.Close()
 
diff --git a/pkg/documents/storage/bucket.go b/pkg/documents/storage/bucket.go
index 90d6006..23ad062 100644
--- a/pkg/documents/storage/bucket.go
+++ b/pkg/documents/storage/bucket.go
@@ -6,6 +6,7 @@ package storage
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/url"
 	"os"
@@ -15,6 +16,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/credentials"
 	"github.com/aws/aws-sdk-go/aws/session"
+	"github.com/go-kit/kit/log"
 	"gocloud.dev/blob"
 	"gocloud.dev/blob/fileblob"
 	"gocloud.dev/blob/gcsblob"
@@ -24,40 +26,51 @@ import (
 
 type BucketFunc func() (*blob.Bucket, error)
 
-func GetBucket(bucketName, cloudProvider string, FileblobSigner *fileblob.URLSignerHMAC) BucketFunc {
+func GetBucket(logger log.Logger, bucketName, cloudProvider string, FileblobSigner *fileblob.URLSignerHMAC) BucketFunc {
 	return func() (*blob.Bucket, error) {
 		ctx, cancelFn := context.WithTimeout(context.TODO(), 10*time.Second)
 		defer cancelFn()
 
-		if bucketName == "" || cloudProvider == "" {
-			return nil, fmt.Errorf("storage: missing BUCKET_NAME=%s and/or CLOUD_PROVIDER=%s", bucketName, cloudProvider)
+		if bucketName == "" {
+			return nil, errors.New("missing document bucket")
 		}
-		return openBucket(ctx, bucketName, cloudProvider, FileblobSigner)
+		if cloudProvider == "" {
+			return nil, errors.New("missing documents cloud provider")
+		}
+
+		return openBucket(ctx, logger, bucketName, cloudProvider, FileblobSigner)
 	}
 }
 
 // openBucket returns a Go Cloud Development Kit (Go CDK) Bucket object which can be used to read and write arbitrary
 // blobs from a cloud provider blob store. Checkout https://gocloud.dev/ref/blob/ for more details
-func openBucket(ctx context.Context, bucketName, cloudProvider string, FileblobSigner *fileblob.URLSignerHMAC) (*blob.Bucket, error) {
+func openBucket(ctx context.Context, logger log.Logger, bucketName, cloudProvider string, FileblobSigner *fileblob.URLSignerHMAC) (*blob.Bucket, error) {
 	switch strings.ToLower(cloudProvider) {
 	case "aws":
-		return awsBucket(ctx, bucketName)
+		return awsBucket(ctx, logger, bucketName)
 	case "file":
-		return fileBucket(ctx, bucketName, FileblobSigner)
+		return fileBucket(ctx, logger, bucketName, FileblobSigner)
 	case "gcp":
-		return gcpBucket(ctx, bucketName)
+		return gcpBucket(ctx, logger, bucketName)
 	default:
 		return nil, fmt.Errorf("invalid cloud provider: %s", cloudProvider)
 	}
 }
 
-func awsBucket(ctx context.Context, bucketName string) (*blob.Bucket, error) {
+func awsBucket(ctx context.Context, logger log.Logger, bucketName string) (*blob.Bucket, error) {
 	c := &aws.Config{
 		Region:      aws.String(os.Getenv("AWS_REGION")),
 		Credentials: credentials.NewEnvCredentials(), // reads AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY
 	}
 	s := session.Must(session.NewSession(c))
-	return s3blob.OpenBucket(ctx, s, bucketName, nil)
+
+	bucket, err := s3blob.OpenBucket(ctx, s, bucketName, nil)
+	if err != nil {
+		logger.Log("storage", fmt.Sprintf("ERROR creating %s aws bucket: %v", bucketName, err))
+		return nil, err
+	}
+	logger.Log("storage", fmt.Sprintf("created %s aws bucket: %T", bucketName, bucket))
+	return bucket, nil
 }
 
 func FileblobSigner(baseURL, secret string) (*fileblob.URLSignerHMAC, error) {
@@ -68,16 +81,17 @@ func FileblobSigner(baseURL, secret string) (*fileblob.URLSignerHMAC, error) {
 	}
 }
 
-func fileBucket(ctx context.Context, bucketName string, signer *fileblob.URLSignerHMAC) (*blob.Bucket, error) {
+func fileBucket(ctx context.Context, logger log.Logger, bucketName string, signer *fileblob.URLSignerHMAC) (*blob.Bucket, error) {
 	if err := os.Mkdir(bucketName, 0777); strings.Contains(bucketName, "..") || (err != nil && !os.IsExist(err)) {
 		return nil, fmt.Errorf("problem creating %s error=%v", bucketName, err)
 	}
+	logger.Log("storage", fmt.Sprintf("created %s for file bucket", bucketName))
 	return fileblob.OpenBucket(bucketName, &fileblob.Options{
 		URLSigner: signer,
 	})
 }
 
-func gcpBucket(ctx context.Context, bucketName string) (*blob.Bucket, error) {
+func gcpBucket(ctx context.Context, logger log.Logger, bucketName string) (*blob.Bucket, error) {
 	// DefaultCredentials assumes a user has logged in with gcloud
 	// https://cloud.google.com/docs/authentication/getting-started
 	creds, err := gcp.DefaultCredentials(ctx)
@@ -88,5 +102,12 @@ func gcpBucket(ctx context.Context, bucketName string) (*blob.Bucket, error) {
 	if err != nil {
 		return nil, err
 	}
-	return gcsblob.OpenBucket(ctx, c, bucketName, nil)
+
+	bucket, err := gcsblob.OpenBucket(ctx, c, bucketName, nil)
+	if err != nil {
+		logger.Log("storage", fmt.Sprintf("ERROR creating %s gcp bucket: %v", bucketName, err))
+		return nil, err
+	}
+	logger.Log("storage", fmt.Sprintf("created %s gcp bucket: %v", bucketName, err))
+	return bucket, nil
 }


Test patch:
diff --git a/cmd/server/main_test.go b/cmd/server/main_test.go
index c2214aa..9e32153 100644
--- a/cmd/server/main_test.go
+++ b/cmd/server/main_test.go
@@ -14,21 +14,21 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestSetupStorageBucket(t *testing.T) {
+func TestMain__setupSigner(t *testing.T) {
 	logger := log.NewNopLogger()
 
-	signer := setupStorageBucket(logger, "", "file")
+	signer := setupSigner(logger, "", "file")
 	if signer == nil {
 		t.Fatal("expected non-nil Signer")
 	}
 
-	signer = setupStorageBucket(logger, "", "other")
+	signer = setupSigner(logger, "", "other")
 	if signer != nil {
 		t.Fatal("expected nil Signer")
 	}
 }
 
-func TestSetupValidationStrategies(t *testing.T) {
+func TestMain__setupValidationStrategies(t *testing.T) {
 	logger := log.NewNopLogger()
 	adminServer := admin.NewServer(*adminAddr)
 
diff --git a/pkg/documents/documents_test.go b/pkg/documents/documents_test.go
index d843f51..7920ede 100644
--- a/pkg/documents/documents_test.go
+++ b/pkg/documents/documents_test.go
@@ -11,7 +11,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
@@ -43,6 +42,10 @@ type testDocumentRepository struct {
 	written *client.Document
 }
 
+func (r *testDocumentRepository) exists(customerID string, documentID string, organization string) (bool, error) {
+	return false, r.err
+}
+
 func (r *testDocumentRepository) getCustomerDocuments(customerID string, organization string) ([]*client.Document, error) {
 	if r.err != nil {
 		return nil, r.err
@@ -101,8 +104,7 @@ func TestDocuments__getCustomerDocuments(t *testing.T) {
 	w.Flush()
 
 	if w.Code != http.StatusOK {
-		b, _ := ioutil.ReadAll(w.Body)
-		t.Errorf("bogus status code: %d - %s", w.Code, string(b))
+		t.Errorf("bogus status code: %d\n%s", w.Code, w.Body.String())
 	}
 }
 
@@ -190,12 +192,27 @@ func TestDocumentsUploadAndRetrieval(t *testing.T) {
 	router.ServeHTTP(w, req)
 	w.Flush()
 
-	if w.Code != http.StatusFound {
+	if w.Code != http.StatusBadRequest {
 		t.Errorf("bogus HTTP status: %d", w.Code)
 	}
-	if loc := w.Header().Get("Location"); !strings.Contains(loc, makeDocumentKey("foo", doc.DocumentID)) {
-		t.Errorf("unexpected SignedURL: %s", loc)
+}
+
+func TestDocuments__retrieveError(t *testing.T) {
+	repo := &testDocumentRepository{
+		err: errors.New("bad error"),
 	}
+
+	router := mux.NewRouter()
+	AddDocumentRoutes(log.NewNopLogger(), router, repo, storage.TestBucket)
+
+	customerID, documentID := base.ID(), base.ID()
+
+	w := httptest.NewRecorder()
+	req := httptest.NewRequest("GET", fmt.Sprintf("/customers/%s/documents/%s", customerID, documentID), nil)
+	req.Header.Set("X-Organization", base.ID())
+	router.ServeHTTP(w, req)
+
+	require.Equal(t, http.StatusNotFound, w.Code)
 }
 
 func TestDocuments__delete(t *testing.T) {
@@ -226,7 +243,6 @@ func TestDocuments__delete(t *testing.T) {
 	row := db.DB.QueryRow("SELECT COUNT(*) FROM documents where deleted_at is null")
 	require.NoError(t, row.Scan(&count))
 	require.Equal(t, 0, count)
-
 }
 
 func TestDocuments__uploadCustomerDocument(t *testing.T) {
@@ -253,8 +269,10 @@ func TestDocuments__uploadCustomerDocument(t *testing.T) {
 }
 
 func TestDocuments__makeDocumentKey(t *testing.T) {
-	if v := makeDocumentKey("a", "b"); v != "customer-a-document-b" {
-		t.Errorf("got %q", v)
+	key := makeDocumentKey("a", "b")
+
+	if key != "customers/a/documents/b" {
+		t.Errorf("got %q", key)
 	}
 }
 
@@ -316,6 +334,11 @@ func TestDocumentRepository(t *testing.T) {
 
 			require.Equal(t, doc.DocumentID, docs[0].DocumentID)
 			require.Equal(t, "image/png", docs[0].ContentType)
+
+			// make sure we read the document
+			exists, err := documentRepo.exists(customerID, doc.DocumentID, organization)
+			require.Equal(t, true, exists)
+			require.NoError(t, err)
 		})
 	}
 }
@@ -373,4 +396,9 @@ func TestDocumentsRepository__Delete(t *testing.T) {
 		_, ok := deletedDocIDs[doc.DocumentID]
 		require.Equal(t, doc.deleted, ok)
 	}
+
+	// make sure we find the document as deleted
+	exists, err := repo.exists(customerID, docs[0].DocumentID, "")
+	require.Equal(t, false, exists)
+	require.Equal(t, err, sql.ErrNoRows)
 }
diff --git a/pkg/documents/storage/bucket_test.go b/pkg/documents/storage/bucket_test.go
index 414b126..be9e29a 100644
--- a/pkg/documents/storage/bucket_test.go
+++ b/pkg/documents/storage/bucket_test.go
@@ -8,15 +8,17 @@ import (
 	"context"
 	"io/ioutil"
 	"testing"
+
+	"github.com/go-kit/kit/log"
 )
 
 func TestBucket__openBucket(t *testing.T) {
 	// test the invalid cases
-	bucket, err := openBucket(context.TODO(), "", "", nil)
+	bucket, err := openBucket(context.TODO(), log.NewNopLogger(), "", "", nil)
 	if bucket != nil || err == nil {
 		t.Errorf("expected error: bucket=%v error=%v", bucket, err)
 	}
-	bucket, err = openBucket(context.TODO(), "", "other", nil)
+	bucket, err = openBucket(context.TODO(), log.NewNopLogger(), "", "other", nil)
 	if bucket != nil || err == nil {
 		t.Errorf("expected error: bucket=%v error=%v", bucket, err)
 	}
@@ -30,7 +32,7 @@ func TestBucket__GetBucket(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	bucket, err := GetBucket(dir, "file", signer)()
+	bucket, err := GetBucket(log.NewNopLogger(), dir, "file", signer)()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -39,20 +41,20 @@ func TestBucket__GetBucket(t *testing.T) {
 	}
 
 	// error case
-	if _, err := GetBucket("", "", nil)(); err == nil {
+	if _, err := GetBucket(log.NewNopLogger(), "", "", nil)(); err == nil {
 		t.Fatal("expected error")
 	}
 }
 
 func TestBucketAWS(t *testing.T) {
-	bucket, err := openBucket(context.TODO(), "", "aws", nil)
+	bucket, err := openBucket(context.TODO(), log.NewNopLogger(), "", "aws", nil)
 	if err == nil || bucket != nil {
 		t.Errorf("expected error bucket=%v", bucket)
 	}
 }
 
 func TestBucketGCP(t *testing.T) {
-	bucket, err := openBucket(context.TODO(), "", "gcp", nil)
+	bucket, err := openBucket(context.TODO(), log.NewNopLogger(), "", "gcp", nil)
 	if err == nil || bucket != nil {
 		t.Errorf("expected error bucket=%v", bucket)
 	}
diff --git a/pkg/documents/storage/test_bucket.go b/pkg/documents/storage/test_bucket.go
index 8d5ca3b..bfe4808 100644
--- a/pkg/documents/storage/test_bucket.go
+++ b/pkg/documents/storage/test_bucket.go
@@ -6,6 +6,7 @@ import (
 	"io/ioutil"
 	"time"
 
+	"github.com/go-kit/kit/log"
 	"gocloud.dev/blob"
 )
 
@@ -20,6 +21,6 @@ var (
 		defer cancelFn()
 
 		dir, _ := ioutil.TempDir("", "testBucket")
-		return fileBucket(ctx, dir, signer)
+		return fileBucket(ctx, log.NewNopLogger(), dir, signer)
 	}
 )