From babd795d792e95f6e708af6ee8207ef6877e2b32 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Tue, 20 Feb 2018 10:41:00 -0500 Subject: 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 --- api4/context.go | 12 +++++ api4/file.go | 69 +++++++++++++++++++++-------- api4/file_test.go | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++- api4/params.go | 14 ++++-- 4 files changed, 199 insertions(+), 25 deletions(-) (limited to 'api4') diff --git a/api4/context.go b/api4/context.go index 82c8d9e6c..9f60ab01e 100644 --- a/api4/context.go +++ b/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 diff --git a/api4/file.go b/api4/file.go index acc4c78e5..0b0973b30 100644 --- a/api4/file.go +++ b/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() - m := r.MultipartForm + c.RequireChannelId() + c.RequireFilename() - 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.Err != nil { + return + } - if !c.App.SessionHasPermissionToChannel(c.Session, channelId, model.PERMISSION_UPLOAD_FILE) { - c.SetPermissionError(model.PERMISSION_UPLOAD_FILE) - 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"]) } - 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 + if appErr != nil { + c.Err = appErr return } diff --git a/api4/file_test.go b/api4/file_test.go index 7010b3039..a28420c76 100644 --- a/api4/file_test.go +++ b/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) } diff --git a/api4/params.go b/api4/params.go index 30638578b..070efbbc6 100644 --- a/api4/params.go +++ b/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 -- cgit v1.2.3-1-g7c22