Merged PR 47275: Prevent RefreshSignInAsync if it is called with wron…

…g user

Since this is a patch, instead of throwing an exception in cases where we wouldn't before like we do in the PR to `main`, we instead log an error and fail to refresh the cookie if RefreshSignInAsync is called with a TUser that does not have the same user ID as the currently authenticated user.

While this does not make it *as* obvious to developers that something has gone wrong, error logs are still pretty visible, and stale cookies are something web developers have to account for regardless. The big upside to not throwing in the patch is that we do not have to react to it in the email change confirmation flow to account for the possibility of RefreshSignInAsync throwing.

----
#### AI description  (iteration 1)
#### PR Classification
Bug fix

#### PR Summary
This pull request disables the `RefreshSignInAsync` method if it is called with the wrong user, ensuring proper user authentication handling.
- `src/Identity/Core/src/SignInManager.cs`: Added checks to disable `RefreshSignInAsync` if the user is not authenticated or if the authenticated user ID does not match the provided user ID.
- `src/Identity/test/Identity.Test/SignInManagerTest.cs`: Added tests to verify that `RefreshSignInAsync` logs errors and does not proceed if the user is not authenticated or if authenticated with a different user.

src/Identity/Core/src/SignInManager.cs

@@ -162,8 +162,21 @@ public virtual async Task<bool> CanSignInAsync(TUser user)
     public virtual async Task RefreshSignInAsync(TUser user)
     {
         var auth = await Context.AuthenticateAsync(AuthenticationScheme);
-        IList<Claim> claims = Array.Empty<Claim>();
+        if (!auth.Succeeded || auth.Principal?.Identity?.IsAuthenticated != true)
+        {
+            Logger.LogError("RefreshSignInAsync prevented because the user is not currently authenticated. Use SignInAsync instead for initial sign in.");
+            return;
+        }
 
+        var authenticatedUserId = UserManager.GetUserId(auth.Principal);
+        var newUserId = await UserManager.GetUserIdAsync(user);
+        if (authenticatedUserId == null || authenticatedUserId != newUserId)
+        {
+            Logger.LogError("RefreshSignInAsync prevented because currently authenticated user has a different UserId. Use SignInAsync instead to change users.");
+            return;
+        }
+
+        IList<Claim> claims = Array.Empty<Claim>();
         var authenticationMethod = auth?.Principal?.FindFirst(ClaimTypes.AuthenticationMethod);
         var amr = auth?.Principal?.FindFirst("amr");
 

src/Identity/test/Identity.Test/SignInManagerTest.cs

@@ -592,38 +592,38 @@ public async Task CanExternalSignIn(bool isPersistent, bool supportsLockout)
     [InlineData(true, false)]
     [InlineData(false, true)]
     [InlineData(false, false)]
-    public async Task CanResignIn(
-        // Suppress warning that says theory methods should use all of their parameters.
-        // See comments below about why this isn't used.
-#pragma warning disable xUnit1026
-        bool isPersistent,
-#pragma warning restore xUnit1026
-        bool externalLogin)
+    public async Task CanResignIn(bool isPersistent, bool externalLogin)
     {
         // Setup
         var user = new PocoUser { UserName = "Foo" };
         var context = new DefaultHttpContext();
         var auth = MockAuth(context);
         var loginProvider = "loginprovider";
-        var id = new ClaimsIdentity();
+        var id = new ClaimsIdentity("authscheme");
         if (externalLogin)
         {
             id.AddClaim(new Claim(ClaimTypes.AuthenticationMethod, loginProvider));
         }
-        // REVIEW: auth changes we lost the ability to mock is persistent
-        //var properties = new AuthenticationProperties { IsPersistent = isPersistent };
-        var authResult = AuthenticateResult.NoResult();
+
+        var claimsPrincipal = new ClaimsPrincipal(id);
+        var properties = new AuthenticationProperties { IsPersistent = isPersistent };
+        var authResult = AuthenticateResult.Success(new AuthenticationTicket(claimsPrincipal, properties, "authscheme"));
         auth.Setup(a => a.AuthenticateAsync(context, IdentityConstants.ApplicationScheme))
             .Returns(Task.FromResult(authResult)).Verifiable();
         var manager = SetupUserManager(user);
+        manager.Setup(m => m.GetUserId(claimsPrincipal)).Returns(user.Id.ToString());
         var signInManager = new Mock<SignInManager<PocoUser>>(manager.Object,
             new HttpContextAccessor { HttpContext = context },
             new Mock<IUserClaimsPrincipalFactory<PocoUser>>().Object,
             null, null, new Mock<IAuthenticationSchemeProvider>().Object, null)
         { CallBase = true };
-        //signInManager.Setup(s => s.SignInAsync(user, It.Is<AuthenticationProperties>(p => p.IsPersistent == isPersistent),
-        //externalLogin? loginProvider : null)).Returns(Task.FromResult(0)).Verifiable();
-        signInManager.Setup(s => s.SignInWithClaimsAsync(user, It.IsAny<AuthenticationProperties>(), It.IsAny<IEnumerable<Claim>>())).Returns(Task.FromResult(0)).Verifiable();
+
+        signInManager.Setup(s => s.SignInWithClaimsAsync(user,
+                It.Is<AuthenticationProperties>(properties => properties.IsPersistent == isPersistent),
+                It.Is<IEnumerable<Claim>>(claims => !externalLogin ||
+                    claims.Any(claim => claim.Type == ClaimTypes.AuthenticationMethod && claim.Value == loginProvider))))
+            .Returns(Task.FromResult(0)).Verifiable();
+
         signInManager.Object.Context = context;
 
         // Act
@@ -634,6 +634,58 @@ public async Task CanResignIn(
         signInManager.Verify();
     }
 
+    [Fact]
+    public async Task ResignInNoOpsAndLogsErrorIfNotAuthenticated()
+    {
+        var user = new PocoUser { UserName = "Foo" };
+        var context = new DefaultHttpContext();
+        var auth = MockAuth(context);
+        var manager = SetupUserManager(user);
+        var logger = new TestLogger<SignInManager<PocoUser>>();
+        var signInManager = new Mock<SignInManager<PocoUser>>(manager.Object,
+            new HttpContextAccessor { HttpContext = context },
+            new Mock<IUserClaimsPrincipalFactory<PocoUser>>().Object,
+            null, logger, new Mock<IAuthenticationSchemeProvider>().Object, null)
+        { CallBase = true };
+        auth.Setup(a => a.AuthenticateAsync(context, IdentityConstants.ApplicationScheme))
+            .Returns(Task.FromResult(AuthenticateResult.NoResult())).Verifiable();
+
+        await signInManager.Object.RefreshSignInAsync(user);
+
+        Assert.Contains("RefreshSignInAsync prevented because the user is not currently authenticated. Use SignInAsync instead for initial sign in.", logger.LogMessages);
+        auth.Verify();
+        signInManager.Verify(s => s.SignInWithClaimsAsync(It.IsAny<PocoUser>(), It.IsAny<AuthenticationProperties>(), It.IsAny<IEnumerable<Claim>>()),
+            Times.Never());
+    }
+
+    [Fact]
+    public async Task ResignInNoOpsAndLogsErrorIfAuthenticatedWithDifferentUser()
+    {
+        var user = new PocoUser { UserName = "Foo" };
+        var context = new DefaultHttpContext();
+        var auth = MockAuth(context);
+        var manager = SetupUserManager(user);
+        var logger = new TestLogger<SignInManager<PocoUser>>();
+        var signInManager = new Mock<SignInManager<PocoUser>>(manager.Object,
+            new HttpContextAccessor { HttpContext = context },
+            new Mock<IUserClaimsPrincipalFactory<PocoUser>>().Object,
+            null, logger, new Mock<IAuthenticationSchemeProvider>().Object, null)
+        { CallBase = true };
+        var id = new ClaimsIdentity("authscheme");
+        var claimsPrincipal = new ClaimsPrincipal(id);
+        var authResult = AuthenticateResult.Success(new AuthenticationTicket(claimsPrincipal, new AuthenticationProperties(), "authscheme"));
+        auth.Setup(a => a.AuthenticateAsync(context, IdentityConstants.ApplicationScheme))
+            .Returns(Task.FromResult(authResult)).Verifiable();
+        manager.Setup(m => m.GetUserId(claimsPrincipal)).Returns("different");
+
+        await signInManager.Object.RefreshSignInAsync(user);
+
+        Assert.Contains("RefreshSignInAsync prevented because currently authenticated user has a different UserId. Use SignInAsync instead to change users.", logger.LogMessages);
+        auth.Verify();
+        signInManager.Verify(s => s.SignInWithClaimsAsync(It.IsAny<PocoUser>(), It.IsAny<AuthenticationProperties>(), It.IsAny<IEnumerable<Claim>>()),
+            Times.Never());
+    }
+
     [Theory]
     [InlineData(true, true, true, true)]
     [InlineData(true, true, false, true)]