From 17ef672a10a150532608088cd5ee1c434da58f58 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Mon, 27 Jul 2020 17:13:00 +0300 Subject: [PATCH 01/14] Add bid size and secure markup validations, controlled by host through configuration --- .../server/auction/ExchangeService.java | 25 +-- .../spring/config/ServiceConfiguration.java | 8 +- .../validation/ResponseBidValidator.java | 112 +++++++++++- .../prebid/server/validation/model/Size.java | 11 ++ src/main/resources/application.yaml | 3 + .../server/auction/ExchangeServiceTest.java | 4 +- .../validation/ResponseBidValidatorTest.java | 164 +++++++++++++++--- 7 files changed, 285 insertions(+), 42 deletions(-) create mode 100644 src/main/java/org/prebid/server/validation/model/Size.java diff --git a/src/main/java/org/prebid/server/auction/ExchangeService.java b/src/main/java/org/prebid/server/auction/ExchangeService.java index c0518986d8e..674c2a72fb3 100644 --- a/src/main/java/org/prebid/server/auction/ExchangeService.java +++ b/src/main/java/org/prebid/server/auction/ExchangeService.java @@ -747,13 +747,13 @@ private Future requestBids(BidderRequest bidderRequest, Timeout Map> currencyConversionRates) { final String bidderName = bidderRequest.getBidder(); final BigDecimal bidPriceAdjustmentFactor = bidAdjustments.get(bidderName); - final List cur = bidderRequest.getBidRequest().getCur(); - final String adServerCurrency = cur.get(0); + final BidRequest bidRequest = bidderRequest.getBidRequest(); + final String adServerCurrency = bidRequest.getCur().get(0); final Bidder bidder = bidderCatalog.bidderByName(aliases.resolveBidder(bidderName)); final long startTime = clock.millis(); - return httpBidderRequester.requestBids(bidder, bidderRequest.getBidRequest(), timeout, debugEnabled) - .map(bidderSeatBid -> validBidderSeatBid(bidderSeatBid, cur)) + return httpBidderRequester.requestBids(bidder, bidRequest, timeout, debugEnabled) + .map(bidderSeatBid -> validBidderSeatBid(bidderSeatBid, bidRequest)) .map(seat -> applyBidPriceChanges(seat, currencyConversionRates, adServerCurrency, bidPriceAdjustmentFactor)) .map(result -> BidderResponse.of(bidderName, result, responseTime(startTime))); @@ -766,20 +766,21 @@ private Future requestBids(BidderRequest bidderRequest, Timeout *

* Returns input argument as the result if no errors found or create new {@link BidderSeatBid} otherwise. */ - private BidderSeatBid validBidderSeatBid(BidderSeatBid bidderSeatBid, List requestCurrencies) { - final List bids = bidderSeatBid.getBids(); - - final List validBids = new ArrayList<>(bids.size()); + private BidderSeatBid validBidderSeatBid(BidderSeatBid bidderSeatBid, BidRequest bidRequest) { final List errors = new ArrayList<>(bidderSeatBid.getErrors()); + final List requestCurrencies = bidRequest.getCur(); if (requestCurrencies.size() > 1) { errors.add(BidderError.badInput( String.format("Cur parameter contains more than one currency. %s will be used", requestCurrencies.get(0)))); } - for (BidderBid bid : bids) { - final ValidationResult validationResult = responseBidValidator.validate(bid.getBid()); + final List bids = bidderSeatBid.getBids(); + final List validBids = new ArrayList<>(bids.size()); + + for (final BidderBid bid : bids) { + final ValidationResult validationResult = responseBidValidator.validate(bid, bidRequest); if (validationResult.hasErrors()) { for (String error : validationResult.getErrors()) { errors.add(BidderError.generic(error)); @@ -796,7 +797,7 @@ private BidderSeatBid validBidderSeatBid(BidderSeatBid bidderSeatBid, List - * This method should always be invoked after {@link ExchangeService#validBidderSeatBid(BidderSeatBid, List)} + * This method should always be invoked after {@link #validBidderSeatBid} * to make sure {@link Bid#getPrice()} is not empty. */ private BidderSeatBid applyBidPriceChanges(BidderSeatBid bidderSeatBid, @@ -858,7 +859,7 @@ private Timeout auctionTimeout(Timeout timeout, boolean shouldCacheBids) { * Updates 'request_time', 'responseTime', 'timeout_request', 'error_requests', 'no_bid_requests', * 'prices' metrics for each {@link BidderResponse}. *

- * This method should always be invoked after {@link ExchangeService#validBidderSeatBid(BidderSeatBid, List)} + * This method should always be invoked after {@link #validBidderSeatBid} * to make sure {@link Bid#getPrice()} is not empty. */ private List updateMetricsFromResponses(List bidderResponses, String publisherId) { diff --git a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java index b680d7c6632..0500f60ba78 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -476,8 +476,12 @@ BidderParamValidator bidderParamValidator(BidderCatalog bidderCatalog, JacksonMa } @Bean - ResponseBidValidator responseValidator() { - return new ResponseBidValidator(); + ResponseBidValidator responseValidator( + @Value("${auction.validations.banner-creative-size}") boolean shouldValidateBanner, + @Value("${auction.validations.banner-creative-allowed-sizes:#{null}}") List bannerAllowedSizes, + @Value("${auction.validations.secure-markup}") boolean shouldValidateSecureMarkup) { + + return new ResponseBidValidator(shouldValidateBanner, bannerAllowedSizes, shouldValidateSecureMarkup); } @Bean diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index 352064b6676..d956dc93f80 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -1,26 +1,80 @@ package org.prebid.server.validation; +import com.iab.openrtb.request.BidRequest; +import com.iab.openrtb.request.Imp; import com.iab.openrtb.response.Bid; +import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.StringUtils; +import org.prebid.server.bidder.model.BidderBid; +import org.prebid.server.proto.openrtb.ext.response.BidType; +import org.prebid.server.validation.model.Size; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; +import java.util.List; +import java.util.Objects; +import java.util.stream.Collectors; /** * Validator for response {@link Bid} object. */ public class ResponseBidValidator { - public ValidationResult validate(Bid bid) { + private static final String[] INSECURE_MARKUP_MARKERS = {"http:", "http%3A"}; + + private final boolean shouldValidateBanner; + private final List bannerAllowedSizes; + private final boolean shouldValidateSecureMarkup; + + public ResponseBidValidator(boolean shouldValidateBanner, + List bannerAllowedSizes, + boolean shouldValidateSecureMarkup) { + + this.shouldValidateBanner = shouldValidateBanner; + this.bannerAllowedSizes = toSizes(bannerAllowedSizes); + this.shouldValidateSecureMarkup = shouldValidateSecureMarkup; + } + + public ValidationResult validate(BidderBid bidderBid, BidRequest bidRequest) { try { - validateFieldsFor(bid); + validateCommonFields(bidderBid.getBid()); + + if (shouldValidateBanner(bidderBid)) { + validateBannerFields(bidderBid); + } + + if (shouldValidateSecureMarkup()) { + validateSecureMarkup(bidderBid, bidRequest); + } } catch (ValidationException e) { return ValidationResult.error(e.getMessage()); } return ValidationResult.success(); } - private static void validateFieldsFor(Bid bid) throws ValidationException { + private static List toSizes(List bannerAllowedSizes) { + return CollectionUtils.emptyIfNull(bannerAllowedSizes).stream() + .map(ResponseBidValidator::parseSize) + .collect(Collectors.toList()); + } + + private static Size parseSize(String size) { + final String[] widthAndHeight = size.split("x"); + if (widthAndHeight.length != 2) { + throw new IllegalArgumentException(String.format( + "Invalid size format: %s. Should be '[width]x[height]'", size)); + } + + try { + return Size.of( + Integer.parseInt(widthAndHeight[0]), + Integer.parseInt(widthAndHeight[1])); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid size format", e); + } + } + + private static void validateCommonFields(Bid bid) throws ValidationException { if (bid == null) { throw new ValidationException("Empty bid object submitted."); } @@ -43,4 +97,56 @@ private static void validateFieldsFor(Bid bid) throws ValidationException { throw new ValidationException("Bid \"%s\" missing creative ID", bidId); } } + + private boolean shouldValidateBanner(BidderBid bidderBid) { + return bidderBid.getType() == BidType.banner && shouldValidateBanner; + } + + private void validateBannerFields(BidderBid bidderBid) throws ValidationException { + final Bid bid = bidderBid.getBid(); + + if (bannerSizeIsNotValid(bid)) { + throw new ValidationException( + "Bid \"%s\" has 'w' and 'h' that are not valid. Bid dimensions: '%dx%d'", + bid.getId(), bid.getW(), bid.getH()); + } + } + + private boolean bannerSizeIsNotValid(Bid bid) { + return bannerAllowedSizes.stream().noneMatch(size -> bidHasEqualSize(bid, size)); + } + + private static boolean bidHasEqualSize(Bid bid, Size size) { + return Objects.equals(bid.getW(), size.getWidth()) && Objects.equals(bid.getH(), size.getHeight()); + } + + private boolean shouldValidateSecureMarkup() { + return shouldValidateSecureMarkup; + } + + private static void validateSecureMarkup(BidderBid bidderBid, BidRequest bidRequest) throws ValidationException { + final Bid bid = bidderBid.getBid(); + final Imp imp = findCorrespondingImp(bidRequest, bidderBid.getBid()); + + if (isImpSecure(imp) && markupIsNotSecure(bid)) { + throw new ValidationException( + "Bid \"%s\" has has insecure creative but should be in secure context", bid.getId()); + } + } + + private static Imp findCorrespondingImp(BidRequest bidRequest, Bid bid) throws ValidationException { + return bidRequest.getImp().stream() + .filter(curImp -> Objects.equals(curImp.getId(), bid.getImpid())) + .findFirst() + .orElseThrow(() -> new ValidationException( + "Bid \"%s\" has no corresponding imp in request", bid.getId())); + } + + public static boolean isImpSecure(Imp imp) { + return Objects.equals(imp.getSecure(), 1); + } + + private static boolean markupIsNotSecure(Bid bid) { + return StringUtils.containsAny(bid.getAdm(), INSECURE_MARKUP_MARKERS); + } } diff --git a/src/main/java/org/prebid/server/validation/model/Size.java b/src/main/java/org/prebid/server/validation/model/Size.java new file mode 100644 index 00000000000..ddeb6da363c --- /dev/null +++ b/src/main/java/org/prebid/server/validation/model/Size.java @@ -0,0 +1,11 @@ +package org.prebid.server.validation.model; + +import lombok.Value; + +@Value(staticConstructor = "of") +public class Size { + + Integer width; + + Integer height; +} diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index 98fd3ba2141..1cb7e296294 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -42,6 +42,9 @@ auction: cache: expected-request-time-ms: 10 only-winning-bids: false + validations: + banner-creative-size: false + secure-markup: false video: stored-requests-timeout-ms: 90 amp: diff --git a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java index e9128ad6e80..afc66b258bf 100644 --- a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java +++ b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java @@ -181,7 +181,7 @@ public void setUp() { given(privacyEnforcementService.mask(any(), argThat(MapUtils::isEmpty), any(), any(), any())) .willReturn(Future.succeededFuture(emptyList())); - given(responseBidValidator.validate(any())).willReturn(ValidationResult.success()); + given(responseBidValidator.validate(any(), any())).willReturn(ValidationResult.success()); given(usersyncer.getCookieFamilyName()).willReturn("cookieFamily"); given(currencyService.convertCurrency(any(), any(), any(), any())) @@ -918,7 +918,7 @@ public void shouldTolerateResponseBidValidationErrors() throws JsonProcessingExc .auctiontimestamp(1000L) .build()))); - given(responseBidValidator.validate(any())) + given(responseBidValidator.validate(any(), any())) .willReturn(ValidationResult.error("bid validation error")); final List bidderErrors = singletonList(ExtBidderError.of(BidderError.Type.generic.getCode(), diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 62e778d8b2a..66317b7e985 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -1,87 +1,205 @@ package org.prebid.server.validation; +import com.iab.openrtb.request.BidRequest; +import com.iab.openrtb.request.Imp; import com.iab.openrtb.response.Bid; import org.junit.Before; import org.junit.Test; +import org.prebid.server.bidder.model.BidderBid; +import org.prebid.server.proto.openrtb.ext.response.BidType; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; +import java.util.List; import java.util.function.Function; +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; import static java.util.function.Function.identity; import static org.assertj.core.api.Assertions.assertThat; public class ResponseBidValidatorTest { + private static final List BANNER_ALLOWED_SIZES = asList("1x1", "2x2"); + private ResponseBidValidator responseBidValidator; @Before public void setUp() { - responseBidValidator = new ResponseBidValidator(); + responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, true); } @Test - public void validateShouldFailedIfMissingBid() { - final ValidationResult result = responseBidValidator.validate(null); + public void validateShouldFailIfMissingBid() { + final ValidationResult result = responseBidValidator.validate( + BidderBid.of(null, null, null), givenBidRequest(identity())); - assertThat(result.getErrors()).hasSize(1) + assertThat(result.getErrors()) .containsOnly("Empty bid object submitted."); } @Test - public void validateShouldFailedIfBidHasNoId() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.id(null))); + public void validateShouldFailIfBidHasNoId() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.id(null)), givenBidRequest(identity())); - assertThat(result.getErrors()).hasSize(1) + assertThat(result.getErrors()) .containsOnly("Bid missing required field 'id'"); } @Test - public void validateShouldFailedIfBidHasNoImpId() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.impid(null))); + public void validateShouldFailIfBidHasNoImpId() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.impid(null)), givenBidRequest(identity())); - assertThat(result.getErrors()).hasSize(1) + assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" missing required field 'impid'"); } @Test - public void validateShouldFailedIfBidHasNoPrice() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.price(null))); + public void validateShouldFailIfBidHasNoPrice() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.price(null)), givenBidRequest(identity())); - assertThat(result.getErrors()).hasSize(1) + assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); } @Test - public void validateShouldFailedIfBidHasNegativePrice() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.price( - BigDecimal.valueOf(-1)))); + public void validateShouldFailIfBidHasNegativePrice() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.price(BigDecimal.valueOf(-1))), givenBidRequest(identity())); - assertThat(result.getErrors()).hasSize(1) + assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); } @Test - public void validateShouldFailedIfBidHasNoCrid() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.crid(null))); + public void validateShouldFailIfBidHasNoCrid() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.crid(null)), givenBidRequest(identity())); - assertThat(result.getErrors()).hasSize(1) + assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" missing creative ID"); } + @Test + public void validateShouldFailIfBannerBidHasNoWidthAndHeight() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.w(null).h(null)), givenBidRequest(identity())); + + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: 'nullxnull'"); + } + + @Test + public void validateShouldFailIfBannerBidHasNotAllowedWidthAndHeight() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.w(3).h(3)), givenBidRequest(identity())); + + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: '3x3'"); + } + + @Test + public void validateShouldReturnSuccessIfNonBannerBidHasAnySize() { + final ValidationResult result = responseBidValidator.validate( + givenBid(BidType.video, builder -> builder.w(3).h(3)), givenBidRequest(identity())); + + assertThat(result.hasErrors()).isFalse(); + } + + @Test + public void validateShouldFailIfBidHasNoCorrespondingImp() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.impid("nonExistentsImpid")), givenBidRequest(identity())); + + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has no corresponding imp in request"); + } + + @Test + public void validateShouldFailIfBidHasInsecureCreativeInSecureContext() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), + givenBidRequest(builder -> builder.secure(1))); + + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); + } + + @Test + public void validateShouldFailIfBidHasInsecureEncodedCreativeInSecureContext() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm(">http%3A//site.com/creative.jpg")), + givenBidRequest(builder -> builder.secure(1))); + + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); + } + + @Test + public void validateShouldReturnSuccessIfBidHasInsecureCreativeInInsecureContext() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), + givenBidRequest(identity())); + + assertThat(result.hasErrors()).isFalse(); + } + @Test public void validateShouldReturnSuccessfulResultForValidBid() { - final ValidationResult result = responseBidValidator.validate(givenBid(identity())); + final ValidationResult result = + responseBidValidator.validate(givenBid(identity()), givenBidRequest(builder -> builder.secure(1))); + + assertThat(result.hasErrors()).isFalse(); + } + + @Test + public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { + responseBidValidator = new ResponseBidValidator(false, null, true); + + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.w(null).h(null)), + givenBidRequest(identity())); assertThat(result.hasErrors()).isFalse(); } - private static Bid givenBid(Function bidCustomizer) { + @Test + public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { + responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, false); + + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), + givenBidRequest(builder -> builder.secure(1))); + + assertThat(result.hasErrors()).isFalse(); + } + + private BidRequest givenBidRequest(Function impCustomizer) { + final Imp.ImpBuilder impBuilder = Imp.builder() + .id("impId1"); + + return BidRequest.builder() + .imp(singletonList(impCustomizer.apply(impBuilder).build())) + .build(); + } + + private static BidderBid givenBid(Function bidCustomizer) { + return givenBid(BidType.banner, bidCustomizer); + } + + private static BidderBid givenBid(BidType type, Function bidCustomizer) { final Bid.BidBuilder bidBuilder = Bid.builder() .id("bidId1") .impid("impId1") .crid("crid1") + .w(1) + .h(1) + .adm("https://site.com/creative.jpg") .price(BigDecimal.ONE); - return bidCustomizer.apply(bidBuilder).build(); + + return BidderBid.of(bidCustomizer.apply(bidBuilder).build(), type, null); } } From e17bff57a99e7c2bc441a215640c0f653c8361bc Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Tue, 28 Jul 2020 12:12:47 +0300 Subject: [PATCH 02/14] Add account-level bid validations overrides --- docs/application-settings.md | 14 ++++++++++++-- .../server/settings/JdbcApplicationSettings.java | 15 +++++++++------ .../org/prebid/server/settings/model/Account.java | 2 ++ .../model/AccountBidValidationConfig.java | 13 +++++++++++++ .../settings/FileApplicationSettingsTest.java | 8 ++++++-- .../settings/JdbcApplicationSettingsTest.java | 10 +++++++--- 6 files changed, 49 insertions(+), 13 deletions(-) create mode 100644 src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java diff --git a/docs/application-settings.md b/docs/application-settings.md index 53fb910facc..6b2c704644a 100644 --- a/docs/application-settings.md +++ b/docs/application-settings.md @@ -17,6 +17,7 @@ There are two ways to configure application settings: database and file. This do - `gdpr.purpose-one-treatment-interpretation` - option that allows to skip the Purpose one enforcement workflow. Values: ignore, no-access-allowed, access-allowed. - `analytics-sampling-factor` - Analytics sampling factor value. - `truncate-target-attr` - Maximum targeting attributes size. Values between 1 and 255. +- `bid-validations.banner-creative-allowed-sizes` - Defines valid banner sizes. ``` Purpose | Purpose goal | Purpose meaning for PBS (n\a - not affected) @@ -170,6 +171,7 @@ Query to create accounts_account table: `tcf_config` json DEFAULT NULL, `analytics_sampling_factor` tinyint(4) DEFAULT NULL, `truncate_target_attr` tinyint(3) unsigned DEFAULT NULL, +`bid_validations` json DEFAULT NULL, `status` enum('active','inactive') DEFAULT 'active', `updated_by` int(11) DEFAULT NULL, `updated_by_user` varchar(64) DEFAULT NULL, @@ -181,7 +183,7 @@ ENGINE=InnoDB AUTO_INCREMENT=1726 DEFAULT CHARSET=utf8' where tcf_config column is json with next format -``` +```json { "enabled": true, "purpose-one-treatment-interpretation": "ignore" @@ -286,9 +288,17 @@ where tcf_config column is json with next format } ``` +and bid_validations column is json with next format + +```json +{ + "banner-creative-allowed-sizes": ["1x1", "2x2"] +} +``` + Query used to get an account: ``` -SELECT uuid, price_granularity, banner_cache_ttl, video_cache_ttl, events_enabled, enforce_ccpa, tcf_config, analytics_sampling_factor, truncate_target_attr +SELECT uuid, price_granularity, banner_cache_ttl, video_cache_ttl, events_enabled, enforce_ccpa, tcf_config, analytics_sampling_factor, truncate_target_attr, bid_validations FROM accounts_account where uuid = ? LIMIT 1 diff --git a/src/main/java/org/prebid/server/settings/JdbcApplicationSettings.java b/src/main/java/org/prebid/server/settings/JdbcApplicationSettings.java index 1a6536d4f86..900d0c0b940 100644 --- a/src/main/java/org/prebid/server/settings/JdbcApplicationSettings.java +++ b/src/main/java/org/prebid/server/settings/JdbcApplicationSettings.java @@ -12,6 +12,7 @@ import org.prebid.server.settings.mapper.JdbcStoredDataResultMapper; import org.prebid.server.settings.mapper.JdbcStoredResponseResultMapper; import org.prebid.server.settings.model.Account; +import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; import org.prebid.server.settings.model.StoredDataResult; import org.prebid.server.settings.model.StoredResponseDataResult; @@ -96,9 +97,10 @@ public JdbcApplicationSettings(JdbcClient jdbcClient, */ @Override public Future getAccountById(String accountId, Timeout timeout) { - return jdbcClient.executeQuery("SELECT uuid, price_granularity, banner_cache_ttl, video_cache_ttl," - + " events_enabled, enforce_ccpa, tcf_config, analytics_sampling_factor, truncate_target_attr" - + " FROM accounts_account where uuid = ? LIMIT 1", + return jdbcClient.executeQuery("SELECT uuid, price_granularity, banner_cache_ttl, video_cache_ttl, " + + "events_enabled, enforce_ccpa, tcf_config, analytics_sampling_factor, truncate_target_attr, " + + "bid_validations " + + "FROM accounts_account where uuid = ? LIMIT 1", Collections.singletonList(accountId), result -> mapToModelOrError(result, row -> Account.builder() .id(row.getString(0)) @@ -107,9 +109,10 @@ public Future getAccountById(String accountId, Timeout timeout) { .videoCacheTtl(row.getInteger(3)) .eventsEnabled(row.getBoolean(4)) .enforceCcpa(row.getBoolean(5)) - .gdpr(toAccountTcfConfig(row.getString(6))) + .gdpr(jsonToModel(row.getString(6), AccountGdprConfig.class)) .analyticsSamplingFactor(row.getInteger(7)) .truncateTargetAttr(row.getInteger(8)) + .bidValidations(jsonToModel(row.getString(9), AccountBidValidationConfig.class)) .build()), timeout) .compose(result -> failedIfNull(result, accountId, "Account")); @@ -150,9 +153,9 @@ private static Future failedIfNull(T value, String id, String errorPrefix : Future.failedFuture(new PreBidException(String.format("%s not found: %s", errorPrefix, id))); } - private AccountGdprConfig toAccountTcfConfig(String tcfConfig) { + private T jsonToModel(String json, Class modelClass) { try { - return tcfConfig != null ? mapper.decodeValue(tcfConfig, AccountGdprConfig.class) : null; + return json != null ? mapper.decodeValue(json, modelClass) : null; } catch (DecodeException e) { throw new PreBidException(e.getMessage()); } diff --git a/src/main/java/org/prebid/server/settings/model/Account.java b/src/main/java/org/prebid/server/settings/model/Account.java index 90edc10d2fc..b64e92279ed 100644 --- a/src/main/java/org/prebid/server/settings/model/Account.java +++ b/src/main/java/org/prebid/server/settings/model/Account.java @@ -25,6 +25,8 @@ public class Account { Integer truncateTargetAttr; + AccountBidValidationConfig bidValidations; + public static Account empty(String id) { return Account.builder() .id(id) diff --git a/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java new file mode 100644 index 00000000000..30a786c6199 --- /dev/null +++ b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java @@ -0,0 +1,13 @@ +package org.prebid.server.settings.model; + +import com.fasterxml.jackson.annotation.JsonProperty; +import lombok.Value; + +import java.util.List; + +@Value(staticConstructor = "of") +public class AccountBidValidationConfig { + + @JsonProperty("banner-creative-allowed-sizes") + List bannerCreativeAllowedSizes; +} diff --git a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java index 90ddda6e10a..bed701852d5 100644 --- a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java @@ -9,6 +9,7 @@ import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; import org.prebid.server.settings.model.Account; +import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; import org.prebid.server.settings.model.EnforcePurpose; import org.prebid.server.settings.model.Purpose; @@ -91,7 +92,10 @@ public void getAccountByIdShouldReturnPresentAccount() { + "}," + "purpose-one-treatment-interpretation: access-allowed" + "}," - + "analyticsSamplingFactor : '1'" + + "analyticsSamplingFactor : '1'," + + "bidValidations: {" + + "banner-creative-allowed-sizes: ['1x1', '2x2']" + + "}" + "}" + "]")); @@ -123,8 +127,8 @@ public void getAccountByIdShouldReturnPresentAccount() { .purposeOneTreatmentInterpretation(PurposeOneTreatmentInterpretation.accessAllowed) .build()) .analyticsSamplingFactor(1) + .bidValidations(AccountBidValidationConfig.of(asList("1x1", "2x2"))) .build()); - } @Test diff --git a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java index ad877ce02e5..e85c13c5a25 100644 --- a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java @@ -23,6 +23,7 @@ import org.prebid.server.execution.TimeoutFactory; import org.prebid.server.metric.Metrics; import org.prebid.server.settings.model.Account; +import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; import org.prebid.server.settings.model.StoredDataResult; import org.prebid.server.settings.model.StoredResponseDataResult; @@ -99,7 +100,8 @@ public static void beforeClass() throws SQLException { connection.createStatement().execute("CREATE TABLE accounts_account (id SERIAL PRIMARY KEY, " + "uuid varchar(40) NOT NULL, price_granularity varchar(6), granularityMultiplier numeric(9,3), " + "banner_cache_ttl INT, video_cache_ttl INT, events_enabled BIT, enforce_ccpa BIT, " - + "tcf_config varchar(512), analytics_sampling_factor INT, truncate_target_attr INT);"); + + "tcf_config varchar(512), analytics_sampling_factor INT, truncate_target_attr INT, " + + "bid_validations varchar(512));"); connection.createStatement().execute("CREATE TABLE s2sconfig_config (id SERIAL PRIMARY KEY, uuid varchar(40) " + "NOT NULL, config varchar(512));"); connection.createStatement().execute("CREATE TABLE stored_requests (id SERIAL PRIMARY KEY, reqid varchar(40) " @@ -117,8 +119,9 @@ public static void beforeClass() throws SQLException { + " NOT NULL);"); connection.createStatement().execute("insert into accounts_account " + "(uuid, price_granularity, banner_cache_ttl, video_cache_ttl, events_enabled, enforce_ccpa, " - + "tcf_config, analytics_sampling_factor, truncate_target_attr) " - + "values ('accountId','med', 100, 100, TRUE, TRUE, '{\"enabled\": true}', 1, 0);"); + + "tcf_config, analytics_sampling_factor, truncate_target_attr, bid_validations) " + + "values ('accountId','med', 100, 100, TRUE, TRUE, '{\"enabled\": true}', 1, 0, " + + "'{\"banner-creative-allowed-sizes\": [\"1x1\", \"2x2\"]}');"); connection.createStatement().execute("insert into s2sconfig_config (uuid, config)" + " values ('adUnitConfigId', 'config');"); connection.createStatement().execute("insert into stored_requests (reqid, requestData) values ('1','value1');"); @@ -174,6 +177,7 @@ public void getAccountByIdShouldReturnAccountWithAllFieldsPopulated(TestContext .enabled(true) .build()) .truncateTargetAttr(0) + .bidValidations(AccountBidValidationConfig.of(asList("1x1", "2x2"))) .build()); async.complete(); })); From 74753f39f3bf68f8ffc7f97565ae13c7debef5a0 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Tue, 28 Jul 2020 13:38:25 +0300 Subject: [PATCH 03/14] Apply account level overrides for bid validations --- .../server/auction/ExchangeService.java | 30 ++++--- .../model/AccountBidValidationConfig.java | 3 +- .../spring/config/ServiceConfiguration.java | 5 +- .../validation/ResponseBidValidator.java | 53 +++++------ .../prebid/server/validation/model/Size.java | 31 +++++++ .../server/auction/ExchangeServiceTest.java | 4 +- .../settings/FileApplicationSettingsTest.java | 3 +- .../settings/JdbcApplicationSettingsTest.java | 3 +- .../validation/ResponseBidValidatorTest.java | 90 +++++++++++++------ 9 files changed, 151 insertions(+), 71 deletions(-) diff --git a/src/main/java/org/prebid/server/auction/ExchangeService.java b/src/main/java/org/prebid/server/auction/ExchangeService.java index 674c2a72fb3..57607aa4786 100644 --- a/src/main/java/org/prebid/server/auction/ExchangeService.java +++ b/src/main/java/org/prebid/server/auction/ExchangeService.java @@ -154,11 +154,17 @@ public Future holdAuction(AuctionContext context) { extractBidderRequests(context, impsRequiredRequest, aliases)) .map(bidderRequests -> updateRequestMetric(bidderRequests, uidsCookie, aliases, publisherId, requestTypeMetric)) - .compose(bidderRequests -> CompositeFuture.join(bidderRequests.stream() - .map(bidderRequest -> requestBids(bidderRequest, - auctionTimeout(timeout, cacheInfo.isDoCaching()), debugEnabled, aliases, - bidAdjustments(requestExt), currencyRates(requestExt))) - .collect(Collectors.toList()))) + .compose(bidderRequests -> CompositeFuture.join( + bidderRequests.stream() + .map(bidderRequest -> requestBids( + bidderRequest, + account, + auctionTimeout(timeout, cacheInfo.isDoCaching()), + debugEnabled, + aliases, + bidAdjustments(requestExt), + currencyRates(requestExt))) + .collect(Collectors.toList()))) // send all the requests to the bidders and gathers results .map(CompositeFuture::list) // produce response from bidder results @@ -741,10 +747,14 @@ private static Map bidAdjustments(ExtRequest requestExt) { * Passes the request to a corresponding bidder and wraps response in {@link BidderResponse} which also holds * recorded response time. */ - private Future requestBids(BidderRequest bidderRequest, Timeout timeout, - boolean debugEnabled, BidderAliases aliases, + private Future requestBids(BidderRequest bidderRequest, + Account account, + Timeout timeout, + boolean debugEnabled, + BidderAliases aliases, Map bidAdjustments, Map> currencyConversionRates) { + final String bidderName = bidderRequest.getBidder(); final BigDecimal bidPriceAdjustmentFactor = bidAdjustments.get(bidderName); final BidRequest bidRequest = bidderRequest.getBidRequest(); @@ -753,7 +763,7 @@ private Future requestBids(BidderRequest bidderRequest, Timeout final long startTime = clock.millis(); return httpBidderRequester.requestBids(bidder, bidRequest, timeout, debugEnabled) - .map(bidderSeatBid -> validBidderSeatBid(bidderSeatBid, bidRequest)) + .map(bidderSeatBid -> validBidderSeatBid(bidderSeatBid, bidRequest, account)) .map(seat -> applyBidPriceChanges(seat, currencyConversionRates, adServerCurrency, bidPriceAdjustmentFactor)) .map(result -> BidderResponse.of(bidderName, result, responseTime(startTime))); @@ -766,7 +776,7 @@ private Future requestBids(BidderRequest bidderRequest, Timeout *

* Returns input argument as the result if no errors found or create new {@link BidderSeatBid} otherwise. */ - private BidderSeatBid validBidderSeatBid(BidderSeatBid bidderSeatBid, BidRequest bidRequest) { + private BidderSeatBid validBidderSeatBid(BidderSeatBid bidderSeatBid, BidRequest bidRequest, Account account) { final List errors = new ArrayList<>(bidderSeatBid.getErrors()); final List requestCurrencies = bidRequest.getCur(); @@ -780,7 +790,7 @@ private BidderSeatBid validBidderSeatBid(BidderSeatBid bidderSeatBid, BidRequest final List validBids = new ArrayList<>(bids.size()); for (final BidderBid bid : bids) { - final ValidationResult validationResult = responseBidValidator.validate(bid, bidRequest); + final ValidationResult validationResult = responseBidValidator.validate(bid, bidRequest, account); if (validationResult.hasErrors()) { for (String error : validationResult.getErrors()) { errors.add(BidderError.generic(error)); diff --git a/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java index 30a786c6199..b5b8151e1ba 100644 --- a/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java +++ b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java @@ -2,6 +2,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import lombok.Value; +import org.prebid.server.validation.model.Size; import java.util.List; @@ -9,5 +10,5 @@ public class AccountBidValidationConfig { @JsonProperty("banner-creative-allowed-sizes") - List bannerCreativeAllowedSizes; + List bannerCreativeAllowedSizes; } diff --git a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java index 0500f60ba78..34c20358cb4 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -479,9 +479,10 @@ BidderParamValidator bidderParamValidator(BidderCatalog bidderCatalog, JacksonMa ResponseBidValidator responseValidator( @Value("${auction.validations.banner-creative-size}") boolean shouldValidateBanner, @Value("${auction.validations.banner-creative-allowed-sizes:#{null}}") List bannerAllowedSizes, - @Value("${auction.validations.secure-markup}") boolean shouldValidateSecureMarkup) { + @Value("${auction.validations.secure-markup}") boolean shouldValidateSecureMarkup, + JacksonMapper mapper) { - return new ResponseBidValidator(shouldValidateBanner, bannerAllowedSizes, shouldValidateSecureMarkup); + return new ResponseBidValidator(shouldValidateBanner, bannerAllowedSizes, shouldValidateSecureMarkup, mapper); } @Bean diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index d956dc93f80..f4dc37ed125 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -6,7 +6,11 @@ import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.StringUtils; import org.prebid.server.bidder.model.BidderBid; +import org.prebid.server.json.DecodeException; +import org.prebid.server.json.JacksonMapper; import org.prebid.server.proto.openrtb.ext.response.BidType; +import org.prebid.server.settings.model.Account; +import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.validation.model.Size; import org.prebid.server.validation.model.ValidationResult; @@ -25,25 +29,28 @@ public class ResponseBidValidator { private final boolean shouldValidateBanner; private final List bannerAllowedSizes; private final boolean shouldValidateSecureMarkup; + private final JacksonMapper mapper; public ResponseBidValidator(boolean shouldValidateBanner, List bannerAllowedSizes, - boolean shouldValidateSecureMarkup) { + boolean shouldValidateSecureMarkup, + JacksonMapper mapper) { + this.mapper = Objects.requireNonNull(mapper); this.shouldValidateBanner = shouldValidateBanner; this.bannerAllowedSizes = toSizes(bannerAllowedSizes); this.shouldValidateSecureMarkup = shouldValidateSecureMarkup; } - public ValidationResult validate(BidderBid bidderBid, BidRequest bidRequest) { + public ValidationResult validate(BidderBid bidderBid, BidRequest bidRequest, Account account) { try { validateCommonFields(bidderBid.getBid()); if (shouldValidateBanner(bidderBid)) { - validateBannerFields(bidderBid); + validateBannerFields(bidderBid, account); } - if (shouldValidateSecureMarkup()) { + if (shouldValidateSecureMarkup) { validateSecureMarkup(bidderBid, bidRequest); } } catch (ValidationException e) { @@ -52,24 +59,16 @@ public ValidationResult validate(BidderBid bidderBid, BidRequest bidRequest) { return ValidationResult.success(); } - private static List toSizes(List bannerAllowedSizes) { + private List toSizes(List bannerAllowedSizes) { return CollectionUtils.emptyIfNull(bannerAllowedSizes).stream() - .map(ResponseBidValidator::parseSize) + .map(this::parseSize) .collect(Collectors.toList()); } - private static Size parseSize(String size) { - final String[] widthAndHeight = size.split("x"); - if (widthAndHeight.length != 2) { - throw new IllegalArgumentException(String.format( - "Invalid size format: %s. Should be '[width]x[height]'", size)); - } - + private Size parseSize(String size) { try { - return Size.of( - Integer.parseInt(widthAndHeight[0]), - Integer.parseInt(widthAndHeight[1])); - } catch (NumberFormatException e) { + return mapper.decodeValue(StringUtils.wrap(size, '\"'), Size.class); + } catch (DecodeException e) { throw new IllegalArgumentException("Invalid size format", e); } } @@ -102,26 +101,30 @@ private boolean shouldValidateBanner(BidderBid bidderBid) { return bidderBid.getType() == BidType.banner && shouldValidateBanner; } - private void validateBannerFields(BidderBid bidderBid) throws ValidationException { + private void validateBannerFields(BidderBid bidderBid, Account account) throws ValidationException { final Bid bid = bidderBid.getBid(); - if (bannerSizeIsNotValid(bid)) { + if (bannerSizeIsNotValid(bid, validBannerSizes(account))) { throw new ValidationException( "Bid \"%s\" has 'w' and 'h' that are not valid. Bid dimensions: '%dx%d'", bid.getId(), bid.getW(), bid.getH()); } } - private boolean bannerSizeIsNotValid(Bid bid) { - return bannerAllowedSizes.stream().noneMatch(size -> bidHasEqualSize(bid, size)); + private List validBannerSizes(Account account) { + final AccountBidValidationConfig validationConfig = account.getBidValidations(); + final List accountValidBannerSizes = + validationConfig != null ? validationConfig.getBannerCreativeAllowedSizes() : null; + + return accountValidBannerSizes != null ? accountValidBannerSizes : bannerAllowedSizes; } - private static boolean bidHasEqualSize(Bid bid, Size size) { - return Objects.equals(bid.getW(), size.getWidth()) && Objects.equals(bid.getH(), size.getHeight()); + private static boolean bannerSizeIsNotValid(Bid bid, List validBannerSizes) { + return validBannerSizes.stream().noneMatch(size -> bidHasEqualSize(bid, size)); } - private boolean shouldValidateSecureMarkup() { - return shouldValidateSecureMarkup; + private static boolean bidHasEqualSize(Bid bid, Size size) { + return Objects.equals(bid.getW(), size.getWidth()) && Objects.equals(bid.getH(), size.getHeight()); } private static void validateSecureMarkup(BidderBid bidderBid, BidRequest bidRequest) throws ValidationException { diff --git a/src/main/java/org/prebid/server/validation/model/Size.java b/src/main/java/org/prebid/server/validation/model/Size.java index ddeb6da363c..771500e783c 100644 --- a/src/main/java/org/prebid/server/validation/model/Size.java +++ b/src/main/java/org/prebid/server/validation/model/Size.java @@ -1,11 +1,42 @@ package org.prebid.server.validation.model; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonDeserializer; +import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import lombok.Value; +import java.io.IOException; + @Value(staticConstructor = "of") +@JsonDeserialize(using = Size.SizeDeserializer.class) public class Size { Integer width; Integer height; + + public static class SizeDeserializer extends JsonDeserializer { + + @Override + public Size deserialize(JsonParser parser, DeserializationContext ctx) throws IOException { + final String sizeAsString = parser.getValueAsString(); + + final String[] widthAndHeight = sizeAsString.split("x"); + if (widthAndHeight.length != 2) { + throw new JsonMappingException( + parser, + String.format("Invalid size format: %s. Should be '[width]x[height]'", sizeAsString)); + } + + try { + return Size.of( + Integer.parseInt(widthAndHeight[0]), + Integer.parseInt(widthAndHeight[1])); + } catch (NumberFormatException e) { + throw new JsonMappingException(parser, "Invalid size format", e); + } + } + } } diff --git a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java index afc66b258bf..601dff26df0 100644 --- a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java +++ b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java @@ -181,7 +181,7 @@ public void setUp() { given(privacyEnforcementService.mask(any(), argThat(MapUtils::isEmpty), any(), any(), any())) .willReturn(Future.succeededFuture(emptyList())); - given(responseBidValidator.validate(any(), any())).willReturn(ValidationResult.success()); + given(responseBidValidator.validate(any(), any(), any())).willReturn(ValidationResult.success()); given(usersyncer.getCookieFamilyName()).willReturn("cookieFamily"); given(currencyService.convertCurrency(any(), any(), any(), any())) @@ -918,7 +918,7 @@ public void shouldTolerateResponseBidValidationErrors() throws JsonProcessingExc .auctiontimestamp(1000L) .build()))); - given(responseBidValidator.validate(any(), any())) + given(responseBidValidator.validate(any(), any(), any())) .willReturn(ValidationResult.error("bid validation error")); final List bidderErrors = singletonList(ExtBidderError.of(BidderError.Type.generic.getCode(), diff --git a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java index bed701852d5..07c236be297 100644 --- a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java @@ -19,6 +19,7 @@ import org.prebid.server.settings.model.SpecialFeatures; import org.prebid.server.settings.model.StoredDataResult; import org.prebid.server.settings.model.StoredResponseDataResult; +import org.prebid.server.validation.model.Size; import java.util.HashSet; @@ -127,7 +128,7 @@ public void getAccountByIdShouldReturnPresentAccount() { .purposeOneTreatmentInterpretation(PurposeOneTreatmentInterpretation.accessAllowed) .build()) .analyticsSamplingFactor(1) - .bidValidations(AccountBidValidationConfig.of(asList("1x1", "2x2"))) + .bidValidations(AccountBidValidationConfig.of(asList(Size.of(1, 1), Size.of(2, 2)))) .build()); } diff --git a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java index e85c13c5a25..3102f8f7fb3 100644 --- a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java @@ -27,6 +27,7 @@ import org.prebid.server.settings.model.AccountGdprConfig; import org.prebid.server.settings.model.StoredDataResult; import org.prebid.server.settings.model.StoredResponseDataResult; +import org.prebid.server.validation.model.Size; import org.prebid.server.vertx.jdbc.BasicJdbcClient; import org.prebid.server.vertx.jdbc.JdbcClient; @@ -177,7 +178,7 @@ public void getAccountByIdShouldReturnAccountWithAllFieldsPopulated(TestContext .enabled(true) .build()) .truncateTargetAttr(0) - .bidValidations(AccountBidValidationConfig.of(asList("1x1", "2x2"))) + .bidValidations(AccountBidValidationConfig.of(asList(Size.of(1, 1), Size.of(2, 2)))) .build()); async.complete(); })); diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 66317b7e985..6fdb03c99d2 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -5,8 +5,12 @@ import com.iab.openrtb.response.Bid; import org.junit.Before; import org.junit.Test; +import org.prebid.server.VertxTest; import org.prebid.server.bidder.model.BidderBid; import org.prebid.server.proto.openrtb.ext.response.BidType; +import org.prebid.server.settings.model.Account; +import org.prebid.server.settings.model.AccountBidValidationConfig; +import org.prebid.server.validation.model.Size; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; @@ -18,7 +22,7 @@ import static java.util.function.Function.identity; import static org.assertj.core.api.Assertions.assertThat; -public class ResponseBidValidatorTest { +public class ResponseBidValidatorTest extends VertxTest { private static final List BANNER_ALLOWED_SIZES = asList("1x1", "2x2"); @@ -26,13 +30,13 @@ public class ResponseBidValidatorTest { @Before public void setUp() { - responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, true); + responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, true, jacksonMapper); } @Test public void validateShouldFailIfMissingBid() { final ValidationResult result = responseBidValidator.validate( - BidderBid.of(null, null, null), givenBidRequest(identity())); + BidderBid.of(null, null, null), givenBidRequest(identity()), givenAccount()); assertThat(result.getErrors()) .containsOnly("Empty bid object submitted."); @@ -41,7 +45,7 @@ public void validateShouldFailIfMissingBid() { @Test public void validateShouldFailIfBidHasNoId() { final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.id(null)), givenBidRequest(identity())); + givenBid(builder -> builder.id(null)), givenBidRequest(identity()), givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid missing required field 'id'"); @@ -50,7 +54,7 @@ public void validateShouldFailIfBidHasNoId() { @Test public void validateShouldFailIfBidHasNoImpId() { final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.impid(null)), givenBidRequest(identity())); + givenBid(builder -> builder.impid(null)), givenBidRequest(identity()), givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" missing required field 'impid'"); @@ -59,7 +63,7 @@ public void validateShouldFailIfBidHasNoImpId() { @Test public void validateShouldFailIfBidHasNoPrice() { final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.price(null)), givenBidRequest(identity())); + givenBid(builder -> builder.price(null)), givenBidRequest(identity()), givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); @@ -68,7 +72,8 @@ public void validateShouldFailIfBidHasNoPrice() { @Test public void validateShouldFailIfBidHasNegativePrice() { final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.price(BigDecimal.valueOf(-1))), givenBidRequest(identity())); + givenBid(builder -> builder.price(BigDecimal.valueOf(-1))), givenBidRequest(identity()), + givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); @@ -77,7 +82,7 @@ public void validateShouldFailIfBidHasNegativePrice() { @Test public void validateShouldFailIfBidHasNoCrid() { final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.crid(null)), givenBidRequest(identity())); + givenBid(builder -> builder.crid(null)), givenBidRequest(identity()), givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" missing creative ID"); @@ -86,7 +91,7 @@ public void validateShouldFailIfBidHasNoCrid() { @Test public void validateShouldFailIfBannerBidHasNoWidthAndHeight() { final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(null).h(null)), givenBidRequest(identity())); + givenBid(builder -> builder.w(null).h(null)), givenBidRequest(identity()), givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: 'nullxnull'"); @@ -95,7 +100,7 @@ public void validateShouldFailIfBannerBidHasNoWidthAndHeight() { @Test public void validateShouldFailIfBannerBidHasNotAllowedWidthAndHeight() { final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(3).h(3)), givenBidRequest(identity())); + givenBid(builder -> builder.w(3).h(3)), givenBidRequest(identity()), givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: '3x3'"); @@ -104,7 +109,18 @@ public void validateShouldFailIfBannerBidHasNotAllowedWidthAndHeight() { @Test public void validateShouldReturnSuccessIfNonBannerBidHasAnySize() { final ValidationResult result = responseBidValidator.validate( - givenBid(BidType.video, builder -> builder.w(3).h(3)), givenBidRequest(identity())); + givenBid(BidType.video, builder -> builder.w(3).h(3)), givenBidRequest(identity()), givenAccount()); + + assertThat(result.hasErrors()).isFalse(); + } + + @Test + public void validateShouldReturnSuccessIfBannerBidHasAllowedByAccountWidthAndHeight() { + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.w(3).h(3)), + givenBidRequest(identity()), + givenAccount(builder -> builder.bidValidations( + AccountBidValidationConfig.of(singletonList(Size.of(3, 3)))))); assertThat(result.hasErrors()).isFalse(); } @@ -112,7 +128,7 @@ public void validateShouldReturnSuccessIfNonBannerBidHasAnySize() { @Test public void validateShouldFailIfBidHasNoCorrespondingImp() { final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.impid("nonExistentsImpid")), givenBidRequest(identity())); + givenBid(builder -> builder.impid("nonExistentsImpid")), givenBidRequest(identity()), givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has no corresponding imp in request"); @@ -122,7 +138,8 @@ public void validateShouldFailIfBidHasNoCorrespondingImp() { public void validateShouldFailIfBidHasInsecureCreativeInSecureContext() { final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), - givenBidRequest(builder -> builder.secure(1))); + givenBidRequest(builder -> builder.secure(1)), + givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); @@ -132,7 +149,8 @@ public void validateShouldFailIfBidHasInsecureCreativeInSecureContext() { public void validateShouldFailIfBidHasInsecureEncodedCreativeInSecureContext() { final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http%3A//site.com/creative.jpg")), - givenBidRequest(builder -> builder.secure(1))); + givenBidRequest(builder -> builder.secure(1)), + givenAccount()); assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); @@ -142,7 +160,8 @@ public void validateShouldFailIfBidHasInsecureEncodedCreativeInSecureContext() { public void validateShouldReturnSuccessIfBidHasInsecureCreativeInInsecureContext() { final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), - givenBidRequest(identity())); + givenBidRequest(identity()), + givenAccount()); assertThat(result.hasErrors()).isFalse(); } @@ -150,42 +169,38 @@ public void validateShouldReturnSuccessIfBidHasInsecureCreativeInInsecureContext @Test public void validateShouldReturnSuccessfulResultForValidBid() { final ValidationResult result = - responseBidValidator.validate(givenBid(identity()), givenBidRequest(builder -> builder.secure(1))); + responseBidValidator.validate( + givenBid(identity()), + givenBidRequest(builder -> builder.secure(1)), + givenAccount()); assertThat(result.hasErrors()).isFalse(); } @Test public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { - responseBidValidator = new ResponseBidValidator(false, null, true); + responseBidValidator = new ResponseBidValidator(false, null, true, jacksonMapper); final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.w(null).h(null)), - givenBidRequest(identity())); + givenBidRequest(identity()), + givenAccount()); assertThat(result.hasErrors()).isFalse(); } @Test public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { - responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, false); + responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, false, jacksonMapper); final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), - givenBidRequest(builder -> builder.secure(1))); + givenBidRequest(builder -> builder.secure(1)), + givenAccount()); assertThat(result.hasErrors()).isFalse(); } - private BidRequest givenBidRequest(Function impCustomizer) { - final Imp.ImpBuilder impBuilder = Imp.builder() - .id("impId1"); - - return BidRequest.builder() - .imp(singletonList(impCustomizer.apply(impBuilder).build())) - .build(); - } - private static BidderBid givenBid(Function bidCustomizer) { return givenBid(BidType.banner, bidCustomizer); } @@ -202,4 +217,21 @@ private static BidderBid givenBid(BidType type, Function impCustomizer) { + final Imp.ImpBuilder impBuilder = Imp.builder() + .id("impId1"); + + return BidRequest.builder() + .imp(singletonList(impCustomizer.apply(impBuilder).build())) + .build(); + } + + private static Account givenAccount() { + return givenAccount(identity()); + } + + private static Account givenAccount(Function accountCustomizer) { + return accountCustomizer.apply(Account.builder()).build(); + } } From fa64a7c06f869983b21def942c5bd7d7bd6d41d3 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Tue, 28 Jul 2020 13:50:25 +0300 Subject: [PATCH 04/14] Represent sizes list as string --- .../server/spring/config/ServiceConfiguration.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java index 34c20358cb4..2094a346bcd 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -67,6 +67,8 @@ import javax.validation.constraints.Min; import java.io.IOException; import java.time.Clock; +import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Properties; import java.util.stream.Collectors; @@ -478,11 +480,12 @@ BidderParamValidator bidderParamValidator(BidderCatalog bidderCatalog, JacksonMa @Bean ResponseBidValidator responseValidator( @Value("${auction.validations.banner-creative-size}") boolean shouldValidateBanner, - @Value("${auction.validations.banner-creative-allowed-sizes:#{null}}") List bannerAllowedSizes, + @Value("${auction.validations.banner-creative-allowed-sizes:#{null}}") String bannerAllowedSizesAsString, @Value("${auction.validations.secure-markup}") boolean shouldValidateSecureMarkup, JacksonMapper mapper) { - return new ResponseBidValidator(shouldValidateBanner, bannerAllowedSizes, shouldValidateSecureMarkup, mapper); + return new ResponseBidValidator( + shouldValidateBanner, splitToList(bannerAllowedSizesAsString), shouldValidateSecureMarkup, mapper); } @Bean @@ -542,4 +545,8 @@ ExternalConversionProperties externalConversionProperties( AdminManager adminManager() { return new AdminManager(); } + + private static List splitToList(String listAsString) { + return listAsString != null ? new ArrayList<>(Arrays.asList(listAsString.trim().split(","))) : null; + } } From 5c5a46eef2c9fb274e16aa140ebb68b4771cd451 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Tue, 28 Jul 2020 17:10:18 +0300 Subject: [PATCH 05/14] Update validation error metrics --- docs/metrics.md | 4 + .../server/auction/ExchangeService.java | 9 +- .../prebid/server/metric/AccountMetrics.java | 7 + .../prebid/server/metric/AdapterMetrics.java | 8 ++ .../org/prebid/server/metric/MetricName.java | 4 + .../org/prebid/server/metric/Metrics.java | 5 + .../server/metric/ValidationErrorMetrics.java | 21 +++ .../spring/config/ServiceConfiguration.java | 7 +- .../validation/ResponseBidValidator.java | 26 +++- .../org/prebid/server/metric/MetricsTest.java | 17 +++ .../validation/ResponseBidValidatorTest.java | 127 ++++++++++++++---- 11 files changed, 200 insertions(+), 35 deletions(-) create mode 100644 src/main/java/org/prebid/server/metric/ValidationErrorMetrics.java diff --git a/docs/metrics.md b/docs/metrics.md index b03daafd479..684c7171abc 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -53,10 +53,14 @@ Other available metrics can found at [Vert.x Dropwizard Metrics](https://vertx.i - `adapter..(openrtb2-web|openrtb-app|amp|legacy).tcf.geo_masked` - number of requests made to `` that required geo information removed as a result of TCF enforcement for that bidder - `adapter..(openrtb2-web|openrtb-app|amp|legacy).tcf.request_blocked` - number of requests made to `` that were blocked as a result of TCF enforcement for that bidder - `adapter..(openrtb2-web|openrtb-app|amp|legacy).tcf.analytics_blocked` - number of requests made to `` that required analytics blocked as a result of TCF enforcement for that bidder +- `adapter..validation_err.size` - number of banner bids received from the `` that had invalid size +- `adapter..validation_err.secure` - number of bids received from the `` that had insecure creative while in secure context ## Auction per-account metrics Following metrics are collected and submitted if account is configured with `basic` verbosity: - `account..requests` - number of requests received from account with `` +- `account..validation_err.size` - number of banner bids received from account with `` that had invalid size +- `account..validation_err.secure` - number of bids received from account with `` that had insecure creative while in secure context Following metrics are collected and submitted if account is configured with `detailed` verbosity: - `account..requests.type.(openrtb2-web,openrtb-app,amp,legacy)` - number of requests received from account with `` broken down by type of incoming request diff --git a/src/main/java/org/prebid/server/auction/ExchangeService.java b/src/main/java/org/prebid/server/auction/ExchangeService.java index 57607aa4786..f8fdf8bd605 100644 --- a/src/main/java/org/prebid/server/auction/ExchangeService.java +++ b/src/main/java/org/prebid/server/auction/ExchangeService.java @@ -763,7 +763,7 @@ private Future requestBids(BidderRequest bidderRequest, final long startTime = clock.millis(); return httpBidderRequester.requestBids(bidder, bidRequest, timeout, debugEnabled) - .map(bidderSeatBid -> validBidderSeatBid(bidderSeatBid, bidRequest, account)) + .map(bidderSeatBid -> validBidderSeatBid(bidderSeatBid, bidderRequest, account)) .map(seat -> applyBidPriceChanges(seat, currencyConversionRates, adServerCurrency, bidPriceAdjustmentFactor)) .map(result -> BidderResponse.of(bidderName, result, responseTime(startTime))); @@ -776,7 +776,10 @@ private Future requestBids(BidderRequest bidderRequest, *

* Returns input argument as the result if no errors found or create new {@link BidderSeatBid} otherwise. */ - private BidderSeatBid validBidderSeatBid(BidderSeatBid bidderSeatBid, BidRequest bidRequest, Account account) { + private BidderSeatBid validBidderSeatBid( + BidderSeatBid bidderSeatBid, BidderRequest bidderRequest, Account account) { + + final BidRequest bidRequest = bidderRequest.getBidRequest(); final List errors = new ArrayList<>(bidderSeatBid.getErrors()); final List requestCurrencies = bidRequest.getCur(); @@ -790,7 +793,7 @@ private BidderSeatBid validBidderSeatBid(BidderSeatBid bidderSeatBid, BidRequest final List validBids = new ArrayList<>(bids.size()); for (final BidderBid bid : bids) { - final ValidationResult validationResult = responseBidValidator.validate(bid, bidRequest, account); + final ValidationResult validationResult = responseBidValidator.validate(bid, bidderRequest, account); if (validationResult.hasErrors()) { for (String error : validationResult.getErrors()) { errors.add(BidderError.generic(error)); diff --git a/src/main/java/org/prebid/server/metric/AccountMetrics.java b/src/main/java/org/prebid/server/metric/AccountMetrics.java index 7f9d1420b52..d303db87259 100644 --- a/src/main/java/org/prebid/server/metric/AccountMetrics.java +++ b/src/main/java/org/prebid/server/metric/AccountMetrics.java @@ -20,6 +20,7 @@ class AccountMetrics extends UpdatableMetrics { private final Function requestTypeMetricsCreator; private final Map requestTypeMetrics; private final RequestMetrics requestsMetrics; + private final ValidationErrorMetrics validationErrorMetrics; AccountMetrics(MetricRegistry metricRegistry, CounterType counterType, String account) { super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), @@ -30,6 +31,8 @@ class AccountMetrics extends UpdatableMetrics { new RequestTypeMetrics(metricRegistry, counterType, createPrefix(account), requestType); requestTypeMetrics = new HashMap<>(); requestsMetrics = new RequestMetrics(metricRegistry, counterType, createPrefix(account)); + validationErrorMetrics = new ValidationErrorMetrics( + metricRegistry, counterType, createPrefix(account)); } private static String createPrefix(String account) { @@ -51,4 +54,8 @@ RequestTypeMetrics requestType(MetricName requestType) { RequestMetrics requests() { return requestsMetrics; } + + ValidationErrorMetrics validationError() { + return validationErrorMetrics; + } } diff --git a/src/main/java/org/prebid/server/metric/AdapterMetrics.java b/src/main/java/org/prebid/server/metric/AdapterMetrics.java index 80b406ec94a..f46c2baf04f 100644 --- a/src/main/java/org/prebid/server/metric/AdapterMetrics.java +++ b/src/main/java/org/prebid/server/metric/AdapterMetrics.java @@ -17,6 +17,7 @@ class AdapterMetrics extends UpdatableMetrics { private final RequestMetrics requestMetrics; private final Function bidTypeMetricsCreator; private final Map bidTypeMetrics; + private final ValidationErrorMetrics validationErrorMetrics; AdapterMetrics(MetricRegistry metricRegistry, CounterType counterType, String adapterType) { super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), @@ -29,6 +30,8 @@ class AdapterMetrics extends UpdatableMetrics { requestTypeMetrics = new HashMap<>(); requestMetrics = new RequestMetrics(metricRegistry, counterType, createAdapterPrefix(adapterType)); bidTypeMetrics = new HashMap<>(); + validationErrorMetrics = new ValidationErrorMetrics( + metricRegistry, counterType, createAdapterPrefix(adapterType)); } AdapterMetrics(MetricRegistry metricRegistry, CounterType counterType, String account, String adapterType) { @@ -44,6 +47,7 @@ class AdapterMetrics extends UpdatableMetrics { requestTypeMetrics = null; bidTypeMetricsCreator = null; bidTypeMetrics = null; + validationErrorMetrics = null; } private static String createAdapterPrefix(String adapterType) { @@ -69,4 +73,8 @@ RequestMetrics request() { BidTypeMetrics forBidType(String bidType) { return bidTypeMetrics.computeIfAbsent(bidType, bidTypeMetricsCreator); } + + ValidationErrorMetrics validationError() { + return validationErrorMetrics; + } } diff --git a/src/main/java/org/prebid/server/metric/MetricName.java b/src/main/java/org/prebid/server/metric/MetricName.java index a551456c9a8..7e9d86e15ed 100644 --- a/src/main/java/org/prebid/server/metric/MetricName.java +++ b/src/main/java/org/prebid/server/metric/MetricName.java @@ -58,6 +58,10 @@ public enum MetricName { err, networkerr, + // validation errors + size, + secure, + // cookie sync cookie_sync_requests, opt_outs, diff --git a/src/main/java/org/prebid/server/metric/Metrics.java b/src/main/java/org/prebid/server/metric/Metrics.java index b4fce963c79..fadcd5e6b77 100644 --- a/src/main/java/org/prebid/server/metric/Metrics.java +++ b/src/main/java/org/prebid/server/metric/Metrics.java @@ -247,6 +247,11 @@ public void updateAdapterRequestErrorMetric(String bidder, MetricName errorMetri forAdapter(resolveMetricsBidderName(bidder)).request().incCounter(errorMetric); } + public void updateValidationErrorMetrics(String bidder, String accountId, MetricName type) { + forAdapter(resolveMetricsBidderName(bidder)).validationError().incCounter(type); + forAccount(accountId).validationError().incCounter(type); + } + public void updateUserSyncOptoutMetric() { userSync().incCounter(MetricName.opt_outs); } diff --git a/src/main/java/org/prebid/server/metric/ValidationErrorMetrics.java b/src/main/java/org/prebid/server/metric/ValidationErrorMetrics.java new file mode 100644 index 00000000000..9fed6c8ba72 --- /dev/null +++ b/src/main/java/org/prebid/server/metric/ValidationErrorMetrics.java @@ -0,0 +1,21 @@ +package org.prebid.server.metric; + +import com.codahale.metrics.MetricRegistry; + +import java.util.Objects; +import java.util.function.Function; + +/** + * Request metrics support. + */ +class ValidationErrorMetrics extends UpdatableMetrics { + + ValidationErrorMetrics(MetricRegistry metricRegistry, CounterType counterType, String prefix) { + super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), + nameCreator(Objects.requireNonNull(prefix))); + } + + private static Function nameCreator(String prefix) { + return metricName -> String.format("%s.validation_err.%s", prefix, metricName.toString()); + } +} diff --git a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java index 2094a346bcd..971dfe970d2 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -482,10 +482,15 @@ ResponseBidValidator responseValidator( @Value("${auction.validations.banner-creative-size}") boolean shouldValidateBanner, @Value("${auction.validations.banner-creative-allowed-sizes:#{null}}") String bannerAllowedSizesAsString, @Value("${auction.validations.secure-markup}") boolean shouldValidateSecureMarkup, + Metrics metrics, JacksonMapper mapper) { return new ResponseBidValidator( - shouldValidateBanner, splitToList(bannerAllowedSizesAsString), shouldValidateSecureMarkup, mapper); + shouldValidateBanner, + splitToList(bannerAllowedSizesAsString), + shouldValidateSecureMarkup, + metrics, + mapper); } @Bean diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index f4dc37ed125..56325afb4bf 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -5,9 +5,12 @@ import com.iab.openrtb.response.Bid; import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.StringUtils; +import org.prebid.server.auction.model.BidderRequest; import org.prebid.server.bidder.model.BidderBid; import org.prebid.server.json.DecodeException; import org.prebid.server.json.JacksonMapper; +import org.prebid.server.metric.MetricName; +import org.prebid.server.metric.Metrics; import org.prebid.server.proto.openrtb.ext.response.BidType; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; @@ -29,29 +32,32 @@ public class ResponseBidValidator { private final boolean shouldValidateBanner; private final List bannerAllowedSizes; private final boolean shouldValidateSecureMarkup; + private final Metrics metrics; private final JacksonMapper mapper; public ResponseBidValidator(boolean shouldValidateBanner, List bannerAllowedSizes, boolean shouldValidateSecureMarkup, + Metrics metrics, JacksonMapper mapper) { + this.metrics = Objects.requireNonNull(metrics); this.mapper = Objects.requireNonNull(mapper); this.shouldValidateBanner = shouldValidateBanner; this.bannerAllowedSizes = toSizes(bannerAllowedSizes); this.shouldValidateSecureMarkup = shouldValidateSecureMarkup; } - public ValidationResult validate(BidderBid bidderBid, BidRequest bidRequest, Account account) { + public ValidationResult validate(BidderBid bidderBid, BidderRequest bidderRequest, Account account) { try { validateCommonFields(bidderBid.getBid()); if (shouldValidateBanner(bidderBid)) { - validateBannerFields(bidderBid, account); + validateBannerFields(bidderBid, bidderRequest, account); } if (shouldValidateSecureMarkup) { - validateSecureMarkup(bidderBid, bidRequest); + validateSecureMarkup(bidderBid, bidderRequest, account); } } catch (ValidationException e) { return ValidationResult.error(e.getMessage()); @@ -101,10 +107,14 @@ private boolean shouldValidateBanner(BidderBid bidderBid) { return bidderBid.getType() == BidType.banner && shouldValidateBanner; } - private void validateBannerFields(BidderBid bidderBid, Account account) throws ValidationException { + private void validateBannerFields( + BidderBid bidderBid, BidderRequest bidderRequest, Account account) throws ValidationException { + final Bid bid = bidderBid.getBid(); if (bannerSizeIsNotValid(bid, validBannerSizes(account))) { + metrics.updateValidationErrorMetrics(bidderRequest.getBidder(), account.getId(), MetricName.size); + throw new ValidationException( "Bid \"%s\" has 'w' and 'h' that are not valid. Bid dimensions: '%dx%d'", bid.getId(), bid.getW(), bid.getH()); @@ -127,11 +137,15 @@ private static boolean bidHasEqualSize(Bid bid, Size size) { return Objects.equals(bid.getW(), size.getWidth()) && Objects.equals(bid.getH(), size.getHeight()); } - private static void validateSecureMarkup(BidderBid bidderBid, BidRequest bidRequest) throws ValidationException { + private void validateSecureMarkup( + BidderBid bidderBid, BidderRequest bidderRequest, Account account) throws ValidationException { + final Bid bid = bidderBid.getBid(); - final Imp imp = findCorrespondingImp(bidRequest, bidderBid.getBid()); + final Imp imp = findCorrespondingImp(bidderRequest.getBidRequest(), bidderBid.getBid()); if (isImpSecure(imp) && markupIsNotSecure(bid)) { + metrics.updateValidationErrorMetrics(bidderRequest.getBidder(), account.getId(), MetricName.secure); + throw new ValidationException( "Bid \"%s\" has has insecure creative but should be in secure context", bid.getId()); } diff --git a/src/test/java/org/prebid/server/metric/MetricsTest.java b/src/test/java/org/prebid/server/metric/MetricsTest.java index 1109689fd05..3356f2e74da 100644 --- a/src/test/java/org/prebid/server/metric/MetricsTest.java +++ b/src/test/java/org/prebid/server/metric/MetricsTest.java @@ -574,6 +574,23 @@ public void updateAdapterRequestErrorMetricShouldIncrementMetrics() { assertThat(metricRegistry.counter("adapter.UNKNOWN.requests.badinput").getCount()).isEqualTo(1); } + @Test + public void updateValidationErrorMetricsShouldIncrementMetrics() { + // given + given(bidderCatalog.isValidName(INVALID_BIDDER)).willReturn(false); + given(bidderCatalog.nameByAlias(INVALID_BIDDER)).willReturn(RUBICON, null); + + // when + metrics.updateValidationErrorMetrics(RUBICON, ACCOUNT_ID, MetricName.size); + metrics.updateValidationErrorMetrics(INVALID_BIDDER, ACCOUNT_ID, MetricName.size); + metrics.updateValidationErrorMetrics(INVALID_BIDDER, ACCOUNT_ID, MetricName.size); + + // then + assertThat(metricRegistry.counter("adapter.rubicon.validation_err.size").getCount()).isEqualTo(2); + assertThat(metricRegistry.counter("adapter.UNKNOWN.validation_err.size").getCount()).isEqualTo(1); + assertThat(metricRegistry.counter("account.accountId.validation_err.size").getCount()).isEqualTo(3); + } + @Test public void updateCookieSyncRequestMetricShouldIncrementMetric() { // when diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 6fdb03c99d2..86399fe4377 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -4,9 +4,16 @@ import com.iab.openrtb.request.Imp; import com.iab.openrtb.response.Bid; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; import org.prebid.server.VertxTest; +import org.prebid.server.auction.model.BidderRequest; import org.prebid.server.bidder.model.BidderBid; +import org.prebid.server.metric.MetricName; +import org.prebid.server.metric.Metrics; import org.prebid.server.proto.openrtb.ext.response.BidType; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; @@ -21,186 +28,254 @@ import static java.util.Collections.singletonList; import static java.util.function.Function.identity; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verify; public class ResponseBidValidatorTest extends VertxTest { private static final List BANNER_ALLOWED_SIZES = asList("1x1", "2x2"); + private static final String BIDDER_NAME = "bidder"; + private static final String ACCOUNT_ID = "account"; + + @Rule + public final MockitoRule mockitoRule = MockitoJUnit.rule(); + + @Mock + private Metrics metrics; private ResponseBidValidator responseBidValidator; @Before public void setUp() { - responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, true, jacksonMapper); + responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, true, metrics, jacksonMapper); } @Test public void validateShouldFailIfMissingBid() { + // when final ValidationResult result = responseBidValidator.validate( - BidderBid.of(null, null, null), givenBidRequest(identity()), givenAccount()); + BidderBid.of(null, null, null), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Empty bid object submitted."); } @Test public void validateShouldFailIfBidHasNoId() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.id(null)), givenBidRequest(identity()), givenAccount()); + givenBid(builder -> builder.id(null)), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid missing required field 'id'"); } @Test public void validateShouldFailIfBidHasNoImpId() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.impid(null)), givenBidRequest(identity()), givenAccount()); + givenBid(builder -> builder.impid(null)), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" missing required field 'impid'"); } @Test public void validateShouldFailIfBidHasNoPrice() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.price(null)), givenBidRequest(identity()), givenAccount()); + givenBid(builder -> builder.price(null)), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); } @Test public void validateShouldFailIfBidHasNegativePrice() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.price(BigDecimal.valueOf(-1))), givenBidRequest(identity()), + givenBid(builder -> builder.price(BigDecimal.valueOf(-1))), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); } @Test public void validateShouldFailIfBidHasNoCrid() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.crid(null)), givenBidRequest(identity()), givenAccount()); + givenBid(builder -> builder.crid(null)), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" missing creative ID"); } @Test public void validateShouldFailIfBannerBidHasNoWidthAndHeight() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(null).h(null)), givenBidRequest(identity()), givenAccount()); + givenBid(builder -> builder.w(null).h(null)), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: 'nullxnull'"); } @Test public void validateShouldFailIfBannerBidHasNotAllowedWidthAndHeight() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(3).h(3)), givenBidRequest(identity()), givenAccount()); + givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: '3x3'"); } @Test public void validateShouldReturnSuccessIfNonBannerBidHasAnySize() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(BidType.video, builder -> builder.w(3).h(3)), givenBidRequest(identity()), givenAccount()); + givenBid(BidType.video, builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.hasErrors()).isFalse(); } @Test public void validateShouldReturnSuccessIfBannerBidHasAllowedByAccountWidthAndHeight() { + // when final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.w(3).h(3)), - givenBidRequest(identity()), + givenBidderRequest(identity()), givenAccount(builder -> builder.bidValidations( AccountBidValidationConfig.of(singletonList(Size.of(3, 3)))))); + // then assertThat(result.hasErrors()).isFalse(); } @Test public void validateShouldFailIfBidHasNoCorrespondingImp() { + // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.impid("nonExistentsImpid")), givenBidRequest(identity()), givenAccount()); + givenBid(builder -> builder.impid("nonExistentsImpid")), givenBidderRequest(identity()), + givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has no corresponding imp in request"); } @Test public void validateShouldFailIfBidHasInsecureCreativeInSecureContext() { + // when final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), - givenBidRequest(builder -> builder.secure(1)), + givenBidderRequest(builder -> builder.secure(1)), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); } @Test public void validateShouldFailIfBidHasInsecureEncodedCreativeInSecureContext() { + // when final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http%3A//site.com/creative.jpg")), - givenBidRequest(builder -> builder.secure(1)), + givenBidderRequest(builder -> builder.secure(1)), givenAccount()); + // then assertThat(result.getErrors()) .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); } @Test public void validateShouldReturnSuccessIfBidHasInsecureCreativeInInsecureContext() { + // when final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), - givenBidRequest(identity()), + givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.hasErrors()).isFalse(); } @Test public void validateShouldReturnSuccessfulResultForValidBid() { + // when final ValidationResult result = responseBidValidator.validate( givenBid(identity()), - givenBidRequest(builder -> builder.secure(1)), + givenBidderRequest(builder -> builder.secure(1)), givenAccount()); + // then assertThat(result.hasErrors()).isFalse(); } @Test public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { - responseBidValidator = new ResponseBidValidator(false, null, true, jacksonMapper); + // given + responseBidValidator = new ResponseBidValidator(false, null, true, metrics, jacksonMapper); + // when final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.w(null).h(null)), - givenBidRequest(identity()), + givenBidderRequest(identity()), givenAccount()); + // then assertThat(result.hasErrors()).isFalse(); } @Test public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { - responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, false, jacksonMapper); + // given + responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, false, metrics, jacksonMapper); + // when final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), - givenBidRequest(builder -> builder.secure(1)), + givenBidderRequest(builder -> builder.secure(1)), givenAccount()); + // then assertThat(result.hasErrors()).isFalse(); } + @Test + public void validateShouldIncrementValidationErrorSizeMetrics() { + // when + responseBidValidator.validate( + givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount()); + + // then + verify(metrics).updateValidationErrorMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.size); + } + + @Test + public void validateShouldIncrementValidationErrorSecureMetrics() { + // when + responseBidValidator.validate( + givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), + givenBidderRequest(builder -> builder.secure(1)), + givenAccount()); + + // then + verify(metrics).updateValidationErrorMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.secure); + } + private static BidderBid givenBid(Function bidCustomizer) { return givenBid(BidType.banner, bidCustomizer); } @@ -218,13 +293,15 @@ private static BidderBid givenBid(BidType type, Function impCustomizer) { + private static BidderRequest givenBidderRequest(Function impCustomizer) { final Imp.ImpBuilder impBuilder = Imp.builder() .id("impId1"); - return BidRequest.builder() - .imp(singletonList(impCustomizer.apply(impBuilder).build())) - .build(); + return BidderRequest.of( + BIDDER_NAME, + BidRequest.builder() + .imp(singletonList(impCustomizer.apply(impBuilder).build())) + .build()); } private static Account givenAccount() { @@ -232,6 +309,6 @@ private static Account givenAccount() { } private static Account givenAccount(Function accountCustomizer) { - return accountCustomizer.apply(Account.builder()).build(); + return accountCustomizer.apply(Account.builder().id(ACCOUNT_ID)).build(); } } From 390b11ff9211f210cf7fee62050b5370be7378a5 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Tue, 1 Sep 2020 17:00:18 +0300 Subject: [PATCH 06/14] Include new configuration properties in documentation --- docs/config-app.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/config-app.md b/docs/config-app.md index fe8cf4e8b64..ba1e730e389 100644 --- a/docs/config-app.md +++ b/docs/config-app.md @@ -61,6 +61,9 @@ Removes and downloads file again if depending service cant process probably corr - `auction.ad-server-currency` - default currency for auction, if its value was not specified in request. Important note: PBS uses ISO-4217 codes for the representation of currencies. - `auction.cache.expected-request-time-ms` - approximate value in milliseconds for Cache Service interacting. This time will be subtracted from global timeout. - `auction.cache.only-winning-bids` - if equals to `true` only the winning bids would be cached. Has lower priority than request-specific flags. +- `auction.validations.banner-creative-size` - enables creative size validation for banners. Default is `false`. +- `auction.validations.banner-creative-allowed-sizes` - comma-separated list of allowed creative sizes for banners in `[width]x[height]` format. +- `auction.validations.secure-markup` - enables secure markup validation. Default is `false`. ## Amp (OpenRTB) - `amp.default-timeout-ms` - default operation timeout for OpenRTB Amp requests. From 8518608cd627912f618586b86a03ea2321a38c78 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Tue, 1 Sep 2020 17:20:10 +0300 Subject: [PATCH 07/14] Fix broken test --- src/test/java/org/prebid/server/metric/MetricsTest.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/prebid/server/metric/MetricsTest.java b/src/test/java/org/prebid/server/metric/MetricsTest.java index a1d903277e5..a891e573ca8 100644 --- a/src/test/java/org/prebid/server/metric/MetricsTest.java +++ b/src/test/java/org/prebid/server/metric/MetricsTest.java @@ -573,17 +573,15 @@ public void updateAdapterRequestErrorMetricShouldIncrementMetrics() { public void updateValidationErrorMetricsShouldIncrementMetrics() { // given given(bidderCatalog.isValidName(INVALID_BIDDER)).willReturn(false); - given(bidderCatalog.nameByAlias(INVALID_BIDDER)).willReturn(RUBICON, null); // when metrics.updateValidationErrorMetrics(RUBICON, ACCOUNT_ID, MetricName.size); metrics.updateValidationErrorMetrics(INVALID_BIDDER, ACCOUNT_ID, MetricName.size); - metrics.updateValidationErrorMetrics(INVALID_BIDDER, ACCOUNT_ID, MetricName.size); // then - assertThat(metricRegistry.counter("adapter.rubicon.validation_err.size").getCount()).isEqualTo(2); + assertThat(metricRegistry.counter("adapter.rubicon.validation_err.size").getCount()).isEqualTo(1); assertThat(metricRegistry.counter("adapter.UNKNOWN.validation_err.size").getCount()).isEqualTo(1); - assertThat(metricRegistry.counter("account.accountId.validation_err.size").getCount()).isEqualTo(3); + assertThat(metricRegistry.counter("account.accountId.validation_err.size").getCount()).isEqualTo(2); } @Test From ba2f9bb7d0292e749d7f5ba8cc9155268e55e523 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Tue, 1 Sep 2020 17:54:14 +0300 Subject: [PATCH 08/14] Make use of BidderAliases to correctly resolve aliases on metrics reporting --- .../server/auction/ExchangeService.java | 7 +- .../validation/ResponseBidValidator.java | 27 +++++--- .../server/auction/ExchangeServiceTest.java | 4 +- .../validation/ResponseBidValidatorTest.java | 68 +++++++++++++------ 4 files changed, 71 insertions(+), 35 deletions(-) diff --git a/src/main/java/org/prebid/server/auction/ExchangeService.java b/src/main/java/org/prebid/server/auction/ExchangeService.java index 39bce246cff..8c6d174b955 100644 --- a/src/main/java/org/prebid/server/auction/ExchangeService.java +++ b/src/main/java/org/prebid/server/auction/ExchangeService.java @@ -857,7 +857,7 @@ private Future requestBids(BidderRequest bidderRequest, final long startTime = clock.millis(); return httpBidderRequester.requestBids(bidder, bidRequest, timeout, debugEnabled) - .map(bidderSeatBid -> validBidderSeatBid(bidderSeatBid, bidderRequest, account)) + .map(bidderSeatBid -> validBidderSeatBid(bidderSeatBid, bidderRequest, account, aliases)) .map(seat -> applyBidPriceChanges(seat, currencyConversionRates, adServerCurrency, bidPriceAdjustmentFactor, usepbsrates)) .map(result -> BidderResponse.of(bidderName, result, responseTime(startTime))); @@ -871,7 +871,7 @@ private Future requestBids(BidderRequest bidderRequest, * Returns input argument as the result if no errors found or create new {@link BidderSeatBid} otherwise. */ private BidderSeatBid validBidderSeatBid( - BidderSeatBid bidderSeatBid, BidderRequest bidderRequest, Account account) { + BidderSeatBid bidderSeatBid, BidderRequest bidderRequest, Account account, BidderAliases aliases) { final BidRequest bidRequest = bidderRequest.getBidRequest(); final List errors = new ArrayList<>(bidderSeatBid.getErrors()); @@ -887,7 +887,8 @@ private BidderSeatBid validBidderSeatBid( final List validBids = new ArrayList<>(bids.size()); for (final BidderBid bid : bids) { - final ValidationResult validationResult = responseBidValidator.validate(bid, bidderRequest, account); + final ValidationResult validationResult = + responseBidValidator.validate(bid, bidderRequest, account, aliases); if (validationResult.hasErrors()) { for (String error : validationResult.getErrors()) { errors.add(BidderError.generic(error)); diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index 56325afb4bf..62205e55e10 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -5,6 +5,7 @@ import com.iab.openrtb.response.Bid; import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.StringUtils; +import org.prebid.server.auction.BidderAliases; import org.prebid.server.auction.model.BidderRequest; import org.prebid.server.bidder.model.BidderBid; import org.prebid.server.json.DecodeException; @@ -48,16 +49,18 @@ public ResponseBidValidator(boolean shouldValidateBanner, this.shouldValidateSecureMarkup = shouldValidateSecureMarkup; } - public ValidationResult validate(BidderBid bidderBid, BidderRequest bidderRequest, Account account) { + public ValidationResult validate( + BidderBid bidderBid, BidderRequest bidderRequest, Account account, BidderAliases aliases) { + try { validateCommonFields(bidderBid.getBid()); if (shouldValidateBanner(bidderBid)) { - validateBannerFields(bidderBid, bidderRequest, account); + validateBannerFields(bidderBid, bidderRequest, account, aliases); } if (shouldValidateSecureMarkup) { - validateSecureMarkup(bidderBid, bidderRequest, account); + validateSecureMarkup(bidderBid, bidderRequest, account, aliases); } } catch (ValidationException e) { return ValidationResult.error(e.getMessage()); @@ -107,13 +110,16 @@ private boolean shouldValidateBanner(BidderBid bidderBid) { return bidderBid.getType() == BidType.banner && shouldValidateBanner; } - private void validateBannerFields( - BidderBid bidderBid, BidderRequest bidderRequest, Account account) throws ValidationException { + private void validateBannerFields(BidderBid bidderBid, + BidderRequest bidderRequest, + Account account, + BidderAliases aliases) throws ValidationException { final Bid bid = bidderBid.getBid(); if (bannerSizeIsNotValid(bid, validBannerSizes(account))) { - metrics.updateValidationErrorMetrics(bidderRequest.getBidder(), account.getId(), MetricName.size); + metrics.updateValidationErrorMetrics( + aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), MetricName.size); throw new ValidationException( "Bid \"%s\" has 'w' and 'h' that are not valid. Bid dimensions: '%dx%d'", @@ -137,14 +143,17 @@ private static boolean bidHasEqualSize(Bid bid, Size size) { return Objects.equals(bid.getW(), size.getWidth()) && Objects.equals(bid.getH(), size.getHeight()); } - private void validateSecureMarkup( - BidderBid bidderBid, BidderRequest bidderRequest, Account account) throws ValidationException { + private void validateSecureMarkup(BidderBid bidderBid, + BidderRequest bidderRequest, + Account account, + BidderAliases aliases) throws ValidationException { final Bid bid = bidderBid.getBid(); final Imp imp = findCorrespondingImp(bidderRequest.getBidRequest(), bidderBid.getBid()); if (isImpSecure(imp) && markupIsNotSecure(bid)) { - metrics.updateValidationErrorMetrics(bidderRequest.getBidder(), account.getId(), MetricName.secure); + metrics.updateValidationErrorMetrics( + aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), MetricName.secure); throw new ValidationException( "Bid \"%s\" has has insecure creative but should be in secure context", bid.getId()); diff --git a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java index ba87a575214..728f755fdc7 100644 --- a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java +++ b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java @@ -190,7 +190,7 @@ public void setUp() { given(fpdResolver.resolveSite(any(), any())).willAnswer(invocation -> invocation.getArgument(0)); given(fpdResolver.resolveApp(any(), any())).willAnswer(invocation -> invocation.getArgument(0)); - given(responseBidValidator.validate(any(), any(), any())).willReturn(ValidationResult.success()); + given(responseBidValidator.validate(any(), any(), any(), any())).willReturn(ValidationResult.success()); given(usersyncer.getCookieFamilyName()).willReturn("cookieFamily"); given(currencyService.convertCurrency(any(), any(), any(), any(), any())) @@ -968,7 +968,7 @@ public void shouldTolerateResponseBidValidationErrors() throws JsonProcessingExc .auctiontimestamp(1000L) .build()))); - given(responseBidValidator.validate(any(), any(), any())) + given(responseBidValidator.validate(any(), any(), any(), any())) .willReturn(ValidationResult.error("bid validation error")); final List bidderErrors = singletonList(ExtBidderError.of(BidderError.Type.generic.getCode(), diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 86399fe4377..1dc7b726ff8 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -10,6 +10,7 @@ import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; import org.prebid.server.VertxTest; +import org.prebid.server.auction.BidderAliases; import org.prebid.server.auction.model.BidderRequest; import org.prebid.server.bidder.model.BidderBid; import org.prebid.server.metric.MetricName; @@ -28,6 +29,8 @@ import static java.util.Collections.singletonList; import static java.util.function.Function.identity; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.verify; public class ResponseBidValidatorTest extends VertxTest { @@ -44,16 +47,21 @@ public class ResponseBidValidatorTest extends VertxTest { private ResponseBidValidator responseBidValidator; + @Mock + private BidderAliases bidderAliases; + @Before public void setUp() { responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, true, metrics, jacksonMapper); + + given(bidderAliases.resolveBidder(anyString())).willReturn(BIDDER_NAME); } @Test public void validateShouldFailIfMissingBid() { // when final ValidationResult result = responseBidValidator.validate( - BidderBid.of(null, null, null), givenBidderRequest(identity()), givenAccount()); + BidderBid.of(null, null, null), givenBidderRequest(identity()), givenAccount(), bidderAliases); // then assertThat(result.getErrors()) @@ -64,7 +72,7 @@ public void validateShouldFailIfMissingBid() { public void validateShouldFailIfBidHasNoId() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.id(null)), givenBidderRequest(identity()), givenAccount()); + givenBid(builder -> builder.id(null)), givenBidderRequest(identity()), givenAccount(), bidderAliases); // then assertThat(result.getErrors()) @@ -75,7 +83,8 @@ public void validateShouldFailIfBidHasNoId() { public void validateShouldFailIfBidHasNoImpId() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.impid(null)), givenBidderRequest(identity()), givenAccount()); + givenBid(builder -> builder.impid(null)), givenBidderRequest(identity()), givenAccount(), + bidderAliases); // then assertThat(result.getErrors()) @@ -86,7 +95,8 @@ public void validateShouldFailIfBidHasNoImpId() { public void validateShouldFailIfBidHasNoPrice() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.price(null)), givenBidderRequest(identity()), givenAccount()); + givenBid(builder -> builder.price(null)), givenBidderRequest(identity()), givenAccount(), + bidderAliases); // then assertThat(result.getErrors()) @@ -97,8 +107,10 @@ public void validateShouldFailIfBidHasNoPrice() { public void validateShouldFailIfBidHasNegativePrice() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.price(BigDecimal.valueOf(-1))), givenBidderRequest(identity()), - givenAccount()); + givenBid(builder -> builder.price(BigDecimal.valueOf(-1))), + givenBidderRequest(identity()), + givenAccount(), + bidderAliases); // then assertThat(result.getErrors()) @@ -109,7 +121,7 @@ public void validateShouldFailIfBidHasNegativePrice() { public void validateShouldFailIfBidHasNoCrid() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.crid(null)), givenBidderRequest(identity()), givenAccount()); + givenBid(builder -> builder.crid(null)), givenBidderRequest(identity()), givenAccount(), bidderAliases); // then assertThat(result.getErrors()) @@ -120,7 +132,8 @@ public void validateShouldFailIfBidHasNoCrid() { public void validateShouldFailIfBannerBidHasNoWidthAndHeight() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(null).h(null)), givenBidderRequest(identity()), givenAccount()); + givenBid(builder -> builder.w(null).h(null)), givenBidderRequest(identity()), givenAccount(), + bidderAliases); // then assertThat(result.getErrors()) @@ -131,7 +144,7 @@ public void validateShouldFailIfBannerBidHasNoWidthAndHeight() { public void validateShouldFailIfBannerBidHasNotAllowedWidthAndHeight() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount()); + givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount(), bidderAliases); // then assertThat(result.getErrors()) @@ -142,7 +155,10 @@ public void validateShouldFailIfBannerBidHasNotAllowedWidthAndHeight() { public void validateShouldReturnSuccessIfNonBannerBidHasAnySize() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(BidType.video, builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount()); + givenBid(BidType.video, builder -> builder.w(3).h(3)), + givenBidderRequest(identity()), + givenAccount(), + bidderAliases); // then assertThat(result.hasErrors()).isFalse(); @@ -155,7 +171,8 @@ public void validateShouldReturnSuccessIfBannerBidHasAllowedByAccountWidthAndHei givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount(builder -> builder.bidValidations( - AccountBidValidationConfig.of(singletonList(Size.of(3, 3)))))); + AccountBidValidationConfig.of(singletonList(Size.of(3, 3))))), + bidderAliases); // then assertThat(result.hasErrors()).isFalse(); @@ -165,8 +182,10 @@ public void validateShouldReturnSuccessIfBannerBidHasAllowedByAccountWidthAndHei public void validateShouldFailIfBidHasNoCorrespondingImp() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.impid("nonExistentsImpid")), givenBidderRequest(identity()), - givenAccount()); + givenBid(builder -> builder.impid("nonExistentsImpid")), + givenBidderRequest(identity()), + givenAccount(), + bidderAliases); // then assertThat(result.getErrors()) @@ -179,7 +198,8 @@ public void validateShouldFailIfBidHasInsecureCreativeInSecureContext() { final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), givenBidderRequest(builder -> builder.secure(1)), - givenAccount()); + givenAccount(), + bidderAliases); // then assertThat(result.getErrors()) @@ -192,7 +212,8 @@ public void validateShouldFailIfBidHasInsecureEncodedCreativeInSecureContext() { final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http%3A//site.com/creative.jpg")), givenBidderRequest(builder -> builder.secure(1)), - givenAccount()); + givenAccount(), + bidderAliases); // then assertThat(result.getErrors()) @@ -205,7 +226,8 @@ public void validateShouldReturnSuccessIfBidHasInsecureCreativeInInsecureContext final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), givenBidderRequest(identity()), - givenAccount()); + givenAccount(), + bidderAliases); // then assertThat(result.hasErrors()).isFalse(); @@ -218,7 +240,8 @@ public void validateShouldReturnSuccessfulResultForValidBid() { responseBidValidator.validate( givenBid(identity()), givenBidderRequest(builder -> builder.secure(1)), - givenAccount()); + givenAccount(), + bidderAliases); // then assertThat(result.hasErrors()).isFalse(); @@ -233,7 +256,8 @@ public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.w(null).h(null)), givenBidderRequest(identity()), - givenAccount()); + givenAccount(), + bidderAliases); // then assertThat(result.hasErrors()).isFalse(); @@ -248,7 +272,8 @@ public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { final ValidationResult result = responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), givenBidderRequest(builder -> builder.secure(1)), - givenAccount()); + givenAccount(), + bidderAliases); // then assertThat(result.hasErrors()).isFalse(); @@ -258,7 +283,7 @@ public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { public void validateShouldIncrementValidationErrorSizeMetrics() { // when responseBidValidator.validate( - givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount()); + givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount(), bidderAliases); // then verify(metrics).updateValidationErrorMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.size); @@ -270,7 +295,8 @@ public void validateShouldIncrementValidationErrorSecureMetrics() { responseBidValidator.validate( givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), givenBidderRequest(builder -> builder.secure(1)), - givenAccount()); + givenAccount(), + bidderAliases); // then verify(metrics).updateValidationErrorMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.secure); From db58124afe3e1fdaaf7cd70193418e9dd440fc07 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Wed, 2 Sep 2020 14:55:15 +0300 Subject: [PATCH 09/14] Change validation logic for banners: validate max size if enforced by configuration or account --- docs/application-settings.md | 8 +- docs/config-app.md | 3 +- .../model/AccountBidValidationConfig.java | 7 +- .../model/BannerMaxSizeEnforcement.java | 5 ++ .../spring/config/ServiceConfiguration.java | 11 +-- .../validation/ResponseBidValidator.java | 76 ++++++++--------- .../prebid/server/validation/model/Size.java | 42 --------- src/main/resources/application.yaml | 2 +- .../settings/FileApplicationSettingsTest.java | 6 +- .../settings/JdbcApplicationSettingsTest.java | 6 +- .../validation/ResponseBidValidatorTest.java | 85 ++++++++++++------- 11 files changed, 114 insertions(+), 137 deletions(-) create mode 100644 src/main/java/org/prebid/server/settings/model/BannerMaxSizeEnforcement.java delete mode 100644 src/main/java/org/prebid/server/validation/model/Size.java diff --git a/docs/application-settings.md b/docs/application-settings.md index e1564458d53..59b6bb87261 100644 --- a/docs/application-settings.md +++ b/docs/application-settings.md @@ -21,7 +21,7 @@ There are two ways to configure application settings: database and file. This do - `gdpr.purpose-one-treatment-interpretation` - option that allows to skip the Purpose one enforcement workflow. Values: ignore, no-access-allowed, access-allowed. - `analytics-sampling-factor` - Analytics sampling factor value. - `truncate-target-attr` - Maximum targeting attributes size. Values between 1 and 255. -- `bid-validations.banner-creative-allowed-sizes` - Defines valid banner sizes. +- `bid-validations.banner-creative-max-size` - Overrides creative max size validation for banners. ``` Purpose | Purpose goal | Purpose meaning for PBS (n\a - not affected) @@ -200,8 +200,8 @@ where tcf_config column is json with next format "web": true, "app": true, "amp": true - } - "purpose-one-treatment-interpretation": "ignore" + }, + "purpose-one-treatment-interpretation": "ignore", "purposes": { "p1": { "enforce-purpose": "full", @@ -307,7 +307,7 @@ and bid_validations column is json with next format ```json { - "banner-creative-allowed-sizes": ["1x1", "2x2"] + "banner-creative-max-size": "enforce" } ``` diff --git a/docs/config-app.md b/docs/config-app.md index 70709d1b7b4..6b7a6ec8e03 100644 --- a/docs/config-app.md +++ b/docs/config-app.md @@ -73,8 +73,7 @@ Removes and downloads file again if depending service cant process probably corr - `auction.cache.only-winning-bids` - if equals to `true` only the winning bids would be cached. Has lower priority than request-specific flags. - `auction.generate-bid-id` - whether to generate seatbid[].bid[].ext.prebid.bidid in the OpenRTB response. - `auction.id-generator-type` - if generate-bid-id is on, then this defines how the ID should be generated. Currently onlye `uuid` is supported. -- `auction.validations.banner-creative-size` - enables creative size validation for banners. Default is `false`. -- `auction.validations.banner-creative-allowed-sizes` - comma-separated list of allowed creative sizes for banners in `[width]x[height]` format. +- `auction.validations.banner-creative-max-size` - enables creative max size validation for banners. Possible values: `skip`, `enforce`, `warn`. Default is `skip`. - `auction.validations.secure-markup` - enables secure markup validation. Default is `false`. ## Amp (OpenRTB) diff --git a/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java index b5b8151e1ba..d178fa9624a 100644 --- a/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java +++ b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java @@ -2,13 +2,10 @@ import com.fasterxml.jackson.annotation.JsonProperty; import lombok.Value; -import org.prebid.server.validation.model.Size; - -import java.util.List; @Value(staticConstructor = "of") public class AccountBidValidationConfig { - @JsonProperty("banner-creative-allowed-sizes") - List bannerCreativeAllowedSizes; + @JsonProperty("banner-creative-max-size") + BannerMaxSizeEnforcement bannerMaxSizeEnforcement; } diff --git a/src/main/java/org/prebid/server/settings/model/BannerMaxSizeEnforcement.java b/src/main/java/org/prebid/server/settings/model/BannerMaxSizeEnforcement.java new file mode 100644 index 00000000000..cd19a302388 --- /dev/null +++ b/src/main/java/org/prebid/server/settings/model/BannerMaxSizeEnforcement.java @@ -0,0 +1,5 @@ +package org.prebid.server.settings.model; + +public enum BannerMaxSizeEnforcement { + skip, enforce, warn +} diff --git a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java index 5daf2ba21e8..e8420cd17cb 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -47,6 +47,7 @@ import org.prebid.server.privacy.PrivacyExtractor; import org.prebid.server.privacy.gdpr.TcfDefinerService; import org.prebid.server.settings.ApplicationSettings; +import org.prebid.server.settings.model.BannerMaxSizeEnforcement; import org.prebid.server.spring.config.model.CircuitBreakerProperties; import org.prebid.server.spring.config.model.ExternalConversionProperties; import org.prebid.server.spring.config.model.HttpClientProperties; @@ -520,18 +521,12 @@ BidderParamValidator bidderParamValidator(BidderCatalog bidderCatalog, JacksonMa @Bean ResponseBidValidator responseValidator( - @Value("${auction.validations.banner-creative-size}") boolean shouldValidateBanner, - @Value("${auction.validations.banner-creative-allowed-sizes:#{null}}") String bannerAllowedSizesAsString, + @Value("${auction.validations.banner-creative-max-size}") BannerMaxSizeEnforcement bannerMaxSizeEnforcement, @Value("${auction.validations.secure-markup}") boolean shouldValidateSecureMarkup, Metrics metrics, JacksonMapper mapper) { - return new ResponseBidValidator( - shouldValidateBanner, - splitToList(bannerAllowedSizesAsString), - shouldValidateSecureMarkup, - metrics, - mapper); + return new ResponseBidValidator(bannerMaxSizeEnforcement, shouldValidateSecureMarkup, metrics, mapper); } @Bean diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index 62205e55e10..d75ee67693f 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -1,27 +1,28 @@ package org.prebid.server.validation; +import com.iab.openrtb.request.Banner; import com.iab.openrtb.request.BidRequest; +import com.iab.openrtb.request.Format; import com.iab.openrtb.request.Imp; import com.iab.openrtb.response.Bid; -import org.apache.commons.collections4.CollectionUtils; +import org.apache.commons.collections4.ListUtils; +import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.prebid.server.auction.BidderAliases; import org.prebid.server.auction.model.BidderRequest; import org.prebid.server.bidder.model.BidderBid; -import org.prebid.server.json.DecodeException; import org.prebid.server.json.JacksonMapper; import org.prebid.server.metric.MetricName; import org.prebid.server.metric.Metrics; import org.prebid.server.proto.openrtb.ext.response.BidType; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; -import org.prebid.server.validation.model.Size; +import org.prebid.server.settings.model.BannerMaxSizeEnforcement; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; import java.util.List; import java.util.Objects; -import java.util.stream.Collectors; /** * Validator for response {@link Bid} object. @@ -30,22 +31,19 @@ public class ResponseBidValidator { private static final String[] INSECURE_MARKUP_MARKERS = {"http:", "http%3A"}; - private final boolean shouldValidateBanner; - private final List bannerAllowedSizes; + private final BannerMaxSizeEnforcement bannerMaxSizeEnforcement; private final boolean shouldValidateSecureMarkup; private final Metrics metrics; private final JacksonMapper mapper; - public ResponseBidValidator(boolean shouldValidateBanner, - List bannerAllowedSizes, + public ResponseBidValidator(BannerMaxSizeEnforcement bannerMaxSizeEnforcement, boolean shouldValidateSecureMarkup, Metrics metrics, JacksonMapper mapper) { this.metrics = Objects.requireNonNull(metrics); this.mapper = Objects.requireNonNull(mapper); - this.shouldValidateBanner = shouldValidateBanner; - this.bannerAllowedSizes = toSizes(bannerAllowedSizes); + this.bannerMaxSizeEnforcement = Objects.requireNonNull(bannerMaxSizeEnforcement); this.shouldValidateSecureMarkup = shouldValidateSecureMarkup; } @@ -55,7 +53,7 @@ public ValidationResult validate( try { validateCommonFields(bidderBid.getBid()); - if (shouldValidateBanner(bidderBid)) { + if (bidderBid.getType() == BidType.banner) { validateBannerFields(bidderBid, bidderRequest, account, aliases); } @@ -68,20 +66,6 @@ public ValidationResult validate( return ValidationResult.success(); } - private List toSizes(List bannerAllowedSizes) { - return CollectionUtils.emptyIfNull(bannerAllowedSizes).stream() - .map(this::parseSize) - .collect(Collectors.toList()); - } - - private Size parseSize(String size) { - try { - return mapper.decodeValue(StringUtils.wrap(size, '\"'), Size.class); - } catch (DecodeException e) { - throw new IllegalArgumentException("Invalid size format", e); - } - } - private static void validateCommonFields(Bid bid) throws ValidationException { if (bid == null) { throw new ValidationException("Empty bid object submitted."); @@ -106,10 +90,6 @@ private static void validateCommonFields(Bid bid) throws ValidationException { } } - private boolean shouldValidateBanner(BidderBid bidderBid) { - return bidderBid.getType() == BidType.banner && shouldValidateBanner; - } - private void validateBannerFields(BidderBid bidderBid, BidderRequest bidderRequest, Account account, @@ -117,7 +97,8 @@ private void validateBannerFields(BidderBid bidderBid, final Bid bid = bidderBid.getBid(); - if (bannerSizeIsNotValid(bid, validBannerSizes(account))) { + final BannerMaxSizeEnforcement bannerMaxSizeEnforcement = effectiveBannerMaxSizeEnforcement(account); + if (bannerMaxSizeEnforcement != BannerMaxSizeEnforcement.skip && bannerSizeIsNotValid(bid, bidderRequest)) { metrics.updateValidationErrorMetrics( aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), MetricName.size); @@ -127,20 +108,39 @@ private void validateBannerFields(BidderBid bidderBid, } } - private List validBannerSizes(Account account) { + private BannerMaxSizeEnforcement effectiveBannerMaxSizeEnforcement(Account account) { final AccountBidValidationConfig validationConfig = account.getBidValidations(); - final List accountValidBannerSizes = - validationConfig != null ? validationConfig.getBannerCreativeAllowedSizes() : null; + final BannerMaxSizeEnforcement accountBannerMaxSizeEnforcement = + validationConfig != null ? validationConfig.getBannerMaxSizeEnforcement() : null; - return accountValidBannerSizes != null ? accountValidBannerSizes : bannerAllowedSizes; + return ObjectUtils.defaultIfNull(accountBannerMaxSizeEnforcement, bannerMaxSizeEnforcement); } - private static boolean bannerSizeIsNotValid(Bid bid, List validBannerSizes) { - return validBannerSizes.stream().noneMatch(size -> bidHasEqualSize(bid, size)); + private static boolean bannerSizeIsNotValid(Bid bid, BidderRequest bidderRequest) throws ValidationException { + final Format maxSize = maxSizeForBanner(bid, bidderRequest); + final Integer bidW = bid.getW(); + final Integer bidH = bid.getH(); + + return bidW == null || bidW > maxSize.getW() + || bidH == null || bidH > maxSize.getH(); } - private static boolean bidHasEqualSize(Bid bid, Size size) { - return Objects.equals(bid.getW(), size.getWidth()) && Objects.equals(bid.getH(), size.getHeight()); + private static Format maxSizeForBanner(Bid bid, BidderRequest bidderRequest) throws ValidationException { + int maxW = 0; + int maxH = 0; + for (final Format size : bannerFormats(bid, bidderRequest)) { + maxW = Math.max(0, size.getW()); + maxH = Math.max(0, size.getH()); + } + + return Format.builder().w(maxW).h(maxH).build(); + } + + private static List bannerFormats(Bid bid, BidderRequest bidderRequest) throws ValidationException { + final Imp imp = findCorrespondingImp(bidderRequest.getBidRequest(), bid); + final Banner banner = imp.getBanner(); + + return ListUtils.emptyIfNull(banner != null ? banner.getFormat() : null); } private void validateSecureMarkup(BidderBid bidderBid, diff --git a/src/main/java/org/prebid/server/validation/model/Size.java b/src/main/java/org/prebid/server/validation/model/Size.java deleted file mode 100644 index 771500e783c..00000000000 --- a/src/main/java/org/prebid/server/validation/model/Size.java +++ /dev/null @@ -1,42 +0,0 @@ -package org.prebid.server.validation.model; - -import com.fasterxml.jackson.core.JsonParser; -import com.fasterxml.jackson.databind.DeserializationContext; -import com.fasterxml.jackson.databind.JsonDeserializer; -import com.fasterxml.jackson.databind.JsonMappingException; -import com.fasterxml.jackson.databind.annotation.JsonDeserialize; -import lombok.Value; - -import java.io.IOException; - -@Value(staticConstructor = "of") -@JsonDeserialize(using = Size.SizeDeserializer.class) -public class Size { - - Integer width; - - Integer height; - - public static class SizeDeserializer extends JsonDeserializer { - - @Override - public Size deserialize(JsonParser parser, DeserializationContext ctx) throws IOException { - final String sizeAsString = parser.getValueAsString(); - - final String[] widthAndHeight = sizeAsString.split("x"); - if (widthAndHeight.length != 2) { - throw new JsonMappingException( - parser, - String.format("Invalid size format: %s. Should be '[width]x[height]'", sizeAsString)); - } - - try { - return Size.of( - Integer.parseInt(widthAndHeight[0]), - Integer.parseInt(widthAndHeight[1])); - } catch (NumberFormatException e) { - throw new JsonMappingException(parser, "Invalid size format", e); - } - } - } -} diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index af63087b546..bacf32a99c0 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -74,7 +74,7 @@ auction: expected-request-time-ms: 10 only-winning-bids: false validations: - banner-creative-size: false + banner-creative-max-size: skip secure-markup: false video: stored-requests-timeout-ms: 90 diff --git a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java index eb2dc266e97..408fbb86203 100644 --- a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java @@ -11,6 +11,7 @@ import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; +import org.prebid.server.settings.model.BannerMaxSizeEnforcement; import org.prebid.server.settings.model.EnabledForRequestType; import org.prebid.server.settings.model.EnforcePurpose; import org.prebid.server.settings.model.Purpose; @@ -20,7 +21,6 @@ import org.prebid.server.settings.model.SpecialFeatures; import org.prebid.server.settings.model.StoredDataResult; import org.prebid.server.settings.model.StoredResponseDataResult; -import org.prebid.server.validation.model.Size; import java.util.HashSet; @@ -102,7 +102,7 @@ public void getAccountByIdShouldReturnPresentAccount() { + "}," + "analyticsSamplingFactor : '1'," + "bidValidations: {" - + "banner-creative-allowed-sizes: ['1x1', '2x2']" + + "banner-creative-max-size: 'enforce'" + "}" + "}" + "]")); @@ -136,7 +136,7 @@ public void getAccountByIdShouldReturnPresentAccount() { .purposeOneTreatmentInterpretation(PurposeOneTreatmentInterpretation.accessAllowed) .build()) .analyticsSamplingFactor(1) - .bidValidations(AccountBidValidationConfig.of(asList(Size.of(1, 1), Size.of(2, 2)))) + .bidValidations(AccountBidValidationConfig.of(BannerMaxSizeEnforcement.enforce)) .build()); } diff --git a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java index 45e30fe859d..f9b717532bd 100644 --- a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java @@ -25,10 +25,10 @@ import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; +import org.prebid.server.settings.model.BannerMaxSizeEnforcement; import org.prebid.server.settings.model.EnabledForRequestType; import org.prebid.server.settings.model.StoredDataResult; import org.prebid.server.settings.model.StoredResponseDataResult; -import org.prebid.server.validation.model.Size; import org.prebid.server.vertx.jdbc.BasicJdbcClient; import org.prebid.server.vertx.jdbc.JdbcClient; @@ -124,7 +124,7 @@ public static void beforeClass() throws SQLException { + "tcf_config, analytics_sampling_factor, truncate_target_attr, bid_validations) " + "values ('accountId','med', 100, 100, TRUE, TRUE, '{\"enabled\": true, " + "\"integration-enabled\": {\"amp\": true, \"app\": true, \"video\": true, \"web\": true}}', 1, 0, " - + "'{\"banner-creative-allowed-sizes\": [\"1x1\", \"2x2\"]}');"); + + "'{\"banner-creative-max-size\": \"enforce\"}');"); connection.createStatement().execute("insert into s2sconfig_config (uuid, config)" + " values ('adUnitConfigId', 'config');"); connection.createStatement().execute("insert into stored_requests (reqid, requestData) values ('1','value1');"); @@ -181,7 +181,7 @@ public void getAccountByIdShouldReturnAccountWithAllFieldsPopulated(TestContext .enabledForRequestType(EnabledForRequestType.of(true, true, true, true)) .build()) .truncateTargetAttr(0) - .bidValidations(AccountBidValidationConfig.of(asList(Size.of(1, 1), Size.of(2, 2)))) + .bidValidations(AccountBidValidationConfig.of(BannerMaxSizeEnforcement.enforce)) .build()); async.complete(); })); diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 1dc7b726ff8..90756d817a5 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -1,6 +1,8 @@ package org.prebid.server.validation; +import com.iab.openrtb.request.Banner; import com.iab.openrtb.request.BidRequest; +import com.iab.openrtb.request.Format; import com.iab.openrtb.request.Imp; import com.iab.openrtb.response.Bid; import org.junit.Before; @@ -18,14 +20,12 @@ import org.prebid.server.proto.openrtb.ext.response.BidType; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; -import org.prebid.server.validation.model.Size; +import org.prebid.server.settings.model.BannerMaxSizeEnforcement; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; -import java.util.List; import java.util.function.Function; -import static java.util.Arrays.asList; import static java.util.Collections.singletonList; import static java.util.function.Function.identity; import static org.assertj.core.api.Assertions.assertThat; @@ -35,7 +35,6 @@ public class ResponseBidValidatorTest extends VertxTest { - private static final List BANNER_ALLOWED_SIZES = asList("1x1", "2x2"); private static final String BIDDER_NAME = "bidder"; private static final String ACCOUNT_ID = "account"; @@ -52,7 +51,7 @@ public class ResponseBidValidatorTest extends VertxTest { @Before public void setUp() { - responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, true, metrics, jacksonMapper); + responseBidValidator = new ResponseBidValidator(BannerMaxSizeEnforcement.enforce, true, metrics, jacksonMapper); given(bidderAliases.resolveBidder(anyString())).willReturn(BIDDER_NAME); } @@ -64,8 +63,7 @@ public void validateShouldFailIfMissingBid() { BidderBid.of(null, null, null), givenBidderRequest(identity()), givenAccount(), bidderAliases); // then - assertThat(result.getErrors()) - .containsOnly("Empty bid object submitted."); + assertThat(result.getErrors()).containsOnly("Empty bid object submitted."); } @Test @@ -75,32 +73,33 @@ public void validateShouldFailIfBidHasNoId() { givenBid(builder -> builder.id(null)), givenBidderRequest(identity()), givenAccount(), bidderAliases); // then - assertThat(result.getErrors()) - .containsOnly("Bid missing required field 'id'"); + assertThat(result.getErrors()).containsOnly("Bid missing required field 'id'"); } @Test public void validateShouldFailIfBidHasNoImpId() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.impid(null)), givenBidderRequest(identity()), givenAccount(), + givenBid(builder -> builder.impid(null)), + givenBidderRequest(identity()), + givenAccount(), bidderAliases); // then - assertThat(result.getErrors()) - .containsOnly("Bid \"bidId1\" missing required field 'impid'"); + assertThat(result.getErrors()).containsOnly("Bid \"bidId1\" missing required field 'impid'"); } @Test public void validateShouldFailIfBidHasNoPrice() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.price(null)), givenBidderRequest(identity()), givenAccount(), + givenBid(builder -> builder.price(null)), + givenBidderRequest(identity()), + givenAccount(), bidderAliases); // then - assertThat(result.getErrors()) - .containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); + assertThat(result.getErrors()).containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); } @Test @@ -113,8 +112,7 @@ public void validateShouldFailIfBidHasNegativePrice() { bidderAliases); // then - assertThat(result.getErrors()) - .containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); + assertThat(result.getErrors()).containsOnly("Bid \"bidId1\" does not contain a positive 'price'"); } @Test @@ -124,15 +122,16 @@ public void validateShouldFailIfBidHasNoCrid() { givenBid(builder -> builder.crid(null)), givenBidderRequest(identity()), givenAccount(), bidderAliases); // then - assertThat(result.getErrors()) - .containsOnly("Bid \"bidId1\" missing creative ID"); + assertThat(result.getErrors()).containsOnly("Bid \"bidId1\" missing creative ID"); } @Test public void validateShouldFailIfBannerBidHasNoWidthAndHeight() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(null).h(null)), givenBidderRequest(identity()), givenAccount(), + givenBid(builder -> builder.w(null).h(null)), + givenBidderRequest(identity()), + givenAccount(), bidderAliases); // then @@ -141,14 +140,31 @@ public void validateShouldFailIfBannerBidHasNoWidthAndHeight() { } @Test - public void validateShouldFailIfBannerBidHasNotAllowedWidthAndHeight() { + public void validateShouldFailIfBannerBidWidthIsGreaterThanImposedByImp() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.w(150).h(150)), + givenBidderRequest(identity()), + givenAccount(), + bidderAliases); + + // then + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: '150x150'"); + } + + @Test + public void validateShouldFailIfBannerBidHeightIsGreaterThanImposedByImp() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount(), bidderAliases); + givenBid(builder -> builder.w(50).h(250)), + givenBidderRequest(identity()), + givenAccount(), + bidderAliases); // then assertThat(result.getErrors()) - .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: '3x3'"); + .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: '50x250'"); } @Test @@ -165,13 +181,13 @@ public void validateShouldReturnSuccessIfNonBannerBidHasAnySize() { } @Test - public void validateShouldReturnSuccessIfBannerBidHasAllowedByAccountWidthAndHeight() { + public void validateShouldReturnSuccessIfBannerBidHasInvalidSizeButAccountDoesNotEnforceValidation() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(3).h(3)), + givenBid(builder -> builder.w(150).h(150)), givenBidderRequest(identity()), givenAccount(builder -> builder.bidValidations( - AccountBidValidationConfig.of(singletonList(Size.of(3, 3))))), + AccountBidValidationConfig.of(BannerMaxSizeEnforcement.skip))), bidderAliases); // then @@ -250,11 +266,11 @@ public void validateShouldReturnSuccessfulResultForValidBid() { @Test public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { // given - responseBidValidator = new ResponseBidValidator(false, null, true, metrics, jacksonMapper); + responseBidValidator = new ResponseBidValidator(BannerMaxSizeEnforcement.skip, true, metrics, jacksonMapper); // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.w(null).h(null)), + givenBid(identity()), givenBidderRequest(identity()), givenAccount(), bidderAliases); @@ -266,7 +282,8 @@ public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { @Test public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { // given - responseBidValidator = new ResponseBidValidator(true, BANNER_ALLOWED_SIZES, false, metrics, jacksonMapper); + responseBidValidator = new ResponseBidValidator( + BannerMaxSizeEnforcement.enforce, false, metrics, jacksonMapper); // when final ValidationResult result = responseBidValidator.validate( @@ -283,7 +300,10 @@ public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { public void validateShouldIncrementValidationErrorSizeMetrics() { // when responseBidValidator.validate( - givenBid(builder -> builder.w(3).h(3)), givenBidderRequest(identity()), givenAccount(), bidderAliases); + givenBid(builder -> builder.w(150).h(200)), + givenBidderRequest(identity()), + givenAccount(), + bidderAliases); // then verify(metrics).updateValidationErrorMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.size); @@ -321,7 +341,10 @@ private static BidderBid givenBid(BidType type, Function impCustomizer) { final Imp.ImpBuilder impBuilder = Imp.builder() - .id("impId1"); + .id("impId1") + .banner(Banner.builder() + .format(singletonList(Format.builder().w(100).h(200).build())) + .build()); return BidderRequest.of( BIDDER_NAME, From c5e3a2c656c8cf7fc3592c3814e6663ca146431a Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Wed, 2 Sep 2020 15:23:11 +0300 Subject: [PATCH 10/14] Change validation logic for secure creative: look for secure markers in addition to verifying there is no insecure markers --- docs/config-app.md | 2 +- .../model/AccountBidValidationConfig.java | 2 +- ...ent.java => BidValidationEnforcement.java} | 2 +- .../spring/config/ServiceConfiguration.java | 11 +++-- .../validation/ResponseBidValidator.java | 42 ++++++++++--------- src/main/resources/application.yaml | 2 +- .../settings/FileApplicationSettingsTest.java | 4 +- .../settings/JdbcApplicationSettingsTest.java | 4 +- .../validation/ResponseBidValidatorTest.java | 40 ++++++++++++------ 9 files changed, 62 insertions(+), 47 deletions(-) rename src/main/java/org/prebid/server/settings/model/{BannerMaxSizeEnforcement.java => BidValidationEnforcement.java} (63%) diff --git a/docs/config-app.md b/docs/config-app.md index 6b7a6ec8e03..e9045a3e02c 100644 --- a/docs/config-app.md +++ b/docs/config-app.md @@ -74,7 +74,7 @@ Removes and downloads file again if depending service cant process probably corr - `auction.generate-bid-id` - whether to generate seatbid[].bid[].ext.prebid.bidid in the OpenRTB response. - `auction.id-generator-type` - if generate-bid-id is on, then this defines how the ID should be generated. Currently onlye `uuid` is supported. - `auction.validations.banner-creative-max-size` - enables creative max size validation for banners. Possible values: `skip`, `enforce`, `warn`. Default is `skip`. -- `auction.validations.secure-markup` - enables secure markup validation. Default is `false`. +- `auction.validations.secure-markup` - enables secure markup validation. Possible values: `skip`, `enforce`, `warn`. Default is `skip`. ## Amp (OpenRTB) - `amp.default-timeout-ms` - default operation timeout for OpenRTB Amp requests. diff --git a/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java index d178fa9624a..38c5afb2bfb 100644 --- a/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java +++ b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java @@ -7,5 +7,5 @@ public class AccountBidValidationConfig { @JsonProperty("banner-creative-max-size") - BannerMaxSizeEnforcement bannerMaxSizeEnforcement; + BidValidationEnforcement bannerMaxSizeEnforcement; } diff --git a/src/main/java/org/prebid/server/settings/model/BannerMaxSizeEnforcement.java b/src/main/java/org/prebid/server/settings/model/BidValidationEnforcement.java similarity index 63% rename from src/main/java/org/prebid/server/settings/model/BannerMaxSizeEnforcement.java rename to src/main/java/org/prebid/server/settings/model/BidValidationEnforcement.java index cd19a302388..975cbcfeac4 100644 --- a/src/main/java/org/prebid/server/settings/model/BannerMaxSizeEnforcement.java +++ b/src/main/java/org/prebid/server/settings/model/BidValidationEnforcement.java @@ -1,5 +1,5 @@ package org.prebid.server.settings.model; -public enum BannerMaxSizeEnforcement { +public enum BidValidationEnforcement { skip, enforce, warn } diff --git a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java index e8420cd17cb..2e4348a7209 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -47,7 +47,7 @@ import org.prebid.server.privacy.PrivacyExtractor; import org.prebid.server.privacy.gdpr.TcfDefinerService; import org.prebid.server.settings.ApplicationSettings; -import org.prebid.server.settings.model.BannerMaxSizeEnforcement; +import org.prebid.server.settings.model.BidValidationEnforcement; import org.prebid.server.spring.config.model.CircuitBreakerProperties; import org.prebid.server.spring.config.model.ExternalConversionProperties; import org.prebid.server.spring.config.model.HttpClientProperties; @@ -521,12 +521,11 @@ BidderParamValidator bidderParamValidator(BidderCatalog bidderCatalog, JacksonMa @Bean ResponseBidValidator responseValidator( - @Value("${auction.validations.banner-creative-max-size}") BannerMaxSizeEnforcement bannerMaxSizeEnforcement, - @Value("${auction.validations.secure-markup}") boolean shouldValidateSecureMarkup, - Metrics metrics, - JacksonMapper mapper) { + @Value("${auction.validations.banner-creative-max-size}") BidValidationEnforcement bannerMaxSizeEnforcement, + @Value("${auction.validations.secure-markup}") BidValidationEnforcement secureMarkupEnforcement, + Metrics metrics) { - return new ResponseBidValidator(bannerMaxSizeEnforcement, shouldValidateSecureMarkup, metrics, mapper); + return new ResponseBidValidator(bannerMaxSizeEnforcement, secureMarkupEnforcement, metrics); } @Bean diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index d75ee67693f..12354ddf690 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -11,13 +11,12 @@ import org.prebid.server.auction.BidderAliases; import org.prebid.server.auction.model.BidderRequest; import org.prebid.server.bidder.model.BidderBid; -import org.prebid.server.json.JacksonMapper; import org.prebid.server.metric.MetricName; import org.prebid.server.metric.Metrics; import org.prebid.server.proto.openrtb.ext.response.BidType; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; -import org.prebid.server.settings.model.BannerMaxSizeEnforcement; +import org.prebid.server.settings.model.BidValidationEnforcement; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; @@ -30,21 +29,19 @@ public class ResponseBidValidator { private static final String[] INSECURE_MARKUP_MARKERS = {"http:", "http%3A"}; + private static final String[] SECURE_MARKUP_MARKERS = {"https:", "https%3A"}; - private final BannerMaxSizeEnforcement bannerMaxSizeEnforcement; - private final boolean shouldValidateSecureMarkup; + private final BidValidationEnforcement bannerMaxSizeEnforcement; + private final BidValidationEnforcement secureMarkupEnforcement; private final Metrics metrics; - private final JacksonMapper mapper; - public ResponseBidValidator(BannerMaxSizeEnforcement bannerMaxSizeEnforcement, - boolean shouldValidateSecureMarkup, - Metrics metrics, - JacksonMapper mapper) { + public ResponseBidValidator(BidValidationEnforcement bannerMaxSizeEnforcement, + BidValidationEnforcement secureMarkupEnforcement, + Metrics metrics) { - this.metrics = Objects.requireNonNull(metrics); - this.mapper = Objects.requireNonNull(mapper); this.bannerMaxSizeEnforcement = Objects.requireNonNull(bannerMaxSizeEnforcement); - this.shouldValidateSecureMarkup = shouldValidateSecureMarkup; + this.secureMarkupEnforcement = Objects.requireNonNull(secureMarkupEnforcement); + this.metrics = Objects.requireNonNull(metrics); } public ValidationResult validate( @@ -57,9 +54,7 @@ public ValidationResult validate( validateBannerFields(bidderBid, bidderRequest, account, aliases); } - if (shouldValidateSecureMarkup) { - validateSecureMarkup(bidderBid, bidderRequest, account, aliases); - } + validateSecureMarkup(bidderBid, bidderRequest, account, aliases); } catch (ValidationException e) { return ValidationResult.error(e.getMessage()); } @@ -97,8 +92,8 @@ private void validateBannerFields(BidderBid bidderBid, final Bid bid = bidderBid.getBid(); - final BannerMaxSizeEnforcement bannerMaxSizeEnforcement = effectiveBannerMaxSizeEnforcement(account); - if (bannerMaxSizeEnforcement != BannerMaxSizeEnforcement.skip && bannerSizeIsNotValid(bid, bidderRequest)) { + final BidValidationEnforcement bannerMaxSizeEnforcement = effectiveBannerMaxSizeEnforcement(account); + if (bannerMaxSizeEnforcement != BidValidationEnforcement.skip && bannerSizeIsNotValid(bid, bidderRequest)) { metrics.updateValidationErrorMetrics( aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), MetricName.size); @@ -108,9 +103,9 @@ private void validateBannerFields(BidderBid bidderBid, } } - private BannerMaxSizeEnforcement effectiveBannerMaxSizeEnforcement(Account account) { + private BidValidationEnforcement effectiveBannerMaxSizeEnforcement(Account account) { final AccountBidValidationConfig validationConfig = account.getBidValidations(); - final BannerMaxSizeEnforcement accountBannerMaxSizeEnforcement = + final BidValidationEnforcement accountBannerMaxSizeEnforcement = validationConfig != null ? validationConfig.getBannerMaxSizeEnforcement() : null; return ObjectUtils.defaultIfNull(accountBannerMaxSizeEnforcement, bannerMaxSizeEnforcement); @@ -148,6 +143,10 @@ private void validateSecureMarkup(BidderBid bidderBid, Account account, BidderAliases aliases) throws ValidationException { + if (secureMarkupEnforcement == BidValidationEnforcement.skip) { + return; + } + final Bid bid = bidderBid.getBid(); final Imp imp = findCorrespondingImp(bidderRequest.getBidRequest(), bidderBid.getBid()); @@ -173,6 +172,9 @@ public static boolean isImpSecure(Imp imp) { } private static boolean markupIsNotSecure(Bid bid) { - return StringUtils.containsAny(bid.getAdm(), INSECURE_MARKUP_MARKERS); + final String adm = bid.getAdm(); + + return StringUtils.containsAny(adm, INSECURE_MARKUP_MARKERS) + || !StringUtils.containsAny(adm, SECURE_MARKUP_MARKERS); } } diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index bacf32a99c0..dd796dcb8b1 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -75,7 +75,7 @@ auction: only-winning-bids: false validations: banner-creative-max-size: skip - secure-markup: false + secure-markup: skip video: stored-requests-timeout-ms: 90 amp: diff --git a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java index 408fbb86203..f4efe8c00bd 100644 --- a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java @@ -11,7 +11,7 @@ import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; -import org.prebid.server.settings.model.BannerMaxSizeEnforcement; +import org.prebid.server.settings.model.BidValidationEnforcement; import org.prebid.server.settings.model.EnabledForRequestType; import org.prebid.server.settings.model.EnforcePurpose; import org.prebid.server.settings.model.Purpose; @@ -136,7 +136,7 @@ public void getAccountByIdShouldReturnPresentAccount() { .purposeOneTreatmentInterpretation(PurposeOneTreatmentInterpretation.accessAllowed) .build()) .analyticsSamplingFactor(1) - .bidValidations(AccountBidValidationConfig.of(BannerMaxSizeEnforcement.enforce)) + .bidValidations(AccountBidValidationConfig.of(BidValidationEnforcement.enforce)) .build()); } diff --git a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java index f9b717532bd..d41bcc3cc97 100644 --- a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java @@ -25,7 +25,7 @@ import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; -import org.prebid.server.settings.model.BannerMaxSizeEnforcement; +import org.prebid.server.settings.model.BidValidationEnforcement; import org.prebid.server.settings.model.EnabledForRequestType; import org.prebid.server.settings.model.StoredDataResult; import org.prebid.server.settings.model.StoredResponseDataResult; @@ -181,7 +181,7 @@ public void getAccountByIdShouldReturnAccountWithAllFieldsPopulated(TestContext .enabledForRequestType(EnabledForRequestType.of(true, true, true, true)) .build()) .truncateTargetAttr(0) - .bidValidations(AccountBidValidationConfig.of(BannerMaxSizeEnforcement.enforce)) + .bidValidations(AccountBidValidationConfig.of(BidValidationEnforcement.enforce)) .build()); async.complete(); })); diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 90756d817a5..4bb8ce6de93 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -20,7 +20,6 @@ import org.prebid.server.proto.openrtb.ext.response.BidType; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountBidValidationConfig; -import org.prebid.server.settings.model.BannerMaxSizeEnforcement; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; @@ -32,6 +31,8 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.verify; +import static org.prebid.server.settings.model.BidValidationEnforcement.enforce; +import static org.prebid.server.settings.model.BidValidationEnforcement.skip; public class ResponseBidValidatorTest extends VertxTest { @@ -51,7 +52,7 @@ public class ResponseBidValidatorTest extends VertxTest { @Before public void setUp() { - responseBidValidator = new ResponseBidValidator(BannerMaxSizeEnforcement.enforce, true, metrics, jacksonMapper); + responseBidValidator = new ResponseBidValidator(enforce, enforce, metrics); given(bidderAliases.resolveBidder(anyString())).willReturn(BIDDER_NAME); } @@ -187,7 +188,7 @@ public void validateShouldReturnSuccessIfBannerBidHasInvalidSizeButAccountDoesNo givenBid(builder -> builder.w(150).h(150)), givenBidderRequest(identity()), givenAccount(builder -> builder.bidValidations( - AccountBidValidationConfig.of(BannerMaxSizeEnforcement.skip))), + AccountBidValidationConfig.of(skip))), bidderAliases); // then @@ -209,10 +210,10 @@ public void validateShouldFailIfBidHasNoCorrespondingImp() { } @Test - public void validateShouldFailIfBidHasInsecureCreativeInSecureContext() { + public void validateShouldFailIfBidHasInsecureMarkerInCreativeInSecureContext() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), + givenBid(builder -> builder.adm("http://site.com/creative.jpg")), givenBidderRequest(builder -> builder.secure(1)), givenAccount(), bidderAliases); @@ -223,10 +224,24 @@ public void validateShouldFailIfBidHasInsecureCreativeInSecureContext() { } @Test - public void validateShouldFailIfBidHasInsecureEncodedCreativeInSecureContext() { + public void validateShouldFailIfBidHasInsecureEncodedMarkerInCreativeInSecureContext() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.adm(">http%3A//site.com/creative.jpg")), + givenBid(builder -> builder.adm("http%3A//site.com/creative.jpg")), + givenBidderRequest(builder -> builder.secure(1)), + givenAccount(), + bidderAliases); + + // then + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); + } + + @Test + public void validateShouldFailIfBidHasNoSecureMarkersInCreativeInSecureContext() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm("//site.com/creative.jpg")), givenBidderRequest(builder -> builder.secure(1)), givenAccount(), bidderAliases); @@ -240,7 +255,7 @@ public void validateShouldFailIfBidHasInsecureEncodedCreativeInSecureContext() { public void validateShouldReturnSuccessIfBidHasInsecureCreativeInInsecureContext() { // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), + givenBid(builder -> builder.adm("http://site.com/creative.jpg")), givenBidderRequest(identity()), givenAccount(), bidderAliases); @@ -266,7 +281,7 @@ public void validateShouldReturnSuccessfulResultForValidBid() { @Test public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { // given - responseBidValidator = new ResponseBidValidator(BannerMaxSizeEnforcement.skip, true, metrics, jacksonMapper); + responseBidValidator = new ResponseBidValidator(skip, enforce, metrics); // when final ValidationResult result = responseBidValidator.validate( @@ -282,12 +297,11 @@ public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { @Test public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { // given - responseBidValidator = new ResponseBidValidator( - BannerMaxSizeEnforcement.enforce, false, metrics, jacksonMapper); + responseBidValidator = new ResponseBidValidator(enforce, skip, metrics); // when final ValidationResult result = responseBidValidator.validate( - givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), + givenBid(builder -> builder.adm("http://site.com/creative.jpg")), givenBidderRequest(builder -> builder.secure(1)), givenAccount(), bidderAliases); @@ -313,7 +327,7 @@ public void validateShouldIncrementValidationErrorSizeMetrics() { public void validateShouldIncrementValidationErrorSecureMetrics() { // when responseBidValidator.validate( - givenBid(builder -> builder.adm(">http://site.com/creative.jpg")), + givenBid(builder -> builder.adm("http://site.com/creative.jpg")), givenBidderRequest(builder -> builder.secure(1)), givenAccount(), bidderAliases); From 5b49449338f27575ac676a69ee30dc9304d7d93f Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Wed, 2 Sep 2020 17:26:21 +0300 Subject: [PATCH 11/14] Add warnings support for bids validation --- .../server/auction/ExchangeService.java | 18 +++-- .../validation/ResponseBidValidator.java | 56 ++++++++++----- .../validation/model/ValidationResult.java | 20 ++++-- .../auction/AuctionRequestFactoryTest.java | 4 +- .../server/auction/ExchangeServiceTest.java | 71 ++++++++++++++++--- .../validation/ResponseBidValidatorTest.java | 43 ++++++++++- 6 files changed, 172 insertions(+), 40 deletions(-) diff --git a/src/main/java/org/prebid/server/auction/ExchangeService.java b/src/main/java/org/prebid/server/auction/ExchangeService.java index 8c6d174b955..528447343f0 100644 --- a/src/main/java/org/prebid/server/auction/ExchangeService.java +++ b/src/main/java/org/prebid/server/auction/ExchangeService.java @@ -889,18 +889,26 @@ private BidderSeatBid validBidderSeatBid( for (final BidderBid bid : bids) { final ValidationResult validationResult = responseBidValidator.validate(bid, bidderRequest, account, aliases); + + if (validationResult.hasWarnings()) { + addAsBidderErrors(validationResult.getWarnings(), errors); + } + if (validationResult.hasErrors()) { - for (String error : validationResult.getErrors()) { - errors.add(BidderError.generic(error)); - } - } else { - validBids.add(bid); + addAsBidderErrors(validationResult.getErrors(), errors); + continue; } + + validBids.add(bid); } return errors.isEmpty() ? bidderSeatBid : BidderSeatBid.of(validBids, bidderSeatBid.getHttpCalls(), errors); } + private void addAsBidderErrors(List messages, List errors) { + messages.stream().map(BidderError::generic).forEach(errors::add); + } + /** * Performs changes on {@link Bid}s price depends on different between adServerCurrency and bidCurrency, * and adjustment factor. Will drop bid if currency conversion is needed but not possible. diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index 12354ddf690..15aaf79afc5 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -20,6 +20,8 @@ import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Objects; @@ -47,18 +49,20 @@ public ResponseBidValidator(BidValidationEnforcement bannerMaxSizeEnforcement, public ValidationResult validate( BidderBid bidderBid, BidderRequest bidderRequest, Account account, BidderAliases aliases) { + final List warnings = new ArrayList<>(); + try { validateCommonFields(bidderBid.getBid()); if (bidderBid.getType() == BidType.banner) { - validateBannerFields(bidderBid, bidderRequest, account, aliases); + warnings.addAll(validateBannerFields(bidderBid, bidderRequest, account, aliases)); } - validateSecureMarkup(bidderBid, bidderRequest, account, aliases); + warnings.addAll(validateSecureMarkup(bidderBid, bidderRequest, account, aliases)); } catch (ValidationException e) { - return ValidationResult.error(e.getMessage()); + return ValidationResult.error(warnings, e.getMessage()); } - return ValidationResult.success(); + return ValidationResult.success(warnings); } private static void validateCommonFields(Bid bid) throws ValidationException { @@ -85,10 +89,10 @@ private static void validateCommonFields(Bid bid) throws ValidationException { } } - private void validateBannerFields(BidderBid bidderBid, - BidderRequest bidderRequest, - Account account, - BidderAliases aliases) throws ValidationException { + private List validateBannerFields(BidderBid bidderBid, + BidderRequest bidderRequest, + Account account, + BidderAliases aliases) throws ValidationException { final Bid bid = bidderBid.getBid(); @@ -97,10 +101,13 @@ private void validateBannerFields(BidderBid bidderBid, metrics.updateValidationErrorMetrics( aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), MetricName.size); - throw new ValidationException( + return singleWarningOrValidationException( + bannerMaxSizeEnforcement, "Bid \"%s\" has 'w' and 'h' that are not valid. Bid dimensions: '%dx%d'", bid.getId(), bid.getW(), bid.getH()); } + + return Collections.emptyList(); } private BidValidationEnforcement effectiveBannerMaxSizeEnforcement(Account account) { @@ -138,13 +145,13 @@ private static List bannerFormats(Bid bid, BidderRequest bidderRequest) return ListUtils.emptyIfNull(banner != null ? banner.getFormat() : null); } - private void validateSecureMarkup(BidderBid bidderBid, - BidderRequest bidderRequest, - Account account, - BidderAliases aliases) throws ValidationException { + private List validateSecureMarkup(BidderBid bidderBid, + BidderRequest bidderRequest, + Account account, + BidderAliases aliases) throws ValidationException { if (secureMarkupEnforcement == BidValidationEnforcement.skip) { - return; + return Collections.emptyList(); } final Bid bid = bidderBid.getBid(); @@ -154,9 +161,13 @@ private void validateSecureMarkup(BidderBid bidderBid, metrics.updateValidationErrorMetrics( aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), MetricName.secure); - throw new ValidationException( - "Bid \"%s\" has has insecure creative but should be in secure context", bid.getId()); + return singleWarningOrValidationException( + secureMarkupEnforcement, + "Bid \"%s\" has insecure creative but should be in secure context", + bid.getId()); } + + return Collections.emptyList(); } private static Imp findCorrespondingImp(BidRequest bidRequest, Bid bid) throws ValidationException { @@ -177,4 +188,17 @@ private static boolean markupIsNotSecure(Bid bid) { return StringUtils.containsAny(adm, INSECURE_MARKUP_MARKERS) || !StringUtils.containsAny(adm, SECURE_MARKUP_MARKERS); } + + private static List singleWarningOrValidationException( + BidValidationEnforcement enforcement, String message, Object... args) throws ValidationException { + + switch (enforcement) { + case enforce: + throw new ValidationException(message, args); + case warn: + return Collections.singletonList(String.format(message, args)); + default: + throw new IllegalStateException(String.format("Unexpected enforcement: %s", enforcement)); + } + } } diff --git a/src/main/java/org/prebid/server/validation/model/ValidationResult.java b/src/main/java/org/prebid/server/validation/model/ValidationResult.java index af0f9c13f0d..9b5fc66eee4 100644 --- a/src/main/java/org/prebid/server/validation/model/ValidationResult.java +++ b/src/main/java/org/prebid/server/validation/model/ValidationResult.java @@ -1,26 +1,38 @@ package org.prebid.server.validation.model; -import lombok.AllArgsConstructor; import lombok.Value; import java.util.Collections; import java.util.List; -@AllArgsConstructor @Value public class ValidationResult { + List warnings; + List errors; + public boolean hasWarnings() { + return !warnings.isEmpty(); + } + public boolean hasErrors() { return !errors.isEmpty(); } public static ValidationResult error(String errorMessageFormat, Object... args) { - return new ValidationResult(Collections.singletonList(String.format(errorMessageFormat, args))); + return error(Collections.emptyList(), errorMessageFormat, args); + } + + public static ValidationResult error(List warnings, String errorMessageFormat, Object... args) { + return new ValidationResult(warnings, Collections.singletonList(String.format(errorMessageFormat, args))); } public static ValidationResult success() { - return new ValidationResult(Collections.emptyList()); + return success(Collections.emptyList()); + } + + public static ValidationResult success(List warnings) { + return new ValidationResult(warnings, Collections.emptyList()); } } diff --git a/src/test/java/org/prebid/server/auction/AuctionRequestFactoryTest.java b/src/test/java/org/prebid/server/auction/AuctionRequestFactoryTest.java index 149e6b32b70..0e7c3f8f363 100644 --- a/src/test/java/org/prebid/server/auction/AuctionRequestFactoryTest.java +++ b/src/test/java/org/prebid/server/auction/AuctionRequestFactoryTest.java @@ -67,6 +67,7 @@ import java.util.Map; import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static org.assertj.core.api.Assertions.assertThat; @@ -1236,7 +1237,8 @@ public void shouldReturnFailedFutureIfRequestValidationFailed() { given(storedRequestProcessor.processStoredRequests(any())).willReturn(Future.succeededFuture( BidRequest.builder().build())); - given(requestValidator.validate(any())).willReturn(new ValidationResult(asList("error1", "error2"))); + given(requestValidator.validate(any())) + .willReturn(new ValidationResult(emptyList(), asList("error1", "error2"))); // when final Future future = factory.fromRequest(routingContext, 0L); diff --git a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java index 728f755fdc7..2c9a2346f14 100644 --- a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java +++ b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java @@ -1,6 +1,5 @@ package org.prebid.server.auction; -import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ObjectNode; @@ -955,8 +954,9 @@ public void shouldTolerateNullRequestExtPrebidTargeting() { .allSatisfy(map -> assertThat(map).isNull()); } + @SuppressWarnings("unchecked") @Test - public void shouldTolerateResponseBidValidationErrors() throws JsonProcessingException { + public void shouldTolerateResponseBidValidationErrors() { // given givenBidder("bidder1", mock(Bidder.class), givenSeatBid(singletonList( givenBid(Bid.builder().id("bidId1").impid("impId1").price(BigDecimal.valueOf(1.23)).build())))); @@ -968,20 +968,69 @@ public void shouldTolerateResponseBidValidationErrors() throws JsonProcessingExc .auctiontimestamp(1000L) .build()))); - given(responseBidValidator.validate(any(), any(), any(), any())) - .willReturn(ValidationResult.error("bid validation error")); - - final List bidderErrors = singletonList(ExtBidderError.of(BidderError.Type.generic.getCode(), + given(responseBidValidator.validate(any(), any(), any(), any())).willReturn(ValidationResult.error( + singletonList("bid validation warning"), "bid validation error")); - givenBidResponseCreator(singletonMap("bidder1", bidderErrors)); + + givenBidResponseCreator(singletonList(Bid.builder().build())); // when - final BidResponse bidResponse = exchangeService.holdAuction(givenRequestContext(bidRequest)).result(); + exchangeService.holdAuction(givenRequestContext(bidRequest)).result(); // then - final ExtBidResponse ext = mapper.treeToValue(bidResponse.getExt(), ExtBidResponse.class); - assertThat(ext.getErrors()).hasSize(1) - .containsOnly(entry("bidder1", bidderErrors)); + final ArgumentCaptor> bidderResponsesCaptor = ArgumentCaptor.forClass(List.class); + verify(bidResponseCreator).create(bidderResponsesCaptor.capture(), + any(), any(), any(), any(), anyBoolean(), anyLong(), anyBoolean(), any()); + final List bidderResponses = bidderResponsesCaptor.getValue(); + + assertThat(bidderResponses) + .extracting(BidderResponse::getSeatBid) + .flatExtracting(BidderSeatBid::getBids) + .isEmpty(); + assertThat(bidderResponses) + .extracting(BidderResponse::getSeatBid) + .flatExtracting(BidderSeatBid::getErrors) + .containsOnly( + BidderError.generic("bid validation warning"), + BidderError.generic("bid validation error")); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldTolerateResponseBidValidationWarnings() { + // given + givenBidder("bidder1", mock(Bidder.class), givenSeatBid(singletonList( + givenBid(Bid.builder().id("bidId1").impid("impId1").price(BigDecimal.valueOf(1.23)).build())))); + + final BidRequest bidRequest = givenBidRequest(singletonList( + // imp ids are not really used for matching, included them here for clarity + givenImp(singletonMap("bidder1", 1), builder -> builder.id("impId1"))), + builder -> builder.ext(ExtRequest.of(ExtRequestPrebid.builder() + .auctiontimestamp(1000L) + .build()))); + + given(responseBidValidator.validate(any(), any(), any(), any())).willReturn(ValidationResult.success( + singletonList("bid validation warning"))); + + givenBidResponseCreator(singletonList(Bid.builder().build())); + + // when + exchangeService.holdAuction(givenRequestContext(bidRequest)).result(); + + // then + final ArgumentCaptor> bidderResponsesCaptor = ArgumentCaptor.forClass(List.class); + verify(bidResponseCreator).create(bidderResponsesCaptor.capture(), + any(), any(), any(), any(), anyBoolean(), anyLong(), anyBoolean(), any()); + final List bidderResponses = bidderResponsesCaptor.getValue(); + + assertThat(bidderResponses) + .extracting(BidderResponse::getSeatBid) + .flatExtracting(BidderSeatBid::getBids) + .hasSize(1); + assertThat(bidderResponses) + .extracting(BidderResponse::getSeatBid) + .flatExtracting(BidderSeatBid::getErrors) + .containsOnly(BidderError.generic("bid validation warning")); } @Test diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 4bb8ce6de93..2b0776eff4b 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -33,6 +33,7 @@ import static org.mockito.Mockito.verify; import static org.prebid.server.settings.model.BidValidationEnforcement.enforce; import static org.prebid.server.settings.model.BidValidationEnforcement.skip; +import static org.prebid.server.settings.model.BidValidationEnforcement.warn; public class ResponseBidValidatorTest extends VertxTest { @@ -220,7 +221,7 @@ public void validateShouldFailIfBidHasInsecureMarkerInCreativeInSecureContext() // then assertThat(result.getErrors()) - .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); + .containsOnly("Bid \"bidId1\" has insecure creative but should be in secure context"); } @Test @@ -234,7 +235,7 @@ public void validateShouldFailIfBidHasInsecureEncodedMarkerInCreativeInSecureCon // then assertThat(result.getErrors()) - .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); + .containsOnly("Bid \"bidId1\" has insecure creative but should be in secure context"); } @Test @@ -248,7 +249,7 @@ public void validateShouldFailIfBidHasNoSecureMarkersInCreativeInSecureContext() // then assertThat(result.getErrors()) - .containsOnly("Bid \"bidId1\" has has insecure creative but should be in secure context"); + .containsOnly("Bid \"bidId1\" has insecure creative but should be in secure context"); } @Test @@ -294,6 +295,24 @@ public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { assertThat(result.hasErrors()).isFalse(); } + @Test + public void validateShouldReturnSuccessWithWarningIfBannerSizeEnforcementIsWarn() { + // given + responseBidValidator = new ResponseBidValidator(warn, enforce, metrics); + + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.w(null).h(null)), + givenBidderRequest(identity()), + givenAccount(), + bidderAliases); + + // then + assertThat(result.hasErrors()).isFalse(); + assertThat(result.getWarnings()) + .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: 'nullxnull'"); + } + @Test public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { // given @@ -310,6 +329,24 @@ public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { assertThat(result.hasErrors()).isFalse(); } + @Test + public void validateShouldReturnSuccessWithWarningIfSecureMarkupEnforcementIsWarn() { + // given + responseBidValidator = new ResponseBidValidator(enforce, warn, metrics); + + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm("http://site.com/creative.jpg")), + givenBidderRequest(builder -> builder.secure(1)), + givenAccount(), + bidderAliases); + + // then + assertThat(result.hasErrors()).isFalse(); + assertThat(result.getWarnings()) + .containsOnly("Bid \"bidId1\" has insecure creative but should be in secure context"); + } + @Test public void validateShouldIncrementValidationErrorSizeMetrics() { // when From bb744cd315d1cd68d580a1610691ad48bd18ced8 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Wed, 2 Sep 2020 17:31:01 +0300 Subject: [PATCH 12/14] Fix compilation error after merge --- src/test/java/org/prebid/server/auction/ExchangeServiceTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java index 06ae0fba0ef..df4cdec6c9b 100644 --- a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java +++ b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java @@ -1,5 +1,6 @@ package org.prebid.server.auction; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ObjectNode; From e0769e7382da1de3931c898189e75ae948fecbc7 Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Thu, 1 Oct 2020 12:42:15 +0300 Subject: [PATCH 13/14] Update validation metrics --- .../prebid/server/metric/AccountMetrics.java | 9 ++-- .../prebid/server/metric/AdapterMetrics.java | 11 +++-- .../org/prebid/server/metric/MetricName.java | 5 +-- .../org/prebid/server/metric/Metrics.java | 11 +++-- .../prebid/server/metric/ResponseMetrics.java | 33 +++++++++++++++ .../metric/SpecificValidationMetrics.java | 27 ++++++++++++ .../server/metric/ValidationMetrics.java | 41 +++++++++++++++++++ .../validation/ResponseBidValidator.java | 19 +++++---- .../org/prebid/server/metric/MetricsTest.java | 27 +++++++++--- .../validation/ResponseBidValidatorTest.java | 40 ++++++++++++++++-- 10 files changed, 188 insertions(+), 35 deletions(-) create mode 100644 src/main/java/org/prebid/server/metric/ResponseMetrics.java create mode 100644 src/main/java/org/prebid/server/metric/SpecificValidationMetrics.java create mode 100644 src/main/java/org/prebid/server/metric/ValidationMetrics.java diff --git a/src/main/java/org/prebid/server/metric/AccountMetrics.java b/src/main/java/org/prebid/server/metric/AccountMetrics.java index d303db87259..d855076e6fd 100644 --- a/src/main/java/org/prebid/server/metric/AccountMetrics.java +++ b/src/main/java/org/prebid/server/metric/AccountMetrics.java @@ -20,7 +20,7 @@ class AccountMetrics extends UpdatableMetrics { private final Function requestTypeMetricsCreator; private final Map requestTypeMetrics; private final RequestMetrics requestsMetrics; - private final ValidationErrorMetrics validationErrorMetrics; + private final ResponseMetrics responseMetrics; AccountMetrics(MetricRegistry metricRegistry, CounterType counterType, String account) { super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), @@ -31,8 +31,7 @@ class AccountMetrics extends UpdatableMetrics { new RequestTypeMetrics(metricRegistry, counterType, createPrefix(account), requestType); requestTypeMetrics = new HashMap<>(); requestsMetrics = new RequestMetrics(metricRegistry, counterType, createPrefix(account)); - validationErrorMetrics = new ValidationErrorMetrics( - metricRegistry, counterType, createPrefix(account)); + responseMetrics = new ResponseMetrics(metricRegistry, counterType, createPrefix(account)); } private static String createPrefix(String account) { @@ -55,7 +54,7 @@ RequestMetrics requests() { return requestsMetrics; } - ValidationErrorMetrics validationError() { - return validationErrorMetrics; + ResponseMetrics response() { + return responseMetrics; } } diff --git a/src/main/java/org/prebid/server/metric/AdapterMetrics.java b/src/main/java/org/prebid/server/metric/AdapterMetrics.java index f46c2baf04f..2f98a32cdb1 100644 --- a/src/main/java/org/prebid/server/metric/AdapterMetrics.java +++ b/src/main/java/org/prebid/server/metric/AdapterMetrics.java @@ -17,7 +17,7 @@ class AdapterMetrics extends UpdatableMetrics { private final RequestMetrics requestMetrics; private final Function bidTypeMetricsCreator; private final Map bidTypeMetrics; - private final ValidationErrorMetrics validationErrorMetrics; + private final ResponseMetrics responseMetrics; AdapterMetrics(MetricRegistry metricRegistry, CounterType counterType, String adapterType) { super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), @@ -30,8 +30,7 @@ class AdapterMetrics extends UpdatableMetrics { requestTypeMetrics = new HashMap<>(); requestMetrics = new RequestMetrics(metricRegistry, counterType, createAdapterPrefix(adapterType)); bidTypeMetrics = new HashMap<>(); - validationErrorMetrics = new ValidationErrorMetrics( - metricRegistry, counterType, createAdapterPrefix(adapterType)); + responseMetrics = new ResponseMetrics(metricRegistry, counterType, createAdapterPrefix(adapterType)); } AdapterMetrics(MetricRegistry metricRegistry, CounterType counterType, String account, String adapterType) { @@ -47,7 +46,7 @@ class AdapterMetrics extends UpdatableMetrics { requestTypeMetrics = null; bidTypeMetricsCreator = null; bidTypeMetrics = null; - validationErrorMetrics = null; + responseMetrics = null; } private static String createAdapterPrefix(String adapterType) { @@ -74,7 +73,7 @@ BidTypeMetrics forBidType(String bidType) { return bidTypeMetrics.computeIfAbsent(bidType, bidTypeMetricsCreator); } - ValidationErrorMetrics validationError() { - return validationErrorMetrics; + ResponseMetrics response() { + return responseMetrics; } } diff --git a/src/main/java/org/prebid/server/metric/MetricName.java b/src/main/java/org/prebid/server/metric/MetricName.java index a1562edb377..4ba3f25e2b2 100644 --- a/src/main/java/org/prebid/server/metric/MetricName.java +++ b/src/main/java/org/prebid/server/metric/MetricName.java @@ -58,9 +58,8 @@ public enum MetricName { err, networkerr, - // validation errors - size, - secure, + // bids validation + warn, // cookie sync cookie_sync_requests, diff --git a/src/main/java/org/prebid/server/metric/Metrics.java b/src/main/java/org/prebid/server/metric/Metrics.java index 34b0984f968..4198d012e28 100644 --- a/src/main/java/org/prebid/server/metric/Metrics.java +++ b/src/main/java/org/prebid/server/metric/Metrics.java @@ -239,9 +239,14 @@ public void updateAdapterRequestErrorMetric(String bidder, MetricName errorMetri forAdapter(resolveMetricsBidderName(bidder)).request().incCounter(errorMetric); } - public void updateValidationErrorMetrics(String bidder, String accountId, MetricName type) { - forAdapter(resolveMetricsBidderName(bidder)).validationError().incCounter(type); - forAccount(accountId).validationError().incCounter(type); + public void updateSizeValidationMetrics(String bidder, String accountId, MetricName type) { + forAdapter(resolveMetricsBidderName(bidder)).response().validation().size().incCounter(type); + forAccount(accountId).response().validation().size().incCounter(type); + } + + public void updateSecureValidationMetrics(String bidder, String accountId, MetricName type) { + forAdapter(resolveMetricsBidderName(bidder)).response().validation().secure().incCounter(type); + forAccount(accountId).response().validation().secure().incCounter(type); } public void updateUserSyncOptoutMetric() { diff --git a/src/main/java/org/prebid/server/metric/ResponseMetrics.java b/src/main/java/org/prebid/server/metric/ResponseMetrics.java new file mode 100644 index 00000000000..ba7e93fae81 --- /dev/null +++ b/src/main/java/org/prebid/server/metric/ResponseMetrics.java @@ -0,0 +1,33 @@ +package org.prebid.server.metric; + +import com.codahale.metrics.MetricRegistry; + +import java.util.Objects; +import java.util.function.Function; + +/** + * Request metrics support. + */ +class ResponseMetrics extends UpdatableMetrics { + + private final ValidationMetrics validationMetrics; + + ResponseMetrics(MetricRegistry metricRegistry, CounterType counterType, String prefix) { + super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), + nameCreator(createPrefix(Objects.requireNonNull(prefix)))); + + validationMetrics = new ValidationMetrics(metricRegistry, counterType, createPrefix(prefix)); + } + + private static Function nameCreator(String prefix) { + return metricName -> String.format("%s.%s", prefix, metricName.toString()); + } + + private static String createPrefix(String prefix) { + return String.format("%s.response", prefix); + } + + ValidationMetrics validation() { + return validationMetrics; + } +} diff --git a/src/main/java/org/prebid/server/metric/SpecificValidationMetrics.java b/src/main/java/org/prebid/server/metric/SpecificValidationMetrics.java new file mode 100644 index 00000000000..4a1bc289860 --- /dev/null +++ b/src/main/java/org/prebid/server/metric/SpecificValidationMetrics.java @@ -0,0 +1,27 @@ +package org.prebid.server.metric; + +import com.codahale.metrics.MetricRegistry; + +import java.util.Objects; +import java.util.function.Function; + +/** + * Request metrics support. + */ +class SpecificValidationMetrics extends UpdatableMetrics { + + SpecificValidationMetrics( + MetricRegistry metricRegistry, CounterType counterType, String prefix, String validation) { + + super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), + nameCreator(createPrefix(Objects.requireNonNull(prefix), Objects.requireNonNull(validation)))); + } + + private static Function nameCreator(String prefix) { + return metricName -> String.format("%s.%s", prefix, metricName.toString()); + } + + private static String createPrefix(String prefix, String validation) { + return String.format("%s.%s", prefix, validation); + } +} diff --git a/src/main/java/org/prebid/server/metric/ValidationMetrics.java b/src/main/java/org/prebid/server/metric/ValidationMetrics.java new file mode 100644 index 00000000000..a00ebea15d2 --- /dev/null +++ b/src/main/java/org/prebid/server/metric/ValidationMetrics.java @@ -0,0 +1,41 @@ +package org.prebid.server.metric; + +import com.codahale.metrics.MetricRegistry; + +import java.util.Objects; +import java.util.function.Function; + +/** + * Request metrics support. + */ +class ValidationMetrics extends UpdatableMetrics { + + private final SpecificValidationMetrics sizeValidationMetrics; + private final SpecificValidationMetrics secureValidationMetrics; + + ValidationMetrics(MetricRegistry metricRegistry, CounterType counterType, String prefix) { + super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), + nameCreator(createPrefix(Objects.requireNonNull(prefix)))); + + sizeValidationMetrics = new SpecificValidationMetrics( + metricRegistry, counterType, createPrefix(prefix), "size"); + secureValidationMetrics = new SpecificValidationMetrics( + metricRegistry, counterType, createPrefix(prefix), "secure"); + } + + private static Function nameCreator(String prefix) { + return metricName -> String.format("%s.%s", prefix, metricName.toString()); + } + + private static String createPrefix(String prefix) { + return String.format("%s.validation", prefix); + } + + SpecificValidationMetrics size() { + return sizeValidationMetrics; + } + + SpecificValidationMetrics secure() { + return secureValidationMetrics; + } +} diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index 15aaf79afc5..15a2daffb3c 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -24,6 +24,7 @@ import java.util.Collections; import java.util.List; import java.util.Objects; +import java.util.function.Consumer; /** * Validator for response {@link Bid} object. @@ -98,11 +99,10 @@ private List validateBannerFields(BidderBid bidderBid, final BidValidationEnforcement bannerMaxSizeEnforcement = effectiveBannerMaxSizeEnforcement(account); if (bannerMaxSizeEnforcement != BidValidationEnforcement.skip && bannerSizeIsNotValid(bid, bidderRequest)) { - metrics.updateValidationErrorMetrics( - aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), MetricName.size); - return singleWarningOrValidationException( bannerMaxSizeEnforcement, + metricName -> metrics.updateSizeValidationMetrics( + aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), metricName), "Bid \"%s\" has 'w' and 'h' that are not valid. Bid dimensions: '%dx%d'", bid.getId(), bid.getW(), bid.getH()); } @@ -158,11 +158,10 @@ private List validateSecureMarkup(BidderBid bidderBid, final Imp imp = findCorrespondingImp(bidderRequest.getBidRequest(), bidderBid.getBid()); if (isImpSecure(imp) && markupIsNotSecure(bid)) { - metrics.updateValidationErrorMetrics( - aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), MetricName.secure); - return singleWarningOrValidationException( secureMarkupEnforcement, + metricName -> metrics.updateSecureValidationMetrics( + aliases.resolveBidder(bidderRequest.getBidder()), account.getId(), metricName), "Bid \"%s\" has insecure creative but should be in secure context", bid.getId()); } @@ -189,13 +188,17 @@ private static boolean markupIsNotSecure(Bid bid) { || !StringUtils.containsAny(adm, SECURE_MARKUP_MARKERS); } - private static List singleWarningOrValidationException( - BidValidationEnforcement enforcement, String message, Object... args) throws ValidationException { + private static List singleWarningOrValidationException(BidValidationEnforcement enforcement, + Consumer metricsRecorder, + String message, + Object... args) throws ValidationException { switch (enforcement) { case enforce: + metricsRecorder.accept(MetricName.err); throw new ValidationException(message, args); case warn: + metricsRecorder.accept(MetricName.warn); return Collections.singletonList(String.format(message, args)); default: throw new IllegalStateException(String.format("Unexpected enforcement: %s", enforcement)); diff --git a/src/test/java/org/prebid/server/metric/MetricsTest.java b/src/test/java/org/prebid/server/metric/MetricsTest.java index a891e573ca8..8108ada8b93 100644 --- a/src/test/java/org/prebid/server/metric/MetricsTest.java +++ b/src/test/java/org/prebid/server/metric/MetricsTest.java @@ -570,18 +570,33 @@ public void updateAdapterRequestErrorMetricShouldIncrementMetrics() { } @Test - public void updateValidationErrorMetricsShouldIncrementMetrics() { + public void updateSizeValidationMetricsShouldIncrementMetrics() { // given given(bidderCatalog.isValidName(INVALID_BIDDER)).willReturn(false); // when - metrics.updateValidationErrorMetrics(RUBICON, ACCOUNT_ID, MetricName.size); - metrics.updateValidationErrorMetrics(INVALID_BIDDER, ACCOUNT_ID, MetricName.size); + metrics.updateSizeValidationMetrics(RUBICON, ACCOUNT_ID, MetricName.err); + metrics.updateSizeValidationMetrics(INVALID_BIDDER, ACCOUNT_ID, MetricName.err); // then - assertThat(metricRegistry.counter("adapter.rubicon.validation_err.size").getCount()).isEqualTo(1); - assertThat(metricRegistry.counter("adapter.UNKNOWN.validation_err.size").getCount()).isEqualTo(1); - assertThat(metricRegistry.counter("account.accountId.validation_err.size").getCount()).isEqualTo(2); + assertThat(metricRegistry.counter("adapter.rubicon.response.validation.size.err").getCount()).isEqualTo(1); + assertThat(metricRegistry.counter("adapter.UNKNOWN.response.validation.size.err").getCount()).isEqualTo(1); + assertThat(metricRegistry.counter("account.accountId.response.validation.size.err").getCount()).isEqualTo(2); + } + + @Test + public void updateSecureValidationMetricsShouldIncrementMetrics() { + // given + given(bidderCatalog.isValidName(INVALID_BIDDER)).willReturn(false); + + // when + metrics.updateSecureValidationMetrics(RUBICON, ACCOUNT_ID, MetricName.err); + metrics.updateSecureValidationMetrics(INVALID_BIDDER, ACCOUNT_ID, MetricName.err); + + // then + assertThat(metricRegistry.counter("adapter.rubicon.response.validation.secure.err").getCount()).isEqualTo(1); + assertThat(metricRegistry.counter("adapter.UNKNOWN.response.validation.secure.err").getCount()).isEqualTo(1); + assertThat(metricRegistry.counter("account.accountId.response.validation.secure.err").getCount()).isEqualTo(2); } @Test diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 2b0776eff4b..d02c3be5608 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -348,7 +348,7 @@ public void validateShouldReturnSuccessWithWarningIfSecureMarkupEnforcementIsWar } @Test - public void validateShouldIncrementValidationErrorSizeMetrics() { + public void validateShouldIncrementSizeValidationErrMetrics() { // when responseBidValidator.validate( givenBid(builder -> builder.w(150).h(200)), @@ -357,11 +357,43 @@ public void validateShouldIncrementValidationErrorSizeMetrics() { bidderAliases); // then - verify(metrics).updateValidationErrorMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.size); + verify(metrics).updateSizeValidationMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.err); } @Test - public void validateShouldIncrementValidationErrorSecureMetrics() { + public void validateShouldIncrementSizeValidationWarnMetrics() { + // given + responseBidValidator = new ResponseBidValidator(warn, warn, metrics); + + // when + responseBidValidator.validate( + givenBid(builder -> builder.w(150).h(200)), + givenBidderRequest(identity()), + givenAccount(), + bidderAliases); + + // then + verify(metrics).updateSizeValidationMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.warn); + } + + @Test + public void validateShouldIncrementSecureValidationErrMetrics() { + // when + responseBidValidator.validate( + givenBid(builder -> builder.adm("http://site.com/creative.jpg")), + givenBidderRequest(builder -> builder.secure(1)), + givenAccount(), + bidderAliases); + + // then + verify(metrics).updateSecureValidationMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.err); + } + + @Test + public void validateShouldIncrementSecureValidationWarnMetrics() { + // given + responseBidValidator = new ResponseBidValidator(warn, warn, metrics); + // when responseBidValidator.validate( givenBid(builder -> builder.adm("http://site.com/creative.jpg")), @@ -370,7 +402,7 @@ public void validateShouldIncrementValidationErrorSecureMetrics() { bidderAliases); // then - verify(metrics).updateValidationErrorMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.secure); + verify(metrics).updateSecureValidationMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.warn); } private static BidderBid givenBid(Function bidCustomizer) { From abf13fb6d5e74418c06bbcc360843b31b26f0eac Mon Sep 17 00:00:00 2001 From: Sergii Chernysh Date: Fri, 13 Nov 2020 15:58:12 +0200 Subject: [PATCH 14/14] Fix documentation and remove obsolete class --- docs/metrics.md | 8 +++---- .../server/metric/ValidationErrorMetrics.java | 21 ------------------- 2 files changed, 4 insertions(+), 25 deletions(-) delete mode 100644 src/main/java/org/prebid/server/metric/ValidationErrorMetrics.java diff --git a/docs/metrics.md b/docs/metrics.md index eabcf25b3ed..631d0f8bdf7 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -50,14 +50,14 @@ Other available metrics can found at [Vert.x Dropwizard Metrics](https://vertx.i - `adapter..(openrtb2-web|openrtb-app|amp|legacy).tcf.geo_masked` - number of requests made to `` that required geo information removed as a result of TCF enforcement for that bidder - `adapter..(openrtb2-web|openrtb-app|amp|legacy).tcf.request_blocked` - number of requests made to `` that were blocked as a result of TCF enforcement for that bidder - `adapter..(openrtb2-web|openrtb-app|amp|legacy).tcf.analytics_blocked` - number of requests made to `` that required analytics blocked as a result of TCF enforcement for that bidder -- `adapter..validation_err.size` - number of banner bids received from the `` that had invalid size -- `adapter..validation_err.secure` - number of bids received from the `` that had insecure creative while in secure context +- `adapter..response.validation.size.(warn|err)` - number of banner bids received from the `` that had invalid size +- `adapter..response.validation.secure.(warn|err)` - number of bids received from the `` that had insecure creative while in secure context ## Auction per-account metrics Following metrics are collected and submitted if account is configured with `basic` verbosity: - `account..requests` - number of requests received from account with `` -- `account..validation_err.size` - number of banner bids received from account with `` that had invalid size -- `account..validation_err.secure` - number of bids received from account with `` that had insecure creative while in secure context +- `account..response.validation.size.(warn|err)` - number of banner bids received from account with `` that had invalid size +- `account..response.validation.secure.(warn|err)` - number of bids received from account with `` that had insecure creative while in secure context Following metrics are collected and submitted if account is configured with `detailed` verbosity: - `account..requests.type.(openrtb2-web,openrtb-app,amp,legacy)` - number of requests received from account with `` broken down by type of incoming request diff --git a/src/main/java/org/prebid/server/metric/ValidationErrorMetrics.java b/src/main/java/org/prebid/server/metric/ValidationErrorMetrics.java deleted file mode 100644 index 9fed6c8ba72..00000000000 --- a/src/main/java/org/prebid/server/metric/ValidationErrorMetrics.java +++ /dev/null @@ -1,21 +0,0 @@ -package org.prebid.server.metric; - -import com.codahale.metrics.MetricRegistry; - -import java.util.Objects; -import java.util.function.Function; - -/** - * Request metrics support. - */ -class ValidationErrorMetrics extends UpdatableMetrics { - - ValidationErrorMetrics(MetricRegistry metricRegistry, CounterType counterType, String prefix) { - super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), - nameCreator(Objects.requireNonNull(prefix))); - } - - private static Function nameCreator(String prefix) { - return metricName -> String.format("%s.validation_err.%s", prefix, metricName.toString()); - } -}