From c14621a09fab35b918a6e64f61d63331211ed0b9 Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Wed, 14 Sep 2022 00:08:38 -0500 Subject: [PATCH] Add metrics for captcha scores --- .../controllers/AccountController.java | 109 ++++++++---------- .../limits/RateLimitChallengeManager.java | 2 +- .../recaptcha/RecaptchaClient.java | 25 +++- .../limits/RateLimitChallengeManagerTest.java | 5 +- .../controllers/AccountControllerTest.java | 6 +- 5 files changed, 78 insertions(+), 69 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index 1b23bbb9b..6560ccf69 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -125,6 +125,7 @@ public class AccountController { private static final String CHALLENGE_ISSUED_COUNTER_NAME = name(AccountController.class, "challengeIssued"); private static final String TWILIO_VERIFY_ERROR_COUNTER_NAME = name(AccountController.class, "twilioVerifyError"); + private static final String TWILIO_VERIFY_UNDELIVERED_COUNTER_NAME = name(AccountController.class, "twilioUndelivered"); private static final String INVALID_ACCEPT_LANGUAGE_COUNTER_NAME = name(AccountController.class, "invalidAcceptLanguage"); private static final String NONSTANDARD_USERNAME_COUNTER_NAME = name(AccountController.class, "nonStandardUsername"); @@ -134,6 +135,7 @@ public class AccountController { private static final String COUNTRY_CODE_TAG_NAME = "countryCode"; private static final String REGION_TAG_NAME = "region"; private static final String VERIFICATION_TRANSPORT_TAG_NAME = "transport"; + private static final String SCORE_TAG_NAME = "score"; private static final String VERIFY_EXPERIMENT_TAG_NAME = "twilioVerify"; @@ -231,23 +233,34 @@ public class AccountController { String sourceHost = ForwardedIpUtil.getMostRecentProxy(forwardedFor).orElseThrow(); Optional storedChallenge = pendingAccounts.getCodeForNumber(number); - CaptchaRequirement requirement = requiresCaptcha(number, transport, forwardedFor, sourceHost, captcha, - storedChallenge, pushChallenge, userAgent); - if (requirement.isCaptchaRequired()) { + final String countryCode = Util.getCountryCode(number); + final String region = Util.getRegion(number); + + // if there's a captcha, assess it, otherwise check if we need a captcha + final Optional assessmentResult = captcha + .map(captchaToken -> recaptchaClient.verify(captchaToken, sourceHost)); + + assessmentResult.ifPresent(result -> + Metrics.counter(CAPTCHA_ATTEMPT_COUNTER_NAME, Tags.of( + Tag.of("success", String.valueOf(result.valid())), + UserAgentTagUtil.getPlatformTag(userAgent), + Tag.of(COUNTRY_CODE_TAG_NAME, countryCode), + Tag.of(REGION_TAG_NAME, region), + Tag.of(SCORE_TAG_NAME, result.score()))) + .increment()); + + final boolean requiresCaptcha = assessmentResult + .map(result -> !result.valid()) + .orElseGet(() -> requiresCaptcha(number, transport, forwardedFor, sourceHost, storedChallenge, pushChallenge)); + + if (requiresCaptcha) { captchaRequiredMeter.mark(); - Metrics.counter(CHALLENGE_ISSUED_COUNTER_NAME, Tags.of( UserAgentTagUtil.getPlatformTag(userAgent), Tag.of(COUNTRY_CODE_TAG_NAME, Util.getCountryCode(number)), Tag.of(REGION_TAG_NAME, Util.getRegion(number)))) .increment(); - - if (requirement.isAutoBlock() && shouldAutoBlock(sourceHost)) { - logger.info("Auto-block: {}", sourceHost); - abusiveHostRules.setBlockedHost(sourceHost); - } - return Response.status(402).build(); } @@ -315,6 +328,14 @@ public class AccountController { logger.warn("Error with Twilio Verify", throwable); return; } + if (enrolledInVerifyExperiment && maybeVerificationSid.isEmpty() && assessmentResult.isPresent()) { + Metrics.counter(TWILIO_VERIFY_UNDELIVERED_COUNTER_NAME, Tags.of( + Tag.of(COUNTRY_CODE_TAG_NAME, countryCode), + Tag.of(REGION_TAG_NAME, region), + UserAgentTagUtil.getPlatformTag(userAgent), + Tag.of(SCORE_TAG_NAME, assessmentResult.get().score()))) + .increment(); + } maybeVerificationSid.ifPresent(twilioVerificationSid -> { StoredVerificationCode storedVerificationCodeWithVerificationSid = new StoredVerificationCode( storedVerificationCode.getCode(), @@ -798,37 +819,17 @@ public class AccountController { } } - private CaptchaRequirement requiresCaptcha(String number, String transport, String forwardedFor, + private boolean requiresCaptcha(String number, String transport, String forwardedFor, String sourceHost, - Optional captchaToken, Optional storedVerificationCode, - Optional pushChallenge, - String userAgent) - { + Optional pushChallenge) { if (testDevices.containsKey(number)) { - return new CaptchaRequirement(false, false); + return false; } final String countryCode = Util.getCountryCode(number); final String region = Util.getRegion(number); - if (captchaToken.isPresent()) { - boolean validToken = recaptchaClient.verify(captchaToken.get(), sourceHost); - - Metrics.counter(CAPTCHA_ATTEMPT_COUNTER_NAME, Tags.of( - Tag.of("success", String.valueOf(validToken)), - UserAgentTagUtil.getPlatformTag(userAgent), - Tag.of(COUNTRY_CODE_TAG_NAME, countryCode), - Tag.of(REGION_TAG_NAME, region))) - .increment(); - - if (validToken) { - return new CaptchaRequirement(false, false); - } else { - return new CaptchaRequirement(true, false); - } - } - { final List tags = new ArrayList<>(); tags.add(Tag.of(COUNTRY_CODE_TAG_NAME, countryCode)); @@ -842,14 +843,13 @@ public class AccountController { if (!pushChallenge.get().equals(storedPushChallenge.orElse(null))) { tags.add(Tag.of(CHALLENGE_MATCH_TAG_NAME, "false")); - return new CaptchaRequirement(true, false); + return true; } else { tags.add(Tag.of(CHALLENGE_MATCH_TAG_NAME, "true")); } } else { tags.add(Tag.of(CHALLENGE_PRESENT_TAG_NAME, "false")); - - return new CaptchaRequirement(true, false); + return true; } } finally { Metrics.counter(PUSH_CHALLENGE_COUNTER_NAME, tags).increment(); @@ -870,7 +870,7 @@ public class AccountController { // would be caught by country filter as well countryFilterApplicable.mark(); } - return new CaptchaRequirement(true, false); + return true; } try { @@ -878,7 +878,11 @@ public class AccountController { } catch (RateLimitExceededException e) { logger.info("Rate limit exceeded: {}, {}, {} ({})", transport, number, sourceHost, forwardedFor); rateLimitedHostMeter.mark(); - return new CaptchaRequirement(true, true); + if (shouldAutoBlock(sourceHost)) { + logger.info("Auto-block: {}", sourceHost); + abusiveHostRules.setBlockedHost(sourceHost); + } + return true; } try { @@ -886,15 +890,18 @@ public class AccountController { } catch (RateLimitExceededException e) { logger.info("Prefix rate limit exceeded: {}, {}, {} ({})", transport, number, sourceHost, forwardedFor); rateLimitedPrefixMeter.mark(); - return new CaptchaRequirement(true, true); + if (shouldAutoBlock(sourceHost)) { + logger.info("Auto-block: {}", sourceHost); + abusiveHostRules.setBlockedHost(sourceHost); + } + return true; } if (countryFiltered) { countryFilteredHostMeter.mark(); - return new CaptchaRequirement(true, false); + return true; } - - return new CaptchaRequirement(false, false); + return false; } @Timed @@ -941,22 +948,4 @@ public class AccountController { return Hex.toStringCondensed(challenge); } - - private static class CaptchaRequirement { - private final boolean captchaRequired; - private final boolean autoBlock; - - private CaptchaRequirement(boolean captchaRequired, boolean autoBlock) { - this.captchaRequired = captchaRequired; - this.autoBlock = autoBlock; - } - - boolean isCaptchaRequired() { - return captchaRequired; - } - - boolean isAutoBlock() { - return autoBlock; - } - } } 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 e288e3ed1..446b9d8b4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java @@ -62,7 +62,7 @@ public class RateLimitChallengeManager { rateLimiters.getRecaptchaChallengeAttemptLimiter().validate(account.getUuid()); - final boolean challengeSuccess = recaptchaClient.verify(captcha, mostRecentProxyIp); + final boolean challengeSuccess = recaptchaClient.verify(captcha, mostRecentProxyIp).valid(); final Tags tags = Tags.of( Tag.of(SOURCE_COUNTRY_TAG_NAME, Util.getCountryCode(account.getNumber())), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/recaptcha/RecaptchaClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/recaptcha/RecaptchaClient.java index 3440267a7..711eb32d7 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/recaptcha/RecaptchaClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/recaptcha/RecaptchaClient.java @@ -77,7 +77,20 @@ public class RecaptchaClient { return parts; } - public boolean verify(final String input, final String ip) { + /** + * A captcha assessment + * + * @param valid whether the captcha was passed + * @param score string representation of the risk level + */ + public record AssessmentResult(boolean valid, String score) { + public static AssessmentResult invalid() { + return new AssessmentResult(false, ""); + } + } + + + public AssessmentResult verify(final String input, final String ip) { final String[] parts = parseInputToken(input); final String sitekey = parts[0]; @@ -102,11 +115,13 @@ public class RecaptchaClient { .increment(); if (assessment.getTokenProperties().getValid()) { - return assessment.getRiskAnalysis().getScore() >= - dynamicConfigurationManager.getConfiguration().getCaptchaConfiguration().getScoreFloor().floatValue(); - + final float score = assessment.getRiskAnalysis().getScore(); + return new AssessmentResult( + score >= + dynamicConfigurationManager.getConfiguration().getCaptchaConfiguration().getScoreFloor().floatValue(), + Integer.toString((int) score)); } else { - return false; + return AssessmentResult.invalid(); } } } 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 3e86aae85..bb3aaa9e2 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java @@ -68,7 +68,10 @@ class RateLimitChallengeManagerTest { when(account.getNumber()).thenReturn("+18005551234"); when(account.getUuid()).thenReturn(UUID.randomUUID()); - when(recaptchaClient.verify(any(), any())).thenReturn(successfulChallenge); + when(recaptchaClient.verify(any(), any())) + .thenReturn(successfulChallenge + ? new RecaptchaClient.AssessmentResult(true, "") + : RecaptchaClient.AssessmentResult.invalid()); when(rateLimiters.getRecaptchaChallengeAttemptLimiter()).thenReturn(mock(RateLimiter.class)); when(rateLimiters.getRecaptchaChallengeSuccessLimiter()).thenReturn(mock(RateLimiter.class)); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java index aaa2d3a73..79f74b554 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java @@ -300,8 +300,10 @@ class AccountControllerTest { when(abusiveHostRules.isBlocked(eq(ABUSIVE_HOST))).thenReturn(true); when(abusiveHostRules.isBlocked(eq(NICE_HOST))).thenReturn(false); - when(recaptchaClient.verify(eq(INVALID_CAPTCHA_TOKEN), anyString())).thenReturn(false); - when(recaptchaClient.verify(eq(VALID_CAPTCHA_TOKEN), anyString())).thenReturn(true); + when(recaptchaClient.verify(eq(INVALID_CAPTCHA_TOKEN), anyString())) + .thenReturn(RecaptchaClient.AssessmentResult.invalid()); + when(recaptchaClient.verify(eq(VALID_CAPTCHA_TOKEN), anyString())) + .thenReturn(new RecaptchaClient.AssessmentResult(true, "")); doThrow(new RateLimitExceededException(Duration.ZERO)).when(pinLimiter).validate(eq(SENDER_OVER_PIN));