From dcdcb18f9b4b2764ee07c3da62deae8d3407e765 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Fri, 11 Nov 2016 15:48:04 -0800 Subject: [PATCH 1/5] UserValidator now blocks null security stamps --- .../IdentityErrorDescriber.cs | 16 +++++++++++++- .../Properties/Resources.Designer.cs | 16 ++++++++++++++ .../Resources.resx | 4 ++++ .../UserValidator.cs | 8 +++++++ .../UserValidatorTest.cs | 22 +++++++++++++++++++ 5 files changed, 65 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.AspNetCore.Identity/IdentityErrorDescriber.cs b/src/Microsoft.AspNetCore.Identity/IdentityErrorDescriber.cs index 54c024c79..b074a2d90 100644 --- a/src/Microsoft.AspNetCore.Identity/IdentityErrorDescriber.cs +++ b/src/Microsoft.AspNetCore.Identity/IdentityErrorDescriber.cs @@ -14,7 +14,7 @@ public class IdentityErrorDescriber /// /// Returns the default . /// - /// The default , + /// The default . public virtual IdentityError DefaultError() { return new IdentityError @@ -37,6 +37,20 @@ public virtual IdentityError ConcurrencyFailure() }; } + /// + /// Returns an indicating a security stamp was null. + /// + /// An indicating a security stamp was null. + public virtual IdentityError NullSecurityStamp() + { + return new IdentityError + { + Code = nameof(NullSecurityStamp), + Description = Resources.NullSecurityStamp + }; + } + + /// /// Returns an indicating a password mismatch. /// diff --git a/src/Microsoft.AspNetCore.Identity/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Identity/Properties/Resources.Designer.cs index 002925879..fa00dea7f 100644 --- a/src/Microsoft.AspNetCore.Identity/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Identity/Properties/Resources.Designer.cs @@ -250,6 +250,22 @@ internal static string FormatNoTokenProvider(object p0) return string.Format(CultureInfo.CurrentCulture, GetString("NoTokenProvider"), p0); } + /// + /// User security stamp cannot be null. + /// + internal static string NullSecurityStamp + { + get { return GetString("NullSecurityStamp"); } + } + + /// + /// User security stamp cannot be null. + /// + internal static string FormatNullSecurityStamp() + { + return GetString("NullSecurityStamp"); + } + /// /// Incorrect password. /// diff --git a/src/Microsoft.AspNetCore.Identity/Resources.resx b/src/Microsoft.AspNetCore.Identity/Resources.resx index 85e16fc30..3e0512b16 100644 --- a/src/Microsoft.AspNetCore.Identity/Resources.resx +++ b/src/Microsoft.AspNetCore.Identity/Resources.resx @@ -177,6 +177,10 @@ No IUserTokenProvider named '{0}' is registered. Error when there is no IUserTokenProvider + + User security stamp cannot be null. + Error when a user's security stamp is null. + Incorrect password. Error when a password doesn't match diff --git a/src/Microsoft.AspNetCore.Identity/UserValidator.cs b/src/Microsoft.AspNetCore.Identity/UserValidator.cs index af870f246..e4227c1fd 100644 --- a/src/Microsoft.AspNetCore.Identity/UserValidator.cs +++ b/src/Microsoft.AspNetCore.Identity/UserValidator.cs @@ -56,6 +56,14 @@ public virtual async Task ValidateAsync(UserManager manag { await ValidateEmail(manager, user, errors); } + if (manager.SupportsUserSecurityStamp) + { + var stamp = await manager.GetSecurityStampAsync(user); + if (stamp == null) + { + errors.Add(Describer.NullSecurityStamp()); + } + } return errors.Count > 0 ? IdentityResult.Failed(errors.ToArray()) : IdentityResult.Success; } diff --git a/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs b/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs index eadafb804..2bbacc271 100644 --- a/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs +++ b/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs @@ -2,7 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Threading; using System.Threading.Tasks; +using Moq; using Xunit; namespace Microsoft.AspNetCore.Identity.Test @@ -39,6 +41,26 @@ public async Task ValidateFailsWithTooShortUserNames(string input) IdentityResultAssert.IsFailure(result, new IdentityErrorDescriber().InvalidUserName(input)); } + [Fact] + public async Task ValidateFailsWithNullSecurityStamp() + { + // Setup + var store = new Mock>(); + var manager = MockHelpers.TestUserManager(store.Object); + var validator = new UserValidator(); + var user = new TestUser { UserName = "nulldude" }; + store.Setup(s => s.GetSecurityStampAsync(user, It.IsAny())).ReturnsAsync(null).Verifiable(); + store.Setup(s => s.GetUserNameAsync(user, It.IsAny())).ReturnsAsync(user.UserName).Verifiable(); + + // Act + var result = await validator.ValidateAsync(manager, user); + + // Assert + IdentityResultAssert.IsFailure(result, new IdentityErrorDescriber().NullSecurityStamp()); + + store.VerifyAll(); + } + [Theory] [InlineData("test_email@foo.com", true)] [InlineData("hao", true)] From b0f6dce7a6e21afa31392402a4984be7ccf447ea Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 15 Nov 2016 13:44:56 -0800 Subject: [PATCH 2/5] Move null stamp check to user manager --- .../UserManager.cs | 8 +++++ .../UserValidator.cs | 8 ----- .../UserManagerTest.cs | 36 +++++++++++++++++++ .../UserValidatorTest.cs | 20 ----------- 4 files changed, 44 insertions(+), 28 deletions(-) diff --git a/src/Microsoft.AspNetCore.Identity/UserManager.cs b/src/Microsoft.AspNetCore.Identity/UserManager.cs index d3eff1b91..ffd67bd86 100644 --- a/src/Microsoft.AspNetCore.Identity/UserManager.cs +++ b/src/Microsoft.AspNetCore.Identity/UserManager.cs @@ -2236,6 +2236,14 @@ protected static string GetChangeEmailTokenPurpose(string newEmail) private async Task ValidateUserInternal(TUser user) { + if (SupportsUserSecurityStamp) + { + var stamp = await GetSecurityStampAsync(user); + if (stamp == null) + { + throw new InvalidOperationException(Resources.NullSecurityStamp); + } + } var errors = new List(); foreach (var v in UserValidators) { diff --git a/src/Microsoft.AspNetCore.Identity/UserValidator.cs b/src/Microsoft.AspNetCore.Identity/UserValidator.cs index e4227c1fd..af870f246 100644 --- a/src/Microsoft.AspNetCore.Identity/UserValidator.cs +++ b/src/Microsoft.AspNetCore.Identity/UserValidator.cs @@ -56,14 +56,6 @@ public virtual async Task ValidateAsync(UserManager manag { await ValidateEmail(manager, user, errors); } - if (manager.SupportsUserSecurityStamp) - { - var stamp = await manager.GetSecurityStampAsync(user); - if (stamp == null) - { - errors.Add(Describer.NullSecurityStamp()); - } - } return errors.Count > 0 ? IdentityResult.Failed(errors.ToArray()) : IdentityResult.Success; } diff --git a/test/Microsoft.AspNetCore.Identity.Test/UserManagerTest.cs b/test/Microsoft.AspNetCore.Identity.Test/UserManagerTest.cs index 664bc14f9..61a0e1e0e 100644 --- a/test/Microsoft.AspNetCore.Identity.Test/UserManagerTest.cs +++ b/test/Microsoft.AspNetCore.Identity.Test/UserManagerTest.cs @@ -483,6 +483,42 @@ public async Task CheckPasswordWillRehashPasswordWhenNeeded() hasher.VerifyAll(); } + [Fact] + public async Task CreateFailsWithNullSecurityStamp() + { + // Setup + var store = new Mock>(); + var manager = MockHelpers.TestUserManager(store.Object); + var user = new TestUser { UserName = "nulldude" }; + store.Setup(s => s.GetSecurityStampAsync(user, It.IsAny())).ReturnsAsync(null).Verifiable(); + + // Act + // Assert + var ex = await Assert.ThrowsAsync(() => manager.CreateAsync(user)); + Assert.Contains(Resources.NullSecurityStamp, ex.Message); + + store.VerifyAll(); + } + + [Fact] + public async Task UpdateFailsWithNullSecurityStamp() + { + // Setup + var store = new Mock>(); + var manager = MockHelpers.TestUserManager(store.Object); + var user = new TestUser { UserName = "nulldude" }; + store.Setup(s => s.GetSecurityStampAsync(user, It.IsAny())).ReturnsAsync(null).Verifiable(); + + // Act + // Assert + var ex = await Assert.ThrowsAsync(() => manager.UpdateAsync(user)); + Assert.Contains(Resources.NullSecurityStamp, ex.Message); + + store.VerifyAll(); + } + + + [Fact] public async Task RemoveClaimsCallsStore() { diff --git a/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs b/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs index 2bbacc271..74ce941cd 100644 --- a/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs +++ b/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs @@ -41,26 +41,6 @@ public async Task ValidateFailsWithTooShortUserNames(string input) IdentityResultAssert.IsFailure(result, new IdentityErrorDescriber().InvalidUserName(input)); } - [Fact] - public async Task ValidateFailsWithNullSecurityStamp() - { - // Setup - var store = new Mock>(); - var manager = MockHelpers.TestUserManager(store.Object); - var validator = new UserValidator(); - var user = new TestUser { UserName = "nulldude" }; - store.Setup(s => s.GetSecurityStampAsync(user, It.IsAny())).ReturnsAsync(null).Verifiable(); - store.Setup(s => s.GetUserNameAsync(user, It.IsAny())).ReturnsAsync(user.UserName).Verifiable(); - - // Act - var result = await validator.ValidateAsync(manager, user); - - // Assert - IdentityResultAssert.IsFailure(result, new IdentityErrorDescriber().NullSecurityStamp()); - - store.VerifyAll(); - } - [Theory] [InlineData("test_email@foo.com", true)] [InlineData("hao", true)] From 4ecf8d6258b89bd611f645f41ddc4abc9bf4f6e3 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 15 Nov 2016 13:47:12 -0800 Subject: [PATCH 3/5] Remove error from describer --- .../IdentityErrorDescriber.cs | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/Microsoft.AspNetCore.Identity/IdentityErrorDescriber.cs b/src/Microsoft.AspNetCore.Identity/IdentityErrorDescriber.cs index b074a2d90..914e82e5e 100644 --- a/src/Microsoft.AspNetCore.Identity/IdentityErrorDescriber.cs +++ b/src/Microsoft.AspNetCore.Identity/IdentityErrorDescriber.cs @@ -37,20 +37,6 @@ public virtual IdentityError ConcurrencyFailure() }; } - /// - /// Returns an indicating a security stamp was null. - /// - /// An indicating a security stamp was null. - public virtual IdentityError NullSecurityStamp() - { - return new IdentityError - { - Code = nameof(NullSecurityStamp), - Description = Resources.NullSecurityStamp - }; - } - - /// /// Returns an indicating a password mismatch. /// From 1afc1f08edf2672a0d3e27d584bed59a976fc986 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 15 Nov 2016 13:48:18 -0800 Subject: [PATCH 4/5] Cleanup usings --- test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs b/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs index 74ce941cd..eadafb804 100644 --- a/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs +++ b/test/Microsoft.AspNetCore.Identity.Test/UserValidatorTest.cs @@ -2,9 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Threading; using System.Threading.Tasks; -using Moq; using Xunit; namespace Microsoft.AspNetCore.Identity.Test From b7e400ab343c65aefa2e5f0fe74644ea74788dc9 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 15 Nov 2016 13:57:39 -0800 Subject: [PATCH 5/5] Store also blocks null stamp --- .../UserStore.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Microsoft.AspNetCore.Identity.EntityFrameworkCore/UserStore.cs b/src/Microsoft.AspNetCore.Identity.EntityFrameworkCore/UserStore.cs index ddedc15c3..87cf6d257 100644 --- a/src/Microsoft.AspNetCore.Identity.EntityFrameworkCore/UserStore.cs +++ b/src/Microsoft.AspNetCore.Identity.EntityFrameworkCore/UserStore.cs @@ -1247,6 +1247,10 @@ public async virtual Task FindByLoginAsync(string loginProvider, string p { throw new ArgumentNullException(nameof(user)); } + if (stamp == null) + { + throw new ArgumentNullException(nameof(stamp)); + } user.SecurityStamp = stamp; return TaskCache.CompletedTask; }