Merge pull request #1211 from vector-im/feature/fix_gossiping_to_early

Fix / Send gossip request on other done received

CHANGES.md

@@ -19,6 +19,7 @@ Bugfix 🐛:
  - RiotX can't restore cross signing keys saved by web in SSSS (#1174)
  - Cross- Signing | After signin in new session, verification paper trail in DM is off (#1191)
  - Failed to encrypt message in room (message stays in red), [thanks to pwr22] (#925)
+ - Cross-Signing | web <-> riotX After QR code scan, gossiping fails (#1210)
 
 Translations 🗣:
  -

matrix-sdk-android/src/androidTest/java/im/vector/matrix/android/internal/crypto/gossiping/KeyShareTests.kt

@@ -282,7 +282,7 @@ class KeyShareTests : InstrumentedTest {
             val keysBackupService = aliceSession2.cryptoService().keysBackupService()
             mTestHelper.retryPeriodicallyWithLatch(latch) {
                 Log.d("#TEST", "Recovery :${ keysBackupService.getKeyBackupRecoveryKeyInfo()?.recoveryKey}")
-                keysBackupService.getKeyBackupRecoveryKeyInfo()?.recoveryKey != creationInfo.recoveryKey
+                keysBackupService.getKeyBackupRecoveryKeyInfo()?.recoveryKey == creationInfo.recoveryKey
             }
         }
     }

matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt

@@ -22,6 +22,9 @@ import dagger.Lazy
 import im.vector.matrix.android.api.MatrixCallback
 import im.vector.matrix.android.api.session.crypto.CryptoService
 import im.vector.matrix.android.api.session.crypto.crosssigning.CrossSigningService
+import im.vector.matrix.android.api.session.crypto.crosssigning.KEYBACKUP_SECRET_SSSS_NAME
+import im.vector.matrix.android.api.session.crypto.crosssigning.SELF_SIGNING_KEY_SSSS_NAME
+import im.vector.matrix.android.api.session.crypto.crosssigning.USER_SIGNING_KEY_SSSS_NAME
 import im.vector.matrix.android.api.session.crypto.verification.CancelCode
 import im.vector.matrix.android.api.session.crypto.verification.PendingVerificationRequest
 import im.vector.matrix.android.api.session.crypto.verification.QrCodeVerificationTransaction
@@ -60,6 +63,7 @@ import im.vector.matrix.android.internal.crypto.model.MXUsersDevicesMap
 import im.vector.matrix.android.internal.crypto.model.event.EncryptedEventContent
 import im.vector.matrix.android.internal.crypto.model.rest.KeyVerificationAccept
 import im.vector.matrix.android.internal.crypto.model.rest.KeyVerificationCancel
+import im.vector.matrix.android.internal.crypto.model.rest.KeyVerificationDone
 import im.vector.matrix.android.internal.crypto.model.rest.KeyVerificationKey
 import im.vector.matrix.android.internal.crypto.model.rest.KeyVerificationMac
 import im.vector.matrix.android.internal.crypto.model.rest.KeyVerificationReady
@@ -109,6 +113,10 @@ internal class DefaultVerificationService @Inject constructor(
     // map [sender : [transaction]]
     private val txMap = HashMap<String, HashMap<String, DefaultVerificationTransaction>>()
 
+    // we need to keep track of finished transaction
+    // It will be used for gossiping (to send request after request is completed and 'done' by other)
+    private val pastTransactions = HashMap<String, HashMap<String, DefaultVerificationTransaction>>()
+
     /**
      * Map [sender: [PendingVerificationRequest]]
      * For now we keep all requests (even terminated ones) during the lifetime of the app.
@@ -137,6 +145,9 @@ internal class DefaultVerificationService @Inject constructor(
                 EventType.KEY_VERIFICATION_READY         -> {
                     onReadyReceived(event)
                 }
+                EventType.KEY_VERIFICATION_DONE          -> {
+                    onDoneReceived(event)
+                }
                 MessageType.MSGTYPE_VERIFICATION_REQUEST -> {
                     onRequestReceived(event)
                 }
@@ -778,6 +789,31 @@ internal class DefaultVerificationService @Inject constructor(
         }
     }
 
+    private fun onDoneReceived(event: Event) {
+        Timber.v("## onDoneReceived")
+        val doneReq = event.getClearContent().toModel<KeyVerificationDone>()?.asValidObject()
+        if (doneReq == null || event.senderId != userId) {
+            // ignore
+            Timber.e("## SAS Received invalid done request")
+            return
+        }
+
+        // We only send gossiping request when the other sent us a done
+        // We can ask without checking too much thinks (like trust), because we will check validity of secret on reception
+        getExistingTransaction(userId, doneReq.transactionId)
+                ?: getOldTransaction(userId, doneReq.transactionId)
+                        ?.let { vt ->
+                            val otherDeviceId = vt.otherDeviceId
+                            if (!crossSigningService.canCrossSign()) {
+                                outgoingGossipingRequestManager.sendSecretShareRequest(SELF_SIGNING_KEY_SSSS_NAME, mapOf(userId to listOf(otherDeviceId
+                                        ?: "*")))
+                                outgoingGossipingRequestManager.sendSecretShareRequest(USER_SIGNING_KEY_SSSS_NAME, mapOf(userId to listOf(otherDeviceId
+                                        ?: "*")))
+                            }
+                            outgoingGossipingRequestManager.sendSecretShareRequest(KEYBACKUP_SECRET_SSSS_NAME, mapOf(userId to listOf(otherDeviceId ?: "*")))
+                        }
+    }
+
     private fun onRoomDoneReceived(event: Event) {
         val doneReq = event.getClearContent().toModel<MessageVerificationDoneContent>()
                 ?.copy(
@@ -1003,7 +1039,11 @@ internal class DefaultVerificationService @Inject constructor(
 
     private fun removeTransaction(otherUser: String, tid: String) {
         synchronized(txMap) {
-            txMap[otherUser]?.remove(tid)?.removeListener(this)
+            txMap[otherUser]?.remove(tid)?.also {
+                it.removeListener(this)
+            }
+        }?.let {
+            rememberOldTransaction(it)
         }
     }
 
@@ -1016,6 +1056,20 @@ internal class DefaultVerificationService @Inject constructor(
         }
     }
 
+    private fun rememberOldTransaction(tx: DefaultVerificationTransaction) {
+        synchronized(pastTransactions) {
+            pastTransactions.getOrPut(tx.otherUserId) { HashMap() }[tx.transactionId] = tx
+        }
+    }
+
+    private fun getOldTransaction(userId: String, tid: String?): DefaultVerificationTransaction? {
+        return tid?.let {
+            synchronized(pastTransactions) {
+                pastTransactions[userId]?.get(it)
+            }
+        }
+    }
+
     override fun beginKeyVerification(method: VerificationMethod, otherUserId: String, otherDeviceId: String, transactionId: String?): String? {
         val txID = transactionId?.takeIf { it.isNotEmpty() } ?: createUniqueIDForTransaction(otherUserId, otherDeviceId)
         // should check if already one (and cancel it)

matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationTransaction.kt

@@ -17,9 +17,6 @@ package im.vector.matrix.android.internal.crypto.verification
 
 import im.vector.matrix.android.api.MatrixCallback
 import im.vector.matrix.android.api.session.crypto.crosssigning.CrossSigningService
-import im.vector.matrix.android.api.session.crypto.crosssigning.KEYBACKUP_SECRET_SSSS_NAME
-import im.vector.matrix.android.api.session.crypto.crosssigning.SELF_SIGNING_KEY_SSSS_NAME
-import im.vector.matrix.android.api.session.crypto.crosssigning.USER_SIGNING_KEY_SSSS_NAME
 import im.vector.matrix.android.api.session.crypto.verification.VerificationTransaction
 import im.vector.matrix.android.api.session.crypto.verification.VerificationTxState
 import im.vector.matrix.android.internal.crypto.IncomingGossipingRequestManager
@@ -100,15 +97,15 @@ internal abstract class DefaultVerificationTransaction(
             })
         }
 
-        transport.done(transactionId) {
-            if (otherUserId == userId && !crossSigningService.canCrossSign()) {
-                outgoingGossipingRequestManager.sendSecretShareRequest(SELF_SIGNING_KEY_SSSS_NAME, mapOf(userId to listOf(otherDeviceId ?: "*")))
-                outgoingGossipingRequestManager.sendSecretShareRequest(USER_SIGNING_KEY_SSSS_NAME, mapOf(userId to listOf(otherDeviceId ?: "*")))
-                outgoingGossipingRequestManager.sendSecretShareRequest(KEYBACKUP_SECRET_SSSS_NAME, mapOf(userId to listOf(otherDeviceId ?: "*")))
-            }
-        }
-
         state = VerificationTxState.Verified
+
+        transport.done(transactionId) {
+//            if (otherUserId == userId && !crossSigningService.canCrossSign()) {
+//                outgoingGossipingRequestManager.sendSecretShareRequest(SELF_SIGNING_KEY_SSSS_NAME, mapOf(userId to listOf(otherDeviceId ?: "*")))
+//                outgoingGossipingRequestManager.sendSecretShareRequest(USER_SIGNING_KEY_SSSS_NAME, mapOf(userId to listOf(otherDeviceId ?: "*")))
+//                outgoingGossipingRequestManager.sendSecretShareRequest(KEYBACKUP_SECRET_SSSS_NAME, mapOf(userId to listOf(otherDeviceId ?: "*")))
+//            }
+        }
     }
 
     private fun setDeviceVerified(userId: String, deviceId: String) {

matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/VerificationInfoDone.kt

@@ -18,11 +18,9 @@ package im.vector.matrix.android.internal.crypto.verification
 internal interface VerificationInfoDone : VerificationInfo<ValidVerificationInfoDone> {
 
     override fun asValidObject(): ValidVerificationInfoDone? {
-        if (transactionId.isNullOrEmpty()) {
-            return null
-        }
-        return ValidVerificationInfoDone
+        val validTransactionId = transactionId?.takeIf { it.isNotEmpty() } ?: return null
+        return ValidVerificationInfoDone(validTransactionId)
     }
 }
 
-internal object ValidVerificationInfoDone
+internal data class ValidVerificationInfoDone(val transactionId: String)