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.

Sign up for GitHub

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

Jump to bottom

PBS adapter - fix logic for undefined endpoint URLs #7076

Merged
merged 5 commits into from
Jul 12, 2021
Merged

PBS adapter - fix logic for undefined endpoint URLs #7076

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3c8055e
PBS adapter - fix logic for undefined endpoint URLs
jsnellbaker Jun 22, 2021
793934b
update messages and add unit test
jsnellbaker Jun 22, 2021
e23cf2d
fix lint error
ChrisHuie Jun 22, 2021
86a73c8
new tests and other changes
jsnellbaker Jun 23, 2021
0a0274f
resolve conflict issue
jsnellbaker Jun 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion modules/prebidServerBidAdapter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,13 @@ function formatUrlParams(option) {
let temp = option[prop];
option[prop] = { p1Consent: temp, noP1Consent: temp };
}
if (utils.isPlainObject(option[prop]) && (!option[prop].p1Consent || !option[prop].noP1Consent)) {
msm0504 marked this conversation as resolved.
Show resolved Hide resolved
['p1Consent', 'noP1Consent'].forEach((conUrl) => {
if (!option[prop][conUrl]) {
utils.logWarn(`s2sConfig.${prop}.${conUrl} not defined. PBS request will be skipped in some P1 scenarios.`);
}
});
}
});
}

Expand Down Expand Up @@ -1097,7 +1104,7 @@ export function PrebidServer() {
const request = OPEN_RTB_PROTOCOL.buildRequest(s2sBidRequest, bidRequests, validAdUnits, s2sBidRequest.s2sConfig, requestedBidders);
const requestJson = request && JSON.stringify(request);
utils.logInfo('BidRequest: ' + requestJson);
if (request && requestJson) {
if (request && requestJson && getMatchingConsentUrl(s2sBidRequest.s2sConfig.endpoint, gdprConsent)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put the URL into a variable. Makes refactoring easier as there's only one place to look for

ajax(
getMatchingConsentUrl(s2sBidRequest.s2sConfig.endpoint, gdprConsent),
{
Expand All @@ -1107,6 +1114,8 @@ export function PrebidServer() {
requestJson,
{ contentType: 'text/plain', withCredentials: true }
);
} else {
utils.logError('PBS request not made. Check endpoints.');
}
}
};
Expand Down
13 changes: 13 additions & 0 deletions test/spec/modules/prebidServerBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,19 @@ describe('S2S Adapter', function () {
resetSyncedStatus();
});

it('should block request if config did not define proper URL in endpoint object config', function() {
let badConfig = utils.deepClone(CONFIG);
badConfig.endpoint = { noP1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' };
config.setConfig({ s2sConfig: badConfig });

let badCfgRequest = utils.deepClone(REQUEST);
badCfgRequest.s2sConfig = badConfig;

adapter.callBids(badCfgRequest, BID_REQUESTS, addBidResponse, done, ajax);

expect(server.requests.length).to.equal(0);
});

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add two more test cases

  1. for p1Consent only configuration (which is probably the more common one)
  2. for an empty object
Suggested change
it('should block request if config did not define proper URL in endpoint object config', function() {
let badConfig = utils.deepClone(CONFIG);
badConfig.endpoint = { noP1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' };
config.setConfig({ s2sConfig: badConfig });
let badCfgRequest = utils.deepClone(REQUEST);
badCfgRequest.s2sConfig = badConfig;
adapter.callBids(badCfgRequest, BID_REQUESTS, addBidResponse, done, ajax);
expect(server.requests.length).to.equal(0);
});
[
{ p1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' },
{ noP1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' },
{ }
].forEach(endpoint => {
it('should block request if config did not define proper URL in endpoint object config', function() {
let badConfig = utils.deepClone(CONFIG);
badConfig.endpoint = endpoint;
config.setConfig({ s2sConfig: badConfig });
let badCfgRequest = utils.deepClone(REQUEST);
badCfgRequest.s2sConfig = badConfig;
adapter.callBids(badCfgRequest, BID_REQUESTS, addBidResponse, done, ajax);
expect(server.requests.length).to.equal(0);
});
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean add a test for when p1Consent is the only property defined and the request contains a no-consent scenario - to test that the request is blocked here as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for confirming. I added some tests guessing that was the case. Please take a look when you have the chance (in addition to the other change you suggested).

it('should not add outstrean without renderer', function () {
config.setConfig({ s2sConfig: CONFIG });

Expand Down