From 25ce2a649acd07d32f6ddd3c67f486f2046412ac Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 8 Apr 2021 10:14:14 -0400 Subject: [PATCH] Write additional storage validations based on previous manifest. --- .../securesms/jobs/StorageSyncJob.java | 5 +- .../securesms/jobs/StorageSyncJobV2.java | 5 +- .../storage/StorageSyncValidations.java | 76 ++++++++++++++++++- 3 files changed, 81 insertions(+), 5 deletions(-) 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 6f98f37ab6..8a3a7e897c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java @@ -192,7 +192,7 @@ public class StorageSyncJob extends BaseJob { needsForcePush = true; } - StorageSyncValidations.validate(writeOperationResult); + StorageSyncValidations.validate(writeOperationResult, remoteManifest, needsForcePush); Log.i(TAG, "[Remote Newer] MergeResult :: " + mergeResult); @@ -213,6 +213,7 @@ public class StorageSyncJob extends BaseJob { } remoteManifestVersion = writeOperationResult.getManifest().getVersion(); + remoteManifest = Optional.of(writeOperationResult.getManifest()); needsMultiDeviceSync = true; } else { @@ -255,7 +256,7 @@ public class StorageSyncJob extends BaseJob { Log.i(TAG, String.format(Locale.ENGLISH, "[Local Changes] Local changes present. %d updates, %d inserts, %d deletes, account update: %b, account insert: %b.", pendingUpdates.size(), pendingInsertions.size(), pendingDeletions.size(), pendingAccountUpdate.isPresent(), pendingAccountInsert.isPresent())); WriteOperationResult localWrite = localWriteResult.get().getWriteResult(); - StorageSyncValidations.validate(localWrite); + StorageSyncValidations.validate(localWrite, remoteManifest, needsForcePush); Log.i(TAG, "[Local Changes] WriteOperationResult :: " + localWrite); diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java index e62d48fe23..65461ff3bd 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java @@ -278,7 +278,7 @@ public class StorageSyncJobV2 extends BaseJob { //noinspection unchecked Stop yelling at my beautiful method signatures mergeWriteOperation = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), localStorageIdsAfterMerge, localOnly, contactResult, gv1Result, gv2Result, accountResult); - StorageSyncValidations.validate(mergeWriteOperation); + StorageSyncValidations.validate(mergeWriteOperation, remoteManifest, needsForcePush); db.setTransactionSuccessful(); } finally { db.endTransaction(); @@ -302,6 +302,7 @@ public class StorageSyncJobV2 extends BaseJob { } remoteManifestVersion = mergeWriteOperation.getManifest().getVersion(); + remoteManifest = Optional.of(mergeWriteOperation.getManifest()); needsMultiDeviceSync = true; } else { @@ -337,7 +338,7 @@ public class StorageSyncJobV2 extends BaseJob { Log.i(TAG, String.format(Locale.ENGLISH, "[Local Changes] Local changes present. %d updates, %d inserts, %d deletes, account update: %b, account insert: %b.", pendingUpdates.size(), pendingInsertions.size(), pendingDeletions.size(), pendingAccountUpdate.isPresent(), pendingAccountInsert.isPresent())); WriteOperationResult localWrite = localWriteResult.get().getWriteResult(); - StorageSyncValidations.validate(localWrite); + StorageSyncValidations.validate(localWrite, remoteManifest, needsForcePush); Log.i(TAG, "[Local Changes] WriteOperationResult :: " + localWrite); diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java index 066c29b6c1..713d744dd7 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java @@ -5,9 +5,13 @@ import androidx.annotation.NonNull; import com.annimon.stream.Collectors; import com.annimon.stream.Stream; +import org.signal.core.util.logging.Log; import org.thoughtcrime.securesms.recipients.Recipient; import org.thoughtcrime.securesms.util.Base64; +import org.thoughtcrime.securesms.util.SetUtil; +import org.whispersystems.libsignal.util.guava.Optional; import org.whispersystems.signalservice.api.push.SignalServiceAddress; +import org.whispersystems.signalservice.api.storage.SignalRecord; import org.whispersystems.signalservice.api.storage.SignalStorageManifest; import org.whispersystems.signalservice.api.storage.SignalStorageRecord; import org.whispersystems.signalservice.api.storage.StorageId; @@ -20,9 +24,11 @@ import java.util.Set; public final class StorageSyncValidations { + private static final String TAG = Log.tag(StorageSyncValidations.class); + private StorageSyncValidations() {} - public static void validate(@NonNull StorageSyncHelper.WriteOperationResult result) { + public static void validate(@NonNull StorageSyncHelper.WriteOperationResult result, @NonNull Optional previousManifest, boolean forcePushPending) { validateManifestAndInserts(result.getManifest(), result.getInserts()); if (result.getDeletes().size() > 0) { @@ -35,6 +41,53 @@ public final class StorageSyncValidations { } } } + + if (!previousManifest.isPresent()) { + Log.i(TAG, "No previous manifest, not bothering with additional validations around the diffs between the two manifests."); + return; + } + + if (result.getManifest().getVersion() != previousManifest.get().getVersion() + 1) { + throw new IncorrectManifestVersionError(); + } + + if (forcePushPending) { + Log.i(TAG, "Force push pending, not bothering with additional validations around the diffs between the two manifests."); + return; + } + + Set previousIds = Stream.of(previousManifest.get().getStorageIds()).map(id -> ByteBuffer.wrap(id.getRaw())).collect(Collectors.toSet()); + Set newIds = Stream.of(result.getManifest().getStorageIds()).map(id -> ByteBuffer.wrap(id.getRaw())).collect(Collectors.toSet()); + + Set insertedIds = SetUtil.difference(newIds, previousIds); + Set deletedIds = SetUtil.difference(previousIds, newIds); + + Set writeInserts = Stream.of(result.getInserts()).map(r -> ByteBuffer.wrap(r.getId().getRaw())).collect(Collectors.toSet()); + Set writeDeletes = Stream.of(result.getDeletes()).map(ByteBuffer::wrap).collect(Collectors.toSet()); + + if (writeInserts.size() > insertedIds.size()) { + throw new MoreInsertsThanExpectedError(); + } + + if (writeInserts.size() < insertedIds.size()) { + throw new LessInsertsThanExpectedError(); + } + + if (!writeInserts.containsAll(insertedIds)) { + throw new InsertMismatchError(); + } + + if (writeDeletes.size() > deletedIds.size()) { + throw new MoreDeletesThanExpectedError(); + } + + if (writeDeletes.size() < deletedIds.size()) { + throw new LessDeletesThanExpectedError(); + } + + if (!writeDeletes.containsAll(deletedIds)) { + throw new DeleteMismatchError(); + } } @@ -117,4 +170,25 @@ public final class StorageSyncValidations { private static final class SelfAddedAsContactError extends Error { } + + private static final class IncorrectManifestVersionError extends Error { + } + + private static final class MoreInsertsThanExpectedError extends Error { + } + + private static final class LessInsertsThanExpectedError extends Error { + } + + private static final class InsertMismatchError extends Error { + } + + private static final class MoreDeletesThanExpectedError extends Error { + } + + private static final class LessDeletesThanExpectedError extends Error { + } + + private static final class DeleteMismatchError extends Error { + } }