crypto: Provide EncryptionState to hold reasons why events were UTD

[andybalaam]

Part of #4048

Preparation for surfacing the reason why messages were unable to decrypt: this change is intended to have no effect yet, but introduces an EncryptionState type that we will begin to populate in later PRs.

QUESTION: this will break deserialising existing TimelineEvents and SyncTimelineEvents. I think we don't persist them, so this is fine. Right?

[codecov]

Codecov Report

Attention: Patch coverage is 84.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.54%. Comparing base (866b6e5) to head (7613f2c).
Report is 61 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-crypto/src/machine/mod.rs	66.66%	2 Missing ⚠️
...es/matrix-sdk-common/src/deserialized_responses.rs	85.71%	1 Missing ⚠️
crates/matrix-sdk/src/event_handler/mod.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4053      +/-   ##
==========================================
- Coverage   84.55%   84.54%   -0.01%     
==========================================
  Files         266      266              
  Lines       28449    28462      +13     
==========================================
+ Hits        24054    24064      +10     
- Misses       4395     4398       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[richvdh]

I'll make a couple of tweaks to this PR

[richvdh]

We should probably use the existing UnableToDecryptInfo here (and add a "reason" to it).

[richvdh]

Having poked at this for a while, I'm not convinced it's the correct approach; or if it is, we need to think more carefully about how it fits into the existing implementation. A number of concerns arise:

• It's perfectly valid to have a TimelineEvent or a SyncTimelineEvent which we haven't yet attempted to decrypt. We need a way to represent this, and setting encryption_state to Unencrypted seems wrong to me. One solution would, I suppose, be to add a fourth potential EncryptionState (of NotYetDecrypted or somesuch). Not so, see below.
• matrix_sdk_crypto::OlmMachine::decrypt_room_event currently returns a MegolmResult<TimelineEvent>, set to Err if there is a decryption error. So if we allow TimelineEvents to have EncryptionState::UnableToDecrypt, this presents a rather confusing API. Should we change decrypt_room_event to return Ok in the case of UTD errors? Seems a slightly terrifying breaking change.
• More broadly it seems a bit regrettable that we're changing things in the common and crypto layers for functionality that feels like it ought to be in the higher layers. On the other hand, changing the higher layers to use something other than TimelineEvent or SyncTimelineEvent to pass around decryption results seems like it might be quite invasive.
• It turns out that, in the ultimate design, "decryption error" and "successful decryption" aren't quite as exclusive as you might think: in Invisible crypto: display message content instead of an error, when the sender's verified identity changed element-hq/element-meta#2523, we show the message content, alongside a big red error indicating you shouldn't actually trust that content. That's descoped for now, but it may be worth bearing in mind. Has to be said, I'm not entirely sure what a good implementation would look like.

[BillCarsonFr]

• It's perfectly valid to have a TimelineEvent or a SyncTimelineEvent which we haven't yet attempted to decrypt. We need a way to represent this, and setting encryption_state to Unencrypted seems wrong to me. One solution would, I suppose, be to add a fourth potential EncryptionState (of NotYetDecrypted or somesuch)

FWIW, from the discussion I had, this is not happening in the rust sdk. Any event from any sources gets decrypted (at least attempt)

[bnjbvr]

Responding to the preliminary question before reviewing anything:

QUESTION: this will break deserialising existing TimelineEvents and SyncTimelineEvents. I think we don't persist them, so this is fine. Right?

If this is in the public API, other consumers of the SDK may serialize and deserialize it, so it's a breaking change in that case.

[richvdh]

• It's perfectly valid to have a TimelineEvent or a SyncTimelineEvent which we haven't yet attempted to decrypt. We need a way to represent this, and setting encryption_state to Unencrypted seems wrong to me. One solution would, I suppose, be to add a fourth potential EncryptionState (of NotYetDecrypted or somesuch)

FWIW, from the discussion I had, this is not happening in the rust sdk. Any event from any sources gets decrypted (at least attempt)

Eventually, yes. But before that happens, the event still exists as a TimelineEvent or SyncTimelineEvent.

[richvdh]

Eventually, yes. But before that happens, the event still exists as a TimelineEvent or SyncTimelineEvent.

This is wrong, it turns out: TimelineEvent (or SyncTimelineEvent) are not created until we have tried to decrypt any encrypted events. Sorry Valere!

[richvdh]

Having discussed this a bit with the rust team, the concern was raised that the types proposed allow for a conflict in state between encryption_state and event. (For example: what would it mean if encryption_state indicated a UTD, but event was a successful decryption?)

Instead: it was proposed to replace event and encryption_info in TimelineEvent and SyncTimelineEvent with a new enum:

enum TimelineFoo {
    Utd {
        event: Raw<EncryptedEvent>,
        cause: UtdCause,
    },
    PlainText {
        event: Raw<T>,
    }
    Decrypted {
        event: Raw<T>,
        info: EncryptionInfo,
    }
}

(better name needed, obvs)

We should also update OlmMachine::decrypt_room_event to return a TimelineEvent with TimelineFoo::Utd instead of an Err, on UTD errors.

[richvdh]

superceded by #4082