[PM-28264] Consolidate and update the UI for key connector migration/confirmation

[Thomas-Avery]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28264

📔 Objective

The objective is to:

• Consolidate the RemovePasswordComponent into one shared HTML and angular component.
• Update the UI for key connector confirmation/migration components.

Requires the server PR bitwarden/server#6635 for fetching the key connector confirmation details.

Team reviewer notes:

Auth team
Had to update a unit test because of adding a new property to KeyConnectorDomainConfirmation.

📸 Screenshots

Expand sections to see before and after.

Confirm key connector

The UI showed to new SSO users being setup for a Key connector organization.

Web

Before

After

Browser extension

Before

After

Desktop

Before

After

Remove master password

Converts a pre-existing master password user to a key connector user.

Web

Before

After

Browser extension

Before

After

Desktop

Before

After

Tooltip

Edge case handling

For the key connector confirmation there is a possibility for a new client to be using an old self host server that can't provide the key connector confirmation details. If this edge case occurs the new client will still work but won't display the organization data.

⏰ 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 – a40cfc8e-d726-4756-84d4-99d699fe669a

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

[codecov]

Codecov Report

❌ Patch coverage is 86.36364% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.72%. Comparing base (a0943c3) to head (5de86cc).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
libs/angular/src/services/jslib-services.module.ts	0.00%	2 Missing ⚠️
apps/browser/src/popup/app-routing.module.ts	0.00%	1 Missing ⚠️
apps/desktop/src/app/app-routing.module.ts	0.00%	1 Missing ⚠️
apps/web/src/app/oss-routing.module.ts	0.00%	1 Missing ⚠️
...onnector/abstractions/key-connector-api.service.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17642      +/-   ##
==========================================
+ Coverage   41.70%   41.72%   +0.02%     
==========================================
  Files        3571     3571              
  Lines      103817   103837      +20     
  Branches    15605    15608       +3     
==========================================
+ Hits        43294    43326      +32     
+ Misses      58688    58676      -12     
  Partials     1835     1835              

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @Thomas-Avery's task —— View job

---

Code Review Complete ✅

Tasks

• Analyze PR changes with Bitwarden code review agent
• Compile and post review findings

---

Review Summary

Overall Assessment: ✅ APPROVE

This PR successfully consolidates three platform-specific RemovePasswordComponent implementations into a single shared component while improving the UI for key connector confirmation/migration flows. The code follows Bitwarden engineering standards with good security practices, proper error handling, and backward compatibility support.

Key Strengths

👍 Component Consolidation Done Right

• Successfully eliminated duplicate code across web/browser/desktop
• Shared component properly handles platform-specific differences through routing
• Clean separation of concerns with the new KeyConnectorApiService

👍 Security & Data Handling

• No encryption logic added (follows critical rule)
• No vault data sent to API services
• Proper use of encodeURIComponent for URL parameters (line 13 in default-key-connector-api.service.ts)

👍 Backward Compatibility

• Excellent graceful degradation for older self-hosted servers that lack the new endpoint
• Appropriate error handling at lines 127-131 in confirm-key-connector-domain.component.ts
• Well-tested fallback behavior

Review Feedback

---

❓ QUESTION: Test coverage for error scenarios in API service

The DefaultKeyConnectorApiService unit tests (libs/common/src/key-management/key-connector/services/default-key-connector-api.service.spec.ts) verify URL encoding and successful response parsing, but don't test error scenarios (404, 500, network errors).

While the component-level tests (confirm-key-connector-domain.component.spec.ts:86-99) verify that API errors are handled gracefully, should the service-level tests also explicitly verify that API errors propagate correctly?

Current component handling looks correct - it catches errors and continues with organizationName = undefined for backward compatibility with older servers.

---

🎨 SUGGESTED: Consider consolidating duplicate tooltip implementation

Both RemovePasswordComponent and ConfirmKeyConnectorDomainComponent use identical tooltip implementations for the domain field:

• libs/key-management-ui/src/key-connector/remove-password.component.html:16-22
• libs/key-management-ui/src/key-connector/confirm-key-connector-domain.component.html:22-28

<button
  type="button"
  [label]="'keyConnectorDomainTooltip' | i18n"
  tooltipPosition="above-center"
  bitIconButton="bwi-info-circle"
  size="small"
></button>

Consider extracting this into a reusable component or template reference to maintain consistency. However, given these are the only two uses and the code is simple, current duplication is acceptable.

---

🎨 SUGGESTED: Add error test case for non-ErrorResponse errors

In libs/key-management-ui/src/key-connector/remove-password.component.spec.ts, the handleActionError is tested with both Error and ErrorResponse types (lines 141-186), but leave() only tests ErrorResponse (lines 211-252).

For consistency and completeness, consider adding a test case for leave() that throws a generic Error (not ErrorResponse) to match the coverage pattern in convert().

Current test coverage is adequate, but this would improve consistency.

---

❓ QUESTION: Backward compatibility handling for PM-29133 cleanup

libs/key-management-ui/src/key-connector/confirm-key-connector-domain.component.ts:77-82, 98-102 contains temporary code for handling older self-hosted servers:

// PM-29133 Remove during cleanup.
if (this.organizationName == undefined) {
  this.anonLayoutWrapperDataService.setAnonLayoutWrapperData({
    pageTitle: { key: "verifyYourDomainToLogin" },
  });
}

Is there a tracking task PM-29133 to remove this backward compatibility code once minimum server version requirements are updated? The fallback behavior is well-implemented with appropriate logging.

---

🎨 SUGGESTED: Enhance API service error handling documentation

libs/common/src/key-management/key-connector/services/default-key-connector-api.service.ts:8-18 uses apiService.send() with parameters true, true for the last two arguments, but these aren't documented.

Consider adding JSDoc comments to clarify the API contract:

/**
 * Fetches key connector confirmation details for an organization.
 * Requires authentication.
 * @param orgSsoIdentifier - Organization SSO identifier (will be URL-encoded)
 * @returns Organization details including name
 * @throws Will throw if API endpoint doesn't exist (older servers) or network error occurs
 */
async getConfirmationDetails(
  orgSsoIdentifier: string,
): Promise<KeyConnectorConfirmationDetailsResponse> {

---

Conclusion

The PR is well-implemented with good test coverage and proper handling of edge cases. The component consolidation successfully reduces code duplication while maintaining functionality across all platforms. The suggestions above are minor improvements that don't block approval.

Recommendation: ✅ Approve

---

[JaredSnider-Bitwarden]

Approving auth single test file change without reviewing rest of PR.

[coroiu]

Cleaning up loose components, nice 👍

[quexten]

Approving, but i've put a request for a follow-up ticket. Preferably it is linked directly here in the code.

[Thomas-Avery]

Updated all screenshots with the latest changes.

[coroiu]

No platform change since last review, approving

[quexten]

Approving but there is merge conflicts. Please ping for re-approval.