From 2b412f337c44fc0a6159593b2a807d01e194cecb Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 9 Jun 2020 17:40:35 +1000 Subject: [PATCH] Fixes issue with growing cookie/identity (we don't want to use Actor), fixes GlobalSettings issue, ensures temp claims are not cloned --- .../Models/GlobalSettings.cs | 2 +- .../BackOffice/UmbracoBackOfficeIdentity.cs | 16 ++++++++++++++-- src/Umbraco.Core/Constants-AppSettings.cs | 2 ++ .../BackOffice/UmbracoBackOfficeIdentityTests.cs | 10 ++++++++-- .../Security/BackOfficeCookieManager.cs | 2 -- .../Security/BackOfficeSecureDataFormat.cs | 3 +-- .../Security/ConfigureBackOfficeCookieOptions.cs | 4 ---- 7 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Configuration/Models/GlobalSettings.cs b/src/Umbraco.Configuration/Models/GlobalSettings.cs index e4995cfb0b..908ba71590 100644 --- a/src/Umbraco.Configuration/Models/GlobalSettings.cs +++ b/src/Umbraco.Configuration/Models/GlobalSettings.cs @@ -39,7 +39,7 @@ namespace Umbraco.Configuration.Models } public int TimeOutInMinutes => _configuration.GetValue(Prefix + "TimeOutInMinutes", 20); - public string DefaultUILanguage => _configuration.GetValue(Prefix + "TimeOutInMinutes", "en-US"); + public string DefaultUILanguage => _configuration.GetValue(Prefix + "DefaultUILanguage", "en-US"); public bool HideTopLevelNodeFromPath => _configuration.GetValue(Prefix + "HideTopLevelNodeFromPath", false); diff --git a/src/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentity.cs b/src/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentity.cs index 15db0e0dc7..93acb279dc 100644 --- a/src/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentity.cs +++ b/src/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentity.cs @@ -36,7 +36,6 @@ namespace Umbraco.Core.BackOffice private UmbracoBackOfficeIdentity(ClaimsIdentity identity) : base(identity.Claims, Constants.Security.BackOfficeAuthenticationType) { - Actor = identity; } /// @@ -89,7 +88,7 @@ namespace Umbraco.Core.BackOffice if (string.IsNullOrWhiteSpace(realName)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(realName)); if (string.IsNullOrWhiteSpace(culture)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(culture)); if (string.IsNullOrWhiteSpace(securityStamp)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(securityStamp)); - Actor = childIdentity; + AddRequiredClaims(userId, username, realName, startContentNodes, startMediaNodes, culture, securityStamp, allowedApps, roles); } @@ -204,5 +203,18 @@ namespace Umbraco.Core.BackOffice public string[] Roles => this.FindAll(x => x.Type == DefaultRoleClaimType).Select(role => role.Value).ToArray(); + /// + /// Overridden to remove any temporary claims that shouldn't be copied + /// + /// + public override ClaimsIdentity Clone() + { + var clone = base.Clone(); + + foreach (var claim in clone.FindAll(x => x.Type == Constants.Security.TicketExpiresClaimType).ToList()) + clone.RemoveClaim(claim); + + return clone; + } } } diff --git a/src/Umbraco.Core/Constants-AppSettings.cs b/src/Umbraco.Core/Constants-AppSettings.cs index 3551aa1c31..94370930de 100644 --- a/src/Umbraco.Core/Constants-AppSettings.cs +++ b/src/Umbraco.Core/Constants-AppSettings.cs @@ -9,6 +9,8 @@ namespace Umbraco.Core /// public static class AppSettings { + // TODO: Are these all obsolete in netcore now? + public const string MainDomLock = "Umbraco.Core.MainDom.Lock"; // TODO: Kill me - still used in Umbraco.Core.IO.SystemFiles:27 diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs index 47e1261c09..9e9d29a123 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs @@ -37,6 +37,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice if (!UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out var backofficeIdentity)) Assert.Fail(); + Assert.IsNull(backofficeIdentity.Actor); Assert.AreEqual(1234, backofficeIdentity.Id); //Assert.AreEqual(sessionId, backofficeIdentity.SessionId); Assert.AreEqual(securityStamp, backofficeIdentity.SecurityStamp); @@ -60,7 +61,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice new Claim(ClaimTypes.Name, "testing", ClaimValueTypes.String, TestIssuer, TestIssuer), }); - if (UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out var backofficeIdentity)) + if (UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out _)) Assert.Fail(); Assert.Pass(); @@ -83,7 +84,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice new Claim(ClaimsIdentity.DefaultRoleClaimType, "admin", ClaimValueTypes.String, TestIssuer, TestIssuer), }); - if (UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out var backofficeIdentity)) + if (UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out _)) Assert.Fail(); Assert.Pass(); @@ -105,6 +106,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice 1234, "testing", "hello world", new[] { 654 }, new[] { 654 }, "en-us", securityStamp, new[] { "content", "media" }, new[] { "admin" }); Assert.AreEqual(12, identity.Claims.Count()); + Assert.IsNull(identity.Actor); } @@ -116,7 +118,11 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice var identity = new UmbracoBackOfficeIdentity( 1234, "testing", "hello world", new[] { 654 }, new[] { 654 }, "en-us", securityStamp, new[] { "content", "media" }, new[] { "admin" }); + // this will be filtered out during cloning + identity.AddClaim(new Claim(Constants.Security.TicketExpiresClaimType, "test")); + var cloned = identity.Clone(); + Assert.IsNull(cloned.Actor); Assert.AreEqual(10, cloned.Claims.Count()); } diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeCookieManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeCookieManager.cs index 19f9316cfb..f63b2380af 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeCookieManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeCookieManager.cs @@ -31,7 +31,6 @@ namespace Umbraco.Web.BackOffice.Security private readonly IGlobalSettings _globalSettings; private readonly IRequestCache _requestCache; private readonly string[] _explicitPaths; - private readonly string _getRemainingSecondsPath; public BackOfficeCookieManager( IUmbracoContextAccessor umbracoContextAccessor, @@ -58,7 +57,6 @@ namespace Umbraco.Web.BackOffice.Security _globalSettings = globalSettings; _requestCache = requestCache; _explicitPaths = explicitPaths?.ToArray(); - _getRemainingSecondsPath = linkGenerator.GetUmbracoApiService(x => x.GetRemainingTimeoutSeconds()); } /// diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs index 3a23ae6c88..91b982b5f6 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs @@ -9,7 +9,6 @@ namespace Umbraco.Web.BackOffice.Security /// /// Custom secure format that ensures the Identity in the ticket is and not just a ClaimsIdentity /// - // TODO: Unsure if we really need this, there's no real reason why we have a custom Identity instead of just a ClaimsIdentity internal class BackOfficeSecureDataFormat : ISecureDataFormat { private readonly int _loginTimeoutMinutes; @@ -23,7 +22,7 @@ namespace Umbraco.Web.BackOffice.Security public string Protect(AuthenticationTicket data, string purpose) { - //create a new ticket based on the passed in tickets details, however, we'll adjust the expires utc based on the specified timeout mins + // create a new ticket based on the passed in tickets details, however, we'll adjust the expires utc based on the specified timeout mins var ticket = new AuthenticationTicket(data.Principal, new AuthenticationProperties(data.Properties.Items) { diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index fd4d530591..d51db33a55 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -150,10 +150,6 @@ namespace Umbraco.Web.BackOffice.Security UmbracoBackOfficeIdentity.Issuer, backOfficeIdentity)); - if (_securitySettings.KeepUserLoggedIn) - { - - } }, OnSigningIn = ctx => {