Merge pull request #13857 from transcom/INT-B-20766-filename_correct_…

…on_downloads

Int b 20766 filename correct on downloads

pkg/handlers/ghcapi/documents.go

@@ -22,7 +22,7 @@ func payloadForDocumentModel(storer storage.FileStorer, document models.Document
 		if userUpload.Upload.ID == uuid.Nil {
 			return nil, errors.New("No uploads for user")
 		}
-		url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType)
+		url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType, userUpload.Upload.Filename)
 		if err != nil {
 			return nil, err
 		}

pkg/handlers/ghcapi/documents_test.go

@@ -3,6 +3,7 @@ package ghcapi
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 
 	"github.com/go-openapi/strfmt"
 	"github.com/gofrs/uuid"
@@ -64,7 +65,11 @@ func (suite *HandlerSuite) TestGetDocumentHandler() {
 	}
 
 	uploadPayload := documentPayload.Uploads[0]
-	expectedURL := fmt.Sprintf("https://example.com/dir/%s?contentType=%s&signed=test", userUpload.Upload.StorageKey, uploader.FileTypePDF)
+	values := url.Values{}
+	values.Add("response-content-type", uploader.FileTypePDF)
+	values.Add("response-content-disposition", "attachment; filename="+userUpload.Upload.Filename)
+	values.Add("signed", "test")
+	expectedURL := fmt.Sprintf("https://example.com/dir/%s?", userUpload.Upload.StorageKey) + values.Encode()
 	if (uploadPayload.URL).String() != expectedURL {
 		t.Errorf("wrong URL for upload, expected %s, got %s", expectedURL, uploadPayload.URL)
 	}

pkg/handlers/ghcapi/internal/payloads/model_to_payload.go

@@ -1734,7 +1734,7 @@ func ServiceRequestDoc(serviceRequest models.ServiceRequestDocument, storer stor
 
 	if len(serviceRequest.ServiceRequestDocumentUploads) > 0 {
 		for i, serviceRequestUpload := range serviceRequest.ServiceRequestDocumentUploads {
-			url, err := storer.PresignedURL(serviceRequestUpload.Upload.StorageKey, serviceRequestUpload.Upload.ContentType)
+			url, err := storer.PresignedURL(serviceRequestUpload.Upload.StorageKey, serviceRequestUpload.Upload.ContentType, serviceRequestUpload.Upload.Filename)
 			if err != nil {
 				return nil, err
 			}
@@ -1971,7 +1971,7 @@ func ProofOfServiceDoc(proofOfService models.ProofOfServiceDoc, storer storage.F
 	uploads := make([]*ghcmessages.Upload, len(proofOfService.PrimeUploads))
 	if len(proofOfService.PrimeUploads) > 0 {
 		for i, primeUpload := range proofOfService.PrimeUploads {
-			url, err := storer.PresignedURL(primeUpload.Upload.StorageKey, primeUpload.Upload.ContentType)
+			url, err := storer.PresignedURL(primeUpload.Upload.StorageKey, primeUpload.Upload.ContentType, primeUpload.Upload.Filename)
 			if err != nil {
 				return nil, err
 			}
@@ -2027,7 +2027,7 @@ func PayloadForDocumentModel(storer storage.FileStorer, document models.Document
 		if userUpload.Upload.ID == uuid.Nil {
 			return nil, errors.New("no uploads for user")
 		}
-		url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType)
+		url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType, userUpload.Upload.Filename)
 		if err != nil {
 			return nil, err
 		}

pkg/handlers/ghcapi/uploads.go

@@ -109,7 +109,7 @@ func (h UpdateUploadHandler) Handle(params uploadop.UpdateUploadParams) middlewa
 				return nil, apperror.NewBadDataError("unable to update upload")
 			}
 
-			url, err := h.FileStorer().PresignedURL(newUpload.StorageKey, newUpload.ContentType)
+			url, err := h.FileStorer().PresignedURL(newUpload.StorageKey, newUpload.ContentType, newUpload.Filename)
 			if err != nil {
 				return nil, err
 			}

pkg/handlers/internalapi/documents_test.go

@@ -3,6 +3,7 @@ package internalapi
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 
 	"github.com/go-openapi/strfmt"
 	"github.com/gofrs/uuid"
@@ -100,7 +101,11 @@ func (suite *HandlerSuite) TestShowDocumentHandler() {
 	}
 
 	uploadPayload := documentPayload.Uploads[0]
-	expectedURL := fmt.Sprintf("https://example.com/dir/%s?contentType=%s&signed=test", userUpload.Upload.StorageKey, uploader.FileTypePDF)
+	values := url.Values{}
+	values.Add("response-content-type", uploader.FileTypePDF)
+	values.Add("response-content-disposition", "attachment; filename="+userUpload.Upload.Filename)
+	values.Add("signed", "test")
+	expectedURL := fmt.Sprintf("https://example.com/dir/%s?", userUpload.Upload.StorageKey) + values.Encode()
 	if (uploadPayload.URL).String() != expectedURL {
 		t.Errorf("wrong URL for upload, expected %s, got %s", expectedURL, uploadPayload.URL)
 	}

pkg/handlers/internalapi/internal/payloads/model_to_payload.go

@@ -381,7 +381,7 @@ func PayloadForDocumentModel(storer storage.FileStorer, document models.Document
 		if userUpload.Upload.ID == uuid.Nil {
 			return nil, errors.New("no uploads for user")
 		}
-		url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType)
+		url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType, userUpload.Upload.Filename)
 		if err != nil {
 			return nil, err
 		}

pkg/handlers/internalapi/uploads_test.go

@@ -12,10 +12,12 @@ package internalapi
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 
 	"github.com/go-openapi/runtime/middleware"
 	"github.com/go-openapi/strfmt"
 	"github.com/gofrs/uuid"
+	"golang.org/x/text/encoding/charmap"
 
 	"github.com/transcom/mymove/pkg/factory"
 	ppmop "github.com/transcom/mymove/pkg/gen/internalapi/internaloperations/ppm"
@@ -38,6 +40,7 @@ const FixtureXLSX = "Weight Estimator.xlsx"
 const WeightEstimatorFullXLSX = "Weight Estimator Full.xlsx"
 const WeightEstimatorPrefix = "Weight Estimator Full"
 const FixtureEmpty = "empty.pdf"
+const FixtureScreenshot = "Screenshot 2024-10-10 at 10.46.48 AM.png"
 
 func createPrereqs(suite *HandlerSuite, fixtureFile string) (models.Document, uploadop.CreateUploadParams) {
 	document := factory.BuildDocument(suite.DB(), nil, nil)
@@ -464,9 +467,10 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() {
 		suite.NotEmpty(createdResponse.Payload.ID)
 		suite.Equal(FixtureXLS, createdResponse.Payload.Filename)
 		suite.Equal(uploader.FileTypeExcel, createdResponse.Payload.ContentType)
-		suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String())
-		suite.Contains(createdResponse.Payload.URL, upload.ID.String())
-		suite.Contains(createdResponse.Payload.URL, uploader.FileTypeExcel)
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeExcel))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename))
 	})
 
 	suite.Run("uploads .xlsx file", func() {
@@ -488,9 +492,10 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() {
 		suite.NotEmpty(createdResponse.Payload.ID)
 		suite.Equal(FixtureXLSX, createdResponse.Payload.Filename)
 		suite.Equal(uploader.FileTypeExcelXLSX, createdResponse.Payload.ContentType)
-		suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String())
-		suite.Contains(createdResponse.Payload.URL, upload.ID.String())
-		suite.Contains(createdResponse.Payload.URL, uploader.FileTypeExcelXLSX)
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeExcelXLSX))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename))
 	})
 
 	suite.Run("uploads weight estimator .xlsx file (full weight)", func() {
@@ -512,9 +517,10 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() {
 		suite.NotEmpty(createdResponse.Payload.ID)
 		suite.Contains(createdResponse.Payload.Filename, WeightEstimatorPrefix)
 		suite.Equal(uploader.FileTypePDF, createdResponse.Payload.ContentType)
-		suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String())
-		suite.Contains(createdResponse.Payload.URL, upload.ID.String())
-		suite.Contains(createdResponse.Payload.URL, uploader.FileTypePDF)
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypePDF))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename))
 	})
 
 	suite.Run("uploads file for a progear document", func() {
@@ -536,9 +542,10 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() {
 		suite.NotEmpty(createdResponse.Payload.ID)
 		suite.Equal(FixturePNG, createdResponse.Payload.Filename)
 		suite.Equal(uploader.FileTypePNG, createdResponse.Payload.ContentType)
-		suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String())
-		suite.Contains(createdResponse.Payload.URL, upload.ID.String())
-		suite.Contains(createdResponse.Payload.URL, uploader.FileTypePNG)
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypePNG))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename))
 	})
 
 	suite.Run("uploads file for an expense document", func() {
@@ -560,9 +567,43 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() {
 		suite.NotEmpty(createdResponse.Payload.ID)
 		suite.Equal(FixtureJPG, createdResponse.Payload.Filename)
 		suite.Equal(uploader.FileTypeJPEG, createdResponse.Payload.ContentType)
-		suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String())
-		suite.Contains(createdResponse.Payload.URL, upload.ID.String())
-		suite.Contains(createdResponse.Payload.URL, uploader.FileTypeJPEG)
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeJPEG))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename))
+	})
+
+	suite.Run("uploads file with filename characters not supported by ISO8859_1", func() {
+		fakeS3 := storageTest.NewFakeS3Storage(true)
+		document, params := createPPMExpensePrereqs(suite, FixtureScreenshot)
+
+		response := makePPMRequest(suite, params, document.ServiceMember, fakeS3)
+
+		suite.IsType(&ppmop.CreatePPMUploadCreated{}, response)
+
+		createdResponse, _ := response.(*ppmop.CreatePPMUploadCreated)
+
+		upload := models.Upload{}
+		err := suite.DB().Find(&upload, createdResponse.Payload.ID)
+
+		filenameBuffer := make([]byte, 0)
+		for _, r := range upload.Filename {
+			if encodedRune, ok := charmap.ISO8859_1.EncodeRune(r); ok {
+				filenameBuffer = append(filenameBuffer, encodedRune)
+			}
+		}
+
+		suite.NoError(err)
+		suite.Equal("qEnueX0FLpoz4bTnliprog==", upload.Checksum)
+
+		suite.NotEmpty(createdResponse.Payload.ID)
+		suite.Equal(FixtureScreenshot, createdResponse.Payload.Filename)
+		suite.Equal(uploader.FileTypePNG, createdResponse.Payload.ContentType)
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String()))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypePNG))
+		suite.NotContains(createdResponse.Payload.URL, url.QueryEscape(upload.Filename))
+		suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+string(filenameBuffer)))
 	})
 }
 

pkg/handlers/primeapi/payloads/model_to_payload.go

@@ -810,7 +810,7 @@ func Upload(appCtx appcontext.AppContext, storer storage.FileStorer, upload *mod
 		UpdatedAt:   strfmt.DateTime(upload.UpdatedAt),
 	}
 
-	url, err := storer.PresignedURL(upload.StorageKey, upload.ContentType)
+	url, err := storer.PresignedURL(upload.StorageKey, upload.ContentType, upload.Filename)
 	if err == nil {
 		payload.URL = *handlers.FmtURI(url)
 	} else {

pkg/storage/filesystem.go

@@ -95,9 +95,10 @@ func (fs *Filesystem) Delete(key string) error {
 }
 
 // PresignedURL returns a URL that provides access to a file for 15 mintes.
-func (fs *Filesystem) PresignedURL(key, contentType string) (string, error) {
+func (fs *Filesystem) PresignedURL(key, contentType string, filename string) (string, error) {
 	values := url.Values{}
 	values.Add("contentType", contentType)
+	values.Add("filename", filename)
 	url := fs.webRoot + "/" + key + "?" + values.Encode()
 	return url, nil
 }

pkg/storage/filesystem_test.go

@@ -11,12 +11,12 @@ func TestFilesystemPresignedURL(t *testing.T) {
 	}
 	fs := NewFilesystem(fsParams)
 
-	url, err := fs.PresignedURL("key/to/file/12345", "image/jpeg")
+	url, err := fs.PresignedURL("key/to/file/12345", "image/jpeg", "testimage.jpeg")
 	if err != nil {
 		t.Fatalf("could not get presigned url: %s", err)
 	}
 
-	expected := "https://example.text/files/key/to/file/12345?contentType=image%2Fjpeg"
+	expected := "https://example.text/files/key/to/file/12345?contentType=image%2Fjpeg&filename=testimage.jpeg"
 	if url != expected {
 		t.Errorf("wrong presigned url: expected %s, got %s", expected, url)
 	}

pkg/storage/memory.go

@@ -95,9 +95,10 @@ func (fs *Memory) Delete(key string) error {
 }
 
 // PresignedURL returns a URL that provides access to a file for 15 mintes.
-func (fs *Memory) PresignedURL(key, contentType string) (string, error) {
+func (fs *Memory) PresignedURL(key, contentType string, filename string) (string, error) {
 	values := url.Values{}
 	values.Add("contentType", contentType)
+	values.Add("filename", filename)
 	url := fs.webRoot + "/" + key + "?" + values.Encode()
 	return url, nil
 }

pkg/storage/memory_test.go

@@ -11,12 +11,12 @@ func TestMemoryPresignedURL(t *testing.T) {
 	}
 	fs := NewMemory(fsParams)
 
-	url, err := fs.PresignedURL("key/to/file/12345", "image/jpeg")
+	url, err := fs.PresignedURL("key/to/file/12345", "image/jpeg", "testimage.jpeg")
 	if err != nil {
 		t.Fatalf("could not get presigned url: %s", err)
 	}
 
-	expected := "https://example.text/files/key/to/file/12345?contentType=image%2Fjpeg"
+	expected := "https://example.text/files/key/to/file/12345?contentType=image%2Fjpeg&filename=testimage.jpeg"
 	if url != expected {
 		t.Errorf("wrong presigned url: expected %s, got %s", expected, url)
 	}

pkg/storage/s3.go

@@ -11,8 +11,7 @@ import (
 	"github.com/aws/aws-sdk-go-v2/service/s3/types"
 	"github.com/pkg/errors"
 	"github.com/spf13/afero"
-
-	"github.com/transcom/mymove/pkg/models"
+	"golang.org/x/text/encoding/charmap"
 )
 
 // S3 implements the file storage API using S3.
@@ -114,16 +113,28 @@ func (s *S3) TempFileSystem() *afero.Afero {
 }
 
 // PresignedURL returns a URL that provides access to a file for 15 minutes.
-func (s *S3) PresignedURL(key string, contentType string) (string, error) {
+func (s *S3) PresignedURL(key string, contentType string, filename string) (string, error) {
 	namespacedKey := path.Join(s.keyNamespace, key)
 	presignClient := s3.NewPresignClient(s.client)
 
+	filenameBuffer := make([]byte, 0)
+	for _, r := range filename {
+		if encodedRune, ok := charmap.ISO8859_1.EncodeRune(r); ok {
+			filenameBuffer = append(filenameBuffer, encodedRune)
+		}
+	}
+
+	contentDisposition := "attachment"
+	if len(filenameBuffer) > 0 {
+		contentDisposition += "; filename=" + string(filenameBuffer)
+	}
+
 	req, err := presignClient.PresignGetObject(context.Background(),
 		&s3.GetObjectInput{
 			Bucket:                     &s.bucket,
 			Key:                        &namespacedKey,
 			ResponseContentType:        &contentType,
-			ResponseContentDisposition: models.StringPointer("attachment"),
+			ResponseContentDisposition: &contentDisposition,
 		},
 		func(opts *s3.PresignOptions) {
 			opts.Expires = 15 * time.Minute

pkg/storage/storage.go

@@ -37,7 +37,7 @@ type FileStorer interface {
 	Store(string, io.ReadSeeker, string, *string) (*StoreResult, error)
 	Fetch(string) (io.ReadCloser, error)
 	Delete(string) error
-	PresignedURL(string, string) (string, error)
+	PresignedURL(string, string, string) (string, error)
 	FileSystem() *afero.Afero
 	TempFileSystem() *afero.Afero
 	Tags(string) (map[string]string, error)