RBAC: roles, permissions, admin UI, privacy page, and MCP context - #596
Merged
Merged
Conversation
Ship /privacy as an unauthenticated page describing per-account data storage, admin visibility boundaries, and deployment operator caveats per the RBAC proposal. Link from login, signup, and account pages; add docs/use/privacy.md; extend smoke E2E to assert the page renders without authentication.
- Add D1 migration 0043-rbac.sql with roles, permissions, and seed data - Add typed permission registry and DB/server guard utilities - Load roles/permissions on authenticated user and /session payload - Assign user role at signup; seed script --admin flag - Include user_roles in account deletion cascade
Extend mcpUserContextSchema with optional roles/permissions arrays. Load fresh RBAC data in mcp-auth when building McpCallerContext via users email lookup and getUserRolesAndPermissions. Add requireMcpUserWithPermission helper for future admin capabilities. Includes unit tests for schema, context construction, and permission guard.
Implement /admin/users and /admin/roles with server shells, JSON APIs, client routes, role assignment/removal with last-admin guardrail, audit logging, privacy-boundary shape tests, and Playwright E2E coverage.
Avoid /auth signup rate limits in shared E2E runs by seeding fixture users through the wrangler D1 wrapper and using login-only auth.
Add present-tense authorization.md covering the shipped RBAC model, guards, admin routes, MCP context, privacy boundary, and first-admin bootstrap. Add .agents/skills/rbac/SKILL.md for copy-paste guard patterns. Update project-intent.md and primitives.yaml with the account-administration exception to per-user isolation. Link from architecture index and authentication.md. Delete the RBAC proposal now that behavior is documented.
The default page size of 20 hides newly seeded users when the E2E database accumulates accounts from prior runs. Request pageSize=100 so the spec remains stable on shared local E2E state.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds RBAC roles and permissions across worker logic, client routes, MCP context handling, migrations, tests, docs, and seed tooling. It also adds admin user/role management, privacy pages, last-admin protection, and session propagation of roles and permissions. ChangesRBAC Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant AdminUI as AdminUsersRoute
participant Handler as createAdminUsersApiHandler
participant Permissions as permissions-server
participant Db as permissions-db
participant Audit as audit-log
AdminUI->>Handler: POST /admin/users.json {action, userId, role}
Handler->>Permissions: requireUserWithPermission('update:user:any')
Handler->>Db: assignUserRole/removeUserRole
Db-->>Handler: updated role rows
Handler->>Audit: logAuditEvent(category:'admin')
Handler-->>AdminUI: refreshed user list payload
sequenceDiagram
participant Client
participant McpAuth as mcp-auth.ts
participant ContextBuilder as buildMcpUserContextFromGrantProps
participant Db as permissions-db
Client->>McpAuth: MCP request with bearer token
McpAuth->>ContextBuilder: buildMcpUserContextFromGrantProps(env, grantProps)
ContextBuilder->>Db: getUserRolesAndPermissions(userId)
Db-->>ContextBuilder: roles, permissions
ContextBuilder-->>McpAuth: McpUserContext
McpAuth->>Client: createMcpCallerContext(mcpUser)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kody-bot
marked this pull request as ready for review
July 3, 2026 05:24
cursor
Bot
changed the title
This was referenced Jul 3, 2026
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
🔎 Preview deployed: https://kody-pr-596.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
packages/worker/src/app/handlers/privacy.ts (1)
6-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet a page title for the Privacy route.
Other new handlers in this PR (e.g. admin users) pass a contextual
titletoLayout, but this one leaves the default'kody'title, so the browser tab won't reflect the Privacy page.✏️ Proposed fix
export const privacy = { middleware: [], async handler() { - return render(Layout({})) + return render(Layout({ title: 'Privacy' })) }, } satisfies Action<typeof routes.privacy>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/handlers/privacy.ts` around lines 6 - 11, The Privacy route handler currently renders Layout without a contextual title, so it falls back to the default app title. Update the privacy handler’s render call to pass an appropriate Privacy page title into Layout, following the same pattern used by the other handlers in this PR (for example the admin users handler).e2e/playwright-utils.ts (1)
90-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate signup/login-fallback logic across fixtures.
The signup-with-409-fallback-to-login flow here largely duplicates the logic already in
insertNewUser(Lines 45-65). Consider extracting a shared helper (e.g.signupOrLogin(request, { email, username, password, mode })) used by both fixtures to avoid drift between the two copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/playwright-utils.ts` around lines 90 - 140, The signup-with-409-fallback-to-login logic in the `login` fixture duplicates the existing flow in `insertNewUser`, which risks the two paths drifting apart. Extract the shared request handling into a common helper such as `signupOrLogin` that encapsulates the `/auth` POST, 409 check, fallback login, and error handling, then have both `login` and `insertNewUser` call that helper with the appropriate options.packages/worker/src/app/handlers/session.ts (1)
33-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicate userId/roles-loading logic vs.
readAuthenticatedAppUser.This handler re-implements the same session→userId→user-record→roles/permissions pipeline that
authenticated-user.ts'sreadAuthenticatedAppUseralready centralizes. Consider reusing that helper here (adjusting for the cookie-destroy-on-missing-user branch) to keep the RBAC payload contract in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/handlers/session.ts` around lines 33 - 61, The session handler is duplicating the session-to-user lookup and RBAC loading flow already centralized in readAuthenticatedAppUser. Refactor the logic in the session handler to reuse readAuthenticatedAppUser for resolving the user, roles, and permissions, and keep only the missing-user branch that destroys the auth cookie before returning jsonResponse. Use the existing symbols readAuthenticatedAppUser, getUserRolesAndPermissions, and destroyAuthCookie to consolidate the payload contract in one place.packages/worker/src/app/permissions-db.ts (1)
30-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHardcoded role-name filter duplicates the RoleName vocabulary.
roleSetis filtered against literal'user'/'admin'strings whilepermissionSetaccepts any row unconditionally. If a role is ever added without updating this check, its permissions would still be granted but its name silently dropped fromroles, creating an inconsistent{roles, permissions}payload used downstream by session/MCP context.♻️ Suggested fix using the shared vocabulary
-import { type PermissionString, type RoleName } from '`#app/permissions.ts`' +import { + roleNames, + type PermissionString, + type RoleName, +} from '`#app/permissions.ts`'for (const row of result.results ?? []) { - if (row.role_name === 'user' || row.role_name === 'admin') { + if ((roleNames as ReadonlyArray<string>).includes(row.role_name)) { roleSet.add(row.role_name) } permissionSet.add(formatPermissionString(row)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/permissions-db.ts` around lines 30 - 37, The role filtering in the permissions mapping is hardcoded to literal user/admin strings, which can drift from the shared RoleName vocabulary. Update the logic in the result loop to use the RoleName source of truth already used by the type system or shared constants instead of inline string checks, so any valid role is consistently added to roleSet while permissionSet remains unchanged.packages/worker/src/app/authenticated-user.ts (1)
53-65: 🚀 Performance & Scalability | 🔵 TrivialPer-request roles/permissions fetch adds a second DB query to every authenticated request.
This is consistent with the PR's "fresh per-request role/permission loading" design, but since
readAuthenticatedAppUserbacks most guards/admin routes/MCP context, it's worth keeping an eye on latency/DB load as traffic grows (e.g. via caching with short TTL or batching the user lookup and roles/permissions query into one round trip).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/authenticated-user.ts` around lines 53 - 65, The authenticated-user path is doing a separate roles/permissions lookup for every request, which adds extra DB load in the readAuthenticatedAppUser flow. Update readAuthenticatedAppUser/getUserRolesAndPermissions to avoid the extra round trip by either batching the user and role/permission fetch into one query or adding a short-lived cache for roles/permissions keyed by user/session. Keep the existing return shape from readAuthenticatedAppUser intact.packages/worker/src/app/handlers/admin-roles.ts (1)
24-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate admin-page guard/render boilerplate.
This handler's body (role check → session read → redirect-to-login → render Layout → set-cookie) is near-identical to
createAdminUsersHandlerin admin-users.ts (lines 65-88). Consider extracting a sharedrenderAdminPage({ request, env, title })helper to avoid drift between the two as more admin pages are added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/handlers/admin-roles.ts` around lines 24 - 47, The admin page handler logic is duplicated between createAdminRolesHandler and createAdminUsersHandler, including the role check, auth session lookup, login redirect, Layout rendering, and Set-Cookie handling. Extract that shared flow into a reusable helper such as renderAdminPage({ request, env, title }) and have createAdminRolesHandler call it so the admin page handlers stay consistent as more pages are added.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributing/architecture/authorization.md`:
- Around line 56-65: Clarify the RBAC table ownership wording in the
authorization docs so it does not imply all four D1 tables are keyed on
users.id. Update the table description near the
roles/permissions/role_permissions/user_roles section to state that only
user_roles is account-scoped to the session identifier, while roles,
permissions, and role_permissions are global/shared tables. Reference the
existing RBAC table names and the users.id mention to keep the wording precise.
In `@packages/worker/src/app/handlers/admin-users.ts`:
- Around line 321-355: The last-admin guard in admin-users is not atomic because
countUsersWithRole, loadRolesByUserIds, and removeUserRole run as separate DB
calls, so concurrent remove_role requests can bypass the check. Update the
remove flow in the relevant handler to enforce the invariant inside one
transactional write or a single conditional database operation, using the
existing removeUserRole path and the admin guard logic together so only one
request can succeed when the target is the final admin.
In `@packages/worker/src/app/handlers/auth.ts`:
- Line 7: The signup handler in auth.ts leaves a partial account if
assignUserRole fails because it is awaited without the same defensive handling
used around db.create. Update the auth flow around assignUserRole so failures
are caught, logged/audited, and handled before proceeding to cookie issuance or
success return; use the existing handler context and symbols like assignUserRole
and the user record id to either retry/self-heal or surface a hard failure. If
the role assignment cannot succeed, ensure the created user is not left as a
permission-less orphan, and emit an audit event for the failure path.
In `@packages/worker/src/mcp-auth.ts`:
- Around line 115-126: handleMcpRequest currently lets
buildMcpUserContextFromGrantProps throw during D1 lookup, which can fail the
entire MCP auth flow. Wrap the mcp user-context lookup in a try/catch around
buildMcpUserContextFromGrantProps and decide on a safe fallback path. If lookup
fails, either fall back to the grant-props user when possible or surface a clear
5xx response, and keep the downstream remote connector lookup and
createMcpCallerContext flow working off the resolved mcpUser.
In `@tools/seed-test-data.ts`:
- Around line 238-240: The seeding log in the test-data script is printing the
account email address, which exposes PII in stdout. Update the logging around
the seeded account message in the seed logic to stop including options.email,
while keeping the local/remote and admin context if needed. Use the existing
seed flow in tools/seed-test-data.ts and the console.log statement that reports
the seeded account to locate the change.
---
Nitpick comments:
In `@e2e/playwright-utils.ts`:
- Around line 90-140: The signup-with-409-fallback-to-login logic in the `login`
fixture duplicates the existing flow in `insertNewUser`, which risks the two
paths drifting apart. Extract the shared request handling into a common helper
such as `signupOrLogin` that encapsulates the `/auth` POST, 409 check, fallback
login, and error handling, then have both `login` and `insertNewUser` call that
helper with the appropriate options.
In `@packages/worker/src/app/authenticated-user.ts`:
- Around line 53-65: The authenticated-user path is doing a separate
roles/permissions lookup for every request, which adds extra DB load in the
readAuthenticatedAppUser flow. Update
readAuthenticatedAppUser/getUserRolesAndPermissions to avoid the extra round
trip by either batching the user and role/permission fetch into one query or
adding a short-lived cache for roles/permissions keyed by user/session. Keep the
existing return shape from readAuthenticatedAppUser intact.
In `@packages/worker/src/app/handlers/admin-roles.ts`:
- Around line 24-47: The admin page handler logic is duplicated between
createAdminRolesHandler and createAdminUsersHandler, including the role check,
auth session lookup, login redirect, Layout rendering, and Set-Cookie handling.
Extract that shared flow into a reusable helper such as renderAdminPage({
request, env, title }) and have createAdminRolesHandler call it so the admin
page handlers stay consistent as more pages are added.
In `@packages/worker/src/app/handlers/privacy.ts`:
- Around line 6-11: The Privacy route handler currently renders Layout without a
contextual title, so it falls back to the default app title. Update the privacy
handler’s render call to pass an appropriate Privacy page title into Layout,
following the same pattern used by the other handlers in this PR (for example
the admin users handler).
In `@packages/worker/src/app/handlers/session.ts`:
- Around line 33-61: The session handler is duplicating the session-to-user
lookup and RBAC loading flow already centralized in readAuthenticatedAppUser.
Refactor the logic in the session handler to reuse readAuthenticatedAppUser for
resolving the user, roles, and permissions, and keep only the missing-user
branch that destroys the auth cookie before returning jsonResponse. Use the
existing symbols readAuthenticatedAppUser, getUserRolesAndPermissions, and
destroyAuthCookie to consolidate the payload contract in one place.
In `@packages/worker/src/app/permissions-db.ts`:
- Around line 30-37: The role filtering in the permissions mapping is hardcoded
to literal user/admin strings, which can drift from the shared RoleName
vocabulary. Update the logic in the result loop to use the RoleName source of
truth already used by the type system or shared constants instead of inline
string checks, so any valid role is consistently added to roleSet while
permissionSet remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d941bd4-b9ac-4c92-9676-603af35dc6ac
📒 Files selected for processing (52)
.agents/skills/rbac/SKILL.mddocs/contributing/architecture/authentication.mddocs/contributing/architecture/authorization.mddocs/contributing/architecture/index.mddocs/contributing/architecture/primitives.yamldocs/contributing/index.mddocs/contributing/project-intent.mddocs/use/index.mddocs/use/privacy.mde2e/admin-rbac.spec.tse2e/d1-utils.tse2e/playwright-utils.tse2e/smoke.spec.tspackages/shared/src/chat.node.test.tspackages/shared/src/chat.tspackages/worker/client/app.tsxpackages/worker/client/routes/account.tsxpackages/worker/client/routes/admin-roles.tsxpackages/worker/client/routes/admin-users.tsxpackages/worker/client/routes/index.tsxpackages/worker/client/routes/login.tsxpackages/worker/client/routes/privacy.tsxpackages/worker/client/session.tspackages/worker/migrations/0043-rbac.sqlpackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/audit-log.tspackages/worker/src/app/authenticated-user.tspackages/worker/src/app/handlers/admin-roles.node.test.tspackages/worker/src/app/handlers/admin-roles.tspackages/worker/src/app/handlers/admin-users.node.test.tspackages/worker/src/app/handlers/admin-users.tspackages/worker/src/app/handlers/auth-handler.node.test.tspackages/worker/src/app/handlers/auth.tspackages/worker/src/app/handlers/privacy.tspackages/worker/src/app/handlers/session-handler.node.test.tspackages/worker/src/app/handlers/session.tspackages/worker/src/app/permissions-db.tspackages/worker/src/app/permissions-server.node.test.tspackages/worker/src/app/permissions-server.tspackages/worker/src/app/permissions.node.test.tspackages/worker/src/app/permissions.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/mcp-auth-user-context.node.test.tspackages/worker/src/mcp-auth-user-context.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp/capabilities/meta/require-permission.node.test.tspackages/worker/src/mcp/capabilities/meta/require-permission.tspackages/worker/tsconfig-client.jsontools/seed-test-data.node.test.tstools/seed-test-data.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/worker/src/app/handlers/auth-handler.node.test.ts (1)
345-360: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the failed signup does not persist a roleless account.
This failure happens after user creation but before default-role assignment, so the test should also cover cleanup/rollback; otherwise a 500 can still reserve the email with no role.
Proposed test coverage addition
expect(response.status).toBe(500) expect(await response.json()).toEqual({ error: 'Unable to create account.' }) expect(response.headers.get('Set-Cookie')).toBeNull() + expect(context.testDb.users.has('roleless@example.com')).toBe(false)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/handlers/auth-handler.node.test.ts` around lines 345 - 360, The signup failure test in auth-handler.node.test.ts only checks the 500 response and cookie state, but it should also verify cleanup when default role assignment fails after user creation. Update the test around createAuthTestContext and the signup request to assert the account is rolled back or removed (for example by confirming no user record exists for roleless@example.com after the failure), using the existing signup fails when the default user role cannot be assigned case to cover the rollback path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/public/mcp-apps/kody-ui-utils.css`:
- Around line 4-14: The monospace font stack in the CSS custom property is still
violating the Stylelint rule because the literal family names are unquoted.
Update the --font-mono declaration in kody-ui-utils.css so the font-family
tokens SFMono-Regular, Menlo, Monaco, and Consolas are quoted while keeping the
same fallback order, and leave the surrounding font variables unchanged.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/auth-handler.node.test.ts`:
- Around line 345-360: The signup failure test in auth-handler.node.test.ts only
checks the 500 response and cookie state, but it should also verify cleanup when
default role assignment fails after user creation. Update the test around
createAuthTestContext and the signup request to assert the account is rolled
back or removed (for example by confirming no user record exists for
roleless@example.com after the failure), using the existing signup fails when
the default user role cannot be assigned case to cover the rollback path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6fb6631-d22b-478f-aca0-1dcabed91390
📒 Files selected for processing (9)
.agents/skills/rbac/SKILL.mddocs/contributing/architecture/authorization.mdpackages/worker/public/mcp-apps/kody-ui-utils.csspackages/worker/src/app/handlers/admin-users.node.test.tspackages/worker/src/app/handlers/admin-users.tspackages/worker/src/app/handlers/auth-handler.node.test.tspackages/worker/src/app/handlers/auth.tspackages/worker/src/app/permissions-db.tspackages/worker/src/app/permissions-server.ts
✅ Files skipped from review due to trivial changes (1)
- docs/contributing/architecture/authorization.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/worker/src/app/handlers/auth.ts
- packages/worker/src/app/permissions-server.ts
- packages/worker/src/app/handlers/admin-users.node.test.ts
- packages/worker/src/app/handlers/admin-users.ts
…ited signup role failure, doc wording, seed log PII
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6e843e0. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/seed-test-data.ts (1)
196-222: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
--no-admindoesn't revoke an existing admin grant.
buildAccountSeedSqlonly ever adds theadminrole row (INSERT OR IGNORE) whenadminis true; whenadminis false there's no corresponding removal of a pre-existingadminrole for that user. Re-running the seed script with--no-admin(or without--admin) against an account that was previously granted admin leaves it admin, contradicting the documented behavior ("seed the default account without the admin role").🛠️ Proposed fix to revoke admin when not requested
const adminRoleSql = admin ? ` INSERT OR IGNORE INTO user_roles (user_id, role_id) SELECT u.id, r.id FROM users u, roles r WHERE u.email = ${quoteSql(email)} AND r.name = 'admin';` - : '' + : ` +DELETE FROM user_roles +WHERE user_id = (SELECT id FROM users WHERE email = ${quoteSql(email)}) + AND role_id = (SELECT id FROM roles WHERE name = 'admin');`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/seed-test-data.ts` around lines 196 - 222, `buildAccountSeedSql` only adds the admin role and never removes it, so reseeding with `--no-admin` leaves a previously granted admin intact. Update the SQL generation in `buildAccountSeedSql` to explicitly revoke the `admin` role when `admin` is false, while preserving the existing `user_roles` insert logic for the default user role and the current upsert behavior for `users`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/seed-test-data.ts`:
- Around line 267-274: The fixture insertion in seed-test-data.ts currently runs
for every non-matching email, which seeds the companion test account in remote
environments too. Update the conditional around the accounts.push block in the
seed logic so it only adds the `regularTestEmail`/`regularTestUsername` fixture
when `--local` is enabled, keeping the fixed credential out of remote runs.
---
Outside diff comments:
In `@tools/seed-test-data.ts`:
- Around line 196-222: `buildAccountSeedSql` only adds the admin role and never
removes it, so reseeding with `--no-admin` leaves a previously granted admin
intact. Update the SQL generation in `buildAccountSeedSql` to explicitly revoke
the `admin` role when `admin` is false, while preserving the existing
`user_roles` insert logic for the default user role and the current upsert
behavior for `users`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b83e22f5-a023-4921-9249-0f98154ed98f
📒 Files selected for processing (6)
AGENTS.mddocs/contributing/architecture/authorization.mddocs/contributing/getting-started.mddocs/contributing/setup.mdtools/seed-test-data.node.test.tstools/seed-test-data.ts
✅ Files skipped from review due to trivial changes (2)
- docs/contributing/getting-started.md
- docs/contributing/architecture/authorization.md
This was referenced Jul 8, 2026
This was referenced Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Complete RBAC support, Epic Stack-style: users have roles, roles have permissions (
action:entity:accessstrings), and a user's permissions are the union across their roles. Implemented by parallel subagents and merged into this single PR.0043-rbac.sql:roles,permissions,role_permissions,user_roles(integerusers.idFKs, cascade deletes), withuser/adminroles and their permissions seeded viaINSERT OR IGNOREso every environment gets them. Migration0044-rbac-backfill-user-role.sqlbackfills theuserrole for pre-RBAC accounts. Signup assigns theuserrole (and rolls the account back if that fails);user_rolesjoins the account-deletion cascade.permissions.ts(PermissionStringtemplate-literal type, so an unregistered permission is a compile error),permissions-db.ts(getUserRolesAndPermissionssingle-join query),permissions-server.ts(requireUserWithPermission,requireUserWithRole; login redirect when unauthenticated, 403 when unauthorized). Roles load fresh per request — never stored in cookies or OAuth grant props, so revocation is immediate; transient role-query failures fail closed to empty permissions instead of breaking auth./sessionexposesroles/permissionsfor client-side nav gating./admin/users(paginated list, assign/remove roles, audit-logged) and/admin/roles(read-only roles + permissions), following the/account/secretsserver-shell + JSON API pattern. The last-admin guardrail runs inside the DELETE statement itself (removeAdminRolePreservingLastAdmin), so concurrent removals cannot race the deployment down to zero admins. Admin nav link renders only for admins.id, username, email, created_at, updated_at, roles); never secret values/metadata, tokens, values, memories, packages, jobs, email, chat, storage, or connectors. Enforced structurally: the permission vocabulary has no content entities, admin queries touch identity tables only, and a unit shape test pins the payload. Public/privacypage +docs/use/privacy.mddocument the boundary, including the infrastructure-operator caveat.roles/permissionsonmcpUserContextSchema, resolved fresh per request inmcp-auth(with graceful fallback to the base grant identity on D1 errors); newrequireMcpUserWithPermissionhelper. No existing capability is guarded — everything stays own-scoped.node tools/seed-test-data.ts --localnow seedskody@example.comwith theadminrole (opt out with--no-admin) plus a regular companion accountjane@example.com(userrole only, local-only), so both sides of RBAC are testable out of the box. Both fixtures use passwordilikecode. Custom--emailaccounts stay non-admin unless--adminis passed. The script also resolves the worker Wrangler config automatically, fixing the long-standing "cannot resolveAPP_DB" gotcha.docs/contributing/architecture/authorization.mdis the single reference for the RBAC model, guards (with a copy-pasteable handler example), extension steps, privacy boundary, and first-admin bootstrap.primitives.yamlgains therbacprimitive with an amended per-user-isolation invariant, andproject-intent.mdnames the account-administration exception. No separate agent skill — the docs carry the guidance.First-admin bootstrap (manual, once):
Review loop
All Bugbot and CodeRabbit findings were addressed (each has a threaded reply with the fixing commit):
userrole backfill migration for accounts that predate RBACWalkthrough
Full GUI demo: admin login → Admin nav → metadata-only user list → assign/remove admin role → last-admin guardrail error → read-only roles page → public privacy page → logout → non-admin login → no Admin link → 403 on direct
/admin/usersaccess. (Recorded before the fixture rename, so the regular account appears astwix; it is nowjane@example.com/ilikecode.)System recap — adds the rbac primitive (high risk)
Mode: recap · Base:
main· Head:da0c7e2Classification: adds — new RBAC primitive (registry, guards, tables);
primitives.yamlupdated in this PR.Primitives touched
rbacd1-app-db0043-rbac.sql+0044backfillapp-sessions/sessionapp-ui/admin/users,/admin/roles, public/privacymcp-oauthMcpCallerContextSystem map
Invariants
per-user-isolationamended inprimitives.yaml:access='any'is the single, explicitly-guarded exception, limited to account administration (users, roles), never user content. Admin endpoints never touch content tables; a unit shape test pins the admin payload to metadata fields. Respectscompact-mcp-surface: no new MCP tools.Plan vs actual
Shipped per the RBAC proposal with three small drifts:
permissions-db.tssplit out to break a circular import;mcp-auth-user-context.tsextracted for node-testability; E2E gainedseedE2eUserto avoid signup rate limits during parallel test runs. Review loop added the atomic last-admin guard, signup rollback, role backfill migration, and fail-closed role lookups. Follow-ups made the default seed an admin + regular fixture pair, renamed fixtures tojane@example.com/ilikecode, and dropped the agent skill in favor of the authorization doc.Testing
npm run validategreen on this tree (format, lint, typecheck, 370+ unit tests, Playwright E2E incl.admin-rbac.spec.ts, MCP E2E) — CI runs the same gate and is green/sessionroles for admin vs non-admin, metadata-only admin payload, 403 for non-admin, 302 unauthenticated, last-admin guardrail, assign/remove round-trip, pagination-preserving POSTnpm run migrate:localapplies the0044backfill; all pre-existing accounts gain theuserrolenode tools/seed-test-data.ts --localseeds kody (admin) + jane (regular); both logins verified via/sessionwith the newilikecodepassword, and jane gets 403 on/admin/users.jsonSummary by CodeRabbit