Auth/PM-26745 - Rename SSO management component to SsoManageComponent to make SSO components distinct + Strict TS update

[JaredSnider-Bitwarden]

🎟️ Tracking

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

📔 Objective

We have 2 SsoComponents today. One used for authentication and one where users configure their SSO setup. We should better distinguish between them by naming the setup/management component SsoManageComponent. I elected to keep the other component as SsoComponent since it is still clear without adding something like SsoAuthComponent and it would also require renaming the SSOComponentServices. I also updated the component to meet Strict TS while I was in here.

📸 Screenshots

PM-26745.-.Sso.manage.component.loads.works.mov

⏰ 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 – a743417a-6910-456b-b857-492f515ed61a

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

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[codecov]

Codecov Report

❌ Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.46%. Comparing base (51a5575) to head (7fc2e3c).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...e/bit-web/src/app/auth/sso/sso-manage.component.ts	0.00%	24 Missing ⚠️
...sole/organizations/organizations-routing.module.ts	0.00%	1 Missing ⚠️
...dmin-console/organizations/organizations.module.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16893      +/-   ##
==========================================
- Coverage   40.47%   40.46%   -0.01%     
==========================================
  Files        3496     3496              
  Lines       99968    99983      +15     
  Branches    14981    14987       +6     
==========================================
+ Hits        40462    40463       +1     
- Misses      57790    57805      +15     
+ Partials     1716     1715       -1     

☔ 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 @JaredSnider-Bitwarden's task —— View job

---

PR Review Complete ✅

Summary of Changes

This PR renames SsoComponent to SsoManageComponent to distinguish it from the authentication SSO component, and updates the component to meet Strict TypeScript requirements. The changes affect 4 files:

1. Routing module update - Import and reference renamed component
2. Module declaration update - Import and declare renamed component
3. HTML template rename - Minor template reference updates for type safety
4. Component class rename and Strict TS compliance - Main changes including:
   • Removed @ts-strict-ignore directive
   • Added proper type annotations and null safety
   • Updated FormControl definitions with nonNullable options
   • Added null checks before accessing organizationId
   • Improved getter implementations

---

🔴 Critical Issues

1. Potential Runtime Error - Missing Null Check (sso-manage.component.ts:248-250)

Location: bitwarden_license/bit-web/src/app/auth/sso/sso-manage.component.ts:248-250

this.ssoConfigForm
  .get("configType")
  ?.valueChanges.pipe(takeUntil(this.destroy$))

Issue: While you added optional chaining (?.) to get("configType"), if this returns null, calling .pipe() on undefined will throw a runtime error. The subscription will fail silently.

Solution: Either assert the control exists or handle the null case:

const configTypeControl = this.ssoConfigForm.get("configType");
if (configTypeControl) {
  configTypeControl.valueChanges.pipe(takeUntil(this.destroy$))
    .subscribe((newType: SsoType) => {
      // ... existing logic
    });
}

Or use this.ssoConfigForm.controls.configType directly since you know it exists in the form definition.

2. Same Issue with spSigningBehavior Subscription (sso-manage.component.ts:264-266)

Location: bitwarden_license/bit-web/src/app/auth/sso/sso-manage.component.ts:264-266

this.samlForm
  .get("spSigningBehavior")
  ?.valueChanges.pipe(takeUntil(this.destroy$))

Issue: Same problem as above - potential runtime error if the control is unexpectedly null.

Solution: Use direct control access or add null handling:

const spSigningBehaviorControl = this.samlForm.get("spSigningBehavior");
if (spSigningBehaviorControl) {
  spSigningBehaviorControl.valueChanges.pipe(takeUntil(this.destroy$))
    .subscribe(() => this.samlForm.get("idpX509PublicCert")?.updateValueAndValidity());
}

---

⚠️ Issues Requiring Attention

3. Inconsistent Getter Pattern (sso-manage.component.ts:206-214)

Location: bitwarden_license/bit-web/src/app/auth/sso/sso-manage.component.ts:206-214

get enabledCtrl() {
  return this.ssoConfigForm?.controls?.enabled as FormControl;
}
get ssoIdentifierCtrl() {
  return this.ssoConfigForm?.controls?.ssoIdentifier as FormControl;
}
get configTypeCtrl() {
  return this.ssoConfigForm?.controls?.configType as FormControl;
}

Issue: These getters use optional chaining but then cast to non-nullable FormControl. This is inconsistent with the new keyConnectorUrlFormCtrl getter pattern (line 468-470) which properly types the return as FormControl<string>.

Recommendation: For Strict TS consistency, either:

1. Make these return FormControl | undefined to match the optional chaining, OR
2. Remove optional chaining since ssoConfigForm is always initialized and controls always exist

The second option is better since the form is initialized in the constructor:

get enabledCtrl() {
  return this.ssoConfigForm.controls.enabled;
}

4. Removed Toast Title Without Justification (sso-manage.component.ts:371)

Location: bitwarden_license/bit-web/src/app/auth/sso/sso-manage.component.ts:371

Before:

this.toastService.showToast({
  variant: "success",
  title: null,
  message: this.i18nService.t("ssoSettingsSaved"),
});

After:

this.toastService.showToast({
  variant: "success",
  message: this.i18nService.t("ssoSettingsSaved"),
});

Question: Was title: null removed because it's now optional in the ToastService API? If so, this is fine. Otherwise, verify this doesn't change the toast display behavior.

---

🎨 Suggested Improvements

5. FormControl Type Annotations Could Be More Specific

Location: Throughout the FormControl definitions (lines 133-204)

Current approach: Uses ControlsOf<SsoConfigView> type utility, which is good, but individual FormControl definitions don't always specify the value type.

Example: Line 138:

metadataAddress: new FormControl(),

Suggestion: Consider adding explicit type parameters for better type safety:

metadataAddress: new FormControl<string | null>(null),

This would make the code more self-documenting and catch type mismatches earlier.

6. Error Messages Could Be More Descriptive

Location: Lines 301, 354, 526

Current:

throw new Error("Load: Organization ID is not set");
throw new Error("Submit: Organization ID is not set");
throw new Error("upsertOrganizationWithSsoChanges: Organization ID is not set");

Suggestion: Include more context for debugging:

throw new Error(`Load: Organization ID is not set. Route params: ${JSON.stringify(this.route.snapshot.params)}`);

7. Consider Extracting Magic String

Location: Line 51

const defaultSigningAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256";

Observation: This is used throughout the file. Consider extracting to a const object pattern per ADR-0025:

export const SamlSigningAlgorithm = Object.freeze({
  RsaSha256: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256",
  RsaSha384: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha384",
  RsaSha512: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512",
} as const);

Though this is a minor suggestion and the current approach is acceptable.

---

👍 Good Practices Observed

• Comprehensive Strict TS migration - Added proper null safety throughout
• Consistent null checks - Added organizationId validation in all methods that use it
• Proper form initialization - All FormControls properly configured with nonNullable options where appropriate
• Component renaming - Clear naming distinction between auth and management SSO components
• Template type safety improvements - Updated HTML template references to use strongly-typed getter

---

📋 Action Items for Author

Required Changes

1. Fix critical runtime error risks - Address optional chaining issues on valueChanges subscriptions (Issues i18n #1 and Site Search #2)
2. Standardize getter patterns - Make getters consistent in their null handling approach (Issue Overlay Popup #3)
3. Verify toast behavior - Confirm removal of title: null doesn't break toast display (Issue New Site Notifications #4)

Recommended Changes

1. Add explicit type parameters to FormControl definitions for better type safety (Issue Generate Password Context Menu Option #5)
2. Enhance error messages with more debugging context (Issue Encryption Key Storage #6)
3. Consider extracting magic strings to const objects per ADR-0025 (Issue Keyboard Commands (hotkeys) #7)

---

Test Coverage Note

Codecov reports 0% patch coverage with 24 lines missing coverage. While this component is difficult to test due to its complexity, consider adding unit tests for:

• The new null safety checks in load(), submit(), and upsertOrganizationWithSsoChanges()
• The keyConnectorUrlFormCtrl getter
• Form validation logic

---