From f0a34a4ca46129b8434020dedb8c360e751e2963 Mon Sep 17 00:00:00 2001
From: BrennanConroy <brecon@microsoft.com>
Date: Thu, 20 Sep 2018 13:39:25 -0700
Subject: [PATCH] Clone Windows Identity in LongPolling connections (#2985)

---
 build/dependencies.props                      |  1 +
 .../Internal/HttpConnectionContext.cs         |  9 +++++
 .../Internal/HttpConnectionDispatcher.cs      | 40 +++++++++++++++----
 ...crosoft.AspNetCore.Http.Connections.csproj |  1 +
 4 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/build/dependencies.props b/build/dependencies.props
index 60617932b5..11bca983ea 100644
--- a/build/dependencies.props
+++ b/build/dependencies.props
@@ -69,6 +69,7 @@
     <SystemReactiveLinqPackageVersion>3.1.1</SystemReactiveLinqPackageVersion>
     <SystemReflectionEmitPackageVersion>4.3.0</SystemReflectionEmitPackageVersion>
     <SystemRuntimeCompilerServicesUnsafePackageVersion>4.5.1</SystemRuntimeCompilerServicesUnsafePackageVersion>
+    <SystemSecurityPrincipalWindowsPackageVersion>4.5.0</SystemSecurityPrincipalWindowsPackageVersion>
     <SystemThreadingChannelsPackageVersion>4.5.0</SystemThreadingChannelsPackageVersion>
     <SystemThreadingTasksExtensionsPackageVersion>4.5.1</SystemThreadingTasksExtensionsPackageVersion>
     <XunitAssertPackageVersion>2.3.1</XunitAssertPackageVersion>
diff --git a/src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionContext.cs b/src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionContext.cs
index 3b31756d86..fd9d22490a 100644
--- a/src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionContext.cs
+++ b/src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionContext.cs
@@ -6,6 +6,7 @@ using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.IO.Pipelines;
 using System.Security.Claims;
+using System.Security.Principal;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
@@ -208,6 +209,14 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
                 Cancellation?.Dispose();
 
                 Cancellation = null;
+
+                if (User != null && User.Identity is WindowsIdentity)
+                {
+                    foreach (var identity in User.Identities)
+                    {
+                        (identity as IDisposable)?.Dispose();
+                    }
+                }
             }
 
             await disposeTask;
diff --git a/src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionDispatcher.cs b/src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionDispatcher.cs
index 24e0090986..3b2d467673 100644
--- a/src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionDispatcher.cs
+++ b/src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionDispatcher.cs
@@ -7,11 +7,11 @@ using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
 using System.IO.Pipelines;
+using System.Security.Claims;
+using System.Security.Principal;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
-using Microsoft.AspNetCore.Connections.Features;
-using Microsoft.AspNetCore.Http.Connections.Features;
 using Microsoft.AspNetCore.Http.Connections.Internal.Transports;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Internal;
@@ -608,9 +608,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
                 return false;
             }
 
-            // Setup the connection state from the http context
-            connection.User = context.User;
-
             // Configure transport-specific features.
             if (transportType == HttpTransportType.LongPolling)
             {
@@ -630,7 +627,15 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
                 {
                     // Set the request trace identifier to the current http request handling the poll
                     existing.TraceIdentifier = context.TraceIdentifier;
-                    existing.User = context.User;
+
+                    // Don't copy the identity if it's a windows identity
+                    // We specifically clone the identity on first poll if it's a windows identity
+                    // If we swapped the new User here we'd have to dispose the old identities which could race with the application
+                    // trying to access the identity.
+                    if (context.User.Identity is WindowsIdentity)
+                    {
+                        existing.User = context.User;
+                    }
                 }
             }
             else
@@ -638,6 +643,9 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
                 connection.HttpContext = context;
             }
 
+            // Setup the connection state from the http context
+            connection.User = connection.HttpContext.User;
+
             // Set the Connection ID on the logging scope so that logs from now on will have the
             // Connection ID metadata set.
             logScope.ConnectionId = connection.ConnectionId;
@@ -645,6 +653,23 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
             return true;
         }
 
+        private static void CloneUser(HttpContext newContext, HttpContext oldContext)
+        {
+            if (oldContext.User.Identity is WindowsIdentity)
+            {
+                newContext.User = new ClaimsPrincipal();
+
+                foreach (var identity in oldContext.User.Identities)
+                {
+                    newContext.User.AddIdentity(identity.Clone());
+                }
+            }
+            else
+            {
+                newContext.User = oldContext.User;
+            }
+        }
+
         private static HttpContext CloneHttpContext(HttpContext context)
         {
             // The reason we're copying the base features instead of the HttpContext properties is
@@ -692,7 +717,8 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
 
             var newHttpContext = new DefaultHttpContext(features);
             newHttpContext.TraceIdentifier = context.TraceIdentifier;
-            newHttpContext.User = context.User;
+
+            CloneUser(newHttpContext, context);
 
             // Making request services function property could be tricky and expensive as it would require
             // DI scope per connection. It would also mean that services resolved in middleware leading up to here
diff --git a/src/Microsoft.AspNetCore.Http.Connections/Microsoft.AspNetCore.Http.Connections.csproj b/src/Microsoft.AspNetCore.Http.Connections/Microsoft.AspNetCore.Http.Connections.csproj
index 44cd96c75a..1cc7e47c44 100644
--- a/src/Microsoft.AspNetCore.Http.Connections/Microsoft.AspNetCore.Http.Connections.csproj
+++ b/src/Microsoft.AspNetCore.Http.Connections/Microsoft.AspNetCore.Http.Connections.csproj
@@ -29,6 +29,7 @@
     <PackageReference Include="Microsoft.Extensions.WebEncoders.Sources" Version="$(MicrosoftExtensionsWebEncodersSourcesPackageVersion)" PrivateAssets="All" />
     <PackageReference Include="Microsoft.Extensions.ValueStopwatch.Sources" Version="$(MicrosoftExtensionsValueStopwatchSourcesPackageVersion)" PrivateAssets="All" />
     <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonPackageVersion)" />
+    <PackageReference Include="System.Security.Principal.Windows" Version="$(SystemSecurityPrincipalWindowsPackageVersion)" />
   </ItemGroup>
 
 </Project>