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

Commit

Permalink
Merge pull request #2 from aspnet/haok/2.0-msrc
Browse files Browse the repository at this point in the history 
2.0 CheckPassword only reset lockout when TFA disabled
  • Loading branch information
@HaoK
HaoK authored May 23, 2018
2 parents 952b38a + 17dda64 commit b532f11
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 14 deletions.
29 changes: 19 additions & 10 deletions src/Microsoft.AspNetCore.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 @@ -251,7 +252,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 @@ -302,7 +303,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 @@ -508,7 +515,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 @@ -619,7 +626,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 @@ -669,7 +676,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 @@ -681,10 +693,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/Microsoft.AspNetCore.Identity.Test/SignInManagerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,64 @@ public async Task PasswordSignInWorksWithNonTwoFactorStore()
auth.Verify();
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public async Task CheckPasswordOnlyResetLockoutWhenTfaNotEnabled(bool tfaEnabled)
{
// Setup
var user = new TestUser { 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 TestUser { 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 @@ -286,10 +344,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 b532f11

Please sign in to comment.