Skip to content
This repository was archived by the owner on Dec 20, 2018. It is now read-only.

Commit 2629cee

Browse files
HaoKHaoK
committed
CheckPassword only reset lockout when TFA disabled
1 parent 5403ec4 commit 2629cee

2 files changed

Lines changed: 79 additions & 15 deletions

File tree

‎src/Identity/SignInManager.cs‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,8 @@ public SignInManager(UserManager<TUser> userManager,
8989
/// <summary>
9090
/// The <see cref="HttpContext"/> used.
9191
/// </summary>
92-
public HttpContext Context {
92+
public HttpContext Context
93+
{
9394
get
9495
{
9596
var context = _context ?? _contextAccessor?.HttpContext;
@@ -257,7 +258,7 @@ public virtual async Task<TUser> ValidateTwoFactorSecurityStampAsync(ClaimsPrinc
257258
/// <param name="securityStamp">The expected security stamp value.</param>
258259
/// <returns>True if the stamp matches the persisted value, otherwise it will return false.</returns>
259260
public virtual async Task<bool> ValidateSecurityStampAsync(TUser user, string securityStamp)
260-
=> user != null && UserManager.SupportsUserSecurityStamp
261+
=> user != null && UserManager.SupportsUserSecurityStamp
261262
&& securityStamp == await UserManager.GetSecurityStampAsync(user);
262263

263264
/// <summary>
@@ -279,7 +280,7 @@ public virtual async Task<SignInResult> PasswordSignInAsync(TUser user, string p
279280
}
280281

281282
var attempt = await CheckPasswordSignInAsync(user, password, lockoutOnFailure);
282-
return attempt.Succeeded
283+
return attempt.Succeeded
283284
? await SignInOrTwoFactorAsync(user, isPersistent)
284285
: attempt;
285286
}
@@ -330,7 +331,13 @@ public virtual async Task<SignInResult> CheckPasswordSignInAsync(TUser user, str
330331

331332
if (await UserManager.CheckPasswordAsync(user, password))
332333
{
333-
await ResetLockout(user);
334+
var alwaysLockout = AppContext.TryGetSwitch("Microsoft.AspNetCore.Identity.CheckPasswordSignInAlwaysResetLockoutOnSuccess", out var enabled) && enabled;
335+
// Only reset the lockout when TFA is not enabled when not in quirks mode
336+
if (alwaysLockout || !await IsTfaEnabled(user))
337+
{
338+
await ResetLockout(user);
339+
}
340+
334341
return SignInResult.Success;
335342
}
336343
Logger.LogWarning(2, "User {userId} failed to provide the correct password.", await UserManager.GetUserIdAsync(user));
@@ -534,7 +541,7 @@ public virtual async Task<TUser> GetTwoFactorAuthenticationUserAsync()
534541
/// <param name="isPersistent">Flag indicating whether the sign-in cookie should persist after the browser is closed.</param>
535542
/// <returns>The task object representing the asynchronous operation containing the <see name="SignInResult"/>
536543
/// for the sign-in attempt.</returns>
537-
public virtual Task<SignInResult> ExternalLoginSignInAsync(string loginProvider, string providerKey, bool isPersistent)
544+
public virtual Task<SignInResult> ExternalLoginSignInAsync(string loginProvider, string providerKey, bool isPersistent)
538545
=> ExternalLoginSignInAsync(loginProvider, providerKey, isPersistent, bypassTwoFactor: false);
539546

540547
/// <summary>
@@ -645,7 +652,7 @@ public virtual async Task<IdentityResult> UpdateExternalAuthenticationTokensAsyn
645652

646653
return IdentityResult.Success;
647654
}
648-
655+
649656
/// <summary>
650657
/// Configures the redirect URL and user identifier for the specified external login <paramref name="provider"/>.
651658
/// </summary>
@@ -708,7 +715,12 @@ private ClaimsIdentity CreateIdentity(TwoFactorAuthenticationInfo info)
708715
}
709716
return identity;
710717
}
711-
718+
719+
private async Task<bool> IsTfaEnabled(TUser user)
720+
=> UserManager.SupportsUserTwoFactor &&
721+
await UserManager.GetTwoFactorEnabledAsync(user) &&
722+
(await UserManager.GetValidTwoFactorProvidersAsync(user)).Count > 0;
723+
712724
/// <summary>
713725
/// Signs in the specified <paramref name="user"/> if <paramref name="bypassTwoFactor"/> is set to false.
714726
/// Otherwise stores the <paramref name="user"/> for use after a two factor check.
@@ -720,10 +732,7 @@ private ClaimsIdentity CreateIdentity(TwoFactorAuthenticationInfo info)
720732
/// <returns>Returns a <see cref="SignInResult"/></returns>
721733
protected virtual async Task<SignInResult> SignInOrTwoFactorAsync(TUser user, bool isPersistent, string loginProvider = null, bool bypassTwoFactor = false)
722734
{
723-
if (!bypassTwoFactor &&
724-
UserManager.SupportsUserTwoFactor &&
725-
await UserManager.GetTwoFactorEnabledAsync(user) &&
726-
(await UserManager.GetValidTwoFactorProvidersAsync(user)).Count > 0)
735+
if (!bypassTwoFactor && await IsTfaEnabled(user))
727736
{
728737
if (!await IsTwoFactorClientRememberedAsync(user))
729738
{

‎test/Identity.Test/SignInManagerTest.cs‎

Lines changed: 59 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,64 @@ public async Task PasswordSignInWorksWithNonTwoFactorStore()
266266
auth.Verify();
267267
}
268268

269+
[Theory]
270+
[InlineData(true)]
271+
[InlineData(false)]
272+
public async Task CheckPasswordOnlyResetLockoutWhenTfaNotEnabled(bool tfaEnabled)
273+
{
274+
// Setup
275+
var user = new PocoUser { UserName = "Foo" };
276+
var manager = SetupUserManager(user);
277+
manager.Setup(m => m.SupportsUserLockout).Returns(true).Verifiable();
278+
manager.Setup(m => m.IsLockedOutAsync(user)).ReturnsAsync(false).Verifiable();
279+
manager.Setup(m => m.SupportsUserTwoFactor).Returns(tfaEnabled).Verifiable();
280+
manager.Setup(m => m.CheckPasswordAsync(user, "password")).ReturnsAsync(true).Verifiable();
281+
282+
if (tfaEnabled)
283+
{
284+
manager.Setup(m => m.GetTwoFactorEnabledAsync(user)).ReturnsAsync(true).Verifiable();
285+
manager.Setup(m => m.GetValidTwoFactorProvidersAsync(user)).ReturnsAsync(new string[1] {"Fake"}).Verifiable();
286+
}
287+
else
288+
{
289+
manager.Setup(m => m.ResetAccessFailedCountAsync(user)).ReturnsAsync(IdentityResult.Success).Verifiable();
290+
}
291+
292+
var context = new DefaultHttpContext();
293+
var helper = SetupSignInManager(manager.Object, context);
294+
295+
// Act
296+
var result = await helper.CheckPasswordSignInAsync(user, "password", false);
297+
298+
// Assert
299+
Assert.True(result.Succeeded);
300+
manager.Verify();
301+
}
302+
303+
[Fact]
304+
public async Task CheckPasswordAlwaysResetLockoutWhenQuirked()
305+
{
306+
AppContext.SetSwitch("Microsoft.AspNetCore.Identity.CheckPasswordSignInAlwaysResetLockoutOnSuccess", true);
307+
308+
// Setup
309+
var user = new PocoUser { UserName = "Foo" };
310+
var manager = SetupUserManager(user);
311+
manager.Setup(m => m.SupportsUserLockout).Returns(true).Verifiable();
312+
manager.Setup(m => m.IsLockedOutAsync(user)).ReturnsAsync(false).Verifiable();
313+
manager.Setup(m => m.CheckPasswordAsync(user, "password")).ReturnsAsync(true).Verifiable();
314+
manager.Setup(m => m.ResetAccessFailedCountAsync(user)).ReturnsAsync(IdentityResult.Success).Verifiable();
315+
316+
var context = new DefaultHttpContext();
317+
var helper = SetupSignInManager(manager.Object, context);
318+
319+
// Act
320+
var result = await helper.CheckPasswordSignInAsync(user, "password", false);
321+
322+
// Assert
323+
Assert.True(result.Succeeded);
324+
manager.Verify();
325+
}
326+
269327
[Theory]
270328
[InlineData(true)]
271329
[InlineData(false)]
@@ -285,10 +343,7 @@ public async Task PasswordSignInRequiresVerification(bool supportsLockout)
285343
manager.Setup(m => m.SupportsUserTwoFactor).Returns(true).Verifiable();
286344
manager.Setup(m => m.GetTwoFactorEnabledAsync(user)).ReturnsAsync(true).Verifiable();
287345
manager.Setup(m => m.CheckPasswordAsync(user, "password")).ReturnsAsync(true).Verifiable();
288-
if (supportsLockout)
289-
{
290-
manager.Setup(m => m.ResetAccessFailedCountAsync(user)).ReturnsAsync(IdentityResult.Success).Verifiable();
291-
}
346+
manager.Setup(m => m.GetValidTwoFactorProvidersAsync(user)).ReturnsAsync(new string[1] { "Fake" }).Verifiable();
292347
var context = new DefaultHttpContext();
293348
var helper = SetupSignInManager(manager.Object, context);
294349
var auth = MockAuth(context);

0 commit comments

Comments
 (0)