[Design] Authentication 2.0

[HaoK]

TL:DR summary

• New IAuthenticationService with the same Authenticate/Challenge/SignIn/Out/ForbidAsync API.
• Made up of AuthenticationScheme instances which have: Name, Type, and Settings dictionary (Options instance + old description bag)
• app.UseXyzAuth(new XyzOptions()) => services.AddXyzAuth(o => o.Foo = bar) adds an AuthenticationScheme instance for Xyz
• Single UseAuthentication is required, which takes care of the old 'automatic' challenge, authenticate, in addition to giving each scheme a chance to handle the request like the old middlewares did.
• Auth provider extensibility model is mostly unchanged, just tweaked (no more middleware, handler base class is cleaned up and activated from DI (see DbContext example below)
• 'Automatic' authenticate and challenge are now a single Default[Authenticate/Challenge/SignIn]Scheme in AuthenticationOptions, rather than a flag on each middleware. No ambiguity, altho there's one bit of magic where if none are specified and there's only a single scheme registered, we use that scheme as the default.

Pros of the new design

• AuthN is now a service and 'feels' like AuthZ.
• Auth handlers can now consume services from DI easily (e.g. see DbContext example).
• ClaimsTransformation simplified (sane now, rather than wrapped AuthHandler + xform middleware fun), example below.
• Dynamic schemes supported via replacing the IAuthenticationSchemeProvider service.
• Eliminated a lot of design/complexity issues that we have consistently run into when trying to fix bugs (no more handler chaining, automatic gone)

Example of using scoped DbContext

This was not really easily accomplished in the old stack since handlers were created by the middleware via protected abstract AuthenticationHandler<TOptions> CreateHandler();

   // ConfigureServices:
   services.AddSchemeHandler<CookieAuthenticationOptions, MyCookieAuthenticationHandler>("Cookie1", o => { o.CookieName = "Cookie1" });
   services.AddSchemeHandler<CookieAuthenticationOptions, MyCookieAuthenticationHandler>("Cookie2", o => { o.CookieName = "Cookie2" });

public class MyCookieAuthenticationHandler : CookieAuthenticationSchemeHandler {
    public MyCookieAuthenticationHandler(DbContext context, ILoggerFactory logger, UrlEncoder encoder) : base(logger, encoder) {
       _dbContext = context;
    }
    
        protected async override Task<CookieAuthenticationOptions> CreateOptionsAsync()
        {
            var options = await base.CreateOptionsAsync();
            UseYourDbContextOnThese(options);
        }
}

Composing configuration of a scheme's options looks like this (Identity needs this):

                    services.AddCookieAuthentication("Cookie1");
                    services.ConfigureSchemeHandler<CookieAuthenticationOptions>("Cookie1",
                        o => o.CookieName = "One");

Claims Transformation

A lot simpler, new IClaimsTransformation service which has a single method:
Task<ClaimsPrincipal> TransformAsync(ClaimsPrincipal principal)

We call this on any successful AuthenticateAsync call.

   services.AddAuthentication(o => o.ClaimsTransform = p => {
     p.AddIdentity(new ClaimsIdentity());
     return Task.FromResult(p);
  }

To use a more advanced scoped claims transform that uses DbContext's, they would just need to add their own IClaimsTransformation before any AddAuthentication calls.

Interop options (Currently exploring Partial interop)

• No interop (nuke all old code, takeover namespaces as necessary entirely)
• Partial interop (focusing on Startup.cs interop, aka make UseXyzAuth do something similar with the new stack. PR adds a UseLegacyAuthentication which the old UseXyzAuth methods would call with the XyzOptions, and this would add a new AuthenticationScheme to the new service.
• Full interop (would need to keep the old code, and play some games inside of the IAuthenticationFeature)

Example: Updated with twitter, google social auth + cookie sign in

public class Startup
{
     public void ConfigureServices(IServiceCollection services)
     {
             services.AddAuthentication(o =>
             {
                 o.DefaultAuthenticationScheme = CookieAuthenticationDefaults.AuthenticationScheme;
                 o.DefaultChallengeScheme = CookieAuthenticationDefaults.AuthenticationScheme;
                 o.DefaultSignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;
                 o.ClaimsTransform => p => {
                     if (!p.Identities.Any(i => i.AuthenticationType == "Cookies"))
                     {
                        var id = new ClaimsIdentity("xform");
                        id.AddClaim(new Claim("xform", "yup"));
                        p.AddIdentity(id);
                     }
                     return Task.FromResult(p); 
                 }
            });
            services.AddCookieAuthentication();
            services.AddGoogleAuthentication(options =>
            {
                options.ClientId = "667949426586-0bor1qj05d9fjqkvhil6tvoupjfv46fr.apps.googleusercontent.com";
                options.ClientSecret = "tPycZp08PGQBNDz2XycEB145";
                options.SaveTokens = true;
                options.Events = new OAuthEvents()
                {
                    OnRemoteFailure = ctx =>
                    {
                        ctx.Response.Redirect("/error?FailureMessage=" + UrlEncoder.Default.Encode(ctx.Failure.Message));
                        ctx.HandleResponse();
                        return Task.FromResult(0);
                    }
                };
            });
            services.AddTwitterAuthentication(options =>
            {
                options.ConsumerKey = "BSdJJ0CrDuvEhpkchnukXZBUv";
                options.ConsumerSecret = "xKUNuKhsRdHD03eLn67xhPAyE1wFFEndFo1X2UJaK2m1jdAxf4";
                options.RetrieveUserDetails = true;
                options.SaveTokens = true;
                options.Events = new TwitterEvents()
                {
                    OnCreatingTicket = ctx =>
                    {
                        var profilePic = ctx.User.Value<string>("profile_image_url");
                        ctx.Principal.Identities.First().AddClaim(new Claim("urn:twitter:profilepicture", profilePic, ClaimTypes.Uri, ctx.Options.ClaimsIssuer));
                        return Task.FromResult(0);
                    },
                    OnRemoteFailure = ctx =>
                    {
                        ctx.Response.Redirect("/error?FailureMessage=" + UrlEncoder.Default.Encode(ctx.Failure.Message));
                        ctx.HandleResponse();
                        return Task.FromResult(0);
                    }
                };
            });

     }

     public void Configure(IApplicationBuilder app)
     {
            app.UseAuthentication(); // needed for AutomaticAuthenticate

            // Choose an authentication type
            app.Map("/login", signinApp =>
            {
                signinApp.Run(async context =>
                {
                    var authType = context.Request.Query["authscheme"];
                    if (!string.IsNullOrEmpty(authType))
                    {
                        // By default the client will be redirect back to the URL that issued the challenge (/login?authtype=foo),
                        // send them to the home page instead (/).
                        await context.ChallengeAsync(authType, new AuthenticationProperties2() { RedirectUri = "/" });
                        return;
                    }

                    context.Response.ContentType = "text/html";
                    await context.Response.WriteAsync("<html><body>");
                    await context.Response.WriteAsync("Choose an authentication scheme: <br>");

                    //foreach (var type in context.Authentication.GetAuthenticationSchemes())
                    foreach (var type in context.RequestServices.GetRequiredService<IOptions<AuthenticationOptions2>>().Value.Schemes)
                    {
                        // TODO: display name?
                        await context.Response.WriteAsync("<a href=\"?authscheme=" + type.Name + "\">" + (type.Name ?? "(suppressed)") + "</a><br>");
                    }
                    await context.Response.WriteAsync("</body></html>");
                });
            });

            // Sign-out to remove the user cookie.
            app.Map("/logout", signoutApp =>
            {
                signoutApp.Run(async context =>
                {
                    context.Response.ContentType = "text/html";
                    await context.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
                    await context.Response.WriteAsync("<html><body>");
                    await context.Response.WriteAsync("You have been logged out. Goodbye " + context.User.Identity.Name + "<br>");
                    await context.Response.WriteAsync("<a href=\"/\">Home</a>");
                    await context.Response.WriteAsync("</body></html>");
                });
            });

            // Display the remote error
            app.Map("/error", errorApp =>
            {
                errorApp.Run(async context =>
                {
                    context.Response.ContentType = "text/html";
                    await context.Response.WriteAsync("<html><body>");
                    await context.Response.WriteAsync("An remote failure has occurred: " + context.Request.Query["FailureMessage"] + "<br>");
                    await context.Response.WriteAsync("<a href=\"/\">Home</a>");
                    await context.Response.WriteAsync("</body></html>");
                });
            });


            app.Run(async context =>
            {
                // CookieAuthenticationOptions.AutomaticAuthenticate = true (default) causes User to be set
                var user = context.User;

                // Display user information
                context.Response.ContentType = "text/html";
                await context.Response.WriteAsync("<html><body>");
                await context.Response.WriteAsync("Hello " + (context.User.Identity.Name ?? "anonymous") + "<br>");
                foreach (var claim in context.User.Claims)
                {
                    await context.Response.WriteAsync(claim.Type + ": " + claim.Value + "<br>");
                }

                await context.Response.WriteAsync("Tokens:<br>");
                
                //await context.Response.WriteAsync("Access Token: " + await context.Authentication.GetTokenAsync("access_token") + "<br>");
                //await context.Response.WriteAsync("Refresh Token: " + await context.Authentication.GetTokenAsync("refresh_token") + "<br>");
                //await context.Response.WriteAsync("Token Type: " + await context.Authentication.GetTokenAsync("token_type") + "<br>");
                //await context.Response.WriteAsync("expires_at: " + await context.Authentication.GetTokenAsync("expires_at") + "<br>");
                await context.Response.WriteAsync("<a href=\"/logout\">Logout</a><br>");
                await context.Response.WriteAsync("</body></html>");
            });
     }
}

[dnfclas]

Hi @HaoK, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[HaoK]

Other things to consider for later: (can we get rid of the accept logic in the Challenge/SignIn/SignOut) handlers with the new model do we still care about multiple handlers getting a chance at each scheme? Would drastically simplify the context's/manager logic if we could get rid of that stuff

[davidfowl]

W00t

[davidfowl]

Why do we still need this? Wouldn't it be a service now?

[HaoK]

We don't, vestige that was copied over and not deleted yet

[HaoK]

So I got the cookie sample working on top of the new stack... it became clear that the AuthenticationManager's job really is to maintain the per request cache that maps authenticationScheme to the worker instance per scheme.

There's also a problem with how AddCookies is sugar ontop of AuthenticationOptions, as it needs to add a transient CookieAuthenticationHandler for itself... but it doesn't have access to the services hanging where it is right now...

[HaoK]

You know, we could keep the auth APIs on HttpContext as extension methods, since to get rid of the IHttpContextAccessor we probably have to start taking HttpContext for all IAuthenticationManager Authenticate/Challenge/SignIn apis...

So we could go back to something like:

context.AuthenticateAsync("cookies") => context.RequestServices.GetRequiredService<IAuthenticationManager>().AuthenticateAsync(context, "cookies")

[davidfowl]

@HaoK in your example in the PR description, can you also show a simple middleware with the raw calls to ChallengeAsync and ForbidAsync

[davidfowl]

You know, we could keep the auth APIs on HttpContext as extension methods, since to get rid of the IHttpContextAccessor we probably have to start taking HttpContext for all IAuthenticationManager Authenticate/Challenge/SignIn apis...

That might be cleaner although the settable IAuthenticationManager is questionable. The feature also exposes the User property.

[HaoK]

Yeah I don't think we need the feature, just some extension methods to hang the manager APIs directly off HttpContext.

[davidfowl]

Yeah I don't think we need the feature, just some extension methods to hang the manager APIs directly off HttpContext.

You need to add more code to your samples:

public static class HttpAuthenticationExtensions
{
    public virtual async Task<AuthenticationTicket2> AuthenticateAsync(this HttpContext context, string authenticationScheme) 
    {
        return context.RequestServices.GetRequiredService<IAuthenticationManager>().AuthenticateAsync(context, authenticationScheme);
    }
}

[HaoK]

Updated the sample and the example at the top of the PR with a middleware that simulates basic login flow:

1. First request (no cookie) results in Challenge(Cookies) => Redirect to default login url of "/Account/login")
2. Go to /login to sign in a cookie
3. Return to base path to see "hello oldtimer"
4. Going to /forbid results in a 302 => AccessDeniedPath

[HaoK]

@davidfowl how do you feel about each auth scheme being a top level thing (that internally calls AddAuthentication with its own configuration). Only thing that irritates me is that we end up calling AddAuthentication a bunch of times, but they are TryAdd's so its safe... This lets the auth handlers properly add their handlers.

services.AddCookieAuthentication();
services.AddGoogleAuthentication(o => o.ClientId = "adlkjb");

[HaoK]

@PinpointTownes @brockallen take a look and see how this feels, I'm working on OAuth/Remote today now that cookies is functional

[brockallen]

I'm traveling this week. Won't be back until next week.

…

On Tue, Dec 13, 2016 at 9:36 PM Hao Kung ***@***.***> wrote: @PinpointTownes <https://github.com/PinpointTownes> @brockallen <https://github.com/brockallen> take a look and see how this feels, I'm working on OAuth/Remote today now that cookies is functional — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1065 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnpFK6uW_kgCqau5vkF-J0Lodx4k0iBks5rHwHjgaJpZM4LLAQP> .

[HaoK]

@davidfowl Okay so the remote middleware requires the ability to handle entire requests which cookies does not, so the next iteration will add back some kind of HandleRequest entry point where every scheme gets a chance to see if this is a request for them (otherwise let it continue)... So much for the AuthenticationMiddleware only setting httpContext.User. I'm now more open to your idea of making it a IStartupFilter, its pretty much mandatory for remote auth middleware

[HaoK]

@Tratcher any thoughts on any improvements we can make to the control flow logic we have today? Personally I hate the bool, so we should do something to replace:

        public virtual Task<bool> HandleRequestAsync()

We could use the Enum we have today I guess?

    public enum EventResultState
    {
        /// <summary>
        /// Continue with normal processing.
        /// </summary>
        Continue,

        /// <summary>
        /// Discontinue processing the request in the current middleware and pass control to the next one.
        /// </summary>
        Skipped,

        /// <summary>
        /// Discontinue all processing for this request.
        /// </summary>
        HandledResponse
    }

[Tratcher]

These defaults for auth and challenge scheme belong in Authorization, not Authentication.

[kevinchalet]

How is that an authorization concern? That would be great if we could avoid making the same mistake twice 😄

[Tratcher]

[Authorize] is the component that calls this and it can be configured via DefaultAuthPolicy, so I think this one is redundant. Same for DefaultChallengeScheme.

[kevinchalet]

The fact the authorization filters are currently responsible of determining the applicable scheme doesn't mean it's the right design nor really an authorization concern. In Web API 2 OWIN, the separation of concerns was much better/clearer, thanks to HostAuthenticationFilter/HostAuthenticationAttribute.

[HaoK]

Now is certainly a good time to revisit AuthZ if anything needs to move there, there's a few breaking changes I have on my todo list there, namely a similar change where bool AuthorizeAsync needs to become AuthorizationResult AuthorizeAsync similar to how we are beefing up HandleRequest

[Tratcher]

It's weird that Options isn't first class.

[HaoK]

Tradeoff vs generic hell, once I implement a few more auth handlers, I'll do a cleanup/rollup pass to see if we need anything at this layer but there's is an equivalent AuthenticationSchemeHandler<TOptions> that is similar to AuthenticationHandler<TOptions> today. The options don't need to be typed at the scheme builder layer, since these untyped options are only used by the scheme handler instance later, when it can just cast it to the type it knows. Its not related to the global singleton AuthenticationOptions which deals only with the various schemes and handler types.

[Tratcher]

remove IHttpContextAccessor?

[Tratcher]

We should look at moving the ChallengeBehavior.Automatic logic to Authorization.

[Tratcher]

This was going away right?

[HaoK]

Yeah its on the todo list, possibly we modify the signature of all of the methods to explicitly pass in an HttpContext instead.

[Tratcher]

It's weird that you resolve all of these Options on the first request rather than when the middleware is created. This may cause some weird race conditions.

[HaoK]

The handlers are initialized per request, as enabling DI activation is a goal now. So each auth options is not really like our IOptions instances, each is actually per named scheme instance, similar to what we have today with the UseXyzAuth(new XyzOptions()), and why I stuff it into the dictionary of each scheme... (RIP named options)

[kevinchalet]

I agree with @Tratcher. It's not only weird, it's dangerous (and potentially a perf' disaster if you receive thousands of requests when the app warms up, as there's no locking preventing InitializeAsync from being called multiple times).

There's also another BIG problem with this design approach: configuration errors cannot be detected at startup and won't prevent the app from loading successfully (so each time it will receive a new request, it will try to initialize the handlers... and fail miserably).

[Tratcher]

Perf: How can we avoid re-creating these handlers on every request? Is there some way we can make them stateless and store the relevant state in the HttpContext?

[HaoK]

Yeah if we end up trying to make this real, we need to spend some cycles on this part. The next iteration that's coming introduces a SchemeHandlerCache service, since both the single AuthMiddleware, and the manager need a central place to access the per request handler instances with all the state... The middleware needs the instances to do HandleRequest, while the manager needs them to handle the auth APIs. Does it make sense to split up these responsibilities at all, or do we like everything hanging off of IAuthenticationHandler?

[HaoK]

Specifically, I'm adding back some kind of Task<AuthenticationRequestResult> HandleRequest() back for the remote auth flows, does this make sense to live in IAuthenticationHandler still.

[kevinchalet]

@PinpointTownes @brockallen take a look and see how this feels.

Honestly, this sounds like an interesting approach, but I still have no idea what concrete problem(s) this huge API change is supposed to fix (I'm currently blocked by the fact the migration path will be extremely painful, without being sure the new design solves something that we can't fix in the existing architecture).

Curious: what's your plan for the host-provided authentication handlers?

[kevinchalet]

Okay so the remote middleware requires the ability to handle entire requests which cookies does not, so the next iteration will add back some kind of HandleRequest entry point where every scheme gets a chance to see if this is a request for them (otherwise let it continue)...

Please don't make that RemoteAuthenticationHandler-specific, it's also something ASOS relies on (heavily): https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Server/blob/dev/src/AspNet.Security.OpenIdConnect.Server/OpenIdConnectServerHandler.cs#L25-L140

[kevinchalet]

@HaoK note: I'm not 100% sure EventResultState is a good candidate for HandleRequestAsync, as the logic is really binary in this case (AuthenticationMiddleware.InvokeAsync invoke the rest of the pipeline depending on whether true or false was returned, but there's no "default logic" that can be skipped)

[davidfowl]

Honestly, this sounds like an interesting approach, but I still have no idea what concrete problem(s) this huge API change is supposed to fix (I'm currently blocked by the fact the migration path will be extremely painful, without being sure the new design solves something that we can't fix in the existing architecture).

Migration is painful, no denying that. Maybe we deprecate the old system and make a new system side by side. That would let 3rd party auth middleware gradually migrate.

The concrete things this solves:

• It brings authZ and authN closer together in pattern
• Less middleware to deal with, you configure a single middleware in the pipeline (or maybe a startup filter if that's a good idea) and you configure services to enable different auth schemes.
• Adding an authentication handler to the system isn't request specific as all state is passed to the handler via the contexts.
• The concept of active/passive auth middleware isn't even a thing (arguably, it could be solved in other ways).
• The overall design seems simpler (for now) and the layering looks better than what we currently have.

[HaoK]

@PinpointTownes don't worry the request handling path is coming back, I'm looking at unifying the request flow processing that we already have today in the handlers, with the top level HandleRequest in IAuthenticationHandler, it feels a little weird that they are inconsistent today, hopefully that will result in a single Result class rather than the 2, 3, or 4? we have today...

[HaoK]

@PinpointTownes I think the hosts can just follow the new pattern and add their own authentication handlers/schemes. IIS today doesn't look to be doing anything special, WebListener looks a bit different with the explicit schemes it handles like NTLM/Kerberos, but is there something that sticks out to you where the new pattern breaks down for the hosts?

   services.AddCookiesAuthentication("Cookies")
   services.AddIISIntegratedAuthentication("Windows");

[kevinchalet]

@davidfowl AFAICT, none of the points you list are really "issues" (they are more like general architecture concerns).

The concept of active/passive auth middleware isn't even a thing (arguably, it could be solved in other ways).

Of course it is, it's just handled differently: instead of being handled at the handler level (current design), it's done in AuthenticationMiddleware 😄

The overall design seems simpler (for now) and the layering looks better than what we currently have.

Sure, but for a reason: it doesn't support the same features set yet (e.g it doesn't seem to support "automatic challenge" nor converts raw 401 responses into "challenges").

but is there something that sticks out to you where the new pattern breaks down for the hosts?

@HaoK no no, I'm just curious to know how the layering issue will be solved (IIRC, the host-provided handlers directly implement IAuthenticationHandler because they can't depend on the Security stack).

[Tratcher]

@PinpointTownes We should consider removing the automatic 401 -> Challenge and require auth to be invoked explicitly. This made sense in Katana because challenges were always processed on the way out, but now they're processed immediately except for these 401s so you have two disconnected stages. This caused the original bug you complained about in #930.

[kevinchalet]

We should consider removing the automatic 401 -> Challenge and require auth to be invoked explicitly.

I'm fine with that, but this will be blocking for "pure OWIN" applications/frameworks (e.g like NancyFX), where the ASP.NET Core authentication infrastructure is not directly available. Returning a 401 status code was a convenient way to trigger a challenge.

[Tratcher]

True, but we could move that logic to the owin adapter, or provide something similar in the adapter.

[HaoK]

I'd prefer a clean break but I understand why it might not be a good idea to break the world. That's a lot of customer pain to deal with. Hence these ugly looking changes.

[Tratcher]

At least with obsolete extensions we can give the developer direct feedback about what changed. If we just remove the extensions then it fails to compile and they have no idea why. We may not even have to implement the extensions, just make them throw :-)

[kevinchalet]

If we just remove the extensions then it fails to compile and they have no idea why.

I'm pretty sure that's what migration guides are for

[HaoK]

Cleaned things up a bit, pushed the goop into app.UseLegacyAuthentication(AuthenticationSchemeOptions, Func<HttpContext, IAuthenticationSchemeHandler>) which does a pretty good job sweeping this underneath, all google tests appear unaffected by this change, so I think as far as the new stack goes, either way of registration should behave the same.

[HaoK]

Mixing old and new patterns of registration also don't matter since order no longer has any real meaning to the schemes...

[leastprivilege]

+1 for making breaking changes in v2. People need to understand that this API works differently.

[Obsolete] is not good enough IMO

[Eilon]

@PinpointTownes @leastprivilege thanks for keeping an eye on this PR and staying involved. Honestly, we're still at the point where we're playing with a bunch of different ideas. I mean, wouldn't it be great if we could preserve compatibility as much as possible and have the new features that this design enables? (And eliminate as many problems as possible with the old design?)

Ok, yeah, that's quite optimistic, but that's part of what this PR is about! We've had some design meetings within the team to see what we can come up with.

I agree it could be worse if we make everything look compatible but it behaves very differently, potentially introducing serious app bugs.

But we have to balance that with breaking dozens of NuGet packages (ahem, yours!), and countless apps that use the old system.

And then perhaps the hardest problem of it all: breaking peoples' understanding of how this stuff works!

So, we're not quite sure how this will all end up, but we love what the new design brings, and we've been reading all your reactions to it and trying to account for as much of it as possible.

I hope that once we end up on a design that we like, we'll be able to make it clear that we have an official proposal, and will of course seek a round of feedback on that (probably in a new PR, so that we can start the discussion fresh).

[kevinchalet]

I agree it could be worse if we make everything look compatible but it behaves very differently, potentially introducing serious app bugs.

Unfortunately, I have the weird feeling that's where this PR is currently heading to.

I haven't played with the latest commits yet, but I'm sure there are also security concerns when using authentication middleware with branching. E.g, in this snippet, won't the cookie middleware execute even for non-API calls? (since it will register the cookie handler regardless of the branching and execute it via the authentication middleware registered by app.UseJwtBearerAuthentication())

app.UseJwtBearerAuthentication(...);

app.UseWhen(context => context.Request.Path != "/api", map => {
    map.UseCookieAuthentication();
});

app.UseMvc();

But we have to balance that with breaking dozens of NuGet packages (ahem, yours!), and countless apps that use the old system.

Existing packages targeting ASP.NET Core 1.x will most likely break due to massive changes in .NET Core 2.0 anyway (see aspnet/SignalR#155 (comment) for a concrete case), so if an important design change must be adopted, it's the perfect moment.

So, we're not quite sure how this will all end up, but we love what the new design brings, and we've been reading all your reactions to it and trying to account for as much of it as possible.

That would be great if you could list the concrete things "the new design brings", 'cause it's not clear to me yet.

[HaoK]

Map is an interesting one, and its more or less gone for Auth in the new design. But that example is another instance of bearer/cookies conflicting and a necessary workaround from the old middleware not playing nicely together. In the new stack, there is only a tiny bit of overlap, where each handler is given a chance to handle the request, but there's no automatic challenge like conflict that you need to resolve via conditional existence of schemes. Each scheme is part of the authentication service, and since we don't have support for different services per branch, every scheme will appear in every branch. This isn't that different today from AuthZ, where there's no way to limit policies to a specific branch either, and since policies can reference schemes, having schemes only exist in some branches is a bit weird anyways.

[kevinchalet]

Map is an interesting one, and its more or less gone for Auth in the new design.

Not really gone if you decide to keep your "legacy authentication" extensions

But that example is another instance of bearer/cookies conflicting and a necessary workaround from the old middleware not playing nicely together.

I'm not sure I would consider that as a workaround in this particular case, as it's super common to create subsections in your apps, that can possibly use different schemes.

In the new stack, there is only a tiny bit of overlap, where each handler is given a chance to handle the request, but there's no automatic challenge like conflict that you need to resolve via conditional existence of schemes.

The problem with the sample I mentioned is not with the automatic challenge part, but with automatic authentication: since the cookie handler will be called despite the path restriction, it will populate HttpContext.User with the identity corresponding to the authentication cookie. Obviously, your legacy authentication extensions introduce a huge XSRF risk.

[HaoK]

UseLegacyAuthentication as it stands doesn't really preserve the old behavior (and its not really trying to, its trying to implement the old API behavior using the new stack as best as it can), its just adding the new authentication middleware (if one hasn't been added already), and any schemes added by one branch will be available to all branches as soon as its run once since the service is shared.

[HaoK]

Updated first comment at the top with a cleaned up up to date summary, including a section about some of the things we like about the new design (also my main focus was on fixing things that were existing issues(claims transformation, per request events, using db contexts) and also the things that were hampering our ability to make improvements, aka RIP automatic)

[HaoK]

Made ISystemClock a service since this is now easy (unlike when I tried to do this before)

[HaoK]

Summary of next iteration plan after meeting with @davidfowl @Tratcher @Eilon:

• Abandoning interop via UseLegacy, we won't touch the existing HttpAbstractions/Authentication manager, so old 1.x apps and auth will still work the same
• Add new Auth abstractions package in Http (i.e. Microsoft.Extensions.Authentication.Abstractions?) with the interfaces like IAuthenticationService, IAuthenticationSchemeHandler/Provider etc.
• Take over the existing Security namespaces in a breaking way
• Nuke DefaultChallengeScheme (no naked 401 support)
• Look into ProcessRequest improvements, maybe use routing?

[kevinchalet]

Abandoning interop via UseLegacy, we won't touch the existing HttpAbstractions/Authentication manager, so old 1.x apps and auth will still work the same

How will that work in practice? Will "legacy" middleware be visible using the new HttpContext extensions? E.g will the Identity login template be able to list (and use) legacy social providers?

If the two worlds can't work together, it would be way better to remove the old APIs.

Nuke DefaultChallengeScheme (no naked 401 support)

👍 for removing the "naked 401" handling, but if you remove DefaultChallengeScheme, this means you'll have to remove the parameterless ChallengeAsync overload, which will impact the entire stack (i.e no more parameterless [Authorize] attribute).

[HaoK]

We can keep the DefaultChallengeScheme, I was just talking about killing the naked 401 logic that is in the new auth middleware.

For interop basically the only thing we will support is existing 1.x packages continuing to work, so old identity +with old 1.x providers using the old APIs will continue to work. So in practice, this just means we are preserving the HttpAuthentication interfaces, so old httpContext.Authentication APIs with the old 1.x packages and providers and middleware will continue to do what they always did. The new stack won't affect that at all since the new IAuthenticationService doesn't depend on the old HttpAbstractions IAuthenticationManager at all. Any kind of interop between the two will be left as a choose your own adventure exercise :)

But basically old 1.x providers will continue to work in MVC2.0 side by side with the new stack, but upgrading to 2.0 will require moving to the new stack, and all the old middleware/interfaces will be deleted/updated.

I'm doing some of the incremental cleanup from the review in this PR, but I'll start a new one when I takeover the existing classes/namespaces in the next iteration.

[HaoK]

Regarding identity in 2.0, and everything else, it will be updated to only use the new stack (i.e. enumerating only the authentication schemes by default). The old auth will still be there if you registered them, but identity, and MVC for that matter won't pick them up. For MVC/AuthorizeFilter there was some brief discussion about how we would add something like LegacyAuthFilter that you could plug in to restore talking to the old context.Authentication.Xyz. But [Authorize] would target the new auth system only by default.

[kevinchalet]

So it will be pretty much like using Katana authentication middleware on ASP.NET Core... totally unusable in practice? If even authorization doesn't work with the legacy middleware, there's really no point keeping them in 2.0.

[HaoK]

What do you mean about 'keeping them in 2.0'? The plan is to nuke all the old code in Security as part of 2.0. But you can still use old 1.1 packages in 2.0 apps, and that scenario should still work. (where they use the old AuthenticationManager on context to communicate with each other) which would still exist and behave the same..

[kevinchalet]

What do you mean about 'keeping them in 2.0'? The plan is to nuke all the old code in 2.0.

Ah great. This point wasn't clear to me

But you can still use old 1.1 packages in 2.0 apps, and that scenario should still work.

Hum, are you sure about that? The major version increment means things won't be guaranteed to be binary compatible. See aspnet/SignalR#155 (comment) for a concrete case.

[HaoK]

Yeah I believe the plan is for most to be binary compatible, but basically the interop should be limited to preserving the old HttpAbstractions authentication functionality. But 2.0 Security/IIS/WebListener will depend on a new Authentication.Abstractions package which will be in the HttpAbstractions repo but in the Authentication namespace and contain all the new interfaces/extension methods. We can choose to keep the existing names if we want now (IAuthenticationHandler vs IAuthenticationSchemeHandler etc... I'll start the new PR tomorrow and we can revisit naming there

[HaoK]

This PR is effectively abandoned now, new iteration PR is here: #1113