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.

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

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

Merged
merged 9 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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(StandardClaims.SiteOwner);

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
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
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(StandardClaims.SiteOwner.Type, StandardClaims.SiteOwner.Value))
{
SucceedAllRequirements(context);

return;
}

var userId = user.FindFirstValue(ClaimTypes.NameIdentifier);
if (userId == null)
{
return;
Expand Down
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
37 changes: 19 additions & 18 deletions src/OrchardCore.Modules/OrchardCore.Users/Liquid/UserFilters.cs
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,24 @@ public static async ValueTask<FluidValue> 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) &&
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

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(StandardClaims.SiteOwner.Type, StandardClaims.SiteOwner.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
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
using System.Security.Claims;

namespace OrchardCore.Security;

public static class StandardClaims
{
/// <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 SiteOwner = new("SiteOwner", "true");
}
44 changes: 0 additions & 44 deletions src/OrchardCore/OrchardCore.Roles.Core/RolesPermissionHandler.cs

This file was deleted.

12 changes: 6 additions & 6 deletions src/docs/reference/modules/Liquid/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -421,34 +420,35 @@ 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

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

Checks if the user is in role

```liquid
{{ User | is_in_role:"Administrator" }}
{{ User | is_in_role: "Administrator" }}
```

#### User has_claim filter

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`.
Expand Down
Loading