From 468852550cfc467f5cb80bcf86e394cd48fe4e7c Mon Sep 17 00:00:00 2001
From: Hao Kung <haok@microsoft.com>
Date: Tue, 12 May 2015 15:49:49 -0700
Subject: [PATCH] Tweak  SecurityHelper.AddUserPrincipal logic

---
 .../SecurityHelper.cs                         | 20 ++++----
 .../SecurityHelperTests.cs                    | 47 +++++++++++++++++++
 2 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/src/Microsoft.AspNet.Authentication/SecurityHelper.cs b/src/Microsoft.AspNet.Authentication/SecurityHelper.cs
index 617fe14a16..5f5c765b39 100644
--- a/src/Microsoft.AspNet.Authentication/SecurityHelper.cs
+++ b/src/Microsoft.AspNet.Authentication/SecurityHelper.cs
@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System.Linq;
 using System.Security.Claims;
 using Microsoft.AspNet.Http;
 using Microsoft.Framework.Internal;
@@ -14,24 +15,23 @@ namespace Microsoft.AspNet.Authentication
     {
         /// <summary>
         /// Add all ClaimsIdenities from an additional ClaimPrincipal to the ClaimsPrincipal
+        /// Merges a new claims principal, placing all new identities first, and eliminating
+        /// any empty unauthenticated identities from context.User
         /// </summary>
         /// <param name="identity"></param>
         public static void AddUserPrincipal([NotNull] HttpContext context, [NotNull] ClaimsPrincipal principal)
         {
+            var newPrincipal = new ClaimsPrincipal();
+            // New principal identities go first
+            newPrincipal.AddIdentities(principal.Identities);
+
+            // Then add any existing non empty or authenticated identities
             var existingPrincipal = context.User;
             if (existingPrincipal != null)
             {
-                foreach (var existingClaimsIdentity in existingPrincipal.Identities)
-                {
-                    // REVIEW: No longer use auth type for anything, so we could remove this check, except for the default one HttpContext.user creates
-                    // REVIEW: Need to ignore any identities that did not come from an authentication scheme?
-                    if (existingClaimsIdentity.IsAuthenticated)
-                    {
-                        principal.AddIdentity(existingClaimsIdentity);
-                    }
-                }
+                newPrincipal.AddIdentities(existingPrincipal.Identities.Where(i => i.IsAuthenticated || i.Claims.Count() > 0));
             }
-            context.User = principal;
+            context.User = newPrincipal;
         }
     }
 }
diff --git a/test/Microsoft.AspNet.Authentication.Test/SecurityHelperTests.cs b/test/Microsoft.AspNet.Authentication.Test/SecurityHelperTests.cs
index 1de266682f..a02283ab76 100644
--- a/test/Microsoft.AspNet.Authentication.Test/SecurityHelperTests.cs
+++ b/test/Microsoft.AspNet.Authentication.Test/SecurityHelperTests.cs
@@ -56,5 +56,52 @@ namespace Microsoft.AspNet.Authentication
             principal.Identities.Skip(1).First().Name.ShouldBe("Test2");
             principal.Identities.Skip(2).First().Name.ShouldBe("Test1");
         }
+
+        [Fact]
+        public void AddingPreservesNewIdentitiesAndDropsEmpty()
+        {
+            var context = new DefaultHttpContext();
+            var existingPrincipal = new ClaimsPrincipal(new ClaimsIdentity());
+            var identityNoAuthTypeWithClaim = new ClaimsIdentity();
+            identityNoAuthTypeWithClaim.AddClaim(new Claim("identityNoAuthTypeWithClaim", "yes"));
+            existingPrincipal.AddIdentity(identityNoAuthTypeWithClaim);
+            var identityEmptyWithAuthType = new ClaimsIdentity("empty");
+            existingPrincipal.AddIdentity(identityEmptyWithAuthType);
+            context.User = existingPrincipal;
+
+            context.User.Identity.IsAuthenticated.ShouldBe(false);
+
+            var newPrincipal = new ClaimsPrincipal();
+            var newEmptyIdentity = new ClaimsIdentity();
+            var identityTwo = new ClaimsIdentity("yep");
+            newPrincipal.AddIdentity(newEmptyIdentity);
+            newPrincipal.AddIdentity(identityTwo);
+
+            SecurityHelper.AddUserPrincipal(context, newPrincipal);
+
+            // Preserve newPrincipal order
+            context.User.Identity.IsAuthenticated.ShouldBe(false);
+            context.User.Identity.Name.ShouldBe(null);
+
+            var principal = context.User;
+            principal.Identities.Count().ShouldBe(4);
+            principal.Identities.Skip(0).First().ShouldBe(newEmptyIdentity);
+            principal.Identities.Skip(1).First().ShouldBe(identityTwo);
+            principal.Identities.Skip(2).First().ShouldBe(identityNoAuthTypeWithClaim);
+            principal.Identities.Skip(3).First().ShouldBe(identityEmptyWithAuthType);
+
+            // This merge should drop newEmptyIdentity since its empty
+            SecurityHelper.AddUserPrincipal(context, new GenericPrincipal(new GenericIdentity("Test3", "Gamma"), new string[0]));
+
+            context.User.Identity.AuthenticationType.ShouldBe("Gamma");
+            context.User.Identity.Name.ShouldBe("Test3");
+
+            principal = context.User;
+            principal.Identities.Count().ShouldBe(4);
+            principal.Identities.Skip(0).First().Name.ShouldBe("Test3");
+            principal.Identities.Skip(1).First().ShouldBe(identityTwo);
+            principal.Identities.Skip(2).First().ShouldBe(identityNoAuthTypeWithClaim);
+            principal.Identities.Skip(3).First().ShouldBe(identityEmptyWithAuthType);
+        }
     }
 }