From 12a11a60b12ca57a59caf1e22f99ed847e14b579 Mon Sep 17 00:00:00 2001 From: Mole Date: Fri, 18 Feb 2022 11:40:13 +0100 Subject: [PATCH 1/7] Turn SlidingExpiration off and only renew cookie of not RemainingSeconds request Also adds the TicketExpiresClaim before validating the the security stamp, otherwise the claim won't be merged and "dissappear", leading to the user being instantly logged out Also only EnsureValidSessionId if not RemainingSeconds request, otherwise the session will always be valid, since the remaining seconds request renews it. --- .../Extensions/ClaimsIdentityExtensions.cs | 2 +- .../ConfigureBackOfficeCookieOptions.cs | 31 +++++++++++++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs index bceddf1fd6bf..0319be32979b 100644 --- a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs @@ -132,7 +132,7 @@ public static bool VerifyBackOfficeIdentity(this ClaimsIdentity identity, out Cl } } - verifiedIdentity = identity.AuthenticationType == Constants.Security.BackOfficeAuthenticationType ? identity : new ClaimsIdentity(identity.Claims, Constants.Security.BackOfficeAuthenticationType); + verifiedIdentity = identity.AuthenticationType == Constants.Security.BackOfficeAuthenticationType ? identity : new ClaimsIdentity(identity.Claims, Constants.Security.BackOfficeAuthenticationType); return true; } diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 58a686230028..266a6992b58e 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -15,6 +15,7 @@ using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Web; +using Umbraco.Cms.Web.BackOffice.Controllers; using Umbraco.Extensions; namespace Umbraco.Cms.Web.BackOffice.Security @@ -92,7 +93,7 @@ public void Configure(string name, CookieAuthenticationOptions options) /// public void Configure(CookieAuthenticationOptions options) { - options.SlidingExpiration = true; + options.SlidingExpiration = false; options.ExpireTimeSpan = _globalSettings.TimeOut; options.Cookie.Domain = _securitySettings.AuthCookieDomain; options.Cookie.Name = _securitySettings.AuthCookieName; @@ -150,8 +151,6 @@ public void Configure(CookieAuthenticationOptions options) // ensure the thread culture is set backOfficeIdentity.EnsureCulture(); - await EnsureValidSessionId(ctx); - await securityStampValidator.ValidateAsync(ctx); EnsureTicketRenewalIfKeepUserLoggedIn(ctx); // add or update a claim to track when the cookie expires, we use this to track time remaining @@ -163,6 +162,16 @@ public void Configure(CookieAuthenticationOptions options) Constants.Security.BackOfficeAuthenticationType, backOfficeIdentity)); + await securityStampValidator.ValidateAsync(ctx); + + // This might have been called from GetRemainingTimeoutSeconds, in this case we don't want to ensure valid session + if (IsRemainingSecondsRequest(ctx)) + { + return; + } + + ctx.ShouldRenew = true; + await EnsureValidSessionId(ctx); }, OnSigningIn = ctx => { @@ -276,5 +285,21 @@ private void EnsureTicketRenewalIfKeepUserLoggedIn(CookieValidatePrincipalContex } } } + + private bool IsRemainingSecondsRequest(CookieValidatePrincipalContext context) + { + var routeValues = context.HttpContext.Request.RouteValues; + if (routeValues.TryGetValue("controller", out var controllerName) && + routeValues.TryGetValue("action", out var action)) + { + if (controllerName?.ToString() == nameof(AuthenticationController).TrimEnd("Controller") + && action?.ToString() == nameof(AuthenticationController.GetRemainingTimeoutSeconds)) + { + return true; + } + } + + return false; + } } } From e822df338196c75ca4e0c73b9771e1638be161a8 Mon Sep 17 00:00:00 2001 From: Mole Date: Fri, 18 Feb 2022 11:43:28 +0100 Subject: [PATCH 2/7] Don't ignore SessionIdClaimType and Cookiepath when merging claims Besides what the comment used to state these claims are only issued when logging in, leading you to be logged out once the claims are merged, furthermore when we check the session ID we verify that you session has not expired. --- .../Security/ClaimsIdentityExtensions.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs index de5b6206fc22..1873be22e932 100644 --- a/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs @@ -1,17 +1,19 @@ // Copyright (c) Umbraco. // See LICENSE for more details. + +using System; using System.Linq; using System.Security.Claims; -using Umbraco.Cms.Core; using Umbraco.Cms.Core.Security; namespace Umbraco.Extensions { public static class MergeClaimsIdentityExtensions { - // Ignore these Claims when merging, these claims are dynamically added whenever the ticket - // is re-issued and we don't want to merge old values of these. - private static readonly string[] s_ignoredClaims = new[] { ClaimTypes.CookiePath, Constants.Security.SessionIdClaimType }; + // We used to ignore CookiePath and SessionIdClaimType, but these claims are only issued at login + // meaning if we don't merge these claims you'll be logged out, after the claims are merged + // since your session ID disappears + private static readonly string[] s_ignoredClaims = Array.Empty(); public static void MergeAllClaims(this ClaimsIdentity destination, ClaimsIdentity source) { From 3e972193a28aebc8105f0786e2af2dfbe640903a Mon Sep 17 00:00:00 2001 From: Mole Date: Fri, 18 Feb 2022 14:58:35 +0100 Subject: [PATCH 3/7] Manually specify Issued and Expires when renewing token If we don't we lose 30 minutes of our ExpireTimeSpan every time the principal refreshes --- .../Security/ConfigureBackOfficeCookieOptions.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 266a6992b58e..69a08da388ea 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -165,11 +165,21 @@ public void Configure(CookieAuthenticationOptions options) await securityStampValidator.ValidateAsync(ctx); // This might have been called from GetRemainingTimeoutSeconds, in this case we don't want to ensure valid session + // since that in it self will keep the session valid since we renew the lastVerified date. + // Similarly don't renew the token if (IsRemainingSecondsRequest(ctx)) { return; } + // We have to manually specify Issued and Expires, + // because the SecurityStampValidator refreshes the principal every 30 minutes, + // When the principal is refreshed the Issued is update to time of refresh, however, the Expires remains unchanged + // When we then try and renew, the difference of issued and expires effectively becomes the new ExpireTimeSpan + // meaning we effectively lose 30 minutes of our ExpireTimeSpan for EVERY principal refresh if we don't + // https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs#L115 + ctx.Properties.IssuedUtc = _systemClock.UtcNow; + ctx.Properties.ExpiresUtc = _systemClock.UtcNow.Add(_globalSettings.TimeOut); ctx.ShouldRenew = true; await EnsureValidSessionId(ctx); }, @@ -235,7 +245,7 @@ public void Configure(CookieAuthenticationOptions options) } return Task.CompletedTask; - } + }, }; } From 94bd3e0ada063153f22433ec57afcef7839c6ddd Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Mon, 21 Feb 2022 08:54:28 +0100 Subject: [PATCH 4/7] Re-add ignored claims And use MergeAllClaims on refreshing principal instead. --- .../Security/ClaimsIdentityExtensions.cs | 4 ++-- .../Security/ConfigureSecurityStampOptions.cs | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs index 1873be22e932..516caab1c667 100644 --- a/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs @@ -1,9 +1,9 @@ // Copyright (c) Umbraco. // See LICENSE for more details. -using System; using System.Linq; using System.Security.Claims; +using Umbraco.Cms.Core; using Umbraco.Cms.Core.Security; namespace Umbraco.Extensions @@ -13,7 +13,7 @@ public static class MergeClaimsIdentityExtensions // We used to ignore CookiePath and SessionIdClaimType, but these claims are only issued at login // meaning if we don't merge these claims you'll be logged out, after the claims are merged // since your session ID disappears - private static readonly string[] s_ignoredClaims = Array.Empty(); + private static readonly string[] s_ignoredClaims = { ClaimTypes.CookiePath, Constants.Security.SessionIdClaimType }; public static void MergeAllClaims(this ClaimsIdentity destination, ClaimsIdentity source) { diff --git a/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs b/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs index 03bdf8f4dd27..66cf97fd4c0c 100644 --- a/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs +++ b/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs @@ -30,7 +30,8 @@ public static void ConfigureOptions(SecurityStampValidatorOptions options) ClaimsIdentity newIdentity = refreshingPrincipal.NewPrincipal.Identities.First(); ClaimsIdentity currentIdentity = refreshingPrincipal.CurrentPrincipal.Identities.First(); - newIdentity.MergeClaimsFromCookieIdentity(currentIdentity); + // Since this is refreshing an existing principal, we want to merge all claims. + newIdentity.MergeAllClaims(currentIdentity); return Task.CompletedTask; }; From d385b681b668c2c2afbf648f41614abaddf16df4 Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Mon, 21 Feb 2022 09:47:29 +0100 Subject: [PATCH 5/7] EnsureValidSessionId before updating IssuedUtc --- .../Security/ConfigureBackOfficeCookieOptions.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 69a08da388ea..bece55d8624d 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -172,6 +172,9 @@ public void Configure(CookieAuthenticationOptions options) return; } + // This relies on IssuedUtc, so call it before updating it. + await EnsureValidSessionId(ctx); + // We have to manually specify Issued and Expires, // because the SecurityStampValidator refreshes the principal every 30 minutes, // When the principal is refreshed the Issued is update to time of refresh, however, the Expires remains unchanged @@ -181,7 +184,6 @@ public void Configure(CookieAuthenticationOptions options) ctx.Properties.IssuedUtc = _systemClock.UtcNow; ctx.Properties.ExpiresUtc = _systemClock.UtcNow.Add(_globalSettings.TimeOut); ctx.ShouldRenew = true; - await EnsureValidSessionId(ctx); }, OnSigningIn = ctx => { From 3fc1d47dca26a2f501807c2f4edc11b1e0f5150e Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Mon, 21 Feb 2022 11:26:55 +0100 Subject: [PATCH 6/7] Fix comment --- .../Security/ClaimsIdentityExtensions.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs index 516caab1c667..9e1191622357 100644 --- a/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs @@ -10,9 +10,9 @@ namespace Umbraco.Extensions { public static class MergeClaimsIdentityExtensions { - // We used to ignore CookiePath and SessionIdClaimType, but these claims are only issued at login - // meaning if we don't merge these claims you'll be logged out, after the claims are merged - // since your session ID disappears + // Ignore these Claims when merging, these claims are dynamically added whenever the ticket + // is re-issued and we don't want to merge old values of these. + // We do however want to merge these when the SecurityStampValidator refreshes the principal since it's still the same login session private static readonly string[] s_ignoredClaims = { ClaimTypes.CookiePath, Constants.Security.SessionIdClaimType }; public static void MergeAllClaims(this ClaimsIdentity destination, ClaimsIdentity source) From 1301fbce2990271c7121ed9400273149da06c2df Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 22 Feb 2022 07:40:29 +0100 Subject: [PATCH 7/7] Update src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs --- .../Security/ConfigureBackOfficeCookieOptions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index bece55d8624d..916bfb17c0c3 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -304,7 +304,7 @@ private bool IsRemainingSecondsRequest(CookieValidatePrincipalContext context) if (routeValues.TryGetValue("controller", out var controllerName) && routeValues.TryGetValue("action", out var action)) { - if (controllerName?.ToString() == nameof(AuthenticationController).TrimEnd("Controller") + if (controllerName?.ToString() == ControllerExtensions.GetControllerName() && action?.ToString() == nameof(AuthenticationController.GetRemainingTimeoutSeconds)) { return true;