From 0bc1369e04fa807363f43df7708e79abca837d05 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Wed, 10 Mar 2021 18:13:39 -0500 Subject: [PATCH] Work through the full list of supported locales when choosing a language for voice verification. --- .../controllers/AccountController.java | 13 ++++++--- .../VoiceVerificationController.java | 15 +++++++---- .../textsecuregcm/sms/SmsSender.java | 6 +++-- .../textsecuregcm/sms/TwilioSmsSender.java | 26 +++++++++++------- .../controllers/AccountControllerTest.java | 23 +++++++++++++--- .../VoiceVerificationControllerTest.java | 14 ++++++++++ .../tests/sms/TwilioSmsSenderTest.java | 27 ++++++++++++++++--- 7 files changed, 97 insertions(+), 27 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 73e6f6ebc..967cf471f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -15,6 +15,7 @@ import io.dropwizard.auth.Auth; import java.security.SecureRandom; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Map; @@ -248,11 +249,15 @@ public class AccountController { } else if (transport.equals("sms")) { smsSender.deliverSmsVerification(number, client, verificationCode.getVerificationCodeDisplay()); } else if (transport.equals("voice")) { - final Optional maybeLocale = acceptLanguage.map(Locale.LanguageRange::parse) - .flatMap(ranges -> ranges.stream().findFirst()) - .map(range -> Locale.forLanguageTag(range.getRange())); + final List languageRanges; - smsSender.deliverVoxVerification(number, verificationCode.getVerificationCode(), maybeLocale); + try { + languageRanges = acceptLanguage.map(Locale.LanguageRange::parse).orElse(Collections.emptyList()); + } catch (final IllegalArgumentException e) { + return Response.status(400).build(); + } + + smsSender.deliverVoxVerification(number, verificationCode.getVerificationCode(), languageRanges); } metricRegistry.meter(name(AccountController.class, "create", Util.getCountryCode(number))).mark(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VoiceVerificationController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VoiceVerificationController.java index 81f1b82c4..92d3fa735 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VoiceVerificationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VoiceVerificationController.java @@ -13,6 +13,7 @@ import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import java.util.List; import java.util.Optional; import java.util.Set; @@ -59,19 +60,23 @@ public class VoiceVerificationController { @POST @Path("/description/{code}") @Produces(MediaType.APPLICATION_XML) - public Response getDescription(@PathParam("code") String code, @QueryParam("l") String locale) { + public Response getDescription(@PathParam("code") String code, @QueryParam("l") List locales) { code = code.replaceAll("[^0-9]", ""); if (code.length() != 6) { return Response.status(400).build(); } - if (locale != null && supportedLocales.contains(locale)) { - return getLocalizedDescription(code, locale); + for (final String locale : locales) { + if (locale != null && supportedLocales.contains(locale)) { + return getLocalizedDescription(code, locale); + } } - if (locale != null && locale.split("-").length >= 1 && supportedLocales.contains(locale.split("-")[0])) { - return getLocalizedDescription(code, locale.split("-")[0]); + for (final String locale : locales) { + if (locale != null && locale.split("-").length >= 1 && supportedLocales.contains(locale.split("-")[0])) { + return getLocalizedDescription(code, locale.split("-")[0]); + } } return getLocalizedDescription(code, "en-US"); 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 43dd9803c..d87e99e93 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/sms/SmsSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/sms/SmsSender.java @@ -5,7 +5,9 @@ package org.whispersystems.textsecuregcm.sms; +import java.util.List; import java.util.Locale; +import java.util.Locale.LanguageRange; import java.util.Optional; @SuppressWarnings("OptionalUsedAsFieldOrParameterType") @@ -25,7 +27,7 @@ public class SmsSender { twilioSender.deliverSmsVerification(destination, clientType, verificationCode); } - public void deliverVoxVerification(String destination, String verificationCode, Optional maybeLocale) { - twilioSender.deliverVoxVerification(destination, verificationCode, maybeLocale); + public void deliverVoxVerification(String destination, String verificationCode, List languageRanges) { + twilioSender.deliverVoxVerification(destination, verificationCode, languageRanges); } } 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 d61f5a2cd..accf93511 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java @@ -23,11 +23,13 @@ import java.time.Duration; import java.util.HashMap; import java.util.List; import java.util.Locale; +import java.util.Locale.LanguageRange; import java.util.Map; import java.util.Optional; import java.util.Random; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; +import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.apache.commons.lang3.StringUtils; @@ -144,19 +146,23 @@ public class TwilioSmsSender { } } - public CompletableFuture deliverVoxVerification(String destination, String verificationCode, Optional maybeLocale) { + public CompletableFuture deliverVoxVerification(String destination, String verificationCode, List languageRanges) { String url = "https://" + localDomain + "/v1/voice/description/" + verificationCode; - if (maybeLocale.isPresent()) { - final String localeString = maybeLocale.map(locale -> { - if (StringUtils.isNotBlank(locale.getCountry())) { - return locale.getLanguage().toLowerCase() + "-" + locale.getCountry().toUpperCase(); - } else { - return locale.getLanguage().toLowerCase(); - } - }).get(); + final String languageQueryParams = languageRanges.stream() + .map(range -> Locale.forLanguageTag(range.getRange())) + .map(locale -> { + if (StringUtils.isNotBlank(locale.getCountry())) { + return locale.getLanguage().toLowerCase() + "-" + locale.getCountry().toUpperCase(); + } else { + return locale.getLanguage().toLowerCase(); + } + }) + .map(languageTag -> "l=" + languageTag) + .collect(Collectors.joining("&")); - url += "?l=" + localeString; + if (StringUtils.isNotBlank(languageQueryParams)) { + url += "?" + languageQueryParams; } Map requestParameters = new HashMap<>(); 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 ae2ff82e5..91dbaffb7 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 @@ -308,7 +308,7 @@ public class AccountControllerTest { assertThat(response.getStatus()).isEqualTo(200); - verify(smsSender).deliverVoxVerification(eq(SENDER), anyString(), eq(Optional.empty())); + verify(smsSender).deliverVoxVerification(eq(SENDER), anyString(), eq(Collections.emptyList())); verify(abusiveHostRules).getAbusiveHostRulesFor(eq(NICE_HOST)); } @@ -325,7 +325,7 @@ public class AccountControllerTest { assertThat(response.getStatus()).isEqualTo(200); - verify(smsSender).deliverVoxVerification(eq(SENDER), anyString(), eq(Optional.of(Locale.forLanguageTag("pt-BR")))); + verify(smsSender).deliverVoxVerification(eq(SENDER), anyString(), eq(Locale.LanguageRange.parse("pt-BR"))); verify(abusiveHostRules).getAbusiveHostRulesFor(eq(NICE_HOST)); } @@ -342,7 +342,24 @@ public class AccountControllerTest { assertThat(response.getStatus()).isEqualTo(200); - verify(smsSender).deliverVoxVerification(eq(SENDER), anyString(), eq(Optional.of(Locale.forLanguageTag("en-US")))); + verify(smsSender).deliverVoxVerification(eq(SENDER), anyString(), eq(Locale.LanguageRange.parse("en-US;q=1, ar-US;q=0.9, fa-US;q=0.8, zh-Hans-US;q=0.7, ru-RU;q=0.6, zh-Hant-US;q=0.5"))); + verify(abusiveHostRules).getAbusiveHostRulesFor(eq(NICE_HOST)); + } + + @Test + public void testSendCodeVoiceInvalidLocale() throws Exception { + Response response = + resources.getJerseyTest() + .target(String.format("/v1/accounts/voice/code/%s", SENDER)) + .queryParam("challenge", "1234-push") + .request() + .header("Accept-Language", "This is not a reasonable Accept-Language value") + .header("X-Forwarded-For", NICE_HOST) + .get(); + + assertThat(response.getStatus()).isEqualTo(400); + + verify(smsSender, never()).deliverVoxVerification(eq(SENDER), anyString(), any()); verify(abusiveHostRules).getAbusiveHostRulesFor(eq(NICE_HOST)); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/VoiceVerificationControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/VoiceVerificationControllerTest.java index 21ff61a9f..29e427221 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/VoiceVerificationControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/VoiceVerificationControllerTest.java @@ -77,6 +77,20 @@ public class VoiceVerificationControllerTest { assertThat(response.readEntity(String.class)).isXmlEqualTo(FixtureHelpers.fixture("fixtures/voice_verification_en_us.xml")); } + @Test + public void testTwimlMultipleLocales() { + Response response = + resources.getJerseyTest() + .target("/v1/voice/description/123456") + .queryParam("l", "es-MX") + .queryParam("l", "ru-RU") + .request() + .post(null); + + assertThat(response.getStatus()).isEqualTo(200); + assertThat(response.readEntity(String.class)).isXmlEqualTo(FixtureHelpers.fixture("fixtures/voice_verification_ru.xml")); + } + @Test public void testTwimlMissingLocale() { Response response = diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/sms/TwilioSmsSenderTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/sms/TwilioSmsSenderTest.java index 1e316804e..9d24aaf3c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/sms/TwilioSmsSenderTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/sms/TwilioSmsSenderTest.java @@ -18,7 +18,7 @@ import static org.mockito.Mockito.*; import com.github.tomakehurst.wiremock.junit.WireMockRule; import java.util.List; -import java.util.Locale; +import java.util.Locale.LanguageRange; import java.util.Optional; import javax.annotation.Nonnull; import org.junit.Before; @@ -136,7 +136,7 @@ public class TwilioSmsSenderTest { TwilioConfiguration configuration = createTwilioConfiguration(); TwilioSmsSender sender = new TwilioSmsSender("http://localhost:" + wireMockRule.port(), configuration, dynamicConfigurationManager); - boolean success = sender.deliverVoxVerification("+14153333333", "123-456", Optional.of(Locale.US)).join(); + boolean success = sender.deliverVoxVerification("+14153333333", "123-456", LanguageRange.parse("en-US")).join(); assertThat(success).isTrue(); @@ -145,6 +145,27 @@ public class TwilioSmsSenderTest { .withRequestBody(matching("To=%2B14153333333&From=%2B1415(1111111|2222222)&Url=https%3A%2F%2Ftest.com%2Fv1%2Fvoice%2Fdescription%2F123-456%3Fl%3Den-US"))); } + @Test + public void testSendVoxMultipleLocales() { + wireMockRule.stubFor(post(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Calls.json")) + .withBasicAuth(ACCOUNT_ID, ACCOUNT_TOKEN) + .willReturn(aResponse() + .withHeader("Content-Type", "application/json") + .withBody("{\"price\": -0.00750, \"status\": \"completed\"}"))); + + + TwilioConfiguration configuration = createTwilioConfiguration(); + + TwilioSmsSender sender = new TwilioSmsSender("http://localhost:" + wireMockRule.port(), configuration, dynamicConfigurationManager); + boolean success = sender.deliverVoxVerification("+14153333333", "123-456", LanguageRange.parse("en-US;q=1, ar-US;q=0.9, fa-US;q=0.8, zh-Hans-US;q=0.7, ru-RU;q=0.6, zh-Hant-US;q=0.5")).join(); + + assertThat(success).isTrue(); + + verify(1, postRequestedFor(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Calls.json")) + .withHeader("Content-Type", equalTo("application/x-www-form-urlencoded")) + .withRequestBody(matching("To=%2B14153333333&From=%2B1415(1111111|2222222)&Url=https%3A%2F%2Ftest.com%2Fv1%2Fvoice%2Fdescription%2F123-456%3Fl%3Den-US%26l%3Dar-US%26l%3Dfa-US%26l%3Dzh-US%26l%3Dru-RU%26l%3Dzh-US"))); + } + @Test public void testSendSmsFiveHundred() { wireMockRule.stubFor(post(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Messages.json")) @@ -179,7 +200,7 @@ public class TwilioSmsSenderTest { TwilioConfiguration configuration = createTwilioConfiguration(); TwilioSmsSender sender = new TwilioSmsSender("http://localhost:" + wireMockRule.port(), configuration, dynamicConfigurationManager); - boolean success = sender.deliverVoxVerification("+14153333333", "123-456", Optional.of(Locale.US)).join(); + boolean success = sender.deliverVoxVerification("+14153333333", "123-456", LanguageRange.parse("en-US")).join(); assertThat(success).isFalse();