Create a separate error variant to indicate a failure importing a secret. #5647

uhoreg · 2025-09-09T16:52:53Z

Part of the fix to element-hq/element-x-android#5099

Allows applications to distinguish between errors that occur when unlocking Secret Storage, or errors that occur when importing a secret, so that they can display appropriate feedback (or not) to the user.

Public API changes documented in changelogs (optional)

Signed-off-by:

codspeed-hq · 2025-09-09T17:17:18Z

CodSpeed Performance Report

Merging #5647 will improve performances by 46.97%

_{Comparing uhoreg:separate_import_error (c3ea78e) with main (e71d565)}

Summary

⚡ 1 improvement
✅ 49 untouched

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
⚡	`Restore session [memory store]`	401.7 ms	273.3 ms	+46.97%

codecov · 2025-09-09T17:19:24Z

Codecov Report

❌ Patch coverage is 82.94574% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.30%. Comparing base (2b567e1) to head (c3ea78e).
⚠️ Report is 91 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-crypto/src/olm/signing/mod.rs	35.13%	22 Missing and 2 partials ⚠️
...-sdk/src/encryption/secret_storage/secret_store.rs	73.52%	5 Missing and 4 partials ⚠️
...es/matrix-sdk/src/encryption/secret_storage/mod.rs	27.27%	8 Missing ⚠️
crates/matrix-sdk/src/encryption/recovery/mod.rs	97.70%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #5647    +/-   ##
========================================
  Coverage   88.30%   88.30%            
========================================
  Files         357      357            
  Lines       98509    98743   +234     
  Branches    98509    98743   +234     
========================================
+ Hits        86987    87196   +209     
- Misses       7389     7420    +31     
+ Partials     4133     4127     -6

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

uhoreg · 2025-09-09T18:08:48Z

Not sure why the one unit test is failing. It looks completely unrelated to what I changed, and the test succeeds on my machine.

Hywan

It looks great to me! Thanks. I've a couple of feedback, but no blocker.

crates/matrix-sdk/src/encryption/secret_storage/secret_store.rs

Hywan · 2025-09-12T12:52:03Z

crates/matrix-sdk/src/encryption/secret_storage/secret_store.rs

+        if let Some(secret_content) =
+            self.client.account().fetch_account_data(event_type).await.map_err(ImportError::from)?


Should be a fixup of the prefix patch.

Changed the commit to be a fixup. Hopefully I did it right.

But the CI is complaining about the fixup, so not sure what's going on. :-/

That's just a reminder for us to squash the fixup commits.

poljar · 2025-09-12T13:19:53Z

bindings/matrix-sdk-ffi/src/encryption.rs

+    #[error("Error importing a secret: {error_message}")]
+    Import { error_message: String },


Sorry for the drive-by comment, but don't we want to remember which secret couldn't have been imported?

It wasn't needed for the issue I was working on, but it does sound like it could be useful. I guess it means that I can't just use ImportError::from (or I can't just use the automatic conversion from ImportError to SecretStorageError. Is there some sort of convention that I should follow?

You won't be able to use the automatic conversion, but you can just map_err() from one error to the other and include the context that we can't infer automatically this way.

Sorry, I wasn't clear with my question. I was wondering if there was a convention for what to put in the map_err. Should I just throw an anonymous function in there? Or should I pre-define some functions in ImportError? Or something else?

Depends on how many times you need to do the same map, two or more use a pre-defined function, otherwise anonymous functions are fine.

I've changed SecretStorageError::ImportError to have name and error fields, where name is the name of the secret that was being imported. It all seems fine except that the way SecretStore.import_secrets imports the cross signing keys is that it loads all the secrets from Secret Storage (which is fine, it calls SecretStore.get_secret individually on each secret name), but then it bundles them all up and calls OlmMachine.import_cross_signing_keys on the whole bundle. In there, it will error, for example, if one of the private keys doesn't match the public key.

But that means that I can't tell which secret caused the error, unless I modify the error type returned by OlmMachine.import_cross_signing_keys so that it contains the secret name, which means changing stuff in the crypto crate. I'm a bit hesitant to go deeper into the Rust SDK for this, since it will probably affect more stuff, but I think it's the only way to get the right information. So, just wanted to double check that it's OK for me to do that.

I think the alternative would be to hard-code something in SecretStore.import_secrets saying that it's, say, the Master key that caused the error, and hope for the best, but I'm not too excited about that either.

I think it's fine to include that in the crypto crate as well. Sure that's a breaking change but a fairly minor one.

Signed-off-by: Hubert Chathi <[email protected]>

poljar

Looks good to me.

uhoreg · 2025-09-29T19:40:35Z

Ping @Hywan: can you give this a re-review?

uhoreg · 2025-10-01T14:07:44Z

@Hywan or @poljar: one of you will need to hit the merge button because I don't have permissions in this repo

uhoreg added 2 commits August 26, 2025 15:55

log errors from get_secret

df5ba0b

create a separate error variant to indicate a failure to import a secret

00de190

uhoreg mentioned this pull request Sep 9, 2025

Fix error when attempting to verify with recovery key with missing backup key element-hq/element-x-android#5314

Merged

9 tasks

uhoreg marked this pull request as ready for review September 11, 2025 17:53

uhoreg requested a review from a team as a code owner September 11, 2025 17:53

uhoreg requested review from Hywan and removed request for a team September 11, 2025 17:53

Hywan requested changes Sep 12, 2025

View reviewed changes

poljar reviewed Sep 12, 2025

View reviewed changes

uhoreg added 3 commits September 16, 2025 19:47

fixup! add a missed case

71dc404

add tests

958d0dc

fixup! switch from if statement to match statement

0cb753f

uhoreg force-pushed the separate_import_error branch from dc0ff15 to 0cb753f Compare September 17, 2025 00:06

uhoreg added 2 commits September 19, 2025 17:12

add secret name to import error

6d8b3da

add changelog

fbf8b45

uhoreg requested a review from a team as a code owner September 19, 2025 21:13

uhoreg requested review from poljar and removed request for a team September 19, 2025 21:13

Merge branch 'main' into separate_import_error

d227f74

Signed-off-by: Hubert Chathi <[email protected]>

uhoreg force-pushed the separate_import_error branch from 6394f2c to d227f74 Compare September 23, 2025 02:28

convert to using global_account_data_mock_builder

2bb4707

poljar approved these changes Sep 23, 2025

View reviewed changes

uhoreg added 2 commits September 23, 2025 12:30

fixup! convert to using global_account_data_mock_builder

083f575

fixup! convert to using global_account_data_mock_builder

c3ea78e

uhoreg force-pushed the separate_import_error branch from ee614bc to c3ea78e Compare September 23, 2025 19:07

uhoreg requested a review from Hywan September 23, 2025 19:37

Hywan approved these changes Oct 1, 2025

View reviewed changes

Hywan merged commit 57d21cc into matrix-org:main Oct 2, 2025
53 of 54 checks passed

		if let Some(secret_content) =
		self.client.account().fetch_account_data(event_type).await.map_err(ImportError::from)?

		#[error("Error importing a secret: {error_message}")]
		Import { error_message: String },

Create a separate error variant to indicate a failure importing a secret. #5647

Create a separate error variant to indicate a failure importing a secret. #5647

Uh oh!

Conversation

uhoreg commented Sep 9, 2025

Uh oh!

codspeed-hq bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #5647 will improve performances by 46.97%

Summary

Benchmarks breakdown

Uh oh!

codecov bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

uhoreg commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hywan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

uhoreg commented Sep 29, 2025

Uh oh!

uhoreg commented Oct 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq bot commented Sep 9, 2025 •

edited

Loading

codecov bot commented Sep 9, 2025 •

edited

Loading

uhoreg commented Sep 9, 2025 •

edited

Loading