After implementing a CustomTenantAccessor Signals do not work

[ricksearcy]

Related to this issue https://github.com/elsa-workflows/elsa-core/issues/2861 signals do not work after enabling the tenantId implementation.

[jruckert]

@sfmskywalker - I had to do some investigation around this last night actually (had to implement my own workaround for the Azure Service Bus trigger) to make it work in a multi-tenant environment.

To start the conversation/example:

https://github.com/elsa-workflows/elsa-core/blob/bab82b545023f638110037feac86259d169bf63a/src/activities/Elsa.Activities.AzureServiceBus/Services/Worker.cs#L119

The query here by default is passing in a TenantId = null

Question

"Is the intention behind multi-tenancy when a WorkflowsQuery TenantId = null is set to pickup all of the triggers/bookmarks that either have a TenantId set and Null entries? or just specifically null entries?"

Depending on the answer to this question would determine where the fix would be, as modifying the underlying FindManyAsync call may impact multiple areas to basically ignore the TenantId = null from the underlying EF query etc as its only returning any entry with a NULL tenant id by default.

Workaround for Azure Service Bus

My current workaround is to create a new "Tenants" HashSet (same as how you use Tags) to the Worker.cs file (as each Trigger/Bookmark has a string? TenantId property already it)

public HashSet<string?> Tenants { get; } = new();

Tags are set in this function here, so I have introduced a new parameter "string? tenantId" to this function.

https://github.com/elsa-workflows/elsa-core/blob/bab82b545023f638110037feac86259d169bf63a/src/activities/Elsa.Activities.AzureServiceBus/Services/WorkerManager.cs#L98

worker.Tenants.Add(tenantId);

then modifying the code in the Worker.cs file to loop through the tenants as per

foreach (var tenant in Tenants)
            {
                var launchContext = new WorkflowsQuery(ActivityType, bookmark, correlationId, TenantId: tenant);
                using var scope = _serviceScopeFactory.CreateScope();
                var workflowLaunchpad = scope.ServiceProvider.GetRequiredService<IWorkflowLaunchpad>();
                await workflowLaunchpad.CollectAndDispatchWorkflowsAsync(launchContext, new WorkflowInput(model), cancellationToken);
            }

Not sure if this is a good approach or not, but does work for me as an interim fix.

I'm happy to push a PR around this, but thought I would get your thoughts first around the intent behind TenantId = null and the scenarios you want to ensure are catered for.

[tomy2105]

Question

"Is the intention behind multi-tenancy when a WorkflowsQuery TenantId = null is set to pickup all of the triggers/bookmarks that either have a TenantId set and Null entries? or just specifically null entries?"

Depending on the answer to this question would determine where the fix would be, as modifying the underlying FindManyAsync call may impact multiple areas to basically ignore the TenantId = null from the underlying EF query etc as its only returning any entry with a NULL tenant id by default.

Yes, this is very good question. Since, as you already said, FindManyAsync and some others functions is implemented that null picks up only nulls I kind of assumed this was the intention and decided to live with it (although it created some problems I managed to work around).

Now that you ask the same question I'm definitely interested in @sfmskywalker opinion on what should be the intention when tenant id is null? Should null represent yet another possible value for tenant id or be a "get from all tenants" placeholder/indicator?

[sfmskywalker]

"Is the intention behind multi-tenancy when a WorkflowsQuery TenantId = null is set to pickup all of the triggers/bookmarks that either have a TenantId set and Null entries? or just specifically null entries?"

I would go with picking up only triggers/bookmarks that are NULL, where NULL is to be interpreted as the default tenant ID.

If the application is set up to be multi-tenant, then I think that every object should have a tenant ID. The NULL value could then be interpreted as the default tenant. Although it would probably be better to have an explicitly configured default tenant name, e.g. "Default" rather than NULL.

[jruckert]

We were able to work around this by creating our own implementation of the ISignaler

internal class TenantSignal : Signaler, ISignaler

It is literally a copy and paste of the original implementation with only one key difference.

1. Inject the ITenantAccessor into the contructor

public TenantSignal(IWorkflowLaunchpad workflowLaunchpad, ITokenService tokenService, ITenantAccessor tenantAccessor)
            :base(workflowLaunchpad, tokenService)
        {
            _workflowLaunchpad = workflowLaunchpad;
            _tokenService = tokenService;
            _tenantAccessor = tenantAccessor;
        }

2. Under the TriggerSignalAsync and DispatchSignalAsync get the tenantId and use inside the "CollectAndDispatchWorkflowsAsync"

e.g.

var normalizedSignal = signal.ToLowerInvariant();

            var tenantId = await _tenantAccessor.GetTenantIdAsync();

            return await _workflowLaunchpad.CollectAndExecuteWorkflowsAsync(new WorkflowsQuery(
                nameof(SignalReceived),
                new SignalReceivedBookmark { Signal = normalizedSignal },
                correlationId,
                workflowInstanceId,
                default,
                tenantId
            ), new WorkflowInput(new Signal(normalizedSignal, input)), cancellationToken);

Make sure under your service startup, that you inject this version instead of the default ISignaler

.AddScoped<ISignaler, TenantSignal>()

Works like a charm.

[tomy2105]

@jruckert why don't you do a PR with this?

[jruckert]

@tomy2105 this has been merged into master, so I think we can close this issue now?

[tomy2105]

@jruckert, yes, if the merge has been done, please close this one.

[jruckert]

I'm not the originator of the ticket, so can't close it @tomy2105 :-)

[sfmskywalker]

@ricksearcy we'll close this issue as completed, but please reopen if you think there's still something missing.