diff --git a/credentialsd-common/src/model.rs b/credentialsd-common/src/model.rs index e7d8b1b..98a6e88 100644 --- a/credentialsd-common/src/model.rs +++ b/credentialsd-common/src/model.rs @@ -105,55 +105,6 @@ pub enum PinNotSetError { PinNotSet, } -#[derive(Debug, Clone)] -pub enum Error { - /// Some unknown error with the authenticator occurred. - AuthenticatorError, - /// No matching credentials were found on the device. - NoCredentials, - /// Credential was already registered with this device (credential ID contained in excludeCredentials) - CredentialExcluded, - /// Too many incorrect PIN attempts, and authenticator must be removed and - /// reinserted to continue any more PIN attempts. - /// - /// Note that this is different than exhausting the PIN count that fully - /// locks out the device. - PinAttemptsExhausted, - // TODO: We may want to hide the details on this variant from the public API. - /// Something went wrong with the credential service itself, not the authenticator. - Internal(String), -} - -impl std::error::Error for Error {} - -impl Display for Error { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::AuthenticatorError => f.write_str("AuthenticatorError"), - Self::NoCredentials => f.write_str("NoCredentials"), - Self::CredentialExcluded => f.write_str("CredentialExcluded"), - Self::PinAttemptsExhausted => f.write_str("PinAttemptsExhausted"), - Self::Internal(s) => write!(f, "InternalError: {s}"), - } - } -} - -impl TryFrom<&Value<'_>> for Error { - type Error = zvariant::Error; - - fn try_from(value: &Value<'_>) -> Result { - let err_code: &str = value.downcast_ref()?; - let err = match err_code { - "AuthenticatorError" => crate::model::Error::AuthenticatorError, - "NoCredentials" => crate::model::Error::NoCredentials, - "CredentialExcluded" => crate::model::Error::CredentialExcluded, - "PinAttemptsExhausted" => crate::model::Error::PinAttemptsExhausted, - s => crate::model::Error::Internal(String::from(s)), - }; - Ok(err) - } -} - #[derive(Debug, PartialEq, SerializeDict, DeserializeDict, Type)] #[zvariant(signature = "dict")] pub struct NotifyNeedsPinOptions {} diff --git a/credentialsd/src/credential_service/hybrid.rs b/credentialsd/src/credential_service/hybrid.rs index 4d18cac..369c0b2 100644 --- a/credentialsd/src/credential_service/hybrid.rs +++ b/credentialsd/src/credential_service/hybrid.rs @@ -21,10 +21,8 @@ use tokio::sync::{ use tokio_util::sync::CancellationToken; use tracing::{debug, error}; -use credentialsd_common::{ - memfd::write_secret, - model::{BackgroundEvent, Error}, -}; +use super::CredentialServiceError; +use credentialsd_common::{memfd::write_secret, model::BackgroundEvent}; use crate::model::{CredentialRequest, CredentialResponse}; @@ -148,13 +146,15 @@ impl HybridHandler for InternalHybridHandler { } .map_err(|err| match err { WebAuthnError::Ctap(CtapError::PINAuthBlocked) => { - Error::PinAttemptsExhausted + CredentialServiceError::PinAttemptsExhausted + } + WebAuthnError::Ctap(CtapError::NoCredentials) => { + CredentialServiceError::NoCredentials } - WebAuthnError::Ctap(CtapError::NoCredentials) => Error::NoCredentials, WebAuthnError::Ctap(CtapError::CredentialExcluded) => { - Error::CredentialExcluded + CredentialServiceError::CredentialExcluded } - _ => Error::AuthenticatorError, + _ => CredentialServiceError::AuthenticatorError, }) }; @@ -166,15 +166,26 @@ impl HybridHandler for InternalHybridHandler { Some(resp) => resp, None => { tracing::debug!("Hybrid handler cancelled, stopping processing"); - Err(Error::Internal("Request cancelled".to_string())) + Err(CredentialServiceError::RequestCancelled) } }; let terminal_state = match response { - Ok(auth_response) => HybridStateInternal::Completed(auth_response), - Err(err) => HybridStateInternal::Failed(err), + Ok(auth_response) => Some(HybridStateInternal::Completed(auth_response)), + Err(CredentialServiceError::RequestCancelled) => { + // Cancelled by another transport winning or an explicit user cancel. + // Do not emit a Failed state — complete_request was already called + // by the winning path, and emitting Failed here would produce a + // spurious ErrorAuthenticator in the UI and a redundant + // complete_request invocation. + tracing::debug!("Hybrid handler cancelled, exiting silently"); + None + } + Err(err) => Some(HybridStateInternal::Failed(err)), }; - if let Err(err) = tx.send(terminal_state).await { + if let Some(state) = terminal_state + && let Err(err) = tx.send(state).await + { tracing::error!("Failed to send caBLE update: {:?}", err) } }); @@ -203,11 +214,7 @@ pub(super) enum HybridStateInternal { /// Authenticator data Completed(CredentialResponse), - Failed(Error), - // TODO(cancellation) - // This isn't actually sent from the server. - #[allow(dead_code)] - UserCancelled, + Failed(CredentialServiceError), } // this is here to prevent making HybridStateInternal public to the whole crate. @@ -233,10 +240,7 @@ pub enum HybridState { Completed, /// Hybrid operation failed. - Failed(Error), - - // This isn't actually sent from the server. - UserCancelled, + Failed(CredentialServiceError), } impl From for HybridState { @@ -246,7 +250,6 @@ impl From for HybridState { HybridStateInternal::Connecting => HybridState::Connecting, HybridStateInternal::Connected => HybridState::Connected, HybridStateInternal::Completed(_) => HybridState::Completed, - HybridStateInternal::UserCancelled => HybridState::UserCancelled, HybridStateInternal::Failed(err) => HybridState::Failed(err), } } @@ -269,14 +272,25 @@ impl From<&HybridState> for BackgroundEvent { HybridState::Connecting => BackgroundEvent::HybridConnecting, HybridState::Connected => BackgroundEvent::HybridConnected, HybridState::Completed => BackgroundEvent::CeremonyCompleted, - HybridState::UserCancelled => BackgroundEvent::ErrorCancelled, - HybridState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator, - HybridState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials, - HybridState::Failed(Error::CredentialExcluded) => { + HybridState::Failed(CredentialServiceError::AuthenticatorError) => { + BackgroundEvent::ErrorAuthenticator + } + HybridState::Failed(CredentialServiceError::NoCredentials) => { + BackgroundEvent::ErrorNoCredentials + } + HybridState::Failed(CredentialServiceError::CredentialExcluded) => { BackgroundEvent::ErrorCredentialExcluded } - HybridState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator, - HybridState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal, + HybridState::Failed(CredentialServiceError::PinAttemptsExhausted) => { + BackgroundEvent::ErrorAuthenticator + } + // This should currently never be reached, but we'll likely use it in future refactoring + HybridState::Failed(CredentialServiceError::RequestCancelled) => { + BackgroundEvent::ErrorCancelled + } + HybridState::Failed(CredentialServiceError::Internal(_)) => { + BackgroundEvent::ErrorInternal + } } } } @@ -302,7 +316,9 @@ async fn handle_hybrid_updates( CableUpdate::Connected => Some(HybridStateInternal::Connected), CableUpdate::Error(transport_error) => { error!(?transport_error, "Hybrid transport error"); - Some(HybridStateInternal::Failed(Error::AuthenticatorError)) + Some(HybridStateInternal::Failed( + CredentialServiceError::AuthenticatorError, + )) } }, }; diff --git a/credentialsd/src/credential_service/mod.rs b/credentialsd/src/credential_service/mod.rs index 686e80e..c3068a7 100644 --- a/credentialsd/src/credential_service/mod.rs +++ b/credentialsd/src/credential_service/mod.rs @@ -3,7 +3,7 @@ pub mod nfc; pub mod usb; use std::{ - fmt::Debug, + fmt::{Debug, Display}, pin::Pin, sync::{Arc, Mutex, OnceLock}, task::Poll, @@ -23,9 +23,8 @@ use nfc::{NfcEvent, NfcHandler, NfcState, NfcStateInternal}; use tokio::sync::oneshot; use tokio_util::sync::CancellationToken; -use credentialsd_common::model::{ - BackgroundEvent, Device, Error as CredentialServiceError, Transport, -}; +use credentialsd_common::model::{BackgroundEvent, Device, Transport}; +use zbus::zvariant::{self, Value}; use crate::{ credential_service::{hybrid::HybridEvent, usb::UsbEvent}, @@ -50,7 +49,7 @@ async fn cancellable_sleep( tokio::select! { _ = tokio::time::sleep(duration) => Ok(()), _ = cancellation.cancelled() => { - Err(CredentialServiceError::Internal("Request cancelled".to_string())) + Err(CredentialServiceError::RequestCancelled) } } } @@ -63,6 +62,62 @@ fn persistent_token_store() -> Arc { .clone() } +#[derive(Debug, Clone)] +pub enum CredentialServiceError { + /// Some unknown error with the authenticator occurred. + AuthenticatorError, + /// No matching credentials were found on the device. + NoCredentials, + /// Credential was already registered with this device (credential ID contained in excludeCredentials) + CredentialExcluded, + /// Too many incorrect PIN attempts, and authenticator must be removed and + /// reinserted to continue any more PIN attempts. + /// + /// Note that this is different than exhausting the PIN count that fully + /// locks out the device. + PinAttemptsExhausted, + /// The request was cancelled — either because another transport completed the + /// ceremony first, or because the user or client explicitly cancelled it. + /// This is an expected, non-error termination and should not be treated as an + /// authenticator failure. + RequestCancelled, + // TODO: We may want to hide the details on this variant from the public API. + /// Something went wrong with the credential service itself, not the authenticator. + Internal(String), +} + +impl std::error::Error for CredentialServiceError {} + +impl Display for CredentialServiceError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::AuthenticatorError => f.write_str("AuthenticatorError"), + Self::NoCredentials => f.write_str("NoCredentials"), + Self::CredentialExcluded => f.write_str("CredentialExcluded"), + Self::PinAttemptsExhausted => f.write_str("PinAttemptsExhausted"), + Self::RequestCancelled => f.write_str("RequestCancelled"), + Self::Internal(s) => write!(f, "InternalError: {s}"), + } + } +} + +impl TryFrom<&Value<'_>> for CredentialServiceError { + type Error = zvariant::Error; + + fn try_from(value: &Value<'_>) -> Result { + let err_code: &str = value.downcast_ref()?; + let err = match err_code { + "AuthenticatorError" => Self::AuthenticatorError, + "NoCredentials" => Self::NoCredentials, + "CredentialExcluded" => Self::CredentialExcluded, + "PinAttemptsExhausted" => Self::PinAttemptsExhausted, + "RequestCancelled" => Self::RequestCancelled, + s => Self::Internal(String::from(s)), + }; + Ok(err) + } +} + #[derive(Debug)] struct RequestContext { request: CredentialRequest, @@ -348,6 +403,10 @@ where HybridStateInternal::Completed(response) => { complete_request(ctx, Ok(response.clone())); } + // RequestCancelled (another transport won or user cancelled) + // should not call complete_request — it was already called + // by the winning transport or cancel_request(). + HybridStateInternal::Failed(CredentialServiceError::RequestCancelled) => {} HybridStateInternal::Failed(err) => { complete_request(ctx, Err(err.clone())); } @@ -388,6 +447,10 @@ where UsbStateInternal::Completed(response) => { complete_request(ctx, Ok(response.clone())); } + // RequestCancelled (another transport won or user cancelled) + // should not call complete_request — it was already called + // by the winning transport or cancel_request(). + UsbStateInternal::Failed(CredentialServiceError::RequestCancelled) => {} UsbStateInternal::Failed(error) => { complete_request(ctx, Err(error.clone())); } @@ -430,6 +493,10 @@ where NfcStateInternal::Completed(response) => { complete_request(ctx, Ok(response.clone())); } + // RequestCancelled (another transport won or user cancelled) + // should not call complete_request — it was already called + // by the winning transport or cancel_request(). + NfcStateInternal::Failed(CredentialServiceError::RequestCancelled) => {} NfcStateInternal::Failed(error) => { complete_request(ctx, Err(error.clone())); } @@ -672,7 +739,11 @@ mod tests { let start = tokio::time::Instant::now(); let result = cancellable_sleep(Duration::from_secs(5), &token).await; - assert!(result.is_err()); + // Must return RequestCancelled, not a generic Internal error + assert!( + matches!(result, Err(CredentialServiceError::RequestCancelled)), + "cancellable_sleep must return RequestCancelled when the token is cancelled" + ); // Should return immediately, not after 5 seconds assert!(start.elapsed() < Duration::from_millis(100)); } @@ -1067,8 +1138,6 @@ mod tests { #[tokio::test] async fn test_failed_request_triggers_cancellation() { - use credentialsd_common::model::Error; - let usb_handler = CancellationTrackingHandler::::new(); let usb_ref = usb_handler.get_handler_ref(); @@ -1084,7 +1153,7 @@ mod tests { usb_ref.shift_state(UsbStateInternal::Waiting); assert!(matches!(usb_stream.next().await, Some(UsbState::Waiting))); - usb_ref.shift_state(UsbStateInternal::Failed(Error::Internal( + usb_ref.shift_state(UsbStateInternal::Failed(CredentialServiceError::Internal( "test failure".to_string(), ))); assert!(matches!(usb_stream.next().await, Some(UsbState::Failed(_)))); @@ -1098,8 +1167,6 @@ mod tests { #[tokio::test] async fn test_failed_request_cancels_other_transports() { - use credentialsd_common::model::Error; - let usb_handler = CancellationTrackingHandler::::new(); let hybrid_handler = CancellationTrackingHandler::::new(); let usb_ref = usb_handler.get_handler_ref(); @@ -1127,7 +1194,7 @@ mod tests { // USB fails — UsbStateStream calls complete_request → token cancelled usb_ref.shift_state(UsbStateInternal::Waiting); - usb_ref.shift_state(UsbStateInternal::Failed(Error::Internal( + usb_ref.shift_state(UsbStateInternal::Failed(CredentialServiceError::Internal( "test".to_string(), ))); assert!(matches!(usb_stream.next().await, Some(UsbState::Waiting))); @@ -1229,4 +1296,79 @@ mod tests { "Hybrid handler should have detected cancellation when USB completed" ); } + + /// When USB is cancelled (by another transport winning), the stream must emit + /// no Failed/ErrorInternal state — it should simply end cleanly. + #[tokio::test] + async fn test_cancelled_usb_emits_no_failed_state() { + let usb_handler = CancellationTrackingHandler::::new(); + let usb_ref = usb_handler.get_handler_ref(); + + let service = CredentialService::new(MockHybridHandler, MockNfcHandler, usb_handler); + let request = create_test_request().await; + let (tx, _rx) = oneshot::channel(); + let (request_id, token) = service.init_request(&request, tx).await.unwrap(); + let mut usb_stream = service.get_usb_credential().await; + + // Confirm stream is live + usb_ref.shift_state(UsbStateInternal::Waiting); + assert!(matches!(usb_stream.next().await, Some(UsbState::Waiting))); + + // Queue a RequestCancelled — simulates what process() emits when the + // cancellation token fires internally before the outer branch catches it. + usb_ref.shift_state(UsbStateInternal::Failed( + CredentialServiceError::RequestCancelled, + )); + + // Cancel the request synchronously so the token is already cancelled + // when the stream is next polled. + service.cancel_request(request_id).await; + assert!(token.is_cancelled()); + + // The stream must not yield the Failed(RequestCancelled) state. + // biased select! polls cancellation first; the queued state is discarded. + let remaining: Vec<_> = usb_stream.collect().await; + assert!( + remaining.is_empty(), + "cancelled USB stream must emit no further states, including Failed(RequestCancelled)" + ); + } + + /// When hybrid is cancelled, it must not emit a Failed state on the stream. + /// complete_request must be invoked exactly once (by the winning transport or + /// cancel_request), not a second time from hybrid's terminal-state path. + #[tokio::test] + async fn test_cancelled_hybrid_emits_no_failed_state() { + let hybrid_handler = CancellationTrackingHandler::::new(); + let hybrid_ref = hybrid_handler.get_handler_ref(); + + let service = CredentialService::new(hybrid_handler, MockNfcHandler, MockUsbHandler); + let request = create_test_request().await; + let (tx, _rx) = oneshot::channel(); + let (request_id, token) = service.init_request(&request, tx).await.unwrap(); + let mut hybrid_stream = service.get_hybrid_credential().await; + + // Confirm stream is live + hybrid_ref.shift_state(HybridStateInternal::Init("qr".to_string())); + assert!(matches!( + hybrid_stream.next().await, + Some(HybridState::Init(_)) + )); + + // Queue a RequestCancelled — what the real handler would emit when + // run_until_cancelled returns None + hybrid_ref.shift_state(HybridStateInternal::Failed( + CredentialServiceError::RequestCancelled, + )); + + service.cancel_request(request_id).await; + assert!(token.is_cancelled()); + + // Stream must stop without emitting the Failed(RequestCancelled) state. + let remaining: Vec<_> = hybrid_stream.collect().await; + assert!( + remaining.is_empty(), + "cancelled hybrid stream must emit no further states" + ); + } } diff --git a/credentialsd/src/credential_service/nfc.rs b/credentialsd/src/credential_service/nfc.rs index bd30d78..7ac0edf 100644 --- a/credentialsd/src/credential_service/nfc.rs +++ b/credentialsd/src/credential_service/nfc.rs @@ -16,11 +16,11 @@ use tokio::sync::mpsc::{self, Receiver, Sender, WeakSender}; use tokio_util::sync::CancellationToken; use tracing::{debug, warn}; -use credentialsd_common::model::{BackgroundEvent, Credential, Error, PinNotSetError}; +use credentialsd_common::model::{BackgroundEvent, Credential, PinNotSetError}; use crate::model::{CredentialRequest, GetAssertionResponseInternal}; -use super::{AuthenticatorResponse, CredentialResponse}; +use super::{AuthenticatorResponse, CredentialResponse, CredentialServiceError}; pub(crate) trait NfcHandler { #[expect(unused)] @@ -39,14 +39,11 @@ impl InProcessNfcHandler { failures: &mut usize, prev_nfc_state: &NfcStateInternal, cancellation: &CancellationToken, - ) -> Result { + ) -> Result { let list_device_fut = libwebauthn::transport::nfc::get_nfc_device(); let Some(result) = cancellation.run_until_cancelled(list_device_fut).await else { - // TODO: We should introduce a cancelled-error variant and return this here, - // so we can differentiate between internal errors, cancellation by user - // and cancellation because other transfers finished tracing::debug!("NFC idle polling cancelled"); - return Err(Error::Internal("Request cancelled".to_string())); + return Err(CredentialServiceError::RequestCancelled); }; match result { Ok(Some(nfc_device)) => Ok(NfcStateInternal::Connected(nfc_device)), @@ -57,7 +54,7 @@ impl InProcessNfcHandler { Err(err) => { *failures += 1; if *failures == 5 { - Err(Error::Internal(format!( + Err(CredentialServiceError::Internal(format!( "Failed to list NFC authenticators: {:?}. Cancelling NFC state updates.", err ))) @@ -74,9 +71,9 @@ impl InProcessNfcHandler { } async fn process_select_credential( - response: GetAssertionResponse, + response: &GetAssertionResponse, cred_rx: &mut Receiver, - ) -> Result { + ) -> Result { match cred_rx.recv().await { Some(cred_id) => { let assertion = response @@ -105,12 +102,12 @@ impl InProcessNfcHandler { ), )), )), - None => Err(Error::NoCredentials), + None => Err(CredentialServiceError::NoCredentials), } } None => { tracing::debug!("cred channel closed before receiving cred from client."); - Err(Error::Internal( + Err(CredentialServiceError::Internal( "Cred channel disconnected prematurely".to_string(), )) } @@ -118,9 +115,9 @@ impl InProcessNfcHandler { } async fn process_user_interaction( - signal_rx: &mut Receiver>, + signal_rx: &mut Receiver>, cred_tx: &Sender, - ) -> Result { + ) -> Result { match signal_rx.recv().await { Some(msg) => match msg { Ok(NfcUvMessage::NeedsPin { @@ -162,7 +159,9 @@ impl InProcessNfcHandler { }, Err(err) => Err(err), }, - None => Err(Error::Internal("NFC UV handler channel closed".to_string())), + None => Err(CredentialServiceError::Internal( + "NFC UV handler channel closed".to_string(), + )), } } @@ -170,7 +169,7 @@ impl InProcessNfcHandler { tx: Sender, cred_request: CredentialRequest, cancellation: CancellationToken, - ) -> Result<(), Error> { + ) -> Result<(), CredentialServiceError> { let mut state = NfcStateInternal::Idle; let (signal_tx, mut signal_rx) = mpsc::channel(256); let (cred_tx, mut cred_rx) = mpsc::channel(1); @@ -188,7 +187,8 @@ impl InProcessNfcHandler { Self::process_idle_waiting(&mut failures, &prev_nfc_state, &cancellation) .await } - NfcStateInternal::Connected(device) => { + NfcStateInternal::Connected(ref device) => { + let device = device.clone(); let signal_tx2 = signal_tx.clone(); let cred_request = cred_request.clone(); let cancellation = cancellation.clone(); @@ -203,7 +203,7 @@ impl InProcessNfcHandler { Self::process_user_interaction(&mut signal_rx, &cred_tx).await } NfcStateInternal::SelectCredential { - response, + ref response, cred_tx: _, } => Self::process_select_credential(response, &mut cred_rx).await, // Terminal states - preserve state unchanged, will break loop after sending @@ -218,14 +218,39 @@ impl InProcessNfcHandler { .await else { tracing::debug!("NFC handler cancelled, stopping processing"); - break Err(Error::Internal("Request cancelled".to_string())); + break Ok(()); }; + // Guard: inner future may have raced the cancellation token and returned + // RequestCancelled. Break cleanly without emitting a spurious Failed state. + if matches!( + next_nfc_state, + Err(CredentialServiceError::RequestCancelled) + ) { + tracing::debug!("NFC handler cancelled (inner path), stopping processing"); + break Ok(()); + } + state = next_nfc_state.unwrap_or_else(NfcStateInternal::Failed); - tx.send(state.clone()).await.map_err(|_| { - Error::Internal("NFC state channel receiver closed prematurely".to_string()) - })?; + // Usually, comparing the discriminant is enough, but PinNotSet/NeedsPin + // can be repeated multiple times with different or the same error reasons + // (PIN wrong, PIN too short, PIN too long, etc.) + let state_changed = match (&state, &prev_nfc_state) { + (NfcStateInternal::PinNotSet { .. }, NfcStateInternal::PinNotSet { .. }) => true, + (NfcStateInternal::NeedsPin { .. }, NfcStateInternal::NeedsPin { .. }) => true, + (new_state, old_state) => { + std::mem::discriminant(new_state) != std::mem::discriminant(old_state) + } + }; + if state_changed { + tracing::debug!("NFC current state: {state:?}"); + tx.send(state.clone()).await.map_err(|_| { + CredentialServiceError::Internal( + "NFC state channel receiver closed prematurely".to_string(), + ) + })?; + } // Check for terminal states AFTER sending match state { @@ -240,7 +265,7 @@ impl InProcessNfcHandler { async fn handle_events( cred_request: &CredentialRequest, mut device: NfcDevice, - signal_tx: &Sender>, + signal_tx: &Sender>, cancellation: CancellationToken, ) { let device_debug = device.to_string(); @@ -310,10 +335,16 @@ async fn handle_events( } } .map_err(|err| match err { - WebAuthnError::Ctap(CtapError::PINAuthBlocked) => Error::PinAttemptsExhausted, - WebAuthnError::Ctap(CtapError::NoCredentials) => Error::NoCredentials, - WebAuthnError::Ctap(CtapError::CredentialExcluded) => Error::CredentialExcluded, - _ => Error::AuthenticatorError, + WebAuthnError::Ctap(CtapError::PINAuthBlocked) => { + CredentialServiceError::PinAttemptsExhausted + } + WebAuthnError::Ctap(CtapError::NoCredentials) => { + CredentialServiceError::NoCredentials + } + WebAuthnError::Ctap(CtapError::CredentialExcluded) => { + CredentialServiceError::CredentialExcluded + } + _ => CredentialServiceError::AuthenticatorError, }) }; @@ -327,8 +358,8 @@ async fn handle_events( // Unlike USB, NfcChannelHandle::cancel_ongoing_operation() is a no-op // because libwebauthn drops _handle_rx in NfcChannel::new(). Cancellation // takes effect at the next inter-APDU .await point when the future is - // dropped. - Err(Error::Internal("Request cancelled".to_string())) + // dropped; NFC exchanges are short so the latency is acceptable. + Err(CredentialServiceError::RequestCancelled) } }; @@ -408,10 +439,7 @@ pub(super) enum NfcStateInternal { Completed(CredentialResponse), /// There was an error while interacting with the authenticator. - Failed(Error), - // TODO: implement cancellation - // This isn't actually sent from the server. - //UserCancelled, + Failed(CredentialServiceError), } /// Used to share public state between credential service and UI. @@ -441,9 +469,6 @@ pub enum NfcState { /// The device needs on-device user verification. NeedsUserVerification { attempts_left: Option }, - // TODO: implement cancellation - // This isn't actually sent from the server. - //UserCancelled, // Multiple credentials have been found and the user has to select which to use // List of user-identities to decide which to use. @@ -456,7 +481,7 @@ pub enum NfcState { Completed, /// Interaction with the authenticator failed. - Failed(Error), + Failed(CredentialServiceError), } impl From for NfcState { @@ -479,7 +504,6 @@ impl From for NfcState { NfcState::NeedsUserVerification { attempts_left } } NfcStateInternal::Completed(_) => NfcState::Completed, - // NfcStateInternal::UserCancelled => NfcState:://UserCancelled, NfcStateInternal::SelectCredential { response, cred_tx } => { NfcState::SelectingCredential { creds: response @@ -545,17 +569,28 @@ impl From<&NfcState> for BackgroundEvent { creds: creds.to_vec(), }, NfcState::Completed => BackgroundEvent::CeremonyCompleted, - NfcState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator, - NfcState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials, - NfcState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorCredentialExcluded, - NfcState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator, - NfcState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal, + NfcState::Failed(CredentialServiceError::AuthenticatorError) => { + BackgroundEvent::ErrorAuthenticator + } + NfcState::Failed(CredentialServiceError::NoCredentials) => { + BackgroundEvent::ErrorNoCredentials + } + NfcState::Failed(CredentialServiceError::CredentialExcluded) => { + BackgroundEvent::ErrorCredentialExcluded + } + NfcState::Failed(CredentialServiceError::PinAttemptsExhausted) => { + BackgroundEvent::ErrorAuthenticator + } + NfcState::Failed(CredentialServiceError::RequestCancelled) => { + BackgroundEvent::ErrorCancelled + } + NfcState::Failed(CredentialServiceError::Internal(_)) => BackgroundEvent::ErrorInternal, } } } async fn handle_nfc_updates( - signal_tx: &WeakSender>, + signal_tx: &WeakSender>, mut state_rx: broadcast::Receiver, ) { while let Ok(msg) = state_rx.recv().await { diff --git a/credentialsd/src/credential_service/usb.rs b/credentialsd/src/credential_service/usb.rs index 3cd4362..a68837e 100644 --- a/credentialsd/src/credential_service/usb.rs +++ b/credentialsd/src/credential_service/usb.rs @@ -21,11 +21,11 @@ use tokio::sync::{ use tokio_util::sync::CancellationToken; use tracing::{debug, warn}; -use credentialsd_common::model::{BackgroundEvent, Credential, Error, PinNotSetError}; +use credentialsd_common::model::{BackgroundEvent, Credential, PinNotSetError}; use crate::model::{CredentialRequest, GetAssertionResponseInternal}; -use super::{AuthenticatorResponse, CredentialResponse}; +use super::{AuthenticatorResponse, CredentialResponse, CredentialServiceError}; pub(crate) trait UsbHandler { fn start( @@ -43,14 +43,11 @@ impl InProcessUsbHandler { failures: &mut usize, prev_usb_state: &UsbStateInternal, cancellation: &CancellationToken, - ) -> Result { + ) -> Result { let list_device_fut = libwebauthn::transport::hid::list_devices(); let Some(result) = cancellation.run_until_cancelled(list_device_fut).await else { tracing::debug!("USB idle polling cancelled"); - // TODO: We should introduce a cancelled-error variant and return this here, - // so we can differentiate between internal errors, cancellation by user - // and cancellation because other transfers finished - return Err(Error::Internal("Request cancelled".to_string())); + return Err(CredentialServiceError::RequestCancelled); }; match result { @@ -65,7 +62,7 @@ impl InProcessUsbHandler { Err(err) => { *failures += 1; if *failures == 5 { - Err(Error::Internal(format!( + Err(CredentialServiceError::Internal(format!( "Failed to list USB authenticators: {:?}. Cancelling USB state updates.", err ))) @@ -84,7 +81,7 @@ impl InProcessUsbHandler { async fn process_selecting_device( hid_devices: &[HidDevice], cancellation: &CancellationToken, - ) -> Result { + ) -> Result { let expected_answers = hid_devices.len(); let (blinking_tx, mut blinking_rx) = tokio::sync::mpsc::channel::>(expected_answers); @@ -150,7 +147,7 @@ impl InProcessUsbHandler { tracing::info!("Cancelling blinking device {device:?}."); handle.cancel_ongoing_operation().await; } - return Err(Error::Internal("Request cancelled".to_string())); + return Err(CredentialServiceError::RequestCancelled); }; let Some(msg) = maybe_msg else { @@ -179,7 +176,7 @@ impl InProcessUsbHandler { async fn process_select_credential( response: &GetAssertionResponse, cred_rx: &mut Receiver, - ) -> Result { + ) -> Result { match cred_rx.recv().await { Some(cred_id) => { let assertion = response @@ -208,12 +205,12 @@ impl InProcessUsbHandler { ), )), )), - None => Err(Error::NoCredentials), + None => Err(CredentialServiceError::NoCredentials), } } None => { tracing::debug!("cred channel closed before receiving cred from client."); - Err(Error::Internal( + Err(CredentialServiceError::Internal( "Cred channel disconnected prematurely".to_string(), )) } @@ -221,9 +218,9 @@ impl InProcessUsbHandler { } async fn process_user_interaction( - signal_rx: &mut Receiver>, + signal_rx: &mut Receiver>, cred_tx: &Sender, - ) -> Result { + ) -> Result { match signal_rx.recv().await { Some(msg) => match msg { Ok(UsbUvMessage::NeedsPin { @@ -266,7 +263,9 @@ impl InProcessUsbHandler { }, Err(err) => Err(err), }, - None => Err(Error::Internal("USB UV handler channel closed".to_string())), + None => Err(CredentialServiceError::Internal( + "USB UV handler channel closed".to_string(), + )), } } @@ -274,7 +273,7 @@ impl InProcessUsbHandler { tx: Sender, cred_request: CredentialRequest, cancellation: CancellationToken, - ) -> Result<(), Error> { + ) -> Result<(), CredentialServiceError> { let mut state = UsbStateInternal::Idle; let (signal_tx, mut signal_rx) = mpsc::channel(256); let (cred_tx, mut cred_rx) = mpsc::channel(1); @@ -327,9 +326,21 @@ impl InProcessUsbHandler { .await else { tracing::debug!("USB handler cancelled, stopping processing"); - break Err(Error::Internal("Request cancelled".to_string())); + break Ok(()); }; + // Guard: an inner future may have raced the cancellation token and + // returned RequestCancelled as a value rather than the outer branch + // firing. Treat it the same way — break cleanly without emitting a + // spurious Failed state to the UI. + if matches!( + next_usb_state, + Err(CredentialServiceError::RequestCancelled) + ) { + tracing::debug!("USB handler cancelled (inner path), stopping processing"); + break Ok(()); + } + state = next_usb_state.unwrap_or_else(UsbStateInternal::Failed); // Usually, comparing the discriminant is enough, but PinNotSet/NeedsPin // can be repeated multiple times with different or the same error reasons @@ -344,7 +355,9 @@ impl InProcessUsbHandler { if state_changed { tracing::debug!("USB current state: {state:?}"); tx.send(state.clone()).await.map_err(|_| { - Error::Internal("USB state channel receiver closed prematurely".to_string()) + CredentialServiceError::Internal( + "USB state channel receiver closed prematurely".to_string(), + ) })?; } @@ -361,7 +374,7 @@ impl InProcessUsbHandler { async fn handle_events( cred_request: &CredentialRequest, device: Arc>, - signal_tx: &Sender>, + signal_tx: &Sender>, cancellation: CancellationToken, ) { let mut device = device.lock().await; @@ -433,10 +446,16 @@ async fn handle_events( } } .map_err(|err| match err { - WebAuthnError::Ctap(CtapError::PINAuthBlocked) => Error::PinAttemptsExhausted, - WebAuthnError::Ctap(CtapError::NoCredentials) => Error::NoCredentials, - WebAuthnError::Ctap(CtapError::CredentialExcluded) => Error::CredentialExcluded, - _ => Error::AuthenticatorError, + WebAuthnError::Ctap(CtapError::PINAuthBlocked) => { + CredentialServiceError::PinAttemptsExhausted + } + WebAuthnError::Ctap(CtapError::NoCredentials) => { + CredentialServiceError::NoCredentials + } + WebAuthnError::Ctap(CtapError::CredentialExcluded) => { + CredentialServiceError::CredentialExcluded + } + _ => CredentialServiceError::AuthenticatorError, }) }; @@ -448,7 +467,7 @@ async fn handle_events( None => { tracing::debug!("USB ceremony cancelled, interrupting authenticator operation"); cancel_handle.cancel_ongoing_operation().await; - Err(Error::Internal("Request cancelled".to_string())) + Err(CredentialServiceError::RequestCancelled) } }; @@ -531,10 +550,7 @@ pub(super) enum UsbStateInternal { Completed(CredentialResponse), /// There was an error while interacting with the authenticator. - Failed(Error), - // TODO: implement cancellation - // This isn't actually sent from the server. - //UserCancelled, + Failed(CredentialServiceError), } /// Used to share public state between credential service and UI. @@ -573,9 +589,6 @@ pub enum UsbState { /// The device needs evidence of user presence (e.g. touch) to release the credential. NeedsUserPresence, - // TODO: implement cancellation - // This isn't actually sent from the server. - //UserCancelled, // Multiple credentials have been found and the user has to select which to use // List of user-identities to decide which to use. @@ -588,7 +601,7 @@ pub enum UsbState { Completed, /// Interaction with the authenticator failed. - Failed(Error), + Failed(CredentialServiceError), } impl From for UsbState { @@ -612,7 +625,6 @@ impl From for UsbState { } UsbStateInternal::NeedsUserPresence => UsbState::NeedsUserPresence, UsbStateInternal::Completed(_) => UsbState::Completed, - // UsbStateInternal::UserCancelled => UsbState:://UserCancelled, UsbStateInternal::SelectingDevice(_) => UsbState::SelectingDevice, UsbStateInternal::SelectCredential { response, cred_tx } => { UsbState::SelectingCredential { @@ -681,17 +693,28 @@ impl From<&UsbState> for BackgroundEvent { creds: creds.to_vec(), }, UsbState::Completed => BackgroundEvent::CeremonyCompleted, - UsbState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator, - UsbState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials, - UsbState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorCredentialExcluded, - UsbState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator, - UsbState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal, + UsbState::Failed(CredentialServiceError::AuthenticatorError) => { + BackgroundEvent::ErrorAuthenticator + } + UsbState::Failed(CredentialServiceError::NoCredentials) => { + BackgroundEvent::ErrorNoCredentials + } + UsbState::Failed(CredentialServiceError::CredentialExcluded) => { + BackgroundEvent::ErrorCredentialExcluded + } + UsbState::Failed(CredentialServiceError::PinAttemptsExhausted) => { + BackgroundEvent::ErrorAuthenticator + } + UsbState::Failed(CredentialServiceError::RequestCancelled) => { + BackgroundEvent::ErrorCancelled + } + UsbState::Failed(CredentialServiceError::Internal(_)) => BackgroundEvent::ErrorInternal, } } } async fn handle_usb_updates( - signal_tx: &WeakSender>, + signal_tx: &WeakSender>, mut state_rx: broadcast::Receiver, ) { while let Ok(msg) = state_rx.recv().await { diff --git a/credentialsd/src/dbus/flow_control.rs b/credentialsd/src/dbus/flow_control.rs index 61362a2..63cb299 100644 --- a/credentialsd/src/dbus/flow_control.rs +++ b/credentialsd/src/dbus/flow_control.rs @@ -10,10 +10,7 @@ use std::{ use async_trait::async_trait; use credentialsd_common::{ memfd::read_secret, - model::{ - BackgroundEvent, Error as CredentialServiceError, PortalBackendOptions, - UserInteractedEvent, WindowHandle, - }, + model::{BackgroundEvent, PortalBackendOptions, UserInteractedEvent, WindowHandle}, }; use futures_lite::{Stream, StreamExt}; use tokio::sync::mpsc::Receiver; @@ -30,7 +27,7 @@ use crate::{ model::{CredentialRequest, CredentialResponse}, }; use crate::{ - credential_service::{DeviceStateUpdate, ManageDevice, nfc::NfcState}, + credential_service::{CredentialServiceError, DeviceStateUpdate, ManageDevice, nfc::NfcState}, model::ClientDetails, }; use crate::{dbus::ui_control::Ceremony, gateway::WebAuthnError};