From 3d8d3473d8b01658266bb0c5ef7bd641e1340135 Mon Sep 17 00:00:00 2001 From: Ghenadie <118184705+GhenadieVP@users.noreply.github.com> Date: Thu, 19 Sep 2024 12:48:29 +0300 Subject: [PATCH] Profile snapshot id payload (#214) * wip * wip * fix test * wip * fix ios tests * test * update * wip --- .../SargonOS/SargonOS+Static+Shared.swift | 2 +- apple/Sources/Sargon/SargonOS/TestOS.swift | 9 +-- .../Sources/Sargon/Util/EventPublisher.swift | 8 +-- .../DriversTests/DriversTests.swift | 2 +- .../DriversTests/InsecureStorageTests.swift | 2 +- .../SargonOS/SargonOSTests.swift | 2 +- crates/sargon/src/profile/v100/header/mod.rs | 2 + .../src/profile/v100/header/profile_id.rs | 1 - .../v100/header/profile_id_uniffi_fn.rs | 37 ++++++++++ .../secure_storage_client.rs | 57 ++++++++++----- .../support/secure_storage_key.rs | 69 +++++++++++++++++-- .../src/system/sargon_os/sargon_os_profile.rs | 7 +- .../sargon/os/driver/AndroidStorageDriver.kt | 1 - .../storage/key/ProfileSnapshotKeyMapping.kt | 1 - .../radixdlt/sargon/SecureStorageKeyTest.kt | 2 +- .../os/storage/key/ByteArrayKeyMappingTest.kt | 4 +- .../key/ProfileSnapshotKeyMappingTest.kt | 4 -- 17 files changed, 164 insertions(+), 46 deletions(-) create mode 100644 crates/sargon/src/profile/v100/header/profile_id_uniffi_fn.rs diff --git a/apple/Sources/Sargon/SargonOS/SargonOS+Static+Shared.swift b/apple/Sources/Sargon/SargonOS/SargonOS+Static+Shared.swift index 697dc0184..6ecfd9edf 100644 --- a/apple/Sources/Sargon/SargonOS/SargonOS+Static+Shared.swift +++ b/apple/Sources/Sargon/SargonOS/SargonOS+Static+Shared.swift @@ -47,7 +47,7 @@ extension SargonOS { if !isEmulatingFreshInstall, _shared != nil { throw SargonOSAlreadyBooted() } - let shared = try await SargonOS.boot(bios: bios) + let shared = await SargonOS.boot(bios: bios) Self._shared = shared return shared } diff --git a/apple/Sources/Sargon/SargonOS/TestOS.swift b/apple/Sources/Sargon/SargonOS/TestOS.swift index f425da8ab..a1581d27e 100644 --- a/apple/Sources/Sargon/SargonOS/TestOS.swift +++ b/apple/Sources/Sargon/SargonOS/TestOS.swift @@ -42,14 +42,9 @@ extension TestOS: SargonOSProtocol {} // MARK: Private extension TestOS { - private func nextAccountName() throws -> DisplayName { - do { - let index = try accountsForDisplayOnCurrentNetwork.count + private func nextAccountName() -> DisplayName { + let index = (try? accountsForDisplayOnCurrentNetwork.count) ?? 0 return DisplayName(value: "Unnamed \(index)") - } catch CommonError.NoNetworkInProfile(_) { - return DisplayName(value: "Unnamed 0") - } - } } diff --git a/apple/Sources/Sargon/Util/EventPublisher.swift b/apple/Sources/Sargon/Util/EventPublisher.swift index 25d78832a..098832d06 100644 --- a/apple/Sources/Sargon/Util/EventPublisher.swift +++ b/apple/Sources/Sargon/Util/EventPublisher.swift @@ -1,11 +1,11 @@ import AsyncExtensions public final actor EventPublisher { - public typealias Subject = AsyncPassthroughSubject - public typealias Stream = AsyncThrowingPassthroughSubject + public typealias Subject = AsyncReplaySubject + public typealias Stream = AsyncThrowingReplaySubject - let stream = Stream() - let subject = Subject() + let stream = Stream(bufferSize: 1) + let subject = Subject(bufferSize: 1) public func eventStream() -> AsyncMulticastSequence { subject diff --git a/apple/Tests/IntegrationTests/DriversTests/DriversTests.swift b/apple/Tests/IntegrationTests/DriversTests/DriversTests.swift index 22da9576a..7de9038b1 100644 --- a/apple/Tests/IntegrationTests/DriversTests/DriversTests.swift +++ b/apple/Tests/IntegrationTests/DriversTests/DriversTests.swift @@ -29,6 +29,6 @@ final class DriversTests: TestCase { } func test_bios_insecure() async throws { - let _ = try await SargonOS.boot(bios: BIOS.insecure()) + let _ = await SargonOS.boot(bios: BIOS.insecure()) } } diff --git a/apple/Tests/IntegrationTests/DriversTests/InsecureStorageTests.swift b/apple/Tests/IntegrationTests/DriversTests/InsecureStorageTests.swift index 0df0b7101..bbe604124 100644 --- a/apple/Tests/IntegrationTests/DriversTests/InsecureStorageTests.swift +++ b/apple/Tests/IntegrationTests/DriversTests/InsecureStorageTests.swift @@ -9,7 +9,7 @@ class InsecureStorageDriverTests: DriverTest ProfileID { + ProfileID::sample() +} + +#[uniffi::export] +pub fn new_profile_id_sample_other() -> ProfileID { + ProfileID::sample_other() +} + +#[cfg(test)] +mod uniffi_test { + + use super::*; + + #[allow(clippy::upper_case_acronyms)] + type SUT = ProfileID; + + #[test] + fn hash_of_samples() { + assert_eq!( + HashSet::::from_iter([ + new_profile_id_sample(), + new_profile_id_sample_other(), + // duplicates should get removed + new_profile_id_sample(), + new_profile_id_sample_other(), + ]) + .len(), + 2 + ); + } +} diff --git a/crates/sargon/src/system/clients/client/secure_storage_client/secure_storage_client.rs b/crates/sargon/src/system/clients/client/secure_storage_client/secure_storage_client.rs index 496cf3dd4..38c8f818f 100644 --- a/crates/sargon/src/system/clients/client/secure_storage_client/secure_storage_client.rs +++ b/crates/sargon/src/system/clients/client/secure_storage_client/secure_storage_client.rs @@ -94,7 +94,7 @@ impl SecureStorageClient { /// Loads the Profile. pub async fn load_profile(&self) -> Result> { debug!("Loading profile"); - self.load(SecureStorageKey::ProfileSnapshot) + self.load(SecureStorageKey::load_profile_snapshot()) .await .inspect(|some_profile| { if some_profile.is_some() { @@ -110,10 +110,15 @@ impl SecureStorageClient { pub async fn save_profile(&self, profile: &Profile) -> Result<()> { let profile_id = profile.id(); debug!("Saving profile with id: {}", profile_id); - self.save(SecureStorageKey::ProfileSnapshot, profile) - .await - .inspect(|_| debug!("Saved profile with id {}", profile_id)) - .inspect_err(|e| error!("Failed to save profile, error {e}")) + self.save( + SecureStorageKey::ProfileSnapshot { + profile_id: profile.id(), + }, + profile, + ) + .await + .inspect(|_| debug!("Saved profile with id {}", profile_id)) + .inspect_err(|e| error!("Failed to save profile, error {e}")) } //====== @@ -200,7 +205,9 @@ impl SecureStorageClient { pub async fn delete_profile(&self, id: ProfileID) -> Result<()> { warn!("Deleting profile with id: {}", id); self.driver - .delete_data_for_key(SecureStorageKey::ProfileSnapshot) + .delete_data_for_key(SecureStorageKey::ProfileSnapshot { + profile_id: id, + }) .await } } @@ -230,7 +237,8 @@ mod tests { async fn load_ok_when_none() { let sut = make_sut(); assert_eq!( - sut.load::(SecureStorageKey::ProfileSnapshot).await, + sut.load::(SecureStorageKey::load_profile_snapshot()) + .await, Ok(None) ); } @@ -239,13 +247,22 @@ mod tests { async fn load_successful() { let sut = make_sut(); + let profile = Profile::sample(); assert!(sut - .save(SecureStorageKey::ProfileSnapshot, &Profile::sample()) + .save( + SecureStorageKey::ProfileSnapshot { + profile_id: profile.id() + }, + &profile + ) .await .is_ok()); assert_eq!( - sut.load::(SecureStorageKey::ProfileSnapshot).await, - Ok(Some(Profile::sample())) + sut.load::(SecureStorageKey::ProfileSnapshot { + profile_id: profile.id() + }) + .await, + Ok(Some(profile)) ); } @@ -253,17 +270,25 @@ mod tests { async fn load_unwrap_or_some_default_not_used() { let sut = make_sut(); + let profile = Profile::sample(); assert!(sut - .save(SecureStorageKey::ProfileSnapshot, &Profile::sample()) + .save( + SecureStorageKey::ProfileSnapshot { + profile_id: profile.id() + }, + &profile + ) .await .is_ok()); assert_eq!( sut.load_unwrap_or::( - SecureStorageKey::ProfileSnapshot, - Profile::sample_other() + SecureStorageKey::ProfileSnapshot { + profile_id: profile.id() + }, + profile.clone() ) .await, - Profile::sample() + profile ); } @@ -273,7 +298,7 @@ mod tests { assert_eq!( sut.load_unwrap_or::( - SecureStorageKey::ProfileSnapshot, + SecureStorageKey::load_profile_snapshot(), Profile::sample_other() ) .await, @@ -350,7 +375,7 @@ mod tests { let (sut, _) = SecureStorageClient::ephemeral(); assert_eq!( sut.save( - SecureStorageKey::ProfileSnapshot, + SecureStorageKey::load_profile_snapshot(), &AlwaysFailSerialize {} ) .await, diff --git a/crates/sargon/src/system/drivers/secure_storage_driver/support/secure_storage_key.rs b/crates/sargon/src/system/drivers/secure_storage_driver/support/secure_storage_key.rs index b47c7616f..e9503d048 100644 --- a/crates/sargon/src/system/drivers/secure_storage_driver/support/secure_storage_key.rs +++ b/crates/sargon/src/system/drivers/secure_storage_driver/support/secure_storage_key.rs @@ -1,12 +1,60 @@ use crate::prelude::*; +use std::hash::{Hash, Hasher}; -#[derive(Debug, Clone, PartialEq, Eq, Hash, uniffi::Enum)] +#[derive(Debug, Clone, Eq, uniffi::Enum)] pub enum SecureStorageKey { HostID, DeviceFactorSourceMnemonic { factor_source_id: FactorSourceIDFromHash, }, - ProfileSnapshot, + ProfileSnapshot { + // Note: + // `profile_id` is only meant to be used by the iOS Host for backward compatibility. + // iOS Host stores multiple profiles in the secure storage uniquely identified by `profile_id`, + // while Android Host stores only one profile in the secure storage. + profile_id: ProfileID, + }, +} + +impl PartialEq for SecureStorageKey { + fn eq(&self, other: &SecureStorageKey) -> bool { + match (self, other) { + (SecureStorageKey::HostID, SecureStorageKey::HostID) => true, + ( + SecureStorageKey::DeviceFactorSourceMnemonic { + factor_source_id: a, + }, + SecureStorageKey::DeviceFactorSourceMnemonic { + factor_source_id: b, + }, + ) => a == b, + ( + SecureStorageKey::ProfileSnapshot { .. }, + SecureStorageKey::ProfileSnapshot { .. }, + ) => true, // Note: `profile_id` is not used for comparison, as it is only forwarded as additional payload to the iOS Host. + _ => false, + } + } +} + +impl Hash for SecureStorageKey { + fn hash(&self, state: &mut H) { + match self { + SecureStorageKey::HostID => { + "host_id".hash(state); + } + SecureStorageKey::DeviceFactorSourceMnemonic { + factor_source_id, + } => { + "device_factor_source".hash(state); + factor_source_id.hash(state); + } + // Note: `profile_id` is not used for computing the hash, as it is only forwarded as additional payload to the iOS Host. + SecureStorageKey::ProfileSnapshot { .. } => { + "profile_snapshot".hash(state); + } + } + } } impl SecureStorageKey { @@ -19,13 +67,24 @@ impl SecureStorageKey { SecureStorageKey::DeviceFactorSourceMnemonic { factor_source_id, } => format!("device_factor_source_{}", factor_source_id), - SecureStorageKey::ProfileSnapshot => + SecureStorageKey::ProfileSnapshot { .. } => "profile_snapshot".to_owned(), } ) } } +impl SecureStorageKey { + pub fn load_profile_snapshot() -> Self { + // This id will not be used to load the profile snapshot. + // It is only a stub to conform to the SecureStorageKey definition. + let dummy_id = ProfileID(Uuid::from_bytes([0x00; 16])); + SecureStorageKey::ProfileSnapshot { + profile_id: dummy_id, + } + } +} + #[uniffi::export] pub fn secure_storage_key_identifier(key: &SecureStorageKey) -> String { key.identifier() @@ -45,7 +104,7 @@ mod tests { "secure_storage_key_device_factor_source_device:f1a93d324dd0f2bff89963ab81ed6e0c2ee7e18c0827dc1d3576b2d9f26bbd0a" ); assert_eq!( - SecureStorageKey::ProfileSnapshot.identifier(), + SecureStorageKey::load_profile_snapshot().identifier(), "secure_storage_key_profile_snapshot" ); } @@ -57,7 +116,7 @@ mod uniffi_tests { #[test] fn identifier() { - let key = SecureStorageKey::ProfileSnapshot; + let key = SecureStorageKey::load_profile_snapshot(); assert_eq!( key.clone().identifier(), secure_storage_key_identifier(&key) diff --git a/crates/sargon/src/system/sargon_os/sargon_os_profile.rs b/crates/sargon/src/system/sargon_os/sargon_os_profile.rs index e3c927244..db8dcb763 100644 --- a/crates/sargon/src/system/sargon_os/sargon_os_profile.rs +++ b/crates/sargon/src/system/sargon_os/sargon_os_profile.rs @@ -164,7 +164,12 @@ impl SargonOS { let secure_storage = &self.secure_storage; secure_storage - .save(SecureStorageKey::ProfileSnapshot, profile) + .save( + SecureStorageKey::ProfileSnapshot { + profile_id: profile.id(), + }, + profile, + ) .await?; self.event_bus diff --git a/jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/driver/AndroidStorageDriver.kt b/jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/driver/AndroidStorageDriver.kt index 9508ea7ac..abe18ac6a 100644 --- a/jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/driver/AndroidStorageDriver.kt +++ b/jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/driver/AndroidStorageDriver.kt @@ -61,7 +61,6 @@ internal class AndroidStorageDriver( private fun SecureStorageKey.mapping() = when (this) { is SecureStorageKey.ProfileSnapshot -> ProfileSnapshotKeyMapping( - key = this, encryptedStorage = encryptedPreferencesDatastore ) diff --git a/jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/storage/key/ProfileSnapshotKeyMapping.kt b/jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/storage/key/ProfileSnapshotKeyMapping.kt index 8466ab3d4..21bc7f685 100644 --- a/jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/storage/key/ProfileSnapshotKeyMapping.kt +++ b/jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/storage/key/ProfileSnapshotKeyMapping.kt @@ -16,7 +16,6 @@ import kotlinx.coroutines.delay import java.io.IOException internal class ProfileSnapshotKeyMapping( - private val key: SecureStorageKey.ProfileSnapshot, private val encryptedStorage: DataStore ) : DatastoreKeyMapping { diff --git a/jvm/sargon-android/src/test/java/com/radixdlt/sargon/SecureStorageKeyTest.kt b/jvm/sargon-android/src/test/java/com/radixdlt/sargon/SecureStorageKeyTest.kt index 8ddc800fe..3dfc06594 100644 --- a/jvm/sargon-android/src/test/java/com/radixdlt/sargon/SecureStorageKeyTest.kt +++ b/jvm/sargon-android/src/test/java/com/radixdlt/sargon/SecureStorageKeyTest.kt @@ -23,7 +23,7 @@ class SecureStorageKeyTest { assertEquals( "secure_storage_key_profile_snapshot", - SecureStorageKey.ProfileSnapshot.identifier + SecureStorageKey.ProfileSnapshot(profileId = newProfileIdSample()).identifier ) } diff --git a/jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/storage/key/ByteArrayKeyMappingTest.kt b/jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/storage/key/ByteArrayKeyMappingTest.kt index c4cdf03e3..6b6b90587 100644 --- a/jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/storage/key/ByteArrayKeyMappingTest.kt +++ b/jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/storage/key/ByteArrayKeyMappingTest.kt @@ -1,10 +1,12 @@ package com.radixdlt.sargon.os.storage.key import androidx.datastore.preferences.core.PreferenceDataStoreFactory +import com.radixdlt.sargon.ProfileId import com.radixdlt.sargon.SecureStorageKey import com.radixdlt.sargon.UnsafeStorageKey import com.radixdlt.sargon.extensions.randomBagOfBytes import com.radixdlt.sargon.extensions.toByteArray +import com.radixdlt.sargon.newProfileIdSample import com.radixdlt.sargon.os.storage.EncryptionHelper import com.radixdlt.sargon.os.storage.KeySpec import com.radixdlt.sargon.os.storage.KeystoreAccessRequest @@ -63,7 +65,7 @@ class ByteArrayKeyMappingTest { fun testSecureStorageKeyRoundtrip() = runTest(context = testDispatcher) { // Even thought profile snapshot does not store data in byte array, // it is just used to facilitate the test - val key = SecureStorageKey.ProfileSnapshot + val key = SecureStorageKey.ProfileSnapshot(newProfileIdSample()) mockProfileAccessRequest() val sut = ByteArrayKeyMapping( diff --git a/jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/storage/key/ProfileSnapshotKeyMappingTest.kt b/jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/storage/key/ProfileSnapshotKeyMappingTest.kt index db58420aa..b9a1d3e6c 100644 --- a/jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/storage/key/ProfileSnapshotKeyMappingTest.kt +++ b/jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/storage/key/ProfileSnapshotKeyMappingTest.kt @@ -54,7 +54,6 @@ class ProfileSnapshotKeyMappingTest { mockProfileAccessRequest() val sut = ProfileSnapshotKeyMapping( - key = SecureStorageKey.ProfileSnapshot, encryptedStorage = storage ) @@ -108,7 +107,6 @@ class ProfileSnapshotKeyMappingTest { } val sut = ProfileSnapshotKeyMapping( - key = SecureStorageKey.ProfileSnapshot, encryptedStorage = storage ) @@ -123,7 +121,6 @@ class ProfileSnapshotKeyMappingTest { every { storage.data } returns flow { throw IOException() } val sut = ProfileSnapshotKeyMapping( - key = SecureStorageKey.ProfileSnapshot, encryptedStorage = storage ) @@ -137,7 +134,6 @@ class ProfileSnapshotKeyMappingTest { every { storage.data } returns flow { throw RuntimeException("some error") } val sut = ProfileSnapshotKeyMapping( - key = SecureStorageKey.ProfileSnapshot, encryptedStorage = storage )