Assign Tenant.AgnosticTenantId in populator

[RalfvandenBurg]

When having Workflow in code. After updating to 3.6.0-rc3 WorkflowGraphs cannot be located any more, because the TenantId is set to the DefaultTenantId.

[RalfvandenBurg]

Fix for #7253

[sfmskywalker]

@copilot please review these changes and add/update unit & component tests.

[sfmskywalker]

@greptile

[greptile-apps]

Greptile Overview

Greptile Summary

This PR fixes a critical bug where entities were being persisted with NULL TenantId values instead of the default tenant ID (empty string), causing query filters to fail and making entities invisible to queries.

Changes Made

• Store.cs & TenantAwareDbContextFactory.cs: Changed from Tenant?.Id (which returns null for default tenant) to the TenantId property (which normalizes null to empty string). This ensures the EF Core context and bulk operations correctly assign "" as the tenant ID for the default tenant.

• DefaultWorkflowDefinitionStorePopulator.cs: Changed null handling from NormalizeTenantId() to using Tenant.AgnosticTenantId fallback, but this creates a logic inconsistency.

Issues Found

The changes in DefaultWorkflowDefinitionStorePopulator.cs introduce a logical flaw:

1. Line 72: Workflows with null tenant IDs are now normalized to "*" (tenant-agnostic) instead of "" (default tenant)
2. Line 195: When both workflow.Identity.TenantId and _tenantAccessor.Tenant?.Id are null, the workflow becomes tenant-agnostic instead of being assigned to the current tenant

According to ADR-0009, workflows without explicit tenant IDs should be assigned to the current tenant (empty string for default), not made tenant-agnostic (asterisk). Making workflows tenant-agnostic requires explicit developer intent by setting TenantId = "*".

Recommended Fix

Use _tenantAccessor.TenantId consistently throughout the populator, which already handles normalization internally.

Confidence Score: 2/5

• This PR has critical logic errors that could cause security issues by making workflows unintentionally tenant-agnostic
• While the changes in Store.cs and TenantAwareDbContextFactory.cs correctly fix the NULL TenantId persistence issue, the changes in DefaultWorkflowDefinitionStorePopulator.cs introduce a security flaw. Workflows without explicit tenant IDs will become tenant-agnostic (visible to all tenants) instead of being assigned to the current tenant. This violates the security-by-default design principle documented in ADR-0009 and could lead to unintended data leakage across tenants.
• Pay close attention to src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs - lines 72 and 195 need correction to use _tenantAccessor.TenantId instead of falling back to Tenant.AgnosticTenantId

Important Files Changed

Filename	Overview
src/modules/Elsa.Persistence.EFCore.Common/Store.cs	Changed from Tenant?.Id to TenantId property to ensure default tenant uses empty string instead of null
src/modules/Elsa.Persistence.EFCore.Common/TenantAwareDbContextFactory.cs	Changed from Tenant?.Id to TenantId property to ensure dbContext gets normalized tenant ID
src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs	Changed null handling to use AgnosticTenantId instead of NormalizeTenantId, but creates inconsistency with currentTenantId comparison

Sequence Diagram

sequenceDiagram
    participant WP as WorkflowProvider
    participant Pop as DefaultWorkflowDefinitionStorePopulator
    participant TA as ITenantAccessor
    participant Store as WorkflowDefinitionStore
    participant DB as Database (EF Core)
    participant Factory as TenantAwareDbContextFactory
    
    Note over Pop: PopulateStoreAsync called
    Pop->>TA: Get current tenant
    TA-->>Pop: Tenant (may be null for default)
    Note over Pop: OLD: Tenant?.Id.NormalizeTenantId()<br/>NEW: TenantId property
    
    Pop->>WP: GetWorkflowsAsync()
    WP-->>Pop: Workflows (some with TenantId=null)
    
    Note over Pop: For each workflow:
    Pop->>Pop: Normalize workflow.Identity.TenantId
    Note over Pop: ISSUE: null → "*" (AgnosticTenantId)<br/>SHOULD BE: null → "" (via TenantId)
    
    Pop->>Store: AddOrUpdateAsync(workflow)
    Store->>Factory: CreateDbContextAsync()
    Factory->>TA: Get TenantId property
    TA-->>Factory: Normalized tenant ID ("")
    Note over Factory: OLD: Tenant?.Id (null)<br/>NEW: TenantId property ("")
    Factory-->>Store: DbContext with TenantId=""
    
    Store->>DB: SaveChangesAsync()
    Note over DB: Entities saved with<br/>TenantId="" (default tenant)<br/>instead of NULL

Loading

[greptile-apps]

_{3 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs
use _tenantAccessor.TenantId instead of manual normalization

The ITenantAccessor.TenantId property already normalizes the tenant ID internally (see DefaultTenantAccessor.cs:10), so this manual call to NormalizeTenantId() is redundant.

        var currentTenantId = _tenantAccessor.TenantId;

This is consistent with the changes in Store.cs:185 and TenantAwareDbContextFactory.cs:35 which also switched to using the TenantId property.

[sfmskywalker]

Looks great. Thanks @RalfvandenBurg !

[sfmskywalker]

@RalfvandenBurg

Heads up: this change seems incorrect. What will happen now is that if a workflow without a tenant ID (e.g. a code-first workflow) will receive the ID of the current tenant, which is problematic given that this populator is executed for all tenants in the system. This means that the last tenant being iterated will be the owner of the code-first workflows.

for that reason, I will revert this part - but I also don't want to break your stuff, so if you let me know why this change was necessary in your scenarios, please let me know and we can find a better fix.

[RalfvandenBurg]

@sfmskywalker Thanks for the review (and also for correcting the missing class name mistake)
Here's some context and how this aligns with our setup:

1. CLR workflows never have null TenantId in our case
   We depend on a change in the ClrWorkflowsProvider that sets:
   TenantId = workflow.Identity.TenantId ?? Tenant.AgnosticTenantId
   So when workflows reach the store populator, CLR/as-code workflows never have null for Identity.TenantId. That avoids the “last tenant wins” issue for our CLR workflows, since the provider sets them to Tenant.AgnosticTenantId before the populator runs.

2. Why we changed the behavior
   With the original implementation, tenant-agnostic as-code workflows were being imported once per tenant, causing duplication. That’s what we were trying to fix.

3. How the fix works
   The filtering logic:
   if (definitionTenantId != currentTenantId && definitionTenantId != Tenant.AgnosticTenantId){ // skip – workflow belongs to another tenant continue;}
   means we only skip when the workflow belongs to a different tenant. When definitionTenantId == Tenant.AgnosticTenantId, we don’t skip and import the workflow as shared. That removes the per-tenant duplication for tenant-agnostic workflows.

4. For upstream Elsa
   In upstream Elsa, the default ClrWorkflowsProvider may still produce workflows with null TenantId, so your concern about ?? _tenantAccessor.TenantId is valid for the default implementation, and reverting it makes sense. A better upstream fix would be to handle null at the provider level (e.g. set Tenant.AgnosticTenantId when TenantId is null in ClrWorkflowsProvider) rather than in the populator. We used this change to test the behavior, and with your suggested revert our test cases still pass.

[sfmskywalker]

@RalfvandenBurg Thanks for the context, it makes sense 👍🏻