MM-17438: allow attaching nouser files to posts (#11837)

* tweak AttachToPost formatting

* test attaching files uploaded by nouser

* MM-17438: allow attaching nouser files to posts

api4/post_test.go

@@ -76,6 +76,60 @@ func TestCreatePost(t *testing.T) {
 		t.Fatal("create at should not match")
 	}
 
+	t.Run("with file uploaded by same user", func(t *testing.T) {
+		fileResp, subResponse := Client.UploadFile([]byte("data"), th.BasicChannel.Id, "test")
+		CheckNoError(t, subResponse)
+		fileId := fileResp.FileInfos[0].Id
+
+		postWithFiles, subResponse := Client.CreatePost(&model.Post{
+			ChannelId: th.BasicChannel.Id,
+			Message:   "with files",
+			FileIds:   model.StringArray{fileId},
+		})
+		CheckNoError(t, subResponse)
+		assert.Equal(t, model.StringArray{fileId}, postWithFiles.FileIds)
+
+		actualPostWithFiles, subResponse := Client.GetPost(postWithFiles.Id, "")
+		CheckNoError(t, subResponse)
+		assert.Equal(t, model.StringArray{fileId}, actualPostWithFiles.FileIds)
+	})
+
+	t.Run("with file uploaded by different user", func(t *testing.T) {
+		fileResp, subResponse := th.SystemAdminClient.UploadFile([]byte("data"), th.BasicChannel.Id, "test")
+		CheckNoError(t, subResponse)
+		fileId := fileResp.FileInfos[0].Id
+
+		postWithFiles, subResponse := Client.CreatePost(&model.Post{
+			ChannelId: th.BasicChannel.Id,
+			Message:   "with files",
+			FileIds:   model.StringArray{fileId},
+		})
+		CheckNoError(t, subResponse)
+		assert.Empty(t, postWithFiles.FileIds)
+
+		actualPostWithFiles, subResponse := Client.GetPost(postWithFiles.Id, "")
+		CheckNoError(t, subResponse)
+		assert.Empty(t, actualPostWithFiles.FileIds)
+	})
+
+	t.Run("with file uploaded by nouser", func(t *testing.T) {
+		fileInfo, err := th.App.UploadFile([]byte("data"), th.BasicChannel.Id, "test")
+		require.Nil(t, err)
+		fileId := fileInfo.Id
+
+		postWithFiles, subResponse := Client.CreatePost(&model.Post{
+			ChannelId: th.BasicChannel.Id,
+			Message:   "with files",
+			FileIds:   model.StringArray{fileId},
+		})
+		CheckNoError(t, subResponse)
+		assert.Equal(t, model.StringArray{fileId}, postWithFiles.FileIds)
+
+		actualPostWithFiles, subResponse := Client.GetPost(postWithFiles.Id, "")
+		CheckNoError(t, subResponse)
+		assert.Equal(t, model.StringArray{fileId}, actualPostWithFiles.FileIds)
+	})
+
 	post.RootId = ""
 	post.ParentId = ""
 	post.Type = model.POST_SYSTEM_GENERIC

app/plugin_api_test.go

@@ -319,6 +319,7 @@ func TestPluginAPIGetFile(t *testing.T) {
 	require.NotNil(t, err)
 	require.Nil(t, data)
 }
+
 func TestPluginAPISavePluginConfig(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()
@@ -1244,3 +1245,37 @@ func TestPluginCreateBot(t *testing.T) {
 	require.NotNil(t, err)
 
 }
+
+func TestPluginCreatePostWithUploadedFile(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+	api := th.SetupPluginAPI()
+
+	data := []byte("Hello World")
+	channelId := th.BasicChannel.Id
+	filename := "testGetFile"
+	fileInfo, err := api.UploadFile(data, channelId, filename)
+	require.Nil(t, err)
+	defer func() {
+		th.App.Srv.Store.FileInfo().PermanentDelete(fileInfo.Id)
+		th.App.RemoveFile(fileInfo.Path)
+	}()
+
+	actualData, err := api.GetFile(fileInfo.Id)
+	require.Nil(t, err)
+	assert.Equal(t, data, actualData)
+
+	userId := th.BasicUser.Id
+	post, err := api.CreatePost(&model.Post{
+		Message:   "test",
+		UserId:    userId,
+		ChannelId: channelId,
+		FileIds:   model.StringArray{fileInfo.Id},
+	})
+	require.Nil(t, err)
+	assert.Equal(t, model.StringArray{fileInfo.Id}, post.FileIds)
+
+	actualPost, err := api.GetPost(post.Id)
+	require.Nil(t, err)
+	assert.Equal(t, model.StringArray{fileInfo.Id}, actualPost.FileIds)
+}

store/sqlstore/file_info_store.go

@@ -196,15 +196,20 @@ func (fs SqlFileInfoStore) GetForUser(userId string) ([]*model.FileInfo, *model.
 }
 
 func (fs SqlFileInfoStore) AttachToPost(fileId, postId, creatorId string) *model.AppError {
-	sqlResult, err := fs.GetMaster().Exec(
+	sqlResult, err := fs.GetMaster().Exec(`
-		`UPDATE
+		UPDATE
-					FileInfo
+			FileInfo
-				SET
+		SET
-					PostId = :PostId
+			PostId = :PostId
-				WHERE
+		WHERE
-					Id = :Id
+			Id = :Id
-					AND PostId = ''
+			AND PostId = ''
-					AND CreatorId = :CreatorId`, map[string]interface{}{"PostId": postId, "Id": fileId, "CreatorId": creatorId})
+			AND (CreatorId = :CreatorId OR CreatorId = 'nouser')
+	`, map[string]interface{}{
+		"PostId":    postId,
+		"Id":        fileId,
+		"CreatorId": creatorId,
+	})
 	if err != nil {
 		return 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)

store/storetest/file_info_store.go

@@ -5,6 +5,7 @@ package storetest
 
 import (
 	"fmt"
+	"sort"
 	"testing"
 
 	"github.com/mattermost/mattermost-server/model"
@@ -255,6 +256,12 @@ func testFileInfoGetForUser(t *testing.T, ss store.Store) {
 	assert.Len(t, userPosts, 1)
 }
 
+type byFileInfoId []*model.FileInfo
+
+func (a byFileInfoId) Len() int           { return len(a) }
+func (a byFileInfoId) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
+func (a byFileInfoId) Less(i, j int) bool { return a[i].Id < a[j].Id }
+
 func testFileInfoAttachToPost(t *testing.T, ss store.Store) {
 	t.Run("should attach files", func(t *testing.T) {
 		userId := model.NewId()
@@ -276,16 +283,19 @@ func testFileInfoAttachToPost(t *testing.T, ss store.Store) {
 
 		err = ss.FileInfo().AttachToPost(info1.Id, postId, userId)
 		assert.Nil(t, err)
+		info1.PostId = postId
 
 		err = ss.FileInfo().AttachToPost(info2.Id, postId, userId)
 		assert.Nil(t, err)
+		info2.PostId = postId
 
 		data, err := ss.FileInfo().GetForPost(postId, true, false, false)
-		assert.Nil(t, err)
+		require.Nil(t, err)
 
-		assert.Len(t, data, 2)
+		expected := []*model.FileInfo{info1, info2}
-		assert.True(t, data[0].Id == info1.Id || data[0].Id == info2.Id)
+		sort.Sort(byFileInfoId(expected))
-		assert.True(t, data[1].Id == info1.Id || data[1].Id == info2.Id)
+		sort.Sort(byFileInfoId(data))
+		assert.Equal(t, expected, data)
 	})
 
 	t.Run("should not attach files to multiple posts", func(t *testing.T) {
@@ -301,10 +311,10 @@ func testFileInfoAttachToPost(t *testing.T, ss store.Store) {
 		require.Equal(t, "", info.PostId)
 
 		err = ss.FileInfo().AttachToPost(info.Id, model.NewId(), userId)
-		assert.Nil(t, err)
+		require.Nil(t, err)
 
 		err = ss.FileInfo().AttachToPost(info.Id, postId, userId)
-		assert.NotNil(t, err)
+		require.NotNil(t, err)
 	})
 
 	t.Run("should not attach files owned from a different user", func(t *testing.T) {
@@ -322,6 +332,25 @@ func testFileInfoAttachToPost(t *testing.T, ss store.Store) {
 		err = ss.FileInfo().AttachToPost(info.Id, postId, userId)
 		assert.NotNil(t, err)
 	})
+
+	t.Run("should attach files uploaded by nouser", func(t *testing.T) {
+		postId := model.NewId()
+
+		info, err := ss.FileInfo().Save(&model.FileInfo{
+			CreatorId: "nouser",
+			Path:      "file.txt",
+		})
+		require.Nil(t, err)
+		assert.Equal(t, "", info.PostId)
+
+		err = ss.FileInfo().AttachToPost(info.Id, postId, model.NewId())
+		require.Nil(t, err)
+
+		data, err := ss.FileInfo().GetForPost(postId, true, false, false)
+		require.Nil(t, err)
+		info.PostId = postId
+		assert.Equal(t, []*model.FileInfo{info}, data)
+	})
 }
 
 func testFileInfoDeleteForPost(t *testing.T, ss store.Store) {