From 8f6d1bc7b81491502129c30e36607f42bdf830cb Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Mon, 10 Feb 2025 10:52:41 +0100 Subject: [PATCH] Add validation to prevent update of a user or member to an invalid username. (#18263) Avoid password manager updates of user name field on user details screen. --- .../Member/MemberControllerBase.cs | 5 ++- .../User/UserOrCurrentUserControllerBase.cs | 4 ++ .../OperationStatus/UserOperationStatus.cs | 3 +- src/Umbraco.Core/Services/UserService.cs | 15 ++++--- .../Services/MemberEditingService.cs | 40 ++++++++++++++++++- ...user-workspace-profile-settings.element.ts | 1 + 6 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Member/MemberControllerBase.cs b/src/Umbraco.Cms.Api.Management/Controllers/Member/MemberControllerBase.cs index d371865c14..b2ef50589f 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Member/MemberControllerBase.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Member/MemberControllerBase.cs @@ -1,4 +1,4 @@ -using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Umbraco.Cms.Api.Common.Builders; @@ -58,7 +58,8 @@ public class MemberControllerBase : ContentControllerBase .WithTitle("Invalid name supplied") .Build()), MemberEditingOperationStatus.InvalidUsername => BadRequest(problemDetailsBuilder - .WithTitle("Invalid username supplied") + .WithTitle("Invalid username") + .WithDetail("The username is either empty or contains one or more invalid characters.") .Build()), MemberEditingOperationStatus.InvalidEmail => BadRequest(problemDetailsBuilder .WithTitle("Invalid email supplied") diff --git a/src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs b/src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs index 65490a9c21..43db8355b7 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs @@ -35,6 +35,10 @@ public abstract class UserOrCurrentUserControllerBase : ManagementApiControllerB .WithTitle("Email Cannot be changed") .WithDetail("Local login is disabled, so the email cannot be changed.") .Build()), + UserOperationStatus.InvalidUserName => BadRequest(problemDetailsBuilder + .WithTitle("Invalid username") + .WithDetail("The username contains one or more invalid characters.") + .Build()), UserOperationStatus.DuplicateUserName => BadRequest(problemDetailsBuilder .WithTitle("Duplicate Username") .WithDetail("The username is already in use.") diff --git a/src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs b/src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs index ef16cd1471..2702b94344 100644 --- a/src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs +++ b/src/Umbraco.Core/Services/OperationStatus/UserOperationStatus.cs @@ -1,4 +1,4 @@ -namespace Umbraco.Cms.Core.Services.OperationStatus; +namespace Umbraco.Cms.Core.Services.OperationStatus; /// /// Used to signal a user operation succeeded or an atomic failure reason @@ -41,4 +41,5 @@ public enum UserOperationStatus SelfPasswordResetNotAllowed, DuplicateId, InvalidUserType, + InvalidUserName, } diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index 657507f17b..e92a5b0cd0 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -1,13 +1,11 @@ using System.ComponentModel.DataAnnotations; -using System.Globalization; using System.Linq.Expressions; -using Microsoft.Extensions.DependencyInjection; using System.Security.Claims; using System.Security.Cryptography; using System.Text.RegularExpressions; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; -using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Editors; @@ -40,7 +38,6 @@ internal partial class UserService : RepositoryService, IUserService { private readonly GlobalSettings _globalSettings; private readonly SecuritySettings _securitySettings; - private readonly ILogger _logger; private readonly IUserGroupRepository _userGroupRepository; private readonly UserEditorAuthorizationHelper _userEditorAuthorizationHelper; private readonly IServiceScopeFactory _serviceScopeFactory; @@ -137,7 +134,6 @@ internal partial class UserService : RepositoryService, IUserService _globalSettings = globalSettings.Value; _securitySettings = securitySettings.Value; _contentSettings = contentSettings.Value; - _logger = loggerFactory.CreateLogger(); } /// @@ -966,6 +962,15 @@ internal partial class UserService : RepositoryService, IUserService return Attempt.FailWithStatus(UserOperationStatus.MissingUser, existingUser); } + // User names can only contain the configured allowed characters. This is validated by ASP.NET Identity on create + // as the setting is applied to the BackOfficeIdentityOptions, but we need to check ourselves for updates. + var allowedUserNameCharacters = _securitySettings.AllowedUserNameCharacters; + if (model.UserName.Any(c => allowedUserNameCharacters.Contains(c) == false)) + { + scope.Complete(); + return Attempt.FailWithStatus(UserOperationStatus.InvalidUserName, existingUser); + } + IEnumerable allUserGroups = _userGroupRepository.GetMany().ToArray(); var userGroups = allUserGroups.Where(x => model.UserGroupKeys.Contains(x.Key)).ToHashSet(); diff --git a/src/Umbraco.Infrastructure/Services/MemberEditingService.cs b/src/Umbraco.Infrastructure/Services/MemberEditingService.cs index 061f3ecf62..f7cd7060c3 100644 --- a/src/Umbraco.Infrastructure/Services/MemberEditingService.cs +++ b/src/Umbraco.Infrastructure/Services/MemberEditingService.cs @@ -1,5 +1,9 @@ -using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.Identity; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Models.Membership; @@ -19,7 +23,9 @@ internal sealed class MemberEditingService : IMemberEditingService private readonly IPasswordChanger _passwordChanger; private readonly ILogger _logger; private readonly IMemberGroupService _memberGroupService; + private readonly SecuritySettings _securitySettings; + [Obsolete("Use the constructor that takes all parameters. Scheduled for removal in V16.")] public MemberEditingService( IMemberService memberService, IMemberTypeService memberTypeService, @@ -29,6 +35,29 @@ internal sealed class MemberEditingService : IMemberEditingService IPasswordChanger passwordChanger, ILogger logger, IMemberGroupService memberGroupService) + : this( + memberService, + memberTypeService, + memberContentEditingService, + memberManager, + twoFactorLoginService, + passwordChanger, + logger, + memberGroupService, + StaticServiceProvider.Instance.GetRequiredService>()) + { + } + + public MemberEditingService( + IMemberService memberService, + IMemberTypeService memberTypeService, + IMemberContentEditingService memberContentEditingService, + IMemberManager memberManager, + ITwoFactorLoginService twoFactorLoginService, + IPasswordChanger passwordChanger, + ILogger logger, + IMemberGroupService memberGroupService, + IOptions securitySettings) { _memberService = memberService; _memberTypeService = memberTypeService; @@ -38,6 +67,7 @@ internal sealed class MemberEditingService : IMemberEditingService _passwordChanger = passwordChanger; _logger = logger; _memberGroupService = memberGroupService; + _securitySettings = securitySettings.Value; } public async Task GetAsync(Guid key) @@ -225,6 +255,14 @@ internal sealed class MemberEditingService : IMemberEditingService return MemberEditingOperationStatus.InvalidUsername; } + // User names can only contain the configured allowed characters. This is validated by ASP.NET Identity on create + // as the setting is applied to the BackOfficeIdentityOptions, but we need to check ourselves for updates. + var allowedUserNameCharacters = _securitySettings.AllowedUserNameCharacters; + if (model.Username.Any(c => allowedUserNameCharacters.Contains(c) == false)) + { + return MemberEditingOperationStatus.InvalidUsername; + } + if (model.Email.IsEmail() is false) { return MemberEditingOperationStatus.InvalidEmail; diff --git a/src/Umbraco.Web.UI.Client/src/packages/user/user/workspace/user/components/user-workspace-profile-settings/user-workspace-profile-settings.element.ts b/src/Umbraco.Web.UI.Client/src/packages/user/user/workspace/user/components/user-workspace-profile-settings/user-workspace-profile-settings.element.ts index c3a6a54501..536d2040f8 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/user/user/workspace/user/components/user-workspace-profile-settings/user-workspace-profile-settings.element.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/user/user/workspace/user/components/user-workspace-profile-settings/user-workspace-profile-settings.element.ts @@ -97,6 +97,7 @@ export class UmbUserWorkspaceProfileSettingsElement extends UmbLitElement {