From 9d4be3e6653359dbe78d88b144087ec43b29c125 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Fri, 21 May 2021 14:14:45 -0400 Subject: [PATCH 1/9] Require host to specify GDPR default value or hard fail --- config/config.go | 13 ++++++++++++- config/config_test.go | 17 +++++++++++++++++ endpoints/auction_test.go | 1 + main_test.go | 2 ++ 4 files changed, 32 insertions(+), 1 deletion(-) diff --git a/config/config.go b/config/config.go index 1260d28a779..3770a497316 100644 --- a/config/config.go +++ b/config/config.go @@ -467,6 +467,10 @@ func (cfg *TimeoutNotification) validate(errs []error) []error { // New uses viper to get our server configurations. func New(v *viper.Viper) (*Configuration, error) { + if errs := validateRequired(v); len(errs) > 0 { + return nil, errortypes.NewAggregateError("validation errors", errs) + } + var c Configuration if err := v.Unmarshal(&c); err != nil { return nil, fmt.Errorf("viper failed to unmarshal app config: %v", err) @@ -940,7 +944,6 @@ func SetupViper(v *viper.Viper, filename string) { v.SetDefault("amp_timeout_adjustment_ms", 0) v.SetDefault("gdpr.enabled", true) v.SetDefault("gdpr.host_vendor_id", 0) - v.SetDefault("gdpr.default_value", "1") v.SetDefault("gdpr.timeouts_ms.init_vendorlist_fetches", 0) v.SetDefault("gdpr.timeouts_ms.active_vendorlist_fetch", 0) v.SetDefault("gdpr.non_standard_publishers", []string{""}) @@ -1057,3 +1060,11 @@ func isValidCookieSize(maxCookieSize int) error { } return nil } + +func validateRequired(v *viper.Viper) []error { + errs := make([]error, 0) + if !v.IsSet("gdpr.default_value") { + errs = append(errs, fmt.Errorf("Required config flag gdpr.default_value not specified")) + } + return errs +} diff --git a/config/config_test.go b/config/config_test.go index a2b28d026d7..11489bf65c0 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -118,6 +118,7 @@ func TestExternalCacheURLValidate(t *testing.T) { func TestDefaults(t *testing.T) { v := viper.New() SetupViper(v, "") + v.Set("gdpr.default_value", false) cfg, err := New(v) assert.NoError(t, err, "Setting up config should work but it doesn't") @@ -429,6 +430,7 @@ func TestFullConfig(t *testing.T) { func TestUnmarshalAdapterExtraInfo(t *testing.T) { v := viper.New() SetupViper(v, "") + v.Set("gdpr.default_value", false) v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer(adapterExtraInfoConfig)) cfg, err := New(v) @@ -492,6 +494,7 @@ func TestValidConfig(t *testing.T) { func TestMigrateConfig(t *testing.T) { v := viper.New() SetupViper(v, "") + v.Set("gdpr.default_value", false) v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer(oldStoredRequestsConfig)) migrateConfig(v) @@ -510,6 +513,7 @@ func TestMigrateConfigFromEnv(t *testing.T) { os.Setenv("PBS_STORED_REQUESTS_FILESYSTEM", "true") v := viper.New() SetupViper(v, "") + v.Set("gdpr.default_value", false) cfg, err := New(v) assert.NoError(t, err, "Setting up config should work but it doesn't") cmpBools(t, "stored_requests.filesystem.enabled", true, cfg.StoredRequests.Files.Enabled) @@ -591,6 +595,7 @@ func TestMigrateConfigPurposeOneTreatment(t *testing.T) { func TestInvalidAdapterEndpointConfig(t *testing.T) { v := viper.New() SetupViper(v, "") + v.Set("gdpr.default_value", false) v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer(invalidAdapterEndpointConfig)) _, err := New(v) @@ -600,6 +605,7 @@ func TestInvalidAdapterEndpointConfig(t *testing.T) { func TestInvalidAdapterUserSyncURLConfig(t *testing.T) { v := viper.New() SetupViper(v, "") + v.Set("gdpr.default_value", false) v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer(invalidUserSyncURLConfig)) _, err := New(v) @@ -659,6 +665,15 @@ func TestInvalidGDPRDefaultValue(t *testing.T) { assertOneError(t, cfg.validate(), "gdpr.default_value must be 0 or 1") } +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") +} + func TestNegativeCurrencyConverterFetchInterval(t *testing.T) { cfg := Configuration{ CurrencyConverter: CurrencyConverter{ @@ -733,6 +748,7 @@ func TestNewCallsRequestValidation(t *testing.T) { for _, test := range testCases { v := viper.New() SetupViper(v, "") + v.Set("gdpr.default_value", false) v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer([]byte( `request_validation: @@ -771,6 +787,7 @@ func TestValidateAccountsConfigRestrictions(t *testing.T) { func newDefaultConfig(t *testing.T) *Configuration { v := viper.New() SetupViper(v, "") + v.Set("gdpr.default_value", false) v.SetConfigType("yaml") cfg, err := New(v) assert.NoError(t, err) diff --git a/endpoints/auction_test.go b/endpoints/auction_test.go index 2062589e895..c2f85137d27 100644 --- a/endpoints/auction_test.go +++ b/endpoints/auction_test.go @@ -346,6 +346,7 @@ func TestCacheVideoOnly(t *testing.T) { ctx := context.TODO() v := viper.New() config.SetupViper(v, "") + v.Set("gdpr.default_value", false) cfg, err := config.New(v) if err != nil { t.Fatal(err.Error()) diff --git a/main_test.go b/main_test.go index 70eea2825f0..4badc52ae41 100644 --- a/main_test.go +++ b/main_test.go @@ -41,6 +41,7 @@ func forceEnv(t *testing.T, key string, val string) func() { func TestViperInit(t *testing.T) { v := viper.New() config.SetupViper(v, "") + v.Set("gdpr.default_value", false) compareStrings(t, "Viper error: external_url expected to be %s, found %s", "http://localhost:8000", v.Get("external_url").(string)) compareStrings(t, "Viper error: adapters.pulsepoint.endpoint expected to be %s, found %s", "http://bid.contextweb.com/header/s/ortb/prebid-s2s", v.Get("adapters.pulsepoint.endpoint").(string)) } @@ -48,6 +49,7 @@ func TestViperInit(t *testing.T) { func TestViperEnv(t *testing.T) { v := viper.New() config.SetupViper(v, "") + v.Set("gdpr.default_value", false) port := forceEnv(t, "PBS_PORT", "7777") defer port() From fbd7a76f74ca085f7181981619a5b266711cfd24 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Fri, 21 May 2021 14:18:04 -0400 Subject: [PATCH 2/9] Simplify logic --- config/config.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/config/config.go b/config/config.go index 3770a497316..85c4e92989f 100644 --- a/config/config.go +++ b/config/config.go @@ -467,8 +467,8 @@ func (cfg *TimeoutNotification) validate(errs []error) []error { // New uses viper to get our server configurations. func New(v *viper.Viper) (*Configuration, error) { - if errs := validateRequired(v); len(errs) > 0 { - return nil, errortypes.NewAggregateError("validation errors", errs) + if !v.IsSet("gdpr.default_value") { + return nil, fmt.Errorf("Required config flag gdpr.default_value not specified") } var c Configuration @@ -1060,11 +1060,3 @@ func isValidCookieSize(maxCookieSize int) error { } return nil } - -func validateRequired(v *viper.Viper) []error { - errs := make([]error, 0) - if !v.IsSet("gdpr.default_value") { - errs = append(errs, fmt.Errorf("Required config flag gdpr.default_value not specified")) - } - return errs -} From b7e5231d401ffc96d9d38b11da1302b9ed5f7800 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 3/9] Move the GDPR default value validation into the GDPR validate method --- config/config.go | 16 ++++++------- config/config_test.go | 53 +++++++++++++++++++++++++++++++------------ 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/config/config.go b/config/config.go index 85c4e92989f..8e79af3fcb6 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) @@ -207,8 +207,10 @@ type GDPR struct { EEACountriesMap map[string]struct{} } -func (cfg *GDPR) validate(errs []error) []error { - if cfg.DefaultValue != "0" && cfg.DefaultValue != "1" { +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")) + } else if cfg.DefaultValue != "0" && cfg.DefaultValue != "1" { errs = append(errs, fmt.Errorf("gdpr.default_value must be 0 or 1")) } if cfg.HostVendorID < 0 || cfg.HostVendorID > 0xffff { @@ -467,10 +469,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) @@ -524,7 +522,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 11489bf65c0..db57af89a03 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -486,8 +486,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) } @@ -613,16 +616,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) { @@ -644,9 +653,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) @@ -654,9 +666,12 @@ func TestInvalidHostVendorID(t *testing.T) { } 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 TestInvalidGDPRDefaultValue(t *testing.T) { @@ -667,30 +682,34 @@ func TestInvalidGDPRDefaultValue(t *testing.T) { 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) } @@ -766,20 +785,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")) } From a1724c7b4a27d887b3bc9dbd79ad50351c73f8bf Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Tue, 8 Jun 2021 12:09:04 -0400 Subject: [PATCH 4/9] Fix broken tests --- config/config_test.go | 66 ++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 42 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index db57af89a03..5dc056d6e10 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -118,7 +118,7 @@ func TestExternalCacheURLValidate(t *testing.T) { func TestDefaults(t *testing.T) { v := viper.New() SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") cfg, err := New(v) assert.NoError(t, err, "Setting up config should work but it doesn't") @@ -149,7 +149,7 @@ func TestDefaults(t *testing.T) { var fullConfig = []byte(` gdpr: host_vendor_id: 15 - default_value: "0" + default_value: "1" non_standard_publishers: ["siteID","fake-site-id","appID","agltb3B1Yi1pbmNyDAsSA0FwcBiJkfIUDA"] ccpa: enforce: true @@ -353,7 +353,7 @@ func TestFullConfig(t *testing.T) { cmpInts(t, "http_client_cache.max_idle_connections_per_host", cfg.CacheClient.MaxIdleConnsPerHost, 2) cmpInts(t, "http_client_cache.idle_connection_timeout_seconds", cfg.CacheClient.IdleConnTimeout, 3) cmpInts(t, "gdpr.host_vendor_id", cfg.GDPR.HostVendorID, 15) - cmpStrings(t, "gdpr.default_value", cfg.GDPR.DefaultValue, "0") + cmpStrings(t, "gdpr.default_value", cfg.GDPR.DefaultValue, "1") //Assert the NonStandardPublishers was correctly unmarshalled cmpStrings(t, "gdpr.non_standard_publishers", cfg.GDPR.NonStandardPublishers[0], "siteID") @@ -430,7 +430,7 @@ func TestFullConfig(t *testing.T) { func TestUnmarshalAdapterExtraInfo(t *testing.T) { v := viper.New() SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer(adapterExtraInfoConfig)) cfg, err := New(v) @@ -487,7 +487,7 @@ func TestValidConfig(t *testing.T) { } v := viper.New() - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") resolvedStoredRequestsConfig(&cfg) err := cfg.validate(v) @@ -497,7 +497,7 @@ func TestValidConfig(t *testing.T) { func TestMigrateConfig(t *testing.T) { v := viper.New() SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer(oldStoredRequestsConfig)) migrateConfig(v) @@ -516,7 +516,7 @@ func TestMigrateConfigFromEnv(t *testing.T) { os.Setenv("PBS_STORED_REQUESTS_FILESYSTEM", "true") v := viper.New() SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") cfg, err := New(v) assert.NoError(t, err, "Setting up config should work but it doesn't") cmpBools(t, "stored_requests.filesystem.enabled", true, cfg.StoredRequests.Files.Enabled) @@ -598,7 +598,7 @@ func TestMigrateConfigPurposeOneTreatment(t *testing.T) { func TestInvalidAdapterEndpointConfig(t *testing.T) { v := viper.New() SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer(invalidAdapterEndpointConfig)) _, err := New(v) @@ -608,7 +608,7 @@ func TestInvalidAdapterEndpointConfig(t *testing.T) { func TestInvalidAdapterUserSyncURLConfig(t *testing.T) { v := viper.New() SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer(invalidUserSyncURLConfig)) _, err := New(v) @@ -616,19 +616,13 @@ func TestInvalidAdapterUserSyncURLConfig(t *testing.T) { } func TestNegativeRequestSize(t *testing.T) { - v := viper.New() - v.Set("gdpr.default_value", false) - - cfg := newDefaultConfig(t) + cfg, v := newDefaultConfig(t) cfg.MaxRequestSize = -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, v := newDefaultConfig(t) cfg.Metrics.Prometheus.Port = 8001 cfg.Metrics.Prometheus.TimeoutMillisRaw = 0 assertOneError(t, cfg.validate(v), "metrics.prometheus.timeout_ms must be positive if metrics.prometheus.port is defined. Got timeout=0 and port=8001") @@ -653,10 +647,7 @@ func TestInvalidHostVendorID(t *testing.T) { } for _, tt := range tests { - v := viper.New() - v.Set("gdpr.default_value", false) - - cfg := newDefaultConfig(t) + cfg, v := newDefaultConfig(t) cfg.GDPR.HostVendorID = tt.vendorID errs := cfg.validate(v) @@ -666,30 +657,27 @@ func TestInvalidHostVendorID(t *testing.T) { } func TestInvalidAMPException(t *testing.T) { - v := viper.New() - v.Set("gdpr.default_value", false) - - cfg := newDefaultConfig(t) + cfg, v := newDefaultConfig(t) cfg.GDPR.AMPException = true 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 TestInvalidGDPRDefaultValue(t *testing.T) { - cfg := newDefaultConfig(t) + cfg, v := newDefaultConfig(t) cfg.GDPR.DefaultValue = "2" - assertOneError(t, cfg.validate(), "gdpr.default_value must be 0 or 1") + assertOneError(t, cfg.validate(v), "gdpr.default_value must be 0 or 1") } func TestMissingGDPRDefaultValue(t *testing.T) { v := viper.New() - cfg := newDefaultConfig(t) + 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) + v.Set("gdpr.default_value", "0") cfg := Configuration{ CurrencyConverter: CurrencyConverter{ @@ -702,7 +690,7 @@ func TestNegativeCurrencyConverterFetchInterval(t *testing.T) { func TestOverflowedCurrencyConverterFetchInterval(t *testing.T) { v := viper.New() - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") cfg := Configuration{ CurrencyConverter: CurrencyConverter{ @@ -767,7 +755,7 @@ func TestNewCallsRequestValidation(t *testing.T) { for _, test := range testCases { v := viper.New() SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") v.SetConfigType("yaml") v.ReadConfig(bytes.NewBuffer([]byte( `request_validation: @@ -785,10 +773,7 @@ func TestNewCallsRequestValidation(t *testing.T) { } func TestValidateDebug(t *testing.T) { - v := viper.New() - v.Set("gdpr.default_value", false) - - cfg := newDefaultConfig(t) + cfg, v := newDefaultConfig(t) cfg.Debug.TimeoutNotification.SamplingRate = 1.1 err := cfg.validate(v) @@ -796,10 +781,7 @@ func TestValidateDebug(t *testing.T) { } func TestValidateAccountsConfigRestrictions(t *testing.T) { - v := viper.New() - v.Set("gdpr.default_value", false) - - cfg := newDefaultConfig(t) + cfg, v := newDefaultConfig(t) cfg.Accounts.Files.Enabled = true cfg.Accounts.HTTP.Endpoint = "http://localhost" cfg.Accounts.Postgres.ConnectionInfo.Database = "accounts" @@ -809,14 +791,14 @@ func TestValidateAccountsConfigRestrictions(t *testing.T) { assert.Contains(t, errs, errors.New("accounts.postgres: retrieving accounts via postgres not available, use accounts.files")) } -func newDefaultConfig(t *testing.T) *Configuration { +func newDefaultConfig(t *testing.T) (*Configuration, *viper.Viper) { v := viper.New() SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") v.SetConfigType("yaml") cfg, err := New(v) assert.NoError(t, err) - return cfg + return cfg, v } func assertOneError(t *testing.T, errs []error, message string) { From 65b5cea98286be7e3c5efbaabb13906eccb7cc91 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Wed, 9 Jun 2021 13:46:25 -0400 Subject: [PATCH 5/9] Convert GDPR default value string to signal type in exchange --- exchange/exchange.go | 15 ++++++++++----- exchange/exchange_test.go | 7 ++++++- exchange/targeting_test.go | 2 +- exchange/utils.go | 4 ++-- exchange/utils_test.go | 21 +++++++++++++-------- 5 files changed, 32 insertions(+), 17 deletions(-) diff --git a/exchange/exchange.go b/exchange/exchange.go index 7b156b5dee2..e96aadbce86 100644 --- a/exchange/exchange.go +++ b/exchange/exchange.go @@ -59,7 +59,7 @@ type exchange struct { gDPR gdpr.Permissions currencyConverter *currency.RateConverter externalURL string - gdprDefaultValue string + gdprDefaultValue gdpr.Signal privacyConfig config.Privacy categoriesFetcher stored_requests.CategoryFetcher bidIDGenerator BidIDGenerator @@ -110,6 +110,11 @@ func (randomDeduplicateBidBooleanGenerator) Generate() bool { } func NewExchange(adapters map[openrtb_ext.BidderName]adaptedBidder, cache prebid_cache_client.Client, cfg *config.Configuration, metricsEngine metrics.MetricsEngine, infos config.BidderInfos, gDPR gdpr.Permissions, currencyConverter *currency.RateConverter, categoriesFetcher stored_requests.CategoryFetcher) Exchange { + gdprDefaultValue := gdpr.SignalYes + if cfg.GDPR.DefaultValue == "0" { + gdprDefaultValue = gdpr.SignalNo + } + return &exchange{ adapterMap: adapters, bidderInfo: infos, @@ -120,7 +125,7 @@ func NewExchange(adapters map[openrtb_ext.BidderName]adaptedBidder, cache prebid externalURL: cfg.ExternalURL, gDPR: gDPR, me: metricsEngine, - gdprDefaultValue: cfg.GDPR.DefaultValue, + gdprDefaultValue: gdprDefaultValue, privacyConfig: config.Privacy{ CCPA: cfg.CCPA, GDPR: cfg.GDPR, @@ -301,7 +306,7 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog * return e.buildBidResponse(ctx, liveAdapters, adapterBids, r.BidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, errs) } -func (e *exchange) parseGDPRDefaultValue(bidRequest *openrtb2.BidRequest) string { +func (e *exchange) parseGDPRDefaultValue(bidRequest *openrtb2.BidRequest) gdpr.Signal { gdprDefaultValue := e.gdprDefaultValue var geo *openrtb2.Geo = nil @@ -314,10 +319,10 @@ func (e *exchange) parseGDPRDefaultValue(bidRequest *openrtb2.BidRequest) string // If we have a country set, and it is on the list, we assume GDPR applies if not set on the request. // Otherwise we assume it does not apply as long as it appears "valid" (is 3 characters long). if _, found := e.privacyConfig.GDPR.EEACountriesMap[strings.ToUpper(geo.Country)]; found { - gdprDefaultValue = "1" + gdprDefaultValue = gdpr.SignalYes } else if len(geo.Country) == 3 { // The country field is formatted properly as a three character country code - gdprDefaultValue = "0" + gdprDefaultValue = gdpr.SignalNo } } diff --git a/exchange/exchange_test.go b/exchange/exchange_test.go index 5a54b5e14d9..2d1306af093 100644 --- a/exchange/exchange_test.go +++ b/exchange/exchange_test.go @@ -2162,6 +2162,11 @@ func newExchangeForTests(t *testing.T, filename string, expectations map[string] t.Fatalf("Failed to create a category Fetcher: %v", error) } + gdprDefaultValue := gdpr.SignalYes + if privacyConfig.GDPR.DefaultValue == "0" { + gdprDefaultValue = gdpr.SignalNo + } + return &exchange{ adapterMap: bidderAdapters, me: metricsConf.NewMetricsEngine(&config.Configuration{}, openrtb_ext.CoreBidderNames()), @@ -2169,7 +2174,7 @@ func newExchangeForTests(t *testing.T, filename string, expectations map[string] cacheTime: 0, gDPR: &permissionsMock{allowAllBidders: true}, currencyConverter: currency.NewRateConverter(&http.Client{}, "", time.Duration(0)), - gdprDefaultValue: privacyConfig.GDPR.DefaultValue, + gdprDefaultValue: gdprDefaultValue, privacyConfig: privacyConfig, categoriesFetcher: categoriesFetcher, bidderInfo: bidderInfos, diff --git a/exchange/targeting_test.go b/exchange/targeting_test.go index 86646957091..f38a6c0266c 100644 --- a/exchange/targeting_test.go +++ b/exchange/targeting_test.go @@ -93,7 +93,7 @@ func runTargetingAuction(t *testing.T, mockBids map[openrtb_ext.BidderName][]*op cacheTime: time.Duration(0), gDPR: gdpr.AlwaysAllow{}, currencyConverter: currency.NewRateConverter(&http.Client{}, "", time.Duration(0)), - gdprDefaultValue: "1", + gdprDefaultValue: gdpr.SignalYes, categoriesFetcher: categoriesFetcher, bidIDGenerator: &mockBidIDGenerator{false, false}, } diff --git a/exchange/utils.go b/exchange/utils.go index c5cf673250d..3def0425819 100644 --- a/exchange/utils.go +++ b/exchange/utils.go @@ -57,7 +57,7 @@ func cleanOpenRTBRequests(ctx context.Context, requestExt *openrtb_ext.ExtRequest, gDPR gdpr.Permissions, metricsEngine metrics.MetricsEngine, - gdprDefaultValue string, + gdprDefaultValue gdpr.Signal, privacyConfig config.Privacy, account *config.Account) (allowedBidderRequests []BidderRequest, privacyLabels metrics.PrivacyLabels, errs []error) { @@ -87,7 +87,7 @@ func cleanOpenRTBRequests(ctx context.Context, if err != nil { errs = append(errs, err) } - gdprEnforced := gdprSignal == gdpr.SignalYes || (gdprSignal == gdpr.SignalAmbiguous && gdprDefaultValue == "1") + gdprEnforced := gdprSignal == gdpr.SignalYes || (gdprSignal == gdpr.SignalAmbiguous && gdprDefaultValue == gdpr.SignalYes) ccpaEnforcer, err := extractCCPA(req.BidRequest, privacyConfig, &req.Account, aliases, integrationTypeMap[req.LegacyLabels.RType]) if err != nil { diff --git a/exchange/utils_test.go b/exchange/utils_test.go index dad0d69db15..5a281f9a360 100644 --- a/exchange/utils_test.go +++ b/exchange/utils_test.go @@ -479,7 +479,7 @@ func TestCleanOpenRTBRequests(t *testing.T) { for _, test := range testCases { metricsMock := metrics.MetricsEngineMock{} permissions := permissionsMock{allowAllBidders: true, passGeo: true, passID: true} - bidderRequests, _, err := cleanOpenRTBRequests(context.Background(), test.req, nil, &permissions, &metricsMock, "0", privacyConfig, nil) + bidderRequests, _, err := cleanOpenRTBRequests(context.Background(), test.req, nil, &permissions, &metricsMock, gdpr.SignalNo, privacyConfig, nil) if test.hasError { assert.NotNil(t, err, "Error shouldn't be nil") } else { @@ -636,7 +636,7 @@ func TestCleanOpenRTBRequestsCCPA(t *testing.T) { nil, &permissionsMock{allowAllBidders: true, passGeo: true, passID: true}, &metrics.MetricsEngineMock{}, - "0", + gdpr.SignalNo, privacyConfig, nil) result := bidderRequests[0] @@ -698,7 +698,7 @@ func TestCleanOpenRTBRequestsCCPAErrors(t *testing.T) { } permissions := permissionsMock{allowAllBidders: true, passGeo: true, passID: true} metrics := metrics.MetricsEngineMock{} - _, _, errs := cleanOpenRTBRequests(context.Background(), auctionReq, &reqExtStruct, &permissions, &metrics, "0", privacyConfig, nil) + _, _, errs := cleanOpenRTBRequests(context.Background(), auctionReq, &reqExtStruct, &permissions, &metrics, gdpr.SignalNo, privacyConfig, nil) assert.ElementsMatch(t, []error{test.expectError}, errs, test.description) } @@ -740,7 +740,7 @@ func TestCleanOpenRTBRequestsCOPPA(t *testing.T) { permissions := permissionsMock{allowAllBidders: true, passGeo: true, passID: true} metrics := metrics.MetricsEngineMock{} - bidderRequests, privacyLabels, errs := cleanOpenRTBRequests(context.Background(), auctionReq, nil, &permissions, &metrics, "0", config.Privacy{}, nil) + bidderRequests, privacyLabels, errs := cleanOpenRTBRequests(context.Background(), auctionReq, nil, &permissions, &metrics, gdpr.SignalNo, config.Privacy{}, nil) result := bidderRequests[0] assert.Nil(t, errs) @@ -849,7 +849,7 @@ func TestCleanOpenRTBRequestsSChain(t *testing.T) { permissions := permissionsMock{allowAllBidders: true, passGeo: true, passID: true} metrics := metrics.MetricsEngineMock{} - bidderRequests, _, errs := cleanOpenRTBRequests(context.Background(), auctionReq, extRequest, &permissions, &metrics, "0", config.Privacy{}, nil) + bidderRequests, _, errs := cleanOpenRTBRequests(context.Background(), auctionReq, extRequest, &permissions, &metrics, gdpr.SignalNo, config.Privacy{}, nil) if test.hasError == true { assert.NotNil(t, errs) assert.Len(t, bidderRequests, 0) @@ -1432,7 +1432,7 @@ func TestCleanOpenRTBRequestsLMT(t *testing.T) { permissions := permissionsMock{allowAllBidders: true, passGeo: true, passID: true} metrics := metrics.MetricsEngineMock{} - results, privacyLabels, errs := cleanOpenRTBRequests(context.Background(), auctionReq, nil, &permissions, &metrics, "0", privacyConfig, nil) + results, privacyLabels, errs := cleanOpenRTBRequests(context.Background(), auctionReq, nil, &permissions, &metrics, gdpr.SignalNo, privacyConfig, nil) result := results[0] assert.Nil(t, errs) @@ -1639,13 +1639,18 @@ func TestCleanOpenRTBRequestsGDPR(t *testing.T) { Account: accountConfig, } + gdprDefaultValue := gdpr.SignalYes + if test.gdprDefaultValue == "0" { + gdprDefaultValue = gdpr.SignalNo + } + results, privacyLabels, errs := cleanOpenRTBRequests( context.Background(), auctionReq, nil, &permissionsMock{allowAllBidders: true, passGeo: !test.gdprScrub, passID: !test.gdprScrub, activitiesError: test.permissionsError}, &metrics.MetricsEngineMock{}, - test.gdprDefaultValue, + gdprDefaultValue, privacyConfig, nil) result := results[0] @@ -1736,7 +1741,7 @@ func TestCleanOpenRTBRequestsGDPRBlockBidRequest(t *testing.T) { nil, &permissionsMock{allowedBidders: test.gdprAllowedBidders, passGeo: true, passID: true, activitiesError: nil}, &metricsMock, - "0", + gdpr.SignalNo, privacyConfig, nil) From 0d356a1cabd8e4ac2b0e70496b1f058c530cf266 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Wed, 9 Jun 2021 13:46:53 -0400 Subject: [PATCH 6/9] Convert GDPR default value string to signal type in GDPR permissions --- gdpr/gdpr.go | 10 ++++++++-- gdpr/impl.go | 9 +++++---- gdpr/impl_test.go | 14 +++++++++++++- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/gdpr/gdpr.go b/gdpr/gdpr.go index 4179d8122ac..47d20a50899 100644 --- a/gdpr/gdpr.go +++ b/gdpr/gdpr.go @@ -39,9 +39,15 @@ func NewPermissions(ctx context.Context, cfg config.GDPR, vendorIDs map[openrtb_ return &AlwaysAllow{} } + gdprDefaultValue := SignalYes + if cfg.DefaultValue == "0" { + gdprDefaultValue = SignalNo + } + permissionsImpl := &permissionsImpl{ - cfg: cfg, - vendorIDs: vendorIDs, + cfg: cfg, + gdprDefaultValue: gdprDefaultValue, + vendorIDs: vendorIDs, fetchVendorList: map[uint8]func(ctx context.Context, id uint16) (vendorlist.VendorList, error){ tcf2SpecVersion: newVendorListFetcher(ctx, cfg, client, vendorListURLMaker)}, } diff --git a/gdpr/impl.go b/gdpr/impl.go index 9c09e90b58e..af7150a8755 100644 --- a/gdpr/impl.go +++ b/gdpr/impl.go @@ -28,9 +28,10 @@ const ( ) type permissionsImpl struct { - cfg config.GDPR - vendorIDs map[openrtb_ext.BidderName]uint16 - fetchVendorList map[uint8]func(ctx context.Context, id uint16) (vendorlist.VendorList, error) + cfg config.GDPR + gdprDefaultValue Signal + vendorIDs map[openrtb_ext.BidderName]uint16 + fetchVendorList map[uint8]func(ctx context.Context, id uint16) (vendorlist.VendorList, error) } func (p *permissionsImpl) HostCookiesAllowed(ctx context.Context, gdprSignal Signal, consent string) (bool, error) { @@ -94,7 +95,7 @@ func (p *permissionsImpl) normalizeGDPR(gdprSignal Signal) Signal { return gdprSignal } - if p.cfg.DefaultValue == "0" { + if p.gdprDefaultValue == SignalNo { return SignalNo } diff --git a/gdpr/impl_test.go b/gdpr/impl_test.go index 345dd52621d..f4b0392876b 100644 --- a/gdpr/impl_test.go +++ b/gdpr/impl_test.go @@ -21,7 +21,8 @@ func TestDisallowOnEmptyConsent(t *testing.T) { HostVendorID: 3, DefaultValue: "0", }, - vendorIDs: nil, + gdprDefaultValue: SignalNo, + vendorIDs: nil, fetchVendorList: map[uint8]func(ctx context.Context, id uint16) (vendorlist.VendorList, error){ tcf2SpecVersion: failedListFetcher, }, @@ -303,6 +304,11 @@ func TestAllowActivities(t *testing.T) { for _, tt := range tests { perms.cfg.DefaultValue = tt.gdprDefaultValue + if tt.gdprDefaultValue == "0" { + perms.gdprDefaultValue = SignalNo + } else { + perms.gdprDefaultValue = SignalYes + } _, _, passID, err := perms.AuctionActivitiesAllowed(context.Background(), tt.bidderName, tt.publisherID, tt.gdpr, tt.consent, tt.weakVendorEnforcement) @@ -719,6 +725,12 @@ func TestNormalizeGDPR(t *testing.T) { }, } + if tt.gdprDefaultValue == "0" { + perms.gdprDefaultValue = SignalNo + } else { + perms.gdprDefaultValue = SignalYes + } + normalizedSignal := perms.normalizeGDPR(tt.giveSignal) assert.Equal(t, tt.wantSignal, normalizedSignal, tt.description) From f2c0cf7d82f361d85fe5563ad9a7c1e39fa2f88b Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Tue, 15 Jun 2021 15:48:11 -0400 Subject: [PATCH 7/9] Allow gdpr.default_value to be set as an env var --- config/config.go | 1 + main_test.go | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index 8e79af3fcb6..5621c2ec416 100644 --- a/config/config.go +++ b/config/config.go @@ -940,6 +940,7 @@ func SetupViper(v *viper.Viper, filename string) { v.SetDefault("analytics.pubstack.buffers.count", 100) v.SetDefault("analytics.pubstack.buffers.timeout", "900s") v.SetDefault("amp_timeout_adjustment_ms", 0) + v.BindEnv("gdpr.default_value") v.SetDefault("gdpr.enabled", true) v.SetDefault("gdpr.host_vendor_id", 0) v.SetDefault("gdpr.timeouts_ms.init_vendorlist_fetches", 0) diff --git a/main_test.go b/main_test.go index 4badc52ae41..70eea2825f0 100644 --- a/main_test.go +++ b/main_test.go @@ -41,7 +41,6 @@ func forceEnv(t *testing.T, key string, val string) func() { func TestViperInit(t *testing.T) { v := viper.New() config.SetupViper(v, "") - v.Set("gdpr.default_value", false) compareStrings(t, "Viper error: external_url expected to be %s, found %s", "http://localhost:8000", v.Get("external_url").(string)) compareStrings(t, "Viper error: adapters.pulsepoint.endpoint expected to be %s, found %s", "http://bid.contextweb.com/header/s/ortb/prebid-s2s", v.Get("adapters.pulsepoint.endpoint").(string)) } @@ -49,7 +48,6 @@ func TestViperInit(t *testing.T) { func TestViperEnv(t *testing.T) { v := viper.New() config.SetupViper(v, "") - v.Set("gdpr.default_value", false) port := forceEnv(t, "PBS_PORT", "7777") defer port() From 3c50962a29c7c7409f1383b5bb155f900b86aaf2 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Tue, 15 Jun 2021 16:02:23 -0400 Subject: [PATCH 8/9] Simplify tests --- config/config_test.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 5dc056d6e10..fa9dcdc5195 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -116,11 +116,7 @@ func TestExternalCacheURLValidate(t *testing.T) { } func TestDefaults(t *testing.T) { - v := viper.New() - SetupViper(v, "") - v.Set("gdpr.default_value", "0") - cfg, err := New(v) - assert.NoError(t, err, "Setting up config should work but it doesn't") + cfg, _ := newDefaultConfig(t) cmpInts(t, "port", cfg.Port, 8000) cmpInts(t, "admin_port", cfg.AdminPort, 6060) @@ -514,11 +510,7 @@ func TestMigrateConfigFromEnv(t *testing.T) { defer os.Unsetenv("PBS_STORED_REQUESTS_FILESYSTEM") } os.Setenv("PBS_STORED_REQUESTS_FILESYSTEM", "true") - v := viper.New() - SetupViper(v, "") - v.Set("gdpr.default_value", "0") - cfg, err := New(v) - assert.NoError(t, err, "Setting up config should work but it doesn't") + cfg, _ := newDefaultConfig(t) cmpBools(t, "stored_requests.filesystem.enabled", true, cfg.StoredRequests.Files.Enabled) } @@ -797,7 +789,7 @@ func newDefaultConfig(t *testing.T) (*Configuration, *viper.Viper) { v.Set("gdpr.default_value", "0") v.SetConfigType("yaml") cfg, err := New(v) - assert.NoError(t, err) + assert.NoError(t, err, "Setting up config should work but it doesn't") return cfg, v } From d2f2ccbc9c4707e8b156a8bbf0598a74c6e711f0 Mon Sep 17 00:00:00 2001 From: bsardo <1168933+bsardo@users.noreply.github.com> Date: Wed, 16 Jun 2021 09:05:25 -0400 Subject: [PATCH 9/9] change default_value in test from bool to string --- endpoints/auction_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/endpoints/auction_test.go b/endpoints/auction_test.go index c2f85137d27..bdf68db5be7 100644 --- a/endpoints/auction_test.go +++ b/endpoints/auction_test.go @@ -346,7 +346,7 @@ func TestCacheVideoOnly(t *testing.T) { ctx := context.TODO() v := viper.New() config.SetupViper(v, "") - v.Set("gdpr.default_value", false) + v.Set("gdpr.default_value", "0") cfg, err := config.New(v) if err != nil { t.Fatal(err.Error())