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 1: AMP Endpoint #1125

Merged
merged 4 commits into from
Dec 6, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
43 changes: 27 additions & 16 deletions endpoints/openrtb2/amp_auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ import (
"github.com/prebid/prebid-server/exchange"
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/pbsmetrics"
"github.com/prebid/prebid-server/privacy"
"github.com/prebid/prebid-server/privacy/ccpa"
"github.com/prebid/prebid-server/privacy/gdpr"
"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 @@ -379,22 +382,19 @@ func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *ope
req.Imp[0].TagID = slot
}

//In the AMP endpoint the consent string found in the http.Request query overrides that of the prebid query
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GDPR implementation is scattered throughout the code. I'm beginning to consolidate the logic to a privacy package. I did a full refactor locally, but it was way too much change... so I'll be phasing it in.

queryConsentString := httpRequest.FormValue("gdpr_consent")
if queryConsentString != "" {
jsonMsg := json.RawMessage(`{"consent":"` + queryConsentString + `"}`)
// If nil, initialize
if req.User == nil {
req.User = &openrtb.User{Ext: jsonMsg}
} else if req.User.Ext == nil {
req.User.Ext = jsonMsg
} else { // req.User.Ext != nil, keep whatever is in there and only substitute the consent string
var parserErr error
req.User.Ext, parserErr = jsonparser.Set(req.User.Ext, []byte(`"`+queryConsentString+`"`), "consent")
if parserErr != nil {
return parserErr
}
}
gdprConsent := getQueryParam(httpRequest, "gdpr_consent")
ccpaValue := getQueryParam(httpRequest, "us_privacy")
privacyPolicies := privacy.Policies{
GDPR: gdpr.Policy{
Consent: gdprConsent,
},
CCPA: ccpa.Policy{
Value: ccpaValue,
},
}

if err := privacyPolicies.Write(req); err != nil {
return err
}

if timeout, err := strconv.ParseInt(httpRequest.FormValue("timeout"), 10, 64); err == nil {
Expand All @@ -404,6 +404,17 @@ 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
96 changes: 96 additions & 0 deletions endpoints/openrtb2/amp_auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,102 @@ func TestWidthOnly(t *testing.T) {
}.execute(t)
}

func TestCCPAPresent(t *testing.T) {
req, err := getTestBidRequest(false, false, "", "digitrustId")
if err != nil {
t.Fatalf("Failed to marshal the complete openrtb.BidRequest object %v", err)
}

reqStored := map[string]json.RawMessage{
"1": json.RawMessage(req),
}

theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{})

exchange := &mockAmpExchange{}

endpoint, _ := NewAmpEndpoint(
exchange,
newParamsValidator(t),
&mockAmpStoredReqFetcher{reqStored},
empty_fetcher.EmptyFetcher{},
&config.Configuration{MaxRequestSize: maxSize},
theMetrics,
analyticsConf.NewPBSAnalytics(&config.Analytics{}),
map[string]string{},
[]byte{},
openrtb_ext.BidderMap,
)

usPrivacy := "1YYN"
httpReq := httptest.NewRequest("GET", "/openrtb2/auction/amp?tag_id=1&us_privacy="+usPrivacy, nil)
httpRecorder := httptest.NewRecorder()
endpoint(httpRecorder, httpReq, nil)

// Assert our bidRequest was valid
if !assert.NotNil(t, exchange.lastRequest, "Endpoint responded with %d: %s", httpRecorder.Code, httpRecorder.Body.String()) {
return
}
// Assert our bidRequest had a valid "Regs" field
if !assert.NotNil(t, exchange.lastRequest.Regs) {
return
}
// Assert our bidRequest had a valid "Regs.Ext" field
if !assert.NotNil(t, exchange.lastRequest.Regs.Ext) {
return
}

var regs openrtb_ext.ExtRegs
err = json.Unmarshal(exchange.lastRequest.Regs.Ext, &regs)
assert.NoError(t, err)
assert.Equal(t, usPrivacy, regs.USPrivacy)
}

func TestCCPANotPresent(t *testing.T) {
req, err := getTestBidRequest(false, false, "", "digitrustId")
if err != nil {
t.Fatalf("Failed to marshal the complete openrtb.BidRequest object %v", err)
}

reqStored := map[string]json.RawMessage{
"1": json.RawMessage(req),
}

theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{})

exchange := &mockAmpExchange{}

endpoint, _ := NewAmpEndpoint(
exchange,
newParamsValidator(t),
&mockAmpStoredReqFetcher{reqStored},
empty_fetcher.EmptyFetcher{},
&config.Configuration{MaxRequestSize: maxSize},
theMetrics,
analyticsConf.NewPBSAnalytics(&config.Analytics{}),
map[string]string{},
[]byte{},
openrtb_ext.BidderMap,
)

httpReq := httptest.NewRequest("GET", "/openrtb2/auction/amp?tag_id=1", nil)
httpRecorder := httptest.NewRecorder()
endpoint(httpRecorder, httpReq, nil)

// Assert our bidRequest was valid
if !assert.NotNil(t, exchange.lastRequest, "Endpoint responded with %d: %s", httpRecorder.Code, httpRecorder.Body.String()) {
return
}

// Assert CCPA Signal Not Found
if exchange.lastRequest.Regs != nil && exchange.lastRequest.Regs.Ext != nil {
var regs openrtb_ext.ExtRegs
err = json.Unmarshal(exchange.lastRequest.Regs.Ext, &regs)
assert.NoError(t, err)
assert.Empty(t, regs.USPrivacy)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log an error if we don't make it into the true branch of the if statement?

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 don't think so. I'm testing that the signal isn't present, so if there is no regs or no regs.ext or an empty regs.ext.us_privacy then I think the test should pass.

}

type formatOverrideSpec struct {
width uint64
height uint64
Expand Down
3 changes: 3 additions & 0 deletions openrtb_ext/regs.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,7 @@ type ExtRegs struct {
// GDPR should be "1" if the caller believes the user is subject to GDPR laws, "0" if not, and undefined
// if it's unknown. For more info on this parameter, see: https://iabtechlab.com/wp-content/uploads/2018/02/OpenRTB_Advisory_GDPR_2018-02.pdf
GDPR *int8 `json:"gdpr,omitempty"`

// USPrivacy should be a four character string, see: https://iabtechlab.com/wp-content/uploads/2019/11/OpenRTB-Extension-U.S.-Privacy-IAB-Tech-Lab.pdf
USPrivacy string `json:"us_privacy,omitempty"`
}
33 changes: 33 additions & 0 deletions privacy/ccpa/ccpa.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package ccpa

import (
"encoding/json"

"github.com/buger/jsonparser"
"github.com/mxmCherry/openrtb"
)

// Policy represents the CCPA regulation for an OpenRTB bid request.
type Policy struct {
Value string
}

// Write mutates an OpenRTB bid request with the context of the CCPA policy.
func (p Policy) Write(req *openrtb.BidRequest) error {
if p.Value == "" {
return nil
}

if req.Regs == nil {
req.Regs = &openrtb.Regs{}
}

if req.Regs.Ext == nil {
req.Regs.Ext = json.RawMessage(`{"us_privacy":"` + p.Value + `"}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit strange how the CCPA regulation maps to the us_privacy symbol. Should we call it USPrivacy in all of the models or keep the CCPA terminology?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we can call it the same. Maybe someone has a better take on it but I'm fine with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the idea is that several states are enacting similar laws, and we will just filter based on the most strict version of the law as "us_privacy" signal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Kept CCPA in code for now.

return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Before we do lines 31 to 33, shouldn't we check the lenght of the string first? It occurs to me that if p.Value == "" maybe we want us_privacy to be nil. To my understanding, since the USPolicy fields is defined to a pointer to a string, we can totally pass a nil as a JSON value, can't we?

 22     if req.Regs == nil {
 23         req.Regs = &openrtb.Regs{}
 24     }
 25
    +   if p.Value == "" {
    +       req.Regs.Ext = json.RawMessage(`{"us_privacy":nil}`)
    +       return nil
    +   } else if req.Regs.Ext == nil {
 26 -   if req.Regs.Ext == nil {
 27         req.Regs.Ext = json.RawMessage(`{"us_privacy":"` + p.Value + `"}`)
 28         return nil
 29     }
 30
 31     var err error
 32     req.Regs.Ext, err = jsonparser.Set(req.Regs.Ext, []byte(`"`+p.Value+`"`), "us_privacy")
 33     return err
 34 }
privacy/ccpa/ccpa.go [RO]

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is probably to do the same as in the GDPR field and just:

 392         CCPA: ccpa.Policy{
 393 -           Enabled: ccpaOK,
     +           Enabled: ccpaOK || len(ccpaValue),
 394             Value:   ccpaValue,
 395         },
 396     }
endpoints/openrtb2/amp_auction.go 

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we even need a string pointer, as not set and empty mean the same thing for the signal?

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've been going back and forth a lot on this. If we get an empty string in the AMP params then adding an empty string in the OpenRTB request makes sense. If we receive no query param at all in the AMP params then does it still make sense to add an empty string to the OpenRTB request? Or perhaps just treat nil and empty as the same and only add non-empty values in the OpenRTB request.

In GDPR land, the signal is an int but we have it parsed as an int pointer because nil and empty (0) mean different things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

String pointer removed.

var err error
req.Regs.Ext, err = jsonparser.Set(req.Regs.Ext, []byte(`"`+p.Value+`"`), "us_privacy")
return err
}
74 changes: 74 additions & 0 deletions privacy/ccpa/ccpa_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package ccpa

import (
"encoding/json"
"testing"

"github.com/mxmCherry/openrtb"
"github.com/stretchr/testify/assert"
)

func TestWrite(t *testing.T) {
testCases := []struct {
description string
policy Policy
request *openrtb.BidRequest
expected *openrtb.BidRequest
expectedError bool
}{
{
description: "Disabled",
policy: Policy{Value: ""},
request: &openrtb.BidRequest{},
expected: &openrtb.BidRequest{},
},
{
description: "Enabled With Nil Request Regs Object",
policy: Policy{Value: "anyValue"},
request: &openrtb.BidRequest{},
expected: &openrtb.BidRequest{Regs: &openrtb.Regs{
Ext: json.RawMessage(`{"us_privacy":"anyValue"}`)}},
},
{
description: "Enabled With Nil Request Regs Ext Object",
policy: Policy{Value: "anyValue"},
request: &openrtb.BidRequest{Regs: &openrtb.Regs{}},
expected: &openrtb.BidRequest{Regs: &openrtb.Regs{
Ext: json.RawMessage(`{"us_privacy":"anyValue"}`)}},
},
{
description: "Enabled With Existing Request Regs Ext Object - Doesn't Overwrite",
policy: Policy{Value: "anyValue"},
request: &openrtb.BidRequest{Regs: &openrtb.Regs{
Ext: json.RawMessage(`{"existing":"any"}`)}},
expected: &openrtb.BidRequest{Regs: &openrtb.Regs{
Ext: json.RawMessage(`{"existing":"any","us_privacy":"anyValue"}`)}},
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been looking in the codebase for the recipient field of the "existing" value and found nothing. Was it meant to be a filler value in the json.RawMessage depicted in the tests? What exactly is its purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It represents an existing json value to test the overwriting (or lack thereof) behavior.

},
{
description: "Enabled With Existing Request Regs Ext Object - Overwrites",
policy: Policy{Value: "anyValue"},
request: &openrtb.BidRequest{Regs: &openrtb.Regs{
Ext: json.RawMessage(`{"existing":"any","us_privacy":"toBeOverwritten"}`)}},
expected: &openrtb.BidRequest{Regs: &openrtb.Regs{
Ext: json.RawMessage(`{"existing":"any","us_privacy":"anyValue"}`)}},
},
{
description: "Enabled With Existing Malformed Request Regs Ext Object",
policy: Policy{Value: "anyValue"},
request: &openrtb.BidRequest{Regs: &openrtb.Regs{
Ext: json.RawMessage(`malformed`)}},
expectedError: true,
},
}

for _, test := range testCases {
err := test.policy.Write(test.request)

if test.expectedError {
assert.Error(t, err, test.description)
} else {
assert.NoError(t, err, test.description)
assert.Equal(t, test.expected, test.request, test.description)
}
}
}
33 changes: 33 additions & 0 deletions privacy/gdpr/gdpr.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package gdpr

import (
"encoding/json"

"github.com/buger/jsonparser"
"github.com/mxmCherry/openrtb"
)

// Policy represents the GDPR regulation of an OpenRTB bid request.
type Policy struct {
Consent string
}

// Write mutates an OpenRTB bid request with the context of the GDPR policy.
func (p Policy) Write(req *openrtb.BidRequest) error {
if p.Consent == "" {
return nil
}

if req.User == nil {
req.User = &openrtb.User{}
}

if req.User.Ext == nil {
req.User.Ext = json.RawMessage(`{"consent":"` + p.Consent + `"}`)
return nil
}

var err error
req.User.Ext, err = jsonparser.Set(req.User.Ext, []byte(`"`+p.Consent+`"`), "consent")
return err
}
74 changes: 74 additions & 0 deletions privacy/gdpr/gdpr_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package gdpr

import (
"encoding/json"
"testing"

"github.com/mxmCherry/openrtb"
"github.com/stretchr/testify/assert"
)

func TestWrite(t *testing.T) {
testCases := []struct {
description string
policy Policy
request *openrtb.BidRequest
expected *openrtb.BidRequest
expectedError bool
}{
{
description: "Disabled",
policy: Policy{Consent: ""},
request: &openrtb.BidRequest{},
expected: &openrtb.BidRequest{},
},
{
description: "Enabled With Nil Request User Object",
policy: Policy{Consent: "anyValue"},
request: &openrtb.BidRequest{},
expected: &openrtb.BidRequest{User: &openrtb.User{
Ext: json.RawMessage(`{"consent":"anyValue"}`)}},
},
{
description: "Enabled With Nil Request User Ext Object",
policy: Policy{Consent: "anyValue"},
request: &openrtb.BidRequest{User: &openrtb.User{}},
expected: &openrtb.BidRequest{User: &openrtb.User{
Ext: json.RawMessage(`{"consent":"anyValue"}`)}},
},
{
description: "Enabled With Existing Request User Ext Object - Doesn't Overwrite",
policy: Policy{Consent: "anyValue"},
request: &openrtb.BidRequest{User: &openrtb.User{
Ext: json.RawMessage(`{"existing":"any"}`)}},
expected: &openrtb.BidRequest{User: &openrtb.User{
Ext: json.RawMessage(`{"existing":"any","consent":"anyValue"}`)}},
},
{
description: "Enabled With Existing Request User Ext Object - Overwrites",
policy: Policy{Consent: "anyValue"},
request: &openrtb.BidRequest{User: &openrtb.User{
Ext: json.RawMessage(`{"existing":"any","consent":"toBeOverwritten"}`)}},
expected: &openrtb.BidRequest{User: &openrtb.User{
Ext: json.RawMessage(`{"existing":"any","consent":"anyValue"}`)}},
},
{
description: "Enabled With Existing Malformed Request User Ext Object",
policy: Policy{Consent: "anyValue"},
request: &openrtb.BidRequest{User: &openrtb.User{
Ext: json.RawMessage(`malformed`)}},
expectedError: true,
},
}

for _, test := range testCases {
err := test.policy.Write(test.request)

if test.expectedError {
assert.Error(t, err, test.description)
} else {
assert.NoError(t, err, test.description)
assert.Equal(t, test.expected, test.request, test.description)
}
}
}
Loading