Fix default tenant data visibility leak by removing NullIfEmpty conversion#7229
Merged
sfmskywalker merged 26 commits intoenh/tenant-agnostic-entitiesfrom Feb 1, 2026
Merged
Conversation
- Updated ElsaDbContextBase to use empty string for default tenant - Updated ApplyTenantId to stop converting empty string to null - Updated TenantAwareDbContextFactory to preserve empty string for default tenant - Updated Store.cs to preserve empty string for default tenant - This ensures: null = tenant-agnostic (visible to all), "" = default tenant Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
…qlServer - Added Management migration to convert null to "" for WorkflowDefinitions and WorkflowInstances - Added Runtime migration to convert null to "" for all runtime entities - This ensures existing default tenant data is properly migrated per ADR-0008 - Note: Similar migrations needed for PostgreSql, MySql, Sqlite, and Oracle providers Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix null TenantId implementation for tenant-agnostic entities
Fix default tenant data visibility leak by removing NullIfEmpty conversion
Jan 31, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical tenant data visibility leak in the persistence layer by removing NullIfEmpty() conversions that were incorrectly transforming empty string tenant IDs to null. According to ADR-0008 and ADR-0009, empty string ("") represents the default tenant (visible only to default tenant), while null represents tenant-agnostic entities (visible to all tenants). The previous conversion caused default tenant data to leak across all tenants.
Changes:
- Removed
.NullIfEmpty()conversions from persistence layer to preserve empty string tenant IDs - Added SQL Server database migrations to convert existing null TenantId values to empty strings (assumes pre-migration nulls are default tenant data)
- Updated four persistence layer components: ElsaDbContextBase, ApplyTenantId, TenantAwareDbContextFactory, and Store.cs
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/modules/Elsa.Persistence.EFCore.Common/ElsaDbContextBase.cs |
Removed .NullIfEmpty() call in constructor when setting TenantId from tenant accessor |
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/ApplyTenantId.cs |
Removed .NullIfEmpty() call when applying tenant ID to entities before saving |
src/modules/Elsa.Persistence.EFCore.Common/TenantAwareDbContextFactory.cs |
Removed .NullIfEmpty() call when setting tenant ID on created DbContext instances |
src/modules/Elsa.Persistence.EFCore.Common/Store.cs |
Removed .NullIfEmpty() call in bulk upsert operations when assigning tenant ID |
src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Runtime/20260131023442_ConvertNullTenantIdToEmptyString.cs |
Migration to convert null TenantId to empty string for 7 Runtime tables (ActivityExecutionRecords, BookmarkQueueItems, Bookmarks, KeyValuePairs, Triggers, WorkflowExecutionLogRecords, WorkflowInboxMessages) |
src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Runtime/20260131023442_ConvertNullTenantIdToEmptyString.Designer.cs |
Auto-generated EF Core migration designer file for Runtime context |
src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.cs |
Migration to convert null TenantId to empty string for 2 Management tables (WorkflowDefinitions, WorkflowInstances) |
src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.Designer.cs |
Auto-generated EF Core migration designer file for Management context |
Files not reviewed (2)
- src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.Designer.cs: Language not supported
- src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Runtime/20260131023442_ConvertNullTenantIdToEmptyString.Designer.cs: Language not supported
src/modules/Elsa.Persistence.EFCore.Common/ElsaDbContextBase.cs
Outdated
Show resolved
Hide resolved
Copilot stopped work on behalf of
sfmskywalker due to an error
January 31, 2026 09:47
- Introduce `AgnosticTenantId` constant to manage tenant-agnostic entities. - Modify entity handling logic to respect tenant-agnostic designations. - Adjust workflow processing to include tenant-agnostic workflows. - Update caching and activity descriptor logic to accommodate the `AgnosticTenantId`.
…or improved clarity and separation of tenant-specific and tenant-agnostic activity descriptors. Remove `TestTenantResolver` and update workflow definition handling for tenant support.
…to copilot/sub-pr-7226-another-one
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 31 changed files in this pull request and generated 12 comments.
Files not reviewed (2)
- src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.Designer.cs: Language not supported
- src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Runtime/20260131023442_ConvertNullTenantIdToEmptyString.Designer.cs: Language not supported
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/SetTenantIdFilter.cs
Show resolved
Hide resolved
...ce.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.cs
Show resolved
Hide resolved
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/ApplyTenantId.cs
Show resolved
Hide resolved
...tence.EFCore.SqlServer/Migrations/Runtime/20260131023442_ConvertNullTenantIdToEmptyString.cs
Outdated
Show resolved
Hide resolved
…ver tenant-agnostic and simplify descriptor retrieval logic.
…tasks without blocking
…ties Replace the previous convention of using `null` for tenant-agnostic entities with an asterisk (`"*"`) for improved clarity and system architecture. Updated ADR documentation, TOC, and dependency graph accordingly.
…d designer file to clean up the codebase.
…ogic and simplify tenant ID checks.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 34 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.Designer.cs: Language not supported
...ce.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.cs
Show resolved
Hide resolved
...ce.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…able `currentTenantId`.
… into copilot/sub-pr-7226-another-one
Document the tenant ID flow from entity creation to query, emphasizing normalization and tenant-agnostic workflows. Update semantic flow diagrams and provide testing considerations for preserving `"*"` values in multi-tenant scenarios.
Enhance Architecture Decision Record to detail explicit requirements for tenant-agnostic database entities, highlighting differences between in-memory activity descriptors and persistent entities. Emphasize importance of setting `TenantId = "*"` to prevent accidental data leakage.
…agnostic IDs, reducing redundant processing.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 23 changed files in this pull request and generated 7 comments.
Files not reviewed (1)
- src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.Designer.cs: Language not supported
...ce.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.cs
Show resolved
Hide resolved
...ce.EFCore.SqlServer/Migrations/Management/20260131023442_ConvertNullTenantIdToEmptyString.cs
Show resolved
Hide resolved
src/common/Elsa.Mediator/Middleware/Command/Components/CommandHandlerInvokerMiddleware.cs
Show resolved
Hide resolved
sfmskywalker
force-pushed
the
copilot/sub-pr-7226-another-one
branch
from
94d4506 to
de0f654
Compare
…nal task cancellation.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… tenant-agnostic descriptors.
… into copilot/sub-pr-7226-another-one
sfmskywalker
added a commit
that referenced
this pull request
Feb 2, 2026
* Add ADR for Null Tenant ID, implement tenant-agnostic logic Introduce ADR-0009 to document the use of `null` for tenant-agnostic entities, enhancing multitenancy handling. Update multitenancy features across the codebase, including EF Core query filters and ActivityRegistry, to handle null as a tenant ID, ensuring tenant-agnostic entities are accessible across all tenants. * Add multitenancy support in `ActivityTestFixture` by registering `ITenantAccessor`. * Update src/modules/Elsa.Common/Multitenancy/Implementations/DefaultTenantAccessor.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Remove unused `using Elsa.Common.Multitenancy;` from WorkflowDefinitionActivityDescriptorFactory (#7230) * Initial plan * Remove unused using Elsa.Common.Multitenancy statement Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Optimize ActivityRegistry.Find to prefer tenant-specific descriptors without performance regression (#7227) * Initial plan * Optimize Find(string type) to prefer tenant-specific descriptors with single-pass iteration Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Apply review feedback: combine if statements and add comprehensive unit tests Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Refactor tests for DRYness using theories and helper methods Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Clean up extra whitespace in test file Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Fix default tenant data visibility leak by removing NullIfEmpty conversion (#7229) * Initial plan * Remove NullIfEmpty conversion to align with ADR-0008 and ADR-0009 - Updated ElsaDbContextBase to use empty string for default tenant - Updated ApplyTenantId to stop converting empty string to null - Updated TenantAwareDbContextFactory to preserve empty string for default tenant - Updated Store.cs to preserve empty string for default tenant - This ensures: null = tenant-agnostic (visible to all), "" = default tenant Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Add database migration to convert null TenantId to empty string for SqlServer - Added Management migration to convert null to "" for WorkflowDefinitions and WorkflowInstances - Added Runtime migration to convert null to "" for all runtime entities - This ensures existing default tenant data is properly migrated per ADR-0008 - Note: Similar migrations needed for PostgreSql, MySql, Sqlite, and Oracle providers Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Clarify tenant handling logic in `ElsaDbContextBase` with new ADR references. * Add tenant ID convention analysis documents and quick reference guide * Implement tenant-agnostic functionality across modules - Introduce `AgnosticTenantId` constant to manage tenant-agnostic entities. - Modify entity handling logic to respect tenant-agnostic designations. - Adjust workflow processing to include tenant-agnostic workflows. - Update caching and activity descriptor logic to accommodate the `AgnosticTenantId`. * Refactor tenant management and registry logic in `ActivityRegistry` for improved clarity and separation of tenant-specific and tenant-agnostic activity descriptors. Remove `TestTenantResolver` and update workflow definition handling for tenant support. * Refactor `ActivityRegistry`: prioritize tenant-specific descriptors over tenant-agnostic and simplify descriptor retrieval logic. * Improve async handling in `CommandHandlerInvokerMiddleware` to await tasks without blocking * Update ADR to use asterisk as sentinel value for tenant-agnostic entities Replace the previous convention of using `null` for tenant-agnostic entities with an asterisk (`"*"`) for improved clarity and system architecture. Updated ADR documentation, TOC, and dependency graph accordingly. * Remove migration `ConvertNullTenantIdToEmptyString` and its associated designer file to clean up the codebase. * Refactor `ActivityRegistry`: streamline activity descriptor removal logic and simplify tenant ID checks. * Update Elsa.sln Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Simplify `RefreshDescriptorsAsync` by removing unnecessary local variable `currentTenantId`. * Remove unused `currentTenantId` variable from `ActivityRegistry`. * Add detailed semantic flow and key points to ADR 0009 Document the tenant ID flow from entity creation to query, emphasizing normalization and tenant-agnostic workflows. Update semantic flow diagrams and provide testing considerations for preserving `"*"` values in multi-tenant scenarios. * Remove outdated Tenant ID Analysis and associated documents * Add security-by-default design for tenant-agnostic entities in ADR Enhance Architecture Decision Record to detail explicit requirements for tenant-agnostic database entities, highlighting differences between in-memory activity descriptors and persistent entities. Emphasize importance of setting `TenantId = "*"` to prevent accidental data leakage. * Normalize tenant ID grouping in `ActivityRegistry` to unify null and agnostic IDs, reducing redundant processing. * Refactor `SignalManager`: improve timeout handling and streamline signal task cancellation. * Update src/modules/Elsa.Workflows.Core/Models/TenantRegistryData.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/modules/Elsa.Workflows.Core/Services/ActivityRegistry.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Refactor tests to use `Tenant.AgnosticTenantId` instead of `null` for tenant-agnostic descriptors. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Enhance logging in recurring tasks: add error handling and logger support to prevent crashes in scheduled timers. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement asterisk sentinel value for tenant-agnostic entities
This change introduces the asterisk character (
"*") as a sentinel value to represent tenant-agnostic entities in the Elsa Workflows engine. It also refactors theActivityRegistryto use a three-dictionary architecture for tenant isolation and updates EF Core query filters to include tenant-agnostic records.Changes
ADR Update
Replaces the concept of using
nullas the tenant ID for tenant-agnostic entities with the asterisk character ("*") as documented indoc/adr/0009-asterisk-sentinel-value-for-tenant-agnostic-entities.md.Tenant ID Convention
Introduces
Tenant.AgnosticTenantIdconstant ("*") to represent tenant-agnostic entities and enforces its use in code.Tenant.DefaultTenantIdremains""for the default tenant.ActivityRegistry Refactoring
Implements a three-dictionary architecture in
ActivityRegistryusing:_tenantRegistries(tenant-specific)_agnosticRegistry(tenant-agnostic)_manualActivityDescriptors(legacy)ActivityRegistrymethods are updated to use the new architecture and prioritize tenant-specific descriptors.EF Core Query Filter Update
Modifies
SetTenantIdFilterto include records whereTenantId == dbContext.TenantIdORTenantId == "*".Entity Handler Update
Updates
ApplyTenantIdto preserveTenantId = "*"and only apply the current tenant ID to entities whereTenantId == null.Migration Removal
Removes the
ConvertNullTenantIdToEmptyStringmigration, as the approach to tenant-agnostic entities has changed.Database Context Modification
Updates
ElsaDbContextBaseandTenantAwareDbContextFactoryto align with the new tenant ID handling strategy.Testing Updates
ComponentTestTenantResolverto resolve toTenant.DefaultTenantId.TestTenantResolver.Impact
Behavioral Changes
Entities with
TenantId = "*"are now treated as tenant-agnostic and accessible across all tenants.Entities with
TenantId = nullare assigned to the current tenant.Structural Changes
The
ActivityRegistrynow uses a three-dictionary architecture for improved tenant isolation and performance.Dependencies Affected
Affects all modules and components that interact with multitenant entities (e.g.,
WorkflowDefinition,WorkflowInstance), especially those usingITenantAccessoror querying the database.Breaking Changes
Existing systems relying on
nullfor tenant-agnostic entities require data migration.The
"*"character is now reserved and cannot be used as a tenant ID.Performance Implications
Storing tenant-agnostic entities only once improves database efficiency.
The new
ActivityRegistryarchitecture potentially improves thread safety and reduces race conditions.