Skip to content

[PM-35118] fix: Prevent vault timeout from re-firing for already-soft-logged-out accounts#2571

Merged
morganzellers-bw merged 5 commits into
mainfrom
pm-35118-vault-timeout-issue-claude
May 8, 2026
Merged

[PM-35118] fix: Prevent vault timeout from re-firing for already-soft-logged-out accounts#2571
morganzellers-bw merged 5 commits into
mainfrom
pm-35118-vault-timeout-issue-claude

Conversation

@morganzellers-bw
Copy link
Copy Markdown
Contributor

@morganzellers-bw morganzellers-bw commented Apr 24, 2026

🎟️ Tracking

PM-35118

📔 Objective

After setting Vault Timeout to Immediate + Log Out, the account is soft-logged-out on foreground (PM-11472 design, access token removed but account kept in state for email pre-fill). However, checkSessionTimeouts was re-firing the timeout on every subsequent foreground event because hasPassedSessionTimeout always returns true for .immediately (0 seconds elapsed ≥ 0). This caused the app to navigate the user back to the landing screen even while they were mid-way through re-authenticating on the login screen.

Fix: Add && !account.isLoggedOut to the shouldTimeout guard in checkSessionTimeouts. account.isLoggedOut is !isAuthenticated, which (after PM-35285) correctly returns true for soft-logged-out accounts. This prevents the timeout from re-firing for accounts that are already logged out.

Test corrections included:

  • Two existing tests (lockAccount, logoutAccount) needed stateService.isAuthenticated[userId] = trueMockStateService defaults isAuthenticated to false, so without it the accounts appeared already-logged-out and the timeout was skipped.
  • One existing test (timedOut_activeAccount_handleActiveUser) was vacuously passing because the closure assertion was never reached; hardened with an explicit handleActiveUserCalled flag.
  • One router test (didTimeout_sessionExpired_logout) was asserting .landing but production actually returns .landingSoftLoggedOut — the old assertion only passed because authRepository.activeAccount was unset, forcing getAccount() through the catch path.

Screenshots

Before & After Videos
pm-35118-after-720.mov
pm-35118-before-720.mov

@morganzellers-bw morganzellers-bw added the ai-review Request a Claude code review label Apr 24, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels Apr 24, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.23%. Comparing base (4047574) to head (3dbb061).

Files with missing lines Patch % Lines
...d/Core/Auth/Repositories/AuthRepositoryTests.swift 90.47% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2571   +/-   ##
=======================================
  Coverage   87.22%   87.23%           
=======================================
  Files        1899     1899           
  Lines      168083   168105   +22     
=======================================
+ Hits       146612   146646   +34     
+ Misses      21471    21459   -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 30, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the surgical fix to DefaultAuthRepository.checkSessionTimeouts that prevents the time-based timeout branch from re-firing on already soft-logged-out accounts by adding && !account.isLoggedOut to the shouldTimeout guard. Verified the production change against Account.isLoggedOut (defined as !isAuthenticated at AuthRepository.swift:1162) and confirmed the existing shouldLogoutDueToNoUnlockMethod already guards on the same condition, so the new clause makes the two paths consistent. The two existing test corrections are justified — MockStateService defaults isAuthenticated to false, so the original tests were silently skipping the timeout — and the previously vacuous closure assertion is now properly asserted via handleActiveUserCalled. The AuthRouterTests adjustment correctly reflects production routing through timeoutRedirect (AuthRouter+Redirects.swift:312 returns .landingSoftLoggedOut); the prior assertion only passed because getAccount() was hitting the catch path.

Code Review Details

No findings.

@morganzellers-bw morganzellers-bw marked this pull request as ready for review May 6, 2026 16:44
@morganzellers-bw morganzellers-bw requested review from a team and matt-livefront as code owners May 6, 2026 16:44
@morganzellers-bw morganzellers-bw self-assigned this May 6, 2026
@morganzellers-bw morganzellers-bw merged commit f96041c into main May 8, 2026
17 checks passed
@morganzellers-bw morganzellers-bw deleted the pm-35118-vault-timeout-issue-claude branch May 8, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants