Code review fixes.

This commit is contained in:
Onuray Sahin 2021-02-05 11:56:42 +03:00 committed by Benoit Marty
parent e92edc7cb1
commit a2de80091d
10 changed files with 119 additions and 96 deletions

View file

@ -53,6 +53,7 @@ import org.matrix.android.sdk.api.session.room.model.RoomMemberContent
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
import org.matrix.android.sdk.internal.crypto.algorithms.IMXGroupEncryption
import org.matrix.android.sdk.internal.crypto.algorithms.IMXWithHeldExtension
import org.matrix.android.sdk.internal.crypto.algorithms.megolm.MXMegolmEncryptionFactory
import org.matrix.android.sdk.internal.crypto.algorithms.olm.MXOlmEncryptionFactory
@ -667,7 +668,12 @@ internal class DefaultCryptoService @Inject constructor(
override fun discardOutboundSession(roomId: String) {
cryptoCoroutineScope.launch(coroutineDispatchers.crypto) {
roomEncryptorsStore.get(roomId)?.discardSessionKey()
val roomEncryptor = roomEncryptorsStore.get(roomId)
if (roomEncryptor is IMXGroupEncryption) {
roomEncryptor.discardSessionKey()
} else {
Timber.e("## CRYPTO | discardOutboundSession() for:$roomId: Unable to handle IMXGroupEncryption")
}
}
}
@ -1298,7 +1304,12 @@ internal class DefaultCryptoService @Inject constructor(
getEncryptionAlgorithm(roomId)?.let { safeAlgorithm ->
val userIds = getRoomUserIds(roomId)
if (setEncryptionInRoom(roomId, safeAlgorithm, false, userIds)) {
roomEncryptorsStore.get(roomId)?.ensureOutboundSession(getRoomUserIds(roomId))
val roomEncryptor = roomEncryptorsStore.get(roomId)
if (roomEncryptor is IMXGroupEncryption) {
roomEncryptor.ensureOutboundSession(getRoomUserIds(roomId))
} else {
Timber.e("## CRYPTO | ensureOutboundSession() for:$roomId: Unable to handle IMXGroupEncryption for $safeAlgorithm")
}
}
}
}

View file

@ -28,6 +28,7 @@ import org.matrix.android.sdk.api.session.crypto.keyshare.GossipingRequestListen
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.toModel
import org.matrix.android.sdk.internal.crypto.algorithms.IMXGroupEncryption
import org.matrix.android.sdk.internal.crypto.crosssigning.toBase64NoPadding
import org.matrix.android.sdk.internal.crypto.keysbackup.util.extractCurveKeyFromRecoveryKey
import org.matrix.android.sdk.internal.crypto.model.rest.GossipingDefaultContent
@ -290,12 +291,16 @@ internal class IncomingGossipingRequestManager @Inject constructor(
.also { cryptoStore.updateGossipingRequestState(request, GossipingRequestState.REJECTED) }
cryptoCoroutineScope.launch(coroutineDispatchers.crypto) {
val isSuccess = roomEncryptor.reshareKey(sessionId, userId, deviceId, senderKey)
if (roomEncryptor is IMXGroupEncryption) {
val isSuccess = roomEncryptor.reshareKey(sessionId, userId, deviceId, senderKey)
if (isSuccess) {
cryptoStore.updateGossipingRequestState(request, GossipingRequestState.ACCEPTED)
if (isSuccess) {
cryptoStore.updateGossipingRequestState(request, GossipingRequestState.ACCEPTED)
} else {
cryptoStore.updateGossipingRequestState(request, GossipingRequestState.UNABLE_TO_PROCESS)
}
} else {
cryptoStore.updateGossipingRequestState(request, GossipingRequestState.UNABLE_TO_PROCESS)
Timber.e("## CRYPTO | handleKeyRequestFromOtherUser() from:$userId: Unable to handle IMXGroupEncryption.reshareKey for $alg")
}
}
cryptoStore.updateGossipingRequestState(request, GossipingRequestState.RE_REQUESTED)

View file

@ -32,41 +32,4 @@ internal interface IMXEncrypting {
* @return the encrypted content
*/
suspend fun encryptEventContent(eventContent: Content, eventType: String, userIds: List<String>): Content
/**
* In Megolm, each recipient maintains a record of the ratchet value which allows
* them to decrypt any messages sent in the session after the corresponding point
* in the conversation. If this value is compromised, an attacker can similarly
* decrypt past messages which were encrypted by a key derived from the
* compromised or subsequent ratchet values. This gives 'partial' forward
* secrecy.
*
* To mitigate this issue, the application should offer the user the option to
* discard historical conversations, by winding forward any stored ratchet values,
* or discarding sessions altogether.
*/
fun discardSessionKey()
/**
* Re-shares a session key with devices if the key has already been
* sent to them.
*
* @param sessionId The id of the outbound session to share.
* @param userId The id of the user who owns the target device.
* @param deviceId The id of the target device.
* @param senderKey The key of the originating device for the session.
*
* @return true in case of success
*/
suspend fun reshareKey(sessionId: String,
userId: String,
deviceId: String,
senderKey: String): Boolean
/**
* Ensure the outbound session
*
* @param usersInRoom the users in the room
*/
suspend fun ensureOutboundSession(usersInRoom: List<String>)
}

View file

@ -0,0 +1,57 @@
/*
* 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.algorithms
internal interface IMXGroupEncryption {
/**
* In Megolm, each recipient maintains a record of the ratchet value which allows
* them to decrypt any messages sent in the session after the corresponding point
* in the conversation. If this value is compromised, an attacker can similarly
* decrypt past messages which were encrypted by a key derived from the
* compromised or subsequent ratchet values. This gives 'partial' forward
* secrecy.
*
* To mitigate this issue, the application should offer the user the option to
* discard historical conversations, by winding forward any stored ratchet values,
* or discarding sessions altogether.
*/
fun discardSessionKey()
/**
* Re-shares a session key with devices if the key has already been
* sent to them.
*
* @param sessionId The id of the outbound session to share.
* @param userId The id of the user who owns the target device.
* @param deviceId The id of the target device.
* @param senderKey The key of the originating device for the session.
*
* @return true in case of success
*/
suspend fun reshareKey(sessionId: String,
userId: String,
deviceId: String,
senderKey: String): Boolean
/**
* Ensure the outbound session
*
* @param usersInRoom the users in the room
*/
suspend fun ensureOutboundSession(usersInRoom: List<String>)
}

View file

@ -30,6 +30,7 @@ import org.matrix.android.sdk.internal.crypto.MXOlmDevice
import org.matrix.android.sdk.internal.crypto.actions.EnsureOlmSessionsForDevicesAction
import org.matrix.android.sdk.internal.crypto.actions.MessageEncrypter
import org.matrix.android.sdk.internal.crypto.algorithms.IMXEncrypting
import org.matrix.android.sdk.internal.crypto.algorithms.IMXGroupEncryption
import org.matrix.android.sdk.internal.crypto.keysbackup.DefaultKeysBackupService
import org.matrix.android.sdk.internal.crypto.model.CryptoDeviceInfo
import org.matrix.android.sdk.internal.crypto.model.MXUsersDevicesMap
@ -61,7 +62,7 @@ internal class MXMegolmEncryption(
private val taskExecutor: TaskExecutor,
private val coroutineDispatchers: MatrixCoroutineDispatchers,
private val cryptoCoroutineScope: CoroutineScope
) : IMXEncrypting {
) : IMXEncrypting, IMXGroupEncryption {
// OutboundSessionInfo. Null if we haven't yet started setting one up. Note
// that even if this is non-null, it may not be ready for use (in which

View file

@ -76,17 +76,4 @@ internal class MXOlmEncryption(
deviceListManager.downloadKeys(users, false)
ensureOlmSessionsForUsersAction.handle(users)
}
override fun discardSessionKey() {
// No need for olm
}
override suspend fun reshareKey(sessionId: String, userId: String, deviceId: String, senderKey: String): Boolean {
// No need for olm
return false
}
override suspend fun ensureOutboundSession(usersInRoom: List<String>) {
// NOOP
}
}

View file

@ -20,6 +20,11 @@ import androidx.lifecycle.LiveData
import androidx.lifecycle.Transformations
import com.zhuinden.monarchy.Monarchy
import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.MatrixConfiguration
import org.matrix.android.sdk.api.NoOpMatrixCallback
import org.matrix.android.sdk.api.crypto.OutboundSessionKeySharingStrategy
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.session.crypto.CryptoService
import org.matrix.android.sdk.api.session.events.model.Event
import org.matrix.android.sdk.api.session.room.Room
import org.matrix.android.sdk.api.session.room.RoomService
@ -39,6 +44,7 @@ import org.matrix.android.sdk.internal.session.room.alias.DeleteRoomAliasTask
import org.matrix.android.sdk.internal.session.room.alias.GetRoomIdByAliasTask
import org.matrix.android.sdk.internal.session.room.alias.RoomAliasDescription
import org.matrix.android.sdk.internal.session.room.create.CreateRoomTask
import org.matrix.android.sdk.internal.session.room.membership.LoadRoomMembersTask
import org.matrix.android.sdk.internal.session.room.membership.RoomChangeMembershipStateDataSource
import org.matrix.android.sdk.internal.session.room.membership.RoomMemberHelper
import org.matrix.android.sdk.internal.session.room.membership.joining.JoinRoomTask
@ -65,7 +71,10 @@ internal class DefaultRoomService @Inject constructor(
private val roomGetter: RoomGetter,
private val roomSummaryDataSource: RoomSummaryDataSource,
private val roomChangeMembershipStateDataSource: RoomChangeMembershipStateDataSource,
private val taskExecutor: TaskExecutor
private val taskExecutor: TaskExecutor,
private val loadRoomMembersTask: LoadRoomMembersTask,
private val matrixConfiguration: MatrixConfiguration,
private val cryptoService: CryptoService
) : RoomService {
override fun createRoom(createRoomParams: CreateRoomParams, callback: MatrixCallback<String>): Cancelable {
@ -105,6 +114,18 @@ internal class DefaultRoomService @Inject constructor(
}
override fun onRoomDisplayed(roomId: String): Cancelable {
// Ensure to load all room members
loadRoomMembersTask
.configureWith(LoadRoomMembersTask.Params(roomId)) {
this.callback = NoOpMatrixCallback()
}
.executeBy(taskExecutor)
// Ensure to share the outbound session keys with all members
if (roomGetter.getRoom(roomId)?.isEncrypted().orFalse()
&& matrixConfiguration.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenEnteringRoom) {
cryptoService.ensureOutboundSession(roomId)
}
return updateBreadcrumbsTask
.configureWith(UpdateBreadcrumbsTask.Params(roomId))
.executeBy(taskExecutor)

View file

@ -24,13 +24,8 @@ import io.realm.RealmQuery
import io.realm.RealmResults
import io.realm.Sort
import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.MatrixConfiguration
import org.matrix.android.sdk.api.NoOpMatrixCallback
import org.matrix.android.sdk.api.crypto.OutboundSessionKeySharingStrategy
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.extensions.tryOrNull
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.crypto.CryptoService
import org.matrix.android.sdk.api.session.events.model.EventType
import org.matrix.android.sdk.api.session.events.model.RelationType
import org.matrix.android.sdk.api.session.events.model.toModel
@ -54,7 +49,6 @@ import org.matrix.android.sdk.internal.database.query.filterEvents
import org.matrix.android.sdk.internal.database.query.findAllInRoomWithSendStates
import org.matrix.android.sdk.internal.database.query.where
import org.matrix.android.sdk.internal.database.query.whereRoomId
import org.matrix.android.sdk.internal.session.room.membership.LoadRoomMembersTask
import org.matrix.android.sdk.internal.task.TaskExecutor
import org.matrix.android.sdk.internal.task.configureWith
import org.matrix.android.sdk.internal.util.Debouncer
@ -83,11 +77,7 @@ internal class DefaultTimeline(
private val hiddenReadReceipts: TimelineHiddenReadReceipts,
private val timelineInput: TimelineInput,
private val eventDecryptor: TimelineEventDecryptor,
private val realmSessionProvider: RealmSessionProvider,
private val loadRoomMembersTask: LoadRoomMembersTask,
private val session: Session,
private val matrixConfiguration: MatrixConfiguration,
private val cryptoService: CryptoService
private val realmSessionProvider: RealmSessionProvider
) : Timeline,
TimelineHiddenReadReceipts.Delegate,
TimelineInput.Listener {
@ -189,17 +179,6 @@ internal class DefaultTimeline(
hiddenReadReceipts.start(realm, filteredEvents, nonFilteredEvents, this)
}
loadRoomMembersTask
.configureWith(LoadRoomMembersTask.Params(roomId)) {
this.callback = NoOpMatrixCallback()
}
.executeBy(taskExecutor)
if (session.getRoom(roomId)?.isEncrypted().orFalse()
&& matrixConfiguration.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenEnteringRoom) {
cryptoService.ensureOutboundSession(roomId)
}
isReady.set(true)
}
}

View file

@ -21,8 +21,13 @@ import dagger.assisted.Assisted
import dagger.assisted.AssistedInject
import dagger.assisted.AssistedFactory
import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.MatrixConfiguration
import org.matrix.android.sdk.api.crypto.OutboundSessionKeySharingStrategy
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.session.crypto.CryptoService
import org.matrix.android.sdk.api.session.room.typing.TypingService
import org.matrix.android.sdk.api.util.Cancelable
import org.matrix.android.sdk.internal.session.room.RoomGetter
import org.matrix.android.sdk.internal.task.TaskExecutor
import org.matrix.android.sdk.internal.task.configureWith
import timber.log.Timber
@ -36,7 +41,10 @@ import timber.log.Timber
internal class DefaultTypingService @AssistedInject constructor(
@Assisted private val roomId: String,
private val taskExecutor: TaskExecutor,
private val sendTypingTask: SendTypingTask
private val sendTypingTask: SendTypingTask,
private val matrixConfiguration: MatrixConfiguration,
private val cryptoService: CryptoService,
private val roomGetter: RoomGetter
) : TypingService {
@AssistedFactory
@ -69,6 +77,11 @@ internal class DefaultTypingService @AssistedInject constructor(
currentTask?.cancel()
if (roomGetter.getRoom(roomId)?.isEncrypted().orFalse()
&& matrixConfiguration.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenTyping) {
cryptoService.ensureOutboundSession(roomId)
}
val params = SendTypingTask.Params(roomId, true)
currentTask = sendTypingTask
.configureWith(params)

View file

@ -21,11 +21,6 @@ import org.matrix.android.sdk.internal.network.executeRequest
import org.matrix.android.sdk.internal.session.room.RoomAPI
import org.matrix.android.sdk.internal.task.Task
import kotlinx.coroutines.delay
import org.matrix.android.sdk.api.MatrixConfiguration
import org.matrix.android.sdk.api.crypto.OutboundSessionKeySharingStrategy
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.crypto.CryptoService
import org.matrix.android.sdk.internal.network.GlobalErrorReceiver
import javax.inject.Inject
@ -43,21 +38,12 @@ internal interface SendTypingTask : Task<SendTypingTask.Params, Unit> {
internal class DefaultSendTypingTask @Inject constructor(
private val roomAPI: RoomAPI,
@UserId private val userId: String,
private val globalErrorReceiver: GlobalErrorReceiver,
private val matrixConfiguration: MatrixConfiguration,
private val session: Session,
private val cryptoService: CryptoService
private val globalErrorReceiver: GlobalErrorReceiver
) : SendTypingTask {
override suspend fun execute(params: SendTypingTask.Params) {
delay(params.delay ?: -1)
if (params.isTyping
&& session.getRoom(params.roomId)?.isEncrypted().orFalse()
&& matrixConfiguration.outboundSessionKeySharingStrategy == OutboundSessionKeySharingStrategy.WhenTyping) {
cryptoService.ensureOutboundSession(params.roomId)
}
executeRequest<Unit>(globalErrorReceiver) {
apiCall = roomAPI.sendTypingState(
params.roomId,