diff --git a/AGENTS.md b/AGENTS.md index 28b8954a8..b24310477 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -53,7 +53,7 @@ or adding an aggregate state. ```bash dotnet build Cluckwork.sln # warnings are errors — keep it clean -dotnet test Cluckwork.sln # 2278 tests as of 2026-09; integration needs Docker +dotnet test Cluckwork.sln # 2295 tests as of 2026-09; integration needs Docker ``` - **Integration tests** spin up a real Postgres via Testcontainers (`docker` required). No SQLite — EF SQL semantics differ. @@ -75,7 +75,7 @@ dotnet test Cluckwork.sln # 2278 tests as of 2026-09; integrati ### Data and correctness - **Every aggregate mutation must bump `Version`.** `Version` is an EF concurrency token (`IsConcurrencyToken`): EF puts the *original* value in the UPDATE's `WHERE` but never auto-increments it, so a mutation without `Version++` silently loses concurrent races — both writers match `WHERE Version = N` — instead of 409ing. This shipped three times; each fix carries a parallel-race integration test, and so must any new mutation. -- **Multi-tenancy:** every tenant-owned entity has `AccountId`, enforced by an EF **global query filter** plus a **`TenantStampInterceptor`** (stamps on insert, and rejects a mismatched write on update/delete). `TenantContext` resolves per-request from the JWT `account_id` claim and is **single-assignment** — a differing re-resolve throws; at startup it is unresolved, so seeders use `IgnoreQueryFilters()`. Several farms now coexist on one deployment: sign-in takes a farm code, and one email address can belong to a user in more than one farm. **`AccountId` is also an EF concurrency token on every entity that carries one (#562):** the `UPDATE`/`DELETE` the database runs carries `AccountId = `, so a detached stub aimed at another farm's row matches nothing — the interceptor alone cannot see a detached write, and an owned-only edit on an attached stub was writing through until the token landed. The token comes from a model walk in `AppDbContext.OnModelCreating`, so a new entity is covered automatically; never remove it, and a database refusal under a resolved tenant is logged as `Tenant.WriteRefusedByDatabase`. **Identity's `AspNetUserRoles` carries a shadow `AccountId` plus a composite foreign key to `AspNetUsers(Id, AccountId)` (#670):** both layers select by property NAME, so the shadow column is stamped, verified and tokened like any other, and the FK makes a role grant to another farm's user — or under no resolved tenant — a database refusal; the other four user-keyed Identity tables have no writer in `src/` and are recorded as accepted risk. → [`530-multi-farm-tenancy.md`](docs/decisions/530-multi-farm-tenancy.md) +- **Multi-tenancy:** every tenant-owned entity has `AccountId`, enforced by an EF **global query filter** plus a **`TenantStampInterceptor`** (stamps on insert, and rejects a mismatched write on update/delete). `TenantContext` resolves per-request from the JWT `account_id` claim and is **single-assignment** — a differing re-resolve throws; at startup it is unresolved, so seeders use `IgnoreQueryFilters()`. Several farms now coexist on one deployment: sign-in takes a farm code, and one email address can belong to a user in more than one farm. **`AccountId` is also an EF concurrency token on every entity that carries one (#562):** the `UPDATE`/`DELETE` the database runs carries `AccountId = `, so a detached stub aimed at another farm's row matches nothing — the interceptor alone cannot see a detached write, and an owned-only edit on an attached stub was writing through until the token landed. The token comes from a model walk in `AppDbContext.OnModelCreating`, so a new entity is covered automatically; never remove it, and a database refusal under a resolved tenant is logged as `Tenant.WriteRefusedByDatabase`. **`AccountId` must be a non-nullable `Guid`, and both layers now fail closed on anything else (#673):** the walk throws `TenantAccountIdShapeException` at model build for any other CLR type and the interceptor throws it at `SaveChanges` for a mapped type that is not `Guid` or a value that does not box to one — before, a `Guid?` or a strongly-typed id got no token and no check, which is #562's detached-write hole reopened by one mapping with every test green. Map a new tenant-owned entity's `AccountId` as a plain `Guid`; a strongly-typed id there needs this decision reopened, not a cast. **Identity's `AspNetUserRoles` carries a shadow `AccountId` plus a composite foreign key to `AspNetUsers(Id, AccountId)` (#670):** both layers select by property NAME, so the shadow column is stamped, verified and tokened like any other, and the FK makes a role grant to another farm's user — or under no resolved tenant — a database refusal; the other four user-keyed Identity tables have no writer in `src/` and are recorded as accepted risk. → [`530-multi-farm-tenancy.md`](docs/decisions/530-multi-farm-tenancy.md) - **Flock-scoped EF reads are discovered from the model (#613).** Every mapped `Flock` or entity with a scalar `FlockId` must keep one structural `AccountId AND flock-scope` query filter; `UserRoleAssignment` is the sole deliberate exclusion because those rows resolve the scope itself. Two no-`FlockId` children rely on named parent/policy gates: daily-entry list/detail loads `DailyEntryGrade` only through a filtered `DailyEntry` aggregate and Worker-readable production totals correlate it through that same filtered parent; the lot movement ledger gates `EggInventoryMovement` through filtered `EggLot`; both children's direct exports remain `AdminOnly`. Any new exclusion or parent-derived child needs an explicit rationale and a causal mutation test; do not replace the model walk with a recalled entity list or expression `ToString()` checks. - **Transient-DB retry stops at unreplayable work (#269).** `EnableRetryOnFailure` covers self-contained EF units only; an automatic replay above a stateful detector (a counter, a CAS stamp, a single-use claim) cannot tell "this request racing itself" from the signal the detector exists to catch. Two cures, and picking wrong ships the bug: `SingleAttemptExecution` when the replay is itself observable, a durability probe on a self-minted token when the replay writes nothing. → [`269-transient-db-retry-boundary.md`](docs/decisions/269-transient-db-retry-boundary.md) - **`AuditEvents` is not time-partitioned, on purpose (#505).** The dominant read filters on `AccountId`+`EntityType`+`EntityId` with no date predicate, so monthly partitions would turn one index lookup into one per partition for no pruning benefit. If it is ever needed, partition by `AccountId`. → [`505-audit-events-no-time-partition.md`](docs/decisions/505-audit-events-no-time-partition.md) diff --git a/docs/decisions/530-multi-farm-tenancy.md b/docs/decisions/530-multi-farm-tenancy.md index 137ca24a0..8a11cf15d 100644 --- a/docs/decisions/530-multi-farm-tenancy.md +++ b/docs/decisions/530-multi-farm-tenancy.md @@ -208,6 +208,29 @@ and `RoleManager.DeleteAsync`, which deletes a *global* role and, through change tracker never holding an `IdentityUserRole` row; no caller exists, and one would be a farm-wide operation by construction, never a per-farm one. +**Both layers now refuse an `AccountId` that is not a `Guid` (#673, 2026-09-05).** Latent, not +live: every mapped `AccountId` is a non-nullable `Guid` today. But both layers *keyed* on that +without requiring it — the #562 walk skipped any property whose `ClrType != typeof(Guid)` (so a +`Guid?` or a strongly-typed id got no token) and `TenantStampInterceptor` skipped any value that +did not box to a `Guid` (so a null `Guid?` was inserted unstamped and never checked on +update/delete). One future entity mapped `public Guid? AccountId` therefore reopened #562's +detached-write hole with every test green. The walk now **throws +`TenantAccountIdShapeException` at model build** for any other CLR type — checked before the +primary-key exclusion, so a key `AccountId` must be a `Guid` too, and that ordering is pinned by +its own probe rather than left to a comment — which fails every boot and every test rather than +degrading silently; the interceptor throws the same exception at +`SaveChanges`, under a resolved tenant, on Added, Modified and Deleted alike — for a **mapped +type** that is not `Guid` as well as for a value that is not a `Guid`, because a non-null `Guid?` +boxes to `Guid` and the value check alone would accept it while the row carries no token to +compare (found by the deep review round on #695, and the reason the interceptor holds without the +walk rather than beside it). The two throws are independent on purpose: the interceptor's case is proved +against a context that never runs the walk. `AccountIdConcurrencyTokenModelTests`' by-name +selector stays as the second net for the whole set at once, and +`AccountIdShapeFailClosedTests` pins both refusals plus the must-still-pass control that a plain +`Guid` still gets its token. **What this does not change:** an unresolved tenant still disables +the interceptor entirely (the CLI verbs and the design-time factory depend on it), so the write +guard's floor is unchanged there. + --- ## 6. Suspension is immediate for use diff --git a/src/Cluckwork.Infrastructure/Persistence/AppDbContext.cs b/src/Cluckwork.Infrastructure/Persistence/AppDbContext.cs index e91dcf991..9a4b04b43 100644 --- a/src/Cluckwork.Infrastructure/Persistence/AppDbContext.cs +++ b/src/Cluckwork.Infrastructure/Persistence/AppDbContext.cs @@ -249,10 +249,28 @@ protected override void OnModelCreating(ModelBuilder builder) // The snapshot records the flag but EF emits no schema for it, so the // migration that accompanies this walk (AccountIdConcurrencyToken) is // deliberately empty — it exists to keep the snapshot equal to the model. + ApplyAccountIdTenantTokens(builder); + } + + // Extracted so #673's fail-closed case can be proved against a throwaway + // entity: this walk is the only thing a probe context needs, and a mapping + // it must refuse cannot be added to AppDbContext's own model by definition. + internal static void ApplyAccountIdTenantTokens(ModelBuilder builder) + { foreach (var entityType in builder.Model.GetEntityTypes()) { var accountId = entityType.FindProperty(nameof(Entity.AccountId)); - if (accountId is null || accountId.ClrType != typeof(Guid)) continue; + if (accountId is null) continue; + + // #673 — fail CLOSED on any other CLR type instead of walking past + // it. A Guid? or a converted AccountId used to get no token here and + // no check in the interceptor, which is the #562 hole reopened with + // every test green. Throwing at model build makes it fail every boot + // and every test on the day it is mapped. Checked before the + // primary-key exclusion below, so a key AccountId must be a Guid too. + if (accountId.ClrType != typeof(Guid)) + throw TenantAccountIdShapeException.ForModel(entityType.DisplayName(), accountId.ClrType); + if (accountId.IsPrimaryKey()) continue; accountId.IsConcurrencyToken = true; } diff --git a/src/Cluckwork.Infrastructure/Persistence/Interceptors/TenantStampInterceptor.cs b/src/Cluckwork.Infrastructure/Persistence/Interceptors/TenantStampInterceptor.cs index 889315657..3d81ebbd8 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Interceptors/TenantStampInterceptor.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Interceptors/TenantStampInterceptor.cs @@ -113,6 +113,17 @@ private void StampTenant(DbContext? context) .FirstOrDefault(p => p.Metadata.Name == nameof(Entity.AccountId)); if (prop is null) continue; + // #673 review round — the SHAPE is checked here, not just the value + // below. A non-null Guid? boxes to Guid, so the value checks alone + // accept a nullable AccountId whose row carries no #562 concurrency + // token: the value looks right and the database is never told to + // compare it. AppDbContext's walk refuses that mapping at model + // build, but this layer is meant to hold on its own — a context + // configured without the walk is exactly the case it exists for. + if (prop.Metadata.ClrType != typeof(Guid)) + throw TenantAccountIdShapeException.ForMapping( + entry.Metadata.ClrType.Name, entry.State.ToString(), prop.Metadata.ClrType); + switch (entry.State) { case EntityState.Added: @@ -149,7 +160,12 @@ private void StampTenant(DbContext? context) private void StampOrVerifyAdded(EntityEntry entry, PropertyEntry prop) { - if (prop.CurrentValue is not Guid accountId) return; + // #673 — anything that is not a Guid is unCHECKABLE, not exempt. This + // used to return, so a null Guid? was inserted unstamped and every + // later write to that row went unverified. + if (prop.CurrentValue is not Guid accountId) + throw TenantAccountIdShapeException.ForWrite( + entry.Metadata.ClrType.Name, nameof(EntityState.Added), prop.CurrentValue); if (accountId == Guid.Empty) { @@ -164,7 +180,12 @@ private void StampOrVerifyAdded(EntityEntry entry, PropertyEntry prop) private void Verify(EntityEntry entry, object? value) { - if (value is not Guid accountId) return; + // #673 — same fail-closed rule as StampOrVerifyAdded: a value that is + // not a Guid cannot be compared to the tenant, so refuse the write + // rather than let it past unchecked. + if (value is not Guid accountId) + throw TenantAccountIdShapeException.ForWrite( + entry.Metadata.ClrType.Name, entry.State.ToString(), value); if (accountId != tenant.AccountId) throw new TenantWriteMismatchException( diff --git a/src/Cluckwork.Infrastructure/Persistence/TenantAccountIdShapeException.cs b/src/Cluckwork.Infrastructure/Persistence/TenantAccountIdShapeException.cs new file mode 100644 index 000000000..bf053071e --- /dev/null +++ b/src/Cluckwork.Infrastructure/Persistence/TenantAccountIdShapeException.cs @@ -0,0 +1,40 @@ +namespace Cluckwork.Infrastructure.Persistence; + +// #673 — both write-side tenant layers key on AccountId being a non-nullable +// Guid, and both used to fall THROUGH for any other shape: the model walk in +// AppDbContext.OnModelCreating gave a Guid? or a converted AccountId no +// concurrency token, and TenantStampInterceptor skipped any value that did not +// box to a Guid. A tenant-owned entity mapped with `Guid? AccountId` therefore +// reopened the #562 hole — a detached Update/Remove naming another farm's row +// wrote through, with every test green. +// +// Both layers now throw this instead of continuing: at model build for the +// wrong CLR type (so a mis-typed AccountId fails every boot and every test), +// and at SaveChanges for a value that is not a Guid. Distinct from +// TenantWriteMismatchException, which means a real cross-tenant VALUE; this one +// means the mapping itself cannot be checked. Caught nowhere, deliberately. +public sealed class TenantAccountIdShapeException : InvalidOperationException +{ + public string EntityType { get; } + + private TenantAccountIdShapeException(string message, string entityType) + : base(message) => EntityType = entityType; + + public static TenantAccountIdShapeException ForModel(string entityType, Type clrType) => + new($"{entityType}.AccountId is mapped as {clrType.Name}, but tenant isolation requires a " + + "non-nullable Guid: the concurrency-token walk and the write guard can only check that " + + "shape, so any other one is a silent cross-tenant write (#673).", + entityType); + + public static TenantAccountIdShapeException ForMapping(string entityType, string state, Type clrType) => + new($"Refusing to save {state} {entityType}: its AccountId is mapped as {clrType.Name} rather than " + + "a non-nullable Guid, so the value this guard sees can be right while the row carries no " + + "concurrency token to compare it against (#673).", + entityType); + + public static TenantAccountIdShapeException ForWrite(string entityType, string state, object? value) => + new($"Refusing to save {state} {entityType}: its AccountId is " + + $"{(value is null ? "null" : $"a {value.GetType().Name}")} rather than a Guid, so the write " + + "guard cannot check it against the resolved tenant (#673).", + entityType); +} diff --git a/tests/Cluckwork.Api.IntegrationTests/AccountIdConcurrencyTokenModelTests.cs b/tests/Cluckwork.Api.IntegrationTests/AccountIdConcurrencyTokenModelTests.cs index 9e6c7c651..d66db460f 100644 --- a/tests/Cluckwork.Api.IntegrationTests/AccountIdConcurrencyTokenModelTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/AccountIdConcurrencyTokenModelTests.cs @@ -29,15 +29,15 @@ public void EveryNonKeyAccountId_IsAConcurrencyToken() { using var db = BuildContext(); - // Selected by NAME and non-key only — deliberately NOT by CLR type. - // The token walk skips any AccountId whose CLR type is not exactly Guid - // (so a Guid? or a converted id gets no token at all), and the - // interceptor skips any VALUE that does not box to a Guid (a Guid? that - // is null is neither stamped on Added nor checked on Modified/Deleted; - // a populated Guid? boxes to Guid and is checked). If this selector - // mirrored the walk's own predicate, such a property would vanish from - // the guard with every test green; naming the property instead makes it - // red the day one is mapped (#673 tracks closing that by construction). + // Selected by NAME and non-key only — deliberately NOT by CLR type. A + // selector mirroring the walk's own predicate would let a Guid? or a + // converted AccountId vanish from this guard with every test green. + // + // Since #673 the walk itself THROWS on any CLR type other than Guid, so + // BuildContext above would already have failed and this assertion is the + // second net rather than the first — kept because it names the whole + // set at once, and because the day the walk's throw is weakened is the + // day this has to be the one that reddens. var carriers = db.Model.GetEntityTypes() .Select(t => (Type: t, AccountId: t.FindProperty("AccountId"))) .Where(x => x.AccountId is not null && !x.AccountId.IsPrimaryKey()) diff --git a/tests/Cluckwork.Api.IntegrationTests/AccountIdShapeFailClosedTests.cs b/tests/Cluckwork.Api.IntegrationTests/AccountIdShapeFailClosedTests.cs new file mode 100644 index 000000000..9e5d0e543 --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/AccountIdShapeFailClosedTests.cs @@ -0,0 +1,263 @@ +namespace Cluckwork.Api.IntegrationTests; + +using Cluckwork.Application.Common; +using Cluckwork.Infrastructure.Persistence; +using Cluckwork.Infrastructure.Persistence.Interceptors; +using Microsoft.EntityFrameworkCore; + +// #673 — tenant isolation's two write-side layers both key on AccountId being a +// non-nullable Guid, and both used to walk PAST any other shape: no concurrency +// token from the #562 model walk, no check in TenantStampInterceptor. That is +// the #562 hole reopened by a single mapping, with every test green. +// +// These tests prove each layer now refuses that shape independently, against a +// throwaway entity: the mapping under test cannot exist in AppDbContext's own +// model, because the walk it must fail is the one AppDbContext runs. No +// database — both refusals happen before any SQL is sent. +public sealed class AccountIdShapeFailClosedTests +{ + private const string ModelOnly = "Host=localhost;Database=model-only;Username=none;Password=none"; + + private sealed class NullableAccountIdEntity + { + public Guid Id { get; set; } + public Guid? AccountId { get; set; } + } + + private sealed class ConvertedAccountIdEntity + { + public Guid Id { get; set; } + public AccountKey AccountId { get; set; } + } + + private sealed class PlainAccountIdEntity + { + public Guid Id { get; set; } + public Guid AccountId { get; set; } + } + + // Keyed BY the converted AccountId, so the walk's shape check has to run + // before its primary-key exclusion to catch it (#695 review). + private sealed class ConvertedKeyEntity + { + public AccountKey AccountId { get; set; } + } + + private readonly record struct AccountKey(Guid Value); + + // One probe context PER SHAPE, and that is load-bearing: EF caches the + // built model by context TYPE, so a single parameterised probe would hand + // every test whichever model happened to be built first — two of these + // tests passed vacuously that way while being written. + private sealed class NullableProbeContext(DbContextOptions options) + : DbContext(options) + { + protected override void OnModelCreating(ModelBuilder builder) + { + builder.Entity(); + AppDbContext.ApplyAccountIdTenantTokens(builder); + } + } + + private sealed class ConvertedProbeContext(DbContextOptions options) + : DbContext(options) + { + protected override void OnModelCreating(ModelBuilder builder) + { + builder.Entity() + .Property(e => e.AccountId) + .HasConversion(key => key.Value, value => new AccountKey(value)); + AppDbContext.ApplyAccountIdTenantTokens(builder); + } + } + + private sealed class ConvertedKeyProbeContext(DbContextOptions options) + : DbContext(options) + { + protected override void OnModelCreating(ModelBuilder builder) + { + var entity = builder.Entity(); + entity.HasKey(e => e.AccountId); + entity.Property(e => e.AccountId).HasConversion(key => key.Value, value => new AccountKey(value)); + AppDbContext.ApplyAccountIdTenantTokens(builder); + } + } + + private sealed class PlainProbeContext(DbContextOptions options) + : DbContext(options) + { + protected override void OnModelCreating(ModelBuilder builder) + { + builder.Entity(); + AppDbContext.ApplyAccountIdTenantTokens(builder); + } + } + + // Maps `Guid? AccountId` WITHOUT the walk, which is the point: the + // interceptor has to refuse the write on its own, not lean on a model that + // would have failed to build. + private sealed class WriteProbeContext(DbContextOptions options) : DbContext(options) + { + protected override void OnModelCreating(ModelBuilder builder) => + builder.Entity(); + } + + private static TContext Probe() where TContext : DbContext => + (TContext)Activator.CreateInstance( + typeof(TContext), + new DbContextOptionsBuilder().UseNpgsql(ModelOnly).Options)!; + + private static WriteProbeContext Writes(out TenantContext tenant) + { + var resolved = new TenantContext(); + resolved.Resolve(Guid.NewGuid()); + tenant = resolved; + return new WriteProbeContext(new DbContextOptionsBuilder() + .UseNpgsql(ModelOnly) + .AddInterceptors(new TenantStampInterceptor(resolved)) + .Options); + } + + [Fact] + public void ModelWalk_RefusesANullableAccountId() + { + using var db = Probe(); + + var ex = Assert.Throws(() => _ = db.Model); + + Assert.Equal(nameof(NullableAccountIdEntity), ex.EntityType); + Assert.Contains("non-nullable Guid", ex.Message, StringComparison.Ordinal); + } + + // A strongly-typed id is the other way the same hole opens: the property's + // CLR type is the wrapper, so the walk's `!= typeof(Guid)` skipped it even + // though the column is a uuid. + [Fact] + public void ModelWalk_RefusesAConvertedAccountId() + { + using var db = Probe(); + + var ex = Assert.Throws(() => _ = db.Model); + + Assert.Equal(nameof(ConvertedAccountIdEntity), ex.EntityType); + } + + // Must-still-pass control: the walk keeps doing its #562 job for the shape + // it accepts. Without this, deleting the whole walk would still pass the two + // refusal tests above — they would just fail for a different reason. + [Fact] + public void ModelWalk_StillTokensAPlainGuidAccountId() + { + using var db = Probe(); + + var accountId = db.Model.FindEntityType(typeof(PlainAccountIdEntity))!.FindProperty("AccountId")!; + + Assert.Equal(typeof(Guid), accountId.ClrType); + Assert.True(accountId.IsConcurrencyToken); + } + + // The insert that used to slip through unstamped: a null AccountId is not a + // Guid, so the old `is not Guid ... return` left the row with no tenant at + // all — and every later write to it unverified for the same reason. + [Fact] + public void Interceptor_RefusesAnUnstampableInsert() + { + using var db = Writes(out _); + db.Add(new NullableAccountIdEntity { Id = Guid.NewGuid(), AccountId = null }); + + var ex = Assert.Throws(() => db.SaveChanges()); + + Assert.Equal(nameof(NullableAccountIdEntity), ex.EntityType); + Assert.Contains("Added", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void Interceptor_RefusesAnUncheckableUpdate() + { + using var db = Writes(out _); + var stub = new NullableAccountIdEntity { Id = Guid.NewGuid(), AccountId = null }; + db.Attach(stub); + db.Entry(stub).State = EntityState.Modified; + + var ex = Assert.Throws(() => db.SaveChanges()); + + Assert.Contains("Modified", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void Interceptor_RefusesAnUncheckableDelete() + { + using var db = Writes(out _); + var stub = new NullableAccountIdEntity { Id = Guid.NewGuid(), AccountId = null }; + db.Attach(stub); + db.Remove(stub); + + var ex = Assert.Throws(() => db.SaveChanges()); + + Assert.Contains("Deleted", ex.Message, StringComparison.Ordinal); + } + + // The refusal is scoped to a resolved tenant exactly as every other check in + // the interceptor is: the CLI verbs, the seeders' pre-checks and + // AppDbContextDesignTimeFactory all save with no tenant by design. + [Fact] + public void Interceptor_LeavesAnUnresolvedTenantAlone() + { + var unresolved = new TenantContext(); + using var db = new WriteProbeContext(new DbContextOptionsBuilder() + .UseNpgsql(ModelOnly) + .AddInterceptors(new TenantStampInterceptor(unresolved)) + .Options); + db.Add(new NullableAccountIdEntity { Id = Guid.NewGuid(), AccountId = null }); + + // No TenantAccountIdShapeException: the save gets as far as the database + // and fails there, on the connection this test deliberately cannot open. + Assert.IsNotType(Record.Exception(() => db.SaveChanges())); + } + + // The walk checks the CLR type BEFORE excluding a primary-key AccountId, and + // this is what holds that ordering: with the two swapped, a key AccountId of + // the wrong shape would be skipped as a key and never type-checked. The + // other converted probe cannot see it — its key is a separate Id (#695 + // review). + [Fact] + public void ModelWalk_RefusesAConvertedAccountIdEvenAsThePrimaryKey() + { + using var db = Probe(); + + var ex = Assert.Throws(() => _ = db.Model); + + Assert.Equal(nameof(ConvertedKeyEntity), ex.EntityType); + } + + // The value checks alone are NOT enough, and this is why: a non-null Guid? + // boxes to Guid, so `value is not Guid` accepts it and the write passes with + // the tenant's own id in a column the model gave no #562 token. The + // interceptor must refuse the SHAPE, not only a null (#695 review). + [Theory] + [InlineData(EntityState.Added)] + [InlineData(EntityState.Modified)] + [InlineData(EntityState.Deleted)] + public void Interceptor_RefusesAPopulatedNullableAccountId(EntityState state) + { + using var db = Writes(out var tenant); + + // The tenant's OWN id: the only thing wrong here is the mapped type, so + // a guard that only compared values would let this through. + var stub = new NullableAccountIdEntity { Id = Guid.NewGuid(), AccountId = tenant.AccountId }; + if (state == EntityState.Added) + { + db.Add(stub); + } + else + { + db.Attach(stub); + db.Entry(stub).State = state; + } + + var ex = Assert.Throws(() => db.SaveChanges()); + + Assert.Equal(nameof(NullableAccountIdEntity), ex.EntityType); + Assert.Contains("Nullable", ex.Message, StringComparison.Ordinal); + } +}