Update passkey-rs #606
Update passkey-rs #606
Conversation
|
Claude finished @iinuwa's task —— View job Code Review CompleteTasks
Code Review SummaryOverall AssessmentStatus: Approved with Minor Concerns ✅ This PR successfully updates the passkey-rs fork to sync with upstream changes. The implementation of the two main breaking changes is correct and consistent across the codebase. Breaking Changes Implemented1.
|
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
|
I've noticed that your patches on top of 1Password's passkey-rs are bumping Ideally that version should be as low as it can to increase compatibility, and none of the changes look like they need the bump. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #606 +/- ##
==========================================
- Coverage 78.61% 78.57% -0.04%
==========================================
Files 281 283 +2
Lines 29131 29187 +56
==========================================
+ Hits 22901 22934 +33
- Misses 6230 6253 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@dani-garcia, no, that's not required. I had seen the recent bump in the SDK's Rust version to 1.91.1, so I assumed I should make this the same in our fork to unify the Rust versions across crates we owned. I can decrease it! |
|
Ah, yeah updating the
This ensures we use the latest version but allow users to build with older versions. You can see that we do the same thing ourselves, we bumped to 1.91.1, but our rust-version is still 1.85: Line 13 in 26df671 Sidenote, you may need to bump that file from 1.85 to 1.85.1 to match passkey-rs, otherwise CI might complain |
iinuwa
force-pushed
the
iinuwa/update-passkey-rs
branch
from
0d43370 to
9536365
Compare
iinuwa
force-pushed
the
iinuwa/update-passkey-rs
branch
from
9536365 to
4e5cb4a
Compare
|
Nice, thanks for the changes! Happy to know we're almost at the end goal of removing the need for our fork. Two more things that are causing problems in CI:
|
This function is intended to signal whether user verification may potentially be available for the authenticator, not whether user verification will be available for a particular request. Since this can be known ahead of time for our clients, there is no need for this function to be async. Making this async also aligns us closer with upstream. Per-request availability checks should be done in the caller's implementation of check_user().
iinuwa
force-pushed
the
iinuwa/update-passkey-rs
branch
from
4e5cb4a to
2576075
Compare
|
Thanks for pointing all that out. I think I've addressed all of the concerns now:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let asset_link_url = { | ||
| let url = value | ||
| .asset_link_url | ||
| .unwrap_or_else(|| format!("https://{}/.well-known/assetlinks.json", value.host)); |
There was a problem hiding this comment.
The asset link URL construction has been changed to always generate a URL from the host, even when asset_link_url is provided. This is a breaking behavioral change that differs from the previous implementation.
Previous behavior: Used the provided asset_link_url if available, otherwise returned None.
New behavior: Always generates a URL from the host, using the provided asset_link_url or falling back to the default well-known path.
While this change makes the URL always available (which passkey-rs now expects), it may silently ignore an explicitly provided asset_link_url that differs from the host-derived URL. Consider whether this is the intended behavior or if there should be validation to ensure consistency.
There was a problem hiding this comment.
unwrap_or_else() will take the provided asset link URL or compute a fallback value from the value.host parameter. It is incorrect to say that this always generates a URL from the host.
This performs the same functionality as before: passkey-rs was doing this internally when None is passed, but has shifted that responsibility to the caller.
| rp_id: value.rp_id.clone(), | ||
| user_handle: user_handle.map(|u| u.into_bytes().into()), | ||
| counter, | ||
| extensions: CredentialExtensions { hmac_secret: None }, |
There was a problem hiding this comment.
Good: The new extensions field is properly initialized with hmac_secret: None. This ensures compatibility with the updated passkey-rs structure for Passkey.
| }; | ||
|
|
||
| let attestation_object = response.as_bytes().to_vec(); | ||
| let attestation_object = response.as_webauthn_bytes().to_vec(); |
There was a problem hiding this comment.
The method call has been updated from as_bytes() to as_webauthn_bytes(). This is correct for the new passkey-rs API, but note that this returns WebAuthn format bytes which may have different encoding than the raw CBOR bytes. Verify that downstream consumers expect WebAuthn-formatted attestation objects.
| let result = self.credential_store.find_credentials(None, rp_id).await?; | ||
| let result = self | ||
| .credential_store | ||
| .find_credentials(None, rp_id, user_handle) |
There was a problem hiding this comment.
The user_handle parameter has been added to find_credentials as required by the updated passkey-rs API. However, this is a silent breaking change to the trait interface:
- All implementations of
Fido2CredentialStoremust now handle this parameter - The filtering logic for credentials now potentially includes user_handle matching
- Passing
Nonepreserves backward compatibility, but implementations should validate whether user_handle filtering is properly implemented
Consider whether the credential store implementations in client apps have been updated to properly filter by user_handle when provided.
| async fn convert_requested_uv(&self, uv: UV) -> bool { | ||
| let verification_enabled = self.user_interface.is_verification_enabled().await; | ||
| fn convert_requested_uv(&self, uv: UV) -> bool { | ||
| let verification_enabled = self.user_interface.is_verification_enabled(); |
There was a problem hiding this comment.
Good: The method signature change from async fn to fn for convert_requested_uv is correct. Since is_verification_enabled() is now synchronous, this method no longer needs to be async.
| ) -> Result<Vec<CipherView>, BitFido2CallbackError> { | ||
| self.0 | ||
| .find_credentials(ids, rip_id) | ||
| .find_credentials(ids, rip_id, user_handle) |
There was a problem hiding this comment.
The user_handle parameter is correctly passed through from the trait implementation to the internal call. The implementation properly maintains the signature compatibility with the updated trait definition.
| )) | ||
|
|
||
| let _ = try await authenticator.silentlyDiscoverCredentials(rpId: "") | ||
| let _ = try await authenticator.silentlyDiscoverCredentials(rpId: "", userHandle: nil) |
There was a problem hiding this comment.
Good: The Swift demo app has been correctly updated to pass the new userHandle parameter (as nil in this test case). This demonstrates proper handling of the breaking change.
| } | ||
|
|
||
| func findCredentials(ids: [Data]?, ripId: String) async throws -> [BitwardenSdk.CipherView] { | ||
| func findCredentials(ids: [Data]?, ripId: String, userHandle: Data?) async throws -> [BitwardenSdk.CipherView] { |
There was a problem hiding this comment.
Good: The findCredentials method signature has been updated to include the userHandle parameter. This correctly implements the breaking change from the trait definition.
| } | ||
|
|
||
| func isVerificationEnabled() async -> Bool { | ||
| func isVerificationEnabled() -> Bool { |
There was a problem hiding this comment.
Good: The method signature has been correctly updated from async func to func for isVerificationEnabled(), reflecting the synchronous nature of this method in the updated passkey-rs API.
🎟️ Tracking
PM-17241
📔 Objective
This updates the SDK to point to a new version of our passkey-rs fork that has been synced with upstream. (The remaining differences between upstream and our fork are documented in the README of our fork.)
If approved, we should update
passkey-rs'smainbranch to point to the version referenced in this PR (bitwarden/passkey-rs@2e5c0ba).🚨 Breaking Changes
is_user_verification_enabled()is no longer asynchronous. Clients should implement any dynamic checks for UV availability in thecheck_user()implementation.user_handle has been added to thefind_credential()method as an optional parameter. If the incoming request contains the user handle, then clients should pass it on. Otherwise, pass anull` value, and the previous behavior.iOS PR: bitwarden/ios#2190
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes