[PM-26209] Use AuthRequestAnsweringService on Desktop #16906
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
|
Claude finished @rr-bw's task —— View job Code Review CompleteThis PR successfully implements SummaryAll previous Claude findings have been addressed appropriately by the author. The implementation follows Angular best practices, uses RxJS observables correctly, and maintains proper separation of concerns between Desktop, Browser, and common library code. Key improvements:
FindingsFinding 1: Optional method pattern creates potential runtime issues The // In default-server-notifications.service.ts:298-306
if (this.authRequestAnsweringService.receivedPendingAuthRequest) {
try {
await this.authRequestAnsweringService.receivedPendingAuthRequest(...)
} catch (error) {
this.logService.error(`Failed to process auth request notification: ${error}`);
}
}This pattern works but has trade-offs:
Alternative approaches considered:
Recommendation: Current approach is acceptable given the cross-platform nature of the codebase. The try-catch error handling provides adequate safety. However, if more optional methods are added in the future, consider refactoring to separate interfaces. Finding 2: Code duplication between Desktop and Browser app components The
Both implement identical do/while loop patterns with the same flags ( Impact:
Mitigation options:
Author's response indicates this is a known trade-off accepted for scope management. The duplication is documented in the review history. Finding 3: Dialog dismissal state management is intentional Previous review suggested clearing pending state when dialogs are dismissed without response. Author clarified this is intentional behavior:
This design ensures:
This is the correct behavior for security-sensitive operations. Positive Observations
Checklist for Author
RecommendationApprove with observations noted. The code is production-ready. The findings above are architectural observations rather than blocking issues. Human reviewers have already approved, and all technical concerns have been addressed. The optional method pattern (Finding 1) and code duplication (Finding 2) are acceptable trade-offs given the constraints. Consider addressing these in future refactoring if the patterns expand to more methods or clients. |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16906 +/- ##
==========================================
+ Coverage 40.79% 40.81% +0.02%
==========================================
Files 3542 3541 -1
Lines 101502 101583 +81
Branches 15222 15237 +15
==========================================
+ Hits 41403 41458 +55
- Misses 58349 58375 +26
Partials 1750 1750 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rr-bw
requested review from
Patrick-Pimentel-Bitwarden,
coroiu and
nick-livefront
coroiu
left a comment
coroiu
left a comment
There was a problem hiding this comment.
No real changes to platform code, approving
nick-livefront
left a comment
nick-livefront
left a comment
There was a problem hiding this comment.
Only deletion of vault code ✅
Claude is calling out two issues above that are worth glancing through
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
👍 Overall excellent work, I have just one outstanding question I've pinged you on slack. Thank you for extending the feature set to the desktop.
rr-bw
dismissed stale reviews from Patrick-Pimentel-Bitwarden, nick-livefront, and coroiu
via
7ba4672
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const result = await firstValueFrom(dialogRef.closed); | ||
|
|
||
| if (result !== undefined && typeof result === "boolean") { | ||
| respondedIds.add(req.id); | ||
|
|
||
| if (respondedIds.size === pendingList.length && this.activeUserId != null) { | ||
| await this.pendingAuthRequestsState.clear(this.activeUserId); | ||
| } | ||
| } |
There was a problem hiding this comment.
❌ Finding 1: Incomplete state cleanup on dialog dismissal
The pending auth state is only cleared when all requests receive a boolean response. If a user dismisses the dialog without responding (e.g., ESC key, clicking outside), result will be undefined and the pending marker remains in state until the 15-minute timeout.
Suggested fix:
const respondedIds = new Set<string>();
const dismissedIds = new Set<string>();
for (const req of pendingList) {
if (req?.id == null) {
continue;
}
const dialogRef = LoginApprovalDialogComponent.open(this.dialogService, {
notificationId: req.id,
});
const result = await firstValueFrom(dialogRef.closed);
if (result !== undefined && typeof result === "boolean") {
respondedIds.add(req.id);
} else {
// Dialog dismissed without response - still remove from pending state
dismissedIds.add(req.id);
}
// Clear state when all requests have been either responded to or dismissed
if (respondedIds.size + dismissedIds.size === pendingList.length && this.activeUserId != null) {
await this.pendingAuthRequestsState.clear(this.activeUserId);
}
}There was a problem hiding this comment.
If a user dismisses the dialog without responding (e.g., ESC key, clicking outside), result will be undefined and the pending marker remains.
This is actually what we want. Only clear the markers for the user if all of their auth requests have been specifically responded to with an approve or deny.
| if (this.authRequestAnsweringService.receivedPendingAuthRequest) { | ||
| await this.authRequestAnsweringService.receivedPendingAuthRequest( | ||
| notification.payload.userId, | ||
| notification.payload.id, | ||
| ); |
There was a problem hiding this comment.
If receivedPendingAuthRequest throws an error, notification processing fails silently. Add try-catch for robustness:
if (this.authRequestAnsweringService.receivedPendingAuthRequest) {
try {
await this.authRequestAnsweringService.receivedPendingAuthRequest(
notification.payload.userId,
notification.payload.id,
);
} catch (error) {
this.logService.error(`Failed to process auth request notification: ${error}`);
}
}
| do { | ||
| this.shouldRerunAuthRequestProcessing = false; | ||
| await this.processPendingAuthRequests(); | ||
| // If an "openLoginApproval" message was received while processPendingAuthRequests() was running, then | ||
| // shouldRerunAuthRequestProcessing will have been set to true | ||
| } while (this.shouldRerunAuthRequestProcessing); |
There was a problem hiding this comment.
💭 Finding 3: Consider error handling in do/while loop
If processPendingAuthRequests() consistently throws errors, they may be swallowed. Consider adding error logging:
do {
this.shouldRerunAuthRequestProcessing = false;
try {
await this.processPendingAuthRequests();
} catch (error) {
this.logService.error(`Error processing pending auth requests: ${error}`);
// Reset flag to prevent infinite loop on persistent errors
this.shouldRerunAuthRequestProcessing = false;
}
} while (this.shouldRerunAuthRequestProcessing);
| ); | ||
| } else { | ||
| // This call is necessary for Web, which uses a NoopAuthRequestAnsweringService | ||
| // that does not have a recievedPendingAuthRequest() method |
There was a problem hiding this comment.
💭 Finding 4: Typo in comment
"recievedPendingAuthRequest" should be "receivedPendingAuthRequest"
…revent an infinite loop
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
👍 Thanks Ryan! Really really good work. Thank you for going so far with this.
|
|
🎟️ Tracking
PM-26209
📔 Objective
Update Desktop to use the
AuthRequestAnsweringService, bringing it into feature parity with the Extension.📸 Screenshares
Note: OS notifications don't show while recording, which is why I'm clicking the menu to show the OS notifications in the recordings below.
Basic Login Scenario
basic-login.mov
Active User Scenarios
User is Active and Locked
active-locked.mov
User is Active and Locked (window is hidden)
active-locked-window-hidden.mov
User is Active and Unlocked
active-unlocked.mov
User is Active and Unlocked (window is hidden)
active-unlocked-window-hidden.mov
Inactive User Scenarios
User is Inactive and Locked
inactive-locked.mov
User is Inactive and Locked (window is hidden)
inactive-locked-window-hidden.mov
User is Inactive and Unlocked
inactive-unlocked.mov
User is Inactive and Unlocked (window is hidden)
inactive-unlocked-window-hidden.mov
Auth request gets sent for a user. That user then authenticates but is prompted to change password upon login
login-but-required-to-change-password.mov
User is already authenticated and on change password screen when an auth request comes in
unlocked-but-required-to-change-password.mov
⏰ 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