From cddf0ffa7aa5147fefc14f7cf915c28de177e206 Mon Sep 17 00:00:00 2001 From: maliming Date: Tue, 3 Feb 2026 14:21:27 +0800 Subject: [PATCH 1/4] Prevent Privilege Escalation: Add Assignment Restrictions for Roles and Permissions --- .../Abp/Identity/IdentityUserAppService.cs | 29 ++++- .../Pages/Identity/Users/EditModal.cshtml | 9 +- .../Pages/Identity/Users/EditModal.cshtml.cs | 34 ++++-- .../Identity/FakeCurrentPrincipalAccessor.cs | 49 ++++++++ .../Identity/IdentityUserAppService_Tests.cs | 113 ++++++++++++++++++ .../Identity/AbpIdentityTestDataBuilder.cs | 5 +- .../Identity/IdentityRoleRepository_Tests.cs | 8 +- .../Volo/Abp/Identity/IdentityTestData.cs | 1 + .../Identity/IdentityUserRepository_Tests.cs | 8 +- .../PermissionGrantInfoDto.cs | 2 + .../PermissionAppService.cs | 48 +++++++- .../PermissionManagementModal.cshtml.cs | 20 +++- ...PermissionManagementApplicationTestBase.cs | 6 + .../FakePermissionChecker.cs | 53 ++++++++ .../PermissionAppService_Tests.cs | 79 ++++++++++++ 15 files changed, 437 insertions(+), 27 deletions(-) create mode 100644 modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs create mode 100644 modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/FakePermissionChecker.cs diff --git a/modules/identity/src/Volo.Abp.Identity.Application/Volo/Abp/Identity/IdentityUserAppService.cs b/modules/identity/src/Volo.Abp.Identity.Application/Volo/Abp/Identity/IdentityUserAppService.cs index 582d9ade42..8d7e2f4062 100644 --- a/modules/identity/src/Volo.Abp.Identity.Application/Volo/Abp/Identity/IdentityUserAppService.cs +++ b/modules/identity/src/Volo.Abp.Identity.Application/Volo/Abp/Identity/IdentityUserAppService.cs @@ -9,6 +9,7 @@ using Volo.Abp.Application.Dtos; 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 class IdentityUserAppService : IdentityAppServiceBase, IIdentityUserAppSe [Authorize(IdentityPermissions.Users.Default)] public virtual async Task> 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(ObjectMapper.Map, List>(list)); } @@ -145,7 +147,9 @@ public class IdentityUserAppService : IdentityAppServiceBase, IIdentityUserAppSe { 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 @@ public class IdentityUserAppService : IdentityAppServiceBase, IIdentityUserAppSe (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 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(inputRoleNames ?? Array.Empty(), StringComparer.OrdinalIgnoreCase); + var keepUnmanageableRoles = targetCurrentRoleSet.Except(operatorOwnRoleSet); + + var desiredManageableRoles = inputRoleNameSet.Intersect(operatorOwnRoleSet); + + return keepUnmanageableRoles + .Concat(desiredManageableRoles) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToArray(); + } } diff --git a/modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml b/modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml index 0c85be8f18..b60978f32e 100644 --- a/modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml +++ b/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]; - + @if (role.IsAssignable) + { + + } + else + { + + } } diff --git a/modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml.cs b/modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml.cs index 074f3f8481..08ea27081b 100644 --- a/modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml.cs +++ b/modules/identity/src/Volo.Abp.Identity.Web/Pages/Identity/Users/EditModal.cshtml.cs @@ -41,18 +41,32 @@ public class EditModalModel : IdentityPageModel UserInfo = ObjectMapper.Map(user); if (await PermissionChecker.IsGrantedAsync(IdentityPermissions.Users.ManageRoles)) { - Roles = ObjectMapper.Map, 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, 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(); + } + + IsEditCurrentUser = CurrentUser.Id == id; Detail = ObjectMapper.Map(user); @@ -129,6 +143,8 @@ public class EditModalModel : IdentityPageModel public string Name { get; set; } public bool IsAssigned { get; set; } + + public bool IsAssignable { get; set; } } public class DetailViewModel diff --git a/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs b/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs new file mode 100644 index 0000000000..53b1e3cd4e --- /dev/null +++ b/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 + { + new Claim(AbpClaimTypes.UserId, _testData.UserAdminId.ToString()), + new Claim(AbpClaimTypes.UserName, "administrator"), + new Claim(AbpClaimTypes.Email, "administrator@abp.io") + } + ) + ); + } + } + } + + return _principal; + } +} diff --git a/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/IdentityUserAppService_Tests.cs b/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/IdentityUserAppService_Tests.cs index acd50b7947..dd80c8c9d0 100644 --- a/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/IdentityUserAppService_Tests.cs +++ b/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 class IdentityUserAppService_Tests : AbpIdentityApplicationTestBase _permissionManager = GetRequiredService(); _userPermissionManagementProvider = GetRequiredService(); _testData = GetRequiredService(); + _currentPrincipalAccessor = GetRequiredService(); } [Fact] @@ -239,6 +244,114 @@ public class IdentityUserAppService_Tests : AbpIdentityApplicationTestBase 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() + } + ); + } + + // "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"; diff --git a/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/AbpIdentityTestDataBuilder.cs b/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/AbpIdentityTestDataBuilder.cs index 15ef80b4c4..7132dd9662 100644 --- a/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/AbpIdentityTestDataBuilder.cs +++ b/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/AbpIdentityTestDataBuilder.cs @@ -124,8 +124,11 @@ public class AbpIdentityTestDataBuilder : ITransientDependency 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); diff --git a/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityRoleRepository_Tests.cs b/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityRoleRepository_Tests.cs index ffeb1220b1..a677fb4841 100644 --- a/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityRoleRepository_Tests.cs +++ b/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityRoleRepository_Tests.cs @@ -81,9 +81,9 @@ public abstract class IdentityRoleRepository_Tests : AbpIdentity 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 abstract class IdentityRoleRepository_Tests : AbpIdentity 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); } } diff --git a/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityTestData.cs b/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityTestData.cs index b6cb14de5c..0b8cc2ce42 100644 --- a/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityTestData.cs +++ b/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(); diff --git a/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityUserRepository_Tests.cs b/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityUserRepository_Tests.cs index 58dc072359..59a5c790f8 100644 --- a/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityUserRepository_Tests.cs +++ b/modules/identity/test/Volo.Abp.Identity.TestBase/Volo/Abp/Identity/IdentityUserRepository_Tests.cs @@ -119,7 +119,8 @@ public abstract class IdentityUserRepository_Tests : AbpIdentity 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"); } @@ -127,14 +128,17 @@ public abstract class IdentityUserRepository_Tests : AbpIdentity [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); } diff --git a/modules/permission-management/src/Volo.Abp.PermissionManagement.Application.Contracts/Volo/Abp/PermissionManagement/PermissionGrantInfoDto.cs b/modules/permission-management/src/Volo.Abp.PermissionManagement.Application.Contracts/Volo/Abp/PermissionManagement/PermissionGrantInfoDto.cs index b608a69b54..79ddc93047 100644 --- a/modules/permission-management/src/Volo.Abp.PermissionManagement.Application.Contracts/Volo/Abp/PermissionManagement/PermissionGrantInfoDto.cs +++ b/modules/permission-management/src/Volo.Abp.PermissionManagement.Application.Contracts/Volo/Abp/PermissionManagement/PermissionGrantInfoDto.cs @@ -15,4 +15,6 @@ public class PermissionGrantInfoDto public List AllowedProviders { get; set; } public List GrantedProviders { get; set; } + + public bool IsEditable { get; set; } } diff --git a/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs b/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs index e26e387fae..98c7b2bc35 100644 --- a/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs +++ b/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs @@ -128,9 +128,42 @@ public class PermissionAppService : ApplicationService, IPermissionAppService } } + // Filter permissions for non-admin users: only show permissions they have or that are already granted + await FilterOutputPermissionsByCurrentUserAsync(result); + return result; } + protected virtual async Task FilterOutputPermissionsByCurrentUserAsync(GetPermissionListResultDto result) + { + // Collect all permission names + var allPermissionNames = result.Groups + .SelectMany(g => g.Permissions) + .Select(p => p.Name) + .ToArray(); + + if (!allPermissionNames.Any()) + { + return; + } + + // Check which permissions current user has + var currentUserPermissions = await PermissionChecker.IsGrantedAsync(allPermissionNames); + var grantedPermissionNames = currentUserPermissions.Result + .Where(x => x.Value == PermissionGrantResult.Granted) + .Select(x => x.Key) + .ToHashSet(); + + // Mark editability: users can only edit permissions they currently have + foreach (var group in result.Groups) + { + foreach (var permission in group.Permissions) + { + permission.IsEditable = grantedPermissionNames.Contains(permission.Name); + } + } + } + protected virtual PermissionGrantInfoDto CreatePermissionGrantInfoDto(PermissionDefinition permission) { return new PermissionGrantInfoDto @@ -139,7 +172,8 @@ public class PermissionAppService : ApplicationService, IPermissionAppService DisplayName = permission.DisplayName?.Localize(StringLocalizerFactory), ParentName = permission.Parent?.Name, AllowedProviders = permission.Providers, - GrantedProviders = new List() + GrantedProviders = new List(), + IsEditable = true }; } @@ -162,6 +196,7 @@ public class PermissionAppService : ApplicationService, IPermissionAppService public virtual async Task UpdateAsync(string providerName, string providerKey, UpdatePermissionsDto input) { await CheckProviderPolicy(providerName); + await FilterInputPermissionsByCurrentUserAsync(input); foreach (var permissionDto in input.Permissions) { @@ -379,4 +414,15 @@ public class PermissionAppService : ApplicationService, IPermissionAppService await AuthorizationService.CheckAsync(policyName); } + + protected virtual async Task FilterInputPermissionsByCurrentUserAsync(UpdatePermissionsDto input) + { + var currentUserPermissions = await PermissionChecker.IsGrantedAsync(input.Permissions.Select(p => p.Name).ToArray()); + var grantedPermissions = currentUserPermissions.Result + .Where(x => x.Value == PermissionGrantResult.Granted) + .Select(x => x.Key) + .ToHashSet(); + + input.Permissions = input.Permissions.Where(x => grantedPermissions.Contains(x.Name)).ToArray(); + } } diff --git a/modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs b/modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs index 6f6d6113e9..3061532879 100644 --- a/modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs +++ b/modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs @@ -137,7 +137,7 @@ public class PermissionManagementModal : AbpPageModel { var grantedProviders = Permissions.SelectMany(x => x.GrantedProviders); - return Permissions.All(x => x.IsGranted) && grantedProviders.All(p => p.ProviderName != currentProviderName); + return Permissions.All(x => x.IsGranted && x.IsGranted) && grantedProviders.All(p => p.ProviderName != currentProviderName); } } @@ -159,9 +159,11 @@ public class PermissionManagementModal : AbpPageModel public List GrantedProviders { get; set; } + public bool IsEditable { get; set; } + public bool IsDisabled(string currentProviderName) { - return IsGranted && GrantedProviders.All(p => p.ProviderName != currentProviderName); + return IsEditable && IsGranted && GrantedProviders.All(p => p.ProviderName != currentProviderName); } public string GetShownName(string currentProviderName) @@ -171,13 +173,19 @@ public class PermissionManagementModal : AbpPageModel return DisplayName; } + var grantedByOtherProviders = GrantedProviders + .Where(p => p.ProviderName != currentProviderName) + .ToList(); + + if (!grantedByOtherProviders.Any()) + { + return DisplayName; + } + return string.Format( "{0} ({1})", DisplayName, - GrantedProviders - .Where(p => p.ProviderName != currentProviderName) - .Select(p => p.ProviderName) - .JoinAsString(", ") + grantedByOtherProviders.JoinAsString(", ") ); } } diff --git a/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/AbpPermissionManagementApplicationTestBase.cs b/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/AbpPermissionManagementApplicationTestBase.cs index 36697e9c6c..986d751a9e 100644 --- a/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/AbpPermissionManagementApplicationTestBase.cs +++ b/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/AbpPermissionManagementApplicationTestBase.cs @@ -2,7 +2,9 @@ using System.Collections.Generic; using System.Text; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; using NSubstitute; +using Volo.Abp.Authorization.Permissions; using Volo.Abp.Users; namespace Volo.Abp.PermissionManagement; @@ -22,5 +24,9 @@ public class AbpPermissionManagementApplicationTestBase : PermissionManagementTe currentUser.IsAuthenticated.Returns(true); services.AddSingleton(currentUser); + + var fakePermissionChecker = new FakePermissionChecker(); + services.AddSingleton(fakePermissionChecker); + services.Replace(ServiceDescriptor.Singleton(fakePermissionChecker)); } } diff --git a/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/FakePermissionChecker.cs b/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/FakePermissionChecker.cs new file mode 100644 index 0000000000..19bba864c4 --- /dev/null +++ b/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/FakePermissionChecker.cs @@ -0,0 +1,53 @@ +using System.Collections.Generic; +using System.Security.Claims; +using System.Threading.Tasks; +using Volo.Abp.Authorization.Permissions; + +namespace Volo.Abp.PermissionManagement; + +public class FakePermissionChecker : IPermissionChecker +{ + private HashSet? _grantedPermissions; + + public void GrantAllPermissions() + { + _grantedPermissions = null; + } + + public void SetGrantedPermissions(params string[] permissions) + { + _grantedPermissions = new HashSet(permissions); + } + + private bool IsGranted(string name) + { + return _grantedPermissions == null || _grantedPermissions.Contains(name); + } + + public Task IsGrantedAsync(string name) + { + return Task.FromResult(IsGranted(name)); + } + + public Task IsGrantedAsync(ClaimsPrincipal? claimsPrincipal, string name) + { + return Task.FromResult(IsGranted(name)); + } + + public Task IsGrantedAsync(string[] names) + { + return IsGrantedAsync(null, names); + } + + public Task IsGrantedAsync(ClaimsPrincipal? claimsPrincipal, string[] names) + { + var result = new MultiplePermissionGrantResult(); + foreach (var name in names) + { + result.Result[name] = IsGranted(name) + ? PermissionGrantResult.Granted + : PermissionGrantResult.Undefined; + } + return Task.FromResult(result); + } +} diff --git a/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/PermissionAppService_Tests.cs b/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/PermissionAppService_Tests.cs index 61f984a27b..a0c08850d3 100644 --- a/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/PermissionAppService_Tests.cs +++ b/modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/PermissionAppService_Tests.cs @@ -16,12 +16,14 @@ public class PermissionAppService_Tests : AbpPermissionManagementApplicationTest private readonly IPermissionAppService _permissionAppService; private readonly IPermissionGrantRepository _permissionGrantRepository; private readonly ICurrentPrincipalAccessor _currentPrincipalAccessor; + private readonly FakePermissionChecker _fakePermissionChecker; public PermissionAppService_Tests() { _permissionAppService = GetRequiredService(); _permissionGrantRepository = GetRequiredService(); _currentPrincipalAccessor = GetRequiredService(); + _fakePermissionChecker = GetRequiredService(); } [Fact] @@ -135,4 +137,81 @@ public class PermissionAppService_Tests : AbpPermissionManagementApplicationTest (await _permissionGrantRepository.FindAsync("MyPermission1", "Test", "Test")).ShouldBeNull(); } + + [Fact] + public async Task Get_Should_Mark_Permissions_As_Non_Editable_When_Current_User_Does_Not_Have_Them() + { + // Current user only has MyPermission1 and MyPermission2 + _fakePermissionChecker.SetGrantedPermissions("MyPermission1", "MyPermission2"); + + var result = await _permissionAppService.GetAsync( + UserPermissionValueProvider.ProviderName, + PermissionTestDataBuilder.User1Id.ToString()); + + var testGroup = result.Groups.FirstOrDefault(g => g.Name == "TestGroup"); + testGroup.ShouldNotBeNull(); + + // Permissions the current user has -> IsEditable = true + testGroup.Permissions.First(p => p.Name == "MyPermission1").IsEditable.ShouldBeTrue(); + testGroup.Permissions.First(p => p.Name == "MyPermission2").IsEditable.ShouldBeTrue(); + + // Permissions the current user does NOT have -> IsEditable = false + testGroup.Permissions.First(p => p.Name == "MyPermission2.ChildPermission1").IsEditable.ShouldBeFalse(); + testGroup.Permissions.First(p => p.Name == "MyPermission3").IsEditable.ShouldBeFalse(); + testGroup.Permissions.First(p => p.Name == "MyPermission4").IsEditable.ShouldBeFalse(); + testGroup.Permissions.First(p => p.Name == "MyPermission6").IsEditable.ShouldBeFalse(); + testGroup.Permissions.First(p => p.Name == "MyPermission6.ChildPermission2").IsEditable.ShouldBeFalse(); + } + + [Fact] + public async Task Update_Should_Not_Grant_Permission_That_Current_User_Does_Not_Have() + { + // Current user only has MyPermission1, NOT MyPermission2 + _fakePermissionChecker.SetGrantedPermissions("MyPermission1"); + + // Try to grant both MyPermission1 and MyPermission2 + await _permissionAppService.UpdateAsync("Test", "Test", new UpdatePermissionsDto() + { + Permissions = new UpdatePermissionDto[] + { + new UpdatePermissionDto() { IsGranted = true, Name = "MyPermission1" }, + new UpdatePermissionDto() { IsGranted = true, Name = "MyPermission2" } + } + }); + + // MyPermission1 should be granted (current user has it) + (await _permissionGrantRepository.FindAsync("MyPermission1", "Test", "Test")).ShouldNotBeNull(); + + // MyPermission2 should NOT be granted (current user doesn't have it, filtered out) + (await _permissionGrantRepository.FindAsync("MyPermission2", "Test", "Test")).ShouldBeNull(); + } + + [Fact] + public async Task Update_Should_Not_Revoke_Permission_That_Current_User_Does_Not_Have() + { + // First, grant both permissions + await _permissionGrantRepository.InsertAsync( + new PermissionGrant(Guid.NewGuid(), "MyPermission1", "Test", "Test")); + await _permissionGrantRepository.InsertAsync( + new PermissionGrant(Guid.NewGuid(), "MyPermission2", "Test", "Test")); + + // Current user only has MyPermission1, NOT MyPermission2 + _fakePermissionChecker.SetGrantedPermissions("MyPermission1"); + + // Try to revoke both + await _permissionAppService.UpdateAsync("Test", "Test", new UpdatePermissionsDto() + { + Permissions = new UpdatePermissionDto[] + { + new UpdatePermissionDto() { IsGranted = false, Name = "MyPermission1" }, + new UpdatePermissionDto() { IsGranted = false, Name = "MyPermission2" } + } + }); + + // MyPermission1 should be revoked (current user has it) + (await _permissionGrantRepository.FindAsync("MyPermission1", "Test", "Test")).ShouldBeNull(); + + // MyPermission2 should still be granted (current user doesn't have it, revoke filtered out) + (await _permissionGrantRepository.FindAsync("MyPermission2", "Test", "Test")).ShouldNotBeNull(); + } } From 771f96031d3c1672551efa104fae8afe563d64cf Mon Sep 17 00:00:00 2001 From: maliming Date: Tue, 3 Feb 2026 15:03:26 +0800 Subject: [PATCH 2/4] Refactor permission management logic to simplify IsDisabled checks --- .../PermissionManagementModal.cshtml.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs b/modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs index 3061532879..8b62db692d 100644 --- a/modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs +++ b/modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs @@ -135,9 +135,7 @@ public class PermissionManagementModal : AbpPageModel public bool IsDisabled(string currentProviderName) { - var grantedProviders = Permissions.SelectMany(x => x.GrantedProviders); - - return Permissions.All(x => x.IsGranted && x.IsGranted) && grantedProviders.All(p => p.ProviderName != currentProviderName); + return Permissions.All(p => p.IsDisabled(currentProviderName)); } } @@ -163,7 +161,7 @@ public class PermissionManagementModal : AbpPageModel public bool IsDisabled(string currentProviderName) { - return IsEditable && IsGranted && GrantedProviders.All(p => p.ProviderName != currentProviderName); + return !IsEditable || (IsGranted && GrantedProviders.All(p => p.ProviderName != currentProviderName)); } public string GetShownName(string currentProviderName) @@ -175,6 +173,7 @@ public class PermissionManagementModal : AbpPageModel var grantedByOtherProviders = GrantedProviders .Where(p => p.ProviderName != currentProviderName) + .Select(p => p.ProviderName) .ToList(); if (!grantedByOtherProviders.Any()) From e2ceb453167829f51bdb059abf5fa9b04b6bd966 Mon Sep 17 00:00:00 2001 From: Ma Liming Date: Tue, 3 Feb 2026 16:00:01 +0800 Subject: [PATCH 3/4] Update modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../Volo/Abp/PermissionManagement/PermissionAppService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs b/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs index 98c7b2bc35..4f06f2de93 100644 --- a/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs +++ b/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs @@ -128,7 +128,7 @@ public class PermissionAppService : ApplicationService, IPermissionAppService } } - // Filter permissions for non-admin users: only show permissions they have or that are already granted + // Filter permissions for the current user: only show permissions they have or that are already granted await FilterOutputPermissionsByCurrentUserAsync(result); return result; From fdae1e3fb9bf8c8e6d78b1b58cb3482e2b96c2fb Mon Sep 17 00:00:00 2001 From: maliming Date: Tue, 3 Feb 2026 17:01:40 +0800 Subject: [PATCH 4/4] Use Lazy for principal and guard empty permissions --- .../Identity/FakeCurrentPrincipalAccessor.cs | 36 +++++++------------ .../PermissionAppService.cs | 7 ++++ 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs b/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs index 53b1e3cd4e..dabd95dbc3 100644 --- a/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs +++ b/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Security.Claims; using Volo.Abp.DependencyInjection; @@ -9,10 +10,21 @@ namespace Volo.Abp.Identity; public class FakeCurrentPrincipalAccessor : ThreadCurrentPrincipalAccessor { private readonly IdentityTestData _testData; + private readonly Lazy _principal; public FakeCurrentPrincipalAccessor(IdentityTestData testData) { _testData = testData; + _principal = new Lazy(() => new ClaimsPrincipal( + new ClaimsIdentity( + new List + { + new Claim(AbpClaimTypes.UserId, _testData.UserAdminId.ToString()), + new Claim(AbpClaimTypes.UserName, "administrator"), + new Claim(AbpClaimTypes.Email, "administrator@abp.io") + } + ) + )); } protected override ClaimsPrincipal GetClaimsPrincipal() @@ -20,30 +32,8 @@ public class FakeCurrentPrincipalAccessor : ThreadCurrentPrincipalAccessor return GetPrincipal(); } - private ClaimsPrincipal _principal; - private ClaimsPrincipal GetPrincipal() { - if (_principal == null) - { - lock (this) - { - if (_principal == null) - { - _principal = new ClaimsPrincipal( - new ClaimsIdentity( - new List - { - new Claim(AbpClaimTypes.UserId, _testData.UserAdminId.ToString()), - new Claim(AbpClaimTypes.UserName, "administrator"), - new Claim(AbpClaimTypes.Email, "administrator@abp.io") - } - ) - ); - } - } - } - - return _principal; + return _principal.Value; } } diff --git a/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs b/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs index 4f06f2de93..7cd83916f2 100644 --- a/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs +++ b/modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs @@ -417,12 +417,19 @@ public class PermissionAppService : ApplicationService, IPermissionAppService protected virtual async Task FilterInputPermissionsByCurrentUserAsync(UpdatePermissionsDto input) { + if (input.Permissions.IsNullOrEmpty()) + { + input.Permissions = Array.Empty(); + return; + } + var currentUserPermissions = await PermissionChecker.IsGrantedAsync(input.Permissions.Select(p => p.Name).ToArray()); var grantedPermissions = currentUserPermissions.Result .Where(x => x.Value == PermissionGrantResult.Granted) .Select(x => x.Key) .ToHashSet(); + // Filters the input DTO in-place to only include manageable permissions. input.Permissions = input.Permissions.Where(x => grantedPermissions.Contains(x.Name)).ToArray(); } }