Skip to content
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

configuration stored in appsettings.json is not taken into account #15310

Closed
lampersky opened this issue Feb 12, 2024 · 9 comments
Closed

configuration stored in appsettings.json is not taken into account #15310

lampersky opened this issue Feb 12, 2024 · 9 comments
Labels
Milestone

Comments

@lampersky
Copy link
Contributor

lampersky commented Feb 12, 2024

I've noticed that configuration stored in appsettings.json is not binded in many places:

  • SmtpSettings,
  • FacebookSettings,
  • GitHubAuthenticationSettings,
  • GoogleAuthenticationSettings,
  • MicrosoftAccountSettings,
  • AzureADSettings,
  • ReverseProxySettings,
  • TwitterSettings.

It looks like somebody created some extensions methods, but they were not used, other than that they were wrong in my opinion.

Steps to reproduces:

  • go to appsettings.json,
  • uncomment a few lines, and provide some values, even fake one:
    "OrchardCore_Facebook": {
      "AppId": "1111111",
      "AppSecret": "2222222",
      "FBInit": false,
      "FBInitParams": "status:true,xfbml:true,autoLogAppEvents:true",
      "SdkJs": "sdk.js",
      "Version": "v3.2"
    },
    "OrchardCore_GitHub": {
      "ClientID": "33333333",
      "ClientSecret": "4444444",
      "CallbackPath": "/signin-github",
      "SaveTokens": false
    },
    "OrchardCore_Google": {
      "ClientID": "555555555",
      "ClientSecret": "666666666",
      "CallbackPath": "/signin-google",
      "SaveTokens": false
    },
    "OrchardCore_Twitter": {
      "ConsumerKey": "777777",
      "ConsumerSecret": "8888888",
      "AccessToken": "9999999999",
      "AccessTokenSecret": "0000000000000"
    },
    "OrchardCore_Microsoft_Authentication_MicrosoftAccount": {
      "AppId": "111111111",
      "AppSecret": "222222222",
      "CallbackPath": "/signin-microsoft",
      "SaveTokens": false
    },
    "OrchardCore_Microsoft_Authentication_AzureAD": {
      "DisplayName": "33333333",
      "AppId": "4444444",
      "TenantId": "55555555555",
      "CallbackPath": "/signin-oidc",
      "SaveTokens": false
    }
  • make sure you don't have any configuration saved to the database, you can check this with this sql query: select Content from document where Type = 'OrchardCore.Settings.SiteSettings, OrchardCore.Settings',
  • run the app and observe the issue.
@rjpowers10
Copy link
Contributor

I'm not the original author, but my interpretation is those extension methods are intentionally not called in base Orchard Core code. They are provided as a way for consumers of OC to override the DB settings with configuration settings, if desired.

Here is the docs for email

I'm using this override for email settings myself. And one thing I noticed is the configuration values do override the DB settings (as intended), but the admin UI does not reflect these overrides, so it can be a little confusing. Which is perhaps part of the reason these are provided as optional overrides.

@lampersky
Copy link
Contributor Author

I'm not the original author, but my interpretation is those extension methods are intentionally not called in base Orchard Core code. They are provided as a way for consumers of OC to override the DB settings with configuration settings, if desired.

It makes sense thank you @rjpowers10, but in my opinion it would be better if those settings from appsettings.json were always taken into account when present in the file (or in env variables).

For example If I will configure Facebook (via portal azure) I still need to deploy new version of the APP with additional call in Startup.cs to one of those extension methods ConfigureFacebookSettings. Which could be avoided.

image

@ns8482e
Copy link
Contributor

ns8482e commented Feb 13, 2024

@lampersky It's by design - By default in orchard cms setting are configurable in UI.

If you would like to use appsettings instead of UI then it's opt-in feature, you have to call ConfigureFacebookSettings explicitly in your Program.cs/Startup.cs as it's documented here https://docs.orchardcore.net/en/latest/docs/reference/modules/Facebook/#meta-settings-configuration

To opt-in to use appsettings change your program.cs as following
eg.

builder.Services
    .AddOrchardCms()
    .ConfigureFacebookSettings()

@Piedone
Copy link
Member

Piedone commented Feb 13, 2024

Yep, this works like that by design, as others have elaborated.

@Piedone
Copy link
Member

Piedone commented Feb 14, 2024

That being said, we can change the design.

The settings coming from the configuration providers should already only apply if you have configurations available (do they only apply then?). So, it's opt-in. Currently, we support the scenario of having configuration but not wanting to use it, which seems like at most an edge case.

There can be some performance impact on the app start. However, at that point the DB configurations could be optional as well, since those also have a perhaps even larger impact.

@ns8482e
Copy link
Contributor

ns8482e commented Feb 14, 2024

Sure we can change the design, as if that adds flexibility and new features.

However, currently when developer can use Orchard CMS target as nuget package reference in the webapp - that allows them to customize Program.cs and opt-in for behavior asked in the issue - is already supported scenario without needing to have change in codebase.

@lampersky
Copy link
Contributor Author

I have to admit something to you guys, I've never ventured into that part of the documentation :-P

I mistakenly assumed that it should work like for other configurable modules: OrchardCore_Apis_GraphQL, OrchardCore_Media_Azure and many others. The configuration from the appsettings.json file is taken into account automatically, you do not need to activate it additionally by calling any dedicated extension methods.

So, thank you @ns8482e, @Piedone and sorry for getting you involved unnecessarily. Maybe I was too quick into preparing the fix.
If you guys will decide to reject the PR, let me know and I will extract this part which solves #14050 into another PR.

@sebastienros
Copy link
Member

Should we close the PR and add more documentation about the behavior instead?

@Piedone
Copy link
Member

Piedone commented Mar 18, 2024

I'm OK with that. @lampersky would you be so kind to improve the docs instead?

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

Successfully merging a pull request may close this issue.

5 participants