diff --git a/backend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Infrastructure/Persistence/Migrations/20260903213832_tenant_settings_org_write_guard.cs b/backend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Infrastructure/Persistence/Migrations/20260903213832_tenant_settings_org_write_guard.cs index f8b7d26..dad0b5f 100644 --- a/backend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Infrastructure/Persistence/Migrations/20260903213832_tenant_settings_org_write_guard.cs +++ b/backend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Infrastructure/Persistence/Migrations/20260903213832_tenant_settings_org_write_guard.cs @@ -25,7 +25,7 @@ namespace LearnStack.Modules.Tenancy.Infrastructure.Persistence.Migrations /// /// Intra-tenant rather than cross-tenant — the tenant term is untouched, and no row /// crosses a tenant boundary — so this is a write-scope correction, not an isolation - /// fix. See ADR-0003 Amendment 4 and Database Standards § Tenant-Owned and + /// fix. See ADR-0003 Amendment 5 and Database Standards § Tenant-Owned and /// Organization-Scoped Tables, which carries the corrected template. /// /// diff --git a/backend/tests/LearnStack.Tests.Architecture/AggregateWriteTests.cs b/backend/tests/LearnStack.Tests.Architecture/AggregateWriteTests.cs index 543d722..349980d 100644 --- a/backend/tests/LearnStack.Tests.Architecture/AggregateWriteTests.cs +++ b/backend/tests/LearnStack.Tests.Architecture/AggregateWriteTests.cs @@ -80,7 +80,8 @@ public void Every_Write_Port_Is_Countable_Or_Enumerated() .Where(type => type.IsInterface) .Where(type => type.GetMethods().Any(method => method.GetParameters().Any(parameter => - domainAssemblies.Contains(parameter.ParameterType.Assembly)))) + Unwrap(parameter.ParameterType).Any(inner => + domainAssemblies.Contains(inner.Assembly))))) .Where(type => !WriteStoreConstructions(type).Any()) .Select(type => type.Name) .Distinct() @@ -163,4 +164,39 @@ private static IEnumerable ProductionAssemblies() => .Select(Path.GetFileNameWithoutExtension) .Where(name => !string.IsNullOrEmpty(name)) .Select(name => name!); + + /// A declared type and every type reachable through it. + /// + /// Through wrappers, because a bulk write port takes one: IEnumerable<Course> + /// lives in System.Private.CoreLib, so a check on the parameter's own assembly + /// sees the wrapper and not the domain type inside it — and the port escapes the + /// enumeration while satisfying every word of what this rule claims to detect. + /// Transitive, and through arrays and by-ref, because those nest too: + /// Course[] and in Course are the same port with different syntax. + /// + private static IEnumerable Unwrap(Type declared) + { + yield return declared; + + if (declared.HasElementType && declared.GetElementType() is { } element) + { + foreach (var inner in Unwrap(element)) + { + yield return inner; + } + } + + if (!declared.IsGenericType || declared.IsGenericTypeDefinition) + { + yield break; + } + + foreach (var argument in declared.GetGenericArguments()) + { + foreach (var inner in Unwrap(argument)) + { + yield return inner; + } + } + } } diff --git a/backend/tests/LearnStack.Tests.Integration/Database/TenancySchemaTests.cs b/backend/tests/LearnStack.Tests.Integration/Database/TenancySchemaTests.cs index 7850e13..fd476dc 100644 --- a/backend/tests/LearnStack.Tests.Integration/Database/TenancySchemaTests.cs +++ b/backend/tests/LearnStack.Tests.Integration/Database/TenancySchemaTests.cs @@ -569,7 +569,7 @@ public async Task An_Organization_Scoped_Session_Cannot_Write_A_Tenant_Wide_Row( // // Both AS RESTRICTIVE guards used a bare `organization_id IS NULL` first arm, // which exists so a TENANT-scope session can write those rows — and admitted an - // org-scoped one to them as well. Measured before ADR-0003 Amendment 4: a session + // org-scoped one to them as well. Measured before ADR-0003 Amendment 5: a session // announcing tenant A and organization A1 rewrote tenant A's tenant-wide row. // // The refusal is silent by construction: a RESTRICTIVE USING clause on UPDATE diff --git a/docs/decisions/0003-tenant-isolation-defense-in-depth.md b/docs/decisions/0003-tenant-isolation-defense-in-depth.md index 480a2df..103b010 100644 --- a/docs/decisions/0003-tenant-isolation-defense-in-depth.md +++ b/docs/decisions/0003-tenant-isolation-defense-in-depth.md @@ -4,7 +4,9 @@ Accepted (Amendment 1: 2026-05-18 — adds Organization scope; Amendment 2: 2026-05-19 — identity row terminology; **Amendment 3: 2026-08-08 — corrects the RLS policy template -and adds the database role model**; see bottom of document) +and adds the database role model**; Amendment 4: 2026-08-29 — the Phase 02a table list +has gone stale; **Amendment 5: 2026-09-04 — the write guards exclude an +organization-scoped session from tenant-wide rows**; see bottom of document) ## Decision @@ -301,7 +303,7 @@ The single authority for which class a table belongs to is into three documents drifts in three directions, and this copy already had — which is why the assignment lives in one file and this section keeps only the *rule*. -### Amendment 4 (2026-09-04): the write guards admitted an organization-scoped session to tenant-wide rows +## Amendment 5 — The write guards admitted an organization-scoped session to tenant-wide rows (2026-09-04) **What was wrong.** Amendment 3's template closes the `USING`-only write paths with two `AS RESTRICTIVE` guards, one `FOR UPDATE` and one `FOR DELETE`. Both read: diff --git a/docs/roadmap/phase-02a-kernel-tenancy.md b/docs/roadmap/phase-02a-kernel-tenancy.md index ad8a1ff..b706db7 100644 --- a/docs/roadmap/phase-02a-kernel-tenancy.md +++ b/docs/roadmap/phase-02a-kernel-tenancy.md @@ -2437,9 +2437,11 @@ one — and the second round repeatedly found the first round's fix. > **Packet 7 — Tenant and organization resolution, isolation, two tenants ✅** > -> **Measured at close: 1187 tests green** — 1 contract, 76 architecture, 802 unit, -> 308 integration. Counted from a run, not computed from a plan; an earlier commit -> message in this packet carried an arithmetic total that was eight short. +> **Measured at merge: 1208 tests green** — 1 contract, 79 architecture, 813 unit, +> 315 integration. Counted from a run, not computed from a plan; an earlier commit +> message in this packet carried an arithmetic total that was eight short. The number +> moved after this record was first written, because the pull request's own review found +> more — see § What the pull-request review found. ### What shipped @@ -2506,6 +2508,50 @@ one — and the second round repeatedly found the first round's fix. took a raw `Guid` tenant after the ADR said it and `ITenantContext.TenantId` "both move together". Amendment 4 records the conversion and the divergence. +### What the pull-request review found + +The record above was written at packet close. Fifteen commits landed after it, from three +review rounds on the pull request itself, and the sharpest defect of the packet was among +them — so the record would be a false account without them. + +- **A shipped invariant that did not hold.** Promoting a default locale through the + aggregate raised `23505` every time. `PromoteDefault` clears the incumbent and then sets + the target in memory, and EF does not preserve that order: same-table commands go out in + its comparer's order, and the composite key `(tenant_id, locale)` sorts `en-US` before + `tr-TR` — exactly the seeded pair. Nothing caught it because the cases covering that + index drive raw SQL in an order they choose, so they pin what PostgreSQL does with two + statements rather than what EF emits for one save. **This packet's own PR description + cited that invariant as the migration's safety rationale.** The store now saves in two + passes; a partial unique index permits zero defaults, so the intermediate state is one + the schema allows. +- **A canonical template that admitted the wrong writer.** Both `AS RESTRICTIVE` write + guards began with a bare `organization_id IS NULL` — the arm that lets a *tenant*-scope + session write rows belonging to no organization. It admitted an *organization*-scoped + session to them as well, so one organization could rewrite the tenant-wide fallback + every other organization reads. Measured. Corrected in the template, because every + organization-scoped table is told to copy it, and recorded as + [ADR-0003 Amendment 5](../decisions/0003-tenant-isolation-defense-in-depth.md). +- **Three credential and transaction gaps.** The host resolver announced a session + variable in a transaction four documents call read-only and that was not; the + platform-admin guard accepted `rolsuper`, which bypasses the GRANT matrix that bounds + the role; and an unparseable connection string was echoed through a redaction pattern + that could not cross a `/` or a second `@` inside a password. +- **Two architecture rules with escapes, and one blind test.** The effective-host rule + banned a header the code never reads instead of the one it does; the resolving-host rule + exempted its sole setter by filename suffix; and the host-reclaim case passed with the + hostname uniqueness dropped entirely, because nothing in it asked for a conflict. +- **Three ADR edits made outside the rules that govern them** — a stale statement rewritten + rather than amended, a normative paragraph added to an Accepted body, and an amendment + placed above the decision it amends. +- **Two findings refuted by measurement**, which is why they are worth recording: MediatR + already deduplicates pipeline behaviours, so a guard written for it was removed rather + than shipped; and the query filter's pinned `DbContext` is harmless because its + `CurrentTenantId` delegates to the process-wide accessor. + +**A hook now catches what CI caught three times.** Three commits on the branch tripped the +72-character subject limit, each found after a push — and a subject is only fixable by +rewriting history. `.githooks/commit-msg` enforces exactly what CI's `meta` job enforces. + ### What it did not ship, and who owns each - **Nothing is audited.** `AuditLogBehavior` lights up in Packet 9; `TransactionBehavior` @@ -2518,3 +2564,17 @@ one — and the second round repeatedly found the first round's fix. - **The host-resolution cache is invalidated before the commit, not after.** The guarantee is therefore the request *after* the write, not the one racing it; closing the rest needs a post-commit seam on `IUnitOfWork`, whose surface ADR-0040 governs. +- **Two idempotency limits do not bound memory,** and are recorded at the line rather than + fixed. The store's admission check reads a census refreshed once per sweep interval, so + within that window its caps admit every new key; and the filter buffers a response in + full before applying the 256 KiB cap. Both are Packet 4's, both fixes revisit an + ordering [ADR-0037](../decisions/0037-idempotency-key-contract.md) chose deliberately, + and neither is reachable through an endpoint that exists — every idempotent surface + today answers with an identifier, a receipt or a status. They belong with the durable + store, on that ADR's trigger. +- **Two migrations shipped in one pull request,** which + [Git Standards § Branching](../standards/14-git-workflow.md) says should not happen. + Splitting was attempted and measured infeasible: the migration commit carries the model + change it was generated from, and cherry-picking it onto `main` conflicts through the + marker and typed-identifier commits before it — a "migration PR" would have carried + roughly the first thirty commits of the packet. diff --git a/docs/standards/05-database.md b/docs/standards/05-database.md index 0a4dcff..4c51bbb 100644 --- a/docs/standards/05-database.md +++ b/docs/standards/05-database.md @@ -5,7 +5,8 @@ (Amendments 1 + 2), [ADR-0003 Tenant Isolation Defense in Depth](../decisions/0003-tenant-isolation-defense-in-depth.md) (Amendment 1: Organization Scope; **Amendment 3: corrected RLS policy template and -database role model**), +database role model**; **Amendment 5: the write guards exclude an organization-scoped +session from tenant-wide rows**), [ADR-0006 Events and Outbox](../decisions/0006-events-and-outbox.md) (Amendment 1: Dapr pub/sub dispatch transport), [ADR-0038 Cross-Cutting Port and Event Contracts](../decisions/0038-cross-cutting-port-and-event-contracts.md), diff --git a/docs/standards/21-architecture-tests-catalogue.md b/docs/standards/21-architecture-tests-catalogue.md index 3ff5281..7e92c1f 100644 --- a/docs/standards/21-architecture-tests-catalogue.md +++ b/docs/standards/21-architecture-tests-catalogue.md @@ -1082,6 +1082,81 @@ because the filters hold, and removing both turns all five red. - **Status:** **Implemented** (Packet 7, `LearnStack.Tests.Architecture`). - **Phase:** 02a Packet 7. +#### `Every_Write_Port_Is_Countable_Or_Enumerated` + +- **Asserts:** every interface in a production assembly whose method takes a type from a + module's `Domain` assembly — **directly, or inside a generic, array or by-ref wrapper** + — either derives from `IAggregateWriteStore`, and is therefore visible to + the cross-aggregate census above, or appears on a literal allow-list. The list holds one + name: `IPlatformHostMappingStore`. +- **Wrappers are unwrapped transitively,** because a bulk write port is written with one: + `IEnumerable` lives in `System.Private.CoreLib`, so a check on the parameter's + own assembly sees the wrapper rather than the domain type inside it — and the port would + escape the enumeration while satisfying every word of what this rule claims. +- **Why it exists.** The census counts derivations, so a port that does not derive is + invisible to it. One already is, deliberately: `PlatformHostMapping` is a projection + with a string key rather than an aggregate root. That exemption is fine; being *silent* + about it is not, because a second such port would join the first with nothing to notice, + and the census that keeps ADR-0042's exception at one entry would stop describing the + system. +- **Detected by shape, not by name.** "Takes a domain type" rather than "ends in `Store`": + a rule keyed on a suffix is satisfied by renaming. +- **Source:** [ADR-0042](../decisions/0042-tenant-provisioning-cross-aggregate-transaction.md). +- **Type:** xUnit + reflection. **Kind:** structural. +- **Status:** **Implemented** (Packet 7 review, `LearnStack.Tests.Architecture`, + `AggregateWriteTests`). +- **Phase:** 02a Packet 7. + +#### `Out_Of_Band_Setters_Open_Read_Only_Transactions` + +- **Asserts:** the two components that announce a session variable outside the ambient + unit of work — `CachedHostToTenantResolver` and `OrganizationScopeValidator` — each + contain `SET TRANSACTION READ ONLY`, and contain it **at a lower source offset than + their first `set_config(`**. +- **What the offset comparison does and does not prove.** PostgreSQL refuses + `SET TRANSACTION` after the transaction's *first statement of any kind*, and this scan + only orders it against the announcement. A setter that ran some other statement — a + `SELECT`, a second `SET` — between `BEGIN` and `SET TRANSACTION READ ONLY` would satisfy + the rule and fail at runtime. That failure is loud and immediate rather than silent, + which is why the cheap ordering check is the one that ships; the expensive alternative + is parsing the method for every command execution, and + [§ What a structural test proves](#what-a-structural-test-proves--and-what-it-does-not) + states the general limit. +- **Why the property matters at all.** Read-only is what makes an out-of-band setter of a + session variable acceptable, because `learnstack_app` holds write grants on the tables + these connections reach — so nothing but this statement stops a future edit from writing + under an announcement no request made. +- **Why a scan and not a behavioural test.** The transaction is opened, used and disposed + inside one method, so nothing outside can observe its settings. Measured: the resolver + shipped without the statement while four carriers — Database Standards, Security + Standards, the glossary and ADR-0040 — described it as read-only, and the validator two + files away had carried it since Packet 6. +- **Source:** [ADR-0040](../decisions/0040-ambient-unit-of-work.md); + [05-database.md](05-database.md); [11-security.md](11-security.md). +- **Type:** xUnit + source scan. **Kind:** structural. +- **Status:** **Implemented** (Packet 7 review, `LearnStack.Tests.Architecture`, + `TenancyConventionTests`). +- **Phase:** 02a Packet 7. + +#### `Registering_The_Pipeline_Twice_Registers_It_Once` + +- **Asserts:** calling `AddLearnStackMediatRPipeline` twice on one `ServiceCollection` + yields the same registrations as calling it once — the same behaviour count and the same + total. +- **The property is MediatR's, not ours.** `AddBehavior` deduplicates; measured at seven + behaviours and eleven registrations either way. It is pinned because the repository + depends on it and did not write it: every test fixture registers its probe handler by + hand specifically to avoid re-running `AddMediatR`, and if deduplication stopped + holding, that workaround would become load-bearing rather than cautious with nothing to + say so. A doubled `TransactionBehavior` is a nested frame on every request. +- **A guard of our own was written and removed.** It changed nothing under mutation, and a + guard no test can kill is a comment. +- **Source:** [ADR-0032 § Sub-decision 2](../decisions/0032-exception-handling-logging-and-observability.md). +- **Type:** xUnit + DI registration inspection. **Kind:** structural. +- **Status:** **Implemented** (Packet 7 review, `LearnStack.Tests.Architecture`, + `CrossCuttingFoundationTests`). +- **Phase:** 02a Packet 7. + #### `Tenant_Context_Guard_Fires_Only_On_An_Unmarked_Transaction` - **Asserts:** both arms of the `DbCommandInterceptor` guard. A command a module `DbContext` issues on a transaction no sanctioned setter announced throws `TenantContextMissingException`; the same command on an announced transaction runs. One arm is not the rule: a guard keyed on `TransactionBehavior` instead of on the marker passes the first arm and rejects the writes the idempotency store and the audit store legitimately make on their own short transactions.