Merge branch 'master' into mark-as-unread

api4/plugin.go

@@ -10,6 +10,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/url"
+	"time"
 
 	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
@@ -17,6 +18,9 @@ import (
 
 const (
 	MAXIMUM_PLUGIN_FILE_SIZE = 50 * 1024 * 1024
+	// INSTALL_PLUGIN_FROM_URL_HTTP_REQUEST_TIMEOUT defines a high timeout for installing plugins
+	// from an external URL to avoid slow connections or large plugins from failing to install.
+	INSTALL_PLUGIN_FROM_URL_HTTP_REQUEST_TIMEOUT = 60 * time.Minute
 )
 
 func (api *API) InitPlugin() {
@@ -115,6 +119,8 @@ func installPluginFromUrl(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 
 	client := c.App.HTTPService.MakeClient(true)
+	client.Timeout = INSTALL_PLUGIN_FROM_URL_HTTP_REQUEST_TIMEOUT
+
 	resp, err := client.Get(downloadUrl)
 	if err != nil {
 		c.Err = model.NewAppError("installPluginFromUrl", "api.plugin.install.download_failed.app_error", nil, err.Error(), http.StatusBadRequest)

api4/plugin_test.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/testlib"
@@ -59,6 +60,24 @@ func TestPlugin(t *testing.T) {
 	CheckNoError(t, resp)
 	assert.Equal(t, "testplugin", manifest.Id)
 
+	t.Run("install plugin from URL with slow response time", func(t *testing.T) {
+		if testing.Short() {
+			t.Skip("skipping test to install plugin from a slow response server")
+		}
+
+		// Install from URL - slow server to simulate longer bundle download times
+		slowTestServer := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) {
+			time.Sleep(60 * time.Second) // Wait longer than the previous default 30 seconds timeout
+			res.WriteHeader(http.StatusOK)
+			res.Write(tarData)
+		}))
+		defer func() { slowTestServer.Close() }()
+
+		manifest, resp = th.SystemAdminClient.InstallPluginFromUrl(slowTestServer.URL, true)
+		CheckNoError(t, resp)
+		assert.Equal(t, "testplugin", manifest.Id)
+	})
+
 	// Stored in File Store: Install Plugin from URL case
 	pluginStored, err := th.App.FileExists("./plugins/" + manifest.Id + ".tar.gz")
 	assert.Nil(t, err)
@@ -255,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)
@@ -266,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,
 	}
@@ -275,30 +297,141 @@ func TestNotifyClusterPluginEvent(t *testing.T) {
 		WaitForAllToSend: true,
 		Data:             expectedPluginData.ToJson(),
 	}
-	expectedMessages := findClusterMessages(model.CLUSTER_EVENT_INSTALL_PLUGIN, testCluster.GetMessages())
-	require.Equal(t, []*model.ClusterMessage{expectedInstallMessage}, expectedMessages)
+	actualMessages := findClusterMessages(model.CLUSTER_EVENT_INSTALL_PLUGIN, messages)
+	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())
-	require.Equal(t, []*model.ClusterMessage{expectedRemoveMessage}, expectedMessages)
+	actualMessages = findClusterMessages(model.CLUSTER_EVENT_REMOVE_PLUGIN, messages)
+	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 {

api4/system.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net/http"
 	"runtime"
+	"time"
 
 	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
@@ -64,11 +65,32 @@ func getSystemPing(c *Context, w http.ResponseWriter, r *http.Request) {
 	if r.FormValue("get_server_status") != "" {
 		dbStatusKey := "database_status"
 		s[dbStatusKey] = model.STATUS_OK
-		_, appErr := c.App.Srv.Store.System().Get()
-		if appErr != nil {
-			mlog.Debug(fmt.Sprintf("Unable to get database status: %s", appErr.Error()))
+
+		// Database Write/Read Check
+		currentTime := fmt.Sprintf("%d", time.Now().Unix())
+		healthCheckKey := "health_check"
+
+		writeErr := c.App.Srv.Store.System().SaveOrUpdate(&model.System{
+			Name:  healthCheckKey,
+			Value: currentTime,
+		})
+		if writeErr != nil {
+			mlog.Debug(fmt.Sprintf("Unable to write to database: %s", writeErr.Error()))
 			s[dbStatusKey] = model.STATUS_UNHEALTHY
 			s[model.STATUS] = model.STATUS_UNHEALTHY
+		} else {
+			healthCheck, readErr := c.App.Srv.Store.System().GetByName(healthCheckKey)
+			if readErr != nil {
+				mlog.Debug(fmt.Sprintf("Unable to read from database: %s", readErr.Error()))
+				s[dbStatusKey] = model.STATUS_UNHEALTHY
+				s[model.STATUS] = model.STATUS_UNHEALTHY
+			} else if healthCheck.Value != currentTime {
+				mlog.Debug(fmt.Sprintf("Incorrect healthcheck value, expected %s, got %s", currentTime, healthCheck.Value))
+				s[dbStatusKey] = model.STATUS_UNHEALTHY
+				s[model.STATUS] = model.STATUS_UNHEALTHY
+			} else {
+				mlog.Debug("Able to write/read files to database")
+			}
 		}
 
 		filestoreStatusKey := "filestore_status"

app/notification.go

@@ -640,7 +640,7 @@ type postNotification struct {
 func (n *postNotification) GetChannelName(userNameFormat string, excludeId string) string {
 	switch n.channel.Type {
 	case model.CHANNEL_DIRECT:
-		return n.sender.GetDisplayName(userNameFormat)
+		return n.sender.GetDisplayNameWithPrefix(userNameFormat, "@")
 	case model.CHANNEL_GROUP:
 		names := []string{}
 		for _, user := range n.profileMap {
@@ -670,7 +670,7 @@ func (n *postNotification) GetSenderName(userNameFormat string, overridesAllowed
 		}
 	}
 
-	return n.sender.GetDisplayName(userNameFormat)
+	return n.sender.GetDisplayNameWithPrefix(userNameFormat, "@")
 }
 
 // addMentionedUsers will add the mentioned user id in the struct's list for mentioned users

app/notification_test.go

@@ -1207,12 +1207,12 @@ func TestPostNotificationGetChannelName(t *testing.T) {
 		},
 		"direct channel, unspecified": {
 			channel:  &model.Channel{Type: model.CHANNEL_DIRECT},
-			expected: "sender",
+			expected: "@sender",
 		},
 		"direct channel, username": {
 			channel:    &model.Channel{Type: model.CHANNEL_DIRECT},
 			nameFormat: model.SHOW_USERNAME,
-			expected:   "sender",
+			expected:   "@sender",
 		},
 		"direct channel, full name": {
 			channel:    &model.Channel{Type: model.CHANNEL_DIRECT},
@@ -1290,11 +1290,11 @@ func TestPostNotificationGetSenderName(t *testing.T) {
 		expected       string
 	}{
 		"name format unspecified": {
-			expected: sender.Username,
+			expected: "@" + sender.Username,
 		},
 		"name format username": {
 			nameFormat: model.SHOW_USERNAME,
-			expected:   sender.Username,
+			expected:   "@" + sender.Username,
 		},
 		"name format full name": {
 			nameFormat: model.SHOW_FULLNAME,
@@ -1317,12 +1317,12 @@ func TestPostNotificationGetSenderName(t *testing.T) {
 			channel:        &model.Channel{Type: model.CHANNEL_DIRECT},
 			post:           overriddenPost,
 			allowOverrides: true,
-			expected:       sender.Username,
+			expected:       "@" + sender.Username,
 		},
 		"overridden username, overrides disabled": {
 			post:           overriddenPost,
 			allowOverrides: false,
-			expected:       sender.Username,
+			expected:       "@" + sender.Username,
 		},
 	} {
 		t.Run(name, func(t *testing.T) {

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() {
-					message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_PLUGIN_ENABLED, "", "", "", nil)
-					message.Add("manifest", updatedManifest.ClientManifest())
-					a.Publish(message)
+				if activated {
+					// Notify all cluster clients if ready
+					if err := a.notifyPluginEnabled(updatedManifest); err != nil {
+						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 {
-		return nil, model.NewAppError("uploadPlugin", "app.plugin.store_bundle.app_error", nil, err.Error(), http.StatusInternalServerError)
+	if _, appErr := a.WriteFile(pluginFile, a.getBundleStorePath(manifest.Id)); appErr != nil {
+		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 {
-		a.EnablePlugin(manifest.Id)
+	if manifest.HasWebapp() {
+		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 {
-		mlog.Error("failed to notify plugin status changed", mlog.Err(err))
+	// Activate plugin if it was previously activated.
+	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 != nil {
+	if err := os.RemoveAll(pluginPath); err != nil {
 		return model.NewAppError("removePlugin", "app.plugin.remove.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 

app/post_metadata.go

@@ -176,7 +176,10 @@ func (a *App) getEmbedForPost(post *model.Post, firstLink string, isNewPost bool
 		}, nil
 	}
 
-	return nil, nil
+	return &model.PostEmbed{
+		Type: model.POST_EMBED_LINK,
+		URL:  firstLink,
+	}, nil
 }
 
 func (a *App) getImagesForPost(post *model.Post, imageURLs []string, isNewPost bool) map[string]*model.PostImage {

app/post_metadata_test.go

@@ -536,6 +536,13 @@ func TestGetEmbedForPost(t *testing.T) {
 
 			w.Header().Set("Content-Type", "image/png")
 			w.Write(file)
+		} else if r.URL.Path == "/other" {
+			w.Header().Set("Content-Type", "text/html")
+			w.Write([]byte(`
+			<html>
+			<head>
+			</head>
+			</html>`))
 		} else {
 			t.Fatal("Invalid path", r.URL.Path)
 		}
@@ -544,6 +551,7 @@ func TestGetEmbedForPost(t *testing.T) {
 
 	ogURL := server.URL + "/index.html"
 	imageURL := server.URL + "/image.png"
+	otherURL := server.URL + "/other"
 
 	t.Run("with link previews enabled", func(t *testing.T) {
 		th := Setup(t)
@@ -593,6 +601,16 @@ func TestGetEmbedForPost(t *testing.T) {
 			}, embed)
 			assert.Nil(t, err)
 		})
+
+		t.Run("should return a link embed", func(t *testing.T) {
+			embed, err := th.App.getEmbedForPost(&model.Post{}, otherURL, false)
+
+			assert.Equal(t, &model.PostEmbed{
+				Type: model.POST_EMBED_LINK,
+				URL:  otherURL,
+			}, embed)
+			assert.Nil(t, err)
+		})
 	})
 
 	t.Run("with link previews disabled", func(t *testing.T) {
@@ -634,6 +652,13 @@ func TestGetEmbedForPost(t *testing.T) {
 			assert.Nil(t, embed)
 			assert.Nil(t, err)
 		})
+
+		t.Run("should not return a link embed", func(t *testing.T) {
+			embed, err := th.App.getEmbedForPost(&model.Post{}, otherURL, false)
+
+			assert.Nil(t, embed)
+			assert.Nil(t, err)
+		})
 	})
 }
 

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"

model/client4.go

@@ -2898,7 +2898,7 @@ func (c *Client4) GetPing() (string, *Response) {
 }
 
 // GetPingWithServerStatus will return ok if several basic server health checks
-// all psss successfully.
+// all pass successfully.
 func (c *Client4) GetPingWithServerStatus() (string, *Response) {
 	r, err := c.DoApiGet(c.GetSystemRoute()+"/ping?get_server_status=true", "")
 	if r != nil && r.StatusCode == 500 {

model/post_embed.go

@@ -7,6 +7,7 @@ const (
 	POST_EMBED_IMAGE              PostEmbedType = "image"
 	POST_EMBED_MESSAGE_ATTACHMENT PostEmbedType = "message_attachment"
 	POST_EMBED_OPENGRAPH          PostEmbedType = "opengraph"
+	POST_EMBED_LINK               PostEmbedType = "link"
 )
 
 type PostEmbedType string

model/user.go

@@ -539,8 +539,8 @@ func (u *User) GetFullName() string {
 	}
 }
 
-func (u *User) GetDisplayName(nameFormat string) string {
-	displayName := u.Username
+func (u *User) getDisplayName(baseName, nameFormat string) string {
+	displayName := baseName
 
 	if nameFormat == SHOW_NICKNAME_FULLNAME {
 		if len(u.Nickname) > 0 {
@@ -557,6 +557,18 @@ func (u *User) GetDisplayName(nameFormat string) string {
 	return displayName
 }
 
+func (u *User) GetDisplayName(nameFormat string) string {
+	displayName := u.Username
+
+	return u.getDisplayName(displayName, nameFormat)
+}
+
+func (u *User) GetDisplayNameWithPrefix(nameFormat, prefix string) string {
+	displayName := prefix + u.Username
+
+	return u.getDisplayName(displayName, nameFormat)
+}
+
 func (u *User) GetRoles() []string {
 	return strings.Fields(u.Roles)
 }

model/user_test.go

@@ -252,6 +252,42 @@ func TestUserGetDisplayName(t *testing.T) {
 	}
 }
 
+func TestUserGetDisplayNameWithPrefix(t *testing.T) {
+	user := User{Username: "username"}
+
+	if displayName := user.GetDisplayNameWithPrefix(SHOW_FULLNAME, "@"); displayName != "@username" {
+		t.Fatal("Display name should be username")
+	}
+
+	if displayName := user.GetDisplayNameWithPrefix(SHOW_NICKNAME_FULLNAME, "@"); displayName != "@username" {
+		t.Fatal("Display name should be username")
+	}
+
+	if displayName := user.GetDisplayNameWithPrefix(SHOW_USERNAME, "@"); displayName != "@username" {
+		t.Fatal("Display name should be username")
+	}
+
+	user.FirstName = "first"
+	user.LastName = "last"
+
+	if displayName := user.GetDisplayNameWithPrefix(SHOW_FULLNAME, "@"); displayName != "first last" {
+		t.Fatal("Display name should be full name")
+	}
+
+	if displayName := user.GetDisplayNameWithPrefix(SHOW_NICKNAME_FULLNAME, "@"); displayName != "first last" {
+		t.Fatal("Display name should be full name since there is no nickname")
+	}
+
+	if displayName := user.GetDisplayNameWithPrefix(SHOW_USERNAME, "@"); displayName != "@username" {
+		t.Fatal("Display name should be username")
+	}
+
+	user.Nickname = "nickname"
+	if displayName := user.GetDisplayNameWithPrefix(SHOW_NICKNAME_FULLNAME, "@"); displayName != "nickname" {
+		t.Fatal("Display name should be nickname")
+	}
+}
+
 var usernames = []struct {
 	value    string
 	expected bool

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)
-		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)
+		updatedManifest, err := env.UnpackWebappBundle(id)
 		if err != nil {
-			return nil, false, errors.Wrapf(err, "unable to read 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)
+			return nil, false, errors.Wrapf(err, "unable to generate 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.