From 6325c5b569e00bd1e5af58190868994a503f84ec Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Tue, 22 Jan 2019 16:58:22 -0400 Subject: [PATCH] MM-13718 Prevent files from being attached to multiple posts (#10094) * MM-13718 Prevent files from being attached to multiple posts * Switch back to non-batched AttachToPost * Change status code when failing to attach a file --- api4/file_test.go | 4 +- app/import_functions.go | 2 +- app/post.go | 34 +++++++-- app/post_test.go | 57 +++++++++++++++ app/slackimport.go | 4 +- model/post.go | 3 + store/sqlstore/file_info_store.go | 20 +++++- store/store.go | 2 +- store/storetest/file_info_store.go | 98 ++++++++++++++++---------- store/storetest/mocks/FileInfoStore.go | 10 +-- 10 files changed, 176 insertions(+), 58 deletions(-) diff --git a/api4/file_test.go b/api4/file_test.go index 4ec8c5e7cb..79d94f986b 100644 --- a/api4/file_test.go +++ b/api4/file_test.go @@ -951,7 +951,7 @@ func TestGetFileLink(t *testing.T) { CheckBadRequestStatus(t, resp) // Hacky way to assign file to a post (usually would be done by CreatePost call) - store.Must(th.App.Srv.Store.FileInfo().AttachToPost(fileId, th.BasicPost.Id)) + store.Must(th.App.Srv.Store.FileInfo().AttachToPost(fileId, th.BasicPost.Id, th.BasicUser.Id)) th.App.UpdateConfig(func(cfg *model.Config) { cfg.FileSettings.EnablePublicLink = false }) _, resp = Client.GetFileLink(fileId) @@ -1143,7 +1143,7 @@ func TestGetPublicFile(t *testing.T) { } // Hacky way to assign file to a post (usually would be done by CreatePost call) - store.Must(th.App.Srv.Store.FileInfo().AttachToPost(fileId, th.BasicPost.Id)) + store.Must(th.App.Srv.Store.FileInfo().AttachToPost(fileId, th.BasicPost.Id, th.BasicUser.Id)) result := <-th.App.Srv.Store.FileInfo().Get(fileId) info := result.Data.(*model.FileInfo) diff --git a/app/import_functions.go b/app/import_functions.go index 88a4c1d189..8038b54b51 100644 --- a/app/import_functions.go +++ b/app/import_functions.go @@ -1028,7 +1028,7 @@ func (a *App) uploadAttachments(attachments *[]AttachmentImportData, post *model func (a *App) UpdateFileInfoWithPostId(post *model.Post) { for _, fileId := range post.FileIds { - if result := <-a.Srv.Store.FileInfo().AttachToPost(fileId, post.Id); result.Err != nil { + if result := <-a.Srv.Store.FileInfo().AttachToPost(fileId, post.Id, post.UserId); result.Err != nil { mlog.Error(fmt.Sprintf("Error attaching files to post. postId=%v, fileIds=%v, message=%v", post.Id, post.FileIds, result.Err), mlog.String("post_id", post.Id)) } } diff --git a/app/post.go b/app/post.go index a6a67d1f25..584a63d4f5 100644 --- a/app/post.go +++ b/app/post.go @@ -273,13 +273,8 @@ func (a *App) CreatePost(post *model.Post, channel *model.Channel, triggerWebhoo } if len(post.FileIds) > 0 { - // There's a rare bug where the client sends up duplicate FileIds so protect against that - post.FileIds = utils.RemoveDuplicatesFromStringArray(post.FileIds) - - for _, fileId := range post.FileIds { - if result := <-a.Srv.Store.FileInfo().AttachToPost(fileId, post.Id); result.Err != nil { - mlog.Error(fmt.Sprintf("Encountered error attaching files to post, post_id=%s, user_id=%s, file_ids=%v, err=%v", post.Id, post.FileIds, post.UserId, result.Err), mlog.String("post_id", post.Id)) - } + if err := a.attachFilesToPost(post); err != nil { + mlog.Error("Encountered error attaching files to post", mlog.String("post_id", post.Id), mlog.Any("file_ids", post.FileIds), mlog.Err(result.Err)) } if a.Metrics != nil { @@ -298,6 +293,31 @@ func (a *App) CreatePost(post *model.Post, channel *model.Channel, triggerWebhoo return rpost, nil } +func (a *App) attachFilesToPost(post *model.Post) *model.AppError { + var attachedIds []string + for _, fileId := range post.FileIds { + result := <-a.Srv.Store.FileInfo().AttachToPost(fileId, post.Id, post.UserId) + if result.Err != nil { + mlog.Warn("Failed to attach file to post", mlog.String("file_id", fileId), mlog.String("post_id", post.Id), mlog.Err(result.Err)) + continue + } + + attachedIds = append(attachedIds, fileId) + } + + if len(post.FileIds) != len(attachedIds) { + // We couldn't attach all files to the post, so ensure that post.FileIds reflects what was actually attached + post.FileIds = attachedIds + + result := <-a.Srv.Store.Post().Overwrite(post) + if result.Err != nil { + return result.Err + } + } + + return nil +} + // FillInPostProps should be invoked before saving posts to fill in properties such as // channel_mentions. // diff --git a/app/post_test.go b/app/post_test.go index f39997b17b..1727342ab6 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -187,6 +187,63 @@ func TestCreatePostDeduplicate(t *testing.T) { }) } +func TestAttachFilesToPost(t *testing.T) { + t.Run("should attach files", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + info1 := store.Must(th.App.Srv.Store.FileInfo().Save(&model.FileInfo{ + CreatorId: th.BasicUser.Id, + Path: "path.txt", + })).(*model.FileInfo) + info2 := store.Must(th.App.Srv.Store.FileInfo().Save(&model.FileInfo{ + CreatorId: th.BasicUser.Id, + Path: "path.txt", + })).(*model.FileInfo) + + post := th.BasicPost + post.FileIds = []string{info1.Id, info2.Id} + + err := th.App.attachFilesToPost(post) + assert.Nil(t, err) + + infos, err := th.App.GetFileInfosForPost(post.Id) + assert.Nil(t, err) + assert.Len(t, infos, 2) + }) + + t.Run("should update File.PostIds after failing to add files", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + info1 := store.Must(th.App.Srv.Store.FileInfo().Save(&model.FileInfo{ + CreatorId: th.BasicUser.Id, + Path: "path.txt", + PostId: model.NewId(), + })).(*model.FileInfo) + info2 := store.Must(th.App.Srv.Store.FileInfo().Save(&model.FileInfo{ + CreatorId: th.BasicUser.Id, + Path: "path.txt", + })).(*model.FileInfo) + + post := th.BasicPost + post.FileIds = []string{info1.Id, info2.Id} + + err := th.App.attachFilesToPost(post) + assert.Nil(t, err) + + infos, err := th.App.GetFileInfosForPost(post.Id) + assert.Nil(t, err) + assert.Len(t, infos, 1) + assert.Equal(t, info2.Id, infos[0].Id) + + updated, err := th.App.GetSinglePost(post.Id) + require.Nil(t, err) + assert.Len(t, updated.FileIds, 1) + assert.Contains(t, updated.FileIds, info2.Id) + }) +} + func TestUpdatePostEditAt(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() diff --git a/app/slackimport.go b/app/slackimport.go index a9863fe10b..546f59ce38 100644 --- a/app/slackimport.go +++ b/app/slackimport.go @@ -248,7 +248,7 @@ func (a *App) SlackAddPosts(teamId string, channel *model.Channel, posts []Slack } a.OldImportPost(&newPost) for _, fileId := range newPost.FileIds { - if result := <-a.Srv.Store.FileInfo().AttachToPost(fileId, newPost.Id); result.Err != nil { + if result := <-a.Srv.Store.FileInfo().AttachToPost(fileId, newPost.Id, newPost.UserId); result.Err != nil { mlog.Error(fmt.Sprintf("Slack Import: An error occurred when attaching files to a message, post_id=%s, file_ids=%v, err=%v.", newPost.Id, newPost.FileIds, result.Err)) } } @@ -723,7 +723,7 @@ func (a *App) OldImportPost(post *model.Post) { } for _, fileId := range post.FileIds { - if result := <-a.Srv.Store.FileInfo().AttachToPost(fileId, post.Id); result.Err != nil { + if result := <-a.Srv.Store.FileInfo().AttachToPost(fileId, post.Id, post.UserId); result.Err != nil { mlog.Error(fmt.Sprintf("Error attaching files to post. postId=%v, fileIds=%v, message=%v", post.Id, post.FileIds, result.Err), mlog.String("post_id", post.Id)) } } diff --git a/model/post.go b/model/post.go index 8b107e4049..dd78079436 100644 --- a/model/post.go +++ b/model/post.go @@ -291,6 +291,9 @@ func (o *Post) PreCommit() { } o.GenerateActionIds() + + // There's a rare bug where the client sends up duplicate FileIds so protect against that + o.FileIds = RemoveDuplicateStrings(o.FileIds) } func (o *Post) MakeNonNil() { diff --git a/store/sqlstore/file_info_store.go b/store/sqlstore/file_info_store.go index 824e415835..92cd8966ca 100644 --- a/store/sqlstore/file_info_store.go +++ b/store/sqlstore/file_info_store.go @@ -201,18 +201,32 @@ func (fs SqlFileInfoStore) GetForUser(userId string) store.StoreChannel { }) } -func (fs SqlFileInfoStore) AttachToPost(fileId, postId string) store.StoreChannel { +func (fs SqlFileInfoStore) AttachToPost(fileId, postId, creatorId string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { - if _, err := fs.GetMaster().Exec( + sqlResult, err := fs.GetMaster().Exec( `UPDATE FileInfo SET PostId = :PostId WHERE Id = :Id - AND PostId = ''`, map[string]interface{}{"PostId": postId, "Id": fileId}); err != nil { + AND PostId = '' + AND CreatorId = :CreatorId`, map[string]interface{}{"PostId": postId, "Id": fileId, "CreatorId": creatorId}) + if err != nil { result.Err = model.NewAppError("SqlFileInfoStore.AttachToPost", "store.sql_file_info.attach_to_post.app_error", nil, "post_id="+postId+", file_id="+fileId+", err="+err.Error(), http.StatusInternalServerError) + return + } + + count, err := sqlResult.RowsAffected() + if err != nil { + // RowsAffected should never fail with the MySQL or Postgres drivers + result.Err = model.NewAppError("SqlFileInfoStore.AttachToPost", + "store.sql_file_info.attach_to_post.app_error", nil, "post_id="+postId+", file_id="+fileId+", err="+err.Error(), http.StatusInternalServerError) + } else if count == 0 { + // Could not attach the file to the post + result.Err = model.NewAppError("SqlFileInfoStore.AttachToPost", + "store.sql_file_info.attach_to_post.app_error", nil, "post_id="+postId+", file_id="+fileId, http.StatusBadRequest) } }) } diff --git a/store/store.go b/store/store.go index 398529882d..f16c6cbb1e 100644 --- a/store/store.go +++ b/store/store.go @@ -456,7 +456,7 @@ type FileInfoStore interface { GetForPost(postId string, readFromMaster bool, allowFromCache bool) StoreChannel GetForUser(userId string) StoreChannel InvalidateFileInfosForPostCache(postId string) - AttachToPost(fileId string, postId string) StoreChannel + AttachToPost(fileId string, postId string, creatorId string) StoreChannel DeleteForPost(postId string) StoreChannel PermanentDelete(fileId string) StoreChannel PermanentDeleteBatch(endTime int64, limit int64) StoreChannel diff --git a/store/storetest/file_info_store.go b/store/storetest/file_info_store.go index 50b5cf059d..d6ed6982e8 100644 --- a/store/storetest/file_info_store.go +++ b/store/storetest/file_info_store.go @@ -9,6 +9,9 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestFileInfoStore(t *testing.T, ss store.Store) { @@ -204,50 +207,71 @@ func testFileInfoGetForUser(t *testing.T, ss store.Store) { } func testFileInfoAttachToPost(t *testing.T, ss store.Store) { - userId := model.NewId() - postId := model.NewId() + t.Run("should attach files", func(t *testing.T) { + userId := model.NewId() + postId := model.NewId() - info1 := store.Must(ss.FileInfo().Save(&model.FileInfo{ - CreatorId: userId, - Path: "file.txt", - })).(*model.FileInfo) - defer func() { - <-ss.FileInfo().PermanentDelete(info1.Id) - }() + info1 := store.Must(ss.FileInfo().Save(&model.FileInfo{ + CreatorId: userId, + Path: "file.txt", + })).(*model.FileInfo) + info2 := store.Must(ss.FileInfo().Save(&model.FileInfo{ + CreatorId: userId, + Path: "file2.txt", + })).(*model.FileInfo) - if len(info1.PostId) != 0 { - t.Fatal("file shouldn't have a PostId") - } + require.Equal(t, "", info1.PostId) + require.Equal(t, "", info2.PostId) - if result := <-ss.FileInfo().AttachToPost(info1.Id, postId); result.Err != nil { - t.Fatal(result.Err) - } else { - info1 = store.Must(ss.FileInfo().Get(info1.Id)).(*model.FileInfo) - } + result := <-ss.FileInfo().AttachToPost(info1.Id, postId, userId) + assert.Nil(t, result.Err) - if len(info1.PostId) == 0 { - t.Fatal("file should now have a PostId") - } + result = <-ss.FileInfo().AttachToPost(info2.Id, postId, userId) + assert.Nil(t, result.Err) - info2 := store.Must(ss.FileInfo().Save(&model.FileInfo{ - CreatorId: userId, - Path: "file.txt", - })).(*model.FileInfo) - defer func() { - <-ss.FileInfo().PermanentDelete(info2.Id) - }() + result = <-ss.FileInfo().GetForPost(postId, true, false) + assert.Nil(t, result.Err) - if result := <-ss.FileInfo().AttachToPost(info2.Id, postId); result.Err != nil { - t.Fatal(result.Err) - } else { - info2 = store.Must(ss.FileInfo().Get(info2.Id)).(*model.FileInfo) - } + data := result.Data.([]*model.FileInfo) - if result := <-ss.FileInfo().GetForPost(postId, true, false); result.Err != nil { - t.Fatal(result.Err) - } else if infos := result.Data.([]*model.FileInfo); len(infos) != 2 { - t.Fatal("should've returned exactly 2 file infos") - } + assert.Len(t, data, 2) + assert.True(t, data[0].Id == info1.Id || data[0].Id == info2.Id) + assert.True(t, data[1].Id == info1.Id || data[1].Id == info2.Id) + }) + + t.Run("should not attach files to multiple posts", func(t *testing.T) { + userId := model.NewId() + postId := model.NewId() + + info := store.Must(ss.FileInfo().Save(&model.FileInfo{ + CreatorId: userId, + Path: "file.txt", + })).(*model.FileInfo) + + require.Equal(t, "", info.PostId) + + result := <-ss.FileInfo().AttachToPost(info.Id, model.NewId(), userId) + assert.Nil(t, result.Err) + + result = <-ss.FileInfo().AttachToPost(info.Id, postId, userId) + assert.NotNil(t, result.Err) + }) + + t.Run("should not attach files owned from a different user", func(t *testing.T) { + userId := model.NewId() + postId := model.NewId() + + info := store.Must(ss.FileInfo().Save(&model.FileInfo{ + CreatorId: model.NewId(), + Path: "file.txt", + })).(*model.FileInfo) + + require.Equal(t, "", info.PostId) + + result := <-ss.FileInfo().AttachToPost(info.Id, postId, userId) + + assert.NotNil(t, result.Err) + }) } func testFileInfoDeleteForPost(t *testing.T, ss store.Store) { diff --git a/store/storetest/mocks/FileInfoStore.go b/store/storetest/mocks/FileInfoStore.go index fa8ee444f3..22de7f4d0a 100644 --- a/store/storetest/mocks/FileInfoStore.go +++ b/store/storetest/mocks/FileInfoStore.go @@ -13,13 +13,13 @@ type FileInfoStore struct { mock.Mock } -// AttachToPost provides a mock function with given fields: fileId, postId -func (_m *FileInfoStore) AttachToPost(fileId string, postId string) store.StoreChannel { - ret := _m.Called(fileId, postId) +// AttachToPost provides a mock function with given fields: fileId, postId, creatorId +func (_m *FileInfoStore) AttachToPost(fileId string, postId string, creatorId string) store.StoreChannel { + ret := _m.Called(fileId, postId, creatorId) var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string) store.StoreChannel); ok { - r0 = rf(fileId, postId) + if rf, ok := ret.Get(0).(func(string, string, string) store.StoreChannel); ok { + r0 = rf(fileId, postId, creatorId) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(store.StoreChannel)