DI dependencies of features in the same assembly are mapped to the wrong feature

[gvkries]

Describe the bug

In case a single assembly contains several features, the dependencies added to the ITypeFeatureProvider may not be correct. All dependencies are added to one arbitrary feature, no matter which feature actually registers it in DI. I guess anything is added to the first feature being enabled.

In turn this causes things like the RoleUpdater to not correctly apply default permissions, because enabling a second feature in a single module is not correctly detected.

This can only be prevented, when all necessary dependencies are marked with the FeatureAttribute.

[Piedone]

This came up here.

[gvkries]

I've reviewed the ExtensionManager and it appears to function correctly and by design. It's essential to apply the FeatureAttribute to map specific types to features accurately.

Additionally, I've taken a brief look at ITypeFeatureProvider and found that the main issue arises with RoleUpdater when detecting whether to apply default permissions. This process relies on the correct feature-to-type mapping, which requires all IPermissionProvider implementations to be marked with the FeatureAttribute if more than one provider is present in a module.

Given this, I'm hesitant to recommend changes to the ExtensionManager, as it could lead us into complex territory. Instead, it may suffice to correct the IPermissionProviders by ensuring the FeatureAttribute is properly applied.

[MikeAlhayek]

Yes the solution is to use FeatureAttribute. The issue is that the ShellContainerFactory has no way of knowing which feature should the type belong to without explicitly using FeatureAttribute attribute.

OrchardCore/src/OrchardCore/OrchardCore/Shell/Builders/ShellContainerFactory.cs

Lines 150 to 158 in dc6729b

	var attribute = type.GetCustomAttributes<FeatureAttribute>(false).FirstOrDefault();
	if (attribute is not null)
	{
	feature = featureServiceCollection.Key.Extension.Features
	.FirstOrDefault(f => f.Id == attribute.FeatureName)
	?? feature;
	}
	}

[MikeAlhayek]

Oh and by the way, any implementation of IShapeTableProvider and data migration IDataMigration that belongs to a specific feature in a module SHOULD also be decorated with the feature attribute.

[Piedone]

That looks like an inconvenient API, since otherwise we only need [Feature]s for controllers, due to them being auto-registered, otherwise it's all up to the Startups. Can't we do better and rather figure out the registration source of the types, so we don't have to add a lot of attributes to the code for no obviously apparent reason?

[MikeAlhayek]

Yeah it would be nice if we don't have to use Feature attribute all over the place. I agree. Can you think of a better way to associate each time with a feature?

Maybe one way to eliminate the use of ITypeFeatureProvider. Or scan all the registered type when calling all the startups and assign each type to the feature if the startup.

[gvkries]

The ShellContainerFactory walks over all types and we can use that fact to update the mapping in the ITypeFeatureProvider, e.g. in the TypeFeatureProvider (https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore/OrchardCore/Extensions/Features/TypeFeatureProvider.cs) directly:

public void TryAdd(Type type, IFeatureInfo feature)
{
---     _features.TryAdd(type, feature);

+++  _ = _features.AddOrUpdate(type, (t, f) => f, (curType, curFeature, newFeature) =>
+++  {
+++    // If the type is currently only mapped to the module, update it with the more specific feature.
+++    if (curFeature != newFeature &&
+++        curFeature.Extension.Manifest.ModuleInfo.Id == curFeature.Id &&
+++        curFeature.Extension.Features.Contains(newFeature))
+++    {
+++      Debug.WriteLine($"TypeFeatureProvider changed mapping of type '{curType}' from '{curFeature.Id}' to '{newFeature.Id}'.");
+++      return newFeature;
+++    }
+++
+++    return curFeature;
+++  }, feature);
}

That would bring us almost there, all types are then assigned to the correct feature in the ITypeFeatureProvider.

But there is also the FeatureEntry.ExportedTypes property, which is populated by the ExtensionManager before the shell container factory is building the service collections. At this point we have no way of actually telling the correct feature without the attribute and the types assigned to the FeatureEntry would still not be correct and maybe worse, they would then be differently mapped as by the type feature provider.

As the FeatureEntry.ExportedTypes are only used to get the required features in the ComposisitionStrategy and again by the RoleUpdater, maybe the best solution is to get rid of FeatureEntry and reuse the ITypeFeatureProvider for this functionality as well.

[MikeAlhayek]

possible solution in this comment #15793 (comment)