From f200da48d82eed52a94ea1a456f775e812bc452c Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Mon, 31 Mar 2025 15:16:41 +0200 Subject: [PATCH] Revert rather than prevent updates to sensitive properties on members without sensitive data access (#18794) * Revert rather than prevent updates to sensitive properties on members without sensitive data access. * Added suppression for integration test updates. --- .../Services/MemberEditingService.cs | 11 ++-- .../member-workspace-view-member.element.ts | 45 ++++++++----- .../CompatibilitySuppressions.xml | 14 +++++ .../Services/MemberEditingServiceTests.cs | 63 +------------------ 4 files changed, 50 insertions(+), 83 deletions(-) diff --git a/src/Umbraco.Infrastructure/Services/MemberEditingService.cs b/src/Umbraco.Infrastructure/Services/MemberEditingService.cs index 4b6a50fae0..d87c9d43ce 100644 --- a/src/Umbraco.Infrastructure/Services/MemberEditingService.cs +++ b/src/Umbraco.Infrastructure/Services/MemberEditingService.cs @@ -149,12 +149,11 @@ internal sealed class MemberEditingService : IMemberEditingService 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()); - } + // Handle sensitive data. Certain member properties (IsApproved, IsLockedOut) are subject to "sensitive data" rules. + // The client won't have received these, so will always be false. + // We should reset them back to their original values before proceeding with the update. + updateModel.IsApproved = member.IsApproved; + updateModel.IsLockedOut = member.IsLockedOut; } MemberIdentityUser? identityMember = await _memberManager.FindByIdAsync(member.Id.ToString()); diff --git a/src/Umbraco.Web.UI.Client/src/packages/members/member/workspace/member/views/member/member-workspace-view-member.element.ts b/src/Umbraco.Web.UI.Client/src/packages/members/member/workspace/member/views/member/member-workspace-view-member.element.ts index b8a2ccb8cc..f048becc14 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/members/member/workspace/member/views/member/member-workspace-view-member.element.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/members/member/workspace/member/views/member/member-workspace-view-member.element.ts @@ -9,6 +9,7 @@ import type { UUIBooleanInputEvent } from '@umbraco-cms/backoffice/external/uui' import './member-workspace-view-member-info.element.js'; import type { UmbInputMemberGroupElement } from '@umbraco-cms/backoffice/member-group'; +import { UMB_CURRENT_USER_CONTEXT } from '@umbraco-cms/backoffice/current-user'; @customElement('umb-member-workspace-view-member') export class UmbMemberWorkspaceViewMemberElement extends UmbLitElement implements UmbWorkspaceViewElement { @@ -24,6 +25,12 @@ export class UmbMemberWorkspaceViewMemberElement extends UmbLitElement implement this._isNew = !!isNew; }); }); + + this.consumeContext(UMB_CURRENT_USER_CONTEXT, (context) => { + this.observe(context.hasAccessToSensitiveData, (hasAccessToSensitiveData) => { + this._hasAccessToSensitiveData = hasAccessToSensitiveData === true; + }); + }); } @state() @@ -35,6 +42,9 @@ export class UmbMemberWorkspaceViewMemberElement extends UmbLitElement implement @state() private _isNew = true; + @state() + private _hasAccessToSensitiveData = false; + #onChange(propertyName: keyof UmbMemberDetailModel, value: UmbMemberDetailModel[keyof UmbMemberDetailModel]) { if (!this._workspaceContext) return; @@ -172,23 +182,26 @@ export class UmbMemberWorkspaceViewMemberElement extends UmbLitElement implement .selection=${this._workspaceContext.memberGroups}> - - this.#onChange('isApproved', e.target.checked)}> - - - - - this.#onChange('isLockedOut', e.target.checked)}> - - + ${when(this._hasAccessToSensitiveData, + () => html` + + this.#onChange('isApproved', e.target.checked)}> + + + + this.#onChange('isLockedOut', e.target.checked)}> + + + `) + } lib/net9.0/Umbraco.Tests.Integration.dll true + + CP0002 + M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.MemberEditingServiceTests.Cannot_Change_IsApproved_Without_Access + lib/net9.0/Umbraco.Tests.Integration.dll + lib/net9.0/Umbraco.Tests.Integration.dll + true + + + CP0002 + M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.MemberEditingServiceTests.Cannot_Change_IsLockedOut_Without_Access + lib/net9.0/Umbraco.Tests.Integration.dll + lib/net9.0/Umbraco.Tests.Integration.dll + true + CP0002 M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.TemplateServiceTests.Deleting_Master_Template_Also_Deletes_Children diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs index 0727f9c0ad..0b6829f1dd 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/MemberEditingServiceTests.cs @@ -1,4 +1,4 @@ -using NUnit.Framework; +using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; @@ -341,7 +341,6 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest { Email = "test-updated@test.com", Username = "test-updated", - IsApproved = member.IsApproved, InvariantName = "T. Est Updated", InvariantProperties = new[] { @@ -363,67 +362,9 @@ public class MemberEditingServiceTests : UmbracoIntegrationTest 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); + // IsApproved and IsLockedOut are always sensitive properties. 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); }