Member password roll forward

src/Umbraco.Infrastructure/Security/BackOfficeIdentityBuilder.cs

@@ -29,19 +29,19 @@ private void InitializeServices(IServiceCollection services)
         {
             // 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<IUserValidator<BackOfficeIdentityUser>, UserValidator<BackOfficeIdentityUser>>();
-            services.TryAddScoped<IPasswordValidator<BackOfficeIdentityUser>, PasswordValidator<BackOfficeIdentityUser>>();
-            services.TryAddScoped<IPasswordHasher<BackOfficeIdentityUser>>(
+            services.AddScoped<IUserValidator<BackOfficeIdentityUser>, UserValidator<BackOfficeIdentityUser>>();
+            services.AddScoped<IPasswordValidator<BackOfficeIdentityUser>, PasswordValidator<BackOfficeIdentityUser>>();
+            services.AddScoped<IPasswordHasher<BackOfficeIdentityUser>>(
                 services => new BackOfficePasswordHasher(
                     new LegacyPasswordSecurity(),
                     services.GetRequiredService<IJsonSerializer>()));
-            services.TryAddScoped<IUserConfirmation<BackOfficeIdentityUser>, UmbracoUserConfirmation<BackOfficeIdentityUser>>();
+            services.AddScoped<IUserConfirmation<BackOfficeIdentityUser>, UmbracoUserConfirmation<BackOfficeIdentityUser>>();
         }
 
         // override to add itself, by default identity only wants a single IdentityErrorDescriber

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
+{
+    /// <summary>
+    /// A password hasher for members
+    /// </summary>
+    /// <remarks>
+    /// This will check for the ASP.NET Identity password hash flag before falling back to the legacy password hashing format ("HMACSHA256")
+    /// </remarks>
+    public class MemberPasswordHasher : PasswordHasher<MemberIdentityUser>
+    {
+        private readonly LegacyPasswordSecurity _legacyPasswordHasher;
+
+        public MemberPasswordHasher(LegacyPasswordSecurity legacyPasswordHasher) => _legacyPasswordHasher = legacyPasswordHasher ?? throw new ArgumentNullException(nameof(legacyPasswordHasher));
+
+        /// <summary>
+        /// Verifies a user's hashed password
+        /// </summary>
+        /// <param name="user"></param>
+        /// <param name="hashedPassword"></param>
+        /// <param name="providedPassword"></param>
+        /// <returns></returns>
+        /// <exception cref="InvalidOperationException">Thrown when the correct hashing algorith cannot be determined</exception>
+        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;
+        }
+    }
+}

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<InvalidOperationException>(() => 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);
+        }
+    }
+}

src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs

@@ -33,6 +33,8 @@ public static IUmbracoBuilder AddBackOfficeIdentity(this IUmbracoBuilder builder
                 .AddClaimsPrincipalFactory<BackOfficeClaimsPrincipalFactory>()
                 .AddErrorDescriber<BackOfficeErrorDescriber>();
 
+            services.TryAddSingleton<IBackOfficeUserPasswordChecker, NoopBackOfficeUserPasswordChecker>();
+
             // Configure the options specifically for the UmbracoBackOfficeIdentityOptions instance
             services.ConfigureOptions<ConfigureBackOfficeIdentityOptions>();
             services.ConfigureOptions<ConfigureBackOfficeSecurityStampValidatorOptions>();

src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilder.MembersIdentity.cs

@@ -25,6 +25,14 @@ public static IUmbracoBuilder AddMembersIdentity(this IUmbracoBuilder builder)
                 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<MemberIdentityUser, UmbracoIdentityRole>()
@@ -39,6 +47,8 @@ public static IUmbracoBuilder AddMembersIdentity(this IUmbracoBuilder builder)
 
             services.ConfigureOptions<ConfigureMemberIdentityOptions>();
 
+            services.AddScoped<IPasswordHasher<MemberIdentityUser>, MemberPasswordHasher>();
+
             services.ConfigureApplicationCookie(x =>
             {
                 // TODO: We may want/need to configure these further

src/Umbraco.Web.Common/Security/BackOfficeUserManager.cs

@@ -20,6 +20,7 @@ public class BackOfficeUserManager : UmbracoUserManager<BackOfficeIdentityUser,
     {
         private readonly IHttpContextAccessor _httpContextAccessor;
         private readonly IEventAggregator _eventAggregator;
+        private readonly IBackOfficeUserPasswordChecker _backOfficeUserPasswordChecker;
 
         public BackOfficeUserManager(
             IIpResolver ipResolver,
@@ -33,53 +34,43 @@ public BackOfficeUserManager(
             IHttpContextAccessor httpContextAccessor,
             ILogger<UserManager<BackOfficeIdentityUser>> logger,
             IOptions<UserPasswordConfigurationSettings> passwordConfiguration,
-            IEventAggregator eventAggregator)
+            IEventAggregator eventAggregator,
+            IBackOfficeUserPasswordChecker backOfficeUserPasswordChecker)
             : base(ipResolver, store, optionsAccessor, passwordHasher, userValidators, passwordValidators, errors, services, logger, passwordConfiguration)
         {
             _httpContextAccessor = httpContextAccessor;
             _eventAggregator = eventAggregator;
+            _backOfficeUserPasswordChecker = backOfficeUserPasswordChecker;
         }
 
         /// <summary>
-        /// Gets or sets the default back office user password checker
+        /// Override to allow checking the password via the <see cref="IBackOfficeUserPasswordChecker"/> if one is configured
         /// </summary>
-        public IBackOfficeUserPasswordChecker BackOfficeUserPasswordChecker { get; set; } // TODO: This isn't a good way to set this, it needs to be injected
-
-        /// <inheritdoc />
-        /// <remarks>
-        /// 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.
-        /// </remarks>
-        public override async Task<bool> CheckPasswordAsync(BackOfficeIdentityUser user, string password)
+        /// <param name="store"></param>
+        /// <param name="user"></param>
+        /// <param name="password"></param>
+        /// <returns></returns>
+        protected override async Task<PasswordVerificationResult> VerifyPasswordAsync(
+            IUserPasswordStore<BackOfficeIdentityUser> store,
+            BackOfficeIdentityUser user,
+            string password)
         {
-            if (BackOfficeUserPasswordChecker != null)
+            if (user.HasIdentity == false)
+            {
+                return PasswordVerificationResult.Failed;
+            }
+
+            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)
             {
-                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 result == BackOfficeUserPasswordCheckerResult.ValidCredentials
+                    ? PasswordVerificationResult.Success
+                    : PasswordVerificationResult.Failed;
             }
 
-            // use the default behavior
-            return await base.CheckPasswordAsync(user, password);
+            return await base.VerifyPasswordAsync(store, user, password);
         }
 
         /// <summary>
@@ -139,7 +130,7 @@ public override async Task<IdentityResult> ChangePasswordAsync(BackOfficeIdentit
 
             return result;
         }
-        
+
         /// <inheritdoc/>
         public override async Task<IdentityResult> SetLockoutEndDateAsync(BackOfficeIdentityUser user, DateTimeOffset? lockoutEnd)
         {
@@ -218,7 +209,7 @@ public SignOutSuccessResult NotifyLogoutSuccess(IPrincipal currentUser, string u
                 (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,

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<BackOfficeUserPasswordCheckerResult> CheckPasswordAsync(BackOfficeIdentityUser user, string password)
+            => Task.FromResult(BackOfficeUserPasswordCheckerResult.FallbackToDefaultChecker);
+    }
+}