diff options
-rw-r--r-- | api/file.go | 2 | ||||
-rw-r--r-- | api4/context.go | 12 | ||||
-rw-r--r-- | api4/file.go | 69 | ||||
-rw-r--r-- | api4/file_test.go | 129 | ||||
-rw-r--r-- | api4/params.go | 14 | ||||
-rw-r--r-- | app/file.go | 39 | ||||
-rw-r--r-- | i18n/en.json | 4 | ||||
-rw-r--r-- | model/client4.go | 9 |
8 files changed, 242 insertions, 36 deletions
diff --git a/api/file.go b/api/file.go index 3b8984816..63432eda5 100644 --- a/api/file.go +++ b/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 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 diff --git a/app/file.go b/app/file.go index bb20585bb..06ee61c92 100644 --- a/app/file.go +++ b/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 { - 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() - + for i, file := range files { 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 } diff --git a/i18n/en.json b/i18n/en.json index 033aa85ea..dcbcd4c8d 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1369,6 +1369,10 @@ "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}}" }, diff --git a/model/client4.go b/model/client4.go index 962b816bb..4772d38b3 100644 --- a/model/client4.go +++ b/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 { |