diff --git a/src/Umbraco.Infrastructure/Security/IUmbracoUserManager.cs b/src/Umbraco.Infrastructure/Security/IUmbracoUserManager.cs index 6acf52c3cb..ec364eb850 100644 --- a/src/Umbraco.Infrastructure/Security/IUmbracoUserManager.cs +++ b/src/Umbraco.Infrastructure/Security/IUmbracoUserManager.cs @@ -375,5 +375,13 @@ namespace Umbraco.Cms.Core.Security /// A user can only support a phone number if the BackOfficeUserStore is replaced with another that implements IUserPhoneNumberStore /// Task GetPhoneNumberAsync(TUser user); + + /// + /// Validates that a user's credentials are correct without actually logging them in. + /// + /// The user name. + /// The password. + /// True if the credentials are valid. + Task ValidateCredentialsAsync(string username, string password); } } diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 59f6686f68..fe12a349f8 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -1,9 +1,7 @@ using System; using System.Collections.Generic; -using System.ComponentModel; using System.Data; using System.Linq; -using System.Security.Claims; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Identity; @@ -17,13 +15,12 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.Security { - /// /// A custom user store that uses Umbraco member data /// public class MemberUserStore : UmbracoUserStore, IMemberUserStore { - private const string genericIdentityErrorCode = "IdentityErrorUserStore"; + private const string GenericIdentityErrorCode = "IdentityErrorUserStore"; private readonly IMemberService _memberService; private readonly IUmbracoMapper _mapper; private readonly IScopeProvider _scopeProvider; @@ -103,7 +100,7 @@ namespace Umbraco.Cms.Core.Security } catch (Exception ex) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message })); } } @@ -134,7 +131,7 @@ namespace Umbraco.Cms.Core.Security // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins)); - var memberChangeType = UpdateMemberProperties(found, user); + MemberDataChangeType memberChangeType = UpdateMemberProperties(found, user); if (memberChangeType == MemberDataChangeType.FullSave) { _memberService.Save(found); @@ -163,7 +160,7 @@ namespace Umbraco.Cms.Core.Security } catch (Exception ex) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message })); } } @@ -192,7 +189,7 @@ namespace Umbraco.Cms.Core.Security } catch (Exception ex) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message })); } } @@ -505,7 +502,7 @@ namespace Umbraco.Cms.Core.Security private MemberDataChangeType UpdateMemberProperties(IMember member, MemberIdentityUser identityUser) { - var changeType = MemberDataChangeType.None; + MemberDataChangeType changeType = MemberDataChangeType.None; // don't assign anything if nothing has changed as this will trigger the track changes of the model if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.LastLoginDateUtc)) diff --git a/src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs b/src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs index 7f77b9d8c6..bf198af1c3 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs @@ -75,11 +75,9 @@ namespace Umbraco.Cms.Core.Security /// True if the session is valid, else false public virtual async Task ValidateSessionIdAsync(string userId, string sessionId) { - var userSessionStore = Store as IUserSessionStore; - // if this is not set, for backwards compat (which would be super rare), we'll just approve it // TODO: This should be removed after members supports this - if (userSessionStore == null) + if (Store is not IUserSessionStore userSessionStore) { return true; } @@ -221,8 +219,7 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(user)); } - var lockoutStore = Store as IUserLockoutStore; - if (lockoutStore == null) + if (Store is not IUserLockoutStore lockoutStore) { throw new NotSupportedException("The current user store does not implement " + typeof(IUserLockoutStore<>)); } @@ -241,5 +238,23 @@ namespace Umbraco.Cms.Core.Security return result; } + /// + public async Task ValidateCredentialsAsync(string username, string password) + { + TUser user = await FindByNameAsync(username); + if (user == null) + { + return false; + } + + if (Store is not IUserPasswordStore userPasswordStore) + { + throw new NotSupportedException("The current user store does not implement " + typeof(IUserPasswordStore<>)); + } + + var hash = await userPasswordStore.GetPasswordHashAsync(user, new CancellationToken()); + + return await VerifyPasswordAsync(userPasswordStore, user, password) == PasswordVerificationResult.Success; + } } } diff --git a/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs b/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs index 1db823cc78..3884ee31a1 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs @@ -10,7 +10,8 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.Security { - public abstract class UmbracoUserStore : UserStoreBase, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> + public abstract class UmbracoUserStore + : UserStoreBase, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> where TUser : UmbracoIdentityUser where TRole : IdentityRole { diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs index a4aaa9311c..c8f90050e2 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; @@ -9,6 +8,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Moq; using NUnit.Framework; +using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Mapping; using Umbraco.Cms.Core.Models; @@ -35,11 +35,21 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public MemberManager CreateSut() { - var scopeProvider = new Mock().Object; + IScopeProvider scopeProvider = new Mock().Object; _mockMemberService = new Mock(); + + var mapDefinitions = new List() + { + new IdentityMapDefinition( + Mock.Of(), + Mock.Of(), + Options.Create(new GlobalSettings()), + AppCaches.Disabled), + }; + _fakeMemberStore = new MemberUserStore( _mockMemberService.Object, - new UmbracoMapper(new MapDefinitionCollection(new List()), scopeProvider), + new UmbracoMapper(new MapDefinitionCollection(mapDefinitions), scopeProvider), scopeProvider, new IdentityErrorDescriber(), Mock.Of()); @@ -131,25 +141,11 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { //arrange MemberManager sut = CreateSut(); - var fakeUser = new MemberIdentityUser(777) - { - UserName = "testUser", - Email = "test@test.com", - Name = "Test", - MemberTypeAlias = "Anything", - PasswordConfig = "testConfig" - }; + MemberIdentityUser fakeUser = CreateValidUser(); - var builder = new MemberTypeBuilder(); - MemberType memberType = builder.BuildSimpleMemberType(); + IMember fakeMember = CreateMember(fakeUser); - IMember fakeMember = new Member(memberType) - { - Id = 777 - }; - - _mockMemberService.Setup(x => x.CreateMember(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())).Returns(fakeMember); - _mockMemberService.Setup(x => x.Save(fakeMember, false)); + MockMemberServiceForCreateMember(fakeMember); //act IdentityResult identityResult = await sut.CreateAsync(fakeUser); @@ -158,5 +154,99 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Assert.IsTrue(identityResult.Succeeded); Assert.IsTrue(!identityResult.Errors.Any()); } + + [Test] + public async Task GivenAUserExists_AndTheCorrectCredentialsAreProvided_ThenACheckOfCredentialsShouldSucceed() + { + //arrange + var password = "password"; + MemberManager sut = CreateSut(); + + MemberIdentityUser fakeUser = CreateValidUser(); + + IMember fakeMember = CreateMember(fakeUser); + + MockMemberServiceForCreateMember(fakeMember); + + _mockMemberService.Setup(x => x.GetByUsername(It.Is(y => y == fakeUser.UserName))).Returns(fakeMember); + + _mockPasswordHasher.Setup(x => x.VerifyHashedPassword(It.IsAny(), It.IsAny(), It.IsAny())).Returns(PasswordVerificationResult.Success); + + //act + await sut.CreateAsync(fakeUser); + var result = await sut.ValidateCredentialsAsync(fakeUser.UserName, password); + + //assert + Assert.IsTrue(result); + } + + [Test] + public async Task GivenAUserExists_AndIncorrectCredentialsAreProvided_ThenACheckOfCredentialsShouldFail() + { + //arrange + var password = "password"; + MemberManager sut = CreateSut(); + + MemberIdentityUser fakeUser = CreateValidUser(); + + IMember fakeMember = CreateMember(fakeUser); + + MockMemberServiceForCreateMember(fakeMember); + + _mockMemberService.Setup(x => x.GetByUsername(It.Is(y => y == fakeUser.UserName))).Returns(fakeMember); + + _mockPasswordHasher.Setup(x => x.VerifyHashedPassword(It.IsAny(), It.IsAny(), It.IsAny())).Returns(PasswordVerificationResult.Failed); + + //act + await sut.CreateAsync(fakeUser); + var result = await sut.ValidateCredentialsAsync(fakeUser.UserName, password); + + //assert + Assert.IsFalse(result); + } + + [Test] + public async Task GivenAUserDoesExists_AndCredentialsAreProvided_ThenACheckOfCredentialsShouldFail() + { + //arrange + var password = "password"; + MemberManager sut = CreateSut(); + + _mockMemberService.Setup(x => x.GetByUsername(It.Is(y => y == "testUser"))).Returns((IMember)null); + + //act + var result = await sut.ValidateCredentialsAsync("testUser", password); + + //assert + Assert.IsFalse(result); + } + + private static MemberIdentityUser CreateValidUser() => + new MemberIdentityUser(777) + { + UserName = "testUser", + Email = "test@test.com", + Name = "Test", + MemberTypeAlias = "Anything", + PasswordConfig = "testConfig", + PasswordHash = "hashedPassword" + }; + + private static IMember CreateMember(MemberIdentityUser fakeUser) + { + var builder = new MemberTypeBuilder(); + MemberType memberType = builder.BuildSimpleMemberType(); + return new Member(memberType) + { + Id = 777, + Username = fakeUser.UserName, + }; + } + + private void MockMemberServiceForCreateMember(IMember fakeMember) + { + _mockMemberService.Setup(x => x.CreateMember(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())).Returns(fakeMember); + _mockMemberService.Setup(x => x.Save(fakeMember, false)); + } } } diff --git a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs index ac45c932da..9ea0728ce5 100644 --- a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs +++ b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs @@ -1,3 +1,4 @@ +using System.Security.Claims; using Microsoft.AspNetCore.Http; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Models.Membership; @@ -23,8 +24,6 @@ namespace Umbraco.Cms.Web.Common.Security _httpContextAccessor = httpContextAccessor; } - - /// public IUser CurrentUser { @@ -39,7 +38,7 @@ namespace Umbraco.Cms.Web.Common.Security //Check again if (_currentUser == null) { - var id = GetUserId(); + Attempt id = GetUserId(); _currentUser = id ? _userService.GetUserById(id.Result) : null; } } @@ -52,22 +51,18 @@ namespace Umbraco.Cms.Web.Common.Security /// public Attempt GetUserId() { - var identity = _httpContextAccessor.HttpContext?.GetCurrentIdentity(); + ClaimsIdentity identity = _httpContextAccessor.HttpContext?.GetCurrentIdentity(); return identity == null ? Attempt.Fail() : Attempt.Succeed(identity.GetId()); } /// public bool IsAuthenticated() { - var httpContext = _httpContextAccessor.HttpContext; + HttpContext httpContext = _httpContextAccessor.HttpContext; return httpContext?.User != null && httpContext.User.Identity.IsAuthenticated && httpContext.GetCurrentIdentity() != null; } /// - public bool UserHasSectionAccess(string section, IUser user) - { - return user.HasSectionAccess(section); - } - + public bool UserHasSectionAccess(string section, IUser user) => user.HasSectionAccess(section); } } diff --git a/src/Umbraco.Web.Common/Security/MemberManager.cs b/src/Umbraco.Web.Common/Security/MemberManager.cs index 9bc8b284c7..f7d0c8e43e 100644 --- a/src/Umbraco.Web.Common/Security/MemberManager.cs +++ b/src/Umbraco.Web.Common/Security/MemberManager.cs @@ -1,18 +1,17 @@ using System; -using System.Linq; using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; -using Umbraco.Extensions; using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Net; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; -using System.Threading.Tasks; -using Umbraco.Cms.Core.PublishedCache; -using Umbraco.Cms.Core.Models.PublishedContent; +using Umbraco.Extensions; namespace Umbraco.Cms.Web.Common.Security {