AddIdentityCookies() should allow for chaining additional AuthenticationBuilders so it is consistent with other providers.

[ahazelwood]

Background and Motivation

When chaining multiple Application providers together in a asp.net core project, there is no way to chain additional providers off of "AddIdentityCookies()". This seems counterintuitive when other providers such as AddMicrosoftAccount return AuthenticationBuilder instances so the chaining can continue. Currently a instance of IdentityCookiesBuilder is returned which doesn't extend AuthenticationBuilder but instead just allows reference to various cookies.

Proposed API

In IdentityCookieAuthenticationBuilderExtensions.cs the signature should be changed to:

    /// <summary>
    /// Adds cookie authentication.
    /// </summary>
    /// <param name="builder">The current <see cref="AuthenticationBuilder"/> instance.</param>
    /// <returns>The <see cref="IdentityCookiesBuilder"/> which can be used to configure the identity cookies.</returns>
+    public static AuthenticationBuilder AddIdentityCookies(this AuthenticationBuilder builder)
        => builder.AddIdentityCookies(o => { });

    /// <summary>
    /// Adds the cookie authentication needed for sign in manager.
    /// </summary>
    /// <param name="builder">The current <see cref="AuthenticationBuilder"/> instance.</param>
    /// <param name="configureCookies">Action used to configure the cookies.</param>
    /// <returns>The <see cref="IdentityCookiesBuilder"/> which can be used to configure the identity cookies.</returns>
+    public static AuthenticationBuilder AddIdentityCookies(this AuthenticationBuilder builder, Action<IdentityCookiesBuilder> configureCookies)
    {
        var cookieBuilder = new IdentityCookiesBuilder();
        cookieBuilder.ApplicationCookie = builder.AddApplicationCookie();
        cookieBuilder.ExternalCookie = builder.AddExternalCookie();
        cookieBuilder.TwoFactorRememberMeCookie = builder.AddTwoFactorRememberMeCookie();
        cookieBuilder.TwoFactorUserIdCookie = builder.AddTwoFactorUserIdCookie();
        configureCookies?.Invoke(cookieBuilder);
+      return builder;
    }

   ...

Usage Examples

Alternative Designs

An alternative design would be to have IdentityCookiesBuilder extend AuthenticationBuilder since it is only exposes four OptionsBuilder instances. This would allow chaining to potentially continue since the base class can be leveraged.

Risks

There may be a need to still access the underlying IdentityCookieBuilder that is used, and I'm not sure what would be a good way to still get access to this functionality outside of having IdentityCookiesBuilder extend AuthenticationBuilder.

[halter73]

Unfortunately, it's not possible to overload methods based on return type, and we cannot change the existing AddIdentityCookies methods that returns an IdentityCookiesBuilder to return an AuthenticationBuilder without breaking people.

An alternative design would be to have IdentityCookiesBuilder extend AuthenticationBuilder since it is only exposes four OptionsBuilder instances. This would allow chaining to potentially continue since the base class can be leveraged.

This might be possible, but it would be unusual. I'm not sure it's worth it, but I'll mark this as api-ready-for-review to get feedback for the rest of the team.

[dotnet-policy-service[bot]]

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.

[janusqa]

I would like to do something like the below

builder.Services.AddAuthentication(options =>
{
    options.DefaultAuthenticateScheme = "CUSTOM123";
    options.DefaultChallengeScheme = "CUSTOM123";
})
.AddScheme<JwtBearerOptions, CustomJwtBearerHandler>(JwtBearerDefaults.AuthenticationScheme, options =>
{
// some jwt stuff
})
.AddIdentityCookies()
.AddPolicyScheme("CUSTOM123", "CUSTOM123", options =>
{
    // runs on each request
    options.ForwardDefaultSelector = context =>
    {
            // some stuff
            if (some_more_stuff) return "Bearer";

        return "Cookies";
    };
});

How would I accomplish this without being able to chain. I want to get built in identity going.

[halter73]

There are several workarounds @janusqa. You could save AddIdentityCookies for last of course. Or you can just store the AuthenticationBuilder in a variable and not rely on chaining at all.

var authBuilder  = builder.Services.AddAuthentication(options =>
{
    options.DefaultAuthenticateScheme = "CUSTOM123";
    options.DefaultChallengeScheme = "CUSTOM123";
});
authBuilder.AddScheme<JwtBearerOptions, CustomJwtBearerHandler>(JwtBearerDefaults.AuthenticationScheme, options =>
{
// some jwt stuff
});
authBuilder.AddIdentityCookies();
authBuilder.AddPolicyScheme("CUSTOM123", "CUSTOM123", options =>
{
    // runs on each request
    options.ForwardDefaultSelector = context =>
    {
            // some stuff
            if (some_more_stuff) return "Bearer";

        return "Cookies";
    };
});

[halter73]

API Review Notes:

• Another option would be to add a new method to IdentityCookiesBuilder like GetAuthenticationBuilder() which would allow you to keep your code "fluent" (i.e. not require an authBuilder variable).

  uilder.Services.AddAuthentication(options =>
  
     options.DefaultAuthenticateScheme = "CUSTOM123";
     options.DefaultChallengeScheme = "CUSTOM123";
  )
  AddScheme<JwtBearerOptions, CustomJwtBearerHandler>(JwtBearerDefaults.AuthenticationScheme, options =>
  
  / some jwt stuff
  )
  AddIdentityCookies()
  GetAuthenticationBuilder()
  AddPolicyScheme("CUSTOM123", "CUSTOM123", options =>
  
     // runs on each request
     options.ForwardDefaultSelector = context =>
     {
             // some stuff
             if (some_more_stuff) return "Bearer";
  
         return "Cookies";
     };
  );
  

  • This seems better than adding a new base type to IdentityCookiesBuilder and is also non-breaking.

Thanks for submitting the API proposal. At this time, we don't think that any new API is necessary here considering the workaround of storing the AuthenticationBuilder in a variable is simple, so are rejecting this API.