-
Notifications
You must be signed in to change notification settings - Fork 4
register secure signals only when a valid identity is present #213
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
Conversation
| }, | ||
| }); | ||
|
|
||
| if (!this.hasRegisteredSecureSignals) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can actually cause incorrect behavior with bidding if we allow this to register multiple times. The only downside here is that clearing secure signals cache and then logging in again without refreshing the page will not reset the flag. I'm not sure how likely that scenario is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do recommend clearing the cache at some point I think.
Yep - https://unifiedid.com/docs/guides/integration-google-ss#publisher-integration
So we can expect some publishers to be calling window.googletag.secureSignalProviders.clearAllCache(); at some point. This ended up being the workaround they suggested for if Secure Signals cached a null identity and then the user logged in.
Unfortunately I don't know if we have a way to tell if we're currently registered with Google :(
src/secureSignal_shared.ts
Outdated
| }); | ||
| }; | ||
|
|
||
| public resetSecureSignalsCache = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache helper that also resets the internal state to allow re-registering secure signals within the same session.
| }); | ||
|
|
||
| if (!this.hasRegisteredSecureSignals) { | ||
| this.hasRegisteredSecureSignals = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any error that could happen below this that would cause registering secure signals to actually be false?
| ) { | ||
| if (window.__euid.getIdentity()) { | ||
| window.__euidSecureSignalProvider.registerSecureSignalProvider(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not do the check here to see if it is already registered instead of inside the register function?
| ) { | ||
| if ('getIdentity' in window.__uid2! && window.__uid2!.getIdentity()) { | ||
| if (eventType === 'IdentityUpdated') { | ||
| window.__uid2SecureSignalProvider?.resetProviderRegistration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will reset the flag, but that means we'll register the collector again whenever identity is updated - is that OK? You mentioned registering it multiple times can cause problems with the bidder.
No description provided.