From 1df139eb6d8ef1c1e74a14dd20c9dd13eb6e0e23 Mon Sep 17 00:00:00 2001 From: "Chris Ross (ASP.NET)" Date: Fri, 2 Mar 2018 12:37:19 -0800 Subject: [PATCH] Clone tickets for sliding refresh #1607 --- .../CookieAuthenticationHandler.cs | 21 +++- .../CookieTests.cs | 116 +++++++++++++++++- 2 files changed, 134 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs b/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs index 9a2fbfbc74..5993f75325 100644 --- a/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs +++ b/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs @@ -31,6 +31,7 @@ namespace Microsoft.AspNetCore.Authentication.Cookies private DateTimeOffset? _refreshExpiresUtc; private string _sessionKey; private Task _readCookieTask; + private AuthenticationTicket _refreshTicket; public CookieAuthenticationHandler(IOptionsMonitor options, ILoggerFactory logger, UrlEncoder encoder, ISystemClock clock) : base(options, logger, encoder, clock) @@ -99,9 +100,27 @@ namespace Microsoft.AspNetCore.Authentication.Cookies _refreshIssuedUtc = currentUtc; var timeSpan = expiresUtc.Value.Subtract(issuedUtc.Value); _refreshExpiresUtc = currentUtc.Add(timeSpan); + _refreshTicket = CloneTicket(ticket); } } + private AuthenticationTicket CloneTicket(AuthenticationTicket ticket) + { + var newPrincipal = new ClaimsPrincipal(); + foreach (var identity in ticket.Principal.Identities) + { + newPrincipal.AddIdentity(identity.Clone()); + } + + var newProperties = new AuthenticationProperties(); + foreach (var item in ticket.Properties.Items) + { + newProperties.Items[item.Key] = item.Value; + } + + return new AuthenticationTicket(newPrincipal, newProperties, ticket.AuthenticationScheme); + } + private async Task ReadCookieTicket() { var cookie = Options.CookieManager.GetRequestCookie(Context, Options.Cookie.Name); @@ -190,7 +209,7 @@ namespace Microsoft.AspNetCore.Authentication.Cookies return; } - var ticket = (await HandleAuthenticateOnceSafeAsync())?.Ticket; + var ticket = _refreshTicket; if (ticket != null) { var properties = ticket.Properties; diff --git a/test/Microsoft.AspNetCore.Authentication.Test/CookieTests.cs b/test/Microsoft.AspNetCore.Authentication.Test/CookieTests.cs index b2726bac8c..945ec82ee6 100644 --- a/test/Microsoft.AspNetCore.Authentication.Test/CookieTests.cs +++ b/test/Microsoft.AspNetCore.Authentication.Test/CookieTests.cs @@ -515,8 +515,10 @@ namespace Microsoft.AspNetCore.Authentication.Cookies private Task SignInAsAlice(HttpContext context) { + var user = new ClaimsIdentity(new GenericIdentity("Alice", "Cookies")); + user.AddClaim(new Claim("marker", "true")); return context.SignInAsync("Cookies", - new ClaimsPrincipal(new ClaimsIdentity(new GenericIdentity("Alice", "Cookies"))), + new ClaimsPrincipal(user), new AuthenticationProperties()); } @@ -942,6 +944,61 @@ namespace Microsoft.AspNetCore.Authentication.Cookies Assert.Null(FindClaimValue(transaction5, ClaimTypes.Name)); } + [Fact] + public async Task CookieCanBeRenewedByValidatorWithModifiedProperties() + { + var server = CreateServer(o => + { + o.ExpireTimeSpan = TimeSpan.FromMinutes(10); + o.Events = new CookieAuthenticationEvents + { + OnValidatePrincipal = ctx => + { + ctx.ShouldRenew = true; + var id = ctx.Principal.Identities.First(); + var claim = id.FindFirst("counter"); + if (claim == null) + { + id.AddClaim(new Claim("counter", "1")); + } + else + { + id.RemoveClaim(claim); + id.AddClaim(new Claim("counter", claim.Value + "1")); + } + return Task.FromResult(0); + } + }; + }, + context => + context.SignInAsync("Cookies", + new ClaimsPrincipal(new ClaimsIdentity(new GenericIdentity("Alice", "Cookies"))))); + + var transaction1 = await SendAsync(server, "http://example.com/testpath"); + + var transaction2 = await SendAsync(server, "http://example.com/me/Cookies", transaction1.CookieNameValue); + Assert.NotNull(transaction2.SetCookie); + Assert.Equal("1", FindClaimValue(transaction2, "counter")); + + _clock.Add(TimeSpan.FromMinutes(5)); + + var transaction3 = await SendAsync(server, "http://example.com/me/Cookies", transaction2.CookieNameValue); + Assert.NotNull(transaction3.SetCookie); + Assert.Equal("11", FindClaimValue(transaction3, "counter")); + + _clock.Add(TimeSpan.FromMinutes(6)); + + var transaction4 = await SendAsync(server, "http://example.com/me/Cookies", transaction3.CookieNameValue); + Assert.NotNull(transaction4.SetCookie); + Assert.Equal("111", FindClaimValue(transaction4, "counter")); + + _clock.Add(TimeSpan.FromMinutes(11)); + + var transaction5 = await SendAsync(server, "http://example.com/me/Cookies", transaction4.CookieNameValue); + Assert.Null(transaction5.SetCookie); + Assert.Null(FindClaimValue(transaction5, "counter")); + } + [Fact] public async Task CookieValidatorOnlyCalledOnce() { @@ -1114,6 +1171,51 @@ namespace Microsoft.AspNetCore.Authentication.Cookies Assert.Equal("Alice", FindClaimValue(transaction5, ClaimTypes.Name)); } + [Fact] + public async Task CookieIsRenewedWithSlidingExpirationWithoutTransformations() + { + var server = CreateServer(o => + { + o.ExpireTimeSpan = TimeSpan.FromMinutes(10); + o.SlidingExpiration = true; + o.Events.OnValidatePrincipal = c => + { + // https://github.com/aspnet/Security/issues/1607 + // On sliding refresh the transformed principal should not be serialized into the cookie, only the original principal. + Assert.Single(c.Principal.Identities); + Assert.True(c.Principal.Identities.First().HasClaim("marker", "true")); + return Task.CompletedTask; + }; + }, + SignInAsAlice, + claimsTransform: true); + + var transaction1 = await SendAsync(server, "http://example.com/testpath"); + + var transaction2 = await SendAsync(server, "http://example.com/me/Cookies", transaction1.CookieNameValue); + Assert.Null(transaction2.SetCookie); + Assert.Equal("Alice", FindClaimValue(transaction2, ClaimTypes.Name)); + + _clock.Add(TimeSpan.FromMinutes(4)); + + var transaction3 = await SendAsync(server, "http://example.com/me/Cookies", transaction1.CookieNameValue); + Assert.Null(transaction3.SetCookie); + Assert.Equal("Alice", FindClaimValue(transaction3, ClaimTypes.Name)); + + _clock.Add(TimeSpan.FromMinutes(4)); + + // transaction4 should arrive with a new SetCookie value + var transaction4 = await SendAsync(server, "http://example.com/me/Cookies", transaction1.CookieNameValue); + Assert.NotNull(transaction4.SetCookie); + Assert.Equal("Alice", FindClaimValue(transaction4, ClaimTypes.Name)); + + _clock.Add(TimeSpan.FromMinutes(4)); + + var transaction5 = await SendAsync(server, "http://example.com/me/Cookies", transaction4.CookieNameValue); + Assert.Null(transaction5.SetCookie); + Assert.Equal("Alice", FindClaimValue(transaction5, ClaimTypes.Name)); + } + [Fact] public async Task CookieUsesPathBaseByDefault() { @@ -1643,6 +1745,13 @@ namespace Microsoft.AspNetCore.Authentication.Cookies { public Task TransformAsync(ClaimsPrincipal p) { + var firstId = p.Identities.First(); + if (firstId.HasClaim("marker", "true")) + { + firstId.RemoveClaim(firstId.FindFirst("marker")); + } + // TransformAsync could be called twice on one request if you have a default scheme and also + // call AuthenticateAsync. if (!p.Identities.Any(i => i.AuthenticationType == "xform")) { var id = new ClaimsIdentity("xform"); @@ -1658,7 +1767,10 @@ namespace Microsoft.AspNetCore.Authentication.Cookies { s.AddSingleton(_clock); s.AddAuthentication(CookieAuthenticationDefaults.AuthenticationScheme).AddCookie(configureOptions); - s.AddSingleton(); + if (claimsTransform) + { + s.AddSingleton(); + } }, testpath, baseAddress); private static TestServer CreateServerWithServices(Action configureServices, Func testpath = null, Uri baseAddress = null)