Skip to content

Commit cfac798

Browse files
katherine-signalkatherine-signal
authored
Handle account reclamation with equivalent phone numbers
1 parent 34e8e04 commit cfac798

3 files changed

Lines changed: 285 additions & 13 deletions

File tree

‎service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java‎

Lines changed: 130 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import static java.util.Objects.requireNonNull;
88
import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name;
9+
import static org.whispersystems.textsecuregcm.util.Util.getAlternateForms;
910

1011
import com.fasterxml.jackson.core.JsonProcessingException;
1112
import com.fasterxml.jackson.databind.ObjectWriter;
@@ -371,7 +372,28 @@ CompletionStage<Void> reclaimAccount(final Account existingAccount,
371372
.build())
372373
.build());
373374
}
374-
writeItems.add(UpdateAccountSpec.forAccount(accountsTableName, accountToCreate).transactItem());
375+
376+
// Phone number canonicalization means that a user can use a different phone number in the same equivalence class
377+
// to reclaim the account.
378+
if (!existingAccount.getNumber().equals(accountToCreate.getNumber())) {
379+
if (getAlternateForms(existingAccount.getNumber()).contains(accountToCreate.getNumber())) {
380+
final AttributeValue uuidAttr = AttributeValues.fromUUID(existingAccount.getUuid());
381+
final AttributeValue numberAttr = AttributeValues.fromString(accountToCreate.getNumber());
382+
final TransactWriteItem phoneNumberConstraintPut = buildConstraintTablePutIfAbsent(
383+
phoneNumberConstraintTableName, uuidAttr, ATTR_ACCOUNT_E164, numberAttr);
384+
385+
writeItems.add(buildDelete(phoneNumberConstraintTableName, ATTR_ACCOUNT_E164, existingAccount.getNumber()));
386+
writeItems.add(phoneNumberConstraintPut);
387+
} else {
388+
log.error("Reclaiming account with a non-equivalent phone number. Old account {}:{}:{}, new account {}:{}:{}",
389+
existingAccount.getUuid(), existingAccount.getNumber(), existingAccount.getPhoneNumberIdentifier(),
390+
accountToCreate.getUuid(), accountToCreate.getNumber(), accountToCreate.getPhoneNumberIdentifier());
391+
throw new IllegalArgumentException("reclaimed accounts must have equivalent phone numbers");
392+
}
393+
}
394+
395+
final int updateAccountItemIndex = writeItems.size();
396+
writeItems.add(UpdateAccountSpec.forReclaimedAccount(accountsTableName, accountToCreate, existingAccount.getNumber()).transactItem());
375397
writeItems.addAll(additionalWriteItems);
376398

377399
return dynamoDbAsyncClient.transactWriteItems(TransactWriteItemsRequest.builder().transactItems(writeItems).build())
@@ -382,6 +404,16 @@ CompletionStage<Void> reclaimAccount(final Account existingAccount,
382404
.exceptionally(throwable -> {
383405
final Throwable unwrapped = ExceptionUtils.unwrap(throwable);
384406
if (unwrapped instanceof TransactionCanceledException te) {
407+
if (Accounts.conditionalCheckFailed(te.cancellationReasons().get(updateAccountItemIndex))) {
408+
final Map<String, AttributeValue> item = te.cancellationReasons().get(updateAccountItemIndex).item();
409+
final String existingNumber = AttributeValues.getString(item, Accounts.ATTR_ACCOUNT_E164, null);
410+
if (!existingAccount.getNumber().equals(existingNumber)) {
411+
log.error("Failed to update account due to unexpected existing phone number. Account {}. Expected {}, got {}",
412+
existingAccount.getUuid(), existingAccount.getNumber(), existingNumber);
413+
throw new UnexpectedExistingPhoneNumberException();
414+
}
415+
}
416+
385417
if (te.cancellationReasons().stream().anyMatch(Accounts::conditionalCheckFailed)) {
386418
throw new ContestedOptimisticLockException();
387419
}
@@ -889,6 +921,37 @@ public void clearUsernameHash(final Account account) throws ContestedOptimisticL
889921
}
890922
}
891923

924+
record UpdateExpression (List<String> setClauses, List<String> addClauses, List<String> removeClauses) {
925+
public String toExpressionString() {
926+
final StringBuilder updateExpressionBuilder = new StringBuilder();
927+
928+
if (!setClauses.isEmpty()) {
929+
updateExpressionBuilder.append("SET ");
930+
updateExpressionBuilder.append(String.join(", ", setClauses));
931+
}
932+
933+
if (!removeClauses.isEmpty()) {
934+
if (!updateExpressionBuilder.isEmpty()) {
935+
updateExpressionBuilder.append(" ");
936+
}
937+
938+
updateExpressionBuilder.append("REMOVE ");
939+
updateExpressionBuilder.append(String.join(", ", removeClauses));
940+
}
941+
942+
if (!addClauses.isEmpty()) {
943+
if (!updateExpressionBuilder.isEmpty()) {
944+
updateExpressionBuilder.append(" ");
945+
}
946+
947+
updateExpressionBuilder.append("ADD ");
948+
updateExpressionBuilder.append(String.join(", ", addClauses));
949+
}
950+
951+
return updateExpressionBuilder.toString();
952+
}
953+
}
954+
892955
/**
893956
* A ddb update that can be used as part of a transaction or single-item update statement.
894957
*/
@@ -897,13 +960,13 @@ record UpdateAccountSpec(
897960
Map<String, AttributeValue> key,
898961
Map<String, String> attrNames,
899962
Map<String, AttributeValue> attrValues,
900-
String updateExpression,
963+
UpdateExpression updateExpression,
901964
String conditionExpression) {
902965
UpdateItemRequest updateItemRequest() {
903966
return UpdateItemRequest.builder()
904967
.tableName(tableName)
905968
.key(key)
906-
.updateExpression(updateExpression)
969+
.updateExpression(updateExpression.toExpressionString())
907970
.conditionExpression(conditionExpression)
908971
.expressionAttributeNames(attrNames)
909972
.expressionAttributeValues(attrValues)
@@ -914,17 +977,46 @@ TransactWriteItem transactItem() {
914977
return TransactWriteItem.builder().update(Update.builder()
915978
.tableName(tableName)
916979
.key(key)
917-
.updateExpression(updateExpression)
980+
.updateExpression(updateExpression.toExpressionString())
918981
.conditionExpression(conditionExpression)
982+
.returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD)
919983
.expressionAttributeNames(attrNames)
920984
.expressionAttributeValues(attrValues)
921985
.build()).build();
922986
}
923987

988+
static UpdateAccountSpec forReclaimedAccount(
989+
final String accountTableName,
990+
final Account account,
991+
final String expectedExistingE164) {
992+
final UpdateAccountSpec base = forAccount(accountTableName, account);
993+
994+
final Map<String, AttributeValue> attrValues = new HashMap<>(base.attrValues());
995+
attrValues.put(":number", AttributeValues.fromString(account.getNumber()));
996+
997+
final UpdateExpression updateExpression = base.updateExpression();
998+
final List<String> setClauses = new ArrayList<>(updateExpression.setClauses());
999+
setClauses.add("#number = :number");
1000+
1001+
final MembershipExpression membershipExpression = MembershipExpression.build(getAlternateForms(expectedExistingE164));
1002+
attrValues.putAll(membershipExpression.values());
1003+
1004+
// Defensive check: we should only update the e164 to another e164 in the same equivalence class
1005+
final String conditionExpression = base.conditionExpression() + " AND #number IN %s".formatted(membershipExpression.expression());
1006+
1007+
return new UpdateAccountSpec(
1008+
base.tableName(),
1009+
base.key(),
1010+
base.attrNames(),
1011+
attrValues,
1012+
new UpdateExpression(setClauses, updateExpression.addClauses(), updateExpression.removeClauses()),
1013+
conditionExpression
1014+
);
1015+
}
1016+
9241017
static UpdateAccountSpec forAccount(
9251018
final String accountTableName,
9261019
final Account account) {
927-
// username, e164, and pni cannot be modified through this method
9281020
final Map<String, String> attrNames = new HashMap<>(Map.of(
9291021
"#number", ATTR_ACCOUNT_E164,
9301022
"#data", ATTR_ACCOUNT_DATA,
@@ -937,19 +1029,20 @@ static UpdateAccountSpec forAccount(
9371029
":version", AttributeValues.fromInt(account.getVersion()),
9381030
":version_increment", AttributeValues.fromInt(1)));
9391031

940-
final StringBuilder updateExpressionBuilder = new StringBuilder("SET #data = :data, #cds = :cds");
1032+
final List<String> setClauses = new ArrayList<>(List.of("#data = :data", "#cds = :cds"));
1033+
9411034
if (account.getUnidentifiedAccessKey().isPresent()) {
9421035
// if it's present in the account, also set the uak
9431036
attrNames.put("#uak", ATTR_UAK);
9441037
attrValues.put(":uak", AttributeValues.fromByteArray(account.getUnidentifiedAccessKey().get()));
945-
updateExpressionBuilder.append(", #uak = :uak");
1038+
setClauses.add("#uak = :uak");
9461039
}
9471040

9481041
if (account.getUsernameHash().isPresent()) {
9491042
// if it's present in the account, also set the username hash
9501043
attrNames.put("#usernameHash", ATTR_USERNAME_HASH);
9511044
attrValues.put(":usernameHash", AttributeValues.fromByteArray(account.getUsernameHash().get()));
952-
updateExpressionBuilder.append(", #usernameHash = :usernameHash");
1045+
setClauses.add("#usernameHash = :usernameHash");
9531046
}
9541047

9551048
// If the account has a username/handle pair, we should add it to the top level attributes.
@@ -959,7 +1052,7 @@ static UpdateAccountSpec forAccount(
9591052
if (account.getEncryptedUsername().isPresent() && account.getUsernameLinkHandle() != null) {
9601053
attrNames.put("#ul", ATTR_USERNAME_LINK_UUID);
9611054
attrValues.put(":ul", AttributeValues.fromUUID(account.getUsernameLinkHandle()));
962-
updateExpressionBuilder.append(", #ul = :ul");
1055+
setClauses.add("#ul = :ul");
9631056
}
9641057

9651058
// Some operations may remove the usernameLink or the usernameHash (re-registration, clear username link, and
@@ -974,17 +1067,20 @@ static UpdateAccountSpec forAccount(
9741067
attrNames.put("#username_hash", ATTR_USERNAME_HASH);
9751068
removes.add("#username_hash");
9761069
}
1070+
1071+
final List<String> removeClauses = new ArrayList<>();
9771072
if (!removes.isEmpty()) {
978-
updateExpressionBuilder.append(" REMOVE %s".formatted(String.join(",", removes)));
1073+
removeClauses.add(String.join(",", removes));
9791074
}
980-
updateExpressionBuilder.append(" ADD #version :version_increment");
1075+
1076+
final List<String> addClauses = List.of("#version :version_increment");
9811077

9821078
return new UpdateAccountSpec(
9831079
accountTableName,
9841080
Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid())),
9851081
attrNames,
9861082
attrValues,
987-
updateExpressionBuilder.toString(),
1083+
new UpdateExpression(setClauses, addClauses, removeClauses),
9881084
"attribute_exists(#number) AND #version = :version");
9891085
}
9901086
}
@@ -1584,4 +1680,26 @@ private static String redactPhoneNumber(final String phoneNumber) {
15841680
: phoneNumber.substring(phoneNumber.length() - 2));
15851681
return sb.toString();
15861682
}
1683+
1684+
record MembershipExpression(String expression, Map<String, AttributeValue> values) {
1685+
public static MembershipExpression build(final Collection<String> values) {
1686+
if (values.isEmpty()) {
1687+
throw new IllegalArgumentException("must have at least one value");
1688+
}
1689+
1690+
final Map<String, AttributeValue> expressionValues = new HashMap<>();
1691+
final List<String> placeholders = new ArrayList<>();
1692+
final String prefix = "val";
1693+
int i = 0;
1694+
for (final String value : values) {
1695+
String key = ":" + prefix + i;
1696+
placeholders.add(key);
1697+
expressionValues.put(key, AttributeValues.fromString(value));
1698+
i++;
1699+
}
1700+
1701+
final String expression = "(" + String.join(", ", placeholders) + ")";
1702+
return new MembershipExpression(expression, expressionValues);
1703+
}
1704+
}
15871705
}

‎service/src/main/java/org/whispersystems/textsecuregcm/storage/UnexpectedExistingPhoneNumberException.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
/*
2+
* Copyright 2026 Signal Messenger, LLC
3+
* SPDX-License-Identifier: AGPL-3.0-only
4+
*/
5+
6+
package org.whispersystems.textsecuregcm.storage;
7+
8+
public class UnexpectedExistingPhoneNumberException extends RuntimeException {
9+
}

0 commit comments

Comments
 (0)