Reject attempts to change phone numbers if the account doesn't have a number in the first place

service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java

@@ -94,6 +94,13 @@ public class AccountControllerV2 {
       throw new ForbiddenException();
     }
 
+    final Account account = accountsManager.getByAccountIdentifier(authenticatedDevice.accountIdentifier())
+        .orElseThrow(() -> new WebApplicationException(Response.Status.UNAUTHORIZED));
+
+    if (account.getNumberOptional().isEmpty()) {
+      throw new ForbiddenException();
+    }
+
     if (!request.isSignatureValidOnEachSignedPreKey(userAgentString)) {
       throw new WebApplicationException("Invalid signature", 422);
     }

service/src/main/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcService.java

@@ -362,6 +362,12 @@ public class AccountsGrpcService extends SimpleAccountsGrpc.AccountsImplBase {
 
     final AuthenticatedDevice authenticatedDevice = AuthenticationUtil.requireAuthenticatedPrimaryDevice();
 
+    if (getAuthenticatedAccount(authenticatedDevice).getNumberOptional().isEmpty()) {
+      return ChangeNumberResponse.newBuilder()
+          .setAccountDoesNotHavePhoneNumber(FailedPrecondition.getDefaultInstance())
+          .build();
+    }
+
     final IdentityKey pniIdentityKey;
     try {
       pniIdentityKey = new IdentityKey(request.getPniIdentityKey().toByteArray());

service/src/main/proto/org/signal/chat/account.proto

@@ -407,6 +407,9 @@ message ChangeNumberResponse {
     errors.FailedPrecondition invalid_registration_session = 7 [(tag.reason) = "invalid_registration_session"];
 
     errors.FailedPrecondition recovery_password_verification_failed = 8 [(tag.reason) = "recovery_password_verification_failed"];
+
+    // The account does not have a phone number
+    errors.FailedPrecondition account_does_not_have_phone_number = 9 [(tag.reason) = "account_has_no_phone_number"];
   }
 }
 

service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java

@@ -153,6 +153,13 @@ class AccountControllerV2Test {
     @ParameterizedTest
     @ValueSource(booleans = {true, false})
     void changeNumberSuccess(final boolean useSessionVerification) throws Exception {
+      final Account account = mock(Account.class);
+      when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+          PhoneNumberUtil.getInstance().getExampleNumber("DE"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+      when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID))
+          .thenReturn(Optional.of(account));
+
       @Nullable final String sessionId = useSessionVerification ? encodeSessionId("session") : null;
       @Nullable final byte[] recoveryPassword = useSessionVerification ? null : "recovery-password".getBytes(StandardCharsets.UTF_8);
 
@@ -244,6 +251,13 @@ class AccountControllerV2Test {
     @ParameterizedTest
     @MethodSource
     void invalidRegistrationId(final Integer pniRegistrationId, final int expectedStatusCode) {
+      final Account account = mock(Account.class);
+      when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+          PhoneNumberUtil.getInstance().getExampleNumber("DE"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+      when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID))
+          .thenReturn(Optional.of(account));
+
       final ChangeNumberRequest changeNumberRequest = new ChangeNumberRequest(encodeSessionId("session"), null, NEW_NUMBER, "123", IDENTITY_KEY,
           Collections.emptyList(),
           Map.of(Device.PRIMARY_ID, KeysHelper.signedECPreKey(1, IDENTITY_KEY_PAIR)),
@@ -273,6 +287,13 @@ class AccountControllerV2Test {
 
     @Test
     void rateLimitedNumber() throws Exception {
+      final Account account = mock(Account.class);
+      when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+          PhoneNumberUtil.getInstance().getExampleNumber("DE"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+      when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID))
+          .thenReturn(Optional.of(account));
+
       doThrow(new RateLimitExceededException(null))
           .when(changeNumberManager).changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any());
 
@@ -291,6 +312,13 @@ class AccountControllerV2Test {
     @MethodSource
     void phoneVerificationException(final Exception exception, final int expectedStatus)
         throws Exception {
+      final Account account = mock(Account.class);
+      when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+          PhoneNumberUtil.getInstance().getExampleNumber("DE"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+      when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID))
+          .thenReturn(Optional.of(account));
+
       doThrow(exception)
           .when(changeNumberManager).changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any());
 
@@ -316,6 +344,13 @@ class AccountControllerV2Test {
 
     @Test
     void deviceMessageTooLarge() throws Exception {
+      final Account account = mock(Account.class);
+      when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+          PhoneNumberUtil.getInstance().getExampleNumber("DE"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+      when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID))
+          .thenReturn(Optional.of(account));
+
       doThrow(MessageTooLargeException.class)
           .when(changeNumberManager).changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any());
 
@@ -338,6 +373,13 @@ class AccountControllerV2Test {
 
     @Test
     void messageDeliveryNotAllowed() throws Exception {
+      final Account account = mock(Account.class);
+      when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+          PhoneNumberUtil.getInstance().getExampleNumber("DE"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+      when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID))
+          .thenReturn(Optional.of(account));
+
       doThrow(MessageDeliveryNotAllowedException.class)
           .when(changeNumberManager).changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any());
 
@@ -358,6 +400,32 @@ class AccountControllerV2Test {
       }
     }
 
+    @Test
+    void accountHasNoNumber() throws Exception {
+      final Account numberlessAccount = mock(Account.class);
+      when(numberlessAccount.getNumberOptional()).thenReturn(Optional.empty());
+
+      when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID))
+          .thenReturn(Optional.of(numberlessAccount));
+
+      try (final Response response = resources.getJerseyTest()
+          .target("/v2/accounts/number")
+          .request()
+          .header(HttpHeaders.AUTHORIZATION,
+              AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD))
+          .put(Entity.entity(
+              new ChangeNumberRequest(encodeSessionId("session"), null, NEW_NUMBER, "123", IDENTITY_KEY,
+                  Collections.emptyList(),
+                  Map.of(Device.PRIMARY_ID, KeysHelper.signedECPreKey(1, IDENTITY_KEY_PAIR)),
+                  Map.of(Device.PRIMARY_ID, KeysHelper.signedKEMPreKey(2, IDENTITY_KEY_PAIR)),
+                  Map.of(Device.PRIMARY_ID, 17)),
+              MediaType.APPLICATION_JSON_TYPE))) {
+
+        assertEquals(403, response.getStatus());
+        verify(changeNumberManager, never()).changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any());
+      }
+    }
+
     /**
      * Valid request JSON with the give session ID and recovery password
      */

service/src/test/java/org/whispersystems/textsecuregcm/grpc/AccountsGrpcServiceTest.java

@@ -836,6 +836,12 @@ class AccountsGrpcServiceTest extends SimpleBaseGrpcTest<AccountsGrpcService, Ac
     when(updatedAccount.getPhoneNumberIdentifierOptional()).thenReturn(Optional.of(updatedPni));
     when(updatedAccount.getUsernameHash()).thenReturn(Optional.empty());
 
+    final Account originalAccount = mock(Account.class);
+    when(originalAccount.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+        PhoneNumberUtil.getInstance().getExampleNumber("DE"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+    when(accountsManager.getByAccountIdentifier(AUTHENTICATED_ACI)).thenReturn(Optional.of(originalAccount));
+
     when(changeNumberManager.changeNumber(eq(AUTHENTICATED_ACI), any(), any(), any(), eq(newNumber),
         any(), any(), any(), any(), any(), any(), any(), any()))
         .thenReturn(updatedAccount);
@@ -873,6 +879,12 @@ class AccountsGrpcServiceTest extends SimpleBaseGrpcTest<AccountsGrpcService, Ac
   void changeNumberErrorResponse(final Exception exceptionToThrow, final ChangeNumberResponse expectedResponse)
       throws Exception {
 
+    final Account account = mock(Account.class);
+    when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+        PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+    when(accountsManager.getByAccountIdentifier(AUTHENTICATED_ACI)).thenReturn(Optional.of(account));
+
     when(changeNumberManager.changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any()))
         .thenThrow(exceptionToThrow);
 
@@ -906,6 +918,12 @@ class AccountsGrpcServiceTest extends SimpleBaseGrpcTest<AccountsGrpcService, Ac
   @ParameterizedTest
   @MethodSource
   void changeNumberUnavailable(final Exception exceptionToThrow) throws Exception {
+    final Account account = mock(Account.class);
+    when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+        PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+    when(accountsManager.getByAccountIdentifier(AUTHENTICATED_ACI)).thenReturn(Optional.of(account));
+
     when(changeNumberManager.changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any()))
         .thenThrow(exceptionToThrow);
 
@@ -923,6 +941,12 @@ class AccountsGrpcServiceTest extends SimpleBaseGrpcTest<AccountsGrpcService, Ac
   void changeNumberRegistrationLockFailure() throws Exception {
     final long timeRemaining = Duration.ofDays(7).toMillis();
 
+    final Account account = mock(Account.class);
+    when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+        PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+    when(accountsManager.getByAccountIdentifier(AUTHENTICATED_ACI)).thenReturn(Optional.of(account));
+
     when(changeNumberManager.changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any()))
         .thenThrow(new RegistrationLockFailureException(new org.whispersystems.textsecuregcm.entities.RegistrationLockFailure(
             timeRemaining,
@@ -943,6 +967,12 @@ class AccountsGrpcServiceTest extends SimpleBaseGrpcTest<AccountsGrpcService, Ac
   void changeNumberStaleDevices() throws Exception {
     final byte staleDeviceId = (byte) (Device.PRIMARY_ID + 1);
 
+    final Account account = mock(Account.class);
+    when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+        PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+    when(accountsManager.getByAccountIdentifier(AUTHENTICATED_ACI)).thenReturn(Optional.of(account));
+
     when(changeNumberManager.changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any()))
         .thenThrow(new MismatchedDevicesException(new MismatchedDevices(Set.of(), Set.of(), Set.of(staleDeviceId))));
 
@@ -958,6 +988,12 @@ class AccountsGrpcServiceTest extends SimpleBaseGrpcTest<AccountsGrpcService, Ac
     final byte missingDeviceId = (byte) (Device.PRIMARY_ID + 1);
     final byte extraDeviceId = (byte) (Device.PRIMARY_ID + 2);
 
+    final Account account = mock(Account.class);
+    when(account.getNumberOptional()).thenReturn(Optional.of(PhoneNumberUtil.getInstance().format(
+        PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164)));
+
+    when(accountsManager.getByAccountIdentifier(AUTHENTICATED_ACI)).thenReturn(Optional.of(account));
+
     when(changeNumberManager.changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any()))
         .thenThrow(new MismatchedDevicesException(new MismatchedDevices(Set.of(missingDeviceId), Set.of(extraDeviceId), Set.of())));
 
@@ -971,6 +1007,52 @@ class AccountsGrpcServiceTest extends SimpleBaseGrpcTest<AccountsGrpcService, Ac
     assertEquals(expectedResponse, authenticatedServiceStub().changeNumber(createChangeNumberRequest()));
   }
 
+  @Test
+  void changeNumberAccountHasNoNumber() throws Exception {
+    final Account numberlessAccount = mock(Account.class);
+    when(numberlessAccount.getNumberOptional()).thenReturn(Optional.empty());
+
+    when(accountsManager.getByAccountIdentifier(AUTHENTICATED_ACI))
+        .thenReturn(Optional.of(numberlessAccount));
+
+    final String newNumber = PhoneNumberUtil.getInstance().format(
+        PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164);
+
+    final ECKeyPair pniIdentityKeyPair = ECKeyPair.generate();
+    final IdentityKey pniIdentityKey = new IdentityKey(pniIdentityKeyPair.getPublicKey());
+
+    final ECSignedPreKey ecSignedPreKey = KeysHelper.signedECPreKey(1, pniIdentityKeyPair);
+    final KEMSignedPreKey kemSignedPreKey = KeysHelper.signedKEMPreKey(2, pniIdentityKeyPair);
+
+    final byte[] sessionId = TestRandomUtil.nextBytes(16);
+    final UUID updatedPni = UUID.randomUUID();
+
+    final ChangeNumberResponse response = authenticatedServiceStub().changeNumber(ChangeNumberRequest.newBuilder()
+        .setSessionId(ByteString.copyFrom(sessionId))
+        .setNumber(newNumber)
+        .setRegistrationLock(ByteString.copyFrom(TestRandomUtil.nextBytes(32)))
+        .setPniIdentityKey(ByteString.copyFrom(pniIdentityKey.serialize()))
+        .putDevicePniSignedPreKeys(Device.PRIMARY_ID, EcSignedPreKey.newBuilder()
+            .setKeyId(KeyIdUtil.toUnsignedInt(ecSignedPreKey.keyId()))
+            .setPublicKey(ByteString.copyFrom(ecSignedPreKey.serializedPublicKey()))
+            .setSignature(ByteString.copyFrom(ecSignedPreKey.signature()))
+            .build())
+        .putDevicePniPqLastResortPreKeys(Device.PRIMARY_ID, KemSignedPreKey.newBuilder()
+            .setKeyId(KeyIdUtil.toUnsignedInt(kemSignedPreKey.keyId()))
+            .setPublicKey(ByteString.copyFrom(kemSignedPreKey.serializedPublicKey()))
+            .setSignature(ByteString.copyFrom(kemSignedPreKey.signature()))
+            .build())
+        .putPniRegistrationIds(Device.PRIMARY_ID, 17)
+        .build());
+
+    final ChangeNumberResponse expectedResponse = ChangeNumberResponse.newBuilder()
+        .setAccountDoesNotHavePhoneNumber(FailedPrecondition.getDefaultInstance())
+        .build();
+
+    assertEquals(expectedResponse, response);
+    verify(changeNumberManager, never()).changeNumber(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), any());
+  }
+
   private static ChangeNumberRequest createChangeNumberRequest() {
     final ECKeyPair pniIdentityKeyPair = ECKeyPair.generate();
     final IdentityKey pniIdentityKey = new IdentityKey(pniIdentityKeyPair.getPublicKey());