Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JoschiZ opened this issue Nov 1, 2023 · 5 comments
Closed

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

JoschiZ opened this issue Nov 1, 2023 · 5 comments
Milestone

Comments

@JoschiZ
Copy link

JoschiZ commented Nov 1, 2023

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.

@JoschiZ JoschiZ added the bug label Nov 1, 2023
@Tratcher
Copy link

Tratcher commented Nov 1, 2023

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

@kevinchalet
Copy link
Member

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
Copy link
Author

JoschiZ commented Nov 1, 2023

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 added a commit to martincostello/AspNet.Security.OAuth.Providers that referenced this issue Nov 7, 2023
@martincostello
Copy link
Member

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

martincostello added a commit that referenced this issue Nov 14, 2023
Use unified BattleNet server

Resolves #812.
@martincostello martincostello added this to the 8.0.0 milestone Nov 14, 2023
@martincostello
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants