From 33a99df73f652c5ff86df974eea0f3a873e042e6 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Feb 2021 11:50:19 +0100 Subject: [PATCH] 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; }