[MM-38375] Fix processing bulk import with attachments (#18352)

* Fix improper attachments in replies

* Fix import data path

* Improve errors

* Fix importing attachments directly from zip file

* Add some test cases to cover error paths

app/export.go

@@ -463,7 +463,7 @@ func (a *App) buildPostReplies(postID string, withAttachments bool) ([]ReplyImpo
 			if appErr != nil {
 				return nil, nil, appErr
 			}
-			replyImportObject.Attachments = &attachments
+			replyImportObject.Attachments = &postAttachments
 			if withAttachments && len(postAttachments) > 0 {
 				attachments = append(attachments, postAttachments...)
 			}

app/export_test.go

@@ -5,6 +5,7 @@ package app
 
 import (
 	"bytes"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -638,3 +639,67 @@ func TestBulkExport(t *testing.T) {
 	appErr, _ = th.App.BulkImportWithPath(th.Context, jsonFile, nil, false, 1, filepath.Join(dir, "data"))
 	require.Nil(t, appErr)
 }
+
+func TestBuildPostReplies(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	createPostWithAttachments := func(th *TestHelper, n int, rootID string) *model.Post {
+		var fileIDs []string
+		for i := 0; i < n; i++ {
+			info, err := th.App.Srv().Store.FileInfo().Save(&model.FileInfo{
+				CreatorId: th.BasicUser.Id,
+				Name:      fmt.Sprintf("file%d", i),
+				Path:      fmt.Sprintf("/data/file%d", i),
+			})
+			require.NoError(t, err)
+			fileIDs = append(fileIDs, info.Id)
+		}
+
+		post, err := th.App.CreatePost(th.Context, &model.Post{UserId: th.BasicUser.Id, ChannelId: th.BasicChannel.Id, RootId: rootID, FileIds: fileIDs}, th.BasicChannel, false, true)
+		require.Nil(t, err)
+
+		return post
+	}
+
+	t.Run("basic post", func(t *testing.T) {
+		data, attachments, err := th.App.buildPostReplies(th.BasicPost.Id, true)
+		require.Nil(t, err)
+		require.Empty(t, data)
+		require.Empty(t, attachments)
+	})
+
+	t.Run("root post with attachments and no replies", func(t *testing.T) {
+		post := createPostWithAttachments(th, 5, "")
+		data, attachments, err := th.App.buildPostReplies(post.Id, true)
+		require.Nil(t, err)
+		require.Empty(t, data)
+		require.Empty(t, attachments)
+	})
+
+	t.Run("root post with attachments and a reply", func(t *testing.T) {
+		post := createPostWithAttachments(th, 5, "")
+		createPostWithAttachments(th, 0, post.Id)
+		data, attachments, err := th.App.buildPostReplies(post.Id, true)
+		require.Nil(t, err)
+		require.Len(t, data, 1)
+		require.Empty(t, attachments)
+	})
+
+	t.Run("root post with attachments and multiple replies with attachments", func(t *testing.T) {
+		post := createPostWithAttachments(th, 5, "")
+		reply1 := createPostWithAttachments(th, 2, post.Id)
+		reply2 := createPostWithAttachments(th, 3, post.Id)
+		data, attachments, err := th.App.buildPostReplies(post.Id, true)
+		require.Nil(t, err)
+		require.Len(t, data, 2)
+		require.Len(t, attachments, 5)
+		if reply1.Id < reply2.Id {
+			require.Len(t, *data[0].Attachments, 2)
+			require.Len(t, *data[1].Attachments, 3)
+		} else {
+			require.Len(t, *data[1].Attachments, 2)
+			require.Len(t, *data[0].Attachments, 3)
+		}
+	})
+}

app/imaging/orientation.go

@@ -4,6 +4,7 @@
 package imaging
 
 import (
+	"fmt"
 	"image"
 	"io"
 
@@ -59,17 +60,17 @@ func MakeImageUpright(img image.Image, orientation int) image.Image {
 func GetImageOrientation(input io.Reader) (int, error) {
 	exifData, err := exif.Decode(input)
 	if err != nil {
-		return Upright, err
+		return Upright, fmt.Errorf("failed to decode exif data: %w", err)
 	}
 
 	tag, err := exifData.Get("Orientation")
 	if err != nil {
-		return Upright, err
+		return Upright, fmt.Errorf("failed to get orientation field from exif data: %w", err)
 	}
 
 	orientation, err := tag.Int(0)
 	if err != nil {
-		return Upright, err
+		return Upright, fmt.Errorf("failed to get value from exif tag: %w", err)
 	}
 
 	return orientation, nil

app/import.go

@@ -38,44 +38,74 @@ func stopOnError(err LineImportWorkerError) bool {
 	}
 }
 
-func rewriteAttachmentPaths(files *[]AttachmentImportData, basePath string) {
+func processAttachmentPaths(files *[]AttachmentImportData, basePath string, filesMap map[string]*zip.File) error {
 	if files == nil {
-		return
+		return nil
 	}
-	for _, f := range *files {
+	var ok bool
+	for i, f := range *files {
 		if f.Path != nil {
-			*f.Path = filepath.Join(basePath, *f.Path)
+			path := filepath.Join(basePath, *f.Path)
+			*f.Path = path
+			if len(filesMap) > 0 {
+				if (*files)[i].Data, ok = filesMap[path]; !ok {
+					return fmt.Errorf("attachment %q not found in map", path)
+				}
+			}
 		}
 	}
+
+	return nil
 }
 
-func rewriteFilePaths(line *LineImportData, basePath string) {
+func processAttachments(line *LineImportData, basePath string, filesMap map[string]*zip.File) error {
+	var ok bool
 	switch line.Type {
 	case "post", "direct_post":
 		var replies []ReplyImportData
 		if line.Type == "direct_post" {
-			rewriteAttachmentPaths(line.DirectPost.Attachments, basePath)
+			if err := processAttachmentPaths(line.DirectPost.Attachments, basePath, filesMap); err != nil {
+				return err
+			}
 			if line.DirectPost.Replies != nil {
 				replies = *line.DirectPost.Replies
 			}
 		} else {
-			rewriteAttachmentPaths(line.Post.Attachments, basePath)
+			if err := processAttachmentPaths(line.Post.Attachments, basePath, filesMap); err != nil {
+				return err
+			}
 			if line.Post.Replies != nil {
 				replies = *line.Post.Replies
 			}
 		}
 		for _, reply := range replies {
-			rewriteAttachmentPaths(reply.Attachments, basePath)
+			if err := processAttachmentPaths(reply.Attachments, basePath, filesMap); err != nil {
+				return err
+			}
 		}
 	case "user":
 		if line.User.ProfileImage != nil {
-			*line.User.ProfileImage = filepath.Join(basePath, *line.User.ProfileImage)
+			path := filepath.Join(basePath, *line.User.ProfileImage)
+			*line.User.ProfileImage = path
+			if len(filesMap) > 0 {
+				if line.User.ProfileImageData, ok = filesMap[path]; !ok {
+					return fmt.Errorf("attachment %q not found in map", path)
+				}
+			}
 		}
 	case "emoji":
 		if line.Emoji.Image != nil {
-			*line.Emoji.Image = filepath.Join(basePath, *line.Emoji.Image)
+			path := filepath.Join(basePath, *line.Emoji.Image)
+			*line.Emoji.Image = path
+			if len(filesMap) > 0 {
+				if line.Emoji.Data, ok = filesMap[path]; !ok {
+					return fmt.Errorf("attachment %q not found in map", path)
+				}
+			}
 		}
 	}
+
+	return nil
 }
 
 func (a *App) bulkImportWorker(c *request.Context, dryRun bool, wg *sync.WaitGroup, lines <-chan LineImportWorkerData, errors chan<- LineImportWorkerError) {
@@ -169,18 +199,8 @@ func (a *App) bulkImport(c *request.Context, jsonlReader io.Reader, attachmentsR
 			return model.NewAppError("BulkImport", "app.import.bulk_import.json_decode.error", nil, err.Error(), http.StatusBadRequest), lineNumber
 		}
 
-		if len(attachedFiles) > 0 && line.Post != nil && line.Post.Attachments != nil {
-			for i, attachment := range *line.Post.Attachments {
-				var ok bool
-				path := *attachment.Path
-				if (*line.Post.Attachments)[i].Data, ok = attachedFiles[path]; !ok {
-					return model.NewAppError("BulkImport", "app.import.bulk_import.json_decode.error", nil, fmt.Sprintf("attachment '%s' not found in map", path), http.StatusBadRequest), lineNumber
-				}
-			}
-		}
-
-		if importPath != "" {
-			rewriteFilePaths(&line, importPath)
+		if err := processAttachments(&line, importPath, attachedFiles); err != nil {
+			return model.NewAppError("BulkImport", "app.import.bulk_import.process_attachments.error", nil, err.Error(), http.StatusBadRequest), lineNumber
 		}
 
 		if lineNumber == 1 {

app/import_functions.go

@@ -565,13 +565,22 @@ func (a *App) importUser(data *UserImportData, dryRun bool) *model.AppError {
 	}
 
 	if data.ProfileImage != nil {
-		file, err := os.Open(*data.ProfileImage)
+		var file io.ReadCloser
+		var err error
+		if data.ProfileImageData != nil {
+			file, err = data.ProfileImageData.Open()
+		} else {
+			file, err = os.Open(*data.ProfileImage)
+		}
+
 		if err != nil {
 			mlog.Warn("Unable to open the profile image.", mlog.Err(err))
-		}
-		defer file.Close()
-		if err == nil {
-			if err := a.SetProfileImageFromMultiPartFile(savedUser.Id, file); err != nil {
+		} else {
+			defer file.Close()
+			if limitErr := checkImageLimits(file, *a.Config().FileSettings.MaxImageResolution); limitErr != nil {
+				return model.NewAppError("SetProfileImage", "api.user.upload_profile_user.check_image_limits.app_error", nil, "", http.StatusBadRequest)
+			}
+			if err := a.SetProfileImageFromFile(savedUser.Id, file); err != nil {
 				mlog.Warn("Unable to set the profile image from a file.", mlog.Err(err))
 			}
 		}
@@ -1804,7 +1813,12 @@ func (a *App) importEmoji(data *EmojiImportData, dryRun bool) *model.AppError {
 		emoji.PreSave()
 	}
 
-	file, err := os.Open(*data.Image)
+	var file io.ReadCloser
+	if data.Data != nil {
+		file, err = data.Data.Open()
+	} else {
+		file, err = os.Open(*data.Image)
+	}
 	if err != nil {
 		return model.NewAppError("BulkImport", "app.import.emoji.bad_file.error", map[string]interface{}{"EmojiName": *data.Name}, "", http.StatusBadRequest)
 	}

app/import_test.go

@@ -4,6 +4,8 @@
 package app
 
 import (
+	"archive/zip"
+	"io"
 	"io/ioutil"
 	"net/http"
 	"os"
@@ -278,7 +280,7 @@ func AssertFileIdsInPost(files []*model.FileInfo, th *TestHelper, t *testing.T)
 	}
 }
 
-func TestRewriteFilePaths(t *testing.T) {
+func TestProcessAttachments(t *testing.T) {
 	genAttachments := func() *[]AttachmentImportData {
 		return &[]AttachmentImportData{
 			{
@@ -327,9 +329,11 @@ func TestRewriteFilePaths(t *testing.T) {
 				Path: model.NewString("somedir/file.jpg"),
 			},
 		}
-		rewriteFilePaths(&line, "")
+		err := processAttachments(&line, "", nil)
+		require.NoError(t, err)
 		require.Equal(t, expected, line.Post.Attachments)
-		rewriteFilePaths(&line2, "")
+		err = processAttachments(&line2, "", nil)
+		require.NoError(t, err)
 		require.Equal(t, expected, line2.DirectPost.Attachments)
 	})
 
@@ -344,27 +348,81 @@ func TestRewriteFilePaths(t *testing.T) {
 		}
 
 		t.Run("post attachments", func(t *testing.T) {
-			rewriteFilePaths(&line, "/tmp")
+			err := processAttachments(&line, "/tmp", nil)
+			require.NoError(t, err)
 			require.Equal(t, expected, line.Post.Attachments)
 		})
 
 		t.Run("direct post attachments", func(t *testing.T) {
-			rewriteFilePaths(&line2, "/tmp")
+			err := processAttachments(&line2, "/tmp", nil)
+			require.NoError(t, err)
 			require.Equal(t, expected, line2.DirectPost.Attachments)
 		})
 
 		t.Run("profile image", func(t *testing.T) {
 			expected := "/tmp/profile.jpg"
-			rewriteFilePaths(&userLine, "/tmp")
+			err := processAttachments(&userLine, "/tmp", nil)
+			require.NoError(t, err)
 			require.Equal(t, expected, *userLine.User.ProfileImage)
 		})
 
 		t.Run("emoji", func(t *testing.T) {
 			expected := "/tmp/emoji.png"
-			rewriteFilePaths(&emojiLine, "/tmp")
+			err := processAttachments(&emojiLine, "/tmp", nil)
+			require.NoError(t, err)
 			require.Equal(t, expected, *emojiLine.Emoji.Image)
 		})
 	})
+
+	t.Run("with filesMap", func(t *testing.T) {
+		t.Run("post attachments", func(t *testing.T) {
+			filesMap := map[string]*zip.File{
+				"/tmp/file.jpg": nil,
+			}
+			err := processAttachments(&line, "", filesMap)
+			require.Error(t, err)
+
+			filesMap["/tmp/somedir/file.jpg"] = nil
+			err = processAttachments(&line, "", filesMap)
+			require.NoError(t, err)
+		})
+
+		t.Run("direct post attachments", func(t *testing.T) {
+			filesMap := map[string]*zip.File{
+				"/tmp/file.jpg": nil,
+			}
+			err := processAttachments(&line2, "", filesMap)
+			require.Error(t, err)
+
+			filesMap["/tmp/somedir/file.jpg"] = nil
+			err = processAttachments(&line2, "", filesMap)
+			require.NoError(t, err)
+		})
+
+		t.Run("profile image", func(t *testing.T) {
+			filesMap := map[string]*zip.File{
+				"/tmp/file.jpg": nil,
+			}
+			err := processAttachments(&userLine, "", filesMap)
+			require.Error(t, err)
+
+			filesMap["/tmp/profile.jpg"] = nil
+			err = processAttachments(&userLine, "", filesMap)
+			require.NoError(t, err)
+		})
+
+		t.Run("emoji", func(t *testing.T) {
+			filesMap := map[string]*zip.File{
+				"/tmp/file.jpg": nil,
+			}
+			err := processAttachments(&emojiLine, "", filesMap)
+			require.Error(t, err)
+
+			filesMap["/tmp/emoji.png"] = nil
+			err = processAttachments(&emojiLine, "", filesMap)
+			require.NoError(t, err)
+		})
+	})
 }
 
 func BenchmarkBulkImport(b *testing.B) {
@@ -398,3 +456,45 @@ func BenchmarkBulkImport(b *testing.B) {
 	}
 	b.StopTimer()
 }
+
+func TestImportBulkImportWithAttachments(t *testing.T) {
+	th := Setup(t)
+	defer th.TearDown()
+
+	testsDir, _ := fileutils.FindDir("tests")
+
+	importFile, err := os.Open(testsDir + "/export_test.zip")
+	require.NoError(t, err)
+	defer importFile.Close()
+
+	info, err := importFile.Stat()
+	require.NoError(t, err)
+
+	importZipReader, err := zip.NewReader(importFile, info.Size())
+	require.NoError(t, err)
+	require.NotNil(t, importZipReader)
+
+	var jsonFile io.ReadCloser
+	for _, f := range importZipReader.File {
+		if filepath.Ext(f.Name) != ".jsonl" {
+			continue
+		}
+
+		jsonFile, err = f.Open()
+		require.NoError(t, err)
+		defer jsonFile.Close()
+		break
+	}
+	require.NotNil(t, jsonFile)
+
+	th.App.UpdateConfig(func(cfg *model.Config) { cfg.TeamSettings.MaxUsersPerTeam = model.NewInt(1000) })
+
+	appErr, _ := th.App.BulkImportWithPath(th.Context, jsonFile, importZipReader, false, 1, ExportDataDir)
+	require.Nil(t, appErr)
+
+	adminUser, appErr := th.App.GetUserByUsername("sysadmin")
+	require.Nil(t, appErr)
+
+	files := GetAttachments(adminUser.Id, th, t)
+	require.Len(t, files, 11)
+}

app/import_types.go

@@ -44,22 +44,23 @@ type ChannelImportData struct {
 }
 
 type UserImportData struct {
-	ProfileImage       *string `json:"profile_image,omitempty"`
-	Username           *string `json:"username"`
-	Email              *string `json:"email"`
-	AuthService        *string `json:"auth_service"`
-	AuthData           *string `json:"auth_data,omitempty"`
-	Password           *string `json:"password,omitempty"`
-	Nickname           *string `json:"nickname"`
-	FirstName          *string `json:"first_name"`
-	LastName           *string `json:"last_name"`
-	Position           *string `json:"position"`
-	Roles              *string `json:"roles"`
-	Locale             *string `json:"locale"`
-	UseMarkdownPreview *string `json:"feature_enabled_markdown_preview,omitempty"`
-	UseFormatting      *string `json:"formatting,omitempty"`
-	ShowUnreadSection  *string `json:"show_unread_section,omitempty"`
-	DeleteAt           *int64  `json:"delete_at,omitempty"`
+	ProfileImage       *string   `json:"profile_image,omitempty"`
+	ProfileImageData   *zip.File `json:"-"`
+	Username           *string   `json:"username"`
+	Email              *string   `json:"email"`
+	AuthService        *string   `json:"auth_service"`
+	AuthData           *string   `json:"auth_data,omitempty"`
+	Password           *string   `json:"password,omitempty"`
+	Nickname           *string   `json:"nickname"`
+	FirstName          *string   `json:"first_name"`
+	LastName           *string   `json:"last_name"`
+	Position           *string   `json:"position"`
+	Roles              *string   `json:"roles"`
+	Locale             *string   `json:"locale"`
+	UseMarkdownPreview *string   `json:"feature_enabled_markdown_preview,omitempty"`
+	UseFormatting      *string   `json:"formatting,omitempty"`
+	ShowUnreadSection  *string   `json:"show_unread_section,omitempty"`
+	DeleteAt           *int64    `json:"delete_at,omitempty"`
 
 	Teams *[]UserTeamImportData `json:"teams,omitempty"`
 
@@ -109,8 +110,9 @@ type UserChannelNotifyPropsImportData struct {
 }
 
 type EmojiImportData struct {
-	Name  *string `json:"name"`
-	Image *string `json:"image"`
+	Name  *string   `json:"name"`
+	Image *string   `json:"image"`
+	Data  *zip.File `json:"-"`
 }
 
 type ReactionImportData struct {

i18n/en.json

@@ -4867,6 +4867,10 @@
     "id": "app.import.bulk_import.json_decode.error",
     "translation": "JSON decode of line failed."
   },
+  {
+    "id": "app.import.bulk_import.process_attachments.error",
+    "translation": "Error while processing bulk import attachments."
+  },
   {
     "id": "app.import.bulk_import.unsupported_version.error",
     "translation": "Incorrect or missing version in the data import file. Make sure version is the first object in your import file and try again."

jobs/import_process/worker.go

@@ -162,7 +162,7 @@ func (w *ImportProcessWorker) doJob(job *model.Job) {
 	}
 
 	// do the actual import.
-	appErr, lineNumber := w.app.BulkImport(w.appContext, jsonFile, importZipReader, false, runtime.NumCPU())
+	appErr, lineNumber := w.app.BulkImportWithPath(w.appContext, jsonFile, importZipReader, false, runtime.NumCPU(), app.ExportDataDir)
 	if appErr != nil {
 		job.Data["line_number"] = strconv.Itoa(lineNumber)
 		w.setJobError(job, appErr)