Skip to content

[PM-33569] fix: Remove pending login request if it no longer exists#2498

Merged
matt-livefront merged 1 commit intomainfrom
matt/PM-33569-auth-request-pending
Apr 1, 2026
Merged

[PM-33569] fix: Remove pending login request if it no longer exists#2498
matt-livefront merged 1 commit intomainfrom
matt/PM-33569-auth-request-pending

Conversation

@matt-livefront
Copy link
Copy Markdown
Collaborator

@matt-livefront matt-livefront commented Mar 26, 2026

🎟️ Tracking

PM-33569

📔 Objective

When a login with device push notification is received by the app, the app will attempt to prompt the user immediately.

It's possible that it can't be shown immediately (app is backgrounded and killed before the user returns or the user's vault is locked when the push notification is received). For this case, the login with device request is persisted until the next vault unlock, when we attempt to show it to the user. In the meantime, it's possible that the request has expired and been removed from the server, resulting in a 404.

When this occurred, an error was logged to the console/flight recorder, but the persisted request was never deleted. So the app would continue to try and fetch the request on each vault unlock.

This catches the error and deletes the request so the app doesn't continue to try fetching it.

@matt-livefront matt-livefront requested a review from a team as a code owner March 26, 2026 19:41
@matt-livefront matt-livefront added ai-review Request a Claude code review t:bug Change Type - Bug labels Mar 26, 2026
@github-actions github-actions bot added the app:password-manager Bitwarden Password Manager app context label Mar 26, 2026
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 26, 2026

Claude finished @matt-livefront's task in 2m 3s —— View job


Code Review: PM-33569 Remove pending login request if it no longer exists

  • Gather context and review PR changes
  • Review PendingLoginRequest.swift changes
  • Review NotificationService.swift changes
  • Review VaultListProcessor.swift changes
  • Review test coverage

Summary

This PR correctly fixes a bug where expired/deleted pending login requests were never cleaned up from local state, causing the app to repeatedly attempt to fetch them on each vault unlock.

Changes reviewed:

  • PendingLoginRequest.swift — Adds PendingLoginRequestError.notFound and a validate override that throws on HTTP 404. Clean and well-scoped.
  • NotificationService.swift — Refactors duplicated show/switch logic into showOrSwitchForLoginRequest, catches the new error to clear persisted state. Good DRY improvement.
  • VaultListProcessor.swift — Adds a catch is PendingLoginRequestError block to clear state on the vault-unlock path. Correct handling.
  • Tests — Three new tests cover the 404 handling across both notification paths (received/tapped) and the vault-appear path.

No actionable findings. The fix is correct, well-scoped, and properly tested. The error handling follows established patterns in the codebase, and the refactoring in NotificationService is a nice cleanup of duplicated logic.

@github-actions
Copy link
Copy Markdown
Contributor

Logo
Checkmarx One – Scan Summary & Details50677ad2-23e0-4ec1-8131-8e97768f9a6d

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 98.76543% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.92%. Comparing base (2686997) to head (afc4464).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
...rvices/API/Auth/Requests/PendingLoginRequest.swift 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2498   +/-   ##
=======================================
  Coverage   86.92%   86.92%           
=======================================
  Files        1846     1846           
  Lines      163487   163542   +55     
=======================================
+ Hits       142110   142158   +48     
- Misses      21377    21384    +7     

☔ 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.

let content = UNMutableNotificationContent()
content.title = Localizations.logInRequested
content.body = Localizations.confimLogInAttempForX(loginSourceEmail)
content.body = Localizations.confirmLogInAttemptForX(loginSourceEmail)
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.

nice 👍

@matt-livefront matt-livefront merged commit 6b8c37d into main Apr 1, 2026
28 checks passed
@matt-livefront matt-livefront deleted the matt/PM-33569-auth-request-pending branch April 1, 2026 20:42
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