From 3d03bb4c1f918a42e991be480a85cfff98c04e22 Mon Sep 17 00:00:00 2001 From: Jared Snider Date: Thu, 11 Jun 2026 13:47:53 -0400 Subject: [PATCH 1/3] PM-38811 - Persist LastPasswordChangeDate from EF user-key rotation write UpdateUserKeyAndEncryptedDataV2Async enumerated a fixed column list and omitted LastPasswordChangeDate, silently dropping the field on PostgreSQL/MySQL/SQLite even when callers set it. The MSSQL sproc User_Update already persists this column, so this aligns EF with the existing Dapper behavior. --- .../Repositories/UserRepository.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Infrastructure.EntityFramework/Repositories/UserRepository.cs b/src/Infrastructure.EntityFramework/Repositories/UserRepository.cs index dab556e72f0c..4cafd79d0564 100644 --- a/src/Infrastructure.EntityFramework/Repositories/UserRepository.cs +++ b/src/Infrastructure.EntityFramework/Repositories/UserRepository.cs @@ -250,6 +250,7 @@ public async Task UpdateUserKeyAndEncryptedDataV2Async(Core.Entities.User user, userEntity.MasterPassword = user.MasterPassword; userEntity.MasterPasswordHint = user.MasterPasswordHint; + userEntity.LastPasswordChangeDate = user.LastPasswordChangeDate; userEntity.LastKeyRotationDate = user.LastKeyRotationDate; userEntity.AccountRevisionDate = user.AccountRevisionDate; userEntity.RevisionDate = user.RevisionDate; From 20b523af618d6c2bc481eddccab25602f56efa36 Mon Sep 17 00:00:00 2001 From: Jared Snider Date: Thu, 11 Jun 2026 13:48:14 -0400 Subject: [PATCH 2/3] PM-38811 - Delegate password-change rotation to MasterPasswordService Wires PasswordChangeAndRotateUserAccountKeysAsync to IMasterPasswordService.PrepareUpdateExistingMasterPasswordAsync (Prepare* tier from PM-35392), replacing the inline master password mutation block. RefreshStamp is false so the existing BaseRotateUserAccountKeysAsync SecurityStamp + V2UpgradeToken logic remains the sole owner of session-invalidation behavior. The hint is sourced from the request because a password change can update it. Closes the parity gap where LastPasswordChangeDate was not set on this path even though the master password is changing. Other rotation variants (master-password-only, TDE, Key Connector) are untouched. Unit tests cover delegation, OneOf failure mapping, and short-circuit on old-password mismatch. --- .../RotateUserAccountKeysCommand.cs | 31 +++++-- .../RotateUserAccountKeysCommandTests.cs | 86 ++++++++++++++++++- 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountKeysCommand.cs b/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountKeysCommand.cs index 742c92aeae36..3b3cb4351832 100644 --- a/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountKeysCommand.cs +++ b/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountKeysCommand.cs @@ -2,6 +2,8 @@ #nullable disable using Bit.Core.Auth.Repositories; +using Bit.Core.Auth.UserFeatures.UserMasterPassword.Data; +using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.KeyManagement.Repositories; @@ -30,7 +32,7 @@ public class RotateUserAccountKeysCommand : IRotateUserAccountKeysCommand private readonly IPushNotificationService _pushService; private readonly IdentityErrorDescriber _identityErrorDescriber; private readonly IWebAuthnCredentialRepository _credentialRepository; - private readonly IPasswordHasher _passwordHasher; + private readonly IMasterPasswordService _masterPasswordService; private readonly IUserSignatureKeyPairRepository _userSignatureKeyPairRepository; /// @@ -44,7 +46,7 @@ public class RotateUserAccountKeysCommand : IRotateUserAccountKeysCommand /// Provides a method to update re-encrypted emergency access data /// Provides a method to update re-encrypted organization user data /// Provides a method to update re-encrypted device keys - /// Hashes the new master password + /// Applies master password mutations (hash, key, hint, time markers) on the password-change rotation path /// Logs out user from other devices after successful rotation /// Provides a password mismatch error if master password hash validation fails /// Provides a method to update re-encrypted WebAuthn keys @@ -53,7 +55,7 @@ public RotateUserAccountKeysCommand(IUserService userService, IUserRepository us ICipherRepository cipherRepository, IFolderRepository folderRepository, ISendRepository sendRepository, IEmergencyAccessRepository emergencyAccessRepository, IOrganizationUserRepository organizationUserRepository, IDeviceRepository deviceRepository, - IPasswordHasher passwordHasher, + IMasterPasswordService masterPasswordService, IPushNotificationService pushService, IdentityErrorDescriber errors, IWebAuthnCredentialRepository credentialRepository, IUserSignatureKeyPairRepository userSignatureKeyPairRepository) { @@ -68,7 +70,7 @@ public RotateUserAccountKeysCommand(IUserService userService, IUserRepository us _pushService = pushService; _identityErrorDescriber = errors; _credentialRepository = credentialRepository; - _passwordHasher = passwordHasher; + _masterPasswordService = masterPasswordService; _userSignatureKeyPairRepository = userSignatureKeyPairRepository; } @@ -90,9 +92,24 @@ public async Task PasswordChangeAndRotateUserAccountKeysAsync(Us List saveEncryptedDataActions = []; var shouldPersistV2UpgradeToken = await BaseRotateUserAccountKeysAsync(model.BaseData, user, saveEncryptedDataActions); - user.Key = model.MasterPasswordUnlockData.MasterKeyWrappedUserKey; - user.MasterPassword = _passwordHasher.HashPassword(user, model.MasterPasswordAuthenticationData.MasterPasswordAuthenticationHash); - user.MasterPasswordHint = model.MasterPasswordHint; + // Delegate the master password mutation (hash, wrapped user key, hint, time markers) to + // MasterPasswordService. + // - RefreshStamp = false: BaseRotateUserAccountKeysAsync already owns SecurityStamp rotation + // with V2-upgrade-token-aware logic; the service must not double-handle it. + // - MasterPasswordHint populated from the request because a password change can update the hint. + var updatePasswordData = new UpdateExistingPasswordData + { + MasterPasswordAuthentication = model.MasterPasswordAuthenticationData, + MasterPasswordUnlock = model.MasterPasswordUnlockData, + ValidatePassword = true, + RefreshStamp = false, + MasterPasswordHint = model.MasterPasswordHint, + }; + var preparedResult = await _masterPasswordService.PrepareUpdateExistingMasterPasswordAsync(user, updatePasswordData); + if (preparedResult.TryPickT1(out var errors, out _)) + { + return IdentityResult.Failed(errors); + } await _userRepository.UpdateUserKeyAndEncryptedDataV2Async(user, saveEncryptedDataActions); diff --git a/test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs b/test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs index 46acfe9e3795..5d04dda75998 100644 --- a/test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs +++ b/test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs @@ -1,4 +1,6 @@ -using Bit.Core.Entities; +using Bit.Core.Auth.UserFeatures.UserMasterPassword.Data; +using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces; +using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; using Bit.Core.KeyManagement.Enums; @@ -19,6 +21,7 @@ using Microsoft.AspNetCore.Identity; using NSubstitute; using NSubstitute.ReturnsExtensions; +using OneOf; using Xunit; namespace Bit.Core.Test.KeyManagement.UserKey; @@ -593,6 +596,87 @@ await sutProvider.GetDependency().Received(1) .PushLogOutAsync(user.Id); } + [Theory, BitAutoData] + public async Task PasswordChangeAndRotateUserAccountKeysAsync_DelegatesToMasterPasswordService( + SutProvider sutProvider, User user, PasswordChangeAndRotateUserAccountKeysData model) + { + // Arrange + SetTestKdfAndSaltForUserAndModel(user, model); + var signatureRepository = sutProvider.GetDependency(); + SetV1ExistingUser(user, signatureRepository); + SetV1ModelUser(model.BaseData); + + sutProvider.GetDependency() + .CheckPasswordAsync(user, model.OldMasterKeyAuthenticationHash) + .Returns(true); + sutProvider.GetDependency() + .PrepareUpdateExistingMasterPasswordAsync(user, Arg.Any()) + .Returns(OneOf.FromT0(user)); + + // Act + var result = await sutProvider.Sut.PasswordChangeAndRotateUserAccountKeysAsync(user, model); + + // Assert - delegation occurred with correct data shape + Assert.Equal(IdentityResult.Success, result); + await sutProvider.GetDependency().Received(1) + .PrepareUpdateExistingMasterPasswordAsync(user, Arg.Is(d => + d.MasterPasswordAuthentication == model.MasterPasswordAuthenticationData && + d.MasterPasswordUnlock == model.MasterPasswordUnlockData && + d.MasterPasswordHint == model.MasterPasswordHint && + d.ValidatePassword == true && + d.RefreshStamp == false)); + } + + [Theory, BitAutoData] + public async Task PasswordChangeAndRotateUserAccountKeysAsync_MasterPasswordServiceFails_ReturnsFailedAndSkipsPersist( + SutProvider sutProvider, User user, PasswordChangeAndRotateUserAccountKeysData model) + { + // Arrange + SetTestKdfAndSaltForUserAndModel(user, model); + var signatureRepository = sutProvider.GetDependency(); + SetV1ExistingUser(user, signatureRepository); + SetV1ModelUser(model.BaseData); + + sutProvider.GetDependency() + .CheckPasswordAsync(user, model.OldMasterKeyAuthenticationHash) + .Returns(true); + + var serviceError = new IdentityError { Code = "SomeError", Description = "Service rejected" }; + sutProvider.GetDependency() + .PrepareUpdateExistingMasterPasswordAsync(user, Arg.Any()) + .Returns(OneOf.FromT1(new[] { serviceError })); + + // Act + var result = await sutProvider.Sut.PasswordChangeAndRotateUserAccountKeysAsync(user, model); + + // Assert - failure surfaced and side effects skipped + Assert.False(result.Succeeded); + Assert.Contains(result.Errors, e => e.Code == "SomeError"); + await sutProvider.GetDependency().DidNotReceive() + .UpdateUserKeyAndEncryptedDataV2Async(Arg.Any(), Arg.Any>()); + await sutProvider.GetDependency().DidNotReceive() + .PushLogOutAsync(Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Theory, BitAutoData] + public async Task PasswordChangeAndRotateUserAccountKeysAsync_WrongOldPassword_DoesNotCallMasterPasswordService( + SutProvider sutProvider, User user, PasswordChangeAndRotateUserAccountKeysData model) + { + // Arrange + user.Email = model.MasterPasswordUnlockData.Salt; + sutProvider.GetDependency() + .CheckPasswordAsync(user, model.OldMasterKeyAuthenticationHash) + .Returns(false); + + // Act + var result = await sutProvider.Sut.PasswordChangeAndRotateUserAccountKeysAsync(user, model); + + // Assert - rejection short-circuits before the MP service + Assert.False(result.Succeeded); + await sutProvider.GetDependency().DidNotReceive() + .PrepareUpdateExistingMasterPasswordAsync(Arg.Any(), Arg.Any()); + } + [Theory] [BitAutoData] public async Task MasterPasswordRotateUserAccountKeysAsync_MissingUser_Throws( From 5cb9ab273069c6570167dc18e110e1f9f5006868 Mon Sep 17 00:00:00 2001 From: Jared Snider Date: Thu, 11 Jun 2026 13:48:40 -0400 Subject: [PATCH 3/3] PM-38811 - Assert parity in password-change rotation integration test Extends the existing happy-path integration test to verify the master-key-wrapped user key, master password hint, master password hash (rewritten and verifies against the new authentication hash), and LastPasswordChangeDate are persisted as expected after a password-change-and-rotate call. --- .../AccountsKeyManagementControllerTests.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs b/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs index 20c56f7c5e44..6ac0a9b9c5a9 100644 --- a/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs +++ b/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs @@ -191,6 +191,7 @@ public async Task PasswordChangeAndRotateUserAccountKeysAsync_Success(RotateUser SetupRotateUserAccountUnlockData(request, user); SetupRotateUserAccountData(request); SetupRotateUserAccountKeys(request, isV2Crypto: false); + var originalMasterPassword = user.MasterPassword; var response = await _client.PostAsJsonAsync("/accounts/key-management/rotate-user-account-keys", request); var responseMessage = await response.Content.ReadAsStringAsync(); @@ -203,6 +204,23 @@ public async Task PasswordChangeAndRotateUserAccountKeysAsync_Success(RotateUser Assert.Equal(request.AccountUnlockData.MasterPasswordUnlockData.KdfIterations, userNewState.KdfIterations); Assert.Equal(request.AccountUnlockData.MasterPasswordUnlockData.KdfMemory, userNewState.KdfMemory); Assert.Equal(request.AccountUnlockData.MasterPasswordUnlockData.KdfParallelism, userNewState.KdfParallelism); + + // Parity preservation: master-key-wrapped user key and hint are applied from the request + Assert.Equal(request.AccountUnlockData.MasterPasswordUnlockData.MasterKeyEncryptedUserKey, userNewState.Key); + Assert.Equal(request.AccountUnlockData.MasterPasswordUnlockData.MasterPasswordHint, userNewState.MasterPasswordHint); + + // Parity preservation: master password hash is rewritten and verifies against the new authentication hash + Assert.NotEqual(originalMasterPassword, userNewState.MasterPassword); + Assert.Equal( + PasswordVerificationResult.Success, + _passwordHasher.VerifyHashedPassword( + userNewState, + userNewState.MasterPassword!, + request.AccountUnlockData.MasterPasswordUnlockData.MasterKeyAuthenticationHash)); + + // PM-38811 parity gap closer: LastPasswordChangeDate is set on this path via MasterPasswordService + Assert.NotNull(userNewState.LastPasswordChangeDate); + Assert.Equal(DateTime.UtcNow, userNewState.LastPasswordChangeDate.Value, TimeSpan.FromMinutes(1)); } [Theory]