Split SIgnIn/SignOut #1267
Split SIgnIn/SignOut #1267
Conversation
|
@HaoK, |
|
|
||
| properties = properties ?? new AuthenticationProperties(); | ||
| await HandleSignInAsync(user, properties); | ||
| Logger.SignedIn(Scheme.Name); |
There was a problem hiding this comment.
So we must now do that in EVERY handler? What a bummer 👎
There was a problem hiding this comment.
You actually have a handler that implements sign in?
There was a problem hiding this comment.
ASOS does (exactly like OAuthAuthorizationServerMiddleware used to).
We use that for both the authorization and token endpoints: http://kevinchalet.com/2017/01/30/implementing-simple-token-authentication-in-aspnet-core-with-openiddict/
There was a problem hiding this comment.
So yeah if you want to log that you signed in, you need to log it yourself (and granted the logging extensions code is ridiculously gross)
There was a problem hiding this comment.
But I imagine you can just add a Logger.Log at the top of your method is the only change for you, and implement the interface method directly instead of HandleSignIn
There was a problem hiding this comment.
At least, do we get a sensible advantage if we split IAuthenticationHandler? 😅
There was a problem hiding this comment.
SignIn/SignOut default will be more likely to be correct in applications with only a single cookie configured So OIDC + Cookie won't need to specify DefaultSignInScheme anymore, well once we actually change RemoteAuthenticationHandler to stop requiring SignInScheme now that it will work.
There was a problem hiding this comment.
Heh, pretty sure I said "advantage"... making the default scheme selection more magical than it is already is not what I'd call an advantage.
I can already hear my users say "oh, my cookies+Facebook app worked fine before I added OpenIddict and now, I have an exception"... 😢
|
Rolled into #1266 |
@Tratcher
Reacts to aspnet/HttpAbstractions#873
I'll merge this into the events PR, but easier to review these seperately first