diff --git a/apps/web/components/settings/system/users-table.tsx b/apps/web/components/settings/system/users-table.tsx index 74da531b3f..7e40a09f76 100644 --- a/apps/web/components/settings/system/users-table.tsx +++ b/apps/web/components/settings/system/users-table.tsx @@ -85,10 +85,12 @@ function StatusBadge({ status }: { status: string }) { function UserRow({ user, + isLastActiveAdmin, onToggleRole, onToggleStatus, }: { user: AuthUser; + isLastActiveAdmin: boolean; onToggleRole: (user: AuthUser) => void; onToggleStatus: (user: AuthUser) => void; }) { @@ -116,6 +118,7 @@ function UserRow({ variant="ghost" className="cursor-pointer" onClick={() => onToggleRole(user)} + disabled={isLastActiveAdmin} data-testid="users-table-toggle-role" > {/* Two whole-word variants get their own keys rather than @@ -128,6 +131,7 @@ function UserRow({ variant="ghost" className="cursor-pointer text-destructive" onClick={() => onToggleStatus(user)} + disabled={isLastActiveAdmin} data-testid="users-table-toggle-status" > {isDisabled ? t("system:usersEnable") : t("system:usersDisable")} @@ -203,6 +207,10 @@ function UsersTableList({ onToggleStatus: (u: AuthUser) => void; }) { const { t } = useTranslation(); + // Mirrors the backend last-admin guard (ensureAnotherAdmin): an active + // admin cannot be demoted or disabled when no other active admin exists, + // so those toggles are greyed out on that row. + const activeAdminCount = users.filter((u) => u.role === "admin" && u.status === "active").length; return ( @@ -215,14 +223,19 @@ function UsersTableList({ - {users.map((user) => ( - - ))} + {users.map((user) => { + const isLastActiveAdmin = + user.role === "admin" && user.status === "active" && activeAdminCount === 1; + return ( + + ); + })}
); diff --git a/apps/web/e2e/playwright.config.ts b/apps/web/e2e/playwright.config.ts index 280438071f..f4e7dbe167 100644 --- a/apps/web/e2e/playwright.config.ts +++ b/apps/web/e2e/playwright.config.ts @@ -58,6 +58,9 @@ export default defineConfig({ // ApiClient). Serial within the file; afterAll restarts to baseline. name: "auth", testMatch: /auth\/.*\.spec\.ts/, + // mobile-users-* specs under tests/auth/ run in the mobile-chrome + // project (Pixel 5) instead of here. + testIgnore: /mobile-.*\.spec\.ts/, use: { ...devices["Desktop Chrome"] }, }, { diff --git a/apps/web/e2e/tests/auth/mobile-users-self-actions.spec.ts b/apps/web/e2e/tests/auth/mobile-users-self-actions.spec.ts new file mode 100644 index 0000000000..856d7eb603 --- /dev/null +++ b/apps/web/e2e/tests/auth/mobile-users-self-actions.spec.ts @@ -0,0 +1,131 @@ +import { devices, expect } from "@playwright/test"; +import path from "node:path"; +import { backendFixture as test } from "../../fixtures/backend"; +import { login, setupAdmin } from "../../helpers/auth"; +import { responseUserId } from "./users-auth-helpers"; + +/** + * Mobile parity for the self-actions guard: the Users page must grey out the + * role/status toggles on an active-admin row when no other active admin + * exists on the Pixel 5 path too. The component and state derivation are + * shared with desktop; this spec proves the rendered disabled state at the + * mobile viewport. + * + * Runs in the `mobile-chrome` project (routed away from the desktop `auth` + * project via its testIgnore) and restarts the worker backend with auth on + * and its own database, mirroring users-self-actions.spec.ts. afterAll + * restarts to the fixture baseline. + */ +const ADMIN = { email: "admin@demo.dev", password: "adminpass123", displayName: "Ada Admin" }; +const MEMBER = { email: "sam@demo.dev", password: "memberpass123", displayName: "Sam Member" }; +const DISABLED_ADMIN = { + email: "carol@demo.dev", + password: "carolpass123", + displayName: "Carol Admin (off)", +}; +const SECOND_ADMIN = { email: "bob@demo.dev", password: "bobpass123", displayName: "Bob Admin" }; + +test.describe.serial("users self-actions guard (mobile)", () => { + test.beforeAll(async ({ backend }) => { + await backend.restart({ + KANDEV_FEATURES_AUTH: "true", + KANDEV_DATABASE_PATH: path.join(backend.tmpDir, "kandev-mobile-self-actions.db"), + }); + }); + + test.afterAll(async ({ backend }) => { + await backend.restart(); + }); + + test("own-row toggles track the last-admin guard on mobile", async ({ browser, backend }) => { + // Manual contexts do not inherit the project's device options; spread + // them explicitly so this spec actually exercises the mobile viewport. + const ctx = await browser.newContext({ + ...devices["Pixel 5"], + baseURL: backend.frontendUrl, + }); + await setupAdmin(ctx, backend.baseUrl, ADMIN); + await login(ctx, backend.baseUrl, ADMIN); + + const meRes = await ctx.request.get(`${backend.baseUrl}/api/v1/auth/me`); + expect(meRes.ok(), await meRes.text()).toBeTruthy(); + const adminId = responseUserId(await meRes.json()); + + const memberRes = await ctx.request.post(`${backend.baseUrl}/api/v1/users`, { + data: { + email: MEMBER.email, + password: MEMBER.password, + display_name: MEMBER.displayName, + role: "member", + }, + }); + expect(memberRes.status(), await memberRes.text()).toBe(201); + const memberId = responseUserId(await memberRes.json()); + + const disabledRes = await ctx.request.post(`${backend.baseUrl}/api/v1/users`, { + data: { + email: DISABLED_ADMIN.email, + password: DISABLED_ADMIN.password, + display_name: DISABLED_ADMIN.displayName, + role: "member", + }, + }); + expect(disabledRes.status(), await disabledRes.text()).toBe(201); + const disabledAdminId = responseUserId(await disabledRes.json()); + expect( + ( + await ctx.request.patch(`${backend.baseUrl}/api/v1/users/${disabledAdminId}`, { + data: { role: "admin" }, + }) + ).status(), + ).toBe(200); + expect( + ( + await ctx.request.patch(`${backend.baseUrl}/api/v1/users/${disabledAdminId}`, { + data: { status: "disabled" }, + }) + ).status(), + ).toBe(200); + + const page = await ctx.newPage(); + // Guard against the manual-context device regression: the assertions + // below must run on the Pixel 5 width (393 CSS px; headless mobile + // emulation trims the height, so only the width is pinned). + expect((await page.viewportSize())?.width).toBe(393); + await page.goto("/settings/system/users"); + const ownRow = page.locator(`[data-user-id="${adminId}"]`); + const memberRow = page.locator(`[data-user-id="${memberId}"]`); + const disabledAdminRow = page.locator(`[data-user-id="${disabledAdminId}"]`); + await expect(ownRow).toBeVisible({ timeout: 15_000 }); + + // Sole active admin: own row disabled; member and disabled-admin rows + // are not gated by the last-admin guard. + await expect(ownRow.getByTestId("users-table-toggle-role")).toBeDisabled(); + await expect(ownRow.getByTestId("users-table-toggle-status")).toBeDisabled(); + await expect(memberRow.getByTestId("users-table-toggle-role")).toBeEnabled(); + await expect(memberRow.getByTestId("users-table-toggle-status")).toBeEnabled(); + await expect(disabledAdminRow.getByTestId("users-table-toggle-role")).toBeEnabled(); + await expect(disabledAdminRow.getByTestId("users-table-toggle-status")).toBeEnabled(); + + // A second active admin lifts the guard on every active-admin row. + const secondRes = await ctx.request.post(`${backend.baseUrl}/api/v1/users`, { + data: { + email: SECOND_ADMIN.email, + password: SECOND_ADMIN.password, + display_name: SECOND_ADMIN.displayName, + role: "admin", + }, + }); + expect(secondRes.status(), await secondRes.text()).toBe(201); + const secondAdminId = responseUserId(await secondRes.json()); + + await page.reload(); + const secondAdminRow = page.locator(`[data-user-id="${secondAdminId}"]`); + await expect(ownRow.getByTestId("users-table-toggle-role")).toBeEnabled(); + await expect(ownRow.getByTestId("users-table-toggle-status")).toBeEnabled(); + await expect(secondAdminRow.getByTestId("users-table-toggle-role")).toBeEnabled(); + await expect(secondAdminRow.getByTestId("users-table-toggle-status")).toBeEnabled(); + + await ctx.close(); + }); +}); diff --git a/apps/web/e2e/tests/auth/users-auth-helpers.ts b/apps/web/e2e/tests/auth/users-auth-helpers.ts new file mode 100644 index 0000000000..0f07293837 --- /dev/null +++ b/apps/web/e2e/tests/auth/users-auth-helpers.ts @@ -0,0 +1,15 @@ +/** + * Shared helpers for the auth user-management E2E specs + * (users-self-actions.spec.ts desktop and mobile-users-self-actions.spec.ts). + */ + +/** Reads `user.id` from an API response body, narrowing the unknown shape. */ +export function responseUserId(body: unknown): string { + if (body && typeof body === "object" && "user" in body) { + const user = body.user; + if (user && typeof user === "object" && "id" in user) { + return String(user.id); + } + } + throw new Error("API response is missing user.id"); +} diff --git a/apps/web/e2e/tests/auth/users-self-actions.spec.ts b/apps/web/e2e/tests/auth/users-self-actions.spec.ts new file mode 100644 index 0000000000..9ac0c0db9a --- /dev/null +++ b/apps/web/e2e/tests/auth/users-self-actions.spec.ts @@ -0,0 +1,150 @@ +import { expect } from "@playwright/test"; +import path from "node:path"; +import { backendFixture as test } from "../../fixtures/backend"; +import { login, setupAdmin } from "../../helpers/auth"; +import { responseUserId } from "./users-auth-helpers"; + +/** + * The Users page must grey out the role/status toggles on an active-admin row + * when no other active admin exists, mirroring the backend's existing + * last-admin guard (ErrLastAdmin -> 409). The backend itself is unchanged: + * self-demotion stays allowed while another active admin exists. + * + * Runs in the `auth` project (backend restarted with auth required). Serial: + * shares the admin and users created inside the single test. + * + * Isolation: the worker-scoped backend fixture and `restart()` preserve the + * SQLite DB, and auth setup is single-shot per database. This spec restarts + * with its own database path so the full auth project stays deterministic in + * either file order (auth-screenshots.spec.ts sets up the same admin email on + * the baseline database). + */ +const ADMIN = { email: "admin@demo.dev", password: "adminpass123", displayName: "Ada Admin" }; +const MEMBER = { email: "sam@demo.dev", password: "memberpass123", displayName: "Sam Member" }; +const DISABLED_ADMIN = { + email: "carol@demo.dev", + password: "carolpass123", + displayName: "Carol Admin (off)", +}; +const SECOND_ADMIN = { email: "bob@demo.dev", password: "bobpass123", displayName: "Bob Admin" }; + +test.describe.serial("users self-actions guard", () => { + test.beforeAll(async ({ backend }) => { + // The features.auth flag turns authentication on (setup mode) and reveals + // the admin surfaces. A per-file database keeps setup single-shot from + // colliding with other auth specs sharing the worker. + await backend.restart({ + KANDEV_FEATURES_AUTH: "true", + KANDEV_DATABASE_PATH: path.join(backend.tmpDir, "kandev-auth-self-actions.db"), + }); + }); + + test.afterAll(async ({ backend }) => { + await backend.restart(); + }); + + test("own-row toggles track the last-admin guard; backend unchanged", async ({ + browser, + backend, + }) => { + const ctx = await browser.newContext({ baseURL: backend.frontendUrl }); + await setupAdmin(ctx, backend.baseUrl, ADMIN); + await login(ctx, backend.baseUrl, ADMIN); + + // The setup helper does not return the user; fetch the admin id. + const meRes = await ctx.request.get(`${backend.baseUrl}/api/v1/auth/me`); + expect(meRes.ok(), await meRes.text()).toBeTruthy(); + const adminId = responseUserId(await meRes.json()); + + // A member row provides the enabled baseline. + const memberRes = await ctx.request.post(`${backend.baseUrl}/api/v1/users`, { + data: { + email: MEMBER.email, + password: MEMBER.password, + display_name: MEMBER.displayName, + role: "member", + }, + }); + expect(memberRes.status(), await memberRes.text()).toBe(201); + const memberId = responseUserId(await memberRes.json()); + + const page = await ctx.newPage(); + await page.goto("/settings/system/users"); + const ownRow = page.locator(`[data-user-id="${adminId}"]`); + const memberRow = page.locator(`[data-user-id="${memberId}"]`); + await expect(ownRow).toBeVisible({ timeout: 15_000 }); + + // Sole active admin: the own row's demote/disable toggles are disabled, + // exactly where the backend would reject the action. + await expect(ownRow.getByTestId("users-table-toggle-role")).toBeDisabled(); + await expect(ownRow.getByTestId("users-table-toggle-status")).toBeDisabled(); + // A member row is never gated by the last-admin guard. + await expect(memberRow.getByTestId("users-table-toggle-role")).toBeEnabled(); + await expect(memberRow.getByTestId("users-table-toggle-status")).toBeEnabled(); + + // Existing guard still enforced at the API: the sole admin cannot + // self-demote. + const selfDemote = await ctx.request.patch(`${backend.baseUrl}/api/v1/users/${adminId}`, { + data: { role: "member" }, + }); + expect(selfDemote.status(), await selfDemote.text()).toBe(409); + + // A disabled admin is NOT an active admin: the guard does not apply to + // its toggles, and counting it must not lift the sole active admin's + // disablement (regression: dropping `status === "active"` in either the + // count or the per-row condition would flip these two assertions). + const disabledRes = await ctx.request.post(`${backend.baseUrl}/api/v1/users`, { + data: { + email: DISABLED_ADMIN.email, + password: DISABLED_ADMIN.password, + display_name: DISABLED_ADMIN.displayName, + role: "member", + }, + }); + expect(disabledRes.status(), await disabledRes.text()).toBe(201); + const disabledAdminId = responseUserId(await disabledRes.json()); + const promote = await ctx.request.patch(`${backend.baseUrl}/api/v1/users/${disabledAdminId}`, { + data: { role: "admin" }, + }); + expect(promote.status(), await promote.text()).toBe(200); + const disable = await ctx.request.patch(`${backend.baseUrl}/api/v1/users/${disabledAdminId}`, { + data: { status: "disabled" }, + }); + expect(disable.status(), await disable.text()).toBe(200); + + await page.reload(); + const disabledAdminRow = page.locator(`[data-user-id="${disabledAdminId}"]`); + await expect(ownRow.getByTestId("users-table-toggle-role")).toBeDisabled(); + await expect(ownRow.getByTestId("users-table-toggle-status")).toBeDisabled(); + await expect(disabledAdminRow.getByTestId("users-table-toggle-role")).toBeEnabled(); + await expect(disabledAdminRow.getByTestId("users-table-toggle-status")).toBeEnabled(); + + // A second active admin lifts the guard on every active-admin row. + const secondRes = await ctx.request.post(`${backend.baseUrl}/api/v1/users`, { + data: { + email: SECOND_ADMIN.email, + password: SECOND_ADMIN.password, + display_name: SECOND_ADMIN.displayName, + role: "admin", + }, + }); + expect(secondRes.status(), await secondRes.text()).toBe(201); + const secondAdminId = responseUserId(await secondRes.json()); + + await page.reload(); + const secondAdminRow = page.locator(`[data-user-id="${secondAdminId}"]`); + await expect(ownRow.getByTestId("users-table-toggle-role")).toBeEnabled(); + await expect(ownRow.getByTestId("users-table-toggle-status")).toBeEnabled(); + await expect(secondAdminRow.getByTestId("users-table-toggle-role")).toBeEnabled(); + await expect(secondAdminRow.getByTestId("users-table-toggle-status")).toBeEnabled(); + + // No new backend behavior: self-demotion is allowed again once another + // active admin exists. + const selfDemoteAfter = await ctx.request.patch(`${backend.baseUrl}/api/v1/users/${adminId}`, { + data: { role: "member" }, + }); + expect(selfDemoteAfter.status(), await selfDemoteAfter.text()).toBe(200); + + await ctx.close(); + }); +}); diff --git a/docs/plans/self-actions-guard/plan.md b/docs/plans/self-actions-guard/plan.md new file mode 100644 index 0000000000..55c8aaae55 --- /dev/null +++ b/docs/plans/self-actions-guard/plan.md @@ -0,0 +1,165 @@ +--- +spec: docs/specs/auth/self-actions-guard.md +created: 2026-08-15 +updated: 2026-08-15 +status: complete +--- + +# Implementation Plan: Self-Actions Guard in System Users + +## Overview + +Settings > System > Users lets an admin change the role or status of any listed +account, including their own. The backend already rejects demoting or disabling +the last active admin (409 `ErrLastAdmin`), but the page shows the toggle +buttons as enabled, so the rejection surfaces only as a toast after the click. +The fix greys out exactly those buttons the backend would reject: on any row +that is an active admin, the role toggle ("Make member") and status toggle +("Disable") are disabled when no other active admin exists. + +This is a UI-only change. The backend keeps its current last-admin guard +verbatim; self-demotion and self-disable remain possible while another active +admin exists, exactly as today. + +## Current behavior + +- Backend: `ensureAnotherAdmin` in + `apps/backend/internal/auth/service_users.go` rejects a change that would + leave zero users with `role = admin` and `status = active` (excluding the + target). `AdminSetRoleStatus` applies it to any target, self included; the + `updateUser` handler documents that self-demotion/disable is allowed when + another active admin remains. This behavior is correct and stays unchanged. +- Frontend: `UserRow` in + `apps/web/components/settings/system/users-table.tsx` renders the role and + status toggle buttons for every row with no guard awareness. The user list + (`useUsersList`) is the same data source the backend guard scans + (`ListUsers`), so the disabled state can be derived from the rendered rows. +- E2E: auth-enabled specs live in `apps/web/e2e/tests/auth/*.spec.ts`, run in + the `auth` Playwright project (`KANDEV_FEATURES_AUTH=true` backend restart; + helpers in `e2e/helpers/auth.ts`). No functional spec exists for the Users + page today (only `auth-screenshots.spec.ts`). + +## Frontend + +- `apps/web/components/settings/system/users-table.tsx`: + - Derive `activeAdminCount` from the loaded users: + `users.filter((u) => u.role === "admin" && u.status === "active").length`, + mirroring `ensureAnotherAdmin` exactly. + - In `UsersTableList` / `UserRow`, disable a row's role and status toggles + when the row is an active admin and `activeAdminCount === 1` + (`isLastActiveAdmin`). This is precisely the set of actions the backend + rejects today (demote or disable of the last active admin); every other + button stays enabled. + - No current-user identity is needed: the guard is target-based, not + caller-based, and the own row is just an active-admin row. + - Keep existing labels and `data-testid` attributes + (`users-table-toggle-role`, `users-table-toggle-status`). Shadcn button + already styles the disabled state + (`disabled:opacity-45 disabled:cursor-not-allowed`). + - No new user-facing copy; no i18n key changes. + +## Tests + +- No backend tests change: the backend is untouched. +- No frontend unit tests: the change is button state in component markup, + which this repo does not unit-test. The behavior is covered by the E2E + spec below plus the existing `TestAdminUserManagement` backend guard tests + (which stay green and prove the guard still exists). + +## E2E + +- New `apps/web/e2e/tests/auth/users-self-actions.spec.ts` in the `auth` + project: serial describe, `backend.restart({ KANDEV_FEATURES_AUTH: "true" })` + in `beforeAll`, restart to baseline in `afterAll` (pattern from + `auth-screenshots.spec.ts`). +- Setup admin + login via `e2e/helpers/auth.ts`; create a member via + `POST /api/v1/users` with the admin context (`context.request` shares the + session cookie). +- Assertions: + - Sole admin (fresh setup): own row (`[data-user-id="{adminId}"]`) has both + toggles `toBeDisabled()`; the member row has both `toBeEnabled()`. + - Existing guard proof: `PATCH /api/v1/users/{self}` with + `{ "role": "member" }` as the sole admin returns 409 `ErrLastAdmin` + (backend unchanged). + - Create a second active admin via the API, reload the page: the own row's + toggles are now `toBeEnabled()` (guard passes; button state tracks it). + - No-new-behavior proof: `PATCH /api/v1/users/{self}` with + `{ "role": "member" }` now returns 200 (self-demotion stays allowed when + another active admin exists). +- Run: `cd apps/web && pnpm e2e:run --project auth + tests/auth/users-self-actions.spec.ts`. + +## Implementation Waves And Parallel Candidates + +Wave 1: + +- [x] [Task 01: Disable last-admin toggles in users table](task-01-frontend-disable-last-admin.md) + +Wave 2: + +- [x] [Task 02: E2E coverage for guard-matched buttons](task-02-e2e-guard-mirror.md) + +Execution is sequential. Task 02 depends on the frontend change from Task 01. + +## Verification + +Focused, per task: + +```bash +cd "$(git rev-parse --show-toplevel)" && make typecheck +cd "$(git rev-parse --show-toplevel)/apps/backend" && go test ./internal/auth/... -run TestAdminUserManagement -count=1 +cd apps/web && pnpm e2e:run --project auth tests/auth/users-self-actions.spec.ts +``` + +Final gate (per AGENTS.md): `make fmt`, then `make typecheck test lint`. E2E +is not part of `make test`; the auth-project spec runs via `make test-e2e` in +CI. + +## Mobile parity + +The change adds a `disabled` attribute to two buttons in the shared +`users-table.tsx` component; layout, scrolling, navigation, and touch targets +are unchanged, and the state derivation is shared markup. Mobile coverage is +still provided: `apps/web/e2e/tests/auth/mobile-users-self-actions.spec.ts` +runs in the `mobile-chrome` project (Pixel 5) with the same per-file isolated +auth database and asserts the sole-admin own row is disabled while member and +disabled-admin rows stay enabled, plus re-enablement after a second active +admin. It is routed out of the desktop `auth` project via its `testIgnore` +(`e2e/playwright.config.ts`) so it runs exactly once, on the mobile viewport. + +## Recorded Results + +- E2E: `cd apps/web && pnpm e2e:run --project auth + tests/auth/users-self-actions.spec.ts` — RED first (sole-admin toggle + enabled, expected disabled), then 1 passed after the component change. +- `make fmt` passed (unrelated pre-existing gofmt drift in four backend files + was restored, not committed). +- `make typecheck` passed. +- `make lint` passed: golangci-lint 0 issues, eslint `--max-warnings 0` + clean, 118 harness files passed, architecture lint passed. +- `go test ./internal/auth/... -run TestAdminUserManagement -count=1` passed + (backend guard untouched). +- `make test` is not fully green in this environment for pre-existing + reasons, all proven unrelated by stashing this change: harness-injected + `KANDEV_VERSION=v0.88.0-dirty` breaks two launcher metadata tests; the host + `~/.profile` references a missing `/tmp/cargo/env` (agentctl config test); + two agentctl process-lifecycle tests are timing-sensitive under host load; + `lib/http-git-server.test.ts` requires a Docker bridge gateway (no Docker + daemon on this host); `test-scripts` needs the `unzip` binary (absent, no + sudo to install). CI has none of these gaps. + +## Risks and limits + +- Button state is a mirror of the last loaded user list, not a live + authorization decision. A stale list (an admin demoted or disabled in + another session since the fetch) can show an enabled button whose request + the backend rejects; the existing 409 `ErrLastAdmin` toast covers that path + unchanged. +- The frontend count must match `ensureAnotherAdmin`'s condition exactly + (`role = admin` AND `status = active`, excluding the target row). Any drift + between the mirror and the guard would show either a clickable-rejected + button or a needlessly disabled one; the E2E asserts both directions + (sole-admin disabled, two-admin enabled). +- The `users` snapshot used for the count is the same `ListUsers` data the + backend guard scans, so the mirror and the guard evaluate the same set + modulo request-time staleness. diff --git a/docs/plans/self-actions-guard/task-01-frontend-disable-last-admin.md b/docs/plans/self-actions-guard/task-01-frontend-disable-last-admin.md new file mode 100644 index 0000000000..2210ead1b9 --- /dev/null +++ b/docs/plans/self-actions-guard/task-01-frontend-disable-last-admin.md @@ -0,0 +1,100 @@ +--- +id: "01-frontend-disable-last-admin" +title: "Disable last-admin toggles in users table" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/auth/self-actions-guard.md" +--- + +# Task 01: Disable last-admin toggles in users table + +## Intent + +Grey out the role and status toggles on any active-admin row when no other +active admin exists, mirroring the backend's existing last-admin guard. No +backend change. + +## Inputs + +- The spec `What` and `Scenarios` sections. +- `apps/web/components/settings/system/users-table.tsx` (`UsersTable`, + `useUsersList`, `UsersTableList`, `UserRow`). +- The backend condition to mirror: + `apps/backend/internal/auth/service_users.go` `ensureAnotherAdmin`: reject + when no user other than the target has `role = "admin"` AND + `status = "active"`. + +## Change + +- In `UsersTable` (or `UsersTableList`), derive + `activeAdminCount = users.filter((u) => u.role === "admin" && u.status === "active").length` + from the loaded list. +- Thread the count (or a per-row `isLastActiveAdmin` boolean) into `UserRow`. +- In `UserRow`, compute + `isLastActiveAdmin = user.role === "admin" && user.status === "active" && activeAdminCount === 1` + and add `disabled={isLastActiveAdmin}` to both the role toggle + (`users-table-toggle-role`) and status toggle (`users-table-toggle-status`). +- Do not read the current user id: the guard is target-based, not caller-based, + and applies to every row identically. +- Keep existing labels, `data-testid` attributes, and styling. The shadcn + button variant already applies `disabled:opacity-45` and + `disabled:cursor-not-allowed`. +- No new user-facing copy; no i18n key changes. No backend files change. + +## Acceptance + +- When the loaded list contains exactly one active admin, that row's role and + status toggles carry the `disabled` attribute; all other rows' toggles do + not. +- When the list contains two or more active admins, no row's toggles are + disabled. +- Member rows and disabled-admin rows are never disabled by this change. +- `make typecheck` and web lint pass; `go test ./internal/auth/... -run + TestAdminUserManagement` stays green untouched (the backend guard is + unchanged). + +## TDD sequence + +Per project convention, React component markup is not unit-tested; the +behavior is asserted by the Task 02 E2E spec (run after this task). Verify by +`typecheck`/lint here and the E2E spec in Task 02. + +## Verification + +```bash +cd "$(git rev-parse --show-toplevel)" && make typecheck +cd "$(git rev-parse --show-toplevel)/apps/backend" && go test ./internal/auth/... -run TestAdminUserManagement -count=1 +``` + +(The behavioral check is `pnpm e2e:run --project auth +tests/auth/users-self-actions.spec.ts` from Task 02.) + +## Dependencies + +None. + +## Parallelism + +`sequential`. Single component file; the E2E task depends on it. + +## Output contract + +Report the files changed, verification results, blockers, and risks. Update +this task and `plan.md` in the same conversation. + +## Results + +- Red: the E2E spec (written before the change) failed as expected on the + sole-admin own row: `expect(locator).toBeDisabled()` received "enabled" for + `[data-user-id="default-user"]` `users-table-toggle-role`. +- Green: `pnpm e2e:run --project auth tests/auth/users-self-actions.spec.ts` + passed 1 test (7.6s) covering the sole-admin disabled state, the member-row + enabled baseline, re-enablement after adding a second admin, and the + unchanged 409/200 self-PATCH backend outcomes. +- `make typecheck` passed; `make lint` passed (eslint `--max-warnings 0`, + golangci-lint 0 issues). +- `go test ./internal/auth/... -run TestAdminUserManagement -count=1` passed + untouched, proving the backend guard is unchanged. +- Only `apps/web/components/settings/system/users-table.tsx` changed. diff --git a/docs/plans/self-actions-guard/task-02-e2e-guard-mirror.md b/docs/plans/self-actions-guard/task-02-e2e-guard-mirror.md new file mode 100644 index 0000000000..fe72977190 --- /dev/null +++ b/docs/plans/self-actions-guard/task-02-e2e-guard-mirror.md @@ -0,0 +1,109 @@ +--- +id: "02-e2e-guard-mirror" +title: "E2E coverage for guard-matched buttons" +status: done +wave: 2 +depends_on: ["01-frontend-disable-last-admin"] +plan: "plan.md" +spec: "../../specs/auth/self-actions-guard.md" +--- + +# Task 02: E2E coverage for guard-matched buttons + +## Intent + +Prove from the browser that the toggle buttons are disabled exactly when the +existing backend last-admin guard would reject the action, and that the +backend behavior itself is unchanged. + +## Inputs + +- The spec `Scenarios` section. +- E2E auth patterns: `apps/web/e2e/tests/auth/auth-screenshots.spec.ts` + (serial describe, `backend.restart({ KANDEV_FEATURES_AUTH: "true" })` in + `beforeAll`, baseline restart in `afterAll`) and + `apps/web/e2e/helpers/auth.ts` (`setupAdmin`, `login`). +- Row locators: `users-table-row` rows carry `data-user-id`; the toggle + buttons carry `users-table-toggle-role` / `users-table-toggle-status`. + +## Change + +New file `apps/web/e2e/tests/auth/users-self-actions.spec.ts`: + +1. Serial describe; `beforeAll` restarts the backend with + `KANDEV_FEATURES_AUTH: "true"`; `afterAll` restarts to baseline. +2. Setup the admin (`setupAdmin`), login the admin context, and create a + member via `POST /api/v1/users` on `context.request` (shares the session + cookie). Capture the admin and member ids. +3. Sole-admin UI: open `/settings/system/users`; own row + `[data-user-id="{adminId}"]` has both toggles `toBeDisabled()`; the member + row has both `toBeEnabled()`. +4. Existing-guard proof: `PATCH /api/v1/users/{adminId}` with + `{ "role": "member" }` as the sole admin returns 409 (backend unchanged). +5. Create a second active admin (`POST /api/v1/users`, `role: "admin"`); + reload the page; the own row's toggles are now `toBeEnabled()`. +6. No-new-behavior proof: `PATCH /api/v1/users/{adminId}` with + `{ "role": "member" }` now returns 200 (self-demotion still allowed when + another active admin exists). + +## Acceptance + +- The spec passes with the production build (the managed runner builds it). +- Test discovery: `pnpm e2e:run --project auth + tests/auth/users-self-actions.spec.ts` runs exactly the new tests. + +## TDD sequence + +1. Write the spec (RED: buttons are enabled for the sole admin today). +2. Run it via the auth project; confirm the failures are the expected + assertions. +3. With Task 01 landed, re-run; confirm green. + +## Verification + +```bash +cd "$(git rev-parse --show-toplevel)/apps/web" && pnpm e2e:run --project auth tests/auth/users-self-actions.spec.ts +``` + +## Dependencies + +Task 01 (disabled-button state). + +## Parallelism + +`sequential`. The spec asserts the Task 01 change and the unchanged backend. + +## Output contract + +Report the spec path, the red and green run results (with the exact command), +blockers, and risks. Update this task and `plan.md` in the same conversation. + +## Results + +- Spec: `apps/web/e2e/tests/auth/users-self-actions.spec.ts`, one serial test + in the `auth` project (`backend.restart({ KANDEV_FEATURES_AUTH: "true" })`). +- Red (before Task 01): failed at the sole-admin `toBeDisabled()` assertion on + `users-table-toggle-role` — button was enabled. +- Green (after Task 01): `cd apps/web && pnpm e2e:run --project auth + tests/auth/users-self-actions.spec.ts` → 1 passed (7.6s). +- The spec also proves the backend is unchanged: sole-admin self-PATCH + `{role:"member"}` → 409; after a second active admin, same PATCH → 200. +- Review fixup (luna round 1): `beforeAll` now restarts with a per-file + `KANDEV_DATABASE_PATH` inside the fixture tmpDir. The worker-scoped + fixture and `restart()` preserve the SQLite DB and auth setup is + single-shot, so two auth specs sharing the worker collided regardless of + admin email; the isolated DB keeps the full `--project auth` suite + deterministic in either file order. Coverage widened: a disabled-admin row + is created and asserted enabled while the sole active admin stays disabled + (catches dropping `status === "active"` from the count or the per-row + condition), and both active-admin rows are asserted enabled after the + second admin is added. +- Full auth project run: `pnpm e2e:run --no-build --project auth tests/auth/` + → exit 0, 15 passed (both spec files in one worker). +- Review fixup (luna round 2, mobile parity): added + `apps/web/e2e/tests/auth/mobile-users-self-actions.spec.ts` (runs in the + `mobile-chrome` Pixel 5 project via a `testIgnore` on the desktop `auth` + project) asserting the same disabled/enabled states at the mobile viewport, + plus the shared `users-auth-helpers.ts` used by both specs. Mobile run: + `pnpm e2e:run --no-build --project mobile-chrome + tests/auth/mobile-users-self-actions.spec.ts` → 1 passed (8.1s). diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index 6f78b598d5..5a013b0de0 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -265,6 +265,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance, | [mcp-session-observability](mcp-session-observability/spec.md) | approved | | [subagent-context-persistence](subagent-context-persistence/spec.md) | draft | | [auth](auth/spec.md) | building | +| [auth — self-actions guard](auth/self-actions-guard.md) | shipped | | [create-local-repository](create-local-repository/spec.md) | shipped | | [workflow-cycle-guardrails](workflow-cycle-guardrails/spec.md) | building | | [improve-kandev](improve-kandev/spec.md) | building | diff --git a/docs/specs/auth/self-actions-guard.md b/docs/specs/auth/self-actions-guard.md new file mode 100644 index 0000000000..64be57d373 --- /dev/null +++ b/docs/specs/auth/self-actions-guard.md @@ -0,0 +1,76 @@ +--- +status: shipped +created: 2026-08-15 +--- + +# Self-Actions Guard in System Users + +## Why + +Settings > System > Users lets an admin change the role or status of any listed +account, including their own. The backend already enforces one guard on these +actions: an active admin cannot be demoted or disabled when no other active +admin exists (`ErrLastAdmin` -> 409). The page does not reflect that guard, so +the last active admin can click "Make member" or "Disable" on a row and only +then discover the rejection in an error toast. The buttons should show the +guard up front: greyed out exactly when the backend would reject the action. + +## What + +- On any row that is an active admin, the role toggle ("Make member") and the + status toggle ("Disable") are disabled when no other active admin exists in + the user list. This mirrors the backend's existing `ensureAnotherAdmin` + condition exactly: another user with `role = admin` and `status = active`. +- All other buttons keep their current behavior. "Make admin" on member rows, + "Enable" on disabled rows, and every action on rows where another active + admin exists stay enabled. +- The API behavior is unchanged. The backend keeps its current last-admin + guard for any target, including the caller's own account; self-demotion and + self-disable remain possible when another active admin exists. + +## API surface + +No change. `PATCH /api/v1/users/:id` keeps its current semantics: role/status +normalization, the last-admin guard (409 `ErrLastAdmin`), and session/token +revocation on disable. The guard applies to any target, self included. + +## Permissions + +Unchanged. The route stays admin-only (`RequireRealIdentity` + +`RequireAdmin`). + +## Failure modes + +- Button state is computed from the last loaded user list. If the list is + stale (an admin was demoted elsewhere since the fetch), the backend still + rejects with 409 and the existing `usersLastAdminGuard` toast appears; the + guard is authoritative at request time. +- No new failure mode is introduced on the backend. + +## Scenarios + +- **GIVEN** the signed-in admin is the only user with `role = admin` and + `status = active`, **WHEN** the Users page renders, **THEN** the own row's + role toggle ("Make member") and status toggle ("Disable") are disabled; no + other row is affected. +- **GIVEN** the only active admin, **WHEN** the page shows another row (a + member or a disabled admin), **THEN** that row's toggles remain enabled + (the guard only fires on active admins). +- **GIVEN** at least one other user with `role = admin` and `status = active`, + **WHEN** the Users page renders any active-admin row (including the own + row), **THEN** both toggles on that row are enabled. +- **GIVEN** the last active admin calls `PATCH /api/v1/users/{self}` with + `{ "role": "member" }` or `{ "status": "disabled" }`, **THEN** the API + responds 409 `ErrLastAdmin` (existing behavior, unchanged). +- **GIVEN** an active admin with another active admin present calls + `PATCH /api/v1/users/{self}` with `{ "role": "member" }`, **THEN** the API + applies the change (existing behavior, unchanged). + +## Out of scope + +- No backend change: no new self-action rejection, no signature changes, no + new error codes or messages. +- No change to the last-admin guard itself. +- No change to invite creation, member restrictions, or the account page. +- Button disablement only mirrors the existing guard; it is not a new + authorization rule.