SignOut() without args is broken

[davidfowl]

This logic passes an empty array for authenticationSchemes which blows up now.

Mvc/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

Line 1606 in 3e8cd1e

public virtual SignOutResult SignOut(params string[] authenticationSchemes)

/cc @HaoK

[HaoK]

Potential fixes would be either adding a new DefaultSignOutScheme to Auth, or adding a check to require at least 1 scheme is specified.

cc @Tratcher

[Tratcher]

There already is a count check:

Mvc/src/Microsoft.AspNetCore.Mvc.Core/SignOutResult.cs

Line 99 in 3e8cd1e

if (AuthenticationSchemes.Count == 0)

DefaultSignOutScheme, with a fallback to DefaultSignInScheme may be the best option.

[HaoK]

Is this really broken then? The error message is clear I would assume given the resource name: "MustSpecifyAtLeastOneAuthenticationScheme". Neither of the default schemes are required to be set, so this exception would be still sometimes get hit.

We don't currently have an extension method for SignOut without a scheme either (https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticationHttpContextExtensions.cs#L151). We should be consistent regardless.

If we do add a DefaultSignOutScheme that falls back to DefaultSignInScheme, we should probably just add one for Forbid too so we have one for each method on IAuthenticationService.

[davidfowl]

We should be consistent regardless

This is what surprised me. The fact that the extension method required a string and the SignOut method on the MVC controller didn't.

[HaoK]

I guess there's another option too, which is eliminating the ability to pass in an arbitrary number of schemes, and only have 2 SignOutResult ctors: (string) and (string, AuthenticationProperties)

[HaoK]

Downside would be this would be a breaking change for MVC

Well I found Pinpoint's PR that added these, looks like we did go back and forth and all around on this #4259 (comment)

I knew we debated these overloads...bleh

[HaoK]

Heh we still have an open issue related to this in HttpAbstractions even aspnet/HttpAbstractions#585

[HaoK]

cc @PinpointTownes since it was initially his sugar PR

[kevinchalet]

I guess there's another option too, which is eliminating the ability to pass in an arbitrary number of schemes, and only have 2 SignOutResult ctors: (string) and (string, AuthenticationProperties)

Nooooooo, please. I use this syntax almost in every app I develop 😅
It's super useful to clear a local authentication cookie and trigger a remote signout.

E.g:

[HttpPost("~/connect/logout"), ValidateAntiForgeryToken]
public async Task<IActionResult> Logout()
{
    return SignOut(CookieAuthenticationDefaults.AuthenticationScheme,
                   OpenIdConnectDefaults.AuthenticationScheme);
}

[brockallen]

Signout is harder than signin

[HaoK]

Ok, since the sugar is useful, we could always do something kinda weird with SignOut in BaseController to require at least one scheme...

public virtual SignOutResult SignOut(string authenticationScheme, params string[] moreSchemes)
public virtual SignOutResult SignOut(AuthenticationProperties properties, string authenticationScheme, params string[] moreSchemes)

This forces a scheme and still lets you specify as many as you want...

[kevinchalet]

Adding a DefaultSignOutScheme/DefaultForbidScheme as you initially suggested sounds way better, IMHO.

[HaoK]

So to summarize the new behavior:

• Add Defaults for SignOut/Forbid (which fallback to Challenge/SignIn)
• Remove the checks in MVC that require at least 1 sign out scheme.
• SignOut() no args will still blow up with defaults are set, it will just blow up in the Auth service instead of the result with a different error message.

@davidfowl does that address your concerns?

[kevinchalet]

@HaoK you get a bonus point if you add a parameterless constructor for SignOutResult and a parameterless SignOut() overload 😄

[Eilon]

@HaoK - what further work do we need to do in MVC?

[HaoK]

I'll submit a PR, just need a few overloads I think

[rynowak]

Is this on track for 2.0.0? What's the importance of this?

[HaoK]

The MVC changes are just minor sugar, the auth changes that these are dependent on will go in today/tomorrow

[HaoK]

eeee3ef