Enforce phone number normalization when creating accounts or changing numbers

service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java

@@ -72,6 +72,9 @@ import org.whispersystems.textsecuregcm.entities.RegistrationLock;
 import org.whispersystems.textsecuregcm.entities.RegistrationLockFailure;
 import org.whispersystems.textsecuregcm.limits.RateLimiter;
 import org.whispersystems.textsecuregcm.limits.RateLimiters;
+import org.whispersystems.textsecuregcm.mappers.ImpossiblePhoneNumberExceptionMapper;
+import org.whispersystems.textsecuregcm.mappers.NonNormalizedPhoneNumberExceptionMapper;
+import org.whispersystems.textsecuregcm.mappers.NonNormalizedPhoneNumberResponse;
 import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper;
 import org.whispersystems.textsecuregcm.push.APNSender;
 import org.whispersystems.textsecuregcm.push.ApnMessage;
@@ -154,6 +157,8 @@ class AccountControllerTest {
               ImmutableSet.of(AuthenticatedAccount.class,
                   DisabledPermittedAuthenticatedAccount.class)))
       .addProvider(new RateLimitExceededExceptionMapper())
+      .addProvider(new ImpossiblePhoneNumberExceptionMapper())
+      .addProvider(new NonNormalizedPhoneNumberExceptionMapper())
       .setMapper(SystemMapper.getMapper())
       .setTestContainerFactory(new GrizzlyWebTestContainerFactory())
       .addResource(new AccountController(pendingAccountsManager,
@@ -395,6 +400,39 @@ class AccountControllerTest {
     verifyNoMoreInteractions(gcmSender);
   }
 
+  @Test
+  void testGetPreauthImpossibleNumber() {
+    final Response response = resources.getJerseyTest()
+        .target("/v1/accounts/fcm/preauth/mytoken/BogusNumber")
+        .request()
+        .get();
+
+    assertThat(response.getStatus()).isEqualTo(400);
+    assertThat(response.readEntity(String.class)).isBlank();
+
+    verifyNoMoreInteractions(gcmSender);
+    verifyNoMoreInteractions(apnSender);
+  }
+
+  @Test
+  void testGetPreauthNonNormalized() {
+    final String number = "+4407700900111";
+
+    final Response response = resources.getJerseyTest()
+        .target("/v1/accounts/fcm/preauth/mytoken/" + number)
+        .request()
+        .get();
+
+    assertThat(response.getStatus()).isEqualTo(400);
+
+    final NonNormalizedPhoneNumberResponse responseEntity = response.readEntity(NonNormalizedPhoneNumberResponse.class);
+    assertThat(responseEntity.getOriginalNumber()).isEqualTo(number);
+    assertThat(responseEntity.getNormalizedNumber()).isEqualTo("+447700900111");
+
+    verifyNoMoreInteractions(gcmSender);
+    verifyNoMoreInteractions(apnSender);
+  }
+
   @ParameterizedTest
   @ValueSource(booleans = {false, true})
   void testSendCode(final boolean enrolledInVerifyExperiment) throws Exception {
@@ -434,6 +472,43 @@ class AccountControllerTest {
     verify(abusiveHostRules).getAbusiveHostRulesFor(eq(NICE_HOST));
   }
 
+  @Test
+  void testSendCodeImpossibleNumber() {
+    final Response response =
+        resources.getJerseyTest()
+            .target(String.format("/v1/accounts/sms/code/%s", "Definitely not a real number"))
+            .queryParam("challenge", "1234-push")
+            .request()
+            .header("X-Forwarded-For", NICE_HOST)
+            .get();
+
+    assertThat(response.getStatus()).isEqualTo(400);
+    assertThat(response.readEntity(String.class)).isBlank();
+
+    verify(smsSender, never()).deliverSmsVerification(any(), any(), any());
+  }
+
+  @Test
+  void testSendCodeNonNormalized() {
+    final String number = "+4407700900111";
+
+    final Response response =
+        resources.getJerseyTest()
+            .target(String.format("/v1/accounts/sms/code/%s", number))
+            .queryParam("challenge", "1234-push")
+            .request()
+            .header("X-Forwarded-For", NICE_HOST)
+            .get();
+
+    assertThat(response.getStatus()).isEqualTo(400);
+
+    final NonNormalizedPhoneNumberResponse responseEntity = response.readEntity(NonNormalizedPhoneNumberResponse.class);
+    assertThat(responseEntity.getOriginalNumber()).isEqualTo(number);
+    assertThat(responseEntity.getNormalizedNumber()).isEqualTo("+447700900111");
+
+    verify(smsSender, never()).deliverSmsVerification(any(), any(), any());
+  }
+
   @ParameterizedTest
   @ValueSource(booleans = {false, true})
   public void testSendCodeVoiceNoLocale(final boolean enrolledInVerifyExperiment) throws Exception {
@@ -1151,6 +1226,46 @@ class AccountControllerTest {
     verify(accountsManager).changeNumber(AuthHelper.VALID_ACCOUNT, number);
   }
 
+  @Test
+  void testChangePhoneNumberImpossibleNumber() throws InterruptedException {
+    final String number = "This is not a real phone number";
+    final String code = "987654";
+
+    final Response response =
+        resources.getJerseyTest()
+            .target("/v1/accounts/number")
+            .request()
+            .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD))
+            .put(Entity.entity(new ChangePhoneNumberRequest(number, code, null),
+                MediaType.APPLICATION_JSON_TYPE));
+
+    assertThat(response.getStatus()).isEqualTo(400);
+    assertThat(response.readEntity(String.class)).isBlank();
+    verify(accountsManager, never()).changeNumber(any(), any());
+  }
+
+  @Test
+  void testChangePhoneNumberNonNormalized() throws InterruptedException {
+    final String number = "+4407700900111";
+    final String code = "987654";
+
+    final Response response =
+        resources.getJerseyTest()
+            .target("/v1/accounts/number")
+            .request()
+            .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD))
+            .put(Entity.entity(new ChangePhoneNumberRequest(number, code, null),
+                MediaType.APPLICATION_JSON_TYPE));
+
+    assertThat(response.getStatus()).isEqualTo(400);
+
+    final NonNormalizedPhoneNumberResponse responseEntity = response.readEntity(NonNormalizedPhoneNumberResponse.class);
+    assertThat(responseEntity.getOriginalNumber()).isEqualTo(number);
+    assertThat(responseEntity.getNormalizedNumber()).isEqualTo("+447700900111");
+
+    verify(accountsManager, never()).changeNumber(any(), any());
+  }
+
   @Test
   void testChangePhoneNumberSameNumber() throws InterruptedException {
     final Response response =

service/src/test/java/org/whispersystems/textsecuregcm/tests/util/ValidNumberTest.java

@@ -10,8 +10,8 @@ import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 import org.junit.jupiter.params.provider.ValueSource;
-import org.whispersystems.textsecuregcm.util.ImpossibleNumberException;
-import org.whispersystems.textsecuregcm.util.NonNormalizedNumberException;
+import org.whispersystems.textsecuregcm.util.ImpossiblePhoneNumberException;
+import org.whispersystems.textsecuregcm.util.NonNormalizedPhoneNumberException;
 import org.whispersystems.textsecuregcm.util.Util;
 
 import java.util.stream.Stream;
@@ -22,44 +22,6 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 
 class ValidNumberTest {
 
-  @ParameterizedTest
-  @MethodSource
-  void isValid(final String number, final boolean expectValid) {
-    assertEquals(expectValid, Util.isValidNumber(number));
-  }
-
-  private static Stream<Arguments> isValid() {
-    return Stream.of(
-        // Valid numbers
-        Arguments.of("+14151231234", true),
-        Arguments.of("+71234567890", true),
-        Arguments.of("+447535742222", true),
-        Arguments.of("+4915174108888", true),
-
-        // Invalid e164s
-        Arguments.of("+141512312341", false),
-        Arguments.of("+712345678901", false),
-        Arguments.of("+4475357422221", false),
-        Arguments.of("+491517410888811111", false),
-
-        // Non-e164s
-        Arguments.of("+1 415 123 1234", false),
-        Arguments.of("+1 (415) 123-1234", false),
-        Arguments.of("+1 415)123-1234", false),
-        Arguments.of("71234567890", false),
-        Arguments.of("001447535742222", false),
-        Arguments.of(" +14151231234", false),
-        Arguments.of("+1415123123a", false),
-
-        // Short region
-        Arguments.of("+298123456", true),
-        Arguments.of("+299123456", true),
-        Arguments.of("+376123456", true),
-        Arguments.of("+68512345", true),
-        Arguments.of("+689123456", true)
-    );
-  }
-
   @ParameterizedTest
   @ValueSource(strings = {
       "+447700900111",
@@ -78,7 +40,7 @@ class ValidNumberTest {
 
   @Test
   void requireNormalizedNumberNull() {
-    assertThrows(ImpossibleNumberException.class, () -> Util.requireNormalizedNumber(null));
+    assertThrows(ImpossiblePhoneNumberException.class, () -> Util.requireNormalizedNumber(null));
   }
 
   @ParameterizedTest
@@ -93,7 +55,7 @@ class ValidNumberTest {
       "+1415123123a"
   })
   void requireNormalizedNumberImpossibleNumber(final String number) {
-    assertThrows(ImpossibleNumberException.class, () -> Util.requireNormalizedNumber(number));
+    assertThrows(ImpossiblePhoneNumberException.class, () -> Util.requireNormalizedNumber(number));
   }
 
   @ParameterizedTest
@@ -104,6 +66,6 @@ class ValidNumberTest {
       "+1 415)123-1234",
       " +14151231234"})
   void requireNormalizedNumberNonNormalized(final String number) {
-    assertThrows(NonNormalizedNumberException.class, () -> Util.requireNormalizedNumber(number));
+    assertThrows(NonNormalizedPhoneNumberException.class, () -> Util.requireNormalizedNumber(number));
   }
 }