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

Scrubber refactoring #3108

Merged
merged 16 commits into from
Nov 16, 2023
Merged

Scrubber refactoring #3108

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
76c472e
ScrubRequest refactoring
VeronikaSolovei9 Sep 13, 2023
571ae2a
First working version, needs a duplicated code refactoring
VeronikaSolovei9 Sep 14, 2023
b85dc81
Duplicated code refactoring
VeronikaSolovei9 Sep 14, 2023
330af18
Added unit tests
VeronikaSolovei9 Sep 14, 2023
edf120a
Code clean up
VeronikaSolovei9 Sep 24, 2023
b4f9ad2
Merge branch 'master' into scrubber_refactoring
VeronikaSolovei9 Sep 24, 2023
e25b154
Unit tests improvements
VeronikaSolovei9 Sep 25, 2023
8f3dd23
Added clone functions
VeronikaSolovei9 Oct 5, 2023
06c9e92
Merge branch 'master' into scrubber_refactoring
VeronikaSolovei9 Oct 23, 2023
f3059c2
Merge fixes
VeronikaSolovei9 Oct 23, 2023
57e08ea
Typo fix
VeronikaSolovei9 Oct 25, 2023
1ee688b
Typo fix in tests
VeronikaSolovei9 Oct 25, 2023
ea36a24
Code clean up
VeronikaSolovei9 Oct 29, 2023
ea4a9e3
Code clean up
VeronikaSolovei9 Oct 30, 2023
a836351
Code clean up
VeronikaSolovei9 Oct 30, 2023
8d24ed7
Code clean up
VeronikaSolovei9 Oct 31, 2023
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
78 changes: 62 additions & 16 deletions exchange/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/prebid/prebid-server/v2/gdpr"
"github.com/prebid/prebid-server/v2/metrics"
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/ortb"
"github.com/prebid/prebid-server/v2/privacy"
"github.com/prebid/prebid-server/v2/privacy/ccpa"
"github.com/prebid/prebid-server/v2/privacy/lmt"
Expand Down Expand Up @@ -153,11 +154,6 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,

// bidder level privacy policies
for _, bidderRequest := range allBidderRequests {
privacyEnforcement := privacy.Enforcement{
COPPA: coppa,
LMT: lmt,
}

// fetchBids activity
scopedName := privacy.Component{Type: privacy.ComponentTypeBidder, Name: bidderRequest.BidderName.String()}
fetchBidsActivityAllowed := auctionReq.Activities.Allow(privacy.ActivityFetchBids, scopedName, privacy.NewRequestFromBidRequest(*req))
Expand All @@ -180,46 +176,68 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,
}
}

ipConf := privacy.IPConf{IPV6: auctionReq.Account.Privacy.IPv6Config, IPV4: auctionReq.Account.Privacy.IPv4Config}

// FPD should be applied before policies, otherwise it overrides policies and activities restricted data
applyFPD(auctionReq.FirstPartyData, bidderRequest)

reqWrapper := cloneBidderReq(bidderRequest.BidRequest)

passIDActivityAllowed := auctionReq.Activities.Allow(privacy.ActivityTransmitUserFPD, scopedName, privacy.NewRequestFromBidRequest(*req))
if !passIDActivityAllowed {
privacyEnforcement.UFPD = true
//UFPD
privacy.ScrubUserFPD(reqWrapper)
} else {
// run existing policies (GDPR, CCPA, COPPA, LMT)
// potentially block passing IDs based on GDPR
if gdprEnforced {
if gdprErr == nil {
privacyEnforcement.GDPRID = !auctionPermissions.PassID
if !auctionPermissions.PassID {
privacy.ScrubGdprID(reqWrapper)
}
} else {
privacyEnforcement.GDPRID = true
privacy.ScrubGdprID(reqWrapper)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be simplified to:

if gdprErr != nil || !auctionPermissions.PassID {
	privacy.ScrubGdprID(reqWrapper)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I also combined it with the previous if statement.

if gdprEnforced && (gdprErr != nil || !auctionPermissions.PassID) {
   privacy.ScrubGdprID(reqWrapper)
}

}
// potentially block passing IDs based on CCPA
privacyEnforcement.CCPA = ccpaEnforcer.ShouldEnforce(bidderRequest.BidderName.String())
if ccpaEnforcer.ShouldEnforce(bidderRequest.BidderName.String()) {
privacy.ScrubDeviceIDsIPsUserDemoExt(reqWrapper, ipConf, "eids", false)
}
}

passGeoActivityAllowed := auctionReq.Activities.Allow(privacy.ActivityTransmitPreciseGeo, scopedName, privacy.NewRequestFromBidRequest(*req))
if !passGeoActivityAllowed {
privacyEnforcement.PreciseGeo = true
privacy.ScrubGeoAndDeviceIP(reqWrapper, ipConf)
} else {
// run existing policies (GDPR, CCPA, COPPA, LMT)
// potentially block passing geo based on GDPR
if gdprEnforced {
if gdprErr == nil {
privacyEnforcement.GDPRGeo = !auctionPermissions.PassGeo
if !auctionPermissions.PassGeo {
privacy.ScrubGeoAndDeviceIP(reqWrapper, ipConf)
}
} else {
privacyEnforcement.GDPRGeo = true
privacy.ScrubGeoAndDeviceIP(reqWrapper, ipConf)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be simplified to:

if gdprErr != nil || !auctionPermissions.PassGeo {
    privacy.ScrubGeoAndDeviceIP(reqWrapper, ipConf)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored

}
// potentially block passing geo based on CCPA
privacyEnforcement.CCPA = ccpaEnforcer.ShouldEnforce(bidderRequest.BidderName.String())
if ccpaEnforcer.ShouldEnforce(bidderRequest.BidderName.String()) {
privacy.ScrubDeviceIDsIPsUserDemoExt(reqWrapper, ipConf, "eids", false)
}
}

if lmt || coppa {
privacy.ScrubDeviceIDsIPsUserDemoExt(reqWrapper, ipConf, "eids", coppa)
}

applyFPD(auctionReq.FirstPartyData, bidderRequest)
passTIDAllowed := auctionReq.Activities.Allow(privacy.ActivityTransmitTIDs, scopedName, privacy.NewRequestFromBidRequest(*req))
if !passTIDAllowed {
privacy.ScrubTID(reqWrapper)
}

privacyEnforcement.TID = !auctionReq.Activities.Allow(privacy.ActivityTransmitTIDs, scopedName, privacy.NewRequestFromBidRequest(*req))
reqWrapper.RebuildRequest()
bidderRequest.BidRequest = reqWrapper.BidRequest

privacyEnforcement.Apply(bidderRequest.BidRequest, auctionReq.Account.Privacy)
allowedBidderRequests = append(allowedBidderRequests, bidderRequest)

// GPP downgrade: always downgrade unless we can confirm GPP is supported
Expand All @@ -232,6 +250,34 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,
return
}

// cloneBidderReq - clones bidder request and replaces req.User and req.Device with new copies
func cloneBidderReq(req *openrtb2.BidRequest) *openrtb_ext.RequestWrapper {

// bidder request may be modified differently per bidder based on privacy configs
// new request should be created for each bidder request
// pointer fields like User and Device should be cloned and set back to the request copy
var newReq *openrtb2.BidRequest
newReq = ptrutil.Clone(req)
Comment on lines +247 to +248
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these lines be rewritten as newReq := ptrutil.Clone(req)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be, yes, but IDE will miss the variable type (because it's generic after Clone) and underlines follow up code to red. We discussed it in one of the previous PRs. It's ok to leave the variable instantiation, it doesn't take memory and CPU and makes IDE happy.


if req.User != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use ortb.CloneUser instead.

userCopy := ortb.CloneUser(req.User)
newReq.User = userCopy
}

if req.Device != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add ortb.CloneDevice and call it from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no CloneDevice and CloneSource functions. Is there a reason not to have them? Should I add them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clone functions added

deviceCopy := ortb.CloneDevice(req.Device)
newReq.Device = deviceCopy
}

if req.Source != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add ortb.CloneSource and call it from here.

sourceCopy := ortb.CloneSource(req.Source)
newReq.Source = sourceCopy
}

reqWrapper := &openrtb_ext.RequestWrapper{BidRequest: newReq}
return reqWrapper
}

func shouldSetLegacyPrivacy(bidderInfo config.BidderInfos, bidder string) bool {
binfo, defined := bidderInfo[bidder]

Expand Down
Loading
Loading