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

Entities linked to Factor Source #302

Merged
merged 17 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,9 @@
public func deleteDataForKey(key: SecureStorageKey) async throws {
dictionary.removeValue(forKey: key)
}

public func containsDataForKey(key: SecureStorageKey) async throws -> Bool {
dictionary.keys.contains(key)
}

Check warning on line 31 in apple/Sources/Sargon/Drivers/TestDrivers/SecureStorage/SecureStorageDriver+InsecureTestOnlyEphemeral_SecureStorage.swift

View check run for this annotation

Codecov / codecov/patch

apple/Sources/Sargon/Drivers/TestDrivers/SecureStorage/SecureStorageDriver+InsecureTestOnlyEphemeral_SecureStorage.swift#L29-L31

Added lines #L29 - L31 were not covered by tests
}
#endif
1 change: 1 addition & 0 deletions apple/Sources/Sargon/Drivers/TestDrivers/TestDrivers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ extension BIOS {
BIOS(
bundle: bundle,
userDefaultsSuite: userDefaultsSuite,
unsafeStorageKeyMapping: [:],
secureStorageDriver: Insecure︕!TestOnly︕!Ephemeral︕!SecureStorage(
keychainService: "test"
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,22 @@ extension UnsafeStorageDriver where Self == UnsafeStorage {
public static var shared: Self { Self.shared }
}

public typealias UnsafeStorageKeyMapping = [UnsafeStorageKey: String]

// MARK: - UnsafeStorage
/// An `UnsafeStorageDriver` implementation which
/// wraps `UserDefaults`.
public final class UnsafeStorage: Sendable {
public typealias Key = UnsafeStorageKey
fileprivate let userDefaults: UserDefaults
public init(userDefaults: UserDefaults = .standard) {

/// A dictionary containing the custom String value used for a given `UnsafeStorageKey`.
/// This is necessary since some UserDefaults were saved by the Host apps prior to Sargon.
fileprivate let keyMapping: [UnsafeStorageKey: String]

public init(userDefaults: UserDefaults = .standard, keyMapping: [UnsafeStorageKey: String] = [:]) {
self.userDefaults = userDefaults
self.keyMapping = keyMapping
}

/// Singleton `UnsafeStorageDriver` of type `UnsafeStorage,
Expand All @@ -30,22 +38,30 @@ public final class UnsafeStorage: Sendable {
extension UnsafeStorageKey {
/// Translates this `UnsafeStorageKey` into a String
/// identifier which we can use with `UserDefaults`
var identifier: String {
public var identifier: String {
unsafeStorageKeyIdentifier(key: self)
}
}

// MARK: - UnsafeStorage + UnsafeStorageDriver
extension UnsafeStorage: UnsafeStorageDriver {
public func loadData(key: Key) -> Data? {
userDefaults.data(forKey: key.identifier)
userDefaults.data(forKey: identifier(for: key))
}

public func saveData(key: Key, data: Data) {
userDefaults.setValue(data, forKey: key.identifier)
userDefaults.setValue(data, forKey: identifier(for: key))
}

public func deleteDataForKey(key: Key) {
userDefaults.removeObject(forKey: key.identifier)
userDefaults.removeObject(forKey: identifier(for: key))
}

private func identifier(for key: Key) -> String {
if let mapped = keyMapping[key] {
mapped
} else {
key.identifier
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ extension BIOS {
public convenience init(
bundle: Bundle,
userDefaultsSuite: String,
unsafeStorageKeyMapping: UnsafeStorageKeyMapping,
secureStorageDriver: SecureStorageDriver
) {
let drivers = Drivers(
bundle: bundle,
userDefaultsSuite: userDefaultsSuite,
unsafeStorageKeyMapping: unsafeStorageKeyMapping,
secureStorageDriver: secureStorageDriver
)
// https://en.wikipedia.org/wiki/Power-on_self-test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,29 @@ extension Drivers {
public convenience init(
bundle: Bundle,
userDefaultsSuite: String,
unsafeStorageKeyMapping: UnsafeStorageKeyMapping,
secureStorageDriver: SecureStorageDriver
) {
self.init(
appVersion: (bundle.infoDictionary?["CFBundleShortVersionString"] as? String) ?? "Unknown",
userDefaultsSuite: userDefaultsSuite,
unsafeStorageKeyMapping: unsafeStorageKeyMapping,
secureStorageDriver: secureStorageDriver
)
}

public convenience init(
appVersion: String,
userDefaultsSuite: String,
unsafeStorageKeyMapping: UnsafeStorageKeyMapping,
secureStorageDriver: SecureStorageDriver
) {
self.init(
secureStorage: secureStorageDriver,
hostInfo: AppleHostInfoDriver(appVersion: appVersion),
unsafeStorage: UnsafeStorage(
userDefaults: .init(suiteName: userDefaultsSuite)!
userDefaults: .init(suiteName: userDefaultsSuite)!,
keyMapping: unsafeStorageKeyMapping
)
)
}
Expand Down
2 changes: 2 additions & 0 deletions apple/Sources/Sargon/SargonOS/TestOS.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ extension BIOS {
public static func test(
bundle: Bundle = .main,
userDefaultsSuite: String = "Test",
unsafeStorageKeyMapping: UnsafeStorageKeyMapping = [:],
secureStorageDriver: SecureStorageDriver
) -> BIOS {
BIOS(
bundle: bundle,
userDefaultsSuite: userDefaultsSuite,
unsafeStorageKeyMapping: unsafeStorageKeyMapping,
secureStorageDriver: secureStorageDriver
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ extension Drivers {
Drivers(
appVersion: "0.0.1",
userDefaultsSuite: "works.rdx",
unsafeStorageKeyMapping: [:],
secureStorageDriver: Insecure︕!TestOnly︕!Ephemeral︕!SecureStorage(
keychainService: "test"
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,24 @@ class UnsafeStorageDriverTests: DriverTest<UnsafeStorage> {
sut.deleteDataForKey(key: key)
XCTAssertNil(sut.loadData(key: key))
}

func test_keyMapping() async throws {
// Set up SUT with keyMapping that uses custom key
let key = "custom_key"
let keyMapping: UnsafeStorageKeyMapping = [.factorSourceUserHasWrittenDown: key]
let data = Data([0x01])
let sut = SUT(keyMapping: keyMapping)

// Make sure there is no value on UserDefault for custom key
UserDefaults.standard.removeObject(forKey: key)
XCTAssertNil(UserDefaults.standard.value(forKey: key))

// Save the value via SUT
sut.saveData(key: .factorSourceUserHasWrittenDown, data: data)

// Verify the data is saved on UserDefaults using custom key, and not with the one Sargon defines
XCTAssertEqual(sut.loadData(key: .factorSourceUserHasWrittenDown), data)
XCTAssertEqual(UserDefaults.standard.data(forKey: key), data)
XCTAssertNil(UserDefaults.standard.data(forKey: UnsafeStorageKey.factorSourceUserHasWrittenDown.identifier))
}
}
3 changes: 2 additions & 1 deletion apple/Tests/IntegrationTests/SargonOS/BIOSTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@ extension BIOS {
static func creatingShared(
bundle: Bundle = .main,
userDefaultsSuite: String = "test",
unsafeStorageKeyMapping: UnsafeStorageKeyMapping = [:],
secureStorageDriver: SecureStorageDriver = Insecure︕!TestOnly︕!Ephemeral︕!SecureStorage(
keychainService: "test"
)
) -> BIOS {
creatingShared(drivers: .init(bundle: bundle, userDefaultsSuite: userDefaultsSuite, secureStorageDriver: secureStorageDriver))
creatingShared(drivers: .init(bundle: bundle, userDefaultsSuite: userDefaultsSuite, unsafeStorageKeyMapping: unsafeStorageKeyMapping, secureStorageDriver: secureStorageDriver))
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/sargon-uniffi/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "sargon-uniffi"
# Don't forget to update version in crates/sargon/Cargo.toml
version = "1.1.84"
version = "1.1.85"
edition = "2021"
build = "build.rs"

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use crate::prelude::*;
use sargon::EntitiesLinkedToFactorSource as InternalEntitiesLinkedToFactorSource;

/// This is the result of checking what entities are controlled by a given `FactorSource`.
#[derive(Clone, PartialEq, InternalConversion, uniffi::Record)]
pub struct EntitiesLinkedToFactorSource {
/// The integrity of the factor source.
pub integrity: FactorSourceIntegrity,

/// The visible accounts linked to the factor source.
pub accounts: Vec<Account>,

/// The hidden accounts linked to the factor source.
pub hidden_accounts: Vec<Account>,

/// The visible personas linked to the factor source.
pub personas: Vec<Persona>,

/// The hidden personas linked to the factor source.
pub hidden_personas: Vec<Persona>,
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
use crate::prelude::*;
use sargon::DeviceFactorSourceIntegrity as InternalDeviceFactorSourceIntegrity;

/// A struct representing the integrity of a device factor source.
#[derive(Clone, PartialEq, Eq, InternalConversion, uniffi::Record)]
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,
}
Copy link
Contributor

@giannis-rdx giannis-rdx Dec 13, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

we have similar thing in Android mnemonicExist

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it to replace every reference to _in_keychain to _in_secure_stroage.

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
use crate::prelude::*;
use sargon::FactorSourceIntegrity as InternalFactorSourceIntegrity;

/// An enum representing the integrity of a factor source.
#[derive(Clone, PartialEq, InternalConversion, uniffi::Enum)]
pub enum FactorSourceIntegrity {
Device(DeviceFactorSourceIntegrity),

Ledger(LedgerHardwareWalletFactorSource),
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
mod device;
mod integrity;

pub use device::*;
pub use integrity::*;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
mod entities_linked_to_factor_source;
mod integrity;
mod profile_to_check;

pub use entities_linked_to_factor_source::*;
pub use integrity::*;
pub use profile_to_check::*;
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
use crate::prelude::*;
use sargon::ProfileToCheck as InternalProfileToCheck;

/// The Profile to which we want to check the entities linked to a factor source.
#[derive(Clone, PartialEq, InternalConversion, uniffi::Enum)]
#[allow(clippy::large_enum_variant)]
pub enum ProfileToCheck {
/// We should check against the current Profile.
Current,

/// We should check against a specific Profile.
/// Useful when we are in the Import Mnenmonics flow.
Specific(Profile),
}
2 changes: 2 additions & 0 deletions crates/sargon-uniffi/src/profile/v100/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
mod address;
mod app_preferences;
mod entities_linked_to_factor_source;
mod entity;
mod entity_security_state;
mod factors;
Expand All @@ -11,6 +12,7 @@ mod profile_file_contents;

pub use address::*;
pub use app_preferences::*;
pub use entities_linked_to_factor_source::*;
pub use entity::*;
pub use entity_security_state::*;
pub use factors::*;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

&self,
key: SecureStorageKey,
) -> Result<bool>;
}

#[derive(Debug)]
Expand Down Expand Up @@ -58,4 +63,14 @@ impl InternalSecureStorageDriver for SecureStorageDriverAdapter {
.await
.into_internal_result()
}

async fn contains_data_for_key(
&self,
key: InternalSecureStorageKey,
) -> InternalResult<bool> {
self.wrapped
.contains_data_for_key(key.into())
.await
.into_internal_result()
}
}
2 changes: 2 additions & 0 deletions crates/sargon-uniffi/src/system/sargon_os/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ mod pre_authorization;
mod profile_state_holder;
mod sargon_os;
mod sargon_os_accounts;
mod sargon_os_entities_linked_to_factor_source;
mod sargon_os_factors;
mod sargon_os_gateway;
mod sargon_os_profile;
Expand All @@ -17,6 +18,7 @@ pub use pre_authorization::*;
pub use profile_state_holder::*;
pub use sargon_os::*;
pub use sargon_os_accounts::*;
pub use sargon_os_entities_linked_to_factor_source::*;
pub use sargon_os_factors::*;
pub use sargon_os_gateway::*;
pub use sargon_os_profile::*;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
use crate::prelude::*;

#[uniffi::export]
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> {
self.wrapped
.entities_linked_to_factor_source(
factor_source.into_internal(),
profile_to_check.into_internal(),
)
.await
.into_result()
}
}
2 changes: 1 addition & 1 deletion crates/sargon/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "sargon"
# Don't forget to update version in crates/sargon-uniffi/Cargo.toml
version = "1.1.84"
version = "1.1.85"
edition = "2021"
build = "build.rs"

Expand Down
Loading
Loading