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.

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

Merged
merged 14 commits into from
Jul 27, 2023
15 changes: 15 additions & 0 deletions endpoints/cookie_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -170,6 +177,7 @@ func (c *cookieSyncEndpoint) parseRequest(r *http.Request) (usersync.Request, pr
Privacy: usersyncPrivacy{
gdprPermissions: gdprPerms,
ccpaParsedPolicy: ccpaParsedPolicy,
activityControl: activityControl,
},
SyncTypeFilter: syncTypeFilter,
}
Expand Down Expand Up @@ -499,6 +507,7 @@ type usersyncPrivacyConfig struct {
type usersyncPrivacy struct {
gdprPermissions gdpr.Permissions
ccpaParsedPolicy ccpa.ParsedPolicy
activityControl privacy.ActivityControl
}

func (p usersyncPrivacy) GDPRAllowsHostCookie() bool {
Expand All @@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

}
33 changes: 25 additions & 8 deletions endpoints/setuid.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()))
Expand Down Expand Up @@ -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
Copy link
Collaborator

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.
Should the param passed to metricsEngine.RecordSetUid be one of the account options (metrics.SetUidAccountConfigMalformed or SetUidAccountInvalid) or something more generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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 activities set to nil.

}
}

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
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

}

tcf2Cfg := tcf2CfgBuilder(cfg.GDPR.TCF2, account.GDPR)

if shouldReturn, status, body := preventSyncsGDPR(query.Get("gdpr"), query.Get("gdpr_consent"), gdprPermsBuilder, tcf2Cfg); shouldReturn {
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?


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.
Expand Down
32 changes: 32 additions & 0 deletions endpoints/setuid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,34 @@ func TestSetUIDEndpoint(t *testing.T) {
expectedBody: "account is disabled, please reach out to the prebid server host",
description: "Set uid for valid bidder with valid disabled account provided",
},
{
uri: "/setuid?bidder=pubmatic&uid=123&account=valid_acct_with_valid_activities_usersync_enabled",
syncersBidderNameToKey: map[string]string{"pubmatic": "pubmatic"},
existingSyncs: nil,
gdprAllowsHostCookies: true,
expectedSyncs: map[string]string{"pubmatic": "123"},
expectedStatusCode: http.StatusOK,
expectedHeaders: map[string]string{"Content-Type": "text/html", "Content-Length": "0"},
description: "Set uid for valid bidder with valid account provided with user sync allowed activity",
},
{
uri: "/setuid?bidder=pubmatic&uid=123&account=valid_acct_with_valid_activities_usersync_disabled",
syncersBidderNameToKey: map[string]string{"pubmatic": "pubmatic"},
existingSyncs: nil,
gdprAllowsHostCookies: true,
expectedSyncs: nil,
expectedStatusCode: http.StatusUnavailableForLegalReasons,
description: "Set uid for valid bidder with valid account provided with user sync disallowed activity",
},
{
uri: "/setuid?bidder=pubmatic&uid=123&account=valid_acct_with_invalid_activities",
syncersBidderNameToKey: map[string]string{"pubmatic": "pubmatic"},
existingSyncs: nil,
gdprAllowsHostCookies: true,
expectedSyncs: nil,
expectedStatusCode: http.StatusBadRequest,
description: "Set uid for valid bidder with valid account provided with invalid user sync activity",
},
}

analytics := analyticsConf.NewPBSAnalytics(&config.Analytics{})
Expand Down Expand Up @@ -740,6 +768,10 @@ func doRequest(req *http.Request, analytics analytics.PBSAnalyticsModule, metric
"disabled_acct": json.RawMessage(`{"disabled":true}`),
"malformed_acct": json.RawMessage(`{"disabled":"malformed"}`),
"invalid_json_acct": json.RawMessage(`{"}`),

"valid_acct_with_valid_activities_usersync_enabled": json.RawMessage(`{"privacy":{"allowactivities":{"syncUser":{"default": true}}}}`),
"valid_acct_with_valid_activities_usersync_disabled": json.RawMessage(`{"privacy":{"allowactivities":{"syncUser":{"default": false}}}}`),
"valid_acct_with_invalid_activities": json.RawMessage(`{"privacy":{"allowactivities":{"syncUser":{"rules":[{"condition":{"componentName": ["bidderA.bidderB.bidderC"]}}]}}}}`),
}}

endpoint := NewSetUIDEndpoint(&cfg, syncersByBidder, gdprPermsBuilder, tcf2ConfigBuilder, analytics, fakeAccountsFetcher, metrics)
Expand Down
4 changes: 0 additions & 4 deletions privacy/enforcer.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package privacy
import (
"fmt"
"github.com/prebid/prebid-server/config"
"github.com/prebid/prebid-server/errortypes"
"strings"
)

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

if privacyConf == nil {
return ac, err
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this turned back from nil to err?

} 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)
Expand Down
2 changes: 1 addition & 1 deletion privacy/enforcer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"testing"
)

func TemporarilyDisabledTestNewActivityControl(t *testing.T) {
func TestNewActivityControl(t *testing.T) {

testCases := []struct {
name string
Expand Down
13 changes: 13 additions & 0 deletions usersync/chooser.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
package usersync

import (
privacyActivity "github.com/prebid/prebid-server/privacy"
Copy link
Contributor

Choose a reason for hiding this comment

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

)

// 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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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()}
}
Expand Down
25 changes: 19 additions & 6 deletions usersync/chooser_test.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
package usersync

import (
"testing"
"time"

"github.com/prebid/prebid-server/privacy"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"testing"
"time"
)

func TestNewChooser(t *testing.T) {
Expand Down Expand Up @@ -322,6 +321,15 @@ func TestChooserEvaluate(t *testing.T) {
expectedSyncer: nil,
expectedEvaluation: BidderEvaluation{Bidder: "a", SyncerKey: "keyA", Status: StatusBlockedByCCPA},
},
{
description: "Blocked By activity control",
givenBidder: "a",
givenSyncersSeen: map[string]struct{}{},
givenPrivacy: fakePrivacy{gdprAllowsHostCookie: true, gdprAllowsBidderSync: true, ccpaAllowsBidderSync: true, activityAllowIUserSync: privacy.ActivityDeny},
givenCookie: cookieNeedsSync,
expectedSyncer: nil,
expectedEvaluation: BidderEvaluation{Bidder: "a", SyncerKey: "keyA", Status: StatusBlockedByPrivacy},
},
}

for _, test := range testCases {
Expand Down Expand Up @@ -373,9 +381,10 @@ func (fakeSyncer) GetSync(syncTypes []SyncType, privacyPolicies privacy.Policies
}

type fakePrivacy struct {
gdprAllowsHostCookie bool
gdprAllowsBidderSync bool
ccpaAllowsBidderSync bool
gdprAllowsHostCookie bool
gdprAllowsBidderSync bool
ccpaAllowsBidderSync bool
activityAllowIUserSync privacy.ActivityResult
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 Jul 12, 2023

Choose a reason for hiding this comment

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

This is a typo. Fixed

}

func (p fakePrivacy) GDPRAllowsHostCookie() bool {
Expand All @@ -389,3 +398,7 @@ func (p fakePrivacy) GDPRAllowsBidderSync(bidder string) bool {
func (p fakePrivacy) CCPAAllowsBidderSync(bidder string) bool {
return p.ccpaAllowsBidderSync
}

func (p fakePrivacy) ActivityAllowsUserSync(bidder string) privacy.ActivityResult {
return p.activityAllowIUserSync
}