[PM-39454] feat(km): expose the user key id on sync and add the backfill endpoint - #8112
[PM-39454] feat(km): expose the user key id on sync and add the backfill endpoint#8112quexten wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## km/user-key-id-06-validate-key-id #8112 +/- ##
=====================================================================
+ Coverage 62.88% 62.90% +0.02%
=====================================================================
Files 2304 2305 +1
Lines 100407 100426 +19
Branches 9041 9043 +2
=====================================================================
+ Hits 63137 63170 +33
+ Misses 35083 35069 -14
Partials 2187 2187 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7a89c6f to
f9f46d1
Compare
f9f46d1 to
da9b23f
Compare
quexten
added
ai-review
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the new Code Review Details
Previously raised threads remain addressed by the author (same-key-id 400 handled in the SDK; sync-field assertion thread resolved). Note: this PR targets the stacked branch |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
da9b23f to
fc604e4
Compare
…tch the stored one Now that the key id is recorded, use it. Two checks, for two different mistakes. ValidateUserKeyUnchangedForUser guards the flows that re-wrap an existing user key without replacing it — changing a master password, changing a KDF. If the client reports a key id that is not the one the account has, the client is working from a user key the server does not believe is current, and the write would leave the account wrapped under a key nothing else agrees on. Reject rather than persist that. ValidateContainedKeyIdMatches guards rotation, which does replace the user key. There the request carries the new key id, and the unlock data must name that same key rather than the one being retired. Both are no-ops when either side is absent, so accounts predating the key id and clients that do not send one are unaffected.
…ill endpoint Accounts that predate the key id have none stored, and the server cannot derive one — only the client knows which key it holds. Close that gap from both ends. Sync now returns the stored key id, so a client can tell whether the server already knows its key. When it does not, POST /accounts/key-management/user-key-id reports it. The backfill is deliberately fill-only: TrySetUserKeyIdAsync writes just when the column is still null, and the command turns a refusal into a 400 rather than silently ignoring it. Reporting a key id must never be able to rename the key an account is already known to use. Adds end-to-end coverage of the endpoint: successful backfill, the rejected second attempt, malformed input, and unauthenticated access.
fc604e4 to
ff87137
Compare
|
|
||
| user.Key = updateExistingData.MasterPasswordUnlock.MasterKeyWrappedUserKey; | ||
| ApplyKdfStateOnUser(user, updateExistingData.MasterPasswordUnlock.Kdf); | ||
| updateExistingData.MasterPasswordUnlock.ValidateUserKeyUnchangedForUser(user); |
There was a problem hiding this comment.
❌ CRITICAL: ValidateUserKeyUnchangedForUser does not exist — this does not compile, and the check is already performed above.
Details and fix
The only method on MasterPasswordUnlockData is ValidateUserKeyIdUnchangedForUser (src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs:42); ValidateUserKeyUnchangedForUser (no Id) is not defined anywhere in the repo, so this is a CS1061 build break. The failing Run tests check on this PR is consistent with that.
The line is also redundant: updateExistingData.ValidateDataForUser(user) on line 179 already calls MasterPasswordUnlock.ValidateUserKeyIdUnchangedForUser(user) (src/Core/Auth/UserFeatures/UserMasterPassword/Data/UpdateExistingKdfConfigurationData.cs:67), and it runs there before user.Key and the KDF state are mutated, which is the correct ordering for a validation. Commit c9360ddb already removed the identical stray call from PrepareUpdateExistingMasterPasswordAsync; this second occurrence appears to have been missed.
user.Key = updateExistingData.MasterPasswordUnlock.MasterKeyWrappedUserKey;
ApplyKdfStateOnUser(user, updateExistingData.MasterPasswordUnlock.Kdf);The test names in test/Core.Test/KeyManagement/Models/Data/MasterPasswordUnlockDataTests.cs also use the ValidateUserKeyUnchangedForUser_* prefix while invoking ValidateUserKeyIdUnchangedForUser, suggesting a rename that was only half applied — worth aligning those names in whichever direction you settle on.
| [Fact] | ||
| public void ValidateUserKeyIdUnchangedForUser_Accepts_WhenStoredKeyIdIsEmptyString() | ||
| public void ValidateUserKeyUnchangedForUser_WhenServerHasNoKeyId_DoesNotThrow() | ||
| { | ||
| // An empty column value is treated as unset rather than as a malformed key id, so a legacy | ||
| // row does not fail every password change. | ||
| var user = BuildUser(string.Empty); | ||
|
|
||
| BuildUnlockData(DifferentKeyId).ValidateUserKeyIdUnchangedForUser(user); | ||
| // Backfill case: nothing to compare against yet. | ||
| MakeData(_keyIdA).ValidateUserKeyIdUnchangedForUser(MakeUser(null)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ValidateUserKeyIdUnchangedForUser_DoesNotMutateTheUser() | ||
| public void ValidateUserKeyUnchangedForUser_WhenRequestOmitsKeyId_DoesNotThrow() | ||
| { | ||
| var user = BuildUser(StoredKeyId); | ||
|
|
||
| BuildUnlockData(StoredKeyId).ValidateUserKeyIdUnchangedForUser(user); | ||
|
|
||
| Assert.Equal(StoredKeyId, user.UserKeyId); | ||
| // Clients that predate the field must keep working. | ||
| MakeData(null).ValidateUserKeyIdUnchangedForUser(MakeUser(_keyIdA)); | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: The rewrite drops the only test covering the empty-string key id column, a branch that is still deliberately handled in production code.
Details and fix
User.GetUserKeyId() (src/Core/Entities/User.cs:137) treats an empty UserKeyId column as unset rather than as a malformed key id:
public KeyId? GetUserKeyId() =>
KeyId.FromHexEncodedString(string.IsNullOrEmpty(UserKeyId) ? null : UserKeyId);That guard is what keeps a legacy row with '' from failing every password change through ValidateUserKeyIdUnchangedForUser. The deleted ValidateUserKeyIdUnchangedForUser_Accepts_WhenStoredKeyIdIsEmptyString test was the only coverage of it — grepping GetUserKeyId across test/ now returns a single unrelated assertion in RegisterFinishRequestModelTests, so the branch is untested after this PR.
Re-adding the case keeps the defensive behavior pinned:
[Fact]
public void ValidateUserKeyIdUnchangedForUser_WhenStoredKeyIdIsEmptyString_DoesNotThrow()
{
// An empty column value is treated as unset, so a legacy row does not fail every password change.
MakeData(_keyIdA).ValidateUserKeyIdUnchangedForUser(MakeUser(string.Empty));
}
1 participant
Add the user id on sync in the user decryption options, and add the endpoint for back-filling it for v1 users.