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

Cleanup UserFilters #17081

Merged
merged 4 commits into from
Nov 27, 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
27 changes: 0 additions & 27 deletions src/OrchardCore.Modules/OrchardCore.Users/Liquid/UserFilters.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@
using Fluid.Values;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using OrchardCore.Liquid;
using OrchardCore.Security;
using OrchardCore.Security.Permissions;

namespace OrchardCore.Users.Liquid;

Expand All @@ -29,30 +26,6 @@ public static ValueTask<FluidValue> HasClaim(FluidValue input, FilterArguments a
{
return BooleanValue.True;
}

// 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 logger = context.Services.GetRequiredService<ILogger<Startup>>();

logger.LogWarning("The tenant is using the 'has_claim' Liquid filter for Permission claims '{ClaimName}', which will break in the next major release of OrchardCore; please use 'has_permission: \"{ClaimName}\"' instead.", claimName, claimName);

return BooleanValue.True;
}
}
}

Expand Down
14 changes: 14 additions & 0 deletions src/docs/releases/3.0.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,20 @@ The `ExternalLogin` action has been removed from the `Account` controller. If yo

The `AssignRoleToUsers` permission is no longer implicitly granted by `EditUsers`. To maintain the same behavior, make sure to explicitly assign the `AssignRoleToUsers` permission to any role that already has the `EditUsers` permission.

#### The Behavior of 'has_claim' Liquid Filter Changed.

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" %}
```

### Sealing Types

Many type commonly used by classes can be `sealed`, which improves runtime performance. While it's not mandatory, we recommend that you consider applying this improvement to your own extensions as well. We've implemented this enhancement in pull request [#16897](https://github.com/OrchardCMS/OrchardCore/pull/16897).
Expand Down
Loading