Cancellation part 2: Don't emit failed states on cancellation

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<Self, Self::Error> {
-        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 {}

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<HybridStateInternal> for HybridState {
@@ -246,7 +250,6 @@ impl From<HybridStateInternal> 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,
+                    ))
                 }
             },
         };