OSOE-638: Feature to edit shell settings from the admin in Lombiq.Hosting.Tenants

[wAsnk]

OSOE-638
Fixes #66

[jtkech]

@wAsnk

Some info here #66 (comment)

I looked at your controller before your last commit, it looks good, normally it should work.

Are you sure that you provide kvps and with different values than those that may already be configured in another config source like the main appsettings.json? If for a given key the same value is already configured, it will not be persisted in the tenant specific config source.

[wAsnk]

Thanks for checking!

Yeah I'm sure.

This is what happens in SaveSettingsAsync, I'm trying to set the following in my Test tenant (I added both lines to test if the tenant name does anything):

"Lombiq_Hosting_Tenants_EmailQuotaManagement:EmailQuotaPerMonth": "42",
"Test:Lombiq_Hosting_Tenants_EmailQuotaManagement:EmailQuotaPerMonth": "24"

1. SaveSettingsAsync settings parameter contains the changes
   
2. tenantSettings configSettings contains these keys if JObject.FromObject used, so only those can be set by the logic.
   
3. Then only those are saved that doesn't have a children, so it is not possible to save such keys:
   

I can save key/values such a "TestKey": "TestValue", but these are too simple.

[wAsnk]

So basically for this:

[Route("/shellSettingsTest")]
    public async Task<IActionResult> Test()
    {
        if (!_shellHost.TryGetSettings("Default", out var shellSettings))
        {
            return NotFound();
        }

        shellSettings["SimpleKey"] = "SimpleValue";
        shellSettings["Not:So:Simple:Key"] = "NotSoSimpleValue";
        shellSettings["Default:Not:So:Simple:Key"] = "TenantSpecificNotSoSimpleValue";

        await _shellHost.UpdateShellSettingsAsync(shellSettings);

        if (!_shellHost.TryGetSettings("Default", out var shellSettingsNew))
        {
            return NotFound();
        }

        var result = $"{shellSettingsNew["SimpleKey"]}, {shellSettingsNew["Not:So:Simple:Key"]}, {shellSettingsNew["Default:Not:So:Simple:Key"]}";
        return Json(result);
    }

You get this result:

[jtkech]

Okay I understand, I thought it was not saving simple values too.

Yes we use !s.GetChildren().Any() for simplicity, would be a little more complex to check if a config already exists with the same value in a previous configuration source, and maybe also to only have to manage simple config strings in the tenant specific source, tenant appsettings.json by default.

Maybe we are missing an extension point to be able to override this part, maybe worth to suggest a PR.

And then yes, we only pass a Dictionary<string, string> to the _tenantConfigSources.

await _tenantConfigSources.SaveAsync(
    settings.Name, tenantConfig.ToObject<Dictionary<string, string>>());

In the meantime a workaround would be to save the more complex values as json strings, but in that case we would not be able to check if the same underlying value exists in a previous source, hmm but maybe this checking could be done at the editor level.

[wAsnk]

With my current solution I only save key-values if they are not already present in any configuration before, so it works this way.
My only concern is what @Piedone said that we are displaying values that might be sensitive, but to be honest if you have this permission you should be able to see these values anyway. Maybe I can put a blur on top of the editor or hide it completely and show it only with a "click to show" button.
What do you think @Piedone ?

[Piedone]

If you can access this screen then you're an admin in Orchard, but that doesn't mean that you should be able to see secrets coming from the infrastructure; that's a different access level. Hiding these on the client side, i.e. in a way that people can still get to them if they want to, is not a suitable solution. These shouldn't be sent to the client at all.

[wAsnk]

We can limit showing it to users with a new custom permission.

[Piedone]

I really don't see a utility in that, since it adds more config requirements and opens up accidental disclosure. Please don't display anything, then, but a comment with instructions on how to use the feature, how to add config values.

[wAsnk]

What settings do we don't want to show exactly? Because only settings under "OrchardCore" section are displayed and tenant configuration.

If we don't display anything how do we know what's configured already for the tenant? Then you can only check blob storage or the file system for the tenant settings.

But let me know if that's okay for you that we don't display anything we just display an input field with a prefilled example/hint.

[Piedone]

We should only display the configuration that users configured via the module, nothing else.

[wAsnk]

If we don't save that to a new place other than the shellsettings I'm afraid we can't do that because we can't distinguish between settings saved from there or anywhere else.

[Piedone]

Then it seems we'll need to do that. Can't you otherwise mark a configuration key (like a comment)?

[wAsnk]

I wouldn't complicate it with another provider or place to save it, because it depends where the key/value is saved depending on what feature is currently on, e.g. if blob storage is on it should be saved there e.g. So I will save a dummy key/value pair and the correct key/value pair as well, then I can filter by that.

[Piedone]

Look into what JT has written.

[wAsnk]

Look into what JT has written.
Ok