UserSync activity #2897

VeronikaSolovei9 · 2023-07-04T21:55:08Z

User sync activities for bidder.

In this PR I removed a statement that disables activities. If activities are not specified in config then nil plans will be returned. Default result is Abstain and it will not modify existing behavior.
This is needed because more and more unit tests start to depend on NewActivityControl function as this is a part of auction logic.

bsardo · 2023-07-06T03:28:09Z

endpoints/setuid.go

+				w.WriteHeader(http.StatusBadRequest)
+				return


We should probably record a metric and update the analytics object so here as well.
Should the param passed to metricsEngine.RecordSetUid be one of the account options (metrics.SetUidAccountConfigMalformed or SetUidAccountInvalid) or something more generic?

This is good point. We discussed this offline and decided to add metrics in a separate PR.

The spec calls for us to only write to metrics / logs. In the case of error, we should continue with activities set to nil.

bsardo · 2023-07-06T03:30:56Z

endpoints/setuid.go

+			w.WriteHeader(http.StatusUnavailableForLegalReasons)
+			return


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 is metrics.SetUidGDPRHostCookieBlocked. Maybe we need to add a more generic privacy related blocking metric like metrics.SetUidPrivacyHostCookieBlocked?

We discussed this offline and decided to add metrics in a separate PR.

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.

AlexBVolcy · 2023-07-10T20:30:16Z

endpoints/cookie_sync.go

+		if errortypes.ContainsFatalError([]error{activitiesErr}) {
+			return usersync.Request{}, privacy.Policies{}, err


Can you add tests to cover these lines?

AlexBVolcy · 2023-07-10T20:30:52Z

endpoints/cookie_sync.go

+func (p usersyncPrivacy) ActivityAllowsUserSync(bidder string) privacy.ActivityResult {
+	activityResult := p.activityControl.Allow(privacy.ActivitySyncUser,
+		privacy.ScopedName{Scope: privacy.ScopeTypeBidder, Name: bidder})
+	return activityResult


Can you add tests to cover these lines?

AlexBVolcy · 2023-07-11T22:22:01Z

endpoints/cookie_sync_test.go

@@ -1870,6 +1902,42 @@ func TestUsersyncPrivacyCCPAAllowsBidderSync(t *testing.T) {
 	}
 }

+func TestActivityDefaultToDefaultResult(t *testing.T) {
+


Nitpick: Could you remove this blank line?

AlexBVolcy · 2023-07-11T22:23:34Z

usersync/chooser_test.go

+	gdprAllowsHostCookie   bool
+	gdprAllowsBidderSync   bool
+	ccpaAllowsBidderSync   bool
+	activityAllowIUserSync privacy.ActivityResult


Is there a typo here with activityAllowIUserSync? Should it be activityAllowsUserSync?

This is a typo. Fixed

SyntaxNode · 2023-07-12T19:32:39Z

endpoints/cookie_sync_test.go

@@ -972,6 +973,36 @@ func TestCookieSyncParseRequest(t *testing.T) {
 			expectedError:        errCookieSyncAccountBlocked.Error(),
 			givenAccountRequired: true,
 		},
+
+		{
+			description: "Account Defaults - Invalid activities Activities",


Nitpick: Looks like you have activities twice in the test description.

SyntaxNode · 2023-07-12T19:36:01Z

endpoints/cookie_sync.go

+	activityControl, activitiesErr := privacy.NewActivityControl(account.Privacy)
+	if activitiesErr != nil {
+		if errortypes.ContainsFatalError([]error{activitiesErr}) {
+			return usersync.Request{}, privacy.Policies{}, err


We should ignore account config errors for activity control. Per the spec:

If a rule is invalid for any reason, skip it and emit an error at N% sampling.

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.

SyntaxNode · 2023-07-12T19:39:50Z

endpoints/cookie_sync_test.go

@@ -1870,6 +1902,41 @@ func TestUsersyncPrivacyCCPAAllowsBidderSync(t *testing.T) {
 	}
 }

+func TestActivityDefaultToDefaultResult(t *testing.T) {


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 TestCookieSyncActivityControlIntegration.

SyntaxNode · 2023-07-12T19:44:17Z

privacy/enforcer.go

@@ -33,9 +32,6 @@ func NewActivityControl(privacyConf *config.AccountPrivacy) (ActivityControl, er

 	if privacyConf == nil {
 		return ac, err


Nitpick: Consider returning nil for an error here to make it clear this a valid state.

Why was this turned back from nil to err?

SyntaxNode · 2023-07-12T19:48:29Z

endpoints/setuid.go

+				w.WriteHeader(http.StatusBadRequest)
+				return


The spec calls for us to only write to metrics / logs. In the case of error, we should continue with activities set to nil.

SyntaxNode · 2023-07-12T19:50:12Z

endpoints/setuid.go

+			w.WriteHeader(http.StatusUnavailableForLegalReasons)
+			return


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.

SyntaxNode · 2023-07-12T20:01:03Z

endpoints/setuid.go

-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")


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.

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 syncersByBidder map that gets passed into NewSetUIDEndpoint() can be used to get the proper bidder name?

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 syncersByBidder map to get the syncer key.

Wow! Let's discuss it in a team meeting. I'll follow your instructions here.

I saw that you closed your bidderNameBySyncer PR. Will your changes be reflected in this PR or will you open a new one for this?

SyntaxNode · 2023-07-12T20:04:46Z

usersync/chooser.go

@@ -1,5 +1,9 @@
 package usersync

+import (
+	privacyActivity "github.com/prebid/prebid-server/privacy"


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 true.

VeronikaSolovei9 · 2023-07-13T21:28:42Z

All comments are addressed except of one. We need to find the right way to get a bidder name.

AlexBVolcy

Left one comment! Also could you merge with master to fix conflicts!

AlexBVolcy · 2023-07-18T21:05:10Z

endpoints/setuid.go

-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")


I saw that you closed your bidderNameBySyncer PR. Will your changes be reflected in this PR or will you open a new one for this?

# Conflicts: # endpoints/setuid.go

VeronikaSolovei9 · 2023-07-19T00:01:37Z

Alex, good point!
I believe this PR will stay the same. It will not be merged until we find a way to pass a bidder name properly.
bidderName here will be actual bidder name, not a syncer key.

guscarreon · 2023-07-19T00:02:38Z

usersync/chooser.go

 )

 // 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


Should we rename to ActivityAllowsBidderSync to match GDPRAllowsBidderSync and CCPAAllowsBidderSync?

Gus, activity name we execute here is syncUser :

prebid-server/privacy/activity.go

Line 19 in 5723674

case ActivitySyncUser:

and this is how it is in configs:

prebid-server/config/activity.go

Line 4 in 5723674

SyncUser Activity `mapstructure:"syncUser" json:"syncUser"`

I'm open to rename it if you think ActivityAllowsBidderSync is better than ActivityAllowsUserSync.

# Conflicts: # endpoints/setuid.go

AlexBVolcy · 2023-07-27T02:19:30Z

endpoints/setuid.go

+	if len(strSID) > 0 {
+		gppSID, err := stringutil.StrToInt8Slice(strSID)
+		if err != nil {
+			return gdpr.SignalAmbiguous, fmt.Errorf("Error parsing gpp_sid %s", err.Error())


For my own understanding is it okay to use fmt when returning an error? I typically use something like errors.New()

I use fmt.Error when I want to pass a parameter to error message. For instance when you want to build dynamic error messages and use %s, %d to format values. fmt.Error returns usual error, it just helps to format the message.

AlexBVolcy · 2023-07-27T02:19:56Z

endpoints/setuid.go

+// first and the 'gdpr' and 'gdpr_consent' query params second. If found in both, throws a
+// warning. Can also throw a parsing or validation error
+func extractGDPRInfo(query url.Values) (reqInfo gdpr.RequestInfo, err error) {
+


Nitpick: Could you remove this blank line?

# Conflicts: # privacy/enforcer.go

AlexBVolcy

Very close to approval, just one last comment

AlexBVolcy · 2023-07-27T21:33:06Z

privacy/enforcer.go

@@ -33,9 +32,6 @@ func NewActivityControl(privacyConf *config.AccountPrivacy) (ActivityControl, er

 	if privacyConf == nil {
 		return ac, err


Why was this turned back from nil to err?

AlexBVolcy

LGTM

guscarreon

LGTM

bsardo self-assigned this Jul 5, 2023

bsardo reviewed Jul 6, 2023

View reviewed changes

bsardo added the do not port label Jul 6, 2023

UserSync activity

ea46c83

VeronikaSolovei9 force-pushed the activity_userSync branch from 7f0de64 to ea46c83 Compare July 6, 2023 22:14

VeronikaSolovei9 changed the base branch from activities to master July 6, 2023 22:15

Removed comment and removed statement to disable actions.

5c9cff1

VeronikaSolovei9 marked this pull request as ready for review July 6, 2023 22:36

VeronikaSolovei9 added 2 commits July 6, 2023 15:39

Uncomment disabled test

75b7e8b

Moved ActivityControl inside the privacy interface and object

b7d1ab5

SyntaxNode assigned SyntaxNode and AlexBVolcy and unassigned bsardo Jul 10, 2023

AlexBVolcy reviewed Jul 10, 2023

View reviewed changes

Added unit tests

5a388eb

AlexBVolcy reviewed Jul 11, 2023

View reviewed changes

Minor cleanup

2bdc95e

SyntaxNode reviewed Jul 12, 2023

View reviewed changes

CR refactoring

2cdc048

AlexBVolcy reviewed Jul 18, 2023

View reviewed changes

VeronikaSolovei9 added 2 commits July 18, 2023 14:21

Merge branch 'master' into activity_userSync

89272f2

# Conflicts: # endpoints/setuid.go

Merge conflicts

f33bd5a

guscarreon reviewed Jul 19, 2023

View reviewed changes

hhhjort added the blocked label Jul 19, 2023

VeronikaSolovei9 mentioned this pull request Jul 20, 2023

Use bidder name instead of syncer key #2948

Merged

VeronikaSolovei9 added 2 commits July 26, 2023 11:47

Merge branch 'master' into activity_userSync

0e4eb7c

# Conflicts: # endpoints/setuid.go

Merge conflicts

384c00b

VeronikaSolovei9 removed the blocked label Jul 26, 2023

AlexBVolcy reviewed Jul 27, 2023

View reviewed changes

VeronikaSolovei9 added 2 commits July 26, 2023 22:13

Removed empty line

695ab46

Merge branch 'master' into activity_userSync

d64dfb8

# Conflicts: # privacy/enforcer.go

AlexBVolcy reviewed Jul 27, 2023

View reviewed changes

Error to nil fix.

cc2854a

AlexBVolcy approved these changes Jul 27, 2023

View reviewed changes

guscarreon approved these changes Jul 27, 2023

View reviewed changes

VeronikaSolovei9 merged commit 555c0a1 into master Jul 27, 2023

SyntaxNode deleted the activity_userSync branch October 2, 2023 17:52

		if errortypes.ContainsFatalError([]error{activitiesErr}) {
		return usersync.Request{}, privacy.Policies{}, err

		@@ -33,9 +32,6 @@ func NewActivityControl(privacyConf *config.AccountPrivacy) (ActivityControl, er

		if privacyConf == nil {
		return ac, err

UserSync activity #2897

UserSync activity #2897

Conversation

VeronikaSolovei9 commented Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jul 13, 2023

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy left a comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jul 4, 2023 •

edited

Loading

VeronikaSolovei9 Jul 12, 2023 •

edited

Loading

AlexBVolcy Jul 27, 2023 •

edited

Loading