-
Notifications
You must be signed in to change notification settings - Fork 855
New Adapter: TRUSTX (remove Grid alias) #4614
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
base: master
Are you sure you want to change the base?
Conversation
Code coverage summaryNote:
trustxRefer here for heat map coverage report |
|
Please rebase this PR with the latest changes as that is causing the server to not be up due to a bug which has been fixed |
6e452fe to
2b6b83f
Compare
Code coverage summaryNote:
trustxRefer here for heat map coverage report |
|
Hi @karwaankit32, rebased! Is there anything else I can do to expedite merging this PR? |
Thanks. I am in the process of reviewing the PR today and will post feedback by EOD. Thanks! |
| } | ||
|
|
||
| var validParams = []string{ | ||
| `{}`, |
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.
Question: is a default Uid being 0 acceptable here? Is there a reason why we are not making Uid required?
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.
Hi, 0 is acceptable, as publishers can also pass the value in other fields (e.g. imp.tagid)
| `{"uid": 1234}`, | ||
| `{"uid": 1234, "keywords":{"site": {}, "user": {}}}`, | ||
| } | ||
| var invalidParams = []string{ |
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.
Can we add other test cases where we also check data types of Uid and Keywords?
Ex: passing a "string" Uid and similarly for Keywords?
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.
Added 2 test cases.
| func setImpExtData(imp *openrtb2.Imp) { | ||
| var ext impExt | ||
| if err := jsonutil.Unmarshal(imp.Ext, &ext); err != nil { | ||
| return |
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.
do we want to log these errors?
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.
I don't think so, as the data read from the imp ext (adslot) is optional to us, and the request is valid without it.
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.
Makes sense but i'd recommend adding a test case for this
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.
Added a test case for this!
| return nil | ||
| } | ||
| var bidMeta *openrtb_ext.ExtBidPrebidMeta | ||
| if be.Bidder.TrustX.NetworkName != "" { |
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.
Can there be a case where TrustX is empty? Won't this logic break in such cases?
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.
Yes, there can be a case where we don't return the network name, but TrustX isn't a pointer so nothing will break, bid meta will just be nil.
Code coverage summaryNote:
trustxRefer here for heat map coverage report |
Code coverage summaryNote:
trustxRefer here for heat map coverage report |
| redirect: | ||
| url: "https://x.bidswitch.net/check_uuid/{{.RedirectURL}}?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}" | ||
| userMacro: "${BSW_UUID}" | ||
|
|
||
| url: "https://sync.trustx.org/usync-pbs?us_privacy={{.USPrivacy}}&gpp={{.GPP}}&gpp_sid={{.GPPSID}}&redirect={{.RedirectURL}}" | ||
| userMacro: "$UID" |
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.
| iframe: | ||
| url: "https://static.cdn.trustx.org/x/user_sync.html?source=pbs&us_privacy={{.USPrivacy}}&gpp={{.GPP}}&gpp_sid={{.GPPSID}}&redirect={{.RedirectURL}}" | ||
| userMacro: "$UID" |
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.
Iframe does not appear to be working. I see a 200 response from this endpoint with an html body which is triggering a request to https://sync.trustx.org/usync and that is redirecting to the PBS setuid endpoint assuming https but in this case it is http.
Is the redirect necessary? I believe I have seen other adapters embed the PBS host URL in the html response firing a request to the setuid endpoint directly from the client.
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.
Hi, the failure is caused by browser mixed-content restrictions: an HTTPS iframe cannot initiate requests to an HTTP PBS setuid endpoint.
This would fail even without the redirect hop (calling the endpoint directly from the client).
For iframe sync to work reliably, the PBS endpoint must be served over HTTPS.
This is not adapter-specific behavior, but a native browser security restriction.
Reference: https://developer.mozilla.org/en-US/docs/Web/Security/Defenses/Mixed_content

Docs PR: prebid/prebid.github.io#6355