Merged PR 47274: Prevent RefreshSignInAsync if it is called with wrong user (#60881)

Author: wtgodbe

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)]