diff --git a/pkg/api/plugins.go b/pkg/api/plugins.go index c628009d458..7c424ad6bf3 100644 --- a/pkg/api/plugins.go +++ b/pkg/api/plugins.go @@ -22,7 +22,6 @@ import ( "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/backendplugin" "github.com/grafana/grafana/pkg/plugins/repo" - "github.com/grafana/grafana/pkg/plugins/storage" ac "github.com/grafana/grafana/pkg/services/accesscontrol" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/datasources" @@ -491,10 +490,6 @@ func (hs *HTTPServer) UninstallPlugin(c *contextmodel.ReqContext) response.Respo if errors.Is(err, plugins.ErrUninstallCorePlugin) { return response.Error(http.StatusForbidden, "Cannot uninstall a Core plugin", err) } - if errors.Is(err, storage.ErrUninstallOutsideOfPluginDir) { - return response.Error(http.StatusForbidden, "Cannot uninstall a plugin outside of the plugins directory", err) - } - return response.Error(http.StatusInternalServerError, "Failed to uninstall plugin", err) } return response.JSON(http.StatusOK, []byte{}) diff --git a/pkg/cmd/grafana-cli/commands/install_command.go b/pkg/cmd/grafana-cli/commands/install_command.go index be8b7b75499..35a8cd2a0ec 100644 --- a/pkg/cmd/grafana-cli/commands/install_command.go +++ b/pkg/cmd/grafana-cli/commands/install_command.go @@ -84,7 +84,7 @@ func installPlugin(ctx context.Context, pluginID, version string, c utils.Comman } pluginFs := storage.FileSystem(services.Logger, c.PluginDirectory()) - extractedArchive, err := pluginFs.Add(ctx, pluginID, archive.File) + extractedArchive, err := pluginFs.Extract(ctx, pluginID, archive.File) if err != nil { return err } @@ -96,7 +96,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.Add(ctx, dep.ID, d.File) + _, err = pluginFs.Extract(ctx, dep.ID, d.File) if err != nil { return err } diff --git a/pkg/plugins/ifaces.go b/pkg/plugins/ifaces.go index 6b0d6bc3499..9a523686310 100644 --- a/pkg/plugins/ifaces.go +++ b/pkg/plugins/ifaces.go @@ -58,6 +58,10 @@ type FS interface { Files() []string } +type FSRemover interface { + Remove() error +} + type FoundBundle struct { Primary FoundPlugin Children []*FoundPlugin diff --git a/pkg/plugins/localfiles.go b/pkg/plugins/localfiles.go index 92708796a90..cb0b442f8d1 100644 --- a/pkg/plugins/localfiles.go +++ b/pkg/plugins/localfiles.go @@ -82,6 +82,17 @@ func (f LocalFS) Files() []string { return files } +func (f LocalFS) Remove() error { + // extra security check to ensure we only remove a directory that looks like a plugin + if _, err := os.Stat(filepath.Join(f.basePath, "plugin.json")); os.IsNotExist(err) { + if _, err = os.Stat(filepath.Join(f.basePath, "dist/plugin.json")); os.IsNotExist(err) { + return ErrUninstallInvalidPluginDir + } + } + + return os.RemoveAll(f.basePath) +} + var _ fs.File = (*LocalFile)(nil) // LocalFile implements a fs.File for accessing the local filesystem. diff --git a/pkg/plugins/localfiles_test.go b/pkg/plugins/localfiles_test.go index ed66190c048..86ddc6d9ec8 100644 --- a/pkg/plugins/localfiles_test.go +++ b/pkg/plugins/localfiles_test.go @@ -3,44 +3,97 @@ package plugins import ( "io" "os" + "path/filepath" "testing" "github.com/stretchr/testify/require" ) -type tempFileScenario struct { - filePath string -} +func TestLocalFS_Remove(t *testing.T) { + pluginDir := t.TempDir() + pluginJSON := filepath.Join(pluginDir, "plugin.json") + //nolint:gosec + f, err := os.Create(pluginJSON) + require.NoError(t, err) + err = f.Close() + require.NoError(t, err) -func (s tempFileScenario) cleanup() error { - return os.Remove(s.filePath) -} + fs := NewLocalFS( + map[string]struct{}{ + "plugin.json": {}, + }, + pluginDir, + ) -func (s tempFileScenario) newLocalFile() LocalFile { - return LocalFile{path: s.filePath} -} + err = fs.Remove() + require.NoError(t, err) -func newTempFileScenario() (tempFileScenario, error) { - tf, err := os.CreateTemp(os.TempDir(), "*") - if err != nil { - return tempFileScenario{}, err - } - defer tf.Close() //nolint - if _, err := tf.Write([]byte("hello\n")); err != nil { - return tempFileScenario{}, err - } - return tempFileScenario{ - filePath: tf.Name(), - }, nil -} + _, err = os.Stat(pluginDir) + require.Error(t, err) + require.True(t, os.IsNotExist(err)) -func newTempFileScenarioForTest(t *testing.T) tempFileScenario { - s, err := newTempFileScenario() - require.NoError(t, err) - t.Cleanup(func() { - require.NoError(t, s.cleanup()) + _, err = os.Stat(pluginJSON) + require.Error(t, err) + require.True(t, os.IsNotExist(err)) + + t.Run("Uninstall will search in nested dist folder for plugin.json", func(t *testing.T) { + pluginDistDir := filepath.Join(t.TempDir(), "dist") + err = os.Mkdir(pluginDistDir, os.ModePerm) + require.NoError(t, err) + pluginJSON = filepath.Join(pluginDistDir, "plugin.json") + //nolint:gosec + f, err = os.Create(pluginJSON) + require.NoError(t, err) + err = f.Close() + require.NoError(t, err) + + pluginDir = filepath.Dir(pluginDistDir) + + fs = NewLocalFS( + map[string]struct{}{ + "dist/plugin.json": {}, + }, + pluginDir, + ) + + err = fs.Remove() + require.NoError(t, err) + + _, err = os.Stat(pluginDir) + require.True(t, os.IsNotExist(err)) + + _, err = os.Stat(pluginJSON) + require.Error(t, err) + require.True(t, os.IsNotExist(err)) + }) + + t.Run("Uninstall will not delete folder if cannot recognize plugin structure", func(t *testing.T) { + pluginDir = filepath.Join(t.TempDir(), "system32") + err = os.Mkdir(pluginDir, os.ModePerm) + require.NoError(t, err) + testFile := filepath.Join(pluginDir, "important.exe") + //nolint:gosec + f, err = os.Create(testFile) + require.NoError(t, err) + err = f.Close() + require.NoError(t, err) + + fs = NewLocalFS( + map[string]struct{}{ + "system32/important.exe": {}, + }, + pluginDir, + ) + + err = fs.Remove() + require.ErrorIs(t, err, ErrUninstallInvalidPluginDir) + + _, err = os.Stat(pluginDir) + require.NoError(t, err) + + _, err = os.Stat(testFile) + require.NoError(t, err) }) - return s } func TestLocalFile_Read(t *testing.T) { @@ -141,3 +194,31 @@ func TestLocalFile_Close(t *testing.T) { require.Error(t, f.Close()) }) } + +type tempFileScenario struct { + filePath string +} + +func (s tempFileScenario) newLocalFile() LocalFile { + return LocalFile{path: s.filePath} +} + +func newTempFileScenario(t *testing.T) (tempFileScenario, error) { + tf, err := os.CreateTemp(t.TempDir(), "*") + if err != nil { + return tempFileScenario{}, err + } + defer tf.Close() //nolint + if _, err := tf.Write([]byte("hello\n")); err != nil { + return tempFileScenario{}, err + } + return tempFileScenario{ + filePath: tf.Name(), + }, nil +} + +func newTempFileScenarioForTest(t *testing.T) tempFileScenario { + s, err := newTempFileScenario(t) + require.NoError(t, err) + return s +} diff --git a/pkg/plugins/manager/fakes/fakes.go b/pkg/plugins/manager/fakes/fakes.go index b94a40c7e78..51cea698f6a 100644 --- a/pkg/plugins/manager/fakes/fakes.go +++ b/pkg/plugins/manager/fakes/fakes.go @@ -232,42 +232,20 @@ func (r *FakePluginRepo) GetPluginDownloadOptions(ctx context.Context, pluginID, } type FakePluginStorage struct { - Store map[string]struct{} - AddFunc func(_ context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) - RegisterFunc func(_ context.Context, pluginID, pluginDir string) error - RemoveFunc func(_ context.Context, pluginID string) error + ExtractFunc func(_ context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) } func NewFakePluginStorage() *FakePluginStorage { - return &FakePluginStorage{ - Store: map[string]struct{}{}, - } + return &FakePluginStorage{} } -func (s *FakePluginStorage) Register(ctx context.Context, pluginID, pluginDir string) error { - s.Store[pluginID] = struct{}{} - if s.RegisterFunc != nil { - return s.RegisterFunc(ctx, pluginID, pluginDir) - } - return nil -} - -func (s *FakePluginStorage) Add(ctx context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { - s.Store[pluginID] = struct{}{} - if s.AddFunc != nil { - return s.AddFunc(ctx, pluginID, z) +func (s *FakePluginStorage) Extract(ctx context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { + if s.ExtractFunc != nil { + return s.ExtractFunc(ctx, pluginID, z) } return &storage.ExtractedPluginArchive{}, nil } -func (s *FakePluginStorage) Remove(ctx context.Context, pluginID string) error { - delete(s.Store, pluginID) - if s.RemoveFunc != nil { - return s.RemoveFunc(ctx, pluginID) - } - return nil -} - type FakeProcessManager struct { StartFunc func(_ context.Context, pluginID string) error StopFunc func(_ context.Context, pluginID string) error @@ -363,7 +341,8 @@ func (f *FakeRoleRegistry) DeclarePluginRoles(_ context.Context, _ string, _ str } type FakePluginFiles struct { - OpenFunc func(name string) (fs.File, error) + OpenFunc func(name string) (fs.File, error) + RemoveFunc func() error base string } @@ -389,6 +368,13 @@ func (f *FakePluginFiles) Files() []string { return []string{} } +func (f *FakePluginFiles) Remove() error { + if f.RemoveFunc != nil { + return f.RemoveFunc() + } + return nil +} + type FakeSourceRegistry struct { ListFunc func(_ context.Context) []plugins.PluginSource } diff --git a/pkg/plugins/manager/installer.go b/pkg/plugins/manager/installer.go index d0bc465f175..2ecce8a18ae 100644 --- a/pkg/plugins/manager/installer.go +++ b/pkg/plugins/manager/installer.go @@ -18,7 +18,7 @@ var _ plugins.Installer = (*PluginInstaller)(nil) type PluginInstaller struct { pluginRepo repo.Service - pluginStorage storage.Manager + pluginStorage storage.ZipExtractor pluginRegistry registry.Service pluginLoader loader.Service log log.Logger @@ -30,7 +30,7 @@ func ProvideInstaller(cfg *config.Cfg, pluginRegistry registry.Service, pluginLo } func New(pluginRegistry registry.Service, pluginLoader loader.Service, pluginRepo repo.Service, - pluginStorage storage.Manager) *PluginInstaller { + pluginStorage storage.ZipExtractor) *PluginInstaller { return &PluginInstaller{ pluginLoader: pluginLoader, pluginRegistry: pluginRegistry, @@ -45,7 +45,7 @@ func (m *PluginInstaller) Add(ctx context.Context, pluginID, version string, opt var pluginArchive *repo.PluginArchive if plugin, exists := m.plugin(ctx, pluginID); exists { - if !plugin.IsExternalPlugin() { + if plugin.IsCorePlugin() || plugin.IsBundledPlugin() { return plugins.ErrInstallCorePlugin } @@ -97,7 +97,7 @@ func (m *PluginInstaller) Add(ctx context.Context, pluginID, version string, opt } } - extractedArchive, err := m.pluginStorage.Add(ctx, pluginID, pluginArchive.File) + extractedArchive, err := m.pluginStorage.Extract(ctx, pluginID, pluginArchive.File) if err != nil { return err } @@ -111,7 +111,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.Add(ctx, dep.ID, d.File) + depArchive, err := m.pluginStorage.Extract(ctx, dep.ID, d.File) if err != nil { return err } @@ -134,7 +134,7 @@ func (m *PluginInstaller) Remove(ctx context.Context, pluginID string) error { return plugins.ErrPluginNotInstalled } - if !plugin.IsExternalPlugin() { + if plugin.IsCorePlugin() || plugin.IsBundledPlugin() { return plugins.ErrUninstallCorePlugin } diff --git a/pkg/plugins/manager/installer_test.go b/pkg/plugins/manager/installer_test.go index d9dec824530..94769b29b6d 100644 --- a/pkg/plugins/manager/installer_test.go +++ b/pkg/plugins/manager/installer_test.go @@ -52,18 +52,13 @@ func TestPluginManager_Add_Remove(t *testing.T) { } fs := &fakes.FakePluginStorage{ - AddFunc: func(_ context.Context, id string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { + ExtractFunc: func(_ context.Context, id string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { require.Equal(t, pluginID, id) require.Equal(t, mockZipV1, z) return &storage.ExtractedPluginArchive{ Path: zipNameV1, }, nil }, - RegisterFunc: func(_ context.Context, pluginID, pluginDir string) error { - require.Equal(t, pluginV1.ID, pluginID) - return nil - }, - Store: map[string]struct{}{}, } inst := New(fakes.NewFakePluginRegistry(), loader, pluginRepo, fs) @@ -112,17 +107,13 @@ func TestPluginManager_Add_Remove(t *testing.T) { File: mockZipV2, }, nil } - fs.AddFunc = func(_ context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { + fs.ExtractFunc = func(_ context.Context, pluginID string, z *zip.ReadCloser) (*storage.ExtractedPluginArchive, error) { require.Equal(t, pluginV1.ID, pluginID) require.Equal(t, mockZipV2, z) return &storage.ExtractedPluginArchive{ Path: zipNameV2, }, nil } - fs.RegisterFunc = func(_ context.Context, pluginID, pluginDir string) error { - require.Equal(t, pluginV2.ID, pluginID) - return nil - } err = inst.Add(context.Background(), pluginID, v2, plugins.CompatOpts{}) require.NoError(t, err) diff --git a/pkg/plugins/manager/loader/loader.go b/pkg/plugins/manager/loader/loader.go index b515a0364a3..0ebce9e494a 100644 --- a/pkg/plugins/manager/loader/loader.go +++ b/pkg/plugins/manager/loader/loader.go @@ -18,7 +18,6 @@ import ( "github.com/grafana/grafana/pkg/plugins/manager/process" "github.com/grafana/grafana/pkg/plugins/manager/registry" "github.com/grafana/grafana/pkg/plugins/manager/signature" - "github.com/grafana/grafana/pkg/plugins/storage" "github.com/grafana/grafana/pkg/util" ) @@ -32,24 +31,23 @@ type Loader struct { pluginInitializer initializer.Initializer signatureValidator signature.Validator signatureCalculator plugins.SignatureCalculator - pluginStorage storage.Manager assetPath *assetpath.Service log log.Logger cfg *config.Cfg - errs map[string]*plugins.SignatureError + + errs map[string]*plugins.SignatureError } func ProvideService(cfg *config.Cfg, license plugins.Licensing, authorizer plugins.PluginLoaderAuthorizer, pluginRegistry registry.Service, backendProvider plugins.BackendFactoryProvider, pluginFinder finder.Finder, roleRegistry plugins.RoleRegistry, assetPath *assetpath.Service, signatureCalculator plugins.SignatureCalculator) *Loader { return New(cfg, license, authorizer, pluginRegistry, backendProvider, process.NewManager(pluginRegistry), - storage.FileSystem(log.NewPrettyLogger("loader.fs"), cfg.PluginsPath), roleRegistry, assetPath, - pluginFinder, signatureCalculator) + roleRegistry, assetPath, pluginFinder, signatureCalculator) } func New(cfg *config.Cfg, license plugins.Licensing, authorizer plugins.PluginLoaderAuthorizer, pluginRegistry registry.Service, backendProvider plugins.BackendFactoryProvider, - processManager process.Service, pluginStorage storage.Manager, roleRegistry plugins.RoleRegistry, + processManager process.Service, roleRegistry plugins.RoleRegistry, assetPath *assetpath.Service, pluginFinder finder.Finder, signatureCalculator plugins.SignatureCalculator) *Loader { return &Loader{ pluginFinder: pluginFinder, @@ -58,7 +56,6 @@ func New(cfg *config.Cfg, license plugins.Licensing, authorizer plugins.PluginLo signatureValidator: signature.NewValidator(authorizer), signatureCalculator: signatureCalculator, processManager: processManager, - pluginStorage: pluginStorage, errs: make(map[string]*plugins.SignatureError), log: log.New("plugin.loader"), roleRegistry: roleRegistry, @@ -201,7 +198,7 @@ func (l *Loader) Unload(ctx context.Context, pluginID string) error { return plugins.ErrPluginNotInstalled } - if !plugin.IsExternalPlugin() { + if plugin.IsCorePlugin() || plugin.IsBundledPlugin() { return plugins.ErrUninstallCorePlugin } @@ -220,12 +217,6 @@ func (l *Loader) load(ctx context.Context, p *plugins.Plugin) error { l.log.Info("Plugin registered", "pluginID", p.ID) } - if p.IsExternalPlugin() { - if err := l.pluginStorage.Register(ctx, p.ID, p.FS.Base()); err != nil { - return err - } - } - return l.processManager.Start(ctx, p.ID) } @@ -241,9 +232,12 @@ func (l *Loader) unload(ctx context.Context, p *plugins.Plugin) error { } l.log.Debug("Plugin unregistered", "pluginId", p.ID) - if err := l.pluginStorage.Remove(ctx, p.ID); err != nil { - return err + if remover, ok := p.FS.(plugins.FSRemover); ok { + if err := remover.Remove(); err != nil { + return err + } } + return nil } diff --git a/pkg/plugins/manager/loader/loader_test.go b/pkg/plugins/manager/loader/loader_test.go index 2cc0663a8e8..d0dda4d4c51 100644 --- a/pkg/plugins/manager/loader/loader_test.go +++ b/pkg/plugins/manager/loader/loader_test.go @@ -457,12 +457,10 @@ func TestLoader_Load(t *testing.T) { } for _, tt := range tests { reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() procMgr := fakes.NewFakeProcessManager() l := newLoader(tt.cfg, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(tt.cfg, procPrvdr, &fakes.FakeLicensingService{}) }) @@ -480,7 +478,7 @@ func TestLoader_Load(t *testing.T) { require.Equal(t, tt.pluginErrors[pluginErr.PluginID], pluginErr) } - verifyState(t, tt.want, reg, procPrvdr, storage, procMgr) + verifyState(t, tt.want, reg, procPrvdr, procMgr) }) } } @@ -698,12 +696,10 @@ func TestLoader_Load_MultiplePlugins(t *testing.T) { for _, tt := range tests { reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() procMgr := fakes.NewFakeProcessManager() l := newLoader(tt.cfg, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(tt.cfg, procPrvdr, fakes.NewFakeLicensingService()) }) @@ -734,7 +730,7 @@ func TestLoader_Load_MultiplePlugins(t *testing.T) { for _, pluginErr := range pluginErrs { require.Equal(t, tt.pluginErrors[pluginErr.PluginID], pluginErr) } - verifyState(t, tt.want, reg, procPrvdr, storage, procMgr) + verifyState(t, tt.want, reg, procPrvdr, procMgr) }) } }) @@ -824,12 +820,10 @@ func TestLoader_Load_RBACReady(t *testing.T) { }) setting.AppUrl = "http://localhost:3000" reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() procMgr := fakes.NewFakeProcessManager() l := newLoader(tt.cfg, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(tt.cfg, procPrvdr, fakes.NewFakeLicensingService()) }) @@ -850,7 +844,7 @@ func TestLoader_Load_RBACReady(t *testing.T) { pluginErrs := l.PluginErrors() require.Len(t, pluginErrs, 0) - verifyState(t, tt.want, reg, procPrvdr, storage, procMgr) + verifyState(t, tt.want, reg, procPrvdr, procMgr) } } @@ -908,12 +902,10 @@ func TestLoader_Load_Signature_RootURL(t *testing.T) { } reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() procMgr := fakes.NewFakeProcessManager() l := newLoader(&config.Cfg{}, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService()) }) @@ -930,7 +922,7 @@ func TestLoader_Load_Signature_RootURL(t *testing.T) { if !cmp.Equal(got, expected, compareOpts...) { t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...)) } - verifyState(t, expected, reg, procPrvdr, storage, procMgr) + verifyState(t, expected, reg, procPrvdr, procMgr) }) } @@ -994,12 +986,10 @@ func TestLoader_Load_DuplicatePlugins(t *testing.T) { } reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() procMgr := fakes.NewFakeProcessManager() l := newLoader(&config.Cfg{}, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService()) }) @@ -1017,7 +1007,7 @@ func TestLoader_Load_DuplicatePlugins(t *testing.T) { t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...)) } - verifyState(t, expected, reg, procPrvdr, storage, procMgr) + verifyState(t, expected, reg, procPrvdr, procMgr) }) } @@ -1086,7 +1076,6 @@ func TestLoader_Load_SkipUninitializedPlugins(t *testing.T) { } reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() // Cause an initialization error procPrvdr.BackendFactoryFunc = func(ctx context.Context, p *plugins.Plugin) backendplugin.PluginFactoryFunc { @@ -1100,7 +1089,6 @@ func TestLoader_Load_SkipUninitializedPlugins(t *testing.T) { procMgr := fakes.NewFakeProcessManager() l := newLoader(&config.Cfg{}, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService()) }) @@ -1118,7 +1106,7 @@ func TestLoader_Load_SkipUninitializedPlugins(t *testing.T) { t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...)) } - verifyState(t, expected, reg, procPrvdr, storage, procMgr) + verifyState(t, expected, reg, procPrvdr, procMgr) }) } @@ -1182,7 +1170,6 @@ func TestLoader_Load_UseAPIForManifestPublicKey(t *testing.T) { } reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() procMgr := fakes.NewFakeProcessManager() apiCalled := false @@ -1210,7 +1197,6 @@ func TestLoader_Load_UseAPIForManifestPublicKey(t *testing.T) { cfg.GrafanaComURL = s.URL l := newLoader(cfg, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(cfg, procPrvdr, fakes.NewFakeLicensingService()) }) @@ -1229,7 +1215,7 @@ func TestLoader_Load_UseAPIForManifestPublicKey(t *testing.T) { t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...)) } - verifyState(t, expected, reg, procPrvdr, storage, procMgr) + verifyState(t, expected, reg, procPrvdr, procMgr) }) } @@ -1311,12 +1297,10 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { t.Run("Load nested External plugins", func(t *testing.T) { reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() procMgr := fakes.NewFakeProcessManager() l := newLoader(&config.Cfg{}, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService()) }) @@ -1341,7 +1325,7 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...)) } - verifyState(t, expected, reg, procPrvdr, storage, procMgr) + verifyState(t, expected, reg, procPrvdr, procMgr) t.Run("Load will exclude plugins that already exist", func(t *testing.T) { got, err := l.Load(context.Background(), &fakes.FakePluginSource{ @@ -1363,7 +1347,7 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...)) } - verifyState(t, expected, reg, procPrvdr, storage, procMgr) + verifyState(t, expected, reg, procPrvdr, procMgr) }) }) @@ -1493,12 +1477,10 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { expected := []*plugins.Plugin{parent, child} reg := fakes.NewFakePluginRegistry() - storage := fakes.NewFakePluginStorage() procPrvdr := fakes.NewFakeBackendProcessProvider() procMgr := fakes.NewFakeProcessManager() l := newLoader(&config.Cfg{}, func(l *Loader) { l.pluginRegistry = reg - l.pluginStorage = storage l.processManager = procMgr l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService()) }) @@ -1521,7 +1503,7 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...)) } - verifyState(t, expected, reg, procPrvdr, storage, procMgr) + verifyState(t, expected, reg, procPrvdr, procMgr) }) } @@ -1550,8 +1532,8 @@ func Test_setPathsBasedOnApp(t *testing.T) { func newLoader(cfg *config.Cfg, cbs ...func(loader *Loader)) *Loader { l := New(cfg, &fakes.FakeLicensingService{}, signature.NewUnsignedAuthorizer(cfg), fakes.NewFakePluginRegistry(), - fakes.NewFakeBackendProcessProvider(), fakes.NewFakeProcessManager(), fakes.NewFakePluginStorage(), - fakes.NewFakeRoleRegistry(), assetpath.ProvideService(pluginscdn.ProvideService(cfg)), finder.NewLocalFinder(), signature.ProvideService(cfg)) + fakes.NewFakeBackendProcessProvider(), fakes.NewFakeProcessManager(), fakes.NewFakeRoleRegistry(), + assetpath.ProvideService(pluginscdn.ProvideService(cfg)), finder.NewLocalFinder(), signature.ProvideService(cfg)) for _, cb := range cbs { cb(l) @@ -1561,7 +1543,7 @@ func newLoader(cfg *config.Cfg, cbs ...func(loader *Loader)) *Loader { } func verifyState(t *testing.T, ps []*plugins.Plugin, reg *fakes.FakePluginRegistry, - procPrvdr *fakes.FakeBackendProcessProvider, storage *fakes.FakePluginStorage, procMngr *fakes.FakeProcessManager) { + procPrvdr *fakes.FakeBackendProcessProvider, procMngr *fakes.FakeProcessManager) { t.Helper() for _, p := range ps { @@ -1577,13 +1559,6 @@ func verifyState(t *testing.T, ps []*plugins.Plugin, reg *fakes.FakePluginRegist require.Zero(t, procPrvdr.Invoked[p.ID]) } - _, exists := storage.Store[p.ID] - if p.IsExternalPlugin() { - require.True(t, exists) - } else { - require.False(t, exists) - } - require.Equal(t, 1, procMngr.Started[p.ID]) require.Zero(t, procMngr.Stopped[p.ID]) } diff --git a/pkg/plugins/plugins.go b/pkg/plugins/plugins.go index 8fc2af80a02..fd27a43c15c 100644 --- a/pkg/plugins/plugins.go +++ b/pkg/plugins/plugins.go @@ -21,8 +21,9 @@ import ( ) var ( - ErrFileNotExist = errors.New("file does not exist") - ErrPluginFileRead = errors.New("file could not be read") + ErrFileNotExist = errors.New("file does not exist") + ErrPluginFileRead = errors.New("file could not be read") + ErrUninstallInvalidPluginDir = errors.New("cannot recognize as plugin folder") ) type Plugin struct { diff --git a/pkg/plugins/storage/fs.go b/pkg/plugins/storage/fs.go index 2d938d09968..f3468ab3f23 100644 --- a/pkg/plugins/storage/fs.go +++ b/pkg/plugins/storage/fs.go @@ -12,36 +12,27 @@ import ( "path/filepath" "regexp" "strings" - "sync" "github.com/grafana/grafana/pkg/plugins/log" ) -var _ Manager = (*FS)(nil) +var _ ZipExtractor = (*FS)(nil) var reGitBuild = regexp.MustCompile("^[a-zA-Z0-9_.-]*/") -var ( - ErrUninstallOutsideOfPluginDir = errors.New("cannot uninstall a plugin outside of the plugins directory") - ErrUninstallInvalidPluginDir = errors.New("cannot recognize as plugin folder") -) - type FS struct { - store map[string]string - mu sync.RWMutex pluginsDir string log log.PrettyLogger } func FileSystem(logger log.PrettyLogger, pluginsDir string) *FS { return &FS{ - store: make(map[string]string), pluginsDir: pluginsDir, log: logger, } } -func (fs *FS) Add(ctx context.Context, pluginID string, pluginArchive *zip.ReadCloser) ( +func (fs *FS) Extract(ctx context.Context, pluginID string, pluginArchive *zip.ReadCloser) ( *ExtractedPluginArchive, error) { pluginDir, err := fs.extractFiles(ctx, pluginArchive, pluginID) if err != nil { @@ -71,38 +62,6 @@ func (fs *FS) Add(ctx context.Context, pluginID string, pluginArchive *zip.ReadC }, nil } -func (fs *FS) Register(_ context.Context, pluginID, pluginDir string) error { - fs.mu.Lock() - fs.store[pluginID] = pluginDir - fs.mu.Unlock() - - return nil -} - -func (fs *FS) Remove(_ context.Context, pluginID string) error { - fs.mu.RLock() - pluginDir, exists := fs.store[pluginID] - fs.mu.RUnlock() - if !exists { - return fmt.Errorf("%s does not exist", pluginID) - } - - // extra security check to ensure we only remove plugins that are located in the configured plugins directory - path, err := filepath.Rel(fs.pluginsDir, pluginDir) - if err != nil || strings.HasPrefix(path, ".."+string(filepath.Separator)) { - return ErrUninstallOutsideOfPluginDir - } - - if _, err = os.Stat(filepath.Join(pluginDir, "plugin.json")); os.IsNotExist(err) { - if _, err = os.Stat(filepath.Join(pluginDir, "dist/plugin.json")); os.IsNotExist(err) { - return ErrUninstallInvalidPluginDir - } - } - - fs.log.Infof("Uninstalling plugin %v", pluginDir) - return os.RemoveAll(pluginDir) -} - func (fs *FS) extractFiles(_ context.Context, pluginArchive *zip.ReadCloser, pluginID string) (string, error) { installDir := filepath.Join(fs.pluginsDir, pluginID) if _, err := os.Stat(installDir); !os.IsNotExist(err) { @@ -261,7 +220,7 @@ func removeGitBuildFromName(filename, pluginID string) string { return reGitBuild.ReplaceAllString(filename, pluginID+"/") } -func toPluginDTO(pluginID, pluginDir string) (InstalledPlugin, error) { +func toPluginDTO(pluginID, pluginDir string) (installedPlugin, error) { distPluginDataPath := filepath.Join(pluginDir, "dist", "plugin.json") // It's safe to ignore gosec warning G304 since the file path suffix is hardcoded @@ -273,17 +232,17 @@ func toPluginDTO(pluginID, pluginDir string) (InstalledPlugin, error) { // nolint:gosec data, err = os.ReadFile(pluginDataPath) if err != nil { - return InstalledPlugin{}, fmt.Errorf("could not find dist/plugin.json or plugin.json for %s in %s", pluginID, pluginDir) + return installedPlugin{}, fmt.Errorf("could not find dist/plugin.json or plugin.json for %s in %s", pluginID, pluginDir) } } - res := InstalledPlugin{} + res := installedPlugin{} if err = json.Unmarshal(data, &res); err != nil { return res, err } if res.ID == "" { - return InstalledPlugin{}, fmt.Errorf("could not find valid plugin %s in %s", pluginID, pluginDir) + return installedPlugin{}, fmt.Errorf("could not find valid plugin %s in %s", pluginID, pluginDir) } if res.Info.Version == "" { diff --git a/pkg/plugins/storage/fs_test.go b/pkg/plugins/storage/fs_test.go index e55e69842e2..cc9169a8bb9 100644 --- a/pkg/plugins/storage/fs_test.go +++ b/pkg/plugins/storage/fs_test.go @@ -25,7 +25,7 @@ func TestAdd(t *testing.T) { pluginID := "test-app" fs := FileSystem(&fakeLogger{}, testDir) - archive, err := fs.Add(context.Background(), pluginID, zipFile(t, "./testdata/plugin-with-symlinks.zip")) + archive, err := fs.Extract(context.Background(), pluginID, zipFile(t, "./testdata/plugin-with-symlinks.zip")) require.NotNil(t, archive) require.NoError(t, err) @@ -48,89 +48,6 @@ func TestAdd(t *testing.T) { require.Equal(t, files[5].Name(), "text.txt") } -func TestRemove(t *testing.T) { - pluginDir := t.TempDir() - pluginJSON := filepath.Join(pluginDir, "plugin.json") - //nolint:gosec - _, err := os.Create(pluginJSON) - require.NoError(t, err) - - pluginID := "test-datasource" - i := &FS{ - pluginsDir: filepath.Dir(pluginDir), - store: map[string]string{ - pluginID: pluginDir, - }, - log: &fakeLogger{}, - } - - err = i.Remove(context.Background(), pluginID) - require.NoError(t, err) - - _, err = os.Stat(pluginDir) - require.True(t, os.IsNotExist(err)) - - t.Run("Uninstall will search in nested dir folder for plugin.json", func(t *testing.T) { - pluginDistDir := filepath.Join(t.TempDir(), "dist") - err = os.Mkdir(pluginDistDir, os.ModePerm) - require.NoError(t, err) - pluginJSON = filepath.Join(pluginDistDir, "plugin.json") - //nolint:gosec - _, err = os.Create(pluginJSON) - require.NoError(t, err) - - pluginDir = filepath.Dir(pluginDistDir) - - i = &FS{ - pluginsDir: filepath.Dir(pluginDir), - store: map[string]string{ - pluginID: pluginDir, - }, - log: &fakeLogger{}, - } - - err = i.Remove(context.Background(), pluginID) - require.NoError(t, err) - - _, err = os.Stat(pluginDir) - require.True(t, os.IsNotExist(err)) - }) - - t.Run("Uninstall will not delete folder if cannot recognize plugin structure", func(t *testing.T) { - pluginDir = t.TempDir() - i = &FS{ - pluginsDir: filepath.Dir(pluginDir), - store: map[string]string{ - pluginID: pluginDir, - }, - log: &fakeLogger{}, - } - - err = i.Remove(context.Background(), pluginID) - require.EqualError(t, err, "cannot recognize as plugin folder") - - _, err = os.Stat(pluginDir) - require.False(t, os.IsNotExist(err)) - }) - - t.Run("Uninstall will not delete folder if plugin's directory is not a subdirectory of specified plugins directory", func(t *testing.T) { - pluginDir = t.TempDir() - i = &FS{ - pluginsDir: "/some/other/path", - store: map[string]string{ - pluginID: pluginDir, - }, - log: &fakeLogger{}, - } - - err = i.Remove(context.Background(), pluginID) - require.EqualError(t, err, "cannot uninstall a plugin outside of the plugins directory") - - _, err = os.Stat(pluginDir) - require.False(t, os.IsNotExist(err)) - }) -} - func TestExtractFiles(t *testing.T) { pluginsDir := setupFakePluginsDir(t) diff --git a/pkg/plugins/storage/ifaces.go b/pkg/plugins/storage/ifaces.go index f77a9cece07..027827cf89d 100644 --- a/pkg/plugins/storage/ifaces.go +++ b/pkg/plugins/storage/ifaces.go @@ -5,8 +5,6 @@ import ( "context" ) -type Manager interface { - Add(ctx context.Context, pluginID string, rc *zip.ReadCloser) (*ExtractedPluginArchive, error) - Register(ctx context.Context, pluginID, pluginDir string) error - Remove(ctx context.Context, pluginID string) error +type ZipExtractor interface { + Extract(ctx context.Context, pluginID string, rc *zip.ReadCloser) (*ExtractedPluginArchive, error) } diff --git a/pkg/plugins/storage/models.go b/pkg/plugins/storage/models.go index 5d0b6dbebb8..d758b18660f 100644 --- a/pkg/plugins/storage/models.go +++ b/pkg/plugins/storage/models.go @@ -22,27 +22,27 @@ type Dependency struct { Version string } -type InstalledPlugin struct { +type installedPlugin struct { ID string `json:"id"` Name string `json:"name"` Type string `json:"type"` - Info PluginInfo `json:"info"` - Dependencies Dependencies `json:"dependencies"` + Info pluginInfo `json:"info"` + Dependencies dependencies `json:"dependencies"` } -type Dependencies struct { +type dependencies struct { GrafanaVersion string `json:"grafanaVersion"` - Plugins []PluginDependency `json:"plugins"` + Plugins []pluginDependency `json:"plugins"` } -type PluginDependency struct { +type pluginDependency struct { ID string `json:"id"` Type string `json:"type"` Name string `json:"name"` Version string `json:"version"` } -type PluginInfo struct { +type pluginInfo struct { Version string `json:"version"` Updated string `json:"updated"` }