Fix save_credential failing when the selected cipher doesn't have a Passkey

dani-garcia commented 3 weeks ago

🎟️ Tracking

📔 Objective

The current call to get_selected_credential inside save_credential will try to fetch and decrypt the ciphers FIDO2 credentials, and error if they are not there.

This can only happen when creating a new Passkey, so instead of calling get_selected_credential we just get the value from the lock. The other places where get_selected_credential is used is in update_credential and right at the end of the assertion, register and authenticate operations, so those should be safe.

⏰ 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[bot] commented 3 weeks ago

Checkmarx One – Scan Summary & Details – f2279d40-f563-4a0d-bb3e-bc48fa0ad283

No New Or Fixed Issues Found

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 59.23%. Comparing base (85bfa59) to head (0167bcc).

Files	Patch %	Lines
...ates/bitwarden/src/platform/fido2/authenticator.rs	0.00%	8 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #831 +/- ## ========================================== - Coverage 59.27% 59.23% -0.04% ========================================== Files 186 186 Lines 12421 12428 +7 ========================================== Hits 7362 7362 - Misses 5059 5066 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dani-garcia commented 3 weeks ago

I also thought about entirely renaming the variable from selected_credential to selected_cipher here, knowing that the value doesn't necessarily need to contain a credential.

I didn't do it in this PR so we could get it resolved quickly but it might be worth considering.

bitwarden / sdk