diff --git a/api4/apitestlib.go b/api4/apitestlib.go index ee23132d65..19248cd90e 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -74,7 +74,8 @@ func SetMainHelper(mh *testlib.MainHelper) { mainHelper = mh } -func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, enterprise bool, includeCache bool, updateConfig func(*model.Config)) *TestHelper { +func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, enterprise bool, includeCache bool, + updateConfig func(*model.Config), options []app.Option) *TestHelper { tempWorkspace, err := ioutil.TempDir("", "apptest") if err != nil { panic(err) @@ -104,7 +105,6 @@ func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, ent panic(err) } - var options []app.Option options = append(options, app.ConfigStore(configStore)) if includeCache { // Adds the cache layer to the test store @@ -215,7 +215,7 @@ func SetupEnterprise(tb testing.TB) *TestHelper { dbStore.MarkSystemRanUnitTests() mainHelper.PreloadMigrations() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, true, true, nil) + th := setupTestHelper(dbStore, searchEngine, true, true, nil, nil) th.InitLogin() return th } @@ -234,7 +234,7 @@ func Setup(tb testing.TB) *TestHelper { dbStore.MarkSystemRanUnitTests() mainHelper.PreloadMigrations() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, false, true, nil) + th := setupTestHelper(dbStore, searchEngine, false, true, nil, nil) th.InitLogin() return th } @@ -252,13 +252,13 @@ func SetupConfig(tb testing.TB, updateConfig func(cfg *model.Config)) *TestHelpe dbStore.DropAllTables() dbStore.MarkSystemRanUnitTests() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, false, true, updateConfig) + th := setupTestHelper(dbStore, searchEngine, false, true, updateConfig, nil) th.InitLogin() return th } func SetupConfigWithStoreMock(tb testing.TB, updateConfig func(cfg *model.Config)) *TestHelper { - th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, false, false, updateConfig) + th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, false, false, updateConfig, nil) statusMock := mocks.StatusStore{} statusMock.On("UpdateExpiredDNDStatuses").Return([]*model.Status{}, nil) statusMock.On("Get", "user1").Return(&model.Status{UserId: "user1", Status: model.STATUS_ONLINE}, nil) @@ -272,7 +272,7 @@ func SetupConfigWithStoreMock(tb testing.TB, updateConfig func(cfg *model.Config } func SetupWithStoreMock(tb testing.TB) *TestHelper { - th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, false, false, nil) + th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, false, false, nil, nil) statusMock := mocks.StatusStore{} statusMock.On("UpdateExpiredDNDStatuses").Return([]*model.Status{}, nil) statusMock.On("Get", "user1").Return(&model.Status{UserId: "user1", Status: model.STATUS_ONLINE}, nil) @@ -286,7 +286,7 @@ func SetupWithStoreMock(tb testing.TB) *TestHelper { } func SetupEnterpriseWithStoreMock(tb testing.TB) *TestHelper { - th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, true, false, nil) + th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, true, false, nil, nil) statusMock := mocks.StatusStore{} statusMock.On("UpdateExpiredDNDStatuses").Return([]*model.Status{}, nil) statusMock.On("Get", "user1").Return(&model.Status{UserId: "user1", Status: model.STATUS_ONLINE}, nil) @@ -299,6 +299,25 @@ func SetupEnterpriseWithStoreMock(tb testing.TB) *TestHelper { return th } +func SetupWithServerOptions(tb testing.TB, options []app.Option) *TestHelper { + if testing.Short() { + tb.SkipNow() + } + + if mainHelper == nil { + tb.SkipNow() + } + + dbStore := mainHelper.GetStore() + dbStore.DropAllTables() + dbStore.MarkSystemRanUnitTests() + mainHelper.PreloadMigrations() + searchEngine := mainHelper.GetSearchEngine() + th := setupTestHelper(dbStore, searchEngine, false, true, nil, options) + th.InitLogin() + return th +} + func (th *TestHelper) ShutdownApp() { done := make(chan bool) go func() { diff --git a/api4/config.go b/api4/config.go index 091c3325f2..0d96342b65 100644 --- a/api4/config.go +++ b/api4/config.go @@ -143,13 +143,22 @@ func updateConfig(c *Context, w http.ResponseWriter, r *http.Request) { return } - err = c.App.SaveConfig(cfg, true) + oldCfg, newCfg, err := c.App.SaveConfig(cfg, true) if err != nil { c.Err = err return } - cfg, mergeErr := config.Merge(&model.Config{}, c.App.GetSanitizedConfig(), &utils.MergeConfig{ + diffs, diffErr := config.Diff(oldCfg, newCfg) + if diffErr != nil { + c.Err = model.NewAppError("updateConfig", "api.config.update_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) + return + } + auditRec.AddMeta("diff", diffs) + + newCfg.Sanitize() + + cfg, mergeErr := config.Merge(&model.Config{}, newCfg, &utils.MergeConfig{ StructFieldFilter: func(structField reflect.StructField, base, patch reflect.Value) bool { return readFilter(c, structField) }, @@ -253,15 +262,24 @@ func patchConfig(c *Context, w http.ResponseWriter, r *http.Request) { return } - err = c.App.SaveConfig(updatedCfg, true) + oldCfg, newCfg, err := c.App.SaveConfig(updatedCfg, true) if err != nil { c.Err = err return } + diffs, diffErr := config.Diff(oldCfg, newCfg) + if diffErr != nil { + c.Err = model.NewAppError("patchConfig", "api.config.patch_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) + return + } + auditRec.AddMeta("diff", diffs) + + newCfg.Sanitize() + auditRec.Success() - cfg, mergeErr = config.Merge(&model.Config{}, c.App.GetSanitizedConfig(), &utils.MergeConfig{ + cfg, mergeErr = config.Merge(&model.Config{}, newCfg, &utils.MergeConfig{ StructFieldFilter: func(structField reflect.StructField, base, patch reflect.Value) bool { return readFilter(c, structField) }, diff --git a/api4/config_local.go b/api4/config_local.go index 360f95a0a1..45a2eeb524 100644 --- a/api4/config_local.go +++ b/api4/config_local.go @@ -57,19 +57,26 @@ func localUpdateConfig(c *Context, w http.ResponseWriter, r *http.Request) { return } - err = c.App.SaveConfig(cfg, true) + oldCfg, newCfg, err := c.App.SaveConfig(cfg, true) if err != nil { c.Err = err return } - cfg = c.App.GetSanitizedConfig() + diffs, diffErr := config.Diff(oldCfg, newCfg) + if diffErr != nil { + c.Err = model.NewAppError("updateConfig", "api.config.update_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) + return + } + auditRec.AddMeta("diff", diffs) + + newCfg.Sanitize() auditRec.Success() c.LogAudit("updateConfig") w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate") - w.Write([]byte(cfg.ToJson())) + w.Write([]byte(newCfg.ToJson())) } func localPatchConfig(c *Context, w http.ResponseWriter, r *http.Request) { @@ -106,12 +113,19 @@ func localPatchConfig(c *Context, w http.ResponseWriter, r *http.Request) { return } - err = c.App.SaveConfig(updatedCfg, true) + oldCfg, newCfg, err := c.App.SaveConfig(updatedCfg, true) if err != nil { c.Err = err return } + diffs, diffErr := config.Diff(oldCfg, newCfg) + if diffErr != nil { + c.Err = model.NewAppError("patchConfig", "api.config.patch_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) + return + } + auditRec.AddMeta("diff", diffs) + auditRec.Success() w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate") diff --git a/api4/config_test.go b/api4/config_test.go index cacdaa3d41..2d094a8922 100644 --- a/api4/config_test.go +++ b/api4/config_test.go @@ -4,6 +4,8 @@ package api4 import ( + "fmt" + "io/ioutil" "net/http" "os" "strings" @@ -12,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost-server/v5/app" "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/model" ) @@ -436,6 +439,45 @@ func TestUpdateConfigRestrictSystemAdmin(t *testing.T) { }) } +func TestUpdateConfigDiffInAuditRecord(t *testing.T) { + logFile, err := ioutil.TempFile("", "adv.log") + require.NoError(t, err) + defer os.Remove(logFile.Name()) + + os.Setenv("MM_EXPERIMENTALAUDITSETTINGS_FILEENABLED", "true") + os.Setenv("MM_EXPERIMENTALAUDITSETTINGS_FILENAME", logFile.Name()) + defer os.Unsetenv("MM_EXPERIMENTALAUDITSETTINGS_FILEENABLED") + defer os.Unsetenv("MM_EXPERIMENTALAUDITSETTINGS_FILENAME") + + options := []app.Option{ + func(s *app.Server) error { + s.SetLicense(model.NewTestLicense("advanced_logging")) + return nil + }, + } + th := SetupWithServerOptions(t, options) + defer th.TearDown() + + cfg, resp := th.SystemAdminClient.GetConfig() + CheckNoError(t, resp) + + timeoutVal := *cfg.ServiceSettings.ReadTimeout + cfg.ServiceSettings.ReadTimeout = model.NewInt(timeoutVal + 1) + cfg, resp = th.SystemAdminClient.UpdateConfig(cfg) + CheckNoError(t, resp) + defer th.App.UpdateConfig(func(cfg *model.Config) { + cfg.ServiceSettings.ReadTimeout = model.NewInt(timeoutVal) + }) + require.Equal(t, timeoutVal+1, *cfg.ServiceSettings.ReadTimeout) + + data, err := ioutil.ReadAll(logFile) + require.NoError(t, err) + require.NotEmpty(t, data) + require.Contains(t, string(data), + fmt.Sprintf(`"diff":"[{Path:ServiceSettings.ReadTimeout BaseVal:%d ActualVal:%d}]"`, + timeoutVal, timeoutVal+1)) +} + func TestGetEnvironmentConfig(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://example.mattermost.com") os.Setenv("MM_SERVICESETTINGS_ENABLECUSTOMEMOJI", "true") diff --git a/app/app_iface.go b/app/app_iface.go index 9e84732fa8..78ff840bf5 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -284,7 +284,7 @@ type AppIface interface { // in the server and revoke them RevokeSessionsFromAllUsers() *model.AppError // SaveConfig replaces the active configuration, optionally notifying cluster peers. - SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) *model.AppError + SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) (*model.Config, *model.Config, *model.AppError) // SearchAllChannels returns a list of channels, the total count of the results of the search (if the paginate search option is true), and an error. SearchAllChannels(term string, opts model.ChannelSearchOpts) (*model.ChannelListWithTeamData, int64, *model.AppError) // SearchAllTeams returns a team list and the total count of the results diff --git a/app/config.go b/app/config.go index 3c5a5404fa..8df70944e3 100644 --- a/app/config.go +++ b/app/config.go @@ -54,7 +54,7 @@ func (s *Server) UpdateConfig(f func(*model.Config)) { old := s.Config() updated := old.Clone() f(updated) - if _, err := s.configStore.Set(updated); err != nil { + if _, _, err := s.configStore.Set(updated); err != nil { mlog.Error("Failed to update config", mlog.Err(err)) } } @@ -405,12 +405,13 @@ func (a *App) GetEnvironmentConfig(filter func(reflect.StructField) bool) map[st } // SaveConfig replaces the active configuration, optionally notifying cluster peers. -func (s *Server) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) *model.AppError { - oldCfg, err := s.configStore.Set(newCfg) +// It returns both the previous and current configs. +func (s *Server) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) (*model.Config, *model.Config, *model.AppError) { + oldCfg, newCfg, err := s.configStore.Set(newCfg) if errors.Cause(err) == config.ErrReadOnlyConfiguration { - return model.NewAppError("saveConfig", "ent.cluster.save_config.error", nil, err.Error(), http.StatusForbidden) + return nil, nil, model.NewAppError("saveConfig", "ent.cluster.save_config.error", nil, err.Error(), http.StatusForbidden) } else if err != nil { - return model.NewAppError("saveConfig", "app.save_config.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, nil, model.NewAppError("saveConfig", "app.save_config.app_error", nil, err.Error(), http.StatusInternalServerError) } if s.startMetrics && *s.Config().MetricsSettings.Enable { @@ -423,19 +424,18 @@ func (s *Server) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage } if s.Cluster != nil { - newCfg = s.configStore.RemoveEnvironmentOverrides(newCfg) - oldCfg = s.configStore.RemoveEnvironmentOverrides(oldCfg) - err := s.Cluster.ConfigChanged(oldCfg, newCfg, sendConfigChangeClusterMessage) + err := s.Cluster.ConfigChanged(s.configStore.RemoveEnvironmentOverrides(oldCfg), + s.configStore.RemoveEnvironmentOverrides(newCfg), sendConfigChangeClusterMessage) if err != nil { - return err + return nil, nil, err } } - return nil + return oldCfg, newCfg, nil } // SaveConfig replaces the active configuration, optionally notifying cluster peers. -func (a *App) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) *model.AppError { +func (a *App) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) (*model.Config, *model.Config, *model.AppError) { return a.Srv().SaveConfig(newCfg, sendConfigChangeClusterMessage) } diff --git a/app/migrations.go b/app/migrations.go index 3486e76a57..db52b89f11 100644 --- a/app/migrations.go +++ b/app/migrations.go @@ -70,7 +70,7 @@ func (s *Server) doAdvancedPermissionsMigration() { config := s.Config() if *config.ServiceSettings.DEPRECATED_DO_NOT_USE_AllowEditPost == model.ALLOW_EDIT_POST_ALWAYS { *config.ServiceSettings.PostEditTimeLimit = -1 - if err := s.SaveConfig(config, true); err != nil { + if _, _, err := s.SaveConfig(config, true); err != nil { mlog.Error("Failed to update config in Advanced Permissions Phase 1 Migration.", mlog.Err(err)) } } diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index defab3a207..2c1a7a2f64 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -13392,7 +13392,7 @@ func (a *OpenTracingAppLayer) SaveComplianceReport(job *model.Compliance) (*mode return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) *model.AppError { +func (a *OpenTracingAppLayer) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) (*model.Config, *model.Config, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SaveConfig") @@ -13404,14 +13404,14 @@ func (a *OpenTracingAppLayer) SaveConfig(newCfg *model.Config, sendConfigChangeC }() defer span.Finish() - resultVar0 := a.app.SaveConfig(newCfg, sendConfigChangeClusterMessage) + resultVar0, resultVar1, resultVar2 := a.app.SaveConfig(newCfg, sendConfigChangeClusterMessage) - if resultVar0 != nil { - span.LogFields(spanlog.Error(resultVar0)) + if resultVar2 != nil { + span.LogFields(spanlog.Error(resultVar2)) ext.Error.Set(span, true) } - return resultVar0 + return resultVar0, resultVar1, resultVar2 } func (a *OpenTracingAppLayer) SaveReactionForPost(c *request.Context, reaction *model.Reaction) (*model.Reaction, *model.AppError) { diff --git a/app/plugin.go b/app/plugin.go index 1440d08c9d..bba8b1eaa0 100644 --- a/app/plugin.go +++ b/app/plugin.go @@ -402,7 +402,7 @@ func (s *Server) enablePlugin(id string) *model.AppError { }) // This call will implicitly invoke SyncPluginsActiveState which will activate enabled plugins. - if err := s.SaveConfig(s.Config(), true); err != nil { + if _, _, err := s.SaveConfig(s.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) } @@ -449,7 +449,7 @@ func (s *Server) disablePlugin(id string) *model.AppError { s.unregisterPluginCommands(id) // This call will implicitly invoke SyncPluginsActiveState which will deactivate disabled plugins. - if err := s.SaveConfig(s.Config(), true); err != nil { + if _, _, err := s.SaveConfig(s.Config(), true); err != nil { return model.NewAppError("DisablePlugin", "app.plugin.config.app_error", nil, err.Error(), http.StatusInternalServerError) } diff --git a/app/plugin_api.go b/app/plugin_api.go index 6a5fccf271..c3ab9ea97d 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -109,7 +109,8 @@ func (api *PluginAPI) GetUnsanitizedConfig() *model.Config { } func (api *PluginAPI) SaveConfig(config *model.Config) *model.AppError { - return api.app.SaveConfig(config, true) + _, _, err := api.app.SaveConfig(config, true) + return err } func (api *PluginAPI) GetPluginConfig() map[string]interface{} { @@ -123,7 +124,8 @@ func (api *PluginAPI) GetPluginConfig() map[string]interface{} { func (api *PluginAPI) SavePluginConfig(pluginConfig map[string]interface{}) *model.AppError { cfg := api.app.GetSanitizedConfig() cfg.PluginSettings.Plugins[api.manifest.Id] = pluginConfig - return api.app.SaveConfig(cfg, true) + _, _, err := api.app.SaveConfig(cfg, true) + return err } func (api *PluginAPI) GetBundlePath() (string, error) { diff --git a/cmd/mattermost/commands/config.go b/cmd/mattermost/commands/config.go index dfe1cc4006..e2674da831 100644 --- a/cmd/mattermost/commands/config.go +++ b/cmd/mattermost/commands/config.go @@ -14,6 +14,7 @@ import ( "github.com/pkg/errors" "github.com/spf13/cobra" + "github.com/mattermost/mattermost-server/v5/audit" "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/shared/i18n" @@ -236,8 +237,8 @@ func configSetCmdF(command *cobra.Command, args []string) error { newVal := args[1:] // create the function to update config - oldConfig := configStore.Get() - newConfig := configStore.Get() + oldConfig := configStore.Get().Clone() + newConfig := configStore.Get().Clone() f := updateConfigValue(configSetting, newVal, oldConfig, newConfig) f(newConfig) @@ -249,22 +250,24 @@ func configSetCmdF(command *cobra.Command, args []string) error { return errors.New("Invalid locale configuration") } - if _, errSet := configStore.Set(newConfig); errSet != nil { + oldCfg, newCfg, errSet := configStore.Set(newConfig) + if errSet != nil { return errors.Wrap(errSet, "failed to set config") } - /* - Uncomment when CI unit test fail resolved. + a, errInit := InitDBCommandContextCobra(command) + if errInit != nil { + return errInit + } + defer a.Srv().Shutdown() - a, errInit := InitDBCommandContextCobra(command) - if errInit == nil { - auditRec := a.MakeAuditRecord("configSet", audit.Success) - auditRec.AddMeta("setting", configSetting) - auditRec.AddMeta("new_value", newVal) - a.LogAuditRec(auditRec, nil) - a.Srv().Shutdown() - } - */ + auditRec := a.MakeAuditRecord("configSet", audit.Success) + defer a.LogAuditRec(auditRec, nil) + diffs, diffErr := config.Diff(oldCfg, newCfg) + if diffErr != nil { + return errors.Wrap(diffErr, "failed to diff configs") + } + auditRec.AddMeta("diff", diffs) return nil } @@ -423,14 +426,37 @@ func configResetCmdF(command *cobra.Command, args []string) error { return err } + a, errInit := InitDBCommandContextCobra(command) + if errInit != nil { + return errInit + } + defer a.Srv().Shutdown() + + var oldCfg *model.Config + var newCfg *model.Config + + defer func() { + auditRec := a.MakeAuditRecord("configReset", audit.Success) + if oldCfg != nil && newCfg != nil { + diffs, diffErr := config.Diff(oldCfg, newCfg) + if diffErr != nil { + mlog.Warn("Failed to diff configs", mlog.Err(diffErr)) + return + } + auditRec.AddMeta("diff", diffs) + } + a.LogAuditRec(auditRec, nil) + }() + defaultConfig := &model.Config{} defaultConfig.SetDefaults() confirmFlag, _ := command.Flags().GetBool("confirm") if confirmFlag { - if _, err = configStore.Set(defaultConfig); err != nil { + if oldCfg, newCfg, err = configStore.Set(defaultConfig); err != nil { return errors.Wrap(err, "failed to set config") } + return nil } if !confirmFlag && len(args) == 0 { @@ -438,13 +464,14 @@ func configResetCmdF(command *cobra.Command, args []string) error { CommandPrettyPrintln("Are you sure you want to reset all the configuration settings?(YES/NO): ") fmt.Scanln(&confirmResetAll) if confirmResetAll == "YES" { - if _, err = configStore.Set(defaultConfig); err != nil { + if oldCfg, newCfg, err = configStore.Set(defaultConfig); err != nil { return errors.Wrap(err, "failed to set config") } } + return nil } - tempConfig := configStore.Get() + tempConfig := configStore.Get().Clone() tempConfigMap := configToMap(*tempConfig) defaultConfigMap := configToMap(*defaultConfig) for _, arg := range args { @@ -467,21 +494,11 @@ func configResetCmdF(command *cobra.Command, args []string) error { return errors.New("Invalid locale configuration") } - if _, errSet := configStore.Set(tempConfig); errSet != nil { - return errors.Wrap(errSet, "failed to set config") + oldCfg, newCfg, err = configStore.Set(tempConfig) + if err != nil { + return errors.Wrap(err, "failed to set config") } - /* - Uncomment when CI unit test fail resolved. - - a, errInit := InitDBCommandContextCobra(command) - if errInit == nil { - auditRec := a.MakeAuditRecord("configReset", audit.Success) - a.LogAuditRec(auditRec, nil) - a.Srv().Shutdown() - } - */ - return nil } diff --git a/cmd/mattermost/commands/config_test.go b/cmd/mattermost/commands/config_test.go index 03b8bf25c0..278f7cafd3 100644 --- a/cmd/mattermost/commands/config_test.go +++ b/cmd/mattermost/commands/config_test.go @@ -207,6 +207,8 @@ func TestConfigReset(t *testing.T) { }) t.Run("Success when the confirm boolean flag is given", func(t *testing.T) { + th := Setup(t) + defer th.TearDown() assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunJobs", "false")) assert.NoError(t, th.RunCommand(t, "config", "set", "PrivacySettings.ShowFullName", "false")) assert.NoError(t, th.RunCommand(t, "config", "reset", "--confirm")) @@ -217,10 +219,20 @@ func TestConfigReset(t *testing.T) { }) t.Run("Success when a configuration section is given", func(t *testing.T) { - assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunJobs", "false")) - assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunScheduler", "false")) - assert.NoError(t, th.RunCommand(t, "config", "set", "PrivacySettings.ShowFullName", "false")) - assert.NoError(t, th.RunCommand(t, "config", "reset", "JobSettings")) + th := Setup(t) + defer th.TearDown() + output, err := th.RunCommandWithOutput(t, "config", "set", "JobSettings.RunJobs", "false") + assert.NoErrorf(t, err, "output %s", output) + + output, err = th.RunCommandWithOutput(t, "config", "set", "JobSettings.RunScheduler", "false") + assert.NoErrorf(t, err, "output %s", output) + + output, err = th.RunCommandWithOutput(t, "config", "set", "PrivacySettings.ShowFullName", "false") + assert.NoErrorf(t, err, "output %s", output) + + output, err = th.RunCommandWithOutput(t, "config", "reset", "JobSettings") + assert.NoErrorf(t, err, "output %s", output) + output1 := th.CheckCommand(t, "config", "get", "JobSettings.RunJobs") output2 := th.CheckCommand(t, "config", "get", "JobSettings.RunScheduler") output3 := th.CheckCommand(t, "config", "get", "PrivacySettings.ShowFullName") @@ -230,9 +242,18 @@ func TestConfigReset(t *testing.T) { }) t.Run("Success when a configuration setting is given", func(t *testing.T) { - assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunJobs", "false")) - assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunScheduler", "false")) - assert.NoError(t, th.RunCommand(t, "config", "reset", "JobSettings.RunJobs")) + th := Setup(t) + defer th.TearDown() + + output, err := th.RunCommandWithOutput(t, "config", "set", "JobSettings.RunJobs", "false") + assert.NoErrorf(t, err, "output %s", output) + + output, err = th.RunCommandWithOutput(t, "config", "set", "JobSettings.RunScheduler", "false") + assert.NoErrorf(t, err, "output %s", output) + + output, err = th.RunCommandWithOutput(t, "config", "reset", "JobSettings.RunJobs") + assert.NoErrorf(t, err, "output %s", output) + output1 := th.CheckCommand(t, "config", "get", "JobSettings.RunJobs") output2 := th.CheckCommand(t, "config", "get", "JobSettings.RunScheduler") assert.Contains(t, output1, "true") diff --git a/config/common_test.go b/config/common_test.go index ba56cc311e..bb9c250e68 100644 --- a/config/common_test.go +++ b/config/common_test.go @@ -160,7 +160,7 @@ func TestConfigEnvironmentOverrides(t *testing.T) { }) t.Run("setting config should respect environment variable overrides", func(t *testing.T) { - _, err := base.Set(originalConfig) + _, _, err := base.Set(originalConfig) require.NoError(t, err) assert.Equal(t, "http://overridden.ca", *base.Get().ServiceSettings.SiteURL) diff --git a/config/database_test.go b/config/database_test.go index ad0eb02d31..0a6173c3ac 100644 --- a/config/database_test.go +++ b/config/database_test.go @@ -420,7 +420,7 @@ func TestDatabaseStoreSet(t *testing.T) { require.NoError(t, err) defer ds.Close() - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) if assert.Error(t, err) { assert.EqualError(t, err, "old configuration modified instead of cloning") } @@ -436,7 +436,7 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := &model.Config{} - _, err = ds.Set(newCfg) + _, _, err = ds.Set(newCfg) require.NoError(t, err) assert.Equal(t, "", *ds.Get().ServiceSettings.SiteURL) @@ -453,7 +453,7 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := &model.Config{} newCfg.LdapSettings.BindPassword = model.NewString(model.FAKE_SETTING) - _, err = ds.Set(newCfg) + _, _, err = ds.Set(newCfg) require.NoError(t, err) assert.Equal(t, "password", *ds.Get().LdapSettings.BindPassword) @@ -470,7 +470,7 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := &model.Config{} newCfg.ServiceSettings.SiteURL = model.NewString("invalid") - _, err = ds.Set(newCfg) + _, _, err = ds.Set(newCfg) if assert.Error(t, err) { assert.EqualError(t, err, "new configuration is invalid: Config.IsValid: model.config.is_valid.site_url.app_error, ") } @@ -486,11 +486,11 @@ func TestDatabaseStoreSet(t *testing.T) { require.NoError(t, err) defer ds.Close() - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) require.NoError(t, err) beforeID, _ := getActualDatabaseConfig(t) - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) require.NoError(t, err) afterID, _ := getActualDatabaseConfig(t) @@ -511,7 +511,7 @@ func TestDatabaseStoreSet(t *testing.T) { }, } - _, err = ds.Set(newCfg) + _, _, err = ds.Set(newCfg) require.NoError(t, err) assert.Equal(t, "http://new", *ds.Get().ServiceSettings.SiteURL) @@ -531,7 +531,7 @@ func TestDatabaseStoreSet(t *testing.T) { }, } - _, err = ds.Set(newCfg) + _, _, err = ds.Set(newCfg) require.NoError(t, err) err = ds.Load() @@ -555,7 +555,7 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := minimalConfig - _, err = ds.Set(newCfg) + _, _, err = ds.Set(newCfg) require.Error(t, err) assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: failed to query active configuration"), "unexpected error: "+err.Error()) @@ -577,7 +577,7 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := emptyConfig.Clone() newCfg.ServiceSettings.SiteURL = model.NewString(longSiteURL) - _, err = ds.Set(newCfg) + _, _, err = ds.Set(newCfg) require.Error(t, err) assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: marshalled configuration failed length check: value is too long"), "unexpected error: "+err.Error()) }) @@ -598,7 +598,7 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := minimalConfig - _, err = ds.Set(newCfg) + _, _, err = ds.Set(newCfg) require.NoError(t, err) id, _ := getActualDatabaseConfig(t) @@ -616,7 +616,7 @@ func TestDatabaseStoreSet(t *testing.T) { defer ds.Close() ds.SetReadOnlyFF(true) - _, err = ds.Set(minimalConfig) + _, _, err = ds.Set(minimalConfig) require.NoError(t, err) assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL) @@ -633,7 +633,7 @@ func TestDatabaseStoreSet(t *testing.T) { ds.SetReadOnlyFF(false) - _, err = ds.Set(minimalConfig) + _, _, err = ds.Set(minimalConfig) require.NoError(t, err) assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL) @@ -692,7 +692,7 @@ func TestDatabaseStoreLoad(t *testing.T) { require.NoError(t, err) defer ds.Close() - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) require.NoError(t, err) assert.Equal(t, "http://overridePersistEnvVariables", *ds.Get().ServiceSettings.SiteURL) @@ -715,7 +715,7 @@ func TestDatabaseStoreLoad(t *testing.T) { assert.Equal(t, true, *ds.Get().PluginSettings.EnableUploads) - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) require.NoError(t, err) assert.Equal(t, true, *ds.Get().PluginSettings.EnableUploads) @@ -738,7 +738,7 @@ func TestDatabaseStoreLoad(t *testing.T) { assert.Equal(t, 3000, *ds.Get().TeamSettings.MaxUsersPerTeam) - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) require.NoError(t, err) assert.Equal(t, 3000, *ds.Get().TeamSettings.MaxUsersPerTeam) @@ -761,7 +761,7 @@ func TestDatabaseStoreLoad(t *testing.T) { assert.Equal(t, int64(123456), *ds.Get().ServiceSettings.TLSStrictTransportMaxAge) - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) require.NoError(t, err) assert.Equal(t, int64(123456), *ds.Get().ServiceSettings.TLSStrictTransportMaxAge) @@ -784,7 +784,7 @@ func TestDatabaseStoreLoad(t *testing.T) { assert.Equal(t, []string{"user:pwd@db:5432/test-db"}, ds.Get().SqlSettings.DataSourceReplicas) - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) require.NoError(t, err) assert.Equal(t, []string{"user:pwd@db:5432/test-db"}, ds.Get().SqlSettings.DataSourceReplicas) @@ -809,7 +809,7 @@ func TestDatabaseStoreLoad(t *testing.T) { assert.Equal(t, []string{"user:pwd@db:5432/test-db"}, ds.Get().SqlSettings.DataSourceReplicas) - _, err = ds.Set(ds.Get()) + _, _, err = ds.Set(ds.Get()) require.NoError(t, err) assert.Equal(t, []string{"user:pwd@db:5432/test-db"}, ds.Get().SqlSettings.DataSourceReplicas) diff --git a/config/diff.go b/config/diff.go new file mode 100644 index 0000000000..2a6058774b --- /dev/null +++ b/config/diff.go @@ -0,0 +1,90 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package config + +import ( + "fmt" + "reflect" + + "github.com/mattermost/mattermost-server/v5/model" +) + +type ConfigDiffs []ConfigDiff + +type ConfigDiff struct { + Path string `json:"path"` + BaseVal interface{} `json:"base_val"` + ActualVal interface{} `json:"actual_val"` +} + +func diff(base, actual reflect.Value, label string) ([]ConfigDiff, error) { + var diffs []ConfigDiff + + if base.IsZero() && actual.IsZero() { + return diffs, nil + } + + if base.IsZero() || actual.IsZero() { + return append(diffs, ConfigDiff{ + Path: label, + BaseVal: base.Interface(), + ActualVal: actual.Interface(), + }), nil + } + + baseType := base.Type() + actualType := actual.Type() + + if baseType.Kind() == reflect.Ptr { + base = reflect.Indirect(base) + actual = reflect.Indirect(actual) + baseType = base.Type() + actualType = actual.Type() + } + + if baseType != actualType { + return nil, fmt.Errorf("not same type %s %s", baseType, actualType) + } + + switch baseType.Kind() { + case reflect.Struct: + if base.NumField() != actual.NumField() { + return nil, fmt.Errorf("not same number of fields in struct") + } + for i := 0; i < base.NumField(); i++ { + fieldLabel := baseType.Field(i).Name + if label != "" { + fieldLabel = label + "." + fieldLabel + } + d, err := diff(base.Field(i), actual.Field(i), fieldLabel) + if err != nil { + return nil, err + } + diffs = append(diffs, d...) + } + default: + if !reflect.DeepEqual(base.Interface(), actual.Interface()) { + diffs = append(diffs, ConfigDiff{ + Path: label, + BaseVal: base.Interface(), + ActualVal: actual.Interface(), + }) + } + } + + return diffs, nil +} + +func Diff(base, actual *model.Config) (ConfigDiffs, error) { + if base == nil || actual == nil { + return nil, fmt.Errorf("input configs should not be nil") + } + baseVal := reflect.Indirect(reflect.ValueOf(base)) + actualVal := reflect.Indirect(reflect.ValueOf(actual)) + return diff(baseVal, actualVal, "") +} + +func (cd ConfigDiffs) String() string { + return fmt.Sprintf("%+v", []ConfigDiff(cd)) +} diff --git a/config/diff_test.go b/config/diff_test.go new file mode 100644 index 0000000000..c1a9162890 --- /dev/null +++ b/config/diff_test.go @@ -0,0 +1,466 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package config + +import ( + "testing" + + "github.com/mattermost/mattermost-server/v5/model" + + "github.com/stretchr/testify/require" +) + +func defaultConfigGen() *model.Config { + cfg := &model.Config{} + cfg.SetDefaults() + return cfg +} + +func BenchmarkDiff(b *testing.B) { + b.Run("equal empty", func(b *testing.B) { + baseCfg := &model.Config{} + actualCfg := &model.Config{} + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _ = Diff(baseCfg, actualCfg) + } + }) + + b.Run("equal with defaults", func(b *testing.B) { + baseCfg := defaultConfigGen() + actualCfg := defaultConfigGen() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _ = Diff(baseCfg, actualCfg) + } + }) + + b.Run("actual empty", func(b *testing.B) { + baseCfg := defaultConfigGen() + actualCfg := &model.Config{} + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _ = Diff(baseCfg, actualCfg) + } + }) + + b.Run("base empty", func(b *testing.B) { + baseCfg := &model.Config{} + actualCfg := defaultConfigGen() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _ = Diff(baseCfg, actualCfg) + } + }) + + b.Run("some diffs", func(b *testing.B) { + baseCfg := defaultConfigGen() + actualCfg := defaultConfigGen() + baseCfg.ServiceSettings.SiteURL = model.NewString("http://localhost") + baseCfg.ServiceSettings.ReadTimeout = model.NewInt(300) + baseCfg.SqlSettings.QueryTimeout = model.NewInt(0) + actualCfg.PluginSettings.EnableUploads = nil + actualCfg.TeamSettings.MaxChannelsPerTeam = model.NewInt64(100000) + actualCfg.FeatureFlags = nil + actualCfg.SqlSettings.DataSourceReplicas = []string{ + "ds0", + "ds1", + "ds2", + } + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _ = Diff(baseCfg, actualCfg) + } + }) +} + +func TestDiff(t *testing.T) { + tcs := []struct { + name string + base *model.Config + actual *model.Config + diffs ConfigDiffs + err string + }{ + { + "nil", + nil, + nil, + nil, + "input configs should not be nil", + }, + { + "empty", + &model.Config{}, + &model.Config{}, + nil, + "", + }, + { + "defaults", + defaultConfigGen(), + defaultConfigGen(), + nil, + "", + }, + { + "default base, actual empty", + defaultConfigGen(), + &model.Config{}, + ConfigDiffs{ + { + "", + *defaultConfigGen(), + model.Config{}, + }, + }, + "", + }, + { + "empty base, actual default", + &model.Config{}, + defaultConfigGen(), + ConfigDiffs{ + { + "", + model.Config{}, + *defaultConfigGen(), + }, + }, + "", + }, + { + "string change", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.SiteURL = model.NewString("http://changed") + return cfg + }(), + ConfigDiffs{ + { + Path: "ServiceSettings.SiteURL", + BaseVal: *defaultConfigGen().ServiceSettings.SiteURL, + ActualVal: "http://changed", + }, + }, + "", + }, + { + "string nil", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.SiteURL = nil + return cfg + }(), + ConfigDiffs{ + { + Path: "ServiceSettings.SiteURL", + BaseVal: defaultConfigGen().ServiceSettings.SiteURL, + ActualVal: func() *string { + return nil + }(), + }, + }, + "", + }, + { + "bool change", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.PluginSettings.Enable = model.NewBool(!*cfg.PluginSettings.Enable) + return cfg + }(), + ConfigDiffs{ + { + Path: "PluginSettings.Enable", + BaseVal: true, + ActualVal: false, + }, + }, + "", + }, + { + "bool nil", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.PluginSettings.Enable = nil + return cfg + }(), + ConfigDiffs{ + { + Path: "PluginSettings.Enable", + BaseVal: defaultConfigGen().PluginSettings.Enable, + ActualVal: func() *bool { + return nil + }(), + }, + }, + "", + }, + { + "int change", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.ReadTimeout = model.NewInt(0) + return cfg + }(), + ConfigDiffs{ + { + Path: "ServiceSettings.ReadTimeout", + BaseVal: *defaultConfigGen().ServiceSettings.ReadTimeout, + ActualVal: 0, + }, + }, + "", + }, + { + "int nil", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.ReadTimeout = nil + return cfg + }(), + ConfigDiffs{ + { + Path: "ServiceSettings.ReadTimeout", + BaseVal: defaultConfigGen().ServiceSettings.ReadTimeout, + ActualVal: func() *int { + return nil + }(), + }, + }, + "", + }, + { + "slice addition", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceReplicas = []string{ + "ds0", + "ds1", + } + return cfg + }(), + ConfigDiffs{ + { + Path: "SqlSettings.DataSourceReplicas", + BaseVal: defaultConfigGen().SqlSettings.DataSourceReplicas, + ActualVal: []string{ + "ds0", + "ds1", + }, + }, + }, + "", + }, + { + "slice deletion", + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceReplicas = []string{ + "ds0", + "ds1", + } + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceReplicas = []string{ + "ds0", + } + return cfg + }(), + ConfigDiffs{ + { + Path: "SqlSettings.DataSourceReplicas", + BaseVal: []string{ + "ds0", + "ds1", + }, + ActualVal: []string{ + "ds0", + }, + }, + }, + "", + }, + { + "slice nil", + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceReplicas = []string{ + "ds0", + "ds1", + } + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceReplicas = nil + return cfg + }(), + ConfigDiffs{ + { + Path: "SqlSettings.DataSourceReplicas", + BaseVal: []string{ + "ds0", + "ds1", + }, + ActualVal: func() []string { + return nil + }(), + }, + }, + "", + }, + { + "map change", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.PluginSettings.PluginStates["com.mattermost.nps"] = &model.PluginState{ + Enable: !cfg.PluginSettings.PluginStates["com.mattermost.nps"].Enable, + } + return cfg + }(), + ConfigDiffs{ + { + Path: "PluginSettings.PluginStates", + BaseVal: defaultConfigGen().PluginSettings.PluginStates, + ActualVal: map[string]*model.PluginState{ + "com.mattermost.nps": { + Enable: !defaultConfigGen().PluginSettings.PluginStates["com.mattermost.nps"].Enable, + }, + }, + }, + }, + "", + }, + { + "map addition", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.PluginSettings.PluginStates["com.mattermost.newplugin"] = &model.PluginState{ + Enable: true, + } + return cfg + }(), + ConfigDiffs{ + { + Path: "PluginSettings.PluginStates", + BaseVal: defaultConfigGen().PluginSettings.PluginStates, + ActualVal: map[string]*model.PluginState{ + "com.mattermost.nps": { + Enable: defaultConfigGen().PluginSettings.PluginStates["com.mattermost.nps"].Enable, + }, + "com.mattermost.newplugin": { + Enable: true, + }, + }, + }, + }, + "", + }, + { + "map deletion", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + delete(cfg.PluginSettings.PluginStates, "com.mattermost.nps") + return cfg + }(), + ConfigDiffs{ + { + Path: "PluginSettings.PluginStates", + BaseVal: defaultConfigGen().PluginSettings.PluginStates, + ActualVal: func() interface{} { + return map[string]*model.PluginState{} + }(), + }, + }, + "", + }, + { + "map nil", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.PluginSettings.PluginStates = nil + return cfg + }(), + ConfigDiffs{ + { + Path: "PluginSettings.PluginStates", + BaseVal: defaultConfigGen().PluginSettings.PluginStates, + ActualVal: func() map[string]*model.PluginState { + return nil + }(), + }, + }, + "", + }, + { + "map type change", + func() *model.Config { + cfg := defaultConfigGen() + cfg.PluginSettings.Plugins = map[string]map[string]interface{}{ + "com.mattermost.newplugin": { + "key": true, + }, + } + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.PluginSettings.Plugins = map[string]map[string]interface{}{ + "com.mattermost.newplugin": { + "key": "string", + }, + } + return cfg + }(), + ConfigDiffs{ + { + Path: "PluginSettings.Plugins", + BaseVal: func() interface{} { + return map[string]map[string]interface{}{ + "com.mattermost.newplugin": { + "key": true, + }, + } + }(), + ActualVal: func() interface{} { + return map[string]map[string]interface{}{ + "com.mattermost.newplugin": { + "key": "string", + }, + } + }(), + }, + }, + "", + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + diffs, err := Diff(tc.base, tc.actual) + if tc.err != "" { + require.EqualError(t, err, tc.err) + require.Nil(t, diffs) + } else { + require.NoError(t, err) + } + require.Equal(t, tc.diffs, diffs) + }) + } +} diff --git a/config/file_test.go b/config/file_test.go index e8f234b698..1a5f1beaba 100644 --- a/config/file_test.go +++ b/config/file_test.go @@ -273,7 +273,7 @@ func TestFileStoreGet(t *testing.T) { assert.True(t, cfg == cfg2, "Get() returned different configuration instances") newCfg := &model.Config{} - _, err := configStore.Set(newCfg) + _, _, err := configStore.Set(newCfg) require.NoError(t, err) assert.False(t, newCfg == cfg, "returned config should have been different from original") @@ -472,9 +472,10 @@ func TestFileStoreSet(t *testing.T) { oldCfg := configStore.Get().Clone() newCfg := &model.Config{} - retCfg, err := configStore.Set(newCfg) + retCfg, newConfig, err := configStore.Set(newCfg) require.NoError(t, err) require.Equal(t, oldCfg, retCfg) + require.NotEqual(t, newCfg, newConfig) assert.Equal(t, "", *configStore.Get().ServiceSettings.SiteURL) }) @@ -486,8 +487,9 @@ func TestFileStoreSet(t *testing.T) { newCfg := &model.Config{} newCfg.LdapSettings.BindPassword = model.NewString(model.FAKE_SETTING) - _, err := configStore.Set(newCfg) + _, newConfig, err := configStore.Set(newCfg) require.NoError(t, err) + require.NotEqual(t, newCfg, newConfig) assert.Equal(t, "password", *configStore.Get().LdapSettings.BindPassword) }) @@ -499,7 +501,7 @@ func TestFileStoreSet(t *testing.T) { newCfg := &model.Config{} newCfg.ServiceSettings.SiteURL = model.NewString("invalid") - _, err := configStore.Set(newCfg) + _, _, err := configStore.Set(newCfg) if assert.Error(t, err) { assert.EqualError(t, err, "new configuration is invalid: Config.IsValid: model.config.is_valid.site_url.app_error, ") } @@ -515,7 +517,7 @@ func TestFileStoreSet(t *testing.T) { newReadOnlyConfig.ServiceSettings = model.ServiceSettings{ SiteURL: model.NewString("http://test"), } - _, err := configStore.Set(newReadOnlyConfig) + _, _, err := configStore.Set(newReadOnlyConfig) if assert.Error(t, err) { assert.Equal(t, ErrReadOnlyConfiguration, errors.Cause(err)) } @@ -537,7 +539,7 @@ func TestFileStoreSet(t *testing.T) { newCfg := &model.Config{} - _, err = fs.Set(newCfg) + _, _, err = fs.Set(newCfg) if assert.Error(t, err) { assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: failed to write file")) } @@ -558,7 +560,7 @@ func TestFileStoreSet(t *testing.T) { newCfg := minimalConfig - _, err := configStore.Set(newCfg) + _, _, err := configStore.Set(newCfg) require.NoError(t, err) require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written") @@ -583,7 +585,7 @@ func TestFileStoreSet(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") - _, err := configStore.Set(newCfg) + _, _, err := configStore.Set(newCfg) require.NoError(t, err) require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config changed") @@ -608,7 +610,7 @@ func TestFileStoreSet(t *testing.T) { expectedNewConfig = minimalConfig.Clone() expectedNewConfig.FeatureFlags.TestFeature = "test" - _, err := configStore.Set(expectedNewConfig) + _, _, err := configStore.Set(expectedNewConfig) require.NoError(t, err) require.False(t, wasCalled(called, 5*time.Second)) @@ -616,7 +618,7 @@ func TestFileStoreSet(t *testing.T) { configStore.SetReadOnlyFF(false) expectedNewConfig.FeatureFlags.TestFeature = "test2" - _, err = configStore.Set(expectedNewConfig) + _, _, err = configStore.Set(expectedNewConfig) require.NoError(t, err) require.True(t, wasCalled(called, 5*time.Second)) @@ -636,7 +638,7 @@ func TestFileStoreSet(t *testing.T) { require.NoError(t, err) defer fs.Close() - _, err = fs.Set(minimalConfig) + _, _, err = fs.Set(minimalConfig) require.NoError(t, err) // Let the initial call to invokeConfigListeners finish. @@ -705,7 +707,7 @@ func TestFileStoreLoad(t *testing.T) { assert.Equal(t, "http://overridePersistEnvVariables", *fs.Get().ServiceSettings.SiteURL) - _, err = fs.Set(fs.Get()) + _, _, err = fs.Set(fs.Get()) require.NoError(t, err) assert.Equal(t, "http://overridePersistEnvVariables", *fs.Get().ServiceSettings.SiteURL) @@ -730,7 +732,7 @@ func TestFileStoreLoad(t *testing.T) { assert.Equal(t, true, *fs.Get().PluginSettings.EnableUploads) - _, err = fs.Set(fs.Get()) + _, _, err = fs.Set(fs.Get()) require.NoError(t, err) assert.Equal(t, true, *fs.Get().PluginSettings.EnableUploads) @@ -755,7 +757,7 @@ func TestFileStoreLoad(t *testing.T) { assert.Equal(t, 3000, *fs.Get().TeamSettings.MaxUsersPerTeam) - _, err = fs.Set(fs.Get()) + _, _, err = fs.Set(fs.Get()) require.NoError(t, err) assert.Equal(t, 3000, *fs.Get().TeamSettings.MaxUsersPerTeam) @@ -780,7 +782,7 @@ func TestFileStoreLoad(t *testing.T) { assert.Equal(t, int64(123456), *fs.Get().ServiceSettings.TLSStrictTransportMaxAge) - _, err = fs.Set(fs.Get()) + _, _, err = fs.Set(fs.Get()) require.NoError(t, err) assert.Equal(t, int64(123456), *fs.Get().ServiceSettings.TLSStrictTransportMaxAge) @@ -805,7 +807,7 @@ func TestFileStoreLoad(t *testing.T) { assert.Equal(t, []string{"user:pwd@db:5432/test-db"}, fs.Get().SqlSettings.DataSourceReplicas) - _, err = fs.Set(fs.Get()) + _, _, err = fs.Set(fs.Get()) require.NoError(t, err) assert.Equal(t, []string{"user:pwd@db:5432/test-db"}, fs.Get().SqlSettings.DataSourceReplicas) @@ -832,7 +834,7 @@ func TestFileStoreLoad(t *testing.T) { assert.Equal(t, []string{"user:pwd@db:5432/test-db"}, fs.Get().SqlSettings.DataSourceReplicas) - _, err = fs.Set(fs.Get()) + _, _, err = fs.Set(fs.Get()) require.NoError(t, err) assert.Equal(t, []string{"user:pwd@db:5432/test-db"}, fs.Get().SqlSettings.DataSourceReplicas) @@ -871,7 +873,7 @@ func TestFileStoreLoad(t *testing.T) { defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") newCfg := minimalConfig - _, err := configStore.Set(newCfg) + _, _, err := configStore.Set(newCfg) require.Error(t, err) require.EqualError(t, err, "new configuration is invalid: Config.IsValid: model.config.is_valid.site_url.app_error, ") }) @@ -1043,7 +1045,7 @@ func TestFileStoreWatcherEmitter(t *testing.T) { } fs.AddListener(callback) - _, err = fs.Set(minimalConfig) + _, _, err = fs.Set(minimalConfig) require.NoError(t, err) require.False(t, wasCalled(called, 1*time.Second), "callback should not have been called since no change has happened") @@ -1074,7 +1076,7 @@ func TestFileStoreWatcherEmitter(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") - _, err = fs.Set(minimalConfig) + _, _, err = fs.Set(minimalConfig) require.NoError(t, err) require.True(t, wasCalled(called, 5*time.Second), "callback should have been called since no change has happened") @@ -1092,7 +1094,7 @@ func TestFileStoreSave(t *testing.T) { } t.Run("set with automatic save", func(t *testing.T) { - _, err := store.Set(newCfg) + _, _, err := store.Set(newCfg) require.NoError(t, err) err = store.Load() @@ -1421,7 +1423,7 @@ func TestFileStoreReadOnly(t *testing.T) { } fs.AddListener(callback) - cfg, err := fs.Set(minimalConfig) + cfg, _, err := fs.Set(minimalConfig) require.Nil(t, cfg) require.Equal(t, ErrReadOnlyStore, err) @@ -1439,7 +1441,7 @@ func TestFileStoreSetReadOnlyFF(t *testing.T) { newCfg.FeatureFlags.TestFeature = "test" // store has read-only FF by default. - _, err := store.Set(newCfg) + _, _, err := store.Set(newCfg) require.NoError(t, err) config = store.Get() @@ -1457,7 +1459,7 @@ func TestFileStoreSetReadOnlyFF(t *testing.T) { store.SetReadOnlyFF(false) - _, err := store.Set(newCfg) + _, _, err := store.Set(newCfg) require.NoError(t, err) config = store.Get() diff --git a/config/migrate.go b/config/migrate.go index 5e53685465..f651921a3d 100644 --- a/config/migrate.go +++ b/config/migrate.go @@ -22,7 +22,7 @@ func Migrate(from, to string) error { defer destination.Close() sourceConfig := source.Get() - if _, err = destination.Set(sourceConfig); err != nil { + if _, _, err = destination.Set(sourceConfig); err != nil { return errors.Wrapf(err, "failed to set config") } diff --git a/config/migrate_test.go b/config/migrate_test.go index b36601efc9..7009976843 100644 --- a/config/migrate_test.go +++ b/config/migrate_test.go @@ -68,7 +68,7 @@ func TestMigrate(t *testing.T) { "mysql://mmuser:password@tcp(searchreplicahost:3306)/mattermost", } - _, err := source.Set(cfg) + _, _, err := source.Set(cfg) require.NoError(t, err) for i, file := range files { @@ -77,7 +77,7 @@ func TestMigrate(t *testing.T) { } return func(store *Store) { - _, err := store.Set(originalCfg) + _, _, err := store.Set(originalCfg) require.NoError(t, err) } } diff --git a/config/store.go b/config/store.go index caba220982..ddd6d35849 100644 --- a/config/store.go +++ b/config/store.go @@ -170,12 +170,13 @@ func (s *Store) SetReadOnlyFF(readOnly bool) { } // Set replaces the current configuration in its entirety and updates the backing store. -func (s *Store) Set(newCfg *model.Config) (*model.Config, error) { +// It returns both old and new versions of the config. +func (s *Store) Set(newCfg *model.Config) (*model.Config, *model.Config, error) { s.configLock.Lock() defer s.configLock.Unlock() if s.readOnly { - return nil, ErrReadOnlyStore + return nil, nil, ErrReadOnlyStore } newCfg = newCfg.Clone() @@ -190,7 +191,7 @@ func (s *Store) Set(newCfg *model.Config) (*model.Config, error) { desanitize(oldCfg, newCfg) if err := newCfg.IsValid(); err != nil { - return nil, errors.Wrap(err, "new configuration is invalid") + return nil, nil, errors.Wrap(err, "new configuration is invalid") } // We attempt to remove any environment override that may be present in the input config. @@ -211,7 +212,7 @@ func (s *Store) Set(newCfg *model.Config) (*model.Config, error) { } if err := s.backingStore.Set(newCfgNoEnv); err != nil { - return nil, errors.Wrap(err, "failed to persist") + return nil, nil, errors.Wrap(err, "failed to persist") } // We apply back environment overrides since the input config may or @@ -219,12 +220,12 @@ func (s *Store) Set(newCfg *model.Config) (*model.Config, error) { newCfg = applyEnvironmentMap(newCfgNoEnv, GetEnvironment()) fixConfig(newCfg) if err := newCfg.IsValid(); err != nil { - return nil, errors.Wrap(err, "new configuration is invalid") + return nil, nil, errors.Wrap(err, "new configuration is invalid") } hasChanged, err := equal(oldCfg, newCfg) if err != nil { - return nil, errors.Wrap(err, "failed to compare configs") + return nil, nil, errors.Wrap(err, "failed to compare configs") } // We restore the previously cleared feature flags sections back. @@ -237,13 +238,15 @@ func (s *Store) Set(newCfg *model.Config) (*model.Config, error) { s.configNoEnv = newCfgNoEnv s.config = newCfg + newCfgCopy := newCfg.Clone() + if hasChanged { s.configLock.Unlock() - s.invokeConfigListeners(oldCfg, newCfg.Clone()) + s.invokeConfigListeners(oldCfg, newCfgCopy.Clone()) s.configLock.Lock() } - return oldCfg, nil + return oldCfg, newCfgCopy, nil } // Load updates the current configuration from the backing store, possibly initializing. @@ -337,9 +340,11 @@ func (s *Store) Load() error { s.config = loadedCfg s.configNoEnv = loadedCfgNoEnv + loadedCfgCopy := loadedCfg.Clone() + if hasChanged { s.configLock.Unlock() - s.invokeConfigListeners(oldCfg, loadedCfg.Clone()) + s.invokeConfigListeners(oldCfg, loadedCfgCopy) s.configLock.Lock() } diff --git a/config/store_test.go b/config/store_test.go index da5e20c751..e649cccbfe 100644 --- a/config/store_test.go +++ b/config/store_test.go @@ -70,8 +70,9 @@ func TestNewStoreReadOnly(t *testing.T) { require.NoError(t, err) t.Run("Set", func(t *testing.T) { - cfg, err := ds.Set(emptyConfig) - require.Nil(t, cfg) + oldCfg, newCfg, err := ds.Set(emptyConfig) + require.Nil(t, oldCfg) + require.Nil(t, newCfg) require.Equal(t, ErrReadOnlyStore, err) }) @@ -93,8 +94,9 @@ func TestNewStoreReadOnly(t *testing.T) { require.NoError(t, err) t.Run("Set", func(t *testing.T) { - cfg, err := fs.Set(emptyConfig) - require.Nil(t, cfg) + oldCfg, newCfg, err := fs.Set(emptyConfig) + require.Nil(t, oldCfg) + require.Nil(t, newCfg) require.Equal(t, ErrReadOnlyStore, err) }) diff --git a/i18n/en.json b/i18n/en.json index 06b3750938..5bc5f81c97 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1494,6 +1494,10 @@ "id": "api.config.migrate_config.app_error", "translation": "Failed to migrate config store." }, + { + "id": "api.config.patch_config.diff.app_error", + "translation": "Failed to diff configs" + }, { "id": "api.config.patch_config.restricted_merge.app_error", "translation": "Failed to merge given config." @@ -1502,6 +1506,10 @@ "id": "api.config.update_config.clear_siteurl.app_error", "translation": "Site URL cannot be cleared." }, + { + "id": "api.config.update_config.diff.app_error", + "translation": "Failed to diff configs" + }, { "id": "api.config.update_config.not_allowed_security.app_error", "translation": "Changing {{.Name}} is not allowed due to security reasons." diff --git a/web/context.go b/web/context.go index bd4d812f80..bd4bb2d65b 100644 --- a/web/context.go +++ b/web/context.go @@ -76,7 +76,6 @@ func (c *Context) LogAudit(extraInfo string) { } func (c *Context) LogAuditWithUserId(userId, extraInfo string) { - if c.AppContext.Session().UserId != "" { extraInfo = strings.TrimSpace(extraInfo + " session_user=" + c.AppContext.Session().UserId) }