Auth 2.0 Iteration 2 #1113

HaoK · 2017-02-03T21:41:46Z

Test showing dynamic add/remove of schemes: d92d923#diff-220317c67a0ea163cf751157519c7f66R29

Will move this to HttpAbstractions once its baked (maybe the base Authentication package will need to move as well for servers to use the common infrastructure).

cc @Eilon @Tratcher @davidfowl

Follow up items:

Revisit HandleRequest routing
Normalize/revisit all of the events (some derive from Control some don't), mixed handling patterns.
Scheme options validation (things like SignInScheme defaults are initalized in handler, so can't validate on ConfigureTime)

Test holes:

Need unit tests for base OAuth and new Auth service interfaces, and when UseAuth is missing (RemoteAuth should throw if feature is missing)

dnfclas · 2017-02-03T21:41:50Z

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 · 2017-02-03T22:47:25Z

Rather than nuking the existing auth package first, requiring updating everything at once, implementation of the new stack will be in a Authentication.Impl package until everything is done..

Renamed IAuthenticationSchemeHandler => IAuthenticationHandler (taking over the old interface name), shouldn't lose any clarity
Added IAuthenticationHandlerResolver which has a single method which resolve handler instance for a given scheme + request.

brockallen · 2017-02-03T22:50:29Z

@leastprivilege and I have a TODO item to look over and give feedback. We were hoping to have done this last week, but it didn't happen. Perhaps this weekend or next week. Thanks.

HaoK · 2017-02-03T22:53:50Z

Sure, you guys might want to look at the old PR first for better context/info, and it has more or less the same shape, it will take a bit for me to stitch everything back together again in this PR.

HaoK · 2017-02-09T18:55:46Z

Seems too confusing to have any dependency on the old HttpAuthentication feature, going to copy ChallengeBehavior and AuthenticationProperties instead, since we aren't supporting old and new together.

HaoK · 2017-02-09T20:34:51Z

test/Microsoft.AspNetCore.Authentication.Test/Cookies/CookieMiddlewareTests.cs

@@ -851,68 +740,85 @@ private Task SignOutAsWrong(HttpContext context)
        }

        [Fact]
-        public async Task MapWillNotAffectChallenge()
+        public async Task MapWillAffectChallenge()


Note: this is a behavior change from old stack to new stack (Map does affect redirect url for challenge now that its not tied to any middleware)

Be careful, it's most likely not a wanted change: if /page is configured as the callback path and the user is actually redirected to /login/page, all he'll get is a 404 response, as no login page will handle the request.

I assume it similarly impacts all the RAT-based middleware that use callback paths and have built-in endpoints? (OIDC, OAuth2, Twitter...)

HaoK · 2017-02-09T20:36:30Z

Cookies and tests have been updated to use the new stack, and pull system clock from DI

kevinchalet · 2017-02-11T06:27:25Z

test/Microsoft.AspNetCore.Authentication.Test/Cookies/CookieMiddlewareTests.cs

            Assert.Equal("?ReturnUrl=%2F", location.Query);
        }

-        [Fact]
+        [ConditionalFact(Skip = "Revisit, exception no longer thrown")]


Hum, why? The InvalidOperationException is there to inform the dev' that something is wrong with his setup. I don't think it's preferable to silently ignore the operation.

I think the old test threw because it was doing an AutomaticChallenge followed by an second challenge, which no longer happens in the new stack, I'll have to debug the old test, to see what was triggering it

HaoK · 2017-02-13T20:44:42Z

test/Microsoft.AspNetCore.Authentication.Test/Cookies/CookieMiddlewareTests.cs

+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task MapWillAffectChallengeOnlyWithUseAuth(bool useAuth)


FYI @PinpointTownes fixed the map + resolve url behavior to work similar to before (using the path of the single middleware), via a new IAuthenticationFeature like we discussed @Tratcher

Really great news!

Tratcher · 2017-02-13T22:53:02Z

samples/OpenIdConnectSample/Properties/launchSettings.json

@@ -3,8 +3,8 @@
    "windowsAuthentication": false,
    "anonymousAuthentication": true,
    "iisExpress": {
-      "applicationUrl": "http://localhost:42023",


revert, these samples need to stay on a specific port.

Tratcher · 2017-02-13T23:07:49Z

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

+    {
+        public AuthenticationScheme(string name, Type handlerType, Dictionary<string, object> settings)
+        {
+            Name = name ?? throw new ArgumentNullException(nameof(name));


FYI: @Eilon has rejected the use of this syntax.

Does that catch string.empty?

Tratcher · 2017-02-13T23:08:14Z

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

+{
+    public class AuthenticationScheme
+    {
+        public AuthenticationScheme(string name, Type handlerType, Dictionary<string, object> settings)


Why not just take an IReadOnlyDictionary?

Good point, fixed

Tratcher · 2017-02-13T23:13:01Z

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

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public enum ChallengeBehavior


Looking to eliminate this. Let Challenge and Forbidden be separate code paths, and Automatic would be resolved at a higher level.

Yeah but we likely can't do this until there's a bunch of corresponding AuthZ changes, we should tackle that in a follow on PR specifically for AuthZ (see #901)

Tratcher · 2017-02-13T23:25:37Z

...pNetCore.Authentication.Abstractions/Microsoft.AspNetCore.Authentication.Abstractions.csproj

+  </PropertyGroup>
+  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.DataProtection" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.AspNetCore.Http" Version="1.2.0-*" />


What did you need from .HTTP? It's unusual for an Abstractions package to have this many concrete dependencies.

Good catch, these were just carried over from eixsting Authentication.csproj, I switched them to just the Http.Abstractions package and things were fine.

Tratcher · 2017-02-13T23:33:17Z

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


-namespace Microsoft.AspNetCore.Builder
+namespace Microsoft.AspNetCore.Authentication.Cookies


We'd used this namespace to improve intellisence in Startup, it's the same namespace the extensions were in. Should these move to the DI namespace?

I don't think that's necessary with lambdas, as intellisense knows the type from the signature, the namespace hack was only needed for new

Tratcher · 2017-02-13T23:40:18Z

src/Microsoft.AspNetCore.Authentication.Cookies/Events/BaseCookieContext.cs

@@ -23,5 +24,7 @@ public class BaseCookieContext : BaseContext
        }

        public CookieAuthenticationOptions Options { get; }
+
+        public AuthenticationScheme Scheme { get; }


Would Scheme go on BaseAuthenticationContext?

I tried to bridge the context's between the service and the handlers. The service doesn't care about the scheme object, only the string, but maybe we should just have a BaseAuthenticationHandlerContext that does have the scheme. I have a todo in the top comment to make a pass through all of our events/context objects since they have always been a bit of a hodgepodge mess

Tratcher · 2017-02-13T23:47:34Z

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

-                throw new ArgumentNullException(nameof(options));
-            }
-
-            if (string.IsNullOrEmpty(Options.AppId))


These checks are missing

Grrr yup, I'll add a test for these as well

Moved the checks to Validate in the options which at least causes an exception, but the current behavior isn't perfect, in that it blows up the first time the scheme is used in the auth service.

Weren't we going to call Validate from the UseFooAuth extension too?

Sorry I mispoke, it blows up when the Auth service is constructed as the schemes are built when SchemeProvider/HandlerResolvers are built. So maybe that's not that bad if the auth service itself cannot be resolved due to a configuration error.

Can you share the callstack/exception?

typeof(System.ArgumentException): The 'ClientId' option must be provided. Parameter name: ClientId Stack Trace: at Microsoft.AspNetCore.Authentication.OAuth.OAuthOptions.Validate() in C:\GitHub\Security\src\Microsoft.AspNetCore.Authentication.OAuth\OAuthOptions.cs:line 33 at Microsoft.Extensions.DependencyInjection.AuthenticationServiceCollectionExtensions.<>c__DisplayClass2_0`2.<AddSchemeHandler>b__1(AuthenticationSchemeBuilder b) in C:\GitHub\Security\src\Microsoft.AspNetCore.Authentication.Impl\AuthenticationServiceCollectionExtensions.cs:line 66 at Microsoft.AspNetCore.Authentication.AuthenticationOptions.AddScheme(String name, Action`1 configureBuilder) in C:\GitHub\Security\src\Microsoft.AspNetCore.Authentication.Abstractions\AuthenticationOptions.cs:line 42 at Microsoft.Extensions.DependencyInjection.AuthenticationServiceCollectionExtensions.<>c__DisplayClass2_0`2.<AddSchemeHandler>b__0(AuthenticationOptions o) in C:\GitHub\Security\src\Microsoft.AspNetCore.Authentication.Impl\AuthenticationServiceCollectionExtensions.cs:line 56 at Microsoft.Extensions.Options.OptionsCache`1.CreateOptions() at System.Threading.LazyInitializer.EnsureInitializedCore[T](T& target, Boolean& initialized, Object& syncLock, Func`1 valueFactory) at Microsoft.Extensions.Options.OptionsCache`1.get_Value() at Microsoft.AspNetCore.Authentication.DefaultAuthenticationSchemeProvider..ctor(IOptions`1 options) in C:\GitHub\Security\src\Microsoft.AspNetCore.Authentication.Impl\DefaultAuthenticationSchemeProvider.cs:line 18 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, ServiceProvider provider) at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProvider provider) at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, ServiceProvider provider) at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProvider provider) at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, ServiceProvider provider) at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitScoped(ScopedCallSite scopedCallSite, ServiceProvider provider) at Microsoft.Extensions.DependencyInjection.ServiceProvider.<>c__DisplayClass17_0.<RealizeService>b__0(ServiceProvider provider) at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType) at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider) at Microsoft.AspNetCore.Authentication.AuthenticationHttpContextExtensions.ChallengeAsync(HttpContext context, String scheme, AuthenticationProperties properties, ChallengeBehavior behavior) in C:\GitHub\Security\src\Microsoft.AspNetCore.Authentication.Abstractions\AuthenticationHttpContextExtensions.cs:line 23 at Microsoft.AspNetCore.Authentication.AuthenticationHttpContextExtensions.ChallengeAsync(HttpContext context, String scheme, AuthenticationProperties properties) in C:\GitHub\Security\src\Microsoft.AspNetCore.Authentication.Abstractions\AuthenticationHttpContextExtensions.cs:line 20 at Microsoft.AspNetCore.Authentication.AuthenticationHttpContextExtensions.ChallengeAsync(HttpContext context, String scheme) in C:\GitHub\Security\src\Microsoft.AspNetCore.Authentication.Abstractions\AuthenticationHttpContextExtensions.cs:line 17 at Microsoft.AspNetCore.Authentication.OAuth.OAuthTests.<>c__DisplayClass0_0.<ThrowsIfClientIdMissing>b__4() in C:\GitHub\Security\test\Microsoft.AspNetCore.Authentication.Test\OAuthTests.cs:line 33

That only says OAuth, it doesn't tell you Facebook. And Facebook calls it AppId. This is going to cause confusion.

I copied a different exception failure for the above example from the oauth tests, the facebook one says app id and has a facebookoptions.validate at the top.

Ah, good to know

HaoK · 2017-02-14T00:11:25Z

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

        protected virtual async Task FinishResponseAsync()
        {
            // Only renew if requested, and neither sign in or sign out was called
-            if (!_shouldRefresh || SignInAccepted || SignOutAccepted)
+            if (!_shouldRefresh || SignInCalled || SignOutCalled)


Test hole here, need to add a test that verifies cookies aren't renewed if SignIn or SignOut is called. (this was broken before this change)

HaoK · 2017-02-14T22:55:16Z

We also had lost the OauthOptions validations too which had no test coverage :/ I added some basic validation tests for those, also compact'ed the test structure, I nuked all the folders that had a single file, so basically only OpenIdConnect has a subfolder in Auth.Test now. Also started dropping middleware from the file names/comments where I see it.

brockallen · 2017-02-16T15:50:25Z

Hi all -- again, no time yet to have a look, but I wanted to let you know how we're integrating with the current security middleware in IdentityServer. In short, we're registering our own authentication middleware to be able to do a decorator pattern like feature to applications hosting us. We'd like to know if we can do something similar in this v2 work. Here's the main handler we have now:

https://github.com/IdentityServer/IdentityServer4/blob/dev/src/IdentityServer4/Hosting/AuthenticationHandler.cs

kevinchalet · 2017-02-16T15:58:17Z

@brockallen the "authentication middleware" chain is no longer a thing in this prototype, so you won't be able to port your handler as-is. That said, I guess you could achieve something similar by replacing IAuthenticationService (which is more or less the equivalent of AuthenticationManager, but DI-injected).

brockallen · 2017-02-16T16:06:17Z

I guess you could achieve something similar by replacing IAuthenticationService

Right, but then we'd probably need to decorate whatever implementation is provided by default. Too bad the core DI system is so lacking in so many useful features.

kevinchalet · 2017-02-16T16:18:49Z

Too bad the core DI system is so lacking in so many useful features.

There a few workarounds to make the default DI stack (kinda) work with the decorator pattern, but it's indeed not ideal... I guess that's the price to pay for having full DI here

To be honest, I'm not a huge fan of the "full DI" approach suggested by @HaoK and @brockallen as it will likely introduce a whole new set of issues without really solving the initial one.

#1061 (comment)

brockallen · 2017-02-16T16:34:01Z

Well, the original chained handler design in katana was essentially an attempt at designing a poor man's decorator pattern in a system that had no DI. Moving to a service oriented approach in a DI system with missing features is just changing the surrounding code. I think I agree that it's not addressing the fundamental requirement. I've yet to look into the other changes to see if they've done anything to make the overall design better.

HaoK · 2017-02-16T23:02:51Z

@brockallen it looks like you could just replace the DefaultAuthenticationService with your own derived one, something like this:

  public class AuthenticationService : DefaultAuthenticationService, IAuthenticationService 
  {
        private readonly IdentityServerOptions _options;
        private readonly IHttpContextAccessor _context;
        private readonly ISessionIdService _sessionId;
        private readonly ILogger<AuthenticationHandler> _logger;
        private readonly IEventService _events;

        public AuthenticationService(
            IAuthenticationHandlerResolver cache, 
            IClaimsTransformation transform,
            IHttpContextAccessor context, 
            IdentityServerOptions options, 
            ISessionIdService sessionId,
            IEventService events,
            ILogger<AuthenticationService> logger) : base(cache, transform)
        {
            _context = context;
            _options = options;
            _sessionId = sessionId;
            _events = events;
            _logger = logger;
        }

        public override async Task SignInAsync(SignInContext context)
        {
            if (context.AuthenticationScheme == _options.Authentication.EffectiveAuthenticationScheme)
            {
                AugmentContext(context);
                await RaiseSignInEventAsync(context.Principal);
            }
            await base.SignInAsync(context);
        }

        private void AugmentContext(SignInContext context)
        {
            _logger.LogDebug("Augmenting SignInContext");

            context.Principal.AssertRequiredClaims();
            context.Principal.AugmentMissingClaims();

            _sessionId.CreateSessionId(context);
        }

        public override async Task SignOutAsync(SignOutContext context)
        {
            if (context.AuthenticationScheme == _options.Authentication.EffectiveAuthenticationScheme)
            {
                _sessionId.RemoveCookie();
                await RaiseSignOutEventAsync();
            }
            await base.SignOutAsync(context);
        }

        // Would need to move to your own middleware most likely
        internal async Task InitAsync()
        {
            var auth = GetAuthentication();
            _handler = auth.Handler;
            auth.Handler = this;

            await _sessionId.EnsureSessionCookieAsync();
        }

        internal void Cleanup()
        {
            var auth = GetAuthentication();
            auth.Handler = _handler;
        }

        // No longer needed, DI registration
        IHttpAuthenticationFeature GetAuthentication()
        {
            var auth = _context.HttpContext.Features.Get<IHttpAuthenticationFeature>();
            if (auth == null)
            {
                auth = new HttpAuthenticationFeature();
                _context.HttpContext.Features.Set(auth);
            }
            return auth;
        }

        private async Task RaiseSignInEventAsync(ClaimsPrincipal principal)
        {
            await _events.RaiseLoginEventAsync(principal);
        }

        private async Task RaiseSignOutEventAsync()
        {
            var principal = await _context.HttpContext.GetIdentityServerUserAsync();
            if (principal.IsAuthenticated())
            {
                await _events.RaiseLogoutEventAsync(principal);
            }
        }
    }
   }

HaoK · 2017-02-27T22:01:31Z

Updated: schemes must now explicitly register callback paths in their scheme data. Schemes ProcessRequest will only be called if Request.Path matches a callback path they specify.

Will probably remove HandleRemoteAuthenticate since its no longer needed. Also will change the base ProcessRequest to throw now that it shouldn't be invoked anymore

Tratcher · 2017-02-28T17:02:34Z

How terrible would it be to pull in actual routing?

HaoK · 2017-02-28T18:32:44Z

Well we could add something like "routes.MapAuthenticationRoutes()". We'd still have to have some internal plumbing in the schemes to store the routes for this method to consume. So the required startup pattern would then become:

    app.UseRouter(routes => routes.MapAuthenticationRoutes(appropriateAuthDataThingy));
    app.UseAuthentication();

Doesn't look too unreasonable, but is it reasonable for us to require routing for auth to work?

Tratcher · 2017-02-28T18:39:10Z

Not bad. How well would that work with dynamically added/removed auth schemes?

HaoK · 2017-02-28T18:40:08Z

I'm not sure, depends on how well routing supports dynamically adding routes :) Let me see

kevinchalet · 2017-02-28T18:41:13Z

Forking my comment as it was hidden by GitHub:

@HaoK point taken, tho' I'm not sure why you'd want to support scenarios with "hundreds/thousands of schemes" if there's no real support in the options stack (I'm not sure having hundreds of schemes + hundreds of handlers + hundreds of options instances is really scalable).

Not being able to invoke my handler for every path is really annoying for me. As a compromise, would you consider bringing back your per-scheme "promiscuous mode"? (aka CanHandleRequest 😄)

HaoK · 2017-02-28T18:48:27Z

I'm not an routing expert, but it looks if we forced a pattern on the callback paths to fit into a route constraint, say /auth/signin/[scheme] we could easily plug in something that dispatched dynamically. There might be other ways as well.

HaoK · 2017-02-28T18:49:46Z

@PinpointTownes its going to end up being a question of how easy we want to make it to have a global handler like that. We certainly could have a handleAllRequests flag on AuthenticationScheme that defaults to false.

HaoK · 2017-03-06T20:07:29Z

Started cleaning up some of the legacy handler processing.

AuthenticateResult.Nothing/None() is the replacement for Skip() since we no longer really 'skip' anything. The concept only exists a bit in the Remote/ProcessRequest codepath which keeps the concept of ResponseHandled/StopProcessing (the new name for context.SkipToNextMiddleware() which now returns a None()/Nothing result

I'm considering introducing a new ProcessRequestResult which would let us elimate the funky request processing that shows up in AuthenticateResult today since that's the main return type for Auth as a whole.

kevinchalet · 2017-03-06T21:11:01Z

We certainly could have a handleAllRequests flag on AuthenticationScheme that defaults to false.

That would be much appreciated 😅

The concept only exists a bit in the Remote/ProcessRequest codepath which keeps the concept of ResponseHandled/StopProcessing (the new name for context.SkipToNextMiddleware() which now returns a None()/Nothing result

Hum, but then, what's the difference between HandleResponse() and StopProcessing()?

HaoK · 2017-03-06T21:53:01Z

Difference between HandledResponse and StopProcessing is whether ProcessRequest returns true/false, same as today's handledResponse/Skip. Controls whether the middleware continues to invoke other handlers or not via the ghetto routing :)

kevinchalet · 2017-03-06T22:13:50Z

I'm confused now.

When StopProcessing is called from an event, HandleRequestAsync is supposed to return false, right? (e.g

Security/src/Microsoft.AspNetCore.Authentication.Impl/RemoteAuthenticationHandler.cs

Line 116 in d80b0d3

return false;

)

Yet, returning false asks the authentication middleware to continue invoking the remaining authentication middleware and possibly, invoking the rest of the ASP.NET Core pipeline:

Security/src/Microsoft.AspNetCore.Authentication.Impl/AuthenticationMiddleware.cs

Lines 49 to 52 in d80b0d3

    
           if (await handler.HandleRequestAsync()) 
        
           { 
        
               return; 
        
           }

So AFAICT, your renamed StopProcessing() method actually means "skip the rest of the authentication handler logic but continue invoking the other middleware and the rest of the pipeline"? IMHO, Skip was way clearer.

HaoK · 2017-03-06T22:37:41Z

Fair enough, it was mostly the SkipToNextMiddleware that I was renaming, since the emphasis should just be that its skipping any further processing in this handler. What happens after that is the normal execution.

We can keep it as Continue/Handled/Skip as the three states which correspond to (false, true, false for the AuthMiddleware invoking the rest of the pipeline)

HaoK · 2017-03-06T22:42:09Z

The confusing part is that StopProcessing is the reasonable name from the perspective of the individual handler's usage of this state, since it lives in the BaseControlContext. I'll leave that method name still as ProcessingCompleted since for both HandledResponse + Skipped, that means true for the purposes of the handler who is using the context. All of this smells a bit to me which is why I'm playing around with this area right now...

HaoK · 2017-03-06T22:58:35Z

Ok I revereted the StopProcessing state back to Skip. So the net result of these changes are:

AuthenticateResult.Skip => AuthenticateResult.None to signify no failure, no ticket, nothing
BaseControlContext.CheckEventResult => ProcessingCompleted which is what all of the handlers call on event contexts to determine if they should stop procesing.
Skip remains the verb on the event context objects.

HaoK · 2017-03-07T23:59:43Z

Some implications if we try to use the routing middleware instead of our own ghetto routing:

How to expose the route configuration to the schemes /signin-{Scheme} by default? Scheme a required paramater and AuthHandler would simply lookup the scheme handler and invoke ProcessRequest

    schemeBuilder.AddRoute("/signin-{Scheme}"); // Route that would work for all schemes?
    schemeBuilder.AddRoute("/fixedCustom"); // A fixed route would specify the current scheme

We'd have to find some way to plug into the mvc routes, i.e.

    app.UseAuthentication();
    app.UseMvc(routes => {
         routes.MapRoutes("mvcstuff");
         routes.MapAuthenticationRoutes(); // uses the configured auth schemes to add routes
    });

Or Without MVC, this overload could add routing and configure it?

    app.UseAuthentication(routes => { }); // automatically would call routes.MapAuthenticationRoutes()

We'd lose the ability to 'Skip' and let the normal request processing continue. Once routing selects a route handler, the middleware chain is broken. So there would be no support for an auth handler to inspect all requests (have to add your own middleware)

HaoK · 2017-03-11T02:15:32Z

See #1144 for an iteration using IOptionsFactory which provides named options (and potentially validation). It also split the ProcessRequest into a different interface IAuthenticationRequestHandler which is used instead of CallbackPaths to determine which schemes get called by the authentication middleware

Tratcher

Some feedback. Still reviewing.

Tratcher · 2017-03-15T17:16:37Z

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);


No more generic Challenge? It should work if there's only one or its the default.

We can preserve it, it would line up with naked Authorize with no DefaultPolicy then.

Well if we add an overload for Challenge that omits scheme, we should probably do the same for authenticate and SignIn, basically null scheme will mean use default (since the overloads are implemented as extension methods)

Tratcher · 2017-03-15T17:38:22Z

samples/CookieSessionSample/Startup.cs

@@ -39,7 +36,7 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)
                        claims.Add(new Claim(ClaimTypes.Role, "SomeRandomGroup" + i, ClaimValueTypes.String, "IssuedByBob", "OriginalIssuerJoe"));
                    }

-                    await context.Authentication.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme,
+                    await context.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme,


So AuthenticationManager is completely obsolete and you're rooting all of the extensions on HttpContext?

Yeah we should deprecate the Authentication versions as part of this

Tratcher · 2017-03-15T17:47:38Z

samples/OpenIdConnectSample/Startup.cs

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


DefaultSignInScheme. You also need to set DefaultAuthenticationScheme and DefaultChallengeScheme right?

Same for the AzureAd sample.

Yup, updating these in the other PR

Tratcher · 2017-03-15T17:52:37Z

samples/SocialSample/Startup.cs

-                SaveTokens = true,
+                o.AppId = Configuration["facebook:appid"];
+                o.AppSecret = Configuration["facebook:appsecret"];
+                o.Scope.Clear();


hmm, syntax usage regression.

I don't think the old synatax was Clearing, just Adding.

Clears removed

Tratcher · 2017-03-15T17:58:45Z

samples/SocialSample/Startup.cs

-                ClientId = Configuration["microsoftaccount:clientid"],
-                ClientSecret = Configuration["microsoftaccount:clientsecret"],
-                SaveTokens = true
+                o.DisplayName = "MicrosoftAccount";


Why is the display name set for this one? Isn't this the default value.

Tratcher · 2017-03-15T19:02:25Z

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

+        public Type HandlerType { get; }
+
+        /// <summary>
+        /// If true, the AuthenticationMiddleware will call the handler's HandleRequestAsync method.


Wrong comment?

Many of the comments were already obsolete, we'll need to do a full scrub as part of this

Tratcher · 2017-03-15T19:04:08Z

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; }


This is really weird. Are we using it for anything besides Options and Description? Why not just expose those as first class properties?

No, but I nuked AuthenticationDescription which was similar and unused mostly. We can put the DisplayName as a property on the scheme itself or inside of here like Description used to...

Tratcher · 2017-03-15T19:07:51Z

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

+        /// <summary>
+        /// Gets the claims-principal with authenticated user identities.
+        /// </summary>
+        public ClaimsPrincipal Principal{ get; private set; }


Tratcher · 2017-03-15T19:08:03Z

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

+        public AuthenticationTicket(ClaimsPrincipal principal, AuthenticationProperties properties, string authenticationScheme)
+        {
+            AuthenticationScheme = authenticationScheme;
+            Principal = principal ?? throw new ArgumentNullException(nameof(principal));


rejected syntax

Tratcher · 2017-03-15T19:09:36Z

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

+    public class AuthenticationToken
+    {
+        public string Name { get; set; }
+        public string Value { get; set; }


Make immutable and set via constructor?

Tratcher · 2017-03-15T19:29:23Z

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

+        private readonly AuthenticationOptions _options;
+        private readonly object _lock = new object();
+
+        private IDictionary<string, AuthenticationScheme> _map = new Dictionary<string, AuthenticationScheme>(); // case sensitive?


Specify comparer. Ordinal?

Tratcher · 2017-03-15T19:33:04Z

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

+                    {
+                        if (!_handlerMap.ContainsKey(path))
+                        {
+                            _handlerMap[path] = new List<AuthenticationScheme>();


List? I thought the point was that it would be a unique mapping. Urls usually map to a unique resource.

Tratcher · 2017-03-15T19:36:19Z

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

+    /// <summary>
+    /// Builds the actual AuthenticationScheme instances from the AuthenticationOptions.
+    /// </summary>
+    public class DefaultAuthenticationSchemeProvider : IAuthenticationSchemeProvider


AuthenticationSchemeProvider, remove Default

Good call, dropped Default prefix across the board

Tratcher · 2017-03-15T19:38:07Z

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

+            {
+                throw new InvalidOperationException("Scheme already exists: " + scheme.Name);
+            }
+            lock (_lock)


Locking on modifications is insufficient, you also have to lock on lookups.

Tratcher · 2017-03-15T22:00:20Z

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

+            await handler.SignOutAsync(signOutContext);
+        }
+
+        public virtual Task SignInAsync(HttpContext httpContext, string authenticationScheme, ClaimsPrincipal principal)


Why isn't this an extension?

Yeah unneeded these already exists as extensions on httpContext, I'll remove

Tratcher · 2017-03-15T22:00:46Z

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

+            return SignInAsync(httpContext, authenticationScheme, principal, properties: null);
+        }
+
+        public virtual Task ForbidAsync(HttpContext httpContext, string authenticationScheme)


Tratcher · 2017-03-15T22:01:16Z

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

+    /// <summary>
+    /// Default claims transformation is a no-op.
+    /// </summary>
+    public class DefaultClaimsTransformation : IClaimsTransformation


NoOpClaimsTransformation

Tratcher · 2017-03-15T22:01:53Z

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

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public class DefaultAuthenticationService : IAuthenticationService


AuthenticationService

HaoK · 2017-03-15T22:29:21Z

Replaced with #1151

dnfclas added the cla-already-signed label Feb 3, 2017

HaoK mentioned this pull request Feb 3, 2017

[Design] Authentication 2.0 #1065

Closed

HaoK changed the title ~~First cut of Authentication.Abstractions~~ Auth 2.0 Iteration 2 Feb 3, 2017

HaoK force-pushed the haok/abs branch from 250d160 to 00c9e28 Compare February 9, 2017 20:31

HaoK commented Feb 9, 2017

View reviewed changes

kevinchalet reviewed Feb 11, 2017

View reviewed changes

HaoK commented Feb 13, 2017

View reviewed changes

Tratcher reviewed Feb 13, 2017

View reviewed changes

HaoK commented Feb 14, 2017

View reviewed changes

HaoK force-pushed the haok/abs branch from df8cc65 to a907b4f Compare February 14, 2017 22:04

Schemes must register callbackPaths

7700e08

Add GetAllSchemes

96c2c8b

HaoK mentioned this pull request Mar 4, 2017

AuthenticationMiddleware and dependency injection #1134

Closed

HaoK added 2 commits March 6, 2017 12:01

Rename Skip => Stop/None

a5d6d5a

Cleanup

d80b0d3

Bring back Skip

caf7ac7

HaoK mentioned this pull request Mar 7, 2017

Revisit identity factoring and react to Auth 2.0 aspnet/Identity#651

Closed

Tratcher reviewed Mar 15, 2017

View reviewed changes

HaoK closed this Mar 15, 2017

kevinchalet mentioned this pull request Apr 19, 2017

The Grand Auth Redesign of 2017 #1179

Closed

13 tasks

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


		namespace Microsoft.AspNetCore.Builder
		namespace Microsoft.AspNetCore.Authentication.Cookies

Auth 2.0 Iteration 2 #1113

Auth 2.0 Iteration 2 #1113

Conversation

HaoK commented Feb 3, 2017 • edited Loading

dnfclas commented Feb 3, 2017

HaoK commented Feb 3, 2017 • edited Loading

brockallen commented Feb 3, 2017

HaoK commented Feb 3, 2017

HaoK commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Feb 9, 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

HaoK commented Feb 14, 2017 • edited Loading

brockallen commented Feb 16, 2017

kevinchalet commented Feb 16, 2017 • edited Loading

brockallen commented Feb 16, 2017

kevinchalet commented Feb 16, 2017

brockallen commented Feb 16, 2017

HaoK commented Feb 16, 2017 • edited Loading

HaoK commented Feb 27, 2017

Tratcher commented Feb 28, 2017

HaoK commented Feb 28, 2017 • edited Loading

Tratcher commented Feb 28, 2017

HaoK commented Feb 28, 2017

kevinchalet commented Feb 28, 2017

HaoK commented Feb 28, 2017

HaoK commented Feb 28, 2017

HaoK commented Mar 6, 2017

kevinchalet commented Mar 6, 2017

HaoK commented Mar 6, 2017

kevinchalet commented Mar 6, 2017 • edited Loading

HaoK commented Mar 6, 2017

HaoK commented Mar 6, 2017 • edited Loading

HaoK commented Mar 6, 2017

HaoK commented Mar 7, 2017

HaoK commented Mar 11, 2017

Tratcher left a 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 Feb 3, 2017 •

edited

Loading

HaoK commented Feb 3, 2017 •

edited

Loading

HaoK commented Feb 14, 2017 •

edited

Loading

kevinchalet commented Feb 16, 2017 •

edited

Loading

HaoK commented Feb 16, 2017 •

edited

Loading

HaoK commented Feb 28, 2017 •

edited

Loading

kevinchalet commented Mar 6, 2017 •

edited

Loading

HaoK commented Mar 6, 2017 •

edited

Loading