Prevent Privilege Escalation: Add Assignment Restrictions for Roles and Permissions

modules/identity/src/Volo.Abp.Identity.Application/Volo/Abp/Identity/IdentityUserAppService.cs

@@ -9,6 +9,7 @@
 using Volo.Abp.Authorization.Permissions;
 using Volo.Abp.Data;
 using Volo.Abp.ObjectExtending;
+using Volo.Abp.Users;
 
 namespace Volo.Abp.Identity;
 
@@ -69,7 +70,8 @@ public virtual async Task<ListResultDto<IdentityRoleDto>> GetRolesAsync(Guid id)
     [Authorize(IdentityPermissions.Users.Default)]
     public virtual async Task<ListResultDto<IdentityRoleDto>> GetAssignableRolesAsync()
     {
-        var list = (await RoleRepository.GetListAsync()).OrderBy(x => x.Name).ToList();
+        var currentUserRoles = await UserManager.GetRolesAsync(await UserManager.GetByIdAsync(CurrentUser.GetId()));
+        var list = (await RoleRepository.GetListAsync(currentUserRoles)).OrderBy(x => x.Name).ToList();
         return new ListResultDto<IdentityRoleDto>(ObjectMapper.Map<List<IdentityRole>, List<IdentityRoleDto>>(list));
     }
 
@@ -145,7 +147,9 @@ public virtual async Task UpdateRolesAsync(Guid id, IdentityUserUpdateRolesDto i
     {
         await IdentityOptions.SetAsync();
         var user = await UserManager.GetByIdAsync(id);
-        (await UserManager.SetRolesAsync(user, input.RoleNames)).CheckErrors();
+
+        var effectiveRoles = await FilterRolesByCurrentUserAsync(user, input.RoleNames);
+        (await UserManager.SetRolesAsync(user, effectiveRoles)).CheckErrors();
         await UserRepository.UpdateAsync(user);
     }
 
@@ -189,7 +193,26 @@ protected virtual async Task UpdateUserByInput(IdentityUser user, IdentityUserCr
         (await UserManager.UpdateAsync(user)).CheckErrors();
         if (input.RoleNames != null && await PermissionChecker.IsGrantedAsync(IdentityPermissions.Users.ManageRoles))
         {
-            (await UserManager.SetRolesAsync(user, input.RoleNames)).CheckErrors();
+            var effectiveRoles = await FilterRolesByCurrentUserAsync(user, input.RoleNames);
+            (await UserManager.SetRolesAsync(user, effectiveRoles)).CheckErrors();
         }
     }
+
+    protected virtual async Task<string[]> FilterRolesByCurrentUserAsync(IdentityUser user, string[] inputRoleNames)
+    {
+        var targetCurrentRoleSet = (await UserManager.GetRolesAsync(user)).ToHashSet(StringComparer.OrdinalIgnoreCase);
+
+        var operatorUser = await UserManager.GetByIdAsync(CurrentUser.GetId());
+        var operatorOwnRoleSet = (await UserManager.GetRolesAsync(operatorUser)).ToHashSet(StringComparer.OrdinalIgnoreCase);
+
+        var inputRoleNameSet = new HashSet<string>(inputRoleNames ?? Array.Empty<string>(), StringComparer.OrdinalIgnoreCase);
+        var keepUnmanageableRoles = targetCurrentRoleSet.Except(operatorOwnRoleSet);
+
+        var desiredManageableRoles = inputRoleNameSet.Intersect(operatorOwnRoleSet);
+
+        return keepUnmanageableRoles
+            .Concat(desiredManageableRoles)
+            .Distinct(StringComparer.OrdinalIgnoreCase)
+            .ToArray();
+    }
 }

modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml

@@ -83,7 +83,14 @@
                             @for (var i = 0; i < Model.Roles.Length; i++)
                             {
                                 var role = Model.Roles[i];
-                                <abp-input abp-id-name="@Model.Roles[i].IsAssigned" asp-for="@role.IsAssigned" label="@role.Name" />
+                                @if (role.IsAssignable)
+                                {
+                                    <abp-input abp-id-name="@Model.Roles[i].IsAssigned" asp-for="@role.IsAssigned" label="@role.Name" />
+                                }
+                                else
+                                {
+                                    <abp-input abp-id-name="@Model.Roles[i].IsAssigned" asp-for="@role.IsAssigned" label="@role.Name" disabled="true" readonly="true" />
+                                }
                                 <input abp-id-name="@Model.Roles[i].Name" asp-for="@role.Name" />
                             }
                         </div>

modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml.cs

@@ -41,18 +41,32 @@ public virtual async Task<IActionResult> OnGetAsync(Guid id)
         UserInfo = ObjectMapper.Map<IdentityUserDto, UserInfoViewModel>(user);
         if (await PermissionChecker.IsGrantedAsync(IdentityPermissions.Users.ManageRoles))
         {
-            Roles = ObjectMapper.Map<IReadOnlyList<IdentityRoleDto>, AssignedRoleViewModel[]>((await IdentityUserAppService.GetAssignableRolesAsync()).Items);
-        }
-        IsEditCurrentUser = CurrentUser.Id == id;
-
-        var userRoleIds = (await IdentityUserAppService.GetRolesAsync(UserInfo.Id)).Items.Select(r => r.Id).ToList();
-        foreach (var role in Roles)
-        {
-            if (userRoleIds.Contains(role.Id))
+            var assignableRoles = (await IdentityUserAppService.GetAssignableRolesAsync()).Items;
+            var currentRoles = (await IdentityUserAppService.GetRolesAsync(id)).Items;
+
+            // Combine assignable and current roles to show all roles user has
+            var combinedRoles = assignableRoles
+                .Concat(currentRoles)
+                .GroupBy(role => role.Id)
+                .Select(group => group.First())
+                .ToList();
+
+            Roles = ObjectMapper.Map<IReadOnlyList<IdentityRoleDto>, AssignedRoleViewModel[]>(combinedRoles);
+
+            var currentRoleIds = currentRoles.Select(r => r.Id).ToHashSet();
+            var assignableRoleIds = assignableRoles.Select(r => r.Id).ToHashSet();
+            foreach (var role in Roles)
             {
-                role.IsAssigned = true;
+                role.IsAssigned = currentRoleIds.Contains(role.Id);
+                role.IsAssignable = assignableRoleIds.Contains(role.Id);
             }
         }
+        else
+        {
+            Roles = Array.Empty<AssignedRoleViewModel>();
+        }
+
+        IsEditCurrentUser = CurrentUser.Id == id;
 
         Detail = ObjectMapper.Map<IdentityUserDto, DetailViewModel>(user);
 
@@ -129,6 +143,8 @@ public class AssignedRoleViewModel
         public string Name { get; set; }
 
         public bool IsAssigned { get; set; }
+
+        public bool IsAssignable { get; set; }
     }
 
     public class DetailViewModel

modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs

@@ -0,0 +1,49 @@
+using System.Collections.Generic;
+using System.Security.Claims;
+using Volo.Abp.DependencyInjection;
+using Volo.Abp.Security.Claims;
+
+namespace Volo.Abp.Identity;
+
+[Dependency(ReplaceServices = true)]
+public class FakeCurrentPrincipalAccessor : ThreadCurrentPrincipalAccessor
+{
+    private readonly IdentityTestData _testData;
+
+    public FakeCurrentPrincipalAccessor(IdentityTestData testData)
+    {
+        _testData = testData;
+    }
+
+    protected override ClaimsPrincipal GetClaimsPrincipal()
+    {
+        return GetPrincipal();
+    }
+
+    private ClaimsPrincipal _principal;
+
+    private ClaimsPrincipal GetPrincipal()
+    {
+        if (_principal == null)
+        {
+            lock (this)
+            {
+                if (_principal == null)
+                {
+                    _principal = new ClaimsPrincipal(
+                        new ClaimsIdentity(
+                            new List<Claim>
+                            {
+                                new Claim(AbpClaimTypes.UserId, _testData.UserAdminId.ToString()),
+                                new Claim(AbpClaimTypes.UserName, "administrator"),
+                                new Claim(AbpClaimTypes.Email, "administrator@abp.io")
+                            }
+                        )
+                    );
+                }
+            }
+        }
+
+        return _principal;
+    }
+}

modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/IdentityUserAppService_Tests.cs

@@ -1,10 +1,13 @@
 using System;
+using System.Linq;
+using System.Security.Claims;
 using System.Threading.Tasks;
 using Shouldly;
 using Volo.Abp.Authorization.Permissions;
 using Volo.Abp.Data;
 using Volo.Abp.PermissionManagement;
 using Volo.Abp.PermissionManagement.Identity;
+using Volo.Abp.Security.Claims;
 using Xunit;
 
 namespace Volo.Abp.Identity;
@@ -16,6 +19,7 @@ public class IdentityUserAppService_Tests : AbpIdentityApplicationTestBase
     private readonly IPermissionManager _permissionManager;
     private readonly UserPermissionManagementProvider _userPermissionManagementProvider;
     private readonly IdentityTestData _testData;
+    private readonly ICurrentPrincipalAccessor _currentPrincipalAccessor;
 
     public IdentityUserAppService_Tests()
     {
@@ -24,6 +28,7 @@ public IdentityUserAppService_Tests()
         _permissionManager = GetRequiredService<IPermissionManager>();
         _userPermissionManagementProvider = GetRequiredService<UserPermissionManagementProvider>();
         _testData = GetRequiredService<IdentityTestData>();
+        _currentPrincipalAccessor = GetRequiredService<ICurrentPrincipalAccessor>();
     }
 
     [Fact]
@@ -239,6 +244,114 @@ await _userAppService.UpdateRolesAsync(
         roleNames.ShouldContain("manager");
     }
 
+    [Fact]
+    public async Task UpdateRolesAsync_Should_Not_Assign_Roles_Operator_Does_Not_Have()
+    {
+        // neo only has "supporter" role
+        using (_currentPrincipalAccessor.Change(new Claim(AbpClaimTypes.UserId, _testData.UserNeoId.ToString())))
+        {
+            // Try to assign "admin" and "supporter" to david (who has no roles)
+            await _userAppService.UpdateRolesAsync(
+                _testData.UserDavidId,
+                new IdentityUserUpdateRolesDto
+                {
+                    RoleNames = new[] { "admin", "supporter" }
+                }
+            );
+        }
+
+        // Only "supporter" should be assigned (admin filtered out since neo doesn't have it)
+        var roleNames = await _userRepository.GetRoleNamesAsync(_testData.UserDavidId);
+        roleNames.ShouldContain("supporter");
+        roleNames.ShouldNotContain("admin");
+    }
+
+    [Fact]
+    public async Task UpdateRolesAsync_Should_Preserve_Unmanageable_Roles()
+    {
+        // john.nash has direct roles: moderator, supporter
+        // neo only has "supporter" role, so "moderator" is unmanageable for neo
+        using (_currentPrincipalAccessor.Change(new Claim(AbpClaimTypes.UserId, _testData.UserNeoId.ToString())))
+        {
+            await _userAppService.UpdateRolesAsync(
+                _testData.UserJohnId,
+                new IdentityUserUpdateRolesDto
+                {
+                    RoleNames = new[] { "supporter" }
+                }
+            );
+        }
+
+        // "moderator" should be preserved (unmanageable), "supporter" kept (in input)
+        var roleNames = await _userRepository.GetRoleNamesAsync(_testData.UserJohnId);
+        roleNames.ShouldContain("moderator");
+        roleNames.ShouldContain("supporter");
+    }
+
+    [Fact]
+    public async Task UpdateRolesAsync_Should_Only_Remove_Manageable_Roles()
+    {
+        // john.nash has direct roles: moderator, supporter
+        // neo only has "supporter" role
+        using (_currentPrincipalAccessor.Change(new Claim(AbpClaimTypes.UserId, _testData.UserNeoId.ToString())))
+        {
+            // Input is empty - try to remove all roles
+            await _userAppService.UpdateRolesAsync(
+                _testData.UserJohnId,
+                new IdentityUserUpdateRolesDto
+                {
+                    RoleNames = Array.Empty<string>()
+                }
+            );
+        }
+
+        // "moderator" should be preserved (neo can't manage it), "supporter" removed (neo has it and it's not in input)
+        var roleNames = await _userRepository.GetRoleNamesAsync(_testData.UserJohnId);
+        roleNames.ShouldContain("moderator");
+        roleNames.ShouldNotContain("supporter");
+    }
+
+    [Fact]
+    public async Task UpdateRolesAsync_Self_Cannot_Add_New_Roles()
+    {
+        // neo only has "supporter", tries to add "admin" to self
+        using (_currentPrincipalAccessor.Change(new Claim(AbpClaimTypes.UserId, _testData.UserNeoId.ToString())))
+        {
+            await _userAppService.UpdateRolesAsync(
+                _testData.UserNeoId,
+                new IdentityUserUpdateRolesDto
+                {
+                    RoleNames = new[] { "supporter", "admin" }
+                }
+            );
+        }
+
+        // "admin" should not be added (neo doesn't have it), "supporter" kept
+        var roleNames = await _userRepository.GetRoleNamesAsync(_testData.UserNeoId);
+        roleNames.ShouldContain("supporter");
+        roleNames.ShouldNotContain("admin");
+    }
+
+    [Fact]
+    public async Task UpdateRolesAsync_Self_Can_Remove_Own_Roles()
+    {
+        // admin user has: admin, moderator, supporter, manager
+        // Remove supporter and manager from self
+        await _userAppService.UpdateRolesAsync(
+            _testData.UserAdminId,
+            new IdentityUserUpdateRolesDto
+            {
+                RoleNames = new[] { "admin", "moderator" }
+            }
+        );
+
+        var roleNames = await _userRepository.GetRoleNamesAsync(_testData.UserAdminId);
+        roleNames.ShouldContain("admin");
+        roleNames.ShouldContain("moderator");
+        roleNames.ShouldNotContain("supporter");
+        roleNames.ShouldNotContain("manager");
+    }
+
     private static string CreateRandomEmail()
     {
         return Guid.NewGuid().ToString("N").Left(16) + "@abp.io";

modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/AbpIdentityTestDataBuilder.cs

@@ -124,8 +124,11 @@ private async Task AddOrganizationUnits()
 
     private async Task AddUsers()
     {
-        var adminUser = new IdentityUser(_guidGenerator.Create(), "administrator", "admin@abp.io");
+        var adminUser = new IdentityUser(_testData.UserAdminId, "administrator", "administrator@abp.io");
         adminUser.AddRole(_adminRole.Id);
+        adminUser.AddRole(_moderatorRole.Id);
+        adminUser.AddRole(_supporterRole.Id);
+        adminUser.AddRole(_managerRole.Id);
         adminUser.AddClaim(_guidGenerator, new Claim("TestClaimType", "42"));
         await _userRepository.InsertAsync(adminUser);
 

modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityRoleRepository_Tests.cs

@@ -81,9 +81,9 @@ public async Task GetListWithUserCountAsync()
 
         roles.Count.ShouldBe(5);
         roles.ShouldContain(r => r.Role.Name == "admin" && r.UserCount == 2);
-        roles.ShouldContain(r => r.Role.Name == "moderator" && r.UserCount == 1);
-        roles.ShouldContain(r => r.Role.Name == "supporter" && r.UserCount == 2);
-        roles.ShouldContain(r => r.Role.Name == "manager" && r.UserCount == 1);
+        roles.ShouldContain(r => r.Role.Name == "moderator" && r.UserCount == 2);
+        roles.ShouldContain(r => r.Role.Name == "supporter" && r.UserCount == 3);
+        roles.ShouldContain(r => r.Role.Name == "manager" && r.UserCount == 2);
 
 
         using (var uow = UnitOfWorkManager.Begin())
@@ -96,7 +96,7 @@ public async Task GetListWithUserCountAsync()
         roles = await RoleRepository.GetListWithUserCountAsync();
 
         roles.Count.ShouldBe(5);
-        roles.ShouldContain(r => r.Role.Name == "manager" && r.UserCount == 0);
+        roles.ShouldContain(r => r.Role.Name == "manager" && r.UserCount == 1);
         roles.ShouldContain(r => r.Role.Name == "sale" && r.UserCount == 0);
     }
 }

modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityTestData.cs

@@ -5,6 +5,7 @@ namespace Volo.Abp.Identity;
 
 public class IdentityTestData : ISingletonDependency
 {
+    public Guid UserAdminId { get; } = Guid.NewGuid();
     public Guid RoleModeratorId { get; } = Guid.NewGuid();
     public Guid RoleSupporterId { get; } = Guid.NewGuid();
     public Guid RoleManagerId { get; } = Guid.NewGuid();

modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityUserRepository_Tests.cs

@@ -119,22 +119,26 @@ public async Task GetListByClaimAsync()
     public async Task GetListByNormalizedRoleNameAsync()
     {
         var users = await UserRepository.GetListByNormalizedRoleNameAsync(LookupNormalizer.NormalizeName("supporter"));
-        users.Count.ShouldBe(2);
+        users.Count.ShouldBe(3);
+        users.ShouldContain(u => u.UserName == "administrator");
         users.ShouldContain(u => u.UserName == "john.nash");
         users.ShouldContain(u => u.UserName == "neo");
     }
 
     [Fact]
     public async Task GetUserIdListByRoleIdAsync()
     {
+        var admin = await UserRepository.FindByNormalizedUserNameAsync(LookupNormalizer.NormalizeName("administrator"));
         var john = await UserRepository.FindByNormalizedUserNameAsync(LookupNormalizer.NormalizeName("john.nash"));
         var neo = await UserRepository.FindByNormalizedUserNameAsync(LookupNormalizer.NormalizeName("neo"));
+        admin.ShouldNotBeNull();
         john.ShouldNotBeNull();
         neo.ShouldNotBeNull();
 
         var roleId = (await RoleRepository.FindByNormalizedNameAsync(LookupNormalizer.NormalizeName("supporter"))).Id;
         var users = await UserRepository.GetUserIdListByRoleIdAsync(roleId);
-        users.Count.ShouldBe(2);
+        users.Count.ShouldBe(3);
+        users.ShouldContain(id => id == admin.Id);
         users.ShouldContain(id => id == john.Id);
         users.ShouldContain(id => id == neo.Id);
     }

modules/permission-management/src/Volo.Abp.PermissionManagement.Application.Contracts/Volo/Abp/PermissionManagement/PermissionGrantInfoDto.cs

@@ -15,4 +15,6 @@ public class PermissionGrantInfoDto
     public List<string> AllowedProviders { get; set; }
 
     public List<ProviderInfoDto> GrantedProviders { get; set; }
+
+    public bool IsEditable { get; set; }
 }