MM-17087 - Disable plugin on removal (#11779)

* MM-17087 - Disable plugin on removal

* Updated documentation

* Got reid of notifyPluginEvents

* Updated documentation

* Added plugin installation/activatoin flow as a toplevel go doc in plugin_install.go

* Generating webapp bundle on plugin installation

* Fixed shadowing issue

* Updated doc to include unguarded race condition

* Renamed GenerateWebappBundle

* Added a debug log when peers are not ready to notify

* Updated docs

* Removed extra line

api4/plugin_test.go

@@ -274,6 +274,8 @@ func TestNotifyClusterPluginEvent(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	testCluster.ClearMessages()
+
 	// Successful upload
 	manifest, resp := th.SystemAdminClient.UploadPlugin(bytes.NewReader(tarData))
 	CheckNoError(t, resp)
@@ -285,6 +287,7 @@ func TestNotifyClusterPluginEvent(t *testing.T) {
 	require.Nil(t, err)
 	require.True(t, pluginStored)
 
+	messages := testCluster.GetMessages()
 	expectedPluginData := model.PluginEventData{
 		Id: manifest.Id,
 	}
@@ -294,30 +297,141 @@ func TestNotifyClusterPluginEvent(t *testing.T) {
 		WaitForAllToSend: true,
 		Data:             expectedPluginData.ToJson(),
 	}
-	expectedMessages := findClusterMessages(model.CLUSTER_EVENT_INSTALL_PLUGIN, testCluster.GetMessages())
+	actualMessages := findClusterMessages(model.CLUSTER_EVENT_INSTALL_PLUGIN, messages)
-	require.Equal(t, []*model.ClusterMessage{expectedInstallMessage}, expectedMessages)
+	require.Equal(t, []*model.ClusterMessage{expectedInstallMessage}, actualMessages)
+
+	// Upgrade
+	testCluster.ClearMessages()
+	manifest, resp = th.SystemAdminClient.UploadPluginForced(bytes.NewReader(tarData))
+	CheckNoError(t, resp)
+	require.Equal(t, "testplugin", manifest.Id)
 
 	// Successful remove
 	testCluster.ClearMessages()
-
 	ok, resp := th.SystemAdminClient.RemovePlugin(manifest.Id)
 	CheckNoError(t, resp)
 	require.True(t, ok)
 
+	messages = testCluster.GetMessages()
+
 	expectedRemoveMessage := &model.ClusterMessage{
 		Event:            model.CLUSTER_EVENT_REMOVE_PLUGIN,
 		SendType:         model.CLUSTER_SEND_RELIABLE,
 		WaitForAllToSend: true,
 		Data:             expectedPluginData.ToJson(),
 	}
-	expectedMessages = findClusterMessages(model.CLUSTER_EVENT_REMOVE_PLUGIN, testCluster.GetMessages())
+	actualMessages = findClusterMessages(model.CLUSTER_EVENT_REMOVE_PLUGIN, messages)
-	require.Equal(t, []*model.ClusterMessage{expectedRemoveMessage}, expectedMessages)
+	require.Equal(t, []*model.ClusterMessage{expectedRemoveMessage}, actualMessages)
 
 	pluginStored, err = th.App.FileExists(expectedPath)
 	require.Nil(t, err)
 	require.False(t, pluginStored)
 }
 
+func TestDisableOnRemove(t *testing.T) {
+	path, _ := fileutils.FindDir("tests")
+	tarData, err := ioutil.ReadFile(filepath.Join(path, "testplugin.tar.gz"))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	testCases := []struct {
+		Description string
+		Upgrade     bool
+	}{
+		{
+			"Remove without upgrading",
+			false,
+		},
+		{
+			"Remove after upgrading",
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Description, func(t *testing.T) {
+			th := Setup().InitBasic()
+			defer th.TearDown()
+
+			th.App.UpdateConfig(func(cfg *model.Config) {
+				*cfg.PluginSettings.Enable = true
+				*cfg.PluginSettings.EnableUploads = true
+			})
+
+			// Upload
+			manifest, resp := th.SystemAdminClient.UploadPlugin(bytes.NewReader(tarData))
+			CheckNoError(t, resp)
+			require.Equal(t, "testplugin", manifest.Id)
+
+			// Check initial status
+			pluginsResp, resp := th.SystemAdminClient.GetPlugins()
+			CheckNoError(t, resp)
+			require.Len(t, pluginsResp.Active, 0)
+			require.Equal(t, pluginsResp.Inactive, []*model.PluginInfo{&model.PluginInfo{
+				Manifest: *manifest,
+			}})
+
+			// Enable plugin
+			ok, resp := th.SystemAdminClient.EnablePlugin(manifest.Id)
+			CheckNoError(t, resp)
+			require.True(t, ok)
+
+			// Confirm enabled status
+			pluginsResp, resp = th.SystemAdminClient.GetPlugins()
+			CheckNoError(t, resp)
+			require.Len(t, pluginsResp.Inactive, 0)
+			require.Equal(t, pluginsResp.Active, []*model.PluginInfo{&model.PluginInfo{
+				Manifest: *manifest,
+			}})
+
+			if tc.Upgrade {
+				// Upgrade
+				manifest, resp = th.SystemAdminClient.UploadPluginForced(bytes.NewReader(tarData))
+				CheckNoError(t, resp)
+				require.Equal(t, "testplugin", manifest.Id)
+
+				// Plugin should remain active
+				pluginsResp, resp = th.SystemAdminClient.GetPlugins()
+				CheckNoError(t, resp)
+				require.Len(t, pluginsResp.Inactive, 0)
+				require.Equal(t, pluginsResp.Active, []*model.PluginInfo{&model.PluginInfo{
+					Manifest: *manifest,
+				}})
+			}
+
+			// Remove plugin
+			ok, resp = th.SystemAdminClient.RemovePlugin(manifest.Id)
+			CheckNoError(t, resp)
+			require.True(t, ok)
+
+			// Plugin should have no status
+			pluginsResp, resp = th.SystemAdminClient.GetPlugins()
+			CheckNoError(t, resp)
+			require.Len(t, pluginsResp.Inactive, 0)
+			require.Len(t, pluginsResp.Active, 0)
+
+			// Upload same plugin
+			manifest, resp = th.SystemAdminClient.UploadPlugin(bytes.NewReader(tarData))
+			CheckNoError(t, resp)
+			require.Equal(t, "testplugin", manifest.Id)
+
+			// Plugin should be inactive
+			pluginsResp, resp = th.SystemAdminClient.GetPlugins()
+			CheckNoError(t, resp)
+			require.Len(t, pluginsResp.Active, 0)
+			require.Equal(t, pluginsResp.Inactive, []*model.PluginInfo{&model.PluginInfo{
+				Manifest: *manifest,
+			}})
+
+			// Clean up
+			ok, resp = th.SystemAdminClient.RemovePlugin(manifest.Id)
+			CheckNoError(t, resp)
+			require.True(t, ok)
+		})
+	}
+}
+
 func findClusterMessages(event string, msgs []*model.ClusterMessage) []*model.ClusterMessage {
 	var result []*model.ClusterMessage
 	for _, msg := range msgs {

app/plugin.go

@@ -14,6 +14,7 @@ import (
 	"github.com/mattermost/mattermost-server/plugin"
 	"github.com/mattermost/mattermost-server/services/filesstore"
 	"github.com/mattermost/mattermost-server/utils/fileutils"
+	"github.com/pkg/errors"
 )
 
 // GetPluginsEnvironment returns the plugin environment for use if plugins are enabled and
@@ -99,10 +100,11 @@ func (a *App) SyncPluginsActiveState() {
 					continue
 				}
 
-				if activated && updatedManifest.HasClient() {
+				if activated {
-					message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_PLUGIN_ENABLED, "", "", "", nil)
+					// Notify all cluster clients if ready
-					message.Add("manifest", updatedManifest.ClientManifest())
+					if err := a.notifyPluginEnabled(updatedManifest); err != nil {
-					a.Publish(message)
+						a.Log.Error("Failed to notify cluster on plugin enable", mlog.Err(err))
+					}
 				}
 			}
 		}
@@ -287,6 +289,7 @@ func (a *App) GetActivePluginManifests() ([]*model.Manifest, *model.AppError) {
 
 // EnablePlugin will set the config for an installed plugin to enabled, triggering asynchronous
 // activation if inactive anywhere in the cluster.
+// Notifies cluster peers through config change.
 func (a *App) EnablePlugin(id string) *model.AppError {
 	pluginsEnvironment := a.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {
@@ -316,7 +319,7 @@ func (a *App) EnablePlugin(id string) *model.AppError {
 		cfg.PluginSettings.PluginStates[id] = &model.PluginState{Enable: true}
 	})
 
-	// This call will cause SyncPluginsActiveState to be called and the plugin to be activated
+	// This call will implicitly invoke SyncPluginsActiveState which will activate enabled plugins.
 	if err := a.SaveConfig(a.Config(), true); err != nil {
 		if err.Id == "ent.cluster.save_config.error" {
 			return model.NewAppError("EnablePlugin", "app.plugin.cluster.save_config.app_error", nil, "", http.StatusInternalServerError)
@@ -328,6 +331,7 @@ func (a *App) EnablePlugin(id string) *model.AppError {
 }
 
 // DisablePlugin will set the config for an installed plugin to disabled, triggering deactivation if active.
+// Notifies cluster peers through config change.
 func (a *App) DisablePlugin(id string) *model.AppError {
 	pluginsEnvironment := a.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {
@@ -358,6 +362,7 @@ func (a *App) DisablePlugin(id string) *model.AppError {
 	})
 	a.UnregisterPluginCommands(id)
 
+	// This call will implicitly invoke SyncPluginsActiveState which will deactivate disabled plugins.
 	if err := a.SaveConfig(a.Config(), true); err != nil {
 		return model.NewAppError("DisablePlugin", "app.plugin.config.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
@@ -394,3 +399,54 @@ func (a *App) GetPlugins() (*model.PluginsResponse, *model.AppError) {
 
 	return resp, nil
 }
+
+// notifyPluginEnabled notifies connected websocket clients across all peers if the version of the given
+// plugin is same across them.
+//
+// When a peer finds itself in agreement with all other peers as to the version of the given plugin,
+// it will notify all connected websocket clients (across all peers) to trigger the (re-)installation.
+// There is a small chance that this never occurs, because the last server to finish installing dies before it can announce.
+// There is also a chance that multiple servers notify, but the webapp handles this idempotently.
+func (a *App) notifyPluginEnabled(manifest *model.Manifest) error {
+	pluginsEnvironment := a.GetPluginsEnvironment()
+	if pluginsEnvironment == nil {
+		return errors.New("pluginsEnvironment is nil")
+	}
+	if !manifest.HasClient() || !pluginsEnvironment.IsActive(manifest.Id) {
+		return nil
+	}
+
+	var statuses model.PluginStatuses
+
+	if a.Cluster != nil {
+		var err *model.AppError
+		statuses, err = a.Cluster.GetPluginStatuses()
+		if err != nil {
+			return err
+		}
+	}
+
+	localStatus, err := a.GetPluginStatus(manifest.Id)
+	if err != nil {
+		return err
+	}
+	statuses = append(statuses, localStatus)
+
+	// This will not guard against the race condition of enabling a plugin immediately after installation.
+	// As GetPluginStatuses() will not return the new plugin (since other peers are racing to install),
+	// this peer will end up checking status against itself and will notify all webclients (including peer webclients),
+	// which may result in a 404.
+	for _, status := range statuses {
+		if status.PluginId == manifest.Id && status.Version != manifest.Version {
+			mlog.Debug("Not ready to notify webclients", mlog.String("cluster_id", status.ClusterId), mlog.String("plugin_id", manifest.Id))
+			return nil
+		}
+	}
+
+	// Notify all cluster peer clients.
+	message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_PLUGIN_ENABLED, "", "", "", nil)
+	message.Add("manifest", manifest.ClientManifest())
+	a.Publish(message)
+
+	return nil
+}

app/plugin_install.go

@@ -1,6 +1,39 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
 
+// Installing a managed plugin consists of copying the uploaded plugin (*.tar.gz) to the filestore,
+// unpacking to the configured local directory (PluginSettings.Directory), and copying any webapp bundle therein
+// to the configured local client directory (PluginSettings.ClientDirectory). The unpacking and copy occurs
+// each time the server starts, ensuring it remains synchronized with the set of installed plugins.
+//
+// When a plugin is enabled, all connected websocket clients are notified so as to fetch any webapp bundle and
+// load the client-side portion of the plugin. This works well in a single-server system, but requires careful
+// coordination in a high-availability cluster with multiple servers. In particular, websocket clients must not be
+// notified of the newly enabled plugin until all servers in the cluster have finished unpacking the plugin, otherwise
+// the webapp bundle might not yet be available. Ideally, each server would just notify its own set of connected peers
+// after it finishes this process, but nothing prevents those clients from re-connecting to a different server behind
+// the load balancer that hasn't finished unpacking.
+//
+// To achieve this coordination, each server instead checks the status of its peers after unpacking. If it finds peers with
+// differing versions of the plugin, it skips the notification. If it finds all peers with the same version of the plugin,
+// it notifies all websocket clients connected to all peers. There's a small chance that this never occurs if the the last
+// server to finish unpacking dies before it can announce. There is also a chance that multiple servers decide to notify,
+// but the webapp handles this idempotently.
+//
+// Complicating this flow further are the various means of notifying. In addition to websocket events, there are cluster
+// messages between peers. There is a cluster message when the config changes and a plugin is enabled or disabled.
+// There is a cluster message when installing or uninstalling a plugin. There is a cluster message when peer's plugin change
+// its status. And finally the act of notifying websocket clients is propagated itself via a cluster message.
+//
+// The key methods involved in handling these notifications are notifyPluginEnabled and notifyPluginStatusesChanged.
+// Note that none of this complexity applies to single-server systems or to plugins without a webapp bundle.
+//
+// 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.
+//
 package app
 
 import (
@@ -34,9 +67,18 @@ func (a *App) InstallPluginFromData(data model.PluginEventData) {
 	}
 	defer reader.Close()
 
-	if _, appErr = a.installPluginLocally(reader, true); appErr != nil {
+	manifest, appErr := a.installPluginLocally(reader, true)
+	if appErr != nil {
 		mlog.Error("Failed to unpack plugin from filestore", mlog.Err(appErr), mlog.String("path", fileStorePath))
 	}
+
+	if err := a.notifyPluginEnabled(manifest); err != nil {
+		mlog.Error("Failed notify plugin enabled", mlog.Err(err))
+	}
+
+	if err := a.notifyPluginStatusesChanged(); err != nil {
+		mlog.Error("Failed to notify plugin status changed", mlog.Err(err))
+	}
 }
 
 func (a *App) RemovePluginFromData(data model.PluginEventData) {
@@ -45,6 +87,10 @@ func (a *App) RemovePluginFromData(data model.PluginEventData) {
 	if err := a.removePluginLocally(data.Id); err != nil {
 		mlog.Error("Failed to remove plugin locally", mlog.Err(err), mlog.String("id", data.Id))
 	}
+
+	if err := a.notifyPluginStatusesChanged(); err != nil {
+		mlog.Error("failed to notify plugin status changed", mlog.Err(err))
+	}
 }
 
 // InstallPlugin unpacks and installs a plugin but does not enable or activate it.
@@ -61,8 +107,8 @@ func (a *App) installPlugin(pluginFile io.ReadSeeker, replace bool) (*model.Mani
 	// Store bundle in the file store to allow access from other servers.
 	pluginFile.Seek(0, 0)
 
-	if _, err := a.WriteFile(pluginFile, a.getBundleStorePath(manifest.Id)); err != nil {
+	if _, appErr := a.WriteFile(pluginFile, a.getBundleStorePath(manifest.Id)); appErr != nil {
-		return nil, model.NewAppError("uploadPlugin", "app.plugin.store_bundle.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, model.NewAppError("uploadPlugin", "app.plugin.store_bundle.app_error", nil, appErr.Error(), http.StatusInternalServerError)
 	}
 
 	a.notifyClusterPluginEvent(
@@ -72,29 +118,37 @@ func (a *App) installPlugin(pluginFile io.ReadSeeker, replace bool) (*model.Mani
 		},
 	)
 
+	if err := a.notifyPluginEnabled(manifest); err != nil {
+		mlog.Error("Failed notify plugin enabled", mlog.Err(err))
+	}
+
+	if err := a.notifyPluginStatusesChanged(); err != nil {
+		mlog.Error("Failed to notify plugin status changed", mlog.Err(err))
+	}
+
 	return manifest, nil
 }
 
 func (a *App) installPluginLocally(pluginFile io.ReadSeeker, replace bool) (*model.Manifest, *model.AppError) {
 	pluginsEnvironment := a.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {
-		return nil, model.NewAppError("installPlugin", "app.plugin.disabled.app_error", nil, "", http.StatusNotImplemented)
+		return nil, model.NewAppError("installPluginLocally", "app.plugin.disabled.app_error", nil, "", http.StatusNotImplemented)
 	}
 
 	tmpDir, err := ioutil.TempDir("", "plugintmp")
 	if err != nil {
-		return nil, model.NewAppError("installPlugin", "app.plugin.filesystem.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, model.NewAppError("installPluginLocally", "app.plugin.filesystem.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 	defer os.RemoveAll(tmpDir)
 
 	if err = utils.ExtractTarGz(pluginFile, tmpDir); err != nil {
-		return nil, model.NewAppError("installPlugin", "app.plugin.extract.app_error", nil, err.Error(), http.StatusBadRequest)
+		return nil, model.NewAppError("installPluginLocally", "app.plugin.extract.app_error", nil, err.Error(), http.StatusBadRequest)
 	}
 
 	tmpPluginDir := tmpDir
 	dir, err := ioutil.ReadDir(tmpDir)
 	if err != nil {
-		return nil, model.NewAppError("installPlugin", "app.plugin.filesystem.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, model.NewAppError("installPluginLocally", "app.plugin.filesystem.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
 	if len(dir) == 1 && dir[0].IsDir() {
@@ -103,30 +157,27 @@ func (a *App) installPluginLocally(pluginFile io.ReadSeeker, replace bool) (*mod
 
 	manifest, _, err := model.FindManifest(tmpPluginDir)
 	if err != nil {
-		return nil, model.NewAppError("installPlugin", "app.plugin.manifest.app_error", nil, err.Error(), http.StatusBadRequest)
+		return nil, model.NewAppError("installPluginLocally", "app.plugin.manifest.app_error", nil, err.Error(), http.StatusBadRequest)
 	}
 
 	if !plugin.IsValidId(manifest.Id) {
-		return nil, model.NewAppError("installPlugin", "app.plugin.invalid_id.app_error", map[string]interface{}{"Min": plugin.MinIdLength, "Max": plugin.MaxIdLength, "Regex": plugin.ValidIdRegex}, "", http.StatusBadRequest)
+		return nil, model.NewAppError("installPluginLocally", "app.plugin.invalid_id.app_error", map[string]interface{}{"Min": plugin.MinIdLength, "Max": plugin.MaxIdLength, "Regex": plugin.ValidIdRegex}, "", http.StatusBadRequest)
 	}
 
-	// Stash the previous state of the plugin, if available
-	stashed := a.Config().PluginSettings.PluginStates[manifest.Id]
-
 	bundles, err := pluginsEnvironment.Available()
 	if err != nil {
-		return nil, model.NewAppError("installPlugin", "app.plugin.install.app_error", nil, err.Error(), http.StatusInternalServerError)
+		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
 	for _, bundle := range bundles {
 		if bundle.Manifest != nil && bundle.Manifest.Id == manifest.Id {
 			if !replace {
-				return nil, model.NewAppError("installPlugin", "app.plugin.install_id.app_error", nil, "", http.StatusBadRequest)
+				return nil, model.NewAppError("installPluginLocally", "app.plugin.install_id.app_error", nil, "", http.StatusBadRequest)
 			}
 
 			if err := a.removePluginLocally(manifest.Id); err != nil {
-				return nil, model.NewAppError("installPlugin", "app.plugin.install_id_failed_remove.app_error", nil, "", http.StatusBadRequest)
+				return nil, model.NewAppError("installPluginLocally", "app.plugin.install_id_failed_remove.app_error", nil, "", http.StatusBadRequest)
 			}
 		}
 	}
@@ -134,22 +185,32 @@ func (a *App) installPluginLocally(pluginFile io.ReadSeeker, replace bool) (*mod
 	pluginPath := filepath.Join(*a.Config().PluginSettings.Directory, manifest.Id)
 	err = utils.CopyDir(tmpPluginDir, pluginPath)
 	if err != nil {
-		return nil, model.NewAppError("installPlugin", "app.plugin.mvdir.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, model.NewAppError("installPluginLocally", "app.plugin.mvdir.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
 	// Flag plugin locally as managed by the filestore.
 	f, err := os.Create(filepath.Join(pluginPath, managedPluginFileName))
 	if err != nil {
-		return nil, model.NewAppError("uploadPlugin", "app.plugin.flag_managed.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, model.NewAppError("installPluginLocally", "app.plugin.flag_managed.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 	f.Close()
 
-	if stashed != nil && stashed.Enable {
+	if manifest.HasWebapp() {
-		a.EnablePlugin(manifest.Id)
+		updatedManifest, err := pluginsEnvironment.UnpackWebappBundle(manifest.Id)
+		if err != nil {
+			return nil, model.NewAppError("installPluginLocally", "app.plugin.webapp_bundle.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
+		manifest = updatedManifest
 	}
 
-	if err := a.notifyPluginStatusesChanged(); err != nil {
+	// Activate plugin if it was previously activated.
-		mlog.Error("failed to notify plugin status changed", mlog.Err(err))
+	pluginState := a.Config().PluginSettings.PluginStates[manifest.Id]
+	if pluginState != nil && pluginState.Enable {
+		updatedManifest, _, err := pluginsEnvironment.Activate(manifest.Id)
+		if err != nil {
+			return nil, model.NewAppError("installPluginLocally", "app.plugin.restart.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
+		manifest = updatedManifest
 	}
 
 	return manifest, nil
@@ -160,6 +221,12 @@ func (a *App) RemovePlugin(id string) *model.AppError {
 }
 
 func (a *App) removePlugin(id string) *model.AppError {
+	// Disable plugin before removal to make sure this
+	// plugin remains disabled on re-install.
+	if err := a.DisablePlugin(id); err != nil {
+		return err
+	}
+
 	if err := a.removePluginLocally(id); err != nil {
 		return err
 	}
@@ -212,18 +279,11 @@ func (a *App) removePluginLocally(id string) *model.AppError {
 		return model.NewAppError("removePlugin", "app.plugin.not_installed.app_error", nil, "", http.StatusBadRequest)
 	}
 
-	if pluginsEnvironment.IsActive(id) && manifest.HasClient() {
-		message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_PLUGIN_DISABLED, "", "", "", nil)
-		message.Add("manifest", manifest.ClientManifest())
-		a.Publish(message)
-	}
-
 	pluginsEnvironment.Deactivate(id)
 	pluginsEnvironment.RemovePlugin(id)
 	a.UnregisterPluginCommands(id)
 
-	err = os.RemoveAll(pluginPath)
+	if err := os.RemoveAll(pluginPath); err != nil {
-	if err != nil {
 		return model.NewAppError("removePlugin", "app.plugin.remove.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 

i18n/en.json

@@ -3506,6 +3506,10 @@
     "id": "app.plugin.remove_bundle.app_error",
     "translation": "Unable to remove plugin bundle from file store."
   },
+  {
+    "id": "app.plugin.restart.app_error",
+    "translation": "Unable to restart plugin on upgrade."
+  },
   {
     "id": "app.plugin.store_bundle.app_error",
     "translation": "Unable to store the plugin to the configured file store."
@@ -3522,6 +3526,10 @@
     "id": "app.plugin.upload_disabled.app_error",
     "translation": "Plugins and/or plugin uploads have been disabled."
   },
+  {
+    "id": "app.plugin.webapp_bundle.app_error",
+    "translation": "Unable to generate plugin webapp bundle."
+  },
   {
     "id": "app.role.check_roles_exist.role_not_found",
     "translation": "The provided role does not exist"

plugin/environment.go

@@ -217,38 +217,11 @@ func (env *Environment) Activate(id string) (manifest *model.Manifest, activated
 	componentActivated := false
 
 	if pluginInfo.Manifest.HasWebapp() {
-		bundlePath := filepath.Clean(pluginInfo.Manifest.Webapp.BundlePath)
+		updatedManifest, err := env.UnpackWebappBundle(id)
-		if bundlePath == "" || bundlePath[0] == '.' {
-			return nil, false, fmt.Errorf("invalid webapp bundle path")
-		}
-		bundlePath = filepath.Join(env.pluginDir, id, bundlePath)
-		destinationPath := filepath.Join(env.webappPluginDir, id)
-
-		if err := os.RemoveAll(destinationPath); err != nil {
-			return nil, false, errors.Wrapf(err, "unable to remove old webapp bundle directory: %v", destinationPath)
-		}
-
-		if err := utils.CopyDir(filepath.Dir(bundlePath), destinationPath); err != nil {
-			return nil, false, errors.Wrapf(err, "unable to copy webapp bundle directory: %v", id)
-		}
-
-		sourceBundleFilepath := filepath.Join(destinationPath, filepath.Base(bundlePath))
-
-		sourceBundleFileContents, err := ioutil.ReadFile(sourceBundleFilepath)
 		if err != nil {
-			return nil, false, errors.Wrapf(err, "unable to read webapp bundle: %v", id)
+			return nil, false, errors.Wrapf(err, "unable to generate webapp bundle: %v", id)
-		}
-
-		hash := fnv.New64a()
-		hash.Write(sourceBundleFileContents)
-		pluginInfo.Manifest.Webapp.BundleHash = hash.Sum([]byte{})
-
-		if err := os.Rename(
-			sourceBundleFilepath,
-			filepath.Join(destinationPath, fmt.Sprintf("%s_%x_bundle.js", id, pluginInfo.Manifest.Webapp.BundleHash)),
-		); err != nil {
-			return nil, false, errors.Wrapf(err, "unable to rename webapp bundle: %v", id)
 		}
+		pluginInfo.Manifest.Webapp.BundleHash = updatedManifest.Webapp.BundleHash
 
 		componentActivated = true
 	}
@@ -327,6 +300,63 @@ func (env *Environment) Shutdown() {
 	})
 }
 
+// UnpackWebappBundle unpacks webapp bundle for a given plugin id on disk.
+func (env *Environment) UnpackWebappBundle(id string) (*model.Manifest, error) {
+	plugins, err := env.Available()
+	if err != nil {
+		return nil, errors.New("Unable to get available plugins")
+	}
+	var manifest *model.Manifest
+	for _, p := range plugins {
+		if p.Manifest != nil && p.Manifest.Id == id {
+			if manifest != nil {
+				return nil, fmt.Errorf("multiple plugins found: %v", id)
+			}
+			manifest = p.Manifest
+		}
+	}
+	if manifest == nil {
+		return nil, fmt.Errorf("plugin not found: %v", id)
+	}
+
+	bundlePath := filepath.Clean(manifest.Webapp.BundlePath)
+	if bundlePath == "" || bundlePath[0] == '.' {
+		return nil, fmt.Errorf("invalid webapp bundle path")
+	}
+	bundlePath = filepath.Join(env.pluginDir, id, bundlePath)
+	destinationPath := filepath.Join(env.webappPluginDir, id)
+
+	if err = os.RemoveAll(destinationPath); err != nil {
+		return nil, errors.Wrapf(err, "unable to remove old webapp bundle directory: %v", destinationPath)
+	}
+
+	if err = utils.CopyDir(filepath.Dir(bundlePath), destinationPath); err != nil {
+		return nil, errors.Wrapf(err, "unable to copy webapp bundle directory: %v", id)
+	}
+
+	sourceBundleFilepath := filepath.Join(destinationPath, filepath.Base(bundlePath))
+
+	sourceBundleFileContents, err := ioutil.ReadFile(sourceBundleFilepath)
+	if err != nil {
+		return nil, errors.Wrapf(err, "unable to read webapp bundle: %v", id)
+	}
+
+	hash := fnv.New64a()
+	if _, err = hash.Write(sourceBundleFileContents); err != nil {
+		return nil, errors.Wrapf(err, "unable to generate hash for webapp bundle: %v", id)
+	}
+	manifest.Webapp.BundleHash = hash.Sum([]byte{})
+
+	if err = os.Rename(
+		sourceBundleFilepath,
+		filepath.Join(destinationPath, fmt.Sprintf("%s_%x_bundle.js", id, manifest.Webapp.BundleHash)),
+	); err != nil {
+		return nil, errors.Wrapf(err, "unable to rename webapp bundle: %v", id)
+	}
+
+	return manifest, nil
+}
+
 // HooksForPlugin returns the hooks API for the plugin with the given id.
 //
 // Consider using RunMultiPluginHook instead.