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 the decryption of events in the timeline if backups were enabled #3964

Merged
merged 7 commits into from
Sep 9, 2024

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Sep 9, 2024

This PR is an attempt to fix #3768 and subsequently element-hq/element-x-ios#3055. A review commit by commit is advised.

There are two issues that I have discovered:

  1. Like Historical messages decrypt on cache reloading, but not before #3768 theorized, enabling a backup doesn't trigger a decryption retry in the timeline.
  2. The background task which is supposed to download the room keys upon a decryption failure happens has logic to deduplicate downloads and to stop the download if the backup does not contain the room key. This logic incorrectly marks a room key as downloaded if a backup was not yet enabled and thus no actual download attempt happened.

The first two commits add regression tests for these two issues, then we fix the actual issues, and lastly we address a TODO item in the vicinity of the room key download task, and do some renames.

If this is too much to handle, please feel free to push for separate PRs for these tasks.

This closes: #3768.

  • Public API changes documented in changelogs (optional)

@poljar poljar requested a review from a team as a code owner September 9, 2024 13:11
@poljar poljar requested review from Hywan and removed request for a team September 9, 2024 13:11
Copy link

codecov bot commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 77.27273% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.28%. Comparing base (19e89bb) to head (7df2329).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/encryption/tasks.rs 66.66% 3 Missing ⚠️
crates/matrix-sdk/src/encryption/backups/mod.rs 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3964      +/-   ##
==========================================
+ Coverage   84.16%   84.28%   +0.12%     
==========================================
  Files         267      267              
  Lines       28190    28211      +21     
==========================================
+ Hits        23725    23778      +53     
+ Misses       4465     4433      -32     

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

Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the test, the renaming and the great doc!

crates/matrix-sdk/src/encryption/tasks.rs Show resolved Hide resolved
crates/matrix-sdk-ui/src/timeline/builder.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/tasks.rs Show resolved Hide resolved
The term session is usually only used in the crypto crate to reference a
Megolm session, the rest of the SDK uses the name from the event and the
Matrix spec, this should lower the amount of confusion since the main
crate has already a session concept and its unrelated to end-to-end
encryption.
@poljar poljar force-pushed the poljar/timeline-retry-decryption-if-backups-enable branch from b4886ae to 7df2329 Compare September 9, 2024 14:36
@poljar poljar enabled auto-merge (rebase) September 9, 2024 14:41
@poljar poljar merged commit dcc20b6 into main Sep 9, 2024
40 checks passed
@poljar poljar deleted the poljar/timeline-retry-decryption-if-backups-enable branch September 9, 2024 14:51
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.

Historical messages decrypt on cache reloading, but not before
2 participants