diff --git a/CHANGELOG.md b/CHANGELOG.md index a06f06061d8884a31eb583e1a3ab56db736dc888..b8feec0362f200c7da5132e24798721a46e76e81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Use json for /messages filter instead of filter id +- Fix Verification getting stuck ### Security diff --git a/trixnity-client/src/commonMain/kotlin/net/folivo/trixnity/client/verification/ActiveSasVerificationMethod.kt b/trixnity-client/src/commonMain/kotlin/net/folivo/trixnity/client/verification/ActiveSasVerificationMethod.kt index 1cd4da4997e0d61a2e378ff0ad1a251527c40f83..1799fe60716509ccf240d41872ec906f5ddbe5a0 100644 --- a/trixnity-client/src/commonMain/kotlin/net/folivo/trixnity/client/verification/ActiveSasVerificationMethod.kt +++ b/trixnity-client/src/commonMain/kotlin/net/folivo/trixnity/client/verification/ActiveSasVerificationMethod.kt @@ -58,6 +58,7 @@ class ActiveSasVerificationMethod private constructor( private var theirCommitment: String? = null private var theirPublicKey: String? = null private var theirMac: SasMacEventContent? = null + private var ourMac: SasMacEventContent? = null private var messageAuthenticationCode: SasMessageAuthenticationCode? = null companion object { @@ -306,40 +307,42 @@ class ActiveSasVerificationMethod private constructor( } private suspend fun onMac(stepContent: SasMacEventContent, isOurOwn: Boolean) { - log.trace { "onMac $stepContent (isOurOwn=$isOurOwn)" } + log.trace { "onMac $stepContent (isOurOwn=$isOurOwn, hasOwnMac=${ourMac != null}, hasTheirMac = ${theirMac != null})" } - if (!isOurOwn) theirMac = stepContent + if (isOurOwn) { + ourMac = stepContent + } else { + theirMac = stepContent + } + val ourMac = ourMac val theirMac = theirMac - when { - theirMac == null && state.value is ComparisonByUser -> _state.value = WaitForMacs - theirMac != null && (state.value == WaitForMacs || isOurOwn) -> { - when (messageAuthenticationCode) { - HkdfHmacSha256 -> { - log.trace { "checkHkdfHmacSha256Mac with old (wrong) base64" } + if (ourMac != null && theirMac != null) { + when (messageAuthenticationCode) { + HkdfHmacSha256 -> { + log.trace { "checkHkdfHmacSha256Mac with old (wrong) base64" } checkHkdfHmacSha256Mac(theirMac, olmSas::calculateMac) - } + } - HkdfHmacSha256V2 -> { - log.trace { "checkHkdfHmacSha256Mac with fixed base64" } + HkdfHmacSha256V2 -> { + log.trace { "checkHkdfHmacSha256Mac with fixed base64" } checkHkdfHmacSha256Mac(theirMac, olmSas::calculateMacFixedBase64) - } + } - else -> { - log.warn { "messageAuthenticationCode is not set" } - sendVerificationStep( - VerificationCancelEventContent( - UnexpectedMessage, - "messageAuthenticationCode is not set", - relatesTo, - transactionId - ) + else -> { + log.warn { "messageAuthenticationCode is not set" } + sendVerificationStep( + VerificationCancelEventContent( + UnexpectedMessage, + "messageAuthenticationCode is not set", + relatesTo, + transactionId ) - } + ) } } - - else -> {} + } else if (isOurOwn) { + _state.value = WaitForMacs } } diff --git a/trixnity-client/src/commonMain/kotlin/net/folivo/trixnity/client/verification/ActiveVerification.kt b/trixnity-client/src/commonMain/kotlin/net/folivo/trixnity/client/verification/ActiveVerification.kt index 1df4d8acb9328e7585fedda8f3020e09f4459ba3..7397f619c356f9ded2f61026ede4bcba9896a1cf 100644 --- a/trixnity-client/src/commonMain/kotlin/net/folivo/trixnity/client/verification/ActiveVerification.kt +++ b/trixnity-client/src/commonMain/kotlin/net/folivo/trixnity/client/verification/ActiveVerification.kt @@ -58,7 +58,7 @@ abstract class ActiveVerificationImpl( MutableStateFlow( if (requestIsFromOurOwn) OwnRequest(request) else TheirRequest( - request, ownDeviceId, supportedMethods, relatesTo, transactionId, ::sendVerificationStepAndHandleIt + request, ownDeviceId, supportedMethods, relatesTo, transactionId, ::queueStep ) ) override val state = mutableState.asStateFlow() @@ -80,7 +80,7 @@ abstract class ActiveVerificationImpl( private val handleVerificationStepMutex = Mutex() - protected suspend fun handleVerificationStep(step: VerificationStep, sender: UserId, isOurOwn: Boolean) { + protected open suspend fun handleVerificationStep(step: VerificationStep, sender: UserId, isOurOwn: Boolean) { handleVerificationStepMutex.withReentrantLock { try { if (sender != theirUserId && sender != ownUserId) @@ -149,7 +149,7 @@ abstract class ActiveVerificationImpl( step.methods.intersect(supportedMethods), relatesTo, transactionId, - ::sendVerificationStepAndHandleIt + ::queueStep ) } @@ -170,7 +170,7 @@ abstract class ActiveVerificationImpl( ?: throw IllegalArgumentException("their device id should never be null at this step"), relatesTo = relatesTo, transactionId = transactionId, - sendVerificationStep = ::sendVerificationStepAndHandleIt, + sendVerificationStep = ::queueStep, keyStore = keyStore, keyTrustService = keyTrustService, json = json, @@ -222,6 +222,32 @@ abstract class ActiveVerificationImpl( protected abstract suspend fun sendVerificationStep(step: VerificationStep) + private val sendQueue = ArrayDeque() + private val queueMutex = Mutex() + private val sendMutex = Mutex() + + protected suspend fun queueStep(step: VerificationStep, clearQueue: Boolean = false) { + log.trace { "queuing verification step for send: $step" } + queueMutex.withReentrantLock { + if (clearQueue) { + sendQueue.clear() + } + sendQueue.addLast(step) + } + if (sendMutex.tryLock()) { + var step = queueMutex.withReentrantLock { + sendQueue.removeFirstOrNull() + } + while (step != null) { + sendVerificationStepAndHandleIt(step) + step = queueMutex.withReentrantLock { + sendQueue.removeFirstOrNull() + } + } + sendMutex.unlock() + } + } + private suspend fun sendVerificationStepAndHandleIt(step: VerificationStep) { log.trace { "send verification step and handle it: $step" } when (step) { @@ -257,7 +283,7 @@ abstract class ActiveVerificationImpl( } protected suspend fun cancel(code: Code, reason: String) { - sendVerificationStepAndHandleIt(VerificationCancelEventContent(code, reason, relatesTo, transactionId)) + queueStep(VerificationCancelEventContent(code, reason, relatesTo, transactionId), clearQueue = true) } override suspend fun cancel(message: String) { diff --git a/trixnity-client/src/commonTest/kotlin/net/folivo/trixnity/client/verification/ActiveVerificationOrderTest.kt b/trixnity-client/src/commonTest/kotlin/net/folivo/trixnity/client/verification/ActiveVerificationOrderTest.kt new file mode 100644 index 0000000000000000000000000000000000000000..afebd16fce3cae9692f25ccf1f46a29dd2d64e57 --- /dev/null +++ b/trixnity-client/src/commonTest/kotlin/net/folivo/trixnity/client/verification/ActiveVerificationOrderTest.kt @@ -0,0 +1,102 @@ +package net.folivo.trixnity.client.verification + +import net.folivo.trixnity.client.getInMemoryKeyStore +import net.folivo.trixnity.client.mocks.KeyTrustServiceMock +import net.folivo.trixnity.client.store.KeyStore +import net.folivo.trixnity.core.model.UserId +import net.folivo.trixnity.core.model.events.m.key.verification.SasMacEventContent +import net.folivo.trixnity.core.model.events.m.key.verification.VerificationDoneEventContent +import net.folivo.trixnity.core.model.events.m.key.verification.VerificationMethod.Sas +import net.folivo.trixnity.core.model.events.m.key.verification.VerificationRequestToDeviceEventContent +import net.folivo.trixnity.core.model.events.m.key.verification.VerificationStep +import net.folivo.trixnity.core.model.keys.Keys +import net.folivo.trixnity.core.serialization.createMatrixEventJson +import net.folivo.trixnity.test.utils.TrixnityBaseTest +import net.folivo.trixnity.test.utils.runTest +import kotlin.test.Test +import kotlin.test.assertEquals + +class ActiveVerificationOrderTest : TrixnityBaseTest() { + + private val alice = UserId("alice", "server") + private val aliceDevice = "AAAAAA" + private val bob = UserId("bob", "server") + private val bobDevice = "BBBBBB" + + private val sentEvents = mutableListOf() + private val handledEvents = mutableListOf() + + private val keyStore = getInMemoryKeyStore() + private val cut = TestActiveVerification( + VerificationRequestToDeviceEventContent( + bobDevice, + setOf(Sas), + 1234, + "t" + ), + keyStore + ) + + @Test + fun `events are handled in order`() = runTest { + val macEvent = SasMacEventContent("k", Keys(), cut.relatesTo, cut.transactionId) + cut.sendAndHandleVerificationStep(macEvent) + assertEquals( + expected = listOf(macEvent, VerificationDoneEventContent(cut.relatesTo, cut.transactionId)), + actual = handledEvents, + ) + } + + @Test + fun `events are sent in order`() = runTest { + val macEvent = SasMacEventContent("k", Keys(), cut.relatesTo, cut.transactionId) + cut.sendAndHandleVerificationStep(macEvent) + assertEquals( + expected = listOf(macEvent, VerificationDoneEventContent(cut.relatesTo, cut.transactionId)), + actual = sentEvents, + ) + } + + @Test + fun `events are sent and handled in the same order`() = runTest { + val macEvent = SasMacEventContent("k", Keys(), cut.relatesTo, cut.transactionId) + cut.sendAndHandleVerificationStep(macEvent) + assertEquals(handledEvents, sentEvents) + } + + private inner class TestActiveVerification(request: VerificationRequestToDeviceEventContent, keyStore: KeyStore) : + ActiveVerificationImpl( + request = request, + requestIsFromOurOwn = request.fromDevice == aliceDevice, + ownUserId = alice, + ownDeviceId = aliceDevice, + theirUserId = bob, + theirInitialDeviceId = null, + timestamp = 1234, + setOf(Sas), + null, + "t", + keyStore, + KeyTrustServiceMock(), + createMatrixEventJson(), + ) { + override suspend fun lifecycle() = Unit + + suspend fun sendAndHandleVerificationStep(step: VerificationStep) { + this.queueStep(step) + } + + override suspend fun sendVerificationStep(step: VerificationStep) { + sentEvents.add(step) + } + + override suspend fun handleVerificationStep(step: VerificationStep, sender: UserId, isOurOwn: Boolean) { + handledEvents.add(step) + when (step) { + is SasMacEventContent -> { + sendAndHandleVerificationStep(VerificationDoneEventContent(relatesTo, transactionId)) + } + } + } + } +} \ No newline at end of file