MM-11905: delete plugin commands on removal (#9601)

* defer plugin tear down for testing

* test expected plugin command unregistration

* MM-11905: uninstall plugin commands on remove

3 files changed, 147 insertions, 21 deletions

diff --git a/app/plugin_commands_test.go b/app/plugin_commands_test.go
new file mode 100644
index 000000000..cc9184277
--- /dev/null
+++ b/app/plugin_commands_test.go
@@ -0,0 +1,104 @@
+// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package app
+
+import (
+	"testing"
+
+	"github.com/mattermost/mattermost-server/model"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPluginCommand(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	args := &model.CommandArgs{}
+	args.TeamId = th.BasicTeam.Id
+	args.ChannelId = th.BasicChannel.Id
+	args.UserId = th.BasicUser.Id
+	args.Command = "/plugin"
+
+	t.Run("error before plugin command registered", func(t *testing.T) {
+		_, err := th.App.ExecuteCommand(args)
+		require.NotNil(t, err)
+	})
+
+	t.Run("command handled by plugin", func(t *testing.T) {
+		th.App.UpdateConfig(func(cfg *model.Config) {
+			cfg.PluginSettings.Plugins["testloadpluginconfig"] = map[string]interface{}{
+				"TeamId": args.TeamId,
+			}
+		})
+
+		tearDown, pluginIds, activationErrors := SetAppEnvironmentWithPlugins(t, []string{`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type configuration struct {
+				TeamId string
+			}
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin	
+
+				configuration configuration
+			}
+
+			func (p *MyPlugin) OnConfigurationChange() error {
+				p.API.LogError("hello")
+				if err := p.API.LoadPluginConfiguration(&p.configuration); err != nil {
+					return err
+				}
+
+				return nil
+			}
+
+			func (p *MyPlugin) OnActivate() error {
+				p.API.LogError("team", "team", p.configuration.TeamId)
+				err := p.API.RegisterCommand(&model.Command{
+					TeamId: p.configuration.TeamId,
+					Trigger: "plugin",
+					DisplayName: "Plugin Command",
+				})
+				if err != nil {
+					p.API.LogError("error", "err", err)
+				}
+				p.API.LogDebug("team", "team", p.configuration.TeamId)
+
+				return err
+			}
+
+			func (p *MyPlugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) {
+				return &model.CommandResponse{
+					ResponseType: model.COMMAND_RESPONSE_TYPE_EPHEMERAL,
+					Text: "text",
+				}, nil
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+		`}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
+		require.Len(t, activationErrors, 1)
+		require.Nil(t, nil, activationErrors[0])
+
+		resp, err := th.App.ExecuteCommand(args)
+		require.Nil(t, err)
+		require.Equal(t, model.COMMAND_RESPONSE_TYPE_EPHEMERAL, resp.ResponseType)
+		require.Equal(t, "text", resp.Text)
+
+		th.App.RemovePlugin(pluginIds[0])
+	})
+
+	t.Run("error after plugin command unregistered", func(t *testing.T) {
+		_, err := th.App.ExecuteCommand(args)
+		require.NotNil(t, err)
+	})
+}
diff --git a/app/plugin_hooks_test.go b/app/plugin_hooks_test.go
index f2acd73e4..766385af4 100644
--- a/app/plugin_hooks_test.go
+++ b/app/plugin_hooks_test.go
@@ -35,18 +35,17 @@ func compileGo(t *testing.T, sourceCode, outputPath string) {
 	require.NoError(t, cmd.Run())
 }
 
-func SetAppEnvironmentWithPlugins(t *testing.T, pluginCode []string, app *App, apiFunc func(*model.Manifest) plugin.API) []error {
+func SetAppEnvironmentWithPlugins(t *testing.T, pluginCode []string, app *App, apiFunc func(*model.Manifest) plugin.API) (func(), []string, []error) {
 	pluginDir, err := ioutil.TempDir("", "")
 	require.NoError(t, err)
 	webappPluginDir, err := ioutil.TempDir("", "")
 	require.NoError(t, err)
-	defer os.RemoveAll(pluginDir)
-	defer os.RemoveAll(webappPluginDir)
 
 	env, err := plugin.NewEnvironment(apiFunc, pluginDir, webappPluginDir, app.Log)
 	require.NoError(t, err)
 
 	app.Plugins = env
+	pluginIds := []string{}
 	activationErrors := []error{}
 	for _, code := range pluginCode {
 		pluginId := model.NewId()
@@ -55,10 +54,14 @@ func SetAppEnvironmentWithPlugins(t *testing.T, pluginCode []string, app *App, a
 
 		ioutil.WriteFile(filepath.Join(pluginDir, pluginId, "plugin.json"), []byte(`{"id": "`+pluginId+`", "backend": {"executable": "backend.exe"}}`), 0600)
 		_, _, activationErr := env.Activate(pluginId)
+		pluginIds = append(pluginIds, pluginId)
 		activationErrors = append(activationErrors, activationErr)
 	}
 
-	return activationErrors
+	return func() {
+		os.RemoveAll(pluginDir)
+		os.RemoveAll(webappPluginDir)
+	}, pluginIds, activationErrors
 }
 
 func TestHookMessageWillBePosted(t *testing.T) {
@@ -66,7 +69,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 		th := Setup().InitBasic()
 		defer th.TearDown()
 
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -88,6 +91,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 			}
 			`,
 		}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
 
 		post := &model.Post{
 			UserId:    th.BasicUser.Id,
@@ -105,7 +109,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 		th := Setup().InitBasic()
 		defer th.TearDown()
 
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -128,6 +132,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 			}
 			`,
 		}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
 
 		post := &model.Post{
 			UserId:    th.BasicUser.Id,
@@ -145,7 +150,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 		th := Setup().InitBasic()
 		defer th.TearDown()
 
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -167,6 +172,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 			}
 			`,
 		}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
 
 		post := &model.Post{
 			UserId:    th.BasicUser.Id,
@@ -190,7 +196,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 		th := Setup().InitBasic()
 		defer th.TearDown()
 
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -213,6 +219,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 			}
 			`,
 		}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
 
 		post := &model.Post{
 			UserId:    th.BasicUser.Id,
@@ -236,7 +243,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 		th := Setup().InitBasic()
 		defer th.TearDown()
 
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -281,6 +288,7 @@ func TestHookMessageWillBePosted(t *testing.T) {
 			}
 			`,
 		}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
 
 		post := &model.Post{
 			UserId:    th.BasicUser.Id,
@@ -304,7 +312,7 @@ func TestHookMessageHasBeenPosted(t *testing.T) {
 	mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
 	mockAPI.On("LogDebug", "message").Return(nil)
 
-	SetAppEnvironmentWithPlugins(t,
+	tearDown, _, _ := SetAppEnvironmentWithPlugins(t,
 		[]string{
 			`
 		package main
@@ -326,6 +334,7 @@ func TestHookMessageHasBeenPosted(t *testing.T) {
 			plugin.ClientMain(&MyPlugin{})
 		}
 	`}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+	defer tearDown()
 
 	post := &model.Post{
 		UserId:    th.BasicUser.Id,
@@ -343,7 +352,7 @@ func TestHookMessageWillBeUpdated(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
-	SetAppEnvironmentWithPlugins(t,
+	tearDown, _, _ := SetAppEnvironmentWithPlugins(t,
 		[]string{
 			`
 		package main
@@ -366,6 +375,7 @@ func TestHookMessageWillBeUpdated(t *testing.T) {
 			plugin.ClientMain(&MyPlugin{})
 		}
 	`}, th.App, th.App.NewPluginAPI)
+	defer tearDown()
 
 	post := &model.Post{
 		UserId:    th.BasicUser.Id,
@@ -394,7 +404,7 @@ func TestHookMessageHasBeenUpdated(t *testing.T) {
 	mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
 	mockAPI.On("LogDebug", "message_edited").Return(nil)
 	mockAPI.On("LogDebug", "message_").Return(nil)
-	SetAppEnvironmentWithPlugins(t,
+	tearDown, _, _ := SetAppEnvironmentWithPlugins(t,
 		[]string{
 			`
 		package main
@@ -417,6 +427,7 @@ func TestHookMessageHasBeenUpdated(t *testing.T) {
 			plugin.ClientMain(&MyPlugin{})
 		}
 	`}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+	defer tearDown()
 
 	post := &model.Post{
 		UserId:    th.BasicUser.Id,
@@ -445,7 +456,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 		mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
 		mockAPI.On("LogDebug", "testhook.txt").Return(nil)
 		mockAPI.On("LogDebug", "inputfile").Return(nil)
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -468,6 +479,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 			}
 			`,
 		}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+		defer tearDown()
 
 		_, err := th.App.UploadFiles(
 			"noteam",
@@ -491,7 +503,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 		mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
 		mockAPI.On("LogDebug", "testhook.txt").Return(nil)
 		mockAPI.On("LogDebug", "inputfile").Return(nil)
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -516,6 +528,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 			}
 			`,
 		}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+		defer tearDown()
 
 		_, err := th.App.UploadFiles(
 			"noteam",
@@ -539,7 +552,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 		mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
 		mockAPI.On("LogDebug", "testhook.txt").Return(nil)
 		mockAPI.On("LogDebug", "inputfile").Return(nil)
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -562,6 +575,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 			}
 			`,
 		}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+		defer tearDown()
 
 		response, err := th.App.UploadFiles(
 			"noteam",
@@ -597,7 +611,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 		mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
 		mockAPI.On("LogDebug", "testhook.txt").Return(nil)
 		mockAPI.On("LogDebug", "inputfile").Return(nil)
-		SetAppEnvironmentWithPlugins(t, []string{
+		tearDown, _, _ := SetAppEnvironmentWithPlugins(t, []string{
 			`
 			package main
 
@@ -629,6 +643,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 			}
 			`,
 		}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+		defer tearDown()
 
 		response, err := th.App.UploadFiles(
 			"noteam",
@@ -667,7 +682,7 @@ func TestUserWillLogIn_Blocked(t *testing.T) {
 		t.Errorf("Error updating user password: %s", err)
 	}
 
-	SetAppEnvironmentWithPlugins(t,
+	tearDown, _, _ := SetAppEnvironmentWithPlugins(t,
 		[]string{
 			`
 		package main
@@ -689,6 +704,7 @@ func TestUserWillLogIn_Blocked(t *testing.T) {
 			plugin.ClientMain(&MyPlugin{})
 		}
 	`}, th.App, th.App.NewPluginAPI)
+	defer tearDown()
 
 	user, err := th.App.AuthenticateUserForLogin("", th.BasicUser.Email, "hunter2", "", false)
 
@@ -711,7 +727,7 @@ func TestUserWillLogInIn_Passed(t *testing.T) {
 		t.Errorf("Error updating user password: %s", err)
 	}
 
-	SetAppEnvironmentWithPlugins(t,
+	tearDown, _, _ := SetAppEnvironmentWithPlugins(t,
 		[]string{
 			`
 		package main
@@ -733,6 +749,7 @@ func TestUserWillLogInIn_Passed(t *testing.T) {
 			plugin.ClientMain(&MyPlugin{})
 		}
 	`}, th.App, th.App.NewPluginAPI)
+	defer tearDown()
 
 	user, err := th.App.AuthenticateUserForLogin("", th.BasicUser.Email, "hunter2", "", false)
 
@@ -755,7 +772,7 @@ func TestUserHasLoggedIn(t *testing.T) {
 		t.Errorf("Error updating user password: %s", err)
 	}
 
-	SetAppEnvironmentWithPlugins(t,
+	tearDown, _, _ := SetAppEnvironmentWithPlugins(t,
 		[]string{
 			`
 		package main
@@ -778,6 +795,7 @@ func TestUserHasLoggedIn(t *testing.T) {
 			plugin.ClientMain(&MyPlugin{})
 		}
 	`}, th.App, th.App.NewPluginAPI)
+	defer tearDown()
 
 	user, err := th.App.AuthenticateUserForLogin("", th.BasicUser.Email, "hunter2", "", false)
 
@@ -803,7 +821,7 @@ func TestErrorString(t *testing.T) {
 	defer th.TearDown()
 
 	t.Run("errors.New", func(t *testing.T) {
-		activationErrors := SetAppEnvironmentWithPlugins(t,
+		tearDown, _, activationErrors := SetAppEnvironmentWithPlugins(t,
 			[]string{
 				`
 			package main
@@ -826,6 +844,7 @@ func TestErrorString(t *testing.T) {
 				plugin.ClientMain(&MyPlugin{})
 			}
 		`}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
 
 		require.Len(t, activationErrors, 1)
 		require.NotNil(t, activationErrors[0])
@@ -833,7 +852,7 @@ func TestErrorString(t *testing.T) {
 	})
 
 	t.Run("AppError", func(t *testing.T) {
-		activationErrors := SetAppEnvironmentWithPlugins(t,
+		tearDown, _, activationErrors := SetAppEnvironmentWithPlugins(t,
 			[]string{
 				`
 			package main
@@ -855,6 +874,7 @@ func TestErrorString(t *testing.T) {
 				plugin.ClientMain(&MyPlugin{})
 			}
 		`}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
 
 		require.Len(t, activationErrors, 1)
 		require.NotNil(t, activationErrors[0])
diff --git a/app/plugin_install.go b/app/plugin_install.go
index b97fbc5d5..588f86f08 100644
--- a/app/plugin_install.go
+++ b/app/plugin_install.go
@@ -120,6 +120,8 @@ func (a *App) removePlugin(id string) *model.AppError {
 		a.Publish(message)
 	}
 
+	a.UnregisterPluginCommands(id)
+
 	a.Plugins.Deactivate(id)
 
 	err = os.RemoveAll(pluginPath)