From 360a4793aea9880515b43802331ba5c695f80878 Mon Sep 17 00:00:00 2001
From: Ravi Khadiwala <ravi@signal.org>
Date: Tue, 16 Jan 2024 19:24:45 -0600
Subject: [PATCH] Remove global lock in RateLimitChallengeManager

---
 .../textsecuregcm/WhisperServerService.java        |  9 ++++++---
 .../limits/RateLimitChallengeManager.java          | 14 ++++----------
 .../limits/RateLimitChallengeManagerTest.java      |  6 +++---
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java
index 67d9535fd..0a2fbd23c 100644
--- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java
@@ -29,6 +29,7 @@ import io.micrometer.core.instrument.binder.jvm.ExecutorServiceMetrics;
 import java.net.http.HttpClient;
 import java.time.Clock;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
@@ -603,8 +604,6 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
 
     PushChallengeManager pushChallengeManager = new PushChallengeManager(pushNotificationManager,
         pushChallengeDynamoDb);
-    RateLimitChallengeManager rateLimitChallengeManager = new RateLimitChallengeManager(pushChallengeManager,
-        captchaChecker, rateLimiters);
 
     ChangeNumberManager changeNumberManager = new ChangeNumberManager(messageSender, accountsManager);
 
@@ -740,6 +739,7 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
     boolean registeredSpamFilter = false;
     ReportSpamTokenProvider reportSpamTokenProvider = null;
 
+    List<RateLimitChallengeListener> rateLimitChallengeListeners = new ArrayList<>();
     for (final SpamFilter filter : ServiceLoader.load(SpamFilter.class)) {
       if (filter.getClass().isAnnotationPresent(FilterSpam.class)) {
         try {
@@ -770,9 +770,12 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
 
       if (filter instanceof RateLimitChallengeListener) {
         log.info("Registered rate limit challenge listener: {}", filter.getClass().getName());
-        rateLimitChallengeManager.addListener((RateLimitChallengeListener) filter);
+        rateLimitChallengeListeners.add((RateLimitChallengeListener) filter);
       }
     }
+    RateLimitChallengeManager rateLimitChallengeManager = new RateLimitChallengeManager(pushChallengeManager,
+        captchaChecker, rateLimiters, rateLimitChallengeListeners);
+
 
     if (!registeredSpamFilter) {
       log.warn("No spam filters installed");
diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java
index 26066b27d..21014a096 100644
--- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java
@@ -11,11 +11,8 @@ import io.micrometer.core.instrument.Metrics;
 import io.micrometer.core.instrument.Tag;
 import io.micrometer.core.instrument.Tags;
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
-
 import org.whispersystems.textsecuregcm.captcha.Action;
 import org.whispersystems.textsecuregcm.captcha.CaptchaChecker;
 import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException;
@@ -32,8 +29,7 @@ public class RateLimitChallengeManager {
   private final CaptchaChecker captchaChecker;
   private final RateLimiters rateLimiters;
 
-  private final List<RateLimitChallengeListener> rateLimitChallengeListeners =
-      Collections.synchronizedList(new ArrayList<>());
+  private final List<RateLimitChallengeListener> rateLimitChallengeListeners;
 
   private static final String RECAPTCHA_ATTEMPT_COUNTER_NAME = name(RateLimitChallengeManager.class, "recaptcha", "attempt");
   private static final String RESET_RATE_LIMIT_EXCEEDED_COUNTER_NAME = name(RateLimitChallengeManager.class, "resetRateLimitExceeded");
@@ -44,15 +40,13 @@ public class RateLimitChallengeManager {
   public RateLimitChallengeManager(
       final PushChallengeManager pushChallengeManager,
       final CaptchaChecker captchaChecker,
-      final RateLimiters rateLimiters) {
+      final RateLimiters rateLimiters,
+      final List<RateLimitChallengeListener> rateLimitChallengeListeners) {
 
     this.pushChallengeManager = pushChallengeManager;
     this.captchaChecker = captchaChecker;
     this.rateLimiters = rateLimiters;
-  }
-
-  public void addListener(final RateLimitChallengeListener rateLimitChallengeListener) {
-    rateLimitChallengeListeners.add(rateLimitChallengeListener);
+    this.rateLimitChallengeListeners = rateLimitChallengeListeners;
   }
 
   public void answerPushChallenge(final Account account, final String challenge) throws RateLimitExceededException {
diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java
index 9113523c5..0d904713b 100644
--- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java
+++ b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java
@@ -13,6 +13,7 @@ import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
 
 import java.io.IOException;
+import java.util.List;
 import java.util.Optional;
 import java.util.UUID;
 import java.util.stream.Stream;
@@ -51,9 +52,8 @@ class RateLimitChallengeManagerTest {
     rateLimitChallengeManager = new RateLimitChallengeManager(
         pushChallengeManager,
         captchaChecker,
-        rateLimiters);
-
-    rateLimitChallengeManager.addListener(rateLimitChallengeListener);
+        rateLimiters,
+        List.of(rateLimitChallengeListener));
   }
 
   @ParameterizedTest