Verif / handle concurrent start

Fixes #794
This commit is contained in:
Valere 2020-03-18 10:07:57 +01:00
parent 56c241c9bd
commit 286a5081ff
6 changed files with 160 additions and 33 deletions

View file

@ -5,7 +5,7 @@ Features ✨:
- Cross-Signing | Support SSSS secret sharing (#944)
Improvements 🙌:
-
- Verification DM / Handle concurrent .start after .ready (#794)
Bugfix 🐛:
-

View file

@ -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 <reified T> doSync(block: (MatrixCallback<T>) -> Unit): T {
val lock = CountDownLatch(1)

View file

@ -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

View file

@ -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)
}
}

View file

@ -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)
}
}

View file

@ -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<CryptoDeviceInfo>? {