From 6ea38c7eb0c07a97a620fca85a0021237e30300e Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 30 Apr 2020 10:55:25 +0200 Subject: [PATCH 1/3] Fix / Move DM shield rules to task --- CHANGES.md | 1 + .../crypto/crosssigning/ComputeTrustTask.kt | 19 ++++++++++++++++--- .../SessionToCryptoRoomMembersUpdate.kt | 1 + .../crypto/crosssigning/ShieldTrustUpdater.kt | 7 ++++--- .../session/room/RoomSummaryUpdater.kt | 10 +--------- 5 files changed, 23 insertions(+), 15 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 08eafff2d3..2545872335 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -46,6 +46,7 @@ Bugfix 🐛: - Add user to direct chat by user id (#1065) - Use correct URL for SSO connection (#1178) - Emoji completion :tada: does not completes to 🎉 like on web (#1285) + - Fix bad Shield Logic for DM (#963) Translations 🗣: - diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ComputeTrustTask.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ComputeTrustTask.kt index 841de92130..d6b0c4a8d5 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ComputeTrustTask.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ComputeTrustTask.kt @@ -19,6 +19,7 @@ import im.vector.matrix.android.api.crypto.RoomEncryptionTrustLevel import im.vector.matrix.android.api.extensions.orFalse import im.vector.matrix.android.api.session.crypto.crosssigning.MXCrossSigningInfo import im.vector.matrix.android.internal.crypto.store.IMXCryptoStore +import im.vector.matrix.android.internal.di.UserId import im.vector.matrix.android.internal.task.Task import im.vector.matrix.android.internal.util.MatrixCoroutineDispatchers import kotlinx.coroutines.withContext @@ -26,17 +27,29 @@ import javax.inject.Inject internal interface ComputeTrustTask : Task { data class Params( - val userIds: List + val activeMemberUserIds: List, + val isDirectRoom: Boolean ) } internal class DefaultComputeTrustTask @Inject constructor( private val cryptoStore: IMXCryptoStore, + @UserId private val myUserId: String, private val coroutineDispatchers: MatrixCoroutineDispatchers ) : ComputeTrustTask { override suspend fun execute(params: ComputeTrustTask.Params): RoomEncryptionTrustLevel = withContext(coroutineDispatchers.crypto) { - val allTrustedUserIds = params.userIds + + // The set of “all users” depends on the type of room: + // For regular / topic rooms, all users including yourself, are considered when decorating a room + // For 1:1 and group DM rooms, all other users (i.e. excluding yourself) are considered when decorating a room + val listToCheck = if (params.isDirectRoom) { + params.activeMemberUserIds.filter { it != myUserId } + } else { + params.activeMemberUserIds + } + + val allTrustedUserIds = listToCheck .filter { userId -> getUserCrossSigningKeys(userId)?.isTrusted() == true } if (allTrustedUserIds.isEmpty()) { @@ -60,7 +73,7 @@ internal class DefaultComputeTrustTask @Inject constructor( if (hasWarning) { RoomEncryptionTrustLevel.Warning } else { - if (params.userIds.size == allTrustedUserIds.size) { + if (listToCheck.size == allTrustedUserIds.size) { // all users are trusted and all devices are verified RoomEncryptionTrustLevel.Trusted } else { diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/SessionToCryptoRoomMembersUpdate.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/SessionToCryptoRoomMembersUpdate.kt index 04f63f945a..2f1cb77a21 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/SessionToCryptoRoomMembersUpdate.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/SessionToCryptoRoomMembersUpdate.kt @@ -17,6 +17,7 @@ package im.vector.matrix.android.internal.crypto.crosssigning data class SessionToCryptoRoomMembersUpdate( val roomId: String, + val isDirect: Boolean, val userIds: List ) diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ShieldTrustUpdater.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ShieldTrustUpdater.kt index d7597f00c9..f1e387beb3 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ShieldTrustUpdater.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ShieldTrustUpdater.kt @@ -79,7 +79,7 @@ internal class ShieldTrustUpdater @Inject constructor( return } taskExecutor.executorScope.launch(BACKGROUND_HANDLER_DISPATCHER) { - val updatedTrust = computeTrustTask.execute(ComputeTrustTask.Params(update.userIds)) + val updatedTrust = computeTrustTask.execute(ComputeTrustTask.Params(update.userIds, update.isDirect)) // We need to send that back to session base backgroundSessionRealm.get()?.executeTransaction { realm -> roomSummaryUpdater.updateShieldTrust(realm, update.roomId, updatedTrust) @@ -109,8 +109,9 @@ internal class ShieldTrustUpdater @Inject constructor( if (roomSummary?.isEncrypted.orFalse()) { val allActiveRoomMembers = RoomMemberHelper(realm, roomId).getActiveRoomMemberIds() try { - // Can throw if the crypto database has been closed in between, in this case log and ignore? - val updatedTrust = computeTrustTask.execute(ComputeTrustTask.Params(allActiveRoomMembers)) + val updatedTrust = computeTrustTask.execute( + ComputeTrustTask.Params(allActiveRoomMembers, roomSummary?.isDirect == true) + ) realm.executeTransaction { roomSummaryUpdater.updateShieldTrust(it, roomId, updatedTrust) } diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/RoomSummaryUpdater.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/RoomSummaryUpdater.kt index 69c0877a40..7044b3b47d 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/RoomSummaryUpdater.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/RoomSummaryUpdater.kt @@ -161,15 +161,7 @@ internal class RoomSummaryUpdater @Inject constructor( roomSummaryEntity.otherMemberIds.clear() roomSummaryEntity.otherMemberIds.addAll(otherRoomMembers) if (roomSummaryEntity.isEncrypted) { - // The set of “all users” depends on the type of room: - // For regular / topic rooms, all users including yourself, are considered when decorating a room - // For 1:1 and group DM rooms, all other users (i.e. excluding yourself) are considered when decorating a room - val listToCheck = if (roomSummaryEntity.isDirect) { - roomSummaryEntity.otherMemberIds.toList() - } else { - roomSummaryEntity.otherMemberIds.toList() + userId - } - eventBus.post(SessionToCryptoRoomMembersUpdate(roomId, listToCheck)) + eventBus.post(SessionToCryptoRoomMembersUpdate(roomId, roomSummaryEntity.isDirect, roomSummaryEntity.otherMemberIds.toList() + userId)) } } } From 5840248ffaf3252f590a24727e2078e51af9c802 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 30 Apr 2020 11:11:11 +0200 Subject: [PATCH 2/3] Fix / NPE Optional#get instead of getOrNull --- .../devices/DeviceVerificationInfoBottomSheetViewModel.kt | 4 ++-- .../riotx/features/settings/devices/DevicesViewModel.kt | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/vector/src/main/java/im/vector/riotx/features/settings/devices/DeviceVerificationInfoBottomSheetViewModel.kt b/vector/src/main/java/im/vector/riotx/features/settings/devices/DeviceVerificationInfoBottomSheetViewModel.kt index 47b64df927..3ae95cd5a4 100644 --- a/vector/src/main/java/im/vector/riotx/features/settings/devices/DeviceVerificationInfoBottomSheetViewModel.kt +++ b/vector/src/main/java/im/vector/riotx/features/settings/devices/DeviceVerificationInfoBottomSheetViewModel.kt @@ -64,8 +64,8 @@ class DeviceVerificationInfoBottomSheetViewModel @AssistedInject constructor(@As session.rx().liveCrossSigningInfo(session.myUserId) .execute { copy( - hasAccountCrossSigning = it.invoke()?.get() != null, - accountCrossSigningIsTrusted = it.invoke()?.get()?.isTrusted() == true + hasAccountCrossSigning = it.invoke()?.getOrNull() != null, + accountCrossSigningIsTrusted = it.invoke()?.getOrNull()?.isTrusted() == true ) } diff --git a/vector/src/main/java/im/vector/riotx/features/settings/devices/DevicesViewModel.kt b/vector/src/main/java/im/vector/riotx/features/settings/devices/DevicesViewModel.kt index 560e6f396d..165231b7cd 100644 --- a/vector/src/main/java/im/vector/riotx/features/settings/devices/DevicesViewModel.kt +++ b/vector/src/main/java/im/vector/riotx/features/settings/devices/DevicesViewModel.kt @@ -93,8 +93,8 @@ class DevicesViewModel @AssistedInject constructor( session.rx().liveCrossSigningInfo(session.myUserId) .execute { copy( - hasAccountCrossSigning = it.invoke()?.get() != null, - accountCrossSigningIsTrusted = it.invoke()?.get()?.isTrusted() == true + hasAccountCrossSigning = it.invoke()?.getOrNull() != null, + accountCrossSigningIsTrusted = it.invoke()?.getOrNull()?.isTrusted() == true ) } From 05230a6afa58ef57ff5421a5b259bbfa1acfc7c3 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 30 Apr 2020 11:38:32 +0200 Subject: [PATCH 3/3] Code review --- .../android/internal/crypto/crosssigning/ComputeTrustTask.kt | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ComputeTrustTask.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ComputeTrustTask.kt index d6b0c4a8d5..121479ad66 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ComputeTrustTask.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/ComputeTrustTask.kt @@ -34,17 +34,16 @@ internal interface ComputeTrustTask : Task