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

Refactor: Removed unused GDPR return value #1839

Merged
merged 4 commits into from
May 13, 2021
Merged

Refactor: Removed unused GDPR return value #1839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ca32a0
Remove unused GDPR PersonalInfoAllowed allowPI return value
bsardo May 6, 2021
1bebc71
Use named return values in all implementations of PersonalInfoAllowed
bsardo May 6, 2021
20c207d
Rename permissions object field allowPI on mocks
bsardo May 6, 2021
9a52fb1
Formatting fix
bsardo May 6, 2021
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: 2 additions & 3 deletions endpoints/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,6 @@ func TestShouldUsersync(t *testing.T) {
type auctionMockPermissions struct {
allowBidderSync bool
allowHostCookies bool
allowPI bool
allowGeo bool
allowID bool
}
Expand All @@ -454,8 +453,8 @@ func (m *auctionMockPermissions) BidderSyncAllowed(ctx context.Context, bidder o
return m.allowBidderSync, nil
}

func (m *auctionMockPermissions) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal gdpr.Signal, consent string, weakVendorEnforcement bool) (bool, bool, bool, error) {
return m.allowPI, m.allowGeo, m.allowID, nil
func (m *auctionMockPermissions) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal gdpr.Signal, consent string, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
return m.allowGeo, m.allowID, nil
}

func TestBidSizeValidate(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions endpoints/cookie_sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,6 @@ func (g *gdprPerms) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.Bi
return ok, nil
}

func (g *gdprPerms) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal gdpr.Signal, consent string, weakVendorEnforcement bool) (bool, bool, bool, error) {
return true, true, true, nil
func (g *gdprPerms) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal gdpr.Signal, consent string, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
return true, true, nil
}
16 changes: 8 additions & 8 deletions endpoints/setuid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,9 +389,9 @@ func makeRequest(uri string, existingSyncs map[string]string) *http.Request {
func doRequest(req *http.Request, metrics metrics.MetricsEngine, validFamilyNames []string, gdprAllowsHostCookies bool, gdprReturnsError bool) *httptest.ResponseRecorder {
cfg := config.Configuration{}
perms := &mockPermsSetUID{
allowHost: gdprAllowsHostCookies,
errorHost: gdprReturnsError,
allowPI: true,
allowHost: gdprAllowsHostCookies,
errorHost: gdprReturnsError,
personalInfoAllowed: true,
}
analytics := analyticsConf.NewPBSAnalytics(&cfg.Analytics)
syncers := make(map[openrtb_ext.BidderName]usersync.Usersyncer)
Expand Down Expand Up @@ -422,9 +422,9 @@ func parseCookieString(t *testing.T, response *httptest.ResponseRecorder) *users
}

type mockPermsSetUID struct {
allowHost bool
errorHost bool
allowPI bool
allowHost bool
errorHost bool
personalInfoAllowed bool
}

func (g *mockPermsSetUID) HostCookiesAllowed(ctx context.Context, gdprSignal gdpr.Signal, consent string) (bool, error) {
Expand All @@ -439,8 +439,8 @@ func (g *mockPermsSetUID) BidderSyncAllowed(ctx context.Context, bidder openrtb_
return false, nil
}

func (g *mockPermsSetUID) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal gdpr.Signal, consent string, weakVendorEnforcement bool) (bool, bool, bool, error) {
return g.allowPI, g.allowPI, g.allowPI, nil
func (g *mockPermsSetUID) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal gdpr.Signal, consent string, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
return g.personalInfoAllowed, g.personalInfoAllowed, nil
}

func newFakeSyncer(familyName string) usersync.Usersyncer {
Expand Down
2 changes: 1 addition & 1 deletion exchange/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func cleanOpenRTBRequests(ctx context.Context,
}
}
var publisherID = req.LegacyLabels.PubID
_, geo, id, err := gDPR.PersonalInfoAllowed(ctx, bidderRequest.BidderCoreName, publisherID, gdprSignal, consent, weakVendorEnforcement)
geo, id, err := gDPR.PersonalInfoAllowed(ctx, bidderRequest.BidderCoreName, publisherID, gdprSignal, consent, weakVendorEnforcement)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! This looks a lot cleaner.

if err == nil {
privacyEnforcement.GDPRGeo = !geo
privacyEnforcement.GDPRID = !id
Expand Down
4 changes: 2 additions & 2 deletions exchange/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ func (p *permissionsMock) BidderSyncAllowed(ctx context.Context, bidder openrtb_
return true, nil
}

func (p *permissionsMock) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdpr gdpr.Signal, consent string, weakVendorEnforcement bool) (bool, bool, bool, error) {
return p.personalInfoAllowed, p.personalInfoAllowed, p.personalInfoAllowed, p.personalInfoAllowedError
func (p *permissionsMock) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdpr gdpr.Signal, consent string, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
return p.personalInfoAllowed, p.personalInfoAllowed, p.personalInfoAllowedError
}

func assertReq(t *testing.T, bidderRequests []BidderRequest,
Expand Down
2 changes: 1 addition & 1 deletion gdpr/gdpr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type Permissions interface {
// Determines whether or not to send PI information to a bidder, or mask it out.
//
// If the consent string was nonsensical, the returned error will be an ErrorMalformedConsent.
PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal Signal, consent string, weakVendorEnforcement bool) (bool, bool, bool, error)
PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal Signal, consent string, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error)
}

// Versions of the GDPR TCF technical specification.
Expand Down
47 changes: 18 additions & 29 deletions gdpr/impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,19 @@ func (p *permissionsImpl) PersonalInfoAllowed(ctx context.Context,
PublisherID string,
gdprSignal Signal,
consent string,
weakVendorEnforcement bool) (allowPI bool, allowGeo bool, allowID bool, err error) {
weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
if _, ok := p.cfg.NonStandardPublisherMap[PublisherID]; ok {
return true, true, true, nil
return true, true, nil
}

gdprSignal = p.normalizeGDPR(gdprSignal)

if gdprSignal == SignalNo {
return true, true, true, nil
return true, true, nil
}

if consent == "" && gdprSignal == SignalYes {
return false, false, false, nil
return false, false, nil
}

if id, ok := p.vendorIDs[bidder]; ok {
Expand All @@ -85,8 +85,8 @@ func (p *permissionsImpl) PersonalInfoAllowed(ctx context.Context,
return p.defaultVendorPermissions()
}

func (p *permissionsImpl) defaultVendorPermissions() (allowPI bool, allowGeo bool, allowID bool, err error) {
return false, false, false, nil
func (p *permissionsImpl) defaultVendorPermissions() (allowGeo bool, allowID bool, err error) {
return false, false, nil
}

func (p *permissionsImpl) normalizeGDPR(gdprSignal Signal) Signal {
Expand Down Expand Up @@ -135,35 +135,34 @@ func (p *permissionsImpl) allowSync(ctx context.Context, vendorID uint16, consen
return false, nil
}

func (p *permissionsImpl) allowPI(ctx context.Context, vendorID uint16, consent string, weakVendorEnforcement bool) (bool, bool, bool, error) {
func (p *permissionsImpl) allowPI(ctx context.Context, vendorID uint16, consent string, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
parsedConsent, vendor, err := p.parseVendor(ctx, vendorID, consent)
if err != nil {
return false, false, false, err
return false, false, err
}

if vendor == nil {
return false, false, false, nil
return false, false, nil
}

if parsedConsent.Version() == 2 {
if p.cfg.TCF2.Enabled {
return p.allowPITCF2(parsedConsent, vendor, vendorID, weakVendorEnforcement)
}
if (vendor.Purpose(consentconstants.InfoStorageAccess) || vendor.LegitimateInterest(consentconstants.InfoStorageAccess) || weakVendorEnforcement) && parsedConsent.PurposeAllowed(consentconstants.InfoStorageAccess) && (vendor.Purpose(consentconstants.PersonalizationProfile) || vendor.LegitimateInterest(consentconstants.PersonalizationProfile) || weakVendorEnforcement) && parsedConsent.PurposeAllowed(consentconstants.PersonalizationProfile) && (parsedConsent.VendorConsent(vendorID) || weakVendorEnforcement) {
return true, true, true, nil
return true, true, nil
}
} else {
if (vendor.Purpose(tcf1constants.InfoStorageAccess) || vendor.LegitimateInterest(tcf1constants.InfoStorageAccess)) && parsedConsent.PurposeAllowed(tcf1constants.InfoStorageAccess) && (vendor.Purpose(tcf1constants.AdSelectionDeliveryReporting) || vendor.LegitimateInterest(tcf1constants.AdSelectionDeliveryReporting)) && parsedConsent.PurposeAllowed(tcf1constants.AdSelectionDeliveryReporting) && parsedConsent.VendorConsent(vendorID) {
return true, true, true, nil
return true, true, nil
}
}
return false, false, false, nil
return false, false, nil
}

func (p *permissionsImpl) allowPITCF2(parsedConsent api.VendorConsents, vendor api.Vendor, vendorID uint16, weakVendorEnforcement bool) (allowPI bool, allowGeo bool, allowID bool, err error) {
func (p *permissionsImpl) allowPITCF2(parsedConsent api.VendorConsents, vendor api.Vendor, vendorID uint16, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
consent, ok := parsedConsent.(tcf2.ConsentMetadata)
err = nil
allowPI = false
allowGeo = false
allowID = false
if !ok {
Expand All @@ -181,17 +180,7 @@ func (p *permissionsImpl) allowPITCF2(parsedConsent api.VendorConsents, vendor a
break
}
}
// Set to true so any purpose check can flip it to false
allowPI = true
if p.cfg.TCF2.Purpose1.Enabled {
allowPI = allowPI && p.checkPurpose(consent, vendor, vendorID, consentconstants.InfoStorageAccess, weakVendorEnforcement)
}
if p.cfg.TCF2.Purpose2.Enabled {
allowPI = allowPI && p.checkPurpose(consent, vendor, vendorID, consentconstants.BasicAdserving, weakVendorEnforcement)
}
if p.cfg.TCF2.Purpose7.Enabled {
allowPI = allowPI && p.checkPurpose(consent, vendor, vendorID, consentconstants.AdPerformance, weakVendorEnforcement)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we remove these checks? Is this outdated logic?

Copy link
Collaborator Author

@bsardo bsardo May 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is outdated as I don't see this logic anywhere in the TCF 2.0 spec. Either that or we have a bug because the allowPI value isn't being used anywhere 🙂. The purpose enabled config flags will be added in the near future in separate PRs to appropriately set the allowID and allowGeo flags.

Copy link
Collaborator Author

@bsardo bsardo May 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This logic was added as part of TCF 2.0 about a year ago. I'm wondering if the requirements were misunderstood or have changed since this logic was added. allowPI was used at one point to determine whether user IDs should be stripped when TCF 2.0 was first implemented and this logic is saying to strip it if at least one of the enabled purposes does not have consent while the requirements say that they should be stripped if at least one of the enabled purposes has consent.

From the spec:

  1. User IDs must be stripped from a given vendor’s auction in these scenarios:
  • User has not provided a legal basis for any Purpose 2,3,4,5,6,7,8,9, or 10 and the publisher has not defined an exception for the vendor for any of these purposes.

Also from the spec:

Activity - Passing User IDs into an auction
Legal Basis Required - Any Purpose 2-10. User IDs are important for more than personalizing ads - they can be used in frequency capping, building profiles, counting unique users, etc. So Prebid Server should pass User IDs through the auction if any of Purposes 2-10 pass the legal basis test.

I was planning to add this logic under a separate PR.

@hhhjort thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have both AllowID and AllowPI. So lines 177-182 take care of AllowID for point 19 from the spec quoted above. AllowPI is to flag other non-geo personal information, like age or gender, for screening or not. The valid uses cases for these other pieces of info were considered more limited, and thus only available for a limited selection of purposes.


return
}

Expand Down Expand Up @@ -265,8 +254,8 @@ func (a AlwaysAllow) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.B
return true, nil
}

func (a AlwaysAllow) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal Signal, consent string, weakVendorEnforcement bool) (bool, bool, bool, error) {
return true, true, true, nil
func (a AlwaysAllow) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal Signal, consent string, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
return true, true, nil
}

// Exporting to allow for easy test setups
Expand All @@ -280,6 +269,6 @@ func (a AlwaysFail) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.Bi
return false, nil
}

func (a AlwaysFail) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal Signal, consent string, weakVendorEnforcement bool) (bool, bool, bool, error) {
return false, false, false, nil
func (a AlwaysFail) PersonalInfoAllowed(ctx context.Context, bidder openrtb_ext.BidderName, PublisherID string, gdprSignal Signal, consent string, weakVendorEnforcement bool) (allowGeo bool, allowID bool, err error) {
return false, false, nil
}
Loading