Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
945 stars 605 forks source link

Saml2Options dependency injection #1395

Closed reponemec closed 1 year ago

reponemec commented 1 year ago

I need an instance of Saml2Options to be configured within the service. I am not sure if the code below is written in the correct way.

public class PostConfigureSaml2Options : IPostConfigureOptions<Saml2Options>
{
    readonly IMyService _service;

    public PostConfigureSaml2Options(IMyService service)
    {       
        _service = service;
    }

    public void PostConfigure(string? name, Saml2Options options)
    {
        if (name == "Scheme-A") 
        {
            ConfigureSchemeA();
        }
        else 
       {
            ConfigureSchemeB();
       }
    }

    void ConfigureSchemeA(Saml2Options options) {}    

    void ConfigureSchemeB(Saml2Options options) {}    
}

services.AddSingleton<IPostConfigureOptions<Saml2Options>, PostConfigureSaml2Options>();

authenticationBuilder.AddSaml2("Scheme-A", "Scheme-A", configureOptions: null));
authenticationBuilder.AddSaml2("Scheme-B", "Scheme-B", configureOptions: null));

Please check the code and modify it properly if needed, thank you.

AndersAbel commented 1 year ago

I don't follow what you want to do here. PostConfigure is normally not used by application developers. The normal flow is that the app developer calls Configure. Then a library calls PostConfigure which is invoked after the application has completed it's config. Common uses for PostConfigure is to validate configuration and supply defaults for omitted options.

If you want to do anything dynamic per request, that should not be done by altering the options, as that will give problems on concurrent requests.

reponemec commented 1 year ago

Thank you for the quick reply. When the application starts, I have to configure Saml2Options using other services resolved from DI. Nothing more. For example, something like this:

builder.AddSaml2("Scheme-A", "Scheme-A", options => 
{
    var myService = services.BuildServiceProvider().GetRequiredService<IMyService>();    
    opt.SPOptions.EntityId = myService.ResolveEntityId("Scheme-A");
});

This works, but it is a service-locator pattern and bad way for other reasons as well. I thought IConfigureOptions might be the way to go: https://andrewlock.net/access-services-inside-options-and-startup-using-configureoptions/.

I thought this implementation might be the solution:

public class NamedConfigureSaml2Options : IConfigureNamedOptions<Saml2Options>
{
    readonly IMyService _myService;

    public NamedConfigureSaml2Options (IMyService service)
    {
        _service = service;
    }

    //It works at first glance and I also have important information about the authn scheme (the name parameter).
    public void Configure(string name, Saml2Options options) 
    {
        opt.SPOptions.EntityId = _myService.ResolveEntityId(schemeName: name);
        ...
    }
}
...
services.ConfigureOptions<NamedConfigureSaml2Options>();
...
authenticationBuilder.AddSaml2("Scheme-A", "Scheme-A", null); //It looks like there must be null here.

If not, the question remains - how to configure the Saml2Options using DI in a better way?

AndersAbel commented 1 year ago

Using an IConfigureNamedOptions instance is the right way. Calls to Services.Configure("name", opt => { ... }) is actually just a convenience method for adding a service of type IConfigureNamedOptions<T> to DI. So if you want your configuration method to access other services, this is the right way.