From addf1e5562f69ae1608287c13411f5059d121c64 Mon Sep 17 00:00:00 2001 From: Martin Sirringhaus Date: Tue, 8 Sep 2026 13:29:06 +0200 Subject: [PATCH 1/2] Introduce function to determine if error code should terminate the ceremony (plus renaming of the cancellation variant) --- credentialsd/src/credential_service/hybrid.rs | 6 +- credentialsd/src/credential_service/mod.rs | 241 ++++++++++++++---- credentialsd/src/credential_service/nfc.rs | 10 +- credentialsd/src/credential_service/usb.rs | 16 +- 4 files changed, 203 insertions(+), 70 deletions(-) diff --git a/credentialsd/src/credential_service/hybrid.rs b/credentialsd/src/credential_service/hybrid.rs index 369c0b2..56e9f94 100644 --- a/credentialsd/src/credential_service/hybrid.rs +++ b/credentialsd/src/credential_service/hybrid.rs @@ -166,13 +166,13 @@ impl HybridHandler for InternalHybridHandler { Some(resp) => resp, None => { tracing::debug!("Hybrid handler cancelled, stopping processing"); - Err(CredentialServiceError::RequestCancelled) + Err(CredentialServiceError::NonTerminatingCancellation) } }; let terminal_state = match response { Ok(auth_response) => Some(HybridStateInternal::Completed(auth_response)), - Err(CredentialServiceError::RequestCancelled) => { + Err(CredentialServiceError::NonTerminatingCancellation) => { // 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 @@ -285,7 +285,7 @@ impl From<&HybridState> for BackgroundEvent { BackgroundEvent::ErrorAuthenticator } // This should currently never be reached, but we'll likely use it in future refactoring - HybridState::Failed(CredentialServiceError::RequestCancelled) => { + HybridState::Failed(CredentialServiceError::NonTerminatingCancellation) => { BackgroundEvent::ErrorCancelled } HybridState::Failed(CredentialServiceError::Internal(_)) => { diff --git a/credentialsd/src/credential_service/mod.rs b/credentialsd/src/credential_service/mod.rs index c3068a7..01a04fe 100644 --- a/credentialsd/src/credential_service/mod.rs +++ b/credentialsd/src/credential_service/mod.rs @@ -49,7 +49,7 @@ async fn cancellable_sleep( tokio::select! { _ = tokio::time::sleep(duration) => Ok(()), _ = cancellation.cancelled() => { - Err(CredentialServiceError::RequestCancelled) + Err(CredentialServiceError::NonTerminatingCancellation) } } } @@ -76,11 +76,14 @@ pub enum CredentialServiceError { /// 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, + /// Internal cancellation — the ceremony was cancelled because another transport + /// completed first (superseded), or because a code-path cancellation propagated. + /// This is distinct from user- or client-issued cancellation: the response channel + /// has already been consumed elsewhere, so `complete_request` must NOT be called. + /// + /// A future `TerminatingCancellation` variant will be added for user-initiated + /// cancellation from the trusted UI, which *is* ceremony-terminating. + NonTerminatingCancellation, // 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), @@ -95,7 +98,7 @@ impl Display for CredentialServiceError { 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::NonTerminatingCancellation => f.write_str("NonTerminatingCancellation"), Self::Internal(s) => write!(f, "InternalError: {s}"), } } @@ -111,7 +114,7 @@ impl TryFrom<&Value<'_>> for CredentialServiceError { "NoCredentials" => Self::NoCredentials, "CredentialExcluded" => Self::CredentialExcluded, "PinAttemptsExhausted" => Self::PinAttemptsExhausted, - "RequestCancelled" => Self::RequestCancelled, + "NonTerminatingCancellation" => Self::NonTerminatingCancellation, s => Self::Internal(String::from(s)), }; Ok(err) @@ -403,13 +406,15 @@ 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) => { + HybridStateInternal::Failed(err) if is_ceremony_terminating(err) => { complete_request(ctx, Err(err.clone())); } + HybridStateInternal::Failed(_) => { + // Non-terminating: forward the Failed state to the UI + // without calling complete_request. The ceremony stays alive + // for other transports. The transport is expected to restart + // itself. + } _ => {} } Poll::Ready(Some(state.into())) @@ -447,12 +452,14 @@ 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())); + UsbStateInternal::Failed(err) if is_ceremony_terminating(err) => { + complete_request(ctx, Err(err.clone())); + } + UsbStateInternal::Failed(_) => { + // Non-terminating: forward the Failed state to the UI + // without calling complete_request. The ceremony stays alive + // for other transports. The transport is expected to restart + // itself. } _ => {} } @@ -493,12 +500,14 @@ 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())); + NfcStateInternal::Failed(err) if is_ceremony_terminating(err) => { + complete_request(ctx, Err(err.clone())); + } + NfcStateInternal::Failed(_) => { + // Non-terminating: forward the Failed state to the UI + // without calling complete_request. The ceremony stays alive + // for other transports. The transport is expected to restart + // itself. } _ => {} } @@ -543,6 +552,42 @@ impl From for DeviceStateUpdate { } } +/// Returns `true` if this arm of `poll_next` must call `complete_request()`, +/// terminating the entire ceremony. Returns `false` if the error is +/// per-authenticator or already-handled: the ceremony continues on other +/// transports, and this transport is expected to restart itself. +/// +/// The `match` has no wildcard arm so any new `Error` variant forces a +/// deliberate decision here. The mapping follows the WebAuthn specification: +/// - https://www.w3.org/TR/webauthn-3/#sctn-create-request-exceptions +/// - https://www.w3.org/TR/webauthn-3/#sctn-get-request-exceptions +fn is_ceremony_terminating(err: &CredentialServiceError) -> bool { + match err { + // WebAuthn spec requires CredentialExcluded be remapped to InvalidStateError + // and returned to the RP. The credential is already registered on this + // authenticator. + CredentialServiceError::CredentialExcluded => true, + + // NonTerminatingCancellation is emitted by transports whose ceremony was + // cancelled by *another* path — the winning transports `complete_request()`, + // or `cancel_request()` sending its own response. The response channel is + // already consumed, so this arm must NOT invoke `complete_request()` again. + // A future `TerminatingCancellation` variant will handle user-initiated + // cancellation from the trusted UI, which is ceremony-terminating. + CredentialServiceError::NonTerminatingCancellation => false, + + // Per-authenticator errors: keep the ceremony alive. The user may succeed + // on another transport, or the same transport may recover and retry + // (the latter is not yet implemented). + CredentialServiceError::AuthenticatorError => false, + CredentialServiceError::NoCredentials => false, + CredentialServiceError::PinAttemptsExhausted => false, + + // Transient internal errors: do not kill the ceremony. + CredentialServiceError::Internal(_) => false, + } +} + fn complete_request( ctx: &Mutex>, response: Result, @@ -739,10 +784,13 @@ mod tests { let start = tokio::time::Instant::now(); let result = cancellable_sleep(Duration::from_secs(5), &token).await; - // Must return RequestCancelled, not a generic Internal error + // Must return NonTerminatingCancellation, not a generic Internal error assert!( - matches!(result, Err(CredentialServiceError::RequestCancelled)), - "cancellable_sleep must return RequestCancelled when the token is cancelled" + matches!( + result, + Err(CredentialServiceError::NonTerminatingCancellation) + ), + "cancellable_sleep must return NonTerminatingCancellation when the token is cancelled" ); // Should return immediately, not after 5 seconds assert!(start.elapsed() < Duration::from_millis(100)); @@ -1137,32 +1185,80 @@ mod tests { } #[tokio::test] - async fn test_failed_request_triggers_cancellation() { + async fn test_terminating_failure_ends_ceremony() { 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 (tx, rx) = oneshot::channel(); - let (_request_id, cancellation_token) = service.init_request(&request, tx).await.unwrap(); + let (_id, token) = service.init_request(&request, tx).await.unwrap(); let mut usb_stream = service.get_usb_credential().await; - assert!(!cancellation_token.is_cancelled()); + assert!(!token.is_cancelled()); - usb_ref.shift_state(UsbStateInternal::Waiting); - assert!(matches!(usb_stream.next().await, Some(UsbState::Waiting))); + // CredentialExcluded is ceremony-terminating per the WebAuthn spec + usb_ref.shift_state(UsbStateInternal::Failed( + CredentialServiceError::CredentialExcluded, + )); + assert!(matches!( + usb_stream.next().await, + Some(UsbState::Failed(CredentialServiceError::CredentialExcluded)) + )); - usb_ref.shift_state(UsbStateInternal::Failed(CredentialServiceError::Internal( - "test failure".to_string(), - ))); - assert!(matches!(usb_stream.next().await, Some(UsbState::Failed(_)))); + // complete_request was called: token cancelled, response delivered to caller + assert!( + token.is_cancelled(), + "ceremony must end on terminating error" + ); + assert!( + matches!( + rx.await, + Ok(Err(CredentialServiceError::CredentialExcluded)) + ), + "caller must receive the terminating error" + ); + } - // UsbStateStream calls complete_request on Failed, which cancels the token + #[tokio::test] + async fn test_non_terminating_failure_keeps_ceremony_alive() { + let usb_handler = CancellationTrackingHandler::::new(); + let hybrid_handler = CancellationTrackingHandler::::new(); + let usb_ref = usb_handler.get_handler_ref(); + let hybrid_ref = hybrid_handler.get_handler_ref(); + + let service = CredentialService::new(hybrid_handler, MockNfcHandler, usb_handler); + let request = create_test_request().await; + let (tx, _rx) = oneshot::channel(); + let (id, token) = service.init_request(&request, tx).await.unwrap(); + + let mut usb_stream = service.get_usb_credential().await; + let mut hybrid_stream = service.get_hybrid_credential().await; + + // USB fails with a non-terminating error — forwarded to UI but ceremony lives + usb_ref.shift_state(UsbStateInternal::Failed( + CredentialServiceError::AuthenticatorError, + )); + assert!(matches!( + usb_stream.next().await, + Some(UsbState::Failed(CredentialServiceError::AuthenticatorError)) + )); + + // Token must still be live — is_ceremony_terminating(AuthenticatorError) == false assert!( - cancellation_token.is_cancelled(), - "Cancellation token should be triggered when request fails" + !token.is_cancelled(), + "ceremony must stay alive on non-terminating error" ); + + // Other transports must still be operational + hybrid_ref.shift_state(HybridStateInternal::Connecting); + assert!(matches!( + hybrid_stream.next().await, + Some(HybridState::Connecting) + )); + + service.cancel_request(id).await; } #[tokio::test] @@ -1192,13 +1288,16 @@ mod tests { // It sits in the channel when complete_request() cancels the token. hybrid_ref.shift_state(HybridStateInternal::Connecting); - // USB fails — UsbStateStream calls complete_request → token cancelled + // USB fails with a ceremony-terminating error → complete_request → token cancelled usb_ref.shift_state(UsbStateInternal::Waiting); - usb_ref.shift_state(UsbStateInternal::Failed(CredentialServiceError::Internal( - "test".to_string(), - ))); + usb_ref.shift_state(UsbStateInternal::Failed( + CredentialServiceError::CredentialExcluded, + )); assert!(matches!(usb_stream.next().await, Some(UsbState::Waiting))); - assert!(matches!(usb_stream.next().await, Some(UsbState::Failed(_)))); + assert!(matches!( + usb_stream.next().await, + Some(UsbState::Failed(CredentialServiceError::CredentialExcluded)) + )); assert!( cancellation_token.is_cancelled(), @@ -1314,10 +1413,10 @@ mod tests { 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. + // Queue a NonTerminatingCancellation — simulates what process() emits when + // the cancellation token fires internally before the outer branch catches it. usb_ref.shift_state(UsbStateInternal::Failed( - CredentialServiceError::RequestCancelled, + CredentialServiceError::NonTerminatingCancellation, )); // Cancel the request synchronously so the token is already cancelled @@ -1325,12 +1424,12 @@ mod tests { service.cancel_request(request_id).await; assert!(token.is_cancelled()); - // The stream must not yield the Failed(RequestCancelled) state. + // The stream must not yield the Failed(NonTerminatingCancellation) 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)" + "cancelled USB stream must emit no further states, including Failed(NonTerminatingCancellation)" ); } @@ -1355,20 +1454,54 @@ mod tests { Some(HybridState::Init(_)) )); - // Queue a RequestCancelled — what the real handler would emit when + // Queue a NonTerminatingCancellation — what the real handler would emit when // run_until_cancelled returns None hybrid_ref.shift_state(HybridStateInternal::Failed( - CredentialServiceError::RequestCancelled, + CredentialServiceError::NonTerminatingCancellation, )); service.cancel_request(request_id).await; assert!(token.is_cancelled()); - // Stream must stop without emitting the Failed(RequestCancelled) state. + // Stream must stop without emitting the Failed(NonTerminatingCancellation) state. let remaining: Vec<_> = hybrid_stream.collect().await; assert!( remaining.is_empty(), "cancelled hybrid stream must emit no further states" ); } + + // The following tests are stupid, but try to prevent regressions regarding changes around + // `is_ceremony_terminating()` + #[test] + fn test_classifier_ceremony_terminating_errors() { + // These return true — this arm must call complete_request and end the ceremony. + assert!(is_ceremony_terminating( + &CredentialServiceError::CredentialExcluded + )); + } + + #[test] + fn test_classifier_per_authenticator_errors() { + // Per-authenticator: ceremony stays alive; the same or another transport can retry. + let errors = [ + CredentialServiceError::AuthenticatorError, + CredentialServiceError::NoCredentials, + CredentialServiceError::PinAttemptsExhausted, + CredentialServiceError::Internal("x".into()), + ]; + for err in errors { + assert!(!is_ceremony_terminating(&err)); + } + } + + #[test] + fn test_classifier_non_terminating_cancellation() { + // NonTerminatingCancellation: the response channel has already been consumed + // by the winning transport's complete_request or by cancel_request directly. + // This arm must NOT invoke complete_request again — hence false. + assert!(!is_ceremony_terminating( + &CredentialServiceError::NonTerminatingCancellation + )); + } } diff --git a/credentialsd/src/credential_service/nfc.rs b/credentialsd/src/credential_service/nfc.rs index 7ac0edf..2dbba68 100644 --- a/credentialsd/src/credential_service/nfc.rs +++ b/credentialsd/src/credential_service/nfc.rs @@ -43,7 +43,7 @@ impl InProcessNfcHandler { let list_device_fut = libwebauthn::transport::nfc::get_nfc_device(); let Some(result) = cancellation.run_until_cancelled(list_device_fut).await else { tracing::debug!("NFC idle polling cancelled"); - return Err(CredentialServiceError::RequestCancelled); + return Err(CredentialServiceError::NonTerminatingCancellation); }; match result { Ok(Some(nfc_device)) => Ok(NfcStateInternal::Connected(nfc_device)), @@ -222,10 +222,10 @@ impl InProcessNfcHandler { }; // Guard: inner future may have raced the cancellation token and returned - // RequestCancelled. Break cleanly without emitting a spurious Failed state. + // NonTerminatingCancellation. Break cleanly without emitting a spurious Failed state. if matches!( next_nfc_state, - Err(CredentialServiceError::RequestCancelled) + Err(CredentialServiceError::NonTerminatingCancellation) ) { tracing::debug!("NFC handler cancelled (inner path), stopping processing"); break Ok(()); @@ -359,7 +359,7 @@ async fn handle_events( // because libwebauthn drops _handle_rx in NfcChannel::new(). Cancellation // takes effect at the next inter-APDU .await point when the future is // dropped; NFC exchanges are short so the latency is acceptable. - Err(CredentialServiceError::RequestCancelled) + Err(CredentialServiceError::NonTerminatingCancellation) } }; @@ -581,7 +581,7 @@ impl From<&NfcState> for BackgroundEvent { NfcState::Failed(CredentialServiceError::PinAttemptsExhausted) => { BackgroundEvent::ErrorAuthenticator } - NfcState::Failed(CredentialServiceError::RequestCancelled) => { + NfcState::Failed(CredentialServiceError::NonTerminatingCancellation) => { BackgroundEvent::ErrorCancelled } NfcState::Failed(CredentialServiceError::Internal(_)) => BackgroundEvent::ErrorInternal, diff --git a/credentialsd/src/credential_service/usb.rs b/credentialsd/src/credential_service/usb.rs index a68837e..13ab453 100644 --- a/credentialsd/src/credential_service/usb.rs +++ b/credentialsd/src/credential_service/usb.rs @@ -47,7 +47,7 @@ impl InProcessUsbHandler { 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"); - return Err(CredentialServiceError::RequestCancelled); + return Err(CredentialServiceError::NonTerminatingCancellation); }; match result { @@ -147,7 +147,7 @@ impl InProcessUsbHandler { tracing::info!("Cancelling blinking device {device:?}."); handle.cancel_ongoing_operation().await; } - return Err(CredentialServiceError::RequestCancelled); + return Err(CredentialServiceError::NonTerminatingCancellation); }; let Some(msg) = maybe_msg else { @@ -330,12 +330,12 @@ impl InProcessUsbHandler { }; // 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. + // returned NonTerminatingCancellation 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) + Err(CredentialServiceError::NonTerminatingCancellation) ) { tracing::debug!("USB handler cancelled (inner path), stopping processing"); break Ok(()); @@ -467,7 +467,7 @@ async fn handle_events( None => { tracing::debug!("USB ceremony cancelled, interrupting authenticator operation"); cancel_handle.cancel_ongoing_operation().await; - Err(CredentialServiceError::RequestCancelled) + Err(CredentialServiceError::NonTerminatingCancellation) } }; @@ -705,7 +705,7 @@ impl From<&UsbState> for BackgroundEvent { UsbState::Failed(CredentialServiceError::PinAttemptsExhausted) => { BackgroundEvent::ErrorAuthenticator } - UsbState::Failed(CredentialServiceError::RequestCancelled) => { + UsbState::Failed(CredentialServiceError::NonTerminatingCancellation) => { BackgroundEvent::ErrorCancelled } UsbState::Failed(CredentialServiceError::Internal(_)) => BackgroundEvent::ErrorInternal, From 427f449bde3709555db33a86edf6be26ad536d58 Mon Sep 17 00:00:00 2001 From: Martin Sirringhaus Date: Tue, 8 Sep 2026 14:55:12 +0200 Subject: [PATCH 2/2] Add new TransportRestarted signal that lets the UI recover from a non-terminal error --- CHANGELOG.md | 2 + credentialsd-common/src/model.rs | 40 ++- credentialsd-ui/src/dbus.rs | 49 +++- credentialsd-ui/src/gui/mod.rs | 4 + credentialsd-ui/src/gui/view_model/gtk/mod.rs | 12 + .../src/gui/view_model/gtk/window.rs | 14 + credentialsd-ui/src/gui/view_model/mod.rs | 11 + credentialsd/src/credential_service/hybrid.rs | 269 ++++++++++-------- credentialsd/src/credential_service/mod.rs | 129 ++++++++- credentialsd/src/credential_service/nfc.rs | 40 ++- credentialsd/src/credential_service/usb.rs | 40 ++- credentialsd/src/dbus/ui_control.rs | 52 +++- 12 files changed, 527 insertions(+), 135 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1da53b..6b95367 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # [unreleased] +- ui: Basic recovery from failed attempts to use devices (e.g. flaky bluetooth) + # 0.3.1 [2026-09-05] ## Improvements diff --git a/credentialsd-common/src/model.rs b/credentialsd-common/src/model.rs index 98a6e88..1c74184 100644 --- a/credentialsd-common/src/model.rs +++ b/credentialsd-common/src/model.rs @@ -17,25 +17,45 @@ pub const BACKGROUND_EVENT_ERROR_PIN_NOT_SET: u32 = 0x80000008; #[derive(Debug, PartialEq)] pub enum BackgroundEvent { CeremonyCompleted, - NeedsPin { attempts_left: Option }, - PinNotSet { error: PinNotSetError }, - NeedsUserVerification { attempts_left: Option }, + NeedsPin { + attempts_left: Option, + }, + PinNotSet { + error: PinNotSetError, + }, + NeedsUserVerification { + attempts_left: Option, + }, NeedsUserPresence, - SelectingCredential { creds: Vec }, + SelectingCredential { + creds: Vec, + }, HybridIdle, HybridStarted(OwnedFd), HybridConnecting, HybridConnected, + /// The hybrid ceremony was interrupted by a non-terminating error and a new + /// QR code is about to be issued. The UI should navigate back to the start + /// page so the new QR becomes visible. + HybridRestarting, NfcIdle, NfcWaiting, NfcConnected, + /// The NFC ceremony was interrupted by a non-terminating error and the + /// transport is polling for a new device tap. The UI should navigate back + /// to the start page. + NfcRestarting, UsbIdle, UsbWaiting, UsbSelectingDevice, UsbConnected, + /// The USB ceremony was interrupted by a non-terminating error and the + /// transport is polling for a device. The UI should navigate back to the + /// start page. + UsbRestarting, ErrorInternal, ErrorTimedOut, @@ -146,6 +166,18 @@ pub struct NotifyNfcConnectedOptions {} #[zvariant(signature = "dict")] pub struct NotifyUsbConnectedOptions {} +#[derive(Debug, SerializeDict, DeserializeDict, Type)] +#[zvariant(signature = "dict")] +pub struct NotifyHybridRestartingOptions {} + +#[derive(Debug, SerializeDict, DeserializeDict, Type)] +#[zvariant(signature = "dict")] +pub struct NotifyUsbRestartingOptions {} + +#[derive(Debug, SerializeDict, DeserializeDict, Type)] +#[zvariant(signature = "dict")] +pub struct NotifyNfcRestartingOptions {} + #[derive(Clone, Debug, Serialize, Deserialize, Type)] pub enum Operation { PublicKeyCreate, diff --git a/credentialsd-ui/src/dbus.rs b/credentialsd-ui/src/dbus.rs index 8456556..7f10823 100644 --- a/credentialsd-ui/src/dbus.rs +++ b/credentialsd-ui/src/dbus.rs @@ -29,9 +29,10 @@ use credentialsd_common::model::{ BACKGROUND_EVENT_ERROR_PIN_NOT_SET, BACKGROUND_EVENT_ERROR_TIMED_OUT, BackgroundEvent, ClientPinEnteredOptions, Credential, CredentialSelectedOptions, Device, DiscoveryRequestedOptions, NotifyHybridConnectedOptions, NotifyHybridConnectingOptions, - NotifyHybridStartedOptions, NotifyNeedsPinOptions, NotifyNeedsUserPresenceOptions, - NotifyNeedsUserVerificationOptions, NotifyNfcConnectedOptions, NotifyPinNotSetOptions, - NotifySelectingCredentialOptions, NotifyUsbConnectedOptions, Operation, PinNotSetError, + NotifyHybridRestartingOptions, NotifyHybridStartedOptions, NotifyNeedsPinOptions, + NotifyNeedsUserPresenceOptions, NotifyNeedsUserVerificationOptions, NotifyNfcConnectedOptions, + NotifyNfcRestartingOptions, NotifyPinNotSetOptions, NotifySelectingCredentialOptions, + NotifyUsbConnectedOptions, NotifyUsbRestartingOptions, Operation, PinNotSetError, PortalBackendOptions, SetDevicePinOptions, UserInteractedEvent, WindowHandle, }; @@ -307,6 +308,48 @@ impl CredentialPortalBackend { .await } + async fn notify_hybrid_restarting( + &self, + #[zbus(object_server)] object_server: &ObjectServer, + session_handle: ObjectPath<'_>, + _options: NotifyHybridRestartingOptions, + ) -> fdo::Result<()> { + self.notify_state_changed( + object_server, + session_handle, + BackgroundEvent::HybridRestarting, + ) + .await + } + + async fn notify_usb_restarting( + &self, + #[zbus(object_server)] object_server: &ObjectServer, + session_handle: ObjectPath<'_>, + _options: NotifyUsbRestartingOptions, + ) -> fdo::Result<()> { + self.notify_state_changed( + object_server, + session_handle, + BackgroundEvent::UsbRestarting, + ) + .await + } + + async fn notify_nfc_restarting( + &self, + #[zbus(object_server)] object_server: &ObjectServer, + session_handle: ObjectPath<'_>, + _options: NotifyNfcRestartingOptions, + ) -> fdo::Result<()> { + self.notify_state_changed( + object_server, + session_handle, + BackgroundEvent::NfcRestarting, + ) + .await + } + /// Called when the authentication ceremony completes successfully. async fn notify_ceremony_completed( &self, diff --git a/credentialsd-ui/src/gui/mod.rs b/credentialsd-ui/src/gui/mod.rs index e2acb3f..e55ada9 100644 --- a/credentialsd-ui/src/gui/mod.rs +++ b/credentialsd-ui/src/gui/mod.rs @@ -95,6 +95,10 @@ pub enum ViewUpdate { HybridConnecting, HybridConnected, + /// A transport ceremony was interrupted by a non-terminating error and + /// is restarting. The UI should navigate back to the start page. + TransportRestarting, + Completed, Cancelled, Failed(String), diff --git a/credentialsd-ui/src/gui/view_model/gtk/mod.rs b/credentialsd-ui/src/gui/view_model/gtk/mod.rs index 109bc89..76afc9e 100644 --- a/credentialsd-ui/src/gui/view_model/gtk/mod.rs +++ b/credentialsd-ui/src/gui/view_model/gtk/mod.rs @@ -84,6 +84,9 @@ mod imp { #[property(get, set)] pub qr_spinner_visible: RefCell, + #[property(get, set)] + pub transport_restarting: RefCell, + #[property(get, set)] pub start_setting_new_pin_visible: RefCell, @@ -138,6 +141,7 @@ impl ViewModel { // TODO: hack so I don't have to unset this in every event manually. view_model.set_usb_nfc_pin_entry_visible(false); view_model.set_start_setting_new_pin_visible(false); + view_model.set_transport_restarting(false); view_model.set_failed(false); match update { ViewUpdate::SetTitle { @@ -239,6 +243,14 @@ impl ViewModel { )); view_model.set_qr_spinner_visible(false); } + ViewUpdate::TransportRestarting => { + // Signal the window to navigate back to start_page. + // The transport will emit a fresh Init/Connected state + // next, which will update the prompt and show the new + // QR code or device-waiting UI from start_page. + view_model.set_qr_spinner_visible(false); + view_model.set_transport_restarting(true); + } ViewUpdate::Completed => { view_model.set_qr_spinner_visible(false); view_model.set_completed(true); diff --git a/credentialsd-ui/src/gui/view_model/gtk/window.rs b/credentialsd-ui/src/gui/view_model/gtk/window.rs index 58d1cac..dceb718 100644 --- a/credentialsd-ui/src/gui/view_model/gtk/window.rs +++ b/credentialsd-ui/src/gui/view_model/gtk/window.rs @@ -226,6 +226,20 @@ impl CredentialsUiWindow { } )); + // When any transport restarts after a non-terminating error, navigate back to + // start_page. For hybrid this ensures the new QR code (which lives on start_page) + // is visible; for USB/NFC it clears stale prompts and lets the user re-plug or + // choose a different transport. + view_model.connect_transport_restarting_notify(clone!( + #[weak] + stack, + move |vm| { + if vm.transport_restarting() { + stack.set_visible_child_name("start_page"); + } + } + )); + view_model.connect_completed_notify(clone!( #[weak] stack, diff --git a/credentialsd-ui/src/gui/view_model/mod.rs b/credentialsd-ui/src/gui/view_model/mod.rs index 0a791e1..6cf331b 100644 --- a/credentialsd-ui/src/gui/view_model/mod.rs +++ b/credentialsd-ui/src/gui/view_model/mod.rs @@ -348,6 +348,17 @@ impl ViewModel { .await .unwrap(); } + Event::Background( + BackgroundEvent::HybridRestarting + | BackgroundEvent::UsbRestarting + | BackgroundEvent::NfcRestarting, + ) => { + self.hybrid_qr_code_data = None; + self.tx_update + .send(ViewUpdate::TransportRestarting) + .await + .unwrap(); + } Event::Background(BackgroundEvent::ErrorCancelled) => { self.hybrid_qr_code_data = None; break; diff --git a/credentialsd/src/credential_service/hybrid.rs b/credentialsd/src/credential_service/hybrid.rs index 56e9f94..c5094e9 100644 --- a/credentialsd/src/credential_service/hybrid.rs +++ b/credentialsd/src/credential_service/hybrid.rs @@ -68,127 +68,65 @@ impl HybridHandler for InternalHybridHandler { } else { CableTransports::CloudAssistedOnly }; - let mut device = match CableQrCodeDevice::new_transient(hint, hybrid_transports) { - Ok(device) => device, - Err(err) => { - tracing::error!("Failed to create caBLE QR code device: {:?}", err); - return; - } - }; - let qr_code = device.qr_code.to_string(); - if let Err(err) = tx.send(HybridStateInternal::Init(qr_code)).await { - tracing::error!("Failed to send caBLE update: {:?}", err); - return; - }; - tokio::spawn(async move { - let mut channel = match device.channel(ChannelSettings::default()).await { - Ok(channel) => channel, - Err(e) => { - tracing::error!("Failed to open hybrid channel: {:?}", e); - panic!(); + + // Outer retry loop: re-issues QR on non-terminating failures. + // Each iteration creates a fresh CableQrCodeDevice (the previous one + // is consumed by channel()), so the old QR secret is discarded. + loop { + let mut device = match CableQrCodeDevice::new_transient(hint, hybrid_transports) { + Ok(device) => device, + Err(err) => { + tracing::error!("Failed to create caBLE QR code device: {:?}", err); + // Device creation failure cannot be retried meaningfully — + // break to avoid a tight error loop. + let _ = tx + .send(HybridStateInternal::Failed( + CredentialServiceError::Internal(format!( + "Failed to create caBLE device: {err:?}" + )), + )) + .await; + break; } }; - let state_sender_clone = tx.clone(); - let ux_updates_rx = channel.get_ux_update_receiver(); - tokio::spawn(async move { - handle_hybrid_updates(&state_sender_clone, ux_updates_rx).await; - debug!("Reached end of Hybrid updates stream."); - }); + let qr_code = device.qr_code.to_string(); + if let Err(err) = tx.send(HybridStateInternal::Init(qr_code)).await { + tracing::error!("Failed to send caBLE update: {:?}", err); + break; + } - let wait_for_response_fut = async { - loop { - let response: Result = match &request { - CredentialRequest::CreatePublicKeyCredentialRequest(make_request) => { - channel.webauthn_make_credential(make_request).await.map( - |make_credential_response| { - CredentialResponse::from_make_credential( - &make_credential_response, - &["hybrid"], - "cross-platform", - ) - }, - ) - } - CredentialRequest::GetPublicKeyCredentialRequest(get_request) => { - channel.webauthn_get_assertion(get_request).await.map( - |get_assertion_response| { - CredentialResponse::from_get_assertion( - // When doing hybrid, the authenticator is capable of displaying it's own UI. - // So we assume here, it only ever returns one assertion. - // In case this doesn't hold true, we have to implement credential selection here, - // like USB, for example. - &get_assertion_response.assertions[0], - "cross-platform", - ) - }, - ) - } - }; - match response { - Ok(response) => { - tracing::debug!("Received credential from hybrid authenticator"); - break Ok(response); - } - Err(WebAuthnError::Ctap(ctap_error)) - if ctap_error.is_retryable_user_error() => - { - tracing::debug!(%ctap_error, "Retrying WebAuthn operation"); - continue; - } - Err(err) => { - tracing::error!(%err, - "Failed to make/get credential with hybrid authenticator" - ); - break Err(err); - } - } + // Run the ceremony awaited directly (not in a nested spawn) so that + // the retry loop is sequential and no orphaned tasks can arise. + let response = + run_hybrid_ceremony(&mut device, &request, &tx, cancellation.clone()).await; + + match response { + Ok(auth_response) => { + let _ = tx.send(HybridStateInternal::Completed(auth_response)).await; + break; } - .map_err(|err| match err { - WebAuthnError::Ctap(CtapError::PINAuthBlocked) => { - CredentialServiceError::PinAttemptsExhausted - } - WebAuthnError::Ctap(CtapError::NoCredentials) => { - CredentialServiceError::NoCredentials + Err(err) if super::is_ceremony_terminating(&err) => { + // Terminating errors (CredentialExcluded, NonTerminatingCancellation + // from a winning transport, etc.) stop the loop. + // NonTerminatingCancellation exits silently; others surface as Failed. + if !matches!(err, CredentialServiceError::NonTerminatingCancellation) { + let _ = tx.send(HybridStateInternal::Failed(err)).await; + } else { + tracing::debug!("Hybrid handler cancelled, exiting silently"); } - WebAuthnError::Ctap(CtapError::CredentialExcluded) => { - CredentialServiceError::CredentialExcluded - } - _ => CredentialServiceError::AuthenticatorError, - }) - }; - - tracing::debug!("Polling hybrid channel for updates."); - let response = match cancellation - .run_until_cancelled(wait_for_response_fut) - .await - { - Some(resp) => resp, - None => { - tracing::debug!("Hybrid handler cancelled, stopping processing"); - Err(CredentialServiceError::NonTerminatingCancellation) + break; } - }; - - let terminal_state = match response { - Ok(auth_response) => Some(HybridStateInternal::Completed(auth_response)), - Err(CredentialServiceError::NonTerminatingCancellation) => { - // 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) => { + // Non-terminating: notify the UI that a restart is in progress + // so it can navigate back to the start page, then reissue a + // fresh QR on the next iteration. + tracing::warn!(?err, "Hybrid error, reissuing QR"); + let _ = tx.send(HybridStateInternal::Restarting).await; + continue; } - Err(err) => Some(HybridStateInternal::Failed(err)), - }; - if let Some(state) = terminal_state - && let Err(err) = tx.send(state).await - { - tracing::error!("Failed to send caBLE update: {:?}", err) } - }); + } }); Box::pin(stream! { while let Some(state) = rx.recv().await { @@ -215,6 +153,10 @@ pub(super) enum HybridStateInternal { Completed(CredentialResponse), Failed(CredentialServiceError), + + /// The ceremony was interrupted by a non-terminating error. A fresh QR code + /// is about to be issued on the next iteration. + Restarting, } // this is here to prevent making HybridStateInternal public to the whole crate. @@ -241,6 +183,10 @@ pub enum HybridState { /// Hybrid operation failed. Failed(CredentialServiceError), + + /// The ceremony was interrupted by a non-terminating error and a new QR + /// code is being issued. The UI should navigate back to the start page. + Restarting, } impl From for HybridState { @@ -251,6 +197,7 @@ impl From for HybridState { HybridStateInternal::Connected => HybridState::Connected, HybridStateInternal::Completed(_) => HybridState::Completed, HybridStateInternal::Failed(err) => HybridState::Failed(err), + HybridStateInternal::Restarting => HybridState::Restarting, } } } @@ -272,6 +219,7 @@ impl From<&HybridState> for BackgroundEvent { HybridState::Connecting => BackgroundEvent::HybridConnecting, HybridState::Connected => BackgroundEvent::HybridConnected, HybridState::Completed => BackgroundEvent::CeremonyCompleted, + HybridState::Restarting => BackgroundEvent::HybridRestarting, HybridState::Failed(CredentialServiceError::AuthenticatorError) => { BackgroundEvent::ErrorAuthenticator } @@ -295,6 +243,103 @@ impl From<&HybridState> for BackgroundEvent { } } +/// Runs a single hybrid ceremony attempt: opens the caBLE channel, spawns the UX +/// update forwarder, and drives the `webauthn_make_credential` / `webauthn_get_assertion` +/// retry loop until a terminal result or cancellation. +/// +/// Returns `Ok(CredentialResponse)` on success, or `Err(CredentialServiceError)` on +/// failure. `NonTerminatingCancellation` is returned when the cancellation token fires. +async fn run_hybrid_ceremony( + device: &mut CableQrCodeDevice, + request: &CredentialRequest, + tx: &Sender, + cancellation: CancellationToken, +) -> Result { + let mut channel = match device.channel(ChannelSettings::default()).await { + Ok(channel) => channel, + Err(e) => { + tracing::error!("Failed to open hybrid channel: {:?}", e); + return Err(CredentialServiceError::AuthenticatorError); + } + }; + + let state_sender_clone = tx.clone(); + let ux_updates_rx = channel.get_ux_update_receiver(); + tokio::spawn(async move { + handle_hybrid_updates(&state_sender_clone, ux_updates_rx).await; + debug!("Reached end of Hybrid updates stream."); + }); + + let wait_for_response_fut = async { + loop { + let response: Result = match request { + CredentialRequest::CreatePublicKeyCredentialRequest(make_request) => { + channel.webauthn_make_credential(make_request).await.map( + |make_credential_response| { + CredentialResponse::from_make_credential( + &make_credential_response, + &["hybrid"], + "cross-platform", + ) + }, + ) + } + CredentialRequest::GetPublicKeyCredentialRequest(get_request) => { + channel.webauthn_get_assertion(get_request).await.map( + |get_assertion_response| { + CredentialResponse::from_get_assertion( + // When doing hybrid, the authenticator is capable of + // displaying its own UI, so we assume it only ever + // returns one assertion. If this doesn't hold true, + // credential selection must be implemented here, as + // done for USB. + &get_assertion_response.assertions[0], + "cross-platform", + ) + }, + ) + } + }; + match response { + Ok(response) => { + tracing::debug!("Received credential from hybrid authenticator"); + break Ok(response); + } + Err(WebAuthnError::Ctap(ctap_error)) if ctap_error.is_retryable_user_error() => { + tracing::debug!(%ctap_error, "Retrying WebAuthn operation"); + continue; + } + Err(err) => { + tracing::error!(%err, "Failed to make/get credential with hybrid authenticator"); + break Err(err); + } + } + } + .map_err(|err| match err { + WebAuthnError::Ctap(CtapError::PINAuthBlocked) => { + CredentialServiceError::PinAttemptsExhausted + } + WebAuthnError::Ctap(CtapError::NoCredentials) => CredentialServiceError::NoCredentials, + WebAuthnError::Ctap(CtapError::CredentialExcluded) => { + CredentialServiceError::CredentialExcluded + } + _ => CredentialServiceError::AuthenticatorError, + }) + }; + + tracing::debug!("Polling hybrid channel for updates."); + match cancellation + .run_until_cancelled(wait_for_response_fut) + .await + { + Some(resp) => resp, + None => { + tracing::debug!("Hybrid handler cancelled, stopping processing"); + Err(CredentialServiceError::NonTerminatingCancellation) + } + } +} + async fn handle_hybrid_updates( state_sender: &Sender, mut ux_update_receiver: broadcast::Receiver, diff --git a/credentialsd/src/credential_service/mod.rs b/credentialsd/src/credential_service/mod.rs index 01a04fe..459361c 100644 --- a/credentialsd/src/credential_service/mod.rs +++ b/credentialsd/src/credential_service/mod.rs @@ -561,7 +561,7 @@ impl From for DeviceStateUpdate { /// deliberate decision here. The mapping follows the WebAuthn specification: /// - https://www.w3.org/TR/webauthn-3/#sctn-create-request-exceptions /// - https://www.w3.org/TR/webauthn-3/#sctn-get-request-exceptions -fn is_ceremony_terminating(err: &CredentialServiceError) -> bool { +pub(super) fn is_ceremony_terminating(err: &CredentialServiceError) -> bool { match err { // WebAuthn spec requires CredentialExcluded be remapped to InvalidStateError // and returned to the RP. The credential is already registered on this @@ -1504,4 +1504,131 @@ mod tests { &CredentialServiceError::NonTerminatingCancellation )); } + + /// After a non-terminating USB failure, the mock stream remains live and + /// continues to emit subsequent states. + #[tokio::test] + async fn test_usb_non_terminating_error_transport_continues() { + 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 (id, token) = service.init_request(&request, tx).await.unwrap(); + let mut usb_stream = service.get_usb_credential().await; + + // Non-terminating error: forwarded to UI, ceremony stays alive + usb_ref.shift_state(UsbStateInternal::Failed( + CredentialServiceError::AuthenticatorError, + )); + assert!(matches!( + usb_stream.next().await, + Some(UsbState::Failed(CredentialServiceError::AuthenticatorError)) + )); + assert!( + !token.is_cancelled(), + "ceremony must stay alive on non-terminating error" + ); + + // Transport is still live — subsequent states arrive + usb_ref.shift_state(UsbStateInternal::Waiting); + assert!(matches!(usb_stream.next().await, Some(UsbState::Waiting))); + + service.cancel_request(id).await; + } + + /// After a non-terminating hybrid failure, a new QR Init is emitted — simulating + /// the retry loop in run_hybrid_ceremony / start() re-issuing a fresh QR code. + #[tokio::test] + async fn test_hybrid_qr_reissued_after_non_terminating_error() { + 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 (id, token) = service.init_request(&request, tx).await.unwrap(); + let mut hybrid_stream = service.get_hybrid_credential().await; + + // First QR issued + hybrid_ref.shift_state(HybridStateInternal::Init("qr-code-1".to_string())); + assert!(matches!( + hybrid_stream.next().await, + Some(HybridState::Init(_)) + )); + + // Tunnel fails with a non-terminating error — forwarded to UI + hybrid_ref.shift_state(HybridStateInternal::Failed( + CredentialServiceError::AuthenticatorError, + )); + assert!(matches!( + hybrid_stream.next().await, + Some(HybridState::Failed( + CredentialServiceError::AuthenticatorError + )) + )); + assert!(!token.is_cancelled(), "ceremony must stay alive"); + + // Real retry loop re-issues a new QR; simulated here via shift_state + hybrid_ref.shift_state(HybridStateInternal::Init("qr-code-2".to_string())); + assert!(matches!( + hybrid_stream.next().await, + Some(HybridState::Init(_)) + )); + + service.cancel_request(id).await; + } + + /// A Restarting state from the hybrid handler is forwarded to the UI stream + /// and does not call complete_request or cancel the ceremony token. + #[tokio::test] + async fn test_hybrid_restarting_forwarded_ceremony_alive() { + 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 (id, token) = service.init_request(&request, tx).await.unwrap(); + let mut hybrid_stream = service.get_hybrid_credential().await; + + hybrid_ref.shift_state(HybridStateInternal::Restarting); + assert!( + matches!(hybrid_stream.next().await, Some(HybridState::Restarting)), + "Restarting state must be forwarded to the UI stream" + ); + assert!( + !token.is_cancelled(), + "ceremony must stay alive on Restarting" + ); + + service.cancel_request(id).await; + } + + /// A Restarting state from the USB handler is forwarded to the UI stream + /// and does not call complete_request or cancel the ceremony token. + #[tokio::test] + async fn test_usb_restarting_forwarded_ceremony_alive() { + 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 (id, token) = service.init_request(&request, tx).await.unwrap(); + let mut usb_stream = service.get_usb_credential().await; + + usb_ref.shift_state(UsbStateInternal::Restarting); + assert!( + matches!(usb_stream.next().await, Some(UsbState::Restarting)), + "Restarting state must be forwarded to the UI stream" + ); + assert!( + !token.is_cancelled(), + "ceremony must stay alive on Restarting" + ); + + service.cancel_request(id).await; + } } diff --git a/credentialsd/src/credential_service/nfc.rs b/credentialsd/src/credential_service/nfc.rs index 2dbba68..f3ae821 100644 --- a/credentialsd/src/credential_service/nfc.rs +++ b/credentialsd/src/credential_service/nfc.rs @@ -206,10 +206,13 @@ impl InProcessNfcHandler { ref response, cred_tx: _, } => Self::process_select_credential(response, &mut cred_rx).await, - // Terminal states - preserve state unchanged, will break loop after sending - NfcStateInternal::Completed(_) | NfcStateInternal::Failed(_) => { - Ok(prev_nfc_state.clone()) - } + // Terminal states - preserve state unchanged, will break loop after sending. + // Restarting is a transient signal state only; it is immediately replaced + // by Idle in the non-terminating branch so it should never be the prev + // state, but we cover it here for exhaustiveness. + NfcStateInternal::Completed(_) + | NfcStateInternal::Failed(_) + | NfcStateInternal::Restarting => Ok(prev_nfc_state.clone()), } }; @@ -243,7 +246,13 @@ impl InProcessNfcHandler { std::mem::discriminant(new_state) != std::mem::discriminant(old_state) } }; - if state_changed { + // Suppress forwarding a non-terminating Failed state to the UI directly: + // the Restarting state emitted below takes its place with cleaner semantics. + let is_non_terminating_failure = matches!( + &state, + NfcStateInternal::Failed(err) if !super::is_ceremony_terminating(err) + ); + if state_changed && !is_non_terminating_failure { tracing::debug!("NFC current state: {state:?}"); tx.send(state.clone()).await.map_err(|_| { CredentialServiceError::Internal( @@ -255,7 +264,16 @@ impl InProcessNfcHandler { // Check for terminal states AFTER sending match state { NfcStateInternal::Completed(_) => break Ok(()), - NfcStateInternal::Failed(err) => break Err(err), + NfcStateInternal::Failed(ref err) => { + if super::is_ceremony_terminating(err) { + break Err(err.clone()); + } + // Non-terminating: notify the UI that a restart is in progress so + // it can navigate back to the start page, then restart polling. + tracing::warn!(?err, "NFC authenticator error, restarting transport"); + let _ = tx.send(NfcStateInternal::Restarting).await; + state = NfcStateInternal::Idle; + } _ => {} } } @@ -440,6 +458,10 @@ pub(super) enum NfcStateInternal { /// There was an error while interacting with the authenticator. Failed(CredentialServiceError), + + /// The ceremony was interrupted by a non-terminating error and the transport + /// is restarting. The UI should navigate back to the start page. + Restarting, } /// Used to share public state between credential service and UI. @@ -482,6 +504,10 @@ pub enum NfcState { /// Interaction with the authenticator failed. Failed(CredentialServiceError), + + /// The ceremony was interrupted by a non-terminating error and the transport + /// is restarting. The UI should navigate back to the start page. + Restarting, } impl From for NfcState { @@ -504,6 +530,7 @@ impl From for NfcState { NfcState::NeedsUserVerification { attempts_left } } NfcStateInternal::Completed(_) => NfcState::Completed, + NfcStateInternal::Restarting => NfcState::Restarting, NfcStateInternal::SelectCredential { response, cred_tx } => { NfcState::SelectingCredential { creds: response @@ -585,6 +612,7 @@ impl From<&NfcState> for BackgroundEvent { BackgroundEvent::ErrorCancelled } NfcState::Failed(CredentialServiceError::Internal(_)) => BackgroundEvent::ErrorInternal, + NfcState::Restarting => BackgroundEvent::NfcRestarting, } } } diff --git a/credentialsd/src/credential_service/usb.rs b/credentialsd/src/credential_service/usb.rs index 13ab453..1207833 100644 --- a/credentialsd/src/credential_service/usb.rs +++ b/credentialsd/src/credential_service/usb.rs @@ -314,10 +314,13 @@ impl InProcessUsbHandler { ref response, cred_tx: _, } => Self::process_select_credential(response, &mut cred_rx).await, - // Terminal states - preserve state unchanged, will break loop after sending - UsbStateInternal::Completed(_) | UsbStateInternal::Failed(_) => { - Ok(prev_usb_state.clone()) - } + // Terminal states - preserve state unchanged, will break loop after sending. + // Restarting is a transient signal state only; it is immediately replaced + // by Idle in the non-terminating branch so it should never be the prev + // state, but we cover it here for exhaustiveness. + UsbStateInternal::Completed(_) + | UsbStateInternal::Failed(_) + | UsbStateInternal::Restarting => Ok(prev_usb_state.clone()), } }; @@ -352,7 +355,13 @@ impl InProcessUsbHandler { std::mem::discriminant(new_state) != std::mem::discriminant(old_state) } }; - if state_changed { + // Suppress forwarding a non-terminating Failed state to the UI directly: + // the Restarting state emitted below takes its place with cleaner semantics. + let is_non_terminating_failure = matches!( + &state, + UsbStateInternal::Failed(err) if !super::is_ceremony_terminating(err) + ); + if state_changed && !is_non_terminating_failure { tracing::debug!("USB current state: {state:?}"); tx.send(state.clone()).await.map_err(|_| { CredentialServiceError::Internal( @@ -364,7 +373,16 @@ impl InProcessUsbHandler { // Check for terminal states AFTER sending match state { UsbStateInternal::Completed(_) => break Ok(()), - UsbStateInternal::Failed(err) => break Err(err), + UsbStateInternal::Failed(ref err) => { + if super::is_ceremony_terminating(err) { + break Err(err.clone()); + } + // Non-terminating: notify the UI that a restart is in progress so + // it can navigate back to the start page, then restart polling. + tracing::warn!(?err, "USB authenticator error, restarting transport"); + let _ = tx.send(UsbStateInternal::Restarting).await; + state = UsbStateInternal::Idle; + } _ => {} } } @@ -551,6 +569,10 @@ pub(super) enum UsbStateInternal { /// There was an error while interacting with the authenticator. Failed(CredentialServiceError), + + /// The ceremony was interrupted by a non-terminating error and the transport + /// is restarting. The UI should navigate back to the start page. + Restarting, } /// Used to share public state between credential service and UI. @@ -602,6 +624,10 @@ pub enum UsbState { /// Interaction with the authenticator failed. Failed(CredentialServiceError), + + /// The ceremony was interrupted by a non-terminating error and the transport + /// is restarting. The UI should navigate back to the start page. + Restarting, } impl From for UsbState { @@ -659,6 +685,7 @@ impl From for UsbState { } } UsbStateInternal::Failed(err) => UsbState::Failed(err), + UsbStateInternal::Restarting => UsbState::Restarting, } } } @@ -709,6 +736,7 @@ impl From<&UsbState> for BackgroundEvent { BackgroundEvent::ErrorCancelled } UsbState::Failed(CredentialServiceError::Internal(_)) => BackgroundEvent::ErrorInternal, + UsbState::Restarting => BackgroundEvent::UsbRestarting, } } } diff --git a/credentialsd/src/dbus/ui_control.rs b/credentialsd/src/dbus/ui_control.rs index 788ad60..65a9090 100644 --- a/credentialsd/src/dbus/ui_control.rs +++ b/credentialsd/src/dbus/ui_control.rs @@ -22,9 +22,10 @@ use credentialsd_common::model::{ BACKGROUND_EVENT_ERROR_PIN_NOT_SET, BACKGROUND_EVENT_ERROR_TIMED_OUT, BackgroundEvent, ClientPinEnteredOptions, Credential, CredentialSelectedOptions, Device, DiscoveryRequestedOptions, NotifyHybridConnectedOptions, NotifyHybridConnectingOptions, - NotifyHybridStartedOptions, NotifyNeedsPinOptions, NotifyNeedsUserPresenceOptions, - NotifyNeedsUserVerificationOptions, NotifyNfcConnectedOptions, NotifyPinNotSetOptions, - NotifySelectingCredentialOptions, NotifyUsbConnectedOptions, Operation, PinNotSetError, + NotifyHybridRestartingOptions, NotifyHybridStartedOptions, NotifyNeedsPinOptions, + NotifyNeedsUserPresenceOptions, NotifyNeedsUserVerificationOptions, NotifyNfcConnectedOptions, + NotifyNfcRestartingOptions, NotifyPinNotSetOptions, NotifySelectingCredentialOptions, + NotifyUsbConnectedOptions, NotifyUsbRestartingOptions, Operation, PinNotSetError, PortalBackendOptions, SetDevicePinOptions, UserInteractedEvent, WindowHandle, }; @@ -148,6 +149,27 @@ trait UiControlService { _options: NotifyUsbConnectedOptions, ) -> fdo::Result<()>; + #[zbus(no_reply)] + async fn notify_hybrid_restarting( + &self, + session_handle: ObjectPath<'_>, + _options: NotifyHybridRestartingOptions, + ) -> fdo::Result<()>; + + #[zbus(no_reply)] + async fn notify_usb_restarting( + &self, + session_handle: ObjectPath<'_>, + _options: NotifyUsbRestartingOptions, + ) -> fdo::Result<()>; + + #[zbus(no_reply)] + async fn notify_nfc_restarting( + &self, + session_handle: ObjectPath<'_>, + _options: NotifyNfcRestartingOptions, + ) -> fdo::Result<()>; + #[zbus(no_reply)] async fn notify_ceremony_completed(&self, session_handle: ObjectPath<'_>) -> fdo::Result<()>; @@ -299,6 +321,30 @@ impl Ceremony { ) .await } + BackgroundEvent::HybridRestarting => { + self.proxy + .notify_hybrid_restarting( + self.session_handle.as_ref(), + NotifyHybridRestartingOptions {}, + ) + .await + } + BackgroundEvent::UsbRestarting => { + self.proxy + .notify_usb_restarting( + self.session_handle.as_ref(), + NotifyUsbRestartingOptions {}, + ) + .await + } + BackgroundEvent::NfcRestarting => { + self.proxy + .notify_nfc_restarting( + self.session_handle.as_ref(), + NotifyNfcRestartingOptions {}, + ) + .await + } BackgroundEvent::ErrorInternal => { let error = BACKGROUND_EVENT_ERROR_INTERNAL; self.proxy