diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/sms/SenderIdSupplier.java b/service/src/main/java/org/whispersystems/textsecuregcm/sms/SenderIdSupplier.java deleted file mode 100644 index a6aa75565..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/sms/SenderIdSupplier.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Copyright 2013-2020 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.sms; - -import org.apache.commons.lang3.StringUtils; -import org.whispersystems.textsecuregcm.configuration.TwilioCountrySenderIdConfiguration; -import org.whispersystems.textsecuregcm.configuration.TwilioSenderIdConfiguration; -import org.whispersystems.textsecuregcm.util.Util; - -import javax.validation.constraints.NotNull; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.Set; -import java.util.stream.Collectors; - -class SenderIdSupplier { - private final String defaultSenderId; - private final Map countrySpecificSenderIds; - private final Set countryCodesWithoutSenderId; - - SenderIdSupplier(String defaultSenderId, List countrySpecificSenderIds, Set countryCodesWithoutSenderId) { - this.defaultSenderId = defaultSenderId; - this.countrySpecificSenderIds = countrySpecificSenderIds.stream().collect(Collectors.toMap( - TwilioCountrySenderIdConfiguration::getCountryCode, - TwilioCountrySenderIdConfiguration::getSenderId)); - this.countryCodesWithoutSenderId = countryCodesWithoutSenderId; - } - - SenderIdSupplier(TwilioSenderIdConfiguration configuration) { - this(configuration.getDefaultSenderId(), - configuration.getCountrySpecificSenderIds(), - configuration.getCountryCodesWithoutSenderId()); - } - - Optional get(@NotNull String destination) { - final String countryCode = Util.getCountryCode(destination); - if (countryCodesWithoutSenderId.contains(countryCode)) { - return Optional.empty(); - } - - return Optional.ofNullable(StringUtils.stripToNull(countrySpecificSenderIds.getOrDefault(countryCode, defaultSenderId))); - } -} 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 75c632091..8229ff919 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java @@ -38,20 +38,16 @@ import java.util.Map; import java.util.Optional; import java.util.Random; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CompletionStage; import java.util.concurrent.Executor; import static com.codahale.metrics.MetricRegistry.name; @SuppressWarnings("OptionalUsedAsFieldOrParameterType") public class TwilioSmsSender { - private static final int TWILIO_UNREACHABLE_ERROR_CODE = 21612; - private static final Logger logger = LoggerFactory.getLogger(TwilioSmsSender.class); private final MetricRegistry metricRegistry = SharedMetricRegistries.getOrCreate(Constants.METRICS_NAME); private final Meter smsMeter = metricRegistry.meter(name(getClass(), "sms", "delivered")); - private final Meter retriedSmsMeter = metricRegistry.meter(name(getClass(), "sms", "retried")); private final Meter voxMeter = metricRegistry.meter(name(getClass(), "vox", "delivered")); private final Meter priceMeter = metricRegistry.meter(name(getClass(), "price")); @@ -61,7 +57,6 @@ public class TwilioSmsSender { private final String messagingServiceSid; private final String nanpaMessagingServiceSid; private final String localDomain; - private final SenderIdSupplier senderIdSupplier; private final Random random; private final String androidNgVerificationText; private final String android202001VerificationText; @@ -82,7 +77,6 @@ public class TwilioSmsSender { this.localDomain = twilioConfiguration.getLocalDomain(); this.messagingServiceSid = twilioConfiguration.getMessagingServiceSid(); this.nanpaMessagingServiceSid = twilioConfiguration.getNanpaMessagingServiceSid(); - this.senderIdSupplier = new SenderIdSupplier(twilioConfiguration.getSenderId()); this.random = new Random(System.currentTimeMillis()); this.androidNgVerificationText = twilioConfiguration.getAndroidNgVerificationText(); this.android202001VerificationText = twilioConfiguration.getAndroid202001VerificationText(); @@ -106,14 +100,17 @@ public class TwilioSmsSender { } public CompletableFuture deliverSmsVerification(String destination, Optional clientType, String verificationCode) { - return internalSendSmsVerification(destination, clientType, verificationCode, true) - .handle(this::processResponse); - } - - private CompletableFuture internalSendSmsVerification(String destination, Optional clientType, String verificationCode, boolean enableSenderId) { Map requestParameters = new HashMap<>(); requestParameters.put("To", destination); - boolean usedSenderId = setOriginationRequestParameter(destination, requestParameters, enableSenderId); + + if (StringUtils.isNotEmpty(nanpaMessagingServiceSid) && "1".equals(Util.getCountryCode(destination))) { + requestParameters.put("MessagingServiceSid", nanpaMessagingServiceSid); + } else if (StringUtils.isNotEmpty(messagingServiceSid)) { + requestParameters.put("MessagingServiceSid", messagingServiceSid); + } else { + requestParameters.put("From", getRandom(random, numbers)); + } + requestParameters.put("Body", String.format(Locale.US, getBodyFormatString(destination, clientType.orElse(null)), verificationCode)); HttpRequest request = HttpRequest.newBuilder() @@ -126,8 +123,7 @@ public class TwilioSmsSender { smsMeter.mark(); return httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofString()) - .thenApply(this::parseResponse) - .thenCompose(twilioResponse -> retrySendSmsVerificationIfApplicable(usedSenderId, twilioResponse, destination, clientType, verificationCode)); + .thenApply(this::parseResponse).handle(this::processResponse); } private String getBodyFormatString(@Nonnull String destination, @Nullable String clientType) { @@ -176,38 +172,10 @@ public class TwilioSmsSender { .handle(this::processResponse); } - /** - * @return true if alphanumeric sender id was used instead of a messaging service or phone number; false otherwise - */ - private boolean setOriginationRequestParameter(String destination, Map requestParameters, boolean enableSenderId) { - final Optional senderId = senderIdSupplier.get(destination); - boolean retval = false; - if (senderId.isPresent() && enableSenderId) { - requestParameters.put("From", senderId.get()); - retval = true; - } else if (StringUtils.isNotEmpty(nanpaMessagingServiceSid) && "1".equals(Util.getCountryCode(destination))) { - requestParameters.put("MessagingServiceSid", nanpaMessagingServiceSid); - } else if (StringUtils.isNotEmpty(messagingServiceSid)) { - requestParameters.put("MessagingServiceSid", messagingServiceSid); - } else { - requestParameters.put("From", getRandom(random, numbers)); - } - return retval; - } - private String getRandom(Random random, ArrayList elements) { return elements.get(random.nextInt(elements.size())); } - private CompletionStage retrySendSmsVerificationIfApplicable(boolean usedSenderId, TwilioResponse response, String destination, Optional clientType, String verificationCode) { - if (response != null && response.isFailure() && response.failureResponse.code == TWILIO_UNREACHABLE_ERROR_CODE && usedSenderId) { - retriedSmsMeter.mark(); - return internalSendSmsVerification(destination, clientType, verificationCode, false); - } else { - return CompletableFuture.completedFuture(response); - } - } - private boolean processResponse(TwilioResponse response, Throwable throwable) { if (response != null && response.isSuccess()) { priceMeter.mark((long) (response.successResponse.price * 1000)); 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 d551e436b..d4479286e 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 @@ -6,6 +6,7 @@ package org.whispersystems.textsecuregcm.tests.sms; import com.github.tomakehurst.wiremock.junit.WireMockRule; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.whispersystems.textsecuregcm.configuration.TwilioConfiguration; @@ -215,6 +216,7 @@ public class TwilioSmsSenderTest { } @Test + @Ignore public void testSendAlphaIdByCountryCode() { runSenderIdTest("+85278675309", "SIGNAL", () -> { TwilioSenderIdConfiguration senderIdConfiguration = new TwilioSenderIdConfiguration(); @@ -227,6 +229,7 @@ public class TwilioSmsSenderTest { } @Test + @Ignore public void testDefaultSenderId() { runSenderIdTest("+14098675309", "SIGNALFOO", () -> { TwilioSenderIdConfiguration senderIdConfiguration = new TwilioSenderIdConfiguration(); @@ -236,33 +239,7 @@ public class TwilioSmsSenderTest { } @Test - public void testDefaultSenderIdWithDisabledCountry() { - final Supplier senderIdConfigurationSupplier = () -> { - TwilioSenderIdConfiguration senderIdConfiguration = new TwilioSenderIdConfiguration(); - senderIdConfiguration.setDefaultSenderId("SIGNALBAR"); - senderIdConfiguration.setCountryCodesWithoutSenderId(Set.of("1")); - return senderIdConfiguration; - }; - runSenderIdTest("+14098675309", null, senderIdConfigurationSupplier); - runSenderIdTest("+447911123456", "SIGNALBAR", senderIdConfigurationSupplier); - } - - @Test - public void testDefaultSenderIdWithOverriddenCountry() { - final Supplier senderIdConfigurationSupplier = () -> { - TwilioSenderIdConfiguration senderIdConfiguration = new TwilioSenderIdConfiguration(); - TwilioCountrySenderIdConfiguration twilioCountrySenderIdConfiguration = new TwilioCountrySenderIdConfiguration(); - twilioCountrySenderIdConfiguration.setCountryCode("1"); - twilioCountrySenderIdConfiguration.setSenderId("OFCOURSEISTILLLOVEYOU"); - senderIdConfiguration.setDefaultSenderId("JUSTREADTHEINSTRUCTIONS"); - senderIdConfiguration.setCountrySpecificSenderIds(List.of(twilioCountrySenderIdConfiguration)); - return senderIdConfiguration; - }; - runSenderIdTest("+15128675309", "OFCOURSEISTILLLOVEYOU", senderIdConfigurationSupplier); - runSenderIdTest("+6433456789", "JUSTREADTHEINSTRUCTIONS", senderIdConfigurationSupplier); - } - - @Test + @Ignore public void testSenderIdWithAllFieldsPopulated() { final Supplier senderIdConfigurationSupplier = () -> { TwilioSenderIdConfiguration senderIdConfiguration = new TwilioSenderIdConfiguration(); @@ -304,41 +281,6 @@ public class TwilioSmsSenderTest { .withRequestBody(equalTo("MessagingServiceSid=test_messaging_services_id&To=%2B14153333333&Body=%3C%23%3E+Verify+on+AndroidNg%3A+123-456%0A%0Acharacters"))); } - @Test - public void testRetrySmsOnUnreachableErrorCodeSkipsSenderIdSecondTime() { - wireMockRule.stubFor(post(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Messages.json")) - .withBasicAuth(ACCOUNT_ID, ACCOUNT_TOKEN) - .withRequestBody(equalTo("To=%2B14153333333&From=WHENTHEMUSICPLAYS&Body=%3C%23%3E+Verify+on+AndroidNg%3A+123-456%0A%0Acharacters")) - .willReturn(aResponse() - .withStatus(400) - .withHeader("Content-Type", "application/json") - .withBody("{\"status\": 400, \"message\": \"is not currently reachable\", \"code\": 21612}"))); - - wireMockRule.stubFor(post(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Messages.json")) - .withBasicAuth(ACCOUNT_ID, ACCOUNT_TOKEN) - .withRequestBody(equalTo("MessagingServiceSid=test_messaging_services_id&To=%2B14153333333&Body=%3C%23%3E+Verify+on+AndroidNg%3A+123-456%0A%0Acharacters")) - .willReturn(aResponse() - .withHeader("Content-Type", "application/json") - .withBody("{\"price\": -0.00750, \"status\": \"sent\"}"))); - - TwilioConfiguration configuration = createTwilioConfiguration(); - TwilioSenderIdConfiguration twilioSenderIdConfiguration = new TwilioSenderIdConfiguration(); - twilioSenderIdConfiguration.setDefaultSenderId("WHENTHEMUSICPLAYS"); - configuration.setSenderId(twilioSenderIdConfiguration); - - TwilioSmsSender sender = new TwilioSmsSender("http://localhost:" + wireMockRule.port(), configuration); - boolean success = sender.deliverSmsVerification("+14153333333", Optional.of("android-ng"), "123-456").join(); - - assertThat(success).isTrue(); - - verify(1, postRequestedFor(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Messages.json")) - .withHeader("Content-Type", equalTo("application/x-www-form-urlencoded")) - .withRequestBody(equalTo("To=%2B14153333333&From=WHENTHEMUSICPLAYS&Body=%3C%23%3E+Verify+on+AndroidNg%3A+123-456%0A%0Acharacters"))); - verify(1, postRequestedFor(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Messages.json")) - .withHeader("Content-Type", equalTo("application/x-www-form-urlencoded")) - .withRequestBody(equalTo("MessagingServiceSid=test_messaging_services_id&To=%2B14153333333&Body=%3C%23%3E+Verify+on+AndroidNg%3A+123-456%0A%0Acharacters"))); - } - @Test public void testSendSmsChina() { setupSuccessStubForSms();