From 0877d6a25edbf87aa4a7c0059fe9337ad9b75f49 Mon Sep 17 00:00:00 2001
From: Greyson Parrelli <greyson@signal.org>
Date: Tue, 8 Feb 2022 11:53:48 -0500
Subject: [PATCH] Improve handling of unknown fields in storage service.

Improve handling of unknown fields in storage service.

Found a lovely bug today where unmuting chats on mobile didn't sync to my linked devices. Turns out this was a result of the unknown field merging.

1. When a proto has unknown fields, we store the entire proto in a column in our database.
2. After building a proto that we want to write remotely, we merge the saved proto with unknown fields into constructed proto. Most of the time this is fine.
3. _However_, if one of the values you're trying to set happens to be the same as the default value for the given data type (e.g. setting a long like mutedUntil = 0), then when the protos merge, it treats that field as unset and can override it with the field from the proto with unknown fields.
4. Because we currently have unknown fields in every GV2 record, we could never unmute a GV2 group :(

This changes the order of things so that unknown fields are the first thing applied in the record builder. I did this by requiring them in the builder constructors. That way start off with the unknown fields and then can manually set whatever you want, and it'll be guaranteed to override it.
---
 .../storage/AccountRecordProcessor.java       |  3 +-
 .../storage/ContactRecordProcessor.java       |  3 +-
 .../storage/GroupV1RecordProcessor.java       |  3 +-
 .../storage/GroupV2RecordProcessor.java       |  3 +-
 .../securesms/storage/StorageSyncHelper.java  |  3 +-
 .../securesms/storage/StorageSyncModels.java  |  9 ++--
 .../storage/StorageSyncHelperTest.java        |  2 +-
 .../api/storage/SignalAccountRecord.java      | 37 ++++++++---------
 .../api/storage/SignalContactRecord.java      | 36 ++++++++--------
 .../api/storage/SignalGroupV1Record.java      | 36 ++++++++--------
 .../api/storage/SignalGroupV2Record.java      | 41 +++++++++----------
 .../api/storage/SignalContactRecordTest.java  |  2 +-
 12 files changed, 80 insertions(+), 98 deletions(-)

diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/AccountRecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/AccountRecordProcessor.java
index 9e49e6931..791fae137 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/storage/AccountRecordProcessor.java
+++ b/app/src/main/java/org/thoughtcrime/securesms/storage/AccountRecordProcessor.java
@@ -119,8 +119,7 @@ public class AccountRecordProcessor extends DefaultStorageRecordProcessor<Signal
     } else if (matchesLocal) {
       return local;
     } else {
-      return new SignalAccountRecord.Builder(keyGenerator.generate())
-                                    .setUnknownFields(unknownFields)
+      return new SignalAccountRecord.Builder(keyGenerator.generate(), unknownFields)
                                     .setGivenName(givenName)
                                     .setFamilyName(familyName)
                                     .setAvatarUrlPath(avatarUrlPath)
diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java
index e26e03230..a885de696 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java
+++ b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java
@@ -118,8 +118,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
     } else if (matchesLocal) {
       return local;
     } else {
-      return new SignalContactRecord.Builder(keyGenerator.generate(), address)
-                                    .setUnknownFields(unknownFields)
+      return new SignalContactRecord.Builder(keyGenerator.generate(), address, unknownFields)
                                     .setGivenName(givenName)
                                     .setFamilyName(familyName)
                                     .setProfileKey(profileKey)
diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java
index 215af5667..d760dc516 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java
+++ b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java
@@ -89,8 +89,7 @@ public final class GroupV1RecordProcessor extends DefaultStorageRecordProcessor<
     } else if (matchesLocal) {
       return local;
     } else {
-      return new SignalGroupV1Record.Builder(keyGenerator.generate(), remote.getGroupId())
-                                    .setUnknownFields(unknownFields)
+      return new SignalGroupV1Record.Builder(keyGenerator.generate(), remote.getGroupId(), unknownFields)
                                     .setBlocked(blocked)
                                     .setProfileSharingEnabled(blocked)
                                     .setForcedUnread(forcedUnread)
diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java
index 9606a728b..3e40627ea 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java
+++ b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java
@@ -81,8 +81,7 @@ public final class GroupV2RecordProcessor extends DefaultStorageRecordProcessor<
     } else if (matchesLocal) {
       return local;
     } else {
-      return new SignalGroupV2Record.Builder(keyGenerator.generate(), remote.getMasterKeyBytes())
-                                    .setUnknownFields(unknownFields)
+      return new SignalGroupV2Record.Builder(keyGenerator.generate(), remote.getMasterKeyBytes(), unknownFields)
                                     .setBlocked(blocked)
                                     .setProfileSharingEnabled(blocked)
                                     .setArchived(archived)
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 6d9286ede..2cf8e308e 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java
+++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java
@@ -111,8 +111,7 @@ public final class StorageSyncHelper {
                                                     .map(recipientDatabase::getRecordForSync)
                                                     .toList();
 
-    SignalAccountRecord account = new SignalAccountRecord.Builder(self.getStorageServiceId())
-                                                         .setUnknownFields(record != null ? record.getSyncExtras().getStorageProto() : null)
+    SignalAccountRecord account = new SignalAccountRecord.Builder(self.getStorageServiceId(), record != null ? record.getSyncExtras().getStorageProto() : null)
                                                          .setProfileKey(self.getProfileKey())
                                                          .setGivenName(self.getProfileName().getGivenName())
                                                          .setFamilyName(self.getProfileName().getFamilyName())
diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.java
index cdc5ddcdd..eaa0f9aef 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.java
+++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.java
@@ -97,8 +97,7 @@ public final class StorageSyncModels {
 
     ACI aci = recipient.getAci() != null ? recipient.getAci() : ACI.UNKNOWN;
 
-    return new SignalContactRecord.Builder(rawStorageId, new SignalServiceAddress(aci, recipient.getE164()))
-                                  .setUnknownFields(recipient.getSyncExtras().getStorageProto())
+    return new SignalContactRecord.Builder(rawStorageId, new SignalServiceAddress(aci, recipient.getE164()), recipient.getSyncExtras().getStorageProto())
                                   .setProfileKey(recipient.getProfileKey())
                                   .setGivenName(recipient.getProfileName().getGivenName())
                                   .setFamilyName(recipient.getProfileName().getFamilyName())
@@ -123,8 +122,7 @@ public final class StorageSyncModels {
       throw new AssertionError("Group is not V1");
     }
 
-    return new SignalGroupV1Record.Builder(rawStorageId, groupId.getDecodedId())
-                                  .setUnknownFields(recipient.getSyncExtras().getStorageProto())
+    return new SignalGroupV1Record.Builder(rawStorageId, groupId.getDecodedId(), recipient.getSyncExtras().getStorageProto())
                                   .setBlocked(recipient.isBlocked())
                                   .setProfileSharingEnabled(recipient.isProfileSharing())
                                   .setArchived(recipient.getSyncExtras().isArchived())
@@ -148,8 +146,7 @@ public final class StorageSyncModels {
       throw new AssertionError("Group master key not on recipient record");
     }
 
-    return new SignalGroupV2Record.Builder(rawStorageId, groupMasterKey)
-                                  .setUnknownFields(recipient.getSyncExtras().getStorageProto())
+    return new SignalGroupV2Record.Builder(rawStorageId, groupMasterKey, recipient.getSyncExtras().getStorageProto())
                                   .setBlocked(recipient.isBlocked())
                                   .setProfileSharingEnabled(recipient.isProfileSharing())
                                   .setArchived(recipient.getSyncExtras().isArchived())
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 ce7704c66..62689405f 100644
--- a/app/src/test/java/org/thoughtcrime/securesms/storage/StorageSyncHelperTest.java
+++ b/app/src/test/java/org/thoughtcrime/securesms/storage/StorageSyncHelperTest.java
@@ -177,7 +177,7 @@ public final class StorageSyncHelperTest {
                                                             String e164,
                                                             String profileName)
   {
-    return new SignalContactRecord.Builder(byteArray(key), new SignalServiceAddress(aci, e164))
+    return new SignalContactRecord.Builder(byteArray(key), new SignalServiceAddress(aci, e164), null)
                                   .setGivenName(profileName);
   }
 
diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java
index 1bd8f51ef..8a1f3f8e5 100644
--- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java
+++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java
@@ -11,6 +11,7 @@ import org.whispersystems.signalservice.api.push.SignalServiceAddress;
 import org.whispersystems.signalservice.api.util.OptionalUtil;
 import org.whispersystems.signalservice.api.util.ProtoUtil;
 import org.whispersystems.signalservice.internal.storage.protos.AccountRecord;
+import org.whispersystems.signalservice.internal.storage.protos.GroupV2Record;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -455,16 +456,14 @@ public final class SignalAccountRecord implements SignalRecord {
     private final StorageId             id;
     private final AccountRecord.Builder builder;
 
-    private byte[] unknownFields;
+    public Builder(byte[] rawId, byte[] serializedUnknowns) {
+      this.id = StorageId.forAccount(rawId);
 
-    public Builder(byte[] rawId) {
-      this.id      = StorageId.forAccount(rawId);
-      this.builder = AccountRecord.newBuilder();
-    }
-
-    public Builder setUnknownFields(byte[] serializedUnknowns) {
-      this.unknownFields = serializedUnknowns;
-      return this;
+      if (serializedUnknowns != null) {
+        this.builder = parseUnknowns(serializedUnknowns);
+      } else {
+        this.builder = AccountRecord.newBuilder();
+      }
     }
 
     public Builder setGivenName(String givenName) {
@@ -601,19 +600,17 @@ public final class SignalAccountRecord implements SignalRecord {
       return this;
     }
 
-    public SignalAccountRecord build() {
-      AccountRecord proto = builder.build();
-
-      if (unknownFields != null) {
-        try {
-          proto = ProtoUtil.combineWithUnknownFields(proto, unknownFields);
-        } catch (InvalidProtocolBufferException e) {
-          Log.w(TAG, "Failed to combine unknown fields!", e);
-          throw new IllegalStateException(e);
-        }
+    private static AccountRecord.Builder parseUnknowns(byte[] serializedUnknowns) {
+      try {
+        return AccountRecord.parseFrom(serializedUnknowns).toBuilder();
+      } catch (InvalidProtocolBufferException e) {
+        Log.w(TAG, "Failed to combine unknown fields!", e);
+        return AccountRecord.newBuilder();
       }
+    }
 
-      return new SignalAccountRecord(id, proto);
+    public SignalAccountRecord build() {
+      return new SignalAccountRecord(id, builder.build());
     }
   }
 }
diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java
index 63039f8e1..4d47694e5 100644
--- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java
+++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java
@@ -12,6 +12,7 @@ import org.whispersystems.signalservice.api.util.ProtoUtil;
 import org.whispersystems.signalservice.api.util.UuidUtil;
 import org.whispersystems.signalservice.internal.storage.protos.ContactRecord;
 import org.whispersystems.signalservice.internal.storage.protos.ContactRecord.IdentityState;
+import org.whispersystems.signalservice.internal.storage.protos.GroupV2Record;
 
 import java.util.Arrays;
 import java.util.LinkedList;
@@ -206,21 +207,19 @@ public final class SignalContactRecord implements SignalRecord {
     private final StorageId             id;
     private final ContactRecord.Builder builder;
 
-    private byte[] unknownFields;
+    public Builder(byte[] rawId, SignalServiceAddress address, byte[] serializedUnknowns) {
+      this.id = StorageId.forContact(rawId);
 
-    public Builder(byte[] rawId, SignalServiceAddress address) {
-      this.id      = StorageId.forContact(rawId);
-      this.builder = ContactRecord.newBuilder();
+      if (serializedUnknowns != null) {
+        this.builder = parseUnknowns(serializedUnknowns);
+      } else {
+        this.builder = ContactRecord.newBuilder();
+      }
 
       builder.setServiceUuid(address.getAci().toString());
       builder.setServiceE164(address.getNumber().or(""));
     }
 
-    public Builder setUnknownFields(byte[] serializedUnknowns) {
-      this.unknownFields = serializedUnknowns;
-      return this;
-    }
-
     public Builder setGivenName(String givenName) {
       builder.setGivenName(givenName == null ? "" : givenName);
       return this;
@@ -276,18 +275,17 @@ public final class SignalContactRecord implements SignalRecord {
       return this;
     }
 
-    public SignalContactRecord build() {
-      ContactRecord proto = builder.build();
-
-      if (unknownFields != null) {
-        try {
-          proto = ProtoUtil.combineWithUnknownFields(proto, unknownFields);
-        } catch (InvalidProtocolBufferException e) {
-          Log.w(TAG, "Failed to combine unknown fields!", e);
-        }
+    private static ContactRecord.Builder parseUnknowns(byte[] serializedUnknowns) {
+      try {
+        return ContactRecord.parseFrom(serializedUnknowns).toBuilder();
+      } catch (InvalidProtocolBufferException e) {
+        Log.w(TAG, "Failed to combine unknown fields!", e);
+        return ContactRecord.newBuilder();
       }
+    }
 
-      return new SignalContactRecord(id, proto);
+    public SignalContactRecord build() {
+      return new SignalContactRecord(id, builder.build());
     }
   }
 }
diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java
index b150cb355..8c404b42a 100644
--- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java
+++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java
@@ -6,6 +6,7 @@ import com.google.protobuf.InvalidProtocolBufferException;
 import org.whispersystems.libsignal.logging.Log;
 import org.whispersystems.signalservice.api.util.ProtoUtil;
 import org.whispersystems.signalservice.internal.storage.protos.GroupV1Record;
+import org.whispersystems.signalservice.internal.storage.protos.GroupV2Record;
 
 import java.util.Arrays;
 import java.util.LinkedList;
@@ -136,20 +137,18 @@ public final class SignalGroupV1Record implements SignalRecord {
     private final StorageId             id;
     private final GroupV1Record.Builder builder;
 
-    private byte[] unknownFields;
+    public Builder(byte[] rawId, byte[] groupId, byte[] serializedUnknowns) {
+      this.id = StorageId.forGroupV1(rawId);
 
-    public Builder(byte[] rawId, byte[] groupId) {
-      this.id      = StorageId.forGroupV1(rawId);
-      this.builder = GroupV1Record.newBuilder();
+      if (serializedUnknowns != null) {
+        this.builder = parseUnknowns(serializedUnknowns);
+      } else {
+        this.builder = GroupV1Record.newBuilder();
+      }
 
       builder.setId(ByteString.copyFrom(groupId));
     }
 
-    public Builder setUnknownFields(byte[] serializedUnknowns) {
-      this.unknownFields = serializedUnknowns;
-      return this;
-    }
-
     public Builder setBlocked(boolean blocked) {
       builder.setBlocked(blocked);
       return this;
@@ -175,18 +174,17 @@ public final class SignalGroupV1Record implements SignalRecord {
       return this;
     }
 
-    public SignalGroupV1Record build() {
-      GroupV1Record proto = builder.build();
-
-      if (unknownFields != null) {
-        try {
-          proto = ProtoUtil.combineWithUnknownFields(proto, unknownFields);
-        } catch (InvalidProtocolBufferException e) {
-          Log.w(TAG, "Failed to combine unknown fields!", e);
-        }
+    private static GroupV1Record.Builder parseUnknowns(byte[] serializedUnknowns) {
+      try {
+        return GroupV1Record.parseFrom(serializedUnknowns).toBuilder();
+      } catch (InvalidProtocolBufferException e) {
+        Log.w(TAG, "Failed to combine unknown fields!", e);
+        return GroupV1Record.newBuilder();
       }
+    }
 
-      return new SignalGroupV1Record(id, proto);
+    public SignalGroupV1Record build() {
+      return new SignalGroupV1Record(id, builder.build());
     }
   }
 }
diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java
index 0d7dcd093..0dc904d20 100644
--- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java
+++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java
@@ -147,24 +147,22 @@ public final class SignalGroupV2Record implements SignalRecord {
     private final StorageId             id;
     private final GroupV2Record.Builder builder;
 
-    private byte[] unknownFields;
-
-    public Builder(byte[] rawId, GroupMasterKey masterKey) {
-      this(rawId, masterKey.serialize());
+    public Builder(byte[] rawId, GroupMasterKey masterKey, byte[] serializedUnknowns) {
+      this(rawId, masterKey.serialize(), serializedUnknowns);
     }
 
-    public Builder(byte[] rawId, byte[] masterKey) {
-      this.id      = StorageId.forGroupV2(rawId);
-      this.builder = GroupV2Record.newBuilder();
+    public Builder(byte[] rawId, byte[] masterKey, byte[] serializedUnknowns) {
+      this.id = StorageId.forGroupV2(rawId);
+
+      if (serializedUnknowns != null) {
+        this.builder = parseUnknowns(serializedUnknowns);
+      } else {
+        this.builder = GroupV2Record.newBuilder();
+      }
 
       builder.setMasterKey(ByteString.copyFrom(masterKey));
     }
 
-    public Builder setUnknownFields(byte[] serializedUnknowns) {
-      this.unknownFields = serializedUnknowns;
-      return this;
-    }
-
     public Builder setBlocked(boolean blocked) {
       builder.setBlocked(blocked);
       return this;
@@ -190,18 +188,17 @@ public final class SignalGroupV2Record implements SignalRecord {
       return this;
     }
 
-    public SignalGroupV2Record build() {
-      GroupV2Record proto = builder.build();
-
-      if (unknownFields != null) {
-        try {
-          proto = ProtoUtil.combineWithUnknownFields(proto, unknownFields);
-        } catch (InvalidProtocolBufferException e) {
-          Log.w(TAG, "Failed to combine unknown fields!", e);
-        }
+    private static GroupV2Record.Builder parseUnknowns(byte[] serializedUnknowns) {
+      try {
+        return GroupV2Record.parseFrom(serializedUnknowns).toBuilder();
+      } catch (InvalidProtocolBufferException e) {
+        Log.w(TAG, "Failed to combine unknown fields!", e);
+        return GroupV2Record.newBuilder();
       }
+    }
 
-      return new SignalGroupV2Record(id, proto);
+    public SignalGroupV2Record build() {
+      return new SignalGroupV2Record(id, builder.build());
     }
   }
 }
diff --git a/libsignal/service/src/test/java/org/whispersystems/signalservice/api/storage/SignalContactRecordTest.java b/libsignal/service/src/test/java/org/whispersystems/signalservice/api/storage/SignalContactRecordTest.java
index dddaf5724..45ab49f81 100644
--- a/libsignal/service/src/test/java/org/whispersystems/signalservice/api/storage/SignalContactRecordTest.java
+++ b/libsignal/service/src/test/java/org/whispersystems/signalservice/api/storage/SignalContactRecordTest.java
@@ -52,7 +52,7 @@ public class SignalContactRecordTest {
                                                             String e164,
                                                             String givenName)
   {
-    return new SignalContactRecord.Builder(byteArray(key), new SignalServiceAddress(aci, e164))
+    return new SignalContactRecord.Builder(byteArray(key), new SignalServiceAddress(aci, e164), null)
                                   .setGivenName(givenName);
   }
 }