Make it easier to specify a default scheme

[danroth27]

If I just want to use cookies I have to specify cookies as the default scheme three times:

            services.AddAuthentication(sharedOptions =>
            {
                sharedOptions.DefaultAuthenticateScheme = CookieAuthenticationDefaults.AuthenticationScheme;
                sharedOptions.DefaultSignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;
                sharedOptions.DefaultChallengeScheme = CookieAuthenticationDefaults.AuthenticationScheme;
            });

Can we make this less verbose? Maybe add an AddAuthentication overload that takes a single default scheme parameter? Or maybe add a AuthenticationOptions.DefaultScheme property? Or add more fallback behavior?

[Tratcher]

DefaultChallengeScheme could fall back to DefaultAuthenticateScheme. DefaultSignInScheme needs to stay independent though.

[Eilon]

Let's make these all ultimately fall back to sharedOptions.DefaultAuthenticateScheme. So, if DefaultSignInScheme or DefaultChallengeScheme are being read by a consumer (e.g. in HttpAbstractions) and they're not set, the calling code should fall back to reading DefaultAuthenticateScheme. This is similar to how we already do fallback for other properties.

[kevinchalet]

Let's make these all ultimately fall back to sharedOptions.DefaultAuthenticateScheme.

Sounds dangerous for at least DefaultSignInScheme (and DefaultSignOutScheme) when there's only one handler registered (e.g OIDC):

• If the handler doesn't override HandleSignInAsync() (like OIDC), the sign-in part will be basically no-op but the user will never know why his app is not working (with the 1.0/1.1 bits, you have a message telling you that something is wrong with the configuration).

• If the handler overrides HandleSignInAsync() and calls SignInAsync(SignInScheme), it will result in an endless recursive call and eventually, a stack overflow exception.

[Tratcher]

I remembered signin throwing by default... Looks like that doesn't happen in the base class, we should change that.

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

Line 195 in 879f0b7

return Task.CompletedTask;

Some providers do throw:
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.JwtBearer/JwtBearerHandler.cs#L340

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

Line 184 in 879f0b7

throw new NotSupportedException();

[kevinchalet]

@Tratcher looks like it throws for RAH-based handlers: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs#L184

Unfortunately, it doesn't change anything to the fact that...

1. The exception message is totally useless to debug what's going on.
2. There's a real SOE risk if the handler writer overrides it to do return Context.SignInAsync(SignInScheme); exactly like you did with HandleForbiddenAsync: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs#L186

[Tratcher]

@PinpointTownes but do any handlers actually override SignIn except cookies?

[kevinchalet]

In the built-in handlers? I don't think so. That said, you never know how people implement things. Having such a trap in place would be a very nasty surprise.

When you think about it, it doesn't make sense for a handler to have a sign-in scheme that points to itself.

[kevinchalet]

It would even be a good idea to add a check somewhere in the RAH thing to ensure SignInScheme/SignOutScheme are never equal to AuthenticationScheme.

[Tratcher]

Those already throw NotSupported, so I'm not as concerned. If we actually see people break it then we can add mitigations as needed.

[kevinchalet]

It's not just about breaking things, it's about the user experience.

As a user, if do something like that:

services.AddAuthentication(options =>
{
    options.DefaultAuthenticateScheme = FacebookDefaults.AuthenticationScheme;
    options.DefaultSignInScheme = FacebookDefaults.AuthenticationScheme;
    options.DefaultChallengeScheme = FacebookDefaults.AuthenticationScheme;
});

... there's nothing that tells me that my configuration is totally invalid. It's very hard for beginners to figure out what's going on if they don't register the right strings here.

[Tratcher]

Except there is, Facebook throws NotImplemented when it tries to call SignIn.

This class of error has always been possible, all the way back to Katana, but we haven't seen people actually do it.

[kevinchalet]

Except there is, Facebook throws NotImplemented when it tries to call SignIn.

So when folks see a NotImplemented exception, they are supposed to think... "oh, I can't use the same value for DefaultAuthenticateScheme and DefaultSignInScheme"?

This class of error has always been possible, all the way back to Katana, but we haven't seen people actually do it.

That's the problem with you, guys. You're so focused on your own framework that you have no idea how people use it concretely (I don't blame you for that, it's really common).

Posted yesterday on SO: https://stackoverflow.com/questions/44566998/asp-net-core-google-authentication (I took me 10 seconds to find it using the SO search engine):

var googleOptions = new GoogleOptions
{
    AuthenticationScheme = "Google",
    ClientId = "clientid",
    ClientSecret = "cs",
    SignInScheme = "Google"
};

app.UseGoogleAuthentication(googleOptions);

[Tratcher]

Fair, we primarily hear about issues reported directly to us, though we do browser SO.

[HaoK]

This has convinced me that we should just do #1244 now, if we split SignIn into IAuthenticationSignInHandler and SignOut into IAuthenticationSignOutHandler, most of this issue goes away as you will get a proper exception saying "Google" does not support SignIn.

[brockallen]

Except there is, Facebook throws NotImplemented when it tries to call SignIn.

But what about SignOut?

[HaoK]

SignIn/SignOut/Forbid all throw for the remote handlers. It's just going to change from NotImplemented in the handler to InvalidOperation from the AuthService

[HaoK]

SignOut is only valid for IAuthenticationSignOutHandlers, you can't blindly call sign out on everything.

[brockallen]

SignOut is only valid for IAuthenticationSignOutHandlers, you can't blindly call sign out on everything.

IIRC, I thought I opened an issue in 1.x on this, and it was agreed that it's probably ok to nop instead.

[HaoK]

If you really want to blindly call it, you can always just wrap it in a try/catch ignore InvalidOperationException

[brockallen]

Yea, that's what we already do now. It's not pretty.

[HaoK]

That was in 1.x, in 2.0 we are stricter

[HaoK]

Can you explain your scenario again, why don't you know what kind of scheme it is?

[HaoK]

Basically the only 2 things which support sign out are OIDC (remote sign out) and cookies. Nothing else will work

[brockallen]

Because we've built an OIDC provider that allows arbitrary upstream IdPs and we don't know what style of IdP has been configured. I can dig out the original issue...

[brockallen]

Basically the only 2 things which support sign out are OIDC (remote sign out) and cookies. Nothing else will work

And WS-Fed?

[brockallen]

Original issue: #1039

[HaoK]

So in 2.0 you can always just drop in your own IAuthenticationService which no-ops instead of throws if the handler doesn't exist...

[HaoK]

To summarize where things look in the current PRs:

DefaultAuthenticateScheme => only defaults to a single auth handler, otherwise null
DefaultChallengeScheme => defaults to authenticate scheme
DefaultForbidScheme => defaults to challenge scheme
DefaultSignInScheme => defaults to single sign in handler, otherwise defaultAuthenticateScheme
DefaultSignOutScheme => defaults to single sign out scheme, otherwise defaultSignInScheme

So its now unlikely to ever need to set more than DefaultAuthenticateScheme + DefaultChallengeScheme, as the scenario with multiple cookies usually will set DefaultAuthenticateScheme.

[HaoK]

Another noteworthy side effect is that SignInScheme is no longer required for RemoteAuthenticationHandlers (altho we basically always used DefaultSignInScheme to set that anyways today)

[HaoK]

ff9f145