Erase group entries a config merge recreated after another device erased them

app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt

@@ -34,10 +34,12 @@ import org.session.libsession.utilities.getGroup
 import org.session.libsession.utilities.withGroupConfigs
 import org.session.libsession.utilities.withMutableUserConfigs
 import org.session.libsignal.utilities.AccountId
+import org.session.libsignal.utilities.Log
 import org.thoughtcrime.securesms.auth.LoginStateRepository
 import org.thoughtcrime.securesms.configs.ConfigToDatabaseSync
 import org.thoughtcrime.securesms.database.ConfigDatabase
 import org.thoughtcrime.securesms.database.ConfigVariant
+import org.thoughtcrime.securesms.groups.isErasedGroupStub
 import java.util.EnumSet
 import java.util.concurrent.locks.ReentrantReadWriteLock
 import javax.inject.Inject
@@ -55,6 +57,8 @@ class ConfigFactory @Inject constructor(
     @param:ManagerScope private val coroutineScope: CoroutineScope
 ) : ConfigFactoryProtocol {
     companion object {
+        private const val TAG = "ConfigFactory"
+
         // This is a buffer period within which we will process messages which would result in a
         // config change, any message which would normally result in a config change which was sent
         // before `lastConfigMessage.timestamp - configChangeBufferPeriod` will not  actually have
@@ -276,6 +280,8 @@ class ConfigFactory @Inject constructor(
             return
         }
 
+        var erasedGroupStubs = emptyList<AccountId>()
+
         val result = doWithMutableUserConfigs(fromMerge = true) { configs ->
             val config = when (userConfigType) {
                 UserConfigType.CONTACTS -> configs.contacts
@@ -291,11 +297,30 @@ class ConfigFactory @Inject constructor(
                 .mapNotNull { hash -> messages.firstOrNull { it.hash == hash } }
                 .maxOfOrNull { it.timestamp }
 
+            // Finish the erase the merge undid before anything reads the entry back as a group
+            if (userConfigType == UserConfigType.USER_GROUPS) {
+                erasedGroupStubs = configs.userGroups.allClosedGroupInfo()
+                    .filter { it.isErasedGroupStub() }
+                    .map { AccountId(it.groupAccountId) }
+
+                erasedGroupStubs.forEach { groupId ->
+                    Log.w(TAG, "Erasing a group the merge recreated after another device erased it")
+                    configs.userGroups.eraseClosedGroup(groupId.hexString)
+                    configs.convoInfoVolatile.eraseClosedGroup(groupId.hexString)
+                }
+            }
+
+            val changed = EnumSet.of(userConfigType)
+            if (erasedGroupStubs.isNotEmpty()) changed.add(UserConfigType.CONVO_INFO_VOLATILE)
+
             maxTimestamp?.let {
-                (config.dump() to it) to EnumSet.of(userConfigType)
-            } ?: (null to emptySet())
+                (config.dump() to it) to changed
+            } ?: (null to if (erasedGroupStubs.isEmpty()) emptySet() else changed)
         }
 
+        // A merge write is neither pushed nor fully dumped; removeGroup's write is both
+        erasedGroupStubs.forEach(::removeGroup)
+
         // Dump now regardless so we can save the timestamp to the database
         if (result != null) {
             val (dump, timestamp) = result

app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt

@@ -0,0 +1,20 @@
+package org.thoughtcrime.securesms.groups
+
+import network.loki.messenger.libsession_util.util.GroupInfo
+
+/**
+ * Whether this UserGroups entry is what a config merge leaves of a group another of our devices
+ * erased, rather than a group we are (or were) in.
+ *
+ * libsession applies each device's diff in turn, so when one device erases a group while another
+ * changes one of its fields (marking it destroyed after seeing the group's info, say), the merge
+ * recreates the erased entry holding only the changed fields: no name and no keys. Every client
+ * keeps the name on its kicked and destroyed entries (libsession's mark_kicked/mark_destroyed clear
+ * only the keys), so a removed entry without one can only be this. libsession's header describes
+ * the name as invite-only, though: if any client starts clearing it after joining, this rule has to
+ * change.
+ *
+ * The other clients apply the same rule; keep them in step.
+ */
+fun GroupInfo.ClosedGroupInfo.isErasedGroupStub(): Boolean =
+    (destroyed || kicked) && name.isEmpty() && adminKey == null && authData == null

app/src/test/java/org/thoughtcrime/securesms/groups/ErasedGroupStubTest.kt

@@ -0,0 +1,43 @@
+package org.thoughtcrime.securesms.groups
+
+import com.google.common.truth.Truth.assertThat
+import network.loki.messenger.libsession_util.PRIORITY_VISIBLE
+import network.loki.messenger.libsession_util.util.Bytes
+import network.loki.messenger.libsession_util.util.GroupInfo
+import org.junit.Test
+
+class ErasedGroupStubTest {
+    private val stub = GroupInfo.ClosedGroupInfo(
+        groupAccountId = "03" + "ab".repeat(32),
+        adminKey = null,
+        authData = null,
+        priority = PRIORITY_VISIBLE,
+        invited = false,
+        name = "",
+        destroyed = true,
+        joinedAtSecs = 0L,
+        kicked = false,
+    )
+
+    @Test
+    fun `a removed entry with no name and no keys is an erased-group stub`() {
+        assertThat(stub.isErasedGroupStub()).isTrue()
+        assertThat(stub.copy(destroyed = false, kicked = true).isErasedGroupStub()).isTrue()
+    }
+
+    @Test
+    fun `a destroyed group we were in keeps its name and is not a stub`() {
+        assertThat(stub.copy(name = "Book club").isErasedGroupStub()).isFalse()
+    }
+
+    @Test
+    fun `an entry with an admin key or auth data is not a stub`() {
+        assertThat(stub.copy(adminKey = Bytes(ByteArray(64) { 1 })).isErasedGroupStub()).isFalse()
+        assertThat(stub.copy(authData = Bytes(ByteArray(100) { 7 })).isErasedGroupStub()).isFalse()
+    }
+
+    @Test
+    fun `a group still in use is never a stub, even without a name`() {
+        assertThat(stub.copy(destroyed = false).isErasedGroupStub()).isFalse()
+    }
+}