Prevent storage service loops on ACI identity key changes

This commit is contained in:
AsamK 2026-09-10 16:26:41 +02:00
parent 48731286eb
commit 7cc0cc3e0e
6 changed files with 131 additions and 30 deletions

View File

@ -103,8 +103,16 @@ public final class ProfileHelper {
return getRecipientProfiles(recipientIds, false);
}
public void refreshRecipientProfile(RecipientId recipientId) {
getRecipientProfile(recipientId, true);
public boolean refreshRecipientProfile(RecipientId recipientId) {
try {
blockingGetProfile(retrieveProfile(recipientId, SignalServiceProfile.RequestType.PROFILE, false));
return true;
} catch (IOException e) {
logger.warn("Failed to retrieve profile for {}, ignoring: {}",
context.getRecipientHelper().resolveSignalServiceAddress(recipientId).getIdentifier(),
e.getMessage());
return false;
}
}
public void refreshRecipientProfiles(Collection<RecipientId> recipientIds) {

View File

@ -46,6 +46,7 @@ import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import static org.asamk.signal.manager.util.Utils.handleResponseException;
@ -88,6 +89,7 @@ public class StorageHelper {
final var result = storageServiceRepository.getStorageManifestIfDifferentVersion(storageKey,
localManifestVersion);
final var fetchedRemoteManifest = result instanceof ManifestIfDifferentVersionResult.DifferentVersion;
final Set<StorageId> identityConflictsPendingRepair = new HashSet<>();
var needsForcePush = false;
final var remoteManifest = switch (result) {
@ -116,13 +118,16 @@ public class StorageHelper {
if (remoteManifest.version > localManifestVersion) {
logger.trace("Remote version was newer, reading records.");
needsForcePush = readDataFromStorage(storageKey, localManifest, remoteManifest);
needsForcePush = readDataFromStorage(storageKey,
localManifest,
remoteManifest,
identityConflictsPendingRepair);
} else if (remoteManifest.version < localManifest.version) {
logger.debug("Remote storage manifest version was older. User might have switched accounts.");
}
logger.trace("Done reading data from remote storage");
readRecordsWithPreviouslyUnknownTypes(storageKey, remoteManifest);
readRecordsWithPreviouslyUnknownTypes(storageKey, remoteManifest, identityConflictsPendingRepair);
}
logger.trace("Adding missing storageIds to local data");
@ -153,7 +158,8 @@ public class StorageHelper {
needsMultiDeviceSync = writeToStorage(storageKey,
remoteManifest,
needsForcePush,
fetchedRemoteManifest);
fetchedRemoteManifest,
identityConflictsPendingRepair);
} catch (RetryLaterException e) {
// TODO retry later
return;
@ -198,7 +204,8 @@ public class StorageHelper {
private boolean readDataFromStorage(
final StorageKey storageKey,
final SignalStorageManifest localManifest,
final SignalStorageManifest remoteManifest
final SignalStorageManifest remoteManifest,
final Set<StorageId> identityConflictsPendingRepair
) throws IOException {
var needsForcePush = false;
try (final var connection = account.getAccountDatabase().getConnection()) {
@ -225,7 +232,9 @@ public class StorageHelper {
remoteOnlyRecords.size());
}
final var listListPair = processKnownRecords(connection, remoteOnlyRecords);
final var listListPair = processKnownRecords(connection,
remoteOnlyRecords,
identityConflictsPendingRepair);
final var unknownInserts = listListPair.first();
final var updatedStorageIds = listListPair.second();
final var oldUnregisteredLocalOnlyIds = new HashSet<>(idDifference.localOnlyIds());
@ -268,7 +277,8 @@ public class StorageHelper {
private void readRecordsWithPreviouslyUnknownTypes(
final StorageKey storageKey,
final SignalStorageManifest remoteManifest
final SignalStorageManifest remoteManifest,
final Set<StorageId> identityConflictsPendingRepair
) throws IOException {
try (final var connection = account.getAccountDatabase().getConnection()) {
connection.setAutoCommit(false);
@ -282,7 +292,7 @@ public class StorageHelper {
logger.debug("Found {} of the known-unknowns remotely.", remote.size());
processKnownRecords(connection, remote);
processKnownRecords(connection, remote, identityConflictsPendingRepair);
account.getUnknownStorageIdStore()
.deleteUnknownStorageIds(connection, remote.stream().map(SignalStorageRecord::getId).toList());
}
@ -296,7 +306,8 @@ public class StorageHelper {
final StorageKey storageKey,
final SignalStorageManifest remoteManifest,
final boolean needsForcePush,
final boolean fetchedRemoteManifest
final boolean fetchedRemoteManifest,
final Set<StorageId> identityConflictsPendingRepair
) throws IOException, RetryLaterException {
final WriteOperationResult remoteWriteOperation;
try (final var connection = account.getAccountDatabase().getConnection()) {
@ -338,6 +349,15 @@ public class StorageHelper {
return false;
}
final var onlyIdentityConflictsPendingRepair = containsOnlyIdentityConflictsPendingRepair(remoteWriteOperation,
identityConflictsPendingRepair);
if (onlyIdentityConflictsPendingRepair) {
logger.warn(
"Deferring remote write until the profile fetch says whose identity key is correct. WriteOperationResult :: {}",
remoteWriteOperation);
return false;
}
final var loopCheck = storageSyncLoopDetector.onWriteAttempt(remoteWriteOperation,
fetchedRemoteManifest,
false);
@ -386,6 +406,15 @@ public class StorageHelper {
}
}
static boolean containsOnlyIdentityConflictsPendingRepair(
final WriteOperationResult writeOperation,
final Set<StorageId> identityConflictsPendingRepair
) {
return !writeOperation.inserts().isEmpty() && writeOperation.inserts()
.stream()
.allMatch(record -> identityConflictsPendingRepair.contains(record.getId()));
}
private void forcePushToStorage(
final StorageKey storageServiceKey
) throws IOException, RetryLaterException {
@ -705,7 +734,8 @@ public class StorageHelper {
private Pair<List<StorageId>, List<StorageId>> processKnownRecords(
final Connection connection,
List<SignalStorageRecord> records
List<SignalStorageRecord> records,
final Set<StorageId> identityConflictsPendingRepair
) throws SQLException {
final var unknownRecords = new ArrayList<StorageId>();
final var processedRecords = new ArrayList<StorageId>();
@ -713,7 +743,10 @@ public class StorageHelper {
final var accountRecordProcessor = new AccountRecordProcessor(account, connection, context.getJobExecutor());
final var groupV1RecordProcessor = new GroupV1RecordProcessor(account, connection);
final var groupV2RecordProcessor = new GroupV2RecordProcessor(account, connection);
final var contactRecordProcessor = new ContactRecordProcessor(account, connection, context.getJobExecutor());
final var contactRecordProcessor = new ContactRecordProcessor(account,
connection,
context.getJobExecutor(),
identityConflictsPendingRepair);
final var stickerPackRecordProcessor = new StickerPackRecordProcessor(account, connection);
final var contactRecords = records.stream()

View File

@ -9,9 +9,15 @@ public class DownloadProfileJob implements Job {
private static final Logger logger = LoggerFactory.getLogger(DownloadProfileJob.class);
private final RecipientAddress address;
private final boolean resolveIdentityKeyConflict;
public DownloadProfileJob(RecipientAddress address) {
this(address, false);
}
public DownloadProfileJob(RecipientAddress address, boolean resolveIdentityKeyConflict) {
this.address = address;
this.resolveIdentityKeyConflict = resolveIdentityKeyConflict;
}
@Override
@ -19,6 +25,9 @@ public class DownloadProfileJob implements Job {
logger.trace("Refreshing profile for {}", address);
final var account = context.getAccount();
final var recipientId = account.getRecipientStore().resolveRecipient(address);
context.getProfileHelper().refreshRecipientProfile(recipientId);
final var refreshed = context.getProfileHelper().refreshRecipientProfile(recipientId);
if (refreshed && resolveIdentityKeyConflict) {
context.getJobExecutor().enqueueJob(new SyncStorageJob());
}
}
}

View File

@ -27,6 +27,7 @@ import java.util.Arrays;
import java.util.Collection;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
import okio.ByteString;
@ -47,11 +48,18 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
private final SignalAccount account;
private final Connection connection;
private final JobExecutor jobExecutor;
private final Set<StorageId> identityConflictsPendingRepair;
public ContactRecordProcessor(SignalAccount account, Connection connection, final JobExecutor jobExecutor) {
public ContactRecordProcessor(
SignalAccount account,
Connection connection,
final JobExecutor jobExecutor,
final Set<StorageId> identityConflictsPendingRepair
) {
this.account = account;
this.connection = connection;
this.jobExecutor = jobExecutor;
this.identityConflictsPendingRepair = identityConflictsPendingRepair;
this.selfAci = account.getAci();
this.selfPni = account.getPni();
this.selfNumber = account.getNumber();
@ -88,10 +96,12 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
final long localUnregisteredAtTimestamp,
final boolean unrepairableIdentityKeyConflict
) {
return remoteIdentityKeySize > 0 && (statesDiffer
|| localIdentityKeySize == 0
|| localUnregisteredAtTimestamp > 0
|| (unrepairableIdentityKeyConflict && !isPrimaryDevice));
return remoteIdentityKeySize > 0 && (
statesDiffer || localIdentityKeySize == 0 || localUnregisteredAtTimestamp > 0 || (
unrepairableIdentityKeyConflict
&& !isPrimaryDevice
)
);
}
@Override
@ -177,17 +187,17 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
final var localIdentityKeySize = local.identityKey.size();
final var statesDiffer = remote.identityState != local.identityState;
final var identityKeysExistAndConflict = remoteIdentityKeySize > 0
&& localIdentityKeySize > 0
&& !remote.identityKey.equals(local.identityKey);
&& localIdentityKeySize > 0
&& !remote.identityKey.equals(local.identityKey);
final var conflictAci = firstNonNull(localAci, remoteAci);
final var unrepairableIdentityKeyConflict = identityKeysExistAndConflict && conflictAci == null;
if (shouldUseRemoteIdentityKey(account.isPrimaryDevice(),
statesDiffer,
remoteIdentityKeySize,
localIdentityKeySize,
local.unregisteredAtTimestamp,
unrepairableIdentityKeyConflict)) {
statesDiffer,
remoteIdentityKeySize,
localIdentityKeySize,
local.unregisteredAtTimestamp,
unrepairableIdentityKeyConflict)) {
identityState = remote.identityState;
identityKey = remote.identityKey;
} else {
@ -235,7 +245,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
if (identityKeysExistAndConflict) {
if (conflictAci != null) {
logger.debug("Identity keys conflict for {}. Enqueueing a profile fetch.", conflictAci);
jobExecutor.enqueueJob(new DownloadProfileJob(new RecipientAddress(conflictAci, pni, e164)));
jobExecutor.enqueueJob(new DownloadProfileJob(new RecipientAddress(conflictAci, pni, e164), true));
} else {
logger.debug("Identity keys conflict for {}. No ACI, so no profile fetch is possible.", localPni);
}
@ -268,8 +278,8 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
.unregisteredAtTimestamp(remote.unregisteredAtTimestamp)
.hidden(remote.hidden)
.pniSignatureVerified((remote.pniSignatureVerified || local.pniSignatureVerified)
&& pni != null
&& pni.isValid())
&& pni != null
&& pni.isValid())
.nickname(remote.nickname)
.note(remote.note)
.avatarColor(remote.avatarColor);
@ -304,6 +314,9 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
final var matchesLocal = doProtosMatch(merged, local);
if (matchesLocal) {
if (identityKeysExistAndConflict && conflictAci != null) {
identityConflictsPendingRepair.add(localRecord.getId());
}
return localRecord;
}

View File

@ -100,8 +100,8 @@ public final class StorageSyncModels {
final var builder = SignalContactRecord.Companion.newBuilder(recipient.getStorageRecord())
.e164(address.number().orElse(""))
.username(address.username().orElse(""))
.pniSignatureVerified(address.pni().map(PNI::isValid).orElse(false)
&& recipient.isPniSignatureVerified())
.pniSignatureVerified(address.pni().map(PNI::isValid).orElse(false)
&& recipient.isPniSignatureVerified())
.profileKey(recipient.getProfileKey() == null
? ByteString.EMPTY
: ByteString.of(recipient.getProfileKey().serialize()));

View File

@ -0,0 +1,38 @@
package org.asamk.signal.manager.helper;
import org.asamk.signal.manager.syncStorage.WriteOperationResult;
import org.junit.jupiter.api.Test;
import org.whispersystems.signalservice.api.storage.SignalStorageRecord;
import org.whispersystems.signalservice.api.storage.StorageId;
import org.whispersystems.signalservice.internal.storage.protos.StorageRecord;
import java.util.List;
import java.util.Set;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
class StorageHelperTest {
@Test
void defersWritesContainingOnlyIdentityConflictsPendingRepair() {
final var pendingId = StorageId.forContact(new byte[]{1});
final var otherId = StorageId.forContact(new byte[]{2});
final var pendingRecord = record(pendingId);
final var otherRecord = record(otherId);
assertTrue(StorageHelper.containsOnlyIdentityConflictsPendingRepair(write(List.of(pendingRecord)),
Set.of(pendingId)));
assertFalse(StorageHelper.containsOnlyIdentityConflictsPendingRepair(write(List.of(pendingRecord, otherRecord)),
Set.of(pendingId)));
assertFalse(StorageHelper.containsOnlyIdentityConflictsPendingRepair(write(List.of()), Set.of(pendingId)));
}
private static SignalStorageRecord record(final StorageId id) {
return new SignalStorageRecord(id, new StorageRecord.Builder().build());
}
private static WriteOperationResult write(final List<SignalStorageRecord> inserts) {
return new WriteOperationResult(null, inserts, List.of());
}
}