From 93604f53d4254bb81e69bbb1648eb92dca92608a Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Wed, 15 Jan 2025 11:50:00 -0500 Subject: [PATCH] Fix incorrect left group in local state bug. --- .../securesms/database/GroupTable.kt | 8 +++ .../v2/processing/GroupsV2StateProcessor.kt | 28 +++++--- .../securesms/jobs/JobManagerFactories.java | 2 + .../migrations/ApplicationMigrations.java | 7 +- .../InactiveGroupCheckMigrationJob.kt | 67 +++++++++++++++++++ .../processing/GroupsV2StateProcessorTest.kt | 55 +++++++++++++++ 6 files changed, 156 insertions(+), 11 deletions(-) create mode 100644 app/src/main/java/org/thoughtcrime/securesms/migrations/InactiveGroupCheckMigrationJob.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt index 9742bbc012..8374633a15 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt @@ -30,6 +30,7 @@ import org.signal.core.util.requireLong import org.signal.core.util.requireNonNullString import org.signal.core.util.requireString import org.signal.core.util.select +import org.signal.core.util.toInt import org.signal.core.util.update import org.signal.core.util.withinTransaction import org.signal.libsignal.zkgroup.InvalidInputException @@ -510,6 +511,13 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT return Reader(cursor) } + fun getInactiveGroups(): Reader { + val query = SqlUtil.buildQuery("$TABLE_NAME.$ACTIVE = ?", false.toInt()) + val select = "${joinedGroupSelect()} WHERE ${query.where}" + + return Reader(readableDatabase.query(select, query.whereArgs)) + } + fun getActiveGroupCount(): Int { return readableDatabase .select("COUNT(*)") diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.kt index 79553b530b..2435cd3867 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.kt @@ -112,14 +112,15 @@ class GroupsV2StateProcessor private constructor( @WorkerThread @Throws(IOException::class, GroupNotAMemberException::class) fun forceSanityUpdateFromServer(timestamp: Long): GroupUpdateResult { - val currentLocalState: DecryptedGroup? = SignalDatabase.groups.getGroup(groupId).map { it.requireV2GroupProperties().decryptedGroup }.orNull() + val groupRecord = SignalDatabase.groups.getGroup(groupId).orNull() + val currentLocalState: DecryptedGroup? = groupRecord?.requireV2GroupProperties()?.decryptedGroup if (currentLocalState == null) { Log.i(TAG, "$logPrefix No local state to force update") return GroupUpdateResult.CONSISTENT_OR_AHEAD } - return when (val result = updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = true)) { + return when (val result = updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = true, forceUpdate = !groupRecord.isActive)) { InternalUpdateResult.NoUpdateNeeded -> GroupUpdateResult.CONSISTENT_OR_AHEAD is InternalUpdateResult.Updated -> GroupUpdateResult(GroupUpdateResult.UpdateStatus.GROUP_UPDATED, result.updatedLocalState) is InternalUpdateResult.NotAMember -> throw result.exception @@ -267,7 +268,8 @@ class GroupsV2StateProcessor private constructor( timestamp = timestamp, serverGuid = serverGuid, groupStateDiff = groupStateDiff, - groupSendEndorsements = null + groupSendEndorsements = null, + forceSave = forceApply ) } @@ -281,7 +283,7 @@ class GroupsV2StateProcessor private constructor( if (targetRevision == LATEST && (currentLocalState == null || currentLocalState.revision == RESTORE_PLACEHOLDER_REVISION)) { Log.i(TAG, "$logPrefix Latest revision only, update to latest directly") - return updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = false) + return updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = false, forceUpdate = false) } Log.i(TAG, "$logPrefix Paging from server targetRevision: ${if (targetRevision == LATEST) "latest" else targetRevision}") @@ -292,7 +294,7 @@ class GroupsV2StateProcessor private constructor( val joinedAtFailure = InternalUpdateResult.from(joinedAtResult.getCause()!!) if (joinedAtFailure is InternalUpdateResult.NotAMember) { Log.i(TAG, "$logPrefix Not a member, try to update to latest directly") - return updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = currentLocalState != null) + return updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = currentLocalState != null, forceUpdate = true) } else { return joinedAtFailure } @@ -322,7 +324,9 @@ class GroupsV2StateProcessor private constructor( val applyGroupStateDiffResult: AdvanceGroupStateResult = GroupStatePatcher.applyGroupStateDiff(remoteGroupStateDiff, targetRevision) val updatedGroupState: DecryptedGroup? = applyGroupStateDiffResult.updatedGroupState - if (updatedGroupState == null || updatedGroupState == remoteGroupStateDiff.previousGroupState) { + if (groupRecord.map { it.isActive }.orNull() == false && updatedGroupState != null && updatedGroupState == remoteGroupStateDiff.previousGroupState) { + Log.w(TAG, "$logPrefix Local state is not active, but server is returning state for us, apply regardless of revision") + } else if (updatedGroupState == null || updatedGroupState == remoteGroupStateDiff.previousGroupState) { Log.i(TAG, "$logPrefix Local state is at or later than server revision: ${currentLocalState?.revision ?: "null"}") if (currentLocalState != null && applyGroupStateDiffResult.remainingRemoteGroupChanges.isEmpty()) { @@ -391,7 +395,7 @@ class GroupsV2StateProcessor private constructor( return InternalUpdateResult.Updated(currentLocalState!!) } - private fun updateToLatestViaServer(timestamp: Long, currentLocalState: DecryptedGroup?, reconstructChange: Boolean): InternalUpdateResult { + private fun updateToLatestViaServer(timestamp: Long, currentLocalState: DecryptedGroup?, reconstructChange: Boolean, forceUpdate: Boolean): InternalUpdateResult { val result = groupsApi.getGroupAsResult(groupSecretParams, groupsV2Authorization.getAuthorizationForToday(serviceIds, groupSecretParams)) val groupResponse = if (result is NetworkResult.Success) { @@ -407,7 +411,8 @@ class GroupsV2StateProcessor private constructor( timestamp = timestamp, serverGuid = null, groupStateDiff = remoteGroupStateDiff, - groupSendEndorsements = groupOperations.receiveGroupSendEndorsements(serviceIds.aci, groupResponse.group, groupResponse.groupSendEndorsementsResponse) + groupSendEndorsements = groupOperations.receiveGroupSendEndorsements(serviceIds.aci, groupResponse.group, groupResponse.groupSendEndorsementsResponse), + forceSave = forceUpdate && groupResponse.group.members.asSequence().mapNotNull { ACI.parseOrNull(it.aciBytes) }.any { serviceIds.matches(it) } ) } @@ -480,13 +485,16 @@ class GroupsV2StateProcessor private constructor( timestamp: Long, serverGuid: String?, groupStateDiff: GroupStateDiff, - groupSendEndorsements: ReceivedGroupSendEndorsements? + groupSendEndorsements: ReceivedGroupSendEndorsements?, + forceSave: Boolean ): InternalUpdateResult { val currentLocalState: DecryptedGroup? = groupStateDiff.previousGroupState val applyGroupStateDiffResult = GroupStatePatcher.applyGroupStateDiff(groupStateDiff, GroupStatePatcher.LATEST) val updatedGroupState = applyGroupStateDiffResult.updatedGroupState - if (updatedGroupState == null || updatedGroupState == groupStateDiff.previousGroupState) { + if (forceSave && updatedGroupState != null) { + Log.w(TAG, "$logPrefix Forcing local state update regardless of revision") + } else if (updatedGroupState == null || updatedGroupState == groupStateDiff.previousGroupState) { Log.i(TAG, "$logPrefix Local state and server state are equal") if (groupSendEndorsements != null) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java index 20706ed2a8..e78be1968a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java @@ -67,6 +67,7 @@ import org.thoughtcrime.securesms.migrations.EmojiDownloadMigrationJob; import org.thoughtcrime.securesms.migrations.EmojiSearchIndexCheckMigrationJob; import org.thoughtcrime.securesms.migrations.GooglePlayBillingPurchaseTokenMigrationJob; import org.thoughtcrime.securesms.migrations.IdentityTableCleanupMigrationJob; +import org.thoughtcrime.securesms.migrations.InactiveGroupCheckMigrationJob; import org.thoughtcrime.securesms.migrations.LegacyMigrationJob; import org.thoughtcrime.securesms.migrations.MigrationCompleteJob; import org.thoughtcrime.securesms.migrations.OptimizeMessageSearchIndexMigrationJob; @@ -293,6 +294,7 @@ public final class JobManagerFactories { put(EmojiSearchIndexCheckMigrationJob.KEY, new EmojiSearchIndexCheckMigrationJob.Factory()); put(GooglePlayBillingPurchaseTokenMigrationJob.KEY, new GooglePlayBillingPurchaseTokenMigrationJob.Factory()); put(IdentityTableCleanupMigrationJob.KEY, new IdentityTableCleanupMigrationJob.Factory()); + put(InactiveGroupCheckMigrationJob.KEY, new InactiveGroupCheckMigrationJob.Factory()); put(LegacyMigrationJob.KEY, new LegacyMigrationJob.Factory()); put(MigrationCompleteJob.KEY, new MigrationCompleteJob.Factory()); put(OptimizeMessageSearchIndexMigrationJob.KEY, new OptimizeMessageSearchIndexMigrationJob.Factory()); diff --git a/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java b/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java index 00f2d102c9..ebb4c1a73c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java +++ b/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java @@ -168,9 +168,10 @@ public class ApplicationMigrations { static final int GPB_TOKEN_MIGRATION = 124; static final int GROUP_ADD_MIGRATION = 125; static final int SSRE2_CAPABILITY = 126; + static final int FIX_INACTIVE_GROUPS = 127; } - public static final int CURRENT_VERSION = 126; + public static final int CURRENT_VERSION = 127; /** * This *must* be called after the {@link JobManager} has been instantiated, but *before* the call @@ -773,6 +774,10 @@ public class ApplicationMigrations { jobs.put(Version.SSRE2_CAPABILITY, new AttributesMigrationJob()); } + if (lastSeenVersion < Version.FIX_INACTIVE_GROUPS) { + jobs.put(Version.FIX_INACTIVE_GROUPS, new InactiveGroupCheckMigrationJob()); + } + return jobs; } diff --git a/app/src/main/java/org/thoughtcrime/securesms/migrations/InactiveGroupCheckMigrationJob.kt b/app/src/main/java/org/thoughtcrime/securesms/migrations/InactiveGroupCheckMigrationJob.kt new file mode 100644 index 0000000000..c64b221f41 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/migrations/InactiveGroupCheckMigrationJob.kt @@ -0,0 +1,67 @@ +package org.thoughtcrime.securesms.migrations + +import org.signal.core.util.logging.Log +import org.signal.storageservice.protos.groups.local.DecryptedGroup +import org.thoughtcrime.securesms.database.SignalDatabase +import org.thoughtcrime.securesms.dependencies.AppDependencies +import org.thoughtcrime.securesms.jobmanager.Job +import org.thoughtcrime.securesms.jobs.RequestGroupV2InfoJob +import org.thoughtcrime.securesms.keyvalue.SignalStore +import org.whispersystems.signalservice.api.push.ServiceId +import org.whispersystems.signalservice.api.push.ServiceIds + +/** + * Migration to fix groups that we locally marked as inactive because of the server + * but may not actually be left. + */ +internal class InactiveGroupCheckMigrationJob( + parameters: Parameters = Parameters.Builder().build() +) : MigrationJob(parameters) { + + companion object { + val TAG = Log.tag(InactiveGroupCheckMigrationJob::class.java) + const val KEY = "InactiveGroupCheckMigrationJob" + } + + override fun getFactoryKey(): String = KEY + + override fun isUiBlocking(): Boolean = false + + override fun performMigration() { + if (SignalStore.account.aci == null) { + Log.w(TAG, "ACI missing, abort") + return + } + + val serviceIds = SignalStore.account.getServiceIds() + + SignalDatabase + .groups + .getInactiveGroups() + .use { reader -> + reader + .asSequence() + .filter { it.isV2Group } + .filter { it.requireV2GroupProperties().decryptedGroup.isMember(serviceIds) } + .forEach { + AppDependencies.jobManager.add(RequestGroupV2InfoJob(it.id.requireV2())) + } + } + } + + private fun DecryptedGroup.isMember(serviceIds: ServiceIds): Boolean { + return this + .members + .asSequence() + .mapNotNull { ServiceId.ACI.Companion.parseOrNull(it.aciBytes) } + .any { serviceIds.matches(it) } + } + + override fun shouldRetry(e: Exception): Boolean = false + + class Factory : Job.Factory { + override fun create(parameters: Parameters, serializedData: ByteArray?): InactiveGroupCheckMigrationJob { + return InactiveGroupCheckMigrationJob(parameters) + } + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessorTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessorTest.kt index 2b058d89b1..dcd98bf6fe 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessorTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessorTest.kt @@ -990,4 +990,59 @@ class GroupsV2StateProcessorTest { verify { groupTable.update(masterKey, result.latestServer!!, null) } } + + /** + * If we think a group is not active locally, but the server returns state for us indicating it thinks we are active, + * apply the state regardless of revision. + */ + @Test + fun localInactiveButActiveOnServer() { + given { + localState( + active = false, + revision = 5, + members = selfAndOthers + ) + changeSet { + } + apiCallParameters(requestedRevision = 5, includeFirst = false) + joinedAtRevision = 0 + expectTableUpdate = true + } + + val result = processor.updateLocalGroupToRevision( + targetRevision = GroupsV2StateProcessor.LATEST, + timestamp = 0 + ) + + assertThat(result.updateStatus, "inactive local is still updated with same revision").isEqualTo(GroupUpdateResult.UpdateStatus.GROUP_UPDATED) + } + + /** + * If we think a group is not active locally, but the server returns state for us indicating it thinks we are active, + * apply the state regardless of revision. + */ + @Test + fun forceUpdateLocalInactiveButActiveOnServer() { + given { + localState( + active = false, + revision = 5, + members = selfAndOthers + ) + serverState( + revision = 5, + members = selfAndOthers + ) + apiCallParameters(requestedRevision = 5, includeFirst = false) + joinedAtRevision = 0 + expectTableUpdate = true + } + + val result = processor.forceSanityUpdateFromServer( + timestamp = 0 + ) + + assertThat(result.updateStatus, "inactive local is still updated given same revision from server").isEqualTo(GroupUpdateResult.UpdateStatus.GROUP_UPDATED) + } }