Merge pull request #919 from vector-im/qr_step_validate

Qr step validate

matrix-sdk-android/src/main/java/im/vector/matrix/android/api/session/crypto/crosssigning/CrossSigningService.kt

@@ -25,7 +25,7 @@ import im.vector.matrix.android.internal.crypto.model.rest.UserPasswordAuth
 
 interface CrossSigningService {
 
-    fun isCrossSigningEnabled(): Boolean
+    fun isCrossSigningVerified(): Boolean
 
     fun isUserTrusted(otherUserId: String): Boolean
 

matrix-sdk-android/src/main/java/im/vector/matrix/android/api/session/crypto/sas/QrCodeVerificationTransaction.kt

@@ -27,4 +27,14 @@ interface QrCodeVerificationTransaction : VerificationTransaction {
      * Call when you have scan the other user QR code
      */
     fun userHasScannedOtherQrCode(otherQrCodeText: String)
+
+    /**
+     * Call when you confirm that other user has scanned your QR code
+     */
+    fun otherUserScannedMyQrCode()
+
+    /**
+     * Call when you do not confirm that other user has scanned your QR code
+     */
+    fun otherUserDidNotScannedMyQrCode()
 }

matrix-sdk-android/src/main/java/im/vector/matrix/android/api/session/crypto/sas/VerificationTxState.kt

@@ -39,7 +39,10 @@ sealed class VerificationTxState {
     object Verifying : VerificationSasTxState()
 
     // Specific for QR code
-    // TODO Add code for the confirmation step for the user who has been scanned
+    abstract class VerificationQrTxState : VerificationTxState()
+
+    // Will be used to ask the user if the other user has correctly scanned
+    object QrScannedByOther : VerificationQrTxState()
 
     // Terminal states
     abstract class TerminalTxState : VerificationTxState()

matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/DefaultCrossSigningService.kt

@@ -305,7 +305,7 @@ internal class DefaultCrossSigningService @Inject constructor(
         return cryptoStore.getCrossSigningInfo(userId)?.isTrusted() == true
     }
 
-    override fun isCrossSigningEnabled(): Boolean {
+    override fun isCrossSigningVerified(): Boolean {
         return checkSelfTrust().isVerified()
     }
 

matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/store/db/model/CrossSigningInfoEntity.kt

@@ -55,5 +55,4 @@ internal open class CrossSigningInfoEntity(
                 .forEach { crossSigningKeys.remove(it) }
         info?.let { crossSigningKeys.add(it) }
     }
-
 }

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

@@ -878,8 +878,8 @@ internal class DefaultVerificationService @Inject constructor(
                 otherUserId = otherUserId
         )
 
-        // We can SCAN or SHOW QR codes only if cross-signing is enabled
+        // We can SCAN or SHOW QR codes only if cross-signing is verified
-        val methodValues = if (crossSigningService.isCrossSigningEnabled()) {
+        val methodValues = if (crossSigningService.isCrossSigningVerified()) {
             // Add reciprocate method if application declares it can scan or show QR codes
             // Not sure if it ok to do that (?)
             val reciprocateMethod = methods

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

@@ -198,13 +198,24 @@ internal class DefaultQrCodeVerificationTransaction(
         if (startReq.sharedSecret == qrCodeData.sharedSecret) {
             // Ok, we can trust the other user
             // We can only trust the master key in this case
-            trust(true, emptyList())
+            // But first, ask the user for a confirmation
+            state = VerificationTxState.QrScannedByOther
         } else {
             // Display a warning
             cancel(CancelCode.MismatchedKeys)
         }
     }
 
+    override fun otherUserScannedMyQrCode() {
+        trust(true, emptyList())
+    }
+
+    override fun otherUserDidNotScannedMyQrCode() {
+        // What can I do then?
+        // At least remove the transaction...
+        state = VerificationTxState.Cancelled(CancelCode.MismatchedKeys, true)
+    }
+
     private fun trust(canTrustOtherUserMasterKey: Boolean, toVerifyDeviceIds: List<String>) {
         // If not me sign his MSK and upload the signature
         if (otherUserId != userId && canTrustOtherUserMasterKey) {

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

@@ -52,7 +52,7 @@ fun QrCodeData.toUrl(): String {
         append(URLEncoder.encode(action, ENCODING))
 
         for ((keyId, key) in keys) {
-            append("&key_$keyId=")
+            append("&key_${URLEncoder.encode(keyId, ENCODING)}=")
             append(URLEncoder.encode(key, ENCODING))
         }
 
@@ -105,7 +105,7 @@ fun String.toQrCodeData(): QrCodeData? {
     val keys = keyValues.keys
             .filter { it.startsWith("key_") }
             .map {
-                it.substringAfter("key_") to (keyValues[it] ?: return null)
+                URLDecoder.decode(it.substringAfter("key_"), ENCODING) to (keyValues[it] ?: return null)
             }
             .toMap()
 

matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/create/CreateRoomTask.kt

@@ -17,7 +17,6 @@
 package im.vector.matrix.android.internal.session.room.create
 
 import com.zhuinden.monarchy.Monarchy
-import im.vector.matrix.android.api.extensions.orTrue
 import im.vector.matrix.android.api.session.crypto.crosssigning.CrossSigningService
 import im.vector.matrix.android.api.session.room.failure.CreateRoomFailure
 import im.vector.matrix.android.api.session.room.model.create.CreateRoomParams
@@ -58,32 +57,11 @@ internal class DefaultCreateRoomTask @Inject constructor(
 ) : CreateRoomTask {
 
     override suspend fun execute(params: CreateRoomParams): String {
-        val createRoomParams = params
+        val createRoomParams = if (canEnableEncryption(params)) {
-                .takeIf { it.enableEncryptionIfInvitedUsersSupportIt }
+            params.enableEncryptionWithAlgorithm()
-                ?.takeIf { crossSigningService.isCrossSigningEnabled() }
+        } else {
-                ?.takeIf { it.invite3pids.isNullOrEmpty() }
+            params
-                ?.invitedUserIds
+        }
-                ?.let { userIds ->
-                    val keys = deviceListManager.downloadKeys(userIds, forceDownload = false)
-
-                    userIds.any { userId ->
-                        if (keys.map[userId].isNullOrEmpty()) {
-                            // A user has no device, so do not enable encryption
-                            true
-                        } else {
-                            // Check that every user's device have at least one key
-                            keys.map[userId]?.values?.any { it.keys.isNullOrEmpty() } ?: true
-                        }
-                    }
-                }
-                .orTrue()
-                .let { cannotEnableEncryption ->
-                    if (!cannotEnableEncryption) {
-                        params.enableEncryptionWithAlgorithm()
-                    } else {
-                        params
-                    }
-                }
 
         val createRoomResponse = executeRequest<CreateRoomResponse>(eventBus) {
             apiCall = roomAPI.createRoom(createRoomParams)
@@ -105,6 +83,28 @@ internal class DefaultCreateRoomTask @Inject constructor(
         return roomId
     }
 
+    private suspend fun canEnableEncryption(params: CreateRoomParams): Boolean {
+        return params.enableEncryptionIfInvitedUsersSupportIt
+                && crossSigningService.isCrossSigningVerified()
+                && params.invite3pids.isNullOrEmpty()
+                && params.invitedUserIds?.isNotEmpty() == true
+                && params.invitedUserIds.let { userIds ->
+            val keys = deviceListManager.downloadKeys(userIds, forceDownload = false)
+
+            userIds.all { userId ->
+                keys.map[userId].let { deviceMap ->
+                    if (deviceMap.isNullOrEmpty()) {
+                        // A user has no device, so do not enable encryption
+                        false
+                    } else {
+                        // Check that every user's device have at least one key
+                        deviceMap.values.all { !it.keys.isNullOrEmpty() }
+                    }
+                }
+            }
+        }
+    }
+
     private suspend fun handleDirectChatCreation(params: CreateRoomParams, roomId: String) {
         val otherUserId = params.getFirstInvitedUserId()
                 ?: throw IllegalStateException("You can't create a direct room without an invitedUser")

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

@@ -84,6 +84,29 @@ class QrCodeTest {
         decodedData.otherUserKey shouldBeEqualTo "otherUserKey"
     }
 
+    @Test
+    fun testUrlCharInKeys() {
+        val url = basicQrCodeData
+                .copy(
+                        keys = mapOf(
+                                "/=" to "abcdef",
+                                "&?" to "ghijql"
+                        )
+                )
+                .toUrl()
+
+        url shouldBeEqualTo basicUrl
+                .replace("key_1=abcdef", "key_%2F%3D=abcdef")
+                .replace("key_2=ghijql", "key_%26%3F=ghijql")
+
+        val decodedData = url.toQrCodeData()
+
+        decodedData.shouldNotBeNull()
+
+        decodedData.keys["/="]?.shouldBeEqualTo("abcdef")
+        decodedData.keys["&&"]?.shouldBeEqualTo("ghijql")
+    }
+
     @Test
     fun testMissingActionCase() {
         basicUrl.replace("&action=verify", "")

vector/src/main/java/im/vector/riotx/core/di/FragmentModule.kt

@@ -28,6 +28,7 @@ import im.vector.riotx.features.crypto.keysbackup.settings.KeysBackupSettingsFra
 import im.vector.riotx.features.crypto.verification.choose.VerificationChooseMethodFragment
 import im.vector.riotx.features.crypto.verification.conclusion.VerificationConclusionFragment
 import im.vector.riotx.features.crypto.verification.emoji.VerificationEmojiCodeFragment
+import im.vector.riotx.features.crypto.verification.qrconfirmation.VerificationQrScannedByOtherFragment
 import im.vector.riotx.features.crypto.verification.request.VerificationRequestFragment
 import im.vector.riotx.features.grouplist.GroupListFragment
 import im.vector.riotx.features.home.HomeDetailFragment
@@ -77,7 +78,6 @@ import im.vector.riotx.features.signout.soft.SoftLogoutFragment
 
 @Module
 interface FragmentModule {
-
     /**
      * Fragments with @IntoMap will be injected by this factory
      */
@@ -319,6 +319,11 @@ interface FragmentModule {
     @FragmentKey(VerificationEmojiCodeFragment::class)
     fun bindVerificationEmojiCodeFragment(fragment: VerificationEmojiCodeFragment): Fragment
 
+    @Binds
+    @IntoMap
+    @FragmentKey(VerificationQrScannedByOtherFragment::class)
+    fun bindVerificationQrScannedByOtherFragment(fragment: VerificationQrScannedByOtherFragment): Fragment
+
     @Binds
     @IntoMap
     @FragmentKey(VerificationConclusionFragment::class)

vector/src/main/java/im/vector/riotx/features/crypto/verification/VerificationAction.kt

@@ -18,10 +18,13 @@ package im.vector.riotx.features.crypto.verification
 
 import im.vector.riotx.core.platform.VectorViewModelAction
 
+// TODO Remove otherUserId and transactionId when it's not necessary. Should be known by the ViewModel, no?
 sealed class VerificationAction : VectorViewModelAction {
     data class RequestVerificationByDM(val otherUserId: String, val roomId: String?) : VerificationAction()
     data class StartSASVerification(val otherUserId: String, val pendingRequestTransactionId: String) : VerificationAction()
     data class RemoteQrCodeScanned(val otherUserId: String, val transactionId: String, val scannedData: String) : VerificationAction()
+    object OtherUserScannedSuccessfully : VerificationAction()
+    object OtherUserDidNotScanned : VerificationAction()
     data class SASMatchAction(val otherUserId: String, val sasTransactionId: String) : VerificationAction()
     data class SASDoNotMatchAction(val otherUserId: String, val sasTransactionId: String) : VerificationAction()
     object GotItConclusion : VerificationAction()

vector/src/main/java/im/vector/riotx/features/crypto/verification/VerificationBottomSheet.kt

@@ -39,6 +39,7 @@ import im.vector.riotx.core.platform.VectorBaseBottomSheetDialogFragment
 import im.vector.riotx.features.crypto.verification.choose.VerificationChooseMethodFragment
 import im.vector.riotx.features.crypto.verification.conclusion.VerificationConclusionFragment
 import im.vector.riotx.features.crypto.verification.emoji.VerificationEmojiCodeFragment
+import im.vector.riotx.features.crypto.verification.qrconfirmation.VerificationQrScannedByOtherFragment
 import im.vector.riotx.features.crypto.verification.request.VerificationRequestFragment
 import im.vector.riotx.features.home.AvatarRenderer
 import kotlinx.android.parcel.Parcelize
@@ -149,19 +150,23 @@ class VerificationBottomSheet : VectorBaseBottomSheetDialogFragment() {
         }
 
         when (it.qrTransactionState) {
-            is VerificationTxState.Verified  -> {
+            is VerificationTxState.QrScannedByOther -> {
+                showFragment(VerificationQrScannedByOtherFragment::class, Bundle())
+                return@withState
+            }
+            is VerificationTxState.Verified         -> {
                 showFragment(VerificationConclusionFragment::class, Bundle().apply {
                     putParcelable(MvRx.KEY_ARG, VerificationConclusionFragment.Args(true, null))
                 })
                 return@withState
             }
-            is VerificationTxState.Cancelled -> {
+            is VerificationTxState.Cancelled        -> {
                 showFragment(VerificationConclusionFragment::class, Bundle().apply {
                     putParcelable(MvRx.KEY_ARG, VerificationConclusionFragment.Args(false, it.qrTransactionState.cancelCode.value))
                 })
                 return@withState
             }
-            else                             -> Unit
+            else                                    -> Unit
         }
 
         // At this point there is no SAS transaction for this request

vector/src/main/java/im/vector/riotx/features/crypto/verification/VerificationBottomSheetViewModel.kt

@@ -42,6 +42,7 @@ import im.vector.matrix.android.api.util.MatrixItem
 import im.vector.matrix.android.api.util.toMatrixItem
 import im.vector.matrix.android.internal.crypto.verification.PendingVerificationRequest
 import im.vector.riotx.core.di.HasScreenInjector
+import im.vector.riotx.core.extensions.exhaustive
 import im.vector.riotx.core.platform.EmptyViewEvents
 import im.vector.riotx.core.platform.VectorViewModel
 import im.vector.riotx.core.utils.LiveEvent
@@ -118,7 +119,7 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(@Assisted ini
                 ?: session.getExistingDirectRoomWithUser(otherUserId)?.roomId
 
         when (action) {
-            is VerificationAction.RequestVerificationByDM -> {
+            is VerificationAction.RequestVerificationByDM      -> {
                 if (roomId == null) {
                     val localID = LocalEcho.createLocalEchoId()
                     setState {
@@ -163,8 +164,9 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(@Assisted ini
                         )
                     }
                 }
+                Unit
             }
-            is VerificationAction.StartSASVerification    -> {
+            is VerificationAction.StartSASVerification         -> {
                 val request = session.getVerificationService().getExistingVerificationRequest(otherUserId, action.pendingRequestTransactionId)
                         ?: return@withState
                 if (roomId == null) return@withState
@@ -177,8 +179,9 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(@Assisted ini
                         otherDeviceId = otherDevice ?: "",
                         callback = null
                 )
+                Unit
             }
-            is VerificationAction.RemoteQrCodeScanned     -> {
+            is VerificationAction.RemoteQrCodeScanned          -> {
                 val existingTransaction = session.getVerificationService()
                         .getExistingTransaction(action.otherUserId, action.transactionId) as? QrCodeVerificationTransaction
                 existingTransaction
@@ -189,21 +192,37 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(@Assisted ini
                             // TODO
                         }
             }
-            is VerificationAction.SASMatchAction          -> {
+            is VerificationAction.OtherUserScannedSuccessfully -> {
+                val transactionId = state.transactionId ?: return@withState
+
+                val existingTransaction = session.getVerificationService()
+                        .getExistingTransaction(otherUserId, transactionId) as? QrCodeVerificationTransaction
+                existingTransaction
+                        ?.otherUserScannedMyQrCode()
+            }
+            is VerificationAction.OtherUserDidNotScanned       -> {
+                val transactionId = state.transactionId ?: return@withState
+
+                val existingTransaction = session.getVerificationService()
+                        .getExistingTransaction(otherUserId, transactionId) as? QrCodeVerificationTransaction
+                existingTransaction
+                        ?.otherUserDidNotScannedMyQrCode()
+            }
+            is VerificationAction.SASMatchAction               -> {
                 (session.getVerificationService()
                         .getExistingTransaction(action.otherUserId, action.sasTransactionId)
                         as? SasVerificationTransaction)?.userHasVerifiedShortCode()
             }
-            is VerificationAction.SASDoNotMatchAction     -> {
+            is VerificationAction.SASDoNotMatchAction          -> {
                 (session.getVerificationService()
                         .getExistingTransaction(action.otherUserId, action.sasTransactionId)
                         as? SasVerificationTransaction)
                         ?.shortCodeDoesNotMatch()
             }
-            is VerificationAction.GotItConclusion         -> {
+            is VerificationAction.GotItConclusion              -> {
                 _requestLiveData.postValue(LiveEvent(Success(action)))
             }
-        }
+        }.exhaustive
     }
 
     override fun transactionCreated(tx: VerificationTransaction) {

vector/src/main/java/im/vector/riotx/features/crypto/verification/qrconfirmation/VerificationQrScannedByOtherController.kt

@@ -0,0 +1,76 @@
+/*
+ * Copyright 2020 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package im.vector.riotx.features.crypto.verification.qrconfirmation
+
+import com.airbnb.epoxy.EpoxyController
+import im.vector.riotx.R
+import im.vector.riotx.core.epoxy.dividerItem
+import im.vector.riotx.core.resources.ColorProvider
+import im.vector.riotx.core.resources.StringProvider
+import im.vector.riotx.features.crypto.verification.epoxy.bottomSheetVerificationActionItem
+import im.vector.riotx.features.crypto.verification.epoxy.bottomSheetVerificationNoticeItem
+import javax.inject.Inject
+
+class VerificationQrScannedByOtherController @Inject constructor(
+        private val stringProvider: StringProvider,
+        private val colorProvider: ColorProvider
+) : EpoxyController() {
+
+    var listener: Listener? = null
+
+    init {
+        requestModelBuild()
+    }
+
+    override fun buildModels() {
+        bottomSheetVerificationNoticeItem {
+            id("notice")
+            notice(stringProvider.getString(R.string.qr_code_scanned_by_other_notice))
+        }
+
+        dividerItem {
+            id("sep0")
+        }
+
+        bottomSheetVerificationActionItem {
+            id("confirm")
+            title(stringProvider.getString(R.string.qr_code_scanned_by_other_yes))
+            titleColor(colorProvider.getColor(R.color.riotx_accent))
+            iconRes(R.drawable.ic_check_on)
+            iconColor(colorProvider.getColor(R.color.riotx_accent))
+            listener { listener?.onUserConfirmsQrCodeScanned() }
+        }
+
+        dividerItem {
+            id("sep1")
+        }
+
+        bottomSheetVerificationActionItem {
+            id("deny")
+            title(stringProvider.getString(R.string.qr_code_scanned_by_other_no))
+            titleColor(colorProvider.getColor(R.color.vector_error_color))
+            iconRes(R.drawable.ic_check_off)
+            iconColor(colorProvider.getColor(R.color.vector_error_color))
+            listener { listener?.onUserDeniesQrCodeScanned() }
+        }
+    }
+
+    interface Listener {
+        fun onUserConfirmsQrCodeScanned()
+        fun onUserDeniesQrCodeScanned()
+    }
+}

vector/src/main/java/im/vector/riotx/features/crypto/verification/qrconfirmation/VerificationQrScannedByOtherFragment.kt

@@ -0,0 +1,62 @@
+/*
+ * Copyright 2019 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package im.vector.riotx.features.crypto.verification.qrconfirmation
+
+import android.os.Bundle
+import android.view.View
+import com.airbnb.mvrx.parentFragmentViewModel
+import im.vector.riotx.R
+import im.vector.riotx.core.extensions.cleanup
+import im.vector.riotx.core.extensions.configureWith
+import im.vector.riotx.core.platform.VectorBaseFragment
+import im.vector.riotx.features.crypto.verification.VerificationAction
+import im.vector.riotx.features.crypto.verification.VerificationBottomSheetViewModel
+import kotlinx.android.synthetic.main.bottom_sheet_verification_child_fragment.*
+import javax.inject.Inject
+
+class VerificationQrScannedByOtherFragment @Inject constructor(
+        val controller: VerificationQrScannedByOtherController
+) : VectorBaseFragment(), VerificationQrScannedByOtherController.Listener {
+
+    private val sharedViewModel by parentFragmentViewModel(VerificationBottomSheetViewModel::class)
+
+    override fun getLayoutResId() = R.layout.bottom_sheet_verification_child_fragment
+
+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
+        super.onViewCreated(view, savedInstanceState)
+
+        setupRecyclerView()
+    }
+
+    override fun onDestroyView() {
+        bottomSheetVerificationRecyclerView.cleanup()
+        controller.listener = null
+        super.onDestroyView()
+    }
+
+    private fun setupRecyclerView() {
+        bottomSheetVerificationRecyclerView.configureWith(controller, hasFixedSize = false, disableItemAnimation = true)
+        controller.listener = this
+    }
+
+    override fun onUserConfirmsQrCodeScanned() {
+        sharedViewModel.handle(VerificationAction.OtherUserScannedSuccessfully)
+    }
+
+    override fun onUserDeniesQrCodeScanned() {
+        sharedViewModel.handle(VerificationAction.OtherUserDidNotScanned)
+    }
+}

vector/src/main/res/values/strings_riotX.xml

@@ -147,4 +147,8 @@
     <string name="reset_cross_signing">Reset Keys</string>
 
     <string name="a11y_qr_code_for_verification">QR code</string>
+
+    <string name="qr_code_scanned_by_other_notice">Did the other user successfully scan the QR code?</string>
+    <string name="qr_code_scanned_by_other_yes">Yes</string>
+    <string name="qr_code_scanned_by_other_no">No</string>
 </resources>