From 7e8e7aa012c83944c85b0846c5795ce330890752 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Thu, 27 Aug 2026 11:45:49 +0200 Subject: [PATCH] docs(secrets): document null-tenant index gaps and the MySql TFM pin (#7998) * docs(secrets): document null-tenant index gaps and the MySql TFM pin The per-tenant unique indexes only backstop rows whose TenantId is non-null, so single-tenant deployments and pre-upgrade rows fall back to the pre-save existence checks for name uniqueness. Recorded in secrets-tenancy.md and the authorization-model guide, with a comment at the EFCore secret repository's write path. Also documents why Elsa.Secrets.Persistence.EFCore.MySql stays pinned to net8.0/net9.0: Pomelo.EntityFrameworkCore.MySql tops out at EF Core 9, and the project references Elsa.Persistence.EFCore.MySql which carries the same pin. Comment-only csproj change; no behavior changes anywhere in this commit. Co-Authored-By: Claude Fable 5 * docs(secrets): say plainly that the null-tenant backfill is not a fix The guide implied backfilling TenantId to "" restored the uniqueness guarantee in single-tenant mode. It does not: disabled-mode writes keep persisting null, so new rows still land outside the index and two concurrent creates can still commit the same name. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Fable 5 --- doc/migrations/authorization-model.md | 2 + doc/migrations/secrets-tenancy.md | 39 +++++++++++++++++++ ...sa.Secrets.Persistence.EFCore.MySql.csproj | 9 ++++- .../Repositories/EFCoreSecretRepository.cs | 6 +++ 4 files changed, 55 insertions(+), 1 deletion(-) diff --git a/doc/migrations/authorization-model.md b/doc/migrations/authorization-model.md index f66009b03..78f02d45d 100644 --- a/doc/migrations/authorization-model.md +++ b/doc/migrations/authorization-model.md @@ -130,6 +130,8 @@ Included in the same release: `User.Name`, `Role.Name`, `Application.Name` and ` If you have duplicate names across tenants today, they were impossible to create, so no data conflict can arise. Going the other way — downgrading — will fail if duplicates exist by then. +One caveat: the composite indexes only cover rows whose `TenantId` is non-null (SQL Server filters null rows out of the index; SQLite, PostgreSQL and MySQL treat nulls as distinct — Oracle alone still rejects null-tenant duplicates). `TenantId` is only assigned when multitenancy is enabled, so in a single-tenant deployment every row keeps null and user, role and application name uniqueness becomes application-enforced rather than schema-enforced: the pre-save existence checks block sequential duplicates, but the database no longer backstops concurrent ones. Likewise, rows written before the upgrade keep a null `TenantId`, and in a multi-tenant deployment's default tenant those legacy rows and new `""`-tenant rows are distinct index keys, so the index cannot catch a name collision between them. See the same caveat, with the reasoning, in [secrets-tenancy.md](secrets-tenancy.md). + ## Full mapping | Legacy permission | Replacement | diff --git a/doc/migrations/secrets-tenancy.md b/doc/migrations/secrets-tenancy.md index 4aa9a5b3c..5b7f4112a 100644 --- a/doc/migrations/secrets-tenancy.md +++ b/doc/migrations/secrets-tenancy.md @@ -46,6 +46,45 @@ the first tenant to claim a name took it globally. Downgrading recreates the global unique index and **will fail if two tenants hold the same secret name by then**. Reconcile the duplicates first. +### Null-tenant rows sit outside the index + +"Unique per tenant" is enforced by the database only for rows whose `TenantId` is non-null. SQL Server +creates the composite index with a `[TenantId] IS NOT NULL` filter, and SQLite, PostgreSQL and MySQL treat +nulls as distinct in unique indexes — either way, null-tenant rows never collide in it. Only Oracle, where +equal nulls do count as duplicates in a composite unique index, still rejects them. + +This matters more than it sounds, because null is the common case. With multitenancy **disabled** — the +default single-tenant deployment — nothing ever assigns a `TenantId`, so every row keeps null and the schema +no longer enforces secret-name uniqueness at all. Uniqueness then rests on the repository's read-before-write +check, which blocks sequential duplicates but not two concurrent creates racing past it. The old global index +was the backstop for exactly that race; accepting its loss for null rows is a consequence of the no-backfill +decision above. The same gap applies in a multi-tenant deployment's default tenant: pre-upgrade null rows and +new `""`-tenant rows are distinct index keys, so the index cannot stop a new default-tenant secret from +colliding by name with a legacy row. + +Backfilling `TenantId` to `""` yourself does **not** restore the database guarantee, and it is worth being +precise about why. The backfill indexes the rows that exist when you run it, but nothing changes what happens +afterwards: with multitenancy disabled no `TenantId` is ever assigned, so every subsequent write still lands +as a null row outside the index. Two concurrent creates can still both pass the repository's read-before-write +check and commit the same name. Restoring the guarantee for real would mean making disabled-mode writes use +the same non-null sentinel the index is built on — Elsa does not do that, and the backfill alone does not +substitute for it. Until it does, treat the read-before-write check as the only protection in single-tenant +mode and serialize secret creation if you cannot tolerate the race. (The `SetTenantIdFilter` +null-compatibility clause keeps backfilled and straggler rows visible either way, so a backfill is still +useful for de-duplicating what you already have.) + +## The MySQL provider ships for net8.0 and net9.0 only + +`Elsa.Secrets.Persistence.EFCore.MySql` targets `net8.0;net9.0`, while the Sqlite, SQL Server, PostgreSQL and +Oracle secrets providers also target `net10.0`. That is a dependency constraint, not an oversight: +`Pomelo.EntityFrameworkCore.MySql` tops out at 9.0.0, built for EF Core 9, so there is no net10.0 provider to +build against. Every MySQL project in the repository carries the same pin, and the secrets one additionally +inherits it by referencing `Elsa.Persistence.EFCore.MySql`. + +A net10.0 host referencing the MySQL secrets provider resolves the net9.0 asset and runs normally, including +the `SecretTenancy` migration — migrations are ordinary C# and do not depend on the host framework. The pins +come out together once Pomelo ships for EF Core 10. + ## The VNext persistence provider does not support this `Elsa.Secrets.Persistence.VNext` stores documents keyed by secret name alone, and `Elsa.Persistence.VNext` has diff --git a/src/modules/Elsa.Secrets.Persistence.EFCore.MySql/Elsa.Secrets.Persistence.EFCore.MySql.csproj b/src/modules/Elsa.Secrets.Persistence.EFCore.MySql/Elsa.Secrets.Persistence.EFCore.MySql.csproj index 163f49f6c..5167acb62 100644 --- a/src/modules/Elsa.Secrets.Persistence.EFCore.MySql/Elsa.Secrets.Persistence.EFCore.MySql.csproj +++ b/src/modules/Elsa.Secrets.Persistence.EFCore.MySql/Elsa.Secrets.Persistence.EFCore.MySql.csproj @@ -1,7 +1,14 @@ - + net8.0;net9.0 Provides MySQL EF Core migrations for Elsa secrets. diff --git a/src/modules/Elsa.Secrets.Persistence.EFCore/Repositories/EFCoreSecretRepository.cs b/src/modules/Elsa.Secrets.Persistence.EFCore/Repositories/EFCoreSecretRepository.cs index c30d78f64..2f3a19980 100644 --- a/src/modules/Elsa.Secrets.Persistence.EFCore/Repositories/EFCoreSecretRepository.cs +++ b/src/modules/Elsa.Secrets.Persistence.EFCore/Repositories/EFCoreSecretRepository.cs @@ -116,6 +116,12 @@ public class EFCoreSecretRepository(Store store, I return dbContext.Secrets.AnyAsync(x => EF.Property(x, SecretShadowPropertyNames.NormalizedName) == normalizedName, cancellationToken); } + // The DbUpdateException-to-name-conflict translation below relies on the (TenantId, NormalizedName) + // unique index, which only covers rows with a non-null TenantId (SQL Server filters null rows out of the + // index; SQLite/PostgreSQL/MySQL treat nulls as distinct — Oracle alone rejects null-tenant duplicates). + // With multitenancy disabled nothing assigns a TenantId, so this backstop never fires there and + // uniqueness rests solely on the FindByNameAsync/ExistsByNormalizedNameAsync pre-checks — two concurrent + // creates racing past the pre-check both commit. See doc/migrations/secrets-tenancy.md. private async Task SaveChangesAsync(SecretsElsaDbContext dbContext, string name, CancellationToken cancellationToken) { try