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

Request validation upgrade to 2.6 locations #3906

Merged
merged 6 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
59 changes: 11 additions & 48 deletions endpoints/openrtb2/auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/prebid/prebid-server/v2/ortb"
"github.com/prebid/prebid-server/v2/privacy"
"github.com/prebid/prebid-server/v2/privacysandbox"
"github.com/prebid/prebid-server/v2/schain"
"golang.org/x/net/publicsuffix"
jsonpatch "gopkg.in/evanphx/json-patch.v4"

Expand All @@ -43,7 +44,6 @@ import (
"github.com/prebid/prebid-server/v2/prebid_cache_client"
"github.com/prebid/prebid-server/v2/privacy/ccpa"
"github.com/prebid/prebid-server/v2/privacy/lmt"
"github.com/prebid/prebid-server/v2/schain"
"github.com/prebid/prebid-server/v2/stored_requests"
"github.com/prebid/prebid-server/v2/stored_requests/backends/empty_fetcher"
"github.com/prebid/prebid-server/v2/stored_responses"
Expand Down Expand Up @@ -823,10 +823,6 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
}
}

if err := mapSChains(req); err != nil {
return []error{err}
}

if err := validateOrFillChannel(req, isAmp); err != nil {
return []error{err}
}
Expand Down Expand Up @@ -930,32 +926,6 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
return errL
}

// mapSChains maps an schain defined in an ORTB 2.4 location (req.ext.schain) to the ORTB 2.5 location
// (req.source.ext.schain) if no ORTB 2.5 schain (req.source.ext.schain, req.ext.prebid.schains) exists.
Comment on lines -933 to -934
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we need to update the upconvert logic for schain to only perform an upgrade from 2.4 to 2.5 if req.ext.prebid.schains does not exist. req.ext.prebid.schains is a 2.5 location so it should take precedence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed offline. I'll take a look to see if this is a problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The order of precedence from highest to lowest is as follows:
req.ext.prebid.schains
req.source.schain (2.6)
req.source.ext.schain (2.5)
req.ext.schain (2.4)

We're upconverting right after we parse the request which will handle the ORTB standard locations resulting in a value in the 2.6 location if an schain exists in the 2.4, 2.5 or 2.6 location. Downstream we will use the schain writer to potentially overwrite the 2.6 location with a value from the req.ext.prebid.schains.

No additional change is needed to the upconvert logic.

// An ORTB 2.4 schain is always deleted from the 2.4 location regardless of whether an ORTB 2.5 schain exists.
func mapSChains(req *openrtb_ext.RequestWrapper) error {
reqExt, err := req.GetRequestExt()
if err != nil {
return fmt.Errorf("req.ext is invalid: %v", err)
}
sourceExt, err := req.GetSourceExt()
if err != nil {
return fmt.Errorf("source.ext is invalid: %v", err)
}

reqExtSChain := reqExt.GetSChain()
reqExt.SetSChain(nil)

if reqPrebid := reqExt.GetPrebid(); reqPrebid != nil && reqPrebid.SChains != nil {
return nil
} else if sourceExt.GetSChain() != nil {
return nil
} else if reqExtSChain != nil {
sourceExt.SetSChain(reqExtSChain)
}
return nil
}

func validateAndFillSourceTID(req *openrtb_ext.RequestWrapper, generateRequestID bool, hasStoredBidRequest bool, isAmp bool) error {
if req.Source == nil {
req.Source = &openrtb2.Source{}
Expand Down Expand Up @@ -1294,21 +1264,19 @@ func (deps *endpointDeps) validateUser(req *openrtb_ext.RequestWrapper, aliases
}

// Check Universal User ID
eids := userExt.GetEid()
if eids != nil {
eidsValue := *eids
for eidIndex, eid := range eidsValue {
if req.User.EIDs != nil {
for eidIndex, eid := range req.User.EIDs {
if eid.Source == "" {
return append(errL, fmt.Errorf("request.user.ext.eids[%d] missing required field: \"source\"", eidIndex))
return append(errL, fmt.Errorf("request.user.eids[%d] missing required field: \"source\"", eidIndex))
}

if len(eid.UIDs) == 0 {
return append(errL, fmt.Errorf("request.user.ext.eids[%d].uids must contain at least one element or be undefined", eidIndex))
return append(errL, fmt.Errorf("request.user.eids[%d].uids must contain at least one element or be undefined", eidIndex))
}

for uidIndex, uid := range eid.UIDs {
if uid.ID == "" {
return append(errL, fmt.Errorf("request.user.ext.eids[%d].uids[%d] missing required field: \"id\"", eidIndex, uidIndex))
return append(errL, fmt.Errorf("request.user.eids[%d].uids[%d] missing required field: \"id\"", eidIndex, uidIndex))
}
}
}
Expand Down Expand Up @@ -1338,16 +1306,12 @@ func validateRegs(req *openrtb_ext.RequestWrapper, gpp gpplib.GppContainer) []er
WarningCode: errortypes.InvalidPrivacyConsentWarningCode})
}
}
regsExt, err := req.GetRegExt()
if err != nil {
return append(errL, fmt.Errorf("request.regs.ext is invalid: %v", err))
}

gdpr := regsExt.GetGDPR()
if gdpr != nil && *gdpr != 0 && *gdpr != 1 {
return append(errL, errors.New("request.regs.ext.gdpr must be either 0 or 1"))
if req.BidRequest.Regs.GDPR != nil {
reqGDPR := req.BidRequest.Regs.GDPR
if reqGDPR != nil && *reqGDPR != 0 && *reqGDPR != 1 {
return append(errL, errors.New("request.regs.gdpr must be either 0 or 1"))
}
}

return errL
}

Expand All @@ -1370,7 +1334,6 @@ func validateDevice(device *openrtb2.Device) error {
if device.Geo != nil && device.Geo.Accuracy < 0 {
return errors.New("request.device.geo.accuracy must be a positive number")
}

return nil
}

Expand Down
128 changes: 0 additions & 128 deletions endpoints/openrtb2/auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2938,134 +2938,6 @@ func TestSChainInvalid(t *testing.T) {
assert.ElementsMatch(t, errL, []error{expectedError})
}

func TestMapSChains(t *testing.T) {
const seller1SChain string = `"schain":{"complete":1,"nodes":[{"asi":"directseller1.com","sid":"00001","rid":"BidRequest1","hp":1}],"ver":"1.0"}`
const seller2SChain string = `"schain":{"complete":2,"nodes":[{"asi":"directseller2.com","sid":"00002","rid":"BidRequest2","hp":2}],"ver":"2.0"}`

seller1SChainUnpacked := openrtb2.SupplyChain{
Complete: 1,
Nodes: []openrtb2.SupplyChainNode{{
ASI: "directseller1.com",
SID: "00001",
RID: "BidRequest1",
HP: openrtb2.Int8Ptr(1),
}},
Ver: "1.0",
}

tests := []struct {
description string
bidRequest openrtb2.BidRequest
wantReqExtSChain *openrtb2.SupplyChain
wantSourceExtSChain *openrtb2.SupplyChain
wantError bool
}{
{
description: "invalid req.ext",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{"prebid":{"schains":invalid}}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{}`),
},
},
wantError: true,
},
{
description: "invalid source.ext",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{"schain":invalid}}`),
},
},
wantError: true,
},
{
description: "req.ext.prebid.schains, req.source.ext.schain and req.ext.schain are nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: nil,
},
{
description: "req.ext.prebid.schains is not nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{"prebid":{"schains":[{"bidders":["appnexus"],` + seller1SChain + `}]}}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: nil,
},
{
description: "req.source.ext is not nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{` + seller1SChain + `}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: &seller1SChainUnpacked,
},
{
description: "req.ext.schain is not nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{` + seller1SChain + `}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: &seller1SChainUnpacked,
},
{
description: "req.source.ext.schain and req.ext.schain are not nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{` + seller2SChain + `}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{` + seller1SChain + `}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: &seller1SChainUnpacked,
},
}

for _, test := range tests {
reqWrapper := openrtb_ext.RequestWrapper{
BidRequest: &test.bidRequest,
}

err := mapSChains(&reqWrapper)

if test.wantError {
assert.NotNil(t, err, test.description)
} else {
assert.Nil(t, err, test.description)

reqExt, err := reqWrapper.GetRequestExt()
if err != nil {
assert.Fail(t, "Error getting request ext from wrapper", test.description)
}
reqExtSChain := reqExt.GetSChain()
assert.Equal(t, test.wantReqExtSChain, reqExtSChain, test.description)

sourceExt, err := reqWrapper.GetSourceExt()
if err != nil {
assert.Fail(t, "Error getting source ext from wrapper", test.description)
}
sourceExtSChain := sourceExt.GetSChain()
assert.Equal(t, test.wantSourceExtSChain, sourceExtSChain, test.description)
}
}
}

func TestSearchAccountID(t *testing.T) {
// Correctness for lookup within Publisher object left to TestGetAccountID
// This however tests the expected lookup paths in outer site, app and dooh
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,5 @@
}
},
"expectedReturnCode": 400,
"expectedErrorMessage": "Invalid request: request.regs.ext is invalid: expect { or n, but found "
"expectedErrorMessage": "Invalid request: error reading request.regs.ext: expect { or n, but found "
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,12 @@
}
],
"regs": {
"ext": {
"gdpr": 2
}
"gdpr": 2
},
"user": {
"ext": {}
}
},
"expectedReturnCode": 400,
"expectedErrorMessage": "Invalid request: request.regs.ext.gdpr must be either 0 or 1\n"
"expectedErrorMessage": "Invalid request: request.regs.gdpr must be either 0 or 1\n"
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"description": "Bid request where a request.user.ext.eids.uids array element is missing its id field",
"description": "Bid request with user.eids array element that does not contain source field",
"mockBidRequest": {
"id": "anyRequestID",
"site": {
Expand Down Expand Up @@ -29,14 +29,13 @@
}],
"tmax": 1000,
"user": {
"ext": {
"eids": [{
"source": "source1",
"uids": [{}]
"eids": [{
"uids": [{
"id": "A"
}]
}
}]
}
},
"expectedReturnCode": 400,
"expectedErrorMessage": "Invalid request: request.user.ext.eids[0].uids[0] missing required field: \"id\"\n"
"expectedErrorMessage": "Invalid request: request.user.eids[0] missing required field: \"source\"\n"
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"description": "Bid request with user.ext.eids array element array element that does not contain source field",
"description": "Bid request where a request.user.eids.uids array element is missing its id field",
"mockBidRequest": {
"id": "anyRequestID",
"site": {
Expand Down Expand Up @@ -29,15 +29,12 @@
}],
"tmax": 1000,
"user": {
"ext": {
"eids": [{
"uids": [{
"id": "A"
}]
}]
}
"eids": [{
"source": "source1",
"uids": [{}]
}]
}
},
"expectedReturnCode": 400,
"expectedErrorMessage": "Invalid request: request.user.ext.eids[0] missing required field: \"source\"\n"
"expectedErrorMessage": "Invalid request: request.user.eids[0].uids[0] missing required field: \"id\"\n"
}
Loading