Propagate exclude credential

CHANGELOG.md

@@ -33,6 +33,7 @@ run xdg-desktop-portal while we work on upstreaming the changes.
 - daemon: Deduplicate USB state events emitted over D-Bus.
 - daemon: Don't use hybrid when not available
 - daemon: Cancel other transports, if one succeeded/failed
+- daemon: Return InvalidStateError to caller when credential is excluded.
 - ui: Add Georgian translations. (Thank you, @EkaterinePopova!)
 - ui: Add a portal backend API to credentialsd-ui.
 - ui: Allow setting client PIN during the flow when required.

credentialsd/src/credential_service/hybrid.rs

@@ -3,31 +3,30 @@ use std::fmt::Debug;
 
 use async_stream::stream;
 use futures_lite::Stream;
+use libwebauthn::{
+    proto::CtapError,
+    transport::{
+        Channel, ChannelSettings, Device,
+        cable::{
+            channel::{CableUpdate, CableUxUpdate},
+            qr_code_device::{CableQrCodeDevice, CableTransports, QrCodeOperationHint},
+        },
+    },
+    webauthn::{WebAuthn, error::WebAuthnError},
+};
 use tokio::sync::{
     broadcast,
     mpsc::{self, Sender},
 };
 use tokio_util::sync::CancellationToken;
 use tracing::{debug, error};
 
-use libwebauthn::transport::cable::qr_code_device::{
-    CableQrCodeDevice, CableTransports, QrCodeOperationHint,
-};
-use libwebauthn::transport::{Channel, ChannelSettings, Device};
-use libwebauthn::webauthn::{WebAuthn, error::WebAuthnError};
-use libwebauthn::{
-    proto::CtapError,
-    transport::cable::channel::{CableUpdate, CableUxUpdate},
-};
-
 use credentialsd_common::{
     memfd::write_secret,
     model::{BackgroundEvent, Error},
 };
 
-use crate::model::CredentialRequest;
-
-use super::AuthenticatorResponse;
+use crate::model::{CredentialRequest, CredentialResponse};
 
 pub(crate) trait HybridHandler {
     fn start(
@@ -101,18 +100,31 @@ impl HybridHandler for InternalHybridHandler {
 
                 let wait_for_response_fut = async {
                     loop {
-                        let response: Result<AuthenticatorResponse, _> = match &request {
+                        let response: Result<CredentialResponse, _> = match &request {
                             CredentialRequest::CreatePublicKeyCredentialRequest(make_request) => {
-                                channel
-                                    .webauthn_make_credential(make_request)
-                                    .await
-                                    .map(|response| response.into())
+                                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(|response| response.into())
+                                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 {
@@ -159,8 +171,8 @@ impl HybridHandler for InternalHybridHandler {
                 };
 
                 let terminal_state = match response {
-                    Ok(auth_response) => HybridStateInternal::Completed(Box::new(auth_response)),
-                    Err(_) => HybridStateInternal::Failed,
+                    Ok(auth_response) => HybridStateInternal::Completed(auth_response),
+                    Err(err) => HybridStateInternal::Failed(err),
                 };
                 if let Err(err) = tx.send(terminal_state).await {
                     tracing::error!("Failed to send caBLE update: {:?}", err)
@@ -189,9 +201,9 @@ pub(super) enum HybridStateInternal {
     Connected,
 
     /// Authenticator data
-    Completed(Box<AuthenticatorResponse>),
+    Completed(CredentialResponse),
 
-    Failed,
+    Failed(Error),
     // TODO(cancellation)
     // This isn't actually sent from the server.
     #[allow(dead_code)]
@@ -221,7 +233,7 @@ pub enum HybridState {
     Completed,
 
     /// Hybrid operation failed.
-    Failed,
+    Failed(Error),
 
     // This isn't actually sent from the server.
     UserCancelled,
@@ -235,7 +247,7 @@ impl From<HybridStateInternal> for HybridState {
             HybridStateInternal::Connected => HybridState::Connected,
             HybridStateInternal::Completed(_) => HybridState::Completed,
             HybridStateInternal::UserCancelled => HybridState::UserCancelled,
-            HybridStateInternal::Failed => HybridState::Failed,
+            HybridStateInternal::Failed(err) => HybridState::Failed(err),
         }
     }
 }
@@ -258,7 +270,13 @@ impl From<&HybridState> for BackgroundEvent {
             HybridState::Connected => BackgroundEvent::HybridConnected,
             HybridState::Completed => BackgroundEvent::CeremonyCompleted,
             HybridState::UserCancelled => BackgroundEvent::ErrorCancelled,
-            HybridState::Failed => BackgroundEvent::ErrorAuthenticator,
+            HybridState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator,
+            HybridState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials,
+            HybridState::Failed(Error::CredentialExcluded) => {
+                BackgroundEvent::ErrorCredentialExcluded
+            }
+            HybridState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator,
+            HybridState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal,
         }
     }
 }
@@ -284,7 +302,7 @@ async fn handle_hybrid_updates(
                 CableUpdate::Connected => Some(HybridStateInternal::Connected),
                 CableUpdate::Error(transport_error) => {
                     error!(?transport_error, "Hybrid transport error");
-                    Some(HybridStateInternal::Failed)
+                    Some(HybridStateInternal::Failed(Error::AuthenticatorError))
                 }
             },
         };

credentialsd/src/credential_service/mod.rs

@@ -139,7 +139,11 @@ impl<H: HybridHandler + Send, N: NfcHandler + Send, U: UsbHandler + Send>
                 .unwrap()
                 .start(request, cancellation.clone());
             let ctx = self.ctx.clone();
-            Box::pin(HybridStateStream { inner: stream, ctx })
+            Box::pin(HybridStateStream {
+                inner: stream,
+                ctx,
+                cancellation_token: cancellation.clone(),
+            })
         } else {
             tracing::error!(
                 "Attempted to start hybrid credential flow, but no request context was found."
@@ -162,7 +166,11 @@ impl<H: HybridHandler + Send, N: NfcHandler + Send, U: UsbHandler + Send>
                 .unwrap()
                 .start(request, cancellation.clone());
             let ctx = self.ctx.clone();
-            Box::pin(UsbStateStream { inner: stream, ctx })
+            Box::pin(UsbStateStream {
+                inner: stream,
+                ctx,
+                cancellation_token: cancellation.clone(),
+            })
         } else {
             tracing::error!(
                 "Attempted to start usb credential flow, but no request context was found."
@@ -185,7 +193,11 @@ impl<H: HybridHandler + Send, N: NfcHandler + Send, U: UsbHandler + Send>
                 .unwrap()
                 .start(request, cancellation.clone());
             let ctx = self.ctx.clone();
-            Box::pin(NfcStateStream { inner: stream, ctx })
+            Box::pin(NfcStateStream {
+                inner: stream,
+                ctx,
+                cancellation_token: cancellation.clone(),
+            })
         } else {
             tracing::error!(
                 "Attempted to start nfc credential flow, but no request context was found."
@@ -311,6 +323,7 @@ impl<H: HybridHandler + Send, N: NfcHandler + Send, U: UsbHandler + Send> Manage
 pub struct HybridStateStream<H> {
     inner: H,
     ctx: Arc<Mutex<Option<RequestContext>>>,
+    cancellation_token: CancellationToken,
 }
 
 impl<H> Stream for HybridStateStream<H>
@@ -324,34 +337,19 @@ where
         cx: &mut std::task::Context<'_>,
     ) -> Poll<Option<Self::Item>> {
         let ctx = &self.ctx.clone();
+        let cancellation_token = self.cancellation_token.clone();
         match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) {
             Poll::Pending => Poll::Pending,
             Poll::Ready(Some(HybridEvent { state })) => {
+                if cancellation_token.is_cancelled() {
+                    return Poll::Ready(None);
+                }
                 match &state {
-                    HybridStateInternal::Completed(hybrid_response) => {
-                        let response = match &**hybrid_response {
-                            AuthenticatorResponse::CredentialCreated(make_credential_response) => {
-                                CredentialResponse::from_make_credential(
-                                    make_credential_response,
-                                    &["hybrid"],
-                                    "cross-platform",
-                                )
-                            }
-                            AuthenticatorResponse::CredentialsAsserted(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,
-                                    // as is done for USB.
-                                    &get_assertion_response.assertions[0],
-                                    "cross-platform",
-                                )
-                            }
-                        };
+                    HybridStateInternal::Completed(response) => {
                         complete_request(ctx, Ok(response.clone()));
                     }
-                    HybridStateInternal::Failed => {
-                        complete_request(ctx, Err(CredentialServiceError::AuthenticatorError));
+                    HybridStateInternal::Failed(err) => {
+                        complete_request(ctx, Err(err.clone()));
                     }
                     _ => {}
                 }
@@ -365,6 +363,7 @@ where
 struct UsbStateStream<H> {
     inner: H,
     ctx: Arc<Mutex<Option<RequestContext>>>,
+    cancellation_token: CancellationToken,
 }
 
 impl<H> Stream for UsbStateStream<H>
@@ -378,9 +377,13 @@ where
         cx: &mut std::task::Context<'_>,
     ) -> Poll<Option<Self::Item>> {
         let ctx = &self.ctx.clone();
+        let cancellation_token = self.cancellation_token.clone();
         match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) {
             Poll::Pending => Poll::Pending,
             Poll::Ready(Some(UsbEvent { state })) => {
+                if cancellation_token.is_cancelled() {
+                    return Poll::Ready(None);
+                }
                 match &state {
                     UsbStateInternal::Completed(response) => {
                         complete_request(ctx, Ok(response.clone()));
@@ -401,6 +404,7 @@ where
 struct NfcStateStream<H> {
     inner: H,
     ctx: Arc<Mutex<Option<RequestContext>>>,
+    cancellation_token: CancellationToken,
 }
 
 impl<H> Stream for NfcStateStream<H>
@@ -414,9 +418,14 @@ where
         cx: &mut std::task::Context<'_>,
     ) -> Poll<Option<Self::Item>> {
         let ctx = &self.ctx.clone();
+        let cancellation_token = self.cancellation_token.clone();
         match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) {
             Poll::Pending => Poll::Pending,
             Poll::Ready(Some(NfcEvent { state })) => {
+                if cancellation_token.is_cancelled() {
+                    return Poll::Ready(None);
+                }
+
                 match &state {
                     NfcStateInternal::Completed(response) => {
                         complete_request(ctx, Ok(response.clone()));
@@ -770,15 +779,24 @@ mod tests {
     {
         Box::pin(async_stream::stream! {
             let Some(mut rx) = rx else { return; };
+            // This allows to simulate when the handler detected cancellation,
+            // but still emit a single event after cancellation to simulate a
+            // race.
+            let mut cancel_detected = false;
             loop {
                 tokio::select! {
                     biased;
-                    _ = cancellation.cancelled() => {
+                    _ = cancellation.cancelled(), if !cancel_detected => {
+                        cancel_detected = true;
                         cancelled.store(true, Ordering::SeqCst);
-                        break;
                     }
                     maybe = rx.recv() => match maybe {
-                        Some(state) => yield wrap(state),
+                        Some(state) => {
+                            yield wrap(state)
+                            if cancel_detected {
+                                break;
+                            }
+                        },
                         None => break, // all senders dropped
                     }
                 }
@@ -1018,6 +1036,10 @@ mod tests {
             cancellation_token.is_cancelled(),
             "Cancellation token should be triggered after cancel_request"
         );
+        // Add explicit post-cancellation message.
+        usb_ref.shift_state(UsbStateInternal::Failed(CredentialServiceError::Internal(
+            "Cancelled".to_string(),
+        )));
 
         // biased select! polls cancellation first, discarding the queued states
         let usb_remaining: Vec<_> = usb_stream.collect().await;

credentialsd/src/credential_service/nfc.rs

@@ -547,7 +547,7 @@ impl From<&NfcState> for BackgroundEvent {
             NfcState::Completed => BackgroundEvent::CeremonyCompleted,
             NfcState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator,
             NfcState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials,
-            NfcState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorAuthenticator,
+            NfcState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorCredentialExcluded,
             NfcState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator,
             NfcState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal,
         }

credentialsd/src/credential_service/usb.rs

@@ -683,7 +683,7 @@ impl From<&UsbState> for BackgroundEvent {
             UsbState::Completed => BackgroundEvent::CeremonyCompleted,
             UsbState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator,
             UsbState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials,
-            UsbState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorAuthenticator,
+            UsbState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorCredentialExcluded,
             UsbState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator,
             UsbState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal,
         }

credentialsd/src/dbus/flow_control.rs

@@ -352,15 +352,9 @@ impl CredentialRequestController for CredentialRequestControllerClient {
             tracing::error!("Credential response channel closed prematurely");
             WebAuthnError::NotAllowedError
         })?;
-        // TODO: CredentialServiceError is returning the wrong errors types to the flow controller
-        // We need to be able to bubble up the InvalidStateError, when the
-        // selected authenticator has the credential known by the RP, and
-        // the user wants to let the RP know.
-        // All the other possible errors from the spec (AbortError,
-        // ConstraintError, SecurityError, TypeError) should be handled
-        // earlier by the gateway.
-        // Every other error should be squashed into NotAllowed as a catch-all
-        // For now, just squashing.
-        response.map_err(|_| WebAuthnError::NotAllowedError)
+        response.map_err(|err| match err {
+            CredentialServiceError::CredentialExcluded => WebAuthnError::InvalidStateError,
+            _ => WebAuthnError::NotAllowedError,
+        })
     }
 }