Validate client IDs before applying them (#17426)

* Validate client IDs before applying them

* Add operation result to return

* Update src/Umbraco.Cms.Api.Management/Controllers/User/ClientCredentials/ClientCredentialsUserControllerBase.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Zeegaan <skrivdetud@gmail.com>
Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

src/Umbraco.Cms.Api.Management/Controllers/User/ClientCredentials/ClientCredentialsUserControllerBase.cs

@@ -18,6 +18,10 @@ public abstract class ClientCredentialsUserControllerBase : UserControllerBase
                 .WithTitle("Duplicate client ID")
                 .WithDetail("The specified client ID is already in use. Choose another client ID.")
                 .Build()),
+            BackOfficeUserClientCredentialsOperationStatus.InvalidClientId => BadRequest(problemDetailsBuilder
+                .WithTitle("Invalid client ID")
+                .WithDetail("The specified client ID is invalid. A valid client ID can only contain [a-z], [A-Z], [0-9], and [-._~].")
+                .Build()),
             _ => StatusCode(StatusCodes.Status500InternalServerError, problemDetailsBuilder
                 .WithTitle("Unknown client credentials operation status.")
                 .Build()),

src/Umbraco.Core/Services/OperationStatus/UserClientCredentialsOperationStatus.cs

@@ -4,5 +4,6 @@ public enum UserClientCredentialsOperationStatus
 {
     Success,
     DuplicateClientId,
-    InvalidUser
+    InvalidUser,
+    InvalidClientId
 }

src/Umbraco.Core/Services/UserService.cs

@@ -4,6 +4,7 @@ using System.Linq.Expressions;
 using Microsoft.Extensions.DependencyInjection;
 using System.Security.Claims;
 using System.Security.Cryptography;
+using System.Text.RegularExpressions;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Cache;
@@ -35,7 +36,7 @@ namespace Umbraco.Cms.Core.Services;
 ///     Represents the UserService, which is an easy access to operations involving <see cref="IProfile" />,
 ///     <see cref="IMembershipUser" /> and eventually Backoffice Users.
 /// </summary>
-internal class UserService : RepositoryService, IUserService
+internal partial class UserService : RepositoryService, IUserService
 {
     private readonly GlobalSettings _globalSettings;
     private readonly SecuritySettings _securitySettings;
@@ -2483,6 +2484,11 @@ internal class UserService : RepositoryService, IUserService
 
     public async Task<UserClientCredentialsOperationStatus> AddClientIdAsync(Guid userKey, string clientId)
     {
+        if (ValidClientId().IsMatch(clientId) is false)
+        {
+            return UserClientCredentialsOperationStatus.InvalidClientId;
+        }
+
         using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
 
         IEnumerable<string> currentClientIds = _userRepository.GetAllClientIds();
@@ -2670,5 +2676,8 @@ internal class UserService : RepositoryService, IUserService
         }
     }
 
+    [GeneratedRegex(@"^[\w\d\-\._~]*$")]
+    private static partial Regex ValidClientId();
+
     #endregion
 }

src/Umbraco.Infrastructure/Security/BackOfficeUserClientCredentialsManager.cs

@@ -35,6 +35,7 @@ public sealed class BackOfficeUserClientCredentialsManager : ClientCredentialsMa
             {
                 UserClientCredentialsOperationStatus.InvalidUser => Attempt.Fail(BackOfficeUserClientCredentialsOperationStatus.InvalidUser),
                 UserClientCredentialsOperationStatus.DuplicateClientId => Attempt.Fail(BackOfficeUserClientCredentialsOperationStatus.DuplicateClientId),
+                UserClientCredentialsOperationStatus.InvalidClientId => Attempt.Fail(BackOfficeUserClientCredentialsOperationStatus.InvalidClientId),
                 _ => throw new ArgumentOutOfRangeException($"Unsupported client ID operation status: {result}")
             };
         }

src/Umbraco.Infrastructure/Security/OperationStatus/BackOfficeUserClientCredentialsOperationStatus.cs

@@ -4,5 +4,6 @@ public enum BackOfficeUserClientCredentialsOperationStatus
 {
     Success,
     DuplicateClientId,
-    InvalidUser
+    InvalidUser,
+    InvalidClientId
 }

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/UserServiceTests.cs

@@ -13,6 +13,7 @@ using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.Membership;
 using Umbraco.Cms.Core.Persistence.Querying;
 using Umbraco.Cms.Core.Services;
+using Umbraco.Cms.Core.Services.OperationStatus;
 using Umbraco.Cms.Tests.Common.Builders;
 using Umbraco.Cms.Tests.Common.Testing;
 using Umbraco.Cms.Tests.Integration.Testing;
@@ -967,6 +968,54 @@ public class UserServiceTests : UmbracoIntegrationTest
         }
     }
 
+    [TestCase(UserKind.Default, UserClientCredentialsOperationStatus.InvalidUser)]
+    [TestCase(UserKind.Api, UserClientCredentialsOperationStatus.Success)]
+    public async Task Can_Assign_ClientId_To_Api_User(UserKind userKind, UserClientCredentialsOperationStatus expectedResult)
+    {
+        // Arrange
+        var user = await CreateTestUser(userKind);
+
+        // Act
+        var result = await UserService.AddClientIdAsync(user.Key, "client-one");
+
+        // Assert
+        Assert.AreEqual(expectedResult, result);
+    }
+
+    [TestCase("abcdefghijklmnopqrstuvwxyz", UserClientCredentialsOperationStatus.Success)]
+    [TestCase("ABCDEFGHIJKLMNOPQRSTUVWXYZ", UserClientCredentialsOperationStatus.Success)]
+    [TestCase("1234567890", UserClientCredentialsOperationStatus.Success)]
+    [TestCase(".-_~", UserClientCredentialsOperationStatus.Success)]
+    [TestCase("!", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("#", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("$", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("&", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("'", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("(", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase(")", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("*", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("+", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase(",", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("/", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase(":", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase(";", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("=", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("?", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("@", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("[", UserClientCredentialsOperationStatus.InvalidClientId)]
+    [TestCase("]", UserClientCredentialsOperationStatus.InvalidClientId)]
+    public async Task Can_Use_Only_Unreserved_Characters_For_ClientId(string clientId, UserClientCredentialsOperationStatus expectedResult)
+    {
+        // Arrange
+        var user = await CreateTestUser(UserKind.Api);
+
+        // Act
+        var result = await UserService.AddClientIdAsync(user.Key, clientId);
+
+        // Assert
+        Assert.AreEqual(expectedResult, result);
+    }
+
     private Content[] BuildContentItems(int numberToCreate)
     {
         var template = TemplateBuilder.CreateTextPageTemplate();
@@ -999,6 +1048,25 @@ public class UserServiceTests : UmbracoIntegrationTest
         return user;
     }
 
+    private async Task<IUser> CreateTestUser(UserKind userKind)
+    {
+        var userGroup = CreateTestUserGroup();
+
+        var result = await UserService.CreateAsync(
+            Constants.Security.SuperUserKey,
+            new UserCreateModel
+            {
+                Email = "test1@test.com",
+                Id = Guid.NewGuid(),
+                Kind = userKind,
+                Name = "test1@test.com",
+                UserName = "test1@test.com",
+                UserGroupKeys = new HashSet<Guid> { userGroup.Key }
+            });
+        Assert.IsTrue(result.Success);
+        return result.Result.CreatedUser!;
+    }
+
     private List<IUser> CreateTestUsers(int[] startContentIds, IUserGroup userGroup, int numberToCreate)
     {
         var users = new List<IUser>();