From a71fe0fd751f0f028c6135adcb3362d4b71d285a Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 25 Jun 2021 16:35:42 -0400 Subject: [PATCH] Fix issue with group creation on linked devices. --- .../securesms/database/GroupDatabase.java | 44 +++++++++++ .../securesms/jobs/StorageSyncJob.java | 2 + .../messages/MessageContentProcessor.java | 73 ++++++++++++------- .../storage/GroupV2RecordProcessor.java | 5 +- 4 files changed, 96 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java index c5c280bdb5..06bcc49bec 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java @@ -22,6 +22,9 @@ import org.signal.zkgroup.InvalidInputException; import org.signal.zkgroup.groups.GroupMasterKey; import org.thoughtcrime.securesms.crypto.SenderKeyUtil; import org.thoughtcrime.securesms.database.helpers.SQLCipherOpenHelper; +import org.thoughtcrime.securesms.dependencies.ApplicationDependencies; +import org.thoughtcrime.securesms.groups.v2.processing.GroupsV2StateProcessor; +import org.thoughtcrime.securesms.jobs.RequestGroupV2InfoJob; import org.whispersystems.signalservice.api.push.DistributionId; import org.thoughtcrime.securesms.groups.GroupAccessControl; import org.thoughtcrime.securesms.groups.GroupId; @@ -441,6 +444,47 @@ private static final String[] GROUP_PROJECTION = { return groupId; } + /** + * There was a point in time where we weren't properly responding to group creates on linked devices. This would result in us having a Recipient entry for the + * group, but we'd either be missing the group entry, or that entry would be missing a master key. This method fixes this scenario. + */ + public void fixMissingMasterKey(@NonNull GroupMasterKey groupMasterKey) { + GroupId.V2 groupId = GroupId.v2(groupMasterKey); + + if (getGroupV1ByExpectedV2(groupId).isPresent()) { + throw new MissedGroupMigrationInsertException(groupId); + } + + SQLiteDatabase db = databaseHelper.getWritableDatabase(); + + db.beginTransaction(); + try { + String query = GROUP_ID + " = ?"; + String[] args = SqlUtil.buildArgs(groupId); + ContentValues values = new ContentValues(); + + values.put(V2_MASTER_KEY, groupMasterKey.serialize()); + + int updated = db.update(TABLE_NAME, values, query, args); + + if (updated < 1) { + Log.w(TAG, "No group entry. Creating restore placeholder for " + groupId); + create(groupMasterKey, DecryptedGroup.newBuilder() + .setRevision(GroupsV2StateProcessor.RESTORE_PLACEHOLDER_REVISION) + .build()); + } else { + Log.w(TAG, "Had a group entry, but it was missing a master key. Updated."); + } + + db.setTransactionSuccessful(); + } finally { + db.endTransaction(); + } + + Log.w(TAG, "Scheduling request for latest group info for " + groupId); + ApplicationDependencies.getJobManager().add(new RequestGroupV2InfoJob(groupId)); + } + /** * @param groupMasterKey null for V1, must be non-null for V2 (presence dictates group version). */ 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 d92d407490..a85ee9e3b8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java @@ -274,6 +274,8 @@ public class StorageSyncJob extends BaseJob { try { self = freshSelf(); + Log.i(TAG, "[Remote Sync] Remote-Only :: Contacts: " + remoteContacts.size() + ", GV1: " + remoteGv1.size() + ", GV2: " + remoteGv2.size() + ", Account: " + remoteAccount.size()); + new ContactRecordProcessor(context, self).process(remoteContacts, StorageSyncHelper.KEY_GENERATOR); new GroupV1RecordProcessor(context).process(remoteGv1, StorageSyncHelper.KEY_GENERATOR); new GroupV2RecordProcessor(context).process(remoteGv2, StorageSyncHelper.KEY_GENERATOR); diff --git a/app/src/main/java/org/thoughtcrime/securesms/messages/MessageContentProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/messages/MessageContentProcessor.java index e8fc31b402..ed86d16624 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/messages/MessageContentProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/messages/MessageContentProcessor.java @@ -155,7 +155,6 @@ import org.whispersystems.signalservice.api.messages.shared.SharedContact; import org.whispersystems.signalservice.api.payments.Money; import org.whispersystems.signalservice.api.push.DistributionId; import org.whispersystems.signalservice.api.push.SignalServiceAddress; -import org.whispersystems.signalservice.internal.push.SignalServiceProtos; import java.io.IOException; import java.security.SecureRandom; @@ -243,21 +242,7 @@ public final class MessageContentProcessor { boolean isGv2Message = groupId.isPresent() && groupId.get().isV2(); if (isGv2Message) { - GroupId.V2 groupIdV2 = groupId.get().requireV2(); - - Optional possibleGv1 = groupDatabase.getGroupV1ByExpectedV2(groupIdV2); - if (possibleGv1.isPresent()) { - GroupsV1MigrationUtil.performLocalMigration(context, possibleGv1.get().getId().requireV1()); - } - - if (!updateGv2GroupFromServerOrP2PChange(content, message.getGroupContext().get().getGroupV2().get())) { - log(String.valueOf(content.getTimestamp()), "Ignoring GV2 message for group we are not currently in " + groupIdV2); - return; - } - - Recipient sender = Recipient.externalPush(context, content.getSender()); - if (!groupDatabase.isCurrentMember(groupIdV2, sender.getId())) { - log(String.valueOf(content.getTimestamp()), "Ignoring GV2 message from member not in group " + groupIdV2); + if (handleGv2PreProcessing(groupId.orNull().requireV2(), content, content.getDataMessage().get().getGroupContext().get().getGroupV2().get())) { return; } } @@ -395,8 +380,35 @@ public final class MessageContentProcessor { .enqueue(); } - private static @Nullable - SignalServiceGroupContext getGroupContextIfPresent(@NonNull SignalServiceContent content) { + /** + * @return True if the content should be ignored, otherwise false. + */ + private boolean handleGv2PreProcessing(GroupId.V2 groupId, SignalServiceContent content, SignalServiceGroupV2 groupV2) + throws IOException, GroupChangeBusyException + { + GroupDatabase groupDatabase = DatabaseFactory.getGroupDatabase(context); + Optional possibleGv1 = groupDatabase.getGroupV1ByExpectedV2(groupId); + + if (possibleGv1.isPresent()) { + GroupsV1MigrationUtil.performLocalMigration(context, possibleGv1.get().getId().requireV1()); + } + + if (!updateGv2GroupFromServerOrP2PChange(content, groupV2)) { + log(String.valueOf(content.getTimestamp()), "Ignoring GV2 message for group we are not currently in " + groupId); + return true; + } + + Recipient sender = Recipient.externalPush(context, content.getSender()); + if (!groupDatabase.isCurrentMember(groupId, sender.getId())) { + log(String.valueOf(content.getTimestamp()), "Ignoring GV2 message from member not in group " + groupId); + return true; + } + + return false; + } + + + private static @Nullable SignalServiceGroupContext getGroupContextIfPresent(@NonNull SignalServiceContent content) { if (content.getDataMessage().isPresent() && content.getDataMessage().get().getGroupContext().isPresent()) { return content.getDataMessage().get().getGroupContext().get(); } else if (content.getSyncMessage().isPresent() && @@ -678,10 +690,13 @@ public final class MessageContentProcessor { if (groupV1.getType() != SignalServiceGroup.Type.REQUEST_INFO) { ApplicationDependencies.getJobManager().add(new RequestGroupInfoJob(Recipient.externalHighTrustPush(context, content.getSender()).getId(), GroupId.v1(groupV1.getGroupId()))); } else { - warn(String.valueOf(content.getTimestamp()), "Received a REQUEST_INFO message for a group we don't know about. Ignoring."); + warn(content.getTimestamp(), "Received a REQUEST_INFO message for a group we don't know about. Ignoring."); } + } else if (group.getGroupV2().isPresent()) { + warn(content.getTimestamp(), "Received a GV2 message for a group we have no knowledge of -- attempting to fix this state."); + DatabaseFactory.getGroupDatabase(context).fixMissingMasterKey(group.getGroupV2().get().getMasterKey()); } else { - warn(String.valueOf(content.getTimestamp()), "Received a message for a group we don't know about without a GV1 context. Ignoring."); + warn(content.getTimestamp(), "Received a message for a group we don't know about without a group context. Ignoring."); } } @@ -870,7 +885,7 @@ public final class MessageContentProcessor { StorageSyncHelper.scheduleSyncForDataChange(); break; default: - Log.w(TAG, "Received a fetch message for an unknown type."); + warn(TAG, "Received a fetch message for an unknown type."); } } @@ -929,7 +944,7 @@ public final class MessageContentProcessor { Optional address = outgoingPaymentMessage.getAddress().transform(MobileCoinPublicAddress::fromBytes); if (!address.isPresent() && recipientId == null) { - Log.i(TAG, "Inserting defrag"); + log("Inserting defrag"); address = Optional.of(ApplicationDependencies.getPayments().getWallet().getMobileCoinPublicAddress()); recipientId = Recipient.self().getId(); } @@ -960,9 +975,9 @@ public final class MessageContentProcessor { GroupDatabase groupDatabase = DatabaseFactory.getGroupDatabase(context); if (message.getMessage().isGroupV2Message()) { - Optional possibleGv1 = groupDatabase.getGroupV1ByExpectedV2(GroupId.v2(message.getMessage().getGroupContext().get().getGroupV2().get().getMasterKey())); - if (possibleGv1.isPresent()) { - GroupsV1MigrationUtil.performLocalMigration(context, possibleGv1.get().getId().requireV1()); + GroupId.V2 groupId = GroupId.v2(message.getMessage().getGroupContext().get().getGroupV2().get().getMasterKey()); + if (handleGv2PreProcessing(groupId, content, message.getMessage().getGroupContext().get().getGroupV2().get())) { + return; } } @@ -981,7 +996,7 @@ public final class MessageContentProcessor { } else if (Build.VERSION.SDK_INT > 19 && message.getMessage().getGroupCallUpdate().isPresent()) { handleGroupCallUpdateMessage(content, message.getMessage(), GroupUtil.idFromGroupContext(message.getMessage().getGroupContext())); } else if (message.getMessage().isEmptyGroupV2Message()) { - // Do nothing + warn(content.getTimestamp(), "Empty GV2 message! Doing nothing."); } else if (message.getMessage().isExpirationUpdate()) { threadId = handleSynchronizeSentExpirationUpdate(message); } else if (message.getMessage().getReaction().isPresent()) { @@ -1013,7 +1028,7 @@ public final class MessageContentProcessor { } if (SignalStore.rateLimit().needsRecaptcha()) { - Log.i(TAG, "Got a sent transcript while in reCAPTCHA mode. Assuming we're good to message again."); + log(content.getTimestamp(), "Got a sent transcript while in reCAPTCHA mode. Assuming we're good to message again."); RateLimitUtil.retryAllRateLimitedMessages(context); } @@ -2150,6 +2165,10 @@ public final class MessageContentProcessor { Log.i(TAG, message); } + protected void log(long timestamp, @NonNull String message) { + log(String.valueOf(timestamp), message); + } + protected void log(@NonNull String extra, @NonNull String message) { String extraLog = Util.isEmpty(extra) ? "" : "[" + extra + "] "; Log.i(TAG, extraLog + message); 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 40fc48b83c..b367448fce 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java @@ -26,6 +26,7 @@ public final class GroupV2RecordProcessor extends DefaultStorageRecordProcessor< private final Context context; private final RecipientDatabase recipientDatabase; + private final GroupDatabase groupDatabase; private final Map gv1GroupsByExpectedGv2Id; public GroupV2RecordProcessor(@NonNull Context context) { @@ -35,6 +36,7 @@ public final class GroupV2RecordProcessor extends DefaultStorageRecordProcessor< GroupV2RecordProcessor(@NonNull Context context, @NonNull RecipientDatabase recipientDatabase, @NonNull GroupDatabase groupDatabase) { this.context = context; this.recipientDatabase = recipientDatabase; + this.groupDatabase = groupDatabase; this.gv1GroupsByExpectedGv2Id = groupDatabase.getAllExpectedV2Ids(); } @@ -54,7 +56,8 @@ public final class GroupV2RecordProcessor extends DefaultStorageRecordProcessor< if (settings.getSyncExtras().getGroupMasterKey() != null) { return StorageSyncModels.localToRemoteRecord(settings); } else { - Log.w(TAG, "No local master key. Assuming it matches remote since the groupIds match."); + Log.w(TAG, "No local master key. Assuming it matches remote since the groupIds match. Enqueuing a fetch to fix the bad state."); + groupDatabase.fixMissingMasterKey(record.getMasterKeyOrThrow()); return StorageSyncModels.localToRemoteRecord(settings, record.getMasterKeyOrThrow()); } })