Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(timeline): update responses after a UTD has been decrypted #4210

Merged
merged 2 commits into from
Nov 4, 2024
+178 −58
Merged

fix(timeline): update responses after a UTD has been decrypted #4210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb19cc2
fix(timeline): update responses after a successful decryption
bnjbvr Nov 4, 2024
2665ea3
refactor(timeline): factor out in-reply-to updates
bnjbvr Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 45 additions & 41 deletions crates/matrix-sdk-ui/src/timeline/event_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,25 +547,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
let internal_id = item.internal_id.to_owned();

// Update all events that replied to this message with the edited content.
self.items.for_each(|mut entry| {
let Some(event_item) = entry.as_event() else { return };
let Some(message) = event_item.content.as_message() else { return };
let Some(in_reply_to) = message.in_reply_to() else { return };
if replacement.event_id == in_reply_to.event_id {
trace!(reply_event_id = ?event_item.identifier(), "Updating response to edited event");
let in_reply_to = InReplyToDetails {
event_id: in_reply_to.event_id.clone(),
event: TimelineDetails::Ready(Box::new(
RepliedToEvent::from_timeline_item(&new_item),
)),
};
let new_reply_content =
TimelineItemContent::Message(message.with_in_reply_to(in_reply_to));
let new_reply_item =
entry.with_kind(event_item.with_content(new_reply_content, None));
ObservableVectorTransactionEntry::set(&mut entry, new_reply_item);
}
});
Self::maybe_update_responses(self.items, &replacement.event_id, &new_item);

// Update the event itself.
self.items.set(item_pos, TimelineItem::new(new_item, internal_id));
Expand Down Expand Up @@ -945,7 +927,13 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
debug!("event item is already redacted");
} else {
let new_item = item.redact(&self.meta.room_version);
self.items.set(idx, TimelineItem::new(new_item, item.internal_id.to_owned()));
let internal_id = item.internal_id.to_owned();

// Look for any timeline event that's a reply to the redacted event, and redact
// the replied-to event there as well.
Self::maybe_update_responses(self.items, &redacted, &new_item);

self.items.set(idx, TimelineItem::new(new_item, internal_id));
self.result.items_updated += 1;
}
} else {
Expand All @@ -954,26 +942,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
} else {
debug!("Timeline item not found, discarding redaction");
};

// Look for any timeline event that's a reply to the redacted event, and redact
// the replied-to event there as well.
self.items.for_each(|mut entry| {
let Some(event_item) = entry.as_event() else { return };
let Some(message) = event_item.content.as_message() else { return };
let Some(in_reply_to) = message.in_reply_to() else { return };
let TimelineDetails::Ready(replied_to_event) = &in_reply_to.event else { return };
if redacted == in_reply_to.event_id {
let replied_to_event = replied_to_event.redact(&self.meta.room_version);
let in_reply_to = InReplyToDetails {
event_id: in_reply_to.event_id.clone(),
event: TimelineDetails::Ready(Box::new(replied_to_event)),
};
let content = TimelineItemContent::Message(message.with_in_reply_to(in_reply_to));
let new_item = entry.with_kind(event_item.with_content(content, None));

ObservableVectorTransactionEntry::set(&mut entry, new_item);
}
});
}

/// Attempts to redact a reaction, local or remote.
Expand Down Expand Up @@ -1190,8 +1158,16 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
}
}

Flow::Remote { position: TimelineItemPosition::Update(idx), .. } => {
Flow::Remote {
event_id: decrypted_event_id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really part of this PR but, oh boy, I feel like this could express that this is an UTD -> Decrypted transition a bit better.

Flow::Remote is generally for remote echoes and TimlineItemPosition::Update does not sound UTD specific at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to rename this Update; will do in a follow-up, thanks!

position: TimelineItemPosition::Update(idx),
..
} => {
trace!("Updating timeline item at position {idx}");

// Update all events that replied to this previously encrypted message.
Self::maybe_update_responses(self.items, decrypted_event_id, &item);

let internal_id = self.items[*idx].internal_id.clone();
self.items.set(*idx, TimelineItem::new(item, internal_id));
}
Expand All @@ -1203,6 +1179,34 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
}
}

/// After updating the timeline item `new_item` which id is
/// `target_event_id`, update other items that are responses to this item.
fn maybe_update_responses(
items: &mut ObservableVectorTransaction<'_, Arc<TimelineItem>>,
target_event_id: &EventId,
new_item: &EventTimelineItem,
) {
items.for_each(|mut entry| {
let Some(event_item) = entry.as_event() else { return };
let Some(message) = event_item.content.as_message() else { return };
let Some(in_reply_to) = message.in_reply_to() else { return };
if target_event_id == in_reply_to.event_id {
trace!(reply_event_id = ?event_item.identifier(), "Updating response to edited event");
let in_reply_to = InReplyToDetails {
event_id: in_reply_to.event_id.clone(),
event: TimelineDetails::Ready(Box::new(
RepliedToEvent::from_timeline_item(new_item),
)),
};
let new_reply_content =
TimelineItemContent::Message(message.with_in_reply_to(in_reply_to));
let new_reply_item =
entry.with_kind(event_item.with_content(new_reply_content, None));
ObservableVectorTransactionEntry::set(&mut entry, new_reply_item);
}
});
}

fn pending_reactions(
&mut self,
content: &TimelineItemContent,
Expand Down
10 changes: 1 addition & 9 deletions crates/matrix-sdk-ui/src/timeline/event_item/content/message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use ruma::{
},
html::RemoveReplyFallback,
serde::Raw,
OwnedEventId, OwnedUserId, RoomVersionId, UserId,
OwnedEventId, OwnedUserId, UserId,
};
use tracing::{error, trace};

Expand Down Expand Up @@ -337,14 +337,6 @@ impl RepliedToEvent {
}
}

pub(in crate::timeline) fn redact(&self, room_version: &RoomVersionId) -> Self {
Self {
content: self.content.redact(room_version),
sender: self.sender.clone(),
sender_profile: self.sender_profile.clone(),
}
}

/// Try to create a `RepliedToEvent` from a `TimelineEvent` by providing the
/// room.
pub async fn try_from_timeline_event_for_room(
Expand Down
130 changes: 127 additions & 3 deletions crates/matrix-sdk-ui/src/timeline/tests/encryption.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@ use std::{
sync::{Arc, Mutex},
};

use as_variant::as_variant;
use assert_matches::assert_matches;
use assert_matches2::assert_let;
use eyeball_im::VectorDiff;
use matrix_sdk::{
assert_next_matches_with_timeout,
crypto::{decrypt_room_key_export, types::events::UtdCause, OlmMachine},
test_utils::test_client_builder,
};
use matrix_sdk_base::deserialized_responses::{SyncTimelineEvent, UnableToDecryptReason};
use matrix_sdk_test::{async_test, BOB};
use matrix_sdk_test::{async_test, ALICE, BOB};
use ruma::{
assign,
assign, event_id,
events::room::encrypted::{
EncryptedEventScheme, MegolmV1AesSha2ContentInit, Relation, Replacement,
RoomEncryptedEventContent,
Expand All @@ -44,7 +46,7 @@ use stream_assert::assert_next_matches;

use super::TestTimeline;
use crate::{
timeline::{EncryptedMessage, TimelineItemContent},
timeline::{EncryptedMessage, TimelineDetails, TimelineItemContent},
unable_to_decrypt_hook::{UnableToDecryptHook, UnableToDecryptInfo, UtdHookManager},
};

Expand Down Expand Up @@ -557,6 +559,128 @@ async fn test_utd_cause_for_missing_membership_is_unknown() {
assert_eq!(*cause, UtdCause::Unknown);
}

#[async_test]
async fn test_retry_decryption_updates_response() {
const SESSION_ID: &str = "gM8i47Xhu0q52xLfgUXzanCMpLinoyVyH7R58cBuVBU";
const SESSION_KEY: &[u8] = b"\
-----BEGIN MEGOLM SESSION DATA-----\n\
ASKcWoiAVUM97482UAi83Avce62hSLce7i5JhsqoF6xeAAAACqt2Cg3nyJPRWTTMXxXH7TXnkfdlmBXbQtq5\
bpHo3LRijcq2Gc6TXilESCmJN14pIsfKRJrWjZ0squ/XsoTFytuVLWwkNaW3QF6obeg2IoVtJXLMPdw3b2vO\
vgwGY3OMP0XafH13j1vcb6YLzvgLkZQLnYvd47hv3yK/9GmKS9tokuaQ7dCVYckYcIOS09EDTs70YdxUd5WG\
rQynATCLFP1p/NAGv70r9MK7Cy/mNpjD0r4qC7UEDIoi1kOWzHgnLo19wtvwsb8Fg8ATxcs3Wmtj8hIUYpDx\
ia4sM10zbytUuaPUAfCDf42IyxdmOnGe1CueXhgI71y+RW0s0argNqUt7jB70JT0o9CyX6UBGRaqLk2MPY9T\
hUu5J8X3UgIa6rcbWigzohzWm9rdbEHFrSWqjpfQYMaAKQQgETrjSy4XTrp2RhC2oNqG/hylI4ab+F4X6fpH\
DYP1NqNMP5g36xNu7LhDnrUB5qsPjYOmWORxGLfudpF3oLYCSlr3DgHqEIB6HjQblLZ3KQuPBse3zxyROTnS\
AhdPH4a/z1wioFtKNVph3hecsiKEdqnz4Y2coSIdhz58mJ9JWNQoFAENE5CSsoEZAGvafYZVpW4C75YY2zq1\
wIeiFi1dT43/jLAUGkslsi1VvnyfUu8qO404RxYO3XHoGLMFoFLOO+lZ+VGci2Vz10AhxJhEBHxRKxw4k2uB\
HztoSJUr/2Y\n\
-----END MEGOLM SESSION DATA-----";

let timeline = TestTimeline::new();
let mut stream = timeline.subscribe_events().await;

let original_event_id = event_id!("$original");
let f = &timeline.factory;
timeline
.handle_live_event(
f.event(RoomEncryptedEventContent::new(
EncryptedEventScheme::MegolmV1AesSha2(
MegolmV1AesSha2ContentInit {
ciphertext: "\
AwgAEtABPRMavuZMDJrPo6pGQP4qVmpcuapuXtzKXJyi3YpEsjSWdzuRKIgJzD4P\
cSqJM1A8kzxecTQNJsC5q22+KSFEPxPnI4ltpm7GFowSoPSW9+bFdnlfUzEP1jPq\
YevHAsMJp2fRKkzQQbPordrUk1gNqEpGl4BYFeRqKl9GPdKFwy45huvQCLNNueql\
CFZVoYMuhxrfyMiJJAVNTofkr2um2mKjDTlajHtr39pTG8k0eOjSXkLOSdZvNOMz\
hGhSaFNeERSA2G2YbeknOvU7MvjiO0AKuxaAe1CaVhAI14FCgzrJ8g0y5nly+n7x\
QzL2G2Dn8EoXM5Iqj8W99iokQoVsSrUEnaQ1WnSIfewvDDt4LCaD/w7PGETMCQ"
.to_owned(),
sender_key: "DeHIg4gwhClxzFYcmNntPNF9YtsdZbmMy8+3kzCMXHA".to_owned(),
device_id: "NLAZCWIOCO".into(),
session_id: SESSION_ID.into(),
}
.into(),
),
None,
))
.event_id(original_event_id)
.sender(&BOB)
.into_utd_sync_timeline_event(),
)
.await;

timeline
.handle_live_event(f.text_msg("well said!").reply_to(original_event_id).sender(&ALICE))
.await;

// We receive the UTD.
{
let event = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
assert_let!(
TimelineItemContent::UnableToDecrypt(EncryptedMessage::MegolmV1AesSha2 {
session_id,
..
}) = event.content()
);
assert_eq!(session_id, SESSION_ID);
}

// We receive the text response.
{
let event = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
let msg = event.content().as_message().unwrap();
assert_eq!(msg.body(), "well said!");

let reply_details = msg.in_reply_to().unwrap();
assert_eq!(reply_details.event_id, original_event_id);

let replied_to = as_variant!(&reply_details.event, TimelineDetails::Ready).unwrap();
assert!(replied_to.content.as_unable_to_decrypt().is_some());
}

// Import a room key backup.
let own_user_id = user_id!("@example:morheus.localhost");
let exported_keys = decrypt_room_key_export(Cursor::new(SESSION_KEY), "1234").unwrap();

let olm_machine = OlmMachine::new(own_user_id, "SomeDeviceId".into()).await;
olm_machine.store().import_exported_room_keys(exported_keys, |_, _| {}).await.unwrap();

// Retry decrypting the UTD.
timeline
.controller
.retry_event_decryption_test(
room_id!("!DovneieKSTkdHKpIXy:morpheus.localhost"),
olm_machine,
Some(iter::once(SESSION_ID.to_owned()).collect()),
)
.await;

// The response is updated.
{
let event = assert_next_matches_with_timeout!(
stream,
VectorDiff::Set { index: 1, value } => value
);

let msg = event.content().as_message().unwrap();
assert_eq!(msg.body(), "well said!");

let reply_details = msg.in_reply_to().unwrap();
assert_eq!(reply_details.event_id, original_event_id);

let replied_to = as_variant!(&reply_details.event, TimelineDetails::Ready).unwrap();
assert_eq!(replied_to.content.as_message().unwrap().body(), "It's a secret to everybody");
}

// The event itself is decrypted.
{
let event = assert_next_matches!(stream, VectorDiff::Set { index: 0, value } => value);
assert_matches!(event.encryption_info(), Some(_));
assert_let!(TimelineItemContent::Message(message) = event.content());
assert_eq!(message.body(), "It's a secret to everybody");
assert!(!event.is_highlighted());
}
}

fn utd_event_with_unsigned(unsigned: serde_json::Value) -> SyncTimelineEvent {
let raw = Raw::from_json(
to_raw_value(&json!({
Expand Down
10 changes: 5 additions & 5 deletions crates/matrix-sdk-ui/src/timeline/tests/redaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,17 @@ async fn test_redact_replied_to_event() {

timeline.handle_live_event(f.redaction(first_item.event_id().unwrap()).sender(&ALICE)).await;

let first_item_again =
assert_next_matches!(stream, VectorDiff::Set { index: 0, value } => value);
assert_matches!(first_item_again.content(), TimelineItemContent::RedactedMessage);
assert_matches!(first_item_again.original_json(), None);

let second_item_again =
assert_next_matches!(stream, VectorDiff::Set { index: 1, value } => value);
let message = second_item_again.content().as_message().unwrap();
let in_reply_to = message.in_reply_to().unwrap();
assert_let!(TimelineDetails::Ready(replied_to_event) = &in_reply_to.event);
assert_matches!(replied_to_event.content(), TimelineItemContent::RedactedMessage);

let first_item_again =
assert_next_matches!(stream, VectorDiff::Set { index: 0, value } => value);
assert_matches!(first_item_again.content(), TimelineItemContent::RedactedMessage);
assert_matches!(first_item_again.original_json(), None);
Comment on lines +100 to +104
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of the assertion is changed here, because now we update the responses before the original event. This avoids a copy in the timeline code 👀 . You would be right calling this a code smell and asking for a change here, as I didn't bother too much (observers wouldn't notice a big difference).

}

#[async_test]
Expand Down
Loading