[PM-25652] Add endpoint to fetch key connector confirmation details

[Thomas-Avery]

🎟️ Tracking

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

📔 Objective

The objective of this PR is to provide a new endpoint to fetch information needed when confirming/migrating a new key connector user.

See the associated client PR bitwarden/clients#17642

⏰ 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 – 1e6d369f-21a8-4496-9ee5-733aebb71b8e

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 97

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.12%. Comparing base (9573cab) to head (3ee3974).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6635       +/-   ##
===========================================
+ Coverage   13.38%   53.12%   +39.73%     
===========================================
  Files        1125     1906      +781     
  Lines       49593    84915    +35322     
  Branches     3914     7636     +3722     
===========================================
+ Hits         6640    45112    +38472     
+ Misses      42831    38053     -4778     
- Partials      122     1750     +1628     

☔ 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.

[claude]

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

---

Code Review - Changes Since Last Review

• Read project documentation and guidelines
• Review changes since last Claude review
• Analyze all modified files
• Address open questions from human reviewers
• Provide comprehensive code review feedback

---

Summary

The most recent commit (3ee3974) addresses the previous finding about null user handling by changing NotFoundException to UnauthorizedAccessException in AccountsKeyManagementController.cs:192, which correctly aligns with the pattern used throughout the controller. The corresponding unit test was also updated.

However, one open question from @quexten remains unresolved regarding exception handling for the organization user check in KeyConnectorConfirmationDetailsQuery.cs:30.

---

Findings

Finding 1: 💭 Exception type for null organization user needs clarification

In KeyConnectorConfirmationDetailsQuery.cs:27-30, when GetByOrganizationAsync returns null, the code throws NotFoundException. @quexten raised a valid question about whether this should be UnauthorizedAccessException instead, since the user is not part of the organization.

Analysis of exception patterns in codebase

I examined other usages of GetByOrganizationAsync in the codebase:

1. OrganizationsController.cs:172-176 - Throws NotFoundException when orgUser is null
2. OrganizationUsersController.cs:479-480 - Assumes non-null and accesses .Status (would throw NullReferenceException if null)
3. UserDecryptionOptionsBuilder.cs:147 - Assumes non-null in subsequent logic

The pattern is inconsistent across the codebase, which makes this a judgment call. However, semantically:

• NotFoundException suggests "the resource you're looking for doesn't exist"
• UnauthorizedAccessException suggests "you don't have permission to access this"

For a user attempting to access an organization they're not a member of, UnauthorizedAccessException seems more semantically correct as it's an authorization issue, not a missing resource issue.

Recommendation: Consider changing line 30 to throw new UnauthorizedAccessException(); for semantic clarity. If keeping NotFoundException, please document the reasoning.

---

Good Practices Observed

• ✅ Comprehensive unit test coverage for the new endpoint and query
• ✅ Integration tests updated to cover the new functionality
• ✅ Proper dependency injection using TryAdd pattern
• ✅ Consistent with existing controller patterns
• ✅ Test addressed for user null check

---

Action Items

1. @Thomas-Avery: Address @quexten's question about the orgUser null check exception type in KeyConnectorConfirmationDetailsQuery.cs:30 - clarify whether NotFoundException or UnauthorizedAccessException is the intended behavior and update code/tests if needed

[quexten]

Nice, mostly looks good. My only concern is with the returned exception types, which seem a little bit unintutive as a consumer. If you feel that these are the right types to use, please at least provide an explanation why we are choosing not found over unauthorized, especially for user.