feat: wire-in key id on registration and key rotation flows

[quexten]

Wire in the different Key-Id flows to write the key id. A new endpoint is added for backfilling the key id, and the v2 registration flows gain the key id as an optional property, that is set if presented. Further, the key rotation endpoints gain it as an optional property.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR wires the user-key id through V2 registration finish, POST /accounts/keys, key connector setup, SSO JIT master-password provisioning, and all four key rotation flows, replaces the User.GetUserKeyId()/SetUserKeyId() stubs with real column access, and adds a set-once POST /accounts/key-management/user-key-id backfill endpoint. I traced every SetV2AccountCryptographicStateAsync call site and confirmed the SetUserKeyId delegate is always ordered after the delegate that performs the broader user write, that rotation mutates the same User instance later persisted by UpdateUserKeyAndEncryptedDataV2Async (User_Update and the EF entity both carry UserKeyId), and that every request-model entry point carrying a key id is annotated with [KeyId], so KeyId.FromHexEncodedString cannot throw past the ModelStateValidationFilterAttribute registered for all API controllers via PublicApiControllersModelConvention. The two MasterPasswordUnlockData key-id validators become live now that GetUserKeyId() reads the column; both call sites (SelfServicePasswordChangeCommand, ChangeKdfCommand) are key-preserving flows where "unchanged" is the correct semantic, while rotation instead validates ContainedKeyId against NewUserKeyId before BaseRotateUserAccountKeysAsync overwrites it. V1 paths deliberately ignore the field and are commented as such, and the intentional clear-on-rotation behaviour for pre-field clients is covered by an explicit test.

Code Review Details

No blocking findings.

• ❓ : SetUserKeyIdCommand checks user.GetUserKeyId() in application code and then issues an unconditional User_SetUserKeyId UPDATE, so the set-once invariant is not enforced at the database level — a backfill racing a concurrent key rotation could persist a stale key id, which would then reject legitimate password/KDF changes until the next rotation.
  • src/Core/KeyManagement/Commands/SetUserKeyIdCommand.cs:19
  • Very narrow window, self-healing on the next rotation, and a DB-level guard would need a separate procedure since the same one serves the unconditional registration/rotation paths — noting rather than requesting a change.

[codecov]

Codecov Report

❌ Patch coverage is 92.85714% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.53%. Comparing base (73ed2f6) to head (eb43f28).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Core/Services/Implementations/UserService.cs	55.55%	3 Missing and 1 partial ⚠️
...re/KeyManagement/Models/Data/RegisterFinishData.cs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #8164       +/-   ##
===========================================
+ Coverage   15.10%   67.53%   +52.42%     
===========================================
  Files        1417     2317      +900     
  Lines       61422   100561    +39139     
  Branches     4901     9051     +4150     
===========================================
+ Hits         9279    67911    +58632     
+ Misses      51978    30358    -21620     
- Partials      165     2292     +2127     

☔ View full report in Codecov by Harness.
📢 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.

[mzieniukbw]

❓ Why do we need this on entity ? In other cases, this is used by EF to generate correct column types, but not here.

[quexten]

Makes sense, I'll remove it in a follow-up PR given this is already approved.

[mzieniukbw]

❓ Why not name it, for what it is: UserKeyId ?

[quexten]

On safe primitives, the value is called ContainedKeyId. Eventually, when we replace the kc-keywrapped-user-key with a safe primitive version, the value is called, we will drop this value and use the contained key id that is in the kc-wrapped-user-key:
https://github.com/bitwarden/sdk-internal/blob/23383b7c0ac01667a0ef78257230c1ceb030b07c/crates/bitwarden-crypto/src/safe/password_protected_key_envelope.rs#L273

Essentially, in the current state this is that same value, just not placed on the wrapped key object.

[mzieniukbw]

❓ Why not name it, for what it is: UserKeyId ?

[quexten]

(See other comment)

[ike-kottlowski]

Just a question about model ownership

[ike-kottlowski]

🎨 : do we want to move this to KM ownership?

[quexten]

I agree, except that we use it for a sub request model, but also a top level request model for JIT crypto init

server/src/Api/Auth/Controllers/AccountsController.cs

Line 509 in 2a5fa8b

public async Task<KeysResponseModel> PostKeys([FromBody] KeysRequestModel model)

. We should split this, and auth should still own a JIT crypto init request.

Do you agree with this split @ike-kottlowski ?