dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.41k stars 10k forks source link

AddScheme() doesn't configure options #57393

Open iKingNinja opened 2 months ago

iKingNinja commented 2 months ago

Is there an existing issue for this?

Describe the bug

I'm having the same problem reported in #17539. The AddScheme<TOptions,THandler>(String, Action<TOptions>) method is not setting the options' values.

public class ApiKeyAuthenticationOptions : AuthenticationSchemeOptions
{
    public string ApiKey { get; set; } = null!;
}
public class ApiKeyAuthenticationHandler : AuthenticationHandler<ApiKeyAuthenticationOptions>
{
    public ApiKeyAuthenticationHandler(IOptionsMonitor<ApiKeyAuthenticationOptions> options,
        ILoggerFactory logger,
        UrlEncoder encoder) : base(options, logger, encoder)
    {
        if (Options.ApiKey == null) throw new ArgumentNullException(nameof(Options.ApiKey)); // NullReferenceException
    }

    protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
    {
        // Validate API key

        if (!Request.Headers.TryGetValue("x-api-key", out var key)) return AuthenticateResult.Fail("Missing API key");
        if (key != Options.ApiKey) return AuthenticateResult.Fail("Invalid API key");

        return AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(), Scheme.Name));
    }
}

The handler throws a NullReferenceException exception with the following details

System.NullReferenceException: 'Object reference not set to an instance of an object.'

Microsoft.AspNetCore.Authentication.AuthenticationHandler<TOptions>.Options.get returned null.

This is how i'm configuring the scheme:

builder.Services.AddAuthentication(options =>
{
    options.DefaultAuthenticateScheme = ApiKeyAuthenticationDefaults.AuthenticationScheme;
    options.DefaultChallengeScheme = ApiKeyAuthenticationDefaults.AuthenticationScheme;
})
    .AddScheme<ApiKeyAuthenticationOptions, ApiKeyAuthenticationHandler>(ApiKeyAuthenticationDefaults.AuthenticationScheme, options =>
    {
        options.ApiKey = configuration["ApiKey"] ?? throw new Exception("No API key was configured");
    });

I even tried another approach which follows:

builder.Services.AddAuthentication(options =>
{
    options.DefaultAuthenticateScheme = ApiKeyAuthenticationDefaults.AuthenticationScheme;
    options.DefaultChallengeScheme = ApiKeyAuthenticationDefaults.AuthenticationScheme;
})
    .AddScheme<ApiKeyAuthenticationOptions, ApiKeyAuthenticationHandler>(ApiKeyAuthenticationDefaults.AuthenticationScheme, _ => { });

builder.Services.Configure<ApiKeyAuthenticationOptions>(options =>
{
    options.ApiKey = configuration["ApiKey"] ?? throw new Exception("No API key was configured");
});

However there's no change in the error.

Of course both the previous configuration code blocks are followed by

app.UseAuthentication();
app.UseAuthorization();

I'm sure configuration["ApiKey"] is not null as its logged value is correct.

Expected Behavior

I expect the AddScheme() method to set options values.

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

8.0.400

Anything else?

No response

halter73 commented 2 months ago

The AuthenticationHandler<TOptions> constructor doesn't initialize Options because it doesn't have the scheme name yet. It's initialized after the authentication middleware calls InitializeAsync.

https://github.com/dotnet/aspnetcore/blob/987dbfb60798183912f52d0769272619b6d7ad7d/src/Security/Authentication/Core/src/AuthenticationHandler.cs#L147-L155

If you need access to Options while initializing your handler, we recommend overriding InitializeHandlerAsync where Options should be initialized. Does this address your issue?

iKingNinja commented 2 months ago

Yes, this indeed solves the issue. It would be nice if this was mentioned in the docs. Thank you for the help.

MackinnonBuck commented 2 months ago

@guardrex, we think we should update the docs to include this scenario - is there enough context on this issue for you to add a bit about InitializeHandlerAsync?

guardrex commented 2 months ago

Yes, but this isn't Blazor specific, so it would map to either @mjrousos (the listed author of the overview security article) and/or @Rick-Anderson, the listed MS author.

Rick-Anderson commented 2 months ago

@iKingNinja could you write the simplest possible app to demonstrate this for https://github.com/dotnet/AspNetCore.Docs.Samples/issues/258 and then outline the context for https://github.com/dotnet/AspNetCore.Docs/issues/33405 ?

iKingNinja commented 2 months ago

@Rick-Anderson Sorry I'm new to these things, do you want me to write a simple app that throws the exception and create a pull request to https://github.com/dotnet/AspNetCore.Docs.Samples/issues/258?

Rick-Anderson commented 2 months ago

@iKingNinja

@Rick-Anderson Sorry I'm new to these things, do you want me to write a simple app that throws the exception and create a pull request to dotnet/AspNetCore.Docs.Samples#258?

To access the Options while initializing your handler, override the InitializeHandlerAsync where Options should be initialized.

Is that the typical way to solve the problem? If so, write a sample that does that.