From 4bf3d6a7f565e1de0deb7bc1f10982117c9b6eda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillermo=20Vay=C3=A1?= Date: Tue, 15 Mar 2022 12:57:29 +0100 Subject: [PATCH] [MM-40179] Go into directories when trying to export attachments (#19249) * MM-40179 traverse directories when trying to export * fix test * fix s3 to behabe the same way * add test for paths way too deep * test recursion only on localstorage * make list directories non recursively the default approach * fix linting error * fix non-recursive case Co-authored-by: = <=> Co-authored-by: Mattermod --- app/app_iface.go | 1 + app/file.go | 19 +++++-- app/opentracing/opentracing_layer.go | 22 ++++++++ app/plugin.go | 2 +- shared/filestore/filesstore.go | 1 + shared/filestore/filesstore_test.go | 74 +++++++++++++++++++++++++-- shared/filestore/localstore.go | 45 +++++++++++++--- shared/filestore/mocks/FileBackend.go | 23 +++++++++ shared/filestore/s3store.go | 13 ++++- 9 files changed, 183 insertions(+), 17 deletions(-) diff --git a/app/app_iface.go b/app/app_iface.go index 80bc7158ac..5ada83a7dc 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -840,6 +840,7 @@ type AppIface interface { LimitedClientConfig() map[string]string ListAllCommands(teamID string, T i18n.TranslateFunc) ([]*model.Command, *model.AppError) ListDirectory(path string) ([]string, *model.AppError) + ListDirectoryRecursively(path string) ([]string, *model.AppError) ListExports() ([]string, *model.AppError) ListImports() ([]string, *model.AppError) ListPluginKeys(pluginID string, page, perPage int) ([]string, *model.AppError) diff --git a/app/file.go b/app/file.go index 15bcd5e4ad..ccf4863575 100644 --- a/app/file.go +++ b/app/file.go @@ -179,11 +179,24 @@ func (s *Server) removeFile(path string) *model.AppError { } func (a *App) ListDirectory(path string) ([]string, *model.AppError) { - return a.Srv().listDirectory(path) + return a.Srv().listDirectory(path, false) } -func (s *Server) listDirectory(path string) ([]string, *model.AppError) { - paths, nErr := s.FileBackend().ListDirectory(path) +func (a *App) ListDirectoryRecursively(path string) ([]string, *model.AppError) { + return a.Srv().listDirectory(path, true) +} + +func (s *Server) listDirectory(path string, recursion bool) ([]string, *model.AppError) { + backend := s.FileBackend() + var paths []string + var nErr error + + if recursion { + paths, nErr = backend.ListDirectoryRecursively(path) + } else { + paths, nErr = backend.ListDirectory(path) + } + if nErr != nil { return nil, model.NewAppError("ListDirectory", "api.file.list_directory.app_error", nil, nErr.Error(), http.StatusInternalServerError) } diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 4e3d0f4c3e..13c679dcb1 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -11395,6 +11395,28 @@ func (a *OpenTracingAppLayer) ListDirectory(path string) ([]string, *model.AppEr return resultVar0, resultVar1 } +func (a *OpenTracingAppLayer) ListDirectoryRecursively(path string) ([]string, *model.AppError) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.ListDirectoryRecursively") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.ListDirectoryRecursively(path) + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (a *OpenTracingAppLayer) ListExports() ([]string, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.ListExports") diff --git a/app/plugin.go b/app/plugin.go index 8ad6e81bd6..579c83497b 100644 --- a/app/plugin.go +++ b/app/plugin.go @@ -823,7 +823,7 @@ func (ch *Channels) notifyPluginEnabled(manifest *model.Manifest) error { } func (ch *Channels) getPluginsFromFolder() (map[string]*pluginSignaturePath, *model.AppError) { - fileStorePaths, appErr := ch.srv.listDirectory(fileStorePluginFolder) + fileStorePaths, appErr := ch.srv.listDirectory(fileStorePluginFolder, false) if appErr != nil { return nil, model.NewAppError("getPluginsFromDir", "app.plugin.sync.list_filestore.app_error", nil, appErr.Error(), http.StatusInternalServerError) } diff --git a/shared/filestore/filesstore.go b/shared/filestore/filesstore.go index ef02895d55..c17ea1f62d 100644 --- a/shared/filestore/filesstore.go +++ b/shared/filestore/filesstore.go @@ -35,6 +35,7 @@ type FileBackend interface { FileModTime(path string) (time.Time, error) ListDirectory(path string) ([]string, error) + ListDirectoryRecursively(path string) ([]string, error) RemoveDirectory(path string) error } diff --git a/shared/filestore/filesstore_test.go b/shared/filestore/filesstore_test.go index da909c93d6..76823e398d 100644 --- a/shared/filestore/filesstore_test.go +++ b/shared/filestore/filesstore_test.go @@ -270,14 +270,19 @@ func (s *FileBackendTestSuite) TestListDirectory() { s.Len(paths, 1) s.Equal(path1, (paths)[0]) - paths, err = s.backend.ListDirectory("19700101/") + paths, err = s.backend.ListDirectory("19800101/") s.Nil(err) s.Len(paths, 1) - s.Equal(path1, (paths)[0]) + s.Equal(path2, (paths)[0]) + + if s.settings.DriverName == driverLocal { + paths, err = s.backend.ListDirectory("19800102") + s.Nil(err) + s.Len(paths, 0) + } paths, err = s.backend.ListDirectory("") s.Nil(err) - found1 := false found2 := false for _, path := range paths { @@ -294,6 +299,69 @@ func (s *FileBackendTestSuite) TestListDirectory() { s.backend.RemoveFile(path2) } +func (s *FileBackendTestSuite) TestListDirectoryRecursively() { + b := []byte("test") + path1 := "19700101/" + randomString() + path2 := "19800101/" + randomString() + longPath := "19800102/this/is/a/way/too/long/path/for/this/function/to/handle" + randomString() + + paths, err := s.backend.ListDirectoryRecursively("19700101") + s.Nil(err) + s.Len(paths, 0) + + written, err := s.backend.WriteFile(bytes.NewReader(b), path1) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + + written, err = s.backend.WriteFile(bytes.NewReader(b), path2) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + + written, err = s.backend.WriteFile(bytes.NewReader(b), longPath) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + + paths, err = s.backend.ListDirectoryRecursively("19700101") + s.Nil(err) + s.Len(paths, 1) + s.Equal(path1, (paths)[0]) + + paths, err = s.backend.ListDirectoryRecursively("19800101/") + s.Nil(err) + s.Len(paths, 1) + s.Equal(path2, (paths)[0]) + + if s.settings.DriverName == driverLocal { + paths, err = s.backend.ListDirectory("19800102") + s.Nil(err) + s.Len(paths, 1) + } + + paths, err = s.backend.ListDirectoryRecursively("") + s.Nil(err) + found1 := false + found2 := false + found3 := false + for _, path := range paths { + if path == path1 { + found1 = true + } else if path == path2 { + found2 = true + } else if path == longPath { + found3 = true + } + } + s.True(found1) + s.True(found2) + if s.settings.DriverName == driverLocal { + s.False(found3) + } + + s.backend.RemoveFile(path1) + s.backend.RemoveFile(path2) + s.backend.RemoveFile(longPath) +} + func (s *FileBackendTestSuite) TestRemoveDirectory() { b := []byte("test") diff --git a/shared/filestore/localstore.go b/shared/filestore/localstore.go index 5ed882de25..e2d53e4942 100644 --- a/shared/filestore/localstore.go +++ b/shared/filestore/localstore.go @@ -188,19 +188,48 @@ func (b *LocalFileBackend) RemoveFile(path string) error { return nil } -func (b *LocalFileBackend) ListDirectory(path string) ([]string, error) { - var paths []string - fileInfos, err := ioutil.ReadDir(filepath.Join(b.directory, path)) +// basePath: path to get to the file but won't be added to the end result +// path: basePath+path current directory we are looking at +// maxDepth: parameter to prevent infinite recursion, once this is reached we won't look any further +func appendRecursively(basePath, path string, maxDepth int) ([]string, error) { + results := []string{} + dirEntries, err := os.ReadDir(filepath.Join(basePath, path)) if err != nil { if os.IsNotExist(err) { - return paths, nil + return results, nil } - return nil, errors.Wrapf(err, "unable to list the directory %s", path) + return results, errors.Wrapf(err, "unable to list the directory %s", path) } - for _, fileInfo := range fileInfos { - paths = append(paths, filepath.Join(path, fileInfo.Name())) + for _, dirEntry := range dirEntries { + entryName := dirEntry.Name() + entryPath := filepath.Join(path, entryName) + if entryName == "." || entryName == ".." || entryPath == path { + continue + } + if dirEntry.IsDir() { + if maxDepth <= 0 { + mlog.Warn("Max Depth reached", mlog.String("path", entryPath)) + results = append(results, entryPath) + continue // we'll ignore it if max depth is reached. + } + nestedResults, err := appendRecursively(basePath, entryPath, maxDepth-1) + if err != nil { + return results, err + } + results = append(results, nestedResults...) + } else { + results = append(results, entryPath) + } } - return paths, nil + return results, nil +} + +func (b *LocalFileBackend) ListDirectory(path string) ([]string, error) { + return appendRecursively(b.directory, path, 0) +} + +func (b *LocalFileBackend) ListDirectoryRecursively(path string) ([]string, error) { + return appendRecursively(b.directory, path, 10) } func (b *LocalFileBackend) RemoveDirectory(path string) error { diff --git a/shared/filestore/mocks/FileBackend.go b/shared/filestore/mocks/FileBackend.go index 2bc0c7a700..51c45d82e7 100644 --- a/shared/filestore/mocks/FileBackend.go +++ b/shared/filestore/mocks/FileBackend.go @@ -140,6 +140,29 @@ func (_m *FileBackend) ListDirectory(path string) ([]string, error) { return r0, r1 } +// ListDirectoryRecursively provides a mock function with given fields: path +func (_m *FileBackend) ListDirectoryRecursively(path string) ([]string, error) { + ret := _m.Called(path) + + var r0 []string + if rf, ok := ret.Get(0).(func(string) []string); ok { + r0 = rf(path) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]string) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(path) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // MoveFile provides a mock function with given fields: oldPath, newPath func (_m *FileBackend) MoveFile(oldPath string, newPath string) error { ret := _m.Called(oldPath, newPath) diff --git a/shared/filestore/s3store.go b/shared/filestore/s3store.go index c45f626ad5..5ab6f1ce3e 100644 --- a/shared/filestore/s3store.go +++ b/shared/filestore/s3store.go @@ -396,7 +396,7 @@ func getPathsFromObjectInfos(in <-chan s3.ObjectInfo) <-chan s3.ObjectInfo { return out } -func (b *S3FileBackend) ListDirectory(path string) ([]string, error) { +func (b *S3FileBackend) listDirectory(path string, recursion bool) ([]string, error) { path = filepath.Join(b.pathPrefix, path) if !strings.HasSuffix(path, "/") && path != "" { // s3Clnt returns only the path itself when "/" is not present @@ -405,7 +405,8 @@ func (b *S3FileBackend) ListDirectory(path string) ([]string, error) { } opts := s3.ListObjectsOptions{ - Prefix: path, + Prefix: path, + Recursive: recursion, } var paths []string for object := range b.client.ListObjects(context.Background(), b.bucket, opts) { @@ -424,6 +425,14 @@ func (b *S3FileBackend) ListDirectory(path string) ([]string, error) { return paths, nil } +func (b *S3FileBackend) ListDirectory(path string) ([]string, error) { + return b.listDirectory(path, false) +} + +func (b *S3FileBackend) ListDirectoryRecursively(path string) ([]string, error) { + return b.listDirectory(path, true) +} + func (b *S3FileBackend) RemoveDirectory(path string) error { opts := s3.ListObjectsOptions{ Prefix: filepath.Join(b.pathPrefix, path),