From c9229880da8d322e5931982dc828071b517184b9 Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Wed, 3 Mar 2021 17:33:57 +0000 Subject: [PATCH 01/25] Renamed and added initial member role store methods, and initial unit tests --- .../Security/MemberRolesUserStore.cs | 57 ----- .../Security/MembersRoleStore.cs | 193 +++++++++++++++ .../Security/MemberIdentityUserStoreTests.cs | 3 - .../Security/MemberRoleStoreTests.cs | 222 ++++++++++++++++++ 4 files changed, 415 insertions(+), 60 deletions(-) delete mode 100644 src/Umbraco.Infrastructure/Security/MemberRolesUserStore.cs create mode 100644 src/Umbraco.Infrastructure/Security/MembersRoleStore.cs create mode 100644 src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs diff --git a/src/Umbraco.Infrastructure/Security/MemberRolesUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberRolesUserStore.cs deleted file mode 100644 index 65b3cf1ef9..0000000000 --- a/src/Umbraco.Infrastructure/Security/MemberRolesUserStore.cs +++ /dev/null @@ -1,57 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Security.Claims; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Identity; -using Umbraco.Cms.Core.Scoping; -using Umbraco.Cms.Core.Services; - -namespace Umbraco.Cms.Core.Security -{ - /// - /// A custom user store that uses Umbraco member data - /// - public class MemberRolesUserStore : RoleStoreBase, string, IdentityUserRole, IdentityRoleClaim> - { - private readonly IMemberService _memberService; - private readonly IMemberGroupService _memberGroupService; - private readonly IScopeProvider _scopeProvider; - - public MemberRolesUserStore(IMemberService memberService, IMemberGroupService memberGroupService, IScopeProvider scopeProvider, IdentityErrorDescriber describer) - : base(describer) - { - _memberService = memberService ?? throw new ArgumentNullException(nameof(memberService)); - _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); - _scopeProvider = scopeProvider ?? throw new ArgumentNullException(nameof(scopeProvider)); - } - - /// - public override IQueryable> Roles { get; } - - /// - public override Task CreateAsync(IdentityRole role, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task UpdateAsync(IdentityRole role, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task DeleteAsync(IdentityRole role, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task> FindByIdAsync(string id, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task> FindByNameAsync(string normalizedName, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task> GetClaimsAsync(IdentityRole role, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task AddClaimAsync(IdentityRole role, Claim claim, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task RemoveClaimAsync(IdentityRole role, Claim claim, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - } -} diff --git a/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs b/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs new file mode 100644 index 0000000000..3f238c5c48 --- /dev/null +++ b/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs @@ -0,0 +1,193 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Linq; +using System.Security.Claims; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Identity; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Core.Services; + +namespace Umbraco.Cms.Core.Security +{ + /// + /// A custom user store that uses Umbraco member data + /// + public class MembersRoleStore : RoleStoreBase, string, IdentityUserRole, IdentityRoleClaim> + { + private readonly IMemberService _memberService; + private readonly IMemberGroupService _memberGroupService; + private readonly IScopeProvider _scopeProvider; + + public MembersRoleStore(IMemberService memberService, IMemberGroupService memberGroupService, IScopeProvider scopeProvider, IdentityErrorDescriber describer) + : base(describer) + { + _memberService = memberService ?? throw new ArgumentNullException(nameof(memberService)); + _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); + _scopeProvider = scopeProvider ?? throw new ArgumentNullException(nameof(scopeProvider)); + } + + /// + public override IQueryable> Roles + { + get + { + IEnumerable memberGroups = _memberGroupService.GetAll(); + var identityRoles = new List>(); + foreach (IMemberGroup group in memberGroups) + { + IdentityRole identityRole = MapFromMemberGroup(group); + identityRoles.Add(identityRole); + } + + return identityRoles.AsQueryable(); + } + } + + /// + public override Task CreateAsync( + IdentityRole role, + CancellationToken cancellationToken = new CancellationToken()) + { + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + var memberGroup = new MemberGroup + { + Name = role.Name + }; + + _memberGroupService.Save(memberGroup); + + role.Id = memberGroup.Id.ToString(); + + return Task.FromResult(IdentityResult.Success); + } + + + /// + public override Task UpdateAsync(IdentityRole role, + CancellationToken cancellationToken = new CancellationToken()) + { + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + if (!int.TryParse(role.Id, out int roleId)) + { + return new Task(() => IdentityResult.Failed()); + } + + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + if (MapToMemberGroup(role, memberGroup)) + { + _memberGroupService.Save(memberGroup); + } + } + + return Task.FromResult(IdentityResult.Success); + } + + /// + public override Task DeleteAsync(IdentityRole role, + CancellationToken cancellationToken = new CancellationToken()) + { + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + if (!int.TryParse(role.Id, out int roleId)) + { + return new Task(() => IdentityResult.Failed()); + } + + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + _memberGroupService.Delete(memberGroup); + } + else + { + //TODO: throw exception when not found, or return failure? + return Task.FromResult(IdentityResult.Failed()); + } + + return Task.FromResult(IdentityResult.Success); + } + + /// + public override Task> FindByIdAsync(string id, + CancellationToken cancellationToken = new CancellationToken()) + { + if (!int.TryParse(id, out int roleId)) + { + return null; + } + + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + + return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); + } + + /// + public override Task> FindByNameAsync(string normalizedName, + CancellationToken cancellationToken = new CancellationToken()) + { + IMemberGroup memberGroup = _memberGroupService.GetByName(normalizedName); + + return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); + } + + /// + public override Task> GetClaimsAsync(IdentityRole role, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); + + /// + public override Task AddClaimAsync(IdentityRole role, Claim claim, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); + + /// + public override Task RemoveClaimAsync(IdentityRole role, Claim claim, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); + + /// + /// Maps a member group to an identity role + /// + /// + /// + private IdentityRole MapFromMemberGroup(IMemberGroup memberGroup) + { + var result = new IdentityRole + { + Id = memberGroup.Id.ToString(), + Name = memberGroup.Name + }; + + return result; + } + + /// + /// Map an identity role to a member group + /// + /// + /// + /// + private bool MapToMemberGroup(IdentityRole role, IMemberGroup memberGroup) + { + var anythingChanged = false; + + if (!string.IsNullOrEmpty(role.Name) && memberGroup.Name != role.Name) + { + memberGroup.Name = role.Name; + anythingChanged = true; + } + + return anythingChanged; + } + } +} diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs index 1a4f05d984..8afe7fe198 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs @@ -74,8 +74,5 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Assert.IsTrue(identityResult.Succeeded); Assert.IsTrue(!identityResult.Errors.Any()); } - - //GetPasswordHashAsync - //GetUserIdAsync } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs new file mode 100644 index 0000000000..d9337f6f10 --- /dev/null +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -0,0 +1,222 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Identity; +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core.Services; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security +{ + [TestFixture] + public class MemberRoleStoreTests + { + private Mock _mockMemberService; + private Mock _mockMemberGroupService; + + public MembersRoleStore CreateSut() + { + _mockMemberService = new Mock(); + _mockMemberGroupService = new Mock(); + return new MembersRoleStore( + _mockMemberService.Object, + _mockMemberGroupService.Object, + new Mock().Object, + new IdentityErrorDescriber()); + } + + [Test] + public void GivenICreateAMemberRole_AndTheGroupIsNull_ThenIShouldGetAFailedResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + CancellationToken fakeCancellationToken = new CancellationToken() { }; + + // act + Action actual = () => sut.CreateAsync(null, fakeCancellationToken); + + // assert + Assert.That(actual, Throws.ArgumentNullException); + } + + + [Test] + public async Task GivenICreateAMemberRole_AndTheGroupIsPopulatedCorrectly_ThenIShouldGetASuccessResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "777", + Name = "testname" + }; + var fakeCancellationToken = new CancellationToken() { }; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && m.CreatorId == 77); + + bool raiseEvents = false; + + _mockMemberGroupService.Setup(x => x.Save(mockMemberGroup, raiseEvents)); + + // act + IdentityResult identityResult = await sut.CreateAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded); + Assert.IsTrue(!identityResult.Errors.Any()); + _mockMemberGroupService.Verify(x => x.Save(It.IsAny(), It.IsAny())); + } + + [Test] + public async Task GivenIUpdateAMemberRole_AndTheGroupExists_ThenIShouldGetASuccessResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "777", + Name = "testname" + }; + var fakeCancellationToken = new CancellationToken() { }; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && m.CreatorId == 77); + + bool raiseEvents = false; + + _mockMemberGroupService.Setup(x => x.GetById(777)).Returns(mockMemberGroup); + _mockMemberGroupService.Setup(x => x.Save(mockMemberGroup, raiseEvents)); + + // act + IdentityResult identityResult = await sut.UpdateAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded); + Assert.IsTrue(!identityResult.Errors.Any()); + _mockMemberGroupService.Verify(x => x.Save(mockMemberGroup, false)); + _mockMemberGroupService.Verify(x => x.GetById(777)); + } + + [Test] + public async Task GivenIUpdateAMemberRole_AndTheGroupDoesntExist_ThenIShouldGetAFailureResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "777", + Name = "testname" + }; + var fakeCancellationToken = new CancellationToken() { }; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && m.CreatorId == 77); + + bool raiseEvents = false; + + + // act + IdentityResult identityResult = await sut.UpdateAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded == false); + Assert.IsTrue(identityResult.Errors.Any()); + _mockMemberGroupService.Verify(x => x.GetById(777)); + } + + [Test] + public async Task GivenIDeleteAMemberRole_AndItExists_ThenTheMemberGroupShouldBeDeleted_AndIShouldGetASuccessResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "777", + Name = "testname" + }; + var fakeCancellationToken = new CancellationToken() { }; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && m.CreatorId == 77); + + _mockMemberGroupService.Setup(x => x.GetById(777)).Returns(mockMemberGroup); + + // act + IdentityResult identityResult = await sut.DeleteAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded); + Assert.IsTrue(!identityResult.Errors.Any()); + _mockMemberGroupService.Verify(x => x.GetById(777)); + _mockMemberGroupService.Verify(x => x.Delete(mockMemberGroup)); + } + + [Test] + public async Task GivenIDeleteAMemberRole_AndItDoesntExist_ThenTheMemberGroupShouldBeDeleted_AndIShouldGetASuccessResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "777", + Name = "testname" + }; + var fakeCancellationToken = new CancellationToken() { }; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && m.CreatorId == 77); + + + // act + IdentityResult identityResult = await sut.DeleteAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded == false); + Assert.IsTrue(identityResult.Errors.Any()); + _mockMemberGroupService.Verify(x => x.GetById(777)); + _mockMemberGroupService.Verify(x => x.Delete(mockMemberGroup)); + } + + [Test] + public async Task GivenIGetAllMemberRoles_ThenIShouldGetAllMemberGroups_AndASuccessResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "777", + Name = "testname" + }; + IEnumerable> expected = new List>() + { + new IdentityRole("fakeGroupName") + { + Id = "77" + } + }; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && m.CreatorId == 77); + + IEnumerable fakeMemberGroups = new List() + { + mockMemberGroup + }; + + _mockMemberGroupService.Setup(x => x.GetAll()).Returns(fakeMemberGroups); + + // act + IQueryable> actual = sut.Roles; + + // assert + Assert.AreEqual(expected.AsQueryable(), actual); + _mockMemberGroupService.Verify(x => x.GetAll()); + } + } +} From 4cb9923c24bd75f004c38305e8a254e63a6d69c3 Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Thu, 4 Mar 2021 13:46:12 +0000 Subject: [PATCH 02/25] Updated errors and unit tests --- .../Security/MembersRoleStore.cs | 15 ++- .../Security/MemberRoleStoreTests.cs | 117 ++++++++++++++---- 2 files changed, 107 insertions(+), 25 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs b/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs index 3f238c5c48..3aafa702cd 100644 --- a/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs @@ -80,7 +80,8 @@ namespace Umbraco.Cms.Core.Security if (!int.TryParse(role.Id, out int roleId)) { - return new Task(() => IdentityResult.Failed()); + //TODO: what identity error should we return in this case? + return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); } IMemberGroup memberGroup = _memberGroupService.GetById(roleId); @@ -91,6 +92,11 @@ namespace Umbraco.Cms.Core.Security _memberGroupService.Save(memberGroup); } } + else + { + //TODO: throw exception when not found, or return failure? And is this the correcet message + return Task.FromResult(IdentityResult.Failed(ErrorDescriber.InvalidRoleName(role.Name))); + } return Task.FromResult(IdentityResult.Success); } @@ -106,7 +112,8 @@ namespace Umbraco.Cms.Core.Security if (!int.TryParse(role.Id, out int roleId)) { - return new Task(() => IdentityResult.Failed()); + //TODO: what identity error should we return in this case? + return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); } IMemberGroup memberGroup = _memberGroupService.GetById(roleId); @@ -116,8 +123,8 @@ namespace Umbraco.Cms.Core.Security } else { - //TODO: throw exception when not found, or return failure? - return Task.FromResult(IdentityResult.Failed()); + //TODO: throw exception when not found, or return failure? And is this the correcet message + return Task.FromResult(IdentityResult.Failed(ErrorDescriber.InvalidRoleName(role.Name))); } return Task.FromResult(IdentityResult.Success); diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index d9337f6f10..6c3c741376 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -18,6 +18,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { private Mock _mockMemberService; private Mock _mockMemberGroupService; + private IdentityErrorDescriber ErrorDescriber => new IdentityErrorDescriber(); public MembersRoleStore CreateSut() { @@ -27,7 +28,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security _mockMemberService.Object, _mockMemberGroupService.Object, new Mock().Object, - new IdentityErrorDescriber()); + ErrorDescriber); } [Test] @@ -74,19 +75,19 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security } [Test] - public async Task GivenIUpdateAMemberRole_AndTheGroupExists_ThenIShouldGetASuccessResultAsync() + public async Task GivenIUpdateAMemberRole_AndTheGroupExistsWithTheSameName_ThenIShouldGetASuccessResultAsyncButNoUpdatesMade() { // arrange MembersRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "777", - Name = "testname" + Name = "fakeGroupName" }; var fakeCancellationToken = new CancellationToken() { }; IMemberGroup mockMemberGroup = Mock.Of(m => - m.Name == "fakeGroupName" && m.CreatorId == 77); + m.Name == "fakeGroupName" && m.CreatorId == 777); bool raiseEvents = false; @@ -99,7 +100,36 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded); Assert.IsTrue(!identityResult.Errors.Any()); - _mockMemberGroupService.Verify(x => x.Save(mockMemberGroup, false)); + _mockMemberGroupService.Verify(x => x.GetById(777)); + } + + [Test] + public async Task GivenIUpdateAMemberRole_AndTheGroupExistsWithADifferentSameName_ThenIShouldGetASuccessResultAsyncWithUpdatesMade() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "777", + Name = "fakeGroup777" + }; + var fakeCancellationToken = new CancellationToken() { }; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && m.CreatorId == 777); + + bool raiseEvents = false; + + _mockMemberGroupService.Setup(x => x.GetById(777)).Returns(mockMemberGroup); + _mockMemberGroupService.Setup(x => x.Save(mockMemberGroup, raiseEvents)); + + // act + IdentityResult identityResult = await sut.UpdateAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded); + Assert.IsTrue(!identityResult.Errors.Any()); + _mockMemberGroupService.Verify(x => x.Save(It.IsAny(), It.IsAny())); _mockMemberGroupService.Verify(x => x.GetById(777)); } @@ -115,8 +145,29 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security }; var fakeCancellationToken = new CancellationToken() { }; - IMemberGroup mockMemberGroup = Mock.Of(m => - m.Name == "fakeGroupName" && m.CreatorId == 77); + bool raiseEvents = false; + + + // act + IdentityResult identityResult = await sut.UpdateAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded == false); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); + _mockMemberGroupService.Verify(x => x.GetById(777)); + } + + [Test] + public async Task GivenIUpdateAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "7a77", + Name = "testname" + }; + var fakeCancellationToken = new CancellationToken() { }; bool raiseEvents = false; @@ -126,8 +177,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any()); - _mockMemberGroupService.Verify(x => x.GetById(777)); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "DefaultError" && x.Description == "An unknown failure has occurred.")); } [Test] @@ -158,7 +208,32 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security } [Test] - public async Task GivenIDeleteAMemberRole_AndItDoesntExist_ThenTheMemberGroupShouldBeDeleted_AndIShouldGetASuccessResultAsync() + public async Task GivenIDeleteAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAFailResultAsync() + { + // arrange + MembersRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole() + { + Id = "7a77", + Name = "testname" + }; + var fakeCancellationToken = new CancellationToken() { }; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && m.CreatorId == 77); + + + // act + IdentityResult identityResult = await sut.DeleteAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded == false); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "DefaultError" && x.Description == "An unknown failure has occurred.")); + } + + + [Test] + public async Task GivenIDeleteAMemberRole_AndItDoesntExist_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAFailResultAsync() { // arrange MembersRoleStore sut = CreateSut(); @@ -178,9 +253,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any()); + Assert.IsTrue(identityResult.Errors.Any(x=>x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); _mockMemberGroupService.Verify(x => x.GetById(777)); - _mockMemberGroupService.Verify(x => x.Delete(mockMemberGroup)); } [Test] @@ -188,21 +262,17 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { // arrange MembersRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + var fakeRole = new IdentityRole("fakeGroupName") { - Id = "777", - Name = "testname" + Id = "777" }; IEnumerable> expected = new List>() { - new IdentityRole("fakeGroupName") - { - Id = "77" - } + fakeRole }; IMemberGroup mockMemberGroup = Mock.Of(m => - m.Name == "fakeGroupName" && m.CreatorId == 77); + m.Name == "fakeGroupName" && m.CreatorId == 123 && m.Id == 777); IEnumerable fakeMemberGroups = new List() { @@ -215,7 +285,12 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security IQueryable> actual = sut.Roles; // assert - Assert.AreEqual(expected.AsQueryable(), actual); + Assert.AreEqual(expected.AsQueryable().First().Id, actual.First().Id); + Assert.AreEqual(expected.AsQueryable().First().Name, actual.First().Name); + //Always null: + //Assert.AreEqual(expected.AsQueryable().First().NormalizedName, actual.First().NormalizedName); + //Always different: + //Assert.AreEqual(expected.AsQueryable().First().ConcurrencyStamp, actual.First().ConcurrencyStamp); _mockMemberGroupService.Verify(x => x.GetAll()); } } From e124ee336aef7c2146ec528286e03df63df630b2 Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Thu, 4 Mar 2021 17:29:11 +0000 Subject: [PATCH 03/25] Deleted MembersRoleProvider. Switched to role store. Made everything single not plural. Fixed formatting. --- .../Mapping/MemberTabsAndPropertiesMapper.cs | 1 + .../Security/IMemberManager.cs | 2 +- .../Security/IdentityMapDefinition.cs | 6 +- ...ityBuilder.cs => MemberIdentityBuilder.cs} | 10 +- ...ityOptions.cs => MemberIdentityOptions.cs} | 2 +- ...sIdentityUser.cs => MemberIdentityUser.cs} | 12 +- ...MembersRoleStore.cs => MemberRoleStore.cs} | 21 ++-- ...MembersUserStore.cs => MemberUserStore.cs} | 106 +++++++++--------- ...MembersServiceCollectionExtensionsTests.cs | 4 +- .../MemberIdentityUserManagerTests.cs | 42 +++---- .../Security/MemberIdentityUserStoreTests.cs | 10 +- .../Security/MemberRoleStoreTests.cs | 28 ++--- .../Controllers/MemberControllerUnitTests.cs | 40 +++---- .../Controllers/ContentController.cs | 1 + .../Controllers/MemberController.cs | 10 +- .../Controllers/MemberGroupController.cs | 41 ++++--- .../UmbracoBuilderExtensions.cs | 2 +- .../Trees/MemberGroupTreeController.cs | 2 + .../ServiceCollectionExtensions.cs | 12 +- .../Extensions/IdentityBuilderExtensions.cs | 4 +- .../Security/MemberManager.cs | 14 +-- .../Security/Providers/MembersRoleProvider.cs | 106 ------------------ src/Umbraco.Web/Umbraco.Web.csproj | 3 +- 23 files changed, 187 insertions(+), 292 deletions(-) rename src/Umbraco.Infrastructure/Security/{MembersIdentityBuilder.cs => MemberIdentityBuilder.cs} (71%) rename src/Umbraco.Infrastructure/Security/{MembersIdentityOptions.cs => MemberIdentityOptions.cs} (78%) rename src/Umbraco.Infrastructure/Security/{MembersIdentityUser.cs => MemberIdentityUser.cs} (90%) rename src/Umbraco.Infrastructure/Security/{MembersRoleStore.cs => MemberRoleStore.cs} (87%) rename src/Umbraco.Infrastructure/Security/{MembersUserStore.cs => MemberUserStore.cs} (81%) delete mode 100644 src/Umbraco.Web/Security/Providers/MembersRoleProvider.cs diff --git a/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs b/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs index 92aab36bd4..9ac1b05d0f 100644 --- a/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs +++ b/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs @@ -237,6 +237,7 @@ namespace Umbraco.Cms.Core.Models.Mapping var userRoles = username.IsNullOrWhiteSpace() ? null : _memberService.GetAllRoles(username); // create a dictionary of all roles (except internal roles) + "false" + //TODO: use MembersRoleStore var result = _memberGroupService.GetAll() .Select(x => x.Name) // if a role starts with __umbracoRole we won't show it as it's an internal role used for public access diff --git a/src/Umbraco.Infrastructure/Security/IMemberManager.cs b/src/Umbraco.Infrastructure/Security/IMemberManager.cs index 85b4c0c300..b310e9434f 100644 --- a/src/Umbraco.Infrastructure/Security/IMemberManager.cs +++ b/src/Umbraco.Infrastructure/Security/IMemberManager.cs @@ -3,7 +3,7 @@ namespace Umbraco.Cms.Core.Security /// /// The user manager for members /// - public interface IMemberManager : IUmbracoUserManager + public interface IMemberManager : IUmbracoUserManager { } } diff --git a/src/Umbraco.Infrastructure/Security/IdentityMapDefinition.cs b/src/Umbraco.Infrastructure/Security/IdentityMapDefinition.cs index 1a76dec2d5..50c4d1e505 100644 --- a/src/Umbraco.Infrastructure/Security/IdentityMapDefinition.cs +++ b/src/Umbraco.Infrastructure/Security/IdentityMapDefinition.cs @@ -42,10 +42,10 @@ namespace Umbraco.Cms.Core.Security target.EnableChangeTracking(); }); - mapper.Define( + mapper.Define( (source, context) => { - var target = new MembersIdentityUser(source.Id); + var target = new MemberIdentityUser(source.Id); target.DisableChangeTracking(); return target; }, @@ -94,7 +94,7 @@ namespace Umbraco.Cms.Core.Security //target.Roles =; } - private void Map(IMember source, MembersIdentityUser target) + private void Map(IMember source, MemberIdentityUser target) { target.Email = source.Email; target.UserName = source.Username; diff --git a/src/Umbraco.Infrastructure/Security/MembersIdentityBuilder.cs b/src/Umbraco.Infrastructure/Security/MemberIdentityBuilder.cs similarity index 71% rename from src/Umbraco.Infrastructure/Security/MembersIdentityBuilder.cs rename to src/Umbraco.Infrastructure/Security/MemberIdentityBuilder.cs index 726b999b89..4e2e4a39b1 100644 --- a/src/Umbraco.Infrastructure/Security/MembersIdentityBuilder.cs +++ b/src/Umbraco.Infrastructure/Security/MemberIdentityBuilder.cs @@ -5,18 +5,18 @@ using Microsoft.Extensions.DependencyInjection; namespace Umbraco.Cms.Core.Security { - public class MembersIdentityBuilder : IdentityBuilder + public class MemberIdentityBuilder : IdentityBuilder { - public MembersIdentityBuilder(IServiceCollection services) : base(typeof(MembersIdentityUser), services) + public MemberIdentityBuilder(IServiceCollection services) : base(typeof(MemberIdentityUser), services) { } - public MembersIdentityBuilder(Type role, IServiceCollection services) : base(typeof(MembersIdentityUser), role, services) + public MemberIdentityBuilder(Type role, IServiceCollection services) : base(typeof(MemberIdentityUser), role, services) { } /// - /// Adds a token provider for the . + /// Adds a token provider for the . /// /// The name of the provider to add. /// The type of the to add. @@ -27,7 +27,7 @@ namespace Umbraco.Cms.Core.Security { throw new InvalidOperationException($"Invalid Type for TokenProvider: {provider.FullName}"); } - Services.Configure(options => + Services.Configure(options => { options.Tokens.ProviderMap[providerName] = new TokenProviderDescriptor(provider); }); diff --git a/src/Umbraco.Infrastructure/Security/MembersIdentityOptions.cs b/src/Umbraco.Infrastructure/Security/MemberIdentityOptions.cs similarity index 78% rename from src/Umbraco.Infrastructure/Security/MembersIdentityOptions.cs rename to src/Umbraco.Infrastructure/Security/MemberIdentityOptions.cs index 8f993a1f76..4e05797a04 100644 --- a/src/Umbraco.Infrastructure/Security/MembersIdentityOptions.cs +++ b/src/Umbraco.Infrastructure/Security/MemberIdentityOptions.cs @@ -5,7 +5,7 @@ namespace Umbraco.Cms.Core.Security /// /// Identity options specifically for the Umbraco members identity implementation /// - public class MembersIdentityOptions : IdentityOptions + public class MemberIdentityOptions : IdentityOptions { } } diff --git a/src/Umbraco.Infrastructure/Security/MembersIdentityUser.cs b/src/Umbraco.Infrastructure/Security/MemberIdentityUser.cs similarity index 90% rename from src/Umbraco.Infrastructure/Security/MembersIdentityUser.cs rename to src/Umbraco.Infrastructure/Security/MemberIdentityUser.cs index 6e3473c3ce..539234ac65 100644 --- a/src/Umbraco.Infrastructure/Security/MembersIdentityUser.cs +++ b/src/Umbraco.Infrastructure/Security/MemberIdentityUser.cs @@ -11,7 +11,7 @@ namespace Umbraco.Cms.Core.Security /// /// The identity user used for the member /// - public class MembersIdentityUser : UmbracoIdentityUser + public class MemberIdentityUser : UmbracoIdentityUser { private string _name; private string _passwordConfig; @@ -23,29 +23,29 @@ namespace Umbraco.Cms.Core.Security groups => groups.GetHashCode()); /// - /// Initializes a new instance of the class. + /// Initializes a new instance of the class. /// - public MembersIdentityUser(int userId) + public MemberIdentityUser(int userId) { // use the property setters - they do more than just setting a field Id = UserIdToString(userId); } - public MembersIdentityUser() + public MemberIdentityUser() { } /// /// Used to construct a new instance without an identity /// - public static MembersIdentityUser CreateNew(string username, string email, string memberTypeAlias, string name = null) + public static MemberIdentityUser CreateNew(string username, string email, string memberTypeAlias, string name = null) { if (string.IsNullOrWhiteSpace(username)) { throw new ArgumentException("Value cannot be null or whitespace.", nameof(username)); } - var user = new MembersIdentityUser(); + var user = new MemberIdentityUser(); user.DisableChangeTracking(); user.UserName = username; user.Email = email; diff --git a/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs similarity index 87% rename from src/Umbraco.Infrastructure/Security/MembersRoleStore.cs rename to src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index 3aafa702cd..399da9aec0 100644 --- a/src/Umbraco.Infrastructure/Security/MembersRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -1,5 +1,4 @@ using System; -using System.Collections; using System.Collections.Generic; using System.Linq; using System.Security.Claims; @@ -7,7 +6,6 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Identity; using Umbraco.Cms.Core.Models; -using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Services; namespace Umbraco.Cms.Core.Security @@ -15,19 +13,12 @@ namespace Umbraco.Cms.Core.Security /// /// A custom user store that uses Umbraco member data /// - public class MembersRoleStore : RoleStoreBase, string, IdentityUserRole, IdentityRoleClaim> + public class MemberRoleStore : RoleStoreBase, string, IdentityUserRole, IdentityRoleClaim> { - private readonly IMemberService _memberService; private readonly IMemberGroupService _memberGroupService; - private readonly IScopeProvider _scopeProvider; - public MembersRoleStore(IMemberService memberService, IMemberGroupService memberGroupService, IScopeProvider scopeProvider, IdentityErrorDescriber describer) - : base(describer) - { - _memberService = memberService ?? throw new ArgumentNullException(nameof(memberService)); - _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); - _scopeProvider = scopeProvider ?? throw new ArgumentNullException(nameof(scopeProvider)); - } + public MemberRoleStore(IMemberGroupService memberGroupService, IdentityErrorDescriber describer) + : base(describer) => _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); /// public override IQueryable> Roles @@ -94,7 +85,7 @@ namespace Umbraco.Cms.Core.Security } else { - //TODO: throw exception when not found, or return failure? And is this the correcet message + //TODO: throw exception when not found, or return failure? And is this the correct message return Task.FromResult(IdentityResult.Failed(ErrorDescriber.InvalidRoleName(role.Name))); } @@ -123,7 +114,7 @@ namespace Umbraco.Cms.Core.Security } else { - //TODO: throw exception when not found, or return failure? And is this the correcet message + //TODO: throw exception when not found, or return failure? And is this the correct message return Task.FromResult(IdentityResult.Failed(ErrorDescriber.InvalidRoleName(role.Name))); } @@ -153,6 +144,8 @@ namespace Umbraco.Cms.Core.Security return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); } + ///TODO: are we implementing these claims methods? + /// public override Task> GetClaimsAsync(IdentityRole role, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); diff --git a/src/Umbraco.Infrastructure/Security/MembersUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs similarity index 81% rename from src/Umbraco.Infrastructure/Security/MembersUserStore.cs rename to src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 9405992ba8..17a45764ad 100644 --- a/src/Umbraco.Infrastructure/Security/MembersUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -19,20 +19,20 @@ namespace Umbraco.Cms.Core.Security /// /// A custom user store that uses Umbraco member data /// - public class MembersUserStore : UserStoreBase, string, IdentityUserClaim, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> + public class MemberUserStore : UserStoreBase, string, IdentityUserClaim, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> { private readonly IMemberService _memberService; private readonly UmbracoMapper _mapper; private readonly IScopeProvider _scopeProvider; /// - /// Initializes a new instance of the class for the members identity store + /// Initializes a new instance of the class for the members identity store /// /// The member service /// The mapper for properties /// The scope provider /// The error describer - public MembersUserStore(IMemberService memberService, UmbracoMapper mapper, IScopeProvider scopeProvider, IdentityErrorDescriber describer) + public MemberUserStore(IMemberService memberService, UmbracoMapper mapper, IScopeProvider scopeProvider, IdentityErrorDescriber describer) : base(describer) { _memberService = memberService ?? throw new ArgumentNullException(nameof(memberService)); @@ -45,16 +45,16 @@ namespace Umbraco.Cms.Core.Security /// /// [EditorBrowsable(EditorBrowsableState.Never)] - public override IQueryable Users => throw new NotImplementedException(); + public override IQueryable Users => throw new NotImplementedException(); /// - public override Task GetNormalizedUserNameAsync(MembersIdentityUser user, CancellationToken cancellationToken) => GetUserNameAsync(user, cancellationToken); + public override Task GetNormalizedUserNameAsync(MemberIdentityUser user, CancellationToken cancellationToken) => GetUserNameAsync(user, cancellationToken); /// - public override Task SetNormalizedUserNameAsync(MembersIdentityUser user, string normalizedName, CancellationToken cancellationToken) => SetUserNameAsync(user, normalizedName, cancellationToken); + public override Task SetNormalizedUserNameAsync(MemberIdentityUser user, string normalizedName, CancellationToken cancellationToken) => SetUserNameAsync(user, normalizedName, cancellationToken); /// - public override Task CreateAsync(MembersIdentityUser user, CancellationToken cancellationToken = default) + public override Task CreateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -100,7 +100,7 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task UpdateAsync(MembersIdentityUser user, CancellationToken cancellationToken = default) + public override Task UpdateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -121,7 +121,7 @@ namespace Umbraco.Cms.Core.Security if (found != null) { // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. - var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MembersIdentityUser.Logins)); + var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins)); if (UpdateMemberProperties(found, user)) { @@ -148,7 +148,7 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task DeleteAsync(MembersIdentityUser user, CancellationToken cancellationToken = default) + public override Task DeleteAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -170,10 +170,10 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task FindByIdAsync(string userId, CancellationToken cancellationToken = default) => FindUserAsync(userId, cancellationToken); + public override Task FindByIdAsync(string userId, CancellationToken cancellationToken = default) => FindUserAsync(userId, cancellationToken); /// - protected override Task FindUserAsync(string userId, CancellationToken cancellationToken) + protected override Task FindUserAsync(string userId, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -181,30 +181,30 @@ namespace Umbraco.Cms.Core.Security IMember user = _memberService.GetById(UserIdToInt(userId)); if (user == null) { - return Task.FromResult((MembersIdentityUser)null); + return Task.FromResult((MemberIdentityUser)null); } - return Task.FromResult(AssignLoginsCallback(_mapper.Map(user))); + return Task.FromResult(AssignLoginsCallback(_mapper.Map(user))); } /// - public override Task FindByNameAsync(string userName, CancellationToken cancellationToken = default) + public override Task FindByNameAsync(string userName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); IMember user = _memberService.GetByUsername(userName); if (user == null) { - return Task.FromResult((MembersIdentityUser)null); + return Task.FromResult((MemberIdentityUser)null); } - MembersIdentityUser result = AssignLoginsCallback(_mapper.Map(user)); + MemberIdentityUser result = AssignLoginsCallback(_mapper.Map(user)); return Task.FromResult(result); } /// - public override async Task SetPasswordHashAsync(MembersIdentityUser user, string passwordHash, CancellationToken cancellationToken = default) + public override async Task SetPasswordHashAsync(MemberIdentityUser user, string passwordHash, CancellationToken cancellationToken = default) { await base.SetPasswordHashAsync(user, passwordHash, cancellationToken); @@ -213,7 +213,7 @@ namespace Umbraco.Cms.Core.Security } /// - public override async Task HasPasswordAsync(MembersIdentityUser user, CancellationToken cancellationToken = default) + public override async Task HasPasswordAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { // This checks if it's null var result = await base.HasPasswordAsync(user, cancellationToken); @@ -227,28 +227,28 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task FindByEmailAsync(string email, CancellationToken cancellationToken = default) + public override Task FindByEmailAsync(string email, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); IMember member = _memberService.GetByEmail(email); - MembersIdentityUser result = member == null + MemberIdentityUser result = member == null ? null - : _mapper.Map(member); + : _mapper.Map(member); return Task.FromResult(AssignLoginsCallback(result)); } /// - public override Task GetNormalizedEmailAsync(MembersIdentityUser user, CancellationToken cancellationToken) + public override Task GetNormalizedEmailAsync(MemberIdentityUser user, CancellationToken cancellationToken) => GetEmailAsync(user, cancellationToken); /// - public override Task SetNormalizedEmailAsync(MembersIdentityUser user, string normalizedEmail, CancellationToken cancellationToken) + public override Task SetNormalizedEmailAsync(MemberIdentityUser user, string normalizedEmail, CancellationToken cancellationToken) => SetEmailAsync(user, normalizedEmail, cancellationToken); /// - public override Task AddLoginAsync(MembersIdentityUser user, UserLoginInfo login, CancellationToken cancellationToken = default) + public override Task AddLoginAsync(MemberIdentityUser user, UserLoginInfo login, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -271,7 +271,7 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task RemoveLoginAsync(MembersIdentityUser user, string loginProvider, string providerKey, CancellationToken cancellationToken = default) + public override Task RemoveLoginAsync(MemberIdentityUser user, string loginProvider, string providerKey, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -290,7 +290,7 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task> GetLoginsAsync(MembersIdentityUser user, CancellationToken cancellationToken = default) + public override Task> GetLoginsAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -308,7 +308,7 @@ namespace Umbraco.Cms.Core.Security cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - MembersIdentityUser user = await FindUserAsync(userId, cancellationToken); + MemberIdentityUser user = await FindUserAsync(userId, cancellationToken); if (user == null) { return null; @@ -356,7 +356,7 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task AddToRoleAsync(MembersIdentityUser user, string role, CancellationToken cancellationToken = default) + public override Task AddToRoleAsync(MemberIdentityUser user, string role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -387,7 +387,7 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task RemoveFromRoleAsync(MembersIdentityUser user, string role, CancellationToken cancellationToken = default) + public override Task RemoveFromRoleAsync(MemberIdentityUser user, string role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); @@ -420,7 +420,7 @@ namespace Umbraco.Cms.Core.Security /// /// Gets a list of role names the specified user belongs to. /// - public override Task> GetRolesAsync(MembersIdentityUser user, CancellationToken cancellationToken = default) + public override Task> GetRolesAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -443,7 +443,7 @@ namespace Umbraco.Cms.Core.Security /// /// Returns true if a user is in the role /// - public override Task IsInRoleAsync(MembersIdentityUser user, string normalizedRoleName, CancellationToken cancellationToken = default) + public override Task IsInRoleAsync(MemberIdentityUser user, string normalizedRoleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -458,7 +458,7 @@ namespace Umbraco.Cms.Core.Security /// /// Lists all users of a given role. /// - public override Task> GetUsersInRoleAsync(string normalizedRoleName, CancellationToken cancellationToken = default) + public override Task> GetUsersInRoleAsync(string normalizedRoleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -469,7 +469,7 @@ namespace Umbraco.Cms.Core.Security IEnumerable members = _memberService.GetMembersByMemberType(normalizedRoleName); - IList membersIdentityUsers = members.Select(x => _mapper.Map(x)).ToList(); + IList membersIdentityUsers = members.Select(x => _mapper.Map(x)).ToList(); return Task.FromResult(membersIdentityUsers); } @@ -493,7 +493,7 @@ namespace Umbraco.Cms.Core.Security /// protected override async Task> FindUserRoleAsync(string userId, string roleId, CancellationToken cancellationToken) { - MembersIdentityUser user = await FindUserAsync(userId, cancellationToken); + MemberIdentityUser user = await FindUserAsync(userId, cancellationToken); if (user == null) { return null; @@ -504,7 +504,7 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task GetSecurityStampAsync(MembersIdentityUser user, CancellationToken cancellationToken = default) + public override Task GetSecurityStampAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -519,7 +519,7 @@ namespace Umbraco.Cms.Core.Security : user.SecurityStamp); } - private MembersIdentityUser AssignLoginsCallback(MembersIdentityUser user) + private MemberIdentityUser AssignLoginsCallback(MemberIdentityUser user) { if (user != null) { @@ -530,12 +530,12 @@ namespace Umbraco.Cms.Core.Security return user; } - private bool UpdateMemberProperties(IMember member, MembersIdentityUser identityUserMember) + private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityUserMember) { var anythingChanged = false; // don't assign anything if nothing has changed as this will trigger the track changes of the model - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.LastLoginDateUtc)) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.LastLoginDateUtc)) || (member.LastLoginDate != default && identityUserMember.LastLoginDateUtc.HasValue == false) || (identityUserMember.LastLoginDateUtc.HasValue && member.LastLoginDate.ToUniversalTime() != identityUserMember.LastLoginDateUtc.Value)) { @@ -546,7 +546,7 @@ namespace Umbraco.Cms.Core.Security member.LastLoginDate = dt; } - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.LastPasswordChangeDateUtc)) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.LastPasswordChangeDateUtc)) || (member.LastPasswordChangeDate != default && identityUserMember.LastPasswordChangeDateUtc.HasValue == false) || (identityUserMember.LastPasswordChangeDateUtc.HasValue && member.LastPasswordChangeDate.ToUniversalTime() != identityUserMember.LastPasswordChangeDateUtc.Value)) { @@ -554,7 +554,7 @@ namespace Umbraco.Cms.Core.Security member.LastPasswordChangeDate = identityUserMember.LastPasswordChangeDateUtc.Value.ToLocalTime(); } - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.EmailConfirmed)) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.EmailConfirmed)) || (member.EmailConfirmedDate.HasValue && member.EmailConfirmedDate.Value != default && identityUserMember.EmailConfirmed == false) || ((member.EmailConfirmedDate.HasValue == false || member.EmailConfirmedDate.Value == default) && identityUserMember.EmailConfirmed)) { @@ -562,21 +562,21 @@ namespace Umbraco.Cms.Core.Security member.EmailConfirmedDate = identityUserMember.EmailConfirmed ? (DateTime?)DateTime.Now : null; } - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.Name)) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.Name)) && member.Name != identityUserMember.Name && identityUserMember.Name.IsNullOrWhiteSpace() == false) { anythingChanged = true; member.Name = identityUserMember.Name; } - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.Email)) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.Email)) && member.Email != identityUserMember.Email && identityUserMember.Email.IsNullOrWhiteSpace() == false) { anythingChanged = true; member.Email = identityUserMember.Email; } - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.AccessFailedCount)) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.AccessFailedCount)) && member.FailedPasswordAttempts != identityUserMember.AccessFailedCount) { anythingChanged = true; @@ -595,14 +595,14 @@ namespace Umbraco.Cms.Core.Security } } - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.UserName)) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.UserName)) && member.Username != identityUserMember.UserName && identityUserMember.UserName.IsNullOrWhiteSpace() == false) { anythingChanged = true; member.Username = identityUserMember.UserName; } - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.PasswordHash)) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.PasswordHash)) && member.RawPasswordValue != identityUserMember.PasswordHash && identityUserMember.PasswordHash.IsNullOrWhiteSpace() == false) { anythingChanged = true; @@ -617,7 +617,7 @@ namespace Umbraco.Cms.Core.Security } // TODO: Fix this for Groups too (as per backoffice comment) - if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.Roles)) || identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.Groups))) + if (identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.Roles)) || identityUserMember.IsPropertyDirty(nameof(MemberIdentityUser.Groups))) { } @@ -645,42 +645,42 @@ namespace Umbraco.Cms.Core.Security /// /// [EditorBrowsable(EditorBrowsableState.Never)] - public override Task> GetClaimsAsync(MembersIdentityUser user, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + public override Task> GetClaimsAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) => throw new NotImplementedException(); /// /// Not supported in Umbraco /// /// [EditorBrowsable(EditorBrowsableState.Never)] - public override Task AddClaimsAsync(MembersIdentityUser user, IEnumerable claims, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + public override Task AddClaimsAsync(MemberIdentityUser user, IEnumerable claims, CancellationToken cancellationToken = default) => throw new NotImplementedException(); /// /// Not supported in Umbraco /// /// [EditorBrowsable(EditorBrowsableState.Never)] - public override Task ReplaceClaimAsync(MembersIdentityUser user, Claim claim, Claim newClaim, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + public override Task ReplaceClaimAsync(MemberIdentityUser user, Claim claim, Claim newClaim, CancellationToken cancellationToken = default) => throw new NotImplementedException(); /// /// Not supported in Umbraco /// /// [EditorBrowsable(EditorBrowsableState.Never)] - public override Task RemoveClaimsAsync(MembersIdentityUser user, IEnumerable claims, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + public override Task RemoveClaimsAsync(MemberIdentityUser user, IEnumerable claims, CancellationToken cancellationToken = default) => throw new NotImplementedException(); /// /// Not supported in Umbraco /// /// [EditorBrowsable(EditorBrowsableState.Never)] - public override Task> GetUsersForClaimAsync(Claim claim, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + public override Task> GetUsersForClaimAsync(Claim claim, CancellationToken cancellationToken = default) => throw new NotImplementedException(); /// /// Not supported in Umbraco /// /// [EditorBrowsable(EditorBrowsableState.Never)] - protected override Task> FindTokenAsync(MembersIdentityUser user, string loginProvider, string name, CancellationToken cancellationToken) => throw new NotImplementedException(); + protected override Task> FindTokenAsync(MemberIdentityUser user, string loginProvider, string name, CancellationToken cancellationToken) => throw new NotImplementedException(); /// /// Not supported in Umbraco diff --git a/src/Umbraco.Tests.Integration/Umbraco.Web.Common/MembersServiceCollectionExtensionsTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Web.Common/MembersServiceCollectionExtensionsTests.cs index c426e26750..e76716c152 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Web.Common/MembersServiceCollectionExtensionsTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Web.Common/MembersServiceCollectionExtensionsTests.cs @@ -16,10 +16,10 @@ namespace Umbraco.Tests.Integration.Umbraco.Web.Common [Test] public void AddMembersIdentity_ExpectMembersUserStoreResolvable() { - IUserStore userStore = Services.GetService>(); + IUserStore userStore = Services.GetService>(); Assert.IsNotNull(userStore); - Assert.AreEqual(typeof(MembersUserStore), userStore.GetType()); + Assert.AreEqual(typeof(MemberUserStore), userStore.GetType()); } [Test] diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserManagerTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserManagerTests.cs index b88189a08b..a319d33b34 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserManagerTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserManagerTests.cs @@ -19,36 +19,36 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security [TestFixture] public class MemberIdentityUserManagerTests { - private Mock> _mockMemberStore; - private Mock> _mockIdentityOptions; - private Mock> _mockPasswordHasher; - private Mock> _mockUserValidators; - private Mock>> _mockPasswordValidators; + private Mock> _mockMemberStore; + private Mock> _mockIdentityOptions; + private Mock> _mockPasswordHasher; + private Mock> _mockUserValidators; + private Mock>> _mockPasswordValidators; private Mock _mockNormalizer; private IdentityErrorDescriber _mockErrorDescriber; private Mock _mockServiceProviders; - private Mock>> _mockLogger; + private Mock>> _mockLogger; private Mock> _mockPasswordConfiguration; public MemberManager CreateSut() { - _mockMemberStore = new Mock>(); - _mockIdentityOptions = new Mock>(); + _mockMemberStore = new Mock>(); + _mockIdentityOptions = new Mock>(); - var idOptions = new MembersIdentityOptions { Lockout = { AllowedForNewUsers = false } }; + var idOptions = new MemberIdentityOptions { Lockout = { AllowedForNewUsers = false } }; _mockIdentityOptions.Setup(o => o.Value).Returns(idOptions); - _mockPasswordHasher = new Mock>(); + _mockPasswordHasher = new Mock>(); - var userValidators = new List>(); - _mockUserValidators = new Mock>(); - var validator = new Mock>(); + var userValidators = new List>(); + _mockUserValidators = new Mock>(); + var validator = new Mock>(); userValidators.Add(validator.Object); - _mockPasswordValidators = new Mock>>(); + _mockPasswordValidators = new Mock>>(); _mockNormalizer = new Mock(); _mockErrorDescriber = new IdentityErrorDescriber(); _mockServiceProviders = new Mock(); - _mockLogger = new Mock>>(); + _mockLogger = new Mock>>(); _mockPasswordConfiguration = new Mock>(); _mockPasswordConfiguration.Setup(x => x.Value).Returns(() => new MemberPasswordConfigurationSettings() @@ -56,9 +56,9 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security }); - var pwdValidators = new List> + var pwdValidators = new List> { - new PasswordValidator() + new PasswordValidator() }; var userManager = new MemberManager( @@ -71,12 +71,12 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security new BackOfficeIdentityErrorDescriber(), _mockServiceProviders.Object, new Mock().Object, - new Mock>>().Object, + new Mock>>().Object, _mockPasswordConfiguration.Object); validator.Setup(v => v.ValidateAsync( userManager, - It.IsAny())) + It.IsAny())) .Returns(Task.FromResult(IdentityResult.Success)).Verifiable(); return userManager; @@ -87,7 +87,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { //arrange MemberManager sut = CreateSut(); - MembersIdentityUser fakeUser = new MembersIdentityUser() + MemberIdentityUser fakeUser = new MemberIdentityUser() { PasswordConfig = "testConfig" }; @@ -148,7 +148,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { //arrange MemberManager sut = CreateSut(); - MembersIdentityUser fakeUser = new MembersIdentityUser() + MemberIdentityUser fakeUser = new MemberIdentityUser() { PasswordConfig = "testConfig" }; diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs index 8afe7fe198..8a0bf149b7 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs @@ -20,10 +20,10 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { private Mock _mockMemberService; - public MembersUserStore CreateSut() + public MemberUserStore CreateSut() { _mockMemberService = new Mock(); - return new MembersUserStore( + return new MemberUserStore( _mockMemberService.Object, new UmbracoMapper(new MapDefinitionCollection(new List())), new Mock().Object, @@ -34,7 +34,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public void GivenICreateUser_AndTheUserIsNull_ThenIShouldGetAFailedResultAsync() { // arrange - MembersUserStore sut = CreateSut(); + MemberUserStore sut = CreateSut(); CancellationToken fakeCancellationToken = new CancellationToken(){}; // act @@ -49,8 +49,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenICreateANewUser_AndTheUserIsPopulatedCorrectly_ThenIShouldGetASuccessResultAsync() { // arrange - MembersUserStore sut = CreateSut(); - var fakeUser = new MembersIdentityUser() { }; + MemberUserStore sut = CreateSut(); + var fakeUser = new MemberIdentityUser() { }; var fakeCancellationToken = new CancellationToken() { }; IMemberType fakeMemberType = new MemberType(new MockShortStringHelper(), 77); diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index 6c3c741376..f74b6cef94 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -16,18 +16,14 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security [TestFixture] public class MemberRoleStoreTests { - private Mock _mockMemberService; private Mock _mockMemberGroupService; private IdentityErrorDescriber ErrorDescriber => new IdentityErrorDescriber(); - public MembersRoleStore CreateSut() + public MemberRoleStore CreateSut() { - _mockMemberService = new Mock(); _mockMemberGroupService = new Mock(); - return new MembersRoleStore( - _mockMemberService.Object, + return new MemberRoleStore( _mockMemberGroupService.Object, - new Mock().Object, ErrorDescriber); } @@ -35,7 +31,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public void GivenICreateAMemberRole_AndTheGroupIsNull_ThenIShouldGetAFailedResultAsync() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); CancellationToken fakeCancellationToken = new CancellationToken() { }; // act @@ -50,7 +46,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenICreateAMemberRole_AndTheGroupIsPopulatedCorrectly_ThenIShouldGetASuccessResultAsync() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "777", @@ -78,7 +74,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupExistsWithTheSameName_ThenIShouldGetASuccessResultAsyncButNoUpdatesMade() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "777", @@ -107,7 +103,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupExistsWithADifferentSameName_ThenIShouldGetASuccessResultAsyncWithUpdatesMade() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "777", @@ -137,7 +133,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupDoesntExist_ThenIShouldGetAFailureResultAsync() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "777", @@ -161,7 +157,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "7a77", @@ -184,7 +180,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIDeleteAMemberRole_AndItExists_ThenTheMemberGroupShouldBeDeleted_AndIShouldGetASuccessResultAsync() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "777", @@ -211,7 +207,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIDeleteAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAFailResultAsync() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "7a77", @@ -236,7 +232,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIDeleteAMemberRole_AndItDoesntExist_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAFailResultAsync() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole() { Id = "777", @@ -261,7 +257,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIGetAllMemberRoles_ThenIShouldGetAllMemberGroups_AndASuccessResultAsync() { // arrange - MembersRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole("fakeGroupName") { Id = "777" diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs index 5bb4613912..52722f75d1 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs @@ -68,7 +68,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers IMemberGroupService memberGroupService, IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, - IPasswordChanger passwordChanger, + IPasswordChanger passwordChanger, IOptions globalSettings, IUser user) { @@ -78,7 +78,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers sut.ModelState.AddModelError("key", "Invalid model state"); Mock.Get(umbracoMembersUserManager) - .Setup(x => x.CreateAsync(It.IsAny(), It.IsAny())) + .Setup(x => x.CreateAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(umbracoMembersUserManager) .Setup(x => x.ValidatePasswordAsync(It.IsAny())) @@ -107,14 +107,14 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IBackOfficeSecurity backOfficeSecurity, - IPasswordChanger passwordChanger, + IPasswordChanger passwordChanger, IOptions globalSettings, IUser user) { // arrange Member member = SetupMemberTestData(out MemberSave fakeMemberData, out MemberDisplay memberDisplay, ContentSaveAction.SaveNew); Mock.Get(umbracoMembersUserManager) - .Setup(x => x.CreateAsync(It.IsAny(), It.IsAny())) + .Setup(x => x.CreateAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(umbracoMembersUserManager) .Setup(x => x.ValidatePasswordAsync(It.IsAny())) @@ -148,14 +148,14 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IBackOfficeSecurity backOfficeSecurity, - IPasswordChanger passwordChanger, + IPasswordChanger passwordChanger, IOptions globalSettings, IUser user) { // arrange Member member = SetupMemberTestData(out MemberSave fakeMemberData, out MemberDisplay memberDisplay, ContentSaveAction.SaveNew); Mock.Get(umbracoMembersUserManager) - .Setup(x => x.CreateAsync(It.IsAny(), It.IsAny())) + .Setup(x => x.CreateAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(umbracoMembersUserManager) .Setup(x => x.ValidatePasswordAsync(It.IsAny())) @@ -190,13 +190,13 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IBackOfficeSecurity backOfficeSecurity, - IPasswordChanger passwordChanger, + IPasswordChanger passwordChanger, IOptions globalSettings, IUser user) { // arrange Member member = SetupMemberTestData(out MemberSave fakeMemberData, out MemberDisplay memberDisplay, ContentSaveAction.Save); - var membersIdentityUser = new MembersIdentityUser(123); + var membersIdentityUser = new MemberIdentityUser(123); Mock.Get(umbracoMembersUserManager) .Setup(x => x.FindByIdAsync(It.IsAny())) .ReturnsAsync(() => membersIdentityUser); @@ -206,7 +206,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers string password = "fakepassword9aw89rnyco3938cyr^%&*()i8Y"; Mock.Get(umbracoMembersUserManager) - .Setup(x => x.UpdateAsync(It.IsAny())) + .Setup(x => x.UpdateAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(memberTypeService).Setup(x => x.GetDefault()).Returns("fakeAlias"); Mock.Get(globalSettings); @@ -242,13 +242,13 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IBackOfficeSecurity backOfficeSecurity, - IPasswordChanger passwordChanger, + IPasswordChanger passwordChanger, IOptions globalSettings, IUser user) { // arrange Member member = SetupMemberTestData(out MemberSave fakeMemberData, out MemberDisplay memberDisplay, ContentSaveAction.Save); - var membersIdentityUser = new MembersIdentityUser(123); + var membersIdentityUser = new MemberIdentityUser(123); Mock.Get(umbracoMembersUserManager) .Setup(x => x.FindByIdAsync(It.IsAny())) .ReturnsAsync(() => membersIdentityUser); @@ -257,7 +257,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers .ReturnsAsync(() => IdentityResult.Success); Mock.Get(umbracoMembersUserManager) - .Setup(x => x.UpdateAsync(It.IsAny())) + .Setup(x => x.UpdateAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(memberTypeService).Setup(x => x.GetDefault()).Returns("fakeAlias"); Mock.Get(globalSettings); @@ -289,7 +289,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers Mock.Get(backOfficeSecurity).Setup(x => x.CurrentUser).Returns(user); } - private static void SetupPasswordSuccess(IMemberManager umbracoMembersUserManager, IPasswordChanger passwordChanger, bool successful = true) + private static void SetupPasswordSuccess(IMemberManager umbracoMembersUserManager, IPasswordChanger passwordChanger, bool successful = true) { var passwordChanged = new PasswordChangedModel() { @@ -322,14 +322,14 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IBackOfficeSecurity backOfficeSecurity, - IPasswordChanger passwordChanger, + IPasswordChanger passwordChanger, IOptions globalSettings, IUser user) { // arrange Member member = SetupMemberTestData(out MemberSave fakeMemberData, out MemberDisplay memberDisplay, ContentSaveAction.SaveNew); Mock.Get(umbracoMembersUserManager) - .Setup(x => x.CreateAsync(It.IsAny())) + .Setup(x => x.CreateAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(memberTypeService).Setup(x => x.GetDefault()).Returns("fakeAlias"); Mock.Get(backOfficeSecurityAccessor).Setup(x => x.BackOfficeSecurity).Returns(backOfficeSecurity); @@ -364,7 +364,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IBackOfficeSecurity backOfficeSecurity, - IPasswordChanger passwordChanger, + IPasswordChanger passwordChanger, IOptions globalSettings, IUser user) { @@ -376,7 +376,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers { roleName }; - var membersIdentityUser = new MembersIdentityUser(123); + var membersIdentityUser = new MemberIdentityUser(123); Mock.Get(umbracoMembersUserManager) .Setup(x => x.FindByIdAsync(It.IsAny())) .ReturnsAsync(() => membersIdentityUser); @@ -384,7 +384,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers .Setup(x => x.ValidatePasswordAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(umbracoMembersUserManager) - .Setup(x => x.UpdateAsync(It.IsAny())) + .Setup(x => x.UpdateAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(memberTypeService).Setup(x => x.GetDefault()).Returns("fakeAlias"); Mock.Get(backOfficeSecurityAccessor).Setup(x => x.BackOfficeSecurity).Returns(backOfficeSecurity); @@ -429,10 +429,10 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers IMemberService memberService, IMemberTypeService memberTypeService, IMemberGroupService memberGroupService, - IUmbracoUserManager membersUserManager, + IUmbracoUserManager membersUserManager, IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, - IPasswordChanger passwordChanger, + IPasswordChanger passwordChanger, IOptions globalSettings, IUser user) { diff --git a/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs b/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs index b314cb4fa1..00f2fff56c 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs @@ -2435,6 +2435,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers .Select(_umbracoMapper.Map) .ToArray(); + //TODO: change to role store var allGroups = _memberGroupService.GetAll().ToArray(); var groups = entry.Rules .Where(rule => rule.RuleType == Constants.Conventions.PublicAccess.MemberRoleRuleType) diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs index 3c5adb8ebe..b5f8674f9e 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs @@ -57,7 +57,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor; private readonly IJsonSerializer _jsonSerializer; private readonly IShortStringHelper _shortStringHelper; - private readonly IPasswordChanger _passwordChanger; + private readonly IPasswordChanger _passwordChanger; /// /// Initializes a new instance of the class. @@ -90,7 +90,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IJsonSerializer jsonSerializer, - IPasswordChanger passwordChanger) + IPasswordChanger passwordChanger) : base(cultureDictionary, loggerFactory, shortStringHelper, eventMessages, localizedTextService, jsonSerializer) { _propertyEditors = propertyEditors; @@ -355,7 +355,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers throw new InvalidOperationException($"No member type found with alias {contentItem.ContentTypeAlias}"); } - var identityMember = MembersIdentityUser.CreateNew( + var identityMember = MemberIdentityUser.CreateNew( contentItem.Username, contentItem.Email, memberType.Alias, @@ -445,7 +445,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers ModelState.AddModelError("custom", "An admin cannot lock a user"); } - MembersIdentityUser identityMember = await _memberManager.FindByIdAsync(contentItem.Id.ToString()); + MemberIdentityUser identityMember = await _memberManager.FindByIdAsync(contentItem.Id.ToString()); if (identityMember == null) { return new ValidationErrorResult("Identity member was not found"); @@ -586,7 +586,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// The member content item /// The member as an identity user - private async Task AddOrUpdateRoles(MemberSave contentItem, MembersIdentityUser identityMember) + private async Task AddOrUpdateRoles(MemberSave contentItem, MemberIdentityUser identityMember) { // We're gonna look up the current roles now because the below code can cause // events to be raised and developers could be manually adding roles to members in diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs index 7146cd5820..3ea1d615b8 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs @@ -1,7 +1,9 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Mapping; @@ -26,17 +28,19 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers private readonly IMemberGroupService _memberGroupService; private readonly UmbracoMapper _umbracoMapper; private readonly ILocalizedTextService _localizedTextService; - + private readonly RoleManager> _roleManager; + public MemberGroupController( IMemberGroupService memberGroupService, UmbracoMapper umbracoMapper, - ILocalizedTextService localizedTextService + ILocalizedTextService localizedTextService, + RoleManager> roleManager ) { _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); + _roleManager = roleManager ?? throw new ArgumentNullException(nameof(roleManager)); _umbracoMapper = umbracoMapper ?? throw new ArgumentNullException(nameof(umbracoMapper)); - _localizedTextService = - localizedTextService ?? throw new ArgumentNullException(nameof(localizedTextService)); + _localizedTextService = localizedTextService ?? throw new ArgumentNullException(nameof(localizedTextService)); } /// @@ -46,13 +50,13 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// public ActionResult GetById(int id) { - var memberGroup = _memberGroupService.GetById(id); + IdentityRole memberGroup = _roleManager.FindByIdAsync(id.ToString()).Result; if (memberGroup == null) { return NotFound(); } - var dto = _umbracoMapper.Map(memberGroup); + MemberGroupDisplay dto = _umbracoMapper.Map, MemberGroupDisplay>(memberGroup); return dto; } @@ -64,7 +68,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// public ActionResult GetById(Guid id) { - var memberGroup = _memberGroupService.GetById(id); + IMemberGroup memberGroup = _memberGroupService.GetById(id); if (memberGroup == null) { return NotFound(); @@ -82,9 +86,11 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers { var guidUdi = id as GuidUdi; if (guidUdi == null) + { return NotFound(); + } - var memberGroup = _memberGroupService.GetById(guidUdi.Guid); + IMemberGroup memberGroup = _memberGroupService.GetById(guidUdi.Guid); if (memberGroup == null) { return NotFound(); @@ -95,15 +101,22 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers public IEnumerable GetByIds([FromQuery]int[] ids) { - return _memberGroupService.GetByIds(ids) - .Select(_umbracoMapper.Map); + var roles = new List>(); + + foreach (int id in ids) + { + Task> role = _roleManager.FindByIdAsync(id.ToString()); + roles.Add(role.Result); + } + + return roles.Select(x=> _umbracoMapper.Map, MemberGroupDisplay>(x)); } [HttpDelete] [HttpPost] public IActionResult DeleteById(int id) { - var memberGroup = _memberGroupService.GetById(id); + IMemberGroup memberGroup = _memberGroupService.GetById(id); if (memberGroup == null) { return NotFound(); @@ -113,11 +126,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return Ok(); } - public IEnumerable GetAllGroups() - { - return _memberGroupService.GetAll() - .Select(_umbracoMapper.Map); - } + public IEnumerable GetAllGroups() => _roleManager.Roles.Select(x => _umbracoMapper.Map, MemberGroupDisplay>(x)); public MemberGroupDisplay GetEmpty() { diff --git a/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilderExtensions.cs index 53ea801490..6be3008a32 100644 --- a/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilderExtensions.cs @@ -86,7 +86,7 @@ namespace Umbraco.Extensions builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddUnique, PasswordChanger>(); - builder.Services.AddUnique, PasswordChanger>(); + builder.Services.AddUnique, PasswordChanger>(); return builder; } diff --git a/src/Umbraco.Web.BackOffice/Trees/MemberGroupTreeController.cs b/src/Umbraco.Web.BackOffice/Trees/MemberGroupTreeController.cs index 7445f35f29..5888068656 100644 --- a/src/Umbraco.Web.BackOffice/Trees/MemberGroupTreeController.cs +++ b/src/Umbraco.Web.BackOffice/Trees/MemberGroupTreeController.cs @@ -32,6 +32,7 @@ namespace Umbraco.Cms.Web.BackOffice.Trees _memberGroupService = memberGroupService; } + //TODO: change to role store protected override IEnumerable GetTreeNodesFromService(string id, FormCollection queryStrings) { return _memberGroupService.GetAll() @@ -49,6 +50,7 @@ namespace Umbraco.Cms.Web.BackOffice.Trees var root = rootResult.Value; //check if there are any groups + //TODO: change to role store root.HasChildren = _memberGroupService.GetAll().Any(); return root; } diff --git a/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs index 8b81fc673c..39a6b2906d 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs @@ -65,17 +65,17 @@ namespace Umbraco.Extensions public static void AddMembersIdentity(this IServiceCollection services) => services.BuildMembersIdentity() .AddDefaultTokenProviders() - .AddUserStore() + .AddUserStore() .AddMembersManager(); - private static MembersIdentityBuilder BuildMembersIdentity(this IServiceCollection services) + private static MemberIdentityBuilder BuildMembersIdentity(this IServiceCollection services) { // Services used by Umbraco members identity - services.TryAddScoped, UserValidator>(); - services.TryAddScoped, PasswordValidator>(); - services.TryAddScoped, PasswordHasher>(); - return new MembersIdentityBuilder(services); + services.TryAddScoped, UserValidator>(); + services.TryAddScoped, PasswordValidator>(); + services.TryAddScoped, PasswordHasher>(); + return new MemberIdentityBuilder(services); } private static void RemoveIntParamenterIfValueGreatherThen(IDictionary commands, string parameter, int maxValue) diff --git a/src/Umbraco.Web.Common/Extensions/IdentityBuilderExtensions.cs b/src/Umbraco.Web.Common/Extensions/IdentityBuilderExtensions.cs index 4daf5457bf..b1c2944565 100644 --- a/src/Umbraco.Web.Common/Extensions/IdentityBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/IdentityBuilderExtensions.cs @@ -10,13 +10,13 @@ namespace Umbraco.Extensions public static class IdentityBuilderExtensions { /// - /// Adds a for the . + /// Adds a for the . /// /// The usermanager interface /// The usermanager type /// The current instance. public static IdentityBuilder AddMembersManager(this IdentityBuilder identityBuilder) - where TUserManager : UserManager, TInterface + where TUserManager : UserManager, TInterface { identityBuilder.Services.AddScoped(typeof(TInterface), typeof(TUserManager)); return identityBuilder; diff --git a/src/Umbraco.Web.Common/Security/MemberManager.cs b/src/Umbraco.Web.Common/Security/MemberManager.cs index c36ae0c0fc..ae91852f46 100644 --- a/src/Umbraco.Web.Common/Security/MemberManager.cs +++ b/src/Umbraco.Web.Common/Security/MemberManager.cs @@ -16,21 +16,21 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Web.Common.Security { - public class MemberManager : UmbracoUserManager, IMemberManager + public class MemberManager : UmbracoUserManager, IMemberManager { private readonly IHttpContextAccessor _httpContextAccessor; public MemberManager( IIpResolver ipResolver, - IUserStore store, - IOptions optionsAccessor, - IPasswordHasher passwordHasher, - IEnumerable> userValidators, - IEnumerable> passwordValidators, + IUserStore store, + IOptions optionsAccessor, + IPasswordHasher passwordHasher, + IEnumerable> userValidators, + IEnumerable> passwordValidators, BackOfficeIdentityErrorDescriber errors, IServiceProvider services, IHttpContextAccessor httpContextAccessor, - ILogger> logger, + ILogger> logger, IOptions passwordConfiguration) : base(ipResolver, store, optionsAccessor, passwordHasher, userValidators, passwordValidators, errors, services, logger, passwordConfiguration) { diff --git a/src/Umbraco.Web/Security/Providers/MembersRoleProvider.cs b/src/Umbraco.Web/Security/Providers/MembersRoleProvider.cs deleted file mode 100644 index f28aa75e48..0000000000 --- a/src/Umbraco.Web/Security/Providers/MembersRoleProvider.cs +++ /dev/null @@ -1,106 +0,0 @@ -using System; -using System.Configuration.Provider; -using System.Linq; -using System.Web.Security; -using Umbraco.Cms.Core.Models; -using Umbraco.Cms.Core.Persistence.Querying; -using Umbraco.Cms.Core.Services; -using Umbraco.Web.Composing; - -namespace Umbraco.Web.Security.Providers -{ - //TODO: Delete: should not be used - [Obsolete("We are now using ASP.NET Core Identity instead of membership providers")] - public class MembersRoleProvider : RoleProvider - { - private readonly IMembershipRoleService _roleService; - private string _applicationName; - - public MembersRoleProvider(IMembershipRoleService roleService) - { - _roleService = roleService; - } - - public MembersRoleProvider() - : this(Current.Services.MemberService) - { - } - - public override bool IsUserInRole(string username, string roleName) - { - return GetRolesForUser(username).Any(x => x == roleName); - } - - public override string[] GetRolesForUser(string username) - { - return _roleService.GetAllRoles(username).ToArray(); - } - - public override void CreateRole(string roleName) - { - _roleService.AddRole(roleName); - } - - public override bool DeleteRole(string roleName, bool throwOnPopulatedRole) - { - return _roleService.DeleteRole(roleName, throwOnPopulatedRole); - } - - /// - /// Returns true if the specified member role name exists - /// - /// Member role name - /// True if member role exists, otherwise false - public override bool RoleExists(string roleName) => _roleService.GetAllRoles().Any(x => x.Name == roleName); - - public override void AddUsersToRoles(string[] usernames, string[] roleNames) - { - _roleService.AssignRoles(usernames, roleNames); - } - - public override void RemoveUsersFromRoles(string[] usernames, string[] roleNames) - { - _roleService.DissociateRoles(usernames, roleNames); - } - - public override string[] GetUsersInRole(string roleName) - { - return _roleService.GetMembersInRole(roleName).Select(x => x.Username).ToArray(); - } - - /// - /// Gets all the member roles - /// - /// A list of member roles - public override string[] GetAllRoles() => _roleService.GetAllRoles().Select(x => x.Name).ToArray(); - - public override string[] FindUsersInRole(string roleName, string usernameToMatch) - { - return _roleService.FindMembersInRole(roleName, usernameToMatch, StringPropertyMatchType.Wildcard).Select(x => x.Username).ToArray(); - } - - /// - /// The name of the application using the custom role provider. - /// - /// - /// The name of the application using the custom membership provider. - public override string ApplicationName - { - get - { - return _applicationName; - } - - set - { - if (string.IsNullOrEmpty(value)) - throw new ProviderException("ApplicationName cannot be empty."); - - if (value.Length > 0x100) - throw new ProviderException("Provider application name too long."); - - _applicationName = value; - } - } - } -} diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 93cb8da83d..fbe21702f9 100644 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -176,7 +176,6 @@ - @@ -229,4 +228,4 @@ - + \ No newline at end of file From 3d53690048ff8b45c614b6057a4627606c60de98 Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Sat, 6 Mar 2021 10:53:34 +0000 Subject: [PATCH 04/25] Add role store to service collection. Updated tests with new role methods. --- .../Services/IMemberGroupService.cs | 2 +- .../Security/MemberRoleStore.cs | 147 +++++++++----- .../Services/Implement/MemberGroupService.cs | 21 +- .../Security/MemberRoleStoreTests.cs | 185 +++++++++++++----- .../Umbraco.Tests.UnitTests.csproj | 6 + .../Controllers/MemberGroupController.cs | 6 +- .../ServiceCollectionExtensions.cs | 18 +- .../Extensions/IdentityBuilderExtensions.cs | 6 +- .../Umbraco.Web.Common.csproj | 1 + 9 files changed, 290 insertions(+), 102 deletions(-) diff --git a/src/Umbraco.Core/Services/IMemberGroupService.cs b/src/Umbraco.Core/Services/IMemberGroupService.cs index e584537ab1..16028ded3f 100644 --- a/src/Umbraco.Core/Services/IMemberGroupService.cs +++ b/src/Umbraco.Core/Services/IMemberGroupService.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using Umbraco.Cms.Core.Models; diff --git a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index 399da9aec0..0b91237af9 100644 --- a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -1,7 +1,4 @@ using System; -using System.Collections.Generic; -using System.Linq; -using System.Security.Claims; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Identity; @@ -13,35 +10,27 @@ namespace Umbraco.Cms.Core.Security /// /// A custom user store that uses Umbraco member data /// - public class MemberRoleStore : RoleStoreBase, string, IdentityUserRole, IdentityRoleClaim> + public class MemberRoleStore : IRoleStore where TRole : IdentityRole { private readonly IMemberGroupService _memberGroupService; + private bool _disposed; - public MemberRoleStore(IMemberGroupService memberGroupService, IdentityErrorDescriber describer) - : base(describer) => _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); - - /// - public override IQueryable> Roles + public MemberRoleStore(IMemberGroupService memberGroupService, IdentityErrorDescriber errorDescriber) { - get - { - IEnumerable memberGroups = _memberGroupService.GetAll(); - var identityRoles = new List>(); - foreach (IMemberGroup group in memberGroups) - { - IdentityRole identityRole = MapFromMemberGroup(group); - identityRoles.Add(identityRole); - } - - return identityRoles.AsQueryable(); - } + _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); + ErrorDescriber = errorDescriber ?? throw new ArgumentNullException(nameof(errorDescriber)); } + /// + /// Gets or sets the for any error that occurred with the current operation. + /// + public IdentityErrorDescriber ErrorDescriber { get; set; } + /// - public override Task CreateAsync( - IdentityRole role, - CancellationToken cancellationToken = new CancellationToken()) + public Task CreateAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); if (role == null) { throw new ArgumentNullException(nameof(role)); @@ -61,9 +50,10 @@ namespace Umbraco.Cms.Core.Security /// - public override Task UpdateAsync(IdentityRole role, - CancellationToken cancellationToken = new CancellationToken()) + public Task UpdateAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); if (role == null) { throw new ArgumentNullException(nameof(role)); @@ -93,9 +83,10 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task DeleteAsync(IdentityRole role, - CancellationToken cancellationToken = new CancellationToken()) + public Task DeleteAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); if (role == null) { throw new ArgumentNullException(nameof(role)); @@ -122,9 +113,60 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task> FindByIdAsync(string id, - CancellationToken cancellationToken = new CancellationToken()) + + public Task GetRoleIdAsync(TRole role, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + return Task.FromResult(role.Id); + } + + public Task GetRoleNameAsync(TRole role, CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (!int.TryParse(role.Id, out int roleId)) + { + return null; + } + + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + + return Task.FromResult(memberGroup?.Name); + } + + public Task SetRoleNameAsync(TRole role, string roleName, CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + return null; + } + + public Task GetNormalizedRoleNameAsync(TRole role, CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + return null; + } + + public Task SetNormalizedRoleNameAsync(TRole role, string normalizedName, CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + return null; + } + + /// + public Task FindByIdAsync(string id, CancellationToken cancellationToken = new CancellationToken()) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); if (!int.TryParse(id, out int roleId)) { return null; @@ -136,39 +178,34 @@ namespace Umbraco.Cms.Core.Security } /// - public override Task> FindByNameAsync(string normalizedName, - CancellationToken cancellationToken = new CancellationToken()) + public Task FindByNameAsync(string name, CancellationToken cancellationToken = new CancellationToken()) { - IMemberGroup memberGroup = _memberGroupService.GetByName(normalizedName); + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (name == null) + { + throw new ArgumentNullException(nameof(name)); + } + IMemberGroup memberGroup = _memberGroupService.GetByName(name); return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); } - ///TODO: are we implementing these claims methods? - - /// - public override Task> GetClaimsAsync(IdentityRole role, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task AddClaimAsync(IdentityRole role, Claim claim, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - - /// - public override Task RemoveClaimAsync(IdentityRole role, Claim claim, CancellationToken cancellationToken = new CancellationToken()) => throw new System.NotImplementedException(); - /// /// Maps a member group to an identity role /// /// /// - private IdentityRole MapFromMemberGroup(IMemberGroup memberGroup) + private TRole MapFromMemberGroup(IMemberGroup memberGroup) { var result = new IdentityRole { Id = memberGroup.Id.ToString(), Name = memberGroup.Name + //TODO: Are we interested in NormalizedRoleName? }; - return result; + return result as TRole; } /// @@ -177,7 +214,7 @@ namespace Umbraco.Cms.Core.Security /// /// /// - private bool MapToMemberGroup(IdentityRole role, IMemberGroup memberGroup) + private bool MapToMemberGroup(TRole role, IMemberGroup memberGroup) { var anythingChanged = false; @@ -189,5 +226,21 @@ namespace Umbraco.Cms.Core.Security return anythingChanged; } + + public void Dispose() + { + //TODO: is any dispose action necessary here or is this all managed by the IOC container? + } + + /// + /// Throws if this class has been disposed. + /// + protected void ThrowIfDisposed() + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().Name); + } + } } } diff --git a/src/Umbraco.Infrastructure/Services/Implement/MemberGroupService.cs b/src/Umbraco.Infrastructure/Services/Implement/MemberGroupService.cs index 5e6138980a..1f6d4743be 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/MemberGroupService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/MemberGroupService.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -130,6 +130,25 @@ namespace Umbraco.Cms.Core.Services.Implement } } + + public void GetByRole(IMemberGroup memberGroup) + { + using (var scope = ScopeProvider.CreateScope()) + { + var deleteEventArgs = new DeleteEventArgs(memberGroup); + if (scope.Events.DispatchCancelable(Deleting, this, deleteEventArgs)) + { + scope.Complete(); + return; + } + + _memberGroupRepository.Delete(memberGroup); + scope.Complete(); + deleteEventArgs.CanCancel = false; + scope.Events.Dispatch(Deleted, this, deleteEventArgs); + } + } + /// /// Occurs before Delete of a member group /// diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index f74b6cef94..ed77c63afc 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -7,7 +6,6 @@ using Microsoft.AspNetCore.Identity; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Models; -using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; @@ -19,19 +17,19 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security private Mock _mockMemberGroupService; private IdentityErrorDescriber ErrorDescriber => new IdentityErrorDescriber(); - public MemberRoleStore CreateSut() + public MemberRoleStore CreateSut() { _mockMemberGroupService = new Mock(); - return new MemberRoleStore( + return new MemberRoleStore( _mockMemberGroupService.Object, ErrorDescriber); } [Test] - public void GivenICreateAMemberRole_AndTheGroupIsNull_ThenIShouldGetAFailedResultAsync() + public void GivenICreateAMemberRole_AndTheGroupIsNull_ThenIShouldGetAnArgumentException() { // arrange - MemberRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); CancellationToken fakeCancellationToken = new CancellationToken() { }; // act @@ -46,8 +44,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenICreateAMemberRole_AndTheGroupIsPopulatedCorrectly_ThenIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { Id = "777", Name = "testname" @@ -74,8 +72,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupExistsWithTheSameName_ThenIShouldGetASuccessResultAsyncButNoUpdatesMade() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { Id = "777", Name = "fakeGroupName" @@ -103,8 +101,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupExistsWithADifferentSameName_ThenIShouldGetASuccessResultAsyncWithUpdatesMade() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { Id = "777", Name = "fakeGroup777" @@ -133,8 +131,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupDoesntExist_ThenIShouldGetAFailureResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { Id = "777", Name = "testname" @@ -157,31 +155,29 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { Id = "7a77", Name = "testname" }; var fakeCancellationToken = new CancellationToken() { }; - - bool raiseEvents = false; - - + // act IdentityResult identityResult = await sut.UpdateAsync(fakeRole, fakeCancellationToken); // assert Assert.IsTrue(identityResult.Succeeded == false); Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "DefaultError" && x.Description == "An unknown failure has occurred.")); + _mockMemberGroupService.VerifyNoOtherCalls(); } [Test] public async Task GivenIDeleteAMemberRole_AndItExists_ThenTheMemberGroupShouldBeDeleted_AndIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { Id = "777", Name = "testname" @@ -201,14 +197,15 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Assert.IsTrue(!identityResult.Errors.Any()); _mockMemberGroupService.Verify(x => x.GetById(777)); _mockMemberGroupService.Verify(x => x.Delete(mockMemberGroup)); + _mockMemberGroupService.VerifyNoOtherCalls(); } [Test] public async Task GivenIDeleteAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAFailResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { Id = "7a77", Name = "testname" @@ -225,6 +222,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded == false); Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "DefaultError" && x.Description == "An unknown failure has occurred.")); + _mockMemberGroupService.VerifyNoOtherCalls(); } @@ -232,8 +230,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIDeleteAMemberRole_AndItDoesntExist_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAFailResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole() + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { Id = "777", Name = "testname" @@ -251,43 +249,142 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Assert.IsTrue(identityResult.Succeeded == false); Assert.IsTrue(identityResult.Errors.Any(x=>x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); _mockMemberGroupService.Verify(x => x.GetById(777)); + _mockMemberGroupService.VerifyNoOtherCalls(); } [Test] - public async Task GivenIGetAllMemberRoles_ThenIShouldGetAllMemberGroups_AndASuccessResultAsync() + public async Task GivenIFindAMemberRoleByRoleId_AndRoleIdExists_ThenIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeRole = new IdentityRole("fakeGroupName") { Id = "777" }; - IEnumerable> expected = new List>() + int fakeRoleId = 777; + + IMemberGroup mockMemberGroup = Mock.Of(m => + m.Name == "fakeGroupName" && + m.CreatorId == 123 && + m.Id == 777); + + var fakeCancellationToken = new CancellationToken() { }; + + _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(mockMemberGroup); + + // act + IdentityRole actual = await sut.FindByIdAsync(fakeRole.Id, fakeCancellationToken); + + // assert + Assert.AreEqual(fakeRole.Name, actual.Name); + Assert.AreEqual(fakeRole.Id, actual.Id); + _mockMemberGroupService.Verify(x => x.GetById(fakeRoleId)); + _mockMemberGroupService.VerifyNoOtherCalls(); + } + + [Test] + public void GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync() + { + // arrange + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole { - fakeRole + Id = "7a77", + Name = "testname" + }; + var fakeCancellationToken = new CancellationToken() { }; + + // act + Task actual = sut.FindByIdAsync(fakeRole.Id, fakeCancellationToken); + + // assert + Assert.IsNull(actual); + _mockMemberGroupService.VerifyNoOtherCalls(); + } + + [Test] + public async Task GivenIFindAMemberRoleByRoleName_AndRoleNameExists_ThenIShouldGetASuccessResultAsync() + { + // arrange + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole("fakeGroupName") + { + Id = "777" }; IMemberGroup mockMemberGroup = Mock.Of(m => - m.Name == "fakeGroupName" && m.CreatorId == 123 && m.Id == 777); + m.Name == "fakeGroupName" && + m.CreatorId == 123 && + m.Id == 777); - IEnumerable fakeMemberGroups = new List() - { - mockMemberGroup - }; - _mockMemberGroupService.Setup(x => x.GetAll()).Returns(fakeMemberGroups); + _mockMemberGroupService.Setup(x => x.GetByName(fakeRole.Name)).Returns(mockMemberGroup); // act - IQueryable> actual = sut.Roles; + IdentityRole actual = await sut.FindByNameAsync(fakeRole.Name); // assert - Assert.AreEqual(expected.AsQueryable().First().Id, actual.First().Id); - Assert.AreEqual(expected.AsQueryable().First().Name, actual.First().Name); - //Always null: - //Assert.AreEqual(expected.AsQueryable().First().NormalizedName, actual.First().NormalizedName); - //Always different: - //Assert.AreEqual(expected.AsQueryable().First().ConcurrencyStamp, actual.First().ConcurrencyStamp); - _mockMemberGroupService.Verify(x => x.GetAll()); + Assert.AreEqual(fakeRole.Name, actual.Name); + Assert.AreEqual(fakeRole.Id, actual.Id); + _mockMemberGroupService.Verify(x => x.GetByName(fakeRole.Name)); + } + + [Test] + public void GivenIFindAMemberRoleByRoleName_AndTheNameIsNull_ThenIShouldGetAnArgumentException() + { + // arrange + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole + { + Id = "777" + }; + var fakeCancellationToken = new CancellationToken() { }; + + // act + Action actual = () => sut.FindByNameAsync(fakeRole.Name, fakeCancellationToken); + + // assert + Assert.That(actual, Throws.ArgumentNullException); + _mockMemberGroupService.VerifyNoOtherCalls(); + + } + + [Test] + public void GivenIGetAMemberRoleId_AndTheRoleIsNull_ThenIShouldGetAnArgumentException() + { + // arrange + MemberRoleStore sut = CreateSut(); + var fakeCancellationToken = new CancellationToken() { }; + + // act + Action actual = () => sut.GetRoleIdAsync(null, fakeCancellationToken); + + // assert + Assert.That(actual, Throws.ArgumentNullException); + } + + [Test] + public void GivenIGetAMemberRoleId_AndTheRoleIsNotNull_ThenIShouldGetTheMemberRole() + { + // arrange + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole("fakeGroupName") + { + Id = "777" + }; + string fakeRoleId = fakeRole.Id; + + var fakeCancellationToken = new CancellationToken(); + + // act + Task actual = sut.GetRoleIdAsync(fakeRole, fakeCancellationToken); + + // assert + Assert.That(actual, Throws.ArgumentNullException); + + + // assert + Assert.AreEqual(fakeRoleId, actual.Result); } } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj b/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj index bcb4022ec7..a85b158810 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj @@ -38,4 +38,10 @@ + + + ..\Umbraco.Web\bin\Debug\Umbraco.Infrastructure.dll + + + diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs index 3ea1d615b8..df76736de9 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs @@ -28,13 +28,13 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers private readonly IMemberGroupService _memberGroupService; private readonly UmbracoMapper _umbracoMapper; private readonly ILocalizedTextService _localizedTextService; - private readonly RoleManager> _roleManager; + private readonly RoleManager _roleManager; public MemberGroupController( IMemberGroupService memberGroupService, UmbracoMapper umbracoMapper, ILocalizedTextService localizedTextService, - RoleManager> roleManager + RoleManager roleManager ) { _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); @@ -105,7 +105,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers foreach (int id in ids) { - Task> role = _roleManager.FindByIdAsync(id.ToString()); + Task role = _roleManager.FindByIdAsync(id.ToString()); roles.Add(role.Result); } diff --git a/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs index 39a6b2906d..b123a659c5 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs @@ -4,6 +4,7 @@ using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Options; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.Web.Caching; using SixLabors.ImageSharp.Web.Commands; @@ -62,12 +63,23 @@ namespace Umbraco.Extensions /// /// Adds the services required for using Members Identity /// - public static void AddMembersIdentity(this IServiceCollection services) => + public static void AddMembersIdentity(this IServiceCollection services) + { services.BuildMembersIdentity() .AddDefaultTokenProviders() + .AddMemberManager() + //.AddRoles() .AddUserStore() - .AddMembersManager(); + .AddRoleStore>() + .AddRoleValidator>() + .AddRoleManager>(); + //services.AddScoped>( + // s => new UserClaimsPrincipalFactory( + // s.GetService(), + // s.GetService>(), + // s.GetService>())); + } private static MemberIdentityBuilder BuildMembersIdentity(this IServiceCollection services) { @@ -75,7 +87,7 @@ namespace Umbraco.Extensions services.TryAddScoped, UserValidator>(); services.TryAddScoped, PasswordValidator>(); services.TryAddScoped, PasswordHasher>(); - return new MemberIdentityBuilder(services); + return new MemberIdentityBuilder(typeof(IdentityRole), services); } private static void RemoveIntParamenterIfValueGreatherThen(IDictionary commands, string parameter, int maxValue) diff --git a/src/Umbraco.Web.Common/Extensions/IdentityBuilderExtensions.cs b/src/Umbraco.Web.Common/Extensions/IdentityBuilderExtensions.cs index b1c2944565..6940221518 100644 --- a/src/Umbraco.Web.Common/Extensions/IdentityBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/IdentityBuilderExtensions.cs @@ -12,10 +12,10 @@ namespace Umbraco.Extensions /// /// Adds a for the . /// - /// The usermanager interface - /// The usermanager type + /// The member manager interface + /// The member manager type /// The current instance. - public static IdentityBuilder AddMembersManager(this IdentityBuilder identityBuilder) + public static IdentityBuilder AddMemberManager(this IdentityBuilder identityBuilder) where TUserManager : UserManager, TInterface { identityBuilder.Services.AddScoped(typeof(TInterface), typeof(TUserManager)); diff --git a/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj b/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj index 2bcf4a3e55..3dc8575dd7 100644 --- a/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj +++ b/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj @@ -22,6 +22,7 @@ + From 05ccb9e8bbe6285521653c6e4652f5b9cd65c355 Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sat, 6 Mar 2021 13:44:02 +0000 Subject: [PATCH 05/25] Extended and updated unit tests. Put store methods into try and catch where accessing a service. Added more identity results and try/catches where needed. Updated memberuserstore. Ignore temporary test cached files. --- .gitignore | 1 + .../Security/MemberRoleStore.cs | 294 ++++++++++------ .../Security/MemberUserStore.cs | 320 ++++++++++++------ .../Security/MemberRoleStoreTests.cs | 30 +- 4 files changed, 411 insertions(+), 234 deletions(-) diff --git a/.gitignore b/.gitignore index 0cffac8343..f8a2ce511f 100644 --- a/.gitignore +++ b/.gitignore @@ -201,3 +201,4 @@ src/Umbraco.Tests/TEMP/ /src/Umbraco.Web.UI/config/umbracoSettings.config /src/Umbraco.Web.UI.NetCore/Umbraco/models/* +src/Umbraco.Tests.UnitTests/umbraco/Data/TEMP/ diff --git a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index 0b91237af9..4477c4870c 100644 --- a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -15,6 +15,11 @@ namespace Umbraco.Cms.Core.Security private readonly IMemberGroupService _memberGroupService; private bool _disposed; + //TODO: Move into custom error describer. + //TODO: How revealing can the error messages be? + private readonly IdentityError _intParseError = new IdentityError { Code = "IdentityIdParseError", Description = "Cannot parse ID to int" }; + private readonly IdentityError _memberGroupNotFoundError = new IdentityError { Code = "IdentityMemberGroupNotFound", Description = "Member group not found" }; + public MemberRoleStore(IMemberGroupService memberGroupService, IdentityErrorDescriber errorDescriber) { _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); @@ -25,99 +30,123 @@ namespace Umbraco.Cms.Core.Security /// Gets or sets the for any error that occurred with the current operation. /// public IdentityErrorDescriber ErrorDescriber { get; set; } - - /// - public Task CreateAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (role == null) - { - throw new ArgumentNullException(nameof(role)); - } - - var memberGroup = new MemberGroup - { - Name = role.Name - }; - - _memberGroupService.Save(memberGroup); - - role.Id = memberGroup.Id.ToString(); - - return Task.FromResult(IdentityResult.Success); - } - /// - public Task UpdateAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) + public Task CreateAsync(TRole role, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (role == null) + try { - throw new ArgumentNullException(nameof(role)); - } + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); - if (!int.TryParse(role.Id, out int roleId)) - { - //TODO: what identity error should we return in this case? - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); - } - - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - if (memberGroup != null) - { - if (MapToMemberGroup(role, memberGroup)) + if (role == null) { - _memberGroupService.Save(memberGroup); + throw new ArgumentNullException(nameof(role)); } - } - else - { - //TODO: throw exception when not found, or return failure? And is this the correct message - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.InvalidRoleName(role.Name))); - } - return Task.FromResult(IdentityResult.Success); + var memberGroup = new MemberGroup + { + Name = role.Name + }; + + _memberGroupService.Save(memberGroup); + + role.Id = memberGroup.Id.ToString(); + + return Task.FromResult(IdentityResult.Success); + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + } + } + + + /// + public Task UpdateAsync(TRole role, CancellationToken cancellationToken = default) + { + try + { + cancellationToken.ThrowIfCancellationRequested(); + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + ThrowIfDisposed(); + + if (!int.TryParse(role.Id, out int roleId)) + { + return Task.FromResult(IdentityResult.Failed(_intParseError)); + } + + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + if (MapToMemberGroup(role, memberGroup)) + { + _memberGroupService.Save(memberGroup); + } + //TODO: if nothing changed, do we need to report this? + return Task.FromResult(IdentityResult.Success); + } + else + { + //TODO: throw exception when not found, or return failure? + return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); + } + + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + } } /// - public Task DeleteAsync(TRole role, CancellationToken cancellationToken = new CancellationToken()) + public Task DeleteAsync(TRole role, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (role == null) + try { - throw new ArgumentNullException(nameof(role)); - } + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); - if (!int.TryParse(role.Id, out int roleId)) - { - //TODO: what identity error should we return in this case? - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); - } + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - if (memberGroup != null) - { - _memberGroupService.Delete(memberGroup); - } - else - { - //TODO: throw exception when not found, or return failure? And is this the correct message - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.InvalidRoleName(role.Name))); - } + if (!int.TryParse(role.Id, out int roleId)) + { + //TODO: what identity error should we return in this case? + return Task.FromResult(IdentityResult.Failed(_intParseError)); + } - return Task.FromResult(IdentityResult.Success); + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + _memberGroupService.Delete(memberGroup); + } + else + { + return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); + } + + return Task.FromResult(IdentityResult.Success); + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + } } /// - public Task GetRoleIdAsync(TRole role, CancellationToken cancellationToken) + public Task GetRoleIdAsync(TRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); + if (role == null) { throw new ArgumentNullException(nameof(role)); @@ -126,67 +155,118 @@ namespace Umbraco.Cms.Core.Security return Task.FromResult(role.Id); } - public Task GetRoleNameAsync(TRole role, CancellationToken cancellationToken) + /// + public Task GetRoleNameAsync(TRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (!int.TryParse(role.Id, out int roleId)) + if (role == null) { - return null; + throw new ArgumentNullException(nameof(role)); } - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - - return Task.FromResult(memberGroup?.Name); - } - - public Task SetRoleNameAsync(TRole role, string roleName, CancellationToken cancellationToken) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - return null; - } - - public Task GetNormalizedRoleNameAsync(TRole role, CancellationToken cancellationToken) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - return null; - } - - public Task SetNormalizedRoleNameAsync(TRole role, string normalizedName, CancellationToken cancellationToken) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - return null; + return Task.FromResult(role.Name); } /// - public Task FindByIdAsync(string id, CancellationToken cancellationToken = new CancellationToken()) + public Task SetRoleNameAsync(TRole role, string roleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (!int.TryParse(id, out int roleId)) + + if (role == null) { - return null; + throw new ArgumentNullException(nameof(role)); + } + + if (!int.TryParse(role.Id, out int roleId)) + { + //TODO: what identity error should we return in this case? + return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); } IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + //TODO: confirm logic + memberGroup.Name = roleName; + _memberGroupService.Save(memberGroup); + role.Name = roleName; + } + else + { + return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); + } + + return Task.CompletedTask; + } + + /// + public Task GetNormalizedRoleNameAsync(TRole role, CancellationToken cancellationToken = default) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + //TODO: are we utilising NormalizedRoleName? + return Task.FromResult(role.NormalizedName); + } + + /// + public Task SetNormalizedRoleNameAsync(TRole role, string normalizedName, CancellationToken cancellationToken = default) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (role == null) + { + throw new ArgumentNullException(nameof(role)); + } + + //TODO: are we utilising NormalizedRoleName and do we need to set it in the memberGroupService? + role.NormalizedName = normalizedName; + + return Task.CompletedTask; + } + + /// + public Task FindByIdAsync(string roleId, CancellationToken cancellationToken = default) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (string.IsNullOrWhiteSpace(roleId)) + { + throw new ArgumentNullException(nameof(roleId)); + } + + if (!int.TryParse(roleId, out int id)) + { + throw new ArgumentOutOfRangeException(nameof(roleId), $"{nameof(roleId)} is not a valid Int"); + } + + IMemberGroup memberGroup = _memberGroupService.GetById(id); return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); } /// - public Task FindByNameAsync(string name, CancellationToken cancellationToken = new CancellationToken()) + public Task FindByNameAsync(string name, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (name == null) + + if (string.IsNullOrWhiteSpace(name)) { throw new ArgumentNullException(nameof(name)); } IMemberGroup memberGroup = _memberGroupService.GetByName(name); + //TODO: throw exception when not found? return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); } @@ -227,10 +307,12 @@ namespace Umbraco.Cms.Core.Security return anythingChanged; } - public void Dispose() - { - //TODO: is any dispose action necessary here or is this all managed by the IOC container? - } + //TODO: is any dispose action necessary here? + + /// + /// Dispose the store + /// + public void Dispose() => _disposed = true; /// /// Throws if this class has been disposed. diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 17a45764ad..1d871e4134 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -19,7 +19,7 @@ namespace Umbraco.Cms.Core.Security /// /// A custom user store that uses Umbraco member data /// - public class MemberUserStore : UserStoreBase, string, IdentityUserClaim, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> + public class MemberUserStore : UserStoreBase, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> { private readonly IMemberService _memberService; private readonly UmbracoMapper _mapper; @@ -40,6 +40,7 @@ namespace Umbraco.Cms.Core.Security _scopeProvider = scopeProvider ?? throw new ArgumentNullException(nameof(scopeProvider)); } + //TODO: why is this not supported? /// /// Not supported in Umbraco /// @@ -56,117 +57,140 @@ namespace Umbraco.Cms.Core.Security /// public override Task CreateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (user == null) + try { - throw new ArgumentNullException(nameof(user)); + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } + + // create member + IMember memberEntity = _memberService.CreateMember( + user.UserName, + user.Email, + user.Name.IsNullOrWhiteSpace() ? user.UserName : user.Name, + user.MemberTypeAlias.IsNullOrWhiteSpace() ? Constants.Security.DefaultMemberTypeAlias : user.MemberTypeAlias); + + UpdateMemberProperties(memberEntity, user); + + // create the member + _memberService.Save(memberEntity); + + if (!memberEntity.HasIdentity) + { + throw new DataException("Could not create the member, check logs for details"); + } + + // re-assign id + user.Id = UserIdToString(memberEntity.Id); + + // [from backofficeuser] we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. + // var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MembersIdentityUser.Logins)); + // TODO: confirm re externallogins implementation + //if (isLoginsPropertyDirty) + //{ + // _externalLoginService.Save( + // user.Id, + // user.Logins.Select(x => new ExternalLogin( + // x.LoginProvider, + // x.ProviderKey, + // x.UserData))); + //} + + + return Task.FromResult(IdentityResult.Success); } - - // create member - IMember memberEntity = _memberService.CreateMember( - user.UserName, - user.Email, - user.Name.IsNullOrWhiteSpace() ? user.UserName : user.Name, - user.MemberTypeAlias.IsNullOrWhiteSpace() ? Constants.Security.DefaultMemberTypeAlias : user.MemberTypeAlias); - - UpdateMemberProperties(memberEntity, user); - - // create the member - _memberService.Save(memberEntity); - - if (!memberEntity.HasIdentity) + catch (Exception ex) { - throw new DataException("Could not create the member, check logs for details"); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); } - - // re-assign id - user.Id = UserIdToString(memberEntity.Id); - - // [from backofficeuser] we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. - // var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MembersIdentityUser.Logins)); - // TODO: confirm re externallogins implementation - //if (isLoginsPropertyDirty) - //{ - // _externalLoginService.Save( - // user.Id, - // user.Logins.Select(x => new ExternalLogin( - // x.LoginProvider, - // x.ProviderKey, - // x.UserData))); - //} - - return Task.FromResult(IdentityResult.Success); } /// public override Task UpdateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (user == null) + try { - throw new ArgumentNullException(nameof(user)); - } - - Attempt asInt = user.Id.TryConvertTo(); - if (asInt == false) - { - throw new InvalidOperationException("The user id must be an integer to work with the Umbraco"); - } - - using (IScope scope = _scopeProvider.CreateScope()) - { - IMember found = _memberService.GetById(asInt.Result); - if (found != null) + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (user == null) { - // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. - var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins)); - - if (UpdateMemberProperties(found, user)) - { - _memberService.Save(found); - } - - // TODO: when to implement external login service? - - //if (isLoginsPropertyDirty) - //{ - // _externalLoginService.Save( - // found.Id, - // user.Logins.Select(x => new ExternalLogin( - // x.LoginProvider, - // x.ProviderKey, - // x.UserData))); - //} + throw new ArgumentNullException(nameof(user)); } - scope.Complete(); - } + Attempt asInt = user.Id.TryConvertTo(); + if (asInt == false) + { + //TODO: should this be thrown, or an identity result? + throw new InvalidOperationException("The user id must be an integer to work with Umbraco"); + } - return Task.FromResult(IdentityResult.Success); + using (IScope scope = _scopeProvider.CreateScope()) + { + IMember found = _memberService.GetById(asInt.Result); + if (found != null) + { + // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. + var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins)); + + if (UpdateMemberProperties(found, user)) + { + _memberService.Save(found); + } + + // TODO: when to implement external login service? + + //if (isLoginsPropertyDirty) + //{ + // _externalLoginService.Save( + // found.Id, + // user.Logins.Select(x => new ExternalLogin( + // x.LoginProvider, + // x.ProviderKey, + // x.UserData))); + //} + } + + scope.Complete(); + + return Task.FromResult(IdentityResult.Success); + } + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + } } /// public override Task DeleteAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (user == null) + try { - throw new ArgumentNullException(nameof(user)); - } + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } - IMember found = _memberService.GetById(UserIdToInt(user.Id)); - if (found != null) + IMember found = _memberService.GetById(UserIdToInt(user.Id)); + if (found != null) + { + _memberService.Delete(found); + } + + // TODO: when to implement external login service? + //_externalLoginService.DeleteUserLogins(UserIdToInt(user.Id)); + + return Task.FromResult(IdentityResult.Success); + } + catch (Exception ex) { - _memberService.Delete(found); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); } - - // TODO: when to implement external login service? - //_externalLoginService.DeleteUserLogins(UserIdToInt(user.Id)); - - return Task.FromResult(IdentityResult.Success); } /// @@ -178,6 +202,16 @@ namespace Umbraco.Cms.Core.Security cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); + if (string.IsNullOrWhiteSpace(userId)) + { + throw new ArgumentNullException(nameof(userId)); + } + + if (!Guid.TryParse(userId, out Guid id)) + { + throw new ArgumentOutOfRangeException(nameof(userId), $"{nameof(userId)} is not a valid x"); + } + IMember user = _memberService.GetById(UserIdToInt(userId)); if (user == null) { @@ -208,7 +242,8 @@ namespace Umbraco.Cms.Core.Security { await base.SetPasswordHashAsync(user, passwordHash, cancellationToken); - user.PasswordConfig = null; // Clear this so that it's reset at the repository level + // Clear this so that it's reset at the repository level + user.PasswordConfig = null; user.LastPasswordChangeDateUtc = DateTime.UtcNow; } @@ -216,7 +251,7 @@ namespace Umbraco.Cms.Core.Security public override async Task HasPasswordAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) { // This checks if it's null - var result = await base.HasPasswordAsync(user, cancellationToken); + bool result = await base.HasPasswordAsync(user, cancellationToken); if (result) { // we also want to check empty @@ -262,8 +297,22 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(login)); } + if (string.IsNullOrWhiteSpace(login.LoginProvider)) + { + throw new ArgumentNullException(nameof(login.LoginProvider)); + } + + if (string.IsNullOrWhiteSpace(login.ProviderKey)) + { + throw new ArgumentNullException(nameof(login.ProviderKey)); + } + ICollection logins = user.Logins; - var instance = new IdentityUserLogin(login.LoginProvider, login.ProviderKey, user.Id.ToString()); + var instance = new IdentityUserLogin( + login.LoginProvider, + login.ProviderKey, + user.Id.ToString()); + IdentityUserLogin userLogin = instance; logins.Add(userLogin); @@ -280,6 +329,16 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(user)); } + if (string.IsNullOrWhiteSpace(loginProvider)) + { + throw new ArgumentNullException(nameof(loginProvider)); + } + + if (string.IsNullOrWhiteSpace(providerKey)) + { + throw new ArgumentNullException(nameof(providerKey)); + } + IIdentityUserLogin userLogin = user.Logins.SingleOrDefault(l => l.LoginProvider == loginProvider && l.ProviderKey == providerKey); if (userLogin != null) { @@ -311,21 +370,34 @@ namespace Umbraco.Cms.Core.Security MemberIdentityUser user = await FindUserAsync(userId, cancellationToken); if (user == null) { - return null; + //TODO: error throw or null result? + return await Task.FromResult((IdentityUserLogin)null); + } + + if (string.IsNullOrWhiteSpace(loginProvider)) + { + throw new ArgumentNullException(nameof(loginProvider)); + } + + if (string.IsNullOrWhiteSpace(providerKey)) + { + throw new ArgumentNullException(nameof(providerKey)); } IList logins = await GetLoginsAsync(user, cancellationToken); UserLoginInfo found = logins.FirstOrDefault(x => x.ProviderKey == providerKey && x.LoginProvider == loginProvider); if (found == null) { - return null; + //TODO: error throw or null result? + return await Task.FromResult((IdentityUserLogin)null); } return new IdentityUserLogin { LoginProvider = found.LoginProvider, ProviderKey = found.ProviderKey, - ProviderDisplayName = found.ProviderDisplayName, // TODO: We don't store this value so it will be null + // TODO: We don't store this value so it will be null + ProviderDisplayName = found.ProviderDisplayName, UserId = user.Id }; } @@ -336,9 +408,19 @@ namespace Umbraco.Cms.Core.Security cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); + if (string.IsNullOrWhiteSpace(loginProvider)) + { + throw new ArgumentNullException(nameof(loginProvider)); + } + + if (string.IsNullOrWhiteSpace(providerKey)) + { + throw new ArgumentNullException(nameof(providerKey)); + } + var logins = new List(); - // TODO: external login needed? + // TODO: external login needed //_externalLoginService.Find(loginProvider, providerKey).ToList(); if (logins.Count == 0) { @@ -350,7 +432,8 @@ namespace Umbraco.Cms.Core.Security { LoginProvider = found.LoginProvider, ProviderKey = found.ProviderKey, - ProviderDisplayName = null, // TODO: We don't store this value so it will be null + // TODO: We don't store this value so it will be null + ProviderDisplayName = null, UserId = found.UserId }); } @@ -358,7 +441,11 @@ namespace Umbraco.Cms.Core.Security /// public override Task AddToRoleAsync(MemberIdentityUser user, string role, CancellationToken cancellationToken = default) { - cancellationToken.ThrowIfCancellationRequested(); + if (cancellationToken != null) + { + cancellationToken.ThrowIfCancellationRequested(); + } + ThrowIfDisposed(); if (user == null) { @@ -443,7 +530,7 @@ namespace Umbraco.Cms.Core.Security /// /// Returns true if a user is in the role /// - public override Task IsInRoleAsync(MemberIdentityUser user, string normalizedRoleName, CancellationToken cancellationToken = default) + public override Task IsInRoleAsync(MemberIdentityUser user, string roleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -452,22 +539,28 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(user)); } - return Task.FromResult(user.Roles.Select(x => x.RoleId).InvariantContains(normalizedRoleName)); + if (string.IsNullOrWhiteSpace(roleName)) + { + throw new ArgumentNullException(nameof(roleName)); + } + + return Task.FromResult(user.Roles.Select(x => x.RoleId).InvariantContains(roleName)); } /// /// Lists all users of a given role. /// - public override Task> GetUsersInRoleAsync(string normalizedRoleName, CancellationToken cancellationToken = default) + public override Task> GetUsersInRoleAsync(string roleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (normalizedRoleName == null) + + if (string.IsNullOrWhiteSpace(roleName)) { - throw new ArgumentNullException(nameof(normalizedRoleName)); + throw new ArgumentNullException(nameof(roleName)); } - IEnumerable members = _memberService.GetMembersByMemberType(normalizedRoleName); + IEnumerable members = _memberService.GetMembersByMemberType(roleName); IList membersIdentityUsers = members.Select(x => _mapper.Map(x)).ToList(); @@ -475,18 +568,23 @@ namespace Umbraco.Cms.Core.Security } /// - protected override Task> FindRoleAsync(string normalizedRoleName, CancellationToken cancellationToken) + protected override Task FindRoleAsync(string roleName, CancellationToken cancellationToken) { - IMemberGroup group = _memberService.GetAllRoles().SingleOrDefault(x => x.Name == normalizedRoleName); - if (group == null) + if (string.IsNullOrWhiteSpace(roleName)) { - return Task.FromResult((IdentityRole)null); + throw new ArgumentNullException(nameof(roleName)); } - return Task.FromResult(new IdentityRole(group.Name) + IMemberGroup group = _memberService.GetAllRoles().SingleOrDefault(x => x.Name == roleName); + if (group == null) + { + return Task.FromResult((IdentityRole)null); + } + + return Task.FromResult(new IdentityRole(group.Name) { //TODO: what should the alias be? - Id = @group.Id.ToString() + Id = group.Id.ToString() }); } @@ -523,7 +621,7 @@ namespace Umbraco.Cms.Core.Security { if (user != null) { - //TODO: when to + //TODO: implement //user.SetLoginsCallback(new Lazy>(() => _externalLoginService.GetAll(UserIdToInt(user.Id)))); } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index ed77c63afc..3b78bc1b39 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -162,7 +162,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Name = "testname" }; var fakeCancellationToken = new CancellationToken() { }; - + // act IdentityResult identityResult = await sut.UpdateAsync(fakeRole, fakeCancellationToken); @@ -247,7 +247,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any(x=>x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); _mockMemberGroupService.Verify(x => x.GetById(777)); _mockMemberGroupService.VerifyNoOtherCalls(); } @@ -257,23 +257,23 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { // arrange MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole("fakeGroupName") + var fakeRole = new IdentityRole("fakeGroupName") { Id = "777" }; int fakeRoleId = 777; - IMemberGroup mockMemberGroup = Mock.Of(m => - m.Name == "fakeGroupName" && - m.CreatorId == 123 && - m.Id == 777); + IMemberGroup fakeMemberGroup = new MemberGroup() + { + Name = "fakeGroupName", + CreatorId = 123, + Id = 777 + }; - var fakeCancellationToken = new CancellationToken() { }; - - _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(mockMemberGroup); + _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(fakeMemberGroup); // act - IdentityRole actual = await sut.FindByIdAsync(fakeRole.Id, fakeCancellationToken); + IdentityRole actual = await sut.FindByIdAsync(fakeRole.Id); // assert Assert.AreEqual(fakeRole.Name, actual.Name); @@ -293,7 +293,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Name = "testname" }; var fakeCancellationToken = new CancellationToken() { }; - + // act Task actual = sut.FindByIdAsync(fakeRole.Id, fakeCancellationToken); @@ -342,7 +342,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // act Action actual = () => sut.FindByNameAsync(fakeRole.Name, fakeCancellationToken); - + // assert Assert.That(actual, Throws.ArgumentNullException); _mockMemberGroupService.VerifyNoOtherCalls(); @@ -379,10 +379,6 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // act Task actual = sut.GetRoleIdAsync(fakeRole, fakeCancellationToken); - // assert - Assert.That(actual, Throws.ArgumentNullException); - - // assert Assert.AreEqual(fakeRoleId, actual.Result); } From b1685905e3fb0a0a45b6e6eed00f2ad9418ef960 Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sat, 6 Mar 2021 13:51:41 +0000 Subject: [PATCH 06/25] Added delete tests --- .../Security/MemberIdentityUserStoreTests.cs | 62 ++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs index 8a0bf149b7..68e3f555ab 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs @@ -35,7 +35,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { // arrange MemberUserStore sut = CreateSut(); - CancellationToken fakeCancellationToken = new CancellationToken(){}; + CancellationToken fakeCancellationToken = new CancellationToken() { }; // act Action actual = () => sut.CreateAsync(null, fakeCancellationToken); @@ -73,6 +73,66 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded); Assert.IsTrue(!identityResult.Errors.Any()); + _mockMemberService.Verify(x => x.CreateMember(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())); + _mockMemberService.Verify(x => x.Save(mockMember, It.IsAny())); + } + + + [Test] + public async Task GivenIDeleteUser_AndTheUserIsNotPresent_ThenIShouldGetAFailedResultAsync() + { + // arrange + MemberUserStore sut = CreateSut(); + var fakeUser = new MemberIdentityUser(777); + var fakeCancellationToken = new CancellationToken() { }; + + IMemberType fakeMemberType = new MemberType(new MockShortStringHelper(), 77); + IMember mockMember = Mock.Of(m => + m.Name == "fakeName" && + m.Email == "fakeemail@umbraco.com" && + m.Username == "fakeUsername" && + m.RawPasswordValue == "fakePassword" && + m.ContentTypeAlias == fakeMemberType.Alias && + m.HasIdentity == true); + + // act + Action actual = () => sut.DeleteAsync(fakeUser, fakeCancellationToken); + + // assert + Assert.That(actual, Throws.ArgumentNullException); + _mockMemberService.VerifyNoOtherCalls(); + } + + [Test] + public async Task GivenIDeleteUser_AndTheUserIsDeletedCorrectly_ThenIShouldGetASuccessResultAsync() + { + // arrange + MemberUserStore sut = CreateSut(); + var fakeUser = new MemberIdentityUser(777); + var fakeCancellationToken = new CancellationToken() { }; + + IMemberType fakeMemberType = new MemberType(new MockShortStringHelper(), 77); + IMember mockMember = new Member(fakeMemberType) + { + Id = 777, + Name = "fakeName", + Email = "fakeemail@umbraco.com", + Username = "fakeUsername", + RawPasswordValue = "fakePassword" + }; + + _mockMemberService.Setup(x => x.GetById(mockMember.Id)).Returns(mockMember); + _mockMemberService.Setup(x => x.Delete(mockMember)); + + // act + IdentityResult identityResult = await sut.DeleteAsync(fakeUser, fakeCancellationToken); + + // assert + Assert.IsTrue(identityResult.Succeeded); + Assert.IsTrue(!identityResult.Errors.Any()); + _mockMemberService.Verify(x => x.GetById(mockMember.Id)); + _mockMemberService.Verify(x => x.Delete(mockMember)); + _mockMemberService.VerifyNoOtherCalls(); } } } From df25e6ea531f379d705d307e217537834a317380 Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sat, 6 Mar 2021 13:53:09 +0000 Subject: [PATCH 07/25] Fixed test --- .../Security/MemberIdentityUserStoreTests.cs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs index 68e3f555ab..a56b794ffb 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs @@ -83,20 +83,9 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { // arrange MemberUserStore sut = CreateSut(); - var fakeUser = new MemberIdentityUser(777); - var fakeCancellationToken = new CancellationToken() { }; - - IMemberType fakeMemberType = new MemberType(new MockShortStringHelper(), 77); - IMember mockMember = Mock.Of(m => - m.Name == "fakeName" && - m.Email == "fakeemail@umbraco.com" && - m.Username == "fakeUsername" && - m.RawPasswordValue == "fakePassword" && - m.ContentTypeAlias == fakeMemberType.Alias && - m.HasIdentity == true); // act - Action actual = () => sut.DeleteAsync(fakeUser, fakeCancellationToken); + Action actual = () => sut.DeleteAsync(null); // assert Assert.That(actual, Throws.ArgumentNullException); From fa684222e8c45ab97554b3cb8395c47583a70201 Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sat, 6 Mar 2021 18:57:54 +0000 Subject: [PATCH 08/25] Updated unit tests --- .../Security/MemberUserStore.cs | 11 +- .../Security/MemberIdentityUserStoreTests.cs | 118 +++++++++++++++++- .../Security/MemberRoleStoreTests.cs | 24 ++-- 3 files changed, 131 insertions(+), 22 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 1d871e4134..c0e4373826 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -21,6 +21,7 @@ namespace Umbraco.Cms.Core.Security /// public class MemberUserStore : UserStoreBase, IdentityUserRole, IdentityUserLogin, IdentityUserToken, IdentityRoleClaim> { + private const string genericIdentityErrorCode = "IdentityErrorUserStore"; private readonly IMemberService _memberService; private readonly UmbracoMapper _mapper; private readonly IScopeProvider _scopeProvider; @@ -49,10 +50,10 @@ namespace Umbraco.Cms.Core.Security public override IQueryable Users => throw new NotImplementedException(); /// - public override Task GetNormalizedUserNameAsync(MemberIdentityUser user, CancellationToken cancellationToken) => GetUserNameAsync(user, cancellationToken); + public override Task GetNormalizedUserNameAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) => GetUserNameAsync(user, cancellationToken); /// - public override Task SetNormalizedUserNameAsync(MemberIdentityUser user, string normalizedName, CancellationToken cancellationToken) => SetUserNameAsync(user, normalizedName, cancellationToken); + public override Task SetNormalizedUserNameAsync(MemberIdentityUser user, string normalizedName, CancellationToken cancellationToken = default) => SetUserNameAsync(user, normalizedName, cancellationToken); /// public override Task CreateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) @@ -104,7 +105,7 @@ namespace Umbraco.Cms.Core.Security } catch (Exception ex) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); } } @@ -160,7 +161,7 @@ namespace Umbraco.Cms.Core.Security } catch (Exception ex) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); } } @@ -189,7 +190,7 @@ namespace Umbraco.Cms.Core.Security } catch (Exception ex) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs index a56b794ffb..5d9303b908 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Data; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -31,19 +32,127 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security } [Test] - public void GivenICreateUser_AndTheUserIsNull_ThenIShouldGetAFailedResultAsync() + public void GivenIGetNormalizedUserName_AndTheUserIsNull_ThenIShouldGetAnException() { // arrange MemberUserStore sut = CreateSut(); CancellationToken fakeCancellationToken = new CancellationToken() { }; // act - Action actual = () => sut.CreateAsync(null, fakeCancellationToken); + Action actual = () => sut.GetNormalizedUserNameAsync(null, fakeCancellationToken); // assert Assert.That(actual, Throws.ArgumentNullException); } + [Test] + public async Task GivenIGetNormalizedUserName_AndTheEverythingIsPopulatedCorrectly_ThenIShouldGetACorrectUsername() + { + // arrange + MemberUserStore sut = CreateSut(); + var fakeUser = new MemberIdentityUser() + { + UserName = "fakeuser" + }; + + // act + string actual = await sut.GetNormalizedUserNameAsync(fakeUser); + + // assert + Assert.AreEqual(actual, fakeUser.UserName); + } + + [Test] + public void GivenISetNormalizedUserName_AndTheUserIsNull_ThenIShouldGetAnException() + { + // arrange + MemberUserStore sut = CreateSut(); + CancellationToken fakeCancellationToken = new CancellationToken() { }; + + // act + Action actual = () => sut.SetNormalizedUserNameAsync(null, "username", fakeCancellationToken); + + // assert + Assert.That(actual, Throws.ArgumentNullException); + } + + + [Test] + public void GivenISetNormalizedUserName_AndTheUserNameIsNull_ThenIShouldGetANull() + { + // arrange + MemberUserStore sut = CreateSut(); + CancellationToken fakeCancellationToken = new CancellationToken() { }; + var fakeUser = new MemberIdentityUser() { }; + + // act + Task actual = sut.SetNormalizedUserNameAsync(fakeUser, null, fakeCancellationToken); + + // assert + Assert.AreEqual(null, actual); + } + + [Test] + public void GivenISetNormalizedUserName_AndEverythingIsPopulated_ThenIShouldGetASuccessResult() + { + // arrange + MemberUserStore sut = CreateSut(); + CancellationToken fakeCancellationToken = new CancellationToken() { }; + var fakeUser = new MemberIdentityUser() + { + UserName = "MyName" + }; + + // act + Task actual = sut.SetNormalizedUserNameAsync(fakeUser, "NewName", fakeCancellationToken); + + // assert + Assert.IsTrue(actual.IsCompletedSuccessfully); + } + + [Test] + public async Task GivenICreateUser_AndTheUserIsNull_ThenIShouldGetAFailedResultAsync() + { + // arrange + MemberUserStore sut = CreateSut(); + + // act + IdentityResult actual = await sut.CreateAsync(null); + + // assert + Assert.IsFalse(actual.Succeeded); + Assert.IsTrue(actual.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'user')")); + _mockMemberService.VerifyNoOtherCalls(); + } + + [Test] + public async Task GivenICreateUser_AndTheUserDoesNotHaveIdentity_ThenIShouldGetAFailedResultAsync() + { + // arrange + MemberUserStore sut = CreateSut(); + var fakeUser = new MemberIdentityUser() { }; + var fakeCancellationToken = new CancellationToken() { }; + + IMemberType fakeMemberType = new MemberType(new MockShortStringHelper(), 77); + IMember mockMember = Mock.Of(m => + m.Name == "fakeName" && + m.Email == "fakeemail@umbraco.com" && + m.Username == "fakeUsername" && + m.RawPasswordValue == "fakePassword" && + m.ContentTypeAlias == fakeMemberType.Alias && + m.HasIdentity == false); + + _mockMemberService.Setup(x => x.CreateMember(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())).Returns(mockMember); + _mockMemberService.Setup(x => x.Save(mockMember, It.IsAny())); + + // act + IdentityResult actual = await sut.CreateAsync(null); + + // assert + Assert.IsFalse(actual.Succeeded); + Assert.IsTrue(actual.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'user')")); + _mockMemberService.VerifyNoOtherCalls(); + } [Test] public async Task GivenICreateANewUser_AndTheUserIsPopulatedCorrectly_ThenIShouldGetASuccessResultAsync() @@ -85,10 +194,11 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security MemberUserStore sut = CreateSut(); // act - Action actual = () => sut.DeleteAsync(null); + IdentityResult actual = await sut.DeleteAsync(null); // assert - Assert.That(actual, Throws.ArgumentNullException); + Assert.IsTrue(actual.Succeeded == false); + Assert.IsTrue(actual.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'user')")); _mockMemberService.VerifyNoOtherCalls(); } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index 3b78bc1b39..e6bb0b9ff5 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -26,17 +26,18 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security } [Test] - public void GivenICreateAMemberRole_AndTheGroupIsNull_ThenIShouldGetAnArgumentException() + public void GivenICreateAMemberRole_AndTheGroupIsNull_ThenIShouldGetAFailedIdentityResult() { // arrange MemberRoleStore sut = CreateSut(); CancellationToken fakeCancellationToken = new CancellationToken() { }; // act - Action actual = () => sut.CreateAsync(null, fakeCancellationToken); + Task actual = sut.CreateAsync(null, fakeCancellationToken); // assert - Assert.That(actual, Throws.ArgumentNullException); + Assert.IsTrue(actual.Result.Succeeded == false); + Assert.IsTrue(actual.Result.Errors.Any(x => x.Code == "IdentityMemberGroupNotFound" && x.Description == "Member group not found")); } @@ -139,15 +140,12 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security }; var fakeCancellationToken = new CancellationToken() { }; - bool raiseEvents = false; - - // act IdentityResult identityResult = await sut.UpdateAsync(fakeRole, fakeCancellationToken); // assert Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "IdentityMemberGroupNotFound" && x.Description == "Member group not found")); _mockMemberGroupService.Verify(x => x.GetById(777)); } @@ -168,7 +166,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "DefaultError" && x.Description == "An unknown failure has occurred.")); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "IdentityIdParseError" && x.Description == "Cannot parse ID to int")); _mockMemberGroupService.VerifyNoOtherCalls(); } @@ -221,7 +219,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "DefaultError" && x.Description == "An unknown failure has occurred.")); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "IdentityIdParseError" && x.Description == "Cannot parse ID to int")); _mockMemberGroupService.VerifyNoOtherCalls(); } @@ -247,7 +245,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // assert Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "InvalidRoleName" && x.Description == "Role name 'testname' is invalid.")); + Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "IdentityMemberGroupNotFound" && x.Description == "Member group not found")); _mockMemberGroupService.Verify(x => x.GetById(777)); _mockMemberGroupService.VerifyNoOtherCalls(); } @@ -283,7 +281,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security } [Test] - public void GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync() + public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync() { // arrange MemberRoleStore sut = CreateSut(); @@ -295,10 +293,10 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security var fakeCancellationToken = new CancellationToken() { }; // act - Task actual = sut.FindByIdAsync(fakeRole.Id, fakeCancellationToken); + Action actual = () => sut.FindByIdAsync(fakeRole.Id, fakeCancellationToken); // assert - Assert.IsNull(actual); + Assert.That(actual, Throws.TypeOf()); _mockMemberGroupService.VerifyNoOtherCalls(); } From 4e9423687cceefc40b3732885240dc937c6ffd9c Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sat, 6 Mar 2021 23:09:19 +0000 Subject: [PATCH 09/25] Changed controller to use role manager. Updated mapping. Don't commit test database. --- .gitignore | 1 + .../Security/MemberRoleStore.cs | 17 +++- .../Security/MemberRoleStoreTests.cs | 73 ++++++++++++++- .../Controllers/MemberController.cs | 1 + .../Controllers/MemberGroupController.cs | 90 +++++++++++++------ .../Mapping/MemberMapDefinition.cs | 16 ++++ 6 files changed, 164 insertions(+), 34 deletions(-) diff --git a/.gitignore b/.gitignore index f8a2ce511f..8aa2769368 100644 --- a/.gitignore +++ b/.gitignore @@ -202,3 +202,4 @@ src/Umbraco.Tests/TEMP/ /src/Umbraco.Web.UI/config/umbracoSettings.config /src/Umbraco.Web.UI.NetCore/Umbraco/models/* src/Umbraco.Tests.UnitTests/umbraco/Data/TEMP/ +src/Umbraco.Tests.Integration/DatabaseContextTests.sdf diff --git a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index 4477c4870c..466b2435f6 100644 --- a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -246,12 +246,25 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(roleId)); } + IMemberGroup memberGroup; + + // member group can be found by int or Guid, so try both if (!int.TryParse(roleId, out int id)) { - throw new ArgumentOutOfRangeException(nameof(roleId), $"{nameof(roleId)} is not a valid Int"); + if (!Guid.TryParse(roleId, out Guid guid)) + { + throw new ArgumentOutOfRangeException(nameof(roleId), $"{nameof(roleId)} is not a valid Guid"); + } + else + { + memberGroup = _memberGroupService.GetById(guid); + } + } + else + { + memberGroup = _memberGroupService.GetById(id); } - IMemberGroup memberGroup = _memberGroupService.GetById(id); return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index e6bb0b9ff5..d89bda494c 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -251,7 +251,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security } [Test] - public async Task GivenIFindAMemberRoleByRoleId_AndRoleIdExists_ThenIShouldGetASuccessResultAsync() + public async Task GivenIFindAMemberRoleByRoleKey_AndRoleKeyExists_ThenIShouldGetASuccessResultAsync() { // arrange MemberRoleStore sut = CreateSut(); @@ -265,7 +265,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { Name = "fakeGroupName", CreatorId = 123, - Id = 777 + Id = 777, + Key = Guid.NewGuid() }; _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(fakeMemberGroup); @@ -281,7 +282,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security } [Test] - public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync() + public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnIntOrGuid_ThenIShouldGetAFailureResultAsync() { // arrange MemberRoleStore sut = CreateSut(); @@ -300,6 +301,72 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security _mockMemberGroupService.VerifyNoOtherCalls(); } + [Test] + public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnIntButCanBeToGuid_ThenIShouldGetASuccessResultAsync() + { + // arrange + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole("fakeGroupName") + { + Id = "777" + }; + + var fakeRoleGuid = Guid.NewGuid(); + + IMemberGroup fakeMemberGroup = new MemberGroup() + { + Name = "fakeGroupName", + CreatorId = 123, + Id = 777, + Key = fakeRoleGuid + }; + + _mockMemberGroupService.Setup(x => x.GetById(fakeRoleGuid)).Returns(fakeMemberGroup); + + // act + IdentityRole actual = await sut.FindByIdAsync(fakeRoleGuid.ToString()); + + // assert + Assert.AreEqual(fakeRole.Name, actual.Name); + Assert.AreEqual(fakeRole.Id, actual.Id); + _mockMemberGroupService.Verify(x => x.GetById(fakeRoleGuid)); + _mockMemberGroupService.VerifyNoOtherCalls(); + } + + + [Test] + public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAGuidButCanBeToInt_ThenIShouldGetASuccessResultAsync() + { + // arrange + MemberRoleStore sut = CreateSut(); + var fakeRole = new IdentityRole("fakeGroupName") + { + Id = "777" + }; + + var fakeRoleId = 777; + + IMemberGroup fakeMemberGroup = new MemberGroup() + { + Name = "fakeGroupName", + CreatorId = 123, + Id = 777, + Key = Guid.NewGuid() + }; + + _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(fakeMemberGroup); + + // act + IdentityRole actual = await sut.FindByIdAsync(fakeRoleId.ToString()); + + // assert + Assert.AreEqual(fakeRole.Name, actual.Name); + Assert.AreEqual(fakeRole.Id, actual.Id); + _mockMemberGroupService.Verify(x => x.GetById(fakeRoleId)); + _mockMemberGroupService.VerifyNoOtherCalls(); + } + + [Test] public async Task GivenIFindAMemberRoleByRoleName_AndRoleNameExists_ThenIShouldGetASuccessResultAsync() { diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs index b5f8674f9e..4a24cf1377 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs @@ -190,6 +190,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers [OutgoingEditorModelEvent] public MemberDisplay GetByKey(Guid key) { + //TODO: convert to identity IMember foundMember = _memberService.GetByKey(key); if (foundMember == null) { diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs index df76736de9..f083f1e1b5 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs @@ -48,15 +48,23 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// /// - public ActionResult GetById(int id) + public async Task> GetById(int id) { - IdentityRole memberGroup = _roleManager.FindByIdAsync(id.ToString()).Result; + //TODO: did we envisage this - combination of service and identity manager? + IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString()); + if (identityRole == null) + { + return NotFound(); + } + + IMemberGroup memberGroup = _memberGroupService.GetById(id); if (memberGroup == null) { return NotFound(); } - MemberGroupDisplay dto = _umbracoMapper.Map, MemberGroupDisplay>(memberGroup); + //TODO: the default identity role doesn't have all the properties IMemberGroup had, e.g. CreatorId + MemberGroupDisplay dto = _umbracoMapper.Map(memberGroup); return dto; } @@ -66,15 +74,21 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// /// - public ActionResult GetById(Guid id) + public async Task> GetById(Guid id) { - IMemberGroup memberGroup = _memberGroupService.GetById(id); - if (memberGroup == null) + //TODO: did we envisage just identity or a combination of service and identity manager? + IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString()); + if (identityRole == null) { return NotFound(); } + //IMemberGroup memberGroup = _memberGroupService.GetById(id); + //if (memberGroup == null) + //{ + // return NotFound(); + //} - return _umbracoMapper.Map(memberGroup); + return _umbracoMapper.Map(identityRole); } /// @@ -82,7 +96,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// /// - public ActionResult GetById(Udi id) + public async Task> GetById(Udi id) { var guidUdi = id as GuidUdi; if (guidUdi == null) @@ -90,43 +104,53 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return NotFound(); } - IMemberGroup memberGroup = _memberGroupService.GetById(guidUdi.Guid); - if (memberGroup == null) + //TODO: can we do this via identity? + IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString()); + if (identityRole == null) { return NotFound(); } - return _umbracoMapper.Map(memberGroup); + return _umbracoMapper.Map(identityRole); } - public IEnumerable GetByIds([FromQuery]int[] ids) + public async Task> GetByIds([FromQuery]int[] ids) { - var roles = new List>(); + var roles = new List(); foreach (int id in ids) { - Task role = _roleManager.FindByIdAsync(id.ToString()); - roles.Add(role.Result); + IdentityRole role = await _roleManager.FindByIdAsync(id.ToString()); + roles.Add(role); } - return roles.Select(x=> _umbracoMapper.Map, MemberGroupDisplay>(x)); + return roles.Select(x=> _umbracoMapper.Map(x)); } [HttpDelete] [HttpPost] - public IActionResult DeleteById(int id) + public async Task DeleteById(int id) { - IMemberGroup memberGroup = _memberGroupService.GetById(id); - if (memberGroup == null) + //TODO: are there any repercussions elsewhere for us changing these to async? + IdentityRole role = await _roleManager.FindByIdAsync(id.ToString()); + + if (role == null) { return NotFound(); } - _memberGroupService.Delete(memberGroup); - return Ok(); + IdentityResult roleDeleted = await _roleManager.DeleteAsync(role); + if (roleDeleted.Succeeded) + { + return Ok(); + } + else + { + return Problem("Issue during deletion - please see logs"); + } } - public IEnumerable GetAllGroups() => _roleManager.Roles.Select(x => _umbracoMapper.Map, MemberGroupDisplay>(x)); + public IEnumerable GetAllGroups() => _roleManager.Roles.Select(x => _umbracoMapper.Map(x)); public MemberGroupDisplay GetEmpty() { @@ -134,20 +158,28 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return _umbracoMapper.Map(item); } - public ActionResult PostSave(MemberGroupSave saveModel) + public async Task> PostSave(MemberGroupSave saveModel) { + int id = int.Parse(saveModel.Id.ToString()); - var id = int.Parse(saveModel.Id.ToString()); - var memberGroup = id > 0 ? _memberGroupService.GetById(id) : new MemberGroup(); - if (memberGroup == null) + IdentityRole role = id > 0 ? await _roleManager.FindByIdAsync(saveModel.Id.ToString()) : null; + + if (role == null) { return NotFound(); } - memberGroup.Name = saveModel.Name; - _memberGroupService.Save(memberGroup); + role.Name = saveModel.Name; + IdentityResult updatedResult = await _roleManager.UpdateAsync(role); - var display = _umbracoMapper.Map(memberGroup); + if (!updatedResult.Succeeded) + { + //TODO: what to retrun if there is a failed identity result + return Problem(); + } + + //TODO: should we return the identity role or return the group from the service? + MemberGroupDisplay display = _umbracoMapper.Map(role); display.AddSuccessNotification( _localizedTextService.Localize("speechBubbles/memberGroupSavedHeader"), diff --git a/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs b/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs index e88503794e..7ee456a323 100644 --- a/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs +++ b/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs @@ -1,4 +1,6 @@ +using System; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Identity; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Mapping; using Umbraco.Cms.Core.Models; @@ -33,6 +35,7 @@ namespace Umbraco.Cms.Web.BackOffice.Mapping mapper.Define((source, context) => new MemberDisplay(), Map); mapper.Define((source, context) => new MemberBasic(), Map); mapper.Define((source, context) => new MemberGroupDisplay(), Map); + mapper.Define((source, context) => new MemberGroupDisplay(), Map); mapper.Define((source, context) => new ContentPropertyCollectionDto(), Map); } @@ -100,5 +103,18 @@ namespace Umbraco.Cms.Web.BackOffice.Mapping { target.Properties = context.MapEnumerable(source.Properties); } + + /// + /// Maps an identity role to a member group display + /// + /// + /// + /// + private void Map(IdentityRole source, MemberGroupDisplay target, MapperContext context) + { + //TODO: this is all that is mapped at this time, we're losing a lot of properties + target.Id = source.Id; + target.Name = source.Name; + } } } From 5051f8160ff1955ecb19c4c5c73d6ffb8d8316b9 Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sat, 6 Mar 2021 23:13:15 +0000 Subject: [PATCH 10/25] Remove unecessary check for int --- src/Umbraco.Infrastructure/Security/MemberUserStore.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index c0e4373826..6be981b3d1 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -208,11 +208,6 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(userId)); } - if (!Guid.TryParse(userId, out Guid id)) - { - throw new ArgumentOutOfRangeException(nameof(userId), $"{nameof(userId)} is not a valid x"); - } - IMember user = _memberService.GetById(UserIdToInt(userId)); if (user == null) { From 401f713ca522c01a08714089f45733164bcc8538 Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sun, 7 Mar 2021 15:21:49 +0000 Subject: [PATCH 11/25] Updated logic in member group controller to ensure group is saved when new --- .../Controllers/MemberGroupController.cs | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs index f083f1e1b5..cc712ca7c4 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs @@ -29,7 +29,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers private readonly UmbracoMapper _umbracoMapper; private readonly ILocalizedTextService _localizedTextService; private readonly RoleManager _roleManager; - + public MemberGroupController( IMemberGroupService memberGroupService, UmbracoMapper umbracoMapper, @@ -114,7 +114,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return _umbracoMapper.Map(identityRole); } - public async Task> GetByIds([FromQuery]int[] ids) + public async Task> GetByIds([FromQuery] int[] ids) { var roles = new List(); @@ -124,7 +124,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers roles.Add(role); } - return roles.Select(x=> _umbracoMapper.Map(x)); + return roles.Select(x => _umbracoMapper.Map(x)); } [HttpDelete] @@ -158,25 +158,39 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return _umbracoMapper.Map(item); } + /// + /// Saves the member group via the identity role + /// If new, creates a new role, else updates the existing role + /// + /// + /// public async Task> PostSave(MemberGroupSave saveModel) { int id = int.Parse(saveModel.Id.ToString()); - IdentityRole role = id > 0 ? await _roleManager.FindByIdAsync(saveModel.Id.ToString()) : null; - - if (role == null) + IdentityRole role; + if (id > 0) { - return NotFound(); + role = await _roleManager.FindByIdAsync(saveModel.Id.ToString()); + role.Name = saveModel.Name; + IdentityResult updatedResult = await _roleManager.UpdateAsync(role); + if (!updatedResult.Succeeded) + { + //TODO: what to retrun if there is a failed identity result + return Problem(); + } + } + else + { + role = new IdentityRole(saveModel.Name); + IdentityResult updatedResult = await _roleManager.CreateAsync(role); } - role.Name = saveModel.Name; - IdentityResult updatedResult = await _roleManager.UpdateAsync(role); - - if (!updatedResult.Succeeded) - { - //TODO: what to retrun if there is a failed identity result - return Problem(); - } + //TODO: do we need to refetch the role? + //if (role == null) + //{ + // return NotFound(); + //} //TODO: should we return the identity role or return the group from the service? MemberGroupDisplay display = _umbracoMapper.Map(role); From a53688c5030a116ae5e6075d81c3d64b8afafdea Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sun, 7 Mar 2021 15:58:11 +0000 Subject: [PATCH 12/25] Fixed logic for groups. --- .../Controllers/MemberGroupController.cs | 88 +++++++++++-------- 1 file changed, 53 insertions(+), 35 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs index cc712ca7c4..f90bd6458c 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs @@ -43,6 +43,8 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers _localizedTextService = localizedTextService ?? throw new ArgumentNullException(nameof(localizedTextService)); } + //TODO: are there any repercussions elsewhere for us changing these to async? + /// /// Gets the member group json for the member group id /// @@ -68,7 +70,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return dto; } - /// /// Gets the member group json for the member group guid /// @@ -77,18 +78,18 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers public async Task> GetById(Guid id) { //TODO: did we envisage just identity or a combination of service and identity manager? - IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString()); - if (identityRole == null) - { - return NotFound(); - } - //IMemberGroup memberGroup = _memberGroupService.GetById(id); - //if (memberGroup == null) + //IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString()); + //if (identityRole == null) //{ // return NotFound(); //} + IMemberGroup memberGroup = _memberGroupService.GetById(id); + if (memberGroup == null) + { + return NotFound(); + } - return _umbracoMapper.Map(identityRole); + return _umbracoMapper.Map(memberGroup); } /// @@ -111,27 +112,36 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return NotFound(); } - return _umbracoMapper.Map(identityRole); - } - - public async Task> GetByIds([FromQuery] int[] ids) - { - var roles = new List(); - - foreach (int id in ids) + IMemberGroup memberGroup = _memberGroupService.GetById(guidUdi.Guid); + if (memberGroup == null) { - IdentityRole role = await _roleManager.FindByIdAsync(id.ToString()); - roles.Add(role); + return NotFound(); } - return roles.Select(x => _umbracoMapper.Map(x)); + return _umbracoMapper.Map(memberGroup); + } + + public IEnumerable GetByIds([FromQuery] int[] ids) + { + //var roles = new List(); + + //foreach (int id in ids) + //{ + // IdentityRole role = await _roleManager.FindByIdAsync(id.ToString()); + // roles.Add(role); + //} + + //return roles.Select(x => _umbracoMapper.Map(x)); + //TODO: does this need to be done via identity? + + return _memberGroupService.GetByIds(ids) + .Select(_umbracoMapper.Map); } [HttpDelete] [HttpPost] public async Task DeleteById(int id) { - //TODO: are there any repercussions elsewhere for us changing these to async? IdentityRole role = await _roleManager.FindByIdAsync(id.ToString()); if (role == null) @@ -150,7 +160,9 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers } } - public IEnumerable GetAllGroups() => _roleManager.Roles.Select(x => _umbracoMapper.Map(x)); + //TODO: we don't currently implement IQueryableRoleStore, still using original service + //public IEnumerable GetAllGroups() => _roleManager.Roles.Select(x => _umbracoMapper.Map(x)); + public IEnumerable GetAllGroups() => _memberGroupService.GetAll().Select(x => _umbracoMapper.Map(x)); public MemberGroupDisplay GetEmpty() { @@ -169,31 +181,37 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers int id = int.Parse(saveModel.Id.ToString()); IdentityRole role; + IdentityResult updatedResult; if (id > 0) { role = await _roleManager.FindByIdAsync(saveModel.Id.ToString()); role.Name = saveModel.Name; - IdentityResult updatedResult = await _roleManager.UpdateAsync(role); - if (!updatedResult.Succeeded) - { - //TODO: what to retrun if there is a failed identity result - return Problem(); - } + updatedResult = await _roleManager.UpdateAsync(role); + } else { role = new IdentityRole(saveModel.Name); - IdentityResult updatedResult = await _roleManager.CreateAsync(role); + updatedResult = await _roleManager.CreateAsync(role); } - //TODO: do we need to refetch the role? - //if (role == null) - //{ - // return NotFound(); - //} + if (!updatedResult.Succeeded) + { + //TODO: what to return if there is a failed identity result + return Problem(); + } + + //TODO: do we need to refetch the member group? + int roleId = int.Parse(role.Id); + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + + if (memberGroup == null) + { + return NotFound(); + } //TODO: should we return the identity role or return the group from the service? - MemberGroupDisplay display = _umbracoMapper.Map(role); + MemberGroupDisplay display = _umbracoMapper.Map(memberGroup); display.AddSuccessNotification( _localizedTextService.Localize("speechBubbles/memberGroupSavedHeader"), From 1df78c1321fa31cec091240d63e77f2af2e087f0 Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Mon, 8 Mar 2021 17:12:30 +0000 Subject: [PATCH 13/25] Updates to unit store tests and managing null username in members user store --- .../Security/MemberRoleStore.cs | 3 ++- .../Security/MemberUserStore.cs | 25 ++++++++++++++++++- .../Security/MemberIdentityUserStoreTests.cs | 14 +++++++---- .../Security/MemberRoleStoreTests.cs | 6 ++--- 4 files changed, 38 insertions(+), 10 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index 466b2435f6..9b0618e8dd 100644 --- a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -19,6 +19,7 @@ namespace Umbraco.Cms.Core.Security //TODO: How revealing can the error messages be? private readonly IdentityError _intParseError = new IdentityError { Code = "IdentityIdParseError", Description = "Cannot parse ID to int" }; private readonly IdentityError _memberGroupNotFoundError = new IdentityError { Code = "IdentityMemberGroupNotFound", Description = "Member group not found" }; + private const string genericIdentityErrorCode = "IdentityErrorUserStore"; public MemberRoleStore(IMemberGroupService memberGroupService, IdentityErrorDescriber errorDescriber) { @@ -57,7 +58,7 @@ namespace Umbraco.Cms.Core.Security } catch (Exception ex) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); } } diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 6be981b3d1..233f053333 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -53,7 +53,30 @@ namespace Umbraco.Cms.Core.Security public override Task GetNormalizedUserNameAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) => GetUserNameAsync(user, cancellationToken); /// - public override Task SetNormalizedUserNameAsync(MemberIdentityUser user, string normalizedName, CancellationToken cancellationToken = default) => SetUserNameAsync(user, normalizedName, cancellationToken); + public override Task SetNormalizedUserNameAsync(MemberIdentityUser user, string normalizedName, + CancellationToken cancellationToken = default) + { + try + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } + + if (normalizedName == null) + { + throw new ArgumentNullException(nameof(normalizedName)); + } + + return SetUserNameAsync(user, normalizedName, cancellationToken); + } + catch (Exception ex) + { + return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); + } + } /// public override Task CreateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs index 5d9303b908..de8ba1e36b 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs @@ -67,13 +67,15 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { // arrange MemberUserStore sut = CreateSut(); - CancellationToken fakeCancellationToken = new CancellationToken() { }; + var fakeCancellationToken = new CancellationToken() { }; // act - Action actual = () => sut.SetNormalizedUserNameAsync(null, "username", fakeCancellationToken); + var actual = (Task)sut.SetNormalizedUserNameAsync(null, "username", fakeCancellationToken); // assert - Assert.That(actual, Throws.ArgumentNullException); + Assert.IsFalse(actual.Result.Succeeded); + Assert.IsTrue(actual.Result.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'user')")); + _mockMemberService.VerifyNoOtherCalls(); } @@ -86,10 +88,12 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security var fakeUser = new MemberIdentityUser() { }; // act - Task actual = sut.SetNormalizedUserNameAsync(fakeUser, null, fakeCancellationToken); + var actual = (Task)sut.SetNormalizedUserNameAsync(fakeUser, null, fakeCancellationToken); // assert - Assert.AreEqual(null, actual); + Assert.IsFalse(actual.Result.Succeeded); + Assert.IsTrue(actual.Result.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'normalizedName')")); + _mockMemberService.VerifyNoOtherCalls(); } [Test] diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index d89bda494c..2c3cb901fc 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -30,14 +30,14 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { // arrange MemberRoleStore sut = CreateSut(); - CancellationToken fakeCancellationToken = new CancellationToken() { }; + var fakeCancellationToken = new CancellationToken(); // act Task actual = sut.CreateAsync(null, fakeCancellationToken); // assert - Assert.IsTrue(actual.Result.Succeeded == false); - Assert.IsTrue(actual.Result.Errors.Any(x => x.Code == "IdentityMemberGroupNotFound" && x.Description == "Member group not found")); + Assert.IsFalse(actual.Result.Succeeded); + Assert.IsTrue(actual.Result.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'role')")); } From 1a7bc6aa757cd2002f3570be1b2ab0f7f16644fe Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Mon, 8 Mar 2021 17:26:53 +0000 Subject: [PATCH 14/25] Corrected formatting and removed unwanted commented out code --- .../ServiceCollectionExtensions.cs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs index b123a659c5..1ff044ed3f 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs @@ -63,24 +63,15 @@ namespace Umbraco.Extensions /// /// Adds the services required for using Members Identity /// - public static void AddMembersIdentity(this IServiceCollection services) - { + public static void AddMembersIdentity(this IServiceCollection services) => services.BuildMembersIdentity() .AddDefaultTokenProviders() .AddMemberManager() - //.AddRoles() .AddUserStore() .AddRoleStore>() .AddRoleValidator>() .AddRoleManager>(); - //services.AddScoped>( - // s => new UserClaimsPrincipalFactory( - // s.GetService(), - // s.GetService>(), - // s.GetService>())); - } - private static MemberIdentityBuilder BuildMembersIdentity(this IServiceCollection services) { // Services used by Umbraco members identity From 4873ecc0f90b2c0da9c74df94d2e9564604b351a Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Tue, 9 Mar 2021 10:13:44 +0000 Subject: [PATCH 15/25] Deleted unwanted extra method --- .../Services/Implement/MemberGroupService.cs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/Umbraco.Infrastructure/Services/Implement/MemberGroupService.cs b/src/Umbraco.Infrastructure/Services/Implement/MemberGroupService.cs index 1f6d4743be..9024e3c4e1 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/MemberGroupService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/MemberGroupService.cs @@ -130,25 +130,6 @@ namespace Umbraco.Cms.Core.Services.Implement } } - - public void GetByRole(IMemberGroup memberGroup) - { - using (var scope = ScopeProvider.CreateScope()) - { - var deleteEventArgs = new DeleteEventArgs(memberGroup); - if (scope.Events.DispatchCancelable(Deleting, this, deleteEventArgs)) - { - scope.Complete(); - return; - } - - _memberGroupRepository.Delete(memberGroup); - scope.Complete(); - deleteEventArgs.CanCancel = false; - scope.Events.Dispatch(Deleted, this, deleteEventArgs); - } - } - /// /// Occurs before Delete of a member group /// From 9e867eeeefddff13fa0295ecc8e2d2a711aeeb73 Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Fri, 12 Mar 2021 15:05:50 +0000 Subject: [PATCH 16/25] Changes as per PR comments from Scott Brady --- .../Security/MemberRoleStore.cs | 36 +++---------------- .../Controllers/ContentController.cs | 2 +- .../Umbraco.Web.Common.csproj | 1 - 3 files changed, 5 insertions(+), 34 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index 9b0618e8dd..d76f3b5c7b 100644 --- a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -69,13 +69,13 @@ namespace Umbraco.Cms.Core.Security try { cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + if (role == null) { throw new ArgumentNullException(nameof(role)); } - ThrowIfDisposed(); - if (!int.TryParse(role.Id, out int roleId)) { return Task.FromResult(IdentityResult.Failed(_intParseError)); @@ -205,36 +205,10 @@ namespace Umbraco.Cms.Core.Security } /// - public Task GetNormalizedRoleNameAsync(TRole role, CancellationToken cancellationToken = default) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - - if (role == null) - { - throw new ArgumentNullException(nameof(role)); - } - - //TODO: are we utilising NormalizedRoleName? - return Task.FromResult(role.NormalizedName); - } + public Task GetNormalizedRoleNameAsync(TRole role, CancellationToken cancellationToken = default) => GetRoleNameAsync(role, cancellationToken); /// - public Task SetNormalizedRoleNameAsync(TRole role, string normalizedName, CancellationToken cancellationToken = default) - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - - if (role == null) - { - throw new ArgumentNullException(nameof(role)); - } - - //TODO: are we utilising NormalizedRoleName and do we need to set it in the memberGroupService? - role.NormalizedName = normalizedName; - - return Task.CompletedTask; - } + public Task SetNormalizedRoleNameAsync(TRole role, string normalizedName, CancellationToken cancellationToken = default) => SetRoleNameAsync(role, normalizedName, cancellationToken); /// public Task FindByIdAsync(string roleId, CancellationToken cancellationToken = default) @@ -280,8 +254,6 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(name)); } IMemberGroup memberGroup = _memberGroupService.GetByName(name); - //TODO: throw exception when not found? - return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup)); } diff --git a/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs b/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs index 00f2fff56c..c4c601593f 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs @@ -2435,7 +2435,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers .Select(_umbracoMapper.Map) .ToArray(); - //TODO: change to role store + //TODO: change to role manager var allGroups = _memberGroupService.GetAll().ToArray(); var groups = entry.Rules .Where(rule => rule.RuleType == Constants.Conventions.PublicAccess.MemberRoleRuleType) diff --git a/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj b/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj index 0fe68d4cc9..738a9389dc 100644 --- a/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj +++ b/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj @@ -24,7 +24,6 @@ - From 8535a2915c05a4b4ab084e3d23a85ee16ff1422c Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Fri, 12 Mar 2021 15:21:56 +0000 Subject: [PATCH 17/25] Changed wording, PR feedback --- .../Mapping/MemberTabsAndPropertiesMapper.cs | 2 +- .../Security/MemberUserStore.cs | 25 +------------------ 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs b/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs index 9ac1b05d0f..02ee4fe744 100644 --- a/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs +++ b/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs @@ -237,7 +237,7 @@ namespace Umbraco.Cms.Core.Models.Mapping var userRoles = username.IsNullOrWhiteSpace() ? null : _memberService.GetAllRoles(username); // create a dictionary of all roles (except internal roles) + "false" - //TODO: use MembersRoleStore + //TODO: use member role manager instead var result = _memberGroupService.GetAll() .Select(x => x.Name) // if a role starts with __umbracoRole we won't show it as it's an internal role used for public access diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 233f053333..6be981b3d1 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -53,30 +53,7 @@ namespace Umbraco.Cms.Core.Security public override Task GetNormalizedUserNameAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) => GetUserNameAsync(user, cancellationToken); /// - public override Task SetNormalizedUserNameAsync(MemberIdentityUser user, string normalizedName, - CancellationToken cancellationToken = default) - { - try - { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - if (user == null) - { - throw new ArgumentNullException(nameof(user)); - } - - if (normalizedName == null) - { - throw new ArgumentNullException(nameof(normalizedName)); - } - - return SetUserNameAsync(user, normalizedName, cancellationToken); - } - catch (Exception ex) - { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); - } - } + public override Task SetNormalizedUserNameAsync(MemberIdentityUser user, string normalizedName, CancellationToken cancellationToken = default) => SetUserNameAsync(user, normalizedName, cancellationToken); /// public override Task CreateAsync(MemberIdentityUser user, CancellationToken cancellationToken = default) From dccce92cb33cb90e8ce915931daac630107e73d4 Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sun, 14 Mar 2021 15:14:53 +0000 Subject: [PATCH 18/25] Updated the user and roles to throw exceptions instead of being caught, as per PR feedback --- .../Security/MemberRoleStore.cs | 143 ++++++++---------- .../Security/MemberRoleStoreTests.cs | 22 ++- ...rStoreTests.cs => MemberUserStoreTests.cs} | 14 +- 3 files changed, 84 insertions(+), 95 deletions(-) rename src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/{MemberIdentityUserStoreTests.cs => MemberUserStoreTests.cs} (92%) diff --git a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index d76f3b5c7b..8f4bd0cd17 100644 --- a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -35,110 +35,89 @@ namespace Umbraco.Cms.Core.Security /// public Task CreateAsync(TRole role, CancellationToken cancellationToken = default) { - try + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (role == null) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - - if (role == null) - { - throw new ArgumentNullException(nameof(role)); - } - - var memberGroup = new MemberGroup - { - Name = role.Name - }; - - _memberGroupService.Save(memberGroup); - - role.Id = memberGroup.Id.ToString(); - - return Task.FromResult(IdentityResult.Success); + throw new ArgumentNullException(nameof(role)); } - catch (Exception ex) + + var memberGroup = new MemberGroup { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = genericIdentityErrorCode, Description = ex.Message })); - } + Name = role.Name + }; + + _memberGroupService.Save(memberGroup); + + role.Id = memberGroup.Id.ToString(); + + return Task.FromResult(IdentityResult.Success); } /// public Task UpdateAsync(TRole role, CancellationToken cancellationToken = default) { - try + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (role == null) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - - if (role == null) - { - throw new ArgumentNullException(nameof(role)); - } - - if (!int.TryParse(role.Id, out int roleId)) - { - return Task.FromResult(IdentityResult.Failed(_intParseError)); - } - - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - if (memberGroup != null) - { - if (MapToMemberGroup(role, memberGroup)) - { - _memberGroupService.Save(memberGroup); - } - //TODO: if nothing changed, do we need to report this? - return Task.FromResult(IdentityResult.Success); - } - else - { - //TODO: throw exception when not found, or return failure? - return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); - } - + throw new ArgumentNullException(nameof(role)); } - catch (Exception ex) + + if (!int.TryParse(role.Id, out int roleId)) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + return Task.FromResult(IdentityResult.Failed(_intParseError)); } + + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + if (MapToMemberGroup(role, memberGroup)) + { + _memberGroupService.Save(memberGroup); + } + //TODO: if nothing changed, do we need to report this? + return Task.FromResult(IdentityResult.Success); + } + else + { + //TODO: throw exception when not found, or return failure? + return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); + } + } /// public Task DeleteAsync(TRole role, CancellationToken cancellationToken = default) { - try + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (role == null) { - cancellationToken.ThrowIfCancellationRequested(); - ThrowIfDisposed(); - - if (role == null) - { - throw new ArgumentNullException(nameof(role)); - } - - if (!int.TryParse(role.Id, out int roleId)) - { - //TODO: what identity error should we return in this case? - return Task.FromResult(IdentityResult.Failed(_intParseError)); - } - - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - if (memberGroup != null) - { - _memberGroupService.Delete(memberGroup); - } - else - { - return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); - } - - return Task.FromResult(IdentityResult.Success); + throw new ArgumentNullException(nameof(role)); } - catch (Exception ex) + + if (!int.TryParse(role.Id, out int roleId)) { - return Task.FromResult(IdentityResult.Failed(new IdentityError { Code = ex.Message, Description = ex.Message })); + //TODO: what identity error should we return in this case? + return Task.FromResult(IdentityResult.Failed(_intParseError)); } + + IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + if (memberGroup != null) + { + _memberGroupService.Delete(memberGroup); + } + else + { + return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); + } + + return Task.FromResult(IdentityResult.Success); } /// diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index 2c3cb901fc..a6c534d24d 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -33,14 +33,13 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security var fakeCancellationToken = new CancellationToken(); // act - Task actual = sut.CreateAsync(null, fakeCancellationToken); + Action actual = () => sut.CreateAsync(null, fakeCancellationToken); // assert - Assert.IsFalse(actual.Result.Succeeded); - Assert.IsTrue(actual.Result.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'role')")); + Assert.That(actual, Throws.ArgumentNullException); + _mockMemberGroupService.VerifyNoOtherCalls(); } - [Test] public async Task GivenICreateAMemberRole_AndTheGroupIsPopulatedCorrectly_ThenIShouldGetASuccessResultAsync() { @@ -170,6 +169,21 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security _mockMemberGroupService.VerifyNoOtherCalls(); } + [Test] + public void GivenIUpdateAMemberRole_AndTheRoleIsNull_ThenAnExceptionShouldBeThrown() + { + // arrange + MemberRoleStore sut = CreateSut(); + var fakeCancellationToken = new CancellationToken() { }; + + // act + Action actual = () => sut.UpdateAsync(null, fakeCancellationToken); + + // assert + Assert.That(actual, Throws.ArgumentNullException); + _mockMemberGroupService.VerifyNoOtherCalls(); + } + [Test] public async Task GivenIDeleteAMemberRole_AndItExists_ThenTheMemberGroupShouldBeDeleted_AndIShouldGetASuccessResultAsync() { diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs similarity index 92% rename from src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs rename to src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs index de8ba1e36b..7f80ff9382 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberIdentityUserStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Data; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -17,7 +16,7 @@ using Umbraco.Cms.Tests.UnitTests.Umbraco.Core.ShortStringHelper; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { [TestFixture] - public class MemberIdentityUserStoreTests + public class MemberUserStoreTests { private Mock _mockMemberService; @@ -70,17 +69,16 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security var fakeCancellationToken = new CancellationToken() { }; // act - var actual = (Task)sut.SetNormalizedUserNameAsync(null, "username", fakeCancellationToken); + Action actual = () => sut.SetNormalizedUserNameAsync(null, "username", fakeCancellationToken); // assert - Assert.IsFalse(actual.Result.Succeeded); - Assert.IsTrue(actual.Result.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'user')")); + Assert.That(actual, Throws.ArgumentNullException); _mockMemberService.VerifyNoOtherCalls(); } [Test] - public void GivenISetNormalizedUserName_AndTheUserNameIsNull_ThenIShouldGetANull() + public void GivenISetNormalizedUserName_AndTheUserNameIsNull_ThenAnExceptionShouldBeThrown() { // arrange MemberUserStore sut = CreateSut(); @@ -88,11 +86,9 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security var fakeUser = new MemberIdentityUser() { }; // act - var actual = (Task)sut.SetNormalizedUserNameAsync(fakeUser, null, fakeCancellationToken); + Action actual = () => sut.SetNormalizedUserNameAsync(fakeUser, null, fakeCancellationToken); // assert - Assert.IsFalse(actual.Result.Succeeded); - Assert.IsTrue(actual.Result.Errors.Any(x => x.Code == "IdentityErrorUserStore" && x.Description == "Value cannot be null. (Parameter 'normalizedName')")); _mockMemberService.VerifyNoOtherCalls(); } From ed931a96294cf7fc48fb1c57bb30391707f8222f Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 18 Mar 2021 08:26:28 +0100 Subject: [PATCH 19/25] Fix build --- src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj | 6 ------ src/Umbraco.Web.Common/Umbraco.Web.Common.csproj | 2 -- 2 files changed, 8 deletions(-) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj b/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj index 1a7788591d..1bd1600721 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj @@ -37,10 +37,4 @@ - - - ..\Umbraco.Web\bin\Debug\Umbraco.Infrastructure.dll - - - diff --git a/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj b/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj index 738a9389dc..dce3ad7336 100644 --- a/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj +++ b/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj @@ -24,8 +24,6 @@ - - From 7193486b77333a14a94899ec5b35b7925f584e71 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 18 Mar 2021 08:38:57 +0100 Subject: [PATCH 20/25] Fix unit tests --- .../Controllers/MemberControllerUnitTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs index 8c8b6b0504..857f51dc20 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs @@ -19,6 +19,7 @@ using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.ContentApps; using Umbraco.Cms.Core.Dictionary; using Umbraco.Cms.Core.Events; +using Umbraco.Cms.Core.Hosting; using Umbraco.Cms.Core.Mapping; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; @@ -38,7 +39,6 @@ using Umbraco.Cms.Web.BackOffice.Controllers; using Umbraco.Cms.Web.BackOffice.Mapping; using Umbraco.Cms.Web.Common.ActionsResults; using Umbraco.Cms.Web.Common.Security; -using IHostingEnvironment = Umbraco.Cms.Core.Hosting.IHostingEnvironment; using MemberMapDefinition = Umbraco.Cms.Web.BackOffice.Mapping.MemberMapDefinition; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers @@ -338,7 +338,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers .Setup(x => x.ValidatePasswordAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(umbracoMembersUserManager) - .Setup(x => x.AddToRolesAsync(It.IsAny(), It.IsAny>())) + .Setup(x => x.AddToRolesAsync(It.IsAny(), It.IsAny>())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(memberService).SetupSequence( @@ -389,7 +389,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers .Setup(x => x.UpdateAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(umbracoMembersUserManager) - .Setup(x => x.AddToRolesAsync(It.IsAny(), It.IsAny>())) + .Setup(x => x.AddToRolesAsync(It.IsAny(), It.IsAny>())) .ReturnsAsync(() => IdentityResult.Success); Mock.Get(memberTypeService).Setup(x => x.GetDefault()).Returns("fakeAlias"); Mock.Get(backOfficeSecurityAccessor).Setup(x => x.BackOfficeSecurity).Returns(backOfficeSecurity); From 33b5f402b433f777d1c5e9617505969ccb45edb1 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 26 Mar 2021 14:06:17 +1100 Subject: [PATCH 21/25] reverts MemberGroupController and removes TODOs --- .../Controllers/MemberController.cs | 5 - .../Controllers/MemberGroupController.cs | 117 +++--------------- 2 files changed, 19 insertions(+), 103 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs index 05e4cc7486..567303e150 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs @@ -194,7 +194,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers [OutgoingEditorModelEvent] public MemberDisplay GetByKey(Guid key) { - //TODO: convert to identity IMember foundMember = _memberService.GetByKey(key); if (foundMember == null) { @@ -395,9 +394,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers } } - //TODO: do we need to resave the key? - // contentItem.PersistedContent.Key = contentItem.Key; - // now the member has been saved via identity, resave the member with mapped content properties _memberService.Save(member); contentItem.PersistedContent = member; @@ -655,7 +651,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers [HttpPost] public IActionResult DeleteByKey(Guid key) { - //TODO: move to MembersUserStore IMember foundMember = _memberService.GetByKey(key); if (foundMember == null) { diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs index f90bd6458c..88825c0d9a 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs @@ -1,9 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Mapping; @@ -28,44 +26,30 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers private readonly IMemberGroupService _memberGroupService; private readonly UmbracoMapper _umbracoMapper; private readonly ILocalizedTextService _localizedTextService; - private readonly RoleManager _roleManager; public MemberGroupController( IMemberGroupService memberGroupService, UmbracoMapper umbracoMapper, - ILocalizedTextService localizedTextService, - RoleManager roleManager - ) + ILocalizedTextService localizedTextService) { _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); - _roleManager = roleManager ?? throw new ArgumentNullException(nameof(roleManager)); _umbracoMapper = umbracoMapper ?? throw new ArgumentNullException(nameof(umbracoMapper)); _localizedTextService = localizedTextService ?? throw new ArgumentNullException(nameof(localizedTextService)); } - //TODO: are there any repercussions elsewhere for us changing these to async? - /// /// Gets the member group json for the member group id /// /// /// - public async Task> GetById(int id) + public ActionResult GetById(int id) { - //TODO: did we envisage this - combination of service and identity manager? - IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString()); - if (identityRole == null) - { - return NotFound(); - } - IMemberGroup memberGroup = _memberGroupService.GetById(id); if (memberGroup == null) { return NotFound(); } - //TODO: the default identity role doesn't have all the properties IMemberGroup had, e.g. CreatorId MemberGroupDisplay dto = _umbracoMapper.Map(memberGroup); return dto; } @@ -75,14 +59,8 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// /// - public async Task> GetById(Guid id) + public ActionResult GetById(Guid id) { - //TODO: did we envisage just identity or a combination of service and identity manager? - //IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString()); - //if (identityRole == null) - //{ - // return NotFound(); - //} IMemberGroup memberGroup = _memberGroupService.GetById(id); if (memberGroup == null) { @@ -97,7 +75,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// /// - public async Task> GetById(Udi id) + public ActionResult GetById(Udi id) { var guidUdi = id as GuidUdi; if (guidUdi == null) @@ -105,13 +83,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return NotFound(); } - //TODO: can we do this via identity? - IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString()); - if (identityRole == null) - { - return NotFound(); - } - IMemberGroup memberGroup = _memberGroupService.GetById(guidUdi.Guid); if (memberGroup == null) { @@ -122,47 +93,25 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers } public IEnumerable GetByIds([FromQuery] int[] ids) - { - //var roles = new List(); - - //foreach (int id in ids) - //{ - // IdentityRole role = await _roleManager.FindByIdAsync(id.ToString()); - // roles.Add(role); - //} - - //return roles.Select(x => _umbracoMapper.Map(x)); - //TODO: does this need to be done via identity? - - return _memberGroupService.GetByIds(ids) - .Select(_umbracoMapper.Map); - } + => _memberGroupService.GetByIds(ids).Select(_umbracoMapper.Map); [HttpDelete] [HttpPost] - public async Task DeleteById(int id) + public IActionResult DeleteById(int id) { - IdentityRole role = await _roleManager.FindByIdAsync(id.ToString()); - - if (role == null) + var memberGroup = _memberGroupService.GetById(id); + if (memberGroup == null) { return NotFound(); } - IdentityResult roleDeleted = await _roleManager.DeleteAsync(role); - if (roleDeleted.Succeeded) - { - return Ok(); - } - else - { - return Problem("Issue during deletion - please see logs"); - } + _memberGroupService.Delete(memberGroup); + return Ok(); } - //TODO: we don't currently implement IQueryableRoleStore, still using original service - //public IEnumerable GetAllGroups() => _roleManager.Roles.Select(x => _umbracoMapper.Map(x)); - public IEnumerable GetAllGroups() => _memberGroupService.GetAll().Select(x => _umbracoMapper.Map(x)); + public IEnumerable GetAllGroups() + => _memberGroupService.GetAll() + .Select(_umbracoMapper.Map); public MemberGroupDisplay GetEmpty() { @@ -170,47 +119,19 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return _umbracoMapper.Map(item); } - /// - /// Saves the member group via the identity role - /// If new, creates a new role, else updates the existing role - /// - /// - /// - public async Task> PostSave(MemberGroupSave saveModel) + public ActionResult PostSave(MemberGroupSave saveModel) { - int id = int.Parse(saveModel.Id.ToString()); - - IdentityRole role; - IdentityResult updatedResult; - if (id > 0) - { - role = await _roleManager.FindByIdAsync(saveModel.Id.ToString()); - role.Name = saveModel.Name; - updatedResult = await _roleManager.UpdateAsync(role); - - } - else - { - role = new IdentityRole(saveModel.Name); - updatedResult = await _roleManager.CreateAsync(role); - } - - if (!updatedResult.Succeeded) - { - //TODO: what to return if there is a failed identity result - return Problem(); - } - - //TODO: do we need to refetch the member group? - int roleId = int.Parse(role.Id); - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); + var id = int.Parse(saveModel.Id.ToString()); + IMemberGroup memberGroup = id > 0 ? _memberGroupService.GetById(id) : new MemberGroup(); if (memberGroup == null) { return NotFound(); } - //TODO: should we return the identity role or return the group from the service? + memberGroup.Name = saveModel.Name; + _memberGroupService.Save(memberGroup); + MemberGroupDisplay display = _umbracoMapper.Map(memberGroup); display.AddSuccessNotification( From db3dc01321ccba0977cc33b3f39406ad263863ea Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 26 Mar 2021 14:18:41 +1100 Subject: [PATCH 22/25] removes todos, reverts tree controller, fixes up review notes. --- .../Mapping/MemberTabsAndPropertiesMapper.cs | 15 +++++++++------ .../Security/MemberUserStore.cs | 14 ++++++-------- .../Mapping/MemberMapDefinition.cs | 14 -------------- .../Trees/MemberGroupTreeController.cs | 14 ++++---------- 4 files changed, 19 insertions(+), 38 deletions(-) diff --git a/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs b/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs index 02ee4fe744..0d3a5b7536 100644 --- a/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs +++ b/src/Umbraco.Core/Models/Mapping/MemberTabsAndPropertiesMapper.cs @@ -119,7 +119,7 @@ namespace Umbraco.Cms.Core.Models.Mapping Value = _localizedTextService.UmbracoDictionaryTranslate(CultureDictionary, member.ContentType.Name), View = _propertyEditorCollection[Constants.PropertyEditors.Aliases.Label].GetValueEditor().View }, - GetLoginProperty(_memberTypeService, member, _localizedTextService), + GetLoginProperty(member, _localizedTextService), new ContentPropertyDisplay { Alias = $"{Constants.PropertyEditors.InternalGenericPropertiesPrefix}email", @@ -208,7 +208,6 @@ namespace Umbraco.Cms.Core.Models.Mapping /// /// Returns the login property display field /// - /// /// /// /// @@ -218,7 +217,7 @@ namespace Umbraco.Cms.Core.Models.Mapping /// the membership provider is a custom one, we cannot allow changing the username because MembershipProvider's do not actually natively /// allow that. /// - internal static ContentPropertyDisplay GetLoginProperty(IMemberTypeService memberTypeService, IMember member, ILocalizedTextService localizedText) + internal static ContentPropertyDisplay GetLoginProperty(IMember member, ILocalizedTextService localizedText) { var prop = new ContentPropertyDisplay { @@ -234,10 +233,9 @@ namespace Umbraco.Cms.Core.Models.Mapping internal IDictionary GetMemberGroupValue(string username) { - var userRoles = username.IsNullOrWhiteSpace() ? null : _memberService.GetAllRoles(username); + IEnumerable userRoles = username.IsNullOrWhiteSpace() ? null : _memberService.GetAllRoles(username); // create a dictionary of all roles (except internal roles) + "false" - //TODO: use member role manager instead var result = _memberGroupService.GetAll() .Select(x => x.Name) // if a role starts with __umbracoRole we won't show it as it's an internal role used for public access @@ -246,11 +244,16 @@ namespace Umbraco.Cms.Core.Models.Mapping .ToDictionary(x => x, x => false); // if user has no roles, just return the dictionary - if (userRoles == null) return result; + if (userRoles == null) + { + return result; + } // else update the dictionary to "true" for the user roles (except internal roles) foreach (var userRole in userRoles.Where(x => x.StartsWith(Constants.Conventions.Member.InternalRolePrefix) == false)) + { result[userRole] = true; + } return result; } diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 2f2127201d..c0b9a19ef1 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -363,13 +363,6 @@ namespace Umbraco.Cms.Core.Security cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - MemberIdentityUser user = await FindUserAsync(userId, cancellationToken); - if (user == null) - { - //TODO: error throw or null result? - return await Task.FromResult((IdentityUserLogin)null); - } - if (string.IsNullOrWhiteSpace(loginProvider)) { throw new ArgumentNullException(nameof(loginProvider)); @@ -380,11 +373,16 @@ namespace Umbraco.Cms.Core.Security throw new ArgumentNullException(nameof(providerKey)); } + MemberIdentityUser user = await FindUserAsync(userId, cancellationToken); + if (user == null) + { + return await Task.FromResult((IdentityUserLogin)null); + } + IList logins = await GetLoginsAsync(user, cancellationToken); UserLoginInfo found = logins.FirstOrDefault(x => x.ProviderKey == providerKey && x.LoginProvider == loginProvider); if (found == null) { - //TODO: error throw or null result? return await Task.FromResult((IdentityUserLogin)null); } diff --git a/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs b/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs index 7ee456a323..be713ddfa9 100644 --- a/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs +++ b/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs @@ -35,7 +35,6 @@ namespace Umbraco.Cms.Web.BackOffice.Mapping mapper.Define((source, context) => new MemberDisplay(), Map); mapper.Define((source, context) => new MemberBasic(), Map); mapper.Define((source, context) => new MemberGroupDisplay(), Map); - mapper.Define((source, context) => new MemberGroupDisplay(), Map); mapper.Define((source, context) => new ContentPropertyCollectionDto(), Map); } @@ -103,18 +102,5 @@ namespace Umbraco.Cms.Web.BackOffice.Mapping { target.Properties = context.MapEnumerable(source.Properties); } - - /// - /// Maps an identity role to a member group display - /// - /// - /// - /// - private void Map(IdentityRole source, MemberGroupDisplay target, MapperContext context) - { - //TODO: this is all that is mapped at this time, we're losing a lot of properties - target.Id = source.Id; - target.Name = source.Name; - } } } diff --git a/src/Umbraco.Web.BackOffice/Trees/MemberGroupTreeController.cs b/src/Umbraco.Web.BackOffice/Trees/MemberGroupTreeController.cs index 5888068656..0559a17a53 100644 --- a/src/Umbraco.Web.BackOffice/Trees/MemberGroupTreeController.cs +++ b/src/Umbraco.Web.BackOffice/Trees/MemberGroupTreeController.cs @@ -28,29 +28,23 @@ namespace Umbraco.Cms.Web.BackOffice.Trees IMemberGroupService memberGroupService, IEventAggregator eventAggregator) : base(localizedTextService, umbracoApiControllerTypeCollection, menuItemCollectionFactory, eventAggregator) - { - _memberGroupService = memberGroupService; - } + => _memberGroupService = memberGroupService; - //TODO: change to role store protected override IEnumerable GetTreeNodesFromService(string id, FormCollection queryStrings) - { - return _memberGroupService.GetAll() + => _memberGroupService.GetAll() .OrderBy(x => x.Name) .Select(dt => CreateTreeNode(dt.Id.ToString(), id, queryStrings, dt.Name, Constants.Icons.MemberGroup, false)); - } protected override ActionResult CreateRootNode(FormCollection queryStrings) { - var rootResult = base.CreateRootNode(queryStrings); + ActionResult rootResult = base.CreateRootNode(queryStrings); if (!(rootResult.Result is null)) { return rootResult; } - var root = rootResult.Value; + TreeNode root = rootResult.Value; //check if there are any groups - //TODO: change to role store root.HasChildren = _memberGroupService.GetAll().Any(); return root; } From 48a2a0a357e213e2b44b73cefcf75970a78a8ae6 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 26 Mar 2021 15:54:45 +1100 Subject: [PATCH 23/25] remove httpcontext requirement from mapper --- .../Controllers/MemberControllerUnitTests.cs | 3 +-- src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs | 5 +---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs index 857f51dc20..321f34f517 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs @@ -484,8 +484,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers memberGroupService, mockPasswordConfig.Object, contentTypeBaseServiceProvider.Object, - propertyEditorCollection), - httpContextAccessor); + propertyEditorCollection)); var map = new MapDefinitionCollection(new List() { diff --git a/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs b/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs index be713ddfa9..b5592b08ff 100644 --- a/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs +++ b/src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs @@ -19,15 +19,12 @@ namespace Umbraco.Cms.Web.BackOffice.Mapping private readonly CommonMapper _commonMapper; private readonly CommonTreeNodeMapper _commonTreeNodeMapper; private readonly MemberTabsAndPropertiesMapper _tabsAndPropertiesMapper; - private readonly IHttpContextAccessor _httpContextAccessor; - public MemberMapDefinition(CommonMapper commonMapper, CommonTreeNodeMapper commonTreeNodeMapper, MemberTabsAndPropertiesMapper tabsAndPropertiesMapper, IHttpContextAccessor httpContextAccessor) + public MemberMapDefinition(CommonMapper commonMapper, CommonTreeNodeMapper commonTreeNodeMapper, MemberTabsAndPropertiesMapper tabsAndPropertiesMapper) { _commonMapper = commonMapper; _commonTreeNodeMapper = commonTreeNodeMapper; - _tabsAndPropertiesMapper = tabsAndPropertiesMapper; - _httpContextAccessor = httpContextAccessor; } public void DefineMaps(UmbracoMapper mapper) From 29b1aa1b33976c23193c23e6baef74de1e64c182 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 26 Mar 2021 16:43:49 +1100 Subject: [PATCH 24/25] Refactor MemberRoleStore to not be generic and use our own UmbracoIdentityRole which has support for change tracking, uses builders for models in test --- .../Security/MemberRoleStore.cs | 75 +++----- .../Security/UmbracoIdentityRole.cs | 97 ++++++++++ .../Security/UmbracoIdentityUser.cs | 1 + .../Builders/Extensions/BuilderExtensions.cs | 7 + .../Builders/Interfaces/IWithIdBuilder.cs | 5 + .../Builders/UmbracoIdentityRoleBuilder.cs | 47 +++++ .../Builders/UserBuilder.cs | 2 + .../Security/MemberRoleStoreTests.cs | 169 ++++++------------ .../ServiceCollectionExtensions.cs | 2 +- 9 files changed, 239 insertions(+), 166 deletions(-) create mode 100644 src/Umbraco.Infrastructure/Security/UmbracoIdentityRole.cs create mode 100644 src/Umbraco.Tests.Common/Builders/UmbracoIdentityRoleBuilder.cs diff --git a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs index 8f4bd0cd17..279735bfa2 100644 --- a/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberRoleStore.cs @@ -3,6 +3,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Identity; using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Models.Identity; using Umbraco.Cms.Core.Services; namespace Umbraco.Cms.Core.Security @@ -10,7 +11,7 @@ namespace Umbraco.Cms.Core.Security /// /// A custom user store that uses Umbraco member data /// - public class MemberRoleStore : IRoleStore where TRole : IdentityRole + public class MemberRoleStore : IRoleStore { private readonly IMemberGroupService _memberGroupService; private bool _disposed; @@ -33,7 +34,7 @@ namespace Umbraco.Cms.Core.Security public IdentityErrorDescriber ErrorDescriber { get; set; } /// - public Task CreateAsync(TRole role, CancellationToken cancellationToken = default) + public Task CreateAsync(UmbracoIdentityRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -57,7 +58,7 @@ namespace Umbraco.Cms.Core.Security /// - public Task UpdateAsync(TRole role, CancellationToken cancellationToken = default) + public Task UpdateAsync(UmbracoIdentityRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -79,19 +80,17 @@ namespace Umbraco.Cms.Core.Security { _memberGroupService.Save(memberGroup); } - //TODO: if nothing changed, do we need to report this? + return Task.FromResult(IdentityResult.Success); } else { - //TODO: throw exception when not found, or return failure? return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); } - } /// - public Task DeleteAsync(TRole role, CancellationToken cancellationToken = default) + public Task DeleteAsync(UmbracoIdentityRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -103,8 +102,7 @@ namespace Umbraco.Cms.Core.Security if (!int.TryParse(role.Id, out int roleId)) { - //TODO: what identity error should we return in this case? - return Task.FromResult(IdentityResult.Failed(_intParseError)); + throw new ArgumentException("The Id of the role is not an integer"); } IMemberGroup memberGroup = _memberGroupService.GetById(roleId); @@ -121,8 +119,7 @@ namespace Umbraco.Cms.Core.Security } /// - - public Task GetRoleIdAsync(TRole role, CancellationToken cancellationToken = default) + public Task GetRoleIdAsync(UmbracoIdentityRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -136,7 +133,7 @@ namespace Umbraco.Cms.Core.Security } /// - public Task GetRoleNameAsync(TRole role, CancellationToken cancellationToken = default) + public Task GetRoleNameAsync(UmbracoIdentityRole role, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -150,47 +147,28 @@ namespace Umbraco.Cms.Core.Security } /// - public Task SetRoleNameAsync(TRole role, string roleName, CancellationToken cancellationToken = default) + public Task SetRoleNameAsync(UmbracoIdentityRole role, string roleName, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - if (role == null) { throw new ArgumentNullException(nameof(role)); } - - if (!int.TryParse(role.Id, out int roleId)) - { - //TODO: what identity error should we return in this case? - return Task.FromResult(IdentityResult.Failed(ErrorDescriber.DefaultError())); - } - - IMemberGroup memberGroup = _memberGroupService.GetById(roleId); - - if (memberGroup != null) - { - //TODO: confirm logic - memberGroup.Name = roleName; - _memberGroupService.Save(memberGroup); - role.Name = roleName; - } - else - { - return Task.FromResult(IdentityResult.Failed(_memberGroupNotFoundError)); - } - + role.Name = roleName; return Task.CompletedTask; } /// - public Task GetNormalizedRoleNameAsync(TRole role, CancellationToken cancellationToken = default) => GetRoleNameAsync(role, cancellationToken); + public Task GetNormalizedRoleNameAsync(UmbracoIdentityRole role, CancellationToken cancellationToken = default) + => GetRoleNameAsync(role, cancellationToken); /// - public Task SetNormalizedRoleNameAsync(TRole role, string normalizedName, CancellationToken cancellationToken = default) => SetRoleNameAsync(role, normalizedName, cancellationToken); + public Task SetNormalizedRoleNameAsync(UmbracoIdentityRole role, string normalizedName, CancellationToken cancellationToken = default) + => SetRoleNameAsync(role, normalizedName, cancellationToken); /// - public Task FindByIdAsync(string roleId, CancellationToken cancellationToken = default) + public Task FindByIdAsync(string roleId, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -223,7 +201,7 @@ namespace Umbraco.Cms.Core.Security } /// - public Task FindByNameAsync(string name, CancellationToken cancellationToken = default) + public Task FindByNameAsync(string name, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); @@ -241,16 +219,16 @@ namespace Umbraco.Cms.Core.Security /// /// /// - private TRole MapFromMemberGroup(IMemberGroup memberGroup) + private UmbracoIdentityRole MapFromMemberGroup(IMemberGroup memberGroup) { - var result = new IdentityRole + var result = new UmbracoIdentityRole { Id = memberGroup.Id.ToString(), Name = memberGroup.Name - //TODO: Are we interested in NormalizedRoleName? + // TODO: Implement this functionality, requires DB and logic updates + //ConcurrencyStamp }; - - return result as TRole; + return result; } /// @@ -259,12 +237,15 @@ namespace Umbraco.Cms.Core.Security /// /// /// - private bool MapToMemberGroup(TRole role, IMemberGroup memberGroup) + private bool MapToMemberGroup(UmbracoIdentityRole role, IMemberGroup memberGroup) { var anythingChanged = false; - if (!string.IsNullOrEmpty(role.Name) && memberGroup.Name != role.Name) + if (role.IsPropertyDirty(nameof(UmbracoIdentityRole.Name)) + && !string.IsNullOrEmpty(role.Name) && memberGroup.Name != role.Name) { + // TODO: Need to support ConcurrencyStamp and logic + memberGroup.Name = role.Name; anythingChanged = true; } @@ -272,8 +253,6 @@ namespace Umbraco.Cms.Core.Security return anythingChanged; } - //TODO: is any dispose action necessary here? - /// /// Dispose the store /// diff --git a/src/Umbraco.Infrastructure/Security/UmbracoIdentityRole.cs b/src/Umbraco.Infrastructure/Security/UmbracoIdentityRole.cs new file mode 100644 index 0000000000..9d06dcd037 --- /dev/null +++ b/src/Umbraco.Infrastructure/Security/UmbracoIdentityRole.cs @@ -0,0 +1,97 @@ +using System.Collections.Generic; +using System.ComponentModel; +using Microsoft.AspNetCore.Identity; +using Umbraco.Cms.Core.Models.Entities; + +namespace Umbraco.Cms.Core.Models.Identity +{ + public class UmbracoIdentityRole : IdentityRole, IRememberBeingDirty + { + private string _id; + private string _name; + + public event PropertyChangedEventHandler PropertyChanged + { + add + { + BeingDirty.PropertyChanged += value; + } + + remove + { + BeingDirty.PropertyChanged -= value; + } + } + + /// + public override string Id + { + get => _id; + set + { + _id = value; + HasIdentity = true; + } + } + + /// + public override string Name + { + get => _name; + set => BeingDirty.SetPropertyValueAndDetectChanges(value, ref _name, nameof(Name)); + } + + /// + public override string NormalizedName { get => base.Name; set => base.Name = value; } + + /// + /// Gets or sets a value indicating whether returns an Id has been set on this object this will be false if the object is new and not persisted to the database + /// + public bool HasIdentity { get; protected set; } + + // TODO: We should support this and it's logic + public override string ConcurrencyStamp { get => base.ConcurrencyStamp; set => base.ConcurrencyStamp = value; } + + /// + /// Gets the for change tracking + /// + protected BeingDirty BeingDirty { get; } = new BeingDirty(); + + /// + public bool IsDirty() => BeingDirty.IsDirty(); + + /// + public bool IsPropertyDirty(string propName) => BeingDirty.IsPropertyDirty(propName); + + /// + public IEnumerable GetDirtyProperties() => BeingDirty.GetDirtyProperties(); + + /// + public void ResetDirtyProperties() => BeingDirty.ResetDirtyProperties(); + + /// + public bool WasDirty() => BeingDirty.WasDirty(); + + /// + public bool WasPropertyDirty(string propertyName) => BeingDirty.WasPropertyDirty(propertyName); + + /// + public void ResetWereDirtyProperties() => BeingDirty.ResetWereDirtyProperties(); + + /// + public void ResetDirtyProperties(bool rememberDirty) => BeingDirty.ResetDirtyProperties(rememberDirty); + + /// + public IEnumerable GetWereDirtyProperties() => BeingDirty.GetWereDirtyProperties(); + + /// + /// Disables change tracking. + /// + public void DisableChangeTracking() => BeingDirty.DisableChangeTracking(); + + /// + /// Enables change tracking. + /// + public void EnableChangeTracking() => BeingDirty.EnableChangeTracking(); + } +} diff --git a/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs b/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs index 525e7f839a..bf553b3d30 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs @@ -8,6 +8,7 @@ using Umbraco.Cms.Core.Models.Entities; namespace Umbraco.Cms.Core.Models.Identity { + /// /// Abstract class for use in Umbraco Identity for users and members /// diff --git a/src/Umbraco.Tests.Common/Builders/Extensions/BuilderExtensions.cs b/src/Umbraco.Tests.Common/Builders/Extensions/BuilderExtensions.cs index b563cc3ec4..872a6ac367 100644 --- a/src/Umbraco.Tests.Common/Builders/Extensions/BuilderExtensions.cs +++ b/src/Umbraco.Tests.Common/Builders/Extensions/BuilderExtensions.cs @@ -17,6 +17,13 @@ namespace Umbraco.Cms.Tests.Common.Builders.Extensions return builder; } + public static T WithId(this T builder, TId id) + where T : IWithIdBuilder + { + builder.Id = id; + return builder; + } + public static T WithoutIdentity(this T builder) where T : IWithIdBuilder { diff --git a/src/Umbraco.Tests.Common/Builders/Interfaces/IWithIdBuilder.cs b/src/Umbraco.Tests.Common/Builders/Interfaces/IWithIdBuilder.cs index fe26c89d85..604f683dd7 100644 --- a/src/Umbraco.Tests.Common/Builders/Interfaces/IWithIdBuilder.cs +++ b/src/Umbraco.Tests.Common/Builders/Interfaces/IWithIdBuilder.cs @@ -7,4 +7,9 @@ namespace Umbraco.Cms.Tests.Common.Builders.Interfaces { int? Id { get; set; } } + + public interface IWithIdBuilder + { + TId Id { get; set; } + } } diff --git a/src/Umbraco.Tests.Common/Builders/UmbracoIdentityRoleBuilder.cs b/src/Umbraco.Tests.Common/Builders/UmbracoIdentityRoleBuilder.cs new file mode 100644 index 0000000000..6ffe4fd5c5 --- /dev/null +++ b/src/Umbraco.Tests.Common/Builders/UmbracoIdentityRoleBuilder.cs @@ -0,0 +1,47 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using Umbraco.Cms.Core.Models.Identity; +using Umbraco.Cms.Tests.Common.Builders.Interfaces; + +namespace Umbraco.Cms.Tests.Common.Builders +{ + public class UmbracoIdentityRoleBuilder : BuilderBase, + IWithIdBuilder, + IWithNameBuilder + { + private string _id; + private string _name; + + public UmbracoIdentityRoleBuilder WithTestName(string id) + { + _name = "testname"; + _id = id; + return this; + } + + string IWithNameBuilder.Name + { + get => _name; + set => _name = value; + } + + string IWithIdBuilder.Id + { + get => _id; + set => _id = value; + } + + public override UmbracoIdentityRole Build() + { + var id = _id; + var name = _name; + + return new UmbracoIdentityRole + { + Id = id, + Name = name, + }; + } + } +} diff --git a/src/Umbraco.Tests.Common/Builders/UserBuilder.cs b/src/Umbraco.Tests.Common/Builders/UserBuilder.cs index 95fbc3a435..9d00962a9f 100644 --- a/src/Umbraco.Tests.Common/Builders/UserBuilder.cs +++ b/src/Umbraco.Tests.Common/Builders/UserBuilder.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Linq; using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.Models.Identity; using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Tests.Common.Builders.Extensions; using Umbraco.Cms.Tests.Common.Builders.Interfaces; @@ -12,6 +13,7 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Tests.Common.Builders { + public class UserBuilder : UserBuilder { public UserBuilder() diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs index a6c534d24d..15f4b7f30d 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs @@ -6,8 +6,11 @@ using Microsoft.AspNetCore.Identity; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Models.Identity; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Tests.Common.Builders; +using Umbraco.Cms.Tests.Common.Builders.Extensions; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { @@ -16,20 +19,29 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security { private Mock _mockMemberGroupService; private IdentityErrorDescriber ErrorDescriber => new IdentityErrorDescriber(); + private UmbracoIdentityRoleBuilder _roleBuilder; + private MemberGroupBuilder _groupBuilder; - public MemberRoleStore CreateSut() + public MemberRoleStore CreateSut() { _mockMemberGroupService = new Mock(); - return new MemberRoleStore( + return new MemberRoleStore( _mockMemberGroupService.Object, ErrorDescriber); } + [SetUp] + public void SetUp() + { + _roleBuilder = new UmbracoIdentityRoleBuilder(); + _groupBuilder = new MemberGroupBuilder(); + } + [Test] public void GivenICreateAMemberRole_AndTheGroupIsNull_ThenIShouldGetAFailedIdentityResult() { // arrange - MemberRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeCancellationToken = new CancellationToken(); // act @@ -44,12 +56,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenICreateAMemberRole_AndTheGroupIsPopulatedCorrectly_ThenIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "777", - Name = "testname" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithTestName("777").Build(); var fakeCancellationToken = new CancellationToken() { }; IMemberGroup mockMemberGroup = Mock.Of(m => @@ -72,12 +80,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupExistsWithTheSameName_ThenIShouldGetASuccessResultAsyncButNoUpdatesMade() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "777", - Name = "fakeGroupName" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithName("fakeGroupName").WithId("777").Build(); var fakeCancellationToken = new CancellationToken() { }; IMemberGroup mockMemberGroup = Mock.Of(m => @@ -101,12 +105,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupExistsWithADifferentSameName_ThenIShouldGetASuccessResultAsyncWithUpdatesMade() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "777", - Name = "fakeGroup777" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithName("fakeGroup777").WithId("777").Build(); var fakeCancellationToken = new CancellationToken() { }; IMemberGroup mockMemberGroup = Mock.Of(m => @@ -131,12 +131,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheGroupDoesntExist_ThenIShouldGetAFailureResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "777", - Name = "testname" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithTestName("777").Build(); var fakeCancellationToken = new CancellationToken() { }; // act @@ -152,12 +148,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIUpdateAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "7a77", - Name = "testname" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithTestName("7a77").Build(); var fakeCancellationToken = new CancellationToken() { }; // act @@ -173,7 +165,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public void GivenIUpdateAMemberRole_AndTheRoleIsNull_ThenAnExceptionShouldBeThrown() { // arrange - MemberRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeCancellationToken = new CancellationToken() { }; // act @@ -188,12 +180,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIDeleteAMemberRole_AndItExists_ThenTheMemberGroupShouldBeDeleted_AndIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "777", - Name = "testname" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithTestName("777").Build(); var fakeCancellationToken = new CancellationToken() { }; IMemberGroup mockMemberGroup = Mock.Of(m => @@ -213,15 +201,11 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security } [Test] - public async Task GivenIDeleteAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAFailResultAsync() + public async Task GivenIDeleteAMemberRole_AndTheIdCannotBeParsedToAnInt_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAnArgumentException() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "7a77", - Name = "testname" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithTestName("7a77").Build(); var fakeCancellationToken = new CancellationToken() { }; IMemberGroup mockMemberGroup = Mock.Of(m => @@ -229,12 +213,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // act - IdentityResult identityResult = await sut.DeleteAsync(fakeRole, fakeCancellationToken); - - // assert - Assert.IsTrue(identityResult.Succeeded == false); - Assert.IsTrue(identityResult.Errors.Any(x => x.Code == "IdentityIdParseError" && x.Description == "Cannot parse ID to int")); - _mockMemberGroupService.VerifyNoOtherCalls(); + Assert.ThrowsAsync(async () => await sut.DeleteAsync(fakeRole, fakeCancellationToken)); } @@ -242,12 +221,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIDeleteAMemberRole_AndItDoesntExist_ThenTheMemberGroupShouldNotBeDeleted_AndIShouldGetAFailResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "777", - Name = "testname" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithTestName("777").Build(); var fakeCancellationToken = new CancellationToken() { }; IMemberGroup mockMemberGroup = Mock.Of(m => @@ -268,20 +243,11 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIFindAMemberRoleByRoleKey_AndRoleKeyExists_ThenIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole("fakeGroupName") - { - Id = "777" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithName("fakeGroupName").WithId("777").Build(); int fakeRoleId = 777; - IMemberGroup fakeMemberGroup = new MemberGroup() - { - Name = "fakeGroupName", - CreatorId = 123, - Id = 777, - Key = Guid.NewGuid() - }; + IMemberGroup fakeMemberGroup = _groupBuilder.WithName("fakeGroupName").WithCreatorId(123).WithId(777).WithKey(Guid.NewGuid()).Build(); _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(fakeMemberGroup); @@ -299,12 +265,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnIntOrGuid_ThenIShouldGetAFailureResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "7a77", - Name = "testname" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithTestName("7a77").Build(); var fakeCancellationToken = new CancellationToken() { }; // act @@ -319,21 +281,12 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnIntButCanBeToGuid_ThenIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole("fakeGroupName") - { - Id = "777" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithName("fakeGroupName").WithId("777").Build(); var fakeRoleGuid = Guid.NewGuid(); - IMemberGroup fakeMemberGroup = new MemberGroup() - { - Name = "fakeGroupName", - CreatorId = 123, - Id = 777, - Key = fakeRoleGuid - }; + IMemberGroup fakeMemberGroup = _groupBuilder.WithName("fakeGroupName").WithCreatorId(123).WithId(777).WithKey(fakeRoleGuid).Build(); _mockMemberGroupService.Setup(x => x.GetById(fakeRoleGuid)).Returns(fakeMemberGroup); @@ -352,21 +305,12 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAGuidButCanBeToInt_ThenIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole("fakeGroupName") - { - Id = "777" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithName("fakeGroupName").WithId("777").Build(); var fakeRoleId = 777; - IMemberGroup fakeMemberGroup = new MemberGroup() - { - Name = "fakeGroupName", - CreatorId = 123, - Id = 777, - Key = Guid.NewGuid() - }; + IMemberGroup fakeMemberGroup = _groupBuilder.WithName("fakeGroupName").WithCreatorId(123).WithId(777).WithKey(Guid.NewGuid()).Build(); _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(fakeMemberGroup); @@ -385,11 +329,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public async Task GivenIFindAMemberRoleByRoleName_AndRoleNameExists_ThenIShouldGetASuccessResultAsync() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole("fakeGroupName") - { - Id = "777" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithName("fakeGroupName").WithId("777").Build(); IMemberGroup mockMemberGroup = Mock.Of(m => m.Name == "fakeGroupName" && @@ -412,11 +353,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public void GivenIFindAMemberRoleByRoleName_AndTheNameIsNull_ThenIShouldGetAnArgumentException() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole - { - Id = "777" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithId("777").Build(); var fakeCancellationToken = new CancellationToken() { }; // act @@ -432,7 +370,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public void GivenIGetAMemberRoleId_AndTheRoleIsNull_ThenIShouldGetAnArgumentException() { // arrange - MemberRoleStore sut = CreateSut(); + MemberRoleStore sut = CreateSut(); var fakeCancellationToken = new CancellationToken() { }; // act @@ -446,11 +384,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security public void GivenIGetAMemberRoleId_AndTheRoleIsNotNull_ThenIShouldGetTheMemberRole() { // arrange - MemberRoleStore sut = CreateSut(); - var fakeRole = new IdentityRole("fakeGroupName") - { - Id = "777" - }; + MemberRoleStore sut = CreateSut(); + UmbracoIdentityRole fakeRole = _roleBuilder.WithName("fakeGroupName").WithId("777").Build(); string fakeRoleId = fakeRole.Id; var fakeCancellationToken = new CancellationToken(); diff --git a/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs index 1ff044ed3f..5182db4e20 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/ServiceCollectionExtensions.cs @@ -68,7 +68,7 @@ namespace Umbraco.Extensions .AddDefaultTokenProviders() .AddMemberManager() .AddUserStore() - .AddRoleStore>() + .AddRoleStore() .AddRoleValidator>() .AddRoleManager>(); From 005a23958c4196d0fbc2b5c396f86cc0a24b33d0 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 26 Mar 2021 16:52:42 +1100 Subject: [PATCH 25/25] remove todo --- .../Persistence/Repositories/Implement/MemberRepository.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs index 406eb08c62..e97add3f5e 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs @@ -314,7 +314,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement // persist the member dto dto.NodeId = nodeDto.NodeId; - // TODO: password parts of this file need updating // if the password is empty, generate one with the special prefix // this will hash the guid with a salt so should be nicely random if (entity.RawPasswordValue.IsNullOrWhiteSpace())