Add an app.id blacklist to PBS and reject requests from it

[guscarreon]

Still to do:

• Get the list with app ids to include in the list
• Configure environment variable in bitbucket

[mansinahar]

On this line: https://github.com/prebid/prebid-server/pull/969/files#diff-09e5d415f33d74fd6df20b2584f318d1R110, the if fatalError(errL) && writeError(errL, w) condition will be false in case of BlacklistedApp errors and so we will continue processing the request. I don't think that's the expected behavior, right?

[mansinahar]

@guscarreon Do we need a slice with empty string as default? I mean why can't it be just an empty slice?

[guscarreon]

Since the flow keyword was added, the BlacklistedApps field in line 55 below expects an array of strings.

 21 type Configuration struct {
 22     ExternalURL string     `mapstructure:"external_url"`
 23 +-- 28 lines: Host        string     `mapstructure:"host"`---------------------------------------
 51
 52     VideoStoredRequestRequired bool `mapstructure:"video_stored_request_required"`
 53
 54     // PBS-418 array of blacklisted apps and its correpondent hash table to be instantly accessed.
 55     BlacklistedApps   []string `mapstructure:"blacklisted_apps,flow"`
 56     BlacklistedAppMap map[string]bool
 57 }
 58

Therefore, I figured it was better to add a slice with an empty string because other string fields of the Configuration struct add an empty string instead of a nil string as default. Take the Host field for example, in config/config.go its default is set to an empty string as well as StatusResponse and RecaptchaSecret to name a few:

 21 type Configuration struct {
 22     ExternalURL string     `mapstructure:"external_url"`
 23     Host        string     `mapstructure:"host"`
 24     Port        int        `mapstructure:"port"`
 25     Client      HTTPClient `mapstructure:"http_client"`
 26     AdminPort   int        `mapstructure:"admin_port"`
 27     EnableGzip  bool       `mapstructure:"enable_gzip"`
 28     // StatusResponse is the string which will be returned by the /status endpoint when things are OK.
 29     // If empty, it will return a 204 with no content.
 30     StatusResponse  string             `mapstructure:"status_response"`
 31     AuctionTimeouts AuctionTimeouts    `mapstructure:"auction_timeouts_ms"`
 32     CacheURL        Cache              `mapstructure:"cache"`
 33     RecaptchaSecret string             `mapstructure:"recaptcha_secret"`
 34     HostCookie      HostCookie         `mapstructure:"host_cookie"`
 35     Metrics         Metrics            `mapstructure:"metrics"`
 36 +--444 lines: DataCache       DataCache          `mapstructure:"datacache"`---------------------
480 func SetupViper(v *viper.Viper, filename string) {
481     if filename != "" {
482         v.SetConfigName(filename)
483         v.AddConfigPath(".")
484         v.AddConfigPath("/etc/config")
485     }
486     // Fixes #475: Some defaults will be set just so they are accessible via environment variables
487     // (basically so viper knows they exist)
488     v.SetDefault("external_url", "http://localhost:8000")
489     v.SetDefault("host", "")
490     v.SetDefault("port", 8000)
491     v.SetDefault("admin_port", 6060)
492     v.SetDefault("enable_gzip", false)
493     v.SetDefault("status_response", "")
494     v.SetDefault("auction_timeouts_ms.default", 0)
495     v.SetDefault("auction_timeouts_ms.max", 0)
496     v.SetDefault("cache.scheme", "")
497     v.SetDefault("cache.host", "")
498     v.SetDefault("cache.query", "")
499     v.SetDefault("cache.expected_millis", 10)
500     v.SetDefault("cache.default_ttl_seconds.banner", 0)
501     v.SetDefault("cache.default_ttl_seconds.video", 0)
502     v.SetDefault("cache.default_ttl_seconds.native", 0)
503     v.SetDefault("cache.default_ttl_seconds.audio", 0)
504     v.SetDefault("recaptcha_secret", "")
505     v.SetDefault("host_cookie.domain", "")
506     v.SetDefault("host_cookie.family", "")

[mansinahar]

Again, we should check the entire errs slice rather than just the first element.

[guscarreon]

corrected

[guscarreon]

On this line: https://github.com/prebid/prebid-server/pull/969/files#diff-09e5d415f33d74fd6df20b2584f318d1R110, the if fatalError(errL) && writeError(errL, w) condition will be false in case of BlacklistedApp errors and so we will continue processing the request. I don't think that's the expected behavior, right?

Good catch! Corrected.

[mansinahar]

You never set the foundBlacklisted anywhere in the above code, so this block will always get executed. I don't think that's what we want.

[guscarreon]

You are right. Corrected