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

Remove legacy GDPR AMP config flag used to prevent buyer ID scrub on AMP requests #1565

Merged
merged 7 commits into from
Jan 5, 2021
Merged

Remove legacy GDPR AMP config flag used to prevent buyer ID scrub on AMP requests #1565

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f9e7b2a
Remove legacy GDPR AMP config flag used to prevent buyer ID scrub on …
bsardo Nov 5, 2020
722dcb2
Add warning if amp_exception config flag is set
bsardo Dec 2, 2020
93d2823
Merge removal of GDPR AMP Exception with master
bsardo Dec 17, 2020
c0f7829
Change amp_exception config flag set warning to fatal error
bsardo Dec 17, 2020
79a202c
Merge with master
bsardo Dec 18, 2020
50700f0
Update gdpr.amp_exception error message
bsardo Dec 18, 2020
dc57a78
Remove mock AMPException methods and unnecessary test struct field
bsardo Dec 22, 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
2 changes: 0 additions & 2 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,6 @@ type GDPR struct {
NonStandardPublisherMap map[string]struct{}
TCF1 TCF1 `mapstructure:"tcf1"`
TCF2 TCF2 `mapstructure:"tcf2"`
AMPException bool `mapstructure:"amp_exception"`
// EEACountries (EEA = European Economic Area) are a list of countries where we should assume GDPR applies.
// If the gdpr flag is unset in a request, but geo.country is set, we will assume GDPR applies if and only
// if the country matches one on this list. If both the GDPR flag and country are not set, we default
Expand Down Expand Up @@ -1045,7 +1044,6 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("gdpr.tcf2.special_purpose1.enabled", true)
v.SetDefault("gdpr.tcf2.purpose_one_treatement.enabled", true)
v.SetDefault("gdpr.tcf2.purpose_one_treatement.access_allowed", true)
v.SetDefault("gdpr.amp_exception", false)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of providing a warning to the host if this is enabled to guide them to understanding the change?

v.SetDefault("gdpr.eea_countries", []string{"ALA", "AUT", "BEL", "BGR", "HRV", "CYP", "CZE", "DNK", "EST",
"FIN", "FRA", "GUF", "DEU", "GIB", "GRC", "GLP", "GGY", "HUN", "ISL", "IRL", "IMN", "ITA", "JEY", "LVA",
"LIE", "LTU", "LUX", "MLT", "MTQ", "MYT", "NLD", "NOR", "POL", "PRT", "REU", "ROU", "BLM", "MAF", "SPM",
Expand Down
3 changes: 1 addition & 2 deletions exchange/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ func cleanOpenRTBRequests(ctx context.Context,

gdpr := extractGDPR(orig, usersyncIfAmbiguous)
consent := extractConsent(orig)
ampGDPRException := (labels.RType == pbsmetrics.ReqTypeAMP) && gDPR.AMPException()

ccpaEnforcer, err := extractCCPA(orig, privacyConfig, account, aliases, integrationTypeMap[labels.RType])
if err != nil {
Expand Down Expand Up @@ -131,7 +130,7 @@ func cleanOpenRTBRequests(ctx context.Context,
privacyEnforcement.GDPRID = false
}

privacyEnforcement.Apply(bidReq, ampGDPRException)
privacyEnforcement.Apply(bidReq)
}

return
Expand Down
3 changes: 0 additions & 3 deletions gdpr/gdpr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ type Permissions interface {
//
// If the consent string was nonsensical, the returned error will be an ErrorMalformedConsent.
PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, consent string) (bool, bool, bool, error)

// Exposes the AMP execption flag
AMPException() bool
}

// Versions of the GDPR TCF technical specification.
Expand Down
12 changes: 0 additions & 12 deletions gdpr/impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,6 @@ func (p *permissionsImpl) PersonalInfoAllowed(ctx context.Context, bidder openrt
return false, false, false, nil
}

func (p *permissionsImpl) AMPException() bool {
return p.cfg.AMPException
}

func (p *permissionsImpl) allowSync(ctx context.Context, vendorID uint16, consent string) (bool, error) {
// If we're not given a consent string, respect the preferences in the app config.
if consent == "" {
Expand Down Expand Up @@ -225,10 +221,6 @@ func (a AlwaysAllow) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext
return true, true, true, nil
}

func (a AlwaysAllow) AMPException() bool {
return false
}

// Exporting to allow for easy test setups
type AlwaysFail struct{}

Expand All @@ -243,7 +235,3 @@ func (a AlwaysFail) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.Bi
func (a AlwaysFail) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, consent string) (bool, bool, bool, error) {
return false, false, false, nil
}

func (a AlwaysFail) AMPException() bool {
return false
}
12 changes: 6 additions & 6 deletions privacy/enforcement.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ func (e Enforcement) Any() bool {
}

// Apply cleans personally identifiable information from an OpenRTB bid request.
func (e Enforcement) Apply(bidRequest *openrtb.BidRequest, ampGDPRException bool) {
e.apply(bidRequest, ampGDPRException, NewScrubber())
func (e Enforcement) Apply(bidRequest *openrtb.BidRequest) {
e.apply(bidRequest, NewScrubber())
}

func (e Enforcement) apply(bidRequest *openrtb.BidRequest, ampGDPRException bool, scrubber Scrubber) {
func (e Enforcement) apply(bidRequest *openrtb.BidRequest, scrubber Scrubber) {
if bidRequest != nil && e.Any() {
bidRequest.Device = scrubber.ScrubDevice(bidRequest.Device, e.getDeviceIDScrubStrategy(), e.getIPv4ScrubStrategy(), e.getIPv6ScrubStrategy(), e.getGeoScrubStrategy())
bidRequest.User = scrubber.ScrubUser(bidRequest.User, e.getUserScrubStrategy(ampGDPRException), e.getGeoScrubStrategy())
bidRequest.User = scrubber.ScrubUser(bidRequest.User, e.getUserScrubStrategy(), e.getGeoScrubStrategy())
}
}

Expand Down Expand Up @@ -70,7 +70,7 @@ func (e Enforcement) getGeoScrubStrategy() ScrubStrategyGeo {
return ScrubStrategyGeoNone
}

func (e Enforcement) getUserScrubStrategy(ampGDPRException bool) ScrubStrategyUser {
func (e Enforcement) getUserScrubStrategy() ScrubStrategyUser {
if e.COPPA {
return ScrubStrategyUserIDAndDemographic
}
Expand All @@ -79,7 +79,7 @@ func (e Enforcement) getUserScrubStrategy(ampGDPRException bool) ScrubStrategyUs
return ScrubStrategyUserID
}

if e.GDPRID && !ampGDPRException {
if e.GDPRID {
return ScrubStrategyUserID
}

Expand Down
57 changes: 3 additions & 54 deletions privacy/enforcement_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,23 +132,6 @@ func TestApply(t *testing.T) {
expectedUser: ScrubStrategyUserID,
expectedUserGeo: ScrubStrategyGeoReducedPrecision,
},
{
description: "GDPR Only - Full - AMP Exception",
enforcement: Enforcement{
CCPA: false,
COPPA: false,
GDPRGeo: true,
GDPRID: true,
LMT: false,
},
ampGDPRException: true,
expectedDeviceID: ScrubStrategyDeviceIDAll,
expectedDeviceIPv4: ScrubStrategyIPV4Lowest8,
expectedDeviceIPv6: ScrubStrategyIPV6Lowest16,
expectedDeviceGeo: ScrubStrategyGeoReducedPrecision,
expectedUser: ScrubStrategyUserNone,
expectedUserGeo: ScrubStrategyGeoReducedPrecision,
},
{
description: "GDPR Only - ID Only",
enforcement: Enforcement{
Expand All @@ -166,23 +149,6 @@ func TestApply(t *testing.T) {
expectedUser: ScrubStrategyUserID,
expectedUserGeo: ScrubStrategyGeoNone,
},
{
description: "GDPR Only - ID Only - AMP Exception",
enforcement: Enforcement{
CCPA: false,
COPPA: false,
GDPRGeo: false,
GDPRID: true,
LMT: false,
},
ampGDPRException: true,
expectedDeviceID: ScrubStrategyDeviceIDAll,
expectedDeviceIPv4: ScrubStrategyIPV4None,
expectedDeviceIPv6: ScrubStrategyIPV6None,
expectedDeviceGeo: ScrubStrategyGeoNone,
expectedUser: ScrubStrategyUserNone,
expectedUserGeo: ScrubStrategyGeoNone,
},
{
description: "GDPR Only - Geo Only",
enforcement: Enforcement{
Expand Down Expand Up @@ -216,23 +182,6 @@ func TestApply(t *testing.T) {
expectedUser: ScrubStrategyUserID,
expectedUserGeo: ScrubStrategyGeoReducedPrecision,
},
{
description: "Interactions: COPPA Only + AMP Exception",
enforcement: Enforcement{
CCPA: false,
COPPA: true,
GDPRGeo: false,
GDPRID: false,
LMT: false,
},
ampGDPRException: true,
expectedDeviceID: ScrubStrategyDeviceIDAll,
expectedDeviceIPv4: ScrubStrategyIPV4Lowest8,
expectedDeviceIPv6: ScrubStrategyIPV6Lowest32,
expectedDeviceGeo: ScrubStrategyGeoFull,
expectedUser: ScrubStrategyUserIDAndDemographic,
expectedUserGeo: ScrubStrategyGeoFull,
},
{
description: "Interactions: COPPA + GDPR Full + AMP Exception",
enforcement: Enforcement{
Expand Down Expand Up @@ -264,7 +213,7 @@ func TestApply(t *testing.T) {
m.On("ScrubDevice", req.Device, test.expectedDeviceID, test.expectedDeviceIPv4, test.expectedDeviceIPv6, test.expectedDeviceGeo).Return(replacedDevice).Once()
m.On("ScrubUser", req.User, test.expectedUser, test.expectedUserGeo).Return(replacedUser).Once()

test.enforcement.apply(req, test.ampGDPRException, m)
test.enforcement.apply(req, m)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're not using ampGDPRException any longer, any reason to still keep it as part of the testCase struct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, I'll remove. Good catch.


m.AssertExpectations(t)
assert.Same(t, replacedDevice, req.Device, "Device")
Expand All @@ -284,7 +233,7 @@ func TestApplyNoneApplicable(t *testing.T) {
GDPRID: false,
LMT: false,
}
enforcement.apply(req, false, m)
enforcement.apply(req, m)

m.AssertNotCalled(t, "ScrubDevice")
m.AssertNotCalled(t, "ScrubUser")
Expand All @@ -294,7 +243,7 @@ func TestApplyNil(t *testing.T) {
m := &mockScrubber{}

enforcement := Enforcement{}
enforcement.apply(nil, false, m)
enforcement.apply(nil, m)

m.AssertNotCalled(t, "ScrubDevice")
m.AssertNotCalled(t, "ScrubUser")
Expand Down