From 65c6c3608d0cca20168a69c8cf14d043a0d39a45 Mon Sep 17 00:00:00 2001 From: Laurentiu Badea Date: Mon, 14 Sep 2020 07:19:40 -0700 Subject: [PATCH] Refactor: move getAccount to accounts package (from openrtb2) (#1483) --- account/account.go | 69 +++++++++++++++++++++ account/account_test.go | 94 +++++++++++++++++++++++++++++ endpoints/openrtb2/amp_auction.go | 3 +- endpoints/openrtb2/auction.go | 56 +---------------- endpoints/openrtb2/auction_test.go | 70 +-------------------- endpoints/openrtb2/video_auction.go | 3 +- 6 files changed, 170 insertions(+), 125 deletions(-) create mode 100644 account/account.go create mode 100644 account/account_test.go diff --git a/account/account.go b/account/account.go new file mode 100644 index 00000000000..2f27b61efab --- /dev/null +++ b/account/account.go @@ -0,0 +1,69 @@ +package account + +import ( + "context" + "encoding/json" + "fmt" + + jsonpatch "github.com/evanphx/json-patch" + "github.com/prebid/prebid-server/config" + "github.com/prebid/prebid-server/errortypes" + "github.com/prebid/prebid-server/pbsmetrics" + "github.com/prebid/prebid-server/stored_requests" +) + +// GetAccount looks up the config.Account object referenced by the given accountID, with access rules applied +func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_requests.AccountFetcher, accountID string) (account *config.Account, errs []error) { + // Check BlacklistedAcctMap until we have deprecated it + if _, found := cfg.BlacklistedAcctMap[accountID]; found { + return nil, []error{&errortypes.BlacklistedAcct{ + Message: fmt.Sprintf("Prebid-server has disabled Account ID: %s, please reach out to the prebid server host.", accountID), + }} + } + if cfg.AccountRequired && accountID == pbsmetrics.PublisherUnknown { + return nil, []error{&errortypes.AcctRequired{ + Message: fmt.Sprintf("Prebid-server has been configured to discard requests without a valid Account ID. Please reach out to the prebid server host."), + }} + } + if accountJSON, accErrs := fetcher.FetchAccount(ctx, accountID); len(accErrs) > 0 || accountJSON == nil { + // accountID does not reference a valid account + for _, e := range accErrs { + if _, ok := e.(stored_requests.NotFoundError); !ok { + errs = append(errs, e) + } + } + if cfg.AccountRequired && cfg.AccountDefaults.Disabled { + errs = append(errs, &errortypes.AcctRequired{ + Message: fmt.Sprintf("Prebid-server could not verify the Account ID. Please reach out to the prebid server host."), + }) + return nil, errs + } + // Make a copy of AccountDefaults instead of taking a reference, + // to preserve original accountID in case is needed to check NonStandardPublisherMap + pubAccount := cfg.AccountDefaults + pubAccount.ID = accountID + account = &pubAccount + } else { + // accountID resolved to a valid account, merge with AccountDefaults for a complete config + account = &config.Account{} + completeJSON, err := jsonpatch.MergePatch(cfg.AccountDefaultsJSON(), accountJSON) + if err == nil { + err = json.Unmarshal(completeJSON, account) + } + if err != nil { + errs = append(errs, err) + return nil, errs + } + // Fill in ID if needed, so it can be left out of account definition + if len(account.ID) == 0 { + account.ID = accountID + } + } + if account.Disabled { + errs = append(errs, &errortypes.BlacklistedAcct{ + Message: fmt.Sprintf("Prebid-server has disabled Account ID: %s, please reach out to the prebid server host.", accountID), + }) + return nil, errs + } + return account, nil +} diff --git a/account/account_test.go b/account/account_test.go new file mode 100644 index 00000000000..0d192f18510 --- /dev/null +++ b/account/account_test.go @@ -0,0 +1,94 @@ +package account + +import ( + "context" + "encoding/json" + "fmt" + "testing" + + "github.com/prebid/prebid-server/config" + "github.com/prebid/prebid-server/errortypes" + "github.com/prebid/prebid-server/pbsmetrics" + "github.com/prebid/prebid-server/stored_requests" + "github.com/stretchr/testify/assert" +) + +var mockAccountData = map[string]json.RawMessage{ + "valid_acct": json.RawMessage(`{"disabled":false}`), + "disabled_acct": json.RawMessage(`{"disabled":true}`), +} + +type mockAccountFetcher struct { +} + +func (af mockAccountFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) { + if account, ok := mockAccountData[accountID]; ok { + return account, nil + } + return nil, []error{stored_requests.NotFoundError{accountID, "Account"}} +} + +func TestGetAccount(t *testing.T) { + unknown := pbsmetrics.PublisherUnknown + testCases := []struct { + accountID string + // account_required + required bool + // account_defaults.disabled + disabled bool + // expected error, or nil if account should be found + err error + }{ + // Blacklisted account is always rejected even in permissive setup + {accountID: "bad_acct", required: false, disabled: false, err: &errortypes.BlacklistedAcct{}}, + + // empty pubID + {accountID: unknown, required: false, disabled: false, err: nil}, + {accountID: unknown, required: true, disabled: false, err: &errortypes.AcctRequired{}}, + {accountID: unknown, required: false, disabled: true, err: &errortypes.BlacklistedAcct{}}, + {accountID: unknown, required: true, disabled: true, err: &errortypes.AcctRequired{}}, + + // pubID given but is not a valid host account (does not exist) + {accountID: "doesnt_exist_acct", required: false, disabled: false, err: nil}, + {accountID: "doesnt_exist_acct", required: true, disabled: false, err: nil}, + {accountID: "doesnt_exist_acct", required: false, disabled: true, err: &errortypes.BlacklistedAcct{}}, + {accountID: "doesnt_exist_acct", required: true, disabled: true, err: &errortypes.AcctRequired{}}, + + // pubID given and matches a valid host account with Disabled: false + {accountID: "valid_acct", required: false, disabled: false, err: nil}, + {accountID: "valid_acct", required: true, disabled: false, err: nil}, + {accountID: "valid_acct", required: false, disabled: true, err: nil}, + {accountID: "valid_acct", required: true, disabled: true, err: nil}, + + // pubID given and matches a host account explicitly disabled (Disabled: true on account json) + {accountID: "disabled_acct", required: false, disabled: false, err: &errortypes.BlacklistedAcct{}}, + {accountID: "disabled_acct", required: true, disabled: false, err: &errortypes.BlacklistedAcct{}}, + {accountID: "disabled_acct", required: false, disabled: true, err: &errortypes.BlacklistedAcct{}}, + {accountID: "disabled_acct", required: true, disabled: true, err: &errortypes.BlacklistedAcct{}}, + } + + for _, test := range testCases { + description := fmt.Sprintf(`ID=%s/required=%t/disabled=%t`, test.accountID, test.required, test.disabled) + t.Run(description, func(t *testing.T) { + cfg := &config.Configuration{ + BlacklistedAcctMap: map[string]bool{"bad_acct": true}, + AccountRequired: test.required, + AccountDefaults: config.Account{Disabled: test.disabled}, + } + fetcher := &mockAccountFetcher{} + assert.NoError(t, cfg.MarshalAccountDefaults()) + + account, errors := GetAccount(context.Background(), cfg, fetcher, test.accountID) + + if test.err == nil { + assert.Empty(t, errors) + assert.Equal(t, test.accountID, account.ID, "account.ID must match requested ID") + assert.Equal(t, false, account.Disabled, "returned account must not be disabled") + } else { + assert.NotEmpty(t, errors, "expected errors but got success") + assert.Nil(t, account, "return account must be nil on error") + assert.IsType(t, test.err, errors[0], "error is of unexpected type") + } + }) + } +} diff --git a/endpoints/openrtb2/amp_auction.go b/endpoints/openrtb2/amp_auction.go index 54f4706902d..1e92569e260 100644 --- a/endpoints/openrtb2/amp_auction.go +++ b/endpoints/openrtb2/amp_auction.go @@ -16,6 +16,7 @@ import ( "github.com/golang/glog" "github.com/julienschmidt/httprouter" "github.com/mxmCherry/openrtb" + accountService "github.com/prebid/prebid-server/account" "github.com/prebid/prebid-server/analytics" "github.com/prebid/prebid-server/config" "github.com/prebid/prebid-server/errortypes" @@ -158,7 +159,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h } labels.PubID = getAccountID(req.Site.Publisher) // Look up account now that we have resolved the pubID value - account, acctIDErrs := deps.getAccount(ctx, labels.PubID) + account, acctIDErrs := accountService.GetAccount(ctx, deps.cfg, deps.accounts, labels.PubID) if len(acctIDErrs) > 0 { errL = append(errL, acctIDErrs...) httpStatus := http.StatusBadRequest diff --git a/endpoints/openrtb2/auction.go b/endpoints/openrtb2/auction.go index 41c1c1677a5..d6cbc2285fb 100644 --- a/endpoints/openrtb2/auction.go +++ b/endpoints/openrtb2/auction.go @@ -22,6 +22,7 @@ import ( "github.com/mxmCherry/openrtb" "github.com/mxmCherry/openrtb/native" nativeRequests "github.com/mxmCherry/openrtb/native/request" + accountService "github.com/prebid/prebid-server/account" "github.com/prebid/prebid-server/analytics" "github.com/prebid/prebid-server/config" "github.com/prebid/prebid-server/errortypes" @@ -156,7 +157,7 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http } // Look up account now that we have resolved the pubID value - account, acctIDErrs := deps.getAccount(ctx, labels.PubID) + account, acctIDErrs := accountService.GetAccount(ctx, deps.cfg, deps.accounts, labels.PubID) if len(acctIDErrs) > 0 { errL = append(errL, acctIDErrs...) writeError(errL, w, &labels) @@ -1317,56 +1318,3 @@ func getAccountID(pub *openrtb.Publisher) string { } return pbsmetrics.PublisherUnknown } - -func (deps *endpointDeps) getAccount(ctx context.Context, pubID string) (account *config.Account, errs []error) { - // Check BlacklistedAcctMap until we have deprecated it - if _, found := deps.cfg.BlacklistedAcctMap[pubID]; found { - return nil, []error{&errortypes.BlacklistedAcct{ - Message: fmt.Sprintf("Prebid-server has disabled Account ID: %s, please reach out to the prebid server host.", pubID), - }} - } - if deps.cfg.AccountRequired && pubID == pbsmetrics.PublisherUnknown { - return nil, []error{&errortypes.AcctRequired{ - Message: fmt.Sprintf("Prebid-server has been configured to discard requests without a valid Account ID. Please reach out to the prebid server host."), - }} - } - if accountJSON, accErrs := deps.accounts.FetchAccount(ctx, pubID); len(accErrs) > 0 || accountJSON == nil { - // pubID does not reference a valid account - if len(accErrs) > 0 { - errs = append(errs, errs...) - } - if deps.cfg.AccountRequired && deps.cfg.AccountDefaults.Disabled { - errs = append(errs, &errortypes.AcctRequired{ - Message: fmt.Sprintf("Prebid-server has been configured to discard requests without a valid Account ID. Please reach out to the prebid server host."), - }) - return nil, errs - } - // Make a copy of AccountDefaults instead of taking a reference, - // to preserve original pubID in case is needed to check NonStandardPublisherMap - pubAccount := deps.cfg.AccountDefaults - pubAccount.ID = pubID - account = &pubAccount - } else { - // pubID resolved to a valid account, merge with AccountDefaults for a complete config - account = &config.Account{} - completeJSON, err := jsonpatch.MergePatch(deps.cfg.AccountDefaultsJSON(), accountJSON) - if err == nil { - err = json.Unmarshal(completeJSON, account) - } - if err != nil { - errs = append(errs, err) - return nil, errs - } - // Fill in ID if needed, so it can be left out of account definition - if len(account.ID) == 0 { - account.ID = pubID - } - } - if account.Disabled { - errs = append(errs, &errortypes.BlacklistedAcct{ - Message: fmt.Sprintf("Prebid-server has disabled Account ID: %s, please reach out to the prebid server host.", pubID), - }) - return nil, errs - } - return -} diff --git a/endpoints/openrtb2/auction_test.go b/endpoints/openrtb2/auction_test.go index 7dc244a28c3..925cffcebeb 100644 --- a/endpoints/openrtb2/auction_test.go +++ b/endpoints/openrtb2/auction_test.go @@ -1984,8 +1984,7 @@ func (cf mockStoredReqFetcher) FetchRequests(ctx context.Context, requestIDs []s } var mockAccountData = map[string]json.RawMessage{ - "valid_acct": json.RawMessage(`{"disabled":false}`), - "disabled_acct": json.RawMessage(`{"disabled":true}`), + "valid_acct": json.RawMessage(`{"disabled":false}`), } type mockAccountFetcher struct { @@ -2058,70 +2057,3 @@ type hardcodedResponseIPValidator struct { func (v hardcodedResponseIPValidator) IsValid(net.IP, iputil.IPVersion) bool { return v.response } - -func TestGetAccount(t *testing.T) { - unknown := pbsmetrics.PublisherUnknown - testCases := []struct { - accountID string - // account_required - required bool - // account_defaults.disabled - disabled bool - // expected error, or nil if account should be found - err error - }{ - // Blacklisted account is always rejected even in permissive setup - {accountID: "bad_acct", required: false, disabled: false, err: &errortypes.BlacklistedAcct{}}, - - // empty pubID - {accountID: unknown, required: false, disabled: false, err: nil}, - {accountID: unknown, required: true, disabled: false, err: &errortypes.AcctRequired{}}, - {accountID: unknown, required: false, disabled: true, err: &errortypes.BlacklistedAcct{}}, - {accountID: unknown, required: true, disabled: true, err: &errortypes.AcctRequired{}}, - - // pubID given but is not a valid host account (does not exist) - {accountID: "doesnt_exist_acct", required: false, disabled: false, err: nil}, - {accountID: "doesnt_exist_acct", required: true, disabled: false, err: nil}, - {accountID: "doesnt_exist_acct", required: false, disabled: true, err: &errortypes.BlacklistedAcct{}}, - {accountID: "doesnt_exist_acct", required: true, disabled: true, err: &errortypes.AcctRequired{}}, - - // pubID given and matches a valid host account with Disabled: false - {accountID: "valid_acct", required: false, disabled: false, err: nil}, - {accountID: "valid_acct", required: true, disabled: false, err: nil}, - {accountID: "valid_acct", required: false, disabled: true, err: nil}, - {accountID: "valid_acct", required: true, disabled: true, err: nil}, - - // pubID given and matches a host account explicitly disabled (Disabled: true on account json) - {accountID: "disabled_acct", required: false, disabled: false, err: &errortypes.BlacklistedAcct{}}, - {accountID: "disabled_acct", required: true, disabled: false, err: &errortypes.BlacklistedAcct{}}, - {accountID: "disabled_acct", required: false, disabled: true, err: &errortypes.BlacklistedAcct{}}, - {accountID: "disabled_acct", required: true, disabled: true, err: &errortypes.BlacklistedAcct{}}, - } - - for _, test := range testCases { - description := fmt.Sprintf(`ID=%s/required=%t/disabled=%t`, test.accountID, test.required, test.disabled) - t.Run(description, func(t *testing.T) { - deps := &endpointDeps{ - cfg: &config.Configuration{ - BlacklistedAcctMap: map[string]bool{"bad_acct": true}, - AccountRequired: test.required, - AccountDefaults: config.Account{Disabled: test.disabled}, - }, - accounts: &mockAccountFetcher{}, - } - assert.NoError(t, deps.cfg.MarshalAccountDefaults()) - - account, errors := deps.getAccount(context.Background(), test.accountID) - - if test.err == nil { - assert.Empty(t, errors) - assert.Equal(t, test.accountID, account.ID, "account.ID must match requested ID") - assert.Equal(t, false, account.Disabled, "returned account must not be disabled") - } else { - assert.NotEmpty(t, errors, "expected errors but got success") - assert.Nil(t, account, "return account must be nil on error") - assert.IsType(t, test.err, errors[0], "error is of unexpected type") - } - }) - } -} diff --git a/endpoints/openrtb2/video_auction.go b/endpoints/openrtb2/video_auction.go index f5494751cc2..ab5634c7853 100644 --- a/endpoints/openrtb2/video_auction.go +++ b/endpoints/openrtb2/video_auction.go @@ -23,6 +23,7 @@ import ( "github.com/golang/glog" "github.com/julienschmidt/httprouter" "github.com/mxmCherry/openrtb" + accountService "github.com/prebid/prebid-server/account" "github.com/prebid/prebid-server/analytics" "github.com/prebid/prebid-server/config" "github.com/prebid/prebid-server/exchange" @@ -255,7 +256,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re } // Look up account now that we have resolved the pubID value - account, acctIDErrs := deps.getAccount(ctx, labels.PubID) + account, acctIDErrs := accountService.GetAccount(ctx, deps.cfg, deps.accounts, labels.PubID) if len(acctIDErrs) > 0 { handleError(&labels, w, acctIDErrs, &vo, &debugLog) return