fix(sliding_sync): Add ignore_verification_requests method to SlidingSyncBuilder
#4705
Conversation
…ingSyncBuilder` This can be used to ignore verification requests in this sliding sync instance, preventing issues found where several sliding sync instances with the same client process events simultaneously and re-process the same verification request events during their initial syncs.
…en the `e2e-encryption` feature is disabled
…sts` when the `e2e-encryption` feature is disabled
…sts` when the `e2e-encryption` feature is disabled
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4705 +/- ##
==========================================
- Coverage 85.89% 85.89% -0.01%
==========================================
Files 292 292
Lines 33912 33923 +11
==========================================
+ Hits 29129 29138 +9
- Misses 4783 4785 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This feels a bit like a hack, though sadly I don't have any better ideas how to deal with this problem.
I'm wondering if we should ignore all verification events. Additionally could you write a test that the notification client won't create Verification objects?
An integration test would probably be the easiest to write.
| if !ignore_verification_requests { | ||
| Box::pin(self.handle_verification_event(e, room.room_id())) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
Shouldn't we ignore all verification events? I guess the crypto crate already does so if it doesn't receive an m.key.verification.request event, but I think it'll log a bunch of warnings in that case.
There was a problem hiding this comment.
Isn't that what I'm doing below? I'm not sure I fully understand the underlying logic, but I think this branch takes the verification requests and the one below any other verification event.
There was a problem hiding this comment.
Ah, how did I miss that one. You are indeed right.
Can we then rename the flag and setting? Since we indeed don't just ignore the requests ignore_verification_events might make more sense.
| Box::pin(self.handle_verification_event(e, room.room_id())) | ||
| .await?; | ||
| if !ignore_verification_requests { | ||
| Box::pin(self.handle_verification_event(e, room.room_id())) |
There was a problem hiding this comment.
Shouldn't we use this ignore_verification_requests inside the handle_verification_event method instead of here?
| #[instrument(skip_all, level = "trace")] | ||
| pub async fn process_sliding_sync<PEP: PreviousEventsProvider>( | ||
| &self, | ||
| response: &http::Response, | ||
| previous_events_provider: &PEP, | ||
| ignore_verification_requests: bool, |
There was a problem hiding this comment.
Can't we get this information from the BaseClient instead of passing an argument everywhere?
There was a problem hiding this comment.
BaseClient doesn't hold this parameter, and it makes sense that it doesn't AFAICT, since this is something only the sliding sync should know, since it can (and will in Android) wrap an existing client that wants to process the verification requests.
This can be used to ignore verification requests in this sliding sync instance, preventing issues found where several sliding sync instances with the same client process events simultaneously and re-process the same verification request events during their initial syncs.
This is a blocker for user verification on Android clients, since both the normal sync and the one initiated to get the event for a notification will process the same verification request events and end up with conflicts, making the verification flow fail most of the time.
Signed-off-by: