QRCode: requestId is not supposed to be an eventId

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

@@ -786,25 +786,25 @@ internal class DefaultVerificationService @Inject constructor(
         ))
     }
 
-    private fun createQrCodeData(requestEventId: String?, otherUserId: String, otherDeviceId: String?): QrCodeData? {
-        requestEventId ?: run {
-            Timber.w("## Unknown requestEventId")
+    private fun createQrCodeData(requestId: String?, otherUserId: String, otherDeviceId: String?): QrCodeData? {
+        requestId ?: run {
+            Timber.w("## Unknown requestId")
             return null
         }
 
         return when {
             userId != otherUserId                        ->
-                createQrCodeDataForDistinctUser(requestEventId, otherUserId, otherDeviceId)
+                createQrCodeDataForDistinctUser(requestId, otherUserId, otherDeviceId)
             crossSigningService.isCrossSigningVerified() ->
                 // This is a self verification and I am the old device (Osborne2)
-                createQrCodeDataForVerifiedDevice(requestEventId, otherDeviceId)
+                createQrCodeDataForVerifiedDevice(requestId, otherDeviceId)
             else                                         ->
                 // This is a self verification and I am the new device (Dynabook)
-                createQrCodeDataForUnVerifiedDevice(requestEventId, otherDeviceId)
+                createQrCodeDataForUnVerifiedDevice(requestId, otherDeviceId)
         }
     }
 
-    private fun createQrCodeDataForDistinctUser(requestEventId: String, otherUserId: String, otherDeviceId: String?): QrCodeData? {
+    private fun createQrCodeDataForDistinctUser(requestId: String, otherUserId: String, otherDeviceId: String?): QrCodeData? {
         val myMasterKey = crossSigningService.getMyCrossSigningKeys()
                 ?.masterKey()
                 ?.unpaddedBase64PublicKey
@@ -840,7 +840,7 @@ internal class DefaultVerificationService @Inject constructor(
 
         return QrCodeData(
                 userId = userId,
-                requestEventId = requestEventId,
+                requestId = requestId,
                 action = QrCodeData.ACTION_VERIFY,
                 keys = hashMapOf(
                         myMasterKey to myMasterKey,
@@ -853,7 +853,7 @@ internal class DefaultVerificationService @Inject constructor(
     }
 
     // Create a QR code to display on the old device (Osborne2)
-    private fun createQrCodeDataForVerifiedDevice(requestEventId: String, otherDeviceId: String?): QrCodeData? {
+    private fun createQrCodeDataForVerifiedDevice(requestId: String, otherDeviceId: String?): QrCodeData? {
         val myMasterKey = crossSigningService.getMyCrossSigningKeys()
                 ?.masterKey()
                 ?.unpaddedBase64PublicKey
@@ -885,7 +885,7 @@ internal class DefaultVerificationService @Inject constructor(
 
         return QrCodeData(
                 userId = userId,
-                requestEventId = requestEventId,
+                requestId = requestId,
                 action = QrCodeData.ACTION_VERIFY,
                 keys = hashMapOf(
                         myMasterKey to myMasterKey,
@@ -898,7 +898,7 @@ internal class DefaultVerificationService @Inject constructor(
     }
 
     // Create a QR code to display on the new device (Dynabook)
-    private fun createQrCodeDataForUnVerifiedDevice(requestEventId: String, otherDeviceId: String?): QrCodeData? {
+    private fun createQrCodeDataForUnVerifiedDevice(requestId: String, otherDeviceId: String?): QrCodeData? {
         val myMasterKey = crossSigningService.getMyCrossSigningKeys()
                 ?.masterKey()
                 ?.unpaddedBase64PublicKey
@@ -926,7 +926,7 @@ internal class DefaultVerificationService @Inject constructor(
 
         return QrCodeData(
                 userId = userId,
-                requestEventId = requestEventId,
+                requestId = requestId,
                 action = QrCodeData.ACTION_VERIFY,
                 keys = hashMapOf(
                         // Note: no master key here

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

@@ -81,8 +81,8 @@ internal class DefaultQrCodeVerificationTransaction(
             return
         }
 
-        if (otherQrCodeData.requestEventId != transactionId) {
-            Timber.d("## Verification QR: Invalid transaction actual ${otherQrCodeData.requestEventId} expected:$transactionId")
+        if (otherQrCodeData.requestId != transactionId) {
+            Timber.d("## Verification QR: Invalid transaction actual ${otherQrCodeData.requestId} expected:$transactionId")
             cancel(CancelCode.QrCodeInvalid)
             return
         }

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

@@ -49,7 +49,7 @@ fun QrCodeData.toUrl(): String {
     return buildString {
         append(PermalinkFactory.createPermalink(userId))
         append("?request=")
-        append(URLEncoder.encode(requestEventId, ENCODING))
+        append(URLEncoder.encode(requestId, ENCODING))
         append("&action=")
         append(URLEncoder.encode(action, ENCODING))
 
@@ -105,10 +105,10 @@ fun String.toQrCodeData(): QrCodeData? {
         (it.substringBefore("=") to it.substringAfter("=").let { value -> URLDecoder.decode(value, ENCODING) })
     }.toMap()
 
-    val action = keyValues["action"] ?: return null
+    val action = keyValues["action"]?.takeIf { it.isNotBlank() } ?: return null
 
-    val requestEventId = keyValues["request"]?.takeIf { MatrixPatterns.isEventId(it) } ?: return null
-    val sharedSecret = keyValues["secret"] ?: return null
+    val requestEventId = keyValues["request"]?.takeIf { it.isNotBlank() } ?: return null
+    val sharedSecret = keyValues["secret"]?.takeIf { it.isNotBlank() } ?: return null
     val otherUserKey = keyValues["other_user_key"]
     val otherDeviceKey = keyValues["other_device_key"]
 

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

@@ -21,8 +21,8 @@ package im.vector.matrix.android.internal.crypto.verification.qrcode
  */
 data class QrCodeData(
         val userId: String,
-        // the event ID of the associated verification request event.
-        val requestEventId: String,
+        // Request Id. Can be an arbitrary value. In DM, it will be the event ID of the associated verification request event.
+        val requestId: String,
         // The action
         val action: String,
         // key_<key_id>: each key that the user wants verified will have an entry of this form, where the value is the key in unpadded base64.

matrix-sdk-android/src/test/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeTest.kt

@@ -30,7 +30,7 @@ class QrCodeTest {
 
     private val basicQrCodeData = QrCodeData(
             userId = "@benoit:matrix.org",
-            requestEventId = "\$azertyazerty",
+            requestId = "\$azertyazerty",
             action = QrCodeData.ACTION_VERIFY,
             keys = mapOf(
                     "1" to "abcdef",
@@ -61,7 +61,7 @@ class QrCodeTest {
         decodedData.shouldNotBeNull()
 
         decodedData.userId shouldBeEqualTo "@benoit:matrix.org"
-        decodedData.requestEventId shouldBeEqualTo "\$azertyazerty"
+        decodedData.requestId shouldBeEqualTo "\$azertyazerty"
         decodedData.keys["1"]?.shouldBeEqualTo("abcdef")
         decodedData.keys["2"]?.shouldBeEqualTo("ghijql")
         decodedData.sharedSecret shouldBeEqualTo "sharedSecret"
@@ -74,7 +74,7 @@ class QrCodeTest {
         val url = basicQrCodeData
                 .copy(
                         userId = "@benoit/foo:matrix.org",
-                        requestEventId = "\$azertyazerty/bar"
+                        requestId = "\$azertyazerty/bar"
                 )
                 .toUrl()
 
@@ -87,7 +87,7 @@ class QrCodeTest {
         decodedData.shouldNotBeNull()
 
         decodedData.userId shouldBeEqualTo "@benoit/foo:matrix.org"
-        decodedData.requestEventId shouldBeEqualTo "\$azertyazerty/bar"
+        decodedData.requestId shouldBeEqualTo "\$azertyazerty/bar"
         decodedData.keys["1"]?.shouldBeEqualTo("abcdef")
         decodedData.keys["2"]?.shouldBeEqualTo("ghijql")
         decodedData.sharedSecret shouldBeEqualTo "sharedSecret"
@@ -111,7 +111,7 @@ class QrCodeTest {
         decodedData.shouldNotBeNull()
 
         decodedData.userId shouldBeEqualTo "@benoit:matrix.org"
-        decodedData.requestEventId shouldBeEqualTo "\$azertyazerty"
+        decodedData.requestId shouldBeEqualTo "\$azertyazerty"
         decodedData.keys["1"]?.shouldBeEqualTo("abcdef")
         decodedData.keys["2"]?.shouldBeEqualTo("ghijql")
         decodedData.sharedSecret shouldBeEqualTo "sharedSecret"
@@ -135,7 +135,7 @@ class QrCodeTest {
         decodedData.shouldNotBeNull()
 
         decodedData.userId shouldBeEqualTo "@benoit:matrix.org"
-        decodedData.requestEventId shouldBeEqualTo "\$azertyazerty"
+        decodedData.requestId shouldBeEqualTo "\$azertyazerty"
         decodedData.keys["1"]?.shouldBeEqualTo("abcdef")
         decodedData.keys["2"]?.shouldBeEqualTo("ghijql")
         decodedData.sharedSecret shouldBeEqualTo "sharedSecret"
@@ -173,6 +173,13 @@ class QrCodeTest {
                 .shouldBeNull()
     }
 
+    @Test
+    fun testEmptyActionCase() {
+        basicUrl.replace("&action=verify", "&action=")
+                .toQrCodeData()
+                .shouldBeNull()
+    }
+
     @Test
     fun testOtherActionCase() {
         basicUrl.replace("&action=verify", "&action=confirm")
@@ -182,8 +189,15 @@ class QrCodeTest {
     }
 
     @Test
-    fun testBadRequestEventId() {
-        basicUrl.replace("%24azertyazerty", "%32azertyazerty")
+    fun testMissingRequestId() {
+        basicUrl.replace("request=%24azertyazerty", "")
+                .toQrCodeData()
+                .shouldBeNull()
+    }
+
+    @Test
+    fun testEmptyRequestId() {
+        basicUrl.replace("request=%24azertyazerty", "request=")
                 .toQrCodeData()
                 .shouldBeNull()
     }
@@ -208,4 +222,25 @@ class QrCodeTest {
                 .toQrCodeData()
                 .shouldBeNull()
     }
+
+    @Test
+    fun testEmptySecret() {
+        basicUrl.replace("&secret=sharedSecret", "&secret=")
+                .toQrCodeData()
+                .shouldBeNull()
+    }
+
+    @Test
+    fun testSelfSigning() {
+        // request is not an eventId in this case
+        val url = "https://matrix.to/#/@benoit0815:matrix.org" +
+                "?request=local.4dff40e1-7bf1-4e80-81ed-c6090d43bf20" +
+                "&action=verify" +
+                "&key_utbSRFcFjFDYf0KcNv3FoBHFSbvUPXtCYutuOg6WQ%2Bs=utbSRFcFjFDYf0KcNv3FoBHFSbvUPXtCYutuOg6WQ%2Bs" +
+                "&key_YSOXZVBXIZ=F0XWqgUePgwm5HMYG3yhBNneHmscrAxxlooLHjy8YQc" +
+                "&secret=LYVcEQmfdorbJ3vbQnq7nbNZc%2BGmDxUen1rByV9hRM4" +
+                "&other_device_key=eGoUqZqAroCYpjp7FLGIkTEzYHBFED4uUAfJ267gqQQ"
+
+        url.toQrCodeData()!!.requestId shouldBeEqualTo "local.4dff40e1-7bf1-4e80-81ed-c6090d43bf20"
+    }
 }