From 2629ceed08f6588fec8d0e80297ea9fa0b245b9f Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Mon, 23 Apr 2018 13:48:27 -0700 Subject: [PATCH] CheckPassword only reset lockout when TFA disabled --- src/Identity/SignInManager.cs | 31 +++++++----- test/Identity.Test/SignInManagerTest.cs | 63 +++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 15 deletions(-) diff --git a/src/Identity/SignInManager.cs b/src/Identity/SignInManager.cs index 879a38dcf..d553cd71c 100644 --- a/src/Identity/SignInManager.cs +++ b/src/Identity/SignInManager.cs @@ -89,7 +89,8 @@ public SignInManager(UserManager userManager, /// /// The used. /// - public HttpContext Context { + public HttpContext Context + { get { var context = _context ?? _contextAccessor?.HttpContext; @@ -257,7 +258,7 @@ public virtual async Task ValidateTwoFactorSecurityStampAsync(ClaimsPrinc /// The expected security stamp value. /// True if the stamp matches the persisted value, otherwise it will return false. public virtual async Task ValidateSecurityStampAsync(TUser user, string securityStamp) - => user != null && UserManager.SupportsUserSecurityStamp + => user != null && UserManager.SupportsUserSecurityStamp && securityStamp == await UserManager.GetSecurityStampAsync(user); /// @@ -279,7 +280,7 @@ public virtual async Task PasswordSignInAsync(TUser user, string p } var attempt = await CheckPasswordSignInAsync(user, password, lockoutOnFailure); - return attempt.Succeeded + return attempt.Succeeded ? await SignInOrTwoFactorAsync(user, isPersistent) : attempt; } @@ -330,7 +331,13 @@ public virtual async Task CheckPasswordSignInAsync(TUser user, str if (await UserManager.CheckPasswordAsync(user, password)) { - await ResetLockout(user); + var alwaysLockout = AppContext.TryGetSwitch("Microsoft.AspNetCore.Identity.CheckPasswordSignInAlwaysResetLockoutOnSuccess", out var enabled) && enabled; + // Only reset the lockout when TFA is not enabled when not in quirks mode + if (alwaysLockout || !await IsTfaEnabled(user)) + { + await ResetLockout(user); + } + return SignInResult.Success; } Logger.LogWarning(2, "User {userId} failed to provide the correct password.", await UserManager.GetUserIdAsync(user)); @@ -534,7 +541,7 @@ public virtual async Task GetTwoFactorAuthenticationUserAsync() /// Flag indicating whether the sign-in cookie should persist after the browser is closed. /// The task object representing the asynchronous operation containing the /// for the sign-in attempt. - public virtual Task ExternalLoginSignInAsync(string loginProvider, string providerKey, bool isPersistent) + public virtual Task ExternalLoginSignInAsync(string loginProvider, string providerKey, bool isPersistent) => ExternalLoginSignInAsync(loginProvider, providerKey, isPersistent, bypassTwoFactor: false); /// @@ -645,7 +652,7 @@ public virtual async Task UpdateExternalAuthenticationTokensAsyn return IdentityResult.Success; } - + /// /// Configures the redirect URL and user identifier for the specified external login . /// @@ -708,7 +715,12 @@ private ClaimsIdentity CreateIdentity(TwoFactorAuthenticationInfo info) } return identity; } - + + private async Task IsTfaEnabled(TUser user) + => UserManager.SupportsUserTwoFactor && + await UserManager.GetTwoFactorEnabledAsync(user) && + (await UserManager.GetValidTwoFactorProvidersAsync(user)).Count > 0; + /// /// Signs in the specified if is set to false. /// Otherwise stores the for use after a two factor check. @@ -720,10 +732,7 @@ private ClaimsIdentity CreateIdentity(TwoFactorAuthenticationInfo info) /// Returns a protected virtual async Task SignInOrTwoFactorAsync(TUser user, bool isPersistent, string loginProvider = null, bool bypassTwoFactor = false) { - if (!bypassTwoFactor && - UserManager.SupportsUserTwoFactor && - await UserManager.GetTwoFactorEnabledAsync(user) && - (await UserManager.GetValidTwoFactorProvidersAsync(user)).Count > 0) + if (!bypassTwoFactor && await IsTfaEnabled(user)) { if (!await IsTwoFactorClientRememberedAsync(user)) { diff --git a/test/Identity.Test/SignInManagerTest.cs b/test/Identity.Test/SignInManagerTest.cs index 4ac34d4d0..392e0e9ac 100644 --- a/test/Identity.Test/SignInManagerTest.cs +++ b/test/Identity.Test/SignInManagerTest.cs @@ -266,6 +266,64 @@ public async Task PasswordSignInWorksWithNonTwoFactorStore() auth.Verify(); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task CheckPasswordOnlyResetLockoutWhenTfaNotEnabled(bool tfaEnabled) + { + // Setup + var user = new PocoUser { UserName = "Foo" }; + var manager = SetupUserManager(user); + manager.Setup(m => m.SupportsUserLockout).Returns(true).Verifiable(); + manager.Setup(m => m.IsLockedOutAsync(user)).ReturnsAsync(false).Verifiable(); + manager.Setup(m => m.SupportsUserTwoFactor).Returns(tfaEnabled).Verifiable(); + manager.Setup(m => m.CheckPasswordAsync(user, "password")).ReturnsAsync(true).Verifiable(); + + if (tfaEnabled) + { + manager.Setup(m => m.GetTwoFactorEnabledAsync(user)).ReturnsAsync(true).Verifiable(); + manager.Setup(m => m.GetValidTwoFactorProvidersAsync(user)).ReturnsAsync(new string[1] {"Fake"}).Verifiable(); + } + else + { + manager.Setup(m => m.ResetAccessFailedCountAsync(user)).ReturnsAsync(IdentityResult.Success).Verifiable(); + } + + var context = new DefaultHttpContext(); + var helper = SetupSignInManager(manager.Object, context); + + // Act + var result = await helper.CheckPasswordSignInAsync(user, "password", false); + + // Assert + Assert.True(result.Succeeded); + manager.Verify(); + } + + [Fact] + public async Task CheckPasswordAlwaysResetLockoutWhenQuirked() + { + AppContext.SetSwitch("Microsoft.AspNetCore.Identity.CheckPasswordSignInAlwaysResetLockoutOnSuccess", true); + + // Setup + var user = new PocoUser { UserName = "Foo" }; + var manager = SetupUserManager(user); + manager.Setup(m => m.SupportsUserLockout).Returns(true).Verifiable(); + manager.Setup(m => m.IsLockedOutAsync(user)).ReturnsAsync(false).Verifiable(); + manager.Setup(m => m.CheckPasswordAsync(user, "password")).ReturnsAsync(true).Verifiable(); + manager.Setup(m => m.ResetAccessFailedCountAsync(user)).ReturnsAsync(IdentityResult.Success).Verifiable(); + + var context = new DefaultHttpContext(); + var helper = SetupSignInManager(manager.Object, context); + + // Act + var result = await helper.CheckPasswordSignInAsync(user, "password", false); + + // Assert + Assert.True(result.Succeeded); + manager.Verify(); + } + [Theory] [InlineData(true)] [InlineData(false)] @@ -285,10 +343,7 @@ public async Task PasswordSignInRequiresVerification(bool supportsLockout) manager.Setup(m => m.SupportsUserTwoFactor).Returns(true).Verifiable(); manager.Setup(m => m.GetTwoFactorEnabledAsync(user)).ReturnsAsync(true).Verifiable(); manager.Setup(m => m.CheckPasswordAsync(user, "password")).ReturnsAsync(true).Verifiable(); - if (supportsLockout) - { - manager.Setup(m => m.ResetAccessFailedCountAsync(user)).ReturnsAsync(IdentityResult.Success).Verifiable(); - } + manager.Setup(m => m.GetValidTwoFactorProvidersAsync(user)).ReturnsAsync(new string[1] { "Fake" }).Verifiable(); var context = new DefaultHttpContext(); var helper = SetupSignInManager(manager.Object, context); var auth = MockAuth(context);