MirrorMirror
/
Signal-Server
mirror of https://github.com/signalapp/Signal-Server.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Reuse account UUIDs when registering an account with a recently-deleted e164.

This commit is contained in:
Jon Chambers 2021-07-26 10:21:40 -04:00 committed by Jon Chambers
parent be20c04cd8
commit 3a966ef345
4 changed files with 164 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java
View File

@ -331,8 +331,7 @@ public class AccountController {
@HeaderParam("User-Agent") String userAgent,
@QueryParam("transfer") Optional<Boolean> availableForTransfer,
@Valid AccountAttributes accountAttributes)
throws RateLimitExceededException
{
throws RateLimitExceededException, InterruptedException {
try {
AuthorizationHeader header = AuthorizationHeader.fromFullHeader(authorizationHeader);
String number = header.getIdentifier().getNumber();

142
service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java
View File

@ -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<Account> 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;
}
}

19
service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java
View File

@ -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;
{

77
service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java
View File

@ -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<String, String> 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));