-
Notifications
You must be signed in to change notification settings - Fork 737
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.
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
UserSync activity #2897
UserSync activity #2897
Changes from 9 commits
ea46c83
5c9cff1
75b7e8b
b7d1ab5
5a388eb
2bdc95e
2cdc048
89272f2
f33bd5a
0e4eb7c
384c00b
695ab46
d64dfb8
cc2854a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import ( | |
"github.com/prebid/prebid-server/errortypes" | ||
"github.com/prebid/prebid-server/gdpr" | ||
"github.com/prebid/prebid-server/metrics" | ||
"github.com/prebid/prebid-server/privacy" | ||
gppPrivacy "github.com/prebid/prebid-server/privacy/gpp" | ||
"github.com/prebid/prebid-server/stored_requests" | ||
"github.com/prebid/prebid-server/usersync" | ||
|
@@ -65,7 +66,7 @@ func NewSetUIDEndpoint(cfg *config.Configuration, syncersByBidder map[string]use | |
|
||
query := r.URL.Query() | ||
|
||
syncer, err := getSyncer(query, syncersByKey) | ||
syncer, bidderName, err := getSyncer(query, syncersByKey) | ||
if err != nil { | ||
w.WriteHeader(http.StatusBadRequest) | ||
w.Write([]byte(err.Error())) | ||
|
@@ -110,6 +111,20 @@ func NewSetUIDEndpoint(cfg *config.Configuration, syncersByBidder map[string]use | |
return | ||
} | ||
|
||
activities, activitiesErr := privacy.NewActivityControl(account.Privacy) | ||
if activitiesErr != nil { | ||
if errortypes.ContainsFatalError([]error{activitiesErr}) { | ||
activities = privacy.ActivityControl{} | ||
} | ||
} | ||
|
||
userSyncActivityAllowed := activities.Allow(privacy.ActivitySyncUser, | ||
privacy.ScopedName{Scope: privacy.ScopeTypeBidder, Name: bidderName}) | ||
if userSyncActivityAllowed == privacy.ActivityDeny { | ||
w.WriteHeader(http.StatusUnavailableForLegalReasons) | ||
return | ||
} | ||
|
||
gdprRequestInfo, err := extractGDPRInfo(query) | ||
if err != nil { | ||
// Only exit if non-warning | ||
|
@@ -313,19 +328,19 @@ func parseConsentFromGppStr(gppQueryValue string) (string, error) { | |
return gdprConsent, nil | ||
} | ||
|
||
func getSyncer(query url.Values, syncersByKey map[string]usersync.Syncer) (usersync.Syncer, error) { | ||
key := query.Get("bidder") | ||
func getSyncer(query url.Values, syncersByKey map[string]usersync.Syncer) (usersync.Syncer, string, error) { | ||
bidderName := query.Get("bidder") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe this change is valid. The "bidder" parameter is actually the syncer key. There is often a 1:1 relationship between them, but it's possible to have a 1:many. Let's discuss at our next meeting how to address this, since we need the proper bidder name now for activity rule matching. Since we only ever expect the setuid endpoint to be called as a redirect from a cookie sync, we should have the freedom to make this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch Scott, yeah this function gets passed a query from a cookie sync redirect, where "bidder" would actually grab the syncer key. Maybe that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good though. The problem is there is a 1:many relationship from the syncer key to the bidder name, so we can't be sure which bidder it is. We'll likely be fine if at least one of the linked bidders is allowed, but that's complicated to check. I think the easier approach would be to actually set bidder to the bidder name. SetUID would still need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow! Let's discuss it in a team meeting. I'll follow your instructions here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw that you closed your |
||
|
||
if key == "" { | ||
return nil, errors.New(`"bidder" query param is required`) | ||
if bidderName == "" { | ||
return nil, "", errors.New(`"bidder" query param is required`) | ||
} | ||
|
||
syncer, syncerExists := syncersByKey[key] | ||
syncer, syncerExists := syncersByKey[bidderName] | ||
if !syncerExists { | ||
return nil, errors.New("The bidder name provided is not supported by Prebid Server") | ||
return nil, "", errors.New("The bidder name provided is not supported by Prebid Server") | ||
} | ||
|
||
return syncer, nil | ||
return syncer, bidderName, nil | ||
} | ||
|
||
// getResponseFormat reads the format query parameter or falls back to the syncer's default. | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -85,13 +85,17 @@ const ( | |||||
|
||||||
// StatusDuplicate specifies the bidder is a duplicate or shared a syncer key with another bidder choice. | ||||||
StatusDuplicate | ||||||
|
||||||
// StatusBlockedByPrivacy specifies a bidder sync url is not allowed by privacy activities | ||||||
StatusBlockedByPrivacy | ||||||
) | ||||||
|
||||||
// Privacy determines which privacy policies will be enforced for a user sync request. | ||||||
type Privacy interface { | ||||||
GDPRAllowsHostCookie() bool | ||||||
GDPRAllowsBidderSync(bidder string) bool | ||||||
CCPAAllowsBidderSync(bidder string) bool | ||||||
ActivityAllowsUserSync(bidder string) bool | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we rename to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gus, activity name we execute here is prebid-server/privacy/activity.go Line 19 in 5723674
and this is how it is in configs: prebid-server/config/activity.go Line 4 in 5723674
I'm open to rename it if you think |
||||||
} | ||||||
|
||||||
// standardChooser implements the user syncer algorithm per official Prebid specification. | ||||||
|
@@ -151,6 +155,11 @@ func (c standardChooser) evaluate(bidder string, syncersSeen map[string]struct{} | |||||
return nil, BidderEvaluation{Status: StatusAlreadySynced, Bidder: bidder, SyncerKey: syncer.Key()} | ||||||
} | ||||||
|
||||||
userSyncActivityAllowed := privacy.ActivityAllowsUserSync(bidder) | ||||||
if !userSyncActivityAllowed { | ||||||
return nil, BidderEvaluation{Status: StatusBlockedByPrivacy, Bidder: bidder, SyncerKey: syncer.Key()} | ||||||
} | ||||||
|
||||||
if !privacy.GDPRAllowsBidderSync(bidder) { | ||||||
return nil, BidderEvaluation{Status: StatusBlockedByGDPR, Bidder: bidder, SyncerKey: syncer.Key()} | ||||||
} | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably record a metric and update the analytics object
so
here as well.For GDPR, the metric we use when the error is
http.StatusUnavailableForLegalReasons
ismetrics.SetUidGDPRHostCookieBlocked
. Maybe we need to add a more generic privacy related blocking metric likemetrics.SetUidPrivacyHostCookieBlocked
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this offline and decided to add metrics in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a generic metric is a good idea, especially as the number of privacy policies is expected to increase. IMHO the existign GDPR and CCPA metrics could use the new generalized privacy blocking metric.