Skip to content

Commit

Permalink
Read deleted accounts by PNI rather than e164
Browse files Browse the repository at this point in the history
  • Loading branch information
@jkt-signal
jkt-signal authored Nov 27, 2024
1 parent 0e04cac commit 557a6ec
Show file tree
Hide file tree
Showing 14 changed files with 220 additions and 387 deletions.
6 changes: 2 additions & 4 deletions service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,6 @@
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;
Expand Down Expand Up @@ -333,7 +332,6 @@ public void initialize(final Bootstrap<WhisperServerConfiguration> bootstrap) {
"Processes scheduled jobs to send notifications to idle devices",
new IdleDeviceNotificationSchedulerFactory()));

bootstrap.addCommand(new MigrateDeletedAccountsCommand());
bootstrap.addCommand(new DeleteE164RegistrationRecoveryPasswordsCommand());
bootstrap.addCommand(new BackfillBeninPhoneNumberFormsCommand());
}
Expand Down Expand Up @@ -1104,8 +1102,8 @@ protected void configureServer(final ServerBuilder<?> serverBuilder) {
new KeysController(rateLimiters, keysManager, accountsManager, zkSecretParams, Clock.systemUTC()),
new KeyTransparencyController(keyTransparencyServiceClient),
new MessageController(rateLimiters, messageByteLimitCardinalityEstimator, messageSender, receiptSender,
accountsManager, messagesManager, pushNotificationManager, pushNotificationScheduler, reportMessageManager,
multiRecipientMessageExecutor, messageDeliveryScheduler, clientReleaseManager,
accountsManager, messagesManager, phoneNumberIdentifiers, pushNotificationManager, pushNotificationScheduler,
reportMessageManager, multiRecipientMessageExecutor, messageDeliveryScheduler, clientReleaseManager,
dynamicConfigurationManager, zkSecretParams, spamChecker, messageMetrics, messageDeliveryLoopMonitor,
Clock.systemUTC()),
new PaymentsController(currencyManager, paymentsCredentialsGenerator),
Expand Down
15 changes: 10 additions & 5 deletions service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@
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.ReportMessageManager;
import org.whispersystems.textsecuregcm.util.DestinationDeviceValidator;
import org.whispersystems.textsecuregcm.util.ExceptionUtils;
Expand Down Expand Up @@ -157,6 +158,7 @@ private record MultiRecipientDeliveryData(
private final ReceiptSender receiptSender;
private final AccountsManager accountsManager;
private final MessagesManager messagesManager;
private final PhoneNumberIdentifiers phoneNumberIdentifiers;
private final PushNotificationManager pushNotificationManager;
private final PushNotificationScheduler pushNotificationScheduler;
private final ReportMessageManager reportMessageManager;
Expand Down Expand Up @@ -216,6 +218,7 @@ public MessageController(
ReceiptSender receiptSender,
AccountsManager accountsManager,
MessagesManager messagesManager,
PhoneNumberIdentifiers phoneNumberIdentifiers,
PushNotificationManager pushNotificationManager,
PushNotificationScheduler pushNotificationScheduler,
ReportMessageManager reportMessageManager,
Expand All @@ -234,6 +237,7 @@ public MessageController(
this.receiptSender = receiptSender;
this.accountsManager = accountsManager;
this.messagesManager = messagesManager;
this.phoneNumberIdentifiers = phoneNumberIdentifiers;
this.pushNotificationManager = pushNotificationManager;
this.pushNotificationScheduler = pushNotificationScheduler;
this.reportMessageManager = reportMessageManager;
Expand Down Expand Up @@ -853,19 +857,19 @@ public Response reportSpamMessage(
@Nullable SpamReport spamReport,
@HeaderParam(HttpHeaders.USER_AGENT) String userAgent
) {

final Optional<String> sourceNumber;
final Optional<UUID> sourceAci;
final Optional<UUID> sourcePni;

if (source.startsWith("+")) {
sourceNumber = Optional.of(source);
final Optional<Account> maybeAccount = accountsManager.getByE164(source);
if (maybeAccount.isPresent()) {
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));
Expand All @@ -874,8 +878,9 @@ public Response reportSpamMessage(

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);
Expand Down
82 changes: 19 additions & 63 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 @@ -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";

Expand Down Expand Up @@ -1166,7 +1169,7 @@ private TransactWriteItem buildPutDeletedAccount(final UUID uuid, final String e
.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())
Expand All @@ -1178,7 +1181,7 @@ private TransactWriteItem buildPutDeletedAccount(final UUID aci, final UUID pni)
.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())
Expand All @@ -1189,7 +1192,7 @@ 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)))
.key(Map.of(DELETED_ACCOUNTS_KEY_ACCOUNT_PNI, AttributeValues.fromString(e164)))
.build())
.build();
}
Expand All @@ -1198,7 +1201,7 @@ private TransactWriteItem buildRemoveDeletedAccount(final UUID pni) {
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();
}
Expand All @@ -1210,44 +1213,35 @@ public CompletableFuture<Optional<Account>> getByAccountIdentifierAsync(final UU
.toCompletableFuture();
}

public Optional<UUID> 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<UUID> 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<String> findRecentlyDeletedE164(final UUID uuid) {
public Optional<UUID> 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) {
return Optional.empty();
}

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<Void> delete(final UUID uuid, final List<TransactWriteItem> additionalWriteItems) {
Expand Down Expand Up @@ -1313,51 +1307,13 @@ public Flux<Tuple3<String, UUID, Long>> getE164KeyedDeletedAccounts(final int se
.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<Boolean> 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<Account> getByIndirectLookup(
final Timer timer,
Expand Down
39 changes: 13 additions & 26 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 @@ -277,7 +277,7 @@ public Account create(final String number,
return createTimer.record(() -> {
accountLockManager.withLock(List.of(pni), () -> {
final Optional<UUID> 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.
Expand Down Expand Up @@ -654,16 +654,16 @@ public Account changeNumber(final Account account,

// 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<UUID> recentlyDeletedAci = accounts.findRecentlyDeletedAccountIdentifier(targetNumber);
// 3. No account with the target PNI exists at all, in which case no additional action is needed.
final Optional<UUID> recentlyDeletedAci = accounts.findRecentlyDeletedAccountIdentifier(targetPhoneNumberIdentifier);
final Optional<Account> maybeExistingAccount = getByE164(targetNumber);
final Optional<UUID> maybeDisplacedUuid;

Expand Down Expand Up @@ -1205,31 +1205,18 @@ public UUID getPhoneNumberIdentifier(String e164) {
return phoneNumberIdentifiers.getPhoneNumberIdentifier(e164).join();
}

public Optional<UUID> findRecentlyDeletedAccountIdentifier(final String e164) {
return accounts.findRecentlyDeletedAccountIdentifier(e164);
public Optional<UUID> findRecentlyDeletedAccountIdentifier(final UUID phoneNumberIdentifier) {
return accounts.findRecentlyDeletedAccountIdentifier(phoneNumberIdentifier);
}

public Optional<String> findRecentlyDeletedE164(final UUID uuid) {
return accounts.findRecentlyDeletedE164(uuid);
public Optional<UUID> findRecentlyDeletedPhoneNumberIdentifier(final UUID accountIdentifier) {
return accounts.findRecentlyDeletedPhoneNumberIdentifier(accountIdentifier);
}

public Flux<Account> streamAllFromDynamo(final int segments, final Scheduler scheduler) {
return accounts.getAll(segments, scheduler);
}

public Flux<Tuple3<String, UUID, Long>> getE164KeyedDeletedAccounts(final int segments, final Scheduler scheduler) {
return accounts.getE164KeyedDeletedAccounts(segments, scheduler);
}

public CompletableFuture<Boolean> 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<Void> delete(final Account account, final DeletionReason deletionReason) {
final Timer.Sample sample = Timer.start();

Expand Down
Loading

0 comments on commit 557a6ec

Please sign in to comment.