Defensive coding to ensure encryption when room was once e2e #5136
Conversation
| * But the crypto layer has additional guaranty to ensure that encryption would never been reverted | ||
| * It's defensive coding out of precaution (if ever state is reset) | ||
| */ | ||
| fun shouldEncryptInRoom(roomId: String?): Boolean |
There was a problem hiding this comment.
If we have roomId as nullable here we should do the same for isRoomEncrypted I guess
There was a problem hiding this comment.
roomId in event are optional (but almost always there), all codes ends up having optional, and function call wrapped in a let with an elvis.
It's a bit annoying, I let the caller pass an optional and return false if null in the code. I find the ending code more readable
There was a problem hiding this comment.
Yeah I know...the fact is I think we shouldn't have nullable roomId in Event :/
Matrix SDKIntegration Tests Results:
|
| * But the crypto layer has additional guaranty to ensure that encryption would never been reverted | ||
| * It's defensive coding out of precaution (if ever state is reset) | ||
| */ | ||
| fun shouldEncryptInRoom(roomId: String?): Boolean |
There was a problem hiding this comment.
Do we really need to expose this to the application?
Using the CryptoStore may be enough internally
Also we can have isRoomEncrypted which return false and shouldEncryptInRoom which return true. How the UI should react in this case? Updating the UI will be maybe for another PR so this is the rational to expose that API?
There was a problem hiding this comment.
The source of trust is still the presence or not of a state event of type m.room.encryption, so the UI will always reflect that. For example UI will detect a missconfigured encryption.
The shouldEncrypt is more some internal protection, and should not be reflected on UI.
Also I don't expose it anymore
There was a problem hiding this comment.
It's still in the interface, can you remove it?
| @@ -303,7 +303,7 @@ internal class DefaultSendService @AssistedInject constructor( | |||
| private fun internalSendMedia(allLocalEchoes: List<Event>, attachment: ContentAttachmentData, compressBeforeSending: Boolean): Cancelable { | |||
| val cancelableBag = CancelableBag() | |||
|
|
|||
| allLocalEchoes.groupBy { cryptoSessionInfoProvider.isRoomEncrypted(it.roomId!!) } | |||
| allLocalEchoes.groupBy { cryptoStore.roomWasOnceEncrypted(it.roomId!!) } | |||
There was a problem hiding this comment.
We still have !! here, this is not ideal :/ but this is not Event coming from the wild so I think it's fine...
There was a problem hiding this comment.
Yes it's a private function, so acceptable :/
BillCarsonFr
force-pushed
the
feature/bca/crypto_ensure_e2e_defensive
branch
from
6424ec6 to
48fffc3
Compare
| import org.matrix.android.sdk.internal.crypto.store.db.model.CryptoRoomEntityFields | ||
| import org.matrix.android.sdk.internal.util.database.RealmMigrator | ||
|
|
||
| // Version 14L Update the way we remember key sharing |
There was a problem hiding this comment.
This comment need to be updated (or removed). I can delete it on develop.
Some defensive coding on the crypto side to ensure that a room that was once known locally has encrypted (with a valid algorithm) will still stays encrypted.
This is very defensive and resist to
state resetsvariationsIn the CryptoRoom entity we add a new boolean to remember if the room was once encrypted with a valid algorithm.
We still store the alg got from the state, in order to properly give feedback of misconfigured room