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

[Epic]: Configuring minimal APIs for auth should be suitably simple & featured #34545

Closed
6 tasks done
DamianEdwards opened this issue Jul 20, 2021 · 20 comments
Closed
6 tasks done
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Epic Groups multiple user stories. Can be grouped under a theme. feature-minimal-actions Controller-like actions for endpoint routing Needs: Design This issue requires design work before implementating.

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Jul 20, 2021

Configuring authentication and authorization requirements for minimal APIs should be suitably inline with the overall experience.

@DamianEdwards DamianEdwards added area-runtime feature-minimal-actions Controller-like actions for endpoint routing labels Jul 20, 2021
@DamianEdwards DamianEdwards added the Needs: Design This issue requires design work before implementating. label Jul 20, 2021
@ghost
Copy link

ghost commented Jul 20, 2021

Thanks for contacting us.

We're moving this issue to the Next sprint 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.

@bradygaster
Copy link
Member

@DamianEdwards @davidfowl @LadyNaggaga do we have a minimum set of authz providers we wish to support? "that which we support now," meaning Microsoft Identity Platform (MIP/AAD), ASP.NET Core identity, and identity server? I'd prioritize them in that order, tbh.

@davidfowl
Copy link
Member

I don't know the scope of this work, this doesn't even work by default (AFAIK) using MVC, so it's low on the priority list IMO.

@DamianEdwards
Copy link
Member Author

Right, we need to figure out what's required here. It wasn't intended to be a "make the thing work automagically" but rather to force us to actually try it (I think @glennc started) and if anything falls out of that that's considered a real showstopper then it would get paged-in.

@LadyNaggaga
Copy link

LadyNaggaga commented Jul 20, 2021

Here is @glennc sample. This is how Glenn got auth to work with minimal APIs. I don't know if this is the experience we want

@DamianEdwards
Copy link
Member Author

Wow OK, so the actual adding auth & CORS to the app isn't bad at all, but configuring Swashbuckle to support the auth scheme is quite a lot of code.

@adityamandaleeka adityamandaleeka added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Aug 27, 2021
@rafikiassumani-msft
Copy link
Contributor

Items to consider:

  • Simple configurations for Oauth2 (JWT), Identity Providers
  • Simple ways to validate claims at the endpoint level. This will include providing simple extension methods or attributes to validate roles, scopes, groups at the endpoint level as opposed to global policy configurations.
  • Should we consider providing a simpler way to issue JWT tokens (First-class API) baked into the framework
  • AuthN and Authorization for grouped endpoints

I have written some raw thoughts in the following gist: https://gist.github.com/rafikiassumani-msft/1c8bfa9b4b05444a5473efe2ecbe7191

@rafikiassumani-msft rafikiassumani-msft added the triage-focus Add this label to flag the issue for focus at triage label Jan 11, 2022
@rafikiassumani-msft rafikiassumani-msft self-assigned this Jan 13, 2022
@captainsafia
Copy link
Member

I had originally categorized this under the OpenAPI work in #34514. Since this topic isn't strictly related to OpenAPI + auth, I'm removing it from that epic.

Note: we have another issue that more specifically tracks OpenAPI + auth.

@DamianEdwards
Copy link
Member Author

@davidfowl and I chatted about this briefly last week and got the start of an idea. Not saying much I know but we'll look into it a bit more and report back. General gist of the idea is to elevate authnz configuration to a builder-level concern and auto-add the required services and middleware, along the lines of:

var builder = WebApplication.CreateBuilder();

// Top-level auth configuration, auto-adds required middleware to pipeline after routing middleware
builder.ConfigureAuthentication(AuthenticationKind.JwtBearer);
// Or perhaps a property?
builder.Authentication.Kind = AuthenticationKind.JwtBearer;
// Maybe overloads of Create/CreateBuilder?
// var builder = WebApplication.CreateBuilder(AuthenticationKind.JwtBearer);
// Maybe simple top-level policy too
builder.Authorization.AddPolicy("todo:read-write",
    p => p.RequireAuthenticatedUser().RequireClaim("scope", "todo:read-write")

builder.Services.AddEndpointsApiExplorer();
builder.Services.AddSqlite<TodoDb>("Data Source=todos.db");

var app = builder.Builder();

app.MapGet("/todos", async (TodoDb db) => await db.Todos.ToListAsync());
app.MapGet("/todos/{id}", async (int id, TodoDb db) => await db.Todos.FindAsync(id) is Todo todo
    ? Results.Ok(todo)
    : Results.NotFound())
app.MapPost("/todos", async (Todo todo, TodoDb db) =>
    {
        db.Todos.Add(todo);
        await db.SaveChangesAsync();
        return Results.Created($"/todos/{todo.Id}", todo);
    })
    // This will throw at startup if authentication is not configured
    .RequireAuthorization("todo:read-write");
app.MapDelete("/todos/{id}", async (int id, TodoDb db) =>
    {
        if (await db.Todos.FindAsync(id) is Todo todo)
        {
            db.Todos.Remove(todo);
            await db.SaveChangesAsync();
            return Results.Ok(todo);
        }

        return Results.NotFound();
    })
    // Inline simple user claims checking, potentially need to figure out lifetime issues of these though
    .RequireAuthorization(user => user.HasClaim("scope", "todo:delete"));

app.Run();

class Todo
{
    public int Id { get; set; }
    public string? Title { get; set; }
    public bool IsComplete { get; set; }
}

class TodoDb : DbContext
{
    public TodoDb(DbContextOptions<TodoDb> options)
        : base(options) { }

    public DbSet<Todo> Todos => Set<Todo>();
}

@rafikiassumani-msft
Copy link
Contributor

rafikiassumani-msft commented Jan 25, 2022

@DamianEdwards This is a good start for the discussion, a few things to consider:

  1. What if the API or endpoint requires multiple scopes? With the first attempt, will the expectation be to repeat the RequiredAuthorization as shown below?
  app.MapDelete("/todo/{id}", async (int id, Todo todo) => {...} )
       .RequireAuthorization(user => user.HasClaim("scope", "todo:delete")
       .RequireAuthorization(user => user.HasClaim("groups", "admin");

There are some nuances to clarify. RequireAuthorization(user => user.HasClaim("scope", "todo:delete") could be a little bit confusing as we are checking the claim, but yet passing the scope as the value. When doing API authZ, I have often seen two patterns:

  • You check the scopes as shown in the suggestion below:
   app.MapDelete("/todo/{id}", async (int id, Todo todo) => {...} )
       .RequireAuthorization(principal => principal.HasScopes("scope1", "scope2", ...)
  • You check the claims. Please note that scopes are usually mapped to claims (Not all the time but most of the time). See example below with a group or role claim checks :
  app.MapDelete("/todo/{id}", async (int id, Todo todo) => {...} )
       .RequireAuthorization(principal => principal.HasClaim("roles", "can:delete")
       .RequireAuthorization(principal => principal .HasClaim("groups", "admin");

Notice that I am using principal, because there might not be a user in context for the case of API-to-API call or background jobs that do a client credentials flow (Oauth2) for obtaining a JWT token.

  1. Create new extension methods RequiredScopes(...) or RequireCaims(...) that will be a combination of RequireAuthorization( ) plus scope checking or claim checking.

@DamianEdwards
Copy link
Member Author

@rafikiassumani-msft yep treat my example as pseudo-code at best. Lots of details to consider.

@bloggrammer
Copy link

How do you add custom attributes with the minimal API?

@davidfowl
Copy link
Member

@bloggrammer What do you mean exactly?

@bloggrammer
Copy link

@davidfowl please, look at this answer on SO (https://stackoverflow.com/a/68974923/12476466).

How can you implement such for a minimal API such that you can return a custom result like context.Result = new ChallengeResult(CookieAuthenticationDefaults.AuthenticationScheme);?

@kjpgit
Copy link

kjpgit commented Mar 27, 2022

I thought I'd add my 2 cents as a recent user of the minimal apis. I gave up on figuring out how the 'frame work' did auth and logging and config files -- for auth, everything was worded as "JWT", and too many builders and factories to comprehend, and I just want a generic header validator -- so I just did this: https://github.com/kjpgit/techdemo/blob/274811d854d614bb09966983153ebc2339db1504/dotnet_ultra_minimal/program.cs#L96-L108

And honestly, I like it. The MyUserContext is passed in explicitly to each function, or raises an exception if it can't be constructed, so you know it's correct unlike an [Attribute] which was silently ignored(!) when I tried it at the class level.

If your "new simple solution" isn't as simple as what I did... basically a few lines of code... I see no reason to switch :-)

On one person projects, just an AsyncLocal and a custom, trivial stage(s) in the pipeline to handle auth and final post-request structured logging seem sufficient for me...

@rafikiassumani-msft rafikiassumani-msft changed the title Configuring minimal APIs for authorization should be suitably simple & featured [Epic]: Configuring minimal APIs for authorization should be suitably simple & featured Apr 4, 2022
@rafikiassumani-msft rafikiassumani-msft removed the triage-focus Add this label to flag the issue for focus at triage label Apr 4, 2022
@rafikiassumani-msft rafikiassumani-msft changed the title [Epic]: Configuring minimal APIs for authorization should be suitably simple & featured Discussion: Configuring minimal APIs for authorization should be suitably simple & featured Apr 4, 2022
@damienbod
Copy link
Contributor

I think it would be good if you could define the scheme as well and include support for multiple schemes. (Also on the group function/method wrapping the minimal APIs)

@davidfowl
Copy link
Member

davidfowl commented May 7, 2022

I think it would be good if you could define the scheme as well and include support for multiple schemes. (Also on the group function/method wrapping the minimal APIs)

Different to how that is done today? These are normal auth policies.

app.MapGet("/", () => "Hello World!")
   .RequireAuthorization(pb => pb.AddAuthenticationSchemes("Foo").RequireClaim("claim"));

@damienbod
Copy link
Contributor

Looks like to need to try this and read the docs :) This is perfect. Could I apply something like this to a group of minimal APIs as well?

@davidfowl
Copy link
Member

davidfowl commented May 7, 2022

Yep. Either by passing the policy to many endpoints or using the new grouping feature coming in next .NET 7 preview.

@rafikiassumani-msft rafikiassumani-msft changed the title Discussion: Configuring minimal APIs for authorization should be suitably simple & featured [Epic]: Configuring minimal APIs for authorization should be suitably simple & featured Jun 14, 2022
@rafikiassumani-msft rafikiassumani-msft added the Epic Groups multiple user stories. Can be grouped under a theme. label Jun 14, 2022
@rafikiassumani-msft rafikiassumani-msft changed the title [Epic]: Configuring minimal APIs for authorization should be suitably simple & featured [Epic]: Configuring minimal APIs for auth should be suitably simple & featured Jun 14, 2022
@ghost
Copy link

ghost commented Oct 11, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 1, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Epic Groups multiple user stories. Can be grouped under a theme. feature-minimal-actions Controller-like actions for endpoint routing Needs: Design This issue requires design work before implementating.
Projects
None yet
Development

No branches or pull requests