GPP in the setuid endpoint #2761

guscarreon · 2023-05-09T21:23:39Z

Requires #2756

This pull request is a continuation of GPP Phase 2, more info in this document and issue #2380. Makes the /setuid endpoint handler to prioritize the GPP string values, parsed with the go-gpp library, over the GDPR param values it currently takes into account.

bsardo · 2023-05-31T15:19:44Z

endpoints/setuid.go

+		if gdprQuerySignal == "1" && gdprConsent == "" {
+			return gdpr.RequestInfo{}, errors.New("gdpr_consent is required when gdpr=1")
+		}
+		if i, err := strconv.Atoi(gdprQuerySignal); err == nil {


If the GDPR signal was not present in the GPP query params, you're looking at the legacy GDPR query params. If gdprQuerySignal was not specified, we should be defaulting gdprSignal to SignalAmbiguous as you'll see in the code you deleted below. The perms object instantiated via permsBuilder in preventSyncsGDPR will properly handle the ambiguous signal.

You're completely right. Made major corrections to this function in the next commit.

bsardo · 2023-05-31T15:28:05Z

endpoints/setuid.go

+	if len(gppSID) > 0 && gppPrivacy.IsSIDInList(gppSID, gppConstants.SectionTCFEU2) {
+		gdprSignal = gdpr.SignalYes
+		signalSet = true
+	}


if the length of gppSID is greater than 0 but gppConstants.SectionTCFEU2 is not present in gppSID, should we be treating that as a SignalNo and not bother to consult the legacy query params? It looks like that is what we're doing in the cookie sync endpoint.

Good catch. Logic was corrected

endpoints/setuid.go

AlexBVolcy

Looks good, just one minor comment

AlexBVolcy · 2023-06-13T18:57:22Z

endpoints/setuid_test.go

+		expected testOutput
+	}
+	testSuite := []struct {
+		sDesc string


Is this sDesc value used anywhere in these tests? If not, could it be removed?

Corrected in df562e9

bsardo · 2023-06-15T14:54:09Z

endpoints/setuid.go

From looking at the requirements doc referenced in the description, it appears we aren't detecting or providing any debug information indicating mismatched data when the gdpr signal is set in the GPPSID and in the original location at regs.gdpr. This also appears to be true with respect to the consent strings. Was this intentionally omitted? I see that detected mismatches are meant to be reported as debug warnings, however, at first glance I'm not seeing support for debug mode on this endpoint so perhaps that's the reason we aren't handling it?

Discussed offline. There's no need to add debug functionality to these endpoints. The setuid and cookie sync endpoints currently support adding warnings to the response. This PR adds the appropriate warnings for the setuid endpoint. The cookie sync warnings will be added in another PR.

bsardo · 2023-06-15T15:08:33Z

endpoints/setuid.go

+		}
+
+		if i := gppPrivacy.IndexOfSID(gpp, gppConstants.SectionTCFEU2); i >= 0 {
+			gdprConsent = gpp.Sections[i].GetValue()


The requirements for the cookie sync and setuid endpoints state that if the gdpr consent string is provided in the GPP container and also in the query string, we just compare and warn without copying. That would mean the query string value takes precedence. I wasn't sure if that was the intention but after looking at the same requirements for the auction endpoint I see this same compare requirement but with the following note right after it: "Prebid should prefer the string originally in user.consent". We should confirm the intended behavior. and if the logic is similar for the gdpr flag.

Discussed offline. The requirements do not appear to be up to date. It was agreed upon that we should always favor the GPP values over the legacy values.

An edge case was identified that does not appear to be called out in the spec. If both GPP and the legacy location contain consent strings, we will always favor the GPP string, even if it is malformed. For now we will report the error found in the GPP string rather than falling back to the legacy location. This can always be added later. We can discuss with the committee.

AlexBVolcy · 2023-06-27T22:07:01Z

endpoints/cookie_sync.go

-		}
-		return gdpr.SignalNo, strconv.Itoa(int(gdpr.SignalNo)), nil
+func extractGDPRSignal(requestGDPR *int, gppSidStr string) (gdpr.Signal, string, error) {
+


Nitpick: Could you remove this blank line?

Changes in this file were discarded

AlexBVolcy · 2023-06-27T22:09:43Z

endpoints/setuid.go

@@ -154,6 +158,7 @@ func NewSetUIDEndpoint(cfg *config.Configuration, syncersByBidder map[string]use
 			w.Header().Add("Content-Type", httputil.Pixel1x1PNG.ContentType)
 			w.Header().Add("Content-Length", strconv.Itoa(len(httputil.Pixel1x1PNG.Content)))
 			w.WriteHeader(http.StatusOK)
+			// signal, consent, error := parseGDPRFromGPP(query)


Can this be deleted?

AlexBVolcy · 2023-06-27T22:10:53Z

endpoints/setuid.go

-		if err != nil {
+
+		if !errortypes.IsWarning(err) && err != nil {
+			//if err != nil {


Can this be deleted?

AlexBVolcy · 2023-06-27T22:11:08Z

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: Can this line be deleted?

AlexBVolcy · 2023-06-27T22:11:21Z

endpoints/setuid.go

+	}
+
+	// If warning, or there was no GDPR data in the GPP wrapper
+	//if (err != nil && isWarning) || (reqInfo.Consent == "" && reqInfo.GDPRSignal == gdpr.SignalAmbiguous) {


Can this comment be deleted?

AlexBVolcy · 2023-06-27T22:21:56Z

endpoints/setuid.go

+
+	legacySignal, legacyConsent, err := parseLegacyGDPRFields(query, reqInfo.GDPRSignal, reqInfo.Consent)
+	isWarning := errortypes.IsWarning(err)
+	// If not warning, throw error


I think this comment can be deleted since the flow of the code explains what it's doing nicely.

AlexBVolcy

LGTM

hhhjort · 2023-07-05T19:47:31Z

endpoints/setuid.go

+				return gdpr.SignalAmbiguous, "", errors.New("the gdpr query param must be either 0 or 1. You gave " + gdprQuerySignal)
+			}
+
+			if i, err := strconv.Atoi(gdprQuerySignal); err == nil {


Since there are only 2 possible values, would a case statement be more efficient? This could even be combined with the above check, so if it does not match one of the two values, the default case is the above error.

Makes perfect sense. Modified

hhhjort · 2023-07-05T19:53:01Z

errortypes/severity.go

@@ -20,7 +20,7 @@ func isFatal(err error) bool {
 	return !ok || s.Severity() == SeverityFatal
 }

-func isWarning(err error) bool {
+func IsWarning(err error) bool {


Since this is now being exported, might want to make a comment to explain that an error can only be a warning if it is a warning type from this package. There is no magic to divine that a random error is intended to only be a warning.

Added a comment here and similar one in the definition of the Warning type. Let me know if you agree with the wording

hhhjort

LGTM

guscarreon added 5 commits May 9, 2023 17:14

First commit

ae3593c

StrToInt8Slice now throws error

a489a65

Merge branch 'master' into gppSetuid

7ac1ca6

Handle StrToInt8Slice error

7de58ed

Merge branch 'master' into gppSetuid

99bf55a

SyntaxNode added the do not port label May 19, 2023

guscarreon changed the title ~~GPP in the '/setuid' endpoint~~ GPP in the 'setuid' endpoint May 22, 2023

guscarreon changed the title ~~GPP in the 'setuid' endpoint~~ GPP in the /setuid endpoint May 22, 2023

bsardo assigned bsardo, hhhjort and AlexBVolcy and unassigned hhhjort May 23, 2023

bsardo changed the title ~~GPP in the /setuid endpoint~~ GPP in the setuid endpoint May 24, 2023

bsardo reviewed May 31, 2023

View reviewed changes

guscarreon added 2 commits May 31, 2023 17:44

Master May 31st

a0a4de0

Brian's review and additional tests and test descriptions

ca77848

AlexBVolcy reviewed Jun 5, 2023

View reviewed changes

endpoints/setuid.go Outdated Show resolved Hide resolved

endpoints/setuid.go Outdated Show resolved Hide resolved

endpoints/setuid.go Outdated Show resolved Hide resolved

guscarreon added 3 commits June 6, 2023 15:07

Merge branch 'master' into gppSetuid

e2ec7ee

Alex's review

4c58299

Trimmed comment length

e94febc

AlexBVolcy previously approved these changes Jun 7, 2023

View reviewed changes

gppSetuid

97ff080

guscarreon dismissed AlexBVolcy’s stale review via 97ff080 June 12, 2023 15:19

AlexBVolcy reviewed Jun 13, 2023

View reviewed changes

Test case tweak

df562e9

bsardo reviewed Jun 15, 2023

View reviewed changes

guscarreon added 4 commits June 20, 2023 17:00

Merge branch 'master' into gppSetuid

c3565fe

step out quick

3c6704d

Merge branch 'master' into gppSetuid

cf26128

Refactored extractGDPRInfo

9fafffe

guscarreon added 4 commits June 27, 2023 12:59

Warnings not yet written into response

c72d23f

setuid endpoint responds with GDPR warnings

201e368

Merge branch 'master' into gppSetuid

d74f83d

Cleaned up code and rolled back cookie_sync

c8f902c

AlexBVolcy reviewed Jun 27, 2023

View reviewed changes

guscarreon added 2 commits June 28, 2023 15:19

Merge branch 'master' into gppSetuid

a9f7844

Alex's review

43773fd

AlexBVolcy previously approved these changes Jul 1, 2023

View reviewed changes

bsardo assigned hhhjort and unassigned bsardo Jul 5, 2023

hhhjort reviewed Jul 5, 2023

View reviewed changes

Hans' review

eefe724

guscarreon dismissed AlexBVolcy’s stale review via eefe724 July 7, 2023 04:03

hhhjort approved these changes Jul 10, 2023

View reviewed changes

AlexBVolcy approved these changes Jul 10, 2023

View reviewed changes

bsardo merged commit be632e0 into master Jul 17, 2023
3 checks passed

SyntaxNode deleted the gppSetuid branch October 2, 2023 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPP in the setuid endpoint #2761

GPP in the setuid endpoint #2761

guscarreon commented May 9, 2023

bsardo May 31, 2023

guscarreon Jun 1, 2023

bsardo May 31, 2023

guscarreon Jun 1, 2023

guscarreon Jun 1, 2023

AlexBVolcy left a comment

AlexBVolcy Jun 13, 2023

guscarreon Jun 14, 2023

bsardo Jun 15, 2023

bsardo Jun 27, 2023

bsardo Jun 15, 2023

bsardo Jun 15, 2023

bsardo Jun 27, 2023

AlexBVolcy Jun 27, 2023

guscarreon Jun 28, 2023

AlexBVolcy Jun 27, 2023

guscarreon Jun 28, 2023

AlexBVolcy Jun 27, 2023

AlexBVolcy Jun 27, 2023

AlexBVolcy Jun 27, 2023

guscarreon Jun 28, 2023

AlexBVolcy Jun 27, 2023

guscarreon Jun 28, 2023

AlexBVolcy left a comment

hhhjort Jul 5, 2023

guscarreon Jul 7, 2023

hhhjort Jul 5, 2023

guscarreon Jul 7, 2023

hhhjort left a comment

GPP in the setuid endpoint #2761

GPP in the setuid endpoint #2761

Conversation

guscarreon commented May 9, 2023

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

AlexBVolcy left a 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

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

AlexBVolcy left a 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

hhhjort left a comment

Choose a reason for hiding this comment