Changes based on review

crates/bitwarden/Cargo.toml

@@ -51,7 +51,7 @@ getrandom = { version = ">=0.2.9, <0.3", features = ["js"] }
 hmac = ">=0.12.1, <0.13"
 log = ">=0.4.18, <0.5"
 p256 = ">=0.13.2, <0.14"
-passkey = { git = "https://github.com/bitwarden/passkey-rs", rev = "12da886102707f87ad97e499c857c0857ece0b85", optional = true }
+passkey = { git = "https://github.com/bitwarden/passkey-rs", rev = "2df7838b8832322d309c399a4c0f6ca2979af12d", optional = true }
 rand = ">=0.8.5, <0.9"
 reqwest = { version = ">=0.12, <0.13", features = [
     "http2",

crates/bitwarden/src/platform/fido2/authenticator.rs

@@ -33,7 +33,7 @@ impl<'a> Fido2Authenticator<'a> {
         &mut self,
         request: MakeCredentialRequest,
     ) -> Result<MakeCredentialResult> {
-        let mut authenticator = self.get_passkey_authenticator();
+        let mut authenticator = self.get_authenticator();
 
         let response = authenticator
             .make_credential(ctap2::make_credential::Request {
@@ -51,22 +51,18 @@ impl<'a> Fido2Authenticator<'a> {
                     .pub_key_cred_params
                     .into_iter()
                     .map(TryInto::try_into)
-                    .collect::<Result<Vec<_>, _>>()?,
+                    .collect::<Result<_, _>>()?,
                 exclude_list: request
                     .exclude_list
-                    .map(|x| {
-                        x.into_iter()
-                            .map(TryInto::try_into)
-                            .collect::<Result<Vec<_>, _>>()
-                    })
+                    .map(|x| x.into_iter().map(TryInto::try_into).collect())
                     .transpose()?,
                 extensions: request
                     .extensions
                     .map(|e| serde_json::from_str(&e))
                     .transpose()?,
                 // TODO(Fido2): Where do we get these from?
                 options: passkey::types::ctap2::make_credential::Options {
-                    rk: true,
+                    rk: request.require_resident_key,
                     up: true,
                     uv: true,
                 },
@@ -97,7 +93,7 @@ impl<'a> Fido2Authenticator<'a> {
         &mut self,
         request: GetAssertionRequest,
     ) -> Result<GetAssertionResult> {
-        let mut authenticator = self.get_passkey_authenticator();
+        let mut authenticator = self.get_authenticator();
 
         let response = authenticator
             .get_assertion(ctap2::get_assertion::Request {
@@ -116,7 +112,6 @@ impl<'a> Fido2Authenticator<'a> {
                     .map(|e| serde_json::from_str(&e))
                     .transpose()?,
                 options: passkey::types::ctap2::make_credential::Options {
-                    // TODO(Fido2): Are these right?
                     rk: request.options.rk,
                     up: true,
                     uv: request.options.uv != UV::Discouraged,
@@ -159,7 +154,7 @@ impl<'a> Fido2Authenticator<'a> {
             .collect())
     }
 
-    pub(super) fn get_passkey_authenticator(
+    pub(super) fn get_authenticator(
         &self,
     ) -> Authenticator<CredentialStoreImpl, UserValidationMethodImpl> {
         Authenticator::new(
@@ -200,7 +195,6 @@ pub(super) struct UserValidationMethodImpl<'a> {
 #[async_trait::async_trait]
 impl passkey::authenticator::CredentialStore for CredentialStoreImpl<'_> {
     type PasskeyItem = CipherViewContainer;
-
     async fn find_credentials(
         &self,
         ids: Option<&[passkey::types::webauthn::PublicKeyCredentialDescriptor]>,
@@ -258,22 +252,23 @@ impl passkey::authenticator::CredentialStore for CredentialStoreImpl<'_> {
         cred: Passkey,
         user: passkey::types::ctap2::make_credential::PublicKeyCredentialUserEntity,
         rp: passkey::types::ctap2::make_credential::PublicKeyCredentialRpEntity,
+        // TODO(Fido2): Use this
+        _options: passkey::types::ctap2::get_assertion::Options,
     ) -> Result<(), StatusCode> {
         // This is just a wrapper around the actual implementation to allow for ? error handling
-        // TODO(Fido2): User is unused, do we need it?
         async fn inner(
             this: &mut CredentialStoreImpl<'_>,
             cred: Passkey,
-            _user: passkey::types::ctap2::make_credential::PublicKeyCredentialUserEntity,
+            user: passkey::types::ctap2::make_credential::PublicKeyCredentialUserEntity,
             rp: passkey::types::ctap2::make_credential::PublicKeyCredentialRpEntity,
         ) -> Result<()> {
             let creds = this
                 .authenticator
                 .credential_store
-                .find_credentials(None, rp.id)
+                .find_credentials(None, rp.id.clone())
                 .await?;
 
-            let cred: Fido2CredentialFullView = cred.clone().try_into()?;
+            let cred = Fido2CredentialFullView::try_from_credential(cred, user, rp)?;
 
             let mut picked = this
                 .authenticator
@@ -322,9 +317,14 @@ impl passkey::authenticator::CredentialStore for CredentialStoreImpl<'_> {
                 .await?;
 
             // Get the credential with the same id as the one we're updating
-            // TODO(Fido2): Is this the right id to check?
             creds.retain(|c| {
-                c.id.map(|i| i.as_bytes().as_slice() == cred.credential_id.deref())
+                c.login
+                    .as_ref()
+                    .and_then(|l| l.fido2_credentials.as_ref())
+                    .and_then(|f| f.first())
+                    .map(|cipher_cred| {
+                        cipher_cred.credential_id.expose().as_bytes() == cred.credential_id.deref()
+                    })
                     .unwrap_or(false)
             });
             let cred = match creds.len() {

crates/bitwarden/src/platform/fido2/client.rs

@@ -23,7 +23,7 @@ impl<'a> Fido2Client<'a> {
         client_data: ClientData,
     ) -> Result<PublicKeyCredentialAuthenticatorAttestationResponse> {
         let mut client =
-            passkey::client::Client::new(self.authenticator.get_passkey_authenticator());
+            passkey::client::Client::new(self.authenticator.get_authenticator());
 
         // TODO(Fido2): Handle this error
         let origin = Url::parse(&origin).expect("Invalid URL");
@@ -69,7 +69,7 @@ impl<'a> Fido2Client<'a> {
         client_data: ClientData,
     ) -> Result<PublicKeyCredentialAuthenticatorAssertionResponse> {
         let mut client =
-            passkey::client::Client::new(self.authenticator.get_passkey_authenticator());
+            passkey::client::Client::new(self.authenticator.get_authenticator());
 
         // TODO(Fido2): Handle this error
         let origin = Url::parse(&origin).expect("Invalid URL");

crates/bitwarden/src/platform/fido2/mod.rs

@@ -120,10 +120,12 @@ impl TryFrom<Fido2CredentialFullView> for Passkey {
     }
 }
 
-impl TryFrom<Passkey> for Fido2CredentialFullView {
-    type Error = crate::error::Error;
-
-    fn try_from(value: Passkey) -> Result<Self, Self::Error> {
+impl Fido2CredentialFullView {
+    pub(crate) fn try_from_credential(
+        value: Passkey,
+        user: passkey::types::ctap2::make_credential::PublicKeyCredentialUserEntity,
+        rp: passkey::types::ctap2::make_credential::PublicKeyCredentialRpEntity,
+    ) -> Result<Self> {
         let cred_id: Vec<u8> = value.credential_id.into();
 
         Ok(Fido2CredentialFullView {
@@ -133,14 +135,14 @@ impl TryFrom<Passkey> for Fido2CredentialFullView {
             key_curve: SensitiveString::new(Box::new("P-256".to_owned())),
             key_value: SensitiveVec::new(Box::new(cose_key_to_pkcs8(&value.key)?)),
             rp_id: SensitiveString::new(Box::new(value.rp_id)),
-            rp_name: None,
+            rp_name: rp.name.map(|n| SensitiveString::new(Box::new(n))),
             user_handle: Some(SensitiveVec::new(Box::new(cred_id))),
 
             // TODO(Fido2): Storing the counter as a string seems like a bad idea, but we don't have
             // support for EncString -> number decryption
             counter: SensitiveString::new(Box::new(value.counter.unwrap_or(0).to_string())),
-            user_name: None,
-            user_display_name: None,
+            user_name: user.name.map(|n| SensitiveString::new(Box::new(n))),
+            user_display_name: user.display_name.map(|n| SensitiveString::new(Box::new(n))),
             // TODO(Fido2): Same as the counter, but with booleans this time
             discoverable: SensitiveString::new(Box::new("true".to_owned())),
             creation_date: chrono::offset::Utc::now(),