Skip to content

Commit 93604f5

Browse files
cody-signalcody-signal
authored
Fix incorrect left group in local state bug.
1 parent 8777c1f commit 93604f5

File tree

6 files changed

+156
-11
lines changed

6 files changed

+156
-11
lines changed

‎app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import org.signal.core.util.requireLong
3030
import org.signal.core.util.requireNonNullString
3131
import org.signal.core.util.requireString
3232
import org.signal.core.util.select
33+
import org.signal.core.util.toInt
3334
import org.signal.core.util.update
3435
import org.signal.core.util.withinTransaction
3536
import org.signal.libsignal.zkgroup.InvalidInputException
@@ -510,6 +511,13 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT
510511
return Reader(cursor)
511512
}
512513

514+
fun getInactiveGroups(): Reader {
515+
val query = SqlUtil.buildQuery("$TABLE_NAME.$ACTIVE = ?", false.toInt())
516+
val select = "${joinedGroupSelect()} WHERE ${query.where}"
517+
518+
return Reader(readableDatabase.query(select, query.whereArgs))
519+
}
520+
513521
fun getActiveGroupCount(): Int {
514522
return readableDatabase
515523
.select("COUNT(*)")

‎app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.kt

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -112,14 +112,15 @@ class GroupsV2StateProcessor private constructor(
112112
@WorkerThread
113113
@Throws(IOException::class, GroupNotAMemberException::class)
114114
fun forceSanityUpdateFromServer(timestamp: Long): GroupUpdateResult {
115-
val currentLocalState: DecryptedGroup? = SignalDatabase.groups.getGroup(groupId).map { it.requireV2GroupProperties().decryptedGroup }.orNull()
115+
val groupRecord = SignalDatabase.groups.getGroup(groupId).orNull()
116+
val currentLocalState: DecryptedGroup? = groupRecord?.requireV2GroupProperties()?.decryptedGroup
116117

117118
if (currentLocalState == null) {
118119
Log.i(TAG, "$logPrefix No local state to force update")
119120
return GroupUpdateResult.CONSISTENT_OR_AHEAD
120121
}
121122

122-
return when (val result = updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = true)) {
123+
return when (val result = updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = true, forceUpdate = !groupRecord.isActive)) {
123124
InternalUpdateResult.NoUpdateNeeded -> GroupUpdateResult.CONSISTENT_OR_AHEAD
124125
is InternalUpdateResult.Updated -> GroupUpdateResult(GroupUpdateResult.UpdateStatus.GROUP_UPDATED, result.updatedLocalState)
125126
is InternalUpdateResult.NotAMember -> throw result.exception
@@ -267,7 +268,8 @@ class GroupsV2StateProcessor private constructor(
267268
timestamp = timestamp,
268269
serverGuid = serverGuid,
269270
groupStateDiff = groupStateDiff,
270-
groupSendEndorsements = null
271+
groupSendEndorsements = null,
272+
forceSave = forceApply
271273
)
272274
}
273275

@@ -281,7 +283,7 @@ class GroupsV2StateProcessor private constructor(
281283

282284
if (targetRevision == LATEST && (currentLocalState == null || currentLocalState.revision == RESTORE_PLACEHOLDER_REVISION)) {
283285
Log.i(TAG, "$logPrefix Latest revision only, update to latest directly")
284-
return updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = false)
286+
return updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = false, forceUpdate = false)
285287
}
286288

287289
Log.i(TAG, "$logPrefix Paging from server targetRevision: ${if (targetRevision == LATEST) "latest" else targetRevision}")
@@ -292,7 +294,7 @@ class GroupsV2StateProcessor private constructor(
292294
val joinedAtFailure = InternalUpdateResult.from(joinedAtResult.getCause()!!)
293295
if (joinedAtFailure is InternalUpdateResult.NotAMember) {
294296
Log.i(TAG, "$logPrefix Not a member, try to update to latest directly")
295-
return updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = currentLocalState != null)
297+
return updateToLatestViaServer(timestamp, currentLocalState, reconstructChange = currentLocalState != null, forceUpdate = true)
296298
} else {
297299
return joinedAtFailure
298300
}
@@ -322,7 +324,9 @@ class GroupsV2StateProcessor private constructor(
322324
val applyGroupStateDiffResult: AdvanceGroupStateResult = GroupStatePatcher.applyGroupStateDiff(remoteGroupStateDiff, targetRevision)
323325
val updatedGroupState: DecryptedGroup? = applyGroupStateDiffResult.updatedGroupState
324326

325-
if (updatedGroupState == null || updatedGroupState == remoteGroupStateDiff.previousGroupState) {
327+
if (groupRecord.map { it.isActive }.orNull() == false && updatedGroupState != null && updatedGroupState == remoteGroupStateDiff.previousGroupState) {
328+
Log.w(TAG, "$logPrefix Local state is not active, but server is returning state for us, apply regardless of revision")
329+
} else if (updatedGroupState == null || updatedGroupState == remoteGroupStateDiff.previousGroupState) {
326330
Log.i(TAG, "$logPrefix Local state is at or later than server revision: ${currentLocalState?.revision ?: "null"}")
327331

328332
if (currentLocalState != null && applyGroupStateDiffResult.remainingRemoteGroupChanges.isEmpty()) {
@@ -391,7 +395,7 @@ class GroupsV2StateProcessor private constructor(
391395
return InternalUpdateResult.Updated(currentLocalState!!)
392396
}
393397

394-
private fun updateToLatestViaServer(timestamp: Long, currentLocalState: DecryptedGroup?, reconstructChange: Boolean): InternalUpdateResult {
398+
private fun updateToLatestViaServer(timestamp: Long, currentLocalState: DecryptedGroup?, reconstructChange: Boolean, forceUpdate: Boolean): InternalUpdateResult {
395399
val result = groupsApi.getGroupAsResult(groupSecretParams, groupsV2Authorization.getAuthorizationForToday(serviceIds, groupSecretParams))
396400

397401
val groupResponse = if (result is NetworkResult.Success) {
@@ -407,7 +411,8 @@ class GroupsV2StateProcessor private constructor(
407411
timestamp = timestamp,
408412
serverGuid = null,
409413
groupStateDiff = remoteGroupStateDiff,
410-
groupSendEndorsements = groupOperations.receiveGroupSendEndorsements(serviceIds.aci, groupResponse.group, groupResponse.groupSendEndorsementsResponse)
414+
groupSendEndorsements = groupOperations.receiveGroupSendEndorsements(serviceIds.aci, groupResponse.group, groupResponse.groupSendEndorsementsResponse),
415+
forceSave = forceUpdate && groupResponse.group.members.asSequence().mapNotNull { ACI.parseOrNull(it.aciBytes) }.any { serviceIds.matches(it) }
411416
)
412417
}
413418

@@ -480,13 +485,16 @@ class GroupsV2StateProcessor private constructor(
480485
timestamp: Long,
481486
serverGuid: String?,
482487
groupStateDiff: GroupStateDiff,
483-
groupSendEndorsements: ReceivedGroupSendEndorsements?
488+
groupSendEndorsements: ReceivedGroupSendEndorsements?,
489+
forceSave: Boolean
484490
): InternalUpdateResult {
485491
val currentLocalState: DecryptedGroup? = groupStateDiff.previousGroupState
486492
val applyGroupStateDiffResult = GroupStatePatcher.applyGroupStateDiff(groupStateDiff, GroupStatePatcher.LATEST)
487493
val updatedGroupState = applyGroupStateDiffResult.updatedGroupState
488494

489-
if (updatedGroupState == null || updatedGroupState == groupStateDiff.previousGroupState) {
495+
if (forceSave && updatedGroupState != null) {
496+
Log.w(TAG, "$logPrefix Forcing local state update regardless of revision")
497+
} else if (updatedGroupState == null || updatedGroupState == groupStateDiff.previousGroupState) {
490498
Log.i(TAG, "$logPrefix Local state and server state are equal")
491499

492500
if (groupSendEndorsements != null) {

‎app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767
import org.thoughtcrime.securesms.migrations.EmojiSearchIndexCheckMigrationJob;
6868
import org.thoughtcrime.securesms.migrations.GooglePlayBillingPurchaseTokenMigrationJob;
6969
import org.thoughtcrime.securesms.migrations.IdentityTableCleanupMigrationJob;
70+
import org.thoughtcrime.securesms.migrations.InactiveGroupCheckMigrationJob;
7071
import org.thoughtcrime.securesms.migrations.LegacyMigrationJob;
7172
import org.thoughtcrime.securesms.migrations.MigrationCompleteJob;
7273
import org.thoughtcrime.securesms.migrations.OptimizeMessageSearchIndexMigrationJob;
@@ -293,6 +294,7 @@ public static Map<String, Job.Factory> getJobFactories(@NonNull Application appl
293294
put(EmojiSearchIndexCheckMigrationJob.KEY, new EmojiSearchIndexCheckMigrationJob.Factory());
294295
put(GooglePlayBillingPurchaseTokenMigrationJob.KEY, new GooglePlayBillingPurchaseTokenMigrationJob.Factory());
295296
put(IdentityTableCleanupMigrationJob.KEY, new IdentityTableCleanupMigrationJob.Factory());
297+
put(InactiveGroupCheckMigrationJob.KEY, new InactiveGroupCheckMigrationJob.Factory());
296298
put(LegacyMigrationJob.KEY, new LegacyMigrationJob.Factory());
297299
put(MigrationCompleteJob.KEY, new MigrationCompleteJob.Factory());
298300
put(OptimizeMessageSearchIndexMigrationJob.KEY, new OptimizeMessageSearchIndexMigrationJob.Factory());

‎app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,9 +168,10 @@ static final class Version {
168168
static final int GPB_TOKEN_MIGRATION = 124;
169169
static final int GROUP_ADD_MIGRATION = 125;
170170
static final int SSRE2_CAPABILITY = 126;
171+
static final int FIX_INACTIVE_GROUPS = 127;
171172
}
172173

173-
public static final int CURRENT_VERSION = 126;
174+
public static final int CURRENT_VERSION = 127;
174175

175176
/**
176177
* This *must* be called after the {@link JobManager} has been instantiated, but *before* the call
@@ -773,6 +774,10 @@ private static LinkedHashMap<Integer, MigrationJob> getMigrationJobs(@NonNull Co
773774
jobs.put(Version.SSRE2_CAPABILITY, new AttributesMigrationJob());
774775
}
775776

777+
if (lastSeenVersion < Version.FIX_INACTIVE_GROUPS) {
778+
jobs.put(Version.FIX_INACTIVE_GROUPS, new InactiveGroupCheckMigrationJob());
779+
}
780+
776781
return jobs;
777782
}
778783

‎app/src/main/java/org/thoughtcrime/securesms/migrations/InactiveGroupCheckMigrationJob.kt

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
package org.thoughtcrime.securesms.migrations
2+
3+
import org.signal.core.util.logging.Log
4+
import org.signal.storageservice.protos.groups.local.DecryptedGroup
5+
import org.thoughtcrime.securesms.database.SignalDatabase
6+
import org.thoughtcrime.securesms.dependencies.AppDependencies
7+
import org.thoughtcrime.securesms.jobmanager.Job
8+
import org.thoughtcrime.securesms.jobs.RequestGroupV2InfoJob
9+
import org.thoughtcrime.securesms.keyvalue.SignalStore
10+
import org.whispersystems.signalservice.api.push.ServiceId
11+
import org.whispersystems.signalservice.api.push.ServiceIds
12+
13+
/**
14+
* Migration to fix groups that we locally marked as inactive because of the server
15+
* but may not actually be left.
16+
*/
17+
internal class InactiveGroupCheckMigrationJob(
18+
parameters: Parameters = Parameters.Builder().build()
19+
) : MigrationJob(parameters) {
20+
21+
companion object {
22+
val TAG = Log.tag(InactiveGroupCheckMigrationJob::class.java)
23+
const val KEY = "InactiveGroupCheckMigrationJob"
24+
}
25+
26+
override fun getFactoryKey(): String = KEY
27+
28+
override fun isUiBlocking(): Boolean = false
29+
30+
override fun performMigration() {
31+
if (SignalStore.account.aci == null) {
32+
Log.w(TAG, "ACI missing, abort")
33+
return
34+
}
35+
36+
val serviceIds = SignalStore.account.getServiceIds()
37+
38+
SignalDatabase
39+
.groups
40+
.getInactiveGroups()
41+
.use { reader ->
42+
reader
43+
.asSequence()
44+
.filter { it.isV2Group }
45+
.filter { it.requireV2GroupProperties().decryptedGroup.isMember(serviceIds) }
46+
.forEach {
47+
AppDependencies.jobManager.add(RequestGroupV2InfoJob(it.id.requireV2()))
48+
}
49+
}
50+
}
51+
52+
private fun DecryptedGroup.isMember(serviceIds: ServiceIds): Boolean {
53+
return this
54+
.members
55+
.asSequence()
56+
.mapNotNull { ServiceId.ACI.Companion.parseOrNull(it.aciBytes) }
57+
.any { serviceIds.matches(it) }
58+
}
59+
60+
override fun shouldRetry(e: Exception): Boolean = false
61+
62+
class Factory : Job.Factory<InactiveGroupCheckMigrationJob> {
63+
override fun create(parameters: Parameters, serializedData: ByteArray?): InactiveGroupCheckMigrationJob {
64+
return InactiveGroupCheckMigrationJob(parameters)
65+
}
66+
}
67+
}

‎app/src/test/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessorTest.kt

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -990,4 +990,59 @@ class GroupsV2StateProcessorTest {
990990

991991
verify { groupTable.update(masterKey, result.latestServer!!, null) }
992992
}
993+
994+
/**
995+
* If we think a group is not active locally, but the server returns state for us indicating it thinks we are active,
996+
* apply the state regardless of revision.
997+
*/
998+
@Test
999+
fun localInactiveButActiveOnServer() {
1000+
given {
1001+
localState(
1002+
active = false,
1003+
revision = 5,
1004+
members = selfAndOthers
1005+
)
1006+
changeSet {
1007+
}
1008+
apiCallParameters(requestedRevision = 5, includeFirst = false)
1009+
joinedAtRevision = 0
1010+
expectTableUpdate = true
1011+
}
1012+
1013+
val result = processor.updateLocalGroupToRevision(
1014+
targetRevision = GroupsV2StateProcessor.LATEST,
1015+
timestamp = 0
1016+
)
1017+
1018+
assertThat(result.updateStatus, "inactive local is still updated with same revision").isEqualTo(GroupUpdateResult.UpdateStatus.GROUP_UPDATED)
1019+
}
1020+
1021+
/**
1022+
* If we think a group is not active locally, but the server returns state for us indicating it thinks we are active,
1023+
* apply the state regardless of revision.
1024+
*/
1025+
@Test
1026+
fun forceUpdateLocalInactiveButActiveOnServer() {
1027+
given {
1028+
localState(
1029+
active = false,
1030+
revision = 5,
1031+
members = selfAndOthers
1032+
)
1033+
serverState(
1034+
revision = 5,
1035+
members = selfAndOthers
1036+
)
1037+
apiCallParameters(requestedRevision = 5, includeFirst = false)
1038+
joinedAtRevision = 0
1039+
expectTableUpdate = true
1040+
}
1041+
1042+
val result = processor.forceSanityUpdateFromServer(
1043+
timestamp = 0
1044+
)
1045+
1046+
assertThat(result.updateStatus, "inactive local is still updated given same revision from server").isEqualTo(GroupUpdateResult.UpdateStatus.GROUP_UPDATED)
1047+
}
9931048
}

0 commit comments

Comments
 (0)