Introduce WebApplicationBuilder.Authorization property that allows for AuthZ setup

[DamianEdwards]

Spin-off from #42172 which was a spin-off of #39855

Goal is to create a top-level (i.e. WebApplicationBuilder level) API for setting up AuthZ, mirroring WebApplicationBuilder.Authentication and removing the need to have to find and use builder.Services.AddAuthorization() or builder.Services.Configure<AuthorizationOptions>() to setup global AuthZ options in an app.

We will introduce a new AuthorizationBuilder class to be the public service area for AuthZ going forward (similar to AuthenticationBuilder for AuthN). It will also be accessible via a new IServiceCollcetion extension method AddAuthorizationBuilder() since we cannot change AddAuthorization() to return this without a breaking change

builder.Authentication.UseJwtBearer();

// New type AuthorizationBuilder, accessible via builder OR via IServiceCollection
builder.Authorization // or builder.Services.AddAuthorizationBuilder()
    // Can set as default policy as part of creation
    .AddDefaultPolicy("MyPolicy", policy =>
        policy.RequireAuthenticatedUser()
              .RequireClaim("scope", "foo:bar")
    .AddPolicy("AnotherPolicy", p =>
            p.RequireAuthenticatedUser()
             .RequireClaim("scope", "baz:bleh"));
    });

Proposed new API:

public class WebApplicationBuilder
{
+    public AuthorizationBuilder Authorization { get; }
}

public static class PolicyServiceCollectionExtensions
{
+   public static AuthorizationBuilder AddAuthorizationBuilder(this IServiceCollection services)
}

+public class AuthorizationBuilder
+{
+    public AuthorizationBuilder AddPolicy(
+        string name,
+        Action<AuthorizationPolicyBuilder> configurePolicy);
+    public AuthorizationBuilder AddPolicy(
+        string name,
+        AuthorizationPolicy policy);
+    public AuthorizationBuilder AddDefaultPolicy(
+        string name,
+        Action<AuthorizationPolicyBuilder> configurePolicy);
+    public AuthorizationBuilder AddDefaultPolicy(
+        string name,
+        AuthorizationPolicy policy);
+    public AuthorizationBuilder AddFallbackPolicy(
+        string name,
+        Action<AuthorizationPolicyBuilder> configurePolicy);
+    public AuthorizationBuilder AddFallbackPolicy(
+        string name,
+        AuthorizationPolicy policy);
+    public AuthorizationBuilder SetDefaultPolicy(string name);
+    public AuthorizationBuilder SetFallbackPolicy(string name);
+    public AuthorizationBuilder SetInvokeHandlersAfterFailure(bool invoke)
+}

[HaoK]

So basically the main difference from today is something like this?

builder.Authorization.AddPolicy("MyPolicy", policy =>
        policy.RequireAuthenticatedUser()
              .RequireClaim("scope", "foo:bar"),
        setAsDefault: true) 

vs today

services.AddAuthorization(o => {
   o.AddPolicy("MyPolicy", p => p.RequireAuthenticatedUser()
              .RequireClaim("scope", "foo:bar"));
   o.DefaultPolicy = o.GetPolicy("MyPolicy");
})

[DamianEdwards]

Right, except that the second example actually has to be qualified to the builder, so:

builder.Services.AddAuthorization(o =>
{
    o.AddPolicy("MyPolicy", p =>
        p.RequireAuthenticatedUser()
         .RequireClaim("scope", "foo:bar"));
    o.DefaultPolicy = o.GetPolicy("MyPolicy");
})

And of course builder.Authentication is already calling builder.Services.AddAuthorization() on your behalf, so this is about creating something more top-level for the sake of symmetry (sugar, aesthetics, etc.). So comparing the discoverability/aesthetics between these two:

// Both APIs top-level
builder.Authentication.UseJwtBearer();
builder.Authorization.AddPolicy(...);

// Split APIs
builder.Authentication.UseJwtBearer();
builder.Services.AddAuthorization(c => c.AddPolicy(...));

[HaoK]

Sure would it be enough just to have a single Configure which just is shortening Configure<AuthorizationOptions>, that gives you symmetry without adding very much, and feels pretty much identical to today.

builder.Authorization.Configure(o =>
{
    o.AddPolicy("MyPolicy", p =>
        p.RequireAuthenticatedUser()
         .RequireClaim("scope", "foo:bar"));
    o.DefaultPolicy = o.GetPolicy("MyPolicy");
})

[captainsafia]

Another benefit of the model is removing the nesting involved with having to call AddPolicy on the options which contributes to our goal of reducing LoC to configure authz in apps.

[DamianEdwards]

Another benefit of the model is removing the nesting involved with having to call AddPolicy on the options which contributes to our goal of reducing LoC to configure authz in apps.

Right, one-less nested delegate, on top of one less property to discover/navigate, plus remove the slight dissonance of the AddAuthorization needing to be called even though it's already added (minor I know).

[ghost]

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:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[davidfowl]

I'm not sure removing a single top level delegate is worth the overhead of adding 4 top level methods to the WebApplicationBuilder which is an alternative way to configure authorization that doesn't seem that much simpler. I'd rather see more effort going into simplifying whatever we think is complex about wiring up authorization policies themselves.

[DamianEdwards]

@davidfowl it's not 4 methods on the WebApplicationBuilder, it's a single property, which has methods on it.

[davidfowl]

Ah I see. Still, I'd rather have the a similar API that isn't tied to WebApplicationBuilder. IMHO this adds a property to make it more discoverable at the cost of having 2 APIs we'd have to document that do the exact same thing in different ways.

[DamianEdwards]

IMHO this adds a property to make it more discoverable at the cost of having 2 APIs we'd have to document that do the exact same thing in different ways.

Unfortunately we don't have the advantage of an existing builder type for AuthZ like we did for AuthN that meant we only needed to add a top-level property that exposed the already existing AuthenticationBuilder, and we can't introduce AuthorizationBuilder as the return type of services.AddAuthorization() because that's breaking. It saddens me that we're struggling to come up with a simple addition to avoid the following asymmertry:

builder.Authentication.AddJwtBearer();
builder.Services.AddAuthorization(options =>
{
    options.AddPolicy("ApiReadAccess", policy =>
        policy.RequireAuthenticatedUsers()
            .RequireClaim("scope", "myapi:read"));
    options.DefaultPolicy = options.GetPolicy("ApiReadAccess");
});

[HaoK]

Okay, since we don't have the equivalent for an AuthorizationBuilder right now, I can take a stab at mocking something up for that as part of config stuff I'm prototyping right now, since I was looking for a place to hang methods other than extension methods on ServiceCollection

[DamianEdwards]

@HaoK here's the hacky one I was using to explore this idea:

public class WebApplicationAuthorizationBuilder
{
    private readonly WebApplicationBuilder _builder;

    public WebApplicationAuthorizationBuilder(WebApplicationBuilder builder)
    {
        _builder = builder;
    }

    public WebApplicationAuthorizationBuilder AddPolicy(
        string name,
        Action<AuthorizationPolicyBuilder> configurePolicy,
        bool setAsDefault = false,
        bool setAsFallback = false)
    {
        _builder.Services.AddAuthorization(authzOptions =>
        {
            var policyBuilder = new AuthorizationPolicyBuilder();
            configurePolicy(policyBuilder);
            var policy = policyBuilder.Build();
            
            authzOptions.AddPolicy(name, policy);
            
            if (setAsDefault)
            {
                authzOptions.DefaultPolicy = policy;
            }
            
            if (setAsFallback)
            {
                authzOptions.FallbackPolicy = policy;
            }
        });

        return this;
    }

    public WebApplicationAuthorizationBuilder SetDefaultPolicy(string name)
    {
        _builder.Services.AddAuthorization(authzOptions =>
        {
            if (authzOptions.GetPolicy(name) is AuthorizationPolicy policy)
            {
                authzOptions.DefaultPolicy = policy;
            }
            else
            {
                throw new InvalidOperationException($"Can't find policy named '{name}'.");
            }
        });

        return this;
    }

    public WebApplicationAuthorizationBuilder SetFallbackPolicy(string name)
    {
        _builder.Services.Configure<AuthorizationOptions>(authzOptions =>
        {
            if (authzOptions.GetPolicy(name) is AuthorizationPolicy policy)
            {
                authzOptions.FallbackPolicy = policy;
            }
            else
            {
                throw new InvalidOperationException($"Can't find policy named '{name}'.");
            }
        });

        return this;
    }

    public WebApplicationAuthorizationBuilder Configure(Action<AuthorizationOptions> configure)
    {
        _builder.Services.Configure(configure);

        return this;
    }
}

public static class WebApplicationBuilderExtensions
{
    private static readonly object _key = new();

    public static WebApplicationAuthorizationBuilder Authorization(this WebApplicationBuilder builder)
    {
        if (builder.Host.Properties.TryGetValue(_key, out var value) && value is WebApplicationAuthorizationBuilder authzBuilder)
        {
            return authzBuilder;
        }

        if (value is { })
        {
            throw new InvalidOperationException("There's a different object living in our slot!");
        }

        authzBuilder = new WebApplicationAuthorizationBuilder(builder);
        builder.Host.Properties.Add(_key, authzBuilder);
        return authzBuilder;
    }
}

[HaoK]

So if this is symmetrical to AuthenticationBuilder, we don't expose a generic Configure on it, since it already exposes the ServiceCollection

What about something like this

    public class AuthorizationBuilder {
           public IServiceCollection Services { get; }
           public void AddPolicy(...) // whatever overloads we want
           public AuthorizationPolicy DefaultPolicy { get; set; }
           public void AddDefaultPolicy(string policyName, ...); // Does AddPolicy with name and sets it to default
           public AuthorizationPolicy? FallbackPolicy { get; set; }
           public void AddFallbackPolicy(string policyName, ...); // Does AddPolicy with name and sets it to fallback
    }

EDIT: didn't realize the default/fallback require an instance but don't reqiure a name, so maybe we just have sugar methods for adding/setting default/fallback policy in addition to the normal instance sets.

[DamianEdwards]

How would this be accessed via IServiceCollection (i.e. like AuthenticationBuilder IServiceCollection.AddAuthentication()?

[HaoK]

Unfortunately our AddAuthZ methods return IServiceCollection so unless we are going to do a breaking change I guess people just have to new AuthorizationBuilder(services)

[HaoK]

We can use this type on WebApplicationBuilder though right? WebApplicationBuilder.Authorization can expose this once we add it

[DamianEdwards]

I think that's the thing that @davidfowl is pushing back on though, i.e. having a WAB-level way of doing something that doesn't match the services equivalent. Without getting past that we'd be locked into doing nothing here it seems.

[HaoK]

I don't think so, this is how we added builder to everything before. AuthenticationBuilder and OptionsBuilder were both sugar APIs we added ontop of IServiceCollection. AuthorizationBuilder follows that pattern, my read on the pushback is that it shouldn't be done at the WAB layer, this type of sugar belongs at the AuthZ layer.

[davidfowl]

How do you get an AuthorizationBuilder? Seems we've already used the parameterless method here...

[HaoK]

Yeah this is the janky part.

So new AuthorizationBuilder(IServiceCollection), or we do a breaking change to change the parameterless overload to return this instead of IServiceCollection? Will be a easy compilation break for customers to react to at least?

[DamianEdwards]

If we add this though the intent would be to add/support it at both places, right, exactly like we did for AuthenticationBuilder?

[HaoK]

Yeah that's my impression, we'd expose AuthorizationBuilder via WAB.Authorization, but we'd also want to make existing services.AddAuthorization be equivalent, which would necessitate a breaking change or a new pattern

[DamianEdwards]

Right, so what I'm still not clear on is how it's exposed when working against IServiceCollection, can you add a code sample of what a non-breaking approach would be?

[HaoK]

Yeah that's what I was actually noodling around with right now (how to update the unit tests to use this new api).

I mean this is the one I was looking at the canonical add policy

            services.AddAuthorization(options =>
            {
                options.AddPolicy("Basic", policy => policy.RequireClaim("Permission", "CanViewPage", "CanViewAnything"));
            });

Breaking change:

            services.AddAuthorization().AddPolicy("Basic", policy => 
                      policy.RequireClaim("Permission", "CanViewPage", "CanViewAnything"));

Non breaking change (kinds gross)

            var builder = new AuthorizationBUilder(services.AddAuthorization()).AddPolicy("Basic", policy => 
                      policy.RequireClaim("Permission", "CanViewPage", "CanViewAnything"));

Or

            services.AddAuthorization();
            new AuthorizationBuilder(services).AddPolicy("Basic", policy => 
                      policy.RequireClaim("Permission", "CanViewPage", "CanViewAnything"));

[DamianEdwards]

Ahh if only C# supported overloads on return types 😃

The breaking change is obviously the most natural looking but I really don't think we can justify making a break of that magnitude. The only other idea I can think of is another services extension overload that returns the builder, e.g.:

builder.Services.AddAuthorization(useBuilder: true)
    .AddPolicy(...);

or

builder.Services.AddAuthorizationWithBuilder()
    .AddPolicy(...);

[HaoK]

Actually this is much easier than I realized, we just have AuthorizationBuilder implement IServiceCollection and proxy all calls into the service collection, so we get everything we want without breaking change

[HaoK]

So this: pros: no breaking change, cons: the full service collection API is exposed rather than scoped to authZ for intellesense

    public class AuthorizationBuilder : IServiceCollection {
           private IServiceCollection _services; // Don't need this to be public since all the API is now on the builder

           public void AddPolicy(...) // whatever overloads we want
           public void SetDefaultPolicy(AuthorizationPolicy policy) // null default policy is not allowed
           public void AddDefaultPolicy(string policyName, ...); // Does AddPolicy with name and sets it to default
           public AuthorizationPolicy? FallbackPolicy { get; set; }
           public void SetFallbackPolicy(AuthorizationPolicy? policy) // null fallback policy is allowed
           public void AddFallbackPolicy(string policyName, ...); // Does AddPolicy with name and sets it to fallback
    }

Or maybe we add two types:

// AddAuthorization returns this, so no breaking changes
public class AuthorizationBuilderServiceCollection : AuthorizationBuilder, IServiceCollection,

// For WebApplicationBuilder.Authorization
public class AuthorizationBuilder like before

[DamianEdwards]

I'm going to say we can't make a breaking change here, so I'd vote for the two types approach you outline, which would make the builder.Authorization access path the cleanest.

[DamianEdwards]

To make it super clear, these would be equivalent:

// Off of WebApplicationBuilder, returning AuthorizationBuilder
builder.Authorization.AddPolicy(...);

// Off of IServiceCollection, returning AuthorizationBuilderServiceCollection
builder.Services.AddAuthorization()
    .AddPolicy(...);

[DamianEdwards]

Changing the return type of the method is breaking so we need to consider other patterns, e.g. a new method name or a new overload of AddAuthorization, like suggested here

[DamianEdwards]

Current leader is services.AddAuthorizationBuilder()

[HaoK]

@DamianEdwards should I edit the description of this issue to use for API-review or do you want me to create a new issue?

[DamianEdwards]

@HaoK let's just use this issue to cover it all

[HaoK]

@DamianEdwards updated comments with the current PR, can you double check this has everything you wanted?

[DamianEdwards]

@HaoK looks good, marking ready for review.

[ghost]

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:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[BrennanConroy]

API review notes:

• public AuthorizationBuilder SetInvokeHandlersAfterFailure(bool invoke)
  • Let's rename this to InvokeHandlersAfterFailure
• Not touching

public class WebApplicationBuilder
{
+    public AuthorizationBuilder Authorization { get; }
}

letting David and co. discuss this one.

namespace Microsoft.AspNetCore.Authorization;

public static class PolicyServiceCollectionExtensions
{
+   public static AuthorizationBuilder AddAuthorizationBuilder(this IServiceCollection services)
}

+public class AuthorizationBuilder
+{
+    AuthorizationBuilder(IServiceCollection services);
+    public virtual IServiceCollection { get; }
+    public virtual AuthorizationBuilder AddPolicy(
+        string name,
+        Action<AuthorizationPolicyBuilder> configurePolicy);
+    public virtual AuthorizationBuilder AddPolicy(
+        string name,
+        AuthorizationPolicy policy);
+    public virtual AuthorizationBuilder AddDefaultPolicy(
+        string name,
+        Action<AuthorizationPolicyBuilder> configurePolicy);
+    public virtual AuthorizationBuilder AddDefaultPolicy(
+        string name,
+        AuthorizationPolicy policy);
+    public virtual AuthorizationBuilder AddFallbackPolicy(
+        string name,
+        Action<AuthorizationPolicyBuilder> configurePolicy);
+    public virtual AuthorizationBuilder AddFallbackPolicy(
+        string name,
+        AuthorizationPolicy policy);
+    public virtual AuthorizationBuilder SetDefaultPolicy(string name);
+    public virtual AuthorizationBuilder SetFallbackPolicy(string name);
+    public virtual AuthorizationBuilder InvokeHandlersAfterFailure(bool invoke);
+}

API approved!

[HaoK]

Done in #42264