Details
- Reviewers
varun marcin kamil - Commits
- rCOMM57d2efb99127: [native] Expose SecureStore to Rust
Run this code at the beginning of create_backup function:
println!(
"{:?}\n{:?}\n{:?}\n{:?}\n",
// Value that doesn't exist
secure_store_get("non_existant"),
// Value that does exist
secure_store_get("DATABASE_MANAGER_STATUS"),
// Test that values can be set
secure_store_set(
"rust_testing_secure_store",
// Pseudo-random value from `backup_id` in `create_backup`
backup_id.chars().next().unwrap_or('X').to_string()
),
secure_store_get("rust_testing_secure_store")
);
let non_existant = secure_store::get("non_existant").await;
let device_id = secure_store::get("DATABASE_MANAGER_STATUS").await;
// Test that promises are run on order (if awaited) and that
// "rust_testing_stuff" will always have the newest value
// Used promise_id as a "random" value for testing
let set_result =
secure_store::set("rust_testing_stuff", promise_id.to_string()).await;
let set_test = secure_store::get("rust_testing_stuff").await;
let ret = format!(
"
SecureStore (nonExistant): {non_existant:?}
SecureStore (dbStatus): {device_id:?}
SecureStore set: {set_result:?}
SecureStore (setTest): {set_test:?}"
);Results:
Err(Exception { what: "Empty Optional cannot be unwrapped" })
Ok("WORKABLE")
Ok(())
Ok("F")where the last value changes every call.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/native_rust_library/src/cxx_promise_manager.rs | ||
|---|---|---|
| 57 ↗ | (On Diff #33392) | Wasn't sure what to do here. This should never happen (mutex becomes poisoned) but I didn't want to unwrap to not quit the whole app. |
| native/native_rust_library/src/secure_store.rs | ||
|---|---|---|
| 6–8 ↗ | (On Diff #33392) | Looks like there is a "round trip":
Is there a reason on why can't we just create a promise in Rust that directly gets the result of C++ calculations and returns them? We would have just Rust call C++, while now Rust calls C++, that calls Rust back. |
| native/native_rust_library/RustSecureStore.cpp | ||
|---|---|---|
| 9 ↗ | (On Diff #33392) | Sorry for not catching this earlier but from my perspective there is no reason to run this code on GlobalDBSingleton. GlobalDBSingleton is used to ensure that database operations run sequentially on one thread. The reason some calls to CommSecureStore are placed on GlobalDBSingleton is that we keep there several flags that indicate health condition of database file so we semantically treat them as database operations. In this case however you are going to retrieve data that are stored outside of SQLite so there is no reason yo run them on GlobalDBSingleton. @kamil - can you confirm my statement? |
| native/native_rust_library/src/secure_store.rs | ||
| 6–8 ↗ | (On Diff #33392) | I discussed this with @michal privately. I was told that such design is enforced by the fact that CommSecureStore accesses run on dedicated C++ thread asynchronously. If I am correct in my previous comment (cc @kamil ) then we don't need to run CommSecureStore accesses on dedicated thread and this code can be rewritten to avoid Rust -> C++ -> Rust round trip. On the other hand if I am wrong and we must run CommSecureStore accesses on dedicated thread then this approach is fine from my perspective. |
| native/native_rust_library/RustSecureStore.cpp | ||
|---|---|---|
| 9 ↗ | (On Diff #33392) | In D9224 there were functions added for storing auth information in secure store (userID, deviceID, commServicesAccessToken), that I will need to get for the backup networking client. It's run inside of the GlobalDBSingleton (see setCommServicesAuthMetadata function). Is it also a mistake, or is there some reason that we run it there? One possible reason I can see is synchronization - we want to set them all "atomically". |
| native/native_rust_library/RustSecureStore.cpp | ||
|---|---|---|
| 9 ↗ | (On Diff #33392) | Please accept my apologies for not answering this question earlier. As far as I am concerned running commServicesAccessToken related code on GlobalDBSingleton is unnecessary.
Setting things in CommSecureStore is atomic. Think of it as a key-value database that is synchronized by the OS. There is no reason to run this staff on GlobalDBSingleton and Rust can call CommSecureStore directly without "round-trip" with promises. |
Accepting but please make sure you either:
- refactor the code not to use promises and call C++ directly from Rust (in other words remove GlobalDBSingleton from this code)
OR
- if the above is too much of a hassle and might cause you missing your goal then create high priority follow-up task to remove GlobalDBSingleton from this code.