Signal/Signal-Server
1
0
Fork 0
mirror of https://github.com/signalapp/Signal-Server synced 2026-04-19 19:48:04 +01:00
Code Issues Packages Projects Releases Wiki Activity

Remove "no action on change to same number" optimization for "change number" operations

Browse Source
This commit is contained in:
Jon Chambers
2025-07-11 12:28:53 -04:00
committed by Jon Chambers
parent e62b3d390f
commit a36fba061a
5 changed files with 37 additions and 224 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java
View File

@@ -11,7 +11,6 @@ import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectWriter;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
import com.google.i18n.phonenumbers.PhoneNumberUtil;
import io.micrometer.core.instrument.Metrics;
import io.micrometer.core.instrument.Timer;
import java.io.IOException;
@@ -424,11 +423,14 @@ public class Accounts {
final AttributeValue numberAttr = AttributeValues.fromString(number);
final AttributeValue pniAttr = AttributeValues.fromUUID(phoneNumberIdentifier);
writeItems.add(buildDelete(phoneNumberConstraintTableName, ATTR_ACCOUNT_E164, originalNumber));
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(phoneNumberIdentifier));
if (!originalNumber.equals(number)) {
writeItems.add(buildDelete(phoneNumberConstraintTableName, ATTR_ACCOUNT_E164, originalNumber));
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(phoneNumberIdentifier));
}
maybeDisplacedAccountIdentifier.ifPresent(displacedAccountIdentifier ->
writeItems.add(buildPutDeletedAccount(displacedAccountIdentifier, originalPni)));

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

@@ -663,15 +663,10 @@ public class AccountsManager extends RedisPubSubAdapter<String, String> implemen
final Map<Byte, KEMSignedPreKey> pniPqLastResortPreKeys,
final Map<Byte, Integer> pniRegistrationIds) throws InterruptedException, MismatchedDevicesException {
final UUID originalPhoneNumberIdentifier = account.getPhoneNumberIdentifier();
final UUID targetPhoneNumberIdentifier = phoneNumberIdentifiers.getPhoneNumberIdentifier(targetNumber).join();
if (originalPhoneNumberIdentifier.equals(targetPhoneNumberIdentifier)) {
return account;
}
try {
return accountLockManager.withLock(Set.of(account.getPhoneNumberIdentifier(), targetPhoneNumberIdentifier),
return accountLockManager.withLock(new HashSet<>(List.of(account.getPhoneNumberIdentifier(), targetPhoneNumberIdentifier)),
() -> changeNumber(account, targetNumber, targetPhoneNumberIdentifier, pniIdentityKey, pniSignedPreKeys, pniPqLastResortPreKeys, pniRegistrationIds), accountLockExecutor);
} catch (final Exception e) {
if (e instanceof MismatchedDevicesException mismatchedDevicesException) {
@@ -699,24 +694,30 @@ public class AccountsManager extends RedisPubSubAdapter<String, String> implemen
redisDelete(account);
// There are three possible states for accounts associated with the target phone number:
// There are four possible states for accounts associated with the target phone number:
//
// 1. An account exists with the target PNI; the caller has proved ownership of the number, so delete the
// 1. The authenticated account already has the given phone number. We don't want to delete the account, but do want
// to update keys.
// 2. 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
// 3. 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 PNI exists at all, in which case no additional action is needed.
// 4. 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;
if (maybeExistingAccount.isPresent()) {
delete(maybeExistingAccount.get()).join();
maybeDisplacedUuid = maybeExistingAccount.map(Account::getUuid);
if (maybeExistingAccount.get().getIdentifier(IdentityType.ACI).equals(account.getIdentifier(IdentityType.ACI))) {
maybeDisplacedUuid = Optional.empty();
} else {
delete(maybeExistingAccount.get()).join();
maybeDisplacedUuid = maybeExistingAccount.map(Account::getUuid);
}
} else {
maybeDisplacedUuid = recentlyDeletedAci;
}

32
service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java
View File

@@ -10,7 +10,6 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
import org.apache.commons.lang3.ObjectUtils;
import org.signal.libsignal.protocol.IdentityKey;
import org.slf4j.Logger;
@@ -57,20 +56,6 @@ public class ChangeNumberManager {
throw new IllegalArgumentException("PNI identity key, signed pre-keys, device messages, and registration IDs must be all null or all non-null");
}
if (number.equals(account.getNumber())) {
// The client has gotten confused/desynchronized with us about their own phone number, most likely due to losing
// our OK response to an immediately preceding change-number request, and are sending a change they don't realize
// is a no-op change.
//
// We don't need to actually do a number-change operation in our DB, but we *do* need to accept their new key
// material and distribute the sync messages, to be sure all clients agree with us and each other about what their
// keys are. Pretend this change-number request was actually a PNI key distribution request.
if (pniIdentityKey == null) {
return account;
}
return updatePniKeys(account, pniIdentityKey, deviceSignedPreKeys, devicePqLastResortPreKeys, deviceMessages, pniRegistrationIds, senderUserAgent);
}
final Account updatedAccount = accountsManager.changeNumber(
account, number, pniIdentityKey, deviceSignedPreKeys, devicePqLastResortPreKeys, pniRegistrationIds);
@@ -81,23 +66,6 @@ public class ChangeNumberManager {
return updatedAccount;
}
public Account updatePniKeys(final Account account,
final IdentityKey pniIdentityKey,
final Map<Byte, ECSignedPreKey> deviceSignedPreKeys,
@Nullable final Map<Byte, KEMSignedPreKey> devicePqLastResortPreKeys,
final List<IncomingMessage> deviceMessages,
final Map<Byte, Integer> pniRegistrationIds,
final String senderUserAgent) throws MismatchedDevicesException, MessageTooLargeException {
// Don't try to be smart about ignoring unnecessary retries. If we make literally no change we will skip the ddb
// write anyway. Linked devices can handle some wasted extra key rotations.
final Account updatedAccount = accountsManager.updatePniKeys(
account, pniIdentityKey, deviceSignedPreKeys, devicePqLastResortPreKeys, pniRegistrationIds);
sendDeviceMessages(updatedAccount, deviceMessages, senderUserAgent);
return updatedAccount;
}
private void sendDeviceMessages(final Account account,
final List<IncomingMessage> deviceMessages,
final String senderUserAgent) throws MessageTooLargeException, MismatchedDevicesException {