GDPR: pass geo based on special feature 1 opt-in

[bsardo]

This PR fixes a GDPR bug and depends on prebid/go-gdpr#34.
We currently determine if geo information should be passed in a bid request based on whether special purpose 1 is set in the GVL for the vendor which is incorrect. Instead we should be looking at whether special feature 1 is set for the vendor. The main change is here in impl.go on line 137:
passGeo = vendorException || (consentMeta.SpecialFeatureOptIn(1) && (vendor.SpecialFeature(1) || weakVendorEnforcement))

Special purposes are defined in the TCF2 spec but they are not used by PBS. As a result, all references to special purpose 1 have been renamed to eliminate confusion.

[SyntaxNode]

Interesting. We were using the wrong terminology?

[bsardo]

It's not just a matter of the wrong terminology. We are looking at the wrong field in the GVL. The go-gdpr package provides a vendor.SpecialPurpose method that checks if a special purpose is defined in the GVL specialPurposes field for a vendor. We are currently calling this method but that isn't correct. Instead we should be looking at GVL specialFeatures for a vendor. I have a go-gdpr PR out that is currently under review that will add a new method that allows us to access the correct field for a given vendor.

[SyntaxNode]

That'a concerning. What's the difference between a special purpose and a special feature? The TCF folks could've done better in the naming there.

[SyntaxNode]

Please verify this code has full test coverage for cases of none, one, and many (2). It's been a problem for us in the past.

[bsardo]

👍 Done

[guscarreon]

Left a nitpick

[guscarreon]

I find this to be a very comprehensive test case but: can we make one last assertion in the resulting Configuration object itself? I realize TestSpecialFeature1VendorExceptionMap does this to an extent but not when oldSpecialFeature1Config is set. Maybe we could add an v.Unmarshal() call at the end of this test case so we assert that cfg.GDPR.TCF2.SpecialFeature1 was correctly set even when gdpr.tcf2.special_purpose1 is set as in oldSpecialFeature1Config.

824     for _, tt := range tests {
825         v := viper.New()
826         v.SetConfigType("yaml")
827         v.ReadConfig(bytes.NewBuffer(tt.config))
828
829         migrateConfigSpecialFeature1(v)
830
831         if len(tt.config) > 0 {
832             assert.Equal(t, tt.wantSpecialFeature1Enforce, v.Get("gdpr.tcf2.special_feature1.enforce").(bool), tt.description)
833             assert.Equal(t, tt.wantSpecialFeature1VendorExceptions, v.GetStringSlice("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description)
834         } else {
835             assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.enforce"), tt.description)
836             assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description)
837         }
    +
    +	    var c Configuration
    +       err := v.Unmarshal(&c)
    +       assert.NoError(t, err, tt.desc)
    +
    +       // Assert expected values on the `cfg.GDPR.TCF2.SpecialFeature1` object itself
    +       assert.Equal(t, tt.wantSpecialFeature1Enforce, cfg.GDPR.TCF2.SpecialFeature1.Enforce, tt.desc)
    +       assert.ElementsMatch(t, tt.wantSpecialFeature1VendorExceptions, cfg.GDPR.TCF2.SpecialFeature1.VendorExceptions, tt.desc)
838     }
config/config_test.go

Or even better, a New(v) call so we can also assert the correctness of the cfg.GDPR.TCF2.SpecialFeature1.VendorExceptionMap

824     for _, tt := range tests {
825         v := viper.New()
826         v.SetConfigType("yaml")
827         v.ReadConfig(bytes.NewBuffer(tt.config))
828
829         migrateConfigSpecialFeature1(v)
830
831         if len(tt.config) > 0 {
832             assert.Equal(t, tt.wantSpecialFeature1Enforce, v.Get("gdpr.tcf2.special_feature1.enforce").(bool), tt.description)
833             assert.Equal(t, tt.wantSpecialFeature1VendorExceptions, v.GetStringSlice("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description)
834         } else {
835             assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.enforce"), tt.description)
836             assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description)
837         }
    +
    +	    cfg := New(v)
    +
    +       // Assert expected values on the `cfg.GDPR.TCF2.SpecialFeature1` object itself
    +       assert.Equal(t, tt.wantSpecialFeature1Enforce, cfg.GDPR.TCF2.SpecialFeature1.Enforce, tt.desc)
    +       assert.ElementsMatch(t, tt.wantSpecialFeature1VendorExceptions, cfg.GDPR.TCF2.SpecialFeature1.VendorExceptions, tt.desc)
    +       assert.Equal(t, tt.wantSpecialFeature1VendorExceptionsMap, cfg.GDPR.TCF2.SpecialFeature1.VendorExceptionMap, tt.desc)
838     }
config/config_test.go

Another option is to probably add an oldSpecialFeature1Config scenatio to TestSpecialFeature1VendorExceptionMap? What do you think?

[bsardo]

@guscarreon and I spoke offline about this.

My line of thinking was that the coverage was there across three unit tests but it was hard to see:
TestMigrateConfigSpecialFeature1 —> verifies the deprecated special purpose config values are properly mapped to the new special feature config values
TestFullConfig —> verifies all of the special purpose struct fields, including the vendor exception map, are correctly populated based on what’s set in the special feature config values
TestSpecialFeature1VendorExceptionMap —> verifies the vendor exception map is created correctly no matter how many elements are in the vendor exception slice

I'm hesitant to make use of another function in the package to perform assertions in a pure unit test so instead of calling New() we agreed to call unmarshal as a happy medium to ensure what's written to the viper config in the migrate function is able to be unmarshaled into the struct.

[bsardo]

I took a closer look at the special purpose to special feature config migration logic added here in config/config.go with @mansinahar's suggestion in mind and realized that no update is needed here.

To recap, during our group review it was pointed out that the TCF2Purpose struct had an Enabled field, which was recently deprecated, and an EnforcePurpose field, which was recently added as the replacement for Enabled. Before this PR, this struct was being used for both general purposes and special purposes so, from just looking at the struct, it appeared as though hosts could set EnforcePurpose for a special purpose and would receive a deprecation warning when doing so. However, that isn't the case. The migration logic that was added to migrate the TCF2Purpose Enabled field to EnforcePurpose a few months ago only threw deprecation warnings and mapped the Enabled value to EnforcePurpose for general purposes so hosts would never have seen a warning for a special purpose (see migrateConfigTCF2PurposeEnabledFlags in config/config.go.).

As such, I don't think it makes sense to do anything with the EnforcePurpose field in this new special purpose to special feature migration logic. The field simply existed on an internal data structure because the struct was being used to represent two kinds of things; it was never set nor was host ever informed to set it.

[mansinahar]

Sounds good! Thanks for looking into it @bsardo and providing clarification.

[guscarreon]

Looks great. Thank you for addressing my feedback

[VeronikaSolovei9]

Looks good, very straight forward.