diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index e7630acbb..cd2c4d3ee 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -264,7 +264,6 @@ import org.whispersystems.textsecuregcm.workers.DeleteE164RegistrationRecoveryPa import org.whispersystems.textsecuregcm.workers.DeleteUserCommand; import org.whispersystems.textsecuregcm.workers.IdleDeviceNotificationSchedulerFactory; import org.whispersystems.textsecuregcm.workers.MessagePersisterServiceCommand; -import org.whispersystems.textsecuregcm.workers.MigrateDeletedAccountsCommand; import org.whispersystems.textsecuregcm.workers.NotifyIdleDevicesCommand; import org.whispersystems.textsecuregcm.workers.ProcessScheduledJobsServiceCommand; import org.whispersystems.textsecuregcm.workers.RemoveExpiredAccountsCommand; @@ -333,7 +332,6 @@ public class WhisperServerService extends Application sourceNumber; final Optional sourceAci; final Optional sourcePni; + if (source.startsWith("+")) { sourceNumber = Optional.of(source); final Optional maybeAccount = accountsManager.getByE164(source); @@ -864,8 +868,8 @@ public class MessageController { sourceAci = maybeAccount.map(Account::getUuid); sourcePni = maybeAccount.map(Account::getPhoneNumberIdentifier); } else { - sourceAci = accountsManager.findRecentlyDeletedAccountIdentifier(source); - sourcePni = Optional.ofNullable(accountsManager.getPhoneNumberIdentifier(source)); + sourcePni = Optional.ofNullable(phoneNumberIdentifiers.getPhoneNumberIdentifier(source).join()); + sourceAci = sourcePni.flatMap(accountsManager::findRecentlyDeletedAccountIdentifier); } } else { sourceAci = Optional.of(UUID.fromString(source)); @@ -874,8 +878,9 @@ public class MessageController { if (sourceAccount.isEmpty()) { logger.warn("Could not find source: {}", sourceAci.get()); - sourceNumber = accountsManager.findRecentlyDeletedE164(sourceAci.get()); - sourcePni = sourceNumber.map(accountsManager::getPhoneNumberIdentifier); + sourcePni = accountsManager.findRecentlyDeletedPhoneNumberIdentifier(sourceAci.get()); + sourceNumber = sourcePni.flatMap(pni -> + Util.getCanonicalNumber(phoneNumberIdentifiers.getPhoneNumber(pni).join())); } else { sourceNumber = sourceAccount.map(Account::getNumber); sourcePni = sourceAccount.map(Account::getPhoneNumberIdentifier); 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 8fe146bbe..9c6baf87f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -139,10 +139,13 @@ public class Accounts extends AbstractDynamoDbStore { // unidentified access key; byte[] or null static final String ATTR_UAK = "UAK"; - static final String DELETED_ACCOUNTS_KEY_ACCOUNT_E164 = "P"; + // For historical reasons, deleted-accounts PNI is stored as a string-format UUID rather than a + // compact byte array. + static final String DELETED_ACCOUNTS_KEY_ACCOUNT_PNI = "P"; + static final String DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID = "U"; static final String DELETED_ACCOUNTS_ATTR_EXPIRES = "E"; - static final String DELETED_ACCOUNTS_UUID_TO_E164_INDEX_NAME = "u_to_p"; + static final String DELETED_ACCOUNTS_UUID_TO_PNI_INDEX_NAME = "u_to_p"; static final String USERNAME_LINK_TO_UUID_INDEX = "ul_to_u"; @@ -1166,7 +1169,7 @@ public class Accounts extends AbstractDynamoDbStore { .put(Put.builder() .tableName(deletedAccountsTableName) .item(Map.of( - DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164), + DELETED_ACCOUNTS_KEY_ACCOUNT_PNI, AttributeValues.fromString(e164), DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, AttributeValues.fromUUID(uuid), DELETED_ACCOUNTS_ATTR_EXPIRES, AttributeValues.fromLong(clock.instant().plus(DELETED_ACCOUNTS_TIME_TO_LIVE).getEpochSecond()))) .build()) @@ -1178,7 +1181,7 @@ public class Accounts extends AbstractDynamoDbStore { .put(Put.builder() .tableName(deletedAccountsTableName) .item(Map.of( - DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(pni.toString()), + DELETED_ACCOUNTS_KEY_ACCOUNT_PNI, AttributeValues.fromString(pni.toString()), DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, AttributeValues.fromUUID(aci), DELETED_ACCOUNTS_ATTR_EXPIRES, AttributeValues.fromLong(clock.instant().plus(DELETED_ACCOUNTS_TIME_TO_LIVE).getEpochSecond()))) .build()) @@ -1189,7 +1192,7 @@ public class Accounts extends AbstractDynamoDbStore { return TransactWriteItem.builder() .delete(Delete.builder() .tableName(deletedAccountsTableName) - .key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164))) + .key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_PNI, AttributeValues.fromString(e164))) .build()) .build(); } @@ -1198,7 +1201,7 @@ public class Accounts extends AbstractDynamoDbStore { return TransactWriteItem.builder() .delete(Delete.builder() .tableName(deletedAccountsTableName) - .key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(pni.toString()))) + .key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_PNI, AttributeValues.fromString(pni.toString()))) .build()) .build(); } @@ -1210,34 +1213,24 @@ public class Accounts extends AbstractDynamoDbStore { .toCompletableFuture(); } - public Optional findRecentlyDeletedAccountIdentifier(final String e164) { - final GetItemResponse response = db().getItem(GetItemRequest.builder() - .tableName(deletedAccountsTableName) - .consistentRead(true) - .key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164))) - .build()); - - return Optional.ofNullable(AttributeValues.getUUID(response.item(), DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, null)); - } - public Optional findRecentlyDeletedAccountIdentifier(final UUID phoneNumberIdentifier) { final GetItemResponse response = db().getItem(GetItemRequest.builder() .tableName(deletedAccountsTableName) .consistentRead(true) - .key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(phoneNumberIdentifier.toString()))) + .key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_PNI, AttributeValues.fromString(phoneNumberIdentifier.toString()))) .build()); return Optional.ofNullable(AttributeValues.getUUID(response.item(), DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, null)); } - public Optional findRecentlyDeletedE164(final UUID uuid) { + public Optional findRecentlyDeletedPhoneNumberIdentifier(final UUID uuid) { final QueryResponse response = db().query(QueryRequest.builder() .tableName(deletedAccountsTableName) - .indexName(DELETED_ACCOUNTS_UUID_TO_E164_INDEX_NAME) + .indexName(DELETED_ACCOUNTS_UUID_TO_PNI_INDEX_NAME) .keyConditionExpression("#uuid = :uuid") - .projectionExpression("#e164") + .projectionExpression("#pni") .expressionAttributeNames(Map.of("#uuid", DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, - "#e164", DELETED_ACCOUNTS_KEY_ACCOUNT_E164)) + "#pni", DELETED_ACCOUNTS_KEY_ACCOUNT_PNI)) .expressionAttributeValues(Map.of(":uuid", AttributeValues.fromUUID(uuid))).build()); if (response.count() == 0) { @@ -1245,9 +1238,10 @@ public class Accounts extends AbstractDynamoDbStore { } return response.items().stream() - .map(item -> item.get(DELETED_ACCOUNTS_KEY_ACCOUNT_E164).s()) - .filter(e164OrPni -> e164OrPni.startsWith("+")) - .findFirst(); + .map(item -> item.get(DELETED_ACCOUNTS_KEY_ACCOUNT_PNI).s()) + .filter(e164OrPni -> !e164OrPni.startsWith("+")) + .findFirst() + .map(UUID::fromString); } public CompletableFuture delete(final UUID uuid, final List additionalWriteItems) { @@ -1313,51 +1307,13 @@ public class Accounts extends AbstractDynamoDbStore { .items()) .map(item -> Tuples.of( - item.get(DELETED_ACCOUNTS_KEY_ACCOUNT_E164).s(), + item.get(DELETED_ACCOUNTS_KEY_ACCOUNT_PNI).s(), AttributeValues.getUUID(item, DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, null), AttributeValues.getLong(item, DELETED_ACCOUNTS_ATTR_EXPIRES, 0))) .filter(item -> item.getT1().startsWith("+")) .sequential(); } - public CompletableFuture insertPniDeletedAccount(final String e164, final UUID pni, final UUID aci, final long expiration) { - // This happens under a pessimistic lock, but that wasn't taken before we found the record we want to migrate, - // so make sure the e164 record is unchanged before updating the PNI record - return asyncClient.getItem(GetItemRequest.builder() - .tableName(deletedAccountsTableName) - .consistentRead(true) - .key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164.toString()))) - .build()) - .thenComposeAsync(getItemResponse -> - getItemResponse.hasItem() - && AttributeValues.getString( - getItemResponse.item(), DELETED_ACCOUNTS_KEY_ACCOUNT_E164, "").equals(e164) - && AttributeValues.getUUID( - getItemResponse.item(), DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, UUID.randomUUID()).equals(aci) - && AttributeValues.getLong( - getItemResponse.item(), DELETED_ACCOUNTS_ATTR_EXPIRES, 0) == expiration - ? asyncClient.putItem( - PutItemRequest.builder() - .tableName(deletedAccountsTableName) - .item( - Map.of( - DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(pni.toString()), - DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, AttributeValues.fromUUID(aci), - DELETED_ACCOUNTS_ATTR_EXPIRES, AttributeValues.fromLong(expiration))) - .conditionExpression("attribute_not_exists(#key)") - .expressionAttributeNames(Map.of("#key", DELETED_ACCOUNTS_KEY_ACCOUNT_E164)) - .build()) - .thenApply(ignored -> true) - .exceptionally(throwable -> { - if (ExceptionUtils.unwrap(throwable) instanceof ConditionalCheckFailedException) { - // there was already a PNI record; no problem, do nothing - return false; - } - throw ExceptionUtils.wrap(throwable); - }) - : CompletableFuture.completedFuture(false)); - } - @Nonnull private Optional getByIndirectLookup( final Timer timer, 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 12bfab02f..2fb5a2f5f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -277,7 +277,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen return createTimer.record(() -> { accountLockManager.withLock(List.of(pni), () -> { final Optional maybeRecentlyDeletedAccountIdentifier = - accounts.findRecentlyDeletedAccountIdentifier(number); + accounts.findRecentlyDeletedAccountIdentifier(pni); // Reuse the ACI from any recently-deleted account with this number to cover cases where somebody is // re-registering. @@ -654,16 +654,16 @@ public class AccountsManager extends RedisPubSubAdapter implemen // There are three possible states for accounts associated with the target phone number: // - // 1. An account exists with the target number; the caller has proved ownership of the number, so delete the - // account with the target number. This will leave a "deleted account" record for the deleted account mapping - // the UUID of the deleted account to the target phone number. We'll then overwrite that so it points to the - // original number to facilitate switching back and forth between numbers. - // 2. No account with the target number exists, but one has recently been deleted. In that case, add a "deleted - // account" record that maps the ACI of the recently-deleted account to the now-abandoned original phone number + // 1. An account exists with the target PNI; the caller has proved ownership of the number, so delete the + // account with the target PNI. This will leave a "deleted account" record for the deleted account mapping + // the UUID of the deleted account to the target PNI. We'll then overwrite that so it points to the + // original PNI to facilitate switching back and forth between numbers. + // 2. No account with the target PNI exists, but one has recently been deleted. In that case, add a "deleted + // account" record that maps the ACI of the recently-deleted account to the now-abandoned original PNI // of the account changing its number (which facilitates ACI consistency in cases that a party is switching // back and forth between numbers). - // 3. No account with the target number exists at all, in which case no additional action is needed. - final Optional recentlyDeletedAci = accounts.findRecentlyDeletedAccountIdentifier(targetNumber); + // 3. No account with the target PNI exists at all, in which case no additional action is needed. + final Optional recentlyDeletedAci = accounts.findRecentlyDeletedAccountIdentifier(targetPhoneNumberIdentifier); final Optional maybeExistingAccount = getByE164(targetNumber); final Optional maybeDisplacedUuid; @@ -1205,31 +1205,18 @@ public class AccountsManager extends RedisPubSubAdapter implemen return phoneNumberIdentifiers.getPhoneNumberIdentifier(e164).join(); } - public Optional findRecentlyDeletedAccountIdentifier(final String e164) { - return accounts.findRecentlyDeletedAccountIdentifier(e164); + public Optional findRecentlyDeletedAccountIdentifier(final UUID phoneNumberIdentifier) { + return accounts.findRecentlyDeletedAccountIdentifier(phoneNumberIdentifier); } - public Optional findRecentlyDeletedE164(final UUID uuid) { - return accounts.findRecentlyDeletedE164(uuid); + public Optional findRecentlyDeletedPhoneNumberIdentifier(final UUID accountIdentifier) { + return accounts.findRecentlyDeletedPhoneNumberIdentifier(accountIdentifier); } public Flux streamAllFromDynamo(final int segments, final Scheduler scheduler) { return accounts.getAll(segments, scheduler); } - public Flux> getE164KeyedDeletedAccounts(final int segments, final Scheduler scheduler) { - return accounts.getE164KeyedDeletedAccounts(segments, scheduler); - } - - public CompletableFuture migrateDeletedAccount(final String e164, final UUID aci, final long expiration) { - return phoneNumberIdentifiers.getPhoneNumberIdentifier(e164) - .thenCompose( - pni -> accountLockManager.withLockAsync( - List.of(pni), - () -> accounts.insertPniDeletedAccount(e164, pni, aci, expiration), - accountLockExecutor)); - } - public CompletableFuture delete(final Account account, final DeletionReason deletionReason) { final Timer.Sample sample = Timer.start(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiers.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiers.java index 16ce8f100..3adbc3632 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiers.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiers.java @@ -13,6 +13,7 @@ import io.micrometer.core.instrument.Timer; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.function.Supplier; @@ -30,7 +31,10 @@ import software.amazon.awssdk.services.dynamodb.DynamoDbAsyncClient; import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.BatchGetItemRequest; import software.amazon.awssdk.services.dynamodb.model.CancellationReason; +import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; import software.amazon.awssdk.services.dynamodb.model.KeysAndAttributes; +import software.amazon.awssdk.services.dynamodb.model.QueryRequest; +import software.amazon.awssdk.services.dynamodb.model.ReturnValue; import software.amazon.awssdk.services.dynamodb.model.ReturnValuesOnConditionCheckFailure; import software.amazon.awssdk.services.dynamodb.model.ScanRequest; import software.amazon.awssdk.services.dynamodb.model.TransactWriteItem; @@ -84,6 +88,34 @@ public class PhoneNumberIdentifiers { .thenCompose(mappings -> setPniIfRequired(phoneNumber, allPhoneNumberForms, mappings))); } + /** + * Returns the list of phone numbers associated with a given phone number identifier. If this + * UUID was not previously assigned as a PNI by {@link #getPhoneNumberIdentifier(String)}, the + * returned list will be empty. + * + * @param UUID a phone number identifier + * @return the list of all e164s associated with the given phone number identifier + */ + public CompletableFuture> getPhoneNumber(final UUID phoneNumberIdentifier) { + return dynamoDbClient.query(QueryRequest.builder() + .tableName(tableName) + .indexName(INDEX_NAME) + .keyConditionExpression("#pni = :pni") + .projectionExpression("#phone_number") + .expressionAttributeNames(Map.of( + "#phone_number", KEY_E164, + "#pni", ATTR_PHONE_NUMBER_IDENTIFIER + )) + .expressionAttributeValues(Map.of( + ":pni", AttributeValues.fromUUID(phoneNumberIdentifier) + )) + .build()) + .thenApply(response -> { + return response.items().stream().map(item -> item.get(KEY_E164).s()).toList(); + }); + } + + @VisibleForTesting static CompletableFuture retry( final int numRetries, final Class exceptionToRetry, final Supplier> supplier) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/util/Util.java b/service/src/main/java/org/whispersystems/textsecuregcm/util/Util.java index d17a3d880..e1fa79dc0 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/util/Util.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/util/Util.java @@ -14,6 +14,7 @@ import java.time.Duration; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Comparator; import java.util.HashSet; import java.util.List; import java.util.Locale; @@ -24,6 +25,8 @@ import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.random.RandomGenerator; +import java.util.stream.Collectors; + import org.apache.commons.lang3.StringUtils; public class Util { @@ -145,6 +148,44 @@ public class Util { } } + /** + * Returns the preferred form of an e164 from a list of equivalents. Only use this when there is no other reason (such + * as the form specifically provided by a user) to prefer a particular form and we want to reduce nondeterminism. + * + * @apiNote This method is intended to support number format transitions in cases where we do not already have + * multiple accounts registered with different forms of the same number. As a result, this method does not cover all + * possible cases of equivalent formats, but instead focuses on the cases where we can and choose to prevent multiple + * accounts from using different formats of the same number. + * + * @param e164s a list of equivalent forms of a single phone number + * + * @return a single preferred canonical form for the number + */ + public static Optional getCanonicalNumber(List e164s) { + if (e164s.size() <= 1) { + return e164s.stream().findFirst(); + } + try { + final List phoneNumbers = new ArrayList<>(e164s.size()); + for (String e164 : e164s) { + phoneNumbers.add(PHONE_NUMBER_UTIL.parse(e164, null)); + } + final Set regions = phoneNumbers.stream().map(PHONE_NUMBER_UTIL::getRegionCodeForNumber).collect(Collectors.toSet()); + if (regions.size() != 1) { + throw new IllegalArgumentException("Numbers from different countries cannot be equivalent alternate forms"); + } + if (regions.contains("BJ")) { + // Benin is changing phone number formats from +229 XXXXXXXX to +229 01XXXXXXXX starting on November 30, 2024 + // We prefer the longest form for long-term stability + return e164s.stream().sorted(Comparator.comparingInt(String::length).reversed()).findFirst(); + } + // No matching country; fall back to something that's at least stable + return e164s.stream().sorted().findFirst(); + } catch (final NumberParseException e) { + return e164s.stream().sorted().findFirst(); + } + } + /** * Tests whether the decimal form of the given number (without leading zeroes) begins with the decimal form of the * given prefix (without leading zeroes). diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/MigrateDeletedAccountsCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/MigrateDeletedAccountsCommand.java deleted file mode 100644 index 94f369a18..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/MigrateDeletedAccountsCommand.java +++ /dev/null @@ -1,115 +0,0 @@ -/* - * Copyright 2024 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.workers; - -import io.dropwizard.core.Application; -import io.dropwizard.core.setup.Environment; -import io.micrometer.core.instrument.Counter; -import io.micrometer.core.instrument.Metrics; -import net.sourceforge.argparse4j.inf.Namespace; -import net.sourceforge.argparse4j.inf.Subparser; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.whispersystems.textsecuregcm.WhisperServerConfiguration; -import org.whispersystems.textsecuregcm.metrics.MetricsUtil; -import org.whispersystems.textsecuregcm.storage.AccountsManager; -import reactor.core.publisher.Mono; -import reactor.core.scheduler.Schedulers; -import reactor.util.retry.Retry; -import java.time.Duration; - -public class MigrateDeletedAccountsCommand extends AbstractCommandWithDependencies { - - private static final String RECORDS_INSPECTED_COUNTER_NAME = - MetricsUtil.name(MigrateDeletedAccountsCommand.class, "recordsInspected"); - - private static final String RECORDS_MIGRATED_COUNTER_NAME = - MetricsUtil.name(MigrateDeletedAccountsCommand.class, "recordsMigrated"); - - private static final String DRY_RUN_TAG = "dryRun"; - - private final Logger logger = LoggerFactory.getLogger(getClass()); - - private static final String SEGMENT_COUNT_ARGUMENT = "segments"; - private static final String DRY_RUN_ARGUMENT = "dry-run"; - private static final String MAX_CONCURRENCY_ARGUMENT = "max-concurrency"; - - private static final int DEFAULT_SEGMENT_COUNT = 1; - private static final int DEFAULT_CONCURRENCY = 16; - - public MigrateDeletedAccountsCommand() { - super(new Application<>() { - @Override - public void run(final WhisperServerConfiguration configuration, final Environment environment) { - } - }, "migrate-deleted-accounts", "Migrates recently-deleted account records from E164 to PNI-keyed schema"); - } - - @Override - public void configure(final Subparser subparser) { - super.configure(subparser); - - subparser.addArgument("--segments") - .type(Integer.class) - .dest(SEGMENT_COUNT_ARGUMENT) - .required(false) - .setDefault(DEFAULT_SEGMENT_COUNT) - .help("The total number of segments for a DynamoDB scan"); - - subparser.addArgument("--max-concurrency") - .type(Integer.class) - .dest(MAX_CONCURRENCY_ARGUMENT) - .required(false) - .setDefault(DEFAULT_CONCURRENCY) - .help("Max concurrency for migrations."); - - subparser.addArgument("--dry-run") - .type(Boolean.class) - .dest(DRY_RUN_ARGUMENT) - .required(false) - .setDefault(true) - .help("If true, don’t actually migrate any deleted accounts records"); - } - - @Override - protected void run(final Environment environment, final Namespace namespace, - final WhisperServerConfiguration configuration, final CommandDependencies commandDependencies) throws Exception { - final int segments = namespace.getInt(SEGMENT_COUNT_ARGUMENT); - final int concurrency = namespace.getInt(MAX_CONCURRENCY_ARGUMENT); - final boolean dryRun = namespace.getBoolean(DRY_RUN_ARGUMENT); - - logger.info("Crawling deleted accounts with {} segments and {} processors", - segments, - Runtime.getRuntime().availableProcessors()); - - final Counter recordsInspectedCounter = - Metrics.counter(RECORDS_INSPECTED_COUNTER_NAME, DRY_RUN_TAG, String.valueOf(dryRun)); - - final Counter recordsMigratedCounter = - Metrics.counter(RECORDS_MIGRATED_COUNTER_NAME, DRY_RUN_TAG, String.valueOf(dryRun)); - - final AccountsManager accounts = commandDependencies.accountsManager(); - - accounts.getE164KeyedDeletedAccounts(segments, Schedulers.parallel()) - .doOnNext(tuple -> recordsInspectedCounter.increment()) - .flatMap( - tuple -> dryRun - ? Mono.just(false) - : Mono.fromFuture( - accounts.migrateDeletedAccount( - tuple.getT1(), tuple.getT2(), tuple.getT3())) - .retryWhen(Retry.backoff(3, Duration.ofSeconds(1))) - .onErrorResume(throwable -> { - logger.warn("Failed to migrate record for {}", tuple.getT1(), throwable); - return Mono.empty(); - }), - concurrency) - .filter(migrated -> migrated) - .doOnNext(ignored -> recordsMigratedCounter.increment()) - .then() - .block(); - } -} diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java index c6bc89109..0d538c4bf 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java @@ -121,6 +121,7 @@ import org.whispersystems.textsecuregcm.storage.ClientReleaseManager; import org.whispersystems.textsecuregcm.storage.Device; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; import org.whispersystems.textsecuregcm.storage.MessagesManager; +import org.whispersystems.textsecuregcm.storage.PhoneNumberIdentifiers; import org.whispersystems.textsecuregcm.storage.RemovedMessage; import org.whispersystems.textsecuregcm.storage.ReportMessageManager; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; @@ -182,6 +183,7 @@ class MessageControllerTest { private static final RateLimiters rateLimiters = mock(RateLimiters.class); private static final CardinalityEstimator cardinalityEstimator = mock(CardinalityEstimator.class); private static final RateLimiter rateLimiter = mock(RateLimiter.class); + private static final PhoneNumberIdentifiers phoneNumberIdentifiers = mock(PhoneNumberIdentifiers.class); private static final PushNotificationManager pushNotificationManager = mock(PushNotificationManager.class); private static final PushNotificationScheduler pushNotificationScheduler = mock(PushNotificationScheduler.class); private static final ReportMessageManager reportMessageManager = mock(ReportMessageManager.class); @@ -205,9 +207,9 @@ class MessageControllerTest { .setTestContainerFactory(new GrizzlyWebTestContainerFactory()) .addResource( new MessageController(rateLimiters, cardinalityEstimator, messageSender, receiptSender, accountsManager, - messagesManager, pushNotificationManager, pushNotificationScheduler, reportMessageManager, multiRecipientMessageExecutor, - messageDeliveryScheduler, mock(ClientReleaseManager.class), dynamicConfigurationManager, - serverSecretParams, SpamChecker.noop(), new MessageMetrics(), mock(MessageDeliveryLoopMonitor.class), + messagesManager, phoneNumberIdentifiers, pushNotificationManager, pushNotificationScheduler, + reportMessageManager, multiRecipientMessageExecutor, messageDeliveryScheduler, mock(ClientReleaseManager.class), + dynamicConfigurationManager, serverSecretParams, SpamChecker.noop(), new MessageMetrics(), mock(MessageDeliveryLoopMonitor.class), clock)) .build(); @@ -286,6 +288,7 @@ class MessageControllerTest { rateLimiters, rateLimiter, cardinalityEstimator, + phoneNumberIdentifiers, pushNotificationManager, reportMessageManager ); @@ -807,7 +810,6 @@ class MessageControllerTest { @Test void testReportMessageByE164() { - final String senderNumber = "+12125550001"; final UUID senderAci = UUID.randomUUID(); final UUID senderPni = UUID.randomUUID(); @@ -820,8 +822,6 @@ class MessageControllerTest { when(account.getPhoneNumberIdentifier()).thenReturn(senderPni); when(accountsManager.getByE164(senderNumber)).thenReturn(Optional.of(account)); - when(accountsManager.findRecentlyDeletedAccountIdentifier(senderNumber)).thenReturn(Optional.of(senderAci)); - when(accountsManager.getPhoneNumberIdentifier(senderNumber)).thenReturn(senderPni); try (final Response response = resources.getJerseyTest() @@ -835,12 +835,27 @@ class MessageControllerTest { verify(reportMessageManager).report(Optional.of(senderNumber), Optional.of(senderAci), Optional.of(senderPni), messageGuid, AuthHelper.VALID_UUID, Optional.empty(), userAgent); - verify(accountsManager, never()).findRecentlyDeletedE164(any(UUID.class)); - verify(accountsManager, never()).getPhoneNumberIdentifier(anyString()); + verify(accountsManager, never()).findRecentlyDeletedPhoneNumberIdentifier(any(UUID.class)); + verify(phoneNumberIdentifiers, never()).getPhoneNumber(any()); } + } + + @Test + void testReportMesageByE164DeletedAccount() { + final String senderNumber = "+12125550001"; + final UUID senderAci = UUID.randomUUID(); + final UUID senderPni = UUID.randomUUID(); + final String userAgent = "user-agent"; + UUID messageGuid = UUID.randomUUID(); + + final Account account = mock(Account.class); + when(account.getUuid()).thenReturn(senderAci); + when(account.getNumber()).thenReturn(senderNumber); + when(account.getPhoneNumberIdentifier()).thenReturn(senderPni); when(accountsManager.getByE164(senderNumber)).thenReturn(Optional.empty()); - messageGuid = UUID.randomUUID(); + when(phoneNumberIdentifiers.getPhoneNumberIdentifier(senderNumber)).thenReturn(CompletableFuture.completedFuture(senderPni)); + when(accountsManager.findRecentlyDeletedAccountIdentifier(senderPni)).thenReturn(Optional.of(senderAci)); try (final Response response = resources.getJerseyTest() @@ -859,7 +874,6 @@ class MessageControllerTest { @Test void testReportMessageByAci() { - final String senderNumber = "+12125550001"; final UUID senderAci = UUID.randomUUID(); final UUID senderPni = UUID.randomUUID(); @@ -872,8 +886,7 @@ class MessageControllerTest { when(account.getPhoneNumberIdentifier()).thenReturn(senderPni); when(accountsManager.getByAccountIdentifier(senderAci)).thenReturn(Optional.of(account)); - when(accountsManager.findRecentlyDeletedE164(senderAci)).thenReturn(Optional.of(senderNumber)); - when(accountsManager.getPhoneNumberIdentifier(senderNumber)).thenReturn(senderPni); + when(phoneNumberIdentifiers.getPhoneNumber(senderPni)).thenReturn(CompletableFuture.completedFuture(List.of(senderNumber))); try (final Response response = resources.getJerseyTest() @@ -887,11 +900,27 @@ class MessageControllerTest { verify(reportMessageManager).report(Optional.of(senderNumber), Optional.of(senderAci), Optional.of(senderPni), messageGuid, AuthHelper.VALID_UUID, Optional.empty(), userAgent); - verify(accountsManager, never()).findRecentlyDeletedE164(any(UUID.class)); - verify(accountsManager, never()).getPhoneNumberIdentifier(anyString()); + verify(accountsManager, never()).findRecentlyDeletedPhoneNumberIdentifier(any(UUID.class)); + verify(phoneNumberIdentifiers, never()).getPhoneNumber(any()); } + } + + @Test + void testReportMessageByAciDeletedAccount() { + final String senderNumber = "+12125550001"; + final UUID senderAci = UUID.randomUUID(); + final UUID senderPni = UUID.randomUUID(); + final String userAgent = "user-agent"; + UUID messageGuid = UUID.randomUUID(); + + final Account account = mock(Account.class); + when(account.getUuid()).thenReturn(senderAci); + when(account.getNumber()).thenReturn(senderNumber); + when(account.getPhoneNumberIdentifier()).thenReturn(senderPni); when(accountsManager.getByAccountIdentifier(senderAci)).thenReturn(Optional.empty()); + when(accountsManager.findRecentlyDeletedPhoneNumberIdentifier(senderAci)).thenReturn(Optional.of(senderPni)); + when(phoneNumberIdentifiers.getPhoneNumber(senderPni)).thenReturn(CompletableFuture.completedFuture(List.of(senderNumber))); messageGuid = UUID.randomUUID(); @@ -924,8 +953,8 @@ class MessageControllerTest { when(account.getPhoneNumberIdentifier()).thenReturn(senderPni); when(accountsManager.getByAccountIdentifier(senderAci)).thenReturn(Optional.of(account)); - when(accountsManager.findRecentlyDeletedE164(senderAci)).thenReturn(Optional.of(senderNumber)); - when(accountsManager.getPhoneNumberIdentifier(senderNumber)).thenReturn(senderPni); + when(accountsManager.findRecentlyDeletedPhoneNumberIdentifier(senderAci)).thenReturn(Optional.of(senderPni)); + when(phoneNumberIdentifiers.getPhoneNumber(senderPni)).thenReturn(CompletableFuture.completedFuture(List.of(senderNumber))); Entity entity = Entity.entity(new SpamReport(new byte[3]), "application/json"); @@ -944,8 +973,8 @@ class MessageControllerTest { eq(AuthHelper.VALID_UUID), argThat(maybeBytes -> maybeBytes.map(bytes -> Arrays.equals(bytes, new byte[3])).orElse(false)), any()); - verify(accountsManager, never()).findRecentlyDeletedE164(any(UUID.class)); - verify(accountsManager, never()).getPhoneNumberIdentifier(anyString()); + verify(accountsManager, never()).findRecentlyDeletedPhoneNumberIdentifier(any(UUID.class)); + verify(phoneNumberIdentifiers, never()).getPhoneNumber(any()); } when(accountsManager.getByAccountIdentifier(senderAci)).thenReturn(Optional.empty()); @@ -987,8 +1016,8 @@ class MessageControllerTest { when(account.getPhoneNumberIdentifier()).thenReturn(senderPni); when(accountsManager.getByAccountIdentifier(senderAci)).thenReturn(Optional.of(account)); - when(accountsManager.findRecentlyDeletedE164(senderAci)).thenReturn(Optional.of(senderNumber)); - when(accountsManager.getPhoneNumberIdentifier(senderNumber)).thenReturn(senderPni); + when(accountsManager.findRecentlyDeletedPhoneNumberIdentifier(senderAci)).thenReturn(Optional.of(senderPni)); + when(phoneNumberIdentifiers.getPhoneNumber(senderPni)).thenReturn(CompletableFuture.completedFuture(List.of(senderNumber))); try (final Response response = resources.getJerseyTest() diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java index 31874a0ec..ef1c2679f 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java @@ -176,14 +176,13 @@ class AccountsManagerChangeNumberIntegrationTest { assertTrue(accountsManager.getByE164(originalNumber).isEmpty()); - assertTrue(accountsManager.getByE164(secondNumber).isPresent()); - assertEquals(originalUuid, accountsManager.getByE164(secondNumber).map(Account::getUuid).orElseThrow()); - assertNotEquals(originalPni, accountsManager.getByE164(secondNumber).map(Account::getPhoneNumberIdentifier).orElseThrow()); + final Account updatedAccount = accountsManager.getByE164(secondNumber).orElseThrow(); + assertEquals(originalUuid, updatedAccount.getUuid()); + assertEquals(secondNumber, updatedAccount.getNumber()); + assertNotEquals(originalPni, updatedAccount.getPhoneNumberIdentifier()); - assertEquals(secondNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); - - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalPni)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(updatedAccount.getPhoneNumberIdentifier())); } @Test @@ -207,17 +206,19 @@ class AccountsManagerChangeNumberIntegrationTest { final Map registrationIds = Map.of(Device.PRIMARY_ID, rotatedPniRegistrationId); final Account updatedAccount = accountsManager.changeNumber(account, secondNumber, pniIdentityKey, preKeys, null, registrationIds); + final UUID secondPni = updatedAccount.getPhoneNumberIdentifier(); assertTrue(accountsManager.getByE164(originalNumber).isEmpty()); assertTrue(accountsManager.getByE164(secondNumber).isPresent()); assertEquals(originalUuid, accountsManager.getByE164(secondNumber).map(Account::getUuid).orElseThrow()); - assertNotEquals(originalPni, accountsManager.getByE164(secondNumber).map(Account::getPhoneNumberIdentifier).orElseThrow()); + assertNotEquals(originalPni, secondPni); + assertEquals(secondPni, accountsManager.getByE164(secondNumber).map(Account::getPhoneNumberIdentifier).orElseThrow()); assertEquals(secondNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalPni)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondPni)); assertEquals(pniIdentityKey, updatedAccount.getIdentityKey(IdentityType.PNI)); @@ -239,6 +240,7 @@ class AccountsManagerChangeNumberIntegrationTest { final UUID originalPni = account.getPhoneNumberIdentifier(); account = accountsManager.changeNumber(account, secondNumber, null, null, null, null); + final UUID secondPni = account.getPhoneNumberIdentifier(); accountsManager.changeNumber(account, originalNumber, null, null, null, null); assertTrue(accountsManager.getByE164(originalNumber).isPresent()); @@ -249,8 +251,8 @@ class AccountsManagerChangeNumberIntegrationTest { assertEquals(originalNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalPni)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondPni)); } @Test @@ -261,12 +263,14 @@ class AccountsManagerChangeNumberIntegrationTest { final Account account = AccountsHelper.createAccount(accountsManager, originalNumber); final UUID originalUuid = account.getUuid(); + final UUID originalPni = account.getPhoneNumberIdentifier(); final Account existingAccount = AccountsHelper.createAccount(accountsManager, secondNumber); final UUID existingAccountUuid = existingAccount.getUuid(); accountsManager.changeNumber(account, secondNumber, null, null, null, null); + final UUID secondPni = accountsManager.getByE164(secondNumber).get().getPhoneNumberIdentifier(); assertTrue(accountsManager.getByE164(originalNumber).isEmpty()); @@ -277,8 +281,8 @@ class AccountsManagerChangeNumberIntegrationTest { verify(disconnectionRequestManager).requestDisconnection(existingAccountUuid); - assertEquals(Optional.of(existingAccountUuid), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); + assertEquals(Optional.of(existingAccountUuid), accountsManager.findRecentlyDeletedAccountIdentifier(originalPni)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondPni)); accountsManager.changeNumber(accountsManager.getByAccountIdentifier(originalUuid).orElseThrow(), originalNumber, null, null, null, null); @@ -309,13 +313,13 @@ class AccountsManagerChangeNumberIntegrationTest { assertEquals(existingAccountUuid, reRegisteredAccount.getUuid()); assertEquals(originalPni, reRegisteredAccount.getPhoneNumberIdentifier()); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalPni)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondPni)); final Account changedNumberReRegisteredAccount = accountsManager.changeNumber(reRegisteredAccount, secondNumber, null, null, null, null); - assertEquals(Optional.of(originalUuid), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); - assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); + assertEquals(Optional.of(originalUuid), accountsManager.findRecentlyDeletedAccountIdentifier(originalPni)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondPni)); assertEquals(secondPni, changedNumberReRegisteredAccount.getPhoneNumberIdentifier()); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java index d807df65b..ce9338a28 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -896,7 +896,7 @@ class AccountsManagerTest { void testCreateAccountRecentlyDeleted() throws InterruptedException, AccountAlreadyExistsException { final UUID recentlyDeletedUuid = UUID.randomUUID(); - when(accounts.findRecentlyDeletedAccountIdentifier(anyString())).thenReturn(Optional.of(recentlyDeletedUuid)); + when(accounts.findRecentlyDeletedAccountIdentifier(any())).thenReturn(Optional.of(recentlyDeletedUuid)); when(accounts.create(any(), any())).thenReturn(true); final String e164 = "+18005550123"; 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 547e5dc8c..d91bccd54 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -212,7 +212,6 @@ class AccountsTest { assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier(), account.getUuid()); accounts.delete(originalUuid, Collections.emptyList()).join(); - assertThat(accounts.findRecentlyDeletedAccountIdentifier(account.getNumber())).hasValue(originalUuid); assertThat(accounts.findRecentlyDeletedAccountIdentifier(account.getPhoneNumberIdentifier())).hasValue(originalUuid); freshUser = createAccount(account); @@ -222,7 +221,6 @@ class AccountsTest { assertPhoneNumberConstraintExists("+14151112222", account.getUuid()); assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier(), account.getUuid()); - assertThat(accounts.findRecentlyDeletedAccountIdentifier(account.getNumber())).isEmpty(); assertThat(accounts.findRecentlyDeletedAccountIdentifier(account.getPhoneNumberIdentifier())).isEmpty(); } @@ -745,7 +743,6 @@ class AccountsTest { createAccount(deletedAccount); createAccount(retainedAccount); - assertThat(accounts.findRecentlyDeletedAccountIdentifier(deletedAccount.getNumber())).isEmpty(); assertThat(accounts.findRecentlyDeletedAccountIdentifier(deletedAccount.getPhoneNumberIdentifier())).isEmpty(); assertPhoneNumberConstraintExists("+14151112222", deletedAccount.getUuid()); @@ -759,7 +756,6 @@ class AccountsTest { accounts.delete(deletedAccount.getUuid(), Collections.emptyList()).join(); assertThat(accounts.getByAccountIdentifier(deletedAccount.getUuid())).isNotPresent(); - assertThat(accounts.findRecentlyDeletedAccountIdentifier(deletedAccount.getNumber())).hasValue(deletedAccount.getUuid()); assertThat(accounts.findRecentlyDeletedAccountIdentifier(deletedAccount.getPhoneNumberIdentifier())).hasValue(deletedAccount.getUuid()); assertPhoneNumberConstraintDoesNotExist(deletedAccount.getNumber()); @@ -898,7 +894,6 @@ class AccountsTest { assertThat(accounts.getByPhoneNumberIdentifier(targetPni)).isPresent(); } - assertThat(accounts.findRecentlyDeletedAccountIdentifier(originalNumber)).isEqualTo(maybeDisplacedAccountIdentifier); assertThat(accounts.findRecentlyDeletedAccountIdentifier(originalPni)).isEqualTo(maybeDisplacedAccountIdentifier); } @@ -1714,133 +1709,6 @@ class AccountsTest { accounts.getE164KeyedDeletedAccounts(1, Schedulers.immediate()).collectList().block()); } - @Test - public void insertPniDeletedAccount() throws Exception { - final String e164 = "+18005551234"; - final UUID aci = UUID.randomUUID(); - final UUID pni = UUID.randomUUID(); - final Long expires = 1234567890L; - - final ScanRequest scanRequest = ScanRequest.builder() - .tableName(Tables.DELETED_ACCOUNTS.tableName()) - .build(); - assertThat( - DYNAMO_DB_EXTENSION.getDynamoDbClient() - .scan(scanRequest) - .items()) - .isEmpty(); - - - DYNAMO_DB_EXTENSION.getDynamoDbClient().putItem( - PutItemRequest.builder() - .tableName(Tables.DELETED_ACCOUNTS.tableName()) - .item(Map.of( - Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164), - Accounts.DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, AttributeValues.fromUUID(aci), - Accounts.DELETED_ACCOUNTS_ATTR_EXPIRES, AttributeValues.fromLong(expires))) - .build()); - - assertThat(accounts.insertPniDeletedAccount(e164, pni, aci, expires).get()).isTrue(); - - List> items = - DYNAMO_DB_EXTENSION.getDynamoDbClient() - .scan(scanRequest) - .items(); - assertThat(items).hasSize(2); - final Map item = items.stream().filter(i -> !i.get(Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_E164).s().equals(e164)).findFirst().get(); - assertThat(item.get(Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_E164).s()) - .isEqualTo(pni.toString()); - assertThat(AttributeValues.getUUID(item, Accounts.DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, null)) - .isEqualTo(aci); - assertThat(AttributeValues.getLong(item, Accounts.DELETED_ACCOUNTS_ATTR_EXPIRES, 0)) - .isEqualTo(expires); - } - - @Test - public void insertPniDeletedAccount_concurrentChange() throws Exception { - final String e164 = "+18005551234"; - final UUID aci = UUID.randomUUID(); - final UUID pni = UUID.randomUUID(); - final Long expires = 1234567890L; - - final ScanRequest scanRequest = ScanRequest.builder() - .tableName(Tables.DELETED_ACCOUNTS.tableName()) - .build(); - assertThat( - DYNAMO_DB_EXTENSION.getDynamoDbClient() - .scan(scanRequest) - .items()) - .isEmpty(); - - - DYNAMO_DB_EXTENSION.getDynamoDbClient().putItem( - PutItemRequest.builder() - .tableName(Tables.DELETED_ACCOUNTS.tableName()) - .item(Map.of( - Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164), - Accounts.DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, AttributeValues.fromUUID(aci), - Accounts.DELETED_ACCOUNTS_ATTR_EXPIRES, AttributeValues.fromLong(expires + 1))) - .build()); - - assertThat(accounts.insertPniDeletedAccount(e164, pni, aci, expires).get()).isFalse(); - - List> items = - DYNAMO_DB_EXTENSION.getDynamoDbClient() - .scan(scanRequest) - .items(); - assertThat(items).hasSize(1); - } - - @Test - public void insertPniDeletedAccount_concurrentDeletion() throws Exception { - final String e164 = "+18005551234"; - final UUID aci = UUID.randomUUID(); - final UUID pni = UUID.randomUUID(); - final Long expires = 1234567890L; - - final ScanRequest scanRequest = ScanRequest.builder() - .tableName(Tables.DELETED_ACCOUNTS.tableName()) - .build(); - assertThat( - DYNAMO_DB_EXTENSION.getDynamoDbClient() - .scan(scanRequest) - .items()) - .isEmpty(); - - assertThat(accounts.insertPniDeletedAccount(e164, pni, aci, expires).get()).isFalse(); - - List> items = - DYNAMO_DB_EXTENSION.getDynamoDbClient() - .scan(scanRequest) - .items(); - assertThat(items).isEmpty(); - } - - @Test - public void insertPniDeletedAccount_alreadyMigrated() throws Exception { - final Account deletedAccount = generateAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID()); - - createAccount(deletedAccount); - accounts.delete(deletedAccount.getUuid(), List.of()).join(); - - final ScanRequest scanRequest = ScanRequest.builder() - .tableName(Tables.DELETED_ACCOUNTS.tableName()) - .build(); - assertThat( - DYNAMO_DB_EXTENSION.getDynamoDbClient() - .scan(scanRequest) - .items()) - .hasSize(2); - - assertThat(accounts.insertPniDeletedAccount(deletedAccount.getNumber(), deletedAccount.getPhoneNumberIdentifier(), deletedAccount.getUuid(), clock.instant().plus(Accounts.DELETED_ACCOUNTS_TIME_TO_LIVE).toEpochMilli() / 1000).get()).isFalse(); - - List> items = - DYNAMO_DB_EXTENSION.getDynamoDbClient() - .scan(scanRequest) - .items(); - assertThat(items).hasSize(2); - } - private static Device generateDevice(byte id) { return DevicesHelper.createDevice(id); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/DynamoDbExtensionSchema.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/DynamoDbExtensionSchema.java index 7ab8400a9..13e0f009c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/DynamoDbExtensionSchema.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/DynamoDbExtensionSchema.java @@ -75,11 +75,11 @@ public final class DynamoDbExtensionSchema { List.of()), DELETED_ACCOUNTS("deleted_accounts_test", - Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_E164, + Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_PNI, null, List.of( AttributeDefinition.builder() - .attributeName(Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_E164) + .attributeName(Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_PNI) .attributeType(ScalarAttributeType.S).build(), AttributeDefinition.builder() .attributeName(Accounts.DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID) @@ -87,7 +87,7 @@ public final class DynamoDbExtensionSchema { .build()), List.of( GlobalSecondaryIndex.builder() - .indexName(Accounts.DELETED_ACCOUNTS_UUID_TO_E164_INDEX_NAME) + .indexName(Accounts.DELETED_ACCOUNTS_UUID_TO_PNI_INDEX_NAME) .keySchema( KeySchemaElement.builder().attributeName(Accounts.DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID).keyType(KeyType.HASH).build() ) diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiersTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiersTest.java index cfb189867..07a1dbf73 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiersTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiersTest.java @@ -7,6 +7,7 @@ package org.whispersystems.textsecuregcm.storage; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.i18n.phonenumbers.PhoneNumberUtil; import java.io.IOException; @@ -189,4 +190,14 @@ class PhoneNumberIdentifiersTest { IOException.class, PhoneNumberIdentifiers.retry(10, RuntimeException.class, new FailN(1))); } + + @Test + void getPhoneNumber() { + final String number = "+18005551234"; + + assertTrue(phoneNumberIdentifiers.getPhoneNumber(UUID.randomUUID()).join().isEmpty()); + + final UUID pni = phoneNumberIdentifiers.getPhoneNumberIdentifier(number).join(); + assertEquals(List.of(number), phoneNumberIdentifiers.getPhoneNumber(pni).join()); + } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/util/UtilTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/util/UtilTest.java index e27296349..0af2994fe 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/util/UtilTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/util/UtilTest.java @@ -9,6 +9,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import com.google.i18n.phonenumbers.PhoneNumberUtil; import java.util.List; +import java.util.Optional; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.CsvSource; @@ -39,6 +41,21 @@ class UtilTest { ); } + @Test + void getCanonicalNumber() { + final String usE164 = PhoneNumberUtil.getInstance().format( + PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164); + assertEquals(Optional.of(usE164), Util.getCanonicalNumber(List.of(usE164))); + + final String newFormatBeninE164 = PhoneNumberUtil.getInstance() + .format(PhoneNumberUtil.getInstance().getExampleNumber("BJ"), PhoneNumberUtil.PhoneNumberFormat.E164); + + final String oldFormatBeninE164 = newFormatBeninE164.replaceFirst("01", ""); + assertEquals(Optional.of(newFormatBeninE164), Util.getCanonicalNumber(List.of(oldFormatBeninE164, newFormatBeninE164))); + + assertEquals(Optional.empty(), Util.getCanonicalNumber(List.of())); + } + @ParameterizedTest @CsvSource({ "0, 1, false",