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

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

Previous PR: #1151

@Tratcher clean PR to start with, rebased and using the Http Abstractions/Options 2.0 interfaces

dnfclas · 2017-03-31T19:41:35Z

@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

kevinchalet · 2017-03-31T20:30:26Z

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

+                        throw new InvalidOperationException("The MetadataAddress or Authority must use HTTPS unless disabled for development by setting RequireHttpsMetadata=false.");
+                    }
+
+                    Options.ConfigurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(Options.MetadataAddress, new OpenIdConnectConfigurationRetriever(),


@HaoK hum, I thought all the initialization stuff was supposed to be done in the options?
'cause AFAICT, this iteration still suffers from the same concurrency issues as the previous PRs 😅

Whoops, I forgot we needed validation/fixup in options for that reason, well good thing you reminded me why I need to push for that feature :)

HaoK · 2017-04-13T22:16:44Z

Updated PR with some refactoring/cleaning up around Options/Events initialization.

@Tratcher I think this has everything we need for the initial iteration (other than the AddXyz() empty overloads that use Config from DI, I only added that to Google so far)

kevinchalet · 2017-04-13T22:37:54Z

src/Microsoft.AspNetCore.Authentication/AuthenticationSchemeOptions.cs

+        /// <summary>
+        /// Used to ensure that the options are only initialized once.
+        /// </summary>
+        public bool Initialized { get; set; }


Yurk 😲

Is there a particular reason this initialization logic is not something handled directly by the options stack?

Well auth is doing something a bit different than normal options usages, as handlers are doing fixup after all the Configures have been run. If we bring back some kind of lifetime/second validation pass we could do it in options directly, but for now, we have to roll our own...

I guess it would be cleaner (and probably safer?) if options was responsible of the entire initialization process, 'cause that's likely the most appropriate place to determine whether options are singleton (= initialization should happen only once) or scoped (= initialization should happen for every instance).

I took a look at the options changes, and there's indeed nothing (yet) that could help with that. Have you considered introducing an IOptionsValidator<T>/IOptionsInitializerT> that would be run after all the IConfigureOptions<T>? (or you could bring back ordered setups).

Yeah Validator/Intializer was there in the origional options PR but cut due to not wanting to rush that feature into options, we still have time to revisit where responsibility for single intialization goes before we ship, but for this initial checkin, its going to live in security

kevinchalet · 2017-04-13T22:38:06Z

src/Microsoft.AspNetCore.Authentication/AuthenticationSchemeOptions.cs

+        /// <summary>
+        /// Used to prevent concurrent access during intialization.
+        /// </summary>
+        public object InitalizeLock { get; } = new object();


nit: typo 😄

Tratcher · 2017-04-14T14:52:04Z

Security.sln

@@ -54,286 +52,436 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenIdConnect.AzureAdSample
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.ChunkingCookieManager.Sources.Test", "test\Microsoft.AspNetCore.ChunkingCookieManager.Sources.Test\Microsoft.AspNetCore.ChunkingCookieManager.Sources.Test.csproj", "{51563775-C659-4907-9BAF-9995BAB87D01}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Authentication", "src\Microsoft.AspNetCore.Authentication\Microsoft.AspNetCore.Authentication.csproj", "{BC0D4B56-1A5B-4D88-AFBF-68C0F2D545FB}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Options", "..\Options\src\Microsoft.Extensions.Options\Microsoft.Extensions.Options.csproj", "{151B7C1C-8E47-48CC-B04C-7414001D0F7B}"


isn't the options work done? What did you tweak from dataprotection?

Yeah I forgot to push the updated sln, doing that now

Tratcher · 2017-04-14T14:54:23Z

samples/JwtBearerSample/Startup.cs

                if (user?.Identity?.IsAuthenticated ?? false)
                {
                    await next();
                }
                else
                {
-                    // We can do this because of options.AutomaticChallenge = true;
-                    await context.Authentication.ChallengeAsync();
+                    await context.ChallengeAsync(JwtBearerDefaults.AuthenticationScheme);


ChallengeAsync()

Tratcher · 2017-04-14T14:57:45Z

samples/OpenIdConnect.AzureAdSample/Startup.cs

            services.AddAuthentication(sharedOptions =>
-                sharedOptions.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme);
+                sharedOptions.DefaultAuthenticationScheme = CookieAuthenticationDefaults.AuthenticationScheme);


also DefaultSignInScheme and OIDC as the DefaultChallengeScheme.

Tratcher · 2017-04-14T15:00:42Z

samples/OpenIdConnectSample/Startup.cs

-                sharedOptions.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme);
+            {
+                sharedOptions.DefaultAuthenticationScheme = CookieAuthenticationDefaults.AuthenticationScheme;
+                sharedOptions.DefaultSignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;


OIDC as DefaultChallengeScheme

Tratcher · 2017-04-14T15:01:30Z

samples/OpenIdConnectSample/Startup.cs

@@ -158,7 +164,7 @@ await context.Authentication.SignOutAsync(OpenIdConnectDefaults.AuthenticationSc
                {
                    // This is what [Authorize] calls
                    // The cookie middleware will intercept this 401 and redirect to /login
-                    await context.Authentication.ChallengeAsync();
+                    await context.ChallengeAsync(CookieAuthenticationDefaults.AuthenticationScheme);


ChallengeAsync()
This comment is wrong, this sample has no /login. It may have been copied from SocialAuth. This sample should challenge OIDC implicitly.

Tratcher · 2017-04-14T15:11:36Z

samples/SocialSample/Startup.cs


                // Deny anonymous request beyond this point.
                if (user == null || !user.Identities.Any(identity => identity.IsAuthenticated))
                {
                    // This is what [Authorize] calls
                    // The cookie middleware will intercept this 401 and redirect to /login
-                    await context.Authentication.ChallengeAsync();
+                    await context.ChallengeAsync(CookieAuthenticationDefaults.AuthenticationScheme);


ChallengeAsync()

Tratcher · 2017-04-14T15:12:37Z

samples/SocialSample/Startup.cs

-                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("Access Token: " + await context.GetTokenAsync(CookieAuthenticationDefaults.AuthenticationScheme, "access_token") + "<br>");


Why do you need the auth scheme here? Can't it use DefaultAuthScheme?

That overload is gone right now, could add it back if we really want.

Yeah, this is clunky

Tratcher · 2017-04-14T15:17:49Z

src/Microsoft.AspNetCore.Authentication/baseline.net45.json

@@ -2474,7 +2474,7 @@
        },
        {
          "Kind": "Method",
-          "Name": "SkipToNextMiddleware",
+          "Name": "StopProcessing",


revert the baseline files, disable APICheck, and file a followup bug

Tratcher · 2017-04-14T15:21:19Z

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs

-    /// </summary>
-    /// <typeparam name="TOptions">Specifies which type for of AuthenticationOptions property</typeparam>
-    public abstract class AuthenticationHandler<TOptions> : IAuthenticationHandler where TOptions : AuthenticationOptions
+namespace Microsoft.AspNetCore.Authentication {


nit: new lines

Tratcher · 2017-04-14T15:24:18Z

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs

-
-        protected UrlEncoder UrlEncoder { get; private set; }
-
-        public IAuthenticationHandler PriorHandler { get; set; }


Should we leave all of the public/protected properties as Obsolete Error with the fwlink?

Easier to diff if you revert the order change

I don't think its worth preserving things in the Handler just for obsolete, since they will see the error already from all of the UseXyzs

Those are different audiences. These would be for implementors, not consumers.

HaoK · 2017-04-14T20:31:34Z

Updated samples feedback, I'm not sure I love DefaultAuthenticationScheme, I thought I had named it DefaultAuthenticateScheme, which maps more closely to what it means... thoughts?

HaoK · 2017-04-14T20:32:55Z

Ah I see what happened, its GetDefaultAuthenticateSchemeAsync in the real interface IAuthenticationSchemeProvider, I just forgot to rename the property on the options. So we need to be consistent one way or the other. I'll submit a PR in HTTP

Tratcher · 2017-04-14T21:02:53Z

samples/SocialSample/Startup.cs

                var user = context.User;

                // This is what [Authorize] calls
-                // var user = await context.AuthenticateAsync(AuthenticationManager.AutomaticScheme);
+                // var user = await context.AuthenticateAsync(DefaultAuthenticateScheme);


Is there AuthenticateAsync() that resolves the default from the service?

Tratcher · 2017-04-14T21:04:32Z

samples/OpenIdConnectSample/Startup.cs

                var user = context.User;

                // This is what [Authorize] calls
-                // var user = await context.Authentication.AuthenticateAsync(AuthenticationManager.AutomaticScheme);
+                // var user = await context.AuthenticateAsync(AuthenticationManager.AutomaticScheme);


AutomaticScheme.
AuthenticateAsync()

Tratcher · 2017-04-14T21:05:20Z

Security.sln

@@ -54,286 +52,382 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenIdConnect.AzureAdSample
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.ChunkingCookieManager.Sources.Test", "test\Microsoft.AspNetCore.ChunkingCookieManager.Sources.Test\Microsoft.AspNetCore.ChunkingCookieManager.Sources.Test.csproj", "{51563775-C659-4907-9BAF-9995BAB87D01}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Authentication", "src\Microsoft.AspNetCore.Authentication\Microsoft.AspNetCore.Authentication.csproj", "{BC0D4B56-1A5B-4D88-AFBF-68C0F2D545FB}"


Tratcher · 2017-04-14T22:00:24Z

samples/OpenIdConnect.AzureAdSample/Startup.cs

+                };
+            });
+
+            services.AddAuthentication(sharedOptions =>


Let's put these defaults at the top of each sample so they don't get lost under all of the options noise.

Tratcher · 2017-04-14T22:00:39Z

samples/OpenIdConnect.AzureAdSample/Startup.cs

+
+        public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)
+        {
+            loggerfactory.AddConsole(Microsoft.Extensions.Logging.LogLevel.Information);


Its because Azure also has a log level so this is disambiguiating

Tratcher · 2017-04-14T22:02:16Z

samples/OpenIdConnectSample/Startup.cs

+            });
+
+
+            services.AddAuthentication(sharedOptions =>


Move defaults to top

Tratcher · 2017-04-14T22:03:08Z

samples/OpenIdConnectSample/Startup.cs

@@ -144,32 +130,31 @@ await context.Authentication.SignOutAsync(OpenIdConnectDefaults.AuthenticationSc
                    return;
                }

-                // CookieAuthenticationOptions.AutomaticAuthenticate = true (default) causes User to be set
+                // DefaultAuthenticationScheme causes User to be set


DefaultAuthenticateScheme

Tratcher · 2017-04-14T22:13:36Z

samples/SocialSample/Startup.cs

                var user = context.User;

                // This is what [Authorize] calls
-                // var user = await context.Authentication.AuthenticateAsync(AuthenticationManager.AutomaticScheme);
+                // var user = await context.AuthenticateAsync(DefaultAuthenticateScheme);


AuthenticateAsync()

Tratcher · 2017-04-14T22:14:07Z

samples/SocialSample/Startup.cs

-                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("Access Token: " + await context.GetTokenAsync(CookieAuthenticationDefaults.AuthenticationScheme, "access_token") + "<br>");


Yeah, this is clunky

Tratcher · 2017-04-14T22:16:00Z

samples/OpenIdConnect.AzureAdSample/Startup.cs

+
+            services.AddAuthentication(sharedOptions =>
+            {
+                sharedOptions.DefaultAuthenticationScheme = CookieAuthenticationDefaults.AuthenticationScheme;


DefaultAuthenticateScheme

Tratcher · 2017-04-14T22:25:35Z

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs

-        {
-            if (!ChallengeCalled && Options.AutomaticChallenge && Response.StatusCode == 401)
+            Events = Options.Events;
+            if (Options.EventsType != null)


While it's strange for someone to set both EventsType and Events, it's even stranger that EventsType is preferred.

Yeah we have to do this because we always new up the events today.

I just meant we should change it to if (Events == null && Options.EventsType != null)

Yeah so Events is always not null in general, as the default is an empty events object, if we added that check, they'd have to explicitly null out events and set eventstype.

Tratcher · 2017-04-14T22:29:32Z

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs

-            var handler = (AuthenticationHandler<TOptions>)state;
-            await handler.FinishResponseOnce();
-        }
+            // REVIEW: should we validate options only once inside of the lock?


I think it needs to be outside. If the options are wrong then you need to fail every request, not just the first one.

Fair enough, I'll nuke the review

Tratcher · 2017-04-14T22:36:50Z

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs


-            if (ShouldHandleScheme(AuthenticationManager.AutomaticScheme, Options.AutomaticAuthenticate))
+            Options = OptionsSnapshot.Get(Scheme.Name) ?? new TOptions();


I think cookie auth is the only one that works with default options. We'd be better off throwing a specific error here and requiring providers to register their options.

So its pretty much an error in any implementation of IOptionsSnapshot for Get to return null, so we could actually just assume that it always returns not null.

Tratcher · 2017-04-14T22:38:03Z

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs

-            }
-        }
+        /// <returns>A new instance of the events instance.</returns>
+        protected virtual Task<object> CreateEventsAsync() => Task.FromResult<object>(null);


Not null, return a new object or NoEventsDefined

Tratcher · 2017-04-14T22:41:57Z

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs

        }

-        public async Task AuthenticateAsync(AuthenticateContext context)
+        public async Task<AuthenticateResult> AuthenticateAsync(AuthenticateContext context)


Context is not used? It's not even null checked.

Sweet, means we can nuke the type then

Tratcher · 2017-04-14T22:45:47Z

src/Microsoft.AspNetCore.Authentication/AuthenticationMiddleware.cs

-            }
-
-            _next = next;
+            _next = next ?? throw new ArgumentNullException(nameof(next));


Tratcher · 2017-04-14T22:49:31Z

src/Microsoft.AspNetCore.Authentication/AuthenticationMiddleware.cs


        public async Task Invoke(HttpContext context)
        {
-            var handler = CreateHandler();
-            await handler.InitializeAsync(Options, context, Logger, UrlEncoder);
+            var oldFeature = context.Features.Get<IAuthenticationFeature>();


Why are you expecting there to be an old feature? There can only be one auth middleware now, correct?

"Should be one" as opposed to 'can only be one'. Really we just need at least one. But I'm fine with dropping the save/restore goop

Is it important to null out features after we are done? Or can I nuke the try/finally entirely?

Tratcher · 2017-04-14T22:52:29Z

src/Microsoft.AspNetCore.Authentication/AuthenticationMiddleware.cs

+                // REVIEW: alternatively could depend on a routing middleware to do this
+
+                // Give any IAuthenticationRequestHandler schemes a chance to handle the request
+                var handlers = context.RequestServices.GetRequiredService<IAuthenticationHandlerProvider>();


Move HandleRequestAsync processing above defaultAuthenticate. HandleRequestAsync may short circuit the request.

Tratcher · 2017-04-15T02:05:05Z

src/Microsoft.AspNetCore.Authentication/AuthenticationMiddleware.cs

+
+                await _next(context);
+            }
+            finally


Yes, you can remove the finally and the nulling out.

Tratcher · 2017-04-17T22:32:58Z

src/Microsoft.AspNetCore.Authentication.JwtBearer/JwtBearerHandler.cs

-                await Options.Events.MessageReceived(messageReceivedContext);
-                if (messageReceivedContext.CheckEventResult(out result))
+                await Events.MessageReceived(messageReceivedContext);
+                if (messageReceivedContext.IsProcessingComplete(out result))


Still not a verb... IsFoo is used for bool properties. This might be good enough though...

Yeah this is internal plumbing right? This typically isn't called by event handlers is it?

Tratcher · 2017-04-17T22:43:34Z

src/Microsoft.AspNetCore.Authentication/AuthAppBuilderExtensions.cs

@@ -12,9 +12,9 @@ namespace Microsoft.AspNetCore.Builder
    public static class AuthAppBuilderExtensions
    {
        /// <summary>
-        /// Adds the <see cref="AuthenticationMiddleware"/> middleware to the specified <see cref="IApplicationBuilder"/>, which enables authentication capabilities.
+        /// Adds the <see cref="AuthenticationMiddleware"/> handler to the specified <see cref="IApplicationBuilder"/>, which enables authentication capabilities.


One too many replacements...

HaoK · 2017-04-17T23:58:38Z

Okay I think everything major has been addressed here, MVC/Identity are pretty much done as well. Working on MusicStore next.

kevinchalet · 2017-04-18T00:19:45Z

src/Microsoft.AspNetCore.Authentication.Facebook/FacebookExtensions.cs

+
+        public static IServiceCollection AddFacebookAuthentication(this IServiceCollection services, string authenticationScheme, Action<FacebookOptions> configureOptions)
+        {
+            services.AddSingleton<IConfigureOptions<FacebookOptions>, FacebookConfigureOptions>();


So, the magical IConfiguration-based binding will be used even if you specify a configuration delegate? Not sure I'm a huge fan of that... specially since this binder doesn't seem foolproof 😄

Well, your delegate runs after the binding so it 'wins'. Consider the AddXyz stuff early preview stuff we want people to try and give feedback on, the behavior will almost certainly evolve from here. Also in general the binding will be against an empty configuration section so its basically a default no-op (unless you happen to actually define a matching section to opt into the behavior)

Well, your delegate runs after the binding so it 'wins'.

Only if the DI container respects the services registration order, right? (IConfigureOptions.Order is no longer a thing in 1.0/2.0, so it entirely relies on the DI capabilities)

It would be way clearer if this feature was more explicit (e.g by flowing the IConfiguration as a parameter). It would also the developer to select the nodes he/she wants:

services.AddFacebookAuthentication(Configuration.GetSection("security:authentication:facebook"));

That already exists, if you want to bind against a section, you can just call Configure(Configuration.GetSection("facebook")).

DI respecting registration order is an implicit contract we already have today with options, especially without order, we must use registration order as order...

That already exists, if you want to bind against a section, you can just call

I know. My point is that the automatic configuration import should be explicit. If I provide a delegate, I don't expect the system to combine it with another source, specially since it's not consistent (cookies and OAuth2 don't use it)

DI respecting registration order is an implicit contract we already have today with options, especially without order, we must use registration order as order...

A requirement that was not clear at all in 1.0 and caused much much pain when using custom containers (aspnet/Hosting#785 / aspnet/DependencyInjection#433). But okay, point taken.

Yeah so the convention totally doesn't work when its not a 'default' section, which is why I didn't add it for cookies. If multiple OIDC handlers are going to be used frequently, it doesn't make a good candidate either.

I agree that might be a potential problem with no way to turn off the binding.

Not just a potential problem: it's actually completely broken when you have multiple schemes of the same type (e.g multiple OIDC or JWT middleware), because the options system has no way to tell that JwtBearerConfigureOptions registered with the first AddJwtBearerAuthentication() call should go with the scheme A and not the scheme B (all the JwtBearerConfigureOptions apply to all the registered schemes).

To fix that, you'd have to introduce "named configure options".

Well the named configure options already exist, but this 'default' binding section really won't work with multiple schemes of the same type, that's why I didn't push it everywhere, its makes the most sense for singleton frameworks like identity/MVC.

Its possible that this won't work well for auth at all if all of the schemes are likely to have multiple instances of the same type.

For the first iteration I'll just decouple the AddXyz() from the AddXyz(o => { }) again for now, we can figure out how to make things consistent later

@HaoK yeah, sounds better 👍

kevinchalet · 2017-04-18T00:27:20Z

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

+
+        public override async Task InitializeAsync(AuthenticationScheme scheme, HttpContext context)
+        {
+            await base.InitializeAsync(scheme, context);


The cool thing with the previous design is that we didn't have to call any of the base methods in the handler stuff. Any particular not to reason to preserve this pattern in 2.0?

You mean if you override Initialize and don't call base?

Or are you just asking for us to make it impossible to forget to call base via a separate virtual for children to implement.

InitializeAsync(scheme, context) => // do stuff, then call protected virtual InitializeAsync() { }

You mean if you override Initialize and don't call base?

Yep. This way, the chances of messing with the default initialization logic are low if you don't forget to call the base method.

Initialize on the other hand, can be refactored a bit, with a new parameterless protected virtual InitializeHandler() that's called after Options/Events have all been initialized.

kevinchalet · 2017-04-18T00:27:27Z

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

+
+        protected override void InitializeOptions()
+        {
+            base.InitializeOptions();


(same remark)

I'm not sure we can do too much to avoid requiring the call to base, because we do have a chain in the base classes today (Base => Remote => OAuth, etc), other than adding another virtual InitializeYourOptions() which is really the same as this one, but less likely to clobber :)

Tratcher · 2017-04-18T16:58:36Z

Looks like there's still several smaller comments un-addressed, but otherwise you're ready to go.

kevinchalet · 2017-04-18T19:33:25Z

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

@@ -16,14 +16,16 @@ public static class OpenIdConnectExtensions
        /// <param name="services"></param>
        /// <returns></returns>
        public static IServiceCollection AddOpenIdConnectAuthentication(this IServiceCollection services)
-            => services.AddOpenIdConnectAuthentication(OpenIdConnectDefaults.AuthenticationScheme, _ => { });
+        {
+            services.AddSingleton<IConfigureOptions<OpenIdConnectOptions>, OpenIdConnectConfigureOptions>();


Wait, if you call this method to register an OIDC provider using IConfiguration + one of the other overloads to register another provider using a configuration delegate, this "configure options" instance will also apply to the other instances, right?

No, these only configure the 'default' instance today, so the one named "OpenIdConnect" or whatever the default scheme name for each auth handler.

public OpenIdConnectConfigureOptions(IConfiguration config) : base(OpenIdConnectDefaults.AuthenticationScheme, options => config.GetSection(OpenIdConnectDefaults.AuthenticationScheme).Bind(options)) { } ``

The only way to apply a configure to all named instances is to use ConfigureAll/specify null name, the overloads that don't take name, use the empty string which means the 'default' instance.

But for auth we always have the scheme name, so we can always just use that.

Ah, cool then. Other question: what happens if I call AddOpenIdConnectAuthentication() twice? (which is likely not a good idea). Will the "configure options" be run twice? Wouldn't using TryAddSingleton make sense here?

TryAddSingleton isn't appropriate for options since they are meant to be layered on, for example that means if you Configure before you call Add, the bind wouldn't get added, which would be super weird.

Binding twice isn't wrong, just a bit weird, and you'd have weirdness like, Bind, Configure, Bind would likely stomp all over stuff since the binder is a bit eager (and dumb)

Okay. Anyway, this shouldn't be a problem, since the second call to AddOpenIdConnectAuthentication() should throw immediately.

HaoK · 2017-04-18T20:19:20Z

Okay I think that's the last change I have planned for this iteration (other than anything we will need from the other repos), I made the Initialize implementation non virtual, and have it call a new protected virtual InitializeHandler(), moved the two overrides we have today (Twitter/Cookies) to override that one instead, eliminating the need to call base in those situations.

kevinchalet · 2017-04-18T20:23:43Z

@HaoK thanks for the great work (and for being so patient )

Please ping me when consistent CI packages are available, I'd love to try them ASAP, to make sure everything I need is there.

HaoK · 2017-04-18T20:29:13Z

Will do, thanks as always for all the great feedback @PinpointTownes :)

HaoK · 2017-04-20T07:48:50Z

@PinpointTownes 24524 should have these changes now.

kevinchalet · 2017-04-20T19:50:19Z

@HaoK thanks!

FWIW, I just finished porting ASOS to ASP.NET Core 2.0 and so far, the experience is not that bad (except all the duplicate abstraction types...!). It seems to work as expected and all the tests pass successfully.

I have a few remarks/suggestions/demands, but I'll open dedicated threads for them.

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

HaoK mentioned this pull request Mar 31, 2017

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

Closed

kevinchalet reviewed Mar 31, 2017

View reviewed changes

This was referenced Apr 4, 2017

Make IsAjaxRequest overridable or configurable #1169

Closed

Middleware Extensibility: Support "Multi-tenancy" In AuthenticationOptions (and more) #35

Closed

HaoK added 3 commits April 12, 2017 12:58

Iteration 4 - Using Auth 2.0/Options 2.0 packages

781b1e5

Google auth

8918566

Use real packages

331ad23

HaoK mentioned this pull request Apr 13, 2017

The Grand Auth Redesign of 2017 #1179

Closed

13 tasks

HaoK added 2 commits April 13, 2017 15:13

Add locking for options initialization

1b57569

Fix

9b4676c

HaoK force-pushed the haok/auth2 branch from 3f0e783 to 9b4676c Compare April 13, 2017 22:14

kevinchalet reviewed Apr 13, 2017

View reviewed changes

Typo

6617557

Tratcher reviewed Apr 14, 2017

View reviewed changes

HaoK added 2 commits April 14, 2017 12:54

Clean up sln

acf7970

Samples tweaks

3f77eea

HaoK added 2 commits April 14, 2017 13:44

PR fixes

d86d251

Revert order changes

4c6c5db

Tratcher reviewed Apr 14, 2017

View reviewed changes

HaoK added 3 commits April 14, 2017 17:28

PR feedback

083cad6

Cleanup

e85fa58

Cleanup

5d348b5

Tratcher reviewed Apr 15, 2017

View reviewed changes

Add tests

6a4e43e

HaoK mentioned this pull request Apr 17, 2017

AddXyz() binds to default config section #1180

Closed

HaoK added 2 commits April 17, 2017 13:04

PR fixes

983b15c

Syntax

8373118

Tratcher reviewed Apr 17, 2017

View reviewed changes

Tratcher mentioned this pull request Apr 17, 2017

Add ASP.NET Core 1 and 2 cookie auth interop tests #1182

Closed

Tratcher reviewed Apr 17, 2017

View reviewed changes

HaoK added 5 commits April 17, 2017 15:53

React

8fafa80

Fix cookie purpose

217bf40

Fix comment

fc029e7

Allow DataProtectionProvider to be set

542f5ee

Used new token overload

956ca74

kevinchalet reviewed Apr 18, 2017

View reviewed changes

Split AddXyz and AddXyz(o => { }) for now

e68898d

kevinchalet suggested changes Apr 18, 2017

View reviewed changes

kevinchalet approved these changes Apr 18, 2017

View reviewed changes

Split Initialize into two

b7bc41f

HaoK closed this Apr 20, 2017

HaoK deleted the haok/auth2 branch April 28, 2017 21:28

HaoK restored the haok/auth2 branch April 28, 2017 21:28

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


		protected UrlEncoder UrlEncoder { get; private set; }

		public IAuthenticationHandler PriorHandler { get; set; }


		if (ShouldHandleScheme(AuthenticationManager.AutomaticScheme, Options.AutomaticAuthenticate))
		Options = OptionsSnapshot.Get(Scheme.Name) ?? new TOptions();

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

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

Conversation

HaoK commented Mar 31, 2017

dnfclas commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet Apr 13, 2017 • edited Loading

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 Apr 14, 2017

HaoK commented Apr 14, 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

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

Tratcher Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Apr 17, 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

kevinchalet Apr 13, 2017 •

edited

Loading

Tratcher Apr 17, 2017 •

edited

Loading