Add defaults + split SignIn/SignOut #873

HaoK · 2017-06-16T21:46:02Z

Move SignIn/Signout into IAuthenticationHandlerSign[In\Out]Handler
Add tests for AuthenticationService
add overloads for SignOut/Forbid to use new default

HaoK · 2017-06-16T21:47:35Z

Fixes aspnet/Security#1244 and aspnet/Security#1264

Need to update Security/Hosts (to remove SignIn/Signout impls only) as a result of this PR as well

HaoK · 2017-06-16T21:49:19Z

I'm going to combine the events changes we want for aspnet/Security#1266 into this PR as well. So the update to AuthenticationResult + rename of None => Ignore will go in this PR as well

Tratcher · 2017-06-17T15:11:17Z

src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs

@@ -93,7 +90,7 @@ public Task<AuthenticationScheme> GetDefaultForbidSchemeAsync()
        /// <summary>
        /// Returns the scheme that will be used by default for <see cref="IAuthenticationService.SignInAsync(HttpContext, string, System.Security.Claims.ClaimsPrincipal, AuthenticationProperties)"/>.
        /// This is typically specified via <see cref="AuthenticationOptions.DefaultSignInScheme"/>.
-        /// Otherwise, if only a single scheme exists, that will be used, if more than one exists, null will be returned.
+        /// Otherwise, this will fallback to <see cref="GetDefaultAuthenticateSchemeAsync"/>.


Update comment to include the single scenario?

Tratcher · 2017-06-17T15:12:01Z

src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs

@@ -121,6 +118,10 @@ public Task<AuthenticationScheme> GetDefaultSignOutSchemeAsync()
            {
                return GetSchemeAsync(_options.DefaultSignOutScheme);
            }
+            if (_signOutHandlers.Count() == 1)


Add to doc comments?

Tratcher · 2017-06-17T15:13:49Z

src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs

+            if (_signOutHandlers.Count() == 1)
+            {
+                return Task.FromResult(_signOutHandlers[0]);
+            }
            return GetDefaultSignInSchemeAsync();


There's now no guarantee that a signin handler supports signout. Maybe we should change the hierarchy to ISignIn -> ISignOut -> IAuth

I was just mirroring what we have in security today, where we have OIDC only implementing remote sign out...

Maybe we just tweak this to also ensure that the sign in scheme handler implements sign out before returning it, we can have it continue to fallback to sign in

This problem also exists for the fallback for SignIn => Authenticate, so we need to add a similar check for that too

No, the SignIn interface derives from the Auth interface, but SignOut does not derive from SignIn.

That's ok, I just mean:

var signIn = GetDefaultSignInSchemeAsync(); if (signIn != null && typeof(IAuthenticationSignOutHandler).IsAssignableFrom(signIn.HandlerType)) return signIn; else return null;

Tratcher · 2017-06-17T15:17:07Z

src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs

+                if (typeof(IAuthenticationSignOutHandler).IsAssignableFrom(scheme.HandlerType))
+                {
+                    _signOutHandlers.Add(scheme);
+                }
                _map[scheme.Name] = scheme;


There's no check that the HandlerType actually implements IAuthenticationHandler, which the doc comments say is required.

That's done in the scheme itself,

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

Line 31 in 88a797c

if (!typeof(IAuthenticationHandler).IsAssignableFrom(handlerType))

campersau · 2017-06-17T15:17:14Z

src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs

@@ -102,11 +99,11 @@ public Task<AuthenticationScheme> GetDefaultSignInSchemeAsync()
            {
                return GetSchemeAsync(_options.DefaultSignInScheme);
            }
-            if (_map.Count == 1)
+            if (_signInHandlers.Count() == 1)


Why using Linq?

campersau · 2017-06-17T15:17:21Z

src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs

@@ -121,6 +118,10 @@ public Task<AuthenticationScheme> GetDefaultSignOutSchemeAsync()
            {
                return GetSchemeAsync(_options.DefaultSignOutScheme);
            }
+            if (_signOutHandlers.Count() == 1)


Why using Linq?

davidfowl · 2017-06-17T16:37:44Z

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

+    /// <summary>
+    /// Used to determine if a handler supports SignOut.
+    /// </summary>
+    public interface IAuthenticationSignOutHandler : IAuthenticationHandler


Why the derivation?

We require every handler to implement authenticate/challenge/forbid at a minimum, derivation makes this clearer, if we didn't require this, wouldn't that lead ppl to think they could only implement SignOut and use their handler?

davidfowl · 2017-06-17T16:37:53Z

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

+    /// <summary>
+    /// Used to determine if a handler supports SignIn.
+    /// </summary>
+    public interface IAuthenticationSignInHandler : IAuthenticationHandler


Why the derivation?

kevinchalet · 2017-06-20T18:34:08Z

So much sorcery in this PR... 😢

It would be much better to throw an InvalidOperationException when there's more than 1 scheme to encourage the developers to read the related documentation, so they can set the different schemes themselves.

kevinchalet · 2017-06-20T18:35:03Z

Related post: aspnet/Security#1267 (comment).

HaoK · 2017-06-22T18:25:05Z

Updated the default sign/signout to enforce the handlers implement the interface, otherwise they return null (should this throw instead?)

HaoK · 2017-06-22T18:25:18Z

We don't throw if the default scheme doesn't exist today, so it feels weird to throw

HaoK · 2017-06-22T18:29:39Z

We'll also make SignIn derive from SignOut which dervies in Auth

HaoK · 2017-06-22T20:47:49Z

Updated, SignInHandler is now the old uber AuthenticationHandler that has all methods implemented. Renamed None() => NoResult() with the bool property Nothing => None (done for Security)

Tratcher · 2017-06-22T21:00:37Z

src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs

        {
            if (_options.DefaultSignOutScheme != null)
            {
-                return GetSchemeAsync(_options.DefaultSignOutScheme);
+                return EnsureAssignable<IAuthenticationSignOutHandler>(await GetSchemeAsync(_options.DefaultSignOutScheme));


The type check is duplicated in SignOutAsync

Oh right, ok I can back out the extra complexity in the scheme provider then, and leave the type interface enforcement to the service

HaoK · 2017-06-26T18:29:10Z

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

        /// Indicates that there was no information returned for this authentication scheme.
        /// </summary>
        /// <returns>The result.</returns>
-        public static AuthenticateResult None()
+        public static AuthenticateResult NoResult()


@Tratcher how about we keep this None, but call it NoResult() on the ResultContext's that will get exposed to events?

That would leave you with a method and property that have the same name. NoResult is fine here.

These are currently called Nothing and None(), I just don't see much benefit renaming None => NoResult as we do have usages in the code internally already today (i.e. Identity), we can keep NoResult on ResultContext so the public API for events is still the same.

NoResult() and None are better than None() and Nothing.

HaoK · 2017-06-30T17:22:38Z

271faf1

dnfclas added the cla-already-signed label Jun 16, 2017

HaoK mentioned this pull request Jun 16, 2017

Fallback to DefaultAuthenticateScheme #872

Closed

This was referenced Jun 16, 2017

Pinpoint's Grand Eventure aspnet/Security#1266

Closed

Split SIgnIn/SignOut aspnet/Security#1267

Closed

Tratcher reviewed Jun 17, 2017

View reviewed changes

campersau reviewed Jun 17, 2017

View reviewed changes

davidfowl reviewed Jun 17, 2017

View reviewed changes

HaoK mentioned this pull request Jun 20, 2017

Remove AuthenticateResult.Handle()/Handled #871

Closed

Tratcher approved these changes Jun 22, 2017

View reviewed changes

HaoK commented Jun 26, 2017

View reviewed changes

HaoK added 7 commits June 27, 2017 14:33

Split SignIn/Signout into their own interfaces

8bf0bdc

Fallback to DefaultAuthenticateScheme

0d92b28

Updates

f8a2cfa

Correctly fallback for SignIn/SignOut

cd17a22

Ensure handlers implement signin/signout

2a2b27e

React

25fcbb5

Remove interface checks in SchemeProvider

45c82a3

HaoK force-pushed the haok/split branch from 9bb43bd to 45c82a3 Compare June 27, 2017 21:35

HaoK mentioned this pull request Jun 27, 2017

React to HttpAbstractions aspnet/SignalR#600

Closed

HaoK closed this Jun 30, 2017

HaoK mentioned this pull request Jul 27, 2017

Split SignIn/SignOut into IAuthenticationStore interface aspnet/Security#1244

Closed

HaoK deleted the haok/split branch August 7, 2017 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add defaults + split SignIn/SignOut #873

Add defaults + split SignIn/SignOut #873

HaoK commented Jun 16, 2017

HaoK commented Jun 16, 2017

HaoK commented Jun 16, 2017

Tratcher Jun 17, 2017

Tratcher Jun 17, 2017

Tratcher Jun 17, 2017

HaoK Jun 17, 2017

HaoK Jun 17, 2017 •

edited

Loading

HaoK Jun 17, 2017

Tratcher Jun 20, 2017

HaoK Jun 20, 2017

Tratcher Jun 17, 2017 •

edited

Loading

HaoK Jun 17, 2017

campersau Jun 17, 2017

campersau Jun 17, 2017

davidfowl Jun 17, 2017

HaoK Jun 17, 2017

davidfowl Jun 17, 2017

kevinchalet commented Jun 20, 2017

kevinchalet commented Jun 20, 2017

HaoK commented Jun 22, 2017

HaoK commented Jun 22, 2017

HaoK commented Jun 22, 2017

HaoK commented Jun 22, 2017

Tratcher Jun 22, 2017

HaoK Jun 22, 2017

HaoK Jun 26, 2017

Tratcher Jun 26, 2017

HaoK Jun 26, 2017

Tratcher Jun 26, 2017

HaoK commented Jun 30, 2017

Add defaults + split SignIn/SignOut #873

Add defaults + split SignIn/SignOut #873

Conversation

HaoK commented Jun 16, 2017

HaoK commented Jun 16, 2017

HaoK commented Jun 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

HaoK Jun 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

Choose a reason for hiding this comment

Tratcher Jun 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

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 commented Jun 20, 2017

kevinchalet commented Jun 20, 2017

HaoK commented Jun 22, 2017

HaoK commented Jun 22, 2017

HaoK commented Jun 22, 2017

HaoK commented Jun 22, 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

HaoK commented Jun 30, 2017

HaoK Jun 17, 2017 •

edited

Loading

Tratcher Jun 17, 2017 •

edited

Loading