mirror of
https://github.com/Sea-Haven-Industries/shoc-backend.git
synced 2026-10-04 12:32:10 +00:00
fix(auth): reject an unconfirmed new password and report only policy failures as weak
This commit is contained in:
parent
66a49ab957
commit
9091335ff2
6 changed files with 108 additions and 4 deletions
|
|
@ -166,6 +166,40 @@ public class AuthenticationControllerTests
|
||||||
"Password must be at least 6 characters and include one uppercase letter, one number, and one special character.");
|
"Password must be at least 6 characters and include one uppercase letter, one number, and one special character.");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task ChangePassword_ConfirmationMismatch_IsRejectedWithoutChangingThePassword()
|
||||||
|
{
|
||||||
|
var service = new Mock<IAuthenticationService>();
|
||||||
|
var controller = NewController(service, "42");
|
||||||
|
|
||||||
|
var result = await controller.ChangePassword(
|
||||||
|
new ChangePasswords { Currentpassword = "Current1!", Newpassword = "Next2@x", Confirmpassword = "Next2@y" },
|
||||||
|
CancellationToken.None);
|
||||||
|
|
||||||
|
var bad = result.Should().BeOfType<BadRequestObjectResult>().Subject;
|
||||||
|
bad.Value.Should().BeOfType<Response>().Subject.Message.Should().Be("Passwords don't match");
|
||||||
|
service.Verify(
|
||||||
|
s => s.ChangePasswordAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()),
|
||||||
|
Times.Never);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task ChangePassword_NonPolicyFailure_ReturnsTheGenericMessage()
|
||||||
|
{
|
||||||
|
var service = new Mock<IAuthenticationService>();
|
||||||
|
service.Setup(s => s.ChangePasswordAsync("42", "Current1!", "Next2@x", It.IsAny<CancellationToken>()))
|
||||||
|
.ReturnsAsync(new ChangePasswordResultDTO { Status = ChangePasswordStatus.Failed });
|
||||||
|
var controller = NewController(service, "42");
|
||||||
|
|
||||||
|
var result = await controller.ChangePassword(
|
||||||
|
new ChangePasswords { Currentpassword = "Current1!", Newpassword = "Next2@x", Confirmpassword = "Next2@x" },
|
||||||
|
CancellationToken.None);
|
||||||
|
|
||||||
|
var response = result.Should().BeOfType<BadRequestObjectResult>().Subject.Value.Should().BeOfType<Response>().Subject;
|
||||||
|
response.Message.Should().Be("Your password could not be changed. Try again.");
|
||||||
|
response.Message.Should().NotContain("at least 6 characters");
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void ChangePassword_RequiresAuthenticatedCaller()
|
public void ChangePassword_RequiresAuthenticatedCaller()
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -114,6 +114,44 @@ public sealed class PasswordPolicyTests : IAsyncDisposable
|
||||||
(await UserManager.CheckPasswordAsync(reloaded!, "Next2@x")).Should().BeTrue();
|
(await UserManager.CheckPasswordAsync(reloaded!, "Next2@x")).Should().BeTrue();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Theory]
|
||||||
|
[InlineData("ConcurrencyFailure")]
|
||||||
|
[InlineData("PasswordMismatch")]
|
||||||
|
[InlineData("DefaultError")]
|
||||||
|
public async Task ChangePassword_NonPolicyIdentityFailure_IsNotReportedAsAWeakPassword(string code)
|
||||||
|
{
|
||||||
|
var user = new ApplicationUser { Id = "member-1", UserName = "member@example.com" };
|
||||||
|
var userManager = new Mock<UserManager<ApplicationUser>>(
|
||||||
|
Mock.Of<IUserStore<ApplicationUser>>(), null!, null!, null!, null!, null!, null!, null!, null!);
|
||||||
|
userManager.Setup(m => m.FindByIdAsync(user.Id)).ReturnsAsync(user);
|
||||||
|
userManager.Setup(m => m.CheckPasswordAsync(user, CurrentPassword)).ReturnsAsync(true);
|
||||||
|
userManager.Setup(m => m.ChangePasswordAsync(user, CurrentPassword, "Next2@x"))
|
||||||
|
.ReturnsAsync(IdentityResult.Failed(new IdentityError { Code = code, Description = "failed" }));
|
||||||
|
var service = new AuthenticationService(
|
||||||
|
userManager.Object,
|
||||||
|
Microsoft.Extensions.Options.Options.Create(new JwtOptions { Secret = new string('x', 64) }),
|
||||||
|
Mock.Of<IUserDataService>(),
|
||||||
|
Mock.Of<IForgetPasswordDataService>(),
|
||||||
|
Mock.Of<IEmailSender>());
|
||||||
|
|
||||||
|
var result = await service.ChangePasswordAsync(user.Id, CurrentPassword, "Next2@x", CancellationToken.None);
|
||||||
|
|
||||||
|
result.Status.Should().Be(ChangePasswordStatus.Failed);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Theory]
|
||||||
|
[InlineData("PasswordTooShort", true)]
|
||||||
|
[InlineData("PasswordRequiresUpper", true)]
|
||||||
|
[InlineData("PasswordRequiresDigit", true)]
|
||||||
|
[InlineData("PasswordRequiresNonAlphanumeric", true)]
|
||||||
|
[InlineData("ConcurrencyFailure", false)]
|
||||||
|
[InlineData("PasswordMismatch", false)]
|
||||||
|
public void IsPolicyRejection_MatchesOnlyThePasswordRuleCodes(string code, bool expected)
|
||||||
|
{
|
||||||
|
IdentityPasswordPolicy.IsPolicyRejection(IdentityResult.Failed(new IdentityError { Code = code }))
|
||||||
|
.Should().Be(expected);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task ChangePassword_CancelledToken_Throws()
|
public async Task ChangePassword_CancelledToken_Throws()
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -60,6 +60,11 @@ namespace Api.SeaHavenIndustries.Controllers
|
||||||
[HttpPost]
|
[HttpPost]
|
||||||
public async Task<IActionResult> ChangePassword(ChangePasswords usermodel, CancellationToken cancellationToken)
|
public async Task<IActionResult> ChangePassword(ChangePasswords usermodel, CancellationToken cancellationToken)
|
||||||
{
|
{
|
||||||
|
// [Compare] already rejects this during model validation; the check here keeps the
|
||||||
|
// unconfirmed password from ever being set if that validation is bypassed.
|
||||||
|
if (!string.Equals(usermodel.Newpassword, usermodel.Confirmpassword, StringComparison.Ordinal))
|
||||||
|
return BadRequest(new Response { Status = "Password confirmation does not match", Message = "Passwords don't match" });
|
||||||
|
|
||||||
var userid = User.FindFirstValue(ClaimTypes.NameIdentifier) ?? "";
|
var userid = User.FindFirstValue(ClaimTypes.NameIdentifier) ?? "";
|
||||||
var result = await _authenticationService.ChangePasswordAsync(userid, usermodel.Currentpassword, usermodel.Newpassword, cancellationToken);
|
var result = await _authenticationService.ChangePasswordAsync(userid, usermodel.Currentpassword, usermodel.Newpassword, cancellationToken);
|
||||||
return result.Status switch
|
return result.Status switch
|
||||||
|
|
@ -68,6 +73,8 @@ namespace Api.SeaHavenIndustries.Controllers
|
||||||
Ok(new Response { Status = "Success ", Message = "Password successfully changed" }),
|
Ok(new Response { Status = "Success ", Message = "Password successfully changed" }),
|
||||||
ChangePasswordStatus.PasswordRejected =>
|
ChangePasswordStatus.PasswordRejected =>
|
||||||
BadRequest(new Response { Status = "Password does not meet requirements", Message = PasswordRequirementsMessage }),
|
BadRequest(new Response { Status = "Password does not meet requirements", Message = PasswordRequirementsMessage }),
|
||||||
|
ChangePasswordStatus.Failed =>
|
||||||
|
BadRequest(new Response { Status = "Password not changed", Message = "Your password could not be changed. Try again." }),
|
||||||
_ => BadRequest(new Response { Status = "Old Password is incorrect", Message = "Current password is incorrect" })
|
_ => BadRequest(new Response { Status = "Old Password is incorrect", Message = "Current password is incorrect" })
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -23,5 +23,25 @@ namespace Data.SeaHavenIndustries
|
||||||
options.RequireLowercase = false;
|
options.RequireLowercase = false;
|
||||||
options.RequiredUniqueChars = 1;
|
options.RequiredUniqueChars = 1;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private static readonly HashSet<string> PolicyErrorCodes = new(StringComparer.Ordinal)
|
||||||
|
{
|
||||||
|
nameof(IdentityErrorDescriber.PasswordTooShort),
|
||||||
|
nameof(IdentityErrorDescriber.PasswordRequiresUpper),
|
||||||
|
nameof(IdentityErrorDescriber.PasswordRequiresLower),
|
||||||
|
nameof(IdentityErrorDescriber.PasswordRequiresDigit),
|
||||||
|
nameof(IdentityErrorDescriber.PasswordRequiresNonAlphanumeric),
|
||||||
|
nameof(IdentityErrorDescriber.PasswordRequiresUniqueChars)
|
||||||
|
};
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// True when Identity refused the password itself. Other failures, such as a
|
||||||
|
/// concurrency conflict, must not be reported to the user as a weak password.
|
||||||
|
/// </summary>
|
||||||
|
public static bool IsPolicyRejection(IdentityResult result)
|
||||||
|
{
|
||||||
|
ArgumentNullException.ThrowIfNull(result);
|
||||||
|
return result.Errors.Any(error => PolicyErrorCodes.Contains(error.Code));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -15,7 +15,9 @@ namespace SeaHaven.Services.DTOs
|
||||||
{
|
{
|
||||||
Succeeded,
|
Succeeded,
|
||||||
CurrentPasswordIncorrect,
|
CurrentPasswordIncorrect,
|
||||||
PasswordRejected
|
PasswordRejected,
|
||||||
|
/// <summary>Identity failed for a reason other than the password policy.</summary>
|
||||||
|
Failed
|
||||||
}
|
}
|
||||||
|
|
||||||
public sealed class ChangePasswordResultDTO
|
public sealed class ChangePasswordResultDTO
|
||||||
|
|
|
||||||
|
|
@ -93,9 +93,12 @@ namespace SeaHaven.Services.Implementation
|
||||||
return ChangePasswordResult(ChangePasswordStatus.CurrentPasswordIncorrect);
|
return ChangePasswordResult(ChangePasswordStatus.CurrentPasswordIncorrect);
|
||||||
|
|
||||||
var result = await _userManager.ChangePasswordAsync(user, currentPassword ?? "", newPassword ?? "");
|
var result = await _userManager.ChangePasswordAsync(user, currentPassword ?? "", newPassword ?? "");
|
||||||
return ChangePasswordResult(result.Succeeded
|
if (result.Succeeded)
|
||||||
? ChangePasswordStatus.Succeeded
|
return ChangePasswordResult(ChangePasswordStatus.Succeeded);
|
||||||
: ChangePasswordStatus.PasswordRejected);
|
|
||||||
|
return ChangePasswordResult(IdentityPasswordPolicy.IsPolicyRejection(result)
|
||||||
|
? ChangePasswordStatus.PasswordRejected
|
||||||
|
: ChangePasswordStatus.Failed);
|
||||||
}
|
}
|
||||||
|
|
||||||
private static ChangePasswordResultDTO ChangePasswordResult(ChangePasswordStatus status) =>
|
private static ChangePasswordResultDTO ChangePasswordResult(ChangePasswordStatus status) =>
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue