Add support for changing phone numbers

2026-04-20 05:08:03 +01:00 · 2021-08-02 16:41:10 -04:00
parent aa4bd92fee
commit ba58a95a0f
11 changed files with 947 additions and 66 deletions
--- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java
@@ -54,6 +54,7 @@ import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicSignupCaptc
 import org.whispersystems.textsecuregcm.entities.AccountAttributes;
 import org.whispersystems.textsecuregcm.entities.AccountCreationResult;
 import org.whispersystems.textsecuregcm.entities.ApnRegistrationId;
+import org.whispersystems.textsecuregcm.entities.ChangePhoneNumberRequest;
 import org.whispersystems.textsecuregcm.entities.DeviceName;
 import org.whispersystems.textsecuregcm.entities.GcmRegistrationId;
 import org.whispersystems.textsecuregcm.entities.RegistrationLock;
@@ -347,32 +348,18 @@ public class AccountController {
    storedVerificationCode.flatMap(StoredVerificationCode::getTwilioVerificationSid)
        .ifPresent(smsSender::reportVerificationSucceeded);

-    Optional<Account>                    existingAccount           = accounts.get(number);
-    Optional<StoredRegistrationLock>     existingRegistrationLock  = existingAccount.map(Account::getRegistrationLock);
-    Optional<ExternalServiceCredentials> existingBackupCredentials = existingAccount.map(Account::getUuid)
-                                                                                    .map(uuid -> backupServiceCredentialGenerator.generateFor(uuid.toString()));
+      Optional<Account> existingAccount = accounts.get(number);

-    if (existingRegistrationLock.isPresent() && existingRegistrationLock.get().requiresClientRegistrationLock()) {
-      rateLimiters.getVerifyLimiter().clear(number);
-
-      if (!Util.isEmpty(accountAttributes.getRegistrationLock())) {
-        rateLimiters.getPinLimiter().validate(number);
+      if (existingAccount.isPresent()) {
+        verifyRegistrationLock(existingAccount.get(), accountAttributes.getRegistrationLock());
      }

-      if (!existingRegistrationLock.get().verify(accountAttributes.getRegistrationLock())) {
-        throw new WebApplicationException(Response.status(423)
-                                                  .entity(new RegistrationLockFailure(existingRegistrationLock.get().getTimeRemaining(),
-                                                                                      existingRegistrationLock.get().needsFailureCredentials() ? existingBackupCredentials.orElseThrow() : null))
-                                                  .build());
-      }
-
-      rateLimiters.getPinLimiter().clear(number);
-    }
-
    if (availableForTransfer.orElse(false) && existingAccount.map(Account::isTransferSupported).orElse(false)) {
      throw new WebApplicationException(Response.status(409).build());
    }

+    rateLimiters.getVerifyLimiter().clear(number);
+
    Account account = accounts.create(number, password, signalAgent, accountAttributes);

    {
@@ -392,6 +379,42 @@ public class AccountController {
    return new AccountCreationResult(account.getUuid(), account.getNumber(), existingAccount.map(Account::isStorageSupported).orElse(false));
  }

+  @Timed
+  @PUT
+  @Path("/number")
+  @Produces(MediaType.APPLICATION_JSON)
+  public void changeNumber(@Auth final AuthenticatedAccount authenticatedAccount, @Valid final ChangePhoneNumberRequest request)
+      throws RateLimitExceededException, InterruptedException {
+
+    if (request.getNumber().equals(authenticatedAccount.getAccount().getNumber())) {
+      // This may be a request that got repeated due to poor network conditions or other client error; take no action,
+      // but report success since the account is in the desired state
+      return;
+    }
+
+    rateLimiters.getVerifyLimiter().validate(request.getNumber());
+
+    final Optional<StoredVerificationCode> storedVerificationCode =
+        pendingAccounts.getCodeForNumber(request.getNumber());
+
+    if (storedVerificationCode.isEmpty() || !storedVerificationCode.get().isValid(request.getCode())) {
+      throw new WebApplicationException(Response.status(403).build());
+    }
+
+    storedVerificationCode.flatMap(StoredVerificationCode::getTwilioVerificationSid)
+        .ifPresent(smsSender::reportVerificationSucceeded);
+
+    final Optional<Account> existingAccount = accounts.get(request.getNumber());
+
+    if (existingAccount.isPresent()) {
+      verifyRegistrationLock(existingAccount.get(), request.getRegistrationLock());
+    }
+
+    rateLimiters.getVerifyLimiter().clear(request.getNumber());
+
+    accounts.changeNumber(authenticatedAccount.getAccount(), request.getNumber());
+  }
+
  @Timed
  @GET
  @Path("/turn/")
@@ -590,6 +613,29 @@ public class AccountController {
    return Response.ok().build();
  }

+  private void verifyRegistrationLock(final Account existingAccount, @Nullable final String clientRegistrationLock)
+      throws RateLimitExceededException, WebApplicationException {
+
+    final StoredRegistrationLock existingRegistrationLock = existingAccount.getRegistrationLock();
+    final ExternalServiceCredentials existingBackupCredentials =
+        backupServiceCredentialGenerator.generateFor(existingAccount.getUuid().toString());
+
+    if (existingRegistrationLock.requiresClientRegistrationLock()) {
+      if (!Util.isEmpty(clientRegistrationLock)) {
+        rateLimiters.getPinLimiter().validate(existingAccount.getNumber());
+      }
+
+      if (!existingRegistrationLock.verify(clientRegistrationLock)) {
+        throw new WebApplicationException(Response.status(423)
+            .entity(new RegistrationLockFailure(existingRegistrationLock.getTimeRemaining(),
+                existingRegistrationLock.needsFailureCredentials() ? existingBackupCredentials : null))
+            .build());
+      }
+
+      rateLimiters.getPinLimiter().clear(existingAccount.getNumber());
+    }
+  }
+
  private CaptchaRequirement requiresCaptcha(String number, String transport, String forwardedFor,
                                             String                           sourceHost,
                                             Optional<String>                 captchaToken,
--- a/service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangePhoneNumberRequest.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangePhoneNumberRequest.java
@@ -0,0 +1,49 @@
+/*
+ * Copyright 2013-2021 Signal Messenger, LLC
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+package org.whispersystems.textsecuregcm.entities;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import javax.annotation.Nullable;
+import javax.validation.constraints.NotBlank;
+
+public class ChangePhoneNumberRequest {
+
+  @JsonProperty
+  @NotBlank
+  final String number;
+
+  @JsonProperty
+  @NotBlank
+  final String code;
+
+  @JsonProperty("reglock")
+  @Nullable
+  final String registrationLock;
+
+  @JsonCreator
+  public ChangePhoneNumberRequest(@JsonProperty("number") final String number,
+      @JsonProperty("code") final String code,
+      @JsonProperty("reglock") @Nullable final String registrationLock) {
+
+    this.number = number;
+    this.code = code;
+    this.registrationLock = registrationLock;
+  }
+
+  public String getNumber() {
+    return number;
+  }
+
+  public String getCode() {
+    return code;
+  }
+
+  @Nullable
+  public String getRegistrationLock() {
+    return registrationLock;
+  }
+}
--- a/service/src/main/java/org/whispersystems/textsecuregcm/sqs/DirectoryQueue.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/sqs/DirectoryQueue.java
@@ -96,14 +96,20 @@ public class DirectoryQueue implements Managed {
  }

  public void refreshAccount(final Account account) {
-    sendUpdateMessage(account, account.shouldBeVisibleInDirectory() ? UpdateAction.ADD : UpdateAction.DELETE);
+    sendUpdateMessage(account.getUuid(), account.getNumber(),
+            account.shouldBeVisibleInDirectory() ? UpdateAction.ADD : UpdateAction.DELETE);
  }

  public void deleteAccount(final Account account) {
-    sendUpdateMessage(account, UpdateAction.DELETE);
+    sendUpdateMessage(account.getUuid(), account.getNumber(), UpdateAction.DELETE);
  }

-  private void sendUpdateMessage(final Account account, final UpdateAction action) {
+  public void changePhoneNumber(final Account account, final String originalNumber, final String newNumber) {
+    sendUpdateMessage(account.getUuid(), originalNumber, UpdateAction.DELETE);
+    sendUpdateMessage(account.getUuid(), newNumber, account.shouldBeVisibleInDirectory() ? UpdateAction.ADD : UpdateAction.DELETE);
+  }
+
+  private void sendUpdateMessage(final UUID uuid, final String number, final UpdateAction action) {
    for (final String queueUrl : queueUrls) {
      final Timer.Context timerContext = sendMessageBatchTimer.time();

@@ -111,10 +117,10 @@ public class DirectoryQueue implements Managed {
          .queueUrl(queueUrl)
          .messageBody("-")
          .messageDeduplicationId(UUID.randomUUID().toString())
-          .messageGroupId(account.getNumber())
+          .messageGroupId(number)
          .messageAttributes(Map.of(
-              "id", MessageAttributeValue.builder().dataType("String").stringValue(account.getNumber()).build(),
-              "uuid", MessageAttributeValue.builder().dataType("String").stringValue(account.getUuid().toString()).build(),
+              "id", MessageAttributeValue.builder().dataType("String").stringValue(number).build(),
+              "uuid", MessageAttributeValue.builder().dataType("String").stringValue(uuid.toString()).build(),
              "action", action.toMessageAttributeValue()
          ))
          .build();
--- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java
@@ -12,6 +12,7 @@ import io.micrometer.core.instrument.Metrics;
 import io.micrometer.core.instrument.Timer;
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -35,6 +36,7 @@ import software.amazon.awssdk.services.dynamodb.model.TransactWriteItem;
 import software.amazon.awssdk.services.dynamodb.model.TransactWriteItemsRequest;
 import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException;
 import software.amazon.awssdk.services.dynamodb.model.TransactionConflictException;
+import software.amazon.awssdk.services.dynamodb.model.Update;
 import software.amazon.awssdk.services.dynamodb.model.UpdateItemRequest;
 import software.amazon.awssdk.services.dynamodb.model.UpdateItemResponse;

@@ -59,6 +61,7 @@ public class Accounts extends AbstractDynamoDbStore {
  private final int scanPageSize;

  private static final Timer CREATE_TIMER = Metrics.timer(name(Accounts.class, "create"));
+  private static final Timer CHANGE_NUMBER_TIMER = Metrics.timer(name(Accounts.class, "changeNumber"));
  private static final Timer UPDATE_TIMER = Metrics.timer(name(Accounts.class, "update"));
  private static final Timer GET_BY_NUMBER_TIMER = Metrics.timer(name(Accounts.class, "getByNumber"));
  private static final Timer GET_BY_UUID_TIMER = Metrics.timer(name(Accounts.class, "getByUuid"));
@@ -167,6 +170,85 @@ public class Accounts extends AbstractDynamoDbStore {
        .build();
  }

+  /**
+   * Changes the phone number for the given account. The given account's number should be its current, pre-change
+   * number. If this method succeeds, the account's number will be changed to the new number. If the update fails for
+   * any reason, the account's number will be unchanged.
+   * <p/>
+   * This method expects that any accounts with conflicting numbers will have been removed by the time this method is
+   * called. This method may fail with an unspecified {@link RuntimeException} if another account with the same number
+   * exists in the data store.
+   *
+   * @param account the account for which to change the phone number
+   * @param number the new phone number
+   */
+  public void changeNumber(final Account account, final String number) {
+    CHANGE_NUMBER_TIMER.record(() -> {
+      final String originalNumber = account.getNumber();
+      boolean succeeded = false;
+
+      account.setNumber(number);
+
+      try {
+        final List<TransactWriteItem> writeItems = new ArrayList<>();
+
+        writeItems.add(TransactWriteItem.builder()
+            .delete(Delete.builder()
+                .tableName(phoneNumbersTableName)
+                .key(Map.of(ATTR_ACCOUNT_E164, AttributeValues.fromString(originalNumber)))
+                .build())
+            .build());
+
+        writeItems.add(TransactWriteItem.builder()
+            .put(Put.builder()
+                .tableName(phoneNumbersTableName)
+                .item(Map.of(
+                    KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()),
+                    ATTR_ACCOUNT_E164, AttributeValues.fromString(number)))
+                .conditionExpression("attribute_not_exists(#number)")
+                .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164))
+                .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD)
+                .build())
+            .build());
+
+        writeItems.add(
+            TransactWriteItem.builder()
+                .update(Update.builder()
+                    .tableName(accountsTableName)
+                    .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid())))
+                    .updateExpression("SET #data = :data, #number = :number, #cds = :cds ADD #version :version_increment")
+                    .conditionExpression("attribute_exists(#number) AND #version = :version")
+                    .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164,
+                        "#data", ATTR_ACCOUNT_DATA,
+                        "#cds", ATTR_CANONICALLY_DISCOVERABLE,
+                        "#version", ATTR_VERSION))
+                    .expressionAttributeValues(Map.of(
+                        ":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)),
+                        ":number", AttributeValues.fromString(number),
+                        ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()),
+                        ":version", AttributeValues.fromInt(account.getVersion()),
+                        ":version_increment", AttributeValues.fromInt(1)))
+                    .build())
+                .build());
+
+        final TransactWriteItemsRequest request = TransactWriteItemsRequest.builder()
+            .transactItems(writeItems)
+            .build();
+
+        client.transactWriteItems(request);
+
+        account.setVersion(account.getVersion() + 1);
+        succeeded = true;
+      } catch (final JsonProcessingException e) {
+        throw new IllegalArgumentException(e);
+      } finally {
+        if (!succeeded) {
+          account.setNumber(originalNumber);
+        }
+      }
+    });
+  }
+
  public void update(Account account) throws ContestedOptimisticLockException {
    UPDATE_TIMER.record(() -> {
      UpdateItemRequest updateItemRequest;
--- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java
@@ -21,6 +21,7 @@ import java.io.IOException;
 import java.util.Optional;
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
@@ -196,6 +197,45 @@ public class AccountsManager {
    }
  }

+  public Account changeNumber(final Account account, final String number) throws InterruptedException {
+    final String originalNumber = account.getNumber();
+
+    if (originalNumber.equals(number)) {
+      return account;
+    }
+
+    final AtomicReference<Account> updatedAccount = new AtomicReference<>();
+
+    deletedAccountsManager.lockAndPut(account.getNumber(), number, () -> {
+      redisDelete(account);
+
+      final Optional<Account> maybeExistingAccount = get(number);
+      final Optional<UUID> displacedUuid;
+
+      if (maybeExistingAccount.isPresent()) {
+        delete(maybeExistingAccount.get());
+        displacedUuid = maybeExistingAccount.map(Account::getUuid);
+      } else {
+        displacedUuid = Optional.empty();
+      }
+
+      final UUID uuid = account.getUuid();
+
+      final Account numberChangedAccount = updateWithRetries(
+          account,
+          a -> true,
+          a -> dynamoChangeNumber(a, number),
+          () -> dynamoGet(uuid).orElseThrow());
+
+      updatedAccount.set(numberChangedAccount);
+      directoryQueue.changePhoneNumber(numberChangedAccount, originalNumber, number);
+
+      return displacedUuid;
+    });
+
+    return updatedAccount.get();
+  }
+
  public Account update(Account account, Consumer<Account> updater) {

    return update(account, a -> {
@@ -243,9 +283,17 @@ public class AccountsManager {
      redisDelete(account);

      final UUID uuid = account.getUuid();
+      final String originalNumber = account.getNumber();

      updatedAccount = updateWithRetries(account, updater, this::dynamoUpdate, () -> dynamoGet(uuid).get());

+      assert updatedAccount.getNumber().equals(originalNumber);
+
+      if (!updatedAccount.getNumber().equals(originalNumber)) {
+        logger.error("Account number changed via \"normal\" update; numbers must be changed via changeNumber method",
+            new RuntimeException());
+      }
+
      redisSet(updatedAccount);
    }

@@ -343,24 +391,8 @@ public class AccountsManager {
  public void delete(final Account account, final DeletionReason deletionReason) throws InterruptedException {
    try (final Timer.Context ignored = deleteTimer.time()) {
      deletedAccountsManager.lockAndPut(account.getNumber(), () -> {
-        final CompletableFuture<Void> deleteStorageServiceDataFuture = secureStorageClient.deleteStoredData(account.getUuid());
-        final CompletableFuture<Void> deleteBackupServiceDataFuture = secureBackupClient.deleteBackups(account.getUuid());
-
-        usernamesManager.delete(account.getUuid());
+        delete(account);
        directoryQueue.deleteAccount(account);
-        profilesManager.deleteAll(account.getUuid());
-        keysDynamoDb.delete(account.getUuid());
-        messagesManager.clear(account.getUuid());
-
-        deleteStorageServiceDataFuture.join();
-        deleteBackupServiceDataFuture.join();
-
-        redisDelete(account);
-        dynamoDelete(account);
-
-        RedisOperation.unchecked(() ->
-            account.getDevices().forEach(device ->
-                clientPresenceManager.displacePresence(account.getUuid(), device.getId())));

        return account.getUuid();
      });
@@ -375,6 +407,26 @@ public class AccountsManager {
        .increment();
  }

+  private void delete(final Account account) {
+    final CompletableFuture<Void> deleteStorageServiceDataFuture = secureStorageClient.deleteStoredData(account.getUuid());
+    final CompletableFuture<Void> deleteBackupServiceDataFuture = secureBackupClient.deleteBackups(account.getUuid());
+
+    usernamesManager.delete(account.getUuid());
+    profilesManager.deleteAll(account.getUuid());
+    keysDynamoDb.delete(account.getUuid());
+    messagesManager.clear(account.getUuid());
+
+    deleteStorageServiceDataFuture.join();
+    deleteBackupServiceDataFuture.join();
+
+    redisDelete(account);
+    dynamoDelete(account);
+
+    RedisOperation.unchecked(() ->
+        account.getDevices().forEach(device ->
+            clientPresenceManager.displacePresence(account.getUuid(), device.getId())));
+  }
+
  private String getAccountMapKey(String number) {
    return "AccountMap::" + number;
  }
@@ -461,4 +513,7 @@ public class AccountsManager {
    accounts.delete(account.getUuid());
  }

+  private void dynamoChangeNumber(final Account account, final String number) {
+    accounts.changeNumber(account, number);
+  }
 }
--- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManager.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManager.java
@@ -61,38 +61,92 @@ public class DeletedAccountsManager {
            .build());
  }

+  /**
+   * Acquires a pessimistic lock for the given phone number and performs the given action, passing the UUID of the
+   * recently-deleted account (if any) that previously held the given number.
+   *
+   * @param e164 the phone number to lock and with which to perform an action
+   * @param consumer the action to take; accepts the UUID of the account that previously held the given e164, if any,
+   * as an argument
+   *
+   * @throws InterruptedException if interrupted while waiting to acquire a lock on the given phone number
+   */
  public void lockAndTake(final String e164, final Consumer<Optional<UUID>> consumer) throws InterruptedException {
-    withLock(e164, () -> {
+    withLock(List.of(e164), () -> {
      try {
        consumer.accept(deletedAccounts.findUuid(e164));
        deletedAccounts.remove(e164);
      } catch (final Exception e) {
        log.warn("Consumer threw an exception while holding lock on a deleted account record", e);
+        throw new RuntimeException(e);
      }
    });
  }

+  /**
+   * Acquires a pessimistic lock for the given phone number and performs an action that deletes an account, returning
+   * the UUID of the deleted account. The UUID of the deleted account will be stored in the list of recently-deleted
+   * e164-to-UUID mappings.
+   *
+   * @param e164 the phone number to lock and with which to perform an action
+   * @param supplier the deletion action to take on the account associated with the given number; must return the UUID
+   * of the deleted account
+   *
+   * @throws InterruptedException if interrupted while waiting to acquire a lock on the given phone number
+   */
  public void lockAndPut(final String e164, final Supplier<UUID> supplier) throws InterruptedException {
-    withLock(e164, () -> {
+    withLock(List.of(e164), () -> {
      try {
        deletedAccounts.put(supplier.get(), e164, true);
      } catch (final Exception e) {
        log.warn("Supplier threw an exception while holding lock on a deleted account record", e);
+        throw new RuntimeException(e);
      }
    });
  }

-  private void withLock(final String e164, final Runnable task) throws InterruptedException {
-    final LockItem lockItem = lockClient.acquireLock(AcquireLockOptions.builder(e164)
-        .withAcquireReleasedLocksConsistently(true)
-        .build());
+  /**
+   * Acquires a pessimistic lock for the given phone numbers and performs an action that may or may not delete an
+   * account associated with the target number. The UUID of the deleted account (if any) will be stored in the list of
+   * recently-deleted e164-to-UUID mappings.
+   *
+   * @param original the phone number of an existing account to lock and with which to perform an action
+   * @param target the phone number of an account that may or may not exist with which to perform an action
+   * @param supplier the action to take on the given phone numbers; the action may delete the account identified by the
+   * target number, in which case it must return the UUID of that account
+   *
+   * @throws InterruptedException if interrupted while waiting to acquire a lock on the given phone numbers
+   */
+  public void lockAndPut(final String original, final String target, final Supplier<Optional<UUID>> supplier)
+      throws InterruptedException {
+
+    withLock(List.of(original, target), () -> {
+      try {
+        supplier.get().ifPresent(uuid -> deletedAccounts.put(uuid, original, true));
+      } catch (final Exception e) {
+        log.warn("Supplier threw an exception while holding lock on a deleted account record", e);
+        throw new RuntimeException(e);
+      }
+    });
+  }
+
+  private void withLock(final List<String> e164s, final Runnable task) throws InterruptedException {
+    final List<LockItem> lockItems = new ArrayList<>(e164s.size());

    try {
+      for (final String e164 : e164s) {
+        lockItems.add(lockClient.acquireLock(AcquireLockOptions.builder(e164)
+            .withAcquireReleasedLocksConsistently(true)
+            .build()));
+      }
+
      task.run();
    } finally {
-      lockClient.releaseLock(ReleaseLockOptions.builder(lockItem)
-          .withBestEffort(true)
-          .build());
+      for (final LockItem lockItem : lockItems) {
+        lockClient.releaseLock(ReleaseLockOptions.builder(lockItem)
+            .withBestEffort(true)
+            .build());
+      }
    }
  }