From 1b9bf01ab1f49730adae05ed87be2d53719795ef Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Tue, 12 Sep 2023 22:27:36 -0400 Subject: [PATCH] Absorb `DeletedAccounts` into `Accounts` --- .../textsecuregcm/WhisperServerService.java | 9 +- .../controllers/MessageController.java | 8 +- .../storage/AccountLockManager.java | 4 +- .../textsecuregcm/storage/Accounts.java | 67 +++++++++++++- .../storage/AccountsManager.java | 21 +++-- .../storage/DeletedAccounts.java | 87 ------------------- .../workers/AssignUsernameCommand.java | 6 +- .../workers/CommandDependencies.java | 6 +- .../controllers/MessageControllerTest.java | 19 ++-- ...ntsManagerChangeNumberIntegrationTest.java | 30 +++---- ...ConcurrentModificationIntegrationTest.java | 9 +- .../storage/AccountsManagerTest.java | 14 +-- ...ccountsManagerUsernameIntegrationTest.java | 7 +- .../textsecuregcm/storage/AccountsTest.java | 61 ++++++++++++- .../storage/DeletedAccountsTest.java | 80 ----------------- .../storage/DynamoDbExtensionSchema.java | 14 +-- 16 files changed, 181 insertions(+), 261 deletions(-) delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccounts.java delete mode 100644 service/src/test/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsTest.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 1b7185a8f..3bda191e8 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -176,7 +176,6 @@ import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.ChangeNumberManager; import org.whispersystems.textsecuregcm.storage.ClientReleaseManager; import org.whispersystems.textsecuregcm.storage.ClientReleases; -import org.whispersystems.textsecuregcm.storage.DeletedAccounts; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; import org.whispersystems.textsecuregcm.storage.IssuedReceiptsManager; import org.whispersystems.textsecuregcm.storage.KeysManager; @@ -299,9 +298,6 @@ public class WhisperServerService extends Application dynamicConfigurationManager = new DynamicConfigurationManager<>(config.getAppConfig().getApplication(), config.getAppConfig().getEnvironment(), @@ -325,6 +321,7 @@ public class WhisperServerService extends Application> getByAccountIdentifierAsync(final UUID uuid) { return AsyncTimerUtil.record(GET_BY_UUID_TIMER, () -> itemByKeyAsync(accountsTableName, KEY_ACCOUNT_UUID, AttributeValues.fromUUID(uuid)) @@ -713,6 +741,37 @@ 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 findRecentlyDeletedE164(final UUID uuid) { + final QueryResponse response = db().query(QueryRequest.builder() + .tableName(deletedAccountsTableName) + .indexName(DELETED_ACCOUNTS_UUID_TO_E164_INDEX_NAME) + .keyConditionExpression("#uuid = :uuid") + .projectionExpression("#e164") + .expressionAttributeNames(Map.of("#uuid", DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, + "#e164", DELETED_ACCOUNTS_KEY_ACCOUNT_E164)) + .expressionAttributeValues(Map.of(":uuid", AttributeValues.fromUUID(uuid))).build()); + + if (response.count() == 0) { + return Optional.empty(); + } + + if (response.count() > 1) { + throw new RuntimeException("Impossible result: more than one phone number returned for UUID: " + uuid); + } + + return Optional.ofNullable(response.items().get(0).get(DELETED_ACCOUNTS_KEY_ACCOUNT_E164).s()); + } + public void delete(final UUID uuid) { DELETE_TIMER.record(() -> getByAccountIdentifier(uuid).ifPresent(account -> { 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 dc7e7197e..26b3b7578 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -101,7 +101,6 @@ public class AccountsManager { private final PhoneNumberIdentifiers phoneNumberIdentifiers; private final FaultTolerantRedisCluster cacheCluster; private final AccountLockManager accountLockManager; - private final DeletedAccounts deletedAccounts; private final KeysManager keysManager; private final MessagesManager messagesManager; private final ProfilesManager profilesManager; @@ -147,7 +146,6 @@ public class AccountsManager { final PhoneNumberIdentifiers phoneNumberIdentifiers, final FaultTolerantRedisCluster cacheCluster, final AccountLockManager accountLockManager, - final DeletedAccounts deletedAccounts, final KeysManager keysManager, final MessagesManager messagesManager, final ProfilesManager profilesManager, @@ -162,7 +160,6 @@ public class AccountsManager { this.phoneNumberIdentifiers = phoneNumberIdentifiers; this.cacheCluster = cacheCluster; this.accountLockManager = accountLockManager; - this.deletedAccounts = deletedAccounts; this.keysManager = keysManager; this.messagesManager = messagesManager; this.profilesManager = profilesManager; @@ -201,7 +198,7 @@ public class AccountsManager { // Reuse the ACI from any recently-deleted account with this number to cover cases where somebody is // re-registering. - account.setUuid(deletedAccounts.findUuid(number).orElseGet(UUID::randomUUID)); + account.setUuid(accounts.findRecentlyDeletedAccountIdentifier(number).orElseGet(UUID::randomUUID)); account.addDevice(device); account.setRegistrationLockFromAttributes(accountAttributes); account.setUnidentifiedAccessKey(accountAttributes.getUnidentifiedAccessKey()); @@ -261,7 +258,7 @@ public class AccountsManager { // Clear any "recently deleted account" record for this number since, if it existed, we've used its old ACI for // the newly-created account. - deletedAccounts.remove(number); + accounts.removeRecentlyDeletedAccount(number); }); return account; @@ -303,7 +300,7 @@ public class AccountsManager { // 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 = deletedAccounts.findUuid(targetNumber); + final Optional recentlyDeletedAci = accounts.findRecentlyDeletedAccountIdentifier(targetNumber); final Optional maybeExistingAccount = getByE164(targetNumber); final Optional maybeDisplacedUuid; @@ -314,7 +311,7 @@ public class AccountsManager { maybeDisplacedUuid = recentlyDeletedAci; } - maybeDisplacedUuid.ifPresent(displacedUuid -> deletedAccounts.put(displacedUuid, originalNumber)); + maybeDisplacedUuid.ifPresent(displacedUuid -> accounts.putRecentlyDeletedAccount(displacedUuid, originalNumber)); final UUID uuid = account.getUuid(); final UUID phoneNumberIdentifier = phoneNumberIdentifiers.getPhoneNumberIdentifier(targetNumber); @@ -836,6 +833,14 @@ public class AccountsManager { return phoneNumberIdentifiers.getPhoneNumberIdentifier(e164); } + public Optional findRecentlyDeletedAccountIdentifier(final String e164) { + return accounts.findRecentlyDeletedAccountIdentifier(e164); + } + + public Optional findRecentlyDeletedE164(final UUID uuid) { + return accounts.findRecentlyDeletedE164(uuid); + } + public AccountCrawlChunk getAllFromDynamo(int length) { return accounts.getAllFromStart(length); } @@ -856,7 +861,7 @@ public class AccountsManager { delete(account); - deletedAccounts.put(accountIdentifier, e164); + accounts.putRecentlyDeletedAccount(accountIdentifier, e164); }); } catch (final RuntimeException | InterruptedException e) { logger.warn("Failed to delete account", e); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccounts.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccounts.java deleted file mode 100644 index 07e697a6d..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccounts.java +++ /dev/null @@ -1,87 +0,0 @@ -/* - * Copyright 2013-2021 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ -package org.whispersystems.textsecuregcm.storage; - -import java.time.Duration; -import java.time.Instant; -import java.util.Map; -import java.util.Optional; -import java.util.UUID; -import org.whispersystems.textsecuregcm.util.AttributeValues; -import software.amazon.awssdk.services.dynamodb.DynamoDbClient; -import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest; -import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; -import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; -import software.amazon.awssdk.services.dynamodb.model.PutItemRequest; -import software.amazon.awssdk.services.dynamodb.model.QueryRequest; -import software.amazon.awssdk.services.dynamodb.model.QueryResponse; - -public class DeletedAccounts { - - // e164, primary key - static final String KEY_ACCOUNT_E164 = "P"; - static final String ATTR_ACCOUNT_UUID = "U"; - static final String ATTR_EXPIRES = "E"; - - static final String UUID_TO_E164_INDEX_NAME = "u_to_p"; - - static final Duration TIME_TO_LIVE = Duration.ofDays(30); - - private final DynamoDbClient dynamoDbClient; - private final String tableName; - - public DeletedAccounts(final DynamoDbClient dynamoDbClient, final String tableName) { - this.dynamoDbClient = dynamoDbClient; - this.tableName = tableName; - } - - public void put(UUID uuid, String e164) { - dynamoDbClient.putItem(PutItemRequest.builder() - .tableName(tableName) - .item(Map.of( - KEY_ACCOUNT_E164, AttributeValues.fromString(e164), - ATTR_ACCOUNT_UUID, AttributeValues.fromUUID(uuid), - ATTR_EXPIRES, AttributeValues.fromLong(Instant.now().plus(TIME_TO_LIVE).getEpochSecond()))) - .build()); - } - - public Optional findUuid(final String e164) { - final GetItemResponse response = dynamoDbClient.getItem(GetItemRequest.builder() - .tableName(tableName) - .consistentRead(true) - .key(Map.of(KEY_ACCOUNT_E164, AttributeValues.fromString(e164))) - .build()); - - return Optional.ofNullable(AttributeValues.getUUID(response.item(), ATTR_ACCOUNT_UUID, null)); - } - - public Optional findE164(final UUID uuid) { - final QueryResponse response = dynamoDbClient.query(QueryRequest.builder() - .tableName(tableName) - .indexName(UUID_TO_E164_INDEX_NAME) - .keyConditionExpression("#uuid = :uuid") - .projectionExpression("#e164") - .expressionAttributeNames(Map.of("#uuid", ATTR_ACCOUNT_UUID, - "#e164", KEY_ACCOUNT_E164)) - .expressionAttributeValues(Map.of(":uuid", AttributeValues.fromUUID(uuid))).build()); - - if (response.count() == 0) { - return Optional.empty(); - } - - if (response.count() > 1) { - throw new RuntimeException("Impossible result: more than one phone number returned for UUID: " + uuid); - } - - return Optional.ofNullable(response.items().get(0).get(KEY_ACCOUNT_E164).s()); - } - - public void remove(final String e164) { - dynamoDbClient.deleteItem(DeleteItemRequest.builder() - .tableName(tableName) - .key(Map.of(KEY_ACCOUNT_E164, AttributeValues.fromString(e164))) - .build()); - } -} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/AssignUsernameCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/AssignUsernameCommand.java index 7a7b3da9d..e2f9b5a79 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/AssignUsernameCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/AssignUsernameCommand.java @@ -38,7 +38,6 @@ import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountLockManager; import org.whispersystems.textsecuregcm.storage.Accounts; import org.whispersystems.textsecuregcm.storage.AccountsManager; -import org.whispersystems.textsecuregcm.storage.DeletedAccounts; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; import org.whispersystems.textsecuregcm.storage.KeysManager; import org.whispersystems.textsecuregcm.storage.MessagesCache; @@ -141,8 +140,6 @@ public class AssignUsernameCommand extends EnvironmentCommand entity = Entity.entity(new SpamReport(new byte[3]), "application/json"); @@ -800,7 +797,7 @@ class MessageControllerTest { eq(AuthHelper.VALID_UUID), argThat(maybeBytes -> maybeBytes.map(bytes -> Arrays.equals(bytes, new byte[3])).orElse(false)), any()); - verify(deletedAccounts, never()).findE164(any(UUID.class)); + verify(accountsManager, never()).findRecentlyDeletedE164(any(UUID.class)); verify(accountsManager, never()).getPhoneNumberIdentifier(anyString()); when(accountsManager.getByAccountIdentifier(senderAci)).thenReturn(Optional.empty()); @@ -839,7 +836,7 @@ class MessageControllerTest { when(account.getPhoneNumberIdentifier()).thenReturn(senderPni); when(accountsManager.getByAccountIdentifier(senderAci)).thenReturn(Optional.of(account)); - when(deletedAccounts.findE164(senderAci)).thenReturn(Optional.of(senderNumber)); + when(accountsManager.findRecentlyDeletedE164(senderAci)).thenReturn(Optional.of(senderNumber)); when(accountsManager.getPhoneNumberIdentifier(senderNumber)).thenReturn(senderPni); Response response = 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 090c406de..39a7bc9d0 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java @@ -58,7 +58,6 @@ class AccountsManagerChangeNumberIntegrationTest { static final RedisClusterExtension CACHE_CLUSTER_EXTENSION = RedisClusterExtension.builder().build(); private ClientPresenceManager clientPresenceManager; - private DeletedAccounts deletedAccounts; private AccountsManager accountsManager; @@ -79,11 +78,9 @@ class AccountsManagerChangeNumberIntegrationTest { Tables.NUMBERS.tableName(), Tables.PNI_ASSIGNMENTS.tableName(), Tables.USERNAMES.tableName(), + Tables.DELETED_ACCOUNTS.tableName(), SCAN_PAGE_SIZE); - deletedAccounts = new DeletedAccounts(DYNAMO_DB_EXTENSION.getDynamoDbClient(), - Tables.DELETED_ACCOUNTS.tableName()); - final AccountLockManager accountLockManager = new AccountLockManager(DYNAMO_DB_EXTENSION.getDynamoDbClient(), Tables.DELETED_ACCOUNTS_LOCK.tableName()); @@ -112,7 +109,6 @@ class AccountsManagerChangeNumberIntegrationTest { phoneNumberIdentifiers, CACHE_CLUSTER_EXTENSION.getRedisCluster(), accountLockManager, - deletedAccounts, keysManager, messagesManager, mock(ProfilesManager.class), @@ -145,8 +141,8 @@ class AccountsManagerChangeNumberIntegrationTest { assertEquals(secondNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); - assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); - assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); } @Test @@ -178,8 +174,8 @@ class AccountsManagerChangeNumberIntegrationTest { assertEquals(secondNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); - assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); - assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); assertEquals(pniIdentityKey, updatedAccount.getIdentityKey(IdentityType.PNI)); @@ -209,8 +205,8 @@ class AccountsManagerChangeNumberIntegrationTest { assertEquals(originalNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); - assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); - assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); } @Test @@ -235,8 +231,8 @@ class AccountsManagerChangeNumberIntegrationTest { verify(clientPresenceManager).disconnectPresence(existingAccountUuid, Device.MASTER_ID); - assertEquals(Optional.of(existingAccountUuid), deletedAccounts.findUuid(originalNumber)); - assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + assertEquals(Optional.of(existingAccountUuid), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); accountsManager.changeNumber(accountsManager.getByAccountIdentifier(originalUuid).orElseThrow(), originalNumber, null, null, null, null); @@ -266,13 +262,13 @@ class AccountsManagerChangeNumberIntegrationTest { assertEquals(existingAccountUuid, reRegisteredAccount.getUuid()); assertEquals(originalPni, reRegisteredAccount.getPhoneNumberIdentifier()); - assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); - assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); final Account changedNumberReRegisteredAccount = accountsManager.changeNumber(reRegisteredAccount, secondNumber, null, null, null, null); - assertEquals(Optional.of(originalUuid), deletedAccounts.findUuid(originalNumber)); - assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + assertEquals(Optional.of(originalUuid), accountsManager.findRecentlyDeletedAccountIdentifier(originalNumber)); + assertEquals(Optional.empty(), accountsManager.findRecentlyDeletedAccountIdentifier(secondNumber)); assertEquals(secondPni, changedNumberReRegisteredAccount.getPhoneNumberIdentifier()); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java index d2587e64b..4780b86a0 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -23,7 +23,6 @@ import java.io.IOException; import java.time.Clock; import java.time.Instant; import java.util.ArrayList; -import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; @@ -63,7 +62,8 @@ class AccountsManagerConcurrentModificationIntegrationTest { static final DynamoDbExtension DYNAMO_DB_EXTENSION = new DynamoDbExtension( Tables.ACCOUNTS, Tables.NUMBERS, - Tables.PNI_ASSIGNMENTS + Tables.PNI_ASSIGNMENTS, + Tables.DELETED_ACCOUNTS ); private Accounts accounts; @@ -88,6 +88,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { Tables.NUMBERS.tableName(), Tables.PNI_ASSIGNMENTS.tableName(), Tables.USERNAMES.tableName(), + Tables.DELETED_ACCOUNTS.tableName(), SCAN_PAGE_SIZE); { @@ -103,9 +104,6 @@ class AccountsManagerConcurrentModificationIntegrationTest { return null; }).when(accountLockManager).withLock(any(), any()); - final DeletedAccounts deletedAccounts = mock(DeletedAccounts.class); - when(deletedAccounts.findUuid(any())).thenReturn(Optional.empty()); - final PhoneNumberIdentifiers phoneNumberIdentifiers = mock(PhoneNumberIdentifiers.class); when(phoneNumberIdentifiers.getPhoneNumberIdentifier(anyString())) .thenAnswer((Answer) invocation -> UUID.randomUUID()); @@ -115,7 +113,6 @@ class AccountsManagerConcurrentModificationIntegrationTest { phoneNumberIdentifiers, RedisClusterHelper.builder().stringCommands(commands).build(), accountLockManager, - deletedAccounts, mock(KeysManager.class), mock(MessagesManager.class), mock(ProfilesManager.class), 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 f82c67600..d61790e48 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -92,7 +92,6 @@ class AccountsManagerTest { private static final byte[] ENCRYPTED_USERNAME_2 = Base64.getUrlDecoder().decode(BASE_64_URL_ENCRYPTED_USERNAME_2); private Accounts accounts; - private DeletedAccounts deletedAccounts; private KeysManager keysManager; private MessagesManager messagesManager; private ProfilesManager profilesManager; @@ -125,7 +124,6 @@ class AccountsManagerTest { @BeforeEach void setup() throws InterruptedException { accounts = mock(Accounts.class); - deletedAccounts = mock(DeletedAccounts.class); keysManager = mock(KeysManager.class); messagesManager = mock(MessagesManager.class); profilesManager = mock(ProfilesManager.class); @@ -152,8 +150,6 @@ class AccountsManagerTest { return null; }).when(accounts).changeNumber(any(), anyString(), any()); - when(deletedAccounts.findUuid(anyString())).thenReturn(Optional.empty()); - final SecureStorageClient storageClient = mock(SecureStorageClient.class); when(storageClient.deleteStoredData(any())).thenReturn(CompletableFuture.completedFuture(null)); @@ -202,7 +198,6 @@ class AccountsManagerTest { .stringAsyncCommands(asyncCommands) .build(), accountLockManager, - deletedAccounts, keysManager, messagesManager, profilesManager, @@ -954,7 +949,7 @@ class AccountsManagerTest { void testCreateAccountRecentlyDeleted() throws InterruptedException { final UUID recentlyDeletedUuid = UUID.randomUUID(); - when(deletedAccounts.findUuid(anyString())).thenReturn(Optional.of(recentlyDeletedUuid)); + when(accounts.findRecentlyDeletedAccountIdentifier(anyString())).thenReturn(Optional.of(recentlyDeletedUuid)); when(accounts.create(any())).thenReturn(true); final String e164 = "+18005550123"; @@ -1036,7 +1031,7 @@ class AccountsManagerTest { account = accountsManager.changeNumber(account, number, null, null, null, null); assertEquals(number, account.getNumber()); - verify(deletedAccounts, never()).put(any(), any()); + verify(accounts, never()).putRecentlyDeletedAccount(any(), any()); verify(keysManager, never()).delete(any()); } @@ -1052,7 +1047,6 @@ class AccountsManagerTest { "AccountsManager should not allow use of changeNumber with new PNI keys but without changing number"); verify(accounts, never()).update(any()); - verifyNoInteractions(deletedAccounts); verifyNoInteractions(keysManager); } @@ -1210,7 +1204,6 @@ class AccountsManagerTest { updatedAccount.getDevices().stream().collect(Collectors.toMap(Device::getId, d -> d.getPhoneNumberIdentityRegistrationId().getAsInt()))); verify(accounts).update(any()); - verifyNoInteractions(deletedAccounts); verify(keysManager).delete(oldPni); } @@ -1264,7 +1257,6 @@ class AccountsManagerTest { updatedAccount.getDevices().stream().collect(Collectors.toMap(Device::getId, d -> d.getPhoneNumberIdentityRegistrationId().getAsInt()))); verify(accounts).update(any()); - verifyNoInteractions(deletedAccounts); verify(keysManager).delete(oldPni); verify(keysManager).storeEcSignedPreKeys(oldPni, newSignedKeys); @@ -1297,7 +1289,6 @@ class AccountsManagerTest { () -> accountsManager.updatePniKeys(account, pniIdentityKey, newSignedKeys, null, newRegistrationIds)); verifyNoInteractions(accounts); - verifyNoInteractions(deletedAccounts); verifyNoInteractions(keysManager); } @@ -1327,7 +1318,6 @@ class AccountsManagerTest { () -> accountsManager.updatePniKeys(account, pniIdentityKey, newSignedKeys, newSignedPqKeys, newRegistrationIds)); verifyNoInteractions(accounts); - verifyNoInteractions(deletedAccounts); verifyNoInteractions(keysManager); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java index 8ad5685c5..59694db81 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java @@ -28,7 +28,6 @@ import java.util.Base64; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.UUID; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -65,6 +64,7 @@ class AccountsManagerUsernameIntegrationTest { Tables.ACCOUNTS, Tables.NUMBERS, Tables.USERNAMES, + Tables.DELETED_ACCOUNTS, Tables.PNI, Tables.PNI_ASSIGNMENTS); @@ -94,6 +94,7 @@ class AccountsManagerUsernameIntegrationTest { Tables.NUMBERS.tableName(), Tables.PNI_ASSIGNMENTS.tableName(), Tables.USERNAMES.tableName(), + Tables.DELETED_ACCOUNTS.tableName(), SCAN_PAGE_SIZE)); final AccountLockManager accountLockManager = mock(AccountLockManager.class); @@ -105,9 +106,6 @@ class AccountsManagerUsernameIntegrationTest { return null; }).when(accountLockManager).withLock(any(), any()); - final DeletedAccounts deletedAccounts = mock(DeletedAccounts.class); - when(deletedAccounts.findUuid(any())).thenReturn(Optional.empty()); - final PhoneNumberIdentifiers phoneNumberIdentifiers = new PhoneNumberIdentifiers(DYNAMO_DB_EXTENSION.getDynamoDbClient(), Tables.PNI.tableName()); @@ -119,7 +117,6 @@ class AccountsManagerUsernameIntegrationTest { phoneNumberIdentifiers, CACHE_CLUSTER_EXTENSION.getRedisCluster(), accountLockManager, - deletedAccounts, mock(KeysManager.class), mock(MessagesManager.class), mock(ProfilesManager.class), 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 64458a4ff..de1fd7b38 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -98,7 +98,8 @@ class AccountsTest { Tables.ACCOUNTS, Tables.NUMBERS, Tables.PNI_ASSIGNMENTS, - Tables.USERNAMES); + Tables.USERNAMES, + Tables.DELETED_ACCOUNTS); private final TestClock clock = TestClock.pinned(Instant.EPOCH); private DynamicConfigurationManager mockDynamicConfigManager; @@ -121,6 +122,7 @@ class AccountsTest { Tables.NUMBERS.tableName(), Tables.PNI_ASSIGNMENTS.tableName(), Tables.USERNAMES.tableName(), + Tables.DELETED_ACCOUNTS.tableName(), SCAN_PAGE_SIZE); } @@ -166,7 +168,6 @@ class AccountsTest { mock(PhoneNumberIdentifiers.class), mock(FaultTolerantRedisCluster.class), mock(AccountLockManager.class), - mock(DeletedAccounts.class), mock(KeysManager.class), mock(MessagesManager.class), mock(ProfilesManager.class), @@ -442,7 +443,8 @@ class AccountsTest { final DynamoDbAsyncClient dynamoDbAsyncClient = mock(DynamoDbAsyncClient.class); accounts = new Accounts(mock(DynamoDbClient.class), dynamoDbAsyncClient, Tables.ACCOUNTS.tableName(), - Tables.NUMBERS.tableName(), Tables.PNI_ASSIGNMENTS.tableName(), Tables.USERNAMES.tableName(), SCAN_PAGE_SIZE); + Tables.NUMBERS.tableName(), Tables.PNI_ASSIGNMENTS.tableName(), Tables.USERNAMES.tableName(), + Tables.DELETED_ACCOUNTS.tableName(), SCAN_PAGE_SIZE); Exception e = TransactionConflictException.builder().build(); e = wrapException ? new CompletionException(e) : e; @@ -990,6 +992,59 @@ class AccountsTest { assertThat(account.getUsernameHash()).isEmpty(); } + @Test + void testPutFindRecentlyDeletedAccount() { + final UUID uuid = UUID.randomUUID(); + final String e164 = "+18005551234"; + + assertEquals(Optional.empty(), accounts.findRecentlyDeletedAccountIdentifier(e164)); + + accounts.putRecentlyDeletedAccount(uuid, e164); + + assertEquals(Optional.of(uuid), accounts.findRecentlyDeletedAccountIdentifier(e164)); + } + + @Test + void testRemoveRecentlyDeletedAccount() { + final UUID uuid = UUID.randomUUID(); + final String e164 = "+18005551234"; + + assertEquals(Optional.empty(), accounts.findRecentlyDeletedAccountIdentifier(e164)); + + accounts.putRecentlyDeletedAccount(uuid, e164); + + assertEquals(Optional.of(uuid), accounts.findRecentlyDeletedAccountIdentifier(e164)); + + accounts.removeRecentlyDeletedAccount(e164); + + assertEquals(Optional.empty(), accounts.findRecentlyDeletedAccountIdentifier(e164)); + } + + @Test + void testFindRecentlyDeletedE164() { + assertEquals(Optional.empty(), accounts.findRecentlyDeletedE164(UUID.randomUUID())); + + final UUID uuid = UUID.randomUUID(); + final String e164 = "+18005551234"; + + accounts.putRecentlyDeletedAccount(uuid, e164); + + assertEquals(Optional.of(e164), accounts.findRecentlyDeletedE164(uuid)); + } + + @Test + void testFindRecentlyDeletedUUID() { + final String e164 = "+18005551234"; + + assertEquals(Optional.empty(), accounts.findRecentlyDeletedAccountIdentifier(e164)); + + final UUID uuid = UUID.randomUUID(); + + accounts.putRecentlyDeletedAccount(uuid, e164); + + assertEquals(Optional.of(uuid), accounts.findRecentlyDeletedAccountIdentifier(e164)); + } + @Test public void testIgnoredFieldsNotAddedToDataAttribute() throws Exception { final Account account = generateAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID()); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsTest.java deleted file mode 100644 index 60954e7a9..000000000 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsTest.java +++ /dev/null @@ -1,80 +0,0 @@ -/* - * 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 java.util.Optional; -import java.util.UUID; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; -import org.whispersystems.textsecuregcm.storage.DynamoDbExtensionSchema.Tables; - -class DeletedAccountsTest { - - @RegisterExtension - static final DynamoDbExtension DYNAMO_DB_EXTENSION = new DynamoDbExtension(Tables.DELETED_ACCOUNTS); - - private DeletedAccounts deletedAccounts; - - @BeforeEach - void setUp() { - deletedAccounts = new DeletedAccounts(DYNAMO_DB_EXTENSION.getDynamoDbClient(), Tables.DELETED_ACCOUNTS.tableName()); - } - - @Test - void testPutFind() { - final UUID uuid = UUID.randomUUID(); - final String e164 = "+18005551234"; - - assertEquals(Optional.empty(), deletedAccounts.findUuid(e164)); - - deletedAccounts.put(uuid, e164); - - assertEquals(Optional.of(uuid), deletedAccounts.findUuid(e164)); - } - - @Test - void testRemove() { - final UUID uuid = UUID.randomUUID(); - final String e164 = "+18005551234"; - - assertEquals(Optional.empty(), deletedAccounts.findUuid(e164)); - - deletedAccounts.put(uuid, e164); - - assertEquals(Optional.of(uuid), deletedAccounts.findUuid(e164)); - - deletedAccounts.remove(e164); - - assertEquals(Optional.empty(), deletedAccounts.findUuid(e164)); - } - - @Test - void testFindE164() { - assertEquals(Optional.empty(), deletedAccounts.findE164(UUID.randomUUID())); - - final UUID uuid = UUID.randomUUID(); - final String e164 = "+18005551234"; - - deletedAccounts.put(uuid, e164); - - assertEquals(Optional.of(e164), deletedAccounts.findE164(uuid)); - } - - @Test - void testFindUUID() { - final String e164 = "+18005551234"; - - assertEquals(Optional.empty(), deletedAccounts.findUuid(e164)); - - final UUID uuid = UUID.randomUUID(); - - deletedAccounts.put(uuid, e164); - - assertEquals(Optional.of(uuid), deletedAccounts.findUuid(e164)); - } -} 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 8946dba2e..d167368b6 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/DynamoDbExtensionSchema.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/DynamoDbExtensionSchema.java @@ -63,21 +63,21 @@ public final class DynamoDbExtensionSchema { List.of()), DELETED_ACCOUNTS("deleted_accounts_test", - DeletedAccounts.KEY_ACCOUNT_E164, + Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_E164, null, List.of( AttributeDefinition.builder() - .attributeName(DeletedAccounts.KEY_ACCOUNT_E164) + .attributeName(Accounts.DELETED_ACCOUNTS_KEY_ACCOUNT_E164) .attributeType(ScalarAttributeType.S).build(), AttributeDefinition.builder() - .attributeName(DeletedAccounts.ATTR_ACCOUNT_UUID) + .attributeName(Accounts.DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID) .attributeType(ScalarAttributeType.B) .build()), List.of( GlobalSecondaryIndex.builder() - .indexName(DeletedAccounts.UUID_TO_E164_INDEX_NAME) + .indexName(Accounts.DELETED_ACCOUNTS_UUID_TO_E164_INDEX_NAME) .keySchema( - KeySchemaElement.builder().attributeName(DeletedAccounts.ATTR_ACCOUNT_UUID).keyType(KeyType.HASH).build() + KeySchemaElement.builder().attributeName(Accounts.DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID).keyType(KeyType.HASH).build() ) .projection(Projection.builder().projectionType(ProjectionType.KEYS_ONLY).build()) .provisionedThroughput(ProvisionedThroughput.builder().readCapacityUnits(10L).writeCapacityUnits(10L).build()) @@ -86,10 +86,10 @@ public final class DynamoDbExtensionSchema { ), DELETED_ACCOUNTS_LOCK("deleted_accounts_lock_test", - DeletedAccounts.KEY_ACCOUNT_E164, + AccountLockManager.KEY_ACCOUNT_E164, null, List.of(AttributeDefinition.builder() - .attributeName(DeletedAccounts.KEY_ACCOUNT_E164) + .attributeName(AccountLockManager.KEY_ACCOUNT_E164) .attributeType(ScalarAttributeType.S).build()), List.of(), List.of()),