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 974a919fe..d7314566c 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaChecker.java @@ -50,6 +50,7 @@ public class CaptchaChecker { * @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} * @param ip IP of the solver + * @param userAgent User-Agent of the solver * @return An {@link AssessmentResult} indicating whether the solution should be accepted, and a score that can be * used for metrics * @throws IOException if there is an error validating the captcha with the underlying service @@ -58,7 +59,8 @@ public class CaptchaChecker { public AssessmentResult verify( final Action expectedAction, final String input, - final String ip) throws IOException { + final String ip, + final String userAgent) throws IOException { final String[] parts = input.split("\\" + SEPARATOR, 4); // we allow missing actions, if we're missing 1 part, assume it's the action @@ -102,7 +104,7 @@ public class CaptchaChecker { throw new BadRequestException("invalid captcha site-key"); } - final AssessmentResult result = client.verify(siteKey, parsedAction, token, ip); + final AssessmentResult result = client.verify(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 f22a52ab1..bc2da6e3c 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/CaptchaClient.java @@ -26,10 +26,11 @@ public interface CaptchaClient { /** * Verify a provided captcha solution * - * @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 - * @param ip the ip of the captcha solver + * @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 + * @param ip the ip of the captcha solver + * @param userAgent the User-Agent string of the captcha solver * @return An {@link AssessmentResult} indicating whether the solution should be accepted * @throws IOException if the underlying captcha provider returns an error */ @@ -37,5 +38,6 @@ public interface CaptchaClient { final String siteKey, final Action action, final String token, - final String ip) throws IOException; + final String ip, + final String userAgent) throws IOException; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java index b2582370c..6c40977d9 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java @@ -27,6 +27,8 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import javax.ws.rs.core.Response; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration; @@ -34,6 +36,7 @@ import org.whispersystems.textsecuregcm.configuration.RetryConfiguration; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient; +import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; import org.whispersystems.textsecuregcm.util.ExceptionUtils; import org.whispersystems.textsecuregcm.util.SystemMapper; @@ -100,7 +103,8 @@ public class HCaptchaClient implements CaptchaClient { final String siteKey, final Action action, final String token, - final String ip) + final String ip, + final String userAgent) throws IOException { final DynamicCaptchaConfiguration config = dynamicConfigurationManager.getConfiguration().getCaptchaConfiguration(); @@ -134,9 +138,11 @@ public class HCaptchaClient implements CaptchaClient { if (!hCaptchaResponse.success) { for (String errorCode : hCaptchaResponse.errorCodes) { - Metrics.counter(INVALID_REASON_COUNTER_NAME, - "action", action.getActionName(), - "reason", errorCode).increment(); + Metrics.counter(INVALID_REASON_COUNTER_NAME, Tags.of( + Tag.of("action", action.getActionName()), + Tag.of("reason", errorCode), + UserAgentTagUtil.getPlatformTag(userAgent) + )).increment(); } return AssessmentResult.invalid(); } 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 fa3b02522..8f943288e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/RegistrationCaptchaManager.java @@ -17,10 +17,10 @@ public class RegistrationCaptchaManager { } @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - public Optional assessCaptcha(final Optional captcha, final String sourceHost) + public Optional assessCaptcha(final Optional captcha, final String sourceHost, final String userAgent) throws IOException { return captcha.isPresent() - ? Optional.of(captchaChecker.verify(Action.REGISTRATION, captcha.get(), sourceHost)) + ? Optional.of(captchaChecker.verify(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 efbf1f6c8..7fb61b380 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java @@ -386,7 +386,7 @@ public class VerificationController { try { assessmentResult = registrationCaptchaManager.assessCaptcha( - Optional.of(updateVerificationSessionRequest.captcha()), sourceHost) + Optional.of(updateVerificationSessionRequest.captcha()), sourceHost, userAgent) .orElseThrow(() -> new ServerErrorException(Response.Status.INTERNAL_SERVER_ERROR)); Metrics.counter(CAPTCHA_ATTEMPT_COUNTER_NAME, Tags.of( 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 7bf5f7fe7..c91a447fc 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java @@ -67,7 +67,7 @@ public class RateLimitChallengeManager { rateLimiters.getCaptchaChallengeAttemptLimiter().validate(account.getUuid()); - final boolean challengeSuccess = captchaChecker.verify(Action.CHALLENGE, captcha, mostRecentProxyIp).isValid(scoreThreshold); + final boolean challengeSuccess = captchaChecker.verify(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 c61d9d866..2b8bcb02c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/CaptchaCheckerTest.java @@ -34,6 +34,7 @@ public class CaptchaCheckerTest { private static final String PREFIX = "prefix"; private static final String PREFIX_A = "prefix-a"; private static final String PREFIX_B = "prefix-b"; + private static final String USER_AGENT = "user-agent"; static Stream parseInputToken() { return Stream.of( @@ -65,7 +66,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())).thenReturn(AssessmentResult.invalid()); + when(captchaClient.verify(any(), any(), any(), any(), any())).thenReturn(AssessmentResult.invalid()); return captchaClient; } @@ -78,8 +79,8 @@ public class CaptchaCheckerTest { final String siteKey, final Action expectedAction) throws IOException { final CaptchaClient captchaClient = mockClient(PREFIX); - new CaptchaChecker(null, List.of(captchaClient)).verify(expectedAction, input, null); - verify(captchaClient, times(1)).verify(eq(siteKey), eq(expectedAction), eq(expectedToken), any()); + new CaptchaChecker(null, List.of(captchaClient)).verify(expectedAction, input, null, USER_AGENT); + verify(captchaClient, times(1)).verify(eq(siteKey), eq(expectedAction), eq(expectedToken), any(), eq(USER_AGENT)); } @ParameterizedTest @@ -106,11 +107,11 @@ public class CaptchaCheckerTest { final CaptchaClient a = mockClient(PREFIX_A); final CaptchaClient b = mockClient(PREFIX_B); - new CaptchaChecker(null, List.of(a, b)).verify(Action.CHALLENGE, ainput, null); - verify(a, times(1)).verify(any(), any(), any(), any()); + new CaptchaChecker(null, List.of(a, b)).verify(Action.CHALLENGE, ainput, null, USER_AGENT); + verify(a, times(1)).verify(any(), any(), any(), any(), any()); - new CaptchaChecker(null, List.of(a, b)).verify(Action.CHALLENGE, binput, null); - verify(b, times(1)).verify(any(), any(), any(), any()); + new CaptchaChecker(null, List.of(a, b)).verify(Action.CHALLENGE, binput, null, USER_AGENT); + verify(b, times(1)).verify(any(), any(), any(), any(), any()); } static Stream badArgs() { @@ -131,7 +132,7 @@ public class CaptchaCheckerTest { public void badArgs(final String input) throws IOException { final CaptchaClient cc = mockClient(PREFIX); assertThrows(BadRequestException.class, - () -> new CaptchaChecker(null, List.of(cc)).verify(Action.CHALLENGE, input, null)); + () -> new CaptchaChecker(null, List.of(cc)).verify(Action.CHALLENGE, input, null, USER_AGENT)); } @@ -141,8 +142,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, List.of(captchaClient)).verify(Action.REGISTRATION, input, null); - verify(captchaClient, times(1)).verify(eq(REG_SITE_KEY), eq(Action.REGISTRATION), eq(TOKEN), any()); + new CaptchaChecker(retriever, List.of(captchaClient)).verify(Action.REGISTRATION, input, null, USER_AGENT); + verify(captchaClient, times(1)).verify(eq(REG_SITE_KEY), eq(Action.REGISTRATION), eq(TOKEN), any(), any()); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java index 6f6d98597..06dcef0ba 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java @@ -30,6 +30,7 @@ public class HCaptchaClientTest { private static final String SITE_KEY = "site-key"; private static final String TOKEN = "token"; + private static final String USER_AGENT = "user-agent"; static Stream captchaProcessed() { @@ -56,7 +57,7 @@ public class HCaptchaClientTest { success, 1 - score)); // hCaptcha scores are inverted compared to recaptcha scores. (low score is good) final AssessmentResult result = new HCaptchaClient("fake", client, mockConfig(true, 0.5)) - .verify(SITE_KEY, Action.CHALLENGE, TOKEN, null); + .verify(SITE_KEY, Action.CHALLENGE, TOKEN, null, USER_AGENT); if (!success) { assertThat(result).isEqualTo(AssessmentResult.invalid()); } else { @@ -68,7 +69,7 @@ public class HCaptchaClientTest { public void errorResponse() throws IOException, InterruptedException { final FaultTolerantHttpClient httpClient = mockResponder(503, ""); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); - assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null)); + assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null, USER_AGENT)); } @Test @@ -77,7 +78,7 @@ public class HCaptchaClientTest { {"success" : true, "score": 1.1} """); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); - assertThat(client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null)).isEqualTo(AssessmentResult.invalid()); + assertThat(client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null, USER_AGENT)).isEqualTo(AssessmentResult.invalid()); } @Test @@ -86,7 +87,7 @@ public class HCaptchaClientTest { {"success" : true, """); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); - assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null)); + assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null, USER_AGENT)); } @Test diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/configuration/StubHCaptchaClientFactory.java b/service/src/test/java/org/whispersystems/textsecuregcm/configuration/StubHCaptchaClientFactory.java index 21025ee11..599fa6d2e 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/configuration/StubHCaptchaClientFactory.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/configuration/StubHCaptchaClientFactory.java @@ -49,8 +49,9 @@ public class StubHCaptchaClientFactory implements HCaptchaClientFactory { } @Override - public AssessmentResult verify(final String siteKey, final Action action, final String token, final String ip) - throws IOException { + public AssessmentResult verify(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/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java index b70f28e91..c0bf3c890 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())) + when(registrationCaptchaManager.assessCaptcha(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())) + when(registrationCaptchaManager.assessCaptcha(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())) + when(registrationCaptchaManager.assessCaptcha(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())) + when(registrationCaptchaManager.assessCaptcha(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 bd5a0e0b7..8c286cd97 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())) + when(captchaChecker.verify(eq(Action.CHALLENGE), any(), any(), any())) .thenReturn(AssessmentResult.fromScore(actualScore, DEFAULT_SCORE_THRESHOLD)); when(rateLimiters.getCaptchaChallengeAttemptLimiter()).thenReturn(mock(RateLimiter.class));