Add more detail to tests for readability

apps/desktop/desktop_native/autofill_provider/src/assertion.rs

@@ -6,10 +6,10 @@ use serde::{Deserialize, Serialize};
 use crate::TimedCallback;
 use crate::{BitwardenError, Callback, Position, UserVerification};
 
+/// Request to assert a credential.
 #[cfg_attr(target_os = "macos", derive(uniffi::Record))]
 #[derive(Debug, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
-/// Request to assert a credential.
 pub struct PasskeyAssertionRequest {
     /// Relying Party ID for the request.
     pub rp_id: String,
@@ -71,17 +71,17 @@ pub struct PasskeyAssertionWithoutUserInterfaceRequest {
     /// The allowed credential ID for the request.
     pub credential_id: Vec<u8>,
 
-    #[cfg(target_os = "macos")]
     /// The user name for the credential that was previously given to the OS.
+    #[cfg(target_os = "macos")]
     pub user_name: String,
 
-    #[cfg(target_os = "macos")]
     /// The user ID for the credential that was previously given to the OS.
+    #[cfg(target_os = "macos")]
     pub user_handle: Vec<u8>,
 
-    #[cfg(target_os = "macos")]
     /// The app-specific local identifier for the credential, in our case, the
     /// cipher ID.
+    #[cfg(target_os = "macos")]
     pub record_identifier: Option<String>,
 
     /// SHA-256 hash of the `clientDataJSON` for the assertion request.
@@ -102,7 +102,6 @@ pub struct PasskeyAssertionWithoutUserInterfaceRequest {
     /// On Windows, this must be logical pixels, not physical pixels.
     pub window_xy: Position,
 
-    #[cfg(not(target_os = "macos"))]
     /// Byte string representing the native OS window handle for the WebAuthn client.
     /// # Operating System Differences
     ///
@@ -111,6 +110,7 @@ pub struct PasskeyAssertionWithoutUserInterfaceRequest {
     ///
     /// ## Windows
     /// On Windows, this is a HWND.
+    #[cfg(not(target_os = "macos"))]
     pub client_window_handle: Vec<u8>,
 
     // #[cfg(not(target_os = "macos"))]
@@ -150,8 +150,8 @@ pub struct PasskeyAssertionResponse {
     pub credential_id: Vec<u8>,
 }
 
-#[cfg_attr(target_os = "macos", uniffi::export(with_foreign))]
 /// Callback to process a response to passkey assertion request.
+#[cfg_attr(target_os = "macos", uniffi::export(with_foreign))]
 pub trait PreparePasskeyAssertionCallback: Send + Sync {
     /// Function to call if a successful response is returned.
     fn on_complete(&self, credential: PasskeyAssertionResponse);

apps/desktop/desktop_native/autofill_provider/src/lib.rs

@@ -50,20 +50,20 @@ uniffi::setup_scaffolding!();
 #[cfg(target_os = "macos")]
 static INIT: Once = Once::new();
 
+/// User verification preference for WebAuthn requests.
 #[cfg_attr(target_os = "macos", derive(uniffi::Enum))]
 #[derive(Debug, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
-/// User verification preference for WebAuthn requests.
 pub enum UserVerification {
     Preferred,
     Required,
     Discouraged,
 }
 
+/// Coordinates representing a point on the screen.
 #[cfg_attr(target_os = "macos", derive(uniffi::Record))]
 #[derive(Debug, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
-/// Coordinates representing a point on the screen.
 pub struct Position {
     pub x: i32,
     pub y: i32,
@@ -97,16 +97,15 @@ trait Callback: Send + Sync {
     fn error(&self, error: BitwardenError);
 }
 
-#[cfg_attr(target_os = "macos", derive(uniffi::Enum))]
-#[derive(Debug)]
 /// Store the connection status between the credential provider extension
 /// and the desktop application's IPC server.
+#[cfg_attr(target_os = "macos", derive(uniffi::Enum))]
+#[derive(Debug)]
 pub enum ConnectionStatus {
     Connected,
     Disconnected,
 }
 
-#[cfg_attr(target_os = "macos", derive(uniffi::Object))]
 /// A client to send and receive messages to the autofill service on the desktop
 /// client.
 ///
@@ -155,6 +154,7 @@ pub enum ConnectionStatus {
 ///     // use client here
 /// }
 /// ```
+#[cfg_attr(target_os = "macos", derive(uniffi::Object))]
 pub struct AutofillProviderClient {
     to_server_send: tokio::sync::mpsc::Sender<String>,
 
@@ -167,10 +167,10 @@ pub struct AutofillProviderClient {
     connection_status: Arc<std::sync::atomic::AtomicBool>,
 }
 
-#[derive(Serialize, Deserialize)]
-#[serde(rename_all = "camelCase")]
 /// Store native desktop status information to use for IPC communication
 /// between the application and the credential provider.
+#[derive(Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
 pub struct NativeStatus {
     key: String,
     value: String,
@@ -288,11 +288,11 @@ impl AutofillProviderClient {
 
 #[cfg_attr(target_os = "macos", uniffi::export)]
 impl AutofillProviderClient {
-    #[cfg_attr(target_os = "macos", uniffi::constructor)]
     /// Asynchronously initiates a connection to the autofill service on the desktop client.
     ///
     /// See documentation at the top-level of [this struct][AutofillProviderClient] for usage
     /// information.
+    #[cfg_attr(target_os = "macos", uniffi::constructor)]
     pub fn connect() -> Self {
         tracing::trace!("Autofill provider attempting to connect to Electron IPC...");
         let path = desktop_core::ipc::path(IPC_PATH);
@@ -451,8 +451,8 @@ fn send_message_helper(
     Ok(())
 }
 
-#[derive(Debug)]
 /// Types of errors for callbacks.
+#[derive(Debug)]
 pub enum CallbackError {
     Timeout,
     Cancelled,
@@ -583,6 +583,7 @@ impl PreparePasskeyRegistrationCallback for TimedCallback<PasskeyRegistrationRes
 #[cfg(test)]
 mod tests {
     use std::{
+        path::PathBuf,
         sync::{atomic::AtomicU32, Arc},
         time::Duration,
     };
@@ -597,41 +598,53 @@ mod tests {
         SerializedMessage, TimedCallback, IPC_PATH,
     };
 
+    /// Generates a path for a server and client to connect with.
+    ///
+    /// [`AutofillProviderClient`] is currently hardcoded to use sockets from the filesystem.
+    /// In order for paths not to conflict between tests, we use a counter and add it to the path name.
+    fn get_server_path() -> PathBuf {
+        static SERVER_COUNTER: AtomicU32 = AtomicU32::new(0);
+        let counter = SERVER_COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+        let name = format!("{}-{}", IPC_PATH, counter);
+        desktop_core::ipc::path(&name)
+    }
+
+    /// Sets up an in-memory server based on the passed handler and returns a client to the server.
     fn get_client<
         F: Fn(Result<Value, BitwardenError>) -> Result<Value, BitwardenError> + Send + 'static,
     >(
         handler: F,
     ) -> AutofillProviderClient {
-        static SERVER_COUNTER: AtomicU32 = AtomicU32::new(0);
         let (signal_tx, signal_rx) = std::sync::mpsc::channel();
-        // use a counter to allow tests to run in parallel without interfering with each other.
-        let counter = SERVER_COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
-        let name = format!("{}-{}", IPC_PATH, counter);
-        let path = desktop_core::ipc::path(&name);
+        let path = get_server_path();
         let server_path = path.clone();
+
+        // Start server thread
         std::thread::spawn(move || {
             let _span = tracing::span!(Level::DEBUG, "server").entered();
-            tracing::info!("[server] Starting server thread");
+            tracing::info!("Starting server thread");
             let (tx, mut rx) = mpsc::channel(8);
             let rt = tokio::runtime::Builder::new_current_thread()
                 .enable_io()
                 .build()
                 .unwrap();
             rt.block_on(async move {
-                tracing::debug!("[server] starting server at {server_path:?}");
+                tracing::debug!("Starting server at {server_path:?}");
                 let server = desktop_core::ipc::server::Server::start(&server_path, tx).unwrap();
-                tracing::debug!("[server] Server started");
+
+                // Signal to main thread that the server is ready to process messages.
+                tracing::debug!(" Server started");
                 signal_tx.send(()).unwrap();
-                tracing::debug!("[server] Waiting for messages");
+
+                // Handle incoming messages
+                tracing::debug!("Waiting for messages");
                 while let Some(data) = rx.recv().await {
+                    tracing::debug!("Received {data:?}");
                     match data.kind {
-                        MessageType::Connected => tracing::debug!("[server] Connected"),
-                        MessageType::Disconnected => tracing::debug!("[server] Disconnected"),
+                        MessageType::Connected => {}
+                        MessageType::Disconnected => {}
                         MessageType::Message => {
-                            tracing::debug!(
-                                "[server] received {}",
-                                data.message.as_ref().unwrap().to_string()
-                            );
+                            // Deserialize and handle messages using the given handler function.
                             let msg: SerializedMessage =
                                 serde_json::from_str(&data.message.unwrap()).unwrap();
 
@@ -652,38 +665,54 @@ mod tests {
                 }
             });
         });
-        tracing::debug!("[client] waiting for server...");
+
+        // Wait for server to startup and client to connect to server before returning client to test method.
+        let _span = tracing::span!(Level::DEBUG, "client");
+        tracing::debug!("Waiting for server...");
         signal_rx.recv_timeout(Duration::from_millis(1000)).unwrap();
-        tracing::debug!("[client] Starting client...");
+
+        // This starts a background task to connect to the server.
+        tracing::debug!("Starting client...");
         let client = AutofillProviderClient::connect_to_path(path.to_path_buf());
-        tracing::debug!("[client] Client connecting...");
-        for _ in 0..10 {
+
+        // The client connects to the server asynchronously in a background
+        // thread, so wait for client to report itself as Connected so that test
+        // methods don't have to do this everytime.
+        // Note, this has the potential to be flaky on a very busy server, but that's unavoidable with the current API.
+        tracing::debug!("Client connecting...");
+        for _ in 0..20 {
             if let ConnectionStatus::Connected = client.get_connection_status() {
                 break;
             }
             std::thread::sleep(Duration::from_millis(10));
         }
+
         assert!(matches!(
             client.get_connection_status(),
             ConnectionStatus::Connected
         ));
+
         client
     }
 
     #[test]
     fn test_disconnected_gives_error() {
-        let client = AutofillProviderClient::connect();
+        // There is no server running at this path, so this client should always be disconnected.
+        let client = AutofillProviderClient::connect_to_path(get_server_path());
+
+        // use an arbitrary request to test whether the client is disconnected.
         let callback = Arc::new(TimedCallback::new());
         client.get_lock_status(callback.clone());
         let response = callback
             .wait_for_response(Duration::from_millis(10), None)
             .unwrap();
+
         assert!(matches!(response, Err(BitwardenError::Disconnected)));
     }
 
     #[test]
     fn test_get_lock_status() {
-        // tracing_subscriber::fmt().init();
+        // The server should expect a lock status request and return a valid response.
         let handler = |value: Result<Value, BitwardenError>| {
             let value = value.unwrap();
             if let Ok(LockStatusRequest {}) = serde_json::from_value(value.clone()) {
@@ -694,6 +723,8 @@ mod tests {
                 )))
             }
         };
+
+        // send a lock status request
         let client = get_client(handler);
         let callback = Arc::new(TimedCallback::new());
         client.get_lock_status(callback.clone());
@@ -701,6 +732,7 @@ mod tests {
             .wait_for_response(Duration::from_millis(3000), None)
             .unwrap()
             .unwrap();
+
         assert!(response.is_unlocked);
     }
 }

apps/desktop/desktop_native/autofill_provider/src/lock_status.rs

@@ -4,12 +4,12 @@ use serde::{Deserialize, Serialize};
 
 use crate::{BitwardenError, Callback, TimedCallback};
 
-/// Request to retrieve the lock status of the desktop client.
 #[derive(Debug, Serialize, Deserialize)]
+/// Request to retrieve the lock status of the desktop client.
 pub(super) struct LockStatusRequest {}
 
-/// Response for the lock status of the desktop client.
 #[derive(Debug, Deserialize)]
+/// Response for the lock status of the desktop client.
 pub struct LockStatusResponse {
     /// Whether the desktop client is unlocked.
     #[serde(rename = "isUnlocked")]

apps/desktop/desktop_native/autofill_provider/src/registration.rs

@@ -4,10 +4,10 @@ use serde::{Deserialize, Serialize};
 
 use crate::{BitwardenError, Callback, Position, UserVerification};
 
+/// Request to create a credential.
 #[cfg_attr(target_os = "macos", derive(uniffi::Record))]
 #[derive(Debug, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
-/// Request to create a credential.
 pub struct PasskeyRegistrationRequest {
     /// Relying Party ID for the request.
     pub rp_id: String,
@@ -42,7 +42,6 @@ pub struct PasskeyRegistrationRequest {
     /// List of excluded credential IDs.
     pub excluded_credentials: Vec<Vec<u8>>,
 
-    #[cfg(not(target_os = "macos"))]
     /// Byte string representing the native OS window handle for the WebAuthn client.
     /// # Operating System Differences
     ///
@@ -51,9 +50,9 @@ pub struct PasskeyRegistrationRequest {
     ///
     /// ## Windows
     /// On Windows, this is a HWND.
+    #[cfg(not(target_os = "macos"))]
     pub client_window_handle: Vec<u8>,
 
-    #[cfg(not(target_os = "macos"))]
     /// Native context required for callbacks to the OS. Format differs by OS.
     /// # Operating System Differences
     ///
@@ -63,13 +62,14 @@ pub struct PasskeyRegistrationRequest {
     /// ## Windows
     /// On Windows, this is a base64-string representing the following data:
     /// `request transaction id (GUID, 16 bytes) || SHA-256(pluginOperationRequest)`
+    #[cfg(not(target_os = "macos"))]
     pub context: String,
 }
 
+/// Response for a passkey registration request.
 #[cfg_attr(target_os = "macos", derive(uniffi::Record))]
 #[derive(Debug, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
-/// Response for a passkey registration request.
 pub struct PasskeyRegistrationResponse {
     /// Relying Party ID.
     pub rp_id: String,

apps/desktop/desktop_native/autofill_provider/src/window_handle_query.rs

@@ -4,8 +4,8 @@ use serde::{Deserialize, Serialize};
 
 use crate::{BitwardenError, Callback, TimedCallback};
 
-#[derive(Debug, Default, Serialize, Deserialize)]
 /// Request to get the window handle of the desktop client.
+#[derive(Debug, Default, Serialize, Deserialize)]
 pub(super) struct WindowHandleQueryRequest {
     #[serde(rename = "windowHandle")]
     /// Marker field for parsing; data is never read.
@@ -15,9 +15,9 @@ pub(super) struct WindowHandleQueryRequest {
     window_handle: String,
 }
 
+/// Response to window handle request.
 #[derive(Debug, Deserialize)]
 #[serde(rename_all = "camelCase")]
-/// Response to window handle request.
 pub struct WindowHandleQueryResponse {
     /// Whether the desktop client is currently visible.
     pub is_visible: bool,