fix(auth): fail closed on unresolved flock-scope actors (#787)

AGENTS.md

@@ -82,6 +82,7 @@ dotnet test  Cluckwork.sln                 # 2295 tests as of 2026-09; integrati
 - **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 = <original>`, 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)
 - **A farm code changes only through `Account.Rename`, reached by the `rename-account` verb (#732).** No endpoint, no Settings field, and never a raw `UPDATE` — the hand-guarded `UPDATE "Accounts"` that preceded it bumped `Version` by hand, wrote no audit row and checked neither the slug pattern nor the reserved set. `Rename` validates through the same `TryValidateSlug` provisioning uses, bumps `Version` on a real change and deliberately does **not** bump it on a no-op (a rename to the code the farm already has mutates nothing, so "every mutation bumps `Version`" still holds and a Farm Settings form held open does not break). The service resolves the code to an id, resolves the tenant, takes the tenant-keyed `FOR UPDATE`, and then **re-compares the locked row against the snapshot the lookup took — both its slug and its `Version`** — lookup and lock are two statements, so a write that committed in between would otherwise be silently overwritten. Neither half of that comparison is redundant: `Version` catches the ABA schedule (renamed away and back, so the code the lookup read is the code the lock finds), the slug comparison catches a code change that never advanced `Version` (a raw `UPDATE`), and the price is that a rename racing a Farm Settings save or a suspend also returns `Account.SlugStale`. `IX_Accounts_Slug` and the `DbUpdateException` catch are the authority on the destination code; the pre-read in front of them is convenience. There is **no retired-code list**: a code a farm has moved off is immediately reusable, so `--slug <retired>` targets whoever holds it now — an accepted cost, which is why the verb and the runbook both say to run `list-accounts` immediately first. → [`732-farm-code-rename.md`](docs/decisions/732-farm-code-rename.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.
+- **Flock-scoped writes require a resolved actor (#787).** `FlockScopeGuard` returns `Auth.Unauthorized` before reading assignments when `ICurrentUser` is unresolved. HTTP middleware resolves the actor; a non-HTTP caller must resolve a real user or deliberately declare a system actor. This rule covers writes only: an unresolved `FlockScope` remains unrestricted for EF reads. A resolved system actor has no assignment rows and is therefore account-wide, so adding a system-actor caller of a flock-scoped handler requires an explicit access review.
 - **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` carries a durable monotonic ordering key, and it is GLOBAL (#508).** `Sequence` is a `bigint GENERATED ALWAYS AS IDENTITY` column, mapped as an EF **shadow property** so it reaches no API response, export or screen, and it is the tiebreak in all five order clauses that have one — `ListAsync`, and `created`/`creator`/`latest`/`promoted` in `GetProvenanceChunkAsync`. Before it, those ties broke on `Id`, which `AuditWriter` mints as a random v4 Guid, so a same-microsecond pair of changes named the **wrong actor** as the last changer. Three things not to re-derive: the counter is **one per table, not per account**, so values interleave across farms and it is never a per-farm revision number; ordering still leads with `OccurredAtUtc`, so **a clock rollback is unhandled** — a later insert stamped earlier sorts earlier and `Sequence` is never consulted; and rows written **before** that migration have no recoverable write order, because the backfill ordered same-timestamp legacy rows by `row_number()` over that same random Guid. → `docs/plans/508-audit-monotonic-order/01-diagnosis.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)

src/Cluckwork.Api/Middleware/FlockScopeResolutionMiddleware.cs

@@ -13,10 +13,11 @@ namespace Cluckwork.Api.Middleware;
 //
 // #612 — skips the DB read for any non-Worker effective role (Owner, Manager,
 // Sales, ReadOnly, Denied — role check from the resolved user, no I/O).
-// An unresolved user (seeders, one-shot verbs, background jobs) is Unrestricted
-// (matches FlockScopeGuard line 70 fail-open behavior).
+// An unresolved user (seeders, one-shot verbs, background jobs) is Unrestricted.
+// This read-scope default is separate from FlockScopeGuard, which fails closed
+// for unresolved actors on writes (#787).
 // 0 assignment rows (grandfathered #73) and any farm-wide row (FlockId=null)
-// are Unrestricted too — mirroring FlockScopeGuard lines 80 and 84 exactly.
+// are Unrestricted too, matching FlockScopeGuard's resolved-worker rules.
 public sealed class FlockScopeResolutionMiddleware(RequestDelegate next)
 {
     public async Task InvokeAsync(HttpContext context, FlockScope scope, CurrentUserContext user, AppDbContext db)
@@ -67,7 +68,7 @@ public async Task InvokeAsync(HttpContext context, FlockScope scope, CurrentUser
 
         if (assignments.Any(a => a.FlockId == null))
         {
-            // Farm-wide row: grants everything (matches FlockScopeGuard line 84). Unrestricted.
+            // Farm-wide row: grants everything. Unrestricted.
             scope.Resolve(true, []);
             await next(context);
             return;

src/Cluckwork.Application/Common/ICurrentUser.cs

@@ -5,13 +5,12 @@ namespace Cluckwork.Application.Common;
 // non-HTTP paths: both seeders and the one-shot CLI verbs (#500).
 //
 // NOT audit metadata. Roles and UserId are an AUTHORIZATION input:
-// FlockScopeGuard lets Owner and Manager past by role, treats a user with no
-// UserRoleAssignment rows as account-wide, and narrows everyone else to their
-// assigned flocks. RecordDailyEntry, SubmitDailyEntry, RecordFeedUsage and
-// RecordWaterUsage all consult it. So what is resolved here decides what the
-// caller MAY DO, not merely whose name lands on the audit row. This comment
-// used to say "handlers use it only through IAuditWriter", and #500's plan
-// reasoned from that for three revisions before anyone walked the consumers.
+// FlockScopeGuard refuses an unresolved actor (#787), lets non-Worker roles
+// past by role, treats a Worker with no UserRoleAssignment rows as
+// account-wide, and otherwise narrows the Worker to assigned flocks.
+// RecordDailyEntry, SubmitDailyEntry, RecordFeedUsage and RecordWaterUsage all
+// consult it. What is resolved here decides what the caller may do, not merely
+// whose name lands on the audit row.
 public interface ICurrentUser
 {
     bool IsResolved { get; }

src/Cluckwork.Infrastructure/Identity/CurrentUserContext.cs

@@ -38,20 +38,10 @@ public void Resolve(Guid userId, string email, IReadOnlyList<string>? roles = nu
     // label is now chosen deliberately instead of defaulted to "(unresolved)"
     // by a fallback nobody could see.
     //
-    // Roles stays empty on purpose — but READ THE NEXT PARAGRAPH before treating
-    // that as "unprivileged", because it is not.
-    //
-    // Empty roles means FlockScopeGuard's role bypass does not apply. It does
-    // NOT mean the actor is restricted: the guard then looks for
-    // UserRoleAssignment rows, a system actor has none, and zero rows is its
-    // "unscoped, account-wide" case. So a system actor has MORE effective flock
-    // reach than a restricted worker, not less.
-    //
-    // That is acceptable only because every current system-actor caller touches
-    // no flock-scoped handler at all. A future system
-    // actor that reaches RecordDailyEntry, SubmitDailyEntry, RecordFeedUsage or
-    // RecordWaterUsage would silently bypass flock scoping, and needs its own
-    // answer rather than this one.
+    // Roles stays empty on purpose. If a system actor later reaches a
+    // flock-scoped handler, FlockScopeGuard will treat its zero assignment rows
+    // as account-wide. No current system-actor caller reaches such a handler;
+    // adding one requires an explicit review of that access.
     public void ResolveSystemActor(string label)
     {
         UserId = Guid.Empty;

src/Cluckwork.Infrastructure/Persistence/FlockScope.cs

@@ -17,7 +17,8 @@ public sealed class FlockScope
     // Unresolved contexts are Unrestricted. HTTP middleware resolves every
     // request explicitly; design-time factories, seeders, one-shot verbs and
     // hand-built test contexts do not run that middleware and must retain the
-    // existing account-wide read behavior (INV-3/INV-4, FlockScopeGuard line 70).
+    // existing account-wide READ behavior (INV-3/INV-4). #787 made the separate
+    // write guard fail closed; unresolved reads remain unrestricted by design.
     public bool IsUnrestricted { get; private set; } = true;
     public IReadOnlyCollection<Guid> AssignedFlockIds { get; private set; } = [];
     public bool IsResolved { get; private set; }

src/Cluckwork.Infrastructure/Repositories/UserRoleAssignmentRepository.cs

@@ -67,37 +67,11 @@ public sealed class FlockScopeGuard(
 {
     public async Task<Result> CheckAsync(Guid flockId, CancellationToken ct = default)
     {
-        // #500 — this branch FAILS OPEN, and its old justification was false in a
-        // security-relevant way. Read the whole comment before touching it.
-        //
-        // It used to read: "non-HTTP system callers (startup/demo seeders) have
-        // no resolved user and are account-level by definition." That named the
-        // seeders as the reason for a bypass in an AUTHORIZATION path, and the
-        // seeders were in fact the only callers taking it. They now resolve a
-        // real actor, so the bypass they justified is gone with them.
-        //
-        // It is tempting to call the branch unreachable. That is TRUE FOR ONLY
-        // HALF THE CALLERS, and the #500 plan asserted the whole of it before
-        // anyone checked — the correction is worth stating exactly:
-        //
-        //   * RecordDailyEntry and SubmitDailyEntry DO write an audit event, and
-        //     IAuditWriter fails closed on an unresolved actor, so an unresolved
-        //     caller throws there before it ever reaches this line.
-        //   * RecordFeedUsage and RecordWaterUsage audit NOTHING. For those two,
-        //     an unresolved caller arrives here and is granted account-wide
-        //     access, silently. Nothing downstream catches it.
-        //
-        // No such caller exists today: both seeders declare an actor before every
-        // feed/water-usage call, and every HTTP route behind this guard requires
-        // authentication. So this is a live gap for a FUTURE non-HTTP caller, not
-        // a present defect — and the fix for such a caller is to make it declare
-        // an actor, not to lean on this branch.
-        //
-        // Closing the gap here means flipping an authorization default from open
-        // to closed, which is a behaviour change beyond #500's scope and deserves
-        // its own issue rather than a drive-by. Documented rather than silently
-        // narrowed.
-        if (!user.IsResolved) return Result.Success();
+        // #787 — authorization fails closed when a non-HTTP caller forgets to
+        // declare its actor. This must happen before any assignment lookup.
+        // HTTP requests resolve the actor in TenantResolutionMiddleware; both
+        // seeders resolve a real user before invoking flock-scoped handlers.
+        if (!user.IsResolved) return Result.Failure(AppError.Unauthorized());
 
         // #612 — only a plain Worker is ever flock-scoped. Owner, Manager,
         // Sales, ReadOnly and Denied all bypass assignment rows entirely.

tests/Cluckwork.Api.IntegrationTests/FlockScopeMiddlewareTests.cs

@@ -49,7 +49,7 @@ public async Task Worker_ZeroAssignments_Request_Succeeds()
         Assert.Equal(HttpStatusCode.OK, (await worker.GetAsync("/api/v1/flocks")).StatusCode);
 
         // M4 strengthen (mutation-check record): a 0-assignment worker is
-        // UNRESTRICTED (grandfathered #73, matches FlockScopeGuard line 80), so
+        // UNRESTRICTED (grandfathered #73, matches FlockScopeGuard), so
         // BOTH seeded flocks must be visible in its list — not an empty
         // restricted scope. Guards a Resolve(false, []) in the 0-assignment
         // branch (which would empty every read).
@@ -109,7 +109,7 @@ await factory.WithTenantScopeAsync(accountId, async db =>
         Assert.Equal(HttpStatusCode.OK, response.StatusCode);
 
         // M5 strengthen (mutation-check record): a farm-wide (FlockId=null) row
-        // grants everything (matches FlockScopeGuard line 84), so BOTH flocks
+        // grants everything (matches FlockScopeGuard), so BOTH flocks
         // must be visible — not an empty restricted scope. Guards removing the
         // farm-wide branch entirely (which would fall through to Resolve(false,
         // []) and empty every read).

tests/Cluckwork.Application.Tests/FlockScope/FlockScopeGuardTests.cs

@@ -0,0 +1,24 @@
+namespace Cluckwork.Application.Tests.FlockScope;
+
+using Cluckwork.Application.Common;
+using Cluckwork.Infrastructure.Identity;
+using Cluckwork.Infrastructure.Persistence;
+using Cluckwork.Infrastructure.Repositories;
+using Microsoft.EntityFrameworkCore;
+
+public sealed class FlockScopeGuardTests
+{
+    [Fact]
+    public async Task CheckAsync_WithUnresolvedActor_ReturnsUnauthorizedWithoutDatabaseAccess()
+    {
+        var options = new DbContextOptionsBuilder<AppDbContext>()
+            .UseNpgsql("Host=localhost;Port=1;Database=unreachable;Username=unreachable;Password=unreachable")
+            .Options;
+        using var db = new AppDbContext(options, new TenantContext(), new FlockScope());
+        var guard = new FlockScopeGuard(db, new CurrentUserContext());
+
+        var result = await guard.CheckAsync(Guid.NewGuid());
+
+        Assert.Equal(AppError.Unauthorized(), result.Error);
+    }
+}