-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add unable-to-decrypt timeline item support #1140
Conversation
// Do nothing for these, as they would not produce a new | ||
// timeline item when decrypted either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on this? Is this about an encrypted event, that we can't decrypt, being a replacement that is an edit.
The assumption here is that edits only update timeline items but don't produce new ones, right? If that's so, then this sounds correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I was wondering whether we should show a UTD item for an edit that can't be decrypted. this branch also includes "annotations", i.e. reactions which are currently never encrypted so maybe that variant shouldn't exist at all 🤔
@@ -258,6 +258,9 @@ pub enum TimelineItemContent { | |||
|
|||
/// A redacted message. | |||
RedactedMessage, | |||
|
|||
/// An `m.room.encrypted` event that could not be decrypted. | |||
UnableToDecrypt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know nothing about the content that we would like to "present" to SDK users here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we have like the sender at least? And some other metadata like the time it was sent? That's at least what element-web shows...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't know anything about the thing that's encrypted, if you mean the content type or some other metadata.
Though you will want to show quite a bit, you'll likely want to have some "request room key functionality", what springs to mind, besides the sender and timestamp, is:
- Algorithm that the event claims to use
- Session id
sender_key
and Device id if there is one
Hmm, that's almost the whole content of the event, perhaps it makes sense to leave the whole content in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnunicorn Sender and time are not part of the content, they are fields of the timeline item (one level up basically).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we store the raw event separately as well and probably need to deserialize to matrix-sdk-crypto
's RoomEncryptedEventContent
, I'd rather introduce a payload-free version of Ruma's RoomEncryptedEventContent
to use here. The encrypted payload isn't going to be very useful outside of show-event-source, if at all.
Codecov ReportBase: 73.09% // Head: 72.96% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1140 +/- ##
==========================================
- Coverage 73.09% 72.96% -0.13%
==========================================
Files 110 110
Lines 12302 12323 +21
==========================================
Hits 8992 8992
- Misses 3310 3331 +21
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Ready for another review. edit: Oh, UniFFI panic ._. |
744f537
to
237af1f
Compare
I don't know if it should be in this PR, or a separate item in #1103, but it would also be nice to retry to decrypt those items when new room keys are received. |
I think that can be a separate PR. |
237af1f
to
2e5f7cb
Compare
Now using UDL since the uniffi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this works fine for new timeline items, but it produces a kind of dichotomy for local echo and perhaps even updates.
I need to handle the case of a local echo being an UTD, this is obviously unreachable and it seems to be quite pointless to force every consumer of the lib to introduce unreachable()
calls.
I think the same applies to an update_at
signal, can you think of a scenario where we'll want to replace a decrypted event with an UTD?
Edits come to mind, where we can decrypt the original event, but can't decrypt the edit. I think in that case we'll want to leave the original there, remember the edit, and trigger an update if and when we're able to decrypt the edit.
I'll approve this but I think that long-term we'll want to remove the problem with local echo being able to be a UTD.
2e5f7cb
to
39937d2
Compare
Hm, why would you need special-case code to replace timeline item |
39937d2
to
4dca3a4
Compare
Alright, let's assume that replacements aren't problematic. Local echo does require different UI presentation, i.e. it used to be greyed out on Element as well. |
Right. I'll open an issue so we can potentially update the data model to make local utd's impossible. |
Part of #1103.