Skip to content

[PM-10548] fix: Biometric auth setting being reset on logout.#2628

Open
morganzellers-bw wants to merge 16 commits into
mainfrom
pm-17704-triage
Open

[PM-10548] fix: Biometric auth setting being reset on logout.#2628
morganzellers-bw wants to merge 16 commits into
mainfrom
pm-17704-triage

Conversation

@morganzellers-bw
Copy link
Copy Markdown
Contributor

@morganzellers-bw morganzellers-bw commented May 10, 2026

🎟️ Tracking

There are a few tickets covering this bug, both on iOS and Android; I've linked below the two I assigned to myself to track the fix, all others are linked to these as related items.

PM-17704

PM-10548

📔 Objective

Users' biometric unlock preference is being cleared on logout. When that user logs back in, there's no preference to dictate whether we should restore the biometric auth key value in the keychain or not.

These changes update the logout flow to leave the biometric unlock preference in place in UserDefaults on logout. Then, this is checked on login and if true, the auth key is added back to the keychain on login.

📸 Screenshots

No biometric setting on simulator for me to do a true test, but I reproduced on production iOS version 2026.4.0 (3082). Hoping a reviewer with a test device can test.

@morganzellers-bw morganzellers-bw added app:password-manager Bitwarden Password Manager app context ai-review Request a Claude code review t:bug Change Type - Bug labels May 10, 2026
@github-actions github-actions Bot added app:authenticator Bitwarden Authenticator app context and removed t:bug Change Type - Bug labels May 10, 2026
@morganzellers-bw morganzellers-bw changed the title potential fix. [PM-17704] fix: Biometric auth setting being reset on logout. May 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 10, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the fix for biometric unlock preference being reset on logout (PM-17704 / PM-10548). The change preserves the per-user biometricAuthenticationEnabled flag through logout, removes the matching clear from DefaultStateService.logoutAccount, and on each subsequent vault unlock checks getBiometricUnlockStatus().isEnabled before silently restoring the keychain entry via the new restoreBiometricUnlockKey (which does not call evaluateBiometricPolicy and so does not prompt). Latest commits added hasBiometricUnlockKey / containsValue(for:) to skip the restore when the key already exists, fixing the double Face ID prompt flagged in earlier review. Test coverage looks solid across BiometricsRepositoryTests, AuthRepositoryTests, KeychainServiceFacadeTests, and the Authenticator KeychainRepositoryTests.

Code Review Details

No new findings on this pass. Substantive concerns are tracked in existing threads:

  • configureBiometricUnlockIfNeeded log noise when biometrics is .lockedOut (existing thread on AuthRepository.swift:1282) — acknowledged by author; potential fix tied to no-op on .lockedOut.
  • BiometricsRepository.restoreBiometricUnlockKey no-op rationale for .lockedOut / .notDetermined (existing thread) — explained by author.
  • Double Face ID prompt from re-writing the biometric key on every unlock — resolved by hasBiometricUnlockKey() guard plus regression tests.
  • Alphabetization of switch cases in BiometricsRepository — addressed.
  • StateServiceTests assertion correctness — resolved.

@morganzellers-bw morganzellers-bw self-assigned this May 10, 2026
@morganzellers-bw morganzellers-bw added the bug Something isn't working label May 10, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 10, 2026

Codecov Report

❌ Patch coverage is 98.75622% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.38%. Comparing base (511cf32) to head (82b644b).

Files with missing lines Patch % Lines
...Shared/Core/Auth/Services/KeychainRepository.swift 0.00% 3 Missing ⚠️
...ervices/Biometrics/BiometricsRepositoryTests.swift 98.82% 1 Missing ⚠️
...Shared/Core/Auth/Repositories/AuthRepository.swift 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2628      +/-   ##
==========================================
- Coverage   87.36%   86.38%   -0.99%     
==========================================
  Files        1918     2141     +223     
  Lines      171225   185975   +14750     
==========================================
+ Hits       149596   160650   +11054     
- Misses      21629    25325    +3696     

☔ 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.

@morganzellers-bw morganzellers-bw marked this pull request as ready for review May 10, 2026 15:50
@morganzellers-bw morganzellers-bw requested review from a team and matt-livefront as code owners May 10, 2026 15:50
@morganzellers-bw morganzellers-bw changed the title [PM-17704] fix: Biometric auth setting being reset on logout. [PM-10548] fix: Biometric auth setting being reset on logout. May 11, 2026
@github-actions github-actions Bot added the t:bug Change Type - Bug label May 11, 2026
@morganzellers-bw morganzellers-bw removed the bug Something isn't working label May 11, 2026
Comment thread BitwardenKit/Core/Auth/Services/Biometrics/BiometricsRepository.swift Outdated
Comment thread BitwardenShared/Core/Platform/Services/StateServiceTests.swift
///
private func configureBiometricUnlockIfNeeded() async {
do {
guard try await biometricsRepository.getBiometricUnlockStatus().isEnabled else { return }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QUESTION: When biometrics is .lockedOut, getBiometricUnlockStatus() throws BiometricsServiceError.biometryLocked, which gets caught and logged as an error here even though it's an expected transient state. This also makes the case .lockedOut: break branch in restoreBiometricUnlockKey unreachable from this caller — was that the intent?

Details and possible alternative

DefaultBiometricsRepository.getBiometricUnlockStatus throws when biometricsService.getBiometricAuthStatus() is .lockedOut (see BiometricsRepository.swift:168-170). With the current implementation, on a password unlock when biometry is temporarily locked out:

  1. getBiometricUnlockStatus().isEnabled throws.
  2. The catch block logs an error via errorReporter.log(error:), treating an expected transient OS state as an error.
  3. restoreBiometricUnlockKey is never invoked, so its .lockedOut: break case is dead code from this caller.

If the goal is to silently no-op on .lockedOut (per the existing thread response on BiometricsRepository.swift:163), reading the stored preference directly side-steps the throw and lets restoreBiometricUnlockKey handle all statuses uniformly:

private func configureBiometricUnlockIfNeeded() async {
    do {
        guard try await stateService.getBiometricAuthenticationEnabled() else { return }
        let authKey = try await clientService.crypto().getUserEncryptionKey()
        try await biometricsRepository.restoreBiometricUnlockKey(authKey: authKey)
    } catch {
        errorReporter.log(error: error)
    }
}

The next unlock attempt will retry once biometry is no longer locked out, so functionality is unaffected — this is purely about log noise and reachability.

Copy link
Copy Markdown
Contributor Author

@morganzellers-bw morganzellers-bw May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to no-op on .lockedOut, this will fix:

private func configureBiometricUnlockIfNeeded() async {
    do {
        ...
    } catch BiometricsServiceError.biometryLocked {
      // Lockout is transient; do nothing and let the user retry later.
   } catch {
        errorReporter.log(error: error)
    }
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth going the no-op route to avoid logging an error here.

Comment thread BitwardenShared/Core/Auth/Repositories/AuthRepository.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants