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 3 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
15 changes: 15 additions & 0 deletions config/accounts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,19 @@ type Account struct {
Disabled bool `mapstructure:"disabled" json:"disabled"`
CacheTTL DefaultTTLs `mapstructure:"cache_ttl" json:"cache_ttl"`
EventsEnabled bool `mapstructure:"events_enabled" json:"events_enabled"`
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"`
}

// AccountGDPRIntegration indicates whether GDPR is enabled for each request type
type AccountGDPRIntegration struct {
AMP *bool `mapstructure:"amp" json:"amp,omitempty"`
App *bool `mapstructure:"app" json:"app,omitempty"`
Video *bool `mapstructure:"video" json:"video,omitempty"`
Web *bool `mapstructure:"web" json:"web,omitempty"`
}
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.Enabled,
UsersyncIfAmbiguous: !spec.AssumeGDPRApplies,
EEACountriesMap: eeac,
},
Expand Down Expand Up @@ -2456,6 +2457,7 @@ func TestUpdateHbPbCatDur(t *testing.T) {
}

type exchangeSpec struct {
Enabled bool `json:"enabled"`
Copy link
Contributor

Choose a reason for hiding this comment

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

minor, shouldn't this be GDPREnabled / 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,
"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,
"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,
"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,
"enabled": true,
"incomingRequest": {
"ortbRequest": {
"id": "some-request-id",
Expand Down
42 changes: 39 additions & 3 deletions exchange/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,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 +95,12 @@ func cleanOpenRTBRequests(ctx context.Context,
privacyLabels.COPPAEnforced = privacyEnforcement.COPPA
privacyLabels.LMTEnforced = lmtEnforcer.ShouldEnforce(unknownBidder)

if gdpr == 1 {
GDPREnabled := accountLevelGDPREnabled(account, labels.RType)
if GDPREnabled == nil {
GDPREnabled = hostLevelGDPREnabled(privacyConfig)
}

if gdpr == 1 && *GDPREnabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

This made me scratch my head for what the if statement would do if the GDPREnabled is nil. Segfault? That's not possible with the code path, so perhaps a bit of a refactor can eliminate that uncertainty. Consider moving the host level check into the helper func and have the func return a bool instead of a bool pointer.

privacyLabels.GDPREnforced = true
parsedConsent, err := vendorconsent.ParseString(consent)
if err == nil {
Expand All @@ -109,7 +115,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 +133,36 @@ func cleanOpenRTBRequests(ctx context.Context,
return
}

// GDPRAccountEnabled returns the GDPR setting for the request type in the account config if defined or
// the general GDPR setting in the account config if defined; otherwise it returns nil
func accountLevelGDPREnabled(account *config.Account, requestType pbsmetrics.RequestType) *bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better situated as a receiver of config.Account.GDPR?

account.GDPR.EnabledForRequestType(requestType pbsmetrics.RequestType) *bool

.. or with my refactor comment above, this could still be simplified to:

func accountLevelGDPREnabled(accountConfig *config.Account, privacyConfig config.Privacy, requestType pbsmetrics.RequestType) bool {
    if x := accountConfig.GDPR.EnabledForRequestType(requestType); x != nil {
        return *x
    }
    return privacyConfig.GDPR.Enabled
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yep I was just working on CCPA thinking about possibly refactoring that so it's a receiver; I'll move forward with that change.

var integrationEnabled *bool

switch requestType {
case pbsmetrics.ReqTypeAMP:
integrationEnabled = account.GDPR.IntegrationEnabled.AMP
case pbsmetrics.ReqTypeORTB2App:
integrationEnabled = account.GDPR.IntegrationEnabled.App
case pbsmetrics.ReqTypeVideo:
integrationEnabled = account.GDPR.IntegrationEnabled.Video
case pbsmetrics.ReqTypeORTB2Web:
integrationEnabled = account.GDPR.IntegrationEnabled.Web
}

if integrationEnabled != nil {
return integrationEnabled
}
if account.GDPR.Enabled != nil {
return account.GDPR.Enabled
}

return nil
}

func hostLevelGDPREnabled(privacyConfig config.Privacy) *bool {
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