[v14] Add missing alias and id to usergroup related api models (#16154)

* Added missing alias and Id to usergroup models create/update/response/item * Changed userGroup IsSystemGroup to more meaningfull fields Also enforced the AliasCanBeChanged businessrule 🙈 --------- Co-authored-by: Sven Geusens <sge@umbraco.dk> Co-authored-by: Mads Rasmussen <madsr@hey.com>
umbraco · May 3, 2024 · f9c0235 · f9c0235
1 parent 8ad6c36
commit f9c0235
Show file tree

Hide file tree

Showing 13 changed files with 175 additions and 28 deletions.
diff --git a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/ByKeyUserGroupController.cs b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/ByKeyUserGroupController.cs
@@ -3,7 +3,6 @@
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
 using Umbraco.Cms.Api.Management.Factories;
-using Umbraco.Cms.Api.Management.Security.Authorization.UserGroup;
 using Umbraco.Cms.Api.Management.ViewModels.UserGroup;
 using Umbraco.Cms.Core.Models.Membership;
 using Umbraco.Cms.Core.Security.Authorization;

diff --git a/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupControllerBase.cs b/src/Umbraco.Cms.Api.Management/Controllers/UserGroup/UserGroupControllerBase.cs
@@ -29,11 +29,15 @@ protected IActionResult UserGroupOperationStatusResult(UserGroupOperationStatus
                 .WithTitle("Duplicate alias")
                 .WithDetail("A user group already exists with the attempted alias.")
                 .Build()),
+            UserGroupOperationStatus.CanNotUpdateAliasIsSystemUserGroup => BadRequest(problemDetailsBuilder
+                .WithTitle("System user group")
+                .WithDetail("Changing the alias is not allowed on a system user group.")
+                .Build()),
             UserGroupOperationStatus.MissingUser => Unauthorized(problemDetailsBuilder
                 .WithTitle("Missing user")
                 .WithDetail("A performing user was not found when attempting the operation.")
                 .Build()),
-            UserGroupOperationStatus.IsSystemUserGroup => BadRequest(problemDetailsBuilder
+            UserGroupOperationStatus.CanNotDeleteIsSystemUserGroup => BadRequest(problemDetailsBuilder
                 .WithTitle("System user group")
                 .WithDetail("The operation is not allowed on a system user group.")
                 .Build()),

diff --git a/src/Umbraco.Cms.Api.Management/Factories/UserGroupPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/UserGroupPresentationFactory.cs
@@ -51,8 +51,9 @@ public async Task<UserGroupResponseModel> CreateAsync(IUserGroup userGroup)
 
         return new UserGroupResponseModel
         {
-            Name = userGroup.Name ?? string.Empty,
             Id = userGroup.Key,
+            Name = userGroup.Name ?? string.Empty,
+            Alias = userGroup.Alias,
             DocumentStartNode = ReferenceByIdModel.ReferenceOrNull(contentStartNodeKey),
             DocumentRootAccess = contentRootAccess,
             MediaStartNode = ReferenceByIdModel.ReferenceOrNull(mediaStartNodeKey),
@@ -63,7 +64,8 @@ public async Task<UserGroupResponseModel> CreateAsync(IUserGroup userGroup)
             FallbackPermissions = userGroup.Permissions,
             Permissions = await _permissionPresentationFactory.CreateAsync(userGroup.GranularPermissions),
             Sections = userGroup.AllowedSections.Select(SectionMapper.GetName),
-            IsSystemGroup = userGroup.IsSystemUserGroup()
+            IsDeletable = !userGroup.IsSystemUserGroup(),
+            AliasCanBeChanged = !userGroup.IsSystemUserGroup(),
         };
     }
 
@@ -83,8 +85,9 @@ public async Task<UserGroupResponseModel> CreateAsync(IReadOnlyUserGroup userGro
 
         return new UserGroupResponseModel
         {
-            Name = userGroup.Name ?? string.Empty,
             Id = userGroup.Key,
+            Name = userGroup.Name ?? string.Empty,
+            Alias = userGroup.Alias,
             DocumentStartNode = ReferenceByIdModel.ReferenceOrNull(contentStartNodeKey),
             MediaStartNode = ReferenceByIdModel.ReferenceOrNull(mediaStartNodeKey),
             Icon = userGroup.Icon,
@@ -93,6 +96,8 @@ public async Task<UserGroupResponseModel> CreateAsync(IReadOnlyUserGroup userGro
             FallbackPermissions = userGroup.Permissions,
             Permissions = await _permissionPresentationFactory.CreateAsync(userGroup.GranularPermissions),
             Sections = userGroup.AllowedSections.Select(SectionMapper.GetName),
+            IsDeletable = !userGroup.IsSystemUserGroup(),
+            AliasCanBeChanged = !userGroup.IsSystemUserGroup(),
         };
     }
 
@@ -107,6 +112,7 @@ public async Task<IEnumerable<UserGroupResponseModel>> CreateMultipleAsync(IEnum
 
         return userGroupViewModels;
     }
+
     /// <inheritdoc />
     public async Task<IEnumerable<UserGroupResponseModel>> CreateMultipleAsync(IEnumerable<IReadOnlyUserGroup> userGroups)
     {
@@ -122,18 +128,21 @@ public async Task<IEnumerable<UserGroupResponseModel>> CreateMultipleAsync(IEnum
     /// <inheritdoc />
     public async Task<Attempt<IUserGroup, UserGroupOperationStatus>> CreateAsync(CreateUserGroupRequestModel requestModel)
     {
-        var cleanedName = requestModel.Name.CleanForXss('[', ']', '(', ')', ':');
-
         var group = new UserGroup(_shortStringHelper)
         {
-            Name = cleanedName,
-            Alias = cleanedName,
+            Name = CleanUserGroupNameOrAliasForXss(requestModel.Name),
+            Alias = CleanUserGroupNameOrAliasForXss(requestModel.Alias),
             Icon = requestModel.Icon,
             HasAccessToAllLanguages = requestModel.HasAccessToAllLanguages,
             Permissions = requestModel.FallbackPermissions,
-            GranularPermissions = await _permissionPresentationFactory.CreatePermissionSetsAsync(requestModel.Permissions)
+            GranularPermissions = await _permissionPresentationFactory.CreatePermissionSetsAsync(requestModel.Permissions),
         };
 
+        if (requestModel.Id.HasValue)
+        {
+            group.Key = requestModel.Id.Value;
+        }
+
         Attempt<UserGroupOperationStatus> assignmentAttempt = AssignStartNodesToUserGroup(requestModel, group);
         if (assignmentAttempt.Success is false)
         {
@@ -186,7 +195,8 @@ public async Task<Attempt<IUserGroup, UserGroupOperationStatus>> UpdateAsync(IUs
             current.AddAllowedSection(SectionMapper.GetAlias(sectionName));
         }
 
-        current.Name = request.Name.CleanForXss('[', ']', '(', ')', ':');
+        current.Name = CleanUserGroupNameOrAliasForXss(request.Name);
+        current.Alias = CleanUserGroupNameOrAliasForXss(request.Alias);
         current.Icon = request.Icon;
         current.HasAccessToAllLanguages = request.HasAccessToAllLanguages;
 
@@ -196,6 +206,9 @@ public async Task<Attempt<IUserGroup, UserGroupOperationStatus>> UpdateAsync(IUs
         return Attempt.SucceedWithStatus(UserGroupOperationStatus.Success, current);
     }
 
+    private static string CleanUserGroupNameOrAliasForXss(string input)
+        => input.CleanForXss('[', ']', '(', ')', ':');
+
     private async Task<Attempt<IEnumerable<string>, UserGroupOperationStatus>> MapLanguageIdsToIsoCodeAsync(IEnumerable<int> ids)
     {
         IEnumerable<ILanguage> languages = await _languageService.GetAllAsync();

diff --git a/src/Umbraco.Cms.Api.Management/Mapping/Item/ItemTypeMapDefinition.cs b/src/Umbraco.Cms.Api.Management/Mapping/Item/ItemTypeMapDefinition.cs
@@ -112,6 +112,7 @@ private static void Map(IUserGroup source, UserGroupItemResponseModel target, Ma
         target.Id = source.Key;
         target.Name = source.Name ?? source.Alias;
         target.Icon = source.Icon;
+        target.Alias = source.Alias;
     }
 
     // Umbraco.Code.MapAll

diff --git a/src/Umbraco.Cms.Api.Management/OpenApi.json b/src/Umbraco.Cms.Api.Management/OpenApi.json
@@ -34131,6 +34131,7 @@
       },
       "CreateUserGroupRequestModel": {
         "required": [
+          "alias",
           "documentRootAccess",
           "fallbackPermissions",
           "hasAccessToAllLanguages",
@@ -34145,6 +34146,9 @@
           "name": {
             "type": "string"
           },
+          "alias": {
+            "type": "string"
+          },
           "icon": {
             "type": "string",
             "nullable": true
@@ -34206,6 +34210,11 @@
                 }
               ]
             }
+          },
+          "id": {
+            "type": "string",
+            "format": "uuid",
+            "nullable": true
           }
         },
         "additionalProperties": false
@@ -43105,6 +43114,7 @@
       },
       "UpdateUserGroupRequestModel": {
         "required": [
+          "alias",
           "documentRootAccess",
           "fallbackPermissions",
           "hasAccessToAllLanguages",
@@ -43119,6 +43129,9 @@
           "name": {
             "type": "string"
           },
+          "alias": {
+            "type": "string"
+          },
           "icon": {
             "type": "string",
             "nullable": true
@@ -43432,12 +43445,17 @@
           "icon": {
             "type": "string",
             "nullable": true
+          },
+          "alias": {
+            "type": "string",
+            "nullable": true
           }
         },
         "additionalProperties": false
       },
       "UserGroupResponseModel": {
         "required": [
+          "alias",
           "documentRootAccess",
           "fallbackPermissions",
           "hasAccessToAllLanguages",
@@ -43454,6 +43472,9 @@
           "name": {
             "type": "string"
           },
+          "alias": {
+            "type": "string"
+          },
           "icon": {
             "type": "string",
             "nullable": true

diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/CreateUserGroupRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/CreateUserGroupRequestModel.cs
@@ -2,5 +2,5 @@
 
 public class CreateUserGroupRequestModel : UserGroupBase
 {
-
+    public Guid? Id { get; set; }
 }
diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/Item/UserGroupItemResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/Item/UserGroupItemResponseModel.cs
@@ -5,4 +5,6 @@ namespace Umbraco.Cms.Api.Management.ViewModels.UserGroup.Item;
 public class UserGroupItemResponseModel : NamedItemResponseModelBase
 {
     public string? Icon { get; set; }
+
+    public string? Alias { get; set; }
 }
diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupBase.cs b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupBase.cs
@@ -17,6 +17,11 @@ public class UserGroupBase
     /// </summary>
     public required string Name { get; init; }
 
+    /// <summary>
+    /// The alias of the user groups
+    /// </summary>
+    public required string Alias { get; init; }
+
     /// <summary>
     /// The Icon for the user group
     /// </summary>

diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/UserGroup/UserGroupResponseModel.cs
@@ -10,5 +10,10 @@ public class UserGroupResponseModel : UserGroupBase
     /// <summary>
     /// Whether this user group is required at system level (thus cannot be removed)
     /// </summary>
-    public bool IsSystemGroup { get; set; }
+    public bool IsDeletable { get; set; }
+
+    /// <summary>
+    /// Whether this user group is required at system level (thus alias needs to be fixed)
+    /// </summary>
+    public bool AliasCanBeChanged { get; set; }
 }
diff --git a/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs b/src/Umbraco.Core/Services/OperationStatus/UserGroupOperationStatus.cs
@@ -8,7 +8,8 @@ public enum UserGroupOperationStatus
     AlreadyExists,
     DuplicateAlias,
     MissingUser,
-    IsSystemUserGroup,
+    CanNotDeleteIsSystemUserGroup,
+    CanNotUpdateAliasIsSystemUserGroup,
     CancelledByNotification,
     MediaStartNodeKeyNotFound,
     DocumentStartNodeKeyNotFound,

diff --git a/src/Umbraco.Core/Services/UserGroupService.cs b/src/Umbraco.Core/Services/UserGroupService.cs
@@ -263,7 +263,7 @@ private Attempt<UserGroupOperationStatus> ValidateUserGroupDeletion(IUserGroup?
 
         if (userGroup.IsSystemUserGroup())
         {
-            return Attempt.Fail(UserGroupOperationStatus.IsSystemUserGroup);
+            return Attempt.Fail(UserGroupOperationStatus.CanNotDeleteIsSystemUserGroup);
         }
 
         return Attempt.Succeed(UserGroupOperationStatus.Success);
@@ -520,12 +520,18 @@ private async Task<UserGroupOperationStatus> ValidateUserGroupUpdateAsync(IUserG
             return UserGroupOperationStatus.NotFound;
         }
 
-        IUserGroup? existing = _userGroupRepository.Get(userGroup.Alias);
-        if (existing is not null && existing.Key != userGroup.Key)
+        IUserGroup? existingByAlias = _userGroupRepository.Get(userGroup.Alias);
+        if (existingByAlias is not null && existingByAlias.Key != userGroup.Key)
         {
             return UserGroupOperationStatus.DuplicateAlias;
         }
 
+        IUserGroup? existingByKey = await GetAsync(userGroup.Key);
+        if (existingByKey is not null && existingByKey.IsSystemUserGroup() && existingByKey.Alias != userGroup.Alias)
+        {
+            return UserGroupOperationStatus.CanNotUpdateAliasIsSystemUserGroup;
+        }
+
         return UserGroupOperationStatus.Success;
     }
 

diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserGroupServiceValidationTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserGroupServiceValidationTests.cs
@@ -185,7 +185,7 @@ public async Task Cannot_Delete_System_UserGroups(string userGroupKeyAsString, s
         var result = await UserGroupService.DeleteAsync(key);
 
         Assert.IsFalse(result.Success);
-        Assert.AreEqual(UserGroupOperationStatus.IsSystemUserGroup, result.Result);
+        Assert.AreEqual(UserGroupOperationStatus.CanNotDeleteIsSystemUserGroup, result.Result);
     }
 
     // these keys are not defined as "const" in Constants.Security but as "static readonly", so we have to hardcode