diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java index b7e71cd3d..969aa536e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java @@ -131,6 +131,7 @@ public class StorageSyncJob extends BaseJob { StorageKey storageServiceKey = SignalStore.storageServiceValues().getOrCreateStorageKey(); boolean needsMultiDeviceSync = false; + boolean needsForcePush = false; long localManifestVersion = TextSecurePreferences.getStorageManifestVersion(context); Optional remoteManifest = accountManager.getStorageManifestIfDifferentVersion(storageServiceKey, localManifestVersion); long remoteManifestVersion = remoteManifest.transform(SignalStorageManifest::getVersion).or(localManifestVersion); @@ -143,6 +144,11 @@ public class StorageSyncJob extends BaseJob { List allLocalStorageKeys = getAllLocalStorageIds(context, Recipient.self().fresh()); KeyDifferenceResult keyDifference = StorageSyncHelper.findKeyDifference(remoteManifest.get().getStorageIds(), allLocalStorageKeys); + if (keyDifference.hasTypeMismatches()) { + Log.w(TAG, "Found type mismatches in the key sets! Scheduling a force push after this sync completes."); + needsForcePush = true; + } + if (!keyDifference.isEmpty()) { Log.i(TAG, "[Remote Newer] There's a difference in keys. Local-only: " + keyDifference.getLocalOnlyKeys().size() + ", Remote-only: " + keyDifference.getRemoteOnlyKeys().size()); @@ -152,6 +158,11 @@ public class StorageSyncJob extends BaseJob { MergeResult mergeResult = StorageSyncHelper.resolveConflict(remoteOnly, localOnly); WriteOperationResult writeOperationResult = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), allLocalStorageKeys, mergeResult); + if (remoteOnly.size() != keyDifference.getRemoteOnlyKeys().size()) { + Log.w(TAG, "Could not find all remote-only records! Requested: " + keyDifference.getRemoteOnlyKeys().size() + ", Found: " + remoteOnly.size() + ". Scheduling a force push after this sync completes."); + needsForcePush = true; + } + StorageSyncValidations.validate(writeOperationResult); Log.i(TAG, "[Remote Newer] MergeResult :: " + mergeResult); @@ -248,6 +259,11 @@ public class StorageSyncJob extends BaseJob { Log.i(TAG, "[Local Changes] No local changes."); } + if (needsForcePush) { + Log.w(TAG, "Scheduling a force push."); + ApplicationDependencies.getJobManager().add(new StorageForcePushJob()); + } + return needsMultiDeviceSync; } diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java index aa4801a3f..134fcd8a7 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java @@ -6,6 +6,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; +import com.annimon.stream.Collectors; import com.annimon.stream.Stream; import org.thoughtcrime.securesms.database.DatabaseFactory; @@ -19,6 +20,7 @@ import org.thoughtcrime.securesms.keyvalue.SignalStore; import org.thoughtcrime.securesms.logging.Log; import org.thoughtcrime.securesms.recipients.Recipient; import org.thoughtcrime.securesms.recipients.RecipientId; +import org.thoughtcrime.securesms.util.Base64; import org.thoughtcrime.securesms.util.SetUtil; import org.thoughtcrime.securesms.util.TextSecurePreferences; import org.thoughtcrime.securesms.util.Util; @@ -41,6 +43,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.LinkedList; import java.util.List; import java.util.Locale; import java.util.Map; @@ -213,10 +216,30 @@ public final class StorageSyncHelper { public static @NonNull KeyDifferenceResult findKeyDifference(@NonNull Collection remoteKeys, @NonNull Collection localKeys) { - Set remoteOnlyKeys = SetUtil.difference(remoteKeys, localKeys); - Set localOnlyKeys = SetUtil.difference(localKeys, remoteKeys); + Map remoteByRawId = Stream.of(remoteKeys).collect(Collectors.toMap(id -> Base64.encodeBytes(id.getRaw()), id -> id)); + Map localByRawId = Stream.of(localKeys).collect(Collectors.toMap(id -> Base64.encodeBytes(id.getRaw()), id -> id)); - return new KeyDifferenceResult(new ArrayList<>(remoteOnlyKeys), new ArrayList<>(localOnlyKeys)); + boolean hasTypeMismatch = remoteByRawId.size() != remoteKeys.size() || localByRawId.size() != localKeys.size(); + + Set remoteOnlyRawIds = SetUtil.difference(remoteByRawId.keySet(), localByRawId.keySet()); + Set localOnlyRawIds = SetUtil.difference(localByRawId.keySet(), remoteByRawId.keySet()); + Set sharedRawIds = SetUtil.intersection(localByRawId.keySet(), remoteByRawId.keySet()); + + for (String rawId : sharedRawIds) { + StorageId remote = Objects.requireNonNull(remoteByRawId.get(rawId)); + StorageId local = Objects.requireNonNull(localByRawId.get(rawId)); + + if (remote.getType() != local.getType()) { + remoteOnlyRawIds.remove(rawId); + localOnlyRawIds.remove(rawId); + hasTypeMismatch = true; + } + } + + List remoteOnlyKeys = Stream.of(remoteOnlyRawIds).map(remoteByRawId::get).toList(); + List localOnlyKeys = Stream.of(localOnlyRawIds).map(localByRawId::get).toList(); + + return new KeyDifferenceResult(remoteOnlyKeys, localOnlyKeys, hasTypeMismatch); } /** @@ -459,12 +482,15 @@ public final class StorageSyncHelper { public static final class KeyDifferenceResult { private final List remoteOnlyKeys; private final List localOnlyKeys; + private final boolean hasTypeMismatches; private KeyDifferenceResult(@NonNull List remoteOnlyKeys, - @NonNull List localOnlyKeys) + @NonNull List localOnlyKeys, + boolean hasTypeMismatches) { - this.remoteOnlyKeys = remoteOnlyKeys; - this.localOnlyKeys = localOnlyKeys; + this.remoteOnlyKeys = remoteOnlyKeys; + this.localOnlyKeys = localOnlyKeys; + this.hasTypeMismatches = hasTypeMismatches; } public @NonNull List getRemoteOnlyKeys() { @@ -475,6 +501,14 @@ public final class StorageSyncHelper { return localOnlyKeys; } + /** + * @return True if there exist some keys that have matching raw ID's but different types, + * otherwise false. + */ + public boolean hasTypeMismatches() { + return hasTypeMismatches; + } + public boolean isEmpty() { return remoteOnlyKeys.isEmpty() && localOnlyKeys.isEmpty(); } diff --git a/app/src/test/java/org/thoughtcrime/securesms/storage/StorageSyncHelperTest.java b/app/src/test/java/org/thoughtcrime/securesms/storage/StorageSyncHelperTest.java index 0088160a1..dfa9371da 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/storage/StorageSyncHelperTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/storage/StorageSyncHelperTest.java @@ -28,9 +28,11 @@ import org.whispersystems.signalservice.api.storage.StorageId; import org.whispersystems.signalservice.api.util.UuidUtil; import java.util.Arrays; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.UUID; @@ -85,20 +87,57 @@ public final class StorageSyncHelperTest { KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(1, 2, 3), keyListOf(1, 2, 3)); assertTrue(result.getLocalOnlyKeys().isEmpty()); assertTrue(result.getRemoteOnlyKeys().isEmpty()); + assertFalse(result.hasTypeMismatches()); } @Test public void findKeyDifference_noOverlap() { KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(1, 2, 3), keyListOf(4, 5, 6)); - assertEquals(keyListOf(1, 2, 3), result.getRemoteOnlyKeys()); - assertEquals(keyListOf(4, 5, 6), result.getLocalOnlyKeys()); + assertContentsEqual(keyListOf(1, 2, 3), result.getRemoteOnlyKeys()); + assertContentsEqual(keyListOf(4, 5, 6), result.getLocalOnlyKeys()); + assertFalse(result.hasTypeMismatches()); } @Test public void findKeyDifference_someOverlap() { KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(1, 2, 3), keyListOf(2, 3, 4)); - assertEquals(keyListOf(1), result.getRemoteOnlyKeys()); - assertEquals(keyListOf(4), result.getLocalOnlyKeys()); + assertContentsEqual(keyListOf(1), result.getRemoteOnlyKeys()); + assertContentsEqual(keyListOf(4), result.getLocalOnlyKeys()); + assertFalse(result.hasTypeMismatches()); + } + + @Test + public void findKeyDifference_typeMismatch_allOverlap() { + KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(new HashMap() {{ + put(100, 1); + put(200, 2); + }}), + keyListOf(new HashMap() {{ + put(100, 1); + put(200, 1); + }})); + + assertTrue(result.getLocalOnlyKeys().isEmpty()); + assertTrue(result.getRemoteOnlyKeys().isEmpty()); + assertTrue(result.hasTypeMismatches()); + } + + @Test + public void findKeyDifference_typeMismatch_someOverlap() { + KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(new HashMap() {{ + put(100, 1); + put(200, 2); + put(300, 1); + }}), + keyListOf(new HashMap() {{ + put(100, 1); + put(200, 1); + put(400, 1); + }})); + + assertContentsEqual(Arrays.asList(StorageId.forType(byteArray(300), 1)), result.getRemoteOnlyKeys()); + assertContentsEqual(Arrays.asList(StorageId.forType(byteArray(400), 1)), result.getLocalOnlyKeys()); + assertTrue(result.hasTypeMismatches()); } @Test @@ -488,6 +527,10 @@ public final class StorageSyncHelperTest { return Stream.of(byteListOf(vals)).map(b -> StorageId.forType(b, 1)).toList(); } + private static List keyListOf(Map vals) { + return Stream.of(vals).map(e -> StorageId.forType(byteArray(e.getKey()), e.getValue())).toList(); + } + private static StorageId contactKey(int val) { return StorageId.forContact(byteArray(val)); } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java index 6f62e5b7d..b20351f56 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java @@ -470,10 +470,6 @@ public class SignalServiceAccountManager { String authToken = this.pushServiceSocket.getStorageAuth(); StorageItems items = this.pushServiceSocket.readStorageItems(authToken, operation.build()); - if (items.getItemsCount() != storageKeys.size()) { - Log.w(TAG, "Failed to find all remote keys! Requested: " + storageKeys.size() + ", Found: " + items.getItemsCount()); - } - for (StorageItem item : items.getItemsList()) { Integer type = typeMap.get(item.getKey()); if (type != null) {