Skip to content

Commit

Permalink
Refactor: move getAccount to accounts package (from openrtb2) (#1483)
Browse files Browse the repository at this point in the history
  • Loading branch information
laurb9 authored Sep 14, 2020
1 parent fa23f5c commit 65c6c36
Show file tree
Hide file tree
Showing 6 changed files with 170 additions and 125 deletions.
69 changes: 69 additions & 0 deletions account/account.go
Original file line number Diff line number Diff line change
@@ -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
}
94 changes: 94 additions & 0 deletions account/account_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
})
}
}
3 changes: 2 additions & 1 deletion endpoints/openrtb2/amp_auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
56 changes: 2 additions & 54 deletions endpoints/openrtb2/auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
70 changes: 1 addition & 69 deletions endpoints/openrtb2/auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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")
}
})
}
}
3 changes: 2 additions & 1 deletion endpoints/openrtb2/video_auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 65c6c36

Please sign in to comment.