Code Review

This commit is contained in:
Valere 2021-02-17 16:47:40 +01:00 committed by Benoit Marty
parent 4450f51d78
commit 533a7bb180
14 changed files with 197 additions and 85 deletions

View file

@ -1,62 +0,0 @@
/*
* Copyright 2020 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
import androidx.test.ext.junit.runners.AndroidJUnit4
import org.junit.FixMethodOrder
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.MethodSorters
import org.matrix.android.sdk.InstrumentedTest
import org.matrix.android.sdk.common.CommonTestHelper
import org.matrix.android.sdk.common.CryptoTestHelper
import org.matrix.android.sdk.common.SessionTestParams
import org.matrix.android.sdk.common.TestConstants
import kotlin.test.assertTrue
@RunWith(AndroidJUnit4::class)
@FixMethodOrder(MethodSorters.JVM)
class CryptoServiceTest : InstrumentedTest {
private val mTestHelper = CommonTestHelper(context())
private val mCryptoTestHelper = CryptoTestHelper(mTestHelper)
@Test
fun ensure_outbound_session_happy_path() {
val aliceSession = mTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(true))
val bobSession = mTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(true))
// Initialize cross signing on both
mCryptoTestHelper.initializeCrossSigning(aliceSession)
mCryptoTestHelper.initializeCrossSigning(bobSession)
val roomId = mCryptoTestHelper.createDM(aliceSession, bobSession)
mCryptoTestHelper.verifySASCrossSign(aliceSession, bobSession, roomId)
aliceSession.cryptoService().ensureOutboundSession(roomId)
assertTrue(
aliceSession
.cryptoService()
.getGossipingEvents()
.map { it.senderId }
.containsAll(
listOf(aliceSession.myUserId, bobSession.myUserId)
)
)
}
}

View file

@ -0,0 +1,103 @@
/*
* Copyright 2020 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
import android.util.Log
import androidx.test.ext.junit.runners.AndroidJUnit4
import org.junit.FixMethodOrder
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.MethodSorters
import org.matrix.android.sdk.InstrumentedTest
import org.matrix.android.sdk.api.session.events.model.EventType
import org.matrix.android.sdk.api.session.events.model.toModel
import org.matrix.android.sdk.common.CommonTestHelper
import org.matrix.android.sdk.common.CryptoTestHelper
import org.matrix.android.sdk.internal.crypto.model.event.EncryptedEventContent
import org.matrix.android.sdk.internal.crypto.model.event.RoomKeyContent
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
@RunWith(AndroidJUnit4::class)
@FixMethodOrder(MethodSorters.JVM)
class PreShareKeysTest : InstrumentedTest {
private val mTestHelper = CommonTestHelper(context())
private val mCryptoTestHelper = CryptoTestHelper(mTestHelper)
@Test
fun ensure_outbound_session_happy_path() {
val testData = mCryptoTestHelper.doE2ETestWithAliceAndBobInARoom(true)
val e2eRoomID = testData.roomId
val aliceSession = testData.firstSession
val bobSession = testData.secondSession!!
// clear any outbound session
aliceSession.cryptoService().discardOutboundSession(e2eRoomID)
val preShareCount = bobSession.cryptoService().getGossipingEvents().count {
it.senderId == aliceSession.myUserId
&& it.getClearType() == EventType.ROOM_KEY
}
assertEquals(0, preShareCount, "Bob should not have receive any key from alice at this point")
Log.d("#Test", "Room Key Received from alice $preShareCount")
// Force presharing of new outbound key
mTestHelper.doSync<Unit> {
aliceSession.cryptoService().prepareToEncrypt(e2eRoomID, it)
}
mTestHelper.waitWithLatch { latch ->
mTestHelper.retryPeriodicallyWithLatch(latch) {
val newGossipCount = bobSession.cryptoService().getGossipingEvents().count {
it.senderId == aliceSession.myUserId
&& it.getClearType() == EventType.ROOM_KEY
}
newGossipCount > preShareCount
}
}
val latest = bobSession.cryptoService().getGossipingEvents().lastOrNull {
it.senderId == aliceSession.myUserId
&& it.getClearType() == EventType.ROOM_KEY
}
val content = latest?.getClearContent().toModel<RoomKeyContent>()
assertNotNull(content, "Bob should have received and decrypted a room key event from alice")
assertEquals(e2eRoomID, content.roomId, "Wrong room")
val megolmSessionId = content.sessionId!!
val sharedIndex = aliceSession.cryptoService().getSharedWithInfo(e2eRoomID, megolmSessionId)
.getObject(bobSession.myUserId, bobSession.sessionParams.deviceId)
assertEquals(0, sharedIndex, "The session received by bob should match what alice sent")
// Just send a real message as test
val sentEvent = mTestHelper.sendTextMessage(aliceSession.getRoom(e2eRoomID)!!, "Allo", 1).first()
assertEquals(megolmSessionId, sentEvent.root.content.toModel<EncryptedEventContent>()?.sessionId, "Unexpected megolm session")
mTestHelper.waitWithLatch { latch ->
mTestHelper.retryPeriodicallyWithLatch(latch) {
bobSession.getRoom(e2eRoomID)?.getTimeLineEvent(sentEvent.eventId)?.root?.getClearType() == EventType.MESSAGE
}
}
mTestHelper.signOutAndClose(aliceSession)
mTestHelper.signOutAndClose(bobSession)
}
}

View file

@ -51,7 +51,7 @@ class WithHeldTests : InstrumentedTest {
// =============================
val aliceSession = mTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(true))
val bobSession = mTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(true))
val bobSession = mTestHelper.createAccount(TestConstants.USER_BOB, SessionTestParams(true))
// Initialize cross signing on both
mCryptoTestHelper.initializeCrossSigning(aliceSession)

View file

@ -40,7 +40,6 @@ data class MatrixConfiguration(
* True to advertise support for call transfers to other parties on Matrix calls.
*/
val supportsCallTransfer: Boolean = false
val outboundSessionKeySharingStrategy: OutboundSessionKeySharingStrategy = OutboundSessionKeySharingStrategy.WhenSendingEvent
) {
/**

View file

@ -157,5 +157,9 @@ interface CryptoService {
fun logDbUsageInfo()
fun ensureOutboundSession(roomId: String)
/**
* Perform any background tasks that can be done before a message is ready to
* send, in order to speed up sending of the message.
*/
fun prepareToEncrypt(roomId: String, callback: MatrixCallback<Unit>)
}

View file

@ -36,5 +36,5 @@ interface RoomCryptoService {
* Call this method according to [OutboundSessionKeySharingStrategy].
* If this method is not called, CryptoService will ensure it before sending events.
*/
fun ensureOutboundSession()
suspend fun prepareToEncrypt()
}

View file

@ -50,7 +50,6 @@ import org.matrix.android.sdk.api.session.room.model.Membership
import org.matrix.android.sdk.api.session.room.model.RoomHistoryVisibility
import org.matrix.android.sdk.api.session.room.model.RoomHistoryVisibilityContent
import org.matrix.android.sdk.api.session.room.model.RoomMemberContent
import org.matrix.android.sdk.api.util.emptyJsonDict
import org.matrix.android.sdk.internal.crypto.actions.MegolmSessionDataImporter
import org.matrix.android.sdk.internal.crypto.actions.SetDeviceVerificationAction
import org.matrix.android.sdk.internal.crypto.algorithms.IMXEncrypting
@ -99,7 +98,6 @@ import org.matrix.olm.OlmManager
import timber.log.Timber
import java.util.concurrent.atomic.AtomicBoolean
import javax.inject.Inject
import kotlin.jvm.Throws
import kotlin.math.max
/**
@ -1297,11 +1295,16 @@ internal class DefaultCryptoService @Inject constructor(
cryptoStore.logDbUsageInfo()
}
override fun ensureOutboundSession(roomId: String) {
override fun prepareToEncrypt(roomId: String, callback: MatrixCallback<Unit>) {
cryptoCoroutineScope.launch(coroutineDispatchers.crypto) {
Timber.d("## CRYPTO | prepareToEncrypt() : Check room members up to date")
// Ensure to load all room members
runCatching {
try {
loadRoomMembersTask.execute(LoadRoomMembersTask.Params(roomId))
} catch (failure: Throwable) {
Timber.e("## CRYPTO | prepareToEncrypt() : Failed to load room members")
callback.onFailure(failure)
return@launch
}
val userIds = getRoomUserIds(roomId)
@ -1312,15 +1315,20 @@ internal class DefaultCryptoService @Inject constructor(
if (alg == null) {
val reason = String.format(MXCryptoError.UNABLE_TO_ENCRYPT_REASON, MXCryptoError.NO_MORE_ALGORITHM_REASON)
Timber.e("## CRYPTO | encryptEventContent() : $reason")
Timber.e("## CRYPTO | prepareToEncrypt() : $reason")
callback.onFailure(IllegalArgumentException("Missing algorithm"))
return@launch
}
runCatching {
alg.encryptEventContent(emptyJsonDict, EventType.DUMMY, userIds)
}.onFailure {
Timber.e("## CRYPTO | encryptEventContent() failed.")
}
(alg as? IMXGroupEncryption)?.preshareKey(userIds)
}.fold(
{ callback.onSuccess(Unit) },
{
Timber.e("## CRYPTO | prepareToEncrypt() failed.")
callback.onFailure(it)
}
)
}
}

View file

@ -105,7 +105,7 @@ internal class EventDecryptor @Inject constructor(
try {
return alg.decryptEvent(event, timeline)
} catch (mxCryptoError: MXCryptoError) {
Timber.d("## CRYPTO | internalDecryptEvent : Failed to decrypt ${event.eventId} reason: $mxCryptoError")
Timber.v("## CRYPTO | internalDecryptEvent : Failed to decrypt ${event.eventId} reason: $mxCryptoError")
if (algorithm == MXCRYPTO_ALGORITHM_OLM) {
if (mxCryptoError is MXCryptoError.Base
&& mxCryptoError.errorType == MXCryptoError.ErrorType.BAD_ENCRYPTED_MESSAGE) {

View file

@ -32,6 +32,8 @@ internal interface IMXGroupEncryption {
*/
fun discardSessionKey()
suspend fun preshareKey(userIds: List<String>)
/**
* Re-shares a session key with devices if the key has already been
* sent to them.

View file

@ -94,6 +94,7 @@ internal class MXMegolmEncryption(
// annoyingly we have to serialize again the saved outbound session to store message index :/
// if not we would see duplicate message index errors
olmDevice.storeOutboundGroupSessionForRoom(roomId, outboundSession.sessionId)
Timber.v("## CRYPTO | encryptEventContent: Finished in ${System.currentTimeMillis() - ts} millis")
}
}
@ -118,6 +119,16 @@ internal class MXMegolmEncryption(
olmDevice.discardOutboundGroupSessionForRoom(roomId)
}
override suspend fun preshareKey(userIds: List<String>) {
val ts = System.currentTimeMillis()
Timber.v("## CRYPTO | preshareKey : getDevicesInRoom")
val devices = getDevicesInRoom(userIds)
val outboundSession = ensureOutboundSession(devices.allowedDevices)
notifyWithheldForSession(devices.withHeldDevices, outboundSession)
Timber.v("## CRYPTO | preshareKey ${System.currentTimeMillis() - ts} millis")
}
/**
* Prepare a new session.
*
@ -253,7 +264,7 @@ internal class MXMegolmEncryption(
continue
}
Timber.i("## CRYPTO | shareUserDevicesKey() : Sharing keys with device $userId:$deviceID")
Timber.i("## CRYPTO | shareUserDevicesKey() : Add to share keys contentMap for $userId:$deviceID")
contentMap.setObject(userId, deviceID, messageEncrypter.encryptMessage(payload, listOf(sessionResult.deviceInfo)))
haveTargets = true
}
@ -264,10 +275,12 @@ internal class MXMegolmEncryption(
// attempted to share with) rather than the contentMap (those we did
// share with), because we don't want to try to claim a one-time-key
// for dead devices on every message.
val gossipingEventBuffer = arrayListOf<Event>()
for ((userId, devicesToShareWith) in devicesByUser) {
for ((deviceId) in devicesToShareWith) {
session.sharedWithHelper.markedSessionAsShared(userId, deviceId, chainIndex)
cryptoStore.saveGossipingEvent(Event(
gossipingEventBuffer.add(
Event(
type = EventType.ROOM_KEY,
senderId = credentials.userId,
content = submap.apply {
@ -279,6 +292,8 @@ internal class MXMegolmEncryption(
}
}
cryptoStore.saveGossipingEvents(gossipingEventBuffer)
if (haveTargets) {
t0 = System.currentTimeMillis()
Timber.i("## CRYPTO | shareUserDevicesKey() ${session.sessionId} : has target")
@ -296,7 +311,7 @@ internal class MXMegolmEncryption(
}
private fun notifyKeyWithHeld(targets: List<UserDevice>, sessionId: String, senderKey: String?, code: WithHeldCode) {
Timber.i("## CRYPTO | notifyKeyWithHeld() :sending withheld key for $targets session:$sessionId ")
Timber.i("## CRYPTO | notifyKeyWithHeld() :sending withheld key for $targets session:$sessionId and code $code ")
val withHeldContent = RoomKeyWithHeldContent(
roomId = roomId,
senderKey = senderKey,

View file

@ -45,6 +45,7 @@ import org.matrix.android.sdk.internal.session.room.summary.RoomSummaryDataSourc
import org.matrix.android.sdk.internal.session.search.SearchTask
import org.matrix.android.sdk.internal.task.TaskExecutor
import org.matrix.android.sdk.internal.task.configureWith
import org.matrix.android.sdk.internal.util.awaitCallback
import java.security.InvalidParameterException
import javax.inject.Inject
@ -104,8 +105,10 @@ internal class DefaultRoom @Inject constructor(override val roomId: String,
return cryptoService.shouldEncryptForInvitedMembers(roomId)
}
override fun ensureOutboundSession() {
cryptoService.ensureOutboundSession(roomId)
override suspend fun prepareToEncrypt() {
awaitCallback<Unit> {
cryptoService.prepareToEncrypt(roomId, it)
}
}
override suspend fun enableEncryption(algorithm: String) {

View file

@ -104,4 +104,6 @@ sealed class RoomDetailAction : VectorViewModelAction {
// Preview URL
data class DoNotShowPreviewUrlFor(val eventId: String, val url: String) : RoomDetailAction()
data class ComposerFocusChange(val focused: Boolean) : RoomDetailAction()
}

View file

@ -70,6 +70,7 @@ import com.airbnb.mvrx.args
import com.airbnb.mvrx.fragmentViewModel
import com.airbnb.mvrx.withState
import com.google.android.material.snackbar.Snackbar
import com.jakewharton.rxbinding3.view.focusChanges
import com.jakewharton.rxbinding3.widget.textChanges
import com.vanniktech.emoji.EmojiPopup
import im.vector.app.R
@ -1144,6 +1145,12 @@ class RoomDetailFragment @Inject constructor(
roomDetailViewModel.handle(RoomDetailAction.UserIsTyping(it))
}
.disposeOnDestroyView()
views.composerLayout.views.composerEditText.focusChanges()
.subscribe {
roomDetailViewModel.handle(RoomDetailAction.ComposerFocusChange(it))
}
.disposeOnDestroyView()
}
private fun sendUri(uri: Uri): Boolean {

View file

@ -19,9 +19,13 @@ package im.vector.app.features.home.room.detail
import android.net.Uri
import androidx.annotation.IdRes
import androidx.lifecycle.viewModelScope
import com.airbnb.mvrx.Async
import com.airbnb.mvrx.Fail
import com.airbnb.mvrx.FragmentViewModelContext
import com.airbnb.mvrx.Loading
import com.airbnb.mvrx.MvRxViewModelFactory
import com.airbnb.mvrx.Success
import com.airbnb.mvrx.Uninitialized
import com.airbnb.mvrx.ViewModelContext
import com.jakewharton.rxrelay2.BehaviorRelay
import com.jakewharton.rxrelay2.PublishRelay
@ -37,6 +41,7 @@ import im.vector.app.features.call.dialpad.DialPadLookup
import im.vector.app.features.call.webrtc.WebRtcCallManager
import im.vector.app.features.command.CommandParser
import im.vector.app.features.command.ParsedCommand
import im.vector.app.features.crypto.keysrequest.OutboundSessionKeySharingStrategy
import im.vector.app.features.createdirect.DirectRoomHelper
import im.vector.app.features.crypto.verification.SupportedVerificationMethodsProvider
import im.vector.app.features.home.room.detail.composer.rainbow.RainbowGenerator
@ -61,7 +66,6 @@ import org.commonmark.renderer.html.HtmlRenderer
import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.MatrixPatterns
import org.matrix.android.sdk.api.NoOpMatrixCallback
import im.vector.app.features.crypto.keysrequest.OutboundSessionKeySharingStrategy
import org.matrix.android.sdk.api.extensions.tryOrNull
import org.matrix.android.sdk.api.query.QueryStringValue
import org.matrix.android.sdk.api.raw.RawService
@ -142,6 +146,8 @@ class RoomDetailViewModel @AssistedInject constructor(
private var trackUnreadMessages = AtomicBoolean(false)
private var mostRecentDisplayedEvent: TimelineEvent? = null
private var prepareToEncrypt: Async<Unit> = Uninitialized
@AssistedFactory
interface Factory {
fun create(initialState: RoomDetailViewState): RoomDetailViewModel
@ -184,7 +190,23 @@ class RoomDetailViewModel @AssistedInject constructor(
// Ensure to share the outbound session keys with all members
if (room.isEncrypted() && BuildConfig.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenEnteringRoom) {
room.ensureOutboundSession()
prepareForEncryption()
}
}
private fun prepareForEncryption() {
// check if there is not already a call made
if (prepareToEncrypt !is Loading) {
prepareToEncrypt = Loading()
viewModelScope.launch {
kotlin.runCatching {
room.prepareToEncrypt()
}.fold({
prepareToEncrypt = Success(Unit)
}, {
prepareToEncrypt = Fail(it)
})
}
}
}
@ -241,6 +263,7 @@ class RoomDetailViewModel @AssistedInject constructor(
override fun handle(action: RoomDetailAction) {
when (action) {
is RoomDetailAction.UserIsTyping -> handleUserIsTyping(action)
is RoomDetailAction.ComposerFocusChange -> handleComposerFocusChange(action)
is RoomDetailAction.SaveDraft -> handleSaveDraft(action)
is RoomDetailAction.SendMessage -> handleSendMessage(action)
is RoomDetailAction.SendMedia -> handleSendMedia(action)
@ -598,9 +621,17 @@ class RoomDetailViewModel @AssistedInject constructor(
room.userStopsTyping()
}
}
}
private fun handleComposerFocusChange(action: RoomDetailAction.ComposerFocusChange) {
// Ensure outbound session keys
if (action.isTyping && room.isEncrypted() && BuildConfig.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenTyping) {
room.ensureOutboundSession()
if (BuildConfig.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenTyping && room.isEncrypted()) {
if (action.focused) {
// Should we add some rate limit here, or do it only once per model lifecycle?
prepareForEncryption()
} else {
// we could eventually cancel here :/
}
}
}