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.

Sign up for GitHub

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
Merged

CCPA Phase 2: Enforcement #1138

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ace7f43
Implement CCPA For Auctions
SyntaxNode Dec 10, 2019
534c39a
Added CCPA Enforce Flag
SyntaxNode Dec 11, 2019
a219ded
Merge branch 'master' into ccpa_enforce
SyntaxNode Dec 11, 2019
a237cf7
Ignore (With Warning) Invalid CCPA Values
SyntaxNode Dec 11, 2019
675efaf
Tweaked Name Of Test JSON
SyntaxNode Dec 11, 2019
7e25811
PR Feedback
SyntaxNode Dec 12, 2019
75179b3
Removed Unused Method
SyntaxNode Dec 12, 2019
c810703
Enforce CCPA For Cookie Sync
SyntaxNode Dec 13, 2019
6b43612
Case Sensitivity Is Intended Per Spec
SyntaxNode Dec 13, 2019
f074e60
Added Feature Flag Test
SyntaxNode Dec 13, 2019
db833a1
Expanded Tests + Comments
SyntaxNode Dec 13, 2019
44da58e
Formatting
SyntaxNode Dec 13, 2019
dc94e39
Throwing In A Small TL Update
SyntaxNode Dec 13, 2019
40bcddc
Added Exchange Tests
SyntaxNode Dec 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion adapters/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion adapters/syncer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func TestGetUsersyncInfo(t *testing.T) {
Consent: "B",
},
CCPA: ccpa.Policy{
Signal: "C",
Value: "C",
},
}

Expand Down
20 changes: 17 additions & 3 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`

Expand Down Expand Up @@ -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"`
}
Expand Down Expand Up @@ -202,6 +207,7 @@ const (
dummyPublisherID string = "12"
dummyGDPR string = "0"
dummyGDPRConsent string = "someGDPRConsentString"
dummyCCPA string = "1NYN"
)

type Adapter struct {
Expand All @@ -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"`
Expand Down Expand Up @@ -275,7 +282,13 @@ 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))
}
Expand Down Expand Up @@ -706,6 +719,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", "")
Expand Down
8 changes: 6 additions & 2 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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=
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
35 changes: 22 additions & 13 deletions endpoints/cookie_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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{
Expand Down Expand Up @@ -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
}
Expand Down
68 changes: 62 additions & 6 deletions endpoints/cookie_sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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()))
Expand All @@ -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)
Expand All @@ -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 {
Expand Down
16 changes: 2 additions & 14 deletions endpoints/openrtb2/amp_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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{{
Expand Down
11 changes: 11 additions & 0 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 •
edited
Loading

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]
Expand Down
Loading