From 8802c6c9d6ae31b1620ec8a3a2a7f25b2fc39053 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Mon, 16 May 2022 15:12:12 +0300 Subject: [PATCH] config/diff: add utility function to get diffs scoped with struct tags (#20206) --- api4/config.go | 20 ++++++--- config/diff.go | 43 +++++++++++++++++-- config/diff_test.go | 102 ++++++++++++++++++++++++++++++++++++++++++++ model/config.go | 14 +++--- 4 files changed, 163 insertions(+), 16 deletions(-) diff --git a/api4/config.go b/api4/config.go index cd009c46a1..947f0dd189 100644 --- a/api4/config.go +++ b/api4/config.go @@ -154,10 +154,13 @@ func updateConfig(c *Context, w http.ResponseWriter, r *http.Request) { // There are some settings that cannot be changed in a cloud env if c.App.Channels().License() != nil && *c.App.Channels().License().Features.Cloud { - // Both of them cannot be nil since cfg.SetDefaults is called earlier for cfg, - // and appCfg is the existing earlier config and if it's nil, server sets a default value. - if *appCfg.ComplianceSettings.Directory != *cfg.ComplianceSettings.Directory { - c.Err = model.NewAppError("updateConfig", "api.config.update_config.not_allowed_security.app_error", map[string]interface{}{"Name": "ComplianceSettings.Directory"}, "", http.StatusForbidden) + diffs, diffErr := config.DiffTags(appCfg, cfg, "access", "cloud_restrictable") + if diffErr != nil { + c.Err = model.NewAppError("updateConfig", "api.config.update_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) + return + } + if len(diffs) > 0 { + c.Err = model.NewAppError("updateConfig", "api.config.update_config.not_allowed_security.app_error", map[string]interface{}{"Name": diffs[0].Path}, "", http.StatusForbidden) return } } @@ -288,8 +291,13 @@ func patchConfig(c *Context, w http.ResponseWriter, r *http.Request) { // There are some settings that cannot be changed in a cloud env if c.App.Channels().License() != nil && *c.App.Channels().License().Features.Cloud { - if cfg.ComplianceSettings.Directory != nil && *appCfg.ComplianceSettings.Directory != *cfg.ComplianceSettings.Directory { - c.Err = model.NewAppError("patchConfig", "api.config.update_config.not_allowed_security.app_error", map[string]interface{}{"Name": "ComplianceSettings.Directory"}, "", http.StatusForbidden) + diffs, diffErr := config.DiffTags(appCfg, cfg, "access", "cloud_restrictable") + if diffErr != nil { + c.Err = model.NewAppError("patchConfig", "api.config.update_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) + return + } + if len(diffs) > 0 { + c.Err = model.NewAppError("patchConfig", "api.config.update_config.not_allowed_security.app_error", map[string]interface{}{"Name": diffs[0].Path}, "", http.StatusForbidden) return } } diff --git a/config/diff.go b/config/diff.go index 1390918445..4716ea1899 100644 --- a/config/diff.go +++ b/config/diff.go @@ -6,6 +6,7 @@ package config import ( "fmt" "reflect" + "strings" "github.com/mattermost/mattermost-server/v6/model" ) @@ -71,7 +72,7 @@ func (cd ConfigDiffs) Sanitize() ConfigDiffs { return cd } -func diff(base, actual reflect.Value, label string) ([]ConfigDiff, error) { +func diff(base, actual reflect.Value, structField reflect.StructField, label string, tag, tagValue string) ([]ConfigDiff, error) { var diffs []ConfigDiff if base.IsZero() && actual.IsZero() { @@ -100,17 +101,42 @@ func diff(base, actual reflect.Value, label string) ([]ConfigDiff, error) { return nil, fmt.Errorf("not same type %s %s", baseType, actualType) } + // skip if not tag scoped, field does not have any tags or if it's just empty + if tag != "" && string(structField.Tag) != "" && structField.Name != "" { + // we are getting the diffs scoped with a specific tag + // therefore we first lookup if the field has the tag, if not we skip + // to check if it's changed or not as it's out of the scope + val, ok := structField.Tag.Lookup(tag) + if !ok { + return diffs, nil + } + + // tag scope also cares about the tag value, if we don't have the value + // there is no need to get the diff + if !strings.Contains(val, tagValue) { + return diffs, nil + } + + // prevent going further scoping according this tag because we are already + // scoped the struct on higher level + if baseType.Kind() == reflect.Struct { + tag = "" + } + } + 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) + + d, err := diff(base.Field(i), actual.Field(i), actualType.Field(i), fieldLabel, tag, tagValue) if err != nil { return nil, err } @@ -129,13 +155,24 @@ func diff(base, actual reflect.Value, label string) ([]ConfigDiff, error) { return diffs, nil } +// Diff returns the diff between two configs 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, "") + return diff(baseVal, actualVal, reflect.StructField{}, "", "", "") +} + +// DiffTags behaves similar with Diff but it is scoped against a tag and it's value +func DiffTags(base, actual *model.Config, tag, value string) (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, reflect.StructField{}, "", tag, value) } func (cd ConfigDiffs) String() string { diff --git a/config/diff_test.go b/config/diff_test.go index 6c05da327c..0def47fa7e 100644 --- a/config/diff_test.go +++ b/config/diff_test.go @@ -958,3 +958,105 @@ func TestDiff(t *testing.T) { }) } } + +func TestDiffTags(t *testing.T) { + tcs := []struct { + name string + base *model.Config + actual *model.Config + diffs ConfigDiffs + tag string + value string + err string + }{ + { + name: "changed field is not in scope", + base: defaultConfigGen(), + actual: func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.EnableLinkPreviews = model.NewBool(false) + return cfg + }(), + diffs: nil, + tag: "access", + value: "cloud_restrictable", + err: "", + }, + { + name: "changed field is in scope", + base: defaultConfigGen(), + actual: func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.ReadTimeout = model.NewInt(500) + return cfg + }(), + diffs: ConfigDiffs{ + { + Path: "ServiceSettings.ReadTimeout", + BaseVal: 300, + ActualVal: 500, + }, + }, + tag: "access", + value: "cloud_restrictable", + err: "", + }, + { + name: "changed struct is in scope", + base: defaultConfigGen(), + actual: func() *model.Config { + cfg := defaultConfigGen() + cfg.BleveSettings.BatchSize = model.NewInt(500) + return cfg + }(), + diffs: ConfigDiffs{ + { + Path: "BleveSettings.BatchSize", + BaseVal: 10000, + ActualVal: 500, + }, + }, + tag: "access", + value: "cloud_restrictable", + err: "", + }, + { + name: "changed struct is not in scope", + base: defaultConfigGen(), + actual: func() *model.Config { + cfg := defaultConfigGen() + cfg.LocalizationSettings.DefaultServerLocale = model.NewString("Elvish") + return cfg + }(), + diffs: nil, + tag: "access", + value: "cloud_restrictable", + err: "", + }, + { + name: "changed field is not in scope due to its value but has the same tag", + base: defaultConfigGen(), + actual: func() *model.Config { + cfg := defaultConfigGen() + cfg.TeamSettings.SiteName = model.NewString("Mordor") + return cfg + }(), + diffs: nil, + tag: "access", + value: "cloud_restrictable", + err: "", + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + diffs, err := DiffTags(tc.base, tc.actual, tc.tag, tc.value) + 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/model/config.go b/model/config.go index 93db2af25b..f4abddd1a6 100644 --- a/model/config.go +++ b/model/config.go @@ -2250,7 +2250,7 @@ func (s *LdapSettings) SetDefaults() { type ComplianceSettings struct { Enable *bool `access:"compliance_compliance_monitoring"` - Directory *string `access:"compliance_compliance_monitoring"` // telemetry: none + Directory *string `access:"compliance_compliance_monitoring,cloud_restrictable"` // telemetry: none EnableDaily *bool `access:"compliance_compliance_monitoring"` BatchSize *int `access:"compliance_compliance_monitoring"` // telemetry: none } @@ -3115,18 +3115,18 @@ type Config struct { ExperimentalSettings ExperimentalSettings AnalyticsSettings AnalyticsSettings ElasticsearchSettings ElasticsearchSettings - BleveSettings BleveSettings + BleveSettings BleveSettings `access:"cloud_restrictable"` DataRetentionSettings DataRetentionSettings MessageExportSettings MessageExportSettings JobSettings JobSettings PluginSettings PluginSettings DisplaySettings DisplaySettings GuestAccountsSettings GuestAccountsSettings - ImageProxySettings ImageProxySettings - CloudSettings CloudSettings // telemetry: none - FeatureFlags *FeatureFlags `access:"*_read" json:",omitempty"` - ImportSettings ImportSettings // telemetry: none - ExportSettings ExportSettings + ImageProxySettings ImageProxySettings `access:"cloud_restrictable"` + CloudSettings CloudSettings // telemetry: none + FeatureFlags *FeatureFlags `access:"*_read" json:",omitempty"` + ImportSettings ImportSettings `access:"cloud_restrictable"` // telemetry: none + ExportSettings ExportSettings `access:"cloud_restrictable"` } func (o *Config) Clone() *Config {