diff --git a/docs/config-app.md b/docs/config-app.md index 1ab92b1c929..24ddbb9a5f7 100644 --- a/docs/config-app.md +++ b/docs/config-app.md @@ -96,6 +96,7 @@ Removes and downloads file again if depending service cant process probably corr - `auction.cache.expected-request-time-ms` - approximate value in milliseconds for Cache Service interacting. - `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.enforce-random-bid-id` - whether to enforce generating a robust random seatbid[].bid[].id in the OpenRTB response if the initial value is less than 17 characters. - `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`. - `auction.host-schain-node` - defines global schain node that will be appended to `request.source.ext.schain.nodes` passed to bidders diff --git a/src/main/java/org/prebid/server/auction/BidResponseCreator.java b/src/main/java/org/prebid/server/auction/BidResponseCreator.java index 4242281a5dd..3522997c400 100644 --- a/src/main/java/org/prebid/server/auction/BidResponseCreator.java +++ b/src/main/java/org/prebid/server/auction/BidResponseCreator.java @@ -59,7 +59,6 @@ import org.prebid.server.hooks.v1.bidder.AllProcessedBidResponsesPayload; import org.prebid.server.hooks.v1.bidder.BidderResponsePayload; import org.prebid.server.identity.IdGenerator; -import org.prebid.server.identity.IdGeneratorType; import org.prebid.server.json.DecodeException; import org.prebid.server.json.JacksonMapper; import org.prebid.server.log.ConditionalLogger; @@ -139,6 +138,7 @@ public class BidResponseCreator { public static final String DEFAULT_DEBUG_KEY = "prebid"; private static final String TARGETING_ENV_APP_VALUE = "mobile-app"; private static final String TARGETING_ENV_AMP_VALUE = "amp"; + private static final int MIN_BID_ID_LENGTH = 17; private final double logSamplingRate; private final CoreCacheService coreCacheService; @@ -148,9 +148,11 @@ public class BidResponseCreator { private final StoredRequestProcessor storedRequestProcessor; private final WinningBidComparatorFactory winningBidComparatorFactory; private final IdGenerator bidIdGenerator; + private final IdGenerator enforcedBidIdGenerator; private final HookStageExecutor hookStageExecutor; private final CategoryMappingService categoryMappingService; private final int truncateAttrChars; + private final boolean enforceRandomBidId; private final Clock clock; private final JacksonMapper mapper; private final Metrics metrics; @@ -169,9 +171,11 @@ public BidResponseCreator(double logSamplingRate, StoredRequestProcessor storedRequestProcessor, WinningBidComparatorFactory winningBidComparatorFactory, IdGenerator bidIdGenerator, + IdGenerator enforcedBidIdGenerator, HookStageExecutor hookStageExecutor, CategoryMappingService categoryMappingService, int truncateAttrChars, + boolean enforceRandomBidId, Clock clock, JacksonMapper mapper, Metrics metrics, @@ -185,9 +189,11 @@ public BidResponseCreator(double logSamplingRate, this.storedRequestProcessor = Objects.requireNonNull(storedRequestProcessor); this.winningBidComparatorFactory = Objects.requireNonNull(winningBidComparatorFactory); this.bidIdGenerator = Objects.requireNonNull(bidIdGenerator); + this.enforcedBidIdGenerator = Objects.requireNonNull(enforcedBidIdGenerator); this.hookStageExecutor = Objects.requireNonNull(hookStageExecutor); this.categoryMappingService = Objects.requireNonNull(categoryMappingService); this.truncateAttrChars = validateTruncateAttrChars(truncateAttrChars); + this.enforceRandomBidId = enforceRandomBidId; this.clock = Objects.requireNonNull(clock); this.mapper = Objects.requireNonNull(mapper); this.mediaTypeCacheTtl = Objects.requireNonNull(mediaTypeCacheTtl); @@ -303,12 +309,12 @@ private Bid updateBid(Bid bid, final Account account = auctionContext.getAccount(); final List debugWarnings = auctionContext.getDebugWarnings(); - final String generatedBidId = bidIdGenerator.getType() != IdGeneratorType.none - ? bidIdGenerator.generateId() - : null; - final String effectiveBidId = ObjectUtils.defaultIfNull(generatedBidId, bid.getId()); + final String generatedBidId = bidIdGenerator.generateId(); + final String enforcedRandomBidId = enforcedBidId(bid); + final String effectiveBidId = ObjectUtils.defaultIfNull(generatedBidId, enforcedRandomBidId); return bid.toBuilder() + .id(enforcedRandomBidId) .adm(updateBidAdm(bid, bidType, bidder, @@ -328,6 +334,13 @@ private Bid updateBid(Bid bid, .build(); } + private String enforcedBidId(Bid bid) { + final int bidIdLength = Optional.ofNullable(bid.getId()).map(String::length).orElse(0); + return enforceRandomBidId && bidIdLength < MIN_BID_ID_LENGTH + ? enforcedBidIdGenerator.generateId() + : bid.getId(); + } + private String updateBidAdm(Bid bid, BidType bidType, String bidder, 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 15cf29a19f1..10fd17ad162 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -824,6 +824,7 @@ BidResponseCreator bidResponseCreator( HookStageExecutor hookStageExecutor, CategoryMappingService categoryMappingService, @Value("${settings.targeting.truncate-attr-chars}") int truncateAttrChars, + @Value("${auction.enforce-random-bid-id:false}") boolean enforceRandomBidId, Clock clock, JacksonMapper mapper, Metrics metrics, @@ -840,9 +841,11 @@ BidResponseCreator bidResponseCreator( storedRequestProcessor, winningBidComparatorFactory, bidIdGenerator, + new UUIDIdGenerator(), hookStageExecutor, categoryMappingService, truncateAttrChars, + enforceRandomBidId, clock, mapper, metrics, diff --git a/src/test/groovy/org/prebid/server/functional/tests/AuctionSpec.groovy b/src/test/groovy/org/prebid/server/functional/tests/AuctionSpec.groovy index f9b9688d4da..e5e9649d82b 100644 --- a/src/test/groovy/org/prebid/server/functional/tests/AuctionSpec.groovy +++ b/src/test/groovy/org/prebid/server/functional/tests/AuctionSpec.groovy @@ -42,14 +42,17 @@ import static org.prebid.server.functional.util.SystemProperties.PBS_VERSION class AuctionSpec extends BaseSpec { private static final String USER_SYNC_URL = "$networkServiceContainer.rootUri/generic-usersync" - private static final boolean CORS_SUPPORT = false + private static final Boolean CORS_SUPPORT = false private static final UserSyncInfo.Type USER_SYNC_TYPE = REDIRECT - private static final int DEFAULT_TIMEOUT = getRandomTimeout() + private static final Integer DEFAULT_TIMEOUT = getRandomTimeout() + private static final Integer MIN_BID_ID_LENGTH = 17 + private static final Integer DEFAULT_UUID_LENGTH = 36 private static final Map PBS_CONFIG = ["auction.biddertmax.max" : MAX_TIMEOUT as String, "auction.default-timeout-ms": DEFAULT_TIMEOUT as String] private static final Map GENERIC_CONFIG = [ "adapters.${GENERIC.value}.usersync.${USER_SYNC_TYPE.value}.url" : USER_SYNC_URL, "adapters.${GENERIC.value}.usersync.${USER_SYNC_TYPE.value}.support-cors": CORS_SUPPORT.toString()] + @Shared PrebidServerService prebidServerService = pbsServiceFactory.getService(PBS_CONFIG) @@ -591,4 +594,131 @@ class AuctionSpec extends BaseSpec { def bidderRequest = bidder.getBidderRequest(bidRequest.id) assert !bidderRequest?.device?.ext?.cdep } + + def "PBS should override short bid.id with random uuid when enforce-random-bid-id is enabled"() { + given: "PBS with enabled generate-bid-id" + def pbsConfig = ['auction.enforce-random-bid-id': 'true'] + def pbsService = pbsServiceFactory.getService(pbsConfig) + + and: "Default bid request" + def bidRequest = BidRequest.defaultBidRequest.tap { + enableEvents() + } + + and: "Default bid response" + def originalBidId = PBSUtils.getRandomString(PBSUtils.getRandomNumber(1, MIN_BID_ID_LENGTH - 1)) + def bidResponse = BidResponse.getDefaultBidResponse(bidRequest).tap { + seatbid.first.bid.first.id = originalBidId + } + bidder.setResponse(bidRequest.id, bidResponse) + + and: "Save account in DB" + def account = new Account(uuid: bidRequest.accountId, eventsEnabled: true) + accountDao.save(account) + + when: "PBS processes auction request" + def response = pbsService.sendAuctionRequest(bidRequest) + + then: "Should include imp from original request" + def bidderRequest = bidder.getBidderRequest(bidRequest.id) + assert bidderRequest.imp.id.sort() == bidRequest.imp.id.sort() + + and: "Bid response should contain changed bid.id for wins event" + def bidIds = response.seatbid.bid.id.flatten() + def bidResponseEvents = response.seatbid.first.bid.first.ext.prebid.events + assert bidResponseEvents.win.contains("win&b=${bidIds.first}") + assert bidResponseEvents.imp.contains("imp&b=${bidIds.first}") + + and: "BidResponse should contain different bid.id" + assert bidIds.sort() != [originalBidId] + + and: "BidResponse should contain generated UUID" + assert PBSUtils.isUUID(response.seatbid.first.bid.first.id) + + cleanup: "Stop and remove pbs container" + pbsServiceFactory.removeContainer(pbsConfig) + } + + def "PBS shouldn't override short bid.id when enforce-random-bid-id in default or disabled"() { + given: "PBS with disabled generate-bid-id" + def pbsConfig = ["auction.enforce-random-bid-id": enforceRandomBidId] + def pbsService = pbsServiceFactory.getService(pbsConfig) + + and: "Default bid request" + def bidRequest = BidRequest.defaultBidRequest.tap { + enableEvents() + } + + and: "Default bid response" + def originalBidId = PBSUtils.getRandomString(PBSUtils.getRandomNumber(0, MIN_BID_ID_LENGTH)) + def bidResponse = BidResponse.getDefaultBidResponse(bidRequest).tap { + seatbid.first.bid.first.id = originalBidId + } + bidder.setResponse(bidRequest.id, bidResponse) + + and: "Save account in DB" + def account = new Account(uuid: bidRequest.accountId, eventsEnabled: true) + accountDao.save(account) + + when: "PBS processes auction request" + def response = pbsService.sendAuctionRequest(bidRequest) + + then: "Should include imp from original request" + def bidderRequest = bidder.getBidderRequest(bidRequest.id) + assert bidderRequest.imp.id.sort() == bidRequest.imp.id.sort() + + and: "Bid response should contain changed bid.id for wins event" + def bidResponseEvents = response.seatbid.first.bid.first.ext.prebid.events + assert bidResponseEvents.win.contains("win&b=${originalBidId}") + assert bidResponseEvents.imp.contains("imp&b=${originalBidId}") + + and: "BidResponse should contain original bid.id" + assert response.seatbid.bid.id.flatten().sort() == [originalBidId] + + cleanup: "Stop and remove pbs container" + pbsServiceFactory.removeContainer(pbsConfig) + + where: + enforceRandomBidId << [null, 'false'] + } + + def "PBS shouldn't override long enough bid.id with random uuid when enforce-random-bid-id is enabled"() { + given: "PBS with enabled generate-bid-id" + def pbsConfig = ['auction.enforce-random-bid-id': 'true'] + def pbsService = pbsServiceFactory.getService(pbsConfig) + + and: "Default bid request" + def bidRequest = BidRequest.defaultBidRequest.tap { + enableEvents() + } + + and: "Default bid response" + def originalBidId = PBSUtils.getRandomString(PBSUtils.getRandomNumber(MIN_BID_ID_LENGTH, DEFAULT_UUID_LENGTH)) + def bidResponse = BidResponse.getDefaultBidResponse(bidRequest).tap { + seatbid.first.bid.first.id = originalBidId + } + bidder.setResponse(bidRequest.id, bidResponse) + + and: "Save account in DB" + def account = new Account(uuid: bidRequest.accountId, eventsEnabled: true) + accountDao.save(account) + + when: "PBS processes auction request" + def response = pbsService.sendAuctionRequest(bidRequest) + + then: "Should include imp from original request" + def bidderRequest = bidder.getBidderRequest(bidRequest.id) + assert bidderRequest.imp.id.sort() == bidRequest.imp.id.sort() + + and: "Bid response should contain changed bid.id for wins event" + def bidResponseEvents = response.seatbid.first.bid.first.ext.prebid.events + assert bidResponseEvents.win.contains("win&b=${originalBidId}") + assert bidResponseEvents.imp.contains("imp&b=${originalBidId}") + + and: "BidResponse should contain original bid.id" + assert response.seatbid.bid.id.flatten().sort() == [originalBidId] + + cleanup: "Stop and remove pbs container" + pbsServiceFactory.removeContainer(pbsConfig) + } } diff --git a/src/test/groovy/org/prebid/server/functional/util/PBSUtils.groovy b/src/test/groovy/org/prebid/server/functional/util/PBSUtils.groovy index c82e96268c6..e1e7750ea05 100644 --- a/src/test/groovy/org/prebid/server/functional/util/PBSUtils.groovy +++ b/src/test/groovy/org/prebid/server/functional/util/PBSUtils.groovy @@ -155,4 +155,16 @@ class PBSUtils implements ObjectMapperWrapper { def version = versionParts.join('.') return (version >= minVersion && version <= maxVersion) ? version : getRandomVersion(minVersion, maxVersion) } + + static Boolean isUUID(String str) { + if (str == null) { + return false + } + try { + UUID.fromString(str) + return true + } catch (IllegalArgumentException e) { + return false + } + } } diff --git a/src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java b/src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java index 04b7d279956..21e20ccbcf1 100644 --- a/src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java +++ b/src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java @@ -68,7 +68,6 @@ import org.prebid.server.hooks.execution.v1.bidder.AllProcessedBidResponsesPayloadImpl; import org.prebid.server.hooks.execution.v1.bidder.BidderResponsePayloadImpl; import org.prebid.server.identity.IdGenerator; -import org.prebid.server.identity.IdGeneratorType; import org.prebid.server.metric.MetricName; import org.prebid.server.metric.Metrics; import org.prebid.server.proto.openrtb.ext.ExtIncludeBrandCategory; @@ -192,6 +191,8 @@ public class BidResponseCreatorTest extends VertxTest { @Mock(strictness = LENIENT) private IdGenerator idGenerator; @Mock(strictness = LENIENT) + private IdGenerator enforcedIdGenerator; + @Mock(strictness = LENIENT) private CategoryMappingService categoryMappingService; @Mock(strictness = LENIENT) private HookStageExecutor hookStageExecutor; @@ -205,7 +206,6 @@ public class BidResponseCreatorTest extends VertxTest { private BidderAliases aliases; @Mock(strictness = LENIENT) private Metrics metrics; - @Spy private WinningBidComparatorFactory winningBidComparatorFactory; @@ -238,7 +238,8 @@ public void setUp() { given(storedRequestProcessor.videoStoredDataResult(any(), anyList(), anyList(), any())) .willReturn(Future.succeededFuture(VideoStoredDataResult.empty())); - given(idGenerator.getType()).willReturn(IdGeneratorType.none); + given(idGenerator.generateId()).willReturn(null); + given(enforcedIdGenerator.generateId()).willReturn(null); given(hookStageExecutor.executeProcessedBidderResponseStage(any(), any())) .willAnswer(invocation -> Future.succeededFuture(HookStageExecutionResult.of( @@ -253,7 +254,7 @@ public void setUp() { clock = Clock.fixed(Instant.ofEpochMilli(1000L), ZoneOffset.UTC); - target = givenBidResponseCreator(0); + target = givenBidResponseCreator(0, false); timeout = new TimeoutFactory(Clock.fixed(Instant.now(), ZoneId.systemDefault())).create(500); } @@ -261,7 +262,7 @@ public void setUp() { @Test public void shouldThrowErrorWhenTruncateAttrCharsLessThatZeroOrBiggestThatTwoHundredFiftyFive() { assertThatIllegalArgumentException() - .isThrownBy(() -> givenBidResponseCreator(-5)) + .isThrownBy(() -> givenBidResponseCreator(-5, false)) .withMessage("truncateAttrChars must be between 0 and 255"); } @@ -270,7 +271,6 @@ public void shouldPassBidWithGeneratedIdAndPreserveExtFieldsWhenIdGeneratorTypeU // given final Imp imp = givenImp(); final String generatedId = "generatedId"; - given(idGenerator.getType()).willReturn(IdGeneratorType.uuid); given(idGenerator.generateId()).willReturn(generatedId); final ObjectNode receivedBidExt = mapper.createObjectNode() @@ -798,7 +798,6 @@ public void shouldOverrideBidIdWhenIdGeneratorIsUUID() { givenBidRequest(givenImp()), contextBuilder -> contextBuilder.auctionParticipations(toAuctionParticipant(bidderResponses))); - given(idGenerator.getType()).willReturn(IdGeneratorType.uuid); given(idGenerator.generateId()).willReturn("de7fc739-0a6e-41ad-8961-701c30c82166"); // when @@ -818,6 +817,88 @@ public void shouldOverrideBidIdWhenIdGeneratorIsUUID() { verify(coreCacheService, never()).cacheBidsOpenrtb(anyList(), any(), any(), any()); } + @Test + public void shouldOverrideBidIdWhenBidIdHasLessThen17CharsAndRandomBidIdGenerationIsEnforced() { + // given + final Bid bid = Bid.builder() + .id("aaaaaaaaaaaaaaaa") + .impid(IMP_ID) + .ext(mapper.createObjectNode() + .set("prebid", mapper.valueToTree(ExtBidPrebid.builder().type(banner).build()))) + .build(); + final List bidderResponses = singletonList( + BidderResponse.of("bidder2", givenSeatBid(BidderBid.of(bid, banner, "USD")), 0)); + + final AuctionContext auctionContext = givenAuctionContext( + givenBidRequest(givenImp()), + contextBuilder -> contextBuilder.auctionParticipations(toAuctionParticipant(bidderResponses))); + + given(idGenerator.generateId()).willReturn("generatedId"); + given(enforcedIdGenerator.generateId()).willReturn("enforcedGeneratedId"); + + target = givenBidResponseCreator(0, true); + + // when + final BidResponse bidResponse = target.create(auctionContext, CACHE_INFO, aliases, MULTI_BIDS).result(); + + // then + assertThat(bidResponse.getSeatbid()) + .flatExtracting(SeatBid::getBid) + .extracting(Bid::getExt) + .extracting(ext -> ext.get("prebid")) + .extracting(extPrebid -> mapper.treeToValue(extPrebid, ExtBidPrebid.class)) + .extracting(ExtBidPrebid::getBidid) + .containsOnly("generatedId"); + + assertThat(bidResponse.getSeatbid()) + .flatExtracting(SeatBid::getBid) + .extracting(Bid::getId) + .containsOnly("enforcedGeneratedId"); + + verify(coreCacheService, never()).cacheBidsOpenrtb(anyList(), any(), any(), any()); + } + + @Test + public void shouldNotOverrideBidIdWhenBidIdHas17CharsAndRandomBidIdGenerationIsEnforced() { + // given + final Bid bid = Bid.builder() + .id("aaaaaaaaaaaaaaaaa") + .impid(IMP_ID) + .ext(mapper.createObjectNode() + .set("prebid", mapper.valueToTree(ExtBidPrebid.builder().type(banner).build()))) + .build(); + final List bidderResponses = singletonList( + BidderResponse.of("bidder2", givenSeatBid(BidderBid.of(bid, banner, "USD")), 0)); + + final AuctionContext auctionContext = givenAuctionContext( + givenBidRequest(givenImp()), + contextBuilder -> contextBuilder.auctionParticipations(toAuctionParticipant(bidderResponses))); + + given(idGenerator.generateId()).willReturn("generatedId"); + given(enforcedIdGenerator.generateId()).willReturn("enforcedGeneratedId"); + + target = givenBidResponseCreator(0, true); + + // when + final BidResponse bidResponse = target.create(auctionContext, CACHE_INFO, aliases, MULTI_BIDS).result(); + + // then + assertThat(bidResponse.getSeatbid()) + .flatExtracting(SeatBid::getBid) + .extracting(Bid::getExt) + .extracting(ext -> ext.get("prebid")) + .extracting(extPrebid -> mapper.treeToValue(extPrebid, ExtBidPrebid.class)) + .extracting(ExtBidPrebid::getBidid) + .containsOnly("generatedId"); + + assertThat(bidResponse.getSeatbid()) + .flatExtracting(SeatBid::getBid) + .extracting(Bid::getId) + .containsOnly("aaaaaaaaaaaaaaaaa"); + + verify(coreCacheService, never()).cacheBidsOpenrtb(anyList(), any(), any(), any()); + } + @Test public void shouldUseGeneratedBidIdForEventAndCacheWhenIdGeneratorIsUUIDAndEventEnabledForAccountAndRequest() { // given @@ -854,7 +935,6 @@ public void shouldUseGeneratedBidIdForEventAndCacheWhenIdGeneratorIsUUIDAndEvent .auctionParticipations(toAuctionParticipant(bidderResponses))); final String generatedBidId = "de7fc739-0a6e-41ad-8961-701c30c82166"; - given(idGenerator.getType()).willReturn(IdGeneratorType.uuid); given(idGenerator.generateId()).willReturn(generatedBidId); final BidRequestCacheInfo cacheInfo = BidRequestCacheInfo.builder().doCaching(true).build(); @@ -1656,9 +1736,11 @@ public void shouldTruncateTargetingKeywordsByGlobalConfig() { storedRequestProcessor, winningBidComparatorFactory, idGenerator, + enforcedIdGenerator, hookStageExecutor, categoryMappingService, 20, + false, clock, jacksonMapper, metrics, @@ -5462,7 +5544,7 @@ private static ExtBidPrebid toExtBidPrebid(ObjectNode ext) { } } - private BidResponseCreator givenBidResponseCreator(int truncateAttrChars) { + private BidResponseCreator givenBidResponseCreator(int truncateAttrChars, boolean enforcedRandomBidId) { return new BidResponseCreator( 0, coreCacheService, @@ -5472,9 +5554,11 @@ private BidResponseCreator givenBidResponseCreator(int truncateAttrChars) { storedRequestProcessor, winningBidComparatorFactory, idGenerator, + enforcedIdGenerator, hookStageExecutor, categoryMappingService, truncateAttrChars, + enforcedRandomBidId, clock, jacksonMapper, metrics,