[Analyzer/Codefixer] Recommend using `AddAuthorizationBuilder` for configuring global policies

[captainsafia]

Background and Motivation

In .NET 7, we introduced an AddAuthorizationBuilder extension method on IServiceCollection that would register authorization-related services and provide an AuthorizationBuilder for constructing policies. This is an abbreviated syntax that allows reduces nesting in the original pattern of calling AddAuthorization and providing a policy construct as a callback.

Proposed Analyzer

Analyzer Behavior and Message

When the user provides code where AddAuthorizationBuilder would provide a more abbreviated style, recommend a codefix with the following message:

Use AddAuthorizationBuilder to register authorization services and construct policies.

Category

• Design
• Documentation
• Globalization
• Interoperability
• Maintainability
• Naming
• Performance
• Reliability
• Security
• Style
• Usage

Severity Level

• Error
• Warning
• Info
• Hidden

Usage Scenarios

Before

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddAuthorization(options =>
{
    options.AddPolicy("AtLeast21", policy =>
        policy.Requirements.Add(new MinimumAgeRequirement(21)));
});

var app = builder.Build();

app.UseAuthorization();

app.Run();

After

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddAuthorizationBuilder()
  .AddPolicy("AtLeast21", policy =>
  {
        policy.Requirements.Add(new MinimumAgeRequirement(21)));
  });

var app = builder.Build();

app.UseAuthorization();

app.Run();

Risks

Marking this as an info-only analyzer strikes a good balance between informing the user of this feature without being too presumptuous (via a warning). A refactoring would not have been good at educating the user about the functionality.

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.

[halter73]

API Review Notes:

1. Is the message to passive? Starting with "Recommend using" seems wrong.
2. Can you use AddAuthorization and AddAuthorizationBuilder together? Do the policies just merge?

[captainsafia]

Can you use AddAuthorization and AddAuthorizationBuilder together? Do the policies just merge?

I tried this out. You can use them together and the policies end up being merged with a last-in wins strategy for policies with the same name.

using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.JwtBearer;

var builder = WebApplication.CreateBuilder();

builder.Services.AddAuthentication().AddJwtBearer();
builder.Services.AddAuthorization(options =>
{
    options.AddPolicy("AtLeast21", policy => policy.RequireClaim("foo", "bar"));
    options.AddPolicy("OtherThing", policy => policy.RequireClaim("one", "two"));
});

builder.Services.AddAuthorizationBuilder()
  .AddPolicy("AtLeast21", policy => policy.RequireClaim("other", "claim"));


var app = builder.Build();

app.MapGet("/", () => "Hello world!")
    .RequireAuthorization("AtLeast21", "OtherThing");

app.Run();

That being said, I'm not sure how likely this is to occur in user code. Considering the merge strategy, calling both is equivalent to calling one with the policies registered in the same order. It would be helpful to provide another analyzer that recommended people unify under one invocation.

Given this, I think it is safe to provide the analyzer and codefix and extend this with functionality (maybe a refactoring) that unifies multiple calls to AddAuthorizationBuilder.

[captainsafia]

Is the message to passive? Starting with "Recommend using" seems wrong.

With regard to this, it seems like the general convention is to start with "Use ..." so I've updated the description to mimick this.

[halter73]

Given that combining multiple calls of AddAuthorization and AddAuthorizationBuilder allows merging policies, it sounds like the fixer would be very unlikely to break people. So, I think the fixer makes sense. I agree this is probably pretty rare to do both too, so doubt there's much value in a refactoring that unifies multiple calls, but maybe.