From bc6698e202bf025c474ca2b64e625e8b9a4b87b8 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 18 Sep 2026 17:37:28 +1000 Subject: [PATCH] Fix: a duplicate invite demoted a member who had already joined Issue #2215, reported with a reliable repro and against every Session Android version. handleInvitation built a fresh ClosedGroupInfo and wrote it over the existing record, so a second invitation for a group we were already in set invited back to true and discarded joinedAtSecs. It now checks the same thing handlePromotion already checks, and still rebuilds when we were kicked or the group was destroyed - a fresh invitation is how we get back in. A second invitation still has to be answered, though, and that is why this is not simply a guard. Our invite response is sent once, on approval, with its failure swallowed, and the admin marks us accepted only on receiving it - so an admin whose copy was lost has no repair except re-inviting, and the reported demotion is what that repair did on our side. The already-joined path now sends the response and drops the invitation from our swarm without touching membership, so both halves hold. Admin side of the same defect: inviteMembersInternal called setInvited() whatever state the member was in, and InviteContactsJob then wrote the send result over the top - so re-inviting someone who had accepted reset them twice, and fixing only the first place would have changed nothing. Both now leave anyone already in the group alone, using isAdminOrBeingPromoted so a promotion in flight counts as membership rather than as an invitation waiting on an answer. Tests cover the receiving side. The admin side has none: GroupMember's state setters are native, so no JVM unit test can reach them. --- .../messaging/jobs/InviteContactsJob.kt | 12 ++ .../securesms/groups/GroupManagerV2Impl.kt | 95 +++++++-- .../groups/GroupManagerV2ImplTest.kt | 198 ++++++++++++++++++ 3 files changed, 287 insertions(+), 18 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/groups/GroupManagerV2ImplTest.kt diff --git a/app/src/main/java/org/session/libsession/messaging/jobs/InviteContactsJob.kt b/app/src/main/java/org/session/libsession/messaging/jobs/InviteContactsJob.kt index 0b74ddb853..13b442f1a9 100644 --- a/app/src/main/java/org/session/libsession/messaging/jobs/InviteContactsJob.kt +++ b/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 { diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2Impl.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2Impl.kt index bfdca69681..6c1a715314 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2Impl.kt +++ b/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)) diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupManagerV2ImplTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupManagerV2ImplTest.kt new file mode 100644 index 0000000000..2cfca2fe18 --- /dev/null +++ b/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() + } + + // 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() + 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(), 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(), any()) + verify(swarmApiExecutor).send(any(), any()) + } +}