Allow case-insensitive tenant names in ShellHost

[MikeAlhayek]

Is your feature request related to a problem? Please describe.

In a mobile version, I created a page when a user can login to a specific tenant by providing the tenant name, username, password. To identify which tenant the user is trying to access I user _shellHost.TryGetSettings(tenantName, out settings) which looks up the tenant by name. The issue here is if the user does not provide a case sensitive tenant name, the ShellHost does not find anything.

Describe the solution you'd like

It would be helpful if user case insensitive to find tenants. We can easily achieve this by storing the tenant key as lowercase and when we get/remove the tenant we will also use lowercase so no matter what the value is, we treat it the same.

@jtkech is there an issue from making this change? I can push a PR if no issue with doing this.

For example the line [if (!_shellContexts.TryAdd(settings.Name, new ShellContext.PlaceHolder { Settings = settings }))] (

OrchardCore/src/OrchardCore/OrchardCore/Shell/ShellHost.cs

Line 197 in 51f0db7

if (!_shellContexts.TryAdd(settings.Name, new ShellContext.PlaceHolder { Settings = settings }))

) will become something like this instead if (!_shellContexts.TryAdd(settings.Name.ToLower(), new ShellContext.PlaceHolder { Settings = settings }))

Obviosly, we have to make the same change on TryGetValue and TryRemove

Describe alternatives you've considered

I did the following

private ShellSettings GetSettings(string tenantName)
{
    if (_shellHost.TryGetSettings(tenantName, out ShellSettings settings))
    {
        return settings;
    }

    var shells = _shellHost.ListShellContexts();
    var tenant = shells.FirstOrDefault(x => x.Settings != null && x.Settings.Name.Equals(tenantName, StringComparison.OrdinalIgnoreCase));

    if (tenant != null)
    {
        return tenant.Settings;
    }

    return null;
}

[jtkech]

Yes, for example we create folders based on the tenant name, so we can't have Foo and foo and normally we don't allow it when creating tenants, then yes all the code is not case insensitive.

But I understant that to retrieve a tenant we could do a case insensitive lookup, but we would need to ensure that we then use the settings.Name of the returned settings, not the 1st passed name.

Or as you suggested use normalized keys, maybe better to use ToUpperInvariant(), or declare case insensitive dictionaries, but would need to check that it doesn't break anything, not the best for perf, and as said would need to ensure that in all places we then use the settings.Name of the returned settings.

Your helper looks good, yes not the best for perf but if it is not in a hot code path. For info you also have the _shellHost.GetAllSettings() method that would be better to use.

_shellHost.GetAllSettings().FirstOrDefault(s =>
    s.Name.Equals(tenantName, StringComparison.OrdinalIgnoreCase));

As Lombiq did you can also make a string extension allowing

_shellHost.GetAllSettings().FirstOrDefault(s => s.Name.EqualsOrdinalIgnoreCase(tenantName));

[MikeAlhayek]

@jtkech _shellContexts is private so the change here should not impact external code. External code user Settings.Name because that is what is available externally. things like _shellSettings[settings.Name] = settings; would have to change.

Are you okay with submitting a PR with this change? Or, are you apposing this change to the core code?

[jtkech]

For now I would prefer to just use an extension helper to get a tenant settings in a case insensitive way, but I will think about it ;)

[jtkech]

@CrestApps

Yes, I already thought about this, in fact if it doesn't impact too much the perf, we could just define case insensitive dictionnaries.

    private readonly ConcurrentDictionary<string, ShellContext> _shellContexts =
        new(StringComparer.OrdinalIgnoreCase);

    private readonly ConcurrentDictionary<string, ShellSettings> _shellSettings =
        new (StringComparer.OrdinalIgnoreCase);

If you want you can suggest a PR mentioning that we discussed about this, and will see what @sebastienros think about the perf impact.

Then if we do it, we would not need anymore in some places to do an explicit case insensitive lookup. For example as I remember in the OC.Tenants module to prevent duplicate names.

[MikeAlhayek]

@jtkech great call on private readonly ConcurrentDictionary<string, ShellSettings> _shellSettings = new (StringComparer.OrdinalIgnoreCase);

I’ll submit a PR tomorrow with this change and see if I can find where we check for duplicate in Tenants also.

thank you