v2: RBAC — fixed roles + permissions layer - #151
Conversation
Last-admin guard only checked role==="member"; PATCH to "operator" bypassed it and could zero out an org's admins. Also revert the premature OrgMemberWire/OrgResponse/MeResponse wire widenings (web typecheck breakage) and narrow at the mapping boundaries instead.
Admission now carries orgRole (full admin|operator|member) alongside the binary users.role stamp; userCreateAfter inserts org_members.role from it instead of collapsing to admin/member. Also corrects a stale teams.ts comment claiming operators hold members:manage.
Rail entries and org route guards now gate on the specific OrgPermissionWire each page needs (org:manage/members:manage/ providers:manage/infra:manage) instead of callerRole === "admin", so operators reach Models/GitHub/Sandbox images without General/ Members/Teams. Members role picker gains Operator; badges render all three roles.
yourbuddyconner
left a comment
yourbuddyconner
left a comment
There was a problem hiding this comment.
Sorted by severity.
Blockers — correctness bugs that should land in this PR
sso-role-sync.ts:67— silent demote on absent claim.extractMappedRoledefaults to"member"on a missing/empty claim path; combined withprovisionUserOnEveryLogin: truethis silently downgrades every admin/operator whose next login's claims lack the mapped path (Keycloak scope drift, group removal, case-mismatch inAUTH_OIDC_ROLE_MAP). The last-admin guard only saves the very last admin — 2nd-to-last is stripped without a signal.admin.ts:24— un-migrated global-flag gate. (not in diff — flagging here.)/api/admin/submissions+ force-settle still gate onusers.role === "admin", andprovisioning.ts:63stamps that global flag on every accepted admin invite. Latent in single-org; the sibling team-resources spec makes this cross-tenant escalation the momentorg_membersgrows a second row per user (listAllUnsettledSubmissions()/forceSettleare unscoped by name). The PR body calls this out as "worth a deliberate decision later" — with the highest-blast-radius endpoint on the deprecated axis, that's the wrong pass.sso-role-sync.ts:107— cross-org rewrite. SELECTs org_members foruserIdwith no orgId filter, then writes the same role into every returned row. Sole-admin guard runs per-row so it doesn't catch cross-org bleed.sso-role-sync.ts:109— stale read outside the transaction. The decision to enter the guard branch readsrow.rolefrom an outside-tx SELECT; the count-and-update is atomic, the decision to run the guard isn't.
Important — real bugs, narrower impact
_org-admin.ts:16— requirePermission crashes on undefined user. Internal-token rung sets no user and applies no path allow-list (unlike the sandbox rung). A future internal call landing on an org path 500s instead of 401ing.credentials.test.ts:271— operator coverage gap. The PR's central behavior change (operator tier) is untested on every broadened surface: credentials, providers, GitHub App, image catalog, prebuilds. A future refactor could narrow the grant back with every existing test still green.sso-role-sync.ts:118— silentconsole.error. No audit-log row, no metric, no user-facing signal on refused demotions.members-table.tsx:76— optimistic-update race across the new three-tier UI. Two admins concurrently demoting each other: one succeeds, one 400s, roles diverge across clients until refetch. The old admin/member table masked this by narrower use.
Nits — cleanup / consistency
teams.ts:106— leftoverisOrgAdminDB roundtrip on the list route (line 143 in file). Behavior-identical today but drifts from the RBAC design's "routes never match on role names" binding.credentials.ts:73— inconsistent 403 wire copy. Retains"org admin required"while every other org gate returns"forbidden"; the doc comment cites parity withroutes/org.tsthat no longer holds.
Details inline below.
| return entry.role; | ||
| } | ||
| } | ||
| return "member"; |
There was a problem hiding this comment.
Blocker — silent demote on absent claim. With provisionUserOnEveryLogin: true, returning "member" here whenever the claim path is missing/empty means any Keycloak scope drift or misconfig (roles mapper removed from userinfo, a AUTH_OIDC_ROLE_MAP casing mismatch on line 63's case-sensitive includes(), or a claim of []) silently downgrades every admin/operator whose next login's claims don't traverse roleClaim. The last-admin guard only saves the very last admin — the second-to-last is stripped with no signal.
Distinguish "claim absent" from "claim present but no match": treat an entirely-missing claim path as a hard error (skip the sync, log, keep the current role) and only default to member when the path exists but nothing in it matched. Also normalize case at config load.
| * count-then-update-in-one-transaction posture in services/org.ts. | ||
| */ | ||
| export async function syncSsoOrgRole(db: AppDb, userId: string, role: OrgRole): Promise<void> { | ||
| const rows = await db.select().from(orgMembers).where(eq(orgMembers.userId, userId)); |
There was a problem hiding this comment.
Blocker — cross-org rewrite. This SELECT filters only by userId, not by orgId. Single-org today, but the moment a user is a member of multiple orgs (the team-resources spec's stated direction, staging↔prod shared DB, seeded fixtures crossing envs), one login's IdP claim rewrites every membership row in lockstep. The sole-admin guard on line 117 runs per-row so it doesn't catch the cross-org bleed.
Scope the query and the write to the user's active org (or make the claim map orgId-aware) rather than blanket-scanning org_members on userId.
| export async function syncSsoOrgRole(db: AppDb, userId: string, role: OrgRole): Promise<void> { | ||
| const rows = await db.select().from(orgMembers).where(eq(orgMembers.userId, userId)); | ||
| for (const row of rows) { | ||
| if (row.role === role) continue; |
There was a problem hiding this comment.
Blocker — stale read outside the tx drives the guard decision. row.role here (and on line 112) is read outside the transaction that opens on line 111; the count-and-update is atomic, the decision to enter the guard branch is not.
Concurrent flow: login reads row.role='admin' while a PATCH /org/members/A has just landed setting operator. This branch enters the tx on the stale value; if A was the 2nd-to-last admin the guard skips a write for a role that's already changed. Symmetric case: stale row.role='member' skips the guard entirely on what is actually now the last admin.
Re-SELECT the row inside the tx (or use SELECT ... FOR UPDATE) so the guard decision is over live state.
| .from(orgMembers) | ||
| .where(and(eq(orgMembers.orgId, row.orgId), eq(orgMembers.role, "admin"))); | ||
| if (admins.length <= 1) { | ||
| console.error( |
There was a problem hiding this comment.
Important — no operational signal. console.error scrolls off. If an ops team drops a Keycloak group globally, every admin login triggers this refusal for the next hour and nobody sees it — the org roster still shows the old admins while the IdP-of-record disagrees.
Emit a structured event or bump a counter so ops can alert, and — as buildProvisionUser's doc comment already hints — consider surfacing it on GET /api/org ("IdP wanted to demote you but was refused") so an admin sees the disagreement in the product.
| * Synchronous — permissions were resolved by the auth middleware. */ | ||
| export function requirePermission(permission: Permission) { | ||
| return (c: Context<AppEnv>): Response | undefined => { | ||
| if (!can(c.var.user, permission)) { |
There was a problem hiding this comment.
Important — 500 on unset user. c.var.user is AuthUser in the type but can be undefined at runtime: the auth middleware's internal-token rung (middleware/auth.ts:120-122) calls next() unconditionally, sets no user, and applies no path allow-list (unlike rung 2 which explicitly 403s unknown paths for sandbox tokens). A future memory-adjacent route added under an org path — or a misdirected internal call today — will crash with undefined.permissions.has(...) into the global 500 handler instead of returning 401/403.
Mirror requireUser's pattern: return 401 when user is absent instead of trusting the compile-time type.
| const res = await fetch(`${api.baseUrl}/api/credentials?scope=org`, { | ||
| headers: { "x-valet-test-user-id": "test-admin" }, | ||
| }); | ||
| expect(res.status).toBe(403); // was 200 pre-fix — stale users.role honored |
There was a problem hiding this comment.
Important — operator coverage gap. These new 403 assertions still fire as test-member and pin the pre-PR "org admin required" copy. The PR's central behavior change — the operator tier — is untested on every broadened surface: credentials:org, providers:manage, infra:manage. Seed a test-operator (org_members.role="operator") and assert both the granted (operator succeeds on org credentials) and denied (operator can't hit members:manage surfaces) sides.
Without this, a future refactor could narrow credentials:org back to admin-only or rename the permission with every existing test still green — regression only surfacing when an operator loses the ability to rotate a shared bot token they were trusted with. Same shape gap exists on llm-providers.test.ts, github-app.test.ts, prebuilds.test.ts, image-catalog.test.ts — the org.test.ts pattern (per-surface operator/member matrices) is the fix.
| const user = c.var.user; | ||
| const scope = c.req.query("scope") === "org" ? "org" : "user"; | ||
| if (scope === "org" && user.role !== "admin") { | ||
| if (scope === "org" && !can(user, "credentials:org")) { |
There was a problem hiding this comment.
Nit — wire copy inconsistency (re: ORG_ADMIN_REQUIRED on file line 59). This route still returns { error: "org admin required" } on the three 403 paths (lines 74, 120, 159) while every other org gate migrated to { error: "forbidden" }. Same authorization concept, two different wire shapes.
The doc comment (line 16) still cites "matching routes/org.ts's copy" — a parity claim the diff itself contradicts. Either standardize on "forbidden" here or add a discriminating code field everywhere.
| user: AuthUser, | ||
| ): Promise<boolean> { | ||
| if (await isOrgAdmin(db, user.orgId, user.id)) return true; | ||
| if (user.permissions.has("members:manage")) return true; |
There was a problem hiding this comment.
Nit — leftover isOrgAdmin (re: GET / on file line 143). canMutateTeam and canViewTeam already read from the permission set; the list route below still calls isOrgAdmin(db, ...) — a role-name DB roundtrip, and a second source of truth for a signal the middleware already computed.
Behavior-identical today (only admins hold members:manage), but the RBAC design's binding says routes must never match on role names — this is the last such caller, and dropping it lets services/org.ts:57 isOrgAdmin go too. When custom roles / scoped API principals land (the design's stated next step), this call site silently keeps answering on the literal role string 'admin' while every other gate reads a permission set.
| if (role === "admin") return "accent"; | ||
| if (role === "operator") return "success"; | ||
| return "neutral"; | ||
| } |
There was a problem hiding this comment.
Important — optimistic-update race across the new three-tier UI (re: isSoleAdmin computation and disable logic). isSoleAdmin disables the dropdown only for the last admin, but with the new operator tier there are now three transitions to guard. Combined with useSetOrgMemberRole's optimistic update, two admins acting concurrently can each optimistically demote the other and only one server-side write wins.
A and B both open Members. A picks operator for B — optimistic flip in A's cache (2 admins, guard passes). B picks operator for A — optimistic flip in B's cache. Server-side: A's PATCH lands first, B is now operator; B's PATCH then runs — setOrgMemberRole counts admins=1 and returns { ok:false, reason:last_admin } 400. Rollback restores B's cached state, but roles are now inconsistent across the two clients until refetch. The old admin/member table masked this by rarity; the operator tier makes concurrent demotions more likely.
Either disable any admin-out-of-admin transition until the mutation settles, or pessimistic-refetch before opening the second dropdown.
Conflicts:
- packages/api/src/routes/llm-providers.test.ts: keep both imports
(RBAC adds orgMembers seed for operator-role tests; dev-v2 adds
OPENROUTER_DEFAULT_MODEL_IDS for the openrouter registry expectation).
- packages/web/src/components/settings/settings-rail.tsx: keep RBAC's
per-item permission gating AND dev-v2's single-user-mode MODELS_ITEM
fallback — the two additions are orthogonal.
Post-merge migrations:
- routes/linear-connect.ts (added on dev-v2, still called requireOrgAdmin
which the RBAC pass retired): switch to requirePermission("infra:manage")
to match github-app.ts's workspace-installation surface.
- routes/llm-providers.ts GET /openrouter/models (added on dev-v2): switch
to requirePermission("providers:manage") to match the rest of the file.
Two review-driven follow-ups on the RBAC pass. 1. SSO role-sync silently-demote cascade (finding #2). `syncSsoOrgRole` previously only logged the guarded last-admin case; the N-1 preceding admin demotions in an admins-drift scenario applied without a trace. `extractMappedRole` also collapsed two failure modes ("claim missing" vs "no roleMap match") into an undifferentiated "member" return, so an operator investigating a silent demotion couldn't tell whether the IdP stopped sending the claim or was sending values the map didn't cover. - `extractMappedRole` now returns `{ role, source, matchedClaim?, observedValues }` — `source` is `"matched"` / `"no-claim-values"` / `"no-map-match"`. The caller in `auth/index.ts`'s `buildProvisionUser` logs a warn for each unmatched case naming the observed values. - `syncSsoOrgRole` logs every applied change: `console.warn` for promotions, `console.error` for demotions (both are the head of a possible admin-loss cascade and should be greppable). The existing sole-admin refusal keeps its error log verbatim. 2. API-key scope-intersection seam (finding #3). Before, the API-key branch of the auth middleware stamped `permissions: permissionsForOrgRole(orgRole)` — the owner's full org bundle, ignoring any declared scopes on the key. The RBAC design's binding compatibility note promises scoped keys will "just work" without a per-route retrofit; the seam existed at `can()` but the intersection wasn't wired. - New `effectiveApiKeyPermissions(ownerBundle, scopes)` in `auth/permissions.ts`: null/empty scopes → owner's full bundle (back-compat), non-empty scopes → owner ∩ scopes with unknown strings ignored (forward-compat with additive PERMISSIONS bumps). - New `extractApiKeyScopes(metadata)` in `middleware/auth.ts` reads `metadata.scopes: string[]` off the api-key row. Isolated so the middleware doesn't reach into JSON directly. - Middleware API-key branch now passes both through `effectiveApiKeyPermissions`. Existing unscoped keys keep every permission their owner holds — no route test needs to change. Also fixes stale error-copy assert in linear-connect.test.ts that survived the retirement of `requireOrgAdmin` (expected "org admin required", now the shared `{ error: "forbidden" }`). Tests: sso-role-sync.test.ts asserts the new discriminated result + new logging behavior; permissions.test.ts pins the intersection rules; new middleware/auth.api-key-scopes.test.ts pins the metadata reader. 11 auth+middleware test files (137 tests) pass.
Review follow-ups #4-#7 batched into a single polish pass. No behavior change on any hot path; only removes drift-prone duplication. - wire/types.ts: `OrgPermissionWire` now aliases `Permission` (type-only import from auth/permissions.ts) instead of hand-listing the string union. Adding a permission on the server automatically flows to the web client; the earlier "kept in sync by hand" comment could silently disagree because both were plain string unions. - routes/teams.ts GET /api/teams: swap `isOrgAdmin(db, ...)` DB call for `user.permissions.has("members:manage")`. Matches the same file's `canMutateTeam`/`canViewTeam` seam (three enforcement styles → two; DB-backed → in-memory), removes one round-trip per teams-list request. - components/settings/role-badge.tsx (new): shared `<RoleBadge role={...} />` used by members-table and invites-panel. Both files previously inlined the same three-way ternary for variant + label; a rename of "Operator" now touches one file. - api/settings.ts: new `useHasPermission(perm): boolean` hook. Consumed by settings.profile.tsx and settings.organization.members.tsx, which previously did `orgQ.data?.permissions.includes(...)` inline with slightly different loading/undefined handling. settings-rail.tsx and settings.organization.tsx keep their existing usage — they iterate over the permissions array (rail item filter) or take permission as a prop (guard), where the hook wouldn't simplify. - components/settings/settings-rail.tsx: tighten the `MODELS_ITEM` fallback to fire only when `features.organizations === false` (true single-user mode). Previously fired whenever the Organization group was hidden, including when a member in an org-mode deploy simply lacked permissions — they'd see a Models link they had no access to hit. Test updated to cover both cases. Tests: fixed the pre-existing stale `mockOrg({...})` calls in -settings.test.tsx that omitted the `permissions` field; added `useHasPermission` to the two `vi.mock("~/api/settings")` blocks that don't use `importOriginal`. 451/453 web tests pass; the 2 remaining are pre-existing AppearancePage `localStorage` failures unrelated to RBAC.
…ror) Vitest's jsdom environment defaults `url` to `about:blank`, an *opaque* origin. Per WHATWG storage-partitioning, `window.localStorage` throws `SecurityError` on method invocation there — `readStoredTheme` in `AppearancePage` hits this, and `afterEach(() => window.localStorage. clear())` in `-settings.sections.test.tsx` hits it on teardown. The two appearance tests have been failing since jsdom v22 tightened this. Adding `environmentOptions.jsdom.url = "http://localhost/"` to `vitest.config.ts` didn't propagate reliably to the per-file `// @vitest-environment jsdom` opt-in path. Setup-file shim instead: probe by CALLING `getItem` (property access alone doesn't throw — jsdom throws on invocation), and if that throws, define a plain in-memory Storage on `window`. Production paths are unaffected. 453/453 web tests pass, up from 451/453.
`(perm: string) => permissions?.includes(perm)` compiled under
`pnpm typecheck` (project references) but not under `web-build`'s
`tsc --noEmit`, which is stricter: `.includes()` on a
`readonly OrgPermission[]` rejects a bare `string`. Add the wire type
to both `vi.mock("~/api/settings", ...)` blocks.
Fixes e2e web-build step; individual isolation of orchestrator-loop,
prebuilds, store-postgres, and llm-providers e2e tests all pass —
prior failures were docker-contention flakes when running e2e steps
in parallel.
dev-v2 tip commit 34e8fcf added the `docs-lint` row to `STEPS` (bumping the array to 34) and updated the "includes every spec row id" list at line 42, but missed the two `toHaveLength(33)` asserts at lines 36 and 244. `make e2e` shipped broken on dev-v2. Bump both to 34.
1 participant
Implements docs/specs/2026-07-21-rbac-permissions-design.md. Also carries the approved spec for the follow-on team-resources passes (docs/specs/2026-07-21-team-resources-design.md, spec-only).
What's here
auth/permissions.ts):org:manage,members:manage,providers:manage,infra:manage,credentials:org— binding: these strings are the future OAuth scope vocabulary;can(principal, permission)is typed against a permission-set shape so scoped API keys / OAuth tokens / sandbox principals plug in later with zero route changes. Bundle contents are pinned by test.AuthUserstampsorgRole+permissionsfromorg_members.roleper request;users.roleshrinks back to the/api/adminglobal-operator flag. Fixes the divergence bug where a demoted admin kept org-credential write access via staleusers.role(regression test included).requirePermission(...)per the spec's binding map;requireOrgAdmindeleted. Last-admin guard now covers demote-to-operator (was bypassable). Operator invites redeem toorg_members.role=operatorwhileusers.rolestays binary.AUTH_OIDC_ROLE_MAP=valet-admin:admin,valet-operator:operator+AUTH_OIDC_ROLE_CLAIM(defaultrealm_access.roles); synced on every SSO login via the sso plugin'sprovisionUser. Safety valve: an IdP demotion that would leave zero org admins is refused transactionally + logged; sync errors never brick login. Map unset → byte-identical to today.valet-admin(alice) /valet-operator(bob), verified live via admin API;make dev-keycloakprints the role-map env.Review notes
GET /api/teamsstill usesisOrgAdmin(behavior-identical; the team-resources pass owns teams). Informational: org-admin invites still grant globalusers.role=admin(pre-existing; worth a deliberate decision later); IdP demotion never revokes/api/admin(spec-mandated).Tests
api 1181 ✓ (9 skipped incl. keyless e2e), web 439 ✓, engine ✓, typechecks clean. New suites: permissions bundles, orgRole middleware resolution, per-surface operator/member 403-405 matrices, role-map config parsing, SSO sync (incl. valve + idempotency), permission-driven UI rendering.