Authentication 2.0 Iteration 3 (with Names/OptionFactory) #1151

HaoK · 2017-03-15T22:26:30Z

New IOptionsFactory which provides named options in addition to validation, both of which are triggered via IOptionsFactory.Get("name"). Allows normal options configuration for schemes, Validation is just a different set of Actions that run after the Configures

IOptionsFactory Usage:

  services.ConfigureAll<GoogleOptions>(o => o.SignInScheme = "GoogleCookie");
  services.Configure<GoogleOptions>("Google", o => ClientId = "avsdlkj");
  services.Configure<GoogleOptions>("Google2" p => ClientId = "two");
  services.ValidateAll<GoogleOptions>(o => if (o.ClientId == null) throw new ArgumentException());

  optionsFactory.Get<GoogleOptions>("Google"); // OK
  optionsFactory.Get<GoogleOptions>("Default"); // throws ArgumentException

split HandleRequest into its own IAuthenticationRequestHandler interface instead, CallbackPaths routing is disabled for now (all IAuthenticationRequestHandler schemes are given a chance to handle the request)

TL:DR Auth 2.0 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' 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/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);
        }
}

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.

dnfclas · 2017-03-15T22:26:34Z

@HaoK,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

HaoK · 2017-03-16T00:27:13Z

@brockallen @PinpointTownes we've decided today that we are going ahead with this change, the implementation details are still subject to change, but the high level switch from middleware to services/DI/NamedOptions is a go. Definitely let us know if any of your important scenarios are affected, or if you have any ideas for quality of life improvements in the base auth handler classes, since the entire auth code base is opened up for surgery right now...

HaoK · 2017-03-16T00:27:57Z

Note to self: need to investigate adding schemes and configuring them outside of ConfigureServices. We can probably cheat and use the AuthenticationScheme.Settings bag to stuff options instances for this scenario

kevinchalet · 2017-03-16T20:31:57Z

@brockallen @PinpointTownes we've decided today that we are going ahead with this change, the implementation details are still subject to change, but the high level switch from middleware to services/DI/NamedOptions is a go.

Thanks for the info, I'll take a look ASAP. When do you plan to merge this PR?

HaoK · 2017-03-16T22:05:40Z

No firm ETA on merge, need to start working on the changes in all the other repos (Identity/servers), which likely will require some more changes here. But we can start settling on implementation choices now. Also the options changes will go in first, so I'd start there if you are interested on how that lands: aspnet/Options#176

kevinchalet · 2017-03-16T22:22:33Z

src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs

+                SignOutScheme = SignInScheme;
+            }
+
+            if (Options.StateDataFormat == null)


@HaoK correct me if I'm wrong, but Options is a shared instance, right? (at least, when using the default options factory). So this means that the options could be modified concurrently, as InitializeAsync is invoked per request. Isn't that risky as hell?

Yeah so that's why I'm trying to move all the Options manipulations into 'validations' that occur as part of OptionsFactory.Get("schemeName")

We can also switch to using the form of 'scoped' options for the schemes, IOptionsSnapshot for example which wouldn't be shared.

Makes sense. Thanks for the clarification.

Now that I think about it, that's probably the easiest fix to this issue since the scheme options don't want to share

kevinchalet · 2017-03-16T22:31:50Z

src/Microsoft.AspNetCore.Authentication.Impl/AuthenticationHandler.cs

+        /// The handler calls methods on the events which give the application control at certain points where processing is occurring. 
+        /// If it is not provided a default instance is supplied which does nothing when the methods are called.
+        /// </summary>
+        protected virtual object Events { get; set; }


How bad would it be if AuthenticationHandler had a TEvents generic argument?

But yeah the events stuff needs some love, I guess every handler does have an events class today.

The events is a bit weird to live on options, maybe we can move it into the AuthenticationScheme instead, as something that gets fed into Intialize(IOptionsFactory<TOptions>, TEvent events, HttpContext)

I've always felt configuring the events as part of the options makes it look ginormous. Can we do better if we tease it apart/have a new pattern for hooking events that's not tied to options configuration?

Are you sure every event isn't overridable? Isn't that the only purpose for the IXyzEvents to exist? basically its the list of virtual methods that would exist if you could plug in your own handler.

Are you sure every event isn't overridable?

The events are all overridable, but not the infrastructure code that call them: the OAuthHandler.CreateTicketAsync method you used in your sample is quite an exception, as it's the only handler that was really designed to be subclassed.

So moving to a virtual method would allow us to nuke all of the context classes, and just pass them as parameters (or let them access handler directly since its now in the handler, which they couldn't even do before).

I personally like the fact the public hooks are separated from the rest of the infrastructure code, so I'd really love this events pattern to be present in 2.0.0, but heh, your PR, your choice 😄

(also, I'm not a huge fan of having to override a class to just to do something that can be done in a 2-line event delegate)

Fair enough, @Tratcher had the same opinion, there's so many other things on my list right now, so its most likely not going to change soon

Tratcher · 2017-03-17T22:18:29Z

src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticationScheme.cs

+
+        // Holds things like the configured options instances for the handler
+        // Also replacement for AuthenticationDescription
+        public IReadOnlyDictionary<string, object> Settings { get; }


Still a weird property bag

Yeah this can go away now, since we can add/remove options dynamically I don't think this is needed at all. Fleshing out the add/remove auth scheme using custom options in the next update

Tratcher · 2017-03-17T22:18:39Z

src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticationScheme.cs

+                throw new ArgumentException("handlerType must implement IAuthenticationSchemeHandler.");
+            }
+            HandlerType = handlerType ?? throw new ArgumentNullException(nameof(handlerType));
+            Settings = settings ?? throw new ArgumentNullException(nameof(settings));


Tratcher · 2017-03-17T22:18:59Z

src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticationSchemeBuilder.cs

+        public Type HandlerType { get; set; }
+
+        // Holds things like the configured options instances for the handler
+        public Dictionary<string, object> Settings { get; set; } = new Dictionary<string, object>(); // casing?


Tratcher · 2017-03-17T22:19:22Z

src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticationTicket.cs

            AuthenticationScheme = authenticationScheme;
-            Principal = principal;
+            Principal = principal ?? throw new ArgumentNullException(nameof(principal));


Tratcher · 2017-03-17T22:23:12Z

src/Microsoft.AspNetCore.Authentication.Abstractions/IAuthenticationHandler.cs

+        Task SignOutAsync(SignOutContext context);
+    }
+
+    public interface IAuthenticationRequestHandler : IAuthenticationHandler


Tratcher · 2017-03-17T22:41:45Z

src/Microsoft.AspNetCore.Authentication.Abstractions/IAuthenticationSchemeProvider.cs

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public interface IAuthenticationSchemeProvider


Where did the implementation go?

I nuked the old Authentication package and moved Impl into there

It's not in this PR anywhere.

HaoK · 2017-03-20T21:08:09Z

Updated with some cleanup, nuked settings bag on AuthenticationScheme, RIP ?? throw

HaoK · 2017-03-20T21:12:15Z

So in terms of final homes:

Authentication.Abstractions => Http Repo (keeps its own package)
Should we split Authentication into Authentication.Core (lives in Http), and Authentication(.Security)? Basically the core interfaces/implementations for AddAuthentication live in Core, and the base class stuff we use for the Security auth packages would stay in this repo...

So...
Authentication.Abstractions (HttpAbstractions)
Authentication.Core (HttpAbstractions)
Authentication (Security)

Tratcher · 2017-03-20T22:11:17Z

Which repos need to depend on Authentication.Core?

Tratcher · 2017-03-20T22:26:47Z

samples/SocialSample/Startup.cs

+            loggerfactory.AddConsole(LogLevel.Information);
+
+            // Simple error page to avoid a repo dependency.
+            app.Use(async (context, next) =>


Can we just have Security depend on Diagnostics so we can stop doing this stuff? I don't think it creates any cycles, it just bumps security up the chart one level.

Seems fine to me, I'll add it

Tratcher · 2017-03-20T22:27:56Z

samples/SocialSample/Startup.cs

+
+            app.UseAuthentication();
+
+            if (string.IsNullOrEmpty(Configuration["facebook:appid"]))


Move this check to ConfigureServices

Tratcher · 2017-03-20T22:34:02Z

src/Microsoft.AspNetCore.Authentication.Abstractions/IAuthenticationSchemeProvider.cs

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public interface IAuthenticationSchemeProvider


It's not in this PR anywhere.

Tratcher · 2017-03-20T22:38:08Z

src/Microsoft.AspNetCore.Authentication.Cookies/CookieAppBuilderExtensions.cs

-        /// <param name="app">The <see cref="IApplicationBuilder"/> to add the middleware to.</param>
-        /// <param name="options">A <see cref="CookieAuthenticationOptions"/> that specifies options for the middleware.</param>
-        /// <returns>A reference to this instance after the operation has completed.</returns>
-        public static IApplicationBuilder UseCookieAuthentication(this IApplicationBuilder app, CookieAuthenticationOptions options)


Bring these back as Obsolete Error with a fwlink.

Tratcher · 2017-03-20T22:45:05Z

src/Microsoft.AspNetCore.Authentication.Abstractions/SignInContext.cs

+        public SignInContext(HttpContext context, string authenticationScheme, ClaimsPrincipal principal, AuthenticationProperties properties)
+            : base(context, authenticationScheme, properties)
+        {
+            Principal = principal ?? throw new ArgumentNullException(nameof(principal));


Tratcher · 2017-03-20T22:58:06Z

src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationOptions.cs

@@ -38,25 +56,20 @@ public class RemoteAuthenticationOptions : AuthenticationOptions
        /// Gets or sets the authentication scheme corresponding to the middleware
        /// responsible of persisting user's identity after a successful authentication.
        /// This value typically corresponds to a cookie middleware registered in the Startup class.
-        /// When omitted, <see cref="SharedAuthenticationOptions.SignInScheme"/> is used as a fallback value.
+        /// When omitted, <see cref="SignInScheme"/> is used as a fallback value.


This comment is now referencing itself?

HaoK · 2017-03-21T04:34:19Z

src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticationScheme.cs

 using System.Reflection;
-using Microsoft.AspNetCore.Http;



@Tratcher they are here, but perhaps hard to find in the 178 file diff... maybe easier to just view via commits

HaoK · 2017-03-21T04:37:03Z

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs

@@ -71,8 +71,17 @@ protected AuthenticationHandler(IOptionsService<TOptions> optionsFactory, ILogge
        /// <returns></returns>
        public virtual Task InitializeAsync(AuthenticationScheme scheme, HttpContext context)
        {
-            Scheme = scheme ?? throw new ArgumentNullException(nameof(scheme));
-            Context = context ?? throw new ArgumentNullException(nameof(context));
+            if (scheme == null)


@Tratcher See AuthenticationHandler here

HaoK · 2017-03-21T19:55:26Z

@Tratcher I'll break this PR up into 2 new PRs to make things more managable, I'll start the PR for Abstractions/Core in HttpAbstractions and open a new one for only the Security changes

HaoK · 2017-03-23T19:08:03Z

Updated removing the stuff that is being moved to HttpAbstractions via: aspnet/HttpAbstractions#798

Hopefully easier to review

kevinchalet · 2017-03-23T19:14:33Z

@HaoK hum, is there a particular reason to move so many types to HttpAbstractions? Of course, IAuthenticationHandler and the types used to register the handler in the DI need to live there for layering reasons, but is it really useful to move things like AuthenticationMiddleware there?

Tratcher · 2017-03-23T19:16:44Z

@PinpointTownes I think they're needed to write tests for HttpSysServer and IISIntegration

kevinchalet · 2017-03-23T19:25:19Z

AuthenticationMiddleware's role is extremely limited: it simply adds a special feature to track the original paths, calls IAuthenticationSchemeProvider to get the default sign-in scheme (which is kinda the equivalent of "active authentication") and calls handler.HandleRequestAsync() for "promiscuous handlers". AFAICT, none of the HttpSysServer/IISIntegration tests couldn't live without this type. Do you have any specific example in mind?

HaoK · 2017-03-23T19:36:50Z

The middleware isn't critical, I can move that back along with UseAuthentication to Security. What's critical is the core services/AddAuthenticationCore as you can't even use Auth without those. I'll move the Middleware/Use out now

HaoK · 2017-03-23T20:13:39Z

Updated, UseXyz's are back as obsolete/throw not Supported. Also brought back the AuthMiddleware from HttpAbstractions.

Tratcher · 2017-03-23T20:52:08Z

src/Microsoft.AspNetCore.Authentication.Cookies/CookieAppBuilderExtensions.cs

@@ -12,26 +12,26 @@ namespace Microsoft.AspNetCore.Builder
    public static class CookieAppBuilderExtensions
    {
        /// <summary>
-        /// Obsolete, see TODO: fwdlink
+        /// Obsolete, see https://go.microsoft.com/fwlink/?linkid=845470
        /// </summary>
        /// <param name="app">The <see cref="IApplicationBuilder"/> to add the middleware to.</param>
        /// <returns>A reference to this instance after the operation has completed.</returns>
        [Obsolete]


Put the link in the Obsolete comment

Tratcher · 2017-03-23T21:03:52Z

src/Microsoft.AspNetCore.Authentication.Cookies/CookieAppBuilderExtensions.cs

@@ -16,7 +16,7 @@ public static class CookieAppBuilderExtensions
        /// </summary>
        /// <param name="app">The <see cref="IApplicationBuilder"/> to add the middleware to.</param>
        /// <returns>A reference to this instance after the operation has completed.</returns>
-        [Obsolete]
+        [Obsolete("See https://go.microsoft.com/fwlink/?linkid=845470")]


Hmm, it's only a warning by default, you have to set IsError true
https://msdn.microsoft.com/en-us/library/system.obsoleteattribute.iserror(v=vs.110).aspx

HaoK · 2017-03-31T19:41:57Z

Replaced with #1170

dnfclas added the cla-already-signed label Mar 15, 2017

This was referenced Mar 15, 2017

Auth 2.0 using OptionsFactory (named options) #1144

Closed

Auth 2.0 Iteration 2 #1113

Closed

kevinchalet reviewed Mar 16, 2017

View reviewed changes

Tratcher reviewed Mar 17, 2017

View reviewed changes

Tratcher reviewed Mar 20, 2017

View reviewed changes

HaoK commented Mar 21, 2017

View reviewed changes

HaoK mentioned this pull request Mar 21, 2017

Auth 2.0: Add Auth Abstractions, Core aspnet/HttpAbstractions#798

Closed

Tratcher reviewed Mar 23, 2017

View reviewed changes

HaoK added 3 commits March 29, 2017 15:41

First cut of Authentication.Abstractions

85c061b

Add impl package

7e6ee64

Cleanup

5ca7293

HaoK added 23 commits March 29, 2017 15:54

Add options factory tests

4d76bc1

Add default behavior

520116a

Update sln

a1400ad

Fix merge issues

d86b0d0

Drop Default prefix

3d0dbcf

PR feedback + fix SocialSample

4b7ad77

Remove clears

2d6de68

Move IOptionsFactory to Options repo

d3b0aca

React to options rename

2e4e176

Takeover Authentication (nuke .Impl)

2fca4b2

Cleanup

633f187

Move stuff into HttpAbstractions

47eda07

Cleanup

80a32ad

Refactor into HttpAbstracionts: Core/Abstractions

2555eda

PR cleanup

35659d3

Cleanup

5cec0e1

Add back UseXyz as obslete/throw

59ced83

Add google back

75d757d

Update to real fwlink

523548d

Update obsolete to include fwlink

5650eec

Update obsolete to error

1397e05

Fix formatting

da921ba

React to options

7804858

HaoK force-pushed the haok/named branch from 4a0423d to 7804858 Compare March 31, 2017 18:38

Update to latest options/http

06871dd

HaoK mentioned this pull request Mar 31, 2017

Iteration 4 - Using Auth 2.0/Options 2.0 packages #1170

Closed

HaoK closed this Mar 31, 2017

HaoK mentioned this pull request Apr 22, 2017

The Grand Auth Redesign of 2017 #1179

Closed

13 tasks

HaoK deleted the haok/named branch August 7, 2017 16:59


		app.UseAuthentication();

		if (string.IsNullOrEmpty(Configuration["facebook:appid"]))

Authentication 2.0 Iteration 3 (with Names/OptionFactory) #1151

Authentication 2.0 Iteration 3 (with Names/OptionFactory) #1151

Conversation

HaoK commented Mar 15, 2017

TL:DR Auth 2.0 summary

Pros of the new design

Example of using scoped DbContext

Claims Transformation

dnfclas commented Mar 15, 2017

HaoK commented Mar 16, 2017

HaoK commented Mar 16, 2017 • edited Loading

kevinchalet commented Mar 16, 2017

HaoK commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 20, 2017

HaoK commented Mar 20, 2017

Tratcher commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 21, 2017

HaoK commented Mar 23, 2017

kevinchalet commented Mar 23, 2017

Tratcher commented Mar 23, 2017

kevinchalet commented Mar 23, 2017

HaoK commented Mar 23, 2017

HaoK commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 31, 2017

HaoK commented Mar 16, 2017 •

edited

Loading

HaoK Mar 21, 2017 •

edited

Loading