MM-53355: tidy up plugins (#24194)

* remove feature flag managed plugins

* remove unneeded plugin blocklist

* remove unnecessary wrappers

* documentation and logging improvements

* avoid use of global logger
* leverage wrapped loggers (e.g. consistently log `plugin_id`)
* promote some logs from `Debug` to `Info` for better visibility.
* extract installPluginToFilestore
* rename some variables for consistency / clarity

* make generated

server/channels/app/plugin_install.go

@@ -46,7 +46,6 @@ import (
 	"github.com/blang/semver"
 
 	"github.com/mattermost/mattermost/server/public/model"
-	"github.com/mattermost/mattermost/server/public/plugin"
 	"github.com/mattermost/mattermost/server/public/shared/mlog"
 	"github.com/mattermost/mattermost/server/public/utils"
 	"github.com/mattermost/mattermost/server/v8/platform/shared/filestore"
@@ -59,88 +58,93 @@ const managedPluginFileName = ".filestore"
 // fileStorePluginFolder is the folder name in the file store of the plugin bundles installed.
 const fileStorePluginFolder = "plugins"
 
-func (ch *Channels) installPluginFromData(data model.PluginEventData) {
-	mlog.Debug("Installing plugin as per cluster message", mlog.String("plugin_id", data.Id))
+// installPluginFromClusterMessage is called when a peer activates a plugin in the filestore,
+// signalling all other servers to do the same.
+func (ch *Channels) installPluginFromClusterMessage(pluginID string) {
+	logger := ch.srv.Log().With(mlog.String("plugin_id", pluginID))
+
+	logger.Info("Installing plugin as per cluster message")
 
 	pluginSignaturePathMap, appErr := ch.getPluginsFromFolder()
 	if appErr != nil {
-		mlog.Error("Failed to get plugin signatures from filestore. Can't install plugin from data.", mlog.Err(appErr))
+		logger.Error("Failed to get plugin signatures from filestore.", mlog.Err(appErr))
 		return
 	}
-	plugin, ok := pluginSignaturePathMap[data.Id]
+	plugin, ok := pluginSignaturePathMap[pluginID]
 	if !ok {
-		mlog.Error("Failed to get plugin signature from filestore. Can't install plugin from data.", mlog.String("plugin id", data.Id))
+		logger.Error("Failed to get plugin signature from filestore.")
 		return
 	}
 
-	reader, appErr := ch.srv.fileReader(plugin.path)
+	bundle, appErr := ch.srv.fileReader(plugin.bundlePath)
 	if appErr != nil {
-		mlog.Error("Failed to open plugin bundle from file store.", mlog.String("bundle", plugin.path), mlog.Err(appErr))
+		logger.Error("Failed to open plugin bundle from file store.", mlog.Err(appErr))
 		return
 	}
-	defer reader.Close()
+	defer bundle.Close()
 
 	var signature filestore.ReadCloseSeeker
 	if *ch.cfgSvc.Config().PluginSettings.RequirePluginSignature {
 		signature, appErr = ch.srv.fileReader(plugin.signaturePath)
 		if appErr != nil {
-			mlog.Error("Failed to open plugin signature from file store.", mlog.Err(appErr))
+			logger.Error("Failed to open plugin signature from file store.", mlog.Err(appErr))
 			return
 		}
 		defer signature.Close()
 	}
 
-	manifest, appErr := ch.installPluginLocally(reader, signature, installPluginLocallyAlways)
+	manifest, appErr := ch.installPluginLocally(bundle, signature, installPluginLocallyAlways)
 	if appErr != nil {
 		// A log line already appears if the plugin is on the blocklist or skipped
 		if appErr.Id != "app.plugin.blocked.app_error" && appErr.Id != "app.plugin.skip_installation.app_error" {
-			mlog.Error("Failed to sync plugin from file store", mlog.String("bundle", plugin.path), mlog.Err(appErr))
+			logger.Error("Failed to sync plugin from file store", mlog.Err(appErr))
 		}
 		return
 	}
 
 	if err := ch.notifyPluginEnabled(manifest); err != nil {
-		mlog.Error("Failed notify plugin enabled", mlog.Err(err))
+		logger.Error("Failed notify plugin enabled", mlog.Err(err))
 	}
 
 	if err := ch.notifyPluginStatusesChanged(); err != nil {
-		mlog.Error("Failed to notify plugin status changed", mlog.Err(err))
+		logger.Error("Failed to notify plugin status changed", mlog.Err(err))
 	}
 }
 
-func (ch *Channels) removePluginFromData(data model.PluginEventData) {
-	mlog.Debug("Removing plugin as per cluster message", mlog.String("plugin_id", data.Id))
+// removePluginFromClusterMessage is called when a peer removes a plugin, signalling all other
+// servers to do the same.
+func (ch *Channels) removePluginFromClusterMessage(pluginID string) {
+	logger := ch.srv.Log().With(mlog.String("plugin_id", pluginID))
 
-	if err := ch.removePluginLocally(data.Id); err != nil {
-		mlog.Warn("Failed to remove plugin locally", mlog.Err(err), mlog.String("id", data.Id))
+	logger.Info("Removing plugin as per cluster message")
+
+	if err := ch.removePluginLocally(pluginID); err != nil {
+		logger.Error("Failed to remove plugin locally", mlog.Err(err))
 	}
 
 	if err := ch.notifyPluginStatusesChanged(); err != nil {
-		mlog.Warn("failed to notify plugin status changed", mlog.Err(err))
+		logger.Error("failed to notify plugin status changed", mlog.Err(err))
 	}
 }
 
-// InstallPluginWithSignature verifies and installs plugin.
-func (ch *Channels) installPluginWithSignature(pluginFile, signature io.ReadSeeker) (*model.Manifest, *model.AppError) {
-	return ch.installPlugin(pluginFile, signature, installPluginLocallyAlways)
-}
-
-// InstallPlugin unpacks and installs a plugin but does not enable or activate it.
+// InstallPlugin unpacks and installs a plugin but does not enable or activate it unless the the
+// plugin was already enabled.
 func (a *App) InstallPlugin(pluginFile io.ReadSeeker, replace bool) (*model.Manifest, *model.AppError) {
 	installationStrategy := installPluginLocallyOnlyIfNew
 	if replace {
 		installationStrategy = installPluginLocallyAlways
 	}
 
-	return a.installPlugin(pluginFile, nil, installationStrategy)
+	return a.ch.installPlugin(pluginFile, nil, installationStrategy)
 }
 
-func (a *App) installPlugin(pluginFile, signature io.ReadSeeker, installationStrategy pluginInstallationStrategy) (*model.Manifest, *model.AppError) {
-	return a.ch.installPlugin(pluginFile, signature, installationStrategy)
-}
-
-func (ch *Channels) installPlugin(pluginFile, signature io.ReadSeeker, installationStrategy pluginInstallationStrategy) (*model.Manifest, *model.AppError) {
-	manifest, appErr := ch.installPluginLocally(pluginFile, signature, installationStrategy)
+// installPlugin extracts and installs the given plugin bundle (optionally signed) for the
+// current server, activating the plugin if already enabled, installs it to the filestore for
+// cluster peers to use, and then broadcasts the change to connected websockets.
+//
+// The given installation strategy decides how to handle upgrade scenarios.
+func (ch *Channels) installPlugin(bundle, signature io.ReadSeeker, installationStrategy pluginInstallationStrategy) (*model.Manifest, *model.AppError) {
+	manifest, appErr := ch.installPluginLocally(bundle, signature, installationStrategy)
 	if appErr != nil {
 		return nil, appErr
 	}
@@ -149,17 +153,42 @@ func (ch *Channels) installPlugin(pluginFile, signature io.ReadSeeker, installat
 		return nil, nil
 	}
 
+	logger := ch.srv.Log().With(mlog.String("plugin_id", manifest.Id))
+
+	appErr = ch.installPluginToFilestore(manifest, bundle, signature)
+	if appErr != nil {
+		return nil, appErr
+	}
+
+	if err := ch.notifyPluginEnabled(manifest); err != nil {
+		logger.Warn("Failed notify plugin enabled", mlog.Err(err))
+	}
+
+	if err := ch.notifyPluginStatusesChanged(); err != nil {
+		logger.Warn("Failed to notify plugin status changed", mlog.Err(err))
+	}
+
+	return manifest, nil
+}
+
+// installPluginToFilestore saves the given plugin bundle (optionally signed) to the filestore,
+// notifying cluster peers accordingly.
+func (ch *Channels) installPluginToFilestore(manifest *model.Manifest, bundle, signature io.ReadSeeker) *model.AppError {
 	if signature != nil {
-		signature.Seek(0, 0)
-		if _, appErr = ch.srv.writeFile(signature, getSignatureStorePath(manifest.Id)); appErr != nil {
-			return nil, model.NewAppError("saveSignature", "app.plugin.store_signature.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr)
+		_, err := signature.Seek(0, 0)
+		if err != nil {
+			return model.NewAppError("saveSignature", "app.plugin.store_signature.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
+		}
+
+		if _, appErr := ch.srv.writeFile(signature, getSignatureStorePath(manifest.Id)); appErr != nil {
+			return model.NewAppError("saveSignature", "app.plugin.store_signature.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr)
 		}
 	}
 
 	// Store bundle in the file store to allow access from other servers.
-	pluginFile.Seek(0, 0)
-	if _, appErr := ch.srv.writeFile(pluginFile, getBundleStorePath(manifest.Id)); appErr != nil {
-		return nil, model.NewAppError("uploadPlugin", "app.plugin.store_bundle.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr)
+	bundle.Seek(0, 0)
+	if _, appErr := ch.srv.writeFile(bundle, getBundleStorePath(manifest.Id)); appErr != nil {
+		return model.NewAppError("uploadPlugin", "app.plugin.store_bundle.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr)
 	}
 
 	ch.notifyClusterPluginEvent(
@@ -169,20 +198,15 @@ func (ch *Channels) installPlugin(pluginFile, signature io.ReadSeeker, installat
 		},
 	)
 
-	if err := ch.notifyPluginEnabled(manifest); err != nil {
-		mlog.Warn("Failed notify plugin enabled", mlog.Err(err))
-	}
-
-	if err := ch.notifyPluginStatusesChanged(); err != nil {
-		mlog.Warn("Failed to notify plugin status changed", mlog.Err(err))
-	}
-
-	return manifest, nil
+	return nil
 }
 
-// InstallMarketplacePlugin installs a plugin listed in the marketplace server. It will get the plugin bundle
-// from the prepackaged folder, if available, or remotely if EnableRemoteMarketplace is true.
+// InstallMarketplacePlugin installs a plugin listed in the marketplace server. It will get the
+// plugin bundle from the prepackaged folder, if available, or remotely if EnableRemoteMarketplace
+// is true.
 func (ch *Channels) InstallMarketplacePlugin(request *model.InstallMarketplacePluginRequest) (*model.Manifest, *model.AppError) {
+	logger := ch.srv.Log().With(mlog.String("plugin_id", request.Id))
+
 	var pluginFile, signatureFile io.ReadSeeker
 
 	prepackagedPlugin, appErr := ch.getPrepackagedPlugin(request.Id, request.Version)
@@ -205,7 +229,7 @@ func (ch *Channels) InstallMarketplacePlugin(request *model.InstallMarketplacePl
 		plugin, appErr = ch.getRemoteMarketplacePlugin(request.Id, request.Version)
 		// The plugin might only be prepackaged and not on the Marketplace.
 		if appErr != nil && appErr.Id != "app.plugin.marketplace_plugins.not_found.app_error" {
-			mlog.Warn("Failed to reach Marketplace to install plugin", mlog.String("plugin_id", request.Id), mlog.Err(appErr))
+			logger.Warn("Failed to reach Marketplace to install plugin", mlog.Err(appErr))
 		}
 
 		if plugin != nil {
@@ -245,7 +269,7 @@ func (ch *Channels) InstallMarketplacePlugin(request *model.InstallMarketplacePl
 		return nil, model.NewAppError("InstallMarketplacePlugin", "app.plugin.marketplace_plugins.signature_not_found.app_error", nil, "", http.StatusInternalServerError)
 	}
 
-	manifest, appErr := ch.installPluginWithSignature(pluginFile, signatureFile)
+	manifest, appErr := ch.installPlugin(pluginFile, signatureFile, installPluginLocallyAlways)
 	if appErr != nil {
 		return nil, appErr
 	}
@@ -264,7 +288,11 @@ const (
 	installPluginLocallyAlways
 )
 
-func (ch *Channels) installPluginLocally(pluginFile, signature io.ReadSeeker, installationStrategy pluginInstallationStrategy) (*model.Manifest, *model.AppError) {
+// installPluginLocally extracts and installs the given plugin bundle (optionally signed) for the
+// current server, activating the plugin if already enabled.
+//
+// The given installation strategy decides how to handle upgrade scenarios.
+func (ch *Channels) installPluginLocally(bundle, signature io.ReadSeeker, installationStrategy pluginInstallationStrategy) (*model.Manifest, *model.AppError) {
 	pluginsEnvironment := ch.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {
 		return nil, model.NewAppError("installPluginLocally", "app.plugin.disabled.app_error", nil, "", http.StatusNotImplemented)
@@ -272,7 +300,7 @@ func (ch *Channels) installPluginLocally(pluginFile, signature io.ReadSeeker, in
 
 	// verify signature
 	if signature != nil {
-		if err := ch.verifyPlugin(pluginFile, signature); err != nil {
+		if err := ch.verifyPlugin(bundle, signature); err != nil {
 			return nil, err
 		}
 	}
@@ -283,7 +311,7 @@ func (ch *Channels) installPluginLocally(pluginFile, signature io.ReadSeeker, in
 	}
 	defer os.RemoveAll(tmpDir)
 
-	manifest, pluginDir, appErr := extractPlugin(pluginFile, tmpDir)
+	manifest, pluginDir, appErr := extractPlugin(bundle, tmpDir)
 	if appErr != nil {
 		return nil, appErr
 	}
@@ -296,9 +324,10 @@ func (ch *Channels) installPluginLocally(pluginFile, signature io.ReadSeeker, in
 	return manifest, nil
 }
 
-func extractPlugin(pluginFile io.ReadSeeker, extractDir string) (*model.Manifest, string, *model.AppError) {
-	pluginFile.Seek(0, 0)
-	if err := extractTarGz(pluginFile, extractDir); err != nil {
+// extractPlugin unpacks the given plugin bundle into the specified directory.
+func extractPlugin(bundle io.ReadSeeker, extractDir string) (*model.Manifest, string, *model.AppError) {
+	bundle.Seek(0, 0)
+	if err := extractTarGz(bundle, extractDir); err != nil {
 		return nil, "", model.NewAppError("extractPlugin", "app.plugin.extract.app_error", nil, "", http.StatusBadRequest).Wrap(err)
 	}
 
@@ -307,6 +336,8 @@ func extractPlugin(pluginFile io.ReadSeeker, extractDir string) (*model.Manifest
 		return nil, "", model.NewAppError("extractPlugin", "app.plugin.filesystem.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
+	// If the root of the plugin bundle consists of exactly one directory, assume the plugin
+	// is contained therein. Otherwise the root directory is expected to contain the plugin.
 	if len(dir) == 1 && dir[0].IsDir() {
 		extractDir = filepath.Join(extractDir, dir[0].Name())
 	}
@@ -317,13 +348,21 @@ func extractPlugin(pluginFile io.ReadSeeker, extractDir string) (*model.Manifest
 	}
 
 	if !model.IsValidPluginId(manifest.Id) {
-		return nil, "", model.NewAppError("installPluginLocally", "app.plugin.invalid_id.app_error", map[string]any{"Min": model.MinIdLength, "Max": model.MaxIdLength, "Regex": model.ValidIdRegex}, "", http.StatusBadRequest)
+		return nil, "", model.NewAppError("extractPlugin", "app.plugin.invalid_id.app_error", map[string]any{"Min": model.MinIdLength, "Max": model.MaxIdLength, "Regex": model.ValidIdRegex}, "", http.StatusBadRequest)
 	}
 
 	return manifest, extractDir, nil
 }
 
+// installExtractedPlugin installs a plugin previously extracted to a temporary directory,
+// activating the plugin automatically if already enabled by the server configuration.
+//
+// The given installation strategy decides how to handle upgrade scenarios.
 func (ch *Channels) installExtractedPlugin(manifest *model.Manifest, fromPluginDir string, installationStrategy pluginInstallationStrategy) (*model.Manifest, *model.AppError) {
+	logger := ch.srv.Log().With(mlog.String("plugin_id", manifest.Id))
+
+	logger.Info("Installing extracted plugin", mlog.String("version", manifest.Version))
+
 	pluginsEnvironment := ch.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {
 		return nil, model.NewAppError("installExtractedPlugin", "app.plugin.disabled.app_error", nil, "", http.StatusNotImplemented)
@@ -334,12 +373,6 @@ func (ch *Channels) installExtractedPlugin(manifest *model.Manifest, fromPluginD
 		return nil, model.NewAppError("installExtractedPlugin", "app.plugin.install.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
-	// Check plugin id is not blocked
-	if plugin.PluginIDIsBlocked(manifest.Id) {
-		mlog.Debug("Skipping installation of plugin since plugin is on blocklist. Some plugins are blocked because they are built into this version of Mattermost.", mlog.String("plugin_id", manifest.Id))
-		return nil, model.NewAppError("installExtractedPlugin", "app.plugin.blocked.app_error", map[string]any{"Id": manifest.Id}, "", http.StatusInternalServerError)
-	}
-
 	// Check for plugins installed with the same ID.
 	var existingManifest *model.Manifest
 	for _, bundle := range bundles {
@@ -366,30 +399,30 @@ func (ch *Channels) installExtractedPlugin(manifest *model.Manifest, fromPluginD
 
 			existingVersion, err = semver.Parse(existingManifest.Version)
 			if err != nil {
-				return nil, model.NewAppError("installExtractedPlugin", "app.plugin.invalid_version.app_error", nil, "", http.StatusBadRequest)
+				return nil, model.NewAppError("installExtractedPlugin", "app.plugin.invalid_version.app_error", nil, "", http.StatusInternalServerError)
 			}
 
 			if version.LTE(existingVersion) {
-				mlog.Debug("Skipping local installation of plugin since existing version is newer", mlog.String("plugin_id", manifest.Id))
+				logger.Warn("Skipping local installation of plugin since existing version is newer", mlog.String("version", version.String()), mlog.String("existing_version", existingVersion.String()))
 				return nil, model.NewAppError("installExtractedPlugin", "app.plugin.skip_installation.app_error", map[string]any{"Id": manifest.Id}, "", http.StatusInternalServerError)
 			}
 		}
 
-		// 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))
+		// Otherwise remove the existing installation prior to installing below.
+		logger.Info("Removing existing installation of plugin before local install", mlog.String("existing_version", existingManifest.Version))
 		if err := ch.removePluginLocally(existingManifest.Id); err != nil {
-			return nil, model.NewAppError("installExtractedPlugin", "app.plugin.install_id_failed_remove.app_error", nil, "", http.StatusBadRequest)
+			return nil, model.NewAppError("installExtractedPlugin", "app.plugin.install_id_failed_remove.app_error", nil, "", http.StatusInternalServerError)
 		}
 	}
 
-	pluginPath := filepath.Join(*ch.cfgSvc.Config().PluginSettings.Directory, manifest.Id)
-	err = utils.CopyDir(fromPluginDir, pluginPath)
+	bundlePath := filepath.Join(*ch.cfgSvc.Config().PluginSettings.Directory, manifest.Id)
+	err = utils.CopyDir(fromPluginDir, bundlePath)
 	if err != nil {
 		return nil, model.NewAppError("installExtractedPlugin", "app.plugin.mvdir.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
 	// Flag plugin locally as managed by the filestore.
-	f, err := os.Create(filepath.Join(pluginPath, managedPluginFileName))
+	f, err := os.Create(filepath.Join(bundlePath, managedPluginFileName))
 	if err != nil {
 		return nil, model.NewAppError("installExtractedPlugin", "app.plugin.flag_managed.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
@@ -419,12 +452,13 @@ func (ch *Channels) installExtractedPlugin(manifest *model.Manifest, fromPluginD
 		manifest = updatedManifest
 	}
 
-	mlog.Debug("Installing plugin", mlog.String("plugin_id", manifest.Id), mlog.String("version", manifest.Version))
-
 	return manifest, nil
 }
 
+// RemovePlugin removes a plugin from all servers.
 func (ch *Channels) RemovePlugin(id string) *model.AppError {
+	logger := ch.srv.Log().With(mlog.String("plugin_id", id))
+
 	// Disable plugin before removal to make sure this
 	// plugin remains disabled on re-install.
 	if err := ch.disablePlugin(id); err != nil {
@@ -436,19 +470,19 @@ func (ch *Channels) RemovePlugin(id string) *model.AppError {
 	}
 
 	// Remove bundle from the file store.
-	storePluginFileName := getBundleStorePath(id)
-	bundleExist, err := ch.srv.fileExists(storePluginFileName)
+	bundlePath := getBundleStorePath(id)
+	bundleExists, err := ch.srv.fileExists(bundlePath)
 	if err != nil {
 		return model.NewAppError("removePlugin", "app.plugin.remove_bundle.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
-	if !bundleExist {
+	if !bundleExists {
 		return nil
 	}
-	if err = ch.srv.removeFile(storePluginFileName); err != nil {
+	if err := ch.srv.removeFile(bundlePath); err != nil {
 		return model.NewAppError("removePlugin", "app.plugin.remove_bundle.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
-	if err = ch.removeSignature(id); err != nil {
-		mlog.Warn("Can't remove signature", mlog.Err(err))
+	if err := ch.removeSignature(id); err != nil {
+		logger.Warn("Can't remove signature", mlog.Err(err))
 	}
 
 	ch.notifyClusterPluginEvent(
@@ -459,12 +493,13 @@ func (ch *Channels) RemovePlugin(id string) *model.AppError {
 	)
 
 	if err := ch.notifyPluginStatusesChanged(); err != nil {
-		mlog.Warn("Failed to notify plugin status changed", mlog.Err(err))
+		logger.Warn("Failed to notify plugin status changed", mlog.Err(err))
 	}
 
 	return nil
 }
 
+// removePluginLocally removes the given plugin from the current server.
 func (ch *Channels) removePluginLocally(id string) *model.AppError {
 	pluginsEnvironment := ch.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {
@@ -477,11 +512,11 @@ func (ch *Channels) removePluginLocally(id string) *model.AppError {
 	}
 
 	var manifest *model.Manifest
-	var pluginPath string
+	var unpackedBundlePath string
 	for _, p := range plugins {
 		if p.Manifest != nil && p.Manifest.Id == id {
 			manifest = p.Manifest
-			pluginPath = filepath.Dir(p.ManifestPath)
+			unpackedBundlePath = filepath.Dir(p.ManifestPath)
 			break
 		}
 	}
@@ -494,33 +529,39 @@ func (ch *Channels) removePluginLocally(id string) *model.AppError {
 	pluginsEnvironment.RemovePlugin(id)
 	ch.unregisterPluginCommands(id)
 
-	if err := os.RemoveAll(pluginPath); err != nil {
+	if err := os.RemoveAll(unpackedBundlePath); err != nil {
 		return model.NewAppError("removePlugin", "app.plugin.remove.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
 	return nil
 }
 
+// removeSignature removes the signature file installed alongside the plugin.
 func (ch *Channels) removeSignature(pluginID string) *model.AppError {
-	filePath := getSignatureStorePath(pluginID)
-	exists, err := ch.srv.fileExists(filePath)
+	logger := ch.srv.Log().With(mlog.String("plugin_id", pluginID))
+
+	signaturePath := getSignatureStorePath(pluginID)
+	exists, err := ch.srv.fileExists(signaturePath)
 	if err != nil {
 		return model.NewAppError("removeSignature", "app.plugin.remove_bundle.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 	if !exists {
-		mlog.Debug("no plugin signature to remove", mlog.String("plugin_id", pluginID))
+		logger.Debug("no plugin signature to remove")
 		return nil
 	}
-	if err = ch.srv.removeFile(filePath); err != nil {
+	if err = ch.srv.removeFile(signaturePath); err != nil {
 		return model.NewAppError("removeSignature", "app.plugin.remove_bundle.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 	return nil
 }
 
+// getBundleStorePath maps the given plugin id to the file path of the corresponding plugin bundle.
 func getBundleStorePath(id string) string {
 	return filepath.Join(fileStorePluginFolder, fmt.Sprintf("%s.tar.gz", id))
 }
 
+// getSignatureStorePath maps the given plugin id to the file path of the corresponding plugin
+// signature, if one exists.
 func getSignatureStorePath(id string) string {
 	return filepath.Join(fileStorePluginFolder, fmt.Sprintf("%s.tar.gz.sig", id))
 }