From fac0be17014d714ccdeaa0f90b9ac54ad2a60038 Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 16 Feb 2021 15:43:15 +0100 Subject: [PATCH 01/12] Remove clone It's no longer needed --- .../Security/UmbracoBackOfficeIdentity.cs | 16 +--------------- .../UmbracoBackOfficeIdentityTests.cs | 17 ----------------- 2 files changed, 1 insertion(+), 32 deletions(-) diff --git a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs index 5fd9f23c92..18841d4448 100644 --- a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs +++ b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs @@ -14,7 +14,7 @@ namespace Umbraco.Core.Security { // TODO: Ideally we remove this class and only deal with ClaimsIdentity as a best practice. All things relevant to our own // identity are part of claims. This class would essentially become extension methods on a ClaimsIdentity for resolving - // values from it. + // values from it. public static bool FromClaimsIdentity(ClaimsIdentity identity, out UmbracoBackOfficeIdentity backOfficeIdentity) { // validate that all claims exist @@ -214,19 +214,5 @@ namespace Umbraco.Core.Security public string SecurityStamp => this.FindFirstValue(Constants.Security.SecurityStampClaimType); public string[] Roles => 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.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs index 35e143277a..fd52ab8c4c 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs @@ -126,22 +126,5 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice Assert.AreEqual(12, identity.Claims.Count()); Assert.IsNull(identity.Actor); } - - [Test] - public void Clone() - { - var securityStamp = Guid.NewGuid().ToString(); - - 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")); - - ClaimsIdentity cloned = identity.Clone(); - Assert.IsNull(cloned.Actor); - - Assert.AreEqual(10, cloned.Claims.Count()); - } } } From d14aa007ea3aee246378cba62153fac8e29dbdf5 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Feb 2021 09:50:27 +0100 Subject: [PATCH 02/12] Add extension methods to replace UmbracoBackOfficeIdentity --- .../Security/ClaimsIdentityExtensions.cs | 152 ++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs diff --git a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs new file mode 100644 index 0000000000..65e12895cc --- /dev/null +++ b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs @@ -0,0 +1,152 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using System.Collections.Generic; +using System.Linq; +using System.Security.Claims; +using Umbraco.Core; + +namespace Umbraco.Extensions +{ + public static class ClaimsIdentityExtensions + { + /// + /// Returns the required claim types for a back office identity + /// + /// + /// This does not include the role claim type or allowed apps type since that is a collection and in theory could be empty + /// + public static IEnumerable RequiredBackOfficeClaimTypes => new[] + { + ClaimTypes.NameIdentifier, //id + ClaimTypes.Name, //username + ClaimTypes.GivenName, + Constants.Security.StartContentNodeIdClaimType, + Constants.Security.StartMediaNodeIdClaimType, + ClaimTypes.Locality, + Constants.Security.SecurityStampClaimType + }; + + public const string Issuer = Constants.Security.BackOfficeAuthenticationType; + + /// + /// Verify that a ClaimsIdentity has all the required claim types + /// + /// + /// Verified identity wrapped in a ClaimsIdentity with BackOfficeAuthentication type + /// True if ClaimsIdentity + public static bool VerifyBackOfficeIdentity(this ClaimsIdentity identity, out ClaimsIdentity verifiedIdentity) + { + // Validate that all required claims exist + foreach (var claimType in RequiredBackOfficeClaimTypes) + { + if (identity.HasClaim(x => x.Type == claimType) == false || + identity.HasClaim(x => x.Type == claimType && x.Value.IsNullOrWhiteSpace())) + { + verifiedIdentity = null; + return false; + } + } + + verifiedIdentity = new ClaimsIdentity(identity.Claims, Constants.Security.BackOfficeAuthenticationType); + return true; + } + + public static void AddRequiredClaims(this ClaimsIdentity identity, string userId, string username, + string realName, IEnumerable startContentNodes, IEnumerable startMediaNodes, string culture, + string securityStamp, IEnumerable allowedApps, IEnumerable roles) + { + //This is the id that 'identity' uses to check for the user id + if (identity.HasClaim(x => x.Type == ClaimTypes.NameIdentifier) == false) + { + identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, userId, ClaimValueTypes.String, + Issuer, Issuer, identity)); + } + + if (identity.HasClaim(x => x.Type == ClaimTypes.Name) == false) + { + identity.AddClaim(new Claim(ClaimTypes.Name, username, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + + if (identity.HasClaim(x => x.Type == ClaimTypes.GivenName) == false) + { + identity.AddClaim(new Claim(ClaimTypes.GivenName, realName, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + + if (identity.HasClaim(x => x.Type == Constants.Security.StartContentNodeIdClaimType) == false && + startContentNodes != null) + { + foreach (var startContentNode in startContentNodes) + { + identity.AddClaim(new Claim(Constants.Security.StartContentNodeIdClaimType, startContentNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, identity)); + } + } + + if (identity.HasClaim(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) == false) + { + foreach (var startMediaNode in startMediaNodes) + { + identity.AddClaim(new Claim(Constants.Security.StartMediaNodeIdClaimType, startMediaNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, identity)); + } + } + + if (identity.HasClaim(x => x.Type == ClaimTypes.Locality) == false) + { + identity.AddClaim(new Claim(ClaimTypes.Locality, culture, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + + // The security stamp claim is also required + if (identity.HasClaim(x => x.Type == Constants.Security.SecurityStampClaimType) == false) + { + identity.AddClaim(new Claim(Constants.Security.SecurityStampClaimType, securityStamp, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + + // Add each app as a separate claim + if (identity.HasClaim(x => x.Type == Constants.Security.AllowedApplicationsClaimType) == false && + allowedApps != null) + { + foreach (var application in allowedApps) + { + identity.AddClaim(new Claim(Constants.Security.AllowedApplicationsClaimType, application, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + } + + // Claims are added by the ClaimsIdentityFactory because our UserStore supports roles, however this identity might + // not be made with that factory if it was created with a different ticket so perform the check + if (identity.HasClaim(x => x.Type == ClaimsIdentity.DefaultRoleClaimType) == false && roles != null) + { + // Manually add them + foreach (var roleName in roles) + { + identity.AddClaim(new Claim(identity.RoleClaimType, roleName, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + } + } + + public static int[] GetStartContentNodes(this ClaimsIdentity identity) => + identity.FindAll(x => x.Type == Constants.Security.StartContentNodeIdClaimType) + .Select(node => int.TryParse(node.Value, out var i) ? i : default) + .Where(x => x != default).ToArray(); + + public static int[] GetStartMediaNodes(this ClaimsIdentity identity) => + identity.FindAll(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) + .Select(node => int.TryParse(node.Value, out var i) ? i : default) + .Where(x => x != default).ToArray(); + + public static string[] GetAllowedApplications(this ClaimsIdentity identity) => identity + .FindAll(x => x.Type == Constants.Security.AllowedApplicationsClaimType).Select(app => app.Value).ToArray(); + + public static int GetId(this ClaimsIdentity identity) => int.Parse(identity.FindFirstValue(ClaimTypes.NameIdentifier)); + + public static string GetRealName(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.GivenName); + + public static string GetUsername(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Name); + + public static string GetCulture(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Locality); + + public static string GetSecurityStamp(this ClaimsIdentity identity) => identity.FindFirstValue(Constants.Security.SecurityStampClaimType); + + public static string[] GetRoles(this ClaimsIdentity identity) => identity + .FindAll(x => x.Type == ClaimsIdentity.DefaultRoleClaimType).Select(role => role.Value).ToArray(); + } +} From a87075a941ac325d52fa6ea7ab52acc053f9f424 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Feb 2021 10:11:04 +0100 Subject: [PATCH 03/12] Switch simple properties to extension methods --- .../Security/AuthenticationExtensions.cs | 3 ++- .../Security/ClaimsIdentityExtensions.cs | 2 +- .../Security/UmbracoBackOfficeIdentity.cs | 22 +------------------ .../Compose/AuditEventsComponent.cs | 2 +- .../UmbracoBackOfficeIdentityTests.cs | 19 ++++++++-------- .../PublishedContent/PublishedContentTests.cs | 8 +++---- .../TestControllerActivatorBase.cs | 17 +++++++------- .../CheckIfUserTicketDataIsStaleAttribute.cs | 14 ++++++------ .../FilterAllowedOutgoingContentAttribute.cs | 4 ++-- .../ConfigureBackOfficeCookieOptions.cs | 4 ++-- .../Security/BackofficeSecurity.cs | 2 +- 11 files changed, 40 insertions(+), 57 deletions(-) diff --git a/src/Umbraco.Core/Security/AuthenticationExtensions.cs b/src/Umbraco.Core/Security/AuthenticationExtensions.cs index 13eab4ff80..aa17b4e323 100644 --- a/src/Umbraco.Core/Security/AuthenticationExtensions.cs +++ b/src/Umbraco.Core/Security/AuthenticationExtensions.cs @@ -1,6 +1,7 @@ using System.Globalization; using System.Security.Principal; using System.Threading; +using Umbraco.Extensions; namespace Umbraco.Core.Security { @@ -22,7 +23,7 @@ namespace Umbraco.Core.Security { if (identity is UmbracoBackOfficeIdentity umbIdentity && umbIdentity.IsAuthenticated) { - return CultureInfo.GetCultureInfo(umbIdentity.Culture); + return CultureInfo.GetCultureInfo(umbIdentity.GetCultureString()); } return null; diff --git a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs index 65e12895cc..9e304b8916 100644 --- a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs @@ -142,7 +142,7 @@ namespace Umbraco.Extensions public static string GetUsername(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Name); - public static string GetCulture(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Locality); + public static string GetCultureString(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Locality); public static string GetSecurityStamp(this ClaimsIdentity identity) => identity.FindFirstValue(Constants.Security.SecurityStampClaimType); diff --git a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs index 18841d4448..db146d3cc9 100644 --- a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs +++ b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs @@ -130,6 +130,7 @@ namespace Umbraco.Core.Security IEnumerable startContentNodes, IEnumerable startMediaNodes, string culture, string securityStamp, IEnumerable allowedApps, IEnumerable roles) { + // TODO: This has been moved, delete //This is the id that 'identity' uses to check for the user id if (HasClaim(x => x.Type == ClaimTypes.NameIdentifier) == false) AddClaim(new Claim(ClaimTypes.NameIdentifier, userId, ClaimValueTypes.String, Issuer, Issuer, this)); @@ -193,26 +194,5 @@ namespace Umbraco.Core.Security /// The type of authenticated identity. This property always returns "UmbracoBackOffice". /// public override string AuthenticationType => Issuer; - - private int[] _startContentNodes; - public int[] StartContentNodes => _startContentNodes ?? (_startContentNodes = FindAll(x => x.Type == Constants.Security.StartContentNodeIdClaimType).Select(app => int.TryParse(app.Value, out var i) ? i : default).Where(x => x != default).ToArray()); - - private int[] _startMediaNodes; - public int[] StartMediaNodes => _startMediaNodes ?? (_startMediaNodes = FindAll(x => x.Type == Constants.Security.StartMediaNodeIdClaimType).Select(app => int.TryParse(app.Value, out var i) ? i : default).Where(x => x != default).ToArray()); - - private string[] _allowedApplications; - public string[] AllowedApplications => _allowedApplications ?? (_allowedApplications = FindAll(x => x.Type == Constants.Security.AllowedApplicationsClaimType).Select(app => app.Value).ToArray()); - - public int Id => int.Parse(this.FindFirstValue(ClaimTypes.NameIdentifier)); - - public string RealName => this.FindFirstValue(ClaimTypes.GivenName); - - public string Username => this.FindFirstValue(ClaimTypes.Name); - - public string Culture => this.FindFirstValue(ClaimTypes.Locality); - - public string SecurityStamp => this.FindFirstValue(Constants.Security.SecurityStampClaimType); - - public string[] Roles => FindAll(x => x.Type == DefaultRoleClaimType).Select(role => role.Value).ToArray(); } } diff --git a/src/Umbraco.Infrastructure/Compose/AuditEventsComponent.cs b/src/Umbraco.Infrastructure/Compose/AuditEventsComponent.cs index c085db2496..77ef45a301 100644 --- a/src/Umbraco.Infrastructure/Compose/AuditEventsComponent.cs +++ b/src/Umbraco.Infrastructure/Compose/AuditEventsComponent.cs @@ -70,7 +70,7 @@ namespace Umbraco.Core.Compose get { var identity = Thread.CurrentPrincipal?.GetUmbracoIdentity(); - var user = identity == null ? null : _userService.GetUserById(Convert.ToInt32(identity.Id)); + var user = identity == null ? null : _userService.GetUserById(Convert.ToInt32(identity.GetId())); return user ?? UnknownUser(_globalSettings); } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs index fd52ab8c4c..ab241f2522 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs @@ -7,6 +7,7 @@ using System.Security.Claims; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Security; +using Umbraco.Extensions; namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice { @@ -44,16 +45,16 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice } Assert.IsNull(backofficeIdentity.Actor); - Assert.AreEqual(1234, backofficeIdentity.Id); + Assert.AreEqual(1234, backofficeIdentity.GetId()); //// Assert.AreEqual(sessionId, backofficeIdentity.SessionId); - Assert.AreEqual(securityStamp, backofficeIdentity.SecurityStamp); - Assert.AreEqual("testing", backofficeIdentity.Username); - Assert.AreEqual("hello world", backofficeIdentity.RealName); - Assert.AreEqual(1, backofficeIdentity.StartContentNodes.Length); - Assert.IsTrue(backofficeIdentity.StartMediaNodes.UnsortedSequenceEqual(new[] { 5543, 5555 })); - Assert.IsTrue(new[] { "content", "media" }.SequenceEqual(backofficeIdentity.AllowedApplications)); - Assert.AreEqual("en-us", backofficeIdentity.Culture); - Assert.IsTrue(new[] { "admin" }.SequenceEqual(backofficeIdentity.Roles)); + Assert.AreEqual(securityStamp, backofficeIdentity.GetSecurityStamp()); + Assert.AreEqual("testing", backofficeIdentity.GetUsername()); + Assert.AreEqual("hello world", backofficeIdentity.GetRealName()); + Assert.AreEqual(1, backofficeIdentity.GetStartContentNodes().Length); + Assert.IsTrue(backofficeIdentity.GetStartMediaNodes().UnsortedSequenceEqual(new[] { 5543, 5555 })); + Assert.IsTrue(new[] { "content", "media" }.SequenceEqual(backofficeIdentity.GetAllowedApplications())); + Assert.AreEqual("en-us", backofficeIdentity.GetCultureString()); + Assert.IsTrue(new[] { "admin" }.SequenceEqual(backofficeIdentity.GetRoles())); Assert.AreEqual(11, backofficeIdentity.Claims.Count()); } diff --git a/src/Umbraco.Tests/PublishedContent/PublishedContentTests.cs b/src/Umbraco.Tests/PublishedContent/PublishedContentTests.cs index a076deb8b3..9f4a61d1cc 100644 --- a/src/Umbraco.Tests/PublishedContent/PublishedContentTests.cs +++ b/src/Umbraco.Tests/PublishedContent/PublishedContentTests.cs @@ -582,7 +582,7 @@ namespace Umbraco.Tests.PublishedContent Assert.IsNotNull(result); Assert.AreEqual(3, result.Length); - Assert.IsTrue(result.Select(x => ((dynamic)x).Id).ContainsAll(new dynamic[] { 1174, 1173, 1046 })); + Assert.IsTrue(result.Select(x => ((dynamic)x).GetId()).ContainsAll(new dynamic[] { 1174, 1173, 1046 })); } [Test] @@ -595,7 +595,7 @@ namespace Umbraco.Tests.PublishedContent Assert.IsNotNull(result); Assert.AreEqual(2, result.Length); - Assert.IsTrue(result.Select(x => ((dynamic)x).Id).ContainsAll(new dynamic[] { 1173, 1046 })); + Assert.IsTrue(result.Select(x => ((dynamic)x).GetId()).ContainsAll(new dynamic[] { 1173, 1046 })); } [Test] @@ -708,7 +708,7 @@ namespace Umbraco.Tests.PublishedContent Assert.IsNotNull(result); Assert.AreEqual(10, result.Count()); - Assert.IsTrue(result.Select(x => ((dynamic)x).Id).ContainsAll(new dynamic[] { 1046, 1173, 1174, 1176, 1175 })); + Assert.IsTrue(result.Select(x => ((dynamic)x).GetId()).ContainsAll(new dynamic[] { 1046, 1173, 1174, 1176, 1175 })); } [Test] @@ -721,7 +721,7 @@ namespace Umbraco.Tests.PublishedContent Assert.IsNotNull(result); Assert.AreEqual(9, result.Count()); - Assert.IsTrue(result.Select(x => ((dynamic)x).Id).ContainsAll(new dynamic[] { 1173, 1174, 1176, 1175, 4444 })); + Assert.IsTrue(result.Select(x => ((dynamic)x).GetId()).ContainsAll(new dynamic[] { 1173, 1174, 1176, 1175, 4444 })); } [Test] diff --git a/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs b/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs index f993ee5b6a..3e9add3b89 100644 --- a/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs +++ b/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs @@ -17,6 +17,7 @@ using Umbraco.Web.WebApi; using Umbraco.Tests.Common; using Umbraco.Tests.TestHelpers.Entities; using Umbraco.Core.Security; +using Umbraco.Extensions; namespace Umbraco.Tests.TestHelpers.ControllerTesting { @@ -98,23 +99,23 @@ namespace Umbraco.Tests.TestHelpers.ControllerTesting //mock CurrentUser var groups = new List(); - for (var index = 0; index < backofficeIdentity.Roles.Length; index++) + for (var index = 0; index < backofficeIdentity.GetRoles().Length; index++) { - var role = backofficeIdentity.Roles[index]; + var role = backofficeIdentity.GetRoles()[index]; groups.Add(new ReadOnlyUserGroup(index + 1, role, "icon-user", null, null, role, new string[0], new string[0])); } var mockUser = MockedUser.GetUserMock(); mockUser.Setup(x => x.IsApproved).Returns(true); mockUser.Setup(x => x.IsLockedOut).Returns(false); - mockUser.Setup(x => x.AllowedSections).Returns(backofficeIdentity.AllowedApplications); + mockUser.Setup(x => x.AllowedSections).Returns(backofficeIdentity.GetAllowedApplications()); mockUser.Setup(x => x.Groups).Returns(groups); mockUser.Setup(x => x.Email).Returns("admin@admin.com"); - mockUser.Setup(x => x.Id).Returns((int)backofficeIdentity.Id); + mockUser.Setup(x => x.Id).Returns((int)backofficeIdentity.GetId()); mockUser.Setup(x => x.Language).Returns("en"); - mockUser.Setup(x => x.Name).Returns(backofficeIdentity.RealName); - mockUser.Setup(x => x.StartContentIds).Returns(backofficeIdentity.StartContentNodes); - mockUser.Setup(x => x.StartMediaIds).Returns(backofficeIdentity.StartMediaNodes); - mockUser.Setup(x => x.Username).Returns(backofficeIdentity.Username); + mockUser.Setup(x => x.Name).Returns(backofficeIdentity.GetRealName()); + mockUser.Setup(x => x.StartContentIds).Returns(backofficeIdentity.GetStartContentNodes()); + mockUser.Setup(x => x.StartMediaIds).Returns(backofficeIdentity.GetStartMediaNodes()); + mockUser.Setup(x => x.Username).Returns(backofficeIdentity.GetUsername()); backofficeSecurity.Setup(x => x.CurrentUser) .Returns(mockUser.Object); diff --git a/src/Umbraco.Web.BackOffice/Filters/CheckIfUserTicketDataIsStaleAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/CheckIfUserTicketDataIsStaleAttribute.cs index 97765df837..db3da94eb1 100644 --- a/src/Umbraco.Web.BackOffice/Filters/CheckIfUserTicketDataIsStaleAttribute.cs +++ b/src/Umbraco.Web.BackOffice/Filters/CheckIfUserTicketDataIsStaleAttribute.cs @@ -118,7 +118,7 @@ namespace Umbraco.Web.BackOffice.Filters return; } - Attempt userId = identity.Id.TryConvertTo(); + Attempt userId = identity.GetId().TryConvertTo(); if (userId == false) { return; @@ -133,23 +133,23 @@ namespace Umbraco.Web.BackOffice.Filters // a list of checks to execute, if any of them pass then we resync var checks = new Func[] { - () => user.Username != identity.Username, + () => user.Username != identity.GetUsername(), () => { CultureInfo culture = user.GetUserCulture(_localizedTextService, _globalSettings.Value); - return culture != null && culture.ToString() != identity.Culture; + return culture != null && culture.ToString() != identity.GetCultureString(); }, - () => user.AllowedSections.UnsortedSequenceEqual(identity.AllowedApplications) == false, - () => user.Groups.Select(x => x.Alias).UnsortedSequenceEqual(identity.Roles) == false, + () => user.AllowedSections.UnsortedSequenceEqual(identity.GetAllowedApplications()) == false, + () => user.Groups.Select(x => x.Alias).UnsortedSequenceEqual(identity.GetRoles()) == false, () => { var startContentIds = user.CalculateContentStartNodeIds(_entityService); - return startContentIds.UnsortedSequenceEqual(identity.StartContentNodes) == false; + return startContentIds.UnsortedSequenceEqual(identity.GetStartContentNodes()) == false; }, () => { var startMediaIds = user.CalculateMediaStartNodeIds(_entityService); - return startMediaIds.UnsortedSequenceEqual(identity.StartMediaNodes) == false; + return startMediaIds.UnsortedSequenceEqual(identity.GetStartMediaNodes()) == false; } }; diff --git a/src/Umbraco.Web.BackOffice/Filters/FilterAllowedOutgoingContentAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/FilterAllowedOutgoingContentAttribute.cs index 38c0333d8b..363bd1a5d0 100644 --- a/src/Umbraco.Web.BackOffice/Filters/FilterAllowedOutgoingContentAttribute.cs +++ b/src/Umbraco.Web.BackOffice/Filters/FilterAllowedOutgoingContentAttribute.cs @@ -95,7 +95,7 @@ namespace Umbraco.Web.BackOffice.Filters var ids = new List(); for (var i = 0; i < length; i++) { - ids.Add(((dynamic)items[i]).Id); + ids.Add(((dynamic)items[i]).GetId()); } //get all the permissions for these nodes in one call var permissions = _userService.GetPermissions(user, ids.ToArray()); @@ -104,7 +104,7 @@ namespace Umbraco.Web.BackOffice.Filters { //get the combined permission set across all user groups for this node //we're in the world of dynamics here so we need to cast - var nodePermission = ((IEnumerable)permissions.GetAllPermissions(item.Id)).ToArray(); + var nodePermission = ((IEnumerable)permissions.GetAllPermissions(item.GetId())).ToArray(); //if the permission being checked doesn't exist then remove the item if (nodePermission.Contains(_permissionToCheck.ToString(CultureInfo.InvariantCulture)) == false) diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index c267cb7489..557489cb73 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -119,7 +119,7 @@ namespace Umbraco.Web.BackOffice.Security options.CookieManager = new BackOfficeCookieManager( _umbracoContextAccessor, _runtimeState, - _umbracoRequestPaths); + _umbracoRequestPaths); options.Events = new CookieAuthenticationEvents { @@ -171,7 +171,7 @@ namespace Umbraco.Web.BackOffice.Security // generate a session id and assign it // create a session token - if we are configured and not in an upgrade state then use the db, otherwise just generate one Guid session = _runtimeState.Level == RuntimeLevel.Run - ? _userService.CreateLoginSession(backOfficeIdentity.Id, _ipResolver.GetCurrentRequestIpAddress()) + ? _userService.CreateLoginSession(backOfficeIdentity.GetId(), _ipResolver.GetCurrentRequestIpAddress()) : Guid.NewGuid(); // add our session claim diff --git a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs index 9d8fdd9174..9605bcf3ad 100644 --- a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs +++ b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs @@ -56,7 +56,7 @@ namespace Umbraco.Web.Common.Security public Attempt GetUserId() { var identity = _httpContextAccessor.HttpContext?.GetCurrentIdentity(); - return identity == null ? Attempt.Fail() : Attempt.Succeed(identity.Id); + return identity == null ? Attempt.Fail() : Attempt.Succeed(identity.GetId()); } /// From 80716a18d20abb9c554f633ac9c1b76fb3e5fa4c Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Feb 2021 10:16:00 +0100 Subject: [PATCH 04/12] Fix mistaken use of GetId() --- .../Filters/FilterAllowedOutgoingContentAttribute.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Filters/FilterAllowedOutgoingContentAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/FilterAllowedOutgoingContentAttribute.cs index 363bd1a5d0..38c0333d8b 100644 --- a/src/Umbraco.Web.BackOffice/Filters/FilterAllowedOutgoingContentAttribute.cs +++ b/src/Umbraco.Web.BackOffice/Filters/FilterAllowedOutgoingContentAttribute.cs @@ -95,7 +95,7 @@ namespace Umbraco.Web.BackOffice.Filters var ids = new List(); for (var i = 0; i < length; i++) { - ids.Add(((dynamic)items[i]).GetId()); + ids.Add(((dynamic)items[i]).Id); } //get all the permissions for these nodes in one call var permissions = _userService.GetPermissions(user, ids.ToArray()); @@ -104,7 +104,7 @@ namespace Umbraco.Web.BackOffice.Filters { //get the combined permission set across all user groups for this node //we're in the world of dynamics here so we need to cast - var nodePermission = ((IEnumerable)permissions.GetAllPermissions(item.GetId())).ToArray(); + var nodePermission = ((IEnumerable)permissions.GetAllPermissions(item.Id)).ToArray(); //if the permission being checked doesn't exist then remove the item if (nodePermission.Contains(_permissionToCheck.ToString(CultureInfo.InvariantCulture)) == false) From 33a99df73f652c5ff86df974eea0f3a873e042e6 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Feb 2021 11:50:19 +0100 Subject: [PATCH 05/12] Remove usage of FromClaimsIdentity --- .../Security/AuthenticationExtensions.cs | 3 +- .../Security/ClaimsIdentityExtensions.cs | 4 +-- .../Security/ClaimsPrincipalExtensions.cs | 24 +++++--------- .../Security/UmbracoBackOfficeIdentity.cs | 32 +++++++++---------- .../UmbracoBackOfficeIdentityTests.cs | 28 ++++++++-------- .../Security/BackOfficeSecureDataFormat.cs | 13 +++++--- .../ConfigureBackOfficeCookieOptions.cs | 4 +-- .../Extensions/HttpContextExtensions.cs | 2 +- .../Security/BackOfficeUserManager.cs | 3 +- 9 files changed, 56 insertions(+), 57 deletions(-) diff --git a/src/Umbraco.Core/Security/AuthenticationExtensions.cs b/src/Umbraco.Core/Security/AuthenticationExtensions.cs index aa17b4e323..a51b4e7394 100644 --- a/src/Umbraco.Core/Security/AuthenticationExtensions.cs +++ b/src/Umbraco.Core/Security/AuthenticationExtensions.cs @@ -1,4 +1,5 @@ using System.Globalization; +using System.Security.Claims; using System.Security.Principal; using System.Threading; using Umbraco.Extensions; @@ -21,7 +22,7 @@ namespace Umbraco.Core.Security public static CultureInfo GetCulture(this IIdentity identity) { - if (identity is UmbracoBackOfficeIdentity umbIdentity && umbIdentity.IsAuthenticated) + if (identity is ClaimsIdentity umbIdentity && umbIdentity.VerifyBackOfficeIdentity(out _) && umbIdentity.IsAuthenticated) { return CultureInfo.GetCultureInfo(umbIdentity.GetCultureString()); } diff --git a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs index 9e304b8916..9278f6773e 100644 --- a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs @@ -21,8 +21,8 @@ namespace Umbraco.Extensions ClaimTypes.NameIdentifier, //id ClaimTypes.Name, //username ClaimTypes.GivenName, - Constants.Security.StartContentNodeIdClaimType, - Constants.Security.StartMediaNodeIdClaimType, + // Constants.Security.StartContentNodeIdClaimType, These seem to be able to be null... + // Constants.Security.StartMediaNodeIdClaimType, ClaimTypes.Locality, Constants.Security.SecurityStampClaimType }; diff --git a/src/Umbraco.Core/Security/ClaimsPrincipalExtensions.cs b/src/Umbraco.Core/Security/ClaimsPrincipalExtensions.cs index 395465cfb7..53a159a5fc 100644 --- a/src/Umbraco.Core/Security/ClaimsPrincipalExtensions.cs +++ b/src/Umbraco.Core/Security/ClaimsPrincipalExtensions.cs @@ -11,29 +11,21 @@ namespace Umbraco.Extensions public static class ClaimsPrincipalExtensions { /// - /// This will return the current back office identity if the IPrincipal is the correct type + /// This will return the current back office identity if the IPrincipal is the correct type and authenticated. /// /// /// - public static UmbracoBackOfficeIdentity GetUmbracoIdentity(this IPrincipal user) + public static ClaimsIdentity GetUmbracoIdentity(this IPrincipal user) { - // TODO: It would be nice to get rid of this and only rely on Claims, not a strongly typed identity instance - - //If it's already a UmbracoBackOfficeIdentity - if (user.Identity is UmbracoBackOfficeIdentity backOfficeIdentity) return backOfficeIdentity; - - //Check if there's more than one identity assigned and see if it's a UmbracoBackOfficeIdentity and use that - if (user is ClaimsPrincipal claimsPrincipal) - { - backOfficeIdentity = claimsPrincipal.Identities.OfType().FirstOrDefault(); - if (backOfficeIdentity != null) return backOfficeIdentity; - } - - //Otherwise convert to a UmbracoBackOfficeIdentity if it's auth'd + // Check if the identity is a ClaimsIdentity, and that's it's authenticated and has all required claims. if (user.Identity is ClaimsIdentity claimsIdentity && claimsIdentity.IsAuthenticated - && UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out var umbracoIdentity)) + && claimsIdentity.VerifyBackOfficeIdentity(out ClaimsIdentity umbracoIdentity)) { + if (claimsIdentity.AuthenticationType == Constants.Security.BackOfficeAuthenticationType) + { + return claimsIdentity; + } return umbracoIdentity; } diff --git a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs index db146d3cc9..86ebd7f7d3 100644 --- a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs +++ b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs @@ -15,22 +15,22 @@ namespace Umbraco.Core.Security // TODO: Ideally we remove this class and only deal with ClaimsIdentity as a best practice. All things relevant to our own // identity are part of claims. This class would essentially become extension methods on a ClaimsIdentity for resolving // values from it. - public static bool FromClaimsIdentity(ClaimsIdentity identity, out UmbracoBackOfficeIdentity backOfficeIdentity) - { - // validate that all claims exist - foreach (var t in RequiredBackOfficeIdentityClaimTypes) - { - // if the identity doesn't have the claim, or the claim value is null - if (identity.HasClaim(x => x.Type == t) == false || identity.HasClaim(x => x.Type == t && x.Value.IsNullOrWhiteSpace())) - { - backOfficeIdentity = null; - return false; - } - } - - backOfficeIdentity = new UmbracoBackOfficeIdentity(identity); - return true; - } + // public static bool FromClaimsIdentity(ClaimsIdentity identity, out UmbracoBackOfficeIdentity backOfficeIdentity) + // { + // // validate that all claims exist + // foreach (var t in RequiredBackOfficeIdentityClaimTypes) + // { + // // if the identity doesn't have the claim, or the claim value is null + // if (identity.HasClaim(x => x.Type == t) == false || identity.HasClaim(x => x.Type == t && x.Value.IsNullOrWhiteSpace())) + // { + // backOfficeIdentity = null; + // return false; + // } + // } + // + // backOfficeIdentity = new UmbracoBackOfficeIdentity(identity); + // return true; + // } /// /// Create a back office identity based on an existing claims identity diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs index ab241f2522..825abd6658 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs @@ -39,24 +39,24 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice new Claim(Constants.Security.SecurityStampClaimType, securityStamp, ClaimValueTypes.String, TestIssuer, TestIssuer), }); - if (!UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out UmbracoBackOfficeIdentity backofficeIdentity)) + if (!claimsIdentity.VerifyBackOfficeIdentity(out ClaimsIdentity verifiedIdentity)) { Assert.Fail(); } - Assert.IsNull(backofficeIdentity.Actor); - Assert.AreEqual(1234, backofficeIdentity.GetId()); + Assert.IsNull(verifiedIdentity.Actor); + Assert.AreEqual(1234, verifiedIdentity.GetId()); //// Assert.AreEqual(sessionId, backofficeIdentity.SessionId); - Assert.AreEqual(securityStamp, backofficeIdentity.GetSecurityStamp()); - Assert.AreEqual("testing", backofficeIdentity.GetUsername()); - Assert.AreEqual("hello world", backofficeIdentity.GetRealName()); - Assert.AreEqual(1, backofficeIdentity.GetStartContentNodes().Length); - Assert.IsTrue(backofficeIdentity.GetStartMediaNodes().UnsortedSequenceEqual(new[] { 5543, 5555 })); - Assert.IsTrue(new[] { "content", "media" }.SequenceEqual(backofficeIdentity.GetAllowedApplications())); - Assert.AreEqual("en-us", backofficeIdentity.GetCultureString()); - Assert.IsTrue(new[] { "admin" }.SequenceEqual(backofficeIdentity.GetRoles())); + Assert.AreEqual(securityStamp, verifiedIdentity.GetSecurityStamp()); + Assert.AreEqual("testing", verifiedIdentity.GetUsername()); + Assert.AreEqual("hello world", verifiedIdentity.GetRealName()); + Assert.AreEqual(1, verifiedIdentity.GetStartContentNodes().Length); + Assert.IsTrue(verifiedIdentity.GetStartMediaNodes().UnsortedSequenceEqual(new[] { 5543, 5555 })); + Assert.IsTrue(new[] { "content", "media" }.SequenceEqual(verifiedIdentity.GetAllowedApplications())); + Assert.AreEqual("en-us", verifiedIdentity.GetCultureString()); + Assert.IsTrue(new[] { "admin" }.SequenceEqual(verifiedIdentity.GetRoles())); - Assert.AreEqual(11, backofficeIdentity.Claims.Count()); + Assert.AreEqual(11, verifiedIdentity.Claims.Count()); } [Test] @@ -68,7 +68,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice new Claim(ClaimTypes.Name, "testing", ClaimValueTypes.String, TestIssuer, TestIssuer), }); - if (UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out _)) + if (claimsIdentity.VerifyBackOfficeIdentity(out _)) { Assert.Fail(); } @@ -93,7 +93,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice new Claim(ClaimsIdentity.DefaultRoleClaimType, "admin", ClaimValueTypes.String, TestIssuer, TestIssuer), }); - if (UmbracoBackOfficeIdentity.FromClaimsIdentity(claimsIdentity, out _)) + if (claimsIdentity.VerifyBackOfficeIdentity(out _)) { Assert.Fail(); } diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs index 377801a0b7..c16430097f 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs @@ -1,7 +1,9 @@ using Microsoft.AspNetCore.Authentication; using System; using System.Security.Claims; +using MimeKit.Cryptography; using Umbraco.Core.Security; +using Umbraco.Extensions; namespace Umbraco.Web.BackOffice.Security { @@ -19,7 +21,7 @@ namespace Umbraco.Web.BackOffice.Security _loginTimeoutMinutes = loginTimeoutMinutes; _ticketDataFormat = ticketDataFormat ?? throw new ArgumentNullException(nameof(ticketDataFormat)); } - + 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 @@ -38,7 +40,7 @@ namespace Umbraco.Web.BackOffice.Security public string Protect(AuthenticationTicket data) => Protect(data, string.Empty); - + public AuthenticationTicket Unprotect(string protectedText) => Unprotect(protectedText, string.Empty); /// @@ -59,11 +61,14 @@ namespace Umbraco.Web.BackOffice.Security return null; } - if (!UmbracoBackOfficeIdentity.FromClaimsIdentity((ClaimsIdentity)decrypt.Principal.Identity, out var identity)) + var identity = (ClaimsIdentity)decrypt.Principal.Identity; + if (!identity.VerifyBackOfficeIdentity(out ClaimsIdentity verifiedIdentity)) + { return null; + } //return the ticket with a UmbracoBackOfficeIdentity - var ticket = new AuthenticationTicket(new ClaimsPrincipal(identity), decrypt.Properties, decrypt.AuthenticationScheme); + var ticket = new AuthenticationTicket(new ClaimsPrincipal(verifiedIdentity), decrypt.Properties, decrypt.AuthenticationScheme); return ticket; } diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 557489cb73..8f0f0c4f8d 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -138,7 +138,7 @@ namespace Umbraco.Web.BackOffice.Security // Same goes for the signinmanager IBackOfficeSignInManager signInManager = ctx.HttpContext.RequestServices.GetRequiredService(); - UmbracoBackOfficeIdentity backOfficeIdentity = ctx.Principal.GetUmbracoIdentity(); + ClaimsIdentity backOfficeIdentity = ctx.Principal.GetUmbracoIdentity(); if (backOfficeIdentity == null) { ctx.RejectPrincipal(); @@ -165,7 +165,7 @@ namespace Umbraco.Web.BackOffice.Security OnSigningIn = ctx => { // occurs when sign in is successful but before the ticket is written to the outbound cookie - UmbracoBackOfficeIdentity backOfficeIdentity = ctx.Principal.GetUmbracoIdentity(); + ClaimsIdentity backOfficeIdentity = ctx.Principal.GetUmbracoIdentity(); if (backOfficeIdentity != null) { // generate a session id and assign it diff --git a/src/Umbraco.Web.Common/Extensions/HttpContextExtensions.cs b/src/Umbraco.Web.Common/Extensions/HttpContextExtensions.cs index f484ddac18..7472505231 100644 --- a/src/Umbraco.Web.Common/Extensions/HttpContextExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/HttpContextExtensions.cs @@ -35,7 +35,7 @@ namespace Umbraco.Extensions /// /// Returns the current back office identity if an admin is authenticated otherwise null /// - public static UmbracoBackOfficeIdentity GetCurrentIdentity(this HttpContext http) + public static ClaimsIdentity GetCurrentIdentity(this HttpContext http) { if (http == null) throw new ArgumentNullException(nameof(http)); if (http.User == null) return null; //there's no user at all so no identity diff --git a/src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs b/src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs index 081ca6b581..ed06affa7c 100644 --- a/src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs +++ b/src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Security.Claims; using System.Security.Principal; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; @@ -178,7 +179,7 @@ namespace Umbraco.Web.Common.Security private string GetCurrentUserId(IPrincipal currentUser) { - UmbracoBackOfficeIdentity umbIdentity = currentUser?.GetUmbracoIdentity(); + ClaimsIdentity umbIdentity = currentUser?.GetUmbracoIdentity(); var currentUserId = umbIdentity?.GetUserId() ?? Core.Constants.Security.SuperUserIdAsString; return currentUserId; } From b9d61f3ad8038ba7dc74b3cf8c36e148d379f862 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Feb 2021 14:17:38 +0100 Subject: [PATCH 06/12] Gut UmbracoBackOfficeIdentity --- .../Security/ClaimsIdentityExtensions.cs | 3 +- .../Security/UmbracoBackOfficeIdentity.cs | 182 ------------------ .../BackOfficeClaimsPrincipalFactory.cs | 7 +- .../BackOfficeClaimsPrincipalFactoryTests.cs | 4 +- .../UmbracoBackOfficeIdentityTests.cs | 7 +- .../ClaimsPrincipalExtensionsTests.cs | 9 +- .../Security/BackOfficeAntiforgeryTests.cs | 24 ++- .../AuthenticateEverythingMiddleware.cs | 15 +- .../ConfigureBackOfficeCookieOptions.cs | 9 +- 9 files changed, 48 insertions(+), 212 deletions(-) diff --git a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs index 9278f6773e..a7338cebb1 100644 --- a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs @@ -82,7 +82,8 @@ namespace Umbraco.Extensions } } - if (identity.HasClaim(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) == false) + if (identity.HasClaim(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) == false && + startMediaNodes != null) { foreach (var startMediaNode in startMediaNodes) { diff --git a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs index 86ebd7f7d3..03d0699a12 100644 --- a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs +++ b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs @@ -12,187 +12,5 @@ namespace Umbraco.Core.Security [Serializable] public class UmbracoBackOfficeIdentity : ClaimsIdentity { - // TODO: Ideally we remove this class and only deal with ClaimsIdentity as a best practice. All things relevant to our own - // identity are part of claims. This class would essentially become extension methods on a ClaimsIdentity for resolving - // values from it. - // public static bool FromClaimsIdentity(ClaimsIdentity identity, out UmbracoBackOfficeIdentity backOfficeIdentity) - // { - // // validate that all claims exist - // foreach (var t in RequiredBackOfficeIdentityClaimTypes) - // { - // // if the identity doesn't have the claim, or the claim value is null - // if (identity.HasClaim(x => x.Type == t) == false || identity.HasClaim(x => x.Type == t && x.Value.IsNullOrWhiteSpace())) - // { - // backOfficeIdentity = null; - // return false; - // } - // } - // - // backOfficeIdentity = new UmbracoBackOfficeIdentity(identity); - // return true; - // } - - /// - /// Create a back office identity based on an existing claims identity - /// - /// - private UmbracoBackOfficeIdentity(ClaimsIdentity identity) - : base(identity.Claims, Constants.Security.BackOfficeAuthenticationType) - { - } - - /// - /// Creates a new UmbracoBackOfficeIdentity - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - public UmbracoBackOfficeIdentity(string userId, string username, string realName, - IEnumerable startContentNodes, IEnumerable startMediaNodes, string culture, - string securityStamp, IEnumerable allowedApps, IEnumerable roles) - : base(Enumerable.Empty(), Constants.Security.BackOfficeAuthenticationType) //this ctor is used to ensure the IsAuthenticated property is true - { - if (allowedApps == null) - throw new ArgumentNullException(nameof(allowedApps)); - if (string.IsNullOrWhiteSpace(username)) - throw new ArgumentException("Value cannot be null or whitespace.", nameof(username)); - 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)); - AddRequiredClaims(userId, username, realName, startContentNodes, startMediaNodes, culture, securityStamp, allowedApps, roles); - } - - /// - /// Creates a new UmbracoBackOfficeIdentity - /// - /// - /// The original identity created by the ClaimsIdentityFactory - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - public UmbracoBackOfficeIdentity(ClaimsIdentity childIdentity, - string userId, string username, string realName, - IEnumerable startContentNodes, IEnumerable startMediaNodes, string culture, - string securityStamp, IEnumerable allowedApps, IEnumerable roles) - : base(childIdentity.Claims, Constants.Security.BackOfficeAuthenticationType) - { - if (string.IsNullOrWhiteSpace(username)) - throw new ArgumentException("Value cannot be null or whitespace.", nameof(username)); - 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)); - - AddRequiredClaims(userId, username, realName, startContentNodes, startMediaNodes, culture, securityStamp, allowedApps, roles); - } - - public const string Issuer = Constants.Security.BackOfficeAuthenticationType; - - /// - /// Returns the required claim types for a back office identity - /// - /// - /// This does not include the role claim type or allowed apps type since that is a collection and in theory could be empty - /// - public static IEnumerable RequiredBackOfficeIdentityClaimTypes => new[] - { - ClaimTypes.NameIdentifier, //id - ClaimTypes.Name, //username - ClaimTypes.GivenName, - Constants.Security.StartContentNodeIdClaimType, - Constants.Security.StartMediaNodeIdClaimType, - ClaimTypes.Locality, - Constants.Security.SecurityStampClaimType - }; - - /// - /// Adds claims based on the ctor data - /// - private void AddRequiredClaims(string userId, string username, string realName, - IEnumerable startContentNodes, IEnumerable startMediaNodes, string culture, - string securityStamp, IEnumerable allowedApps, IEnumerable roles) - { - // TODO: This has been moved, delete - //This is the id that 'identity' uses to check for the user id - if (HasClaim(x => x.Type == ClaimTypes.NameIdentifier) == false) - AddClaim(new Claim(ClaimTypes.NameIdentifier, userId, ClaimValueTypes.String, Issuer, Issuer, this)); - - if (HasClaim(x => x.Type == ClaimTypes.Name) == false) - AddClaim(new Claim(ClaimTypes.Name, username, ClaimValueTypes.String, Issuer, Issuer, this)); - - if (HasClaim(x => x.Type == ClaimTypes.GivenName) == false) - AddClaim(new Claim(ClaimTypes.GivenName, realName, ClaimValueTypes.String, Issuer, Issuer, this)); - - if (HasClaim(x => x.Type == Constants.Security.StartContentNodeIdClaimType) == false && startContentNodes != null) - { - foreach (var startContentNode in startContentNodes) - { - AddClaim(new Claim(Constants.Security.StartContentNodeIdClaimType, startContentNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, this)); - } - } - - if (HasClaim(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) == false && startMediaNodes != null) - { - foreach (var startMediaNode in startMediaNodes) - { - AddClaim(new Claim(Constants.Security.StartMediaNodeIdClaimType, startMediaNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, this)); - } - } - - if (HasClaim(x => x.Type == ClaimTypes.Locality) == false) - AddClaim(new Claim(ClaimTypes.Locality, culture, ClaimValueTypes.String, Issuer, Issuer, this)); - - //The security stamp claim is also required - if (HasClaim(x => x.Type == Constants.Security.SecurityStampClaimType) == false) - AddClaim(new Claim(Constants.Security.SecurityStampClaimType, securityStamp, ClaimValueTypes.String, Issuer, Issuer, this)); - - //Add each app as a separate claim - if (HasClaim(x => x.Type == Constants.Security.AllowedApplicationsClaimType) == false && allowedApps != null) - { - foreach (var application in allowedApps) - { - AddClaim(new Claim(Constants.Security.AllowedApplicationsClaimType, application, ClaimValueTypes.String, Issuer, Issuer, this)); - } - } - - //Claims are added by the ClaimsIdentityFactory because our UserStore supports roles, however this identity might - // not be made with that factory if it was created with a different ticket so perform the check - if (HasClaim(x => x.Type == DefaultRoleClaimType) == false && roles != null) - { - //manually add them - foreach (var roleName in roles) - { - AddClaim(new Claim(RoleClaimType, roleName, ClaimValueTypes.String, Issuer, Issuer, this)); - } - } - - } - - /// - /// - /// Gets the type of authenticated identity. - /// - /// - /// The type of authenticated identity. This property always returns "UmbracoBackOffice". - /// - public override string AuthenticationType => Issuer; } } diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs b/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs index 77f707d812..f3f284fa84 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs @@ -4,7 +4,7 @@ using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Options; -using Umbraco.Core.Security; +using Umbraco.Extensions; namespace Umbraco.Core.Security { @@ -45,8 +45,7 @@ namespace Umbraco.Core.Security // TODO: We want to remove UmbracoBackOfficeIdentity and only rely on ClaimsIdentity, once // that is done then we'll create a ClaimsIdentity with all of the requirements here instead - var umbracoIdentity = new UmbracoBackOfficeIdentity( - baseIdentity, + baseIdentity.AddRequiredClaims( user.Id, user.UserName, user.Name, @@ -57,7 +56,7 @@ namespace Umbraco.Core.Security user.AllowedSections, user.Roles.Select(x => x.RoleId).ToArray()); - return new ClaimsPrincipal(umbracoIdentity); + return new ClaimsPrincipal(baseIdentity); } /// diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/BackOfficeClaimsPrincipalFactoryTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/BackOfficeClaimsPrincipalFactoryTests.cs index e681fc6841..8662b6cfcb 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/BackOfficeClaimsPrincipalFactoryTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/BackOfficeClaimsPrincipalFactoryTests.cs @@ -56,13 +56,13 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice } [Test] - public async Task CreateAsync_Should_Create_Principal_With_Umbraco_Identity() + public async Task CreateAsync_Should_Create_Principal_With_Claims_Identity() { BackOfficeClaimsPrincipalFactory sut = CreateSut(); ClaimsPrincipal claimsPrincipal = await sut.CreateAsync(_testUser); - var umbracoBackOfficeIdentity = claimsPrincipal.Identity as UmbracoBackOfficeIdentity; + var umbracoBackOfficeIdentity = claimsPrincipal.Identity as ClaimsIdentity; Assert.IsNotNull(umbracoBackOfficeIdentity); } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs index 825abd6658..018c375982 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs @@ -112,8 +112,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice new Claim("TestClaim1", "test", ClaimValueTypes.Integer32, TestIssuer, TestIssuer) }); - var identity = new UmbracoBackOfficeIdentity( - claimsIdentity, + claimsIdentity.AddRequiredClaims( "1234", "testing", "hello world", @@ -124,8 +123,8 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice new[] { "content", "media" }, new[] { "admin" }); - Assert.AreEqual(12, identity.Claims.Count()); - Assert.IsNull(identity.Actor); + Assert.AreEqual(12, claimsIdentity.Claims.Count()); + Assert.IsNull(claimsIdentity.Actor); } } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs index 7b699e7b0c..d8930cc58e 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs @@ -8,6 +8,7 @@ using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Security; using Umbraco.Extensions; +using ClaimsIdentityExtensions = Umbraco.Extensions.ClaimsIdentityExtensions; namespace Umbraco.Tests.UnitTests.Umbraco.Core.Extensions { @@ -17,7 +18,8 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Extensions [Test] public void Get_Remaining_Ticket_Seconds() { - var backOfficeIdentity = new UmbracoBackOfficeIdentity( + var backOfficeIdentity = new ClaimsIdentity(); + backOfficeIdentity.AddRequiredClaims( Constants.Security.SuperUserIdAsString, "test", "test", @@ -27,6 +29,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Extensions Guid.NewGuid().ToString(), Enumerable.Empty(), Enumerable.Empty()); + var principal = new ClaimsPrincipal(backOfficeIdentity); var expireSeconds = 99; @@ -40,8 +43,8 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Extensions Constants.Security.TicketExpiresClaimType, expires, ClaimValueTypes.DateTime, - UmbracoBackOfficeIdentity.Issuer, - UmbracoBackOfficeIdentity.Issuer, + ClaimsIdentityExtensions.Issuer, + ClaimsIdentityExtensions.Issuer, backOfficeIdentity)); var ticketRemainingSeconds = principal.GetRemainingAuthSeconds(then); diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeAntiforgeryTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeAntiforgeryTests.cs index 568318006e..494ab7fdc3 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeAntiforgeryTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeAntiforgeryTests.cs @@ -13,6 +13,7 @@ using Microsoft.Net.Http.Headers; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Security; +using Umbraco.Extensions; using Umbraco.Web.BackOffice.Security; namespace Umbraco.Tests.UnitTests.Umbraco.Web.BackOffice.Security @@ -22,18 +23,21 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.BackOffice.Security { private HttpContext GetHttpContext() { + var identity = new ClaimsIdentity(); + identity.AddRequiredClaims( + Constants.Security.SuperUserIdAsString, + "test", + "test", + Enumerable.Empty(), + Enumerable.Empty(), + "en-US", + Guid.NewGuid().ToString(), + Enumerable.Empty(), + Enumerable.Empty()); + var httpContext = new DefaultHttpContext() { - User = new ClaimsPrincipal(new UmbracoBackOfficeIdentity( - Constants.Security.SuperUserIdAsString, - "test", - "test", - Enumerable.Empty(), - Enumerable.Empty(), - "en-US", - Guid.NewGuid().ToString(), - Enumerable.Empty(), - Enumerable.Empty())) + User = new ClaimsPrincipal(identity) }; httpContext.Request.IsHttps = true; return httpContext; diff --git a/src/Umbraco.Tests/TestHelpers/ControllerTesting/AuthenticateEverythingMiddleware.cs b/src/Umbraco.Tests/TestHelpers/ControllerTesting/AuthenticateEverythingMiddleware.cs index e702753236..104486bf14 100644 --- a/src/Umbraco.Tests/TestHelpers/ControllerTesting/AuthenticateEverythingMiddleware.cs +++ b/src/Umbraco.Tests/TestHelpers/ControllerTesting/AuthenticateEverythingMiddleware.cs @@ -1,10 +1,12 @@ using System; +using System.Security.Claims; using System.Threading.Tasks; using Microsoft.Owin; using Microsoft.Owin.Security; using Microsoft.Owin.Security.Infrastructure; using Owin; using Umbraco.Core.Security; +using Umbraco.Extensions; namespace Umbraco.Tests.TestHelpers.ControllerTesting { @@ -28,8 +30,17 @@ namespace Umbraco.Tests.TestHelpers.ControllerTesting protected override Task AuthenticateCoreAsync() { var securityStamp = Guid.NewGuid().ToString(); - var identity = new UmbracoBackOfficeIdentity( - Umbraco.Core.Constants.Security.SuperUserIdAsString, "admin", "Admin", new[] { -1 }, new[] { -1 }, "en-US", securityStamp, new[] { "content", "media", "members" }, new[] { "admin" }); + + var identity = new ClaimsIdentity(); + identity.AddRequiredClaims(Core.Constants.Security.SuperUserIdAsString, + "admin", + "Admin", + new[] { -1 }, + new[] { -1 }, + "en-US", + securityStamp, + new[] { "content", "media", "members" }, + new[] { "admin" }); return Task.FromResult(new AuthenticationTicket( identity, diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 8f0f0c4f8d..2223fbfb6b 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -19,6 +19,7 @@ using Umbraco.Core.Services; using Umbraco.Extensions; using Umbraco.Net; using Umbraco.Web.Common.Security; +using ClaimsIdentityExtensions = Umbraco.Extensions.ClaimsIdentityExtensions; namespace Umbraco.Web.BackOffice.Security { @@ -157,8 +158,8 @@ namespace Umbraco.Web.BackOffice.Security Constants.Security.TicketExpiresClaimType, ctx.Properties.ExpiresUtc.Value.ToString("o"), ClaimValueTypes.DateTime, - UmbracoBackOfficeIdentity.Issuer, - UmbracoBackOfficeIdentity.Issuer, + ClaimsIdentityExtensions.Issuer, + ClaimsIdentityExtensions.Issuer, backOfficeIdentity)); }, @@ -175,10 +176,10 @@ namespace Umbraco.Web.BackOffice.Security : Guid.NewGuid(); // add our session claim - backOfficeIdentity.AddClaim(new Claim(Constants.Security.SessionIdClaimType, session.ToString(), ClaimValueTypes.String, UmbracoBackOfficeIdentity.Issuer, UmbracoBackOfficeIdentity.Issuer, backOfficeIdentity)); + backOfficeIdentity.AddClaim(new Claim(Constants.Security.SessionIdClaimType, session.ToString(), ClaimValueTypes.String, ClaimsIdentityExtensions.Issuer, ClaimsIdentityExtensions.Issuer, backOfficeIdentity)); // since it is a cookie-based authentication add that claim - backOfficeIdentity.AddClaim(new Claim(ClaimTypes.CookiePath, "/", ClaimValueTypes.String, UmbracoBackOfficeIdentity.Issuer, UmbracoBackOfficeIdentity.Issuer, backOfficeIdentity)); + backOfficeIdentity.AddClaim(new Claim(ClaimTypes.CookiePath, "/", ClaimValueTypes.String, ClaimsIdentityExtensions.Issuer, ClaimsIdentityExtensions.Issuer, backOfficeIdentity)); } return Task.CompletedTask; From 8ba3f7ccb4a96566c5efb244936f75ca60ebff3b Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Feb 2021 14:21:59 +0100 Subject: [PATCH 07/12] Remove UmbracoBackOfficeIdentity --- .../Security/UmbracoBackOfficeIdentity.cs | 16 ---------------- .../Security/BackOfficeClaimsPrincipalFactory.cs | 2 +- .../TestControllerActivatorBase.cs | 3 ++- .../CheckIfUserTicketDataIsStaleAttribute.cs | 2 +- .../Security/BackOfficeSecureDataFormat.cs | 2 +- 5 files changed, 5 insertions(+), 20 deletions(-) delete mode 100644 src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs diff --git a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs b/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs deleted file mode 100644 index 03d0699a12..0000000000 --- a/src/Umbraco.Core/Security/UmbracoBackOfficeIdentity.cs +++ /dev/null @@ -1,16 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Security.Claims; - -namespace Umbraco.Core.Security -{ - - /// - /// A custom user identity for the Umbraco backoffice - /// - [Serializable] - public class UmbracoBackOfficeIdentity : ClaimsIdentity - { - } -} diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs b/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs index f3f284fa84..906d15c220 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs @@ -26,7 +26,7 @@ namespace Umbraco.Core.Security /// /// - /// Returns a custom and allows flowing claims from the external identity + /// Returns a ClaimsIdentity that has the required claims, and allows flowing of claims from external identity /// public override async Task CreateAsync(BackOfficeIdentityUser user) { diff --git a/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs b/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs index 3e9add3b89..18115b9dc9 100644 --- a/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs +++ b/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Net.Http; +using System.Security.Claims; using System.Web; using System.Web.Http; using System.Web.Http.Controllers; @@ -93,7 +94,7 @@ namespace Umbraco.Tests.TestHelpers.ControllerTesting //chuck it into the props since this is what MS does when hosted and it's needed there request.Properties["MS_HttpContext"] = httpContext; - var backofficeIdentity = (UmbracoBackOfficeIdentity) owinContext.Authentication.User.Identity; + var backofficeIdentity = (ClaimsIdentity) owinContext.Authentication.User.Identity; var backofficeSecurity = new Mock(); diff --git a/src/Umbraco.Web.BackOffice/Filters/CheckIfUserTicketDataIsStaleAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/CheckIfUserTicketDataIsStaleAttribute.cs index db3da94eb1..88182464e0 100644 --- a/src/Umbraco.Web.BackOffice/Filters/CheckIfUserTicketDataIsStaleAttribute.cs +++ b/src/Umbraco.Web.BackOffice/Filters/CheckIfUserTicketDataIsStaleAttribute.cs @@ -112,7 +112,7 @@ namespace Umbraco.Web.BackOffice.Filters return; } - var identity = actionContext.HttpContext.User.Identity as UmbracoBackOfficeIdentity; + var identity = actionContext.HttpContext.User.Identity as ClaimsIdentity; if (identity == null) { return; diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs index c16430097f..0e12a173f7 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSecureDataFormat.cs @@ -9,7 +9,7 @@ namespace Umbraco.Web.BackOffice.Security { /// - /// Custom secure format that ensures the Identity in the ticket is and not just a ClaimsIdentity + /// Custom secure format that ensures the Identity in the ticket is verified /// internal class BackOfficeSecureDataFormat : ISecureDataFormat { From 63c8365e6a136cc4db37d1627cd8832ab1fa0d09 Mon Sep 17 00:00:00 2001 From: Mole Date: Mon, 22 Feb 2021 08:43:28 +0100 Subject: [PATCH 08/12] Fix merge and consolidate ClaimsIdentityExtensions into one file. --- .../Extensions/ClaimsIdentityExtensions.cs | 201 ++++++++++++++++++ .../Security/ClaimsIdentityExtensions.cs | 153 ------------- .../Security/BackOfficeAntiforgeryTests.cs | 4 +- .../AuthenticateEverythingMiddleware.cs | 2 +- .../ConfigureBackOfficeCookieOptions.cs | 4 - 5 files changed, 204 insertions(+), 160 deletions(-) delete mode 100644 src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs diff --git a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs index 3d58253dd9..08a0b01749 100644 --- a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs @@ -2,8 +2,11 @@ // See LICENSE for more details. using System; +using System.Collections.Generic; +using System.Linq; using System.Security.Claims; using System.Security.Principal; +using Umbraco.Cms.Core; namespace Umbraco.Extensions { @@ -72,5 +75,203 @@ namespace Umbraco.Extensions return identity.FindFirst(claimType)?.Value; } + + /// + /// Returns the required claim types for a back office identity + /// + /// + /// This does not include the role claim type or allowed apps type since that is a collection and in theory could be empty + /// + public static IEnumerable RequiredBackOfficeClaimTypes => new[] + { + ClaimTypes.NameIdentifier, //id + ClaimTypes.Name, //username + ClaimTypes.GivenName, + // Constants.Security.StartContentNodeIdClaimType, These seem to be able to be null... + // Constants.Security.StartMediaNodeIdClaimType, + ClaimTypes.Locality, + Constants.Security.SecurityStampClaimType + }; + + public const string Issuer = Constants.Security.BackOfficeAuthenticationType; + + /// + /// Verify that a ClaimsIdentity has all the required claim types + /// + /// + /// Verified identity wrapped in a ClaimsIdentity with BackOfficeAuthentication type + /// True if ClaimsIdentity + public static bool VerifyBackOfficeIdentity(this ClaimsIdentity identity, out ClaimsIdentity verifiedIdentity) + { + // Validate that all required claims exist + foreach (var claimType in RequiredBackOfficeClaimTypes) + { + if (identity.HasClaim(x => x.Type == claimType) == false || + identity.HasClaim(x => x.Type == claimType && x.Value.IsNullOrWhiteSpace())) + { + verifiedIdentity = null; + return false; + } + } + + verifiedIdentity = new ClaimsIdentity(identity.Claims, Constants.Security.BackOfficeAuthenticationType); + return true; + } + + /// + /// Add the required claims to be a BackOffice ClaimsIdentity + /// + /// this + /// The users Id + /// Username + /// Real name + /// Start content nodes + /// Start media nodes + /// The locality of the user + /// Security stamp + /// Allowed apps + /// Roles + public static void AddRequiredClaims(this ClaimsIdentity identity, string userId, string username, + string realName, IEnumerable startContentNodes, IEnumerable startMediaNodes, string culture, + string securityStamp, IEnumerable allowedApps, IEnumerable roles) + { + //This is the id that 'identity' uses to check for the user id + if (identity.HasClaim(x => x.Type == ClaimTypes.NameIdentifier) == false) + { + identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, userId, ClaimValueTypes.String, + Issuer, Issuer, identity)); + } + + if (identity.HasClaim(x => x.Type == ClaimTypes.Name) == false) + { + identity.AddClaim(new Claim(ClaimTypes.Name, username, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + + if (identity.HasClaim(x => x.Type == ClaimTypes.GivenName) == false) + { + identity.AddClaim(new Claim(ClaimTypes.GivenName, realName, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + + if (identity.HasClaim(x => x.Type == Constants.Security.StartContentNodeIdClaimType) == false && + startContentNodes != null) + { + foreach (var startContentNode in startContentNodes) + { + identity.AddClaim(new Claim(Constants.Security.StartContentNodeIdClaimType, startContentNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, identity)); + } + } + + if (identity.HasClaim(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) == false && + startMediaNodes != null) + { + foreach (var startMediaNode in startMediaNodes) + { + identity.AddClaim(new Claim(Constants.Security.StartMediaNodeIdClaimType, startMediaNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, identity)); + } + } + + if (identity.HasClaim(x => x.Type == ClaimTypes.Locality) == false) + { + identity.AddClaim(new Claim(ClaimTypes.Locality, culture, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + + // The security stamp claim is also required + if (identity.HasClaim(x => x.Type == Constants.Security.SecurityStampClaimType) == false) + { + identity.AddClaim(new Claim(Constants.Security.SecurityStampClaimType, securityStamp, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + + // Add each app as a separate claim + if (identity.HasClaim(x => x.Type == Constants.Security.AllowedApplicationsClaimType) == false && + allowedApps != null) + { + foreach (var application in allowedApps) + { + identity.AddClaim(new Claim(Constants.Security.AllowedApplicationsClaimType, application, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + } + + // Claims are added by the ClaimsIdentityFactory because our UserStore supports roles, however this identity might + // not be made with that factory if it was created with a different ticket so perform the check + if (identity.HasClaim(x => x.Type == ClaimsIdentity.DefaultRoleClaimType) == false && roles != null) + { + // Manually add them + foreach (var roleName in roles) + { + identity.AddClaim(new Claim(identity.RoleClaimType, roleName, ClaimValueTypes.String, Issuer, Issuer, identity)); + } + } + } + + /// + /// Get the start content nodes from a ClaimsIdentity + /// + /// + /// Array of start content nodes + public static int[] GetStartContentNodes(this ClaimsIdentity identity) => + identity.FindAll(x => x.Type == Constants.Security.StartContentNodeIdClaimType) + .Select(node => int.TryParse(node.Value, out var i) ? i : default) + .Where(x => x != default).ToArray(); + + /// + /// Get the start media nodes from a ClaimsIdentity + /// + /// + /// Array of start media nodes + public static int[] GetStartMediaNodes(this ClaimsIdentity identity) => + identity.FindAll(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) + .Select(node => int.TryParse(node.Value, out var i) ? i : default) + .Where(x => x != default).ToArray(); + + /// + /// Get the allowed applications from a ClaimsIdentity + /// + /// + /// + public static string[] GetAllowedApplications(this ClaimsIdentity identity) => identity + .FindAll(x => x.Type == Constants.Security.AllowedApplicationsClaimType).Select(app => app.Value).ToArray(); + + /// + /// Get the user ID from a ClaimsIdentity + /// + /// + /// User ID as integer + public static int GetId(this ClaimsIdentity identity) => int.Parse(identity.FindFirstValue(ClaimTypes.NameIdentifier)); + + /// + /// Get the real name belonging to the user from a ClaimsIdentity + /// + /// + /// Real name of the user + public static string GetRealName(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.GivenName); + + /// + /// Get the username of the user from a ClaimsIdentity + /// + /// + /// Username of the user + public static string GetUsername(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Name); + + /// + /// Get the culture string from a ClaimsIdentity + /// + /// + /// Culture string + public static string GetCultureString(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Locality); + + /// + /// Get the security stamp from a ClaimsIdentity + /// + /// + /// Security stamp + public static string GetSecurityStamp(this ClaimsIdentity identity) => identity.FindFirstValue(Constants.Security.SecurityStampClaimType); + + /// + /// Get the roles assigned to a user from a ClaimsIdentity + /// + /// + /// Array of roles + public static string[] GetRoles(this ClaimsIdentity identity) => identity + .FindAll(x => x.Type == ClaimsIdentity.DefaultRoleClaimType).Select(role => role.Value).ToArray(); } } diff --git a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs deleted file mode 100644 index a7338cebb1..0000000000 --- a/src/Umbraco.Core/Security/ClaimsIdentityExtensions.cs +++ /dev/null @@ -1,153 +0,0 @@ -// Copyright (c) Umbraco. -// See LICENSE for more details. - -using System.Collections.Generic; -using System.Linq; -using System.Security.Claims; -using Umbraco.Core; - -namespace Umbraco.Extensions -{ - public static class ClaimsIdentityExtensions - { - /// - /// Returns the required claim types for a back office identity - /// - /// - /// This does not include the role claim type or allowed apps type since that is a collection and in theory could be empty - /// - public static IEnumerable RequiredBackOfficeClaimTypes => new[] - { - ClaimTypes.NameIdentifier, //id - ClaimTypes.Name, //username - ClaimTypes.GivenName, - // Constants.Security.StartContentNodeIdClaimType, These seem to be able to be null... - // Constants.Security.StartMediaNodeIdClaimType, - ClaimTypes.Locality, - Constants.Security.SecurityStampClaimType - }; - - public const string Issuer = Constants.Security.BackOfficeAuthenticationType; - - /// - /// Verify that a ClaimsIdentity has all the required claim types - /// - /// - /// Verified identity wrapped in a ClaimsIdentity with BackOfficeAuthentication type - /// True if ClaimsIdentity - public static bool VerifyBackOfficeIdentity(this ClaimsIdentity identity, out ClaimsIdentity verifiedIdentity) - { - // Validate that all required claims exist - foreach (var claimType in RequiredBackOfficeClaimTypes) - { - if (identity.HasClaim(x => x.Type == claimType) == false || - identity.HasClaim(x => x.Type == claimType && x.Value.IsNullOrWhiteSpace())) - { - verifiedIdentity = null; - return false; - } - } - - verifiedIdentity = new ClaimsIdentity(identity.Claims, Constants.Security.BackOfficeAuthenticationType); - return true; - } - - public static void AddRequiredClaims(this ClaimsIdentity identity, string userId, string username, - string realName, IEnumerable startContentNodes, IEnumerable startMediaNodes, string culture, - string securityStamp, IEnumerable allowedApps, IEnumerable roles) - { - //This is the id that 'identity' uses to check for the user id - if (identity.HasClaim(x => x.Type == ClaimTypes.NameIdentifier) == false) - { - identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, userId, ClaimValueTypes.String, - Issuer, Issuer, identity)); - } - - if (identity.HasClaim(x => x.Type == ClaimTypes.Name) == false) - { - identity.AddClaim(new Claim(ClaimTypes.Name, username, ClaimValueTypes.String, Issuer, Issuer, identity)); - } - - if (identity.HasClaim(x => x.Type == ClaimTypes.GivenName) == false) - { - identity.AddClaim(new Claim(ClaimTypes.GivenName, realName, ClaimValueTypes.String, Issuer, Issuer, identity)); - } - - if (identity.HasClaim(x => x.Type == Constants.Security.StartContentNodeIdClaimType) == false && - startContentNodes != null) - { - foreach (var startContentNode in startContentNodes) - { - identity.AddClaim(new Claim(Constants.Security.StartContentNodeIdClaimType, startContentNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, identity)); - } - } - - if (identity.HasClaim(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) == false && - startMediaNodes != null) - { - foreach (var startMediaNode in startMediaNodes) - { - identity.AddClaim(new Claim(Constants.Security.StartMediaNodeIdClaimType, startMediaNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, identity)); - } - } - - if (identity.HasClaim(x => x.Type == ClaimTypes.Locality) == false) - { - identity.AddClaim(new Claim(ClaimTypes.Locality, culture, ClaimValueTypes.String, Issuer, Issuer, identity)); - } - - // The security stamp claim is also required - if (identity.HasClaim(x => x.Type == Constants.Security.SecurityStampClaimType) == false) - { - identity.AddClaim(new Claim(Constants.Security.SecurityStampClaimType, securityStamp, ClaimValueTypes.String, Issuer, Issuer, identity)); - } - - // Add each app as a separate claim - if (identity.HasClaim(x => x.Type == Constants.Security.AllowedApplicationsClaimType) == false && - allowedApps != null) - { - foreach (var application in allowedApps) - { - identity.AddClaim(new Claim(Constants.Security.AllowedApplicationsClaimType, application, ClaimValueTypes.String, Issuer, Issuer, identity)); - } - } - - // Claims are added by the ClaimsIdentityFactory because our UserStore supports roles, however this identity might - // not be made with that factory if it was created with a different ticket so perform the check - if (identity.HasClaim(x => x.Type == ClaimsIdentity.DefaultRoleClaimType) == false && roles != null) - { - // Manually add them - foreach (var roleName in roles) - { - identity.AddClaim(new Claim(identity.RoleClaimType, roleName, ClaimValueTypes.String, Issuer, Issuer, identity)); - } - } - } - - public static int[] GetStartContentNodes(this ClaimsIdentity identity) => - identity.FindAll(x => x.Type == Constants.Security.StartContentNodeIdClaimType) - .Select(node => int.TryParse(node.Value, out var i) ? i : default) - .Where(x => x != default).ToArray(); - - public static int[] GetStartMediaNodes(this ClaimsIdentity identity) => - identity.FindAll(x => x.Type == Constants.Security.StartMediaNodeIdClaimType) - .Select(node => int.TryParse(node.Value, out var i) ? i : default) - .Where(x => x != default).ToArray(); - - public static string[] GetAllowedApplications(this ClaimsIdentity identity) => identity - .FindAll(x => x.Type == Constants.Security.AllowedApplicationsClaimType).Select(app => app.Value).ToArray(); - - public static int GetId(this ClaimsIdentity identity) => int.Parse(identity.FindFirstValue(ClaimTypes.NameIdentifier)); - - public static string GetRealName(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.GivenName); - - public static string GetUsername(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Name); - - public static string GetCultureString(this ClaimsIdentity identity) => identity.FindFirstValue(ClaimTypes.Locality); - - public static string GetSecurityStamp(this ClaimsIdentity identity) => identity.FindFirstValue(Constants.Security.SecurityStampClaimType); - - public static string[] GetRoles(this ClaimsIdentity identity) => identity - .FindAll(x => x.Type == ClaimsIdentity.DefaultRoleClaimType).Select(role => role.Value).ToArray(); - } -} diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeAntiforgeryTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeAntiforgeryTests.cs index e84e70adaf..f3abcabe61 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeAntiforgeryTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeAntiforgeryTests.cs @@ -11,9 +11,9 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using Microsoft.Net.Http.Headers; using NUnit.Framework; -using Umbraco.Core; +using Umbraco.Cms.Core; +using Umbraco.Cms.Web.BackOffice.Security; using Umbraco.Extensions; -using Umbraco.Web.BackOffice.Security; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Security { diff --git a/src/Umbraco.Tests/TestHelpers/ControllerTesting/AuthenticateEverythingMiddleware.cs b/src/Umbraco.Tests/TestHelpers/ControllerTesting/AuthenticateEverythingMiddleware.cs index a26945672c..1561ad5611 100644 --- a/src/Umbraco.Tests/TestHelpers/ControllerTesting/AuthenticateEverythingMiddleware.cs +++ b/src/Umbraco.Tests/TestHelpers/ControllerTesting/AuthenticateEverythingMiddleware.cs @@ -32,7 +32,7 @@ namespace Umbraco.Tests.TestHelpers.ControllerTesting var identity = new ClaimsIdentity(); identity.AddRequiredClaims( - Core.Constants.Security.SuperUserIdAsString, + Cms.Core.Constants.Security.SuperUserIdAsString, "admin", "Admin", new[] { -1 }, diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 0d1f740843..4aaa6ec4b1 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -5,7 +5,6 @@ using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.DataProtection; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using Umbraco.Cms.Core; @@ -13,12 +12,9 @@ using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Hosting; using Umbraco.Cms.Core.Net; using Umbraco.Cms.Core.Routing; -using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Web; using Umbraco.Extensions; -using Umbraco.Net; -using Umbraco.Web.Common.Security; using ClaimsIdentityExtensions = Umbraco.Extensions.ClaimsIdentityExtensions; using Constants = Umbraco.Cms.Core.Constants; From 1c1394e0bf5eb84ae7bda68fca70a5e31d98d7ad Mon Sep 17 00:00:00 2001 From: Mole Date: Mon, 22 Feb 2021 15:30:28 +0100 Subject: [PATCH 09/12] Replace usages of ClaimsIdentityExtensions.Issuer with Constants.Security.BackOfficeAuthenticationType Also remove todo and cases of using ClaimsIdentityExtensions = Umbraco.Extensions.ClaimsIdentityExtensions; --- .../Extensions/ClaimsIdentityExtensions.cs | 75 ++++++++++++++++--- .../BackOfficeClaimsPrincipalFactory.cs | 2 - .../ClaimsPrincipalExtensionsTests.cs | 5 +- .../ConfigureBackOfficeCookieOptions.cs | 9 +-- 4 files changed, 69 insertions(+), 22 deletions(-) diff --git a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs index 08a0b01749..62da5b0bf8 100644 --- a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs @@ -93,8 +93,6 @@ namespace Umbraco.Extensions Constants.Security.SecurityStampClaimType }; - public const string Issuer = Constants.Security.BackOfficeAuthenticationType; - /// /// Verify that a ClaimsIdentity has all the required claim types /// @@ -138,18 +136,35 @@ namespace Umbraco.Extensions //This is the id that 'identity' uses to check for the user id if (identity.HasClaim(x => x.Type == ClaimTypes.NameIdentifier) == false) { - identity.AddClaim(new Claim(ClaimTypes.NameIdentifier, userId, ClaimValueTypes.String, - Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + ClaimTypes.NameIdentifier, + userId, + ClaimValueTypes.String, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } if (identity.HasClaim(x => x.Type == ClaimTypes.Name) == false) { - identity.AddClaim(new Claim(ClaimTypes.Name, username, ClaimValueTypes.String, Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + ClaimTypes.Name, + username, + ClaimValueTypes.String, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } if (identity.HasClaim(x => x.Type == ClaimTypes.GivenName) == false) { - identity.AddClaim(new Claim(ClaimTypes.GivenName, realName, ClaimValueTypes.String, Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + ClaimTypes.GivenName, + realName, + ClaimValueTypes.String, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } if (identity.HasClaim(x => x.Type == Constants.Security.StartContentNodeIdClaimType) == false && @@ -157,7 +172,13 @@ namespace Umbraco.Extensions { foreach (var startContentNode in startContentNodes) { - identity.AddClaim(new Claim(Constants.Security.StartContentNodeIdClaimType, startContentNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + Constants.Security.StartContentNodeIdClaimType, + startContentNode.ToInvariantString(), + ClaimValueTypes.Integer32, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } } @@ -166,19 +187,37 @@ namespace Umbraco.Extensions { foreach (var startMediaNode in startMediaNodes) { - identity.AddClaim(new Claim(Constants.Security.StartMediaNodeIdClaimType, startMediaNode.ToInvariantString(), ClaimValueTypes.Integer32, Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + Constants.Security.StartMediaNodeIdClaimType, + startMediaNode.ToInvariantString(), + ClaimValueTypes.Integer32, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } } if (identity.HasClaim(x => x.Type == ClaimTypes.Locality) == false) { - identity.AddClaim(new Claim(ClaimTypes.Locality, culture, ClaimValueTypes.String, Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + ClaimTypes.Locality, + culture, + ClaimValueTypes.String, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } // The security stamp claim is also required if (identity.HasClaim(x => x.Type == Constants.Security.SecurityStampClaimType) == false) { - identity.AddClaim(new Claim(Constants.Security.SecurityStampClaimType, securityStamp, ClaimValueTypes.String, Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + Constants.Security.SecurityStampClaimType, + securityStamp, + ClaimValueTypes.String, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } // Add each app as a separate claim @@ -187,7 +226,13 @@ namespace Umbraco.Extensions { foreach (var application in allowedApps) { - identity.AddClaim(new Claim(Constants.Security.AllowedApplicationsClaimType, application, ClaimValueTypes.String, Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + Constants.Security.AllowedApplicationsClaimType, + application, + ClaimValueTypes.String, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } } @@ -198,7 +243,13 @@ namespace Umbraco.Extensions // Manually add them foreach (var roleName in roles) { - identity.AddClaim(new Claim(identity.RoleClaimType, roleName, ClaimValueTypes.String, Issuer, Issuer, identity)); + identity.AddClaim(new Claim( + identity.RoleClaimType, + roleName, + ClaimValueTypes.String, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + identity)); } } } diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs b/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs index 906d15c220..a57a5c1737 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs @@ -43,8 +43,6 @@ namespace Umbraco.Core.Security baseIdentity.AddClaim(new Claim(claim.ClaimType, claim.ClaimValue)); } - // TODO: We want to remove UmbracoBackOfficeIdentity and only rely on ClaimsIdentity, once - // that is done then we'll create a ClaimsIdentity with all of the requirements here instead baseIdentity.AddRequiredClaims( user.Id, user.UserName, diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs index b7f2392c30..0a941f9b59 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs @@ -8,7 +8,6 @@ using NUnit.Framework; using Umbraco.Cms.Core.Security; using Umbraco.Extensions; using Constants = Umbraco.Cms.Core.Constants; -using ClaimsIdentityExtensions = Umbraco.Extensions.ClaimsIdentityExtensions; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Extensions { @@ -43,8 +42,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Extensions Constants.Security.TicketExpiresClaimType, expires, ClaimValueTypes.DateTime, - ClaimsIdentityExtensions.Issuer, - ClaimsIdentityExtensions.Issuer, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, backOfficeIdentity)); var ticketRemainingSeconds = principal.GetRemainingAuthSeconds(then); diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 4aaa6ec4b1..248fd72ac2 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -15,7 +15,6 @@ using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Web; using Umbraco.Extensions; -using ClaimsIdentityExtensions = Umbraco.Extensions.ClaimsIdentityExtensions; using Constants = Umbraco.Cms.Core.Constants; namespace Umbraco.Cms.Web.BackOffice.Security @@ -155,8 +154,8 @@ namespace Umbraco.Cms.Web.BackOffice.Security Constants.Security.TicketExpiresClaimType, ctx.Properties.ExpiresUtc.Value.ToString("o"), ClaimValueTypes.DateTime, - ClaimsIdentityExtensions.Issuer, - ClaimsIdentityExtensions.Issuer, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, backOfficeIdentity)); }, @@ -173,10 +172,10 @@ namespace Umbraco.Cms.Web.BackOffice.Security : Guid.NewGuid(); // add our session claim - backOfficeIdentity.AddClaim(new Claim(Constants.Security.SessionIdClaimType, session.ToString(), ClaimValueTypes.String, ClaimsIdentityExtensions.Issuer, ClaimsIdentityExtensions.Issuer, backOfficeIdentity)); + backOfficeIdentity.AddClaim(new Claim(Constants.Security.SessionIdClaimType, session.ToString(), ClaimValueTypes.String, Constants.Security.BackOfficeAuthenticationType, Constants.Security.BackOfficeAuthenticationType, backOfficeIdentity)); // since it is a cookie-based authentication add that claim - backOfficeIdentity.AddClaim(new Claim(ClaimTypes.CookiePath, "/", ClaimValueTypes.String, ClaimsIdentityExtensions.Issuer, ClaimsIdentityExtensions.Issuer, backOfficeIdentity)); + backOfficeIdentity.AddClaim(new Claim(ClaimTypes.CookiePath, "/", ClaimValueTypes.String, Constants.Security.BackOfficeAuthenticationType, Constants.Security.BackOfficeAuthenticationType, backOfficeIdentity)); } return Task.CompletedTask; From f1128d7d70f08d3a412f78c665a763bb0f10c862 Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 23 Feb 2021 08:38:27 +0100 Subject: [PATCH 10/12] Replace usage of Thread.CurrentPrincipal with IBackofficeSecurityAccessor --- .../Compose/AuditEventsComponent.cs | 14 +++++++++++--- src/Umbraco.Web.Common/Security/MemberManager.cs | 3 ++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Infrastructure/Compose/AuditEventsComponent.cs b/src/Umbraco.Infrastructure/Compose/AuditEventsComponent.cs index f76d3b3a91..8dc25d85eb 100644 --- a/src/Umbraco.Infrastructure/Compose/AuditEventsComponent.cs +++ b/src/Umbraco.Infrastructure/Compose/AuditEventsComponent.cs @@ -26,14 +26,22 @@ namespace Umbraco.Core.Compose private readonly IUserService _userService; private readonly IEntityService _entityService; private readonly IIpResolver _ipResolver; + private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor; private readonly GlobalSettings _globalSettings; - public AuditEventsComponent(IAuditService auditService, IUserService userService, IEntityService entityService, IIpResolver ipResolver, IOptions globalSettings) + public AuditEventsComponent( + IAuditService auditService, + IUserService userService, + IEntityService entityService, + IIpResolver ipResolver, + IOptions globalSettings, + IBackOfficeSecurityAccessor backOfficeSecurityAccessor) { _auditService = auditService; _userService = userService; _entityService = entityService; _ipResolver = ipResolver; + _backOfficeSecurityAccessor = backOfficeSecurityAccessor; _globalSettings = globalSettings.Value; } @@ -73,8 +81,8 @@ namespace Umbraco.Core.Compose { get { - var identity = Thread.CurrentPrincipal?.GetUmbracoIdentity(); - var user = identity == null ? null : _userService.GetUserById(Convert.ToInt32(identity.GetId())); + var identity = _backOfficeSecurityAccessor.BackOfficeSecurity.CurrentUser; + var user = identity == null ? null : _userService.GetUserById(Convert.ToInt32(identity.Id)); return user ?? UnknownUser(_globalSettings); } } diff --git a/src/Umbraco.Web.Common/Security/MemberManager.cs b/src/Umbraco.Web.Common/Security/MemberManager.cs index 386b1ba231..00bdcbd436 100644 --- a/src/Umbraco.Web.Common/Security/MemberManager.cs +++ b/src/Umbraco.Web.Common/Security/MemberManager.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Security.Claims; using System.Security.Principal; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; @@ -39,7 +40,7 @@ namespace Umbraco.Web.Common.Security private string GetCurrentUserId(IPrincipal currentUser) { - UmbracoBackOfficeIdentity umbIdentity = currentUser?.GetUmbracoIdentity(); + ClaimsIdentity umbIdentity = currentUser?.GetUmbracoIdentity(); var currentUserId = umbIdentity?.GetUserId() ?? Cms.Core.Constants.Security.SuperUserIdAsString; return currentUserId; } From bbaba0c54265dfafd36b3c166f1354a28ab7aa0c Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 23 Feb 2021 10:07:17 +0100 Subject: [PATCH 11/12] Add or update the TicketExpiresClaimType claim, to ensure its not added multiple times, now that is can be there after a clone. --- .../Extensions/ClaimsIdentityExtensions.cs | 17 +++++++++++++++++ .../ConfigureBackOfficeCookieOptions.cs | 5 ++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs index 62da5b0bf8..7569b64cb7 100644 --- a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs @@ -324,5 +324,22 @@ namespace Umbraco.Extensions /// Array of roles public static string[] GetRoles(this ClaimsIdentity identity) => identity .FindAll(x => x.Type == ClaimsIdentity.DefaultRoleClaimType).Select(role => role.Value).ToArray(); + + + /// + /// Adds or updates and existing claim. + /// + public static void AddOrUpdateClaim(this ClaimsIdentity identity, Claim claim) + { + if (identity == null) + { + throw new ArgumentNullException(nameof(identity)); + } + + Claim existingClaim = identity.Claims.FirstOrDefault(x => x.Type == claim.Type); + identity.TryRemoveClaim(existingClaim); + + identity.AddClaim(claim); + } } } diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 248fd72ac2..9e9549134e 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -15,7 +15,6 @@ using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Web; using Umbraco.Extensions; -using Constants = Umbraco.Cms.Core.Constants; namespace Umbraco.Cms.Web.BackOffice.Security { @@ -149,8 +148,8 @@ namespace Umbraco.Cms.Web.BackOffice.Security await securityStampValidator.ValidateAsync(ctx); EnsureTicketRenewalIfKeepUserLoggedIn(ctx); - // add a claim to track when the cookie expires, we use this to track time remaining - backOfficeIdentity.AddClaim(new Claim( + // add or update a claim to track when the cookie expires, we use this to track time remaining + backOfficeIdentity.AddOrUpdateClaim(new Claim( Constants.Security.TicketExpiresClaimType, ctx.Properties.ExpiresUtc.Value.ToString("o"), ClaimValueTypes.DateTime, From bc17b7463daee9ff834e9bc55b27299f154f766e Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 23 Feb 2021 10:52:55 +0100 Subject: [PATCH 12/12] Added test of AddOrUpdateClaim --- .../ClaimsPrincipalExtensionsTests.cs | 51 ++++++++++++++++--- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs index 0a941f9b59..f0ec8698ee 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs @@ -5,9 +5,8 @@ using System; using System.Linq; using System.Security.Claims; using NUnit.Framework; -using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core; using Umbraco.Extensions; -using Constants = Umbraco.Cms.Core.Constants; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Extensions { @@ -39,16 +38,52 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Extensions var expires = now.AddSeconds(expireSeconds).ToString("o"); backOfficeIdentity.AddClaim(new Claim( - Constants.Security.TicketExpiresClaimType, - expires, - ClaimValueTypes.DateTime, - Constants.Security.BackOfficeAuthenticationType, - Constants.Security.BackOfficeAuthenticationType, - backOfficeIdentity)); + Constants.Security.TicketExpiresClaimType, + expires, + ClaimValueTypes.DateTime, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + backOfficeIdentity)); var ticketRemainingSeconds = principal.GetRemainingAuthSeconds(then); Assert.AreEqual(remainingSeconds, ticketRemainingSeconds); } + + [Test] + public void AddOrUpdateClaim__Should_ensure_a_claim_is_not_added_twice() + { + var backOfficeIdentity = new ClaimsIdentity(); + backOfficeIdentity.AddRequiredClaims( + Constants.Security.SuperUserIdAsString, + "test", + "test", + Enumerable.Empty(), + Enumerable.Empty(), + "en-US", + Guid.NewGuid().ToString(), + Enumerable.Empty(), + Enumerable.Empty()); + + var expireSeconds = 99; + + DateTimeOffset now = DateTimeOffset.Now; + + var expires = now.AddSeconds(expireSeconds).ToString("o"); + + + var claim = new Claim( + Constants.Security.TicketExpiresClaimType, + expires, + ClaimValueTypes.DateTime, + Constants.Security.BackOfficeAuthenticationType, + Constants.Security.BackOfficeAuthenticationType, + backOfficeIdentity); + + backOfficeIdentity.AddOrUpdateClaim(claim); + backOfficeIdentity.AddOrUpdateClaim(claim); + + Assert.AreEqual(1, backOfficeIdentity.Claims.Count(x=>x.Type == Constants.Security.TicketExpiresClaimType)); + } } }