Incorporate Bitwarden FIDO SDK/passkey-rs changes

[iinuwa]

🎟️ Tracking

PM-17241

Depends on bitwarden/sdk-internal#606.

📔 Objective

This addresses the breaking changes introduced by syncing our fork of passkey-rs with upstream.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation 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 confirmed issue 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

[github-actions]

Checkmarx One – Scan Summary & Details – 58a47477-537b-4d55-8eaf-74793effdb9b

Great job! No new security vulnerabilities introduced in this pull request

[fedemkr]

❓ Why does this return true at all times now?

[iinuwa]

Good question. This part of the class is in fulfillment of the PasskeyAuthenticator trait from passkey-rs. That is intended to be a static configuration for whether user verification would ever be available for an authenticator, not whether it's available for this specific call. (It essentially enables an early return from the library for processing UV.) Cf 1Password/passkey-rs#74 (comment)

This dynamic call should instead deferred to the checkUser() function, which we're still doing in checkEnforceRequiredVerification() at L234 (called from checkUser() at L144)

[fedemkr]

Oh alright, have you tested that the flows keep working as expected for .preferred UV? Particularly with session timeout -> Never.
If so, given that's the case then in the same file this should be removed alongside with their calls as part of this change as they're not needed anymore and just were there to workaround having UV in isVerificationEnabled which was a wrong assumption:

private var userVerificationPreference: Uv? (code)
func setupCurrentUserVerificationPreference(userVerificationPreference: Uv) (code)

[iinuwa]

On the simulator, I can confirm that the iPhone prompted with FaceID when user ID after removing the code referenced above and setting the session timeout to "Never".

(Curiously, it seems that when I set userVerification: discouraged, iOS is upgrading the request to required. preferred is still set as preferred.)

[KatherineInCode]

🤔 Is this function likely to change in the future, to have more logic in it? If not, it would make sense to me to just delete it, and cascade up the true.

[iinuwa]

No, not on iOS, where using passkeys requires setting up at least one local OS auth method. After thinking about this, I agree, I think it would be good to push it into the SDK.

This method exists because the SDK is passing through a required method to fulfill a passkey-rs trait, so we can't remove it without changing it for all consumers of the SDK. I think there are two reasons we should push this into the SDK:

• I believe that all of our clients should have a way to do user verification, even it's just redoing whatever auth/unlock ceremony they have enabled for their vault. So this would just become a static value inside the SDK.
• We are only using this method to:
  • set the options.uv value in authenticatorGetInfo, which is merely a hint for platforms, and to
  • set the options.uv value in checkUser on authenticatorMakeCredential and authenticatorGetAssertion—which is always ignored in our implementation, since we pass the WebAuthn UserVerificationRequirement enum instead.¹

For those reasons, and because of the confusion having this method introduces, I think removing this method from the SDK trait and letting the SDK abstract over it would be beneficial.

But the current implementation, where isVerificationEnabled() always returns true produces the same behavior as pushing this SDK trait method, so I'd recommend following that up in a separate PR in the SDK.

---

I think I should clarify two other things that I am not talking about changing:

• changing checkUser(options: BitwardenSdk.CheckUserOptions, ...) to checkUser(shouldVerify: bool, ...). Even though we could decide whether we should prompt the user for verification from the SDK, if we change this to a simple bool, it still loses information on how hard we should try to verify the user. For example, the current iOS implementation will prompt for UV on both preferred and required, but if set to required and no verification methods are available, it will start the PIN setup flow, but it will just return userVerified: false if only preferred.

• removing shouldThrowEnforcingRequiredVerification parameter from checkUser(). I'm a little fuzzy on this one. It seems that we don't set this flag when calling this from the SDK, since the SDK will check this and return an appropriate CTAP2 error code. But we call this method from other places that don't go through the SDK FIDO methods (VaultAutofillListProcessor, AddEditItemProcessor), so this method is used there. I don't know enough about these two call sites to know whether this is useful or not.

Footnotes

1. To expand on this point, passkey-rs only sends the CTAP2 options.uv boolean, which loses context on whether the UV request can be ignored or not (e.g. preferred is not represented.) So instead, we send the original WebAuthn UV preference, discouraged, required, or preferred (found in options.authenticatorSelection.userVerification in navigator.credential.create() and userVerification in navigator.credential.get()) which gives more context. ↩

[abergs]

A ticket has been created to clean up / refine ´isVerificationEnabled´ so that we can unblock this PR https://bitwarden.atlassian.net/browse/PM-30454

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.58%. Comparing base (79c37d7) to head (57f3f0e).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2190       +/-   ##
===========================================
- Coverage   85.47%   43.58%   -41.89%     
===========================================
  Files        1746      584     -1162     
  Lines      147479    30505   -116974     
===========================================
- Hits       126053    13295   -112758     
+ Misses      21426    17210     -4216     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.