diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeIdentityBuilder.cs b/src/Umbraco.Infrastructure/Security/BackOfficeIdentityBuilder.cs index 088afc7149..d19dfc3c05 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeIdentityBuilder.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeIdentityBuilder.cs @@ -29,19 +29,19 @@ namespace Umbraco.Cms.Core.Security { // We need to manually register some identity services here because we cannot rely on normal // AddIdentity calls for back office users - // For example: https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Extensions.Core/src/IdentityServiceCollectionExtensions.cs#L33 + // For example: https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Extensions.Core/src/IdentityServiceCollectionExtensions.cs#L33 // The reason we need our own is because the Identity system doesn't cater easily for multiple identity systems and particularly being // able to configure IdentityOptions to a specific provider since there is no named options. So we have strongly typed options // and strongly typed ILookupNormalizer and IdentityErrorDescriber since those are 'global' and we need to be unintrusive. // Services used by identity - services.TryAddScoped, UserValidator>(); - services.TryAddScoped, PasswordValidator>(); - services.TryAddScoped>( + services.AddScoped, UserValidator>(); + services.AddScoped, PasswordValidator>(); + services.AddScoped>( services => new BackOfficePasswordHasher( new LegacyPasswordSecurity(), services.GetRequiredService())); - services.TryAddScoped, UmbracoUserConfirmation>(); + services.AddScoped, UmbracoUserConfirmation>(); } // override to add itself, by default identity only wants a single IdentityErrorDescriber diff --git a/src/Umbraco.Infrastructure/Security/MemberPasswordHasher.cs b/src/Umbraco.Infrastructure/Security/MemberPasswordHasher.cs new file mode 100644 index 0000000000..6a0f2f21e2 --- /dev/null +++ b/src/Umbraco.Infrastructure/Security/MemberPasswordHasher.cs @@ -0,0 +1,62 @@ +using System; +using Microsoft.AspNetCore.Identity; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Security; + +namespace Umbraco.Cms.Core.Security +{ + /// + /// A password hasher for members + /// + /// + /// This will check for the ASP.NET Identity password hash flag before falling back to the legacy password hashing format ("HMACSHA256") + /// + public class MemberPasswordHasher : PasswordHasher + { + private readonly LegacyPasswordSecurity _legacyPasswordHasher; + + public MemberPasswordHasher(LegacyPasswordSecurity legacyPasswordHasher) => _legacyPasswordHasher = legacyPasswordHasher ?? throw new ArgumentNullException(nameof(legacyPasswordHasher)); + + /// + /// Verifies a user's hashed password + /// + /// + /// + /// + /// + /// Thrown when the correct hashing algorith cannot be determined + public override PasswordVerificationResult VerifyHashedPassword(MemberIdentityUser user, string hashedPassword, string providedPassword) + { + byte[] decodedHashedPassword = null; + bool isAspNetIdentityHash = false; + + try + { + decodedHashedPassword = Convert.FromBase64String(hashedPassword); + isAspNetIdentityHash = true; + } + catch (Exception) + { + // ignored - decoding throws + } + + // check for default ASP.NET Identity password hash flags + if (isAspNetIdentityHash) + { + if (decodedHashedPassword[0] == 0x00 || decodedHashedPassword[0] == 0x01) + { + return base.VerifyHashedPassword(user, hashedPassword, providedPassword); + } + + throw new InvalidOperationException("unable to determine member password hashing algorith"); + } + + var isValid = _legacyPasswordHasher.VerifyPassword( + Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName, + providedPassword, + hashedPassword); + + return isValid ? PasswordVerificationResult.SuccessRehashNeeded : PasswordVerificationResult.Failed; + } + } +} diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Security/MemberPasswordHasherTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Security/MemberPasswordHasherTests.cs new file mode 100644 index 0000000000..967b628a10 --- /dev/null +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Security/MemberPasswordHasherTests.cs @@ -0,0 +1,73 @@ +using System; +using Microsoft.AspNetCore.Identity; +using NUnit.Framework; +using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Infrastructure.Security; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Security +{ + [TestFixture] + public class MemberPasswordHasherTests + { + private MemberPasswordHasher CreateSut() => new MemberPasswordHasher(new LegacyPasswordSecurity()); + + [Test] + public void VerifyHashedPassword_GivenAnAspNetIdentity2PasswordHash_ThenExpectSuccessRehashNeeded() + { + const string password = "Password123!"; + const string hash = "AJszAsQqxOYbASKfL3JVUu6cjU18ouizXDfX4j7wLlir8SWj2yQaTepE9e5bIohIsQ=="; + + var sut = CreateSut(); + var result = sut.VerifyHashedPassword(null, hash, password); + + Assert.AreEqual(result, PasswordVerificationResult.SuccessRehashNeeded); + } + + [Test] + public void VerifyHashedPassword_GivenAnAspNetCoreIdentityPasswordHash_ThenExpectSuccess() + { + const string password = "Password123!"; + const string hash = "AQAAAAEAACcQAAAAEGF/tTVoL6ef3bQPZFYfbgKFu1CDQIAMgyY1N4EDt9jqdG/hsOX93X1U6LNvlIQ3mw=="; + + var sut = CreateSut(); + var result = sut.VerifyHashedPassword(null, hash, password); + + Assert.AreEqual(result, PasswordVerificationResult.Success); + } + + [Test] + public void VerifyHashedPassword_GivenALegacyPasswordHash_ThenExpectSuccessRehashNeeded() + { + const string password = "Password123!"; + const string hash = "yDiU2YyuYZU4jz6F0fpErQ==BxNRHkXBVyJs9gwWF6ktWdfDwYf5bwm+rvV7tOcNNx8="; + + var sut = CreateSut(); + var result = sut.VerifyHashedPassword(null, hash, password); + + Assert.AreEqual(result, PasswordVerificationResult.SuccessRehashNeeded); + } + + [Test] + public void VerifyHashedPassword_GivenAnUnknownBase64Hash_ThenExpectInvalidOperationException() + { + var hashBytes = new byte[] {3, 2, 1}; + var hash = Convert.ToBase64String(hashBytes); + + var sut = CreateSut(); + Assert.Throws(() => sut.VerifyHashedPassword(null, hash, "password")); + } + + [TestCase("AJszAsQqxOYbASKfL3JVUu6cjU18ouizXDfX4j7wLlir8SWj2yQaTepE9e5bIohIsQ==")] + [TestCase("AQAAAAEAACcQAAAAEGF/tTVoL6ef3bQPZFYfbgKFu1CDQIAMgyY1N4EDt9jqdG/hsOX93X1U6LNvlIQ3mw==")] + [TestCase("yDiU2YyuYZU4jz6F0fpErQ==BxNRHkXBVyJs9gwWF6ktWdfDwYf5bwm+rvV7tOcNNx8=")] + public void VerifyHashedPassword_GivenAnInvalidPassword_ThenExpectFailure(string hash) + { + const string invalidPassword = "nope"; + + var sut = CreateSut(); + var result = sut.VerifyHashedPassword(null, hash, invalidPassword); + + Assert.AreEqual(result, PasswordVerificationResult.Failed); + } + } +} diff --git a/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs b/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs index dfc2423d6d..4e95236b5f 100644 --- a/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs +++ b/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs @@ -33,6 +33,8 @@ namespace Umbraco.Extensions .AddClaimsPrincipalFactory() .AddErrorDescriber(); + services.TryAddSingleton(); + // Configure the options specifically for the UmbracoBackOfficeIdentityOptions instance services.ConfigureOptions(); services.ConfigureOptions(); diff --git a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilder.MembersIdentity.cs b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilder.MembersIdentity.cs index 7ac28b04c8..a05b6c84c3 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilder.MembersIdentity.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilder.MembersIdentity.cs @@ -25,6 +25,14 @@ namespace Umbraco.Extensions return builder; } + // NOTE: We are using AddIdentity which is going to add all of the default AuthN/AuthZ configurations = OK! + // This will also add all of the default identity services for our user/role types that we aren't overriding = OK! + // If a developer wishes to use Umbraco Members with different AuthN/AuthZ values, like different cookie values + // or authentication scheme's then they can call the default identity configuration methods like ConfigureApplicationCookie. + // BUT ... if a developer wishes to use the default auth schemes for entirely separate purposes alongside Umbraco members, + // then we'll probably have to change this and make it more flexible like how we do for Users. Which means booting up + // identity here with the basics and registering all of our own custom services. + // Since we are using the defaults in v8 (and below) for members, I think using the default for members now is OK! // TODO: We may need to use services.AddIdentityCore instead if this is doing too much services.AddIdentity() @@ -39,6 +47,8 @@ namespace Umbraco.Extensions services.ConfigureOptions(); + services.AddScoped, MemberPasswordHasher>(); + services.ConfigureApplicationCookie(x => { // TODO: We may want/need to configure these further diff --git a/src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs b/src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs index 6356979409..0b15378289 100644 --- a/src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs +++ b/src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs @@ -20,6 +20,7 @@ namespace Umbraco.Cms.Web.Common.Security { private readonly IHttpContextAccessor _httpContextAccessor; private readonly IEventAggregator _eventAggregator; + private readonly IBackOfficeUserPasswordChecker _backOfficeUserPasswordChecker; public BackOfficeUserManager( IIpResolver ipResolver, @@ -33,53 +34,43 @@ namespace Umbraco.Cms.Web.Common.Security IHttpContextAccessor httpContextAccessor, ILogger> logger, IOptions passwordConfiguration, - IEventAggregator eventAggregator) + IEventAggregator eventAggregator, + IBackOfficeUserPasswordChecker backOfficeUserPasswordChecker) : base(ipResolver, store, optionsAccessor, passwordHasher, userValidators, passwordValidators, errors, services, logger, passwordConfiguration) { _httpContextAccessor = httpContextAccessor; _eventAggregator = eventAggregator; + _backOfficeUserPasswordChecker = backOfficeUserPasswordChecker; } /// - /// Gets or sets the default back office user password checker + /// Override to allow checking the password via the if one is configured /// - public IBackOfficeUserPasswordChecker BackOfficeUserPasswordChecker { get; set; } // TODO: This isn't a good way to set this, it needs to be injected - - /// - /// - /// By default this uses the standard ASP.Net Identity approach which is: - /// * Get password store - /// * Call VerifyPasswordAsync with the password store + user + password - /// * Uses the PasswordHasher.VerifyHashedPassword to compare the stored password - /// - /// In some cases people want simple custom control over the username/password check, for simplicity - /// sake, developers would like the users to simply validate against an LDAP directory but the user - /// data remains stored inside of Umbraco. - /// See: http://issues.umbraco.org/issue/U4-7032 for the use cases. - /// - /// We've allowed this check to be overridden with a simple callback so that developers don't actually - /// have to implement/override this class. - /// - public override async Task CheckPasswordAsync(BackOfficeIdentityUser user, string password) + /// + /// + /// + /// + protected override async Task VerifyPasswordAsync( + IUserPasswordStore store, + BackOfficeIdentityUser user, + string password) { - if (BackOfficeUserPasswordChecker != null) + if (user.HasIdentity == false) { - BackOfficeUserPasswordCheckerResult result = await BackOfficeUserPasswordChecker.CheckPasswordAsync(user, password); - - if (user.HasIdentity == false) - { - return false; - } - - // if the result indicates to not fallback to the default, then return true if the credentials are valid - if (result != BackOfficeUserPasswordCheckerResult.FallbackToDefaultChecker) - { - return result == BackOfficeUserPasswordCheckerResult.ValidCredentials; - } + return PasswordVerificationResult.Failed; } - // use the default behavior - return await base.CheckPasswordAsync(user, password); + BackOfficeUserPasswordCheckerResult result = await _backOfficeUserPasswordChecker.CheckPasswordAsync(user, password); + + // if the result indicates to not fallback to the default, then return true if the credentials are valid + if (result != BackOfficeUserPasswordCheckerResult.FallbackToDefaultChecker) + { + return result == BackOfficeUserPasswordCheckerResult.ValidCredentials + ? PasswordVerificationResult.Success + : PasswordVerificationResult.Failed; + } + + return await base.VerifyPasswordAsync(store, user, password); } /// @@ -139,7 +130,7 @@ namespace Umbraco.Cms.Web.Common.Security return result; } - + /// public override async Task SetLockoutEndDateAsync(BackOfficeIdentityUser user, DateTimeOffset? lockoutEnd) { @@ -218,7 +209,7 @@ namespace Umbraco.Cms.Web.Common.Security (currentUserId, ip) => new UserLogoutSuccessNotification(ip, userId, currentUserId) ); - return new SignOutSuccessResult {SignOutRedirectUrl = notification.SignOutRedirectUrl}; + return new SignOutSuccessResult { SignOutRedirectUrl = notification.SignOutRedirectUrl }; } public void NotifyPasswordChanged(IPrincipal currentUser, string userId) => Notify(currentUser, diff --git a/src/Umbraco.Web.Common/Security/NoopBackOfficeUserPasswordChecker.cs b/src/Umbraco.Web.Common/Security/NoopBackOfficeUserPasswordChecker.cs new file mode 100644 index 0000000000..4c8e52be84 --- /dev/null +++ b/src/Umbraco.Web.Common/Security/NoopBackOfficeUserPasswordChecker.cs @@ -0,0 +1,11 @@ +using System.Threading.Tasks; +using Umbraco.Cms.Core.Security; + +namespace Umbraco.Cms.Web.Common.Security +{ + public class NoopBackOfficeUserPasswordChecker : IBackOfficeUserPasswordChecker + { + public Task CheckPasswordAsync(BackOfficeIdentityUser user, string password) + => Task.FromResult(BackOfficeUserPasswordCheckerResult.FallbackToDefaultChecker); + } +}