Merge pull request #76 from bitwarden/bitwarden/add-ui-hints

Progdrasil · web-flow · commit 2677e25375e4 · 2025-10-30T12:45:04.000-04:00
Add more context to check_user()
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+- ⚠ BREAKING: The `UserValidationMethod` trait has been updated to use `UiHint`
+  to give the implementation more information about the request, which can be used
+  to decide whether additional validations are needed. To reflect this, the
+  `UserValidationMethod` trait now also returns which validations were performed.
+
 ## Passkey v0.5.0
 
 - Migrate project to Rust 2024 edition
diff --git a/passkey-authenticator/src/authenticator.rs b/passkey-authenticator/src/authenticator.rs
@@ -4,7 +4,7 @@ use passkey_types::{
     webauthn,
 };
 
-use crate::{CredentialStore, UserValidationMethod};
+use crate::{CredentialStore, UserValidationMethod, user_validation::UiHint};
 
 pub mod extensions;
 mod get_assertion;
@@ -201,16 +201,16 @@ where
     ///            CTAP2_ERR_OPERATION_DENIED error.
     async fn check_user(
         &self,
+        hint: UiHint<'_, <U as UserValidationMethod>::PasskeyItem>,
         options: &passkey_types::ctap2::make_credential::Options,
-        credential: Option<&<U as UserValidationMethod>::PasskeyItem>,
     ) -> Result<Flags, Ctap2Error> {
         if options.uv && self.user_validation.is_verification_enabled() != Some(true) {
             return Err(Ctap2Error::UnsupportedOption);
         };
 
         let check_result = self
             .user_validation
-            .check_user(credential, options.up, options.uv)
+            .check_user(hint, options.up, options.uv)
             .await?;
 
         if options.up && !check_result.presence {
diff --git a/passkey-authenticator/src/authenticator/get_assertion.rs b/passkey-authenticator/src/authenticator/get_assertion.rs
@@ -7,7 +7,10 @@ use passkey_types::{
     webauthn::PublicKeyCredentialUserEntity,
 };
 
-use crate::{Authenticator, CredentialStore, UserValidationMethod, private_key_from_cose_key};
+use crate::{
+    Authenticator, CredentialStore, UserValidationMethod, private_key_from_cose_key,
+    user_validation::UiHint,
+};
 
 impl<S, U> Authenticator<S, U>
 where
@@ -70,9 +73,11 @@ where
         // 7. Collect user consent if required. This step MUST happen before the following steps due
         //    to privacy reasons (i.e., authenticator cannot disclose existence of a credential
         //    until the user interacted with the device):
-        let flags = self
-            .check_user(&input.options, maybe_credential.as_ref().ok())
-            .await?;
+        let hint = match &maybe_credential {
+            Ok(credential) => UiHint::RequestExistingCredential(credential),
+            Err(_) => UiHint::InformNoCredentialsFound,
+        };
+        let flags = self.check_user(hint, &input.options).await?;
 
         // 8. If no credentials were located in step 1, return CTAP2_ERR_NO_CREDENTIALS.
         let mut credential = maybe_credential?
diff --git a/passkey-authenticator/src/authenticator/get_assertion/tests.rs b/passkey-authenticator/src/authenticator/get_assertion/tests.rs
@@ -1,7 +1,7 @@
 use passkey_types::{
     Passkey, StoredHmacSecret,
     ctap2::{
-        Aaguid,
+        Aaguid, Ctap2Error,
         get_assertion::{ExtensionInputs, Options, Request},
     },
     rand::random_vec,
@@ -10,6 +10,7 @@ use passkey_types::{
 use crate::{
     Authenticator, MockUserValidationMethod,
     extensions::{self, prf_eval_request},
+    user_validation::MockUiHint,
 };
 
 fn create_passkey(hmac_secret: Option<Vec<u8>>) -> Passkey {
@@ -42,18 +43,40 @@ fn good_request() -> Request {
     }
 }
 
+#[tokio::test]
+async fn get_assertion_returns_no_credentials_found() {
+    // Arrange
+    let request = good_request();
+    let store = None;
+    let mut authenticator = Authenticator::new(
+        Aaguid::new_empty(),
+        store,
+        MockUserValidationMethod::verified_user_with_hint(1, MockUiHint::InformNoCredentialsFound),
+    );
+
+    // Act
+    let response = authenticator.get_assertion(request).await;
+
+    // Assert
+    assert_eq!(response.unwrap_err(), Ctap2Error::NoCredentials.into(),);
+}
+
 #[tokio::test]
 async fn get_assertion_increments_signature_counter_when_counter_is_some() {
     // Arrange
     let request = good_request();
-    let store = Some(Passkey {
+    let passkey = Passkey {
         counter: Some(9000),
         ..create_passkey(None)
-    });
+    };
+    let store = Some(passkey.clone());
     let mut authenticator = Authenticator::new(
         Aaguid::new_empty(),
         store,
-        MockUserValidationMethod::verified_user(1),
+        MockUserValidationMethod::verified_user_with_hint(
+            1,
+            MockUiHint::RequestExistingCredential(passkey),
+        ),
     );
 
     // Act
diff --git a/passkey-authenticator/src/authenticator/make_credential.rs b/passkey-authenticator/src/authenticator/make_credential.rs
@@ -1,5 +1,3 @@
-use std::ops::Not;
-
 use p256::SecretKey;
 use passkey_types::{
     Passkey,
@@ -9,18 +7,16 @@ use passkey_types::{
     },
 };
 
-use crate::{Authenticator, CoseKeyPair, CredentialStore, UserValidationMethod};
+use crate::{Authenticator, CoseKeyPair, CredentialStore, UiHint, UserValidationMethod};
 
 impl<S, U> Authenticator<S, U>
 where
     S: CredentialStore + Sync,
-    U: UserValidationMethod + Sync,
+    U: UserValidationMethod<PasskeyItem = <S as CredentialStore>::PasskeyItem> + Sync,
 {
     /// This method is invoked by the host to request generation of a new credential in the authenticator.
     pub async fn make_credential(&mut self, input: Request) -> Result<Response, StatusCode> {
-        let flags = if input.options.up {
-            self.check_user(&input.options, None).await?
-        } else {
+        if !input.options.up {
             return Err(Ctap2Error::InvalidOption.into());
         };
 
@@ -35,22 +31,23 @@ where
             .as_ref()
             .filter(|list| !list.is_empty())
             .is_some()
-            && self
+        {
+            if let Some(excluded_credential) = self
                 .store()
                 .find_credentials(
                     input.exclude_list.as_deref(),
                     &input.rp.id,
                     Some(&input.user.id),
                 )
                 .await?
-                .is_empty()
-                .not()
-        {
-            // TODO: We should instead remove the excluded credentials from a possible future
-            // "update list".
-            // This would require prompting from within the context of this function using
-            // the [UserValidationMethod]
-            log::warn!("An excluded credential was found, may be overwritten.")
+                .first()
+            {
+                self.check_user(
+                    UiHint::InformExcludedCredentialFound(excluded_credential),
+                    &input.options,
+                )
+                .await?;
+            }
         }
 
         // 2. If the pubKeyCredParams parameter does not contain a valid COSEAlgorithmIdentifier
@@ -98,6 +95,12 @@ where
         //    authenticator-specific way (e.g., flash the LED light). Request permission to create
         //    a credential. If the user declines permission, return the CTAP2_ERR_OPERATION_DENIED
         //    error.
+        let flags = self
+            .check_user(
+                UiHint::RequestNewCredential(&input.user.clone().into(), &input.rp),
+                &input.options,
+            )
+            .await?;
 
         // 9. Generate a new credential key pair for the algorithm specified.
         let credential_id = passkey_types::rand::random_vec(self.credential_id_length.into());
@@ -124,7 +127,7 @@ where
             key: private,
             rp_id: input.rp.id.clone(),
             credential_id: credential_id.into(),
-            user_handle: is_passkey_rk.then(|| input.user.id.clone()),
+            user_handle: is_passkey_rk.then_some(input.user.id.clone()),
             counter: self.make_credentials_with_signature_counter.then_some(0),
             extensions: extensions.credential,
         };
diff --git a/passkey-authenticator/src/authenticator/make_credential/tests.rs b/passkey-authenticator/src/authenticator/make_credential/tests.rs
@@ -21,7 +21,7 @@ use crate::{
     MemoryStore,
     credential_store::{DiscoverabilitySupport, StoreInfo},
     extensions,
-    user_validation::MockUserValidationMethod,
+    user_validation::{MockUiHint, MockUserValidationMethod},
 };
 
 fn good_request() -> Request {
@@ -54,14 +54,17 @@ fn good_request() -> Request {
 
 #[tokio::test]
 async fn assert_storage_on_success() {
+    let request = good_request();
+
     let shared_store = Arc::new(Mutex::new(MemoryStore::new()));
-    let user_mock = MockUserValidationMethod::verified_user(1);
+    let user_mock = MockUserValidationMethod::verified_user_with_hint(
+        1,
+        MockUiHint::RequestNewCredential(request.user.clone().into(), request.rp.clone()),
+    );
 
     let mut authenticator =
         Authenticator::new(Aaguid::new_empty(), shared_store.clone(), user_mock);
 
-    let request = good_request();
-
     authenticator
         .make_credential(request)
         .await
@@ -93,7 +96,27 @@ async fn assert_excluded_credentials() {
         extensions: Default::default(),
     };
     let shared_store = Arc::new(Mutex::new(MemoryStore::new()));
-    let user_mock = MockUserValidationMethod::verified_user(1);
+    let mut user_mock = MockUserValidationMethod::verified_user_with_hint(
+        1,
+        MockUiHint::InformExcludedCredentialFound(passkey.clone()),
+    );
+    let expected_user = response.user.clone().into();
+    let expected_rp = response.rp.clone();
+    user_mock
+        .expect_check_user()
+        .withf(move |hint, _up, _uv| {
+            if let UiHint::RequestNewCredential(user, rp) = *hint {
+                *user == expected_user && *rp == expected_rp
+            } else {
+                false
+            }
+        })
+        .returning(|_hint, up, uv| {
+            Ok(crate::UserCheck {
+                presence: up,
+                verification: uv,
+            })
+        });
 
     shared_store.lock().await.insert(cred_id.into(), passkey);
 
@@ -112,7 +135,7 @@ async fn assert_excluded_credentials() {
 
 #[tokio::test]
 async fn assert_unsupported_algorithm() {
-    let user_mock = MockUserValidationMethod::verified_user(1);
+    let user_mock = MockUserValidationMethod::verified_user(0);
     let mut authenticator = Authenticator::new(Aaguid::new_empty(), MemoryStore::new(), user_mock);
 
     let request = Request {
@@ -401,7 +424,7 @@ async fn make_credential_returns_err_when_rk_is_requested_but_not_supported() {
 
     // Arrange
     let store = StoreWithoutDiscoverableSupport;
-    let user_mock = MockUserValidationMethod::verified_user(1);
+    let user_mock = MockUserValidationMethod::verified_user(0);
     let request = good_request();
     let mut authenticator = Authenticator::new(Aaguid::new_empty(), store, user_mock);
     authenticator.set_make_credentials_with_signature_counter(true);
diff --git a/passkey-authenticator/src/authenticator/tests.rs b/passkey-authenticator/src/authenticator/tests.rs
@@ -1,6 +1,8 @@
 use passkey_types::ctap2::{Aaguid, Flags};
 
-use crate::{Authenticator, CredentialIdLength, MockUserValidationMethod, UserCheck};
+use crate::{
+    Authenticator, CredentialIdLength, MockUserValidationMethod, UserCheck, user_validation::UiHint,
+};
 
 #[tokio::test]
 async fn check_user_does_not_check_up_or_uv_when_not_requested() {
@@ -31,7 +33,10 @@ async fn check_user_does_not_check_up_or_uv_when_not_requested() {
     };
 
     // Act
-    let result = authenticator.check_user(&options, None).await.unwrap();
+    let result = authenticator
+        .check_user(UiHint::InformNoCredentialsFound, &options)
+        .await
+        .unwrap();
 
     // Assert
     assert_eq!(result, Flags::empty());
@@ -66,7 +71,10 @@ async fn check_user_checks_up_when_requested() {
     };
 
     // Act
-    let result = authenticator.check_user(&options, None).await.unwrap();
+    let result = authenticator
+        .check_user(UiHint::InformNoCredentialsFound, &options)
+        .await
+        .unwrap();
 
     // Assert
     assert_eq!(result, Flags::UP);
@@ -104,7 +112,10 @@ async fn check_user_checks_uv_when_requested() {
     };
 
     // Act
-    let result = authenticator.check_user(&options, None).await.unwrap();
+    let result = authenticator
+        .check_user(UiHint::InformNoCredentialsFound, &options)
+        .await
+        .unwrap();
 
     // Assert
     assert_eq!(result, Flags::UP | Flags::UV);
@@ -139,7 +150,9 @@ async fn check_user_returns_operation_denied_when_up_was_requested_but_not_retur
     };
 
     // Act
-    let result = authenticator.check_user(&options, None).await;
+    let result = authenticator
+        .check_user(UiHint::InformNoCredentialsFound, &options)
+        .await;
 
     // Assert
     assert_eq!(
@@ -180,7 +193,9 @@ async fn check_user_returns_operation_denied_when_uv_was_requested_but_not_retur
     };
 
     // Act
-    let result = authenticator.check_user(&options, None).await;
+    let result = authenticator
+        .check_user(UiHint::InformNoCredentialsFound, &options)
+        .await;
 
     // Assert
     assert_eq!(
@@ -207,7 +222,9 @@ async fn check_user_returns_unsupported_option_when_uv_was_requested_but_is_not_
     };
 
     // Act
-    let result = authenticator.check_user(&options, None).await;
+    let result = authenticator
+        .check_user(UiHint::InformNoCredentialsFound, &options)
+        .await;
 
     // Assert
     assert_eq!(
@@ -249,7 +266,10 @@ async fn check_user_returns_up_and_uv_flags_when_neither_up_or_uv_was_requested_
     };
 
     // Act
-    let result = authenticator.check_user(&options, None).await.unwrap();
+    let result = authenticator
+        .check_user(UiHint::InformNoCredentialsFound, &options)
+        .await
+        .unwrap();
 
     // Assert
     assert_eq!(result, Flags::UP | Flags::UV);
diff --git a/passkey-authenticator/src/lib.rs b/passkey-authenticator/src/lib.rs
@@ -46,7 +46,7 @@ pub use self::{
     credential_store::{CredentialStore, DiscoverabilitySupport, MemoryStore, StoreInfo},
     ctap2::Ctap2Api,
     u2f::U2fApi,
-    user_validation::{UserCheck, UserValidationMethod},
+    user_validation::{UiHint, UserCheck, UserValidationMethod},
 };
 
 #[cfg(any(test, feature = "testable"))]
diff --git a/passkey-authenticator/src/user_validation.rs b/passkey-authenticator/src/user_validation.rs
diff --git a/passkey-types/src/ctap2/make_credential.rs b/passkey-types/src/ctap2/make_credential.rs
diff --git a/passkey/examples/usage.rs b/passkey/examples/usage.rs
diff --git a/passkey/src/lib.rs b/passkey/src/lib.rs
