From 4e9423687cceefc40b3732885240dc937c6ffd9c Mon Sep 17 00:00:00 2001 From: emmagarland Date: Sat, 6 Mar 2021 23:09:19 +0000 Subject: [PATCH] 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; + } } }