fix(identity): tenant-safe role stores for Dapper and Mongo (elsa-core#8615)

[sfmskywalker]

Fixes #282
Follows elsa-core#8615 and the core design in elsa-core#8616 (not merged yet). Role ids used to be derived from the role name and were unique across the whole store, so tenants sharing one store could not have same-named roles. Core now generates role ids and looks up roles by name within the tenant. This PR makes the extension role stores match that.

The Dapper Ids → Name mapping is not only a multi-tenant bug. Once core generates ids, a single-tenant install also fails: a newly seeded admin role has a generated id, User.Roles still stores that id, and FindByIdsAsync / RoleFilter.Ids would look the id up in the Name column and miss. Mapping Ids to Id is required before ext takes a core preview that includes #8616, including for hosts with one tenant.

Release note. Dapper users whose User.Roles / Application.Roles hold a legacy id that differs from the role name (for example power-user) now receive that role's permissions on upgrade. Before this PR those lookups searched the Name column and missed.

Scope is the Dapper and Mongo role stores and their tests only. No data migration: existing name-derived ids (id == name) keep resolving via RoleFilter.Id / Ids.

Dapper (DapperRoleStore)

• RoleFilter.Ids now maps to the Id column. It previously mapped to Name (RoleStore.cs filter), which only worked because id equalled name. Generated ids would not have resolved for User.Roles / Application.Roles (FindByIdsAsync), on single-tenant and shared-store hosts alike.
• Upsert tenant guard (role store only). Store.SaveAsync upserts on Id alone (SQLite INSERT OR REPLACE, SQL Server MERGE). That re-homed another tenant's row when ids collided. DapperRoleStore.SaveAsync now updates only a row the ambient tenant already owns via the filtered UpdateAsync overload (Store.cs applies the tenant WHERE plus Id), inserts when the id is unused, and refuses when the id belongs to a different tenant or a shared * row (message distinguishes those cases). Existing name-derived rows are not rewritten.
• This role-store guard covers the role-id collision case called out in Dapper Store: SetTenantId overwrites every TenantId (incl. "*"), key-only upserts allow cross-tenant overwrite, and reads have no "*" visibility #245. It does not close Dapper Store: SetTenantId overwrites every TenantId (incl. "*"), key-only upserts allow cross-tenant overwrite, and reads have no "*" visibility #245: that issue is the generic Store<T> (SetTenantId overwriting * and explicit tenants, key-only upserts on every entity, no * read visibility). Those remain a separate Store-level fix.
• RoleFilter.Name is deferred. This repo pins ElsaVersion 3.10.0-preview.5760. That RoleFilter has Id, Ids, and TenantId only — no Name yet. The Ids→Id fix, the tenant guard, the Mongo index, and the Dapper unique index do not need it. A small bump PR will add .Is(nameof(RoleRecord.Name), filter.Name) once #8616 is published to feedz.

Dapper unique index (Identity:V3.10 / IX_Roles_TenantId_Name)

3.9 Roles had no name uniqueness (PK is Id only; V3_3 added nullable TenantId). Two tenants must be able to share admin; one tenant must not.

Mechanism. New FluentMigrator migration V3_10 (version 30005, description Elsa:Identity:V3.10) in Elsa.Persistence.Dapper.Migrations.Identity, same assembly and style as Initial / V3_1 / V3_2 / V3_3. Create.Index(...).OnColumn("TenantId").OnColumn("Name").WithOptions().Unique() is provider-neutral, so SQLite, SQL Server, PostgreSQL, MySQL, and Oracle all get IX_Roles_TenantId_Name. Existence guards no-op if Roles, TenantId, or the index is already there. Down drops the index.

Existing 3.9 data — fail closed, never rewrite. FluentMigrator has no first-class "skip this step, log, and still record the version" that would leave later hosts without uniqueness. Logging-and-skipping while applying 30005 would hide collisions and skip the index forever. So the migration:

1. Reads Roles (Id, TenantId, Name) using the repo's IfDatabase + MigrationDatabases pattern (QuotedIdentifiers for PostgreSQL/Oracle, UnquotedIdentifiers for SQLite/SQL Server/MySQL).
2. Groups by coalesced tenant (NULL and '' → default) and Name.ToLowerInvariant().
3. If any group has more than one row, throws InvalidOperationException listing tenant, stored name spellings, and ids, and says "No rows were changed."
4. FluentMigrator does not record version 30005 (the Create.Index never runs). The operator renames or deletes the extras and re-runs.

No rows are deleted, renamed, or tenant-rewritten.

Case. RoleManager compares names with OrdinalIgnoreCase and does not trim. Duplicate detection matches that for case (invariant lower) and does not treat "admin" / "admin " as the same. The unique index itself is on the stored columns and follows the database collation: typically case-insensitive on SQL Server (matches RoleManager), case-sensitive on SQLite / PostgreSQL / MySQL / Oracle. Case variants on SQLite and PostgreSQL therefore rely on core's OrdinalIgnoreCase pre-save check; the index only rejects exact stored names there. Detection refuses 3.9 case-variants at upgrade time so they cannot land under a case-sensitive index.

NULL vs '' tenant ids. Store.SetTenantId writes tenant?.Id (null for the default tenant). ApplyTenantFilter treats NULL and '' as the same default tenant (IsNullOrEmpty). Duplicate detection matches that (both key as default), so a mixed NULL/'' pair with the same name fails the upgrade. The unique index does not:

• SQL Server treats NULLs as equal in a unique index, so one (NULL, Name) per name is allowed (right outcome for legacy default-tenant rows).
• SQLite, PostgreSQL, MySQL, and Oracle treat NULLs as distinct, so after a clean migrate a second (NULL, 'admin') is allowed, and (NULL, 'admin') plus ('', 'admin') can both exist.

Closing that gap needs NULL/'' normalisation (elsa-extensions#242 / #245), not a data rewrite here.

Mongo (MongoRoleStore / CreateIndices)

• MongoRoleStore still uses filter.Apply. It will pick up RoleFilter.Name for free once ext builds against a core that has #8616. No filter change in this PR.
• Upsert tenant guard is already on MongoDbStore. Covered again through MongoRoleStore tests.
• Index change (roles only): drop the store-wide unique index on Role.Name and create a compound unique index on (TenantId, Name).
  • Before (3.9.0 shape): Name_1 (unique on Name), TenantId_1, _id_
  • After: TenantId_1_Name_1 (unique on TenantId+Name), TenantId_1, _id_
• Create then drop (concurrent startup). Create TenantId_1_Name_1 first if missing (the two unique indexes can coexist; existing Name_1 data always satisfies the compound). Then drop Name_1. A second node whose DropOne races gets IndexNotFound (code 27) and logs "was not found" at Debug instead of failing IHostedService.StartAsync. That keeps name uniqueness on the collection the whole time and makes the drop idempotent across nodes.
• Idempotent: create TenantId_1_Name_1 only if it is missing; drop Name_1 by that exact name and treat code 27 as already dropped. Logs: created / already present / not found at Debug; the single "Dropped … Name_1" line at Information.
• Not touched: User.Name (Name_1 unique) and Application.Name / Application.ClientId (Name_1 / ClientId_1 unique).

Proved against a 3.9.0-shaped database that already had roles in two tenants and the old single-field index: create-then-drop succeeds on that data, two tenants can then hold a same-named role, a same-tenant duplicate is rejected, a second run is a no-op, and a start after Name_1 was already dropped (or after the compound was already created) does not throw.

Tests

Dapper (SQLite) and Mongo (Testcontainers mongo:7.0.24) cover:

• legacy id == name still resolves via Id / Ids
• generated ids resolve via Ids (Dapper also asserts Ids no longer matches Name)
• two tenants on one store with a same-named role
• a role in tenant A is invisible to tenant B
• an upsert cannot hijack another tenant's row or a shared * row
• Mongo index migration on existing 3.9.0-shaped data, plus idempotency, already-dropped Name_1, compound-already-created, and User/Application indexes left intact
• Dapper V3_10 on a 3.9-shaped SQLite database that already has two tenants with a same-named role plus legacy id == name rows: index is created, rows are unchanged, and those ids still resolve
• Dapper duplicate-detection path: exact same-tenant names, case variants, and NULL/'' default-tenant pairs fail the migration, leave rows unchanged, and do not record version 30005
• After V3_10, a same-tenant duplicate insert is rejected both through DapperRoleStore.SaveAsync and raw SQL

Analysis (not in this PR — candidate 3.10 follow-up)

Mongo still has store-wide unique indexes on:

Collection	Index	Blocks shared-store multi-tenancy?
User.Name	Name_1 unique	Yes. Two tenants cannot each have a user named admin. Same class of bug as roles, for the default admin user.
Application.Name	Name_1 unique	Yes. Two tenants cannot each have an application with the same display name.
Application.ClientId	ClientId_1 unique	Maybe should stay global. The client is resolved from the incoming credential before the tenant is known, so a lookup by ClientId cannot apply a tenant filter. A compound (TenantId, ClientId) unique index would allow two tenants to reuse a client id, and the first match (or a default-tenant miss) would be wrong. Treat ClientId as store-global unless the token/API-key pipeline is taught to resolve tenant first.

Dapper Roles now has IX_Roles_TenantId_Name. Dapper Users and Applications still have no unique indexes on Name or ClientId — only the Id primary key (Initial + V3_3 TenantId column). Dapper therefore does not reject same-named users or applications at the database. Lookups still go through the store's tenant filter (UserFilter.Name, ApplicationFilter.Name / ClientId), so same-tenant duplicates are possible and a pre-tenant ClientId lookup would see only the ambient/default tenant's row. That is a separate 3.10 issue, not this PR.

  

[sfmskywalker]

Elsa 3 Code Review: REQUEST_CHANGES + MEDIUM-HIGH @ a592352

Code Review, Round 1/4

The Dapper half, including the new V3_10 migration, is correct. The Mongo index migration still drops the old index before creating the new one, so several nodes starting together can fail host startup. That is the only blocker.

Blocker

1. The Mongo role index migration is unchanged at this head and is not safe when several nodes start at the same time. CreateIndices.cs:100-110 lists the indexes, then drops Name_1 at line 104. Two nodes starting together can both see Name_1. The second DropOneAsync then fails with IndexNotFound (code 27). IndexHelpers.CreateAsync does not catch it, and this runs inside IHostedService.StartAsync (CreateIndices.cs:28-35), so that node fails to start. Before this PR the role step only created identical indexes, which is idempotent across nodes. Between the drop at line 104 and the create at line 119, the collection also has no name uniqueness. Fix: create TenantId_1_Name_1 first (the two indexes can coexist, and existing data always satisfies the compound because the old index was stricter), then drop Name_1, and treat a MongoCommandException with code 27 as already dropped.

V3_10 (Dapper unique index on Roles (TenantId, Name))

• Ordering: [Migration(30005, "Elsa:Identity:V3.10")] follows Identity 30001 to 30004 and matches the naming of the existing Identity and Runtime migrations.
• Index creation: FluentMigrator 7.2 emits a plain CREATE UNIQUE INDEX on (TenantId, Name) for SQL Server 2008 and 2016, PostgreSQL, SQLite and MySQL. I checked each generator's output. The existing PostgreSQL Testcontainers suites run the whole migration assembly on a fresh database, so CI runs V3_10 on PostgreSQL. No test runs it on SQL Server.
• NULL tenant ids:
  • SQL Server treats NULLs as equal in a unique index, so it allows one (NULL, Name) row per name. That is the right outcome for legacy default-tenant rows, and the pre-check guarantees existing data passes.
  • SQLite, PostgreSQL and MySQL treat NULLs as distinct, so repeated (NULL, 'admin') rows are still allowed. I confirmed this on SQLite.
  • Dapper writes NULL when no tenant context is set and '' under the default tenant context (Store.cs:552-559). The seeder runs in a tenant context, so its rows are covered.
  • The remark at V3_10.cs:18-20 says "most providers treat NULLs as distinct". Naming SQL Server as the exception would save a maintainer a lookup.
• Case sensitivity: the index follows the database collation. SQL Server's default case-insensitive collation rejects case variants, matching core's OrdinalIgnoreCase check. SQLite and PostgreSQL compare case-sensitively, so the index enforces only exact names there, and a case-variant race still relies on core's pre-save check. Acceptable, but the remarks should say it.
• The pre-check is stricter than the index on purpose. It groups by normalized tenant and ToLowerInvariant name (V3_10.cs:57-88), so case variants and NULL/'' pairs fail the migration even where the index would accept them. That matches core's notion of the same role. The cost: such data blocks startup until an operator fixes it. The error message is actionable. It names the index, the tenant, the names and the ids, and says no rows were changed. Using StringComparer.OrdinalIgnoreCase semantics (for example upper-invariant keys) would match core exactly instead of almost exactly.
• Pre-check SQL: RolesSelectSql (V3_10.cs:108-116) picks quoting by sniffing the connection type name. It works with the raw provider connections FluentMigrator creates, and PostgreSQL is exercised in CI. It is the one clever part of the file, though, and it departs from the repo's documented IfDatabase plus MigrationDatabases pattern (MigrationDatabases.cs:3-13). Two IfDatabase(...) branches would be clearer.
• Transaction: FluentMigrator wraps each migration in a transaction by default, and the read uses it (V3_10.cs:94). A failed pre-check rolls back and does not record version 30005, which VersionApplied(30005) asserts.
• Down() (V3_10.cs:51-55) drops the index only if it exists. Correct.
• Readability: the file is mostly the error message builder. Apart from the type-name sniffing, a maintainer can follow it.
• Runtime: an index violation surfaces as a raw provider exception. That gives a 500 from POST /identity/roles, and the seeder logs a background-task error. The second node in a concurrent seeding race now fails instead of creating a duplicate admin role.

Verified (unchanged from the previous head)

• RoleFilter.Ids maps to the Id column (Dapper/.../Stores/RoleStore.cs:71). Legacy rows where id equals name still resolve.
• The SaveAsync tenant guard (RoleStore.cs:18-38) updates only owned rows, inserts unused ids and refuses foreign ids with an "already exists" InvalidOperationException (409 on core's create endpoint). It cannot re-home another tenant's row, * rows are refused, and NULL legacy rows belong to the default tenant. It adds no dialect-specific SQL.
• RoleFilter.Name deferral is safe: the pinned 3.10.0-preview.5760 has no RoleFilter.Name, and after the bump core's FindByNameAsync re-matches in memory. The bump PR should still add the Name clause, so that a future name-only FindAsync or DeleteAsync cannot match every tenant role.

Non-blocking

1. Put the tenant in the Dapper UPDATE WHERE clause through the filtered UpdateAsync overload (Store.cs:451) for defense in depth.
2. The refusal message says "in another tenant" even when the owner is a * row.
3. Mongo index detection is by name only (CreateIndices.cs:102,113). The "was not found" and "already present" lines log at Information on every startup, so Debug fits better. The new Where rewrite (CreateIndices.cs:143-145) looks the name up twice and reads worse than the loop it replaced. The bot suggestion was optional.
4. Tests:
   • The private TestTenantAccessor now appears in two Dapper test files and one Mongo test file. A shared helper would remove the copies.
   • Path.Combine(GetTempPath(), Path.GetFileName(...)) reads oddly. Path.Join states the intent.
   • There is still no test for a default-tenant update of a NULL legacy row, or for a tenant saving over a * role's id.
5. Release-note items:
   • With the Ids fix, Dapper users whose roles hold a legacy id that differs from the name start receiving that role's permissions.
   • V3_10 fails startup on same-tenant duplicate role names, case variants included, until the rows are fixed.

Residual risk (out of scope, not blocking)

• The Dapper role index is now covered. Concurrent seeding can no longer create two admin roles in one tenant, except for NULL-tenant rows on SQLite, PostgreSQL and MySQL, and case variants on case-sensitive databases.
• Dapper Users still has no name index, so two nodes seeding the same tenant at once can still create two admin users.
• Mongo keeps store-wide unique indexes on User.Name, Application.Name and Application.ClientId, so in a shared store tenant B's admin user still fails with a duplicate key.

Tests and CI

• Locally I ran the five DapperRoleNameUniquenessMigrationTests and the five DapperRoleStoreTests on SQLite, all passing.
• This machine has no Docker, so the PostgreSQL and Mongo Testcontainers suites ran only in CI.
• CI is green at this head:
  • ubuntu-latest: Elsa.Persistence.Dapper.UnitTests 74/74 and Elsa.MongoDb.UnitTests 51/51.
  • CodeQL with both Analyze (csharp) runs, Analyze (actions), submit-nuget, GitGuardian and license/cla.
• No Greptile or CodeRabbit review has been posted. The gate is waived for this repo. Of the five github-code-quality threads, two are resolved and three are outdated but still open.

[sfmskywalker]

Addressed CR R1 (REQUEST_CHANGES @ a592352) on 395b94ded5f26e4c554a4761ae35e375a602769b:

B1. CreateIndices now creates TenantId_1_Name_1 first, then drops Name_1, and treats MongoCommandException code 27 as already dropped. Tests cover the second run, an already-dropped Name_1, and a compound that already exists while Name_1 is still present.

Non-blocking. Owned updates use the filtered UpdateAsync (tenant in the WHERE). The refusal message distinguishes another tenant from a shared * row. Mongo created/already-present/not-found lines are Debug; the single “dropped legacy index” line stays Information. V3_10 remarks name SQL Server as the NULL-equal exception and note that SQLite/Postgres case variants rely on core. Identifier quoting is IfDatabase + MigrationDatabases.QuotedIdentifiers / UnquotedIdentifiers. Dapper TestTenantAccessor is shared; tenantARoles is now rolesNamedAdmin. PR body has the power-user release note.

The Mongo helper stays in MongoRoleStoreTests only — that project does not reference the Dapper test assembly.

[sfmskywalker]

Superseded by elsa-workflows/elsa-core#8616 (merged as 35f577a5), which ports this change, including CR's create-first-then-drop Mongo index fix, into core's src/extensions for 3.10. ext main doesn't publish 3.10, and Sipke decided against a 3.9.x backport, so this PR isn't needed. Thanks for the work here; it was the source for the port.