Don't assign role base claims when a role has full access

OrchardCMS · Sep 23, 2024 · cfc94a7 · cfc94a7
1 parent 067ad37
commit cfc94a7
Show file tree

Hide file tree

Showing 12 changed files with 121 additions and 34 deletions.
diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs b/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs
@@ -31,6 +31,7 @@
 using OrchardCore.Recipes;
 using OrchardCore.Recipes.Services;
 using OrchardCore.ResourceManagement;
+using OrchardCore.Roles;
 using OrchardCore.Security;
 using OrchardCore.Security.Permissions;
 using OrchardCore.Settings.Deployment;
@@ -277,6 +278,7 @@ public sealed class RolesStartup : StartupBase
 {
     public override void ConfigureServices(IServiceCollection services)
     {
+        services.AddScoped<IUserClaimsProvider, RoleClaimsProvider>();
         services.AddScoped<IRoleRemovedEventHandler, UserRoleRemovedEventHandler>();
         services.AddIndexProvider<UserByRoleNameIndexProvider>();
         services.AddScoped<IDisplayDriver<User>, UserRoleDisplayDriver>();

diff --git a/src/OrchardCore/OrchardCore.Roles.Core/IRoleTracker.cs b/src/OrchardCore/OrchardCore.Roles.Core/IRoleTracker.cs
@@ -5,9 +5,9 @@ namespace OrchardCore.Roles.Core;
 
 public interface IRoleTracker
 {
-    Task AddAsync(IRole role);
+    ValueTask AddAsync(IRole role);
 
-    Task<IReadOnlySet<string>> GetAsync();
+    ValueTask<IReadOnlySet<string>> GetAsync();
 
-    Task RemoveAsync(IRole role);
+    ValueTask RemoveAsync(IRole role);
 }
diff --git a/src/OrchardCore/OrchardCore.Roles.Core/OrchardCore.Roles.Core.csproj b/src/OrchardCore/OrchardCore.Roles.Core/OrchardCore.Roles.Core.csproj
@@ -16,6 +16,7 @@
 
   <ItemGroup>
     <ProjectReference Include="..\..\OrchardCore\OrchardCore.Infrastructure.Abstractions\OrchardCore.Infrastructure.Abstractions.csproj" />
+    <ProjectReference Include="..\OrchardCore.Users.Abstractions\OrchardCore.Users.Abstractions.csproj" />
   </ItemGroup>
 
 </Project>
diff --git a/src/OrchardCore/OrchardCore.Roles.Core/RoleTracker.cs b/src/OrchardCore/OrchardCore.Roles.Core/RoleTracker.cs
@@ -13,11 +13,10 @@ public class RoleTracker : IRoleTracker
     private readonly IDistributedCache _distributedCache;
     private readonly IMemoryCache _memoryCache;
     private readonly RoleManager<IRole> _roleManager;
+    private readonly SemaphoreSlim _semaphore = new(1);
 
     private HashSet<string> _roleWithFullAccess;
 
-    private readonly SemaphoreSlim _semaphore = new(1);
-
     public RoleTracker(
         IDistributedCache distributedCache,
         IMemoryCache memoryCache,
@@ -29,7 +28,7 @@ public RoleTracker(
         memoryCache.TryGetValue(_roleTrackerCacheKey, out _roleWithFullAccess);
     }
 
-    public async Task<IReadOnlySet<string>> GetAsync()
+    public async ValueTask<IReadOnlySet<string>> GetAsync()
     {
         if (_roleWithFullAccess is null)
         {
@@ -39,7 +38,7 @@ public async Task<IReadOnlySet<string>> GetAsync()
         return _roleWithFullAccess;
     }
 
-    public async Task AddAsync(IRole role)
+    public async ValueTask AddAsync(IRole role)
     {
         if (_roleWithFullAccess is null)
         {
@@ -64,7 +63,7 @@ public async Task AddAsync(IRole role)
         }
     }
 
-    public async Task RemoveAsync(IRole role)
+    public async ValueTask RemoveAsync(IRole role)
     {
         if (_roleWithFullAccess is null)
         {

diff --git a/src/OrchardCore/OrchardCore.Roles.Core/RolesPermissionHandler.cs b/src/OrchardCore/OrchardCore.Roles.Core/RolesPermissionHandler.cs
@@ -1,16 +1,22 @@
 using System.Security.Claims;
 using Microsoft.AspNetCore.Authorization;
+using Microsoft.AspNetCore.Identity;
+using Microsoft.Extensions.Options;
 using OrchardCore.Security;
 
 namespace OrchardCore.Roles.Core;
 
 public class RolesPermissionHandler : AuthorizationHandler<PermissionRequirement>
 {
     private readonly IRoleTracker _roleTracker;
+    private readonly IdentityOptions _identityOptions;
 
-    public RolesPermissionHandler(IRoleTracker roleTracker)
+    public RolesPermissionHandler(
+        IRoleTracker roleTracker,
+        IOptions<IdentityOptions> identityOptions)
     {
         _roleTracker = roleTracker;
+        _identityOptions = identityOptions.Value;
     }
 
     protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, PermissionRequirement requirement)
@@ -22,16 +28,18 @@ protected override async Task HandleRequirementAsync(AuthorizationHandlerContext
 
         var rolesWithFullAccess = await _roleTracker.GetAsync();
 
-        if (rolesWithFullAccess.Count > 0)
+        if (rolesWithFullAccess.Count == 0)
         {
-            foreach (var role in context.User.FindAll(c => c.Type is "role" or ClaimTypes.Role))
+            return;
+        }
+
+        foreach (var role in context.User.FindAll(c => c.Type == _identityOptions.ClaimsIdentity.RoleClaimType || c.Type is "role" or ClaimTypes.Role))
+        {
+            if (rolesWithFullAccess.Contains(role.Value))
             {
-                if (rolesWithFullAccess.Contains(role.Value))
-                {
-                    context.Succeed(requirement);
+                context.Succeed(requirement);
 
-                    return;
-                }
+                return;
             }
         }
     }

diff --git a/src/OrchardCore/OrchardCore.Users.Core/Services/DefaultUserClaimsPrincipalProviderFactory.cs b/src/OrchardCore/OrchardCore.Users.Core/Services/DefaultUserClaimsPrincipalProviderFactory.cs
@@ -3,24 +3,23 @@
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using OrchardCore.Modules;
-using OrchardCore.Security;
 
 namespace OrchardCore.Users.Services;
 
 /// <summary>
 /// Custom implementation of <see cref="IUserClaimsPrincipalFactory{TUser}"/> allowing adding claims by implementing the <see cref="IUserClaimsProvider"/>.
 /// </summary>
-public class DefaultUserClaimsPrincipalProviderFactory : UserClaimsPrincipalFactory<IUser, IRole>
+public class DefaultUserClaimsPrincipalProviderFactory : UserClaimsPrincipalFactory<IUser>
 {
     private readonly IEnumerable<IUserClaimsProvider> _claimsProviders;
     private readonly ILogger _logger;
 
     public DefaultUserClaimsPrincipalProviderFactory(
         UserManager<IUser> userManager,
-        RoleManager<IRole> roleManager,
         IOptions<IdentityOptions> identityOptions,
         IEnumerable<IUserClaimsProvider> claimsProviders,
-        ILogger<DefaultUserClaimsPrincipalProviderFactory> logger) : base(userManager, roleManager, identityOptions)
+        ILogger<DefaultUserClaimsPrincipalProviderFactory> logger)
+        : base(userManager, identityOptions)
     {
         _claimsProviders = claimsProviders;
         _logger = logger;

diff --git a/src/OrchardCore/OrchardCore.Users.Core/Services/RoleClaimsProvider.cs b/src/OrchardCore/OrchardCore.Users.Core/Services/RoleClaimsProvider.cs
@@ -0,0 +1,65 @@
+using System.Security.Claims;
+using Microsoft.AspNetCore.Identity;
+using Microsoft.Extensions.Options;
+using OrchardCore.Security;
+using OrchardCore.Users;
+using OrchardCore.Users.Services;
+
+namespace OrchardCore.Roles;
+
+public class RoleClaimsProvider : IUserClaimsProvider
+{
+    private readonly UserManager<IUser> _userManager;
+    private readonly RoleManager<IRole> _roleManager;
+    private readonly IdentityOptions _identityOptions;
+
+    public RoleClaimsProvider(
+        UserManager<IUser> userManager,
+        RoleManager<IRole> roleManager,
+        IOptions<IdentityOptions> identityOptions)
+    {
+        _userManager = userManager;
+        _roleManager = roleManager;
+        _identityOptions = identityOptions.Value;
+    }
+
+    public async Task GenerateAsync(IUser user, ClaimsIdentity claims)
+    {
+        if (!_userManager.SupportsUserRole)
+        {
+            return;
+        }
+
+        var roleNames = await _userManager.GetRolesAsync(user);
+        var roles = new List<IRole>();
+
+        foreach (var roleName in roleNames)
+        {
+            claims.AddClaim(new Claim(_identityOptions.ClaimsIdentity.RoleClaimType, roleName));
+
+            if (!_roleManager.SupportsRoleClaims)
+            {
+                continue;
+            }
+
+            var role = await _roleManager.FindByNameAsync(roleName);
+
+            if (role == null)
+            {
+                continue;
+            }
+
+            roles.Add(role);
+        }
+
+        if (roles.Count == 0 || roles.Any(x => x.HasFullAccess))
+        {
+            return;
+        }
+
+        foreach (var role in roles)
+        {
+            claims.AddClaims(await _roleManager.GetClaimsAsync(role));
+        }
+    }
+}
diff --git a/src/docs/releases/2.1.0.md b/src/docs/releases/2.1.0.md
@@ -71,6 +71,9 @@ Two new properties have been added to the `IRole` interface:
 
 - **`HasFullAccess`**: This property eliminates the need to specify individual permissions. When set to `true`, the role is granted all permissions by default, which is particularly useful for roles like `Administrator`.
 
+!!! note
+    When a user with a role that has `HasFullAccess` logs in, role-specific claims are excluded from the claims principal.
+
 ### Azure Communication Services Email Feature
 
 #### Azure Communication Services Email Feature Name Update

diff --git a/...rchardCore.Users/OrchardCore.Users.Core/DefaultUserClaimsPrincipalProviderFactoryTests.cs b/...rchardCore.Users/OrchardCore.Users.Core/DefaultUserClaimsPrincipalProviderFactoryTests.cs
@@ -1,4 +1,3 @@
-using OrchardCore.Security;
 using OrchardCore.Users;
 using OrchardCore.Users.Models;
 using OrchardCore.Users.Services;
@@ -24,8 +23,6 @@ public async Task EnsurePrincipalHasExpectedClaims(bool emailVerified)
             userManager.Setup(m => m.IsEmailConfirmedAsync(user)).ReturnsAsync(emailVerified);
         }
 
-        var roleManager = UsersMockHelper.MockRoleManager<IRole>().Object;
-
         var options = new Mock<IOptions<IdentityOptions>>();
         options.Setup(a => a.Value).Returns(new IdentityOptions());
 
@@ -34,7 +31,7 @@ public async Task EnsurePrincipalHasExpectedClaims(bool emailVerified)
             new EmailClaimsProvider(userManager.Object)
         };
 
-        var factory = new DefaultUserClaimsPrincipalProviderFactory(userManager.Object, roleManager, options.Object, claimsProviders, null);
+        var factory = new DefaultUserClaimsPrincipalProviderFactory(userManager.Object, options.Object, claimsProviders, null);
 
         //Act
         var principal = await factory.CreateAsync(user);

diff --git a/test/OrchardCore.Tests/Security/PermissionHandlerTests.cs b/test/OrchardCore.Tests/Security/PermissionHandlerTests.cs
@@ -96,7 +96,7 @@ public async Task RolesWithFullAccessShouldAutoGrantPermissions()
 
         var context = PermissionHandlerHelper.CreateTestAuthorizationHandlerContext(required, [adminRolePermission], true);
 
-        var permissionHandler = new RolesPermissionHandler(new RoleTrackerTest(["Administrator"]));
+        var permissionHandler = RolesPermissionHandlerTests.GetRolesPermissionHandler("Administrator");
 
         // Act
         await permissionHandler.HandleAsync(context);

diff --git a/test/OrchardCore.Tests/Security/RoleTrackerTest.cs b/test/OrchardCore.Tests/Security/RoleTrackerTest.cs
@@ -19,22 +19,22 @@ public RoleTrackerTest(IEnumerable<string> roles)
             _roles.Add(role);
         }
     }
-    public Task AddAsync(IRole role)
+    public ValueTask AddAsync(IRole role)
     {
         _roles.Add(role.RoleName);
 
-        return Task.CompletedTask;
+        return ValueTask.CompletedTask;
     }
 
-    public Task<IReadOnlySet<string>> GetAsync()
+    public ValueTask<IReadOnlySet<string>> GetAsync()
     {
-        return Task.FromResult<IReadOnlySet<string>>(_roles);
+        return ValueTask.FromResult<IReadOnlySet<string>>(_roles);
     }
 
-    public Task RemoveAsync(IRole role)
+    public ValueTask RemoveAsync(IRole role)
     {
         _roles.Remove(role.RoleName);
 
-        return Task.CompletedTask;
+        return ValueTask.CompletedTask;
     }
 }
diff --git a/test/OrchardCore.Tests/Security/RolesPermissionHandlerTests.cs b/test/OrchardCore.Tests/Security/RolesPermissionHandlerTests.cs
@@ -1,4 +1,4 @@
-using OrchardCore.Roles.Core;
+using OrchardCore.Roles.Core;
 using OrchardCore.Security.Permissions;
 
 namespace OrchardCore.Tests.Security;
@@ -14,7 +14,9 @@ public async Task RolesWithFullAccessShouldAutoGrantPermissions()
 
         var context = PermissionHandlerHelper.CreateTestAuthorizationHandlerContext(required, [adminRolePermission], true);
 
-        var permissionHandler = new RolesPermissionHandler(new RoleTrackerTest(["Administrator"]));
+        var options = new Mock<IOptions<IdentityOptions>>();
+
+        var permissionHandler = GetRolesPermissionHandler("Administrator");
 
         // Act
         await permissionHandler.HandleAsync(context);
@@ -32,12 +34,23 @@ public async Task RolesWithNoFullAccessShouldNotGrantPermissions()
 
         var context = PermissionHandlerHelper.CreateTestAuthorizationHandlerContext(required, [adminRolePermission], true);
 
-        var permissionHandler = new RolesPermissionHandler(new RoleTrackerTest());
+        var permissionHandler = GetRolesPermissionHandler();
 
         // Act
         await permissionHandler.HandleAsync(context);
 
         // Assert
         Assert.False(context.HasSucceeded);
     }
+
+    public static RolesPermissionHandler GetRolesPermissionHandler(params string[] roles)
+    {
+        var options = new Mock<IOptions<IdentityOptions>>();
+
+        options.Setup(x => x.Value).Returns(new IdentityOptions());
+
+        var permissionHandler = new RolesPermissionHandler(new RoleTrackerTest(roles), options.Object);
+
+        return permissionHandler;
+    }
 }