feat(api): promote/demote a user's role - #475
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3b7b9d3d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| + "and assigning workers to flocks. A user's name can be changed later from the row's <strong>edit</strong> " | ||
| + "action, and the <strong>password</strong> action sets a forgotten password without needing the old " | ||
| + "one. Changing an existing user's role comes with a later release. Controls you can't use are hidden, " | ||
| + "one. The <strong>role</strong> action promotes or demotes an existing user among the five roles — it " |
There was a problem hiding this comment.
Owners reading the updated Help page are told to use a row-level role action, but the shipped web/src/routes/UsersPage.tsx still exposes only edit, password, and flock-assignment actions, and a repo-wide search shows no role-change client in web/src/api/cluckwork.ts. Thus the new endpoint cannot be used through the SPA as documented; either add the client/control in this change or describe it as API-only/future UI until that action exists.
AGENTS.md reference: AGENTS.md:L169-L169
Useful? React with 👍 / 👎.
codex+pi review of PR #475's diff surfaced three real issues beyond the plan-review rounds: - ChangeUserRoleAsync's actor re-check reused a stale EF-tracked ApplicationUser (loaded earlier by StepUpGrantService.ValidateAsync on the same scoped DbContext), so a disabled actor's already-queued promotion could still succeed once #356 ships DisabledAt writes. Fixed with an AsNoTracking projection + a fresh role join, instead of the identity-map-poisoned FirstOrDefaultAsync/GetRolesAsync pair. - A role change bumped CredentialEpoch and revoked refresh tokens but never rotated SecurityStamp, so a step-up grant (#308) issued to the target just before their own role changed stayed spendable after they signed back in. Fixed via UserManager.UpdateSecurityStampAsync, handling its IdentityResult the same way as the role-row mutations. - GLOSSARY.md/en.ts described a Users-row "role" action that the SPA never shipped (API-only). Built it: changeUserRole in cluckwork.ts, a UsersPage dialog mirroring the existing password-reset dialog's step-up flow, and en/es/tl copy. Both fixes are mutation-tested (reverted, confirmed the new test goes red, restored, confirmed green). Full suite green: backend 1449/1449, frontend 1465+186/1465+186, typecheck clean.
|
Ran a local codex + pi review pass against this PR's diff (not just the pre-code plan) and fixed the three real findings in 8b0c01a:
pi's other findings (last-Owner count needing Full suite green: backend 1449/1449, frontend 1465+186/1465+186, typecheck clean. @codex please re-review. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
An Owner can change an existing user's role to any assignable role or plain worker. Self-targeting is refused; demoting the account's last active Owner is refused (account-wide locked, race-safe against concurrent demotions); promoting to Owner requires a step-up grant, matching CreateUser's existing threshold. Any real change bumps the target's credential epoch and revokes their sessions, and is audited (User.RoleChanged, old/new role arrays). Guards against two real gaps found in review: Identity permits more than one role row per user even though every write path assigns exactly one, so role removal is plural and the audit payload carries full role-name arrays, not a single old/new string; and an actor whose own Owner-ness is revoked while their unrelated request sits queued behind the account lock is re-verified inside the transaction, closing a stale-authorization race that would otherwise let a just-demoted actor complete a pending promotion. Plan reviewed through three rounds with codex + a local model before implementation; every finding traced to a concrete test or code change.
codex+pi review of PR #475's diff surfaced three real issues beyond the plan-review rounds: - ChangeUserRoleAsync's actor re-check reused a stale EF-tracked ApplicationUser (loaded earlier by StepUpGrantService.ValidateAsync on the same scoped DbContext), so a disabled actor's already-queued promotion could still succeed once #356 ships DisabledAt writes. Fixed with an AsNoTracking projection + a fresh role join, instead of the identity-map-poisoned FirstOrDefaultAsync/GetRolesAsync pair. - A role change bumped CredentialEpoch and revoked refresh tokens but never rotated SecurityStamp, so a step-up grant (#308) issued to the target just before their own role changed stayed spendable after they signed back in. Fixed via UserManager.UpdateSecurityStampAsync, handling its IdentityResult the same way as the role-row mutations. - GLOSSARY.md/en.ts described a Users-row "role" action that the SPA never shipped (API-only). Built it: changeUserRole in cluckwork.ts, a UsersPage dialog mirroring the existing password-reset dialog's step-up flow, and en/es/tl copy. Both fixes are mutation-tested (reverted, confirmed the new test goes red, restored, confirmed green). Full suite green: backend 1449/1449, frontend 1465+186/1465+186, typecheck clean.
Rebasing onto main (#474/#476 landed since this branch was cut) shifted IdentityProvider.cs's ResetPasswordAndRevokeAsync call site again; AuditVocabularyCoverageTests' line-pinned exemption is deliberately brittle to exactly this. Re-confirmed it's still the same call site and updated the pin. Full suite green post-rebase: backend 1454/1454, frontend 74 files / 1537 tests, typecheck clean.
8b0c01a to
ce1c198
Compare
1 participant
Summary
Closes #355.
An Owner can change an existing user's role to any assignable role, or to plain worker. Design was interviewed (grilling skill), architected (feature-dev code-architect), then reviewed through three rounds with codex (full repo read access) and a local model (plan-only) before any code was written — every finding traced to a concrete guard, test, or doc fix below.
Guards
SetUserPassword's existing precedent.Users.LastOwner) — demoting the account's only Owner is refused. Race-safe: the whole operation takes the account-wide row lock unconditionally (matchingUpdateFarmSettingsHandler's Close the §4.6 currency-lock race properly (shared lock on the account row across money-writing handlers) #162 precedent), so two concurrent demotions can't both pass the count check and reach zero Owners. Excludes disabled Owners from the survivor count — a no-op today (nothing setsDisabledAtyet) but closes a real landmine once the sibling disable-user slice ships.Identity.StepUpRequired), matchingCreateUser's existing threshold exactly — every other target role is ungated.oldRoles/newRolesarrays, not a single string, so a "stray row removed but effective role unchanged" case reads correctly instead of a misleadingX -> X.UserManagerswallows a concurrency loss into a failedIdentityResultrather than throwing (this repo's own documented prior incident, seeAccountLockout.cs), so that's inspected explicitly; plus the usualDbUpdateConcurrencyExceptioncatch on the final save, for a race against an unlockedSetUserPassword/UpdateUsercall on the same target.Any real change bumps the target's
CredentialEpochand revokes their refresh tokens (unconditionally, both promotion and demotion) and is audited (User.RoleChanged). A true no-op (requested role already matches, exact set equality — correctly handles Worker's zero-role-row case) skips all side effects.Docs
specs/product/GLOSSARY.md, the Help page, and the in-app glossary (en/es/tl) are updated in place — not just a new bullet appended — since the existing step-up copy said "two actions" and role editing was described as "coming later." Source comments onStepUpGrantService,IStepUpGrantService,AuthEndpoints, andInMemoryStepUpGrantRegistryupdated to say three gated actions. SPA audit-vocabulary catalog (enums.ts+ en/es/tl) updated to keepAuditVocabularyCoverageTestsgreen.Test plan
ChangeUserRoleTests.cs(HTTP-driven): promote/demote round-trip, self-target 400, step-up required/not-required, non-Owner 403, foreign-account 404 (role unchanged), validator 400s, no-op (incl. Worker→Worker) skips epoch/revoke/audit, multi-role cleanup, epoch bumps by exactly 1 with literalRevokedAtassertion, audit content, idempotency replay doesn't double-consume step-up, oversized body 413.ChangeUserRoleRaceTests.cs(DI-resolved, barrier-controlled — theUsers.LastOwnerguard is structurally unreachable via a legitimate non-racing HTTP actor given OwnerOnly + the self-block, so it's exercised directly against the provider): sole-Owner self-demotion hits the guard, disabled-Owner exclusion, disabled-actor 403, mutual-demotion race (lock serializes, second fails as stale actor), asymmetric race (both legitimate ops succeed), stale-actor race, and a deterministically constructedIdentityResult-level concurrency conflict (fence the target user row, not the account row, and mutate the stamp out from under a queuedAddToRoleAsync).ConcurrencyFailuredetection), confirmed the corresponding test(s) went red, reverted.dotnet build Cluckwork.sln— clean, 0 warnings.dotnet test Cluckwork.sln— 315 Domain + 144 Application + 1027 API integration, all green.npm run typecheck,npm run i18n:scan(COUNT: 3, unchanged baseline), fullvitest run(1456 tests) — all green.