Skip to content
Draft
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 @@ -35,6 +35,29 @@ public enum SessionTimeoutValue: Codable, RawRepresentable, Equatable, Hashable,
/// A custom timeout value.
case custom(Int)

/// Whether this timeout value allows the user session key to be shared across process boundaries.
///
/// Returns `false` for values that treat a process boundary as a lock trigger (`.immediately` and
/// `.onAppRestart`) and for `.never`, which uses a dedicated `neverLock` keychain item instead.
/// Returns `true` for all timed timeout values.
///
public var allowsUserSessionKeySharing: Bool {
switch self {
case .immediately,
.never,
.onAppRestart:
false
case .custom,
.fifteenMinutes,
.fiveMinutes,
.fourHours,
.oneHour,
.oneMinute,
.thirtyMinutes:
true
}
}

/// The session timeout value in seconds.
public var seconds: Int {
rawValue * 60
Expand Down
43 changes: 42 additions & 1 deletion BitwardenShared/Core/Auth/Repositories/AuthRepository.swift
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,12 @@ protocol AuthRepository: AnyObject {
///
func unlockVaultWithPIN(pin: String) async throws

/// Attempts to unlock the user's vault with the stored user session key.
///
/// - Returns: `true` if the session key was found and the vault was unlocked; `false` if no session key is stored.
///
func unlockVaultWithSessionKey() async throws -> Bool

/// Updates the user's master password.
///
/// - Parameters:
Expand Down Expand Up @@ -611,6 +617,7 @@ extension DefaultAuthRepository: AuthRepository {
&& !account.isLoggedOut // Isn't already logged out (soft-logout)

if (shouldTimeout && !account.isLoggedOut) || shouldLogoutDueToNoUnlockMethod {
try? await keychainService.deleteUserAuthKey(for: .userSessionKey(userId: userId))
if userId == activeUserId {
await handleActiveUser?(activeUserId)
} else {
Expand Down Expand Up @@ -793,7 +800,9 @@ extension DefaultAuthRepository: AuthRepository {
await vaultTimeoutService.lockVault(userId: userId)
if isManuallyLocking {
do {
try await stateService.setManuallyLockedAccount(true, userId: userId)
let resolvedId = try await stateService.getAccountIdOrActiveId(userId: userId)
try? await keychainService.deleteUserAuthKey(for: .userSessionKey(userId: resolvedId))
try await stateService.setManuallyLockedAccount(true, userId: resolvedId)
} catch {
errorReporter.log(error: error)
}
Expand Down Expand Up @@ -965,6 +974,11 @@ extension DefaultAuthRepository: AuthRepository {
)
}

// Delete the session key when switching to a timeout that excludes it.
if !newValue.allowsUserSessionKeySharing {
try? await keychainService.deleteUserAuthKey(for: .userSessionKey(userId: id))
}

// Then configure the vault timeout service with the correct value.
try await vaultTimeoutService.setVaultTimeout(
value: newValue,
Expand Down Expand Up @@ -1077,6 +1091,22 @@ extension DefaultAuthRepository: AuthRepository {
}
}

func unlockVaultWithSessionKey() async throws -> Bool {
let id = try await stateService.getActiveAccountId()
do {
let sessionKey = try await keychainService.getUserAuthKeyValue(for: .userSessionKey(userId: id))
do {
try await unlockVault(method: .decryptedKey(decryptedUserKey: sessionKey), hadUserInteraction: false)
} catch {
try? await keychainService.deleteUserAuthKey(for: .userSessionKey(userId: id))
throw error
}
return true
} catch KeychainServiceError.osStatusError(errSecItemNotFound), KeychainServiceError.keyNotFound {
return false
}
}
Comment on lines +1094 to +1108
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.

⚠️ IMPORTANT: New unlockVaultWithSessionKey() has no unit tests covering its three branches.

Details and fix

AuthRepositoryTests.swift contains zero references to unlockVaultWithSessionKey. Per Docs/Testing.md, "every type containing logic must be tested." Existing parallel functionality (unlockVaultWithNeverlockKey) is fully covered. Please add tests for:

  1. Returns true and calls unlockVault(method: .decryptedKey(...)) when a key exists in the keychain.
  2. Returns false when keychain throws KeychainServiceError.osStatusError(errSecItemNotFound) or KeychainServiceError.keyNotFound β€” no exception escapes.
  3. Deletes the keychain item and rethrows when unlockVault(...) itself throws (e.g., simulate an initializeUserCrypto failure).
  4. The call passes hadUserInteraction: false (important because this controls trust-device behavior).

This branch is exercised by extensions, so silent regressions here can leave users unable to autofill until they manually unlock the main app.


func validatePassword(_ password: String) async throws -> Bool {
if let passwordHash = try await stateService.getMasterPasswordHash() {
return try await clientService.auth().validatePassword(password: password, passwordHash: passwordHash)
Expand Down Expand Up @@ -1198,6 +1228,17 @@ extension DefaultAuthRepository: AuthRepository {
} catch {
errorReporter.log(error: error)
}
do {
let timeoutValue = try await vaultTimeoutService.sessionTimeoutValue(userId: account.profile.userId)
if timeoutValue.allowsUserSessionKeySharing {
try await keychainService.setUserAuthKey(
for: .userSessionKey(userId: account.profile.userId),
value: clientService.crypto().getUserEncryptionKey(),
)
}
} catch {
errorReporter.log(error: error)
}
}

/// Updates the user's KDF settings to the minimums.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ class MockAuthRepository: AuthRepository { // swiftlint:disable:this type_body_l
var convertNewUserToKeyConnectorKeyResult: Result<Void, Error> = .success(())
var unlockVaultWithNeverlockKeyCalled = false
var unlockVaultWithNeverlockResult: Result<Void, Error> = .success(())
var unlockVaultWithSessionKeyCalled = false
var unlockVaultWithSessionKeyResult: Result<Bool, Error> = .success(false)
var verifyOtpOpt: String?
var verifyOtpResult: Result<Void, Error> = .success(())

Expand Down Expand Up @@ -414,6 +416,11 @@ class MockAuthRepository: AuthRepository { // swiftlint:disable:this type_body_l
return try unlockVaultWithNeverlockResult.get()
}

func unlockVaultWithSessionKey() async throws -> Bool {
unlockVaultWithSessionKeyCalled = true
return try unlockVaultWithSessionKeyResult.get()
}

/// Attempts to convert a possible user id into a known account id.
///
/// - Parameter userId: If nil, the active account id is returned. Otherwise, validate the id.
Expand Down
8 changes: 8 additions & 0 deletions BitwardenShared/Core/Auth/Services/KeychainRepository.swift
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ enum BitwardenKeychainItem: Equatable, KeychainItem {
/// The keychain item for the number of unsuccessful unlock attempts.
case unsuccessfulUnlockAttempts(userId: String)

/// The keychain item for the user session key (cross-process unlock).
case userSessionKey(userId: String)

/// The keychain item for a user's vault timeout.
case vaultTimeout(userId: String)

Expand All @@ -78,6 +81,7 @@ enum BitwardenKeychainItem: Equatable, KeychainItem {
.refreshToken,
.serverCommunicationConfig,
.unsuccessfulUnlockAttempts,
.userSessionKey,
.vaultTimeout:
nil
case .biometrics,
Expand All @@ -99,6 +103,7 @@ enum BitwardenKeychainItem: Equatable, KeychainItem {
.neverLock,
.pendingAdminLoginRequest,
.unsuccessfulUnlockAttempts,
.userSessionKey,
.vaultTimeout:
kSecAttrAccessibleWhenUnlockedThisDeviceOnly
case .accessToken,
Expand Down Expand Up @@ -147,6 +152,8 @@ enum BitwardenKeychainItem: Equatable, KeychainItem {
"serverCommunicationConfig_\(hostname)"
case let .unsuccessfulUnlockAttempts(userId):
"unsuccessfulUnlockAttempts_\(userId)"
case let .userSessionKey(userId: id):
"userSessionKey_" + id
case let .vaultTimeout(userId):
"vaultTimeout_\(userId)"
}
Expand Down Expand Up @@ -369,6 +376,7 @@ extension DefaultKeychainRepository {
// is approved, the next login for the user will succeed with the pending request.
.refreshToken(userId: userId),
.unsuccessfulUnlockAttempts(userId: userId),
.userSessionKey(userId: userId),
// Exclude `vaultTimeout` since it should be maintained for users who log out and back in regularly.
]
for keychainItem in keychainItems {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ protocol NotificationCenterService: AnyObject {
///
func isInForegroundPublisher() -> AsyncPublisher<AnyPublisher<Bool, Never>>

/// A publisher for when the app is about to resign active status.
/// This fires when the user opens the app switcher (before the app enters the background),
/// covering the foreground-to-kill path where `didEnterBackground` never fires.
///
func willResignActivePublisher() -> AsyncPublisher<AnyPublisher<Void, Never>>

/// A publisher for when the app enters the foreground.
///
func willEnterForegroundPublisher() -> AsyncPublisher<AnyPublisher<Void, Never>>
Expand Down Expand Up @@ -73,6 +79,14 @@ class DefaultNotificationCenterService: NotificationCenterService {
isInForegroundSubject.eraseToAnyPublisher().values
}

func willResignActivePublisher() -> AsyncPublisher<AnyPublisher<Void, Never>> {
notificationCenter
.publisher(for: UIApplication.willResignActiveNotification)
.map { _ in }
.eraseToAnyPublisher()
.values
}

func willEnterForegroundPublisher() -> AsyncPublisher<AnyPublisher<Void, Never>> {
notificationCenter
.publisher(for: UIApplication.willEnterForegroundNotification)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Foundation
class MockNotificationCenterService: NotificationCenterService {
var didEnterBackgroundSubject = CurrentValueSubject<Void, Never>(())
var isInForegroundSubject = CurrentValueSubject<Bool, Never>(true)
var willResignActiveSubject = PassthroughSubject<Void, Never>()
var willEnterForegroundSubject = CurrentValueSubject<Void, Never>(())

func didEnterBackgroundPublisher() -> AsyncPublisher<AnyPublisher<Void, Never>> {
Expand All @@ -16,6 +17,10 @@ class MockNotificationCenterService: NotificationCenterService {
isInForegroundSubject.eraseToAnyPublisher().values
}

func willResignActivePublisher() -> AsyncPublisher<AnyPublisher<Void, Never>> {
willResignActiveSubject.eraseToAnyPublisher().values
}

func willEnterForegroundPublisher() -> AsyncPublisher<AnyPublisher<Void, Never>> {
willEnterForegroundSubject.eraseToAnyPublisher().values
}
Expand Down
3 changes: 2 additions & 1 deletion BitwardenShared/UI/Auth/AuthCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ final class AuthCoordinator: NSObject, // swiftlint:disable:this type_body_lengt
case let .checkEmail(email):
showCheckEmail(email)
case .complete,
.completeWithNeverUnlockKey:
.completeWithNeverUnlockKey,
.completeWithUserSessionKey:
completeAuth()
case let .completeRegistration(emailVerificationToken, userEmail):
showCompleteRegistration(
Expand Down
3 changes: 3 additions & 0 deletions BitwardenShared/UI/Auth/AuthRoute.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ public enum AuthRoute: Equatable {
/// - rehydratableTarget: The target that we want to restore and rehydrate after the vault is unlocked..
case completeWithRehydration(_ rehydratableTarget: RehydratableTarget)

/// Dismisses the auth flow because the vault was unlocked with the user session key.
case completeWithUserSessionKey

/// A route that dismisses a presented sheet.
case dismiss

Expand Down
15 changes: 15 additions & 0 deletions BitwardenShared/UI/Auth/Extensions/AuthRouter+Redirects.swift
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,21 @@ extension AuthRouter {
return .completeWithNeverUnlockKey
case (_, false, _):
return .complete
case (_, true, false):
guard try await services.stateService.isAuthenticated(userId: userId) else {
return .landingSoftLoggedOut(email: activeAccount.profile.email)
}
// If a session key is present, auto-unlock without user interaction.
// checkSessionTimeouts() has already verified the timeout hasn't elapsed.
if try await services.authRepository.unlockVaultWithSessionKey() {
return .completeWithUserSessionKey
}
return .vaultUnlock(
activeAccount,
animated: animated,
attemptAutomaticBiometricUnlock: attemptAutomaticBiometricUnlock,
didSwitchAccountAutomatically: didSwitchAccountAutomatically,
)
Comment on lines +383 to +397
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.

⚠️ IMPORTANT: New (_, true, false) redirect branch (auto-unlock via session key) has no test coverage in AuthRouterTests.swift.

Details and fix

This branch decides whether to bypass the unlock screen entirely β€” a security-relevant behavior. Suggested tests:

  1. When unlockVaultWithSessionKey() returns true, the redirect resolves to .completeWithUserSessionKey.
  2. When unlockVaultWithSessionKey() returns false, the redirect resolves to .vaultUnlock(...) with the correct animated, attemptAutomaticBiometricUnlock, and didSwitchAccountAutomatically propagated.
  3. When unlockVaultWithSessionKey() throws, the outer catch is invoked, the error is logged via errorReporter, and the route is .vaultUnlock(...).
  4. When isAuthenticated returns false, the route is .landingSoftLoggedOut(email:) and unlockVaultWithSessionKey is not called (avoids decrypting a soft-logged-out account's data).

Also worth adding a complementary test in AuthCoordinatorTests.swift for .completeWithUserSessionKey mirroring the existing test_navigate_completeWithNeverUnlockKey.

default:
guard try await services.stateService.isAuthenticated(userId: userId) else {
return .landingSoftLoggedOut(email: activeAccount.profile.email)
Expand Down
20 changes: 20 additions & 0 deletions BitwardenShared/UI/Platform/Application/AppProcessor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ public class AppProcessor {

listenForWillEnterForeground(debugWillEnterForeground: debugWillEnterForeground)
listenForDidEnterBackground(debugDidEnterBackground: debugDidEnterBackground)
listenForWillResignActive()
listenForPendingAppIntentActions()
listenForAcquireCookies()
}
Expand Down Expand Up @@ -538,6 +539,25 @@ extension AppProcessor {
}
}

/// Subscribes to the will-resign-active notification and records the last active time for
/// the current user. This covers the foreground-to-app-switcher-kill path where the app is
/// terminated before `didEnterBackground` fires.
///
private func listenForWillResignActive() {
Task {
for await _ in services.notificationCenterService.willResignActivePublisher() {
do {
let userId = try await self.services.stateService.getActiveAccountId()
try await services.vaultTimeoutService.setLastActiveTime(userId: userId)
} catch StateServiceError.noActiveAccount {
// No-op: nothing to do if there's no active account.
} catch {
services.errorReporter.log(error: error)
}
}
}
}
Comment on lines +546 to +559
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.

⚠️ IMPORTANT: listenForWillResignActive() has no test coverage in AppProcessorTests.swift.

Details and fix

This subscription gates whether the "foreground β†’ app-switcher-kill" auto-unlock path correctly records the last-active timestamp. Without a test, a regression here silently breaks the timeout window check on cold restart from the switcher. Suggested tests (mirroring the existing didEnterBackground tests):

  1. Sending a value through mockNotificationCenterService.willResignActiveSubject calls vaultTimeoutService.setLastActiveTime(userId:) with the active account id.
  2. When getActiveAccountId() throws StateServiceError.noActiveAccount, no error is logged and setLastActiveTime is not called.
  3. When setLastActiveTime(userId:) throws an unrelated error, it is logged via errorReporter.


/// Subscribes to the pending `AppIntent` actions publisher and executes any pending actions
/// as they arrive, updating navigation and UI accordingly.
///
Expand Down
Loading