Entities linked to Factor Source #302

matiasbzurovski · 2024-12-11T16:10:33Z

Changelog

Exposes a function on SargonOS to allow checking entities related to a FactorSource.

This allow us to determine, for example, which entities were created under a given DeviceFactorSource or a LedgerFactorSource. Also, it will work for Securified Entities to see if the given factor source is in any way involved in the matrix that securifies it.

Output

Asides from returning the corresponding entities and their visibility, the output includes also an integrity field which will become helpful for host apps to determine the integrity of the mentioned FactorSource.

In the case of DeviceFactorSource, its integrity will detail whether its mnemonic is present in keychain, and if it has been backed up by the user.

Storage clients

To check if the mnemonic is present in Keychain, I make use of the SecureStorageClient using the previously supported key DeviceFactorSourceMnemonic.

However, in order to check if the mnemonic is backed up I need to make usage of the UnsafeStorageClient. In iOS, when booting SargonOS, we implement the UnsafeStorageDriver directly from native UserDefaults. In other words, every time Sargon needs to read/write some values on unsafe storage, it will directly use the UserDefaults.

Then, the only pending thing I needed to add was a way to map the keys previously defined by iOS Wallet, into the ones expected by Sargon. Right now we only have one UnsafeStorageKey which is FactorSourceUserHasWrittenDown, which I map on iOS into its pre-Sargon-defined-key named "mnemonicsUserClaimsToHaveBackedUp".

Important

This approach only works for both platforms if someone from Android team can validate that:

they are also saving in Unsafe Storage a set of FactorSourceIDFromHash of device factor sources whose mnemonic has been written down.
they are either using the SargonOS key, or they can also map it to the expected ones as iOS does.

If this is not the case, then we may not be able to have the straightforward solution of interacting directly with UserDefaults on iOS, and instead we would need to treat unsafe storage similarly to secure storage: just send the key to the host and let it read/write the value however it decides to.

crates/sargon/src/profile/mfa/secured_entity_control/secured_entity_control.rs

crates/sargon/src/system/drivers/unsafe_storage_driver/support/unsafe_storage_key.rs

...on_os/entities_controlled_by_factor_source/sargon_os_entities_controlled_by_factor_source.rs

...rgon_os/entities_controlled_by_factor_source/support/entities_controlled_by_factor_source.rs

Sajjon · 2024-12-11T16:31:02Z

...rgon_os/entities_controlled_by_factor_source/support/entities_controlled_by_factor_source.rs

+    pub factor_source: FactorSource,
+
+    /// Whether the mnemonic of the factor source is present in keychain.
+    pub is_mnemonic_present_in_keychain: bool,


many of the fields here are DeviceFactorSource specific. If the WHOLE query is that, we should name it EntitiesUsingDeviceFactorSource? Or we should generalize it, probably with enums with associated data, so that is_mnemonic_present_in_keychain: bool only exists when the factor source is of kind device

well, on iOS it is currently related to DeviceFactorSource, but the idea of moving it to Sargon is to generalize it so that we can later check which entities are related to, for example, an Arculus Card (ref).

You are right that is_mnemonic_present_in_keychain & is_mnemonic_marked_as_backed_up only applies to DeviceFactorSource, so probabky the enum with associated data is the way to go here.

codecov · 2024-12-11T16:34:15Z

Codecov Report

Attention: Patch coverage is 87.11656% with 21 lines in your changes missing coverage. Please review.

Project coverage is 93.2%. Comparing base (56c6e58) to head (e7a80b1).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ent/unsafe_storage_client/unsafe_storage_client.rs	77.2%	5 Missing ⚠️
...iver+InsecureTestOnlyEphemeral_SecureStorage.swift	0.0%	3 Missing ⚠️
...n_os/sargon_os_entities_linked_to_factor_source.rs	90.9%	3 Missing ⚠️
...ixdlt/sargon/os/storage/key/ByteArrayKeyMapping.kt	0.0%	2 Missing and 1 partial ⚠️
...ava/com/radixdlt/sargon/os/storage/StorageUtils.kt	0.0%	2 Missing ⚠️
...torage/key/DeviceFactorSourceMnemonicKeyMapping.kt	0.0%	2 Missing ⚠️
...ent/secure_storage_client/secure_storage_client.rs	80.0%	1 Missing ⚠️
...radixdlt/sargon/os/storage/key/HostIdKeyMapping.kt	0.0%	1 Missing ⚠️
...sargon/os/storage/key/ProfileSnapshotKeyMapping.kt	0.0%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #302     +/-   ##
=======================================
- Coverage   93.3%   93.2%   -0.1%     
=======================================
  Files       1098    1104      +6     
  Lines      23129   23283    +154     
  Branches      77      79      +2     
=======================================
+ Hits       21582   21717    +135     
- Misses      1533    1551     +18     
- Partials      14      15      +1

Flag	Coverage Δ
kotlin	`97.1% <0.0%> (-0.8%)`	⬇️
rust	`92.7% <93.1%> (+<0.1%)`	⬆️
swift	`94.8% <86.3%> (-0.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...urces/Sargon/Drivers/TestDrivers/TestDrivers.swift	`100.0% <100.0%> (ø)`
...safeStorage/UnsafeStorageDriver+UserDefaults.swift	`95.8% <100.0%> (+2.0%)`	⬆️
...sions/Swiftified/System/BIOS/BIOS+Swiftified.swift	`100.0% <100.0%> (ø)`
...Swiftified/System/Drivers/Drivers+Swiftified.swift	`100.0% <100.0%> (ø)`
apple/Sources/Sargon/SargonOS/TestOS.swift	`47.3% <100.0%> (+1.4%)`	⬆️
...n/src/profile/logic/account/accounts_visibility.rs	`100.0% <100.0%> (ø)`
...sargon/src/profile/logic/account/query_accounts.rs	`95.4% <100.0%> (+0.2%)`	⬆️
...sargon/src/profile/logic/persona/query_personas.rs	`100.0% <100.0%> (ø)`
...rofile_network_entities_linked_to_factor_source.rs	`100.0% <100.0%> (ø)`
...ic/profile_network/profile_network_get_entities.rs	`100.0% <100.0%> (ø)`
... and 16 more

... and 1 file with indirect coverage changes

Sajjon · 2024-12-12T09:52:38Z

crates/sargon-uniffi/src/system/drivers/secure_storage_driver/secure_storage_driver.rs

@@ -19,6 +19,11 @@ pub trait SecureStorageDriver: Send + Sync + std::fmt::Debug {
    ) -> Result<()>;

    async fn delete_data_for_key(&self, key: SecureStorageKey) -> Result<()>;
+
+    async fn contains_data_for_key(


maybe a batch query would make sense? contains_data_for_keys ? Check with @micbakos-rdx but on iOS we can load the entire contents of Keychain in one go without prompting for biometrics and check the list of key-values. So we can actually impl check_existence_of_mnemonics_for_device_factor_sources(ids: IndexSet<FactorSourceIDFromHash>) -> IndexSet<FactorSourceIDFromHash> where if return value == ids.len() then all was found.

for the current implementation we will only make the request for a given Factor Source, but I can extend it to take a set of keys instead.

@micbakos-rdx mentioned that we can check for key existence without any biometrics.

...fi/src/system/sargon_os/entities_controlled_by_factor_source/support/accessibility/device.rs

crates/sargon/src/profile/mfa/secured_entity_control/secured_entity_control.rs

...tem/sargon_os/entities_linked_to_factor_source/sargon_os_entities_linked_to_factor_source.rs

Sajjon · 2024-12-12T10:03:58Z

...tem/sargon_os/entities_linked_to_factor_source/sargon_os_entities_linked_to_factor_source.rs

+        factor_source: FactorSource,
+        profile_to_check: ProfileToCheck,
+    ) -> Result<EntitiesLinkedToFactorSource> {
+        let accessibility = self.accessibility(factor_source.clone()).await?;


Same review comment as in some PRs ago... :) this should not be on SargonOS :) SargonOS ought to only "forward" calls to Profile (via ProfileHolder.). since you here support ProfileToCheck::Specific so maybe something like?:

impl SargonOS { /// Returns the entities linked to a given `FactorSource`, either on the current `Profile` or a specific one. pub async fn entities_linked_to_factor_source( &self, factor_source: FactorSource, profile_to_check: ProfileToCheck, ) -> Result<EntitiesLinkedToFactorSource> { let existence = self.claimed_and_actual_existence_of_factor_source(factor_source.clone()).await?; match profile_to_check { ProfileToCheck::Specific(profile) => profile.entities_linked_to_factor_source(factor_source, existence), ProfileToCheck::Current => self.profile().entities_linked_to_factor_source(factor_source, existence), } } }

Sajjon · 2024-12-12T20:33:18Z

crates/sargon/src/system/drivers/secure_storage_driver/support/test/mock_secure_storage.rs

+
+#[derive(Debug)]
+pub struct MockSecureStorage {
+    mock_contains_data_for_key: IndexMap<SecureStorageKey, bool>,


Hmm is this needed? We have:

pub struct EphemeralSecureStorage { pub storage: RwLock<HashMap<SecureStorageKey, BagOfBytes>>, }

GhenadieVP

Looks good overall, very nice improvement.

GhenadieVP · 2024-12-13T07:17:11Z

...sargon/src/profile/logic/profile_network/profile_network_entities_linked_to_factor_source.rs

+        let hidden_accounts = self
+            .accounts
+            .hidden()


Minor: I guess we should have similar function to accounts_non_hidden on ProfileNetwork - accounts_hidden

GhenadieVP · 2024-12-13T07:18:03Z

...sargon/src/profile/logic/profile_network/profile_network_entities_linked_to_factor_source.rs

+                a.security_state
+                    .is_linked_to_factor_source(factor_source.clone())


This could be in a common function, then passed to filter.

that what was my original idea: to define the filter predicate once and then use it on every collection:

let filter = |e: &dyn HasSecurityState| { e.security_state().is_linked_to_factor_source(factor_source.clone()) };

I wasn't able to make it work with a closure, but using a function that sends the factor_source each time works, thanks!

giannis-rdx · 2024-12-13T09:38:16Z

crates/sargon-uniffi/src/profile/v100/entities_linked_to_factor_source/integrity/device.rs

+pub struct DeviceFactorSourceIntegrity {
+    /// The factor source that is linked to the entities.
+    pub factor_source: DeviceFactorSource,
+
+    /// Whether the mnemonic of the factor source is present in keychain.
+    pub is_mnemonic_present_in_keychain: bool,
+
+    /// Whether the mnemonic of the factor source is marked as backed up.
+    pub is_mnemonic_marked_as_backed_up: bool,
+}


~~I am a bit lost here.~~
~~first of all is this input for sargon or output that hosts will use?~~
it's the output

what is the difference between is_mnemonic_present_in_keychain and is_mnemonic_marked_as_backed_up?
If is_mnemonic_marked_as_backed_up is true then doesn't that mean the is_mnemonic_present_in_keychain is also true?

What android has to do with the is_mnemonic_present_in_keychain? Or is it a boolean that just used by iOS?

is_mnemonic_marked_as_backed_up is set to true if the user says "I have written this down".

is_mnemonic_present_in_keychain is app/sargon checking that SecureStorage at least contains a key-value tuple where the key matches the FactorSourceID of some DeviceFactorSource.

why the name has to be related to iOS though?
shouldn't be is_mnemonic_present or something else?

we have similar thing in Android mnemonicExist

it should probably be _in_secure_storage. Not "keychain", good catch @giannis-rdx

And @matiasbzurovski we can have a computed property in Swift Sargon called ...inKeychain, to make it clear

Updated it to replace every reference to _in_keychain to _in_secure_stroage.

…droid SargonOS

giannis-rdx · 2024-12-13T10:37:22Z

This approach only works for both platforms if someone from Android team can validate that:

they are also saving in Unsafe Storage a set of FactorSourceIDFromHash of device factor sources whose mnemonic has been written down.

they are either using the SargonOS key, or they can also map it to the expected ones as iOS does.

yes for both.

Sajjon · 2024-12-13T10:46:21Z

crates/sargon/src/system/sargon_os/sargon_os_entities_linked_to_factor_source.rs

+        profile_to_check: ProfileToCheck,
+    ) -> Result<EntitiesLinkedToFactorSource> {
+        let integrity = self.integrity(factor_source.clone()).await?;
+        match profile_to_check {


prefer expressions:

let profile_network = match profile_to_check { ProfileToCheck::Current => self.profile()?.current_network()?, ProfileToCheck::Specific(specific_profile) => specific_profile.networks .get_id(NetworkID::Mainnet) .ok_or(CommonError::Unknown)? }; profile_network .entities_linked_to_factor_source(factor_source, integrity),

this doesn't compile because of multiple errors about borrowed values not living long enough, and in order to fix it I need to add more variables which I think complicates the code more.

matiasbzurovski added 2 commits December 11, 2024 15:45

WIP

666bafa

WIP

bbbf801

matiasbzurovski commented Dec 11, 2024

View reviewed changes

crates/sargon/src/profile/mfa/secured_entity_control/secured_entity_control.rs Outdated Show resolved Hide resolved

matiasbzurovski commented Dec 11, 2024

View reviewed changes

crates/sargon/src/system/drivers/unsafe_storage_driver/support/unsafe_storage_key.rs Outdated Show resolved Hide resolved

Sajjon reviewed Dec 11, 2024

View reviewed changes

...on_os/entities_controlled_by_factor_source/sargon_os_entities_controlled_by_factor_source.rs Outdated Show resolved Hide resolved

Sajjon reviewed Dec 11, 2024

View reviewed changes

...rgon_os/entities_controlled_by_factor_source/support/entities_controlled_by_factor_source.rs Outdated Show resolved Hide resolved

Sajjon reviewed Dec 11, 2024

View reviewed changes

matiasbzurovski added 2 commits December 11, 2024 18:05

key mapping solution

66c16b7

using accessibility

897ca57

matiasbzurovski changed the title ~~Entities controlled by Factor Source~~ Entities linked to Factor Source Dec 12, 2024

rename to linkedTo

2e751aa

Sajjon reviewed Dec 12, 2024

View reviewed changes

...fi/src/system/sargon_os/entities_controlled_by_factor_source/support/accessibility/device.rs Outdated Show resolved Hide resolved

Sajjon reviewed Dec 12, 2024

View reviewed changes

crates/sargon/src/profile/mfa/secured_entity_control/secured_entity_control.rs Outdated Show resolved Hide resolved

Sajjon reviewed Dec 12, 2024

View reviewed changes

...tem/sargon_os/entities_linked_to_factor_source/sargon_os_entities_linked_to_factor_source.rs Outdated Show resolved Hide resolved

Sajjon reviewed Dec 12, 2024

View reviewed changes

matiasbzurovski added 4 commits December 12, 2024 13:23

WIP

31c1f3b

rename to integrity

c327385

WIP

f562663

missing tests

7fa5564

matiasbzurovski marked this pull request as ready for review December 12, 2024 16:11

Merge branch 'main' into mb/entities-controlled-factor-source

068d92b

matiasbzurovski requested a review from micbakos-rdx December 12, 2024 16:26

version bump

1263959

Sajjon reviewed Dec 12, 2024

View reviewed changes

GhenadieVP approved these changes Dec 13, 2024

View reviewed changes

matiasbzurovski added 2 commits December 13, 2024 10:12

changes after feedback

e81710e

bump again

b6b39df

matiasbzurovski force-pushed the mb/entities-controlled-factor-source branch from 29fbf1d to b6b39df Compare December 13, 2024 09:13

Merge branch 'main' into mb/entities-controlled-factor-source

c51c7d8

giannis-rdx reviewed Dec 13, 2024

View reviewed changes

implemented containsDataForKey in the AndroidStorageDriver for the An…

fa5b6f0

…droid SargonOS

Sajjon reviewed Dec 13, 2024

View reviewed changes

giannis-rdx and others added 2 commits December 13, 2024 13:04

updated Fakes in unit tests

46d9069

changes after feedback

e7a80b1

giannis-rdx approved these changes Dec 13, 2024

View reviewed changes

matiasbzurovski merged commit 51c15f1 into main Dec 13, 2024
12 of 13 checks passed

matiasbzurovski deleted the mb/entities-controlled-factor-source branch December 13, 2024 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entities linked to Factor Source #302

Entities linked to Factor Source #302

matiasbzurovski commented Dec 11, 2024 •

edited

Loading

Sajjon Dec 11, 2024

matiasbzurovski Dec 11, 2024

codecov bot commented Dec 11, 2024 •

edited

Loading

Sajjon Dec 12, 2024

matiasbzurovski Dec 12, 2024

GhenadieVP Dec 13, 2024

Sajjon Dec 12, 2024

Sajjon Dec 12, 2024

matiasbzurovski Dec 13, 2024

GhenadieVP left a comment

GhenadieVP Dec 13, 2024

GhenadieVP Dec 13, 2024

matiasbzurovski Dec 13, 2024

giannis-rdx Dec 13, 2024 •

edited

Loading

Sajjon Dec 13, 2024

giannis-rdx Dec 13, 2024

giannis-rdx Dec 13, 2024

Sajjon Dec 13, 2024

matiasbzurovski Dec 13, 2024

giannis-rdx commented Dec 13, 2024

Sajjon Dec 13, 2024 •

edited

Loading

matiasbzurovski Dec 13, 2024

		a.security_state
		.is_linked_to_factor_source(factor_source.clone())

Entities linked to Factor Source #302

Entities linked to Factor Source #302

Conversation

matiasbzurovski commented Dec 11, 2024 • edited Loading

Changelog

Output

Storage clients

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GhenadieVP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannis-rdx Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannis-rdx commented Dec 13, 2024

Sajjon Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiasbzurovski commented Dec 11, 2024 •

edited

Loading

codecov bot commented Dec 11, 2024 •

edited

Loading

giannis-rdx Dec 13, 2024 •

edited

Loading

Sajjon Dec 13, 2024 •

edited

Loading