MM-16366 - Prevent attachment duplication on import (#11770)

* prevent attachment duplication

* improved tests

* corrected import into a new post

app/import_functions.go

@@ -5,10 +5,12 @@ package app
 
 import (
 	"bytes"
+	"crypto/sha1"
 	"fmt"
 	"io"
 	"net/http"
 	"os"
+	"path"
 	"strings"
 
 	"github.com/mattermost/mattermost-server/mlog"
@@ -916,8 +918,32 @@ func (a *App) ImportAttachment(data *AttachmentImportData, post *model.Post, tea
 	if file != nil {
 		timestamp := utils.TimeFromMillis(post.CreateAt)
 		buf := bytes.NewBuffer(nil)
-		io.Copy(buf, file)
+		_, _ = io.Copy(buf, file)
+		// Go over existing files in the post and see if there already exists a file with the same name, size and hash. If so - skip it
+		if post.Id != "" {
+			if oldFiles, err := a.GetFileInfosForPost(post.Id, true); err == nil {
+				for _, oldFile := range oldFiles {
+					if oldFile.Name != path.Base(file.Name()) || oldFile.Size != int64(buf.Len()) {
+						continue
+					}
+					// check md5
+					newHash := sha1.Sum(buf.Bytes())
+					oldFileData, err := a.GetFile(oldFile.Id)
+					if err != nil {
+						return nil, model.NewAppError("BulkImport", "app.import.attachment.file_upload.error", map[string]interface{}{"FilePath": *data.Path}, "", http.StatusBadRequest)
+					}
+					oldHash := sha1.Sum(oldFileData)
 
+					if bytes.Equal(oldHash[:], newHash[:]) {
+						mlog.Info(fmt.Sprintf("Skipping uploading of file with name %s, already exists", file.Name()))
+						return nil, nil
+					}
+
+				}
+			} else {
+				return nil, model.NewAppError("BulkImport", "app.import.attachment.file_upload.error", map[string]interface{}{"FilePath": *data.Path}, "", http.StatusBadRequest)
+			}
+		}
 		fileInfo, err := a.DoUploadFile(timestamp, teamId, post.ChannelId, post.UserId, file.Name(), buf.Bytes())
 
 		if err != nil {
@@ -1055,7 +1081,9 @@ func (a *App) uploadAttachments(attachments *[]AttachmentImportData, post *model
 		if err != nil {
 			return nil, err
 		}
-		fileIds = append(fileIds, fileInfo.Id)
+		if fileInfo != nil { // nil is returned when the file was skipped due to duplication
+			fileIds = append(fileIds, fileInfo.Id)
+		}
 	}
 	return fileIds, nil
 }

app/import_functions_test.go

@@ -5,6 +5,8 @@ package app
 
 import (
 	"fmt"
+	"io/ioutil"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -2843,6 +2845,14 @@ func TestImportDirectPostWithAttachments(t *testing.T) {
 
 	testsDir, _ := fileutils.FindDir("tests")
 	testImage := filepath.Join(testsDir, "test.png")
+	testImage2 := filepath.Join(testsDir, "test.svg")
+	// create a temp file with same name as original but with a different first byte
+	tmpFolder, _ := ioutil.TempDir("", "imgFake")
+	testImageFake := filepath.Join(tmpFolder, "test.png")
+	fakeFileData, _ := ioutil.ReadFile(testImage)
+	fakeFileData[0] = 0
+	_ = ioutil.WriteFile(testImageFake, fakeFileData, 0644)
+	defer os.RemoveAll(tmpFolder)
 
 	// Create a user.
 	username := model.NewId()
@@ -2877,12 +2887,63 @@ func TestImportDirectPostWithAttachments(t *testing.T) {
 		Attachments: &[]AttachmentImportData{{Path: &testImage}},
 	}
 
-	if err := th.App.ImportDirectPost(directImportData, false); err != nil {
+	t.Run("Regular import of attachment", func(t *testing.T) {
-		t.Fatalf("Expected success.")
+		if err := th.App.ImportDirectPost(directImportData, false); err != nil {
-	}
+			t.Fatalf("Expected success.")
+		}
 
-	attachments := GetAttachments(user1.Id, th, t)
+		attachments := GetAttachments(user1.Id, th, t)
-	assert.Equal(t, len(attachments), 1)
+		assert.Equal(t, len(attachments), 1)
-	assert.Contains(t, attachments[0].Path, "noteam")
+		assert.Contains(t, attachments[0].Path, "noteam")
-	AssertFileIdsInPost(attachments, th, t)
+		AssertFileIdsInPost(attachments, th, t)
+	})
+
+	t.Run("Attempt to import again with same file entirely, should NOT add an attachment", func(t *testing.T) {
+		if err := th.App.ImportDirectPost(directImportData, false); err != nil {
+			t.Fatalf("Expected success.")
+		}
+
+		attachments := GetAttachments(user1.Id, th, t)
+		assert.Equal(t, len(attachments), 1)
+	})
+
+	t.Run("Attempt to import again with same name and size but different content, SHOULD add an attachment", func(t *testing.T) {
+		directImportDataFake := &DirectPostImportData{
+			ChannelMembers: &[]string{
+				user1.Username,
+				user2.Username,
+			},
+			User:        &user1.Username,
+			Message:     ptrStr("Direct message"),
+			CreateAt:    ptrInt64(model.GetMillis()),
+			Attachments: &[]AttachmentImportData{{Path: &testImageFake}},
+		}
+
+		if err := th.App.ImportDirectPost(directImportDataFake, false); err != nil {
+			t.Fatalf("Expected success.")
+		}
+
+		attachments := GetAttachments(user1.Id, th, t)
+		assert.Equal(t, len(attachments), 2)
+	})
+
+	t.Run("Attempt to import again with same data, SHOULD add an attachment, since it's different name", func(t *testing.T) {
+		directImportData2 := &DirectPostImportData{
+			ChannelMembers: &[]string{
+				user1.Username,
+				user2.Username,
+			},
+			User:        &user1.Username,
+			Message:     ptrStr("Direct message"),
+			CreateAt:    ptrInt64(model.GetMillis()),
+			Attachments: &[]AttachmentImportData{{Path: &testImage2}},
+		}
+
+		if err := th.App.ImportDirectPost(directImportData2, false); err != nil {
+			t.Fatalf("Expected success.")
+		}
+
+		attachments := GetAttachments(user1.Id, th, t)
+		assert.Equal(t, len(attachments), 3)
+	})
 }