diff --git a/AGENTS.md b/AGENTS.md index 967d7b1d2..34b055230 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -41,6 +41,7 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Endpoints:** minimal APIs grouped under `/api/v1/...` via `MapEndpoints`; writes require auth + an `Idempotency-Key` (middleware). - **Multi-tenancy:** every tenant-owned entity has `AccountId`; enforced by an EF **global query filter** + a **`TenantStampInterceptor`** (stamps on insert). `TenantContext` is resolved per-request from the JWT `account_id` claim. At startup it's unresolved → use `IgnoreQueryFilters()` in seeders. - **Auth:** asymmetric JWT + rotating refresh tokens. PEM keys come from config with escaped `\n`; normalize via `PemKey.Normalize` before `ImportFromPem`. Integration tests generate an ephemeral RSA pair at test-process startup (`TestJwtKeys` in `CluckworkWebApplicationFactory.cs`) — no real key material is ever committed to source (`deploy/.env.example` carries PEM armor with a `replace-me` body, not a usable key). +- **Credential epoch revocation (#364):** every access and refresh token is bound to the user's `CredentialEpoch`; all password-reset paths bump it so already-issued credentials fail on their next use. Epoch **`0` is permanently retired**: users start at `1`, legacy refresh rows start at `0`, and a missing or malformed `credential_epoch` claim is always a mismatch — never default the parsed claim to the current database epoch or otherwise treat it as an exemption. `CredentialEpochMiddleware` deliberately performs a fresh database read for every authenticated request; the round trip is the fail-closed revocation guarantee, so do not cache it. Keep the middleware after `TenantResolutionMiddleware` (the lookup is tenant-scoped) and before `MustChangePasswordMiddleware`; `/api/v1/auth/logout` is deliberately exempt so a superseded credential can still terminate its own session. Login and step-up snapshot the epoch + security stamp that the password actually verified **before** clearing the failed-access count, then refuse issuance if a concurrency-loss reload reveals different credentials — never let an old password adopt a concurrent reset. Their disabled-account branches still pay exactly one real password-hash cost, but must not mutate failed-access/lockout state; `AccountLockout` applies the same boundary after a concurrency reload, stopping rather than charging a stale failure to a newly disabled or reset credential. The SPA serializes refresh and self-password-change through the same per-tab queue + cross-tab Web Lock because both replace the shared HttpOnly cookie — a late old-epoch refresh must never overwrite the fresh pair that keeps the self-changing device signed in. This epoch is distinct from #283's first-login password-change flag, #265's operator-only break-glass temporary password, and #308's single-use browser step-up grant: the mechanisms have different audiences and lifetimes and none substitutes for another. - **Migrations + base provisioning (#283):** EF migrations auto-apply on startup (`Database:MigrateOnStartup`, default true — but **Production sets it `false`**; the `migrate` command runs DDL out-of-band, see the #263 bullet below). The default account, the four assignable roles (`Roles.Assignable`), the default egg grades, and the default packed-unit conversions are **static reference data baked into the migrations themselves** — deterministic and multi-instance-safe by construction, **no runtime seeder, no `Seed:*` config**. They ship as **hand-written `migrationBuilder.Sql` with `WHERE NOT EXISTS` guards, deliberately NOT EF's `HasData`/`InsertData`** — `HasData` keys on the PRIMARY KEY, which assumes a virgin schema, but every install that ever booted pre-#283 already has these rows under *different* ids (the old `DatabaseSeeder` minted roles/grades/conversions with `Guid.NewGuid()`), so `InsertData` either collides on `PK_Accounts` or silently violates the real natural-key unique indexes. The #245 squash retires *that* particular reason (a virgin database is now the only starting state), but **do not "regenerate" these into `HasData`**: `HasData` rows become part of the **model**, and every one of these tables is user-mutable through the app (Settings renames the account, `PUT /api/v1/egg-grades/{id}` renames a grade, `EggUnitConversion.Update` retunes `EggsPerUnit`), so a later model-diff would emit `UpdateData`/`DeleteData` that silently reverts the farm's own edits. Raw SQL seeds once and then leaves the rows alone forever. The guard's scope differs per batch, and the rule is *whether the user can change the natural key or remove a set member*: the **egg grades** batch is gated on the account's grade catalog being **entirely empty** (whole-set), because grades are user-managed — `PUT /api/v1/egg-grades/{id}` renames one, and a per-name guard cannot see a `Small` the farm renamed to `Pullet`, so it would resurrect an active, saleable duplicate (the old seeder skipped the whole set for exactly this reason); **roles, unit conversions, and the default account** stay **per-key**, because their keys are not user-mutable (no role CRUD exists at all; `EggUnitConversion.Update` cannot change `UnitCode` and there is no create/delete; the account's key is a fixed id), and per-key additionally lets a later-added role or unit code back-fill into an older install. `MigrationSecurityReviewTests` pins the SQL shape (12 INSERT statements, 21 rows, every one `WHERE NOT EXISTS`-guarded, the grades guard blind to `"Name"`) and `BaseReferenceDataMigrationTests` pins the result against a real virgin Postgres. `MigrationSecurityReviewTests` also asserts no migration ever inserts a credential-shaped row (`AspNetUsers`, `PasswordHash`, `SecurityStamp`, …) — the schema itself is never a place a credential can hide. The first admin is a **separate, deliberately NOT migration-baked** concern — see the #283 bullet below. - **One migration, squashed (#245):** `Persistence/Migrations/` holds exactly **one** migration, `InitialCreate`, generated from the current model on 2026-08-01. It replaced the 34 that had accumulated since 2026-06-27 (12 of them carrying raw SQL written for mid-history data states — backfills that touch 0 rows on an empty database but still execute). Done once, before the first production deploy, precisely because no `__EFMigrationsHistory` anywhere needed baselining; **it cannot be repeated after go-live**. Any database still carrying the old history (a dev DB from `deploy/docker-compose.dev.yml`, an old demo/sim instance) can no longer migrate forward — **drop and recreate**: `docker compose -f deploy/docker-compose.dev.yml down -v && docker compose -f deploy/docker-compose.dev.yml up -d`. Two things in that migration are **hand-carried and un-regenerable** — a plain `dotnet ef migrations add` drops both on the floor, so never regenerate `InitialCreate` without re-adding them: (1) the four **`lower("Name")` expression unique indexes** (`IX_EggGrades_AccountId_FarmId_LowerName`, `IX_ExpenseCategories_NameCi`, `UX_InventoryItems_Account_Farm_LowerName`, `IX_Products_AccountId_LowerName`) — EF cannot model a functional index, and losing them loses four uniqueness constraints; (2) the **static base reference data** raw SQL (see the bullet above). `MigrationSecurityReviewTests` and `BaseReferenceDataMigrationTests` fail if either goes missing. - **Seed command (#280, #284):** demo sample data is NOT boot-seeded — there is no `Seed__Demo` flag anymore. Run it explicitly, against a database that has **already been booted once** so the base seed above exists (default account, `Admin` role, default egg grades — the demo seed needs all three and fails fast with a clear message if they're missing): `ASPNETCORE_ENVIRONMENT=Development dotnet Cluckwork.Api.dll seed --profile demo` (an unset `ASPNETCORE_ENVIRONMENT` defaults to Production, which the guard below blocks — always set it for a local/dev/CI run). The command migrates the schema, runs `DemoDataSeeder`, then exits (Kestrel never starts); it is **authoritative** (seeds regardless of any config) and **fail-loud** (a real exit code: `0` only when data was actually seeded or was already present, non-zero with an stderr message otherwise — never a silent no-op). `DemoDataSeeder` is registered only when `!IsProduction()` (defense-in-depth): running the command against a Production-env process fails with a clear message instead of writing fake data. Intended flow: the serving process for a real farm stays Production; a non-Production process (dev box, CI, or a sim/load-test harness) runs `seed --profile demo` against its own already-base-seeded database. The **`simulation` profile** (#279) sits in the same `switch` with the same authoritative + fail-loud + Production-guarded contract (`SimulationDataSeeder`); it additionally records its date anchor + a completion marker in a durable `SimulationSeedState` row (anchor written before the first fixture mutation; completion stamped only after the manifest succeeds). A clean re-run — even one straddling a UTC-midnight rollover — reuses that anchor and converges to `AlreadySeeded` with an unchanged manifest fingerprint instead of writing a shifted duplicate set; `AlreadySeeded` is reported only when the counts are unchanged, so a `SimulationOptions` change that adds fixtures re-runs as `Seeded`. The durable anchor can't be poisoned by a mid-seed crash or by foreign daily entries a load test writes into the account (a data-derived anchor could); but a polluted account still fails the exact count validation **closed** (`Failed`) rather than certifying a fixture that no longer matches the definition. diff --git a/deploy/README.md b/deploy/README.md index e095f77a1..3905e4440 100644 --- a/deploy/README.md +++ b/deploy/README.md @@ -5,6 +5,22 @@ serves both the JSON API (`/api/*`) and the built React SPA (from `wwwroot`) on one domain. There is no separate frontend host — no CORS, no version skew between an app bundle and its API. +## Credential-epoch rollout (#364) + +Deploy the credential-epoch release everywhere before enabling the later +user-administration mutations that increment the epoch. Do not expose those +mutations until every pre-epoch process has drained: an older process cannot +validate the epoch claim and could otherwise accept an access token that a new +process has invalidated. Because this release is folded into the pre-production +`InitialCreate` migration, there are no deployed legacy rows to cut over. + +Rollback is deliberately ordered too. Drain every process running this +credential-epoch release, revoke **all active refresh tokens**, and only then +start a pre-epoch image. A pre-epoch binary ignores `IssuedEpoch`; starting it +before that revocation can resurrect an epoch-0 child written during a mixed +fleet window. Treat rollback as one forced re-login, not as a plain image +downgrade. + - `docker-compose.yml` — the production stack (app + Postgres), fronted by Traefik for TLS. See the root [README](../README.md) for the full run/backup walkthrough. diff --git a/docs/runbooks/break-glass-account-recovery.md b/docs/runbooks/break-glass-account-recovery.md index 28a6543e3..63d1e4d8e 100644 --- a/docs/runbooks/break-glass-account-recovery.md +++ b/docs/runbooks/break-glass-account-recovery.md @@ -20,14 +20,14 @@ Without this command the only recourse would be direct SQL surgery on the 1. Finds the user by email — **case-insensitive**, matching the login path (across accounts; disambiguate with `--account` if ever ambiguous). 2. Sets a **freshly generated** strong temporary password (never one you pass on the command line). -3. **Rotates the security stamp** (invalidates Identity-derived state). +3. **Rotates the security stamp and bumps the credential epoch** (#364) — invalidates Identity-derived state, and since every authenticated request is checked against the epoch, makes any access token already issued fail on its very next use. 4. **Clears any active lockout** and failed-attempt count — so a user locked out by repeated failed logins (the common trigger) can actually use the new password immediately, not after the 15-minute lockout expires. 5. **Revokes every refresh token** for that user — all existing sessions/devices are signed out. 6. Writes a conspicuous **`User.BreakGlassReset` audit row** carrying your `--reason` **and the host/OS-user that ran the command**. -> An access token already issued stays valid until it expires (~15 min) — there -> is no server-side denylist. Refresh is dead immediately, so the session cannot -> be extended. +> Since #364, an access token already issued is rejected on its very next +> request — it does not linger for the rest of its ~15-min lifetime. Refresh is +> dead immediately too, so the session cannot be extended. Unlike `seed --profile demo|simulation`, this command is **NOT** environment-gated: it is designed to run against a real **Production** database. Its safety comes diff --git a/docs/superpowers/plans/2026-08-02-credential-epoch.md b/docs/superpowers/plans/2026-08-02-credential-epoch.md new file mode 100644 index 000000000..2eeb12316 --- /dev/null +++ b/docs/superpowers/plans/2026-08-02-credential-epoch.md @@ -0,0 +1,107 @@ +# Credential Epoch Implementation Plan + +> **Status:** Implemented by PR #399. This is the implementation record, not a pending runbook. The application had never been deployed, so the schema changes were folded into the repository's sole `InitialCreate` migration as required by `AGENTS.md`; do not create a second credential-epoch migration or a legacy-data backfill. + +**Goal:** Add Deploy A's credential-epoch reader, immediate password-reset revocation, and client sign-out reason plumbing without exposing the future user-disable/email-change mutations. + +**Architecture:** `ApplicationUser.CredentialEpoch` and `RefreshToken.IssuedEpoch` bind all accepted credentials to a monotonic user value. JWT and refresh validation enforce the binding; a middleware database projection enforces access-token binding before authorization. Password-reset paths increment the epoch in the transaction that already revokes refresh tokens. The credential columns and defaults live directly in `InitialCreate`; a virgin database therefore has no pre-epoch credentials to revoke at cutover. + +**Tech Stack:** .NET 10, ASP.NET Core minimal APIs/middleware, EF Core/Postgres, ASP.NET Identity, xUnit/Testcontainers, React 19, Vite/Vitest, Playwright, k6. + +## Global Constraints + +- Do not implement #356 or #357 mutations/UI. +- Epoch is `int`, starts at `1`, and only moves upward; `0` is permanently invalid. +- `AspNetUsers.CredentialEpoch` defaults to `1`; `refresh_tokens.IssuedEpoch` defaults to `0`; every known mint site explicitly stamps the user epoch. +- The middleware must fail closed for absent or malformed claims by parsing them to `0`, query by `(UserId, AccountId)` without tracking, allow logout, and skip exception-handler re-execution. +- Pipeline order is `UseAuthentication → TenantResolution → CredentialEpoch → MustChangePassword → UseAuthorization → Idempotency`. +- Refresh mismatch must occur before grace/replay and must not revoke a family; same-epoch replay revocation is restricted to the presented token epoch. +- No credentials are hard-coded. Generated test credentials only. +- Deploy B documentation says it may not be exposed until all pre-Deploy-A processes are drained. + +--- + +### Task 1: Storage, migration, and token issuance + +**Files:** +- Modify: `src/Cluckwork.Infrastructure/Identity/ApplicationUser.cs` +- Modify: `src/Cluckwork.Infrastructure/Identity/RefreshToken.cs` +- Modify: `src/Cluckwork.Infrastructure/Identity/JwtTokenService.cs` +- Modify: `src/Cluckwork.Infrastructure/Persistence/Configurations/ApplicationUserConfiguration.cs` +- Modify: `src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs` +- Modify: `src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs` +- Test: `tests/Cluckwork.Api.IntegrationTests/CredentialEpochTests.cs` + +**Interfaces:** +- Produces `ApplicationUser.CredentialEpoch`, `ApplicationUser.DisabledAt`, `ApplicationUser.DisabledBy`, `RefreshToken.IssuedEpoch`, and the JWT `credential_epoch` claim. + +- [ ] **Step 1: Write failing integration tests** for a valid current-epoch JWT and a token minted from every login/change-password/refresh path carrying the matching epoch. Name each test for the removed claim/property/mint assignment that must make it fail. +- [ ] **Step 2: Run the focused tests** and confirm failure because the claim and columns do not exist. +- [ ] **Step 3: Implement the minimal model/configuration/JWT/mint assignments** and fold the columns/defaults into `InitialCreate`, preserving its hand-written indexes and reference-data SQL. Do not generate a second migration. +- [ ] **Step 4: Run focused tests** and confirm green. + +### Task 2: Credential epoch enforcement and reset bumps + +**Files:** +- Create: `src/Cluckwork.Api/Middleware/CredentialEpochMiddleware.cs` +- Modify: `src/Cluckwork.Api/Program.cs` +- Modify: `src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs` +- Modify: `src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs` +- Test: `tests/Cluckwork.Api.IntegrationTests/CredentialEpochTests.cs` + +**Interfaces:** +- Consumes the Task 1 columns/claim. +- Produces a 401 `Auth.CredentialsSuperseded` response for an access-token mismatch and generic invalid-refresh failures for rejected refresh credentials. + +- [ ] **Step 1: Write failing integration tests** for a live access token rejected immediately after each password reset path, separately absent/malformed epoch claims, logout allowance, exception-handler re-execution, disabled-user login/refresh/step-up refusal, and precise middleware ordering. +- [ ] **Step 2: Run the focused tests** and confirm each fails at the missing enforcement/bump behavior. +- [ ] **Step 3: Add the middleware and reset epoch mutation** in their existing transactions; place refresh checking before grace/replay and scope replay family invalidation by issued epoch. +- [ ] **Step 4: Run focused tests** and confirm green. + +### Task 3: Virgin-schema and concurrency contracts + +**Files:** +- Modify: `tests/Cluckwork.Api.IntegrationTests/CredentialEpochTests.cs` +- Modify: `tests/Cluckwork.Api.IntegrationTests/RefreshTokenFlowTests.cs` +- Modify: `tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs` as needed + +**Interfaces:** +- Consumes runtime epoch enforcement and migration artifacts. +- Produces coverage for retired `IssuedEpoch=0` rows, virgin-schema defaults, race-safe child rejection, no cross-epoch replay revocation, and preserved same-epoch theft detection. + +- [ ] **Step 1: Write failing database-backed tests** that insert retired-epoch rows directly and use barriers around refresh/reset interleavings; assert each child token's usability or rejection by presenting it. +- [ ] **Step 2: Run the focused tests** and confirm they fail under the pre-epoch or incorrectly ordered/scoped implementations. +- [ ] **Step 3: Add only the synchronization test hooks established by the integration harness, complete the production query predicates required by these observable contracts, and keep `MigrationSecurityReviewTests`' exactly-one-migration fence intact.** +- [ ] **Step 4: Run focused tests and mutation checks** (remove/check-bypass mutations locally) and restore production code after each confirmed red result. + +### Task 4: SPA teardown reason and documentation + +**Files:** +- Modify: `web/src/api/client.ts` +- Modify: `web/src/auth/AuthContext.tsx` +- Modify: `web/src/routes/Login.tsx` +- Modify: `web/src/api/client.test.ts` +- Modify: `web/src/auth/AuthContext.lifecycle.test.tsx` +- Modify: `web/src/routes/Login.test.tsx` +- Modify: `web/src/i18n/en.ts`, `web/src/i18n/es.ts`, `web/src/i18n/tl.ts`, and translation namespace metadata +- Modify: `web/src/routes/HelpPage.tsx` and its test +- Modify: `specs/product/GLOSSARY.md` +- Modify: the repository deployment documentation selected by existing deployment guidance + +**Interfaces:** +- `onUnauthenticated` receives the original 401 problem title. +- Login maps `Auth.CredentialsSuperseded` and `Auth.AccountDisabled` to translated copy while retaining generic invalid-credential behavior for a direct failed sign-in. + +- [ ] **Step 1: Write failing Vitest cases** showing 401 titles survive refresh exhaustion/auth teardown and are rendered on Login, including generic 401 compatibility. +- [ ] **Step 2: Run focused Vitest files** and confirm failure. +- [ ] **Step 3: Implement typed reason propagation, localized messages, Help/Glossary/deploy-drain prose, and only the related test changes.** +- [ ] **Step 4: Run focused Vitest files** and confirm green. + +### Task 5: Non-CI harness contracts and final verification + +**Files:** +- Inspect/update only when the changed token/cookie contract requires it: `tools/simulation/ui/specs/session-refresh.spec.ts`, `tools/simulation/ui/specs/session-races.spec.ts`, `tools/simulation/k6/auth.js`, `tools/simulation/k6/auth-smoke.js`, `tools/simulation/k6/config.js`, `tools/simulation/reset.sh`, `tools/simulation/bootstrap.sh`. + +- [ ] **Step 1: Inspect the direct refresh/401/cookie assertions and adjust only behavior made intentionally different by the issue.** +- [ ] **Step 2: Run every required command exactly as requested:** `dotnet build Cluckwork.sln`, `dotnet test Cluckwork.sln`, `cd web && npm run typecheck && npm test`, `bash tools/simulation/reset.sh`, and `cd tools/simulation/ui && npm test && npm run mutation`. +- [ ] **Step 3: Review `git diff --check` and inspect the diff against the issue checklist before handing the implementation off for publication.** diff --git a/specs/product/GLOSSARY.md b/specs/product/GLOSSARY.md index 1c30d0414..c1fe663e4 100644 --- a/specs/product/GLOSSARY.md +++ b/specs/product/GLOSSARY.md @@ -518,6 +518,13 @@ replayed/stale token is still caught and revokes the whole family. Consuming a token is an atomic compare-and-swap (a per-token concurrency stamp), so concurrent replays can never fork one token into two live sessions. +**Credential epoch (#364)** — a monotonically increasing per-user number carried +in every access token and stamped onto every refresh token. A request is valid +only when its epoch matches the current user record, so an administrative +credential reset can invalidate access tokens immediately without a per-request +revocation list. The epoch readers ship before the mutations that advance it; +deploys must drain older replicas before enabling those mutations. + **Version (concurrency token)** — every mutable aggregate carries a `Version` that each mutation bumps. Two concurrent edits: first save wins, second gets a 409 and retries against fresh state. Append-only aggregates (bird @@ -586,17 +593,19 @@ grant** because the target holds Owner (see below) — resetting any other role's password is unchanged and stays ungated. Both revoke every refresh token for that user, so other devices are signed out — the self-service path hands the device that made the change a fresh pair so it -stays signed in. Eviction is bounded by the access-token lifetime: an -already-issued access token keeps working until it expires (~15 min), because -tokens are stateless and there is no server-side denylist. +stays signed in. Since #364, both paths also bump the user's **credential +epoch** (see above) in that same transaction, and every authenticated request +is checked against it — so an already-issued access token is rejected on its +very next request, not merely bounded by the ~15-min access-token lifetime. **Step-up authentication (#308)** — a fresh proof of identity required, on top of a normal valid Owner access token, before two specific actions: **creating another Owner**, and **resetting an existing Owner's password**. The threat it -closes: a stolen-but-still-valid Owner access token (good for ~15 min, no -server-side denylist — see **Password change** above) is otherwise enough on -its own to mint a second, independent Owner or take over an existing one, -turning short-lived token theft into durable account control. Every other +closes: a stolen-but-still-valid Owner access token (good for ~15 min — merely +holding it bumps no credential epoch, see **Credential epoch** above) is +otherwise enough on its own to mint a second, independent Owner or take over +an existing one, turning short-lived token theft into durable account +control. Every other action on the Users screen — creating a Worker/Manager/Sales/Read-only user, resetting one of their passwords, editing a display name, flock assignment — is **unchanged and ungated**; the point is to gate exactly the two operations diff --git a/src/Cluckwork.Api/Middleware/CredentialEpochMiddleware.cs b/src/Cluckwork.Api/Middleware/CredentialEpochMiddleware.cs new file mode 100644 index 000000000..d9b0aac90 --- /dev/null +++ b/src/Cluckwork.Api/Middleware/CredentialEpochMiddleware.cs @@ -0,0 +1,66 @@ +namespace Cluckwork.Api.Middleware; + +using System.Globalization; +using System.IdentityModel.Tokens.Jwt; +using Cluckwork.Infrastructure.Persistence; +using Microsoft.AspNetCore.Diagnostics; +using Microsoft.AspNetCore.Mvc; +using Microsoft.EntityFrameworkCore; + +// #364 — server-side access-token revocation. Every authenticated request is +// bound to the credential epoch held by its exact (user, account) row. Missing +// and malformed claims deliberately become retired epoch zero, never an opt-out. +public sealed class CredentialEpochMiddleware(RequestDelegate next) +{ + private const string LogoutPath = "/api/v1/auth/logout"; + + public async Task InvokeAsync(HttpContext context, AppDbContext db) + { + if (context.User.Identity?.IsAuthenticated == true + && context.Features.Get() is null + && !IsLogoutPath(context.Request.Path)) + { + var userIdClaim = context.User.FindFirst(JwtRegisteredClaimNames.Sub)?.Value; + var accountIdClaim = context.User.FindFirst("account_id")?.Value; + var epochClaim = context.User.FindFirst("credential_epoch")?.Value; + var tokenEpoch = int.TryParse( + epochClaim, NumberStyles.None, CultureInfo.InvariantCulture, out var parsedEpoch) + ? parsedEpoch + : 0; + + var credentialState = Guid.TryParse(userIdClaim, out var userId) + && Guid.TryParse(accountIdClaim, out var accountId) + ? await db.Users.AsNoTracking() + .Where(user => user.Id == userId && user.AccountId == accountId) + .Select(user => new { user.CredentialEpoch, user.DisabledAt }) + .SingleOrDefaultAsync(context.RequestAborted) + : null; + + if (credentialState is null + || credentialState.CredentialEpoch != tokenEpoch + || credentialState.DisabledAt is not null) + { + var disabled = credentialState?.DisabledAt is not null; + context.Response.StatusCode = StatusCodes.Status401Unauthorized; + context.Response.ContentType = "application/problem+json"; + await context.Response.WriteAsJsonAsync(new ProblemDetails + { + Title = disabled ? "Auth.AccountDisabled" : "Auth.CredentialsSuperseded", + Detail = disabled + ? "Your account has been disabled." + : "Your credentials have been superseded. Sign in again.", + Status = StatusCodes.Status401Unauthorized, + }); + return; + } + } + + await next(context); + } + + // Endpoint routing accepts the conventional trailing-slash form too. Keep + // both spellings reachable for a superseded bearer, without widening the + // exemption to descendants such as /auth/logout/anything. + private static bool IsLogoutPath(PathString path) => + path == LogoutPath || path == $"{LogoutPath}/"; +} diff --git a/src/Cluckwork.Api/Middleware/MustChangePasswordMiddleware.cs b/src/Cluckwork.Api/Middleware/MustChangePasswordMiddleware.cs index 7af3582c1..056fb98c9 100644 --- a/src/Cluckwork.Api/Middleware/MustChangePasswordMiddleware.cs +++ b/src/Cluckwork.Api/Middleware/MustChangePasswordMiddleware.cs @@ -36,11 +36,11 @@ public sealed class MustChangePasswordMiddleware(RequestDelegate next) // Nothing else — not even GET /me — needs to be reachable: the SPA // decodes email/role straight from the JWT it already holds (claims.ts), // so the first-login screen needs no additional API call. - private static readonly HashSet AllowedPaths = - [ + private static readonly HashSet AllowedPaths = new(StringComparer.OrdinalIgnoreCase) + { "/api/v1/auth/change-password", "/api/v1/auth/logout", - ]; + }; public async Task InvokeAsync(HttpContext context) { @@ -56,7 +56,7 @@ public async Task InvokeAsync(HttpContext context) if (context.User.Identity?.IsAuthenticated == true && context.User.FindFirst("must_change_password")?.Value == "true" && context.Features.Get() is null - && !AllowedPaths.Contains(context.Request.Path.Value ?? string.Empty)) + && !IsAllowedPath(context.Request.Path)) { context.Response.StatusCode = StatusCodes.Status403Forbidden; context.Response.ContentType = "application/problem+json"; @@ -71,4 +71,12 @@ await context.Response.WriteAsJsonAsync(new ProblemDetails await next(context); } + + private static bool IsAllowedPath(PathString path) + { + var value = path.Value ?? string.Empty; + return AllowedPaths.Contains(value) + || (value.EndsWith("/", StringComparison.Ordinal) + && AllowedPaths.Contains(value[..^1])); + } } diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index b655e3da1..d29284551 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -248,6 +248,7 @@ app.UseAuthentication(); app.UseMiddleware(); +app.UseMiddleware(); // #283 — the first-run "you must set a new password" gate. BEFORE // UseAuthorization (deliberately) so it applies uniformly regardless of which // AuthPolicies tier an endpoint carries, and before idempotency so a blocked diff --git a/src/Cluckwork.Application/Common/IIdentityProvider.cs b/src/Cluckwork.Application/Common/IIdentityProvider.cs index 627518867..8ed43bd0c 100644 --- a/src/Cluckwork.Application/Common/IIdentityProvider.cs +++ b/src/Cluckwork.Application/Common/IIdentityProvider.cs @@ -44,8 +44,9 @@ Task UpdateUserAsync( // #165 — an Owner sets another user's password without knowing the current // one. Account-scoped (foreign id -> NotFound) and it REVOKES every refresh // token for the target, so a reset actually evicts whoever held the old - // password. Bounded by the access-token lifetime: an already-issued JWT stays - // valid until it expires (~15 min) — there is no server-side denylist. + // password. Since #364 it also bumps the target's credential epoch in the + // same transaction, so an already-issued JWT is rejected on its very next + // request — no longer merely bounded by the ~15-min access-token lifetime. Task SetUserPasswordAsync( Guid accountId, Guid userId, string newPassword, CancellationToken ct = default); diff --git a/src/Cluckwork.Infrastructure/Identity/AccountLockout.cs b/src/Cluckwork.Infrastructure/Identity/AccountLockout.cs index d1b719e83..d2a9160a9 100644 --- a/src/Cluckwork.Infrastructure/Identity/AccountLockout.cs +++ b/src/Cluckwork.Infrastructure/Identity/AccountLockout.cs @@ -53,8 +53,16 @@ internal static class AccountLockout public static async Task RecordFailedAccessAsync( UserManager userManager, AppDbContext db, ApplicationUser user) { + // Bind this durable failure to the credential state whose password was + // actually rejected. A concurrent disable or password reset supersedes + // that proof; after a concurrency reload, never charge the stale guess + // to the newer state (or leave a disabled account locked on re-enable). + var attemptedCredentialEpoch = user.CredentialEpoch; for (var attempt = 0; attempt < 10; attempt++) { + if (user.DisabledAt is not null || user.CredentialEpoch != attemptedCredentialEpoch) + return; + var result = await SingleAttemptExecution.RunAsync( db.Database, () => userManager.AccessFailedAsync(user)); if (result.Succeeded) diff --git a/src/Cluckwork.Infrastructure/Identity/ApplicationUser.cs b/src/Cluckwork.Infrastructure/Identity/ApplicationUser.cs index 0fe2b0daa..4b7377a09 100644 --- a/src/Cluckwork.Infrastructure/Identity/ApplicationUser.cs +++ b/src/Cluckwork.Infrastructure/Identity/ApplicationUser.cs @@ -20,4 +20,14 @@ public sealed class ApplicationUser : IdentityUser // successful password reset (self-service change, an Owner's SetUserPassword, // or break-glass recovery) clears it — see IdentityProvider. public bool MustChangePassword { get; set; } + + // #364 — every access and refresh credential is bound to this monotonic + // version. Zero is permanently retired so data written by a pre-epoch binary + // can never match a live user. + public int CredentialEpoch { get; set; } = 1; + + // #364 ships the readers but no mutation endpoint; #356 will supply the + // audited administration workflow that sets these fields. + public DateTimeOffset? DisabledAt { get; set; } + public Guid? DisabledBy { get; set; } } diff --git a/src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs b/src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs index 2956d291e..af371b61c 100644 --- a/src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs +++ b/src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs @@ -29,6 +29,20 @@ public async Task> LoginAsync(string email, string password, C return Result.Failure(Error.Validation("Identity.InvalidCredentials", "Invalid email or password.")); } + if (user.DisabledAt is not null) + { + // Pay the same password-hash cost for a correct or incorrect + // password, but never feed a disabled account into Identity's + // failed-access counter. Otherwise password validity is observable + // through both timing and durable lockout state, and an account can + // emerge from a later re-enable already locked by guesses made while + // it was disabled. + userManager.PasswordHasher.VerifyHashedPassword( + user, user.PasswordHash ?? TimingEqualization.DummyHash, password); + return Result.Failure(Error.Validation( + "Identity.InvalidCredentials", "Invalid email or password.")); + } + // Account lockout (#128): once failures reach the configured threshold the // account is locked for a cool-off window. A locked account is refused with // the SAME generic error (and PBKDF2 is still paid) as a wrong password, so @@ -47,8 +61,22 @@ public async Task> LoginAsync(string email, string password, C return Result.Failure(Error.Validation("Identity.InvalidCredentials", "Invalid email or password.")); } + // Bind the successful proof to the credential values that were actually + // verified. ResetFailedAccessCountAsync may lose optimistic concurrency + // to a password reset/disable and reload that newer row into `user`; the + // old password must never mint tokens carrying the superseding epoch. + var verifiedCredentialEpoch = user.CredentialEpoch; + var verifiedSecurityStamp = user.SecurityStamp; + // Correct password — clear any accumulated failures (no-op DB-wise if zero). await ResetFailedAccessCountAsync(user, ct); + if (user.DisabledAt is not null + || user.CredentialEpoch != verifiedCredentialEpoch + || !string.Equals(user.SecurityStamp, verifiedSecurityStamp, StringComparison.Ordinal)) + { + return Result.Failure(Error.Validation( + "Identity.InvalidCredentials", "Invalid email or password.")); + } var (rawToken, tokenHash) = GenerateRefreshToken(); var minted = NewToken(user, tokenHash); @@ -99,6 +127,13 @@ public async Task> RefreshAsync(string refreshToken, Cancellat if (stored is null) return Result.Failure(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid.")); + // This MUST precede the #176 grace/replay branch. A retired token is + // not evidence about the current family, so it fails inert rather than + // letting replay detection revoke a later epoch's credentials. + var user = await userManager.FindByIdAsync(stored.UserId.ToString()); + if (user is null || user.DisabledAt is not null || stored.IssuedEpoch != user.CredentialEpoch) + return Result.Failure(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid.")); + // Presenting an already-rotated/revoked token normally means it was replayed — // treat as a possible theft and revoke every active token for the user. var viaGrace = false; @@ -113,10 +148,21 @@ public async Task> RefreshAsync(string refreshToken, Cancellat // delivered, so this does not fork the chain. Anything else — a stale // token, an expired grace, or a replacement already gone — is a genuine // replay and still burns the family down. - var graced = await TryGraceReplacementAsync(stored, now, ct); + var (graced, crossEpochReplacement) = + await InspectGraceReplacementAsync(stored, now, ct); + if (crossEpochReplacement) + { + // A mixed-version replica linked this current-epoch token to a + // child whose default epoch is permanently retired. That child + // is not evidence of theft in the current family: fail inertly, + // or repeated presentation of the parent could keep revoking + // unrelated sessions minted after the new version took over. + return Result.Failure(Error.Validation( + "Identity.InvalidRefreshToken", "Refresh token is invalid.")); + } if (graced is null) { - await RevokeAllActiveForUserAsync(stored.UserId, now, ct); + await RevokeAllActiveForUserAsync(stored.UserId, stored.IssuedEpoch, now, ct); return Result.Failure(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid.")); } stored = graced; @@ -126,10 +172,6 @@ public async Task> RefreshAsync(string refreshToken, Cancellat if (stored.ExpiresAt <= now) return Result.Failure(Error.Validation("Identity.ExpiredRefreshToken", "Refresh token has expired.")); - var user = await userManager.FindByIdAsync(stored.UserId.ToString()); - if (user is null) - return Result.Failure(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid.")); - // Rotate: revoke the presented token and issue a fresh one. Marking the // revocation as grace-sourced (#176) bounds the grace to a single hop: the // token minted here can be rotated normally, but this just-revoked link @@ -359,8 +401,11 @@ public async Task BreakGlassResetAsync( // without the current one — which applies the full policy and rotates the // SecurityStamp — clear any lockout, evict every live session, and append one // audit row, all in a single transaction so the password change and the - // session revocation land together or not at all (#165 review). An already- - // issued access token stays valid until it expires (~15 min) — no denylist. + // session revocation land together or not at all (#165 review). Since #364 + // it also bumps CredentialEpoch in that same transaction (below), so an + // already-issued access token is rejected by CredentialEpochMiddleware on + // its very next request — no longer merely bounded by the ~15-min + // access-token lifetime. private Task ResetPasswordAndRevokeAsync( ApplicationUser user, string newPassword, string auditAction, string? reason, object? details, CancellationToken ct) @@ -392,6 +437,7 @@ private Task ResetPasswordAndRevokeAsync( // there is nothing left to force. `user` is the same DbContext-tracked // instance db.SaveChangesAsync persists below — no separate save needed. user.MustChangePassword = false; + user.CredentialEpoch++; // Clear any active lockout / failed-attempt count (#265 review). Without // this, the exact case break-glass exists for — a user locked out by @@ -455,6 +501,7 @@ public async Task> ChangeOwnPasswordAsync( // the must_change_password claim, un-gating the rest of the app. if (user.MustChangePassword) user.MustChangePassword = false; + user.CredentialEpoch++; // Every session dies (other devices are signed out), then this caller gets // a fresh pair so the device that made the change stays signed in. @@ -624,39 +671,55 @@ private RefreshToken NewToken(ApplicationUser user, string tokenHash) UserId = user.Id, AccountId = user.AccountId, TokenHash = tokenHash, + IssuedEpoch = user.CredentialEpoch, CreatedAt = now, ExpiresAt = now.AddDays(jwtOptions.Value.RefreshTokenDays) }; } - // #176 — returns the live replacement to rotate when `revoked` is a benign - // grace retry (rotated within the grace window and its replacement is still - // active), or null when it is a genuine replay that must revoke the family. - private async Task TryGraceReplacementAsync( + // #176/#364 — identifies three distinct states for a revoked token: + // a live same-epoch grace replacement, a genuine same-epoch replay, or a + // linked child written with a retired epoch by an old replica. The last one + // must fail inertly even outside the grace window; it is mixed-version + // evidence, not evidence that the current-epoch family was stolen. + private async Task<(RefreshToken? Replacement, bool CrossEpochReplacement)> + InspectGraceReplacementAsync( RefreshToken revoked, DateTimeOffset now, CancellationToken ct) { + if (revoked.ReplacedByTokenHash is null || revoked.RevokedAt is null) + return (null, false); + + var replacement = await db.RefreshTokens.FirstOrDefaultAsync(t => + t.TokenHash == revoked.ReplacedByTokenHash + && t.UserId == revoked.UserId + && t.AccountId == revoked.AccountId, ct); + if (replacement is not null && replacement.IssuedEpoch != revoked.IssuedEpoch) + return (null, true); + var graceSeconds = jwtOptions.Value.RefreshReuseGraceSeconds; - var elapsed = now - (revoked.RevokedAt ?? now); - if (graceSeconds <= 0 // grace disabled → strict replay - || revoked.RevokedByGrace // already a grace hop → don't chain (one-hop bound) - || revoked.ReplacedByTokenHash is null - || revoked.RevokedAt is null + var elapsed = now - revoked.RevokedAt.Value; + if (graceSeconds <= 0 // grace disabled → strict replay + || revoked.RevokedByGrace // already a grace hop → don't chain (one-hop bound) || elapsed < TimeSpan.Zero // clock-skew guard: a future RevokedAt must not widen the window || elapsed > TimeSpan.FromSeconds(graceSeconds)) - return null; + return (null, false); - var replacement = await db.RefreshTokens - .FirstOrDefaultAsync(t => t.TokenHash == revoked.ReplacedByTokenHash, ct); - return replacement is not null && replacement.IsActive(now) ? replacement : null; + return (replacement is not null && replacement.IsActive(now) ? replacement : null, false); } - private async Task RevokeAllActiveForUserAsync(Guid userId, DateTimeOffset now, CancellationToken ct) + private async Task RevokeAllActiveForUserAsync( + Guid userId, DateTimeOffset now, CancellationToken ct) => + await RevokeAllActiveForUserAsync(userId, issuedEpoch: null, now, ct); + + private async Task RevokeAllActiveForUserAsync( + Guid userId, int? issuedEpoch, DateTimeOffset now, CancellationToken ct) { // Bulk update rather than tracked read-modify-save: it never trips the // #176 xmin concurrency token (so it is safe to call from the rotation // fail path) and revokes the whole family in one statement. await db.RefreshTokens - .Where(t => t.UserId == userId && t.RevokedAt == null) + .Where(t => t.UserId == userId && t.RevokedAt == null + && (!issuedEpoch.HasValue || t.IssuedEpoch == issuedEpoch.Value)) .ExecuteUpdateAsync(s => s.SetProperty(t => t.RevokedAt, now), ct); } diff --git a/src/Cluckwork.Infrastructure/Identity/JwtTokenService.cs b/src/Cluckwork.Infrastructure/Identity/JwtTokenService.cs index 22e325a88..f6ce462cb 100644 --- a/src/Cluckwork.Infrastructure/Identity/JwtTokenService.cs +++ b/src/Cluckwork.Infrastructure/Identity/JwtTokenService.cs @@ -33,7 +33,8 @@ public TokenPair CreateTokenPair(ApplicationUser user, IReadOnlyCollection new Claim("role", r))); // #283 — carried only when true (mirrors the role claims' omit-if-absent @@ -41,11 +42,12 @@ public TokenPair CreateTokenPair(ApplicationUser user, IReadOnlyCollection builder) builder.Property(e => e.TokenHash).HasMaxLength(64).IsRequired(); builder.Property(e => e.ReplacedByTokenHash).HasMaxLength(64); builder.Property(e => e.RevokedByGrace).HasDefaultValue(false); + builder.Property(e => e.IssuedEpoch).HasDefaultValue(0); builder.HasIndex(e => e.TokenHash).IsUnique(); builder.HasIndex(e => e.UserId); // #270 — covers RefreshTokenPurgeSweep's delete predicate diff --git a/src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs b/src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs index 93f8e2cef..75465e5d7 100644 --- a/src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs +++ b/src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs @@ -174,6 +174,16 @@ public async Task> IssueAsync( return Result.Failure(WrongPasswordError); } + if (user.DisabledAt is not null) + { + // Match login's disabled-account path: pay exactly one real hash + // cost for either password, but never let guesses mutate durable + // failed-access/lockout state while the account is disabled. + userManager.PasswordHasher.VerifyHashedPassword( + user, user.PasswordHash ?? TimingEqualization.DummyHash, currentPassword); + return Result.Failure(WrongPasswordError); + } + // #128 per-account lockout, same as LoginAsync. This endpoint is a SECOND // password oracle — and the one guarding Owner takeover — so leaving it // out would have left only the per-IP limiter here, which a distributed @@ -193,11 +203,24 @@ public async Task> IssueAsync( return Result.Failure(WrongPasswordError); } + // ResetFailedAccessCountAsync can reload a concurrently reset/disabled + // row after an optimistic-concurrency loss. Preserve the exact + // credential proof we just accepted so the old password cannot issue a + // grant stamped with credentials that superseded it. + var verifiedCredentialEpoch = user.CredentialEpoch; + var verifiedSecurityStamp = user.SecurityStamp; + // Correct password — clear any accumulated failures, as login does. // Shared with login via AccountLockout (#269 review): a discarded // IdentityResult here left `user` tracked with a stale ConcurrencyStamp, // which any later save on this scoped DbContext would re-flush and throw. await AccountLockout.ResetFailedAccessCountAsync(userManager, db, user, ct); + if (user.DisabledAt is not null + || user.CredentialEpoch != verifiedCredentialEpoch + || !string.Equals(user.SecurityStamp, verifiedSecurityStamp, StringComparison.Ordinal)) + { + return Result.Failure(WrongPasswordError); + } var now = timeProvider.GetUtcNow(); var expires = now.AddMinutes(Math.Max(1, jwtOptions.Value.StepUpGrantMinutes)); diff --git a/src/Cluckwork.Infrastructure/Persistence/Configurations/ApplicationUserConfiguration.cs b/src/Cluckwork.Infrastructure/Persistence/Configurations/ApplicationUserConfiguration.cs index 110a79b0f..e90c07c03 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Configurations/ApplicationUserConfiguration.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Configurations/ApplicationUserConfiguration.cs @@ -10,6 +10,9 @@ namespace Cluckwork.Infrastructure.Persistence.Configurations; public sealed class ApplicationUserConfiguration : IEntityTypeConfiguration { // 16 > the 8-char grammar max: headroom without an unbounded text column. - public void Configure(EntityTypeBuilder builder) => + public void Configure(EntityTypeBuilder builder) + { builder.Property(u => u.Language).HasMaxLength(16); + builder.Property(u => u.CredentialEpoch).HasDefaultValue(1); + } } diff --git a/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.Designer.cs b/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.Designer.cs index 9de892656..796be0365 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.Designer.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.Designer.cs @@ -1277,6 +1277,17 @@ protected override void BuildTargetModel(ModelBuilder modelBuilder) .IsConcurrencyToken() .HasColumnType("text"); + b.Property("CredentialEpoch") + .ValueGeneratedOnAdd() + .HasColumnType("integer") + .HasDefaultValue(1); + + b.Property("DisabledAt") + .HasColumnType("timestamp with time zone"); + + b.Property("DisabledBy") + .HasColumnType("uuid"); + b.Property("DisplayName") .HasColumnType("text"); @@ -1360,6 +1371,11 @@ protected override void BuildTargetModel(ModelBuilder modelBuilder) b.Property("ExpiresAt") .HasColumnType("timestamp with time zone"); + b.Property("IssuedEpoch") + .ValueGeneratedOnAdd() + .HasColumnType("integer") + .HasDefaultValue(0); + b.Property("ReplacedByTokenHash") .HasMaxLength(64) .HasColumnType("character varying(64)"); diff --git a/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs b/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs index e15ea31d8..5c2f5b91c 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs @@ -35,11 +35,31 @@ namespace Cluckwork.Infrastructure.Persistence.Migrations // — the snapshot has never recorded them — so EF always wrote those // columns explicitly and never relied on them. They are not // reproduced here; the only DB-level defaults that remain are the - // ones the model actually declares (refresh_tokens."RevokedByGrace"). + // ones the model actually declares (refresh_tokens."RevokedByGrace", + // and — folded in afterward, see the #364 note below — + // AspNetUsers."CredentialEpoch" and refresh_tokens."IssuedEpoch"). // Everything else — every table, column type/nullability/length, PK, FK // with its delete behaviour, unique and non-unique index (including the // four lower(Name) expression indexes hand-carried below), and the // Version concurrency tokens — is byte-identical. + // + // #364 — ADDENDUM, folded in the same way and for the same reason before + // this repo was ever deployed. Credential-epoch revocation added four + // columns (AspNetUsers.CredentialEpoch/DisabledAt/DisabledBy, + // refresh_tokens.IssuedEpoch). A virgin database was still the only + // starting state that existed anywhere, so these were hand-folded + // directly into this migration rather than shipped as a second one — + // the #245 reasoning applies again verbatim: no `__EFMigrationsHistory` + // anywhere needs baselining, so there was never anything to "add a + // migration" on top of. The original #364 patch also carried an + // `UPDATE refresh_tokens SET "RevokedAt" = now() WHERE "RevokedAt" IS + // NULL` cutover backfill (plus a `SET LOCAL lock_timeout` to bound the + // ADD COLUMN lock); both were dropped in the fold — the backfill always + // touches zero rows here, precisely the dead-backfill anti-pattern #245 + // squashed away, and the lock guard exists only to protect a rolling + // upgrade over pre-existing traffic that likewise cannot exist yet. + // MigrationSecurityReviewTests pins that this directory still holds + // exactly one migration. public partial class InitialCreate : Migration { /// @@ -106,7 +126,16 @@ protected override void Up(MigrationBuilder migrationBuilder) TwoFactorEnabled = table.Column(type: "boolean", nullable: false), LockoutEnd = table.Column(type: "timestamp with time zone", nullable: true), LockoutEnabled = table.Column(type: "boolean", nullable: false), - AccessFailedCount = table.Column(type: "integer", nullable: false) + AccessFailedCount = table.Column(type: "integer", nullable: false), + // #364 — credential-epoch revocation. CredentialEpoch starts at 1 and + // RefreshToken.IssuedEpoch (below) starts at 0, so a missing/malformed + // credential_epoch claim (which CredentialEpochMiddleware maps to 0) + // can never equal a live user's epoch. Load-bearing: do not change + // either default. DisabledAt/DisabledBy are schema-only here — no + // mutation sets them yet. + CredentialEpoch = table.Column(type: "integer", nullable: false, defaultValue: 1), + DisabledAt = table.Column(type: "timestamp with time zone", nullable: true), + DisabledBy = table.Column(type: "uuid", nullable: true) }, constraints: table => { @@ -370,7 +399,12 @@ protected override void Up(MigrationBuilder migrationBuilder) RevokedAt = table.Column(type: "timestamp with time zone", nullable: true), ReplacedByTokenHash = table.Column(type: "character varying(64)", maxLength: 64, nullable: true), RevokedByGrace = table.Column(type: "boolean", nullable: false, defaultValue: false), - ConcurrencyStamp = table.Column(type: "character varying(36)", maxLength: 36, nullable: false) + ConcurrencyStamp = table.Column(type: "character varying(36)", maxLength: 36, nullable: false), + // #364 — see the matching comment on AspNetUsers.CredentialEpoch above. + // Every known mint site stamps this explicitly to the user's current + // epoch; the 0 default is a defense-in-depth backstop for a writer + // that doesn't, and 0 is permanently invalid (no user is ever epoch 0). + IssuedEpoch = table.Column(type: "integer", nullable: false, defaultValue: 0) }, constraints: table => { diff --git a/src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs b/src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs index 3dc9c2b41..0a860c8d2 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs @@ -1274,6 +1274,17 @@ protected override void BuildModel(ModelBuilder modelBuilder) .IsConcurrencyToken() .HasColumnType("text"); + b.Property("CredentialEpoch") + .ValueGeneratedOnAdd() + .HasColumnType("integer") + .HasDefaultValue(1); + + b.Property("DisabledAt") + .HasColumnType("timestamp with time zone"); + + b.Property("DisabledBy") + .HasColumnType("uuid"); + b.Property("DisplayName") .HasColumnType("text"); @@ -1357,6 +1368,11 @@ protected override void BuildModel(ModelBuilder modelBuilder) b.Property("ExpiresAt") .HasColumnType("timestamp with time zone"); + b.Property("IssuedEpoch") + .ValueGeneratedOnAdd() + .HasColumnType("integer") + .HasDefaultValue(0); + b.Property("ReplacedByTokenHash") .HasMaxLength(64) .HasColumnType("character varying(64)"); diff --git a/tests/Cluckwork.Api.IntegrationTests/AdminRecoveryServiceTests.cs b/tests/Cluckwork.Api.IntegrationTests/AdminRecoveryServiceTests.cs index 1f62d25b4..d510e00b0 100644 --- a/tests/Cluckwork.Api.IntegrationTests/AdminRecoveryServiceTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/AdminRecoveryServiceTests.cs @@ -60,6 +60,7 @@ public async Task Recover_ResetsPassword_RevokesSessions_RotatesStamp_AndAuditsB Guid adminUserId; Guid adminAccountId; string originalStamp; + int originalCredentialEpoch; using (var s = Scope()) { var idp = s.ServiceProvider.GetRequiredService(); @@ -72,6 +73,7 @@ public async Task Recover_ResetsPassword_RevokesSessions_RotatesStamp_AndAuditsB adminUserId = admin.Id; adminAccountId = admin.AccountId; originalStamp = admin.SecurityStamp!; + originalCredentialEpoch = admin.CredentialEpoch; // Lock the account out — the exact state break-glass must recover from // (repeated failed logins → a 15-min lockout). LoginAsync checks @@ -125,6 +127,7 @@ public async Task Recover_ResetsPassword_RevokesSessions_RotatesStamp_AndAuditsB var db = s.ServiceProvider.GetRequiredService(); var admin = await db.Users.SingleAsync(u => u.Id == adminUserId); Assert.NotEqual(originalStamp, admin.SecurityStamp); + Assert.Equal(originalCredentialEpoch + 1, admin.CredentialEpoch); // AuditEvent carries the tenant query filter, so IgnoreQueryFilters is // required here — this scope never resolved a tenant. diff --git a/tests/Cluckwork.Api.IntegrationTests/CredentialEpochMiddlewareOrderTests.cs b/tests/Cluckwork.Api.IntegrationTests/CredentialEpochMiddlewareOrderTests.cs new file mode 100644 index 000000000..2f4ceae2b --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/CredentialEpochMiddlewareOrderTests.cs @@ -0,0 +1,88 @@ +namespace Cluckwork.Api.IntegrationTests; + +// #364 — the credential gate's position is a security property, not an +// incidental startup detail. Runtime endpoint tests pin the observable edges; +// this source-level fence pins the complete contiguous sequence so adding, +// duplicating, or reordering middleware cannot silently leave an untested gap. +public sealed class CredentialEpochMiddlewareOrderTests +{ + [Fact] + public void Program_PinsTheCompleteCredentialGateSequence() + { + var repository = FindRepositoryRoot(); + var program = File.ReadAllText(Path.Combine( + repository.FullName, "src", "Cluckwork.Api", "Program.cs")); + string[] expectedOrder = + [ + "app.UseAuthentication();", + "app.UseMiddleware();", + "app.UseMiddleware();", + "app.UseMiddleware();", + "app.UseAuthorization();", + "app.UseMiddleware();", + ]; + + var first = program.IndexOf(expectedOrder[0], StringComparison.Ordinal); + var last = program.IndexOf(expectedOrder[^1], StringComparison.Ordinal); + Assert.True(first >= 0, $"Program.cs is missing required middleware statement: {expectedOrder[0]}"); + Assert.True(last > first, $"Program.cs is missing or misorders: {expectedOrder[^1]}"); + + foreach (var statement in expectedOrder) + { + Assert.Equal(1, CountOccurrences(program, statement)); + } + + var securitySlice = program[first..(last + expectedOrder[^1].Length)]; + var actualOrder = ExecutableStatements(securitySlice); + Assert.Equal(expectedOrder, actualOrder); + } + + [Fact] + public void FenceIncludesAnyInterveningExecutableRegistration() + { + const string source = """ + app.UseAuthentication(); + // A comment is harmless. + app.MapWhen(_ => true, branch => branch.Run(_ => Task.CompletedTask)); + app.UseAuthorization(); + """; + + Assert.Equal( + [ + "app.UseAuthentication();", + "app.MapWhen(_ => true, branch => branch.Run(_ => Task.CompletedTask));", + "app.UseAuthorization();", + ], ExecutableStatements(source)); + } + + private static string[] ExecutableStatements(string source) => source.Split('\n') + .Select(line => line.Trim()) + .Where(line => line.Length > 0 && !line.StartsWith("//", StringComparison.Ordinal)) + .ToArray(); + + private static int CountOccurrences(string source, string value) + { + var count = 0; + var start = 0; + while ((start = source.IndexOf(value, start, StringComparison.Ordinal)) >= 0) + { + count++; + start += value.Length; + } + + return count; + } + + private static DirectoryInfo FindRepositoryRoot() + { + for (var directory = new DirectoryInfo(AppContext.BaseDirectory); + directory is not null; + directory = directory.Parent) + { + if (File.Exists(Path.Combine(directory.FullName, "Cluckwork.sln"))) + return directory; + } + + throw new DirectoryNotFoundException("Could not locate the Cluckwork repository root."); + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/CredentialEpochRaceTests.cs b/tests/Cluckwork.Api.IntegrationTests/CredentialEpochRaceTests.cs new file mode 100644 index 000000000..9fe06e1ce --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/CredentialEpochRaceTests.cs @@ -0,0 +1,344 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Data.Common; +using System.Net; +using Cluckwork.Api.IntegrationTests.Infrastructure; +using Cluckwork.Application.Common; +using Cluckwork.Infrastructure.Identity; +using Cluckwork.Infrastructure.Persistence; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.TestHost; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.Extensions.DependencyInjection; + +public sealed class CredentialEpochRaceFactory : CluckworkWebApplicationFactory +{ + public EpochReplayBarrierInterceptor Barrier { get; } = new(); + public CredentialResetBarrierInterceptor CredentialResetBarrier { get; } = new(); + public CountingPasswordHasher Hasher { get; } = new(); + + protected override void ConfigureWebHost(IWebHostBuilder builder) + { + base.ConfigureWebHost(builder); + builder.UseSetting("Jwt:RefreshReuseGraceSeconds", "0"); + builder.ConfigureTestServices(services => + { + services.AddSingleton>(Hasher); + services.AddDbContext((_, options) => + options.AddInterceptors(Barrier, CredentialResetBarrier)); + }); + } +} + +// Pauses the successful-password failed-count reset immediately before its +// UPDATE executes. The password has already been verified at this point. A +// concurrent password reset can then replace the credential epoch and security +// stamp, making Identity's UPDATE lose optimistic concurrency and reload the +// newer row into the request's tracked user. +public sealed class CredentialResetBarrierInterceptor : DbCommandInterceptor +{ + private TaskCompletionSource _reached = NewSignal(); + private TaskCompletionSource _release = NewSignal(); + private int _armed; + + public void Arm() + { + _reached = NewSignal(); + _release = NewSignal(); + Interlocked.Exchange(ref _armed, 1); + } + + public Task WaitUntilReachedAsync(CancellationToken ct) => _reached.Task.WaitAsync(ct); + + public void Release() => _release.TrySetResult(); + + public override async ValueTask> ReaderExecutingAsync( + DbCommand command, CommandEventData eventData, InterceptionResult result, + CancellationToken cancellationToken = default) + { + if (command.CommandText.Contains("UPDATE \"AspNetUsers\"", StringComparison.OrdinalIgnoreCase) + && command.CommandText.Contains("AccessFailedCount", StringComparison.Ordinal) + && Interlocked.CompareExchange(ref _armed, 0, 1) == 1) + { + _reached.TrySetResult(); + await _release.Task.WaitAsync(cancellationToken); + } + + return result; + } + + private static TaskCompletionSource NewSignal() => + new(TaskCreationOptions.RunContinuationsAsynchronously); +} + +// Pauses the replay-family UPDATE immediately before PostgreSQL executes it. +// By then RefreshAsync has loaded the user and accepted the presented epoch, +// which gives the test a deterministic window to bump the epoch and mint the +// replacement family before releasing the stale request. +public sealed class EpochReplayBarrierInterceptor : DbCommandInterceptor +{ + private TaskCompletionSource _reached = NewSignal(); + private TaskCompletionSource _release = NewSignal(); + private int _armed; + + public void Arm() + { + _reached = NewSignal(); + _release = NewSignal(); + Interlocked.Exchange(ref _armed, 1); + } + + public Task WaitUntilReachedAsync(CancellationToken ct) => _reached.Task.WaitAsync(ct); + + public void Release() => _release.TrySetResult(); + + public override async ValueTask> NonQueryExecutingAsync( + DbCommand command, CommandEventData eventData, InterceptionResult result, + CancellationToken cancellationToken = default) + { + await WaitIfArmedAsync(command, cancellationToken); + + return result; + } + + public override async ValueTask> ReaderExecutingAsync( + DbCommand command, CommandEventData eventData, InterceptionResult result, + CancellationToken cancellationToken = default) + { + await WaitIfArmedAsync(command, cancellationToken); + return result; + } + + private async Task WaitIfArmedAsync(DbCommand command, CancellationToken cancellationToken) + { + if (!command.CommandText.Contains("UPDATE refresh_tokens", StringComparison.OrdinalIgnoreCase) + || !command.CommandText.Contains("RevokedAt", StringComparison.Ordinal) + || Interlocked.CompareExchange(ref _armed, 0, 1) != 1) + return; + + _reached.TrySetResult(); + await _release.Task.WaitAsync(cancellationToken); + } + + private static TaskCompletionSource NewSignal() => + new(TaskCreationOptions.RunContinuationsAsynchronously); +} + +public sealed class CredentialEpochRaceTests(CredentialEpochRaceFactory factory) + : IClassFixture +{ + [Fact] + public async Task ActiveRefreshThatPassedTheOldEpochCheck_MintsAChildRejectedAfterTheBump() + { + var email = $"epoch-race-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var active = await factory.LoginAsync(email); + + factory.Barrier.Arm(); + var rotationTask = factory.CreateClient().PostRefreshAsync(active.RefreshToken); + using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + await factory.Barrier.WaitUntilReachedAsync(timeout.Token); + + try + { + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + user.CredentialEpoch++; + await db.SaveChangesAsync(); + }); + } + finally + { + factory.Barrier.Release(); + } + + var rotation = await rotationTask; + rotation.EnsureSuccessStatusCode(); + var child = await TestHarness.ReadTokensAsync(rotation); + + Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.CreateClient().PostRefreshAsync(child.RefreshToken)).StatusCode); + } + + [Fact] + public async Task ReplayThatPassedTheOldEpochCheck_CannotRevokeTheFreshEpochFamily() + { + var email = $"epoch-race-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var stale = await factory.LoginAsync(email); + await factory.WithTenantScopeAsync(accountId, async db => + { + var userId = await db.Users.Where(user => user.Email == email) + .Select(user => user.Id).SingleAsync(); + var staleRow = await db.RefreshTokens.SingleAsync(token => token.UserId == userId); + staleRow.RevokedAt = DateTimeOffset.UtcNow; + await db.SaveChangesAsync(); + }); + + factory.Barrier.Arm(); + var replayTask = factory.CreateClient().PostRefreshAsync(stale.RefreshToken); + using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + await factory.Barrier.WaitUntilReachedAsync(timeout.Token); + + TokenPairDto fresh; + try + { + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + user.CredentialEpoch++; + await db.SaveChangesAsync(); + }); + fresh = await factory.LoginAsync(email); + } + finally + { + factory.Barrier.Release(); + } + + Assert.Equal(HttpStatusCode.Unauthorized, (await replayTask).StatusCode); + Assert.Equal(HttpStatusCode.OK, + (await factory.CreateClient().PostRefreshAsync(fresh.RefreshToken)).StatusCode); + } + + [Fact] + public async Task LoginThatVerifiedTheOldPassword_CannotAdoptAConcurrentPasswordReset() + { + var email = $"login-proof-race-{Guid.NewGuid():N}@test.local"; + var rotatedPassword = TemporaryPassword.Generate(); + var accountId = await factory.SeedAccountWithUserAsync(email); + await PrimeFailedCountAsync(email); + + factory.CredentialResetBarrier.Arm(); + var loginTask = factory.TryLoginAsync(email, TestHarness.Password); + using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + await factory.CredentialResetBarrier.WaitUntilReachedAsync(timeout.Token); + + try + { + await RotatePasswordAsync(accountId, email, rotatedPassword); + } + finally + { + factory.CredentialResetBarrier.Release(); + } + + Assert.Equal(HttpStatusCode.Unauthorized, (await loginTask).StatusCode); + Assert.Equal(HttpStatusCode.OK, + (await factory.TryLoginAsync(email, rotatedPassword)).StatusCode); + } + + [Fact] + public async Task StepUpThatVerifiedTheOldPassword_CannotAdoptAConcurrentPasswordReset() + { + var email = $"step-up-proof-race-{Guid.NewGuid():N}@test.local"; + var rotatedPassword = TemporaryPassword.Generate(); + var accountId = await factory.SeedAccountWithUserAsync(email); + await PrimeFailedCountAsync(email); + + using var scope = factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService().Resolve(accountId); + var db = scope.ServiceProvider.GetRequiredService(); + var userId = await db.Users.Where(user => user.Email == email) + .Select(user => user.Id).SingleAsync(); + var stepUp = scope.ServiceProvider.GetRequiredService(); + + factory.CredentialResetBarrier.Arm(); + var issueTask = stepUp.IssueAsync(accountId, userId, TestHarness.Password); + using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + await factory.CredentialResetBarrier.WaitUntilReachedAsync(timeout.Token); + + try + { + await RotatePasswordAsync(accountId, email, rotatedPassword); + } + finally + { + factory.CredentialResetBarrier.Release(); + } + + var issued = await issueTask; + Assert.True(issued.IsFailure); + Assert.Equal("Users.CurrentPasswordIncorrect", issued.Error.Code); + } + + [Theory] + [InlineData("wrong-password")] + [InlineData(TestHarness.Password)] + public async Task DisabledLogin_PerformsOneFullHashWithoutMutatingLockout(string password) + { + var email = $"disabled-login-cost-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + await DisableAsync(accountId, email); + factory.Hasher.Reset(); + + var response = await factory.TryLoginAsync(email, password); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + Assert.Equal(1, factory.Hasher.VerifyCount); + await AssertLockoutUntouchedAsync(accountId, email); + } + + [Theory] + [InlineData("wrong-password")] + [InlineData(TestHarness.Password)] + public async Task DisabledStepUp_PerformsOneFullHashWithoutMutatingLockout(string password) + { + var email = $"disabled-step-up-cost-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + await DisableAsync(accountId, email); + using var scope = factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService().Resolve(accountId); + var db = scope.ServiceProvider.GetRequiredService(); + var userId = await db.Users.Where(user => user.Email == email) + .Select(user => user.Id).SingleAsync(); + var stepUp = scope.ServiceProvider.GetRequiredService(); + factory.Hasher.Reset(); + + var result = await stepUp.IssueAsync(accountId, userId, password); + + Assert.True(result.IsFailure); + Assert.Equal("Users.CurrentPasswordIncorrect", result.Error.Code); + Assert.Equal(1, factory.Hasher.VerifyCount); + await AssertLockoutUntouchedAsync(accountId, email); + } + + private async Task PrimeFailedCountAsync(string email) + { + Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.TryLoginAsync(email, "wrong-password")).StatusCode); + } + + private Task RotatePasswordAsync(Guid accountId, string email, string rotatedPassword) => + factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + user.PasswordHash = new PasswordHasher() + .HashPassword(user, rotatedPassword); + user.CredentialEpoch++; + user.SecurityStamp = Guid.NewGuid().ToString(); + user.ConcurrencyStamp = Guid.NewGuid().ToString(); + await db.SaveChangesAsync(); + }); + + private Task DisableAsync(Guid accountId, string email) => + factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + user.DisabledAt = DateTimeOffset.UtcNow; + await db.SaveChangesAsync(); + }); + + private async Task AssertLockoutUntouchedAsync(Guid accountId, string email) + { + var state = await factory.WithTenantScopeAsync(accountId, async db => await db.Users + .Where(user => user.Email == email) + .Select(user => new { user.AccessFailedCount, user.LockoutEnd }) + .SingleAsync()); + Assert.Equal(0, state.AccessFailedCount); + Assert.Null(state.LockoutEnd); + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/CredentialEpochTests.cs b/tests/Cluckwork.Api.IntegrationTests/CredentialEpochTests.cs new file mode 100644 index 000000000..f6a517d60 --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/CredentialEpochTests.cs @@ -0,0 +1,413 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Net; +using System.IdentityModel.Tokens.Jwt; +using System.Security.Claims; +using System.Security.Cryptography; +using System.Text; +using Cluckwork.Api.Endpoints.Auth; +using Cluckwork.Api.IntegrationTests.Infrastructure; +using Cluckwork.Application.Common; +using Cluckwork.Domain.Accounts; +using Cluckwork.Infrastructure.Identity; +using Microsoft.AspNetCore.Mvc; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.IdentityModel.Tokens; + +[Collection(IntegrationCollection.Name)] +public sealed class CredentialEpochTests(CluckworkWebApplicationFactory factory) +{ + [Fact] + public async Task ChangeOwnPassword_RejectsTheAccessTokenIssuedBeforeTheCredentialEpochBump() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + await factory.SeedAccountWithUserAsync(email); + var beforeReset = await factory.LoginAsync(email); + var client = factory.CreateAuthedClient(beforeReset.AccessToken); + + var response = await client.PostAsJsonAsync("/api/v1/auth/change-password", new + { + currentPassword = TestHarness.Password, + newPassword = CreatePassword(), + }); + response.EnsureSuccessStatusCode(); + var afterReset = await TestHarness.ReadTokensAsync(response); + + var staleClient = factory.CreateAuthedClient(beforeReset.AccessToken); + var staleResponse = await staleClient.GetAsync("/api/v1/users"); + + Assert.Equal(HttpStatusCode.Unauthorized, staleResponse.StatusCode); + Assert.Equal("Auth.CredentialsSuperseded", (await staleResponse.Content + .ReadFromJsonAsync())!.Title); + + // Logout intentionally remains reachable for a stale bearer so the SPA + // can clear its cookie and local state after the 401. + Assert.Equal(HttpStatusCode.NoContent, (await staleClient.PostLogoutAsync( + beforeReset.RefreshToken, accessToken: beforeReset.AccessToken)).StatusCode); + + Assert.Equal(HttpStatusCode.OK, (await factory.CreateAuthedClient(afterReset.AccessToken) + .GetAsync("/api/v1/users")).StatusCode); + } + + [Fact] + public async Task AccessTokenWithoutCredentialEpoch_IsRejected() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var userId = await UserIdForAsync(accountId, email); + + var response = await factory.CreateAuthedClient(CreateAccessToken(userId, accountId, null)) + .GetAsync("/api/v1/users"); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + } + + [Fact] + public async Task AccessTokenWithMalformedCredentialEpoch_IsRejected() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var userId = await UserIdForAsync(accountId, email); + + var response = await factory.CreateAuthedClient(CreateAccessToken(userId, accountId, "not-an-epoch")) + .GetAsync("/api/v1/users"); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + } + + [Fact] + public async Task AccessTokenCannotCrossTheUserAccountBoundary() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var userId = await UserIdForAsync(accountId, email); + var foreignAccountId = await factory.SeedAccountWithUserAsync( + $"epoch-{Guid.NewGuid():N}@test.local"); + + var response = await factory.CreateAuthedClient(CreateAccessToken( + userId, foreignAccountId, "1")) + .GetAsync("/api/v1/users"); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + } + + [Fact] + public async Task PreviousEpochRefreshReplay_DoesNotRevokeTheFreshEpochSession() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + await factory.SeedAccountWithUserAsync(email); + var beforeReset = await factory.LoginAsync(email); + var client = factory.CreateAuthedClient(beforeReset.AccessToken); + + var passwordChange = await client.PostAsJsonAsync("/api/v1/auth/change-password", new + { + currentPassword = TestHarness.Password, + newPassword = CreatePassword(), + }); + passwordChange.EnsureSuccessStatusCode(); + var fresh = await TestHarness.ReadTokensAsync(passwordChange); + + // The old token is already revoked by the password change. It must fail + // before the reuse detector, rather than revoking this new epoch's token. + Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.CreateClient().PostRefreshAsync(beforeReset.RefreshToken)).StatusCode); + Assert.Equal(HttpStatusCode.OK, + (await factory.CreateClient().PostRefreshAsync(fresh.RefreshToken)).StatusCode); + } + + [Fact] + public async Task GraceReplacementWrittenByAPreEpochReplica_IsRejectedInertly() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var presented = await factory.LoginAsync(email); + var presentedRowId = await factory.WithTenantScopeAsync(accountId, async db => + { + var userId = await db.Users.Where(user => user.Email == email) + .Select(user => user.Id).SingleAsync(); + return await db.RefreshTokens.Where(token => token.UserId == userId) + .Select(token => token.Id).SingleAsync(); + }); + var currentEpochSibling = await factory.LoginAsync(email); + var replacementRaw = Convert.ToBase64String(RandomNumberGenerator.GetBytes(32)); + var replacementHash = HashToken(replacementRaw); + + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + var presentedRow = await db.RefreshTokens.SingleAsync(token => token.Id == presentedRowId); + var now = DateTimeOffset.UtcNow; + + presentedRow.RevokedAt = now; + presentedRow.ReplacedByTokenHash = replacementHash; + presentedRow.ConcurrencyStamp = Guid.NewGuid().ToString(); + db.RefreshTokens.Add(new RefreshToken + { + Id = Guid.NewGuid(), + UserId = user.Id, + AccountId = accountId, + TokenHash = replacementHash, + IssuedEpoch = 0, + CreatedAt = now, + ExpiresAt = now.AddDays(1), + }); + await db.SaveChangesAsync(); + }); + + var response = await factory.CreateClient().PostRefreshAsync(presented.RefreshToken); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + var revokedAt = await factory.WithTenantScopeAsync(accountId, async db => await db.RefreshTokens + .Where(token => token.TokenHash == replacementHash) + .Select(token => token.RevokedAt) + .SingleAsync()); + Assert.Null(revokedAt); + Assert.Equal(HttpStatusCode.OK, + (await factory.CreateClient().PostRefreshAsync(currentEpochSibling.RefreshToken)).StatusCode); + } + + [Fact] + public async Task RetiredEpochRefresh_IsRejectedInertly() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var currentEpochSibling = await factory.LoginAsync(email); + var retiredRaw = Convert.ToBase64String(RandomNumberGenerator.GetBytes(32)); + var retiredHash = HashToken(retiredRaw); + + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + var now = DateTimeOffset.UtcNow; + db.RefreshTokens.Add(new RefreshToken + { + Id = Guid.NewGuid(), + UserId = user.Id, + AccountId = accountId, + TokenHash = retiredHash, + // Deliberately omit IssuedEpoch: the database default must + // produce permanently retired epoch zero. + CreatedAt = now, + ExpiresAt = now.AddDays(1), + }); + await db.SaveChangesAsync(); + }); + + var response = await factory.CreateClient().PostRefreshAsync(retiredRaw); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + var retired = await factory.WithTenantScopeAsync(accountId, async db => await db.RefreshTokens + .Where(token => token.TokenHash == retiredHash) + .Select(token => new { token.IssuedEpoch, token.RevokedAt }) + .SingleAsync()); + Assert.Equal(0, retired.IssuedEpoch); + Assert.Null(retired.RevokedAt); + Assert.Equal(HttpStatusCode.OK, + (await factory.CreateClient().PostRefreshAsync(currentEpochSibling.RefreshToken)).StatusCode); + } + + [Fact] + public async Task DisabledUser_CannotLoginRefreshOrObtainAStepUpGrant() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var tokens = await factory.LoginAsync(email); + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(user => user.Email == email); + user.DisabledAt = DateTimeOffset.UtcNow; + await db.SaveChangesAsync(); + }); + + var login = await factory.TryLoginAsync(email, TestHarness.Password); + Assert.Equal(HttpStatusCode.Unauthorized, login.StatusCode); + Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.CreateClient().PostRefreshAsync(tokens.RefreshToken)).StatusCode); + var stepUp = await factory.CreateAuthedClient(tokens.AccessToken) + .PostAsJsonAsync("/api/v1/auth/step-up", new { password = TestHarness.Password }); + Assert.Equal(HttpStatusCode.Unauthorized, stepUp.StatusCode); + Assert.Equal("Auth.AccountDisabled", (await stepUp.Content + .ReadFromJsonAsync())!.Title); + } + + [Fact] + public async Task DisabledLogin_DoesNotRevealPasswordValidityOrMutateLockoutState() + { + var disabledEmail = $"epoch-{Guid.NewGuid():N}@test.local"; + var disabledAccountId = await factory.SeedAccountWithUserAsync(disabledEmail); + await factory.WithTenantScopeAsync(disabledAccountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == disabledEmail); + user.DisabledAt = DateTimeOffset.UtcNow; + await db.SaveChangesAsync(); + }); + var enabledEmail = $"epoch-{Guid.NewGuid():N}@test.local"; + await factory.SeedAccountWithUserAsync(enabledEmail); + + using var scope = factory.Services.CreateScope(); + var identity = scope.ServiceProvider.GetRequiredService(); + var disabledWrong = await identity.LoginAsync(disabledEmail, CreatePassword()); + var disabledCorrect = await identity.LoginAsync(disabledEmail, TestHarness.Password); + var enabledWrong = await identity.LoginAsync(enabledEmail, CreatePassword()); + + Assert.True(disabledWrong.IsFailure); + Assert.True(disabledCorrect.IsFailure); + Assert.True(enabledWrong.IsFailure); + Assert.Equal(enabledWrong.Error, disabledWrong.Error); + Assert.Equal(enabledWrong.Error, disabledCorrect.Error); + var lockoutState = await factory.WithTenantScopeAsync(disabledAccountId, async db => await db.Users + .Where(user => user.Email == disabledEmail) + .Select(user => new { user.AccessFailedCount, user.LockoutEnd }) + .SingleAsync()); + Assert.Equal(0, lockoutState.AccessFailedCount); + Assert.Null(lockoutState.LockoutEnd); + } + + [Fact] + public async Task DisabledUser_AccessTokenIsRejectedWithTheDisabledReason() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var tokens = await factory.LoginAsync(email); + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + user.DisabledAt = DateTimeOffset.UtcNow; + await db.SaveChangesAsync(); + }); + + var response = await factory.CreateAuthedClient(tokens.AccessToken).GetAsync("/api/v1/users"); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + Assert.Equal("Auth.AccountDisabled", (await response.Content + .ReadFromJsonAsync())!.Title); + } + + [Fact] + public async Task SupersededWriteIsRejectedBeforeIdempotencyRequiresAKey() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var tokens = await factory.LoginAsync(email); + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + user.CredentialEpoch++; + await db.SaveChangesAsync(); + }); + + // Deliberately omit Idempotency-Key. The credential gate must answer + // first, so a rejected write cannot consume or demand a key. + var response = await factory.CreateAuthedClient(tokens.AccessToken) + .PostAsJsonAsync("/api/v1/expense-categories", new { name = "Must not run" }); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + var problem = (await response.Content.ReadFromJsonAsync())!; + Assert.Equal("Auth.CredentialsSuperseded", problem.Title); + Assert.DoesNotContain("Idempotency-Key", problem.Detail ?? string.Empty, StringComparison.Ordinal); + } + + [Fact] + public async Task SupersededBearer_CanReachTheTrailingSlashLogoutRoute() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var tokens = await factory.LoginAsync(email); + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + user.CredentialEpoch++; + await db.SaveChangesAsync(); + }); + var request = new HttpRequestMessage(HttpMethod.Post, "/api/v1/auth/logout/"); + request.Headers.Add(AuthCookies.CsrfHeaderName, "1"); + request.Headers.Add("Cookie", $"{AuthCookies.RefreshCookieName}={tokens.RefreshToken}"); + + var response = await factory.CreateAuthedClient(tokens.AccessToken).SendAsync(request); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + } + + [Fact] + public async Task SupersededPendingUser_IsRejectedBeforeTheMustChangePasswordGate() + { + var accountId = Guid.NewGuid(); + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + await factory.WithTenantScopeAsync(accountId, async db => + { + db.Accounts.Add(Account.Create(accountId, "Epoch Gate Farm", "UTC", "USD")); + await db.SaveChangesAsync(); + }); + await factory.SeedUserPendingPasswordChangeAsync(accountId, email); + var accessToken = await factory.LoginForAccessTokenAsync(email); + await factory.WithTenantScopeAsync(accountId, async db => + { + var user = await db.Users.SingleAsync(candidate => candidate.Email == email); + user.CredentialEpoch++; + await db.SaveChangesAsync(); + }); + + var response = await factory.CreateAuthedClient(accessToken).GetAsync("/api/v1/flocks"); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + Assert.Equal("Auth.CredentialsSuperseded", (await response.Content + .ReadFromJsonAsync())!.Title); + } + + [Fact] + public async Task Login_StampsRefreshTokenAndAccessTokenWithTheCurrentEpoch() + { + var email = $"epoch-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var tokens = await factory.LoginAsync(email); + + var user = await factory.WithTenantScopeAsync(accountId, async db => await db.Users + .Where(candidate => candidate.Email == email) + .Select(candidate => new { candidate.Id, candidate.CredentialEpoch }) + .SingleAsync()); + var issuedEpoch = await factory.WithTenantScopeAsync(accountId, async db => await db.RefreshTokens + .Where(token => token.UserId == user.Id) + .Select(token => token.IssuedEpoch) + .SingleAsync()); + var accessEpoch = new JwtSecurityTokenHandler().ReadJwtToken(tokens.AccessToken) + .Claims.Single(claim => claim.Type == "credential_epoch").Value; + + Assert.Equal(user.CredentialEpoch, issuedEpoch); + Assert.Equal(user.CredentialEpoch.ToString(System.Globalization.CultureInfo.InvariantCulture), accessEpoch); + } + + private Task UserIdForAsync(Guid accountId, string email) => + factory.WithTenantScopeAsync(accountId, async db => + await db.Users.Where(user => user.Email == email).Select(user => user.Id).SingleAsync()); + + private static string CreateAccessToken(Guid userId, Guid accountId, string? credentialEpoch) + { + using var rsa = RSA.Create(); + rsa.ImportFromPem(TestJwtKeys.PrivateKeyPem.Replace("\\n", "\n", StringComparison.Ordinal)); + var claims = new List + { + new(JwtRegisteredClaimNames.Sub, userId.ToString()), + new("account_id", accountId.ToString()), + new("role", Roles.Owner), + }; + if (credentialEpoch is not null) + claims.Add(new Claim("credential_epoch", credentialEpoch)); + + var credentials = new SigningCredentials(new RsaSecurityKey(rsa), SecurityAlgorithms.RsaSha256) + { + CryptoProviderFactory = new CryptoProviderFactory { CacheSignatureProviders = false }, + }; + var token = new JwtSecurityToken( + "cluckwork-test", "cluckwork-api-test", claims, + DateTime.UtcNow.AddMinutes(-1), DateTime.UtcNow.AddMinutes(5), + credentials); + return new JwtSecurityTokenHandler().WriteToken(token); + } + + private static string CreatePassword() => + $"Aa1!{Convert.ToHexString(System.Security.Cryptography.RandomNumberGenerator.GetBytes(16))}"; + + private static string HashToken(string token) => Convert.ToHexString( + SHA256.HashData(Encoding.UTF8.GetBytes(token))).ToLowerInvariant(); +} diff --git a/tests/Cluckwork.Api.IntegrationTests/ExceptionHandlerReExecutionTests.cs b/tests/Cluckwork.Api.IntegrationTests/ExceptionHandlerReExecutionTests.cs index 6eab89fa3..e47e1b8ed 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ExceptionHandlerReExecutionTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ExceptionHandlerReExecutionTests.cs @@ -22,9 +22,13 @@ namespace Cluckwork.Api.IntegrationTests; // request under this TestServer, so a suite built on them flakes. internal sealed class ThrowingChangeOwnPasswordValidator : AbstractValidator { - public ThrowingChangeOwnPasswordValidator() => - RuleFor(c => c.NewPassword).Custom((_, _) => - throw new InvalidOperationException("change-password fault")); + public ThrowingChangeOwnPasswordValidator(AppDbContext db) => + RuleFor(c => c.NewPassword).CustomAsync(async (_, _, ct) => + { + await db.Users.ExecuteUpdateAsync(setters => setters + .SetProperty(user => user.CredentialEpoch, user => user.CredentialEpoch + 1), ct); + throw new InvalidOperationException("change-password fault"); + }); } internal sealed class ThrowingCreateExpenseValidator : AbstractValidator @@ -143,6 +147,7 @@ public async Task Exempt_auth_route_that_throws_returns_the_mapped_status_not_an Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); Assert.Contains(ProblemDetails500, body, StringComparison.Ordinal); Assert.DoesNotContain("idempotency-key-required", body, StringComparison.Ordinal); + Assert.DoesNotContain("Auth.CredentialsSuperseded", body, StringComparison.Ordinal); } // (b) CHARACTERIZATION, not a mutant-killer: a plain idempotency-TRACKED diff --git a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs index 8f151598c..c6af20487 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs @@ -70,6 +70,23 @@ private static bool LooksHashShaped(string value) => // shape a migrationBuilder.Sql(...) credential would actually take. private static readonly Regex SqlStringLiteral = new(@"'([^']*)'", RegexOptions.Compiled); + // #245's whole contract is "exactly one migration, InitialCreate" — the + // application has never been deployed, so a virgin database is the only + // starting state that exists anywhere, and there is therefore never a + // reason to add a second migration file instead of hand-folding a change + // into InitialCreate (see that migration's own header comment, most + // recently its #364 addendum). This is the automated fence: a PR that + // adds e.g. "AddFoo.cs" beside InitialCreate.cs now fails here instead of + // only getting flagged in review — which is exactly what happened to the + // migration this test itself replaces (PR #399 shipped a second + // migration; this guard did not yet exist to catch it). + [Fact] + public void ExactlyOneMigrationExists_AndItIsInitialCreate() + { + var migration = Assert.Single(AllMigrations()); + Assert.Equal("InitialCreate", migration.GetType().Name); + } + [Fact] public void NoMigration_EverInsertsIntoTheUsersTable() { diff --git a/tests/Cluckwork.Api.IntegrationTests/MustChangePasswordGateTests.cs b/tests/Cluckwork.Api.IntegrationTests/MustChangePasswordGateTests.cs index c91760414..d7bdc0e47 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MustChangePasswordGateTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MustChangePasswordGateTests.cs @@ -2,6 +2,7 @@ namespace Cluckwork.Api.IntegrationTests; using System.Net; using System.Net.Http.Headers; +using Cluckwork.Api.Endpoints.Auth; using Cluckwork.Api.IntegrationTests.Infrastructure; using Cluckwork.Application.Common; using Cluckwork.Domain.Accounts; @@ -138,11 +139,33 @@ await factory.WithTenantScopeAsync(secondAccountId, async db => }); await factory.SeedUserPendingPasswordChangeAsync(secondAccountId, secondEmail); var secondTokens = await factory.LoginAsync(secondEmail); - var anonClient = factory.CreateClient(); - var logoutResponse = await anonClient.PostLogoutAsync(secondTokens.RefreshToken); + var logoutResponse = await factory.CreateClient().PostLogoutAsync( + secondTokens.RefreshToken, accessToken: secondTokens.AccessToken); Assert.Equal(HttpStatusCode.NoContent, logoutResponse.StatusCode); } + [Fact] + public async Task PendingUser_CanReachTheTrailingSlashLogoutRoute() + { + var accountId = Guid.NewGuid(); + var email = $"pending-{Guid.NewGuid():N}@test.local"; + await factory.WithTenantScopeAsync(accountId, async db => + { + db.Accounts.Add(Account.Create(accountId, "Gate Farm", "UTC", "USD")); + await db.SaveChangesAsync(); + }); + await factory.SeedUserPendingPasswordChangeAsync(accountId, email); + var tokens = await factory.LoginAsync(email); + var request = new HttpRequestMessage(HttpMethod.Post, "/api/v1/auth/logout/"); + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", tokens.AccessToken); + request.Headers.Add(AuthCookies.CsrfHeaderName, "1"); + request.Headers.Add("Cookie", $"{AuthCookies.RefreshCookieName}={tokens.RefreshToken}"); + + var response = await factory.CreateClient().SendAsync(request); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + } + [Fact] public async Task ChangingPassword_ClearsTheFlag_AndUnblocksOrdinaryEndpoints() { diff --git a/tests/Cluckwork.Api.IntegrationTests/RetryBoundaryTests.cs b/tests/Cluckwork.Api.IntegrationTests/RetryBoundaryTests.cs index 05854ea6d..42635895b 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RetryBoundaryTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RetryBoundaryTests.cs @@ -297,6 +297,41 @@ await AccountLockout.RecordFailedAccessAsync( Assert.Equal(2, await FailedAccessCountAsync(accountId, email)); } + // A disable racing a wrong-password attempt also invalidates the tracked + // ConcurrencyStamp. Unlike another wrong-password writer, however, the + // freshly reloaded row is no longer eligible for lockout accounting: guesses + // made while an account is disabled must not leave it locked when an Owner + // later re-enables it. The retry loop therefore stops when reload observes + // DisabledAt instead of applying the stale attempt to the newer state. + [Fact] + public async Task ConcurrentDisable_StopsTheFailedAccessRetry_WithoutMutatingLockoutState() + { + var email = $"retry-boundary-{Guid.NewGuid():N}@test.local"; + var accountId = await _factory.SeedAccountWithUserAsync(email); + + using var failingAttempt = _factory.Services.CreateScope(); + using var disabler = _factory.Services.CreateScope(); + var failingUsers = failingAttempt.ServiceProvider.GetRequiredService>(); + var disablingUsers = disabler.ServiceProvider.GetRequiredService>(); + + // The failed attempt reads first. Disabling through a different context + // then supersedes its ConcurrencyStamp before AccessFailedAsync saves. + var staleUser = await failingUsers.FindByEmailAsync(email); + var userToDisable = await disablingUsers.FindByEmailAsync(email); + Assert.NotNull(staleUser); + Assert.NotNull(userToDisable); + + userToDisable.DisabledAt = DateTimeOffset.UtcNow; + Assert.True((await disablingUsers.UpdateAsync(userToDisable)).Succeeded); + + await AccountLockout.RecordFailedAccessAsync( + failingUsers, + failingAttempt.ServiceProvider.GetRequiredService(), + staleUser); + + Assert.Equal(0, await FailedAccessCountAsync(accountId, email)); + } + // --- codex 3696925253 (P2), and the round-5 sweep it opened --- // // THE SHAPE, stated once for the four tests below: an automatic replay diff --git a/tests/Cluckwork.Api.IntegrationTests/StepUpAuthTests.cs b/tests/Cluckwork.Api.IntegrationTests/StepUpAuthTests.cs index b120d3a8a..b4ec9f0e5 100644 --- a/tests/Cluckwork.Api.IntegrationTests/StepUpAuthTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/StepUpAuthTests.cs @@ -252,20 +252,26 @@ public async Task CreateOwner_StepUpRevokedBySecurityStampChange_Is403_AndNoOwne var grant = await StepUpAsync(owner, TestHarness.Password); - // Rotates the SecurityStamp. The caller's (old) access token stays - // valid regardless (no server-side denylist — the realistic "stolen - // token" shape this guard exists for), but the grant embeds the - // stamp as it was AT ISSUANCE. + // Rotates the SecurityStamp. JWT validation never checks SecurityStamp + // (it isn't embedded in the access token), so the token used for THIS + // call keeps authenticating it regardless — the realistic "stolen + // token" shape this guard exists for. Since #364 this same + // change-password call also bumps CredentialEpoch, so a captured OLD + // token would fail CredentialEpochMiddleware on its next request + // either way; the grant is what closes the narrower gap before that, + // since it separately embeds and checks the stamp AT ISSUANCE. var changed = await owner.PostWithKeyAsync("/api/v1/auth/change-password", Guid.NewGuid().ToString(), new { currentPassword = TestHarness.Password, newPassword = FreshPassword() }); Assert.Equal(HttpStatusCode.OK, changed.StatusCode); + var ownerAfterPasswordChange = factory.CreateAuthedClient( + (await TestHarness.ReadTokensAsync(changed)).AccessToken); var newOwnerEmail = $"boss-{Guid.NewGuid():N}@test.local"; - var response = await CreateUserWithStepUpAsync(owner, newOwnerEmail, "Admin", grant.Token); + var response = await CreateUserWithStepUpAsync(ownerAfterPasswordChange, newOwnerEmail, "Admin", grant.Token); Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); - var users = await owner.GetFromJsonAsync>("/api/v1/users"); + var users = await ownerAfterPasswordChange.GetFromJsonAsync>("/api/v1/users"); Assert.DoesNotContain(users!, u => u.Email == newOwnerEmail); } @@ -749,6 +755,26 @@ public bool IsRevokedByLogout(Guid userId, DateTimeOffset issuedAt) return (stepUp, db, accountId, user!.Id); } + [Fact] + public async Task IssueAsync_DirectlyRefusesADisabledUser() + { + using var scope = factory.Services.CreateScope(); + var (stepUp, db, accountId, userId) = await DirectStepUpServiceAsync( + scope, new InMemoryStepUpGrantRegistry()); + await using var _ = db; + var userManager = scope.ServiceProvider.GetRequiredService>(); + var user = await userManager.FindByIdAsync(userId.ToString()); + Assert.NotNull(user); + user.DisabledAt = DateTimeOffset.UtcNow; + var disabled = await userManager.UpdateAsync(user); + Assert.True(disabled.Succeeded); + + var result = await stepUp.IssueAsync(accountId, userId, TestHarness.Password); + + Assert.True(result.IsFailure); + Assert.Equal("Users.CurrentPasswordIncorrect", result.Error.Code); + } + [Fact] public async Task ValidateAsync_MakesTheAdmissionDecisionInOneAtomicRegistryCall() { diff --git a/tests/Cluckwork.Api.IntegrationTests/UserPasswordTests.cs b/tests/Cluckwork.Api.IntegrationTests/UserPasswordTests.cs index b9a864934..3161ddf54 100644 --- a/tests/Cluckwork.Api.IntegrationTests/UserPasswordTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/UserPasswordTests.cs @@ -3,6 +3,7 @@ namespace Cluckwork.Api.IntegrationTests; using System.Net; using System.Net.Http.Json; using Cluckwork.Api.IntegrationTests.Infrastructure; +using Microsoft.EntityFrameworkCore; // #165 — password management. Two halves with different rules: an Owner sets // another user's password without the current one, and any signed-in user @@ -63,6 +64,8 @@ public async Task AdminSet_RevokesTheTargetsExistingSessions() { var (admin, accountId) = await AdminAsync(); var (email, id) = await SeedWorkerAsync(admin, accountId); + var epochBeforeReset = await factory.WithTenantScopeAsync(accountId, async db => + await db.Users.Where(user => user.Id == id).Select(user => user.CredentialEpoch).SingleAsync()); // The worker is signed in BEFORE the reset — this is the session a reset // has to evict (otherwise a compromised session survives the reset). @@ -78,6 +81,8 @@ public async Task AdminSet_RevokesTheTargetsExistingSessions() var reset = await admin.PutWithKeyAsync($"/api/v1/users/{id}/password", Guid.NewGuid().ToString(), new { newPassword = FreshPassword() }); Assert.Equal(HttpStatusCode.NoContent, reset.StatusCode); + Assert.Equal(epochBeforeReset + 1, await factory.WithTenantScopeAsync(accountId, async db => + await db.Users.Where(user => user.Id == id).Select(user => user.CredentialEpoch).SingleAsync())); // The worker's CURRENT token is dead: no new access tokens for the old holder. Assert.Equal(HttpStatusCode.Unauthorized, @@ -125,10 +130,13 @@ public async Task SelfChange_NeedsNoIdempotencyKey_AndIsNotReplayedFromCache() // The rotated cookie really is on the response (a cached replay would omit it). Assert.Contains(first.Headers.GetValues("Set-Cookie"), c => c.StartsWith("cluckwork_rt=", StringComparison.Ordinal)); + var rotated = await TestHarness.ReadTokensAsync(first); - // Repeating the identical request is NOT served from cache — it re-executes - // and now fails, because the current password has changed. - var repeat = await client.PostAsJsonAsync("/api/v1/auth/change-password", + // Use the newly issued access token: the original one is deliberately + // retired by the credential epoch. This still proves the response was + // not cached — the handler re-executes and the old current password fails. + var repeat = await factory.CreateAuthedClient(rotated.AccessToken) + .PostAsJsonAsync("/api/v1/auth/change-password", new { currentPassword = TestHarness.Password, newPassword }); Assert.Equal(HttpStatusCode.BadRequest, repeat.StatusCode); } diff --git a/tools/simulation/k6/package.json b/tools/simulation/k6/package.json new file mode 100644 index 000000000..e986b24bb --- /dev/null +++ b/tools/simulation/k6/package.json @@ -0,0 +1,4 @@ +{ + "private": true, + "type": "module" +} diff --git a/tools/simulation/ui/specs/worker.spec.ts b/tools/simulation/ui/specs/worker.spec.ts index 42bc2f7fb..81061e7cc 100644 --- a/tools/simulation/ui/specs/worker.spec.ts +++ b/tools/simulation/ui/specs/worker.spec.ts @@ -35,6 +35,7 @@ // defect being fixed here.) import { expect, test } from "../src/fixtures"; +import type { Page } from "@playwright/test"; import { restrictedWorker, unrestrictedWorker } from "../src/cast"; import { farmToday } from "../src/farm"; import { tEn } from "../src/i18n"; @@ -44,6 +45,12 @@ import { selectOptionContaining } from "../src/dom"; const ASSIGNED_FLOCK = "Sim House A"; /** The flock it deliberately leaves unassigned, so the narrowing is genuine. */ const UNASSIGNED_FLOCK = "Sim House B"; +const GRADES = ["Small", "Medium", "Large", "Jumbo", "Seconds"]; + +async function clearSeededGrades(page: Page) { + for (const grade of GRADES) + await page.getByLabel(grade, { exact: true }).fill("0"); +} test.describe("Worker", () => { test("records a daily entry by grade on an assigned flock", async ({ page, signIn, farm }) => { @@ -63,6 +70,8 @@ test.describe("Worker", () => { await page.getByLabel(tEn("dailyEntry:totalEggsLabel"), { exact: true }).fill("120"); await page.getByLabel(tEn("dailyEntry:crackedLabel"), { exact: true }).fill("2"); await page.getByLabel(tEn("dailyEntry:mortalityLabel"), { exact: true }).fill("1"); + await clearSeededGrades(page); + await page.getByLabel("Small", { exact: true }).fill("114"); await page.getByRole("button", { name: tEn("dailyEntry:saveDraftButton") }).click(); @@ -90,6 +99,7 @@ test.describe("Worker", () => { // this selectOption is what will start failing, which is the right place to // notice. await page.getByLabel(tEn("dailyEntry:totalEggsLabel"), { exact: true }).fill("120"); + await clearSeededGrades(page); await page.getByRole("button", { name: tEn("dailyEntry:saveDraftButton") }).click(); // The refusal text comes from the SERVER's ProblemDetails detail, which is @@ -118,6 +128,7 @@ test.describe("Worker", () => { await page.getByLabel(tEn("dailyEntry:dateLabel")).fill(today); await selectOptionContaining(page.getByLabel(tEn("dailyEntry:flockLabel")), UNASSIGNED_FLOCK); await page.getByLabel(tEn("dailyEntry:totalEggsLabel"), { exact: true }).fill("130"); + await clearSeededGrades(page); await page.getByRole("button", { name: tEn("dailyEntry:saveDraftButton") }).click(); await expect(page.getByText(/not assigned to this flock/i)).toBeHidden(); diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 28996b383..d26996386 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -116,6 +116,30 @@ describe("apiFetch — no in-memory token", () => { expect(callsTo(fetchMock, "/stock")).toHaveLength(0); }); + it("preserves a credential-revocation reason from a failed silent refresh", async () => { + clearAccessToken(); + fetchMock.mockResolvedValueOnce(jsonResponse({ + title: "Auth.CredentialsSuperseded", + detail: "Your credentials changed.", + }, 401)); + + await expect(apiGet("/stock")).rejects.toMatchObject({ status: 401 }); + + expect(onUnauth).toHaveBeenCalledWith("Auth.CredentialsSuperseded"); + }); + + it("preserves a credential-revocation reason during load-time session restore", async () => { + clearAccessToken(); + fetchMock.mockResolvedValueOnce(jsonResponse({ + title: "Auth.AccountDisabled", + detail: "Your account has been disabled.", + }, 401)); + + await expect(restoreSession()).resolves.toBe(false); + + expect(onUnauth).toHaveBeenCalledWith("Auth.AccountDisabled"); + }); + it("uses the refreshed token when the silent refresh succeeds", async () => { clearAccessToken(); fetchMock @@ -127,6 +151,17 @@ describe("apiFetch — no in-memory token", () => { }); }); +describe("apiFetch — unrecoverable session reason", () => { + it("passes the original protected-request 401 title to auth teardown after refresh fails", async () => { + fetchMock + .mockResolvedValueOnce(jsonResponse({ title: "Auth.CredentialsSuperseded" }, 401)) + .mockResolvedValueOnce(jsonResponse({ title: "Identity.InvalidRefreshToken" }, 401)); + + await expect(apiGet("/stock")).rejects.toMatchObject({ title: "Auth.CredentialsSuperseded" }); + expect(onUnauth).toHaveBeenCalledWith("Auth.CredentialsSuperseded"); + }); +}); + // #217 — every request mints a W3C traceparent so a browser action correlates // with the API's request log and spans (#214). The client remembers the last // trace id so a crash report can join the failed screen's server-side story. @@ -704,6 +739,24 @@ describe("changePassword (#165)", () => { await expect(changePassword({ currentPassword: "x", newPassword: "y" })).rejects.toThrow(ApiError); expect(getAccessToken()).toBe("at1"); // unchanged }); + + it("refreshes and retries inside the cookie lock when the access token expired", async () => { + fetchMock + .mockResolvedValueOnce(jsonResponse({ title: "Unauthorized" }, 401)) + .mockResolvedValueOnce(accessResponse("refreshed-access")) + .mockResolvedValueOnce(accessResponse("password-change-access")); + + await changePassword({ currentPassword: "a", newPassword: "b" }); + + const changes = callsTo(fetchMock, "/auth/change-password"); + expect(changes).toHaveLength(2); + expect(callsTo(fetchMock, "/auth/refresh")).toHaveLength(1); + // The one logical write keeps its idempotency key across the auth retry. + expect(headerOf(changes[1], "Idempotency-Key")) + .toBe(headerOf(changes[0], "Idempotency-Key")); + expect(authOf(changes[1])).toBe("Bearer refreshed-access"); + expect(getAccessToken()).toBe("password-change-access"); + }); }); // #308 — step-up grant issuance. Consumed by UsersPage.tsx immediately before @@ -832,6 +885,249 @@ describe("cross-tab refresh coordination (#169)", () => { expect(getAccessToken()).toBe("at2"); }); + it("serializes password change after an in-flight refresh so stale credentials cannot win last", async () => { + const locks = fakeLockManager(); + vi.stubGlobal("navigator", { locks }); + + const refreshGate = deferred(); + const changeGate = deferred(); + fetchMock.mockImplementation((url: string) => { + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/change-password")) return changeGate.promise; + throw new Error(`unexpected fetch: ${url}`); + }); + + // The refresh proves the old credential epoch, then remains in flight. A + // password change that overtakes it can commit E+1 first, only for the late + // refresh response to overwrite both browser credentials with retired E. + const refreshing = restoreSession(); + await drain(); + const changing = changePassword({ currentPassword: "a", newPassword: "b" }); + await drain(); + + // Resolve in the dangerous order. Before serialization, change-password + // commits first and the stale refresh wins last. With the shared lock, the + // already-resolved password response is not requested until refresh exits. + changeGate.resolve(accessResponse("password-change-token")); + await drain(); + refreshGate.resolve(accessResponse("stale-refresh-token")); + + await expect(refreshing).resolves.toBe(true); + await expect(changing).resolves.toBeUndefined(); + expect(getAccessToken()).toBe("password-change-token"); + expect(callsTo(fetchMock, "/auth/refresh")).toHaveLength(1); + expect(callsTo(fetchMock, "/auth/change-password")).toHaveLength(1); + expect(sutLockName(locks)).toBe("cluckwork.auth.refresh"); + }); + + it("drops a queued password change before any request when logout supersedes it", async () => { + const locks = fakeLockManager(); + vi.stubGlobal("navigator", { locks }); + + // Another tab owns the cookie lock, so this tab's password change queues + // without starting a request. Logout must make that queued operation inert. + const otherTab = deferred(); + locks.request("cluckwork.auth.refresh", () => otherTab.promise); + fetchMock.mockImplementation(async (url: string) => { + if (url.endsWith("/auth/logout")) return new Response(null, { status: 204 }); + if (url.endsWith("/auth/refresh")) return accessResponse("post-logout-refresh"); + if (url.endsWith("/auth/change-password")) return accessResponse("post-logout-change"); + throw new Error(`unexpected fetch: ${url}`); + }); + + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((err: unknown) => err); + await drain(); + await logout(); + otherTab.resolve(); + await changing; + + expect(getAccessToken()).toBeNull(); + expect(callsTo(fetchMock, "/auth/refresh")).toHaveLength(0); + expect(callsTo(fetchMock, "/auth/change-password")).toHaveLength(0); + expect(callsTo(fetchMock, "/auth/logout")).toHaveLength(1); + }); + + it("drops a queued password change before it can write into a newer login", async () => { + const locks = fakeLockManager(); + vi.stubGlobal("navigator", { locks }); + + const otherTab = deferred(); + locks.request("cluckwork.auth.refresh", () => otherTab.promise); + fetchMock.mockImplementation(async (url: string) => { + if (url.endsWith("/auth/login")) return accessResponse("new-login-token"); + if (url.endsWith("/auth/change-password")) return accessResponse("stale-change-token"); + throw new Error(`unexpected fetch: ${url}`); + }); + + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((err: unknown) => err); + await drain(); + await login({ email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + otherTab.resolve(); + await changing; + + expect(getAccessToken()).toBe("new-login-token"); + expect(callsTo(fetchMock, "/auth/change-password")).toHaveLength(0); + }); + + it("drops a password change whose initial refresh is superseded by a newer login", async () => { + const refreshGate = deferred(); + fetchMock.mockImplementation(async (url: string) => { + if (url.endsWith("/auth/refresh")) return refreshGate.promise; + if (url.endsWith("/auth/login")) return accessResponse("new-login-token"); + if (url.endsWith("/auth/change-password")) return accessResponse("stale-change-token"); + throw new Error(`unexpected fetch: ${url}`); + }); + + clearAccessToken(); + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((err: unknown) => err); + await drain(); + + // The form is already inside the cookie lock, obtaining an access token. + // A newer login must prevent that old form from borrowing the new bearer + // when the now-stale refresh finishes. + await login({ email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + refreshGate.resolve(accessResponse("stale-refresh-token")); + await changing; + + expect(getAccessToken()).toBe("new-login-token"); + expect(callsTo(fetchMock, "/auth/refresh")).toHaveLength(1); + expect(callsTo(fetchMock, "/auth/change-password")).toHaveLength(0); + }); + + it("never retries an in-flight password change against a newer login", async () => { + const firstChange = deferred(); + let changeCalls = 0; + fetchMock.mockImplementation(async (url: string) => { + if (url.endsWith("/auth/change-password")) { + changeCalls += 1; + if (changeCalls === 1) return firstChange.promise; + return accessResponse("stale-form-retry-token"); + } + if (url.endsWith("/auth/login")) return accessResponse("new-login-token"); + if (url.endsWith("/auth/refresh")) return accessResponse("new-login-refreshed"); + throw new Error(`unexpected fetch: ${url}`); + }); + + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((err: unknown) => err); + await drain(); + + // Supersede the form while its first request is already on the wire, then + // make that old request answer 401. Generic apiFetch behavior would refresh + // the newer session and resend the old form body against it. + await login({ email: "new@session.test", password: `pw-${crypto.randomUUID()}` }); + firstChange.resolve(jsonResponse({ title: "Unauthorized" }, 401)); + await changing; + + expect(getAccessToken()).toBe("new-login-token"); + expect(callsTo(fetchMock, "/auth/change-password")).toHaveLength(1); + expect(callsTo(fetchMock, "/auth/refresh")).toHaveLength(0); + }); + + it("does not apply the refresh timeout to a non-replayable password change", async () => { + vi.useFakeTimers(); + try { + let changeSignal: AbortSignal | undefined; + const changeGate = deferred(); + fetchMock.mockImplementation(async (url: string, init: RequestInit) => { + if (url.endsWith("/auth/change-password")) { + changeSignal = init.signal ?? undefined; + return changeGate.promise; + } + throw new Error(`unexpected fetch: ${url}`); + }); + + const changing = changePassword({ currentPassword: "a", newPassword: "b" }); + await vi.advanceTimersByTimeAsync(0); + expect(callsTo(fetchMock, "/auth/change-password")).toHaveLength(1); + + // A slow committed response cannot be retried with the old password. The + // 15-second refresh timeout therefore must not abort this mutation. + await vi.advanceTimersByTimeAsync(20_000); + const wasAborted = changeSignal?.aborted; + + changeGate.resolve(accessResponse("password-change-token")); + await changing.catch(() => undefined); + expect(wasAborted).toBe(false); + } finally { + vi.useRealTimers(); + } + }); + + it("still times out a refresh nested inside the unbounded password-change lock", async () => { + vi.useFakeTimers(); + try { + let refreshSignal: AbortSignal | undefined; + let rejectRefresh!: (reason: unknown) => void; + let refreshCalls = 0; + fetchMock.mockImplementation(async (url: string, init: RequestInit) => { + if (url.endsWith("/auth/refresh")) { + refreshCalls += 1; + if (refreshCalls > 1) return accessResponse("recovered-token"); + refreshSignal = init.signal ?? undefined; + return new Promise((_resolve, reject) => { + rejectRefresh = reject; + refreshSignal?.addEventListener("abort", () => + reject(new DOMException("timed out", "AbortError")), + ); + }); + } + if (url.endsWith("/auth/change-password")) return accessResponse("must-not-send"); + throw new Error(`unexpected fetch: ${url}`); + }); + + clearAccessToken(); + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(0); + expect(callsTo(fetchMock, "/auth/refresh")).toHaveLength(1); + + await vi.advanceTimersByTimeAsync(20_000); + const wasAborted = refreshSignal?.aborted; + // Cleanup for the red implementation, whose missing timeout leaves the + // promise pending. This is a no-op after the fixed abort already rejected. + rejectRefresh(new DOMException("test cleanup", "AbortError")); + await changing; + + expect(wasAborted).toBe(true); + expect(callsTo(fetchMock, "/auth/change-password")).toHaveLength(0); + // The failed operation released both the local queue and Web Lock. + await expect(restoreSession()).resolves.toBe(true); + } finally { + vi.useRealTimers(); + } + }); + + it("still aborts an in-flight password change when the user explicitly logs out", async () => { + const changeStarted = deferred(); + let changeSignal: AbortSignal | undefined; + fetchMock.mockImplementation(async (url: string, init: RequestInit) => { + if (url.endsWith("/auth/change-password")) { + changeSignal = init.signal ?? undefined; + changeStarted.resolve(); + return new Promise((_resolve, reject) => + changeSignal?.addEventListener("abort", () => + reject(new DOMException("logged out", "AbortError")), + ), + ); + } + if (url.endsWith("/auth/logout")) return new Response(null, { status: 204 }); + throw new Error(`unexpected fetch: ${url}`); + }); + + const changing = changePassword({ currentPassword: "a", newPassword: "b" }) + .catch((err: unknown) => err); + await changeStarted.promise; + await logout(); + + expect(changeSignal?.aborted).toBe(true); + expect(await changing).toMatchObject({ name: "AbortError" }); + expect(getAccessToken()).toBeNull(); + }); + it("a lock held under a DIFFERENT name does not block refresh (fake is name-scoped, like the real API)", async () => { const locks = fakeLockManager(); vi.stubGlobal("navigator", { locks }); diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 15ca0f86f..7cfc1cb29 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -29,8 +29,8 @@ export class ApiError extends Error { // Called when the session is unrecoverable (refresh failed). AuthContext wires // this to drop React auth state and bounce to /login. -let onUnauthenticated: (() => void) | null = null; -export function setOnUnauthenticated(cb: (() => void) | null): void { +let onUnauthenticated: ((title?: string) => void) | null = null; +export function setOnUnauthenticated(cb: ((title?: string) => void) | null): void { onUnauthenticated = cb; } @@ -202,7 +202,28 @@ export async function changePassword( // before it lands would otherwise have the response write a fresh token back // over the cleared session. const generation = sessionGeneration; - const res = await apiPost("/auth/change-password", body); + // Refresh and password-change both replace the one per-origin HttpOnly + // cookie. Keep the password change inside the same cross-tab lock as refresh, + // or an already-started old-epoch refresh can answer last and overwrite the + // E+1 cookie this request just issued. Passing the held lock context lets + // apiFetch do its normal 401 refresh-and-retry inside this lock without + // trying to acquire the non-reentrant Web Lock a second time. + const res = await withAuthCookieLock((signal) => { + // The operation may have waited behind another tab's refresh. Logout or a + // newer login that happened while queued wins before this request sends a + // byte; never let the stale password form act on the superseding session. + if (sessionGeneration !== generation) throw new StaleSessionError(); + return apiFetch( + "/auth/change-password", + { + method: "POST", + headers: { "Idempotency-Key": newId() }, + body: JSON.stringify(body), + signal, + }, + { generation, signal }, + ); + }); // This response rotates the refresh cookie too, so a discarded one needs that // cookie revoked, not merely the in-memory token dropped. if (sessionGeneration !== generation) { @@ -246,7 +267,7 @@ export async function logout(): Promise { // instead of only being // discarded on arrival by the generation check in refreshTokens(). sessionGeneration++; - abortInFlightRefresh?.(); + abortInFlightAuthCookieOperation?.(); // #336 — capture BEFORE clearing: this tab's access token names the user who // actually clicked logout, and the clear below is deliberately synchronous. // Read it after and we would always send nothing. @@ -281,7 +302,8 @@ export async function logout(): Promise { } } -// #169 — serialise refresh across tabs. The refresh token lives only in the +// #169/#364 — serialise refresh and self-password-change across tabs. The +// refresh token lives only in the // shared HttpOnly cookie (#145), so two tabs refreshing at once each present the // SAME cookie value; the second hits an already-rotated token, the server reads // that as a replay and revokes the whole family — logging BOTH tabs out. The Web @@ -296,7 +318,7 @@ export async function logout(): Promise { // receiving the rotated cookie, the lock auto-releases while the cookie is still // the old value, so the next tab can still trip reuse-detection. Narrow, but a // full fix needs an idempotent server refresh — tracked in #176. -const REFRESH_LOCK = "cluckwork.auth.refresh"; +const AUTH_COOKIE_LOCK = "cluckwork.auth.refresh"; // Cap how long one tab may hold the cross-tab lock. fetch() has no default // timeout, so a hung /auth/refresh would otherwise keep the lock — and every @@ -304,23 +326,46 @@ const REFRESH_LOCK = "cluckwork.auth.refresh"; // which releases the lock; the aborting tab recovers on its next request. const REFRESH_TIMEOUT_MS = 15_000; -// #310 — the abort handle of whichever refresh attempt is currently running -// (if any), so logout() can cancel it outright rather than only waiting for -// the generation check below to discard its result on arrival. -let abortInFlightRefresh: (() => void) | null = null; +// #310/#364 — the abort handle of whichever cookie-mutating auth operation is +// currently running (if any), so logout() can cancel it outright rather than +// only waiting for the generation check below to discard its result on arrival. +let abortInFlightAuthCookieOperation: (() => void) | null = null; -function withRefreshLock(run: (signal: AbortSignal) => Promise): Promise { +// Web Locks coordinate tabs, while this queue provides the same ordering within +// one tab and on browsers where navigator.locks is unavailable. Rejections are +// absorbed only by the tail so one failed auth write never poisons later work; +// each caller still receives its own original result. +let authCookieTail: Promise = Promise.resolve(); + +function withAuthCookieLock( + run: (signal: AbortSignal) => Promise, + timeoutMs?: number, +): Promise { const attempt = () => { const controller = new AbortController(); - abortInFlightRefresh = () => controller.abort(); - const timer = setTimeout(() => controller.abort(), REFRESH_TIMEOUT_MS); + const abort = () => controller.abort(); + abortInFlightAuthCookieOperation = abort; + // Refresh is replayable and must not starve every tab indefinitely. A + // password change is not: aborting after the server commits can strand the + // browser with neither the old credential nor the fresh response, so that + // caller deliberately supplies no timeout. Explicit logout still aborts it + // through abortInFlightAuthCookieOperation above. + const timer = timeoutMs === undefined ? undefined : setTimeout(abort, timeoutMs); return run(controller.signal).finally(() => { - clearTimeout(timer); - abortInFlightRefresh = null; + if (timer !== undefined) clearTimeout(timer); + if (abortInFlightAuthCookieOperation === abort) + abortInFlightAuthCookieOperation = null; }); }; - const locks: LockManager | undefined = globalThis.navigator?.locks; - return locks ? (locks.request(REFRESH_LOCK, attempt) as Promise) : attempt(); + const queued = authCookieTail.then(() => { + const locks: LockManager | undefined = globalThis.navigator?.locks; + return locks ? (locks.request(AUTH_COOKIE_LOCK, attempt) as Promise) : attempt(); + }); + authCookieTail = queued.then( + () => undefined, + () => undefined, + ); + return queued; } // Single-flight refresh: concurrent 401s (and the load-time bootstrap) share one @@ -329,7 +374,68 @@ function withRefreshLock(run: (signal: AbortSignal) => Promise): Promise | null = null; -async function refreshTokens(): Promise { +type HeldAuthCookieLock = Readonly<{ + generation: number; + signal: AbortSignal; +}>; + +async function executeRefresh(generation: number, signal?: AbortSignal): Promise { + try { + const res = await raw("/auth/refresh", { + method: "POST", + headers: { [CSRF_HEADER]: "1" }, + signal, + }); + + // A SUCCESSFUL refresh rotated the cookie before we got here, so a + // discarded one leaves live credentials in the browser — revoke them. + if (sessionGeneration !== generation) { + await revokeSupersededCookie(); + throw new StaleSessionError(); + } + setAccessToken(res.accessToken); + onTokensChanged?.(); + return res.accessToken; + } catch (err) { + // A late FAILURE from an obsolete generation is discarded the same as a + // late success: the real error (e.g. a genuinely revoked refresh token) + // might otherwise tear down a newer, still-valid session. No revoke here — + // a failed refresh issued no cookie to revoke. + if (err instanceof StaleSessionError) throw err; + throw sessionGeneration !== generation ? new StaleSessionError() : err; + } +} + +async function executeHeldRefresh( + generation: number, + parentSignal: AbortSignal, +): Promise { + // The parent password-change operation is deliberately unbounded because it + // cannot be replayed safely after an ambiguous commit. Refresh remains + // replayable, though, and must not hold the shared cookie lock forever. Give + // only this nested request a timeout while forwarding explicit logout's abort. + const controller = new AbortController(); + const abort = () => controller.abort(); + if (parentSignal.aborted) abort(); + else parentSignal.addEventListener("abort", abort, { once: true }); + const timer = setTimeout(abort, REFRESH_TIMEOUT_MS); + try { + return await executeRefresh(generation, controller.signal); + } finally { + clearTimeout(timer); + parentSignal.removeEventListener("abort", abort); + } +} + +async function refreshTokens(heldLock?: HeldAuthCookieLock): Promise { + // changePassword owns the shared cookie lock while apiFetch performs its + // normal 401 refresh-and-retry. Web Locks are not reentrant, so execute that + // nested refresh directly under the lock already held by the caller. Keep + // the generation that entered the queue: resnapshotting here could attach a + // stale password-change operation to a logout/new-login generation. + if (heldLock) + return executeHeldRefresh(heldLock.generation, heldLock.signal); + if (refreshInFlight) return refreshInFlight; // #310 — captured once, at the START of this flight. Checked again at @@ -337,33 +443,10 @@ async function refreshTokens(): Promise { // network call is in the air discards its outcome instead of committing it. const generation = sessionGeneration; - refreshInFlight = withRefreshLock((signal) => - raw("/auth/refresh", { - method: "POST", - headers: { [CSRF_HEADER]: "1" }, - signal, - }), + refreshInFlight = withAuthCookieLock( + (signal) => executeRefresh(generation, signal), + REFRESH_TIMEOUT_MS, ) - .then( - async (res) => { - // A SUCCESSFUL refresh rotated the cookie before we got here, so a - // discarded one leaves live credentials in the browser — revoke them. - if (sessionGeneration !== generation) { - await revokeSupersededCookie(); - throw new StaleSessionError(); - } - setAccessToken(res.accessToken); - onTokensChanged?.(); - return res.accessToken; - }, - (err: unknown) => { - // A late FAILURE from an obsolete generation is discarded the same as - // a late success: the real error (e.g. a genuinely revoked refresh - // token) might otherwise tear down a newer, still-valid session. No - // revoke here — a failed refresh issued no cookie to revoke. - throw sessionGeneration !== generation ? new StaleSessionError() : err; - }, - ) .finally(() => { refreshInFlight = null; }); @@ -378,7 +461,14 @@ export async function restoreSession(): Promise { try { await refreshTokens(); return true; - } catch { + } catch (err) { + // The normal no-cookie case remains a quiet false result in the UI because + // Login only renders the two credential-revocation titles. Preserve a + // server-provided 401 title nevertheless: if the refresh endpoint can + // classify a superseded/disabled session, load-time bootstrap must not be + // the one teardown path that discards that reason. + if (err instanceof ApiError && err.status === 401) + onUnauthenticated?.(err.title); return false; } } @@ -473,7 +563,7 @@ export async function apiGetBlob( } if (isTransientRefreshFailure(refreshErr)) throw refreshErr; clearAccessToken(); - onUnauthenticated?.(); + onUnauthenticated?.(err.title); throw err; } } @@ -495,27 +585,44 @@ async function rawBlob( }; } -export async function apiFetch(path: string, init: RequestInit = {}): Promise { - const token = await currentAccessToken(); +export async function apiFetch( + path: string, + init: RequestInit = {}, + heldAuthCookieLock?: HeldAuthCookieLock, +): Promise { + const token = await currentAccessToken(heldAuthCookieLock); + // currentAccessToken may itself await a cookie refresh. A newer login can + // supersede the non-replayable password form during that await; never let the + // stale form borrow the newer session's bearer for its first request. + if (heldAuthCookieLock && sessionGeneration !== heldAuthCookieLock.generation) + throw new StaleSessionError(); try { return await raw(path, init, token); } catch (err) { if (!(err instanceof ApiError) || err.status !== 401) throw err; + // A held context identifies the non-replayable password form. Generic API + // requests may continue on a newer login after supersession; this one must + // not refresh that newer cookie or resend the old password body against it. + if (heldAuthCookieLock && sessionGeneration !== heldAuthCookieLock.generation) + throw new StaleSessionError(); try { - const refreshed = await refreshTokens(); + const refreshed = await refreshTokens(heldAuthCookieLock); + if (heldAuthCookieLock && sessionGeneration !== heldAuthCookieLock.generation) + throw new StaleSessionError(); return await raw(path, init, refreshed); } catch (refreshErr) { // #310 review — superseded mid-retry: retry once on the newer login's // token, or surface the ORIGINAL 401 if a logout ended the session. // Never let the internal marker reach the caller. if (refreshErr instanceof StaleSessionError) { + if (heldAuthCookieLock) throw refreshErr; const fresh = getAccessToken(); if (fresh) return await raw(path, init, fresh); throw err; } if (isTransientRefreshFailure(refreshErr)) throw refreshErr; clearAccessToken(); - onUnauthenticated?.(); + onUnauthenticated?.(err.title); throw err; } } @@ -524,18 +631,20 @@ export async function apiFetch(path: string, init: RequestInit = {}): Promise // The in-memory access token, or one obtained via a silent refresh when memory // is empty (e.g. a request racing the load-time bootstrap). Throws + signals // unauthenticated if no session can be established. -async function currentAccessToken(): Promise { +async function currentAccessToken( + heldAuthCookieLock?: HeldAuthCookieLock, +): Promise { const token = getAccessToken(); if (token) return token; try { - return await refreshTokens(); + return await refreshTokens(heldAuthCookieLock); } catch (err) { // #310 — a stale (obsolete-generation) failure belongs to an already- // superseded session: a newer login already committed its own token, or a // logout already tore everything down. Either way, this failure must not // touch the CURRENT session — no clearing, no onUnauthenticated navigate. if (err instanceof StaleSessionError) return supersededToken(); - onUnauthenticated?.(); + onUnauthenticated?.(err instanceof ApiError ? err.title : undefined); throw new ApiError(401, "NoSession", "Not authenticated."); } } diff --git a/web/src/auth/AuthContext.tsx b/web/src/auth/AuthContext.tsx index d45605a38..888249273 100644 --- a/web/src/auth/AuthContext.tsx +++ b/web/src/auth/AuthContext.tsx @@ -23,6 +23,7 @@ interface AuthState { // reads this to show the set-password screen instead of the app shell; the // API's MustChangePasswordMiddleware is the actual enforcement. mustChangePassword: boolean; + unauthenticatedReason: string | null; login: (email: string, password: string) => Promise; logout: () => Promise; } @@ -38,6 +39,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { const [isAdmin, setIsAdmin] = useState(currentUserIsAdmin); const [role, setRole] = useState(currentUserRole); const [mustChangePassword, setMustChangePassword] = useState(currentUserMustChangePassword); + const [unauthenticatedReason, setUnauthenticatedReason] = useState(null); // One place that re-derives every claim-decoded field from the current // token, called everywhere a token pair changes (login, transparent @@ -55,9 +57,10 @@ export function AuthProvider({ children }: { children: ReactNode }) { // re-derives the role — the server re-reads roles on every refresh, so a // demotion reaches the UI within one token lifetime (codex review of PR #78). useEffect(() => { - setOnUnauthenticated(() => { + setOnUnauthenticated((title) => { clearAccessToken(); setIsAuthenticated(false); + setUnauthenticatedReason(title ?? null); }); setOnTokensChanged(refreshClaims); return () => { @@ -97,6 +100,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { const login = useCallback(async (email: string, password: string) => { await apiLogin({ email, password }); setIsAuthenticated(true); + setUnauthenticatedReason(null); refreshClaims(); }, [refreshClaims]); @@ -110,11 +114,12 @@ export function AuthProvider({ children }: { children: ReactNode }) { setIsAdmin(false); setRole("Worker"); setMustChangePassword(false); + setUnauthenticatedReason(null); }, []); const value = useMemo( - () => ({ isAuthenticated, isLoading, isAdmin, role, mustChangePassword, login, logout }), - [isAuthenticated, isLoading, isAdmin, role, mustChangePassword, login, logout], + () => ({ isAuthenticated, isLoading, isAdmin, role, mustChangePassword, unauthenticatedReason, login, logout }), + [isAuthenticated, isLoading, isAdmin, role, mustChangePassword, unauthenticatedReason, login, logout], ); return {children}; diff --git a/web/src/i18n/en.ts b/web/src/i18n/en.ts index 90594d9c9..2292e6d75 100644 --- a/web/src/i18n/en.ts +++ b/web/src/i18n/en.ts @@ -34,6 +34,8 @@ export const en = { signIn: "Sign in", signingIn: "Signing in…", invalidCredentials: "Invalid email or password.", + credentialsSuperseded: "Your credentials changed. Please sign in again.", + accountDisabled: "Your account has been disabled.", tooManyAttempts: "Too many sign-in attempts. Please wait a few minutes and try again.", apiDown: "Could not sign in. Is the API running?", @@ -1919,6 +1921,9 @@ export const en = { + "right there in the dialog: creating another Owner, and resetting an existing Owner's password. This " + "confirms it's really you before handing out that much access — every other action on that screen " + "(creating a Worker/Manager/Sales/Read-only user, resetting one of their passwords) does not ask again.", + signingInCredentialEpoch: + "When an administrator resets a password, your existing sign-in can be invalidated immediately. If you " + + "see a message that your credentials changed, sign in again with your current password.", interfaceLanguage: "Interface language. Everyone can choose the language the interface is shown in from " + "Account → Preferences — English, Español, or Tagalog. Translation is a work in " @@ -1962,9 +1967,9 @@ export const en = { ownPassword: "Your own password. Anyone, in any role, can change their own password on the " + "Account screen by entering the current one and a new one (at least 12 characters). " - + "Changing it — or an admin setting it for you — signs that account out on every other " - + "device; the device you changed it on stays signed in. An already-open session elsewhere can linger " - + "for a few minutes before it stops working.", + + "Changing your own password keeps this device signed in with fresh credentials and ends every " + + "other open session on its next request. If an admin sets your password instead, every one " + + "of your open sessions ends on its next request.", // Adding & correcting dialogsHeading: "Adding & correcting", diff --git a/web/src/i18n/es.ts b/web/src/i18n/es.ts index 9ce3724dd..287418af6 100644 --- a/web/src/i18n/es.ts +++ b/web/src/i18n/es.ts @@ -33,6 +33,8 @@ export const es = { signIn: "Iniciar sesión", signingIn: "Iniciando sesión…", invalidCredentials: "Correo electrónico o contraseña inválidos.", + credentialsSuperseded: "Sus credenciales cambiaron. Inicie sesión de nuevo.", + accountDisabled: "Su cuenta ha sido deshabilitada.", tooManyAttempts: "Demasiados intentos de inicio de sesión. Espere unos minutos y vuelva a intentarlo.", apiDown: "No se pudo iniciar sesión. ¿Está la API en ejecución?", @@ -1581,6 +1583,9 @@ export const es = { + "propietario existente. Esto confirma que realmente es usted antes de otorgar tanto acceso — ninguna " + "otra acción en esa pantalla (crear un usuario Worker/Manager/Sales/Read-only, restablecer su " + "contraseña) vuelve a preguntar.", + signingInCredentialEpoch: + "Cuando un administrador restablece una contraseña, su sesión actual puede invalidarse inmediatamente. Si " + + "ve un mensaje indicando que sus credenciales cambiaron, inicie sesión de nuevo con su contraseña actual.", interfaceLanguage: "Idioma de la interfaz. Cualquiera puede elegir el idioma en que se muestra la " + "interfaz desde Cuenta → Preferencias — inglés, español o tagalo. La traducción es " @@ -1627,10 +1632,9 @@ export const es = { ownPassword: "Su propia contraseña. Cualquiera, en cualquier rol, puede cambiar su propia " + "contraseña en la pantalla Cuenta ingresando la actual y una nueva (al menos 12 " - + "caracteres). Cambiarla — o que un administrador se la establezca — cierra la sesión de esa cuenta " - + "en cada otro dispositivo; el dispositivo en el que la cambió permanece con la sesión " - + "iniciada. Una sesión ya abierta en otro lugar puede seguir activa unos minutos antes de dejar de " - + "funcionar.", + + "caracteres). Cambiar su propia contraseña mantiene este dispositivo conectado con credenciales " + + "nuevas y termina cada otra sesión abierta en su siguiente solicitud. Si un administrador le " + + "establece la contraseña, todas sus sesiones abiertas terminan en su siguiente solicitud.", dialogsHeading: "Agregar y corregir", dialogsPopup: diff --git a/web/src/i18n/tl.ts b/web/src/i18n/tl.ts index 1f137537b..9f4f2627a 100644 --- a/web/src/i18n/tl.ts +++ b/web/src/i18n/tl.ts @@ -38,6 +38,8 @@ export const tl = { signIn: "Mag-sign in", signingIn: "Nagsa-sign in…", invalidCredentials: "Mali ang email o password.", + credentialsSuperseded: "Nagbago ang iyong mga kredensyal. Mag-sign in muli.", + accountDisabled: "Na-disable ang iyong account.", tooManyAttempts: "Sobra na ang subok sa pag-sign in. Maghintay ng ilang minuto at subukan ulit.", apiDown: "Hindi makapag-sign in. Gumagana ba ang API?", @@ -1640,6 +1642,9 @@ export const tl = { + "ng password ng isang umiiral na Owner. Kinukumpirma nito na ikaw talaga bago bigyan ng ganoong " + "kalaking access — walang ibang aksyon sa screen na iyon (paggawa ng Worker/Manager/Sales/Read-only " + "user, pag-reset ng password nila) ang muling nagtatanong.", + signingInCredentialEpoch: + "Kapag ni-reset ng administrator ang password, maaaring agad ma-invalid ang kasalukuyan mong sign-in. Kung " + + "makakita ka ng mensaheng nagbago ang iyong credentials, mag-sign in muli gamit ang kasalukuyan mong password.", interfaceLanguage: "Wika ng interface. Kahit sino ay puwedeng pumili ng wikang gagamitin sa interface " + "mula sa Account → Mga Kagustuhan — English, Español, o Tagalog. Isang patuloy na " @@ -1689,10 +1694,10 @@ export const tl = { ownPassword: "Ang sarili mong password. Kahit sino, sa kahit anong tungkulin, ay puwedeng " + "magpalit ng sariling password sa screen na Account sa pamamagitan ng pag-enter ng " - + "kasalukuyan at bagong password (hindi bababa sa 12 karakter). Ang pagpapalit nito — o ang " - + "pagseset ng admin nito para sa iyo — ay nagsa-sign out sa account na iyon sa bawat ibang " - + "device; mananatiling naka-sign in ang device na pinagpalitan mo. Ang isang bukas nang session sa " - + "ibang lugar ay puwedeng magtagal ng ilang minuto bago tumigil sa paggana.", + + "kasalukuyan at bagong password (hindi bababa sa 12 karakter). Ang pagpapalit ng sarili mong password " + + "ay nagpapanatiling naka-sign in sa device na ito gamit ang bagong credentials at nagpapatigil sa bawat " + + "ibang bukas na session sa susunod nitong request. Kung admin ang magse-set ng password mo, " + + "titigil ang lahat ng bukas mong session sa susunod nitong request.", dialogsHeading: "Pagdagdag at pagtatama", dialogsPopup: diff --git a/web/src/routes/HelpPage.test.tsx b/web/src/routes/HelpPage.test.tsx index 6bfdadee8..cf39fc61f 100644 --- a/web/src/routes/HelpPage.test.tsx +++ b/web/src/routes/HelpPage.test.tsx @@ -2,6 +2,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { render, screen, within, act } from "@testing-library/react"; import { HelpPage } from "./HelpPage"; import i18n from "../i18n"; +import { en } from "../i18n/en"; +import { es } from "../i18n/es"; +import { tl } from "../i18n/tl"; // Minimal IntersectionObserver stub (jsdom has none): capture the callback so a // test can simulate a section scrolling into view. @@ -89,6 +92,33 @@ describe("HelpPage", () => { expect(signIn).toBeInTheDocument(); }); + it("distinguishes self-change session retention from an admin reset in every catalog", () => { + render(); + expect(screen.getByText(/keeps this device signed in/i)).toBeInTheDocument(); + expect(screen.getByText(/every one of your open sessions ends on its next request/i)).toBeInTheDocument(); + + expect(en.help.ownPassword).toMatch( + /keeps this device signed in.*every other<\/em> open session.*next request/i, + ); + expect(en.help.ownPassword).toMatch( + /admin sets your password.*every one of your open sessions ends on its next request/i, + ); + expect(es.help.ownPassword).toMatch( + /mantiene este dispositivo conectado.*cada otra<\/em> sesión abierta.*siguiente solicitud/i, + ); + expect(es.help.ownPassword).toMatch( + /administrador le establece la contraseña.*todas sus sesiones abiertas terminan.*siguiente solicitud/i, + ); + expect(tl.help.ownPassword).toMatch( + /naka-sign in sa device na ito.*bawat ibang<\/em> bukas na session.*susunod nitong request/i, + ); + expect(tl.help.ownPassword).toMatch( + /admin ang magse-set ng password mo.*lahat ng bukas mong session.*susunod nitong request/i, + ); + for (const catalog of [en, es, tl]) + expect(catalog.help.ownPassword).not.toMatch(/few minutes|unos minutos|ilang minuto/i); + }); + it("documents the per-account report throttle a user can actually hit (#311)", () => { // #311 caps concurrently in-flight reports per account, so a real user can // meet a 429 on the Reports screen. Both in-app surfaces must say so — the diff --git a/web/src/routes/HelpPage.tsx b/web/src/routes/HelpPage.tsx index 0a7bf8f37..08cef7e07 100644 --- a/web/src/routes/HelpPage.tsx +++ b/web/src/routes/HelpPage.tsx @@ -132,6 +132,9 @@ export function HelpPage() {
  • }} />
    • +
  • + }} /> +

    • diff --git a/web/src/routes/Login.test.tsx b/web/src/routes/Login.test.tsx index 4b1a40f43..0561691c4 100644 --- a/web/src/routes/Login.test.tsx +++ b/web/src/routes/Login.test.tsx @@ -4,7 +4,7 @@ import { Routes, Route } from "react-router"; import { Login } from "./Login"; import { ProtectedRoute } from "./ProtectedRoute"; import { renderWithProviders } from "../test/renderWithProviders"; -import { login as apiLogin, ApiError } from "../api/client"; +import { login as apiLogin, ApiError, setOnUnauthenticated } from "../api/client"; import { setStoredToken } from "../test/jwt"; import i18n from "../i18n"; @@ -22,6 +22,7 @@ vi.mock("../api/client", async (importOriginal) => { }); const mockApiLogin = vi.mocked(apiLogin); +const mockSetOnUnauthenticated = vi.mocked(setOnUnauthenticated); // /dashboard is behind the real ProtectedRoute, so navigation there only // succeeds if login actually established authenticated state — a bare public @@ -108,6 +109,19 @@ describe("Login", () => { expect(screen.queryByText("dashboard (protected)")).not.toBeInTheDocument(); }); + it.each([ + ["Auth.CredentialsSuperseded", i18n.t("auth:credentialsSuperseded")], + ["Auth.AccountDisabled", i18n.t("auth:accountDisabled")], + ])("surfaces a protected-request %s reason on the login page", async (title, message) => { + renderWithProviders(tree(), { route: "/dashboard", token: { sub: "u1", role: "Sales" } }); + expect(await screen.findByText("dashboard (protected)")).toBeInTheDocument(); + + const callback = mockSetOnUnauthenticated.mock.calls[0][0]; + await act(async () => callback?.(title)); + + expect(await screen.findByText(message)).toBeInTheDocument(); + }); + it("shows a rate-limit message on a 429 and stays on /login", async () => { mockApiLogin.mockRejectedValue(new ApiError(429, "Too many requests", "slow down")); renderWithProviders(tree(), { route: "/login", token: null }); diff --git a/web/src/routes/Login.tsx b/web/src/routes/Login.tsx index e4462cdf3..6eac52afb 100644 --- a/web/src/routes/Login.tsx +++ b/web/src/routes/Login.tsx @@ -17,6 +17,8 @@ interface LocationState { // account has no Owner. Mirrors AuthEndpoints.NoOwnerProvisionedCode; it // rides the ProblemDetails `title`, which parseError puts on ApiError.title. const NO_OWNER_PROVISIONED = "Auth.NoOwnerProvisioned"; +const CREDENTIALS_SUPERSEDED = "Auth.CredentialsSuperseded"; +const ACCOUNT_DISABLED = "Auth.AccountDisabled"; // Matched on the code, never on the message: the copy is translated and the // server's English detail is not what identifies the case. @@ -54,7 +56,7 @@ function messageFor(err: unknown): string { export function Login() { const { t } = useTranslation("auth"); - const { login, isAuthenticated, isLoading } = useAuth(); + const { login, isAuthenticated, isLoading, unauthenticatedReason } = useAuth(); const navigate = useNavigate(); const location = useLocation(); const from = (location.state as LocationState | null)?.from?.pathname ?? "/"; @@ -70,6 +72,13 @@ export function Login() { const [error, setError] = useState(null); const { busy, run } = usePendingAction(); + useEffect(() => { + if (unauthenticatedReason === CREDENTIALS_SUPERSEDED) + setError(t("credentialsSuperseded")); + else if (unauthenticatedReason === ACCOUNT_DISABLED) + setError(t("accountDisabled")); + }, [t, unauthenticatedReason]); + // #283 follow-up — a freshly migrated default account has base reference data // but no Owner, because no credential is ever migration-baked, so there is // nobody for the operator to sign in as and the form used to say nothing diff --git a/web/src/routes/UsersPage.test.tsx b/web/src/routes/UsersPage.test.tsx index 72a404edc..2d2a6fc10 100644 --- a/web/src/routes/UsersPage.test.tsx +++ b/web/src/routes/UsersPage.test.tsx @@ -1178,6 +1178,7 @@ describe("UsersPage step-up authentication (#308)", () => { // #283 — an Owner working the Users screen is already past the // first-run set-password gate. mustChangePassword: false, + unauthenticatedReason: null, login: vi.fn(), logout: vi.fn(), }} >