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

Implement EID Permissions #1633

Merged
merged 7 commits into from
Jan 14, 2021
Merged

Implement EID Permissions #1633

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b9731f
Implement EID Permissions
SyntaxNode Dec 21, 2020
8c9c600
Merge branch 'master' into eid_permissions
SyntaxNode Dec 21, 2020
45f8b09
Fix Merge Conflict
SyntaxNode Dec 21, 2020
c937b2f
Feedback
SyntaxNode Jan 12, 2021
96c9dc4
Fix Typo
SyntaxNode Jan 12, 2021
085ae36
Don't Prematurely Remove EIDs + Tweaks
SyntaxNode Jan 12, 2021
43a9fc0
Fixed Test Case
SyntaxNode Jan 14, 2021
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
49 changes: 49 additions & 0 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,10 @@ func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
if err := validateSChains(bidExt); err != nil {
return []error{err}
}

if err := deps.validateEidPermissions(bidExt, aliases); err != nil {
return []error{err}
}
}

if (req.Site == nil && req.App == nil) || (req.Site != nil && req.App != nil) {
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return nil
}
Copy link
Contributor

@guscarreon guscarreon Jan 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

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 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?


uniqueSources := make(map[string]struct{}, len(req.Prebid.Data.EidPermissions))
for i, eid := range req.Prebid.Data.EidPermissions {
if len(eid.Source) == 0 {
return fmt.Errorf(`request.ext.prebid.data.eidpermissions[%d] missing required field: "source"`, i)
}

if _, exists := uniqueSources[eid.Source]; exists {
return fmt.Errorf(`request.ext.prebid.data.eidpermissions[%d] duplicate entry with field: "source"`, i)
}
uniqueSources[eid.Source] = struct{}{}

if len(eid.Bidders) == 0 {
return fmt.Errorf(`request.ext.prebid.data.eidpermissions[%d] missing or empty required field: "bidders"`, i)
}

if err := validateBidders(eid.Bidders, deps.bidderMap, aliases); err != nil {
return fmt.Errorf(`request.ext.prebid.data.eidpermissions[%d] contains %v`, i, err)
}
}

return nil
}

func validateBidders(bidders []string, knownBidders map[string]openrtb_ext.BidderName, knownAliases map[string]string) error {
for _, bidder := range bidders {
if bidder == "*" {
if len(bidders) > 1 {
return errors.New(`bidder wildcard "*" mixed with specific bidders`)
}
} else {
_, isCoreBidder := knownBidders[bidder]
_, isAlias := knownAliases[bidder]
if !isCoreBidder && !isAlias {
return fmt.Errorf(`unrecognized bidder "%v"`, bidder)
}
}
}
return nil
}

func (deps *endpointDeps) validateImp(imp *openrtb.Imp, aliases map[string]string, index int) []error {
if imp.ID == "" {
return []error{fmt.Errorf("request.imp[%d] missing required field: \"id\"", index)}
Expand Down
Loading