From 424a44283dafe25f1cb5e26d32395618cf0c3205 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Fri, 21 May 2021 14:58:23 -0400 Subject: [PATCH] Move the GDPR default value validation into the GDPR validate method --- config/config.go | 15 ++++++----- config/config_test.go | 58 ++++++++++++++++++++++++++++++++----------- 2 files changed, 50 insertions(+), 23 deletions(-) diff --git a/config/config.go b/config/config.go index 0a905de6733..757ef6da31b 100644 --- a/config/config.go +++ b/config/config.go @@ -93,7 +93,7 @@ type HTTPClient struct { IdleConnTimeout int `mapstructure:"idle_connection_timeout_seconds"` } -func (cfg *Configuration) validate() []error { +func (cfg *Configuration) validate(v *viper.Viper) []error { var errs []error errs = cfg.AuctionTimeouts.validate(errs) errs = cfg.StoredRequests.validate(errs) @@ -105,7 +105,7 @@ func (cfg *Configuration) validate() []error { if cfg.MaxRequestSize < 0 { errs = append(errs, fmt.Errorf("cfg.max_request_size must be >= 0. Got %d", cfg.MaxRequestSize)) } - errs = cfg.GDPR.validate(errs) + errs = cfg.GDPR.validate(v, errs) errs = cfg.CurrencyConverter.validate(errs) errs = validateAdapters(cfg.Adapters, errs) errs = cfg.Debug.validate(errs) @@ -208,7 +208,10 @@ type GDPR struct { EEACountriesMap map[string]struct{} } -func (cfg *GDPR) validate(errs []error) []error { +func (cfg *GDPR) validate(v *viper.Viper, errs []error) []error { + if !v.IsSet("gdpr.default_value") { + errs = append(errs, fmt.Errorf("gdpr.default_value is required and must be specified")) + } if cfg.HostVendorID < 0 || cfg.HostVendorID > 0xffff { errs = append(errs, fmt.Errorf("gdpr.host_vendor_id must be in the range [0, %d]. Got %d", 0xffff, cfg.HostVendorID)) } @@ -473,10 +476,6 @@ func (cfg *TimeoutNotification) validate(errs []error) []error { // New uses viper to get our server configurations. func New(v *viper.Viper) (*Configuration, error) { - if !v.IsSet("gdpr.default_value") { - return nil, fmt.Errorf("Required config flag gdpr.default_value not specified") - } - var c Configuration if err := v.Unmarshal(&c); err != nil { return nil, fmt.Errorf("viper failed to unmarshal app config: %v", err) @@ -530,7 +529,7 @@ func New(v *viper.Viper) (*Configuration, error) { glog.Info("Logging the resolved configuration:") logGeneral(reflect.ValueOf(c), " \t") - if errs := c.validate(); len(errs) > 0 { + if errs := c.validate(v); len(errs) > 0 { return &c, errortypes.NewAggregateError("validation errors", errs) } diff --git a/config/config_test.go b/config/config_test.go index c5f6fc9b299..c0e54dccf0f 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -480,8 +480,11 @@ func TestValidConfig(t *testing.T) { }, } + v := viper.New() + v.Set("gdpr.default_value", false) + resolvedStoredRequestsConfig(&cfg) - err := cfg.validate() + err := cfg.validate(v) assert.Nil(t, err, "OpenRTB filesystem config should work. %v", err) } @@ -534,16 +537,22 @@ func TestInvalidAdapterUserSyncURLConfig(t *testing.T) { } func TestNegativeRequestSize(t *testing.T) { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := newDefaultConfig(t) cfg.MaxRequestSize = -1 - assertOneError(t, cfg.validate(), "cfg.max_request_size must be >= 0. Got -1") + assertOneError(t, cfg.validate(v), "cfg.max_request_size must be >= 0. Got -1") } func TestNegativePrometheusTimeout(t *testing.T) { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := newDefaultConfig(t) cfg.Metrics.Prometheus.Port = 8001 cfg.Metrics.Prometheus.TimeoutMillisRaw = 0 - assertOneError(t, cfg.validate(), "metrics.prometheus.timeout_ms must be positive if metrics.prometheus.port is defined. Got timeout=0 and port=8001") + assertOneError(t, cfg.validate(v), "metrics.prometheus.timeout_ms must be positive if metrics.prometheus.port is defined. Got timeout=0 and port=8001") } func TestInvalidHostVendorID(t *testing.T) { @@ -565,9 +574,12 @@ func TestInvalidHostVendorID(t *testing.T) { } for _, tt := range tests { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := newDefaultConfig(t) cfg.GDPR.HostVendorID = tt.vendorID - errs := cfg.validate() + errs := cfg.validate(v) assert.Equal(t, 1, len(errs), tt.description) assert.EqualError(t, errs[0], tt.wantErrorMsg, tt.description) @@ -575,43 +587,53 @@ func TestInvalidHostVendorID(t *testing.T) { } func TestInvalidFetchGVL(t *testing.T) { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := newDefaultConfig(t) cfg.GDPR.TCF1.FetchGVL = true - assertOneError(t, cfg.validate(), "gdpr.tcf1.fetch_gvl has been discontinued and must be removed from your config. TCF1 will always use the fallback GVL going forward") + assertOneError(t, cfg.validate(v), "gdpr.tcf1.fetch_gvl has been discontinued and must be removed from your config. TCF1 will always use the fallback GVL going forward") } func TestInvalidAMPException(t *testing.T) { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := newDefaultConfig(t) cfg.GDPR.AMPException = true - assertOneError(t, cfg.validate(), "gdpr.amp_exception has been discontinued and must be removed from your config. If you need to disable GDPR for AMP, you may do so per-account (gdpr.integration_enabled.amp) or at the host level for the default account (account_defaults.gdpr.integration_enabled.amp)") + assertOneError(t, cfg.validate(v), "gdpr.amp_exception has been discontinued and must be removed from your config. If you need to disable GDPR for AMP, you may do so per-account (gdpr.integration_enabled.amp) or at the host level for the default account (account_defaults.gdpr.integration_enabled.amp)") } func TestMissingGDPRDefaultValue(t *testing.T) { v := viper.New() - SetupViper(v, "") - cfg, err := New(v) - assert.Nil(t, cfg, "cfg is nil when gdpr.default_value is not specified") - assert.Error(t, err, "err is set when gdpr.default_value is not specified") - assert.Contains(t, err.Error(), "Required config flag gdpr.default_value not specified", "err msg indicates gdpr.default_value is required") + + cfg := newDefaultConfig(t) + assertOneError(t, cfg.validate(v), "gdpr.default_value is required and must be specified") } func TestNegativeCurrencyConverterFetchInterval(t *testing.T) { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := Configuration{ CurrencyConverter: CurrencyConverter{ FetchIntervalSeconds: -1, }, } - err := cfg.validate() + err := cfg.validate(v) assert.NotNil(t, err, "cfg.currency_converter.fetch_interval_seconds should prevent negative values, but it doesn't") } func TestOverflowedCurrencyConverterFetchInterval(t *testing.T) { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := Configuration{ CurrencyConverter: CurrencyConverter{ FetchIntervalSeconds: (0xffff) + 1, }, } - err := cfg.validate() + err := cfg.validate(v) assert.NotNil(t, err, "cfg.currency_converter.fetch_interval_seconds prevent values over %d, but it doesn't", 0xffff) } @@ -687,20 +709,26 @@ func TestNewCallsRequestValidation(t *testing.T) { } func TestValidateDebug(t *testing.T) { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := newDefaultConfig(t) cfg.Debug.TimeoutNotification.SamplingRate = 1.1 - err := cfg.validate() + err := cfg.validate(v) assert.NotNil(t, err, "cfg.debug.timeout_notification.sampling_rate should not be allowed to be greater than 1.0, but it was allowed") } func TestValidateAccountsConfigRestrictions(t *testing.T) { + v := viper.New() + v.Set("gdpr.default_value", false) + cfg := newDefaultConfig(t) cfg.Accounts.Files.Enabled = true cfg.Accounts.HTTP.Endpoint = "http://localhost" cfg.Accounts.Postgres.ConnectionInfo.Database = "accounts" - errs := cfg.validate() + errs := cfg.validate(v) assert.Len(t, errs, 1) assert.Contains(t, errs, errors.New("accounts.postgres: retrieving accounts via postgres not available, use accounts.files")) }