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
2019-11-18 13:40:49 -04:00
--- 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) {
--- 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) {
--- 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)
--- a/app/plugin_install_test.go
+++ 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})
+		})
+	})
+}
--- 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)
--- 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"
--- 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.