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

Remove Authentication property from WebApplicationBuilder #42577

Closed
captainsafia opened this issue Jul 5, 2022 · 5 comments
Closed

Remove Authentication property from WebApplicationBuilder #42577

captainsafia opened this issue Jul 5, 2022 · 5 comments
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-authentication
Milestone

Comments

@captainsafia
Copy link
Member

captainsafia commented Jul 5, 2022

Background and Motivation

In preview5, we introduced an Authentication property on the WebApplicationBuilder that provided access to a custom AuthenticationBuilder for registering new authentication schemes onto an app. The custom AuthenticationBuilder was also responsible for automatically registering custom authentication schemes in an application.

However, there's a hiccup with that particular design. In particular, when a user wants to configure global authentication options in their application, they will have to fall back to using the pre-existing AddAuthentication extension method on the IServiceCollection.

To remedy this, we move from an Authentication property to an AddAuthentication extension method to provide the benefits of a (1) top-level authentication property/method on the WAB and (2) to make it easier to configure global authentication options and configure authentication schemes in their application in one step.

We also provide a parallel AddAuthorization method to support the new AuthorizationBuilder APIs introduced in preview6.

Proposed API

// Assembly: Microsoft.AspNetCore
namespace Microsoft.AspNetCore.Builder;

public static class AuthenticationWebApplicationBuilderExtensions
{
  public static AuthenticationBuilder AddAuthentication(this WebApplicationBuilder builder);
  public static AuthenticationBuilder AddAuthentication(this WebApplicationBuilder builder, string defaultScheme);
  public static AuthenticationBuilder AddAuthentication(this WebApplicationBuilder builder, Action<AuthenticationOptions> configureOptions);
}
// Assembly: Microsoft.AspNetCore
namespace Microsoft.AspNetCore.Builder;

public static class AuthorizationWebApplicationBuilderExtensions
{
  public static AuthorizationBuilder AddAuthorization(this WebApplicationBuilder builder);
  public static AuthorizationBuilder AddAuthorization(this WebApplicationBuilder builder, Action<AuthorizationOptions> configure);
}

Usage Examples

Previously:

var builder = WebApplication.CreateBuilder(args);
builder.Authentication.AddJwtBearer("foobar");
builder.Services.AddAuthentication(o =>
{
  o.DefaultScheme = "foobar";
});

Now:

var builder = WebApplication.CreateBuilder(args);
builder.AddAuthentication(o =>
{
  o.DefaultScheme = "foobar";
}).AddJwtBearer("foobar");

Alternative Designs

  • Remove top-level property altogether
  • Add support for a builder.Authentication.ConfigureDefaults method

Risks

We're shipping this update to the API in preview7 so there's limited options to iterate if we find any other problems.

@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-authentication labels Jul 5, 2022
@ghost
Copy link

ghost commented Jul 5, 2022

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.

@HaoK
Copy link
Member

HaoK commented Jul 5, 2022

I think we should also have the configure overload for AddAuthorization

@captainsafia
Copy link
Member Author

I think we should also have the configure overload for AddAuthorization

Good call. Updated the PR and will update the API review.

@captainsafia
Copy link
Member Author

Update on this since we weren't totally confident with the changes here. For now, we've decided to remove the top-level property altogether for .NET 7. The changes we'll be working on in #42481 make large parts of this property redundant. Update API proposal is below.

Proposed API

namespace Microsoft.AspNetCore.Builder;

public sealed class WebApplicationBuilder
{
-  public AuthenticationBuilder Authentication { get; }
}

Usage Examples

Previous:

var builder = WebApplication.CreateBuilder(args);
builder.Authentication.AddJwtBearer("foobar");

Now (same as .NET 6):

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddAuthentication().AddJwtBearer("foobar");

@captainsafia captainsafia changed the title AddAddAuthentiation and AddAuthorization methods to WebApplicationBuilder Remove Authentication property from WebApplicationBuilder Jul 5, 2022
@rafikiassumani-msft rafikiassumani-msft added this to the 7.0-preview7 milestone Jul 5, 2022
@halter73
Copy link
Member

halter73 commented Jul 6, 2022

API review notes:

  1. It's less aesthetically pleasing to write builder.Services.AddAuthentication() instead of builder.Authentication, but it's not worth a top-level property on WAB at the moment. Maybe in the future.
namespace Microsoft.AspNetCore.Builder;

public sealed class WebApplicationBuilder
{
-  public AuthenticationBuilder Authentication { get; }
}

API Approved.

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-authentication
Projects
None yet
Development

No branches or pull requests

4 participants