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()) + } +}