From 4dca7495f87b216bb294a77b623b1b1de6152995 Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Mon, 25 Mar 2024 16:56:13 +0100 Subject: [PATCH] Handle sensitive properties in the Management API (#15936) * Handle sensitive properties in the Management API * Use Assert.Multiple to catch all failing tests in one run --------- Co-authored-by: Sven Geusens --- .../DocumentTypeControllerBase.cs | 4 + .../Member/ByKeyMemberController.cs | 10 +- .../Filter/FilterMemberFilterController.cs | 8 +- .../MemberType/ByKeyMemberTypeController.cs | 16 +- .../MemberTypeBuilderExtensions.cs | 1 + .../Factories/IMemberPresentationFactory.cs | 5 +- .../IMemberTypePresentationFactory.cs | 9 + .../Factories/MemberPresentationFactory.cs | 41 +++- .../MemberTypeEditingPresentationFactory.cs | 23 ++ .../MemberTypePresentationFactory.cs | 28 +++ .../Factories/UserPresentationFactory.cs | 4 +- src/Umbraco.Cms.Api.Management/OpenApi.json | 110 +++++++++- ...reateMemberTypePropertyTypeRequestModel.cs | 6 +- .../MemberTypePropertyTypeModelBase.cs | 10 + .../MemberTypePropertyTypeResponseModel.cs | 6 +- .../MemberTypePropertyTypeVisibility.cs | 8 + ...pdateMemberTypePropertyTypeRequestModel.cs | 6 +- .../User/Current/CurrentUserResponseModel.cs | 4 + .../Extensions/MemberTypeExtensions.cs | 12 + .../MemberTypePropertyTypeModel.cs | 5 + .../MemberTypeEditingService.cs | 68 +++++- .../Services/MemberContentEditingService.cs | 67 +++++- .../ContentTypeOperationStatus.cs | 1 + .../Services/MemberEditingService.cs | 14 +- .../ContentTypeEditingServiceTestsBase.cs | 6 +- .../Services/MemberTypeEditingServiceTests.cs | 205 ++++++++++++++++++ .../Services/MemberEditingServiceTests.cs | 164 +++++++++++++- 27 files changed, 788 insertions(+), 53 deletions(-) create mode 100644 src/Umbraco.Cms.Api.Management/Factories/IMemberTypePresentationFactory.cs create mode 100644 src/Umbraco.Cms.Api.Management/Factories/MemberTypePresentationFactory.cs create mode 100644 src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeModelBase.cs create mode 100644 src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeVisibility.cs create mode 100644 src/Umbraco.Core/Extensions/MemberTypeExtensions.cs create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MemberTypeEditingServiceTests.cs diff --git a/src/Umbraco.Cms.Api.Management/Controllers/DocumentType/DocumentTypeControllerBase.cs b/src/Umbraco.Cms.Api.Management/Controllers/DocumentType/DocumentTypeControllerBase.cs index 83b9fc5714..2fe34df7a1 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/DocumentType/DocumentTypeControllerBase.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/DocumentType/DocumentTypeControllerBase.cs @@ -73,6 +73,10 @@ public abstract class DocumentTypeControllerBase : ManagementApiControllerBase .WithTitle("Duplicate property type alias") .WithDetail("One or more property type aliases are already in use, all property type aliases must be unique.") .Build()), + ContentTypeOperationStatus.NotAllowed => new BadRequestObjectResult(problemDetailsBuilder + .WithTitle("Operation not permitted") + .WithDetail("The attempted operation was not permitted, likely due to a permission/configuration mismatch with the operation.") + .Build()), _ => new ObjectResult("Unknown content type operation status") { StatusCode = StatusCodes.Status500InternalServerError }, }); diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Member/ByKeyMemberController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Member/ByKeyMemberController.cs index 5da3383f93..5f29df4eca 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Member/ByKeyMemberController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Member/ByKeyMemberController.cs @@ -4,6 +4,7 @@ using Microsoft.AspNetCore.Mvc; using Umbraco.Cms.Api.Management.Factories; using Umbraco.Cms.Api.Management.ViewModels.Member; using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; namespace Umbraco.Cms.Api.Management.Controllers.Member; @@ -13,11 +14,16 @@ public class ByKeyMemberController : MemberControllerBase { private readonly IMemberEditingService _memberEditingService; private readonly IMemberPresentationFactory _memberPresentationFactory; + private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor; - public ByKeyMemberController(IMemberEditingService memberEditingService, IMemberPresentationFactory memberPresentationFactory) + public ByKeyMemberController( + IMemberEditingService memberEditingService, + IMemberPresentationFactory memberPresentationFactory, + IBackOfficeSecurityAccessor backOfficeSecurityAccessor) { _memberEditingService = memberEditingService; _memberPresentationFactory = memberPresentationFactory; + _backOfficeSecurityAccessor = backOfficeSecurityAccessor; } [HttpGet("{id:guid}")] @@ -32,7 +38,7 @@ public class ByKeyMemberController : MemberControllerBase return MemberNotFound(); } - MemberResponseModel model = await _memberPresentationFactory.CreateResponseModelAsync(member); + MemberResponseModel model = await _memberPresentationFactory.CreateResponseModelAsync(member, CurrentUser(_backOfficeSecurityAccessor)); return Ok(model); } } diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Member/Filter/FilterMemberFilterController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Member/Filter/FilterMemberFilterController.cs index 8c858d7c33..2ca5640763 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Member/Filter/FilterMemberFilterController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Member/Filter/FilterMemberFilterController.cs @@ -7,6 +7,7 @@ using Umbraco.Cms.Api.Management.ViewModels.Member; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Membership; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; namespace Umbraco.Cms.Api.Management.Controllers.Member.Filter; @@ -16,13 +17,16 @@ public class FilterMemberFilterController : MemberFilterControllerBase { private readonly IMemberService _memberService; private readonly IMemberPresentationFactory _memberPresentationFactory; + private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor; public FilterMemberFilterController( IMemberService memberService, - IMemberPresentationFactory memberPresentationFactory) + IMemberPresentationFactory memberPresentationFactory, + IBackOfficeSecurityAccessor backOfficeSecurityAccessor) { _memberService = memberService; _memberPresentationFactory = memberPresentationFactory; + _backOfficeSecurityAccessor = backOfficeSecurityAccessor; } [HttpGet] @@ -53,7 +57,7 @@ public class FilterMemberFilterController : MemberFilterControllerBase var pageViewModel = new PagedViewModel { - Items = await _memberPresentationFactory.CreateMultipleAsync(members.Items), + Items = await _memberPresentationFactory.CreateMultipleAsync(members.Items, CurrentUser(_backOfficeSecurityAccessor)), Total = members.Total, }; diff --git a/src/Umbraco.Cms.Api.Management/Controllers/MemberType/ByKeyMemberTypeController.cs b/src/Umbraco.Cms.Api.Management/Controllers/MemberType/ByKeyMemberTypeController.cs index 6e6e057d27..27ab56bf45 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/MemberType/ByKeyMemberTypeController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/MemberType/ByKeyMemberTypeController.cs @@ -1,8 +1,8 @@ using Asp.Versioning; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; +using Umbraco.Cms.Api.Management.Factories; using Umbraco.Cms.Api.Management.ViewModels.MemberType; -using Umbraco.Cms.Core.Mapping; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Services.OperationStatus; @@ -13,12 +13,12 @@ namespace Umbraco.Cms.Api.Management.Controllers.MemberType; public class ByKeyMemberTypeController : MemberTypeControllerBase { private readonly IMemberTypeService _memberTypeService; - private readonly IUmbracoMapper _umbracoMapper; + private readonly IMemberTypePresentationFactory _memberTypePresentationFactory; - public ByKeyMemberTypeController(IMemberTypeService memberTypeService, IUmbracoMapper umbracoMapper) + public ByKeyMemberTypeController(IMemberTypeService memberTypeService, IMemberTypePresentationFactory memberTypePresentationFactory) { _memberTypeService = memberTypeService; - _umbracoMapper = umbracoMapper; + _memberTypePresentationFactory = memberTypePresentationFactory; } [HttpGet("{id:guid}")] @@ -27,13 +27,13 @@ public class ByKeyMemberTypeController : MemberTypeControllerBase [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] public async Task ByKey(Guid id) { - IMemberType? MemberType = await _memberTypeService.GetAsync(id); - if (MemberType == null) + IMemberType? memberType = await _memberTypeService.GetAsync(id); + if (memberType is null) { return OperationStatusResult(ContentTypeOperationStatus.NotFound); } - MemberTypeResponseModel model = _umbracoMapper.Map(MemberType)!; - return await Task.FromResult(Ok(model)); + MemberTypeResponseModel model = await _memberTypePresentationFactory.CreateResponseModelAsync(memberType); + return Ok(model); } } diff --git a/src/Umbraco.Cms.Api.Management/DependencyInjection/MemberTypeBuilderExtensions.cs b/src/Umbraco.Cms.Api.Management/DependencyInjection/MemberTypeBuilderExtensions.cs index 2cd581fbca..d8304e7549 100644 --- a/src/Umbraco.Cms.Api.Management/DependencyInjection/MemberTypeBuilderExtensions.cs +++ b/src/Umbraco.Cms.Api.Management/DependencyInjection/MemberTypeBuilderExtensions.cs @@ -10,6 +10,7 @@ internal static class MemberTypeBuilderExtensions { internal static IUmbracoBuilder AddMemberTypes(this IUmbracoBuilder builder) { + builder.Services.AddTransient(); builder.Services.AddTransient(); builder.WithCollectionBuilder().Add(); diff --git a/src/Umbraco.Cms.Api.Management/Factories/IMemberPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/IMemberPresentationFactory.cs index 90f0e22c55..21f2099f46 100644 --- a/src/Umbraco.Cms.Api.Management/Factories/IMemberPresentationFactory.cs +++ b/src/Umbraco.Cms.Api.Management/Factories/IMemberPresentationFactory.cs @@ -4,14 +4,15 @@ using Umbraco.Cms.Api.Management.ViewModels.Member.Item; using Umbraco.Cms.Api.Management.ViewModels.MemberType; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Entities; +using Umbraco.Cms.Core.Models.Membership; namespace Umbraco.Cms.Api.Management.Factories; public interface IMemberPresentationFactory { - Task CreateResponseModelAsync(IMember member); + Task CreateResponseModelAsync(IMember member, IUser currentUser); - Task> CreateMultipleAsync(IEnumerable members); + Task> CreateMultipleAsync(IEnumerable members, IUser currentUser); MemberItemResponseModel CreateItemResponseModel(IMemberEntitySlim entity); diff --git a/src/Umbraco.Cms.Api.Management/Factories/IMemberTypePresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/IMemberTypePresentationFactory.cs new file mode 100644 index 0000000000..32ca32d3f4 --- /dev/null +++ b/src/Umbraco.Cms.Api.Management/Factories/IMemberTypePresentationFactory.cs @@ -0,0 +1,9 @@ +using Umbraco.Cms.Api.Management.ViewModels.MemberType; +using Umbraco.Cms.Core.Models; + +namespace Umbraco.Cms.Api.Management.Factories; + +public interface IMemberTypePresentationFactory +{ + Task CreateResponseModelAsync(IMemberType memberType); +} diff --git a/src/Umbraco.Cms.Api.Management/Factories/MemberPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/MemberPresentationFactory.cs index d0905e4e1a..34d1c46b0a 100644 --- a/src/Umbraco.Cms.Api.Management/Factories/MemberPresentationFactory.cs +++ b/src/Umbraco.Cms.Api.Management/Factories/MemberPresentationFactory.cs @@ -2,10 +2,13 @@ using Umbraco.Cms.Api.Management.ViewModels.Member; using Umbraco.Cms.Api.Management.ViewModels.Member.Item; using Umbraco.Cms.Api.Management.ViewModels.MemberType; +using Umbraco.Cms.Core.Extensions; using Umbraco.Cms.Core.Mapping; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Entities; +using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.Services; +using Umbraco.Extensions; namespace Umbraco.Cms.Api.Management.Factories; @@ -13,34 +16,39 @@ internal sealed class MemberPresentationFactory : IMemberPresentationFactory { private readonly IUmbracoMapper _umbracoMapper; private readonly IMemberService _memberService; + private readonly IMemberTypeService _memberTypeService; private readonly ITwoFactorLoginService _twoFactorLoginService; public MemberPresentationFactory( IUmbracoMapper umbracoMapper, IMemberService memberService, + IMemberTypeService memberTypeService, ITwoFactorLoginService twoFactorLoginService) { _umbracoMapper = umbracoMapper; _memberService = memberService; + _memberTypeService = memberTypeService; _twoFactorLoginService = twoFactorLoginService; } - public async Task CreateResponseModelAsync(IMember member) + public async Task CreateResponseModelAsync(IMember member, IUser currentUser) { MemberResponseModel responseModel = _umbracoMapper.Map(member)!; responseModel.IsTwoFactorEnabled = await _twoFactorLoginService.IsTwoFactorEnabledAsync(member.Key); responseModel.Groups = _memberService.GetAllRoles(member.Username); - return responseModel; + return currentUser.HasAccessToSensitiveData() + ? responseModel + : await RemoveSensitiveDataAsync(member, responseModel); } - public async Task> CreateMultipleAsync(IEnumerable members) + public async Task> CreateMultipleAsync(IEnumerable members, IUser currentUser) { var memberResponseModels = new List(); foreach (IMember member in members) { - memberResponseModels.Add(await CreateResponseModelAsync(member)); + memberResponseModels.Add(await CreateResponseModelAsync(member, currentUser)); } return memberResponseModels; @@ -72,4 +80,29 @@ internal sealed class MemberPresentationFactory : IMemberPresentationFactory public MemberTypeReferenceResponseModel CreateMemberTypeReferenceResponseModel(IMemberEntitySlim entity) => _umbracoMapper.Map(entity)!; + + private async Task RemoveSensitiveDataAsync(IMember member, MemberResponseModel responseModel) + { + // these properties are considered sensitive; some of them are not nullable, so for + // those we can't do much more than force revert them to their default values. + responseModel.IsApproved = false; + responseModel.IsLockedOut = false; + responseModel.IsTwoFactorEnabled = false; + responseModel.FailedPasswordAttempts = 0; + responseModel.LastLoginDate = null; + responseModel.LastLockoutDate = null; + responseModel.LastPasswordChangeDate = null; + + IMemberType memberType = await _memberTypeService.GetAsync(member.ContentType.Key) + ?? throw new InvalidOperationException($"The member type {member.ContentType.Alias} could not be found"); + + var sensitivePropertyAliases = memberType.GetSensitivePropertyTypeAliases().ToArray(); + + // remove all properties whose property types are flagged as sensitive + responseModel.Values = responseModel.Values + .Where(valueModel => sensitivePropertyAliases.InvariantContains(valueModel.Alias) is false) + .ToArray(); + + return responseModel; + } } diff --git a/src/Umbraco.Cms.Api.Management/Factories/MemberTypeEditingPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/MemberTypeEditingPresentationFactory.cs index 7f4e391b46..77ef127467 100644 --- a/src/Umbraco.Cms.Api.Management/Factories/MemberTypeEditingPresentationFactory.cs +++ b/src/Umbraco.Cms.Api.Management/Factories/MemberTypeEditingPresentationFactory.cs @@ -25,6 +25,8 @@ internal sealed class MemberTypeEditingPresentationFactory : ContentTypeEditingP createModel.Key = requestModel.Id; createModel.Compositions = MapCompositions(requestModel.Compositions); + MapPropertyTypeSensitivityAndVisibility(createModel.Properties, requestModel.Properties); + return createModel; } @@ -40,6 +42,8 @@ internal sealed class MemberTypeEditingPresentationFactory : ContentTypeEditingP updateModel.Compositions = MapCompositions(requestModel.Compositions); + MapPropertyTypeSensitivityAndVisibility(updateModel.Properties, requestModel.Properties); + return updateModel; } @@ -50,4 +54,23 @@ internal sealed class MemberTypeEditingPresentationFactory : ContentTypeEditingP => MapCompositions(documentTypeCompositions .DistinctBy(c => c.MemberType.Id) .ToDictionary(c => c.MemberType.Id, c => c.CompositionType)); + + private void MapPropertyTypeSensitivityAndVisibility( + IEnumerable propertyTypes, + IEnumerable requestPropertyTypes) + where TRequestPropertyTypeModel : MemberTypePropertyTypeModelBase + { + var requestModelPropertiesByAlias = requestPropertyTypes.ToDictionary(p => p.Alias); + foreach (MemberTypePropertyTypeModel propertyType in propertyTypes) + { + if (requestModelPropertiesByAlias.TryGetValue(propertyType.Alias, out TRequestPropertyTypeModel? requestPropertyType) is false) + { + throw new InvalidOperationException($"Could not find the property type model {propertyType.Alias} in the request"); + } + + propertyType.IsSensitive = requestPropertyType.IsSensitive; + propertyType.MemberCanView = requestPropertyType.Visibility.MemberCanView; + propertyType.MemberCanEdit = requestPropertyType.Visibility.MemberCanEdit; + } + } } diff --git a/src/Umbraco.Cms.Api.Management/Factories/MemberTypePresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/MemberTypePresentationFactory.cs new file mode 100644 index 0000000000..0b055d417c --- /dev/null +++ b/src/Umbraco.Cms.Api.Management/Factories/MemberTypePresentationFactory.cs @@ -0,0 +1,28 @@ +using Umbraco.Cms.Api.Management.ViewModels.MemberType; +using Umbraco.Cms.Core.Mapping; +using Umbraco.Cms.Core.Models; + +namespace Umbraco.Cms.Api.Management.Factories; + +internal sealed class MemberTypePresentationFactory : IMemberTypePresentationFactory +{ + private readonly IUmbracoMapper _umbracoMapper; + + public MemberTypePresentationFactory(IUmbracoMapper umbracoMapper) + => _umbracoMapper = umbracoMapper; + + public Task CreateResponseModelAsync(IMemberType memberType) + { + MemberTypeResponseModel model = _umbracoMapper.Map(memberType)!; + + foreach (MemberTypePropertyTypeResponseModel propertyType in model.Properties) + { + propertyType.IsSensitive = memberType.IsSensitiveProperty(propertyType.Alias); + + propertyType.Visibility.MemberCanEdit = memberType.MemberCanEditProperty(propertyType.Alias); + propertyType.Visibility.MemberCanView = memberType.MemberCanViewProperty(propertyType.Alias); + } + + return Task.FromResult(model); + } +} diff --git a/src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs index d95488c728..585b87cc05 100644 --- a/src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs +++ b/src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs @@ -164,6 +164,7 @@ public class UserPresentationFactory : IUserPresentationFactory var hasAccessToAllLanguages = presentationGroups.Any(x => x.HasAccessToAllLanguages); var allowedSections = presentationGroups.SelectMany(x => x.Sections).ToHashSet(); + return await Task.FromResult(new CurrentUserResponseModel() { Id = presentationUser.Id, @@ -178,7 +179,8 @@ public class UserPresentationFactory : IUserPresentationFactory Permissions = permissions, FallbackPermissions = fallbackPermissions, HasAccessToAllLanguages = hasAccessToAllLanguages, - AllowedSections = allowedSections + HasAccessToSensitiveData = user.HasAccessToSensitiveData(), + AllowedSections = allowedSections, }); } diff --git a/src/Umbraco.Cms.Api.Management/OpenApi.json b/src/Umbraco.Cms.Api.Management/OpenApi.json index 930878dc0e..4382cc29bc 100644 --- a/src/Umbraco.Cms.Api.Management/OpenApi.json +++ b/src/Umbraco.Cms.Api.Management/OpenApi.json @@ -34497,7 +34497,7 @@ "type": "object", "allOf": [ { - "$ref": "#/components/schemas/PropertyTypeModelBaseModel" + "$ref": "#/components/schemas/MemberTypePropertyTypeModelBaseModel" } ], "additionalProperties": false @@ -34739,6 +34739,7 @@ "email", "fallbackPermissions", "hasAccessToAllLanguages", + "hasAccessToSensitiveData", "id", "languages", "mediaStartNodeIds", @@ -34796,6 +34797,9 @@ "hasAccessToAllLanguages": { "type": "boolean" }, + "hasAccessToSensitiveData": { + "type": "boolean" + }, "fallbackPermissions": { "uniqueItems": true, "type": "array", @@ -37652,15 +37656,115 @@ ], "additionalProperties": false }, + "MemberTypePropertyTypeModelBaseModel": { + "required": [ + "alias", + "appearance", + "dataType", + "id", + "isSensitive", + "name", + "sortOrder", + "validation", + "variesByCulture", + "variesBySegment", + "visibility" + ], + "type": "object", + "properties": { + "id": { + "type": "string", + "format": "uuid" + }, + "container": { + "oneOf": [ + { + "$ref": "#/components/schemas/ReferenceByIdModel" + } + ], + "nullable": true + }, + "sortOrder": { + "type": "integer", + "format": "int32" + }, + "alias": { + "minLength": 1, + "type": "string" + }, + "name": { + "minLength": 1, + "type": "string" + }, + "description": { + "type": "string", + "nullable": true + }, + "dataType": { + "oneOf": [ + { + "$ref": "#/components/schemas/ReferenceByIdModel" + } + ] + }, + "variesByCulture": { + "type": "boolean" + }, + "variesBySegment": { + "type": "boolean" + }, + "validation": { + "oneOf": [ + { + "$ref": "#/components/schemas/PropertyTypeValidationModel" + } + ] + }, + "appearance": { + "oneOf": [ + { + "$ref": "#/components/schemas/PropertyTypeAppearanceModel" + } + ] + }, + "isSensitive": { + "type": "boolean" + }, + "visibility": { + "oneOf": [ + { + "$ref": "#/components/schemas/MemberTypePropertyTypeVisibilityModel" + } + ] + } + }, + "additionalProperties": false + }, "MemberTypePropertyTypeResponseModel": { "type": "object", "allOf": [ { - "$ref": "#/components/schemas/PropertyTypeModelBaseModel" + "$ref": "#/components/schemas/MemberTypePropertyTypeModelBaseModel" } ], "additionalProperties": false }, + "MemberTypePropertyTypeVisibilityModel": { + "required": [ + "memberCanEdit", + "memberCanView" + ], + "type": "object", + "properties": { + "memberCanView": { + "type": "boolean" + }, + "memberCanEdit": { + "type": "boolean" + } + }, + "additionalProperties": false + }, "MemberTypeReferenceResponseModel": { "type": "object", "allOf": [ @@ -41472,7 +41576,7 @@ "type": "object", "allOf": [ { - "$ref": "#/components/schemas/PropertyTypeModelBaseModel" + "$ref": "#/components/schemas/MemberTypePropertyTypeModelBaseModel" } ], "additionalProperties": false diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/CreateMemberTypePropertyTypeRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/CreateMemberTypePropertyTypeRequestModel.cs index 66828c040e..a9fe229e0f 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/CreateMemberTypePropertyTypeRequestModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/CreateMemberTypePropertyTypeRequestModel.cs @@ -1,7 +1,5 @@ -using Umbraco.Cms.Api.Management.ViewModels.ContentType; +namespace Umbraco.Cms.Api.Management.ViewModels.MemberType; -namespace Umbraco.Cms.Api.Management.ViewModels.MemberType; - -public class CreateMemberTypePropertyTypeRequestModel : PropertyTypeModelBase +public class CreateMemberTypePropertyTypeRequestModel : MemberTypePropertyTypeModelBase { } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeModelBase.cs b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeModelBase.cs new file mode 100644 index 0000000000..67a7e9b901 --- /dev/null +++ b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeModelBase.cs @@ -0,0 +1,10 @@ +using Umbraco.Cms.Api.Management.ViewModels.ContentType; + +namespace Umbraco.Cms.Api.Management.ViewModels.MemberType; + +public abstract class MemberTypePropertyTypeModelBase : PropertyTypeModelBase +{ + public bool IsSensitive { get; set; } + + public MemberTypePropertyTypeVisibility Visibility { get; set; } = new(); +} diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeResponseModel.cs index 5337423d81..d80bae34ae 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeResponseModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeResponseModel.cs @@ -1,7 +1,5 @@ -using Umbraco.Cms.Api.Management.ViewModels.ContentType; +namespace Umbraco.Cms.Api.Management.ViewModels.MemberType; -namespace Umbraco.Cms.Api.Management.ViewModels.MemberType; - -public class MemberTypePropertyTypeResponseModel : PropertyTypeModelBase +public class MemberTypePropertyTypeResponseModel : MemberTypePropertyTypeModelBase { } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeVisibility.cs b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeVisibility.cs new file mode 100644 index 0000000000..7fe041f6ee --- /dev/null +++ b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/MemberTypePropertyTypeVisibility.cs @@ -0,0 +1,8 @@ +namespace Umbraco.Cms.Api.Management.ViewModels.MemberType; + +public class MemberTypePropertyTypeVisibility +{ + public bool MemberCanView { get; set; } + + public bool MemberCanEdit { get; set; } +} diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/UpdateMemberTypePropertyTypeRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/UpdateMemberTypePropertyTypeRequestModel.cs index 4aec003899..909692983a 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/UpdateMemberTypePropertyTypeRequestModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/MemberType/UpdateMemberTypePropertyTypeRequestModel.cs @@ -1,7 +1,5 @@ -using Umbraco.Cms.Api.Management.ViewModels.ContentType; +namespace Umbraco.Cms.Api.Management.ViewModels.MemberType; -namespace Umbraco.Cms.Api.Management.ViewModels.MemberType; - -public class UpdateMemberTypePropertyTypeRequestModel : PropertyTypeModelBase +public class UpdateMemberTypePropertyTypeRequestModel : MemberTypePropertyTypeModelBase { } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/User/Current/CurrentUserResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/User/Current/CurrentUserResponseModel.cs index d3b5477a7c..994a4c5e03 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/User/Current/CurrentUserResponseModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/User/Current/CurrentUserResponseModel.cs @@ -26,7 +26,11 @@ public class CurrentUserResponseModel public required bool HasAccessToAllLanguages { get; init; } + public required bool HasAccessToSensitiveData { get; set; } + public required ISet FallbackPermissions { get; init; } + public required ISet Permissions { get; init; } + public required ISet AllowedSections { get; init; } } diff --git a/src/Umbraco.Core/Extensions/MemberTypeExtensions.cs b/src/Umbraco.Core/Extensions/MemberTypeExtensions.cs new file mode 100644 index 0000000000..6c3209a52e --- /dev/null +++ b/src/Umbraco.Core/Extensions/MemberTypeExtensions.cs @@ -0,0 +1,12 @@ +using Umbraco.Cms.Core.Models; + +namespace Umbraco.Cms.Core.Extensions; + +public static class MemberTypeExtensions +{ + public static IEnumerable GetSensitivePropertyTypes(this IMemberType memberType) + => memberType.CompositionPropertyTypes.Where(p => memberType.IsSensitiveProperty(p.Alias)); + + public static IEnumerable GetSensitivePropertyTypeAliases(this IMemberType memberType) + => memberType.GetSensitivePropertyTypes().Select(p => p.Alias); +} diff --git a/src/Umbraco.Core/Models/ContentTypeEditing/MemberTypePropertyTypeModel.cs b/src/Umbraco.Core/Models/ContentTypeEditing/MemberTypePropertyTypeModel.cs index db2462f94d..9ab2a356c0 100644 --- a/src/Umbraco.Core/Models/ContentTypeEditing/MemberTypePropertyTypeModel.cs +++ b/src/Umbraco.Core/Models/ContentTypeEditing/MemberTypePropertyTypeModel.cs @@ -2,4 +2,9 @@ public class MemberTypePropertyTypeModel : PropertyTypeModelBase { + public bool IsSensitive { get; set; } + + public bool MemberCanView { get; set; } + + public bool MemberCanEdit { get; set; } } diff --git a/src/Umbraco.Core/Services/ContentTypeEditing/MemberTypeEditingService.cs b/src/Umbraco.Core/Services/ContentTypeEditing/MemberTypeEditingService.cs index 240fd6b015..60707728af 100644 --- a/src/Umbraco.Core/Services/ContentTypeEditing/MemberTypeEditingService.cs +++ b/src/Umbraco.Core/Services/ContentTypeEditing/MemberTypeEditingService.cs @@ -1,5 +1,6 @@ using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentTypeEditing; +using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.Services.OperationStatus; using Umbraco.Cms.Core.Strings; @@ -8,37 +9,58 @@ namespace Umbraco.Cms.Core.Services.ContentTypeEditing; internal sealed class MemberTypeEditingService : ContentTypeEditingServiceBase, IMemberTypeEditingService { private readonly IMemberTypeService _memberTypeService; + private readonly IUserService _userService; public MemberTypeEditingService( IContentTypeService contentTypeService, IMemberTypeService memberTypeService, IDataTypeService dataTypeService, IEntityService entityService, - IShortStringHelper shortStringHelper) + IShortStringHelper shortStringHelper, + IUserService userService) : base(contentTypeService, memberTypeService, dataTypeService, entityService, shortStringHelper) - => _memberTypeService = memberTypeService; + { + _memberTypeService = memberTypeService; + _userService = userService; + } public async Task> CreateAsync(MemberTypeCreateModel model, Guid userKey) { Attempt result = await ValidateAndMapForCreationAsync(model, model.Key, containerKey: null); - if (result.Success) + if (result.Success is false) { - IMemberType memberType = result.Result ?? throw new InvalidOperationException($"{nameof(ValidateAndMapForCreationAsync)} succeeded but did not yield any result"); - await _memberTypeService.SaveAsync(memberType, userKey); + return result; } + IMemberType memberType = result.Result ?? throw new InvalidOperationException($"{nameof(ValidateAndMapForCreationAsync)} succeeded but did not yield any result"); + if (await UpdatePropertyTypeSensitivity(memberType, model, userKey) is false) + { + return Attempt.FailWithStatus(ContentTypeOperationStatus.NotAllowed, memberType); + } + + UpdatePropertyTypeVisibility(memberType, model); + + await _memberTypeService.SaveAsync(memberType, userKey); return result; } public async Task> UpdateAsync(IMemberType memberType, MemberTypeUpdateModel model, Guid userKey) { Attempt result = await ValidateAndMapForUpdateAsync(memberType, model); - if (result.Success) + if (result.Success is false) { - memberType = result.Result ?? throw new InvalidOperationException($"{nameof(ValidateAndMapForUpdateAsync)} succeeded but did not yield any result"); - await _memberTypeService.SaveAsync(memberType, userKey); + return result; } + memberType = result.Result ?? throw new InvalidOperationException($"{nameof(ValidateAndMapForUpdateAsync)} succeeded but did not yield any result"); + if (await UpdatePropertyTypeSensitivity(memberType, model, userKey) is false) + { + return Attempt.FailWithStatus(ContentTypeOperationStatus.NotAllowed, memberType); + } + + UpdatePropertyTypeVisibility(memberType, model); + + await _memberTypeService.SaveAsync(memberType, userKey); return result; } @@ -56,4 +78,34 @@ internal sealed class MemberTypeEditingService : ContentTypeEditingServiceBase UmbracoObjectTypes.MemberType; protected override UmbracoObjectTypes ContainerObjectType => throw new NotSupportedException("Member type tree does not support containers"); + + private void UpdatePropertyTypeVisibility(IMemberType memberType, MemberTypeModelBase model) + { + foreach (MemberTypePropertyTypeModel propertyType in model.Properties) + { + memberType.SetMemberCanViewProperty(propertyType.Alias, propertyType.MemberCanView); + memberType.SetMemberCanEditProperty(propertyType.Alias, propertyType.MemberCanEdit); + } + } + + private async Task UpdatePropertyTypeSensitivity(IMemberType memberType, MemberTypeModelBase model, Guid userKey) + { + IUser user = await _userService.GetAsync(userKey) + ?? throw new ArgumentException($"Could not find a user with the specified user key ({userKey})", nameof(userKey)); + + var canChangeSensitivity = user.HasAccessToSensitiveData(); + + foreach (MemberTypePropertyTypeModel propertyType in model.Properties) + { + var changed = memberType.IsSensitiveProperty(propertyType.Alias) != propertyType.IsSensitive; + if (changed && canChangeSensitivity is false) + { + return false; + } + + memberType.SetIsSensitiveProperty(propertyType.Alias, propertyType.IsSensitive); + } + + return true; + } } diff --git a/src/Umbraco.Core/Services/MemberContentEditingService.cs b/src/Umbraco.Core/Services/MemberContentEditingService.cs index 285a010676..0d564e71d7 100644 --- a/src/Umbraco.Core/Services/MemberContentEditingService.cs +++ b/src/Umbraco.Core/Services/MemberContentEditingService.cs @@ -1,6 +1,8 @@ using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core.Extensions; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; +using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.PropertyEditors; using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Services.OperationStatus; @@ -11,6 +13,7 @@ internal sealed class MemberContentEditingService : ContentEditingServiceBase, IMemberContentEditingService { private readonly ILogger> _logger; + private readonly IUserService _userService; public MemberContentEditingService( IMemberService contentService, @@ -20,22 +23,42 @@ internal sealed class MemberContentEditingService ILogger> logger, ICoreScopeProvider scopeProvider, IUserIdKeyResolver userIdKeyResolver, - IMemberValidationService memberValidationService) + IMemberValidationService memberValidationService, + IUserService userService) : base(contentService, contentTypeService, propertyEditorCollection, dataTypeService, logger, scopeProvider, userIdKeyResolver, memberValidationService) - => _logger = logger; + { + _logger = logger; + _userService = userService; + } public async Task> ValidateAsync(MemberEditingModelBase editingModel, Guid memberTypeKey) => await ValidatePropertiesAsync(editingModel, memberTypeKey); public async Task> UpdateAsync(IMember member, MemberEditingModelBase updateModel, Guid userKey) { - // FIXME: handle sensitive property data + IMemberType memberType = await ContentTypeService.GetAsync(member.ContentType.Key) + ?? throw new InvalidOperationException($"The member type {member.ContentType.Alias} could not be found."); + + IUser user = await _userService.GetAsync(userKey) + ?? throw new InvalidOperationException("The supplied user key did not match any existing user"); + + if (ValidateAccessToSensitiveProperties(member, memberType, updateModel, user) is false) + { + return Attempt.FailWithStatus(ContentEditingOperationStatus.NotAllowed, new MemberUpdateResult()); + } + + // store any sensitive properties that must be explicitly carried over between updates (due to missing user access to sensitive data) + Dictionary sensitivePropertiesToRetain = FindSensitivePropertiesToRetain(member, memberType, user); + Attempt result = await MapUpdate(member, updateModel); if (result.Success == false) { return Attempt.FailWithStatus(result.Status, result.Result); } + // restore the retained sensitive properties after the base update (they have been removed from the member at this point) + RetainSensitiveProperties(member, sensitivePropertiesToRetain); + // the create mapping might succeed, but this doesn't mean the model is valid at property level. // we'll return the actual property validation status if the entire operation succeeds. ContentEditingOperationStatus validationStatus = result.Status; @@ -51,7 +74,6 @@ internal sealed class MemberContentEditingService public async Task> DeleteAsync(Guid key, Guid userKey) => await HandleDeleteAsync(key, userKey, false); - protected override IMember New(string? name, int parentId, IMemberType memberType) => throw new NotSupportedException("Member creation is not supported by this service. This should never be called."); @@ -88,4 +110,41 @@ internal sealed class MemberContentEditingService return ContentEditingOperationStatus.Unknown; } } + + private bool ValidateAccessToSensitiveProperties(IMember member, IMemberType memberType, MemberEditingModelBase updateModel, IUser user) + { + if (user.HasAccessToSensitiveData()) + { + return true; + } + + var sensitivePropertyAliases = memberType.GetSensitivePropertyTypeAliases().ToArray(); + return updateModel + .InvariantProperties + .Union(updateModel.Variants.SelectMany(variant => variant.Properties)) + .Select(property => property.Alias) + .Intersect(sensitivePropertyAliases, StringComparer.OrdinalIgnoreCase) + .Any() is false; + } + + private Dictionary FindSensitivePropertiesToRetain(IMember member, IMemberType memberType, IUser user) + { + if (user.HasAccessToSensitiveData()) + { + return new Dictionary(); + } + + var sensitivePropertyAliases = memberType.GetSensitivePropertyTypeAliases().ToArray(); + // NOTE: this is explicitly NOT handling variance. if variance becomes a thing for members, this needs to be amended. + return sensitivePropertyAliases.ToDictionary(alias => alias, alias => member.GetValue(alias)); + } + + private void RetainSensitiveProperties(IMember member, Dictionary sensitivePropertyValues) + { + foreach (KeyValuePair sensitiveProperty in sensitivePropertyValues) + { + // NOTE: this is explicitly NOT handling variance. if variance becomes a thing for members, this needs to be amended. + member.SetValue(sensitiveProperty.Key, sensitiveProperty.Value); + } + } } diff --git a/src/Umbraco.Core/Services/OperationStatus/ContentTypeOperationStatus.cs b/src/Umbraco.Core/Services/OperationStatus/ContentTypeOperationStatus.cs index f1b73de5fd..86a32f7fc8 100644 --- a/src/Umbraco.Core/Services/OperationStatus/ContentTypeOperationStatus.cs +++ b/src/Umbraco.Core/Services/OperationStatus/ContentTypeOperationStatus.cs @@ -15,4 +15,5 @@ public enum ContentTypeOperationStatus MissingContainer, DuplicateContainer, NotFound, + NotAllowed } diff --git a/src/Umbraco.Infrastructure/Services/MemberEditingService.cs b/src/Umbraco.Infrastructure/Services/MemberEditingService.cs index 0a98639ad1..d2c6f52888 100644 --- a/src/Umbraco.Infrastructure/Services/MemberEditingService.cs +++ b/src/Umbraco.Infrastructure/Services/MemberEditingService.cs @@ -111,7 +111,17 @@ internal sealed class MemberEditingService : IMemberEditingService if (member is null) { status.ContentEditingOperationStatus = ContentEditingOperationStatus.NotFound; - return Attempt.FailWithStatus(new MemberEditingStatus(), new MemberUpdateResult()); + return Attempt.FailWithStatus(status, new MemberUpdateResult()); + } + + if (user.HasAccessToSensitiveData() is false) + { + // handle sensitive data. certain member properties (IsApproved, IsLockedOut) are subject to "sensitive data" rules. + if (member.IsLockedOut != updateModel.IsLockedOut || member.IsApproved != updateModel.IsApproved) + { + status.ContentEditingOperationStatus = ContentEditingOperationStatus.NotAllowed; + return Attempt.FailWithStatus(status, new MemberUpdateResult()); + } } MemberIdentityUser? identityMember = await _memberManager.FindByIdAsync(member.Id.ToString()); @@ -165,8 +175,6 @@ internal sealed class MemberEditingService : IMemberEditingService return Attempt.FailWithStatus(status, new MemberUpdateResult { Content = member }); } - // FIXME: handle sensitive data. certain properties (IsApproved, IsLockedOut, ...) are subject to "sensitive data" rules. - // reverse engineer what's happening in the old backoffice MemberController and replicate here member.IsLockedOut = updateModel.IsLockedOut; member.IsApproved = updateModel.IsApproved; member.Email = updateModel.Email; diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTestsBase.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTestsBase.cs index 98af01c082..1068f4b9a1 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTestsBase.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTestsBase.cs @@ -131,7 +131,7 @@ public abstract class ContentTypeEditingServiceTestsBase : UmbracoIntegrationTes Guid? key = null) => CreateContainer(name, type, key); - private TModel CreateContentEditingModel( + protected TModel CreateContentEditingModel( string name, string? alias = null, bool isElement = false, @@ -151,7 +151,7 @@ public abstract class ContentTypeEditingServiceTestsBase : UmbracoIntegrationTes IsElement = isElement }; - private TModel CreatePropertyType( + protected TModel CreatePropertyType( string name, string? alias = null, Guid? key = null, @@ -169,7 +169,7 @@ public abstract class ContentTypeEditingServiceTestsBase : UmbracoIntegrationTes Appearance = new PropertyTypeAppearance(), }; - private TModel CreateContainer( + protected TModel CreateContainer( string name, string type = TabContainerType, Guid? key = null) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MemberTypeEditingServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MemberTypeEditingServiceTests.cs new file mode 100644 index 0000000000..5f87ca923f --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MemberTypeEditingServiceTests.cs @@ -0,0 +1,205 @@ +using NUnit.Framework; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Models.ContentTypeEditing; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Core.Services.ContentTypeEditing; +using Umbraco.Cms.Core.Services.OperationStatus; +using Umbraco.Cms.Tests.Common.Builders; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Services; + +/// +/// Tests for the member type editing service. Please notice that a lot of functional test is covered by the content type +/// editing service tests, since these services share the same base implementation. +/// +public class MemberTypeEditingServiceTests : ContentTypeEditingServiceTestsBase +{ + private IMemberTypeEditingService MemberTypeEditingService => GetRequiredService(); + + private IMemberTypeService MemberTypeService => GetRequiredService(); + + private IUserService UserService => GetRequiredService(); + + [Test] + public async Task Can_Create_Sensitive_Properties_With_Sensitive_Data_Access() + { + // arrange + var createModel = MemberTypeCreateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(isSensitive: true) }); + + // act + var result = await MemberTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey); + + // assert + var memberType = await MemberTypeService.GetAsync(result.Result.Key)!; + Assert.Multiple(() => + { + Assert.IsTrue(result.Success); + Assert.IsNotEmpty(memberType.PropertyTypes); + Assert.IsTrue(memberType.PropertyTypes.All(propertyType => memberType.IsSensitiveProperty(propertyType.Alias))); + }); + } + + [TestCase(true)] + [TestCase(false)] + public async Task Can_Change_Property_Sensitivity_With_Sensitive_Data_Access(bool initialIsSensitiveValue) + { + // arrange + var createModel = MemberTypeCreateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(isSensitive: initialIsSensitiveValue) }); + IMemberType memberType = (await MemberTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey)).Result!; + + var newIsSensitiveValue = initialIsSensitiveValue is false; + var updateModel = MemberTypeUpdateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(isSensitive: newIsSensitiveValue) }); + + // act + var result = await MemberTypeEditingService.UpdateAsync(memberType, updateModel, Constants.Security.SuperUserKey); + + // assert + memberType = await MemberTypeService.GetAsync(memberType.Key)!; + Assert.Multiple(() => + { + Assert.IsTrue(result.Success); + Assert.AreEqual(ContentTypeOperationStatus.Success, result.Status); + Assert.IsTrue(memberType.PropertyTypes.All(propertyType => memberType.IsSensitiveProperty(propertyType.Alias) == newIsSensitiveValue)); + }); + } + + [Test] + public async Task Cannot_Create_Sensitive_Properties_Without_Sensitive_Data_Access() + { + // arrange + // this user does NOT have access to sensitive data + var user = UserBuilder.CreateUser(); + UserService.Save(user); + var createModel = MemberTypeCreateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(isSensitive: true) }); + + // act + var result = await MemberTypeEditingService.CreateAsync(createModel, user.Key); + + // assert + var memberType = await MemberTypeService.GetAsync(result.Result.Key)!; + Assert.Multiple(() => + { + Assert.IsFalse(result.Success); + Assert.IsNull(memberType); + }); + } + + [TestCase(true)] + [TestCase(false)] + public async Task Cannot_Change_Property_Sensitivity_Without_Sensitive_Data_Access(bool initialIsSensitiveValue) + { + // arrange + // this user does NOT have access to sensitive data + var user = UserBuilder.CreateUser(); + UserService.Save(user); + + var createModel = MemberTypeCreateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(isSensitive: initialIsSensitiveValue) }); + IMemberType memberType = (await MemberTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey)).Result!; + + var newIsSensitiveValue = initialIsSensitiveValue is false; + var updateModel = MemberTypeUpdateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(isSensitive: newIsSensitiveValue) }); + + // act + var result = await MemberTypeEditingService.UpdateAsync(memberType, updateModel, user.Key); + + // assert + memberType = await MemberTypeService.GetAsync(memberType.Key)!; + Assert.Multiple(() => + { + Assert.IsFalse(result.Success); + Assert.AreEqual(ContentTypeOperationStatus.NotAllowed, result.Status); + Assert.IsTrue(memberType.PropertyTypes.All(propertyType => memberType.IsSensitiveProperty(propertyType.Alias) == initialIsSensitiveValue)); + }); + + } + + [TestCase(true, true)] + [TestCase(true, false)] + [TestCase(false, true)] + [TestCase(false, false)] + public async Task Can_Define_Property_Visibility_When_Creating(bool memberCanView, bool memberCanEdit) + { + // arrange + var createModel = MemberTypeCreateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(memberCanView: memberCanView, memberCanEdit: memberCanEdit) }); + + // act + var result = await MemberTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey); + + // assert + var memberType = await MemberTypeService.GetAsync(result.Result.Key)!; + Assert.Multiple(() => + { + Assert.IsTrue(result.Success); + Assert.IsNotEmpty(memberType.PropertyTypes); + Assert.IsTrue(memberType.PropertyTypes.All(propertyType => memberType.MemberCanViewProperty(propertyType.Alias) == memberCanView)); + Assert.IsTrue(memberType.PropertyTypes.All(propertyType => memberType.MemberCanEditProperty(propertyType.Alias) == memberCanEdit)); + }); + } + + [TestCase(true, true)] + [TestCase(true, false)] + [TestCase(false, true)] + [TestCase(false, false)] + public async Task Can_Update_Property_Visibility(bool memberCanView, bool memberCanEdit) + { + // arrange + var createModel = MemberTypeCreateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(memberCanView: memberCanView is false, memberCanEdit: memberCanEdit is false) }); + IMemberType memberType = (await MemberTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey)).Result!; + var updateModel = MemberTypeUpdateModel(propertyTypes: new[] { MemberTypePropertyTypeModel(memberCanView: memberCanView, memberCanEdit: memberCanEdit) }); + + // act + var result = await MemberTypeEditingService.UpdateAsync(memberType, updateModel, Constants.Security.SuperUserKey); + + // assert + memberType = await MemberTypeService.GetAsync(result.Result.Key)!; + Assert.Multiple(() => + { + Assert.IsNotEmpty(memberType.PropertyTypes); + Assert.IsTrue(memberType.PropertyTypes.All(propertyType => memberType.MemberCanViewProperty(propertyType.Alias) == memberCanView)); + Assert.IsTrue(memberType.PropertyTypes.All(propertyType => memberType.MemberCanEditProperty(propertyType.Alias) == memberCanEdit)); + }); + } + + private MemberTypeCreateModel MemberTypeCreateModel( + string name = "Test", + string? alias = null, + Guid? key = null, + IEnumerable? propertyTypes = null) + { + var model = CreateContentEditingModel( + name, + alias, + isElement: false, + propertyTypes); + model.Key = key ?? Guid.NewGuid(); + model.Alias = alias ?? ShortStringHelper.CleanStringForSafeAlias(name); + return model; + } + + private MemberTypeUpdateModel MemberTypeUpdateModel( + string name = "Test", + string? alias = null, + IEnumerable? propertyTypes = null) + => CreateContentEditingModel( + name, + alias, + isElement: false, + propertyTypes); + + private MemberTypePropertyTypeModel MemberTypePropertyTypeModel( + string name = "Title", + string? alias = null, + Guid? key = null, + Guid? dataTypeKey = null, + bool isSensitive = false, + bool memberCanView = false, + bool memberCanEdit = false) + { + var propertyType = CreatePropertyType(name, alias, key, containerKey: null, dataTypeKey); + propertyType.IsSensitive = isSensitive; + propertyType.MemberCanView = memberCanView; + propertyType.MemberCanEdit = memberCanEdit; + return propertyType; + } +} diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs index 1c936e9df6..5e25d01a50 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs @@ -23,6 +23,8 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest private IMemberTypeService MemberTypeService => GetRequiredService(); + private IUserService UserService => GetRequiredService(); + [Test] public async Task Can_Create_Member() { @@ -263,11 +265,171 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest Assert.AreEqual(authorValue, result.Result.Content!.GetValue("author")); } + [TestCase(true)] + [TestCase(false)] + public async Task Cannot_Update_Sensitive_Properties_Without_Access(bool useSuperUser) + { + // this user does NOT have access to sensitive data + var user = UserBuilder.CreateUser(); + UserService.Save(user); + + var member = await CreateMemberAsync(titleIsSensitive: true); + + var updateModel = new MemberUpdateModel + { + Email = "test-updated@test.com", + Username = "test-updated", + IsApproved = member.IsApproved, + InvariantName = "T. Est Updated", + InvariantProperties = new[] + { + new PropertyValueModel { Alias = "title", Value = "The updated title value" }, + new PropertyValueModel { Alias = "author", Value = "The updated author value" } + } + }; + + var result = await MemberEditingService.UpdateAsync(member.Key, updateModel, useSuperUser ? SuperUser() : user); + if (useSuperUser) + { + Assert.IsTrue(result.Success); + Assert.AreEqual(ContentEditingOperationStatus.Success, result.Status.ContentEditingOperationStatus); + Assert.AreEqual(MemberEditingOperationStatus.Success, result.Status.MemberEditingOperationStatus); + } + else + { + Assert.IsFalse(result.Success); + Assert.AreEqual(ContentEditingOperationStatus.NotAllowed, result.Status.ContentEditingOperationStatus); + Assert.AreEqual(MemberEditingOperationStatus.Success, result.Status.MemberEditingOperationStatus); + } + + member = await MemberEditingService.GetAsync(member.Key); + Assert.IsNotNull(member); + + if (useSuperUser) + { + Assert.AreEqual("The updated title value", member.GetValue("title")); + Assert.AreEqual("The updated author value", member.GetValue("author")); + + Assert.AreEqual("test-updated@test.com", member.Email); + Assert.AreEqual("test-updated", member.Username); + Assert.AreEqual("T. Est Updated", member.Name); + } + else + { + Assert.AreEqual("The title value", member.GetValue("title")); + Assert.AreEqual("The author value", member.GetValue("author")); + + Assert.AreEqual("test@test.com", member.Email); + Assert.AreEqual("test", member.Username); + Assert.AreEqual("T. Est", member.Name); + } + } + + [Test] + public async Task Sensitive_Properties_Are_Retained_When_Updating_Without_Access() + { + // this user does NOT have access to sensitive data + var user = UserBuilder.CreateUser(); + UserService.Save(user); + + var member = await CreateMemberAsync(titleIsSensitive: true); + + var updateModel = new MemberUpdateModel + { + Email = "test-updated@test.com", + Username = "test-updated", + IsApproved = member.IsApproved, + InvariantName = "T. Est Updated", + InvariantProperties = new[] + { + new PropertyValueModel { Alias = "author", Value = "The updated author value" } + } + }; + + var result = await MemberEditingService.UpdateAsync(member.Key, updateModel, user); + Assert.IsTrue(result.Success); + Assert.AreEqual(ContentEditingOperationStatus.Success, result.Status.ContentEditingOperationStatus); + Assert.AreEqual(MemberEditingOperationStatus.Success, result.Status.MemberEditingOperationStatus); + + member = await MemberEditingService.GetAsync(member.Key); + Assert.IsNotNull(member); + + Assert.AreEqual("The title value", member.GetValue("title")); + Assert.AreEqual("The updated author value", member.GetValue("author")); + + Assert.AreEqual("test-updated@test.com", member.Email); + Assert.AreEqual("test-updated", member.Username); + Assert.AreEqual("T. Est Updated", member.Name); + } + + [Test] + public async Task Cannot_Change_IsApproved_Without_Access() + { + // this user does NOT have access to sensitive data + var user = UserBuilder.CreateUser(); + UserService.Save(user); + + var member = await CreateMemberAsync(); + + var updateModel = new MemberUpdateModel + { + Email = member.Email, + Username = member.Username, + IsApproved = false, + InvariantName = member.Name, + InvariantProperties = member.Properties.Select(property => new PropertyValueModel + { + Alias = property.Alias, + Value = property.GetValue() + }) + }; + + var result = await MemberEditingService.UpdateAsync(member.Key, updateModel, user); + Assert.IsFalse(result.Success); + Assert.AreEqual(ContentEditingOperationStatus.NotAllowed, result.Status.ContentEditingOperationStatus); + + member = await MemberEditingService.GetAsync(member.Key); + Assert.IsNotNull(member); + Assert.IsTrue(member.IsApproved); + } + + [Test] + public async Task Cannot_Change_IsLockedOut_Without_Access() + { + // this user does NOT have access to sensitive data + var user = UserBuilder.CreateUser(); + UserService.Save(user); + + var member = await CreateMemberAsync(); + + var updateModel = new MemberUpdateModel + { + Email = member.Email, + Username = member.Username, + IsLockedOut = true, + InvariantName = member.Name, + InvariantProperties = member.Properties.Select(property => new PropertyValueModel + { + Alias = property.Alias, + Value = property.GetValue() + }) + }; + + var result = await MemberEditingService.UpdateAsync(member.Key, updateModel, user); + Assert.IsFalse(result.Success); + Assert.AreEqual(ContentEditingOperationStatus.NotAllowed, result.Status.ContentEditingOperationStatus); + + member = await MemberEditingService.GetAsync(member.Key); + Assert.IsNotNull(member); + Assert.IsFalse(member.IsLockedOut); + } + private IUser SuperUser() => GetRequiredService().GetAsync(Constants.Security.SuperUserKey).GetAwaiter().GetResult(); - private async Task CreateMemberAsync(Guid? key = null) + private async Task CreateMemberAsync(Guid? key = null, bool titleIsSensitive = false) { IMemberType memberType = MemberTypeBuilder.CreateSimpleMemberType(); + memberType.SetIsSensitiveProperty("title", titleIsSensitive); MemberTypeService.Save(memberType); MemberService.AddRole("RoleOne");