Implement EID Permissions #1633

SyntaxNode · 2020-12-21T21:59:55Z

Implements: #1621

guscarreon

A couple of comments

exchange/utils.go

guscarreon · 2021-01-06T06:03:18Z

exchange/utils.go

+	}
+
+	var eids []openrtb_ext.ExtUserEid
+	if err := json.Unmarshal(eidsRaw, &eids); err != nil {


Given that extractBuyerUIDs unmarshals the User.Ext into an openrtb_ext.ExtUser struct that already reads the eids into an []ExtUserEid array, maybe we can work with this field and avoid additional unmarshalling inside removeUnpermissionedEids. Both extractBuyerUIDs and removeUnpermissionedEids use the unmarshalled User.Ext, therefore we could maybe:

174 func getAuctionBidderRequests(req AuctionRequest, requestExt *openrtb_ext.ExtRequest, impsByBidder map[string][]openrtb.I 175 176 bidderRequests := make([]BidderRequest, 0, len(impsByBidder)) 177 + userExt := unmarshalUserExt(req.BidRequest.User) + 178 - explicitBuyerUIDs, err := extractBuyerUIDs(req.BidRequest.User) + explicitBuyerUIDs, err := extractBuyerUIDs(userExt) 179 if err != nil { 180 return nil, []error{err} 181 } 182 183 *-- 11 lines: var sChainsByBidder map[string]*openrtb_ext.ExtRequestPrebidSChainSChain--- 194 195 var errs []error 196 for bidder, imps := range impsByBidder { 197 coreBidder := resolveBidder(bidder, aliases) 198 199 *-- 5 lines: reqCopy := *req.BidRequest------------------------------------------------- 204 205 if err := removeUnpermissionedEids(&reqCopy, bidder, requestExt); err != nil { + if err := removeUnpermissionedEids(&reqCopy, bidder, requestExt, userExt); err != nil { 206 errs = append(errs, fmt.Errorf("unable to enforce request.ext.prebid.data.eidpermissions because %v", err)) 207 continue 208 } exchange/utils.go

and modify removeUnpermissionedEids accordingly.

Without a doubt there's room for improvement here. It's complicated though since extractBuyerUIDs applies for the entire request and removeUnpermissionedEids applies to a request copy for the bidder. I experimented locally with what you're suggesting.. but object tracking and map cloning became complicated. We'll be addressing duplicate serialization over time in the near future, so let's just tackle it then.

guscarreon · 2021-01-06T06:25:51Z

endpoints/openrtb2/auction.go

+func (deps *endpointDeps) validateEidPermissions(req *openrtb_ext.ExtRequest, aliases map[string]string) error {
+	if req == nil || req.Prebid.Data == nil {
+		return nil
+	}


According to issue #1621: "If eidPermissions is not an array, ignore it and emit a 400 error with a reasonable description in the body". We do check the array lenght downstream in exchange/utils.go line 464:

457 func removeUnpermissionedEids(request *openrtb.BidRequest, bidder string, requestExt *openrtb_ext.ExtRequest) error { 458 // ensure request might have eids (as much as we can check before unmarshalling) 459 if request.User == nil || len(request.User.Ext) == 0 { 460 return nil 461 } 462 463 // ensure request has eid permissions to enforce 464 if requestExt == nil || requestExt.Prebid.Data == nil || len(requestExt.Prebid.Data.EidPermissions) == 0 { 465 return nil //empty array check ----^ 466 }

Should we error out when empty instead? If so, should we do it here before it gets downstream?

426 func (deps *endpointDeps) validateEidPermissions(req *openrtb_ext.ExtRequest, aliases map[string]string) error { 427 if req == nil || req.Prebid.Data == nil { 428 return nil 429 } + if len(requestExt.Prebid.Data.EidPermissions) == 0 { + return fmt.Errorf(`eidPermissions is not an array, ignore it and emit a 400 error...`) + } 430 431 uniqueSources := make(map[string]struct{}, len(req.Prebid.Data.EidPermissions)) 432 for i, eid := range req.Prebid.Data.EidPermissions { 433 if len(eid.Source) == 0 { 434 return fmt.Errorf(`request.ext.prebid.data.eidpermissions[%d] missing required field: "source"`, i) 435 } endpoints/openrtb2/auction.go

The unmarshaller enforces type correctness. If it's not an array, the core json.Unmarshal method will return an error and we already consider that a fatal error. I don't really see a need to consider an empty array as a validation error. It's silly to pass an empty array.. but is it worth rejecting a request for?

SyntaxNode · 2020-12-21T22:10:37Z

endpoints/openrtb2/auction_test.go

@@ -1440,19 +1440,19 @@ func TestCurrencyTrunc(t *testing.T) {

 	ui := uint64(1)
 	req := openrtb.BidRequest{
-		ID: "someID",
+		ID: "anyRequestID",


Unrelated: Cleaned up some of the test ids to make it clear they have no special meaning.

SyntaxNode · 2021-01-12T17:41:59Z

endpoints/openrtb2/auction.go

+func (deps *endpointDeps) validateEidPermissions(req *openrtb_ext.ExtRequest, aliases map[string]string) error {
+	if req == nil || req.Prebid.Data == nil {
+		return nil
+	}


The unmarshaller enforces type correctness. If it's not an array, the core json.Unmarshal method will return an error and we already consider that a fatal error. I don't really see a need to consider an empty array as a validation error. It's silly to pass an empty array.. but is it worth rejecting a request for?

SyntaxNode · 2021-01-12T17:46:30Z

exchange/exchange_test.go

-					expectations:  map[string]*bidderRequest{coreBidder: spec.ExpectedRequest},
-					mockResponses: map[string]bidderResponse{coreBidder: spec.MockResponse},
+					expectations:  map[string]*bidderRequest{alias: spec.ExpectedRequest},
+					mockResponses: map[string]bidderResponse{alias: spec.MockResponse},


Fixes an unrelated test framework bug. We never met the condition before, so it's gone unnoticed.

SyntaxNode · 2021-01-12T17:49:20Z

exchange/utils.go

 		prepareSource(&reqCopy, bidder, sChainsByBidder)

-		bidder := BidderRequest{
+		if err := removeUnpermissionedEids(&reqCopy, bidder, requestExt); err != nil {
+			errs = append(errs, fmt.Errorf("unable to enforce request.ext.prebid.data.eidpermissions because %v", err))


The errors are just from marshalling, which shouldn't occur here. Is a continue the correct behavior?

SyntaxNode · 2021-01-12T20:24:40Z

exchange/utils.go

+	}
+
+	var eids []openrtb_ext.ExtUserEid
+	if err := json.Unmarshal(eidsRaw, &eids); err != nil {


Without a doubt there's room for improvement here. It's complicated though since extractBuyerUIDs applies for the entire request and removeUnpermissionedEids applies to a request copy for the bidder. I experimented locally with what you're suggesting.. but object tracking and map cloning became complicated. We'll be addressing duplicate serialization over time in the near future, so let's just tackle it then.

hhhjort · 2021-01-13T16:33:27Z

exchange/utils_test.go

+			expectedUserExt: json.RawMessage(`{"eids":[{"source":"source1","id":"anyID"}]}`),
+		},
+		{
+			description:     "Allowed By Empty Permissions",


What is the difference between nil permissions and empty permissions?

As of now, nothing. Good catch. This was a copy/paste error. Fixed.

hhhjort

LGTM

guscarreon

LGTM

guscarreon · 2021-01-14T17:21:08Z

endpoints/openrtb2/auction.go

@@ -419,6 +423,51 @@ func validateSChains(req *openrtb_ext.ExtRequest) error {
 	return err
 }

+func (deps *endpointDeps) validateEidPermissions(req *openrtb_ext.ExtRequest, aliases map[string]string) error {
+	if req == nil || req.Prebid.Data == nil {


The first condition will never be false because validateEidPermissions only gets called if condition in line 320 of endpoints/openrtb2/auction.go evaluates to true

320 } else if bidExt != nil { . . . 335 if err := deps.validateEidPermissions(bidExt, aliases); err != nil { 336 return []error{err} 337 } 338 } endpoints/openrtb2/auction.go

guscarreon · 2021-01-14T17:21:29Z

endpoints/openrtb2/auction_test.go

+
+	testCases := []struct {
+		description   string
+		request       *openrtb_ext.ExtRequest


Just to not get confused with an incoming bid request, can we change the name to requestExt? Or bidRequestExt?

guscarreon · 2021-01-14T17:26:12Z

endpoints/openrtb2/auction_test.go

+				{Source: "sourceB", Bidders: []string{"z"}},
+			}}}},
+			expectedError: errors.New(`request.ext.prebid.data.eidpermissions[1] contains unrecognized bidder "z"`),
+		},


Can we add the test: "Valid, more than one element in the Bidders array"?

SyntaxNode added 3 commits December 21, 2020 16:59

Implement EID Permissions

6b9731f

Merge branch 'master' into eid_permissions

8c9c600

Fix Merge Conflict

45f8b09

guscarreon reviewed Jan 6, 2021

View reviewed changes

SyntaxNode assigned guscarreon Jan 11, 2021

SyntaxNode requested a review from hhhjort January 11, 2021 15:30

SyntaxNode assigned hhhjort Jan 11, 2021

SyntaxNode commented Jan 11, 2021

View reviewed changes

SyntaxNode added 3 commits January 12, 2021 12:55

Feedback

c937b2f

Fix Typo

96c9dc4

Don't Prematurely Remove EIDs + Tweaks

085ae36

SyntaxNode commented Jan 13, 2021

View reviewed changes

hhhjort reviewed Jan 13, 2021

View reviewed changes

Fixed Test Case

43a9fc0

hhhjort approved these changes Jan 14, 2021

View reviewed changes

guscarreon approved these changes Jan 14, 2021

View reviewed changes

guscarreon merged commit 9c2e630 into prebid:master Jan 14, 2021

SyntaxNode deleted the eid_permissions branch January 23, 2021 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EID Permissions #1633

Implement EID Permissions #1633

SyntaxNode commented Dec 21, 2020

guscarreon left a comment

guscarreon Jan 6, 2021

SyntaxNode Jan 12, 2021

guscarreon Jan 6, 2021 •

edited

Loading

SyntaxNode Jan 12, 2021

SyntaxNode Dec 21, 2020

SyntaxNode Jan 12, 2021

SyntaxNode Jan 12, 2021

SyntaxNode Jan 12, 2021

SyntaxNode Jan 12, 2021

hhhjort Jan 13, 2021

SyntaxNode Jan 14, 2021

hhhjort left a comment

guscarreon left a comment

guscarreon Jan 14, 2021

guscarreon Jan 14, 2021

guscarreon Jan 14, 2021

Implement EID Permissions #1633

Implement EID Permissions #1633

Conversation

SyntaxNode commented Dec 21, 2020

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon Jan 6, 2021 •

edited

Loading