Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CCPA Phase 2: Enforcement #1138

Merged
merged 14 commits into from
Dec 16, 2019
2 changes: 1 addition & 1 deletion adapters/syncer.go
Original file line number Diff line number Diff line change
@@ -46,7 +46,7 @@ func (s *Syncer) GetUsersyncInfo(privacyPolicies privacy.Policies) (*usersync.Us
syncURL, err := macros.ResolveMacros(*s.urlTemplate, macros.UserSyncTemplateParams{
GDPR: privacyPolicies.GDPR.Signal,
GDPRConsent: privacyPolicies.GDPR.Consent,
USPrivacy: privacyPolicies.CCPA.Signal,
USPrivacy: privacyPolicies.CCPA.Value,
})
if err != nil {
return nil, err
2 changes: 1 addition & 1 deletion adapters/syncer_test.go
Original file line number Diff line number Diff line change
@@ -17,7 +17,7 @@ func TestGetUsersyncInfo(t *testing.T) {
Consent: "B",
},
CCPA: ccpa.Policy{
Signal: "C",
Value: "C",
},
}

4 changes: 1 addition & 3 deletions adapters/triplelift_native/triplelift_native.go
Original file line number Diff line number Diff line change
@@ -28,8 +28,6 @@ type TripleliftNativeExtInfo struct {
// Array is used for deserialization.
PublisherWhitelist []string `json:"publisher_whitelist"`

Endpoint string `json:"endpoint"`

// Map is used for optimized memory access and should be constructed after deserialization.
PublisherWhitelistMap map[string]bool
}
@@ -198,5 +196,5 @@ func NewTripleliftNativeBidder(client *http.Client, endpoint string, extraInfo s

return &TripleliftNativeAdapter{
extInfo: extInfo,
endpoint: extInfo.Endpoint}
endpoint: endpoint}
}
2 changes: 1 addition & 1 deletion adapters/triplelift_native/triplelift_native_test.go
Original file line number Diff line number Diff line change
@@ -19,5 +19,5 @@ func TestBadConfig(t *testing.T) {
}

func TestJsonSamples(t *testing.T) {
adapterstest.RunJSONBidderTest(t, "triplelift_nativetest", NewTripleliftNativeBidder(nil, "http://tlx.3lift.net/s2s/auction?supplier_id=19", "{\"publisher_whitelist\":[\"foo\",\"bar\",\"baz\"], \"endpoint\":\"http://tlx.3lift.net/s2sn/auction?supplier_id=20\"}"))
adapterstest.RunJSONBidderTest(t, "triplelift_nativetest", NewTripleliftNativeBidder(nil, "http://tlx.3lift.net/s2sn/auction?supplier_id=20", "{\"publisher_whitelist\":[\"foo\",\"bar\",\"baz\"]}"))
}
19 changes: 16 additions & 3 deletions config/config.go
Original file line number Diff line number Diff line change
@@ -47,6 +47,7 @@ type Configuration struct {
Analytics Analytics `mapstructure:"analytics"`
AMPTimeoutAdjustment int64 `mapstructure:"amp_timeout_adjustment_ms"`
GDPR GDPR `mapstructure:"gdpr"`
CCPA CCPA `mapstructure:"ccpa"`
CurrencyConverter CurrencyConverter `mapstructure:"currency_converter"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CCPA is a California State Bill, GDPR is a European law, what if other Privacy laws start getting enforced elsewhere like in Asia, Latin America or even another State of the Union? Do you think it makes more sense to put all of these privacy data structure under a single struct? We could call it PrivacyEnforcement, PrivacyConfig, or something else.

 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    -- 20 lines: AdminPort   int        `mapstructure:"admin_port"`-------------------------------------
 46        MaxRequestSize       int64              `mapstructure:"max_request_size"`
 47        Analytics            Analytics          `mapstructure:"analytics"`
 48        AMPTimeoutAdjustment int64              `mapstructure:"amp_timeout_adjustment_ms"`
 49 -      GDPR                 GDPR               `mapstructure:"gdpr"`
 50 -      CCPA                 CCPA               `mapstructure:"ccpa"`
    +      PrivacyConfig        PrivacyEnforcement `mapstructure:"privacy_info"`
 51        CurrencyConverter    CurrencyConverter  `mapstructure:"currency_converter"`
 52        DefReqConfig         DefReqConfig       `mapstructure:"default_request"`
 53
 54        VideoStoredRequestRequired bool `mapstructure:"video_stored_request_required"`
 55
 56    --  7 lines: Array of blacklisted apps that is used to create the hash table BlacklistedAppMap so Ap
 63        AccountRequired bool `mapstructure:"account_required"`
 64        // Local private file containing SSL certificates
 65        PemCertsFile string `mapstructure:"certificates_file"`
 66    }
    +  type PrivacyEnforcement struct {
    +      GDPR                 GDPR               `mapstructure:"gdpr"`
    +      CCPA                 CCPA               `mapstructure:"ccpa"`
    +  }
config/config.go [RO]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you on establishing a more common approach. This PR expands on the new privacy package to do just that. This here is just a temporary app config until Prebid.org devises an official enforcement stance.

DefReqConfig DefReqConfig `mapstructure:"default_request"`

@@ -160,6 +161,10 @@ func (t *GDPRTimeouts) ActiveTimeout() time.Duration {
return time.Duration(t.ActiveVendorlistFetch) * time.Millisecond
}

type CCPA struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This looks to be a long term option, with additional config coming in future PRs. You may want to re-review in that context. I don't want to merge GDPR and CCPA in the config object because I don't want to break existing GDPR config.

Enforce bool `mapstructure:"enforce"`
}

type Analytics struct {
File FileLogs `mapstructure:"file"`
}
@@ -202,6 +207,7 @@ const (
dummyPublisherID string = "12"
dummyGDPR string = "0"
dummyGDPRConsent string = "someGDPRConsentString"
dummyCCPA string = "1NYN"
)

type Adapter struct {
@@ -216,8 +222,9 @@ type Adapter struct {
//
// This value will be interpreted as a Golang Template. At runtime, the following Template variables will be replaced.
//
// {{.GDPR}} -- This will be replaced with the "gdpr" property sent to /cookie_sync
// {{.Consent}} -- This will be replaced with the "consent" property sent to /cookie_sync
// {{.GDPR}} -- This will be replaced with the "gdpr" property sent to /cookie_sync
// {{.Consent}} -- This will be replaced with the "consent" property sent to /cookie_sync
// {{.USPrivacy}} -- This will be replaced with the "us_privacy" property sent to /cookie_sync
//
// For more info on templates, see: https://golang.org/pkg/text/template/
UserSyncURL string `mapstructure:"usersync_url"`
@@ -275,7 +282,12 @@ func validateAdapterUserSyncURL(userSyncURL string, adapterName string, errs con
return append(errs, fmt.Errorf("Invalid user sync URL template: %s for adapter: %s. %v", userSyncURL, adapterName, err))
}
// Resolve macros (if any) in the user_sync URL
resolvedUserSyncURL, err := macros.ResolveMacros(*userSyncTemplate, macros.UserSyncTemplateParams{GDPR: dummyGDPR, GDPRConsent: dummyGDPRConsent})
dummyMacroValues := macros.UserSyncTemplateParams{
GDPR: dummyGDPR,
GDPRConsent: dummyGDPRConsent,
USPrivacy: dummyCCPA,
}
resolvedUserSyncURL, err := macros.ResolveMacros(*userSyncTemplate, dummyMacroValues)
if err != nil {
return append(errs, fmt.Errorf("Unable to resolve user sync URL: %s for adapter: %s. %v", userSyncURL, adapterName, err))
}
@@ -706,6 +718,7 @@ func SetupViper(v *viper.Viper, filename string) {
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{""})
v.SetDefault("ccpa.enforce", false)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default to disabled for now. Will change in the future when the Prebid.org CCPA enforcement spec is finalized.

v.SetDefault("currency_converter.fetch_url", "https://cdn.jsdelivr.net/gh/prebid/currency-file@1/latest.json")
v.SetDefault("currency_converter.fetch_interval_seconds", 1800) // fetch currency rates every 30 minutes
v.SetDefault("default_request.type", "")
8 changes: 6 additions & 2 deletions config/config_test.go
Original file line number Diff line number Diff line change
@@ -41,6 +41,8 @@ gdpr:
host_vendor_id: 15
usersync_if_ambiguous: true
non_standard_publishers: ["siteID","fake-site-id","appID","agltb3B1Yi1pbmNyDAsSA0FwcBiJkfIUDA"]
ccpa:
enforce: true
host_cookie:
cookie_name: userid
family: prebid
@@ -102,7 +104,7 @@ adapters:
username: rubiuser
password: rubipw23
brightroll:
usersync_url: http://test-bh.ybp.yahoo.com/sync/appnexuspbs?gdpr={{.GDPR}}&euconsent={{.GDPRConsent}}&url=%s
usersync_url: http://test-bh.ybp.yahoo.com/sync/appnexuspbs?gdpr={{.GDPR}}&euconsent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&url=%s
endpoint: http://test-bid.ybp.yahoo.com/bid/appnexuspbs
adkerneladn:
usersync_url: https://tag.adkernel.com/syncr?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&r=
@@ -230,6 +232,8 @@ func TestFullConfig(t *testing.T) {
_, found = cfg.GDPR.NonStandardPublisherMap["appnexus"]
cmpBools(t, "cfg.GDPR.NonStandardPublisherMap", found, false)

cmpBools(t, "ccpa.enforce", cfg.CCPA.Enforce, true)

//Assert the NonStandardPublishers was correctly unmarshalled
cmpStrings(t, "blacklisted_apps", cfg.BlacklistedApps[0], "spamAppID")
cmpStrings(t, "blacklisted_apps", cfg.BlacklistedApps[1], "sketchy-app-id")
@@ -265,7 +269,7 @@ func TestFullConfig(t *testing.T) {
cmpStrings(t, "adapters.rubicon.xapi.username", cfg.Adapters[string(openrtb_ext.BidderRubicon)].XAPI.Username, "rubiuser")
cmpStrings(t, "adapters.rubicon.xapi.password", cfg.Adapters[string(openrtb_ext.BidderRubicon)].XAPI.Password, "rubipw23")
cmpStrings(t, "adapters.brightroll.endpoint", cfg.Adapters[string(openrtb_ext.BidderBrightroll)].Endpoint, "http://test-bid.ybp.yahoo.com/bid/appnexuspbs")
cmpStrings(t, "adapters.brightroll.usersync_url", cfg.Adapters[string(openrtb_ext.BidderBrightroll)].UserSyncURL, "http://test-bh.ybp.yahoo.com/sync/appnexuspbs?gdpr={{.GDPR}}&euconsent={{.GDPRConsent}}&url=%s")
cmpStrings(t, "adapters.brightroll.usersync_url", cfg.Adapters[string(openrtb_ext.BidderBrightroll)].UserSyncURL, "http://test-bh.ybp.yahoo.com/sync/appnexuspbs?gdpr={{.GDPR}}&euconsent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&url=%s")
cmpStrings(t, "adapters.adkerneladn.usersync_url", cfg.Adapters[strings.ToLower(string(openrtb_ext.BidderAdkernelAdn))].UserSyncURL, "https://tag.adkernel.com/syncr?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&r=")
cmpStrings(t, "adapters.rhythmone.endpoint", cfg.Adapters[string(openrtb_ext.BidderRhythmone)].Endpoint, "http://tag.1rx.io/rmp")
cmpStrings(t, "adapters.rhythmone.usersync_url", cfg.Adapters[string(openrtb_ext.BidderRhythmone)].UserSyncURL, "https://sync.1rx.io/usersync2/rmphb?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&redir=http%3A%2F%2Fprebid-server.prebid.org%2F%2Fsetuid%3Fbidder%3Drhythmone%26gdpr%3D{{.GDPR}}%26gdpr_consent%3D{{.GDPRConsent}}%26uid%3D%5BRX_UUID%5D")
35 changes: 22 additions & 13 deletions endpoints/cookie_sync.go
Original file line number Diff line number Diff line change
@@ -32,6 +32,7 @@ func NewCookieSyncEndpoint(syncers map[openrtb_ext.BidderName]usersync.Usersynce
syncPermissions: syncPermissions,
metrics: metrics,
pbsAnalytics: pbsAnalytics,
enforceCCPA: cfg.CCPA.Enforce,
}
return deps.Endpoint
}
@@ -43,6 +44,7 @@ type cookieSyncDeps struct {
syncPermissions gdpr.Permissions
metrics pbsmetrics.MetricsEngine
pbsAnalytics analytics.PBSAnalyticsModule
enforceCCPA bool
}

func (deps *cookieSyncDeps) Endpoint(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
@@ -103,15 +105,26 @@ func (deps *cookieSyncDeps) Endpoint(w http.ResponseWriter, r *http.Request, _ h
}
}

privacyPolicy := privacy.Policies{
GDPR: gdprPolicy.Policy{
Signal: gdprToString(parsedReq.GDPR),
Consent: parsedReq.Consent,
},
CCPA: ccpa.Policy{
Value: parsedReq.USPrivacy,
},
}

parsedReq.filterExistingSyncs(deps.syncers, userSyncCookie, needSyncupForSameSite)

adapterSyncs := make(map[openrtb_ext.BidderName]bool)
// assume all bidders will be privacy blocked
for _, b := range parsedReq.Bidders {
// assume all bidders will be GDPR blocked
adapterSyncs[openrtb_ext.BidderName(b)] = true
}
parsedReq.filterForGDPR(deps.syncPermissions)
parsedReq.filterForPrivacy(deps.syncPermissions, privacyPolicy, deps.enforceCCPA)
// surviving bidders are not privacy blocked
for _, b := range parsedReq.Bidders {
// surviving bidders are not GDPR blocked
adapterSyncs[openrtb_ext.BidderName(b)] = false
}
for b, g := range adapterSyncs {
@@ -125,15 +138,6 @@ func (deps *cookieSyncDeps) Endpoint(w http.ResponseWriter, r *http.Request, _ h
}
for i := 0; i < len(parsedReq.Bidders); i++ {
bidder := parsedReq.Bidders[i]
privacyPolicy := privacy.Policies{
GDPR: gdprPolicy.Policy{
Signal: gdprToString(parsedReq.GDPR),
Consent: parsedReq.Consent,
},
CCPA: ccpa.Policy{
Signal: parsedReq.USPrivacy,
},
}
syncInfo, err := deps.syncers[openrtb_ext.BidderName(bidder)].GetUsersyncInfo(privacyPolicy)
if err == nil {
newSync := &usersync.CookieSyncBidders{
@@ -219,7 +223,12 @@ func (req *cookieSyncRequest) filterExistingSyncs(valid map[openrtb_ext.BidderNa
}
}

func (req *cookieSyncRequest) filterForGDPR(permissions gdpr.Permissions) {
func (req *cookieSyncRequest) filterForPrivacy(permissions gdpr.Permissions, privacyPolicies privacy.Policies, enforceCCPA bool) {
if enforceCCPA && privacyPolicies.CCPA.ShouldEnforce() {
req.Bidders = nil
return
}

if req.GDPR != nil && *req.GDPR == 0 {
return
}
68 changes: 62 additions & 6 deletions endpoints/cookie_sync_test.go
Original file line number Diff line number Diff line change
@@ -65,6 +65,61 @@ func TestGDPRConsentRequired(t *testing.T) {
assert.Equal(t, "gdpr_consent is required if gdpr=1\n", rr.Body.String())
}

func TestCCPA(t *testing.T) {
testCases := []struct {
description string
requestBody string
enforceCCPA bool
expectedSyncs []string
}{
{
description: "Feature Flag On & Opt-Out Yes",
requestBody: `{"bidders":["appnexus"], "us_privacy":"1-Y-"}`,
enforceCCPA: true,
expectedSyncs: []string{},
},
{
description: "Feature Flag Off & Opt-Out Yes",
requestBody: `{"bidders":["appnexus"], "us_privacy":"1-Y-"}`,
enforceCCPA: false,
expectedSyncs: []string{"appnexus"},
},
{
description: "Feature Flag On & Opt-Out No",
requestBody: `{"bidders":["appnexus"], "us_privacy":"1-N-"}`,
enforceCCPA: false,
expectedSyncs: []string{"appnexus"},
},
{
description: "Feature Flag On & Opt-Out Unknown",
requestBody: `{"bidders":["appnexus"], "us_privacy":"1---"}`,
enforceCCPA: false,
expectedSyncs: []string{"appnexus"},
},
{
description: "Feature Flag On & Opt-Out Invalid",
requestBody: `{"bidders":["appnexus"], "us_privacy":"invalid"}`,
enforceCCPA: false,
expectedSyncs: []string{"appnexus"},
},
{
description: "Feature Flag On & Opt-Out Not Provided",
requestBody: `{"bidders":["appnexus"]}`,
enforceCCPA: false,
expectedSyncs: []string{"appnexus"},
},
}

for _, test := range testCases {
gdpr := config.GDPR{UsersyncIfAmbiguous: true}
ccpa := config.CCPA{Enforce: test.enforceCCPA}
rr := doConfigurablePost(test.requestBody, nil, true, syncersForTest(), gdpr, ccpa)
assert.Equal(t, http.StatusOK, rr.Code, test.description+":httpResponseCode")
assert.ElementsMatch(t, test.expectedSyncs, parseSyncs(t, rr.Body.Bytes()), test.description+":syncs")
assert.Equal(t, "no_cookie", parseStatus(t, rr.Body.Bytes()), test.description+":status")
}
}

func TestCookieSyncHasCookies(t *testing.T) {
rr := doPost(`{"bidders":["appnexus", "audienceNetwork", "random"]}`, map[string]string{
"adnxs": "1234",
@@ -95,7 +150,7 @@ func TestCookieSyncNoBidders(t *testing.T) {
}

func TestCookieSyncNoCookiesBrokenGDPR(t *testing.T) {
rr := doConfigurablePost(`{"bidders":["appnexus", "audienceNetwork", "random"],"gdpr_consent":"GLKHGKGKKGK"}`, nil, true, map[openrtb_ext.BidderName]usersync.Usersyncer{}, config.GDPR{UsersyncIfAmbiguous: true})
rr := doConfigurablePost(`{"bidders":["appnexus", "audienceNetwork", "random"],"gdpr_consent":"GLKHGKGKKGK"}`, nil, true, map[openrtb_ext.BidderName]usersync.Usersyncer{}, config.GDPR{UsersyncIfAmbiguous: true}, config.CCPA{})
assert.Equal(t, rr.Header().Get("Content-Type"), "application/json; charset=utf-8")
assert.Equal(t, http.StatusOK, rr.Code)
assert.ElementsMatch(t, []string{"appnexus", "audienceNetwork"}, parseSyncs(t, rr.Body.Bytes()))
@@ -118,15 +173,16 @@ func TestCookieSyncWithLargeLimit(t *testing.T) {
}

func doPost(body string, existingSyncs map[string]string, gdprHostConsent bool, gdprBidders map[openrtb_ext.BidderName]usersync.Usersyncer) *httptest.ResponseRecorder {
return doConfigurablePost(body, existingSyncs, gdprHostConsent, gdprBidders, config.GDPR{})
return doConfigurablePost(body, existingSyncs, gdprHostConsent, gdprBidders, config.GDPR{}, config.CCPA{})
}

func doConfigurablePost(body string, existingSyncs map[string]string, gdprHostConsent bool, gdprBidders map[openrtb_ext.BidderName]usersync.Usersyncer, cfgGDPR config.GDPR) *httptest.ResponseRecorder {
endpoint := testableEndpoint(mockPermissions(gdprHostConsent, gdprBidders), cfgGDPR)
func doConfigurablePost(body string, existingSyncs map[string]string, gdprHostConsent bool, gdprBidders map[openrtb_ext.BidderName]usersync.Usersyncer, cfgGDPR config.GDPR, cfgCCPA config.CCPA) *httptest.ResponseRecorder {
endpoint := testableEndpoint(mockPermissions(gdprHostConsent, gdprBidders), cfgGDPR, cfgCCPA)
router := httprouter.New()
router.POST("/cookie_sync", endpoint)
req, _ := http.NewRequest("POST", "/cookie_sync", strings.NewReader(body))
if len(existingSyncs) > 0 {

pcs := usersync.NewPBSCookie()
for bidder, uid := range existingSyncs {
pcs.TrySync(bidder, uid)
@@ -139,8 +195,8 @@ func doConfigurablePost(body string, existingSyncs map[string]string, gdprHostCo
return rr
}

func testableEndpoint(perms gdpr.Permissions, cfgGDPR config.GDPR) httprouter.Handle {
return NewCookieSyncEndpoint(syncersForTest(), &config.Configuration{GDPR: cfgGDPR}, perms, &metricsConf.DummyMetricsEngine{}, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
func testableEndpoint(perms gdpr.Permissions, cfgGDPR config.GDPR, cfgCCPA config.CCPA) httprouter.Handle {
return NewCookieSyncEndpoint(syncersForTest(), &config.Configuration{GDPR: cfgGDPR, CCPA: cfgCCPA}, perms, &metricsConf.DummyMetricsEngine{}, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
}

func syncersForTest() map[openrtb_ext.BidderName]usersync.Usersyncer {
16 changes: 2 additions & 14 deletions endpoints/openrtb2/amp_auction.go
Original file line number Diff line number Diff line change
@@ -384,13 +384,12 @@ func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *ope

privacyPolicies := privacy.Policies{
GDPR: gdpr.Policy{
Consent: getQueryParam(httpRequest, "gdpr_consent"),
Consent: httpRequest.URL.Query().Get("gdpr_consent"),
},
CCPA: ccpa.Policy{
Signal: getQueryParam(httpRequest, "us_privacy"),
Value: httpRequest.URL.Query().Get("us_privacy"),
},
}

if err := privacyPolicies.Write(req); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following was tested in the Go playground:

package main

import (
	"fmt"
	"log"
	"net/url"
)

func main() {
	u, err := url.Parse("http://bing.com/search?q=dotnet")
	if err != nil {
		log.Fatal(err)
	}
	
	fmt.Printf("Value of Get(\"gdpr_consent\") = \"%s\" \n",u.Query().Get("gdpr_consent"))
	fmt.Printf("Lenght of Get(\"gdpr_consent\") = %d \n",len(u.Query().Get("gdpr_consent")))
	fmt.Printf("Value of Get(\"q\") = \"%s\" \n",u.Query().Get("q"))
}

Output is:

Value of Get("gdpr_consent") = "" 
Lenght of Get("gdpr_consent") = 0 
Value of Get("q") = "dotnet" 

Therefore, is fair to assume Query().Get("gdpr_consent") is safe to use and we could spare maintaining one less function

346   func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *openrtb.BidRequest) error {
347       if req.Site == nil {
348           req.Site = &openrtb.Site{}
349       }
350   --- 34 lines: Override the stored request sizes with AMP ones, if they exist.-----------------------------------------------------------------------------------------------------------
384
385       privacyPolicies := privacy.Policies{
386           GDPR: gdpr.Policy{
387 -             Consent: getQueryParam(httpRequest, "gdpr_consent"),
    +             Consent: httpRequest.URL.Query().Get("gdpr_consent"), //No nil pointer dereference because Query returns a non-nil map
388           },
389           CCPA: ccpa.Policy{
390 -             Value: getQueryParam(httpRequest, "us_privacy"),
    +             Value: httpRequest.URL.Query().Get("us_privacy"),
391           },
392       }
393       if err := privacyPolicies.Write(req); err != nil {
394           return err
395       }
396
397   ---  3 lines: if timeout, err := strconv.ParseInt(httpRequest.FormValue("timeout"), 10, 64); err == nil {-------------------------------------------------------------------------------
400
401       return nil
402   }
403
404 - func getQueryParam(httpRequest *http.Request, name string) string {
405 -     values, ok := httpRequest.URL.Query()[name]
406 -
407 -     if !ok || len(values) == 0 {
408 -         return ""
409 -     }
410 -
411 -     // return first value of the query param, matching the behavior of httpRequest.FormValue
412 -     return values[0]
413 - }
414
endpoints/openrtb2/amp_auction.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much. Changed.

return err
}
@@ -402,17 +401,6 @@ func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *ope
return nil
}

func getQueryParam(httpRequest *http.Request, name string) string {
values, ok := httpRequest.URL.Query()[name]

if !ok || len(values) == 0 {
return ""
}

// return first value of the query param, matching the behavior of httpRequest.FormValue
return values[0]
}

func makeFormatReplacement(overrideWidth uint64, overrideHeight uint64, width uint64, height uint64, multisize string) []openrtb.Format {
if overrideWidth != 0 && overrideHeight != 0 {
return []openrtb.Format{{
11 changes: 11 additions & 0 deletions endpoints/openrtb2/auction.go
Original file line number Diff line number Diff line change
@@ -27,6 +27,7 @@ import (
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/pbsmetrics"
"github.com/prebid/prebid-server/prebid"
"github.com/prebid/prebid-server/privacy/ccpa"
"github.com/prebid/prebid-server/stored_requests"
"github.com/prebid/prebid-server/stored_requests/backends/empty_fetcher"
"github.com/prebid/prebid-server/usersync"
@@ -301,6 +302,16 @@ func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
return errL
}

ccpaPolicy, ccpaPolicyErr := ccpa.ReadPolicy(req)
if ccpaPolicyErr != nil {
errL = append(errL, ccpaPolicyErr)
return errL
}

if err := ccpaPolicy.Validate(); err != nil {
Copy link
Contributor Author

@SyntaxNode SyntaxNode Dec 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the guidance I have so far, an invalid CCPA value can be ignored but I think a warning would be useful. Right now, warnings are not included in the bid response, but I view that as a separate issue.

errL = append(errL, &errortypes.Warning{Message: fmt.Sprintf("CCPA value is invalid and will be ignored. (%s)", err.Error())})
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReadPolicy(req *openrtb.BidRequest) unnecessarily checks Regs and Regs.Ext for a second time so, given that validateRegs(req.Regs) already does it in line 300, can we validate CCPA in there too? We could probably spare maintaining an extra function if we just call policy.Validate () and check for errors from inside validateRegs(req.Regs) on the already unmarshalled ext object.

 246   func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
 247       errL := []error{}
 248       if req.ID == "" {
 249           return []error{errors.New("request missing required field: \"id\"")}
 250       }
 251   --- 48 lines: if req.TMax < 0 {-----------------------------------------------
 299
 300       if err := validateRegs(req.Regs); err != nil {
 301           errL = append(errL, err)
 302           return errL
 303       }
 304
 305 -     ccpaPolicy, ccpaPolicyErr := ccpa.ReadPolicy(req)
 306 -     if ccpaPolicyErr != nil {
 307 -         errL = append(errL, ccpaPolicyErr)
 308 -         return errL
 309 -     }
 310 -
 311 -     if err := ccpaPolicy.Validate(); err != nil {
 312 -         errL = append(errL, err)
 313 -         return errL
 314 -     }
 315
 316       impIDs := make(map[string]int, len(req.Imp))
 317       for index := range req.Imp {
 318   --- 12 lines: imp := &req.Imp[index]------------------------------------------
 330       }
 331
 332       return errL
 333   } 
 334
 335   ---558 lines: func validateBidAdjustmentFactors(adjustmentFactors map[string--
 893
 894   func validateRegs(regs *openrtb.Regs) (openrtb_ext.ExtRegs,error) {
 895       if regs != nil && len(regs.Ext) > 0 {
 896           var regsExt openrtb_ext.ExtRegs
 897           if err := json.Unmarshal(regs.Ext, &regsExt); err != nil {
 898               return fmt.Errorf("request.regs.ext is invalid: %v", err)
 899           }
 900           if regsExt.GDPR != nil && (*regsExt.GDPR < 0 || *regsExt.GDPR > 1) {
 901               return errors.New("request.regs.ext.gdpr must be either 0 or 1.")
 902           }
 903 +         ccpaPolicy := Policy{Value = regsExt.USPrivacy}
 904 +         if err := ccpaPolicy.Validate(); err != nil {
 905 +             return err
 906 +         }
 907       }
 908       return nil
 909   }
endpoints/openrtb2/auction.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I consider this part of 'transition pain'. I'm trying to keep all privacy logic in each respective privacy policy and will move the GDPR and COPPA logic there too, but that's a bit too much for this PR. Do you think this will cause a major performance issue? I could restructure a few things for now to avoid the extra unmarshal if you feel it's better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'll think it'd be perfectly fine to refactor in another PR latter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Can we write a test case to cover ReadPolicy(req) error scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. That code isn't reachable. It will only get triggered if the regs.ext is malformed and there' s already a check for that earlier in the func. File this under pain of a partial refactor of privacy things. More to come soon.

impIDs := make(map[string]int, len(req.Imp))
for index := range req.Imp {
imp := &req.Imp[index]
Loading