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

Eidpermissions Request Validation Update #3666

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ private Future<BidRequest> updateBidRequest(AuctionContext auctionContext) {
.compose(resolvedBidRequest -> ortb2RequestFactory.validateRequest(
resolvedBidRequest,
auctionContext.getHttpRequest(),
auctionContext.getDebugContext(),
auctionContext.getDebugWarnings()));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,8 @@ private Future<BidRequest> updateAndValidateBidRequest(AuctionContext auctionCon

return storedRequestProcessor.processAuctionRequest(account.getId(), auctionContext.getBidRequest())
.compose(auctionStoredResult -> updateBidRequest(auctionStoredResult, auctionContext))
.compose(bidRequest -> ortb2RequestFactory.validateRequest(bidRequest, httpRequest, debugWarnings))
.compose(bidRequest -> ortb2RequestFactory.validateRequest(
bidRequest, httpRequest, auctionContext.getDebugContext(), debugWarnings))
.map(interstitialProcessor::process);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,11 @@ public Future<ActivityInfrastructure> activityInfrastructureFrom(AuctionContext

public Future<BidRequest> validateRequest(BidRequest bidRequest,
HttpRequestContext httpRequestContext,
DebugContext debugContext,
List<String> warnings) {

final ValidationResult validationResult = requestValidator.validate(bidRequest, httpRequestContext);
final ValidationResult validationResult = requestValidator.validate(
bidRequest, httpRequestContext, debugContext);

if (validationResult.hasWarnings()) {
warnings.addAll(validationResult.getWarnings());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,24 @@ public Future<WithPodErrors<AuctionContext>> fromRequest(RoutingContext routingC
return ortb2RequestFactory.executeEntrypointHooks(routingContext, body, initialAuctionContext)
.compose(httpRequest -> createBidRequest(httpRequest)
.map(bidRequest -> removeEmptyEids(bidRequest, initialAuctionContext.getDebugWarnings()))
.compose(bidRequest -> validateRequest(
bidRequest,
httpRequest,
initialAuctionContext.getDebugWarnings()))

.map(bidRequestWithErrors -> populatePodErrors(
bidRequestWithErrors.getPodErrors(), podErrors, bidRequestWithErrors))

.map(bidRequestWithErrors -> ortb2RequestFactory.enrichAuctionContext(
initialAuctionContext, httpRequest, bidRequestWithErrors.getData(), startTime)))

.compose(auctionContext -> ortb2RequestFactory.fetchAccountWithoutStoredRequestLookup(auctionContext)
.map(auctionContext -> auctionContext.with(debugResolver.debugContextFrom(auctionContext)))

.compose(auctionContext -> ortb2RequestFactory.validateRequest(
auctionContext.getBidRequest(),
auctionContext.getHttpRequest(),
auctionContext.getDebugContext(),
auctionContext.getDebugWarnings())
.map(auctionContext::with))

.map(auctionContext -> auctionContext.with(debugResolver.debugContextFrom(auctionContext)))
.compose(auctionContext -> ortb2RequestFactory.fetchAccountWithoutStoredRequestLookup(auctionContext)
.map(auctionContext::with))

.compose(auctionContext -> geoLocationServiceWrapper.lookup(auctionContext)
.map(auctionContext::with))
Expand Down Expand Up @@ -323,12 +326,4 @@ private WithPodErrors<BidRequest> fillImplicitParameters(HttpRequestContext http

return WithPodErrors.of(updatedWithDebugBidRequest, bidRequestToErrors.getPodErrors());
}

private Future<WithPodErrors<BidRequest>> validateRequest(WithPodErrors<BidRequest> requestWithPodErrors,
HttpRequestContext httpRequestContext,
List<String> warnings) {

return ortb2RequestFactory.validateRequest(requestWithPodErrors.getData(), httpRequestContext, warnings)
.map(bidRequest -> requestWithPodErrors);
}
}
40 changes: 27 additions & 13 deletions src/main/java/org/prebid/server/validation/RequestValidator.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.apache.commons.collections4.MapUtils;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.StringUtils;
import org.prebid.server.auction.model.debug.DebugContext;
import org.prebid.server.bidder.BidderCatalog;
import org.prebid.server.json.JacksonMapper;
import org.prebid.server.log.ConditionalLogger;
Expand Down Expand Up @@ -96,8 +97,12 @@ public RequestValidator(BidderCatalog bidderCatalog,
* Validates the {@link BidRequest} against a list of validation checks, however, reports only one problem
* at a time.
*/
public ValidationResult validate(BidRequest bidRequest, HttpRequestContext httpRequestContext) {
public ValidationResult validate(BidRequest bidRequest,
HttpRequestContext httpRequestContext,
DebugContext debugContext) {

final List<String> warnings = new ArrayList<>();
final boolean isDebugEnabled = debugContext != null && debugContext.isDebugEnabled();
try {
if (StringUtils.isBlank(bidRequest.getId())) {
throw new ValidationException("request missing required field: \"id\"");
Expand All @@ -124,7 +129,7 @@ public ValidationResult validate(BidRequest bidRequest, HttpRequestContext httpR
validateAliases(aliases);
validateAliasesGvlIds(extRequestPrebid, aliases);
validateBidAdjustmentFactors(extRequestPrebid.getBidadjustmentfactors(), aliases);
validateExtBidPrebidData(extRequestPrebid.getData(), aliases);
validateExtBidPrebidData(extRequestPrebid.getData(), aliases, isDebugEnabled, warnings);
validateSchains(extRequestPrebid.getSchains());
}

Expand Down Expand Up @@ -313,15 +318,20 @@ private void validateSchains(List<ExtRequestPrebidSchain> schains) throws Valida
}
}

private void validateExtBidPrebidData(ExtRequestPrebidData data, Map<String, String> aliases)
throws ValidationException {
private void validateExtBidPrebidData(ExtRequestPrebidData data,
Map<String, String> aliases,
boolean isDebugEnabled,
List<String> warnings) throws ValidationException {

if (data != null) {
validateEidPermissions(data.getEidPermissions(), aliases);
validateEidPermissions(data.getEidPermissions(), aliases, isDebugEnabled, warnings);
}
}

private void validateEidPermissions(List<ExtRequestPrebidDataEidPermissions> eidPermissions,
Map<String, String> aliases) throws ValidationException {
Map<String, String> aliases,
boolean isDebugEnabled,
List<String> warnings) throws ValidationException {

if (eidPermissions == null) {
return;
Expand All @@ -333,7 +343,7 @@ private void validateEidPermissions(List<ExtRequestPrebidDataEidPermissions> eid
}

validateEidPermissionSource(eidPermission.getSource());
validateEidPermissionBidders(eidPermission.getBidders(), aliases);
validateEidPermissionBidders(eidPermission.getBidders(), aliases, isDebugEnabled, warnings);
}
}

Expand All @@ -344,18 +354,22 @@ private void validateEidPermissionSource(String source) throws ValidationExcepti
}

private void validateEidPermissionBidders(List<String> bidders,
Map<String, String> aliases) throws ValidationException {
Map<String, String> aliases,
boolean isDebugEnabled,
List<String> warnings) throws ValidationException {

if (CollectionUtils.isEmpty(bidders)) {
throw new ValidationException("request.ext.prebid.data.eidpermissions[].bidders[] required values"
+ " but was empty or null");
}

for (String bidder : bidders) {
if (!bidderCatalog.isValidName(bidder) && !bidderCatalog.isValidName(aliases.get(bidder))
&& ObjectUtils.notEqual(bidder, ASTERISK)) {
throw new ValidationException(
"request.ext.prebid.data.eidPermissions[].bidders[] unrecognized biddercode: '%s'", bidder);
if (isDebugEnabled) {
for (String bidder : bidders) {
if (!bidderCatalog.isValidName(bidder) && !bidderCatalog.isValidName(aliases.get(bidder))
&& ObjectUtils.notEqual(bidder, ASTERISK)) {
warnings.add("request.ext.prebid.data.eidPermissions[].bidders[] unrecognized biddercode: '%s'"
.formatted(bidder));
}
}
}
}
Expand Down
66 changes: 66 additions & 0 deletions src/test/groovy/org/prebid/server/functional/tests/EidsSpec.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,17 @@ import org.prebid.server.functional.util.PBSUtils
import static org.prebid.server.functional.model.bidder.BidderName.ALIAS
import static org.prebid.server.functional.model.bidder.BidderName.GENERIC
import static org.prebid.server.functional.model.bidder.BidderName.OPENX
import static org.prebid.server.functional.model.bidder.BidderName.UNKNOWN
import static org.prebid.server.functional.model.bidder.BidderName.WILDCARD
import static org.prebid.server.functional.model.request.auction.DebugCondition.DISABLED
import static org.prebid.server.functional.model.request.auction.DebugCondition.ENABLED
import static org.prebid.server.functional.model.response.auction.ErrorType.PREBID
import static org.prebid.server.functional.testcontainers.Dependencies.getNetworkServiceContainer

class EidsSpec extends BaseSpec {

private static final String EMPTY_STRING = ""
private static final String RANDOM_SOURCE_ID = PBSUtils.randomString

def "PBS shouldn't populate user.id from user.ext data"() {
given: "Default basic BidRequest with generic bidder"
Expand Down Expand Up @@ -91,6 +95,68 @@ class EidsSpec extends BaseSpec {
assert !bidderRequest.user.eids
}

def "PBS eids shouldn't include warning for unknown bidder when test and debug disabled"() {
given: "Default bid request with generic bidder"
def bidRequest = BidRequest.defaultBidRequest.tap {
user = requestUser
ext.prebid.data = new ExtRequestPrebidData(eidpermissions: [new EidPermission(source: RANDOM_SOURCE_ID, bidders: [UNKNOWN])])
it.ext.prebid.debug = DISABLED
it.test = DISABLED
}

when: "PBS processes auction request"
def bidResponse = defaultPbsService.sendAuctionRequest(bidRequest)

then: "Bidder request shouldn't contain requested eids"
def bidderRequest = bidder.getBidderRequest(bidRequest.id)
assert !bidderRequest.user.eids

and: "Bid response shouldn't contain warning"
assert !bidResponse.ext.warnings

where:
requestUser << [new User(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)]),
new User(ext: new UserExt(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)])),
new User(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)],
ext: new UserExt(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)]))]
}

def "PBS eids should include warning for unknown bidder when request in debug mode"() {
given: "Default bid request with generic bidder"
def bidRequest = BidRequest.defaultBidRequest.tap {
user = requestUser
ext.prebid.data = new ExtRequestPrebidData(eidpermissions: [new EidPermission(source: RANDOM_SOURCE_ID, bidders: [UNKNOWN])])
it.ext.prebid.debug = debug
it.test = test
}

when: "PBS processes auction request"
def bidResponse = defaultPbsService.sendAuctionRequest(bidRequest)

then: "Bidder request shouldn't contain requested eids"
def bidderRequest = bidder.getBidderRequest(bidRequest.id)
assert !bidderRequest.user.eids

and: "Bid response should contain warning"
assert bidResponse.ext.warnings[PREBID]?.code == [999]
assert bidResponse.ext.warnings[PREBID]?.message ==
["request.ext.prebid.data.eidPermissions[].bidders[] unrecognized biddercode: '$UNKNOWN'"]

where:
debug | test | requestUser
DISABLED | ENABLED | new User(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)])
DISABLED | ENABLED | new User(ext: new UserExt(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)]))
DISABLED | ENABLED | new User(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)], ext: new UserExt(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)]))

ENABLED | DISABLED | new User(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)])
ENABLED | DISABLED | new User(ext: new UserExt(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)]))
ENABLED | DISABLED | new User(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)], ext: new UserExt(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)]))

ENABLED | ENABLED | new User(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)])
ENABLED | ENABLED | new User(ext: new UserExt(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)]))
ENABLED | ENABLED | new User(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)], ext: new UserExt(eids: [Eid.getDefaultEid(RANDOM_SOURCE_ID)]))
}

def "PBs eid permissions should affect only specified on source"() {
given: "PBs with openx bidder"
def pbsService = pbsServiceFactory.getService(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1753,7 +1753,7 @@ private void givenBidRequest(

given(ortb2ImplicitParametersResolver.resolve(any(), any(), any(), anyBoolean())).willAnswer(
answerWithFirstArgument());
given(ortb2RequestFactory.validateRequest(any(), any(), any()))
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any()))
.willAnswer(invocation -> Future.succeededFuture((BidRequest) invocation.getArgument(0)));

given(ortb2RequestFactory.enrichBidRequestWithAccountAndPrivacyData(any()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ public void setUp() {
given(ortb2RequestFactory.executeRawAuctionRequestHooks(any()))
.willAnswer(invocation -> Future.succeededFuture(
((AuctionContext) invocation.getArgument(0)).getBidRequest()));
given(ortb2RequestFactory.validateRequest(any(), any(), any()))
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any()))
.willAnswer(invocationOnMock -> Future.succeededFuture((BidRequest) invocationOnMock.getArgument(0)));
given(ortb2RequestFactory.removeEmptyEids(any(), any()))
.willAnswer(invocationOnMock -> invocationOnMock.getArgument(0));
Expand Down Expand Up @@ -662,7 +662,7 @@ public void shouldReturnFailedFutureIfRequestValidationFailed() {
// given
givenValidBidRequest();

given(ortb2RequestFactory.validateRequest(any(), any(), any()))
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any()))
.willReturn(Future.failedFuture(new InvalidRequestException("errors")));

// when
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -658,14 +658,15 @@ public void enrichAuctionContextShouldSetDebugOff() {
@Test
public void validateRequestShouldThrowInvalidRequestExceptionIfRequestIsInvalid() {
// given
given(requestValidator.validate(any(), any())).willReturn(ValidationResult.error("error"));
given(requestValidator.validate(any(), any(), any())).willReturn(ValidationResult.error("error"));

final BidRequest bidRequest = givenBidRequest(identity());

// when
final Future<BidRequest> result = target.validateRequest(
bidRequest,
HttpRequestContext.builder().build(),
DebugContext.empty(),
new ArrayList<>());

// then
Expand All @@ -674,24 +675,25 @@ public void validateRequestShouldThrowInvalidRequestExceptionIfRequestIsInvalid(
.isInstanceOf(InvalidRequestException.class)
.hasMessage("error");

verify(requestValidator).validate(eq(bidRequest), any());
verify(requestValidator).validate(eq(bidRequest), any(), any());
}

@Test
public void validateRequestShouldReturnSameBidRequest() {
// given
given(requestValidator.validate(any(), any())).willReturn(ValidationResult.success());
given(requestValidator.validate(any(), any(), any())).willReturn(ValidationResult.success());

final BidRequest bidRequest = givenBidRequest(identity());

// when
final BidRequest result = target.validateRequest(
bidRequest,
HttpRequestContext.builder().build(),
DebugContext.empty(),
new ArrayList<>()).result();

// then
verify(requestValidator).validate(eq(bidRequest), any());
verify(requestValidator).validate(eq(bidRequest), any(), any());

assertThat(result).isSameAs(bidRequest);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ public void shouldReturnExpectedResultAndReturnErrors() throws JsonProcessingExc
verify(ortb2RequestFactory).createAuctionContext(any(), eq(MetricName.video));
verify(ortb2RequestFactory).enrichAuctionContext(any(), any(), eq(bidRequest), eq(0L));
verify(ortb2RequestFactory).fetchAccountWithoutStoredRequestLookup(any());
verify(ortb2RequestFactory).validateRequest(eq(bidRequest), any(), any());
verify(ortb2RequestFactory).validateRequest(eq(bidRequest), any(), any(), any());
verify(paramsResolver)
.resolve(eq(bidRequest), any(), eq(Endpoint.openrtb2_video.value()), eq(false));
verify(ortb2RequestFactory).enrichBidRequestWithAccountAndPrivacyData(
Expand Down Expand Up @@ -404,7 +404,7 @@ private void givenBidRequest(BidRequest bidRequest, List<PodError> podErrors) {
.build());
given(ortb2RequestFactory.fetchAccountWithoutStoredRequestLookup(any())).willReturn(Future.succeededFuture());

given(ortb2RequestFactory.validateRequest(any(), any(), any()))
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any()))
.willAnswer(invocation -> Future.succeededFuture((BidRequest) invocation.getArgument(0)));

given(paramsResolver.resolve(any(), any(), any(), anyBoolean()))
Expand Down
Loading
Loading