PoliNetwork Auth — RBAC stack review

What PR #4 built, what was wrong with it, and what PR #6 changed. Reviewed 17 Sep 2026.

The stack

maincurrent production

←

#4 · toto04/rbacthe feature: roles & permissions

←

#6 · codex/rbac-audit-fixesthe fixes, stacked on #4

#6 is branched off #4, not off main. It cannot be merged on its own. The order is: merge #6 into #4, then merge #4 into main, then deploy once.

1. What #4 does

#4 replaces the old hard-coded admin check with a real permission system. It adds three things:

A database of roles and permissions. Roles can inherit from other roles; permissions can imply other permissions. Both graphs live in role, permission, role_parent, permission_implication, user_role.
An admin UI under /access to create roles and permissions and give them to people, plus /applications for OAuth clients.
Permission-based gates everywhere. Seven built-in idp:* permissions (read/write for roles, permissions and applications, plus people search) now decide who can do what, and they are handed out through ordinary roles.

On top there is one role outside the database: Master Admin. It holds every permission that exists, including ones created later, and it is granted by deployment configuration rather than by an administrator. That is the bootstrap path, so a mistake in the role graph cannot lock the service out of itself.

The design is good. The problems are in how it was enforced.

2. The three structural problems

Problem 1

Authorization only happened at the front door

In #4, every repository function took no actor: saveRole(draft), assignRole(roleId, userId, assignedBy), listRoleMembers(roleId). The only check was the HTTP route asking "do you have idp:roles:write?" before calling them.

That check also read from loadCatalog(), which had a time-based cache. So the answer could be stale, and nothing re-checked it once the write started.

Problem 2

Having a write permission meant having every permission

Nothing limited what a writer could put into a role. Someone with "Manage roles" could create a role carrying idp:applications:write — or any other permission — and assign it to themselves. One step, no extra privileges needed. The same applied to "Manage permissions" through implications.

Problem 3

Read-then-write with no lock, in several places

Student verification attempt counting, the email resend cooldown, account unlinking and role graph authorization all did "read a value, decide, write a value" without a transaction or lock. Concurrent requests each read the old value and each wrote as if they were first.

3. Every concrete failure, and the fix

What could go wrong in #4	What it meant in practice	Fixed in #6 by
Any linked Microsoft account was a full admin src/auth/oidc-admin.ts	`decideOidcAdmin` ended with `if (!groupConfigured) return true;`. `PN_ENTRA_OIDC_ADMIN_GROUP_ID` is commented out in `.env.example`, so on a default deployment every person who had ever linked a PoliNetwork Entra account held Master Admin — every permission there is. They could then sign in with Google or a passkey and still have it, because the check only looks at linked accounts, not how you logged in.	No group and no allowlist now means nobody is admin. `scripts/security-config.mjs` stops the container at startup if neither is configured.
Old "inherit from Master Admin" rows still worked src/auth/rbac.ts	#4 stopped new roles from naming Master Admin as a parent, but `resolveAccess` still followed such an edge if the row already existed. Any role created before that guard landed would still grant everything.	Resolution ignores the edge, migration `0007` records then deletes existing ones, and a database trigger rejects new ones.
"Manage roles" = full admin src/auth/rbac-delegation.ts	Create a role holding any permission, assign it to yourself, reload. Done.	Every change compares the whole graph before and after. You can only hand out permissions you already hold, and you cannot edit built-in roles or roles more privileged than yours.
Permission check ran outside the write src/auth/rbac-store.ts	Someone being revoked right then could still push a change through, and two admins writing at once each validated against a stale copy of the graph.	`withAuthorizedRbacWrite` takes the advisory lock, re-reads the graph, re-authorizes the actor, then writes — all in one transaction.
Access survived removal from a group for 24 hours src/auth/identity-subject.ts	Group membership was stored with `PN_ENTRA_MEMBER_REFRESH_HOURS` (24 by default) and only rechecked once expired. Remove someone from Soci, Direttivo or the admin group and they kept the access for up to a day.	Group facts are rechecked against Microsoft with a 60-second maximum, measured from when the lookup started. A failed or slow lookup grants nothing.
Evidence from the wrong issuer counted src/auth/identity-subject.ts	The join between `account` and `identity_evidence` did not match on provider, and nothing checked that the evidence came from the currently configured tenant. Old evidence kept conferring membership after a tenant change.	The join now matches provider too, and issuer must equal the configured tenant's issuer.
5 student code guesses became unlimited src/auth/student-verification.ts	`confirmStudentVerification` read `attempts`, then wrote `attempts + 1`, with no transaction. Send 20 requests at once: they all read 0 and all write 1. A correct code could also be redeemed twice.	Read, counter, code consumption and evidence write all happen in one serialized transaction. Failed attempts commit their counter before returning the error.
The email cooldown could be reset src/auth/student-verification.ts	A failed confirmation deleted the challenge row, and that row held the "last sent at" timestamp used for the one-minute resend limit. Fail a code on purpose, send another email immediately. Concurrent resends also both passed the check.	The row is kept and blanked instead of deleted, and the whole request/check/write runs under the lock.
Write-only actors got the whole member list src/routes/api/rbac/role-members.ts	The POST replied with `listRoleMembers(roleId)` — every member's name and email — even for someone who only had write and not read.	The write replies `{ changed: true }`. Reading members is authorized separately, inside the repository too.
Member lists stopped at 500, silently src/auth/rbac-store.ts	A hard `.limit(500)`. A larger role just lost people from the UI with no warning.	Cursor pagination, 100 per page, with a 505-person regression test.
No record of who changed what drizzle/0007_mushy_the_fury.sql	Grant yourself a role, use it, remove it. The live row was the only evidence, and deleting it erased the trail.	An `rbac_audit_event` table written in the same transaction as the change. A trigger rejects UPDATE, DELETE and TRUNCATE. If the audit write fails, the change rolls back.
The app rebuilt its own permission rows at runtime src/auth/rbac-store.ts	`ensureManagedRecords()` re-inserted built-in roles, permissions and their default implications. If an operator deliberately removed one, a restart brought it back.	Seeding is migrations only. Authorization never writes.
An unset plugin hook defaulted to allow src/auth/index.ts	`resourcePrivileges` was not set on the OAuth provider plugin, and its default permits any session. Not reachable over HTTP (those endpoints are server-only), but open to any internal caller.	Explicitly `() => false`, plus denial logging on the client-privileges hook.
The UI offered controls the server would reject src/components/rbac/, src/routes/applications/	Delegated admins saw edit buttons for things they had no authority over, and read-only viewers saw Delete, Rotate secret and New application on the applications pages.	Forms go read-only, grant lists only offer what you can hand out, and write-only controls are hidden. The server still validates everything.
Deploy order was not documented README.md	Migration `0005` drops `identity_evidence.state`, which the old running version still reads. A rolling deploy would break the old replicas mid-migration.	README now says: stop old replicas, migrate, start new. Rollback needs the backup and the old image.

4. How one admin request flows, before and after

#4 — "Save this role"

Route asks: do you have idp:roles:write?
Answer comes from a cached catalog.
Yes → call saveRole(draft), which knows nothing about who you are.
Open a transaction, take the lock, re-read the graph, check for cycles.
Write.

Nothing checks whether the role you just wrote gives you more than you had.

#6 — "Save this role"

Route asks the same question (fast rejection).
Refresh group membership from Microsoft, outside any transaction.
Open a transaction, take the lock.
Re-read who you are and the whole graph inside it.
Re-check idp:roles:write against that fresh state.
Apply the change, then compare the graph before and after: does anything now grant more than you hold?
Write the audit event. If any step fails, the whole thing rolls back.

5. Before you deploy

Set either PN_ENTRA_OIDC_ADMIN_GROUP_ID (with full PN Entra tenant, client ID and secret) or a non-empty IDP_ADMIN_USER_IDS. With neither, the container refuses to start — this is deliberate, it is what stops the fail-open admin bug.
Stop the old replicas before migrating. Migration 0005 removes a column they still read.
Apply migrations 0004 through 0007. The normal start command does this.
Tell existing role admins they are no longer root. They now need Master Admin to edit built-in roles and permissions, to grant a capability they do not already hold, or to rename a permission.
The PN_ENTRA_MEMBER_REFRESH_HOURS setting now only affects stored sign-in evidence. It cannot extend permissions.

6. Verification

Run against the tip of #6 (2d84c28) during this review:

pass Format, lint and type check — 140 files, no warnings.
pass Test suite against a disposable PostgreSQL 17 — 136 passed, 0 failed. That includes 26 integration tests that drive real SQL, transactions and triggers: self-escalation attempts, concurrent code guessing, cross-tenant evidence, audit tamper attempts, cross-pool client writes, and a blocked Microsoft Graph lookup running alongside an unrelated write.
pass Production build with no environment variables set — the new startup validation does not break CI or the Docker build.
pass Startup guard checked both ways: no admin config → the process exits before touching the database; allowlist set → it starts.

7. Findings from this review

No code changes. I did not find a correctness or security defect in #6 worth changing, and I checked the earlier automated review findings on both PRs individually — all of them are addressed in the current code. Two things are worth knowing but are design tradeoffs, not bugs, so I left them alone:

Audit events store the full member list

Adding or removing one person from a role writes the role's entire assignment list twice (before and after) into rbac_audit_event, which is append-only and cannot be pruned by the application. For a role with a few thousand members this is a few hundred KB per click. It is the right behaviour for deleting a role — you want to know who lost access — and only wasteful for single-person changes. Worth revisiting if a role ever gets large.

Identity is resolved twice per request

The route guard resolves your permissions, then the repository resolves them again inside the transaction. That is roughly 22 queries for one catalog read. The second resolution is the actual security fix — it is what closes the stale-authorization gap — so it cannot be removed. It is fine for an admin UI, but it is the first thing to look at if these pages ever feel slow.