From 78f6b8d8bcdbcd808ed958f212d815bc90488412 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Nov 2017 23:57:17 +1100 Subject: [PATCH] Fixes up more the the session id validation, makes sure that the GetUserSecondsMiddleWare also validates the session since this is what keeps the user logged in if that option is being used. --- .../BackOfficeCookieAuthenticationProvider.cs | 7 +- .../Security/SessionIdValidator.cs | 172 +++++++----------- src/Umbraco.Tests/packages.config | 1 + .../config/umbracoSettings.Release.config | 7 +- .../Identity/GetUserSecondsMiddleWare.cs | 6 +- src/UmbracoExamine/UmbracoExamine.csproj | 1 - src/UmbracoExamine/packages.config | 1 + src/umbraco.MacroEngines/packages.config | 1 + .../umbraco.MacroEngines.csproj | 1 - 9 files changed, 78 insertions(+), 119 deletions(-) diff --git a/src/Umbraco.Core/Security/BackOfficeCookieAuthenticationProvider.cs b/src/Umbraco.Core/Security/BackOfficeCookieAuthenticationProvider.cs index bcc4ef5fff..0a51359bde 100644 --- a/src/Umbraco.Core/Security/BackOfficeCookieAuthenticationProvider.cs +++ b/src/Umbraco.Core/Security/BackOfficeCookieAuthenticationProvider.cs @@ -67,6 +67,11 @@ namespace Umbraco.Core.Security base.ResponseSignOut(context); //Make sure the definitely all of these cookies are cleared when signing out with cookies + context.Response.Cookies.Append(SessionIdValidator.CookieName, "", new CookieOptions + { + Expires = DateTime.Now.AddYears(-1), + Path = "/" + }); context.Response.Cookies.Append(UmbracoConfig.For.UmbracoSettings().Security.AuthCookieName, "", new CookieOptions { Expires = DateTime.Now.AddYears(-1), @@ -107,7 +112,7 @@ namespace Umbraco.Core.Security protected virtual async Task EnsureValidSessionId(CookieValidateIdentityContext context) { if (_appCtx.IsConfigured && _appCtx.IsUpgrading == false) - await SessionIdValidator.ValidateSession(TimeSpan.FromMinutes(1), context); + await SessionIdValidator.ValidateSessionAsync(TimeSpan.FromMinutes(1), context); } private void EnsureCulture(CookieValidateIdentityContext context) diff --git a/src/Umbraco.Core/Security/SessionIdValidator.cs b/src/Umbraco.Core/Security/SessionIdValidator.cs index d6c39d235f..1737baa778 100644 --- a/src/Umbraco.Core/Security/SessionIdValidator.cs +++ b/src/Umbraco.Core/Security/SessionIdValidator.cs @@ -1,11 +1,13 @@ using System; using System.Collections.Concurrent; +using System.Security.Claims; using System.Threading; using System.Threading.Tasks; using System.Web; using Microsoft.AspNet.Identity; using Microsoft.AspNet.Identity.Owin; using Microsoft.Owin; +using Microsoft.Owin.Infrastructure; using Microsoft.Owin.Security.Cookies; using Umbraco.Core.Configuration; using Umbraco.Core.Models.Identity; @@ -25,142 +27,92 @@ namespace Umbraco.Core.Security internal static class SessionIdValidator { public const string CookieName = "UMB_UCONTEXT_C"; - - public static readonly object CookieLocker = new object(); - - public static async Task ValidateSession(TimeSpan validateInterval, CookieValidateIdentityContext context) + + public static async Task ValidateSessionAsync(TimeSpan validateInterval, CookieValidateIdentityContext context) { if (context.Request.Uri.IsBackOfficeRequest(HttpRuntime.AppDomainAppVirtualPath) == false) return; + + var valid = await ValidateSessionAsync(validateInterval, context.OwinContext, context.Options.CookieManager, context.Options.SystemClock, context.Properties.IssuedUtc, context.Identity); + + if (valid == false) + { + context.RejectIdentity(); + context.OwinContext.Authentication.SignOut(context.Options.AuthenticationType); + } + } + + public static async Task ValidateSessionAsync( + TimeSpan validateInterval, + IOwinContext owinCtx, + ICookieManager cookieManager, + ISystemClock systemClock, + DateTimeOffset? authTicketIssueDate, + ClaimsIdentity currentIdentity) + { + if (owinCtx == null) throw new ArgumentNullException("owinCtx"); + if (cookieManager == null) throw new ArgumentNullException("cookieManager"); + if (systemClock == null) throw new ArgumentNullException("systemClock"); DateTimeOffset? issuedUtc = null; - var currentUtc = DateTimeOffset.UtcNow; + var currentUtc = systemClock.UtcNow; //read the last checked time from a custom cookie - var lastCheckedCookie = context.Options.CookieManager.GetRequestCookie(context.OwinContext, CookieName); - + var lastCheckedCookie = cookieManager.GetRequestCookie(owinCtx, CookieName); + if (lastCheckedCookie.IsNullOrWhiteSpace() == false) { DateTimeOffset parsed; if (DateTimeOffset.TryParse(lastCheckedCookie, out parsed)) + { issuedUtc = parsed; + } } //no cookie, use the issue time of the auth ticket if (issuedUtc.HasValue == false) { - if (context.Options != null && context.Options.SystemClock != null) - { - currentUtc = context.Options.SystemClock.UtcNow; - } - issuedUtc = context.Properties.IssuedUtc; + issuedUtc = authTicketIssueDate; } // Only validate if enough time has elapsed - var validate = (issuedUtc == null); - if (issuedUtc != null) + var validate = issuedUtc.HasValue == false; + if (issuedUtc.HasValue) { var timeElapsed = currentUtc.Subtract(issuedUtc.Value); validate = timeElapsed > validateInterval; } - if (validate) - { - var manager = context.OwinContext.GetUserManager(); - var userId = context.Identity.GetUserId(); - if (manager != null) + + if (validate == false) + return true; + + var manager = owinCtx.GetUserManager(); + if (manager == null) + return false; + + var userId = currentIdentity.GetUserId(); + var user = await manager.FindByIdAsync(userId); + if (user == null) + return false; + + var sessionId = currentIdentity.FindFirstValue(Constants.Security.SessionIdClaimType); + if (await manager.ValidateSessionIdAsync(userId, sessionId) == false) + return false; + + //we will re-issue the cookie last checked cookie + cookieManager.AppendResponseCookie( + owinCtx, + CookieName, + DateTimeOffset.UtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffzzz"), + new CookieOptions { - var user = await manager.FindByIdAsync(userId); - var reject = true; - if (user != null) - { - //get or create the session checker for the user, this allows us to perform some locking and to check the server side - //last checked date/time which allows us to avoid a bunch of requests being checked at the same time and thus a bunch of - //db calls to check the session id at the same time and also writing back cookies for each request. Essentially this just - //makes things perform better to reduce the amount of db calls and cookie writing. - var sessionChecker = SessionChecks.GetOrAdd(user.Id, i => new SessionCheck - { - LastChecked = issuedUtc ?? DateTimeOffset.UtcNow, - //1,1 means only one thread at a time - Locker = new SemaphoreSlim(1, 1) - }); + HttpOnly = true, + Secure = GlobalSettings.UseSSL || owinCtx.Request.IsSecure, + Path = "/" + }); - //re-evaluate - if (issuedUtc != null) - { - var timeElapsed = currentUtc.Subtract(sessionChecker.LastChecked); - validate = timeElapsed > validateInterval; - } - - //immediately update the last checked date which will update it in our SessionChecks memory storage - //so that subsequent requests occuring at the same time don't perform the check - sessionChecker.LastChecked = DateTimeOffset.UtcNow; - - //we use a lock per user to prevent having the same cookie being written multiple times if multiple requests come in at the very same - //time. This has occured for me when clicking on a node that has multiple pickers and multiple REST calls are made at the same time this - //validation occurs. This would mean that we would make more db lookups than required and also write the cookie out more times than required - //so this prevents this scenario. - if (validate) - { - try - { - await sessionChecker.Locker.WaitAsync().ConfigureAwait(false); - - //re-evaluate inside the lock - var timeElapsed = currentUtc.Subtract(sessionChecker.LastChecked); - validate = timeElapsed > validateInterval; - if (validate == false) - { - //exit, another thread must have beat us to it - reject = false; - } - else - { - var sessionId = context.Identity.FindFirstValue(Constants.Security.SessionIdClaimType); - if (await manager.ValidateSessionIdAsync(userId, sessionId)) - { - reject = false; - - //we will re-issue the cookie last checked cookie - context.Options.CookieManager.AppendResponseCookie( - context.OwinContext, - CookieName, - DateTimeOffset.UtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffzzz"), - new CookieOptions - { - HttpOnly = true, - Secure = GlobalSettings.UseSSL || context.Request.IsSecure, - Path = "/" - }); - } - } - } - finally - { - sessionChecker.Locker.Release(); - } - } - } - if (reject) - { - context.RejectIdentity(); - context.OwinContext.Authentication.SignOut(context.Options.AuthenticationType); - } - } - } - } - - /// - /// stores a last checked intance per user - /// - private static readonly ConcurrentDictionary SessionChecks = new ConcurrentDictionary(); - - /// - /// Used to store a locking object and a last checked date/time per user - /// - private struct SessionCheck - { - public SemaphoreSlim Locker { get; set; } - public DateTimeOffset LastChecked { get; set; } + return true; } + } } \ No newline at end of file diff --git a/src/Umbraco.Tests/packages.config b/src/Umbraco.Tests/packages.config index 494df99e12..6972dbc12e 100644 --- a/src/Umbraco.Tests/packages.config +++ b/src/Umbraco.Tests/packages.config @@ -33,6 +33,7 @@ + diff --git a/src/Umbraco.Web.UI/config/umbracoSettings.Release.config b/src/Umbraco.Web.UI/config/umbracoSettings.Release.config index 4e0c24434c..0e431e8202 100644 --- a/src/Umbraco.Web.UI/config/umbracoSettings.Release.config +++ b/src/Umbraco.Web.UI/config/umbracoSettings.Release.config @@ -38,10 +38,6 @@ In Preview Mode - click to end]]> - - - 1800 - - assets/img/installer.jpg + assets/img/installer.jpg + diff --git a/src/Umbraco.Web/Security/Identity/GetUserSecondsMiddleWare.cs b/src/Umbraco.Web/Security/Identity/GetUserSecondsMiddleWare.cs index baf0ef581b..04dea56c89 100644 --- a/src/Umbraco.Web/Security/Identity/GetUserSecondsMiddleWare.cs +++ b/src/Umbraco.Web/Security/Identity/GetUserSecondsMiddleWare.cs @@ -10,6 +10,7 @@ using Microsoft.Owin.Security.Cookies; using Umbraco.Core; using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.UmbracoSettings; +using Umbraco.Core.Security; namespace Umbraco.Web.Security.Identity { @@ -103,7 +104,10 @@ namespace Umbraco.Web.Security.Identity remainingSeconds = (ticket.Properties.ExpiresUtc.Value - currentUtc).TotalSeconds; } - } + } + + //We also need to re-validate the user's session if we are relying on this ping to keep their session alive + await SessionIdValidator.ValidateSessionAsync(TimeSpan.FromMinutes(1), context, _authOptions.CookieManager, _authOptions.SystemClock, issuedUtc, ticket.Identity); } else if (remainingSeconds <= 30) { diff --git a/src/UmbracoExamine/UmbracoExamine.csproj b/src/UmbracoExamine/UmbracoExamine.csproj index 8fdd5b2d04..eb83b5c813 100644 --- a/src/UmbracoExamine/UmbracoExamine.csproj +++ b/src/UmbracoExamine/UmbracoExamine.csproj @@ -87,7 +87,6 @@ ..\packages\SharpZipLib.0.86.0\lib\20\ICSharpCode.SharpZipLib.dll - True ..\packages\Lucene.Net.2.9.4.1\lib\net40\Lucene.Net.dll diff --git a/src/UmbracoExamine/packages.config b/src/UmbracoExamine/packages.config index 94318fda38..36a8eb3060 100644 --- a/src/UmbracoExamine/packages.config +++ b/src/UmbracoExamine/packages.config @@ -2,4 +2,5 @@ + \ No newline at end of file diff --git a/src/umbraco.MacroEngines/packages.config b/src/umbraco.MacroEngines/packages.config index 3e343b299e..a46f0572ef 100644 --- a/src/umbraco.MacroEngines/packages.config +++ b/src/umbraco.MacroEngines/packages.config @@ -12,4 +12,5 @@ + \ No newline at end of file diff --git a/src/umbraco.MacroEngines/umbraco.MacroEngines.csproj b/src/umbraco.MacroEngines/umbraco.MacroEngines.csproj index 929ab4ae73..63d8314084 100644 --- a/src/umbraco.MacroEngines/umbraco.MacroEngines.csproj +++ b/src/umbraco.MacroEngines/umbraco.MacroEngines.csproj @@ -54,7 +54,6 @@ ..\packages\SharpZipLib.0.86.0\lib\20\ICSharpCode.SharpZipLib.dll - True ..\packages\Lucene.Net.2.9.4.1\lib\net40\Lucene.Net.dll