Fix: a duplicate group invite demoted a member who had already joined

app/src/main/java/org/session/libsession/messaging/jobs/InviteContactsJob.kt

@@ -12,6 +12,7 @@ import kotlinx.coroutines.awaitAll
 import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.withContext
 import network.loki.messenger.libsession_util.ED25519
+import network.loki.messenger.libsession_util.util.GroupMember
 import org.session.libsession.messaging.groups.GroupInviteException
 import org.session.libsession.messaging.messages.Destination
 import org.session.libsession.messaging.messages.control.GroupUpdated
@@ -106,6 +107,17 @@ class InviteContactsJob @AssistedInject constructor(
             configFactory.withMutableGroupConfigs(sessionId) { configs ->
                 results.forEach { (memberSessionId, result) ->
                     configs.groupMembers.get(memberSessionId)?.let { member ->
+                        // A re-invite reaches members who already accepted, and the outcome of
+                        // sending them another invitation says nothing about a membership they
+                        // already have - recording it would drag them back to "invite sent" for
+                        // everyone in the group. isAdminOrBeingPromoted covers the admin flag and a
+                        // promotion still in flight, which is the same membership seen later on.
+                        val status = configs.groupMembers.status(member)
+                        if (status == GroupMember.Status.INVITE_ACCEPTED ||
+                            member.isAdminOrBeingPromoted(status)) {
+                            return@let
+                        }
+
                         if (result.isFailure) {
                             member.setInviteFailed()
                         } else {

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

@@ -281,11 +281,13 @@ class GroupManagerV2Impl @Inject constructor(
             for ((id, shareHistory) in memberInvites) {
                 val hex = id.hexString
 
-                val toSet = configs.groupMembers.get(hex)
-                    ?.also { existing ->
-                        val status = configs.groupMembers.status(existing)
-                        if (status == GroupMember.Status.INVITE_FAILED || status == GroupMember.Status.INVITE_SENT) {
-                            existing.setSupplement(shareHistory)
+                val existing = configs.groupMembers.get(hex)
+                val existingStatus = existing?.let(configs.groupMembers::status)
+
+                val toSet = existing
+                    ?.also { member ->
+                        if (existingStatus == GroupMember.Status.INVITE_FAILED || existingStatus == GroupMember.Status.INVITE_SENT) {
+                            member.setSupplement(shareHistory)
                         }
                     }
                     ?: configs.groupMembers.getOrConstruct(hex).also { member ->
@@ -297,7 +299,16 @@ class GroupManagerV2Impl @Inject constructor(
 
                 if (shareHistory) shareHistoryHexes += hex
 
-                toSet.setInvited()
+                // Someone who has already accepted must not be dragged back to "invited": this runs
+                // for a re-invite too, and the member state we hold is the newer truth. InviteContactsJob
+                // writes the send result over this a moment later, and skips the same members for the
+                // same reason.
+                val alreadyInGroup = existing != null && existingStatus != null &&
+                        (existingStatus == GroupMember.Status.INVITE_ACCEPTED ||
+                                existing.isAdminOrBeingPromoted(existingStatus))
+                if (!alreadyInGroup) {
+                    toSet.setInvited()
+                }
                 configs.groupMembers.set(toSet)
             }
 
@@ -720,6 +731,50 @@ class GroupManagerV2Impl @Inject constructor(
             }
         }
 
+    /**
+     * Tells the inviting admin we are in the group, and drops the invitation from our swarm.
+     *
+     * Split out of [approveGroupInvite] for the case where we are already a member: the parts of
+     * approval that write our membership must not run again, but the parts the admin depends on
+     * must.
+     */
+    private suspend fun acknowledgeInvitation(
+        group: GroupInfo.ClosedGroupInfo,
+        inviteMessageHash: String?
+    ) {
+        if (group.adminKey == null) {
+            val inviteResponse = GroupUpdateInviteResponseMessage.newBuilder()
+                .setIsApproved(true)
+            val responseData = GroupUpdateMessage.newBuilder()
+                .setInviteResponse(inviteResponse)
+
+            runCatching {
+                messageSender.sendNonDurably(
+                    GroupUpdated(responseData.build()),
+                    Destination.ClosedGroup(group.groupAccountId),
+                    isSyncMessage = false
+                )
+            }
+        }
+
+        deleteInviteMessage(inviteMessageHash)
+    }
+
+    private suspend fun deleteInviteMessage(inviteMessageHash: String?) {
+        if (inviteMessageHash == null) return
+
+        val auth = requireNotNull(storage.userAuth)
+        swarmApiExecutor.execute(
+            SwarmApiRequest(
+                swarmPubKeyHex = auth.accountId.hexString,
+                api = deleteMessageApiFactory.create(
+                    messageHashes = listOf(inviteMessageHash),
+                    swarmAuth = auth
+                )
+            )
+        )
+    }
+
     private suspend fun approveGroupInvite(
         group: GroupInfo.ClosedGroupInfo,
         inviteMessageHash: String?
@@ -784,18 +839,7 @@ class GroupManagerV2Impl @Inject constructor(
         }
 
         // Delete the invite once we have approved
-        if (inviteMessageHash != null) {
-            val auth = requireNotNull(storage.userAuth)
-            swarmApiExecutor.execute(
-                SwarmApiRequest(
-                    swarmPubKeyHex = auth.accountId.hexString,
-                    api = deleteMessageApiFactory.create(
-                        messageHashes = listOf(inviteMessageHash),
-                        swarmAuth = auth
-                    )
-                )
-            )
-        }
+        deleteInviteMessage(inviteMessageHash)
     }
 
     override suspend fun handleInvitation(
@@ -901,6 +945,21 @@ class GroupManagerV2Impl @Inject constructor(
         inviteMessageTimestamp: Long,
         inviteMessageHash: String,
     ) {
+        val existing = configFactory.getGroup(groupId)
+        if (existing != null && !existing.invited && !existing.kicked && !existing.destroyed) {
+            // Rebuilding ClosedGroupInfo for a group we are already in would put us back to
+            // "invited" and throw away joinedAtSecs. Kicked or destroyed deliberately fall through:
+            // a fresh invitation is how we get back in, and that does need the rebuild.
+            //
+            // The invitation still has to be answered, though. Our invite response is sent once and
+            // its failure is swallowed, and the admin marks us accepted only on receiving it, so an
+            // admin whose copy was lost sees us as invited forever and re-inviting is the only
+            // repair they have. Answering without rebuilding keeps both halves.
+            Log.d(TAG, "Already in this group - answering the invitation without rebuilding it")
+            acknowledgeInvitation(existing, inviteMessageHash)
+            return
+        }
+
         val address = Address.fromSerialized(groupId.hexString)
         val inviterRecipient = recipientRepository.getRecipient(Address.fromSerialized(inviter.hexString))
 

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

@@ -0,0 +1,198 @@
+package org.thoughtcrime.securesms.groups
+
+import com.google.common.truth.Truth.assertThat
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.test.UnconfinedTestDispatcher
+import kotlinx.coroutines.test.runTest
+import network.loki.messenger.libsession_util.MutableUserGroupsConfig
+import network.loki.messenger.libsession_util.PRIORITY_VISIBLE
+import network.loki.messenger.libsession_util.ReadableUserGroupsConfig
+import network.loki.messenger.libsession_util.util.Bytes
+import network.loki.messenger.libsession_util.util.GroupInfo
+import org.junit.Rule
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.mockito.kotlin.*
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.annotation.Config
+import network.loki.messenger.libsession_util.util.KeyPair
+import org.session.libsession.database.StorageProtocol
+import org.session.libsession.messaging.groups.GroupScope
+import org.session.libsession.messaging.sending_receiving.MessageSender
+import org.thoughtcrime.securesms.api.snode.DeleteMessageApi
+import org.thoughtcrime.securesms.api.swarm.SwarmApiExecutor
+import org.session.libsession.utilities.MutableUserConfigs
+import org.session.libsession.utilities.UserConfigs
+import org.session.libsession.messaging.messages.Destination
+import org.session.libsession.utilities.recipients.Recipient
+import org.session.libsignal.utilities.AccountId
+import org.thoughtcrime.securesms.database.RecipientRepository
+import org.thoughtcrime.securesms.dependencies.ConfigFactory
+import org.thoughtcrime.securesms.util.MockLoggingRule
+
+@RunWith(RobolectricTestRunner::class)
+@Config(minSdk = 36)
+class GroupManagerV2ImplTest {
+
+    @get:Rule
+    val logRule = MockLoggingRule()
+
+    private val groupId = AccountId("03" + "ab".repeat(32))
+    private val inviter = AccountId("05" + "cd".repeat(32))
+
+    private val joinedGroup = GroupInfo.ClosedGroupInfo(
+        groupAccountId = groupId.hexString,
+        adminKey = null,
+        authData = Bytes(ByteArray(100) { 7 }),
+        priority = PRIORITY_VISIBLE,
+        invited = false,
+        name = "Synthetic group",
+        destroyed = false,
+        joinedAtSecs = 1_700_000_000L,
+        kicked = false,
+    )
+
+    private val mutableUserGroups: MutableUserGroupsConfig = mock()
+
+    private lateinit var configFactory: ConfigFactory
+    private lateinit var messageSender: MessageSender
+    private lateinit var swarmApiExecutor: SwarmApiExecutor
+
+    /**
+     * [existingGroup] is what the user's own config already holds for this group, or null when the
+     * invitation is genuinely the first one.
+     */
+    private fun manager(
+        scope: CoroutineScope,
+        existingGroup: GroupInfo.ClosedGroupInfo?,
+    ): GroupManagerV2Impl {
+        val readableUserGroups: ReadableUserGroupsConfig = mock {
+            on { getClosedGroup(groupId.hexString) } doReturn existingGroup
+        }
+
+        val userConfigs: UserConfigs = mock { on { userGroups } doReturn readableUserGroups }
+        val mutableUserConfigs: MutableUserConfigs = mock { on { userGroups } doReturn mutableUserGroups }
+
+        configFactory = mock {
+            on { dangerouslyAccessUserConfigs() } doReturn (userConfigs to {})
+            on { dangerouslyAccessMutableUserConfigs() } doReturn (mutableUserConfigs to {})
+        }
+
+        val notApproved: Recipient = mock { on { approved } doReturn false }
+        val recipientRepository: RecipientRepository = mock {
+            onBlocking { getRecipient(any()) } doReturn notApproved
+        }
+
+        messageSender = mock()
+        swarmApiExecutor = mock {
+            onBlocking { send(any(), any()) } doReturn mock<DeleteMessageApi.SuccessResponse>()
+        }
+
+        // userAuth is an extension property, so it is satisfied through what it reads rather than
+        // stubbed: without it the invitation acknowledgement cannot delete the invite message.
+        val storage: StorageProtocol = mock {
+            on { getUserPublicKey() } doReturn "05" + "ef".repeat(32)
+            on { getUserED25519KeyPair() } doReturn KeyPair(
+                Bytes(ByteArray(32) { 1 }),
+                Bytes(ByteArray(64) { 2 }),
+            )
+        }
+
+        return GroupManagerV2Impl(
+            storage = storage,
+            configFactory = configFactory,
+            mmsSmsDatabase = mock(),
+            lokiDatabase = mock(),
+            application = mock(),
+            clock = mock(),
+            messageDataProvider = mock(),
+            lokiAPIDatabase = mock(),
+            receivedMessageHashDatabase = mock(),
+            configUploader = mock(),
+            scope = GroupScope(scope),
+            groupPollerManager = mock(),
+            recipientRepository = recipientRepository,
+            messageSender = messageSender,
+            inviteContactJobFactory = mock(),
+            swarmApiExecutor = swarmApiExecutor,
+            deleteMessageApiFactory = mock { on { create(any(), any()) } doReturn mock() },
+            storeSnodeMessageApiFactory = mock(),
+            unrevokeSubKeyApiFactory = mock(),
+            batchApiFactory = mock(),
+            jobQueue = mock(),
+        )
+    }
+
+    private suspend fun GroupManagerV2Impl.deliverInvitation() {
+        handleInvitation(
+            groupId = groupId,
+            groupName = "Synthetic group",
+            authData = ByteArray(100) { 9 },
+            inviter = inviter,
+            inviterName = "Inviter",
+            inviteMessageHash = "synthetic-hash",
+            inviteMessageTimestamp = 1_700_000_500_000L,
+        )
+    }
+
+    @Test
+    fun `a second invitation leaves a joined group untouched`() = runTest {
+        val manager = manager(
+            scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler)),
+            existingGroup = joinedGroup,
+        )
+
+        manager.deliverInvitation()
+
+        verify(configFactory, never()).dangerouslyAccessMutableUserConfigs()
+        verify(mutableUserGroups, never()).set(any())
+    }
+
+    @Test
+    fun `a first invitation is written to the user's groups`() = runTest {
+        val manager = manager(
+            scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler)),
+            existingGroup = null,
+        )
+
+        manager.deliverInvitation()
+
+        val written = argumentCaptor<GroupInfo>()
+        verify(mutableUserGroups).set(written.capture())
+        val group = written.firstValue as GroupInfo.ClosedGroupInfo
+        assertThat(group.groupAccountId).isEqualTo(groupId.hexString)
+        assertThat(group.invited).isTrue()
+    }
+
+    @Test
+    fun `a second invitation is still answered, so the admin can stop showing us as invited`() = runTest {
+        val manager = manager(
+            scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler)),
+            existingGroup = joinedGroup,
+        )
+
+        manager.deliverInvitation()
+
+        // Our membership is untouched...
+        verify(configFactory, never()).dangerouslyAccessMutableUserConfigs()
+        // ...and the invite response goes out anyway: it is sent once on approval with its failure
+        // swallowed, so a re-invite is the admin's only way to recover a lost one.
+        verify(messageSender).sendNonDurably(any(), any<Destination.ClosedGroup>(), eq(false))
+        // The invitation is dropped from our swarm, as approving one does.
+        verify(swarmApiExecutor).send(any(), any())
+    }
+
+    @Test
+    fun `an admin member answers a second invitation without sending a response`() = runTest {
+        val manager = manager(
+            scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler)),
+            existingGroup = joinedGroup.copy(adminKey = Bytes(ByteArray(64) { 3 })),
+        )
+
+        manager.deliverInvitation()
+
+        // An admin's membership is not established by an invite response - they write it themselves.
+        verify(messageSender, never()).sendNonDurably(any(), any<Destination>(), any())
+        verify(swarmApiExecutor).send(any(), any())
+    }
+}