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

DSA: Inject default in bid requests if not present #3540

Merged
merged 21 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
23eca73
Add account config
bsardo Feb 24, 2024
9cddf2b
Add DSA writer to insert default DSA in requests
bsardo Feb 24, 2024
e0c174e
Use DSA writer to insert default into each bidder request
bsardo Feb 25, 2024
a424d16
Add DSA default JSON test with test framework update
bsardo Feb 25, 2024
3d0a754
Insert default into request before splitting into bidder requests
bsardo Feb 25, 2024
ea58098
Change DataToPub to a pointer
bsardo Feb 27, 2024
2da4088
Rename dsawriter.go to writer.go
bsardo Feb 27, 2024
1bc45e6
Update variable name
bsardo Feb 27, 2024
3828209
Address feedback
bsardo Feb 28, 2024
d2ec47e
Update host config to accept DSA default as stringified JSON and map …
bsardo Mar 1, 2024
94857a3
Update exchange JSON test to accept stringified JSON in account config
bsardo Mar 1, 2024
e202460
Convert UnpackDSADefault from receiver method to function for use in …
bsardo Mar 1, 2024
c6c0d6f
Update account fetching to validate default DSA and unpack to struct
bsardo Mar 1, 2024
2ee9652
Update DSA writer to use unpacked default
bsardo Mar 1, 2024
8c29cb1
Handle error returned from RebuildRequest
bsardo Apr 2, 2024
ff93f52
Merge branch 'master' into DSA-3424
bsardo Apr 2, 2024
5729ab7
Group related conditions in Write
bsardo Apr 3, 2024
bdfca44
Feedback: error msg, env vars, minor refactor
bsardo Apr 4, 2024
f216c82
Clone DSA default unpacked and set on request
bsardo Apr 4, 2024
bcf23f3
Add integration test where default is ignored due to DSA on request
bsardo Apr 4, 2024
59309eb
Return error from RebuildRequest call
bsardo Apr 8, 2024
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
5 changes: 5 additions & 0 deletions account/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r
Message: fmt.Sprintf("The prebid-server account config for account id \"%s\" is malformed. Please reach out to the prebid server host.", accountID),
}}
}
if err := config.UnpackDSADefault(account.Privacy.DSA); err != nil {
return nil, []error{&errortypes.MalformedAcct{
Message: fmt.Sprintf("The prebid-server account config DSA for account id \"%s\" is malformed. Please reach out to the prebid server host.", accountID),
}}
}

// Fill in ID if needed, so it can be left out of account definition
if len(account.ID) == 0 {
Expand Down
34 changes: 31 additions & 3 deletions account/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,20 @@ import (
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/stored_requests"
"github.com/prebid/prebid-server/v2/util/iputil"
"github.com/prebid/prebid-server/v2/util/ptrutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

var (
validDSA = `{\"dsarequired\":1,\"pubrender\":2,\"transparency\":[{\"domain\":\"test.com\"}]}`
invalidDSA = `{\"dsarequired\":\"invalid\",\"pubrender\":2,\"transparency\":[{\"domain\":\"test.com\"}]}`
)

var mockAccountData = map[string]json.RawMessage{
"valid_acct": json.RawMessage(`{"disabled":false}`),
"valid_acct_dsa": json.RawMessage(`{"disabled":false, "privacy": {"dsa": {"default": "` + validDSA + `"}}}`),
"invalid_acct_dsa": json.RawMessage(`{"disabled":false, "privacy": {"dsa": {"default": "` + invalidDSA + `"}}}`),
"invalid_acct_ipv6_ipv4": json.RawMessage(`{"disabled":false, "privacy": {"ipv6": {"anon_keep_bits": -32}, "ipv4": {"anon_keep_bits": -16}}}`),
"disabled_acct": json.RawMessage(`{"disabled":true}`),
"malformed_acct": json.RawMessage(`{"disabled":"invalid type"}`),
Expand All @@ -36,6 +44,16 @@ func (af mockAccountFetcher) FetchAccount(ctx context.Context, accountDefaultsJS
}

func TestGetAccount(t *testing.T) {
validDSA := &openrtb_ext.ExtRegsDSA{
Required: ptrutil.ToPtr[int8](1),
PubRender: ptrutil.ToPtr[int8](2),
Transparency: []openrtb_ext.ExtBidDSATransparency{
{
Domain: "test.com",
},
},
}

unknown := metrics.PublisherUnknown
testCases := []struct {
accountID string
Expand All @@ -44,7 +62,8 @@ func TestGetAccount(t *testing.T) {
// account_defaults.disabled
disabled bool
// checkDefaultIP indicates IPv6 and IPv6 should be set to default values
checkDefaultIP bool
wantDefaultIP bool
wantDSA *openrtb_ext.ExtRegsDSA
// expected error, or nil if account should be found
err error
}{
Expand All @@ -66,7 +85,13 @@ func TestGetAccount(t *testing.T) {
{accountID: "valid_acct", required: false, disabled: true, err: nil},
{accountID: "valid_acct", required: true, disabled: true, err: nil},

{accountID: "invalid_acct_ipv6_ipv4", required: true, disabled: false, err: nil, checkDefaultIP: true},
{accountID: "valid_acct_dsa", required: false, disabled: false, wantDSA: validDSA, err: nil},
{accountID: "valid_acct_dsa", required: true, disabled: false, wantDSA: validDSA, err: nil},
{accountID: "valid_acct_dsa", required: false, disabled: true, wantDSA: validDSA, err: nil},
{accountID: "valid_acct_dsa", required: true, disabled: true, wantDSA: validDSA, err: nil},

{accountID: "invalid_acct_ipv6_ipv4", required: true, disabled: false, err: nil, wantDefaultIP: true},
{accountID: "invalid_acct_dsa", required: false, disabled: false, err: &errortypes.MalformedAcct{}},

// pubID given and matches a host account explicitly disabled (Disabled: true on account json)
{accountID: "disabled_acct", required: false, disabled: false, err: &errortypes.AccountDisabled{}},
Expand Down Expand Up @@ -111,10 +136,13 @@ func TestGetAccount(t *testing.T) {
assert.Nil(t, account, "return account must be nil on error")
assert.IsType(t, test.err, errors[0], "error is of unexpected type")
}
if test.checkDefaultIP {
if test.wantDefaultIP {
assert.Equal(t, account.Privacy.IPv6Config.AnonKeepBits, iputil.IPv6DefaultMaskingBitSize, "ipv6 should be set to default value")
assert.Equal(t, account.Privacy.IPv4Config.AnonKeepBits, iputil.IPv4DefaultMaskingBitSize, "ipv4 should be set to default value")
}
if test.wantDSA != nil {
assert.Equal(t, test.wantDSA, account.Privacy.DSA.DefaultUnpacked)
}
})
}
}
Expand Down
8 changes: 8 additions & 0 deletions config/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ func (m AccountModules) ModuleConfig(id string) (json.RawMessage, error) {

type AccountPrivacy struct {
AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"`
DSA *AccountDSA `mapstructure:"dsa" json:"dsa"`
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
IPv6Config IPv6 `mapstructure:"ipv6" json:"ipv6"`
IPv4Config IPv4 `mapstructure:"ipv4" json:"ipv4"`
PrivacySandbox PrivacySandbox `mapstructure:"privacysandbox" json:"privacysandbox"`
Expand All @@ -349,6 +350,13 @@ type CookieDeprecation struct {
TTLSec int `mapstructure:"ttl_sec"`
}

// AccountDSA represents DSA configuration
type AccountDSA struct {
Default string `mapstructure:"default" json:"default"`
DefaultUnpacked *openrtb_ext.ExtRegsDSA
GDPROnly bool `mapstructure:"gdpr_only" json:"gdpr_only"`
}

type IPv6 struct {
AnonKeepBits int `mapstructure:"anon_keep_bits" json:"anon_keep_bits"`
}
Expand Down
14 changes: 14 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,10 @@ func New(v *viper.Viper, bidderInfos BidderInfos, normalizeBidderName func(strin
return nil, err
}

if err := UnpackDSADefault(c.AccountDefaults.Privacy.DSA); err != nil {
return nil, err
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
}

// Update account defaults and generate base json for patch
c.AccountDefaults.CacheTTL = c.CacheURL.DefaultTTLs // comment this out to set explicitly in config

Expand Down Expand Up @@ -851,6 +855,16 @@ func (cfg *Configuration) MarshalAccountDefaults() error {
return err
}

// UnpackDSADefault validates the JSON DSA default object string by unmarshaling and maps it to a struct
func UnpackDSADefault(dsa *AccountDSA) error {
if dsa != nil && len(dsa.Default) > 0 {
if err := jsonutil.Unmarshal([]byte(dsa.Default), &dsa.DefaultUnpacked); err != nil {
return err
}
}
return nil
}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

// AccountDefaultsJSON returns the precompiled JSON form of account_defaults
func (cfg *Configuration) AccountDefaultsJSON() json.RawMessage {
return cfg.accountDefaultsJSON
Expand Down
89 changes: 89 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/prebid/go-gdpr/consentconstants"
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/util/ptrutil"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
)
Expand Down Expand Up @@ -527,6 +528,9 @@ account_defaults:
anon_keep_bits: 50
ipv4:
anon_keep_bits: 20
dsa:
default: "{\"dsarequired\":3,\"pubrender\":1,\"datatopub\":2,\"transparency\":[{\"domain\":\"domain.com\",\"dsaparams\":[1]}]}"
gdpr_only: true
privacysandbox:
cookiedeprecation:
enabled: true
Expand Down Expand Up @@ -660,6 +664,24 @@ func TestFullConfig(t *testing.T) {
cmpInts(t, "account_defaults.price_floors.fetch.max_age_sec", 6000, cfg.AccountDefaults.PriceFloors.Fetcher.MaxAge)
cmpInts(t, "account_defaults.price_floors.fetch.max_schema_dims", 10, cfg.AccountDefaults.PriceFloors.Fetcher.MaxSchemaDims)

// Assert the DSA was correctly unmarshalled and DefaultUnpacked was built correctly
expectedDSA := AccountDSA{
Default: "{\"dsarequired\":3,\"pubrender\":1,\"datatopub\":2,\"transparency\":[{\"domain\":\"domain.com\",\"dsaparams\":[1]}]}",
DefaultUnpacked: &openrtb_ext.ExtRegsDSA{
Required: ptrutil.ToPtr[int8](3),
PubRender: ptrutil.ToPtr[int8](1),
DataToPub: ptrutil.ToPtr[int8](2),
Transparency: []openrtb_ext.ExtBidDSATransparency{
{
Domain: "domain.com",
Params: []int{1},
},
},
},
GDPROnly: true,
}
assert.Equal(t, &expectedDSA, cfg.AccountDefaults.Privacy.DSA)

cmpBools(t, "account_defaults.events.enabled", true, cfg.AccountDefaults.Events.Enabled)

cmpInts(t, "account_defaults.privacy.ipv6.anon_keep_bits", 50, cfg.AccountDefaults.Privacy.IPv6Config.AnonKeepBits)
Expand Down Expand Up @@ -1853,3 +1875,70 @@ func TestTCF2FeatureOneVendorException(t *testing.T) {
assert.Equal(t, tt.wantIsVendorException, value, tt.description)
}
}

func TestUnpackDSADefault(t *testing.T) {
tests := []struct {
name string
giveDSA *AccountDSA
wantError bool
}{
{
name: "nil",
giveDSA: nil,
wantError: false,
},
{
name: "empty",
giveDSA: &AccountDSA{
Default: "",
},
wantError: false,
},
{
name: "empty_json",
giveDSA: &AccountDSA{
Default: "{}",
},
wantError: false,
},
{
name: "well_formed",
giveDSA: &AccountDSA{
Default: "{\"dsarequired\":3,\"pubrender\":1,\"datatopub\":2,\"transparency\":[{\"domain\":\"domain.com\",\"dsaparams\":[1]}]}",
},
wantError: false,
},
{
name: "well_formed_with_extra_fields",
giveDSA: &AccountDSA{
Default: "{\"unmappedkey\":\"unmappedvalue\",\"dsarequired\":3,\"pubrender\":1,\"datatopub\":2,\"transparency\":[{\"domain\":\"domain.com\",\"dsaparams\":[1]}]}",
},
wantError: false,
},
{
name: "invalid_type",
giveDSA: &AccountDSA{
Default: "{\"dsarequired\":\"invalid\",\"pubrender\":1,\"datatopub\":2,\"transparency\":[{\"domain\":\"domain.com\",\"dsaparams\":[1]}]}",
},
wantError: true,
},
{
name: "invalid_malformed_missing_colon",
giveDSA: &AccountDSA{
Default: "{\"dsarequired\"3,\"pubrender\":1,\"datatopub\":2,\"transparency\":[{\"domain\":\"domain.com\",\"dsaparams\":[1]}]}",
},
wantError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := UnpackDSADefault(tt.giveDSA)
if tt.wantError {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
})
}
}
35 changes: 35 additions & 0 deletions dsa/writer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package dsa

import (
"github.com/prebid/prebid-server/v2/config"
"github.com/prebid/prebid-server/v2/openrtb_ext"
)

// DSAWriter is used to write the default DSA to the request (req.regs.ext.dsa)
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
type Writer struct {
Config *config.AccountDSA
GDPRInScope bool
}

// Write sets the default DSA object on the request at regs.ext.dsa if it is
// defined in the account config and it is not already present on the request
func (dw Writer) Write(req *openrtb_ext.RequestWrapper) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is a stronger version of the function, but here's a version where we combine all of the conditions that result in us returning nil, rather than breaking them all up:

func (dw Writer) Write(req *openrtb_ext.RequestWrapper) error {
	if req == nil || getReqDSA(req) != nil || dw.Config == nil || dw.Config.DefaultUnpacked == nil ||
		(dw.Config.GDPROnly && !dw.GDPRInScope) {
		return nil
	}

	regExt, err := req.GetRegExt()
	if err != nil {
		return err
	}

	regExt.SetDSA(dw.Config.DefaultUnpacked)
	return nil
}

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is really a matter of preference. I see your perspective that there are multiple conditionals that result in the same return value, though my tendency is to break complex conditionals into logical groupings because I think it is easier to digest and instills confidence in the intended behavior. I also think it is easier to scan code vertically. In that case, at a minimum the first two conditionals could be grouped as such: if req == nil || getReqDSA(req) != nil {.
@SyntaxNode do you feel strongly either way here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm thinking your latest update looks great

if req == nil {
return nil
}
if getReqDSA(req) != nil {
return nil
}
if dw.Config == nil || dw.Config.DefaultUnpacked == nil {
return nil
}
if dw.Config.GDPROnly && !dw.GDPRInScope {
return nil
}
regExt, err := req.GetRegExt()
if err != nil {
return err
}
regExt.SetDSA(dw.Config.DefaultUnpacked)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some danger for memory corruption in pointing the request to using the same DefaultUnpacked instance for all requests. However, we also have the convention of "clone + set" when making changes to the request which likely mitigates this issue.

return nil
}
Loading
Loading