dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.19k stars 9.93k forks source link

[API Proposal]: `AuthorizationPolicyBuilder.RequireClaim` overload that take a `Predicate<Claim>` #56331

Open joegoldman2 opened 2 months ago

joegoldman2 commented 2 months ago

Background and Motivation

In many applications, claims-based authorization is a critical component for enforcing security and access controls. The current implementation of AuthorizationPolicyBuilder.RequireClaim (and ClaimsAuthorizationRequirement) is limited to checking claims based on a specific type and value. This restriction can force developers to implement custom claim-checking logic outside the provided methods.

Consider the following scenarios:

These scenarios highlight the need for a more flexible claim-checking mechanism.

By adding an overload that accepts a Predicate<Claim>, these kinds of scenarios are easier to implement without additional custom code.

Proposed API

namespace Microsoft.AspNetCore.Authorization;

public partial class AuthorizationPolicyBuilder
{
    // existing
    public AuthorizationPolicyBuilder RequireClaim(string claimType, params string[] allowedValues);
    public AuthorizationPolicyBuilder RequireClaim(string claimType, IEnumerable<string> allowedValues);
    public AuthorizationPolicyBuilder RequireClaim(string claimType);

    // new
+   public AuthorizationPolicyBuilder RequireClaim(Predicate<Claim> match); // alternative name: claimPredicate
}
namespace Microsoft.AspNetCore.Authorization.Infrastructure;

public partial class ClaimsAuthorizationRequirement : AuthorizationHandler<ClaimsAuthorizationRequirement>, IAuthorizationRequirement
{
    // existing
    public ClaimsAuthorizationRequirement(string claimType, IEnumerable<string>? allowedValues)

    // new
+   public ClaimsAuthorizationRequirement(Predicate<Claim> match) // alternative name: claimPredicate

    // ClaimType can now be null if Predicate<Claim> is provided
-   public string ClaimType { get; }
+   public string? ClaimType { get; }

    // can also be a private field but considering that ClaimType and AllowedValues are exposed, maybe this value should also be exposed.
+   public Predicate<Claim>? Match { get; } // alternative name: ClaimPredicate
}

This proposal matches the method ClaimIdentity.HasClaim(Predicate<System.Security.Claims.Claim> match) that already exists in the BCL.

Usage Examples

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddAuthorization(options =>
{
    // Checking if there is any claim that starts with a certain prefix
    options.AddPolicy("prefix", policy => policy.RequireClaim(claim => claim.Value.StartsWith("prefix-"));

    // Checking if there is a claim with type "role" and value containing "admin"
    options.AddPolicy("admin", policy => policy.RequireClaim(claim => claim.Type == "role" && claim.Value.Contains("admin"));
});

Risks

Nothing I can see now.

joegoldman2 commented 2 months ago

@halter73 do you think there's a chance we could include this in the .NET 9 milestone? The implementation is quite simple, the risk is minimal, and a PR is already ready. Thank you.

halter73 commented 2 months ago

I think there's a good chance. Our next API review meeting is this ~Thursday~ Wednesday. I'll see if we can get this approved.

amcasey commented 1 month ago

[API Review]

@joegoldman2 Can you tell us more about the motivation for this change? An example of a concrete use case (not covered by existing APIs) would be very helpful.

We see that there are some examples in your proposal, but why would one need to check for a string prefix on a claim value or check for a role without using a RequireRole?

joegoldman2 commented 1 month ago

@amcasey thank you for your answer!

but why would one need to check for a string prefix on a claim value

In this example:

builder.Services.AddAuthorization(options =>
{
    // Checking if there is any claim that starts with a certain prefix
    options.AddPolicy("prefix", policy => policy.RequireClaim(claim => claim.Value.StartsWith("prefix-"));
});

Should be claim => claim.Type.StartsWith("prefix-"). Sorry for the confusion.

or check for a role without using a RequireRole?

In my company, tokens issued by the authentication provider have a claim role with a value separated by spaces such as reader writer. In this case RequireRole (relying on ClaimsPrincipal.IsInRole) is not working.

Also, we have complex claims with values that are serialized in JSON (https://github.com/DuendeSoftware/IdentityServer/blob/e9860c6488f90e8fbc11a4452b9dd111dbfae933/src/IdentityServer/IdentityServerConstants.cs#L62). In this case, to validate the content of the value, the current API which just does a simple equality check on the value is not sufficient.

amcasey commented 1 month ago

@joegoldman2 I was just the note taker for a group discussion. :wink:

halter73 commented 1 month ago

Thanks for the clarifications. We've decided to hold off on this API for .NET 9 which has now reached code complete for new API. There's the workaround of writing your own AuthorizationHandler/IAuthorizationRequirement for what appears to be an advanced use case.

I imagine JSON serialized claim values are pretty common, but it's not clear how many people are building authorization policies that require parsing JSON values, and whether those people are really better off using a custom handler. The prefix checking also feels pretty contrived regardless of whether it's checking the claim type or value.

However, if we hear enough people request this, or see a lot of people forced to write custom handlers that would have been more easily written using a claims predicate, we'll reconsider. It's a small API surface that's easy to explain which is definitely a plus. One small downside is that it changes ClaimType to be nullable, but I doubt this will break anyone in practice.

joegoldman2 commented 1 month ago

Thank you for your feedback.

The prefix checking also feels pretty contrived regardless of whether it's checking the claim type or value.

It's more for the claim type. Many of our applications are multi-tenant and our authentication provider is issuing claims prefixed by the tenant. I imagine it's not the most common case, but it can happen.

I imagine JSON serialized claim values are pretty common

From what I saw, this particular use case it's quite common indeed. I really hope to see this API back ready to be reviewed for .NET 10.