-
Notifications
You must be signed in to change notification settings - Fork 121
[PM-23301] feat: Share user session key across process boundaries #2661
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Details and fixThis branch decides whether to bypass the unlock screen entirely β a security-relevant behavior. Suggested tests:
Also worth adding a complementary test in |
||
| default: | ||
| guard try await services.stateService.isAuthenticated(userId: userId) else { | ||
| return .landingSoftLoggedOut(email: activeAccount.profile.email) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,6 +94,7 @@ public class AppProcessor { | |
|
|
||
| listenForWillEnterForeground(debugWillEnterForeground: debugWillEnterForeground) | ||
| listenForDidEnterBackground(debugDidEnterBackground: debugDidEnterBackground) | ||
| listenForWillResignActive() | ||
| listenForPendingAppIntentActions() | ||
| listenForAcquireCookies() | ||
| } | ||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Details and fixThis 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
|
||
|
|
||
| /// Subscribes to the pending `AppIntent` actions publisher and executes any pending actions | ||
| /// as they arrive, updating navigation and UI accordingly. | ||
| /// | ||
|
|
||
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.
unlockVaultWithSessionKey()has no unit tests covering its three branches.Details and fix
AuthRepositoryTests.swiftcontains zero references tounlockVaultWithSessionKey. PerDocs/Testing.md, "every type containing logic must be tested." Existing parallel functionality (unlockVaultWithNeverlockKey) is fully covered. Please add tests for:trueand callsunlockVault(method: .decryptedKey(...))when a key exists in the keychain.falsewhen keychain throwsKeychainServiceError.osStatusError(errSecItemNotFound)orKeychainServiceError.keyNotFoundβ no exception escapes.unlockVault(...)itself throws (e.g., simulate aninitializeUserCryptofailure).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.