diff --git a/api4/config.go b/api4/config.go index 947f0dd189..cd009c46a1 100644 --- a/api4/config.go +++ b/api4/config.go @@ -154,13 +154,10 @@ 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 { - 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) + // 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) return } } @@ -291,13 +288,8 @@ 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 { - 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) + 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) return } } diff --git a/config/diff.go b/config/diff.go index 4716ea1899..1390918445 100644 --- a/config/diff.go +++ b/config/diff.go @@ -6,7 +6,6 @@ package config import ( "fmt" "reflect" - "strings" "github.com/mattermost/mattermost-server/v6/model" ) @@ -72,7 +71,7 @@ func (cd ConfigDiffs) Sanitize() ConfigDiffs { return cd } -func diff(base, actual reflect.Value, structField reflect.StructField, label string, tag, tagValue string) ([]ConfigDiff, error) { +func diff(base, actual reflect.Value, label string) ([]ConfigDiff, error) { var diffs []ConfigDiff if base.IsZero() && actual.IsZero() { @@ -101,42 +100,17 @@ func diff(base, actual reflect.Value, structField reflect.StructField, label str 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), actualType.Field(i), fieldLabel, tag, tagValue) + d, err := diff(base.Field(i), actual.Field(i), fieldLabel) if err != nil { return nil, err } @@ -155,24 +129,13 @@ func diff(base, actual reflect.Value, structField reflect.StructField, label str 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, 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) + return diff(baseVal, actualVal, "") } func (cd ConfigDiffs) String() string { diff --git a/config/diff_test.go b/config/diff_test.go index 0def47fa7e..6c05da327c 100644 --- a/config/diff_test.go +++ b/config/diff_test.go @@ -958,105 +958,3 @@ 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 c1a36f71e7..037fe9abd2 100644 --- a/model/config.go +++ b/model/config.go @@ -2244,7 +2244,7 @@ func (s *LdapSettings) SetDefaults() { type ComplianceSettings struct { Enable *bool `access:"compliance_compliance_monitoring"` - Directory *string `access:"compliance_compliance_monitoring,cloud_restrictable"` // telemetry: none + Directory *string `access:"compliance_compliance_monitoring"` // telemetry: none EnableDaily *bool `access:"compliance_compliance_monitoring"` BatchSize *int `access:"compliance_compliance_monitoring"` // telemetry: none } @@ -3109,18 +3109,18 @@ type Config struct { ExperimentalSettings ExperimentalSettings AnalyticsSettings AnalyticsSettings ElasticsearchSettings ElasticsearchSettings - BleveSettings BleveSettings `access:"cloud_restrictable"` + BleveSettings BleveSettings DataRetentionSettings DataRetentionSettings MessageExportSettings MessageExportSettings JobSettings JobSettings PluginSettings PluginSettings DisplaySettings DisplaySettings GuestAccountsSettings GuestAccountsSettings - 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"` + ImageProxySettings ImageProxySettings + CloudSettings CloudSettings // telemetry: none + FeatureFlags *FeatureFlags `access:"*_read" json:",omitempty"` + ImportSettings ImportSettings // telemetry: none + ExportSettings ExportSettings } func (o *Config) Clone() *Config {