[MM-28692] Include config diffs in audit record for config changing API calls (#17623)

* Replace config generator * Cleanup * Some renaming and docs additions to add clarity * Cleanup logging related methods * Cleanup emitter * Fix TestDefaultsGenerator * Move feature flags synchronization logic out of config package * Remove unnecessary util functions * Simplify load/set logic * Refine semantics and add some test to cover them * Remove unnecessary deep copies * Improve logic further * Fix license header * Review file store tests * Fix test * Fix test * Avoid additional write during initialization * More consistent naming * Update app/feature_flags.go Co-authored-by: Christopher Speller <crspeller@gmail.com> * Update config/store.go Co-authored-by: Christopher Speller <crspeller@gmail.com> * Update config/store.go Co-authored-by: Christopher Speller <crspeller@gmail.com> * Update config/store.go Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com> * Make ConfigStore.Set() return both old and new configs * Implement config diff function * Make app.SaveConfig return previous and current configs * Add config diff to audit record * Fix returned configs * Include high level test * Move FF synchronizer to its own package * Remove unidiomatic use of sync.Once * Add some comments * Rename function * More comment * Save config diff in audit record for local endpoints * Enable audit for config set/reset commands * Improve tests output Co-authored-by: Christopher Speller <crspeller@gmail.com> Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com>
2021-05-21 09:04:39 +02:00
--- 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() {
--- 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)
 		},
--- 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")
--- 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")