Skip to content
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.

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

Introduce moduleType checks when initialising storageManager [DO NOT MERGE] #5643

Closed
wants to merge 1 commit into from

Conversation

Fawke
Copy link
Contributor

@Fawke Fawke commented Aug 20, 2020

Type of change

  • Bugfix

Description of change

Related #5615

The deviceAccessHook in the GDPR Enforcement module had a bug where it was trying to call the function getGvlid(), a function which returns GVL ID only for bid adapters, but the hook can be triggered by any module: it can be a bid adapter, user id submodule or an analytics adapter. Basically, any module which initialises the storageManager by calling, getStorageManager would trigger a call to deviceAccessHook if gdprEnforcement module is included in the build.

Each type of module has it own GVL ID getter function, so, if we call the getGvlid() function for a user id module, it'll return undefined since user id module should call its own function, ie,getGvlidForUserIdModule(). Same for analytics adatpers as well.

I propose a slight change where we include the MODULE_TYPE, along with GVL_ID and MODULE_NAME when calling the getStorageManager function. All three are optional fields and should be included if GDPR Enforcement is part of the build. That way, the deviceAccessHook will have a way of determining which GVL ID getter function to call based on the module type. If no module type is found, it'll default to calling the getGvlIdForBidAdapter() function.

If everyone's fine with this approach, I plan to add a commit to this PR where I go and make the change for the modules that have specified a gvl id and are initialising the storageManager, to add the third parameter, MODULE_TYPE, in the call.

This fixes the issue with id5Id, but #5615, also mentions that Criteo and pubCommonId are blocked despite having consent. That's because both haven't specified GVL ID when registering their user id submodules.

I plan to create an issue calling out all the adapters & modules which haven't listed their GVL IDs. Without GVL ID, gdprEnforcement will block the module unless it's specifically listed under vendorExceptions by the publisher.

Alternate Approach

Have a single getter function for GVL ID for all three module types: Bid Adapter, User ID Submodule & Analytics Adapter. Pros & Cons to this approach:

Pros:
  • Clean, less code since we'll have only one GVL ID getter function.
  • No need to introduce extra parameter moduleType when initializing getStorageManager()
Cons:
  • No way to differentiate between a GVL ID for a Appnexus Bid Adapter or an Appnexus Analytics Adapter. Both will be same. Similarly, Rubicon Bid Adapter and Rubicon Analytics Adapter will have the same GVLID. Essentially, any module with the same identifier, (code for bid adapter, provider for analytics adapter and name for userId submodule), we can't give them a different GVL ID. For example, appnexus (bidAdapter) can't have GVL ID as 1 & appnexus (analyticsAdapter) have GVL ID as 2. Both should be either 1 or 2.

If it is alright for all the modules of a single entity to have the same GVL ID, then the alternate approach works. But if, we wanna provide flexibility in terms of assigning different GVL ID to a bidAdapter and a analyticsAdapter for the same entity (ex - Appnexus), then this approach fails.

@Fawke
Copy link
Contributor Author

Fawke commented Aug 20, 2020

@bretg I was wondering if it would be okay to introduce a table at the end of docs for the GDPR Enforcement module where we list down all the modules that have specified a GVL ID and is compatible to be used with gdprEnforcement module.

@bretg
Copy link
Collaborator

bretg commented Aug 21, 2020

if it would be okay to introduce a table at the end of docs for the GDPR Enforcement module where we list down all the modules that have specified a GVL ID and is compatible to be used with gdprEnforcement module.

Sounds good. You can submit the PR against the docs repo. Please word in the positive: these ones do support GVL ID.

pubcommon ID is going to be a point of contention. It's not really an entity that does anything with data. I guess that's another topic for the lawyers.

@Fawke
Copy link
Contributor Author

Fawke commented Aug 25, 2020

@bretg Added the table, prebid/prebid.github.io#2271

Since this table is not dynamically generated, the challenge will be to maintain it. I've got a script which I can run periodically to capture if any new modules add support for gvlid, but, should it be the responsibility of the PR author do submit a docs PR to make this table change as well?

@smenzer
Copy link
Collaborator

smenzer commented Aug 26, 2020

@Fawke per your second approach of simplifying the calls to a single method...i don't see why a single entity would have multiple GVLIDs...appnexus has one...rubicon has one. It doesn't matter why they are being called, the fact is they are and they're only registered once in the GVL (https://vendorlist.consensu.org/v2/vendor-list.json). If that simplifies the code, I'm all for it.

@Fawke
Copy link
Contributor Author

Fawke commented Aug 26, 2020

@smenzer I'd prefer that there be 1 GVL ID for 1 entity (rubicon, appnexus, id5Id, etc). It simplifies things a lot. But from a legal standpoint, I do not know if there would ever be a scenario where different GVL IDs are required and if there is that requirement, Prebid.js should be flexible enough to accommodate those modules.

Also, we would need to change the design of gvlMapping which basically allows the publisher to override the GVL ID for a module.

For example, currently we have:

pbjs.setConfig({
  gvlMapping: {
    appnexus: 12,
    rubicon: 13
  }
});

Publisher can override the GVL IDs for any module. Now, if we give preference to moduleType as well, there will be a confusion. We will not know if we're overriding bidAdapter or the analyticsAdapter GVL ID. In that case, we'd need to be more specific.

@bretg @jaiminpanchal27 Any thoughts?

@smenzer
Copy link
Collaborator

smenzer commented Aug 28, 2020

@Fawke I don't have a strong preference on how you build this, i think both approaches are fine. my point was just that from a legal perspective i don't see why there would ever be separate gvlids for a given entity (regardless of how many modules/adapters they have).

@jaiminpanchal27
Copy link
Collaborator

@Fawke This looks good but external modules can make mistake here in passing this argument.

Instead of adding extra argument MODULE_TYPE, I would suggest to use the user id module registry and we create registry for analytics adapters. Something similar to _bidderRegistry of adapterManager.

Making Prebid core aware of all modules and its type would give extra benefit at other places.

@Fawke
Copy link
Contributor Author

Fawke commented Aug 31, 2020

@smenzer I think you're right, I think we should go ahead with 1 moduleCode - 1 GVL ID approach. This essentially established a 1-1 mapping of moduleCode and GVL ID. In cases, where a module with the same code needs to have different GVL ID's for different moduleTypes, they will need to register that module under a different moduleCode.

So, am scrapping this idea of moduleTypes for now, and I'll make a fresh commit to this PR to incorporate the new change.

@Fawke
Copy link
Contributor Author

Fawke commented Sep 1, 2020

Created another PR with the implementation of the alternate approach. #5643

Please have a look.

@smenzer
Copy link
Collaborator

smenzer commented Sep 2, 2020

Created another PR with the implementation of the alternate approach. #5643

Please have a look.

I think you meant #5686 right?

@Fawke
Copy link
Contributor Author

Fawke commented Sep 2, 2020

Created another PR with the implementation of the alternate approach. #5643
Please have a look.

I think you meant #5686 right?

Yes, #5686.

@Fawke
Copy link
Contributor Author

Fawke commented Sep 3, 2020

Closing this in favor of #5686

@Fawke Fawke closed this Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants