Skip to content

Commit

Permalink
PT-12375: Consistent password verification in the SecurityController (#…
Browse files Browse the repository at this point in the history 
…2659)

fix: Consistent password verification in the SecurityController
  • Loading branch information
@alexeyshibanov @OlegoO
alexeyshibanov authored and OlegoO committed May 31, 2023
1 parent 771ef5f commit ea7dc78
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 40 deletions.
29 changes: 22 additions & 7 deletions src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ public class SecurityController : Controller
private readonly IPermissionsRegistrar _permissionsProvider;
private readonly IUserSearchService _userSearchService;
private readonly IRoleSearchService _roleSearchService;
private readonly IPasswordValidator<ApplicationUser> _passwordValidator;
private readonly IEmailSender _emailSender;
private readonly IEventPublisher _eventPublisher;
private readonly IUserApiKeyService _userApiKeyService;
Expand All @@ -58,7 +57,6 @@ public class SecurityController : Controller
IOptions<UserOptionsExtended> userOptionsExtended,
IOptions<PasswordOptionsExtended> passwordOptions,
IOptions<PasswordLoginOptions> passwordLoginOptions,
IPasswordValidator<ApplicationUser> passwordValidator,
IEmailSender emailSender,
IEventPublisher eventPublisher,
IUserApiKeyService userApiKeyService,
Expand All @@ -69,7 +67,6 @@ public class SecurityController : Controller
_securityOptions = securityOptions.Value;
_userOptionsExtended = userOptionsExtended.Value;
_passwordOptions = passwordOptions.Value;
_passwordValidator = passwordValidator;
_passwordLoginOptions = passwordLoginOptions.Value ?? new PasswordLoginOptions();
_permissionsProvider = permissionsProvider;
_roleManager = roleManager;
Expand Down Expand Up @@ -634,11 +631,12 @@ public async Task<ActionResult> RequestPasswordReset(string loginOrEmail)
public async Task<ActionResult<IdentityResult>> ValidatePassword([FromBody] string password)
{
ApplicationUser user = null;
if (User.Identity.IsAuthenticated)
if (User.Identity?.IsAuthenticated == true)
{
user = await UserManager.FindByNameAsync(User.Identity.Name);
}
var result = await _passwordValidator.ValidateAsync(UserManager, user, password);

var result = await ValidatePassword(user, password);

return Ok(result);
}
Expand All @@ -659,13 +657,12 @@ public async Task<ActionResult<IdentityResult>> ValidateUserPassword([FromBody]
}

ApplicationUser user = null;

if (validatePassword.UserName != null)
{
user = await UserManager.FindByNameAsync(validatePassword.UserName);
}

var result = await _passwordValidator.ValidateAsync(UserManager, user, validatePassword.NewPassword);
var result = await ValidatePassword(user, validatePassword.NewPassword);

return Ok(result);
}
Expand Down Expand Up @@ -1067,5 +1064,23 @@ private IList<ApplicationUser> ReduceUsersDetails(IList<ApplicationUser> users)
{
return users?.Select(x => ReduceUserDetails(x)).ToList();
}

private async Task<IdentityResult> ValidatePassword(ApplicationUser user, string password)
{
var errors = new List<IdentityError>();
var isValid = true;
foreach (var passwordValidator in UserManager.PasswordValidators)
{
var identityResult = await passwordValidator.ValidateAsync(UserManager, user, password);
if (!identityResult.Succeeded)
{
errors.AddRange(identityResult.Errors);
isValid = false;
}
}

var result = isValid ? IdentityResult.Success : IdentityResult.Failed(errors.ToArray());
return result;
}
}
}
56 changes: 23 additions & 33 deletions tests/VirtoCommerce.Platform.Web.Tests/Controllers/Api/SecurityControllerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
using VirtoCommerce.Platform.Core.Security.Events;
using VirtoCommerce.Platform.Core.Security.Search;
using VirtoCommerce.Platform.Security.ExternalSignIn;
using VirtoCommerce.Platform.Web.Azure;
using VirtoCommerce.Platform.Web.Controllers.Api;
using VirtoCommerce.Platform.Web.Model.Security;
using Xunit;
Expand All @@ -31,7 +30,6 @@ public class SecurityControllerTests : PlatformWebMockHelper
private readonly Mock<IPermissionsRegistrar> _permissionsProviderMock;
private readonly Mock<IUserSearchService> _userSearchServiceMock;
private readonly Mock<IRoleSearchService> _roleSearchServiceMock;
private readonly Mock<IPasswordValidator<ApplicationUser>> _passwordValidatorMock;
private readonly Mock<IEmailSender> _emailSenderMock;
private readonly Mock<IEventPublisher> _eventPublisherMock;
private readonly Mock<IUserApiKeyService> _userApiKeyServiceMock;
Expand All @@ -47,7 +45,6 @@ public SecurityControllerTests()
_permissionsProviderMock = new Mock<IPermissionsRegistrar>();
_userSearchServiceMock = new Mock<IUserSearchService>();
_roleSearchServiceMock = new Mock<IRoleSearchService>();
_passwordValidatorMock = new Mock<IPasswordValidator<ApplicationUser>>();
_emailSenderMock = new Mock<IEmailSender>();
_eventPublisherMock = new Mock<IEventPublisher>();
_userApiKeyServiceMock = new Mock<IUserApiKeyService>();
Expand Down Expand Up @@ -84,16 +81,13 @@ public SecurityControllerTests()
private SecurityController CreateSecurityController(
Mock<IOptions<PasswordOptionsExtended>> passwordOptions = null,
Mock<IOptions<AuthorizationOptions>> securityOptions = null,
Mock<IOptions<AzureAdOptions>> azureAdOptions = null,
Mock<IOptions<PasswordLoginOptions>> passwordLoginOptions = null
)
{
passwordOptions ??= new Mock<IOptions<PasswordOptionsExtended>> { DefaultValue = DefaultValue.Mock };

securityOptions ??= new Mock<IOptions<AuthorizationOptions>> { DefaultValue = DefaultValue.Mock };

azureAdOptions ??= new Mock<IOptions<AzureAdOptions>> { DefaultValue = DefaultValue.Mock };

passwordLoginOptions ??= new Mock<IOptions<PasswordLoginOptions>> { DefaultValue = DefaultValue.Mock };

return new SecurityController(
Expand All @@ -106,7 +100,6 @@ public SecurityControllerTests()
userOptionsExtended: Mock.Of<IOptions<UserOptionsExtended>>(),
passwordOptions: passwordOptions.Object,
passwordLoginOptions: passwordLoginOptions.Object,
passwordValidator: _passwordValidatorMock.Object,
emailSender: _emailSenderMock.Object,
eventPublisher: _eventPublisherMock.Object,
userApiKeyService: _userApiKeyServiceMock.Object,
Expand All @@ -117,7 +110,7 @@ public SecurityControllerTests()
#region Login

/// <summary>
/// If sigin in manager returns fail result we should return 200 OK with succeeded = false to the client
/// If signin in manager returns fail result we should return 200 OK with succeeded = false to the client
/// </summary>
[Fact]
public async Task Login_SignInFailed()
Expand All @@ -138,7 +131,7 @@ public async Task Login_SignInFailed()
}

/// <summary>
/// If sigin in manager returns success result we should publish user login event
/// If signin in manager returns success result we should publish user login event
/// and return 200 OK with succeeded = true to the client
/// </summary>
[Fact]
Expand All @@ -165,7 +158,7 @@ public async Task Login_SignInSuccess()
}

/// <summary>
/// If sigin in manager returns success result, but user role is customer we should publish
/// If signin in manager returns success result, but user role is customer we should publish
/// user login event and return 200 OK with NotAllowed = true to the client
/// </summary>
[Fact]
Expand Down Expand Up @@ -243,7 +236,7 @@ public async Task Logout_UserNotFound()
#region Search roles

/// <summary>
/// Should retranslate result from serach service with 200 status code
/// Should retranslate result from search service with 200 status code
/// </summary>
[Fact]
public async Task SearchRoles()
Expand Down Expand Up @@ -301,7 +294,7 @@ public async Task DeleteRoles()
// Arrange
var roleIds = _fixture.CreateMany<string>().ToList();
var roles = _fixture.CreateMany<Role>(roleIds.Count - 1).ToList();
// Emulate role with last roleid is not exist
// Emulate role with last roleId is not exist
roles.Add(null);

var indexer = 0;
Expand Down Expand Up @@ -433,7 +426,7 @@ public async Task RequestPasswordReset_UserPasswordResetBeforeTimeLimit()
user.UserName = "test";
user.LastPasswordChangeRequestDate = DateTime.UtcNow;
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == user.UserName)))
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == user.UserName)))
.ReturnsAsync(user);

// Act
Expand All @@ -455,15 +448,15 @@ public async Task ResetPassword_NoUser_UnsuccessfulSecurityResult()
var user = _fixture.Create<ApplicationUser>();
user.UserName = "test";

var userName = "test";
const string userName = "test";
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == user.UserName)))
.ReturnsAsync(() => { return null; });
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == user.UserName)))
.ReturnsAsync(() => null);

var currentUser = _fixture.Create<ApplicationUser>();
currentUser.IsAdministrator = false;
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == null)))
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == null)))
.ReturnsAsync(currentUser);

// Act
Expand All @@ -481,13 +474,13 @@ public async Task ResetPassword_UserNonEditable_UnsuccessfulSecurityResult()
var user = _fixture.Create<ApplicationUser>();
user.UserName = "test";
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == user.UserName)))
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == user.UserName)))
.ReturnsAsync(user);

var currentUser = _fixture.Create<ApplicationUser>();
currentUser.IsAdministrator = false;
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == null)))
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == null)))
.ReturnsAsync(currentUser);

var options = new Mock<IOptions<AuthorizationOptions>>();
Expand Down Expand Up @@ -518,10 +511,10 @@ public async Task ResetPassword_UserNonEditable_UnsuccessfulSecurityResult()
public async Task ResetPasswordByToken_NoUser_UnsuccessfulSecurityResult()
{
// Arrange
var userName = "test";
const string userName = "test";
_userManagerMock
.Setup(x => x.FindByNameAsync(It.IsAny<string>()))
.ReturnsAsync(() => { return null; });
.ReturnsAsync(() => null);

// Act
var actual = await _controller.ResetPasswordByToken(userName, null);
Expand All @@ -538,7 +531,7 @@ public async Task ResetPasswordByToken_UserNonEditable_UnsuccessfulSecurityResul
var user = _fixture.Create<ApplicationUser>();
user.UserName = "test";
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == user.UserName)))
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == user.UserName)))
.ReturnsAsync(user);

var options = new Mock<IOptions<AuthorizationOptions>>();
Expand Down Expand Up @@ -569,25 +562,22 @@ public async Task ResetPasswordByToken_UserNonEditable_UnsuccessfulSecurityResul
public async Task ChangePassword_NoUser_UnsuccessfulSecurityResult()
{
// Arrange
var userName = "test";
const string userName = "test";
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == userName)))
.ReturnsAsync(() => { return null; });
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == userName)))
.ReturnsAsync(() => null);

var currentUser = _fixture.Create<ApplicationUser>();
currentUser.IsAdministrator = false;
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == null)))
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == null)))
.ReturnsAsync(currentUser);

var options = new Mock<IOptions<AuthorizationOptions>>();
options.SetupGet(x => x.Value)
.Returns(() =>
.Returns(() => new AuthorizationOptions
{
return new AuthorizationOptions
{
NonEditableUsers = Array.Empty<string>(),
};
NonEditableUsers = Array.Empty<string>(),
});

_controller = CreateSecurityController(securityOptions: options);
Expand All @@ -604,12 +594,12 @@ public async Task ChangePassword_NoUser_UnsuccessfulSecurityResult()
public async Task ChangePassword_UserNonEditable_UnsuccessfulSecurityResult()
{
// Arrange
var userName = "test";
const string userName = "test";

var currentUser = _fixture.Create<ApplicationUser>();
currentUser.IsAdministrator = false;
_userManagerMock
.Setup(x => x.FindByNameAsync(It.Is<string>(x => x == null)))
.Setup(x => x.FindByNameAsync(It.Is<string>(n => n == null)))
.ReturnsAsync(currentUser);

var options = new Mock<IOptions<AuthorizationOptions>>();
Expand Down

0 comments on commit ea7dc78

Please sign in to comment.