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

Add account/ host GDPR enabled flags & account per request type GDPR enabled flags #1564

Merged
merged 11 commits into from
Nov 12, 2020
Merged

Add account/ host GDPR enabled flags & account per request type GDPR enabled flags #1564

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e892e3b
Add account level request type specific and general GDPR enabled flags
bsardo Nov 3, 2020
2e5df5a
Clean up test TestAccountLevelGDPREnabled
bsardo Nov 3, 2020
cc7153b
Add host-level GDPR enabled flag
bsardo Nov 3, 2020
62a62b1
Move account GDPR enable check as receiver method on accountGDPR
bsardo Nov 4, 2020
2f1492a
Remove mapstructure annotations on account structs
bsardo Nov 4, 2020
8ff8bc9
Minor test updates
bsardo Nov 5, 2020
10786f6
Re-add mapstructure annotations on account structs
bsardo Nov 6, 2020
6297b88
Change RequestType to IntegrationType and struct annotation formatting
bsardo Nov 11, 2020
415238c
Update comment
bsardo Nov 12, 2020
4215369
Update account IntegrationType comments
bsardo Nov 12, 2020
f02f0b3
Remove extra space in config/accounts.go via gofmt
bsardo Nov 12, 2020
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
58 changes: 55 additions & 3 deletions config/accounts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,61 @@
package config

// RequestType : Request type enumeration
type RequestType string

// The request types
const (
RequestTypeAMP RequestType = "AMP"
RequestTypeApp RequestType = "app"
RequestTypeVideo RequestType = "video"
RequestTypeWeb RequestType = "web"
)
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

// Account represents a publisher account configuration
type Account struct {
ID string `mapstructure:"id" json:"id"`
Disabled bool `mapstructure:"disabled" json:"disabled"`
CacheTTL DefaultTTLs `mapstructure:"cache_ttl" json:"cache_ttl"`
ID string `mapstructure:"id" json:"id"`
Disabled bool `mapstructure:"disabled" json:"disabled"`
CacheTTL DefaultTTLs `mapstructure:"cache_ttl" json:"cache_ttl"`
EventsEnabled bool `mapstructure:"events_enabled" json:"events_enabled"`
bsardo marked this conversation as resolved.
Show resolved Hide resolved
GDPR AccountGDPR `mapstructure:"gdpr" json:"gdpr"`
}

// AccountGDPR represents account-specific GDPR configuration
type AccountGDPR struct {
Enabled *bool `mapstructure:"enabled" json:"enabled,omitempty"`
IntegrationEnabled AccountGDPRIntegration `mapstructure:"integration_enabled" json:"integration_enabled"`
}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

// EnabledForRequestType indicates whether GDPR is turned on at the account level for the specified request type
// by using the request type setting if defined or the general GDPR setting if defined; otherwise it returns nil
func (a *AccountGDPR) EnabledForRequestType(requestType RequestType) *bool {
var integrationEnabled *bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this additional setting has potential for ambiguous situations. For example, [enabled=true, all others=false], or [enabled=false, all others=true]. With account defaults layered in, activation of the individual settings is even less clear.

What are your thoughts on removing this fallback altogether ? We do not lose any configurability as the behavior can be fully specified by the four settings in the same section, and overrides apply straight on each field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hhhjort @SyntaxNode My take was that the committee already agreed on the configuration settings so it would be preferred to follow that, though I understand the two versions sometimes deviate from one another. I recognize that potential ambiguity @laurb9 pointed out. How do you guys feel about this?

Regarding account defaults, I figured the defaults for all of these is nil; you either need to explicitly set these on the account or fall back to whatever the host settings are.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that those cases are ambiguous. The intent is to provide an easy on/off switch with the ability to fine tune it if needed per request type. I suppose this is geared toward expressiveness in config. If an account doesn't care about each integration type, it's easier to set one enabled flag instead of four. The fallback also better defines future compatibility if a new integration type is added in the future.

Since this is implementing an already approved spec, I recommend moving this conversation to #1323.

I'd prefer to keep the same settings as PBS-Java currently for simplicity's sake.


switch requestType {
case RequestTypeAMP:
integrationEnabled = a.IntegrationEnabled.AMP
case RequestTypeApp:
integrationEnabled = a.IntegrationEnabled.App
case RequestTypeVideo:
integrationEnabled = a.IntegrationEnabled.Video
case RequestTypeWeb:
integrationEnabled = a.IntegrationEnabled.Web
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
}

if integrationEnabled != nil {
return integrationEnabled
}
if a.Enabled != nil {
return a.Enabled
}

return nil
}

// AccountGDPRIntegration indicates whether GDPR is enabled for each request type
type AccountGDPRIntegration struct {
AMP *bool `mapstructure:"amp" json:"amp,omitempty"`
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
App *bool `mapstructure:"app" json:"app,omitempty"`
Video *bool `mapstructure:"video" json:"video,omitempty"`
Web *bool `mapstructure:"web" json:"web,omitempty"`
}
116 changes: 116 additions & 0 deletions config/accounts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package config

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestAccountGDPREnabledForRequestType(t *testing.T) {
trueValue, falseValue := true, false

tests := []struct {
description string
giveRequestType RequestType
giveGDPREnabled *bool
giveAMPGDPREnabled *bool
giveAppGDPREnabled *bool
giveVideoGDPREnabled *bool
giveWebGDPREnabled *bool
wantEnabled *bool
}{
{
description: "GDPR AMP integration enabled, general GDPR disabled",
giveRequestType: RequestTypeAMP,
giveGDPREnabled: &falseValue,
giveAMPGDPREnabled: &trueValue,
wantEnabled: &trueValue,
},
{
description: "GDPR App integration enabled, general GDPR disabled",
giveRequestType: RequestTypeApp,
giveGDPREnabled: &falseValue,
giveAppGDPREnabled: &trueValue,
wantEnabled: &trueValue,
},
{
description: "GDPR Video integration enabled, general GDPR disabled",
giveRequestType: RequestTypeVideo,
giveGDPREnabled: &falseValue,
giveVideoGDPREnabled: &trueValue,
wantEnabled: &trueValue,
},
{
description: "GDPR Web integration enabled, general GDPR disabled",
giveRequestType: RequestTypeWeb,
giveGDPREnabled: &falseValue,
giveWebGDPREnabled: &trueValue,
wantEnabled: &trueValue,
},
{
description: "Web integration enabled, general GDPR unspecified",
giveRequestType: RequestTypeWeb,
giveGDPREnabled: nil,
giveWebGDPREnabled: &trueValue,
wantEnabled: &trueValue,
},
{
description: "GDPR Web integration disabled, general GDPR enabled",
giveRequestType: RequestTypeWeb,
giveGDPREnabled: &trueValue,
giveWebGDPREnabled: &falseValue,
wantEnabled: &falseValue,
},
{
description: "GDPR Web integration disabled, general GDPR unspecified",
giveRequestType: RequestTypeWeb,
giveGDPREnabled: nil,
giveWebGDPREnabled: &falseValue,
wantEnabled: &falseValue,
},
{
description: "GDPR Web integration unspecified, general GDPR disabled",
giveRequestType: RequestTypeWeb,
giveGDPREnabled: &falseValue,
giveWebGDPREnabled: nil,
wantEnabled: &falseValue,
},
{
description: "GDPR Web integration unspecified, general GDPR enabled",
giveRequestType: RequestTypeWeb,
giveGDPREnabled: &trueValue,
giveWebGDPREnabled: nil,
wantEnabled: &trueValue,
},
{
description: "GDPR Web integration unspecified, general GDPR unspecified",
giveRequestType: RequestTypeWeb,
giveGDPREnabled: nil,
giveWebGDPREnabled: nil,
wantEnabled: nil,
},
}

for _, tt := range tests {
account := Account{
GDPR: AccountGDPR{
Enabled: tt.giveGDPREnabled,
IntegrationEnabled: AccountGDPRIntegration{
AMP: tt.giveAMPGDPREnabled,
App: tt.giveAppGDPREnabled,
Video: tt.giveVideoGDPREnabled,
Web: tt.giveWebGDPREnabled,
},
},
}

enabled := account.GDPR.EnabledForRequestType(tt.giveRequestType)

if tt.wantEnabled == nil {
assert.Nil(t, enabled, tt.description)
} else {
assert.NotNil(t, enabled, tt.description)
assert.Equal(t, *tt.wantEnabled, *enabled, tt.description)
}
}
}
2 changes: 2 additions & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ type Privacy struct {
}

type GDPR struct {
Enabled bool `mapstructure:"enabled"`
HostVendorID int `mapstructure:"host_vendor_id"`
UsersyncIfAmbiguous bool `mapstructure:"usersync_if_ambiguous"`
Timeouts GDPRTimeouts `mapstructure:"timeouts_ms"`
Expand Down Expand Up @@ -1019,6 +1020,7 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("analytics.pubstack.buffers.count", 100)
v.SetDefault("analytics.pubstack.buffers.timeout", "900s")
v.SetDefault("amp_timeout_adjustment_ms", 0)
v.SetDefault("gdpr.enabled", true)
v.SetDefault("gdpr.host_vendor_id", 0)
v.SetDefault("gdpr.usersync_if_ambiguous", false)
v.SetDefault("gdpr.timeouts_ms.init_vendorlist_fetches", 0)
Expand Down
2 changes: 1 addition & 1 deletion exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque

// Slice of BidRequests, each a copy of the original cleaned to only contain bidder data for the named bidder
blabels := make(map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels)
cleanRequests, aliases, privacyLabels, errs := cleanOpenRTBRequests(ctx, bidRequest, requestExt, usersyncs, blabels, labels, e.gDPR, usersyncIfAmbiguous, e.privacyConfig)
cleanRequests, aliases, privacyLabels, errs := cleanOpenRTBRequests(ctx, bidRequest, requestExt, usersyncs, blabels, labels, e.gDPR, usersyncIfAmbiguous, e.privacyConfig, account)

e.me.RecordRequestPrivacy(privacyLabels)

Expand Down
2 changes: 2 additions & 0 deletions exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1300,6 +1300,7 @@ func runSpec(t *testing.T, filename string, spec *exchangeSpec) {
Enforce: spec.EnforceLMT,
},
GDPR: config.GDPR{
Enabled: spec.GDPREnabled,
UsersyncIfAmbiguous: !spec.AssumeGDPRApplies,
EEACountriesMap: eeac,
},
Expand Down Expand Up @@ -2456,6 +2457,7 @@ func TestUpdateHbPbCatDur(t *testing.T) {
}

type exchangeSpec struct {
GDPREnabled bool `json:"gdpr_enabled"`
IncomingRequest exchangeRequest `json:"incomingRequest"`
OutgoingRequests map[string]*bidderSpec `json:"outgoingRequests"`
Response exchangeResponse `json:"response,omitempty"`
Expand Down
1 change: 1 addition & 0 deletions exchange/exchangetest/gdpr-geo-eu-off-device.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"assume_gdpr_applies": false,
"gdpr_enabled": true,
"incomingRequest": {
"ortbRequest": {
"id": "some-request-id",
Expand Down
1 change: 1 addition & 0 deletions exchange/exchangetest/gdpr-geo-eu-off.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"assume_gdpr_applies": false,
"gdpr_enabled": true,
"incomingRequest": {
"ortbRequest": {
"id": "some-request-id",
Expand Down
62 changes: 62 additions & 0 deletions exchange/exchangetest/gdpr-geo-eu-on-featureflag-off.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
{
"assume_gdpr_applies": true,
"gdpr_enabled": false,
"incomingRequest": {
"ortbRequest": {
"id": "some-request-id",
"site": {
"page": "test.somepage.com"
},
"imp": [{
"id": "my-imp-id",
"video": {
"mimes": ["video/mp4"]
},
"ext": {
"appnexus": {
"placementId": 1
}
}
}],
"user": {
"buyeruid": "some-buyer-id",
"geo": {
"country": "FRA"
}
}
}
},
"outgoingRequests": {
"appnexus": {
"expectRequest": {
"ortbRequest": {
"id": "some-request-id",
"site": {
"page": "test.somepage.com"
},
"imp": [{
"id": "my-imp-id",
"video": {
"mimes": ["video/mp4"]
},
"ext": {
"bidder": {
"placementId": 1
}
}
}],
"user": {
"buyeruid": "some-buyer-id",
"geo": {
"country": "FRA"
}
}
},
"bidAdjustment": 1.0
},
"mockResponse": {
"errors": ["appnexus-error"]
}
}
}
}
1 change: 1 addition & 0 deletions exchange/exchangetest/gdpr-geo-eu-on.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"assume_gdpr_applies": true,
"gdpr_enabled": true,
"incomingRequest": {
"ortbRequest": {
"id": "some-request-id",
Expand Down
23 changes: 20 additions & 3 deletions exchange/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ import (
"github.com/prebid/prebid-server/privacy/lmt"
)

var requestTypeMap = map[pbsmetrics.RequestType]config.RequestType{
pbsmetrics.ReqTypeAMP: config.RequestTypeAMP,
pbsmetrics.ReqTypeORTB2App: config.RequestTypeApp,
pbsmetrics.ReqTypeVideo: config.RequestTypeVideo,
pbsmetrics.ReqTypeORTB2Web: config.RequestTypeWeb,
}

const unknownBidder string = ""

func BidderToPrebidSChains(req *openrtb_ext.ExtRequest) (map[string]*openrtb_ext.ExtRequestPrebidSChainSChain, error) {
Expand Down Expand Up @@ -53,7 +60,8 @@ func cleanOpenRTBRequests(ctx context.Context,
labels pbsmetrics.Labels,
gDPR gdpr.Permissions,
usersyncIfAmbiguous bool,
privacyConfig config.Privacy) (requestsByBidder map[openrtb_ext.BidderName]*openrtb.BidRequest, aliases map[string]string, privacyLabels pbsmetrics.PrivacyLabels, errs []error) {
privacyConfig config.Privacy,
account *config.Account) (requestsByBidder map[openrtb_ext.BidderName]*openrtb.BidRequest, aliases map[string]string, privacyLabels pbsmetrics.PrivacyLabels, errs []error) {

impsByBidder, errs := splitImps(orig.Imp)
if len(errs) > 0 {
Expand Down Expand Up @@ -94,7 +102,9 @@ func cleanOpenRTBRequests(ctx context.Context,
privacyLabels.COPPAEnforced = privacyEnforcement.COPPA
privacyLabels.LMTEnforced = lmtEnforcer.ShouldEnforce(unknownBidder)

if gdpr == 1 {
gdprEnabled := gdprEnabled(account, privacyConfig, requestTypeMap[labels.RType])

if gdpr == 1 && gdprEnabled {
privacyLabels.GDPREnforced = true
parsedConsent, err := vendorconsent.ParseString(consent)
if err == nil {
Expand All @@ -109,7 +119,7 @@ func cleanOpenRTBRequests(ctx context.Context,
privacyEnforcement.CCPA = ccpaEnforcer.ShouldEnforce(bidder.String())

// GDPR
if gdpr == 1 {
if gdpr == 1 && gdprEnabled {
coreBidder := resolveBidder(bidder.String(), aliases)

var publisherID = labels.PubID
Expand All @@ -127,6 +137,13 @@ func cleanOpenRTBRequests(ctx context.Context,
return
}

func gdprEnabled(account *config.Account, privacyConfig config.Privacy, requestType config.RequestType) bool {
if accountEnabled := account.GDPR.EnabledForRequestType(requestType); accountEnabled != nil {
return *accountEnabled
}
return privacyConfig.GDPR.Enabled
}

func extractCCPA(orig *openrtb.BidRequest, privacyConfig config.Privacy, aliases map[string]string) (privacy.PolicyEnforcer, error) {
ccpaPolicy, err := ccpa.ReadFromRequest(orig)
if err != nil {
Expand Down
Loading