Handle empty consent string during cookie sync and setuid

[bsardo]

The main purpose of this commit is to properly handle an empty consent during a cookie sync. The logic is similar to changes recently made to the openrtb endpoints.

• If the GDPR signal is determined to be 0 indicating GDPR does not apply (either because it was explicitly set to 0 in the request or was ambiguous and normalized to 0), cookie sync is allowed.
• Else If the GDPR signal is determined to be 1 indicating GDPR applies (either because it was explicitly set to 1 in the request or was ambiguous and normalized to 1), and the consent string is empty, cookie sync is not allowed.
• Else the GDPR signal was determined to be 1 with a non-empty consent string so process the consent string to determine whether cookie sync is allowed

Other things to note:

• You’ll notice that the three methods that implement the Permissions interface all generally behave in the same way now for clarity. They attempt to normalize the GDPR signal, early return if the signal is No, early return if the consent string is empty and then process the consent string.
• the GDPR processing switch statements in endpoints/auction.go and endpoints/setuid.go were refactored to flatten the logic with early returns.
• I didn’t bother to tabularize the new tests I added in impl_test.go. I’ll handle that in another PR dedicated to that test file soon.

[bsardo]

Thoughts on this?

[SyntaxNode]

I vote to keep it. Changing external behavior can have side effects. If no one's asking, let it be. IMHO.

[SyntaxNode]

Do we need the (i == 0 || i == 1) check or can gdpr.Signal(i) take care of that logic?

[bsardo]

I think we need that logic. I originally intended to use gdpr.Signal(i) to handle other integer values as you're suggesting but it results in gdprSignal being set to that integer instead of one of the three signal types.

[SyntaxNode]

Gotcha.

[SyntaxNode]

Looking at this again, I dislike that 0 and 1 are hardcoded here far away from the context of 0 and 1 being known signals in the gdpr package. How about something like:

gdprSignal := gdpr.SignalAmbiguous
if i, err := strconv.Atoi(gdprPrivacyPolicy.Signal); err == nil && gdpr.IsKnownSignal(i) {
  gdprSignal = gdpr.Signal(i)
}

or

gdprSignal := gdpr.SignalAmbiguous
if signal, err := gdpr.SignalParse(gdprPrivacyPolicy.Signal); err == nil {
  gdprSignal = signal
}

[SyntaxNode]

If you're willing to convert this to a table driven test, I find this:

 {signal: "0", consent: "", allowBidderSync: true, allowHostCookies: false, expectAllow: true 

much easier to read.

[SyntaxNode]

FYI: We'll revisit in this in the future. We decided on a 451 error code for these situations.

[SyntaxNode]

Why is signal a pointer? Would you consider:

gdprSignal = p.normalizeGDPR(gdprSignal)

to avoid the heap allocation?

[hhhjort]

Looks good to me.

[SyntaxNode]

I have some nitpick comments and test comments. Because we're mocking out parts of the gdpr checks, some of the test cases don't make sense to me.

[SyntaxNode]

Looking at this again, I dislike that 0 and 1 are hardcoded here far away from the context of 0 and 1 being known signals in the gdpr package. How about something like:

gdprSignal := gdpr.SignalAmbiguous
if i, err := strconv.Atoi(gdprPrivacyPolicy.Signal); err == nil && gdpr.IsKnownSignal(i) {
  gdprSignal = gdpr.Signal(i)
}

or

gdprSignal := gdpr.SignalAmbiguous
if signal, err := gdpr.SignalParse(gdprPrivacyPolicy.Signal); err == nil {
  gdprSignal = signal
}

[SyntaxNode]

How about we check the error first? err != nil || !canSync.

[SyntaxNode]

If the gdpr signal is no (don't enforce) and the host allows cookies, shouldn't we allow syncing? The bidder sync wouldn't apply since gdpr is not enforced?

This might be a test setup error since we're using s mock. The real BidderSyncAllowed would return true if the signal is no.

[bsardo]

Do you think I should just remove the cases that are unrealistic in this test given the internals of HostCookiesAllowed and BidderSyncAllowed? My thought here was to exercise the conditionals in the shouldUserSync method ignoring the real implementation of those methods by using the mocks that were already in place.

[bsardo]

@SyntaxNode mentioned offline that he thinks we should remove the unrealistic cases.

[hhhjort]

We shouldn't test conditions that only the mock would pass (test relies on the mock behavior being different from the real behavior). In these cases we should check if the real behavior is already being tested for in the test for the object being mocked. If so we should get rid of the test here. If not, we should consider either adding a test on the original object or improving the mock here so that the code paths can be properly tested.

[bsardo]

That makes sense. I'm remembering now that this is testing the legacy auction endpoint and this will all be deleted soon so I don't think it's worth our time to improve the mock, plus the original object has pretty good coverage already with a story in my queue to further improve it. I removed a bunch of the unrealistic test cases and just focused on testing how HostCookiesAllowed and BidderSyncAllowed affect the return value.

[SyntaxNode]

Same comment as earlier since ambiguous == "no" (well, depending on the UsersyncIfAmbiguous setting, but that's not fully in play here since it's a mock.