From 394eb3ca13c662e1c2288372e05f46fbcb149c16 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Thu, 16 May 2019 09:38:30 -0700 Subject: [PATCH] Allow password validation failures to propogate (#6034) --- .../Extensions.Core/src/UserManager.cs | 10 +- .../src/IdentityResultAssert.cs | 11 +- .../src/UserManagerSpecificationTests.cs | 102 +++++++++++++++++- .../test/Identity.Test/UserManagerTest.cs | 28 ++++- 4 files changed, 140 insertions(+), 11 deletions(-) diff --git a/src/Identity/Extensions.Core/src/UserManager.cs b/src/Identity/Extensions.Core/src/UserManager.cs index b785a82019..4116045c4a 100644 --- a/src/Identity/Extensions.Core/src/UserManager.cs +++ b/src/Identity/Extensions.Core/src/UserManager.cs @@ -2517,15 +2517,21 @@ namespace Microsoft.AspNetCore.Identity protected async Task ValidatePasswordAsync(TUser user, string password) { var errors = new List(); + var isValid = true; foreach (var v in PasswordValidators) { var result = await v.ValidateAsync(this, user, password); if (!result.Succeeded) { - errors.AddRange(result.Errors); + if (result.Errors.Any()) + { + errors.AddRange(result.Errors); + } + + isValid = false; } } - if (errors.Count > 0) + if (!isValid) { Logger.LogWarning(14, "User {userId} password validation failed: {errors}.", await GetUserIdAsync(user), string.Join(";", errors.Select(e => e.Code))); return IdentityResult.Failed(errors.ToArray()); diff --git a/src/Identity/Specification.Tests/src/IdentityResultAssert.cs b/src/Identity/Specification.Tests/src/IdentityResultAssert.cs index 4246d1712f..1e94569442 100644 --- a/src/Identity/Specification.Tests/src/IdentityResultAssert.cs +++ b/src/Identity/Specification.Tests/src/IdentityResultAssert.cs @@ -44,12 +44,15 @@ namespace Microsoft.AspNetCore.Identity.Test /// /// Asserts that the result has not Succeeded and that first error matches error's code and Description. /// - public static void IsFailure(IdentityResult result, IdentityError error) + public static void IsFailure(IdentityResult result, IdentityError error = null) { Assert.NotNull(result); Assert.False(result.Succeeded); - Assert.Equal(error.Description, result.Errors.First().Description); - Assert.Equal(error.Code, result.Errors.First().Code); + if (error != null) + { + Assert.Equal(error.Description, result.Errors.FirstOrDefault()?.Description); + Assert.Equal(error.Code, result.Errors.FirstOrDefault()?.Code); + } } /// @@ -70,4 +73,4 @@ namespace Microsoft.AspNetCore.Identity.Test } } } -} \ No newline at end of file +} diff --git a/src/Identity/Specification.Tests/src/UserManagerSpecificationTests.cs b/src/Identity/Specification.Tests/src/UserManagerSpecificationTests.cs index 9774ea5677..b5fe50375a 100644 --- a/src/Identity/Specification.Tests/src/UserManagerSpecificationTests.cs +++ b/src/Identity/Specification.Tests/src/UserManagerSpecificationTests.cs @@ -163,6 +163,39 @@ namespace Microsoft.AspNetCore.Identity.Test } } + private class EmptyBadValidator : IUserValidator, + IPasswordValidator + { + public Task ValidateAsync(UserManager manager, TUser user, string password) + { + return Task.FromResult(IdentityResult.Failed()); + } + + public Task ValidateAsync(UserManager manager, TUser user) + { + return Task.FromResult(IdentityResult.Failed()); + } + } + + /// + /// Test. + /// + /// Task + [Fact] + public async Task PasswordValidatorWithNoErrorsCanBlockAddPassword() + { + if (ShouldSkipDbTests()) + { + return; + } + var manager = CreateManager(); + var user = CreateTestUser(); + IdentityResultAssert.IsSuccess(await manager.CreateAsync(user)); + manager.PasswordValidators.Clear(); + manager.PasswordValidators.Add(new EmptyBadValidator()); + IdentityResultAssert.IsFailure(await manager.AddPasswordAsync(user, "password")); + } + /// /// Test. /// @@ -497,13 +530,78 @@ namespace Microsoft.AspNetCore.Identity.Test } var manager = CreateManager(); manager.PasswordValidators.Clear(); - manager.PasswordValidators.Add(new AlwaysBadValidator()); + manager.PasswordValidators.Add(new EmptyBadValidator()); manager.PasswordValidators.Add(new AlwaysBadValidator()); var user = CreateTestUser(); IdentityResultAssert.IsSuccess(await manager.CreateAsync(user)); var result = await manager.AddPasswordAsync(user, "pwd"); IdentityResultAssert.IsFailure(result, AlwaysBadValidator.ErrorMessage); - Assert.Equal(2, result.Errors.Count()); + Assert.Single(result.Errors); + } + + /// + /// Test. + /// + /// Task + [Fact] + public async Task PasswordValidatorWithNoErrorsCanBlockChangePassword() + { + if (ShouldSkipDbTests()) + { + return; + } + var manager = CreateManager(); + var user = CreateTestUser(); + IdentityResultAssert.IsSuccess(await manager.CreateAsync(user, "password")); + manager.PasswordValidators.Clear(); + manager.PasswordValidators.Add(new AlwaysBadValidator()); + IdentityResultAssert.IsFailure(await manager.ChangePasswordAsync(user, "password", "new")); + } + + /// + /// Test. + /// + /// Task + [Fact] + public async Task PasswordValidatorWithNoErrorsCanBlockCreateUser() + { + if (ShouldSkipDbTests()) + { + return; + } + var manager = CreateManager(); + var user = CreateTestUser(); + manager.PasswordValidators.Clear(); + manager.PasswordValidators.Add(new AlwaysBadValidator()); + IdentityResultAssert.IsFailure(await manager.CreateAsync(user, "password")); + } + + /// + /// Test. + /// + /// Task + [Fact] + public async Task PasswordValidatorWithNoErrorsCanBlockResetPasswordWithStaticTokenProvider() + { + if (ShouldSkipDbTests()) + { + return; + } + var manager = CreateManager(); + manager.RegisterTokenProvider("Static", new StaticTokenProvider()); + manager.Options.Tokens.PasswordResetTokenProvider = "Static"; + var user = CreateTestUser(); + const string password = "password"; + const string newPassword = "newpassword"; + IdentityResultAssert.IsSuccess(await manager.CreateAsync(user, password)); + var stamp = await manager.GetSecurityStampAsync(user); + Assert.NotNull(stamp); + var token = await manager.GeneratePasswordResetTokenAsync(user); + Assert.NotNull(token); + manager.PasswordValidators.Add(new AlwaysBadValidator()); + IdentityResultAssert.IsFailure(await manager.ResetPasswordAsync(user, token, newPassword)); + Assert.True(await manager.CheckPasswordAsync(user, password)); + Assert.Equal(stamp, await manager.GetSecurityStampAsync(user)); } /// diff --git a/src/Identity/test/Identity.Test/UserManagerTest.cs b/src/Identity/test/Identity.Test/UserManagerTest.cs index 9dd93e39e3..f5c6e938a8 100644 --- a/src/Identity/test/Identity.Test/UserManagerTest.cs +++ b/src/Identity/test/Identity.Test/UserManagerTest.cs @@ -982,11 +982,21 @@ namespace Microsoft.AspNetCore.Identity.Test // TODO: Can switch to Mock eventually var manager = MockHelpers.TestUserManager(new EmptyStore()); manager.PasswordValidators.Clear(); - manager.PasswordValidators.Add(new BadPasswordValidator()); + manager.PasswordValidators.Add(new BadPasswordValidator(true)); IdentityResultAssert.IsFailure(await manager.CreateAsync(new PocoUser(), "password"), BadPasswordValidator.ErrorMessage); } + [Fact] + public async Task PasswordValidatorWithoutErrorsBlocksCreate() + { + // TODO: Can switch to Mock eventually + var manager = MockHelpers.TestUserManager(new EmptyStore()); + manager.PasswordValidators.Clear(); + manager.PasswordValidators.Add(new BadPasswordValidator()); + IdentityResultAssert.IsFailure(await manager.CreateAsync(new PocoUser(), "password")); + } + [Fact] public async Task ResetTokenCallNoopForTokenValueZero() { @@ -1175,10 +1185,22 @@ namespace Microsoft.AspNetCore.Identity.Test { public static readonly IdentityError ErrorMessage = new IdentityError { Description = "I'm Bad." }; - public Task ValidateAsync(UserManager manager, TUser user, string password) + private IdentityResult badResult; + + public BadPasswordValidator(bool includeErrorMessage = false) { - return Task.FromResult(IdentityResult.Failed(ErrorMessage)); + if (includeErrorMessage) + { + badResult = IdentityResult.Failed(ErrorMessage); + } + else + { + badResult = IdentityResult.Failed(); + } } + + public Task ValidateAsync(UserManager manager, TUser user, string password) + => Task.FromResult(badResult); } private class EmptyStore :