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 e1fc9a48b..3d5ad81fb 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -364,7 +364,8 @@ public class AccountController { } storedVerificationCode.flatMap(StoredVerificationCode::getTwilioVerificationSid) - .ifPresent(smsSender::reportVerificationSucceeded); + .ifPresent( + verificationSid -> smsSender.reportVerificationSucceeded(verificationSid, userAgent, "registration")); Optional existingAccount = accounts.getByE164(number); @@ -403,7 +404,9 @@ public class AccountController { @PUT @Path("/number") @Produces(MediaType.APPLICATION_JSON) - public AccountIdentityResponse changeNumber(@Auth final AuthenticatedAccount authenticatedAccount, @NotNull @Valid final ChangePhoneNumberRequest request) + public AccountIdentityResponse changeNumber(@Auth final AuthenticatedAccount authenticatedAccount, + @NotNull @Valid final ChangePhoneNumberRequest request, + @HeaderParam("User-Agent") String userAgent) throws RateLimitExceededException, InterruptedException, ImpossiblePhoneNumberException, NonNormalizedPhoneNumberException { if (!authenticatedAccount.getAuthenticatedDevice().isMaster()) { @@ -426,7 +429,8 @@ public class AccountController { } storedVerificationCode.flatMap(StoredVerificationCode::getTwilioVerificationSid) - .ifPresent(smsSender::reportVerificationSucceeded); + .ifPresent( + verificationSid -> smsSender.reportVerificationSucceeded(verificationSid, userAgent, "changeNumber")); final Optional existingAccount = accounts.getByE164(number); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/sms/SmsSender.java b/service/src/main/java/org/whispersystems/textsecuregcm/sms/SmsSender.java index 14b80b466..f49c20636 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/sms/SmsSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/sms/SmsSender.java @@ -9,6 +9,7 @@ import java.util.List; import java.util.Locale.LanguageRange; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import javax.annotation.Nullable; @SuppressWarnings("OptionalUsedAsFieldOrParameterType") public class SmsSender { @@ -32,7 +33,8 @@ public class SmsSender { twilioSender.deliverVoxVerification(destination, verificationCode, languageRanges); } - public CompletableFuture> deliverSmsVerificationWithTwilioVerify(String destination, Optional clientType, + public CompletableFuture> deliverSmsVerificationWithTwilioVerify(String destination, + Optional clientType, String verificationCode, List languageRanges) { // Fix up mexico numbers to 'mobile' format just for SMS delivery. if (destination.startsWith("+52") && !destination.startsWith("+521")) { @@ -42,13 +44,14 @@ public class SmsSender { return twilioSender.deliverSmsVerificationWithVerify(destination, clientType, verificationCode, languageRanges); } - public CompletableFuture> deliverVoxVerificationWithTwilioVerify(String destination, String verificationCode, + public CompletableFuture> deliverVoxVerificationWithTwilioVerify(String destination, + String verificationCode, List languageRanges) { - return twilioSender.deliverVoxVerificationWithVerify(destination, verificationCode, languageRanges); + return twilioSender.deliverVoxVerificationWithVerify(destination, verificationCode, languageRanges); } - public void reportVerificationSucceeded(String verificationSid) { - twilioSender.reportVerificationSucceeded(verificationSid); + public void reportVerificationSucceeded(String verificationSid, @Nullable String userAgent, String context) { + twilioSender.reportVerificationSucceeded(verificationSid, userAgent, context); } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java b/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java index 5e79713d2..6c1fa3427 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java @@ -246,23 +246,27 @@ public class TwilioSmsSender { } } - public CompletableFuture> deliverSmsVerificationWithVerify(String destination, Optional clientType, String verificationCode, List languageRanges) { + public CompletableFuture> deliverSmsVerificationWithVerify(String destination, + Optional clientType, String verificationCode, List languageRanges) { smsMeter.mark(); - return twilioVerifySender.deliverSmsVerificationWithVerify(destination, clientType, verificationCode, languageRanges); + return twilioVerifySender.deliverSmsVerificationWithVerify(destination, clientType, verificationCode, + languageRanges); } - public CompletableFuture> deliverVoxVerificationWithVerify(String destination, String verificationCode, List languageRanges) { + public CompletableFuture> deliverVoxVerificationWithVerify(String destination, + String verificationCode, List languageRanges) { voxMeter.mark(); return twilioVerifySender.deliverVoxVerificationWithVerify(destination, verificationCode, languageRanges); } - public CompletableFuture reportVerificationSucceeded(String verificationSid) { + public CompletableFuture reportVerificationSucceeded(String verificationSid, @Nullable String userAgent, + String context) { - return twilioVerifySender.reportVerificationSucceeded(verificationSid); + return twilioVerifySender.reportVerificationSucceeded(verificationSid, userAgent, context); } public static class TwilioResponse { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioVerifySender.java b/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioVerifySender.java index 1d71f94b2..a57ceb4dd 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioVerifySender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioVerifySender.java @@ -1,7 +1,12 @@ package org.whispersystems.textsecuregcm.sms; +import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; + import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; +import io.micrometer.core.instrument.Metrics; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; import java.io.IOException; import java.net.URI; import java.net.http.HttpRequest; @@ -14,13 +19,15 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.function.BiFunction; +import javax.annotation.Nullable; import javax.validation.constraints.NotEmpty; -import io.micrometer.core.instrument.Metrics; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.configuration.TwilioConfiguration; import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient; import org.whispersystems.textsecuregcm.http.FormDataBodyPublisher; +import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.Util; @@ -29,6 +36,13 @@ class TwilioVerifySender { private static final Logger logger = LoggerFactory.getLogger(TwilioVerifySender.class); + private static final String VERIFICATION_SUCCEEDED_RESPONSE_COUNTER_NAME = name(TwilioVerifySender.class, + "verificationSucceeded"); + + private static final String CONTEXT_TAG_NAME = "context"; + private static final String STATUS_CODE_TAG_NAME = "statusCode"; + private static final String ERROR_CODE_TAG_NAME = "errorCode"; + static final Set TWILIO_VERIFY_LANGUAGES = Set.of( "af", "ar", @@ -170,11 +184,13 @@ class TwilioVerifySender { .uri(verifyServiceUri) .POST(FormDataBodyPublisher.of(requestParameters)) .header("Content-Type", "application/x-www-form-urlencoded") - .header("Authorization", "Basic " + Base64.getEncoder().encodeToString((accountId + ":" + accountToken).getBytes())) + .header("Authorization", + "Basic " + Base64.getEncoder().encodeToString((accountId + ":" + accountToken).getBytes())) .build(); } - public CompletableFuture reportVerificationSucceeded(String verificationSid) { + public CompletableFuture reportVerificationSucceeded(String verificationSid, @Nullable String userAgent, + String context) { final Map requestParameters = new HashMap<>(); requestParameters.put("Status", "approved"); @@ -183,14 +199,47 @@ class TwilioVerifySender { .uri(verifyApprovalBaseUri.resolve(verificationSid)) .POST(FormDataBodyPublisher.of(requestParameters)) .header("Content-Type", "application/x-www-form-urlencoded") - .header("Authorization", "Basic " + Base64.getEncoder().encodeToString((accountId + ":" + accountToken).getBytes())) + .header("Authorization", + "Basic " + Base64.getEncoder().encodeToString((accountId + ":" + accountToken).getBytes())) .build(); return httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofString()) .thenApply(this::parseResponse) - .handle((response, throwable) -> throwable == null - && response.isSuccess() - && "approved".equals(response.successResponse.getStatus())); + .handle((response, throwable) -> processVerificationSucceededResponse(response, throwable, userAgent, context)); + } + + private boolean processVerificationSucceededResponse(@Nullable final TwilioVerifyResponse response, + @Nullable final Throwable throwable, + final String userAgent, + final String context) { + + if (throwable == null) { + + assert response != null; + + final Tags tags = Tags.of(Tag.of(CONTEXT_TAG_NAME, context), UserAgentTagUtil.getPlatformTag(userAgent)); + + if (response.isSuccess() && "approved".equals(response.successResponse.getStatus())) { + // the other possible values of `status` are `pending` or `canceled`, but these can never happen in a response + // to this POST, so we don‘t consider them + Metrics.counter(VERIFICATION_SUCCEEDED_RESPONSE_COUNTER_NAME, tags) + .increment(); + + return true; + } + + // at this point, response.isFailure() == true + Metrics.counter( + VERIFICATION_SUCCEEDED_RESPONSE_COUNTER_NAME, + Tags.of(ERROR_CODE_TAG_NAME, String.valueOf(response.failureResponse.code), + STATUS_CODE_TAG_NAME, String.valueOf(response.failureResponse.status)) + .and(tags)) + .increment(); + } else { + logger.warn("Failed to send verification succeeded", throwable); + } + + return false; } public static class TwilioVerifyResponse { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/sms/TwilioVerifySenderTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/sms/TwilioVerifySenderTest.java index e3be6b8c2..caa6f6e5a 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/sms/TwilioVerifySenderTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/sms/TwilioVerifySenderTest.java @@ -216,7 +216,7 @@ class TwilioVerifySenderTest { .withHeader("Content-Type", "application/json") .withBody("{\"status\": \"approved\", \"sid\": \"" + VERIFICATION_SID + "\"}"))); - final Boolean success = sender.reportVerificationSucceeded(VERIFICATION_SID).get(); + final Boolean success = sender.reportVerificationSucceeded(VERIFICATION_SID, null, "test").get(); assertThat(success).isTrue(); @@ -225,4 +225,24 @@ class TwilioVerifySenderTest { .withHeader("Content-Type", equalTo("application/x-www-form-urlencoded")) .withRequestBody(equalTo("Status=approved"))); } + + @Test + void reportVerificationFailed() throws Exception { + + wireMock.stubFor(post(urlEqualTo("/v2/Services/" + VERIFY_SERVICE_SID + "/Verifications/" + VERIFICATION_SID)) + .withBasicAuth(ACCOUNT_ID, ACCOUNT_TOKEN) + .willReturn(aResponse() + .withStatus(404) + .withHeader("Content-Type", "application/json") + .withBody("{\"status\": 404, \"code\": 20404}"))); + + final Boolean success = sender.reportVerificationSucceeded(VERIFICATION_SID, null, "test").get(); + + assertThat(success).isFalse(); + + wireMock.verify(1, + postRequestedFor(urlEqualTo("/v2/Services/" + VERIFY_SERVICE_SID + "/Verifications/" + VERIFICATION_SID)) + .withHeader("Content-Type", equalTo("application/x-www-form-urlencoded")) + .withRequestBody(equalTo("Status=approved"))); + } } 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 66f68b72c..af335a7b1 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 @@ -1068,7 +1068,7 @@ class AccountControllerTest { verify(accountsManager).create(eq(SENDER), eq("bar"), any(), any(), anyList()); if (enrolledInVerifyExperiment) { - verify(smsSender).reportVerificationSucceeded("VerificationSid"); + verify(smsSender).reportVerificationSucceeded(eq("VerificationSid"), any(), eq("registration")); } }