V9: Fix login timeout (#12029)

* 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.

* 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.

* Manually specify Issued and Expires when renewing token

If we don't we lose 30 minutes of our ExpireTimeSpan every time the principal refreshes

* Re-add ignored claims

And use MergeAllClaims on refreshing principal instead.

* EnsureValidSessionId before updating IssuedUtc

* Fix comment

* Update src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs

Co-authored-by: nikolajlauridsen <[email protected]>
Co-authored-by: Bjarke Berg <[email protected]>

Author: 3 people

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;
         }
 

src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs

@@ -1,5 +1,6 @@
 // Copyright (c) Umbraco.
 // See LICENSE for more details.
+
 using System.Linq;
 using System.Security.Claims;
 using Umbraco.Cms.Core;
@@ -11,7 +12,8 @@ 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 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)
         {

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)
         /// <inheritdoc />
         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,28 @@ 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
+                    // 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;
+                    }
+
+                    // 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
+                    // 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;
                 },
                 OnSigningIn = ctx =>
                 {
@@ -226,7 +247,7 @@ public void Configure(CookieAuthenticationOptions options)
                     }
 
                     return Task.CompletedTask;
-                }
+                },
             };
         }
 
@@ -276,5 +297,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() == ControllerExtensions.GetControllerName<AuthenticationController>()
+                    && action?.ToString() == nameof(AuthenticationController.GetRemainingTimeoutSeconds))
+                {
+                    return true;
+                }
+            }
+
+            return false;
+        }
     }
 }

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;
             };