From 2d522fbb02ff49247d75e0b169cd0bb0ddbf6df7 Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Mon, 24 Feb 2020 17:12:26 -0800 Subject: [PATCH 1/3] Create WindowsPrincipal when cloning Identity for SignalR --- .../src/Internal/HttpConnectionDispatcher.cs | 18 ++++++- .../test/HttpConnectionDispatcherTests.cs | 53 ++++++++++++++++++- 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs index c40a80b9ce97..b9f97fd05d72 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs @@ -572,12 +572,26 @@ private async Task EnsureConnectionStateAsync(HttpConnectionContext connec private static void CloneUser(HttpContext newContext, HttpContext oldContext) { - if (oldContext.User.Identity is WindowsIdentity) + if (oldContext.User.Identity is WindowsIdentity windowsIdentity) { - newContext.User = new ClaimsPrincipal(); + var skipFirstIdentity = false; + if (oldContext.User is WindowsPrincipal) + { + newContext.User = new WindowsPrincipal((WindowsIdentity)(windowsIdentity.Clone())); + skipFirstIdentity = true; + } + else + { + newContext.User = new ClaimsPrincipal(); + } foreach (var identity in oldContext.User.Identities) { + if (skipFirstIdentity) + { + skipFirstIdentity = false; + continue; + } newContext.User.AddIdentity(identity.Clone()); } } diff --git a/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs b/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs index 2a083c598264..f6beccb2cd83 100644 --- a/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs +++ b/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs @@ -1639,7 +1639,7 @@ public async Task TransferModeSet(HttpTransportType transportType, TransferForma [ConditionalFact] [OSSkipCondition(OperatingSystems.Linux | OperatingSystems.MacOSX)] - public async Task LongPollingKeepsWindowsIdentityBetweenRequests() + public async Task LongPollingKeepsWindowsPrincipalAndIdentityBetweenRequests() { using (StartVerifiableLog()) { @@ -1681,6 +1681,57 @@ public async Task LongPollingKeepsWindowsIdentityBetweenRequests() // This is the important check Assert.Same(currentUser, connection.User); + Assert.IsType(currentUser); + + Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode); + } + } + + [ConditionalFact] + [OSSkipCondition(OperatingSystems.Linux | OperatingSystems.MacOSX)] + public async Task LongPollingKeepsWindowsIdentityWithoutWindowsPrincipalBetweenRequests() + { + using (StartVerifiableLog()) + { + var manager = CreateConnectionManager(LoggerFactory); + var connection = manager.CreateConnection(); + connection.TransportType = HttpTransportType.LongPolling; + var dispatcher = new HttpConnectionDispatcher(manager, LoggerFactory); + var context = new DefaultHttpContext(); + var services = new ServiceCollection(); + services.AddOptions(); + services.AddSingleton(); + services.AddLogging(); + var sp = services.BuildServiceProvider(); + context.Request.Path = "/foo"; + context.Request.Method = "GET"; + context.RequestServices = sp; + var values = new Dictionary(); + values["id"] = connection.ConnectionToken; + values["negotiateVersion"] = "1"; + var qs = new QueryCollection(values); + context.Request.Query = qs; + var builder = new ConnectionBuilder(sp); + builder.UseConnectionHandler(); + var app = builder.Build(); + var options = new HttpConnectionDispatcherOptions(); + + var windowsIdentity = WindowsIdentity.GetAnonymous(); + context.User = new ClaimsPrincipal(windowsIdentity); + + // would get stuck if EndPoint was running + await dispatcher.ExecuteAsync(context, options, app).OrTimeout(); + + Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode); + var currentUser = connection.User; + + var connectionHandlerTask = dispatcher.ExecuteAsync(context, options, app); + await connection.Transport.Output.WriteAsync(Encoding.UTF8.GetBytes("Unblock")).AsTask().OrTimeout(); + await connectionHandlerTask.OrTimeout(); + + // This is the important check + Assert.Same(currentUser, connection.User); + Assert.IsNotType(currentUser); Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode); } From 51f2af277f54344f63ac168793fd8fb1fc862586 Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Tue, 25 Feb 2020 10:15:32 -0800 Subject: [PATCH 2/3] update test --- .../Http.Connections/test/HttpConnectionDispatcherTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs b/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs index f6beccb2cd83..664a79432473 100644 --- a/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs +++ b/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs @@ -1668,6 +1668,7 @@ public async Task LongPollingKeepsWindowsPrincipalAndIdentityBetweenRequests() var windowsIdentity = WindowsIdentity.GetAnonymous(); context.User = new WindowsPrincipal(windowsIdentity); + context.User.AddIdentity(new ClaimsIdentity()); // would get stuck if EndPoint was running await dispatcher.ExecuteAsync(context, options, app).OrTimeout(); @@ -1682,6 +1683,7 @@ public async Task LongPollingKeepsWindowsPrincipalAndIdentityBetweenRequests() // This is the important check Assert.Same(currentUser, connection.User); Assert.IsType(currentUser); + Assert.Equal(2, connection.User.Identities.Count()); Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode); } @@ -1718,6 +1720,7 @@ public async Task LongPollingKeepsWindowsIdentityWithoutWindowsPrincipalBetweenR var windowsIdentity = WindowsIdentity.GetAnonymous(); context.User = new ClaimsPrincipal(windowsIdentity); + context.User.AddIdentity(new ClaimsIdentity()); // would get stuck if EndPoint was running await dispatcher.ExecuteAsync(context, options, app).OrTimeout(); @@ -1732,6 +1735,7 @@ public async Task LongPollingKeepsWindowsIdentityWithoutWindowsPrincipalBetweenR // This is the important check Assert.Same(currentUser, connection.User); Assert.IsNotType(currentUser); + Assert.Equal(2, connection.User.Identities.Count()); Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode); } From 3340f62bdd48eba74439bd0d97fd1e8d6fb39d72 Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Tue, 25 Feb 2020 10:22:42 -0800 Subject: [PATCH 3/3] comments --- .../src/Internal/HttpConnectionDispatcher.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs index b9f97fd05d72..6d685f18d54b 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs @@ -572,11 +572,16 @@ private async Task EnsureConnectionStateAsync(HttpConnectionContext connec private static void CloneUser(HttpContext newContext, HttpContext oldContext) { + // If the identity is a WindowsIdentity we need to clone the User. + // This is because the WindowsIdentity uses SafeHandle's which are disposed at the end of the request + // and accessing the identity can happen outside of the request scope. if (oldContext.User.Identity is WindowsIdentity windowsIdentity) { var skipFirstIdentity = false; if (oldContext.User is WindowsPrincipal) { + // We want to explicitly create a WindowsPrincipal instead of a ClaimsPrincipal + // so methods that WindowsPrincipal overrides like 'IsInRole', work as expected. newContext.User = new WindowsPrincipal((WindowsIdentity)(windowsIdentity.Clone())); skipFirstIdentity = true; }