diff --git a/src/OrchardCore.Modules/OrchardCore.Roles/Recipes/RolesStep.cs b/src/OrchardCore.Modules/OrchardCore.Roles/Recipes/RolesStep.cs index 7b806d26583..26775056502 100644 --- a/src/OrchardCore.Modules/OrchardCore.Roles/Recipes/RolesStep.cs +++ b/src/OrchardCore.Modules/OrchardCore.Roles/Recipes/RolesStep.cs @@ -4,7 +4,6 @@ using OrchardCore.Recipes.Services; using OrchardCore.Security; using OrchardCore.Security.Permissions; -using OrchardCore.Security.Services; namespace OrchardCore.Roles.Recipes; @@ -14,15 +13,15 @@ namespace OrchardCore.Roles.Recipes; public sealed class RolesStep : NamedRecipeStepHandler { private readonly RoleManager _roleManager; - private readonly IRoleService _roleService; + private readonly ISystemRoleNameProvider _systemRoleNameProvider; public RolesStep( RoleManager roleManager, - IRoleService roleService) + ISystemRoleNameProvider systemRoleNameProvider) : base("Roles") { _roleManager = roleManager; - _roleService = roleService; + _systemRoleNameProvider = systemRoleNameProvider; } protected override async Task HandleAsync(RecipeExecutionContext context) @@ -54,7 +53,7 @@ protected override async Task HandleAsync(RecipeExecutionContext context) r.RoleDescription = roleEntry.Description; r.RoleClaims.RemoveAll(c => c.ClaimType == Permission.ClaimType); - if (!await _roleService.IsAdminRoleAsync(roleName)) + if (!await _systemRoleNameProvider.IsAdminRoleAsync(roleName)) { r.RoleClaims.AddRange(roleEntry.Permissions.Select(RoleClaim.Create)); } diff --git a/src/OrchardCore.Modules/OrchardCore.Roles/Services/RoleClaimsProvider.cs b/src/OrchardCore.Modules/OrchardCore.Roles/Services/RoleClaimsProvider.cs index af2599a527a..d22c754b1c9 100644 --- a/src/OrchardCore.Modules/OrchardCore.Roles/Services/RoleClaimsProvider.cs +++ b/src/OrchardCore.Modules/OrchardCore.Roles/Services/RoleClaimsProvider.cs @@ -2,7 +2,6 @@ using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Options; using OrchardCore.Security; -using OrchardCore.Security.Services; using OrchardCore.Users; using OrchardCore.Users.Services; @@ -12,18 +11,18 @@ public class RoleClaimsProvider : IUserClaimsProvider { private readonly UserManager _userManager; private readonly RoleManager _roleManager; - private readonly IRoleService _roleService; + private readonly ISystemRoleNameProvider _systemRoleNameProvider; private readonly IdentityOptions _identityOptions; public RoleClaimsProvider( UserManager userManager, RoleManager roleManager, - IRoleService roleService, + ISystemRoleNameProvider systemRoleNameProvider, IOptions identityOptions) { _userManager = userManager; _roleManager = roleManager; - _roleService = roleService; + _systemRoleNameProvider = systemRoleNameProvider; _identityOptions = identityOptions.Value; } @@ -34,15 +33,22 @@ public async Task GenerateAsync(IUser user, ClaimsIdentity claims) return; } + var isAdministrator = false; + + if (await _userManager.IsInRoleAsync(user, await _systemRoleNameProvider.GetAdminRoleAsync())) + { + claims.AddClaim(StandardClaims.SiteOwner); + + isAdministrator = true; + } + var roleNames = await _userManager.GetRolesAsync(user); - var roles = new List(); - var addClaims = true; foreach (var roleName in roleNames) { claims.AddClaim(new Claim(_identityOptions.ClaimsIdentity.RoleClaimType, roleName)); - if (!_roleManager.SupportsRoleClaims) + if (isAdministrator || !_roleManager.SupportsRoleClaims) { continue; } @@ -54,21 +60,6 @@ public async Task GenerateAsync(IUser user, ClaimsIdentity claims) continue; } - if (addClaims && await _roleService.IsAdminRoleAsync(role.RoleName)) - { - addClaims = false; - } - - roles.Add(role); - } - - if (roles.Count == 0 || !addClaims) - { - return; - } - - foreach (var role in roles) - { claims.AddClaims(await _roleManager.GetClaimsAsync(role)); } } diff --git a/src/OrchardCore.Modules/OrchardCore.Roles/Startup.cs b/src/OrchardCore.Modules/OrchardCore.Roles/Startup.cs index 4886ee4412a..6adff2c006c 100644 --- a/src/OrchardCore.Modules/OrchardCore.Roles/Startup.cs +++ b/src/OrchardCore.Modules/OrchardCore.Roles/Startup.cs @@ -10,7 +10,6 @@ using OrchardCore.Modules; using OrchardCore.Navigation; using OrchardCore.Recipes; -using OrchardCore.Roles.Core; using OrchardCore.Roles.Deployment; using OrchardCore.Roles.Migrations; using OrchardCore.Roles.Recipes; @@ -35,7 +34,6 @@ public override void ConfigureServices(IServiceCollection services) { services.AddScoped(); services.AddDataMigration(); - services.AddScoped(); services.AddScoped(); services.Replace(ServiceDescriptor.Scoped>(sp => sp.GetRequiredService())); services.Replace(ServiceDescriptor.Scoped>(sp => sp.GetRequiredService())); diff --git a/src/OrchardCore.Modules/OrchardCore.Settings/Services/SuperUserHandler.cs b/src/OrchardCore.Modules/OrchardCore.Settings/Services/SuperUserHandler.cs index dc77d78ca22..75b75bc03da 100644 --- a/src/OrchardCore.Modules/OrchardCore.Settings/Services/SuperUserHandler.cs +++ b/src/OrchardCore.Modules/OrchardCore.Settings/Services/SuperUserHandler.cs @@ -18,7 +18,21 @@ public SuperUserHandler(ISiteService siteService) public async Task HandleAsync(AuthorizationHandlerContext context) { - var userId = context?.User?.FindFirstValue(ClaimTypes.NameIdentifier); + var user = context?.User; + + if (user == null) + { + return; + } + + if (user.HasClaim(StandardClaims.SiteOwner.Type, StandardClaims.SiteOwner.Value)) + { + SucceedAllRequirements(context); + + return; + } + + var userId = user.FindFirstValue(ClaimTypes.NameIdentifier); if (userId == null) { return; diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Liquid/HasPermissionFilter.cs b/src/OrchardCore.Modules/OrchardCore.Users/Liquid/HasPermissionFilter.cs index 7b7d44ced88..4cd628e5f92 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Liquid/HasPermissionFilter.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/Liquid/HasPermissionFilter.cs @@ -23,13 +23,32 @@ public async ValueTask ProcessAsync(FluidValue input, FilterArgument if (input.ToObjectValue() is LiquidUserAccessor) { var user = _httpContextAccessor.HttpContext?.User; - if (user != null) + if (user != null && arguments.Count > 0) { var permissionName = arguments["permission"].Or(arguments.At(0)).ToStringValue(); - var resource = arguments["resource"].Or(arguments.At(1)).ToObjectValue(); - if (!string.IsNullOrEmpty(permissionName) && - await _authorizationService.AuthorizeAsync(user, new Permission(permissionName), resource)) + if (string.IsNullOrWhiteSpace(permissionName)) + { + return BooleanValue.False; + } + + var permission = new Permission(permissionName); + + if (arguments.Count > 1) + { + var resource = arguments["resource"].Or(arguments.At(1)).ToObjectValue(); + + if (resource != null) + { + if (!string.IsNullOrEmpty(permissionName) && + await _authorizationService.AuthorizeAsync(user, permission, resource)) + { + return BooleanValue.True; + } + } + } + + if (await _authorizationService.AuthorizeAsync(user, permission)) { return BooleanValue.True; } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Liquid/UserFilters.cs b/src/OrchardCore.Modules/OrchardCore.Users/Liquid/UserFilters.cs index 6113c8b1854..1e1d36564e9 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Liquid/UserFilters.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/Liquid/UserFilters.cs @@ -2,18 +2,16 @@ using Fluid; using Fluid.Values; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Options; using OrchardCore.Liquid; -using OrchardCore.Roles; +using OrchardCore.Security; using OrchardCore.Security.Permissions; namespace OrchardCore.Users.Liquid; public static class UserFilters { - public static async ValueTask HasClaim(FluidValue input, FilterArguments arguments, TemplateContext ctx) + public static ValueTask HasClaim(FluidValue input, FilterArguments arguments, TemplateContext ctx) { if (input.ToObjectValue() is LiquidUserAccessor) { @@ -31,21 +29,24 @@ public static async ValueTask HasClaim(FluidValue input, FilterArgum return BooleanValue.True; } - if (string.Equals(claimType, Permission.ClaimType, StringComparison.OrdinalIgnoreCase)) + // The following if condition was added in 2.1 for backward compatibility. It should be removed in v3 and documented as a breaking change. + // The change log should state the following: + // The `Administrator` role no longer registers permission-based claims by default during login. This means that directly checking for specific claims in Liquid, such as: + // + // ```liquid + // {% assign isAuthorized = User | has_claim: "Permission", "AccessAdminPanel" %} + // ``` + // + // will return `false` for administrators, even though they still have full access. Non-admin users, however, may return `true` if they have the claim. + // it's important to use the `has_permission` filter for permission checks going forward: + // + // ```liquid + // {% assign isAuthorized = User | has_permission: "AccessAdminPanel" %} + // ``` + if (string.Equals(claimType, Permission.ClaimType, StringComparison.OrdinalIgnoreCase) && + user.HasClaim(StandardClaims.SiteOwner.Type, StandardClaims.SiteOwner.Value)) { - var systemRoleNameProvider = context.Services.GetService(); - - if (systemRoleNameProvider != null) - { - // Administrator users do not register individual permissions during login. - // However, they are designed to automatically have all application permissions granted. - var identityOptions = context.Services.GetRequiredService>().Value; - - if (user.HasClaim(identityOptions.ClaimsIdentity.RoleClaimType, await systemRoleNameProvider.GetAdminRoleAsync())) - { - return BooleanValue.True; - } - } + return BooleanValue.True; } } } diff --git a/src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Security/StandardClaims.cs b/src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Security/StandardClaims.cs new file mode 100644 index 00000000000..4f87abf054b --- /dev/null +++ b/src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Security/StandardClaims.cs @@ -0,0 +1,11 @@ +using System.Security.Claims; + +namespace OrchardCore.Security; + +public static class StandardClaims +{ + /// + /// This claim is assigned by the system during the login process if the user belongs to the Administrator role. + /// + public static readonly Claim SiteOwner = new("SiteOwner", "true"); +} diff --git a/src/OrchardCore/OrchardCore.Roles.Core/RolesPermissionHandler.cs b/src/OrchardCore/OrchardCore.Roles.Core/RolesPermissionHandler.cs deleted file mode 100644 index 3157153b85b..00000000000 --- a/src/OrchardCore/OrchardCore.Roles.Core/RolesPermissionHandler.cs +++ /dev/null @@ -1,44 +0,0 @@ -using System.Security.Claims; -using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Identity; -using Microsoft.Extensions.Options; -using OrchardCore.Security; -using OrchardCore.Security.Services; - -namespace OrchardCore.Roles.Core; - -public class RolesPermissionHandler : AuthorizationHandler -{ - private readonly IRoleService _roleService; - private readonly IdentityOptions _identityOptions; - - public RolesPermissionHandler( - IRoleService roleService, - IOptions identityOptions) - { - _roleService = roleService; - _identityOptions = identityOptions.Value; - } - - protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, PermissionRequirement requirement) - { - if (context.HasSucceeded || context?.User?.Identity?.IsAuthenticated == false) - { - return; - } - - foreach (var claim in context.User.FindAll(IsRoleClaim)) - { - if (await _roleService.IsAdminRoleAsync(claim.Value)) - { - context.Succeed(requirement); - - return; - } - } - } - - private bool IsRoleClaim(Claim claim) - => claim.Type == _identityOptions.ClaimsIdentity.RoleClaimType || - (claim.Type is "role" or ClaimTypes.Role); -} diff --git a/src/docs/reference/modules/Liquid/README.md b/src/docs/reference/modules/Liquid/README.md index 97ffba484f0..d40ac1ee2a3 100644 --- a/src/docs/reference/modules/Liquid/README.md +++ b/src/docs/reference/modules/Liquid/README.md @@ -410,7 +410,6 @@ You can use this filter to load the user information of the current authenticate {% assign user = User | user_id | users_by_id %} {{ user.UserName }} - {{ user.Email }} - ``` You can use this filter with the UserPicker field to load the picked user's information. @@ -421,7 +420,6 @@ You can use this filter with the UserPicker field to load the picked user's info {% for user in users %} {{ user.UserName }} - {{ user.Email }} {% endfor %} - ``` #### User has_permission filter @@ -429,7 +427,7 @@ You can use this filter with the UserPicker field to load the picked user's info Checks if the User has permission clearance, optionally on a resource ```liquid -{{ User | has_permission:"EditContent",Model.ContentItem }} +{{ User | has_permission: "EditContent", Model.ContentItem }} ``` #### User is_in_role filter @@ -437,7 +435,7 @@ Checks if the User has permission clearance, optionally on a resource Checks if the user is in role ```liquid -{{ User | is_in_role:"Administrator" }} +{{ User | is_in_role: "Administrator" }} ``` #### User has_claim filter @@ -445,10 +443,12 @@ Checks if the user is in role Checks if the user has a claim of the specified type ```liquid -{{ User | has_claim:"email_verified","true" }} -{{ User | has_claim:"Permission","ManageSettings" }} +{{ User | has_claim: "email_verified", "true" }} ``` +!!! warning + To avoid false negatives for Administrator users, ensure you use the `has_permission` filter instead of `has_claim` when checking if a user has a permission. This ensures accurate permission evaluation, especially for administrators who may not have explicit claims but still possess full access rights. + ### Site Gives access to the current site settings, e.g `Site.SiteName`. diff --git a/src/docs/releases/2.1.0.md b/src/docs/releases/2.1.0.md index 1cc10680dc7..b02da7e6467 100644 --- a/src/docs/releases/2.1.0.md +++ b/src/docs/releases/2.1.0.md @@ -80,13 +80,15 @@ In the Roles feature, there were previously `AssignRoles` and `AssignRole_{RoleN !!! warning Please review all your recipes and replace occurrences of `AssignRoles` with `AssignRoleToUsers`, and `AssignRole_{RoleName}` with `AssignRoleToUsers_{RoleName}`. -#### Site Owner Permission Deprecated, Administrator Role Retained as System Role +### Site Owner Permission Deprecated, Administrator Role Retained as a System Role -The `SiteOwner` permission has been deprecated and will be removed in future releases. Instead, the `Administrator` role has been preserved as a system role, similar to `Authenticated` and `Anonymous`. This change was made to improve performance and grant all permissions at the role level, rather than relying on a single super-permission. A fast-track migration automatically assigns the `Administrator` role to users who previously held the `SiteOwner` permission. +The `SiteOwner` permission has been deprecated and is scheduled for removal in future releases. To streamline performance, the `Administrator` role has been retained as a system role, much like the `Authenticated` and `Anonymous` roles. This update enhances efficiency by assigning permissions at the role level, eliminating reliance on a single super-permission. Users previously granted the `SiteOwner` permission will automatically receive the `Administrator` role through a fast-track migration process. -The Recipes feature now checks permissions against the new **Manage Recipes** permission, replacing the deprecated `SiteOwner` permission. +Additionally, the **Manage Recipes** permission has been introduced, replacing the deprecated `SiteOwner` permission for the Recipes feature. -Lastly, if you'd like to change the admin's role name `Administrator` globally, you can do so through any settings provider. For example, to rename it to `Admin` using `appsettings.json`, add the following configuration: +#### Customizing the Administrator Role Name + +If you wish to globally change the default role name of `Administrator`, this can be easily configured using any settings provider. For instance, to rename it to `Admin` in `appsettings.json`, add the following configuration: ```json "OrchardCore_Roles": { @@ -94,8 +96,26 @@ Lastly, if you'd like to change the admin's role name `Administrator` globally, } ``` -!!! warning - If the existing `Administrator` role did not previously include the `SiteOwner` permission, a new system admin role will be generated. This role may be named `SiteAdmin` or `SiteAdmin{N}`, where `{N}` ensures uniqueness. Users who were assigned the `SiteOwner` permission will automatically be granted this newly created role. +##### New System Admin Role Creation + +If the `Administrator` role did not previously include the `SiteOwner` permission, a new system admin role will be generated. This role may be named `SiteAdmin` or `SiteAdmin{N}`, where `{N}` ensures uniqueness. Users previously assigned the `SiteOwner` permission will automatically be assigned this new role. + +##### Important Change in Admin Permission Handling + +The `Administrator` role no longer registers permission-based claims by default during login. This means that directly checking for specific claims in Liquid, such as: + +```liquid +{% assign isAuthorized = User | has_claim: "Permission", "AccessAdminPanel" %} +``` + +will return `false` for administrators, even though they still have full access. Non-admin users, however, may return `true` if they have the claim. + +For backward compatibility, the current behavior still allows this code to return `true` for administrators, but this will change in a future major release. To avoid potential issues, it’s recommended to use the `has_permission` filter for permission checks going forward: + +```liquid +{% assign isAuthorized = User | has_permission: "AccessAdminPanel" %} +``` + #### New Methods Added to `IRoleService` diff --git a/test/OrchardCore.Tests/Security/RolesPermissionHandlerTests.cs b/test/OrchardCore.Tests/Security/RolesPermissionHandlerTests.cs deleted file mode 100644 index 0b5c38990d5..00000000000 --- a/test/OrchardCore.Tests/Security/RolesPermissionHandlerTests.cs +++ /dev/null @@ -1,60 +0,0 @@ -using OrchardCore.Roles.Core; -using OrchardCore.Security.Permissions; -using OrchardCore.Security.Services; - -namespace OrchardCore.Tests.Security; - -public class RolesPermissionHandlerTests -{ - [Fact] - public async Task HandleAsync_WhenCalled_AdminsShouldBeGrantedPermissions() - { - // Arrange - var adminRolePermission = new Claim("role", "Administrator"); - var required = new Permission("Required", "Foo"); - - var context = PermissionHandlerHelper.CreateTestAuthorizationHandlerContext(required, [adminRolePermission], true); - - var permissionHandler = GetRolesPermissionHandler(true); - - // Act - await permissionHandler.HandleAsync(context); - - // Assert - Assert.True(context.HasSucceeded); - } - - [Fact] - public async Task HandleAsync_WhenCalled_NonAdminsShouldNotBeGrantedPermissions() - { - // Arrange - var adminRolePermission = new Claim("role", "Editor"); - var required = new Permission("Required", "Foo"); - - var context = PermissionHandlerHelper.CreateTestAuthorizationHandlerContext(required, [adminRolePermission], true); - - var permissionHandler = GetRolesPermissionHandler(false); - - // Act - await permissionHandler.HandleAsync(context); - - // Assert - Assert.False(context.HasSucceeded); - } - - private static RolesPermissionHandler GetRolesPermissionHandler(bool userIsAdmin) - { - var options = new Mock>(); - - options.Setup(x => x.Value).Returns(new IdentityOptions()); - - var roleService = new Mock(); - - roleService.Setup(x => x.IsAdminRoleAsync(It.IsAny())) - .ReturnsAsync(userIsAdmin); - - var permissionHandler = new RolesPermissionHandler(roleService.Object, options.Object); - - return permissionHandler; - } -}