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

Okta Provider not respecting Options settings #816

Closed drusellers closed 5 months ago

drusellers commented 8 months ago

Describe the bug

I'm working with a customer that does not have the API Authentication feature which impacts custom authentication servers. I'd like to configure the customers AuthorizationEndpoint, TokenEndpoint and UserInformationEndpoint but cannot because the OktaPostConfigureOptions overwrites any values I use.

Steps To reproduce

.AddOkta("TestScheme", o => 
{
    o.TokenEndpoint = "abc";
})

Expected behaviour

That the provider would respect my configuration options

Actual behaviour

Values overwritten here https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/dev/src/AspNet.Security.OAuth.Okta/OktaPostConfigureOptions.cs#L32:L34

System information

n/a

Additional context

n/a

martincostello commented 8 months ago

We can look at making a change to have it only set the values if they are null.

In the meantime, you could workaround this with the following code snippet:

services.AddOkta("TestScheme");
services.AddOptions<OktaAuthenticationOptions>("TestScheme")
        .PostConfigure((options) =>
        {
            options.AuthorizationEndpoint = "whatever";
            options.TokenEndpoint = "whatever";
            options.UserInformationEndpoint = "whatever";
        });

EDIT: Updated code snippet after testing.

martincostello commented 8 months ago

I'm minded to not change anything for this, as this is customisable using built-in configuration functionality, even though it is maybe not 100% intuitive due to us building the URLs for you in the default case.

You can see in this test that you can further configure the options after OktaPostConfigureOptions has run: https://github.com/martincostello/AspNet.Security.OAuth.Providers/commit/cdeb0223de8b5edd0e00f3661c40268022fdcb9e#diff-410fc1906e363b918ed101da30985db9efb377bb50945d8e3c058af8a7e584ff

Otherwise if we use ??= instead of = to assign the endpoint property values, the original way of configuring things would work, or PostConfigure can be simplified to Configure in that case.

Thoughts @kevinchalet?

kevinchalet commented 8 months ago

I'm minded to not change anything for this, as this is customisable using built-in configuration functionality, even though it is maybe not 100% intuitive due to us building the URLs for you in the default case.

Sounds very reasonable. Customizing the endpoints is generally reserved to very specific scenarios, so not sure it's worth changing anything on our end.