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.

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

Retry decryptions based on the room key stream of the OlmMachine #4348

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Nov 28, 2024

This is a partial fix for #4189.

The fix works if you don't use the cross-process lock, otherwise the stream gets destroyed and updates don't come in anymore.

Opening as a draft, still needs a test.

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.09%. Comparing base (e99939d) to head (66d6f49).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk-ui/src/timeline/builder.rs 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4348      +/-   ##
==========================================
+ Coverage   85.05%   85.09%   +0.04%     
==========================================
  Files         280      280              
  Lines       30382    30402      +20     
==========================================
+ Hits        25840    25871      +31     
+ Misses       4542     4531      -11     

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

@richvdh
Copy link
Member

richvdh commented Nov 28, 2024

So this fixes EXA but not EXI?

@poljar
Copy link
Contributor Author

poljar commented Nov 28, 2024

So this fixes EXA but not EXI?

Yes.

@poljar
Copy link
Contributor Author

poljar commented Dec 6, 2024

Gah, trying to write a test for this and I just realized that I need to delay the delivery of the room key so I can have the following conditions:

  1. Have the timeline open
  2. Let the main client/timeline receive the message, an UTD
  3. Have the notification client receive the room key just now
  4. Watch the UTD resolve itself because the notification client tells the timeline that the room key has been received

I guess I'll have to write the test in complement-crypto.

@bnjbvr
Copy link
Member

bnjbvr commented Dec 9, 2024

delay the delivery of the room key

Our testing suite has something like this:

impl wiremock::Respond for &CustomResponder {
fn respond(&self, request: &wiremock::Request) -> wiremock::ResponseTemplate {
// Convert the mocked request to an actual server request.
let req = self
.client
.request(request.method.clone(), request.url.clone())
.headers(request.headers.clone())
.body(request.body.clone());
// Run await inside of non-async fn by spawning a new thread and creating a new
// runtime. We need to do this because the current runtime can't run blocking
// tasks (hence can't run `Handle::block_on`).
let drop_todevice = self.drop_todevice.clone();
std::thread::spawn(move || {
let rt = tokio::runtime::Runtime::new().unwrap();
rt.block_on(async {
// Send the request to the actual backend.
let response = timeout(Duration::from_secs(2), req.send()).await;
if let Ok(Ok(response)) = response {
// Convert reqwest response to wiremock response.
let mut r = wiremock::ResponseTemplate::new(u16::from(response.status()));
for header in response.headers() {
if header.0 == "Content-Length" {
continue;
}
r = r.append_header(header.0.clone(), header.1.clone());
}
let mut bytes = response.bytes().await.unwrap_or_default();
// Manipulate the response
if *drop_todevice.lock().unwrap() {
drop_todevice_events(&mut bytes);
}
r.set_body_bytes(bytes)
} else {
// Gateway timeout.
wiremock::ResponseTemplate::new(504)
}
})
})
.join()
.expect("Thread panicked")
}
}

ToDevice events are evicted from a server response on purpose, and under the control of the testing code.

@poljar
Copy link
Contributor Author

poljar commented Dec 9, 2024

ToDevice events are evicted from a server response on purpose, and under the control of the testing code.

Well yes, wiremock can also delay responses, but then I'd have to mock a lot of things to get the E2EE and notification client working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants