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

Commit

Permalink
CheckPassword only reset lockout when TFA disabled
Browse files Browse the repository at this point in the history
  • Loading branch information
@HaoK
HaoK committed May 17, 2018
1 parent 5403ec4 commit 2629cee
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 15 deletions.
31 changes: 20 additions & 11 deletions src/Identity/SignInManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ public SignInManager(UserManager<TUser> userManager,
/// <summary>
/// The <see cref="HttpContext"/> used.
/// </summary>
public HttpContext Context {
public HttpContext Context
{
get
{
var context = _context ?? _contextAccessor?.HttpContext;
Expand Down Expand Up @@ -257,7 +258,7 @@ public virtual async Task<TUser> ValidateTwoFactorSecurityStampAsync(ClaimsPrinc
/// <param name="securityStamp">The expected security stamp value.</param>
/// <returns>True if the stamp matches the persisted value, otherwise it will return false.</returns>
public virtual async Task<bool> ValidateSecurityStampAsync(TUser user, string securityStamp)
=> user != null && UserManager.SupportsUserSecurityStamp
=> user != null && UserManager.SupportsUserSecurityStamp
&& securityStamp == await UserManager.GetSecurityStampAsync(user);

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

var attempt = await CheckPasswordSignInAsync(user, password, lockoutOnFailure);
return attempt.Succeeded
return attempt.Succeeded
? await SignInOrTwoFactorAsync(user, isPersistent)
: attempt;
}
Expand Down Expand Up @@ -330,7 +331,13 @@ public virtual async Task<SignInResult> 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));
Expand Down Expand Up @@ -534,7 +541,7 @@ public virtual async Task<TUser> GetTwoFactorAuthenticationUserAsync()
/// <param name="isPersistent">Flag indicating whether the sign-in cookie should persist after the browser is closed.</param>
/// <returns>The task object representing the asynchronous operation containing the <see name="SignInResult"/>
/// for the sign-in attempt.</returns>
public virtual Task<SignInResult> ExternalLoginSignInAsync(string loginProvider, string providerKey, bool isPersistent)
public virtual Task<SignInResult> ExternalLoginSignInAsync(string loginProvider, string providerKey, bool isPersistent)
=> ExternalLoginSignInAsync(loginProvider, providerKey, isPersistent, bypassTwoFactor: false);

/// <summary>
Expand Down Expand Up @@ -645,7 +652,7 @@ public virtual async Task<IdentityResult> UpdateExternalAuthenticationTokensAsyn

return IdentityResult.Success;
}

/// <summary>
/// Configures the redirect URL and user identifier for the specified external login <paramref name="provider"/>.
/// </summary>
Expand Down Expand Up @@ -708,7 +715,12 @@ private ClaimsIdentity CreateIdentity(TwoFactorAuthenticationInfo info)
}
return identity;
}


private async Task<bool> IsTfaEnabled(TUser user)
=> UserManager.SupportsUserTwoFactor &&
await UserManager.GetTwoFactorEnabledAsync(user) &&
(await UserManager.GetValidTwoFactorProvidersAsync(user)).Count > 0;

/// <summary>
/// Signs in the specified <paramref name="user"/> if <paramref name="bypassTwoFactor"/> is set to false.
/// Otherwise stores the <paramref name="user"/> for use after a two factor check.
Expand All @@ -720,10 +732,7 @@ private ClaimsIdentity CreateIdentity(TwoFactorAuthenticationInfo info)
/// <returns>Returns a <see cref="SignInResult"/></returns>
protected virtual async Task<SignInResult> 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))
{
Expand Down
63 changes: 59 additions & 4 deletions test/Identity.Test/SignInManagerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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);
Expand Down

0 comments on commit 2629cee

Please sign in to comment.