Handle empty consent string during cookie sync and setuid (prebid#1671)

* Handle empty consent string during cookie sync and setuid

* Remove todo comment

* Make auction test table driven and convert GDPR impl normalize method to pass by value

* Moved legacy auction endpoint signal parsing into its own method and removed unnecessary test cases

* Fix SignalParse method to return nil for error when raw signal is empty and other PR feedback

endpoints/auction.go

@@ -189,21 +189,16 @@ func (a *auction) recoverSafely(inner func(*pbs.PBSBidder, metrics.AdapterLabels
 }
 
 func (a *auction) shouldUsersync(ctx context.Context, bidder openrtb_ext.BidderName, gdprPrivacyPolicy gdprPrivacy.Policy) bool {
-	switch gdprPrivacyPolicy.Signal {
-	case "0":
-		return true
-	case "1":
-		if gdprPrivacyPolicy.Consent == "" {
-			return false
-		}
-		fallthrough
-	default:
-		if canSync, err := a.gdprPerms.HostCookiesAllowed(ctx, gdprPrivacyPolicy.Consent); !canSync || err != nil {
-			return false
-		}
-		canSync, err := a.gdprPerms.BidderSyncAllowed(ctx, bidder, gdprPrivacyPolicy.Consent)
-		return canSync && err == nil
+	gdprSignal := gdpr.SignalAmbiguous
+	if signal, err := gdpr.SignalParse(gdprPrivacyPolicy.Signal); err != nil {
+		gdprSignal = signal
+	}
+
+	if canSync, err := a.gdprPerms.HostCookiesAllowed(ctx, gdprSignal, gdprPrivacyPolicy.Consent); err != nil || !canSync {
+		return false
 	}
+	canSync, err := a.gdprPerms.BidderSyncAllowed(ctx, bidder, gdprSignal, gdprPrivacyPolicy.Consent)
+	return canSync && err == nil
 }
 
 // cache video bids only for Web

endpoints/auction_test.go

@@ -22,6 +22,8 @@ import (
 	gdprPolicy "github.com/prebid/prebid-server/privacy/gdpr"
 	"github.com/prebid/prebid-server/usersync/usersyncers"
 	"github.com/spf13/viper"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestSortBidsAndAddKeywordsForMobile(t *testing.T) {
@@ -376,32 +378,64 @@ func TestCacheVideoOnly(t *testing.T) {
 }
 
 func TestShouldUsersync(t *testing.T) {
-	doTest := func(gdprApplies string, consent string, allowBidderSync bool, allowHostCookies bool, expectAllow bool) {
-		t.Helper()
+	tests := []struct {
+		description      string
+		signal           string
+		allowHostCookies bool
+		allowBidderSync  bool
+		wantAllow        bool
+	}{
+		{
+			description:      "Don't sync - GDPR on, host cookies disallows and bidder sync disallows",
+			signal:           "1",
+			allowHostCookies: false,
+			allowBidderSync:  false,
+			wantAllow:        false,
+		},
+		{
+			description:      "Don't sync - GDPR on, host cookies disallows and bidder sync allows",
+			signal:           "1",
+			allowHostCookies: false,
+			allowBidderSync:  true,
+			wantAllow:        false,
+		},
+		{
+			description:      "Don't sync - GDPR on, host cookies allows and bidder sync disallows",
+			signal:           "1",
+			allowHostCookies: true,
+			allowBidderSync:  false,
+			wantAllow:        false,
+		},
+		{
+			description:      "Sync - GDPR on, host cookies allows and bidder sync allows",
+			signal:           "1",
+			allowHostCookies: true,
+			allowBidderSync:  true,
+			wantAllow:        true,
+		},
+		{
+			description:      "Don't sync - invalid GDPR signal, host cookies disallows and bidder sync disallows",
+			signal:           "2",
+			allowHostCookies: false,
+			allowBidderSync:  false,
+			wantAllow:        false,
+		},
+	}
+
+	for _, tt := range tests {
 		deps := auction{
-			cfg:     nil,
-			syncers: nil,
 			gdprPerms: &auctionMockPermissions{
-				allowBidderSync:  allowBidderSync,
-				allowHostCookies: allowHostCookies,
+				allowBidderSync:  tt.allowBidderSync,
+				allowHostCookies: tt.allowHostCookies,
 			},
-			metricsEngine: nil,
 		}
 		gdprPrivacyPolicy := gdprPolicy.Policy{
-			Signal:  gdprApplies,
-			Consent: consent,
+			Signal: tt.signal,
 		}
 
-		allowSyncs := deps.shouldUsersync(context.Background(), openrtb_ext.BidderAdform, gdprPrivacyPolicy)
-		if allowSyncs != expectAllow {
-			t.Errorf("Expected syncs: %t, allowed syncs: %t", expectAllow, allowSyncs)
-		}
+		allow := deps.shouldUsersync(context.Background(), openrtb_ext.BidderAdform, gdprPrivacyPolicy)
+		assert.Equal(t, tt.wantAllow, allow, tt.description)
 	}
-	doTest("0", "", false, false, true)
-	doTest("1", "", true, true, false)
-	doTest("1", "a", true, false, false)
-	doTest("1", "a", false, true, false)
-	doTest("1", "a", true, true, true)
 }
 
 type auctionMockPermissions struct {
@@ -412,11 +446,11 @@ type auctionMockPermissions struct {
 	allowID          bool
 }
 
-func (m *auctionMockPermissions) HostCookiesAllowed(ctx context.Context, consent string) (bool, error) {
+func (m *auctionMockPermissions) HostCookiesAllowed(ctx context.Context, gdprSignal gdpr.Signal, consent string) (bool, error) {
 	return m.allowHostCookies, nil
 }
 
-func (m *auctionMockPermissions) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, consent string) (bool, error) {
+func (m *auctionMockPermissions) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, gdprSignal gdpr.Signal, consent string) (bool, error) {
 	return m.allowBidderSync, nil
 }
 

endpoints/cookie_sync.go

@@ -248,13 +248,14 @@ func (req *cookieSyncRequest) filterForGDPR(permissions gdpr.Permissions) {
 		return
 	}
 
-	if allowSync, err := permissions.HostCookiesAllowed(context.Background(), req.Consent); err != nil || !allowSync {
+	// At this point we know the gdpr signal is Yes because the upstream call to parseRequest already denormalized the signal if it was ambiguous
+	if allowSync, err := permissions.HostCookiesAllowed(context.Background(), gdpr.SignalYes, req.Consent); err != nil || !allowSync {
 		req.Bidders = nil
 		return
 	}
 
 	for i := 0; i < len(req.Bidders); i++ {
-		if allowSync, err := permissions.BidderSyncAllowed(context.Background(), openrtb_ext.BidderName(req.Bidders[i]), req.Consent); err != nil || !allowSync {
+		if allowSync, err := permissions.BidderSyncAllowed(context.Background(), openrtb_ext.BidderName(req.Bidders[i]), gdpr.SignalYes, req.Consent); err != nil || !allowSync {
 			req.Bidders = append(req.Bidders[:i], req.Bidders[i+1:]...)
 			i--
 		}

endpoints/cookie_sync_test.go

@@ -245,11 +245,11 @@ type gdprPerms struct {
 	allowedBidders map[openrtb_ext.BidderName]usersync.Usersyncer
 }
 
-func (g *gdprPerms) HostCookiesAllowed(ctx context.Context, consent string) (bool, error) {
+func (g *gdprPerms) HostCookiesAllowed(ctx context.Context, gdprSignal gdpr.Signal, consent string) (bool, error) {
 	return g.allowHost, nil
 }
 
-func (g *gdprPerms) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, consent string) (bool, error) {
+func (g *gdprPerms) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, gdprSignal gdpr.Signal, consent string) (bool, error) {
 	_, ok := g.allowedBidders[bidder]
 	return ok, nil
 }

endpoints/setuid.go

@@ -143,32 +143,38 @@ func checkChromeBrowserVersion(ua string, index int, chromeStrLength int) bool {
 	return result
 }
 
-func preventSyncsGDPR(gdprEnabled string, gdprConsent string, perms gdpr.Permissions) (bool, int, string) {
-	switch gdprEnabled {
-	case "0":
-		return false, 0, ""
-	case "1":
-		if gdprConsent == "" {
-			return true, http.StatusBadRequest, "gdpr_consent is required when gdpr=1"
-		}
-		fallthrough
-	case "":
-		if allowed, err := perms.HostCookiesAllowed(context.Background(), gdprConsent); err != nil {
-			if _, ok := err.(*gdpr.ErrorMalformedConsent); ok {
-				return true, http.StatusBadRequest, "gdpr_consent was invalid. " + err.Error()
-			} else {
-				// We can't really distinguish between requests that are for a new version of the global vendor list, and
-				// ones which are simply malformed (version number is much too large).
-				// Since we try to fetch new versions as requests come in for them, PBS *should* self-correct
-				// rather quickly, meaning that most of these will be malformed strings.
-				return true, http.StatusBadRequest, "No global vendor list was available to interpret this consent string. If this is a new, valid version, it should become available soon."
-			}
-		} else if !allowed {
-			return true, http.StatusOK, "The gdpr_consent string prevents cookies from being saved"
-		} else {
-			return false, 0, ""
-		}
-	default:
+func preventSyncsGDPR(gdprEnabled string, gdprConsent string, perms gdpr.Permissions) (shouldReturn bool, status int, body string) {
+
+	if gdprEnabled != "" && gdprEnabled != "0" && gdprEnabled != "1" {
 		return true, http.StatusBadRequest, "the gdpr query param must be either 0 or 1. You gave " + gdprEnabled
 	}
+
+	if gdprEnabled == "1" && gdprConsent == "" {
+		return true, http.StatusBadRequest, "gdpr_consent is required when gdpr=1"
+	}
+
+	gdprSignal := gdpr.SignalAmbiguous
+
+	if i, err := strconv.Atoi(gdprEnabled); err == nil {
+		gdprSignal = gdpr.Signal(i)
+	}
+
+	allowed, err := perms.HostCookiesAllowed(context.Background(), gdprSignal, gdprConsent)
+	if err != nil {
+		if _, ok := err.(*gdpr.ErrorMalformedConsent); ok {
+			return true, http.StatusBadRequest, "gdpr_consent was invalid. " + err.Error()
+		}
+
+		// We can't really distinguish between requests that are for a new version of the global vendor list, and
+		// ones which are simply malformed (version number is much too large).
+		// Since we try to fetch new versions as requests come in for them, PBS *should* self-correct
+		// rather quickly, meaning that most of these will be malformed strings.
+		return true, http.StatusBadRequest, "No global vendor list was available to interpret this consent string. If this is a new, valid version, it should become available soon."
+	}
+
+	if allowed {
+		return false, 0, ""
+	}
+
+	return true, http.StatusOK, "The gdpr_consent string prevents cookies from being saved"
 }

endpoints/setuid_test.go

@@ -101,12 +101,13 @@ func TestSetUIDEndpoint(t *testing.T) {
 			description:           "Add the uid for the requested bidder to the list of existing syncs",
 		},
 		{
-			uri:                  "/setuid?bidder=pubmatic&uid=123&gdpr=0",
-			validFamilyNames:     []string{"pubmatic"},
-			existingSyncs:        nil,
-			expectedSyncs:        map[string]string{"pubmatic": "123"},
-			expectedResponseCode: http.StatusOK,
-			description:          "Don't care about GDPR consent if GDPR is set to 0",
+			uri:                   "/setuid?bidder=pubmatic&uid=123&gdpr=0",
+			validFamilyNames:      []string{"pubmatic"},
+			existingSyncs:         nil,
+			gdprAllowsHostCookies: true,
+			expectedSyncs:         map[string]string{"pubmatic": "123"},
+			expectedResponseCode:  http.StatusOK,
+			description:           "Don't care about GDPR consent if GDPR is set to 0",
 		},
 		{
 			uri:                  "/setuid?bidder=pubmatic&uid=123",
@@ -426,15 +427,15 @@ type mockPermsSetUID struct {
 	allowPI   bool
 }
 
-func (g *mockPermsSetUID) HostCookiesAllowed(ctx context.Context, consent string) (bool, error) {
+func (g *mockPermsSetUID) HostCookiesAllowed(ctx context.Context, gdprSignal gdpr.Signal, consent string) (bool, error) {
 	var err error
 	if g.errorHost {
 		err = errors.New("something went wrong")
 	}
 	return g.allowHost, err
 }
 
-func (g *mockPermsSetUID) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, consent string) (bool, error) {
+func (g *mockPermsSetUID) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, gdprSignal gdpr.Signal, consent string) (bool, error) {
 	return false, nil
 }
 

exchange/utils_test.go

@@ -24,11 +24,11 @@ type permissionsMock struct {
 	personalInfoAllowedError error
 }
 
-func (p *permissionsMock) HostCookiesAllowed(ctx context.Context, consent string) (bool, error) {
+func (p *permissionsMock) HostCookiesAllowed(ctx context.Context, gdpr gdpr.Signal, consent string) (bool, error) {
 	return true, nil
 }
 
-func (p *permissionsMock) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, consent string) (bool, error) {
+func (p *permissionsMock) BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, gdpr gdpr.Signal, consent string) (bool, error) {
 	return true, nil
 }
 

gdpr/gdpr.go

@@ -3,22 +3,24 @@ package gdpr
 import (
 	"context"
 	"net/http"
+	"strconv"
 
 	"github.com/prebid/go-gdpr/vendorlist"
 	"github.com/prebid/prebid-server/config"
+	"github.com/prebid/prebid-server/errortypes"
 	"github.com/prebid/prebid-server/openrtb_ext"
 )
 
 type Permissions interface {
 	// Determines whether or not the host company is allowed to read/write cookies.
 	//
 	// If the consent string was nonsensical, the returned error will be an ErrorMalformedConsent.
-	HostCookiesAllowed(ctx context.Context, consent string) (bool, error)
+	HostCookiesAllowed(ctx context.Context, gdprSignal Signal, consent string) (bool, error)
 
 	// Determines whether or not the given bidder is allowed to user personal info for ad targeting.
 	//
 	// If the consent string was nonsensical, the returned error will be an ErrorMalformedConsent.
-	BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, consent string) (bool, error)
+	BidderSyncAllowed(ctx context.Context, bidder openrtb_ext.BidderName, gdprSignal Signal, consent string) (bool, error)
 
 	// Determines whether or not to send PI information to a bidder, or mask it out.
 	//
@@ -65,3 +67,21 @@ type ErrorMalformedConsent struct {
 func (e *ErrorMalformedConsent) Error() string {
 	return "malformed consent string " + e.consent + ": " + e.cause.Error()
 }
+
+// SignalParse parses a raw signal and returns
+func SignalParse(rawSignal string) (Signal, error) {
+	if rawSignal == "" {
+		return SignalAmbiguous, nil
+	}
+
+	i, err := strconv.Atoi(rawSignal)
+
+	if err != nil {
+		return SignalAmbiguous, err
+	}
+	if i != 0 && i != 1 {
+		return SignalAmbiguous, &errortypes.BadInput{Message: "GDPR signal should be integer 0 or 1"}
+	}
+
+	return Signal(i), nil
+}

gdpr/gdpr_test.go

@@ -48,3 +48,55 @@ func TestNewPermissions(t *testing.T) {
 		assert.IsType(t, tt.wantType, perms, tt.description)
 	}
 }
+
+func TestSignalParse(t *testing.T) {
+	tests := []struct {
+		description string
+		rawSignal   string
+		wantSignal  Signal
+		wantError   bool
+	}{
+		{
+			description: "valid raw signal is 0",
+			rawSignal:   "0",
+			wantSignal:  SignalNo,
+			wantError:   false,
+		},
+		{
+			description: "Valid signal - raw signal is 1",
+			rawSignal:   "1",
+			wantSignal:  SignalYes,
+			wantError:   false,
+		},
+		{
+			description: "Valid signal - raw signal is empty",
+			rawSignal:   "",
+			wantSignal:  SignalAmbiguous,
+			wantError:   false,
+		},
+		{
+			description: "Invalid signal - raw signal is -1",
+			rawSignal:   "-1",
+			wantSignal:  SignalAmbiguous,
+			wantError:   true,
+		},
+		{
+			description: "Invalid signal - raw signal is abc",
+			rawSignal:   "abc",
+			wantSignal:  SignalAmbiguous,
+			wantError:   true,
+		},
+	}
+
+	for _, tt := range tests {
+		signal, err := SignalParse(tt.rawSignal)
+
+		assert.Equal(t, tt.wantSignal, signal, tt.description)
+
+		if tt.wantError {
+			assert.NotNil(t, err, tt.description)
+		} else {
+			assert.Nil(t, err, tt.description)
+		}
+	}
+}