From ff8bee1ad6520ac52a7d01d66274385701606325 Mon Sep 17 00:00:00 2001 From: dbemiller <27972385+dbemiller@users.noreply.github.com> Date: Fri, 14 Dec 2018 15:41:40 -0500 Subject: [PATCH] Moved bid validation into some adapter middleware. (#772) * Moved bidd validation into some adapter middleware. * Ran gofmt -s to fix the build. --- exchange/adapter_map.go | 8 + exchange/bidder_validate_bids.go | 119 ++++++++++++ exchange/bidder_validate_bids_test.go | 260 ++++++++++++++++++++++++++ exchange/exchange.go | 96 ---------- exchange/validation_test.go | 258 ------------------------- 5 files changed, 387 insertions(+), 354 deletions(-) create mode 100644 exchange/bidder_validate_bids.go create mode 100644 exchange/bidder_validate_bids_test.go delete mode 100644 exchange/validation_test.go diff --git a/exchange/adapter_map.go b/exchange/adapter_map.go index 245de169948..88074506e5c 100644 --- a/exchange/adapter_map.go +++ b/exchange/adapter_map.go @@ -75,6 +75,8 @@ func newAdapterMap(client *http.Client, cfg *config.Configuration, infos adapter allBidders := make(map[openrtb_ext.BidderName]adaptedBidder, len(ortbBidders)+len(legacyBidders)) + // Wrap legacy and openrtb Bidders behind a common interface, so that the Exchange doesn't need to concern + // itself with the differences. for name, bidder := range legacyBidders { // Clean out any disabled bidders if isEnabledBidder(cfg.Adapters, string(name)) { @@ -87,6 +89,12 @@ func newAdapterMap(client *http.Client, cfg *config.Configuration, infos adapter allBidders[name] = adaptBidder(adapters.EnforceBidderInfo(bidder, infos[string(name)]), client) } } + + // Apply any middleware used for global Bidder logic. + for name, bidder := range allBidders { + allBidders[name] = ensureValidBids(bidder) + } + return allBidders } diff --git a/exchange/bidder_validate_bids.go b/exchange/bidder_validate_bids.go new file mode 100644 index 00000000000..d7f4b07f05e --- /dev/null +++ b/exchange/bidder_validate_bids.go @@ -0,0 +1,119 @@ +package exchange + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/mxmCherry/openrtb" + "github.com/prebid/prebid-server/openrtb_ext" + "golang.org/x/text/currency" +) + +// ensureValidBids returns a bidder that removes invalid bids from the argument bidder's response. +// These will be converted into errors instead. +// +// The goal here is to make sure that the response contains Bids which are valid given the initial Request, +// so that Publishers can trust the Bids they get from Prebid Server. +func ensureValidBids(bidder adaptedBidder) adaptedBidder { + return &validatedBidder{ + bidder: bidder, + } +} + +type validatedBidder struct { + bidder adaptedBidder +} + +func (v *validatedBidder) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64) (*pbsOrtbSeatBid, []error) { + seatBid, errs := v.bidder.requestBid(ctx, request, name, bidAdjustment) + if validationErrors := removeInvalidBids(request, seatBid); len(validationErrors) > 0 { + errs = append(errs, validationErrors...) + } + return seatBid, errs +} + +// validateBids will run some validation checks on the returned bids and excise any invalid bids +func removeInvalidBids(request *openrtb.BidRequest, seatBid *pbsOrtbSeatBid) []error { + // Exit early if there is nothing to do. + if seatBid == nil || len(seatBid.bids) == 0 { + return nil + } + + // By design, default currency is USD. + if cerr := validateCurrency(request.Cur, seatBid.currency); cerr != nil { + seatBid.bids = nil + return []error{cerr} + } + + errs := make([]error, 0, len(seatBid.bids)) + validBids := make([]*pbsOrtbBid, 0, len(seatBid.bids)) + for _, bid := range seatBid.bids { + if ok, berr := validateBid(bid); ok { + validBids = append(validBids, bid) + } else { + errs = append(errs, berr) + } + } + seatBid.bids = validBids + return errs +} + +// validateCurrency will run currency validation checks and return true if it passes, false otherwise. +func validateCurrency(requestAllowedCurrencies []string, bidCurrency string) error { + // Default currency is `USD` by design. + defaultCurrency := "USD" + // Make sure bid currency is a valid ISO currency code + if bidCurrency == "" { + // If bid currency is not set, then consider it's default currency. + bidCurrency = defaultCurrency + } + currencyUnit, cerr := currency.ParseISO(bidCurrency) + if cerr != nil { + return cerr + } + // Make sure the bid currency is allowed from bid request via `cur` field. + // If `cur` field array from bid request is empty, then consider it accepts the default currency. + currencyAllowed := false + if len(requestAllowedCurrencies) == 0 { + requestAllowedCurrencies = []string{defaultCurrency} + } + for _, allowedCurrency := range requestAllowedCurrencies { + if strings.ToUpper(allowedCurrency) == currencyUnit.String() { + currencyAllowed = true + break + } + } + if currencyAllowed == false { + return fmt.Errorf( + "Bid currency is not allowed. Was '%s', wants: ['%s']", + currencyUnit.String(), + strings.Join(requestAllowedCurrencies, "', '"), + ) + } + + return nil +} + +// validateBid will run the supplied bid through validation checks and return true if it passes, false otherwise. +func validateBid(bid *pbsOrtbBid) (bool, error) { + if bid.bid == nil { + return false, errors.New("Empty bid object submitted.") + } + + if bid.bid.ID == "" { + return false, errors.New("Bid missing required field 'id'") + } + if bid.bid.ImpID == "" { + return false, fmt.Errorf("Bid \"%s\" missing required field 'impid'", bid.bid.ID) + } + if bid.bid.Price <= 0.0 { + return false, fmt.Errorf("Bid \"%s\" does not contain a positive 'price'", bid.bid.ID) + } + if bid.bid.CrID == "" { + return false, fmt.Errorf("Bid \"%s\" missing creative ID", bid.bid.ID) + } + + return true, nil +} diff --git a/exchange/bidder_validate_bids_test.go b/exchange/bidder_validate_bids_test.go new file mode 100644 index 00000000000..1241616e385 --- /dev/null +++ b/exchange/bidder_validate_bids_test.go @@ -0,0 +1,260 @@ +package exchange + +import ( + "context" + "testing" + + "github.com/mxmCherry/openrtb" + "github.com/prebid/prebid-server/openrtb_ext" + "github.com/stretchr/testify/assert" +) + +func TestAllValidBids(t *testing.T) { + var bidder adaptedBidder = ensureValidBids(&mockAdaptedBidder{ + bidResponse: &pbsOrtbSeatBid{ + bids: []*pbsOrtbBid{ + { + bid: &openrtb.Bid{ + ID: "one-bid", + ImpID: "thisImp", + Price: 0.45, + CrID: "thisCreative", + }, + }, + { + bid: &openrtb.Bid{ + ID: "thatBid", + ImpID: "thatImp", + Price: 0.40, + CrID: "thatCreative", + }, + }, + { + bid: &openrtb.Bid{ + ID: "123", + ImpID: "456", + Price: 0.44, + CrID: "789", + }, + }, + }, + }, + }) + seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0) + assert.Len(t, seatBid.bids, 3) + assert.Len(t, errs, 0) +} + +func TestAllBadBids(t *testing.T) { + bidder := ensureValidBids(&mockAdaptedBidder{ + bidResponse: &pbsOrtbSeatBid{ + bids: []*pbsOrtbBid{ + { + bid: &openrtb.Bid{ + ID: "one-bid", + Price: 0.45, + CrID: "thisCreative", + }, + }, + { + bid: &openrtb.Bid{ + ID: "thatBid", + ImpID: "thatImp", + CrID: "thatCreative", + }, + }, + { + bid: &openrtb.Bid{ + ID: "123", + ImpID: "456", + Price: 0.44, + }, + }, + { + bid: &openrtb.Bid{ + ImpID: "456", + Price: 0.44, + CrID: "blah", + }, + }, + {}, + }, + }, + }) + seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0) + assert.Len(t, seatBid.bids, 0) + assert.Len(t, errs, 5) +} + +func TestMixedBids(t *testing.T) { + bidder := ensureValidBids(&mockAdaptedBidder{ + bidResponse: &pbsOrtbSeatBid{ + bids: []*pbsOrtbBid{ + { + bid: &openrtb.Bid{ + ID: "one-bid", + ImpID: "thisImp", + Price: 0.45, + CrID: "thisCreative", + }, + }, + { + bid: &openrtb.Bid{ + ID: "thatBid", + ImpID: "thatImp", + CrID: "thatCreative", + }, + }, + { + bid: &openrtb.Bid{ + ID: "123", + ImpID: "456", + Price: 0.44, + CrID: "789", + }, + }, + { + bid: &openrtb.Bid{ + ImpID: "456", + Price: 0.44, + CrID: "blah", + }, + }, + {}, + }, + }, + }) + seatBid, errs := bidder.requestBid(context.Background(), &openrtb.BidRequest{}, openrtb_ext.BidderAppnexus, 1.0) + assert.Len(t, seatBid.bids, 2) + assert.Len(t, errs, 3) +} + +func TestCurrencyBids(t *testing.T) { + currencyTestCases := []struct { + brqCur []string + brpCur string + defaultCur string + expectedValidBid bool + }{ + // Case bid request and bid response don't specify any currencies. + // Expected to be valid since both bid request / response will be overridden with default currency (USD). + { + brqCur: []string{}, + brpCur: "", + expectedValidBid: true, + }, + // Case bid request specifies a currency (default one) but bid response doesn't. + // Expected to be valid since bid response will be overridden with default currency (USD). + { + brqCur: []string{"USD"}, + brpCur: "", + expectedValidBid: true, + }, + // Case bid request specifies more than 1 currency (default one and another one) but bid response doesn't. + // Expected to be valid since bid response will be overridden with default currency (USD). + { + brqCur: []string{"USD", "EUR"}, + brpCur: "", + expectedValidBid: true, + }, + // Case bid request specifies more than 1 currency (default one and another one) and bid response specifies default currency (USD). + // Expected to be valid. + { + brqCur: []string{"USD", "EUR"}, + brpCur: "USD", + expectedValidBid: true, + }, + // Case bid request specifies more than 1 currency (default one and another one) and bid response specifies the second currency allowed (not USD). + // Expected to be valid. + { + brqCur: []string{"USD", "EUR"}, + brpCur: "EUR", + expectedValidBid: true, + }, + // Case bid request specifies only 1 currency which is not the default one. + // Bid response doesn't specify any currency. + // Expected to be invalid. + { + brqCur: []string{"JPY"}, + brpCur: "", + expectedValidBid: false, + }, + // Case bid request doesn't specify any currencies. + // Bid response specifies a currency which is not the default one. + // Expected to be invalid. + { + brqCur: []string{}, + brpCur: "JPY", + expectedValidBid: false, + }, + // Case bid request specifies a currency. + // Bid response specifies a currency which is not the one specified in bid request. + // Expected to be invalid. + { + brqCur: []string{"USD"}, + brpCur: "EUR", + expectedValidBid: false, + }, + // Case bid request specifies several currencies. + // Bid response specifies a currency which is not the one specified in bid request. + // Expected to be invalid. + { + brqCur: []string{"USD", "EUR"}, + brpCur: "JPY", + expectedValidBid: false, + }, + } + + for _, tc := range currencyTestCases { + bids := []*pbsOrtbBid{ + { + bid: &openrtb.Bid{ + ID: "one-bid", + ImpID: "thisImp", + Price: 0.45, + CrID: "thisCreative", + }, + }, + { + bid: &openrtb.Bid{ + ID: "thatBid", + ImpID: "thatImp", + Price: 0.44, + CrID: "thatCreative", + }, + }, + } + bidder := ensureValidBids(&mockAdaptedBidder{ + bidResponse: &pbsOrtbSeatBid{ + currency: tc.brpCur, + bids: bids, + }, + }) + + expectedValidBids := len(bids) + expectedErrs := 0 + + if tc.expectedValidBid != true { + // If currency mistmatch, we should have one error + expectedErrs = 1 + expectedValidBids = 0 + } + + request := &openrtb.BidRequest{ + Cur: tc.brqCur, + } + + seatBid, errs := bidder.requestBid(context.Background(), request, openrtb_ext.BidderAppnexus, 1.0) + assert.Len(t, seatBid.bids, expectedValidBids) + assert.Len(t, errs, expectedErrs) + } +} + +type mockAdaptedBidder struct { + bidResponse *pbsOrtbSeatBid + errorResponse []error +} + +func (b *mockAdaptedBidder) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64) (*pbsOrtbSeatBid, []error) { + return b.bidResponse, b.errorResponse +} diff --git a/exchange/exchange.go b/exchange/exchange.go index f799791bb71..f3304a5cf2d 100644 --- a/exchange/exchange.go +++ b/exchange/exchange.go @@ -6,7 +6,6 @@ import ( "fmt" "net/http" "runtime/debug" - "strings" "time" "github.com/golang/glog" @@ -18,7 +17,6 @@ import ( "github.com/prebid/prebid-server/openrtb_ext" "github.com/prebid/prebid-server/pbsmetrics" "github.com/prebid/prebid-server/prebid_cache_client" - "golang.org/x/text/currency" ) // Exchange runs Auctions. Implementations must be threadsafe, and will be shared across many goroutines. @@ -187,11 +185,6 @@ func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext // Add in time reporting elapsed := time.Since(start) brw.adapterBids = bids - // validate bids ASAP, so we don't waste time on invalid bids. - err2 := brw.validateBids(request) - if len(err2) > 0 { - err = append(err, err2...) - } // Structure to record extra tracking data generated during bidding ae := new(seatResponseExtra) ae.ResponseTimeMillis = int(elapsed / time.Millisecond) @@ -399,92 +392,3 @@ func (e *exchange) makeBid(Bids []*pbsOrtbBid, adapter openrtb_ext.BidderName) ( } return bids, errList } - -// validateBids will run some validation checks on the returned bids and excise any invalid bids -func (brw *bidResponseWrapper) validateBids(request *openrtb.BidRequest) (err []error) { - // Exit early if there is nothing to do. - if brw.adapterBids == nil || len(brw.adapterBids.bids) == 0 { - return - } - - err = make([]error, 0, len(brw.adapterBids.bids)) - - // By design, default currency is USD. - if cerr := validateCurrency(request.Cur, brw.adapterBids.currency); cerr != nil { - brw.adapterBids.bids = nil - err = append(err, cerr) - return - } - - validBids := make([]*pbsOrtbBid, 0, len(brw.adapterBids.bids)) - for _, bid := range brw.adapterBids.bids { - if ok, berr := validateBid(bid); ok { - validBids = append(validBids, bid) - } else { - err = append(err, berr) - } - } - if len(validBids) != len(brw.adapterBids.bids) { - // If all bids are valid, the two slices should be equal. Otherwise replace the list of bids with the valid bids. - brw.adapterBids.bids = validBids - } - return err -} - -// validateCurrency will run currency validation checks and return true if it passes, false otherwise. -func validateCurrency(requestAllowedCurrencies []string, bidCurrency string) error { - // Default currency is `USD` by design. - defaultCurrency := "USD" - // Make sure bid currency is a valid ISO currency code - if bidCurrency == "" { - // If bid currency is not set, then consider it's default currency. - bidCurrency = defaultCurrency - } - currencyUnit, cerr := currency.ParseISO(bidCurrency) - if cerr != nil { - return cerr - } - // Make sure the bid currency is allowed from bid request via `cur` field. - // If `cur` field array from bid request is empty, then consider it accepts the default currency. - currencyAllowed := false - if len(requestAllowedCurrencies) == 0 { - requestAllowedCurrencies = []string{defaultCurrency} - } - for _, allowedCurrency := range requestAllowedCurrencies { - if strings.ToUpper(allowedCurrency) == currencyUnit.String() { - currencyAllowed = true - break - } - } - if currencyAllowed == false { - return fmt.Errorf( - "Bid currency is not allowed. Was '%s', wants: ['%s']", - currencyUnit.String(), - strings.Join(requestAllowedCurrencies, "', '"), - ) - } - - return nil -} - -// validateBid will run the supplied bid through validation checks and return true if it passes, false otherwise. -func validateBid(bid *pbsOrtbBid) (bool, error) { - if bid.bid == nil { - return false, fmt.Errorf("Empty bid object submitted.") - } - // These are the three required fields for bids - if bid.bid.ID == "" { - return false, fmt.Errorf("Bid missing required field 'id'") - } - if bid.bid.ImpID == "" { - return false, fmt.Errorf("Bid \"%s\" missing required field 'impid'", bid.bid.ID) - } - if bid.bid.Price <= 0.0 { - return false, fmt.Errorf("Bid \"%s\" does not contain a positive 'price'", bid.bid.ID) - } - if bid.bid.CrID == "" { - return false, fmt.Errorf("Bid \"%s\" missing creative ID", bid.bid.ID) - } - - return true, nil -} diff --git a/exchange/validation_test.go b/exchange/validation_test.go deleted file mode 100644 index 70915931396..00000000000 --- a/exchange/validation_test.go +++ /dev/null @@ -1,258 +0,0 @@ -package exchange - -import ( - "testing" - - "github.com/mxmCherry/openrtb" -) - -func TestAllValidBids(t *testing.T) { - brq := &openrtb.BidRequest{} - - bids := make([]*pbsOrtbBid, 3) - - bids[0] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "one-bid", - ImpID: "thisImp", - Price: 0.45, - CrID: "thisCreative", - }, - } - bids[1] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "thatBid", - ImpID: "thatImp", - Price: 0.40, - CrID: "thatCreative", - }, - } - bids[2] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "123", - ImpID: "456", - Price: 0.44, - CrID: "789", - }, - } - brw := &bidResponseWrapper{ - adapterBids: &pbsOrtbSeatBid{ - bids: bids, - }, - } - assertBids(t, brq, brw, 3, 0) -} - -func TestAllBadBids(t *testing.T) { - brq := &openrtb.BidRequest{} - bids := make([]*pbsOrtbBid, 5) - - bids[0] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "one-bid", - Price: 0.45, - CrID: "thisCreative", - }, - } - bids[1] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "thatBid", - ImpID: "thatImp", - CrID: "thatCreative", - }, - } - bids[2] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "123", - ImpID: "456", - Price: 0.44, - }, - } - bids[3] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ImpID: "456", - Price: 0.44, - CrID: "blah", - }, - } - bids[4] = &pbsOrtbBid{} - brw := &bidResponseWrapper{ - adapterBids: &pbsOrtbSeatBid{ - bids: bids, - }, - } - assertBids(t, brq, brw, 0, 5) -} - -func TestMixeddBids(t *testing.T) { - brq := &openrtb.BidRequest{} - - bids := make([]*pbsOrtbBid, 5) - bids[0] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "one-bid", - ImpID: "thisImp", - Price: 0.45, - CrID: "thisCreative", - }, - } - bids[1] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "thatBid", - ImpID: "thatImp", - CrID: "thatCreative", - }, - } - bids[2] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "123", - ImpID: "456", - Price: 0.44, - CrID: "789", - }, - } - bids[3] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ImpID: "456", - Price: 0.44, - CrID: "blah", - }, - } - bids[4] = &pbsOrtbBid{} - brw := &bidResponseWrapper{ - adapterBids: &pbsOrtbSeatBid{ - bids: bids, - }, - } - assertBids(t, brq, brw, 2, 3) -} - -func TestCurrencyBids(t *testing.T) { - currencyTestCases := []struct { - brqCur []string - brpCur string - defaultCur string - expectedValidBid bool - }{ - // Case bid request and bid response don't specify any currencies. - // Expected to be valid since both bid request / response will be overridden with default currency (USD). - { - brqCur: []string{}, - brpCur: "", - expectedValidBid: true, - }, - // Case bid request specifies a currency (default one) but bid response doesn't. - // Expected to be valid since bid response will be overridden with default currency (USD). - { - brqCur: []string{"USD"}, - brpCur: "", - expectedValidBid: true, - }, - // Case bid request specifies more than 1 currency (default one and another one) but bid response doesn't. - // Expected to be valid since bid response will be overridden with default currency (USD). - { - brqCur: []string{"USD", "EUR"}, - brpCur: "", - expectedValidBid: true, - }, - // Case bid request specifies more than 1 currency (default one and another one) and bid response specifies default currency (USD). - // Expected to be valid. - { - brqCur: []string{"USD", "EUR"}, - brpCur: "USD", - expectedValidBid: true, - }, - // Case bid request specifies more than 1 currency (default one and another one) and bid response specifies the second currency allowed (not USD). - // Expected to be valid. - { - brqCur: []string{"USD", "EUR"}, - brpCur: "EUR", - expectedValidBid: true, - }, - // Case bid request specifies only 1 currency which is not the default one. - // Bid response doesn't specify any currency. - // Expected to be invalid. - { - brqCur: []string{"JPY"}, - brpCur: "", - expectedValidBid: false, - }, - // Case bid request doesn't specify any currencies. - // Bid response specifies a currency which is not the default one. - // Expected to be invalid. - { - brqCur: []string{}, - brpCur: "JPY", - expectedValidBid: false, - }, - // Case bid request specifies a currency. - // Bid response specifies a currency which is not the one specified in bid request. - // Expected to be invalid. - { - brqCur: []string{"USD"}, - brpCur: "EUR", - expectedValidBid: false, - }, - // Case bid request specifies several currencies. - // Bid response specifies a currency which is not the one specified in bid request. - // Expected to be invalid. - { - brqCur: []string{"USD", "EUR"}, - brpCur: "JPY", - expectedValidBid: false, - }, - } - - for _, tc := range currencyTestCases { - - brq := &openrtb.BidRequest{ - Cur: tc.brqCur, - } - - bids := make([]*pbsOrtbBid, 2) - bids[0] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "one-bid", - ImpID: "thisImp", - Price: 0.45, - CrID: "thisCreative", - }, - } - bids[1] = &pbsOrtbBid{ - bid: &openrtb.Bid{ - ID: "thatBid", - ImpID: "thatImp", - Price: 0.44, - CrID: "thatCreative", - }, - } - - brw := &bidResponseWrapper{ - adapterBids: &pbsOrtbSeatBid{ - bids: bids, - currency: tc.brpCur, - }, - } - - expectedValidBids := len(bids) - expectedErrs := 0 - - if tc.expectedValidBid != true { - // If currency mistmatch, we should have one error - expectedErrs = 1 - expectedValidBids = 0 - } - - assertBids(t, brq, brw, expectedValidBids, expectedErrs) - } -} - -func assertBids(t *testing.T, brq *openrtb.BidRequest, brw *bidResponseWrapper, ebids int, eerrs int) { - errs := brw.validateBids(brq) - if len(errs) != eerrs { - t.Errorf("Expected %d Errors validating bids, found %d", eerrs, len(errs)) - } - if len(brw.adapterBids.bids) != ebids { - t.Errorf("Expected %d bids, found %d bids", ebids, len(brw.adapterBids.bids)) - } -}