Feature check inconsistency #9364

bmulinari created 21 days ago

We are having an odd issue with the feature check where a feature can be considered both enabled and disabled depending on where it's checked.

For instance, we have a feature check on the menu contributor to only show the corresponding menu if the feature is enabled:

if (context.Menu.Name == StandardMenus.Main && await featureChecker.IsEnabledAsync(MyDefinitionProvider.MyFeature))
{
    // this correctly shows or hides the menu depending on the feature status
    await ConfigureMainMenuAsync(context);
}

However, when doing the same check on a background worker, sometimes we get a different result:

protected override async Task DoWorkAsync(PeriodicBackgroundWorkerContext workerContext)
{
    await Parallel.ForEachAsync(await TenantStore.GetListAsync(), tenantParallelOptions, async (tenant, ct) =>
    {
        try
        {
            using (CurrentTenant.Change(tenant.Id))
            {
                if (!await featureChecker.IsEnabledAsync(MyDefinitionProvider.MyFeature))
                    return;

                // some logic here that should only execute when the feature is enabled
                // this is where we get the inconsistency, since even with the feature enabled and menu visible, sometimes this is not executed
            }
        }
        catch (Exception ex)
        {
            Logger.LogError(ex, "Error for tenant {TenantId}", tenant.Id);
        }
    });
}

This is a bit convoluted to reproduce, but from what we could gather it has something to do with enabling/disabling the feature for the tenant or edition, and the edition being assigned/unassigned from the tenant.

Ensure the feature is enabled in both edition and tenant
Unassign the edition from the tenant
Reassign the edition to the tenant

This can result in the scenario below where the row that holds the tenant status for the feature is simply deleted from the table. And THIS is when we observe the inconsistency stated above.

I know it's a little confusing, but if necessary we can schedule a call to explain in detail.

maliming created 19 days ago

Support Team Fullstack Developer

We have fixed the delete problem in https://github.com/abpframework/abp/pull/22628/files#diff-bddeb634046b275c40a4ba7f7f33c08c801dbe6d33f24d729ab0a6f741f044d1

You can override the SettingManager.cs

Thanks.

bmulinari created 17 days ago

Hi, @maliming. I've tried doing the override you mentioned, but it doesn't seem to make a difference.

I managed to narrow it down and I've created a fresh project that reproduces the issue. You should receive the code in your email.

Here are the steps to reproduce:

Run DB migrator
Start Blazor App and login to host
Create a new tenant
Assign Standard edition To tenant
Go to editions and enable "My Custom Feature" for the Standard edition
Login to the tenant and navigate to the Dashboard
You should see the feature check displaying the feature as enabled in the page
However, in the app console, you'll see the log from the background worker showing the same feature as disabled

Here's an image demonstrating the issue on the sample project:

maliming created 17 days ago

Support Team Fullstack Developer

We will support this case by https://github.com/abpframework/abp/pull/22632

You can add MyEditionFeatureValueProvider to fix it now.

public class FeatureTestDomainModule : AbpModule
{
    public override void ConfigureServices(ServiceConfigurationContext context)
    {
        //...

        PostConfigure<AbpFeatureOptions>(options =>
        {
            options.ValueProviders.InsertAfter(r => r == typeof(EditionFeatureValueProvider),
                typeof(MyEditionFeatureValueProvider)
            );
            options.ValueProviders.Remove<EditionFeatureValueProvider>();
        });

        //...
    }
}

using System.Security.Principal;
using System.Threading.Tasks;
using Volo.Abp.Features;
using Volo.Abp.MultiTenancy;
using Volo.Abp.Security.Claims;
using Volo.Saas.Tenants;

namespace FeatureTest;

public class MyEditionFeatureValueProvider : FeatureValueProvider
{
    public const string ProviderName = "E";

    public override string Name => ProviderName;

    protected ICurrentPrincipalAccessor PrincipalAccessor;

    protected ITenantRepository TenantRepository;

    protected ICurrentTenant CurrentTenant { get; }

    public MyEditionFeatureValueProvider(IFeatureStore featureStore, ICurrentPrincipalAccessor principalAccessor, ITenantRepository tenantRepository, ICurrentTenant currentTenant)
        : base(featureStore)
    {
        PrincipalAccessor = principalAccessor;
        TenantRepository = tenantRepository;
        CurrentTenant = currentTenant;
    }

    public override async Task<string?> GetOrNullAsync(FeatureDefinition feature)
    {
        var editionId = PrincipalAccessor.Principal?.FindEditionId();
        if (editionId == null)
        {
            if (!CurrentTenant.Id.HasValue)
            {
                return null;
            }

            var tenant = await TenantRepository.FindByIdAsync(CurrentTenant.Id.Value);
            if (tenant == null || !tenant.EditionId.HasValue)
            {
                return null;
            }

            editionId = tenant.EditionId;
        }

        return await FeatureStore.GetOrNullAsync(feature.Name, Name, editionId.Value.ToString());
    }
}

bmulinari created 16 days ago

Thanks, @maliming. This seems to solve the issue.

From my understanding looking at GitHub these fixes will be available in version 9.3, is that right? I just want to confirm that it will be safe to remove these once that version is out and we upgrade to it.

Thank you.