Updating encrypted room list message previews on key updates #5080

ouchadam · 2022-01-27T12:01:04Z

Fixes #4867 - Message previews not decrypting upon receiving crypto keys

Follows the example of the timeline and adds a session.cryptoService().addNewSessionListener which refreshes the RoomSummary.LatestPreviewContent whenever we get a new key for the given room, the decryption will only be attempted if the current preview content doesn't contain an already decrypted json

Timeline example

https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt#L159
https://github.com/vector-im/element-android/blob/develop/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt#L121
https://github.com/vector-im/element-android/blob/develop/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt#L235
https://github.com/vector-im/element-android/blob/develop/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/LoadTimelineStrategy.kt#L143
https://github.com/vector-im/element-android/blob/develop/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/TimelineEventDecryptor.kt#L67

Also removes the onNewSession function from the base IMXDecrypting interface as it's only used for Megolm sessions and allows us to better ensure a room id is available

*before shows a new message coming in at the end, not the original message being decrypted

BEFORE	AFTER

ouchadam · 2022-01-27T12:03:49Z

...sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/DefaultRoomService.kt

@@ -90,6 +94,23 @@ internal class DefaultRoomService @Inject constructor(
        return roomSummaryDataSource.getRoomSummaries(queryParams, sortOrder)
    }

+    override fun refreshJoinedRoomSummaryPreviews(roomId: String?) {
+        val roomSummaries = getRoomSummaries(roomSummaryQueryParams {


due to nullability in the deeper crypto layers the room id isn't always guaranteed, I wasn't sure if we should skip those cases or attempt to refresh the entire room list

github-actions · 2022-01-27T12:39:32Z

Unit Test Results

110 files 110 suites 1m 27s ⏱️
195 tests 195 ✔️ 0 💤 0 ❌
650 runs 650 ✔️ 0 💤 0 ❌

Results for commit 57bf044.

♻️ This comment has been updated with latest results.

github-actions · 2022-01-27T13:09:09Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed="21" failures="0" errors="0" skipped="2"
[org.matrix.android.sdk.account]
passed="5" failures="0" errors="0" skipped="2"
[org.matrix.android.sdk.internal]
passed="23" failures="1" errors="0" skipped="3"
[org.matrix.android.sdk.ordering]
passed="16" failures="0" errors="0" skipped="0"
[org.matrix.android.sdk.PermalinkParserTest]
passed="1" failures="1" errors="0" skipped="0"

bmarty

Nice fix!
Some though about it.

bmarty · 2022-01-27T14:30:33Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/RoomService.kt

+     *
+     * This is useful for refreshing summary content with encrypted messages after receiving new room keys
+     */
+    fun refreshJoinedRoomSummaryPreviews(roomId: String?)


According to your video and following the implementation, the changes LGTM.
But I am wondering why we have to expose such API to the app. Can't this new decryption attempt be managed only SDK side? Since the SDK maintain the latestPreviewableEvent in the RoomSummary to me it would make more sense.

moving to the sdk also makes sense to me, the tricky part is where~

in the timeline it's managed by the SDK DefaultTimeline abstraction which acts as the datasource and holder for the listeners to allow the SDK to manage the crypto/pagination callbacks, from the client ViewModel we simply call timeline.start()

currently the room summaries is just a data model which clients observe using RoomService.getRoomSummariesLive, we could add a start/stop to the RoomService to internally register for crypto callbacks or create another abstraction on top of the summaries similar to Timeline 🤔 what do you think?

using the FlowSession abstraction we can wrap and combine the room and crypto services

fun liveRoomSummaries(queryParams: RoomSummaryQueryParams, sortOrder: RoomSortOrder = RoomSortOrder.NONE): Flow<List<RoomSummary>> { val refreshRoomSummariesOnCryptoSessionChange = object : NewSessionListener { override fun onNewSession(roomId: String?, senderKey: String, sessionId: String) { session.refreshJoinedRoomSummaryPreviews(roomId) } } return session.getRoomSummariesLive(queryParams, sortOrder).asFlow() .startWith(session.coroutineDispatchers.io) { session.getRoomSummaries(queryParams, sortOrder) } .onStart { session.cryptoService().addNewSessionListener(refreshRoomSummariesOnCryptoSessionChange) } .onCompletion { session.cryptoService().removeSessionListener(refreshRoomSummariesOnCryptoSessionChange) } }

with the current keys being updated in background workers it makes it very difficult (and potentially easier to introduce memory leaks) to attach a key listener in the lower levels of the sync handler or dynamically spawning them when calling the room summary update

conceptually I like the idea of an abstraction on top of the Session api (similar to the SessionFlow) which would interact and coordinate with the separate services to provide a source of truth

bmarty · 2022-01-27T14:33:55Z

vector/src/main/java/im/vector/app/features/home/HomeDetailViewModel.kt

@@ -149,6 +157,7 @@ class HomeDetailViewModel @AssistedInject constructor(
    override fun onCleared() {
        super.onCleared()
        callManager.removeProtocolsCheckerListener(this)
+        session.cryptoService().removeSessionListener(refreshRoomSummariesOnCryptoSessionChange)


This is the only change app side and it's clear that we listen for callback from the SDK to trigger a request to the SDK. So I think it confirms that it could be managed only by the SDK.

Yeah I think it makes sense to keep it in the SDK too (my initial thought was to do it in app too, but it probably should be in the SDK).

I was thinking of letting the RoomSummary having an instance of TimelineEventDecryptor.
Then for every undecrypted lastMessage, we just request the TimelineEventDecryptor to decrypt it, it will then by itself listen to session and retry to decrypt.

Two points to check:

When the TimelineEventDecryptor succesfully decrypt an event (and will update EventEntity), would this trigger an update to the LiveRoomSummaries?

We have to encure that it's the same TimelineEventDecryptor that is used during the session lifetime (if not it won't remember all the event to decrypt). It should belong some how to the session

Does it make sense?

the TimelineEventDecrypter is currently tied to the RoomDetailsActivity via the Timeline (and ViewModel), we could potentially lift it out and share with the RoomSummaryUpdater but we would need to register/unregister the key watching somewhere~

When the TimelineEventDecryptor succesfully decrypt an event (and will update EventEntity), would this trigger an update to the LiveRoomSummaries?

as far as I'm aware yes (this PR currently only updates the preview part of the entity at gets the live updates)

BillCarsonFr · 2022-01-27T15:06:16Z

...id/src/main/java/org/matrix/android/sdk/internal/crypto/actions/MegolmSessionDataImporter.kt

@@ -76,7 +77,11 @@ internal class MegolmSessionDataImporter @Inject constructor(private val olmDevi
                    outgoingGossipingRequestManager.cancelRoomKeyRequest(roomKeyRequestBody)

                    // Have another go at decrypting events sent with this session
-                    decrypting.onNewSession(megolmSessionData.senderKey!!, sessionId!!)
+                    when (decrypting) {


For things like that I used the extension pattern. For example there is the IMXWithHeldExtension that is only supported by MXMegolmDecryption and not by MXOlmDecryption. So in code you can do

if (alg is IMXWithHeldExtension) { alg.onRoomKeyWithHeldEvent(withHeldContent) }

Maybe we can use also this principles for new session? Or let me know if you think it's too overengineered

I think that could be quite nice as we're not using any of the other functions from the MXMegolmDecryption

maybe a bit of a hot take but I'm also wondering if we need the base interface? it seems that only fun decryptEvent(event: Event, timeline: String): MXEventDecryptionResult is used by both olm and megolm

when talking to the roomDecryptorProvider.getOrCreateRoomDecryptor we tend to already know which type of decrypter we need, in the case of MegolmSessionDataImporter I wouldn't expect to see any olm decrypters popping up

Florian14 · 2022-01-27T15:17:23Z

...src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/megolm/MXMegolmDecryption.kt

@@ -327,9 +327,9 @@ internal class MXMegolmDecryption(private val userId: String,
     * @param senderKey the session sender key
     * @param sessionId the session id
     */
-    override fun onNewSession(senderKey: String, sessionId: String) {
+    fun onNewSession(roomId: String?, senderKey: String, sessionId: String) {


Please, can you update the documentation about the new param? Also, it would be great if we could have some documentation about the NewSessionListener interface (which would probably be the same as this method) :)

will do 👍 If my understanding is correct onNewSession is triggered when we create a new megolm session for a given room, either by importing existing keys from cross signing/manual import or when receiving a toDevice room key share event

The change is to chain the existing room id from the key payload so that we can use it further down stream as a way know which rooms to attempt to re-decrypt

Thanks for the explanation, the link between the megolm session and the room was unclear to me!

docs added! c679890

Thanks for the docs update! It would be perfect if you could also add the new param in the doc of that function to be up to date with your changes

ah my bad, I thought you meant the interface, will update this method as well 👍

My comment was for both, the interface and the current method which already had documentation :)

updated both and provided context on why the value may be null 57bf044

BillCarsonFr

After rebase I am ok to merge

ouchadam · 2022-03-31T14:27:16Z

@BillCarsonFr thanks for taking a look, will update! hopefully something the rust sdk will provide for us for free in the future 🤞

…levant for Megolm sessions

- matches the same flow as the timeline by starting observing in the ViewModel init

ouchadam · 2022-04-01T10:32:06Z

Still working after the rebase 🙌

GIF AFTER REBASE

Florian14

Just a small remark, SGTM otherwise 💯

Florian14 · 2022-04-01T14:45:10Z

...src/main/java/org/matrix/android/sdk/internal/crypto/algorithms/megolm/MXMegolmDecryption.kt

@@ -327,9 +327,9 @@ internal class MXMegolmDecryption(private val userId: String,
     * @param senderKey the session sender key
     * @param sessionId the session id
     */
-    override fun onNewSession(senderKey: String, sessionId: String) {
+    fun onNewSession(roomId: String?, senderKey: String, sessionId: String) {


Thanks for the docs update! It would be perfect if you could also add the new param in the doc of that function to be up to date with your changes

bmarty

LGTM, but still a bit disappointed that we have to do changes app side. It will not be easy for other application using the SDK to do the same thing.
Since it fixes the issue, I do not want to block merging this PR.

ouchadam · 2022-04-01T16:00:42Z

@bmarty created an SDK issue to track moving the logic #5678

ouchadam commented Jan 27, 2022

View reviewed changes

bmarty reviewed Jan 27, 2022

View reviewed changes

bmarty requested a review from BillCarsonFr January 27, 2022 14:34

BillCarsonFr reviewed Jan 27, 2022

View reviewed changes

Florian14 reviewed Jan 27, 2022

View reviewed changes

BillCarsonFr approved these changes Mar 31, 2022

View reviewed changes

ouchadam added 4 commits March 31, 2022 17:13

removing noop newSession from base decryption interface, it's only re…

4629353

…levant for Megolm sessions

refreshing the room summaries when new crypto sessions are recieved

ced621d

- matches the same flow as the timeline by starting observing in the ViewModel init

adding changelog entry

1d0a87b

removing unused import

eb72587

ouchadam force-pushed the feature/adm/encrypted-last-message branch from 683a551 to eb72587 Compare March 31, 2022 16:24

ouchadam requested review from bmarty and Florian14 April 1, 2022 10:32

Florian14 approved these changes Apr 1, 2022

View reviewed changes

bmarty approved these changes Apr 1, 2022

View reviewed changes

adding documentation to the new session listener

57bf044

ouchadam force-pushed the feature/adm/encrypted-last-message branch from c679890 to 57bf044 Compare April 1, 2022 15:57

ouchadam mentioned this pull request Apr 1, 2022

Handle last message refreshing after verification within the SDK #5678

Open

ouchadam merged commit 3aac59a into develop Apr 1, 2022

ouchadam deleted the feature/adm/encrypted-last-message branch April 1, 2022 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating encrypted room list message previews on key updates #5080

Updating encrypted room list message previews on key updates #5080

ouchadam commented Jan 27, 2022 •

edited

Loading

ouchadam Jan 27, 2022

github-actions bot commented Jan 27, 2022 •

edited

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

bmarty left a comment

bmarty Jan 27, 2022

ouchadam Jan 27, 2022

ouchadam Jan 31, 2022

bmarty Jan 27, 2022

BillCarsonFr Jan 27, 2022

ouchadam Jan 27, 2022 •

edited

Loading

BillCarsonFr Jan 27, 2022

ouchadam Jan 27, 2022

Florian14 Jan 27, 2022 •

edited

Loading

ouchadam Jan 27, 2022

Florian14 Jan 27, 2022

ouchadam Mar 31, 2022

Florian14 Apr 1, 2022

ouchadam Apr 1, 2022

Florian14 Apr 1, 2022

ouchadam Apr 1, 2022

BillCarsonFr left a comment

ouchadam commented Mar 31, 2022

ouchadam commented Apr 1, 2022

Florian14 left a comment

Florian14 Apr 1, 2022

bmarty left a comment

ouchadam commented Apr 1, 2022

Updating encrypted room list message previews on key updates #5080

Updating encrypted room list message previews on key updates #5080

Conversation

ouchadam commented Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 27, 2022 • edited Loading

Unit Test Results

github-actions bot commented Jan 27, 2022 • edited Loading

Matrix SDK

Integration Tests Results:

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Florian14 Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr left a comment

Choose a reason for hiding this comment

ouchadam commented Mar 31, 2022

ouchadam commented Apr 1, 2022

Florian14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ouchadam commented Apr 1, 2022

ouchadam commented Jan 27, 2022 •

edited

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

ouchadam Jan 27, 2022 •

edited

Loading

Florian14 Jan 27, 2022 •

edited

Loading