From 9091335ff23e6a32c0647c99b67f2fdd2c66f05d Mon Sep 17 00:00:00 2001 From: Alexandre Brandizzi Date: Fri, 25 Sep 2026 13:02:58 -0300 Subject: [PATCH] fix(auth): reject an unconfirmed new password and report only policy failures as weak --- .../AuthenticationControllerTests.cs | 34 +++++++++++++++++ .../PasswordPolicyTests.cs | 38 +++++++++++++++++++ .../Controllers/AuthenticationController.cs | 7 ++++ .../Auth/IdentityPasswordPolicy.cs | 20 ++++++++++ SeaHaven.Services/DTOs/IdentityDTOs.cs | 4 +- .../Implementation/AuthenticationService.cs | 9 +++-- 6 files changed, 108 insertions(+), 4 deletions(-) diff --git a/Api.SeaHavenIndustries.Tests/AuthenticationControllerTests.cs b/Api.SeaHavenIndustries.Tests/AuthenticationControllerTests.cs index 4187a24..0eaa5b4 100644 --- a/Api.SeaHavenIndustries.Tests/AuthenticationControllerTests.cs +++ b/Api.SeaHavenIndustries.Tests/AuthenticationControllerTests.cs @@ -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."); } + [Fact] + public async Task ChangePassword_ConfirmationMismatch_IsRejectedWithoutChangingThePassword() + { + var service = new Mock(); + 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().Subject; + bad.Value.Should().BeOfType().Subject.Message.Should().Be("Passwords don't match"); + service.Verify( + s => s.ChangePasswordAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + [Fact] + public async Task ChangePassword_NonPolicyFailure_ReturnsTheGenericMessage() + { + var service = new Mock(); + service.Setup(s => s.ChangePasswordAsync("42", "Current1!", "Next2@x", It.IsAny())) + .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().Subject.Value.Should().BeOfType().Subject; + response.Message.Should().Be("Your password could not be changed. Try again."); + response.Message.Should().NotContain("at least 6 characters"); + } + [Fact] public void ChangePassword_RequiresAuthenticatedCaller() { diff --git a/Api.SeaHavenIndustries.Tests/PasswordPolicyTests.cs b/Api.SeaHavenIndustries.Tests/PasswordPolicyTests.cs index 946a24d..b33d466 100644 --- a/Api.SeaHavenIndustries.Tests/PasswordPolicyTests.cs +++ b/Api.SeaHavenIndustries.Tests/PasswordPolicyTests.cs @@ -114,6 +114,44 @@ public sealed class PasswordPolicyTests : IAsyncDisposable (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>( + Mock.Of>(), 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(), + Mock.Of(), + Mock.Of()); + + 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] public async Task ChangePassword_CancelledToken_Throws() { diff --git a/Api.SeaHavenIndustries/Controllers/AuthenticationController.cs b/Api.SeaHavenIndustries/Controllers/AuthenticationController.cs index 7b6569a..dcd4dfd 100644 --- a/Api.SeaHavenIndustries/Controllers/AuthenticationController.cs +++ b/Api.SeaHavenIndustries/Controllers/AuthenticationController.cs @@ -60,6 +60,11 @@ namespace Api.SeaHavenIndustries.Controllers [HttpPost] public async Task 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 result = await _authenticationService.ChangePasswordAsync(userid, usermodel.Currentpassword, usermodel.Newpassword, cancellationToken); return result.Status switch @@ -68,6 +73,8 @@ namespace Api.SeaHavenIndustries.Controllers Ok(new Response { Status = "Success ", Message = "Password successfully changed" }), ChangePasswordStatus.PasswordRejected => 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" }) }; } diff --git a/Data.SeaHavenIndustries/Auth/IdentityPasswordPolicy.cs b/Data.SeaHavenIndustries/Auth/IdentityPasswordPolicy.cs index a0161ff..50e7543 100644 --- a/Data.SeaHavenIndustries/Auth/IdentityPasswordPolicy.cs +++ b/Data.SeaHavenIndustries/Auth/IdentityPasswordPolicy.cs @@ -23,5 +23,25 @@ namespace Data.SeaHavenIndustries options.RequireLowercase = false; options.RequiredUniqueChars = 1; } + + private static readonly HashSet PolicyErrorCodes = new(StringComparer.Ordinal) + { + nameof(IdentityErrorDescriber.PasswordTooShort), + nameof(IdentityErrorDescriber.PasswordRequiresUpper), + nameof(IdentityErrorDescriber.PasswordRequiresLower), + nameof(IdentityErrorDescriber.PasswordRequiresDigit), + nameof(IdentityErrorDescriber.PasswordRequiresNonAlphanumeric), + nameof(IdentityErrorDescriber.PasswordRequiresUniqueChars) + }; + + /// + /// 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. + /// + public static bool IsPolicyRejection(IdentityResult result) + { + ArgumentNullException.ThrowIfNull(result); + return result.Errors.Any(error => PolicyErrorCodes.Contains(error.Code)); + } } } diff --git a/SeaHaven.Services/DTOs/IdentityDTOs.cs b/SeaHaven.Services/DTOs/IdentityDTOs.cs index e959478..c618500 100644 --- a/SeaHaven.Services/DTOs/IdentityDTOs.cs +++ b/SeaHaven.Services/DTOs/IdentityDTOs.cs @@ -15,7 +15,9 @@ namespace SeaHaven.Services.DTOs { Succeeded, CurrentPasswordIncorrect, - PasswordRejected + PasswordRejected, + /// Identity failed for a reason other than the password policy. + Failed } public sealed class ChangePasswordResultDTO diff --git a/SeaHaven.Services/Implementation/AuthenticationService.cs b/SeaHaven.Services/Implementation/AuthenticationService.cs index e293276..2a44325 100644 --- a/SeaHaven.Services/Implementation/AuthenticationService.cs +++ b/SeaHaven.Services/Implementation/AuthenticationService.cs @@ -93,9 +93,12 @@ namespace SeaHaven.Services.Implementation return ChangePasswordResult(ChangePasswordStatus.CurrentPasswordIncorrect); var result = await _userManager.ChangePasswordAsync(user, currentPassword ?? "", newPassword ?? ""); - return ChangePasswordResult(result.Succeeded - ? ChangePasswordStatus.Succeeded - : ChangePasswordStatus.PasswordRejected); + if (result.Succeeded) + return ChangePasswordResult(ChangePasswordStatus.Succeeded); + + return ChangePasswordResult(IdentityPasswordPolicy.IsPolicyRejection(result) + ? ChangePasswordStatus.PasswordRejected + : ChangePasswordStatus.Failed); } private static ChangePasswordResultDTO ChangePasswordResult(ChangePasswordStatus status) =>