Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V9: Fix login timeout #12029

Merged
merged 7 commits into from Feb 22, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs
Expand Up @@ -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;
}

Expand Down
@@ -1,5 +1,6 @@
// Copyright (c) Umbraco.
// See LICENSE for more details.

using System.Linq;
using System.Security.Claims;
using Umbraco.Cms.Core;
Expand All @@ -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)
{
Expand Down
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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 =>
{
Expand Down Expand Up @@ -226,7 +247,7 @@ public void Configure(CookieAuthenticationOptions options)
}

return Task.CompletedTask;
}
},
};
}

Expand Down Expand Up @@ -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() == nameof(AuthenticationController).TrimEnd("Controller")
bergmania marked this conversation as resolved.
Show resolved Hide resolved
&& action?.ToString() == nameof(AuthenticationController.GetRemainingTimeoutSeconds))
{
return true;
}
}

return false;
}
}
}
Expand Up @@ -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;
};
Expand Down