diff --git a/IdentityServer/v7/SessionMigration/HostingExtensions.cs b/IdentityServer/v7/SessionMigration/HostingExtensions.cs index 771aa0b9..b9f3a8d7 100644 --- a/IdentityServer/v7/SessionMigration/HostingExtensions.cs +++ b/IdentityServer/v7/SessionMigration/HostingExtensions.cs @@ -1,7 +1,9 @@ // Copyright (c) Duende Software. All rights reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. +using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.DataProtection; +using Microsoft.Extensions.Options; namespace SessionMigration; diff --git a/IdentityServer/v7/SessionMigration/MigratingTicketDataFormat.cs b/IdentityServer/v7/SessionMigration/MigratingTicketDataFormat.cs index 332d09fc..0bc79a1e 100644 --- a/IdentityServer/v7/SessionMigration/MigratingTicketDataFormat.cs +++ b/IdentityServer/v7/SessionMigration/MigratingTicketDataFormat.cs @@ -10,13 +10,13 @@ namespace SessionMigration; public class MigratingTicketDataFormat : ISecureDataFormat { - private readonly IHttpContextAccessor httpContextAccessor; - private readonly ISecureDataFormat inner; - private CookieAuthenticationOptions options; - private readonly string scheme; + private readonly IHttpContextAccessor _httpContextAccessor; + private readonly ISecureDataFormat _inner; + private readonly CookieAuthenticationOptions _options; + private readonly string _scheme; - // Copied from Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler, - // unfortunatley it's private and cannot be referenced. + // Copied from Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler. + // Unfortunately, it's private and cannot be referenced. private const string SessionIdClaim = "Microsoft.AspNetCore.Authentication.Cookies-SessionId"; public MigratingTicketDataFormat( @@ -24,21 +24,21 @@ public MigratingTicketDataFormat( CookieAuthenticationOptions options, string scheme) { - this.httpContextAccessor = httpContextAccessor; - this.options = options; - this.scheme = scheme; + _httpContextAccessor = httpContextAccessor; + _options = options; + _scheme = scheme; // Capture the inner at construction as the value in options will be replaced with // a reference to this instance. - inner = options.TicketDataFormat; + _inner = options.TicketDataFormat; } - public string Protect(AuthenticationTicket data) => inner.Protect(data); - public string Protect(AuthenticationTicket data, string purpose) => inner.Protect(data, purpose); - public AuthenticationTicket Unprotect(string protectedText) => inner.Unprotect(protectedText); + public string Protect(AuthenticationTicket data) => _inner.Protect(data); + public string Protect(AuthenticationTicket data, string purpose) => _inner.Protect(data, purpose); + public AuthenticationTicket Unprotect(string protectedText) => _inner.Unprotect(protectedText); public AuthenticationTicket Unprotect(string protectedText, string purpose) { - var ticket = inner.Unprotect(protectedText, purpose); + var ticket = _inner.Unprotect(protectedText, purpose); if (ticket.Principal.HasClaim(c => c.Type == SessionIdClaim)) { @@ -46,38 +46,69 @@ public AuthenticationTicket Unprotect(string protectedText, string purpose) return ticket; } - var context = httpContextAccessor.HttpContext; + var context = _httpContextAccessor.HttpContext; var sessionStore = context.RequestServices.GetRequiredService(); - // Unprotect isn't async so we'll have block the thread and wait using .Result when calling an - // async methods. This is bad for performance, but this only happens once per logged in user - // session so it should be acceptable. Thanks to Asp.Net Core not having any synchronization - // context there is no risk for dead locks. - var sessionId = sessionStore.StoreAsync(ticket).Result; - - // There's a potential race condition where two requests could migrate the same session. Check - // if there's another entry with the same SID and if it is rollback the one we created and - // don't alter the cookie. + // Unprotect isn't async so we have to block synchronously. ISecureDataFormat is a + // synchronous interface defined by ASP.NET Core and cannot be changed. Thanks to ASP.NET + // Core not having a synchronization context there is no risk for deadlocks. + // + // NOTE: In a real implementation with many concurrent users migrating at once (e.g. after + // a deployment), blocking here could still cause thread pool starvation. Consider whether your + // scale warrants an alternative approach, such as splitting the migration into two phases: + // 1. Detect the old cookie-based session in Unprotect and flag the request (e.g. via + // HttpContext.Items) without doing any I/O. + // 2. In async middleware further up the pipeline, check the flag, perform the actual + // store write, and rewrite the cookie — all fully async. + // This avoids sync-over-async entirely at the cost of more architectural complexity. + var sessionId = sessionStore.StoreAsync(ticket).GetAwaiter().GetResult(); + + // There's a potential race condition where two requests could migrate the same session. + // Check if there's another entry with the same SID and if so, rollback the one we + // created and don't alter the cookie. + // + // NOTE: This check has a TOCTOU (time-of-check-time-of-use) issue. Two concurrent + // requests could both store a session, both detect the duplicate, and both roll back, + // leaving no migrated session. This is self-healing (the next request will retry), but + // in a real implementation you may want to use a distributed lock or compare creation + // timestamps to decide which duplicate to keep. if (HasDuplicate(sessionStore, ticket)) { - sessionStore.RemoveAsync(sessionId).Wait(); + sessionStore.RemoveAsync(sessionId).GetAwaiter().GetResult(); } else { + // Carry over relevant properties from the original ticket so the reference ticket + // (and the cookie) accurately reflects the original session state. Without this, + // persistent sessions would be downgraded to session cookies, and any downstream + // code inspecting properties on this request would see empty defaults. + var properties = new AuthenticationProperties + { + IsPersistent = ticket.Properties.IsPersistent, + IssuedUtc = ticket.Properties.IssuedUtc, + ExpiresUtc = ticket.Properties.ExpiresUtc, + AllowRefresh = ticket.Properties.AllowRefresh, + // NOTE: If your application stores custom data in Properties.Items or + // Properties.Parameters, you may want to copy those over as well. + }; + var principal = new ClaimsPrincipal( new ClaimsIdentity( - new[] { new Claim(SessionIdClaim, sessionId, ClaimValueTypes.String, options.ClaimsIssuer) }, - options.ClaimsIssuer)); + new[] { new Claim(SessionIdClaim, sessionId, ClaimValueTypes.String, _options.ClaimsIssuer) }, + _options.ClaimsIssuer)); - ticket = new AuthenticationTicket(principal, null, scheme); + ticket = new AuthenticationTicket(principal, properties, _scheme); - var cookieValue = inner.Protect(ticket, purpose); + var cookieValue = _inner.Protect(ticket, purpose); + // NOTE: AppendResponseCookie will throw if the response has already started + // (e.g. if headers have been sent). In a real implementation, guard against this + // or ensure migration runs early enough in the pipeline. var cookieOptions = CreateCookieOptions(ticket, context); - options.CookieManager.AppendResponseCookie( + _options.CookieManager.AppendResponseCookie( context, - options.Cookie.Name!, + _options.Cookie.Name!, cookieValue, cookieOptions); } @@ -88,28 +119,13 @@ private CookieOptions CreateCookieOptions(AuthenticationTicket ticket, HttpConte { // Cookie option generation copied from cookie handler. - var cookieOptions = options.Cookie.Build(context); + var cookieOptions = _options.Cookie.Build(context); cookieOptions.Expires = null; if (ticket.Properties.IsPersistent) { - DateTimeOffset issuedUtc; - if (ticket.Properties.IssuedUtc.HasValue) - { - issuedUtc = ticket.Properties.IssuedUtc.Value; - } - else - { - issuedUtc = DateTime.UtcNow; - ticket.Properties.IssuedUtc = issuedUtc; - } - - if (!ticket.Properties.ExpiresUtc.HasValue) - { - ticket.Properties.ExpiresUtc = issuedUtc.Add(options.ExpireTimeSpan); - } - - var expiresUtc = ticket.Properties.ExpiresUtc ?? issuedUtc.Add(options.ExpireTimeSpan); + var issuedUtc = ticket.Properties.IssuedUtc ?? DateTimeOffset.UtcNow; + var expiresUtc = ticket.Properties.ExpiresUtc ?? issuedUtc.Add(_options.ExpireTimeSpan); cookieOptions.Expires = expiresUtc.ToUniversalTime(); } @@ -125,7 +141,9 @@ private bool HasDuplicate(IServerSideTicketStore sessionStore, AuthenticationTic SessionId = sid }; - var sessions = sessionStore.QuerySessionsAsync(filter).Result; + var sessions = sessionStore.QuerySessionsAsync(filter, + _httpContextAccessor.HttpContext?.RequestAborted ?? CancellationToken.None) + .GetAwaiter().GetResult(); // There should be only one entry, the one we just created. return sessions.Results.Count > 1; diff --git a/IdentityServer/v7/SessionMigration/README.md b/IdentityServer/v7/SessionMigration/README.md new file mode 100644 index 00000000..3419c71f --- /dev/null +++ b/IdentityServer/v7/SessionMigration/README.md @@ -0,0 +1,28 @@ +# Session Migration sample + +## Instructions + +The purpose of this sample is to show how to migrate existing client side session to server +side session without the user having to login again. To test it, first run the sample as is +and log in (user: alice, password: alice) to get a normal cookie based session in your +browser. Then uncomment the following blocks and run the sample again. The first request +from your browser to IdentityServer will cause the session to be migrated to be a server +side session. + +Note that further restarts will invalidate your session as the server side session +store in this sample is in memory only. +If server side sessions have been enabled and then are removed before another +test run, you have to manually clear the cookie to be able to log in again. + +This is the normal template code to activate server-side sessions: +```csharp +var isBuilder = builder.Services.AddIdentityServer(...); + +isBuilder.AddServerSideSessions(); +``` + +And this code adds migration of sessions. Enabling server-side sessions without adding this migration, will +invalidate all existing sessions. +```csharp +builder.Services.AddTransient, SessionMigrationPostConfigureOptions>(); +``` diff --git a/IdentityServer/v7/SessionMigration/SessionMigrationPostConfigureOptions.cs b/IdentityServer/v7/SessionMigration/SessionMigrationPostConfigureOptions.cs index 8fcb40a5..c16431bd 100644 --- a/IdentityServer/v7/SessionMigration/SessionMigrationPostConfigureOptions.cs +++ b/IdentityServer/v7/SessionMigration/SessionMigrationPostConfigureOptions.cs @@ -9,11 +9,11 @@ namespace SessionMigration; public class SessionMigrationPostConfigureOptions : IPostConfigureOptions { - private readonly IHttpContextAccessor httpContextAccessor; + private readonly IHttpContextAccessor _httpContextAccessor; public SessionMigrationPostConfigureOptions(IHttpContextAccessor httpContextAccessor) { - this.httpContextAccessor = httpContextAccessor; + _httpContextAccessor = httpContextAccessor; } public void PostConfigure(string name, CookieAuthenticationOptions options) @@ -31,6 +31,6 @@ public void PostConfigure(string name, CookieAuthenticationOptions options) // TicketDataFormat is not injected through DI so we have to supply it with an http context accessor // to be able to resolve request services at runtime. - options.TicketDataFormat = new MigratingTicketDataFormat(httpContextAccessor, options, name); + options.TicketDataFormat = new MigratingTicketDataFormat(_httpContextAccessor, options, name); } } diff --git a/IdentityServer/v8/SessionMigration/HostingExtensions.cs b/IdentityServer/v8/SessionMigration/HostingExtensions.cs index 771aa0b9..b9f3a8d7 100644 --- a/IdentityServer/v8/SessionMigration/HostingExtensions.cs +++ b/IdentityServer/v8/SessionMigration/HostingExtensions.cs @@ -1,7 +1,9 @@ // Copyright (c) Duende Software. All rights reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. +using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.DataProtection; +using Microsoft.Extensions.Options; namespace SessionMigration; diff --git a/IdentityServer/v8/SessionMigration/MigratingTicketDataFormat.cs b/IdentityServer/v8/SessionMigration/MigratingTicketDataFormat.cs index 4806db0b..0bc79a1e 100644 --- a/IdentityServer/v8/SessionMigration/MigratingTicketDataFormat.cs +++ b/IdentityServer/v8/SessionMigration/MigratingTicketDataFormat.cs @@ -10,13 +10,13 @@ namespace SessionMigration; public class MigratingTicketDataFormat : ISecureDataFormat { - private readonly IHttpContextAccessor httpContextAccessor; - private readonly ISecureDataFormat inner; - private CookieAuthenticationOptions options; - private readonly string scheme; + private readonly IHttpContextAccessor _httpContextAccessor; + private readonly ISecureDataFormat _inner; + private readonly CookieAuthenticationOptions _options; + private readonly string _scheme; - // Copied from Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler, - // unfortunatley it's private and cannot be referenced. + // Copied from Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler. + // Unfortunately, it's private and cannot be referenced. private const string SessionIdClaim = "Microsoft.AspNetCore.Authentication.Cookies-SessionId"; public MigratingTicketDataFormat( @@ -24,21 +24,21 @@ public MigratingTicketDataFormat( CookieAuthenticationOptions options, string scheme) { - this.httpContextAccessor = httpContextAccessor; - this.options = options; - this.scheme = scheme; + _httpContextAccessor = httpContextAccessor; + _options = options; + _scheme = scheme; // Capture the inner at construction as the value in options will be replaced with // a reference to this instance. - inner = options.TicketDataFormat; + _inner = options.TicketDataFormat; } - public string Protect(AuthenticationTicket data) => inner.Protect(data); - public string Protect(AuthenticationTicket data, string purpose) => inner.Protect(data, purpose); - public AuthenticationTicket Unprotect(string protectedText) => inner.Unprotect(protectedText); + public string Protect(AuthenticationTicket data) => _inner.Protect(data); + public string Protect(AuthenticationTicket data, string purpose) => _inner.Protect(data, purpose); + public AuthenticationTicket Unprotect(string protectedText) => _inner.Unprotect(protectedText); public AuthenticationTicket Unprotect(string protectedText, string purpose) { - var ticket = inner.Unprotect(protectedText, purpose); + var ticket = _inner.Unprotect(protectedText, purpose); if (ticket.Principal.HasClaim(c => c.Type == SessionIdClaim)) { @@ -46,38 +46,69 @@ public AuthenticationTicket Unprotect(string protectedText, string purpose) return ticket; } - var context = httpContextAccessor.HttpContext; + var context = _httpContextAccessor.HttpContext; var sessionStore = context.RequestServices.GetRequiredService(); - // Unprotect isn't async so we'll have block the thread and wait using .Result when calling an - // async methods. This is bad for performance, but this only happens once per logged in user - // session so it should be acceptable. Thanks to Asp.Net Core not having any synchronization - // context there is no risk for dead locks. - var sessionId = sessionStore.StoreAsync(ticket).Result; - - // There's a potential race condition where two requests could migrate the same session. Check - // if there's another entry with the same SID and if it is rollback the one we created and - // don't alter the cookie. + // Unprotect isn't async so we have to block synchronously. ISecureDataFormat is a + // synchronous interface defined by ASP.NET Core and cannot be changed. Thanks to ASP.NET + // Core not having a synchronization context there is no risk for deadlocks. + // + // NOTE: In a real implementation with many concurrent users migrating at once (e.g. after + // a deployment), blocking here could still cause thread pool starvation. Consider whether your + // scale warrants an alternative approach, such as splitting the migration into two phases: + // 1. Detect the old cookie-based session in Unprotect and flag the request (e.g. via + // HttpContext.Items) without doing any I/O. + // 2. In async middleware further up the pipeline, check the flag, perform the actual + // store write, and rewrite the cookie — all fully async. + // This avoids sync-over-async entirely at the cost of more architectural complexity. + var sessionId = sessionStore.StoreAsync(ticket).GetAwaiter().GetResult(); + + // There's a potential race condition where two requests could migrate the same session. + // Check if there's another entry with the same SID and if so, rollback the one we + // created and don't alter the cookie. + // + // NOTE: This check has a TOCTOU (time-of-check-time-of-use) issue. Two concurrent + // requests could both store a session, both detect the duplicate, and both roll back, + // leaving no migrated session. This is self-healing (the next request will retry), but + // in a real implementation you may want to use a distributed lock or compare creation + // timestamps to decide which duplicate to keep. if (HasDuplicate(sessionStore, ticket)) { - sessionStore.RemoveAsync(sessionId).Wait(); + sessionStore.RemoveAsync(sessionId).GetAwaiter().GetResult(); } else { + // Carry over relevant properties from the original ticket so the reference ticket + // (and the cookie) accurately reflects the original session state. Without this, + // persistent sessions would be downgraded to session cookies, and any downstream + // code inspecting properties on this request would see empty defaults. + var properties = new AuthenticationProperties + { + IsPersistent = ticket.Properties.IsPersistent, + IssuedUtc = ticket.Properties.IssuedUtc, + ExpiresUtc = ticket.Properties.ExpiresUtc, + AllowRefresh = ticket.Properties.AllowRefresh, + // NOTE: If your application stores custom data in Properties.Items or + // Properties.Parameters, you may want to copy those over as well. + }; + var principal = new ClaimsPrincipal( new ClaimsIdentity( - new[] { new Claim(SessionIdClaim, sessionId, ClaimValueTypes.String, options.ClaimsIssuer) }, - options.ClaimsIssuer)); + new[] { new Claim(SessionIdClaim, sessionId, ClaimValueTypes.String, _options.ClaimsIssuer) }, + _options.ClaimsIssuer)); - ticket = new AuthenticationTicket(principal, null, scheme); + ticket = new AuthenticationTicket(principal, properties, _scheme); - var cookieValue = inner.Protect(ticket, purpose); + var cookieValue = _inner.Protect(ticket, purpose); + // NOTE: AppendResponseCookie will throw if the response has already started + // (e.g. if headers have been sent). In a real implementation, guard against this + // or ensure migration runs early enough in the pipeline. var cookieOptions = CreateCookieOptions(ticket, context); - options.CookieManager.AppendResponseCookie( + _options.CookieManager.AppendResponseCookie( context, - options.Cookie.Name!, + _options.Cookie.Name!, cookieValue, cookieOptions); } @@ -88,28 +119,13 @@ private CookieOptions CreateCookieOptions(AuthenticationTicket ticket, HttpConte { // Cookie option generation copied from cookie handler. - var cookieOptions = options.Cookie.Build(context); + var cookieOptions = _options.Cookie.Build(context); cookieOptions.Expires = null; if (ticket.Properties.IsPersistent) { - DateTimeOffset issuedUtc; - if (ticket.Properties.IssuedUtc.HasValue) - { - issuedUtc = ticket.Properties.IssuedUtc.Value; - } - else - { - issuedUtc = DateTime.UtcNow; - ticket.Properties.IssuedUtc = issuedUtc; - } - - if (!ticket.Properties.ExpiresUtc.HasValue) - { - ticket.Properties.ExpiresUtc = issuedUtc.Add(options.ExpireTimeSpan); - } - - var expiresUtc = ticket.Properties.ExpiresUtc ?? issuedUtc.Add(options.ExpireTimeSpan); + var issuedUtc = ticket.Properties.IssuedUtc ?? DateTimeOffset.UtcNow; + var expiresUtc = ticket.Properties.ExpiresUtc ?? issuedUtc.Add(_options.ExpireTimeSpan); cookieOptions.Expires = expiresUtc.ToUniversalTime(); } @@ -125,7 +141,9 @@ private bool HasDuplicate(IServerSideTicketStore sessionStore, AuthenticationTic SessionId = sid }; - var sessions = sessionStore.QuerySessionsAsync(filter, httpContextAccessor.HttpContext?.RequestAborted ?? CancellationToken.None).Result; + var sessions = sessionStore.QuerySessionsAsync(filter, + _httpContextAccessor.HttpContext?.RequestAborted ?? CancellationToken.None) + .GetAwaiter().GetResult(); // There should be only one entry, the one we just created. return sessions.Results.Count > 1; diff --git a/IdentityServer/v8/SessionMigration/README.md b/IdentityServer/v8/SessionMigration/README.md new file mode 100644 index 00000000..3419c71f --- /dev/null +++ b/IdentityServer/v8/SessionMigration/README.md @@ -0,0 +1,28 @@ +# Session Migration sample + +## Instructions + +The purpose of this sample is to show how to migrate existing client side session to server +side session without the user having to login again. To test it, first run the sample as is +and log in (user: alice, password: alice) to get a normal cookie based session in your +browser. Then uncomment the following blocks and run the sample again. The first request +from your browser to IdentityServer will cause the session to be migrated to be a server +side session. + +Note that further restarts will invalidate your session as the server side session +store in this sample is in memory only. +If server side sessions have been enabled and then are removed before another +test run, you have to manually clear the cookie to be able to log in again. + +This is the normal template code to activate server-side sessions: +```csharp +var isBuilder = builder.Services.AddIdentityServer(...); + +isBuilder.AddServerSideSessions(); +``` + +And this code adds migration of sessions. Enabling server-side sessions without adding this migration, will +invalidate all existing sessions. +```csharp +builder.Services.AddTransient, SessionMigrationPostConfigureOptions>(); +``` diff --git a/IdentityServer/v8/SessionMigration/SessionMigrationPostConfigureOptions.cs b/IdentityServer/v8/SessionMigration/SessionMigrationPostConfigureOptions.cs index 8fcb40a5..c16431bd 100644 --- a/IdentityServer/v8/SessionMigration/SessionMigrationPostConfigureOptions.cs +++ b/IdentityServer/v8/SessionMigration/SessionMigrationPostConfigureOptions.cs @@ -9,11 +9,11 @@ namespace SessionMigration; public class SessionMigrationPostConfigureOptions : IPostConfigureOptions { - private readonly IHttpContextAccessor httpContextAccessor; + private readonly IHttpContextAccessor _httpContextAccessor; public SessionMigrationPostConfigureOptions(IHttpContextAccessor httpContextAccessor) { - this.httpContextAccessor = httpContextAccessor; + _httpContextAccessor = httpContextAccessor; } public void PostConfigure(string name, CookieAuthenticationOptions options) @@ -31,6 +31,6 @@ public void PostConfigure(string name, CookieAuthenticationOptions options) // TicketDataFormat is not injected through DI so we have to supply it with an http context accessor // to be able to resolve request services at runtime. - options.TicketDataFormat = new MigratingTicketDataFormat(httpContextAccessor, options, name); + options.TicketDataFormat = new MigratingTicketDataFormat(_httpContextAccessor, options, name); } }