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..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 @@ -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,7 +87,7 @@ class KeyShareTests : InstrumentedTest { var outGoingRequestId: String? = null - retryPeriodicallyWithLatch(waitLatch) { + mTestHelper.retryPeriodicallyWithLatch(waitLatch) { aliceSession2.cryptoService().getOutgoingRoomKeyRequest() .filter { req -> // filter out request that was known before @@ -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..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,9 +455,31 @@ 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 - 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? {