diff --git a/framework/src/Volo.Abp.Security/Volo/Abp/Roles/AbpRoleConsts.cs b/framework/src/Volo.Abp.Security/Volo/Abp/Roles/AbpRoleConsts.cs new file mode 100644 index 00000000000..675699788d0 --- /dev/null +++ b/framework/src/Volo.Abp.Security/Volo/Abp/Roles/AbpRoleConsts.cs @@ -0,0 +1,10 @@ +namespace Volo.Abp.Roles; + +public static class AbpRoleConsts +{ + /// + /// The static name of the admin role. + /// Default value: "admin" + /// + public const string AdminRoleName = "admin"; +} 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 582d9ade424..f9b5a53a466 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,8 @@ using Volo.Abp.Authorization.Permissions; using Volo.Abp.Data; using Volo.Abp.ObjectExtending; +using Volo.Abp.Roles; +using Volo.Abp.Users; namespace Volo.Abp.Identity; @@ -69,7 +71,18 @@ public virtual async Task> GetRolesAsync(Guid id) [Authorize(IdentityPermissions.Users.Default)] public virtual async Task> GetAssignableRolesAsync() { - var list = (await RoleRepository.GetListAsync()).OrderBy(x => x.Name).ToList(); + List list; + + if (await HasAdminRoleAsync()) + { + list = (await RoleRepository.GetListAsync()).OrderBy(x => x.Name).ToList(); + } + else + { + var currentUserRoles = await UserManager.GetRolesAsync(await UserManager.GetByIdAsync(CurrentUser.GetId())); + list = (await RoleRepository.GetListAsync(currentUserRoles)).OrderBy(x => x.Name).ToList(); + } + return new ListResultDto(ObjectMapper.Map, List>(list)); } @@ -145,7 +158,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 +204,38 @@ 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 FilterRolesByCurrentUserAsync(IdentityUser user, string[] inputRoleNames) + { + if (await HasAdminRoleAsync()) + { + return (inputRoleNames ?? Array.Empty()) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToArray(); } + + 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, StringComparer.OrdinalIgnoreCase); + + var desiredManageableRoles = inputRoleNameSet.Intersect(operatorOwnRoleSet, StringComparer.OrdinalIgnoreCase); + + return keepUnmanageableRoles + .Concat(desiredManageableRoles) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToArray(); + } + + protected virtual Task HasAdminRoleAsync() + { + return Task.FromResult(CurrentUser.IsInRole(AbpRoleConsts.AdminRoleName)); } } diff --git a/modules/identity/src/Volo.Abp.Identity.Blazor/Pages/Identity/UserManagement.razor b/modules/identity/src/Volo.Abp.Identity.Blazor/Pages/Identity/UserManagement.razor index e71e72b002c..f146ab644f7 100644 --- a/modules/identity/src/Volo.Abp.Identity.Blazor/Pages/Identity/UserManagement.razor +++ b/modules/identity/src/Volo.Abp.Identity.Blazor/Pages/Identity/UserManagement.razor @@ -142,13 +142,13 @@ @if (NewUserRoles != null) { - @foreach (var role in NewUserRoles) - { - - - @role.Name - - } + @foreach (var role in NewUserRoles) + { + + + @role.Name + + } } @@ -277,7 +277,7 @@ { - @role.Name + @role.Name } } diff --git a/modules/identity/src/Volo.Abp.Identity.Blazor/Pages/Identity/UserManagement.razor.cs b/modules/identity/src/Volo.Abp.Identity.Blazor/Pages/Identity/UserManagement.razor.cs index a52474b8b16..fc35245801c 100644 --- a/modules/identity/src/Volo.Abp.Identity.Blazor/Pages/Identity/UserManagement.razor.cs +++ b/modules/identity/src/Volo.Abp.Identity.Blazor/Pages/Identity/UserManagement.razor.cs @@ -103,7 +103,8 @@ protected override async Task OpenCreateModalAsync() NewUserRoles = Roles.Select(x => new AssignedRoleViewModel { Name = x.Name, - IsAssigned = x.IsDefault + IsAssigned = x.IsDefault, + IsAssignable = true }).ToArray(); ChangePasswordTextRole(TextRole.Password); @@ -130,12 +131,23 @@ protected override async Task OpenEditModalAsync(IdentityUserDto entity) if (await PermissionChecker.IsGrantedAsync(IdentityPermissions.Users.ManageRoles)) { - var userRoleIds = (await AppService.GetRolesAsync(entity.Id)).Items.Select(r => r.Id).ToList(); + var assignableRoles = Roles ?? (await AppService.GetAssignableRolesAsync()).Items; + var currentRoles = (await AppService.GetRolesAsync(entity.Id)).Items; - EditUserRoles = Roles.Select(x => new AssignedRoleViewModel + var combinedRoles = assignableRoles + .Concat(currentRoles) + .GroupBy(role => role.Id) + .Select(group => group.First()) + .ToList(); + + var currentRoleIds = currentRoles.Select(r => r.Id).ToHashSet(); + var assignableRoleIds = assignableRoles.Select(r => r.Id).ToHashSet(); + + EditUserRoles = combinedRoles.Select(x => new AssignedRoleViewModel { Name = x.Name, - IsAssigned = userRoleIds.Contains(x.Id) + IsAssigned = currentRoleIds.Contains(x.Id), + IsAssignable = assignableRoleIds.Contains(x.Id) }).ToArray(); ChangePasswordTextRole(TextRole.Password); @@ -262,4 +274,6 @@ public class AssignedRoleViewModel public string Name { get; set; } public bool IsAssigned { get; set; } + + public bool IsAssignable { get; set; } } diff --git a/modules/identity/src/Volo.Abp.Identity.Domain/Volo/Abp/Identity/IdentityDataSeeder.cs b/modules/identity/src/Volo.Abp.Identity.Domain/Volo/Abp/Identity/IdentityDataSeeder.cs index 1c153effd18..6cfa5a329d0 100644 --- a/modules/identity/src/Volo.Abp.Identity.Domain/Volo/Abp/Identity/IdentityDataSeeder.cs +++ b/modules/identity/src/Volo.Abp.Identity.Domain/Volo/Abp/Identity/IdentityDataSeeder.cs @@ -5,6 +5,7 @@ using Volo.Abp.DependencyInjection; using Volo.Abp.Guids; using Volo.Abp.MultiTenancy; +using Volo.Abp.Roles; using Volo.Abp.Uow; namespace Volo.Abp.Identity; @@ -83,7 +84,7 @@ public virtual async Task SeedAsync( result.CreatedAdminUser = true; //"admin" role - const string adminRoleName = "admin"; + const string adminRoleName = AbpRoleConsts.AdminRoleName; var adminRole = await RoleRepository.FindByNormalizedNameAsync(LookupNormalizer.NormalizeName(adminRoleName)); if (adminRole == null) 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 0c85be8f183..b60978f32ec 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 074f3f84817..08ea27081b8 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 virtual async Task OnGetAsync(Guid id) 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 AssignedRoleViewModel 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 00000000000..dabd95dbc33 --- /dev/null +++ b/modules/identity/test/Volo.Abp.Identity.Application.Tests/Volo/Abp/Identity/FakeCurrentPrincipalAccessor.cs @@ -0,0 +1,39 @@ +using System; +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; + 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() + { + return GetPrincipal(); + } + + private ClaimsPrincipal GetPrincipal() + { + return _principal.Value; + } +} 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 acd50b79474..0c1a8d6ffc1 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 IdentityUserAppService_Tests() _permissionManager = GetRequiredService(); _userPermissionManagementProvider = GetRequiredService(); _testData = GetRequiredService(); + _currentPrincipalAccessor = GetRequiredService(); } [Fact] @@ -239,6 +244,137 @@ 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() + } + ); + } + + // "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_Admin_Can_Assign_Any_Role() + { + // admin user can assign roles they do not have (e.g. "sale") + using (_currentPrincipalAccessor.Change(new[] + { + new Claim(AbpClaimTypes.UserId, _testData.UserAdminId.ToString()), + new Claim(AbpClaimTypes.Role, "admin") + })) + { + await _userAppService.UpdateRolesAsync( + _testData.UserDavidId, + new IdentityUserUpdateRolesDto + { + RoleNames = new[] { "sale" } + } + ); + } + + var roleNames = await _userRepository.GetRoleNamesAsync(_testData.UserDavidId); + roleNames.ShouldContain("sale"); + } + + [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 15ef80b4c4b..7132dd9662b 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 @@ 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); 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 ffeb1220b18..a677fb48416 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 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); } } 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 b6cb14de5ca..0b8cc2ce42f 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 762ceb5ff36..ebcc5945152 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 @@ -122,7 +122,8 @@ 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"); } @@ -130,14 +131,17 @@ public async Task GetListByNormalizedRoleNameAsync() [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 b608a69b54d..79ddc93047e 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 e26e387fae7..5baf233c8c0 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 @@ -10,6 +10,8 @@ using Volo.Abp.MultiTenancy; using Volo.Abp.SimpleStateChecking; using Volo.Abp.PermissionManagement.Localization; +using Volo.Abp.Roles; +using Volo.Abp.Users; namespace Volo.Abp.PermissionManagement; @@ -128,9 +130,47 @@ protected virtual async Task GetInternalAsync(string } } + // Filter permissions for the current user: only show permissions they have or that are already granted + await FilterOutputPermissionsByCurrentUserAsync(result); + return result; } + protected virtual async Task FilterOutputPermissionsByCurrentUserAsync(GetPermissionListResultDto result) + { + if (await HasAdminRoleAsync()) + { + return; + } + + // 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 +179,8 @@ protected virtual PermissionGrantInfoDto CreatePermissionGrantInfoDto(Permission DisplayName = permission.DisplayName?.Localize(StringLocalizerFactory), ParentName = permission.Parent?.Name, AllowedProviders = permission.Providers, - GrantedProviders = new List() + GrantedProviders = new List(), + IsEditable = true }; } @@ -162,6 +203,7 @@ protected virtual PermissionGroupDto CreatePermissionGroupDto(PermissionGroupDef 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 +421,32 @@ protected virtual async Task CheckProviderPolicy(string providerName) await AuthorizationService.CheckAsync(policyName); } + + protected virtual async Task FilterInputPermissionsByCurrentUserAsync(UpdatePermissionsDto input) + { + if (await HasAdminRoleAsync()) + { + return; + } + + 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(); + } + + protected virtual Task HasAdminRoleAsync() + { + return Task.FromResult(CurrentUser.IsInRole(AbpRoleConsts.AdminRoleName)); + } } diff --git a/modules/permission-management/src/Volo.Abp.PermissionManagement.Blazor/Components/PermissionManagementModal.razor.cs b/modules/permission-management/src/Volo.Abp.PermissionManagement.Blazor/Components/PermissionManagementModal.razor.cs index d2a105a14d9..356e334f919 100644 --- a/modules/permission-management/src/Volo.Abp.PermissionManagement.Blazor/Components/PermissionManagementModal.razor.cs +++ b/modules/permission-management/src/Volo.Abp.PermissionManagement.Blazor/Components/PermissionManagementModal.razor.cs @@ -28,8 +28,6 @@ public partial class PermissionManagementModal protected List _allGroups; protected List _groups; - protected List _disabledPermissions = new List(); - protected string _selectedTabName; protected bool _selectAllDisabled; @@ -102,19 +100,6 @@ protected virtual void NormalizePermissionGroup(bool checkDisabledPermissions = { _selectAllDisabled = _groups.All(IsPermissionGroupDisabled); - if (checkDisabledPermissions) - { - _disabledPermissions.Clear(); - } - - foreach (var permission in _groups.SelectMany(x => x.Permissions)) - { - if (checkDisabledPermissions && permission.IsGranted && permission.GrantedProviders.All(x => x.ProviderName != _providerName)) - { - _disabledPermissions.Add(permission); - } - } - foreach (var group in _groups) { SetPermissionDepths(group.Permissions, null, 0); @@ -285,12 +270,28 @@ protected virtual void GetChildPermissions(List allChild protected virtual bool IsDisabledPermission(PermissionGrantInfoDto permissionGrantInfo) { - return _disabledPermissions.Any(x => x == permissionGrantInfo); + if (!permissionGrantInfo.IsEditable) + { + return true; + } + + return permissionGrantInfo.IsGranted && + permissionGrantInfo.GrantedProviders.All(p => p.ProviderName != _providerName); } protected virtual string GetShownName(PermissionGrantInfoDto permissionGrantInfo) { - if (!IsDisabledPermission(permissionGrantInfo)) + if (permissionGrantInfo.GrantedProviders.All(p => p.ProviderName == _providerName)) + { + return permissionGrantInfo.DisplayName; + } + + var grantedByOtherProviders = permissionGrantInfo.GrantedProviders + .Where(p => p.ProviderName != _providerName) + .Select(p => p.ProviderName) + .ToList(); + + if (!grantedByOtherProviders.Any()) { return permissionGrantInfo.DisplayName; } @@ -298,10 +299,7 @@ protected virtual string GetShownName(PermissionGrantInfoDto permissionGrantInfo return string.Format( "{0} ({1})", permissionGrantInfo.DisplayName, - permissionGrantInfo.GrantedProviders - .Where(p => p.ProviderName != _providerName) - .Select(p => p.ProviderName) - .JoinAsString(", ") + grantedByOtherProviders.JoinAsString(", ") ); } @@ -313,10 +311,7 @@ protected virtual Task ClosingModal(ModalClosingEventArgs eventArgs) protected virtual bool IsPermissionGroupDisabled(PermissionGroupDto group) { - var permissions = group.Permissions; - var grantedProviders = permissions.SelectMany(x => x.GrantedProviders); - - return permissions.All(x => x.IsGranted) && grantedProviders.Any(p => p.ProviderName != _providerName); + return group.Permissions.All(IsDisabledPermission); } protected virtual async Task ResetSearchTextAsync() diff --git a/modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/PermissionDataSeedContributor.cs b/modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/PermissionDataSeedContributor.cs index 71570aafce8..89390d1515c 100644 --- a/modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/PermissionDataSeedContributor.cs +++ b/modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/PermissionDataSeedContributor.cs @@ -4,6 +4,7 @@ using Volo.Abp.Data; using Volo.Abp.DependencyInjection; using Volo.Abp.MultiTenancy; +using Volo.Abp.Roles; namespace Volo.Abp.PermissionManagement; @@ -34,7 +35,7 @@ public virtual async Task SeedAsync(DataSeedContext context) await PermissionDataSeeder.SeedAsync( RolePermissionValueProvider.ProviderName, - "admin", + AbpRoleConsts.AdminRoleName, permissionNames, context?.TenantId ); 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 6f6d6113e9f..8b62db692db 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 string GetNormalizedGroupName() public bool IsDisabled(string currentProviderName) { - var grantedProviders = Permissions.SelectMany(x => x.GrantedProviders); - - return Permissions.All(x => x.IsGranted) && grantedProviders.All(p => p.ProviderName != currentProviderName); + return Permissions.All(p => p.IsDisabled(currentProviderName)); } } @@ -159,9 +157,11 @@ public class PermissionGrantInfoViewModel : IFlatTreeItem 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 +171,20 @@ public string GetShownName(string currentProviderName) return DisplayName; } + var grantedByOtherProviders = GrantedProviders + .Where(p => p.ProviderName != currentProviderName) + .Select(p => p.ProviderName) + .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 36697e9c6c5..6756b96da82 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 @@ -1,9 +1,7 @@ using System; -using System.Collections.Generic; -using System.Text; using Microsoft.Extensions.DependencyInjection; -using NSubstitute; -using Volo.Abp.Users; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Volo.Abp.Authorization.Permissions; namespace Volo.Abp.PermissionManagement; @@ -17,10 +15,8 @@ protected AbpPermissionManagementApplicationTestBase() } protected override void AfterAddApplication(IServiceCollection services) { - var currentUser = Substitute.For(); - currentUser.Roles.Returns(new[] { "admin" }); - 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 00000000000..19bba864c48 --- /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 61f984a27b5..1fddf071815 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,122 @@ await _permissionAppService.UpdateAsync("Test", (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 Get_Should_Allow_Admin_To_Edit_All_Permissions() + { + // Current user does NOT have these permissions, but has admin role + _fakePermissionChecker.SetGrantedPermissions("MyPermission1"); + + using (_currentPrincipalAccessor.Change(new Claim(AbpClaimTypes.Role, "admin"))) + { + var result = await _permissionAppService.GetAsync( + UserPermissionValueProvider.ProviderName, + PermissionTestDataBuilder.User1Id.ToString()); + + var testGroup = result.Groups.FirstOrDefault(g => g.Name == "TestGroup"); + testGroup.ShouldNotBeNull(); + + testGroup.Permissions.First(p => p.Name == "MyPermission3").IsEditable.ShouldBeTrue(); + testGroup.Permissions.First(p => p.Name == "MyPermission6").IsEditable.ShouldBeTrue(); + } + } + + [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(); + } + + [Fact] + public async Task Update_Should_Allow_Admin_To_Grant_Permissions_Without_Having_Them() + { + (await _permissionGrantRepository.FindAsync("MyPermission2", "Test", "Test")).ShouldBeNull(); + + _fakePermissionChecker.SetGrantedPermissions(); + + using (_currentPrincipalAccessor.Change(new Claim(AbpClaimTypes.Role, "admin"))) + { + await _permissionAppService.UpdateAsync("Test", "Test", new UpdatePermissionsDto() + { + Permissions = new UpdatePermissionDto[] + { + new UpdatePermissionDto() { IsGranted = true, Name = "MyPermission2" } + } + }); + } + + (await _permissionGrantRepository.FindAsync("MyPermission2", "Test", "Test")).ShouldNotBeNull(); + } }