From b7e0e5a356311ddb18177064af4293dbfde443d9 Mon Sep 17 00:00:00 2001 From: Ehren Kret Date: Mon, 13 Jul 2020 23:25:06 -0500 Subject: [PATCH] Create a strategy class to decide which sender id to use The rules around selecting sender ids can get complicated with some countries not supporting it and others requiring pre-registration that may result in having a different sender id for that country than others. This strategy class handles the logic of dealing with this expanded configuration and applying the appropriate sender id or none when it's not appropriate to do so at all. --- .../TwilioAlphaIdConfiguration.java | 31 ------ .../configuration/TwilioConfiguration.java | 17 +-- .../TwilioCountrySenderIdConfiguration.java | 31 ++++++ .../TwilioSenderIdConfiguration.java | 51 +++++++++ .../textsecuregcm/sms/SenderIdStrategy.java | 47 ++++++++ .../textsecuregcm/sms/TwilioSmsSender.java | 39 ++----- .../tests/sms/TwilioSmsSenderTest.java | 105 ++++++++++++++++-- 7 files changed, 240 insertions(+), 81 deletions(-) delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioAlphaIdConfiguration.java create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioCountrySenderIdConfiguration.java create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioSenderIdConfiguration.java create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/sms/SenderIdStrategy.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioAlphaIdConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioAlphaIdConfiguration.java deleted file mode 100644 index f3ecc3093..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioAlphaIdConfiguration.java +++ /dev/null @@ -1,31 +0,0 @@ -package org.whispersystems.textsecuregcm.configuration; - -import com.google.common.annotations.VisibleForTesting; - -import javax.validation.constraints.NotEmpty; - -public class TwilioAlphaIdConfiguration { - @NotEmpty - private String prefix; - - @NotEmpty - private String value; - - public String getPrefix() { - return prefix; - } - - @VisibleForTesting - public void setPrefix(String prefix) { - this.prefix = prefix; - } - - public String getValue() { - return value; - } - - @VisibleForTesting - public void setValue(String value) { - this.value = value; - } -} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioConfiguration.java index 8d2352830..daa8fbf07 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioConfiguration.java @@ -16,34 +16,27 @@ */ package org.whispersystems.textsecuregcm.configuration; -import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.annotations.VisibleForTesting; import javax.validation.Valid; import javax.validation.constraints.NotEmpty; import javax.validation.constraints.NotNull; -import java.util.ArrayList; import java.util.List; public class TwilioConfiguration { @NotEmpty - @JsonProperty private String accountId; @NotEmpty - @JsonProperty private String accountToken; @NotNull - @JsonProperty private List numbers; @NotEmpty - @JsonProperty private String localDomain; - @JsonProperty private String messagingServicesId; @NotNull @@ -56,7 +49,7 @@ public class TwilioConfiguration { @NotNull @Valid - private List alphaId = new ArrayList<>(); + private TwilioSenderIdConfiguration senderId = new TwilioSenderIdConfiguration(); public String getAccountId() { return accountId; @@ -121,12 +114,12 @@ public class TwilioConfiguration { this.retry = retry; } - public List getAlphaId() { - return alphaId; + public TwilioSenderIdConfiguration getSenderId() { + return senderId; } @VisibleForTesting - public void setAlphaId(List alphaId) { - this.alphaId = alphaId; + public void setSenderId(TwilioSenderIdConfiguration senderId) { + this.senderId = senderId; } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioCountrySenderIdConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioCountrySenderIdConfiguration.java new file mode 100644 index 000000000..7f444cdf1 --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioCountrySenderIdConfiguration.java @@ -0,0 +1,31 @@ +package org.whispersystems.textsecuregcm.configuration; + +import com.google.common.annotations.VisibleForTesting; + +import javax.validation.constraints.NotEmpty; + +public class TwilioCountrySenderIdConfiguration { + @NotEmpty + private String countryCode; + + @NotEmpty + private String senderId; + + public String getCountryCode() { + return countryCode; + } + + @VisibleForTesting + public void setCountryCode(String countryCode) { + this.countryCode = countryCode; + } + + public String getSenderId() { + return senderId; + } + + @VisibleForTesting + public void setSenderId(String senderId) { + this.senderId = senderId; + } +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioSenderIdConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioSenderIdConfiguration.java new file mode 100644 index 000000000..7dc769898 --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/TwilioSenderIdConfiguration.java @@ -0,0 +1,51 @@ +package org.whispersystems.textsecuregcm.configuration; + +import com.google.common.annotations.VisibleForTesting; + +import javax.validation.Valid; +import javax.validation.constraints.NotEmpty; +import javax.validation.constraints.NotNull; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +public class TwilioSenderIdConfiguration { + @NotEmpty + private String defaultSenderId; + + @NotNull + @Valid + private List countrySpecificSenderIds = new ArrayList<>(); + + @NotNull + @Valid + private Set countryCodesWithoutSenderId = new HashSet<>(); + + public String getDefaultSenderId() { + return defaultSenderId; + } + + @VisibleForTesting + public void setDefaultSenderId(String defaultSenderId) { + this.defaultSenderId = defaultSenderId; + } + + public List getCountrySpecificSenderIds() { + return countrySpecificSenderIds; + } + + @VisibleForTesting + public void setCountrySpecificSenderIds(List countrySpecificSenderIds) { + this.countrySpecificSenderIds = countrySpecificSenderIds; + } + + public Set getCountryCodesWithoutSenderId() { + return countryCodesWithoutSenderId; + } + + @VisibleForTesting + public void setCountryCodesWithoutSenderId(Set countryCodesWithoutSenderId) { + this.countryCodesWithoutSenderId = countryCodesWithoutSenderId; + } +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/sms/SenderIdStrategy.java b/service/src/main/java/org/whispersystems/textsecuregcm/sms/SenderIdStrategy.java new file mode 100644 index 000000000..900ba6e27 --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/sms/SenderIdStrategy.java @@ -0,0 +1,47 @@ +package org.whispersystems.textsecuregcm.sms; + +import com.google.common.base.Strings; +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; + +public class SenderIdStrategy { + private final String defaultSenderId; + private final Map countrySpecificSenderIds; + private final Set countryCodesWithoutSenderId; + + public SenderIdStrategy(String defaultSenderId, List countrySpecificSenderIds, Set countryCodesWithoutSenderId) { + this.defaultSenderId = defaultSenderId; + this.countrySpecificSenderIds = countrySpecificSenderIds.stream().collect(Collectors.toMap( + TwilioCountrySenderIdConfiguration::getCountryCode, + TwilioCountrySenderIdConfiguration::getSenderId)); + this.countryCodesWithoutSenderId = countryCodesWithoutSenderId; + } + + public SenderIdStrategy(TwilioSenderIdConfiguration configuration) { + this(configuration.getDefaultSenderId(), + configuration.getCountrySpecificSenderIds(), + configuration.getCountryCodesWithoutSenderId()); + } + + public Optional get(@NotNull String destination) { + final String countryCode = Util.getCountryCode(destination); + if (countryCodesWithoutSenderId.contains(countryCode)) { + return Optional.empty(); + } + + final String countrySpecificSenderId = countrySpecificSenderIds.get(countryCode); + if (!Strings.isNullOrEmpty(countrySpecificSenderId)) { + return Optional.of(countrySpecificSenderId); + } + + return Optional.ofNullable(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 193a7b3a3..a81182558 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/sms/TwilioSmsSender.java @@ -24,7 +24,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.annotations.VisibleForTesting; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.whispersystems.textsecuregcm.configuration.TwilioAlphaIdConfiguration; import org.whispersystems.textsecuregcm.configuration.TwilioConfiguration; import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient; import org.whispersystems.textsecuregcm.http.FormDataBodyPublisher; @@ -42,7 +41,6 @@ import java.net.http.HttpResponse; import java.time.Duration; import java.util.ArrayList; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Random; @@ -62,13 +60,13 @@ public class TwilioSmsSender { private final Meter voxMeter = metricRegistry.meter(name(getClass(), "vox", "delivered")); private final Meter priceMeter = metricRegistry.meter(name(getClass(), "price")); - private final String accountId; - private final String accountToken; - private final ArrayList numbers; - private final String messagingServicesId; - private final String localDomain; - private final List alphaId; - private final Random random; + private final String accountId; + private final String accountToken; + private final ArrayList numbers; + private final String messagingServicesId; + private final String localDomain; + private final Random random; + private final SenderIdStrategy senderIdStrategy; private final FaultTolerantHttpClient httpClient; private final URI smsUri; @@ -83,7 +81,7 @@ public class TwilioSmsSender { this.numbers = new ArrayList<>(twilioConfiguration.getNumbers()); this.localDomain = twilioConfiguration.getLocalDomain(); this.messagingServicesId = twilioConfiguration.getMessagingServicesId(); - this.alphaId = twilioConfiguration.getAlphaId(); + this.senderIdStrategy = new SenderIdStrategy(twilioConfiguration.getSenderId()); this.random = new Random(System.currentTimeMillis()); this.smsUri = URI.create(baseUri + "/2010-04-01/Accounts/" + accountId + "/Messages.json"); this.voxUri = URI.create(baseUri + "/2010-04-01/Accounts/" + accountId + "/Calls.json" ); @@ -176,9 +174,9 @@ public class TwilioSmsSender { } private void setOriginationRequestParameter(String destination, Map requestParameters) { - final Optional alphaIdConfiguration = getAlphaIdConfigurationForNumber(destination); - if (alphaIdConfiguration.isPresent()) { - requestParameters.put("From", alphaIdConfiguration.get().getValue()); + final Optional senderId = senderIdStrategy.get(destination); + if (senderId.isPresent()) { + requestParameters.put("From", senderId.get()); } else if (Util.isEmpty(messagingServicesId)) { requestParameters.put("From", getRandom(random, numbers)); } else { @@ -224,21 +222,6 @@ public class TwilioSmsSender { } } - private Optional getAlphaIdConfigurationForNumber(String number) { - if (alphaId == null) { - return Optional.empty(); - } - - final String countryCode = Util.getCountryCode(number); - for (TwilioAlphaIdConfiguration twilioAlphaIdConfiguration : alphaId) { - if (twilioAlphaIdConfiguration.getPrefix().equalsIgnoreCase(countryCode)) { - return Optional.of(twilioAlphaIdConfiguration); - } - } - - return Optional.empty(); - } - public static class TwilioResponse { private TwilioSuccessResponse successResponse; 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 d76a3ebb5..84fcc6fbc 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 @@ -3,15 +3,26 @@ package org.whispersystems.textsecuregcm.tests.sms; import com.github.tomakehurst.wiremock.junit.WireMockRule; import org.junit.Rule; import org.junit.Test; -import org.whispersystems.textsecuregcm.configuration.TwilioAlphaIdConfiguration; import org.whispersystems.textsecuregcm.configuration.TwilioConfiguration; +import org.whispersystems.textsecuregcm.configuration.TwilioCountrySenderIdConfiguration; +import org.whispersystems.textsecuregcm.configuration.TwilioSenderIdConfiguration; import org.whispersystems.textsecuregcm.sms.TwilioSmsSender; import javax.annotation.Nonnull; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Optional; +import java.util.Set; +import java.util.function.Supplier; -import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.matching; +import static com.github.tomakehurst.wiremock.client.WireMock.post; +import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.client.WireMock.verify; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; @@ -133,8 +144,9 @@ public class TwilioSmsSenderTest { assertThat(success).isFalse(); } - @Test - public void testSendAlphaIdByCountryCode() { + private void runSenderIdTest(String destination, + String expectedSenderId, + Supplier senderIdConfigurationSupplier) { wireMockRule.stubFor(post(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Messages.json")) .withBasicAuth(ACCOUNT_ID, ACCOUNT_TOKEN) .willReturn(aResponse() @@ -142,19 +154,92 @@ public class TwilioSmsSenderTest { .withBody("{\"price\": -0.00750, \"status\": \"sent\"}"))); TwilioConfiguration configuration = createTwilioConfiguration(); - TwilioAlphaIdConfiguration alphaIdConfiguration = new TwilioAlphaIdConfiguration(); - alphaIdConfiguration.setPrefix("852"); - alphaIdConfiguration.setValue("SIGNAL"); - configuration.setAlphaId(List.of(alphaIdConfiguration)); + configuration.setSenderId(senderIdConfigurationSupplier.get()); TwilioSmsSender sender = new TwilioSmsSender("http://localhost:" + wireMockRule.port(), configuration); - boolean success = sender.deliverSmsVerification("+85278675309", Optional.of("android-ng"), "987-654").join(); + boolean success = sender.deliverSmsVerification(destination, Optional.of("android-ng"), "987-654").join(); assertThat(success).isTrue(); + final String requestBodyToParam = "To=" + URLEncoder.encode(destination, StandardCharsets.UTF_8); + final String requestBodySuffix = "&Body=%3C%23%3E+Your+Signal+verification+code%3A+987-654%0A%0AdoDiFGKPO1r"; + final String expectedRequestBody; + if (expectedSenderId != null) { + expectedRequestBody = requestBodyToParam + "&From=" + expectedSenderId + requestBodySuffix; + } else { + expectedRequestBody = "MessagingServiceSid=" + MESSAGING_SERVICES_ID + "&" + requestBodyToParam + requestBodySuffix; + } verify(1, postRequestedFor(urlEqualTo("/2010-04-01/Accounts/" + ACCOUNT_ID + "/Messages.json")) .withHeader("Content-Type", equalTo("application/x-www-form-urlencoded")) - .withRequestBody(equalTo("To=%2B85278675309&From=SIGNAL&Body=%3C%23%3E+Your+Signal+verification+code%3A+987-654%0A%0AdoDiFGKPO1r"))); + .withRequestBody(equalTo(expectedRequestBody))); } + @Test + public void testSendAlphaIdByCountryCode() { + runSenderIdTest("+85278675309", "SIGNAL", () -> { + TwilioSenderIdConfiguration senderIdConfiguration = new TwilioSenderIdConfiguration(); + TwilioCountrySenderIdConfiguration twilioCountrySenderIdConfiguration = new TwilioCountrySenderIdConfiguration(); + twilioCountrySenderIdConfiguration.setCountryCode("852"); + twilioCountrySenderIdConfiguration.setSenderId("SIGNAL"); + senderIdConfiguration.setCountrySpecificSenderIds(List.of(twilioCountrySenderIdConfiguration)); + return senderIdConfiguration; + }); + } + + @Test + public void testDefaultSenderId() { + runSenderIdTest("+14098675309", "SIGNALFOO", () -> { + TwilioSenderIdConfiguration senderIdConfiguration = new TwilioSenderIdConfiguration(); + senderIdConfiguration.setDefaultSenderId("SIGNALFOO"); + return senderIdConfiguration; + }); + } + + @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 + public void testSenderIdWithAllFieldsPopulated() { + final Supplier senderIdConfigurationSupplier = () -> { + TwilioSenderIdConfiguration senderIdConfiguration = new TwilioSenderIdConfiguration(); + TwilioCountrySenderIdConfiguration twilioCountrySenderIdConfiguration1 = new TwilioCountrySenderIdConfiguration(); + TwilioCountrySenderIdConfiguration twilioCountrySenderIdConfiguration2 = new TwilioCountrySenderIdConfiguration(); + twilioCountrySenderIdConfiguration1.setCountryCode("1"); + twilioCountrySenderIdConfiguration1.setSenderId("KNOWYOUREOUTTHERE"); + twilioCountrySenderIdConfiguration2.setCountryCode("61"); + twilioCountrySenderIdConfiguration2.setSenderId("SOMEWHERE"); + senderIdConfiguration.setDefaultSenderId("SOMEHOW"); + senderIdConfiguration.setCountrySpecificSenderIds(List.of(twilioCountrySenderIdConfiguration1, twilioCountrySenderIdConfiguration2)); + senderIdConfiguration.setCountryCodesWithoutSenderId(Set.of("7")); + return senderIdConfiguration; + }; + runSenderIdTest("+15128675309", "KNOWYOUREOUTTHERE", senderIdConfigurationSupplier); + runSenderIdTest("+610998765432", "SOMEWHERE", senderIdConfigurationSupplier); + runSenderIdTest("+74991234567", null, senderIdConfigurationSupplier); + runSenderIdTest("+85278675309", "SOMEHOW", senderIdConfigurationSupplier); + } }