[PM-26209] Use AuthRequestAnsweringService on Desktop

[rr-bw]

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

[github-actions]

Checkmarx One – Scan Summary & Details – b6f0ef17-0e32-4d27-95a5-551a9f2610e9

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

[claude]

Claude finished @rr-bw's task —— View job

---

Code Review Complete ✅

Summary of Changes

This PR refactors the authentication request answering service to properly support both Desktop and Browser Extension clients, addressing a bug where Desktop would error when approving auth requests. The main changes:

1. Renamed base service: AuthRequestAnsweringService → DefaultAuthRequestAnsweringService to make room for the abstraction
2. Created client-specific implementations:
   • DesktopAuthRequestAnsweringService - handles Desktop-specific system notifications via IPC
   • ExtensionAuthRequestAnsweringService - handles browser extension notifications
3. Removed dialog component service abstraction - functionality moved into the client-specific services
4. Updated dependency injection across all clients (Desktop, Browser, Web)
5. Refactored Desktop app.component.ts to properly handle pending auth requests

---

Critical Issues ❌

1. Desktop: Missing authRequestId parameter causes exception

Location: apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.ts:39

override async receivedPendingAuthRequest(userId: UserId): Promise<void> {

Problem: The Desktop service doesn't accept authRequestId parameter while the Extension service does (line 45 of Extension service). However, the abstraction defines it as optional:

abstract receivedPendingAuthRequest(userId: UserId, authRequestId?: string): Promise<void>;

The Extension service requires the authRequestId and throws an error if missing (lines 46-48), but Desktop ignores it completely. This inconsistency could cause issues if the notification system passes an authRequestId to Desktop.

Recommendation: Desktop should also accept and potentially use the authRequestId parameter, even if it's not strictly required for its notification system. At minimum, document why it's ignored.

---

2. Test file is outdated and broken

Location: libs/common/src/auth/services/auth-request-answering/auth-request-answering.service.spec.ts:70-80

Problem: The test imports and instantiates a non-existent class. Looking at the test file:

import { AuthRequestAnsweringService } from "../../abstractions/auth-request-answering/auth-request-answering.service.abstraction";
// ...
sut = new AuthRequestAnsweringService(
  accountService,
  actionService,
  authService,
  // ... etc
);

The test is trying to instantiate AuthRequestAnsweringService which is now an abstract class (the interface/abstraction). This test should be testing either:

• ExtensionAuthRequestAnsweringService (which has the actual implementation)
• Or should be deleted if it's meant to test the extension-specific behavior (which now lives in the browser app)

The test parameters also don't match the DefaultAuthRequestAnsweringService constructor signature.

Recommendation: Either update the test to test ExtensionAuthRequestAnsweringService or delete it and add proper tests in the browser app directory. This is blocking proper test coverage.

---

3. Desktop: Commented-out code should be removed

Location: apps/desktop/src/app/app.component.ts:532-538

// if (message.notificationId != null) {
//   this.dialogService.closeAll();
//   const dialogRef = LoginApprovalDialogComponent.open(this.dialogService, {
//     notificationId: message.notificationId,
//   });
//   await firstValueFrom(dialogRef.closed);
// }

Problem: This is dead code that should be removed, not commented out. It adds confusion about the intended logic.

Recommendation: Remove this commented code block entirely.

---

Significant Issues ⚠️

4. Desktop: Race condition with processingPendingAuth flag

Location: apps/desktop/src/app/app.component.ts:497-530

Problem: The processingPendingAuth flag prevents concurrent processing, which is good. However, if an error occurs before reaching the finally block, or if the component is destroyed mid-processing, the flag might get stuck. The component has a destroy$ Subject but it's not used to clean up this particular operation.

Recommendation: Consider using RxJS operators like take(1) or takeUntil(this.destroy$) for better lifecycle management, or add error handling to ensure the flag is always reset.

---

5. Desktop: Window visibility check has misleading comment

Location: apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.ts:65

const meetsExtensionConditions = meetsBasicConditions && isWindowVisible;

Problem: The variable is named meetsExtensionConditions but this is the Desktop service, not Extension. This is clearly copy-pasted from Extension service (line 76 of extension service has the same naming pattern).

Recommendation: Rename to meetsDesktopConditions for clarity.

---

6. Desktop: Unimplemented method throws generic error

Location: apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.ts:71

async handleAuthRequestNotificationClicked(event: SystemNotificationEvent) {
  throw new Error("handleAuthRequestNotificationClicked() not implemented for this client");
}

Problem: If Desktop system notifications are created (lines 52-56) but clicking them calls this unimplemented method, it will crash. The flow seems incomplete - notifications are created but can't be acted upon.

Recommendation: Either implement the handler or add a comment explaining why it's safe to leave unimplemented (e.g., if Desktop notifications never trigger click events).

---

7. Abstraction comment is now incorrect

Location: libs/common/src/auth/abstractions/auth-request-answering/auth-request-answering.service.abstraction.ts:11

/**
 * Currently, this is only implemented for browser extension.
 */

Problem: This comment is outdated - the functionality is now implemented for Desktop as well.

Recommendation: Update the comment to reflect Desktop implementation, or remove it entirely since the abstraction itself documents what needs to be implemented.

---

Improvements & Suggestions 🎨

8. Missing JSDoc on new public services

Location: Both DesktopAuthRequestAnsweringService and ExtensionAuthRequestAnsweringService

Suggestion: Add class-level JSDoc comments explaining the purpose and behavior of these client-specific implementations. This helps future developers understand when to use which implementation.

---

9. Consider extracting magic number

Location:

• apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.ts:78
• libs/common/src/auth/services/auth-request-answering/default-auth-request-answering.service.ts:58

Suggestion: The 15 * 60 * 1000 magic number appears in multiple places. Consider extracting to a named constant at the module level or in a shared location, like:

const AUTH_REQUEST_EXPIRATION_MS = 15 * 60 * 1000; // 15 minutes

The comment references GlobalSettings.cs, so this should ideally be a shared constant.

---

10. Desktop app.component: Consider early return for empty pending list

Location: apps/desktop/src/app/app.component.ts:505-527

Suggestion: You could add an early return after checking if pendingList is empty to reduce nesting:

const pendingList = await firstValueFrom(
  this.authRequestService.getPendingAuthRequests$(),
);

if (!Array.isArray(pendingList) || pendingList.length === 0) {
  return;
}

const respondedIds = new Set<string>();
for (const req of pendingList) {
  // ...
}

---

11. NoopAuthRequestAnsweringService has inconsistent behavior

Location: libs/common/src/auth/services/auth-request-answering/noop-auth-request-answering.service.ts

Observation: The NoopAuthRequestAnsweringService:

• receivedPendingAuthRequest - does nothing (OK for noop)
• userMeetsConditionsToShowApprovalDialog - throws error
• handleAuthRequestNotificationClicked - does nothing (OK for noop)
• processPendingAuthRequests - does nothing (OK for noop)

Suggestion: The error-throwing method is inconsistent with noop pattern. Either all methods should be no-ops, or the class should be renamed to indicate it's not a true no-op. Consider renaming parameter from notificationId to authRequestId to match abstraction (line 9).

---

12. README is too minimal

Location: libs/common/src/auth/abstractions/auth-request-answering/README.md

Suggestion: The README just references a diagram. Consider adding:

• Brief explanation of the feature
• Which clients implement it
• Common integration patterns
• Link to architecture documentation

---

Good Practices Observed 👍

• Proper use of inheritance with extends DefaultAuthRequestAnsweringService
• Following Observable patterns with RxJS (no imperative state management)
• Proper dependency injection setup
• Guard against concurrent processing with processingPendingAuth flag
• Appropriate separation of concerns between base and client-specific implementations

---

Action Items for Author

Must Fix (before merge):

1. Fix or remove broken test file (auth-request-answering.service.spec.ts)
2. Remove commented-out code in app.component.ts:532-538
3. Fix misleading variable name meetsExtensionConditions in Desktop service
4. Update outdated comment in abstraction about "only implemented for browser extension"

Should Fix (high priority):

5. Address Desktop authRequestId parameter inconsistency
6. Implement or document why handleAuthRequestNotificationClicked throws error on Desktop
7. Add proper error handling for processingPendingAuth race condition

Nice to Have:

8. Add JSDoc comments to new service classes
9. Extract magic number for auth request expiration
10. Improve README documentation

---

🔍 Review Methodology

This review examined:

• Architecture adherence (Observable patterns, DI, separation of concerns)
• Security implications (no decrypted data exposure detected)
• Code quality and maintainability
• Test coverage and correctness
• Documentation completeness
• Type safety and error handling
• Consistency across similar implementations

The review prioritized correctness, security, and maintainability over stylistic preferences.

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
22.7% Duplication on New Code

See analysis details on SonarQube Cloud

[bw-ghapp]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and all feature flags disabled.

✅ Fortunately, these BIT tests have passed! 🎉

[bw-ghapp]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and the feature flag configuration used by vault.bitwarden.com.

✅ Fortunately, these BIT tests have passed! 🎉