diff --git a/docs/application-settings.md b/docs/application-settings.md index b6e5b7bc8b6..f2f33b1b499 100644 --- a/docs/application-settings.md +++ b/docs/application-settings.md @@ -23,6 +23,7 @@ There are two ways to configure application settings: database and file. This do - `truncate-target-attr` - Maximum targeting attributes size. Values between 1 and 255. - `default-integration` - Default integration to assume. - `analytics-config.auction-events.` - defines which channels are supported by analytics for this account +- `bid-validations.banner-creative-max-size` - Overrides creative max size validation for banners. ``` Purpose | Purpose goal | Purpose meaning for PBS (n\a - not affected) @@ -187,6 +188,7 @@ Query to create accounts_account table: `truncate_target_attr` tinyint(3) unsigned DEFAULT NULL, `default_integration` varchar(64) DEFAULT NULL, `analytics_config` varchar(512) 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, @@ -198,7 +200,7 @@ ENGINE=InnoDB DEFAULT CHARSET=utf8' where tcf_config column is json with next format -``` +```json { "enabled": true, "integration-enabled": { @@ -206,8 +208,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", @@ -309,9 +311,17 @@ where tcf_config column is json with next format } ``` +and bid_validations column is json with next format + +```json +{ + "banner-creative-max-size": "enforce" +} +``` + 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, default_integration, analytics_config +SELECT uuid, price_granularity, banner_cache_ttl, video_cache_ttl, events_enabled, enforce_ccpa, tcf_config, analytics_sampling_factor, truncate_target_attr, default_integration, analytics_config, bid_validations FROM accounts_account where uuid = ? LIMIT 1 diff --git a/docs/config-app.md b/docs/config-app.md index b748a5c6544..69d0b760303 100644 --- a/docs/config-app.md +++ b/docs/config-app.md @@ -79,6 +79,8 @@ 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-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. Possible values: `skip`, `enforce`, `warn`. Default is `skip`. ## Amp (OpenRTB) - `amp.default-timeout-ms` - default operation timeout for OpenRTB Amp requests. diff --git a/docs/metrics.md b/docs/metrics.md index e8a3afecb48..7360cdb0d04 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -78,10 +78,14 @@ where `[DATASOURCE]` is a data source name, `DEFAULT_DS` by defaul. - `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..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..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/auction/ExchangeService.java b/src/main/java/org/prebid/server/auction/ExchangeService.java index bb6dfb17646..4d78f1a4415 100644 --- a/src/main/java/org/prebid/server/auction/ExchangeService.java +++ b/src/main/java/org/prebid/server/auction/ExchangeService.java @@ -154,18 +154,19 @@ public Future holdAuction(AuctionContext context) { .compose(impsRequiredRequest -> extractBidderRequests(context, impsRequiredRequest, aliases)) .map(bidderRequests -> updateRequestMetric( bidderRequests, uidsCookie, aliases, publisherId, context.getRequestTypeMetric())) - .compose(bidderRequests -> CompositeFuture.join(bidderRequests.stream() - .map(bidderRequest -> requestBids( - bidderRequest, - auctionTimeout(timeout, cacheInfo.isDoCaching()), - debugEnabled, - aliases)) - .collect(Collectors.toList()))) + .compose(bidderRequests -> CompositeFuture.join( + bidderRequests.stream() + .map(bidderRequest -> requestBids( + bidderRequest, + auctionTimeout(timeout, cacheInfo.isDoCaching()), + debugEnabled, + aliases)) + .collect(Collectors.toList()))) // send all the requests to the bidders and gathers results .map(CompositeFuture::list) .map(bidderResponses -> storedResponseProcessor.mergeWithBidderResponses( bidderResponses, storedResponse, bidRequest.getImp())) - .map(bidderResponses -> validateAndAdjustBids(bidRequest, bidderResponses)) + .map(bidderResponses -> validateAndAdjustBids(bidderResponses, context, aliases)) .map(bidderResponses -> updateMetricsFromResponses(bidderResponses, publisherId, aliases)) // produce response from bidder results .compose(bidderResponses -> bidResponseCreator.create( @@ -811,8 +812,10 @@ private static BigDecimal bidAdjustmentForBidder(BidRequest bidRequest, String b * 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, + Timeout timeout, + boolean debugEnabled, + BidderAliases aliases) { final String bidderName = bidderRequest.getBidder(); final Bidder bidder = bidderCatalog.bidderByName(aliases.resolveBidder(bidderName)); @@ -822,10 +825,12 @@ private Future requestBids( .map(seatBid -> BidderResponse.of(bidderName, seatBid, responseTime(startTime))); } - private List validateAndAdjustBids(BidRequest bidRequest, List bidderResponses) { + private List validateAndAdjustBids( + List bidderResponses, AuctionContext auctionContext, BidderAliases aliases) { + return bidderResponses.stream() - .map(bidderResponse -> validBidderResponse(bidderResponse, bidRequest.getCur())) - .map(bidderResponse -> applyBidPriceChanges(bidderResponse, bidRequest)) + .map(bidderResponse -> validBidderResponse(bidderResponse, auctionContext, aliases)) + .map(bidderResponse -> applyBidPriceChanges(bidderResponse, auctionContext.getBidRequest())) .collect(Collectors.toList()); } @@ -836,28 +841,37 @@ private List validateAndAdjustBids(BidRequest bidRequest, List * Returns input argument as the result if no errors found or creates new {@link BidderResponse} otherwise. */ - private BidderResponse validBidderResponse(BidderResponse bidderResponse, List requestCurrencies) { - final BidderSeatBid seatBid = bidderResponse.getSeatBid(); - final List bids = seatBid.getBids(); + private BidderResponse validBidderResponse( + BidderResponse bidderResponse, AuctionContext auctionContext, BidderAliases aliases) { - final List validBids = new ArrayList<>(bids.size()); + final BidRequest bidRequest = auctionContext.getBidRequest(); + final BidderSeatBid seatBid = bidderResponse.getSeatBid(); final List errors = new ArrayList<>(seatBid.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); + final List bids = seatBid.getBids(); + final List validBids = new ArrayList<>(bids.size()); + + for (final BidderBid bid : bids) { + final ValidationResult validationResult = + responseBidValidator.validate(bid, bidderResponse.getBidder(), auctionContext, 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() @@ -865,6 +879,10 @@ private BidderResponse validBidderResponse(BidderResponse bidderResponse, 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/metric/AccountMetrics.java b/src/main/java/org/prebid/server/metric/AccountMetrics.java index 2dcfbba217a..80ae7853dd6 100644 --- a/src/main/java/org/prebid/server/metric/AccountMetrics.java +++ b/src/main/java/org/prebid/server/metric/AccountMetrics.java @@ -21,6 +21,7 @@ class AccountMetrics extends UpdatableMetrics { private final Map requestTypeMetrics; private final RequestMetrics requestsMetrics; private final CacheMetrics cacheMetrics; + private final ResponseMetrics responseMetrics; AccountMetrics(MetricRegistry metricRegistry, CounterType counterType, String account) { super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), @@ -32,6 +33,7 @@ class AccountMetrics extends UpdatableMetrics { requestTypeMetrics = new HashMap<>(); requestsMetrics = new RequestMetrics(metricRegistry, counterType, createPrefix(account)); cacheMetrics = new CacheMetrics(metricRegistry, counterType, createPrefix(account)); + responseMetrics = new ResponseMetrics(metricRegistry, counterType, createPrefix(account)); } private static String createPrefix(String account) { @@ -57,4 +59,8 @@ RequestMetrics requests() { CacheMetrics cache() { return cacheMetrics; } + + 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 80b406ec94a..2f98a32cdb1 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 ResponseMetrics responseMetrics; AdapterMetrics(MetricRegistry metricRegistry, CounterType counterType, String adapterType) { super(Objects.requireNonNull(metricRegistry), Objects.requireNonNull(counterType), @@ -29,6 +30,7 @@ class AdapterMetrics extends UpdatableMetrics { requestTypeMetrics = new HashMap<>(); requestMetrics = new RequestMetrics(metricRegistry, counterType, createAdapterPrefix(adapterType)); bidTypeMetrics = new HashMap<>(); + responseMetrics = new ResponseMetrics(metricRegistry, counterType, createAdapterPrefix(adapterType)); } AdapterMetrics(MetricRegistry metricRegistry, CounterType counterType, String account, String adapterType) { @@ -44,6 +46,7 @@ class AdapterMetrics extends UpdatableMetrics { requestTypeMetrics = null; bidTypeMetricsCreator = null; bidTypeMetrics = null; + responseMetrics = null; } private static String createAdapterPrefix(String adapterType) { @@ -69,4 +72,8 @@ RequestMetrics request() { BidTypeMetrics forBidType(String bidType) { return bidTypeMetrics.computeIfAbsent(bidType, bidTypeMetricsCreator); } + + 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 524d032b883..e1580af8958 100644 --- a/src/main/java/org/prebid/server/metric/MetricName.java +++ b/src/main/java/org/prebid/server/metric/MetricName.java @@ -60,6 +60,9 @@ public enum MetricName { err, networkerr, + // bids validation + warn, + // 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 53f05612ed2..5980206f0a4 100644 --- a/src/main/java/org/prebid/server/metric/Metrics.java +++ b/src/main/java/org/prebid/server/metric/Metrics.java @@ -266,6 +266,16 @@ public void updateAdapterRequestErrorMetric(String bidder, MetricName errorMetri forAdapter(resolveMetricsBidderName(bidder)).request().incCounter(errorMetric); } + 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() { userSync().incCounter(MetricName.opt_outs); } 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/settings/JdbcApplicationSettings.java b/src/main/java/org/prebid/server/settings/JdbcApplicationSettings.java index 39f1e3c75b3..7ace747d3ca 100644 --- a/src/main/java/org/prebid/server/settings/JdbcApplicationSettings.java +++ b/src/main/java/org/prebid/server/settings/JdbcApplicationSettings.java @@ -13,6 +13,7 @@ import org.prebid.server.settings.helper.JdbcStoredResponseResultMapper; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountAnalyticsConfig; +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; @@ -97,9 +98,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," - + " default_integration, analytics_config 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, " + + "default_integration, analytics_config, bid_validations " + + "FROM accounts_account where uuid = ? LIMIT 1", Collections.singletonList(accountId), result -> mapToModelOrError(result, row -> Account.builder() .id(row.getString(0)) @@ -113,6 +115,7 @@ public Future getAccountById(String accountId, Timeout timeout) { .truncateTargetAttr(row.getInteger(8)) .defaultIntegration(row.getString(9)) .analyticsConfig(toModel(row.getString(10), AccountAnalyticsConfig.class)) + .bidValidations(toModel(row.getString(11), AccountBidValidationConfig.class)) .build()), timeout) .compose(result -> failedIfNull(result, accountId, "Account")); 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 0d011c9df31..7e736416298 100644 --- a/src/main/java/org/prebid/server/settings/model/Account.java +++ b/src/main/java/org/prebid/server/settings/model/Account.java @@ -29,6 +29,8 @@ public class Account { AccountAnalyticsConfig analyticsConfig; + 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..38c5afb2bfb --- /dev/null +++ b/src/main/java/org/prebid/server/settings/model/AccountBidValidationConfig.java @@ -0,0 +1,11 @@ +package org.prebid.server.settings.model; + +import com.fasterxml.jackson.annotation.JsonProperty; +import lombok.Value; + +@Value(staticConstructor = "of") +public class AccountBidValidationConfig { + + @JsonProperty("banner-creative-max-size") + BidValidationEnforcement bannerMaxSizeEnforcement; +} diff --git a/src/main/java/org/prebid/server/settings/model/BidValidationEnforcement.java b/src/main/java/org/prebid/server/settings/model/BidValidationEnforcement.java new file mode 100644 index 00000000000..f5ed5a6f510 --- /dev/null +++ b/src/main/java/org/prebid/server/settings/model/BidValidationEnforcement.java @@ -0,0 +1,6 @@ +package org.prebid.server.settings.model; + +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 48acafe055b..ba7957c8d80 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -50,6 +50,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.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; @@ -203,8 +204,8 @@ AuctionRequestFactory auctionRequestFactory( IdGenerator idGenerator, JacksonMapper mapper) { - final List blacklistedApps = splitCommaSeparatedString(blacklistedAppsString); - final List blacklistedAccounts = splitCommaSeparatedString(blacklistedAccountsString); + final List blacklistedApps = splitToList(blacklistedAppsString); + final List blacklistedAccounts = splitToList(blacklistedAccountsString); return new AuctionRequestFactory( maxRequestSize, @@ -296,7 +297,7 @@ VideoStoredRequestProcessor videoStoredRequestProcessor( return VideoStoredRequestProcessor.create( enforceStoredRequest, - splitCommaSeparatedString(blacklistedAccountsString), + splitToList(blacklistedAccountsString), defaultTimeoutMs, adServerCurrency, defaultBidRequestPath, @@ -578,8 +579,12 @@ BidderParamValidator bidderParamValidator(BidderCatalog bidderCatalog, JacksonMa } @Bean - ResponseBidValidator responseValidator() { - return new ResponseBidValidator(); + ResponseBidValidator responseValidator( + @Value("${auction.validations.banner-creative-max-size}") BidValidationEnforcement bannerMaxSizeEnforcement, + @Value("${auction.validations.secure-markup}") BidValidationEnforcement secureMarkupEnforcement, + Metrics metrics) { + + return new ResponseBidValidator(bannerMaxSizeEnforcement, secureMarkupEnforcement, metrics); } @Bean @@ -659,9 +664,11 @@ LoggerControlKnob loggerControlKnob(Vertx vertx) { return new LoggerControlKnob(vertx); } - private static List splitCommaSeparatedString(String listString) { - return Stream.of(listString.split(",")) + private static List splitToList(String listAsString) { + return listAsString != null + ? Stream.of(listAsString.split(",")) .map(String::trim) - .collect(Collectors.toList()); + .collect(Collectors.toList()) + : null; } } diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index 86e29e29bc1..f0e6ac31d89 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -1,29 +1,74 @@ 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.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.AuctionContext; 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; +import org.prebid.server.settings.model.BidValidationEnforcement; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; import java.util.Currency; +import java.util.List; +import java.util.Objects; +import java.util.function.Consumer; /** * Validator for response {@link Bid} object. */ public class ResponseBidValidator { - public ValidationResult validate(BidderBid bidderBid) { + private static final String[] INSECURE_MARKUP_MARKERS = {"http:", "http%3A"}; + private static final String[] SECURE_MARKUP_MARKERS = {"https:", "https%3A"}; + + private final BidValidationEnforcement bannerMaxSizeEnforcement; + private final BidValidationEnforcement secureMarkupEnforcement; + private final Metrics metrics; + + public ResponseBidValidator(BidValidationEnforcement bannerMaxSizeEnforcement, + BidValidationEnforcement secureMarkupEnforcement, + Metrics metrics) { + + this.bannerMaxSizeEnforcement = Objects.requireNonNull(bannerMaxSizeEnforcement); + this.secureMarkupEnforcement = Objects.requireNonNull(secureMarkupEnforcement); + this.metrics = Objects.requireNonNull(metrics); + } + + public ValidationResult validate( + BidderBid bidderBid, String bidder, AuctionContext auctionContext, BidderAliases aliases) { + + final List warnings = new ArrayList<>(); + try { - validateFieldsFor(bidderBid.getBid()); + validateCommonFields(bidderBid.getBid()); validateCurrency(bidderBid.getBidCurrency()); + + if (bidderBid.getType() == BidType.banner) { + warnings.addAll(validateBannerFields(bidderBid, bidder, auctionContext, aliases)); + } + + warnings.addAll(validateSecureMarkup(bidderBid, bidder, auctionContext, 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 validateFieldsFor(Bid bid) throws ValidationException { + private static void validateCommonFields(Bid bid) throws ValidationException { if (bid == null) { throw new ValidationException("Empty bid object submitted."); } @@ -64,4 +109,122 @@ private static void validateCurrency(String currency) { throw new IllegalArgumentException(String.format("BidResponse currency is not valid: %s", currency), e); } } + + private List validateBannerFields(BidderBid bidderBid, + String bidder, + AuctionContext auctionContext, + BidderAliases aliases) throws ValidationException { + + final Bid bid = bidderBid.getBid(); + final Account account = auctionContext.getAccount(); + + final BidValidationEnforcement bannerMaxSizeEnforcement = effectiveBannerMaxSizeEnforcement(account); + if (bannerMaxSizeEnforcement != BidValidationEnforcement.skip + && bannerSizeIsNotValid(bid, auctionContext.getBidRequest())) { + + return singleWarningOrValidationException( + bannerMaxSizeEnforcement, + metricName -> metrics.updateSizeValidationMetrics( + aliases.resolveBidder(bidder), account.getId(), metricName), + "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) { + final AccountBidValidationConfig validationConfig = account.getBidValidations(); + final BidValidationEnforcement accountBannerMaxSizeEnforcement = + validationConfig != null ? validationConfig.getBannerMaxSizeEnforcement() : null; + + return ObjectUtils.defaultIfNull(accountBannerMaxSizeEnforcement, bannerMaxSizeEnforcement); + } + + private static boolean bannerSizeIsNotValid(Bid bid, BidRequest bidRequest) throws ValidationException { + final Format maxSize = maxSizeForBanner(bid, bidRequest); + final Integer bidW = bid.getW(); + final Integer bidH = bid.getH(); + + return bidW == null || bidW > maxSize.getW() + || bidH == null || bidH > maxSize.getH(); + } + + private static Format maxSizeForBanner(Bid bid, BidRequest bidRequest) throws ValidationException { + int maxW = 0; + int maxH = 0; + for (final Format size : bannerFormats(bid, bidRequest)) { + 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, BidRequest bidRequest) throws ValidationException { + final Imp imp = findCorrespondingImp(bidRequest, bid); + final Banner banner = imp.getBanner(); + + return ListUtils.emptyIfNull(banner != null ? banner.getFormat() : null); + } + + private List validateSecureMarkup(BidderBid bidderBid, + String bidder, + AuctionContext auctionContext, + BidderAliases aliases) throws ValidationException { + + if (secureMarkupEnforcement == BidValidationEnforcement.skip) { + return Collections.emptyList(); + } + + final Bid bid = bidderBid.getBid(); + final Imp imp = findCorrespondingImp(auctionContext.getBidRequest(), bidderBid.getBid()); + + if (isImpSecure(imp) && markupIsNotSecure(bid)) { + return singleWarningOrValidationException( + secureMarkupEnforcement, + metricName -> metrics.updateSecureValidationMetrics( + aliases.resolveBidder(bidder), auctionContext.getAccount().getId(), metricName), + "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 { + 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) { + final String adm = bid.getAdm(); + + return StringUtils.containsAny(adm, INSECURE_MARKUP_MARKERS) + || !StringUtils.containsAny(adm, SECURE_MARKUP_MARKERS); + } + + 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/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/main/resources/application.yaml b/src/main/resources/application.yaml index 722a769c10f..5e86babb241 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -87,6 +87,9 @@ auction: cache: expected-request-time-ms: 10 only-winning-bids: false + validations: + banner-creative-max-size: skip + secure-markup: skip video: stored-request-required: false stored-requests-timeout-ms: 90 diff --git a/src/test/java/org/prebid/server/auction/AuctionRequestFactoryTest.java b/src/test/java/org/prebid/server/auction/AuctionRequestFactoryTest.java index c01a38e70f0..732a13b1b38 100644 --- a/src/test/java/org/prebid/server/auction/AuctionRequestFactoryTest.java +++ b/src/test/java/org/prebid/server/auction/AuctionRequestFactoryTest.java @@ -1363,7 +1363,8 @@ public void shouldReturnFailedFutureIfRequestValidationFailed() { given(storedRequestProcessor.processStoredRequests(any(), 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 639155fbcde..6fb43852308 100644 --- a/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java +++ b/src/test/java/org/prebid/server/auction/ExchangeServiceTest.java @@ -188,7 +188,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())).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())) @@ -942,8 +942,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())))); @@ -955,20 +956,67 @@ public void shouldTolerateResponseBidValidationErrors() throws JsonProcessingExc .auctiontimestamp(1000L) .build()))); - given(responseBidValidator.validate(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(), anyBoolean()); + 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(), anyBoolean()); + 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 @@ -985,7 +1033,7 @@ public void shouldRejectBidIfCurrencyIsNotValid() throws JsonProcessingException .auctiontimestamp(1000L) .build()))); - given(responseBidValidator.validate(any())) + given(responseBidValidator.validate(any(), any(), any(), any())) .willReturn(ValidationResult.error("BidResponse currency is not valid: USDD")); final List bidderErrors = singletonList(ExtBidderError.of(BidderError.Type.generic.getCode(), diff --git a/src/test/java/org/prebid/server/metric/MetricsTest.java b/src/test/java/org/prebid/server/metric/MetricsTest.java index 8503dadfb39..a47b0dea119 100644 --- a/src/test/java/org/prebid/server/metric/MetricsTest.java +++ b/src/test/java/org/prebid/server/metric/MetricsTest.java @@ -560,6 +560,36 @@ public void updateAdapterRequestErrorMetricShouldIncrementMetrics() { assertThat(metricRegistry.counter("adapter.UNKNOWN.requests.badinput").getCount()).isEqualTo(2); } + @Test + public void updateSizeValidationMetricsShouldIncrementMetrics() { + // given + given(bidderCatalog.isValidName(INVALID_BIDDER)).willReturn(false); + + // when + metrics.updateSizeValidationMetrics(RUBICON, ACCOUNT_ID, MetricName.err); + metrics.updateSizeValidationMetrics(INVALID_BIDDER, ACCOUNT_ID, MetricName.err); + + // then + 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 public void updateCookieSyncRequestMetricShouldIncrementMetric() { // when diff --git a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java index 412ae6f402e..aab6540a56f 100644 --- a/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/FileApplicationSettingsTest.java @@ -10,7 +10,9 @@ import org.mockito.junit.MockitoRule; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountAnalyticsConfig; +import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; +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; @@ -104,6 +106,9 @@ public void getAccountByIdShouldReturnPresentAccount() { + "defaultIntegration: 'web'," + "analyticsConfig: {" + "auction-events: {amp: 'true'}" + + "}," + + "bidValidations: {" + + "banner-creative-max-size: 'enforce'" + "}" + "}" + "]")); @@ -140,6 +145,7 @@ public void getAccountByIdShouldReturnPresentAccount() { .truncateTargetAttr(20) .defaultIntegration("web") .analyticsConfig(AccountAnalyticsConfig.of(singletonMap("amp", true))) + .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 a54f21e9648..253537e6163 100644 --- a/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java +++ b/src/test/java/org/prebid/server/settings/JdbcApplicationSettingsTest.java @@ -24,7 +24,9 @@ import org.prebid.server.metric.Metrics; import org.prebid.server.settings.model.Account; import org.prebid.server.settings.model.AccountAnalyticsConfig; +import org.prebid.server.settings.model.AccountBidValidationConfig; import org.prebid.server.settings.model.AccountGdprConfig; +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; @@ -107,7 +109,7 @@ public static void beforeClass() throws SQLException { + "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, " - + "default_integration varchar(64), analytics_config varchar(512));"); + + "default_integration varchar(64), analytics_config varchar(512), 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, " @@ -125,10 +127,11 @@ public static void beforeClass() throws SQLException { + "varchar(40) 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, default_integration, analytics_config) " + + "tcf_config, analytics_sampling_factor, truncate_target_attr, default_integration, analytics_config, " + + "bid_validations) " + "values ('1001','med', 100, 100, TRUE, TRUE, '{\"enabled\": true, " + "\"integration-enabled\": {\"amp\": true, \"app\": true, \"video\": true, \"web\": true}}', 1, 0, " - + "'web', '{\"auction-events\": {\"amp\": true}}');"); + + "'web', '{\"auction-events\": {\"amp\": true}}', '{\"banner-creative-max-size\": \"enforce\"}');"); connection.createStatement().execute( "insert into s2sconfig_config (uuid, config) values ('adUnitConfigId', 'config');"); connection.createStatement().execute( @@ -195,6 +198,7 @@ public void getAccountByIdShouldReturnAccountWithAllFieldsPopulated(TestContext .truncateTargetAttr(0) .defaultIntegration("web") .analyticsConfig(AccountAnalyticsConfig.of(singletonMap("amp", true))) + .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 77e2e38aa58..61428c1e16e 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -1,86 +1,142 @@ 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; +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.BidderAliases; +import org.prebid.server.auction.model.AuctionContext; 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; import org.prebid.server.validation.model.ValidationResult; import java.math.BigDecimal; import java.util.function.Function; +import static java.util.Collections.singletonList; import static java.util.function.Function.identity; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +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; +import static org.prebid.server.settings.model.BidValidationEnforcement.warn; -public class ResponseBidValidatorTest { +public class ResponseBidValidatorTest extends VertxTest { + + 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; + @Mock + private BidderAliases bidderAliases; + @Before public void setUp() { - responseBidValidator = new ResponseBidValidator(); + responseBidValidator = new ResponseBidValidator(enforce, enforce, metrics); + + given(bidderAliases.resolveBidder(anyString())).willReturn(BIDDER_NAME); } @Test public void validateShouldFailedIfBidderBidCurrencyIsIncorrect() { assertThatIllegalArgumentException().isThrownBy(() -> - responseBidValidator.validate(BidderBid.of( - Bid.builder() - .id("bidId1") - .impid("impId1") - .crid("crid1") - .price(BigDecimal.ONE).build(), - null, - "USDD"))); + responseBidValidator.validate( + BidderBid.of( + Bid.builder() + .id("bidId1") + .impid("impId1") + .crid("crid1") + .price(BigDecimal.ONE) + .build(), + null, + "USDD"), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases)); } @Test - public void validateShouldFailedIfMissingBid() { - final ValidationResult result = responseBidValidator.validate(BidderBid.of(null, null, "USD")); + public void validateShouldFailIfMissingBid() { + // when + final ValidationResult result = responseBidValidator.validate( + BidderBid.of(null, null, "USD"), BIDDER_NAME, givenAuctionContext(), bidderAliases); - assertThat(result.getErrors()).hasSize(1) - .containsOnly("Empty bid object submitted."); + // then + 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() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.id(null)), BIDDER_NAME, givenAuctionContext(), bidderAliases); - assertThat(result.getErrors()).hasSize(1) - .containsOnly("Bid missing required field 'id'"); + // then + 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() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.impid(null)), BIDDER_NAME, givenAuctionContext(), bidderAliases); - assertThat(result.getErrors()).hasSize(1) - .containsOnly("Bid \"bidId1\" missing required field 'impid'"); + // then + 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() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.price(null)), BIDDER_NAME, givenAuctionContext(), bidderAliases); - assertThat(result.getErrors()).hasSize(1) - .containsOnly("Bid \"bidId1\" does not contain a 'price'"); + // then + assertThat(result.getErrors()).hasSize(1).containsOnly("Bid \"bidId1\" does not contain a 'price'"); } @Test - public void validateShouldFailedIfBidHasNegativePrice() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.price( - BigDecimal.valueOf(-1)))); + public void validateShouldFailIfBidHasNegativePrice() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.price(BigDecimal.valueOf(-1))), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases); - assertThat(result.getErrors()).hasSize(1) - .containsOnly("Bid \"bidId1\" `price `has negative value"); + // then + assertThat(result.getErrors()).hasSize(1).containsOnly("Bid \"bidId1\" `price `has negative value"); } @Test public void validateShouldFailedIfNonDealBidHasZeroPrice() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.price( - BigDecimal.valueOf(0)))); + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.price(BigDecimal.valueOf(0))), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases); assertThat(result.getErrors()).hasSize(1) .containsOnly("Non deal bid \"bidId1\" has 0 price"); @@ -88,37 +144,346 @@ public void validateShouldFailedIfNonDealBidHasZeroPrice() { @Test public void validateShouldSuccessForDealZeroPriceBid() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.price( - BigDecimal.valueOf(0)).dealid("dealId"))); + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.price(BigDecimal.valueOf(0)).dealid("dealId")), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases); assertThat(result.hasErrors()).isFalse(); } @Test - public void validateShouldFailedIfBidHasNoCrid() { - final ValidationResult result = responseBidValidator.validate(givenBid(builder -> builder.crid(null))); + public void validateShouldFailIfBidHasNoCrid() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.crid(null)), BIDDER_NAME, givenAuctionContext(), bidderAliases); - assertThat(result.getErrors()).hasSize(1) - .containsOnly("Bid \"bidId1\" missing creative ID"); + // 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)), BIDDER_NAME, givenAuctionContext(), bidderAliases); + + // then + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: 'nullxnull'"); + } + + @Test + public void validateShouldFailIfBannerBidWidthIsGreaterThanImposedByImp() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.w(150).h(150)), BIDDER_NAME, givenAuctionContext(), 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(50).h(250)), BIDDER_NAME, givenAuctionContext(), bidderAliases); + + // then + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has 'w' and 'h' that are not valid. Bid dimensions: '50x250'"); + } + + @Test + public void validateShouldReturnSuccessIfNonBannerBidHasAnySize() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(BidType.video, builder -> builder.w(3).h(3)), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases); + + // then + assertThat(result.hasErrors()).isFalse(); + } + + @Test + public void validateShouldReturnSuccessIfBannerBidHasInvalidSizeButAccountDoesNotEnforceValidation() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.w(150).h(150)), + BIDDER_NAME, + givenAuctionContext( + givenAccount(builder -> builder.bidValidations(AccountBidValidationConfig.of(skip)))), + bidderAliases); + + // then + assertThat(result.hasErrors()).isFalse(); + } + + @Test + public void validateShouldFailIfBidHasNoCorrespondingImp() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.impid("nonExistentsImpid")), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases); + + // then + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has no corresponding imp in request"); + } + + @Test + public void validateShouldFailIfBidHasInsecureMarkerInCreativeInSecureContext() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm("http://site.com/creative.jpg")), + BIDDER_NAME, + givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), + bidderAliases); + + // then + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" has insecure creative but should be in secure context"); + } + + @Test + public void validateShouldFailIfBidHasInsecureEncodedMarkerInCreativeInSecureContext() { + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm("http%3A//site.com/creative.jpg")), + BIDDER_NAME, + givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), + bidderAliases); + + // then + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" 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")), + BIDDER_NAME, + givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), + bidderAliases); + + // then + assertThat(result.getErrors()) + .containsOnly("Bid \"bidId1\" 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")), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases); + + // then + assertThat(result.hasErrors()).isFalse(); } @Test public void validateShouldReturnSuccessfulResultForValidBid() { - final ValidationResult result = responseBidValidator.validate(givenBid(identity())); + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(identity()), + BIDDER_NAME, + givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), + bidderAliases); + + // then + assertThat(result.hasErrors()).isFalse(); + } + @Test + public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { + // given + responseBidValidator = new ResponseBidValidator(skip, enforce, metrics); + + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(identity()), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases); + + // then + 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)), + BIDDER_NAME, + givenAuctionContext(), + 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 + responseBidValidator = new ResponseBidValidator(enforce, skip, metrics); + + // when + final ValidationResult result = responseBidValidator.validate( + givenBid(builder -> builder.adm("http://site.com/creative.jpg")), + BIDDER_NAME, + givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), + bidderAliases); + + // then assertThat(result.hasErrors()).isFalse(); } - private static BidderBid givenBid(Function bidCustomizer, BidType mediaType) { + @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")), + BIDDER_NAME, + givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), + bidderAliases); + + // then + assertThat(result.hasErrors()).isFalse(); + assertThat(result.getWarnings()) + .containsOnly("Bid \"bidId1\" has insecure creative but should be in secure context"); + } + + @Test + public void validateShouldIncrementSizeValidationErrMetrics() { + // when + responseBidValidator.validate( + givenBid(builder -> builder.w(150).h(200)), + BIDDER_NAME, + givenAuctionContext(), + bidderAliases); + + // then + verify(metrics).updateSizeValidationMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.err); + } + + @Test + public void validateShouldIncrementSizeValidationWarnMetrics() { + // given + responseBidValidator = new ResponseBidValidator(warn, warn, metrics); + + // when + responseBidValidator.validate( + givenBid(builder -> builder.w(150).h(200)), + BIDDER_NAME, + givenAuctionContext(), + 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")), + BIDDER_NAME, + givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), + 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")), + BIDDER_NAME, + givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), + bidderAliases); + + // then + verify(metrics).updateSecureValidationMetrics(BIDDER_NAME, ACCOUNT_ID, MetricName.warn); + } + + 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 BidderBid.of(bidCustomizer.apply(bidBuilder).build(), mediaType, "USD"); + + return BidderBid.of(bidCustomizer.apply(bidBuilder).build(), type, "USD"); } - private static BidderBid givenBid(Function bidCustomizer) { - return givenBid(bidCustomizer, null); + private static AuctionContext givenAuctionContext(BidRequest bidRequest, Account account) { + return AuctionContext.builder() + .account(account) + .bidRequest(bidRequest) + .build(); + } + + private static AuctionContext givenAuctionContext(BidRequest bidRequest) { + return givenAuctionContext(bidRequest, givenAccount()); + } + + private static AuctionContext givenAuctionContext(Account account) { + return givenAuctionContext(givenBidRequest(identity()), account); + } + + private static AuctionContext givenAuctionContext() { + return givenAuctionContext(givenBidRequest(identity()), givenAccount()); + } + + private static BidRequest givenBidRequest(Function impCustomizer) { + final Imp.ImpBuilder impBuilder = Imp.builder() + .id("impId1") + .banner(Banner.builder() + .format(singletonList(Format.builder().w(100).h(200).build())) + .build()); + + 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().id(ACCOUNT_ID)).build(); } }