From 286a5081ff792da7efc482bb94a703de118e5ef2 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 18 Mar 2020 10:07:57 +0100 Subject: [PATCH 1/2] Verif / handle concurrent start Fixes #794 --- CHANGES.md | 2 +- .../matrix/android/common/CommonTestHelper.kt | 19 ++++ .../matrix/android/common/TestConstants.kt | 4 +- .../crypto/gossiping/KeyShareTests.kt | 37 ++----- .../internal/crypto/verification/SASTest.kt | 97 +++++++++++++++++++ .../DefaultVerificationService.kt | 34 ++++++- 6 files changed, 160 insertions(+), 33 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b07a6b567f..180598a980 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,7 +5,7 @@ Features ✨: - Cross-Signing | Support SSSS secret sharing (#944) Improvements 🙌: - - + - Verification DM / Handle concurrent .start after .ready (#794) Bugfix 🐛: - diff --git a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/common/CommonTestHelper.kt b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/common/CommonTestHelper.kt index 386787b882..08c81f56c0 100644 --- a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/common/CommonTestHelper.kt +++ b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/common/CommonTestHelper.kt @@ -38,6 +38,7 @@ import im.vector.matrix.android.api.session.room.timeline.TimelineSettings import im.vector.matrix.android.api.session.sync.SyncState import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals @@ -269,6 +270,24 @@ class CommonTestHelper(context: Context) { assertTrue(latch.await(TestConstants.timeOutMillis, TimeUnit.MILLISECONDS)) } + fun retryPeriodicallyWithLatch(latch: CountDownLatch, condition: (() -> Boolean)) { + GlobalScope.launch { + while (true) { + delay(1000) + if (condition()) { + latch.countDown() + return@launch + } + } + } + } + + fun waitWithLatch(block: (CountDownLatch) -> Unit) { + val latch = CountDownLatch(1) + block(latch) + await(latch) + } + // Transform a method with a MatrixCallback to a synchronous method inline fun doSync(block: (MatrixCallback) -> Unit): T { val lock = CountDownLatch(1) diff --git a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/common/TestConstants.kt b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/common/TestConstants.kt index 2346898ca7..deaa721e40 100644 --- a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/common/TestConstants.kt +++ b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/common/TestConstants.kt @@ -22,8 +22,8 @@ object TestConstants { const val TESTS_HOME_SERVER_URL = "http://10.0.2.2:8080" - // Time out to use when waiting for server response. 10s - private const val AWAIT_TIME_OUT_MILLIS = 10_000 + // Time out to use when waiting for server response. 20s + private const val AWAIT_TIME_OUT_MILLIS = 20_000 // Time out to use when waiting for server response, when the debugger is connected. 10 minutes private const val AWAIT_TIME_OUT_WITH_DEBUGGER_MILLIS = 10 * 60_000 diff --git a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt index 1dc0907b65..c42e94e340 100644 --- a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt +++ b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt @@ -32,9 +32,6 @@ import im.vector.matrix.android.internal.crypto.model.event.EncryptedEventConten import junit.framework.TestCase.assertNotNull import junit.framework.TestCase.assertTrue import junit.framework.TestCase.fail -import kotlinx.coroutines.GlobalScope -import kotlinx.coroutines.delay -import kotlinx.coroutines.launch import org.junit.Assert import org.junit.FixMethodOrder import org.junit.Test @@ -90,10 +87,10 @@ class KeyShareTests : InstrumentedTest { var outGoingRequestId: String? = null - retryPeriodicallyWithLatch(waitLatch) { + mTestHelper.retryPeriodicallyWithLatch(waitLatch) { aliceSession2.cryptoService().getOutgoingRoomKeyRequest() .filter { req -> - // filter out request that was known before + // filter out request thwat was known before !outgoingRequestBefore.any { req.requestId == it.requestId } } .let { @@ -114,8 +111,8 @@ class KeyShareTests : InstrumentedTest { // The first session should see an incoming request // the request should be refused, because the device is not trusted - waitWithLatch { latch -> - retryPeriodicallyWithLatch(latch) { + mTestHelper.waitWithLatch { latch -> + mTestHelper.retryPeriodicallyWithLatch(latch) { // DEBUG LOGS aliceSession.cryptoService().getIncomingRoomKeyRequest().let { Log.v("TEST", "Incoming request Session 1 (looking for $outGoingRequestId)") @@ -144,8 +141,8 @@ class KeyShareTests : InstrumentedTest { // Re request aliceSession2.cryptoService().reRequestRoomKeyForEvent(receivedEvent.root) - waitWithLatch { latch -> - retryPeriodicallyWithLatch(latch) { + mTestHelper.waitWithLatch { latch -> + mTestHelper.retryPeriodicallyWithLatch(latch) { aliceSession.cryptoService().getIncomingRoomKeyRequest().let { Log.v("TEST", "Incoming request Session 1") Log.v("TEST", "=========================") @@ -160,8 +157,8 @@ class KeyShareTests : InstrumentedTest { } Thread.sleep(6_000) - waitWithLatch { latch -> - retryPeriodicallyWithLatch(latch) { + mTestHelper.waitWithLatch { latch -> + mTestHelper.retryPeriodicallyWithLatch(latch) { aliceSession2.cryptoService().getOutgoingRoomKeyRequest().let { it.any { it.requestBody?.sessionId == eventMegolmSessionId && it.state == OutgoingGossipingRequestState.CANCELLED } } @@ -177,22 +174,4 @@ class KeyShareTests : InstrumentedTest { mTestHelper.signOutAndClose(aliceSession) mTestHelper.signOutAndClose(aliceSession2) } - - fun retryPeriodicallyWithLatch(latch: CountDownLatch, condition: (() -> Boolean)) { - GlobalScope.launch { - while (true) { - delay(1000) - if (condition()) { - latch.countDown() - return@launch - } - } - } - } - - fun waitWithLatch(block: (CountDownLatch) -> Unit) { - val latch = CountDownLatch(1) - block(latch) - mTestHelper.await(latch) - } } diff --git a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/verification/SASTest.kt b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/verification/SASTest.kt index eefb040987..1ac70d7f2b 100644 --- a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/verification/SASTest.kt +++ b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/verification/SASTest.kt @@ -16,6 +16,7 @@ package im.vector.matrix.android.internal.crypto.verification +import android.util.Log import androidx.test.ext.junit.runners.AndroidJUnit4 import im.vector.matrix.android.InstrumentedTest import im.vector.matrix.android.api.session.Session @@ -23,6 +24,7 @@ import im.vector.matrix.android.api.session.crypto.verification.CancelCode import im.vector.matrix.android.api.session.crypto.verification.IncomingSasVerificationTransaction import im.vector.matrix.android.api.session.crypto.verification.OutgoingSasVerificationTransaction import im.vector.matrix.android.api.session.crypto.verification.SasMode +import im.vector.matrix.android.api.session.crypto.verification.SasVerificationTransaction import im.vector.matrix.android.api.session.crypto.verification.VerificationMethod import im.vector.matrix.android.api.session.crypto.verification.VerificationService import im.vector.matrix.android.api.session.crypto.verification.VerificationTransaction @@ -355,6 +357,7 @@ class SASTest : InstrumentedTest { val aliceAcceptedLatch = CountDownLatch(1) val aliceListener = object : VerificationService.Listener { override fun transactionUpdated(tx: VerificationTransaction) { + Log.v("TEST", "== aliceTx state ${tx.state} => ${(tx as? OutgoingSasVerificationTransaction)?.uxState}") if ((tx as SASDefaultVerificationTransaction).state === VerificationTxState.OnAccepted) { val at = tx as SASDefaultVerificationTransaction accepted = at.accepted @@ -367,7 +370,9 @@ class SASTest : InstrumentedTest { val bobListener = object : VerificationService.Listener { override fun transactionUpdated(tx: VerificationTransaction) { + Log.v("TEST", "== bobTx state ${tx.state} => ${(tx as? IncomingSasVerificationTransaction)?.uxState}") if ((tx as IncomingSasVerificationTransaction).uxState === IncomingSasVerificationTransaction.UxState.SHOW_ACCEPT) { + bobVerificationService.removeListener(this) val at = tx as IncomingSasVerificationTransaction at.performAccept() } @@ -515,4 +520,96 @@ class SASTest : InstrumentedTest { assertTrue("bob device should be verified from alice point of view", bobDeviceInfoFromAlicePOV!!.isVerified) cryptoTestData.cleanUp(mTestHelper) } + + @Test + fun test_ConcurrentStart() { + val cryptoTestData = mCryptoTestHelper.doE2ETestWithAliceAndBobInARoom() + + val aliceSession = cryptoTestData.firstSession + val bobSession = cryptoTestData.secondSession + + val aliceVerificationService = aliceSession.cryptoService().verificationService() + val bobVerificationService = bobSession!!.cryptoService().verificationService() + + val req = aliceVerificationService.requestKeyVerificationInDMs( + listOf(VerificationMethod.SAS, VerificationMethod.QR_CODE_SCAN, VerificationMethod.QR_CODE_SHOW), + bobSession.myUserId, + cryptoTestData.roomId + ) + + var requestID : String? = null + + mTestHelper.waitWithLatch { + mTestHelper.retryPeriodicallyWithLatch(it) { + val prAlicePOV = aliceVerificationService.getExistingVerificationRequest(bobSession.myUserId)?.firstOrNull() + requestID = prAlicePOV?.transactionId + Log.v("TEST", "== alicePOV is $prAlicePOV") + prAlicePOV?.transactionId != null && prAlicePOV.localId == req.localId + } + } + + Log.v("TEST", "== requestID is $requestID") + + mTestHelper.waitWithLatch { + mTestHelper.retryPeriodicallyWithLatch(it) { + val prBobPOV = bobVerificationService.getExistingVerificationRequest(aliceSession.myUserId)?.firstOrNull() + Log.v("TEST", "== prBobPOV is $prBobPOV") + prBobPOV?.transactionId == requestID + } + } + + bobVerificationService.readyPendingVerification( + listOf(VerificationMethod.SAS, VerificationMethod.QR_CODE_SCAN, VerificationMethod.QR_CODE_SHOW), + aliceSession.myUserId, + requestID!! + ) + + // wait for alice to get the ready + mTestHelper.waitWithLatch { + mTestHelper.retryPeriodicallyWithLatch(it) { + val prAlicePOV = aliceVerificationService.getExistingVerificationRequest(bobSession.myUserId)?.firstOrNull() + Log.v("TEST", "== prAlicePOV is $prAlicePOV") + prAlicePOV?.transactionId == requestID && prAlicePOV?.isReady != null + } + } + + // Start concurrent! + aliceVerificationService.beginKeyVerificationInDMs( + VerificationMethod.SAS, + requestID!!, + cryptoTestData.roomId, + bobSession.myUserId, + bobSession.sessionParams.credentials.deviceId!!, + null) + + bobVerificationService.beginKeyVerificationInDMs( + VerificationMethod.SAS, + requestID!!, + cryptoTestData.roomId, + aliceSession.myUserId, + aliceSession.sessionParams.credentials.deviceId!!, + null) + + // we should reach SHOW SAS on both + var alicePovTx: SasVerificationTransaction? + var bobPovTx: SasVerificationTransaction? + + mTestHelper.waitWithLatch { + mTestHelper.retryPeriodicallyWithLatch(it) { + alicePovTx = aliceVerificationService.getExistingTransaction(bobSession.myUserId, requestID!!) as? SasVerificationTransaction + Log.v("TEST", "== alicePovTx is $alicePovTx") + alicePovTx?.state == VerificationTxState.ShortCodeReady + } + } + // wait for alice to get the ready + mTestHelper.waitWithLatch { + mTestHelper.retryPeriodicallyWithLatch(it) { + bobPovTx = bobVerificationService.getExistingTransaction(aliceSession.myUserId, requestID!!) as? SasVerificationTransaction + Log.v("TEST", "== bobPovTx is $bobPovTx") + bobPovTx?.state == VerificationTxState.ShortCodeReady + } + } + + cryptoTestData.cleanUp(mTestHelper) + } } diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt index f6364c2125..7276debfe2 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt @@ -457,7 +457,29 @@ internal class DefaultVerificationService @Inject constructor( Timber.d("## SAS onStartRequestReceived ${startReq.transactionId}") if (checkKeysAreDownloaded(otherUserId!!, startReq.fromDevice) != null) { val tid = startReq.transactionId - val existing = getExistingTransaction(otherUserId, tid) + var existing = getExistingTransaction(otherUserId, tid) + + // After the m.key.verification.ready event is sent, either party can send an + // m.key.verification.start event to begin the verification. If both parties + // send an m.key.verification.start event, and they both specify the same + // verification method, then the event sent by the user whose user ID is the + // smallest is used, and the other m.key.verification.start event is ignored. + // In the case of a single user verifying two of their devices, the device ID is + // compared instead . + if (existing != null && !existing.isIncoming) { + val readyRequest = getExistingVerificationRequest(otherUserId, tid) + if (readyRequest?.isReady == true) { + if (isOtherPrioritary(otherUserId, existing.otherDeviceId ?: "")) { + // The other is prioritary! + // I should replace my outgoing with an incoming + removeTransaction(otherUserId, tid) + existing = null + } else { + // i am prioritary, ignore this start event! + return null + } + } + } when (startReq) { is ValidVerificationInfoStart.SasVerificationInfoStart -> { @@ -532,6 +554,16 @@ internal class DefaultVerificationService @Inject constructor( } } + private fun isOtherPrioritary(otherUserId: String, otherDeviceId: String) : Boolean { + if (userId < otherUserId) { + return false + } else if (userId > otherUserId) { + return true + } else { + return otherDeviceId < deviceId ?: "" + } + } + // TODO Refacto: It could just return a boolean private suspend fun checkKeysAreDownloaded(otherUserId: String, otherDeviceId: String): MXUsersDevicesMap? { From 6fe77eba724ccad37e899365dc35cbee64874a5f Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 18 Mar 2020 11:25:49 +0100 Subject: [PATCH 2/2] code review --- .../crypto/gossiping/KeyShareTests.kt | 2 +- .../DefaultVerificationService.kt | 22 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt index c42e94e340..c8d2df38ce 100644 --- a/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt +++ b/matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt @@ -90,7 +90,7 @@ class KeyShareTests : InstrumentedTest { mTestHelper.retryPeriodicallyWithLatch(waitLatch) { aliceSession2.cryptoService().getOutgoingRoomKeyRequest() .filter { req -> - // filter out request thwat was known before + // filter out request that was known before !outgoingRequestBefore.any { req.requestId == it.requestId } } .let { diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt index 7276debfe2..400b2c520e 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt @@ -455,7 +455,7 @@ internal class DefaultVerificationService @Inject constructor( startReq: ValidVerificationInfoStart, txConfigure: (DefaultVerificationTransaction) -> Unit): CancelCode? { Timber.d("## SAS onStartRequestReceived ${startReq.transactionId}") - if (checkKeysAreDownloaded(otherUserId!!, startReq.fromDevice) != null) { + if (otherUserId?.let { checkKeysAreDownloaded(it, startReq.fromDevice) } != null) { val tid = startReq.transactionId var existing = getExistingTransaction(otherUserId, tid) @@ -469,15 +469,15 @@ internal class DefaultVerificationService @Inject constructor( if (existing != null && !existing.isIncoming) { val readyRequest = getExistingVerificationRequest(otherUserId, tid) if (readyRequest?.isReady == true) { - if (isOtherPrioritary(otherUserId, existing.otherDeviceId ?: "")) { - // The other is prioritary! - // I should replace my outgoing with an incoming - removeTransaction(otherUserId, tid) - existing = null - } else { - // i am prioritary, ignore this start event! - return null - } + if (isOtherPrioritary(otherUserId, existing.otherDeviceId ?: "")) { + // The other is prioritary! + // I should replace my outgoing with an incoming + removeTransaction(otherUserId, tid) + existing = null + } else { + // i am prioritary, ignore this start event! + return null + } } } @@ -554,7 +554,7 @@ internal class DefaultVerificationService @Inject constructor( } } - private fun isOtherPrioritary(otherUserId: String, otherDeviceId: String) : Boolean { + private fun isOtherPrioritary(otherUserId: String, otherDeviceId: String): Boolean { if (userId < otherUserId) { return false } else if (userId > otherUserId) {