[PM-29161] Remove ReturnErrorOnExistingKeypair feature flag

[Thomas-Avery]

🎟️ Tracking

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

📔 Objective

Remove the feature flag ReturnErrorOnExistingKeypair. This feature flag was server side only and wasn't used in clients.

It has been on in production since May 21 2024.

Added some unit test coverage for the flagged controller.

⏰ 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 – 0ebc2651-0985-4071-89e9-84f06903c321

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.85%. Comparing base (e3d5406) to head (3c330c2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6726      +/-   ##
==========================================
+ Coverage   53.83%   53.85%   +0.02%     
==========================================
  Files        1917     1917              
  Lines       85101    85098       -3     
  Branches     7613     7612       -1     
==========================================
+ Hits        45813    45831      +18     
+ Misses      37527    37504      -23     
- Partials     1761     1763       +2     

☔ 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

---

PR Review Complete

Todo List

• Read changed files to understand the scope
• Run bitwarden-code-reviewer agent to analyze the PR
• Complete review

---

Review Summary

This PR removes the ReturnErrorOnExistingKeypair feature flag and makes the error-on-existing-keypair behavior the default. The feature flag removal is complete and clean.

Key Findings

🔴 Critical: Potential Breaking Change

The new controller logic may introduce a breaking change in API behavior:

• Before (with feature flag): The KeysRequestModel.ToUser() method allowed idempotent calls - users could call PostKeys multiple times with identical keys, and subsequent calls would succeed using FixedTimeEquals comparison
• After (flag removed): The controller now throws BadRequestException for ANY existing keypair, preventing idempotent calls entirely

Recommendation:

1. Clarify if this breaking change is intentional and document it in the PR description
2. If idempotent behavior should be preserved, the controller check needs to allow same-key scenarios
3. Add a test case for the idempotent scenario (calling PostKeys with exact same keys that already exist)

Positive Observations

✅ Feature flag completely removed from codebase
✅ Clean refactoring with proper logic unnesting
✅ Good test coverage for unauthorized access and various key scenarios
✅ No security issues introduced
✅ Follows Bitwarden code standards (no regions, proper error handling)

Action Items

1. Clarify intended behavior regarding idempotent calls with identical keys
2. Add test case for the idempotent scenario if needed
3. Document as breaking change if idempotent behavior is intentionally removed

---