-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1174 +/- ##
=======================================
Coverage 89.39% 89.40%
=======================================
Files 688 688
Lines 43798 43826 +28
=======================================
+ Hits 39152 39181 +29
+ Misses 4646 4645 -1 ☔ View full report in Codecov by Sentry. |
No New Or Fixed Issues Found |
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) | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.
ios/BitwardenShared/UI/Platform/Application/AppProcessor.swift
Lines 347 to 375 in 4c8d061
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) | |
} | |
} |
🎟️ Tracking
PM-11494
📔 Objective
This update addresses the issue where session timeouts are not recorded when a user switches accounts.
lastActiveTime
is now recorded whenever a user switches to a different account.📸 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