-
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 6 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 |
---|---|---|
|
@@ -147,6 +147,13 @@ func (c *cookieSyncEndpoint) parseRequest(r *http.Request) (usersync.Request, pr | |
} | ||
} | ||
|
||
activityControl, activitiesErr := privacy.NewActivityControl(account.Privacy) | ||
if activitiesErr != nil { | ||
if errortypes.ContainsFatalError([]error{activitiesErr}) { | ||
return usersync.Request{}, privacy.Policies{}, err | ||
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. We should ignore account config errors for activity control. Per the spec:
I recommend proceeding with a nil activities control object if there's an error and emit metrics and logs so the host can know about the problem. I understand that metrics / logs are scoped for a follow-up PR. 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. Done |
||
} | ||
} | ||
|
||
syncTypeFilter, err := parseTypeFilter(request.FilterSettings) | ||
if err != nil { | ||
return usersync.Request{}, privacy.Policies{}, err | ||
|
@@ -170,6 +177,7 @@ func (c *cookieSyncEndpoint) parseRequest(r *http.Request) (usersync.Request, pr | |
Privacy: usersyncPrivacy{ | ||
gdprPermissions: gdprPerms, | ||
ccpaParsedPolicy: ccpaParsedPolicy, | ||
activityControl: activityControl, | ||
}, | ||
SyncTypeFilter: syncTypeFilter, | ||
} | ||
|
@@ -499,6 +507,7 @@ type usersyncPrivacyConfig struct { | |
type usersyncPrivacy struct { | ||
gdprPermissions gdpr.Permissions | ||
ccpaParsedPolicy ccpa.ParsedPolicy | ||
activityControl privacy.ActivityControl | ||
} | ||
|
||
func (p usersyncPrivacy) GDPRAllowsHostCookie() bool { | ||
|
@@ -515,3 +524,9 @@ func (p usersyncPrivacy) CCPAAllowsBidderSync(bidder string) bool { | |
enforce := p.ccpaParsedPolicy.CanEnforce() && p.ccpaParsedPolicy.ShouldEnforce(bidder) | ||
return !enforce | ||
} | ||
|
||
func (p usersyncPrivacy) ActivityAllowsUserSync(bidder string) privacy.ActivityResult { | ||
activityResult := p.activityControl.Allow(privacy.ActivitySyncUser, | ||
privacy.ScopedName{Scope: privacy.ScopeTypeBidder, Name: bidder}) | ||
return activityResult | ||
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. Can you add tests to cover these lines? 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. Added! |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -498,6 +498,7 @@ func TestCookieSyncParseRequest(t *testing.T) { | |
expectedPrivacy privacy.Policies | ||
expectedRequest usersync.Request | ||
}{ | ||
|
||
{ | ||
description: "Complete Request - includes GPP string with EU TCF V2", | ||
givenBody: strings.NewReader(`{` + | ||
|
@@ -972,6 +973,36 @@ func TestCookieSyncParseRequest(t *testing.T) { | |
expectedError: errCookieSyncAccountBlocked.Error(), | ||
givenAccountRequired: true, | ||
}, | ||
|
||
{ | ||
description: "Account Defaults - Invalid activities Activities", | ||
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. Nitpick: Looks like you have activities twice in the test description. |
||
givenBody: strings.NewReader(`{` + | ||
`"bidders":["a", "b"],` + | ||
`"account":"ValidAccountInvalidActivities"` + | ||
`}`), | ||
givenGDPRConfig: config.GDPR{Enabled: true, DefaultValue: "0"}, | ||
givenCCPAEnabled: true, | ||
givenConfig: config.UserSync{ | ||
Cooperative: config.UserSyncCooperative{ | ||
EnabledByDefault: false, | ||
PriorityGroups: [][]string{{"a", "b", "c"}}, | ||
}, | ||
}, | ||
expectedPrivacy: privacy.Policies{}, | ||
expectedRequest: usersync.Request{ | ||
Bidders: []string(nil), | ||
Cooperative: usersync.Cooperative{ | ||
Enabled: false, | ||
PriorityGroups: [][]string(nil), | ||
}, | ||
Limit: 0, | ||
Privacy: nil, | ||
SyncTypeFilter: usersync.SyncTypeFilter{ | ||
IFrame: nil, | ||
Redirect: nil, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
for _, test := range testCases { | ||
|
@@ -996,8 +1027,9 @@ func TestCookieSyncParseRequest(t *testing.T) { | |
ccpaEnforce: test.givenCCPAEnabled, | ||
}, | ||
accountsFetcher: FakeAccountsFetcher{AccountData: map[string]json.RawMessage{ | ||
"TestAccount": json.RawMessage(`{"cookie_sync": {"default_limit": 20, "max_limit": 30, "default_coop_sync": true}}`), | ||
"DisabledAccount": json.RawMessage(`{"disabled":true}`), | ||
"TestAccount": json.RawMessage(`{"cookie_sync": {"default_limit": 20, "max_limit": 30, "default_coop_sync": true}}`), | ||
"DisabledAccount": json.RawMessage(`{"disabled":true}`), | ||
"ValidAccountInvalidActivities": json.RawMessage(`{"privacy":{"allowactivities":{"syncUser":{"rules":[{"condition":{"componentName": ["bidderA.bidderB.bidderC"]}}]}}}}`), | ||
}}, | ||
} | ||
assert.NoError(t, endpoint.config.MarshalAccountDefaults()) | ||
|
@@ -1870,6 +1902,41 @@ func TestUsersyncPrivacyCCPAAllowsBidderSync(t *testing.T) { | |
} | ||
} | ||
|
||
func TestActivityDefaultToDefaultResult(t *testing.T) { | ||
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 think you're too specific with this test name. I suspect this is testing the activity control integration where the default value is not important. Consider renaming to something like |
||
testCases := []struct { | ||
name string | ||
bidderName string | ||
allow bool | ||
expectedResult privacy.ActivityResult | ||
}{ | ||
{ | ||
name: "activity_is_allowed", | ||
bidderName: "bidderA", | ||
allow: true, | ||
expectedResult: privacy.ActivityAllow, | ||
}, | ||
{ | ||
name: "activity_is_denied", | ||
bidderName: "bidderA", | ||
allow: false, | ||
expectedResult: privacy.ActivityDeny, | ||
}, | ||
} | ||
|
||
for _, test := range testCases { | ||
t.Run(test.name, func(t *testing.T) { | ||
privacyConfig := getDefaultActivityConfig(test.bidderName, test.allow) | ||
activities, err := privacy.NewActivityControl(privacyConfig) | ||
assert.NoError(t, err) | ||
up := usersyncPrivacy{ | ||
activityControl: activities, | ||
} | ||
actualResult := up.ActivityAllowsUserSync(test.bidderName) | ||
assert.Equal(t, test.expectedResult, actualResult) | ||
}) | ||
} | ||
} | ||
|
||
func TestCombineErrors(t *testing.T) { | ||
testCases := []struct { | ||
description string | ||
|
@@ -2030,3 +2097,22 @@ func (p *fakePermissions) AuctionActivitiesAllowed(ctx context.Context, bidderCo | |
AllowBidRequest: true, | ||
}, nil | ||
} | ||
|
||
func getDefaultActivityConfig(componentName string, allow bool) *config.AccountPrivacy { | ||
return &config.AccountPrivacy{ | ||
AllowActivities: config.AllowActivities{ | ||
SyncUser: config.Activity{ | ||
Default: ptrutil.ToPtr(true), | ||
Rules: []config.ActivityRule{ | ||
{ | ||
Allow: allow, | ||
Condition: config.ActivityCondition{ | ||
ComponentName: []string{componentName}, | ||
ComponentType: []string{"bidder"}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ package endpoints | |
import ( | ||
"context" | ||
"errors" | ||
"github.com/prebid/prebid-server/errortypes" | ||
"github.com/prebid/prebid-server/privacy" | ||
"net/http" | ||
"net/url" | ||
"strconv" | ||
|
@@ -56,7 +58,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())) | ||
|
@@ -101,6 +103,21 @@ func NewSetUIDEndpoint(cfg *config.Configuration, syncersByBidder map[string]use | |
return | ||
} | ||
|
||
activities, activitiesErr := privacy.NewActivityControl(account.Privacy) | ||
if activitiesErr != nil { | ||
if errortypes.ContainsFatalError([]error{activitiesErr}) { | ||
w.WriteHeader(http.StatusBadRequest) | ||
return | ||
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. We should probably record a metric and update the analytics object 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. This is good point. 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 commentThe reason will be displayed to describe this comment to others. Learn more. The spec calls for us to only write to metrics / logs. In the case of error, we should continue with |
||
} | ||
} | ||
|
||
userSyncActivityAllowed := activities.Allow(privacy.ActivitySyncUser, | ||
privacy.ScopedName{Scope: privacy.ScopeTypeBidder, Name: bidderName}) | ||
if userSyncActivityAllowed == privacy.ActivityDeny { | ||
w.WriteHeader(http.StatusUnavailableForLegalReasons) | ||
return | ||
Comment on lines
+117
to
+118
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. We should probably record a metric and update the analytics object 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. 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 commentThe 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. |
||
} | ||
|
||
tcf2Cfg := tcf2CfgBuilder(cfg.GDPR.TCF2, account.GDPR) | ||
|
||
if shouldReturn, status, body := preventSyncsGDPR(query.Get("gdpr"), query.Get("gdpr_consent"), gdprPermsBuilder, tcf2Cfg); shouldReturn { | ||
|
@@ -148,19 +165,19 @@ func NewSetUIDEndpoint(cfg *config.Configuration, syncersByBidder map[string]use | |
}) | ||
} | ||
|
||
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 |
---|---|---|
|
@@ -3,7 +3,6 @@ package privacy | |
import ( | ||
"fmt" | ||
"github.com/prebid/prebid-server/config" | ||
"github.com/prebid/prebid-server/errortypes" | ||
"strings" | ||
) | ||
|
||
|
@@ -33,9 +32,6 @@ func NewActivityControl(privacyConf *config.AccountPrivacy) (ActivityControl, er | |
|
||
if privacyConf == nil { | ||
return ac, err | ||
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. Nitpick: Consider returning 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. Why was this turned back from |
||
} else { | ||
//temporarily disable Activities if they are specified at the account level | ||
return ac, &errortypes.Warning{Message: "account.Privacy has no effect as the feature is under development."} | ||
} | ||
|
||
plans := make(map[Activity]ActivityPlan) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,9 @@ | ||
package usersync | ||
|
||
import ( | ||
privacyActivity "github.com/prebid/prebid-server/privacy" | ||
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. The intent of the privacy interface in this package is to avoid adding a dependency / import. This is just needed for the ActivityResult, so please use a boolean there instead. When evaluating the result, both an Abstain and Allow result should map to |
||
) | ||
|
||
// Chooser determines which syncers are eligible for a given request. | ||
type Chooser interface { | ||
// Choose considers bidders to sync, filters the bidders, and returns the result of the | ||
|
@@ -85,13 +89,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) privacyActivity.ActivityResult | ||
} | ||
|
||
// standardChooser implements the user syncer algorithm per official Prebid specification. | ||
|
@@ -151,6 +159,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 == privacyActivity.ActivityDeny { | ||
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.
Can you add tests to cover these lines?
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.
Added