Add metrics for report-verification-succeeded response

This commit is contained in:
Chris Eager 2022-08-15 16:26:59 -05:00 committed by Chris Eager
parent 393e15815b
commit 27f67a077c
6 changed files with 102 additions and 22 deletions

View File

@ -364,7 +364,8 @@ public class AccountController {
}
storedVerificationCode.flatMap(StoredVerificationCode::getTwilioVerificationSid)
.ifPresent(smsSender::reportVerificationSucceeded);
.ifPresent(
verificationSid -> smsSender.reportVerificationSucceeded(verificationSid, userAgent, "registration"));
Optional<Account> 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<Account> existingAccount = accounts.getByE164(number);

View File

@ -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<Optional<String>> deliverSmsVerificationWithTwilioVerify(String destination, Optional<String> clientType,
public CompletableFuture<Optional<String>> deliverSmsVerificationWithTwilioVerify(String destination,
Optional<String> clientType,
String verificationCode, List<LanguageRange> 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<Optional<String>> deliverVoxVerificationWithTwilioVerify(String destination, String verificationCode,
public CompletableFuture<Optional<String>> deliverVoxVerificationWithTwilioVerify(String destination,
String verificationCode,
List<LanguageRange> 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);
}
}

View File

@ -246,23 +246,27 @@ public class TwilioSmsSender {
}
}
public CompletableFuture<Optional<String>> deliverSmsVerificationWithVerify(String destination, Optional<String> clientType, String verificationCode, List<LanguageRange> languageRanges) {
public CompletableFuture<Optional<String>> deliverSmsVerificationWithVerify(String destination,
Optional<String> clientType, String verificationCode, List<LanguageRange> languageRanges) {
smsMeter.mark();
return twilioVerifySender.deliverSmsVerificationWithVerify(destination, clientType, verificationCode, languageRanges);
return twilioVerifySender.deliverSmsVerificationWithVerify(destination, clientType, verificationCode,
languageRanges);
}
public CompletableFuture<Optional<String>> deliverVoxVerificationWithVerify(String destination, String verificationCode, List<LanguageRange> languageRanges) {
public CompletableFuture<Optional<String>> deliverVoxVerificationWithVerify(String destination,
String verificationCode, List<LanguageRange> languageRanges) {
voxMeter.mark();
return twilioVerifySender.deliverVoxVerificationWithVerify(destination, verificationCode, languageRanges);
}
public CompletableFuture<Boolean> reportVerificationSucceeded(String verificationSid) {
public CompletableFuture<Boolean> reportVerificationSucceeded(String verificationSid, @Nullable String userAgent,
String context) {
return twilioVerifySender.reportVerificationSucceeded(verificationSid);
return twilioVerifySender.reportVerificationSucceeded(verificationSid, userAgent, context);
}
public static class TwilioResponse {

View File

@ -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<String> 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<Boolean> reportVerificationSucceeded(String verificationSid) {
public CompletableFuture<Boolean> reportVerificationSucceeded(String verificationSid, @Nullable String userAgent,
String context) {
final Map<String, String> 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 dont 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 {

View File

@ -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")));
}
}

View File

@ -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"));
}
}