Signout for google throws

[brockallen]

In Katana when you called Signout for social providers, it no-op'd. In Core, it now throws since the default base class has no implementation (not supported exception). Perhaps it should not throw. // @blowdart

[Tratcher]

This was an intentional change to highlight that signout is not supported for some providers. Cookies and OIDC are the only ones that support sign-out right now.

[brockallen]

Yea, sucks for writing a generic logout logic then. It needs to be wrapped in try/catch for NSE.

[blowdart]

Funny I was talking to Dominick about this last night. I'm actually with Brock and Dominick on this one, because it's a horrible shock to folks upgrading from Katana. I'd lean towards no-op, with logging as a warning, rather than throwing an exception

[Tratcher]

But why are you calling SignOut on random providers? Challenge is the only API directly supported by all providers. For 1.0 we also made Authenticate pass through the OAuth providers so it looks like it works. But SignIn and SignOut? Cookies is the only one that really supports either, and we re-purposed SignOut on OIDC for remote sign-out. I don't see how SignOut can make sense for Windows auth, Bearer, Basic, etc. any more than it does for SignIn.

Why not have SignOut(Google) pass through to SignOut(Cookies) like we do for Authenticate? Then at least it would do what you asked.

[brockallen]

Because in our token service, all we track the middleware used for external authentication as a claim. At signout time we need to trigger it, but we don't know if it was social or otherwise (such as AAD, ADFS, etc) See here: https://github.com/IdentityServer/IdentityServer4/blob/dev/src/Host/Controllers/AccountController.cs#L207

[Tratcher]

Ok, so you're trying to trigger the remote sign-out which is only implemented for OIDC right now. WsFed had this as well(?), but none of the other providers implement it. Then you follow up by deleting the auth cookie which makes sense.

You never do this for the persistent auth types correct? Windows, Bearer, Basic, etc?

[brockallen]

There is no UI for Bearer. Windows wouldn't make sense, since logging out would mean signing out of the OS. Basic... no idea. That's not terribly relevant for IdentityServer (in the UI at least).

[Eilon]

From @blowdart

I'd lean towards no-op, with logging as a warning, rather than throwing an exception

We discussed this and this seems reasonable.

Would it make sense for the social providers to pass through and call SignOut on the cookie auth provider?

And then what should Windows auth (throws) and Bearer auth (throws) do for SignOut?

[brockallen]

Would it make sense for the social providers to pass through and call SignOut on the cookie auth provider?

I'd say no, as apps today are already aware that they are expected to clear their own cookie, plus it's hard to know what cookie middleware was ultimately used. In short, leave this up to the app (as it already is today).

[brockallen]

And then what should Windows auth (throws) and Bearer auth (throws) do for SignOut?

Nop as well, I'd imagine, for the same reasons the social ones do.

[blowdart]

@Eilon Yea, what brock said, because you have no idea if cookie middleware is even there.

[Eilon]

Yep this makes sense.

[Eilon]

@HaoK can you close this because this should be taken into account in the Big Auth Redesign of 2017.

[brockallen]

But despite the Big Auth Redesign of 2017, the existing authentication infrastructure will remain, no? So, IMO, this should still be fixed.

[Eilon]

@brockallen given the redesign stuff, we don't have plans to do a further release of the current design (aside from patches, which are non-breaking).

[HaoK]

So we do have the opportunity to pick the new behavior for the new stack. @blowdart @Tratcher throw or no throw in the RemoteAuthenticationHandler for SignIn/SignOut (as it stands the code ported from 1.0 still throws NotSupportedException). Personally I could go either way on this

[Tratcher]

Just no-op then.

[HaoK]

Tracked via #1179