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-11494: fixed session timeout not being respected when switch account #1174

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
24 changes: 24 additions & 0 deletions BitwardenShared/Core/Auth/Repositories/AuthRepository.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ protocol AuthRepository: AnyObject {
///
func canVerifyMasterPassword(userId: String?) async throws -> Bool

/// Checks the session timeout for all non active, unlocked accounts, and locks or logs out as needed.
///
func checkSessionTimeout() async

/// Clears the pins stored on device and in memory.
///
func clearPins() async throws
Expand Down Expand Up @@ -515,6 +519,26 @@ extension DefaultAuthRepository: AuthRepository {
try await stateService.getUserHasMasterPassword(userId: userId)
}

func checkSessionTimeout() async {
let accounts = await (try? getAccounts()) ?? []
guard !accounts.isEmpty else { return }
let activeAccount = try? await getActiveAccount()
for account in accounts where account.userId != activeAccount?.userId && account.isUnlocked {
let shouldTimeout = try? await vaultTimeoutService.hasPassedSessionTimeout(
userId: account.userId
)
if shouldTimeout == true {
let timeoutAction = try? await sessionTimeoutAction(userId: account.userId)
switch timeoutAction {
case .lock:
await vaultTimeoutService.lockVault(userId: account.userId)
case .logout, .none:
try? await logout(userId: account.userId, userInitiated: false)
}
}
}
}
Comment on lines +522 to +540
Copy link
Collaborator

Choose a reason for hiding this comment

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

๐ŸŽจ This is mostly similar to the logic that exists in the AppProcessor. Do you think we could combine/share this somehow? I think the main difference is that this works for inactive accounts while the AppProcessor has different logic for the active account? It might also be helpful to include in the function name that this only applies to inactive accounts (e.g. checkSessionTimeoutForInactiveAccounts())?

We should avoid using try? if possible and logging any unexpected errors, similar to what the AppProcessor is doing.

private func checkAccountsForTimeout() async {
do {
let accounts = try await services.stateService.getAccounts()
let activeUserId = try await services.stateService.getActiveAccountId()
for account in accounts {
let userId = account.profile.userId
let shouldTimeout = try await services.vaultTimeoutService.hasPassedSessionTimeout(userId: userId)
if shouldTimeout {
if userId == activeUserId {
// Allow the AuthCoordinator to handle the timeout for the active user
// so any necessary routing can occur.
await coordinator?.handleEvent(.didTimeout(userId: activeUserId))
} else {
let timeoutAction = try? await services.authRepository.sessionTimeoutAction(userId: userId)
switch timeoutAction {
case .lock:
await services.vaultTimeoutService.lockVault(userId: userId)
case .logout, .none:
try await services.authRepository.logout(userId: userId, userInitiated: false)
}
}
}
}
} catch StateServiceError.noAccounts, StateServiceError.noActiveAccount {
// No-op: nothing to do if there's no accounts or an active account.
} catch {
services.errorReporter.log(error: error)
}
}


func createNewSsoUser(orgIdentifier: String, rememberDevice: Bool) async throws {
let account = try await stateService.getActiveAccount()
let enrollStatus = try await organizationAPIService.getOrganizationAutoEnrollStatus(identifier: orgIdentifier)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,37 @@ class AuthRepositoryTests: BitwardenTestCase { // swiftlint:disable:this type_bo
XCTAssertNil(biometricsRepository.capturedUserAuthKey)
}

/// `checkSessionTimeout()` locks an account when the session timeout action is lock.
func test_checkSessionTimeout_lockAccount() async {
stateService.accounts = [anneAccount, beeAccount]
stateService.activeAccount = beeAccount
stateService.timeoutAction = [anneAccount.profile.userId: .lock]
vaultTimeoutService.shouldSessionTimeout[anneAccount.profile.userId] = true
await subject.checkSessionTimeout()
XCTAssertTrue(vaultTimeoutService.isLocked(userId: anneAccount.profile.userId))
}

/// `checkSessionTimeout()` logs out an account when the session timeout action is logout.
func test_checkSessionTimeout_logoutAccount() async {
stateService.accounts = [anneAccount, beeAccount]
stateService.activeAccount = beeAccount
stateService.timeoutAction = [anneAccount.profile.userId: .logout]
vaultTimeoutService.shouldSessionTimeout[anneAccount.profile.userId] = true
await subject.checkSessionTimeout()
XCTAssertTrue(vaultTimeoutService.removedIds.contains(anneAccount.profile.userId))
XCTAssertTrue(stateService.accountsLoggedOut.contains(anneAccount.profile.userId))
}

/// `checkSessionTimeout()` takes no action to an active account when the session timeout.
func test_checkSessionTimeout_activeAccount() async {
stateService.accounts = [anneAccount, beeAccount]
stateService.activeAccount = beeAccount
stateService.timeoutAction = [beeAccount.profile.userId: .lock]
vaultTimeoutService.shouldSessionTimeout[beeAccount.profile.userId] = true
await subject.checkSessionTimeout()
XCTAssertFalse(vaultTimeoutService.isLocked(userId: anneAccount.profile.userId))
}

/// `getProfilesState()` throws an error when the accounts are nil.
func test_getProfilesState_empty() async {
let state = await subject.getProfilesState(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class MockAuthRepository: AuthRepository {
var canBeLockedResult: [String: Bool] = [:]
var canVerifyMasterPasswordResult: Result<Bool, Error> = .success(true)
var canVerifyMasterPasswordForUserResult: Result<[String: Bool], Error> = .success([:])
var checkSessionTimeoutCalled = false
var clearPinsCalled = false
var createNewSsoUserRememberDevice: Bool = false
var createNewSsoUserOrgIdentifier: String = ""
Expand Down Expand Up @@ -127,6 +128,10 @@ class MockAuthRepository: AuthRepository {
}
}

func checkSessionTimeout() async {
checkSessionTimeoutCalled = true
}

func clearPins() async throws {
clearPinsCalled = true
}
Expand Down
4 changes: 4 additions & 0 deletions BitwardenShared/UI/Auth/AuthRouterTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1247,13 +1247,15 @@ final class AuthRouterTests: BitwardenTestCase { // swiftlint:disable:this type_
let active = Account.fixture()
authRepository.activeAccount = active
authRepository.isLockedResult = .success(false)
vaultTimeoutService.lastActiveTime = [:]
let route = await subject.handleAndRoute(
.action(
.switchAccount(isAutomatic: true, userId: active.profile.userId)
)
)
XCTAssertEqual(route, .complete)
XCTAssertEqual(authRepository.setActiveAccountId, active.profile.userId)
XCTAssertNil(vaultTimeoutService.lastActiveTime[active.profile.userId])
}

/// `handleAndRoute(_ :)` redirects `.switchAccount()` to `.vaultUnlock` when switching to a
Expand All @@ -1265,6 +1267,7 @@ final class AuthRouterTests: BitwardenTestCase { // swiftlint:disable:this type_
authRepository.altAccounts = [inactive]
authRepository.isLockedResult = .success(true)
stateService.isAuthenticated["2"] = true
vaultTimeoutService.lastActiveTime = [:]
let route = await subject.handleAndRoute(
.action(
.switchAccount(isAutomatic: true, userId: inactive.profile.userId)
Expand All @@ -1280,6 +1283,7 @@ final class AuthRouterTests: BitwardenTestCase { // swiftlint:disable:this type_
)
)
XCTAssertEqual(authRepository.setActiveAccountId, inactive.profile.userId)
XCTAssertNotNil(vaultTimeoutService.lastActiveTime[active.profile.userId])
}

/// `handleAndRoute(_ :)` redirects `.switchAccount()` to `.landingSoftLoggedOut` when that
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,14 @@ extension AuthRouter {
///
func switchAccountRedirect(isAutomatic: Bool, userId: String) async -> AuthRoute {
do {
// Set the last active time for the current account before switching.
if let currentActiveAccount = try? await services.authRepository.getAccount(),
currentActiveAccount.profile.userId != userId {
try await services.vaultTimeoutService.setLastActiveTime(
userId: currentActiveAccount.profile.userId
)
}

let activeAccount = try await services.authRepository.setActiveAccount(userId: userId)
// Setup the unlock route for the active account.
let event = AuthEvent.accountBecameActive(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ extension ProfileSwitcherHandler {
showAddAccount()
case let .requestedProfileSwitcher(isVisible):
if isVisible {
await profileServices.authRepository.checkSessionTimeout()
await refreshProfileState()
}
profileSwitcherState.isVisible = isVisible
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ
// Ensure that the profile switcher state is updated
waitFor(subject.state.profileSwitcherState == authRepository.profileSwitcherState)
XCTAssertTrue(subject.state.profileSwitcherState.isVisible)
XCTAssertTrue(authRepository.checkSessionTimeoutCalled)
}

/// `perform(.profileSwitcher(.rowAppeared))` should not update the state for add Account
Expand Down
Loading