Skip to content

CCPA Phase 2: Enforcement #1138

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Dec 16, 2019
Merged

CCPA Phase 2: Enforcement #1138

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ace7f43
Implement CCPA For Auctions
SyntaxNode Dec 10, 2019
534c39a
Added CCPA Enforce Flag
SyntaxNode Dec 11, 2019
a219ded
Merge branch 'master' into ccpa_enforce
SyntaxNode Dec 11, 2019
a237cf7
Ignore (With Warning) Invalid CCPA Values
SyntaxNode Dec 11, 2019
675efaf
Tweaked Name Of Test JSON
SyntaxNode Dec 11, 2019
7e25811
PR Feedback
SyntaxNode Dec 12, 2019
75179b3
Removed Unused Method
SyntaxNode Dec 12, 2019
c810703
Enforce CCPA For Cookie Sync
SyntaxNode Dec 13, 2019
6b43612
Case Sensitivity Is Intended Per Spec
SyntaxNode Dec 13, 2019
f074e60
Added Feature Flag Test
SyntaxNode Dec 13, 2019
db833a1
Expanded Tests + Comments
SyntaxNode Dec 13, 2019
44da58e
Formatting
SyntaxNode Dec 13, 2019
dc94e39
Throwing In A Small TL Update
SyntaxNode Dec 13, 2019
40bcddc
Added Exchange Tests
SyntaxNode Dec 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ type Configuration struct {
Analytics Analytics `mapstructure:"analytics"`
AMPTimeoutAdjustment int64 `mapstructure:"amp_timeout_adjustment_ms"`
GDPR GDPR `mapstructure:"gdpr"`
CCPA CCPA `mapstructure:"ccpa"`
CurrencyConverter CurrencyConverter `mapstructure:"currency_converter"`
Copy link
Contributor

Choose a reason for hiding this comment

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

CCPA is a California State Bill, GDPR is a European law, what if other Privacy laws start getting enforced elsewhere like in Asia, Latin America or even another State of the Union? Do you think it makes more sense to put all of these privacy data structure under a single struct? We could call it PrivacyEnforcement, PrivacyConfig, or something else.

 21    type Configuration struct {
 22        ExternalURL string     `mapstructure:"external_url"`
 23        Host        string     `mapstructure:"host"`
 24        Port        int        `mapstructure:"port"`
 25        Client      HTTPClient `mapstructure:"http_client"`
 26    -- 20 lines: AdminPort   int        `mapstructure:"admin_port"`-------------------------------------
 46        MaxRequestSize       int64              `mapstructure:"max_request_size"`
 47        Analytics            Analytics          `mapstructure:"analytics"`
 48        AMPTimeoutAdjustment int64              `mapstructure:"amp_timeout_adjustment_ms"`
 49 -      GDPR                 GDPR               `mapstructure:"gdpr"`
 50 -      CCPA                 CCPA               `mapstructure:"ccpa"`
    +      PrivacyConfig        PrivacyEnforcement `mapstructure:"privacy_info"`
 51        CurrencyConverter    CurrencyConverter  `mapstructure:"currency_converter"`
 52        DefReqConfig         DefReqConfig       `mapstructure:"default_request"`
 53
 54        VideoStoredRequestRequired bool `mapstructure:"video_stored_request_required"`
 55
 56    --  7 lines: Array of blacklisted apps that is used to create the hash table BlacklistedAppMap so Ap
 63        AccountRequired bool `mapstructure:"account_required"`
 64        // Local private file containing SSL certificates
 65        PemCertsFile string `mapstructure:"certificates_file"`
 66    }
    +  type PrivacyEnforcement struct {
    +      GDPR                 GDPR               `mapstructure:"gdpr"`
    +      CCPA                 CCPA               `mapstructure:"ccpa"`
    +  }
config/config.go [RO]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you on establishing a more common approach. This PR expands on the new privacy package to do just that. This here is just a temporary app config until Prebid.org devises an official enforcement stance.

DefReqConfig DefReqConfig `mapstructure:"default_request"`

Expand Down Expand Up @@ -160,6 +161,10 @@ func (t *GDPRTimeouts) ActiveTimeout() time.Duration {
return time.Duration(t.ActiveVendorlistFetch) * time.Millisecond
}

type CCPA struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: This looks to be a long term option, with additional config coming in future PRs. You may want to re-review in that context. I don't want to merge GDPR and CCPA in the config object because I don't want to break existing GDPR config.

Enforce bool `mapstructure:"enforce"`
}

type Analytics struct {
File FileLogs `mapstructure:"file"`
}
Expand Down Expand Up @@ -706,6 +711,7 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("gdpr.timeouts_ms.init_vendorlist_fetches", 0)
v.SetDefault("gdpr.timeouts_ms.active_vendorlist_fetch", 0)
v.SetDefault("gdpr.non_standard_publishers", []string{""})
v.SetDefault("ccpa.enforce", false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default to disabled for now. Will change in the future when the Prebid.org CCPA enforcement spec is finalized.

v.SetDefault("currency_converter.fetch_url", "https://cdn.jsdelivr.net/gh/prebid/currency-file@1/latest.json")
v.SetDefault("currency_converter.fetch_interval_seconds", 1800) // fetch currency rates every 30 minutes
v.SetDefault("default_request.type", "")
Expand Down
7 changes: 2 additions & 5 deletions endpoints/openrtb2/amp_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,17 +382,14 @@ func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *ope
req.Imp[0].TagID = slot
}

gdprConsent := getQueryParam(httpRequest, "gdpr_consent")
ccpaValue := getQueryParam(httpRequest, "us_privacy")
privacyPolicies := privacy.Policies{
GDPR: gdpr.Policy{
Consent: gdprConsent,
Consent: getQueryParam(httpRequest, "gdpr_consent"),
},
CCPA: ccpa.Policy{
Value: ccpaValue,
Value: getQueryParam(httpRequest, "us_privacy"),
},
}

if err := privacyPolicies.Write(req); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The following was tested in the Go playground:

package main

import (
	"fmt"
	"log"
	"net/url"
)

func main() {
	u, err := url.Parse("http://bing.com/search?q=dotnet")
	if err != nil {
		log.Fatal(err)
	}
	
	fmt.Printf("Value of Get(\"gdpr_consent\") = \"%s\" \n",u.Query().Get("gdpr_consent"))
	fmt.Printf("Lenght of Get(\"gdpr_consent\") = %d \n",len(u.Query().Get("gdpr_consent")))
	fmt.Printf("Value of Get(\"q\") = \"%s\" \n",u.Query().Get("q"))
}

Output is:

Value of Get("gdpr_consent") = "" 
Lenght of Get("gdpr_consent") = 0 
Value of Get("q") = "dotnet"

Therefore, is fair to assume Query().Get("gdpr_consent") is safe to use and we could spare maintaining one less function

346   func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *openrtb.BidRequest) error {
347       if req.Site == nil {
348           req.Site = &openrtb.Site{}
349       }
350   --- 34 lines: Override the stored request sizes with AMP ones, if they exist.-----------------------------------------------------------------------------------------------------------
384
385       privacyPolicies := privacy.Policies{
386           GDPR: gdpr.Policy{
387 -             Consent: getQueryParam(httpRequest, "gdpr_consent"),
    +             Consent: httpRequest.URL.Query().Get("gdpr_consent"), //No nil pointer dereference because Query returns a non-nil map
388           },
389           CCPA: ccpa.Policy{
390 -             Value: getQueryParam(httpRequest, "us_privacy"),
    +             Value: httpRequest.URL.Query().Get("us_privacy"),
391           },
392       }
393       if err := privacyPolicies.Write(req); err != nil {
394           return err
395       }
396
397   ---  3 lines: if timeout, err := strconv.ParseInt(httpRequest.FormValue("timeout"), 10, 64); err == nil {-------------------------------------------------------------------------------
400
401       return nil
402   }
403
404 - func getQueryParam(httpRequest *http.Request, name string) string {
405 -     values, ok := httpRequest.URL.Query()[name]
406 -
407 -     if !ok || len(values) == 0 {
408 -         return ""
409 -     }
410 -
411 -     // return first value of the query param, matching the behavior of httpRequest.FormValue
412 -     return values[0]
413 - }
414
endpoints/openrtb2/amp_auction.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much. Changed.

return err
}
Expand Down
12 changes: 12 additions & 0 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/pbsmetrics"
"github.com/prebid/prebid-server/prebid"
"github.com/prebid/prebid-server/privacy/ccpa"
"github.com/prebid/prebid-server/stored_requests"
"github.com/prebid/prebid-server/stored_requests/backends/empty_fetcher"
"github.com/prebid/prebid-server/usersync"
Expand Down Expand Up @@ -301,6 +302,17 @@ func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
return errL
}

ccpaPolicy, ccpaPolicyErr := ccpa.ReadPolicy(req)
if ccpaPolicyErr != nil {
errL = append(errL, ccpaPolicyErr)
return errL
}

if err := ccpaPolicy.Validate(); err != nil {
Copy link
Contributor Author

@SyntaxNode SyntaxNode Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Per the guidance I have so far, an invalid CCPA value can be ignored but I think a warning would be useful. Right now, warnings are not included in the bid response, but I view that as a separate issue.

errL = append(errL, err)
return errL
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ReadPolicy(req *openrtb.BidRequest) unnecessarily checks Regs and Regs.Ext for a second time so, given that validateRegs(req.Regs) already does it in line 300, can we validate CCPA in there too? We could probably spare maintaining an extra function if we just call policy.Validate () and check for errors from inside validateRegs(req.Regs) on the already unmarshalled ext object.

 246   func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
 247       errL := []error{}
 248       if req.ID == "" {
 249           return []error{errors.New("request missing required field: \"id\"")}
 250       }
 251   --- 48 lines: if req.TMax < 0 {-----------------------------------------------
 299
 300       if err := validateRegs(req.Regs); err != nil {
 301           errL = append(errL, err)
 302           return errL
 303       }
 304
 305 -     ccpaPolicy, ccpaPolicyErr := ccpa.ReadPolicy(req)
 306 -     if ccpaPolicyErr != nil {
 307 -         errL = append(errL, ccpaPolicyErr)
 308 -         return errL
 309 -     }
 310 -
 311 -     if err := ccpaPolicy.Validate(); err != nil {
 312 -         errL = append(errL, err)
 313 -         return errL
 314 -     }
 315
 316       impIDs := make(map[string]int, len(req.Imp))
 317       for index := range req.Imp {
 318   --- 12 lines: imp := &req.Imp[index]------------------------------------------
 330       }
 331
 332       return errL
 333   } 
 334
 335   ---558 lines: func validateBidAdjustmentFactors(adjustmentFactors map[string--
 893
 894   func validateRegs(regs *openrtb.Regs) (openrtb_ext.ExtRegs,error) {
 895       if regs != nil && len(regs.Ext) > 0 {
 896           var regsExt openrtb_ext.ExtRegs
 897           if err := json.Unmarshal(regs.Ext, &regsExt); err != nil {
 898               return fmt.Errorf("request.regs.ext is invalid: %v", err)
 899           }
 900           if regsExt.GDPR != nil && (*regsExt.GDPR < 0 || *regsExt.GDPR > 1) {
 901               return errors.New("request.regs.ext.gdpr must be either 0 or 1.")
 902           }
 903 +         ccpaPolicy := Policy{Value = regsExt.USPrivacy}
 904 +         if err := ccpaPolicy.Validate(); err != nil {
 905 +             return err
 906 +         }
 907       }
 908       return nil
 909   }
endpoints/openrtb2/auction.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I consider this part of 'transition pain'. I'm trying to keep all privacy logic in each respective privacy policy and will move the GDPR and COPPA logic there too, but that's a bit too much for this PR. Do you think this will cause a major performance issue? I could restructure a few things for now to avoid the extra unmarshal if you feel it's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'll think it'd be perfectly fine to refactor in another PR latter

Copy link
Contributor

Choose a reason for hiding this comment

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

image
Can we write a test case to cover ReadPolicy(req) error scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. That code isn't reachable. It will only get triggered if the regs.ext is malformed and there' s already a check for that earlier in the func. File this under pain of a partial refactor of privacy things. More to come soon.

impIDs := make(map[string]int, len(req.Imp))
for index := range req.Imp {
imp := &req.Imp[index]
Expand Down
47 changes: 47 additions & 0 deletions endpoints/openrtb2/sample-requests/invalid-whole/regs-ext-usprivacy-invalid.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"message": "Invalid request: request.regs.ext.us_privacy must contain 4 characters\n",
"requestPayload": {
"id": "b9c97a4b-cbc4-483d-b2c4-58a19ed5cfc5",
"site": {
"page": "prebid.org",
"publisher": {
"id": "a3de7af2-a86a-4043-a77b-c7e86744155e"
}
},
"source": {
"tid": "b9c97a4b-cbc4-483d-b2c4-58a19ed5cfc5"
},
"tmax": 1000,
"imp": [
{
"id": "/19968336/header-bid-tag-0",
"ext": {
"appnexus": {
"placementId": 10433394
}
},
"banner": {
"format": [
{
"w": 300,
"h": 250
},
{
"w": 300,
"h": 300
}
]
}
}
],
"regs": {
"ext": {
"us_privacy": "invalid by length"
}
},
"user": {
"ext": {}
}
}
}

45 changes: 45 additions & 0 deletions endpoints/openrtb2/sample-requests/invalid-whole/regs-ext-usprivacy-malformed.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{
"message": "Invalid request: request.regs.ext is invalid: json: cannot unmarshal string into Go value of type openrtb_ext.ExtRegs\n",
"requestPayload": {
"id": "b9c97a4b-cbc4-483d-b2c4-58a19ed5cfc5",
"site": {
"page": "prebid.org",
"publisher": {
"id": "a3de7af2-a86a-4043-a77b-c7e86744155e"
}
},
"source": {
"tid": "b9c97a4b-cbc4-483d-b2c4-58a19ed5cfc5"
},
"tmax": 1000,
"imp": [
{
"id": "/19968336/header-bid-tag-0",
"ext": {
"appnexus": {
"placementId": 10433394
}
},
"banner": {
"format": [
{
"w": 300,
"h": 250
},
{
"w": 300,
"h": 300
}
]
}
}
],
"regs": {
"ext": "malformed"
},
"user": {
"ext": {}
}
}
}

3 changes: 2 additions & 1 deletion endpoints/openrtb2/sample-requests/valid-whole/exemplary/all-ext.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
},
"regs": {
"ext": {
"gdpr": 1
"gdpr": 1,
"us_privacy": "1NYN"
}
},
"imp": [
Expand Down
41 changes: 41 additions & 0 deletions endpoints/openrtb2/sample-requests/valid-whole/supplementary/ccpa.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"id": "b9c97a4b-cbc4-483d-b2c4-58a19ed5cfc5",
"site": {
"page": "prebid.org",
"publisher": {
"id": "a3de7af2-a86a-4043-a77b-c7e86744155e"
}
},
"source": {
"tid": "b9c97a4b-cbc4-483d-b2c4-58a19ed5cfc5"
},
"tmax": 1000,
"imp": [
{
"id": "/19968336/header-bid-tag-0",
"ext": {
"appnexus": {
"placementId": 10433394
}
},
"banner": {
"format": [
{
"w": 300,
"h": 250
},
{
"w": 300,
"h": 300
}
]
}
}
],
"regs": {
"ext": {
"us_privacy": "1NYN"
}
}
}

4 changes: 3 additions & 1 deletion exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ type exchange struct {
currencyConverter *currencies.RateConverter
UsersyncIfAmbiguous bool
defaultTTLs config.DefaultTTLs
enforceCCPA bool
}

// Container to pass out response ext data from the GetAllBids goroutines back into the main thread
Expand All @@ -71,6 +72,7 @@ func NewExchange(client *http.Client, cache prebid_cache_client.Client, cfg *con
e.currencyConverter = currencyConverter
e.UsersyncIfAmbiguous = cfg.GDPR.UsersyncIfAmbiguous
e.defaultTTLs = cfg.CacheURL.DefaultTTLs
e.enforceCCPA = cfg.CCPA.Enforce
return e
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this change would imply a thorough refactoring effort that is out of the scope of this PR so we don't have to do it right now, but I believe we could be better off if we consolidate all the privacy enforcement in a single class with its own fields and methods

 40   type exchange struct {
 41       adapterMap          map[openrtb_ext.BidderName]adaptedBidder
 42       me                  pbsmetrics.MetricsEngine
 43       cache               prebid_cache_client.Client
 44       cacheTime           time.Duration
 45 -     gDPR                gdpr.Permissions
    +     privacyPolicies     policiesAndPermissions
 46       currencyConverter   *currencies.RateConverter
 47       UsersyncIfAmbiguous bool
 48       defaultTTLs         config.DefaultTTLs
 49 -     enforceCCPA         bool
 50   }
 51
 52   --- 11 lines: Container to pass out response ext data from the GetAllBids goroutines back into the main thread-----------
 63
 64   func NewExchange(client *http.Client, cache prebid_cache_client.Client, cfg *config.Configuration, metricsEngine pbsmetrics.MetricsEngine, infos adapters.BidderInfos, gDPR gdpr.Permis>
 65       e := new(exchange)
 66
 67       e.adapterMap = newAdapterMap(client, cfg, infos)
 68       e.cache = cache
 69       e.cacheTime = time.Duration(cfg.CacheURL.ExpectedTimeMillis) * time.Millisecond
 70       e.me = metricsEngine
 71 -     e.gDPR = gDPR
    +     e.privacyPolicies = &policiesAndPermissions{GDPRPermissions:GDPR,CCPA:&policy.CCPA{enforceCCPA:cfg.CCPA.Enforce}
 72       e.currencyConverter = currencyConverter
 73       e.UsersyncIfAmbiguous = cfg.GDPR.UsersyncIfAmbiguous
 74       e.defaultTTLs = cfg.CacheURL.DefaultTTLs
 75 -     e.enforceCCPA = cfg.CCPA.Enforce
 76       return e
 77   }
exchange/exchange.go [RO]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly the goal I'm working towards. Baby steps.

}

Expand All @@ -93,7 +95,7 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque

// Slice of BidRequests, each a copy of the original cleaned to only contain bidder data for the named bidder
blabels := make(map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels)
cleanRequests, aliases, errs := cleanOpenRTBRequests(ctx, bidRequest, usersyncs, blabels, labels, e.gDPR, e.UsersyncIfAmbiguous)
cleanRequests, aliases, errs := cleanOpenRTBRequests(ctx, bidRequest, usersyncs, blabels, labels, e.gDPR, e.UsersyncIfAmbiguous, e.enforceCCPA)

// List of bidders we have requests for.
liveAdapters := listBiddersWithRequests(cleanRequests)
Expand Down
Loading