okta / okta-sdk-dotnet

A .NET SDK for interacting with the Okta management API, enabling server-side code to manage Okta users, groups, applications, and more.
Other
160 stars 100 forks source link

Can't load configuration from appsettings.json #451

Closed gao-artur closed 3 years ago

gao-artur commented 4 years ago

The underlying FlexibleConfiguration package for loading configuration has a bug. When adding json configuration it actually adds YamlProvider that makes it imposable to load configuration from appsettings.json

Okta.Sdk v3.1.1 FlexibleConfiguration v1.2.2

bryanapellanes-okta commented 4 years ago

@gao-artur, Thanks for bringing this to our attention. Is there a specific usage of the problematic method that you can point us to in the Okta Sdk? I will add this to our backlog for prioritization.

gao-artur commented 4 years ago

It makes it impossible to register OktaClient in DI without explicitly passing it OktaClientConfiguration instance.

This will fail with message: Your Okta URL is missing. You can copy your domain from the Okta Developer Console. Follow these instructions to find it: https://bit.ly/finding-okta-domain (Parameter 'OktaDomain')

services.AddScoped<IOktaClient, OktaClient>()

This will succeed:

services.AddSingleton<IOktaClient, OktaClient>(_ =>
    new OktaClient(configuration.GetSection("okta:client").Get<OktaClientConfiguration>()));
nbarbettini commented 4 years ago

Some background: If I recall correctly, the only reason this library used FlexibleConfiguration is because of some old compat issues with Microsoft.Extensions.Configuration (at the time, not everyone was on the .NET Standard train). FlexibleConfiguration is definitely doing some extra work that could be eliminated by plugging into the standard .NET Core configuration pattern (IOptions).

andriizhegurov-okta commented 4 years ago

@gao-artur I don't think it is a bug in FlexibleConfiguration because yaml format is a superset of json and so json files can be parsed with YamlProvider.

As for the configuration issue you've reported, could you check if your appsettings.json contains Okta properties at the root level? Currently Okta client only expects these properties to be at the root level of this json file.

This should work:

{

 "OktaDomain": "https://<domain>.okta.com",
  "token": "<token>"
}

and this will not work:

{
  "Okta": {
    "OktaDomain": "https://<domain>.okta.com",
    "token": "<token>"
  }
}
gao-artur commented 4 years ago

@andriizhegurov-okta I get same error with

{
 "OktaDomain": "https://<domain>.okta.com",
 "token": "<token>"
}

and with

{
  "client": {
     "OktaDomain": "https://<domain>.okta.com",
     "token": "<token>"
  }
}

Actually I don't think it can work in any form except

{
  "okta": {
    "client": {
      "OktaDomain": "https://<domain>.okta.com",
      "token": "<token>"
    }
  }
}

because of this line

configBuilder.Build().GetSection("okta").GetSection("client").Bind(compiledConfig);
gao-artur commented 4 years ago

Actually it could work without okta:clint sections dew to this line, but it doesn't

configBuilder.Build().Bind(compiledConfig);
andriizhegurov-okta commented 4 years ago

@gao-artur Yes, the following format also works, sorry for misleading you.

{
  "okta": {
    "client": {
      "OktaDomain": "...",
      "token": "..."
    }
  }
}

but so does the one I suggested. Is this possible that you have a really old version of Okta.Sdk? Please have a look, I tried to reproduce your issue with Okta asp.net core sample app, Okta client was properly configured: image

Anyways I think we cannot blame FlexibleConfiguration package in this case.

gao-artur commented 4 years ago

Really weird but it doesn't work for me. Are you sure it wasn't loaded from okta.yaml in .okta or %userprofile%\.okta\okta.yaml folder? I tried to debug the sdk and it wasn't able to find any configuration in any provider. The YamlProvider responsible to parse appsettings.json also was't able to parse it. (I'm using latest sdk) image

andriizhegurov-okta commented 4 years ago

I did another test: I removed Okta settings from anywhere besides appsettings.json and also I changed the values to be sure these exactly values are used. It worked as expected. Then I removed these values only from appsettings and the application failed to start. image

It's really strange that this doesn't work for you. Just to be sure: nothing works for you so far even this format below, am I correct?

{
  "okta": {
    "client": {
      "OktaDomain": "...",
      "token": "..."
    }
  }
}
gao-artur commented 4 years ago

@andriizhegurov-okta thanks, your examples helped me to find the real bug. I had a comment with colon in my appsettings.json.

{
 // comment: with colon
  "okta": {
    "client": {
      "OktaDomain": "...",
      "token": "..."
    }
  }
}

Once I removed the colon YamlProvider was able to parse the json. I think this bug should keep open because when using MS ConfigurationBuilder everything works even with colons in comments.

andriizhegurov-okta commented 4 years ago

@gao-artur Thank you for reporting that, it is something to be considered. But it's more like an enhancement request, not a bug. I investigated this a little bit and it seems like JSON format doesn't allow comments at all, even though the MS JSON parser understands them. See here https://www.json.org and other docs mentioned in this answer: https://stackoverflow.com/questions/244777/can-comments-be-used-in-json#4183018

I'll put this in our backlog for prioritization.