Skip to content

feat(rbac): drop the project-scoped Admin role - #377

Merged
nourshoreibah merged 2 commits into
mainfrom
drop-project-admin-role
Aug 25, 2026
Merged

feat(rbac): drop the project-scoped Admin role#377
nourshoreibah merged 2 commits into
mainfrom
drop-project-admin-role

Conversation

@nourshoreibah

@nourshoreibah nourshoreibah commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Follows #374 (now merged); rebased onto main.

Answering the question first: do we branch off that membership anywhere?

Exactly one line. buildSubject in shared/rbac/src/subject.ts checks the membership role against DIRECTOR_ROLES, which was ['Admin', 'Director']. That is the only place in the repo where behaviour depends on a membership role string — DIRECTOR_ROLES has one reference, and everything downstream reads the derived directorProjectIds instead.

isAdmin is never derived from a membership; it comes only from users.is_admin. So a project-scoped Admin row granted exactly what a Director row granted, and nothing else. It was a synonym.

Every other appearance of the role is a vocabulary list, a display pass-through (GET /projects/{id}/overview, the reports PDF/CSV, the staff card), a spec enum, or docs.

Change

  • PROJECT_ROLES['Director', 'Student'], DIRECTOR_ROLES['Director']. Because both the picker's options and validateMembers' allowlist re-export that one list, the UI stops offering Admin and the API starts rejecting it with no further edits.
  • New migration 20260825023851_drop_project_admin_role.sql: rewrites surviving Admin rows to Director, then narrows the CHECK to ('Director','Student').
  • OpenAPI enums, shared/rbac/README.md, and the stale apps/backend/AGENTS.md role list (which still said PI/Accountant/Staff/Admin) updated.

Admin rows are mapped to Director, not to users.is_admin = true. Promoting them would convert a project-scoped role into global privilege — the exact confusion being removed.

Notes on the migration

It is the contract phase that 20260812011405 promised and never delivered — that migration widened the CHECK to six values (Admin, Director, Student, PI, Accountant, Staff) with a comment saying "a later migration drops them", and the live constraint has accepted all six ever since. This narrows it.

Per apps/backend/db/README.md this is a narrowing constraint, so the honest caveat: migrations run against prod before the lambda deploys. In that window the old lambda still accepts role: 'Admin' and would now hit a constraint violation. The deployed frontend has no role picker at all (#374 adds it), so nothing in the product can post that value — only a direct API call in a ~2 minute window could. No destructive-SQL opt-out is needed; the guard matches DROP TABLE/COLUMN/TRUNCATE, not DROP CONSTRAINT.

Verification

  • Migration applied to a scratch Postgres seeded with the pre-migration schema and an Admin row: the row became Director and a subsequent INSERT ... 'Admin' was rejected by project_memberships_role_check. Scratch DB dropped.
  • shared/rbac: 30/30 pass, including two new cases — the vocabulary has no Admin, and a stale Admin membership row yields neither isAdmin nor director.
  • apps/backend/lambdas/projects: new test/member-roles.unit.test.ts, 2/2 pass — every surviving role is accepted, Admin is rejected with 'role' must be one of: Director, Student.
  • apps/frontend: tsc --noEmit clean and 38/38 suites, 367 tests pass after rebuilding the rbac dist, plus a new case asserting the dropdown offers only Director and Student.
  • Not re-run after the final doc edits: npm run build and next lint in apps/frontend. Both were green earlier on this tree and nothing since touched frontend source. CI covers them.

🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

This PR contains a database migration

  • apps/backend/db/migrations/20260825023851_drop_project_admin_role.sql

It will be applied to the production database automatically when this PR merges, before the new lambda code is deployed. Please confirm before requesting review:

  • Applied and tested locally. cd apps/backend && make migrate, then run the affected lambda's tests (cd apps/backend/lambdas/<name> && npm test). make show-migrations shows what applied.
  • Safe for the code that is live right now. During the deploy window -- and indefinitely if the deploy fails -- the currently deployed lambdas run against your new schema. Additive changes (CREATE TABLE, nullable ADD COLUMN, CREATE INDEX) are fine in one PR. DROP COLUMN, renames, ADD COLUMN NOT NULL with no default, and new UNIQUE/CHECK/FOREIGN KEY constraints need two merged PRs -- see the expand/contract rules in apps/backend/db/README.md.
  • Kept separate from unrelated changes. A migration PR should ideally contain the migration, the code that needs it, and nothing else. It changes production state, it is the one thing here that redeploying cannot roll back, and a reviewer should be able to see the whole schema change without scrolling past unrelated work.
  • No already-merged migration was edited. Fix an old migration by adding a new one; there is no down.

shared/types/db-types.d.ts is regenerated and pushed to this branch automatically -- don't hand-edit it. Expect one red migrations-fresh check before that commit lands.

Base automatically changed from worktree-project-member-roles to main August 25, 2026 02:46
@nourshoreibah nourshoreibah added the no-review The PR review bot won't run label Aug 25, 2026
Admin is a user-level flag. `users.is_admin` is the only thing that grants it,
and `buildSubject` never reads it from a membership -- so the `Admin` value in
`project_memberships.role` only ever meant "director of this project", because
`DIRECTOR_ROLES` held both spellings. It was a synonym for Director that the
new staff picker would have offered as if it meant something more.

`PROJECT_ROLES` is now `Director | Student` and `DIRECTOR_ROLES` is just
`Director`. Everything downstream follows from that one list: the picker's
options, `validateMembers`' allowlist, and the two OpenAPI enums.

The migration is the contract phase 20260812011405 promised and never got. It
rewrites the surviving `Admin` rows to `Director` -- which changes nobody's
permissions, for the reason above -- and narrows the CHECK, which had still
been accepting `PI`, `Accountant` and `Staff` a year after they were renamed.
Deliberately not mapped to `users.is_admin`: that would turn a project-scoped
role into global privilege, which is the bug being removed, not a migration.

Verified against a scratch Postgres: Admin rows become Director and the
constraint then rejects an Admin insert.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nourshoreibah
nourshoreibah force-pushed the drop-project-admin-role branch from c95e170 to e46d789 Compare August 25, 2026 02:47
@github-actions

Copy link
Copy Markdown
Contributor

Database Types Check Complete

The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones.

No changes were needed and the type definitions are already up to date.

@nourshoreibah
nourshoreibah marked this pull request as ready for review August 25, 2026 02:47
@github-actions

Copy link
Copy Markdown
Contributor

Database Types Check Complete

The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones.

No changes were needed and the type definitions are already up to date.

These fixtures predate this branch -- they arrived with #372 and only met the
narrowed vocabulary at the rebase. `rbac.test.ts` asserted that a project_id 4
membership makes the caller a director of it, which held only because
DIRECTOR_ROLES used to include Admin; the case is about loadRbacSubject not
issuing a second query, so Director expresses it without relying on the
synonym. The authenticate.test.ts rows are pass-through assertions that were
passing either way, updated so no fixture claims a role the CHECK now rejects.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Database Types Check Complete

The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones.

No changes were needed and the type definitions are already up to date.

@nourshoreibah
nourshoreibah merged commit 05a38c1 into main Aug 25, 2026
21 checks passed
@nourshoreibah
nourshoreibah deleted the drop-project-admin-role branch August 25, 2026 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-review The PR review bot won't run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant