From a92304c222152d344280cdfe559e0053ed674eb3 Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Wed, 29 Jul 2026 13:05:12 -0400 Subject: [PATCH] Ignore GV1 records in storage service. --- .../securesms/database/RecipientTable.kt | 48 ------- .../securesms/database/ThreadTable.kt | 5 - .../helpers/SignalDatabaseMigrations.kt | 6 +- .../V324_MoveGroupV1StorageIdsToUnknownIds.kt | 33 +++++ .../securesms/jobs/StorageSyncJob.kt | 29 +++-- .../storage/GroupV1RecordProcessor.kt | 99 -------------- .../securesms/storage/StorageSyncModels.kt | 20 --- ...4_MoveGroupV1StorageIdsToUnknownIdsTest.kt | 123 ++++++++++++++++++ .../securesms/jobs/StorageSyncJobTest.kt | 84 ++++++++++++ .../signalservice/api/storage/StorageId.java | 6 +- 10 files changed, 266 insertions(+), 187 deletions(-) create mode 100644 app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V324_MoveGroupV1StorageIdsToUnknownIds.kt delete mode 100644 app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.kt create mode 100644 app/src/test/java/org/thoughtcrime/securesms/database/helpers/migration/V324_MoveGroupV1StorageIdsToUnknownIdsTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt index 39fdee5343..06bf9dcbdf 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt @@ -107,7 +107,6 @@ import org.thoughtcrime.securesms.wallpaper.WallpaperStorage import org.whispersystems.signalservice.api.profiles.SignalServiceProfile import org.whispersystems.signalservice.api.storage.SignalAccountRecord import org.whispersystems.signalservice.api.storage.SignalContactRecord -import org.whispersystems.signalservice.api.storage.SignalGroupV1Record import org.whispersystems.signalservice.api.storage.SignalGroupV2Record import org.whispersystems.signalservice.api.storage.StorageId import org.whispersystems.signalservice.api.storage.signalAci @@ -1018,27 +1017,6 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da } } - fun applyStorageSyncGroupV1Insert(insert: SignalGroupV1Record) { - val id = writableDatabase.insertOrThrow(TABLE_NAME, null, getValuesForStorageGroupV1(insert, true)) - - val recipientId = RecipientId.from(id) - threads.applyStorageSyncUpdate(recipientId, insert) - AppDependencies.databaseObserver.notifyRecipientChanged(recipientId) - } - - fun applyStorageSyncGroupV1Update(update: StorageRecordUpdate) { - val values = getValuesForStorageGroupV1(update.new, false) - - val updateCount = writableDatabase.update(TABLE_NAME, values, STORAGE_SERVICE_ID + " = ?", arrayOf(Base64.encodeWithPadding(update.old.id.raw))) - if (updateCount < 1) { - throw AssertionError("Had an update, but it didn't match any rows!") - } - - val recipient = Recipient.externalGroupExact(GroupId.v1orThrow(update.old.proto.id.toByteArray())) - threads.applyStorageSyncUpdate(recipient.id, update.new) - recipient.live().refresh() - } - fun applyStorageSyncGroupV2Insert(insert: SignalGroupV2Record) { val masterKey = GroupMasterKey(insert.proto.masterKey.toByteArray()) val groupId = GroupId.v2(masterKey) @@ -1285,8 +1263,6 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da $STORAGE_SERVICE_ID NOT NULL AND ( ($TYPE = ${RecipientType.INDIVIDUAL.id} AND ($ACI_COLUMN NOT NULL OR $PNI_COLUMN NOT NULL) AND $ID != ${Recipient.self().id.toLong()}) OR - $TYPE = ${RecipientType.GV1.id} - OR ($TYPE = ${RecipientType.DISTRIBUTION_LIST.id} AND $DISTRIBUTION_LIST_ID NOT NULL AND $DISTRIBUTION_LIST_ID IN ( SELECT ${DistributionListTables.ListTable.ID} FROM ${DistributionListTables.ListTable.TABLE_NAME} @@ -1318,7 +1294,6 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da out[id] = StorageId.forContact(key) } } - RecipientType.GV1 -> out[id] = StorageId.forGroupV1(key) RecipientType.DISTRIBUTION_LIST -> out[id] = StorageId.forStoryDistributionList(key) RecipientType.CALL_LINK -> out[id] = StorageId.forCallLink(key) else -> throw AssertionError() @@ -4481,29 +4456,6 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da } } - private fun getValuesForStorageGroupV1(groupV1: SignalGroupV1Record, isInsert: Boolean): ContentValues { - return ContentValues().apply { - val groupId = GroupId.v1orThrow(groupV1.proto.id.toByteArray()) - - put(GROUP_ID, groupId.toString()) - put(TYPE, RecipientType.GV1.id) - put(PROFILE_SHARING, if (groupV1.proto.whitelisted) "1" else "0") - put(BLOCKED, if (groupV1.proto.blocked) "1" else "0") - put(MUTE_UNTIL, groupV1.proto.mutedUntilTimestamp) - put(STORAGE_SERVICE_ID, Base64.encodeWithPadding(groupV1.id.raw)) - - if (groupV1.proto.hasUnknownFields()) { - put(STORAGE_SERVICE_PROTO, Base64.encodeWithPadding(groupV1.serializedUnknowns!!)) - } else { - putNull(STORAGE_SERVICE_PROTO) - } - - if (isInsert) { - put(AVATAR_COLOR, AvatarColorHash.forGroupId(groupId).serialize()) - } - } - } - private fun getValuesForStorageGroupV2(groupV2: SignalGroupV2Record, isInsert: Boolean): ContentValues { return ContentValues().apply { val groupId = GroupId.v2(GroupMasterKey(groupV2.proto.masterKey.toByteArray())) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/ThreadTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/ThreadTable.kt index 366af18638..39e5dea666 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/ThreadTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/ThreadTable.kt @@ -82,7 +82,6 @@ import org.thoughtcrime.securesms.util.isPoll import org.thoughtcrime.securesms.util.isScheduled import org.whispersystems.signalservice.api.storage.SignalAccountRecord import org.whispersystems.signalservice.api.storage.SignalContactRecord -import org.whispersystems.signalservice.api.storage.SignalGroupV1Record import org.whispersystems.signalservice.api.storage.SignalGroupV2Record import org.whispersystems.signalservice.api.storage.toSignalServiceAddress import org.whispersystems.signalservice.internal.storage.protos.AccountRecord @@ -1608,10 +1607,6 @@ class ThreadTable(context: Context, databaseHelper: SignalDatabase) : DatabaseTa applyStorageSyncUpdate(recipientId, record.proto.archived, record.proto.markedUnread, isGroup = false) } - fun applyStorageSyncUpdate(recipientId: RecipientId, record: SignalGroupV1Record) { - applyStorageSyncUpdate(recipientId, record.proto.archived, record.proto.markedUnread, isGroup = true) - } - fun applyStorageSyncUpdate(recipientId: RecipientId, record: SignalGroupV2Record) { applyStorageSyncUpdate(recipientId, record.proto.archived, record.proto.markedUnread, isGroup = true) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt index 215f883a4b..5765eb62d8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt @@ -176,6 +176,7 @@ import org.thoughtcrime.securesms.database.helpers.migration.V320_AddAttachmentT import org.thoughtcrime.securesms.database.helpers.migration.V321_AddScheduledMessageIndex import org.thoughtcrime.securesms.database.helpers.migration.V322_NormalizeStickerTable import org.thoughtcrime.securesms.database.helpers.migration.V323_AddStickerPackStorageSync +import org.thoughtcrime.securesms.database.helpers.migration.V324_MoveGroupV1StorageIdsToUnknownIds import org.thoughtcrime.securesms.database.SQLiteDatabase as SignalSqliteDatabase /** @@ -359,10 +360,11 @@ object SignalDatabaseMigrations { 320 to V320_AddAttachmentThumbnailFileAndUuidIndexes, 321 to V321_AddScheduledMessageIndex, 322 to V322_NormalizeStickerTable, - 323 to V323_AddStickerPackStorageSync + 323 to V323_AddStickerPackStorageSync, + 324 to V324_MoveGroupV1StorageIdsToUnknownIds ) - const val DATABASE_VERSION = 323 + const val DATABASE_VERSION = 324 @JvmStatic fun migrate(context: Application, db: SignalSqliteDatabase, oldVersion: Int, newVersion: Int) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V324_MoveGroupV1StorageIdsToUnknownIds.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V324_MoveGroupV1StorageIdsToUnknownIds.kt new file mode 100644 index 0000000000..ec3c1b0e0c --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V324_MoveGroupV1StorageIdsToUnknownIds.kt @@ -0,0 +1,33 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.database.helpers.migration + +import android.app.Application +import org.thoughtcrime.securesms.database.SQLiteDatabase + +/** + * Copies the storage id off of every gv1 recipient row into the unknown id table so the ids stay in the storage + * service manifest, then clears the storage columns on those rows. + * + * A gv1 row can still be given a storage id after this runs, it is just never read, so this is not a lasting invariant. + */ +@Suppress("ClassName") +object V324_MoveGroupV1StorageIdsToUnknownIds : SignalDatabaseMigration { + + private const val RECIPIENT_TYPE_GV1 = 2 + private const val MANIFEST_TYPE_GROUPV1 = 2 + + override fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { + db.execSQL( + """ + INSERT OR IGNORE INTO storage_key (type, key) + SELECT $MANIFEST_TYPE_GROUPV1, storage_service_id FROM recipient WHERE type = $RECIPIENT_TYPE_GV1 AND storage_service_id NOT NULL + """ + ) + + db.execSQL("UPDATE recipient SET storage_service_id = NULL, storage_service_proto = NULL WHERE type = $RECIPIENT_TYPE_GV1") + } +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt index 60e341ba12..df1a2922be 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt @@ -26,7 +26,6 @@ import org.thoughtcrime.securesms.storage.AccountRecordProcessor import org.thoughtcrime.securesms.storage.CallLinkRecordProcessor import org.thoughtcrime.securesms.storage.ChatFolderRecordProcessor import org.thoughtcrime.securesms.storage.ContactRecordProcessor -import org.thoughtcrime.securesms.storage.GroupV1RecordProcessor import org.thoughtcrime.securesms.storage.GroupV2RecordProcessor import org.thoughtcrime.securesms.storage.NotificationProfileRecordProcessor import org.thoughtcrime.securesms.storage.StickerPackRecordProcessor @@ -45,7 +44,6 @@ import org.whispersystems.signalservice.api.storage.SignalAccountRecord import org.whispersystems.signalservice.api.storage.SignalCallLinkRecord import org.whispersystems.signalservice.api.storage.SignalChatFolderRecord import org.whispersystems.signalservice.api.storage.SignalContactRecord -import org.whispersystems.signalservice.api.storage.SignalGroupV1Record import org.whispersystems.signalservice.api.storage.SignalGroupV2Record import org.whispersystems.signalservice.api.storage.SignalNotificationProfileRecord import org.whispersystems.signalservice.api.storage.SignalStickerPackRecord @@ -57,7 +55,6 @@ import org.whispersystems.signalservice.api.storage.toSignalAccountRecord import org.whispersystems.signalservice.api.storage.toSignalCallLinkRecord import org.whispersystems.signalservice.api.storage.toSignalChatFolderRecord import org.whispersystems.signalservice.api.storage.toSignalContactRecord -import org.whispersystems.signalservice.api.storage.toSignalGroupV1Record import org.whispersystems.signalservice.api.storage.toSignalGroupV2Record import org.whispersystems.signalservice.api.storage.toSignalNotificationProfileRecord import org.whispersystems.signalservice.api.storage.toSignalStickerPackRecord @@ -336,7 +333,7 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc db.beginTransaction() try { - Log.i(TAG, "[Remote Sync] Remote-Only :: Contacts: ${remoteOnly.contacts.size}, GV1: ${remoteOnly.gv1.size}, GV2: ${remoteOnly.gv2.size}, Account: ${remoteOnly.account.size}, DLists: ${remoteOnly.storyDistributionLists.size}, call links: ${remoteOnly.callLinkRecords.size}, chat folders: ${remoteOnly.chatFolderRecords.size}, notification profiles: ${remoteOnly.notificationProfileRecords.size}, sticker packs: ${remoteOnly.stickerPackRecords.size}") + Log.i(TAG, "[Remote Sync] Remote-Only :: Contacts: ${remoteOnly.contacts.size}, GV2: ${remoteOnly.gv2.size}, Account: ${remoteOnly.account.size}, DLists: ${remoteOnly.storyDistributionLists.size}, call links: ${remoteOnly.callLinkRecords.size}, chat folders: ${remoteOnly.chatFolderRecords.size}, notification profiles: ${remoteOnly.notificationProfileRecords.size}, sticker packs: ${remoteOnly.stickerPackRecords.size}") processKnownRecords(context, remoteOnly) @@ -409,8 +406,20 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc Log.i(TAG, "Removed $removedUnregistered unregistered, $removedDeletedFolders folders, $removedDeletedProfiles notification profiles, $removedDeletedPacks sticker packs from storage service that have been deleted for longer than ${RemoteConfig.messageQueueTime.milliseconds.inWholeDays} days.") } - val localStorageIds = getAllLocalStorageIds(self) - val idDifference = StorageSyncHelper.findIdDifference(remoteManifest.storageIds, localStorageIds) + var localStorageIds = getAllLocalStorageIds(self) + var idDifference = StorageSyncHelper.findIdDifference(remoteManifest.storageIds, localStorageIds) + + // We can't build a record for an unknown id, so declaring one the remote doesn't already have would fail validation. + val localOnlyUnknownIds = idDifference.localOnlyIds.filter { it.isUnknown } + if (localOnlyUnknownIds.isNotEmpty()) { + Log.w(TAG, "Found ${localOnlyUnknownIds.size} unknown ids that aren't in the remote manifest. Removed them. Recalculating diff.") + + SignalDatabase.unknownStorageIds.delete(localOnlyUnknownIds) + + localStorageIds = getAllLocalStorageIds(self) + idDifference = StorageSyncHelper.findIdDifference(remoteManifest.storageIds, localStorageIds) + } + val remoteInserts = buildLocalStorageRecords(context, self, idDifference.localOnlyIds.stream().filter { it: StorageId -> !it.isUnknown }.collect(Collectors.toList())) val remoteDeletes = idDifference.remoteOnlyIds.stream().map { obj: StorageId -> obj.raw }.collect(Collectors.toList()) @@ -469,7 +478,6 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc @Throws(IOException::class) private fun processKnownRecords(context: Context, records: StorageRecordCollection) { ContactRecordProcessor().process(records.contacts, StorageSyncHelper.KEY_GENERATOR) - GroupV1RecordProcessor().process(records.gv1, StorageSyncHelper.KEY_GENERATOR) GroupV2RecordProcessor().process(records.gv2, StorageSyncHelper.KEY_GENERATOR) NotificationProfileRecordProcessor().process(records.notificationProfileRecords, StorageSyncHelper.KEY_GENERATOR) AccountRecordProcessor(context, freshSelf()).process(records.account, StorageSyncHelper.KEY_GENERATOR) @@ -502,7 +510,7 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc } when (type) { - ManifestRecord.Identifier.Type.CONTACT, ManifestRecord.Identifier.Type.GROUPV1, ManifestRecord.Identifier.Type.GROUPV2 -> { + ManifestRecord.Identifier.Type.CONTACT, ManifestRecord.Identifier.Type.GROUPV2 -> { val settings = SignalDatabase.recipients.getByStorageId(id.raw) if (settings != null) { if (settings.recipientType == RecipientTable.RecipientType.GV2 && settings.syncExtras.groupMasterKey == null) { @@ -599,13 +607,12 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc private fun getKnownTypes(): List { return ManifestRecord.Identifier.Type.entries - .filter { it != ManifestRecord.Identifier.Type.UNKNOWN } .map { it.value } + .filter { StorageId.isKnownType(it) } } private class StorageRecordCollection(records: Collection) { val contacts: MutableList = mutableListOf() - val gv1: MutableList = mutableListOf() val gv2: MutableList = mutableListOf() val account: MutableList = mutableListOf() val unknown: MutableList = mutableListOf() @@ -619,8 +626,6 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc for (record in records) { if (record.proto.contact != null) { contacts += record.proto.contact!!.toSignalContactRecord(record.id) - } else if (record.proto.groupV1 != null) { - gv1 += record.proto.groupV1!!.toSignalGroupV1Record(record.id) } else if (record.proto.groupV2 != null) { gv2 += record.proto.groupV2!!.toSignalGroupV2Record(record.id) } else if (record.proto.account != null) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.kt b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.kt deleted file mode 100644 index 14432b55c0..0000000000 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.kt +++ /dev/null @@ -1,99 +0,0 @@ -package org.thoughtcrime.securesms.storage - -import org.signal.core.util.logging.Log -import org.thoughtcrime.securesms.database.GroupTable -import org.thoughtcrime.securesms.database.RecipientTable -import org.thoughtcrime.securesms.database.SignalDatabase -import org.thoughtcrime.securesms.database.model.RecipientRecord -import org.thoughtcrime.securesms.groups.BadGroupIdException -import org.thoughtcrime.securesms.groups.GroupId -import org.whispersystems.signalservice.api.storage.SignalGroupV1Record -import org.whispersystems.signalservice.api.storage.SignalStorageRecord -import org.whispersystems.signalservice.api.storage.StorageId -import org.whispersystems.signalservice.api.storage.toSignalGroupV1Record -import java.util.Optional - -/** - * Record processor for [SignalGroupV1Record]. - * Handles merging and updating our local store when processing remote gv1 storage records. - */ -class GroupV1RecordProcessor(private val groupDatabase: GroupTable, private val recipientTable: RecipientTable) : DefaultStorageRecordProcessor() { - companion object { - private val TAG = Log.tag(GroupV1RecordProcessor::class.java) - } - - constructor() : this(SignalDatabase.groups, SignalDatabase.recipients) - - /** - * We want to catch: - * - Invalid group ID's - * - GV1 ID's that map to GV2 ID's, meaning we've already migrated them. - * - * Note: This method could be written more succinctly, but the logs are useful :) - */ - override fun isInvalid(remote: SignalGroupV1Record): Boolean { - try { - val id = GroupId.v1(remote.proto.id.toByteArray()) - val v2Record = groupDatabase.getGroup(id.deriveV2MigrationGroupId()) - - if (v2Record.isPresent) { - Log.w(TAG, "We already have an upgraded V2 group for this V1 group -- marking as invalid.") - return true - } else { - return false - } - } catch (e: BadGroupIdException) { - Log.w(TAG, "Bad Group ID -- marking as invalid.") - return true - } - } - - override fun getMatching(remote: SignalGroupV1Record, keyGenerator: StorageKeyGenerator): Optional { - val groupId = GroupId.v1orThrow(remote.proto.id.toByteArray()) - - val recipientId = recipientTable.getByGroupId(groupId) - - return recipientId - .map { recipientTable.getRecordForSync(it)!! } - .map { settings: RecipientRecord -> StorageSyncModels.localToRemoteRecord(settings) } - .map { record: SignalStorageRecord -> record.proto.groupV1!!.toSignalGroupV1Record(record.id) } - } - - override fun merge(remote: SignalGroupV1Record, local: SignalGroupV1Record, keyGenerator: StorageKeyGenerator): SignalGroupV1Record { - val merged = SignalGroupV1Record.newBuilder(remote.serializedUnknowns).apply { - id = remote.proto.id - blocked = remote.proto.blocked - whitelisted = remote.proto.whitelisted - archived = remote.proto.archived - markedUnread = remote.proto.markedUnread - mutedUntilTimestamp = remote.proto.mutedUntilTimestamp - }.build().toSignalGroupV1Record(StorageId.forGroupV1(keyGenerator.generate())) - - val matchesRemote = doParamsMatch(remote, merged) - val matchesLocal = doParamsMatch(local, merged) - - return if (matchesRemote) { - remote - } else if (matchesLocal) { - local - } else { - merged - } - } - - override fun insertLocal(record: SignalGroupV1Record) { - recipientTable.applyStorageSyncGroupV1Insert(record) - } - - override fun updateLocal(update: StorageRecordUpdate) { - recipientTable.applyStorageSyncGroupV1Update(update) - } - - override fun compare(lhs: SignalGroupV1Record, rhs: SignalGroupV1Record): Int { - return if (lhs.proto.id == rhs.proto.id) { - 0 - } else { - 1 - } - } -} diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.kt b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.kt index d952774579..b807e2690c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.kt @@ -38,7 +38,6 @@ import org.whispersystems.signalservice.api.storage.IAPSubscriptionId import org.whispersystems.signalservice.api.storage.SignalCallLinkRecord import org.whispersystems.signalservice.api.storage.SignalChatFolderRecord import org.whispersystems.signalservice.api.storage.SignalContactRecord -import org.whispersystems.signalservice.api.storage.SignalGroupV1Record import org.whispersystems.signalservice.api.storage.SignalGroupV2Record import org.whispersystems.signalservice.api.storage.SignalNotificationProfileRecord import org.whispersystems.signalservice.api.storage.SignalStickerPackRecord @@ -48,7 +47,6 @@ import org.whispersystems.signalservice.api.storage.StorageId import org.whispersystems.signalservice.api.storage.toSignalCallLinkRecord import org.whispersystems.signalservice.api.storage.toSignalChatFolderRecord import org.whispersystems.signalservice.api.storage.toSignalContactRecord -import org.whispersystems.signalservice.api.storage.toSignalGroupV1Record import org.whispersystems.signalservice.api.storage.toSignalGroupV2Record import org.whispersystems.signalservice.api.storage.toSignalNotificationProfileRecord import org.whispersystems.signalservice.api.storage.toSignalStickerPackRecord @@ -90,7 +88,6 @@ object StorageSyncModels { fun localToRemoteRecord(settings: RecipientRecord, rawStorageId: ByteArray): SignalStorageRecord { return when (settings.recipientType) { RecipientType.INDIVIDUAL -> localToRemoteContact(settings, rawStorageId).toSignalStorageRecord() - RecipientType.GV1 -> localToRemoteGroupV1(settings, rawStorageId).toSignalStorageRecord() RecipientType.GV2 -> localToRemoteGroupV2(settings, rawStorageId, settings.syncExtras.groupMasterKey!!).toSignalStorageRecord() RecipientType.DISTRIBUTION_LIST -> localToRemoteStoryDistributionList(settings, rawStorageId).toSignalStorageRecord() RecipientType.CALL_LINK -> localToRemoteCallLink(settings, rawStorageId).toSignalStorageRecord() @@ -229,23 +226,6 @@ object StorageSyncModels { }.build().toSignalContactRecord(StorageId.forContact(rawStorageId)) } - private fun localToRemoteGroupV1(recipient: RecipientRecord, rawStorageId: ByteArray): SignalGroupV1Record { - val groupId = recipient.groupId ?: throw AssertionError("Must have a groupId!") - - if (!groupId.isV1) { - throw AssertionError("Group is not V1") - } - - return SignalGroupV1Record.newBuilder(recipient.syncExtras.storageProto).apply { - id = recipient.groupId.requireV1().decodedId.toByteString() - blocked = recipient.isBlocked - whitelisted = recipient.profileSharing - archived = recipient.syncExtras.isArchived - markedUnread = recipient.syncExtras.isForcedUnread - mutedUntilTimestamp = recipient.muteUntil - }.build().toSignalGroupV1Record(StorageId.forGroupV1(rawStorageId)) - } - private fun localToRemoteGroupV2(recipient: RecipientRecord, rawStorageId: ByteArray?, groupMasterKey: GroupMasterKey): SignalGroupV2Record { val groupId = recipient.groupId ?: throw AssertionError("Must have a groupId!") diff --git a/app/src/test/java/org/thoughtcrime/securesms/database/helpers/migration/V324_MoveGroupV1StorageIdsToUnknownIdsTest.kt b/app/src/test/java/org/thoughtcrime/securesms/database/helpers/migration/V324_MoveGroupV1StorageIdsToUnknownIdsTest.kt new file mode 100644 index 0000000000..fdda2eb1da --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/database/helpers/migration/V324_MoveGroupV1StorageIdsToUnknownIdsTest.kt @@ -0,0 +1,123 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.database.helpers.migration + +import android.app.Application +import androidx.test.core.app.ApplicationProvider +import assertk.assertThat +import assertk.assertions.isEqualTo +import assertk.assertions.isNull +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.signal.core.util.insertInto +import org.signal.core.util.readToList +import org.signal.core.util.readToSingleObject +import org.signal.core.util.requireInt +import org.signal.core.util.requireNonNullString +import org.signal.core.util.requireString +import org.signal.core.util.select +import org.thoughtcrime.securesms.testutil.SignalDatabaseMigrationRule + +@Suppress("ClassName") +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, application = Application::class) +class V324_MoveGroupV1StorageIdsToUnknownIdsTest { + + companion object { + private const val RECIPIENT_TYPE_GV1 = 2 + private const val RECIPIENT_TYPE_GV2 = 3 + private const val MANIFEST_TYPE_GROUPV1 = 2 + } + + @get:Rule val signalDatabaseRule = SignalDatabaseMigrationRule(323) + + private val db get() = signalDatabaseRule.database + + @Test + fun migrate_movesGroupV1StorageIdIntoUnknownIds() { + insertRecipient(groupId = "gv1", type = RECIPIENT_TYPE_GV1, storageId = "storage-id-gv1") + + migrate() + + assertThat(unknownIdKeys()).isEqualTo(listOf("storage-id-gv1")) + assertThat(unknownIdTypeOf("storage-id-gv1")).isEqualTo(MANIFEST_TYPE_GROUPV1) + } + + @Test + fun migrate_clearsGroupV1StorageIdAndProto() { + insertRecipient(groupId = "gv1", type = RECIPIENT_TYPE_GV1, storageId = "storage-id-gv1", storageProto = "proto") + + migrate() + + assertThat(storageIdOf("gv1")).isNull() + assertThat(storageProtoOf("gv1")).isNull() + } + + @Test + fun migrate_leavesOtherRecipientTypesAlone() { + insertRecipient(groupId = "gv2", type = RECIPIENT_TYPE_GV2, storageId = "storage-id-gv2") + + migrate() + + assertThat(storageIdOf("gv2")).isEqualTo("storage-id-gv2") + assertThat(unknownIdKeys()).isEqualTo(emptyList()) + } + + @Test + fun migrate_ignoresGroupV1RecipientsWithoutAStorageId() { + insertRecipient(groupId = "gv1", type = RECIPIENT_TYPE_GV1, storageId = null) + + migrate() + + assertThat(unknownIdKeys()).isEqualTo(emptyList()) + } + + /** The unknown ID table has a UNIQUE constraint on key, so a pre-existing row must not blow up the insert. */ + @Test + fun migrate_toleratesAnIdThatIsAlreadyTracked() { + insertRecipient(groupId = "gv1", type = RECIPIENT_TYPE_GV1, storageId = "storage-id-gv1") + db.insertInto("storage_key").values("type" to MANIFEST_TYPE_GROUPV1, "key" to "storage-id-gv1").run() + + migrate() + + assertThat(unknownIdKeys()).isEqualTo(listOf("storage-id-gv1")) + assertThat(storageIdOf("gv1")).isNull() + } + + private fun migrate() { + V324_MoveGroupV1StorageIdsToUnknownIds.migrate(ApplicationProvider.getApplicationContext(), db, 323, 324) + } + + private fun insertRecipient(groupId: String, type: Int, storageId: String?, storageProto: String? = null) { + db.insertInto("recipient") + .values( + "group_id" to groupId, + "type" to type, + "storage_service_id" to storageId, + "storage_service_proto" to storageProto + ) + .run() + } + + private fun unknownIdKeys(): List { + return db.select("key").from("storage_key").run().readToList { it.requireNonNullString("key") } + } + + private fun unknownIdTypeOf(key: String): Int? { + return db.select("type").from("storage_key").where("key = ?", key).run().readToSingleObject { it.requireInt("type") } + } + + private fun storageIdOf(groupId: String): String? { + return db.select("storage_service_id").from("recipient").where("group_id = ?", groupId).run().readToSingleObject { it.requireString("storage_service_id") } + } + + private fun storageProtoOf(groupId: String): String? { + return db.select("storage_service_proto").from("recipient").where("group_id = ?", groupId).run().readToSingleObject { it.requireString("storage_service_proto") } + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt b/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt index 3f4a424f1e..70b069f536 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt @@ -11,6 +11,7 @@ import io.mockk.every import okio.ByteString.Companion.toByteString import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertTrue import org.junit.Before @@ -27,6 +28,7 @@ import org.signal.core.util.logging.Log import org.signal.core.util.withinTransaction import org.thoughtcrime.securesms.database.SignalDatabase import org.thoughtcrime.securesms.database.model.StickerPackId +import org.thoughtcrime.securesms.groups.GroupId import org.thoughtcrime.securesms.jobmanager.Job import org.thoughtcrime.securesms.profiles.ProfileName import org.thoughtcrime.securesms.recipients.Recipient @@ -37,6 +39,7 @@ import org.thoughtcrime.securesms.testutil.SystemOutLogger import org.whispersystems.signalservice.api.storage.SignalStorageRecord import org.whispersystems.signalservice.api.storage.StorageId import org.whispersystems.signalservice.internal.storage.protos.ContactRecord +import org.whispersystems.signalservice.internal.storage.protos.GroupV1Record import org.whispersystems.signalservice.internal.storage.protos.StickerPackRecord import org.whispersystems.signalservice.internal.storage.protos.StorageRecord import java.util.UUID @@ -222,6 +225,74 @@ class StorageSyncJobTest { assertArrayEquals(record.id.raw, pack!!.storageServiceId!!.raw) } + @Test + fun `given a remote GV1 record, when I run, then I keep it in the manifest`() { + val record = groupV1Record() + remoteStorage.addRemoteRecords(listOf(record)) + + val result = runJob(StorageSyncJob.forRemoteChange()) + + assertTrue(result.isSuccess) + assertEquals(0, remoteStorage.writeCount) + assertTrue(remoteStorage.manifest!!.storageIds.contains(record.id)) + assertTrue(SignalDatabase.unknownStorageIds.allUnknownIds.contains(record.id)) + } + + @Test + fun `given a remote GV1 record, when I run, then I do not apply it locally`() { + val groupId = GroupId.v1(Util.getSecretBytes(16)) + remoteStorage.addRemoteRecords(listOf(groupV1Record(groupId))) + + val result = runJob(StorageSyncJob.forRemoteChange()) + + assertTrue(result.isSuccess) + assertFalse(SignalDatabase.recipients.getByGroupId(groupId).isPresent) + } + + @Test + fun `given a GV1 recipient with a storage ID, when I run, then I ignore it entirely`() { + val storageId = StorageId.forGroupV1(Util.getSecretBytes(16)) + val recipientId = SignalDatabase.recipients.getOrInsertFromGroupId(GroupId.v1(Util.getSecretBytes(16))) + SignalDatabase.recipients.updateStorageId(recipientId, storageId.raw) + + val result = runJob(StorageSyncJob.forLocalChange()) + + assertTrue(result.isSuccess) + assertEquals(0, remoteStorage.writeCount) + assertFalse(remoteStorage.manifest!!.storageIds.contains(storageId)) + } + + @Test + fun `given a newer remote manifest without my GV1 ID, when I run, then I stop tracking it`() { + val record = groupV1Record() + remoteStorage.addRemoteRecords(listOf(record)) + check(runJob(StorageSyncJob.forRemoteChange()).isSuccess) + check(SignalDatabase.unknownStorageIds.allUnknownIds.contains(record.id)) + + val withoutGroupV1 = remoteStorage.records.filterNot { it.id == record.id } + remoteStorage.setRemoteState(withoutGroupV1, version = remoteStorage.manifest!!.version + 1) + + val result = runJob(StorageSyncJob.forRemoteChange()) + + assertTrue(result.isSuccess) + assertFalse(SignalDatabase.unknownStorageIds.allUnknownIds.contains(record.id)) + } + + @Test + fun `given a local-only unknown ID and a local contact, when I run, then I write only the contact and drop the unknown ID`() { + val strandedId = StorageId.forGroupV1(Util.getSecretBytes(16)) + insertUnknownStorageId(strandedId) + SignalDatabase.recipients.rotateStorageId(recipients.createRecipient("Local Contact")) + + val result = runJob(StorageSyncJob.forLocalChange()) + + assertTrue(result.isSuccess) + assertEquals(1, remoteStorage.writeCount) + assertEquals(1, remoteStorage.records.count { it.proto.contact?.givenName == "Local" }) + assertFalse(remoteStorage.manifest!!.storageIds.contains(strandedId)) + assertFalse(SignalDatabase.unknownStorageIds.allUnknownIds.contains(strandedId)) + } + /** * Gets us to a steady state: remote holds our account record at version 1, then a sync pushes up everything else * the fresh database came with (the default chat folder), leaving both sides at [BASE_MANIFEST_VERSION]. @@ -278,6 +349,19 @@ class StorageSyncJobTest { } } + private fun groupV1Record(groupId: GroupId.V1 = GroupId.v1(Util.getSecretBytes(16)), storageId: StorageId = StorageId.forGroupV1(Util.getSecretBytes(16))): SignalStorageRecord { + return SignalStorageRecord( + id = storageId, + proto = StorageRecord( + groupV1 = GroupV1Record( + id = groupId.decodedId.toByteString(), + blocked = true, + whitelisted = true + ) + ) + ) + } + private fun runJob(job: StorageSyncJob): Job.Result { job.setContext(ApplicationProvider.getApplicationContext()) return job.run() diff --git a/lib/libsignal-service/src/main/java/org/whispersystems/signalservice/api/storage/StorageId.java b/lib/libsignal-service/src/main/java/org/whispersystems/signalservice/api/storage/StorageId.java index ce3cdded81..d52b12efed 100644 --- a/lib/libsignal-service/src/main/java/org/whispersystems/signalservice/api/storage/StorageId.java +++ b/lib/libsignal-service/src/main/java/org/whispersystems/signalservice/api/storage/StorageId.java @@ -75,9 +75,13 @@ public class StorageId { return new StorageId(type, key); } + /** + * GROUPV1 is deliberately excluded. We no longer read or write gv1 records, so treating them as unknown lets us keep + * their ids in the manifest instead of deleting them. + */ public static boolean isKnownType(int val) { for (ManifestRecord.Identifier.Type type : ManifestRecord.Identifier.Type.values()) { - if (type != ManifestRecord.Identifier.Type.UNKNOWN && type.getValue() == val) { + if (type != ManifestRecord.Identifier.Type.UNKNOWN && type != ManifestRecord.Identifier.Type.GROUPV1 && type.getValue() == val) { return true; } }