Make OC.Features optional to non-default tenant

[MikeAlhayek]

Fix #12723

[Skrypt]

More on this. I think we should stop using the ShellSettings.IsDefaultShell() and rely on a TenantId with a feature that says that this specific tenant is a tenant management one. Hence why I started a PR that introduces a TenantId.

[MikeAlhayek]

More on this. I think we should stop using the ShellSettings.IsDefaultShell() and rely on a TenantId with a feature that says that this specific tenant is a tenant management one. Hence why I started a PR that introduces a TenantId.

@Skrypt this PR isn't using ShellSettings.IsDefaultShell()

[sebastienros]

Migration documentation: modules that use the recipe migrations need to depend on OrchardCore.Recipes.Core.

[jtkech]

I tagged it as don't merge but only temporarily just because I want to review it before ;)

For example in a manifest we can't have a dependency on a core project which is not a module.

I can't right now but I'll try to review it this night

[MikeAlhayek]

@jtkech we'll await your approval before we merge it. The only reason we can list OrchardCore.Recipes.Core in the manifest file is because a new feature was added with the id OrchardCore.Recipes.Core. As you can see this feature is EnabledByDependencyOnly which should be enable/disabled on demand as needed to provide the core services and prevent duplicate service registrations.

[assembly: Feature(
    Id = "OrchardCore.Recipes.Core",
    Name = "Recipes",
    Description = "Provides recipe services.",
    Dependencies = new[]
    {
        "OrchardCore.Recipes.Core"
    },
    Category = "Infrastructure",
    EnabledByDependencyOnly = true
)]

[jtkech]

Ah okay, I will look at it this night.

Hmm but looks wrong that OrchardCore.Recipes.Core depends on itself.

[MikeAlhayek]

@jtkech it looks wrong because it is wrong. nice catch... I was just testing you ;)

I also added the documentation for the 1.6 release as @sebastienros requested.

[jtkech]

Okay cool !!!

[MikeAlhayek]

@jtkech were you able to valid this PR?

[jtkech]

@MikeAlhayek Oops sorry, no problem I will look at it this night.

[MikeAlhayek]

@jtkech I made some updates as per your request. If you are good with it, either merge or approve please.

[jtkech]

@MikeAlhayek Okay, just did an additional little review.

As said in the review, in fact the related modules using the recipe migrator were already missing a dependency on OC.Recipes, and seems to not be related to this PR, not saying that it is not good to be fixed here but just to well understand the goal of this PR.

So, even if core modules is a good idea (we may need to discuss a little more on it), for the goal of this PR it seems that we only need to remove OC.Features from the dependencies of OC.Recipes.

Then we would just need to add or not OC.Features in the recipes of other non Default tenants, If you want the same recipe for the Default tenant, OC.Features can be pre-configured for it only.

Note: Never tried so not sure but maybe we can also use Feature Profiles to filter OC.Features.

---

So in summary, unless some tweaks as missing null checks, maybe this PR only needs to remove one line, the dependency of OC.Recipes on OC.Features that doesn't seem to be required, and maybe also add the already missing dependency of some modules on OC.Recipes.

---

Hmm, the above assuming that we don't change the behavior of OC.Features being an IsAlwaysEnabled feature, meaning that it can't be disabled once enabled.

Yes because, even if here we keep the same behavior for the Default tenant, not sure it's good that this is no more the case for the other tenants, It may be unexpected that we now can disable Features through the Admin UI, particularly on existing non Default tenants.

[MikeAlhayek]

@jtkech I think I went down the path of making Recipes.Core because a service depends on recipes that isn't available unless Features in enabled. I don't recall the exact service name now.

Either way, I don't think there is a good reason to undo some of these good changes that already implemented in this PR. With this PR, we can even make Recipes not always enabled if we want to hide the UI. In theory, everything should still work, as long as everything that needs core recipe services depends on the new core feature.

I'll get the other changes done in the morning.

[MikeAlhayek]

@sebastienros I made the requested change here. The only issue I am seeing from a UI perspective here, if a site admin disable their own Features module, they'll get a 404 on the redirect. So instead of the 404, I redirect the user to the dashboard if they disabled their own features UI.

@jtkech Are you good with this PR or do you have any change to add?

[jtkech]

@MikeAlhayek Okay no problem but there is still something that I don't understand.

So now if a non Default tenant disable its own OC.Features it is redirected to the dashboard, okay good, but it seems that it is still allowed to do it.

I would have made another distinction in the view model, not only to know if this is the Default tenant, but also if it is rendered by itself. If by itself it would not be allowed to disable its own OC.Features, hmm looks like this same first condition could be also applied to the Default tenant.

Then if not managed by itself for now it means that it is by the Default tenant, but maybe safer to still check this other state before finally allowing to disable OC.Features of another tenant.

[MikeAlhayek]

@jtkech Thanks for your feedback.

So now if a non Default tenant disable its own OC.Features it is redirected to the dashboard, okay good, but it seems that it is still allowed to do it.

Yes this is allowed as per @sebastienros request. He said if they disable their own Features feature, no problem for us.

I would have made another distinction in the view model, not only to know if this is the Default tenant, but also if it is rendered by itself. If by itself it would not be allowed to disable its own OC.Features, hmm looks like this same first condition could be also applied to the Default tenant.

This is the exact behavior we have. Maybe we should rename the new IsDefaultTenant? I can't think of a good name.

Then if not managed by itself for now it means that it is by the Default tenant, but maybe safer to still check this other state before finally allowing to disable OC.Features of another tenant.

Now sure that I understand. In the controller we check to see if the current tenant can disable the feature.

[jtkech]

Yes this is allowed as per @sebastienros request. He said if they disable their own Features feature, no problem for us.

Okay I'm not against but this is to prevent this "issue" that we made this feature IsAlwaysEnabled which is no more the case, maybe worth to be confirmed, e.g. in case If it was just to make things simpler.

Easy to keep the IsAlwaysEnabled behavior (can't be disabled once enabled) except when this is the Default tenant that manages another tenant.

• If Manager == Managed => can't disable the Features feature.

• If Manager != Managed AND Manager is the Default tenant => can disable the Features feature.

Note: For now Manager != Managed => Manager is the Default tenant, so the above 2nd condition could be resumed to Manager != Managed.

[MikeAlhayek]

@jtkech Yes I do not disagree with you. This is what I had done. So basically Features will be always enabled for tenants once enabled. Only Default tenant will be able to disable it using the Features UI. If you watch the recording for last meeting on Tuesday, you’ll see his remarks.

The only issue with allowing the tenant to disable their own Features tenant, is that they can’t enable it again from the UI. They will be able to enable it via recipes is the deployment feature is enabled. Either way, let us know what do you recommend to do here and we can discuss with your feedback with @sebastienros

[jtkech]

Okay then, as said I'm open to any suggestion, just to be sure there was no confusion.

otherwise I would recommend to keep the previous behavior and only allow the Default tenant to make new things needed in this PR.

Hmm maybe @sebastienros doesn't want a feature which is no more IsAlwaysEnabled but that a tenant is still not able to disable for itself. Hmm but here still the case for the Default tenant on itself.

Hmm, maybe better to keep OC.Features as IsAlwaysEnabled (less change on what was before) and only allow the Default tenant to violate the disabling restriction when acting on another tenant.

[MikeAlhayek]

@sebastienros thoughts here? It sounds like your thoughts are the opposite of what is @jtkech is suggesting here. We just need a way to be able to disable Features for tenants.

[jtkech]

@sebastienros @MikeAlhayek

Just saw the last meeting, yes we can set a feature as IsAlwaysEnabled in a manifest file, it means that you can enable it but not disable it once enabled, yes I don't like the name but this is an existing setting.

It is only used by the UI and not managed by the infrastructure, so yes I think we can disable it through a recipe, but it was introduced to prevent people from breaking their tenant/site from the UI. For info we also have AlwaysEnabled features which are managed by the infrastructure but this is not the same.

And the Features feature has been marked as IsAlwaysEnabled to fix an issue opened at some point, but here this is no more the case to allow the Default tenant to disable it for other tenants.

So the goal (not saying it is good) was to keep the same IsAlwaysEnabled behavior for all tenants (including the Default) when they are managing their own features, even if hereFeatures is no longer marked as IsAlwaysEnabled, then only allow to disable this feature when a tenant is managing the features of another tenant (here for now can only be done by the Default tenant).

I'm not against anything but maybe keep the exact same behavior as before and then just allow the Default tenant to disable Features for other tenants, which is what introduce this PR.

Hmm, IMHO I think it is better to keep Features marked as IsAlwaysEnabled as it may have been done for a good reason, then just allow the disable button in the UI when we know that we are in the context (state retrieved from the model) of the Default tenant managing features of another tenant, which for now here can be resumed by: "a tenant is managing the features of another tenant".

Edited: Yes in place of removing IsAlwaysEnabled from the Features and try to keep the same behavior in the regular contexts, better I think to keep it as IsAlwaysEnabled and only allow to violate the rule when a tenant "manager" manages features of other "managed" tenants.

[jtkech]

@MikeAlhayek

Yes during the meeting @sebastienros was saying that it is not good to manage differently a given feature as OC.Features and that we would need a setting for that, okay I understand but we already have such a setting IsAlwaysEnabled and that is used for the Feature feature .

So we just need a confirmation, maybe he was not remembering about the IsAlwaysEnabled setting.

Anyway, as done currently where Features is no more IsAlwaysEnabled, we still need a special behavior to still not allow the Default tenant to disable it for himself (only others would be able to do that).

So my recommendation is to keep OC.Features as IsAlwaysEnabled and just allow the disable buton as a special behavior but only in the new special context of a [Default] tenant managing another tenant.

[jtkech]

@MikeAlhayek

I think we can safely re-use IsAlawaysEnabled for OC.Features because it would be as before.

If @sebastienros really wanted to change this existing behavior, will be easy to tweak again.

Then only allow the new behavior (the disable button) for the Default tenant when it manages another tenant, easy to add this state to the model, as I remember by checking if the shellSettings are different.

Then I will let you re-tried it a little, then I will approve and merge.

[MikeAlhayek]

@jtkech so to ensure I understand the request, you want to remove the IsAlwaysEnabled on the feature. However, a tenant should not be able to disable it on their own from their UI. A default tenant will still be able to enable/disable the feature for any other tenant. Correct? I'll try work on this tomorrow.

[jtkech]

@MikeAlhayek

you want to remove the IsAlwaysEnabled on the feature.

No, you already removed IsAlwaysEnabled = true from OC.Features, I suggest to re-use it instead, even if at the end we will have the same desired result.

However, a tenant should not be able to disable it on their own from their UI. A default tenant will still be able to enable/disable the feature for any other tenant. Correct?

Yes the goal will be to preserve this desired behavior, we will just have to change the view model state we pass to the view that will check it to allow the Disable button, this in a simpler way than before I think.

[MikeAlhayek]

If we set the IsAlwaysEnabled back to true, I don't think the default tenant will be able to disable the feature. I'll try it

[jtkech]

Do you mean it is not only checked by the UI? Let me check it.

It seems that IsAlwaysEnabled is only used by FeatureService.GetModuleFeaturesAsync() that is called by the Features Controller, so normally checking our state in the view would be okay.

[MikeAlhayek]

@jtkech you are right. IsAlwaysEnabled is just used by the UI. I made the requested changed. This is what we have now

1. OrchardCore.Features has IsAlwaysEnabled flag set.
2. If a Default tenant is executing on the behalf of other tenants, they are able to disable and re-enable Features feature for that tenant.
3. No tenant can disable Features module for their own instance.

I tested this out and all seems to be doing exactly what we want. Feel free to Merge or at least approve it and I'll merge it.