From 162dde5bdddaf3cac2444c2a52b8aebc03a71f23 Mon Sep 17 00:00:00 2001 From: Will Browne Date: Fri, 14 Jul 2023 11:49:05 +0200 Subject: [PATCH] Plugins: Use suffix for plugin directory (#71375) * plugin dir suffix * fix whitespace * fix cli * fix tests * fixup * simplify * undo uninstall changes --- pkg/api/plugin_resource_test.go | 2 +- .../grafana-cli/commands/install_command.go | 37 +++++++++----- pkg/cmd/grafana-cli/services/services.go | 15 +++++- pkg/plugins/manager/fakes/fakes.go | 6 +-- pkg/plugins/manager/installer.go | 30 ++++++----- pkg/plugins/manager/installer_test.go | 8 +-- pkg/plugins/manager/loader/finder/local.go | 6 +-- .../manager/loader/finder/local_test.go | 8 +-- pkg/plugins/manager/loader/loader_test.go | 2 +- .../manager/manager_integration_test.go | 2 +- pkg/plugins/storage/fs.go | 21 +++++--- pkg/plugins/storage/fs_test.go | 51 +++++++++---------- pkg/plugins/storage/ifaces.go | 4 +- 13 files changed, 112 insertions(+), 80 deletions(-) diff --git a/pkg/api/plugin_resource_test.go b/pkg/api/plugin_resource_test.go index b7409befb4a..97c08eaf6e0 100644 --- a/pkg/api/plugin_resource_test.go +++ b/pkg/api/plugin_resource_test.go @@ -70,7 +70,7 @@ func TestCallResource(t *testing.T) { angularInspector, err := angularinspector.NewStaticInspector() require.NoError(t, err) l := loader.ProvideService(pCfg, fakes.NewFakeLicensingService(), signature.NewUnsignedAuthorizer(pCfg), - reg, provider.ProvideService(coreRegistry), finder.NewLocalFinder(pCfg), fakes.NewFakeRoleRegistry(), + reg, provider.ProvideService(coreRegistry), finder.NewLocalFinder(pCfg.DevMode), fakes.NewFakeRoleRegistry(), assetpath.ProvideService(pluginscdn.ProvideService(pCfg)), signature.ProvideService(statickey.New()), angularInspector, &fakes.FakeOauthService{}) srcs := sources.ProvideService(cfg, pCfg) diff --git a/pkg/cmd/grafana-cli/commands/install_command.go b/pkg/cmd/grafana-cli/commands/install_command.go index a17a3769277..92969cc3335 100644 --- a/pkg/cmd/grafana-cli/commands/install_command.go +++ b/pkg/cmd/grafana-cli/commands/install_command.go @@ -5,11 +5,11 @@ import ( "errors" "fmt" "os" - "path/filepath" "runtime" "strings" "github.com/fatih/color" + "github.com/grafana/grafana/pkg/cmd/grafana-cli/logger" "github.com/grafana/grafana/pkg/cmd/grafana-cli/models" "github.com/grafana/grafana/pkg/cmd/grafana-cli/services" @@ -73,6 +73,14 @@ func installCommand(c utils.CommandLine) error { // installPlugin downloads the plugin code as a zip file from the Grafana.com API // and then extracts the zip into the plugin's directory. func installPlugin(ctx context.Context, pluginID, version string, c utils.CommandLine) error { + // If a version is specified, check if it is already installed + if version != "" { + if services.PluginVersionInstalled(pluginID, version, c.PluginDirectory()) { + services.Logger.Successf("Plugin %s v%s already installed.", pluginID, version) + return nil + } + } + repository := repo.NewManager(repo.ManagerCfg{ SkipTLSVerify: c.Bool("insecure"), BaseURL: c.PluginRepoURL(), @@ -95,7 +103,7 @@ func installPlugin(ctx context.Context, pluginID, version string, c utils.Comman } pluginFs := storage.FileSystem(services.Logger, c.PluginDirectory()) - extractedArchive, err := pluginFs.Extract(ctx, pluginID, archive.File) + extractedArchive, err := pluginFs.Extract(ctx, pluginID, storage.SimpleDirNameGeneratorFunc, archive.File) if err != nil { return err } @@ -107,7 +115,7 @@ func installPlugin(ctx context.Context, pluginID, version string, c utils.Comman return fmt.Errorf("%v: %w", fmt.Sprintf("failed to download plugin %s from repository", dep.ID), err) } - _, err = pluginFs.Extract(ctx, dep.ID, d.File) + _, err = pluginFs.Extract(ctx, dep.ID, storage.SimpleDirNameGeneratorFunc, d.File) if err != nil { return err } @@ -117,16 +125,21 @@ func installPlugin(ctx context.Context, pluginID, version string, c utils.Comman // uninstallPlugin removes the plugin directory func uninstallPlugin(_ context.Context, pluginID string, c utils.CommandLine) error { - logger.Infof("Removing plugin: %v\n", pluginID) - - pluginPath := filepath.Join(c.PluginDirectory(), pluginID) - fs := plugins.NewLocalFS(pluginPath) - - logger.Debugf("Removing directory %v\n", pluginPath) - err := fs.Remove() - if err != nil { - return err + for _, bundle := range services.GetLocalPlugins(c.PluginDirectory()) { + if bundle.Primary.JSONData.ID == pluginID { + logger.Infof("Removing plugin: %v\n", pluginID) + if remover, ok := bundle.Primary.FS.(plugins.FSRemover); ok { + logger.Debugf("Removing directory %v\n\n", bundle.Primary.FS.Base()) + if err := remover.Remove(); err != nil { + return err + } + return nil + } else { + return fmt.Errorf("plugin %v is immutable and therefore cannot be uninstalled", pluginID) + } + } } + return nil } diff --git a/pkg/cmd/grafana-cli/services/services.go b/pkg/cmd/grafana-cli/services/services.go index 46f9b4c643f..00fffc487e2 100644 --- a/pkg/cmd/grafana-cli/services/services.go +++ b/pkg/cmd/grafana-cli/services/services.go @@ -13,7 +13,6 @@ import ( "github.com/grafana/grafana/pkg/cmd/grafana-cli/logger" "github.com/grafana/grafana/pkg/cmd/grafana-cli/models" "github.com/grafana/grafana/pkg/plugins" - "github.com/grafana/grafana/pkg/plugins/config" "github.com/grafana/grafana/pkg/plugins/manager/loader/finder" "github.com/grafana/grafana/pkg/plugins/manager/sources" ) @@ -78,7 +77,7 @@ func GetLocalPlugin(pluginDir, pluginID string) (plugins.FoundPlugin, error) { } func GetLocalPlugins(pluginDir string) []*plugins.FoundBundle { - f := finder.NewLocalFinder(&config.Cfg{}) + f := finder.NewLocalFinder(true) res, err := f.Find(context.Background(), sources.NewLocalSource(plugins.ClassExternal, []string{pluginDir})) if err != nil { @@ -88,3 +87,15 @@ func GetLocalPlugins(pluginDir string) []*plugins.FoundBundle { return res } + +func PluginVersionInstalled(pluginID, version, pluginDir string) bool { + for _, bundle := range GetLocalPlugins(pluginDir) { + pJSON := bundle.Primary.JSONData + if pJSON.ID == pluginID { + if pJSON.Info.Version == version { + return true + } + } + } + return false +} diff --git a/pkg/plugins/manager/fakes/fakes.go b/pkg/plugins/manager/fakes/fakes.go index f099542360a..fd326d16f9c 100644 --- a/pkg/plugins/manager/fakes/fakes.go +++ b/pkg/plugins/manager/fakes/fakes.go @@ -233,16 +233,16 @@ func (r *FakePluginRepo) GetPluginArchiveInfo(ctx context.Context, pluginID, ver } type FakePluginStorage struct { - ExtractFunc func(_ context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) + ExtractFunc func(_ context.Context, pluginID string, dirNameFunc storage.DirNameGeneratorFunc, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) } func NewFakePluginStorage() *FakePluginStorage { return &FakePluginStorage{} } -func (s *FakePluginStorage) Extract(ctx context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { +func (s *FakePluginStorage) Extract(ctx context.Context, pluginID string, dirNameFunc storage.DirNameGeneratorFunc, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { if s.ExtractFunc != nil { - return s.ExtractFunc(ctx, pluginID, z) + return s.ExtractFunc(ctx, pluginID, dirNameFunc, z) } return &storage.ExtractedPluginArchive{}, nil } diff --git a/pkg/plugins/manager/installer.go b/pkg/plugins/manager/installer.go index 834beca8088..947a2f4326b 100644 --- a/pkg/plugins/manager/installer.go +++ b/pkg/plugins/manager/installer.go @@ -18,27 +18,29 @@ import ( var _ plugins.Installer = (*PluginInstaller)(nil) type PluginInstaller struct { - pluginRepo repo.Service - pluginStorage storage.ZipExtractor - pluginRegistry registry.Service - pluginLoader loader.Service - log log.Logger + pluginRepo repo.Service + pluginStorage storage.ZipExtractor + pluginStorageDirFunc storage.DirNameGeneratorFunc + pluginRegistry registry.Service + pluginLoader loader.Service + log log.Logger } func ProvideInstaller(cfg *config.Cfg, pluginRegistry registry.Service, pluginLoader loader.Service, pluginRepo repo.Service) *PluginInstaller { return New(pluginRegistry, pluginLoader, pluginRepo, - storage.FileSystem(log.NewPrettyLogger("installer.fs"), cfg.PluginsPath)) + storage.FileSystem(log.NewPrettyLogger("installer.fs"), cfg.PluginsPath), storage.SimpleDirNameGeneratorFunc) } func New(pluginRegistry registry.Service, pluginLoader loader.Service, pluginRepo repo.Service, - pluginStorage storage.ZipExtractor) *PluginInstaller { + pluginStorage storage.ZipExtractor, pluginStorageDirFunc storage.DirNameGeneratorFunc) *PluginInstaller { return &PluginInstaller{ - pluginLoader: pluginLoader, - pluginRegistry: pluginRegistry, - pluginRepo: pluginRepo, - pluginStorage: pluginStorage, - log: log.New("plugin.installer"), + pluginLoader: pluginLoader, + pluginRegistry: pluginRegistry, + pluginRepo: pluginRepo, + pluginStorage: pluginStorage, + pluginStorageDirFunc: pluginStorageDirFunc, + log: log.New("plugin.installer"), } } @@ -102,7 +104,7 @@ func (m *PluginInstaller) Add(ctx context.Context, pluginID, version string, opt } } - extractedArchive, err := m.pluginStorage.Extract(ctx, pluginID, pluginArchive.File) + extractedArchive, err := m.pluginStorage.Extract(ctx, pluginID, m.pluginStorageDirFunc, pluginArchive.File) if err != nil { return err } @@ -116,7 +118,7 @@ func (m *PluginInstaller) Add(ctx context.Context, pluginID, version string, opt return fmt.Errorf("%v: %w", fmt.Sprintf("failed to download plugin %s from repository", dep.ID), err) } - depArchive, err := m.pluginStorage.Extract(ctx, dep.ID, d.File) + depArchive, err := m.pluginStorage.Extract(ctx, dep.ID, m.pluginStorageDirFunc, d.File) if err != nil { return err } diff --git a/pkg/plugins/manager/installer_test.go b/pkg/plugins/manager/installer_test.go index a0a6043d186..a9af72e8985 100644 --- a/pkg/plugins/manager/installer_test.go +++ b/pkg/plugins/manager/installer_test.go @@ -53,7 +53,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { } fs := &fakes.FakePluginStorage{ - ExtractFunc: func(_ context.Context, id string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { + ExtractFunc: func(_ context.Context, id string, _ storage.DirNameGeneratorFunc, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { require.Equal(t, pluginID, id) require.Equal(t, mockZipV1, z) return &storage.ExtractedPluginArchive{ @@ -62,7 +62,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - inst := New(fakes.NewFakePluginRegistry(), loader, pluginRepo, fs) + inst := New(fakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc) err := inst.Add(context.Background(), pluginID, v1, testCompatOpts()) require.NoError(t, err) @@ -108,7 +108,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { File: mockZipV2, }, nil } - fs.ExtractFunc = func(_ context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { + fs.ExtractFunc = func(_ context.Context, pluginID string, _ storage.DirNameGeneratorFunc, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { require.Equal(t, pluginV1.ID, pluginID) require.Equal(t, mockZipV2, z) return &storage.ExtractedPluginArchive{ @@ -168,7 +168,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - pm := New(reg, &fakes.FakeLoader{}, &fakes.FakePluginRepo{}, &fakes.FakePluginStorage{}) + pm := New(reg, &fakes.FakeLoader{}, &fakes.FakePluginRepo{}, &fakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc) err := pm.Add(context.Background(), p.ID, "3.2.0", testCompatOpts()) require.ErrorIs(t, err, plugins.ErrInstallCorePlugin) diff --git a/pkg/plugins/manager/loader/finder/local.go b/pkg/plugins/manager/loader/finder/local.go index 1712d96f773..e09b96e8750 100644 --- a/pkg/plugins/manager/loader/finder/local.go +++ b/pkg/plugins/manager/loader/finder/local.go @@ -28,15 +28,15 @@ type Local struct { production bool } -func NewLocalFinder(cfg *config.Cfg) *Local { +func NewLocalFinder(devMode bool) *Local { return &Local{ - production: !cfg.DevMode, + production: !devMode, log: log.New("local.finder"), } } func ProvideLocalFinder(cfg *config.Cfg) *Local { - return NewLocalFinder(cfg) + return NewLocalFinder(cfg.DevMode) } func (l *Local) Find(ctx context.Context, src plugins.PluginSource) ([]*plugins.FoundBundle, error) { diff --git a/pkg/plugins/manager/loader/finder/local_test.go b/pkg/plugins/manager/loader/finder/local_test.go index e64dfb4434b..907920d24b2 100644 --- a/pkg/plugins/manager/loader/finder/local_test.go +++ b/pkg/plugins/manager/loader/finder/local_test.go @@ -255,7 +255,7 @@ func TestFinder_Find(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - f := NewLocalFinder(pCfg) + f := NewLocalFinder(pCfg.DevMode) pluginBundles, err := f.Find(context.Background(), &fakes.FakePluginSource{ PluginURIsFunc: func(ctx context.Context) []string { return tc.pluginDirs @@ -292,7 +292,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder(pCfg) + finder := NewLocalFinder(pCfg.DevMode) paths, err := finder.getAbsPluginJSONPaths("test") require.NoError(t, err) require.Empty(t, paths) @@ -307,7 +307,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder(pCfg) + finder := NewLocalFinder(pCfg.DevMode) paths, err := finder.getAbsPluginJSONPaths("test") require.NoError(t, err) require.Empty(t, paths) @@ -322,7 +322,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder(pCfg) + finder := NewLocalFinder(pCfg.DevMode) paths, err := finder.getAbsPluginJSONPaths("test") require.Error(t, err) require.Empty(t, paths) diff --git a/pkg/plugins/manager/loader/loader_test.go b/pkg/plugins/manager/loader/loader_test.go index 24e9b8b1cd4..b93eac7daf0 100644 --- a/pkg/plugins/manager/loader/loader_test.go +++ b/pkg/plugins/manager/loader/loader_test.go @@ -1494,7 +1494,7 @@ func newLoader(t *testing.T, cfg *config.Cfg, cbs ...func(loader *Loader)) *Load require.NoError(t, err) l := New(cfg, &fakes.FakeLicensingService{}, signature.NewUnsignedAuthorizer(cfg), fakes.NewFakePluginRegistry(), fakes.NewFakeBackendProcessProvider(), fakes.NewFakeProcessManager(), fakes.NewFakeRoleRegistry(), - assetpath.ProvideService(pluginscdn.ProvideService(cfg)), finder.NewLocalFinder(cfg), + assetpath.ProvideService(pluginscdn.ProvideService(cfg)), finder.NewLocalFinder(cfg.DevMode), signature.ProvideService(statickey.New()), angularInspector, &fakes.FakeOauthService{}) for _, cb := range cbs { diff --git a/pkg/plugins/manager/manager_integration_test.go b/pkg/plugins/manager/manager_integration_test.go index 08fc07a945f..6350640774b 100644 --- a/pkg/plugins/manager/manager_integration_test.go +++ b/pkg/plugins/manager/manager_integration_test.go @@ -121,7 +121,7 @@ func TestIntegrationPluginManager(t *testing.T) { angularInspector, err := angularinspector.NewStaticInspector() require.NoError(t, err) l := loader.ProvideService(pCfg, lic, signature.NewUnsignedAuthorizer(pCfg), - reg, provider.ProvideService(coreRegistry), finder.NewLocalFinder(pCfg), fakes.NewFakeRoleRegistry(), + reg, provider.ProvideService(coreRegistry), finder.NewLocalFinder(pCfg.DevMode), fakes.NewFakeRoleRegistry(), assetpath.ProvideService(pluginscdn.ProvideService(pCfg)), signature.ProvideService(statickey.New()), angularInspector, &fakes.FakeOauthService{}) srcs := sources.ProvideService(cfg, pCfg) diff --git a/pkg/plugins/storage/fs.go b/pkg/plugins/storage/fs.go index 445ce07c3f9..779330239cb 100644 --- a/pkg/plugins/storage/fs.go +++ b/pkg/plugins/storage/fs.go @@ -32,14 +32,18 @@ func FileSystem(logger log.PrettyLogger, pluginsDir string) *FS { } } -func (fs *FS) Extract(ctx context.Context, pluginID string, pluginArchive *zip.ReadCloser) ( +var SimpleDirNameGeneratorFunc = func(pluginID string) string { + return pluginID +} + +func (fs *FS) Extract(ctx context.Context, pluginID string, dirNameFunc DirNameGeneratorFunc, pluginArchive *zip.ReadCloser) ( *ExtractedPluginArchive, error) { - pluginDir, err := fs.extractFiles(ctx, pluginArchive, pluginID) + pluginDir, err := fs.extractFiles(ctx, pluginArchive, pluginID, dirNameFunc) if err != nil { return nil, fmt.Errorf("%v: %w", "failed to extract plugin archive", err) } - pluginJSON, err := readPluginJSON(pluginID, pluginDir) + pluginJSON, err := readPluginJSON(pluginDir) if err != nil { return nil, fmt.Errorf("%v: %w", "failed to convert to plugin DTO", err) } @@ -62,8 +66,9 @@ func (fs *FS) Extract(ctx context.Context, pluginID string, pluginArchive *zip.R }, nil } -func (fs *FS) extractFiles(_ context.Context, pluginArchive *zip.ReadCloser, pluginID string) (string, error) { - installDir := filepath.Join(fs.pluginsDir, pluginID) +func (fs *FS) extractFiles(_ context.Context, pluginArchive *zip.ReadCloser, pluginID string, dirNameFunc DirNameGeneratorFunc) (string, error) { + pluginDirName := dirNameFunc(pluginID) + installDir := filepath.Join(fs.pluginsDir, pluginDirName) if _, err := os.Stat(installDir); !os.IsNotExist(err) { fs.log.Debugf("Removing existing installation of plugin %s", installDir) err = os.RemoveAll(installDir) @@ -92,7 +97,7 @@ func (fs *FS) extractFiles(_ context.Context, pluginArchive *zip.ReadCloser, plu zf.Name, fs.pluginsDir) } - dstPath := filepath.Clean(filepath.Join(fs.pluginsDir, removeGitBuildFromName(zf.Name, pluginID))) // lgtm[go/zipslip] + dstPath := filepath.Clean(filepath.Join(fs.pluginsDir, removeGitBuildFromName(zf.Name, pluginDirName))) // lgtm[go/zipslip] if zf.FileInfo().IsDir() { // We can ignore gosec G304 here since it makes sense to give all users read access @@ -220,7 +225,7 @@ func removeGitBuildFromName(filename, pluginID string) string { return reGitBuild.ReplaceAllString(filename, pluginID+"/") } -func readPluginJSON(pluginID, pluginDir string) (plugins.JSONData, error) { +func readPluginJSON(pluginDir string) (plugins.JSONData, error) { pluginPath := filepath.Join(pluginDir, "plugin.json") // It's safe to ignore gosec warning G304 since the file path suffix is hardcoded @@ -232,7 +237,7 @@ func readPluginJSON(pluginID, pluginDir string) (plugins.JSONData, error) { // nolint:gosec data, err = os.ReadFile(pluginPath) if err != nil { - return plugins.JSONData{}, fmt.Errorf("could not find plugin.json or dist/plugin.json for %s in %s", pluginID, pluginDir) + return plugins.JSONData{}, fmt.Errorf("could not find plugin.json or dist/plugin.json for in %s", pluginDir) } } diff --git a/pkg/plugins/storage/fs_test.go b/pkg/plugins/storage/fs_test.go index 900069f5a5c..fa504248dd1 100644 --- a/pkg/plugins/storage/fs_test.go +++ b/pkg/plugins/storage/fs_test.go @@ -27,25 +27,24 @@ func TestAdd(t *testing.T) { pluginID := "test-app" fs := FileSystem(log.NewTestPrettyLogger(), testDir) - archive, err := fs.Extract(context.Background(), pluginID, zipFile(t, "./testdata/plugin-with-symlinks.zip")) + archive, err := fs.Extract(context.Background(), pluginID, SimpleDirNameGeneratorFunc, zipFile(t, "./testdata/plugin-with-symlinks.zip")) require.NotNil(t, archive) require.NoError(t, err) // verify extracted contents - files, err := os.ReadDir(filepath.Join(testDir, pluginID)) + files, err := os.ReadDir(archive.Path) require.NoError(t, err) - file2, err := files[2].Info() - require.NoError(t, err) - file4, err := files[4].Info() - require.NoError(t, err) - require.Len(t, files, 6) require.Equal(t, files[0].Name(), "MANIFEST.txt") require.Equal(t, files[1].Name(), "dashboards") require.Equal(t, files[2].Name(), "extra") + file2, err := files[2].Info() + require.NoError(t, err) require.Equal(t, os.ModeSymlink, file2.Mode()&os.ModeSymlink) require.Equal(t, files[3].Name(), "plugin.json") require.Equal(t, files[4].Name(), "symlink_to_txt") + file4, err := files[4].Info() + require.NoError(t, err) require.Equal(t, os.ModeSymlink, file4.Mode()&os.ModeSymlink) require.Equal(t, files[5].Name(), "text.txt") } @@ -59,27 +58,27 @@ func TestExtractFiles(t *testing.T) { skipWindows(t) pluginID := "grafana-simple-json-datasource" - path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/grafana-simple-json-datasource-ec18fa4da8096a952608a7e4c7782b4260b41bcf.zip"), pluginID) + path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/grafana-simple-json-datasource-ec18fa4da8096a952608a7e4c7782b4260b41bcf.zip"), pluginID, SimpleDirNameGeneratorFunc) require.Equal(t, filepath.Join(pluginsDir, pluginID), path) require.NoError(t, err) // File in zip has permissions 755 - fileInfo, err := os.Stat(filepath.Join(pluginsDir, "grafana-simple-json-datasource", "simple-plugin_darwin_amd64")) + fileInfo, err := os.Stat(filepath.Join(path, "simple-plugin_darwin_amd64")) require.NoError(t, err) require.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String()) // File in zip has permission 755 - fileInfo, err = os.Stat(pluginsDir + "/grafana-simple-json-datasource/simple-plugin_linux_amd64") + fileInfo, err = os.Stat(filepath.Join(path, "simple-plugin_linux_amd64")) require.NoError(t, err) require.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String()) // File in zip has permission 644 - fileInfo, err = os.Stat(pluginsDir + "/grafana-simple-json-datasource/simple-plugin_windows_amd64.exe") + fileInfo, err = os.Stat(filepath.Join(path, "simple-plugin_windows_amd64.exe")) require.NoError(t, err) require.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String()) // File in zip has permission 755 - fileInfo, err = os.Stat(pluginsDir + "/grafana-simple-json-datasource/non-plugin-binary") + fileInfo, err = os.Stat(filepath.Join(path, "non-plugin-binary")) require.NoError(t, err) require.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String()) }) @@ -88,43 +87,43 @@ func TestExtractFiles(t *testing.T) { skipWindows(t) pluginID := "plugin-with-symlink" - path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-symlink.zip"), pluginID) + path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-symlink.zip"), pluginID, SimpleDirNameGeneratorFunc) require.Equal(t, filepath.Join(pluginsDir, pluginID), path) require.NoError(t, err) - _, err = os.Stat(pluginsDir + "/plugin-with-symlink/symlink_to_txt") + _, err = os.Stat(filepath.Join(path, "symlink_to_txt")) require.NoError(t, err) - target, err := filepath.EvalSymlinks(pluginsDir + "/plugin-with-symlink/symlink_to_txt") + target, err := filepath.EvalSymlinks(filepath.Join(path, "symlink_to_txt")) require.NoError(t, err) - require.Equal(t, pluginsDir+"/plugin-with-symlink/text.txt", target) + require.Equal(t, filepath.Join(path, "text.txt"), target) }) t.Run("Should extract directory with relative symlink", func(t *testing.T) { skipWindows(t) pluginID := "plugin-with-symlink-dir" - path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-symlink-dir.zip"), pluginID) + path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-symlink-dir.zip"), pluginID, SimpleDirNameGeneratorFunc) require.Equal(t, filepath.Join(pluginsDir, pluginID), path) require.NoError(t, err) - _, err = os.Stat(pluginsDir + "/plugin-with-symlink-dir/symlink_to_dir") + _, err = os.Stat(filepath.Join(path, "symlink_to_dir")) require.NoError(t, err) - target, err := filepath.EvalSymlinks(pluginsDir + "/plugin-with-symlink-dir/symlink_to_dir") + target, err := filepath.EvalSymlinks(filepath.Join(path, "symlink_to_dir")) require.NoError(t, err) - require.Equal(t, pluginsDir+"/plugin-with-symlink-dir/dir", target) + require.Equal(t, filepath.Join(path, "dir"), target) }) t.Run("Should not extract file with absolute symlink", func(t *testing.T) { skipWindows(t) pluginID := "plugin-with-absolute-symlink" - path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-absolute-symlink.zip"), pluginID) + path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-absolute-symlink.zip"), pluginID, SimpleDirNameGeneratorFunc) require.Equal(t, filepath.Join(pluginsDir, pluginID), path) require.NoError(t, err) - _, err = os.Stat(pluginsDir + "/plugin-with-absolute-symlink/test.txt") + _, err = os.Stat(filepath.Join(path, "/test.txt")) require.True(t, os.IsNotExist(err)) }) @@ -132,16 +131,16 @@ func TestExtractFiles(t *testing.T) { skipWindows(t) pluginID := "plugin-with-absolute-symlink-dir" - path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-absolute-symlink-dir.zip"), pluginID) + path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-absolute-symlink-dir.zip"), pluginID, SimpleDirNameGeneratorFunc) require.Equal(t, filepath.Join(pluginsDir, pluginID), path) require.NoError(t, err) - _, err = os.Stat(pluginsDir + "/plugin-with-absolute-symlink-dir/target") + _, err = os.Stat(filepath.Join(path, "/plugin-with-absolute-symlink-dir/target")) require.True(t, os.IsNotExist(err)) }) t.Run("Should detect if archive members point outside of the destination directory", func(t *testing.T) { - path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-parent-member.zip"), "plugin-with-parent-member") + path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-parent-member.zip"), "plugin-with-parent-member", SimpleDirNameGeneratorFunc) require.Empty(t, path) require.EqualError(t, err, fmt.Sprintf( `archive member "../member.txt" tries to write outside of plugin directory: %q, this can be a security risk`, @@ -150,7 +149,7 @@ func TestExtractFiles(t *testing.T) { }) t.Run("Should detect if archive members are absolute", func(t *testing.T) { - path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-absolute-member.zip"), "plugin-with-absolute-member") + path, err := i.extractFiles(context.Background(), zipFile(t, "testdata/plugin-with-absolute-member.zip"), "plugin-with-absolute-member", SimpleDirNameGeneratorFunc) require.Empty(t, path) require.EqualError(t, err, fmt.Sprintf( `archive member "/member.txt" tries to write outside of plugin directory: %q, this can be a security risk`, diff --git a/pkg/plugins/storage/ifaces.go b/pkg/plugins/storage/ifaces.go index 027827cf89d..eb2f2bde8c7 100644 --- a/pkg/plugins/storage/ifaces.go +++ b/pkg/plugins/storage/ifaces.go @@ -6,5 +6,7 @@ import ( ) type ZipExtractor interface { - Extract(ctx context.Context, pluginID string, rc *zip.ReadCloser) (*ExtractedPluginArchive, error) + Extract(ctx context.Context, pluginID string, destDir DirNameGeneratorFunc, rc *zip.ReadCloser) (*ExtractedPluginArchive, error) } + +type DirNameGeneratorFunc = func(pluginID string) string