[PM-39454] feat(km): reject unlock data whose user key id does not match the stored one

[quexten]

When changing the password or changing KDF settings, the user-key MUST NOT be changed. Thus, we add the key id in the unlock data so that the server can validate and assert it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.90%. Comparing base (dc7519e) to head (1dc7f99).

Additional details and impacted files

@@                      Coverage Diff                      @@
##           km/user-key-id-05-write-path    #8111   +/-   ##
=============================================================
  Coverage                         62.90%   62.90%           
=============================================================
  Files                              2304     2304           
  Lines                            100400   100412   +12     
  Branches                           9041     9043    +2     
=============================================================
+ Hits                              63152    63165   +13     
+ Misses                            35062    35061    -1     
  Partials                           2186     2186           

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

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR extends user-key-id validation to the KDF-change and key-rotation write paths: ChangeKdfCommand now asserts the unlock data's key id is unchanged, and BaseRotateUserAccountKeysData.ValidateContainedKeyIdMatches asserts the master-password unlock data names the same key as the rotation itself. The null-handling asymmetry (absent contained key id passes for pre-field clients; a contained key id with no rotation key id is rejected) is deliberate and covered by new unit tests, and KeyId defines value-based ==/!=, so the comparison behaves as intended. All rotation entry points validate before any persistence, and the new integration test confirms User.UserKeyId is left untouched on rejection. TDE and Key Connector rotation carry no unlock-data key id, so nothing is missing there.

Code Review Details

• ♻️ : New KDF-change key-id rejection has no negative test, unlike the adjacent salt checks
  • src/Core/KeyManagement/Kdf/Implementations/ChangeKdfCommand.cs:53
• ❓ : Rotation does not assert the request's key id differs from the stored one
  • src/Core/KeyManagement/UserKey/Models/Data/BaseRotateUserAccountKeysData.cs:38

[claude]

♻️ DEBT: No test covers the rejection this line adds, unlike the salt checks right above it.

Details and fix

ChangeKdfCommandTests has ChangeKdfAsync_AuthDataSaltMismatch_Throws and ChangeKdfAsync_UnlockDataSaltMismatch_Throws for the two duplicated salt checks, each asserting BadRequestException and SaveUpdateExistingKdfConfigurationAsync received 0 calls. Every existing test in that file builds MasterPasswordUnlockData without a UserKeyId, so the new call only ever takes the early-return path — deleting the line would keep the suite green.

Suggest mirroring the salt tests:

[Theory]
[BitAutoData]
public async Task ChangeKdfAsync_UnlockDataUserKeyIdMismatch_Throws(
    SutProvider<ChangeKdfCommand> sutProvider, User user, KdfSettings kdf)
{
    sutProvider.GetDependency<IUserService>().CheckPasswordAsync(Arg.Any<User>(), Arg.Any<string>())
        .Returns(true);
    // user.UserKeyId is set by KeyIdCustomization; send a different one.
    var unlockData = new MasterPasswordUnlockData
    {
        Kdf = kdf,
        MasterKeyWrappedUserKey = "new-wrapped-key",
        Salt = user.GetMasterPasswordSalt(),
        UserKeyId = KeyId.FromHexEncodedString("fedcba9876543210fedcba9876543210")
    };
    // ... authenticationData with matching salt/kdf
    await Assert.ThrowsAsync<BadRequestException>(() =>
        sutProvider.Sut.ChangeKdfAsync(user, "masterPassword", authenticationData, unlockData));
    await sutProvider.GetDependency<IMasterPasswordService>()
        .Received(0)
        .SaveUpdateExistingKdfConfigurationAsync(Arg.Any<User>(), Arg.Any<UpdateExistingKdfConfigurationData>());
}

[claude]

❓ QUESTION: Should a rotation also reject a key id that equals the one already stored for the account?

Details

This validates the unlock data against the request's own key id, but nothing compares the request's key id to user.GetUserKeyId(). A rotation replaces the user key, so a request that reuses the stored key id makes two distinct keys share one id — exactly the confusion key ids exist to prevent, and it would be persisted as-is by BaseRotateUserAccountKeysAsync (RotateUserAccountKeysCommand.cs:330).

ValidateForUser runs before that assignment in all four rotation entry points, so a baseModel.UserKeyId != user.GetUserKeyId() assertion would be cheap to add here. Is this deliberately deferred to a later PR in the stack, or is the reuse case considered a client-only concern?