From 7bbef2a5842e8a4d997f916e4edb32c7657b6831 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Tue, 23 Apr 2024 12:25:36 +0200 Subject: [PATCH] V14: Fix member mapping (#16106) * convert key to name before saving roles * Rework MemberEditingService to convert keys * Rename variable to groups * Extract to variable --- .../Factories/MemberPresentationFactory.cs | 9 +++++-- .../Member/CreateMemberRequestModel.cs | 2 +- .../ViewModels/Member/MemberResponseModel.cs | 2 +- .../Member/UpdateMemberRequestModel.cs | 2 +- .../ContentEditing/MemberEditingModelBase.cs | 2 +- .../Services/MemberEditingService.cs | 26 +++++++++++++++---- .../Services/MemberEditingServiceTests.cs | 8 ++++-- 7 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Factories/MemberPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/MemberPresentationFactory.cs index 34d1c46b0a..ba7bc036fc 100644 --- a/src/Umbraco.Cms.Api.Management/Factories/MemberPresentationFactory.cs +++ b/src/Umbraco.Cms.Api.Management/Factories/MemberPresentationFactory.cs @@ -18,17 +18,20 @@ internal sealed class MemberPresentationFactory : IMemberPresentationFactory private readonly IMemberService _memberService; private readonly IMemberTypeService _memberTypeService; private readonly ITwoFactorLoginService _twoFactorLoginService; + private readonly IMemberGroupService _memberGroupService; public MemberPresentationFactory( IUmbracoMapper umbracoMapper, IMemberService memberService, IMemberTypeService memberTypeService, - ITwoFactorLoginService twoFactorLoginService) + ITwoFactorLoginService twoFactorLoginService, + IMemberGroupService memberGroupService) { _umbracoMapper = umbracoMapper; _memberService = memberService; _memberTypeService = memberTypeService; _twoFactorLoginService = twoFactorLoginService; + _memberGroupService = memberGroupService; } public async Task CreateResponseModelAsync(IMember member, IUser currentUser) @@ -36,8 +39,10 @@ internal sealed class MemberPresentationFactory : IMemberPresentationFactory MemberResponseModel responseModel = _umbracoMapper.Map(member)!; responseModel.IsTwoFactorEnabled = await _twoFactorLoginService.IsTwoFactorEnabledAsync(member.Key); - responseModel.Groups = _memberService.GetAllRoles(member.Username); + IEnumerable roles = _memberService.GetAllRoles(member.Username); + // Get the member groups per role, so we can return the group keys + responseModel.Groups = roles.Select(x => _memberGroupService.GetByName(x)).WhereNotNull().Select(x => x.Key).ToArray(); return currentUser.HasAccessToSensitiveData() ? responseModel : await RemoveSensitiveDataAsync(member, responseModel); diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/Member/CreateMemberRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/Member/CreateMemberRequestModel.cs index a7460bdad7..01844f9c21 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/Member/CreateMemberRequestModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/Member/CreateMemberRequestModel.cs @@ -12,7 +12,7 @@ public class CreateMemberRequestModel : CreateContentRequestModelBase? Groups { get; set; } + public IEnumerable? Groups { get; set; } public bool IsApproved { get; set; } } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/Member/MemberResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/Member/MemberResponseModel.cs index f9521bdba4..13cff882c0 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/Member/MemberResponseModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/Member/MemberResponseModel.cs @@ -25,5 +25,5 @@ public class MemberResponseModel : ContentResponseModelBase Groups { get; set; } = []; + public IEnumerable Groups { get; set; } = []; } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/Member/UpdateMemberRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/Member/UpdateMemberRequestModel.cs index 5bfd3afb6e..6ac0d59816 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/Member/UpdateMemberRequestModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/Member/UpdateMemberRequestModel.cs @@ -12,7 +12,7 @@ public class UpdateMemberRequestModel : UpdateContentRequestModelBase? Groups { get; set; } + public IEnumerable? Groups { get; set; } public bool IsApproved { get; set; } diff --git a/src/Umbraco.Core/Models/ContentEditing/MemberEditingModelBase.cs b/src/Umbraco.Core/Models/ContentEditing/MemberEditingModelBase.cs index 0a3020ecb3..057baecd70 100644 --- a/src/Umbraco.Core/Models/ContentEditing/MemberEditingModelBase.cs +++ b/src/Umbraco.Core/Models/ContentEditing/MemberEditingModelBase.cs @@ -4,7 +4,7 @@ public abstract class MemberEditingModelBase : ContentEditingModelBase { public bool IsApproved { get; set; } - public IEnumerable? Roles { get; set; } + public IEnumerable? Roles { get; set; } public string Email { get; set; } = string.Empty; diff --git a/src/Umbraco.Infrastructure/Services/MemberEditingService.cs b/src/Umbraco.Infrastructure/Services/MemberEditingService.cs index d2c6f52888..68c94ec37a 100644 --- a/src/Umbraco.Infrastructure/Services/MemberEditingService.cs +++ b/src/Umbraco.Infrastructure/Services/MemberEditingService.cs @@ -18,6 +18,7 @@ internal sealed class MemberEditingService : IMemberEditingService private readonly ITwoFactorLoginService _twoFactorLoginService; private readonly IPasswordChanger _passwordChanger; private readonly ILogger _logger; + private readonly IMemberGroupService _memberGroupService; public MemberEditingService( IMemberService memberService, @@ -26,7 +27,8 @@ internal sealed class MemberEditingService : IMemberEditingService IMemberManager memberManager, ITwoFactorLoginService twoFactorLoginService, IPasswordChanger passwordChanger, - ILogger logger) + ILogger logger, + IMemberGroupService memberGroupService) { _memberService = memberService; _memberTypeService = memberTypeService; @@ -35,6 +37,7 @@ internal sealed class MemberEditingService : IMemberEditingService _twoFactorLoginService = twoFactorLoginService; _passwordChanger = passwordChanger; _logger = logger; + _memberGroupService = memberGroupService; } public async Task GetAsync(Guid key) @@ -246,8 +249,20 @@ internal sealed class MemberEditingService : IMemberEditingService return MemberEditingOperationStatus.Success; } - private async Task UpdateRoles(IEnumerable? roles, MemberIdentityUser identityMember) + private async Task UpdateRoles(IEnumerable? roles, MemberIdentityUser identityMember) { + // We have to convert the GUIDS to names here, as roles on a member are stored by name, not key. + var memberGroups = new List(); + foreach (Guid key in roles ?? Enumerable.Empty()) + { + + IMemberGroup? group = await _memberGroupService.GetAsync(key); + if (group is not null) + { + memberGroups.Add(group); + } + } + // 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 // their handlers. If we don't look this up now there's a chance we'll just end up @@ -255,7 +270,8 @@ internal sealed class MemberEditingService : IMemberEditingService IEnumerable currentRoles = (await _memberManager.GetRolesAsync(identityMember)).ToList(); // find the ones to remove and remove them - var rolesToRemove = currentRoles.Except(roles ?? Enumerable.Empty()).ToArray(); + IEnumerable memberGroupNames = memberGroups.Select(x => x.Name).WhereNotNull().ToArray(); + var rolesToRemove = currentRoles.Except(memberGroupNames).ToArray(); // Now let's do the role provider stuff - now that we've saved the content item (that is important since // if we are changing the username, it must be persisted before looking up the member roles). @@ -270,8 +286,8 @@ internal sealed class MemberEditingService : IMemberEditingService } // find the ones to add and add them - var rolesToAdd = roles?.Except(currentRoles).ToArray(); - if (rolesToAdd?.Any() is true) + var rolesToAdd = memberGroupNames.Except(currentRoles).ToArray(); + if (rolesToAdd.Any()) { // add the ones submitted IdentityResult identityResult = await _memberManager.AddToRolesAsync(identityMember, rolesToAdd); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs index 5e25d01a50..0727f9c0ad 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs @@ -25,6 +25,8 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest private IUserService UserService => GetRequiredService(); + private IMemberGroupService MemberGroupService => GetRequiredService(); + [Test] public async Task Can_Create_Member() { @@ -129,6 +131,7 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest { MemberService.AddRole("RoleTwo"); MemberService.AddRole("RoleThree"); + var groups = new[] { MemberGroupService.GetByName("RoleTwo"), MemberGroupService.GetByName("RoleThree") }; var member = await CreateMemberAsync(); @@ -138,7 +141,7 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest Username = member.Username, IsApproved = true, InvariantName = member.Name, - Roles = new [] { "RoleTwo", "RoleThree" } + Roles = groups.Select(x => x.Key), }; var result = await MemberEditingService.UpdateAsync(member.Key, updateModel, SuperUser()); @@ -432,6 +435,7 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest memberType.SetIsSensitiveProperty("title", titleIsSensitive); MemberTypeService.Save(memberType); MemberService.AddRole("RoleOne"); + var group = MemberGroupService.GetByName("RoleOne"); var createModel = new MemberCreateModel { @@ -441,7 +445,7 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest Password = "SuperSecret123", IsApproved = true, ContentTypeKey = memberType.Key, - Roles = new [] { "RoleOne" }, + Roles = new [] { group.Key }, InvariantName = "T. Est", InvariantProperties = new[] {