crypto: New return type for decrypt_room_event
#4064
Conversation
richvdh
force-pushed
the
rav/decryption_result
branch
from
c671494
to
472b96b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4064 +/- ##
==========================================
- Coverage 84.57% 84.56% -0.01%
==========================================
Files 267 267
Lines 28456 28461 +5
==========================================
+ Hits 24067 24069 +2
- Misses 4389 4392 +3 ☔ View full report in Codecov by Sentry. |
richvdh
force-pushed
the
rav/decryption_result
branch
from
472b96b
to
0f445fb
Compare
| @@ -423,6 +444,35 @@ impl fmt::Debug for TimelineEvent { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Clone, Serialize, Deserialize)] | |||
| /// A successfully-decrypted encrypted event. | |||
There was a problem hiding this comment.
The casual reader might wonder why this is here, rather than in the crypto crate. The answer is that, in the fullness of time, I want to make TimelineEvent and SyncTimelineEvent wrap a DecryptedRoomEvent rather than duplicating the fields.
richvdh
requested review from
jmartinesp and
BillCarsonFr
and removed request for
a team
| @@ -1818,14 +1818,9 @@ impl OlmMachine { | |||
| .await; | |||
| } | |||
|
|
|||
| let event = serde_json::from_value::<Raw<AnyTimelineEvent>>(decrypted_event.into())?; | |||
| let event = serde_json::from_value::<Raw<AnyMessageLikeEvent>>(decrypted_event.into())?; | |||
There was a problem hiding this comment.
I know state events aren't currently encrypted, but does the change from AnyTimelineEvent to AnyMessageLikeEvent here mean that we are assuming this is not a state event? Are we OK with that?
There was a problem hiding this comment.
Good question.
Yes I am assuming this is not a state event, and that our lack of encryption for state events is not going to change any time soon. It felt like using the more specific type might be more useful for callers. It's also worth noting that situation can't change without changes elsewhere in the crypto crate (specifically we construct the JSON for the decrypted event in InboundGroupSession::decrypt).
On the other hand: maybe it's better we encourage callers to be ready for state events by continuing to return Raw<AnyTimelineEvent>.
I'd be interested in @poljar's thoughts here.
There was a problem hiding this comment.
I really don't expect state events to be able to use the same mechanisms for encryption as we use for non-state events, perhaps I'm mistaken here but I would expect that they would be different enough to warrant a separate method.
There was a problem hiding this comment.
I would expect that they would be different enough to warrant a separate method.
I guess the point is that it's not just about a separate method: the intention is to embed DecryptedRoomEvent inside TimelineEvent and SyncTimelineEvent -- so we'll be hardcoding this knowledge there too.
That said: on the grand scale of changes that will be needed to support encrypted state events, I don't think changing this later is going to be that significant.
I want to do a bit of a refactoring on `TimelineEvent`, so let's start by giving `decrypt_room_event` its own return type.
richvdh
force-pushed
the
rav/decryption_result
branch
from
73724d3
to
95a2d1f
Compare
I need to do a bit of a refactoring on
TimelineEvent, so let's start by givingdecrypt_room_eventits own return type.(some groundwork for #4048)