From ba58a95a0f98c5d06edc090dbd273341a42eef62 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Mon, 2 Aug 2021 16:41:10 -0400 Subject: [PATCH] Add support for changing phone numbers --- .../controllers/AccountController.java | 84 ++++-- .../entities/ChangePhoneNumberRequest.java | 49 ++++ .../textsecuregcm/sqs/DirectoryQueue.java | 18 +- .../textsecuregcm/storage/Accounts.java | 82 ++++++ .../storage/AccountsManager.java | 89 ++++-- .../storage/DeletedAccountsManager.java | 72 ++++- ...ntsManagerChangeNumberIntegrationTest.java | 261 ++++++++++++++++++ .../textsecuregcm/storage/AccountsTest.java | 53 ++++ .../storage/DeletedAccountsManagerTest.java | 7 +- .../controllers/AccountControllerTest.java | 204 ++++++++++++++ .../tests/storage/AccountsManagerTest.java | 94 ++++++- 11 files changed, 947 insertions(+), 66 deletions(-) create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangePhoneNumberRequest.java create mode 100644 service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index d3dc2b2fe..96c7f0ec8 100644 --- 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 existingAccount = accounts.get(number); - Optional existingRegistrationLock = existingAccount.map(Account::getRegistrationLock); - Optional existingBackupCredentials = existingAccount.map(Account::getUuid) - .map(uuid -> backupServiceCredentialGenerator.generateFor(uuid.toString())); + Optional 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 = + 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 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 captchaToken, diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangePhoneNumberRequest.java b/service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangePhoneNumberRequest.java new file mode 100644 index 000000000..088a4d554 --- /dev/null +++ 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; + } +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/sqs/DirectoryQueue.java b/service/src/main/java/org/whispersystems/textsecuregcm/sqs/DirectoryQueue.java index 65c0d51e0..0887efeea 100644 --- 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(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java index fddd558d1..3b5389f7e 100644 --- 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. + *

+ * 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 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; diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index ae5fd5f5f..2b28de3c1 100644 --- 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 updatedAccount = new AtomicReference<>(); + + deletedAccountsManager.lockAndPut(account.getNumber(), number, () -> { + redisDelete(account); + + final Optional maybeExistingAccount = get(number); + final Optional 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 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 deleteStorageServiceDataFuture = secureStorageClient.deleteStoredData(account.getUuid()); - final CompletableFuture 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 deleteStorageServiceDataFuture = secureStorageClient.deleteStoredData(account.getUuid()); + final CompletableFuture 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); + } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManager.java index 2bb39114c..1d1b54ee4 100644 --- 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> 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 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> 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 e164s, final Runnable task) throws InterruptedException { + final List 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()); + } } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java new file mode 100644 index 000000000..1c6b5ee89 --- /dev/null +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java @@ -0,0 +1,261 @@ +/* + * Copyright 2013-2021 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.storage; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.opentable.db.postgres.embedded.LiquibasePreparer; +import com.opentable.db.postgres.junit5.EmbeddedPostgresExtension; +import com.opentable.db.postgres.junit5.PreparedDbExtension; +import java.util.Optional; +import java.util.UUID; +import java.util.concurrent.CompletableFuture; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; +import org.whispersystems.textsecuregcm.entities.AccountAttributes; +import org.whispersystems.textsecuregcm.push.ClientPresenceManager; +import org.whispersystems.textsecuregcm.redis.RedisClusterExtension; +import org.whispersystems.textsecuregcm.securebackup.SecureBackupClient; +import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; +import org.whispersystems.textsecuregcm.sqs.DirectoryQueue; +import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities; +import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition; +import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest; +import software.amazon.awssdk.services.dynamodb.model.GlobalSecondaryIndex; +import software.amazon.awssdk.services.dynamodb.model.KeySchemaElement; +import software.amazon.awssdk.services.dynamodb.model.KeyType; +import software.amazon.awssdk.services.dynamodb.model.Projection; +import software.amazon.awssdk.services.dynamodb.model.ProjectionType; +import software.amazon.awssdk.services.dynamodb.model.ProvisionedThroughput; +import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType; + +class AccountsManagerChangeNumberIntegrationTest { + + @RegisterExtension + static PreparedDbExtension ACCOUNTS_POSTGRES_EXTENSION = + EmbeddedPostgresExtension.preparedDatabase(LiquibasePreparer.forClasspathLocation("accountsdb.xml")); + + private static final String ACCOUNTS_TABLE_NAME = "accounts_test"; + private static final String NUMBERS_TABLE_NAME = "numbers_test"; + private static final String NEEDS_RECONCILIATION_INDEX_NAME = "needs_reconciliation_test"; + private static final String DELETED_ACCOUNTS_LOCK_TABLE_NAME = "deleted_accounts_lock_test"; + private static final int SCAN_PAGE_SIZE = 1; + + @RegisterExtension + static DynamoDbExtension ACCOUNTS_DYNAMO_EXTENSION = DynamoDbExtension.builder() + .tableName(ACCOUNTS_TABLE_NAME) + .hashKey(Accounts.KEY_ACCOUNT_UUID) + .attributeDefinition(AttributeDefinition.builder() + .attributeName(Accounts.KEY_ACCOUNT_UUID) + .attributeType(ScalarAttributeType.B) + .build()) + .build(); + + @RegisterExtension + static DynamoDbExtension DELETED_ACCOUNTS_DYNAMO_EXTENSION = DynamoDbExtension.builder() + .tableName("deleted_accounts_test") + .hashKey(DeletedAccounts.KEY_ACCOUNT_E164) + .attributeDefinition(AttributeDefinition.builder() + .attributeName(DeletedAccounts.KEY_ACCOUNT_E164) + .attributeType(ScalarAttributeType.S).build()) + .attributeDefinition(AttributeDefinition.builder() + .attributeName(DeletedAccounts.ATTR_NEEDS_CDS_RECONCILIATION) + .attributeType(ScalarAttributeType.N) + .build()) + .globalSecondaryIndex(GlobalSecondaryIndex.builder() + .indexName(NEEDS_RECONCILIATION_INDEX_NAME) + .keySchema(KeySchemaElement.builder().attributeName(DeletedAccounts.KEY_ACCOUNT_E164).keyType(KeyType.HASH).build(), + KeySchemaElement.builder().attributeName(DeletedAccounts.ATTR_NEEDS_CDS_RECONCILIATION).keyType(KeyType.RANGE).build()) + .projection(Projection.builder().projectionType(ProjectionType.INCLUDE).nonKeyAttributes(DeletedAccounts.ATTR_ACCOUNT_UUID).build()) + .provisionedThroughput(ProvisionedThroughput.builder().readCapacityUnits(10L).writeCapacityUnits(10L).build()) + .build()) + .build(); + + @RegisterExtension + static DynamoDbExtension DELETED_ACCOUNTS_LOCK_DYNAMO_EXTENSION = DynamoDbExtension.builder() + .tableName(DELETED_ACCOUNTS_LOCK_TABLE_NAME) + .hashKey(DeletedAccounts.KEY_ACCOUNT_E164) + .attributeDefinition(AttributeDefinition.builder() + .attributeName(DeletedAccounts.KEY_ACCOUNT_E164) + .attributeType(ScalarAttributeType.S).build()) + .build(); + + @RegisterExtension + static RedisClusterExtension CACHE_CLUSTER_EXTENSION = RedisClusterExtension.builder().build(); + + private ClientPresenceManager clientPresenceManager; + private DeletedAccounts deletedAccounts; + + private AccountsManager accountsManager; + + @BeforeEach + void setup() throws InterruptedException { + + { + CreateTableRequest createNumbersTableRequest = CreateTableRequest.builder() + .tableName(NUMBERS_TABLE_NAME) + .keySchema(KeySchemaElement.builder() + .attributeName(Accounts.ATTR_ACCOUNT_E164) + .keyType(KeyType.HASH) + .build()) + .attributeDefinitions(AttributeDefinition.builder() + .attributeName(Accounts.ATTR_ACCOUNT_E164) + .attributeType(ScalarAttributeType.S) + .build()) + .provisionedThroughput(DynamoDbExtension.DEFAULT_PROVISIONED_THROUGHPUT) + .build(); + + ACCOUNTS_DYNAMO_EXTENSION.getDynamoDbClient().createTable(createNumbersTableRequest); + } + + final Accounts accounts = new Accounts( + ACCOUNTS_DYNAMO_EXTENSION.getDynamoDbClient(), + ACCOUNTS_DYNAMO_EXTENSION.getTableName(), + NUMBERS_TABLE_NAME, + SCAN_PAGE_SIZE); + + { + final DynamicConfigurationManager dynamicConfigurationManager = mock(DynamicConfigurationManager.class); + + DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(); + when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration); + + deletedAccounts = new DeletedAccounts(DELETED_ACCOUNTS_DYNAMO_EXTENSION.getDynamoDbClient(), + DELETED_ACCOUNTS_DYNAMO_EXTENSION.getTableName(), + NEEDS_RECONCILIATION_INDEX_NAME); + + final DeletedAccountsManager deletedAccountsManager = new DeletedAccountsManager(deletedAccounts, + DELETED_ACCOUNTS_LOCK_DYNAMO_EXTENSION.getLegacyDynamoClient(), + DELETED_ACCOUNTS_LOCK_DYNAMO_EXTENSION.getTableName()); + + final SecureStorageClient secureStorageClient = mock(SecureStorageClient.class); + when(secureStorageClient.deleteStoredData(any())).thenReturn(CompletableFuture.completedFuture(null)); + + final SecureBackupClient secureBackupClient = mock(SecureBackupClient.class); + when(secureBackupClient.deleteBackups(any())).thenReturn(CompletableFuture.completedFuture(null)); + + clientPresenceManager = mock(ClientPresenceManager.class); + + accountsManager = new AccountsManager( + accounts, + CACHE_CLUSTER_EXTENSION.getRedisCluster(), + deletedAccountsManager, + mock(DirectoryQueue.class), + mock(KeysDynamoDb.class), + mock(MessagesManager.class), + mock(UsernamesManager.class), + mock(ProfilesManager.class), + mock(StoredVerificationCodeManager.class), + secureStorageClient, + secureBackupClient, + clientPresenceManager); + } + } + + @Test + void testChangeNumber() throws InterruptedException { + final String originalNumber = "+18005551111"; + final String secondNumber = "+18005552222"; + + final Account account = accountsManager.create(originalNumber, "password", null, new AccountAttributes()); + final UUID originalUuid = account.getUuid(); + + accountsManager.changeNumber(account, secondNumber); + + assertTrue(accountsManager.get(originalNumber).isEmpty()); + + assertTrue(accountsManager.get(secondNumber).isPresent()); + assertEquals(Optional.of(originalUuid), accountsManager.get(secondNumber).map(Account::getUuid)); + + assertEquals(secondNumber, accountsManager.get(originalUuid).map(Account::getNumber).orElseThrow()); + + assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); + assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + } + + @Test + void testChangeNumberReturnToOriginal() throws InterruptedException { + final String originalNumber = "+18005551111"; + final String secondNumber = "+18005552222"; + + Account account = accountsManager.create(originalNumber, "password", null, new AccountAttributes()); + final UUID originalUuid = account.getUuid(); + + account = accountsManager.changeNumber(account, secondNumber); + accountsManager.changeNumber(account, originalNumber); + + assertTrue(accountsManager.get(originalNumber).isPresent()); + assertEquals(Optional.of(originalUuid), accountsManager.get(originalNumber).map(Account::getUuid)); + + assertTrue(accountsManager.get(secondNumber).isEmpty()); + + assertEquals(originalNumber, accountsManager.get(originalUuid).map(Account::getNumber).orElseThrow()); + + assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); + assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + } + + @Test + void testChangeNumberContested() throws InterruptedException { + final String originalNumber = "+18005551111"; + final String secondNumber = "+18005552222"; + + final Account account = accountsManager.create(originalNumber, "password", null, new AccountAttributes()); + final UUID originalUuid = account.getUuid(); + + final Account existingAccount = accountsManager.create(secondNumber, "password", null, new AccountAttributes()); + final UUID existingAccountUuid = existingAccount.getUuid(); + + accountsManager.update(existingAccount, a -> a.addDevice(new Device(Device.MASTER_ID, "test", "token", "salt", null, null, null, true, 1, null, 0, 0, null, 0, new DeviceCapabilities()))); + + accountsManager.changeNumber(account, secondNumber); + + assertTrue(accountsManager.get(originalNumber).isEmpty()); + + assertTrue(accountsManager.get(secondNumber).isPresent()); + assertEquals(Optional.of(originalUuid), accountsManager.get(secondNumber).map(Account::getUuid)); + + assertEquals(secondNumber, accountsManager.get(originalUuid).map(Account::getNumber).orElseThrow()); + + verify(clientPresenceManager).displacePresence(existingAccountUuid, Device.MASTER_ID); + + assertEquals(Optional.of(existingAccountUuid), deletedAccounts.findUuid(originalNumber)); + assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + } + + @Test + void testChangeNumberChaining() throws InterruptedException { + final String originalNumber = "+18005551111"; + final String secondNumber = "+18005552222"; + + final Account account = accountsManager.create(originalNumber, "password", null, new AccountAttributes()); + final UUID originalUuid = account.getUuid(); + + final Account existingAccount = accountsManager.create(secondNumber, "password", null, new AccountAttributes()); + final UUID existingAccountUuid = existingAccount.getUuid(); + + accountsManager.changeNumber(account, secondNumber); + + final Account reRegisteredAccount = accountsManager.create(originalNumber, "password", null, new AccountAttributes()); + + assertEquals(existingAccountUuid, reRegisteredAccount.getUuid()); + + assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); + assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + + accountsManager.changeNumber(reRegisteredAccount, secondNumber); + + assertEquals(Optional.of(originalUuid), deletedAccounts.findUuid(originalNumber)); + assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + } +} diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java index 5d6430805..7073e6df6 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -7,6 +7,7 @@ package org.whispersystems.textsecuregcm.storage; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -24,6 +25,11 @@ import java.util.Optional; import java.util.Random; import java.util.Set; import java.util.UUID; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.LinkedBlockingDeque; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; +import org.assertj.core.api.AssertionsForClassTypes; import org.jdbi.v3.core.transaction.TransactionException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; @@ -42,6 +48,7 @@ import software.amazon.awssdk.services.dynamodb.model.KeySchemaElement; import software.amazon.awssdk.services.dynamodb.model.KeyType; import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType; 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.UpdateItemRequest; @@ -412,6 +419,52 @@ class AccountsTest { verifyStoredState("+14151112222", account.getUuid(), account, false); } + @Test + public void testChangeNumber() { + final String originalNumber = "+14151112222"; + final String targetNumber = "+14151113333"; + + final Device device = generateDevice(1); + final Account account = generateAccount(originalNumber, UUID.randomUUID(), Collections.singleton(device)); + + accounts.create(account); + + { + final Optional retrieved = accounts.get(originalNumber); + assertThat(retrieved).isPresent(); + + verifyStoredState(originalNumber, account.getUuid(), retrieved.get(), account); + } + + accounts.changeNumber(account, targetNumber); + + assertThat(accounts.get(originalNumber)).isEmpty(); + + { + final Optional retrieved = accounts.get(targetNumber); + assertThat(retrieved).isPresent(); + + verifyStoredState(targetNumber, account.getUuid(), retrieved.get(), account); + } + } + + @Test + public void testChangeNumberConflict() { + final String originalNumber = "+14151112222"; + final String targetNumber = "+14151113333"; + + final Device existingDevice = generateDevice(1); + final Account existingAccount = generateAccount(targetNumber, UUID.randomUUID(), Collections.singleton(existingDevice)); + + final Device device = generateDevice(1); + final Account account = generateAccount(originalNumber, UUID.randomUUID(), Collections.singleton(device)); + + accounts.create(account); + accounts.create(existingAccount); + + assertThrows(TransactionCanceledException.class, () -> accounts.changeNumber(account, targetNumber)); + } + private Device generateDevice(long id) { Random random = new Random(System.currentTimeMillis()); SignedPreKey signedPreKey = new SignedPreKey(random.nextInt(), "testPublicKey-" + random.nextInt(), "testSignature-" + random.nextInt()); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManagerTest.java index afe8d851d..1d706bac3 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManagerTest.java @@ -26,6 +26,7 @@ import java.util.UUID; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; class DeletedAccountsManagerTest { @@ -85,16 +86,16 @@ class DeletedAccountsManagerTest { } @Test - void testLockAndTakeWithException() throws InterruptedException { + void testLockAndTakeWithException() { final UUID uuid = UUID.randomUUID(); final String e164 = "+18005551234"; deletedAccounts.put(uuid, e164, true); - deletedAccountsManager.lockAndTake(e164, maybeUuid -> { + assertThrows(RuntimeException.class, () -> deletedAccountsManager.lockAndTake(e164, maybeUuid -> { assertEquals(Optional.of(uuid), maybeUuid); throw new RuntimeException("OH NO"); - }); + })); assertEquals(Optional.of(uuid), deletedAccounts.findUuid(e164)); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java index ee2c80449..4f1060fdd 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java @@ -66,6 +66,7 @@ import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; 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.GcmRegistrationId; import org.whispersystems.textsecuregcm.entities.RegistrationLock; import org.whispersystems.textsecuregcm.entities.RegistrationLockFailure; @@ -105,6 +106,7 @@ class AccountControllerTest { private static final String SENDER_TRANSFER = "+14151111112"; private static final UUID SENDER_REG_LOCK_UUID = UUID.randomUUID(); + private static final UUID SENDER_TRANSFER_UUID = UUID.randomUUID(); private static final String ABUSIVE_HOST = "192.168.1.1"; private static final String RESTRICTED_HOST = "192.168.1.2"; @@ -200,6 +202,11 @@ class AccountControllerTest { when(senderRegLockAccount.getRegistrationLock()).thenReturn(new StoredRegistrationLock(Optional.of(registrationLockCredentials.getHashedAuthenticationToken()), Optional.of(registrationLockCredentials.getSalt()), System.currentTimeMillis())); when(senderRegLockAccount.getLastSeen()).thenReturn(System.currentTimeMillis()); when(senderRegLockAccount.getUuid()).thenReturn(SENDER_REG_LOCK_UUID); + when(senderRegLockAccount.getNumber()).thenReturn(SENDER_REG_LOCK); + + when(senderTransfer.getRegistrationLock()).thenReturn(new StoredRegistrationLock(Optional.empty(), Optional.empty(), System.currentTimeMillis())); + when(senderTransfer.getUuid()).thenReturn(SENDER_TRANSFER_UUID); + when(senderTransfer.getNumber()).thenReturn(SENDER_TRANSFER); when(pendingAccountsManager.getCodeForNumber(SENDER)).thenReturn(Optional.of(new StoredVerificationCode("1234", System.currentTimeMillis(), "1234-push", null))); when(pendingAccountsManager.getCodeForNumber(SENDER_OLD)).thenReturn(Optional.empty()); @@ -1124,6 +1131,203 @@ class AccountControllerTest { assertThat(response.getStatus()).isEqualTo(200); } + @Test + void testChangePhoneNumber() throws InterruptedException { + final String number = "+18005559876"; + final String code = "987654"; + + when(pendingAccountsManager.getCodeForNumber(number)).thenReturn(Optional.of( + new StoredVerificationCode(code, System.currentTimeMillis(), "push", null))); + + 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(204); + verify(accountsManager).changeNumber(AuthHelper.VALID_ACCOUNT, number); + } + + @Test + void testChangePhoneNumberSameNumber() throws InterruptedException { + 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(AuthHelper.VALID_NUMBER, "567890", null), + MediaType.APPLICATION_JSON_TYPE)); + + assertThat(response.getStatus()).isEqualTo(204); + verify(accountsManager, never()).changeNumber(eq(AuthHelper.VALID_ACCOUNT), any()); + } + + @Test + void testChangePhoneNumberNoPendingCode() throws InterruptedException { + final String number = "+18005559876"; + final String code = "987654"; + + when(pendingAccountsManager.getCodeForNumber(number)).thenReturn(Optional.empty()); + + 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(403); + verify(accountsManager, never()).changeNumber(eq(AuthHelper.VALID_ACCOUNT), any()); + } + + @Test + void testChangePhoneNumberIncorrectCode() throws InterruptedException { + final String number = "+18005559876"; + final String code = "987654"; + + when(pendingAccountsManager.getCodeForNumber(number)).thenReturn(Optional.of( + new StoredVerificationCode(code, System.currentTimeMillis(), "push", null))); + + 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 + "-incorrect", null), + MediaType.APPLICATION_JSON_TYPE)); + + assertThat(response.getStatus()).isEqualTo(403); + verify(accountsManager, never()).changeNumber(eq(AuthHelper.VALID_ACCOUNT), any()); + } + + @Test + void testChangePhoneNumberExistingAccountReglockNotRequired() throws InterruptedException { + final String number = "+18005559876"; + final String code = "987654"; + + when(pendingAccountsManager.getCodeForNumber(number)).thenReturn(Optional.of( + new StoredVerificationCode(code, System.currentTimeMillis(), "push", null))); + + final StoredRegistrationLock existingRegistrationLock = mock(StoredRegistrationLock.class); + when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(false); + + final Account existingAccount = mock(Account.class); + when(existingAccount.getNumber()).thenReturn(number); + when(existingAccount.getUuid()).thenReturn(UUID.randomUUID()); + when(existingAccount.getRegistrationLock()).thenReturn(existingRegistrationLock); + + when(accountsManager.get(number)).thenReturn(Optional.of(existingAccount)); + + 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(204); + verify(accountsManager).changeNumber(eq(AuthHelper.VALID_ACCOUNT), any()); + } + + @Test + void testChangePhoneNumberExistingAccountReglockRequiredNotProvided() throws InterruptedException { + final String number = "+18005559876"; + final String code = "987654"; + + when(pendingAccountsManager.getCodeForNumber(number)).thenReturn(Optional.of( + new StoredVerificationCode(code, System.currentTimeMillis(), "push", null))); + + final StoredRegistrationLock existingRegistrationLock = mock(StoredRegistrationLock.class); + when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(true); + + final Account existingAccount = mock(Account.class); + when(existingAccount.getNumber()).thenReturn(number); + when(existingAccount.getUuid()).thenReturn(UUID.randomUUID()); + when(existingAccount.getRegistrationLock()).thenReturn(existingRegistrationLock); + + when(accountsManager.get(number)).thenReturn(Optional.of(existingAccount)); + + 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(423); + verify(accountsManager, never()).changeNumber(eq(AuthHelper.VALID_ACCOUNT), any()); + } + + @Test + void testChangePhoneNumberExistingAccountReglockRequiredIncorrect() throws InterruptedException { + final String number = "+18005559876"; + final String code = "987654"; + final String reglock = "setec-astronomy"; + + when(pendingAccountsManager.getCodeForNumber(number)).thenReturn(Optional.of( + new StoredVerificationCode(code, System.currentTimeMillis(), "push", null))); + + final StoredRegistrationLock existingRegistrationLock = mock(StoredRegistrationLock.class); + when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(true); + when(existingRegistrationLock.verify(anyString())).thenReturn(false); + + final Account existingAccount = mock(Account.class); + when(existingAccount.getNumber()).thenReturn(number); + when(existingAccount.getUuid()).thenReturn(UUID.randomUUID()); + when(existingAccount.getRegistrationLock()).thenReturn(existingRegistrationLock); + + when(accountsManager.get(number)).thenReturn(Optional.of(existingAccount)); + + 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, reglock), + MediaType.APPLICATION_JSON_TYPE)); + + assertThat(response.getStatus()).isEqualTo(423); + verify(accountsManager, never()).changeNumber(eq(AuthHelper.VALID_ACCOUNT), any()); + } + + @Test + void testChangePhoneNumberExistingAccountReglockRequiredCorrect() throws InterruptedException { + final String number = "+18005559876"; + final String code = "987654"; + final String reglock = "setec-astronomy"; + + when(pendingAccountsManager.getCodeForNumber(number)).thenReturn(Optional.of( + new StoredVerificationCode(code, System.currentTimeMillis(), "push", null))); + + final StoredRegistrationLock existingRegistrationLock = mock(StoredRegistrationLock.class); + when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(true); + when(existingRegistrationLock.verify(reglock)).thenReturn(true); + + final Account existingAccount = mock(Account.class); + when(existingAccount.getNumber()).thenReturn(number); + when(existingAccount.getUuid()).thenReturn(UUID.randomUUID()); + when(existingAccount.getRegistrationLock()).thenReturn(existingRegistrationLock); + + when(accountsManager.get(number)).thenReturn(Optional.of(existingAccount)); + + 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, reglock), + MediaType.APPLICATION_JSON_TYPE)); + + assertThat(response.getStatus()).isEqualTo(204); + verify(accountsManager).changeNumber(eq(AuthHelper.VALID_ACCOUNT), any()); + } + @Test void testSetRegistrationLock() { Response response = diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java index 535fb24a2..a9627bbd3 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java @@ -7,6 +7,7 @@ package org.whispersystems.textsecuregcm.tests.storage; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; @@ -27,7 +28,9 @@ import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands; import java.util.HashSet; import java.util.Optional; import java.util.UUID; +import java.util.concurrent.CompletableFuture; import java.util.function.Consumer; +import java.util.function.Supplier; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -36,7 +39,6 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import org.mockito.stubbing.Answer; -import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; import org.whispersystems.textsecuregcm.entities.AccountAttributes; import org.whispersystems.textsecuregcm.entities.SignedPreKey; import org.whispersystems.textsecuregcm.push.ClientPresenceManager; @@ -50,7 +52,6 @@ import org.whispersystems.textsecuregcm.storage.ContestedOptimisticLockException import org.whispersystems.textsecuregcm.storage.DeletedAccountsManager; import org.whispersystems.textsecuregcm.storage.Device; import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities; -import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; import org.whispersystems.textsecuregcm.storage.KeysDynamoDb; import org.whispersystems.textsecuregcm.storage.MessagesManager; import org.whispersystems.textsecuregcm.storage.ProfilesManager; @@ -63,7 +64,6 @@ class AccountsManagerTest { private Accounts accounts; private DeletedAccountsManager deletedAccountsManager; private DirectoryQueue directoryQueue; - private DynamicConfigurationManager dynamicConfigurationManager; private KeysDynamoDb keys; private MessagesManager messagesManager; private ProfilesManager profilesManager; @@ -84,7 +84,6 @@ class AccountsManagerTest { accounts = mock(Accounts.class); deletedAccountsManager = mock(DeletedAccountsManager.class); directoryQueue = mock(DirectoryQueue.class); - dynamicConfigurationManager = mock(DynamicConfigurationManager.class); keys = mock(KeysDynamoDb.class); messagesManager = mock(MessagesManager.class); profilesManager = mock(ProfilesManager.class); @@ -92,8 +91,14 @@ class AccountsManagerTest { //noinspection unchecked commands = mock(RedisAdvancedClusterCommands.class); - final DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(); - when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration); + doAnswer((Answer) invocation -> { + final Account account = invocation.getArgument(0, Account.class); + final String number = invocation.getArgument(1, String.class); + + account.setNumber(number); + + return null; + }).when(accounts).changeNumber(any(), anyString()); doAnswer(invocation -> { //noinspection unchecked @@ -101,6 +106,12 @@ class AccountsManagerTest { return null; }).when(deletedAccountsManager).lockAndTake(anyString(), any()); + final SecureStorageClient storageClient = mock(SecureStorageClient.class); + when(storageClient.deleteStoredData(any())).thenReturn(CompletableFuture.completedFuture(null)); + + final SecureBackupClient backupClient = mock(SecureBackupClient.class); + when(backupClient.deleteBackups(any())).thenReturn(CompletableFuture.completedFuture(null)); + accountsManager = new AccountsManager( accounts, RedisClusterHelper.buildMockRedisCluster(commands), @@ -111,10 +122,9 @@ class AccountsManagerTest { mock(UsernamesManager.class), profilesManager, mock(StoredVerificationCodeManager.class), - mock(SecureStorageClient.class), - mock(SecureBackupClient.class), - mock(ClientPresenceManager.class) - ); + storageClient, + backupClient, + mock(ClientPresenceManager.class)); } @Test @@ -159,7 +169,6 @@ class AccountsManagerTest { @Test void testGetAccountByNumberNotInCache() { - final boolean dynamoEnabled = true; UUID uuid = UUID.randomUUID(); Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); @@ -182,7 +191,6 @@ class AccountsManagerTest { @Test void testGetAccountByUuidNotInCache() { - final boolean dynamoEnabled = true; UUID uuid = UUID.randomUUID(); Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); @@ -458,4 +466,66 @@ class AccountsManagerTest { Arguments.of(false, 2, 1) ); } + + @Test + void testChangePhoneNumber() throws InterruptedException { + doAnswer(invocation -> invocation.getArgument(2, Supplier.class).get()) + .when(deletedAccountsManager).lockAndPut(anyString(), anyString(), any()); + + final String originalNumber = "+14152222222"; + final String targetNumber = "+14153333333"; + final UUID uuid = UUID.randomUUID(); + + Account account = new Account(originalNumber, uuid, new HashSet<>(), new byte[16]); + account = accountsManager.changeNumber(account, targetNumber); + + assertEquals(targetNumber, account.getNumber()); + + verify(directoryQueue).changePhoneNumber(argThat(a -> a.getUuid().equals(uuid)), eq(originalNumber), eq(targetNumber)); + } + + @Test + void testChangePhoneNumberSameNumber() throws InterruptedException { + final String number = "+14152222222"; + + Account account = new Account(number, UUID.randomUUID(), new HashSet<>(), new byte[16]); + account = accountsManager.changeNumber(account, number); + + assertEquals(number, account.getNumber()); + verify(deletedAccountsManager, never()).lockAndPut(anyString(), anyString(), any()); + verify(directoryQueue, never()).changePhoneNumber(any(), any(), any()); + } + + @Test + void testChangePhoneNumberExistingAccount() throws InterruptedException { + doAnswer(invocation -> invocation.getArgument(2, Supplier.class).get()) + .when(deletedAccountsManager).lockAndPut(anyString(), anyString(), any()); + + final String originalNumber = "+14152222222"; + final String targetNumber = "+14153333333"; + final UUID existingAccountUuid = UUID.randomUUID(); + final UUID uuid = UUID.randomUUID(); + + final Account existingAccount = new Account(targetNumber, existingAccountUuid, new HashSet<>(), new byte[16]); + when(accounts.get(targetNumber)).thenReturn(Optional.of(existingAccount)); + + Account account = new Account(originalNumber, uuid, new HashSet<>(), new byte[16]); + account = accountsManager.changeNumber(account, targetNumber); + + assertEquals(targetNumber, account.getNumber()); + + verify(directoryQueue).changePhoneNumber(argThat(a -> a.getUuid().equals(uuid)), eq(originalNumber), eq(targetNumber)); + verify(directoryQueue, never()).deleteAccount(any()); + } + + @Test + void testChangePhoneNumberViaUpdate() { + final String originalNumber = "+14152222222"; + final String targetNumber = "+14153333333"; + final UUID uuid = UUID.randomUUID(); + + final Account account = new Account(originalNumber, uuid, new HashSet<>(), new byte[16]); + + assertThrows(AssertionError.class, () -> accountsManager.update(account, a -> a.setNumber(targetNumber))); + } }