mirror of
https://github.com/bitwarden/server
synced 2026-01-03 09:03:44 +00:00
[PM-23249] Prevent log-out when changing KDF settings (#6349)
* Prevent log-out when changing KDF settings with feature flag. * validate salt unchanged for user to throw bad request (400), not internal server error (500) * change kdf integration tests * failing tests * iuncorrect tests wording * conditional logout * log out reason as enum * explicit naming
This commit is contained in:
@@ -1,4 +1,5 @@
|
||||
using Bit.Core.Entities;
|
||||
using Bit.Core.Enums;
|
||||
using Bit.Core.Exceptions;
|
||||
using Bit.Core.KeyManagement.Models.Data;
|
||||
using Bit.Core.Platform.Push;
|
||||
@@ -18,17 +19,22 @@ public class ChangeKdfCommand : IChangeKdfCommand
|
||||
private readonly IUserRepository _userRepository;
|
||||
private readonly IdentityErrorDescriber _identityErrorDescriber;
|
||||
private readonly ILogger<ChangeKdfCommand> _logger;
|
||||
private readonly IFeatureService _featureService;
|
||||
|
||||
public ChangeKdfCommand(IUserService userService, IPushNotificationService pushService, IUserRepository userRepository, IdentityErrorDescriber describer, ILogger<ChangeKdfCommand> logger)
|
||||
public ChangeKdfCommand(IUserService userService, IPushNotificationService pushService,
|
||||
IUserRepository userRepository, IdentityErrorDescriber describer, ILogger<ChangeKdfCommand> logger,
|
||||
IFeatureService featureService)
|
||||
{
|
||||
_userService = userService;
|
||||
_pushService = pushService;
|
||||
_userRepository = userRepository;
|
||||
_identityErrorDescriber = describer;
|
||||
_logger = logger;
|
||||
_featureService = featureService;
|
||||
}
|
||||
|
||||
public async Task<IdentityResult> ChangeKdfAsync(User user, string masterPasswordAuthenticationHash, MasterPasswordAuthenticationData authenticationData, MasterPasswordUnlockData unlockData)
|
||||
public async Task<IdentityResult> ChangeKdfAsync(User user, string masterPasswordAuthenticationHash,
|
||||
MasterPasswordAuthenticationData authenticationData, MasterPasswordUnlockData unlockData)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(user);
|
||||
if (!await _userService.CheckPasswordAsync(user, masterPasswordAuthenticationHash))
|
||||
@@ -37,8 +43,8 @@ public class ChangeKdfCommand : IChangeKdfCommand
|
||||
}
|
||||
|
||||
// Validate to prevent user account from becoming un-decryptable from invalid parameters
|
||||
//
|
||||
// Prevent a de-synced salt value from creating an un-decryptable unlock method
|
||||
//
|
||||
// Prevent a de-synced salt value from creating an un-decryptable unlock method
|
||||
authenticationData.ValidateSaltUnchangedForUser(user);
|
||||
unlockData.ValidateSaltUnchangedForUser(user);
|
||||
|
||||
@@ -47,12 +53,15 @@ public class ChangeKdfCommand : IChangeKdfCommand
|
||||
{
|
||||
throw new BadRequestException("KDF settings must be equal for authentication and unlock.");
|
||||
}
|
||||
|
||||
var validationErrors = KdfSettingsValidator.Validate(unlockData.Kdf);
|
||||
if (validationErrors.Any())
|
||||
{
|
||||
throw new BadRequestException("KDF settings are invalid.");
|
||||
}
|
||||
|
||||
var logoutOnKdfChange = !_featureService.IsEnabled(FeatureFlagKeys.NoLogoutOnKdfChange);
|
||||
|
||||
// Update the user with the new KDF settings
|
||||
// This updates the authentication data and unlock data for the user separately. Currently these still
|
||||
// use shared values for KDF settings and salt.
|
||||
@@ -68,7 +77,8 @@ public class ChangeKdfCommand : IChangeKdfCommand
|
||||
// This entire operation MUST be atomic to prevent a user from being locked out of their account.
|
||||
// Salt is ensured to be the same as unlock data, and the value stored in the account and not updated.
|
||||
// KDF is ensured to be the same as unlock data above and updated below.
|
||||
var result = await _userService.UpdatePasswordHash(user, authenticationData.MasterPasswordAuthenticationHash);
|
||||
var result = await _userService.UpdatePasswordHash(user, authenticationData.MasterPasswordAuthenticationHash,
|
||||
refreshStamp: logoutOnKdfChange);
|
||||
if (!result.Succeeded)
|
||||
{
|
||||
_logger.LogWarning("Change KDF failed for user {userId}.", user.Id);
|
||||
@@ -88,7 +98,17 @@ public class ChangeKdfCommand : IChangeKdfCommand
|
||||
user.LastKdfChangeDate = now;
|
||||
|
||||
await _userRepository.ReplaceAsync(user);
|
||||
await _pushService.PushLogOutAsync(user.Id);
|
||||
if (logoutOnKdfChange)
|
||||
{
|
||||
await _pushService.PushLogOutAsync(user.Id);
|
||||
}
|
||||
else
|
||||
{
|
||||
// Clients that support the new feature flag will ignore the logout when it matches the reason and the feature flag is enabled.
|
||||
await _pushService.PushLogOutAsync(user.Id, reason: PushNotificationLogOutReason.KdfChange);
|
||||
await _pushService.PushSyncSettingsAsync(user.Id);
|
||||
}
|
||||
|
||||
return IdentityResult.Success;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
using Bit.Core.Entities;
|
||||
using Bit.Core.Exceptions;
|
||||
|
||||
namespace Bit.Core.KeyManagement.Models.Data;
|
||||
|
||||
@@ -12,7 +13,7 @@ public class MasterPasswordAuthenticationData
|
||||
{
|
||||
if (user.GetMasterPasswordSalt() != Salt)
|
||||
{
|
||||
throw new ArgumentException("Invalid master password salt.");
|
||||
throw new BadRequestException("Invalid master password salt.");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
#nullable enable
|
||||
|
||||
using Bit.Core.Entities;
|
||||
using Bit.Core.Entities;
|
||||
using Bit.Core.Exceptions;
|
||||
|
||||
namespace Bit.Core.KeyManagement.Models.Data;
|
||||
|
||||
@@ -14,7 +13,7 @@ public class MasterPasswordUnlockData
|
||||
{
|
||||
if (user.GetMasterPasswordSalt() != Salt)
|
||||
{
|
||||
throw new ArgumentException("Invalid master password salt.");
|
||||
throw new BadRequestException("Invalid master password salt.");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user