[MM-52924] Implement ConfigurationWillBeSaved plugin hook (#23567)

* Implement ConfigurationWillBeSaved plugin hook

* Add comment

* Update comment

* Fix potential nil dereference if plugin environment is unset

* Address PR review

server/channels/app/platform/config.go

@@ -18,6 +18,7 @@ import (
 	"strconv"
 
 	"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/channels/product"
@@ -74,6 +75,24 @@ func (ps *PlatformService) IsConfigReadOnly() bool {
 // SaveConfig replaces the active configuration, optionally notifying cluster peers.
 // It returns both the previous and current configs.
 func (ps *PlatformService) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) (*model.Config, *model.Config, *model.AppError) {
+	if ps.pluginEnv != nil {
+		var hookErr error
+		ps.pluginEnv.RunMultiHook(func(hooks plugin.Hooks) bool {
+			var cfg *model.Config
+			cfg, hookErr = hooks.ConfigurationWillBeSaved(newCfg)
+			if hookErr == nil && cfg != nil {
+				newCfg = cfg
+			}
+			return hookErr == nil
+		}, plugin.ConfigurationWillBeSavedID)
+		if hookErr != nil {
+			if appErr, ok := hookErr.(*model.AppError); ok {
+				return nil, nil, appErr
+			}
+			return nil, nil, model.NewAppError("saveConfig", "app.save_config.plugin_hook_error", nil, "", http.StatusBadRequest).Wrap(hookErr)
+		}
+	}
+
 	oldCfg, newCfg, err := ps.configStore.Set(newCfg)
 	if errors.Is(err, config.ErrReadOnlyConfiguration) {
 		return nil, nil, model.NewAppError("saveConfig", "ent.cluster.save_config.error", nil, "", http.StatusForbidden).Wrap(err)

server/channels/app/plugin_api_test.go

@@ -5,6 +5,7 @@ package app
 
 import (
 	"bytes"
+	_ "embed"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -2195,3 +2196,106 @@ func TestPluginUploadsAPI(t *testing.T) {
 	require.NotNil(t, manifest)
 	require.True(t, activated)
 }
+
+//go:embed plugin_api_tests/manual.test_configuration_will_be_saved_hook/main.tmpl
+var configurationWillBeSavedHookTemplate string
+
+func TestConfigurationWillBeSavedHook(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	getPluginCode := func(hookCode string) string {
+		return fmt.Sprintf(configurationWillBeSavedHookTemplate, hookCode)
+	}
+
+	runPlugin := func(t *testing.T, code string) {
+		pluginDir, err := os.MkdirTemp("", "")
+		require.NoError(t, err)
+		webappPluginDir, err := os.MkdirTemp("", "")
+		require.NoError(t, err)
+		defer os.RemoveAll(pluginDir)
+		defer os.RemoveAll(webappPluginDir)
+
+		newPluginAPI := func(manifest *model.Manifest) plugin.API {
+			return th.App.NewPluginAPI(th.Context, manifest)
+		}
+		env, err := plugin.NewEnvironment(newPluginAPI, NewDriverImpl(th.App.Srv()), pluginDir, webappPluginDir, th.App.Log(), nil)
+		require.NoError(t, err)
+
+		th.App.ch.SetPluginsEnvironment(env)
+
+		pluginID := "testplugin"
+		pluginManifest := `{"id": "testplugin", "server": {"executable": "backend.exe"}}`
+		backend := filepath.Join(pluginDir, pluginID, "backend.exe")
+		utils.CompileGo(t, code, backend)
+
+		os.WriteFile(filepath.Join(pluginDir, pluginID, "plugin.json"), []byte(pluginManifest), 0600)
+		manifest, activated, reterr := env.Activate(pluginID)
+		require.NoError(t, reterr)
+		require.NotNil(t, manifest)
+		require.True(t, activated)
+	}
+
+	t.Run("error", func(t *testing.T) {
+		hookCode := `
+    return nil, fmt.Errorf("plugin hook failed")
+    `
+
+		runPlugin(t, getPluginCode(hookCode))
+
+		cfg := th.App.Config()
+		_, _, appErr := th.App.SaveConfig(cfg, false)
+		require.NotNil(t, appErr)
+		require.Equal(t, "saveConfig: An error occurred running the plugin hook on configuration save., plugin hook failed", appErr.Error())
+
+		require.Equal(t, cfg, th.App.Config())
+	})
+
+	t.Run("AppError", func(t *testing.T) {
+		hookCode := `
+    return nil, model.NewAppError("saveConfig", "custom_error", nil, "", 400)
+    `
+
+		runPlugin(t, getPluginCode(hookCode))
+
+		cfg := th.App.Config()
+		_, _, appErr := th.App.SaveConfig(cfg, false)
+		require.NotNil(t, appErr)
+		require.Equal(t, "custom_error", appErr.Id)
+
+		require.Equal(t, cfg, th.App.Config())
+	})
+
+	t.Run("no error, no config change", func(t *testing.T) {
+		hookCode := `
+    return nil, nil
+    `
+
+		runPlugin(t, getPluginCode(hookCode))
+
+		cfg := th.App.Config()
+		_, newCfg, appErr := th.App.SaveConfig(cfg, false)
+		require.Nil(t, appErr)
+		require.Equal(t, cfg, newCfg)
+	})
+
+	t.Run("config change", func(t *testing.T) {
+		hookCode := `
+    cfg := newCfg.Clone()
+		cfg.PluginSettings.Plugins["custom_plugin"] = map[string]any{
+		  "custom_key": "custom_val",
+		}
+    return cfg, nil
+    `
+
+		runPlugin(t, getPluginCode(hookCode))
+
+		cfg := th.App.Config()
+		_, newCfg, appErr := th.App.SaveConfig(cfg, false)
+		require.Nil(t, appErr)
+		require.NotEqual(t, cfg, newCfg)
+		require.Equal(t, map[string]any{
+			"custom_key": "custom_val",
+		}, newCfg.PluginSettings.Plugins["custom_plugin"])
+	})
+}

server/channels/app/plugin_api_tests/manual.test_configuration_will_be_saved_hook/main.tmpl

@@ -0,0 +1,27 @@
+package main
+
+import (
+	"fmt"
+
+	"github.com/mattermost/mattermost/server/public/model"
+	"github.com/mattermost/mattermost/server/public/plugin"
+)
+
+type TestPlugin struct {
+	plugin.MattermostPlugin
+}
+
+func (p *TestPlugin) OnActivate() error {
+	fmt.Println("activated")
+	return nil
+}
+
+// This acts like a template as the content of this file gets passed to
+// fmt.Sprintf to inject additional logic based on the test case.
+func (p *TestPlugin) ConfigurationWillBeSaved(newCfg *model.Config) (*model.Config, error) {
+	%s
+}
+
+func main() {
+	plugin.ClientMain(&TestPlugin{})
+}

server/i18n/en.json

@@ -6383,6 +6383,10 @@
     "id": "app.save_config.app_error",
     "translation": "An error occurred saving the configuration."
   },
+  {
+    "id": "app.save_config.plugin_hook_error",
+    "translation": "An error occurred running the plugin hook on configuration save."
+  },
   {
     "id": "app.scheme.delete.app_error",
     "translation": "Unable to delete this scheme."

server/public/model/utils.go

@@ -258,15 +258,16 @@ func AppErrorInit(t i18n.TranslateFunc) {
 }
 
 type AppError struct {
-	Id            string `json:"id"`
-	Message       string `json:"message"`               // Message to be display to the end user without debugging information
-	DetailedError string `json:"detailed_error"`        // Internal error string to help the developer
-	RequestId     string `json:"request_id,omitempty"`  // The RequestId that's also set in the header
-	StatusCode    int    `json:"status_code,omitempty"` // The http status code
-	Where         string `json:"-"`                     // The function where it happened in the form of Struct.Func
-	IsOAuth       bool   `json:"is_oauth,omitempty"`    // Whether the error is OAuth specific
-	params        map[string]any
-	wrapped       error
+	Id              string `json:"id"`
+	Message         string `json:"message"`               // Message to be display to the end user without debugging information
+	DetailedError   string `json:"detailed_error"`        // Internal error string to help the developer
+	RequestId       string `json:"request_id,omitempty"`  // The RequestId that's also set in the header
+	StatusCode      int    `json:"status_code,omitempty"` // The http status code
+	Where           string `json:"-"`                     // The function where it happened in the form of Struct.Func
+	IsOAuth         bool   `json:"is_oauth,omitempty"`    // Whether the error is OAuth specific
+	SkipTranslation bool   `json:"-"`                     // Whether translation for the error should be skipped.
+	params          map[string]any
+	wrapped         error
 }
 
 const maxErrorLength = 1024
@@ -304,6 +305,10 @@ func (er *AppError) Error() string {
 }
 
 func (er *AppError) Translate(T i18n.TranslateFunc) {
+	if er.SkipTranslation {
+		return
+	}
+
 	if T == nil {
 		er.Message = er.Id
 		return

server/public/plugin/client_rpc_generated.go

@@ -1037,6 +1037,42 @@ func (s *hooksRPCServer) GetTopicMetadataByIds(args *Z_GetTopicMetadataByIdsArgs
 	return nil
 }
 
+func init() {
+	hookNameToId["ConfigurationWillBeSaved"] = ConfigurationWillBeSavedID
+}
+
+type Z_ConfigurationWillBeSavedArgs struct {
+	A *model.Config
+}
+
+type Z_ConfigurationWillBeSavedReturns struct {
+	A *model.Config
+	B error
+}
+
+func (g *hooksRPCClient) ConfigurationWillBeSaved(newCfg *model.Config) (*model.Config, error) {
+	_args := &Z_ConfigurationWillBeSavedArgs{newCfg}
+	_returns := &Z_ConfigurationWillBeSavedReturns{}
+	if g.implemented[ConfigurationWillBeSavedID] {
+		if err := g.client.Call("Plugin.ConfigurationWillBeSaved", _args, _returns); err != nil {
+			g.log.Error("RPC call ConfigurationWillBeSaved to plugin failed.", mlog.Err(err))
+		}
+	}
+	return _returns.A, _returns.B
+}
+
+func (s *hooksRPCServer) ConfigurationWillBeSaved(args *Z_ConfigurationWillBeSavedArgs, returns *Z_ConfigurationWillBeSavedReturns) error {
+	if hook, ok := s.impl.(interface {
+		ConfigurationWillBeSaved(newCfg *model.Config) (*model.Config, error)
+	}); ok {
+		returns.A, returns.B = hook.ConfigurationWillBeSaved(args.A)
+		returns.B = encodableError(returns.B)
+	} else {
+		return encodableError(fmt.Errorf("Hook ConfigurationWillBeSaved called but not implemented."))
+	}
+	return nil
+}
+
 type Z_RegisterCommandArgs struct {
 	A *model.Command
 }

server/public/plugin/hooks.go

@@ -49,6 +49,7 @@ const (
 	GetTopicRedirectID              = 31
 	GetCollectionMetadataByIdsID    = 32
 	GetTopicMetadataByIdsID         = 33
+	ConfigurationWillBeSavedID      = 34
 	TotalHooksID                    = iota
 )
 
@@ -336,4 +337,11 @@ type Hooks interface {
 	//
 	// Minimum server version: 7.6
 	GetTopicMetadataByIds(c *Context, topicType string, topicIds []string) (map[string]*model.TopicMetadata, error)
+
+	// ConfigurationWillBeSaved is invoked before saving the configuration to the
+	// backing store.
+	// An error can be returned to reject the operation. Additionally, a new
+	// config object can be returned to be stored in place of the provided one.
+	// Minimum server version: 8.0
+	ConfigurationWillBeSaved(newCfg *model.Config) (*model.Config, error)
 }

server/public/plugin/hooks_timer_layer_generated.go

@@ -253,3 +253,10 @@ func (hooks *hooksTimerLayer) GetTopicMetadataByIds(c *Context, topicType string
 	hooks.recordTime(startTime, "GetTopicMetadataByIds", _returnsB == nil)
 	return _returnsA, _returnsB
 }
+
+func (hooks *hooksTimerLayer) ConfigurationWillBeSaved(newCfg *model.Config) (*model.Config, error) {
+	startTime := timePkg.Now()
+	_returnsA, _returnsB := hooks.hooksImpl.ConfigurationWillBeSaved(newCfg)
+	hooks.recordTime(startTime, "ConfigurationWillBeSaved", _returnsB == nil)
+	return _returnsA, _returnsB
+}

server/public/plugin/plugintest/hooks.go

@@ -25,6 +25,32 @@ func (_m *Hooks) ChannelHasBeenCreated(c *plugin.Context, channel *model.Channel
 	_m.Called(c, channel)
 }
 
+// ConfigurationWillBeSaved provides a mock function with given fields: newCfg
+func (_m *Hooks) ConfigurationWillBeSaved(newCfg *model.Config) (*model.Config, error) {
+	ret := _m.Called(newCfg)
+
+	var r0 *model.Config
+	var r1 error
+	if rf, ok := ret.Get(0).(func(*model.Config) (*model.Config, error)); ok {
+		return rf(newCfg)
+	}
+	if rf, ok := ret.Get(0).(func(*model.Config) *model.Config); ok {
+		r0 = rf(newCfg)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*model.Config)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(*model.Config) error); ok {
+		r1 = rf(newCfg)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // ExecuteCommand provides a mock function with given fields: c, args
 func (_m *Hooks) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) {
 	ret := _m.Called(c, args)

server/public/plugin/product_hooks_generated.go

@@ -138,6 +138,10 @@ type GetTopicMetadataByIdsIFace interface {
 	GetTopicMetadataByIds(c *Context, topicType string, topicIds []string) (map[string]*model.TopicMetadata, error)
 }
 
+type ConfigurationWillBeSavedIFace interface {
+	ConfigurationWillBeSaved(newCfg *model.Config) (*model.Config, error)
+}
+
 type HooksAdapter struct {
 	implemented  map[int]struct{}
 	productHooks any
@@ -430,6 +434,15 @@ func NewAdapter(productHooks any) (*HooksAdapter, error) {
 		return nil, errors.New("hook has GetTopicMetadataByIds method but does not implement plugin.GetTopicMetadataByIds interface")
 	}
 
+	// Assessing the type of the productHooks if it individually implements ConfigurationWillBeSaved interface.
+	tt = reflect.TypeOf((*ConfigurationWillBeSavedIFace)(nil)).Elem()
+
+	if ft.Implements(tt) {
+		a.implemented[ConfigurationWillBeSavedID] = struct{}{}
+	} else if _, ok := ft.MethodByName("ConfigurationWillBeSaved"); ok {
+		return nil, errors.New("hook has ConfigurationWillBeSaved method but does not implement plugin.ConfigurationWillBeSaved interface")
+	}
+
 	return a, nil
 }
 
@@ -711,3 +724,12 @@ func (a *HooksAdapter) GetTopicMetadataByIds(c *Context, topicType string, topic
 	return a.productHooks.(GetTopicMetadataByIdsIFace).GetTopicMetadataByIds(c, topicType, topicIds)
 
 }
+
+func (a *HooksAdapter) ConfigurationWillBeSaved(newCfg *model.Config) (*model.Config, error) {
+	if _, ok := a.implemented[ConfigurationWillBeSavedID]; !ok {
+		panic("product hooks must implement ConfigurationWillBeSaved")
+	}
+
+	return a.productHooks.(ConfigurationWillBeSavedIFace).ConfigurationWillBeSaved(newCfg)
+
+}