Skip to content

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

Open
quexten wants to merge 1 commit into
mainfrom
tmp-part-3
Open

feat: wire-in key id on registration and key rotation flows#8164
quexten wants to merge 1 commit into
mainfrom
tmp-part-3

Conversation

@quexten

@quexten quexten commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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.

@quexten
quexten requested review from a team as code owners August 7, 2026 11:28
@quexten
quexten marked this pull request as draft August 7, 2026 11:28
@quexten quexten added the t:feature Change Type - Feature Development label Aug 7, 2026
@quexten
quexten removed request for a team August 7, 2026 12:08
@quexten quexten changed the title Tmp part 3 feat: wire-in key id on registration and key rotation flows Aug 7, 2026
@quexten quexten added the ai-review Request a Claude code review label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🤖 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 Bot commented Aug 7, 2026

Copy link
Copy Markdown

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.

@quexten
quexten marked this pull request as ready for review August 7, 2026 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant