Provide case-insensitive tenant names in ShellHost

[MikeAlhayek]

Fix #12106

[MikeAlhayek]

@jtkech here is the PR we discussed in #12106. Let me know your thoughts

[jtkech]

Yes as the related extension does the same currently, but not sure I like this extension.

1. To be more strict before doing an action not allowed on the Default tenant, we have better to do a case insensitive comparison. So here okay as this is what the extension currently does.

2. But in other places, before doing an action only allowed by the Default tenant, I would say that we have better to do a case sensitive comparison, in that case IsDefaultShell() is less restrictive and not right for me.

We could remove IsDefaultShell() and just use == or String.Equals(...OrdinalIgnoreCase) as here if it still makes sense ;) but removing it is a breaking change, or just make this extension cases sensitive (see below).

But IsDefaultShell() is always used on a registered shellSettings, we can create any non registered shell with any name, but we can't register e.g. a dEfAuLt tenant if the tenant validator was used, and moreover with the new case insensitive dicos it would have already failed on a duplicate key.

So anyway I think that's okay if IsDefaultShell() is case sensitive or not.

---

So first we could change IsDefaultShell() like this as it makes more sense for me, in most of the places that we are using it we are in the above case number 2..

public bool IsDefaultShell() => Name == ShellHelper.DefaultShellName;

Then because it would be case sensitive, revert the usage of IsDefaultShell() here and around line 254/248 where we are in the case number 1.. OR because of the above analysis, keep this change here and line 254/248 just because the code is cleaner.

I'm hesitating ;)

In any case you can use IsDefaultShell() to init the ShellSettingsEntry.IsDefaultTenant line 108, which is checked in the index view and where we are in the case number 2.. At least it would be as before if we don't change the extension, and better if we change it.

[MikeAlhayek]

@jtkech I am not sure that I agree, or may be I do but could be missing the point.

First, IsDefaultShell() checks if the settings is a host settings. Since this PR is allowing invariant tenant, then Default, dEfAuLt, default or any other form should be treated the same. So I think, IsDefaultShell() => String.Equals(Name, ShellHelper.DefaultShellName, StringComparison.OrdinalIgnoreCase) is valid.

In fact, I think we should add another extension method to ShellSettings to clean up some of the other code. I think we should add IsShell(string tenantName) => String.Equals(Name, tenantName, StringComparison.OrdinalIgnoreCase);

I don't know that I like the name IsShell perhaps you can come up with a better name if you agree. But at the end the combined code will look like this

public bool IsDefaultShell() => IsShell(ShellHelper.DefaultShellName);
public bool IsShell(string tenantName) => String.Equals(Name, tenantName, StringComparison.OrdinalIgnoreCase);

Now, instead of having to write code like if (String.Equals(shellSettings.Name, "name", StringComparison.OrdinalIgnoreCase)) we would simplify it to shellSettings.IsShell("name").

[jtkech]

Since this PR is allowing invariant tenant, then Default, dEfAuLt, default or any other form should be treated the same.

I don't think so, this PR only allows to retrieve a tenant by name in a case insensitive way, then for example the name of the default registered tenant is still always equal to the const name ShellHelper.DefaultShellName which currently is Default.

Anyway I think it is better to cleanup this PR, maybe open a new one to only focus on the new case insensitive shell inner dictionaries. I will comment it in the main thread.

[jtkech]

We will keep this change or not, see my other and more detailed review comment.

[hishamco]

I think I already did a PR for this couple months ago, let me find it

[hishamco]

It's here #11435, so please don't re-invent the wheel, furthermore check @jtkech there ;)

[MikeAlhayek]

@hishamco I see that now. Unfortunately, this change is in 3 different PR which is why I hate when PR site in the queue for so long. So merging any of these 3 PRs, will cause a conflict in two.

Unless you mind, I would like to keep the change here too and when the first PR is merged, we'll just fix this simple conflict.

[jtkech]

@CrestApps

Okay looks good, only some thoughts that could be dealed here or on separate issues/PRs.

See my too long review comment on the IsDefaultShell() extension.

---

Just looked at the TenantValidator, with the new case insensitive dicos we could do around line 80.

if (_shellHost.TryGetSettings(model.Name, out var settings) && model.IsNewTenant)
{
    errors.Add(new ModelError(nameof(model.Name), S["...."]));
}

var allOtherSettings = _shellHost.GetAllSettings().Where(s => s != settings);

if (allOtherSettings.Any(tenant => ...))
{
    errors.Add(new ModelError(nameof(model.RequestUrlPrefix), S["..."]));
}

---

Also for info in TenantValidator I think we can just remove the following lines. TenantValidator is injected and used by the default tenant so here !_shellSettings.IsDefaultShell() is always false. And a tenant which is not the Default tenant can have an empty Host and empty url prefix if the Default tenant has a non empty url prefix (which is allowed and works). We only need to check that 2 tenants don't have the same Host and same prefix, which is checked afterwards. But maybe in a separate issue/PR ;)

if (!_shellSettings.IsDefaultShell() &&
    String.IsNullOrWhiteSpace(model.RequestUrlHost) &&
    String.IsNullOrWhiteSpace(model.RequestUrlPrefix))
{
    errors.Add(new ModelError(nameof(model.RequestUrlPrefix),
        S["Host and url prefix can not be empty at the same time."]));
}

[MikeAlhayek]

@CrestApps

Okay looks good, only some thoughts that could be dealed here or on separate issues/PRs.

See my too long review comment on the IsDefaultShell() extension.

Just looked at the TenantValidator, with the new case insensitive dicos we could do around line 80.

if (_shellHost.TryGetSettings(model.Name, out var settings) && model.IsNewTenant)
{
    errors.Add(new ModelError(nameof(model.Name), S["...."]));
}

var allOtherSettings = _shellHost.GetAllSettings().Where(s => s != settings);

if (allOtherSettings.Any(tenant => ...))
{
    errors.Add(new ModelError(nameof(model.RequestUrlPrefix), S["..."]));
}

Also for info in TenantValidator I think we can just remove the following lines. TenantValidator is injected and used by the default tenant so here !_shellSettings.IsDefaultShell() is always false. And a tenant which is not the Default tenant can have an empty Host and empty url prefix if the Default tenant has a non empty url prefix (which is allowed and works). We only need to check that 2 tenants don't have the same Host and same prefix, which is checked afterwards. But maybe in a separate issue/PR ;)

if (!_shellSettings.IsDefaultShell() &&
    String.IsNullOrWhiteSpace(model.RequestUrlHost) &&
    String.IsNullOrWhiteSpace(model.RequestUrlPrefix))
{
    errors.Add(new ModelError(nameof(model.RequestUrlPrefix),
        S["Host and url prefix can not be empty at the same time."]));
}

@jtkech
I need to avoid making any change to the tenant validator as I have another big PR that is making lots of changes to it. I don’t want to break the other change now. I’ll review this more and can make necessary changes in the other PR. Actually you may also want to review that PR #11822. I’ll review more of your other feedback tomorrow and respond. Thank you for your detailed feedback.

[jtkech]

@CrestApps

Okay, I was not aware of all these mixed PRs e.g. #11435 where I retrieved one of my comments #11435 (comment) where I already didn't totally agree.

So I suggest that you close this PR, then start a new one which only focus on the new shell inner dictionaries that would be case insensitive.

Then I will try to do separate PRs for the other little concerns we discussed here, maybe better after this PR if it get merged because the context would not be the same.

[MikeAlhayek]

Closing this in favor of #12120

[MikeAlhayek]

@jtkech

Just looked at the TenantValidator, with the new case insensitive dicos we could do around line 80.

I made the suggested changes to the TenantValidator in PR #11822. Actually, with this change, the TenantValidatorTests fail because it was using GetAllSettings() and manually compare names. I think by using TryGetSettings() the code is more accurate and more bullet proof to changes in the ShellHost instead of just comparing names.

[jtkech]

Yes I think you're right but normally the TenantValidatorTests should have still worked.

Would need to look at it more in depth.

Also as said in TenantValidator there is a wrong usage of _shellSettings.IsDefaultShell() as normally the injected _shellSettings is always the Default shell so no need to inject and use it. And it is used to check if a non default tenant has both an empty host and prefix but this is allowed for any tenant, so this check can be removed if afterwards we check for host/prefix duplicates.

It works in the unit test as we create a validator that we let be used by a non default tenant, so in this case _shellSettings.IsDefaultShell() is not always the same.

Also I didn't take a look at the AutoSetup as it would need to be consistent, I don't think it already uses the tenant validator.

Hmm, I'm afraid that we already saw too many things (all simple things) that would need to be done before adding new functionalities.

I suggest to first do separate PR(s) to tweak the existing code now that we have case insensitive dictionaries, then let decide what we do with IsDefaultShell(), then tweak the TenantValidator and make the AutoSetup consistent with these new things.

Then and only after that, even if there are new conflicts in PRs, restart to add new features on this base.

[MikeAlhayek]

@jtkech i am not sure I follow you on what duplicate check that should be removed. if you can, please leave a review directly on that PR so I can correct it.

As for the TeantValiadatorTest, the reason it failed is because we mocked GetAllSettings to return in memory collection. Meanwhile. The TryGetSettings looks at a different/internal collection to validate if the given tenant exists. So in this case TryGetSettings() will always compare an empty collection. The change I made, is using an instance of ShellHost now which comparing same internal collection is both placed which is why the update test works.

Regarding IsDefaultShell(), I think that is a bigger topic and should be addressed as you suggested in a separate PR. Since that method is part of the core code, it’ll always be used all over the place. I suggest a quick fix for that soon to reduce the the instances where it’s used.

[jtkech]

Okay cool then

please leave a review directly on that PR so I can correct it.

The lines to remove (that don't appear in your PR changes) are

        if (!_shellSettings.IsDefaultShell() && String.IsNullOrWhiteSpace(model.RequestUrlHost) && String.IsNullOrWhiteSpace(model.RequestUrlPrefix))
        {
            errors.Add(new ModelError(nameof(model.RequestUrlPrefix), S["Host and url prefix can not be empty at the same time."]));
        }

The tenant validator is only intended to be used in the context of the Default tenant, so !_shellSettings.IsDefaultShell() always have the same false value, so no need to inject _shellSettings. It works in the test as we let the validator be used by non default tenants.

To fix this check we would need to check the model.Name instead I think and see if it is equal to the DefaultShellName but here directly on a string and here in a case insensitive way as it doesn't come from a registered shellSettings.

But anyway this check prevent from having both an empty host and prefix which in fact is allowed, so we could just remove the above lines and don't inject anymore _shellSettings.

But need to check step by step.