Closed DamianEdwards closed 2 years ago
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
public static TBuilder RequireAuthorization<TBuilder>(this TBuilder builder, IAuthorizeData authorizeData) where TBuilder : IEndpointConventionBuilder;
We have this one already https://github.com/dotnet/aspnetcore/blob/d4e70bd66865077c9af13067ec6cb84819e68977/src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs#L60
@BrennanConroy to make the attribute work with SignalR, we need to pass the hub method metadata to the auth system so that it can invoke these attributes.
@DamianEdwards I'm not yet sure if we need to make changes to the authZ APIs to allow passing the endpoint metadata as requirements or if the caller needs to do that manually.
Should we add an example of what a more complicated Permissions system would look like to see how much better this is now that the handler has access to the Attribute data? i.e. Policy = "Read" / "Write" / "Delete" can become new RequirePermission(Permissions.Read | Write | Delete)
right?
Yes! @HaoK, exactly. So something like this:
public class RequirePermissionAttribute : Attribute, IAuthorizationHandler, IAuthorizationRequirement
{
public RequirePermission(Permissions permissions)
{
Username = permissions;
}
public Permissions Permissions { get; set; }
public Task HandleAsync(AuthorizationHandlerContext context)
{
var userPerms = GetPermissionsForUser(context.User);
if (userPerms.HasAll(Permissions))
{
context.Succeed(this);
}
return Task.CompletedTask;
}
}
So the one caveat is that the current limitation with the instance based Handler/Requirements, is you lose the ability to inject as easily, which is something worth mentioning up front, so maybe its worth updating the permission example to demonstrate how they'd get the DbContext from the request and pass it through to the GetPermissionsForUser call to make it more 'real'. Worse comes to worse they can probably always just service locate off the request in the handler context
Here's what I was thinking because this issue doesn't quite do it justice:
To summarize what feature we're adding here so it's clear:
This extends how and where you get to define authorization requirements. Today the only way to use this list of requirement is via a policy name. The policy name maps to a list of requirements and a list of authentication schemes. So there's this indirection that stops you from defining data on your resource.
var builder = WebApplication.CreateBuilder();
builder.Services.AddAuthorization(options => options.AddPolicy("Policy", pb => pb.RequireClaim("myclaim")));
app.MapGet("/authed", [Authorize("Policy")] () => { });
app.MapGet("/authed2", () => { }).RequireAuthorization("Policy");
public class MyController : ControllerBase
{
[HttpGet("/authed3")]
[Authorize("Policy")]
public string Get() => "Hello";
}
public class MyHub : Hub
{
[Authorize("Policy")]
public Task Send(string s) => Clients.All.SendAsync("Send", "Hello World");
}
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.SignalR;
var builder = WebApplication.CreateBuilder();
builder.Services.AddAuthorization();
builder.Services.AddSingleton<IAuthorizationHandler, RequiredClaimsAuthorizationHandler>();
var app = builder.Build();
app.UseAuthentication();
app.UseAuthorization();
app.MapGet("/authed", [RequireClaims("myclaim")] () => { });
app.MapGet("/authed2", () => { }).WithMetadata(new RequireClaims("myclaim"));
public class MyController : ControllerBase
{
[HttpGet("/authed3")]
[RequireClaims("myclaim")]
public string Get() => "Hello";
}
public class MyHub : Hub
{
[RequireClaims("myclaim")]
public Task Send(string s) => Clients.All.SendAsync("Send", "Hello World");
}
class RequireClaims : Attribute, IAuthorizationRequirement
{
public RequireClaims(string claimType, params string[] allowedValues)
{
ClaimType = claimType;
AllowedValues = allowedValues;
}
public string ClaimType { get; set; }
public string[] AllowedValues { get; set; }
}
class RequiredClaimsAuthorizationHandler : AuthorizationHandler<RequireClaims>
{
protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, RequireClaims requirement)
{
if (context.User != null)
{
var found = false;
if (requirement.AllowedValues == null || !requirement.AllowedValues.Any())
{
found = context.User.Claims.Any(c => string.Equals(c.Type, requirement.ClaimType, StringComparison.OrdinalIgnoreCase));
}
else
{
found = context.User.Claims.Any(c => string.Equals(c.Type, requirement.ClaimType, StringComparison.OrdinalIgnoreCase)
&& requirement.AllowedValues.Contains(c.Value, StringComparer.Ordinal));
}
if (found)
{
context.Succeed(requirement);
}
}
return Task.CompletedTask;
}
}
This lets you define the requirement directly on the resource and the job of the framework is to pass this to the authZ system. This lets handlers do their thing and work as they do today. The attribute here is using the trick where the requirement can also be a handler but it doesn't require that.
Note you still need to call builder.Services.AddAuthorization()
and app.UseAuthorization()
right now (in your "After" sample) but we're going to look at ways to potentially improve that separately.
Thanks for contacting us.
We're moving this issue to the .NET 7 Planning
milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.
@BrennanConroy I added you for the SignalR work (though once we decide what do to anyone could do it).
API review:
We're happy to approve these two APIs:
// Set authorization metadata via an instance of AuthorizationPolicy
public static TBuilder RequireAuthorization<TBuilder>(this TBuilder builder, AuthorizationPolicy policy) where TBuilder : IEndpointConventionBuilder;
// Set authorization metadata via a callback accepting Action<AuthorizationPolicyBuilder>
public static TBuilder RequireAuthorization<TBuilder>(this TBuilder builder, Action<AuthorizationPolicyBuilder> configurePolicy) where TBuilder : IEndpointConventionBuilder;
The next two overloads look like they need further discussion outside of the scope of API review and will be reviewed later.
Just checking to see what the next steps for this issue are, I have some cycles now to help, is getting https://github.com/dotnet/aspnetcore/pull/39892 in the first step?
@HaoK I'm actually not entirely sure what's next here, but it seems like that PR is probably it?
@DamianEdwards do you have everything you need now from the AuthZ layer for https://github.com/DamianEdwards/AspNetCoreDevJwts
You should have enough to do app.MapGet(...).RequireAuthorization(p => p.RequireClaim("scope", "myapi:protected-access"));
how important is the last big missing piece, to be able to directly flow the requirements as metadata?
Basically, the app.MapGet("/authed", [RequireClaims("myclaim")] () => { });
doesn't currently work, although this is also what prevents signalR from being able to take advantage of this too, so maybe its important to figure out the details now
We need to talk about adding requirements via metadata for things like mvc and signalr
Sure the sticking point I believe in your PR is what do we do by default in combining that with the default policy/etc right? How do we make it easy to compose / override the empty [Authorize]
things
Right the problem was, what happens when you only have a requirement and but you have a requirement.
@HaoK I successfully tested this out in my experiments repo: https://github.com/DamianEdwards/AspNetCoreDevJwts/blob/preview4/SampleWebApi/Program.cs#L33
I also added two more extension methods that build on top of this: RequireRole(params string[] roles)
and RequireScope(params string[] scopes)
that I'd be interested in discussing the utility of adding to the framework.
RequireRole seems pretty useful, but RequireScope seems very specific to jwt, is it that much worse if you had to do: .RequireClaim("scope", "protected:read");
instead? Alternatively we could just have that helper live in whatever jwt helper package we have similar to what you are doing, just not sure any Scope helpers make much sense in the base *.Authorization framework code
what happens when you only have a requirement and but you have a requirement.
I'm going to guess that you mean when you only have a requirement but you also have a default policy?
I'm going to guess that you mean when you only have a requirement but you also have a default policy?
You forget to setup the auth middleware (both N and Z) and you add a requirement to your endpoint. Today we have this check https://github.com/dotnet/aspnetcore/blob/949043857d00dd8ddc521aea977b37fb48ccf36e/src/Http/Routing/src/EndpointMiddleware.cs#L38, we'd need to add one for auth requirements too? Feels a little messy.
Actually hold on, did we miss this for auth policies too? What happens if you attach a policy and don't have anything setup in the pipeline?
Hrm, yeah I think its just metadata that's ignored
Can we just be smart and auto inject the Authorization middleware if we detect any endpoints with authorization metadata? Where metadata would be any IAuthorizeData/requirements/Policy. Or are we still concerned with ordering?
I think we just need to throw for this metadata. @DamianEdwards is looking at auto injecting middlware in the web application builder cases if you configure auth.
I'm using this to track the authz caching work discussed to speed up the hot path so endpoints don't combine authz policies every time
Cache work done in https://github.com/dotnet/aspnetcore/pull/43124
Relates to #34545
We should allow the definition and/or application of authorization policies to specific endpoints via endpoint metadata at the time they're declared. This will make configuration of resource authorization for Minimal API style applications much simpler and more inline with the principals of Minimal APIs while still enabling re-use of policies via language features rather than relying on their definition at the time authorization is added to DI.
The
AuthorizationMiddleware
would be updated to retrieve metadata for the current request and ensure any instances that implementIAuthorizationRequirement
are passed to theIAuthorizationService
for evaluation (e.g. asIAuthorizationHandler
orIAuthorizeData
, etc.).New extension methods would be added to enable setting
AuthorizationPolicy
on endpoint definitions, as well as methods for settingIAuthorizationRequirement
,IAuthorizationHandler
orIAuthorizeData
as metadata:Example usage: