Provide IConfiguration extensions to configure certain modules

[Piedone]

Several modules expose their configuration via IOptions<T> but only include a site settings IConfigureOptions<T> for it: Email, Facebook, GitHub, Google, Reverse Proxy Configuration. These could also provide extension methods like OrchardCore.Shells.Azure does with AddAzureShellsConfiguration() so the configuration can be easily provided by e.g. the appsettings.json file.

[sfmskywalker]

And perhaps setup in such a way that the settings would first come from IConfiguration, but optionally overridden by Site Settings? This way, e.g. SMTP settings can still be configured from the dashboard, while taking advantage of defaults being configured using app settings.json, ENV etc.

[deanmarcussen]

Original issue discussing this one, with a focus on the email settings #4611

It was tried, and reverted, because there was no reflection in the ui to indicate that the settings had been overridden.

No trouble with someone trying todo it properly, but the key part for me, is that it must be made clear in the ui where the settings have come from, or if there are defaults provided by IShellConfiguration (obviously no access to what those defaults are, if they are secrets.)

Also super easy to do currently (yourself) by registering your own IOptions registration later, in the pipeline.

[sfmskywalker]

Exactly. One idea that crossed my mind was perhaps a user should explicitly tick some checkbox that lights up the settings in the UI, switching readonly fields to writeable. But it doesn’t completely solve the issue of making clear what settings are in effect if you then uncheck again if you don’t want to lose the overriding values.

Some applications that support layers of settings solve this using a tabbed UI, where you have one tab showing the settings coming from some source (IConfiguration for instance), another tab where you can override some values, and a tab showing the effective settings. But that’s a lot of UI, and I wonder if that’s worth it.

At the same time, being able to setup build pipelines with configuration values provided by environment variables from e.g. containers is a must IMHO. Arguably more important than having the ability to change settings from the admin screen. I say arguably, because it depends on the user using the system. So probably it’s worth the effort.

Let’s think some more about a smart way to do the UI that (ideally I think) doesn’t involve 3 tabs 🤔

[deanmarcussen]

Some applications that support layers of settings solve this using a tabbed UI, where you have one tab showing the settings coming from some source (IConfiguration for instance), another tab where you can override some values, and a tab showing the effective settings. But that’s a lot of UI, and I wonder if that’s worth it.

That is a lot of ui, indeed.

At the same time, being able to setup build pipelines with configuration values provided by environment variables from e.g. containers is a must IMHO. Arguably more important than having the ability to change settings from the admin screen. I say arguably, because it depends on the user using the system. So probably it’s worth the effort.

Arguable, and the argument is totally dependant on the user of the system.

But I agree, the build pipelines argument here is strong.
I would have preferred all the secrets, to always be appsettings.json based, and the non secrets I don't care so much about either way.
But it hasn't been done that way, for email, anyway, so...

Let’s think some more about a smart way to do the UI that (ideally I think) doesn’t involve 3 tabs 🤔

Maybe one idea could be to make it an either/or feature. I.e. it by default comes from the UI, but as a feature, only comes from appsettings.json.

The feature could include a IShapeProvider to add an alternate, for a ui screen, which just says. Configured by appsettings.json

And you could require the feature on during host Startup with the Orchard Core Builder

Not as flexible, but maybe covers both requirements?

[Piedone]

I think the good thing with having just extension methods that you could use in your web app's Startup.cs, is that then it's up to you to communicate it appropriately if necessary and you can decide which configuration wins if there are overlapping ones.

Imagine a public SaaS scenario: You don't want people to need to configure SMTP for their sites, because you'll have an SMTP server for the whole platform and want to use it on every tenant automatically. Possibly you want people to allow using their own SMPT, possibly you want to prohibit this. You configure this in appsettings, or Azure App Service configuration. OK, but then what should the SMTP Settings page say? A warning? Should it be inaccessible? I don't think there is a one-size-fits-all solution (especially that you can override those settings just partially too) so I'd go the easy way (at least for now, possibly revisit based on feedback) and say it's up to you: Orchard provides the optional capability to override settings, you decide what happens after that.

E.g.: For your own apps you'd use this in a way that appsettings always wins and you don't care about the UI. For customers' individual sites that you host maybe appsettings would be a default, and your admin customers could override it from the admin; you communicate this in a wiki or something. For your public SaaS appsettings always wins and you make the SMTP settings page inaccessible.

And yeah, of course, you can do all this without having extensions but it would make things easier. Imagine just a five-ten minute googling and whatnot for everyone doing this the first time, a lot of effort saved if there are thousands. I wouldn't consider this a super high priority though, of course, there are more important things.

[Skrypt]

There is also an other issue with this when it comes to support also an appsettings.development.json file for example. Tenants are currently not taking this into consideration when I tested 2 weeks ago. There are a lot of implications in using this. Some modules depends on their configuration, now if you enable the module it can show a UI to edit the settings afterward. But if you are using an appsettings.json file and that your module is enabled, if your settings are not properly set you need to make the proper validations on your module to not fail loading.

Example of that is with the OrchardCore.Shells.Azure module.

[jtkech]

@Skrypt as i remember we checked together that it takes into account the app level appsettings.development.json ;) also the one directly under app_data, but higher level config sources may override some values.

Maybe we would have to consider settings as another level of config sources and following the same conventions of the configuration stack, maybe as we implement e.g the tenant specific appsettings. Most of the time config sources are immutable, this is when there are mutable that it becomes more complex.

But would need more time, so here just a first thinking.

E.g we have a higher level tenant sepecific appsettings.json in the tenant folder (can be from blob) that we can edit and save, but we only save a config that doesn't already exist with the same value in the lower level config sources, but when we edit it we saw the result of all the config stack, not only the ones that are persisted in this specific config source.

[Skrypt]

True it takes it into consideration but we can't use it with empty values or null values if we'd want to override values set in the global one. It doesn't allow transformation like with web.release.config XML transform.

[deanmarcussen]

To use with empty or null values, to disable an option, is an interesting thing to do with IConfiguration.

It's not really how it is designed (from an ASP.NET Core pov).

It would be possible to add a Disabled property to something like the Azure Shells, but then you would have to include IConfiguration in the OrchardCoreBuilder, because when it is building the pipeline currently, it does not know about IConfiguration.

So currently there is no way to read the configuration, and noop, before replacing the services.

[Skrypt]

Let's see how they handle this case with common services in .net core. I'm pretty sure you are right about this.

[jtkech]

@Skrypt

True it takes it into consideration but we can't use it with empty values or null values if we'd want to override values set in the global one

Okay, i don't remember the details but yes you're right we talked about it.

So i re-did a little test with this in appsettings.json

"OrchardCore": {
  "abc": "def"

And an appsettings.development.json with this

"OrchardCore": {
  "abc": "ghi" // then "abc": "" in a 2nd test, then "abc": null in a 3rd test

And this in the code

var test = _configuration.GetValue<string>("abc");

Where i got respectively ghi, then null in the 2nd and the 3rd test because an empty string ends up in a null value, but in all 3 tests the value has been overridden. And if nothing is defined in the appsettings.development.json, we get def the one in appsettings.json.

@deanmarcussen

because when it is building the pipeline currently, it does not know about IConfiguration.

IShellConfiguration always takes into account all the configuration stack including the main IConfiguration but restricted to the OrchardCore section this to prevent conflicting names as the well known ConnectionString that may be set through an env variable by another application.

[deanmarcussen]

@jtkech

because when it is building the pipeline currently, it does not know about IConfiguration.

IShellConfiguration always takes into account all the configuration stack including the main

Yes, was just referring to the Azure.Shells and Database.Shells which are loaded in at host or application level to / with the OrchardCoreBuilder which doesn't have an IConfiguration stack built for it.

Re: settings in development, perhaps better to carry that discussion out over on #5808 rather than hijack @Piedone related / but different issue?

[jtkech]

Yes, was just referring to the Azure.Shells and Database.Shells which are loaded in at host or application level to / with the OrchardCoreBuilder which doesn't have an IConfiguration stack built for it.

Ah okay, indeed here we can't use a tenant level IShellConfiguration as we are at the app level, but, as you did and i did, here we can use IConfiguration and explicitly get the OrchardCore section to get config values shared across tenants. But i agree only from the regular config sources, not our custom ones as the appsettings directly under the app_data

rather than hijack @Piedone related / but different issue?

Oups yes you're right ;)

[jptissot]

One thing to keep in mind also is that we should obfuscase all values in the admin ui as mentioned here #6294

[hishamco]

Closed by #12033

[Piedone]

Reacting to your remarks @sebastienros under https://www.youtube.com/watch?v=vLQqQYGl_Xo&t=1010s.

You're right that this issue wasn't selected for 1.x. However, this is rather that the issue was forgotten than an intentional no-go, and apart from that, I don't really see how this was "force pushed" as you mentioned:

• This issue was open for two years. Anybody who wanted to voice their opinion could.
• The corresponding PR Provide IConfiguration extensions to configure certain modules (Lombiq Technologies: OCORE-97) #12033 was open for two months. Again, if you wanted to say anything, there was ample time. The review and design discussions happened all in the open and it's not like it was rushed into main, especially against anybody's will.
• Hisham was indeed contracted by Lombiq for this contribution. We sometimes pay people for contributions for things that are important to Lombiq. However, I fail to see how that would be a problem or even relevant: Everything else happened as usual.

[hishamco]

Is there's an issue for the extension that we added? I might need to watch the recording first

[Piedone]

No technical issues.

[hishamco]

I just watched the Seb's comment, as @Piedone mentioned earlier all the things made in PR, but is this mean for all the Lombiq related PR that we need to mark them as Triage after they are done or do we need a second approval from other one from the team. The first one may fine, but If we go with the second one we might be blocked due the team availability or no one are closed to the original issue

So, please let us decide how everything should go to avoid any conflict

Thanks

[Piedone]

If the problem is that this issue apparently never was officially triaged, then I'm sorry about that and will make sure next time that it'll be. Then again, this is an ancient issue, and nothing was rushed or forced.