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

chore(crypto): Add consistency check on device when loading account #4564

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BillCarsonFr
Copy link
Member

Add a consistency check when loading an account from a store.

When creating an olm machine for an empty store, the own device is explicitely saved before the account (and it is done on purpose before the initial keys/query response):

let device = DeviceData::from_account(&account);
// We just created this device from our own Olm `Account`. Since we are the
// owners of the private keys of this device we can safely mark
// the device as verified.
device.set_trust_state(LocalTrust::Verified);
let changes = Changes {
devices: DeviceChanges { new: vec![device], ..Default::default() },
..Default::default()
};
store.save_changes(changes).await?;
store.save_pending_changes(PendingChanges { account: Some(account) }).await?;

However when loading an existing account we don't check for the existence of this own device.
There are other part in the code when we expect the own device to always exists:

.expect("We should be able to find our own device."))

Let's do the consistency check as soon as possible to avoid future strange states.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@BillCarsonFr BillCarsonFr requested review from a team as code owners January 21, 2025 10:40
@BillCarsonFr BillCarsonFr requested review from bnjbvr and poljar and removed request for a team January 21, 2025 10:40
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.41%. Comparing base (210c574) to head (c3d099b).

Files with missing lines Patch % Lines
crates/matrix-sdk-crypto/src/machine/mod.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4564      +/-   ##
==========================================
+ Coverage   85.40%   85.41%   +0.01%     
==========================================
  Files         285      285              
  Lines       32218    32221       +3     
==========================================
+ Hits        27515    27521       +6     
+ Misses       4703     4700       -3     

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

Comment on lines +34 to +36
/// An account was saved but no own device was found.
#[error("Incomplete account, account data was saved but no own device was found")]
IncompleteAccountNoOwnDevice,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not enthusiastic about adding this here as yet another error code that any API can return: error handling is really hard when you call matrix-sdk-crypto APIs because there are thousands of possible error cases.

Maybe we can specialise the result from with_store?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I was not sure about this one. What do you mean by "specialise" the result?

Copy link
Member

Choose a reason for hiding this comment

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

make it return a different error type in its Result, which more closely reflects the actual possible failure modes of that particular method.

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Marking request changes for Rich's comment, which I agree with.

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