MM-9556 Added ability to upload files without a multipart request (#8306)

* MM-9556 Added ability to upload files without a multipart request

* MM-9556 Handled some unusual test behaviour

api/file.go

@@ -76,7 +76,7 @@ func uploadFile(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	resStruct, err := c.App.UploadFiles(c.TeamId, channelId, c.Session.UserId, m.File["files"], m.Value["client_ids"])
+	resStruct, err := c.App.UploadMultipartFiles(c.TeamId, channelId, c.Session.UserId, m.File["files"], m.Value["client_ids"])
 	if err != nil {
 		c.Err = err
 		return

api4/context.go

@@ -447,6 +447,18 @@ func (c *Context) RequireFileId() *Context {
 	return c
 }
 
+func (c *Context) RequireFilename() *Context {
+	if c.Err != nil {
+		return c
+	}
+
+	if len(c.Params.Filename) == 0 {
+		c.SetInvalidUrlParam("filename")
+	}
+
+	return c
+}
+
 func (c *Context) RequirePluginId() *Context {
 	if c.Err != nil {
 		return c

api4/file.go

@@ -4,6 +4,7 @@
 package api4
 
 import (
+	"io"
 	"net/http"
 	"net/url"
 	"strconv"
@@ -65,32 +66,62 @@ func uploadFile(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if err := r.ParseMultipartForm(*c.App.Config().FileSettings.MaxFileSize); err != nil {
+	var resStruct *model.FileUploadResponse
+	var appErr *model.AppError
+
+	if err := r.ParseMultipartForm(*c.App.Config().FileSettings.MaxFileSize); err != nil && err != http.ErrNotMultipart {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
+	} else if err == http.ErrNotMultipart {
+		defer r.Body.Close()
+
+		c.RequireChannelId()
+		c.RequireFilename()
+
+		if c.Err != nil {
+			return
+		}
+
+		channelId := c.Params.ChannelId
+		filename := c.Params.Filename
+
+		if !c.App.SessionHasPermissionToChannel(c.Session, channelId, model.PERMISSION_UPLOAD_FILE) {
+			c.SetPermissionError(model.PERMISSION_UPLOAD_FILE)
+			return
+		}
+
+		resStruct, appErr = c.App.UploadFiles(
+			FILE_TEAM_ID,
+			channelId,
+			c.Session.UserId,
+			[]io.ReadCloser{r.Body},
+			[]string{filename},
+			[]string{},
+		)
+	} else {
+		m := r.MultipartForm
+
+		props := m.Value
+		if len(props["channel_id"]) == 0 {
+			c.SetInvalidParam("channel_id")
+			return
+		}
+		channelId := props["channel_id"][0]
+		if len(channelId) == 0 {
+			c.SetInvalidParam("channel_id")
+			return
+		}
+
+		if !c.App.SessionHasPermissionToChannel(c.Session, channelId, model.PERMISSION_UPLOAD_FILE) {
+			c.SetPermissionError(model.PERMISSION_UPLOAD_FILE)
+			return
+		}
+
+		resStruct, appErr = c.App.UploadMultipartFiles(FILE_TEAM_ID, channelId, c.Session.UserId, m.File["files"], m.Value["client_ids"])
 	}
 
-	m := r.MultipartForm
+	if appErr != nil {
-
+		c.Err = appErr
-	props := m.Value
-	if len(props["channel_id"]) == 0 {
-		c.SetInvalidParam("channel_id")
-		return
-	}
-	channelId := props["channel_id"][0]
-	if len(channelId) == 0 {
-		c.SetInvalidParam("channel_id")
-		return
-	}
-
-	if !c.App.SessionHasPermissionToChannel(c.Session, channelId, model.PERMISSION_UPLOAD_FILE) {
-		c.SetPermissionError(model.PERMISSION_UPLOAD_FILE)
-		return
-	}
-
-	resStruct, err := c.App.UploadFiles(FILE_TEAM_ID, channelId, c.Session.UserId, m.File["files"], m.Value["client_ids"])
-	if err != nil {
-		c.Err = err
 		return
 	}
 

api4/file_test.go

@@ -14,7 +14,7 @@ import (
 	"github.com/mattermost/mattermost-server/store"
 )
 
-func TestUploadFile(t *testing.T) {
+func TestUploadFileAsMultipart(t *testing.T) {
 	th := Setup().InitBasic().InitSystemAdmin()
 	defer th.TearDown()
 	Client := th.Client
@@ -119,7 +119,132 @@ func TestUploadFile(t *testing.T) {
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnableFileAttachments = false })
 
 	_, resp = th.SystemAdminClient.UploadFile(data, channel.Id, "test.png")
-	if resp.StatusCode != http.StatusNotImplemented && resp.StatusCode != 0 {
+	if resp.StatusCode == 0 {
+		t.Log("file upload request failed completely")
+	} else if resp.StatusCode != http.StatusNotImplemented {
+		// This should return an HTTP 501, but it occasionally causes the http client itself to error
+		t.Fatalf("should've returned HTTP 501 or failed completely, got %v instead", resp.StatusCode)
+	}
+}
+
+func TestUploadFileAsRequestBody(t *testing.T) {
+	th := Setup().InitBasic().InitSystemAdmin()
+	defer th.TearDown()
+	Client := th.Client
+
+	user := th.BasicUser
+	channel := th.BasicChannel
+
+	var uploadInfo *model.FileInfo
+	var data []byte
+	var err error
+	if data, err = readTestFile("test.png"); err != nil {
+		t.Fatal(err)
+	} else if fileResp, resp := Client.UploadFileAsRequestBody(data, channel.Id, "test.png"); resp.Error != nil {
+		t.Fatal(resp.Error)
+	} else if len(fileResp.FileInfos) != 1 {
+		t.Fatal("should've returned a single file infos")
+	} else {
+		uploadInfo = fileResp.FileInfos[0]
+	}
+
+	// The returned file info from the upload call will be missing some fields that will be stored in the database
+	if uploadInfo.CreatorId != user.Id {
+		t.Fatal("file should be assigned to user")
+	} else if uploadInfo.PostId != "" {
+		t.Fatal("file shouldn't have a post")
+	} else if uploadInfo.Path != "" {
+		t.Fatal("file path should not be set on returned info")
+	} else if uploadInfo.ThumbnailPath != "" {
+		t.Fatal("file thumbnail path should not be set on returned info")
+	} else if uploadInfo.PreviewPath != "" {
+		t.Fatal("file preview path should not be set on returned info")
+	}
+
+	var info *model.FileInfo
+	if result := <-th.App.Srv.Store.FileInfo().Get(uploadInfo.Id); result.Err != nil {
+		t.Fatal(result.Err)
+	} else {
+		info = result.Data.(*model.FileInfo)
+	}
+
+	if info.Id != uploadInfo.Id {
+		t.Fatal("file id from response should match one stored in database")
+	} else if info.CreatorId != user.Id {
+		t.Fatal("file should be assigned to user")
+	} else if info.PostId != "" {
+		t.Fatal("file shouldn't have a post")
+	} else if info.Path == "" {
+		t.Fatal("file path should be set in database")
+	} else if info.ThumbnailPath == "" {
+		t.Fatal("file thumbnail path should be set in database")
+	} else if info.PreviewPath == "" {
+		t.Fatal("file preview path should be set in database")
+	}
+
+	date := time.Now().Format("20060102")
+
+	// This also makes sure that the relative path provided above is sanitized out
+	expectedPath := fmt.Sprintf("%v/teams/%v/channels/%v/users/%v/%v/test.png", date, FILE_TEAM_ID, channel.Id, user.Id, info.Id)
+	if info.Path != expectedPath {
+		t.Logf("file is saved in %v", info.Path)
+		t.Fatalf("file should've been saved in %v", expectedPath)
+	}
+
+	expectedThumbnailPath := fmt.Sprintf("%v/teams/%v/channels/%v/users/%v/%v/test_thumb.jpg", date, FILE_TEAM_ID, channel.Id, user.Id, info.Id)
+	if info.ThumbnailPath != expectedThumbnailPath {
+		t.Logf("file thumbnail is saved in %v", info.ThumbnailPath)
+		t.Fatalf("file thumbnail should've been saved in %v", expectedThumbnailPath)
+	}
+
+	expectedPreviewPath := fmt.Sprintf("%v/teams/%v/channels/%v/users/%v/%v/test_preview.jpg", date, FILE_TEAM_ID, channel.Id, user.Id, info.Id)
+	if info.PreviewPath != expectedPreviewPath {
+		t.Logf("file preview is saved in %v", info.PreviewPath)
+		t.Fatalf("file preview should've been saved in %v", expectedPreviewPath)
+	}
+
+	// Wait a bit for files to ready
+	time.Sleep(2 * time.Second)
+
+	if err := th.cleanupTestFile(info); err != nil {
+		t.Fatal(err)
+	}
+
+	_, resp := Client.UploadFileAsRequestBody(data, model.NewId(), "test.png")
+	CheckForbiddenStatus(t, resp)
+
+	_, resp = Client.UploadFileAsRequestBody(data, "../../junk", "test.png")
+	if resp.StatusCode == 0 {
+		t.Log("file upload request failed completely")
+	} else if resp.StatusCode != http.StatusBadRequest {
+		// This should return an HTTP 400, but it occasionally causes the http client itself to error
+		t.Fatalf("should've returned HTTP 400 or failed completely, got %v instead", resp.StatusCode)
+	}
+
+	_, resp = th.SystemAdminClient.UploadFileAsRequestBody(data, model.NewId(), "test.png")
+	CheckForbiddenStatus(t, resp)
+
+	_, resp = th.SystemAdminClient.UploadFileAsRequestBody(data, "../../junk", "test.png")
+	if resp.StatusCode == 0 {
+		t.Log("file upload request failed completely")
+	} else if resp.StatusCode != http.StatusBadRequest {
+		// This should return an HTTP 400, but it occasionally causes the http client itself to error
+		t.Fatalf("should've returned HTTP 400 or failed completely, got %v instead", resp.StatusCode)
+	}
+
+	_, resp = th.SystemAdminClient.UploadFileAsRequestBody(data, channel.Id, "test.png")
+	CheckNoError(t, resp)
+
+	enableFileAttachments := *th.App.Config().FileSettings.EnableFileAttachments
+	defer func() {
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnableFileAttachments = enableFileAttachments })
+	}()
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnableFileAttachments = false })
+
+	_, resp = th.SystemAdminClient.UploadFileAsRequestBody(data, channel.Id, "test.png")
+	if resp.StatusCode == 0 {
+		t.Log("file upload request failed completely")
+	} else if resp.StatusCode != http.StatusNotImplemented {
 		// This should return an HTTP 501, but it occasionally causes the http client itself to error
 		t.Fatalf("should've returned HTTP 501 or failed completely, got %v instead", resp.StatusCode)
 	}

api4/params.go

@@ -27,6 +27,7 @@ type ApiParams struct {
 	ChannelId      string
 	PostId         string
 	FileId         string
+	Filename       string
 	PluginId       string
 	CommandId      string
 	HookId         string
@@ -54,6 +55,7 @@ func ApiParamsFromRequest(r *http.Request) *ApiParams {
 	params := &ApiParams{}
 
 	props := mux.Vars(r)
+	query := r.URL.Query()
 
 	if val, ok := props["user_id"]; ok {
 		params.UserId = val
@@ -73,6 +75,8 @@ func ApiParamsFromRequest(r *http.Request) *ApiParams {
 
 	if val, ok := props["channel_id"]; ok {
 		params.ChannelId = val
+	} else {
+		params.ChannelId = query.Get("channel_id")
 	}
 
 	if val, ok := props["post_id"]; ok {
@@ -83,6 +87,8 @@ func ApiParamsFromRequest(r *http.Request) *ApiParams {
 		params.FileId = val
 	}
 
+	params.Filename = query.Get("filename")
+
 	if val, ok := props["plugin_id"]; ok {
 		params.PluginId = val
 	}
@@ -151,17 +157,17 @@ func ApiParamsFromRequest(r *http.Request) *ApiParams {
 		params.ActionId = val
 	}
 
-	if val, err := strconv.Atoi(r.URL.Query().Get("page")); err != nil || val < 0 {
+	if val, err := strconv.Atoi(query.Get("page")); err != nil || val < 0 {
 		params.Page = PAGE_DEFAULT
 	} else {
 		params.Page = val
 	}
 
-	if val, err := strconv.ParseBool(r.URL.Query().Get("permanent")); err != nil {
+	if val, err := strconv.ParseBool(query.Get("permanent")); err != nil {
 		params.Permanent = val
 	}
 
-	if val, err := strconv.Atoi(r.URL.Query().Get("per_page")); err != nil || val < 0 {
+	if val, err := strconv.Atoi(query.Get("per_page")); err != nil || val < 0 {
 		params.PerPage = PER_PAGE_DEFAULT
 	} else if val > PER_PAGE_MAXIMUM {
 		params.PerPage = PER_PAGE_MAXIMUM
@@ -169,7 +175,7 @@ func ApiParamsFromRequest(r *http.Request) *ApiParams {
 		params.PerPage = val
 	}
 
-	if val, err := strconv.Atoi(r.URL.Query().Get("logs_per_page")); err != nil || val < 0 {
+	if val, err := strconv.Atoi(query.Get("logs_per_page")); err != nil || val < 0 {
 		params.LogsPerPage = LOGS_PER_PAGE_DEFAULT
 	} else if val > LOGS_PER_PAGE_MAXIMUM {
 		params.LogsPerPage = LOGS_PER_PAGE_MAXIMUM

app/file.go

@@ -280,11 +280,38 @@ func GeneratePublicLinkHash(fileId, salt string) string {
 	return base64.RawURLEncoding.EncodeToString(hash.Sum(nil))
 }
 
-func (a *App) UploadFiles(teamId string, channelId string, userId string, fileHeaders []*multipart.FileHeader, clientIds []string) (*model.FileUploadResponse, *model.AppError) {
+func (a *App) UploadMultipartFiles(teamId string, channelId string, userId string, fileHeaders []*multipart.FileHeader, clientIds []string) (*model.FileUploadResponse, *model.AppError) {
+	files := make([]io.ReadCloser, len(fileHeaders))
+	filenames := make([]string, len(fileHeaders))
+
+	for i, fileHeader := range fileHeaders {
+		file, fileErr := fileHeader.Open()
+		if fileErr != nil {
+			return nil, model.NewAppError("UploadFiles", "api.file.upload_file.bad_parse.app_error", nil, fileErr.Error(), http.StatusBadRequest)
+		}
+
+		// Will be closed after UploadFiles returns
+		defer file.Close()
+
+		files[i] = file
+		filenames[i] = fileHeader.Filename
+	}
+
+	return a.UploadFiles(teamId, channelId, userId, files, filenames, clientIds)
+}
+
+// Uploads some files to the given team and channel as the given user. files and filenames should have
+// the same length. clientIds should either not be provided or have the same length as files and filenames.
+// The provided files should be closed by the caller so that they are not leaked.
+func (a *App) UploadFiles(teamId string, channelId string, userId string, files []io.ReadCloser, filenames []string, clientIds []string) (*model.FileUploadResponse, *model.AppError) {
 	if len(*a.Config().FileSettings.DriverName) == 0 {
 		return nil, model.NewAppError("uploadFile", "api.file.upload_file.storage.app_error", nil, "", http.StatusNotImplemented)
 	}
 
+	if len(filenames) != len(files) || (len(clientIds) > 0 && len(clientIds) != len(files)) {
+		return nil, model.NewAppError("UploadFiles", "api.file.upload_file.incorrect_number_of_files.app_error", nil, "", http.StatusBadRequest)
+	}
+
 	resStruct := &model.FileUploadResponse{
 		FileInfos: []*model.FileInfo{},
 		ClientIds: []string{},
@@ -294,18 +321,12 @@ func (a *App) UploadFiles(teamId string, channelId string, userId string, fileHe
 	thumbnailPathList := []string{}
 	imageDataList := [][]byte{}
 
-	for i, fileHeader := range fileHeaders {
+	for i, file := range files {
-		file, fileErr := fileHeader.Open()
-		if fileErr != nil {
-			return nil, model.NewAppError("UploadFiles", "api.file.upload_file.bad_parse.app_error", nil, fileErr.Error(), http.StatusBadRequest)
-		}
-		defer file.Close()
-
 		buf := bytes.NewBuffer(nil)
 		io.Copy(buf, file)
 		data := buf.Bytes()
 
-		info, err := a.DoUploadFile(time.Now(), teamId, channelId, userId, fileHeader.Filename, data)
+		info, err := a.DoUploadFile(time.Now(), teamId, channelId, userId, filenames[i], data)
 		if err != nil {
 			return nil, err
 		}

i18n/en.json

@@ -1368,6 +1368,10 @@
     "id": "api.file.upload_file.bad_parse.app_error",
     "translation": "Unable to upload file. Header cannot be parsed."
   },
+  {
+    "id": "api.file.upload_file.incorrect_number_of_files.app_error",
+    "translation": "Unable to upload files. Incorrect number of files specified."
+  },
   {
     "id": "api.file.upload_file.large_image.app_error",
     "translation": "File above maximum dimensions could not be uploaded: {{.Filename}}"

model/client4.go

@@ -1915,7 +1915,8 @@ func (c *Client4) DoPostAction(postId, actionId string) (bool, *Response) {
 
 // File Section
 
-// UploadFile will upload a file to a channel, to be later attached to a post.
+// UploadFile will upload a file to a channel using a multipart request, to be later attached to a post.
+// This method is functionally equivalent to Client4.UploadFileAsRequestBody.
 func (c *Client4) UploadFile(data []byte, channelId string, filename string) (*FileUploadResponse, *Response) {
 	body := &bytes.Buffer{}
 	writer := multipart.NewWriter(body)
@@ -1939,6 +1940,12 @@ func (c *Client4) UploadFile(data []byte, channelId string, filename string) (*F
 	return c.DoUploadFile(c.GetFilesRoute(), body.Bytes(), writer.FormDataContentType())
 }
 
+// UploadFileAsRequestBody will upload a file to a channel as the body of a request, to be later attached
+// to a post. This method is functionally equivalent to Client4.UploadFile.
+func (c *Client4) UploadFileAsRequestBody(data []byte, channelId string, filename string) (*FileUploadResponse, *Response) {
+	return c.DoUploadFile(c.GetFilesRoute()+fmt.Sprintf("?channel_id=%v&filename=%v", url.QueryEscape(channelId), url.QueryEscape(filename)), data, http.DetectContentType(data))
+}
+
 // GetFile gets the bytes for a file by id.
 func (c *Client4) GetFile(fileId string) ([]byte, *Response) {
 	if r, err := c.DoApiGet(c.GetFileRoute(fileId), ""); err != nil {