-
Notifications
You must be signed in to change notification settings - Fork 25
[PM-18046] Implement session storage #547
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
Changes from all commits
d8eca1d
7f697c4
1a19e98
f1669a0
584ad45
a4828f4
3957ea5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| //! Generic session repository abstraction allowing IPC clients to choose between | ||
| //! SDK-managed (in-memory) and client-managed (JavaScript-backed) session storage. | ||
| //! | ||
| //! This is a workaround because wasm-bindgen does not handle generics. | ||
| //! | ||
| //! Use SDK-managed when state providers might not make sense, for example if they | ||
| //! will use insecure IPC to save the data, defeating the whole point of a secure session. | ||
| use std::sync::Arc; | ||
|
|
||
| use crate::{ | ||
| traits::{InMemorySessionRepository, SessionRepository}, | ||
| wasm::JsSessionRepository, | ||
| }; | ||
|
|
||
| // TODO: Change session type when implementing encryption | ||
| type Session = (); | ||
|
|
||
| pub enum GenericSessionRepository { | ||
| InMemory(Arc<InMemorySessionRepository<Session>>), | ||
| JsSessionRepository(Arc<JsSessionRepository>), | ||
| } | ||
|
Comment on lines
+19
to
+22
|
||
|
|
||
| impl SessionRepository<Session> for GenericSessionRepository { | ||
| type GetError = String; | ||
| type SaveError = String; | ||
| type RemoveError = String; | ||
|
|
||
| async fn get( | ||
| &self, | ||
| endpoint: crate::endpoint::Endpoint, | ||
| ) -> Result<Option<Session>, Self::GetError> { | ||
| match self { | ||
| GenericSessionRepository::InMemory(repo) => repo | ||
| .get(endpoint) | ||
| .await | ||
| .map_err(|_| "InMemorySessionRepository::get should never fail".to_owned()), | ||
| GenericSessionRepository::JsSessionRepository(repo) => { | ||
| <JsSessionRepository as SessionRepository<Session>>::get(repo.as_ref(), endpoint) | ||
| .await | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async fn save( | ||
| &self, | ||
| endpoint: crate::endpoint::Endpoint, | ||
| session: Session, | ||
| ) -> Result<(), Self::SaveError> { | ||
| match self { | ||
| GenericSessionRepository::InMemory(repo) => repo | ||
| .save(endpoint, session) | ||
| .await | ||
| .map_err(|_| "InMemorySessionRepository::save should never fail".to_owned()), | ||
| GenericSessionRepository::JsSessionRepository(repo) => { | ||
| <JsSessionRepository as SessionRepository<Session>>::save( | ||
| repo.as_ref(), | ||
| endpoint, | ||
| session, | ||
| ) | ||
| .await | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async fn remove(&self, endpoint: crate::endpoint::Endpoint) -> Result<(), Self::RemoveError> { | ||
| match self { | ||
| GenericSessionRepository::InMemory(repo) => repo | ||
| .remove(endpoint) | ||
| .await | ||
| .map_err(|_| "InMemorySessionRepository::remove should never fail".to_owned()), | ||
| GenericSessionRepository::JsSessionRepository(repo) => { | ||
| <JsSessionRepository as SessionRepository<Session>>::remove(repo.as_ref(), endpoint) | ||
| .await | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,10 @@ use crate::{ | |
| ipc_client::{IpcClientSubscription, ReceiveError, SubscribeError}, | ||
| message::{IncomingMessage, OutgoingMessage}, | ||
| traits::{InMemorySessionRepository, NoEncryptionCryptoProvider}, | ||
| wasm::{ | ||
| JsSessionRepository, RawJsSessionRepository, | ||
| generic_session_repository::GenericSessionRepository, | ||
| }, | ||
| }; | ||
|
|
||
| /// JavaScript wrapper around the IPC client. For more information, see the | ||
|
|
@@ -21,12 +25,7 @@ pub struct JsIpcClient { | |
| /// send typed messages, etc. For examples see | ||
| /// [wasm::ipc_register_discover_handler](crate::wasm::ipc_register_discover_handler). | ||
| pub client: Arc< | ||
| IpcClient< | ||
| NoEncryptionCryptoProvider, | ||
| JsCommunicationBackend, | ||
| // TODO: Change session provider to a JS-implemented one | ||
| InMemorySessionRepository<()>, | ||
| >, | ||
| IpcClient<NoEncryptionCryptoProvider, JsCommunicationBackend, GenericSessionRepository>, | ||
| >, | ||
| } | ||
|
|
||
|
|
@@ -51,14 +50,36 @@ impl JsIpcClientSubscription { | |
|
|
||
| #[wasm_bindgen(js_class = IpcClient)] | ||
| impl JsIpcClient { | ||
| #[allow(missing_docs)] | ||
| #[wasm_bindgen(constructor)] | ||
| pub fn new(communication_provider: &JsCommunicationBackend) -> JsIpcClient { | ||
| /// Create a new `IpcClient` instance with an in-memory session repository for saving | ||
| /// sessions within the SDK. | ||
| #[wasm_bindgen(js_name = newWithSdkInMemorySessions)] | ||
| pub fn new_with_sdk_in_memory_sessions( | ||
dani-garcia marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding 4 ๐จ: Constructor naming could better reflect the security distinction The PR description mentions that in-memory sessions are preferred "when state providers might not make sense, for example if they will use insecure IPC to save the data, defeating the whole point of a secure session." Consider renaming to emphasize this security aspect:
This would help JavaScript consumers make informed security decisions rather than viewing this as simply an implementation detail preference. |
||
| communication_provider: &JsCommunicationBackend, | ||
| ) -> JsIpcClient { | ||
| JsIpcClient { | ||
| client: IpcClient::new( | ||
| NoEncryptionCryptoProvider, | ||
| communication_provider.clone(), | ||
| GenericSessionRepository::InMemory(Arc::new(InMemorySessionRepository::new( | ||
| HashMap::new(), | ||
| ))), | ||
| ), | ||
| } | ||
| } | ||
| /// Create a new `IpcClient` instance with a client-managed session repository for saving | ||
| /// sessions using State Provider. | ||
| #[wasm_bindgen(js_name = newWithClientManagedSessions)] | ||
| pub fn new_with_client_managed_sessions( | ||
| communication_provider: &JsCommunicationBackend, | ||
| session_repository: RawJsSessionRepository, | ||
| ) -> JsIpcClient { | ||
| JsIpcClient { | ||
| client: IpcClient::new( | ||
| NoEncryptionCryptoProvider, | ||
| communication_provider.clone(), | ||
| InMemorySessionRepository::new(HashMap::new()), | ||
| GenericSessionRepository::JsSessionRepository(Arc::new(JsSessionRepository::new( | ||
| session_repository, | ||
| ))), | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| use bitwarden_threading::ThreadBoundRunner; | ||
| use serde::{Serialize, de::DeserializeOwned}; | ||
| use tsify::serde_wasm_bindgen; | ||
| use wasm_bindgen::prelude::*; | ||
|
|
||
| use crate::{endpoint::Endpoint, traits::SessionRepository}; | ||
|
|
||
| #[wasm_bindgen(typescript_custom_section)] | ||
| const TS_CUSTOM_TYPES: &'static str = r#" | ||
| export interface IpcSessionRepository { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding 3 ๐ญ: Missing documentation for error handling contract The TypeScript interface doesn't document what happens when these Promise methods throw errors. JavaScript consumers need to know:
Consider adding JSDoc comments to the interface explaining the error contract, especially since errors are converted to strings and not propagated meaningfully. |
||
| get(endpoint: Endpoint): Promise<any | undefined>; | ||
| save(endpoint: Endpoint, session: any): Promise<void>; | ||
| remove(endpoint: Endpoint): Promise<void>; | ||
| } | ||
| "#; | ||
|
|
||
| #[wasm_bindgen] | ||
| extern "C" { | ||
| /// JavaScript interface for handling outgoing messages from the IPC framework. | ||
| #[wasm_bindgen(js_name = IpcSessionRepository, typescript_type = "IpcSessionRepository")] | ||
| pub type RawJsSessionRepository; | ||
|
|
||
| /// Used by the IPC framework to get a session for a specific endpoint. | ||
| #[wasm_bindgen(catch, method, structural)] | ||
| pub async fn get(this: &RawJsSessionRepository, endpoint: Endpoint) | ||
| -> Result<JsValue, JsValue>; | ||
|
|
||
| /// Used by the IPC framework to save a session for a specific endpoint. | ||
| #[wasm_bindgen(catch, method, structural)] | ||
| pub async fn save( | ||
| this: &RawJsSessionRepository, | ||
| endpoint: Endpoint, | ||
| session: JsValue, | ||
| ) -> Result<(), JsValue>; | ||
|
|
||
| /// Used by the IPC framework to remove a session for a specific endpoint. | ||
| #[wasm_bindgen(catch, method, structural)] | ||
| pub async fn remove(this: &RawJsSessionRepository, endpoint: Endpoint) -> Result<(), JsValue>; | ||
| } | ||
|
|
||
| /// Thread safe JavaScript implementation of the `SessionRepository` trait for IPC sessions. | ||
| pub struct JsSessionRepository(ThreadBoundRunner<RawJsSessionRepository>); | ||
|
|
||
| impl JsSessionRepository { | ||
| /// Creates a new `JsSessionRepository` instance wrapping the raw JavaScript repository. | ||
| pub fn new(repository: RawJsSessionRepository) -> Self { | ||
| Self(ThreadBoundRunner::new(repository)) | ||
| } | ||
| } | ||
|
|
||
| impl Clone for JsSessionRepository { | ||
| fn clone(&self) -> Self { | ||
| Self(self.0.clone()) | ||
| } | ||
| } | ||
|
|
||
| impl<Session> SessionRepository<Session> for JsSessionRepository | ||
| where | ||
| Session: Serialize + DeserializeOwned + Send + Sync + 'static, | ||
| { | ||
| type GetError = String; | ||
| type SaveError = String; | ||
| type RemoveError = String; | ||
|
|
||
| async fn get(&self, endpoint: Endpoint) -> Result<Option<Session>, Self::GetError> { | ||
| self.0 | ||
| .run_in_thread(move |repo| async move { | ||
| let js_value = repo.get(endpoint).await.map_err(|e| format!("{e:?}"))?; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding 1 Converting errors to strings using ContextSince you mentioned in PR comments that "We don't need to catch these errors" and implementing proper error propagation across JS/WASM boundaries is costly, this is acceptable for the current use case. However, this creates technical debt if error handling requirements change later. Consider documenting this decision in a comment explaining that errors are intentionally simplified because they're not expected to be handled programmatically by consuming applications. |
||
| if js_value.is_undefined() || js_value.is_null() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| Ok(Some( | ||
| serde_wasm_bindgen::from_value(js_value).map_err(|e| e.to_string())?, | ||
| )) | ||
| }) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| } | ||
|
|
||
| async fn save(&self, endpoint: Endpoint, session: Session) -> Result<(), Self::SaveError> { | ||
| self.0 | ||
| .run_in_thread(move |repo| async move { | ||
| let js_value = serde_wasm_bindgen::to_value(&session).map_err(|e| e.to_string())?; | ||
| repo.save(endpoint, js_value) | ||
| .await | ||
| .map_err(|e| format!("{e:?}")) | ||
| }) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| } | ||
|
|
||
| async fn remove(&self, endpoint: Endpoint) -> Result<(), Self::RemoveError> { | ||
| self.0 | ||
| .run_in_thread(move |repo| async move { | ||
| repo.remove(endpoint).await.map_err(|e| format!("{e:?}")) | ||
| }) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,12 @@ | ||
| mod communication_backend; | ||
| mod discover; | ||
| mod generic_session_repository; | ||
| mod ipc_client; | ||
| mod js_session_repository; | ||
| mod message; | ||
|
|
||
| // Re-export types to make sure wasm_bindgen picks them up | ||
| pub use communication_backend::*; | ||
| pub use discover::*; | ||
| pub use ipc_client::*; | ||
| pub use js_session_repository::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finding 2 โป๏ธ: Hardcoded
Sessiontype creates technical debtThe hardcoded
type Session = ()means this enum cannot support typed sessions until refactored. When encryption is implemented, this will require either:The TODO acknowledges this, but consider whether the wasm-bindgen limitation could be worked around by: