From 128f78d99706b5ab7d057ac606b63b75cdd13e55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Wed, 7 Feb 2024 17:06:44 +0100 Subject: [PATCH] Add locking support to bitwarden-json to improve bindings thread safety --- Cargo.lock | 1 + crates/bitwarden-c/src/c.rs | 2 +- crates/bitwarden-c/src/macros/ffi.rs | 2 +- crates/bitwarden-json/Cargo.toml | 1 + crates/bitwarden-json/src/client.rs | 43 +++++++++++++++------------- crates/bitwarden-napi/src/client.rs | 2 +- crates/bitwarden-py/src/client.rs | 6 ++-- crates/bitwarden-wasm/src/client.rs | 18 ++++-------- 8 files changed, 37 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b2728124b..c4b8d76d1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -485,6 +485,7 @@ dependencies = [ name = "bitwarden-json" version = "0.3.0" dependencies = [ + "async-lock", "bitwarden", "log", "schemars", diff --git a/crates/bitwarden-c/src/c.rs b/crates/bitwarden-c/src/c.rs index f9ac57f30..934b20359 100644 --- a/crates/bitwarden-c/src/c.rs +++ b/crates/bitwarden-c/src/c.rs @@ -8,7 +8,7 @@ use crate::{box_ptr, ffi_ref}; #[tokio::main] pub async extern "C" fn run_command( c_str_ptr: *const c_char, - client_ptr: *mut Client, + client_ptr: *const Client, ) -> *mut c_char { let client = unsafe { ffi_ref!(client_ptr) }; let input_str = str::from_utf8(unsafe { CStr::from_ptr(c_str_ptr).to_bytes() }).unwrap(); diff --git a/crates/bitwarden-c/src/macros/ffi.rs b/crates/bitwarden-c/src/macros/ffi.rs index 3325838d1..d7384cd48 100644 --- a/crates/bitwarden-c/src/macros/ffi.rs +++ b/crates/bitwarden-c/src/macros/ffi.rs @@ -3,7 +3,7 @@ macro_rules! ffi_ref { ($name:ident) => {{ assert!(!$name.is_null()); - &mut *$name + &*$name }}; } diff --git a/crates/bitwarden-json/Cargo.toml b/crates/bitwarden-json/Cargo.toml index f7c0fd58d..1e0473c44 100644 --- a/crates/bitwarden-json/Cargo.toml +++ b/crates/bitwarden-json/Cargo.toml @@ -18,6 +18,7 @@ internal = ["bitwarden/internal"] # Internal testing methods secrets = ["bitwarden/secrets"] # Secrets manager API [dependencies] +async-lock = ">=3.3.0, <4.0" log = ">=0.4.18, <0.5" schemars = ">=0.8.12, <0.9" serde = { version = ">=1.0, <2.0", features = ["derive"] } diff --git a/crates/bitwarden-json/src/client.rs b/crates/bitwarden-json/src/client.rs index d484c7b50..ef9414f12 100644 --- a/crates/bitwarden-json/src/client.rs +++ b/crates/bitwarden-json/src/client.rs @@ -1,3 +1,4 @@ +use async_lock::Mutex; use bitwarden::client::client_settings::ClientSettings; #[cfg(feature = "secrets")] @@ -7,15 +8,15 @@ use crate::{ response::{Response, ResponseIntoString}, }; -pub struct Client(bitwarden::Client); +pub struct Client(Mutex); impl Client { pub fn new(settings_input: Option) -> Self { let settings = Self::parse_settings(settings_input); - Self(bitwarden::Client::new(settings)) + Self(Mutex::new(bitwarden::Client::new(settings))) } - pub async fn run_command(&mut self, input_str: &str) -> String { + pub async fn run_command(&self, input_str: &str) -> String { const SUBCOMMANDS_TO_CLEAN: &[&str] = &["Secrets"]; let mut cmd_value: serde_json::Value = match serde_json::from_str(input_str) { Ok(cmd) => cmd, @@ -44,41 +45,43 @@ impl Client { } }; + let mut client = self.0.lock().await; + match cmd { #[cfg(feature = "internal")] - Command::PasswordLogin(req) => self.0.auth().login_password(&req).await.into_string(), + Command::PasswordLogin(req) => client.auth().login_password(&req).await.into_string(), #[cfg(feature = "secrets")] Command::AccessTokenLogin(req) => { - self.0.auth().login_access_token(&req).await.into_string() + client.auth().login_access_token(&req).await.into_string() } #[cfg(feature = "internal")] - Command::GetUserApiKey(req) => self.0.get_user_api_key(&req).await.into_string(), + Command::GetUserApiKey(req) => client.get_user_api_key(&req).await.into_string(), #[cfg(feature = "internal")] - Command::ApiKeyLogin(req) => self.0.auth().login_api_key(&req).await.into_string(), + Command::ApiKeyLogin(req) => client.auth().login_api_key(&req).await.into_string(), #[cfg(feature = "internal")] - Command::Sync(req) => self.0.sync(&req).await.into_string(), + Command::Sync(req) => client.sync(&req).await.into_string(), #[cfg(feature = "internal")] - Command::Fingerprint(req) => self.0.platform().fingerprint(&req).into_string(), + Command::Fingerprint(req) => client.platform().fingerprint(&req).into_string(), #[cfg(feature = "secrets")] Command::Secrets(cmd) => match cmd { - SecretsCommand::Get(req) => self.0.secrets().get(&req).await.into_string(), + SecretsCommand::Get(req) => client.secrets().get(&req).await.into_string(), SecretsCommand::GetByIds(req) => { - self.0.secrets().get_by_ids(req).await.into_string() + client.secrets().get_by_ids(req).await.into_string() } - SecretsCommand::Create(req) => self.0.secrets().create(&req).await.into_string(), - SecretsCommand::List(req) => self.0.secrets().list(&req).await.into_string(), - SecretsCommand::Update(req) => self.0.secrets().update(&req).await.into_string(), - SecretsCommand::Delete(req) => self.0.secrets().delete(req).await.into_string(), + SecretsCommand::Create(req) => client.secrets().create(&req).await.into_string(), + SecretsCommand::List(req) => client.secrets().list(&req).await.into_string(), + SecretsCommand::Update(req) => client.secrets().update(&req).await.into_string(), + SecretsCommand::Delete(req) => client.secrets().delete(req).await.into_string(), }, #[cfg(feature = "secrets")] Command::Projects(cmd) => match cmd { - ProjectsCommand::Get(req) => self.0.projects().get(&req).await.into_string(), - ProjectsCommand::Create(req) => self.0.projects().create(&req).await.into_string(), - ProjectsCommand::List(req) => self.0.projects().list(&req).await.into_string(), - ProjectsCommand::Update(req) => self.0.projects().update(&req).await.into_string(), - ProjectsCommand::Delete(req) => self.0.projects().delete(req).await.into_string(), + ProjectsCommand::Get(req) => client.projects().get(&req).await.into_string(), + ProjectsCommand::Create(req) => client.projects().create(&req).await.into_string(), + ProjectsCommand::List(req) => client.projects().list(&req).await.into_string(), + ProjectsCommand::Update(req) => client.projects().update(&req).await.into_string(), + ProjectsCommand::Delete(req) => client.projects().delete(req).await.into_string(), }, } } diff --git a/crates/bitwarden-napi/src/client.rs b/crates/bitwarden-napi/src/client.rs index 712cd4ffd..f41d5c351 100644 --- a/crates/bitwarden-napi/src/client.rs +++ b/crates/bitwarden-napi/src/client.rs @@ -38,7 +38,7 @@ impl BitwardenClient { } #[napi] - pub async unsafe fn run_command(&mut self, command_input: String) -> String { + pub async fn run_command(&self, command_input: String) -> String { self.0.run_command(&command_input).await } } diff --git a/crates/bitwarden-py/src/client.rs b/crates/bitwarden-py/src/client.rs index 1cfd078bb..f1d282b41 100644 --- a/crates/bitwarden-py/src/client.rs +++ b/crates/bitwarden-py/src/client.rs @@ -13,12 +13,12 @@ impl BitwardenClient { } #[pyo3(text_signature = "($self, command_input)")] - fn run_command(&mut self, command_input: String) -> String { - run_command(&mut self.0, &command_input) + fn run_command(&self, command_input: String) -> String { + run_command(&self.0, &command_input) } } #[tokio::main] -async fn run_command(client: &mut JsonClient, input_str: &str) -> String { +async fn run_command(client: &JsonClient, input_str: &str) -> String { client.run_command(input_str).await } diff --git a/crates/bitwarden-wasm/src/client.rs b/crates/bitwarden-wasm/src/client.rs index b9f6723a6..542759731 100644 --- a/crates/bitwarden-wasm/src/client.rs +++ b/crates/bitwarden-wasm/src/client.rs @@ -1,5 +1,5 @@ extern crate console_error_panic_hook; -use std::{rc::Rc, sync::RwLock}; +use std::rc::Rc; use bitwarden_json::client::Client as JsonClient; use js_sys::Promise; @@ -26,10 +26,10 @@ fn convert_level(level: LogLevel) -> Level { } } -// Rc> is to avoid needing to take ownership of the Client during our async run_command +// Rc<...> is to avoid needing to take ownership of the Client during our async run_command // function https://github.com/rustwasm/wasm-bindgen/issues/2195#issuecomment-799588401 #[wasm_bindgen] -pub struct BitwardenClient(Rc>); +pub struct BitwardenClient(Rc); #[wasm_bindgen] impl BitwardenClient { @@ -42,20 +42,14 @@ impl BitwardenClient { panic!("failed to initialize logger: {:?}", e); } - Self(Rc::new(RwLock::new(bitwarden_json::client::Client::new( - settings_input, - )))) + Self(Rc::new(bitwarden_json::client::Client::new(settings_input))) } #[wasm_bindgen] - pub fn run_command(&mut self, js_input: String) -> Promise { + pub fn run_command(&self, js_input: String) -> Promise { let rc = self.0.clone(); - // TODO: We should probably switch to an async-aware RwLock here, - // but it probably doesn't matter much in a single threaded environment - #[allow(clippy::await_holding_lock)] future_to_promise(async move { - let mut client = rc.write().unwrap(); - let result = client.run_command(&js_input).await; + let result = rc.run_command(&js_input).await; Ok(result.into()) }) }