Prepare for nullable annotations in Microsoft.Extensions.Identity.Core

smfeest · smfeest · commit 280f398c2808 · 2023-03-04T22:16:27.000Z
Nullable annotations have been added to Microsoft.Extensions.Identity.Core [1] in .NET 7, necessitating these changes to avoid CS8604 and CS8634 compiler warnings on upgrading. [1] dotnet/aspnetcore#40007
diff --git a/src/Buttercup.Web.Tests/Authentication/AuthenticationManagerTests.cs b/src/Buttercup.Web.Tests/Authentication/AuthenticationManagerTests.cs
@@ -227,7 +227,7 @@ public async Task ChangePasswordDoesNotChangePasswordIfCurrentPasswordDoesNotMat
         await fixture.ChangePassword();
 
         fixture.MockPasswordHasher.Verify(
-            x => x.HashPassword(null, fixture.NewPassword), Times.Never);
+            x => x.HashPassword(fixture.User, fixture.NewPassword), Times.Never);
     }
 
     [Fact]
@@ -311,7 +311,7 @@ private ChangePasswordFixture(string? hashedPassword)
             this.HttpContext.SetCurrentUser(this.User);
 
             this.MockPasswordHasher
-                .Setup(x => x.HashPassword(null, this.NewPassword))
+                .Setup(x => x.HashPassword(this.User, this.NewPassword))
                 .Returns(this.HashedNewPassword);
 
             this.MockRandomTokenGenerator
@@ -451,7 +451,9 @@ public Task<bool> PasswordResetTokenIsValid() =>
     [Fact]
     public async Task ResetPasswordDeletesExpiredPasswordResetTokens()
     {
-        var fixture = ResetPasswordFixture.ForSuccess();
+        var fixture = new ResetPasswordFixture();
+
+        fixture.SetupSuccess();
 
         await fixture.ResetPassword();
 
@@ -462,7 +464,9 @@ public async Task ResetPasswordDeletesExpiredPasswordResetTokens()
     [Fact]
     public async Task ResetPasswordLogsIfTokenIsInvalid()
     {
-        var fixture = ResetPasswordFixture.ForInvalidToken();
+        var fixture = new ResetPasswordFixture();
+
+        fixture.SetupInvalidToken();
 
         try
         {
@@ -482,40 +486,48 @@ public async Task ResetPasswordLogsIfTokenIsInvalid()
     [Fact]
     public async Task ResetPasswordThrowsIfTokenIsInvalid()
     {
-        var fixture = ResetPasswordFixture.ForInvalidToken();
+        var fixture = new ResetPasswordFixture();
+
+        fixture.SetupInvalidToken();
 
         await Assert.ThrowsAsync<InvalidTokenException>(fixture.ResetPassword);
     }
 
     [Fact]
     public async Task ResetPasswordUpdatesUserOnSuccess()
     {
-        var fixture = ResetPasswordFixture.ForSuccess();
+        var fixture = new ResetPasswordFixture();
+
+        fixture.SetupSuccess();
 
         await fixture.ResetPassword();
 
         fixture.MockUserDataProvider.Verify(x => x.UpdatePassword(
             fixture.MySqlConnection,
-            fixture.UserId!.Value,
+            fixture.User.Id,
             fixture.NewHashedPassword,
             fixture.NewSecurityStamp));
     }
 
     [Fact]
     public async Task ResetPasswordDeletesPasswordResetTokensOnSuccess()
     {
-        var fixture = ResetPasswordFixture.ForSuccess();
+        var fixture = new ResetPasswordFixture();
+
+        fixture.SetupSuccess();
 
         await fixture.ResetPassword();
 
         fixture.MockPasswordResetTokenDataProvider.Verify(
-            x => x.DeleteTokensForUser(fixture.MySqlConnection, fixture.UserId!.Value));
+            x => x.DeleteTokensForUser(fixture.MySqlConnection, fixture.User.Id));
     }
 
     [Fact]
     public async Task ResetPasswordSendsPasswordChangeNotificationOnSuccess()
     {
-        var fixture = ResetPasswordFixture.ForSuccess();
+        var fixture = new ResetPasswordFixture();
+
+        fixture.SetupSuccess();
 
         await fixture.ResetPassword();
 
@@ -526,72 +538,59 @@ public async Task ResetPasswordSendsPasswordChangeNotificationOnSuccess()
     [Fact]
     public async Task ResetPasswordLogsOnSuccess()
     {
-        var fixture = ResetPasswordFixture.ForSuccess();
+        var fixture = new ResetPasswordFixture();
+
+        fixture.SetupSuccess();
 
         await fixture.ResetPassword();
 
         fixture.Logger.AssertSingleEntry(
             LogLevel.Information,
-            $"Password reset for user {fixture.UserId} using token {fixture.RedactedToken}");
+            $"Password reset for user {fixture.User.Id} using token {fixture.RedactedToken}");
 
-        fixture.AssertAuthenticationEventLogged("password_reset_success", fixture.UserId);
+        fixture.AssertAuthenticationEventLogged("password_reset_success", fixture.User.Id);
     }
 
     [Fact]
-    public async Task ResetPasswordReturnsUserOnSuccess()
+    public async Task ResetPasswordReturnsUserWithNewSecurityStampOnSuccess()
     {
-        var fixture = ResetPasswordFixture.ForSuccess();
+        var fixture = new ResetPasswordFixture();
+
+        fixture.SetupSuccess();
 
         var actual = await fixture.ResetPassword();
 
-        Assert.Equal(fixture.User, actual);
+        Assert.Equal(fixture.User with { SecurityStamp = fixture.NewSecurityStamp }, actual);
     }
 
     private class ResetPasswordFixture : AuthenticationManagerFixture
     {
         private const string NewPassword = "new-password";
         private const string Token = "password-reset-token";
 
-        private ResetPasswordFixture()
-        {
-        }
+        public string RedactedToken { get; } = "passwo…";
 
-        private ResetPasswordFixture(long? userId)
-        {
-            this.UserId = userId;
-            this.SetupGetUserIdForToken(Token, userId);
-        }
+        public User User { get; } = ModelFactory.CreateUser();
 
-        private ResetPasswordFixture(User user)
-            : this(user.Id)
-        {
-            this.User = user;
+        public string NewHashedPassword { get; } = "new-hashed-password";
+
+        public string NewSecurityStamp { get; } = "new-security-stamp";
 
-            this.SetupGetUser(user.Id, user);
+        public void SetupInvalidToken() => this.SetupGetUserIdForToken(Token, null);
 
+        public void SetupSuccess()
+        {
+            this.SetupGetUserIdForToken(Token, this.User.Id);
+            this.SetupGetUser(this.User.Id, this.User);
             this.MockPasswordHasher
-                .Setup(x => x.HashPassword(null, NewPassword))
+                .Setup(x => x.HashPassword(this.User, NewPassword))
                 .Returns(this.NewHashedPassword);
 
             this.MockRandomTokenGenerator
                 .Setup(x => x.Generate(2))
                 .Returns(this.NewSecurityStamp);
         }
 
-        public string RedactedToken { get; } = "passwo…";
-
-        public long? UserId { get; }
-
-        public User? User { get; }
-
-        public string NewHashedPassword { get; } = "new-hashed-password";
-
-        public string NewSecurityStamp { get; } = "new-security-stamp";
-
-        public static ResetPasswordFixture ForInvalidToken() => new();
-
-        public static ResetPasswordFixture ForSuccess() => new(ModelFactory.CreateUser());
-
         public Task<User> ResetPassword() =>
             this.AuthenticationManager.ResetPassword(Token, NewPassword);
     }
@@ -1010,7 +1009,7 @@ public AuthenticationManagerFixture()
 
         public Mock<IAuthenticationService> MockAuthenticationService { get; } = new();
 
-        public Mock<IPasswordHasher<User?>> MockPasswordHasher { get; } = new();
+        public Mock<IPasswordHasher<User>> MockPasswordHasher { get; } = new();
 
         public Mock<IPasswordResetTokenDataProvider> MockPasswordResetTokenDataProvider { get; } = new();
 
diff --git a/src/Buttercup.Web/Authentication/AuthenticationManager.cs b/src/Buttercup.Web/Authentication/AuthenticationManager.cs
@@ -18,7 +18,7 @@ public class AuthenticationManager : IAuthenticationManager
     private readonly IClock clock;
     private readonly IMySqlConnectionSource mySqlConnectionSource;
     private readonly ILogger<AuthenticationManager> logger;
-    private readonly IPasswordHasher<User?> passwordHasher;
+    private readonly IPasswordHasher<User> passwordHasher;
     private readonly IPasswordResetTokenDataProvider passwordResetTokenDataProvider;
     private readonly IRandomTokenGenerator randomTokenGenerator;
     private readonly IUrlHelperFactory urlHelperFactory;
@@ -32,7 +32,7 @@ public AuthenticationManager(
         IClock clock,
         IMySqlConnectionSource mySqlConnectionSource,
         ILogger<AuthenticationManager> logger,
-        IPasswordHasher<User?> passwordHasher,
+        IPasswordHasher<User> passwordHasher,
         IPasswordResetTokenDataProvider passwordResetTokenDataProvider,
         IRandomTokenGenerator randomTokenGenerator,
         IUrlHelperFactory urlHelperFactory,
@@ -79,7 +79,7 @@ await this.authenticationEventDataProvider.LogEvent(
             return null;
         }
 
-        if (!this.VerifyPassword(user, password))
+        if (!this.VerifyPassword(user, user.HashedPassword, password))
         {
             AuthenticateLogMessages.IncorrectPassword(this.logger, user.Id, user.Email, null);
 
@@ -113,7 +113,7 @@ await this.authenticationEventDataProvider.LogEvent(
                 $"User {user.Id} ({user.Email}) does not have a password.");
         }
 
-        if (!this.VerifyPassword(user, currentPassword))
+        if (!this.VerifyPassword(user, user.HashedPassword, currentPassword))
         {
             ChangePasswordLogMessages.IncorrectPassword(
                 this.logger, user.Id, user.Email, null);
@@ -124,7 +124,7 @@ await this.authenticationEventDataProvider.LogEvent(
             return false;
         }
 
-        var newSecurityStamp = await this.SetPassword(connection, user.Id, newPassword);
+        var newSecurityStamp = await this.SetPassword(connection, user, newPassword);
 
         ChangePasswordLogMessages.Success(this.logger, user.Id, user.Email, null);
 
@@ -176,18 +176,18 @@ await this.authenticationEventDataProvider.LogEvent(
             throw new InvalidTokenException("Password reset token is invalid");
         }
 
-        await this.SetPassword(connection, userId.Value, newPassword);
+        var user = await this.userDataProvider.GetUser(connection, userId.Value);
+
+        var newSecurityStamp = await this.SetPassword(connection, user, newPassword);
 
         ResetPasswordLogMessages.Success(this.logger, userId.Value, RedactToken(token), null);
 
         await this.authenticationEventDataProvider.LogEvent(
             connection, "password_reset_success", userId.Value);
 
-        var user = await this.userDataProvider.GetUser(connection, userId.Value);
-
         await this.authenticationMailer.SendPasswordChangeNotification(user.Email);
 
-        return user;
+        return user with { SecurityStamp = newSecurityStamp };
     }
 
     public async Task SendPasswordResetLink(ActionContext actionContext, string email)
@@ -258,50 +258,57 @@ public async Task ValidatePrincipal(CookieValidatePrincipalContext context)
     {
         var principal = context.Principal;
 
-        var userId = principal?.GetUserId();
+        if (principal == null)
+        {
+            return;
+        }
 
-        if (userId.HasValue)
+        var userId = principal.GetUserId();
+
+        if (!userId.HasValue)
         {
-            User user;
+            return;
+        }
 
-            using (var connection = await this.mySqlConnectionSource.OpenConnection())
-            {
-                user = await this.userDataProvider.GetUser(connection, userId.Value);
-            }
+        User user;
 
-            var securityStamp = principal.FindFirstValue(CustomClaimTypes.SecurityStamp);
+        using (var connection = await this.mySqlConnectionSource.OpenConnection())
+        {
+            user = await this.userDataProvider.GetUser(connection, userId.Value);
+        }
 
-            if (string.Equals(securityStamp, user.SecurityStamp, StringComparison.Ordinal))
-            {
-                context.HttpContext.SetCurrentUser(user);
+        var securityStamp = principal.FindFirstValue(CustomClaimTypes.SecurityStamp);
 
-                ValidatePrincipalLogMessages.Success(this.logger, user.Id, user.Email, null);
-            }
-            else
-            {
-                ValidatePrincipalLogMessages.IncorrectSecurityStamp(
-                    this.logger, user.Id, user.Email, null);
+        if (string.Equals(securityStamp, user.SecurityStamp, StringComparison.Ordinal))
+        {
+            context.HttpContext.SetCurrentUser(user);
 
-                context.RejectPrincipal();
+            ValidatePrincipalLogMessages.Success(this.logger, user.Id, user.Email, null);
+        }
+        else
+        {
+            ValidatePrincipalLogMessages.IncorrectSecurityStamp(
+                this.logger, user.Id, user.Email, null);
 
-                await this.SignOutCurrentUser(context.HttpContext);
-            }
+            context.RejectPrincipal();
+
+            await this.SignOutCurrentUser(context.HttpContext);
         }
     }
 
     private static string RedactToken(string token) => $"{token[..6]}…";
 
     private async Task<string> SetPassword(
-        MySqlConnection connection, long userId, string newPassword)
+        MySqlConnection connection, User user, string newPassword)
     {
-        var hashedPassword = this.passwordHasher.HashPassword(null, newPassword);
+        var hashedPassword = this.passwordHasher.HashPassword(user, newPassword);
 
         var securityStamp = this.randomTokenGenerator.Generate(2);
 
         await this.userDataProvider.UpdatePassword(
-            connection, userId, hashedPassword, securityStamp);
+            connection, user.Id, hashedPassword, securityStamp);
 
-        await this.passwordResetTokenDataProvider.DeleteTokensForUser(connection, userId);
+        await this.passwordResetTokenDataProvider.DeleteTokensForUser(connection, user.Id);
 
         return securityStamp;
     }
@@ -328,8 +335,8 @@ await this.passwordResetTokenDataProvider.DeleteExpiredTokens(
         return await this.passwordResetTokenDataProvider.GetUserIdForToken(connection, token);
     }
 
-    private bool VerifyPassword(User user, string password) =>
-        this.passwordHasher.VerifyHashedPassword(user, user.HashedPassword, password) ==
+    private bool VerifyPassword(User user, string hashedPassword, string password) =>
+        this.passwordHasher.VerifyHashedPassword(user, hashedPassword, password) ==
             PasswordVerificationResult.Success;
 
     private static class AuthenticateLogMessages
@@ -418,8 +425,8 @@ private static class SignInLogMessages
 
     private static class SignOutLogMessages
     {
-        public static readonly Action<ILogger, long, string, Exception?> SignedOut =
-            LoggerMessage.Define<long, string>(
+        public static readonly Action<ILogger, long, string?, Exception?> SignedOut =
+            LoggerMessage.Define<long, string?>(
                 LogLevel.Information, 213, "User {UserId} ({Email}) signed out");
     }
 
diff --git a/src/Buttercup.Web/Program.cs b/src/Buttercup.Web/Program.cs
@@ -76,7 +76,7 @@
     .AddBugsnag();
 
 services
-    .AddTransient<IPasswordHasher<User?>, PasswordHasher<User?>>()
+    .AddTransient<IPasswordHasher<User>, PasswordHasher<User>>()
     .AddTransient<IAccessTokenEncoder, AccessTokenEncoder>()
     .AddTransient<IAccessTokenSerializer, AccessTokenSerializer>()
     .AddTransient<IAuthenticationMailer, AuthenticationMailer>()
