Storage: refactor - decouple the Upload method from HTTP concepts (#50378)

* add `IsPathValidationError` util to fs api

* refactor storage.Upload method

* remove unused struct

* extract `RootUpload` constant

* move file validation outside of the service

* Make UploadErrorToStatusCode exported

* refactor pathValidationError check

* if -> switch

Co-authored-by: Tania B <yalyna.ts@gmail.com>

pkg/services/store/http.go

@@ -1,6 +1,8 @@
 package store
 
 import (
+	"errors"
+	"io/ioutil"
 	"net/http"
 	"strings"
 
@@ -9,6 +11,8 @@ import (
 	"github.com/grafana/grafana/pkg/web"
 )
 
+var errFileTooBig = response.Error(400, "Please limit file uploaded under 1MB", errors.New("file is too big"))
+
 // HTTPStorageService passes raw HTTP requests to a well typed storage service
 type HTTPStorageService interface {
 	List(c *models.ReqContext) response.Response
@@ -27,26 +31,92 @@ func ProvideHTTPService(store StorageService) HTTPStorageService {
 	}
 }
 
+func UploadErrorToStatusCode(err error) int {
+	switch {
+	case errors.Is(err, ErrUploadFeatureDisabled):
+		return 400
+
+	case errors.Is(err, ErrUnsupportedFolder):
+		return 400
+
+	case errors.Is(err, ErrFileTooBig):
+		return 400
+
+	case errors.Is(err, ErrInvalidPath):
+		return 400
+
+	case errors.Is(err, ErrInvalidFileType):
+		return 400
+
+	case errors.Is(err, ErrFileAlreadyExists):
+		return 400
+
+	default:
+		return 500
+	}
+}
+
 func (s *httpStorage) Upload(c *models.ReqContext) response.Response {
 	// 32 MB is the default used by FormFile()
 	if err := c.Req.ParseMultipartForm(32 << 20); err != nil {
 		return response.Error(400, "error in parsing form", err)
 	}
-	const MAX_UPLOAD_SIZE = 1024 * 1024
 	c.Req.Body = http.MaxBytesReader(c.Resp, c.Req.Body, MAX_UPLOAD_SIZE)
 	if err := c.Req.ParseMultipartForm(MAX_UPLOAD_SIZE); err != nil {
 		return response.Error(400, "Please limit file uploaded under 1MB", err)
 	}
-	res, err := s.store.Upload(c.Req.Context(), c.SignedInUser, c.Req.MultipartForm)
 
+	files := c.Req.MultipartForm.File["file"]
+	if len(files) != 1 {
+		return response.JSON(400, map[string]interface{}{
+			"message": "please upload files one at a time",
+			"err":     true,
+		})
+	}
+
+	fileHeader := files[0]
+	if fileHeader.Size > MAX_UPLOAD_SIZE {
+		return errFileTooBig
+	}
+
+	// restrict file size based on file size
+	// open each file to copy contents
+	file, err := fileHeader.Open()
+	if err != nil {
+		return response.Error(500, "Internal Server Error", err)
+	}
+	err = file.Close()
+	if err != nil {
+		return response.Error(500, "Internal Server Error", err)
+	}
+	data, err := ioutil.ReadAll(file)
 	if err != nil {
 		return response.Error(500, "Internal Server Error", err)
 	}
 
-	return response.JSON(res.statusCode, map[string]interface{}{
+	if (len(data)) > MAX_UPLOAD_SIZE {
-		"message": res.message,
+		return errFileTooBig
-		"path":    res.path,
+	}
-		"file":    res.fileName,
+
+	path := RootUpload + "/" + fileHeader.Filename
+
+	mimeType := http.DetectContentType(data)
+
+	err = s.store.Upload(c.Req.Context(), c.SignedInUser, UploadRequest{
+		Contents:              data,
+		MimeType:              mimeType,
+		Path:                  path,
+		OverwriteExistingFile: true,
+	})
+
+	if err != nil {
+		return response.Error(UploadErrorToStatusCode(err), err.Error(), err)
+	}
+
+	return response.JSON(200, map[string]interface{}{
+		"message": "Uploaded successfully",
+		"path":    path,
+		"file":    fileHeader.Filename,
 		"err":     true,
 	})
 }

pkg/services/store/service.go

@@ -2,10 +2,9 @@ package store
 
 import (
 	"context"
+	"errors"
 	"fmt"
-	"io/ioutil"
+	"strings"
-	"mime/multipart"
-	"net/http"
 
 	"github.com/grafana/grafana-plugin-sdk-go/data"
 	"github.com/grafana/grafana/pkg/infra/filestorage"
@@ -20,8 +19,19 @@ import (
 
 var grafanaStorageLogger = log.New("grafanaStorageLogger")
 
+var ErrUploadFeatureDisabled = errors.New("upload feature is disabled")
+var ErrUnsupportedFolder = errors.New("unsupported folder for uploads")
+var ErrFileTooBig = errors.New("file is too big")
+var ErrInvalidPath = errors.New("path is invalid")
+var ErrUploadInternalError = errors.New("upload internal error")
+var ErrInvalidFileType = errors.New("invalid file type")
+var ErrFileAlreadyExists = errors.New("file exists")
+
 const RootPublicStatic = "public-static"
+const RootUpload = "upload"
+
 const MAX_UPLOAD_SIZE = 1024 * 1024 // 1MB
+
 type StorageService interface {
 	registry.BackgroundService
 
@@ -31,7 +41,7 @@ type StorageService interface {
 	// Read raw file contents out of the store
 	Read(ctx context.Context, user *models.SignedInUser, path string) (*filestorage.File, error)
 
-	Upload(ctx context.Context, user *models.SignedInUser, form *multipart.Form) (*Response, error)
+	Upload(ctx context.Context, user *models.SignedInUser, req UploadRequest) error
 
 	Delete(ctx context.Context, user *models.SignedInUser, path string) error
 }
@@ -41,14 +51,6 @@ type standardStorageService struct {
 	tree *nestedTree
 }
 
-type Response struct {
-	path       string
-	statusCode int
-	message    string
-	fileName   string
-	err        bool
-}
-
 func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, cfg *setting.Cfg) StorageService {
 	globalRoots := []storageRuntime{
 		newDiskStorage(RootPublicStatic, "Public static files", &StorageLocalDiskConfig{
@@ -68,7 +70,7 @@ func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles,
 		storages := make([]storageRuntime, 0)
 		if features.IsEnabled(featuremgmt.FlagStorageLocalUpload) {
 			config := &StorageSQLConfig{orgId: orgId}
-			storages = append(storages, newSQLStorage("upload", "Local file upload", config, sql).setBuiltin(true))
+			storages = append(storages, newSQLStorage(RootUpload, "Local file upload", config, sql).setBuiltin(true))
 		}
 		return storages
 	}
@@ -122,71 +124,72 @@ func isFileTypeValid(filetype string) bool {
 	return false
 }
 
-func (s *standardStorageService) Upload(ctx context.Context, user *models.SignedInUser, form *multipart.Form) (*Response, error) {
+type UploadRequest struct {
-	response := Response{
+	Contents           []byte
-		path: "upload",
+	MimeType           string
-	}
+	Path               string
-	upload, _ := s.tree.getRoot(getOrgId(user), "upload")
+	CacheControl       string
+	ContentDisposition string
+	Properties         map[string]string
+
+	OverwriteExistingFile bool
+}
+
+func (s *standardStorageService) Upload(ctx context.Context, user *models.SignedInUser, req UploadRequest) error {
+	upload, _ := s.tree.getRoot(getOrgId(user), RootUpload)
 	if upload == nil {
-		response.statusCode = 404
+		return ErrUploadFeatureDisabled
-		response.message = "upload feature is not enabled"
-		response.err = true
-		return &response, fmt.Errorf("upload feature is not enabled")
 	}
 
-	files := form.File["file"]
+	if !strings.HasPrefix(req.Path, RootUpload+"/") {
-	for _, fileHeader := range files {
+		return ErrUnsupportedFolder
-		// Restrict the size of each uploaded file to 1MB based on the header
-		if fileHeader.Size > MAX_UPLOAD_SIZE {
-			response.statusCode = 400
-			response.message = "The uploaded image is too big"
-			response.err = true
-			return &response, nil
-		}
-		// restrict file size based on file size
-		// open each file to copy contents
-		file, err := fileHeader.Open()
-		if err != nil {
-			return nil, err
-		}
-		err = file.Close()
-		if err != nil {
-			return nil, err
-		}
-		data, err := ioutil.ReadAll(file)
-		if err != nil {
-			return nil, err
-		}
-
-		filetype := http.DetectContentType(data)
-		path := "/" + fileHeader.Filename
-
-		grafanaStorageLogger.Info("uploading a file", "filetype", filetype, "path", path)
-		// only allow images to be uploaded
-		if !isFileTypeValid(filetype) {
-			return &Response{
-				statusCode: 400,
-				message:    "unsupported file type uploaded",
-				err:        true,
-			}, nil
-		}
-		err = upload.Upsert(ctx, &filestorage.UpsertFileCommand{
-			Path:     path,
-			Contents: data,
-		})
-		if err != nil {
-			return nil, err
-		}
-		response.message = "Uploaded successfully"
-		response.statusCode = 200
-		response.fileName = fileHeader.Filename
-		response.path = "upload/" + fileHeader.Filename
 	}
-	return &response, nil
+
+	validFileType := isFileTypeValid(req.MimeType)
+	if !validFileType {
+		return ErrInvalidFileType
+	}
+
+	grafanaStorageLogger.Info("uploading a file", "filetype", req.MimeType, "path", req.Path)
+
+	storagePath := strings.TrimPrefix(req.Path, RootUpload)
+
+	if err := filestorage.ValidatePath(storagePath); err != nil {
+		grafanaStorageLogger.Info("uploading file failed due to invalid path", "filetype", req.MimeType, "path", req.Path, "err", err)
+		return ErrInvalidPath
+	}
+
+	if !req.OverwriteExistingFile {
+		file, err := upload.Get(ctx, storagePath)
+		if err != nil {
+			grafanaStorageLogger.Error("failed while checking file existence", "err", err, "path", req.Path)
+			return ErrUploadInternalError
+		}
+
+		if file != nil {
+			return ErrFileAlreadyExists
+		}
+	}
+
+	err := upload.Upsert(ctx, &filestorage.UpsertFileCommand{
+		Path:               storagePath,
+		Contents:           req.Contents,
+		MimeType:           req.MimeType,
+		CacheControl:       req.CacheControl,
+		ContentDisposition: req.ContentDisposition,
+		Properties:         req.Properties,
+	})
+
+	if err != nil {
+		grafanaStorageLogger.Error("failed while uploading the file", "err", err, "path", req.Path)
+		return ErrUploadInternalError
+	}
+
+	return nil
 }
 
 func (s *standardStorageService) Delete(ctx context.Context, user *models.SignedInUser, path string) error {
-	upload, _ := s.tree.getRoot(getOrgId(user), "upload")
+	upload, _ := s.tree.getRoot(getOrgId(user), RootUpload)
 	if upload == nil {
 		return fmt.Errorf("upload feature is not enabled")
 	}

pkg/services/store/service_test.go

@@ -3,7 +3,6 @@ package store
 import (
 	"bytes"
 	"context"
-	"mime/multipart"
 	"os"
 	"path/filepath"
 	"testing"
@@ -11,9 +10,9 @@ import (
 	"github.com/grafana/grafana-plugin-sdk-go/experimental"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/tsdb/testdatasource"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -60,12 +59,12 @@ func TestUpload(t *testing.T) {
 	path, err := os.Getwd()
 	require.NoError(t, err)
 	cfg := &setting.Cfg{AppURL: "http://localhost:3000/", DataPath: path}
-	s := ProvideService(nil, features, cfg)
+	s := ProvideService(sqlstore.InitTestDB(t), features, cfg)
-	testForm := &multipart.Form{
+	request := UploadRequest{
-		Value: map[string][]string{},
+		Contents: make([]byte, 0),
-		File:  map[string][]*multipart.FileHeader{},
+		Path:     "upload/myFile.jpg",
+		MimeType: "image/jpeg",
 	}
-	res, err := s.Upload(context.Background(), dummyUser, testForm)
+	err = s.Upload(context.Background(), dummyUser, request)
 	require.NoError(t, err)
-	assert.Equal(t, res.path, "upload")
 }