Skip to content

Commit

Permalink
Update recently-deleted accounts table transactionally as part of acc…
Browse files Browse the repository at this point in the history 
…ount mutations
  • Loading branch information
@jon-signal
jon-signal committed Sep 13, 2023
1 parent 1b9bf01 commit f0544fa
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 93 deletions.
51 changes: 33 additions & 18 deletions service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,9 @@
import software.amazon.awssdk.services.dynamodb.model.CancellationReason;
import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException;
import software.amazon.awssdk.services.dynamodb.model.Delete;
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.Put;
import software.amazon.awssdk.services.dynamodb.model.PutItemRequest;
import software.amazon.awssdk.services.dynamodb.model.QueryRequest;
import software.amazon.awssdk.services.dynamodb.model.QueryResponse;
import software.amazon.awssdk.services.dynamodb.model.ReturnValuesOnConditionCheckFailure;
Expand Down Expand Up @@ -200,8 +198,12 @@ public boolean create(final Account account) {

final TransactWriteItem accountPut = buildAccountPut(account, uuidAttr, numberAttr, pniUuidAttr);

// Clear any "recently deleted account" record for this number since, if it existed, we've used its old ACI for
// the newly-created account.
final TransactWriteItem deletedAccountDelete = buildRemoveDeletedAccount(account.getNumber());

final TransactWriteItemsRequest request = TransactWriteItemsRequest.builder()
.transactItems(phoneNumberConstraintPut, phoneNumberIdentifierConstraintPut, accountPut)
.transactItems(phoneNumberConstraintPut, phoneNumberIdentifierConstraintPut, accountPut, deletedAccountDelete)
.build();

try {
Expand Down Expand Up @@ -270,7 +272,11 @@ public boolean create(final Account account) {
* @param account the account for which to change the phone number
* @param number the new phone number
*/
public void changeNumber(final Account account, final String number, final UUID phoneNumberIdentifier) {
public void changeNumber(final Account account,
final String number,
final UUID phoneNumberIdentifier,
final Optional<UUID> maybeDisplacedAccountIdentifier) {

CHANGE_NUMBER_TIMER.record(() -> {
final String originalNumber = account.getNumber();
final UUID originalPni = account.getPhoneNumberIdentifier();
Expand All @@ -289,6 +295,10 @@ public void changeNumber(final Account account, final String number, final UUID
writeItems.add(buildConstraintTablePut(phoneNumberConstraintTableName, uuidAttr, ATTR_ACCOUNT_E164, numberAttr));
writeItems.add(buildDelete(phoneNumberIdentifierConstraintTableName, ATTR_PNI_UUID, originalPni));
writeItems.add(buildConstraintTablePut(phoneNumberIdentifierConstraintTableName, uuidAttr, ATTR_PNI_UUID, pniAttr));
writeItems.add(buildRemoveDeletedAccount(number));
maybeDisplacedAccountIdentifier.ifPresent(displacedAccountIdentifier ->
writeItems.add(buildPutDeletedAccount(displacedAccountIdentifier, originalNumber)));

writeItems.add(
TransactWriteItem.builder()
.update(Update.builder()
Expand Down Expand Up @@ -717,21 +727,25 @@ public Optional<Account> getByAccountIdentifier(final UUID uuid) {
.map(Accounts::fromItem)));
}

public void putRecentlyDeletedAccount(final UUID uuid, final String e164) {
db().putItem(PutItemRequest.builder()
.tableName(deletedAccountsTableName)
.item(Map.of(
DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164),
DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, AttributeValues.fromUUID(uuid),
DELETED_ACCOUNTS_ATTR_EXPIRES, AttributeValues.fromLong(Instant.now().plus(DELETED_ACCOUNTS_TIME_TO_LIVE).getEpochSecond())))
.build());
private TransactWriteItem buildPutDeletedAccount(final UUID uuid, final String e164) {
return TransactWriteItem.builder()
.put(Put.builder()
.tableName(deletedAccountsTableName)
.item(Map.of(
DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164),
DELETED_ACCOUNTS_ATTR_ACCOUNT_UUID, AttributeValues.fromUUID(uuid),
DELETED_ACCOUNTS_ATTR_EXPIRES, AttributeValues.fromLong(Instant.now().plus(DELETED_ACCOUNTS_TIME_TO_LIVE).getEpochSecond())))
.build())
.build();
}

public void removeRecentlyDeletedAccount(final String e164) {
db().deleteItem(DeleteItemRequest.builder()
.tableName(deletedAccountsTableName)
.key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164)))
.build());
private TransactWriteItem buildRemoveDeletedAccount(final String e164) {
return TransactWriteItem.builder()
.delete(Delete.builder()
.tableName(deletedAccountsTableName)
.key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_E164, AttributeValues.fromString(e164)))
.build())
.build();
}

@Nonnull
Expand Down Expand Up @@ -778,7 +792,8 @@ public void delete(final UUID uuid) {
final List<TransactWriteItem> transactWriteItems = new ArrayList<>(List.of(
buildDelete(phoneNumberConstraintTableName, ATTR_ACCOUNT_E164, account.getNumber()),
buildDelete(accountsTableName, KEY_ACCOUNT_UUID, uuid),
buildDelete(phoneNumberIdentifierConstraintTableName, ATTR_PNI_UUID, account.getPhoneNumberIdentifier())
buildDelete(phoneNumberIdentifierConstraintTableName, ATTR_PNI_UUID, account.getPhoneNumberIdentifier()),
buildPutDeletedAccount(uuid, account.getNumber())
));

account.getUsernameHash().ifPresent(usernameHash -> transactWriteItems.add(
Expand Down
17 changes: 2 additions & 15 deletions service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,6 @@ public Account create(final String number,

accountAttributes.recoveryPassword().ifPresent(registrationRecoveryPassword ->
registrationRecoveryPasswordsManager.storeForCurrentNumber(account.getNumber(), registrationRecoveryPassword));

// Clear any "recently deleted account" record for this number since, if it existed, we've used its old ACI for
// the newly-created account.
accounts.removeRecentlyDeletedAccount(number);
});

return account;
Expand Down Expand Up @@ -311,8 +307,6 @@ public Account changeNumber(final Account account,
maybeDisplacedUuid = recentlyDeletedAci;
}

maybeDisplacedUuid.ifPresent(displacedUuid -> accounts.putRecentlyDeletedAccount(displacedUuid, originalNumber));

final UUID uuid = account.getUuid();
final UUID phoneNumberIdentifier = phoneNumberIdentifiers.getPhoneNumberIdentifier(targetNumber);

Expand All @@ -324,7 +318,7 @@ public Account changeNumber(final Account account,
setPniKeys(account, pniIdentityKey, pniSignedPreKeys, pniRegistrationIds);
return true;
},
a -> accounts.changeNumber(a, targetNumber, phoneNumberIdentifier),
a -> accounts.changeNumber(a, targetNumber, phoneNumberIdentifier, maybeDisplacedUuid),
() -> accounts.getByAccountIdentifier(uuid).orElseThrow(),
AccountChangeValidator.NUMBER_CHANGE_VALIDATOR);

Expand Down Expand Up @@ -855,14 +849,7 @@ public ParallelFlux<Account> streamAllFromDynamo(final int segments, final Sched

public void delete(final Account account, final DeletionReason deletionReason) throws InterruptedException {
try (final Timer.Context ignored = deleteTimer.time()) {
accountLockManager.withLock(List.of(account.getNumber()), () -> {
final UUID accountIdentifier = account.getUuid();
final String e164 = account.getNumber();

delete(account);

accounts.putRecentlyDeletedAccount(accountIdentifier, e164);
});
accountLockManager.withLock(List.of(account.getNumber()), () -> delete(account));
} catch (final RuntimeException | InterruptedException e) {
logger.warn("Failed to delete account", e);
throw e;
Expand Down
3 changes: 1 addition & 2 deletions service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ void setup() throws InterruptedException {
account.setNumber(number, phoneNumberIdentifier);

return null;
}).when(accounts).changeNumber(any(), anyString(), any());
}).when(accounts).changeNumber(any(), anyString(), any(), any());

final SecureStorageClient storageClient = mock(SecureStorageClient.class);
when(storageClient.deleteStoredData(any())).thenReturn(CompletableFuture.completedFuture(null));
Expand Down Expand Up @@ -1031,7 +1031,6 @@ void testChangePhoneNumberSameNumber() throws InterruptedException, MismatchedDe
account = accountsManager.changeNumber(account, number, null, null, null, null);

assertEquals(number, account.getNumber());
verify(accounts, never()).putRecentlyDeletedAccount(any(), any());
verify(keysManager, never()).delete(any());
}

Expand Down
108 changes: 50 additions & 58 deletions service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,15 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.commons.lang3.RandomUtils;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration;
import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager;
Expand Down Expand Up @@ -228,6 +231,34 @@ void testStore() {
assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier(), account.getUuid());
}

@Test
void testStoreRecentlyDeleted() {
final UUID originalUuid = UUID.randomUUID();

Device device = generateDevice(1);
Account account = generateAccount("+14151112222", originalUuid, UUID.randomUUID(), List.of(device));

boolean freshUser = accounts.create(account);

assertThat(freshUser).isTrue();
verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier(), null, account, true);

assertPhoneNumberConstraintExists("+14151112222", account.getUuid());
assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier(), account.getUuid());

accounts.delete(originalUuid);
assertThat(accounts.findRecentlyDeletedAccountIdentifier(account.getNumber())).hasValue(originalUuid);

freshUser = accounts.create(account);
assertThat(freshUser).isTrue();
verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier(), null, account, true);

assertPhoneNumberConstraintExists("+14151112222", account.getUuid());
assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier(), account.getUuid());

assertThat(accounts.findRecentlyDeletedAccountIdentifier(account.getNumber())).isEmpty();
}

@Test
void testStoreMulti() {
final List<Device> devices = List.of(generateDevice(1), generateDevice(2));
Expand Down Expand Up @@ -536,6 +567,8 @@ void testDelete() {
accounts.create(deletedAccount);
accounts.create(retainedAccount);

assertThat(accounts.findRecentlyDeletedAccountIdentifier(deletedAccount.getNumber())).isEmpty();

assertPhoneNumberConstraintExists("+14151112222", deletedAccount.getUuid());
assertPhoneNumberIdentifierConstraintExists(deletedAccount.getPhoneNumberIdentifier(), deletedAccount.getUuid());
assertPhoneNumberConstraintExists("+14151112345", retainedAccount.getUuid());
Expand All @@ -547,6 +580,7 @@ void testDelete() {
accounts.delete(deletedAccount.getUuid());

assertThat(accounts.getByAccountIdentifier(deletedAccount.getUuid())).isNotPresent();
assertThat(accounts.findRecentlyDeletedAccountIdentifier(deletedAccount.getNumber())).hasValue(deletedAccount.getUuid());

assertPhoneNumberConstraintDoesNotExist(deletedAccount.getNumber());
assertPhoneNumberIdentifierConstraintDoesNotExist(deletedAccount.getPhoneNumberIdentifier());
Expand Down Expand Up @@ -637,8 +671,10 @@ void testCanonicallyDiscoverableSet() {
verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier(), null, account, false);
}

@Test
public void testChangeNumber() {
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
@ParameterizedTest
@MethodSource
public void testChangeNumber(final Optional<UUID> maybeDisplacedAccountIdentifier) {
final String originalNumber = "+14151112222";
final String targetNumber = "+14151113333";

Expand All @@ -662,7 +698,7 @@ public void testChangeNumber() {
verifyStoredState(originalNumber, account.getUuid(), account.getPhoneNumberIdentifier(), null, retrieved.get(), account);
}

accounts.changeNumber(account, targetNumber, targetPni);
accounts.changeNumber(account, targetNumber, targetPni, maybeDisplacedAccountIdentifier);

assertThat(accounts.getByE164(originalNumber)).isEmpty();
assertThat(accounts.getByAccountIdentifier(originalPni)).isEmpty();
Expand All @@ -681,6 +717,15 @@ public void testChangeNumber() {
assertThat(retrieved.get().getPhoneNumberIdentifier()).isEqualTo(targetPni);
assertThat(accounts.getByPhoneNumberIdentifier(targetPni)).isPresent();
}

assertThat(accounts.findRecentlyDeletedAccountIdentifier(originalNumber)).isEqualTo(maybeDisplacedAccountIdentifier);
}

private static Stream<Arguments> testChangeNumber() {
return Stream.of(
Arguments.of(Optional.empty()),
Arguments.of(Optional.of(UUID.randomUUID()))
);
}

@Test
Expand All @@ -700,7 +745,7 @@ public void testChangeNumberConflict() {
accounts.create(account);
accounts.create(existingAccount);

assertThrows(TransactionCanceledException.class, () -> accounts.changeNumber(account, targetNumber, targetPni));
assertThrows(TransactionCanceledException.class, () -> accounts.changeNumber(account, targetNumber, targetPni, Optional.of(existingAccount.getUuid())));

assertPhoneNumberConstraintExists(originalNumber, account.getUuid());
assertPhoneNumberIdentifierConstraintExists(originalPni, account.getUuid());
Expand Down Expand Up @@ -736,7 +781,7 @@ public void testChangeNumberPhoneNumberIdentifierConflict() {
Map.of(":uuid", AttributeValues.fromUUID(existingAccountIdentifier)))
.build());

assertThrows(TransactionCanceledException.class, () -> accounts.changeNumber(account, targetNumber, existingPhoneNumberIdentifier));
assertThrows(TransactionCanceledException.class, () -> accounts.changeNumber(account, targetNumber, existingPhoneNumberIdentifier, Optional.empty()));
}

@Test
Expand Down Expand Up @@ -992,59 +1037,6 @@ void testReserveConfirmUsernameHashVersionConflict() {
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());
Expand Down

0 comments on commit f0544fa

Please sign in to comment.