Add explicit valid_account_required config

This setting enforces a valid host account because account_required does not, and enables host to transition from the more relaxed account_required:true to performing strict account validation.
prebid · Aug 26, 2020 · a4ebd35 · a4ebd35
1 parent 06cc3e7
commit a4ebd35
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 33 deletions.
diff --git a/config/config.go b/config/config.go
@@ -41,9 +41,6 @@ type Configuration struct {
 	StoredRequests  StoredRequests     `mapstructure:"stored_requests"`
 	CategoryMapping StoredRequestsSlim `mapstructure:"category_mapping"`
 	Accounts        StoredRequestsSlim `mapstructure:"accounts"`
-	DefaultAccount  Account
-	// DefaultAccountJSON is the serialized form of DefaultAccount used for json merge
-	DefaultAccountJSON json.RawMessage
 	// Note that StoredVideo refers to stored video requests, and has nothing to do with caching video creatives.
 	StoredVideo StoredRequestsSlim `mapstructure:"stored_video_req"`
 
@@ -69,6 +66,13 @@ type Configuration struct {
 	BlacklistedAcctMap map[string]bool
 	// Is publisher/account ID required to be submitted in the OpenRTB2 request
 	AccountRequired bool `mapstructure:"account_required"`
+	// Is publisher/account ID required to be a valid account with the prebid-server host
+	ValidAccountRequired bool `mapstructure:"valid_account_required"`
+	// AccountDefaults defines default settings for valid accounts that are partially defined
+	// and provides a way to set global settings that can be overridden at account level.
+	AccountDefaults Account `mapstructure:"account_defaults"`
+	// AccountDefaultsJSON is the serialized form of AccountDefaults used for json merge
+	AccountDefaultsJSON json.RawMessage
 	// Local private file containing SSL certificates
 	PemCertsFile string `mapstructure:"certificates_file"`
 	// Custom headers to handle request timeouts from queueing infrastructure
@@ -118,6 +122,15 @@ func (cfg *Configuration) validate() configErrors {
 	errs = validateAdapters(cfg.Adapters, errs)
 	errs = cfg.Debug.validate(errs)
 	errs = cfg.ExtCacheURL.validate(errs)
+	if cfg.ValidAccountRequired && !cfg.AccountRequired {
+		glog.Warning("account_required=false is ignored because cfg.valid_account_required=true")
+	}
+	if cfg.AccountDefaults.Disabled {
+		glog.Warning(`With account_defaults.disabled=true, host-defined accounts must have "disabled":false. All other requests will be rejected.`)
+		if !cfg.ValidAccountRequired {
+			glog.Warning(`valid_account_required=false has no effect because account_defaults.disabled=true`)
+		}
+	}
 	return errs
 }
 
@@ -583,15 +596,11 @@ func New(v *viper.Viper) (*Configuration, error) {
 		return nil, err
 	}
 
-	// Migrate global settings to default account
-	if c.AccountRequired == false {
-		c.DefaultAccount.Disabled = false
-	}
-	c.DefaultAccount.CacheTTL = c.CacheURL.DefaultTTLs
-
+	// Update account defaults and generate base json for patch
+	c.AccountDefaults.CacheTTL = c.CacheURL.DefaultTTLs // comment this out to set explicitly in config
 	var err error
-	if c.DefaultAccountJSON, err = json.Marshal(c.DefaultAccount); err != nil {
-		glog.Warningf("Creating default account json: %v", err)
+	if c.AccountDefaultsJSON, err = json.Marshal(c.AccountDefaults); err != nil {
+		glog.Warningf("converting account_defaults to json: %v", err)
 	}
 
 	// To look for a request's publisher_id in the NonStandardPublishers list in
@@ -978,6 +987,8 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("blacklisted_apps", []string{""})
 	v.SetDefault("blacklisted_accts", []string{""})
 	v.SetDefault("account_required", false)
+	v.SetDefault("valid_account_required", false) // true: invalid pubIDs are rejected
+	v.SetDefault("account_defaults.disabled", false)
 	v.SetDefault("certificates_file", "")
 
 	v.SetDefault("request_timeout_headers.request_time_in_queue", "")

diff --git a/endpoints/openrtb2/auction.go b/endpoints/openrtb2/auction.go
@@ -1298,30 +1298,45 @@ func (deps *endpointDeps) getAccount(ctx context.Context, pubID string) (*config
 	var accountJSON json.RawMessage
 	var err error
 	if accountJSON, err = deps.accounts.FetchAccount(ctx, pubID); err != nil || accountJSON == nil {
-		if deps.cfg.DefaultAccount.Disabled || (deps.cfg.AccountRequired && pubID == pbsmetrics.PublisherUnknown) {
-			err = error(&errortypes.AcctRequired{
+		/* pubID does not reference a valid account, or is empty
+
+		   AccountRequired ValidAccRequired pubID        outcome
+		   false           false            empty        ✅
+		   false           false            invalid      ✅
+
+		   true            false            empty        🚫
+		   true            false            invalid      ✅  (legacy behavior)
+
+		   *               true             empty        🚫
+		   *               true             invalid      🚫
+		*/
+		if (deps.cfg.AccountRequired && pubID == pbsmetrics.PublisherUnknown) ||
+			deps.cfg.ValidAccountRequired {
+			return nil, error(&errortypes.AcctRequired{
 				Message: fmt.Sprintf("Prebid-server has been configured to discard requests without a valid Account ID. Please reach out to the prebid server host."),
 			})
 		}
-		// We have an unknown account, so use the DefaultAccount
-		// Transitional: make a copy of DefaultAccount instead of taking a reference,
+		// Make a copy of AccountDefaults instead of taking a reference,
 		// to preserve original pubID in case is needed to check NonStandardPublisherMap
-		pubAccount := deps.cfg.DefaultAccount
+		pubAccount := deps.cfg.AccountDefaults
 		pubAccount.ID = pubID
 		account = &pubAccount
 	} else {
+		// pubID resolved to a valid account, merge with AccountDefaults for a complete config
 		account = &config.Account{}
 		var completeJSON json.RawMessage
-		if completeJSON, err = jsonpatch.MergePatch(deps.cfg.DefaultAccountJSON, accountJSON); err == nil {
+		if completeJSON, err = jsonpatch.MergePatch(deps.cfg.AccountDefaultsJSON, accountJSON); err == nil {
 			err = json.Unmarshal(completeJSON, account)
 		}
-		// Fill in ID so we don't have to stored it in the object
-		account.ID = pubID
-		if account.Disabled {
-			err = error(&errortypes.BlacklistedAcct{
-				Message: fmt.Sprintf("Prebid-server has disabled Account ID: %s, please reach out to the prebid server host.", pubID),
-			})
+		// Fill in ID if needed, so it can be left out of account definition
+		if len(account.ID) == 0 {
+			account.ID = pubID
 		}
 	}
+	if account.Disabled {
+		err = error(&errortypes.BlacklistedAcct{
+			Message: fmt.Sprintf("Prebid-server has disabled Account ID: %s, please reach out to the prebid server host.", pubID),
+		})
+	}
 	return account, err
 }
diff --git a/endpoints/openrtb2/auction_test.go b/endpoints/openrtb2/auction_test.go
@@ -47,6 +47,7 @@ type getResponseFromDirectory struct {
 	disabledBidders []string
 	adaptersConfig  map[string]config.Adapter
 	accountReq      bool
+	validAccountReq bool
 	description     string
 }
 
@@ -260,10 +261,21 @@ func TestRejectAccountRequired(t *testing.T) {
 			file:          "with-acct.json",
 			payloadGetter: getRequestPayload,
 			messageGetter: nilReturner,
-			expectedCode:  http.StatusBadRequest,
+			expectedCode:  http.StatusOK,
 			aliased:       true,
 			accountReq:    true,
 		},
+		{
+			// Account is required, was provided, not blacklisted, but is not valid (not defined by host)
+			dir:             "sample-requests/account-required",
+			file:            "with-acct.json",
+			payloadGetter:   getRequestPayload,
+			messageGetter:   nilReturner,
+			expectedCode:    http.StatusBadRequest,
+			aliased:         true,
+			accountReq:      true,
+			validAccountReq: true,
+		},
 		{
 			// Account is required, was provided, not blacklisted and is a valid account
 			dir:           "sample-requests/account-required",
@@ -357,15 +369,15 @@ func (gr *getResponseFromDirectory) doRequest(t *testing.T, requestData []byte)
 	// As a side effect this gives us some coverage of the go_metrics piece of the metrics engine.
 	theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{})
 	cfg := config.Configuration{
-		MaxRequestSize:     maxSize,
-		BlacklistedApps:    []string{"spam_app"},
-		BlacklistedAppMap:  map[string]bool{"spam_app": true},
-		BlacklistedAccts:   []string{"bad_acct"},
-		BlacklistedAcctMap: map[string]bool{"bad_acct": true},
-		AccountRequired:    gr.accountReq,
-		DefaultAccount:     config.Account{Disabled: gr.accountReq},
-	}
-	cfg.DefaultAccountJSON, _ = json.Marshal(cfg.DefaultAccount)
+		MaxRequestSize:       maxSize,
+		BlacklistedApps:      []string{"spam_app"},
+		BlacklistedAppMap:    map[string]bool{"spam_app": true},
+		BlacklistedAccts:     []string{"bad_acct"},
+		BlacklistedAcctMap:   map[string]bool{"bad_acct": true},
+		AccountRequired:      gr.accountReq,
+		ValidAccountRequired: gr.validAccountReq,
+	}
+	cfg.AccountDefaultsJSON, _ = json.Marshal(cfg.AccountDefaults)
 	endpoint, _ := NewEndpoint(
 		&nobidExchange{},
 		newParamsValidator(t),