feat(tenancy): write-side tenant guard + single-assignment TenantContext (#546)

src/Cluckwork.Infrastructure/Persistence/Interceptors/TenantStampInterceptor.cs

@@ -2,10 +2,27 @@ namespace Cluckwork.Infrastructure.Persistence.Interceptors;
 
 using Cluckwork.Domain.Common;
 using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.ChangeTracking;
 using Microsoft.EntityFrameworkCore.Diagnostics;
 
 // Stamps AccountId on every newly inserted entity so writes can't be mis-tagged
-// even if a handler forgets to pass it (tech spec §4.2, point 3).
+// even if a handler forgets to pass it (tech spec §4.2, point 3), AND refuses
+// any tracked write whose AccountId is not the resolved tenant's (#546).
+//
+// Before #546 this only FILLED an empty AccountId on Added entities: an
+// explicitly WRONG non-empty value was written without complaint, and Modified
+// and Deleted were never inspected at all. Reads have 27 fail-closed query
+// filters; writes had convention. This is the write side's chokepoint.
+//
+// Matching is by property NAME rather than by base type, and that is
+// load-bearing: RefreshToken, IdempotencyRecord and SimulationSeedState all
+// carry AccountId WITHOUT inheriting Entity<TId>, so a type-based test would
+// silently drop them out of scope — including RefreshToken, which is exactly
+// the cross-tenant write /auth/login can attempt once #532 lands.
+//
+// An unresolved tenant disables checking entirely, deliberately: the seeders,
+// the one-shot CLI verbs and AppDbContextDesignTimeFactory all run that way by
+// design.
 public sealed class TenantStampInterceptor(TenantContext tenant) : SaveChangesInterceptor
 {
     public override ValueTask<InterceptionResult<int>> SavingChangesAsync(
@@ -29,15 +46,79 @@ private void StampTenant(DbContext? context)
     {
         if (context is null || !tenant.IsResolved) return;
 
+        // Only the three states EF actually emits SQL for. Unchanged and
+        // Detached write nothing, so inspecting them would reject writes that
+        // never happen.
         foreach (var entry in context.ChangeTracker.Entries()
-            .Where(e => e.State == EntityState.Added))
+            .Where(e => e.State is EntityState.Added or EntityState.Modified or EntityState.Deleted))
         {
-            // Stamp any Entity<TId> subclass that has an AccountId property.
+            // Any entity carrying an AccountId property, Entity<TId> or not.
             var prop = entry.Properties
                 .FirstOrDefault(p => p.Metadata.Name == nameof(Entity<Guid>.AccountId));
+            if (prop is null) continue;
 
-            if (prop is not null && prop.CurrentValue is Guid g && g == Guid.Empty)
-                prop.CurrentValue = tenant.AccountId;
+            switch (entry.State)
+            {
+                case EntityState.Added:
+                    StampOrVerifyAdded(entry, prop);
+                    break;
+
+                // BOTH the value being written and the value the row was loaded
+                // with must be the tenant's. Checking only the current value
+                // would let a row loaded under IgnoreQueryFilters be RELABELLED
+                // into the current tenant and pass — theft, not a leak.
+                //
+                // PRECONDITION, stated because it is easy to over-read this
+                // (#561 review): OriginalValue is database provenance only for
+                // an entity that was LOADED while tracked. DbSet.Update and
+                // DbSet.Remove ATTACH a detached instance and seed its original
+                // values from the caller's own current values, so a hand-built
+                // stub carrying another tenant's primary key and this tenant's
+                // AccountId would satisfy both checks and the UPDATE would key
+                // on the primary key alone.
+                //
+                // That is not reachable today: every repository mutation read
+                // (GetByIdAsync / GetTrackedAsync / GetByIdLockedAsync) is a
+                // TRACKED read behind the tenant query filter, so the snapshot
+                // really is the database's — and TrackedMutationReadTests pins
+                // exactly that, so switching one of them to AsNoTracking goes
+                // red here rather than silently voiding the theft check.
+                // Closing it by construction is #562.
+                case EntityState.Modified:
+                    Verify(entry, prop.OriginalValue);
+                    Verify(entry, prop.CurrentValue);
+                    break;
+
+                // A delete writes no new value, so the loaded one is all there
+                // is to check.
+                case EntityState.Deleted:
+                    Verify(entry, prop.OriginalValue);
+                    break;
+            }
         }
     }
+
+    private void StampOrVerifyAdded(EntityEntry entry, PropertyEntry prop)
+    {
+        if (prop.CurrentValue is not Guid accountId) return;
+
+        if (accountId == Guid.Empty)
+        {
+            prop.CurrentValue = tenant.AccountId;
+            return;
+        }
+
+        if (accountId != tenant.AccountId)
+            throw new TenantWriteMismatchException(
+                entry.Metadata.ClrType.Name, nameof(EntityState.Added), tenant.AccountId, accountId);
+    }
+
+    private void Verify(EntityEntry entry, object? value)
+    {
+        if (value is not Guid accountId) return;
+
+        if (accountId != tenant.AccountId)
+            throw new TenantWriteMismatchException(
+                entry.Metadata.ClrType.Name, entry.State.ToString(), tenant.AccountId, accountId);
+    }
 }

src/Cluckwork.Infrastructure/Persistence/SimulationDataSeeder.cs

@@ -37,6 +37,7 @@ namespace Cluckwork.Infrastructure.Persistence;
 using Cluckwork.Infrastructure.Jobs;
 using Microsoft.AspNetCore.Identity;
 using Microsoft.EntityFrameworkCore;
+using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 
@@ -114,6 +115,7 @@ public sealed class SimulationDataSeeder(
     CreateExpenseHandler createExpense,
     DailyEntryLockSweep lockSweep,
     IClock clock,
+    IServiceScopeFactory scopeFactory,
     IOptions<SimulationOptions> simulationOptions,
     ILogger<SimulationDataSeeder> logger)
 {
@@ -1504,8 +1506,25 @@ private async Task SeedSecondAccountAsync(CancellationToken ct)
         var exists = await db.Accounts.IgnoreQueryFilters().AnyAsync(a => a.Id == SecondAccountId, ct);
         if (exists) return;
 
-        db.Accounts.Add(Account.Create(SecondAccountId, "Simulation Second Farm", "simulation-second-farm", "UTC", "USD"));
-        await db.SaveChangesAsync(ct);
+        // #546 — this row belongs to the SECOND account while this seeder's own
+        // scope is resolved to the PRIMARY one (see SeedAsync), so writing it
+        // through `db` is a cross-tenant write and TenantStampInterceptor now
+        // refuses it. Give the write its own scope resolved to the account it
+        // actually writes, mirroring DailyEntryLockSweep.LockDueEntriesAsync.
+        //
+        // BOTH the TenantContext and the AppDbContext must come from the new
+        // scope. The interceptor is bound to the scope that constructed its
+        // DbContext, so resolving the tenant here and still writing through the
+        // outer `db` would leave the guard reading the PRIMARY account — a
+        // no-op that looks like a fix. Resolving the injected `tenant` field
+        // instead would throw: it is single-assignment as of this same slice.
+        using var scope = scopeFactory.CreateScope();
+        scope.ServiceProvider.GetRequiredService<TenantContext>().Resolve(SecondAccountId);
+        var secondAccountDb = scope.ServiceProvider.GetRequiredService<AppDbContext>();
+
+        secondAccountDb.Accounts.Add(
+            Account.Create(SecondAccountId, "Simulation Second Farm", "simulation-second-farm", "UTC", "USD"));
+        await secondAccountDb.SaveChangesAsync(ct);
         logger.LogInformation("Seeded second pristine simulation account {AccountId}.", SecondAccountId);
     }
 

src/Cluckwork.Infrastructure/Persistence/TenantContext.cs

@@ -3,13 +3,33 @@ namespace Cluckwork.Infrastructure.Persistence;
 // Scoped per-request tenant resolution.
 // Populated by middleware from the authenticated principal's account_id claim
 // before any handler runs (tech spec §4.2).
+//
+// SINGLE-ASSIGNMENT (#546). Re-resolving the SAME account is a no-op; a
+// DIFFERENT account throws. Re-pointing a live scope at another account is
+// never legitimate: everything in the scope reads this lazily — the 27 EF
+// global query filters, TenantStampInterceptor, IAuditWriter — so a second
+// Resolve silently changes what already-executed code MEANT.
+//
+// The hazard becomes reachable with #532 (login by farm code): /auth/login is
+// AllowAnonymous but can still be called carrying a valid Farm A bearer, which
+// TenantResolutionMiddleware resolves before the handler runs. This shuts the
+// door before that lands rather than relying on convention afterwards.
 public sealed class TenantContext
 {
     public Guid AccountId { get; private set; }
     public bool IsResolved { get; private set; }
 
     public void Resolve(Guid accountId)
     {
+        if (IsResolved)
+        {
+            // Same account: a deliberate no-op, NOT an error. A caller that
+            // resolves defensively must not thereby become order-dependent.
+            if (AccountId == accountId) return;
+
+            throw new TenantReassignmentException(AccountId, accountId);
+        }
+
         AccountId = accountId;
         IsResolved = true;
     }

src/Cluckwork.Infrastructure/Persistence/TenantReassignmentException.cs

@@ -0,0 +1,21 @@
+namespace Cluckwork.Infrastructure.Persistence;
+
+// Distinct type so the failure is greppable in logs rather than read as a
+// generic 500 — the same rationale FarmTimeZoneException carries
+// (Infrastructure/Time/FarmClock.cs).
+//
+// Thrown when one DI scope is asked to serve two different accounts. See
+// TenantContext for why that is never legitimate.
+public sealed class TenantReassignmentException : InvalidOperationException
+{
+    public Guid ResolvedAccountId { get; }
+    public Guid AttemptedAccountId { get; }
+
+    public TenantReassignmentException(Guid resolvedAccountId, Guid attemptedAccountId)
+        : base($"TenantContext is already resolved to account {resolvedAccountId} and cannot be " +
+               $"re-resolved to {attemptedAccountId}. One scope serves exactly one account.")
+    {
+        ResolvedAccountId = resolvedAccountId;
+        AttemptedAccountId = attemptedAccountId;
+    }
+}

src/Cluckwork.Infrastructure/Persistence/TenantWriteMismatchException.cs

@@ -0,0 +1,25 @@
+namespace Cluckwork.Infrastructure.Persistence;
+
+// Distinct type so a write-side tenant breach is greppable in logs rather than
+// read as a generic 500 — the same rationale FarmTimeZoneException carries
+// (Infrastructure/Time/FarmClock.cs).
+//
+// Thrown by TenantStampInterceptor when a tracked write carries an AccountId
+// that is not the resolved tenant's. That is a bug or an attack, never user
+// input, so it is an exception rather than a Result failure.
+public sealed class TenantWriteMismatchException : InvalidOperationException
+{
+    public string EntityType { get; }
+    public Guid ExpectedAccountId { get; }
+    public Guid ActualAccountId { get; }
+
+    public TenantWriteMismatchException(
+        string entityType, string state, Guid expectedAccountId, Guid actualAccountId)
+        : base($"Refusing to save {state} {entityType}: AccountId {actualAccountId} does not match the " +
+               $"resolved tenant {expectedAccountId}.")
+    {
+        EntityType = entityType;
+        ExpectedAccountId = expectedAccountId;
+        ActualAccountId = actualAccountId;
+    }
+}

tests/Cluckwork.Api.IntegrationTests/IdempotencyRecordPurgeSweepTests.cs

@@ -44,14 +44,21 @@ private static IdempotencyRecord NewRecord(
 
     private static string Hash() => Guid.NewGuid().ToString("N") + Guid.NewGuid().ToString("N");
 
-    // idempotency_records has no tenant query filter, so seed/read with a throwaway
+    // idempotency_records has no tenant query filter, so READS use a throwaway
     // tenant scope (any AccountId works — the sweep never scopes).
-    private Task SeedAsync(params IdempotencyRecord[] records) =>
-        factory.WithTenantScopeAsync(Guid.NewGuid(), async db =>
-        {
-            db.IdempotencyRecords.AddRange(records);
-            await db.SaveChangesAsync();
-        });
+    //
+    // #546 — WRITES no longer can. These fixtures carry rows for several accounts
+    // and the interceptor now refuses a tracked write whose AccountId is not the
+    // resolved tenant's. A throwaway tenant claimed an account these rows do not
+    // belong to; an UNRESOLVED scope states the truth — the same path the seeders
+    // and one-shot CLI verbs take, where the write guard is deliberately inert.
+    private async Task SeedAsync(params IdempotencyRecord[] records)
+    {
+        using var scope = factory.Services.CreateScope();
+        var db = scope.ServiceProvider.GetRequiredService<AppDbContext>();
+        db.IdempotencyRecords.AddRange(records);
+        await db.SaveChangesAsync();
+    }
 
     private Task RunSweepAsync() =>
         factory.Services.GetRequiredService<IdempotencyRecordPurgeSweep>().RunAsync(CancellationToken.None);

tests/Cluckwork.Api.IntegrationTests/RefreshTokenPurgeSweepTests.cs

@@ -4,6 +4,7 @@ namespace Cluckwork.Api.IntegrationTests;
 using Cluckwork.Api.IntegrationTests.Infrastructure;
 using Cluckwork.Infrastructure.Identity;
 using Cluckwork.Infrastructure.Jobs;
+using Cluckwork.Infrastructure.Persistence;
 using Microsoft.AspNetCore.Mvc.Testing;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.DependencyInjection;
@@ -44,12 +45,25 @@ private static RefreshToken NewToken(
             : Guid.NewGuid().ToString("N") + Guid.NewGuid().ToString("N"),
     };
 
-    private Task SeedAsync(params RefreshToken[] tokens) =>
-        factory.WithTenantScopeAsync(Guid.NewGuid(), async db =>
-        {
-            db.RefreshTokens.AddRange(tokens);
-            await db.SaveChangesAsync();
-        });
+    // #546 — seeded through an UNRESOLVED tenant, deliberately.
+    //
+    // These fixtures span several accounts on purpose (Sweep_IsTenantSafeAndIdempotent
+    // seeds two), and RefreshToken carries an AccountId while being deliberately NOT
+    // tenant-query-filtered — the sweep is filter-free by design (see the header).
+    // Seeding them under a RESOLVED tenant therefore writes rows belonging to other
+    // accounts, which TenantStampInterceptor now refuses, correctly.
+    //
+    // The previous throwaway `WithTenantScopeAsync(Guid.NewGuid(), ...)` claimed a
+    // tenant these rows do not belong to. An unresolved scope states the truth
+    // instead: this is the same path the seeders and one-shot CLI verbs take, where
+    // the write guard is deliberately inert.
+    private async Task SeedAsync(params RefreshToken[] tokens)
+    {
+        using var scope = factory.Services.CreateScope();
+        var db = scope.ServiceProvider.GetRequiredService<AppDbContext>();
+        db.RefreshTokens.AddRange(tokens);
+        await db.SaveChangesAsync();
+    }
 
     private Task RunSweepAsync() =>
         factory.Services.GetRequiredService<RefreshTokenPurgeSweep>().RunAsync(CancellationToken.None);

tests/Cluckwork.Api.IntegrationTests/TenantContextSingleAssignmentTests.cs

@@ -0,0 +1,45 @@
+namespace Cluckwork.Api.IntegrationTests;
+
+using Cluckwork.Infrastructure.Persistence;
+
+// #546 — TenantContext is single-assignment: one DI scope serves exactly one
+// account. See TenantContext for why re-pointing a live scope is never
+// legitimate.
+public sealed class TenantContextSingleAssignmentTests
+{
+    [Fact]
+    public void Resolve_DifferentAccount_Throws()
+    {
+        var first = Guid.NewGuid();
+        var second = Guid.NewGuid();
+        var tenant = new TenantContext();
+        tenant.Resolve(first);
+
+        var ex = Assert.Throws<TenantReassignmentException>(() => tenant.Resolve(second));
+
+        Assert.Equal(first, ex.ResolvedAccountId);
+        Assert.Equal(second, ex.AttemptedAccountId);
+        // The original resolution survives the refusal — a rejected re-resolve
+        // must not leave the scope half-repointed.
+        Assert.Equal(first, tenant.AccountId);
+    }
+
+    // NOT a proof of single-assignment, and deliberately named so nobody reads
+    // it as one: this passes identically against the old plain-setter
+    // TenantContext and would survive deleting the feature outright. Its job is
+    // the opposite — pinning that we did NOT over-tighten into throwing on a
+    // harmless same-account re-resolve, which would make every defensive caller
+    // order-dependent.
+    [Fact]
+    public void Resolve_SameAccountTwice_DoesNotThrow()
+    {
+        var accountId = Guid.NewGuid();
+        var tenant = new TenantContext();
+
+        tenant.Resolve(accountId);
+        tenant.Resolve(accountId);
+
+        Assert.Equal(accountId, tenant.AccountId);
+        Assert.True(tenant.IsResolved);
+    }
+}