Auth & KM/PM-3537 - Remove "KeyConnectorUrl" from root of IdentityTokenResponse

[JaredSnider-Bitwarden]

🎟️ Tracking

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

Clients PR: bitwarden/clients#17593

📔 Objective

Over 2 years ago, we deprecated the usage of the IdentityTokenResponse.keyConnectorUrl so we should remove it.

📸 Screenshots

n/a

⏰ 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

[claude]

Claude finished @JaredSnider-Bitwarden's task —— View job

---

Code Review Complete

Tasks

• Read existing PR comments and threads
• Review changed files
• Analyze PR title and description quality
• Check for security implications
• Assess breaking change impact
• Verify test coverage
• Post comprehensive review findings

---

Summary

This PR removes the deprecated KeyConnectorUrl property from the root of the identity token response. The property was deprecated over 2 years ago and clients should now be reading it from UserDecryptionOptions.KeyConnectorOption.KeyConnectorUrl instead. The changes are minimal and focused: removing one line of code that set the root-level property and updating tests accordingly.

---

Findings

Finding 1: PR title and description should clarify client compatibility requirements

💭 The PR description mentions "ensure mobile won't be affected negatively" (author's own comment), but doesn't explicitly state which client versions are compatible with this breaking change. Consider adding:

• Minimum supported client versions that have migrated away from reading the root-level KeyConnectorUrl
• Link or reference to when clients completed the migration (the linked clients PR is helpful but may not tell the full story about all platforms)

Suggested addition to PR description

## 🔄 Breaking Change Assessment

This change removes `KeyConnectorUrl` from the root of the identity token response. Clients must read this value from `UserDecryptionOptions.KeyConnectorOption.KeyConnectorUrl` instead.

**Client Compatibility:**
- ✅ Web: Migrated in [version/commit]
- ✅ Desktop: Migrated in [version/commit]
- ✅ Browser: Migrated in [version/commit]
- ⚠️ Mobile (iOS/Android): [Status/version info needed]
- ✅ CLI: Migrated in [version/commit]

All active client versions in production must support reading from the new location before this can be safely deployed.

---

Finding 2: Consider verifying all clients have migrated before merging

⚠️ Given the author's comment "Pausing b/c we should ensure mobile won't be affected negatively by this", this appears to be awaiting verification. Before merging, confirm:

1. All client platforms (Web, Desktop, Browser, Mobile iOS, Mobile Android, CLI) have deployed versions that read from UserDecryptionOptions.KeyConnectorOption.KeyConnectorUrl
2. Minimum supported client versions are all updated (i.e., any legacy clients still in use that might break)
3. Rollback strategy is documented if issues arise post-deployment

The linked clients PR (bitwarden/clients#17593) should provide guidance, but mobile platform status needs explicit confirmation.

---

Code Quality Assessment

Security: ✅ No security concerns. This is a cleanup of deprecated functionality and doesn't change authentication/authorization logic.

Breaking Changes: ⚠️ This IS a breaking change for any clients still reading KeyConnectorUrl from the root. Requires careful deployment coordination.

Test Coverage: ✅ Appropriate. The test in IdentityServerSsoTests.cs:524-526 that verified the backward-compatible root-level property was correctly removed, and the primary test for the proper location (KeyConnectorOption.KeyConnectorUrl) remains intact.

Code Maintainability: ✅ Good. The comment update in CustomTokenRequestValidator.cs:162-163 accurately reflects that ResetMasterPassword still needs cleanup, maintaining clear technical debt documentation.

Good Practices Observed:

• Clean removal of deprecated code path
• Updated inline comments to reflect current state
• Test assertions updated to match new behavior

---

Action Items

1. [REQUIRED] Confirm all mobile clients (iOS/Android) have migrated to reading KeyConnectorUrl from UserDecryptionOptions.KeyConnectorOption
2. [RECOMMENDED] Document minimum client version requirements in the PR description
3. [RECOMMENDED] Coordinate deployment timing with client releases to ensure smooth rollout

---

[JaredSnider-Bitwarden]

Pausing b/c we should ensure mobile won't be affected negatively by this.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.10%. Comparing base (9573cab) to head (05fef65).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6627       +/-   ##
===========================================
+ Coverage   13.38%   53.10%   +39.71%     
===========================================
  Files        1125     1903      +778     
  Lines       49593    84876    +35283     
  Branches     3914     7633     +3719     
===========================================
+ Hits         6640    45070    +38430     
+ Misses      42831    38055     -4776     
- Partials      122     1751     +1629     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Checkmarx One – Scan Summary & Details – b21f6266-4e80-4755-9336-1ac64169aac1

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