Bug Fix: CreatingTicket not using properly initialized event

[TehGM]

The bug

Today as I was working on my own project, I noticed that after assigning DiscordAuthenticationOptions.EventsType, the custom events class gets properly initialized with a scope, but CreatingTicket method is not triggered - while DiscordAuthenticationOptions.Events.OnCreatingTicket delegate is.

Cause

After some investigation I noticed that the cause for this is the fact that each of the providers in the library calls Options.Events.CreatingTicket directly. Doing it that way completely ignores the instance generated from EventsType property.

Solution

Looking into source code of base class AuhenticationHandler, I noticed that InitializeEventsAsync async method properly creates an instance and assigns it to Events property. Therefore, using Events.OnCreatingTicket instead of Options.Events.OnCreatingTicket in the Provider solves the issue.

Unit Tests

Unit tests all ran successfully.

[martincostello]

Thanks for raising this - could you elaborate more on the use case which lead you to find this (preferably with a code snippet)?

We have an existing test case that explicitly checks that the OnCreatingTicket event is raised, so I'd want to understand why the test doesn't find this scenario so we can fix it and/or extend it to cover this issue.

AspNet.Security.OAuth.Providers/test/AspNet.Security.OAuth.Providers.Tests/OAuthTests`1.cs

Line 138 in 0d9f3f2

public async Task OnCreatingTicket_Is_Raised_By_Handler()

[TehGM]

Sure - by calling Options.Events.CreatingTicket, default implementation of OAuthEvents present in options class is called. This default implementation only invokes OnCreatingTicket delegate - which is why delegate works perfectly fine.

Whereas the Events property of the handler is constructed when handler is initialized as follows: if EventsType is specified in options, an instance of that type will be resolved from DI; otherwise the default implementation from options will be used. This can be seen in AuthenticationHandler line 95.

By calling Options.Events.CreatingTicket, only delegates will work. By calling Events.CreatingTicket, EventsType will be built if specified, otherwise it'll fallback to delegates, which is expected behaviour as documented on MS Docs.

This can be replicated with following example (not exact code I used, heavily stripped off stuff just to show the idea):

DiscordOAuthEvents.cs:

public class DiscordOAuthEvents : OAuthEvents
{
    public DiscordOAuthEvents()
    {
        // usual constructor DI goes here
    }

    public override async Task CreatingTicket(OAuthCreatingTicketContext context)
    {
        // set breakpoint below to see if it's getting hit
        ;
    }
}

DiscordDependencyInjectionExtensions.cs:

public static class DiscordDependencyInjectionExtensions
{
    public static IServiceCollection AddDiscord(this IServiceCollection services)
    {
        services.TryAddScoped<DiscordOAuthEvents>();
        services.AddAuthentication()
            .AddDiscord(options =>
            {
                options.ClientSecret = "client-secret";
                options.ClientId = "client-id";
                options.EventsType = typeof(DiscordOAuthEvents);
            });

        return services;
    }
}

Using the code above, the breakpoint in DiscordOAuthEvents doesn't get hit. This is cause providers currently call Options.Events.CreatingTicket.

This class-based approach can be useful in multiple cases - it makes DI easier to use, and also decouples events code from service registration code.
Microsoft use this approach in Cookie Auth without Identity guide.

[martincostello]

Great, thanks @TehGM - I understand the bug and the use case now.

I've replicated this with a test locally and verified it passes with this change. I'll merge this and then add the test, and then push the fix to NuGet.org.

[martincostello]

Thanks again @TehGM - this fix has now been pushed to NuGet.org: https://www.nuget.org/packages/AspNet.Security.OAuth.Discord/5.0.13