[Prototype] Revisit the events model #1243
Conversation
|
@PinpointTownes, |
|
@HaoK if you see things that should be changed/reverted, feel free to directly push commits to my forked branch (you have write access). |
|
Cool I'll try to take a look at this sometime this week |
|
@PinpointTownes looks good in general, one minor tweak suggestion, can you try dropping Base from everything other than BaseContext, since we no longer have the other context's in Http, these can just be the ChallengeContext/AuthenticationContext/RemoteAuthenticationContext, etc. |
|
@HaoK thanks! That's a good idea. TBH, I'm not super happy with the context classes names mainly because things like If you (or @Tratcher) have an idea, don't hesitate 😄 |
|
So BaseChallengeContext and BaseAuthenticationContext are siblings? What's the split of responsibility between these two? |
|
There are currently 6 base context classes in this PR:
We could probably merge |
|
Maybe something like this? BaseControlContext => HandleRequestContext? |
|
@Tratcher thoughts? |
|
Needs a UML diagram... I prefer @PinpointTownes's names over @HaoK's. |
The VS 15.3 Community Preview I've installed doesn't seem to have the new tooling (likely an Enterprise feature) so if one of you could generate the diagram, that would be great 😅 |
|
Yeah I think the names are fine in general, just that the Base prefixs can be omitted without any loss of clarity |
|
There's still one problem: we need to find a name for the base class used by context classes like What about merging |
|
I believe we haven't used EventContext or HandlerContext if you need a generic-y name that covers multiple things |
If you like very generic things, I guess something like @Tratcher when you have a sec', please let us know your preferences. Should the If you like the general idea, I'll update the PR, add the missing XML docs and remove the unnecessary logger extensions. |
|
Out of those options that you listed, I'd vote for Event or Handler + Stop() without the extra word. We don't have base in any of our auth handler base classes, so its weird to have base in these. To be fully consistent we could rename BaseContext to something like SchemeContext |
The only concern I have with such a generic name is that it gives the impression it will be used as the super base class for every event, which is not true (it will only be used for events that need to expose the authentication properties and a way to stop the default logic processing). |
|
Okay, so are these only used for IAuthenticationRequestHandlers events? If so HandleRequestEvent could signal that they should only be used in the context of a handle request |
|
@HaoK nah, for flow control events, we have We only need a new name for |
|
Ok so this is the NotRemoteAuthenticationContext basically :) |
|
lol yeah, but we already have |
|
I think we've proven that this stuff is STILL confusing :) |
|
This is kinda where I was going where I honestly prefer extension via derivation of virtuals... but I get this is much easier for people to plug in... but the naming on these context objects is a nightmare... |
|
Maybe we just have duplication, and name the context to exactly match the virtual/event its used in...Its not the end of the world to have a SignIn + Challenge context, its clear what they are used for that way right? |
Yeah. But luckily, it's only a nightmare for us (at least if we want to avoid code duplicate by having base context classes). The final user experience is much better 😅
That would be totally fine if we didn't have events classes like public virtual Task RedirectToIdentityProvider(RedirectContext context) => OnRedirectToIdentityProvider(context);
public virtual Task RedirectToIdentityProviderForSignOut(RedirectContext context) => OnRedirectToIdentityProviderForSignOut(context);public virtual Task RedirectToLogout(CookieRedirectContext context) => OnRedirectToLogout(context);
public virtual Task RedirectToLogin(CookieRedirectContext context) => OnRedirectToLogin(context);
public virtual Task RedirectToReturnUrl(CookieRedirectContext context) => OnRedirectToReturnUrl(context);
public virtual Task RedirectToAccessDenied(CookieRedirectContext context) => OnRedirectToAccessDenied(context);Instead of trying to merge
|
|
@PinpointTownes yeah this naming is basically for us, since end users are using the delegates they don't see the context names, so lets just be pragmatic, I think its okay if the cookie/redirect events share a common type, but we have special Challenge/SignOut contexts if we can't come up with a sensible name. I'm guessing you really like the Base prefix since you keep including it in your examples :) |
|
@PinpointTownes I had an idea you can try, since you made all of these generic anyways, instead of adding Scheme/Options, can you try what making it generic against THandler looks like instead? That is basically a superset of Scheme/Options. As part of that, you can make all the handler types public too, fixing that other issue... That also makes HandlerContext the correct name for whichever Context contains the THandler... |
Haha no, it's simpler: I just copied/pasted the names from previous posts to avoid having to re-type them
I had to remove the .NET Core 2.0 bits temporarily 'cause my VS setup was broken but I'll give them another try this weekend. That said, not sure I'm a huge fan of what you suggest, as it will basically require adding a new generic argument to
Given that middleware used to be public (but are now gone), I suppose it's the right thing to do. That said, it probably deserves a separate issue/PR so we have a chance to discuss the public API (I'm tempted to think most/all the helpers methods should be private and that folks shouldn't be encouraged to subclass the handlers, IMHO). |
|
Yeah so I had something fairly drastic in mind The benefit I saw was that we wouldn't need to add a TOptions because THandler would have the properly typed Options property already, and it would already have Scheme as well. The minor downside is that event handler would have to dig a bit deeper to get the Options, context.Handler.Options, but seems fine. So with Auth 2.0, the handlers are top level concepts and should be made public, as derivation should really be a first class supported scenario. I agree with you that most of that should be left to a different PR (where we rationalize/clean up the properties/methods as appropriate). But if we go the HandlerContext route we should make the types public in the same PR. |
…ng from AuthenticateResultContext
| { | ||
| Logger.RemoteSignOutHandledResponse(); | ||
| return true; | ||
| } | ||
| if (remoteSignOutContext.Skipped) | ||
| if (remoteSignOutContext.State == EventResultState.SkipToNextMiddleware) |
There was a problem hiding this comment.
Nice, now can we rename SkipToNextMiddleware => Skipped, and HandleResponse => Handled
There was a problem hiding this comment.
And have Skipped vs Bypassed? I'm generally all for using short names, but what you suggest is way too generic for such semantic.
There was a problem hiding this comment.
I don't think you've convinced us for the need for Bypass yet.
There was a problem hiding this comment.
Introducing Bypass certainly isn't a blocker for all of your other changes as it doesn't exist today...
There was a problem hiding this comment.
Doesn't it seem better to fix the handlers themselves to make it easy to opt-out of any default behaviors, as opposed to having some framework support for that
There was a problem hiding this comment.
Oh I think I see the problem, I missed the fact that we were now pushing these concepts onto all of our middleware, today only OIDC + JwtBearer expose Control contexts... yeah so we don't want to spread this out.
That's absolutely not the objective of this PR and I believe the current state reflects it (I will submit a separate issue/PR so we can discuss making the events consistent across the stack, but that's a totally different story).
This PR has only two goals: adding the options/scheme to the base context class and removing HandleResponse/SkipToNextMiddleware from every context where it doesn't make any sense to replace it by a BypassDefaultLogic concept that does exactly the same thing as in 1.0/1.1 but with a better name and a way better intellisense story.
There was a problem hiding this comment.
which is why it does require a lot of code
Except this scenario is quite frequent and yet is still extremely painful: ExternalLoginSignInAsync doesn't take a ClaimsPrincipal/claims collection so you pretty much have to rewrite everything it internally calls: SignInOrTwoFactorAsync, StoreTwoFactorInfo and SignInAsync. It's a total nightmare for such a simple thing.
There was a problem hiding this comment.
So looking at the current names,
HandleRequestContext => BaseControlContext since this isn't actually something that all RemoteAuthenticationHandlers use.
RemoteAuthenticateResultContext => AuthenticateResultControlContext which is only used by OIDC/JwtBearer events.
There was a problem hiding this comment.
RemoteAuthenticateResultContext => AuthenticateResultControlContext which is only used by OIDC/JwtBearer events.
RemoteAuthenticateResultContext is not used by the JWT handler. It's purely specific to handlers that implement IAuthenticationRequestHandler AND produce tickets, like the RAH-derived handlers.
There was a problem hiding this comment.
@HaoK now that the objective is clear for you, may I suggest you make another full pass to see if you still have concerns?
| @@ -11,13 +11,18 @@ public enum EventResultState | |||
| Continue, | |||
|
|
|||
| /// <summary> | |||
| /// Bypass the default logic. | |||
| /// </summary> | |||
| BypassDefaultLogic, | |||
There was a problem hiding this comment.
I think maybe Noop is better than bypass. That's what you want right? For the event to be an explicit Noop
…eSignOutAsync()/HandleChallengeAsync()
|
Moving this out of a line comment
I'll let @Tratcher chime in with his thoughts, but from my perspective, my main concern is that all this Handle/Skip/Bypass stuff stays limited in scope to only the OIDC/JwtBearer events that already have these concepts and doesn't bleed out. |
| } | ||
|
|
||
| public EventResultState State { get; protected set; } |
There was a problem hiding this comment.
Yeah so this is a no go. We aren't putting this control flow everywhere.
There was a problem hiding this comment.
This is exactly the bleed I was talking about :)
There was a problem hiding this comment.
This complexity is fine in the context of JwtBearer + OIDC since they use it already, none of the other auth middleware should be exposed to this
There was a problem hiding this comment.
Bleed is really excessive, sir.
State was just moved from BaseControlContext (which is absolutely not OIDC-specific) to BaseContext and I added the BypassDefaultLogic member.
The default value is Continue - which is exactly what ALL the events do by default when you implement them. The property setter is protected and ONLY context classes that expose HandleResponse/SkipToNextMiddleware methods can change this state.
There was a problem hiding this comment.
Well my prior comment about keeping this limited to OIDC/JwtBearer events was basically exactly this :) Keeping Control state concepts/properties away from the other events that don't have them.
There was a problem hiding this comment.
Not a huge fan of that as it ties the base BaseControlContext/HandleRequestContext class to the RAR model, which is not the only one using it (ASOS uses BaseControlContext for all its events and yet it's not a RAH handler).
What's wrong with the enum?
There was a problem hiding this comment.
Rename/make RemoteAuthenticationResult => HandleRequestAuthenticationResult and be the only concept. Enums are annoying since if we do want to add something like a Bypass, we can't do it until a major version, unlike results where we can add a new property whenever we want and use it immediately. We want to only have a single way to do flow, not two (Result + enum)
The public API on the contexts are the same and there's no more enum, and internally we are consistent with only results.
There was a problem hiding this comment.
You'll still be able to have ASOS derive only from HandleRequestContext to only expose Handle/Skip
There was a problem hiding this comment.
Enums are annoying since if we do want to add something like a Bypass, we can't do it until a major version
What? Adding an enum member is not a breaking change (unless you change the associated value of the existing members, obviously).
There was a problem hiding this comment.
Rename/make RemoteAuthenticationResult => HandleRequestAuthenticationResult and be the only concept.
There are events that don't produce authentication tickets and for which HandleRequestAuthenticationResult.Success()/HandleRequestAuthenticationResult.None() or HandleRequestAuthenticationResult.Fail() make absolutely no sense (ASOS has many events like that).
EventResultState is way more neutral/generic.
I dedicated a blog post about this pattern: http://kevinchalet.com/2016/07/13/creating-your-own-openid-connect-server-with-asos-creating-your-own-authorization-provider/ |
|
The new semantic is sooooooooo much nicer and wayyyyy clearer: JWTBefore:services.AddJwtBearerAuthentication(options =>
{
options.Events.OnMessageReceived = context =>
{
if (context.Token == "skip-authentication")
{
// Calling this method skips the default logic but doesn't invoke
// the next middleware because the JWT handler doesn't handle requests.
context.SkipToNextMiddleware();
}
else if (context.Token == "reject-authentication-with-error")
{
// Impossible with context.SkipToNextMiddleware() or context.HandleResponse().
}
else if (context.Token == "complete-authentication")
{
// Calling this method skips the default logic but can't prevent other stuff from being invoked.
context.Ticket = new AuthenticationTicket(new ClaimsPrincipal(), null, context.Scheme.Name);
context.HandleResponse();
}
return Task.CompletedTask;
};
});After:services.AddJwtBearerAuthentication(options =>
{
options.Events.OnMessageReceived = context =>
{
if (context.Token == "skip-authentication")
{
context.SkipAuthentication();
}
else if (context.Token == "reject-authentication-with-error")
{
context.RejectAuthentication("Error message");
// Or...
context.RejectAuthentication(new Exception("Error message"));
}
else if (context.Token == "complete-authentication")
{
var ticket = new AuthenticationTicket(new ClaimsPrincipal(), null, context.Scheme.Name);
context.CompleteAuthentication(ticket);
}
return Task.CompletedTask;
};
});OIDCBefore:services.AddOpenIdConnectAuthentication(options =>
{
options.Events.OnMessageReceived = context =>
{
if (context.Token == "skip-authentication")
{
// Calling this method skips the default logic but doesn't invoke the next
// middleware because IsProcessingComplete() returns a None result in this case.
context.SkipToNextMiddleware();
}
else if (context.Token == "reject-authentication-with-error")
{
// Impossible with context.SkipToNextMiddleware() or context.HandleResponse().
}
else if (context.Token == "complete-authentication")
{
// Calling this method skips the default logic but can't prevent other stuff from being invoked.
context.Ticket = new AuthenticationTicket(new ClaimsPrincipal(), null, context.Scheme.Name);
context.HandleResponse();
}
else if (context.Token == "skip-to-next-middleware")
{
// Impossible with context.SkipToNextMiddleware() (despite its name...).
}
else if (context.Token == "handle-response")
{
// You have to make sure context.Ticket is null. Otherwise, the response
// is NOT handled (an AuthenticateResult.Success is returned!).
context.Ticket = null;
context.HandleResponse();
}
return Task.CompletedTask;
};
});After:services.AddOpenIdConnectAuthentication(options =>
{
options.Events.OnMessageReceived = context =>
{
if (context.Token == "skip-authentication")
{
context.SkipAuthentication();
}
else if (context.Token == "reject-authentication-with-error")
{
context.RejectAuthentication("Error message");
context.RejectAuthentication(new Exception("Error message"));
}
else if (context.Token == "complete-authentication")
{
var ticket = new AuthenticationTicket(new ClaimsPrincipal(), null, context.Scheme.Name);
context.CompleteAuthentication(ticket);
}
else if (context.Token == "skip-to-next-middleware")
{
context.SkipToNextMiddleware();
}
else if (context.Token == "handle-response")
{
context.HandleResponse();
}
return Task.CompletedTask;
};
}); |
|
Yeah its better, but the naming should be consistent with the |
We don't have to, as they are independent (you can create a RAH without using any event and you can create a handler using the control events without using RAR). And honestly, If you believe you can't live with that, I'd prefer keeping the current context APIs and updating |
Naming is always important, we should have one good name for the context/result methods and they should be the same :) We should pick a name for each concept carefully now, since we will be living with it until 3.0... |
They won't be for long, since the enum is going away :) |
|
For names, after seeing your code, I agree that we should distinguish the HandleRequest APIs vs the AuthenticateResult apis. SkipRequest while the AuthResult ones should align with the AuthenticationResult names (which we can still rename as well) |
|
I've already dedicated much more time to this PR than I had originally planned and it's now clear that we're repeating the same things again and again and that finding a compromise that would make everyone happy is impossible at this stage. If you think you can come up with a better design/better names, don't hesitate. For my part, I give up here |
|
@PinpointTownes Thanks for your work on this, I assume you are fine I start with your PR as a starting point? I think we'll end up with pretty much the same context API you wanted. |
I'm not sure I understand what's the point of doing that if you end up with the same/similar API shape, but why not, so... do what you want. |
|
My 2-cent: since you're not familiar with the events model, consider taking a look at how people use it in real world apps. GitHub is full of such apps, so it shouldn't be too complicated. After that, maybe you'll realize it's worth taking a few scenarios into account in your own design 😅 |
|
I'm almost done with the tweaks to your PR that I had in mind, its honestly not all that different for apps (ignoring the names part). Again I was never pushing back against your usage/scenarios, just the naming and some implementation details, the spirit of what you wanted should really be the same after my tweaks, and I think prefer the implementation this way over the enum |
Things I've changed:
AuthenticateResultwas split into 2 classes:AuthenticationResult(living in the abstractions repo) andRemoteAuthenticationResult(living here).AuthenticateResult.Handle()/Skip()was moved toRemoteAuthenticationResult.HandleResponse()/SkipToNextMiddleware()are no longer available from events that don't offer real flow control (basically all the events that are not called fromHandleRemoteAuthenticationAsync().HandleResponse()/SkipToNextMiddleware()for something else that real flow control now haveCompleteAuthentication/SkipAuthentication/RejectAuthentication(or the equivalent for challenge/signout.Please note that it's a very early prototype 😄
/cc @HaoK @Tratcher