aspnet-contrib / AspNet.Security.OAuth.Providers

OAuth 2.0 social authentication providers for ASP.NET Core
Apache License 2.0
2.34k stars 533 forks source link

Cannot register all BattleNet Provider Regions at the same time. #812

Closed JoschiZ closed 7 months ago

JoschiZ commented 8 months ago

Describe the bug

The BattleNet Provider has 5 regions and in general if you plan on supporting one you probably want to support all of them. This is currently impossible, because registering the same provider twice, with different authenticationScheme and displayName, results in the following error, if you try to login with a region that was not the first registered one.

    AuthenticationFailureException: The oauth state was missing or invalid.
    Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler<TOptions>.HandleRequestAsync()
    Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
    Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

I guess it is because

    public static  Microsoft.Extensions.DependencyInjection.AuthenticationBuilder AddOAuth<TOptions, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] THandler>(this AuthenticationBuilder builder, string authenticationScheme, string displayName, Action<TOptions> configureOptions)
        where TOptions : OAuthOptions, new()
        where THandler : OAuthHandler<TOptions>
    {
        builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IPostConfigureOptions<TOptions>, OAuthPostConfigureOptions<TOptions, THandler>>());
        return builder.AddRemoteScheme<TOptions, THandler>(authenticationScheme, displayName, configureOptions);
    }

Calls builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IPostConfigureOptions<TOptions>, OAuthPostConfigureOptions<TOptions, THandler>>()); with the same types multiple times TOptions == BattleNetAuthenticationOptions and THandler == BattleNetAuthenticationHandler. This results in only the first instance being correctly registered and working.

Steps To reproduce

  1. Create a template Blazor WASM Hosted .NET 7 app.
  2. Change this
    builder.Services.AddAuthentication()
    .AddIdentityServerJwt();

    to this

    builder.Services.AddAuthentication()
    .AddIdentityServerJwt()
    .AddBattleNet(
        BattleNetAuthenticationDefaults.AuthenticationScheme + BattleNetAuthenticationRegion.America,
        BattleNetAuthenticationDefaults.DisplayName + BattleNetAuthenticationRegion.America,
        options =>
        {
            options.Region = BattleNetAuthenticationRegion.America;
            options.ClientId = "id";
            options.ClientSecret = "secret";
            options.Scope.Add("oidc");
        })
    .AddBattleNet(
        BattleNetAuthenticationDefaults.AuthenticationScheme + BattleNetAuthenticationRegion.Europe,
        BattleNetAuthenticationDefaults.DisplayName + BattleNetAuthenticationRegion.Europe,
        options =>
        {
            options.Region = BattleNetAuthenticationRegion.Europe;
            options.ClientId = "id";
            options.ClientSecret = "secret";
            options.Scope.Add("oidc");
        });

    Try to login with BattleNetEurope.

Expected behaviour

You should be able to register all regions independently of each other.

Actual behaviour

If you try to register multiple providers with different regions it crashes.

System information

.NET SDK: Version: 7.0.403 Commit: 142776d834

Runtime Environment: OS Name: Windows OS Version: 10.0.19044 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\7.0.403\

Host: Version: 8.0.0-rc.2.23479.6 Architecture: x64 Commit: 0b25e38ad3

Additional context

I tried creating a proxy type, that just inherits from BattleNetAuthenticationHandler

public class BattleNetAuthenticationHandlerAmerica: BattleNetAuthenticationHandler
{
    public BattleNetAuthenticationHandlerAmerica(IOptionsMonitor<BattleNetAuthenticationOptions> options, ILoggerFactory logger, UrlEncoder encoder, ISystemClock clock) : base(options, logger, encoder, clock)
    {
    }
}
    .AddBattleNet<BattleNetAuthenticationOptions, BattleNetAuthenticationHandlerAmerica>(
        BattleNetAuthenticationDefaults.AuthenticationScheme + BattleNetAuthenticationRegion.America,
        BattleNetAuthenticationDefaults.DisplayName + BattleNetAuthenticationRegion.America,
        options =>
        {
            options.Region = BattleNetAuthenticationRegion.America;
            options.ClientId = "id";
            options.ClientSecret = "secret";
            options.Scope.Add("oidc");
        })
    .AddBattleNet

But this doesn't work, because BattleNetAuthenticationOptions is still the same and leads to the same error. Creating a proxy of both doesn't work, because BattleNetAuthenticationHandler inherits OAuthHandler<BattleNetAuthenticationOptions> and AddOAuth requires one to be convertible to the other.

Tratcher commented 8 months ago

Each instance would need a different CallbackPath so they don't get mixed up.

kevinchalet commented 8 months ago

It's worth noting that there's now a unified oauth.battle.net server that covers all regions except China. We may want to update the aspnet-contrib provider to use oauth.battle.net by default and fall back to oauth.battlenet.com.cn for China.

Note: the OpenIddict Battle.net integration already uses the unified server + their OIDC-capable login flow, if you're interested in giving it a try (more info here if you're not familiar with the OpenIddict web providers: https://kevinchalet.com/2022/12/16/getting-started-with-the-openiddict-web-providers/).

JoschiZ commented 8 months ago

Now that you mention it I remember that that they wanted to do some kind of unified server. But thanks @Tratcher for the info, I knew there was something I missed / didn't understand correctly. That problem would have been too common, even though that provider is probably not that common.

I will certainly take a look at OpenIddict, it looks really interesting.

Thanks for the quick answer!

martincostello commented 8 months ago

Proposed change to use the global server as part of our v8.0.0 release is here: #813

martincostello commented 7 months ago

Support for the unified server will be part of our imminent 8.0 release.