-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Implement null TenantId for tenant-agnostic entities #7226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b43695c
Add ADR for Null Tenant ID, implement tenant-agnostic logic
sfmskywalker 4c8c841
Add multitenancy support in `ActivityTestFixture` by registering `ITe…
sfmskywalker fcb8b97
Update src/modules/Elsa.Common/Multitenancy/Implementations/DefaultTe…
sfmskywalker c81dc44
Remove unused `using Elsa.Common.Multitenancy;` from WorkflowDefiniti…
Copilot 55460bd
Optimize ActivityRegistry.Find to prefer tenant-specific descriptors …
Copilot ddffc29
Fix default tenant data visibility leak by removing NullIfEmpty conve…
Copilot a5dc667
Enhance logging in recurring tasks: add error handling and logger sup…
sfmskywalker File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
224 changes: 224 additions & 0 deletions
224
doc/adr/0009-asterisk-sentinel-value-for-tenant-agnostic-entities.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,224 @@ | ||
| # 9. Asterisk Sentinel Value for Tenant-Agnostic Entities | ||
|
|
||
| Date: 2026-01-31 | ||
|
|
||
| ## Status | ||
|
|
||
| Accepted | ||
|
|
||
| ## Context | ||
|
|
||
| The multitenancy system in Elsa supports both tenant-specific and tenant-agnostic entities. The convention established in ADR-0008 uses an empty string (`""`) as the tenant ID for the default tenant. However, we also need a way to represent entities that are **tenant-agnostic** - entities that should be visible and accessible across all tenants. | ||
|
|
||
| Previously, the system did not properly distinguish between: | ||
| 1. **Default tenant entities** (should only be visible to the default tenant with `TenantId = ""`) | ||
| 2. **Tenant-agnostic entities** (should be visible to all tenants regardless of their tenant context) | ||
|
|
||
| This caused issues where: | ||
| - Activity descriptors for built-in activities were being wiped out when different tenants were activated | ||
| - The `ActivityRegistry` used a single global dictionary that was replaced during tenant activation via `Interlocked.Exchange`, causing race conditions | ||
| - EF Core query filters only matched records with an exact tenant ID match, excluding tenant-agnostic records | ||
| - Workflows with no explicit tenant ID could not be found when a specific tenant context was active | ||
|
|
||
| ### Why Use a Sentinel Value Instead of Null? | ||
|
|
||
| We considered using `null` as the marker for tenant-agnostic entities, but chose an explicit sentinel value (`"*"`) instead for several reasons: | ||
|
|
||
| 1. **Explicit Intent**: A sentinel value makes it crystal clear in code and database queries that an entity is intentionally tenant-agnostic, not accidentally missing a tenant assignment | ||
| 2. **Simpler Composite Keys**: Avoids nullable handling complexity in composite keys like `(string TenantId, string Type, int Version)` which would become `(string? TenantId, ...)` | ||
| 3. **Clearer SQL Queries**: Database queries with `TenantId = '*'` are more explicit than `TenantId IS NULL` | ||
| 4. **Better Logging**: Seeing `"*"` in logs immediately signals tenant-agnostic behavior | ||
| 5. **Architecture Alignment**: Works seamlessly with the three-dictionary ActivityRegistry architecture where agnostic entities have their own dedicated registry | ||
|
|
||
| ## Decision | ||
|
|
||
| We will use the asterisk character (`"*"`) as a sentinel value to represent **tenant-agnostic entities** - entities that should be accessible across all tenants. This decision includes: | ||
|
|
||
| ### 1. Convention | ||
|
|
||
| - `"*"` (represented by constant `Tenant.AgnosticTenantId`) = tenant-agnostic (visible to all tenants) | ||
| - `""` (represented by constant `Tenant.DefaultTenantId`) = default tenant (visible only to default tenant) | ||
| - Any other non-null string = specific tenant (visible only to that tenant) | ||
| - `null` = not yet assigned (will be normalized to either agnostic or current tenant by handlers) | ||
|
|
||
| ### 2. Activity Registry Architecture | ||
|
|
||
| Implement a **three-dictionary architecture** in `ActivityRegistry` to properly isolate tenant-specific and tenant-agnostic descriptors: | ||
|
|
||
| - **`_tenantRegistries`**: `ConcurrentDictionary<string, TenantRegistryData>` - Per-tenant activity descriptors (e.g., workflow-as-activities) | ||
| - **`_agnosticRegistry`**: `TenantRegistryData` - Shared tenant-agnostic descriptors (e.g., built-in activities) | ||
| - **`_manualActivityDescriptors`**: `ISet<ActivityDescriptor>` - Legacy support for manually registered activities | ||
|
|
||
| Key behaviors: | ||
| - Descriptors with `TenantId = null` or `TenantId = "*"` are stored in `_agnosticRegistry` | ||
| - Descriptors with any other `TenantId` are stored in the corresponding tenant's registry in `_tenantRegistries` | ||
| - `RefreshDescriptorsAsync()` updates only the affected tenant's registry, not the entire global dictionary | ||
| - Find methods **always prefer tenant-specific descriptors over agnostic ones**, even if agnostic has a higher version number | ||
|
|
||
| ### 3. EF Core Query Filter | ||
|
|
||
| Update `SetTenantIdFilter` to return records where: | ||
| - `TenantId == dbContext.TenantId` (tenant-specific match), OR | ||
| - `TenantId == "*"` (tenant-agnostic records) | ||
|
|
||
| ### 4. Entity Handlers | ||
|
|
||
| Update `ApplyTenantId` handler to: | ||
| - Preserve `TenantId = "*"` (don't overwrite tenant-agnostic entities) | ||
| - Only apply current tenant ID to entities with `TenantId = null` | ||
|
|
||
| **Important: This is a security-by-default design.** Entities with `null` tenant ID are **never** automatically converted to tenant-agnostic (`"*"`). They are always assigned to the current tenant context. To create tenant-agnostic database entities, developers **must explicitly** set `TenantId = "*"`. This prevents accidental data leakage across tenants. | ||
|
|
||
| ### 5. Reserved Character Constraint | ||
|
|
||
| The asterisk character `"*"` is **reserved** and cannot be used as an actual tenant ID. Tenant creation and validation logic should reject any attempt to create a tenant with ID `"*"`. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
|
|
||
| - **Explicit tenant-agnostic marking**: The `"*"` sentinel makes intent clear in code, logs, and database | ||
| - **Proper tenant isolation**: The three-dictionary architecture prevents tenant activation from wiping out other tenants' descriptors | ||
| - **No nullable handling**: Composite keys remain `(string TenantId, ...)` instead of `(string? TenantId, ...)` | ||
| - **Tenant precedence**: Tenant-specific descriptors always take precedence over agnostic ones, allowing tenants to override built-in activities | ||
| - **Dynamic tenant management**: Tenants can be activated and deactivated at runtime without affecting each other | ||
| - **Database efficiency**: Tenant-agnostic entities are stored once and accessible to all tenants | ||
| - **Clear SQL queries**: `WHERE TenantId = current_tenant OR TenantId = '*'` is more explicit than null checks | ||
| - **Thread safety**: Per-tenant dictionaries eliminate the need for `Interlocked.Exchange` and its race conditions | ||
|
|
||
| ### Negative | ||
|
|
||
| - **Reserved character**: The `"*"` character cannot be used as an actual tenant ID (low impact, as tenant IDs are typically alphanumeric) | ||
| - **Two conventions**: Developers must understand the distinction between `"*"` (agnostic) and `""` (default tenant) | ||
| - **Migration complexity**: Existing systems using `null` for agnostic entities would need data migration | ||
|
|
||
| ### Neutral | ||
|
|
||
| - Using a sentinel value for special cases is a common pattern in software architecture | ||
| - The distinction between default tenant and tenant-agnostic is fundamental to proper multitenancy design | ||
| - The three-dictionary architecture adds complexity but is necessary for correct tenant isolation | ||
|
|
||
| ## Implementation Notes | ||
|
|
||
| ### Semantic Flow: From Entity Creation to Query | ||
|
|
||
| Understanding how tenant IDs flow through the system is critical: | ||
|
|
||
| ``` | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ Entity Creation / Deserialization │ | ||
| ├─────────────────────────────────────────────────────────────┤ | ||
| │ TenantId = null → Not yet assigned │ | ||
| │ TenantId = "*" → Explicitly agnostic │ | ||
| │ TenantId = "" → Default tenant │ | ||
| │ TenantId = "foo" → Specific tenant "foo" │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ↓ | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ ApplyTenantId Handler (before DB save) │ | ||
| ├─────────────────────────────────────────────────────────────┤ | ||
| │ TenantId = "*" → PRESERVED (agnostic) │ | ||
| │ TenantId = null → SET to current tenant from context │ | ||
| │ TenantId = "" → PRESERVED (default tenant) │ | ||
| │ TenantId = "foo" → PRESERVED (specific tenant) │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ↓ | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ Database Storage │ | ||
| ├─────────────────────────────────────────────────────────────┤ | ||
| │ TenantId = "*" → Stored as "*" (agnostic) │ | ||
| │ TenantId = "" → Stored as "" (default tenant) │ | ||
| │ TenantId = "foo" → Stored as "foo" (specific tenant) │ | ||
| │ NOTE: No null values in DB after ApplyTenantId handler │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ↓ | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ SetTenantIdFilter (EF Core Query) │ | ||
| ├─────────────────────────────────────────────────────────────┤ | ||
| │ Returns: TenantId == current_tenant OR TenantId == "*" │ | ||
| │ Result: Tenant-specific records + agnostic records │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ↓ | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ ActivityRegistry (In-Memory) │ | ||
| ├─────────────────────────────────────────────────────────────┤ | ||
| │ null or "*" → _agnosticRegistry (shared) │ | ||
| │ TenantId="" → _tenantRegistries[""] (default) │ | ||
| │ TenantId=X → _tenantRegistries[X] (specific tenant X) │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ``` | ||
|
|
||
| **Key Points:** | ||
| - **`null` is transient**: It only exists during entity creation/deserialization before `ApplyTenantId` runs | ||
| - **`"*"` is permanent**: Once set, it's preserved and stored in the database as-is | ||
| - **`NormalizeTenantId()` converts `null` → `""`**: This ensures `null` becomes the default tenant, NOT agnostic | ||
| - **Database has no nulls**: After `ApplyTenantId` handler, all entities have non-null tenant IDs | ||
| - **EF Core filters check for `"*"`**: The query filter explicitly compares against the string `"*"`, not null | ||
| - **ActivityRegistry accepts both**: For flexibility, in-memory registry treats both `null` and `"*"` as agnostic | ||
|
|
||
| ### ActivityRegistry Behavior | ||
|
|
||
| When `Find(string type)` is called: | ||
| 1. First check the current tenant's registry for matching descriptors | ||
| 2. If found, return the highest version from the tenant-specific registry | ||
| 3. Only if no tenant-specific descriptor exists, fall back to the agnostic registry | ||
| 4. This ensures tenant-specific customizations always take precedence | ||
|
|
||
| The `GetOrCreateRegistry()` method treats both `null` and `"*"` as agnostic: | ||
| ```csharp | ||
| if (tenantId is null or Tenant.AgnosticTenantId) | ||
| return _agnosticRegistry; | ||
| ``` | ||
|
|
||
| This provides flexibility for in-memory operations where activity descriptors might temporarily have `null` tenant IDs before normalization. | ||
|
|
||
| ### Activity Descriptors vs Database Entities: Different Rules | ||
|
|
||
| The system treats **in-memory activity descriptors** and **persistent database entities** differently for security and architectural reasons: | ||
|
|
||
| #### In-Memory Activity Descriptors (Ephemeral) | ||
| - Created on startup by activity providers | ||
| - **Built-in activities** (WriteLine, SetVariable, etc.): Created with `TenantId = null` by `ActivityDescriber` | ||
| - **Workflow-as-activities**: Created with `TenantId = definition.TenantId` by `WorkflowDefinitionActivityDescriptorFactory` | ||
| - `null` is acceptable here because descriptors are recreated on each startup and mapped to `_agnosticRegistry` | ||
| - No security risk: descriptors don't contain sensitive data, just metadata about activity types | ||
|
|
||
| #### Persistent Database Entities (WorkflowDefinition, etc.) | ||
| - Stored permanently in the database | ||
| - **Must explicitly set `TenantId = "*"`** to be tenant-agnostic | ||
| - `TenantId = null` is **never** converted to `"*"` - always assigned to current tenant | ||
| - **Security-by-default**: Prevents accidental data leakage across tenants | ||
| - A developer who forgets to set `TenantId` creates a tenant-specific entity, not a global one | ||
|
|
||
| **Example - Creating Tenant-Agnostic Workflow:** | ||
| ```json | ||
| { | ||
| "tenantId": "*", | ||
| "name": "GlobalApprovalWorkflow", | ||
| "description": "Shared across all tenants", | ||
| "root": { ... } | ||
| } | ||
| ``` | ||
|
|
||
| **Why This Asymmetry Is Important:** | ||
| 1. **Safety**: Database entities with null tenant ID default to current tenant (safe) | ||
| 2. **Explicitness**: Tenant-agnostic entities must be intentional (require `"*"`) | ||
| 3. **Different lifecycles**: Descriptors are ephemeral, entities are persistent | ||
| 4. **Backward compatibility**: Built-in activities work without modification | ||
|
|
||
| ### Workflow Import Behavior | ||
|
|
||
| When workflows are imported from providers (e.g., blob storage): | ||
| - Workflows without an explicit `tenantId` field in their JSON have `TenantId = null` | ||
| - During import, these are normalized to the current tenant ID via `NormalizeTenantId()` extension | ||
| - When saved to database, `ApplyTenantId` handler assigns the current tenant from context | ||
| - To create truly tenant-agnostic workflows, explicitly set `"tenantId": "*"` in the workflow JSON | ||
| - The `"*"` value will be preserved through import, save, and query operations | ||
|
|
||
| ### Testing Considerations | ||
|
|
||
| - Component tests use the default tenant (`""`) | ||
| - Built-in activities use the agnostic marker (`"*"`) | ||
| - Tenant-specific tests should create explicit tenant contexts to verify proper isolation | ||
| - Unit tests should verify that `"*"` is preserved through save operations | ||
| - Integration tests should verify that `"*"` entities are returned for all tenant contexts | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,22 @@ | ||
| digraph { | ||
| node [shape=plaintext]; | ||
| subgraph { | ||
| _1 [label="1. Record architecture decisions"; URL="0001-record-architecture-decisions.html"]; | ||
| _2 [label="2. Fault Propagation from Child to Parent Activities"; URL="0002-fault-propagation-from-child-to-parent-activities.html"]; | ||
| _1 -> _2 [style="dotted", weight=1]; | ||
| _3 [label="3. Direct Bookmark Management in WorkflowExecutionContext"; URL="0003-direct-bookmark-management-in-workflowexecutioncontext.html"]; | ||
| _2 -> _3 [style="dotted", weight=1]; | ||
| _4 [label="4. Activity Execution Snapshots"; URL="0004-activity-execution-snapshots.html"]; | ||
| _3 -> _4 [style="dotted", weight=1]; | ||
| _5 [label="5. Token-Centric Flowchart Execution Model"; URL="0005-token-centric-flowchart-execution-model.html"]; | ||
| _4 -> _5 [style="dotted", weight=1]; | ||
| _6 [label="6. Tenant Deleted Event"; URL="0006-tenant-deleted-event.html"]; | ||
| _5 -> _6 [style="dotted", weight=1]; | ||
| _7 [label="7. Adoption of Explicit Merge Modes for Flowchart Joins"; URL="0007-adoption-of-explicit-merge-modes-for-flowchart-joins.html"]; | ||
| _6 -> _7 [style="dotted", weight=1]; | ||
| _8 [label="8. Empty String as Default Tenant ID"; URL="0008-empty-string-as-default-tenant-id.html"]; | ||
| _7 -> _8 [style="dotted", weight=1]; | ||
| } | ||
| node [shape=plaintext]; | ||
| subgraph { | ||
| _1 [label="1. Record architecture decisions"; URL="0001-record-architecture-decisions.html"]; | ||
| _2 [label="2. Fault Propagation from Child to Parent Activities"; URL="0002-fault-propagation-from-child-to-parent-activities.html"]; | ||
| _1 -> _2 [style="dotted", weight=1]; | ||
| _3 [label="3. Direct Bookmark Management in WorkflowExecutionContext"; URL="0003-direct-bookmark-management-in-workflowexecutioncontext.html"]; | ||
| _2 -> _3 [style="dotted", weight=1]; | ||
| _4 [label="4. Activity Execution Snapshots"; URL="0004-activity-execution-snapshots.html"]; | ||
| _3 -> _4 [style="dotted", weight=1]; | ||
| _5 [label="5. Token-Centric Flowchart Execution Model"; URL="0005-token-centric-flowchart-execution-model.html"]; | ||
| _4 -> _5 [style="dotted", weight=1]; | ||
| _6 [label="6. Tenant Deleted Event"; URL="0006-tenant-deleted-event.html"]; | ||
| _5 -> _6 [style="dotted", weight=1]; | ||
| _7 [label="7. Adoption of Explicit Merge Modes for Flowchart Joins"; URL="0007-adoption-of-explicit-merge-modes-for-flowchart-joins.html"]; | ||
| _6 -> _7 [style="dotted", weight=1]; | ||
| _8 [label="8. Empty String as Default Tenant ID"; URL="0008-empty-string-as-default-tenant-id.html"]; | ||
| _7 -> _8 [style="dotted", weight=1]; | ||
| _9 [label="9. Asterisk Sentinel Value for Tenant-Agnostic Entities"; URL="0009-asterisk-sentinel-value-for-tenant-agnostic-entities.html"]; | ||
| _8 -> _9 [style="dotted", weight=1]; | ||
| } | ||
| } |
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
File renamed without changes.
File renamed without changes.
File renamed without changes.
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ADR (and the implementation) establishes "*" as the tenant-agnostic marker and treats null as unassigned, but the PR title/description says tenant-agnostic is represented by TenantId = null. Please align the PR description/title with the actual convention, or adjust the implementation if null is still intended to be the persisted marker.