From 2571d9772387d19666cbc9b95dcfc6ce21e06173 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 18 Nov 2019 13:40:49 -0400 Subject: [PATCH] MM-18517: Plugin Marketplace (Phase 2) (#13086) * MM-17549: use StatusNotFound when deleting plugin (#12983) Trying to delete a plugin that does not exist should fail with a 404, not a 400. Relates-to: https://mattermost.atlassian.net/browse/MM-17549 * MM-19630: marketplace: model ReleaseNotesURL (#13083) Automatic Merge * MM-20065: allow prepackaged plugin upgrade (#13076) * MM-20065: allow prepackaged plugin upgrade When locally installing prepackaged plugins, skip if a plugin exists with the same id and is the same or a newer version. This is effectively a "poor man's" rework of prepackaged plugins to allow upgrade of prepackaged plugins via the marketplace. The larger plan to rework prepackaged plugins was deferred from v5.18. Fixes: https://mattermost.atlassian.net/browse/MM-20065 * eliminate unnecessary installPlugin * fix TestPluginSync defaults to match minio * cleanExistingBundles * close prepackaged filereader * simplify --- api4/plugin_test.go | 6 +- app/plugin.go | 18 ++- app/plugin_install.go | 70 ++++++++-- app/plugin_install_test.go | 258 ++++++++++++++++++++++++++++++++++++ app/plugin_test.go | 2 +- i18n/en.json | 4 + model/marketplace_plugin.go | 9 +- 7 files changed, 340 insertions(+), 27 deletions(-) create mode 100644 app/plugin_install_test.go diff --git a/api4/plugin_test.go b/api4/plugin_test.go index a1732b4db1..d34bb5edc5 100644 --- a/api4/plugin_test.go +++ b/api4/plugin_test.go @@ -206,7 +206,7 @@ func TestPlugin(t *testing.T) { // Deactivate error case ok, resp = th.SystemAdminClient.DisablePlugin("junk") - CheckBadRequestStatus(t, resp) + CheckNotFoundStatus(t, resp) assert.False(t, ok) // Get error cases @@ -241,7 +241,7 @@ func TestPlugin(t *testing.T) { // Remove error cases ok, resp = th.SystemAdminClient.RemovePlugin(manifest.Id) - CheckBadRequestStatus(t, resp) + CheckNotFoundStatus(t, resp) assert.False(t, ok) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.PluginSettings.Enable = false }) @@ -253,7 +253,7 @@ func TestPlugin(t *testing.T) { CheckForbiddenStatus(t, resp) _, resp = th.SystemAdminClient.RemovePlugin("bad.id") - CheckBadRequestStatus(t, resp) + CheckNotFoundStatus(t, resp) } func TestNotifyClusterPluginEvent(t *testing.T) { diff --git a/app/plugin.go b/app/plugin.go index e543a7de51..36a7b4bd58 100644 --- a/app/plugin.go +++ b/app/plugin.go @@ -164,10 +164,18 @@ func (a *App) InitPlugins(pluginDir, webappPluginDir string) { return nil } - if fileReader, err := os.Open(walkPath); err != nil { + fileReader, err := os.Open(walkPath) + if err != nil { mlog.Error("Failed to open prepackaged plugin", mlog.Err(err), mlog.String("path", walkPath)) - } else if _, err := a.installPluginLocally(fileReader, true); err != nil { - mlog.Error("Failed to unpack prepackaged plugin", mlog.Err(err), mlog.String("path", walkPath)) + return nil + } + defer fileReader.Close() + + mlog.Debug("Installing prepackaged plugin", mlog.String("path", walkPath)) + + _, appErr := a.installPluginLocally(fileReader, installPluginLocallyOnlyIfNewOrUpgrade) + if appErr != nil { + mlog.Error("Failed to unpack prepackaged plugin", mlog.Err(appErr), mlog.String("path", walkPath)) } return nil @@ -250,7 +258,7 @@ func (a *App) SyncPlugins() *model.AppError { defer reader.Close() mlog.Info("Syncing plugin from file store", mlog.String("bundle", path)) - if _, err := a.installPluginLocally(reader, true); err != nil { + if _, err := a.installPluginLocally(reader, installPluginLocallyAlways); err != nil { mlog.Error("Failed to sync plugin from file store", mlog.String("bundle", path), mlog.Err(err)) } } @@ -364,7 +372,7 @@ func (a *App) DisablePlugin(id string) *model.AppError { } if manifest == nil { - return model.NewAppError("DisablePlugin", "app.plugin.not_installed.app_error", nil, "", http.StatusBadRequest) + return model.NewAppError("DisablePlugin", "app.plugin.not_installed.app_error", nil, "", http.StatusNotFound) } a.UpdateConfig(func(cfg *model.Config) { diff --git a/app/plugin_install.go b/app/plugin_install.go index 26b768418e..4816b23340 100644 --- a/app/plugin_install.go +++ b/app/plugin_install.go @@ -31,8 +31,8 @@ // Finally, in addition to managed plugins, note that there are unmanaged and prepackaged plugins. // Unmanaged plugins are plugins installed manually to the configured local directory (PluginSettings.Directory). // Prepackaged plugins are included with the server. They otherwise follow the above flow, except do not get uploaded -// to the filestore. Prepackaged plugins override all other plugins with the same plugin id. Managed plugins -// override unmanaged plugins with the same plugin id. +// to the filestore. Prepackaged plugins override all other plugins with the same plugin id, but only when the prepackaged +// plugin is newer. Managed plugins unconditionally override unmanaged plugins with the same plugin id. // package app @@ -44,6 +44,7 @@ import ( "os" "path/filepath" + "github.com/blang/semver" "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/plugin" @@ -67,7 +68,7 @@ func (a *App) InstallPluginFromData(data model.PluginEventData) { } defer reader.Close() - manifest, appErr := a.installPluginLocally(reader, true) + manifest, appErr := a.installPluginLocally(reader, installPluginLocallyAlways) if appErr != nil { mlog.Error("Failed to unpack plugin from filestore", mlog.Err(appErr), mlog.String("path", fileStorePath)) } @@ -95,11 +96,12 @@ func (a *App) RemovePluginFromData(data model.PluginEventData) { // InstallPlugin unpacks and installs a plugin but does not enable or activate it. func (a *App) InstallPlugin(pluginFile io.ReadSeeker, replace bool) (*model.Manifest, *model.AppError) { - return a.installPlugin(pluginFile, replace) -} + installationStrategy := installPluginLocallyOnlyIfNew + if replace { + installationStrategy = installPluginLocallyAlways + } -func (a *App) installPlugin(pluginFile io.ReadSeeker, replace bool) (*model.Manifest, *model.AppError) { - manifest, appErr := a.installPluginLocally(pluginFile, replace) + manifest, appErr := a.installPluginLocally(pluginFile, installationStrategy) if appErr != nil { return nil, appErr } @@ -129,7 +131,18 @@ func (a *App) installPlugin(pluginFile io.ReadSeeker, replace bool) (*model.Mani return manifest, nil } -func (a *App) installPluginLocally(pluginFile io.ReadSeeker, replace bool) (*model.Manifest, *model.AppError) { +type pluginInstallationStrategy int + +const ( + // installPluginLocallyOnlyIfNew installs the given plugin locally only if no plugin with the same id has been unpacked. + installPluginLocallyOnlyIfNew pluginInstallationStrategy = iota + // installPluginLocallyOnlyIfNewOrUpgrade installs the given plugin locally only if no plugin with the same id has been unpacked, or if such a plugin is older. + installPluginLocallyOnlyIfNewOrUpgrade + // installPluginLocallyAlways unconditionally installs the given plugin locally only, clobbering any existing plugin with the same id. + installPluginLocallyAlways +) + +func (a *App) installPluginLocally(pluginFile io.ReadSeeker, installationStrategy pluginInstallationStrategy) (*model.Manifest, *model.AppError) { pluginsEnvironment := a.GetPluginsEnvironment() if pluginsEnvironment == nil { return nil, model.NewAppError("installPluginLocally", "app.plugin.disabled.app_error", nil, "", http.StatusNotImplemented) @@ -169,16 +182,45 @@ func (a *App) installPluginLocally(pluginFile io.ReadSeeker, replace bool) (*mod return nil, model.NewAppError("installPluginLocally", "app.plugin.install.app_error", nil, err.Error(), http.StatusInternalServerError) } - // Check that there is no plugin with the same ID + // Check for plugins installed with the same ID. + var existingManifest *model.Manifest for _, bundle := range bundles { if bundle.Manifest != nil && bundle.Manifest.Id == manifest.Id { - if !replace { - return nil, model.NewAppError("installPluginLocally", "app.plugin.install_id.app_error", nil, "", http.StatusBadRequest) + existingManifest = bundle.Manifest + break + } + } + + if existingManifest != nil { + // Return an error if already installed and strategy disallows installation. + if installationStrategy == installPluginLocallyOnlyIfNew { + return nil, model.NewAppError("installPluginLocally", "app.plugin.install_id.app_error", nil, "", http.StatusBadRequest) + } + + // Skip installation if already installed and newer. + if installationStrategy == installPluginLocallyOnlyIfNewOrUpgrade { + var version, existingVersion semver.Version + + version, err = semver.Parse(manifest.Version) + if err != nil { + return nil, model.NewAppError("installPluginLocally", "app.plugin.invalid_version.app_error", nil, "", http.StatusBadRequest) } - if err := a.removePluginLocally(manifest.Id); err != nil { - return nil, model.NewAppError("installPluginLocally", "app.plugin.install_id_failed_remove.app_error", nil, "", http.StatusBadRequest) + existingVersion, err = semver.Parse(existingManifest.Version) + if err != nil { + return nil, model.NewAppError("installPluginLocally", "app.plugin.invalid_version.app_error", nil, "", http.StatusBadRequest) } + + if version.LTE(existingVersion) { + mlog.Debug("Skipping local installation of plugin since existing version is newer", mlog.String("plugin_id", manifest.Id)) + return nil, nil + } + } + + // Otherwise remove the existing installation prior to install below. + mlog.Debug("Removing existing installation of plugin before local install", mlog.String("plugin_id", existingManifest.Id), mlog.String("version", existingManifest.Version)) + if err := a.removePluginLocally(existingManifest.Id); err != nil { + return nil, model.NewAppError("installPluginLocally", "app.plugin.install_id_failed_remove.app_error", nil, "", http.StatusBadRequest) } } @@ -280,7 +322,7 @@ func (a *App) removePluginLocally(id string) *model.AppError { } if manifest == nil { - return model.NewAppError("removePlugin", "app.plugin.not_installed.app_error", nil, "", http.StatusBadRequest) + return model.NewAppError("removePlugin", "app.plugin.not_installed.app_error", nil, "", http.StatusNotFound) } pluginsEnvironment.Deactivate(id) diff --git a/app/plugin_install_test.go b/app/plugin_install_test.go new file mode 100644 index 0000000000..0afd45ef4c --- /dev/null +++ b/app/plugin_install_test.go @@ -0,0 +1,258 @@ +package app + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "io" + "sort" + "testing" + + "github.com/mattermost/mattermost-server/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type nilReadSeeker struct { +} + +func (r *nilReadSeeker) Read(p []byte) (int, error) { + return 0, io.EOF +} + +func (r *nilReadSeeker) Seek(offset int64, whence int) (int64, error) { + return 0, nil +} + +type testFile struct { + Name, Body string +} + +func makeInMemoryGzipTarFile(t *testing.T, files []testFile) *bytes.Reader { + var buf bytes.Buffer + gzWriter := gzip.NewWriter(&buf) + + tgz := tar.NewWriter(gzWriter) + + for _, file := range files { + hdr := &tar.Header{ + Name: file.Name, + Mode: 0600, + Size: int64(len(file.Body)), + } + err := tgz.WriteHeader(hdr) + require.NoError(t, err, "failed to write %s to in-memory tar file", file.Name) + _, err = tgz.Write([]byte(file.Body)) + require.NoError(t, err, "failed to write body of %s to in-memory tar file", file.Name) + } + err := tgz.Close() + require.NoError(t, err, "failed to close in-memory tar file") + + err = gzWriter.Close() + require.NoError(t, err, "failed to close in-memory tar.gz file") + + return bytes.NewReader(buf.Bytes()) +} + +type byBundleInfoId []*model.BundleInfo + +func (b byBundleInfoId) Len() int { return len(b) } +func (b byBundleInfoId) Swap(i, j int) { b[i], b[j] = b[j], b[i] } +func (b byBundleInfoId) Less(i, j int) bool { return b[i].Manifest.Id < b[j].Manifest.Id } + +func TestInstallPluginLocally(t *testing.T) { + t.Run("invalid tar", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + actualManifest, appErr := th.App.installPluginLocally(&nilReadSeeker{}, installPluginLocallyOnlyIfNew) + require.NotNil(t, appErr) + assert.Equal(t, "app.plugin.extract.app_error", appErr.Id, appErr.Error()) + require.Nil(t, actualManifest) + }) + + t.Run("missing manifest", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + reader := makeInMemoryGzipTarFile(t, []testFile{ + {"test", "test file"}, + }) + + actualManifest, appErr := th.App.installPluginLocally(reader, installPluginLocallyOnlyIfNew) + require.NotNil(t, appErr) + assert.Equal(t, "app.plugin.manifest.app_error", appErr.Id, appErr.Error()) + require.Nil(t, actualManifest) + }) + + installPlugin := func(t *testing.T, th *TestHelper, id, version string, installationStrategy pluginInstallationStrategy) (*model.Manifest, *model.AppError) { + t.Helper() + + manifest := &model.Manifest{ + Id: id, + Version: version, + } + reader := makeInMemoryGzipTarFile(t, []testFile{ + {"plugin.json", manifest.ToJson()}, + }) + + actualManifest, appError := th.App.installPluginLocally(reader, installationStrategy) + if actualManifest != nil { + require.Equal(t, manifest, actualManifest) + } + + return actualManifest, appError + } + + t.Run("invalid plugin id", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + actualManifest, appErr := installPlugin(t, th, "invalid#plugin#id", "version", installPluginLocallyOnlyIfNew) + require.NotNil(t, appErr) + assert.Equal(t, "app.plugin.invalid_id.app_error", appErr.Id, appErr.Error()) + require.Nil(t, actualManifest) + }) + + // The following tests fail mysteriously on CI due to an unexpected bundle being present. + // This exists to clean up manually until we figure out what test isn't cleaning up after + // itself. + cleanExistingBundles := func(t *testing.T, th *TestHelper) { + pluginsEnvironment := th.App.GetPluginsEnvironment() + require.NotNil(t, pluginsEnvironment) + bundleInfos, err := pluginsEnvironment.Available() + require.Nil(t, err) + + for _, bundleInfo := range bundleInfos { + err := th.App.removePluginLocally(bundleInfo.Manifest.Id) + require.Nilf(t, err, "failed to remove existing plugin %s", bundleInfo.Manifest.Id) + } + } + + assertBundleInfoManifests := func(t *testing.T, th *TestHelper, manifests []*model.Manifest) { + pluginsEnvironment := th.App.GetPluginsEnvironment() + require.NotNil(t, pluginsEnvironment) + bundleInfos, err := pluginsEnvironment.Available() + require.Nil(t, err) + + sort.Sort(byBundleInfoId(bundleInfos)) + + actualManifests := make([]*model.Manifest, 0, len(bundleInfos)) + for _, bundleInfo := range bundleInfos { + actualManifests = append(actualManifests, bundleInfo.Manifest) + } + + require.Equal(t, manifests, actualManifests) + } + + t.Run("no plugins already installed", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + cleanExistingBundles(t, th) + + manifest, appErr := installPlugin(t, th, "valid", "0.0.1", installPluginLocallyOnlyIfNew) + require.Nil(t, appErr) + require.NotNil(t, manifest) + + assertBundleInfoManifests(t, th, []*model.Manifest{manifest}) + }) + + t.Run("different plugin already installed", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + cleanExistingBundles(t, th) + + otherManifest, appErr := installPlugin(t, th, "other", "0.0.1", installPluginLocallyOnlyIfNew) + require.Nil(t, appErr) + require.NotNil(t, otherManifest) + + manifest, appErr := installPlugin(t, th, "valid", "0.0.1", installPluginLocallyOnlyIfNew) + require.Nil(t, appErr) + require.NotNil(t, manifest) + + assertBundleInfoManifests(t, th, []*model.Manifest{otherManifest, manifest}) + }) + + t.Run("same plugin already installed", func(t *testing.T) { + t.Run("install only if new", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + cleanExistingBundles(t, th) + + existingManifest, appErr := installPlugin(t, th, "valid", "0.0.1", installPluginLocallyOnlyIfNew) + require.Nil(t, appErr) + require.NotNil(t, existingManifest) + + manifest, appErr := installPlugin(t, th, "valid", "0.0.1", installPluginLocallyOnlyIfNew) + require.NotNil(t, appErr) + require.Equal(t, "app.plugin.install_id.app_error", appErr.Id, appErr.Error()) + require.Nil(t, manifest) + + assertBundleInfoManifests(t, th, []*model.Manifest{existingManifest}) + }) + + t.Run("install if upgrade, but older", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + cleanExistingBundles(t, th) + + existingManifest, appErr := installPlugin(t, th, "valid", "0.0.2", installPluginLocallyOnlyIfNewOrUpgrade) + require.Nil(t, appErr) + require.NotNil(t, existingManifest) + + manifest, appErr := installPlugin(t, th, "valid", "0.0.1", installPluginLocallyOnlyIfNewOrUpgrade) + require.Nil(t, appErr) + require.Nil(t, manifest) + + assertBundleInfoManifests(t, th, []*model.Manifest{existingManifest}) + }) + + t.Run("install if upgrade, but same version", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + cleanExistingBundles(t, th) + + existingManifest, appErr := installPlugin(t, th, "valid", "0.0.2", installPluginLocallyOnlyIfNewOrUpgrade) + require.Nil(t, appErr) + require.NotNil(t, existingManifest) + + manifest, appErr := installPlugin(t, th, "valid", "0.0.2", installPluginLocallyOnlyIfNewOrUpgrade) + require.Nil(t, appErr) + require.Nil(t, manifest) + + assertBundleInfoManifests(t, th, []*model.Manifest{existingManifest}) + }) + + t.Run("install if upgrade, newer version", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + cleanExistingBundles(t, th) + + existingManifest, appErr := installPlugin(t, th, "valid", "0.0.2", installPluginLocallyOnlyIfNewOrUpgrade) + require.Nil(t, appErr) + require.NotNil(t, existingManifest) + + manifest, appErr := installPlugin(t, th, "valid", "0.0.3", installPluginLocallyOnlyIfNewOrUpgrade) + require.Nil(t, appErr) + require.NotNil(t, manifest) + + assertBundleInfoManifests(t, th, []*model.Manifest{manifest}) + }) + + t.Run("install always, old version", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + cleanExistingBundles(t, th) + + existingManifest, appErr := installPlugin(t, th, "valid", "0.0.2", installPluginLocallyAlways) + require.Nil(t, appErr) + require.NotNil(t, existingManifest) + + manifest, appErr := installPlugin(t, th, "valid", "0.0.1", installPluginLocallyAlways) + require.Nil(t, appErr) + require.NotNil(t, manifest) + + assertBundleInfoManifests(t, th, []*model.Manifest{manifest}) + }) + }) +} diff --git a/app/plugin_test.go b/app/plugin_test.go index aa2f5654ac..3a02cf215d 100644 --- a/app/plugin_test.go +++ b/app/plugin_test.go @@ -483,7 +483,7 @@ func TestPluginSync(t *testing.T) { s3Port := os.Getenv("CI_MINIO_PORT") if s3Port == "" { - s3Port = "9001" + s3Port = "9000" } s3Endpoint := fmt.Sprintf("%s:%s", s3Host, s3Port) diff --git a/i18n/en.json b/i18n/en.json index 33d9be2de8..05bbe3519a 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -3514,6 +3514,10 @@ "id": "app.plugin.invalid_id.app_error", "translation": "Plugin Id must be at least {{.Min}} characters, at most {{.Max}} characters and match {{.Regex}}." }, + { + "id": "app.plugin.invalid_version.app_error", + "translation": "Plugin version could not be parsed" + }, { "id": "app.plugin.manifest.app_error", "translation": "Unable to find manifest for extracted plugin" diff --git a/model/marketplace_plugin.go b/model/marketplace_plugin.go index 154bdde00d..d2f2b54e7b 100644 --- a/model/marketplace_plugin.go +++ b/model/marketplace_plugin.go @@ -12,10 +12,11 @@ import ( // BaseMarketplacePlugin is a Mattermost plugin received from the marketplace server. type BaseMarketplacePlugin struct { - HomepageURL string `json:"homepage_url"` - DownloadURL string `json:"download_url"` - IconData string `json:"icon_data"` - Manifest *Manifest `json:"manifest"` + HomepageURL string `json:"homepage_url"` + DownloadURL string `json:"download_url"` + ReleaseNotesURL string `json:"release_notes_url"` + IconData string `json:"icon_data"` + Manifest *Manifest `json:"manifest"` } // MarketplacePlugin is a state aware marketplace plugin.