diff --git a/docs/plans/357-change-user-email/00-driver-handout.md b/docs/plans/357-change-user-email/00-driver-handout.md new file mode 100644 index 000000000..a9963f8ea --- /dev/null +++ b/docs/plans/357-change-user-email/00-driver-handout.md @@ -0,0 +1,263 @@ +# Driver handout — #357 change a user's email + +Driver-owned working artifact. The locked design, increment plan, runbook, verification evidence, +and review history will be added only after their corresponding feature-driver gates pass. + +## 1. Dispatch contract (owner-approved 2026-08-25) + +- **Mode:** feature. +- **Implementer:** isolated Codex cloud subagent, `gpt-5.6-sol`; it will receive a prescriptive + runbook and an explicit file allow-list. Rationale: the implementation needs inference across + Identity, credential revocation, tenant isolation, a barrier-controlled race, HTTP wiring, and SPA + state; spec cost is comparable to diff cost, so a transcriber is not the right implementation tier. +- **Reviewers:** + - Claude Code: repository-rule compliance across the complete changed-file set; its brief will name + `AGENTS.md`, the applicable decision records, and the final changed files, with a five-minute + budget and a MERGE-BLOCKING/FOLLOW-UP verdict for every finding. + - pi `llamacpp/qwen3.8-27b-q5-xl-220k-q8kv`: authentication, authorization, and tenant-isolation + invariants in the Identity adapter, application handler, endpoint, and integration tests; five-minute + budget and a MERGE-BLOCKING/FOLLOW-UP verdict for every finding. + - pi `vllm/deepseek-v4-flash-0731-nvfp4-dspark-ctx262144`: concurrency/race safety and false-green + tests, including credential-epoch, refresh-token, step-up, and mutation coverage; five-minute budget + and a MERGE-BLOCKING/FOLLOW-UP verdict for every finding. + - The SPA/API field-error, accessibility, help, glossary, and `en`/`es`/`tl` contract is explicitly + included in the Claude and Qwen briefs rather than assigned an unchosen fourth backend. +- **Seat briefed on the repo's own written rules:** Claude Code; `AGENTS.md`, + `docs/architecture.md`, `docs/decisions/269-transient-db-retry-boundary.md`, + `docs/decisions/364-credential-epoch-revocation.md`, + `docs/decisions/394-write-contract-callers.md`, and + `docs/decisions/530-multi-farm-tenancy.md`, plus any additional decision record the design walk finds. +- **Background visibility:** concise progress updates in the driving conversation. +- **External review trigger:** none; these are driver-dispatched reviewers, not a PR bot. +- **Escalation:** the same increment failing twice returns to the owner for re-routing; no third attempt + at the same tier. +- **Git/PR authority:** the implementer may create the feature branch, commit, push, and open a draft PR. +- **Lessons log:** `/home/mforce/.local/state/cluckwork-feature-driver/357-lessons.md`, deliberately + outside the repository and excluded from reviewer artifacts. +- **Applied by, per increment:** none yet. +- **Fix budget:** 0 shipped fixes; driver-run Phase 11 mutations may be injected and reverted. +- **Direct applications spent:** 0 of 0. +- **Correctness-critical findings so far:** the pre-signoff Claude architecture pass required + constraint-specific duplicate mapping and restoration of tracked identity fields on every + pre-commit failure; both are design corrections, not product-code fixes. + +### Design-fix ledger + +| Fix ID | Round | Finding | Invariant ID + one-line statement | Enforcement sites (symbols) | Prior entries compared | Same area as / "new" + why | SHA (filled after) | +|---|---|---|---|---|---|---|---| + +## 2. Review loop + +Every selected reviewer returns a MERGE-BLOCKING or FOLLOW-UP verdict per finding; neither verdict is +preferred, and inflating a follow-up to be safe costs a fix round an issue would carry. A round is clean +only when all three reviewers answer on the same unchanged head. Stop after two consecutive rounds find +no product-code defect, or when all findings are regressions created by the preceding fix. + +## 3. Locked design (owner-approved 2026-08-25) + +### Scope and behavior + +- Add `PUT /api/v1/users/{id}/email`, returning 204 on success and on a true no-op. It remains under + the Owner-only users group, requires `Idempotency-Key`, requires an unconditional step-up grant, and + carries a 2048-byte request-body cap. +- The request is `{ "email": "..." }`. Validation mirrors create-user's required/email/256-character + rules. The handler trims the value before comparing or writing it. +- A trimmed value exactly equal to the stored `Email` is a true no-op: no stamp rotation, epoch bump, + token revocation, or audit row. A case-only correction is a real change even when normalization yields + the same indexed value. +- A disabled target may have a typo corrected and remains disabled. The authenticated actor, not the + target, must still be an active Owner at the point of mutation. +- A self-change is allowed when another active Owner exists. A sole Owner's self-change fails with an + actionable message that says to add a second Owner first. +- A same-farm collision returns `409 Users.DuplicateEmail`; the SPA attaches it to the email field. The + same normalized address in another farm succeeds. A foreign user id returns 404. +- No SMTP or verification flow is added. `EmailConfirmed` is preserved. Help and glossary copy explicitly + say that an Owner's administrative change takes effect immediately and sends no confirmation email. + +### Application and HTTP shape + +- Add a `ChangeUserEmailCommand`, validator, and handler beside the existing user feature slices. The + handler consumes the step-up grant before entering Identity and passes `accountId`, target id, acting + user id, and the trimmed address through a new narrow `IIdentityProvider.ChangeUserEmailAsync` port. +- Add the endpoint and request record to `UserEndpoints`, map validation failures with the existing + `ValidationResponse`, map missing/stale proof and a stale actor to 403, foreign ids to 404, duplicate or + concurrency conflicts to 409, `Users.LastOwner` to 422, and all remaining expected Identity failures + to 422. +- Register the handler and validator in + `src/Cluckwork.Api/Hosting/CluckworkFeatureServiceCollectionExtensions.cs`, the current registration + seam reached by `Program.cs`. Do not broaden `UpdateUserAsync`: display-name edit remains ungated and + has none of this operation's credential semantics. + +### Identity transaction and concurrency contract + +`IdentityProvider.ChangeUserEmailAsync` uses `AmbientTransaction.RunAsync` and this order: + +1. Take the account row lock unconditionally and verify it matches `accountId`. +2. Load the target by `(userId, accountId)` and re-read the actor as an active Owner inside the lock. +3. Return the exact-value no-op before any mutation. +4. For a self-target, use `CountOtherActiveOwnersAsync`; zero returns `Users.LastOwner` with “add a second + Owner first” wording. +5. Snapshot every user scalar this operation can touch (`Email`, `NormalizedEmail`, `UserName`, + `NormalizedUserName`, `SecurityStamp`, `ConcurrencyStamp`, and `CredentialEpoch`), together with the + entry's incoming state/modified flags. Then assign the four email/login fields together using + Identity's configured normalizer. Preserve `EmailConfirmed`, role, disabled state, password, and every + other profile field. +6. Rotate `SecurityStamp` through `UserManager.UpdateSecurityStampAsync`. This runs the configured + `AccountScopedUserValidator`, invalidates outstanding step-up grants, and rotates Identity's + concurrency stamp. Do not add a uniqueness pre-read. +7. On a real change, increment `CredentialEpoch`, revoke every active refresh token, and append + `AuditActions.UserEmailChanged` with `{ oldEmail, newEmail }` details before the final save and commit. + +An Identity result containing `DuplicateEmail` or `DuplicateUserName` maps to the one public +`Users.DuplicateEmail` conflict. A database race maps the same way only when the inner +`PostgresException` is SQLSTATE `23505` and its `ConstraintName` is exactly `EmailIndex` or +`UserNameIndex`; every other `DbUpdateException` propagates. Concurrency failures map to +`Users.Conflict`. Restore every snapshotted value and its incoming tracking state on every pre-commit +error return or caught failure, and detach only the audit row this operation added if the final save +failed; transaction rollback alone does not rewind EF's in-memory entity for a longer-lived non-HTTP +caller. Do not clear the whole change tracker or disturb entries that belonged to the caller. + +There is no inner retry and no durability probe: the request-wide idempotency transaction already runs +under `SingleAttemptExecution`. The account lock order matches role/disable/enable and makes the +last-active-Owner decision race-safe. + +### SPA and documentation + +- Add `changeUserEmail` to the API client and a dedicated row action/dialog to `UsersPage`; do not fold + identity changes into the display-name dialog. +- The dialog identifies the target, explains that the new address becomes the login immediately and no + confirmation email is sent, collects the new email and the acting Owner's current password, and uses + the existing `Dialog`, `BusyButton`, `useDialogErrors`, `usePendingAction`, target-ref, step-up, and + idempotency-key patterns. Clear the password from state immediately after sending it for step-up and on + close/logout. +- Render `Users.DuplicateEmail` through dialog-local `emailFieldError` state, not `useDialogErrors` (whose + contract is page/dialog strings only). Bind the input with `aria-invalid` and described error text; + clear the field error on a new attempt, input change, close, and target change, while `activeEmail` + prevents a late response from attaching to a reopened dialog. Other failures remain dialog errors. On + a self-change, the next authenticated request intentionally follows the existing + credentials-superseded sign-in path; no replacement tokens are minted. +- Add complete `en`, `es`, and `tl` catalog entries, Help-page guidance, the in-app glossary entry, and + the corresponding product definition in `specs/product/GLOSSARY.md`. + +### Pre-signoff architecture review + +Claude Code (`claude-sonnet-5`) returned two merge-blocking design findings after reading the +relevant Identity and persistence seams: + +1. A concurrent duplicate must be mapped to `Users.DuplicateEmail` only when the inner PostgreSQL + exception is SQLSTATE `23505` and names `EmailIndex` or `UserNameIndex`; a blanket + `DbUpdateException` catch would misreport unrelated database failures as an email conflict. +2. Before mutating the four identity fields, snapshot `Email`, `NormalizedEmail`, `UserName`, and + `NormalizedUserName`; restore them on every early-return or caught-failure path. Rolling back a + database transaction does not repair EF's tracked in-memory entity in longer-lived non-HTTP callers. + +Two non-blocking decisions are explicit in the revised design: preserve `EmailConfirmed` because the +product has no verification flow, and let a self-change invalidate the current session through the +existing credential-superseded UX rather than minting replacement credentials. + +DeepSeek V4 Flash approved the locked design with two test-strengthening follow-ups, both incorporated: +the tracker-restoration test must reuse the same `AppDbContext` and perform a later save, and the refresh +race must present the late child after the email-change commit and assert 401 rather than merely inspect +revocation rows. The first Qwen design-review attempt reached its five-minute cap without a verdict; its +single permitted retry completed through a Herdr wrapper and approved with two follow-ups, also +incorporated: explicitly map `Users.LastOwner` to 422 and use dedicated accessible field-error state for +the SPA duplicate conflict. + +## 4. Increment plan + +Full checkbox plan: [`docs/superpowers/plans/2026-08-25-change-user-email.md`](../../superpowers/plans/2026-08-25-change-user-email.md). + +1. **Baseline and branch:** record backend/frontend baseline on the untouched base, verify create targets, + and create `feat/change-user-email` only after owner plan signoff. +2. **Backend vertical slice:** write the HTTP/Identity/race tests red, then add the command, validator, + handler, narrow Identity port, account-locked mutation, exact duplicate/concurrency mapping, audit + vocabulary/readers, endpoint, body cap, and DI registrations; commit one independently green backend + unit because the port/provider/endpoint cannot compile honestly as separate partial commits. +3. **SPA + documentation slice:** write Users/Help tests red, then add the client call, dedicated accessible + dialog, field-specific duplicate handling, late-response protection, `en`/`es`/`tl`, Help/in-app + glossary, and product glossary; commit one independently green user-facing unit. +4. **Verification and draft PR:** run CI-equivalent backend/frontend gates in the foreground, verify no + migration/schema or mutation debris, push, and open a draft PR before review. The driver—not the + implementer—then performs the recorded mutation matrix and independent acceptance verification. + +### Increment-plan review disposition + +- Qwen found one accepted blocker: the duplicate classifier was specified `private` while its named + precedent-style test requires the Infrastructure assembly's existing `internal` test seam. The plan now + specifies `internal static`, matching `AccountProvisioner.IsSlugConflict`. +- Qwen's claimed `ApplicationUser` global-filter blocker was rejected: `ApplicationUser` is deliberately + filter-free in `AppDbContext`. The plan nevertheless names `WithTenantScopeAsync` plus an explicit + `(Id, AccountId)` predicate so the test cannot drift into `IgnoreQueryFilters` or an unscoped lookup. +- Qwen's stamp-snapshot ordering concern was already satisfied by the locked sequence; the plan now says + “before any assignment and before `UpdateSecurityStampAsync`” so it cannot be misread. Its 413, + audit-registry, active-target, and normalizer suggestions were accepted as clarity improvements. +- Claude reached its eight-turn limit with no result; DeepSeek reached the five-minute cap with no output. + Neither silence is counted as approval. +- Qwen's focused confirmation approved the corrected plan. Its only note was the mechanical registry-name + typo `AUDIT_ACTION_ENTITY_TYPES`; the plan now names the real singular `AUDIT_ACTION_ENTITY_TYPE`. +- The user-authorized fresh Claude retry approved the corrected plan with no blockers. Its one follow-up + is incorporated: the endpoint must match `Users.DuplicateEmail` literally for 409 because the code does + not satisfy the siblings' `.Conflict` suffix test. + +## 5. Standing repo rules that bite here + +- `AccountScopedUserValidator` remains the sole friendly uniqueness check; the two composite database + indexes remain the race authority. +- `AuditActions.UserEmailChanged` must be passed directly at the `IAuditWriter.WriteAsync` call site, and + every reader of the audit-action registry (including `web/src/i18n/enums.ts` and its parity guards) + must be updated. +- Adding the handler/validator updates `CluckworkFeatureServiceCollectionExtensions.cs` registrations + and their registration guards. +- The endpoint must use the existing body-cap syntax before tests are written, because the guard inspects + call-site syntax. Its write contract requires review of API client and non-CI callers; no seeder or k6 + caller currently invokes an email-change route. +- This changes user-visible behavior, so the Help page, in-app glossary/locales, and + `specs/product/GLOSSARY.md` move in the same PR. +- No migration or generated schema-doc change is expected: all credential and uniqueness storage shipped + in #356/#532. + +## 6. Acceptance criteria + +1. A valid Owner with fresh step-up changes any same-account user's login email and receives 204. +2. All four identity columns contain the configured-normalizer result; a case-only edit is not treated as + a no-op, while an exact trimmed match has no credential or audit side effect. +3. A cross-account normalized duplicate succeeds; a same-account duplicate returns + `409 Users.DuplicateEmail`, including when two writes race at the unique index. +4. The target's pre-change access token fails on its next request, all old refresh tokens are unusable, + and an in-flight refresh cannot leave a usable child. Outstanding step-up grants are invalidated. +5. The new address can log in, the old address cannot, and the old address is released for another user + in the same farm. +6. A sole Owner cannot change their own address and the response says to add a second Owner first; with a + second active Owner, the self-change succeeds and follows the existing signed-out UX. +7. A foreign target id returns 404. A disabled target can be corrected without becoming enabled. +8. The audit event contains old and new addresses and occurs only for a real successful change. +9. The dedicated accessible SPA dialog performs step-up, clears password state, updates the row for a + non-self target, localizes all copy in `en`/`es`/`tl`, and binds duplicate conflict copy to the email + field. +10. Help and both glossaries state that no confirmation email is sent. Relevant backend, integration, + frontend, catalog-parity, typecheck, and build tests pass. +11. A friendly duplicate failure followed by an unrelated save on the same `AppDbContext` does not flush + the rejected email/user-name values or rotate either stamp. A final concurrency failure leaves neither + a changed epoch nor this operation's pending audit entry in the tracker. +12. The barrier-controlled refresh race presents the child minted on the far side of the change commit + and receives 401; inspecting `RevokedAt` alone is insufficient evidence. + +## 7. Merge evidence + +| Slot | Value | +|---|---| +| PR number + head SHA | pending | +| Driver-verified | pending | +| Implementer-attested, NOT driver-verified | pending | +| Reviewers run / rounds / stop-rule count | pending | +| Review threads: all dispositioned? | pending | +| Reviewers that never answered, and why | pending | +| Deferred findings + issue numbers | pending | +| Documentation surfaces rendered — locales + SHA | pending | +| Acceptance criteria: delivered or filed | pending | +| Retrospective | pending | +| Applied by, per increment + budget spent | pending | + +## 8. First moves + +Pending the locked design and increment plan. diff --git a/docs/plans/357-change-user-email/01-implementer-runbook.md b/docs/plans/357-change-user-email/01-implementer-runbook.md new file mode 100644 index 000000000..62ba498bd --- /dev/null +++ b/docs/plans/357-change-user-email/01-implementer-runbook.md @@ -0,0 +1,295 @@ +# Runbook — #357: let Owners change a user's login email + +You are the implementation owner in `/home/mforce/dev/cluckwork`. Execute +`docs/superpowers/plans/2026-08-25-change-user-email.md` from Task 0 through Task 3. This runbook is +controlling wherever it is more specific. Use strict RED → GREEN TDD, make the two planned commits, +push `feat/change-user-email`, and open the draft PR. Do not merge or mark it ready. + +## Rules + +- Read `AGENTS.md`, the driver handout, and the implementation plan completely before editing. +- Use the repository's `test-driven-development`/`superpowers:test-driven-development` workflow. +- The exact protected blocks below are correctness-critical. Transcribe them verbatim or stop and report; + never repair or reshape them. +- Add the named tests before the product code, run the focused RED, and record the stable reason. A + compile/discovery/Docker failure is not the expected RED. +- Do not add packages, migrations, schema changes, SMTP, verification mail, or unrelated refactors. +- Do not use `ChangeTracker.Clear()`, a uniqueness pre-read as race authority, a retry around the identity + transaction, or `IgnoreQueryFilters()` for user lookup. +- Do not change existing tests to accommodate the feature. A conflicting existing guard is a stop. +- The current checkout deliberately contains the two untracked driver artifacts listed below. Preserve + them; they are not product-code dirt. +- The configured `core.hooksPath` is stale (`/home/cesar/dev/cluckwork/.githooks`) on the driver host, + although the repository hook at `.githooks/pre-commit` builds the working tree for staged C# and runs + web typecheck for staged web files. Do not rewrite local Git config. The explicit gates below are the + authority, and no commit may contain a non-compiling tree. + +## Prerequisites and observed baseline + +Run first: + +```bash +git branch --show-current +git rev-parse HEAD +git status --short +git ls-files \ + src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailCommand.cs \ + src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailValidator.cs \ + src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailHandler.cs \ + tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailTests.cs \ + tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailRaceTests.cs +``` + +Expected: branch `main`, SHA `2c34771342cac2df26654ac90b2680567f2ffb1b`, status containing only +`docs/plans/357-change-user-email/` and +`docs/superpowers/plans/2026-08-25-change-user-email.md`, and no output from `git ls-files`. A SHA mismatch +is informational if the same approved plan artifacts are present; an existing create-target or unrelated +worktree change is a stop. + +Driver baseline on that SHA, 2026-08-26: + +- .NET SDK `10.0.111`, Node `v26.7.0`, npm `11.19.0`. +- locked restore passed; Release build: `0 Warning(s)`, `0 Error(s)`. +- full .NET suite: `Total tests: 1423`, `Passed: 1423`, `Test Run Successful.` +- schema-doc check: `docs/schema/ is up to date.` +- web coverage: `87 passed` files, `1940 passed` tests; coverage gates passed. +- web production build and `verify:sw` passed. +- npm and NuGet high+ vulnerability gates reported no advisories. + +Create the branch only after the checks: + +```bash +git switch -c feat/change-user-email +``` + +## Closed file scope + +Create/edit only the files enumerated by Tasks 1–3 of the implementation plan, plus these two controlling +artifacts: + +- `docs/plans/357-change-user-email/00-driver-handout.md` +- `docs/plans/357-change-user-email/01-implementer-runbook.md` +- `docs/superpowers/plans/2026-08-25-change-user-email.md` + +The exhaustive product/test allow-list is: the three new `ChangeUserEmail` application files; the two new +integration-test files; `IIdentityProvider.cs`, `AuditActions.cs`, `IdentityProvider.cs`, +`UserEndpoints.cs`, `CluckworkFeatureServiceCollectionExtensions.cs`; `web/src/api/cluckwork.ts`, +`UsersPage.tsx`, `UsersPage.test.tsx`, `HelpPage.tsx`, `HelpPage.test.tsx`, `web/src/i18n/enums.ts`, and the +three locale catalogs; `specs/product/GLOSSARY.md`; and the pinned tenant-bypass registry +`tests/Cluckwork.Application.Tests/TenantBypass/Data/filter-free-set-sites.tsv`. Anything else is a stop. + +`IdentityProvider.cs` line movement invalidates that registry even when the classified queries are +unchanged. Update only its exact `IdentityProvider.cs` line numbers to match the scanner output from +`TenantBypassRealTreeTests.RealSourceTree_FilterFreeSetSitesAreStableAndClassified`; preserve every detail +and reason verbatim, and classify any genuinely new candidate before proceeding. This is a pinned guard +artifact required by the existing test, not permission to edit the guard itself. + +Before changing `IIdentityProvider`, preserve every existing implementer/decorator found by: + +```bash +rg -l 'IIdentityProvider' src tests web --glob '!**/bin/**' --glob '!**/obj/**' +``` + +Before adding the audit action, run and save the reader enumeration: + +```bash +rg -l 'AuditActions|AUDIT_ACTION_VALUES|AUDIT_ACTION_KEYS|AUDIT_ACTION_ENTITY_TYPE' \ + src tests web --glob '!**/bin/**' --glob '!**/obj/**' +``` + +The direct registry readers that must move together are +`tests/Cluckwork.Application.Tests/Common/AuditVocabularyCoverageTests.cs`, +`web/src/i18n/enums.ts`, and the three locale catalogs. Do not edit unrelated audit call sites. + +## Increment 1 — backend contract and mutation + +Follow Task 1 exactly, including every named test and the deterministic race/tracker fixtures. The focused +RED command is: + +```bash +dotnet test tests/Cluckwork.Api.IntegrationTests/Cluckwork.Api.IntegrationTests.csproj \ + --filter 'FullyQualifiedName~ChangeUserEmail' --verbosity normal +``` + +Record separate RED evidence for route behavior and direct provider behavior; a missing type compile error +is allowed only during the test-authoring checkpoint and must not be committed. + +### PROTECTED — application shapes + +```csharp +public sealed record ChangeUserEmailCommand(Guid UserId, string Email, string? StepUpToken); + +public sealed class ChangeUserEmailHandler(IIdentityProvider identity, IStepUpGrantService stepUp) +{ + public async Task HandleAsync( + ChangeUserEmailCommand command, Guid accountId, Guid actingUserId, CancellationToken ct) + { + var proof = await stepUp.ValidateAsync(accountId, actingUserId, command.StepUpToken, ct); + if (!proof.IsSuccess) return proof; + return await identity.ChangeUserEmailAsync( + accountId, command.UserId, command.Email.Trim(), actingUserId, ct); + } +} +``` + +### PROTECTED — database duplicate discriminator + +```csharp +internal static bool IsUserEmailConflict(DbUpdateException exception) => + exception.InnerException is PostgresException + { + SqlState: PostgresErrorCodes.UniqueViolation, + ConstraintName: "EmailIndex" or "UserNameIndex" + }; +``` + +### PROTECTED — duplicate result and endpoint precedence + +The validator's `DuplicateEmail` and `DuplicateUserName`, and only the filtered database exception above, +map to: + +```csharp +Error.Conflict("Users.DuplicateEmail", "A user with this email already exists.") +``` + +In the endpoint result mapper, this literal branch must precede the suffix branch: + +```csharp +if (result.Error.Code == "Users.DuplicateEmail") + return Results.Problem(result.Error.Description, + statusCode: StatusCodes.Status409Conflict, title: result.Error.Code); +return result.Error.Code.EndsWith(".Conflict", StringComparison.Ordinal) + ? Results.Problem(result.Error.Description, + statusCode: StatusCodes.Status409Conflict, title: result.Error.Code) + : Results.Problem(result.Error.Description, statusCode: 422, title: result.Error.Code); +``` + +### Transaction invariants + +Inside one `AmbientTransaction.RunAsync` single attempt: lock the account unconditionally; verify the +locked account id; load target by `(Id, AccountId)`; re-read the actor as active Owner inside the lock; +perform trimmed ordinal no-op and sole-active-Owner self guard; snapshot all seven identity values plus +incoming entity state/property modified flags and pre-existing tracked audit instances; assign all four +email/login columns through configured normalizers; rotate the security/concurrency stamps; bump epoch +once; revoke all refresh tokens; write `AuditActions.UserEmailChanged` directly with +`new { oldEmail, newEmail }`; save and commit. Preserve `EmailConfirmed` and disabled fields. + +Every failure after snapshot must restore only this user's seven values and incoming flags, and detach only +this operation's newly-added `AuditEvent`. Expected `IdentityResult` failures are restored before return; +filtered duplicate and EF concurrency failures are restored before mapping; every other exception is +restored then rethrown. Do not clear, reload, or detach the target and do not disturb caller-tracked state. + +Run the focused backend and audit/validator guards from Task 1, then clean Release build. Commit exactly: + +```bash +git commit -m "feat: let owners change user email addresses" +``` + +## Increment 2 — dedicated SPA dialog and guidance + +Follow Task 2 exactly. Add all named tests first and record the focused RED. Supported locales are derived +from `web/src/i18n/index.ts` and parity-checked against the catalogs; the effective set is `en`, `es`, +`tl`. Use genuine translations. + +### PROTECTED — API client + +```typescript +export const changeUserEmail = ( + id: string, body: { email: string }, key?: string, stepUpToken?: string, +) => apiPut( + `/users/${id}/email`, body, key, + stepUpToken ? { [STEP_UP_HEADER]: stepUpToken } : undefined, +); +``` + +### PROTECTED — field accessibility + +```tsx +aria-invalid={emailFieldError !== null} +aria-describedby={emailFieldError ? emailErrorId : emailHintId} +``` + +Only a 409 titled `Users.DuplicateEmail` writes `emailFieldError`; generic failures remain in +`DialogError`. Capture the target id before the first await and gate every later state write on +`activeEmail.current`. Clear the password immediately before step-up. Close/target changes clear the active +target, password, field error, generic dialog attempt, and idempotency scope. + +Run Task 2's focused UI/i18n/doc checks, then commit exactly: + +```bash +git commit -m "feat(web): add change-email administration flow" +``` + +## Documentation/runtime surfaces + +| Surface | Path / key | Locales | Increment | Phase-11 evidence required | +|---|---|---|---|---| +| Users dialog | `UsersPage.tsx`, new Users keys | en/es/tl | 2 | Open Change email: row identity, hint says immediate next login/no confirmation email, field-local duplicate and ARIA wiring render | +| In-app Help | `HelpPage.tsx`, `glossaryLoginEmailTerm/Def`, roles guidance | en/es/tl | 2 | Render Help in each locale and verify the translated login-email row and no-confirmation rule | +| Product glossary | `specs/product/GLOSSARY.md` Login email | English | 2 | Render/read committed Markdown; verify farm-code login, immediate replacement, session termination, no confirmation mail | +| API metadata | `UserEndpoints.cs` ChangeUserEmail summary | API | 1 | Open generated OpenAPI document and verify route, PUT, body, and summary | +| Audit UI | `User.EmailChanged` enum/catalog labels | en/es/tl | 1/2 | Render an actual audit event after a change and verify localized label and User entity type | + +## Mutation checks + +After both increments are green, run every mutation row in the plan. Each guard mutant must compile, carry +a temporary `// MUTANT M:` marker, make its named test RED at the discriminating assertion, then be +restored and rebuilt/retested green. Run one at a time. Do not commit mutants. In particular, the tracker +restoration mutant is valid only if the same scoped `AppDbContext` performs the unrelated later save. + +M1 has two layers and uses a corrected protocol established by the first empirical run. Deleting the +explicit `NormalizedUserName` assignment is a **single-layer** mutant and must stay GREEN: the required +`UpdateSecurityStampAsync` call runs Identity's update pipeline and re-normalizes `UserName`, absorbing +that deletion. Then restore it and apply combined M1c: immediately after a successful stamp rotation, +assign `user.NormalizedUserName = null; // MUTANT M1c: corrupt final normalized user name after every +normalizer layer`. `Change_WritesAllFourColumnsThroughTheConfiguredNormalizer` must go RED on the final +`NormalizedUserName` assertion. Restore and prove that named test GREEN. The original M1 GREEN is not a +survivor; it is evidence that the second layer is live. This correction supersedes the first mutation row +in the implementation plan and records the observed mechanism rather than deleting the behavior guard. + +M3 is also layered. Removing either accepted constraint name while running +`ConcurrentSameFarmClaims_OneWins_AndLoserIsDuplicateEmail` is a **single-layer** mutant and must stay +GREEN: the unconditional account lock serializes the endpoint calls, so `AccountScopedUserValidator` +reports the loser before PostgreSQL's unique violation reaches the classifier. Restore between variants. +Then run each removal against the isolated-layer test +`IsUserEmailConflict_AcceptsOnlyTheTwoNamedUniqueConstraints`; each variant must go RED at the matching +positive assertion. That direct test is the guard for the exception-classifier layer. This correction +supersedes the third mutation row in the implementation plan. +Finish with: + +```bash +rg -n 'MUTANT|\[DEBUG-' src tests web +``` + +Expected: no output. A surviving mutant is a finding: report it and stop rather than inventing a fix—the +driver's shipped-fix budget is zero. + +## Full foreground gates and PR + +Run Task 3 plus the two CI self-tests and both blocking vulnerability gates: + +```bash +dotnet restore Cluckwork.sln --locked-mode +dotnet list package --vulnerable --include-transitive --format json --output-version 1 \ + | node .github/scripts/vuln-gate.mjs --ecosystem nuget --level high +dotnet build Cluckwork.sln --configuration Release --no-restore +dotnet test Cluckwork.sln --configuration Release --no-build --verbosity normal +tools/schema-docs/generate.sh --check +cd web +node --test ../.github/scripts/vuln-gate.test.mjs +node --test ../.github/scripts/lockfix.test.mjs +npm ci +npm audit --omit=dev --json \ + | node ../.github/scripts/vuln-gate.mjs --ecosystem npm --level high \ + --exceptions ../.github/security-exceptions.json +npm run test:coverage +npm run build +npm run verify:sw +cd .. +git diff --check +``` + +Push and open the draft PR exactly as Task 3 specifies. Include the two driver artifacts and this runbook +in the PR. Report branch, two commit SHAs, pushed head, PR URL/number, exact full-suite summaries, every +RED/GREEN checkpoint, every mutation result, runtime surfaces not yet verified, and any deviation. Do not +implement review fixes; return findings to the driver. diff --git a/docs/superpowers/plans/2026-08-25-change-user-email.md b/docs/superpowers/plans/2026-08-25-change-user-email.md new file mode 100644 index 000000000..d8751976c --- /dev/null +++ b/docs/superpowers/plans/2026-08-25-change-user-email.md @@ -0,0 +1,530 @@ +# Change User Email (#357) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development or +> superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax +> for tracking. The feature-driver's Phase 9 runbook is the controlling execution artifact when it is +> more prescriptive than this plan. + +**Goal:** Let an Owner safely correct a user's login email within one farm while terminating the target's +old credentials, preserving tenant isolation, and explaining the no-email-verification product contract. + +**Architecture:** A dedicated `ChangeUserEmail` application slice consumes step-up proof and calls a +narrow Identity port. `IdentityProvider` serializes on the account row, rechecks the actor, updates all +four Identity login columns through the configured normalizer, rotates the stamp, bumps the credential +epoch, revokes refresh tokens, and audits atomically. A dedicated Users-page dialog surfaces the +same-farm duplicate as an accessible field error and otherwise follows existing dialog/session patterns. + +**Tech Stack:** .NET 10, ASP.NET Core Identity, EF Core 10, Npgsql/PostgreSQL, FluentValidation, xUnit, +React 19, TypeScript 7, Vitest, Testing Library, i18next. + +## Global Constraints + +- Follow `AGENTS.md`, especially account-scoped Identity uniqueness, `SingleAttemptExecution`, direct + `AuditActions` call-site syntax, write-contract caller review, and same-PR Help/glossary/locales. +- No migration or schema-doc regeneration: #356/#532 already shipped every required column and index. +- `PUT /api/v1/users/{id}/email`: Owner-only group, unconditional step-up, `Idempotency-Key`, 2048-byte + body cap, 204 success/no-op, 403 proof/stale actor, 404 foreign id, 409 duplicate/concurrency, 422 sole + Owner refusal. +- Per-account uniqueness only. A normalized duplicate in another account must succeed. +- Exact equality is `StringComparison.Ordinal` after trimming input; case-only changes are real changes. +- A real change preserves `EmailConfirmed` and disabled state, rotates `SecurityStamp` and + `ConcurrencyStamp`, increments `CredentialEpoch` exactly once, revokes every active refresh token, + and audits `{ oldEmail, newEmail }`. +- Catch a database duplicate only for PostgreSQL SQLSTATE `23505` plus constraint `EmailIndex` or + `UserNameIndex`. Propagate every other `DbUpdateException`. +- Never clear the whole EF tracker. A failed operation restores only values/state it changed and detaches + only its own unsaved audit event. +- TDD: run each named test red for the stated reason before product code that makes it green. Record the + red reason and final summary for the implementer report. +- Warnings are errors; nullable and unused usings are build-breaking. Do not add dependencies. + +--- + +### Task 0: Baseline and branch + +**Files:** none. + +- [ ] **Step 1: Confirm the create targets do not exist.** + +Run: + +```bash +git ls-files \ + src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailCommand.cs \ + src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailValidator.cs \ + src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailHandler.cs \ + tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailTests.cs \ + tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailRaceTests.cs +``` + +Expected: no output. If any path exists, stop and report instead of overwriting it. + +- [ ] **Step 2: Record a clean baseline.** + +Run with Docker available: + +```bash +dotnet build Cluckwork.sln -c Release +dotnet test Cluckwork.sln -c Release --no-build --verbosity normal +cd web && npm test -- --run && npm run typecheck && cd .. +``` + +Expected: build ends `0 Warning(s) 0 Error(s)`; all tests/typecheck pass. Record exact totals and any +pre-existing failure. Do not repair a baseline failure without owner scope approval. + +- [ ] **Step 3: Create the feature branch.** + +```bash +git switch -c feat/change-user-email +``` + +Expected: branch is `feat/change-user-email`, based on the recorded baseline SHA. + +--- + +### Task 1: Backend vertical slice — contract, Identity mutation, and HTTP evidence + +**Files:** + +- Create: `src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailCommand.cs` +- Create: `src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailValidator.cs` +- Create: `src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailHandler.cs` +- Create: `tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailTests.cs` +- Create: `tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailRaceTests.cs` +- Modify: `src/Cluckwork.Application/Common/IIdentityProvider.cs` +- Modify: `src/Cluckwork.Application/Common/AuditActions.cs` +- Modify: `src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs` +- Modify: `src/Cluckwork.Api/Endpoints/Users/UserEndpoints.cs` +- Modify: `src/Cluckwork.Api/Hosting/CluckworkFeatureServiceCollectionExtensions.cs` +- Modify: `tests/Cluckwork.Application.Tests/TenantBypass/Data/filter-free-set-sites.tsv` +- Modify: `web/src/i18n/enums.ts` +- Modify: `web/src/i18n/en.ts` +- Modify: `web/src/i18n/es.ts` +- Modify: `web/src/i18n/tl.ts` + +**Interfaces:** + +- Produces: `ChangeUserEmailCommand(Guid UserId, string Email, string? StepUpToken)`. +- Produces: `ChangeUserEmailHandler.HandleAsync(ChangeUserEmailCommand command, Guid accountId, + Guid actingUserId, CancellationToken ct) -> Task`. +- Produces: `IIdentityProvider.ChangeUserEmailAsync(Guid accountId, Guid userId, string email, + Guid actingUserId, CancellationToken ct = default) -> Task`. +- Produces: `AuditActions.UserEmailChanged = "User.EmailChanged"` and matching SPA audit-enum entries. +- Produces: `PUT /api/v1/users/{id}/email` request `{ email }` and status contract from Global Constraints. + +- [ ] **Step 1: Write the endpoint/Identity tests before adding the route.** + +`ChangeUserEmailTests.cs` uses the HTTP fixture patterns in `ChangeUserRoleTests.cs`. Add these named facts +with positive controls rather than broad status-only assertions: + +```csharp +[Fact] public async Task Change_WritesAllFourColumnsThroughTheConfiguredNormalizer() +[Fact] public async Task CrossAccountDuplicate_SucceedsInBothFarms() +[Fact] public async Task SameAccountDuplicate_Is409UsersDuplicateEmail() +[Fact] public async Task Change_BumpsEpochOnce_AndKillsTheLiveAccessAndRefreshTokens() +[Fact] public async Task NewEmailLogsIn_OldEmailFails_AndOldEmailCanBeReusedInTheFarm() +[Fact] public async Task ExactTrimmedNoOp_LeavesEpochStampsTokensAndAuditUnchanged() +[Fact] public async Task CaseOnlyCorrection_IsARealChange() +[Fact] public async Task SoleOwnerSelfChange_Is422_AndNamesAddingASecondOwner() +[Fact] public async Task SelfChange_WithAnotherActiveOwner_Succeeds() +[Fact] public async Task ForeignUserId_Is404_AndForeignRowIsUnchanged() +[Fact] public async Task DisabledTarget_CanBeCorrected_AndStaysDisabled() +[Fact] public async Task Change_PreservesEmailConfirmed() +[Fact] public async Task Change_AuditsOldAndNewEmail_OnlyOnARealChange() +[Fact] public async Task Change_InvalidatesTheTargetsOutstandingStepUpGrant() +[Fact] public async Task MissingStepUp_Is403_AndLeavesIdentityColumnsUnchanged() +[Fact] public async Task InvalidEmail_Is400WithEmailFieldCode() +[Fact] public async Task OversizedBody_Is413() +``` + +For the four-column assertion, use `factory.WithTenantScopeAsync(accountId, ...)` and still predicate the +filter-free `db.Users` set by both target id and `AccountId`; do not use `IgnoreQueryFilters()`. Resolve +`UserManager` in that scope and derive the normalized expectations before asserting: + +```csharp +var expectedEmail = "Case.Change@Farm.Test"; +var expectedNormalizedEmail = userManager.NormalizeEmail(expectedEmail); +var expectedNormalizedUserName = userManager.NormalizeName(expectedEmail); +Assert.Equal(expectedEmail, row.Email); +Assert.Equal(expectedNormalizedEmail, row.NormalizedEmail); +Assert.Equal(expectedEmail, row.UserName); +Assert.Equal(expectedNormalizedUserName, row.NormalizedUserName); +``` + +- [ ] **Step 2: Write the adversarial transaction/race tests before implementation.** + +`ChangeUserEmailRaceTests.cs` reuses the account-fence and handler-resolution shapes from +`DisableUserRaceTests.cs`/`ChangeUserRoleRaceTests.cs`. Add: + +```csharp +[Fact] public async Task RefreshInFlightAcrossEmailChange_LeavesNoUsableChild() +[Fact] public async Task ConcurrentSameFarmClaims_OneWins_AndLoserIsDuplicateEmail() +[Fact] public async Task ConcurrentStampChange_DuringEmailMutation_Is409() +[Fact] public async Task DuplicateFailure_RestoresTrackedFieldsAndStampsBeforeLaterSave() +[Fact] public async Task FinalConcurrencyFailure_RemovesEpochAndPendingAuditFromTracker() +[Fact] public async Task DisabledCoOwner_DoesNotSatisfySoleOwnerSelfChangeGuard() +[Fact] public async Task ActorDemotedWhileQueued_Is403_AndTargetIsUnchanged() +[Fact] public void IsUserEmailConflict_AcceptsOnlyTheTwoNamedUniqueConstraints() +``` + +The refresh test must deterministically park the email change on the account fence, mint a child from the +pre-change session, release the fence, await the successful change, then present both child faces: + +```csharp +Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.CreateAuthedClient(child.AccessToken).GetAsync("/api/v1/flocks")).StatusCode); +Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.CreateClient().PostRefreshAsync( + child.RefreshToken, expectedAccount: accountId.ToString())).StatusCode); +``` + +The tracker test must keep the same DI scope/`AppDbContext`, receive `Users.DuplicateEmail`, modify and +save an unrelated entity, then query through a fresh scope and prove the old email and both old stamps +remain. A test that disposes the failed context before the later save is invalid. + +- [ ] **Step 3: Run the new tests red.** + +```bash +dotnet test tests/Cluckwork.Api.IntegrationTests/Cluckwork.Api.IntegrationTests.csproj \ + --filter 'FullyQualifiedName~ChangeUserEmail' --verbosity normal +``` + +Expected RED: the route returns 404 and direct handler/provider references do not yet compile. Record +those reasons. A database/Docker startup failure is not the required red. + +- [ ] **Step 4: Add the application contract and validation.** + +Use these exact public shapes: + +```csharp +public sealed record ChangeUserEmailCommand(Guid UserId, string Email, string? StepUpToken); + +public sealed class ChangeUserEmailHandler(IIdentityProvider identity, IStepUpGrantService stepUp) +{ + public async Task HandleAsync( + ChangeUserEmailCommand command, Guid accountId, Guid actingUserId, CancellationToken ct) + { + var proof = await stepUp.ValidateAsync(accountId, actingUserId, command.StepUpToken, ct); + if (!proof.IsSuccess) return proof; + return await identity.ChangeUserEmailAsync( + accountId, command.UserId, command.Email.Trim(), actingUserId, ct); + } +} +``` + +The validator applies required, `EmailAddress()`, and maximum 256 rules with the existing dotted codes +`User.Email.Required`, `User.Email.Format`, and `User.Email.MaxLength`. Register validator and handler in +`CluckworkFeatureServiceCollectionExtensions.AddValidators/AddHandlers`. + +- [ ] **Step 5: Add the narrow Identity port, audit vocabulary, and direct registry readers.** + +Add the method signature above to `IIdentityProvider`. Add `UserEmailChanged` directly to `AuditActions`. +Update all three audit label catalogs and the three named `web/src/i18n/enums.ts` structures: +`AUDIT_ACTION_VALUES`, `AUDIT_ACTION_KEYS`, and `AUDIT_ACTION_ENTITY_TYPE`, so the value, translation +key, entity type, and parity tests remain 1:1. Do not forward the audit action through a helper parameter; +`AuditVocabularyCoverageTests` accepts the direct constant at the write call. + +- [ ] **Step 6: Implement the Identity transaction.** + +Follow the locked order in `docs/plans/357-change-user-email/00-driver-handout.md`: ambient single attempt, +unconditional account lock, scoped target lookup, active-Owner recheck, exact no-op, sole-Owner guard, +snapshot, four-column assignment through `userManager.NormalizeEmail/NormalizeName`, stamp rotation, +epoch increment, full refresh revocation, direct audit, final save, commit. + +The database-race discriminator is `internal static` so the integration-test assembly can exercise it +through the existing `InternalsVisibleTo` seam, matching `AccountProvisioner.IsSlugConflict`: + +```csharp +internal static bool IsUserEmailConflict(DbUpdateException exception) => + exception.InnerException is PostgresException + { + SqlState: PostgresErrorCodes.UniqueViolation, + ConstraintName: "EmailIndex" or "UserNameIndex" + }; +``` + +Map validator errors `DuplicateEmail` or `DuplicateUserName` to: + +```csharp +Error.Conflict("Users.DuplicateEmail", "A user with this email already exists.") +``` + +Before assigning any email/login field and before calling `UpdateSecurityStampAsync`, snapshot +`Email`, `NormalizedEmail`, `UserName`, `NormalizedUserName`, `SecurityStamp`, `ConcurrencyStamp`, and +`CredentialEpoch`, plus incoming `EntityState` and per-property modified flags. Snapshot the set of +already-tracked `AuditEvent` instances before writing this operation's audit. On failure, restore those +exact user values/flags and detach only the newly Added matching audit entry. Do not call +`ChangeTracker.Clear()` and do not disturb caller entries. Preserve `EmailConfirmed` without assigning it. + +- [ ] **Step 7: Wire the HTTP endpoint.** + +Add: + +```csharp +group.MapPut("/{id:guid}/email", ChangeUserEmail) + .WithMaxRequestBodyBytes(2048) + .WithName("ChangeUserEmail") + .WithSummary("Change a user's login email and revoke their sessions."); + +public sealed record ChangeUserEmailRequest(string Email); +``` + +The delegate builds the command from the route, body, and step-up header; validates it; invokes the +handler with tenant/actor ids; returns 204/403/404/409/422 exactly as specified. Match +`result.Error.Code == "Users.DuplicateEmail"` literally for 409 before the sibling +`EndsWith(".Conflict", StringComparison.Ordinal)` concurrency branch; `Users.DuplicateEmail` does not +carry the suffix and would otherwise fall into the trailing 422. Do not add a self-target HTTP +short-circuit: the provider's in-lock active-Owner count decides whether self-change is safe. The 413 is +produced by `.WithMaxRequestBodyBytes(2048)` before the delegate runs; do not add a second manual +body-length check. + +- [ ] **Step 8: Run focused backend tests green, then the backend safety guards.** + +```bash +dotnet test tests/Cluckwork.Api.IntegrationTests/Cluckwork.Api.IntegrationTests.csproj \ + --filter 'FullyQualifiedName~ChangeUserEmail' --verbosity normal +dotnet test tests/Cluckwork.Application.Tests/Cluckwork.Application.Tests.csproj \ + --filter 'FullyQualifiedName~AuditVocabularyCoverageTests|FullyQualifiedName~ValidatorErrorCodeCoverageTests' \ + --verbosity normal +dotnet build Cluckwork.sln -c Release +``` + +Expected: focused tests pass; build ends `0 Warning(s) 0 Error(s)`. + +- [ ] **Step 9: Commit the backend vertical slice.** + +```bash +git add src/Cluckwork.Application src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs \ + src/Cluckwork.Api/Endpoints/Users/UserEndpoints.cs \ + src/Cluckwork.Api/Hosting/CluckworkFeatureServiceCollectionExtensions.cs \ + tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailTests.cs \ + tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailRaceTests.cs \ + web/src/i18n/enums.ts web/src/i18n/en.ts web/src/i18n/es.ts web/src/i18n/tl.ts +git commit -m "feat: let owners change user email addresses" +``` + +--- + +### Task 2: Users-page dialog, field error, and localized no-verification guidance + +**Files:** + +- Modify: `web/src/api/cluckwork.ts` +- Modify: `web/src/routes/UsersPage.tsx` +- Modify: `web/src/routes/UsersPage.test.tsx` +- Modify: `web/src/routes/HelpPage.tsx` +- Modify: `web/src/routes/HelpPage.test.tsx` +- Modify: `web/src/i18n/en.ts` +- Modify: `web/src/i18n/es.ts` +- Modify: `web/src/i18n/tl.ts` +- Modify: `specs/product/GLOSSARY.md` + +**Interfaces:** + +- Produces: `changeUserEmail(id, { email }, key?, stepUpToken?) -> Promise`. +- Produces: dedicated Change-email row action/dialog with `emailFieldError` and `activeEmail` late-response + protection. +- Produces: Help roles guidance plus an in-app glossary row and product glossary definition explaining + immediate login-address replacement and no confirmation email. + +- [ ] **Step 1: Write the SPA tests red before importing the new client function.** + +Extend the `vi.mock("../api/cluckwork")` contract and add these facts to `UsersPage.test.tsx`: + +```typescript +it("opens a dedicated Change email dialog for the selected row") +it("requires step-up, clears the password, and sends trimmed email with one idempotency key") +it("renders Users.DuplicateEmail beside the email input with aria-invalid") +it("clears the field error on edit, close, and target change") +it("does not attach a late duplicate response to a reopened dialog") +it("a successful non-self change refreshes the row and reports the new email") +it("does not claim that a confirmation email will be sent") +``` + +The conflict test returns `new ApiError(409, "Users.DuplicateEmail", "A user with this email already exists.")`, then asserts the dialog's +email input has `aria-invalid="true"`, `aria-describedby` points to the visible translated field error, +and the generic `` is empty. The late-response test uses a controllable promise and switches +dialog target before resolving it. + +- [ ] **Step 2: Write Help/glossary tests red.** + +Add tests to `HelpPage.test.tsx` that override the new English term/definition keys and prove the rendered +row reads from i18n, then assert all three locales have non-English-placeholder definitions containing +their equivalent of “no confirmation email.” Add a product-glossary assertion only if an existing test +already reads `specs/product/GLOSSARY.md`; otherwise verify that file in Step 7 with `rg` rather than +creating a new filesystem-coupled Vitest test. + +- [ ] **Step 3: Run the frontend tests red.** + +```bash +cd web +npm test -- --run src/routes/UsersPage.test.tsx src/routes/HelpPage.test.tsx +``` + +Expected RED: no Change-email control/client mock and no new Help glossary row. An i18n initialization +failure is not the required red. + +- [ ] **Step 4: Add the API client.** + +```typescript +export const changeUserEmail = ( + id: string, body: { email: string }, key?: string, stepUpToken?: string, +) => apiPut( + `/users/${id}/email`, body, key, + stepUpToken ? { [STEP_UP_HEADER]: stepUpToken } : undefined, +); +``` + +- [ ] **Step 5: Implement the dedicated dialog using existing primitives.** + +Add a row action distinct from Edit name. State includes target user, working email, current password, +`emailFieldError`, and `activeEmail`. The submit flow is: + +```typescript +errors.beginAttempt("change-email"); +setEmailFieldError(null); +const password = emailStepUpPassword; +setEmailStepUpPassword(""); +const grant = await stepUp(password); +await changeUserEmail(target.id, { email: emailValue.trim() }, keyFor(scope), grant.token); +``` + +Capture `const targetId = target.id` before the first await and guard every post-await write with +`if (activeEmail.current !== targetId) return;`; opening sets `activeEmail.current` synchronously, and +close sets it to null before clearing state. Use `try/catch` with that exact active-target check. Only +`err instanceof ApiError && err.status === 409 && err.title === "Users.DuplicateEmail"` writes +`emailFieldError`; all other errors go through `errors.report("change-email", errText(err))`. On input +change clear the field error. On close/logout/target change clear password, field error, idempotency scope, +and active target consistently with neighboring dialogs. + +Use `useId()` for hint/error ids. The input must carry: + +```tsx +aria-invalid={emailFieldError !== null} +aria-describedby={emailFieldError ? emailErrorId : emailHintId} +``` + +Render the field error as ``. Keep +`DialogError` for non-field failures. The hint says the new address is the next login and no confirmation +email is sent. The acting Owner's password is `autoComplete="current-password"`. + +- [ ] **Step 6: Add all user-facing copy in English, Spanish, and Tagalog.** + +Add Users namespace keys for action, title, hint, email/password labels, submit/success, duplicate-field +message, and sole-Owner refusal if the API description is surfaced. Add Help roles guidance and +`glossaryLoginEmailTerm`/`glossaryLoginEmailDef` in all three catalogs. Add the corresponding table row in +`HelpPage.tsx`. Spanish and Tagalog must be genuine translations, not English placeholders. + +Update `specs/product/GLOSSARY.md` with “Login email”: the address used with a farm code to sign in; an +Owner can replace it immediately; old sessions end; no confirmation email is sent. + +- [ ] **Step 7: Run focused UI, i18n, and documentation checks green.** + +```bash +cd web +npm test -- --run src/routes/UsersPage.test.tsx src/routes/HelpPage.test.tsx \ + src/i18n/catalogParity.test.ts src/i18n/enums.test.ts +npm run typecheck +npm run i18n:scan +cd .. +rg -n 'Login email|confirmation email' specs/product/GLOSSARY.md +``` + +Expected: all tests/typecheck/scan pass and the product glossary names both the term and no-email rule. + +- [ ] **Step 8: Commit the SPA and documentation slice.** + +```bash +git add web/src/api/cluckwork.ts web/src/routes/UsersPage.tsx web/src/routes/UsersPage.test.tsx \ + web/src/routes/HelpPage.tsx web/src/routes/HelpPage.test.tsx \ + web/src/i18n/en.ts web/src/i18n/es.ts web/src/i18n/tl.ts specs/product/GLOSSARY.md +git commit -m "feat(web): add change-email administration flow" +``` + +--- + +### Task 3: Full verification and draft PR + +**Files:** + +- Modify: `docs/plans/357-change-user-email/00-driver-handout.md` only as directed by the driver/runbook. +- Modify: `docs/superpowers/plans/2026-08-25-change-user-email.md` only to tick executed checkboxes if the + Phase 9 runbook asks for it. + +- [ ] **Step 1: Check scope and generated-artifact expectations.** + +```bash +git status --short +git diff --check +git diff --name-only HEAD~2 +git diff --name-only HEAD~2 | rg 'Migrations|docs/schema' && exit 1 || true +rg -n 'MUTANT|\[DEBUG-' src tests web || true +``` + +Expected: only allowed implementation/plan/doc paths; no migration/schema changes; no mutation/debug tags. + +- [ ] **Step 2: Run the CI-equivalent backend gates in the foreground.** + +```bash +dotnet restore Cluckwork.sln --locked-mode +dotnet build Cluckwork.sln --configuration Release --no-restore +dotnet test Cluckwork.sln --configuration Release --no-build --verbosity normal +tools/schema-docs/generate.sh --check +``` + +Expected: restore/build/test/schema check all pass; record the full final test summary, not a background +process status. + +- [ ] **Step 3: Run the CI-equivalent frontend gates in the foreground.** + +```bash +cd web +npm ci +npm run test:coverage +npm run build +npm run verify:sw +cd .. +``` + +Expected: coverage gate, TypeScript/Vite build, and service-worker verification pass. + +- [ ] **Step 4: Push and open the draft PR before review.** + +```bash +git push -u origin feat/change-user-email +gh pr create --draft \ + --title "feat: let owners change user email addresses" \ + --body-file docs/plans/357-change-user-email/00-driver-handout.md +``` + +Expected: draft PR exists at the exact pushed head. Report branch, head SHA, PR URL, per-commit applied-by +attribution, red/green summaries, and anything not run. Do not mark ready and do not merge. + +## Mutation rows reserved for driver Phase 11 + +| Mutant | Named test that must go red | +|---|---| +| Single-layer: delete explicit `NormalizedUserName` assignment; expect GREEN because `UpdateSecurityStampAsync` re-normalizes `UserName`. Then combined M1c: set `NormalizedUserName = null` immediately after successful stamp rotation; expect RED | `Change_WritesAllFourColumnsThroughTheConfiguredNormalizer` | +| Remove `AccountId` from target lookup | `ForeignUserId_Is404_AndForeignRowIsUnchanged` | +| Single-layer: remove either accepted constraint name and expect the concurrent-claim test GREEN because account-lock + validator absorb it; isolated-layer: run each removal against the direct classifier test and expect RED | `ConcurrentSameFarmClaims_OneWins_AndLoserIsDuplicateEmail`; `IsUserEmailConflict_AcceptsOnlyTheTwoNamedUniqueConstraints` | +| Broaden `IsUserEmailConflict` beyond the two named unique constraints | `IsUserEmailConflict_AcceptsOnlyTheTwoNamedUniqueConstraints` | +| Delete `CredentialEpoch++` | `Change_BumpsEpochOnce_AndKillsTheLiveAccessAndRefreshTokens` and the refresh-race child assertions | +| Delete `UpdateSecurityStampAsync` | `Change_InvalidatesTheTargetsOutstandingStepUpGrant` | +| Remove tracker restoration | `DuplicateFailure_RestoresTrackedFieldsAndStampsBeforeLaterSave` | +| Count disabled co-Owner as active | `DisabledCoOwner_DoesNotSatisfySoleOwnerSelfChangeGuard` | +| Treat normalized equality as no-op | `CaseOnlyCorrection_IsARealChange` | +| Remove `activeEmail` settle guard | `does not attach a late duplicate response to a reopened dialog` | +| Route duplicate through `DialogError` | `renders Users.DuplicateEmail beside the email input with aria-invalid` | +| Delete Help-page glossary row | Help override/wiring test added in Task 2 | + +## Self-review + +- Spec coverage: API/auth/body cap → Task 1; per-account duplicates/four columns → Task 1; epoch, refresh, + stamp, no-op, sole Owner, disabled target, foreign id, audit → Task 1; barrier and tracker false-green + cases → Task 1; SPA field error/accessibility → Task 2; `en`/`es`/`tl`, Help, in-app/product glossaries + → Task 2; CI/PR evidence → Task 3. No acceptance criterion is unassigned. +- Placeholder scan: clean; every code and test step names its exact implementation or assertion. +- Type consistency: the command, handler, port, request body, client body, audit value, and error codes are + named once and consumed unchanged. +- Scope: no migration, SMTP, email verification, display-name refactor, shared dialog-error redesign, or + unrelated credential work. diff --git a/specs/product/GLOSSARY.md b/specs/product/GLOSSARY.md index 346ef3d66..29947d8b0 100644 --- a/specs/product/GLOSSARY.md +++ b/specs/product/GLOSSARY.md @@ -623,6 +623,12 @@ Accepted disclosure: on a shared device the cached list is a durable roster of which farms that browser profile uses — now revocable entry by entry, tracked in #587, with the ADR revision owned by #537. +**Login email (#357)** — the address used with a farm code to sign in. An Owner +can replace it immediately from the Users screen; the new address becomes the +next login, the old address stops working, the user's open sessions end, and no +confirmation email is sent. Changing an Owner's own address is allowed only +while another active Owner remains on the farm. + **Farm provisioning (#533)** — the offline `provision-account` operator command creates a new account, its ten canonical egg grades, its six packed-unit conversions, and its first Owner as one transaction. The Owner receives a diff --git a/src/Cluckwork.Api/Endpoints/Users/UserEndpoints.cs b/src/Cluckwork.Api/Endpoints/Users/UserEndpoints.cs index 9467cf28b..7f2231e17 100644 --- a/src/Cluckwork.Api/Endpoints/Users/UserEndpoints.cs +++ b/src/Cluckwork.Api/Endpoints/Users/UserEndpoints.cs @@ -6,6 +6,7 @@ namespace Cluckwork.Api.Endpoints.Users; using Cluckwork.Application.Features.Users; using Cluckwork.Application.Features.Users.AssignFlock; using Cluckwork.Application.Features.Users.ChangeUserRole; +using Cluckwork.Application.Features.Users.ChangeUserEmail; using Cluckwork.Application.Features.Users.CreateUser; using Cluckwork.Application.Features.Users.DisableUser; using Cluckwork.Application.Features.Users.EnableUser; @@ -65,6 +66,11 @@ public static RouteGroupBuilder MapUserEndpoints(this RouteGroupBuilder group) .WithName("ChangeUserRole") .WithSummary("Change a user's role (promote/demote) and revoke their sessions."); + group.MapPut("/{id:guid}/email", ChangeUserEmail) + .WithMaxRequestBodyBytes(2048) + .WithName("ChangeUserEmail") + .WithSummary("Change a user's login email and revoke their sessions."); + // #356 — disable / re-enable. Separate verbs (following // POST /products/{id}/deactivate) rather than a PUT carrying a boolean: // the two directions are not symmetric on the CREDENTIAL EPOCH — a @@ -290,6 +296,39 @@ private static async Task ChangeUserRole( : Results.Problem(result.Error.Description, statusCode: 422, title: result.Error.Code); } + private static async Task ChangeUserEmail( + Guid id, + ChangeUserEmailRequest request, + [FromHeader(Name = Cluckwork.Api.Endpoints.Auth.AuthEndpoints.StepUpHeaderName)] string? stepUpToken, + ChangeUserEmailHandler handler, + IValidator validator, + TenantContext tenant, + ICurrentUser currentUser, + CancellationToken ct) + { + if (!tenant.IsResolved || !currentUser.IsResolved) return Results.Unauthorized(); + + var command = new ChangeUserEmailCommand(id, request.Email, stepUpToken); + var validation = await validator.ValidateAsync(command, ct); + if (!validation.IsValid) + return ValidationResponse.Problem(validation); + + var result = await handler.HandleAsync(command, tenant.AccountId, currentUser.UserId, ct); + if (result.IsSuccess) return Results.NoContent(); + if (result.Error.Code is Cluckwork.Application.Common.StepUpErrorCodes.Required or "Auth.Forbidden") + return Results.Problem(result.Error.Description, + statusCode: StatusCodes.Status403Forbidden, title: result.Error.Code); + if (result.Error.Code.EndsWith(".NotFound", StringComparison.Ordinal)) + return Results.NotFound(); + if (result.Error.Code == "Users.DuplicateEmail") + return Results.Problem(result.Error.Description, + statusCode: StatusCodes.Status409Conflict, title: result.Error.Code); + return result.Error.Code.EndsWith(".Conflict", StringComparison.Ordinal) + ? Results.Problem(result.Error.Description, + statusCode: StatusCodes.Status409Conflict, title: result.Error.Code) + : Results.Problem(result.Error.Description, statusCode: 422, title: result.Error.Code); + } + private static async Task DisableUser( Guid id, DisableUserRequest? request, @@ -381,6 +420,8 @@ public sealed record SetUserPasswordRequest(string NewPassword); public sealed record ChangeUserRoleRequest(string Role); +public sealed record ChangeUserEmailRequest(string Email); + // #356 — the body is optional: an EMPTY application/json body binds null and // means "no reason given" (pinned by Disable_WithNoBodyAtAll_Is204). Sending // no Content-Type at all is not the same thing — the body parameter gives diff --git a/src/Cluckwork.Api/Hosting/CluckworkFeatureServiceCollectionExtensions.cs b/src/Cluckwork.Api/Hosting/CluckworkFeatureServiceCollectionExtensions.cs index 9a73f60ab..4dc360608 100644 --- a/src/Cluckwork.Api/Hosting/CluckworkFeatureServiceCollectionExtensions.cs +++ b/src/Cluckwork.Api/Hosting/CluckworkFeatureServiceCollectionExtensions.cs @@ -235,6 +235,9 @@ private static void AddValidators(IServiceCollection services) services.AddScoped< IValidator, Cluckwork.Application.Features.Users.ChangeUserRole.ChangeUserRoleValidator>(); + services.AddScoped< + IValidator, + Cluckwork.Application.Features.Users.ChangeUserEmail.ChangeUserEmailValidator>(); // #356 — disable carries an optional reason; enable carries no body at // all and therefore has no validator. services.AddScoped< @@ -315,6 +318,8 @@ private static void AddHandlers(IServiceCollection services) Cluckwork.Application.Features.Users.ChangeOwnPassword.ChangeOwnPasswordHandler>(); services.AddScoped< Cluckwork.Application.Features.Users.ChangeUserRole.ChangeUserRoleHandler>(); + services.AddScoped< + Cluckwork.Application.Features.Users.ChangeUserEmail.ChangeUserEmailHandler>(); services.AddScoped< Cluckwork.Application.Features.Users.DisableUser.DisableUserHandler>(); services.AddScoped< diff --git a/src/Cluckwork.Application/Common/AuditActions.cs b/src/Cluckwork.Application/Common/AuditActions.cs index 893394dbb..24853b942 100644 --- a/src/Cluckwork.Application/Common/AuditActions.cs +++ b/src/Cluckwork.Application/Common/AuditActions.cs @@ -39,6 +39,7 @@ public static class AuditActions public const string UserPasswordSet = "User.PasswordSet"; public const string UserPasswordChanged = "User.PasswordChanged"; public const string UserRoleChanged = "User.RoleChanged"; + public const string UserEmailChanged = "User.EmailChanged"; public const string UserDisabled = "User.Disabled"; public const string UserEnabled = "User.Enabled"; public const string UserBreakGlassReset = "User.BreakGlassReset"; diff --git a/src/Cluckwork.Application/Common/IIdentityProvider.cs b/src/Cluckwork.Application/Common/IIdentityProvider.cs index a0529ce81..9981a9e36 100644 --- a/src/Cluckwork.Application/Common/IIdentityProvider.cs +++ b/src/Cluckwork.Application/Common/IIdentityProvider.cs @@ -88,6 +88,20 @@ Task SetUserPasswordAsync( Task ChangeUserRoleAsync( Guid accountId, Guid userId, string? role, Guid actingUserId, CancellationToken ct = default); + // #357 — change an existing user's login email, account-scoped (foreign + // id -> NotFound). `actingUserId` is re-verified INSIDE the locked + // transaction to still be an active, non-disabled Owner because their + // role can change while this operation waits on the account-wide lock. + // A trimmed ordinal no-op has no side effects. A real change updates all + // four email/user-name columns through the configured normalizers, rotates + // both Identity stamps, bumps CredentialEpoch exactly once, revokes every + // refresh token, and writes User.EmailChanged in the same transaction. + // Changing the last active Owner's own email fails with Users.LastOwner; + // same-account duplicates fail with Users.DuplicateEmail; concurrent + // Identity writes fail with Users.Conflict. + Task ChangeUserEmailAsync( + Guid accountId, Guid userId, string email, Guid actingUserId, CancellationToken ct = default); + // #356 — disable a user, account-scoped (foreign id -> NotFound). The // DisabledAt flag is only half of it: CredentialEpochMiddleware (#364) // already refuses a disabled user, but a flag ALONE means re-enabling diff --git a/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailCommand.cs b/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailCommand.cs new file mode 100644 index 000000000..ab1ca125d --- /dev/null +++ b/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailCommand.cs @@ -0,0 +1,3 @@ +namespace Cluckwork.Application.Features.Users.ChangeUserEmail; + +public sealed record ChangeUserEmailCommand(Guid UserId, string Email, string? StepUpToken); diff --git a/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailHandler.cs b/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailHandler.cs new file mode 100644 index 000000000..ea554a4e1 --- /dev/null +++ b/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailHandler.cs @@ -0,0 +1,16 @@ +namespace Cluckwork.Application.Features.Users.ChangeUserEmail; + +using Cluckwork.Application.Common; +using Cluckwork.Domain.Common; + +public sealed class ChangeUserEmailHandler(IIdentityProvider identity, IStepUpGrantService stepUp) +{ + public async Task HandleAsync( + ChangeUserEmailCommand command, Guid accountId, Guid actingUserId, CancellationToken ct) + { + var proof = await stepUp.ValidateAsync(accountId, actingUserId, command.StepUpToken, ct); + if (!proof.IsSuccess) return proof; + return await identity.ChangeUserEmailAsync( + accountId, command.UserId, command.Email.Trim(), actingUserId, ct); + } +} diff --git a/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailValidator.cs b/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailValidator.cs new file mode 100644 index 000000000..9b3599d52 --- /dev/null +++ b/src/Cluckwork.Application/Features/Users/ChangeUserEmail/ChangeUserEmailValidator.cs @@ -0,0 +1,18 @@ +namespace Cluckwork.Application.Features.Users.ChangeUserEmail; + +using FluentValidation; + +public sealed class ChangeUserEmailValidator : AbstractValidator +{ + public ChangeUserEmailValidator() + { + RuleFor(x => x.Email == null ? string.Empty : x.Email.Trim()) + .Must(v => !string.IsNullOrWhiteSpace(v)).WithMessage("Email is required.") + .WithErrorCode("User.Email.Required") + .EmailAddress() + .WithErrorCode("User.Email.Format") + .MaximumLength(256) + .WithErrorCode("User.Email.MaxLength") + .OverridePropertyName(nameof(ChangeUserEmailCommand.Email)); + } +} diff --git a/src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs b/src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs index ded2d80bf..1f9fb1a0f 100644 --- a/src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs +++ b/src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs @@ -9,8 +9,10 @@ namespace Cluckwork.Infrastructure.Identity; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.ChangeTracking; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Npgsql; public sealed class IdentityProvider( UserManager userManager, @@ -678,6 +680,305 @@ await audit.WriteAsync(AuditActions.UserRoleChanged, "User", user.Id, return Result.Success(); }, ct); + public Task ChangeUserEmailAsync( + Guid accountId, Guid userId, string email, Guid actingUserId, CancellationToken ct = default) => + AmbientTransaction.RunAsync(db.Database, async (transaction, token) => + { + var lockedAccount = await accounts.GetCurrentLockedAsync(token); + if (lockedAccount is null || lockedAccount.Id != accountId) + return Result.Failure(Error.NotFound("Accounts", accountId)); + + var user = await db.Users.FirstOrDefaultAsync( + u => u.Id == userId && u.AccountId == accountId, token); + if (user is null) + return Result.Failure(Error.NotFound("Users", userId)); + + var actor = await RequireActiveOwnerAsync(accountId, actingUserId, token); + if (actor.IsFailure) + return actor; + + if (string.Equals(user.Email, email, StringComparison.Ordinal)) + return Result.Success(); + + if (userId == actingUserId + && await CountOtherActiveOwnersAsync(accountId, userId, token) == 0) + { + return Result.Failure(Error.Validation( + "Users.LastOwner", + "To change the last active Owner's email, add a second Owner first.")); + } + + var entry = db.Entry(user); + var priorModified = entry.Properties.ToDictionary( + property => property.Metadata.Name, + property => property.IsModified, + StringComparer.Ordinal); + var priorValues = new UserEmailMutationSnapshot( + user.Email, + user.NormalizedEmail, + user.UserName, + user.NormalizedUserName, + user.SecurityStamp, + user.ConcurrencyStamp, + user.CredentialEpoch, + entry.Property(candidate => candidate.Email).OriginalValue, + entry.Property(candidate => candidate.NormalizedEmail).OriginalValue, + entry.Property(candidate => candidate.UserName).OriginalValue, + entry.Property(candidate => candidate.NormalizedUserName).OriginalValue, + entry.Property(candidate => candidate.SecurityStamp).OriginalValue, + entry.Property(candidate => candidate.ConcurrencyStamp).OriginalValue, + entry.Property(candidate => candidate.CredentialEpoch).OriginalValue); + var priorAudits = db.ChangeTracker.Entries() + .Select(auditEntry => auditEntry.Entity) + .ToHashSet(ReferenceEqualityComparer.Instance); + // Both UserManager.UpdateSecurityStampAsync and the final save use + // this shared scoped context. Suspend every caller-owned dirty + // entry before either can flush it, then restore the exact pending + // state after our save. Temporary generated keys are deliberately + // never sent to PostgreSQL, so restoration never has to replace a + // real generated key with its old temporary value. + var priorDirtyEntries = db.ChangeTracker.Entries() + .Where(candidate => candidate.State is EntityState.Added or EntityState.Modified or EntityState.Deleted) + .Select(TrackedEntrySnapshot.Capture) + .ToArray(); + + void RestoreTrackerAfterFailure() + { + user.Email = priorValues.Email; + user.NormalizedEmail = priorValues.NormalizedEmail; + user.UserName = priorValues.UserName; + user.NormalizedUserName = priorValues.NormalizedUserName; + user.SecurityStamp = priorValues.SecurityStamp; + user.ConcurrencyStamp = priorValues.ConcurrencyStamp; + user.CredentialEpoch = priorValues.CredentialEpoch; + + entry.Property(candidate => candidate.Email).OriginalValue = priorValues.OriginalEmail; + entry.Property(candidate => candidate.NormalizedEmail).OriginalValue = priorValues.OriginalNormalizedEmail; + entry.Property(candidate => candidate.UserName).OriginalValue = priorValues.OriginalUserName; + entry.Property(candidate => candidate.NormalizedUserName).OriginalValue = priorValues.OriginalNormalizedUserName; + entry.Property(candidate => candidate.SecurityStamp).OriginalValue = priorValues.OriginalSecurityStamp; + entry.Property(candidate => candidate.ConcurrencyStamp).OriginalValue = priorValues.OriginalConcurrencyStamp; + entry.Property(candidate => candidate.CredentialEpoch).OriginalValue = priorValues.OriginalCredentialEpoch; + foreach (var property in entry.Properties) + property.IsModified = priorModified[property.Metadata.Name]; + + foreach (var dirtyEntry in priorDirtyEntries) + { + if (ReferenceEquals(dirtyEntry.Entry.Entity, user)) + dirtyEntry.RestoreTargetAfterFailure(); + else + dirtyEntry.RestoreExact(); + } + + foreach (var auditEntry in db.ChangeTracker.Entries() + .Where(candidate => candidate.State == EntityState.Added + && candidate.Entity.Action == AuditActions.UserEmailChanged + && candidate.Entity.EntityId == userId + && !priorAudits.Contains(candidate.Entity))) + { + auditEntry.State = EntityState.Detached; + } + } + + void RestoreCallerEntriesAfterSuccess() + { + foreach (var dirtyEntry in priorDirtyEntries) + { + if (ReferenceEquals(dirtyEntry.Entry.Entity, user)) + dirtyEntry.RestoreTargetAfterSuccessfulSave(); + else + dirtyEntry.RestoreExact(); + } + } + + try + { + // Suspension mutates tracker state, so it belongs inside the + // same restore boundary as both saves. If a later snapshot + // throws while being suppressed, the earlier ones are still + // restored exactly by the catch below. + foreach (var dirtyEntry in priorDirtyEntries) + dirtyEntry.Suspend(ReferenceEquals(dirtyEntry.Entry.Entity, user)); + + var oldEmail = user.Email; + user.Email = email; + user.NormalizedEmail = userManager.NormalizeEmail(email); + user.UserName = email; + user.NormalizedUserName = userManager.NormalizeName(email); + + var stampRotated = await userManager.UpdateSecurityStampAsync(user); + if (!stampRotated.Succeeded) + { + RestoreTrackerAfterFailure(); + if (IsConcurrencyFailure(stampRotated)) + return Result.Failure(Error.Conflict( + "Users.Conflict", "The user was modified by another request. Reload and retry.")); + if (stampRotated.Errors.Any(error => + error.Code is "DuplicateEmail" or "DuplicateUserName")) + { + return Result.Failure(DuplicateEmailConflict()); + } + + return Result.Failure(Error.Validation( + "Users.EmailChangeFailed", Describe(stampRotated))); + } + + user.CredentialEpoch++; + await RevokeAllActiveForUserAsync(user.Id, timeProvider.GetUtcNow(), token); + await audit.WriteAsync(AuditActions.UserEmailChanged, "User", user.Id, + reason: null, details: new { oldEmail, newEmail = email }, ct: token); + + try + { + await db.SaveChangesAsync(token); + } + catch (DbUpdateConcurrencyException) + { + RestoreTrackerAfterFailure(); + return Result.Failure(Error.Conflict( + "Users.Conflict", "The user was modified by another request. Reload and retry.")); + } + catch (DbUpdateException exception) when (IsUserEmailConflict(exception)) + { + RestoreTrackerAfterFailure(); + return Result.Failure(DuplicateEmailConflict()); + } + + RestoreCallerEntriesAfterSuccess(); + await transaction.CommitAsync(token); + return Result.Success(); + } + catch (DbUpdateException exception) when (IsUserEmailConflict(exception)) + { + RestoreTrackerAfterFailure(); + return Result.Failure(DuplicateEmailConflict()); + } + catch + { + RestoreTrackerAfterFailure(); + throw; + } + }, ct); + + private static Error DuplicateEmailConflict() => + Error.Conflict("Users.DuplicateEmail", "A user with this email already exists."); + + internal static bool IsUserEmailConflict(DbUpdateException exception) => + exception.InnerException is PostgresException + { + SqlState: PostgresErrorCodes.UniqueViolation, + ConstraintName: "EmailIndex" or "UserNameIndex" + }; + + private sealed record UserEmailMutationSnapshot( + // IdentityUser declares these as nullable even though this app's EF + // model requires all four login columns. Keep the compiler-truthful + // nullable types here; the strongly typed EF property access below + // avoids the former object? originals without asserting a narrower + // C# invariant than ApplicationUser actually declares. + string? Email, + string? NormalizedEmail, + string? UserName, + string? NormalizedUserName, + string? SecurityStamp, + string? ConcurrencyStamp, + int CredentialEpoch, + string? OriginalEmail, + string? OriginalNormalizedEmail, + string? OriginalUserName, + string? OriginalNormalizedUserName, + string? OriginalSecurityStamp, + string? OriginalConcurrencyStamp, + int OriginalCredentialEpoch); + + private sealed record TrackedEntrySnapshot( + EntityEntry Entry, + EntityState State, + PropertyValues CurrentValues, + PropertyValues OriginalValues, + IReadOnlyDictionary ModifiedFlags, + IReadOnlySet TemporaryProperties, + IReadOnlyDictionary TemporaryClrValues) + { + public static TrackedEntrySnapshot Capture(EntityEntry entry) => new( + entry, + entry.State, + entry.CurrentValues.Clone(), + entry.OriginalValues.Clone(), + entry.Properties.ToDictionary( + property => property.Metadata.Name, + property => property.IsModified, + StringComparer.Ordinal), + entry.Properties + .Where(property => property.IsTemporary) + .Select(property => property.Metadata.Name) + .ToHashSet(StringComparer.Ordinal), + entry.Properties + .Where(property => property.IsTemporary && property.Metadata.PropertyInfo is not null) + .ToDictionary( + property => property.Metadata.Name, + property => property.Metadata.PropertyInfo!.GetValue(entry.Entity), + StringComparer.Ordinal)); + + public void Suspend(bool isTarget) + { + foreach (var propertyName in TemporaryProperties) + Entry.Property(propertyName).IsTemporary = false; + Entry.State = EntityState.Unchanged; + if (isTarget) + return; + // The state transition reverts Modified current values in EF Core + // 10. Put the caller's values back before either save, then make + // the suppression originals match so DetectChanges stays silent. + // This happens before any store-generated key can be accepted; + // restoration after the save never writes current values. + foreach (var property in Entry.Properties + .Where(property => !TemporaryProperties.Contains(property.Metadata.Name))) + { + property.CurrentValue = CurrentValues[property.Metadata.Name]; + } + Entry.OriginalValues.SetValues(CurrentValues); + Entry.State = EntityState.Unchanged; + } + + public void RestoreExact() + { + Entry.State = State; + Entry.OriginalValues.SetValues(OriginalValues); + foreach (var property in Entry.Properties) + property.IsModified = ModifiedFlags[property.Metadata.Name]; + foreach (var propertyName in TemporaryProperties) + Entry.Property(propertyName).IsTemporary = true; + foreach (var (propertyName, clrValue) in TemporaryClrValues) + Entry.Property(propertyName).Metadata.PropertyInfo!.SetValue(Entry.Entity, clrValue); + } + + public void RestoreTargetAfterFailure() + { + Entry.CurrentValues.SetValues(CurrentValues); + RestoreExact(); + } + + public void RestoreTargetAfterSuccessfulSave() + { + if (State == EntityState.Modified) + { + foreach (var property in Entry.Properties.Where(property => ModifiedFlags[property.Metadata.Name])) + { + property.CurrentValue = CurrentValues[property.Metadata.Name]; + property.OriginalValue = OriginalValues[property.Metadata.Name]; + property.IsModified = true; + } + return; + } + + // A queried target cannot be Added. Preserve a pre-existing delete + // against the successfully updated row using the new accepted + // concurrency originals rather than the stale pre-update ones. + Entry.State = State; + } + } + private static bool IsConcurrencyFailure(IdentityResult result) => result.Errors.Any(e => e.Code == "ConcurrencyFailure"); diff --git a/src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs b/src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs index 76942bdc8..11f6a5306 100644 --- a/src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs +++ b/src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs @@ -8,6 +8,7 @@ namespace Cluckwork.Infrastructure.Identity; using Cluckwork.Domain.Common; using Cluckwork.Infrastructure.Persistence; using Microsoft.AspNetCore.Identity; +using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Options; using Microsoft.IdentityModel.Tokens; @@ -355,7 +356,12 @@ public async Task ValidateAsync( || !int.TryParse(epochClaim, NumberStyles.None, CultureInfo.InvariantCulture, out var grantEpoch)) return Result.Failure(DeniedError); // wrong-account / wrong-user / malformed claims - var user = await userManager.FindByIdAsync(userId.ToString()); + // Validation only compares persisted grant-binding fields. Keep this + // read out of the shared scoped context's identity map: a privileged + // handler may wait on an account lock after validation and must not + // identity-resolve this pre-lock row as its mutation target. + var user = await db.Users.AsNoTracking() + .SingleOrDefaultAsync(candidate => candidate.Id == userId, ct); if (user is null || user.AccountId != accountId || user.SecurityStamp != stamp) return Result.Failure(DeniedError); // revoked by a security-stamp change diff --git a/tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailRaceTests.cs b/tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailRaceTests.cs new file mode 100644 index 000000000..604d9e1e6 --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailRaceTests.cs @@ -0,0 +1,658 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Data.Common; +using System.Net; +using System.Net.Http.Json; +using Cluckwork.Api.Endpoints.Auth; +using Cluckwork.Api.IntegrationTests.Infrastructure; +using Cluckwork.Application.Common; +using Cluckwork.Application.Features.Users.ChangeUserEmail; +using Cluckwork.Domain.Accounts; +using Cluckwork.Domain.Auditing; +using Cluckwork.Domain.Common; +using Cluckwork.Infrastructure.Identity; +using Cluckwork.Infrastructure.Persistence; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.TestHost; +using Microsoft.AspNetCore.Mvc; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.ChangeTracking; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.Extensions.DependencyInjection; +using Npgsql; + +public sealed class ChangeUserEmailFinalSaveFactory : CluckworkWebApplicationFactory +{ + public ChangeUserEmailFinalSaveInterceptor Interceptor { get; } = new(); + + protected override void ConfigureWebHost(IWebHostBuilder builder) + { + base.ConfigureWebHost(builder); + builder.ConfigureTestServices(services => + services.AddDbContext((_, options) => options.AddInterceptors(Interceptor))); + } +} + +public sealed class ChangeUserEmailFinalSaveInterceptor : DbCommandInterceptor +{ + private int _armed; + + public void Arm() => Interlocked.Exchange(ref _armed, 1); + + private void MaybeFail(DbCommand command) + { + if (command.CommandText.Contains("INSERT INTO \"AuditEvents\"", StringComparison.Ordinal) + && Interlocked.CompareExchange(ref _armed, 0, 1) == 1) + throw new DbUpdateConcurrencyException("Simulated final change-email concurrency failure."); + } + + public override ValueTask> NonQueryExecutingAsync( + DbCommand command, CommandEventData eventData, InterceptionResult result, + CancellationToken cancellationToken = default) + { + MaybeFail(command); + return base.NonQueryExecutingAsync(command, eventData, result, cancellationToken); + } + + public override ValueTask> ReaderExecutingAsync( + DbCommand command, CommandEventData eventData, InterceptionResult result, + CancellationToken cancellationToken = default) + { + MaybeFail(command); + return base.ReaderExecutingAsync(command, eventData, result, cancellationToken); + } +} + +[Collection(IntegrationCollection.Name)] +public sealed class ChangeUserEmailRaceTests(CluckworkWebApplicationFactory factory) +{ + private sealed record StepUpDto(string Token, DateTimeOffset ExpiresAt); + private sealed record TrackedEntryState( + EntityState State, + IReadOnlyDictionary CurrentValues, + IReadOnlyDictionary OriginalValues, + IReadOnlyDictionary ModifiedFlags); + + private static string Unique(string label) => $"{label}-{Guid.NewGuid():N}@test.local"; + + private static TrackedEntryState Capture(EntityEntry entry) => new( + entry.State, + entry.Properties.ToDictionary(property => property.Metadata.Name, property => property.CurrentValue), + entry.Properties.ToDictionary(property => property.Metadata.Name, property => property.OriginalValue), + entry.Properties.ToDictionary(property => property.Metadata.Name, property => property.IsModified)); + + private static void AssertTrackedEntryState(TrackedEntryState expected, EntityEntry actual) + { + Assert.Equal(expected.State, actual.State); + foreach (var property in actual.Properties) + { + var name = property.Metadata.Name; + Assert.Equal(expected.CurrentValues[name], property.CurrentValue); + Assert.Equal(expected.OriginalValues[name], property.OriginalValue); + Assert.Equal(expected.ModifiedFlags[name], property.IsModified); + } + } + + private async Task<(Guid AccountId, string Owner, Guid OwnerId)> SeedFarmAsync() + { + var owner = Unique("owner"); + var accountId = await factory.SeedAccountWithUserAsync(owner); + return (accountId, owner, await UserIdAsync(accountId, owner)); + } + + private Task UserIdAsync(Guid accountId, string email) => + factory.WithTenantScopeAsync(accountId, db => db.Users + .Where(u => u.AccountId == accountId && u.Email == email) + .Select(u => u.Id) + .SingleAsync()); + + private async Task StepUpAsync(string email) + { + var client = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(email)); + var response = await client.PostAsJsonAsync( + "/api/v1/auth/step-up", new { password = TestHarness.Password }); + response.EnsureSuccessStatusCode(); + return (await response.Content.ReadFromJsonAsync())!.Token; + } + + private async Task InvokeAsync( + Guid accountId, Guid userId, string email, Guid actingUserId, string? proof = null) + { + using var scope = factory.Services.CreateScope(); + scope.ResolveTenantAndActor(accountId, actingUserId); + return await scope.ServiceProvider.GetRequiredService().HandleAsync( + new ChangeUserEmailCommand(userId, email, proof), + accountId, actingUserId, CancellationToken.None); + } + + private static Task InvokeHttpAsync( + HttpClient client, Guid userId, string email, string proof) + { + var request = new HttpRequestMessage(HttpMethod.Put, $"/api/v1/users/{userId}/email") + { + Content = JsonContent.Create(new { email }) + }; + request.Headers.Add("Idempotency-Key", Guid.NewGuid().ToString()); + request.Headers.Add(AuthEndpoints.StepUpHeaderName, proof); + return client.SendAsync(request); + } + + private async Task<(AppDbContext Db, Microsoft.EntityFrameworkCore.Storage.IDbContextTransaction Tx, int Pid)> + FenceAccountAsync(Guid accountId) + { + var tenant = new TenantContext(); + tenant.Resolve(accountId); + var db = new AppDbContext( + new DbContextOptionsBuilder().UseNpgsql(factory.ConnectionString).Options, tenant); + var tx = await db.Database.BeginTransactionAsync(); + await db.Database.ExecuteSqlInterpolatedAsync( + $"""SELECT 1 FROM "Accounts" WHERE "Id" = {accountId} FOR UPDATE"""); + return (db, tx, await db.BackendPidAsync()); + } + + [Fact] + public async Task RefreshInFlightAcrossEmailChange_LeavesNoUsableChild() + { + var (accountId, owner, ownerId) = await SeedFarmAsync(); + var target = Unique("target"); + await factory.SeedUserAsync(accountId, target, "Manager"); + var targetId = await UserIdAsync(accountId, target); + var session = await factory.LoginAsync(target); + var proof = await StepUpAsync(owner); + var (db, tx, pid) = await FenceAccountAsync(accountId); + await using var _ = db; + await using var __ = tx; + + var change = Task.Run(() => InvokeAsync(accountId, targetId, Unique("changed"), ownerId, proof)); + Assert.True(await factory.WaitUntilDoneOrBlockedAsync(change, pid)); + + var refresh = await factory.CreateClient().PostRefreshAsync( + session.RefreshToken, expectedAccount: accountId.ToString()); + refresh.EnsureSuccessStatusCode(); + var child = await TestHarness.ReadTokensAsync(refresh); + + await tx.RollbackAsync(); + Assert.True((await change).IsSuccess); + Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.CreateAuthedClient(child.AccessToken).GetAsync("/api/v1/flocks")).StatusCode); + Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.CreateClient().PostRefreshAsync( + child.RefreshToken, expectedAccount: accountId.ToString())).StatusCode); + } + + [Fact] + public async Task ConcurrentEmailChanges_AccountLockSerializes_AndFriendlyValidatorRejectsLoser() + { + var (accountId, owner, ownerId) = await SeedFarmAsync(); + var first = Unique("first"); + var second = Unique("second"); + await factory.SeedUserAsync(accountId, first, "Manager"); + await factory.SeedUserAsync(accountId, second, "Manager"); + var firstId = await UserIdAsync(accountId, first); + var secondId = await UserIdAsync(accountId, second); + var claimed = Unique("claimed"); + var firstProof = await StepUpAsync(owner); + var secondProof = await StepUpAsync(owner); + var (db, tx, pid) = await FenceAccountAsync(accountId); + await using var _ = db; + await using var __ = tx; + + // Both email-change operations take the same account lock before their + // Identity validator runs. The second request therefore cannot race + // the first to PostgreSQL's unique constraint: it resumes after the + // winner commits and the account-scoped validator reports the friendly + // duplicate result from the now-persisted row. + var firstClaim = Task.Run(() => InvokeAsync(accountId, firstId, claimed, ownerId, firstProof)); + Assert.True(await factory.WaitUntilDoneOrBlockedAsync(firstClaim, pid)); + var secondClaim = Task.Run(() => InvokeAsync(accountId, secondId, claimed, ownerId, secondProof)); + Assert.True(await factory.WaitUntilDoneOrBlockedAsync(secondClaim, pid, minBlockedCount: 2)); + + await tx.RollbackAsync(); + var results = await Task.WhenAll(firstClaim, secondClaim); + + Assert.Single(results, result => result.IsSuccess); + var loser = Assert.Single(results, result => result.IsFailure); + Assert.Equal("Users.DuplicateEmail", loser.Error.Code); + Assert.Equal(1, await factory.WithTenantScopeAsync(accountId, + d => d.Users.CountAsync(u => u.AccountId == accountId && u.NormalizedEmail == claimed.ToUpperInvariant()))); + } + + [Fact] + public async Task QueuedSelfChange_UsesPostLockEmail_AndCannotReturnFalseNoOp() + { + var (accountId, owner, ownerId) = await SeedFarmAsync(); + await factory.SeedUserAsync(accountId, Unique("co-owner"), Roles.Owner); + var firstEmail = Unique("first"); + var firstProof = await StepUpAsync(owner); + var secondProof = await StepUpAsync(owner); + var client = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(owner)); + var (db, tx, pid) = await FenceAccountAsync(accountId); + await using var _ = db; + await using var __ = tx; + + // Both HTTP requests validate and consume their distinct one-use grants + // before queueing on the account row. The second asks for the email that + // was current before the first request. Once the first commits, the + // second must observe that post-lock row and apply its requested last + // value (or report an explicit conflict), never identity-resolve its + // stale step-up instance into a false no-op 204. + var first = Task.Run(() => InvokeHttpAsync(client, ownerId, firstEmail, firstProof)); + Assert.True(await factory.WaitUntilDoneOrBlockedAsync(first, pid)); + var second = Task.Run(() => InvokeHttpAsync(client, ownerId, owner, secondProof)); + Assert.True(await factory.WaitUntilDoneOrBlockedAsync(second, pid, minBlockedCount: 2)); + + await tx.RollbackAsync(); + var firstResponse = await first; + var secondResponse = await second; + + Assert.Equal(HttpStatusCode.NoContent, firstResponse.StatusCode); + if (secondResponse.StatusCode == HttpStatusCode.NoContent) + { + Assert.Equal(owner, await factory.WithTenantScopeAsync(accountId, + scoped => scoped.Users.Where(user => user.Id == ownerId) + .Select(user => user.Email).SingleAsync())); + } + else + { + Assert.Equal(HttpStatusCode.Conflict, secondResponse.StatusCode); + Assert.Equal("Users.Conflict", + (await secondResponse.Content.ReadFromJsonAsync())!.Title); + } + } + + [Fact] + public async Task ConcurrentStampChange_DuringEmailMutation_Is409() + { + var (accountId, owner, ownerId) = await SeedFarmAsync(); + var target = Unique("target"); + await factory.SeedUserAsync(accountId, target, "Manager"); + var targetId = await UserIdAsync(accountId, target); + var proof = await StepUpAsync(owner); + var tenant = new TenantContext(); + tenant.Resolve(accountId); + await using var fenceDb = new AppDbContext( + new DbContextOptionsBuilder().UseNpgsql(factory.ConnectionString).Options, tenant); + await using var fenceTx = await fenceDb.Database.BeginTransactionAsync(); + await fenceDb.Database.ExecuteSqlInterpolatedAsync( + $"""SELECT 1 FROM "AspNetUsers" WHERE "Id" = {targetId} FOR UPDATE"""); + var fencePid = await fenceDb.BackendPidAsync(); + + var change = Task.Run(() => InvokeAsync(accountId, targetId, Unique("changed"), ownerId, proof)); + Assert.True(await factory.WaitUntilDoneOrBlockedAsync(change, fencePid)); + await fenceDb.Database.ExecuteSqlInterpolatedAsync( + $"""UPDATE "AspNetUsers" SET "ConcurrencyStamp" = {Guid.NewGuid().ToString()} WHERE "Id" = {targetId}"""); + await fenceTx.CommitAsync(); + + var result = await change; + Assert.True(result.IsFailure); + Assert.Equal("Users.Conflict", result.Error.Code); + Assert.Equal(target, await factory.WithTenantScopeAsync(accountId, + d => d.Users.Where(u => u.Id == targetId).Select(u => u.Email).SingleAsync())); + } + + [Fact] + public async Task DuplicateFailure_RestoresTrackedFieldsAndStampsBeforeLaterSave() + { + var (accountId, _, ownerId) = await SeedFarmAsync(); + var existing = Unique("existing"); + var target = Unique("target"); + var bystander = Unique("bystander"); + await factory.SeedUserAsync(accountId, existing, "Manager"); + await factory.SeedUserAsync(accountId, target, "Manager"); + await factory.SeedUserAsync(accountId, bystander, "Manager"); + var targetId = await UserIdAsync(accountId, target); + var bystanderId = await UserIdAsync(accountId, bystander); + + using var scope = factory.Services.CreateScope(); + scope.ResolveTenantAndActor(accountId, ownerId); + var db = scope.ServiceProvider.GetRequiredService(); + var tracked = await db.Users.SingleAsync(u => u.Id == targetId); + var oldStamp = tracked.SecurityStamp; + var oldConcurrency = tracked.ConcurrencyStamp; + var result = await scope.ServiceProvider.GetRequiredService() + .ChangeUserEmailAsync(accountId, targetId, existing, ownerId); + + Assert.True(result.IsFailure); + Assert.Equal("Users.DuplicateEmail", result.Error.Code); + var unrelated = await db.Users.SingleAsync(u => u.Id == bystanderId); + unrelated.DisplayName = "Saved after duplicate"; + await db.SaveChangesAsync(); + + using var fresh = factory.Services.CreateScope(); + fresh.ServiceProvider.GetRequiredService().Resolve(accountId); + var check = await fresh.ServiceProvider.GetRequiredService().Users + .AsNoTracking().SingleAsync(u => u.Id == targetId); + Assert.Equal(target, check.Email); + Assert.Equal(target, check.UserName); + Assert.Equal(oldStamp, check.SecurityStamp); + Assert.Equal(oldConcurrency, check.ConcurrencyStamp); + } + + [Fact] + public async Task SuccessfulChange_PreservesCallerOwnedDirtyEntriesUntilTheirLaterSave() + { + var (accountId, _, ownerId) = await SeedFarmAsync(); + var targetEmail = Unique("target"); + var unrelatedEmail = Unique("unrelated"); + await factory.SeedUserAsync(accountId, targetEmail, "Manager"); + await factory.SeedUserAsync(accountId, unrelatedEmail, "Manager"); + var targetId = await UserIdAsync(accountId, targetEmail); + var unrelatedId = await UserIdAsync(accountId, unrelatedEmail); + var deletedClaimType = $"pending-delete-{Guid.NewGuid():N}"; + var deletedClaimId = await factory.WithTenantScopeAsync(accountId, async db => + { + var claim = new IdentityUserClaim + { + UserId = unrelatedId, + ClaimType = deletedClaimType, + ClaimValue = "still-present-until-caller-save" + }; + db.UserClaims.Add(claim); + await db.SaveChangesAsync(); + return claim.Id; + }); + + using var scope = factory.Services.CreateScope(); + scope.ResolveTenantAndActor(accountId, ownerId); + var db = scope.ServiceProvider.GetRequiredService(); + var target = await db.Users.SingleAsync(user => user.Id == targetId); + var oldTargetDisplayName = target.DisplayName; + target.DisplayName = "Pending target display name"; + var targetEntry = db.Entry(target); + var unrelated = await db.Users.SingleAsync(user => user.Id == unrelatedId); + unrelated.DisplayName = "Pending unrelated display name"; + var unrelatedEntry = db.Entry(unrelated); + var expectedUnrelatedState = Capture(unrelatedEntry); + var addedClaimType = $"pending-add-{Guid.NewGuid():N}"; + var addedClaim = new IdentityUserClaim + { + UserId = unrelatedId, + ClaimType = addedClaimType, + ClaimValue = "inserted-by-caller-save" + }; + db.UserClaims.Add(addedClaim); + var addedClaimEntry = db.Entry(addedClaim); + var temporaryAddedClaimId = addedClaim.Id; + Assert.True(addedClaimEntry.Property(claim => claim.Id).IsTemporary); + var expectedAddedClaimState = Capture(addedClaimEntry); + var deletedClaim = await db.UserClaims.SingleAsync(claim => claim.Id == deletedClaimId); + db.UserClaims.Remove(deletedClaim); + var deletedClaimEntry = db.Entry(deletedClaim); + var expectedDeletedClaimState = Capture(deletedClaimEntry); + var changedEmail = Unique("changed"); + var oldUnrelatedDisplayName = unrelatedEntry.Property(user => user.DisplayName).OriginalValue; + + var result = await scope.ServiceProvider.GetRequiredService() + .ChangeUserEmailAsync(accountId, targetId, changedEmail, ownerId); + + Assert.True(result.IsSuccess); + Assert.Equal(EntityState.Modified, targetEntry.State); + Assert.Equal("Pending target display name", target.DisplayName); + Assert.Equal(oldTargetDisplayName, + targetEntry.Property(user => user.DisplayName).OriginalValue); + Assert.True(targetEntry.Property(user => user.DisplayName).IsModified); + Assert.Equal(changedEmail, target.Email); + Assert.Equal(changedEmail, targetEntry.Property(user => user.Email).OriginalValue); + Assert.False(targetEntry.Property(user => user.Email).IsModified); + AssertTrackedEntryState(expectedUnrelatedState, unrelatedEntry); + AssertTrackedEntryState(expectedAddedClaimState, addedClaimEntry); + Assert.True(addedClaimEntry.Property(claim => claim.Id).IsTemporary); + Assert.Equal(temporaryAddedClaimId, addedClaim.Id); + AssertTrackedEntryState(expectedDeletedClaimState, deletedClaimEntry); + + using (var beforeCallerSave = factory.Services.CreateScope()) + { + beforeCallerSave.ServiceProvider.GetRequiredService().Resolve(accountId); + var freshDb = beforeCallerSave.ServiceProvider.GetRequiredService(); + var persistedTarget = await freshDb.Users.AsNoTracking().SingleAsync(user => user.Id == targetId); + Assert.Equal(changedEmail, persistedTarget.Email); + Assert.Equal(oldTargetDisplayName, persistedTarget.DisplayName); + Assert.Equal(oldUnrelatedDisplayName, + await freshDb.Users.Where(user => user.Id == unrelatedId) + .Select(user => user.DisplayName).SingleAsync()); + Assert.False(await freshDb.UserClaims.AnyAsync(claim => claim.ClaimType == addedClaimType)); + Assert.True(await freshDb.UserClaims.AnyAsync(claim => claim.Id == deletedClaimId)); + } + + await db.SaveChangesAsync(); + + using var afterCallerSave = factory.Services.CreateScope(); + afterCallerSave.ServiceProvider.GetRequiredService().Resolve(accountId); + var afterDb = afterCallerSave.ServiceProvider.GetRequiredService(); + Assert.Equal("Pending target display name", + await afterDb.Users.Where(user => user.Id == targetId).Select(user => user.DisplayName).SingleAsync()); + Assert.Equal("Pending unrelated display name", + await afterDb.Users.Where(user => user.Id == unrelatedId).Select(user => user.DisplayName).SingleAsync()); + Assert.True(await afterDb.UserClaims.AnyAsync(claim => claim.ClaimType == addedClaimType)); + Assert.False(await afterDb.UserClaims.AnyAsync(claim => claim.Id == deletedClaimId)); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task FinalConcurrencyFailure_RemovesEpochAndPendingAuditFromTracker(bool targetWasModified) + { + await using var injected = new ChangeUserEmailFinalSaveFactory(); + await injected.InitializeAsync(); + var owner = Unique("owner"); + var accountId = await injected.SeedAccountWithUserAsync(owner); + var ownerId = await injected.WithTenantScopeAsync(accountId, + db => db.Users.Where(u => u.Email == owner).Select(u => u.Id).SingleAsync()); + var target = Unique("target"); + await injected.SeedUserAsync(accountId, target, "Manager"); + var targetId = await injected.WithTenantScopeAsync(accountId, + db => db.Users.Where(u => u.Email == target).Select(u => u.Id).SingleAsync()); + var preexistingEmail = Unique("preexisting"); + await injected.SeedUserAsync(accountId, preexistingEmail, "Manager"); + var preexistingId = await injected.WithTenantScopeAsync(accountId, + db => db.Users.Where(u => u.Email == preexistingEmail).Select(u => u.Id).SingleAsync()); + var laterEmail = Unique("later"); + await injected.SeedUserAsync(accountId, laterEmail, "Manager"); + var laterId = await injected.WithTenantScopeAsync(accountId, + db => db.Users.Where(u => u.Email == laterEmail).Select(u => u.Id).SingleAsync()); + var deletedClaimType = $"pending-delete-{Guid.NewGuid():N}"; + var deletedClaimId = await injected.WithTenantScopeAsync(accountId, async db => + { + var claim = new IdentityUserClaim + { + UserId = preexistingId, + ClaimType = deletedClaimType, + ClaimValue = "still-present-until-caller-save" + }; + db.UserClaims.Add(claim); + await db.SaveChangesAsync(); + return claim.Id; + }); + + using var scope = injected.Services.CreateScope(); + scope.ResolveTenantAndActor(accountId, ownerId); + var db = scope.ServiceProvider.GetRequiredService(); + var tracked = await db.Users.SingleAsync(u => u.Id == targetId); + var pendingDisplayName = targetWasModified ? "Pending before email change" : tracked.DisplayName; + if (targetWasModified) + tracked.DisplayName = pendingDisplayName; + var targetEntry = db.Entry(tracked); + var expectedTargetState = Capture(targetEntry); + var oldEmail = tracked.Email; + var oldUserName = tracked.UserName; + var oldSecurityStamp = tracked.SecurityStamp; + var oldConcurrencyStamp = tracked.ConcurrencyStamp; + var oldEpoch = tracked.CredentialEpoch; + var preexisting = await db.Users.SingleAsync(u => u.Id == preexistingId); + preexisting.DisplayName = "Pending before email change"; + var preexistingEntry = db.Entry(preexisting); + var expectedPreexistingState = Capture(preexistingEntry); + var addedClaimType = $"pending-add-{Guid.NewGuid():N}"; + var addedClaim = new IdentityUserClaim + { + UserId = preexistingId, + ClaimType = addedClaimType, + ClaimValue = "inserted-by-caller-save" + }; + db.UserClaims.Add(addedClaim); + var addedClaimEntry = db.Entry(addedClaim); + var temporaryAddedClaimId = addedClaim.Id; + Assert.True(addedClaimEntry.Property(claim => claim.Id).IsTemporary); + var expectedAddedClaimState = Capture(addedClaimEntry); + var deletedClaim = await db.UserClaims.SingleAsync(claim => claim.Id == deletedClaimId); + db.UserClaims.Remove(deletedClaim); + var deletedClaimEntry = db.Entry(deletedClaim); + var expectedDeletedClaimState = Capture(deletedClaimEntry); + injected.Interceptor.Arm(); + + var result = await scope.ServiceProvider.GetRequiredService() + .ChangeUserEmailAsync(accountId, targetId, Unique("changed"), ownerId); + + Assert.True(result.IsFailure); + Assert.Equal("Users.Conflict", result.Error.Code); + Assert.Equal(oldEpoch, tracked.CredentialEpoch); + AssertTrackedEntryState(expectedTargetState, targetEntry); + Assert.DoesNotContain(db.ChangeTracker.Entries(), + entry => entry.State == EntityState.Added && entry.Entity.Action == "User.EmailChanged"); + AssertTrackedEntryState(expectedPreexistingState, preexistingEntry); + AssertTrackedEntryState(expectedAddedClaimState, addedClaimEntry); + Assert.True(addedClaimEntry.Property(claim => claim.Id).IsTemporary); + Assert.Equal(temporaryAddedClaimId, addedClaim.Id); + AssertTrackedEntryState(expectedDeletedClaimState, deletedClaimEntry); + + // The failed operation must leave this SAME context usable. A save on + // a second unrelated tracked row is the adversarial check: the caller's + // pre-existing edit must persist with it, while stale target values or + // a leaked Added audit must not be flushed. + var later = await db.Users.SingleAsync(u => u.Id == laterId); + later.DisplayName = "Saved after final concurrency failure"; + await db.SaveChangesAsync(); + + using var fresh = injected.Services.CreateScope(); + fresh.ServiceProvider.GetRequiredService().Resolve(accountId); + var freshDb = fresh.ServiceProvider.GetRequiredService(); + var persisted = await freshDb.Users.AsNoTracking().SingleAsync(u => u.Id == targetId); + Assert.Equal(oldEmail, persisted.Email); + Assert.Equal(oldUserName, persisted.UserName); + Assert.Equal(oldSecurityStamp, persisted.SecurityStamp); + Assert.Equal(oldConcurrencyStamp, persisted.ConcurrencyStamp); + Assert.Equal(oldEpoch, persisted.CredentialEpoch); + Assert.Equal(pendingDisplayName, persisted.DisplayName); + Assert.Equal("Pending before email change", + await freshDb.Users.Where(u => u.Id == preexistingId).Select(u => u.DisplayName).SingleAsync()); + Assert.Equal("Saved after final concurrency failure", + await freshDb.Users.Where(u => u.Id == laterId).Select(u => u.DisplayName).SingleAsync()); + Assert.True(await freshDb.UserClaims.AnyAsync(claim => claim.ClaimType == addedClaimType)); + Assert.False(await freshDb.UserClaims.AnyAsync(claim => claim.Id == deletedClaimId)); + Assert.DoesNotContain(await freshDb.AuditEvents.AsNoTracking() + .Where(audit => audit.EntityId == targetId).ToListAsync(), + audit => audit.Action == "User.EmailChanged"); + } + + [Fact] + public async Task DisabledCoOwner_DoesNotSatisfySoleOwnerSelfChangeGuard() + { + var (accountId, owner, ownerId) = await SeedFarmAsync(); + var disabled = Unique("disabled-owner"); + await factory.SeedUserAsync(accountId, disabled, Roles.Owner); + var disabledId = await UserIdAsync(accountId, disabled); + await factory.WithTenantScopeAsync(accountId, async db => + { + (await db.Users.SingleAsync(u => u.Id == disabledId)).DisabledAt = DateTimeOffset.UtcNow; + await db.SaveChangesAsync(); + }); + + var result = await InvokeAsync(accountId, ownerId, Unique("changed"), ownerId, await StepUpAsync(owner)); + + Assert.True(result.IsFailure); + Assert.Equal("Users.LastOwner", result.Error.Code); + } + + [Fact] + public async Task ActorDemotedWhileQueued_Is403_AndTargetIsUnchanged() + { + var (accountId, actor, actorId) = await SeedFarmAsync(); + var coOwner = Unique("co-owner"); + await factory.SeedUserAsync(accountId, coOwner, Roles.Owner); + var target = Unique("target"); + await factory.SeedUserAsync(accountId, target, "Manager"); + var targetId = await UserIdAsync(accountId, target); + var proof = await StepUpAsync(actor); + var (db, tx, pid) = await FenceAccountAsync(accountId); + await using var _ = db; + await using var __ = tx; + + var change = Task.Run(() => InvokeAsync(accountId, targetId, Unique("changed"), actorId, proof)); + Assert.True(await factory.WaitUntilDoneOrBlockedAsync(change, pid)); + await db.Database.ExecuteSqlInterpolatedAsync($""" + DELETE FROM "AspNetUserRoles" + WHERE "UserId" = {actorId} + AND "RoleId" IN (SELECT "Id" FROM "AspNetRoles" WHERE "Name" = {Roles.Owner}) + """); + await tx.CommitAsync(); + + var result = await change; + Assert.True(result.IsFailure); + Assert.Equal("Auth.Forbidden", result.Error.Code); + Assert.Equal(target, await factory.WithTenantScopeAsync(accountId, + d => d.Users.Where(u => u.Id == targetId).Select(u => u.Email).SingleAsync())); + } + + [Fact] + public async Task ActorDemotedWhileHttpRequestIsQueued_Is403AuthForbidden_AndTargetIsUnchanged() + { + var (accountId, actor, actorId) = await SeedFarmAsync(); + await factory.SeedUserAsync(accountId, Unique("co-owner"), Roles.Owner); + var target = Unique("target"); + await factory.SeedUserAsync(accountId, target, "Manager"); + var targetId = await UserIdAsync(accountId, target); + var client = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(actor)); + var proof = await StepUpAsync(actor); + var changed = Unique("changed"); + var (db, tx, pid) = await FenceAccountAsync(accountId); + await using var _ = db; + await using var __ = tx; + + // The HTTP request validates and consumes its one-use proof before it + // parks on the account fence. Demote the actor while it is queued so + // only the transaction-local authorization re-read can reject it. + var change = Task.Run(() => InvokeHttpAsync(client, targetId, changed, proof)); + Assert.True(await factory.WaitUntilDoneOrBlockedAsync(change, pid)); + await db.Database.ExecuteSqlInterpolatedAsync($""" + DELETE FROM "AspNetUserRoles" + WHERE "UserId" = {actorId} + AND "RoleId" IN (SELECT "Id" FROM "AspNetRoles" WHERE "Name" = {Roles.Owner}) + """); + await tx.CommitAsync(); + + var response = await change; + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + Assert.Equal("Auth.Forbidden", + (await response.Content.ReadFromJsonAsync())!.Title); + Assert.Equal(target, await factory.WithTenantScopeAsync(accountId, + scoped => scoped.Users.Where(u => u.Id == targetId).Select(u => u.Email).SingleAsync())); + } + + [Fact] + public void IsUserEmailConflict_AcceptsOnlyTheTwoNamedUniqueConstraints() + { + // The email-change/account-lock race above intentionally exercises the + // friendly validator layer. A create-vs-change race could reach this + // database layer, but there is no deterministic fence that makes the + // 23505 occur specifically inside UpdateSecurityStampAsync without + // depending on PostgreSQL scheduling. This direct discriminator test + // therefore pins the exact SQLSTATE/constraint mapping instead of a + // false-green race whose asserted layer is accidental. + static DbUpdateException Failure(string sqlState, string constraint) => new( + "failed", + new PostgresException( + "duplicate", "ERROR", "ERROR", sqlState, + "detail", "hint", 0, 0, "query", "where", "public", "AspNetUsers", + "NormalizedEmail", "text", constraint, "file", "1", "routine")); + + Assert.True(IdentityProvider.IsUserEmailConflict( + Failure(PostgresErrorCodes.UniqueViolation, "EmailIndex"))); + Assert.True(IdentityProvider.IsUserEmailConflict( + Failure(PostgresErrorCodes.UniqueViolation, "UserNameIndex"))); + Assert.False(IdentityProvider.IsUserEmailConflict( + Failure(PostgresErrorCodes.UniqueViolation, "RoleNameIndex"))); + Assert.False(IdentityProvider.IsUserEmailConflict( + Failure(PostgresErrorCodes.ForeignKeyViolation, "EmailIndex"))); + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailTests.cs b/tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailTests.cs new file mode 100644 index 000000000..d45d07494 --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/ChangeUserEmailTests.cs @@ -0,0 +1,446 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Net; +using System.Net.Http.Json; +using Cluckwork.Api.Endpoints.Auth; +using Cluckwork.Api.IntegrationTests.Infrastructure; +using Cluckwork.Application.Common; +using Cluckwork.Application.Features.Users.ChangeUserEmail; +using Cluckwork.Infrastructure.Identity; +using Cluckwork.Infrastructure.Persistence; +using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.Mvc; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; + +[Collection(IntegrationCollection.Name)] +public sealed class ChangeUserEmailTests(CluckworkWebApplicationFactory factory) +{ + private sealed record UserRow(Guid Id, string Email, string? DisplayName, string Role, DateTimeOffset? DisabledAt); + private sealed record StepUpDto(string Token, DateTimeOffset ExpiresAt); + private sealed record IdentitySnapshot( + string? Email, + string? NormalizedEmail, + string? UserName, + string? NormalizedUserName, + string? SecurityStamp, + string? ConcurrencyStamp, + int CredentialEpoch, + bool EmailConfirmed, + DateTimeOffset? DisabledAt, + int AuditCount, + IReadOnlyList RefreshRevocations); + + private static string Unique(string label) => $"{label}-{Guid.NewGuid():N}@test.local"; + + private async Task<(HttpClient Owner, Guid AccountId, string Email, Guid Id, string FarmCode)> OwnerAsync() + { + var email = Unique("owner"); + var accountId = await factory.SeedAccountWithUserAsync(email); + var owner = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(email)); + var id = await UserIdAsync(accountId, email); + var farmCode = await factory.WithTenantScopeAsync(accountId, + db => db.Accounts.Where(a => a.Id == accountId).Select(a => a.Slug).SingleAsync()); + return (owner, accountId, email, id, farmCode); + } + + private async Task SeedUserAsync(Guid accountId, string email, string? role = "Manager") + { + await factory.SeedUserAsync(accountId, email, role); + return await UserIdAsync(accountId, email); + } + + private Task UserIdAsync(Guid accountId, string email) => + factory.WithTenantScopeAsync(accountId, db => db.Users + .Where(u => u.AccountId == accountId && u.Email == email) + .Select(u => u.Id) + .SingleAsync()); + + private static async Task StepUpAsync(HttpClient client) + { + var response = await client.PostAsJsonAsync( + "/api/v1/auth/step-up", new { password = TestHarness.Password }); + response.EnsureSuccessStatusCode(); + return (await response.Content.ReadFromJsonAsync())!.Token; + } + + private static Task ChangeEmailAsync( + HttpClient client, Guid userId, string email, string? stepUpToken = null, string? key = null) + { + var request = new HttpRequestMessage(HttpMethod.Put, $"/api/v1/users/{userId}/email") + { + Content = JsonContent.Create(new { email }) + }; + request.Headers.Add("Idempotency-Key", key ?? Guid.NewGuid().ToString()); + if (stepUpToken is not null) + request.Headers.Add(AuthEndpoints.StepUpHeaderName, stepUpToken); + return client.SendAsync(request); + } + + private Task SnapshotAsync(Guid accountId, Guid userId) => + factory.WithTenantScopeAsync(accountId, async db => + { + var row = await db.Users.SingleAsync(u => u.Id == userId && u.AccountId == accountId); + return new IdentitySnapshot( + row.Email, + row.NormalizedEmail, + row.UserName, + row.NormalizedUserName, + row.SecurityStamp, + row.ConcurrencyStamp, + row.CredentialEpoch, + row.EmailConfirmed, + row.DisabledAt, + await db.AuditEvents.CountAsync(e => + e.EntityId == userId && e.Action == "User.EmailChanged"), + await db.RefreshTokens.Where(t => t.UserId == userId) + .OrderBy(t => t.Id) + .Select(t => t.RevokedAt) + .ToListAsync()); + }); + + private static void AssertSnapshotEqual(IdentitySnapshot expected, IdentitySnapshot actual) + { + Assert.Equal(expected.Email, actual.Email); + Assert.Equal(expected.NormalizedEmail, actual.NormalizedEmail); + Assert.Equal(expected.UserName, actual.UserName); + Assert.Equal(expected.NormalizedUserName, actual.NormalizedUserName); + Assert.Equal(expected.SecurityStamp, actual.SecurityStamp); + Assert.Equal(expected.ConcurrencyStamp, actual.ConcurrencyStamp); + Assert.Equal(expected.CredentialEpoch, actual.CredentialEpoch); + Assert.Equal(expected.EmailConfirmed, actual.EmailConfirmed); + Assert.Equal(expected.DisabledAt, actual.DisabledAt); + Assert.Equal(expected.AuditCount, actual.AuditCount); + Assert.Equal(expected.RefreshRevocations, actual.RefreshRevocations); + } + + private Task TryLoginAsync(string farmCode, string email) => + factory.CreateClient().PostAsJsonAsync( + "/api/v1/auth/login", new { farmCode, email, password = TestHarness.Password }); + + [Fact] + public async Task Validator_AppliesFormatToTheEffectiveTrimmedEmail() + { + var validator = new ChangeUserEmailValidator(); + + var valid = await validator.ValidateAsync( + new ChangeUserEmailCommand(Guid.NewGuid(), " valid@farm.test ", null)); + var invalid = await validator.ValidateAsync( + new ChangeUserEmailCommand(Guid.NewGuid(), " @ ", null)); + + Assert.True(valid.IsValid); + Assert.Contains(invalid.Errors, failure => failure.ErrorCode == "User.Email.Format"); + } + + [Fact] + public async Task Validator_AppliesMaximumLengthToTheEffectiveTrimmedEmail() + { + var validator = new ChangeUserEmailValidator(); + var atLimit = $"{new string('a', 245)}@test.local"; + var overLimit = $"{new string('a', 246)}@test.local"; + + var valid = await validator.ValidateAsync( + new ChangeUserEmailCommand(Guid.NewGuid(), $" {atLimit} ", null)); + var invalid = await validator.ValidateAsync( + new ChangeUserEmailCommand(Guid.NewGuid(), $" {overLimit} ", null)); + + Assert.Equal(256, atLimit.Length); + Assert.Equal(257, overLimit.Length); + Assert.True(valid.IsValid); + Assert.Contains(invalid.Errors, failure => failure.ErrorCode == "User.Email.MaxLength"); + } + + [Fact] + public async Task Change_WritesAllFourColumnsThroughTheConfiguredNormalizer() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var oldEmail = Unique("target"); + var targetId = await SeedUserAsync(accountId, oldEmail); + var proof = await StepUpAsync(owner); + + var response = await ChangeEmailAsync(owner, targetId, " Case.Change@Farm.Test ", proof); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + using var scope = factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService().Resolve(accountId); + var db = scope.ServiceProvider.GetRequiredService(); + var userManager = scope.ServiceProvider.GetRequiredService>(); + var row = await db.Users.SingleAsync(u => u.Id == targetId && u.AccountId == accountId); + const string expectedEmail = "Case.Change@Farm.Test"; + Assert.Equal(expectedEmail, row.Email); + Assert.Equal(userManager.NormalizeEmail(expectedEmail), row.NormalizedEmail); + Assert.Equal(expectedEmail, row.UserName); + Assert.Equal(userManager.NormalizeName(expectedEmail), row.NormalizedUserName); + } + + [Fact] + public async Task CrossAccountDuplicate_SucceedsInBothFarms() + { + var duplicate = Unique("shared"); + var (_, accountA, _, _, _) = await OwnerAsync(); + await SeedUserAsync(accountA, duplicate); + var (ownerB, accountB, _, _, _) = await OwnerAsync(); + var targetId = await SeedUserAsync(accountB, Unique("before")); + + var response = await ChangeEmailAsync(ownerB, targetId, duplicate, await StepUpAsync(ownerB)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal(1, await factory.WithTenantScopeAsync(accountA, + db => db.Users.CountAsync(u => u.AccountId == accountA && u.Email == duplicate))); + Assert.Equal(1, await factory.WithTenantScopeAsync(accountB, + db => db.Users.CountAsync(u => u.AccountId == accountB && u.Email == duplicate))); + } + + [Fact] + public async Task SameAccountDuplicate_Is409UsersDuplicateEmail() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var existing = Unique("existing"); + await SeedUserAsync(accountId, existing); + var targetId = await SeedUserAsync(accountId, Unique("target")); + + var response = await ChangeEmailAsync(owner, targetId, existing, await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.Conflict, response.StatusCode); + Assert.Equal("Users.DuplicateEmail", (await response.Content.ReadFromJsonAsync())!.Title); + } + + [Fact] + public async Task Change_BumpsEpochOnce_AndKillsTheLiveAccessAndRefreshTokens() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var targetEmail = Unique("target"); + var targetId = await SeedUserAsync(accountId, targetEmail); + var session = await factory.LoginAsync(targetEmail); + var targetClient = factory.CreateAuthedClient(session.AccessToken); + var before = await SnapshotAsync(accountId, targetId); + + var response = await ChangeEmailAsync(owner, targetId, Unique("changed"), await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + var after = await SnapshotAsync(accountId, targetId); + Assert.Equal(before.CredentialEpoch + 1, after.CredentialEpoch); + Assert.Equal(HttpStatusCode.Unauthorized, (await targetClient.GetAsync("/api/v1/flocks")).StatusCode); + Assert.Equal(HttpStatusCode.Unauthorized, + (await factory.CreateClient().PostRefreshAsync( + session.RefreshToken, expectedAccount: accountId.ToString())).StatusCode); + Assert.NotEmpty(after.RefreshRevocations); + Assert.All(after.RefreshRevocations, value => Assert.NotNull(value)); + } + + [Fact] + public async Task NewEmailLogsIn_OldEmailFails_AndOldEmailCanBeReusedInTheFarm() + { + var (owner, accountId, _, _, farmCode) = await OwnerAsync(); + var oldEmail = Unique("old"); + var newEmail = Unique("new"); + var targetId = await SeedUserAsync(accountId, oldEmail); + + var response = await ChangeEmailAsync(owner, targetId, newEmail, await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal(HttpStatusCode.OK, (await TryLoginAsync(farmCode, newEmail)).StatusCode); + Assert.Equal(HttpStatusCode.Unauthorized, (await TryLoginAsync(farmCode, oldEmail)).StatusCode); + await factory.SeedUserAsync(accountId, oldEmail, role: null); + Assert.Equal(1, await factory.WithTenantScopeAsync(accountId, + db => db.Users.CountAsync(u => u.AccountId == accountId && u.Email == oldEmail))); + } + + [Fact] + public async Task ExactTrimmedNoOp_LeavesEpochStampsTokensAndAuditUnchanged() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var targetEmail = Unique("target"); + var targetId = await SeedUserAsync(accountId, targetEmail); + await factory.LoginAsync(targetEmail); + var before = await SnapshotAsync(accountId, targetId); + + var response = await ChangeEmailAsync(owner, targetId, $" {targetEmail} ", await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + AssertSnapshotEqual(before, await SnapshotAsync(accountId, targetId)); + } + + [Fact] + public async Task CaseOnlyCorrection_IsARealChange() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var original = $"Case-{Guid.NewGuid():N}@test.local"; + var targetId = await SeedUserAsync(accountId, original); + var before = await SnapshotAsync(accountId, targetId); + var corrected = original.ToLowerInvariant(); + + var response = await ChangeEmailAsync(owner, targetId, corrected, await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + var after = await SnapshotAsync(accountId, targetId); + Assert.Equal(corrected, after.Email); + Assert.Equal(before.CredentialEpoch + 1, after.CredentialEpoch); + Assert.NotEqual(before.SecurityStamp, after.SecurityStamp); + } + + [Fact] + public async Task SoleOwnerSelfChange_Is422_AndNamesAddingASecondOwner() + { + var (owner, accountId, oldEmail, ownerId, _) = await OwnerAsync(); + var before = await SnapshotAsync(accountId, ownerId); + + var response = await ChangeEmailAsync(owner, ownerId, Unique("new-owner"), await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + Assert.Contains("add a second Owner first", await response.Content.ReadAsStringAsync(), StringComparison.OrdinalIgnoreCase); + AssertSnapshotEqual(before, await SnapshotAsync(accountId, ownerId)); + Assert.Equal(oldEmail, (await SnapshotAsync(accountId, ownerId)).Email); + } + + [Fact] + public async Task SelfChange_WithAnotherActiveOwner_Succeeds() + { + var (owner, accountId, _, ownerId, _) = await OwnerAsync(); + await SeedUserAsync(accountId, Unique("co-owner"), Cluckwork.Domain.Accounts.Roles.Owner); + var newEmail = Unique("new-owner"); + + var response = await ChangeEmailAsync(owner, ownerId, newEmail, await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal(newEmail, (await SnapshotAsync(accountId, ownerId)).Email); + } + + [Fact] + public async Task ForeignUserId_Is404_AndForeignRowIsUnchanged() + { + var (foreignOwner, foreignAccount, _, _, _) = await OwnerAsync(); + foreignOwner.Dispose(); + var foreignEmail = Unique("foreign"); + var foreignId = await SeedUserAsync(foreignAccount, foreignEmail); + var foreignBefore = await SnapshotAsync(foreignAccount, foreignId); + var (owner, _, _, _, _) = await OwnerAsync(); + + var response = await ChangeEmailAsync(owner, foreignId, Unique("attack"), await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + AssertSnapshotEqual(foreignBefore, await SnapshotAsync(foreignAccount, foreignId)); + } + + [Fact] + public async Task DisabledTarget_CanBeCorrected_AndStaysDisabled() + { + var (owner, accountId, _, ownerId, _) = await OwnerAsync(); + var targetId = await SeedUserAsync(accountId, Unique("disabled")); + await factory.WithTenantScopeAsync(accountId, async db => + { + var target = await db.Users.SingleAsync(u => u.Id == targetId && u.AccountId == accountId); + target.DisabledAt = DateTimeOffset.UtcNow; + target.DisabledBy = ownerId; + await db.SaveChangesAsync(); + }); + var disabledAt = (await SnapshotAsync(accountId, targetId)).DisabledAt; + var newEmail = Unique("corrected"); + + var response = await ChangeEmailAsync(owner, targetId, newEmail, await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + var after = await SnapshotAsync(accountId, targetId); + Assert.Equal(newEmail, after.Email); + Assert.Equal(disabledAt, after.DisabledAt); + } + + [Fact] + public async Task Change_PreservesEmailConfirmed() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var targetId = await SeedUserAsync(accountId, Unique("confirmed")); + await factory.WithTenantScopeAsync(accountId, async db => + { + var target = await db.Users.SingleAsync(u => u.Id == targetId && u.AccountId == accountId); + target.EmailConfirmed = true; + await db.SaveChangesAsync(); + }); + + var response = await ChangeEmailAsync(owner, targetId, Unique("changed"), await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.True((await SnapshotAsync(accountId, targetId)).EmailConfirmed); + } + + [Fact] + public async Task Change_AuditsOldAndNewEmail_OnlyOnARealChange() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var oldEmail = Unique("old"); + var newEmail = Unique("new"); + var targetId = await SeedUserAsync(accountId, oldEmail); + + var changed = await ChangeEmailAsync(owner, targetId, newEmail, await StepUpAsync(owner)); + var noOp = await ChangeEmailAsync(owner, targetId, $" {newEmail} ", await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, changed.StatusCode); + Assert.Equal(HttpStatusCode.NoContent, noOp.StatusCode); + var events = await factory.WithTenantScopeAsync(accountId, db => db.AuditEvents + .Where(e => e.EntityId == targetId && e.Action == "User.EmailChanged") + .ToListAsync()); + var audit = Assert.Single(events); + Assert.Contains(oldEmail, audit.DetailsJson); + Assert.Contains(newEmail, audit.DetailsJson); + } + + [Fact] + public async Task Change_InvalidatesTheTargetsOutstandingStepUpGrant() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var targetEmail = Unique("target"); + var targetId = await SeedUserAsync(accountId, targetEmail); + var targetClient = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(targetEmail)); + var targetGrant = await StepUpAsync(targetClient); + + var response = await ChangeEmailAsync(owner, targetId, Unique("changed"), await StepUpAsync(owner)); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + using var scope = factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService().Resolve(accountId); + var stepUp = scope.ServiceProvider.GetRequiredService(); + Assert.True((await stepUp.ValidateAsync( + accountId, targetId, targetGrant, CancellationToken.None)).IsFailure); + } + + [Fact] + public async Task MissingStepUp_Is403_AndLeavesIdentityColumnsUnchanged() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var targetId = await SeedUserAsync(accountId, Unique("target")); + var before = await SnapshotAsync(accountId, targetId); + + var response = await ChangeEmailAsync(owner, targetId, Unique("changed")); + + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + AssertSnapshotEqual(before, await SnapshotAsync(accountId, targetId)); + } + + [Fact] + public async Task InvalidEmail_Is400WithEmailFieldCode() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var targetId = await SeedUserAsync(accountId, Unique("target")); + + var response = await ChangeEmailAsync(owner, targetId, "not-an-email"); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(); + Assert.Contains("email", body, StringComparison.OrdinalIgnoreCase); + Assert.Contains("User.Email.Format", body, StringComparison.Ordinal); + } + + [Fact] + public async Task OversizedBody_Is413() + { + var (owner, accountId, _, _, _) = await OwnerAsync(); + var targetId = await SeedUserAsync(accountId, Unique("target")); + var request = new HttpRequestMessage(HttpMethod.Put, $"/api/v1/users/{targetId}/email") + { + Content = JsonContent.Create(new { email = new string('a', 4096) }) + }; + request.Headers.Add("Idempotency-Key", Guid.NewGuid().ToString()); + + Assert.Equal(HttpStatusCode.RequestEntityTooLarge, (await owner.SendAsync(request)).StatusCode); + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/MustChangePasswordGateTests.cs b/tests/Cluckwork.Api.IntegrationTests/MustChangePasswordGateTests.cs index 0b2aa4261..3fe390523 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MustChangePasswordGateTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MustChangePasswordGateTests.cs @@ -239,6 +239,10 @@ public Task ChangeUserRoleAsync( Guid accountId, Guid userId, string? role, Guid actingUserId, CancellationToken ct = default) => inner.ChangeUserRoleAsync(accountId, userId, role, actingUserId, ct); + public Task ChangeUserEmailAsync( + Guid accountId, Guid userId, string email, Guid actingUserId, CancellationToken ct = default) => + inner.ChangeUserEmailAsync(accountId, userId, email, actingUserId, ct); + public Task DisableUserAsync( Guid accountId, Guid userId, Guid actingUserId, string? reason, CancellationToken ct = default) => inner.DisableUserAsync(accountId, userId, actingUserId, reason, ct); diff --git a/tests/Cluckwork.Api.IntegrationTests/StepUpAuthTests.cs b/tests/Cluckwork.Api.IntegrationTests/StepUpAuthTests.cs index 8b5ecadd8..41039b768 100644 --- a/tests/Cluckwork.Api.IntegrationTests/StepUpAuthTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/StepUpAuthTests.cs @@ -511,6 +511,10 @@ public Task ChangeUserRoleAsync( Guid accountId, Guid userId, string? role, Guid actingUserId, CancellationToken ct = default) => inner.ChangeUserRoleAsync(accountId, userId, role, actingUserId, ct); + public Task ChangeUserEmailAsync( + Guid accountId, Guid userId, string email, Guid actingUserId, CancellationToken ct = default) => + inner.ChangeUserEmailAsync(accountId, userId, email, actingUserId, ct); + public Task DisableUserAsync( Guid accountId, Guid userId, Guid actingUserId, string? reason, CancellationToken ct = default) => inner.DisableUserAsync(accountId, userId, actingUserId, reason, ct); @@ -899,6 +903,28 @@ public async Task ValidateAsync_MakesTheAdmissionDecisionInOneAtomicRegistryCall spy.Calls); } + [Fact] + public async Task ValidateAsync_ReadOnlySecurityStampLookup_DoesNotTrackTheUser() + { + var email = $"admin-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var userId = await factory.WithTenantScopeAsync(accountId, + db => db.Users.Where(user => user.Email == email).Select(user => user.Id).SingleAsync()); + using var scope = factory.Services.CreateScope(); + scope.ResolveTenantAndActor(accountId, userId); + var db = scope.ServiceProvider.GetRequiredService(); + var stepUp = scope.ServiceProvider.GetRequiredService(); + var issued = await stepUp.IssueAsync(accountId, userId, TestHarness.Password); + Assert.True(issued.IsSuccess); + db.ChangeTracker.Clear(); + + var validated = await stepUp.ValidateAsync(accountId, userId, issued.Value.Token); + + Assert.True(validated.IsSuccess); + Assert.DoesNotContain(db.ChangeTracker.Entries(), + entry => entry.Entity.Id == userId); + } + // A SEPARATE defect, found while writing the test above and fixed in the // same change because it was masking that test's replay assertion. // diff --git a/tests/Cluckwork.Application.Tests/TenantBypass/Data/filter-free-set-sites.tsv b/tests/Cluckwork.Application.Tests/TenantBypass/Data/filter-free-set-sites.tsv index e5d0e23e7..5ba314802 100644 --- a/tests/Cluckwork.Application.Tests/TenantBypass/Data/filter-free-set-sites.tsv +++ b/tests/Cluckwork.Application.Tests/TenantBypass/Data/filter-free-set-sites.tsv @@ -46,17 +46,18 @@ src/Cluckwork.Api/Middleware/IdempotencyMiddleware.cs:443 db.IdempotencyRecords by-key: unique index (AccountId, EndpointHash, IdempotencyKeyHash) is enforced by the DB; the method carries accountId and the record is written with it. The claim read is by (endpointHash, keyHash) within the caller's resolved account. src/Cluckwork.Infrastructure/Identity/AccountScopedUserValidator.cs:59 db.Users by-id: re-reads the current user by primary key (candidate.Id == user.Id) to compare against the persisted row; the user's account is already resolved by the caller. #529 legacy-import guard. src/Cluckwork.Infrastructure/Identity/AdminRecoveryService.cs:136 db.Users by-id+account: break-glass recovery looks up the user by email within the recovered account; the method takes accountId and the lookup is scoped to it. #265. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:126 db.RefreshTokens by-hash: login rotates the user's refresh tokens by user id (account-unique); the token hash embeds the account. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:176 db.RefreshTokens by-hash: refresh-token rotation reads by token hash; the hash is account-scoped by construction. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:299 db.RefreshTokens by-hash: same rotation path, second read by token hash. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1107 db.RefreshTokens by-user: password change revokes the user's tokens by user id (account-unique). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1262 db.RefreshTokens by-hash: logout attribution reads the token row by TokenHash == presentedHash (unique, account-scoped by construction); AccountId is in the SELECT projection (to attribute the logout to the right account), not a predicate. Review P1-3 surfaced this as a candidate because the old Contains("AccountId") read the projection as a predicate; the comparison-shape check correctly does not. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1407 db.RefreshTokens by-user: revokes all active tokens for a user by user id (account-unique). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1476 db.RefreshTokens by-hash: revokes by token hash (account-scoped). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1502 db.RefreshTokens by-hash: checks a minted token's durability by hash (account-scoped). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:128 db.RefreshTokens by-hash: login rotates the user's refresh tokens by user id (account-unique); the token hash embeds the account. +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:178 db.RefreshTokens by-hash: refresh-token rotation reads by token hash; the hash is account-scoped by construction. +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:301 db.RefreshTokens by-hash: same rotation path, second read by token hash. +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1408 db.RefreshTokens by-user: password change revokes the user's tokens by user id (account-unique). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1563 db.RefreshTokens by-hash: logout attribution reads the token row by TokenHash == presentedHash (unique, account-scoped by construction); AccountId is in the SELECT projection (to attribute the logout to the right account), not a predicate. Review P1-3 surfaced this as a candidate because the old Contains("AccountId") read the projection as a predicate; the comparison-shape check correctly does not. +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1708 db.RefreshTokens by-user: revokes all active tokens for a user by user id (account-unique). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1777 db.RefreshTokens by-hash: revokes by token hash (account-scoped). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1803 db.RefreshTokens by-hash: checks a minted token's durability by hash (account-scoped). src/Cluckwork.Infrastructure/Identity/PersistentStepUpGrantRegistry.cs:53 db.Users by-id: reads the user's StepUpLogoutEpoch by user id (account-unique) to check the grant is not revoked; #338 shared-state. src/Cluckwork.Infrastructure/Identity/PersistentStepUpGrantRegistry.cs:92 db.Users by-id: records a logout by bumping the user's StepUpLogoutEpoch (account-unique). src/Cluckwork.Infrastructure/Identity/PersistentStepUpGrantRegistry.cs:104 db.Users by-id: checks revocation by the user's StepUpLogoutEpoch (account-unique). +src/Cluckwork.Infrastructure/Identity/StepUpGrantService.cs:363 db.Users by-id: validates the grant's security stamp by user id (account-unique), then rejects when the persisted user's AccountId does not match the grant account; the read is no-tracking so a later account-locked mutation cannot identity-resolve this pre-lock row. src/Cluckwork.Infrastructure/Jobs/IdempotencyRecordPurgeSweep.cs:111 db.IdempotencyRecords non-tenant sweep: purges expired records across all tenants (runs under the single-leader gate #271 with no tenant resolved); scoped by expiry, not by account. src/Cluckwork.Infrastructure/Jobs/IdempotencyRecordPurgeSweep.cs:130 db.IdempotencyRecords non-tenant sweep: second purge query in the same sweep; scoped by expiry. src/Cluckwork.Infrastructure/Jobs/RefreshTokenPurgeSweep.cs:80 db.RefreshTokens non-tenant sweep: purges expired refresh tokens across all tenants (single-leader gate #271); scoped by expiry. @@ -67,16 +68,16 @@ src/Cluckwork.Infrastructure/Identity/AccountUserDirectory.cs:65 db.UserRoles sc src/Cluckwork.Infrastructure/Identity/AccountUserDirectory.cs:66 db.Roles global-reference: AspNetRoles has no AccountId column — roles are farm-wide reference data (the four base roles are global); the query is scoped by the join to the tenant's UserRows, not by an account predicate. src/Cluckwork.Infrastructure/Identity/FirstRunStatusService.cs:91 db.UserRoles scoped-by-join: checks the Owner role by joining db.Users (carries AccountId) on user.Id; tenant-scoped transitively through the user row. src/Cluckwork.Infrastructure/Identity/FirstRunStatusService.cs:93 db.Roles global-reference: role lookup by name (AspNetRoles is farm-wide reference data); scoped by the join to the tenant's user rows. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:741 db.UserRoles scoped-by-user-id: checks the acting user's Owner role by userRole.UserId == actingUserId (the user id is account-unique); the Role join is to global reference data. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:742 db.Roles global-reference: role lookup by name in the Owner check (AspNetRoles is farm-wide reference data). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:756 db.UserRoles scoped-by-user-id: counts other active Owners by joining UserRoles on the user id (account-unique) and the user's AccountId == accountId. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:757 db.Roles global-reference: Owner-role filter by name in the owner-count (AspNetRoles is farm-wide reference data). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:809 db.UserRoles scoped-by-user-id: DisableUser revokes the user's roles by userRole.UserId == userId (account-unique). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:810 db.Roles global-reference: role lookup by name in the disable path (AspNetRoles is farm-wide reference data). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1143 db.UserRoles scoped-by-account: lists the account's users with their roles; the query carries u.AccountId == accountId (the UserRoles join is scoped by the user rows). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1144 db.Roles global-reference: role name resolution in ListUsers (AspNetRoles is farm-wide reference data). -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1188 db.UserRoles scoped-by-account: GetUser loads the user's roles; the method takes accountId and the user lookup is scoped to it. -src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1189 db.Roles global-reference: role name resolution in GetUser (AspNetRoles is farm-wide reference data). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1042 db.UserRoles scoped-by-user-id: checks the acting user's Owner role by userRole.UserId == actingUserId (the user id is account-unique); the Role join is to global reference data. +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1043 db.Roles global-reference: role lookup by name in the Owner check (AspNetRoles is farm-wide reference data). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1057 db.UserRoles scoped-by-user-id: counts other active Owners by joining UserRoles on the user id (account-unique) and the user's AccountId == accountId. +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1058 db.Roles global-reference: Owner-role filter by name in the owner-count (AspNetRoles is farm-wide reference data). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1110 db.UserRoles scoped-by-user-id: DisableUser revokes the user's roles by userRole.UserId == userId (account-unique). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1111 db.Roles global-reference: role lookup by name in the disable path (AspNetRoles is farm-wide reference data). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1444 db.UserRoles scoped-by-account: lists the account's users with their roles; the query carries u.AccountId == accountId (the UserRoles join is scoped by the user rows). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1445 db.Roles global-reference: role name resolution in ListUsers (AspNetRoles is farm-wide reference data). +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1489 db.UserRoles scoped-by-account: GetUser loads the user's roles; the method takes accountId and the user lookup is scoped to it. +src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:1490 db.Roles global-reference: role name resolution in GetUser (AspNetRoles is farm-wide reference data). src/Cluckwork.Infrastructure/Jobs/DurableJobWorker.cs:149 db.DurableJobs non-tenant table: DurableJob has no AccountId column by design (#271 at-most-one-leader lease, single-leader gate); the worker runs with no tenant resolved and polls by status/RunAfter. src/Cluckwork.Infrastructure/Persistence/DemoDataSeeder.cs:204 db.Roles global-reference: dev-only seeder ensures the base role rows exist (AspNetRoles is farm-wide reference data); runs at unresolved tenant by design (#280). src/Cluckwork.Infrastructure/Persistence/SimulationDataSeeder.cs:455 db.Roles global-reference: simulation seeder ensures base role rows (AspNetRoles is farm-wide reference data); runs at unresolved tenant by design (#279). diff --git a/web/src/api/cluckwork.ts b/web/src/api/cluckwork.ts index 897309c99..e8ff099fc 100644 --- a/web/src/api/cluckwork.ts +++ b/web/src/api/cluckwork.ts @@ -769,6 +769,13 @@ export const changeUserRole = ( ) => apiPut( `/users/${id}/role`, body, key, stepUpToken ? { [STEP_UP_HEADER]: stepUpToken } : undefined); +export const changeUserEmail = ( + id: string, body: { email: string }, key?: string, stepUpToken?: string, +) => apiPut( + `/users/${id}/email`, body, key, + stepUpToken ? { [STEP_UP_HEADER]: stepUpToken } : undefined, +); + // #356 — disable a user: revokes every session and refuses further sign-in. // Unlike setUserPassword/changeUserRole, whose step-up is gated only when the // TARGET holds Owner, stepUpToken is required UNCONDITIONALLY here — a disable diff --git a/web/src/i18n/en.ts b/web/src/i18n/en.ts index dfda7deda..c3dee462e 100644 --- a/web/src/i18n/en.ts +++ b/web/src/i18n/en.ts @@ -1400,6 +1400,7 @@ export const en = { stepUpCreateHint: "Creating another Owner needs your current password again.", stepUpResetHint: "Resetting an Owner's password needs your current password again.", stepUpRoleHint: "Promoting someone to Owner needs your current password again.", + stepUpEmailHint: "Changing a login email needs your current password again.", // #356 — disable/enable's step-up is UNCONDITIONAL (every disable/enable // needs it, not just an Owner target), so these hints are shown every time // rather than only once a sensitive role is picked. @@ -1415,6 +1416,8 @@ export const en = { resetPasswordButton: "password", changeRoleButton: "role", changeRoleSubmitButton: "Change role", + changeEmailButton: "change email", + changeEmailSubmitButton: "Change email", flocksButton: "flocks", // #356 — row actions and their badge. Disabled rows render muted with this // badge (StatusBadge tinted via the "Inactive" status, labelled distinctly @@ -1454,6 +1457,14 @@ export const en = { + "account's last Owner cannot be demoted, and you cannot change " + "your own role — ask another Owner.", + changeEmailTitle: "Change email — {{email}}", + changeEmailHint: + "This address becomes the user's next login immediately and their open sessions end. " + + "No confirmation email is sent.", + loginEmailFieldLabel: "Login email", + duplicateEmailMessage: "A user with this email already exists.", + lastOwnerEmailMessage: "Add a second Owner before changing your own login email.", + // #356 — the single disable/enable dialog: it IS the confirmation, shared // by both modes (unconditional step-up proof, unlike the conditional // Owner-only prompts above). {{email}} is DATA. @@ -1480,6 +1491,7 @@ export const en = { "Password set for {{email}}. They have been signed out everywhere.", updatedMessage: "Updated {{email}}.", roleChangedMessage: "{{email}} is now {{role}}.", + emailChangedMessage: "Login email changed to {{email}}.", userDisabledMessage: "{{email}} has been disabled.", userEnabledMessage: "{{email}} has been re-enabled.", }, @@ -2085,6 +2097,7 @@ export const en = { "auditAction.User.PasswordChanged": "Password changed", "auditAction.User.BreakGlassReset": "Break-glass reset", "auditAction.User.RoleChanged": "Role changed", + "auditAction.User.EmailChanged": "Login email changed", "auditAction.User.Disabled": "User disabled", "auditAction.User.Enabled": "User enabled", "auditAction.User.FlockAssign": "Flock assigned to user", @@ -2244,9 +2257,9 @@ export const en = { // actions. Deliberately does NOT mention "grant"/"token" — that's internal // mechanism, not user-facing language. signingInStepUp: - "Five actions on the Users screen ask you to re-enter your current password " + "Six actions on the Users screen ask you to re-enter your current password " + "right there in the dialog: creating another Owner, resetting an existing Owner's password, promoting " - + "someone to Owner, and — whatever the target's role — disabling or re-enabling a user. This confirms it's " + + "someone to Owner, changing a login email, and — whatever the target's role — disabling or re-enabling a user. This confirms it's " + "really you before handing out that much access or cutting someone else's off; every other action on that " + "screen (creating a Worker/Manager/Sales/Read-only user, resetting one of their passwords, changing " + "someone's role to anything other than Owner) does not ask again.", @@ -2306,6 +2319,10 @@ export const en = { + "Re-enable restores their ability to sign in, but never revives the sessions the " + "disable ended — they sign in fresh with their existing password. You can't disable your own sign-in, " + "and you can't disable the account's last Admin (owner).", + rolesChangeEmail: + "Owners can replace a user's login email immediately from the Users screen. The new " + + "address is used at the next sign-in, every open session ends, and no confirmation email is sent. " + + "Changing your own address requires another active Owner so the farm cannot lose its last working Owner.", 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). " @@ -2887,6 +2904,11 @@ export const en = { + "in for you, without showing the list. If you do not know your farm's code, ask " + "your administrator — they can look it up.", + glossaryLoginEmailTerm: "Login email", + glossaryLoginEmailDef: + "The address used with a farm code to sign in. An Owner can replace it immediately on the Users " + + "screen; the old address stops working, open sessions end, and no confirmation email is sent.", + glossaryFarmProvisioningTerm: "Farm provisioning", glossaryFarmProvisioningDef: "The operator-only command that creates a new farm, its default egg grades and packed-unit " diff --git a/web/src/i18n/enums.ts b/web/src/i18n/enums.ts index 7be51f032..71f9c75ab 100644 --- a/web/src/i18n/enums.ts +++ b/web/src/i18n/enums.ts @@ -284,7 +284,7 @@ export const AUDIT_ACTION_VALUES = [ "WaterUsage.Correct", "Flock.BirdMovement", "Flock.Update", "Flock.Deplete", "Flock.Archive", "Flock.Reactivate", "EggGrade.Update", "EggGrade.Activate", "EggGrade.Deactivate", "User.Create", "User.Update", "User.PasswordSet", - "User.PasswordChanged", "User.BreakGlassReset", "User.RoleChanged", "User.Disabled", "User.Enabled", + "User.PasswordChanged", "User.BreakGlassReset", "User.RoleChanged", "User.EmailChanged", "User.Disabled", "User.Enabled", "User.FlockAssign", "User.FlockUnassign", "Account.Export", "Account.SetLogo", "Account.RemoveLogo", "Account.SetBanner", "Account.RemoveBanner", "Account.UpdateSettings", @@ -320,6 +320,7 @@ const AUDIT_ACTION_KEYS = { "User.PasswordChanged": "enums:auditAction.User.PasswordChanged", "User.BreakGlassReset": "enums:auditAction.User.BreakGlassReset", "User.RoleChanged": "enums:auditAction.User.RoleChanged", + "User.EmailChanged": "enums:auditAction.User.EmailChanged", "User.Disabled": "enums:auditAction.User.Disabled", "User.Enabled": "enums:auditAction.User.Enabled", "User.FlockAssign": "enums:auditAction.User.FlockAssign", @@ -387,6 +388,7 @@ export const AUDIT_ACTION_ENTITY_TYPE = { "User.PasswordChanged": "User", "User.BreakGlassReset": "User", "User.RoleChanged": "User", + "User.EmailChanged": "User", "User.Disabled": "User", "User.Enabled": "User", "User.FlockAssign": "User", diff --git a/web/src/i18n/es.ts b/web/src/i18n/es.ts index 37a19c556..170d3f738 100644 --- a/web/src/i18n/es.ts +++ b/web/src/i18n/es.ts @@ -1144,6 +1144,7 @@ export const es = { stepUpCreateHint: "Crear otro propietario requiere volver a ingresar tu contraseña actual.", stepUpResetHint: "Restablecer la contraseña de un propietario requiere volver a ingresar tu contraseña actual.", stepUpRoleHint: "Ascender a alguien a propietario requiere volver a ingresar tu contraseña actual.", + stepUpEmailHint: "Cambiar un correo de inicio de sesión requiere volver a ingresar tu contraseña actual.", // #356 — re-confirmación incondicional (machine-drafted, pending native review) stepUpDisableHint: "Deshabilitar a un usuario requiere volver a ingresar tu contraseña actual.", stepUpEnableHint: "Volver a habilitar a un usuario requiere volver a ingresar tu contraseña actual.", @@ -1157,6 +1158,8 @@ export const es = { resetPasswordButton: "contraseña", changeRoleButton: "rol", changeRoleSubmitButton: "Cambiar rol", + changeEmailButton: "cambiar correo", + changeEmailSubmitButton: "Cambiar correo", flocksButton: "lotes", disabledBadge: "Deshabilitado", disableButton: "deshabilitar", @@ -1193,6 +1196,14 @@ export const es = { + "dispositivos. No se puede degradar al último propietario de la " + "cuenta, y no puedes cambiar tu propio rol — pide a otro propietario.", + changeEmailTitle: "Cambiar correo — {{email}}", + changeEmailHint: + "Esta dirección se convierte de inmediato en el próximo inicio de sesión del usuario y sus sesiones abiertas terminan. " + + "No se envía ningún correo de confirmación.", + loginEmailFieldLabel: "Correo de inicio de sesión", + duplicateEmailMessage: "Ya existe un usuario con este correo electrónico.", + lastOwnerEmailMessage: "Agregue un segundo propietario antes de cambiar su propio correo de inicio de sesión.", + // #356 — el diálogo único de deshabilitar/habilitar (machine-drafted, // pending native review): el diálogo mismo ES la confirmación. disableStepUpTitle: "Deshabilitar — {{email}}", @@ -1210,6 +1221,7 @@ export const es = { passwordSetMessage: "Contraseña establecida para {{email}}. Se cerró la sesión en todos los dispositivos.", updatedMessage: "Se actualizó {{email}}.", roleChangedMessage: "{{email}} ahora es {{role}}.", + emailChangedMessage: "El correo de inicio de sesión cambió a {{email}}.", userDisabledMessage: "{{email}} ha sido deshabilitado.", userEnabledMessage: "{{email}} ha sido vuelto a habilitar.", }, @@ -1635,6 +1647,7 @@ export const es = { "auditAction.User.PasswordChanged": "Contraseña cambiada", "auditAction.User.BreakGlassReset": "Restablecimiento de emergencia", "auditAction.User.RoleChanged": "Rol cambiado", + "auditAction.User.EmailChanged": "Correo de inicio de sesión cambiado", "auditAction.User.Disabled": "Usuario deshabilitado", "auditAction.User.Enabled": "Usuario habilitado", "auditAction.User.FlockAssign": "Lote asignado al usuario", @@ -1793,9 +1806,9 @@ export const es = { // #308 (machine-drafted, pending native review) // #308/#356 (machine-drafted, pending native review) signingInStepUp: - "Cinco acciones en la pantalla Usuarios le piden volver a ingresar su contraseña " + "Seis acciones en la pantalla Usuarios le piden volver a ingresar su contraseña " + "actual directamente en el diálogo: crear otro propietario, restablecer la contraseña de un " - + "propietario existente, ascender a alguien a propietario, y — sin importar el rol del destinatario — " + + "propietario existente, ascender a alguien a propietario, cambiar un correo de inicio de sesión y — sin importar el rol del destinatario — " + "deshabilitar o volver a habilitar un usuario. Esto confirma que realmente es usted antes de otorgar " + "tanto acceso o de retirárselo a otra persona — ninguna otra acción en esa pantalla (crear un usuario " + "Worker/Manager/Sales/Read-only, restablecer su contraseña, cambiar el rol de alguien a algo distinto " @@ -1861,6 +1874,11 @@ export const es = { + "su capacidad de iniciar sesión, pero nunca revive las sesiones que terminó la deshabilitación — " + "inician sesión de nuevo con su contraseña actual. No puede deshabilitar su propio inicio de sesión, " + "ni puede deshabilitar al último Administrador (propietario) de la cuenta.", + rolesChangeEmail: + "Los propietarios pueden reemplazar de inmediato el correo de inicio de sesión de un " + + "usuario desde la pantalla Usuarios. La nueva dirección se usa en el siguiente inicio de sesión, todas " + + "las sesiones abiertas terminan y no se envía ningún correo de confirmación. Para cambiar su propia " + + "dirección debe haber otro propietario activo.", 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 " @@ -2467,6 +2485,12 @@ export const es = { + "usted, sin mostrar la lista. Si no sabe el código de su granja, pregunte a su " + "administrador: puede consultarlo.", + glossaryLoginEmailTerm: "Correo de inicio de sesión", + glossaryLoginEmailDef: + "La dirección que se usa con un código de granja para iniciar sesión. Un propietario puede reemplazarla " + + "de inmediato en la pantalla Usuarios; la dirección anterior deja de funcionar, terminan las sesiones " + + "abiertas y no se envía ningún correo de confirmación.", + // #533 (machine-drafted, pending native review) glossaryFarmProvisioningTerm: "Aprovisionamiento de granja", glossaryFarmProvisioningDef: diff --git a/web/src/i18n/tl.ts b/web/src/i18n/tl.ts index 783359513..91d25c38b 100644 --- a/web/src/i18n/tl.ts +++ b/web/src/i18n/tl.ts @@ -1186,6 +1186,7 @@ export const tl = { stepUpCreateHint: "Ang paggawa ng isa pang Owner ay nangangailangan muling ilagay ang kasalukuyan mong password.", stepUpResetHint: "Ang pag-reset ng password ng isang Owner ay nangangailangan muling ilagay ang kasalukuyan mong password.", stepUpRoleHint: "Ang pag-promote ng isang tao bilang Owner ay nangangailangan muling ilagay ang kasalukuyan mong password.", + stepUpEmailHint: "Ang pagpapalit ng email sa pag-sign in ay nangangailangan muling ilagay ang kasalukuyan mong password.", // #356 — walang kondisyong muling pagkumpirma (machine-drafted, pending native review) stepUpDisableHint: "Ang pag-disable ng isang user ay nangangailangan muling ilagay ang kasalukuyan mong password.", stepUpEnableHint: "Ang muling pag-enable ng isang user ay nangangailangan muling ilagay ang kasalukuyan mong password.", @@ -1199,6 +1200,8 @@ export const tl = { resetPasswordButton: "password", changeRoleButton: "tungkulin", changeRoleSubmitButton: "Palitan ang tungkulin", + changeEmailButton: "palitan ang email", + changeEmailSubmitButton: "Palitan ang email", flocksButton: "mga kawan", disabledBadge: "Naka-disable", disableButton: "i-disable", @@ -1237,6 +1240,14 @@ export const tl = { + "Owner ng account, at hindi mo mapapalitan ang sarili mong " + "tungkulin — humingi ng tulong sa ibang Owner.", + changeEmailTitle: "Palitan ang email — {{email}}", + changeEmailHint: + "Ang address na ito agad ang gagamitin ng user sa susunod na pag-sign in at matatapos ang mga bukas nilang session. " + + "Walang ipinapadalang confirmation email.", + loginEmailFieldLabel: "Email sa pag-sign in", + duplicateEmailMessage: "May user nang gumagamit ng email na ito.", + lastOwnerEmailMessage: "Magdagdag muna ng pangalawang Owner bago palitan ang sarili mong email sa pag-sign in.", + // #356 — ang iisang disable/enable dialog (machine-drafted, pending // native review): ang dialog mismo ANG kumpirmasyon. disableStepUpTitle: "I-disable — {{email}}", @@ -1254,6 +1265,7 @@ export const tl = { passwordSetMessage: "Naitakda ang password para sa {{email}}. Na-sign out na sila sa lahat ng lugar.", updatedMessage: "Na-update ang {{email}}.", roleChangedMessage: "Si {{email}} ay {{role}} na ngayon.", + emailChangedMessage: "Napalitan ang email sa pag-sign in at naging {{email}}.", userDisabledMessage: "Na-disable si {{email}}.", userEnabledMessage: "Na-enable ulit si {{email}}.", }, @@ -1699,6 +1711,7 @@ export const tl = { "auditAction.User.PasswordChanged": "Napalitan ang password", "auditAction.User.BreakGlassReset": "Emergency na pag-reset ng password", "auditAction.User.RoleChanged": "Binago ang tungkulin", + "auditAction.User.EmailChanged": "Binago ang email sa pag-sign in", "auditAction.User.Disabled": "Na-disable ang user", "auditAction.User.Enabled": "Na-enable ang user", "auditAction.User.FlockAssign": "Na-assign ang kawan sa user", @@ -1862,9 +1875,9 @@ export const tl = { // #308 (machine-drafted, pending native review) // #308/#356 (machine-drafted, pending native review) signingInStepUp: - "May limang aksyon sa Users screen na humihiling sa iyong muling ilagay ang " + "May anim na aksyon sa Users screen na humihiling sa iyong muling ilagay ang " + "kasalukuyan mong password mismo sa dialog: ang paggawa ng isa pang Owner, ang pag-reset " - + "ng password ng isang umiiral na Owner, ang pag-promote ng isang tao maging Owner, at — anuman ang " + + "ng password ng isang umiiral na Owner, ang pag-promote ng isang tao maging Owner, ang pagpapalit ng email sa pag-sign in, at — anuman ang " + "tungkulin ng target — ang pag-disable o muling pag-enable ng isang user. Kinukumpirma nito na ikaw " + "talaga bago bigyan ng ganoong kalaking access o bago alisin ito sa iba — walang ibang aksyon sa " + "screen na iyon (paggawa ng Worker/Manager/Sales/Read-only user, pag-reset ng password nila, " @@ -1933,6 +1946,11 @@ export const tl = { + "in, pero hindi na buhayin pa ang mga session na natapos ng pag-disable — mag-si-sign in sila ulit " + "gamit ang kasalukuyan nilang password. Hindi mo puwedeng i-disable ang sarili mong sign-in, at " + "hindi mo puwedeng i-disable ang huling Admin (may-ari) ng account.", + rolesChangeEmail: + "Maaaring agad palitan ng mga Owner ang email sa pag-sign in ng isang user sa Users " + + "screen. Ang bagong address ang gagamitin sa susunod na pag-sign in, matatapos ang lahat ng bukas na " + + "session, at walang ipinapadalang confirmation email. Kailangan ng isa pang aktibong Owner para palitan " + + "ang sarili mong address.", 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 " @@ -2559,6 +2577,12 @@ export const tl = { + "hindi ipinapakita ang listahan. Kung hindi mo alam ang code ng bukid mo, tanungin " + "ang iyong administrator: matitingnan nila ito.", + glossaryLoginEmailTerm: "Email sa pag-sign in", + glossaryLoginEmailDef: + "Ang address na ginagamit kasama ng code ng bukid para mag-sign in. Maaari itong palitan agad ng Owner " + + "sa Users screen; hindi na gagana ang lumang address, matatapos ang mga bukas na session, at walang " + + "ipinapadalang confirmation email.", + // #533 (machine-drafted, pending native review) glossaryFarmProvisioningTerm: "Pag-provision ng bukid", glossaryFarmProvisioningDef: diff --git a/web/src/routes/HelpPage.test.tsx b/web/src/routes/HelpPage.test.tsx index 419bfa3d0..0895df291 100644 --- a/web/src/routes/HelpPage.test.tsx +++ b/web/src/routes/HelpPage.test.tsx @@ -311,7 +311,7 @@ describe("HelpPage", () => { // would otherwise misstate disable/enable as ungated the moment this // feature shipped — this pins the corrected count and wording. render(); - expect(screen.getByText(/Five actions on the/i)).toBeInTheDocument(); + expect(screen.getByText(/Six actions on the/i)).toBeInTheDocument(); expect(screen.getByText(/disabling or re-enabling a user/i)).toBeInTheDocument(); expect(screen.queryByText(/Three actions on the/i)).not.toBeInTheDocument(); }); @@ -610,4 +610,35 @@ describe("HelpPage glossary i18n wiring (#182, Task 33)", () => { expect(screen.queryByText(/Prices and fractional amounts are still typed/i)) .not.toBeInTheDocument(); }); + + it("documents login-email replacement through the localized glossary row (#357)", () => { + const originalTerm = i18n.getResource("en", "help", "glossaryLoginEmailTerm") as string; + const originalDef = i18n.getResource("en", "help", "glossaryLoginEmailDef") as string; + i18n.addResource("en", "help", "glossaryLoginEmailTerm", "LOGIN-EMAIL-TERM-MARKER"); + i18n.addResource("en", "help", "glossaryLoginEmailDef", "LOGIN-EMAIL-DEF-MARKER"); + try { + render(); + expect(screen.getByRole("rowheader", { name: "LOGIN-EMAIL-TERM-MARKER" })).toBeInTheDocument(); + expect(screen.getByText("LOGIN-EMAIL-DEF-MARKER")).toBeInTheDocument(); + } finally { + i18n.addResource("en", "help", "glossaryLoginEmailTerm", originalTerm); + i18n.addResource("en", "help", "glossaryLoginEmailDef", originalDef); + } + + expect(en.help.glossaryLoginEmailDef).toMatch(/no confirmation email/i); + expect(es.help.glossaryLoginEmailDef).toMatch(/no se envía.*confirmación/i); + expect(tl.help.glossaryLoginEmailDef).toMatch(/walang.*confirmation email/i); + for (const catalog of [es, tl]) { + expect(catalog.help.glossaryLoginEmailTerm).not.toBe(en.help.glossaryLoginEmailTerm); + expect(catalog.help.glossaryLoginEmailDef).not.toBe(en.help.glossaryLoginEmailDef); + } + }); + + it("explains the Owner change-email flow in Who can do what (#357)", () => { + render(); + const guidance = screen.getByText((_, element) => + element?.tagName === "LI" + && /Owners can replace a user's login email immediately/i.test(element.textContent ?? "")); + expect(guidance).toHaveTextContent(/no confirmation email is sent/i); + }); }); diff --git a/web/src/routes/HelpPage.tsx b/web/src/routes/HelpPage.tsx index f6f281830..7b9581a7f 100644 --- a/web/src/routes/HelpPage.tsx +++ b/web/src/routes/HelpPage.tsx @@ -173,6 +173,9 @@ export function HelpPage() { {/* #356 — disable/re-enable a colleague's sign-in. */} }} /> +
  • + }} /> +

    • @@ -548,6 +551,8 @@ export function HelpPage() { {/* #532 — the login screen now asks for it before the email. */} {t("glossaryFarmCodeTerm")} }} /> + {t("glossaryLoginEmailTerm")} + {t("glossaryLoginEmailDef")} {t("glossaryFarmProvisioningTerm")} {t("glossaryFarmProvisioningDef")} {t("glossaryTooManySignInAttemptsTerm")} diff --git a/web/src/routes/UsersPage.test.tsx b/web/src/routes/UsersPage.test.tsx index d614dfd02..f50638358 100644 --- a/web/src/routes/UsersPage.test.tsx +++ b/web/src/routes/UsersPage.test.tsx @@ -1,8 +1,11 @@ import { useState } from "react"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { screen, within, fireEvent, act, render } from "@testing-library/react"; -import { MemoryRouter } from "react-router"; +import userEvent from "@testing-library/user-event"; +import { MemoryRouter, Route, Routes } from "react-router"; import { UsersPage } from "./UsersPage"; +import { Login } from "./Login"; +import { ProtectedRoute } from "./ProtectedRoute"; import { renderWithProviders } from "../test/renderWithProviders"; import { farmState, NO_RECORD_HISTORY } from "../test/fixtures"; import { AuthContext } from "../auth/AuthContext"; @@ -11,11 +14,12 @@ import { FarmContext } from "../farm/FarmContext"; import { MeContext } from "../session/SessionContext"; import type { Me } from "../api/cluckwork"; import { - assignFlock, changeUserRole, createUser, disableUser, enableUser, listFlockAssignments, listFlocks, + assignFlock, changeUserEmail, changeUserRole, createUser, disableUser, enableUser, listFlockAssignments, listFlocks, listUsers, setUserPassword, unassignFlock, updateUser, } from "../api/cluckwork"; import type { Flock, FlockAssignment, User } from "../api/cluckwork"; -import { ApiError, stepUp } from "../api/client"; +import { apiGet, ApiError, stepUp } from "../api/client"; +import { getAccessToken } from "../auth/tokenStore"; import i18n from "../i18n"; // Runtime-generated, with NO static substring — GitGuardian's scanner @@ -33,6 +37,7 @@ vi.mock("../api/cluckwork", () => ({ updateUser: vi.fn(), setUserPassword: vi.fn(), changeUserRole: vi.fn(), + changeUserEmail: vi.fn(), disableUser: vi.fn(), enableUser: vi.fn(), listFlockAssignments: vi.fn(), @@ -54,6 +59,7 @@ const mockCreateUser = vi.mocked(createUser); const mockUpdateUser = vi.mocked(updateUser); const mockSetUserPassword = vi.mocked(setUserPassword); const mockChangeUserRole = vi.mocked(changeUserRole); +const mockChangeUserEmail = vi.mocked(changeUserEmail); const mockDisableUser = vi.mocked(disableUser); const mockEnableUser = vi.mocked(enableUser); const mockListAssignments = vi.mocked(listFlockAssignments); @@ -449,6 +455,299 @@ describe("UsersPage change role (#355)", () => { }); }); +describe("UsersPage change email (#357)", () => { + const openEmail = (rowName: RegExp) => + fireEvent.click(within(screen.getByRole("row", { name: rowName })) + .getByRole("button", { name: /change email/i })); + const emailInput = () => within(dialog()).getByLabelText("Login email"); + const passwordInput = () => within(dialog()).getByLabelText(/Your current password/); + const submit = () => within(dialog()).getByRole("button", { name: "Change email" }); + + function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((ok, fail) => { resolve = ok; reject = fail; }); + return { promise, resolve, reject }; + } + + it("opens a dedicated Change email dialog for the selected row", async () => { + await renderReady(ADMIN); + + openEmail(/worker@farm.test/); + + expect(screen.getByRole("dialog", { name: /Change email — worker@farm\.test/ })).toBeInTheDocument(); + expect(emailInput()).toHaveValue("worker@farm.test"); + expect(passwordInput()).toHaveAttribute("autocomplete", "current-password"); + }); + + it("requires step-up, clears the password, and sends trimmed email with one idempotency key", async () => { + const grant = deferred<{ token: string; expiresAt: string }>(); + mockStepUp.mockReturnValue(grant.promise); + mockChangeUserEmail.mockResolvedValue(undefined); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: " corrected@farm.test " } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + + await act(async () => { fireEvent.click(submit()); }); + expect(mockStepUp).toHaveBeenCalledWith(OWNER_STEP_UP_PASSWORD); + expect(passwordInput()).toHaveValue(""); + + await act(async () => grant.resolve({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" })); + expect(mockChangeUserEmail).toHaveBeenCalledWith( + "u-w", { email: "corrected@farm.test" }, expect.any(String), "email-grant"); + expect(mockChangeUserEmail).toHaveBeenCalledTimes(1); + }); + + it("renders Users.DuplicateEmail beside the email input with aria-invalid", async () => { + mockStepUp.mockResolvedValue({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" }); + mockChangeUserEmail.mockRejectedValue( + new ApiError(409, "Users.DuplicateEmail", "A user with this email already exists.")); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: "taken@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + + await act(async () => { fireEvent.click(submit()); }); + + const fieldError = within(dialog()).getByText("A user with this email already exists."); + expect(fieldError).toHaveAttribute("role", "alert"); + expect(emailInput()).toHaveAttribute("aria-invalid", "true"); + expect(emailInput()).toHaveAttribute("aria-describedby", fieldError.id); + expect(within(dialog()).getAllByRole("alert")).toEqual([fieldError]); + }); + + it("clears the field error on edit, close, and target change", async () => { + mockStepUp.mockResolvedValue({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" }); + mockChangeUserEmail.mockRejectedValue( + new ApiError(409, "Users.DuplicateEmail", "A user with this email already exists.")); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: "taken@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + await act(async () => { fireEvent.click(submit()); }); + expect(emailInput()).toHaveAttribute("aria-invalid", "true"); + + fireEvent.change(emailInput(), { target: { value: "fixed@farm.test" } }); + expect(emailInput()).toHaveAttribute("aria-invalid", "false"); + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + openEmail(/worker@farm.test/); + expect(emailInput()).toHaveAttribute("aria-invalid", "false"); + openEmail(/boss@farm.test/); + expect(emailInput()).toHaveValue("boss@farm.test"); + expect(emailInput()).toHaveAttribute("aria-invalid", "false"); + }); + + it.each([ + ["same", /worker@farm\.test/, "worker@farm.test"], + ["different", /boss@farm\.test/, "boss@farm.test"], + ])("cancel/reopen for the %s target invalidates a step-up continuation that later succeeds", + async (_, reopenedRow, reopenedEmail) => { + const late = deferred<{ token: string; expiresAt: string }>(); + mockStepUp.mockReturnValue(late.promise); + mockChangeUserEmail.mockResolvedValue(undefined); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: "abandoned@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + await act(async () => { fireEvent.click(submit()); }); + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + openEmail(reopenedRow); + + await act(async () => late.resolve({ token: "abandoned-grant", expiresAt: "2026-01-01T00:05:00Z" })); + + expect(mockChangeUserEmail).not.toHaveBeenCalled(); + expect(emailInput()).toHaveValue(reopenedEmail); + }); + + it.each([ + ["same", /worker@farm\.test/, "worker@farm.test"], + ["different", /boss@farm\.test/, "boss@farm.test"], + ])("cancel/reopen for the %s target ignores a step-up continuation that later fails", + async (_, reopenedRow, reopenedEmail) => { + const late = deferred<{ token: string; expiresAt: string }>(); + mockStepUp.mockReturnValue(late.promise); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: "abandoned@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + await act(async () => { fireEvent.click(submit()); }); + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + openEmail(reopenedRow); + + await act(async () => late.reject( + new ApiError(403, "StepUp.Invalid", "The abandoned password proof failed."))); + + expect(emailInput()).toHaveValue(reopenedEmail); + expect(within(dialog()).queryByRole("alert")).not.toBeInTheDocument(); + }); + + it.each([ + ["same", /worker@farm\.test/, "worker@farm.test"], + ["different", /boss@farm\.test/, "boss@farm.test"], + ])("after a PUT is sent, cancel/reopen for the %s target ignores its late success in the reopened dialog", + async (_, reopenedRow, reopenedEmail) => { + const late = deferred(); + mockStepUp.mockResolvedValue({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" }); + mockChangeUserEmail.mockReturnValue(late.promise); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: "abandoned@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + await act(async () => { fireEvent.click(submit()); }); + // Cancel abandons this dialog instance's UI ownership; it cannot abort + // the PUT that changeUserEmail has already sent to the server. + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + openEmail(reopenedRow); + + await act(async () => late.resolve(undefined)); + + expect(screen.getByRole("dialog")).toBeInTheDocument(); + expect(emailInput()).toHaveValue(reopenedEmail); + expect(screen.queryByText(/Login email changed to abandoned@farm\.test/)).not.toBeInTheDocument(); + }); + + it.each([ + ["same", /worker@farm\.test/, "worker@farm.test"], + ["different", /boss@farm\.test/, "boss@farm.test"], + ])("after a PUT is sent, cancel/reopen for the %s target ignores its late failure in the reopened dialog", + async (_, reopenedRow, reopenedEmail) => { + const late = deferred(); + mockStepUp.mockResolvedValue({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" }); + mockChangeUserEmail.mockReturnValue(late.promise); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: "taken@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + await act(async () => { fireEvent.click(submit()); }); + // Cancel abandons this dialog instance's UI ownership; it cannot abort + // the PUT that changeUserEmail has already sent to the server. + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + openEmail(reopenedRow); + + await act(async () => late.reject( + new ApiError(409, "Users.DuplicateEmail", "A user with this email already exists."))); + + expect(emailInput()).toHaveValue(reopenedEmail); + expect(emailInput()).toHaveAttribute("aria-invalid", "false"); + expect(within(dialog()).queryByText("A user with this email already exists.")).not.toBeInTheDocument(); + }); + + it("freezes the submitted email while a late duplicate response is pending for the same target", async () => { + const user = userEvent.setup(); + const late = deferred(); + mockStepUp.mockResolvedValue({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" }); + mockChangeUserEmail.mockReturnValue(late.promise); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: "taken@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + + await act(async () => { fireEvent.click(submit()); }); + + expect(emailInput()).toBeDisabled(); + expect(passwordInput()).toBeDisabled(); + await user.type(emailInput(), "different@farm.test"); + expect(emailInput()).toHaveValue("taken@farm.test"); + + await act(async () => late.reject( + new ApiError(409, "Users.DuplicateEmail", "A user with this email already exists."))); + + expect(emailInput()).toHaveValue("taken@farm.test"); + expect(emailInput()).toHaveAttribute("aria-invalid", "true"); + }); + + it("does not render a generic dialog failure when a self-change refresh reports credentials superseded", async () => { + mockListUsers.mockReset() + .mockResolvedValueOnce([WORKER_USER, ADMIN_USER, SELF_USER]) + .mockRejectedValueOnce(new ApiError( + 401, "Auth.CredentialsSuperseded", "Your credentials changed.")); + mockStepUp.mockResolvedValue({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" }); + mockChangeUserEmail.mockResolvedValue(undefined); + await renderReady(ADMIN); + openEmail(/self@farm.test/); + fireEvent.change(emailInput(), { target: { value: "new-self@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + + await act(async () => { fireEvent.click(submit()); }); + + expect(mockListUsers).toHaveBeenCalledTimes(2); + expect(within(dialog()).queryByText("Your credentials changed.")).not.toBeInTheDocument(); + }); + + it("self-change lets the next authenticated request enter the credentials-superseded sign-in path", async () => { + const fetchMock = vi.fn() + .mockResolvedValueOnce(new Response(JSON.stringify({ + title: "Auth.CredentialsSuperseded", + detail: "Your credentials changed.", + }), { status: 401, headers: { "Content-Type": "application/problem+json" } })) + .mockResolvedValueOnce(new Response(JSON.stringify({ + title: "Identity.InvalidRefreshToken", + detail: "The refresh token is invalid.", + }), { status: 401, headers: { "Content-Type": "application/problem+json" } })); + vi.stubGlobal("fetch", fetchMock); + mockListUsers.mockReset() + .mockResolvedValueOnce([WORKER_USER, ADMIN_USER, SELF_USER]) + .mockImplementationOnce(() => apiGet("/users")); + mockStepUp.mockResolvedValue({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" }); + mockChangeUserEmail.mockResolvedValue(undefined); + renderWithProviders( + + } /> + }> + } /> + + , + { + route: "/users", + token: { sub: "u1", role: "Admin", account_id: "account-1" }, + me: { + id: SELF_USER.id, email: SELF_USER.email, name: null, role: SELF_USER.role, + language: null, preferredStepperUnit: null, + }, + }, + ); + await screen.findByText("worker@farm.test"); + openEmail(/self@farm.test/); + fireEvent.change(emailInput(), { target: { value: "new-self@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + + await act(async () => { fireEvent.click(submit()); }); + + expect(await screen.findByText(i18n.t("auth:credentialsSuperseded"))).toBeInTheDocument(); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + expect(mockListUsers).toHaveBeenCalledTimes(2); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(getAccessToken()).toBeNull(); + }); + + it("a successful non-self change refreshes the row and reports the new email", async () => { + mockStepUp.mockResolvedValue({ token: "email-grant", expiresAt: "2026-01-01T00:05:00Z" }); + mockChangeUserEmail.mockResolvedValue(undefined); + mockListUsers + .mockResolvedValueOnce([WORKER_USER, ADMIN_USER]) + .mockResolvedValueOnce([{ ...WORKER_USER, email: "corrected@farm.test" }, ADMIN_USER]); + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + fireEvent.change(emailInput(), { target: { value: "corrected@farm.test" } }); + fireEvent.change(passwordInput(), { target: { value: OWNER_STEP_UP_PASSWORD } }); + + await act(async () => { fireEvent.click(submit()); }); + + expect(await screen.findByText("corrected@farm.test")).toBeInTheDocument(); + expect(screen.getByText(/Login email changed to corrected@farm\.test/)).toBeInTheDocument(); + expect(mockListUsers).toHaveBeenCalledTimes(2); + }); + + it("does not claim that a confirmation email will be sent", async () => { + await renderReady(ADMIN); + openEmail(/worker@farm.test/); + + expect(within(dialog()).getByText(/no confirmation email is sent/i)).toBeInTheDocument(); + expect(within(dialog()).queryByText(/confirmation email will be sent/i)).not.toBeInTheDocument(); + }); +}); + describe("UsersPage change-role step-up (#308, #355)", () => { const openRole = (rowName: RegExp) => fireEvent.click(within(screen.getByRole("row", { name: rowName })).getByRole("button", { name: "role" })); diff --git a/web/src/routes/UsersPage.tsx b/web/src/routes/UsersPage.tsx index 1ca16f607..63ac9555d 100644 --- a/web/src/routes/UsersPage.tsx +++ b/web/src/routes/UsersPage.tsx @@ -1,9 +1,9 @@ import { useEffect, useId, useRef, useState } from "react"; import type { FormEvent } from "react"; import { useTranslation } from "react-i18next"; -import { Ban, KeyRound, Pencil, Plus, RotateCcw, ShieldCheck } from "lucide-react"; +import { Ban, KeyRound, Mail, Pencil, Plus, RotateCcw, ShieldCheck } from "lucide-react"; import { - assignFlock, changeUserRole, createUser, disableUser, enableUser, listFlockAssignments, listFlocks, + assignFlock, changeUserEmail, changeUserRole, createUser, disableUser, enableUser, listFlockAssignments, listFlocks, listUsers, setUserPassword, unassignFlock, updateUser, } from "../api/cluckwork"; import type { Flock, FlockAssignment, User } from "../api/cluckwork"; @@ -112,6 +112,15 @@ export function UsersPage() { const [roleValue, setRoleValue] = useState("Worker"); const activeRole = useRef(null); + const [emailUser, setEmailUser] = useState(null); + const [emailValue, setEmailValue] = useState(""); + const [emailStepUpPassword, setEmailStepUpPassword] = useState(""); + const [emailFieldError, setEmailFieldError] = useState(null); + const emailDialogGeneration = useRef(0); + const activeEmail = useRef<{ targetId: string; generation: number } | null>(null); + const emailHintId = useId(); + const emailErrorId = useId(); + // #356 — disable/enable a user, both behind ONE dialog that is itself the // confirmation: a destructive warning body, an OPTIONAL reason (disable // only — the API's DisableUserCommand.Reason is nullable, capped at 200 @@ -162,6 +171,18 @@ export function UsersPage() { }; const clearKey = (scope: string) => keys.current.delete(scope); + useEffect(() => { + if (!isAuthenticated) { + const active = activeEmail.current; + if (active !== null) keys.current.delete(`change-email:${active.targetId}`); + activeEmail.current = null; + setEmailStepUpPassword(""); + setEmailFieldError(null); + setEmailUser(null); + errors.abandon("change-email"); + } + }, [isAuthenticated, errors]); + useEffect(() => { Promise.all([listUsers(), listFlocks()]) .then(([u, f]) => { @@ -440,6 +461,76 @@ export function UsersPage() { }); } + function openEmail(u: User) { + if (emailUser !== null && emailUser.id !== u.id) { + clearKey(`change-email:${emailUser.id}`); + errors.abandon("change-email"); + } + setMessage(null); + setEmailValue(u.email); + setEmailStepUpPassword(""); + setEmailFieldError(null); + activeEmail.current = { + targetId: u.id, + generation: ++emailDialogGeneration.current, + }; + setEmailUser(u); + } + + function closeEmail() { + const active = activeEmail.current; + activeEmail.current = null; + if (active !== null) clearKey(`change-email:${active.targetId}`); + setEmailValue(""); + setEmailStepUpPassword(""); + setEmailFieldError(null); + setEmailUser(null); + errors.abandon("change-email"); + } + + async function onChangeEmail(e: FormEvent) { + e.preventDefault(); + const target = emailUser; + if (!target || busy) return; + const targetId = target.id; + const scope = `change-email:${targetId}`; + const dialog = activeEmail.current; + if (dialog === null || dialog.targetId !== targetId) return; + const isCurrentDialog = () => activeEmail.current?.generation === dialog.generation; + errors.beginAttempt("change-email"); + setEmailFieldError(null); + setMessage(null); + await run(scope, async () => { + try { + const password = emailStepUpPassword; + setEmailStepUpPassword(""); + const grant = await stepUp(password); + if (!isCurrentDialog()) return; + const trimmedEmail = emailValue.trim(); + await changeUserEmail(targetId, { email: trimmedEmail }, keyFor(scope), grant.token); + if (!isCurrentDialog()) return; + clearKey(scope); + const fresh = await listUsers(); + if (!isCurrentDialog()) return; + setUsers(fresh); + setMessage(i18n.t("users:emailChangedMessage", { email: trimmedEmail })); + closeEmail(); + } catch (err) { + if (!isCurrentDialog()) return; + if (targetId === myId && err instanceof ApiError + && err.status === 401 && err.title === "Auth.CredentialsSuperseded") { + return; + } else if (err instanceof ApiError && err.status === 409 && err.title === "Users.DuplicateEmail") { + setEmailFieldError(i18n.t("users:duplicateEmailMessage")); + } else if (err instanceof ApiError && err.title === "Users.LastOwner") { + errors.report("change-email", i18n.t("users:lastOwnerEmailMessage")); + } else { + errors.report("change-email", errText(err)); + } + } + }); + } + // #356 — open/close the shared disable/enable dialog. The Disable and Enable // row buttons call this identically; "mode" is the only difference. The // reason is not a parameter: the dialog collects it itself, seeded blank, @@ -647,6 +738,9 @@ export function UsersPage() { + {u.role === "Worker" && ( + + {t("changeEmailSubmitButton")} + + + + + {/* #356 — one dialog is the whole disable/enable flow: it IS the confirmation (no separate askReason step), collects the optional reason (disable only — the API's reason is nullable, never