From 771a700acd67fb54c24b1f2e1e17461b1f640a13 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Fri, 28 Mar 2025 16:45:05 -0400 Subject: [PATCH] Configure fail-open policy on individual rate limiters --- .../dynamic/DynamicConfiguration.java | 8 -- .../dynamic/DynamicRateLimitPolicy.java | 8 -- .../limits/BaseRateLimiters.java | 2 +- .../limits/DynamicRateLimiter.java | 2 +- .../limits/RateLimiterConfig.java | 2 +- .../textsecuregcm/limits/RateLimiters.java | 81 +++++++++---------- .../limits/StaticRateLimiter.java | 29 +++---- .../limits/RateLimiterConfigTest.java | 13 ++- .../limits/RateLimitersLuaScriptTest.java | 23 ++++-- .../limits/RateLimitersTest.java | 38 ++++----- 10 files changed, 93 insertions(+), 113 deletions(-) delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicRateLimitPolicy.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfiguration.java index bf7ca38f7..01273c035 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicConfiguration.java @@ -46,10 +46,6 @@ public class DynamicConfiguration { @Valid DynamicMessagePersisterConfiguration messagePersister = new DynamicMessagePersisterConfiguration(); - @JsonProperty - @Valid - DynamicRateLimitPolicy rateLimitPolicy = new DynamicRateLimitPolicy(false); - @JsonProperty @Valid DynamicRegistrationConfiguration registrationConfiguration = new DynamicRegistrationConfiguration(false); @@ -100,10 +96,6 @@ public class DynamicConfiguration { return messagePersister; } - public DynamicRateLimitPolicy getRateLimitPolicy() { - return rateLimitPolicy; - } - public DynamicRegistrationConfiguration getRegistrationConfiguration() { return registrationConfiguration; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicRateLimitPolicy.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicRateLimitPolicy.java deleted file mode 100644 index 3fb0172ea..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicRateLimitPolicy.java +++ /dev/null @@ -1,8 +0,0 @@ -/* - * Copyright 2023 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.configuration.dynamic; - -public record DynamicRateLimitPolicy(boolean failOpen) {} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/BaseRateLimiters.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/BaseRateLimiters.java index 0a0ef700a..bcc3a4ebf 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/BaseRateLimiters.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/BaseRateLimiters.java @@ -95,6 +95,6 @@ public abstract class BaseRateLimiters { return new DynamicRateLimiter(descriptor.id(), dynamicConfigurationManager, configResolver, validateScript, cacheCluster, clock); } final RateLimiterConfig cfg = configs.getOrDefault(descriptor.id(), descriptor.defaultConfig()); - return new StaticRateLimiter(descriptor.id(), cfg, validateScript, cacheCluster, clock, dynamicConfigurationManager); + return new StaticRateLimiter(descriptor.id(), cfg, validateScript, cacheCluster, clock); } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/DynamicRateLimiter.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/DynamicRateLimiter.java index c8f7e42e6..1f60550a6 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/DynamicRateLimiter.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/DynamicRateLimiter.java @@ -87,7 +87,7 @@ public class DynamicRateLimiter implements RateLimiter { final RateLimiterConfig cfg = configResolver.get(); return currentHolder.updateAndGet(p -> p != null && p.getLeft().equals(cfg) ? p - : Pair.of(cfg, new StaticRateLimiter(name, cfg, validateScript, cluster, clock, dynamicConfigurationManager)) + : Pair.of(cfg, new StaticRateLimiter(name, cfg, validateScript, cluster, clock)) ); } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiterConfig.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiterConfig.java index b6e5405da..703416df6 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiterConfig.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiterConfig.java @@ -8,7 +8,7 @@ package org.whispersystems.textsecuregcm.limits; import jakarta.validation.constraints.AssertTrue; import java.time.Duration; -public record RateLimiterConfig(int bucketSize, Duration permitRegenerationDuration) { +public record RateLimiterConfig(int bucketSize, Duration permitRegenerationDuration, boolean failOpen) { public double leakRatePerMillis() { return 1.0 / (permitRegenerationDuration.toNanos() / 1e6); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiters.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiters.java index c4efab116..25d3e8e2f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiters.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiters.java @@ -17,47 +17,46 @@ import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; public class RateLimiters extends BaseRateLimiters { public enum For implements RateLimiterDescriptor { - BACKUP_AUTH_CHECK("backupAuthCheck", false, new RateLimiterConfig(100, Duration.ofMinutes(15))), - PIN("pin", false, new RateLimiterConfig(10, Duration.ofDays(1))), - ATTACHMENT("attachmentCreate", false, new RateLimiterConfig(50, Duration.ofMillis(1200))), - BACKUP_ATTACHMENT("backupAttachmentCreate", true, new RateLimiterConfig(10_000, Duration.ofSeconds(1))), - PRE_KEYS("prekeys", false, new RateLimiterConfig(6, Duration.ofMinutes(10))), - MESSAGES("messages", false, new RateLimiterConfig(60, Duration.ofSeconds(1))), - STORIES("stories", false, new RateLimiterConfig(5_000, Duration.ofSeconds(8))), - ALLOCATE_DEVICE("allocateDevice", false, new RateLimiterConfig(6, Duration.ofMinutes(2))), - VERIFY_DEVICE("verifyDevice", false, new RateLimiterConfig(6, Duration.ofMinutes(2))), - PROFILE("profile", false, new RateLimiterConfig(4320, Duration.ofSeconds(20))), - STICKER_PACK("stickerPack", false, new RateLimiterConfig(50, Duration.ofMinutes(72))), - USERNAME_LOOKUP("usernameLookup", false, new RateLimiterConfig(100, Duration.ofMinutes(15))), - USERNAME_SET("usernameSet", false, new RateLimiterConfig(100, Duration.ofMinutes(15))), - USERNAME_RESERVE("usernameReserve", false, new RateLimiterConfig(100, Duration.ofMinutes(15))), - USERNAME_LINK_OPERATION("usernameLinkOperation", false, new RateLimiterConfig(10, Duration.ofMinutes(1))), - USERNAME_LINK_LOOKUP_PER_IP("usernameLinkLookupPerIp", false, new RateLimiterConfig(100, Duration.ofSeconds(15))), - CHECK_ACCOUNT_EXISTENCE("checkAccountExistence", false, new RateLimiterConfig(1000, Duration.ofSeconds(4))), - REGISTRATION("registration", false, new RateLimiterConfig(6, Duration.ofSeconds(30))), - VERIFICATION_PUSH_CHALLENGE("verificationPushChallenge", false, new RateLimiterConfig(5, Duration.ofSeconds(30))), - VERIFICATION_CAPTCHA("verificationCaptcha", false, new RateLimiterConfig(10, Duration.ofSeconds(30))), - RATE_LIMIT_RESET("rateLimitReset", true, new RateLimiterConfig(2, Duration.ofHours(12))), - CAPTCHA_CHALLENGE_ATTEMPT("captchaChallengeAttempt", true, new RateLimiterConfig(10, Duration.ofMinutes(144))), - CAPTCHA_CHALLENGE_SUCCESS("captchaChallengeSuccess", true, new RateLimiterConfig(2, Duration.ofHours(12))), - SET_BACKUP_ID("setBackupId", true, new RateLimiterConfig(10, Duration.ofHours(1))), - SET_PAID_MEDIA_BACKUP_ID("setPaidMediaBackupId", true, new RateLimiterConfig(5, Duration.ofDays(7))), - PUSH_CHALLENGE_ATTEMPT("pushChallengeAttempt", true, new RateLimiterConfig(10, Duration.ofMinutes(144))), - PUSH_CHALLENGE_SUCCESS("pushChallengeSuccess", true, new RateLimiterConfig(2, Duration.ofHours(12))), - GET_CALLING_RELAYS("getCallingRelays", false, new RateLimiterConfig(100, Duration.ofMinutes(10))), - CREATE_CALL_LINK("createCallLink", false, new RateLimiterConfig(100, Duration.ofMinutes(15))), - INBOUND_MESSAGE_BYTES("inboundMessageBytes", true, new RateLimiterConfig(128 * 1024 * 1024, Duration.ofNanos(500_000))), - EXTERNAL_SERVICE_CREDENTIALS("externalServiceCredentials", true, new RateLimiterConfig(100, Duration.ofMinutes(15))), - KEY_TRANSPARENCY_DISTINGUISHED_PER_IP("keyTransparencyDistinguished", true, - new RateLimiterConfig(100, Duration.ofSeconds(15))), - KEY_TRANSPARENCY_SEARCH_PER_IP("keyTransparencySearch", true, new RateLimiterConfig(100, Duration.ofSeconds(15))), - KEY_TRANSPARENCY_MONITOR_PER_IP("keyTransparencyMonitor", true, new RateLimiterConfig(100, Duration.ofSeconds(15))), - WAIT_FOR_LINKED_DEVICE("waitForLinkedDevice", true, new RateLimiterConfig(10, Duration.ofSeconds(30))), - UPLOAD_TRANSFER_ARCHIVE("uploadTransferArchive", true, new RateLimiterConfig(10, Duration.ofMinutes(1))), - WAIT_FOR_TRANSFER_ARCHIVE("waitForTransferArchive", true, new RateLimiterConfig(10, Duration.ofSeconds(30))), - RECORD_DEVICE_TRANSFER_REQUEST("recordDeviceTransferRequest", true, new RateLimiterConfig(10, Duration.ofMillis(100))), - WAIT_FOR_DEVICE_TRANSFER_REQUEST("waitForDeviceTransferRequest", true, new RateLimiterConfig(10, Duration.ofMillis(100))), - DEVICE_CHECK_CHALLENGE("deviceCheckChallenge", true, new RateLimiterConfig(10, Duration.ofMinutes(1))), + BACKUP_AUTH_CHECK("backupAuthCheck", false, new RateLimiterConfig(100, Duration.ofMinutes(15), false)), + PIN("pin", false, new RateLimiterConfig(10, Duration.ofDays(1), false)), + ATTACHMENT("attachmentCreate", false, new RateLimiterConfig(50, Duration.ofMillis(1200), false)), + BACKUP_ATTACHMENT("backupAttachmentCreate", true, new RateLimiterConfig(10_000, Duration.ofSeconds(1), false)), + PRE_KEYS("prekeys", false, new RateLimiterConfig(6, Duration.ofMinutes(10), false)), + MESSAGES("messages", false, new RateLimiterConfig(60, Duration.ofSeconds(1), false)), + STORIES("stories", false, new RateLimiterConfig(5_000, Duration.ofSeconds(8), false)), + ALLOCATE_DEVICE("allocateDevice", false, new RateLimiterConfig(6, Duration.ofMinutes(2), false)), + VERIFY_DEVICE("verifyDevice", false, new RateLimiterConfig(6, Duration.ofMinutes(2), false)), + PROFILE("profile", false, new RateLimiterConfig(4320, Duration.ofSeconds(20), false)), + STICKER_PACK("stickerPack", false, new RateLimiterConfig(50, Duration.ofMinutes(72), false)), + USERNAME_LOOKUP("usernameLookup", false, new RateLimiterConfig(100, Duration.ofMinutes(15), false)), + USERNAME_SET("usernameSet", false, new RateLimiterConfig(100, Duration.ofMinutes(15), false)), + USERNAME_RESERVE("usernameReserve", false, new RateLimiterConfig(100, Duration.ofMinutes(15), false)), + USERNAME_LINK_OPERATION("usernameLinkOperation", false, new RateLimiterConfig(10, Duration.ofMinutes(1), false)), + USERNAME_LINK_LOOKUP_PER_IP("usernameLinkLookupPerIp", false, new RateLimiterConfig(100, Duration.ofSeconds(15), false)), + CHECK_ACCOUNT_EXISTENCE("checkAccountExistence", false, new RateLimiterConfig(1000, Duration.ofSeconds(4), false)), + REGISTRATION("registration", false, new RateLimiterConfig(6, Duration.ofSeconds(30), false)), + VERIFICATION_PUSH_CHALLENGE("verificationPushChallenge", false, new RateLimiterConfig(5, Duration.ofSeconds(30), false)), + VERIFICATION_CAPTCHA("verificationCaptcha", false, new RateLimiterConfig(10, Duration.ofSeconds(30), false)), + RATE_LIMIT_RESET("rateLimitReset", true, new RateLimiterConfig(2, Duration.ofHours(12), false)), + CAPTCHA_CHALLENGE_ATTEMPT("captchaChallengeAttempt", true, new RateLimiterConfig(10, Duration.ofMinutes(144), false)), + CAPTCHA_CHALLENGE_SUCCESS("captchaChallengeSuccess", true, new RateLimiterConfig(2, Duration.ofHours(12), false)), + SET_BACKUP_ID("setBackupId", true, new RateLimiterConfig(10, Duration.ofHours(1), false)), + SET_PAID_MEDIA_BACKUP_ID("setPaidMediaBackupId", true, new RateLimiterConfig(5, Duration.ofDays(7), false)), + PUSH_CHALLENGE_ATTEMPT("pushChallengeAttempt", true, new RateLimiterConfig(10, Duration.ofMinutes(144), false)), + PUSH_CHALLENGE_SUCCESS("pushChallengeSuccess", true, new RateLimiterConfig(2, Duration.ofHours(12), false)), + GET_CALLING_RELAYS("getCallingRelays", false, new RateLimiterConfig(100, Duration.ofMinutes(10), false)), + CREATE_CALL_LINK("createCallLink", false, new RateLimiterConfig(100, Duration.ofMinutes(15), false)), + INBOUND_MESSAGE_BYTES("inboundMessageBytes", true, new RateLimiterConfig(128 * 1024 * 1024, Duration.ofNanos(500_000), false)), + EXTERNAL_SERVICE_CREDENTIALS("externalServiceCredentials", true, new RateLimiterConfig(100, Duration.ofMinutes(15), false)), + KEY_TRANSPARENCY_DISTINGUISHED_PER_IP("keyTransparencyDistinguished", true, new RateLimiterConfig(100, Duration.ofSeconds(15), false)), + KEY_TRANSPARENCY_SEARCH_PER_IP("keyTransparencySearch", true, new RateLimiterConfig(100, Duration.ofSeconds(15), false)), + KEY_TRANSPARENCY_MONITOR_PER_IP("keyTransparencyMonitor", true, new RateLimiterConfig(100, Duration.ofSeconds(15), false)), + WAIT_FOR_LINKED_DEVICE("waitForLinkedDevice", true, new RateLimiterConfig(10, Duration.ofSeconds(30), false)), + UPLOAD_TRANSFER_ARCHIVE("uploadTransferArchive", true, new RateLimiterConfig(10, Duration.ofMinutes(1), false)), + WAIT_FOR_TRANSFER_ARCHIVE("waitForTransferArchive", true, new RateLimiterConfig(10, Duration.ofSeconds(30), false)), + RECORD_DEVICE_TRANSFER_REQUEST("recordDeviceTransferRequest", true, new RateLimiterConfig(10, Duration.ofMillis(100), false)), + WAIT_FOR_DEVICE_TRANSFER_REQUEST("waitForDeviceTransferRequest", true, new RateLimiterConfig(10, Duration.ofMillis(100), false)), + DEVICE_CHECK_CHALLENGE("deviceCheckChallenge", true, new RateLimiterConfig(10, Duration.ofMinutes(1), false)), ; private final String id; diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/StaticRateLimiter.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/StaticRateLimiter.java index 24697951f..28b7c31b4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/StaticRateLimiter.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/StaticRateLimiter.java @@ -15,12 +15,10 @@ import java.time.Clock; import java.time.Duration; import java.util.List; import java.util.concurrent.CompletionStage; -import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; import org.whispersystems.textsecuregcm.metrics.MetricsUtil; import org.whispersystems.textsecuregcm.redis.ClusterLuaScript; import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisClusterClient; -import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; import org.whispersystems.textsecuregcm.util.ExceptionUtils; import org.whispersystems.textsecuregcm.util.Util; @@ -30,8 +28,7 @@ public class StaticRateLimiter implements RateLimiter { private final RateLimiterConfig config; - private final Counter counter; - private final DynamicConfigurationManager dynamicConfigurationManager; + private final Counter limitExceededCounter; private final ClusterLuaScript validateScript; @@ -45,15 +42,13 @@ public class StaticRateLimiter implements RateLimiter { final RateLimiterConfig config, final ClusterLuaScript validateScript, final FaultTolerantRedisClusterClient cacheCluster, - final Clock clock, - final DynamicConfigurationManager dynamicConfigurationManager) { + final Clock clock) { this.name = requireNonNull(name); this.config = requireNonNull(config); this.validateScript = requireNonNull(validateScript); this.cacheCluster = requireNonNull(cacheCluster); this.clock = requireNonNull(clock); - this.counter = Metrics.counter(MetricsUtil.name(getClass(), "exceeded"), "rateLimiterName", name); - this.dynamicConfigurationManager = dynamicConfigurationManager; + this.limitExceededCounter = Metrics.counter(MetricsUtil.name(getClass(), "exceeded"), "rateLimiterName", name); } @Override @@ -61,13 +56,13 @@ public class StaticRateLimiter implements RateLimiter { try { final long deficitPermitsAmount = executeValidateScript(key, amount, true); if (deficitPermitsAmount > 0) { - counter.increment(); + limitExceededCounter.increment(); final Duration retryAfter = Duration.ofMillis( (long) Math.ceil((double) deficitPermitsAmount / config.leakRatePerMillis())); throw new RateLimitExceededException(retryAfter); } } catch (final Exception e) { - if (!failOpen()) { + if (!config.failOpen()) { throw e; } } @@ -80,16 +75,16 @@ public class StaticRateLimiter implements RateLimiter { if (deficitPermitsAmount == 0) { return completedFuture((Void) null); } - counter.increment(); + limitExceededCounter.increment(); final Duration retryAfter = Duration.ofMillis( (long) Math.ceil((double) deficitPermitsAmount / config.leakRatePerMillis())); return failedFuture(new RateLimitExceededException(retryAfter)); }) .exceptionally(throwable -> { - if (failOpen()) { + if (config.failOpen()) { return null; } - throw ExceptionUtils.wrap(throwable); + throw ExceptionUtils.wrap(new RateLimitExceededException(null)); }); } @@ -99,7 +94,7 @@ public class StaticRateLimiter implements RateLimiter { final long deficitPermitsAmount = executeValidateScript(key, amount, false); return deficitPermitsAmount == 0; } catch (final Exception e) { - if (failOpen()) { + if (config.failOpen()) { return true; } else { throw e; @@ -112,7 +107,7 @@ public class StaticRateLimiter implements RateLimiter { return executeValidateScriptAsync(key, amount, false) .thenApply(deficitPermitsAmount -> deficitPermitsAmount == 0) .exceptionally(throwable -> { - if (failOpen()) { + if (config.failOpen()) { return true; } throw ExceptionUtils.wrap(throwable); @@ -135,10 +130,6 @@ public class StaticRateLimiter implements RateLimiter { return config; } - private boolean failOpen() { - return this.dynamicConfigurationManager.getConfiguration().getRateLimitPolicy().failOpen(); - } - private long executeValidateScript(final String key, final int amount, final boolean applyChanges) { final List keys = List.of(bucketName(name, key)); final List arguments = List.of( diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimiterConfigTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimiterConfigTest.java index 6e5c82465..42bdd2315 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimiterConfigTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimiterConfigTest.java @@ -8,7 +8,6 @@ package org.whispersystems.textsecuregcm.limits; import org.junit.jupiter.api.Test; import java.time.Duration; -import java.util.Optional; import static org.junit.jupiter.api.Assertions.*; @@ -16,15 +15,15 @@ class RateLimiterConfigTest { @Test void leakRatePerMillis() { - assertEquals(0.001, new RateLimiterConfig(1, Duration.ofSeconds(1)).leakRatePerMillis()); - assertEquals(1e6, new RateLimiterConfig(1, Duration.ofNanos(1)).leakRatePerMillis()); + assertEquals(0.001, new RateLimiterConfig(1, Duration.ofSeconds(1), false).leakRatePerMillis()); + assertEquals(1e6, new RateLimiterConfig(1, Duration.ofNanos(1), false).leakRatePerMillis()); } @Test void isRegenerationRatePositive() { - assertTrue(new RateLimiterConfig(1, Duration.ofSeconds(1)).hasPositiveRegenerationRate()); - assertTrue(new RateLimiterConfig(1, Duration.ofNanos(1)).hasPositiveRegenerationRate()); - assertFalse(new RateLimiterConfig(1, Duration.ZERO).hasPositiveRegenerationRate()); - assertFalse(new RateLimiterConfig(1, Duration.ofSeconds(-1)).hasPositiveRegenerationRate()); + assertTrue(new RateLimiterConfig(1, Duration.ofSeconds(1), false).hasPositiveRegenerationRate()); + assertTrue(new RateLimiterConfig(1, Duration.ofNanos(1), false).hasPositiveRegenerationRate()); + assertFalse(new RateLimiterConfig(1, Duration.ZERO, false).hasPositiveRegenerationRate()); + assertFalse(new RateLimiterConfig(1, Duration.ofSeconds(-1), false).hasPositiveRegenerationRate()); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitersLuaScriptTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitersLuaScriptTest.java index c91a04c72..c1346bccc 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitersLuaScriptTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitersLuaScriptTest.java @@ -5,6 +5,7 @@ package org.whispersystems.textsecuregcm.limits; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -22,8 +23,9 @@ import java.util.Map; import java.util.Optional; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; -import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicRateLimitPolicy; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisClusterClient; import org.whispersystems.textsecuregcm.redis.RedisClusterExtension; @@ -57,7 +59,7 @@ public class RateLimitersLuaScriptTest { final RateLimiters.For descriptor = RateLimiters.For.REGISTRATION; final FaultTolerantRedisClusterClient redisCluster = REDIS_CLUSTER_EXTENSION.getRedisCluster(); final RateLimiters limiters = new RateLimiters( - Map.of(descriptor.id(), new RateLimiterConfig(60, Duration.ofSeconds(1))), + Map.of(descriptor.id(), new RateLimiterConfig(60, Duration.ofSeconds(1), false)), dynamicConfig, RateLimiters.defaultScript(redisCluster), redisCluster, @@ -74,7 +76,7 @@ public class RateLimitersLuaScriptTest { final RateLimiters.For descriptor = RateLimiters.For.REGISTRATION; final FaultTolerantRedisClusterClient redisCluster = REDIS_CLUSTER_EXTENSION.getRedisCluster(); final RateLimiters limiters = new RateLimiters( - Map.of(descriptor.id(), new RateLimiterConfig(1000, Duration.ofSeconds(1))), + Map.of(descriptor.id(), new RateLimiterConfig(1000, Duration.ofSeconds(1), false)), dynamicConfig, RateLimiters.defaultScript(redisCluster), redisCluster, @@ -119,20 +121,25 @@ public class RateLimitersLuaScriptTest { assertEquals(750L, decodeBucket(key).orElseThrow().tokensRemaining); } - @Test - public void testFailOpen() throws Exception { - when(configuration.getRateLimitPolicy()).thenReturn(new DynamicRateLimitPolicy(true)); + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testFailOpen(final boolean failOpen) { final RateLimiters.For descriptor = RateLimiters.For.REGISTRATION; final FaultTolerantRedisClusterClient redisCluster = mock(FaultTolerantRedisClusterClient.class); final RateLimiters limiters = new RateLimiters( - Map.of(descriptor.id(), new RateLimiterConfig(1000, Duration.ofSeconds(1))), + Map.of(descriptor.id(), new RateLimiterConfig(1000, Duration.ofSeconds(1), failOpen)), dynamicConfig, RateLimiters.defaultScript(redisCluster), redisCluster, Clock.systemUTC()); when(redisCluster.withCluster(any())).thenThrow(new RedisException("fail")); final RateLimiter rateLimiter = limiters.forDescriptor(descriptor); - rateLimiter.validate("test", 200); + + if (failOpen) { + assertDoesNotThrow(() -> rateLimiter.validate("test", 200)); + } else { + assertThrows(RedisException.class, () -> rateLimiter.validate("test", 200)); + } } private String serializeToOldBucketValueFormat( diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitersTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitersTest.java index a5732e64e..fcf783df5 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitersTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitersTest.java @@ -5,9 +5,9 @@ package org.whispersystems.textsecuregcm.limits; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -20,7 +20,6 @@ import java.util.HashMap; import java.util.Map; import org.junit.jupiter.api.Test; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; -import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicRateLimitPolicy; import org.whispersystems.textsecuregcm.redis.ClusterLuaScript; import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisClusterClient; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; @@ -56,30 +55,31 @@ public class RateLimitersTest { prekeys: bucketSize: 150 permitRegenerationDuration: PT6S + failOpen: true attachmentCreate: bucketSize: 4 permitRegenerationDuration: PT30S - rateLimitPolicy: - failOpen: true + failOpen: true """; - public record GenericHolder( - @Valid @NotNull @JsonProperty Map limits, - @Valid @JsonProperty DynamicRateLimitPolicy rateLimitPolicy) { + public record SimpleDynamicConfiguration(@Valid @NotNull @JsonProperty Map limits) { } @Test public void testValidateConfigs() throws Exception { assertThrows(IllegalArgumentException.class, () -> { - final GenericHolder cfg = DynamicConfigurationManager.parseConfiguration(BAD_YAML, GenericHolder.class).orElseThrow(); - final RateLimiters rateLimiters = new RateLimiters(cfg.limits(), dynamicConfig, validateScript, redisCluster, clock); + final SimpleDynamicConfiguration dynamicConfiguration = + DynamicConfigurationManager.parseConfiguration(BAD_YAML, SimpleDynamicConfiguration.class).orElseThrow(); + + final RateLimiters rateLimiters = new RateLimiters(dynamicConfiguration.limits(), dynamicConfig, validateScript, redisCluster, clock); rateLimiters.validateValuesAndConfigs(); }); - final GenericHolder cfg = DynamicConfigurationManager.parseConfiguration(GOOD_YAML, GenericHolder.class).orElseThrow(); - assertTrue(cfg.rateLimitPolicy.failOpen()); - final RateLimiters rateLimiters = new RateLimiters(cfg.limits(), dynamicConfig, validateScript, redisCluster, clock); - rateLimiters.validateValuesAndConfigs(); + final SimpleDynamicConfiguration dynamicConfiguration = + DynamicConfigurationManager.parseConfiguration(GOOD_YAML, SimpleDynamicConfiguration.class).orElseThrow(); + + final RateLimiters rateLimiters = new RateLimiters(dynamicConfiguration.limits(), dynamicConfig, validateScript, redisCluster, clock); + assertDoesNotThrow(rateLimiters::validateValuesAndConfigs); } @Test @@ -116,9 +116,9 @@ public class RateLimitersTest { @Test void testChangingConfiguration() { - final RateLimiterConfig initialRateLimiterConfig = new RateLimiterConfig(4, Duration.ofMinutes(1)); - final RateLimiterConfig updatedRateLimiterCongig = new RateLimiterConfig(17, Duration.ofSeconds(3)); - final RateLimiterConfig baseConfig = new RateLimiterConfig(1, Duration.ofMinutes(1)); + final RateLimiterConfig initialRateLimiterConfig = new RateLimiterConfig(4, Duration.ofMinutes(1), false); + final RateLimiterConfig updatedRateLimiterCongig = new RateLimiterConfig(17, Duration.ofSeconds(3), false); + final RateLimiterConfig baseConfig = new RateLimiterConfig(1, Duration.ofMinutes(1), false); final Map limitsConfigMap = new HashMap<>(); @@ -146,8 +146,8 @@ public class RateLimitersTest { @Test public void testRateLimiterHasItsPrioritiesStraight() throws Exception { final RateLimiters.For descriptor = RateLimiters.For.CAPTCHA_CHALLENGE_ATTEMPT; - final RateLimiterConfig configForDynamic = new RateLimiterConfig(1, Duration.ofMinutes(1)); - final RateLimiterConfig configForStatic = new RateLimiterConfig(2, Duration.ofSeconds(30)); + final RateLimiterConfig configForDynamic = new RateLimiterConfig(1, Duration.ofMinutes(1), false); + final RateLimiterConfig configForStatic = new RateLimiterConfig(2, Duration.ofSeconds(30), false); final RateLimiterConfig defaultConfig = descriptor.defaultConfig(); final Map mapForDynamic = new HashMap<>(); @@ -188,7 +188,7 @@ public class RateLimitersTest { @Override public RateLimiterConfig defaultConfig() { - return new RateLimiterConfig(1, Duration.ofMinutes(1)); + return new RateLimiterConfig(1, Duration.ofMinutes(1), false); } }