Auth 2.0 Cleanup: Revisit cleanup events/context

[HaoK]

Sharing context for core IAuthenticationHandler and the events might be a bit awkward, also see if there's any improvements that can be made for the events as a whole

[kevinchalet]

@HaoK now that the authentication scheme is not longer stored in the options, retrieving it from the events is next to impossible. What do you think of introducing a Scheme property in BaseContext?

public class BaseContext
{
    protected BaseContext(HttpContext context, string scheme);

    public string Scheme { get; }
}

If BaseContext was moved to this repo, we could even expose it as a strongly typed property:

public class BaseContext
{
    protected BaseContext(HttpContext context, AuthenticationScheme scheme);

    public AuthenticationScheme Scheme { get; }
}

[kevinchalet]

@HaoK another question: do you remember why you've decided to rename BaseControlContext.SkipToNextMiddleware() to Skip()? The new name is not as clear as the old one, and doesn't really reflect what happens in practice.

[HaoK]

For events, we have: #1181 to revisit exactly that question.

The whole Skip/Handled thing is a bit weird, and what makes sense in some uses (SkipToNextMiddleware) makes sense when its being run in the HandleRequest of a callback for an auth request, but doesn't make sense anywhere else. We can use the same events issue to track revisiting that as well.

[kevinchalet]

For events, we have: #1181 to revisit exactly that question.

So this is the right place?

The whole Skip/Handled thing is a bit weird.

Weird... but insanely powerful 😅

What makes sense in some uses (SkipToNextMiddleware) makes sense when its being run in the HandleRequest of a callback for an auth request, but doesn't make sense anywhere else.

Sure, but doesn't the BaseControlContext name imply that the event should be used only where we have flow control (i.e from HandleRequestAsync) and where Skip really means "skip the default logic and invoke the rest of the pipeline"?

[HaoK]

Possibly, but I think we are using it in a variety events, not just ones that are invoked through the request processing pipeline.

[kevinchalet]

@Eilon I see you moved this item to preview3. If it can help make it happen sooner, I'd love to work on it and come up with a formal API change 😅

[Eilon]

@PinpointTownes we'd certainly be open to that. If you can share a concrete proposal then we can start the discussion! I don't recall the exact background of this issue, so I'm not entirely in the loop on what the requirements/proposals are here.

[HaoK]

Yeah feel free to submit a PR @PinpointTownes, I'm just slammed right now

[kevinchalet]

@HaoK will do, thanks!

Quick question: do you recall why the context classes derived from BaseAuthenticationContext class (i.e SignIn/SignOut/Challenge/ForbidContext) need to expose both the HttpContext and the authentication scheme?

It seems a bit weird, because both the context and the scheme are already injected into the handler via Task InitializeAsync(AuthenticationScheme scheme, HttpContext context);.

I may be wrong, but the context classes living in aspnet/HttpAbstractions don't seem to be really useful. We could probably get rid of them and simplify the signatures:

Task InitializeAsync(AuthenticationScheme scheme, HttpContext context);
Task<AuthenticateResult> AuthenticateAsync();
Task ChallengeAsync(ChallengeContext context);
Task SignInAsync(SignInContext context);
Task SignOutAsync(SignOutContext context);

->

Task InitializeAsync(AuthenticationScheme scheme, HttpContext context);
Task<AuthenticateResult> AuthenticateAsync();
Task ChallengeAsync(AuthenticationProperties properties);
Task SignInAsync(ClaimsPrincipal principal, AuthenticationProperties properties);
Task SignOutAsync(AuthenticationProperties properties);

[Tratcher]

They were on there because the base class is shared with the events (which it shouldn't be).

[kevinchalet]

So it's really just a side-effect of sharing the same base class when you shouldn't be, not an intentional decision to enable some sort of edge scenarios?

In this case, I guess we can remove all the context classes from aspnet/HttpAbstractions, right?

[HaoK]

Yeah I was feeling that way as well, the context classes were needed when we were doing the accept dance, but they have basically no value now, I can do that as part of my change to removing ChallengeBehavior

[kevinchalet]

I can do that as part of my change to removing ChallengeBehavior

FYI, I already revamped the IAuthenticationHandler methods signatures locally, but feel free to go ahead 😄

[HaoK]

You want to rebase and push your changes into the haok/forbid branch in aspnet/HttpAbstractions#842?

[HaoK]

I haven't updated any of the downstream repos for that change yet, so I might as well update them with this change at the same time

[HaoK]

ff9f145

[kevinchalet]

nit: I'm all for squashing PRs, but not giving public credits to folks who work on stuff you end up reusing in your own PRs and not even saying "thanks for your participation" is... meh.

[HaoK]

Yeah sorry, not sure how best to keep changes sane in a mega change like this, I did name the PR after you, I can give you credit in the announcement for the event changes section?

[brockallen]

Not sure where else to add this, and I didn't want to open a new issue. Here's the question:

Now that this has all been redone -- do you think it's better/cleaner than in 1.x? Have the problems been solved (while not introducing a lot of new ones -- this last bit is quite a concern for me)?

Unfortunately I don't have until August to actually look at the changes to review, at which time we'll start to update IdentityServer.

[HaoK]

Yeah, most of the problematic behaviors we were aware of have been eliminated (multiple automatic challenge, chaining) and regressions like those in: aspnet/Announcements#210 should be much less likely going forward.