Switched to events model to DI of event type

[drusellers]

Description

while working on some JWT code, I'd like to set the HttpContext.Items with a hydrated user. This follows a similar pattern seen in the Cookie and various other authentication systems.

We introduce a new JwtAuthenticationEvents type, that mirrors the exact same events. Following the Cookie Authentication I moved all of the OnXxxx to the Events object. I added [Obsolete] tags to the old OnXxx methods with instructions about what to use instead.

If we want we can also extend the OAuthEvents if that suits us as well.

I need to figure out how to best test this, but this is a pattern I've used to build my own Basic auth and MagicLink auth. Before I go any further I wanted to check in with the team on this direction.

The goal is to be able to inject my DbContext into the handler, so that I can populate the HttpContext.Items

.AddJwt(JwtAuthenticationDefaults.AuthenticationScheme, options =>
    {
        options.VerifySignature = true;
        options.EventsType = typeof(MyJwtAuthenticationEvents);
    });

public class MyJwtAuthenticationEvents : JwtAuthenticationEvents 
{
    readonly MyDbContext _context;

    public MyJwtAuthenticationEvents(MyDbContext context)
    {
        _context = context;
    }

    public override AuthenticateResult OnSuccessfulTicket(ILogger logger, AuthenticationTicket ticket)  
    {
        // do database stuff here
    }
}

Checklist

• Tests added
• Version bumped
• Changelog updated

[abatishchev]

Hi Dru, thank you for the contribution! Let me understand the rationale a bit better, I'll come back with questions.

[abatishchev]

Why options.EventsType is of type Type? How it's used later?

Can you post your old and new code side-by-side. I'm all for a better usage experience, so want to see understand how does the proposed change improve it.

[drusellers]

Why options.EventsType is of type Type? How it's used later?

It's of type Type because its used by the Microsoft DI to resolve an instance of the Type from the container. This is the crux of the work. Allowing me to set the instance of Events to an instance out of DI that is of type EventsType lets me customize it and get access to things like DbContext.

[drusellers]

.AddJwt(JwtAuthenticationDefaults.AuthenticationScheme, options =>
{
    options.OnSuccessfulTicket = (logger, ticket) =>
    {
        // ARGH - I can get at the DbContext
    };
});
// later
builder.Services.AddTransient<DoTheWorkHereAsAWorkAround>();

[abatishchev]

Sounds like the right strategy then. Thank you again! Please add some tests and other paperwork (version bump, changelog)

[drusellers]

@abatishchev ok, so the next step that I'd like to discuss is moving the events to a "context" approach - but I'm also happy to make that a different PR. The idea is to copy what we see in other authentication handlers like you see here:
https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/OAuth/src/OAuthHandler.cs#L256

The big things that I think this adds which is nice.

1. moving to a single context parameter allows us to add more data to the context without breaking the method signature
2. we can provide the HttpContext which has IoC on it via RequestServices so people can use the lighter OnXxxx methods before a full upgrade to an IoC type.
3. we can also pass the AuthenticationProperties which is a common data bag for the authentication pipeline.

I hesitate to take this move in this PR because it would break the API. I can get what I need with just what we have. But to bring it even closer to the GoogleAuthenticationHandler this would be the next step I'd like to do - but happy to do it in another PR.

Also, when we are done, I can squash all of these commits into one clean commit. Is that preferred?

[abatishchev]

I like the approach (especially given it's a pattern) and don't mind a breaking change (ideally of course we just mark old things as [Obsolete] first. We would just need to jump the major version

[drusellers]

Ok, it'll be a bit before I get to it but I'll put the context objects in. I'll chew on how to best make the old stuff still work. It should totally be doable. Just need to think on it. :)

[abatishchev]

We also can check in the current PR, just without releasing the package Either way, sounds good to me!

[abatishchev]

Or what I usually did earlier, bump the major version but suffic it with let's say beta1. And 2, and so on until it's stable enough.

[drusellers]

@abatishchev ok, with this latest push we have context classes with more data available. YAY! The old methods are still supported, although right now its either/or. You can't do both. I'd be game to do a beta especially if that turns into a NuGet I can pull in. Then I can run this in my system for a bit to look for any other goodies.

[drusellers]

Ok, did a review of AuthenticationProperties usage, and since we don't have multiple "events" that need to pass data between them we can avoid that step for now. winning. Ok, going to make sure I have the formatting changes cleaned up and will do the other punch list items.

[drusellers]

@abatishchev ok, updated change log - saw you got the version numbers. Feeling pretty good right now. Any other items you want me to square away?

[abatishchev]

@drusellers can you please check why the build failed?

[drusellers]

@abatishchev do we have a way to cut a beta nuget that I can put into my projects and get some "burn in time" with it?

[abatishchev]

Yes, sure. Every check (build) procudes a NuGet package you can download and consume (from the disk). Once merged, I'll publish it to NuGet.org too. But would be great if you could give it a local try first!

[abatishchev]

Here's the latest: https://dev.azure.com/abatishchev/OpenSource/_build/results?buildId=1397&view=artifacts&pathAsName=false&type=publishedArtifacts

[abatishchev]

@drusellers I rebased your branch on the latest main after I ran into a few merge conflicts. Can you please check if I missed anything?

[abatishchev]

All tests passed, I'm ready to check it in

[drusellers]

@abatishchev sorry for the delay, was heads down on some other projects. I'll look to grab the latest and report back. TY!