Merge pull request #3711 from vector-im/feature/bma/sendToDevice

Ensure that txnId is the same if the request is retried

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/verification/VerificationService.kt

@@ -52,7 +52,7 @@ interface VerificationService {
                              transactionId: String?): String?
 
     /**
-     * Request a key verification from another user using toDevice events.
+     * Request key verification with another user via room events (instead of the to-device API)
      */
     fun requestKeyVerificationInDMs(methods: List<VerificationMethod>,
                                     otherUserId: String,

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/CancelGossipRequestWorker.kt

@@ -23,12 +23,12 @@ import org.matrix.android.sdk.api.auth.data.Credentials
 import org.matrix.android.sdk.api.failure.shouldBeRetried
 import org.matrix.android.sdk.api.session.events.model.Event
 import org.matrix.android.sdk.api.session.events.model.EventType
-import org.matrix.android.sdk.api.session.events.model.LocalEcho
 import org.matrix.android.sdk.api.session.events.model.toContent
 import org.matrix.android.sdk.internal.crypto.model.MXUsersDevicesMap
 import org.matrix.android.sdk.internal.crypto.model.rest.ShareRequestCancellation
 import org.matrix.android.sdk.internal.crypto.store.IMXCryptoStore
 import org.matrix.android.sdk.internal.crypto.tasks.SendToDeviceTask
+import org.matrix.android.sdk.internal.crypto.tasks.createUniqueTxnId
 import org.matrix.android.sdk.internal.session.SessionComponent
 import org.matrix.android.sdk.internal.worker.SessionSafeCoroutineWorker
 import org.matrix.android.sdk.internal.worker.SessionWorkerParams
@@ -43,6 +43,9 @@ internal class CancelGossipRequestWorker(context: Context,
             override val sessionId: String,
             val requestId: String,
             val recipients: Map<String, List<String>>,
+            // The txnId for the sendToDevice request. Nullable for compatibility reasons, but MUST always be provided
+            // to use the same value if this worker is retried.
+            val txnId: String? = null,
             override val lastFailureMessage: String? = null
     ) : SessionWorkerParams {
         companion object {
@@ -51,6 +54,7 @@ internal class CancelGossipRequestWorker(context: Context,
                         sessionId = sessionId,
                         requestId = request.requestId,
                         recipients = request.recipients,
+                        txnId = createUniqueTxnId(),
                         lastFailureMessage = null
                 )
             }
@@ -66,7 +70,10 @@ internal class CancelGossipRequestWorker(context: Context,
     }
 
     override suspend fun doSafeWork(params: Params): Result {
-        val localId = LocalEcho.createLocalEchoId()
+        // params.txnId should be provided in all cases now. But Params can be deserialized by
+        // the WorkManager from data serialized in a previous version of the application, so without the txnId field.
+        // So if not present, we create a txnId
+        val txnId = params.txnId ?: createUniqueTxnId()
         val contentMap = MXUsersDevicesMap<Any>()
         val toDeviceContent = ShareRequestCancellation(
                 requestingDeviceId = credentials.deviceId,
@@ -92,7 +99,7 @@ internal class CancelGossipRequestWorker(context: Context,
                     SendToDeviceTask.Params(
                             eventType = EventType.ROOM_KEY_REQUEST,
                             contentMap = contentMap,
-                            transactionId = localId
+                            transactionId = txnId
                     )
             )
             cryptoStore.updateOutgoingGossipingRequestState(params.requestId, OutgoingGossipingRequestState.CANCELLED)

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/IncomingGossipingRequestManager.kt

@@ -35,6 +35,7 @@ import org.matrix.android.sdk.internal.crypto.model.rest.GossipingDefaultContent
 import org.matrix.android.sdk.internal.crypto.model.rest.GossipingToDeviceObject
 import org.matrix.android.sdk.internal.crypto.model.rest.RoomKeyRequestBody
 import org.matrix.android.sdk.internal.crypto.store.IMXCryptoStore
+import org.matrix.android.sdk.internal.crypto.tasks.createUniqueTxnId
 import org.matrix.android.sdk.internal.di.SessionId
 import org.matrix.android.sdk.internal.session.SessionScope
 import org.matrix.android.sdk.internal.util.MatrixCoroutineDispatchers
@@ -356,7 +357,8 @@ internal class IncomingGossipingRequestManager @Inject constructor(
                         secretValue = secretValue,
                         requestUserId = request.userId,
                         requestDeviceId = request.deviceId,
-                        requestId = request.requestId
+                        requestId = request.requestId,
+                        txnId = createUniqueTxnId()
                 )
 
                 cryptoStore.updateGossipingRequestState(request, GossipingRequestState.ACCEPTING)
@@ -376,13 +378,13 @@ internal class IncomingGossipingRequestManager @Inject constructor(
         }
 
         request.share = { secretValue ->
-
             val params = SendGossipWorker.Params(
                     sessionId = userId,
                     secretValue = secretValue,
                     requestUserId = request.userId,
                     requestDeviceId = request.deviceId,
-                    requestId = request.requestId
+                    requestId = request.requestId,
+                    txnId = createUniqueTxnId()
             )
 
             cryptoStore.updateGossipingRequestState(request, GossipingRequestState.ACCEPTING)

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OutgoingGossipingRequestManager.kt

@@ -16,7 +16,6 @@
 
 package org.matrix.android.sdk.internal.crypto
 
-import org.matrix.android.sdk.api.session.events.model.LocalEcho
 import org.matrix.android.sdk.internal.crypto.model.rest.RoomKeyRequestBody
 import org.matrix.android.sdk.internal.crypto.store.IMXCryptoStore
 import org.matrix.android.sdk.internal.di.SessionId
@@ -26,6 +25,8 @@ import org.matrix.android.sdk.internal.worker.WorkerParamsFactory
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.launch
+import org.matrix.android.sdk.internal.crypto.tasks.createUniqueTxnId
+import org.matrix.android.sdk.internal.crypto.util.RequestIdHelper
 import timber.log.Timber
 import javax.inject.Inject
 
@@ -131,7 +132,8 @@ internal class OutgoingGossipingRequestManager @Inject constructor(
         val params = SendGossipRequestWorker.Params(
                 sessionId = sessionId,
                 keyShareRequest = request as? OutgoingRoomKeyRequest,
-                secretShareRequest = request as? OutgoingSecretRequest
+                secretShareRequest = request as? OutgoingSecretRequest,
+                txnId = createUniqueTxnId()
         )
         cryptoStore.updateOutgoingGossipingRequestState(request.requestId, OutgoingGossipingRequestState.SENDING)
         val workRequest = gossipingWorkManager.createWork<SendGossipRequestWorker>(WorkerParamsFactory.toData(params), true)
@@ -154,7 +156,8 @@ internal class OutgoingGossipingRequestManager @Inject constructor(
         if (resend) {
             val reSendParams = SendGossipRequestWorker.Params(
                     sessionId = sessionId,
-                    keyShareRequest = request.copy(requestId = LocalEcho.createLocalEchoId())
+                    keyShareRequest = request.copy(requestId = RequestIdHelper.createUniqueRequestId()),
+                    txnId = createUniqueTxnId()
             )
             val reSendWorkRequest = gossipingWorkManager.createWork<SendGossipRequestWorker>(WorkerParamsFactory.toData(reSendParams), true)
             gossipingWorkManager.postWork(reSendWorkRequest)

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/SendGossipRequestWorker.kt

@@ -23,7 +23,6 @@ import org.matrix.android.sdk.api.auth.data.Credentials
 import org.matrix.android.sdk.api.failure.shouldBeRetried
 import org.matrix.android.sdk.api.session.events.model.Event
 import org.matrix.android.sdk.api.session.events.model.EventType
-import org.matrix.android.sdk.api.session.events.model.LocalEcho
 import org.matrix.android.sdk.api.session.events.model.toContent
 import org.matrix.android.sdk.internal.crypto.model.MXUsersDevicesMap
 import org.matrix.android.sdk.internal.crypto.model.rest.GossipingToDeviceObject
@@ -31,6 +30,7 @@ import org.matrix.android.sdk.internal.crypto.model.rest.RoomKeyShareRequest
 import org.matrix.android.sdk.internal.crypto.model.rest.SecretShareRequest
 import org.matrix.android.sdk.internal.crypto.store.IMXCryptoStore
 import org.matrix.android.sdk.internal.crypto.tasks.SendToDeviceTask
+import org.matrix.android.sdk.internal.crypto.tasks.createUniqueTxnId
 import org.matrix.android.sdk.internal.session.SessionComponent
 import org.matrix.android.sdk.internal.worker.SessionSafeCoroutineWorker
 import org.matrix.android.sdk.internal.worker.SessionWorkerParams
@@ -46,6 +46,9 @@ internal class SendGossipRequestWorker(context: Context,
             override val sessionId: String,
             val keyShareRequest: OutgoingRoomKeyRequest? = null,
             val secretShareRequest: OutgoingSecretRequest? = null,
+            // The txnId for the sendToDevice request. Nullable for compatibility reasons, but MUST always be provided
+            // to use the same value if this worker is retried.
+            val txnId: String? = null,
             override val lastFailureMessage: String? = null
     ) : SessionWorkerParams
 
@@ -58,7 +61,10 @@ internal class SendGossipRequestWorker(context: Context,
     }
 
     override suspend fun doSafeWork(params: Params): Result {
-        val localId = LocalEcho.createLocalEchoId()
+        // params.txnId should be provided in all cases now. But Params can be deserialized by
+        // the WorkManager from data serialized in a previous version of the application, so without the txnId field.
+        // So if not present, we create a txnId
+        val txnId = params.txnId ?: createUniqueTxnId()
         val contentMap = MXUsersDevicesMap<Any>()
         val eventType: String
         val requestId: String
@@ -122,7 +128,7 @@ internal class SendGossipRequestWorker(context: Context,
                     SendToDeviceTask.Params(
                             eventType = eventType,
                             contentMap = contentMap,
-                            transactionId = localId
+                            transactionId = txnId
                     )
             )
             cryptoStore.updateOutgoingGossipingRequestState(requestId, OutgoingGossipingRequestState.SENT)

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/SendGossipWorker.kt

@@ -23,7 +23,6 @@ import org.matrix.android.sdk.api.auth.data.Credentials
 import org.matrix.android.sdk.api.failure.shouldBeRetried
 import org.matrix.android.sdk.api.session.events.model.Event
 import org.matrix.android.sdk.api.session.events.model.EventType
-import org.matrix.android.sdk.api.session.events.model.LocalEcho
 import org.matrix.android.sdk.api.session.events.model.toContent
 import org.matrix.android.sdk.internal.crypto.actions.EnsureOlmSessionsForDevicesAction
 import org.matrix.android.sdk.internal.crypto.actions.MessageEncrypter
@@ -31,6 +30,7 @@ import org.matrix.android.sdk.internal.crypto.model.MXUsersDevicesMap
 import org.matrix.android.sdk.internal.crypto.model.event.SecretSendEventContent
 import org.matrix.android.sdk.internal.crypto.store.IMXCryptoStore
 import org.matrix.android.sdk.internal.crypto.tasks.SendToDeviceTask
+import org.matrix.android.sdk.internal.crypto.tasks.createUniqueTxnId
 import org.matrix.android.sdk.internal.session.SessionComponent
 import org.matrix.android.sdk.internal.worker.SessionSafeCoroutineWorker
 import org.matrix.android.sdk.internal.worker.SessionWorkerParams
@@ -48,6 +48,9 @@ internal class SendGossipWorker(context: Context,
             val requestUserId: String?,
             val requestDeviceId: String?,
             val requestId: String?,
+            // The txnId for the sendToDevice request. Nullable for compatibility reasons, but MUST always be provided
+            // to use the same value if this worker is retried.
+            val txnId: String? = null,
             override val lastFailureMessage: String? = null
     ) : SessionWorkerParams
 
@@ -62,7 +65,10 @@ internal class SendGossipWorker(context: Context,
     }
 
     override suspend fun doSafeWork(params: Params): Result {
-        val localId = LocalEcho.createLocalEchoId()
+        // params.txnId should be provided in all cases now. But Params can be deserialized by
+        // the WorkManager from data serialized in a previous version of the application, so without the txnId field.
+        // So if not present, we create a txnId
+        val txnId = params.txnId ?: createUniqueTxnId()
         val eventType: String = EventType.SEND_SECRET
 
         val toDeviceContent = SecretSendEventContent(
@@ -127,7 +133,7 @@ internal class SendGossipWorker(context: Context,
                     SendToDeviceTask.Params(
                             eventType = EventType.ENCRYPTED,
                             contentMap = sendToDeviceMap,
-                            transactionId = localId
+                            transactionId = txnId
                     )
             )
             cryptoStore.updateGossipingRequestState(

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/store/db/RealmCryptoStore.kt

@@ -27,7 +27,6 @@ import io.realm.Sort
 import io.realm.kotlin.where
 import org.matrix.android.sdk.api.session.crypto.crosssigning.MXCrossSigningInfo
 import org.matrix.android.sdk.api.session.events.model.Event
-import org.matrix.android.sdk.api.session.events.model.LocalEcho
 import org.matrix.android.sdk.api.session.room.send.SendState
 import org.matrix.android.sdk.api.util.Optional
 import org.matrix.android.sdk.api.util.toOptional
@@ -88,6 +87,7 @@ import org.matrix.android.sdk.internal.crypto.store.db.query.delete
 import org.matrix.android.sdk.internal.crypto.store.db.query.get
 import org.matrix.android.sdk.internal.crypto.store.db.query.getById
 import org.matrix.android.sdk.internal.crypto.store.db.query.getOrCreate
+import org.matrix.android.sdk.internal.crypto.util.RequestIdHelper
 import org.matrix.android.sdk.internal.database.mapper.ContentMapper
 import org.matrix.android.sdk.internal.database.tools.RealmDebugTools
 import org.matrix.android.sdk.internal.di.CryptoDatabase
@@ -1121,7 +1121,7 @@ internal class RealmCryptoStore @Inject constructor(
 
             if (existing == null) {
                 request = realm.createObject(OutgoingGossipingRequestEntity::class.java).apply {
-                    this.requestId = LocalEcho.createLocalEchoId()
+                    this.requestId = RequestIdHelper.createUniqueRequestId()
                     this.setRecipients(recipients)
                     this.requestState = OutgoingGossipingRequestState.UNSENT
                     this.type = GossipRequestType.KEY
@@ -1151,7 +1151,7 @@ internal class RealmCryptoStore @Inject constructor(
                     this.type = GossipRequestType.SECRET
                     setRecipients(recipients)
                     this.requestState = OutgoingGossipingRequestState.UNSENT
-                    this.requestId = LocalEcho.createLocalEchoId()
+                    this.requestId = RequestIdHelper.createUniqueRequestId()
                     this.requestedInfoStr = secretName
                 }.toOutgoingGossipingRequest() as? OutgoingSecretRequest
             } else {

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/SendToDeviceTask.kt

@@ -22,8 +22,8 @@ import org.matrix.android.sdk.internal.crypto.model.rest.SendToDeviceBody
 import org.matrix.android.sdk.internal.network.GlobalErrorReceiver
 import org.matrix.android.sdk.internal.network.executeRequest
 import org.matrix.android.sdk.internal.task.Task
+import java.util.UUID
 import javax.inject.Inject
-import kotlin.random.Random
 
 internal interface SendToDeviceTask : Task<SendToDeviceTask.Params, Unit> {
     data class Params(
@@ -31,7 +31,7 @@ internal interface SendToDeviceTask : Task<SendToDeviceTask.Params, Unit> {
             val eventType: String,
             // the content to send. Map from user_id to device_id to content dictionary.
             val contentMap: MXUsersDevicesMap<Any>,
-            // the transactionId
+            // the transactionId. If not provided, a transactionId will be created by the task
             val transactionId: String? = null
     )
 }
@@ -46,16 +46,23 @@ internal class DefaultSendToDeviceTask @Inject constructor(
                 messages = params.contentMap.map
         )
 
+        // If params.transactionId is not provided, we create a unique txnId.
+        // It's important to do that outside the requestBlock parameter of executeRequest()
+        // to use the same value if the request is retried
+        val txnId = params.transactionId ?: createUniqueTxnId()
+
         return executeRequest(
                 globalErrorReceiver,
                 canRetry = true,
                 maxRetriesCount = 3
         ) {
             cryptoApi.sendToDevice(
-                    params.eventType,
-                    params.transactionId ?: Random.nextInt(Integer.MAX_VALUE).toString(),
-                    sendToDeviceBody
+                    eventType = params.eventType,
+                    transactionId = txnId,
+                    body = sendToDeviceBody
             )
         }
     }
 }
+
+internal fun createUniqueTxnId() = UUID.randomUUID().toString()

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/util/RequestIdHelper.kt

@@ -0,0 +1,23 @@
+/*
+ * Copyright (c) 2021 The Matrix.org Foundation C.I.C.
+ *
+ * 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 org.matrix.android.sdk.internal.crypto.util
+
+import java.util.UUID
+
+internal object RequestIdHelper {
+    fun createUniqueRequestId() = UUID.randomUUID().toString()
+}