[PM-27225] Fix nothing showing when biometrics unavailable #17209
Conversation
quexten
force-pushed
the
km/fix-biometrics-tde-unavailable
branch
from
e1bfaf6 to
39666ad
Compare
|
Great job! No new security vulnerabilities introduced in this pull request |
quexten
changed the title
…den/clients into km/fix-biometrics-tde-unavailable
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17209 +/- ##
==========================================
+ Coverage 41.11% 41.12% +0.01%
==========================================
Files 3543 3543
Lines 101843 101854 +11
Branches 15265 15270 +5
==========================================
+ Hits 41872 41887 +15
+ Misses 58208 58201 -7
- Partials 1763 1766 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @quexten's task —— View job Code Review CompleteI've reviewed the latest changes to this PR. The implementation looks solid overall, with comprehensive test coverage added. However, there is one unresolved issue from the previous Claude review that still needs to be addressed. FindingsFinding 1: Inconsistent data source for biometric status check creates potential race condition Location: Issue: The code calls // Line 283-285: unlockOptions already contains biometric status
this.unlockOptions = await firstValueFrom(
this.lockComponentService.getAvailableUnlockOptions$(activeAccount.id),
);
// Line 287-290: Makes a separate call for the same status
const canUseBiometrics = [
BiometricsStatus.Available,
...BIOMETRIC_UNLOCK_TEMPORARY_UNAVAILABLE_STATUSES,
].includes(await this.biometricService.getBiometricsStatusForUser(activeAccount.id));Suggested fix: Use the status already available in const canUseBiometrics = [
BiometricsStatus.Available,
...BIOMETRIC_UNLOCK_TEMPORARY_UNAVAILABLE_STATUSES,
].includes(this.unlockOptions?.biometrics.biometricsStatus ?? BiometricsStatus.PlatformUnsupported);This was previously flagged in review comments and appears to still be unresolved. Positive Observations
SummaryThe PR successfully addresses the core issue of showing a proper UI when biometrics is temporarily unavailable. The test coverage is excellent. Please address the data source inconsistency issue (Finding 1) to ensure the biometric status check is reliable and consistent with the displayed unlock options. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const canUseBiometrics = [ | ||
| BiometricsStatus.Available, | ||
| ...BIOMETRIC_UNLOCK_TEMPORARY_UNAVAILABLE_STATUSES, | ||
| ].includes(await this.biometricService.getBiometricsStatusForUser(activeAccount.id)); |
There was a problem hiding this comment.
getBiometricsStatusForUser() after already fetching unlockOptions, which could result in inconsistent state.
As mzieniukbw suggested, use the status from unlockOptions directly:
const canUseBiometrics = [
BiometricsStatus.Available,
...BIOMETRIC_UNLOCK_TEMPORARY_UNAVAILABLE_STATUSES,
].includes(this.unlockOptions.biometrics.biometricsStatus);This ensures the biometric status check uses the same data already fetched in unlockOptions.
| const canUseBiometrics = [ | ||
| BiometricsStatus.Available, | ||
| ...BIOMETRIC_UNLOCK_TEMPORARY_UNAVAILABLE_STATUSES, | ||
| ].includes(await this.biometricService.getBiometricsStatusForUser(activeAccount.id)); | ||
| if ( | ||
| !this.unlockOptions.masterPassword.enabled && | ||
| !this.unlockOptions.pin.enabled && | ||
| !canUseBiometrics | ||
| ) { | ||
| // User has no available unlock options, force logout. This happens for TDE users without a masterpassword, that don't have a persistent unlock method set. | ||
| this.logService.warning("[LockComponent] User cannot unlock again. Logging out!"); | ||
| await this.logoutService.logout(activeAccount.id); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🤔 Code duplication: This logic overlaps significantly with VaultTimeoutSettingsService.getAvailableVaultTimeoutActions() (lines 280-289 in vault-timeout-settings.service.ts), which has similar logic:
const canLock =
(await this.userHasMasterPassword(userId)) ||
(await this.pinStateService.isPinSet(userId as UserId)) ||
(await this.isBiometricLockSet(userId));Consider extracting this to a shared utility or reusing the existing VaultTimeoutSettingsService.canLock() method to avoid maintenance burden and potential divergence between the two implementations.
…den/clients into km/fix-biometrics-tde-unavailable
| const canUseBiometrics = [ | ||
| BiometricsStatus.Available, | ||
| ...BIOMETRIC_UNLOCK_TEMPORARY_UNAVAILABLE_STATUSES, | ||
| ].includes(await this.biometricService.getBiometricsStatusForUser(activeAccount.id)); |
There was a problem hiding this comment.
getBiometricsStatusForUser() directly after getAvailableUnlockOptions$() already fetched biometric status.
The biometric status is already available in this.unlockOptions.biometrics.biometricsStatus (line 283-285). Using a separate call could result in inconsistent state if the biometric status changes between the two calls.
Suggested change:
const canUseBiometrics = [
BiometricsStatus.Available,
...BIOMETRIC_UNLOCK_TEMPORARY_UNAVAILABLE_STATUSES,
].includes(this.unlockOptions?.biometrics.biometricsStatus ?? BiometricsStatus.PlatformUnsupported);This was flagged in a previous review and appears unresolved.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27225
📔 Objective
When a user without a master password (TDE/key connector) logs in and has an unreliable unlock method (biometrics), if biometrics is temporarily unavailable, currently a broken component is shown. This PR detects that case and shows the biometric component, with the button being unavailable, so the user realizes they have to re-connect their biometrics device (unfold their laptop).
Further, in the case when a user cannot unlock - no password and no temporarily unavailable unlock method - we log out the user automatically.
Also adds the unavailability reason as a tooltip as per product's suggestion.
📸 Screenshots
⏰ Reminders before review
🦮 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