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 031603ace..db8f4ea6c 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -331,8 +331,7 @@ public class AccountController { @HeaderParam("User-Agent") String userAgent, @QueryParam("transfer") Optional availableForTransfer, @Valid AccountAttributes accountAttributes) - throws RateLimitExceededException - { + throws RateLimitExceededException, InterruptedException { try { AuthorizationHeader header = AuthorizationHeader.fromFullHeader(authorizationHeader); String number = header.getIdentifier().getNumber(); 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 05de23b7c..180e952e6 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -143,91 +143,107 @@ public class AccountsManager { public Account create(final String number, final String password, final String signalAgent, - final AccountAttributes accountAttributes) { + final AccountAttributes accountAttributes) throws InterruptedException { try (Timer.Context ignored = createTimer.time()) { - Optional maybeExistingAccount = get(number); + final Account account = new Account(); - Device device = new Device(); - device.setId(Device.MASTER_ID); - device.setAuthenticationCredentials(new AuthenticationCredentials(password)); - device.setFetchesMessages(accountAttributes.getFetchesMessages()); - device.setRegistrationId(accountAttributes.getRegistrationId()); - device.setName(accountAttributes.getName()); - device.setCapabilities(accountAttributes.getCapabilities()); - device.setCreated(System.currentTimeMillis()); - device.setLastSeen(Util.todayInMillis()); - device.setUserAgent(signalAgent); + deletedAccountsManager.lockAndTake(number, maybeRecentlyDeletedUuid -> { + Device device = new Device(); + device.setId(Device.MASTER_ID); + device.setAuthenticationCredentials(new AuthenticationCredentials(password)); + device.setFetchesMessages(accountAttributes.getFetchesMessages()); + device.setRegistrationId(accountAttributes.getRegistrationId()); + device.setName(accountAttributes.getName()); + device.setCapabilities(accountAttributes.getCapabilities()); + device.setCreated(System.currentTimeMillis()); + device.setLastSeen(Util.todayInMillis()); + device.setUserAgent(signalAgent); - Account account = new Account(); - account.setNumber(number); - account.setUuid(UUID.randomUUID()); - account.addDevice(device); - account.setRegistrationLockFromAttributes(accountAttributes); - account.setUnidentifiedAccessKey(accountAttributes.getUnidentifiedAccessKey()); - account.setUnrestrictedUnidentifiedAccess(accountAttributes.isUnrestrictedUnidentifiedAccess()); - account.setDiscoverableByPhoneNumber(accountAttributes.isDiscoverableByPhoneNumber()); + account.setNumber(number); + account.setUuid(maybeRecentlyDeletedUuid.orElseGet(UUID::randomUUID)); + account.addDevice(device); + account.setRegistrationLockFromAttributes(accountAttributes); + account.setUnidentifiedAccessKey(accountAttributes.getUnidentifiedAccessKey()); + account.setUnrestrictedUnidentifiedAccess(accountAttributes.isUnrestrictedUnidentifiedAccess()); + account.setDiscoverableByPhoneNumber(accountAttributes.isDiscoverableByPhoneNumber()); - final UUID originalUuid = account.getUuid(); - boolean freshUser = databaseCreate(account); + final UUID originalUuid = account.getUuid(); - // databaseCreate() sometimes updates the UUID, if there was a number conflict. - // for metrics, we want dynamo to run with the same original UUID - final UUID actualUuid = account.getUuid(); + boolean freshUser = databaseCreate(account); - try { - if (dynamoWriteEnabled()) { + // databaseCreate() sometimes updates the UUID, if there was a number conflict. + // for metrics, we want dynamo to run with the same original UUID + final UUID actualUuid = account.getUuid(); - account.setUuid(originalUuid); + try { + if (dynamoWriteEnabled()) { - runSafelyAndRecordMetrics(() -> dynamoCreate(account), Optional.of(account.getUuid()), freshUser, - (databaseResult, dynamoResult) -> { + account.setUuid(originalUuid); - if (!account.getUuid().equals(actualUuid)) { - logger.warn("dynamoCreate() did not return correct UUID"); - } + runSafelyAndRecordMetrics(() -> dynamoCreate(account), Optional.of(account.getUuid()), freshUser, + (databaseResult, dynamoResult) -> { - if (databaseResult.equals(dynamoResult)) { - return Optional.empty(); - } + if (!account.getUuid().equals(actualUuid)) { + logger.warn("dynamoCreate() did not return correct UUID"); + } - if (dynamoResult) { - return Optional.of("dynamoFreshUser"); - } + if (databaseResult.equals(dynamoResult)) { + return Optional.empty(); + } - return Optional.of("dbFreshUser"); - }, - "create"); + if (dynamoResult) { + return Optional.of("dynamoFreshUser"); + } + + return Optional.of("dbFreshUser"); + }, + "create"); + } + } finally { + account.setUuid(actualUuid); } - } finally { - account.setUuid(actualUuid); - } - redisSet(account); + redisSet(account); - final Tags tags; + pendingAccounts.remove(number); - if (freshUser) { - tags = Tags.of("type", "new"); - newUserMeter.mark(); - } else { - tags = Tags.of("type", "reregister"); - } + // In terms of previously-existing accounts, there are three possible cases: + // + // 1. This is a completely new account; there was no pre-existing account and no recently-deleted account + // 2. This is a re-registration of an existing account. The storage layer will update the existing account in + // place to match the account record created above, and will update the UUID of the newly-created account + // instance to match the stored account record (i.e. originalUuid != actualUuid). + // 3. This is a re-registration of a recently-deleted account, in which case maybeRecentlyDeletedUuid is + // present. + // + // All cases are mutually-exclusive. In the first case, we don't need to do anything. In the third, we can be + // confident that everything has already been deleted. In the second case, though, we're taking over an existing + // account and need to clear out messages and keys that may have been stored for the old account. + if (!originalUuid.equals(actualUuid)) { + messagesManager.clear(actualUuid); + keysDynamoDb.delete(actualUuid); + } - Metrics.counter(CREATE_COUNTER_NAME, tags).increment(); + final Tags tags; - if (!account.isDiscoverableByPhoneNumber()) { - // The newly-created account has explicitly opted out of discoverability - directoryQueue.deleteAccount(account); - } + if (freshUser) { + tags = Tags.of("type", "new"); + newUserMeter.mark(); + } else if (!originalUuid.equals(actualUuid)) { + tags = Tags.of("type", "re-registration"); + } else { + tags = Tags.of("type", "recently-deleted"); + } - maybeExistingAccount.ifPresent(definitelyExistingAccount -> { - messagesManager.clear(definitelyExistingAccount.getUuid()); - keysDynamoDb.delete(definitelyExistingAccount.getUuid()); + Metrics.counter(CREATE_COUNTER_NAME, tags).increment(); + + if (!account.isDiscoverableByPhoneNumber()) { + // The newly-created account has explicitly opted out of discoverability + directoryQueue.deleteAccount(account); + } }); - pendingAccounts.remove(number); - return account; } } 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 0e86f0471..1cd72928b 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -12,6 +12,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -22,11 +23,9 @@ import com.opentable.db.postgres.junit5.PreparedDbExtension; import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands; import java.io.IOException; import java.time.Instant; -import java.util.Arrays; -import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.Random; -import java.util.Set; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; @@ -88,7 +87,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { private Executor mutationExecutor = new ThreadPoolExecutor(20, 20, 5, TimeUnit.SECONDS, new LinkedBlockingDeque<>(20)); @BeforeEach - void setup() { + void setup() throws InterruptedException { { CreateTableRequest createNumbersTableRequest = CreateTableRequest.builder() @@ -145,11 +144,19 @@ class AccountsManagerConcurrentModificationIntegrationTest { commands = mock(RedisAdvancedClusterCommands.class); + final DeletedAccountsManager deletedAccountsManager = mock(DeletedAccountsManager.class); + + doAnswer(invocation -> { + //noinspection unchecked + invocation.getArgument(1, Consumer.class).accept(Optional.empty()); + return null; + }).when(deletedAccountsManager).lockAndTake(anyString(), any()); + accountsManager = new AccountsManager( accounts, accountsDynamoDb, RedisClusterHelper.buildMockRedisCluster(commands), - mock(DeletedAccountsManager.class), + deletedAccountsManager, mock(DirectoryQueue.class), mock(KeysDynamoDb.class), mock(MessagesManager.class), @@ -164,7 +171,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { } @Test - void testConcurrentUpdate() throws IOException { + void testConcurrentUpdate() throws IOException, InterruptedException { final UUID uuid; { 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 e6218a182..cfaf3eec3 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 @@ -11,6 +11,7 @@ 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; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.doAnswer; @@ -68,9 +69,12 @@ class AccountsManagerTest { private Accounts accounts; private AccountsDynamoDb accountsDynamoDb; + private DeletedAccountsManager deletedAccountsManager; private DirectoryQueue directoryQueue; private DynamicConfigurationManager dynamicConfigurationManager; private ExperimentEnrollmentManager experimentEnrollmentManager; + private KeysDynamoDb keys; + private MessagesManager messagesManager; private RedisAdvancedClusterCommands commands; private AccountsManager accountsManager; @@ -84,12 +88,15 @@ class AccountsManagerTest { }; @BeforeEach - void setup() { + void setup() throws InterruptedException { accounts = mock(Accounts.class); accountsDynamoDb = mock(AccountsDynamoDb.class); + deletedAccountsManager = mock(DeletedAccountsManager.class); directoryQueue = mock(DirectoryQueue.class); dynamicConfigurationManager = mock(DynamicConfigurationManager.class); experimentEnrollmentManager = mock(ExperimentEnrollmentManager.class); + keys = mock(KeysDynamoDb.class); + messagesManager = mock(MessagesManager.class); //noinspection unchecked commands = mock(RedisAdvancedClusterCommands.class); @@ -97,14 +104,20 @@ class AccountsManagerTest { final DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(); when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration); + doAnswer(invocation -> { + //noinspection unchecked + invocation.getArgument(1, Consumer.class).accept(Optional.empty()); + return null; + }).when(deletedAccountsManager).lockAndTake(anyString(), any()); + accountsManager = new AccountsManager( accounts, accountsDynamoDb, RedisClusterHelper.buildMockRedisCluster(commands), - mock(DeletedAccountsManager.class), + deletedAccountsManager, directoryQueue, - mock(KeysDynamoDb.class), - mock(MessagesManager.class), + keys, + messagesManager, mock(UsernamesManager.class), mock(ProfilesManager.class), mock(StoredVerificationCodeManager.class), @@ -521,9 +534,61 @@ class AccountsManagerTest { assertEquals(Optional.of("profileName"), accountsManager.compareAccounts(Optional.of(a1), Optional.of(a2))); } + @Test + void testCreateFreshAccount() throws InterruptedException { + when(accounts.create(any())).thenReturn(true); + + final String e164 = "+18005550123"; + final AccountAttributes attributes = new AccountAttributes(false, 0, null, null, null, true, null); + accountsManager.create(e164, "password", null, attributes); + + verify(accounts).create(argThat(account -> e164.equals(account.getNumber()))); + verifyNoInteractions(keys); + verifyNoInteractions(messagesManager); + } + + @Test + void testReregisterAccount() throws InterruptedException { + final UUID existingUuid = UUID.randomUUID(); + + when(accounts.create(any())).thenAnswer(invocation -> { + invocation.getArgument(0, Account.class).setUuid(existingUuid); + return false; + }); + + final String e164 = "+18005550123"; + final AccountAttributes attributes = new AccountAttributes(false, 0, null, null, null, true, null); + accountsManager.create(e164, "password", null, attributes); + + verify(accounts).create(argThat(account -> e164.equals(account.getNumber()) && existingUuid.equals(account.getUuid()))); + verify(keys).delete(existingUuid); + verify(messagesManager).clear(existingUuid); + } + + @Test + void testCreateAccountRecentlyDeleted() throws InterruptedException { + final UUID recentlyDeletedUuid = UUID.randomUUID(); + + doAnswer(invocation -> { + //noinspection unchecked + invocation.getArgument(1, Consumer.class).accept(Optional.of(recentlyDeletedUuid)); + return null; + }).when(deletedAccountsManager).lockAndTake(anyString(), any()); + + when(accounts.create(any())).thenReturn(true); + + final String e164 = "+18005550123"; + final AccountAttributes attributes = new AccountAttributes(false, 0, null, null, null, true, null); + accountsManager.create(e164, "password", null, attributes); + + verify(accounts).create(argThat(account -> e164.equals(account.getNumber()) && recentlyDeletedUuid.equals(account.getUuid()))); + verifyNoInteractions(keys); + verifyNoInteractions(messagesManager); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) - void testCreateWithDiscoverability(final boolean discoverable) { + void testCreateWithDiscoverability(final boolean discoverable) throws InterruptedException { final AccountAttributes attributes = new AccountAttributes(false, 0, null, null, null, discoverable, null); final Account account = accountsManager.create("+18005550123", "password", null, attributes); @@ -536,7 +601,7 @@ class AccountsManagerTest { @ParameterizedTest @ValueSource(booleans = {true, false}) - void testCreateWithStorageCapability(final boolean hasStorage) { + void testCreateWithStorageCapability(final boolean hasStorage) throws InterruptedException { final AccountAttributes attributes = new AccountAttributes(false, 0, null, null, null, true, new DeviceCapabilities(false, false, false, hasStorage, false, false, false, false));