Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PM-11883: Improve handling of biometric unlock errors #917

Merged
merged 1 commit into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,9 @@ class DefaultBiometricsRepository: BiometricsRepository {
kLAErrorUserFallback:
throw BiometricsServiceError.biometryFailed
default:
throw BiometricsServiceError.getAuthKeyFailed
throw error
}
}
} catch {
throw BiometricsServiceError.getAuthKeyFailed
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ final class BiometricsRepositoryTests: BitwardenTestCase { // swiftlint:disable:
}
}

/// `getUserAuthKey` retrieves the key from keychain and updates integrity state.
/// `getUserAuthKey` throws an error if one occurs.
func test_getUserAuthKey_unknownError() async throws {
let active = Account.fixture()
stateService.activeAccount = active
Expand All @@ -360,7 +360,22 @@ final class BiometricsRepositoryTests: BitwardenTestCase { // swiftlint:disable:
active.profile.userId: true,
]
keychainService.getResult = .failure(BitwardenTestError.example)
await assertAsyncThrows(error: BiometricsServiceError.getAuthKeyFailed) {
await assertAsyncThrows(error: BitwardenTestError.example) {
_ = try await subject.getUserAuthKey()
}
}

/// `getUserAuthKey` throws an error if an unknown OS error occurs.
func test_getUserAuthKey_unknownOSError() async throws {
let active = Account.fixture()
stateService.activeAccount = active
let integrity = Data("Face/Off".utf8)
biometricsService.biometricIntegrityState = integrity
stateService.biometricsEnabled = [
active.profile.userId: true,
]
keychainService.getResult = .failure(KeychainServiceError.osStatusError(errSecParam))
await assertAsyncThrows(error: KeychainServiceError.osStatusError(errSecParam)) {
_ = try await subject.getUserAuthKey()
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,17 @@ enum BitwardenError {
/// - Parameters:
/// - type: The type of error. This is used to group the errors in the Crashlytics dashboard.
/// - message: A message describing the error that occurred.
/// - error: An optional underlying error that caused the error.
///
static func generalError(type: String, message: String) -> NSError {
NSError(
static func generalError(type: String, message: String, error: Error? = nil) -> NSError {
var userInfo: [String: Any] = ["ErrorMessage": message]
if let error {
userInfo[NSUnderlyingErrorKey] = error
}
return NSError(
domain: "General Error: \(type)",
code: Code.generalError.rawValue,
userInfo: [
"ErrorMessage": message,
]
userInfo: userInfo
)
}

Expand Down
35 changes: 19 additions & 16 deletions BitwardenShared/UI/Auth/VaultUnlock/VaultUnlockProcessor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -243,29 +243,32 @@ class VaultUnlockProcessor: StateProcessor<
await coordinator.handleEvent(.didCompleteAuth)
state.unsuccessfulUnlockAttemptsCount = 0
await services.stateService.setUnsuccessfulUnlockAttempts(0)
} catch let error as BiometricsServiceError {
Logger.processor.error("BiometricsServiceError unlocking vault with biometrics: \(error)")
} catch BiometricsServiceError.biometryCancelled {
Logger.processor.error("Biometric unlock cancelled.")
// Do nothing if the user cancels.
} catch BiometricsServiceError.biometryLocked {
Logger.processor.error("Biometric unlock failed duo to biometrics lockout.")
// If the user has locked biometry, logout immediately.
if case .biometryLocked = error {
await logoutUser(userInitiated: true)
return
}
if case .biometryCancelled = error {
// Do nothing if the user cancels.
return
}
// There is no biometric auth key stored, set user preference to false.
if case .getAuthKeyFailed = error {
try? await services.authRepository.allowBioMetricUnlock(false)
}
await logoutUser(userInitiated: true)
} catch BiometricsServiceError.getAuthKeyFailed {
services.errorReporter.log(error: BitwardenError.generalError(
type: "VaultUnlock: Get Biometrics Auth Key Failed",
message: "Biometrics auth is enabled but key was unable to be found. Disabling biometric unlock."
))
try? await services.authRepository.allowBioMetricUnlock(false)
await loadData()
} catch let error as StateServiceError {
// If there is no active account, don't add to the unsuccessful count.
Logger.processor.error("StateServiceError unlocking vault with biometrics: \(error)")
services.errorReporter.log(error: error)
// Just send the user back to landing.
coordinator.navigate(to: .landing)
} catch {
Logger.processor.error("Error unlocking vault with biometrics: \(error)")
services.errorReporter.log(error: BitwardenError.generalError(
type: "VaultUnlock: Biometrics Unlock Error",
message: "A biometrics error occurred.",
error: error
))

state.unsuccessfulUnlockAttemptsCount += 1
await services.stateService
.setUnsuccessfulUnlockAttempts(state.unsuccessfulUnlockAttemptsCount)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,21 @@ class VaultUnlockProcessorTests: BitwardenTestCase { // swiftlint:disable:this t
XCTAssertEqual(attemptsInUserDefaults, 0)
}

/// `perform(_:)` with `.unlockVaultWithBiometrics` logs the user out if biometrics is locked
/// due to too many failed attempts.
@MainActor
func test_perform_unlockWithBiometrics_biometryLocked() async throws {
stateService.activeAccount = .fixture()
biometricsRepository.biometricUnlockStatus = .success(
.available(.touchID, enabled: true, hasValidIntegrity: true)
)
authRepository.unlockVaultWithBiometricsResult = .failure(BiometricsServiceError.biometryLocked)

await subject.perform(.unlockVaultWithBiometrics)
XCTAssertNil(coordinator.routes.last)
XCTAssertEqual(coordinator.events, [.action(.logout(userId: nil, userInitiated: true))])
}

/// `perform(_:)` with `.unlockVaultWithBiometrics` shows the KDF warning in an extension if the
/// KDF memory is potentially too high.
@MainActor
Expand Down Expand Up @@ -521,6 +536,7 @@ class VaultUnlockProcessorTests: BitwardenTestCase { // swiftlint:disable:this t
await subject.perform(.unlockVaultWithBiometrics)
let route = try XCTUnwrap(coordinator.routes.last)
XCTAssertEqual(route, .landing)
XCTAssertEqual(errorReporter.errors as? [StateServiceError], [.noActiveAccount])
}

/// `perform(_:)` with `.unlockWithBiometrics` requires a set user preference.
Expand Down Expand Up @@ -573,6 +589,12 @@ class VaultUnlockProcessorTests: BitwardenTestCase { // swiftlint:disable:this t
await subject.perform(.unlockVaultWithBiometrics)
XCTAssertNil(coordinator.routes.last)
XCTAssertEqual(1, subject.state.unsuccessfulUnlockAttemptsCount)

XCTAssertEqual(errorReporter.errors.count, 1)
XCTAssertEqual(
(errorReporter.errors[0] as NSError).domain,
"General Error: VaultUnlock: Biometrics Unlock Error"
)
}

/// `perform(_:)` with `.unlockWithBiometrics` requires successful biometrics.
Expand All @@ -594,6 +616,12 @@ class VaultUnlockProcessorTests: BitwardenTestCase { // swiftlint:disable:this t
.logout(userId: nil, userInitiated: true)
)
)

XCTAssertEqual(errorReporter.errors.count, 1)
XCTAssertEqual(
(errorReporter.errors[0] as NSError).domain,
"General Error: VaultUnlock: Biometrics Unlock Error"
)
}

/// `perform(_:)` with `.unlockWithBiometrics` requires successful biometrics.
Expand Down
Loading