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

@@ -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)

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))
 		}
 	}

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.
 //

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()

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))
 			}
 		}

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() {

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)
 		}
 	})
 }

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

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) {

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)