From 190f2a7fc20839dbbf4f97ae346a5f03295130e9 Mon Sep 17 00:00:00 2001 From: Ameya Lokare Date: Tue, 29 Oct 2024 11:59:02 -0700 Subject: [PATCH] Pass ACI to captcha checker --- .../textsecuregcm/captcha/CaptchaChecker.java | 6 ++++- .../textsecuregcm/captcha/CaptchaClient.java | 6 +++-- .../captcha/RegistrationCaptchaManager.java | 5 ++-- .../controllers/VerificationController.java | 1 + .../limits/RateLimitChallengeManager.java | 8 +++++- .../captcha/CaptchaCheckerTest.java | 26 ++++++++++++------- .../VerificationControllerTest.java | 8 +++--- .../limits/RateLimitChallengeManagerTest.java | 2 +- 8 files changed, 41 insertions(+), 21 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java index d14b4c3a3..f111815cb 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java @@ -11,7 +11,9 @@ import com.google.common.annotations.VisibleForTesting; import io.micrometer.core.instrument.Metrics; import java.io.IOException; import java.util.Locale; +import java.util.Optional; import java.util.Set; +import java.util.UUID; import java.util.function.Function; import javax.ws.rs.BadRequestException; import org.slf4j.Logger; @@ -42,6 +44,7 @@ public class CaptchaChecker { /** * Check if a solved captcha should be accepted * + * @param maybeAci optional account UUID of the user solving the captcha * @param expectedAction the {@link Action} for which this captcha solution is intended * @param input expected to contain a prefix indicating the captcha scheme, sitekey, token, and action. The * expected format is {@code version-prefix.sitekey.action.token} @@ -53,6 +56,7 @@ public class CaptchaChecker { * @throws BadRequestException if input is not in the expected format */ public AssessmentResult verify( + final Optional maybeAci, final Action expectedAction, final String input, final String ip, @@ -100,7 +104,7 @@ public class CaptchaChecker { throw new BadRequestException("invalid captcha site-key"); } - final AssessmentResult result = client.verify(siteKey, parsedAction, token, ip, userAgent); + final AssessmentResult result = client.verify(maybeAci, siteKey, parsedAction, token, ip, userAgent); Metrics.counter(ASSESSMENTS_COUNTER_NAME, "action", action, "score", result.getScoreString(), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java index 0676011cc..c7b99f048 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java @@ -6,9 +6,9 @@ package org.whispersystems.textsecuregcm.captcha; import java.io.IOException; -import java.util.Collections; import java.util.Optional; import java.util.Set; +import java.util.UUID; public interface CaptchaClient { @@ -27,6 +27,7 @@ public interface CaptchaClient { /** * Verify a provided captcha solution * + * @param maybeAci optional account service identifier of the user * @param siteKey identifying string for the captcha service * @param action an action indicating the purpose of the captcha * @param token the captcha solution that will be verified @@ -36,6 +37,7 @@ public interface CaptchaClient { * @throws IOException if the underlying captcha provider returns an error */ AssessmentResult verify( + final Optional maybeAci, final String siteKey, final Action action, final String token, @@ -55,7 +57,7 @@ public interface CaptchaClient { } @Override - public AssessmentResult verify(final String siteKey, final Action action, final String token, final String ip, + public AssessmentResult verify(final Optional maybeAci, final String siteKey, final Action action, final String token, final String ip, final String userAgent) throws IOException { return AssessmentResult.alwaysValid(); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java index 8f943288e..33d7e12d1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java @@ -7,6 +7,7 @@ package org.whispersystems.textsecuregcm.captcha; import java.io.IOException; import java.util.Optional; +import java.util.UUID; public class RegistrationCaptchaManager { @@ -17,10 +18,10 @@ public class RegistrationCaptchaManager { } @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - public Optional assessCaptcha(final Optional captcha, final String sourceHost, final String userAgent) + public Optional assessCaptcha(final Optional aci, final Optional captcha, final String sourceHost, final String userAgent) throws IOException { return captcha.isPresent() - ? Optional.of(captchaChecker.verify(Action.REGISTRATION, captcha.get(), sourceHost, userAgent)) + ? Optional.of(captchaChecker.verify(aci, Action.REGISTRATION, captcha.get(), sourceHost, userAgent)) : Optional.empty(); } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java index 7fc4d1037..99ebd1364 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java @@ -386,6 +386,7 @@ public class VerificationController { try { assessmentResult = registrationCaptchaManager.assessCaptcha( + Optional.empty(), Optional.of(updateVerificationSessionRequest.captcha()), sourceHost, userAgent) .orElseThrow(() -> new ServerErrorException(Response.Status.INTERNAL_SERVER_ERROR)); 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 c91a447fc..f5c84d8e5 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java @@ -7,6 +7,9 @@ package org.whispersystems.textsecuregcm.limits; import static com.codahale.metrics.MetricRegistry.name; +import com.google.i18n.phonenumbers.NumberParseException; +import com.google.i18n.phonenumbers.PhoneNumberUtil; +import com.google.i18n.phonenumbers.Phonenumber; import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.Tag; import io.micrometer.core.instrument.Tags; @@ -16,12 +19,15 @@ import java.util.Optional; import org.whispersystems.textsecuregcm.captcha.Action; import org.whispersystems.textsecuregcm.captcha.CaptchaChecker; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; +import org.whispersystems.textsecuregcm.identity.IdentityType; import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.push.NotPushRegisteredException; import org.whispersystems.textsecuregcm.spam.ChallengeType; import org.whispersystems.textsecuregcm.spam.RateLimitChallengeListener; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.util.Util; +import javax.ws.rs.ServerErrorException; +import javax.ws.rs.core.Response; public class RateLimitChallengeManager { @@ -67,7 +73,7 @@ public class RateLimitChallengeManager { rateLimiters.getCaptchaChallengeAttemptLimiter().validate(account.getUuid()); - final boolean challengeSuccess = captchaChecker.verify(Action.CHALLENGE, captcha, mostRecentProxyIp, userAgent).isValid(scoreThreshold); + final boolean challengeSuccess = captchaChecker.verify(Optional.of(account.getUuid()), Action.CHALLENGE, captcha, mostRecentProxyIp, userAgent).isValid(scoreThreshold); final Tags tags = Tags.of( Tag.of(SOURCE_COUNTRY_TAG_NAME, Util.getCountryCode(account.getNumber())), diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java index 6e68a65bf..1a219f218 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java @@ -20,8 +20,13 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.UUID; import java.util.stream.Stream; import javax.ws.rs.BadRequestException; + +import com.google.i18n.phonenumbers.NumberParseException; +import com.google.i18n.phonenumbers.PhoneNumberUtil; +import com.google.i18n.phonenumbers.Phonenumber; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -36,6 +41,7 @@ public class CaptchaCheckerTest { private static final String PREFIX_A = "prefix-a"; private static final String PREFIX_B = "prefix-b"; private static final String USER_AGENT = "user-agent"; + private static final UUID ACI = UUID.randomUUID(); static Stream parseInputToken() { return Stream.of( @@ -67,7 +73,7 @@ public class CaptchaCheckerTest { when(captchaClient.scheme()).thenReturn(prefix); when(captchaClient.validSiteKeys(eq(Action.CHALLENGE))).thenReturn(Collections.singleton(CHALLENGE_SITE_KEY)); when(captchaClient.validSiteKeys(eq(Action.REGISTRATION))).thenReturn(Collections.singleton(REG_SITE_KEY)); - when(captchaClient.verify(any(), any(), any(), any(), any())).thenReturn(AssessmentResult.invalid()); + when(captchaClient.verify(any(), any(), any(), any(), any(), any())).thenReturn(AssessmentResult.invalid()); return captchaClient; } @@ -80,8 +86,8 @@ public class CaptchaCheckerTest { final String siteKey, final Action expectedAction) throws IOException { final CaptchaClient captchaClient = mockClient(PREFIX); - new CaptchaChecker(null, PREFIX -> captchaClient).verify(expectedAction, input, null, USER_AGENT); - verify(captchaClient, times(1)).verify(eq(siteKey), eq(expectedAction), eq(expectedToken), any(), eq(USER_AGENT)); + new CaptchaChecker(null, PREFIX -> captchaClient).verify(Optional.empty(), expectedAction, input, null, USER_AGENT); + verify(captchaClient, times(1)).verify(any(), eq(siteKey), eq(expectedAction), eq(expectedToken), any(), eq(USER_AGENT)); } @ParameterizedTest @@ -109,11 +115,11 @@ public class CaptchaCheckerTest { final CaptchaClient b = mockClient(PREFIX_B); final Map captchaClientMap = Map.of(PREFIX_A, a, PREFIX_B, b); - new CaptchaChecker(null, captchaClientMap::get).verify(Action.CHALLENGE, ainput, null, USER_AGENT); - verify(a, times(1)).verify(any(), any(), any(), any(), any()); + new CaptchaChecker(null, captchaClientMap::get).verify(Optional.of(ACI), Action.CHALLENGE, ainput, null, USER_AGENT); + verify(a, times(1)).verify(any(), any(), any(), any(), any(), any()); - new CaptchaChecker(null, captchaClientMap::get).verify(Action.CHALLENGE, binput, null, USER_AGENT); - verify(b, times(1)).verify(any(), any(), any(), any(), any()); + new CaptchaChecker(null, captchaClientMap::get).verify(Optional.of(ACI), Action.CHALLENGE, binput, null, USER_AGENT); + verify(b, times(1)).verify(any(), any(), any(), any(), any(), any()); } static Stream badArgs() { @@ -134,7 +140,7 @@ public class CaptchaCheckerTest { public void badArgs(final String input) throws IOException { final CaptchaClient cc = mockClient(PREFIX); assertThrows(BadRequestException.class, - () -> new CaptchaChecker(null, prefix -> PREFIX.equals(prefix) ? cc : null).verify(Action.CHALLENGE, input, null, USER_AGENT)); + () -> new CaptchaChecker(null, prefix -> PREFIX.equals(prefix) ? cc : null).verify(Optional.of(ACI), Action.CHALLENGE, input, null, USER_AGENT)); } @@ -144,8 +150,8 @@ public class CaptchaCheckerTest { final ShortCodeExpander retriever = mock(ShortCodeExpander.class); when(retriever.retrieve("abc")).thenReturn(Optional.of(TOKEN)); final String input = String.join(SEPARATOR, PREFIX + "-short", REG_SITE_KEY, "registration", "abc"); - new CaptchaChecker(retriever, ignored -> captchaClient).verify(Action.REGISTRATION, input, null, USER_AGENT); - verify(captchaClient, times(1)).verify(eq(REG_SITE_KEY), eq(Action.REGISTRATION), eq(TOKEN), any(), any()); + new CaptchaChecker(retriever, ignored -> captchaClient).verify(Optional.of(ACI), Action.REGISTRATION, input, null, USER_AGENT); + verify(captchaClient, times(1)).verify(any(), eq(REG_SITE_KEY), eq(Action.REGISTRATION), eq(TOKEN), any(), any()); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java index c0bf3c890..dcbd56c9b 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java @@ -465,7 +465,7 @@ class VerificationControllerTest { Collections.emptyList(), null, null, false, clock.millis(), clock.millis(), registrationServiceSession.expiration())))); - when(registrationCaptchaManager.assessCaptcha(any(), any(), any())) + when(registrationCaptchaManager.assessCaptcha(any(), any(), any(), any())) .thenReturn(Optional.of(AssessmentResult.invalid())); when(verificationSessionManager.update(any(), any())) @@ -637,7 +637,7 @@ class VerificationControllerTest { Collections.emptyList(), null, null, false, clock.millis(), clock.millis(), registrationServiceSession.expiration())))); - when(registrationCaptchaManager.assessCaptcha(any(), any(), any())) + when(registrationCaptchaManager.assessCaptcha(any(), any(), any(), any())) .thenReturn(Optional.of(AssessmentResult.alwaysValid())); when(verificationSessionManager.update(any(), any())) @@ -685,7 +685,7 @@ class VerificationControllerTest { Collections.emptyList(), null, null, false, clock.millis(), clock.millis(), registrationServiceSession.expiration())))); - when(registrationCaptchaManager.assessCaptcha(any(), any(), any())) + when(registrationCaptchaManager.assessCaptcha(any(), any(), any(), any())) .thenReturn(Optional.of(AssessmentResult.alwaysValid())); when(verificationSessionManager.update(any(), any())) @@ -732,7 +732,7 @@ class VerificationControllerTest { Collections.emptyList(), null, null, false, clock.millis(), clock.millis(), registrationServiceSession.expiration())))); - when(registrationCaptchaManager.assessCaptcha(any(), any(), any())) + when(registrationCaptchaManager.assessCaptcha(any(), any(), any(), any())) .thenThrow(new IOException("expected service error")); when(verificationSessionManager.update(any(), any())) 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 8c286cd97..cb2687d33 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java @@ -85,7 +85,7 @@ class RateLimitChallengeManagerTest { when(account.getNumber()).thenReturn("+18005551234"); when(account.getUuid()).thenReturn(UUID.randomUUID()); - when(captchaChecker.verify(eq(Action.CHALLENGE), any(), any(), any())) + when(captchaChecker.verify(any(), eq(Action.CHALLENGE), any(), any(), any())) .thenReturn(AssessmentResult.fromScore(actualScore, DEFAULT_SCORE_THRESHOLD)); when(rateLimiters.getCaptchaChallengeAttemptLimiter()).thenReturn(mock(RateLimiter.class));