[PM-39454] feat(km): add UserKeyId column to the user row by quexten · Pull Request #8108 · bitwarden/server

quexten · 2026-07-31T11:59:41Z

Adds the user-id column, which is the authoritative ID of the user key. The column contains 32 characters (16 bytes but in hex).

codecov · 2026-07-31T12:25:33Z

Codecov Report

❌ Patch coverage is 22.22222% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.37%. Comparing base (9f5f367) to head (b17a1f0).

Files with missing lines	Patch %	Lines
src/Core/Entities/User.cs	20.00%	4 Missing ⚠️
...frastructure.Dapper/Repositories/UserRepository.cs	0.00%	2 Missing ⚠️
...ure.EntityFramework/Repositories/UserRepository.cs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##           km/user-key-id-02-keyid-type    #8108      +/-   ##
================================================================
+ Coverage                         62.89%   67.37%   +4.47%     
================================================================
  Files                              2303     2303              
  Lines                            100279   100288       +9     
  Branches                           9030     9032       +2     
================================================================
+ Hits                              63071    67569    +4498     
+ Misses                            35023    30438    -4585     
- Partials                           2185     2281      +96

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

github-actions · 2026-07-31T12:38:20Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the addition of the UserKeyId column across both ORM tracks: the SSDT table/view/procedure changes, the dated MSSQL migrations, the three generated EF migrations and snapshots, and the Dapper/EF repository write paths. Dual-ORM parity holds — UpdateUserKeyAndEncryptedDataAsync and UpdateUserKeyAndEncryptedDataV2Async both persist the value on all four databases, and SetV2AccountCryptographicStateAsync correctly leaves it alone since that path does not touch the user key. The migration scripts match the established pattern used by 2026-05-13_00_AlterUserAddLastApiKeyRotationDate.sql (column appended last, COL_LENGTH guard, CREATE OR ALTER view identical to src/Sql/dbo/Views/UserView.sql, same sp_refreshview set, new procedure parameters defaulted to NULL for rolling-deploy compatibility). No new findings.

Code Review Details

No new findings.

Context on the two open items from earlier review rounds:

The VARCHAR(64) question is resolved — column, @UserKeyId parameters, EF migrations, and [MaxLength(32)] are all aligned at 32 characters, matching KeyId.FromHexEncodedString. The KeyIdCustomization.HexEncodedKeyId fixture value is exactly 32 characters, so it fits the narrowed column.
Re AccountRevisionDate = DateTime.UtcNow.AddMinutes(-10) in OrganizationTestHelpers.CreateTestUserAsync ("not sure why this was necessary"): the created value comes from the app process clock (DateTime.UtcNow, 100 ns resolution), while User_BumpAccountRevisionDate writes GETUTCDATE(), which is a datetime with ~3.33 ms granularity taken from the SQL Server container's clock. Assertions like Assert.True(actualUser.AccountRevisionDate.CompareTo(user.AccountRevisionDate) > 0) in OrganizationUserReplaceTests can therefore fail on rounding or host/DB clock skew. Back-dating the creation timestamp makes the comparison deterministic, so the change is a reasonable guard rather than a workaround for something in this PR.

Informational, not reopening the resolved thread: SetUserKeyId/GetUserKeyId in src/Core/Entities/User.cs still have no unit coverage in test/Core.Test/Entities/UserTests.cs; the round trip is exercised only by the new integration tests.

quexten · 2026-07-31T13:34:09Z

            Email = $"{id}@example.com",
            ApiKey = "TEST",
            SecurityStamp = "stamp",
+            AccountRevisionDate = DateTime.UtcNow.AddMinutes(-10),


Not sure why this was necessary, but it was making tests fail 🤷

@quexten This is a pre-existing flake, not caused by your PR. User_BumpAccountRevisionDate uses GETUTCDATE(), which has about 3.3 ms precision, so on a fast run it can land before the full-precision DateTime.UtcNow set during creation. The same strict > assertion at OrganizationUserReplaceTests.cs:90 failed on main in run 30025134779, so backdating the test value here is a valid fix.

The real fix is to update the ten User_BumpAccountRevisionDate* procedures to use SYSUTCDATETIME(), the datetime2(7)-precision equivalent, but that requires a migration.

Adds the nullable UserKeyId column to [dbo].[User] and its view, and threads it through the three procedures that write a user row wholesale: User_Create, User_Update and User_UpdateKeys. The column is nullable and every new procedure parameter defaults to NULL, so an older server that omits it keeps working during a rolling deploy. User_UpdateKeys assigns the column unconditionally, so both rotation paths have to supply the value. The Dapper path passes @UserKeyId; the Entity Framework path was not assigning it at all, which would have left a stale key id behind on the non-MSSQL providers once rotations start carrying one. Both now behave the same. KeyIdCustomization teaches AutoFixture to build a KeyId — it has a private constructor — and pins User.UserKeyId to valid hex so that any test whose subject reads the id does not throw on a random string. It is applied to every BitAutoData fixture because users are generated throughout the suite. Nothing sets a key id yet; this only gives it somewhere to live.

mzieniukbw · 2026-07-31T17:18:34Z

+    [MaxLength(32)]
+    public string? UserKeyId { get; set; }
+
+    public void SetUserKeyId(KeyId userKeyId)


Since KeyId might come null and I assume we will use this function from command, i think it make sense to make it nullable too.

Suggested change

public void SetUserKeyId(KeyId userKeyId)

public void SetUserKeyId(KeyId? userKeyId)

mzieniukbw · 2026-07-31T17:18:43Z

+
+    public void SetUserKeyId(KeyId userKeyId)
+    {
+        UserKeyId = userKeyId.ToString();


Suggested change

UserKeyId = userKeyId.ToString();

UserKeyId = userKeyId?.ToString();

mzieniukbw · 2026-07-31T17:19:19Z

+    /// A key rotation will set a new key id. Account registrations will carry a key id.
+    /// </summary>
+    [MaxLength(32)]
+    public string? UserKeyId { get; set; }


Should we make it more restrictive ?

Suggested change

public string? UserKeyId { get; set; }

public string? UserKeyId { get; private set; }

rkac-bw · 2026-07-31T23:00:03Z

+        [V2UpgradeToken] = @V2UpgradeToken,
+        [MasterPasswordSalt] = @MasterPasswordSalt,
+        [LastApiKeyRotationDate] = @LastApiKeyRotationDate,
+        [UserKeyId] = @UserKeyId


Just confirming the rollout order here: the intent is for #8110 to ship at least one release after this change, correct? Older binaries will omit @UserKeyId, so landing both together would allow User_Update to overwrite an existing value with NULL during deployment or rollback. If they do need to land together, [UserKeyId] = COALESCE(@UserKeyId, [UserKeyId]) closes that compatibility gap — same applies to User_UpdateKeys below, and the SSDT copies need to match either way.

One addition, and one thing to confirm.

Release ordering doesn’t fully close this on its own. #8112 adds a backfill endpoint that writes UserKeyId out of band, so a request holding a User loaded before the backfill ran could still null the column on its next ReplaceAsync, even with no version skew. Since actual writes go through User_SetUserKeyId / User_TrySetUserKeyId, having User_Update preserve the existing value rather than assign NULL closes both cases.

quexten added the ai-review Request a Claude code review label Jul 31, 2026

claude Bot reviewed Jul 31, 2026

View reviewed changes

Comment thread src/Sql/dbo/Tables/User.sql Outdated

claude Bot reviewed Jul 31, 2026

View reviewed changes

Comment thread src/Core/Entities/User.cs

quexten added the t:feature Change Type - Feature Development label Jul 31, 2026

quexten force-pushed the km/user-key-id-03-userkeyid-column branch from 30f5cea to a251250 Compare July 31, 2026 13:33

quexten commented Jul 31, 2026

View reviewed changes

quexten marked this pull request as ready for review July 31, 2026 15:14

quexten requested review from a team as code owners July 31, 2026 15:14

quexten requested a review from JaredScar July 31, 2026 15:14

quexten added 3 commits August 1, 2026 00:17

fix tests

4cc036e

Fix column size

b17a1f0

quexten requested a review from a team as a code owner July 31, 2026 15:20

quexten requested a review from mzieniukbw July 31, 2026 15:20

quexten force-pushed the km/user-key-id-03-userkeyid-column branch from a251250 to b17a1f0 Compare July 31, 2026 15:20

mzieniukbw requested changes Jul 31, 2026

View reviewed changes

rkac-bw reviewed Jul 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[PM-39454] feat(km): add UserKeyId column to the user row - #8108

[PM-39454] feat(km): add UserKeyId column to the user row#8108
quexten wants to merge 3 commits into
km/user-key-id-02-keyid-typefrom
km/user-key-id-03-userkeyid-column

quexten commented Jul 31, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

quexten Jul 31, 2026

Uh oh!

rkac-bw Jul 31, 2026

Uh oh!

mzieniukbw Jul 31, 2026 •

edited

Loading

Uh oh!

mzieniukbw Jul 31, 2026

Uh oh!

mzieniukbw Jul 31, 2026 •

edited

Loading

Uh oh!

rkac-bw Jul 31, 2026

Uh oh!

rkac-bw Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	public void SetUserKeyId(KeyId userKeyId)
	public void SetUserKeyId(KeyId? userKeyId)

	UserKeyId = userKeyId.ToString();
	UserKeyId = userKeyId?.ToString();

	public string? UserKeyId { get; set; }
	public string? UserKeyId { get; private set; }

Uh oh!

Conversation

quexten commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Bitwarden Claude Code Review

Uh oh!

Uh oh!

Uh oh!

quexten Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

rkac-bw Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

mzieniukbw Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzieniukbw Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

mzieniukbw Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkac-bw Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

rkac-bw Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

quexten commented Jul 31, 2026 •

edited

Loading

codecov Bot commented Jul 31, 2026 •

edited

Loading

github-actions Bot commented Jul 31, 2026 •

edited

Loading

mzieniukbw Jul 31, 2026 •

edited

Loading

mzieniukbw Jul 31, 2026 •

edited

Loading