Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify how to check if the user is an admin #16866

Merged
merged 9 commits into from
Oct 11, 2024
Merged

Simplify how to check if the user is an admin #16866

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
40cecf7
Simplify how to check if the user is an admin
MikeAlhayek Oct 10, 2024
0ba53f8
Merge branch 'main' into ma/simplify-admin-check
MikeAlhayek Oct 10, 2024
499e140
cleanup
MikeAlhayek Oct 10, 2024
0a6076c
fix build
MikeAlhayek Oct 10, 2024
62ff192
update HasPermissionFilter
MikeAlhayek Oct 10, 2024
9b7eab0
improve docus
MikeAlhayek Oct 11, 2024
8a975a5
Merge branch 'main' into ma/simplify-admin-check
MikeAlhayek Oct 11, 2024
0bba0c9
cleanup
MikeAlhayek Oct 11, 2024
4df0e71
Merge branch 'ma/simplify-admin-check' of https://github.com/OrchardC…
MikeAlhayek Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/OrchardCore.Modules/OrchardCore.Roles/Recipes/RolesStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using OrchardCore.Recipes.Services;
using OrchardCore.Security;
using OrchardCore.Security.Permissions;
using OrchardCore.Security.Services;

namespace OrchardCore.Roles.Recipes;

Expand All @@ -14,15 +13,15 @@ namespace OrchardCore.Roles.Recipes;
public sealed class RolesStep : NamedRecipeStepHandler
{
private readonly RoleManager<IRole> _roleManager;
private readonly IRoleService _roleService;
private readonly ISystemRoleNameProvider _systemRoleNameProvider;

public RolesStep(
RoleManager<IRole> roleManager,
IRoleService roleService)
ISystemRoleNameProvider systemRoleNameProvider)
: base("Roles")
{
_roleManager = roleManager;
_roleService = roleService;
_systemRoleNameProvider = systemRoleNameProvider;
}

protected override async Task HandleAsync(RecipeExecutionContext context)
Expand Down Expand Up @@ -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));
}
Expand Down
35 changes: 13 additions & 22 deletions src/OrchardCore.Modules/OrchardCore.Roles/Services/RoleClaimsProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -12,18 +11,18 @@ public class RoleClaimsProvider : IUserClaimsProvider
{
private readonly UserManager<IUser> _userManager;
private readonly RoleManager<IRole> _roleManager;
private readonly IRoleService _roleService;
private readonly ISystemRoleNameProvider _systemRoleNameProvider;
private readonly IdentityOptions _identityOptions;

public RoleClaimsProvider(
UserManager<IUser> userManager,
RoleManager<IRole> roleManager,
IRoleService roleService,
ISystemRoleNameProvider systemRoleNameProvider,
IOptions<IdentityOptions> identityOptions)
{
_userManager = userManager;
_roleManager = roleManager;
_roleService = roleService;
_systemRoleNameProvider = systemRoleNameProvider;
_identityOptions = identityOptions.Value;
}

Expand All @@ -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(StandardPermissions.SiteOwnerClaim);

isAdministrator = true;
}

var roleNames = await _userManager.GetRolesAsync(user);
var roles = new List<IRole>();
var addClaims = true;

foreach (var roleName in roleNames)
{
claims.AddClaim(new Claim(_identityOptions.ClaimsIdentity.RoleClaimType, roleName));

if (!_roleManager.SupportsRoleClaims)
if (isAdministrator || !_roleManager.SupportsRoleClaims)
{
continue;
}
Expand All @@ -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));
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/OrchardCore.Modules/OrchardCore.Roles/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -35,7 +34,6 @@ public override void ConfigureServices(IServiceCollection services)
{
services.AddScoped<IUserClaimsProvider, RoleClaimsProvider>();
services.AddDataMigration<RolesMigrations>();
services.AddScoped<IAuthorizationHandler, RolesPermissionHandler>();
services.AddScoped<RoleStore>();
services.Replace(ServiceDescriptor.Scoped<IRoleClaimStore<IRole>>(sp => sp.GetRequiredService<RoleStore>()));
services.Replace(ServiceDescriptor.Scoped<IRoleStore<IRole>>(sp => sp.GetRequiredService<RoleStore>()));
Expand Down
16 changes: 15 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Settings/Services/SuperUserHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(StandardPermissions.SiteOwnerClaim.Type, StandardPermissions.SiteOwnerClaim.Value))
{
SucceedAllRequirements(context);

return;
}

var userId = user.FindFirstValue(ClaimTypes.NameIdentifier);
if (userId == null)
{
return;
Expand Down
27 changes: 23 additions & 4 deletions src/OrchardCore.Modules/OrchardCore.Users/Liquid/HasPermissionFilter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,32 @@ public async ValueTask<FluidValue> 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;
}
Expand Down
23 changes: 5 additions & 18 deletions src/OrchardCore.Modules/OrchardCore.Users/Liquid/UserFilters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<FluidValue> HasClaim(FluidValue input, FilterArguments arguments, TemplateContext ctx)
public static ValueTask<FluidValue> HasClaim(FluidValue input, FilterArguments arguments, TemplateContext ctx)
{
if (input.ToObjectValue() is LiquidUserAccessor)
{
Expand All @@ -31,21 +29,10 @@ public static async ValueTask<FluidValue> HasClaim(FluidValue input, FilterArgum
return BooleanValue.True;
}

if (string.Equals(claimType, Permission.ClaimType, StringComparison.OrdinalIgnoreCase))
if (string.Equals(claimType, Permission.ClaimType, StringComparison.OrdinalIgnoreCase) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this would be removed if we have a custom filter for authorization, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we would. Because we should still return true if the user is an admin. has_authorization would use the IAutheorizationService to do similar check without evaluating the claims explicitly.

If we need to remove it, we'll have to document it as a breaking change for 3.0 and offer has_authorization as a better way to authorize user

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we should still return true if the user is an admin

In 2.x because it would be a breaking change otherwise. So we can remove it in 3.0.

Copy link
Member Author

@MikeAlhayek MikeAlhayek Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are you saying to keep this and at somepoint add has_authorization. then in 3.0, we can remove this check from has_claim, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastienros actually we already have has_permission filter that one can already use. So I don't think there is a need to add has_authorization

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think this should be removed e.g. in 3.0, but lets keep it for backward compatibility until then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note so we can remove it in 3.0

user.HasClaim(StandardPermissions.SiteOwnerClaim.Type, StandardPermissions.SiteOwnerClaim.Value))
{
var systemRoleNameProvider = context.Services.GetService<ISystemRoleNameProvider>();

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<IOptions<IdentityOptions>>().Value;

if (user.HasClaim(identityOptions.ClaimsIdentity.RoleClaimType, await systemRoleNameProvider.GetAdminRoleAsync()))
{
return BooleanValue.True;
}
}
return BooleanValue.True;
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Security/StandardPermissions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Security.Claims;
using OrchardCore.Security.Permissions;

namespace OrchardCore.Security;
Expand All @@ -6,4 +7,9 @@ public static class StandardPermissions
{
[Obsolete("This permission is deprecated and will be removed in future releases. Instead, consider adding users to the system administrator role.")]
public static readonly Permission SiteOwner = new("SiteOwner", "Site Owners Permission", isSecurityCritical: true);

/// <summary>
/// This claim is assigned by the system during the login process if the user belongs to the Administrator role.
/// </summary>
public static readonly Claim SiteOwnerClaim = new("OrchardCorePermissions", "all-permissions");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the type name should be more clear and maybe not related to our permissions only. In the future, we may want to use this claim for other checks, beside checking for the existing permissions.
Something like IsSuperUser = true or similar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to Claim SiteOwner = new("SiteOwner", "true"); and moved it to a new StandardClaims class instead

}
44 changes: 0 additions & 44 deletions src/OrchardCore/OrchardCore.Roles.Core/RolesPermissionHandler.cs
View file
Open in desktop

This file was deleted.

60 changes: 0 additions & 60 deletions test/OrchardCore.Tests/Security/RolesPermissionHandlerTests.cs
View file
Open in desktop

This file was deleted.